From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from frasgout.his.huawei.com (frasgout.his.huawei.com [185.176.79.56]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id EAE393205 for ; Wed, 18 Jan 2023 15:30:28 +0000 (UTC) Received: from lhrpeml500005.china.huawei.com (unknown [172.18.147.201]) by frasgout.his.huawei.com (SkyGuard) with ESMTP id 4Nxq4g0XTrz6J9W6; Wed, 18 Jan 2023 23:12:35 +0800 (CST) Received: from localhost (10.202.227.76) by lhrpeml500005.china.huawei.com (7.191.163.240) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2375.34; Wed, 18 Jan 2023 15:12:58 +0000 Date: Wed, 18 Jan 2023 15:12:57 +0000 From: Jonathan Cameron To: Brent Pappas CC: , , , , , , Subject: Re: [PATCH] staging: iio: frequency: ad9832: Replace macro AD9832_PHASE() with function Message-ID: <20230118151257.0000663a@Huawei.com> In-Reply-To: <20230118150306.16448-1-bpappas@pappasbrent.com> References: <20230118150306.16448-1-bpappas@pappasbrent.com> Organization: Huawei Technologies Research and Development (UK) Ltd. X-Mailer: Claws Mail 4.1.0 (GTK 3.24.33; x86_64-w64-mingw32) Precedence: bulk X-Mailing-List: linux-staging@lists.linux.dev List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset="US-ASCII" Content-Transfer-Encoding: 7bit X-Originating-IP: [10.202.227.76] X-ClientProxiedBy: lhrpeml100001.china.huawei.com (7.191.160.183) To lhrpeml500005.china.huawei.com (7.191.163.240) X-CFilter-Loop: Reflected On Wed, 18 Jan 2023 10:03:06 -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 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);