Re: [PATCH v9 1/2] iio: humidity: Add TI HDC20x0 support

[Jonathan Cameron <jic23@kernel.org>]

On Sat, 11 Jul 2020 18:27:09 +0300
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Fri, Jul 10, 2020 at 2:54 PM Eugene Zaikonnikov <ez@norphonic.com> wrote:
> >
> > Add support for HDC2010/2080 driver and sysfs documentation for its
> > heater element.
> >
> > HDC2010 is an integrated high-accuracy humidity and temperature sensor
> > with very low power consumption. The device includes a resistive heating
> > element. The temperature range is -40C to 125C with 0.2C
> > accuracy. Humidity measurement is 0 to 100% with 2% RH accuracy.  
> 
> Now, some almost context-less comments to the contribution.
> 
> Please, use Datasheet tag(s) here with URL(s) to the datasheet(s).
> Also, be sure you are using https (note S) everywhere.
> 
> Datasheet: https://...
> Datasheet: ...
> 
> > Signed-off-by: Eugene Zaikonnikov <ez@norphonic.com>  
> 
> 1. No need to put file name into the file
> 2. Missed at least bits.h inclusion
> 3. Keep comma in HDC2010_GROUP_HUMIDITY
> 4. IIO_CONST_ATTR can be one line, but hey don't we have IIO core to
> take care of it?

For that one, we could indeed use the read_avail callback here
for the out_current_heater_raw_available. I've not yet started insisting
on this because of the huge number of drivers that predate introduction of
that stuff to the core and as a result a lack of good examples.
Eugene, if you are happy to change this one over to that and hence act
as an example it would be great!

> 5. Use traditional pattern
> 
> if (ret)
>   return ret;
> 
> data->drdy_config = tmp;
> 
> return 0;
> 
> 6. Indent better
> 
> if (!i2c_check_functionality(client->adapter,
>     I2C_FUNC_SMBUS_WORD_DATA | I2C_FUNC_SMBUS_BYTE | I2C_FUNC_I2C))
> 
> (or split last line above by operand per line or alike)
> 
> 7. Drop unneeded casting
> tmp = (data->measurement_config & ~HDC2010_MEAS_CONF) | HDC2010_MEAS_TRIG;
> 
> 8. It's one line
> ret = i2c_smbus_write_byte_data(client,
>     HDC2010_REG_MEASUREMENT_CONF, tmp);
> 
> Ditto:
> dev_warn(&client->dev, "Unable to restore default AMM\n");
> 
> In general it doesn't look bad!
> 

Thanks,

Jonathan