[PATCH 0/4] hwmon: Update handling of chip-id enums

[PATCH 0/4] hwmon: Update handling of chip-id enums

[Guenter Roeck]

With the transition to use device_get_match_data() or i2c_get_match_data()
instead of i2c_match_id() and of_match_device(), the assumption was made
that the data pointer in struct i2c_device_id and struct of_device_id must
not be NULL (0). Initial patches changed enums used in those data pointers
to start with 1.

However, it turns out that this is only necessary if device_get_match_data()
is used. i2c_get_match_data() calls device_get_match_data() and uses
i2c_match_id() as fallback if device_get_match_data() returns NULL.
Therefore, it is perfectly valid to keep using 0 as starting enum value
as long as struct i2c_device_id is complete and matches the data in struct
of_device_id.

It is confusing to have some drivers start enums with 0 and others starting
them with 1, even more so if that is done inconsistently and/or if enums
starting with 1 are used to index arrays. Let enums start with index 0
where possible, and otherwise explain why the index has to start with 1.

----------------------------------------------------------------
Guenter Roeck (4):
      hwmon: (pmbus/lm25066) Let enum chips start with index 0
      hwmon: (nct6775) Let enum kinds start with index 0
      hwmon: (pmbus/mp2856) Let enum chips start with index 0
      hwmon: (pmbus/max31827) Explain why enum chips must not start with 0

 drivers/hwmon/max31827.c      | 5 +++++
 drivers/hwmon/nct6775.h       | 2 +-
 drivers/hwmon/pmbus/lm25066.c | 2 +-
 drivers/hwmon/pmbus/mp2856.c  | 8 ++++----
 4 files changed, 11 insertions(+), 6 deletions(-)

[PATCH 1/4] hwmon: (pmbus/lm25066) Let enum chips start with index 0

[Guenter Roeck]

Commit ac0c26bae662 ("hwmon: (lm25066) Use i2c_get_match_data()") changed
enum chips to start with 1 instead of 0, under the assumption that
the data pointer in of_device_id must not start with 0 (NULL) if
i2c_get_match_data() is used. However, that is perfectly fine as long as
there is also an i2c_device_id array with the same data which is used
as fallback in that case.

Let enum chips start with 0 to avoid confusion against other drivers
where the enum starts with 0 and i2c_get_match_data() is used as well.

Cc: Rob Herring <robh@kernel.org>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/hwmon/pmbus/lm25066.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/hwmon/pmbus/lm25066.c b/drivers/hwmon/pmbus/lm25066.c
index cfffa4cdc0df..c36c124d1a2d 100644
--- a/drivers/hwmon/pmbus/lm25066.c
+++ b/drivers/hwmon/pmbus/lm25066.c
@@ -17,7 +17,7 @@
 #include <linux/of.h>
 #include "pmbus.h"
 
-enum chips { lm25056 = 1, lm25066, lm5064, lm5066, lm5066i };
+enum chips { lm25056, lm25066, lm5064, lm5066, lm5066i };
 
 #define LM25066_READ_VAUX		0xd0
 #define LM25066_MFR_READ_IIN		0xd1
-- 
2.39.2

[PATCH 2/4] hwmon: (nct6775) Let enum kinds start with index 0

[Guenter Roeck]

Commit 10a0575ea09d ("hwmon: (nct6775-i2c) Use i2c_get_match_data()")
introduced calling i2c_get_match_data() to the nct6775 driver. As part
of that commit, enum kinds was changed to start with 1, based on

    Adjust the 'kinds' enum to not use 0, so that no match data can be
    distinguished from a valid enum value.

The patch had to be fixed later with commit 2792fc8f8c83 ("hwmon:
(nct6775-core) Explicitly initialize nct6775_device_names indexes") and
commit efe86092ab31 ("hwmon: (nct6775-platform) Explicitly initialize
nct6775_sio_names indexes").

Various patches submitted later show that the change from 0 to 1 is
not really necessary. As it turns out, it is perfectly fine as long as
there is an i2c_device_id array with the same data as in the of_device_id
array. This data is used as fallback if the data pointer in struct
of_device_id is NULL (0).

Let enum chips start with 0 to avoid confusion against other drivers
where the enum starts with 0 and i2c_get_match_data() is used as well.

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/hwmon/nct6775.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/hwmon/nct6775.h b/drivers/hwmon/nct6775.h
index d31e7a030216..296eff99d003 100644
--- a/drivers/hwmon/nct6775.h
+++ b/drivers/hwmon/nct6775.h
@@ -4,7 +4,7 @@
 
 #include <linux/types.h>
 
-enum kinds { nct6106 = 1, nct6116, nct6775, nct6776, nct6779, nct6791, nct6792,
+enum kinds { nct6106, nct6116, nct6775, nct6776, nct6779, nct6791, nct6792,
 	     nct6793, nct6795, nct6796, nct6797, nct6798, nct6799 };
 enum pwm_enable { off, manual, thermal_cruise, speed_cruise, sf3, sf4 };
 
-- 
2.39.2

[PATCH 3/4] hwmon: (pmbus/mp2856) Let enum chips start with index 0

[Guenter Roeck]

Earlier it was assumed that the data pointer in of_device_id must not start
with 0 (NULL) if i2c_get_match_data() is used. However, it turns out that
this is perfectly fine as long as there is also an i2c_device_id array with
the same data, which is used as fallback in that case.

Let enum chips start with 0 to avoid confusion against other drivers
where the enum starts with 0 and i2c_get_match_data() is used as well.

While doing that, remove chip_id from struct mp2856_data since it is only
used in the probe function, and typecast the result of i2c_get_match_data()
to kernel_ulong_t to avoid the double typecast.

Cc: Peter Yin <peteryin.openbmc@gmail.com>
Cc: Potin Lai <potin.lai.pt@gmail.com>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/hwmon/pmbus/mp2856.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/hwmon/pmbus/mp2856.c b/drivers/hwmon/pmbus/mp2856.c
index 6969350f5d7d..41bb86667091 100644
--- a/drivers/hwmon/pmbus/mp2856.c
+++ b/drivers/hwmon/pmbus/mp2856.c
@@ -46,7 +46,7 @@
 
 #define MP2856_PAGE_NUM			2
 
-enum chips { mp2856 = 1, mp2857 };
+enum chips { mp2856, mp2857 };
 
 static const int mp2856_max_phases[][MP2856_PAGE_NUM] = {
 	[mp2856] = { MP2856_MAX_PHASE_RAIL1, MP2856_MAX_PHASE_RAIL2 },
@@ -66,7 +66,6 @@ struct mp2856_data {
 	int vout_format[MP2856_PAGE_NUM];
 	int curr_sense_gain[MP2856_PAGE_NUM];
 	int max_phases[MP2856_PAGE_NUM];
-	enum chips chip_id;
 };
 
 #define to_mp2856_data(x)	container_of(x, struct mp2856_data, info)
@@ -397,6 +396,7 @@ static int mp2856_probe(struct i2c_client *client)
 {
 	struct pmbus_driver_info *info;
 	struct mp2856_data *data;
+	enum chips chip_id;
 	int ret;
 
 	data = devm_kzalloc(&client->dev, sizeof(struct mp2856_data),
@@ -404,9 +404,9 @@ static int mp2856_probe(struct i2c_client *client)
 	if (!data)
 		return -ENOMEM;
 
-	data->chip_id = (enum chips)(uintptr_t)i2c_get_match_data(client);
+	chip_id = (kernel_ulong_t)i2c_get_match_data(client);
 
-	memcpy(data->max_phases, mp2856_max_phases[data->chip_id],
+	memcpy(data->max_phases, mp2856_max_phases[chip_id],
 	       sizeof(data->max_phases));
 
 	memcpy(&data->info, &mp2856_info, sizeof(*info));
-- 
2.39.2

[PATCH 4/4] hwmon: (pmbus/max31827) Explain why enum chips must not start with 0

[Guenter Roeck]

If a driver calls device_get_match_data(), the .data pointer in its id
data structures must not be NULL/0 because device_get_match_data()
returns NULL if an entry is not found. Explain that in a comment to avoid
confusion why this is required in this driver but not in other drivers.

Cc: Daniel Matyas <daniel.matyas@analog.com>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/hwmon/max31827.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/hwmon/max31827.c b/drivers/hwmon/max31827.c
index f8a13b30f100..391cb059e94d 100644
--- a/drivers/hwmon/max31827.c
+++ b/drivers/hwmon/max31827.c
@@ -46,6 +46,11 @@
 #define MAX31827_M_DGR_TO_16_BIT(x)	(((x) << 4) / 1000)
 #define MAX31827_DEVICE_ENABLE(x)	((x) ? 0xA : 0x0)
 
+/*
+ * The enum passed in the .data pointer of struct of_device_id must
+ * start with a value != 0 since that is a requirement for using
+ * device_get_match_data().
+ */
 enum chips { max31827 = 1, max31828, max31829 };
 
 enum max31827_cnv {
-- 
2.39.2

Re: [PATCH 4/4] hwmon: (pmbus/max31827) Explain why enum chips must not start with 0

[Nuno Sá]

On Mon, 2024-06-10 at 11:10 -0700, Guenter Roeck wrote:
> If a driver calls device_get_match_data(), the .data pointer in its id
> data structures must not be NULL/0 because device_get_match_data()
> returns NULL if an entry is not found. Explain that in a comment to avoid
> confusion why this is required in this driver but not in other drivers.
> 
> Cc: Daniel Matyas <daniel.matyas@analog.com>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---

Acked-by: Nuno Sa <nuno.sa@analog.com>

>  drivers/hwmon/max31827.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/hwmon/max31827.c b/drivers/hwmon/max31827.c
> index f8a13b30f100..391cb059e94d 100644
> --- a/drivers/hwmon/max31827.c
> +++ b/drivers/hwmon/max31827.c
> @@ -46,6 +46,11 @@
>  #define MAX31827_M_DGR_TO_16_BIT(x)	(((x) << 4) / 1000)
>  #define MAX31827_DEVICE_ENABLE(x)	((x) ? 0xA : 0x0)
>  
> +/*
> + * The enum passed in the .data pointer of struct of_device_id must
> + * start with a value != 0 since that is a requirement for using
> + * device_get_match_data().
> + */
>  enum chips { max31827 = 1, max31828, max31829 };
>  
>  enum max31827_cnv {