| Message ID | 20230818075600.24277-2-biju.das.jz@bp.renesas.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | OF/ACPI/ID Match table improvements for ak8975 driver | expand |
On Fri, Aug 18, 2023 at 08:55:56AM +0100, Biju Das wrote: > 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. > static const struct acpi_device_id ak_acpi_match[] = { > + {"AK8975", (kernel_ulong_t)&ak_def_array[AK8975] }, > + {"AK8963", (kernel_ulong_t)&ak_def_array[AK8963] }, > + {"INVN6500", (kernel_ulong_t)&ak_def_array[AK8963] }, > + {"AK009911", (kernel_ulong_t)&ak_def_array[AK09911] }, > + {"AK09911", (kernel_ulong_t)&ak_def_array[AK09911] }, > + {"AKM9911", (kernel_ulong_t)&ak_def_array[AK09911] }, > + {"AK09912", (kernel_ulong_t)&ak_def_array[AK09912] }, Please, sort them by HID. Haven't you read my comments against v1? > { } > }; ... > - {"AK8963", AK8963}, > + {"AK8963", (kernel_ulong_t)&ak_def_array[AK8963] }, This seems to be a wrong ID (we never use capitalized version, at least last 10 years or so). Better to have a precursor patch that removes it for good. There is no in-kernel user of this either. ... > static const struct of_device_id ak8975_of_match[] = { > - { .compatible = "asahi-kasei,ak8975", }, > - { .compatible = "ak8975", }, > - { .compatible = "asahi-kasei,ak8963", }, > - { .compatible = "ak8963", }, > - { .compatible = "asahi-kasei,ak09911", }, > - { .compatible = "ak09911", }, > - { .compatible = "asahi-kasei,ak09912", }, > - { .compatible = "ak09912", }, > - { .compatible = "asahi-kasei,ak09916", }, > - { .compatible = "ak09916", }, > + { .compatible = "asahi-kasei,ak8975", .data = &ak_def_array[AK8975] }, > + { .compatible = "ak8975", .data = &ak_def_array[AK8975] }, > + { .compatible = "asahi-kasei,ak8963", .data = &ak_def_array[AK8963] }, > + { .compatible = "ak8963", .data = &ak_def_array[AK8963] }, > + { .compatible = "asahi-kasei,ak09911", .data = &ak_def_array[AK09911] }, > + { .compatible = "ak09911", .data = &ak_def_array[AK09911] }, > + { .compatible = "asahi-kasei,ak09912", .data = &ak_def_array[AK09912] }, > + { .compatible = "ak09912", .data = &ak_def_array[AK09912] }, > + { .compatible = "asahi-kasei,ak09916", .data = &ak_def_array[AK09916] }, > + { .compatible = "ak09916", .data = &ak_def_array[AK09916] }, > {} So, why have you ignored my comments against v1?
On Fri, Aug 18, 2023 at 02:25:32PM +0300, Andy Shevchenko wrote: > On Fri, Aug 18, 2023 at 08:55:56AM +0100, Biju Das wrote: ... > So, why have you ignored my comments against v1? Ah, I see now, it has not been ignored, sorry, the implementation in the following patches...
On Fri, Aug 18, 2023 at 08:55:56AM +0100, Biju Das wrote: > 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. As this is a straightforward conversion, nothing should be broken, Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Hi, 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> > --- > v1->v2: > * No change > --- > drivers/iio/magnetometer/ak8975.c | 75 +++++++++++++---------------- > -- > 1 file changed, 30 insertions(+), 45 deletions(-) > > diff --git a/drivers/iio/magnetometer/ak8975.c > b/drivers/iio/magnetometer/ak8975.c > index eb706d0bf70b..104798549de1 100644 > --- a/drivers/iio/magnetometer/ak8975.c > +++ b/drivers/iio/magnetometer/ak8975.c > @@ -813,13 +813,13 @@ static const struct iio_info ak8975_info = { > }; > > static const struct acpi_device_id ak_acpi_match[] = { > - {"AK8975", AK8975}, > - {"AK8963", AK8963}, > - {"INVN6500", AK8963}, > - {"AK009911", AK09911}, > - {"AK09911", AK09911}, > - {"AKM9911", AK09911}, > - {"AK09912", AK09912}, > + {"AK8975", (kernel_ulong_t)&ak_def_array[AK8975] }, > + {"AK8963", (kernel_ulong_t)&ak_def_array[AK8963] }, > + {"INVN6500", (kernel_ulong_t)&ak_def_array[AK8963] }, > + {"AK009911", (kernel_ulong_t)&ak_def_array[AK09911] }, > + {"AK09911", (kernel_ulong_t)&ak_def_array[AK09911] }, > + {"AKM9911", (kernel_ulong_t)&ak_def_array[AK09911] }, > + {"AK09912", (kernel_ulong_t)&ak_def_array[AK09912] }, > { } > }; > MODULE_DEVICE_TABLE(acpi, ak_acpi_match); > @@ -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]. 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 > /* Fetch the regulators */ > data->vdd = devm_regulator_get(&client->dev, "vdd"); > @@ -1077,28 +1062,28 @@ static > DEFINE_RUNTIME_DEV_PM_OPS(ak8975_dev_pm_ops, ak8975_runtime_suspend, > ak8975_runtime_resume, NULL); > > static const struct i2c_device_id ak8975_id[] = { > - {"ak8975", AK8975}, > - {"ak8963", AK8963}, > - {"AK8963", AK8963}, > - {"ak09911", AK09911}, > - {"ak09912", AK09912}, > - {"ak09916", AK09916}, > + {"ak8975", (kernel_ulong_t)&ak_def_array[AK8975] }, > + {"ak8963", (kernel_ulong_t)&ak_def_array[AK8963] }, > + {"AK8963", (kernel_ulong_t)&ak_def_array[AK8963] }, > + {"ak09911", (kernel_ulong_t)&ak_def_array[AK09911] }, > + {"ak09912", (kernel_ulong_t)&ak_def_array[AK09912] }, > + {"ak09916", (kernel_ulong_t)&ak_def_array[AK09916] }, > {} > }; > > MODULE_DEVICE_TABLE(i2c, ak8975_id); > > static const struct of_device_id ak8975_of_match[] = { > - { .compatible = "asahi-kasei,ak8975", }, > - { .compatible = "ak8975", }, > - { .compatible = "asahi-kasei,ak8963", }, > - { .compatible = "ak8963", }, > - { .compatible = "asahi-kasei,ak09911", }, > - { .compatible = "ak09911", }, > - { .compatible = "asahi-kasei,ak09912", }, > - { .compatible = "ak09912", }, > - { .compatible = "asahi-kasei,ak09916", }, > - { .compatible = "ak09916", }, > + { .compatible = "asahi-kasei,ak8975", .data = > &ak_def_array[AK8975] }, > + { .compatible = "ak8975", .data = &ak_def_array[AK8975] }, > + { .compatible = "asahi-kasei,ak8963", .data = > &ak_def_array[AK8963] }, > + { .compatible = "ak8963", .data = &ak_def_array[AK8963] }, > + { .compatible = "asahi-kasei,ak09911", .data = > &ak_def_array[AK09911] }, > + { .compatible = "ak09911", .data = &ak_def_array[AK09911] }, > + { .compatible = "asahi-kasei,ak09912", .data = > &ak_def_array[AK09912] }, > + { .compatible = "ak09912", .data = &ak_def_array[AK09912] }, > + { .compatible = "asahi-kasei,ak09916", .data = > &ak_def_array[AK09916] }, > + { .compatible = "ak09916", .data = &ak_def_array[AK09916] }, > {} > }; > MODULE_DEVICE_TABLE(of, ak8975_of_match);
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. > > 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
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 >
Hi Jonathan, On Wed, Oct 18, 2023 at 9:45 PM Jonathan Cameron <jic23@kernel.org> wrote: > On Wed, 18 Oct 2023 09:04:44 +0200 > Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > 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). Oops, I had missed this is used (only) for the iio_dev, and thus cannot cause duplicates in sysfs, I assume. So please ignore my comment. Gr{oetje,eeting}s, Geert
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@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... > 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. André [1] https://gitlab.com/MartijnBraam/hwtest > 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 > > >
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@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... > > > 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. Cheers, Biju
> 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@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... > > > > > 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
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.
> 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; Cheers, Biju
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 >
On Thu, Oct 19, 2023 at 12:17:22PM +0100, Jonathan Cameron wrote: > 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)) Taking the Jonathan's comment below into consideration we can do _something_ like above, but using only a single API call instead of this ugly and monstrous condition. > > 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!
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); but I don't know whether this is applicable here. Thanks to all, for trying to find a solution. André [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/iio/imu/bmi160/bmi160_i2c.c?h=v6.6-rc6#n31 > > 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; > > Cheers, > Biju >
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?? Cheers, Biju
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
diff --git a/drivers/iio/magnetometer/ak8975.c b/drivers/iio/magnetometer/ak8975.c index eb706d0bf70b..104798549de1 100644 --- a/drivers/iio/magnetometer/ak8975.c +++ b/drivers/iio/magnetometer/ak8975.c @@ -813,13 +813,13 @@ static const struct iio_info ak8975_info = { }; static const struct acpi_device_id ak_acpi_match[] = { - {"AK8975", AK8975}, - {"AK8963", AK8963}, - {"INVN6500", AK8963}, - {"AK009911", AK09911}, - {"AK09911", AK09911}, - {"AKM9911", AK09911}, - {"AK09912", AK09912}, + {"AK8975", (kernel_ulong_t)&ak_def_array[AK8975] }, + {"AK8963", (kernel_ulong_t)&ak_def_array[AK8963] }, + {"INVN6500", (kernel_ulong_t)&ak_def_array[AK8963] }, + {"AK009911", (kernel_ulong_t)&ak_def_array[AK09911] }, + {"AK09911", (kernel_ulong_t)&ak_def_array[AK09911] }, + {"AKM9911", (kernel_ulong_t)&ak_def_array[AK09911] }, + {"AK09912", (kernel_ulong_t)&ak_def_array[AK09912] }, { } }; MODULE_DEVICE_TABLE(acpi, ak_acpi_match); @@ -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; /* Fetch the regulators */ data->vdd = devm_regulator_get(&client->dev, "vdd"); @@ -1077,28 +1062,28 @@ static DEFINE_RUNTIME_DEV_PM_OPS(ak8975_dev_pm_ops, ak8975_runtime_suspend, ak8975_runtime_resume, NULL); static const struct i2c_device_id ak8975_id[] = { - {"ak8975", AK8975}, - {"ak8963", AK8963}, - {"AK8963", AK8963}, - {"ak09911", AK09911}, - {"ak09912", AK09912}, - {"ak09916", AK09916}, + {"ak8975", (kernel_ulong_t)&ak_def_array[AK8975] }, + {"ak8963", (kernel_ulong_t)&ak_def_array[AK8963] }, + {"AK8963", (kernel_ulong_t)&ak_def_array[AK8963] }, + {"ak09911", (kernel_ulong_t)&ak_def_array[AK09911] }, + {"ak09912", (kernel_ulong_t)&ak_def_array[AK09912] }, + {"ak09916", (kernel_ulong_t)&ak_def_array[AK09916] }, {} }; MODULE_DEVICE_TABLE(i2c, ak8975_id); static const struct of_device_id ak8975_of_match[] = { - { .compatible = "asahi-kasei,ak8975", }, - { .compatible = "ak8975", }, - { .compatible = "asahi-kasei,ak8963", }, - { .compatible = "ak8963", }, - { .compatible = "asahi-kasei,ak09911", }, - { .compatible = "ak09911", }, - { .compatible = "asahi-kasei,ak09912", }, - { .compatible = "ak09912", }, - { .compatible = "asahi-kasei,ak09916", }, - { .compatible = "ak09916", }, + { .compatible = "asahi-kasei,ak8975", .data = &ak_def_array[AK8975] }, + { .compatible = "ak8975", .data = &ak_def_array[AK8975] }, + { .compatible = "asahi-kasei,ak8963", .data = &ak_def_array[AK8963] }, + { .compatible = "ak8963", .data = &ak_def_array[AK8963] }, + { .compatible = "asahi-kasei,ak09911", .data = &ak_def_array[AK09911] }, + { .compatible = "ak09911", .data = &ak_def_array[AK09911] }, + { .compatible = "asahi-kasei,ak09912", .data = &ak_def_array[AK09912] }, + { .compatible = "ak09912", .data = &ak_def_array[AK09912] }, + { .compatible = "asahi-kasei,ak09916", .data = &ak_def_array[AK09916] }, + { .compatible = "ak09916", .data = &ak_def_array[AK09916] }, {} }; MODULE_DEVICE_TABLE(of, ak8975_of_match);
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> --- v1->v2: * No change --- drivers/iio/magnetometer/ak8975.c | 75 +++++++++++++------------------ 1 file changed, 30 insertions(+), 45 deletions(-)