linux-kernel - Re: [PATCH v2 2/2] iio: adc: rtq6056: Add support for the whole RTQ6056 family

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]

Date: Tue, 2 Jan 2024 19:36:42 +0000
From: Jonathan Cameron <jic23@...nel.org>
To: ChiYuan Huang <cy_huang@...htek.com>
Cc: Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>, Conor Dooley
 <conor+dt@...nel.org>, Lars-Peter Clausen <lars@...afoo.de>, Rob Herring
 <robh+dt@...nel.org>, Uwe Kleine-König
 <u.kleine-koenig@...gutronix.de>, <linux-iio@...r.kernel.org>,
 <devicetree@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 2/2] iio: adc: rtq6056: Add support for the whole
 RTQ6056 family

On Tue, 2 Jan 2024 16:30:42 +0800
ChiYuan Huang <cy_huang@...htek.com> wrote:

> Hi, Johathan:
> 
> Most comments are good and will be fixed in next revision.
> 
> Still one comment I cannot make sure.
> 
> Please see the comment that's below yours.
> 	
Hi ChiYuan,

It's good practice to crop away all the parts where the discussion is finished.
Makes it easier for people to find the bit you want discussion to continue on!

I've done so in this reply.

...
> > > +
> > >  enum {
> > >  	RTQ6056_CH_VSHUNT = 0,
> > >  	RTQ6056_CH_VBUS,
> > > @@ -50,16 +60,29 @@ enum {
> > >  enum {
> > >  	F_OPMODE = 0,
> > >  	F_VSHUNTCT,
> > > +	F_SADC = F_VSHUNTCT,  
> > 
> > If the devices have different register fields, better to have different enums
> > for them as well as that should result in less confusing code.
> >   
> Actually, this is all the same register, just the control naming difference.
> If not to define the new eum, I can remain to use the same field to handle rtq6059 part.

If the bits in the register control the same thing across both parts then
add a comment alongside the enum to make that clear. 

Given the naming that seems very unlikely.  PGA and AVG would eman
very different things to me for starters (oversampling vs a programmble
gain amplifier on the front end)

> >   
> > >  	F_VBUSCT,
> > > +	F_BADC = F_VBUSCT,
> > >  	F_AVG,
> > > +	F_PGA = F_AVG,
> > >  	F_RESET,
> > >  	F_MAX_FIELDS
> > >  };