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

From: Jean Delvare <jdelvare@suse.de>
To: Sascha Hauer <s.hauer@pengutronix.de>
Cc: linux-hwmon@vger.kernel.org, Guenter Roeck <linux@roeck-us.net>
Subject: Re: [PATCH] hwmon: (jc42) Fix name to have no illegal characters
Date: Fri, 17 Apr 2020 15:47:23 +0200	[thread overview]
Message-ID: <20200417154723.09bad8d9@endymion> (raw)
In-Reply-To: <20200417103255.GZ1694@pengutronix.de>

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