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

[Lee Jones <lee@kernel.org>]

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