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@xxxxxxxxxxxxxx
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
