| Message ID | 1482064465-9674-1-git-send-email-pab@pabigot.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
On 12/18/2016 04:34 AM, Peter A. Bigot wrote: > Expose the per-chip unique identifier so it can be used to identify the > sensor producing the measurements. > > Signed-off-by: Peter A. Bigot <pab@pabigot.com> > --- > Documentation/hwmon/sht21 | 6 ++-- > drivers/hwmon/sht21.c | 92 +++++++++++++++++++++++++++++++++++++++++++++-- > 2 files changed, 94 insertions(+), 4 deletions(-) > > diff --git a/Documentation/hwmon/sht21 b/Documentation/hwmon/sht21 > index db17fda..a0db761 100644 > --- a/Documentation/hwmon/sht21 > +++ b/Documentation/hwmon/sht21 > @@ -16,6 +16,7 @@ Supported chips: > > Author: > Urs Fleisch <urs.fleisch@sensirion.com> > + Peter A. Bigot <pab@pabigot.com> Adding one attribute doesn't make you an author. > > Description > ----------- > @@ -35,6 +36,7 @@ sysfs-Interface > > temp1_input - temperature input > humidity1_input - humidity input > +eic1_input - Electronic Identification Code > > Notes > ----- > @@ -45,5 +47,5 @@ humidity and 66 ms for temperature. To keep self heating below 0.1 degree > Celsius, the device should not be active for more than 10% of the time, > e.g. maximum two measurements per second at the given resolution. > > -Different resolutions, the on-chip heater, using the CRC checksum and reading > -the serial number are not supported yet. > +Different resolutions, the on-chip heater, and using the CRC checksum > +are not supported yet. > diff --git a/drivers/hwmon/sht21.c b/drivers/hwmon/sht21.c > index 84cdb1c..530350b 100644 > --- a/drivers/hwmon/sht21.c > +++ b/drivers/hwmon/sht21.c > @@ -1,6 +1,7 @@ > /* Sensirion SHT21 humidity and temperature sensor driver > * > * Copyright (C) 2010 Urs Fleisch <urs.fleisch@sensirion.com> > + * Copyright (C) 2016 Peter A. Bigot <pab@pabigot.com> Please specify what you claim copyright for. > * > * This program is free software; you can redistribute it and/or modify > * it under the terms of the GNU General Public License as published by > @@ -35,6 +36,10 @@ > #define SHT21_TRIG_T_MEASUREMENT_HM 0xe3 > #define SHT21_TRIG_RH_MEASUREMENT_HM 0xe5 > > +/* Valid flags */ > +#define SHT21_MEASUREMENT_VALID 0x01 > +#define SHT21_EIC_VALID 0x02 Please do not overload the valid flag. > + > /** > * struct sht21 - SHT21 device specific data > * @hwmon_dev: device registered with hwmon > @@ -43,6 +48,7 @@ > * @last_update: time of last update (jiffies) > * @temperature: cached temperature measurement value > * @humidity: cached humidity measurement value > + * @eic: cached electronic identification code > */ > struct sht21 { > struct i2c_client *client; > @@ -51,6 +57,7 @@ struct sht21 { > unsigned long last_update; > int temperature; > int humidity; > + u8 eic[8]; > }; > > /** > @@ -101,7 +108,8 @@ static int sht21_update_measurements(struct device *dev) > * SHT2x should not be active for more than 10% of the time - e.g. > * maximum two measurements per second at 12bit accuracy shall be made. > */ > - if (time_after(jiffies, sht21->last_update + HZ / 2) || !sht21->valid) { > + if (time_after(jiffies, sht21->last_update + HZ / 2) > + || !(SHT21_MEASUREMENT_VALID & sht21->valid)) { As a general rule, Yoda programming in kernel undesirable is. > ret = i2c_smbus_read_word_swapped(client, > SHT21_TRIG_T_MEASUREMENT_HM); > if (ret < 0) > @@ -113,7 +121,7 @@ static int sht21_update_measurements(struct device *dev) > goto out; > sht21->humidity = sht21_rh_ticks_to_per_cent_mille(ret); > sht21->last_update = jiffies; > - sht21->valid = 1; > + sht21->valid |= SHT21_MEASUREMENT_VALID; > } > out: > mutex_unlock(&sht21->lock); > @@ -165,15 +173,95 @@ static ssize_t sht21_show_humidity(struct device *dev, > return sprintf(buf, "%d\n", sht21->humidity); > } > > +/** > + * sht21_show_eic() - show Electronic Identification Code in sysfs > + * @dev: device > + * @attr: device attribute > + * @buf: sysfs buffer (PAGE_SIZE) where measurement values are written to > + * > + * Will be called on read access to eic1_input sysfs attribute. > + * Returns number of bytes written into buffer, negative errno on error. > + */ > +static ssize_t sht21_show_eic(struct device *dev, > + struct device_attribute *attr, > + char *buf) Please follow multi-line alignment rules, and do not add unnecessary line breaks. > +{ > + struct sht21 *sht21 = dev_get_drvdata(dev); > + struct i2c_client *client = sht21->client; > + int ret = 0; > + int i; > + char *bp = buf; Please consider passing your patch through checkpatch and fix reported warnings and errors. > + mutex_lock(&sht21->lock); > + if (!(SHT21_EIC_VALID & sht21->valid)) { > + u8 tx[4]; Unless I am missing something, only two bytes are used for transmit. > + u8 rx[8]; > + struct i2c_msg msgs[2] = { > + { > + .addr = client->addr, > + .flags = 0, > + .len = 0, This is overwritten below, thus setting it to 0 here is quite unnecessary. Actually, you could set it to 2 here right away and leave it at that. > + .buf = tx, > + }, > + { > + .addr = client->addr, > + .flags = I2C_M_RD, > + .len = 0, Overwritten later. > + .buf = rx, > + }, > + }; > + u8 *dp = tx; This variable is unnecessary. Just write directly into tx[0] and tx[1]. > + *dp++ = 0xFA; > + *dp++ = 0x0F; Please be consistent with the rest of the code, which uses defines for commands. > + msgs[0].len = dp-tx; You know exactly how long the command is, so this calculation is unnecessary. > + msgs[1].len = 8; > + ret = i2c_transfer(client->adapter, msgs, 2); > + if (ret < 0) > + goto out; > + sht21->eic[2] = rx[0]; > + sht21->eic[3] = rx[2]; > + sht21->eic[4] = rx[4]; > + sht21->eic[5] = rx[6]; > + dp = tx; > + *dp++ = 0xFC; > + *dp++ = 0xC9; > + msgs[0].len = dp-tx; Same comments as above. > + msgs[1].len = 6; > + ret = i2c_transfer(client->adapter, msgs, 2); > + if (ret < 0) > + goto out; > + sht21->eic[0] = rx[3]; > + sht21->eic[1] = rx[4]; > + sht21->eic[6] = rx[0]; > + sht21->eic[7] = rx[1]; > + sht21->valid |= SHT21_EIC_VALID; > + } > + > + for (i = 0; i < sizeof(sht21->eic); ++i) { > + ret = sprintf(bp, "%02x", sht21->eic[i]); > + if (ret < 0) > + goto out; > + bp += ret; > + } Why recalculate this each time the value is returned to the user, instead of writing the string itself into a (string based) sht21->eic once and just returning that string subsequently ? It would be better to have a separate function for reading the eic (once). > +out: > + mutex_unlock(&sht21->lock); > + if (ret >= 0) { > + *bp++ = '\n'; > + ret = bp - buf; > + } > + return ret; > +} > + > /* sysfs attributes */ > static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO, sht21_show_temperature, > NULL, 0); > static SENSOR_DEVICE_ATTR(humidity1_input, S_IRUGO, sht21_show_humidity, > NULL, 0); > +static SENSOR_DEVICE_ATTR(eic1_input, S_IRUGO, sht21_show_eic, NULL, 0); This is not an input attribute, and it applies to the chip. Just "eic", please. > > static struct attribute *sht21_attrs[] = { > &sensor_dev_attr_temp1_input.dev_attr.attr, > &sensor_dev_attr_humidity1_input.dev_attr.attr, > + &sensor_dev_attr_eic1_input.dev_attr.attr, > NULL > }; > > -- To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Thanks for the very clear and helpful feedback. I'll rework the patch, but have two style questions below. On 12/18/2016 11:53 AM, Guenter Roeck wrote: > On 12/18/2016 04:34 AM, Peter A. Bigot wrote: >> @@ -165,15 +173,95 @@ static ssize_t sht21_show_humidity(struct >> device *dev, >> return sprintf(buf, "%d\n", sht21->humidity); >> } >> >> +/** >> + * sht21_show_eic() - show Electronic Identification Code in sysfs >> + * @dev: device >> + * @attr: device attribute >> + * @buf: sysfs buffer (PAGE_SIZE) where measurement values are >> written to >> + * >> + * Will be called on read access to eic1_input sysfs attribute. >> + * Returns number of bytes written into buffer, negative errno on >> error. >> + */ >> +static ssize_t sht21_show_eic(struct device *dev, >> + struct device_attribute *attr, >> + char *buf) > > Please follow multi-line alignment rules, and do not add unnecessary > line breaks. > I was intentionally matching the single-tab before one-parameter-per-line style present in the existing code. Do you prefer that I do that, or that I change only my code, or that I change the style in the whole file? If the latter, should that be a separate style-only commit? >> +{ >> + struct sht21 *sht21 = dev_get_drvdata(dev); >> + struct i2c_client *client = sht21->client; >> + int ret = 0; >> + int i; >> + char *bp = buf; > > Please consider passing your patch through checkpatch and fix reported > warnings and errors. Apologies. There's one I introduced which I'll fix, but there's also the warning about symbolic permissions S_IRUGO which matched pre-existing style. May I ignore that warning, or should I change the old uses as well? Peter -- To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 12/18/2016 10:35 AM, Peter A. Bigot wrote: > Thanks for the very clear and helpful feedback. I'll rework the patch, but have two style questions below. > > On 12/18/2016 11:53 AM, Guenter Roeck wrote: >> On 12/18/2016 04:34 AM, Peter A. Bigot wrote: >>> @@ -165,15 +173,95 @@ static ssize_t sht21_show_humidity(struct device *dev, >>> return sprintf(buf, "%d\n", sht21->humidity); >>> } >>> >>> +/** >>> + * sht21_show_eic() - show Electronic Identification Code in sysfs >>> + * @dev: device >>> + * @attr: device attribute >>> + * @buf: sysfs buffer (PAGE_SIZE) where measurement values are written to >>> + * >>> + * Will be called on read access to eic1_input sysfs attribute. >>> + * Returns number of bytes written into buffer, negative errno on error. >>> + */ >>> +static ssize_t sht21_show_eic(struct device *dev, >>> + struct device_attribute *attr, >>> + char *buf) >> >> Please follow multi-line alignment rules, and do not add unnecessary line breaks. >> > > I was intentionally matching the single-tab before one-parameter-per-line style present in the existing code. Do you prefer that I do that, or that I change only my code, or that I change the style in the whole file? If the latter, should that be a separate style-only commit? > Ah, yes. Grumble. That is what one gets for accepting such code in the first place. Keep it as is. >>> +{ >>> + struct sht21 *sht21 = dev_get_drvdata(dev); >>> + struct i2c_client *client = sht21->client; >>> + int ret = 0; >>> + int i; >>> + char *bp = buf; >> >> Please consider passing your patch through checkpatch and fix reported warnings and errors. > > Apologies. There's one I introduced which I'll fix, but there's also the warning about symbolic permissions S_IRUGO which matched pre-existing style. May I ignore that warning, or should I change the old uses as well? > Another grumble. I knew this change in preferences would hit us big time :-(. Keep the original code, use octals for yours. I plan to submit a patch to change all permissions in all files to get rid of that warning once and for all. Thanks, Guenter -- To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/Documentation/hwmon/sht21 b/Documentation/hwmon/sht21 index db17fda..a0db761 100644 --- a/Documentation/hwmon/sht21 +++ b/Documentation/hwmon/sht21 @@ -16,6 +16,7 @@ Supported chips: Author: Urs Fleisch <urs.fleisch@sensirion.com> + Peter A. Bigot <pab@pabigot.com> Description ----------- @@ -35,6 +36,7 @@ sysfs-Interface temp1_input - temperature input humidity1_input - humidity input +eic1_input - Electronic Identification Code Notes ----- @@ -45,5 +47,5 @@ humidity and 66 ms for temperature. To keep self heating below 0.1 degree Celsius, the device should not be active for more than 10% of the time, e.g. maximum two measurements per second at the given resolution. -Different resolutions, the on-chip heater, using the CRC checksum and reading -the serial number are not supported yet. +Different resolutions, the on-chip heater, and using the CRC checksum +are not supported yet. diff --git a/drivers/hwmon/sht21.c b/drivers/hwmon/sht21.c index 84cdb1c..530350b 100644 --- a/drivers/hwmon/sht21.c +++ b/drivers/hwmon/sht21.c @@ -1,6 +1,7 @@ /* Sensirion SHT21 humidity and temperature sensor driver * * Copyright (C) 2010 Urs Fleisch <urs.fleisch@sensirion.com> + * Copyright (C) 2016 Peter A. Bigot <pab@pabigot.com> * * This program is free software; you can redistribute it and/or modify * it under the terms of the GNU General Public License as published by @@ -35,6 +36,10 @@ #define SHT21_TRIG_T_MEASUREMENT_HM 0xe3 #define SHT21_TRIG_RH_MEASUREMENT_HM 0xe5 +/* Valid flags */ +#define SHT21_MEASUREMENT_VALID 0x01 +#define SHT21_EIC_VALID 0x02 + /** * struct sht21 - SHT21 device specific data * @hwmon_dev: device registered with hwmon @@ -43,6 +48,7 @@ * @last_update: time of last update (jiffies) * @temperature: cached temperature measurement value * @humidity: cached humidity measurement value + * @eic: cached electronic identification code */ struct sht21 { struct i2c_client *client; @@ -51,6 +57,7 @@ struct sht21 { unsigned long last_update; int temperature; int humidity; + u8 eic[8]; }; /** @@ -101,7 +108,8 @@ static int sht21_update_measurements(struct device *dev) * SHT2x should not be active for more than 10% of the time - e.g. * maximum two measurements per second at 12bit accuracy shall be made. */ - if (time_after(jiffies, sht21->last_update + HZ / 2) || !sht21->valid) { + if (time_after(jiffies, sht21->last_update + HZ / 2) + || !(SHT21_MEASUREMENT_VALID & sht21->valid)) { ret = i2c_smbus_read_word_swapped(client, SHT21_TRIG_T_MEASUREMENT_HM); if (ret < 0) @@ -113,7 +121,7 @@ static int sht21_update_measurements(struct device *dev) goto out; sht21->humidity = sht21_rh_ticks_to_per_cent_mille(ret); sht21->last_update = jiffies; - sht21->valid = 1; + sht21->valid |= SHT21_MEASUREMENT_VALID; } out: mutex_unlock(&sht21->lock); @@ -165,15 +173,95 @@ static ssize_t sht21_show_humidity(struct device *dev, return sprintf(buf, "%d\n", sht21->humidity); } +/** + * sht21_show_eic() - show Electronic Identification Code in sysfs + * @dev: device + * @attr: device attribute + * @buf: sysfs buffer (PAGE_SIZE) where measurement values are written to + * + * Will be called on read access to eic1_input sysfs attribute. + * Returns number of bytes written into buffer, negative errno on error. + */ +static ssize_t sht21_show_eic(struct device *dev, + struct device_attribute *attr, + char *buf) +{ + struct sht21 *sht21 = dev_get_drvdata(dev); + struct i2c_client *client = sht21->client; + int ret = 0; + int i; + char *bp = buf; + mutex_lock(&sht21->lock); + if (!(SHT21_EIC_VALID & sht21->valid)) { + u8 tx[4]; + u8 rx[8]; + struct i2c_msg msgs[2] = { + { + .addr = client->addr, + .flags = 0, + .len = 0, + .buf = tx, + }, + { + .addr = client->addr, + .flags = I2C_M_RD, + .len = 0, + .buf = rx, + }, + }; + u8 *dp = tx; + *dp++ = 0xFA; + *dp++ = 0x0F; + msgs[0].len = dp-tx; + msgs[1].len = 8; + ret = i2c_transfer(client->adapter, msgs, 2); + if (ret < 0) + goto out; + sht21->eic[2] = rx[0]; + sht21->eic[3] = rx[2]; + sht21->eic[4] = rx[4]; + sht21->eic[5] = rx[6]; + dp = tx; + *dp++ = 0xFC; + *dp++ = 0xC9; + msgs[0].len = dp-tx; + msgs[1].len = 6; + ret = i2c_transfer(client->adapter, msgs, 2); + if (ret < 0) + goto out; + sht21->eic[0] = rx[3]; + sht21->eic[1] = rx[4]; + sht21->eic[6] = rx[0]; + sht21->eic[7] = rx[1]; + sht21->valid |= SHT21_EIC_VALID; + } + + for (i = 0; i < sizeof(sht21->eic); ++i) { + ret = sprintf(bp, "%02x", sht21->eic[i]); + if (ret < 0) + goto out; + bp += ret; + } +out: + mutex_unlock(&sht21->lock); + if (ret >= 0) { + *bp++ = '\n'; + ret = bp - buf; + } + return ret; +} + /* sysfs attributes */ static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO, sht21_show_temperature, NULL, 0); static SENSOR_DEVICE_ATTR(humidity1_input, S_IRUGO, sht21_show_humidity, NULL, 0); +static SENSOR_DEVICE_ATTR(eic1_input, S_IRUGO, sht21_show_eic, NULL, 0); static struct attribute *sht21_attrs[] = { &sensor_dev_attr_temp1_input.dev_attr.attr, &sensor_dev_attr_humidity1_input.dev_attr.attr, + &sensor_dev_attr_eic1_input.dev_attr.attr, NULL };
Expose the per-chip unique identifier so it can be used to identify the sensor producing the measurements. Signed-off-by: Peter A. Bigot <pab@pabigot.com> --- Documentation/hwmon/sht21 | 6 ++-- drivers/hwmon/sht21.c | 92 +++++++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 94 insertions(+), 4 deletions(-)