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
