linux-kernel - Re: [PATCH v2] iio: frequency: ad9832: Use FIELD

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <Z9j52jGqft2jvT2O@debian-BULLSEYE-live-builder-AMD64>
Date: Tue, 18 Mar 2025 01:43:06 -0300
From: Marcelo Schmitt <marcelo.schmitt1@...il.com>
To: Siddharth Menon <simeddon@...il.com>
Cc: linux-iio@...r.kernel.org, lars@...afoo.de,
	Michael.Hennerich@...log.com, jic23@...nel.org,
	gregkh@...uxfoundation.org, linux-kernel@...r.kernel.org,
	linux-staging@...ts.linux.dev
Subject: Re: [PATCH v2] iio: frequency: ad9832: Use FIELD_PREP macro to set
 bit fields

Hi Siddharth,

On 03/17, Siddharth Menon wrote:
> Refactor code to use the FIELD_PREP macro for setting bit fields
> instead of manual bit manipulation.
> 
> Suggested-by: Marcelo Schmitt <marcelo.schmitt1@...il.com>
> Signed-off-by: Siddharth Menon <simeddon@...il.com>
> ---
...
> +#define CMD_MASK_2   GENMASK(15, 12)
> +#define ADD_MASK_2   GENMASK(11, 8)
> +#define DATA_MASK_2  GENMASK(7, 0)

DATA_MASK_2? Did we already have a data mask?
What about adding the device prefix to the mask name (e.g. AD9832_CMD_MASK)?
Also, this patch fails to compile. Please, apply your patches and build the
kernel before sending the patches to the mailing list. Also, run checkpatch on them.
E.g. 
./scripts/checkpatch.pl --terse --codespell --color=always -strict my_patch.patch

>  
>  /**
>   * struct ad9832_state - driver instance specific data
> @@ -131,6 +134,7 @@ static int ad9832_write_frequency(struct ad9832_state *st,
>  {
>  	unsigned long clk_freq;
>  	unsigned long regval;
> +	u8 regval_bytes[4];
>  
>  	clk_freq = clk_get_rate(st->mclk);
>  
> @@ -138,19 +142,14 @@ static int ad9832_write_frequency(struct ad9832_state *st,
>  		return -EINVAL;
>  
>  	regval = ad9832_calc_freqreg(clk_freq, fout);
> +	put_unaligned_be32(regval, regval_bytes);
>  
> -	st->freq_data[0] = cpu_to_be16((AD9832_CMD_FRE8BITSW << CMD_SHIFT) |
> -					(addr << ADD_SHIFT) |
> -					((regval >> 24) & 0xFF));
> -	st->freq_data[1] = cpu_to_be16((AD9832_CMD_FRE16BITSW << CMD_SHIFT) |
> -					((addr - 1) << ADD_SHIFT) |
> -					((regval >> 16) & 0xFF));
> -	st->freq_data[2] = cpu_to_be16((AD9832_CMD_FRE8BITSW << CMD_SHIFT) |
> -					((addr - 2) << ADD_SHIFT) |
> -					((regval >> 8) & 0xFF));
> -	st->freq_data[3] = cpu_to_be16((AD9832_CMD_FRE16BITSW << CMD_SHIFT) |
> -					((addr - 3) << ADD_SHIFT) |
> -					((regval >> 0) & 0xFF));
> +	for (int i = 0; i < 4; i++) {
> +		st->freq_data[i] = cpu_to_be16(FIELD_PREP(AD9832_CMD_MSK,
> +				(i % 2 == 0) ? AD9832_CMD_FRE8BITSW : AD9832_CMD_FRE16BITSW) |
Hmm, I mentioned using ternary operator and gave an example usage but wasn't
expecting that particular example to really be used. IMHO, the above doesn't
look very good.
Can you try come up with something that, (1) avoids the bit shifting we had
before, (2) uses sound macro/mask/variable naming, and (3) fits into 80 columns?
Might not be an easy task so probably not worth sending much more time on this
if unable to find a good refactoring for the above.

Regards,
Marcelo