Re: [PATCH v4 07/11] iio: dac: adi-axi-dac: extend features

[Nuno Sá]

On Thu, 2024-10-03 at 19:29 +0200, 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
> 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>
> ---
>  drivers/iio/dac/adi-axi-dac.c | 278 +++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 264 insertions(+), 14 deletions(-)
> 
> 

...

> +
> +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;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_FREQUENCY: {
> +		int clk_in, reg;
> +
> +		if (!st->info->bus_controller)
> +			return -EOPNOTSUPP;

Since we'll likely need a more explicitly flag for requesting the reference clock, I
think that can be used in here and so, replace this one. Sorry for suggesting this
one. More details below...

> +
> +		/*
> +		 * 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;
> +
> +		clk_in = clk_get_rate(st->clk);
> +

I don't think the rate is likely to change (at least for now it does not right?). So
we can cache the rate during probe (and then no need to save the struct clk).

> +		if (reg & AXI_DAC_CUSTOM_CTRL_STREAM)
> +			*val = clk_in / 2;
> +		else
> +			*val = clk_in / 8;
> +
> +		return IIO_VAL_INT;
> +		}
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> 

...

>  
> -	clk = devm_clk_get_enabled(&pdev->dev, NULL);
> -	if (IS_ERR(clk))
> -		return dev_err_probe(&pdev->dev, PTR_ERR(clk),
> +	st->clk = devm_clk_get_enabled(&pdev->dev, NULL);
> +	if (IS_ERR(st->clk))
> +		return dev_err_probe(&pdev->dev, PTR_ERR(st->clk),
>  				     "failed to get clock\n");

I think this is wrong. If we look at the docs [1], the clock we want in order to
calculate SCLK is the dac_clk which should be the reference clock. The clock we
previously had in here is the axi clock (s_axi_aclk in the docs) which is the axi bus
clock so we can read/write registers. So, before we had no clk_name before we only
had one clock but now you need proper names for the clocks. For the axi clk, we can
keep a local clock and just make sure we enable it. For the new dac_clk (as named in
the docs), you need to get it only if the IP needs one. And I'm not sure attach the
clk need to the bus_controller flag is a great idea... So I would replace the
bus_controller flag with an explicit has_clkin or has_dacclk (something along those
lines) and use that to request (and enable) the extra clock conditionally.

[1]: https://analogdevicesinc.github.io/hdl/library/axi_ad3552r/index.html
- Nuno Sá