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

[Nuno Sá]

On Tue, 2024-10-15 at 10:57 +0200, Angelo Dureghello wrote:
> On 14.10.2024 16:14, David Lechner wrote:
> > On 10/14/24 5:08 AM, Angelo Dureghello wrote:
> > > From: Angelo Dureghello <adureghello@xxxxxxxxxxxx>
> > > 
> > > 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@xxxxxxxxxxxx>
> > > ---
> > 
> > ...
> > 
> > > +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.
> > 
> > > +
> > > +		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?
> > 
> 
> just realized this poll is wrong and unuseful.
> It's a check on a bit we just set.
> Check must be done in AXI_MSK_BUSY of AXI_REG_UI_STATUS.
> 
> If it fails after 100msecs, looks like things are seriously blocked,
> not sure clearing any bit would help.
> 
> 
> > > +
> > > +	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.
> > 
> 
> mm, are there existing users except this hs driver right now ?
> 
> Clock names are actually described in the example, and if missing,
> also retrieving "dac_clk" would fail.
> 

There's already a frontend DAC using the generic DAC implementation. So, in theory,
yes... We can already have users not setting clock-names in DT that would now fail to
probe with this patch. David is only suggesting leaving this call to NULL. For
dac_clk we do need the *id in devm_clk_get_enabled().

Maybe it would also be worth mentioning in the bindings that s_axi_aclk needs to be
the first entry in clocks and clock-names for backward compatibility.

- Nuno Sá
> >