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

[Jonathan Cameron]

On Tue, 2 Jan 2024 16:30:42 +0800
ChiYuan Huang <cy_huang@xxxxxxxxxxx> 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
> > >  };