[lm-sensors] [PATCH 4/5] hwmon: (lm63) Add support for writing the external critical temperature

Guenter Roeck guenter.roeck at ericsson.com
Tue Jan 10 19:21:53 CET 2012


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 at ericsson.com>
> > 
> > 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 at ericsson.com>
> > Signed-off-by: Guenter Roeck <linux at roeck-us.net>
> > ---
> >  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






More information about the lm-sensors mailing list