Re: [PATCH 1/5] w83627ehf: Use hwmon_device_register_with_info and sensor groups

[Guenter Roeck]

On Thu, Mar 23, 2017 at 07:55:58PM +0100, Peter Huewe wrote:
> 
> 
> 
> 
> 
> Am 23. März 2017 19:43:39 MEZ schrieb Guenter Roeck :
> >On Thu, Mar 23, 2017 at 02:03:04PM +0100, Peter Huewe wrote:
> >> This patch replaces the old, deprecated call to hwmon_device_register
> >> with the new hwmon_device_register_with_info and converts the whole
> >> driver to the new hwmon interface using the hwmon_chip_info methods
> >> and the attribute_group method.
> >> 
> >> All standard attributes were converted to the corresponding
> >> hwmon_chip_info methods.
> >> For some functions a hwmon channel to device channel conversion had
> >to
> >> be performed, e.g. hwmon_in_alarm has the info for alert_5 in channel
> >8.
> >> 
> >> All non-standard attributes are converted to the attribute_group
> >method,
> >> by
> >> - adding them statically to the attribute_group if they are available
> >> for all variants of devices supported by this driver
> >> - adding them at probe time to the attribute_group if the
> >availability
> >> is depending on the actual chip type.
> >> The appropriate count of entries was reserved.
> >> 
> >> As a pre-condition a reference to the sio_data structure was moved
> >into
> >> w83627ehf_data for easier retrieval of the information, since this is
> >> much easier than trying to access the platform_data.
> >> 
> >> The driver is now much more "checkpatch clean" than it used to be,
> >but
> >> still not completely.
> >> The conversion saves about 20k in the resulting .ko
> >> 
> >> Tested with a NCT6776F chip.
> >> 
> >> v2:
> >> - converted to proper is_visible methods
> >> - applied minor feedback of v1
> >> 
> >Nicely done.
> >
> >Couple of nitpicks:
> >- Please no double empty lines (removed some, added some ;-)
> Okay, interestingly checkpatch did not complain.
> 
Yes, that was surprising. You introduced at least one.
Use checkpatch -f --strict on the final file.

> >- still a couple of c99 comments
> I thought I had them removed, or atleast not introduced new ones
> 
> >- (later or intro patch): dem_request_region() in probe could help
> > with cleanup, you could use the devm_ function for hwmon registration,
> >  and drop the remove function entirely.
> Yea makes sense, already thought about it, but then decided against it for 
> the first comversion.
> 
> >- if () return; else return; causes static analyzer hiccup.
> Okay, which analyzer has trouble with that?
> I ran Wolfram's ninja-check against it (sparse, smatch, coccinelle etc...) 
> and didn't see any issues.

Interesting. Who knows, since c99 is now permitted, maybe
if ()
return;
else
return;
is now permitted as well. Though I still think it is bad style.

> >  Please use if () return; return; (no else)
> >  You do that already most of the time but not always.
> 
> Shall I respin one more time?
> Or fixup in seperate patches?
> 
respin, please, to avoid interference with subsequent patches.

Thanks,
Guenter

> 
> >
> >Thanks,
> >Guenter
> >
> >> Signed-off-by: Peter Huewe 
> >> ---
> >>  drivers/hwmon/w83627ehf.c | 1536
> >+++--
> >>  1 file changed, 778 insertions(+), 758 deletions(-)
> >> 
> >> diff --git a/drivers/hwmon/w83627ehf.c b/drivers/hwmon/w83627ehf.c
> >> index ad68b6d9ff17..f2f33dd35fd4 100644
> >> --- a/drivers/hwmon/w83627ehf.c
> >> +++ b/drivers/hwmon/w83627ehf.c
> >> @@ -1,6 +1,7 @@
> >>  /*
> >>   *  w83627ehf - Driver for the hardware monitoring functionality of
> >>   *the Winbond W83627EHF Super-I/O chip
> >> + *  Copyright (C) 2017  Peter Huewe 
> >>   *  Copyright (C) 2005-2012  Jean Delvare 
> >>   *  Copyright (C) 2006  Yuan Mu (Winbond),
> >>   *Rudolf Marek 
> >> @@ -426,6 +427,11 @@ static inline u8 in_to_reg(u32 val, u8 nr, const
> >u16 *scale_in)
> >>  /*
> >>   * Data structures and manipulation thereof
> >>   */
> >> +struct w83627ehf_sio_data {
> >> +  int sioreg;
> >> +  enum kinds kind;
> >> +};
> >> +
> >>  
> >>  struct w83627ehf_data {
> >>int addr;   /* IO base of hw monitor block */
> >> @@ -514,11 +520,7 @@ struct w83627ehf_data {
> >>u8 fandiv1;
> >>u8 fandiv2;
> >>  #endif
> >> -};
> >> -
> >> -struct w83627ehf_sio_data {
> >> -  int sioreg;
> >> -  enum kinds kind;
> >> +  struct w83627ehf_sio_data *sio_data;
> >>  };
> >>  
> >>  /*
> >> @@ -679,7 +681,7 @@ static void w83627ehf_write_fan_div(struct
> >w83627ehf_data *data, int nr)
> >>  static void w83627ehf_write_fan_div_common(struct device *dev,
> >>   struct w83627ehf_data *data, int nr)
> >>  {
> >> -  struct w83627ehf_sio_data *sio_data = dev_get_platdata(dev);
> >> +  struct w83627ehf_sio_data *sio_data = data->sio_data;
> >>  
> >>if (sio_data->kind == nct6776)
> >>; /* no dividers, do nothing */
> >> @@ -730,11 +732,9 @@ static void 

Re: [PATCH 1/5] w83627ehf: Use hwmon_device_register_with_info and sensor groups

[Guenter Roeck]

On Thu, Mar 23, 2017 at 07:55:58PM +0100, Peter Huewe wrote:
> 
> 
> 
> 
> 
> Am 23. März 2017 19:43:39 MEZ schrieb Guenter Roeck :
> >On Thu, Mar 23, 2017 at 02:03:04PM +0100, Peter Huewe wrote:
> >> This patch replaces the old, deprecated call to hwmon_device_register
> >> with the new hwmon_device_register_with_info and converts the whole
> >> driver to the new hwmon interface using the hwmon_chip_info methods
> >> and the attribute_group method.
> >> 
> >> All standard attributes were converted to the corresponding
> >> hwmon_chip_info methods.
> >> For some functions a hwmon channel to device channel conversion had
> >to
> >> be performed, e.g. hwmon_in_alarm has the info for alert_5 in channel
> >8.
> >> 
> >> All non-standard attributes are converted to the attribute_group
> >method,
> >> by
> >> - adding them statically to the attribute_group if they are available
> >> for all variants of devices supported by this driver
> >> - adding them at probe time to the attribute_group if the
> >availability
> >> is depending on the actual chip type.
> >> The appropriate count of entries was reserved.
> >> 
> >> As a pre-condition a reference to the sio_data structure was moved
> >into
> >> w83627ehf_data for easier retrieval of the information, since this is
> >> much easier than trying to access the platform_data.
> >> 
> >> The driver is now much more "checkpatch clean" than it used to be,
> >but
> >> still not completely.
> >> The conversion saves about 20k in the resulting .ko
> >> 
> >> Tested with a NCT6776F chip.
> >> 
> >> v2:
> >> - converted to proper is_visible methods
> >> - applied minor feedback of v1
> >> 
> >Nicely done.
> >
> >Couple of nitpicks:
> >- Please no double empty lines (removed some, added some ;-)
> Okay, interestingly checkpatch did not complain.
> 
Yes, that was surprising. You introduced at least one.
Use checkpatch -f --strict on the final file.

> >- still a couple of c99 comments
> I thought I had them removed, or atleast not introduced new ones
> 
> >- (later or intro patch): dem_request_region() in probe could help
> > with cleanup, you could use the devm_ function for hwmon registration,
> >  and drop the remove function entirely.
> Yea makes sense, already thought about it, but then decided against it for 
> the first comversion.
> 
> >- if () return; else return; causes static analyzer hiccup.
> Okay, which analyzer has trouble with that?
> I ran Wolfram's ninja-check against it (sparse, smatch, coccinelle etc...) 
> and didn't see any issues.

Interesting. Who knows, since c99 is now permitted, maybe
if ()
return;
else
return;
is now permitted as well. Though I still think it is bad style.

> >  Please use if () return; return; (no else)
> >  You do that already most of the time but not always.
> 
> Shall I respin one more time?
> Or fixup in seperate patches?
> 
respin, please, to avoid interference with subsequent patches.

Thanks,
Guenter

> 
> >
> >Thanks,
> >Guenter
> >
> >> Signed-off-by: Peter Huewe 
> >> ---
> >>  drivers/hwmon/w83627ehf.c | 1536
> >+++--
> >>  1 file changed, 778 insertions(+), 758 deletions(-)
> >> 
> >> diff --git a/drivers/hwmon/w83627ehf.c b/drivers/hwmon/w83627ehf.c
> >> index ad68b6d9ff17..f2f33dd35fd4 100644
> >> --- a/drivers/hwmon/w83627ehf.c
> >> +++ b/drivers/hwmon/w83627ehf.c
> >> @@ -1,6 +1,7 @@
> >>  /*
> >>   *  w83627ehf - Driver for the hardware monitoring functionality of
> >>   *the Winbond W83627EHF Super-I/O chip
> >> + *  Copyright (C) 2017  Peter Huewe 
> >>   *  Copyright (C) 2005-2012  Jean Delvare 
> >>   *  Copyright (C) 2006  Yuan Mu (Winbond),
> >>   *Rudolf Marek 
> >> @@ -426,6 +427,11 @@ static inline u8 in_to_reg(u32 val, u8 nr, const
> >u16 *scale_in)
> >>  /*
> >>   * Data structures and manipulation thereof
> >>   */
> >> +struct w83627ehf_sio_data {
> >> +  int sioreg;
> >> +  enum kinds kind;
> >> +};
> >> +
> >>  
> >>  struct w83627ehf_data {
> >>int addr;   /* IO base of hw monitor block */
> >> @@ -514,11 +520,7 @@ struct w83627ehf_data {
> >>u8 fandiv1;
> >>u8 fandiv2;
> >>  #endif
> >> -};
> >> -
> >> -struct w83627ehf_sio_data {
> >> -  int sioreg;
> >> -  enum kinds kind;
> >> +  struct w83627ehf_sio_data *sio_data;
> >>  };
> >>  
> >>  /*
> >> @@ -679,7 +681,7 @@ static void w83627ehf_write_fan_div(struct
> >w83627ehf_data *data, int nr)
> >>  static void w83627ehf_write_fan_div_common(struct device *dev,
> >>   struct w83627ehf_data *data, int nr)
> >>  {
> >> -  struct w83627ehf_sio_data *sio_data = dev_get_platdata(dev);
> >> +  struct w83627ehf_sio_data *sio_data = data->sio_data;
> >>  
> >>if (sio_data->kind == nct6776)
> >>; /* no dividers, do nothing */
> >> @@ -730,11 +732,9 @@ static void w83627ehf_update_fan_div(struct
> >w83627ehf_data *data)
> >>  static void 

Re: [PATCH 1/5] w83627ehf: Use hwmon_device_register_with_info and sensor groups

[Peter Huewe]

Am 23. März 2017 19:43:39 MEZ schrieb Guenter Roeck :
>On Thu, Mar 23, 2017 at 02:03:04PM +0100, Peter Huewe wrote:
>> This patch replaces the old, deprecated call to hwmon_device_register
>> with the new hwmon_device_register_with_info and converts the whole
>> driver to the new hwmon interface using the hwmon_chip_info methods
>> and the attribute_group method.
>> 
>> All standard attributes were converted to the corresponding
>> hwmon_chip_info methods.
>> For some functions a hwmon channel to device channel conversion had
>to
>> be performed, e.g. hwmon_in_alarm has the info for alert_5 in channel
>8.
>> 
>> All non-standard attributes are converted to the attribute_group
>method,
>> by
>> - adding them statically to the attribute_group if they are available
>> for all variants of devices supported by this driver
>> - adding them at probe time to the attribute_group if the
>availability
>> is depending on the actual chip type.
>> The appropriate count of entries was reserved.
>> 
>> As a pre-condition a reference to the sio_data structure was moved
>into
>> w83627ehf_data for easier retrieval of the information, since this is
>> much easier than trying to access the platform_data.
>> 
>> The driver is now much more "checkpatch clean" than it used to be,
>but
>> still not completely.
>> The conversion saves about 20k in the resulting .ko
>> 
>> Tested with a NCT6776F chip.
>> 
>> v2:
>> - converted to proper is_visible methods
>> - applied minor feedback of v1
>> 
>Nicely done.
>
>Couple of nitpicks:
>- Please no double empty lines (removed some, added some ;-)
Okay, interestingly checkpatch did not complain.

>- still a couple of c99 comments
I thought I had them removed, or atleast not introduced new ones

>- (later or intro patch): dem_request_region() in probe could help
> with cleanup, you could use the devm_ function for hwmon registration,
>  and drop the remove function entirely.
Yea makes sense, already thought about it, but then decided against it for the 
first comversion.

>- if () return; else return; causes static analyzer hiccup.
Okay, which analyzer has trouble with that?
I ran Wolfram's ninja-check against it (sparse, smatch, coccinelle etc...) and 
didn't see any issues.
>  Please use if () return; return; (no else)
>  You do that already most of the time but not always.

Shall I respin one more time?
Or fixup in seperate patches?


>
>Thanks,
>Guenter
>
>> Signed-off-by: Peter Huewe 
>> ---
>>  drivers/hwmon/w83627ehf.c | 1536
>+++--
>>  1 file changed, 778 insertions(+), 758 deletions(-)
>> 
>> diff --git a/drivers/hwmon/w83627ehf.c b/drivers/hwmon/w83627ehf.c
>> index ad68b6d9ff17..f2f33dd35fd4 100644
>> --- a/drivers/hwmon/w83627ehf.c
>> +++ b/drivers/hwmon/w83627ehf.c
>> @@ -1,6 +1,7 @@
>>  /*
>>   *  w83627ehf - Driver for the hardware monitoring functionality of
>>   *  the Winbond W83627EHF Super-I/O chip
>> + *  Copyright (C) 2017  Peter Huewe 
>>   *  Copyright (C) 2005-2012  Jean Delvare 
>>   *  Copyright (C) 2006  Yuan Mu (Winbond),
>>   *  Rudolf Marek 
>> @@ -426,6 +427,11 @@ static inline u8 in_to_reg(u32 val, u8 nr, const
>u16 *scale_in)
>>  /*
>>   * Data structures and manipulation thereof
>>   */
>> +struct w83627ehf_sio_data {
>> +int sioreg;
>> +enum kinds kind;
>> +};
>> +
>>  
>>  struct w83627ehf_data {
>>  int addr;   /* IO base of hw monitor block */
>> @@ -514,11 +520,7 @@ struct w83627ehf_data {
>>  u8 fandiv1;
>>  u8 fandiv2;
>>  #endif
>> -};
>> -
>> -struct w83627ehf_sio_data {
>> -int sioreg;
>> -enum kinds kind;
>> +struct w83627ehf_sio_data *sio_data;
>>  };
>>  
>>  /*
>> @@ -679,7 +681,7 @@ static void w83627ehf_write_fan_div(struct
>w83627ehf_data *data, int nr)
>>  static void w83627ehf_write_fan_div_common(struct device *dev,
>> struct w83627ehf_data *data, int nr)
>>  {
>> -struct w83627ehf_sio_data *sio_data = dev_get_platdata(dev);
>> +struct w83627ehf_sio_data *sio_data = data->sio_data;
>>  
>>  if (sio_data->kind == nct6776)
>>  ; /* no dividers, do nothing */
>> @@ -730,11 +732,9 @@ static void w83627ehf_update_fan_div(struct
>w83627ehf_data *data)
>>  static void w83627ehf_update_fan_div_common(struct device *dev,
>>  struct w83627ehf_data *data)
>>  {
>> -struct w83627ehf_sio_data *sio_data = dev_get_platdata(dev);
>
>   struct w83627ehf_sio_data *sio_data = data->sio_data;
>
>Though I agree it doesn't really matter, since this code is going to be
>removed anyway, so feel free to ignore that one. Just had to make the
>point :-)
>
>> -
>> -if (sio_data->kind == nct6776)
>> +if (data->sio_data->kind == nct6776)
>>  ; /* no dividers, do nothing */
>> -else if (sio_data->kind == nct6775)
>> +else if 

Re: [PATCH 1/5] w83627ehf: Use hwmon_device_register_with_info and sensor groups

[Peter Huewe]

Am 23. März 2017 19:43:39 MEZ schrieb Guenter Roeck :
>On Thu, Mar 23, 2017 at 02:03:04PM +0100, Peter Huewe wrote:
>> This patch replaces the old, deprecated call to hwmon_device_register
>> with the new hwmon_device_register_with_info and converts the whole
>> driver to the new hwmon interface using the hwmon_chip_info methods
>> and the attribute_group method.
>> 
>> All standard attributes were converted to the corresponding
>> hwmon_chip_info methods.
>> For some functions a hwmon channel to device channel conversion had
>to
>> be performed, e.g. hwmon_in_alarm has the info for alert_5 in channel
>8.
>> 
>> All non-standard attributes are converted to the attribute_group
>method,
>> by
>> - adding them statically to the attribute_group if they are available
>> for all variants of devices supported by this driver
>> - adding them at probe time to the attribute_group if the
>availability
>> is depending on the actual chip type.
>> The appropriate count of entries was reserved.
>> 
>> As a pre-condition a reference to the sio_data structure was moved
>into
>> w83627ehf_data for easier retrieval of the information, since this is
>> much easier than trying to access the platform_data.
>> 
>> The driver is now much more "checkpatch clean" than it used to be,
>but
>> still not completely.
>> The conversion saves about 20k in the resulting .ko
>> 
>> Tested with a NCT6776F chip.
>> 
>> v2:
>> - converted to proper is_visible methods
>> - applied minor feedback of v1
>> 
>Nicely done.
>
>Couple of nitpicks:
>- Please no double empty lines (removed some, added some ;-)
Okay, interestingly checkpatch did not complain.

>- still a couple of c99 comments
I thought I had them removed, or atleast not introduced new ones

>- (later or intro patch): dem_request_region() in probe could help
> with cleanup, you could use the devm_ function for hwmon registration,
>  and drop the remove function entirely.
Yea makes sense, already thought about it, but then decided against it for the 
first comversion.

>- if () return; else return; causes static analyzer hiccup.
Okay, which analyzer has trouble with that?
I ran Wolfram's ninja-check against it (sparse, smatch, coccinelle etc...) and 
didn't see any issues.
>  Please use if () return; return; (no else)
>  You do that already most of the time but not always.

Shall I respin one more time?
Or fixup in seperate patches?


>
>Thanks,
>Guenter
>
>> Signed-off-by: Peter Huewe 
>> ---
>>  drivers/hwmon/w83627ehf.c | 1536
>+++--
>>  1 file changed, 778 insertions(+), 758 deletions(-)
>> 
>> diff --git a/drivers/hwmon/w83627ehf.c b/drivers/hwmon/w83627ehf.c
>> index ad68b6d9ff17..f2f33dd35fd4 100644
>> --- a/drivers/hwmon/w83627ehf.c
>> +++ b/drivers/hwmon/w83627ehf.c
>> @@ -1,6 +1,7 @@
>>  /*
>>   *  w83627ehf - Driver for the hardware monitoring functionality of
>>   *  the Winbond W83627EHF Super-I/O chip
>> + *  Copyright (C) 2017  Peter Huewe 
>>   *  Copyright (C) 2005-2012  Jean Delvare 
>>   *  Copyright (C) 2006  Yuan Mu (Winbond),
>>   *  Rudolf Marek 
>> @@ -426,6 +427,11 @@ static inline u8 in_to_reg(u32 val, u8 nr, const
>u16 *scale_in)
>>  /*
>>   * Data structures and manipulation thereof
>>   */
>> +struct w83627ehf_sio_data {
>> +int sioreg;
>> +enum kinds kind;
>> +};
>> +
>>  
>>  struct w83627ehf_data {
>>  int addr;   /* IO base of hw monitor block */
>> @@ -514,11 +520,7 @@ struct w83627ehf_data {
>>  u8 fandiv1;
>>  u8 fandiv2;
>>  #endif
>> -};
>> -
>> -struct w83627ehf_sio_data {
>> -int sioreg;
>> -enum kinds kind;
>> +struct w83627ehf_sio_data *sio_data;
>>  };
>>  
>>  /*
>> @@ -679,7 +681,7 @@ static void w83627ehf_write_fan_div(struct
>w83627ehf_data *data, int nr)
>>  static void w83627ehf_write_fan_div_common(struct device *dev,
>> struct w83627ehf_data *data, int nr)
>>  {
>> -struct w83627ehf_sio_data *sio_data = dev_get_platdata(dev);
>> +struct w83627ehf_sio_data *sio_data = data->sio_data;
>>  
>>  if (sio_data->kind == nct6776)
>>  ; /* no dividers, do nothing */
>> @@ -730,11 +732,9 @@ static void w83627ehf_update_fan_div(struct
>w83627ehf_data *data)
>>  static void w83627ehf_update_fan_div_common(struct device *dev,
>>  struct w83627ehf_data *data)
>>  {
>> -struct w83627ehf_sio_data *sio_data = dev_get_platdata(dev);
>
>   struct w83627ehf_sio_data *sio_data = data->sio_data;
>
>Though I agree it doesn't really matter, since this code is going to be
>removed anyway, so feel free to ignore that one. Just had to make the
>point :-)
>
>> -
>> -if (sio_data->kind == nct6776)
>> +if (data->sio_data->kind == nct6776)
>>  ; /* no dividers, do nothing */
>> -else if (sio_data->kind == nct6775)
>> +else if (data->sio_data->kind == nct6775)
>>  nct6775_update_fan_div(data);
>>  else
>>

Re: [PATCH 1/5] w83627ehf: Use hwmon_device_register_with_info and sensor groups

[Guenter Roeck]

On Thu, Mar 23, 2017 at 02:03:04PM +0100, Peter Huewe wrote:
> This patch replaces the old, deprecated call to hwmon_device_register
> with the new hwmon_device_register_with_info and converts the whole
> driver to the new hwmon interface using the hwmon_chip_info methods
> and the attribute_group method.
> 
> All standard attributes were converted to the corresponding
> hwmon_chip_info methods.
> For some functions a hwmon channel to device channel conversion had to
> be performed, e.g. hwmon_in_alarm has the info for alert_5 in channel 8.
> 
> All non-standard attributes are converted to the attribute_group method,
> by
> - adding them statically to the attribute_group if they are available
> for all variants of devices supported by this driver
> - adding them at probe time to the attribute_group if the availability
> is depending on the actual chip type.
> The appropriate count of entries was reserved.
> 
> As a pre-condition a reference to the sio_data structure was moved into
> w83627ehf_data for easier retrieval of the information, since this is
> much easier than trying to access the platform_data.
> 
> The driver is now much more "checkpatch clean" than it used to be, but
> still not completely.
> The conversion saves about 20k in the resulting .ko
> 
> Tested with a NCT6776F chip.
> 
> v2:
> - converted to proper is_visible methods
> - applied minor feedback of v1
> 
Nicely done.

Couple of nitpicks:
- Please no double empty lines (removed some, added some ;-)
- still a couple of c99 comments
- (later or intro patch): dem_request_region() in probe could help
  with cleanup, you could use the devm_ function for hwmon registration,
  and drop the remove function entirely.
- if () return; else return; causes static analyzer hiccup.
  Please use if () return; return; (no else)
  You do that already most of the time but not always.

Thanks,
Guenter

> Signed-off-by: Peter Huewe 
> ---
>  drivers/hwmon/w83627ehf.c | 1536 
> +++--
>  1 file changed, 778 insertions(+), 758 deletions(-)
> 
> diff --git a/drivers/hwmon/w83627ehf.c b/drivers/hwmon/w83627ehf.c
> index ad68b6d9ff17..f2f33dd35fd4 100644
> --- a/drivers/hwmon/w83627ehf.c
> +++ b/drivers/hwmon/w83627ehf.c
> @@ -1,6 +1,7 @@
>  /*
>   *  w83627ehf - Driver for the hardware monitoring functionality of
>   *   the Winbond W83627EHF Super-I/O chip
> + *  Copyright (C) 2017  Peter Huewe 
>   *  Copyright (C) 2005-2012  Jean Delvare 
>   *  Copyright (C) 2006  Yuan Mu (Winbond),
>   *   Rudolf Marek 
> @@ -426,6 +427,11 @@ static inline u8 in_to_reg(u32 val, u8 nr, const u16 
> *scale_in)
>  /*
>   * Data structures and manipulation thereof
>   */
> +struct w83627ehf_sio_data {
> + int sioreg;
> + enum kinds kind;
> +};
> +
>  
>  struct w83627ehf_data {
>   int addr;   /* IO base of hw monitor block */
> @@ -514,11 +520,7 @@ struct w83627ehf_data {
>   u8 fandiv1;
>   u8 fandiv2;
>  #endif
> -};
> -
> -struct w83627ehf_sio_data {
> - int sioreg;
> - enum kinds kind;
> + struct w83627ehf_sio_data *sio_data;
>  };
>  
>  /*
> @@ -679,7 +681,7 @@ static void w83627ehf_write_fan_div(struct w83627ehf_data 
> *data, int nr)
>  static void w83627ehf_write_fan_div_common(struct device *dev,
>  struct w83627ehf_data *data, int nr)
>  {
> - struct w83627ehf_sio_data *sio_data = dev_get_platdata(dev);
> + struct w83627ehf_sio_data *sio_data = data->sio_data;
>  
>   if (sio_data->kind == nct6776)
>   ; /* no dividers, do nothing */
> @@ -730,11 +732,9 @@ static void w83627ehf_update_fan_div(struct 
> w83627ehf_data *data)
>  static void w83627ehf_update_fan_div_common(struct device *dev,
>   struct w83627ehf_data *data)
>  {
> - struct w83627ehf_sio_data *sio_data = dev_get_platdata(dev);

struct w83627ehf_sio_data *sio_data = data->sio_data;

Though I agree it doesn't really matter, since this code is going to be
removed anyway, so feel free to ignore that one. Just had to make the
point :-)

> -
> - if (sio_data->kind == nct6776)
> + if (data->sio_data->kind == nct6776)
>   ; /* no dividers, do nothing */
> - else if (sio_data->kind == nct6775)
> + else if (data->sio_data->kind == nct6775)
>   nct6775_update_fan_div(data);
>   else
>   w83627ehf_update_fan_div(data);
> @@ -787,7 +787,7 @@ static void w83627ehf_update_pwm(struct w83627ehf_data 
> *data)
>  static void w83627ehf_update_pwm_common(struct device *dev,
>   struct w83627ehf_data *data)
>  {
> - struct w83627ehf_sio_data *sio_data = dev_get_platdata(dev);
> + struct w83627ehf_sio_data *sio_data = data->sio_data;
>  
>   if (sio_data->kind == nct6775 || sio_data->kind == nct6776)
>   

Re: [PATCH 1/5] w83627ehf: Use hwmon_device_register_with_info and sensor groups

[Guenter Roeck]

On Thu, Mar 23, 2017 at 02:03:04PM +0100, Peter Huewe wrote:
> This patch replaces the old, deprecated call to hwmon_device_register
> with the new hwmon_device_register_with_info and converts the whole
> driver to the new hwmon interface using the hwmon_chip_info methods
> and the attribute_group method.
> 
> All standard attributes were converted to the corresponding
> hwmon_chip_info methods.
> For some functions a hwmon channel to device channel conversion had to
> be performed, e.g. hwmon_in_alarm has the info for alert_5 in channel 8.
> 
> All non-standard attributes are converted to the attribute_group method,
> by
> - adding them statically to the attribute_group if they are available
> for all variants of devices supported by this driver
> - adding them at probe time to the attribute_group if the availability
> is depending on the actual chip type.
> The appropriate count of entries was reserved.
> 
> As a pre-condition a reference to the sio_data structure was moved into
> w83627ehf_data for easier retrieval of the information, since this is
> much easier than trying to access the platform_data.
> 
> The driver is now much more "checkpatch clean" than it used to be, but
> still not completely.
> The conversion saves about 20k in the resulting .ko
> 
> Tested with a NCT6776F chip.
> 
> v2:
> - converted to proper is_visible methods
> - applied minor feedback of v1
> 
Nicely done.

Couple of nitpicks:
- Please no double empty lines (removed some, added some ;-)
- still a couple of c99 comments
- (later or intro patch): dem_request_region() in probe could help
  with cleanup, you could use the devm_ function for hwmon registration,
  and drop the remove function entirely.
- if () return; else return; causes static analyzer hiccup.
  Please use if () return; return; (no else)
  You do that already most of the time but not always.

Thanks,
Guenter

> Signed-off-by: Peter Huewe 
> ---
>  drivers/hwmon/w83627ehf.c | 1536 
> +++--
>  1 file changed, 778 insertions(+), 758 deletions(-)
> 
> diff --git a/drivers/hwmon/w83627ehf.c b/drivers/hwmon/w83627ehf.c
> index ad68b6d9ff17..f2f33dd35fd4 100644
> --- a/drivers/hwmon/w83627ehf.c
> +++ b/drivers/hwmon/w83627ehf.c
> @@ -1,6 +1,7 @@
>  /*
>   *  w83627ehf - Driver for the hardware monitoring functionality of
>   *   the Winbond W83627EHF Super-I/O chip
> + *  Copyright (C) 2017  Peter Huewe 
>   *  Copyright (C) 2005-2012  Jean Delvare 
>   *  Copyright (C) 2006  Yuan Mu (Winbond),
>   *   Rudolf Marek 
> @@ -426,6 +427,11 @@ static inline u8 in_to_reg(u32 val, u8 nr, const u16 
> *scale_in)
>  /*
>   * Data structures and manipulation thereof
>   */
> +struct w83627ehf_sio_data {
> + int sioreg;
> + enum kinds kind;
> +};
> +
>  
>  struct w83627ehf_data {
>   int addr;   /* IO base of hw monitor block */
> @@ -514,11 +520,7 @@ struct w83627ehf_data {
>   u8 fandiv1;
>   u8 fandiv2;
>  #endif
> -};
> -
> -struct w83627ehf_sio_data {
> - int sioreg;
> - enum kinds kind;
> + struct w83627ehf_sio_data *sio_data;
>  };
>  
>  /*
> @@ -679,7 +681,7 @@ static void w83627ehf_write_fan_div(struct w83627ehf_data 
> *data, int nr)
>  static void w83627ehf_write_fan_div_common(struct device *dev,
>  struct w83627ehf_data *data, int nr)
>  {
> - struct w83627ehf_sio_data *sio_data = dev_get_platdata(dev);
> + struct w83627ehf_sio_data *sio_data = data->sio_data;
>  
>   if (sio_data->kind == nct6776)
>   ; /* no dividers, do nothing */
> @@ -730,11 +732,9 @@ static void w83627ehf_update_fan_div(struct 
> w83627ehf_data *data)
>  static void w83627ehf_update_fan_div_common(struct device *dev,
>   struct w83627ehf_data *data)
>  {
> - struct w83627ehf_sio_data *sio_data = dev_get_platdata(dev);

struct w83627ehf_sio_data *sio_data = data->sio_data;

Though I agree it doesn't really matter, since this code is going to be
removed anyway, so feel free to ignore that one. Just had to make the
point :-)

> -
> - if (sio_data->kind == nct6776)
> + if (data->sio_data->kind == nct6776)
>   ; /* no dividers, do nothing */
> - else if (sio_data->kind == nct6775)
> + else if (data->sio_data->kind == nct6775)
>   nct6775_update_fan_div(data);
>   else
>   w83627ehf_update_fan_div(data);
> @@ -787,7 +787,7 @@ static void w83627ehf_update_pwm(struct w83627ehf_data 
> *data)
>  static void w83627ehf_update_pwm_common(struct device *dev,
>   struct w83627ehf_data *data)
>  {
> - struct w83627ehf_sio_data *sio_data = dev_get_platdata(dev);
> + struct w83627ehf_sio_data *sio_data = data->sio_data;
>  
>   if (sio_data->kind == nct6775 || sio_data->kind == nct6776)
>   nct6775_update_pwm(data);
> @@ -798,8 +798,7 @@ static void 

Re: [PATCH 1/5] w83627ehf: Use hwmon_device_register_with_info and sensor groups

[Guenter Roeck]

On 03/23/2017 06:05 AM, Peter Hüwe wrote:

This is of course v2 of the series
Forgot to add it to git-send-email, sorry.
Shall I resend with v2 in subject?



No, it's ok. At least you have a change log :-).

Thanks,
Guenter

Re: [PATCH 1/5] w83627ehf: Use hwmon_device_register_with_info and sensor groups

[Guenter Roeck]

On 03/23/2017 06:05 AM, Peter Hüwe wrote:

This is of course v2 of the series
Forgot to add it to git-send-email, sorry.
Shall I resend with v2 in subject?



No, it's ok. At least you have a change log :-).

Thanks,
Guenter

Re: [PATCH 1/5] w83627ehf: Use hwmon_device_register_with_info and sensor groups

[Peter Hüwe]

This is of course v2 of the series
Forgot to add it to git-send-email, sorry.
Shall I resend with v2 in subject?

Peter

Re: [PATCH 1/5] w83627ehf: Use hwmon_device_register_with_info and sensor groups

[Peter Hüwe]

This is of course v2 of the series
Forgot to add it to git-send-email, sorry.
Shall I resend with v2 in subject?

Peter