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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



> Subject: RE: [PATCH v2 1/5] iio: magnetometer: ak8975: Convert enum-
> >pointer for data in the match tables
> 
> Hi Andre,
> 
> > Subject: Re: [PATCH v2 1/5] iio: magnetometer: ak8975: Convert enum-
> > >pointer for data in the match tables
> >
> > Am Mittwoch, dem 18.10.2023 um 20:45 +0100 schrieb Jonathan Cameron:
> > > On Wed, 18 Oct 2023 09:04:44 +0200
> > > Geert Uytterhoeven <geert@xxxxxxxxxxxxxx> wrote:
> > >
> > > > Hi André,
> > > >
> > > > On Tue, Oct 17, 2023 at 11:12 PM André Apitzsch <git@xxxxxxxxxxx>
> > > > 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@xxxxxxxxxxxxxx>
> > > >
> > > > > > --- 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...
> > >
> > At least for the longcheer l9100 it is a new bug that was introduced
> > by this patch. But as my only use for this name is via hwtest[1],
> > which uses the name as "pretty model name", it's fine with me if it
> cannot be fixed.
> 
> 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??

Cheers,
Biju




[Index of Archives]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Input]     [Linux Kernel]     [Linux SCSI]     [X.org]

  Powered by Linux