[PATCH v2] mfd: max77620: publish module aliases with variant match data

[PATCH v2] mfd: max77620: publish module aliases with variant match data

[Pengpeng Hou]

MAX77620 can be built as a module, but its I2C ID table is not exported
and it has no OF match table to provide OF aliases.

Publish both tables and obtain the chip variant through
i2c_get_match_data(). Adding OF matching alone is insufficient: a fallback
compatible can match the OF table while client->name has no corresponding
I2C ID. Dereferencing i2c_client_get_device_id() would then be invalid.

Use pointers to chip IDs in both tables. This also keeps MAX77620, whose
enum value is zero, distinguishable from missing match data. Leave the
variant-specific regmap configuration and child devices unchanged.

The issue was found by our static-analysis tool and manually reviewed.

Fixes: 0d084ee9ba87 ("mfd: max77620: Allow building as a module")
Assisted-by: gpt 5
Signed-off-by: Pengpeng Hou <hppiscas@163.com>
---
Changes since v1:
- use the author's real name
- address the matching-path issue Lee asked to review: populate OF data
  and migrate probe to i2c_get_match_data() with a missing-data check
- use non-NULL pointers for all variants, including enum value zero

Previous version:
https://lore.kernel.org/all/20260905134051.66545-1-hppiscas@163.com/

 drivers/mfd/max77620.c |   29 ++++++++++++++++++++++++-----
 1 file changed, 24 insertions(+), 5 deletions(-)

diff --git a/drivers/mfd/max77620.c b/drivers/mfd/max77620.c
index c4f89a9681f34792d24c5b75c4b88df84b3c2c66..4947ee96cc62cdeae37a15f65497d7f1013dbf11 100644
--- a/drivers/mfd/max77620.c
+++ b/drivers/mfd/max77620.c
@@ -496,7 +496,7 @@ static int max77620_pm_power_off(struct sys_off_data *data)
 
 static int max77620_probe(struct i2c_client *client)
 {
-	const struct i2c_device_id *id = i2c_client_get_device_id(client);
+	const enum max77620_chip_id *id = i2c_get_match_data(client);
 	const struct regmap_config *rmap_config;
 	struct max77620_chip *chip;
 	struct regmap_irq_chip *chip_desc;
@@ -504,6 +504,9 @@ static int max77620_probe(struct i2c_client *client)
 	int n_mfd_cells;
 	int ret;
 
+	if (!id)
+		return -ENODEV;
+
 	chip = devm_kzalloc(&client->dev, sizeof(*chip), GFP_KERNEL);
 	if (!chip)
 		return -ENOMEM;
@@ -519,7 +522,7 @@ static int max77620_probe(struct i2c_client *client)
 
 	chip->dev = &client->dev;
 	chip->chip_irq = client->irq;
-	chip->chip_id = (enum max77620_chip_id)id->driver_data;
+	chip->chip_id = *id;
 
 	switch (chip->chip_id) {
 	case MAX77620:
@@ -693,12 +696,27 @@ out:
 	return 0;
 }
 
+static const enum max77620_chip_id max77620_chip_ids[] = {
+	[MAX77620] = MAX77620,
+	[MAX20024] = MAX20024,
+	[MAX77663] = MAX77663,
+};
+
 static const struct i2c_device_id max77620_id[] = {
-	{"max77620", MAX77620},
-	{"max20024", MAX20024},
-	{"max77663", MAX77663},
+	{"max77620", (kernel_ulong_t)&max77620_chip_ids[MAX77620]},
+	{"max20024", (kernel_ulong_t)&max77620_chip_ids[MAX20024]},
+	{"max77663", (kernel_ulong_t)&max77620_chip_ids[MAX77663]},
 	{},
 };
+MODULE_DEVICE_TABLE(i2c, max77620_id);
+
+static const struct of_device_id max77620_of_match[] = {
+	{ .compatible = "maxim,max77620", .data = &max77620_chip_ids[MAX77620] },
+	{ .compatible = "maxim,max20024", .data = &max77620_chip_ids[MAX20024] },
+	{ .compatible = "maxim,max77663", .data = &max77620_chip_ids[MAX77663] },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, max77620_of_match);
 
 static DEFINE_SIMPLE_DEV_PM_OPS(max77620_pm_ops,
 				max77620_i2c_suspend, max77620_i2c_resume);
@@ -706,6 +724,7 @@ static DEFINE_SIMPLE_DEV_PM_OPS(max77620_pm_ops,
 static struct i2c_driver max77620_driver = {
 	.driver = {
 		.name = "max77620",
+		.of_match_table = max77620_of_match,
 		.pm = pm_sleep_ptr(&max77620_pm_ops),
 	},
 	.probe = max77620_probe,

base-commit: 319633b06ff2bfc2a7a60d2cfcab2e681a343b27

Re: [PATCH v2] mfd: max77620: publish module aliases with variant match data

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] mfd: max77620: module locked in memory due to builtin_i2c_driver()
- [Low] mfd: commit message capitalization rule violation
- [Low] i2c: i2c_device_id array missing named initializers
--

commit 136798e63ca9dfe05f3e4f123a426fecb599c415
Author: Pengpeng Hou <hppiscas@163.com>

mfd: max77620: publish module aliases with variant match data

This commit publishes both the I2C ID table and the OF match table for the
MAX77620 driver to provide module aliases. It updates both tables to use
pointers to chip IDs and retrieves the variant through i2c_get_match_data(),
ensuring reliable matching.

> mfd: max77620: publish module aliases with variant match data

[Severity: Low]
This isn't a bug, but the MFD subsystem requires the commit message
description to be capitalized after the subsystem prefix. Should "publish" be
"Publish" to match the guidelines?

> diff --git a/drivers/mfd/max77620.c b/drivers/mfd/max77620.c
> index c4f89a9681f34..4947ee96cc62c 100644
> --- a/drivers/mfd/max77620.c
> +++ b/drivers/mfd/max77620.c

[ ... ]

>  static const struct i2c_device_id max77620_id[] = {
> -	{"max77620", MAX77620},
> -	{"max20024", MAX20024},
> -	{"max77663", MAX77663},
> +	{"max77620", (kernel_ulong_t)&max77620_chip_ids[MAX77620]},
> +	{"max20024", (kernel_ulong_t)&max77620_chip_ids[MAX20024]},
> +	{"max77663", (kernel_ulong_t)&max77620_chip_ids[MAX77663]},
>  	{},
>  };

[Severity: Low]
This isn't a bug, but the I2C subsystem guidelines state that initialized
arrays of type struct i2c_device_id must use named initializers.

Could these positional arguments be replaced with C99 designated initializers
(e.g., .name = "max77620")?

> +MODULE_DEVICE_TABLE(i2c, max77620_id);
> +
> +static const struct of_device_id max77620_of_match[] = {
> +	{ .compatible = "maxim,max77620", .data = &max77620_chip_ids[MAX77620] },
> +	{ .compatible = "maxim,max20024", .data = &max77620_chip_ids[MAX20024] },
> +	{ .compatible = "maxim,max77663", .data = &max77620_chip_ids[MAX77663] },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, max77620_of_match);
>  
>  static DEFINE_SIMPLE_DEV_PM_OPS(max77620_pm_ops,
>  				max77620_i2c_suspend, max77620_i2c_resume);
> @@ -706,6 +724,7 @@ static DEFINE_SIMPLE_DEV_PM_OPS(max77620_pm_ops,
>  static struct i2c_driver max77620_driver = {
>  	.driver = {
>  		.name = "max77620",
> +		.of_match_table = max77620_of_match,
>  		.pm = pm_sleep_ptr(&max77620_pm_ops),
>  	},
>  	.probe = max77620_probe,
>  	.id_table = max77620_id,
>  };

[Severity: Medium]
Looking at the driver registration just below this in the source code:

builtin_i2c_driver(max77620_driver);

Since this driver can be built as a module (CONFIG_MFD_MAX77620=m), does the
use of builtin_i2c_driver() instead of module_i2c_driver() omit the
unregistration and cleanup paths?

If builtin_i2c_driver() is used, the module could be permanently locked in
memory upon loading (rmmod will return -EBUSY). If forcefully unloaded,
i2c_del_driver() is never called, which leaves the I2C core with a dangling
pointer to freed memory, leading to a use-after-free.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261014032139.10971-1-hppiscas@163.com?part=1

Re: [PATCH v2] mfd: max77620: publish module aliases with variant match data

[Lee Jones]

On Wed, 14 Oct 2026, Pengpeng Hou wrote:

Please review, explain or fix the Sashiko review(s).

> MAX77620 can be built as a module, but its I2C ID table is not exported
> and it has no OF match table to provide OF aliases.
>
> Publish both tables and obtain the chip variant through
> i2c_get_match_data(). Adding OF matching alone is insufficient: a fallback
> compatible can match the OF table while client->name has no corresponding
> I2C ID. Dereferencing i2c_client_get_device_id() would then be invalid.
>
> Use pointers to chip IDs in both tables. This also keeps MAX77620, whose
> enum value is zero, distinguishable from missing match data. Leave the
> variant-specific regmap configuration and child devices unchanged.
>
> The issue was found by our static-analysis tool and manually reviewed.
>
> Fixes: 0d084ee9ba87 ("mfd: max77620: Allow building as a module")
> Assisted-by: gpt 5
> Signed-off-by: Pengpeng Hou <hppiscas@163.com>
> ---
> Changes since v1:
> - use the author's real name
> - address the matching-path issue Lee asked to review: populate OF data
>   and migrate probe to i2c_get_match_data() with a missing-data check
> - use non-NULL pointers for all variants, including enum value zero
>
> Previous version:
> https://lore.kernel.org/all/20260905134051.66545-1-hppiscas@163.com/
>
>  drivers/mfd/max77620.c |   29 ++++++++++++++++++++++++-----
>  1 file changed, 24 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/mfd/max77620.c b/drivers/mfd/max77620.c
> index c4f89a9681f34792d24c5b75c4b88df84b3c2c66..4947ee96cc62cdeae37a15f65497d7f1013dbf11 100644
> --- a/drivers/mfd/max77620.c
> +++ b/drivers/mfd/max77620.c
> @@ -496,7 +496,7 @@ static int max77620_pm_power_off(struct sys_off_data *data)
>
>  static int max77620_probe(struct i2c_client *client)
>  {
> -	const struct i2c_device_id *id = i2c_client_get_device_id(client);
> +	const enum max77620_chip_id *id = i2c_get_match_data(client);
>  	const struct regmap_config *rmap_config;
>  	struct max77620_chip *chip;
>  	struct regmap_irq_chip *chip_desc;
> @@ -504,6 +504,9 @@ static int max77620_probe(struct i2c_client *client)
>  	int n_mfd_cells;
>  	int ret;
>
> +	if (!id)
> +		return -ENODEV;
> +
>  	chip = devm_kzalloc(&client->dev, sizeof(*chip), GFP_KERNEL);
>  	if (!chip)
>  		return -ENOMEM;
> @@ -519,7 +522,7 @@ static int max77620_probe(struct i2c_client *client)
>
>  	chip->dev = &client->dev;
>  	chip->chip_irq = client->irq;
> -	chip->chip_id = (enum max77620_chip_id)id->driver_data;
> +	chip->chip_id = *id;
>
>  	switch (chip->chip_id) {
>  	case MAX77620:
> @@ -693,12 +696,27 @@ out:
>  	return 0;
>  }
>
> +static const enum max77620_chip_id max77620_chip_ids[] = {
> +	[MAX77620] = MAX77620,
> +	[MAX20024] = MAX20024,
> +	[MAX77663] = MAX77663,
> +};

What on earth is happening here?  Where have you seen this before?

Please don't create an array of enums just to take the address of its
elements.  Either ensure the enum in 'include/linux/mfd/max77620.h' does
not start at 0, or cast directly via '(const void *)MAX77620' and
'(uintptr_t)i2c_get_match_data(client)'.

>  static const struct i2c_device_id max77620_id[] = {
> -	{"max77620", MAX77620},
> -	{"max20024", MAX20024},
> -	{"max77663", MAX77663},
> +	{"max77620", (kernel_ulong_t)&max77620_chip_ids[MAX77620]},
> +	{"max20024", (kernel_ulong_t)&max77620_chip_ids[MAX20024]},
> +	{"max77663", (kernel_ulong_t)&max77620_chip_ids[MAX77663]},
>  	{},
>  };
> +MODULE_DEVICE_TABLE(i2c, max77620_id);
> +
> +static const struct of_device_id max77620_of_match[] = {
> +	{ .compatible = "maxim,max77620", .data = &max77620_chip_ids[MAX77620] },
> +	{ .compatible = "maxim,max20024", .data = &max77620_chip_ids[MAX20024] },
> +	{ .compatible = "maxim,max77663", .data = &max77620_chip_ids[MAX77663] },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, max77620_of_match);
>
>  static DEFINE_SIMPLE_DEV_PM_OPS(max77620_pm_ops,
>  				max77620_i2c_suspend, max77620_i2c_resume);
> @@ -706,6 +724,7 @@ static DEFINE_SIMPLE_DEV_PM_OPS(max77620_pm_ops,
>  static struct i2c_driver max77620_driver = {
>  	.driver = {
>  		.name = "max77620",
> +		.of_match_table = max77620_of_match,
>  		.pm = pm_sleep_ptr(&max77620_pm_ops),
>  	},
>  	.probe = max77620_probe,
>
> base-commit: 319633b06ff2bfc2a7a60d2cfcab2e681a343b27
>

-- 
Lee Jones