Re: [PATCH v2 2/2] iio: temperature: Add support for AMS AS6200

[Jonathan Cameron]

On Mon, 4 Dec 2023 21:16:34 -0500
Abdel Alkuor <alkuor@xxxxxxxxx> wrote:

> On Mon, Dec 04, 2023 at 01:50:14PM +0000, Jonathan Cameron wrote:
> > On Fri,  1 Dec 2023 23:16:51 -0500
> > Abdel Alkuor <alkuor@xxxxxxxxx> wrote:
> >   
> > > as6200 is a high accuracy temperature sensor of 0.0625C with a range
> > > between -40 to 125 Celsius degrees.
> > > 
> > > The driver implements the alert trigger event in comparator mode where
> > > consecutive faults are converted into periods, high/low temperature
> > > thresholds require to be above/below the set limit for n seconds for
> > > the alert to be triggered/cleared. The alert is only cleared when the
> > > current temperature is below the low temperature threshold for n seconds.
> > > 
> > > The driver supports the following:
> > > - show available sampling frequencey
> > > - read/write sampling frequency
> > > - read raw temperature
> > > - read scaling factor
> > > - read/write temperature period that needs to be met for low/high
> > >   temperature thresholds to trigger an alert
> > > - show available temperature period thresholds
> > > - buffer trigger
> > > - temperature alert event trigger  
> > 
> > Hi Abdel,
> > 
> > A few comments inline. Looking good in general.
> >  
> Hi Jonathon,
> 
> Thank you for your time. I have a couple _silly_ questions about the tag
> and returning from if else. Other than that, your comments will be addressed
> in v3.
> > > 
> > > Datasheet: https://ams.com/documents/20143/36005/AS6200_DS000449_4-00.pdf
> > >   
> > 
> > No blank line here.  Tags block (and Datasheet is a tag) never has blank lines
> > as that breaks some existing tooling.
> >   
> Understood. 
> 
> P.S. when running checkpatch.pl on this patch, I get the following warning:
> 
> WARNING: Unknown link reference 'Datasheet:', use 'Link:' or 'Closes:' instead
> 
> should I use Link instead?

Nah. Just ignore checkpatch. :)

Also, if just accepting a comment, no need to say so.  Assumption of reviewer
is that if you keep quiet on a particular comment you will have it fixed in next
version.  Crop out that section of the email and only keep the bits that
you want the reviewer to see your comments on.

Hopefully I've cropped it appropriately!

> > > Signed-off-by: Abdel Alkuor <alkuor@xxxxxxxxx>
> > > +	if (info == IIO_EV_INFO_VALUE) {
> > > +		*val = sign_extend32(FIELD_GET(AS6200_TEMP_MASK, tmp), 11);
> > > +		ret = IIO_VAL_INT;  
> > return here.
> >   
> > > +	} else {
> > > +		cf = FIELD_GET(AS6200_CONFIG_CF, tmp);
> > > +		cr = FIELD_GET(AS6200_CONFIG_CR, tmp);
> > > +		*val = as6200_temp_thresh_periods[cr][cf][0];
> > > +		*val2 = as6200_temp_thresh_periods[cr][cf][1];
> > > +		ret = IIO_VAL_INT_PLUS_MICRO;  
> > 
> > and here.  If there is nothing more to be done, it simplifies the code
> > flow being read to just return as quick as possible.
> >  
> I did it as you mentioned, and when running check_patch.pl, it gives back a
> warning that else is not needed here because of the return in the if
> statement. So I opted into using ret instead, should I remove the else or ignore
> the warning?

Dropping the else is fine or take the view it's wrong and ignore it.
Checkpatch is providing hints and suggestions on where to double check.
There is no requirement to accept it's suggestions if you feel the
code ends up less readable.

> > > +	}
> > > +
> > > +	return ret;