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

[David Lechner <dlechner@baylibre.com>]

On 10/14/24 5:08 AM, Angelo Dureghello wrote:
> From: Angelo Dureghello <adureghello@baylibre.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@baylibre.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.

> +
> +		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);