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

[Jonathan Cameron <jic23@kernel.org>]

On Tue, 11 Aug 2026 13:27:17 +0300
Andy Shevchenko <andriy.shevchenko@intel.com> wrote:

> 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.
> 

Hmm. I agree with fixing the capitals. However we have more options than
usual because we have a single part with which all supported are backwards
compatible.

For most / maybe all of the other drivers where similar has come up,
 there is no right answer to what data to use (so we break this path).
They have two or more parts supported but they are mutually incompatible.
Here we could at least have provided the subset for now. I would
be careful though as a future part addition might mean we had to drop
that default as it wasn't backwards compatible with it.

Hence it would be a bit of a gamble on whether failing probe on lower
case inputs is enough of a problem to paper over now, given we may
have to break it in the future anyway.

Jonathan