Re: [PATCH v8 2/5] mfd: axp20x-i2c: Fix i2c instantiation

[Jerome Brunet <jbrunet@baylibre.com>]

On mer. 14 oct. 2026 at 11:48, Lee Jones <lee@kernel.org> wrote:

> Note:
>
> --- checkpatch.pl (3 issues) ---
> Line 2: WARNING: Prefer a maximum 75 chars per line (possible unwrapped commit description?)
> Line 3: ERROR: Invalid commit separator - some tools may have problems applying this
> ----------------------------------

I've checked before sending and checked again now, I don't have those
warning. I don't see anything long on line 2 or a separator near line 3.

Any specific way I should run the tool reproduce the problem if there is
one ?

>
> On Thu, 24 Sep 2026, Jerome Brunet wrote:
>
>> On jeu. 24 sept. 2026 at 09:27, Lee Jones <lee@kernel.org> wrote:
>> 
>> > On Thu, 10 Sep 2026, Jerome Brunet wrote:
>> >
>> >> Apparently this device never really supported i2c instantiation via
>> >> sysfs, because the driver does not even look at the i2c device data,
>> >> which are not set. It would only probe if the device matched through DT.
>> >> 
>> >> Fix that for correctness, even-though it is probably useless.
>> >> Add the missing AXP323 while at it.
>> >> 
>> >> Reported-by: Sashiko <sashiko-bot@kernel.org>
>> >> Closes: https://lore.kernel.org/r/20260717090209.2A09B1F000E9@smtp.kernel.org
>> >> Fixes: 41751b033aaa ("mfd: axp20x-i2c: Add i2c-ids to fix module auto-loading")
>> >> Acked-by: Chen-Yu Tsai <wens@kernel.org>
>> >> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
>> >> ---
>> >>  drivers/mfd/axp20x-i2c.c   | 24 +++++++++++++-----------
>> >>  drivers/mfd/axp20x-rsb.c   |  1 +
>> >>  drivers/mfd/axp20x.c       |  1 -
>> >>  include/linux/mfd/axp20x.h |  3 +--
>> >>  4 files changed, 15 insertions(+), 14 deletions(-)
>> >> 
>> >> diff --git a/drivers/mfd/axp20x-i2c.c b/drivers/mfd/axp20x-i2c.c
>> >> index 5c93136f977e..a76bec5604c4 100644
>> >> --- a/drivers/mfd/axp20x-i2c.c
>> >> +++ b/drivers/mfd/axp20x-i2c.c
>> >> @@ -35,6 +35,7 @@ static int axp20x_i2c_probe(struct i2c_client *i2c)
>> >>  	axp20x->irq = i2c->irq;
>> >>  	dev_set_drvdata(axp20x->dev, axp20x);
>> >>  
>> >> +	axp20x->variant = (kernel_ulong_t)i2c_get_match_data(i2c);
>> >
>> > device_get_match_data() is bus agnostic and should still work from the
>> > core driver.  What's the purpose of splitting it out and adding
>> > additional call-sites?
>> 
>> device_get_match_data() works when the data comes through that path,
>> which is generally the case.
>> 
>> i2c_get_match_data() calls device_get_match_data() then fallback to the
>> i2c_device_ids. It covers the instanciation from sysfs mainly, not
>> something usual.
>> 
>> As explained in the description, this changes plugs a hole identified by
>> sashiko that technically exists but I highly doubt anyone will
>> instanciate the PMIC through that interface. Those things do not really
>> come as breakout boards ... That being said, it is cheap to fix.
>
> I'm confused.
>
> We already call device_get_match_data() in axp20x_match_device().

Not anymore with this change. It is the point.

>
> So if you populate .driver_data in axp20x_i2c_id[], you should be
> able to drop these changes altogether, no?

device_get_match_data() only goes through the DT tables.
Not the i2c_driver.id_table which is purpose here.

My understanding is that if you instanciate the device
through sysfs (which nobody does for these devices) and you only call
device_get_match_data(), you will get NULL because there is no fwnode
associated with the device, which in this case silently translate to
AXP152_ID.

device_get_match_data() does not go through the i2c_driver.id_table.
Only the i2c code does, with i2c_get_match_data() and after trying with
device_get_match_data() internally, which is exactly what we want.

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/i2c/i2c-core-base.c?h=v7.3-rc7#n121

As noted earlier, this a problem reported by sashiko that is real but
extremely unlikely to show up in real world. I'd happy to drop
the change if you don't want it. It is not need to support the axp318.

>
>> >>  	ret = axp20x_match_device(axp20x);
>> >>  	if (ret)
>> >>  		return ret;
>> >> @@ -76,17 +77,18 @@ MODULE_DEVICE_TABLE(of, axp20x_i2c_of_match);
>> >>  #endif
>> >>  
>> >>  static const struct i2c_device_id axp20x_i2c_id[] = {
>> >> -	{ "axp152" },
>> >> -	{ "axp192" },
>> >> -	{ "axp202" },
>> >> -	{ "axp209" },
>> >> -	{ "axp221" },
>> >> -	{ "axp223" },
>> >> -	{ "axp313a" },
>> >> -	{ "axp717" },
>> >> -	{ "axp803" },
>> >> -	{ "axp806" },
>> >> -	{ "axp15060" },
>> >> +	{ .name = "axp152",	.driver_data = AXP152_ID },
>> >> +	{ .name = "axp192",	.driver_data = AXP192_ID },
>> >> +	{ .name = "axp202",	.driver_data = AXP202_ID },
>> >> +	{ .name = "axp209",	.driver_data = AXP209_ID },
>> >> +	{ .name = "axp221",	.driver_data = AXP221_ID },
>> >> +	{ .name = "axp223",	.driver_data = AXP223_ID },
>> >> +	{ .name = "axp313a",	.driver_data = AXP313A_ID },
>> >> +	{ .name = "axp323",	.driver_data = AXP323_ID },
>> >> +	{ .name = "axp717",	.driver_data = AXP717_ID },
>> >> +	{ .name = "axp803",	.driver_data = AXP803_ID },
>> >> +	{ .name = "axp806",	.driver_data = AXP806_ID },
>> >> +	{ .name = "axp15060",	.driver_data = AXP15060_ID },
>> >>  	{ }
>> >>  };
>> >>  MODULE_DEVICE_TABLE(i2c, axp20x_i2c_id);
>> >> diff --git a/drivers/mfd/axp20x-rsb.c b/drivers/mfd/axp20x-rsb.c
>> >> index 059656f2a1bd..8fdc870c1fe4 100644
>> >> --- a/drivers/mfd/axp20x-rsb.c
>> >> +++ b/drivers/mfd/axp20x-rsb.c
>> >> @@ -35,6 +35,7 @@ static int axp20x_rsb_probe(struct sunxi_rsb_device *rdev)
>> >>  	axp20x->irq = rdev->irq;
>> >>  	dev_set_drvdata(&rdev->dev, axp20x);
>> >>  
>> >> +	axp20x->variant = (kernel_ulong_t)device_get_match_data(axp20x->dev);
>> >>  	ret = axp20x_match_device(axp20x);
>> >>  	if (ret)
>> >>  		return ret;
>> >> diff --git a/drivers/mfd/axp20x.c b/drivers/mfd/axp20x.c
>> >> index 4d770e718716..de6946d4bb13 100644
>> >> --- a/drivers/mfd/axp20x.c
>> >> +++ b/drivers/mfd/axp20x.c
>> >> @@ -1267,7 +1267,6 @@ int axp20x_match_device(struct axp20x_dev *axp20x)
>> >>  	const struct mfd_cell *cells_no_irq = NULL;
>> >>  	int nr_cells_no_irq = 0;
>> >>  
>> >> -	axp20x->variant = (long)device_get_match_data(dev);
>> >>  	switch (axp20x->variant) {
>> >>  	case AXP152_ID:
>> >>  		axp20x->nr_cells = ARRAY_SIZE(axp152_cells);
>> >> diff --git a/include/linux/mfd/axp20x.h b/include/linux/mfd/axp20x.h
>> >> index b352661d99a1..1badb4868d17 100644
>> >> --- a/include/linux/mfd/axp20x.h
>> >> +++ b/include/linux/mfd/axp20x.h
>> >> @@ -997,8 +997,7 @@ static inline int axp20x_read_variable_width(struct regmap *regmap,
>> >>  /**
>> >>   * axp20x_match_device(): Setup axp20x variant related fields
>> >>   *
>> >> - * @axp20x: axp20x device to setup (.dev field must be set)
>> >> - * @dev: device associated with this axp20x device
>> >> + * @axp20x: axp20x device to setup (.dev and variant fields must be set)
>> >>   *
>> >>   * This lets the axp20x core configure the mfd cells and register maps
>> >>   * for later use.
>> >> 
>> >> -- 
>> >> 2.53.0
>> >> 
>> >
>> > -- 
>> > Lee Jones
>> 
>> -- 
>> Jerome
>
> -- 
> Lee Jones

-- 
Jerome