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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <2e2f0626-b9aa-4230-9396-d241615db8f9@wanadoo.fr>
Date: Sun, 8 Sep 2024 17:40:42 +0200
From: Christophe JAILLET <christophe.jaillet@...adoo.fr>
To: adureghello@...libre.com
Cc: Michael.Hennerich@...log.com, conor+dt@...nel.org,
 devicetree@...r.kernel.org, dlechner@...libre.com, jic23@...nel.org,
 krzk+dt@...nel.org, lars@...afoo.de, linux-iio@...r.kernel.org,
 linux-kernel@...r.kernel.org, nuno.sa@...log.com,
 olivier.moysan@...s.st.com, robh@...nel.org
Subject: Re: [PATCH v2 3/9] iio: backend adi-axi-dac: extend features

Le 05/09/2024 à 17:17, Angelo Dureghello a écrit :
> From: Angelo Dureghello <adureghello-rdvid1DuHRBWk0Htik3J/w@...lic.gmane.org>
> 
> 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;
>   }

...