On Tue, 2012-01-10 at 12:49 -0500, Jean Delvare wrote:
> Hi Guenter,
>
> On Mon, 9 Jan 2012 19:42:02 -0800, Guenter Roeck wrote:
> > From: Guenter Roeck <guenter.roeck@xxxxxxxxxxxx>
> >
> > On LM64, the external critical temperature limit is always writable. On LM96163,
> > it is writable if the chip is configured for it. Add conditional support for
> > writing the register dependent on chip type and configuration.
> >
> > Signed-off-by: Guenter Roeck <guenter.roeck@xxxxxxxxxxxx>
> > Signed-off-by: Guenter Roeck <linux@xxxxxxxxxxxx>
> > ---
> > drivers/hwmon/lm63.c | 57 +++++++++++++++++++++++++++++++++++++++++++++----
> > 1 files changed, 52 insertions(+), 5 deletions(-)
>
> Generates one build-time warning here:
>
> drivers/hwmon/lm63.c:560:2: warning: initialization from incompatible pointer type
>
> This is:
>
> 559 static const struct attribute_group lm63_group = {
> 560 .is_visible = lm63_attribute_mode,
> 561 .attrs = lm63_attributes,
> 562 };
>
> Obviously this is caused by this recent commit:
> http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=commit;h=587a1f1659e8b330b8738ef4901832a2b63f0bed
>
I'll fix and resubmit.
> Review:
>
> >
> > diff --git a/drivers/hwmon/lm63.c b/drivers/hwmon/lm63.c
> > index 24a96f8..6370e28 100644
> > --- a/drivers/hwmon/lm63.c
> > +++ b/drivers/hwmon/lm63.c
> > @@ -117,6 +117,9 @@ static const unsigned short normal_i2c[] = { 0x18, 0x4c, 0x4e, I2C_CLIENT_END };
> > (val) >= 127000 ? 127 : \
> > (val) < 0 ? ((val) - 500) / 1000 : \
> > ((val) + 500) / 1000)
> > +#define TEMP8U_TO_REG(val) ((val) <= 0 ? 0 : \
> > + (val) >= 255000 ? 255 : \
> > + ((val) + 500) / 1000)
> > #define TEMP11_FROM_REG(reg) ((reg) / 32 * 125)
> > #define TEMP11_TO_REG(val) ((val) <= -128000 ? 0x8000 : \
> > (val) >= 127875 ? 0x7FE0 : \
> > @@ -333,6 +336,31 @@ static ssize_t set_local_temp8(struct device *dev,
> > return count;
> > }
> >
> > +static ssize_t set_remote_temp8(struct device *dev,
> > + struct device_attribute *devattr,
> > + const char *buf, size_t count)
> > +{
> > + struct i2c_client *client = to_i2c_client(dev);
> > + struct lm63_data *data = i2c_get_clientdata(client);
> > + long val;
> > + int err;
> > +
> > + err = kstrtol(buf, 10, &val);
> > + if (err)
> > + return err;
> > +
> > + mutex_lock(&data->update_lock);
> > + if (data->remote_unsigned)
> > + data->temp8[2] = TEMP8U_TO_REG(val - data->temp2_offset);
> > + else
> > + data->temp8[2] = TEMP8_TO_REG(val - data->temp2_offset);
> > +
> > + i2c_smbus_write_byte_data(client, LM63_REG_REMOTE_TCRIT,
> > + data->temp8[2]);
> > + mutex_unlock(&data->update_lock);
> > + return count;
> > +}
>
> The code is correct, but wouldn't it make sense to merge
> set_local_temp8() and set_remote_temp8() into a single function?
> There's a lot of common code between these two functions.
>
I'll give it a shot.
> > +
> > static ssize_t show_temp11(struct device *dev, struct device_attribute *devattr,
> > char *buf)
> > {
> > @@ -470,12 +498,8 @@ static SENSOR_DEVICE_ATTR(temp2_max, S_IWUSR | S_IRUGO, show_temp11,
> > set_temp11, 2);
> > static SENSOR_DEVICE_ATTR(temp2_offset, S_IWUSR | S_IRUGO, show_temp11,
> > set_temp11, 3);
> > -/*
> > - * On LM63, temp2_crit can be set only once, which should be job
> > - * of the bootloader.
> > - */
> > static SENSOR_DEVICE_ATTR(temp2_crit, S_IRUGO, show_remote_temp8,
> > - NULL, 2);
> > + set_remote_temp8, 2);
> > static DEVICE_ATTR(temp2_crit_hyst, S_IWUSR | S_IRUGO, show_temp2_crit_hyst,
> > set_temp2_crit_hyst);
> >
> > @@ -510,7 +534,30 @@ static struct attribute *lm63_attributes[] = {
> > NULL
> > };
> >
> > +/*
> > + * On LM63, temp2_crit can be set only once, which should be job
> > + * of the bootloader.
> > + * On LM64, temp2_crit can always be set.
> > + * On LM96163, temp2_crit can be set if bit 1 of the configuration
> > + * register is true.
> > + */
> > +static mode_t lm63_attribute_mode(struct kobject *kobj,
> > + struct attribute *attr, int index)
> > +{
> > + struct device *dev = container_of(kobj, struct device, kobj);
> > + struct i2c_client *client = to_i2c_client(dev);
> > + struct lm63_data *data = i2c_get_clientdata(client);
> > +
> > + if (attr == &sensor_dev_attr_temp2_crit.dev_attr.attr
> > + && (data->kind == lm64 || (data->kind == lm96163
> > + && (data->config & 0x02))))
>
> I think this would be more readable as:
>
> && (data->kind == lm64 ||
> (data->kind == lm96163 && (data->config & 0x02))))
>
>
Ok.
> > + return attr->mode | S_IWUSR;
> FWIW I think you can save a few binary bytes by using instead:
>
> attr->mode |= S_IWUSR;
>
> Thanks to the single exit point.
>
Unless I am missing something, that would set S_IWUSR for all instances
of the driver. Not sure if that is a good idea. I can use a local
variable for mode, though.
Guenter
_______________________________________________
lm-sensors mailing list
lm-sensors@xxxxxxxxxxxxxx
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
