On Sat, 11 Jul 2020 18:27:09 +0300 Andy Shevchenko <andy.shevchenko@xxxxxxxxx> wrote: > On Fri, Jul 10, 2020 at 2:54 PM Eugene Zaikonnikov <ez@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx> > > 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
