Re: [PATCH v2 3/9] iio: backend adi-axi-dac: extend features

[Christophe JAILLET]

Le 05/09/2024 à 17:17, Angelo Dureghello a écrit :
> From: Angelo Dureghello <adureghello-rdvid1DuHRBWk0Htik3J/w@xxxxxxxxxxxxxxxx>
>
> Extend DAC backend with new features required for the AXI driver
> version for the ad3552r DAC. Mainly, a new compatible string has
> been added to support a 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.

Hi,

...

> +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);
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_FREQUENCY:
> +		*val = clk_get_rate(devm_clk_get(st->dev, 0));

Having a devm_clk_get() in such a place is really unusual.
Is it correct?

This look like a memory leak to me.

> +
> +		return IIO_VAL_INT;
> +	default:
> +		return -EINVAL;
> +	}
> +}

...

> +		/*
> +		 * Both REG_CNTRL_2 and AXI_DAC_CNTRL_DATA_WR need to know
> +		 * the data size. So keeping data size control here only,
> +		 * since data size is mandatory for to the current transfer.

"... for to ..." sounds strange to my *non*-native English ears.

> +		 * DDR state handled separately by specific backend calls,
> +		 * generally all raw register writes are SDR.
> +		 */
> +		if (data_size == 1)
> +			ret = regmap_set_bits(st->regmap, AXI_DAC_REG_CNTRL_2,
> +					      AXI_DAC_SYMB_8B);
> +		else
> +			ret = regmap_clear_bits(st->regmap, AXI_DAC_REG_CNTRL_2,
> +						AXI_DAC_SYMB_8B);
> +		if (ret)
> +			return ret;

...

> @@ -556,10 +792,12 @@ static int axi_dac_probe(struct platform_device *pdev)
>   	if (!st)
>   		return -ENOMEM;
>   
> -	expected_ver = device_get_match_data(&pdev->dev);
> -	if (!expected_ver)
> +	info = device_get_match_data(&pdev->dev);
> +	if (!info)

writing:
	st->info = device_get_match_data(&pdev->dev);
	if (!st->info)

would save the 'info' variable and a few lines of code without loosing 
(IMHO) readability.

CJ

>   		return -ENODEV;
>   
> +	st->info = info;
> +
>   	clk = devm_clk_get_enabled(&pdev->dev, NULL);
>   	if (IS_ERR(clk))
>   		return dev_err_probe(&pdev->dev, PTR_ERR(clk),
> @@ -588,12 +826,13 @@ static int axi_dac_probe(struct platform_device *pdev)
>   	if (ret)
>   		return ret;
>   
> -	if (ADI_AXI_PCORE_VER_MAJOR(ver) != ADI_AXI_PCORE_VER_MAJOR(*expected_ver)) {
> +	if (ADI_AXI_PCORE_VER_MAJOR(ver) !=
> +		ADI_AXI_PCORE_VER_MAJOR(st->info->version)) {
>   		dev_err(&pdev->dev,
>   			"Major version mismatch. Expected %d.%.2d.%c, Reported %d.%.2d.%c\n",
> -			ADI_AXI_PCORE_VER_MAJOR(*expected_ver),
> -			ADI_AXI_PCORE_VER_MINOR(*expected_ver),
> -			ADI_AXI_PCORE_VER_PATCH(*expected_ver),
> +			ADI_AXI_PCORE_VER_MAJOR(st->info->version),
> +			ADI_AXI_PCORE_VER_MINOR(st->info->version),
> +			ADI_AXI_PCORE_VER_PATCH(st->info->version),
>   			ADI_AXI_PCORE_VER_MAJOR(ver),
>   			ADI_AXI_PCORE_VER_MINOR(ver),
>   			ADI_AXI_PCORE_VER_PATCH(ver));
> @@ -631,10 +870,18 @@ static int axi_dac_probe(struct platform_device *pdev)
>   	return 0;
>   }

...