Hi Chris,
On Fri, Dec 23, 2011 at 05:15:43AM -0500, kg4ysn@xxxxxxxxx wrote:
> From: Chris Verges <kg4ysn@xxxxxxxxx>
>
> The LM73 sensor supports four resolutions (11-bit to 14-bit). The
> number of bits used are controlled via the control/status register on
> the LM73. This patch adds a sysfs interface to manipulate the
> appropriate bits in the control/status register.
>
> Example usage:
>
> # set resolution to 11-bit (default)
> echo "11" > /sys/class/hwmon/hwmon0/device/temp1_resolution
>
> # set resolution to 14-bit
> echo "14" > /sys/class/hwmon/hwmon0/device/temp1_resolution
>
> # read current resolution
> cat /sys/class/hwmon/hwmon0/device/temp1_resolution
>
> Signed-off-by: Chris Verges <kg4ysn@xxxxxxxxx>
Question is why not just choose 14 bit resolution all the time. The only reason
I can see is that this would affect conversion time .. which it does, though the
datasheet makes a pretty good effort to hide it.
Given that, it would make more sense to add support for the existing
update_interval attribute, introduce a valid flag as well as a last_updated
variable, and make sure that reads from the chip are spaced at least as much
as update_interval apart. This is, for example, implemented in the lm90 driver.
Couple of additional comments below.
> ---
> drivers/hwmon/lm73.c | 60 ++++++++++++++++++++++++++++++++++++++++++++++++-
> 1 files changed, 58 insertions(+), 2 deletions(-)
>
If you introduce a new sysfs attribute, you would have to introduce it in
Documentation/hwmon/sysfs-interface first.
> diff --git a/drivers/hwmon/lm73.c b/drivers/hwmon/lm73.c
> index 9e64d96..e9fb992 100644
> --- a/drivers/hwmon/lm73.c
> +++ b/drivers/hwmon/lm73.c
> @@ -73,6 +73,61 @@ static ssize_t show_temp(struct device *dev, struct device_attribute *da,
> return sprintf(buf, "%d\n", temp);
> }
>
> +static ssize_t set_resolution(struct device *dev,
> + struct device_attribute *da, const char *buf,
> + size_t count)
> +{
> + struct sensor_device_attribute *attr = to_sensor_dev_attr(da);
> + struct i2c_client *client = to_i2c_client(dev);
> + long value;
> + int reg = strict_strtol(buf, 10, &value);
> + if (reg < 0)
> + return reg;
> +
> + if (value < 11 || value > 14)
> + return -EINVAL;
> +
Even if we were to accept the new attribute (which is unlikely), the value range
is not portable (what does "14" mean ?). You would have to use something like
milli-degrees C to describe the desired accuracy.
> + /* Read the current control/status register */
> + reg = i2c_smbus_read_byte_data(client, attr->index);
> + if (reg < 0)
> + return -EIO;
> +
> + reg &= 0x7F; /* Remove all non-essential bits */
> +
> + /*
> + * Control/Status register bit description:
> + *
> + * D[7] Time-Out Disable
> + * D[6:5] Temperature Resolution
> + * 00: 0.25 C/LSB
> + * 01: 0.125 C/LSB
> + * 10: 0.0625 C/LSB
> + * 11: 0.03125 C/LSB
> + * D[4] Reserved
> + * D[3] Alert Pin Status
> + * D[2] Temperature High Flag
> + * D[1] Temperature Low Flag
> + * D[0] Data Available Flag
> + */
> +
> + /* Convert the resolution from the integer value to the bitmask */
> + value -= 11;
> + value <<= 5;
> + value |= reg;
> +
> + /* Write the new control/status register value */
> + i2c_smbus_write_byte_data(client, attr->index, value);
> + return count;
> +}
> +
> +static ssize_t show_resolution(struct device *dev, struct device_attribute *da,
> + char *buf)
> +{
> + struct sensor_device_attribute *attr = to_sensor_dev_attr(da);
> + struct i2c_client *client = to_i2c_client(dev);
> + int value = i2c_smbus_read_byte_data(client, attr->index);
> + return sprintf(buf, "%d\n", value);
For set you convert the the desired resolution into a register value, for read
you just report whatever is in the control/status register. Doesn't make much sense.
Besides, using attr->index to store the register only makes sense if a function
is used to display the value of more than one register. In this case, you don't
need attr->index. You can access LM73_REG_CTRL directly.
> +}
>
> /*-----------------------------------------------------------------------*/
>
> @@ -84,13 +139,14 @@ static SENSOR_DEVICE_ATTR(temp1_min, S_IWUSR | S_IRUGO,
> show_temp, set_temp, LM73_REG_MIN);
> static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO,
> show_temp, NULL, LM73_REG_INPUT);
> -
> +static SENSOR_DEVICE_ATTR(temp1_resolution, S_IWUSR | S_IRUGO,
> + show_resolution, set_resolution, LM73_REG_CTRL);
>
> static struct attribute *lm73_attributes[] = {
> &sensor_dev_attr_temp1_input.dev_attr.attr,
> &sensor_dev_attr_temp1_max.dev_attr.attr,
> &sensor_dev_attr_temp1_min.dev_attr.attr,
> -
> + &sensor_dev_attr_temp1_resolution.dev_attr.attr,
> NULL
> };
>
> --
> 1.7.4.1
>
>
> From 171bd4d58d5e9ded28bf3d105a714c6d9486927b Mon Sep 17 00:00:00 2001
> From: Chris Verges <kg4ysn@xxxxxxxxx>
> Date: Fri, 23 Dec 2011 02:10:27 -0800
> Subject: [PATCH 2/2] Replaced strict_strtol with kstrtol in lm73 set_resolution()
>
> Signed-off-by: Chris Verges <kg4ysn@xxxxxxxxx>
Two patches in a single e-mail ... not a good idea. If/when you resubmit, please
split your patches into separate e-mails, possibly with an additional e-mail
to introduce the series.
> ---
> drivers/hwmon/lm73.c | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/hwmon/lm73.c b/drivers/hwmon/lm73.c
> index e9fb992..ca1eff1 100644
> --- a/drivers/hwmon/lm73.c
> +++ b/drivers/hwmon/lm73.c
> @@ -80,7 +80,7 @@ static ssize_t set_resolution(struct device *dev,
> struct sensor_device_attribute *attr = to_sensor_dev_attr(da);
> struct i2c_client *client = to_i2c_client(dev);
> long value;
> - int reg = strict_strtol(buf, 10, &value);
> + int reg = kstrtol(buf, 10, &value);
> if (reg < 0)
> return reg;
>
> --
> 1.7.4.1
>
>
> _______________________________________________
> lm-sensors mailing list
> lm-sensors@xxxxxxxxxxxxxx
> http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
_______________________________________________
lm-sensors mailing list
lm-sensors@xxxxxxxxxxxxxx
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
