* Re: [lm-sensors] [RFC PATCH 2/3] hwmon: sht15: add support for
2011-03-16 18:44 [lm-sensors] [RFC PATCH 2/3] hwmon: sht15: add support for reading Vivien Didelot
@ 2011-03-21 19:43 ` Jonathan Cameron
2011-03-22 0:00 ` Vivien Didelot
` (8 subsequent siblings)
9 siblings, 0 replies; 11+ messages in thread
From: Jonathan Cameron @ 2011-03-21 19:43 UTC (permalink / raw)
To: lm-sensors
On 03/16/11 18:44, Vivien Didelot wrote:
> Add support to read the sht15 status register. Explicitly:
> * The measurement resolution through the temp_resolution and
> humidity_resolution sysfs attributes;
> * An end of battery indicator through the battery_alarm sysfs attribute;
> * Embedded heater support using the heater_enable sysfs attribute;
> * Reload from OTP available through the otp_reload sysfs attribute.
Hi Vivien.
As Guenter said, these definitely need documenting.. Once that is available
I expect people will debate naming etc.
Some comments inline.
>
> Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
> Reviewed-by: Jerome Oufella <jerome.oufella@savoirfairelinux.com>
> To: lm-sensors@lm-sensors.org
> Cc: khali@linux-fr.org
> ---
> drivers/hwmon/sht15.c | 168 ++++++++++++++++++++++++++++++++++++++++++++++++-
> 1 files changed, 167 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/hwmon/sht15.c b/drivers/hwmon/sht15.c
> index 83822a6..62a1a03 100644
> --- a/drivers/hwmon/sht15.c
> +++ b/drivers/hwmon/sht15.c
> @@ -3,6 +3,7 @@
> *
> * Portions Copyright (c) 2010-2011 Savoir-faire Linux Inc.
> * Jerome Oufella <jerome.oufella@savoirfairelinux.com>
> + * Vivien Didelot <vivien.didelot@savoirfairelinux.com>
> *
> * Copyright (c) 2009 Jonathan Cameron
> *
> @@ -14,7 +15,6 @@
> *
> * Default resolution only (14bit temp, 12bit humidity)
> * Checksum validation can be enabled via the checksumming sysfs attribute
> - * Ignoring battery status.
> * Heater not enabled.
> * Timings are all conservative.
> *
> @@ -45,6 +45,7 @@
> #define SHT15_MEASURE_TEMP 0x03
> #define SHT15_MEASURE_RH 0x05
> #define SHT15_SOFT_RESET 0x1E
> +#define SHT15_READ_STATUS 0x07
Keep this in numerical order.
>
> #define SHT15_READING_NOTHING 0
> #define SHT15_READING_TEMP 1
> @@ -58,6 +59,12 @@
> /* Min timings in msecs */
> #define SHT15_TSRST 11 /* soft reset time */
>
> +/* Status Register Bits */
> +#define SHT15_STATUS_BATTERY 0x40
> +#define SHT15_STATUS_HEATER 0x04
> +#define SHT15_STATUS_OTP_RELOAD 0x02
> +#define SHT15_STATUS_RESOLUTION 0x01
> +
> /**
> * struct sht15_temppair - elements of voltage dependant temp calc
> * @vdd: supply voltage in microvolts
> @@ -114,6 +121,7 @@ static const u8 sht15_crc8_table[] = {
> * @wait_queue: wait queue for getting values from device
> * @val_temp: last temperature value read from device
> * @val_humid: last humidity value read from device
> + * @val_status: last status register value read from device
> * @checksum_ok:last value read from device passed checksum verification
> * @flag: status flag used to identify what the last request was
> * @valid: are the current stored values valid (start condition)
> @@ -132,6 +140,11 @@ static const u8 sht15_crc8_table[] = {
> * @update_supply_work: work struct that is used to update the supply_uV
> * @interrupt_handled: flag used to indicate a hander has been scheduled
> * @checksumming: flag used to indicate the data checksum must be validated
> + * @end_of_battery: flag used to indicate the low voltage detection
> + * @heater: flag used to indicate the heater status
> + * @no_otp_reload: flag used to indicate no reloading from OTP
> + * @temp_resolution: the temperature measurement resolution
> + * @humidity_resolution: the humidity measurement resolution
> */
> struct sht15_data {
> struct sht15_platform_data *pdata;
> @@ -139,6 +152,7 @@ struct sht15_data {
> wait_queue_head_t wait_queue;
> uint16_t val_temp;
> uint16_t val_humid;
Why cached the raw status? Can't see any users.
> + u8 val_status;
> u8 checksum_ok;
> u8 flag;
> u8 valid;
> @@ -153,6 +167,11 @@ struct sht15_data {
> struct work_struct update_supply_work;
> atomic_t interrupt_handled;
> u8 checksumming;
Bit field for all these plesae as they are flags - might as well
allow for possibility of saving a bit of space.
> + u8 end_of_battery;
> + u8 heater;
> + u8 no_otp_reload;
> + u8 temp_resolution;
> + u8 humidity_resolution;
> };
>
> static u8 sht15_read_byte(struct sht15_data *);
> @@ -193,6 +212,26 @@ static u8 sht15_crc8(const unsigned char *data, unsigned char len)
> }
>
> /**
> + * sht15_apply_status() - update data attributes from the status
> + * @data: sht15 specific data.
> + * @status: new status to set to data.
> + */
> +static void sht15_apply_status(struct sht15_data *data, u8 status)
> +{
> + data->val_status = status;
make it easier to read with !!(status & SHT15_STATUS_BATTERY);
etc.
> + data->end_of_battery = (status & SHT15_STATUS_BATTERY) >> 6;
> + data->heater = (status & SHT15_STATUS_HEATER) >> 2;
> + data->no_otp_reload = (status & SHT15_STATUS_OTP_RELOAD) >> 1;
Given these are yoked together could just have a single flag and
interpret it in the few places that care?
> + if (status & SHT15_STATUS_RESOLUTION) {
> + data->temp_resolution = 12;
> + data->humidity_resolution = 8;
> + } else {
> + data->temp_resolution = 14;
> + data->humidity_resolution = 12;
> + }
> +}
> +
> +/**
> * sht15_connection_reset() - reset the comms interface
> * @data: sht15 specific data
> *
> @@ -316,6 +355,49 @@ static inline void sht15_soft_reset(struct sht15_data *data)
> {
> sht15_send_cmd(data, SHT15_SOFT_RESET);
> msleep(SHT15_TSRST);
> + /* reset attributes with default register value */
> + sht15_apply_status(data, 0);
> +}
> +
> +/**
> + * sht15_update_status() - get the status register from device
> + * @data: device instance specific data.
> + *
> + * As described in figure 15 and table 5 of the data sheet.
> + */
> +static int sht15_update_status(struct sht15_data *data)
> +{
> + int ret;
> + u8 status;
> + u8 dev_checksum = 0;
> + u8 checksum_values[2];
> +
> + ret = sht15_send_cmd(data, SHT15_READ_STATUS);
> + if (ret)
> + return ret;
> + status = sht15_read_byte(data);
> +
> + if (data->checksumming) {
> + sht15_ack(data);
> + dev_checksum = reverse(sht15_read_byte(data));
> + checksum_values[0] = SHT15_READ_STATUS;
> + checksum_values[1] = status;
> + data->checksum_ok = (sht15_crc8(checksum_values, 2) = dev_checksum);
> + }
> +
> + sht15_end_transmission(data);
> +
> + /*
> + * Perform checksum validation on the received data.
> + * Specification mentions that in case a checksum verification fails,
> + * a soft reset command must be sent to the device.
> + */
> + if (data->checksumming && !data->checksum_ok) {
> + sht15_soft_reset(data);
> + return -EAGAIN;
> + }
> + sht15_apply_status(data, status);
> + return 0;
> }
>
> /**
> @@ -378,6 +460,9 @@ static int sht15_update_vals(struct sht15_data *data)
> mutex_lock(&data->read_lock);
> if (time_after(jiffies, data->last_updat + timeout)
> || !data->valid) {
As with the previous version, I'd make this optional controlled
by platform data. Not everyone cares and it just made their gpio's
jump up and down a fair bit more. Obviously if it's turned off, you
will also want to change what attributes are registered.
> + ret = sht15_update_status(data);
> + if (ret)
> + goto error_ret;
> data->flag = SHT15_READING_HUMID;
> ret = sht15_update_single_val(data, SHT15_MEASURE_RH, 160);
> if (ret)
> @@ -442,6 +527,71 @@ static inline int sht15_calc_humid(struct sht15_data *data)
> / 1000000 + RHlinear;
> }
>
> +static ssize_t sht15_show_battery(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + int ret;
> + struct sht15_data *data = dev_get_drvdata(dev);
> +
> + /* Technically no need to read temperature and humidity as well */
Given we do a fresh read here, why bother having the cache at all? Just
read it explicitly when we care.
> + ret = sht15_update_vals(data);
> +
> + return ret ? ret : sprintf(buf, "%d\n", data->end_of_battery);
> +}
> +
> +static ssize_t sht15_show_heater(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + int ret;
> + struct sht15_data *data = dev_get_drvdata(dev);
> +
> + /* Technically no need to read temperature and humidity as well */
Indeed, so don't.
> + ret = sht15_update_vals(data);
> +
> + return ret ? ret : sprintf(buf, "%d\n", data->heater);
> +}
> +
I've no idea off the top of my head what OTP is so perhaps a more
informative name or a suitable comment?
> +static ssize_t sht15_show_otp_reload(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + int ret;
> + struct sht15_data *data = dev_get_drvdata(dev);
> +
> + /* Technically no need to read temperature and humidity as well */
> + ret = sht15_update_vals(data);
> +
> + return ret ? ret : sprintf(buf, "%d\n", !(data->no_otp_reload));
> +}
> +
> +static ssize_t sht15_show_temp_resolution(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + int ret;
> + struct sht15_data *data = dev_get_drvdata(dev);
> +
> + /* Technically no need to read temperature and humidity as well */
Pretty big reason for reading temperature at least in this particular instance...
> + ret = sht15_update_vals(data);
> +
> + return ret ? ret : sprintf(buf, "%d\n", data->temp_resolution);
> +}
> +
> +static ssize_t sht15_show_humidity_resolution(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + int ret;
> + struct sht15_data *data = dev_get_drvdata(dev);
> +
> + /* Technically no need to read temperature and humidity as well */
I get the feeling we might have a reason here....
> + ret = sht15_update_vals(data);
> +
> + return ret ? ret : sprintf(buf, "%d\n", data->humidity_resolution);
> +}
> +
> static ssize_t sht15_show_temp(struct device *dev,
> struct device_attribute *attr,
> char *buf)
> @@ -510,6 +660,17 @@ static SENSOR_DEVICE_ATTR(temp1_input,
> static SENSOR_DEVICE_ATTR(humidity1_input,
> S_IRUGO, sht15_show_humidity,
> NULL, 0);
> +static DEVICE_ATTR(battery_alarm, S_IRUGO, sht15_show_battery, NULL);
> +static DEVICE_ATTR(heater_enable, S_IRUGO, sht15_show_heater, NULL);
> +static DEVICE_ATTR(otp_reload, S_IRUGO, sht15_show_otp_reload, NULL);
Not a chance any attribute with that incomprehensible a name is going
to be merged. For reference of others its a question of whether magic
component specific corrections are applied or not.
> +static DEVICE_ATTR(temp_resolution,
> + S_IRUGO,
> + sht15_show_temp_resolution,
> + NULL);
> +static DEVICE_ATTR(humidity_resolution,
> + S_IRUGO,
> + sht15_show_humidity_resolution,
> + NULL);
Probably do resolutions through platform data. Might be some
usecases for dynamically varying them but is it worth while
supporting? Not sure. One for discussion when you propose some
documentation for these.
> static DEVICE_ATTR(checksumming,
> S_IRUGO | S_IWUSR,
> sht15_show_checksum,
> @@ -518,6 +679,11 @@ static DEVICE_ATTR(name, S_IRUGO, show_name, NULL);
> static struct attribute *sht15_attrs[] = {
> &sensor_dev_attr_temp1_input.dev_attr.attr,
> &sensor_dev_attr_humidity1_input.dev_attr.attr,
> + &dev_attr_battery_alarm.attr,
> + &dev_attr_heater_enable.attr,
> + &dev_attr_otp_reload.attr,
> + &dev_attr_temp_resolution.attr,
> + &dev_attr_humidity_resolution.attr,
> &dev_attr_checksumming.attr,
> &dev_attr_name.attr,
> NULL,
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [lm-sensors] [RFC PATCH 2/3] hwmon: sht15: add support for
2011-03-16 18:44 [lm-sensors] [RFC PATCH 2/3] hwmon: sht15: add support for reading Vivien Didelot
2011-03-21 19:43 ` [lm-sensors] [RFC PATCH 2/3] hwmon: sht15: add support for Jonathan Cameron
@ 2011-03-22 0:00 ` Vivien Didelot
2011-03-22 0:07 ` Guenter Roeck
` (7 subsequent siblings)
9 siblings, 0 replies; 11+ messages in thread
From: Vivien Didelot @ 2011-03-22 0:00 UTC (permalink / raw)
To: lm-sensors
Hi Jonathan.
On Mon, Mar 21, 2011 at 07:43:59PM +0000, Jonathan Cameron wrote:
> On 03/16/11 18:44, Vivien Didelot wrote:
> > Add support to read the sht15 status register. Explicitly:
> > * The measurement resolution through the temp_resolution and
> > humidity_resolution sysfs attributes;
> > * An end of battery indicator through the battery_alarm sysfs attribute;
> > * Embedded heater support using the heater_enable sysfs attribute;
> > * Reload from OTP available through the otp_reload sysfs attribute.
> Hi Vivien.
>
> As Guenter said, these definitely need documenting.. Once that is available
> I expect people will debate naming etc.
Sure. After discussing about that, checksumming, otp_reload and
*_resolution attributes have been removed. battery_alarm will be splitted into
temp1_alarm and humidity1_alarm to respect the convention.
I'll also add a documentation in Documentation/hwmon/ for the device.
> > #define SHT15_MEASURE_TEMP 0x03
> > #define SHT15_MEASURE_RH 0x05
> > #define SHT15_SOFT_RESET 0x1E
> > +#define SHT15_READ_STATUS 0x07
> Keep this in numerical order.
Ok.
> Why cached the raw status? Can't see any users.
> > + u8 val_status;
As you've seen, it will be used in the next patch.
> Bit field for all these plesae as they are flags - might as well
> allow for possibility of saving a bit of space.
> > + u8 end_of_battery;
> > + u8 heater;
> > + u8 no_otp_reload;
> > + u8 temp_resolution;
> > + u8 humidity_resolution;
Ok, I'll check it out.
> make it easier to read with !!(status & SHT15_STATUS_BATTERY);
> etc.
> > + data->end_of_battery = (status & SHT15_STATUS_BATTERY) >> 6;
> > + data->heater = (status & SHT15_STATUS_HEATER) >> 2;
> > + data->no_otp_reload = (status & SHT15_STATUS_OTP_RELOAD) >> 1;
Good idea.
> Given these are yoked together could just have a single flag and
> interpret it in the few places that care?
> > + if (status & SHT15_STATUS_RESOLUTION) {
> > + data->temp_resolution = 12;
> > + data->humidity_resolution = 8;
> > + } else {
> > + data->temp_resolution = 14;
> > + data->humidity_resolution = 12;
> > + }
Regarding one of your comment in the next patch, it makes sense to use an
explicit single flag, so I'll remove this as well.
> > /**
> > @@ -378,6 +460,9 @@ static int sht15_update_vals(struct sht15_data *data)
> > mutex_lock(&data->read_lock);
> > if (time_after(jiffies, data->last_updat + timeout)
> > || !data->valid) {
> As with the previous version, I'd make this optional controlled
> by platform data. Not everyone cares and it just made their gpio's
> jump up and down a fair bit more. Obviously if it's turned off, you
> will also want to change what attributes are registered.
> > + ret = sht15_update_status(data);
> > + if (ret)
> > + goto error_ret;
I put the sht15_update_status() call here so it could have benefit of the
jiffies calculation. I also thought that it could be useful to update the
device state before each measurement, in order to notice an eventual low
power notice. Maybe I'm wrong and this test should be done separately?
> > + /* Technically no need to read temperature and humidity as well */
> Given we do a fresh read here, why bother having the cache at all? Just
> read it explicitly when we care.
> > + ret = sht15_update_vals(data);
According to the previous comment, what about adding a mask as an argument
to the sht15_update_vals() function to know which measurement to do (only
status, temperature + humidity, etc.)? Because the discussion could also
be pointed on the temperature measurement, which update unnecessarily the
humidity as well.
> I've no idea off the top of my head what OTP is so perhaps a more
> informative name or a suitable comment?
> > +static ssize_t sht15_show_otp_reload(struct device *dev,
> > + struct device_attribute *attr,
> > + char *buf)
This attribute will be removed and available from the platform data only.
It will be documented as well.
> > +static DEVICE_ATTR(battery_alarm, S_IRUGO, sht15_show_battery, NULL);
> > +static DEVICE_ATTR(heater_enable, S_IRUGO, sht15_show_heater, NULL);
> > +static DEVICE_ATTR(otp_reload, S_IRUGO, sht15_show_otp_reload, NULL);
> Not a chance any attribute with that incomprehensible a name is going
> to be merged. For reference of others its a question of whether magic
> component specific corrections are applied or not.
>
This will be updated.
> > +static DEVICE_ATTR(temp_resolution,
> > + S_IRUGO,
> > + sht15_show_temp_resolution,
> > + NULL);
> > +static DEVICE_ATTR(humidity_resolution,
> > + S_IRUGO,
> > + sht15_show_humidity_resolution,
> > + NULL);
> Probably do resolutions through platform data. Might be some
> usecases for dynamically varying them but is it worth while
> supporting? Not sure. One for discussion when you propose some
> documentation for these.
>
Yes, resolution will be set in platform data.
Regards,
Vivien.
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [lm-sensors] [RFC PATCH 2/3] hwmon: sht15: add support for
2011-03-16 18:44 [lm-sensors] [RFC PATCH 2/3] hwmon: sht15: add support for reading Vivien Didelot
2011-03-21 19:43 ` [lm-sensors] [RFC PATCH 2/3] hwmon: sht15: add support for Jonathan Cameron
2011-03-22 0:00 ` Vivien Didelot
@ 2011-03-22 0:07 ` Guenter Roeck
2011-03-22 10:28 ` Jonathan Cameron
` (6 subsequent siblings)
9 siblings, 0 replies; 11+ messages in thread
From: Guenter Roeck @ 2011-03-22 0:07 UTC (permalink / raw)
To: lm-sensors
On Mon, 2011-03-21 at 20:00 -0400, Vivien Didelot wrote:
> Hi Jonathan.
> On Mon, Mar 21, 2011 at 07:43:59PM +0000, Jonathan Cameron wrote:
> > On 03/16/11 18:44, Vivien Didelot wrote:
> > > Add support to read the sht15 status register. Explicitly:
> > > * The measurement resolution through the temp_resolution and
> > > humidity_resolution sysfs attributes;
> > > * An end of battery indicator through the battery_alarm sysfs attribute;
> > > * Embedded heater support using the heater_enable sysfs attribute;
> > > * Reload from OTP available through the otp_reload sysfs attribute.
> > Hi Vivien.
> >
> > As Guenter said, these definitely need documenting.. Once that is available
> > I expect people will debate naming etc.
> Sure. After discussing about that, checksumming, otp_reload and
> *_resolution attributes have been removed. battery_alarm will be splitted into
> temp1_alarm and humidity1_alarm to respect the convention.
>
> I'll also add a documentation in Documentation/hwmon/ for the device.
>
> > > #define SHT15_MEASURE_TEMP 0x03
> > > #define SHT15_MEASURE_RH 0x05
> > > #define SHT15_SOFT_RESET 0x1E
> > > +#define SHT15_READ_STATUS 0x07
> > Keep this in numerical order.
> Ok.
>
> > Why cached the raw status? Can't see any users.
> > > + u8 val_status;
> As you've seen, it will be used in the next patch.
>
> > Bit field for all these plesae as they are flags - might as well
> > allow for possibility of saving a bit of space.
> > > + u8 end_of_battery;
> > > + u8 heater;
> > > + u8 no_otp_reload;
> > > + u8 temp_resolution;
> > > + u8 humidity_resolution;
> Ok, I'll check it out.
>
> > make it easier to read with !!(status & SHT15_STATUS_BATTERY);
> > etc.
> > > + data->end_of_battery = (status & SHT15_STATUS_BATTERY) >> 6;
> > > + data->heater = (status & SHT15_STATUS_HEATER) >> 2;
> > > + data->no_otp_reload = (status & SHT15_STATUS_OTP_RELOAD) >> 1;
> Good idea.
>
> > Given these are yoked together could just have a single flag and
> > interpret it in the few places that care?
> > > + if (status & SHT15_STATUS_RESOLUTION) {
> > > + data->temp_resolution = 12;
> > > + data->humidity_resolution = 8;
> > > + } else {
> > > + data->temp_resolution = 14;
> > > + data->humidity_resolution = 12;
> > > + }
> Regarding one of your comment in the next patch, it makes sense to use an
> explicit single flag, so I'll remove this as well.
>
> > > /**
> > > @@ -378,6 +460,9 @@ static int sht15_update_vals(struct sht15_data *data)
> > > mutex_lock(&data->read_lock);
> > > if (time_after(jiffies, data->last_updat + timeout)
> > > || !data->valid) {
> > As with the previous version, I'd make this optional controlled
> > by platform data. Not everyone cares and it just made their gpio's
> > jump up and down a fair bit more. Obviously if it's turned off, you
> > will also want to change what attributes are registered.
> > > + ret = sht15_update_status(data);
> > > + if (ret)
> > > + goto error_ret;
> I put the sht15_update_status() call here so it could have benefit of the
> jiffies calculation. I also thought that it could be useful to update the
> device state before each measurement, in order to notice an eventual low
> power notice. Maybe I'm wrong and this test should be done separately?
>
> > > + /* Technically no need to read temperature and humidity as well */
> > Given we do a fresh read here, why bother having the cache at all? Just
> > read it explicitly when we care.
> > > + ret = sht15_update_vals(data);
> According to the previous comment, what about adding a mask as an argument
> to the sht15_update_vals() function to know which measurement to do (only
> status, temperature + humidity, etc.)? Because the discussion could also
> be pointed on the temperature measurement, which update unnecessarily the
> humidity as well.
>
> > I've no idea off the top of my head what OTP is so perhaps a more
> > informative name or a suitable comment?
> > > +static ssize_t sht15_show_otp_reload(struct device *dev,
> > > + struct device_attribute *attr,
> > > + char *buf)
> This attribute will be removed and available from the platform data only.
> It will be documented as well.
>
> > > +static DEVICE_ATTR(battery_alarm, S_IRUGO, sht15_show_battery, NULL);
> > > +static DEVICE_ATTR(heater_enable, S_IRUGO, sht15_show_heater, NULL);
> > > +static DEVICE_ATTR(otp_reload, S_IRUGO, sht15_show_otp_reload, NULL);
> > Not a chance any attribute with that incomprehensible a name is going
> > to be merged. For reference of others its a question of whether magic
> > component specific corrections are applied or not.
> >
> This will be updated.
>
A thought on the remaining attributes - if related to the humidity
sensor, it might make sense to prepend the attribute names with
"humidity1_". For example, you could have "humidity1_heater".
Does this make sense ?
Thanks,
Guenter
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [lm-sensors] [RFC PATCH 2/3] hwmon: sht15: add support for
2011-03-16 18:44 [lm-sensors] [RFC PATCH 2/3] hwmon: sht15: add support for reading Vivien Didelot
` (2 preceding siblings ...)
2011-03-22 0:07 ` Guenter Roeck
@ 2011-03-22 10:28 ` Jonathan Cameron
2011-03-22 14:28 ` Guenter Roeck
` (5 subsequent siblings)
9 siblings, 0 replies; 11+ messages in thread
From: Jonathan Cameron @ 2011-03-22 10:28 UTC (permalink / raw)
To: lm-sensors
On 03/22/11 00:00, Vivien Didelot wrote:
> Hi Jonathan.
> On Mon, Mar 21, 2011 at 07:43:59PM +0000, Jonathan Cameron wrote:
>> On 03/16/11 18:44, Vivien Didelot wrote:
>>> Add support to read the sht15 status register. Explicitly:
>>> * The measurement resolution through the temp_resolution and
>>> humidity_resolution sysfs attributes;
>>> * An end of battery indicator through the battery_alarm sysfs attribute;
>>> * Embedded heater support using the heater_enable sysfs attribute;
>>> * Reload from OTP available through the otp_reload sysfs attribute.
>> Hi Vivien.
>>
>> As Guenter said, these definitely need documenting.. Once that is available
>> I expect people will debate naming etc.
> Sure. After discussing about that, checksumming, otp_reload and
> *_resolution attributes have been removed. battery_alarm will be splitted into
> temp1_alarm and humidity1_alarm to respect the convention.
Really, Guenter / Jean is the is the right option? The alarm isn't really about
either of the individual sensors, but rather about power to the chip?
>
> I'll also add a documentation in Documentation/hwmon/ for the device.
...
>> Why cached the raw status? Can't see any users.
>>> + u8 val_status;
> As you've seen, it will be used in the next patch.
Ideally it would therefore be introduced in the next patch, but that's
a minor point.
...
>>> /**
>>> @@ -378,6 +460,9 @@ static int sht15_update_vals(struct sht15_data *data)
>>> mutex_lock(&data->read_lock);
>>> if (time_after(jiffies, data->last_updat + timeout)
>>> || !data->valid) {
>> As with the previous version, I'd make this optional controlled
>> by platform data. Not everyone cares and it just made their gpio's
>> jump up and down a fair bit more. Obviously if it's turned off, you
>> will also want to change what attributes are registered.
>>> + ret = sht15_update_status(data);
>>> + if (ret)
>>> + goto error_ret;
> I put the sht15_update_status() call here so it could have benefit of the
> jiffies calculation. I also thought that it could be useful to update the
> device state before each measurement, in order to notice an eventual low
> power notice. Maybe I'm wrong and this test should be done separately?
The question is not whether it is worth doing in some cases, but rather
whether this is a change that will effect anyone in a negative fashion.
Its a balance between providing more error checking vs extra transactions
on what may be an extremely slow bus.
Lets go with butting this in.
>
>>> + /* Technically no need to read temperature and humidity as well */
>> Given we do a fresh read here, why bother having the cache at all? Just
>> read it explicitly when we care.
>>> + ret = sht15_update_vals(data);
> According to the previous comment, what about adding a mask as an argument
> to the sht15_update_vals() function to know which measurement to do (only
> status, temperature + humidity, etc.)? Because the discussion could also
> be pointed on the temperature measurement, which update unnecessarily the
> humidity as well.
Good idea.
...
Jonathan
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [lm-sensors] [RFC PATCH 2/3] hwmon: sht15: add support for
2011-03-16 18:44 [lm-sensors] [RFC PATCH 2/3] hwmon: sht15: add support for reading Vivien Didelot
` (3 preceding siblings ...)
2011-03-22 10:28 ` Jonathan Cameron
@ 2011-03-22 14:28 ` Guenter Roeck
2011-03-26 17:16 ` Jean Delvare
` (4 subsequent siblings)
9 siblings, 0 replies; 11+ messages in thread
From: Guenter Roeck @ 2011-03-22 14:28 UTC (permalink / raw)
To: lm-sensors
On Tue, Mar 22, 2011 at 06:28:40AM -0400, Jonathan Cameron wrote:
> On 03/22/11 00:00, Vivien Didelot wrote:
> > Hi Jonathan.
> > On Mon, Mar 21, 2011 at 07:43:59PM +0000, Jonathan Cameron wrote:
> >> On 03/16/11 18:44, Vivien Didelot wrote:
> >>> Add support to read the sht15 status register. Explicitly:
> >>> * The measurement resolution through the temp_resolution and
> >>> humidity_resolution sysfs attributes;
> >>> * An end of battery indicator through the battery_alarm sysfs attribute;
> >>> * Embedded heater support using the heater_enable sysfs attribute;
> >>> * Reload from OTP available through the otp_reload sysfs attribute.
> >> Hi Vivien.
> >>
> >> As Guenter said, these definitely need documenting.. Once that is available
> >> I expect people will debate naming etc.
> > Sure. After discussing about that, checksumming, otp_reload and
> > *_resolution attributes have been removed. battery_alarm will be splitted into
> > temp1_alarm and humidity1_alarm to respect the convention.
> Really, Guenter / Jean is the is the right option? The alarm isn't really about
> either of the individual sensors, but rather about power to the chip?
I had suggested humidity1_alarm. Two alarms for one condition is overkill.
I find it better to have a standard attribute which a standard program such as
"sensors" may have a chance to display at some point. Jean may think differently -
we did not discuss the matter.
Guenter
> >
> > I'll also add a documentation in Documentation/hwmon/ for the device.
> ...
> >> Why cached the raw status? Can't see any users.
> >>> + u8 val_status;
> > As you've seen, it will be used in the next patch.
> Ideally it would therefore be introduced in the next patch, but that's
> a minor point.
> ...
>
> >>> /**
> >>> @@ -378,6 +460,9 @@ static int sht15_update_vals(struct sht15_data *data)
> >>> mutex_lock(&data->read_lock);
> >>> if (time_after(jiffies, data->last_updat + timeout)
> >>> || !data->valid) {
> >> As with the previous version, I'd make this optional controlled
> >> by platform data. Not everyone cares and it just made their gpio's
> >> jump up and down a fair bit more. Obviously if it's turned off, you
> >> will also want to change what attributes are registered.
> >>> + ret = sht15_update_status(data);
> >>> + if (ret)
> >>> + goto error_ret;
> > I put the sht15_update_status() call here so it could have benefit of the
> > jiffies calculation. I also thought that it could be useful to update the
> > device state before each measurement, in order to notice an eventual low
> > power notice. Maybe I'm wrong and this test should be done separately?
> The question is not whether it is worth doing in some cases, but rather
> whether this is a change that will effect anyone in a negative fashion.
> Its a balance between providing more error checking vs extra transactions
> on what may be an extremely slow bus.
>
> Lets go with butting this in.
> >
> >>> + /* Technically no need to read temperature and humidity as well */
> >> Given we do a fresh read here, why bother having the cache at all? Just
> >> read it explicitly when we care.
> >>> + ret = sht15_update_vals(data);
> > According to the previous comment, what about adding a mask as an argument
> > to the sht15_update_vals() function to know which measurement to do (only
> > status, temperature + humidity, etc.)? Because the discussion could also
> > be pointed on the temperature measurement, which update unnecessarily the
> > humidity as well.
> Good idea.
> ...
>
> Jonathan
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [lm-sensors] [RFC PATCH 2/3] hwmon: sht15: add support for
2011-03-16 18:44 [lm-sensors] [RFC PATCH 2/3] hwmon: sht15: add support for reading Vivien Didelot
` (4 preceding siblings ...)
2011-03-22 14:28 ` Guenter Roeck
@ 2011-03-26 17:16 ` Jean Delvare
2011-03-26 20:39 ` Jean Delvare
` (3 subsequent siblings)
9 siblings, 0 replies; 11+ messages in thread
From: Jean Delvare @ 2011-03-26 17:16 UTC (permalink / raw)
To: lm-sensors
On Tue, 22 Mar 2011 07:28:56 -0700, Guenter Roeck wrote:
> On Tue, Mar 22, 2011 at 06:28:40AM -0400, Jonathan Cameron wrote:
> > On 03/22/11 00:00, Vivien Didelot wrote:
> > > Sure. After discussing about that, checksumming, otp_reload and
> > > *_resolution attributes have been removed. battery_alarm will be splitted into
> > > temp1_alarm and humidity1_alarm to respect the convention.
> > Really, Guenter / Jean is the is the right option? The alarm isn't really about
> > either of the individual sensors, but rather about power to the chip?
>
> I had suggested humidity1_alarm. Two alarms for one condition is overkill.
> I find it better to have a standard attribute which a standard program such as
> "sensors" may have a chance to display at some point. Jean may think differently -
> we did not discuss the matter.
Sorry for joining a little late in the game. If I understand properly
the meaning of the "end of battery" status bit, humidity1_alarm is not
a suitable name. The *_alarm attributes are for off-limit measurements,
but the SHT1x has no limits in the first place. The "end of battery"
status bit means "measurements may be invalid", so I would map it to
humidity1_fault and temp1_fault. The later is already standardized, and
it would be easy to introduce the former. The usual meaning of
temp*_fault is a little different (broken / open / short sensor) but
its standard definition is fairly generic.
When the fault flag is set, "sensors" will display "FAULT" instead of
the measurement value, which is the right thing to do here if the
measurements are unreliable.
And there is nothing wrong with mapping a single register bit to two
attributes, if it makes sense as is the case here. I think we already
did it in the past for other chips.
--
Jean Delvare
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [lm-sensors] [RFC PATCH 2/3] hwmon: sht15: add support for
2011-03-16 18:44 [lm-sensors] [RFC PATCH 2/3] hwmon: sht15: add support for reading Vivien Didelot
` (5 preceding siblings ...)
2011-03-26 17:16 ` Jean Delvare
@ 2011-03-26 20:39 ` Jean Delvare
2011-03-26 23:29 ` Guenter Roeck
` (2 subsequent siblings)
9 siblings, 0 replies; 11+ messages in thread
From: Jean Delvare @ 2011-03-26 20:39 UTC (permalink / raw)
To: lm-sensors
Hi Vivien, Jonathan,
On Tue, 22 Mar 2011 10:28:40 +0000, Jonathan Cameron wrote:
> On 03/22/11 00:00, Vivien Didelot wrote:
> > Hi Jonathan.
> > On Mon, Mar 21, 2011 at 07:43:59PM +0000, Jonathan Cameron wrote:
> >> On 03/16/11 18:44, Vivien Didelot wrote:
> >>> /**
> >>> @@ -378,6 +460,9 @@ static int sht15_update_vals(struct sht15_data *data)
> >>> mutex_lock(&data->read_lock);
> >>> if (time_after(jiffies, data->last_updat + timeout)
> >>> || !data->valid) {
> >> As with the previous version, I'd make this optional controlled
> >> by platform data. Not everyone cares and it just made their gpio's
> >> jump up and down a fair bit more. Obviously if it's turned off, you
> >> will also want to change what attributes are registered.
I don't think it makes sense to let the platform data decide of this.
On the same system, different users may have different usage patterns
for the sysfs attributes.
> >>> + ret = sht15_update_status(data);
> >>> + if (ret)
> >>> + goto error_ret;
> > I put the sht15_update_status() call here so it could have benefit of the
> > jiffies calculation. I also thought that it could be useful to update the
> > device state before each measurement, in order to notice an eventual low
> > power notice. Maybe I'm wrong and this test should be done separately?
> The question is not whether it is worth doing in some cases, but rather
> whether this is a change that will effect anyone in a negative fashion.
> Its a balance between providing more error checking vs extra transactions
> on what may be an extremely slow bus.
>
> Lets go with butting this in.
> >
> >>> + /* Technically no need to read temperature and humidity as well */
> >> Given we do a fresh read here, why bother having the cache at all? Just
> >> read it explicitly when we care.
> >>> + ret = sht15_update_vals(data);
> > According to the previous comment, what about adding a mask as an argument
> > to the sht15_update_vals() function to know which measurement to do (only
> > status, temperature + humidity, etc.)? Because the discussion could also
> > be pointed on the temperature measurement, which update unnecessarily the
> > humidity as well.
> Good idea.
> ...
It boils down to coming up with a caching strategy which is optimal for
the use case. Historically, hwmon drivers have grouped register access
and have cached register values for three reasons.
The first reason is that some (old) sensor chips would stop sampling
when I2C was accessed, so letting any user read values continuously
from registers would let them DoS the monitoring, possibly with very
nasty effects (system overheating going unnoticed.) I don't think this
applies to the SHT1x, as sampling isn't done in the background to start
with.
The second reason is the slow access to registers. This holds for both
I2C and SPI devices, and definitely applies here. That being said, it
also depends on the number and size of registers. While we have I2C
hwmon devices with ~50 registers, The SHT1x only has 40 bits worth of
register values, this is really a small amount, so even with slow
register access, performance shouldn't suffer much. So I'm not entirely
sure if the ongoing discussing is really warranted.
The third reason is that sometimes different sysfs attributes map to
the same common register, so caching avoids duplicate register reads
(which is even more important for self-clearing registers.) For the
SHT1x, this would apply to temp1_fault and humidity1_fault, if we go
with these attributes as discussed elsewhere in this thread.
Anyway, the important thing to remember is that there is no universal
caching strategy. Different drivers have different implementations.
Some cache the values, some don't. Some have a single cache for all
register values (and thus a unique cache lifetime) while some partition
the registers in groups, and each group has its own cache lifetime. So
for the sht15 driver, depending on the most frequent usage pattern,
the current caching strategy (single cache for all registers and 1
second lifetime) may or may not be the most appropriate one.
One thing worth keeping in mind when deciding this is that "sensors"
will always read from all available attributes. So if it is the main
target, having a common cache for all register values needed by the
standard hwmon attributes makes sense. However, if there are other
targets (other monitoring tools with different access patterns, or
scripts accessing a single attribute repeatedly) then a different
approach may make more sense.
HTH,
--
Jean Delvare
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [lm-sensors] [RFC PATCH 2/3] hwmon: sht15: add support for
2011-03-16 18:44 [lm-sensors] [RFC PATCH 2/3] hwmon: sht15: add support for reading Vivien Didelot
` (6 preceding siblings ...)
2011-03-26 20:39 ` Jean Delvare
@ 2011-03-26 23:29 ` Guenter Roeck
2011-03-28 16:46 ` Vivien Didelot
2011-03-28 19:19 ` Jean Delvare
9 siblings, 0 replies; 11+ messages in thread
From: Guenter Roeck @ 2011-03-26 23:29 UTC (permalink / raw)
To: lm-sensors
On Sat, Mar 26, 2011 at 01:16:00PM -0400, Jean Delvare wrote:
> On Tue, 22 Mar 2011 07:28:56 -0700, Guenter Roeck wrote:
> > On Tue, Mar 22, 2011 at 06:28:40AM -0400, Jonathan Cameron wrote:
> > > On 03/22/11 00:00, Vivien Didelot wrote:
> > > > Sure. After discussing about that, checksumming, otp_reload and
> > > > *_resolution attributes have been removed. battery_alarm will be splitted into
> > > > temp1_alarm and humidity1_alarm to respect the convention.
> > > Really, Guenter / Jean is the is the right option? The alarm isn't really about
> > > either of the individual sensors, but rather about power to the chip?
> >
> > I had suggested humidity1_alarm. Two alarms for one condition is overkill.
> > I find it better to have a standard attribute which a standard program such as
> > "sensors" may have a chance to display at some point. Jean may think differently -
> > we did not discuss the matter.
>
> Sorry for joining a little late in the game. If I understand properly
No problem - as always I appreciate your insight.
> the meaning of the "end of battery" status bit, humidity1_alarm is not
> a suitable name. The *_alarm attributes are for off-limit measurements,
> but the SHT1x has no limits in the first place. The "end of battery"
> status bit means "measurements may be invalid", so I would map it to
> humidity1_fault and temp1_fault. The later is already standardized, and
> it would be easy to introduce the former. The usual meaning of
> temp*_fault is a little different (broken / open / short sensor) but
> its standard definition is fairly generic.
>
> When the fault flag is set, "sensors" will display "FAULT" instead of
> the measurement value, which is the right thing to do here if the
> measurements are unreliable.
>
> And there is nothing wrong with mapping a single register bit to two
> attributes, if it makes sense as is the case here. I think we already
> did it in the past for other chips.
>
Ok.
Thanks,
Guenter
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [lm-sensors] [RFC PATCH 2/3] hwmon: sht15: add support for
2011-03-16 18:44 [lm-sensors] [RFC PATCH 2/3] hwmon: sht15: add support for reading Vivien Didelot
` (7 preceding siblings ...)
2011-03-26 23:29 ` Guenter Roeck
@ 2011-03-28 16:46 ` Vivien Didelot
2011-03-28 19:19 ` Jean Delvare
9 siblings, 0 replies; 11+ messages in thread
From: Vivien Didelot @ 2011-03-28 16:46 UTC (permalink / raw)
To: lm-sensors
Hi,
On Sat, Mar 26, 2011 at 09:39:08PM +0100, Jean Delvare wrote:
> Hi Vivien, Jonathan,
>
> On Tue, 22 Mar 2011 10:28:40 +0000, Jonathan Cameron wrote:
> > On 03/22/11 00:00, Vivien Didelot wrote:
> > > Hi Jonathan.
> > > On Mon, Mar 21, 2011 at 07:43:59PM +0000, Jonathan Cameron wrote:
> > >> On 03/16/11 18:44, Vivien Didelot wrote:
> > >>> /**
> > >>> @@ -378,6 +460,9 @@ static int sht15_update_vals(struct sht15_data *data)
> > >>> mutex_lock(&data->read_lock);
> > >>> if (time_after(jiffies, data->last_updat + timeout)
> > >>> || !data->valid) {
> > >> As with the previous version, I'd make this optional controlled
> > >> by platform data. Not everyone cares and it just made their gpio's
> > >> jump up and down a fair bit more. Obviously if it's turned off, you
> > >> will also want to change what attributes are registered.
>
> I don't think it makes sense to let the platform data decide of this.
> On the same system, different users may have different usage patterns
> for the sysfs attributes.
>
For this sht15_update_vals() function, I've decided to add a mask to
specify which value should be read. i.e. the status register, the
temperature and/or the humidity. For example, sht15_show_battery() will
call sht15_update_vals(data, SHT15_UPDATE_STATE) and
sht15_show_humidity() will call sht15_update_vals(data,
SHT15_UPDATE_TEMP | SHT15_UPDATE_HUMIDITY) as the humidity calculation
depends on the temperature. Btw, it avoids reading the humidity when
only the temperature is requested.
> > >>> + ret = sht15_update_status(data);
> > >>> + if (ret)
> > >>> + goto error_ret;
> > > I put the sht15_update_status() call here so it could have benefit of the
> > > jiffies calculation. I also thought that it could be useful to update the
> > > device state before each measurement, in order to notice an eventual low
> > > power notice. Maybe I'm wrong and this test should be done separately?
> > The question is not whether it is worth doing in some cases, but rather
> > whether this is a change that will effect anyone in a negative fashion.
> > Its a balance between providing more error checking vs extra transactions
> > on what may be an extremely slow bus.
> >
> > Lets go with butting this in.
> > >
> > >>> + /* Technically no need to read temperature and humidity as well */
> > >> Given we do a fresh read here, why bother having the cache at all? Just
> > >> read it explicitly when we care.
> > >>> + ret = sht15_update_vals(data);
> > > According to the previous comment, what about adding a mask as an argument
> > > to the sht15_update_vals() function to know which measurement to do (only
> > > status, temperature + humidity, etc.)? Because the discussion could also
> > > be pointed on the temperature measurement, which update unnecessarily the
> > > humidity as well.
> > Good idea.
> > ...
>
> It boils down to coming up with a caching strategy which is optimal for
> the use case. Historically, hwmon drivers have grouped register access
> and have cached register values for three reasons.
>
> The first reason is that some (old) sensor chips would stop sampling
> when I2C was accessed, so letting any user read values continuously
> from registers would let them DoS the monitoring, possibly with very
> nasty effects (system overheating going unnoticed.) I don't think this
> applies to the SHT1x, as sampling isn't done in the background to start
> with.
>
> The second reason is the slow access to registers. This holds for both
> I2C and SPI devices, and definitely applies here. That being said, it
> also depends on the number and size of registers. While we have I2C
> hwmon devices with ~50 registers, The SHT1x only has 40 bits worth of
> register values, this is really a small amount, so even with slow
> register access, performance shouldn't suffer much. So I'm not entirely
> sure if the ongoing discussing is really warranted.
>
> The third reason is that sometimes different sysfs attributes map to
> the same common register, so caching avoids duplicate register reads
> (which is even more important for self-clearing registers.) For the
> SHT1x, this would apply to temp1_fault and humidity1_fault, if we go
> with these attributes as discussed elsewhere in this thread.
>
> Anyway, the important thing to remember is that there is no universal
> caching strategy. Different drivers have different implementations.
> Some cache the values, some don't. Some have a single cache for all
> register values (and thus a unique cache lifetime) while some partition
> the registers in groups, and each group has its own cache lifetime. So
> for the sht15 driver, depending on the most frequent usage pattern,
> the current caching strategy (single cache for all registers and 1
> second lifetime) may or may not be the most appropriate one.
>
> One thing worth keeping in mind when deciding this is that "sensors"
> will always read from all available attributes. So if it is the main
> target, having a common cache for all register values needed by the
> standard hwmon attributes makes sense. However, if there are other
> targets (other monitoring tools with different access patterns, or
> scripts accessing a single attribute repeatedly) then a different
> approach may make more sense.
>
> HTH,
> --
> Jean Delvare
Another reason could be that some sensors should not be solicited too
much to prevent bad data readings. As an example, the datasheet says:
"Important: To keep self heating below 0.1°C, SHT1x
should not be active for more than 10% of the time – e.g.
maximum one measurement per second at 12bit accuracy
shall be made.". So caching in this case is crucial.
Regards,
Vivien.
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [lm-sensors] [RFC PATCH 2/3] hwmon: sht15: add support for
2011-03-16 18:44 [lm-sensors] [RFC PATCH 2/3] hwmon: sht15: add support for reading Vivien Didelot
` (8 preceding siblings ...)
2011-03-28 16:46 ` Vivien Didelot
@ 2011-03-28 19:19 ` Jean Delvare
9 siblings, 0 replies; 11+ messages in thread
From: Jean Delvare @ 2011-03-28 19:19 UTC (permalink / raw)
To: lm-sensors
On Mon, 28 Mar 2011 12:46:55 -0400, Vivien Didelot wrote:
> On Sat, Mar 26, 2011 at 09:39:08PM +0100, Jean Delvare wrote:
> > I don't think it makes sense to let the platform data decide of this.
> > On the same system, different users may have different usage patterns
> > for the sysfs attributes.
>
> For this sht15_update_vals() function, I've decided to add a mask to
> specify which value should be read. i.e. the status register, the
> temperature and/or the humidity. For example, sht15_show_battery() will
> call sht15_update_vals(data, SHT15_UPDATE_STATE) and
> sht15_show_humidity() will call sht15_update_vals(data,
> SHT15_UPDATE_TEMP | SHT15_UPDATE_HUMIDITY) as the humidity calculation
> depends on the temperature. Btw, it avoids reading the humidity when
> only the temperature is requested.
That's interesting, I don't think any other driver ever implemented
things this way, but it should work. You'll have to use a separate cache
timestamp and validity flag for each register though, but given the
low number of register, it shouldn't be a problem in practice.
> > (...)
> > It boils down to coming up with a caching strategy which is optimal for
> > the use case. Historically, hwmon drivers have grouped register access
> > and have cached register values for three reasons.
> >
> > The first reason is that some (old) sensor chips would stop sampling
> > when I2C was accessed, so letting any user read values continuously
> > from registers would let them DoS the monitoring, possibly with very
> > nasty effects (system overheating going unnoticed.) I don't think this
> > applies to the SHT1x, as sampling isn't done in the background to start
> > with.
> >
> > The second reason is the slow access to registers. This holds for both
> > I2C and SPI devices, and definitely applies here. That being said, it
> > also depends on the number and size of registers. While we have I2C
> > hwmon devices with ~50 registers, The SHT1x only has 40 bits worth of
> > register values, this is really a small amount, so even with slow
> > register access, performance shouldn't suffer much. So I'm not entirely
> > sure if the ongoing discussing is really warranted.
> >
> > The third reason is that sometimes different sysfs attributes map to
> > the same common register, so caching avoids duplicate register reads
> > (which is even more important for self-clearing registers.) For the
> > SHT1x, this would apply to temp1_fault and humidity1_fault, if we go
> > with these attributes as discussed elsewhere in this thread.
> >
> > Anyway, the important thing to remember is that there is no universal
> > caching strategy. Different drivers have different implementations.
> > Some cache the values, some don't. Some have a single cache for all
> > register values (and thus a unique cache lifetime) while some partition
> > the registers in groups, and each group has its own cache lifetime. So
> > for the sht15 driver, depending on the most frequent usage pattern,
> > the current caching strategy (single cache for all registers and 1
> > second lifetime) may or may not be the most appropriate one.
> >
> > One thing worth keeping in mind when deciding this is that "sensors"
> > will always read from all available attributes. So if it is the main
> > target, having a common cache for all register values needed by the
> > standard hwmon attributes makes sense. However, if there are other
> > targets (other monitoring tools with different access patterns, or
> > scripts accessing a single attribute repeatedly) then a different
> > approach may make more sense.
>
> Another reason could be that some sensors should not be solicited too
> much to prevent bad data readings. As an example, the datasheet says:
> "Important: To keep self heating below 0.1°C, SHT1x
> should not be active for more than 10% of the time – e.g.
> maximum one measurement per second at 12bit accuracy
> shall be made.". So caching in this case is crucial.
Good point.
--
Jean Delvare
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 11+ messages in thread