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: "Andy Shevchenko" <andriy.shevchenko@linux.intel.com>,
	"André Apitzsch" <git@apitzsch.eu>,
	"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: Thu, 19 Oct 2023 12:17:22 +0100	[thread overview]
Message-ID: <20231019121722.00007e6e@Huawei.com> (raw)
In-Reply-To: <TYCPR01MB11269269A3237CC119644F4D286D4A@TYCPR01MB11269.jpnprd01.prod.outlook.com>

On Thu, 19 Oct 2023 09:41:06 +0000
Biju Das <biju.das.jz@bp.renesas.com> wrote:

> > 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.
> 
> Other solution is just add this check,
> 
> if (dev_fwnode(&client->dev) && !(IS_ENABLED(CONFIG_OF) && dev->of_node))
> 	name = dev_name(&client->dev);
> else
> 	name = id->name;

Given this is a userspace regression (caused by accidental "fix" - I missed
the fact it had this impact :(), I think it is valid to special case the ACPI in this rare
case but definitely needs a big fat comment saying why we are doing it and that it
should not be copied into other drivers!!!

If we can get away with fixing the original (many years old ABI misuse - but IIRC from a time
where our ABI docs were lacking) then I'm keen on doing so, but I doubt we can.
Definitely don't want to accidentally spread that bug though to new cases!

Jonathan

> 
> Cheers,
> Biju
> 