Re: [PATCH 2/3] iio: light: stk3310: add per-chip match data

[Andy Shevchenko <andriy.shevchenko@intel.com>]

On Mon, Aug 10, 2026 at 11:51:57PM +0200, Jorijn van der Graaf wrote:
> On Mon, Aug 10, 2026 at 09:34:42PM +0300, Andy Shevchenko wrote:
> > > +	chip_info = i2c_get_match_data(client);
> > > +	if (!chip_info) {
> > > +		/*
> > > +		 * Clients instantiated through the sysfs new_device
> > > +		 * interface under the lowercase compatible-derived name
> > > +		 * have no firmware node and do not match the uppercase
> > > +		 * id table entries.
> > > +		 */
> > > +		chip_info = &stk3310_chip_info;
> >
> > This is an interesting comment and approach.
> >
> > > +	}
> >
> > - Where does this lowercase come from? Is it Linux forced conversion?
> 
> No conversion - the client name is whatever is echoed into the sysfs
> new_device file, and the lowercase spelling is the one that binds via
> the OF table: for a client with no firmware node,
> i2c_of_match_device() falls through to i2c_of_match_device_sysfs(),
> which matches the client name against each compatible and its part
> after the vendor prefix, so "stk3310" binds through
> "sensortek,stk3310" - the same lowercase name a DT client gets from
> of_alias_from_compatible(). The id table lookup stays case-sensitive,
> which is why i2c_get_match_data() comes back NULL for such a client:
> device_get_match_data() needs the firmware node it does not have, and
> i2c_match_id() only knows the historic uppercase names.
> 
> > - What's wrong with simply failing the probe?
> 
> It would break a path that works before this patch: probe consumed no
> match data, so "echo stk3310 0x48 > new_device" bound and probed fine
> (we used exactly that during bring-up, before the DT node existed).
> Failing it would be a userspace-visible regression in a patch that
> claims no functional change.
> 
> > As far as I understand the problem is preexisted. Or was there any default
> > taken? How do we know that the chosen default is a good one?
> 
> The path preexists; only this patch makes probe care about match
> data. Before it the driver had no per-chip data at all - every client
> got the same two channels and the hardcoded "stk3310" name however it
> matched. The fallback selects stk3310_chip_info, which is exactly
> that, so a client that probed before this patch behaves identically
> after it. What the default cannot give is the new part's extra
> channels: an STK36C61 instantiated under the lowercase name gets the
> reduced legacy profile, and the id table spelling "STK36C61" selects
> the full one.

The problem preexists. I just looked into the Git history of this rather broken
table IDs and what I have to say and state is the following:

- the driver initially was written for ACPI and the author blindly (cargo cult?)
  used the same IDs as for ACPI _HID, while they have different rules

- there is no existing in kernel driver that supports enumeration based
on I2C ID table

With that being said, I think we may just lower the case for the I²C ID
table and fail enumeration when there is no chip_info provided.
Yes, it will break very weird use case with capital letters via sysfs,
but TBH it's a niche and people should know that most of the devices
are enumerated as part number of OF compatible in that case.

Also note that the DT users should also not use sysfs, and instead call
DT overlay.

I can take all the blame on very unlikely event of a breakage of somebody's
setup in the future. TL;DR: just add a first patch to lower case the IDs
with Suggested-by my name and I will give a Rb tag with full conscience and
responsibility.

-- 
With Best Regards,
Andy Shevchenko