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

From: Jonathan Cameron <jic23@kernel.org>
To: Geert Uytterhoeven <geert@linux-m68k.org>
Cc: git@apitzsch.eu, "Biju Das" <biju.das.jz@bp.renesas.com>,
	"Lars-Peter Clausen" <lars@metafoo.de>,
	"Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>,
	"Andy Shevchenko" <andriy.shevchenko@linux.intel.com>,
	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
Subject: Re: [PATCH v2 1/5] iio: magnetometer: ak8975: Convert enum->pointer for data in the match tables
Date: Wed, 18 Oct 2023 20:45:33 +0100	[thread overview]
Message-ID: <20231018204533.39399b0b@jic23-huawei> (raw)
In-Reply-To: <CAMuHMdUTxQym7+vYPtnvMQeH8GKSk0iOyuMnLkgr3LH1E8TTYA@mail.gmail.com>

On Wed, 18 Oct 2023 09:04:44 +0200
Geert Uytterhoeven <geert@linux-m68k.org> wrote:

> Hi André,
> 
> On Tue, Oct 17, 2023 at 11:12 PM André Apitzsch <git@apitzsch.eu> wrote:
> > Am Freitag, dem 18.08.2023 um 08:55 +0100 schrieb Biju Das:  
> > > Convert enum->pointer for data in the match tables to simplify the
> > > probe()
> > > by replacing device_get_match_data() and i2c_client_get_device_id by
> > > i2c_get_match_data() as we have similar I2C, ACPI and DT matching
> > > table.
> > >
> > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>  
> 
> > > --- a/drivers/iio/magnetometer/ak8975.c
> > > +++ b/drivers/iio/magnetometer/ak8975.c
> > > @@ -883,10 +883,7 @@ static int ak8975_probe(struct i2c_client
> > > *client)
> > >       struct iio_dev *indio_dev;
> > >       struct gpio_desc *eoc_gpiod;
> > >       struct gpio_desc *reset_gpiod;
> > > -     const void *match;
> > > -     unsigned int i;
> > >       int err;
> > > -     enum asahi_compass_chipset chipset;
> > >       const char *name = NULL;
> > >
> > >       /*
> > > @@ -928,27 +925,15 @@ static int ak8975_probe(struct i2c_client
> > > *client)
> > >               return err;
> > >
> > >       /* id will be NULL when enumerated via ACPI */
> > > -     match = device_get_match_data(&client->dev);
> > > -     if (match) {
> > > -             chipset = (uintptr_t)match;
> > > -             name = dev_name(&client->dev);
> > > -     } else if (id) {
> > > -             chipset = (enum asahi_compass_chipset)(id-  
> > > >driver_data);  
> > > -             name = id->name;
> > > -     } else
> > > -             return -ENOSYS;
> > > -
> > > -     for (i = 0; i < ARRAY_SIZE(ak_def_array); i++)
> > > -             if (ak_def_array[i].type == chipset)
> > > -                     break;
> > > -
> > > -     if (i == ARRAY_SIZE(ak_def_array)) {
> > > -             dev_err(&client->dev, "AKM device type unsupported:
> > > %d\n",
> > > -                     chipset);
> > > +     data->def = i2c_get_match_data(client);
> > > +     if (!data->def)
> > >               return -ENODEV;
> > > -     }
> > >
> > > -     data->def = &ak_def_array[i];
> > > +     /* If enumerated via firmware node, fix the ABI */
> > > +     if (dev_fwnode(&client->dev))
> > > +             name = dev_name(&client->dev);
> > > +     else
> > > +             name = id->name;
> > >  
> >
> > I just noticed, that with the above change '0-000d' instead of the
> > previous and expected 'ak09911' is shown now as name for the
> > magnetometer in longcheer l9100 [1].  
> 
> While this doesn't help much, note that the old name would break
> the case of having two instances of the same device.

Why? In IIO ABI, this is the part number - it's absolutely fine to have
two device with same name. There are lots of other ways of figuring out
which is which (parent device being the easiest).

This is indeed a bug but it isn't a new one and it's been there long
enough that there may be userspace code relying on it...  

There are some of these from a time when I was being unobservant in
catching them in review (this one was approx 2014 I think)
We've never fixed them because of possibility of breaking usersapce.

> 
> >
> > id->name contains the expected string ('ak09911'), but because of
> > dev_fwnode(&client->dev) being true, it is not used.
> >
> > André
> >
> > [1] https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/arch/arm64/boot/dts/qcom/msm8939-longcheer-l9100.dts?h=next-20231017#n127  
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 