| Message ID | 1514976294-31880-1-git-send-email-festevam@gmail.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | Zhang Rui |
| Headers | show |
On Wed, Jan 03, 2018 at 08:44:54AM -0200, Fabio Estevam wrote: > From: Fabio Estevam <fabio.estevam@nxp.com> > > Booting Linux on a mx6q based board leads to the following warning: > > (NULL device *): hwmon_device_register() is deprecated. Please convert the > driver to use hwmon_device_register_with_info(). > > , so do the conversion as suggested. > > Also, this results in the core taking care of creating the 'name' > attribute, so drop the code doing that from the thermal driver. > > The initial attempt to convert this driver to > hwmon_device_register_with_info() caused issues on the N900 platform > in commit 611fb68062f ("thermal: thermal_hwmon: Convert to > hwmon_device_register_with_info()"): > > bq27xxx-battery 2-0055: failed to register battery > bq27xxx-battery: probe of 2-0055 failed with error -22 > ... > rx51-battery: probe of n900-battery failed with error -22 > > , leading to a revert in commit 3feb479cea37 ("Revert "thermal: > thermal_hwmon: Convert to hwmon_device_register_with_info()""). > > The probe errors happened due to the '-' character being present in > the name of the power supply devices: bq27200-0 and rx51-battery. > > Since commit 74d3b6419772 ("hwmon: Relax name attribute validation > for new APIs") hwmon will no longer treat these names as errors, > allowing the transition for hwmon_device_register_with_info() to > happen in a safely manner. I am mostly fine with the patch as long as we get an ack from hwmon folks. > > Cc: Pavel Machek <pavel@ucw.cz> > Cc: Guenter Roeck <linux@roeck-us.net> > Signed-off-by: Fabio Estevam <fabio.estevam@nxp.com> > --- > Pavel, > > Could you please test this patch on your N900 platform? > > Thanks > > drivers/thermal/thermal_hwmon.c | 20 +++----------------- > 1 file changed, 3 insertions(+), 17 deletions(-) > > diff --git a/drivers/thermal/thermal_hwmon.c b/drivers/thermal/thermal_hwmon.c > index 541af59..c4a508a 100644 > --- a/drivers/thermal/thermal_hwmon.c > +++ b/drivers/thermal/thermal_hwmon.c > @@ -59,14 +59,6 @@ static LIST_HEAD(thermal_hwmon_list); > static DEFINE_MUTEX(thermal_hwmon_list_lock); > > static ssize_t > -name_show(struct device *dev, struct device_attribute *attr, char *buf) > -{ > - struct thermal_hwmon_device *hwmon = dev_get_drvdata(dev); > - return sprintf(buf, "%s\n", hwmon->type); > -} > -static DEVICE_ATTR_RO(name); > - > -static ssize_t > temp_input_show(struct device *dev, struct device_attribute *attr, char *buf) > { > int temperature; > @@ -165,15 +157,12 @@ int thermal_add_hwmon_sysfs(struct thermal_zone_device *tz) > > INIT_LIST_HEAD(&hwmon->tz_list); > strlcpy(hwmon->type, tz->type, THERMAL_NAME_LENGTH); > - hwmon->device = hwmon_device_register(NULL); > + hwmon->device = hwmon_device_register_with_info(NULL, hwmon->type, > + hwmon, NULL, NULL); > if (IS_ERR(hwmon->device)) { > result = PTR_ERR(hwmon->device); > goto free_mem; > } > - dev_set_drvdata(hwmon->device, hwmon); > - result = device_create_file(hwmon->device, &dev_attr_name); > - if (result) > - goto free_mem; > > register_sys_interface: > temp = kzalloc(sizeof(*temp), GFP_KERNEL); > @@ -222,10 +211,8 @@ int thermal_add_hwmon_sysfs(struct thermal_zone_device *tz) > free_temp_mem: > kfree(temp); > unregister_name: > - if (new_hwmon_device) { > - device_remove_file(hwmon->device, &dev_attr_name); > + if (new_hwmon_device) > hwmon_device_unregister(hwmon->device); > - } > free_mem: > if (new_hwmon_device) > kfree(hwmon); > @@ -267,7 +254,6 @@ void thermal_remove_hwmon_sysfs(struct thermal_zone_device *tz) > list_del(&hwmon->node); > mutex_unlock(&thermal_hwmon_list_lock); > > - device_remove_file(hwmon->device, &dev_attr_name); > hwmon_device_unregister(hwmon->device); > kfree(hwmon); > } > -- > 2.7.4 >
On 01/10/2018 11:29 AM, Eduardo Valentin wrote: > On Wed, Jan 03, 2018 at 08:44:54AM -0200, Fabio Estevam wrote: >> From: Fabio Estevam <fabio.estevam@nxp.com> >> >> Booting Linux on a mx6q based board leads to the following warning: >> >> (NULL device *): hwmon_device_register() is deprecated. Please convert the >> driver to use hwmon_device_register_with_info(). >> >> , so do the conversion as suggested. >> >> Also, this results in the core taking care of creating the 'name' >> attribute, so drop the code doing that from the thermal driver. >> >> The initial attempt to convert this driver to >> hwmon_device_register_with_info() caused issues on the N900 platform >> in commit 611fb68062f ("thermal: thermal_hwmon: Convert to >> hwmon_device_register_with_info()"): >> >> bq27xxx-battery 2-0055: failed to register battery >> bq27xxx-battery: probe of 2-0055 failed with error -22 >> ... >> rx51-battery: probe of n900-battery failed with error -22 >> >> , leading to a revert in commit 3feb479cea37 ("Revert "thermal: >> thermal_hwmon: Convert to hwmon_device_register_with_info()""). >> >> The probe errors happened due to the '-' character being present in >> the name of the power supply devices: bq27200-0 and rx51-battery. >> >> Since commit 74d3b6419772 ("hwmon: Relax name attribute validation >> for new APIs") hwmon will no longer treat these names as errors, >> allowing the transition for hwmon_device_register_with_info() to >> happen in a safely manner. > > I am mostly fine with the patch as long as we get an ack from hwmon > folks. > An official one ? I am not really happy with the patch. The conversion to the _info API should be a real conversion, not just one to silence the warning. However, that would require a 'dev' argument, and I have absolutely no idea how to do that. Given that, Acked-by: Guenter Roeck <linux@roeck-us.net> Guenter >> >> Cc: Pavel Machek <pavel@ucw.cz> >> Cc: Guenter Roeck <linux@roeck-us.net> >> Signed-off-by: Fabio Estevam <fabio.estevam@nxp.com> >> --- >> Pavel, >> >> Could you please test this patch on your N900 platform? >> >> Thanks >> >> drivers/thermal/thermal_hwmon.c | 20 +++----------------- >> 1 file changed, 3 insertions(+), 17 deletions(-) >> >> diff --git a/drivers/thermal/thermal_hwmon.c b/drivers/thermal/thermal_hwmon.c >> index 541af59..c4a508a 100644 >> --- a/drivers/thermal/thermal_hwmon.c >> +++ b/drivers/thermal/thermal_hwmon.c >> @@ -59,14 +59,6 @@ static LIST_HEAD(thermal_hwmon_list); >> static DEFINE_MUTEX(thermal_hwmon_list_lock); >> >> static ssize_t >> -name_show(struct device *dev, struct device_attribute *attr, char *buf) >> -{ >> - struct thermal_hwmon_device *hwmon = dev_get_drvdata(dev); >> - return sprintf(buf, "%s\n", hwmon->type); >> -} >> -static DEVICE_ATTR_RO(name); >> - >> -static ssize_t >> temp_input_show(struct device *dev, struct device_attribute *attr, char *buf) >> { >> int temperature; >> @@ -165,15 +157,12 @@ int thermal_add_hwmon_sysfs(struct thermal_zone_device *tz) >> >> INIT_LIST_HEAD(&hwmon->tz_list); >> strlcpy(hwmon->type, tz->type, THERMAL_NAME_LENGTH); >> - hwmon->device = hwmon_device_register(NULL); >> + hwmon->device = hwmon_device_register_with_info(NULL, hwmon->type, >> + hwmon, NULL, NULL); >> if (IS_ERR(hwmon->device)) { >> result = PTR_ERR(hwmon->device); >> goto free_mem; >> } >> - dev_set_drvdata(hwmon->device, hwmon); >> - result = device_create_file(hwmon->device, &dev_attr_name); >> - if (result) >> - goto free_mem; >> >> register_sys_interface: >> temp = kzalloc(sizeof(*temp), GFP_KERNEL); >> @@ -222,10 +211,8 @@ int thermal_add_hwmon_sysfs(struct thermal_zone_device *tz) >> free_temp_mem: >> kfree(temp); >> unregister_name: >> - if (new_hwmon_device) { >> - device_remove_file(hwmon->device, &dev_attr_name); >> + if (new_hwmon_device) >> hwmon_device_unregister(hwmon->device); >> - } >> free_mem: >> if (new_hwmon_device) >> kfree(hwmon); >> @@ -267,7 +254,6 @@ void thermal_remove_hwmon_sysfs(struct thermal_zone_device *tz) >> list_del(&hwmon->node); >> mutex_unlock(&thermal_hwmon_list_lock); >> >> - device_remove_file(hwmon->device, &dev_attr_name); >> hwmon_device_unregister(hwmon->device); >> kfree(hwmon); >> } >> -- >> 2.7.4 >> >
On Wed, 2018-01-03 at 08:44 -0200, Fabio Estevam wrote: > From: Fabio Estevam <fabio.estevam@nxp.com> > > Booting Linux on a mx6q based board leads to the following warning: > > (NULL device *): hwmon_device_register() is deprecated. Please > convert the > driver to use hwmon_device_register_with_info(). > > , so do the conversion as suggested. > > Also, this results in the core taking care of creating the 'name' > attribute, so drop the code doing that from the thermal driver. > > The initial attempt to convert this driver to > hwmon_device_register_with_info() caused issues on the N900 platform > in commit 611fb68062f ("thermal: thermal_hwmon: Convert to > hwmon_device_register_with_info()"): the commit id is 7611fb68062f. > > bq27xxx-battery 2-0055: failed to register battery > bq27xxx-battery: probe of 2-0055 failed with error -22 > ... > rx51-battery: probe of n900-battery failed with error -22 > > , leading to a revert in commit 3feb479cea37 ("Revert "thermal: > thermal_hwmon: Convert to hwmon_device_register_with_info()""). > don't know why but checkpatch.pl always complains ERROR: Please use git commit description style 'commit <12+ chars of sha1> ("<title line>")' - ie: 'commit 3feb479cea37 ("Revert "thermal: thermal_hwmon: Convert to hwmon_device_register_with_info()"")' #37: , leading to a revert in commit 3feb479cea37 ("Revert "thermal: total: 1 errors, 0 warnings, 49 lines checked thanks, rui > The probe errors happened due to the '-' character being present in > the name of the power supply devices: bq27200-0 and rx51-battery. > > Since commit 74d3b6419772 ("hwmon: Relax name attribute validation > for new APIs") hwmon will no longer treat these names as errors, > allowing the transition for hwmon_device_register_with_info() to > happen in a safely manner. > > Cc: Pavel Machek <pavel@ucw.cz> > Cc: Guenter Roeck <linux@roeck-us.net> > Signed-off-by: Fabio Estevam <fabio.estevam@nxp.com> > --- > Pavel, > > Could you please test this patch on your N900 platform? > > Thanks > > drivers/thermal/thermal_hwmon.c | 20 +++----------------- > 1 file changed, 3 insertions(+), 17 deletions(-) > > diff --git a/drivers/thermal/thermal_hwmon.c > b/drivers/thermal/thermal_hwmon.c > index 541af59..c4a508a 100644 > --- a/drivers/thermal/thermal_hwmon.c > +++ b/drivers/thermal/thermal_hwmon.c > @@ -59,14 +59,6 @@ static LIST_HEAD(thermal_hwmon_list); > static DEFINE_MUTEX(thermal_hwmon_list_lock); > > static ssize_t > -name_show(struct device *dev, struct device_attribute *attr, char > *buf) > -{ > - struct thermal_hwmon_device *hwmon = dev_get_drvdata(dev); > - return sprintf(buf, "%s\n", hwmon->type); > -} > -static DEVICE_ATTR_RO(name); > - > -static ssize_t > temp_input_show(struct device *dev, struct device_attribute *attr, > char *buf) > { > int temperature; > @@ -165,15 +157,12 @@ int thermal_add_hwmon_sysfs(struct > thermal_zone_device *tz) > > INIT_LIST_HEAD(&hwmon->tz_list); > strlcpy(hwmon->type, tz->type, THERMAL_NAME_LENGTH); > - hwmon->device = hwmon_device_register(NULL); > + hwmon->device = hwmon_device_register_with_info(NULL, hwmon- > >type, > + hwmon, NULL, > NULL); > if (IS_ERR(hwmon->device)) { > result = PTR_ERR(hwmon->device); > goto free_mem; > } > - dev_set_drvdata(hwmon->device, hwmon); > - result = device_create_file(hwmon->device, &dev_attr_name); > - if (result) > - goto free_mem; > > register_sys_interface: > temp = kzalloc(sizeof(*temp), GFP_KERNEL); > @@ -222,10 +211,8 @@ int thermal_add_hwmon_sysfs(struct > thermal_zone_device *tz) > free_temp_mem: > kfree(temp); > unregister_name: > - if (new_hwmon_device) { > - device_remove_file(hwmon->device, &dev_attr_name); > + if (new_hwmon_device) > hwmon_device_unregister(hwmon->device); > - } > free_mem: > if (new_hwmon_device) > kfree(hwmon); > @@ -267,7 +254,6 @@ void thermal_remove_hwmon_sysfs(struct > thermal_zone_device *tz) > list_del(&hwmon->node); > mutex_unlock(&thermal_hwmon_list_lock); > > - device_remove_file(hwmon->device, &dev_attr_name); > hwmon_device_unregister(hwmon->device); > kfree(hwmon); > }
Hi Rui, On Sat, Jan 13, 2018 at 11:47 AM, Zhang Rui <rui.zhang@intel.com> wrote: > the commit id is 7611fb68062f. Thanks, will fix it in v2. > don't know why but checkpatch.pl always complains > > ERROR: Please use git commit description style 'commit <12+ chars of > sha1> ("<title line>")' - ie: 'commit 3feb479cea37 ("Revert "thermal: > thermal_hwmon: Convert to hwmon_device_register_with_info()"")' > #37: > , leading to a revert in commit 3feb479cea37 ("Revert "thermal: > > total: 1 errors, 0 warnings, 49 lines checked Yes, not sure why checkpatch complains on this one too. I am not able to see anything wrong with the commit description. Thanks
On 01/13/2018 05:47 AM, Zhang Rui wrote: > On Wed, 2018-01-03 at 08:44 -0200, Fabio Estevam wrote: >> From: Fabio Estevam <fabio.estevam@nxp.com> >> >> Booting Linux on a mx6q based board leads to the following warning: >> >> (NULL device *): hwmon_device_register() is deprecated. Please >> convert the >> driver to use hwmon_device_register_with_info(). >> >> , so do the conversion as suggested. >> >> Also, this results in the core taking care of creating the 'name' >> attribute, so drop the code doing that from the thermal driver. >> >> The initial attempt to convert this driver to >> hwmon_device_register_with_info() caused issues on the N900 platform >> in commit 611fb68062f ("thermal: thermal_hwmon: Convert to >> hwmon_device_register_with_info()"): > > the commit id is 7611fb68062f. > >> >> bq27xxx-battery 2-0055: failed to register battery >> bq27xxx-battery: probe of 2-0055 failed with error -22 >> ... >> rx51-battery: probe of n900-battery failed with error -22 >> >> , leading to a revert in commit 3feb479cea37 ("Revert "thermal: >> thermal_hwmon: Convert to hwmon_device_register_with_info()""). >> > don't know why but checkpatch.pl always complains > > ERROR: Please use git commit description style 'commit <12+ chars of > sha1> ("<title line>")' - ie: 'commit 3feb479cea37 ("Revert "thermal: > thermal_hwmon: Convert to hwmon_device_register_with_info()"")' > #37: > , leading to a revert in commit 3feb479cea37 ("Revert "thermal: > > total: 1 errors, 0 warnings, 49 lines checked > I bet it gets confused by the quotes. Guenter > thanks, > rui >> The probe errors happened due to the '-' character being present in >> the name of the power supply devices: bq27200-0 and rx51-battery. >> >> Since commit 74d3b6419772 ("hwmon: Relax name attribute validation >> for new APIs") hwmon will no longer treat these names as errors, >> allowing the transition for hwmon_device_register_with_info() to >> happen in a safely manner. >> >> Cc: Pavel Machek <pavel@ucw.cz> >> Cc: Guenter Roeck <linux@roeck-us.net> >> Signed-off-by: Fabio Estevam <fabio.estevam@nxp.com> >> --- >> Pavel, >> >> Could you please test this patch on your N900 platform? >> >> Thanks >> >> drivers/thermal/thermal_hwmon.c | 20 +++----------------- >> 1 file changed, 3 insertions(+), 17 deletions(-) >> >> diff --git a/drivers/thermal/thermal_hwmon.c >> b/drivers/thermal/thermal_hwmon.c >> index 541af59..c4a508a 100644 >> --- a/drivers/thermal/thermal_hwmon.c >> +++ b/drivers/thermal/thermal_hwmon.c >> @@ -59,14 +59,6 @@ static LIST_HEAD(thermal_hwmon_list); >> static DEFINE_MUTEX(thermal_hwmon_list_lock); >> >> static ssize_t >> -name_show(struct device *dev, struct device_attribute *attr, char >> *buf) >> -{ >> - struct thermal_hwmon_device *hwmon = dev_get_drvdata(dev); >> - return sprintf(buf, "%s\n", hwmon->type); >> -} >> -static DEVICE_ATTR_RO(name); >> - >> -static ssize_t >> temp_input_show(struct device *dev, struct device_attribute *attr, >> char *buf) >> { >> int temperature; >> @@ -165,15 +157,12 @@ int thermal_add_hwmon_sysfs(struct >> thermal_zone_device *tz) >> >> INIT_LIST_HEAD(&hwmon->tz_list); >> strlcpy(hwmon->type, tz->type, THERMAL_NAME_LENGTH); >> - hwmon->device = hwmon_device_register(NULL); >> + hwmon->device = hwmon_device_register_with_info(NULL, hwmon- >>> type, >> + hwmon, NULL, >> NULL); >> if (IS_ERR(hwmon->device)) { >> result = PTR_ERR(hwmon->device); >> goto free_mem; >> } >> - dev_set_drvdata(hwmon->device, hwmon); >> - result = device_create_file(hwmon->device, &dev_attr_name); >> - if (result) >> - goto free_mem; >> >> register_sys_interface: >> temp = kzalloc(sizeof(*temp), GFP_KERNEL); >> @@ -222,10 +211,8 @@ int thermal_add_hwmon_sysfs(struct >> thermal_zone_device *tz) >> free_temp_mem: >> kfree(temp); >> unregister_name: >> - if (new_hwmon_device) { >> - device_remove_file(hwmon->device, &dev_attr_name); >> + if (new_hwmon_device) >> hwmon_device_unregister(hwmon->device); >> - } >> free_mem: >> if (new_hwmon_device) >> kfree(hwmon); >> @@ -267,7 +254,6 @@ void thermal_remove_hwmon_sysfs(struct >> thermal_zone_device *tz) >> list_del(&hwmon->node); >> mutex_unlock(&thermal_hwmon_list_lock); >> >> - device_remove_file(hwmon->device, &dev_attr_name); >> hwmon_device_unregister(hwmon->device); >> kfree(hwmon); >> } >
On Sat, Jan 13, 2018 at 7:17 PM, Guenter Roeck <linux@roeck-us.net> wrote:
> I bet it gets confused by the quotes.
In this particular case I think we should ignore checkpatch reported error.
Thanks
diff --git a/drivers/thermal/thermal_hwmon.c b/drivers/thermal/thermal_hwmon.c index 541af59..c4a508a 100644 --- a/drivers/thermal/thermal_hwmon.c +++ b/drivers/thermal/thermal_hwmon.c @@ -59,14 +59,6 @@ static LIST_HEAD(thermal_hwmon_list); static DEFINE_MUTEX(thermal_hwmon_list_lock); static ssize_t -name_show(struct device *dev, struct device_attribute *attr, char *buf) -{ - struct thermal_hwmon_device *hwmon = dev_get_drvdata(dev); - return sprintf(buf, "%s\n", hwmon->type); -} -static DEVICE_ATTR_RO(name); - -static ssize_t temp_input_show(struct device *dev, struct device_attribute *attr, char *buf) { int temperature; @@ -165,15 +157,12 @@ int thermal_add_hwmon_sysfs(struct thermal_zone_device *tz) INIT_LIST_HEAD(&hwmon->tz_list); strlcpy(hwmon->type, tz->type, THERMAL_NAME_LENGTH); - hwmon->device = hwmon_device_register(NULL); + hwmon->device = hwmon_device_register_with_info(NULL, hwmon->type, + hwmon, NULL, NULL); if (IS_ERR(hwmon->device)) { result = PTR_ERR(hwmon->device); goto free_mem; } - dev_set_drvdata(hwmon->device, hwmon); - result = device_create_file(hwmon->device, &dev_attr_name); - if (result) - goto free_mem; register_sys_interface: temp = kzalloc(sizeof(*temp), GFP_KERNEL); @@ -222,10 +211,8 @@ int thermal_add_hwmon_sysfs(struct thermal_zone_device *tz) free_temp_mem: kfree(temp); unregister_name: - if (new_hwmon_device) { - device_remove_file(hwmon->device, &dev_attr_name); + if (new_hwmon_device) hwmon_device_unregister(hwmon->device); - } free_mem: if (new_hwmon_device) kfree(hwmon); @@ -267,7 +254,6 @@ void thermal_remove_hwmon_sysfs(struct thermal_zone_device *tz) list_del(&hwmon->node); mutex_unlock(&thermal_hwmon_list_lock); - device_remove_file(hwmon->device, &dev_attr_name); hwmon_device_unregister(hwmon->device); kfree(hwmon); }