2023-01-18 15:20:51

by Brent Pappas

Subject: [PATCH] staging: iio: frequency: ad9832: Replace macro AD9832_PHASE() with function

Replace the macro AD9832_PHASE() with a static function to comply with
Linux coding style standards.

Signed-off-by: Brent Pappas <[email protected]>
---
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);
--
2.34.1

2023-01-18 15:23:42

by Dan Carpenter

Subject: Re: [PATCH] staging: iio: frequency: ad9832: Replace macro AD9832_PHASE() with function

On Wed, Jan 18, 2023 at 10:03:06AM -0500, Brent Pappas wrote:
> Replace the macro AD9832_PHASE() with a static function to comply with
> Linux coding style standards.
>
> Signed-off-by: Brent Pappas <[email protected]>
> ---
> 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; }

The original is fine here. Just leave it as-is.

regards,
dan carpenter

2023-01-18 15:28:31

by Jonathan Cameron

Subject: Re: [PATCH] staging: iio: frequency: ad9832: Replace macro AD9832_PHASE() with function

On Wed, 18 Jan 2023 10:03:06 -0500
Brent Pappas <[email protected]> wrote:

> Replace the macro AD9832_PHASE() with a static function to comply with
> Linux coding style standards.
>
> Signed-off-by: Brent Pappas <[email protected]>

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);