Hi,
is anybody else working on the conversion of the w83627ehf to the new
hwmon_device_register_with_info interface?
Otherwise I will probably update the driver to this interface within the next
days - but since it's a lot of work I wanted to check for duplication first.
Do you think it makes sense to introduce a hwmon_sensor_types for "intrusion"
as well? - there are currently 8 drivers who offer that interface.
Thanks,
Peter
Hi Peter,
On 03/02/2017 04:33 PM, Peter H?we wrote:
> Hi,
>
> is anybody else working on the conversion of the w83627ehf to the new
> hwmon_device_register_with_info interface?
>
I don't think so.
> Otherwise I will probably update the driver to this interface within the next
> days - but since it's a lot of work I wanted to check for duplication first.
>
Go ahead. I would suggest to drop nct6775/nct6776 support to simplify the code
when you do that. Maybe as separate commit, though.
> Do you think it makes sense to introduce a hwmon_sensor_types for "intrusion"
> as well? - there are currently 8 drivers who offer that interface.
>
I don't really like the idea of introducing another type for just one attribute,
but it might be the easiest and most consistent approach. Feel free to submit
a patch to add it.
Guenter
Hi Peter,
On Fri, 3 Mar 2017 01:33:01 +0100, Peter Hüwe wrote:
> is anybody else working on the conversion of the w83627ehf to the new
> hwmon_device_register_with_info interface?
Not me.
--
Jean Delvare
SUSE L3 Support
Hi Guenter,
I was wondering whether there was a particular reason why
hwmon_attr_show_string passes only an "empty" pointer(pointer) to the ops-
>read_string function rather than the buffer itself?
Wouldn't this mean that in ops->read_string I'd have to reserve some space for
the value on the heap (and taking care to free it somewhere, since returning
an address on the stack is bad idea), instead of calling sprintf(buf, "%s\n",
s) directly?
With the current implementation I have to sprintf it into my local buffer and
you sprintf it again into the final buffer.
Unfortunately there are no other callers, where you show the intended usage.
Thanks,
Peter
On Mon, Mar 06, 2017 at 09:48:35PM +0100, Peter H?we wrote:
> Hi Guenter,
>
> I was wondering whether there was a particular reason why
> hwmon_attr_show_string passes only an "empty" pointer(pointer) to the ops-
> >read_string function rather than the buffer itself?
>
> Wouldn't this mean that in ops->read_string I'd have to reserve some space for
> the value on the heap (and taking care to free it somewhere, since returning
> an address on the stack is bad idea), instead of calling sprintf(buf, "%s\n",
> s) directly?
>
> With the current implementation I have to sprintf it into my local buffer and
> you sprintf it again into the final buffer.
>
The idea was that the called code would return a pointer to a constant string,
ie one that isn't changing from call to call.
What attribute do you see that would require a dynamic (changing) string ?
Thanks,
Guenter
Hi Guenter,
On Mon, 6 Mar 2017 15:47:55 -0800, Guenter Roeck wrote:
> On Mon, Mar 06, 2017 at 09:48:35PM +0100, Peter Hüwe wrote:
> > Hi Guenter,
> >
> > I was wondering whether there was a particular reason why
> > hwmon_attr_show_string passes only an "empty" pointer(pointer) to the ops-
> > >read_string function rather than the buffer itself?
> >
> > Wouldn't this mean that in ops->read_string I'd have to reserve some space for
> > the value on the heap (and taking care to free it somewhere, since returning
> > an address on the stack is bad idea), instead of calling sprintf(buf, "%s\n",
> > s) directly?
> >
> > With the current implementation I have to sprintf it into my local buffer and
> > you sprintf it again into the final buffer.
>
> The idea was that the called code would return a pointer to a constant string,
> ie one that isn't changing from call to call.
In that case, what about the following change?
Subject: hwmon: Constify str parameter of hwmon_ops->read_string
The read_string callback is supposed to retrieve a pointer to a
constant string.
Signed-off-by: Jean Delvare <[email protected]>
---
drivers/hwmon/hwmon.c | 2 +-
include/linux/hwmon.h | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
--- linux-4.10.orig/drivers/hwmon/hwmon.c 2017-02-19 23:34:00.000000000 +0100
+++ linux-4.10/drivers/hwmon/hwmon.c 2017-03-07 08:22:27.784527968 +0100
@@ -186,7 +186,7 @@ static ssize_t hwmon_attr_show_string(st
char *buf)
{
struct hwmon_device_attribute *hattr = to_hwmon_attr(devattr);
- char *s;
+ const char *s;
int ret;
ret = hattr->ops->read_string(dev, hattr->type, hattr->attr,
--- linux-4.10.orig/include/linux/hwmon.h 2017-02-19 23:34:00.000000000 +0100
+++ linux-4.10/include/linux/hwmon.h 2017-03-07 08:21:28.247998585 +0100
@@ -336,7 +336,7 @@ struct hwmon_ops {
int (*read)(struct device *dev, enum hwmon_sensor_types type,
u32 attr, int channel, long *val);
int (*read_string)(struct device *dev, enum hwmon_sensor_types type,
- u32 attr, int channel, char **str);
+ u32 attr, int channel, const char **str);
int (*write)(struct device *dev, enum hwmon_sensor_types type,
u32 attr, int channel, long val);
};
--
Jean Delvare
SUSE L3 Support
Am 7. März 2017 10:08:46 MEZ schrieb Jean Delvare <[email protected]>:
>Hi Guenter,
>
>On Mon, 6 Mar 2017 15:47:55 -0800, Guenter Roeck wrote:
>> On Mon, Mar 06, 2017 at 09:48:35PM +0100, Peter Hüwe wrote:
>> > Hi Guenter,
>> >
>> > I was wondering whether there was a particular reason why
>> > hwmon_attr_show_string passes only an "empty" pointer(pointer) to
>the ops-
>> > >read_string function rather than the buffer itself?
>> >
>> > Wouldn't this mean that in ops->read_string I'd have to reserve
>some space for
>> > the value on the heap (and taking care to free it somewhere, since
>returning
>> > an address on the stack is bad idea), instead of calling
>sprintf(buf, "%s\n",
>> > s) directly?
>> >
>> > With the current implementation I have to sprintf it into my local
>buffer and
>> > you sprintf it again into the final buffer.
>>
>> The idea was that the called code would return a pointer to a
>constant string,
>> ie one that isn't changing from call to call.
>
>In that case, what about the following change?
>
>Subject: hwmon: Constify str parameter of hwmon_ops->read_string
>
>The read_string callback is supposed to retrieve a pointer to a
>constant string.
I would think that clarifies the situation and also gets rid warnings about dropping const qualifier at the point of assignment.
Reviewed-by: Peter Huewe <[email protected]>
>
>Signed-off-by: Jean Delvare <[email protected]>
>---
> drivers/hwmon/hwmon.c | 2 +-
> include/linux/hwmon.h | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
>
>--- linux-4.10.orig/drivers/hwmon/hwmon.c 2017-02-19 23:34:00.000000000
>+0100
>+++ linux-4.10/drivers/hwmon/hwmon.c 2017-03-07 08:22:27.784527968
>+0100
>@@ -186,7 +186,7 @@ static ssize_t hwmon_attr_show_string(st
> char *buf)
> {
> struct hwmon_device_attribute *hattr = to_hwmon_attr(devattr);
>- char *s;
>+ const char *s;
> int ret;
>
> ret = hattr->ops->read_string(dev, hattr->type, hattr->attr,
>--- linux-4.10.orig/include/linux/hwmon.h 2017-02-19 23:34:00.000000000
>+0100
>+++ linux-4.10/include/linux/hwmon.h 2017-03-07 08:21:28.247998585
>+0100
>@@ -336,7 +336,7 @@ struct hwmon_ops {
> int (*read)(struct device *dev, enum hwmon_sensor_types type,
> u32 attr, int channel, long *val);
> int (*read_string)(struct device *dev, enum hwmon_sensor_types type,
>- u32 attr, int channel, char **str);
>+ u32 attr, int channel, const char **str);
> int (*write)(struct device *dev, enum hwmon_sensor_types type,
> u32 attr, int channel, long val);
> };
--
Sent from my mobile
On 03/07/2017 01:08 AM, Jean Delvare wrote:
> Hi Guenter,
>
> On Mon, 6 Mar 2017 15:47:55 -0800, Guenter Roeck wrote:
>> On Mon, Mar 06, 2017 at 09:48:35PM +0100, Peter Hüwe wrote:
>>> Hi Guenter,
>>>
>>> I was wondering whether there was a particular reason why
>>> hwmon_attr_show_string passes only an "empty" pointer(pointer) to the ops-
>>>> read_string function rather than the buffer itself?
>>>
>>> Wouldn't this mean that in ops->read_string I'd have to reserve some space for
>>> the value on the heap (and taking care to free it somewhere, since returning
>>> an address on the stack is bad idea), instead of calling sprintf(buf, "%s\n",
>>> s) directly?
>>>
>>> With the current implementation I have to sprintf it into my local buffer and
>>> you sprintf it again into the final buffer.
>>
>> The idea was that the called code would return a pointer to a constant string,
>> ie one that isn't changing from call to call.
>
> In that case, what about the following change?
>
> Subject: hwmon: Constify str parameter of hwmon_ops->read_string
>
> The read_string callback is supposed to retrieve a pointer to a
> constant string.
>
> Signed-off-by: Jean Delvare <[email protected]>
Makes sense. I'll add it to -next.
Thanks,
Guenter
> ---
> drivers/hwmon/hwmon.c | 2 +-
> include/linux/hwmon.h | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
>
> --- linux-4.10.orig/drivers/hwmon/hwmon.c 2017-02-19 23:34:00.000000000 +0100
> +++ linux-4.10/drivers/hwmon/hwmon.c 2017-03-07 08:22:27.784527968 +0100
> @@ -186,7 +186,7 @@ static ssize_t hwmon_attr_show_string(st
> char *buf)
> {
> struct hwmon_device_attribute *hattr = to_hwmon_attr(devattr);
> - char *s;
> + const char *s;
> int ret;
>
> ret = hattr->ops->read_string(dev, hattr->type, hattr->attr,
> --- linux-4.10.orig/include/linux/hwmon.h 2017-02-19 23:34:00.000000000 +0100
> +++ linux-4.10/include/linux/hwmon.h 2017-03-07 08:21:28.247998585 +0100
> @@ -336,7 +336,7 @@ struct hwmon_ops {
> int (*read)(struct device *dev, enum hwmon_sensor_types type,
> u32 attr, int channel, long *val);
> int (*read_string)(struct device *dev, enum hwmon_sensor_types type,
> - u32 attr, int channel, char **str);
> + u32 attr, int channel, const char **str);
> int (*write)(struct device *dev, enum hwmon_sensor_types type,
> u32 attr, int channel, long val);
> };
>
>
Hi
On Friday 03 March 2017 03:56:01 Guenter Roeck wrote:
> Hi Peter,
>
> On 03/02/2017 04:33 PM, Peter H?we wrote:
> > Hi,
> >
> > is anybody else working on the conversion of the w83627ehf to the new
> > hwmon_device_register_with_info interface?
>
> I don't think so.
>
> > Otherwise I will probably update the driver to this interface within the
> > next days - but since it's a lot of work I wanted to check for
> > duplication first.
>
> Go ahead.
So I'm close to have to conversion done,
the current diff stat is about
647 insertions(+), 787 deletions(-)
for the whole conversion.
Should I try to break it down into smaller chunks for easier review?
Although this would mean to convert stuff from A to B and then from B to C -
otherwise the intermediate steps would be not fully functional.
(since the sysfs nodes partially would exist under hwmon1/ and partially under
hwmon1/device/ (as they currently are)).
or just submit it?
It saves about 20k in compiled size, so the savings from reduced boilerplate
are huge. (and I think it's more readable)
> I would suggest to drop nct6775/nct6776 support to simplify the
> code when you do that. Maybe as separate commit, though.
Hehe - I'm testing on a nct6776 :)
I'll look into it once the first conversion has been accepted.
Thanks,
Peter
On 03/21/2017 03:46 AM, Peter H?we wrote:
> Hi
>
> On Friday 03 March 2017 03:56:01 Guenter Roeck wrote:
>> Hi Peter,
>>
>> On 03/02/2017 04:33 PM, Peter H?we wrote:
>>> Hi,
>>>
>>> is anybody else working on the conversion of the w83627ehf to the new
>>> hwmon_device_register_with_info interface?
>>
>> I don't think so.
>>
>>> Otherwise I will probably update the driver to this interface within the
>>> next days - but since it's a lot of work I wanted to check for
>>> duplication first.
>>
>> Go ahead.
>
> So I'm close to have to conversion done,
> the current diff stat is about
> 647 insertions(+), 787 deletions(-)
> for the whole conversion.
>
> Should I try to break it down into smaller chunks for easier review?
>
> Although this would mean to convert stuff from A to B and then from B to C -
> otherwise the intermediate steps would be not fully functional.
> (since the sysfs nodes partially would exist under hwmon1/ and partially under
> hwmon1/device/ (as they currently are)).
>
> or just submit it?
>
Just submit it. I don't really think a conversion like this can be split up
cleanly.
> It saves about 20k in compiled size, so the savings from reduced boilerplate
> are huge. (and I think it's more readable)
>
>> I would suggest to drop nct6775/nct6776 support to simplify the
>> code when you do that. Maybe as separate commit, though.
> Hehe - I'm testing on a nct6776 :)
> I'll look into it once the first conversion has been accepted.
>
Wondering - why don't you use the nct6775 driver ?
Thanks,
Guenter
> > It saves about 20k in compiled size, so the savings from reduced
> > boilerplate are huge. (and I think it's more readable)
> >
> >> I would suggest to drop nct6775/nct6776 support to simplify the
> >> code when you do that. Maybe as separate commit, though.
> >
> > Hehe - I'm testing on a nct6776 :)
> > I'll look into it once the first conversion has been accepted.
>
> Wondering - why don't you use the nct6775 driver ?
Probably because it got matched by sensors-detect before the nct6775 driver
(?) or because I hadn't had it turned on in my kernel config.
Both drivers work, but interestingly, they show different values for
intrusion1 (and fan alarm).
nct6776.c:
nct6776-isa-0290
Adapter: ISA adapter
Vcore: +1.16 V (min = +0.00 V, max = +1.74 V)
in1: +1.85 V (min = +0.00 V, max = +0.00 V) ALARM
AVCC: +3.36 V (min = +2.98 V, max = +3.63 V)
+3.3V: +3.36 V (min = +2.98 V, max = +3.63 V)
in4: +0.26 V (min = +0.00 V, max = +0.00 V) ALARM
in5: +1.67 V (min = +0.00 V, max = +0.00 V) ALARM
in6: +0.79 V (min = +0.00 V, max = +0.00 V) ALARM
3VSB: +3.44 V (min = +2.98 V, max = +3.63 V)
Vbat: +3.26 V (min = +2.70 V, max = +3.63 V)
fan1: 0 RPM (min = 0 RPM)
fan2: 2106 RPM (min = 0 RPM)
fan3: 0 RPM (min = 0 RPM)
fan4: 0 RPM (min = 0 RPM)
fan5: 0 RPM (min = 0 RPM)
SYSTIN: +35.0?C (high = +0.0?C, hyst = +0.0?C) ALARM sensor =
thermistor
CPUTIN: +44.0?C (high = +80.0?C, hyst = +75.0?C) sensor = thermistor
AUXTIN: +43.5?C (high = +80.0?C, hyst = +75.0?C) sensor = thermistor
PECI Agent 0: +59.0?C (high = +80.0?C, hyst = +75.0?C)
(crit = +105.0?C)
PCH_CHIP_TEMP: +0.0?C
PCH_CPU_TEMP: +0.0?C
PCH_MCH_TEMP: +0.0?C
intrusion0: ALARM
intrusion1: OK
beep_enable: disabled
w83627ehf.c:
nct6776-isa-0290
Adapter: ISA adapter
Vcore: +1.14 V (min = +0.00 V, max = +1.74 V)
in1: +1.85 V (min = +0.00 V, max = +0.00 V) ALARM
AVCC: +3.36 V (min = +2.98 V, max = +3.63 V)
+3.3V: +3.36 V (min = +2.98 V, max = +3.63 V)
in4: +0.24 V (min = +0.00 V, max = +0.00 V) ALARM
in5: +1.68 V (min = +0.00 V, max = +0.00 V) ALARM
3VSB: +3.44 V (min = +2.98 V, max = +3.63 V)
Vbat: +3.26 V (min = +2.70 V, max = +3.63 V)
fan1: 0 RPM (min = 0 RPM) ALARM
fan2: 2142 RPM (min = 0 RPM) ALARM
fan3: 0 RPM (min = 0 RPM) ALARM
fan4: 0 RPM (min = 0 RPM) ALARM
fan5: 0 RPM (min = 0 RPM) ALARM
SYSTIN: +35.0?C (high = +0.0?C, hyst = +0.0?C) ALARM sensor =
thermistor
CPUTIN: +46.0?C (high = +80.0?C, hyst = +75.0?C) sensor = thermistor
AUXTIN: +44.0?C (high = +80.0?C, hyst = +75.0?C) sensor = thermistor
PECI Agent 0: +64.0?C
cpu0_vid: +0.000 V
intrusion0: ALARM
intrusion1: ALARM
Once the other patches have been accepted I'll work on that conversion/removal
of the nct6775 code from the w83627ehf.
Peter