linux-kernel - Re: [PATCH v6 4/8] iio: dac: adi-axi-dac: extend features

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
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <c4a961752398b7ecc6ed2eb59633b662ccfaaf50.camel@gmail.com>
Date: Tue, 15 Oct 2024 08:30:13 +0200
From: Nuno Sá <noname.nuno@...il.com>
To: David Lechner <dlechner@...libre.com>, 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 Mon, 2024-10-14 at 16:14 -0500, David Lechner wrote:
> 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, &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.

But the question then is, what do we return when streaming mode is off? Dividing by 2
in that case won't report the actual SCLK. But you do have a point and I think a very
common pattern from userspace is to first get the sampling frequency and only then
starting buffering. In this case, yes, we get the wrong sampling frequency. Bottom
line I agree with David and we should just care about returning the max sampling
frequency which is the one that apps ultimately care about.

So, I would say to divide it by 2 during probe and just return that value in here.

- Nuno Sá
> 
> > +
> > +		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.

Good catch...

- Nuno Sá