* [PATCH] hwmon: (tmp421) Add nfactor support.
@ 2010-05-10 14:43 Jeff Angielski
2010-05-11 19:03 ` Andre Prendel
0 siblings, 1 reply; 5+ messages in thread
From: Jeff Angielski @ 2010-05-10 14:43 UTC (permalink / raw)
To: Linuxppc-dev, Andre Prendel, Jean Delvare
Add support for reading and writing the n-factor correction
registers. This is needed to compensate for the characteristics
of a particular sensor hanging off of the remote channels.
Signed-off-by: Jeff Angielski <jeff@theptrgroup.com>
---
drivers/hwmon/tmp421.c | 42 ++++++++++++++++++++++++++++++++++++++++++
1 files changed, 42 insertions(+), 0 deletions(-)
diff --git a/drivers/hwmon/tmp421.c b/drivers/hwmon/tmp421.c
index 738c472..c9e9855 100644
--- a/drivers/hwmon/tmp421.c
+++ b/drivers/hwmon/tmp421.c
@@ -49,6 +49,7 @@ enum chips { tmp421, tmp422, tmp423 };
static const u8 TMP421_TEMP_MSB[4] = { 0x00, 0x01, 0x02, 0x03 };
static const u8 TMP421_TEMP_LSB[4] = { 0x10, 0x11, 0x12, 0x13 };
+static const u8 TMP421_NFACTOR[3] = { 0x21, 0x22, 0x23 };
/* Flags */
#define TMP421_CONFIG_SHUTDOWN 0x40
@@ -157,6 +158,38 @@ static ssize_t show_fault(struct device *dev,
return sprintf(buf, "0\n");
}
+static ssize_t show_nfactor(struct device *dev,
+ struct device_attribute *devattr, char *buf)
+{
+ struct i2c_client *client = to_i2c_client(dev);
+ struct tmp421_data *data = i2c_get_clientdata(client);
+ int index = to_sensor_dev_attr(devattr)->index;
+ s8 nfactor;
+
+ mutex_lock(&data->update_lock);
+ nfactor = i2c_smbus_read_byte_data(client, TMP421_NFACTOR[index-1]);
+ mutex_unlock(&data->update_lock);
+
+ return sprintf(buf, "%d\n", nfactor);
+}
+
+static ssize_t set_nfactor(struct device *dev,
+ struct device_attribute *devattr,
+ const char *buf, size_t count)
+{
+ struct i2c_client *client = to_i2c_client(dev);
+ struct tmp421_data *data = i2c_get_clientdata(client);
+ int index = to_sensor_dev_attr(devattr)->index;
+ int nfactor = simple_strtol(buf, NULL, 10);
+
+ mutex_lock(&data->update_lock);
+ i2c_smbus_write_byte_data(client, TMP421_NFACTOR[index-1],
+ SENSORS_LIMIT(nfactor, -128, 127));
+ mutex_unlock(&data->update_lock);
+
+ return count;
+}
+
static mode_t tmp421_is_visible(struct kobject *kobj, struct attribute *a,
int n)
{
@@ -177,19 +210,28 @@ static mode_t tmp421_is_visible(struct kobject
*kobj, struct attribute *a,
static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO, show_temp_value, NULL, 0);
static SENSOR_DEVICE_ATTR(temp2_input, S_IRUGO, show_temp_value, NULL, 1);
static SENSOR_DEVICE_ATTR(temp2_fault, S_IRUGO, show_fault, NULL, 1);
+static SENSOR_DEVICE_ATTR(temp2_nfactor, S_IRUGO | S_IWUSR,
+ show_nfactor, set_nfactor, 1);
static SENSOR_DEVICE_ATTR(temp3_input, S_IRUGO, show_temp_value, NULL, 2);
static SENSOR_DEVICE_ATTR(temp3_fault, S_IRUGO, show_fault, NULL, 2);
+static SENSOR_DEVICE_ATTR(temp3_nfactor, S_IRUGO | S_IWUSR,
+ show_nfactor, set_nfactor, 2);
static SENSOR_DEVICE_ATTR(temp4_input, S_IRUGO, show_temp_value, NULL, 3);
static SENSOR_DEVICE_ATTR(temp4_fault, S_IRUGO, show_fault, NULL, 3);
+static SENSOR_DEVICE_ATTR(temp4_nfactor, S_IRUGO | S_IWUSR,
+ show_nfactor, set_nfactor, 3);
static struct attribute *tmp421_attr[] = {
&sensor_dev_attr_temp1_input.dev_attr.attr,
&sensor_dev_attr_temp2_input.dev_attr.attr,
&sensor_dev_attr_temp2_fault.dev_attr.attr,
+ &sensor_dev_attr_temp2_nfactor.dev_attr.attr,
&sensor_dev_attr_temp3_input.dev_attr.attr,
&sensor_dev_attr_temp3_fault.dev_attr.attr,
+ &sensor_dev_attr_temp3_nfactor.dev_attr.attr,
&sensor_dev_attr_temp4_input.dev_attr.attr,
&sensor_dev_attr_temp4_fault.dev_attr.attr,
+ &sensor_dev_attr_temp4_nfactor.dev_attr.attr,
NULL
};
--
1.7.0.4
--
Jeff Angielski
The PTR Group
www.theptrgroup.com
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] hwmon: (tmp421) Add nfactor support.
2010-05-10 14:43 [PATCH] hwmon: (tmp421) Add nfactor support Jeff Angielski
@ 2010-05-11 19:03 ` Andre Prendel
2010-05-11 19:12 ` Jean Delvare
2010-05-11 19:34 ` Jeff Angielski
0 siblings, 2 replies; 5+ messages in thread
From: Andre Prendel @ 2010-05-11 19:03 UTC (permalink / raw)
To: Jeff Angielski; +Cc: Jean Delvare, Linuxppc-dev, lm-sensors
On Mon, May 10, 2010 at 10:43:07AM -0400, Jeff Angielski wrote:
Hi Jeff,
A few comments below.
> Add support for reading and writing the n-factor correction
> registers. This is needed to compensate for the characteristics
> of a particular sensor hanging off of the remote channels.
>
> Signed-off-by: Jeff Angielski <jeff@theptrgroup.com>
> ---
> drivers/hwmon/tmp421.c | 42 ++++++++++++++++++++++++++++++++++++++++++
> 1 files changed, 42 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/hwmon/tmp421.c b/drivers/hwmon/tmp421.c
> index 738c472..c9e9855 100644
> --- a/drivers/hwmon/tmp421.c
> +++ b/drivers/hwmon/tmp421.c
> @@ -49,6 +49,7 @@ enum chips { tmp421, tmp422, tmp423 };
>
> static const u8 TMP421_TEMP_MSB[4] = { 0x00, 0x01, 0x02, 0x03 };
> static const u8 TMP421_TEMP_LSB[4] = { 0x10, 0x11, 0x12, 0x13 };
> +static const u8 TMP421_NFACTOR[3] = { 0x21, 0x22, 0x23 };
>
> /* Flags */
> #define TMP421_CONFIG_SHUTDOWN 0x40
> @@ -157,6 +158,38 @@ static ssize_t show_fault(struct device *dev,
> return sprintf(buf, "0\n");
> }
>
> +static ssize_t show_nfactor(struct device *dev,
> + struct device_attribute *devattr, char *buf)
> +{
> + struct i2c_client *client = to_i2c_client(dev);
> + struct tmp421_data *data = i2c_get_clientdata(client);
> + int index = to_sensor_dev_attr(devattr)->index;
> + s8 nfactor;
> +
> + mutex_lock(&data->update_lock);
> + nfactor = i2c_smbus_read_byte_data(client, TMP421_NFACTOR[index-1]);
There should be spaces within the array index, [index - 1].
> + mutex_unlock(&data->update_lock);
> +
> + return sprintf(buf, "%d\n", nfactor);
> +}
I'de prefer implementing the sysfs access methods in a consistent way (see other functions). That means adding the nfactor register to the tmp421_data structure and using tmp421_update_device() to update the structure.
> +static ssize_t set_nfactor(struct device *dev,
> + struct device_attribute *devattr,
> + const char *buf, size_t count)
> +{
> + struct i2c_client *client = to_i2c_client(dev);
> + struct tmp421_data *data = i2c_get_clientdata(client);
> + int index = to_sensor_dev_attr(devattr)->index;
> + int nfactor = simple_strtol(buf, NULL, 10);
> +
> + mutex_lock(&data->update_lock);
> + i2c_smbus_write_byte_data(client, TMP421_NFACTOR[index-1],
Missing spaces in array index again.
> + SENSORS_LIMIT(nfactor, -128, 127));
> + mutex_unlock(&data->update_lock);
> +
> + return count;
> +}
> +
> static mode_t tmp421_is_visible(struct kobject *kobj, struct attribute *a,
> int n)
> {
> @@ -177,19 +210,28 @@ static mode_t tmp421_is_visible(struct kobject
> *kobj, struct attribute *a,
> static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO, show_temp_value, NULL, 0);
> static SENSOR_DEVICE_ATTR(temp2_input, S_IRUGO, show_temp_value, NULL, 1);
> static SENSOR_DEVICE_ATTR(temp2_fault, S_IRUGO, show_fault, NULL, 1);
> +static SENSOR_DEVICE_ATTR(temp2_nfactor, S_IRUGO | S_IWUSR,
> + show_nfactor, set_nfactor, 1);
> static SENSOR_DEVICE_ATTR(temp3_input, S_IRUGO, show_temp_value, NULL, 2);
> static SENSOR_DEVICE_ATTR(temp3_fault, S_IRUGO, show_fault, NULL, 2);
> +static SENSOR_DEVICE_ATTR(temp3_nfactor, S_IRUGO | S_IWUSR,
> + show_nfactor, set_nfactor, 2);
> static SENSOR_DEVICE_ATTR(temp4_input, S_IRUGO, show_temp_value, NULL, 3);
> static SENSOR_DEVICE_ATTR(temp4_fault, S_IRUGO, show_fault, NULL, 3);
> +static SENSOR_DEVICE_ATTR(temp4_nfactor, S_IRUGO | S_IWUSR,
> + show_nfactor, set_nfactor, 3);
>
> static struct attribute *tmp421_attr[] = {
> &sensor_dev_attr_temp1_input.dev_attr.attr,
> &sensor_dev_attr_temp2_input.dev_attr.attr,
> &sensor_dev_attr_temp2_fault.dev_attr.attr,
> + &sensor_dev_attr_temp2_nfactor.dev_attr.attr,
> &sensor_dev_attr_temp3_input.dev_attr.attr,
> &sensor_dev_attr_temp3_fault.dev_attr.attr,
> + &sensor_dev_attr_temp3_nfactor.dev_attr.attr,
> &sensor_dev_attr_temp4_input.dev_attr.attr,
> &sensor_dev_attr_temp4_fault.dev_attr.attr,
> + &sensor_dev_attr_temp4_nfactor.dev_attr.attr,
> NULL
> };
>
Regards,
Andre
PS: CC'ed lm-sensors list
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] hwmon: (tmp421) Add nfactor support.
2010-05-11 19:03 ` Andre Prendel
@ 2010-05-11 19:12 ` Jean Delvare
2010-05-11 19:34 ` Jeff Angielski
1 sibling, 0 replies; 5+ messages in thread
From: Jean Delvare @ 2010-05-11 19:12 UTC (permalink / raw)
To: Andre Prendel; +Cc: Linuxppc-dev, lm-sensors
On Tue, 11 May 2010 21:03:27 +0200, Andre Prendel wrote:
> On Mon, May 10, 2010 at 10:43:07AM -0400, Jeff Angielski wrote:
>
> Hi Jeff,
>
> A few comments below.
>
> > Add support for reading and writing the n-factor correction
> > registers. This is needed to compensate for the characteristics
> > of a particular sensor hanging off of the remote channels.
> >
> > Signed-off-by: Jeff Angielski <jeff@theptrgroup.com>
> > ---
> > drivers/hwmon/tmp421.c | 42 ++++++++++++++++++++++++++++++++++++++++++
> > 1 files changed, 42 insertions(+), 0 deletions(-)
> >
> > diff --git a/drivers/hwmon/tmp421.c b/drivers/hwmon/tmp421.c
> > index 738c472..c9e9855 100644
> > --- a/drivers/hwmon/tmp421.c
> > +++ b/drivers/hwmon/tmp421.c
> > @@ -49,6 +49,7 @@ enum chips { tmp421, tmp422, tmp423 };
> >
> > static const u8 TMP421_TEMP_MSB[4] = { 0x00, 0x01, 0x02, 0x03 };
> > static const u8 TMP421_TEMP_LSB[4] = { 0x10, 0x11, 0x12, 0x13 };
> > +static const u8 TMP421_NFACTOR[3] = { 0x21, 0x22, 0x23 };
> >
> > /* Flags */
> > #define TMP421_CONFIG_SHUTDOWN 0x40
> > @@ -157,6 +158,38 @@ static ssize_t show_fault(struct device *dev,
> > return sprintf(buf, "0\n");
> > }
> >
> > +static ssize_t show_nfactor(struct device *dev,
> > + struct device_attribute *devattr, char *buf)
> > +{
> > + struct i2c_client *client = to_i2c_client(dev);
> > + struct tmp421_data *data = i2c_get_clientdata(client);
> > + int index = to_sensor_dev_attr(devattr)->index;
> > + s8 nfactor;
> > +
> > + mutex_lock(&data->update_lock);
> > + nfactor = i2c_smbus_read_byte_data(client, TMP421_NFACTOR[index-1]);
>
> There should be spaces within the array index, [index - 1].
>
> > + mutex_unlock(&data->update_lock);
> > +
> > + return sprintf(buf, "%d\n", nfactor);
> > +}
>
> I'de prefer implementing the sysfs access methods in a consistent way (see other functions). That means adding the nfactor register to the tmp421_data structure and using tmp421_update_device() to update the structure.
>
> > +static ssize_t set_nfactor(struct device *dev,
> > + struct device_attribute *devattr,
> > + const char *buf, size_t count)
> > +{
> > + struct i2c_client *client = to_i2c_client(dev);
> > + struct tmp421_data *data = i2c_get_clientdata(client);
> > + int index = to_sensor_dev_attr(devattr)->index;
> > + int nfactor = simple_strtol(buf, NULL, 10);
> > +
> > + mutex_lock(&data->update_lock);
> > + i2c_smbus_write_byte_data(client, TMP421_NFACTOR[index-1],
>
> Missing spaces in array index again.
>
> > + SENSORS_LIMIT(nfactor, -128, 127));
> > + mutex_unlock(&data->update_lock);
> > +
> > + return count;
> > +}
> > +
> > static mode_t tmp421_is_visible(struct kobject *kobj, struct attribute *a,
> > int n)
> > {
> > @@ -177,19 +210,28 @@ static mode_t tmp421_is_visible(struct kobject
> > *kobj, struct attribute *a,
> > static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO, show_temp_value, NULL, 0);
> > static SENSOR_DEVICE_ATTR(temp2_input, S_IRUGO, show_temp_value, NULL, 1);
> > static SENSOR_DEVICE_ATTR(temp2_fault, S_IRUGO, show_fault, NULL, 1);
> > +static SENSOR_DEVICE_ATTR(temp2_nfactor, S_IRUGO | S_IWUSR,
> > + show_nfactor, set_nfactor, 1);
> > static SENSOR_DEVICE_ATTR(temp3_input, S_IRUGO, show_temp_value, NULL, 2);
> > static SENSOR_DEVICE_ATTR(temp3_fault, S_IRUGO, show_fault, NULL, 2);
> > +static SENSOR_DEVICE_ATTR(temp3_nfactor, S_IRUGO | S_IWUSR,
> > + show_nfactor, set_nfactor, 2);
> > static SENSOR_DEVICE_ATTR(temp4_input, S_IRUGO, show_temp_value, NULL, 3);
> > static SENSOR_DEVICE_ATTR(temp4_fault, S_IRUGO, show_fault, NULL, 3);
> > +static SENSOR_DEVICE_ATTR(temp4_nfactor, S_IRUGO | S_IWUSR,
> > + show_nfactor, set_nfactor, 3);
> >
> > static struct attribute *tmp421_attr[] = {
> > &sensor_dev_attr_temp1_input.dev_attr.attr,
> > &sensor_dev_attr_temp2_input.dev_attr.attr,
> > &sensor_dev_attr_temp2_fault.dev_attr.attr,
> > + &sensor_dev_attr_temp2_nfactor.dev_attr.attr,
> > &sensor_dev_attr_temp3_input.dev_attr.attr,
> > &sensor_dev_attr_temp3_fault.dev_attr.attr,
> > + &sensor_dev_attr_temp3_nfactor.dev_attr.attr,
> > &sensor_dev_attr_temp4_input.dev_attr.attr,
> > &sensor_dev_attr_temp4_fault.dev_attr.attr,
> > + &sensor_dev_attr_temp4_nfactor.dev_attr.attr,
> > NULL
> > };
Any hope to standardize on the sysfs attribute files and units? So that
other drivers can implement the same interface.
--
Jean Delvare
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] hwmon: (tmp421) Add nfactor support.
2010-05-11 19:03 ` Andre Prendel
2010-05-11 19:12 ` Jean Delvare
@ 2010-05-11 19:34 ` Jeff Angielski
2010-05-12 7:27 ` Jean Delvare
1 sibling, 1 reply; 5+ messages in thread
From: Jeff Angielski @ 2010-05-11 19:34 UTC (permalink / raw)
To: Andre Prendel; +Cc: Jean Delvare, Linuxppc-dev, lm-sensors
On 05/11/2010 03:03 PM, Andre Prendel wrote:
> On Mon, May 10, 2010 at 10:43:07AM -0400, Jeff Angielski wrote:
>
> Hi Jeff,
>
> A few comments below.
>
>> Add support for reading and writing the n-factor correction
>> registers. This is needed to compensate for the characteristics
>> of a particular sensor hanging off of the remote channels.
>>
>> Signed-off-by: Jeff Angielski<jeff@theptrgroup.com>
>> ---
>> drivers/hwmon/tmp421.c | 42 ++++++++++++++++++++++++++++++++++++++++++
>> 1 files changed, 42 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/hwmon/tmp421.c b/drivers/hwmon/tmp421.c
>> index 738c472..c9e9855 100644
>> --- a/drivers/hwmon/tmp421.c
>> +++ b/drivers/hwmon/tmp421.c
>> @@ -49,6 +49,7 @@ enum chips { tmp421, tmp422, tmp423 };
>>
>> static const u8 TMP421_TEMP_MSB[4] = { 0x00, 0x01, 0x02, 0x03 };
>> static const u8 TMP421_TEMP_LSB[4] = { 0x10, 0x11, 0x12, 0x13 };
>> +static const u8 TMP421_NFACTOR[3] = { 0x21, 0x22, 0x23 };
>>
>> /* Flags */
>> #define TMP421_CONFIG_SHUTDOWN 0x40
>> @@ -157,6 +158,38 @@ static ssize_t show_fault(struct device *dev,
>> return sprintf(buf, "0\n");
>> }
>>
>> +static ssize_t show_nfactor(struct device *dev,
>> + struct device_attribute *devattr, char *buf)
>> +{
>> + struct i2c_client *client = to_i2c_client(dev);
>> + struct tmp421_data *data = i2c_get_clientdata(client);
>> + int index = to_sensor_dev_attr(devattr)->index;
>> + s8 nfactor;
>> +
>> + mutex_lock(&data->update_lock);
>> + nfactor = i2c_smbus_read_byte_data(client, TMP421_NFACTOR[index-1]);
>
> There should be spaces within the array index, [index - 1].
Ok.
>
>> + mutex_unlock(&data->update_lock);
>> +
>> + return sprintf(buf, "%d\n", nfactor);
>> +}
>
> I'de prefer implementing the sysfs access methods in a consistent way (see other functions). That means adding the nfactor register to the tmp421_data structure and using tmp421_update_device() to update the structure.
I did this on purpose since the nfactor typically only changes once at
runtime when you program it for your sensor. It seemed like a waste of
processing power and i2c bandwidth to read a "pseudo static" register
over and over again.
It can easily be changed if that's what will help the community the best.
>> +static ssize_t set_nfactor(struct device *dev,
>> + struct device_attribute *devattr,
>> + const char *buf, size_t count)
>> +{
>> + struct i2c_client *client = to_i2c_client(dev);
>> + struct tmp421_data *data = i2c_get_clientdata(client);
>> + int index = to_sensor_dev_attr(devattr)->index;
>> + int nfactor = simple_strtol(buf, NULL, 10);
>> +
>> + mutex_lock(&data->update_lock);
>> + i2c_smbus_write_byte_data(client, TMP421_NFACTOR[index-1],
>
> Missing spaces in array index again.
Ok.
--
Jeff Angielski
The PTR Group
www.theptrgroup.com
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] hwmon: (tmp421) Add nfactor support.
2010-05-11 19:34 ` Jeff Angielski
@ 2010-05-12 7:27 ` Jean Delvare
0 siblings, 0 replies; 5+ messages in thread
From: Jean Delvare @ 2010-05-12 7:27 UTC (permalink / raw)
To: Jeff Angielski; +Cc: lm-sensors, Linuxppc-dev, Andre Prendel
Hi Jeff,
On Tue, 11 May 2010 15:34:29 -0400, Jeff Angielski wrote:
> On 05/11/2010 03:03 PM, Andre Prendel wrote:
> > I'de prefer implementing the sysfs access methods in a consistent way (see other functions). That means adding the nfactor register to the tmp421_data structure and using tmp421_update_device() to update the structure.
>
> I did this on purpose since the nfactor typically only changes once at
> runtime when you program it for your sensor. It seemed like a waste of
> processing power and i2c bandwidth to read a "pseudo static" register
> over and over again.
>
> It can easily be changed if that's what will help the community the best.
I get your point and it makes sense. However, we also have to ensure
that no regular user can saturate the I2C link by repeatedly polling
for the same register value. So, any sysfs attribute which triggers an
immediate I2C transaction shouldn't be accessible by regular users
(i.e. change the mode from 0644 to 0640).
An alternative is to slit the cache into short-lived (~ 1 or 2 seconds)
and long-lived (1 to 5 minutes.) A number of drivers do this.
--
Jean Delvare
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2010-05-12 7:27 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2010-05-10 14:43 [PATCH] hwmon: (tmp421) Add nfactor support Jeff Angielski
2010-05-11 19:03 ` Andre Prendel
2010-05-11 19:12 ` Jean Delvare
2010-05-11 19:34 ` Jeff Angielski
2010-05-12 7:27 ` Jean Delvare
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).