Conversion of w83627ehf to hwmon_device_register_with_info ?

Conversion of w83627ehf to hwmon_device_register_with_info ?

[Peter Hüwe]

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

Re: Conversion of w83627ehf to hwmon_device_register_with_info ?

[Guenter Roeck]

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

Re: Conversion of w83627ehf to hwmon_device_register_with_info ?

[Peter Hüwe]

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

Re: Conversion of w83627ehf to hwmon_device_register_with_info ?

[Guenter Roeck]

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

Re: Conversion of w83627ehf to hwmon_device_register_with_info ?

[Peter Hüwe]

> > 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

Re: Conversion of w83627ehf to hwmon_device_register_with_info ?

[Jean Delvare]

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

Question about hwmon_attr_show_string

[Peter Hüwe]

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

Re: Question about hwmon_attr_show_string

[Guenter Roeck]

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

Re: Question about hwmon_attr_show_string

[Jean Delvare]

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 <jdelvare@suse.de>
---
 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

Re: Question about hwmon_attr_show_string

[Peter Huewe]

Am 7. März 2017 10:08:46 MEZ schrieb Jean Delvare <jdelvare@suse.de>:
>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 <peterhuewe@gmx.de>


>
>Signed-off-by: Jean Delvare <jdelvare@suse.de>
>---
> 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

Re: Question about hwmon_attr_show_string

[Guenter Roeck]

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 <jdelvare@suse.de>

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);
>  };
>
>