linux-kernel - Re: [PATCH v2 2/6] iio: frequency: adf41513: driver implementation

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: <20251227165613.264d6e11@jic23-huawei>
Date: Sat, 27 Dec 2025 16:56:13 +0000
From: Jonathan Cameron <jic23@...nel.org>
To: Rodrigo Alencar <455.rodrigo.alencar@...il.com>
Cc: Rodrigo Alencar via B4 Relay
 <devnull+rodrigo.alencar.analog.com@...nel.org>,
 rodrigo.alencar@...log.com, linux-kernel@...r.kernel.org,
 linux-iio@...r.kernel.org, devicetree@...r.kernel.org,
 linux-doc@...r.kernel.org, David Lechner <dlechner@...libre.com>, Andy
 Shevchenko <andy@...nel.org>, Lars-Peter Clausen <lars@...afoo.de>, Michael
 Hennerich <Michael.Hennerich@...log.com>, Rob Herring <robh@...nel.org>,
 Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
 <conor+dt@...nel.org>, Jonathan Corbet <corbet@....net>
Subject: Re: [PATCH v2 2/6] iio: frequency: adf41513: driver implementation


...

> > > +	cfg->ref_div2 = FIELD_GET(ADF41513_REG5_RDIV2_MSK,
> > > +				  st->regs_hw[ADF41513_REG5]);
> > > +	cfg->prescaler = FIELD_GET(ADF41513_REG5_PRESCALER_MSK,
> > > +				   st->regs_hw[ADF41513_REG5]);  
> > For cases like this I think keeping to 80 chars is hurting readability
> > and so it is fine to go a little higher. 
> > 	cfg->int_value = FIELD_GET(ADF41513_REG0_INT_MSK, st->regs_hw[ADF41513_REG0]);
> > 	cfg->frac1 = FIELD_GET(ADF41513_REG1_FRAC1_MSK, st->regs_hw[ADF41513_REG1]);
> > 	cfg->frac2 = FIELD_GET(ADF41513_REG3_FRAC2_MSK, st->regs_hw[ADF41513_REG3]);
> > 	cfg->mod2 = FIELD_GET(ADF41513_REG4_MOD2_MSK, st->regs_hw[ADF41513_REG4]);
> > 	cfg->r_counter = FIELD_GET(ADF41513_REG5_R_CNT_MSK, st->regs_hw[ADF41513_REG5]);
> > 	cfg->ref_doubler = FIELD_GET(ADF41513_REG5_REF_DOUBLER_MSK, st->regs_hw[ADF41513_REG5]);
> > 	cfg->ref_div2 = FIELD_GET(ADF41513_REG5_RDIV2_MSK, st->regs_hw[ADF41513_REG5]);
> > 	cfg->prescaler = FIELD_GET(ADF41513_REG5_PRESCALER_MSK,st->regs_hw[ADF41513_REG5]);
> > Is fine here. I'd also be fine with wrapping the ref_doubler line as it's rather
> > longer than the others.  
> 
> ack

Small kernel development process thing.  For efficiency general rule is
don't bother replying at all to suggestions you accept. It just adds noise.
Much better to just crop that chunk of the reply out so we can
rapidly see where more discussion is needed.

>  
> > > +
> > > +	/* calculate pfd frequency */

...

> > > +static int adf41513_parse_fw(struct adf41513_state *st)
> > > +{
> > > +	struct device *dev = &st->spi->dev;
> > > +	int ret;
> > > +	u32 tmp;
> > > +	u32 cp_resistance;
> > > +	u32 cp_current;  
> > Where you have set of variables of same type and grouping doesn't hurt
> > readability, declare them all on one line.
> > 
> > 	u32 tmp, cp_resistance, cp_current;  
> 
> ack
> 
> > I'll not repeat comments I made on the dt-binding in here but I'd expect
> > this code to change somewhat in response to those.  
> 
> understood. for now, will use -mhz for the power-up frequency, but I will
> see how the discussion follows.

That one is indeed non obvious and so (assuming I didn't miss anything) this
is the one block where there was a non trivial comment in your reply so
ideally would have been only bit there.

I get grumpy about this every now and then and this time you get to be
the target!

Jonathan