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

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ