Re: [PATCH v5 3/4] iio: chemical: Add Senseair Sunrise 006-0-007 driver

[Jonathan Cameron <jic23@kernel.org>]

On Thu, 9 Sep 2021 16:01:48 +0300
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> On Thu, Sep 09, 2021 at 11:45:36AM +0200, Jacopo Mondi wrote:
> > Add support for the Senseair Sunrise 006-0-0007 driver through the
> > IIO subsystem.  
> 
> ...
> 
> > +	/*
> > +	 * Wake up sensor by sending sensor address: START, sensor address,
> > +	 * STOP. Sensor will not ACK this byte.
> > +	 *
> > +	 * The chip enters a low power state after 15msec without  
> 
> msec --> ms (everybody understands 'ms' unit)

yup. millisiemens :)
(couldn't resist)

Context is fine either way here.

> 
> > +	 * communications or after a complete read/write sequence.
> > +	 */  
> 
> ...
> 
> > +	struct i2c_client *client = context;
> > +	union i2c_smbus_data data;
> > +
> > +	/* Discard reg address from values count. */
> > +	if (count < 1)
> > +		return -EINVAL;
> > +	count--;  
> 
> Wouldn't be more natural to decrement and then check against 0?
> 
> ...
> 
> > +	memcpy(&data.block[1], (u8 *)val_buf + 1, count);  
> 
> Not sure I understand why you need an explicit casting here.

C doesn't allow pointer arithmetic on void * (gcc has it as an extension
though so it's probably fine without the cast)



> 
> 
> ...
> 
> > +	mutex_lock(&sunrise->lock);
> > +	ret = sunrise_read_word(sunrise, SUNRISE_ERROR_STATUS_REG, &value);
> > +	if (ret) {
> > +		mutex_unlock(&sunrise->lock);  
> 
> > +		return -EINVAL;  
> 
> Why shadowing an actual error code?
> 
> > +	}  
> 
> ...
> 
> > +			/*
> > +			 * / 10^4 to comply with IIO scale for CO2 (percentage).  
> 
> "1 / 10^4"
> 
> > +			 * The chip CO2 reading range is [400 - 5000] ppm
> > +			 * which corresponds to [0,004 - 0,5] %.
> > +			 */  
>