linux-kernel - Re: [PATCH] staging: iio: frequency: ad9832: Replace macro AD9832

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] [day] [month] [year] [list]

Message-ID: <20230118151257.0000663a@Huawei.com>
Date:   Wed, 18 Jan 2023 15:12:57 +0000
From:   Jonathan Cameron <Jonathan.Cameron@...wei.com>
To:     Brent Pappas <bpappas@...pasbrent.com>
CC:     <lars@...afoo.de>, <Michael.Hennerich@...log.com>,
        <jic23@...nel.org>, <gregkh@...uxfoundation.org>,
        <linux-iio@...r.kernel.org>, <linux-staging@...ts.linux.dev>,
        <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] staging: iio: frequency: ad9832: Replace macro
 AD9832_PHASE() with function

On Wed, 18 Jan 2023 10:03:06 -0500
Brent Pappas <bpappas@...pasbrent.com> wrote:

> Replace the macro AD9832_PHASE() with a static function to comply with
> Linux coding style standards.
> 
> Signed-off-by: Brent Pappas <bpappas@...pasbrent.com>

Hi Brent,

I'd rather see this changed to FIELD_GET() / FIELD_PREP() along
with all other similar cases in this driver.

That would mean defining just the field masks then using those
to mask relevant bits out and put in the new value.

Thanks

Jonathan


> ---
>  drivers/staging/iio/frequency/ad9832.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/staging/iio/frequency/ad9832.c b/drivers/staging/iio/frequency/ad9832.c
> index 6f9eebd6c7ee..537825534a28 100644
> --- a/drivers/staging/iio/frequency/ad9832.c
> +++ b/drivers/staging/iio/frequency/ad9832.c
> @@ -59,7 +59,9 @@
>  #define AD9832_CMD_SLEEPRESCLR	0xC
>  
>  #define AD9832_FREQ		BIT(11)
> -#define AD9832_PHASE(x)		(((x) & 3) << 9)
> +
> +static unsigned short ad9832_phase(int x) { return (x & 3) << 9; }
> +
>  #define AD9832_SYNC		BIT(13)
>  #define AD9832_SELSRC		BIT(12)
>  #define AD9832_SLEEP		BIT(13)
> @@ -221,8 +223,8 @@ static ssize_t ad9832_write(struct device *dev, struct device_attribute *attr,
>  			break;
>  		}
>  
> -		st->ctrl_fp &= ~AD9832_PHASE(3);
> -		st->ctrl_fp |= AD9832_PHASE(val);
> +		st->ctrl_fp &= ~ad9832_phase(3);
> +		st->ctrl_fp |= ad9832_phase(val);
>  
>  		st->data = cpu_to_be16((AD9832_CMD_FPSELECT << CMD_SHIFT) |
>  					st->ctrl_fp);