* [PATCH] hwmon: (max1111) change sysfs interface to in[0-3]_input in millivolts
@ 2011-11-09 11:15 Eric Miao
2011-11-09 12:22 ` [lm-sensors] " Jean Delvare
0 siblings, 1 reply; 6+ messages in thread
From: Eric Miao @ 2011-11-09 11:15 UTC (permalink / raw)
To: linux-kernel, lm-sensors; +Cc: Eric Miao
This patch fixed the inconsistent max1111 sysfs interface as pointed
out by Jean Delvare:
It was pointed to me that the max1111 driver doesn't implement the
standard sysfs interface for hwmon drivers (as described in
Documentation/hwmon/sysfs-interface). It exports files adc[0-3]_in,
which
aren't part of the standard interface. Presumably these should be
renamed to in[0-3]_input. Renaming them is probably not sufficient
though, as I see no scaling done in the driver. As the MAX1111 chip has
a documented full scale of 2.048V, I take it that the LSB of the ADC
has a weight of 8 mV. Exporting raw register values to user-space is
not OK.
Reported-by: Jean Delvare <khali@linux-fr.org>
Signed-off-by: Eric Miao <eric.y.miao@gmail.com>
---
drivers/hwmon/max1111.c | 13 +++++++------
1 files changed, 7 insertions(+), 6 deletions(-)
diff --git a/drivers/hwmon/max1111.c b/drivers/hwmon/max1111.c
index c97b78e..0e9fcfd 100644
--- a/drivers/hwmon/max1111.c
+++ b/drivers/hwmon/max1111.c
@@ -106,11 +106,12 @@ static ssize_t show_adc(struct device *dev,
if (ret < 0)
return ret;
- return sprintf(buf, "%d\n", ret);
+ /* assume the reference voltage to be 2.048V */
+ return sprintf(buf, "%d\n", 2048 * 256 / ret);
}
#define MAX1111_ADC_ATTR(_id) \
- SENSOR_DEVICE_ATTR(adc##_id##_in, S_IRUGO, show_adc, NULL, _id)
+ SENSOR_DEVICE_ATTR(in##_id##_input, S_IRUGO, show_adc, NULL, _id)
static DEVICE_ATTR(name, S_IRUGO, show_name, NULL);
static MAX1111_ADC_ATTR(0);
@@ -120,10 +121,10 @@ static MAX1111_ADC_ATTR(3);
static struct attribute *max1111_attributes[] = {
&dev_attr_name.attr,
- &sensor_dev_attr_adc0_in.dev_attr.attr,
- &sensor_dev_attr_adc1_in.dev_attr.attr,
- &sensor_dev_attr_adc2_in.dev_attr.attr,
- &sensor_dev_attr_adc3_in.dev_attr.attr,
+ &sensor_dev_attr_in0_input.dev_attr.attr,
+ &sensor_dev_attr_in1_input.dev_attr.attr,
+ &sensor_dev_attr_in2_input.dev_attr.attr,
+ &sensor_dev_attr_in3_input.dev_attr.attr,
NULL,
};
--
1.7.5.4
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [lm-sensors] [PATCH] hwmon: (max1111) change sysfs interface to in[0-3]_input in millivolts
2011-11-09 11:15 [PATCH] hwmon: (max1111) change sysfs interface to in[0-3]_input in millivolts Eric Miao
@ 2011-11-09 12:22 ` Jean Delvare
2011-11-09 12:33 ` Eric Miao
0 siblings, 1 reply; 6+ messages in thread
From: Jean Delvare @ 2011-11-09 12:22 UTC (permalink / raw)
To: Eric Miao; +Cc: linux-kernel, lm-sensors
Hi Eric,
On Wed, 9 Nov 2011 19:15:03 +0800, Eric Miao wrote:
> This patch fixed the inconsistent max1111 sysfs interface as pointed
> out by Jean Delvare:
>
> It was pointed to me that the max1111 driver doesn't implement the
> standard sysfs interface for hwmon drivers (as described in
> Documentation/hwmon/sysfs-interface). It exports files adc[0-3]_in,
> which
> aren't part of the standard interface. Presumably these should be
> renamed to in[0-3]_input. Renaming them is probably not sufficient
> though, as I see no scaling done in the driver. As the MAX1111 chip has
> a documented full scale of 2.048V, I take it that the LSB of the ADC
> has a weight of 8 mV. Exporting raw register values to user-space is
> not OK.
>
> Reported-by: Jean Delvare <khali@linux-fr.org>
> Signed-off-by: Eric Miao <eric.y.miao@gmail.com>
> ---
> drivers/hwmon/max1111.c | 13 +++++++------
> 1 files changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/hwmon/max1111.c b/drivers/hwmon/max1111.c
> index c97b78e..0e9fcfd 100644
> --- a/drivers/hwmon/max1111.c
> +++ b/drivers/hwmon/max1111.c
> @@ -106,11 +106,12 @@ static ssize_t show_adc(struct device *dev,
> if (ret < 0)
> return ret;
>
> - return sprintf(buf, "%d\n", ret);
> + /* assume the reference voltage to be 2.048V */
> + return sprintf(buf, "%d\n", 2048 * 256 / ret);
I bet you did not actually test your changes? ret == 0 would obviously
crash the driver. The formula looks wrong anyway, should be 2048 *
ret / 256. Or in short ret * 8, i.e. LSB weight of the ADC is 8 mV.
> }
>
> #define MAX1111_ADC_ATTR(_id) \
> - SENSOR_DEVICE_ATTR(adc##_id##_in, S_IRUGO, show_adc, NULL, _id)
> + SENSOR_DEVICE_ATTR(in##_id##_input, S_IRUGO, show_adc, NULL, _id)
>
> static DEVICE_ATTR(name, S_IRUGO, show_name, NULL);
> static MAX1111_ADC_ATTR(0);
> @@ -120,10 +121,10 @@ static MAX1111_ADC_ATTR(3);
>
> static struct attribute *max1111_attributes[] = {
> &dev_attr_name.attr,
> - &sensor_dev_attr_adc0_in.dev_attr.attr,
> - &sensor_dev_attr_adc1_in.dev_attr.attr,
> - &sensor_dev_attr_adc2_in.dev_attr.attr,
> - &sensor_dev_attr_adc3_in.dev_attr.attr,
> + &sensor_dev_attr_in0_input.dev_attr.attr,
> + &sensor_dev_attr_in1_input.dev_attr.attr,
> + &sensor_dev_attr_in2_input.dev_attr.attr,
> + &sensor_dev_attr_in3_input.dev_attr.attr,
> NULL,
> };
>
The rest of the changes look good.
--
Jean Delvare
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [lm-sensors] [PATCH] hwmon: (max1111) change sysfs interface to in[0-3]_input in millivolts
2011-11-09 12:22 ` [lm-sensors] " Jean Delvare
@ 2011-11-09 12:33 ` Eric Miao
2011-11-09 12:51 ` Jean Delvare
0 siblings, 1 reply; 6+ messages in thread
From: Eric Miao @ 2011-11-09 12:33 UTC (permalink / raw)
To: Jean Delvare; +Cc: linux-kernel, lm-sensors
On Wed, Nov 9, 2011 at 8:22 PM, Jean Delvare <khali@linux-fr.org> wrote:
> Hi Eric,
>
> On Wed, 9 Nov 2011 19:15:03 +0800, Eric Miao wrote:
>> This patch fixed the inconsistent max1111 sysfs interface as pointed
>> out by Jean Delvare:
>>
>> It was pointed to me that the max1111 driver doesn't implement the
>> standard sysfs interface for hwmon drivers (as described in
>> Documentation/hwmon/sysfs-interface). It exports files adc[0-3]_in,
>> which
>> aren't part of the standard interface. Presumably these should be
>> renamed to in[0-3]_input. Renaming them is probably not sufficient
>> though, as I see no scaling done in the driver. As the MAX1111 chip has
>> a documented full scale of 2.048V, I take it that the LSB of the ADC
>> has a weight of 8 mV. Exporting raw register values to user-space is
>> not OK.
>>
>> Reported-by: Jean Delvare <khali@linux-fr.org>
>> Signed-off-by: Eric Miao <eric.y.miao@gmail.com>
>> ---
>> drivers/hwmon/max1111.c | 13 +++++++------
>> 1 files changed, 7 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/hwmon/max1111.c b/drivers/hwmon/max1111.c
>> index c97b78e..0e9fcfd 100644
>> --- a/drivers/hwmon/max1111.c
>> +++ b/drivers/hwmon/max1111.c
>> @@ -106,11 +106,12 @@ static ssize_t show_adc(struct device *dev,
>> if (ret < 0)
>> return ret;
>>
>> - return sprintf(buf, "%d\n", ret);
>> + /* assume the reference voltage to be 2.048V */
>> + return sprintf(buf, "%d\n", 2048 * 256 / ret);
>
> I bet you did not actually test your changes? ret == 0 would obviously
> crash the driver. The formula looks wrong anyway, should be 2048 *
> ret / 256. Or in short ret * 8, i.e. LSB weight of the ADC is 8 mV.
>
Oops, shame to death.
How about simply '2048 * ret / 256' to make it clear and leave it to
the compiler to optimize?
>> }
>>
>> #define MAX1111_ADC_ATTR(_id) \
>> - SENSOR_DEVICE_ATTR(adc##_id##_in, S_IRUGO, show_adc, NULL, _id)
>> + SENSOR_DEVICE_ATTR(in##_id##_input, S_IRUGO, show_adc, NULL, _id)
>>
>> static DEVICE_ATTR(name, S_IRUGO, show_name, NULL);
>> static MAX1111_ADC_ATTR(0);
>> @@ -120,10 +121,10 @@ static MAX1111_ADC_ATTR(3);
>>
>> static struct attribute *max1111_attributes[] = {
>> &dev_attr_name.attr,
>> - &sensor_dev_attr_adc0_in.dev_attr.attr,
>> - &sensor_dev_attr_adc1_in.dev_attr.attr,
>> - &sensor_dev_attr_adc2_in.dev_attr.attr,
>> - &sensor_dev_attr_adc3_in.dev_attr.attr,
>> + &sensor_dev_attr_in0_input.dev_attr.attr,
>> + &sensor_dev_attr_in1_input.dev_attr.attr,
>> + &sensor_dev_attr_in2_input.dev_attr.attr,
>> + &sensor_dev_attr_in3_input.dev_attr.attr,
>> NULL,
>> };
>>
>
> The rest of the changes look good.
>
> --
> Jean Delvare
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [lm-sensors] [PATCH] hwmon: (max1111) change sysfs interface to in[0-3]_input in millivolts
2011-11-09 12:33 ` Eric Miao
@ 2011-11-09 12:51 ` Jean Delvare
2011-11-09 13:02 ` Eric Miao
0 siblings, 1 reply; 6+ messages in thread
From: Jean Delvare @ 2011-11-09 12:51 UTC (permalink / raw)
To: Eric Miao; +Cc: linux-kernel, lm-sensors
On Wed, 9 Nov 2011 20:33:27 +0800, Eric Miao wrote:
> On Wed, Nov 9, 2011 at 8:22 PM, Jean Delvare <khali@linux-fr.org> wrote:
> > Hi Eric,
> >
> > On Wed, 9 Nov 2011 19:15:03 +0800, Eric Miao wrote:
> >> This patch fixed the inconsistent max1111 sysfs interface as pointed
> >> out by Jean Delvare:
> >>
> >> It was pointed to me that the max1111 driver doesn't implement the
> >> standard sysfs interface for hwmon drivers (as described in
> >> Documentation/hwmon/sysfs-interface). It exports files adc[0-3]_in,
> >> which
> >> aren't part of the standard interface. Presumably these should be
> >> renamed to in[0-3]_input. Renaming them is probably not sufficient
> >> though, as I see no scaling done in the driver. As the MAX1111 chip has
> >> a documented full scale of 2.048V, I take it that the LSB of the ADC
> >> has a weight of 8 mV. Exporting raw register values to user-space is
> >> not OK.
> >>
> >> Reported-by: Jean Delvare <khali@linux-fr.org>
> >> Signed-off-by: Eric Miao <eric.y.miao@gmail.com>
> >> ---
> >> drivers/hwmon/max1111.c | 13 +++++++------
> >> 1 files changed, 7 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/drivers/hwmon/max1111.c b/drivers/hwmon/max1111.c
> >> index c97b78e..0e9fcfd 100644
> >> --- a/drivers/hwmon/max1111.c
> >> +++ b/drivers/hwmon/max1111.c
> >> @@ -106,11 +106,12 @@ static ssize_t show_adc(struct device *dev,
> >> if (ret < 0)
> >> return ret;
> >>
> >> - return sprintf(buf, "%d\n", ret);
> >> + /* assume the reference voltage to be 2.048V */
> >> + return sprintf(buf, "%d\n", 2048 * 256 / ret);
> >
> > I bet you did not actually test your changes? ret == 0 would obviously
> > crash the driver. The formula looks wrong anyway, should be 2048 *
> > ret / 256. Or in short ret * 8, i.e. LSB weight of the ADC is 8 mV.
> >
>
> Oops, shame to death.
>
> How about simply '2048 * ret / 256' to make it clear and leave it to
> the compiler to optimize?
I find the concept of LSB weight clear enough, but ultimately that's up
to you, as long as the compiler actually optimizes it.
--
Jean Delvare
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [lm-sensors] [PATCH] hwmon: (max1111) change sysfs interface to in[0-3]_input in millivolts
2011-11-09 12:51 ` Jean Delvare
@ 2011-11-09 13:02 ` Eric Miao
0 siblings, 0 replies; 6+ messages in thread
From: Eric Miao @ 2011-11-09 13:02 UTC (permalink / raw)
To: Jean Delvare; +Cc: linux-kernel, lm-sensors
On Wed, Nov 9, 2011 at 8:51 PM, Jean Delvare <khali@linux-fr.org> wrote:
> On Wed, 9 Nov 2011 20:33:27 +0800, Eric Miao wrote:
>> On Wed, Nov 9, 2011 at 8:22 PM, Jean Delvare <khali@linux-fr.org> wrote:
>> > Hi Eric,
>> >
>> > On Wed, 9 Nov 2011 19:15:03 +0800, Eric Miao wrote:
>> >> This patch fixed the inconsistent max1111 sysfs interface as pointed
>> >> out by Jean Delvare:
>> >>
>> >> It was pointed to me that the max1111 driver doesn't implement the
>> >> standard sysfs interface for hwmon drivers (as described in
>> >> Documentation/hwmon/sysfs-interface). It exports files adc[0-3]_in,
>> >> which
>> >> aren't part of the standard interface. Presumably these should be
>> >> renamed to in[0-3]_input. Renaming them is probably not sufficient
>> >> though, as I see no scaling done in the driver. As the MAX1111 chip has
>> >> a documented full scale of 2.048V, I take it that the LSB of the ADC
>> >> has a weight of 8 mV. Exporting raw register values to user-space is
>> >> not OK.
>> >>
>> >> Reported-by: Jean Delvare <khali@linux-fr.org>
>> >> Signed-off-by: Eric Miao <eric.y.miao@gmail.com>
>> >> ---
>> >> drivers/hwmon/max1111.c | 13 +++++++------
>> >> 1 files changed, 7 insertions(+), 6 deletions(-)
>> >>
>> >> diff --git a/drivers/hwmon/max1111.c b/drivers/hwmon/max1111.c
>> >> index c97b78e..0e9fcfd 100644
>> >> --- a/drivers/hwmon/max1111.c
>> >> +++ b/drivers/hwmon/max1111.c
>> >> @@ -106,11 +106,12 @@ static ssize_t show_adc(struct device *dev,
>> >> if (ret < 0)
>> >> return ret;
>> >>
>> >> - return sprintf(buf, "%d\n", ret);
>> >> + /* assume the reference voltage to be 2.048V */
>> >> + return sprintf(buf, "%d\n", 2048 * 256 / ret);
>> >
>> > I bet you did not actually test your changes? ret == 0 would obviously
>> > crash the driver. The formula looks wrong anyway, should be 2048 *
>> > ret / 256. Or in short ret * 8, i.e. LSB weight of the ADC is 8 mV.
>> >
>>
>> Oops, shame to death.
>>
>> How about simply '2048 * ret / 256' to make it clear and leave it to
>> the compiler to optimize?
>
> I find the concept of LSB weight clear enough, but ultimately that's up
> to you, as long as the compiler actually optimizes it.
Sounds good, updated and resubmitted.
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] hwmon: (max1111) change sysfs interface to in[0-3]_input in millivolts
@ 2011-11-09 13:01 Eric Miao
0 siblings, 0 replies; 6+ messages in thread
From: Eric Miao @ 2011-11-09 13:01 UTC (permalink / raw)
To: linux-kernel, lm-sensors; +Cc: Eric Miao
This patch fixed the inconsistent max1111 sysfs interface as pointed
out by Jean Delvare:
It was pointed to me that the max1111 driver doesn't implement the
standard sysfs interface for hwmon drivers (as described in
Documentation/hwmon/sysfs-interface). It exports files adc[0-3]_in,
which
aren't part of the standard interface. Presumably these should be
renamed to in[0-3]_input. Renaming them is probably not sufficient
though, as I see no scaling done in the driver. As the MAX1111 chip has
a documented full scale of 2.048V, I take it that the LSB of the ADC
has a weight of 8 mV. Exporting raw register values to user-space is
not OK.
Reported-by: Jean Delvare <khali@linux-fr.org>
Signed-off-by: Eric Miao <eric.y.miao@gmail.com>
---
drivers/hwmon/max1111.c | 15 +++++++++------
1 files changed, 9 insertions(+), 6 deletions(-)
diff --git a/drivers/hwmon/max1111.c b/drivers/hwmon/max1111.c
index c97b78e..5586cca 100644
--- a/drivers/hwmon/max1111.c
+++ b/drivers/hwmon/max1111.c
@@ -106,11 +106,14 @@ static ssize_t show_adc(struct device *dev,
if (ret < 0)
return ret;
- return sprintf(buf, "%d\n", ret);
+ /* assume the reference voltage to be 2.048V, with an 8-bit sample,
+ * the LSB weight is 8mV
+ */
+ return sprintf(buf, "%d\n", ret * 8);
}
#define MAX1111_ADC_ATTR(_id) \
- SENSOR_DEVICE_ATTR(adc##_id##_in, S_IRUGO, show_adc, NULL, _id)
+ SENSOR_DEVICE_ATTR(in##_id##_input, S_IRUGO, show_adc, NULL, _id)
static DEVICE_ATTR(name, S_IRUGO, show_name, NULL);
static MAX1111_ADC_ATTR(0);
@@ -120,10 +123,10 @@ static MAX1111_ADC_ATTR(3);
static struct attribute *max1111_attributes[] = {
&dev_attr_name.attr,
- &sensor_dev_attr_adc0_in.dev_attr.attr,
- &sensor_dev_attr_adc1_in.dev_attr.attr,
- &sensor_dev_attr_adc2_in.dev_attr.attr,
- &sensor_dev_attr_adc3_in.dev_attr.attr,
+ &sensor_dev_attr_in0_input.dev_attr.attr,
+ &sensor_dev_attr_in1_input.dev_attr.attr,
+ &sensor_dev_attr_in2_input.dev_attr.attr,
+ &sensor_dev_attr_in3_input.dev_attr.attr,
NULL,
};
--
1.7.5.4
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2011-11-09 13:02 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-11-09 11:15 [PATCH] hwmon: (max1111) change sysfs interface to in[0-3]_input in millivolts Eric Miao
2011-11-09 12:22 ` [lm-sensors] " Jean Delvare
2011-11-09 12:33 ` Eric Miao
2011-11-09 12:51 ` Jean Delvare
2011-11-09 13:02 ` Eric Miao
2011-11-09 13:01 Eric Miao
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®