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

Nuno Sá
2024-10-22T12:32:28Z

On Mon, 2024-10-21 at 14:40 +0200, 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 series 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@baylibre.com>
> ---

Looks mostly good,

one minor thing that (I think) could be improved
>  drivers/iio/dac/adi-axi-dac.c | 269 +++++++++++++++++++++++++++++++++++++++--
> -
>  1 file changed, 255 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/iio/dac/adi-axi-dac.c b/drivers/iio/dac/adi-axi-dac.c
> index 04193a98616e..9d6809fe7a67 100644
> --- a/drivers/iio/dac/adi-axi-dac.c
> +++ b/drivers/iio/dac/adi-axi-dac.c
> @@ -46,9 +46,28 @@
>  #define AXI_DAC_CNTRL_1_REG 0x0044
>  #define   AXI_DAC_CNTRL_1_SYNC BIT(0)
>  #define AXI_DAC_CNTRL_2_REG 0x0048
> +#define   AXI_DAC_CNTRL_2_SDR_DDR_N BIT(16)
> +#define   AXI_DAC_CNTRL_2_SYMB_8B BIT(14)
>  #define   ADI_DAC_CNTRL_2_R1_MODE BIT(5)
> +#define   AXI_DAC_CNTRL_2_UNSIGNED_DATA BIT(4)
> +#define AXI_DAC_STATUS_1_REG 0x0054
> +#define AXI_DAC_STATUS_2_REG 0x0058
>  #define AXI_DAC_DRP_STATUS_REG 0x0074
>  #define   AXI_DAC_DRP_STATUS_DRP_LOCKED BIT(17)
> +#define AXI_DAC_CUSTOM_RD_REG 0x0080
> +#define AXI_DAC_CUSTOM_WR_REG 0x0084
> +#define   AXI_DAC_CUSTOM_WR_DATA_8 GENMASK(23, 16)
> +#define   AXI_DAC_CUSTOM_WR_DATA_16 GENMASK(23, 8)
> +#define AXI_DAC_UI_STATUS_REG 0x0088
> +#define   AXI_DAC_UI_STATUS_IF_BUSY BIT(4)
> +#define AXI_DAC_CUSTOM_CTRL_REG 0x008C
> +#define   AXI_DAC_CUSTOM_CTRL_ADDRESS GENMASK(31, 24)
> +#define   AXI_DAC_CUSTOM_CTRL_SYNCED_TRANSFER BIT(2)
> +#define   AXI_DAC_CUSTOM_CTRL_STREAM BIT(1)
> +#define   AXI_DAC_CUSTOM_CTRL_TRANSFER_DATA BIT(0)

...

>  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,14 +780,29 @@ 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, "s_axi_aclk");
> + if (IS_ERR(clk)) {

If clock-names is not given, then we'll get -EINVAL. Hence we could assume that:

if (PTR_ERR(clk) != -EINVAL)
return dev_err_probe();

- Nuno Sá