[PATCH v2 0/5] OF/ACPI/ID Match table improvements for ak8975 driver

[PATCH v2 0/5] OF/ACPI/ID Match table improvements for ak8975 driver

[Biju Das]

This patch series aims to improve OF/ACPI/ID Match table improvements for
ak8975 driver.
 * Converting enum->pointer for data in the match tables saves code
   meory in probe()
 * Sort OF, ID and ACPI tables for easy maintanance
 * Dropping deprecated enums from bindings and driver for saving memory.

This patch series is only compile tested.

Biju Das (5):
  iio: magnetometer: ak8975: Convert enum->pointer for data in the match
    tables
  iio: magnetometer: ak8975: Sort ID and ACPI tables
  dt-bindings: iio: magnetometer: asahi-kasei,ak8975: Drop deprecated
    enums
  iio: magnetometer: ak8975: Drop deprecated enums from OF table
  iio: magnetometer: ak8975: Sort OF table

 .../iio/magnetometer/asahi-kasei,ak8975.yaml  |  7 --
 drivers/iio/magnetometer/ak8975.c             | 81 +++++++------------
 2 files changed, 30 insertions(+), 58 deletions(-)

-- 
2.25.1

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

[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;
 
 	/* 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);
-- 
2.25.1

[PATCH v2 2/5] iio: magnetometer: ak8975: Sort ID and ACPI tables

[Biju Das]

Sort ID table alphabetically by name and acpi table by HID.

While at it, drop blank line before ID table.

Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
v2:
 * New patch
---
 drivers/iio/magnetometer/ak8975.c | 29 ++++++++++++++---------------
 1 file changed, 14 insertions(+), 15 deletions(-)

diff --git a/drivers/iio/magnetometer/ak8975.c b/drivers/iio/magnetometer/ak8975.c
index 104798549de1..8cfceb007936 100644
--- a/drivers/iio/magnetometer/ak8975.c
+++ b/drivers/iio/magnetometer/ak8975.c
@@ -812,18 +812,6 @@ static const struct iio_info ak8975_info = {
 	.read_raw = &ak8975_read_raw,
 };
 
-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] },
-	{ }
-};
-MODULE_DEVICE_TABLE(acpi, ak_acpi_match);
-
 static void ak8975_fill_buffer(struct iio_dev *indio_dev)
 {
 	struct ak8975_data *data = iio_priv(indio_dev);
@@ -1061,16 +1049,27 @@ static int ak8975_runtime_resume(struct device *dev)
 static DEFINE_RUNTIME_DEV_PM_OPS(ak8975_dev_pm_ops, ak8975_runtime_suspend,
 				 ak8975_runtime_resume, NULL);
 
+static const struct acpi_device_id ak_acpi_match[] = {
+	{"AK8963", (kernel_ulong_t)&ak_def_array[AK8963] },
+	{"AK8975", (kernel_ulong_t)&ak_def_array[AK8975] },
+	{"AK009911", (kernel_ulong_t)&ak_def_array[AK09911] },
+	{"AK09911", (kernel_ulong_t)&ak_def_array[AK09911] },
+	{"AK09912", (kernel_ulong_t)&ak_def_array[AK09912] },
+	{"AKM9911", (kernel_ulong_t)&ak_def_array[AK09911] },
+	{"INVN6500", (kernel_ulong_t)&ak_def_array[AK8963] },
+	{ }
+};
+MODULE_DEVICE_TABLE(acpi, ak_acpi_match);
+
 static const struct i2c_device_id ak8975_id[] = {
-	{"ak8975", (kernel_ulong_t)&ak_def_array[AK8975] },
-	{"ak8963", (kernel_ulong_t)&ak_def_array[AK8963] },
 	{"AK8963", (kernel_ulong_t)&ak_def_array[AK8963] },
+	{"ak8963", (kernel_ulong_t)&ak_def_array[AK8963] },
+	{"ak8975", (kernel_ulong_t)&ak_def_array[AK8975] },
 	{"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[] = {
-- 
2.25.1

[PATCH v2 3/5] dt-bindings: iio: magnetometer: asahi-kasei,ak8975: Drop deprecated enums

[Biju Das]

Drop deprecated enums from bindings as it is been here for a long time.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
v2:
 * New patch
---
 .../bindings/iio/magnetometer/asahi-kasei,ak8975.yaml      | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/Documentation/devicetree/bindings/iio/magnetometer/asahi-kasei,ak8975.yaml b/Documentation/devicetree/bindings/iio/magnetometer/asahi-kasei,ak8975.yaml
index 9790f75fc669..ee77558e9800 100644
--- a/Documentation/devicetree/bindings/iio/magnetometer/asahi-kasei,ak8975.yaml
+++ b/Documentation/devicetree/bindings/iio/magnetometer/asahi-kasei,ak8975.yaml
@@ -18,13 +18,6 @@ properties:
           - asahi-kasei,ak09911
           - asahi-kasei,ak09912
           - asahi-kasei,ak09916
-      - enum:
-          - ak8975
-          - ak8963
-          - ak09911
-          - ak09912
-          - ak09916
-        deprecated: true
 
   reg:
     maxItems: 1
-- 
2.25.1

[PATCH v2 4/5] iio: magnetometer: ak8975: Drop deprecated enums from OF table

[Biju Das]

Drop deprecated enums from OF table as corresponding entries
are removed from bindings and it also saves memory.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
v2:
 * New patch
---
 drivers/iio/magnetometer/ak8975.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/iio/magnetometer/ak8975.c b/drivers/iio/magnetometer/ak8975.c
index 8cfceb007936..295b7be5e36d 100644
--- a/drivers/iio/magnetometer/ak8975.c
+++ b/drivers/iio/magnetometer/ak8975.c
@@ -1074,15 +1074,10 @@ MODULE_DEVICE_TABLE(i2c, ak8975_id);
 
 static const struct of_device_id ak8975_of_match[] = {
 	{ .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);
-- 
2.25.1

[PATCH v2 5/5] iio: magnetometer: ak8975: Sort OF table

[Biju Das]

Sort OF table alphabetically by compatibles.

Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
v2:
 * New patch
---
 drivers/iio/magnetometer/ak8975.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iio/magnetometer/ak8975.c b/drivers/iio/magnetometer/ak8975.c
index 295b7be5e36d..7cc443a86995 100644
--- a/drivers/iio/magnetometer/ak8975.c
+++ b/drivers/iio/magnetometer/ak8975.c
@@ -1073,8 +1073,8 @@ static const struct i2c_device_id ak8975_id[] = {
 MODULE_DEVICE_TABLE(i2c, ak8975_id);
 
 static const struct of_device_id ak8975_of_match[] = {
-	{ .compatible = "asahi-kasei,ak8975", .data = &ak_def_array[AK8975] },
 	{ .compatible = "asahi-kasei,ak8963", .data = &ak_def_array[AK8963] },
+	{ .compatible = "asahi-kasei,ak8975", .data = &ak_def_array[AK8975] },
 	{ .compatible = "asahi-kasei,ak09911", .data = &ak_def_array[AK09911] },
 	{ .compatible = "asahi-kasei,ak09912", .data = &ak_def_array[AK09912] },
 	{ .compatible = "asahi-kasei,ak09916", .data = &ak_def_array[AK09916] },
-- 
2.25.1

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

[Andy Shevchenko]

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?

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

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...

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/5] iio: magnetometer: ak8975: Sort ID and ACPI tables

[Andy Shevchenko]

On Fri, Aug 18, 2023 at 08:55:57AM +0100, Biju Das wrote:
> Sort ID table alphabetically by name and acpi table by HID.

ACPI

> While at it, drop blank line before ID table.

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 4/5] iio: magnetometer: ak8975: Drop deprecated enums from OF table

[Andy Shevchenko]

On Fri, Aug 18, 2023 at 08:55:59AM +0100, Biju Das wrote:
> Drop deprecated enums from OF table as corresponding entries
> are removed from bindings and it also saves memory.

You can't do this.

Only sorting by "prefixed first" criteria is possible.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 5/5] iio: magnetometer: ak8975: Sort OF table

[Andy Shevchenko]

On Fri, Aug 18, 2023 at 08:56:00AM +0100, Biju Das wrote:
> Sort OF table alphabetically by compatibles.

> Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Wrong, I haven't suggested that. See comment to the previous patch.

And this is definitely wrong as Geert explained already why.
You need to fix the code that handles the ID table first.

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

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>

-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH v2 5/5] iio: magnetometer: ak8975: Sort OF table

[Biju Das]

Hi Andy Shevchenko,

Thanks for the feedback.                   

> Subject: Re: [PATCH v2 5/5] iio: magnetometer: ak8975: Sort OF table
> 
> On Fri, Aug 18, 2023 at 08:56:00AM +0100, Biju Das wrote:
> > Sort OF table alphabetically by compatibles.
> 
> > Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> 
> Wrong, I haven't suggested that. See comment to the previous patch.
> 
> And this is definitely wrong as Geert explained already why.
> You need to fix the code that handles the ID table first.

That rule applicable only for fallback. I checked bindings and there are no fallbacks.

Cheers,
Biju

RE: [PATCH v2 4/5] iio: magnetometer: ak8975: Drop deprecated enums from OF table

[Biju Das]

Hi Andy Shevchenko,

> Subject: Re: [PATCH v2 4/5] iio: magnetometer: ak8975: Drop deprecated
> enums from OF table
> 
> On Fri, Aug 18, 2023 at 08:55:59AM +0100, Biju Das wrote:
> > Drop deprecated enums from OF table as corresponding entries are
> > removed from bindings and it also saves memory.
> 
> You can't do this.
> 
> Only sorting by "prefixed first" criteria is possible.

The rule applies only for fallback compatible. I checked bindings
and I don't find any fallback compatibles. All compatibles are just enums. Am I missing anything here??

Cheers,
Biju

RE: [PATCH v2 2/5] iio: magnetometer: ak8975: Sort ID and ACPI tables

[Biju Das]

Hi Andy Shevchenko,

Thanks for the feedback.

> Subject: Re: [PATCH v2 2/5] iio: magnetometer: ak8975: Sort ID and ACPI
> tables
> 
> On Fri, Aug 18, 2023 at 08:55:57AM +0100, Biju Das wrote:
> > Sort ID table alphabetically by name and acpi table by HID.
> 
> ACPI

Agreed, not sure Jonathan can do this fix while applying??

Cheers,
Biju

> 
> > While at it, drop blank line before ID table.
> 
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> 
> --
> With Best Regards,
> Andy Shevchenko
> 

Re: [PATCH v2 5/5] iio: magnetometer: ak8975: Sort OF table

[Geert Uytterhoeven]

Hi Andy,

On Fri, Aug 18, 2023 at 1:30 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> On Fri, Aug 18, 2023 at 08:56:00AM +0100, Biju Das wrote:
> > Sort OF table alphabetically by compatibles.
>
> > Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>
> Wrong, I haven't suggested that. See comment to the previous patch.
>
> And this is definitely wrong as Geert explained already why.
> You need to fix the code that handles the ID table first.

I retracted my own comment:
https://lore.kernel.org/r/CAMuHMdUVCS_D0SBtDBrLQbAkdt0ZUbMOca+ukdwUtnGqzUr+cA@mail.gmail.com

Upon a second read, I agree my reply

    Seems like it is, cfr. the scoring system in drivers/of/base.c

was confusing, as it was not super clear if it was a response to the
first or the second line of your comment:

    You mean the OF ID list must be specifically ordered?! What a nice
minefield!
    This has to be fixed somewhere else, surely.

Conclusion: there is no issue, the scoring system handles primary
vs. fallback compatible values, irrespective of ordering.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2 5/5] iio: magnetometer: ak8975: Sort OF table

[Andy Shevchenko]

On Fri, Aug 18, 2023 at 11:39:03AM +0000, Biju Das wrote:
> > On Fri, Aug 18, 2023 at 08:56:00AM +0100, Biju Das wrote:
> > > Sort OF table alphabetically by compatibles.
> > 
> > > Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > 
> > Wrong, I haven't suggested that. See comment to the previous patch.
> > 
> > And this is definitely wrong as Geert explained already why.
> > You need to fix the code that handles the ID table first.
> 
> That rule applicable only for fallback. I checked bindings and there are no fallbacks.

You can't check the _whole_ world, so you checked only bindings that are in tree.
But it doesn't matter as a user somewhere may use something you have no access to.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 4/5] iio: magnetometer: ak8975: Drop deprecated enums from OF table

[Andy Shevchenko]

On Fri, Aug 18, 2023 at 11:40:37AM +0000, Biju Das wrote:
> Hi Andy Shevchenko,
> 
> > Subject: Re: [PATCH v2 4/5] iio: magnetometer: ak8975: Drop deprecated
> > enums from OF table
> > 
> > On Fri, Aug 18, 2023 at 08:55:59AM +0100, Biju Das wrote:
> > > Drop deprecated enums from OF table as corresponding entries are
> > > removed from bindings and it also saves memory.
> > 
> > You can't do this.
> > 
> > Only sorting by "prefixed first" criteria is possible.
> 
> The rule applies only for fallback compatible. I checked bindings and I don't
> find any fallback compatibles. All compatibles are just enums. Am I missing
> anything here??

Yes. As per above patch. The _whole_ world is not under your / our control.
NAK to this change, sorry.

-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH v2 5/5] iio: magnetometer: ak8975: Sort OF table

[Biju Das]

Hi Andy Shevchenko,

> Subject: Re: [PATCH v2 5/5] iio: magnetometer: ak8975: Sort OF table
> 
> On Fri, Aug 18, 2023 at 11:39:03AM +0000, Biju Das wrote:
> > > On Fri, Aug 18, 2023 at 08:56:00AM +0100, Biju Das wrote:
> > > > Sort OF table alphabetically by compatibles.
> > >
> > > > Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > >
> > > Wrong, I haven't suggested that. See comment to the previous patch.
> > >
> > > And this is definitely wrong as Geert explained already why.
> > > You need to fix the code that handles the ID table first.
> >
> > That rule applicable only for fallback. I checked bindings and there are
> no fallbacks.
> 
> You can't check the _whole_ world, so you checked only bindings that are in
> tree.
> But it doesn't matter as a user somewhere may use something you have no
> access to.

Yes true, for those not using tree can patch bindings and modify the
driver and dts accordingly.

If they are using tree, they must go with binding docs and update driver/bindings accordingly.

+ device_tree.

Cheers,
Biju

RE: [PATCH v2 4/5] iio: magnetometer: ak8975: Drop deprecated enums from OF table

[Biju Das]

Hi Andy Shevchenko,

> Subject: Re: [PATCH v2 4/5] iio: magnetometer: ak8975: Drop deprecated
> enums from OF table
> 
> On Fri, Aug 18, 2023 at 11:40:37AM +0000, Biju Das wrote:
> > Hi Andy Shevchenko,
> >
> > > Subject: Re: [PATCH v2 4/5] iio: magnetometer: ak8975: Drop
> > > deprecated enums from OF table
> > >
> > > On Fri, Aug 18, 2023 at 08:55:59AM +0100, Biju Das wrote:
> > > > Drop deprecated enums from OF table as corresponding entries are
> > > > removed from bindings and it also saves memory.
> > >
> > > You can't do this.
> > >
> > > Only sorting by "prefixed first" criteria is possible.
> >
> > The rule applies only for fallback compatible. I checked bindings and
> > I don't find any fallback compatibles. All compatibles are just enums.
> > Am I missing anything here??
> 
> Yes. As per above patch. The _whole_ world is not under your / our control.
> NAK to this change, sorry.

+ device tree

All drivers that uses OF table and device tree must be based on the device tree documentation on the tree.

I agree the _whole_ world is not under your / our control. But we support whatever available on the kernel tree right? why do we care those not in the tree?

Cheers,
Biju

Re: [PATCH v2 3/5] dt-bindings: iio: magnetometer: asahi-kasei,ak8975: Drop deprecated enums

[Geert Uytterhoeven]

Hi Biju,

Thanks for your patch!

On Fri, Aug 18, 2023 at 9:56 AM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> Drop deprecated enums from bindings as it is been here for a long time.

The single user ever in the upstream kernel was fixed in commit
9846210b1ec9bbaa ("ARM: tegra: seaboard: add missing DT vendor
prefixes") in v3.8 back in 2012.
And it had to be fixed again 9 years later in commit fa0fdb78cb5d4cde
("ARM: dts: am335x: Use correct vendor prefix for Asahi Kasei Corp.").

>
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

> --- a/Documentation/devicetree/bindings/iio/magnetometer/asahi-kasei,ak8975.yaml
> +++ b/Documentation/devicetree/bindings/iio/magnetometer/asahi-kasei,ak8975.yaml
> @@ -18,13 +18,6 @@ properties:
>            - asahi-kasei,ak09911
>            - asahi-kasei,ak09912
>            - asahi-kasei,ak09916
> -      - enum:
> -          - ak8975
> -          - ak8963
> -          - ak09911
> -          - ak09912
> -          - ak09916
> -        deprecated: true
>
>    reg:
>      maxItems: 1

Gr{oetje,eeting}s,

                        Geert


--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2 4/5] iio: magnetometer: ak8975: Drop deprecated enums from OF table

[Geert Uytterhoeven]

Hi Andy,

CC devicetree

On Fri, Aug 18, 2023 at 5:03 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> On Fri, Aug 18, 2023 at 11:40:37AM +0000, Biju Das wrote:
> > > Subject: Re: [PATCH v2 4/5] iio: magnetometer: ak8975: Drop deprecated
> > > enums from OF table
> > >
> > > On Fri, Aug 18, 2023 at 08:55:59AM +0100, Biju Das wrote:
> > > > Drop deprecated enums from OF table as corresponding entries are
> > > > removed from bindings and it also saves memory.
> > >
> > > You can't do this.
> > >
> > > Only sorting by "prefixed first" criteria is possible.
> >
> > The rule applies only for fallback compatible. I checked bindings and I don't
> > find any fallback compatibles. All compatibles are just enums. Am I missing
> > anything here??
>
> Yes. As per above patch. The _whole_ world is not under your / our control.
> NAK to this change, sorry.

The single user ever in the upstream kernel was fixed in commit
9846210b1ec9bbaa ("ARM: tegra: seaboard: add missing DT vendor
prefixes") in v3.8 back in 2012.
And it had to be fixed again 9 years later in commit fa0fdb78cb5d4cde
("ARM: dts: am335x: Use correct vendor prefix for Asahi Kasei Corp.").

There may be other out-of-tree users, which would be broken by this
change.  Typically we wait a few years between deprecating a compatible
value and removing support from the driver.

As Biju is only deprecating these compatible values in PATCH 3/5 of
his series, this may be a bit premature.

Gr{oetje,eeting}s,

                        Geert


--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2 5/5] iio: magnetometer: ak8975: Sort OF table

[Andy Shevchenko]

On Fri, Aug 18, 2023 at 04:55:18PM +0200, Geert Uytterhoeven wrote:
> On Fri, Aug 18, 2023 at 1:30 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Fri, Aug 18, 2023 at 08:56:00AM +0100, Biju Das wrote:
> > > Sort OF table alphabetically by compatibles.
> >
> > > Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> >
> > Wrong, I haven't suggested that. See comment to the previous patch.
> >
> > And this is definitely wrong as Geert explained already why.
> > You need to fix the code that handles the ID table first.
> 
> I retracted my own comment:
> https://lore.kernel.org/r/CAMuHMdUVCS_D0SBtDBrLQbAkdt0ZUbMOca+ukdwUtnGqzUr+cA@mail.gmail.com
> 
> Upon a second read, I agree my reply
> 
>     Seems like it is, cfr. the scoring system in drivers/of/base.c
> 
> was confusing, as it was not super clear if it was a response to the
> first or the second line of your comment:
> 
>     You mean the OF ID list must be specifically ordered?! What a nice
> minefield!
>     This has to be fixed somewhere else, surely.
> 
> Conclusion: there is no issue, the scoring system handles primary
> vs. fallback compatible values, irrespective of ordering.

Now I'm totally confused. Previously you mentioned a couple of
different APIs — one in OF, one in SoC. AFAIU the second one
still needs to be fixed to follow the logic that OF does.

My previous understanding was that
  OF code — no issue
  SoC code — the ordering is required to be correct

Can you confirm that there is no issue in that second case?
And if there is none, why did you mention it?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 5/5] iio: magnetometer: ak8975: Sort OF table

[Geert Uytterhoeven]

Hi Andy,

On Fri, Aug 18, 2023 at 5:35 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> On Fri, Aug 18, 2023 at 04:55:18PM +0200, Geert Uytterhoeven wrote:
> > On Fri, Aug 18, 2023 at 1:30 PM Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> > > On Fri, Aug 18, 2023 at 08:56:00AM +0100, Biju Das wrote:
> > > > Sort OF table alphabetically by compatibles.
> > >
> > > > Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > >
> > > Wrong, I haven't suggested that. See comment to the previous patch.
> > >
> > > And this is definitely wrong as Geert explained already why.
> > > You need to fix the code that handles the ID table first.
> >
> > I retracted my own comment:
> > https://lore.kernel.org/r/CAMuHMdUVCS_D0SBtDBrLQbAkdt0ZUbMOca+ukdwUtnGqzUr+cA@mail.gmail.com
> >
> > Upon a second read, I agree my reply
> >
> >     Seems like it is, cfr. the scoring system in drivers/of/base.c
> >
> > was confusing, as it was not super clear if it was a response to the
> > first or the second line of your comment:
> >
> >     You mean the OF ID list must be specifically ordered?! What a nice
> > minefield!
> >     This has to be fixed somewhere else, surely.
> >
> > Conclusion: there is no issue, the scoring system handles primary
> > vs. fallback compatible values, irrespective of ordering.
>
> Now I'm totally confused. Previously you mentioned a couple of
> different APIs — one in OF, one in SoC. AFAIU the second one
> still needs to be fixed to follow the logic that OF does.
>
> My previous understanding was that
>   OF code — no issue
>   SoC code — the ordering is required to be correct

Correct.

> Can you confirm that there is no issue in that second case?
> And if there is none, why did you mention it?

There is still an issue (read: you have to be careful) in the second
case, which does not matter here, as this driver does not use
soc_device_match().
I mentioned soc_device_match() because it is the second popular way
to match on OF platforms, but behaves slightly different than
of_match_node().

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2 5/5] iio: magnetometer: ak8975: Sort OF table

[Andy Shevchenko]

On Fri, Aug 18, 2023 at 05:43:15PM +0200, Geert Uytterhoeven wrote:
> On Fri, Aug 18, 2023 at 5:35 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Fri, Aug 18, 2023 at 04:55:18PM +0200, Geert Uytterhoeven wrote:
> > > On Fri, Aug 18, 2023 at 1:30 PM Andy Shevchenko
> > > <andriy.shevchenko@linux.intel.com> wrote:
> > > > On Fri, Aug 18, 2023 at 08:56:00AM +0100, Biju Das wrote:
> > > > > Sort OF table alphabetically by compatibles.
> > > >
> > > > > Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > > >
> > > > Wrong, I haven't suggested that. See comment to the previous patch.
> > > >
> > > > And this is definitely wrong as Geert explained already why.
> > > > You need to fix the code that handles the ID table first.
> > >
> > > I retracted my own comment:
> > > https://lore.kernel.org/r/CAMuHMdUVCS_D0SBtDBrLQbAkdt0ZUbMOca+ukdwUtnGqzUr+cA@mail.gmail.com
> > >
> > > Upon a second read, I agree my reply
> > >
> > >     Seems like it is, cfr. the scoring system in drivers/of/base.c
> > >
> > > was confusing, as it was not super clear if it was a response to the
> > > first or the second line of your comment:
> > >
> > >     You mean the OF ID list must be specifically ordered?! What a nice
> > > minefield!
> > >     This has to be fixed somewhere else, surely.
> > >
> > > Conclusion: there is no issue, the scoring system handles primary
> > > vs. fallback compatible values, irrespective of ordering.
> >
> > Now I'm totally confused. Previously you mentioned a couple of
> > different APIs — one in OF, one in SoC. AFAIU the second one
> > still needs to be fixed to follow the logic that OF does.
> >
> > My previous understanding was that
> >   OF code — no issue
> >   SoC code — the ordering is required to be correct
> 
> Correct.
> 
> > Can you confirm that there is no issue in that second case?
> > And if there is none, why did you mention it?
> 
> There is still an issue (read: you have to be careful) in the second
> case, which does not matter here, as this driver does not use
> soc_device_match().
> I mentioned soc_device_match() because it is the second popular way
> to match on OF platforms, but behaves slightly different than
> of_match_node().

Now it's clear, thanks.
Biju, please add that to the commit message.

-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH v2 5/5] iio: magnetometer: ak8975: Sort OF table

[Biju Das]

Hi Andy,

Thanks for the feedback.

> Subject: Re: [PATCH v2 5/5] iio: magnetometer: ak8975: Sort OF table
> 
> On Fri, Aug 18, 2023 at 05:43:15PM +0200, Geert Uytterhoeven wrote:
> > On Fri, Aug 18, 2023 at 5:35 PM Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> > > On Fri, Aug 18, 2023 at 04:55:18PM +0200, Geert Uytterhoeven wrote:
> > > > On Fri, Aug 18, 2023 at 1:30 PM Andy Shevchenko
> > > > <andriy.shevchenko@linux.intel.com> wrote:
> > > > > On Fri, Aug 18, 2023 at 08:56:00AM +0100, Biju Das wrote:
> > > > > > Sort OF table alphabetically by compatibles.
> > > > >
> > > > > > Suggested-by: Andy Shevchenko
> > > > > > <andriy.shevchenko@linux.intel.com>
> > > > >
> > > > > Wrong, I haven't suggested that. See comment to the previous patch.
> > > > >
> > > > > And this is definitely wrong as Geert explained already why.
> > > > > You need to fix the code that handles the ID table first.
> > > >
> > > > I retracted my own comment:
> > > >
> > > > Upon a second read, I agree my reply
> > > >
> > > >     Seems like it is, cfr. the scoring system in drivers/of/base.c
> > > >
> > > > was confusing, as it was not super clear if it was a response to
> > > > the first or the second line of your comment:
> > > >
> > > >     You mean the OF ID list must be specifically ordered?! What a
> > > > nice minefield!
> > > >     This has to be fixed somewhere else, surely.
> > > >
> > > > Conclusion: there is no issue, the scoring system handles primary
> > > > vs. fallback compatible values, irrespective of ordering.
> > >
> > > Now I'm totally confused. Previously you mentioned a couple of
> > > different APIs — one in OF, one in SoC. AFAIU the second one still
> > > needs to be fixed to follow the logic that OF does.
> > >
> > > My previous understanding was that
> > >   OF code — no issue
> > >   SoC code — the ordering is required to be correct
> >
> > Correct.
> >
> > > Can you confirm that there is no issue in that second case?
> > > And if there is none, why did you mention it?
> >
> > There is still an issue (read: you have to be careful) in the second
> > case, which does not matter here, as this driver does not use
> > soc_device_match().
> > I mentioned soc_device_match() because it is the second popular way to
> > match on OF platforms, but behaves slightly different than
> > of_match_node().
> 
> Now it's clear, thanks.
> Biju, please add that to the commit message.

OK, will mention in the commit message about "soc_device_match".

Cheers,
Biju

Re: [PATCH v2 5/5] iio: magnetometer: ak8975: Sort OF table

[Geert Uytterhoeven]

Hi Andy,

On Fri, Aug 18, 2023 at 7:04 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> On Fri, Aug 18, 2023 at 05:43:15PM +0200, Geert Uytterhoeven wrote:
> > On Fri, Aug 18, 2023 at 5:35 PM Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> > > On Fri, Aug 18, 2023 at 04:55:18PM +0200, Geert Uytterhoeven wrote:
> > > > On Fri, Aug 18, 2023 at 1:30 PM Andy Shevchenko
> > > > <andriy.shevchenko@linux.intel.com> wrote:
> > > > > On Fri, Aug 18, 2023 at 08:56:00AM +0100, Biju Das wrote:
> > > > > > Sort OF table alphabetically by compatibles.
> > > > >
> > > > > > Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > > > >
> > > > > Wrong, I haven't suggested that. See comment to the previous patch.
> > > > >
> > > > > And this is definitely wrong as Geert explained already why.
> > > > > You need to fix the code that handles the ID table first.
> > > >
> > > > I retracted my own comment:
> > > > https://lore.kernel.org/r/CAMuHMdUVCS_D0SBtDBrLQbAkdt0ZUbMOca+ukdwUtnGqzUr+cA@mail.gmail.com
> > > >
> > > > Upon a second read, I agree my reply
> > > >
> > > >     Seems like it is, cfr. the scoring system in drivers/of/base.c
> > > >
> > > > was confusing, as it was not super clear if it was a response to the
> > > > first or the second line of your comment:
> > > >
> > > >     You mean the OF ID list must be specifically ordered?! What a nice
> > > > minefield!
> > > >     This has to be fixed somewhere else, surely.
> > > >
> > > > Conclusion: there is no issue, the scoring system handles primary
> > > > vs. fallback compatible values, irrespective of ordering.
> > >
> > > Now I'm totally confused. Previously you mentioned a couple of
> > > different APIs — one in OF, one in SoC. AFAIU the second one
> > > still needs to be fixed to follow the logic that OF does.
> > >
> > > My previous understanding was that
> > >   OF code — no issue
> > >   SoC code — the ordering is required to be correct
> >
> > Correct.
> >
> > > Can you confirm that there is no issue in that second case?
> > > And if there is none, why did you mention it?
> >
> > There is still an issue (read: you have to be careful) in the second
> > case, which does not matter here, as this driver does not use
> > soc_device_match().
> > I mentioned soc_device_match() because it is the second popular way
> > to match on OF platforms, but behaves slightly different than
> > of_match_node().
>
> Now it's clear, thanks.
> Biju, please add that to the commit message.

All of that? The only thing that matters is that OF match tables
use scoring, so order shouldn't matter.

soc_device_match() uses different tables, and is irrelevant here.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2 3/5] dt-bindings: iio: magnetometer: asahi-kasei,ak8975: Drop deprecated enums

[Rob Herring]

On Fri, Aug 18, 2023 at 08:55:58AM +0100, Biju Das wrote:
> Drop deprecated enums from bindings as it is been here for a long time.

Would be good to have "long time" defined as Geert provided.

> 
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> ---
> v2:
>  * New patch
> ---
>  .../bindings/iio/magnetometer/asahi-kasei,ak8975.yaml      | 7 -------
>  1 file changed, 7 deletions(-)

Acked-by: Rob Herring <robh@kernel.org>

Re: [PATCH v2 4/5] iio: magnetometer: ak8975: Drop deprecated enums from OF table

[Jonathan Cameron]

On Fri, 18 Aug 2023 17:17:44 +0200
Geert Uytterhoeven <geert@linux-m68k.org> wrote:

> Hi Andy,
> 
> CC devicetree
> 
> On Fri, Aug 18, 2023 at 5:03 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Fri, Aug 18, 2023 at 11:40:37AM +0000, Biju Das wrote:  
> > > > Subject: Re: [PATCH v2 4/5] iio: magnetometer: ak8975: Drop deprecated
> > > > enums from OF table
> > > >
> > > > On Fri, Aug 18, 2023 at 08:55:59AM +0100, Biju Das wrote:  
> > > > > Drop deprecated enums from OF table as corresponding entries are
> > > > > removed from bindings and it also saves memory.  
> > > >
> > > > You can't do this.
> > > >
> > > > Only sorting by "prefixed first" criteria is possible.  
> > >
> > > The rule applies only for fallback compatible. I checked bindings and I don't
> > > find any fallback compatibles. All compatibles are just enums. Am I missing
> > > anything here??  
> >
> > Yes. As per above patch. The _whole_ world is not under your / our control.
> > NAK to this change, sorry.  
> 
> The single user ever in the upstream kernel was fixed in commit
> 9846210b1ec9bbaa ("ARM: tegra: seaboard: add missing DT vendor
> prefixes") in v3.8 back in 2012.
> And it had to be fixed again 9 years later in commit fa0fdb78cb5d4cde
> ("ARM: dts: am335x: Use correct vendor prefix for Asahi Kasei Corp.").
> 
> There may be other out-of-tree users, which would be broken by this
> change.  Typically we wait a few years between deprecating a compatible
> value and removing support from the driver.
> 
> As Biju is only deprecating these compatible values in PATCH 3/5 of
> his series, this may be a bit premature.
Absolutely.  I'd go a bit further.
Unless there is a maintenance reason to remove these (after a few years from
removal in the binding doc) then we never remove them as it can only hurt users.

Jonathan

> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds

Re: [PATCH v2 3/5] dt-bindings: iio: magnetometer: asahi-kasei,ak8975: Drop deprecated enums

[Jonathan Cameron]

On Fri, 18 Aug 2023 08:55:58 +0100
Biju Das <biju.das.jz@bp.renesas.com> wrote:

> Drop deprecated enums from bindings as it is been here for a long time.
> 
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
Applied patches 1-3

Thanks,

Jonathan

> ---
> v2:
>  * New patch
> ---
>  .../bindings/iio/magnetometer/asahi-kasei,ak8975.yaml      | 7 -------
>  1 file changed, 7 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/iio/magnetometer/asahi-kasei,ak8975.yaml b/Documentation/devicetree/bindings/iio/magnetometer/asahi-kasei,ak8975.yaml
> index 9790f75fc669..ee77558e9800 100644
> --- a/Documentation/devicetree/bindings/iio/magnetometer/asahi-kasei,ak8975.yaml
> +++ b/Documentation/devicetree/bindings/iio/magnetometer/asahi-kasei,ak8975.yaml
> @@ -18,13 +18,6 @@ properties:
>            - asahi-kasei,ak09911
>            - asahi-kasei,ak09912
>            - asahi-kasei,ak09916
> -      - enum:
> -          - ak8975
> -          - ak8963
> -          - ak09911
> -          - ak09912
> -          - ak09916
> -        deprecated: true
>  
>    reg:
>      maxItems: 1

Re: [PATCH v2 5/5] iio: magnetometer: ak8975: Sort OF table

[Jonathan Cameron]

On Fri, 18 Aug 2023 08:56:00 +0100
Biju Das <biju.das.jz@bp.renesas.com> wrote:

> Sort OF table alphabetically by compatibles.
> 
> Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
I adjusted this for not taking patch 4 and applied.

Thanks,

Jonathan

> ---
> v2:
>  * New patch
> ---
>  drivers/iio/magnetometer/ak8975.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/magnetometer/ak8975.c b/drivers/iio/magnetometer/ak8975.c
> index 295b7be5e36d..7cc443a86995 100644
> --- a/drivers/iio/magnetometer/ak8975.c
> +++ b/drivers/iio/magnetometer/ak8975.c
> @@ -1073,8 +1073,8 @@ static const struct i2c_device_id ak8975_id[] = {
>  MODULE_DEVICE_TABLE(i2c, ak8975_id);
>  
>  static const struct of_device_id ak8975_of_match[] = {
> -	{ .compatible = "asahi-kasei,ak8975", .data = &ak_def_array[AK8975] },
>  	{ .compatible = "asahi-kasei,ak8963", .data = &ak_def_array[AK8963] },
> +	{ .compatible = "asahi-kasei,ak8975", .data = &ak_def_array[AK8975] },
>  	{ .compatible = "asahi-kasei,ak09911", .data = &ak_def_array[AK09911] },
>  	{ .compatible = "asahi-kasei,ak09912", .data = &ak_def_array[AK09912] },
>  	{ .compatible = "asahi-kasei,ak09916", .data = &ak_def_array[AK09916] },

Re: [PATCH v2 5/5] iio: magnetometer: ak8975: Sort OF table

[Jonathan Cameron]

On Mon, 28 Aug 2023 15:27:12 +0100
Jonathan Cameron <jic23@kernel.org> wrote:

> On Fri, 18 Aug 2023 08:56:00 +0100
> Biju Das <biju.das.jz@bp.renesas.com> wrote:
> 
> > Sort OF table alphabetically by compatibles.
> > 
> > Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>  
> I adjusted this for not taking patch 4 and applied.
Dropped this one again as I just saw the suggestion to
group the prefixed an on prefixed version which is a good idea.

Jonathan

> 
> Thanks,
> 
> Jonathan
> 
> > ---
> > v2:
> >  * New patch
> > ---
> >  drivers/iio/magnetometer/ak8975.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/iio/magnetometer/ak8975.c b/drivers/iio/magnetometer/ak8975.c
> > index 295b7be5e36d..7cc443a86995 100644
> > --- a/drivers/iio/magnetometer/ak8975.c
> > +++ b/drivers/iio/magnetometer/ak8975.c
> > @@ -1073,8 +1073,8 @@ static const struct i2c_device_id ak8975_id[] = {
> >  MODULE_DEVICE_TABLE(i2c, ak8975_id);
> >  
> >  static const struct of_device_id ak8975_of_match[] = {
> > -	{ .compatible = "asahi-kasei,ak8975", .data = &ak_def_array[AK8975] },
> >  	{ .compatible = "asahi-kasei,ak8963", .data = &ak_def_array[AK8963] },
> > +	{ .compatible = "asahi-kasei,ak8975", .data = &ak_def_array[AK8975] },
> >  	{ .compatible = "asahi-kasei,ak09911", .data = &ak_def_array[AK09911] },
> >  	{ .compatible = "asahi-kasei,ak09912", .data = &ak_def_array[AK09912] },
> >  	{ .compatible = "asahi-kasei,ak09916", .data = &ak_def_array[AK09916] },  
> 

Re: [PATCH v2 4/5] iio: magnetometer: ak8975: Drop deprecated enums from OF table

[Rob Herring]

On Mon, Aug 28, 2023 at 9:22 AM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Fri, 18 Aug 2023 17:17:44 +0200
> Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> > Hi Andy,
> >
> > CC devicetree
> >
> > On Fri, Aug 18, 2023 at 5:03 PM Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> > > On Fri, Aug 18, 2023 at 11:40:37AM +0000, Biju Das wrote:
> > > > > Subject: Re: [PATCH v2 4/5] iio: magnetometer: ak8975: Drop deprecated
> > > > > enums from OF table
> > > > >
> > > > > On Fri, Aug 18, 2023 at 08:55:59AM +0100, Biju Das wrote:
> > > > > > Drop deprecated enums from OF table as corresponding entries are
> > > > > > removed from bindings and it also saves memory.
> > > > >
> > > > > You can't do this.
> > > > >
> > > > > Only sorting by "prefixed first" criteria is possible.
> > > >
> > > > The rule applies only for fallback compatible. I checked bindings and I don't
> > > > find any fallback compatibles. All compatibles are just enums. Am I missing
> > > > anything here??
> > >
> > > Yes. As per above patch. The _whole_ world is not under your / our control.
> > > NAK to this change, sorry.
> >
> > The single user ever in the upstream kernel was fixed in commit
> > 9846210b1ec9bbaa ("ARM: tegra: seaboard: add missing DT vendor
> > prefixes") in v3.8 back in 2012.
> > And it had to be fixed again 9 years later in commit fa0fdb78cb5d4cde
> > ("ARM: dts: am335x: Use correct vendor prefix for Asahi Kasei Corp.").
> >
> > There may be other out-of-tree users, which would be broken by this
> > change.  Typically we wait a few years between deprecating a compatible
> > value and removing support from the driver.
> >
> > As Biju is only deprecating these compatible values in PATCH 3/5 of
> > his series, this may be a bit premature.
> Absolutely.  I'd go a bit further.
> Unless there is a maintenance reason to remove these (after a few years from
> removal in the binding doc) then we never remove them as it can only hurt users.

I'm tracking undocumented compatibles (with 'make
dt_compatible_check') in the kernel tree. Dropping the binding makes
these undocumented (and now showing up in my diff between Linus and
next). So please apply both or none.

Rob

Re: [PATCH v2 4/5] iio: magnetometer: ak8975: Drop deprecated enums from OF table

[Jonathan Cameron]

On Fri, 6 Oct 2023 09:58:34 -0500
Rob Herring <robh@kernel.org> wrote:

> On Mon, Aug 28, 2023 at 9:22 AM Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > On Fri, 18 Aug 2023 17:17:44 +0200
> > Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> >  
> > > Hi Andy,
> > >
> > > CC devicetree
> > >
> > > On Fri, Aug 18, 2023 at 5:03 PM Andy Shevchenko
> > > <andriy.shevchenko@linux.intel.com> wrote:  
> > > > On Fri, Aug 18, 2023 at 11:40:37AM +0000, Biju Das wrote:  
> > > > > > Subject: Re: [PATCH v2 4/5] iio: magnetometer: ak8975: Drop deprecated
> > > > > > enums from OF table
> > > > > >
> > > > > > On Fri, Aug 18, 2023 at 08:55:59AM +0100, Biju Das wrote:  
> > > > > > > Drop deprecated enums from OF table as corresponding entries are
> > > > > > > removed from bindings and it also saves memory.  
> > > > > >
> > > > > > You can't do this.
> > > > > >
> > > > > > Only sorting by "prefixed first" criteria is possible.  
> > > > >
> > > > > The rule applies only for fallback compatible. I checked bindings and I don't
> > > > > find any fallback compatibles. All compatibles are just enums. Am I missing
> > > > > anything here??  
> > > >
> > > > Yes. As per above patch. The _whole_ world is not under your / our control.
> > > > NAK to this change, sorry.  
> > >
> > > The single user ever in the upstream kernel was fixed in commit
> > > 9846210b1ec9bbaa ("ARM: tegra: seaboard: add missing DT vendor
> > > prefixes") in v3.8 back in 2012.
> > > And it had to be fixed again 9 years later in commit fa0fdb78cb5d4cde
> > > ("ARM: dts: am335x: Use correct vendor prefix for Asahi Kasei Corp.").
> > >
> > > There may be other out-of-tree users, which would be broken by this
> > > change.  Typically we wait a few years between deprecating a compatible
> > > value and removing support from the driver.
> > >
> > > As Biju is only deprecating these compatible values in PATCH 3/5 of
> > > his series, this may be a bit premature.  
> > Absolutely.  I'd go a bit further.
> > Unless there is a maintenance reason to remove these (after a few years from
> > removal in the binding doc) then we never remove them as it can only hurt users.  
> 
> I'm tracking undocumented compatibles (with 'make
> dt_compatible_check') in the kernel tree. Dropping the binding makes
> these undocumented (and now showing up in my diff between Linus and
> next). So please apply both or none.
> 
> Rob

Given I have some merges in the togreg tree and it's at least in theory
non rebasing, I'll revert patch 3 from this series rather than dropping it.

Thanks for letting me know.

Jonathan

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

[André Apitzsch]

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);

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

[Geert Uytterhoeven]

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

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

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

[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...  

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
> 

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

[Geert Uytterhoeven]

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

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

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

[André Apitzsch]

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
> > 
> 

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

[Biju Das]

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

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

[Biju Das]

> 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

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

[Andy Shevchenko]

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.

-- 
With Best Regards,
Andy Shevchenko

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

[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.

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

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

[Jonathan Cameron]

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
> 

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

[Andy Shevchenko]

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!

-- 
With Best Regards,
Andy Shevchenko

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

[André Apitzsch]

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
> 

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

[Biju Das]

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

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

[Jonathan Cameron]

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