Re: [PATCH v2 1/5] iio: magnetometer: ak8975: Convert enum->pointer for data in the match tables

From: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
To: Biju Das <biju.das.jz@bp.renesas.com>
Cc: "André Apitzsch" <git@apitzsch.eu>,
	"Andy Shevchenko" <andriy.shevchenko@linux.intel.com>,
	"Jonathan Cameron" <jic23@kernel.org>,
	"Geert Uytterhoeven" <geert@linux-m68k.org>,
	"Lars-Peter Clausen" <lars@metafoo.de>,
	"Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>,
	"linux-iio@vger.kernel.org" <linux-iio@vger.kernel.org>,
	"Geert Uytterhoeven" <geert+renesas@glider.be>,
	"Prabhakar Mahadev Lad" <prabhakar.mahadev-lad.rj@bp.renesas.com>,
	"linux-renesas-soc@vger.kernel.org"
	<linux-renesas-soc@vger.kernel.org>
Subject: Re: [PATCH v2 1/5] iio: magnetometer: ak8975: Convert enum->pointer for data in the match tables
Date: Fri, 20 Oct 2023 16:10:04 +0100	[thread overview]
Message-ID: <20231020161004.00001fd5@Huawei.com> (raw)
In-Reply-To: <TYCPR01MB11269399FE198E67DAD1CA6F986DBA@TYCPR01MB11269.jpnprd01.prod.outlook.com>

On Fri, 20 Oct 2023 07:39:38 +0000
Biju Das <biju.das.jz@bp.renesas.com> wrote:

> Hi Andre,
> 
> > Subject: Re: [PATCH v2 1/5] iio: magnetometer: ak8975: Convert enum-  
> > >pointer for data in the match tables  
> > 
> > Am Donnerstag, dem 19.10.2023 um 09:41 +0000 schrieb Biju Das:  
> > > > Subject: Re: [PATCH v2 1/5] iio: magnetometer: ak8975: Convert
> > > > enum-  
> > > > > pointer for data in the match tables  
> > > >
> > > > On Thu, Oct 19, 2023 at 07:08:23AM +0000, Biju Das wrote:  
> > > > > > Subject: RE: [PATCH v2 1/5] iio: magnetometer: ak8975: Convert
> > > > > > enum-  
> > > >
> > > > ...
> > > >  
> > > > > > As mentioned in the patch.
> > > > > > /* If enumerated via firmware node, fix the ABI */
> > > > > >
> > > > > > Looks like this issue is not introduced by this patch.
> > > > > > The previous code uses device_get_match_data() which returns a
> > > > > > match as it uses DT node and it uses dev_name(&client->dev)
> > > > > > instead of
> > > > > > id->name;
> > > > > >
> > > > > > Am I missing anything here? If it is just a test program, can it
> > > > > > be  
> > > > fixed??  
> > > > > >
> > > > > > Please correct me if I am wrong.  
> > > > >
> > > > > I just realized that there is no .data in previous code for OF
> > > > > tables.
> > > > >
> > > > > Maybe we should add a check, if it is DT node, return id->name?
> > > > >
> > > > > Is there any API to distinguish DT node from ACPI??  
> > > >
> > > > Of course, but I discourage people to use that, you have to have a
> > > > very good justification why you need it (and this case doesn't sound
> > > > good enough to me, or please elaborate). Hence I leave it as a
> > > > homework to find those APIs.  
> > >
> > > Andre, complained that his test app is broken with this patch. I am
> > > waiting for his response whether he can fix his test app?
> > > If not, we need to find a solution. One solution is adding a name
> > > variable and use consistent name across OF/ACPI/I2C tables for various
> > > devices.  
> > 
> > Just to make it clear, the functionality of the test application
> > (hwtest) is not affected by this patch. Only a less/none telling name is
> > shown now in the Model column of its output.
> > 
> > I'm not that familiar with the interfaces provided by the kernel. Is there
> > another way to get the device name if not from for example
> > 
> > /sys/bus/iio/devices/iio:device2/name
> > 
> > (which now shows '0-000d' instead of 'ak09911')
> > 
> > For the bmi160 device[1] the following code is used to get the name
> > 
> > 	if (id)
> > 		name = id->name;
> > 	else
> > 		name = dev_name(&client->dev);  
> 
> This looks good, but what about introducing a new api to get device names for
> fwnodes. (strip vendor from compatible for OF and use name from ACPI id table)??
> So that driver don't depend upon I2C ID to get device names for fw nodes??

ACPI Ids are normally irrelevant stuff we should not have userspace care about.
The correctly assigned ones have to be assigned by a vendor with appropriate
base ID. That isn't necessarily anything to do with the hardware or even
the same vendor. If I want to provide ACPI support for a device and
the device vendor either doesn't have an ACPI manufacturer ID or is ignoring
me I can (after some internal proceses) get a hisilicon one as HISIXXXX
for example.

That lack of meaning (unlike compatibles) is why we normally push the name into
a structure in the driver that is then looked up - hence giving us a meaningful
part number.

Note this is for correctly assigned ACPI ids. There are lots of device vendors
who do it wrong and ship products where the name matches the part number
despite that not being a registered ACPI vendor ID. Where we have contacts
we moan at them and try and get this fixed in updated firmware, where we
don't we sometime add the ID to the kernel tables.
I'll add that this mess is typically from consumer device manufacturers.
If I tried that garbage in a server, there is no way I'd get it upstream,
it would be considered a firmware bug that we'd have to get our firmware
team to fix.

Jonathan

> 
> Cheers,
> Biju