[PATCH] hwmon: (jc42) Fix name to have no illegal characters

[PATCH] hwmon: (jc42) Fix name to have no illegal characters

[Sascha Hauer]

The jc42 driver passes I2C client's name as hwmon device name. In case
of device tree probed devices this ends up being part of the compatible
string, "jc-42.4-temp". This name contains hyphens and the hwmon core
doesn't like this:

jc42 2-0018: hwmon: 'jc-42.4-temp' is not a valid name attribute, please fix

This changes the name to "jc42" which doesn't have any illegal
characters.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/hwmon/jc42.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/hwmon/jc42.c b/drivers/hwmon/jc42.c
index f2d81b0558e5..e3f1ebee7130 100644
--- a/drivers/hwmon/jc42.c
+++ b/drivers/hwmon/jc42.c
@@ -506,7 +506,7 @@ static int jc42_probe(struct i2c_client *client, const struct i2c_device_id *id)
 	}
 	data->config = config;
 
-	hwmon_dev = devm_hwmon_device_register_with_info(dev, client->name,
+	hwmon_dev = devm_hwmon_device_register_with_info(dev, "jc42",
 							 data, &jc42_chip_info,
 							 NULL);
 	return PTR_ERR_OR_ZERO(hwmon_dev);
-- 
2.26.1

Re: [PATCH] hwmon: (jc42) Fix name to have no illegal characters

[Jean Delvare]

Hi Sascha,

On Fri, 17 Apr 2020 11:28:53 +0200, Sascha Hauer wrote:
> The jc42 driver passes I2C client's name as hwmon device name. In case
> of device tree probed devices this ends up being part of the compatible
> string, "jc-42.4-temp". This name contains hyphens and the hwmon core
> doesn't like this:
> 
> jc42 2-0018: hwmon: 'jc-42.4-temp' is not a valid name attribute, please fix
> 
> This changes the name to "jc42" which doesn't have any illegal
> characters.

I don't think "jc-42.4-temp" is a valid i2c client name either. I
believe this should be fixed at the of->i2c level, rather than the
i2c->hwmon level. Not sure how other drivers are dealing with that, it
seems that in most cases the name part of the compatible string matches
exactly the expected client name so no conversion is needed.

Thanks,
-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH] hwmon: (jc42) Fix name to have no illegal characters

[Sascha Hauer]

Hi Jean,

On Fri, Apr 17, 2020 at 11:55:03AM +0200, Jean Delvare wrote:
> Hi Sascha,
> 
> On Fri, 17 Apr 2020 11:28:53 +0200, Sascha Hauer wrote:
> > The jc42 driver passes I2C client's name as hwmon device name. In case
> > of device tree probed devices this ends up being part of the compatible
> > string, "jc-42.4-temp". This name contains hyphens and the hwmon core
> > doesn't like this:
> > 
> > jc42 2-0018: hwmon: 'jc-42.4-temp' is not a valid name attribute, please fix
> > 
> > This changes the name to "jc42" which doesn't have any illegal
> > characters.
> 
> I don't think "jc-42.4-temp" is a valid i2c client name either.

What makes the name invalid then? I am not aware of any constraints of
i2c client names.

> I believe this should be fixed at the of->i2c level, rather than the
> i2c->hwmon level.

Are you suggesting a character conversion from hyphens to underscores or
similar in the i2c core?

> Not sure how other drivers are dealing with that, it
> seems that in most cases the name part of the compatible string matches
> exactly the expected client name so no conversion is needed.

Other i2c hwmon drivers I found do not have any hyphens in their
compatible string, so they are at least not hit by this message.

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH] hwmon: (jc42) Fix name to have no illegal characters

[Jean Delvare]

On Fri, 17 Apr 2020 12:32:55 +0200, Sascha Hauer wrote:
> On Fri, Apr 17, 2020 at 11:55:03AM +0200, Jean Delvare wrote:
> > On Fri, 17 Apr 2020 11:28:53 +0200, Sascha Hauer wrote:  
> > > The jc42 driver passes I2C client's name as hwmon device name. In case
> > > of device tree probed devices this ends up being part of the compatible
> > > string, "jc-42.4-temp". This name contains hyphens and the hwmon core
> > > doesn't like this:
> > > 
> > > jc42 2-0018: hwmon: 'jc-42.4-temp' is not a valid name attribute, please fix
> > > 
> > > This changes the name to "jc42" which doesn't have any illegal
> > > characters.  
> > 
> > I don't think "jc-42.4-temp" is a valid i2c client name either.  
> 
> What makes the name invalid then? I am not aware of any constraints of
> i2c client names.

Historically hwmon devices were i2c devices so libsensors would use
the i2c client name as the device name, and still does as a fallback if
memory serves. Therefore whatever rule applies to hwmon names would
also apply to i2c names (in the context of hwmon devices) even though
this is not enforced.

> > I believe this should be fixed at the of->i2c level, rather than the
> > i2c->hwmon level.  
> 
> Are you suggesting a character conversion from hyphens to underscores or
> similar in the i2c core?

No, my point is that from a userspace perspective it shouldn't matter
if the device comes from the OF tree or not. So the device name should
be the same, i.e. the i2c client should be named "jc42" always. That's
what happens for all other devices I checked, simply because it turns
out that their OF compatible string is in the form
<vendor_name>,<linux_i2c_client_name>, so when you strip the vendor
name you get the right name directly.

My knowledge of the OF subsystem is fairly limited though, so I have no
idea if it is possible to enforce a specific name like that at an early
stage. The proper name can't be guessed by i2c-core, so ideally the
conversion should be provided by the driver itself. I see that struct
of_device_id has a "name" field, can it be used for that purpose? If
not, I suppose the "data" field could be used for that.

> > Not sure how other drivers are dealing with that, it
> > seems that in most cases the name part of the compatible string matches
> > exactly the expected client name so no conversion is needed.  
> 
> Other i2c hwmon drivers I found do not have any hyphens in their
> compatible string, so they are at least not hit by this message.

I drew the same conclusion here, and your patch is definitely better
than nothing as it fixes a real problem, however it is not prefect due
to the reason explained above, plus the fact that it would break if the
driver ever supports more than one device type (say JEDEC releases JC43
tomorrow and we add support... you code forces the name to "jc42" even
if the OF name was something other than "jc-42.4-temp").

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH] hwmon: (jc42) Fix name to have no illegal characters

[Guenter Roeck]

On 4/17/20 6:47 AM, Jean Delvare wrote:
> On Fri, 17 Apr 2020 12:32:55 +0200, Sascha Hauer wrote:
>> On Fri, Apr 17, 2020 at 11:55:03AM +0200, Jean Delvare wrote:
>>> On Fri, 17 Apr 2020 11:28:53 +0200, Sascha Hauer wrote:  
>>>> The jc42 driver passes I2C client's name as hwmon device name. In case
>>>> of device tree probed devices this ends up being part of the compatible
>>>> string, "jc-42.4-temp". This name contains hyphens and the hwmon core
>>>> doesn't like this:
>>>>
>>>> jc42 2-0018: hwmon: 'jc-42.4-temp' is not a valid name attribute, please fix
>>>>
>>>> This changes the name to "jc42" which doesn't have any illegal
>>>> characters.  
>>>
>>> I don't think "jc-42.4-temp" is a valid i2c client name either.  
>>
>> What makes the name invalid then? I am not aware of any constraints of
>> i2c client names.
> 
> Historically hwmon devices were i2c devices so libsensors would use
> the i2c client name as the device name, and still does as a fallback if
> memory serves. Therefore whatever rule applies to hwmon names would
> also apply to i2c names (in the context of hwmon devices) even though
> this is not enforced.
> 
>>> I believe this should be fixed at the of->i2c level, rather than the
>>> i2c->hwmon level.  
>>
>> Are you suggesting a character conversion from hyphens to underscores or
>> similar in the i2c core?
> 
> No, my point is that from a userspace perspective it shouldn't matter
> if the device comes from the OF tree or not. So the device name should
> be the same, i.e. the i2c client should be named "jc42" always. That's
> what happens for all other devices I checked, simply because it turns
> out that their OF compatible string is in the form
> <vendor_name>,<linux_i2c_client_name>, so when you strip the vendor
> name you get the right name directly.
> 
> My knowledge of the OF subsystem is fairly limited though, so I have no
> idea if it is possible to enforce a specific name like that at an early
> stage. The proper name can't be guessed by i2c-core, so ideally the
> conversion should be provided by the driver itself. I see that struct
> of_device_id has a "name" field, can it be used for that purpose? If
> not, I suppose the "data" field could be used for that.
> 
>>> Not sure how other drivers are dealing with that, it
>>> seems that in most cases the name part of the compatible string matches
>>> exactly the expected client name so no conversion is needed.  
>>
>> Other i2c hwmon drivers I found do not have any hyphens in their
>> compatible string, so they are at least not hit by this message.
> 
> I drew the same conclusion here, and your patch is definitely better
> than nothing as it fixes a real problem, however it is not prefect due
> to the reason explained above, plus the fact that it would break if the
> driver ever supports more than one device type (say JEDEC releases JC43
> tomorrow and we add support... you code forces the name to "jc42" even
> if the OF name was something other than "jc-42.4-temp").
> 

The driver should really list all supported devices (not the standard).
It could/should have a generic property name to match, but I have no idea
what that should be. There are existing compatible properties named
"jedec,ufs-1.1" and similar, so maybe the current compatible string
isn't as bad as I thought.

Still, we need to do something. I am open to suggestions.

Guenter

Re: [PATCH] hwmon: (jc42) Fix name to have no illegal characters

[Guenter Roeck]

On 4/17/20 2:55 AM, Jean Delvare wrote:
> Hi Sascha,
> 
> On Fri, 17 Apr 2020 11:28:53 +0200, Sascha Hauer wrote:
>> The jc42 driver passes I2C client's name as hwmon device name. In case
>> of device tree probed devices this ends up being part of the compatible
>> string, "jc-42.4-temp". This name contains hyphens and the hwmon core
>> doesn't like this:
>>
>> jc42 2-0018: hwmon: 'jc-42.4-temp' is not a valid name attribute, please fix
>>
>> This changes the name to "jc42" which doesn't have any illegal
>> characters.
> 
> I don't think "jc-42.4-temp" is a valid i2c client name either. I
> believe this should be fixed at the of->i2c level, rather than the
> i2c->hwmon level. Not sure how other drivers are dealing with that, it
> seems that in most cases the name part of the compatible string matches
> exactly the expected client name so no conversion is needed.
> 

The problem here is that the compatible string is simply wrong. It
precedes the time when I understood devicetree properties well enough
to understand what I was doing. Oh well.

No idea what to do at this point other than accepting this patch.

Guenter

Re: [PATCH] hwmon: (jc42) Fix name to have no illegal characters

[Guenter Roeck]

On Fri, Apr 17, 2020 at 11:28:53AM +0200, Sascha Hauer wrote:
> The jc42 driver passes I2C client's name as hwmon device name. In case
> of device tree probed devices this ends up being part of the compatible
> string, "jc-42.4-temp". This name contains hyphens and the hwmon core
> doesn't like this:
> 
> jc42 2-0018: hwmon: 'jc-42.4-temp' is not a valid name attribute, please fix
> 
> This changes the name to "jc42" which doesn't have any illegal
> characters.
> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>

I have not heard a better suggestion, so applied.

Thanks,
Guenter

> ---
>  drivers/hwmon/jc42.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/hwmon/jc42.c b/drivers/hwmon/jc42.c
> index f2d81b0558e5..e3f1ebee7130 100644
> --- a/drivers/hwmon/jc42.c
> +++ b/drivers/hwmon/jc42.c
> @@ -506,7 +506,7 @@ static int jc42_probe(struct i2c_client *client, const struct i2c_device_id *id)
>  	}
>  	data->config = config;
>  
> -	hwmon_dev = devm_hwmon_device_register_with_info(dev, client->name,
> +	hwmon_dev = devm_hwmon_device_register_with_info(dev, "jc42",
>  							 data, &jc42_chip_info,
>  							 NULL);
>  	return PTR_ERR_OR_ZERO(hwmon_dev);