Conversion of w83627ehf to hwmon_device_register_with_info ?

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.

Hi Peter,

Quoted 4 lines · Peter Hüwe

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.

Quoted 3 lines

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.

Hi

Quoted 8 lines · On Friday 03 March 2017 03:56:01 Guenter Roeck wrote:

Hi Peter,

Quoted 4 lines · Peter Hüwe

Hi,

is anybody else working on the conversion of the w83627ehf to the new hwmon_device_register_with_info interface?

Quoted 7 lines

I don't think so.

Quoted 3 lines

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)

Quoted 2 lines

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.

Quoted 8 lines

It saves about 20k in compiled size, so the savings from reduced boilerplate are huge. (and I think it's more readable)

Quoted 2 lines

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 ?

is anybody else working on the conversion of the w83627ehf to the new hwmon_device_register_with_info interface?

read_string function rather than the buffer itself?

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-

Quoted 1 line

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.

Quoted 11 lines · 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-

Quoted 1 line

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?

Quoted 2 lines

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.

Hi Guenter,

Quoted 5 lines · Guenter Roeck

Quoted 4 lines · 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-

Quoted 3 lines

Quoted 3 lines

Quoted 1 line

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

Quoted 1 line

Quoted 1 line

the value on the heap (and taking care to free it somewhere, since

returning

Quoted 1 line

Quoted 1 line

an address on the stack is bad idea), instead of calling

sprintf(buf, "%s\n",

Quoted 3 lines

Quoted 3 lines

s) directly?

With the current implementation I have to sprintf it into my local

buffer and

Quoted 3 lines

Quoted 1 line

you sprintf it again into the final buffer.

The idea was that the called code would return a pointer to a

constant string,

Quoted 1 line

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

drivers/hwmon/hwmon.c +5 -3

@@ -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 Guenter,

Quoted 12 lines · Guenter Roeck

Quoted 11 lines · 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-

Quoted 1 line

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?

Quoted 5 lines

Quoted 2 lines

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

drivers/hwmon/hwmon.c +1 -1

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

include/linux/hwmon.h +1 -1

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