Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1591575 > unrolled thread

Conversion of w83627ehf to hwmon_device_register_with_info ?

Back to article view | Back to linux.kernel

Contents

  Conversion of w83627ehf to hwmon_device_register_with_info ? Peter Hüwe <PeterHuewe@gmx.de> - 2017-03-03 01:40 +0100
    Re: Conversion of w83627ehf to hwmon_device_register_with_info ? Guenter Roeck <linux@roeck-us.net> - 2017-03-03 04:00 +0100
    Re: Conversion of w83627ehf to hwmon_device_register_with_info ? Jean Delvare <jdelvare@suse.de> - 2017-03-03 16:10 +0100
    Question about hwmon_attr_show_string Peter Hüwe <PeterHuewe@gmx.de> - 2017-03-06 21:50 +0100
      Re: Question about hwmon_attr_show_string Guenter Roeck <linux@roeck-us.net> - 2017-03-07 01:00 +0100
        Re: Question about hwmon_attr_show_string Jean Delvare <jdelvare@suse.de> - 2017-03-07 10:20 +0100
          Re: Question about hwmon_attr_show_string Peter Huewe <peterhuewe@gmx.de> - 2017-03-07 10:30 +0100
          Re: Question about hwmon_attr_show_string Guenter Roeck <linux@roeck-us.net> - 2017-03-07 16:10 +0100

#1591575 — 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.

Thanks,
Peter

[toc] | [next] | [standalone]

#1591642

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

[toc] | [prev] | [next] | [standalone]

#1592035

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

[toc] | [prev] | [next] | [standalone]

#1593713 — Question about hwmon_attr_show_string

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

[toc] | [prev] | [next] | [standalone]

#1593805 — Re: Question about hwmon_attr_show_string

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

[toc] | [prev] | [next] | [standalone]

#1594020 — Re: Question about hwmon_attr_show_string

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

[toc] | [prev] | [next] | [standalone]

#1594037 — Re: Question about hwmon_attr_show_string

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

[toc] | [prev] | [next] | [standalone]

#1594322 — Re: Question about hwmon_attr_show_string

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web