| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <ab559026-7e95-4adc-9978-6db30982b2a6@baylibre.com>
Date: Mon, 14 Oct 2024 16:14:04 -0500
From: David Lechner <dlechner@...libre.com>
To: Angelo Dureghello <adureghello@...libre.com>, Nuno Sá
<nuno.sa@...log.com>, Lars-Peter Clausen <lars@...afoo.de>,
Michael Hennerich <Michael.Hennerich@...log.com>,
Jonathan Cameron <jic23@...nel.org>, Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
<conor+dt@...nel.org>, Olivier Moysan <olivier.moysan@...s.st.com>
Cc: linux-iio@...r.kernel.org, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org, Mark Brown <broonie@...nel.org>
Subject: Re: [PATCH v6 4/8] iio: dac: adi-axi-dac: extend features
On 10/14/24 5:08 AM, Angelo Dureghello wrote:
> From: Angelo Dureghello <adureghello@...libre.com>
>
> Extend AXI-DAC backend with new features required to interface
> to the ad3552r DAC. Mainly, a new compatible string is added to
> support the ad3552r-axi DAC IP, very similar to the generic DAC
> IP but with some customizations to work with the ad3552r.
>
> Then, a serie of generic functions has been added to match with
spelling: series
> ad3552r needs. Function names has been kept generic as much as
> possible, to allow re-utilization from other frontend drivers.
>
> Signed-off-by: Angelo Dureghello <adureghello@...libre.com>
> ---
...
> +static int axi_dac_read_raw(struct iio_backend *back,
> + struct iio_chan_spec const *chan,
> + int *val, int *val2, long mask)
> +{
> + struct axi_dac_state *st = iio_backend_get_priv(back);
> + int err, reg;
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_FREQUENCY:
> +
> + if (!st->info->has_dac_clk)
> + return -EOPNOTSUPP;
> +
> + /*
> + * As from ad3552r AXI IP documentation,
> + * returning the SCLK depending on the stream mode.
> + */
> + err = regmap_read(st->regmap, AXI_DAC_CUSTOM_CTRL_REG, ®);
> + if (err)
> + return err;
> +
> + if (reg & AXI_DAC_CUSTOM_CTRL_STREAM)
> + *val = st->dac_clk_rate / 2;
> + else
> + *val = st->dac_clk_rate / 8;
To get the DAC sample rate, we only care about the streaming mode
rate, so this should just always be / 2 and not / 8. Otherwise
the sampling_frequency attribute in the DAC driver will return
the wrong value when the buffer is not enabled. We never do buffered
writes without enabling streaming mode.
> +
> + return IIO_VAL_INT;
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static int axi_dac_bus_reg_write(struct iio_backend *back, u32 reg, u32 val,
> + size_t data_size)
> +{
> + struct axi_dac_state *st = iio_backend_get_priv(back);
> + int ret;
> + u32 ival;
> +
> + if (data_size == sizeof(u16))
> + ival = FIELD_PREP(AXI_DAC_CUSTOM_WR_DATA_16, val);
> + else
> + ival = FIELD_PREP(AXI_DAC_CUSTOM_WR_DATA_8, val);
> +
> + ret = regmap_write(st->regmap, AXI_DAC_CUSTOM_WR_REG, ival);
> + if (ret)
> + return ret;
> +
> + /*
> + * Both REG_CNTRL_2 and AXI_DAC_CNTRL_DATA_WR need to know
I'm guessing these got renamed. REG_CNTRL_2 = AXI_DAC_CNTRL_2_REG
and AXI_DAC_CNTRL_DATA_WR = AXI_DAC_CUSTOM_WR_REG?
> + * the data size. So keeping data size control here only,
> + * since data size is mandatory for the current transfer.
> + * DDR state handled separately by specific backend calls,
> + * generally all raw register writes are SDR.
> + */
> + if (data_size == sizeof(u8))
> + ret = regmap_set_bits(st->regmap, AXI_DAC_CNTRL_2_REG,
> + AXI_DAC_CNTRL_2_SYMB_8B);
> + else
> + ret = regmap_clear_bits(st->regmap, AXI_DAC_CNTRL_2_REG,
> + AXI_DAC_CNTRL_2_SYMB_8B);
> + if (ret)
> + return ret;
> +
> + ret = regmap_update_bits(st->regmap, AXI_DAC_CUSTOM_CTRL_REG,
> + AXI_DAC_CUSTOM_CTRL_ADDRESS,
> + FIELD_PREP(AXI_DAC_CUSTOM_CTRL_ADDRESS, reg));
> + if (ret)
> + return ret;
> +
> + ret = regmap_update_bits(st->regmap, AXI_DAC_CUSTOM_CTRL_REG,
> + AXI_DAC_CUSTOM_CTRL_TRANSFER_DATA,
> + AXI_DAC_CUSTOM_CTRL_TRANSFER_DATA);
> + if (ret)
> + return ret;
> +
> + ret = regmap_read_poll_timeout(st->regmap,
> + AXI_DAC_CUSTOM_CTRL_REG, ival,
> + ival & AXI_DAC_CUSTOM_CTRL_TRANSFER_DATA,
> + 10, 100 * KILO);
> + if (ret)
> + return ret;
Should we also clear AXI_DAC_CUSTOM_CTRL_TRANSFER_DATA on timeout
so that we don't leave things in a bad state?
> +
> + return regmap_clear_bits(st->regmap, AXI_DAC_CUSTOM_CTRL_REG,
> + AXI_DAC_CUSTOM_CTRL_TRANSFER_DATA);
> +}
> +
...
> static int axi_dac_probe(struct platform_device *pdev)
> {
> - const unsigned int *expected_ver;
> struct axi_dac_state *st;
> void __iomem *base;
> unsigned int ver;
> @@ -566,15 +793,26 @@ static int axi_dac_probe(struct platform_device *pdev)
> if (!st)
> return -ENOMEM;
>
> - expected_ver = device_get_match_data(&pdev->dev);
> - if (!expected_ver)
> + st->info = device_get_match_data(&pdev->dev);
> + if (!st->info)
> return -ENODEV;
>
> - clk = devm_clk_get_enabled(&pdev->dev, NULL);
> + clk = devm_clk_get_enabled(&pdev->dev, "s_axi_aclk");
This will break existing users that don't have clock-names
in the DT. It should be fine to leave it as NULL in which
case it will get the clock at index 0 in the clocks array
even if there is more than one clock.
> if (IS_ERR(clk))
> return dev_err_probe(&pdev->dev, PTR_ERR(clk),
> "failed to get clock\n");
>
> + if (st->info->has_dac_clk) {
> + struct clk *dac_clk;
> +
> + dac_clk = devm_clk_get_enabled(&pdev->dev, "dac_clk");
> + if (IS_ERR(dac_clk))
> + return dev_err_probe(&pdev->dev, PTR_ERR(dac_clk),
> + "failed to get dac_clk clock\n");
> +
> + st->dac_clk_rate = clk_get_rate(dac_clk);
> + }
> +
> base = devm_platform_ioremap_resource(pdev, 0);
> if (IS_ERR(base))
> return PTR_ERR(base);
Powered by blists - more mailing lists