Re: [PATCH 0/2] Fix Regression with AXP20X for 6.13-rc1

[Jonathan Cameron <jic23@kernel.org>]

On Tue, 10 Dec 2024 16:48:57 -0600
Chris Morgan <macroalpha82@gmail.com> wrote:

> From: Chris Morgan <macromorgan@hotmail.com>
> 
> After performing a git bisect, I identified a commit that broke the
> battery and charger driver for my AXP717 PMIC. This was caused by
> commit e37ec3218870 ("mfd: axp20x: Allow multiple regulators").
> 
> After digging into it, it appears when mfd_add_devices was called with
> a platform ID of PLATFORM_DEVID_NONE, the devm_iio_channel_get() call
> made by the various AXP20X power drivers would not be able to generate
> a dev_name(dev) for some reason, and the iio_channel_get() call used in
> the devm_ helper would fall back to making a iio_channel_get_sys()
> call. After the platform ID was updated, now iio_channel_get() is no
> longer falling back to iio_channel_get_sys(). At least this is my
> limited understanding of what happened.

The dev_name(dev) not getting a name doesn't sound quite right to me.

Time to look at the ancient creaking ghost that is the iio_map handling. 

struct iio_channel *iio_channel_get(struct device *dev,
				    const char *channel_name)
{
	const char *name = dev ? dev_name(dev) : NULL;
	struct iio_channel *channel;

	if (dev) {
		channel = fwnode_iio_channel_get_by_name(dev_fwnode(dev),
							 channel_name);
		if (!IS_ERR(channel) || PTR_ERR(channel) != -ENODEV)
			return channel;
	}

	return iio_channel_get_sys(name, channel_name);
}
EXPORT_SYMBOL_GPL(iio_channel_get);

We didn't invent the relevant phandle stuff in DT via the patch you point at
so all that matters is what gets passed to that iio_channel_get_sys()

So key here is that dev should be set, so we are passing dev_name(dev) into
iio_channel_get_sys()
I'm guessing that changed... 

Ah.  The iio_maps in
https://elixir.bootlin.com/linux/v6.12.4/source/drivers/iio/adc/axp20x_adc.c#L158
are our problem. Those hardcode the consumer_dev name. The fix just changed
those names. Back when this infrastructure was written we were in the world of
board files, so everything was hard coded in them - or in an MFD like this
it was treated as a singleton device.

So as to how to fix it... Assuming the new device names are the same for all
the mfd parts that make up each pmic, then you should be able to figure out the
 extra the number and build the channel maps to allow you to find the numbered
devices.

That's a lot lighter change than moving over to DT based phandles for all this.
(which is the modern way to handle it).

As a cheeky check, just edit those maps to whatever IDs you have and see
if it works.  Probably not an upstreamable solution but will confirm we have
it correct.

Your patch works because we allow for some fuzzy matching (I can't remember
why) that doesn't use the consumer device name.
That works as long as there is only one instance.  I'm guessing all this
mess came about because someone has a board with two of these devices. On such
a board we need the precise matching including the device name.

Jonathan

> 
> To fix this, I added a new devm_ helper of devm_iio_channel_get_sys()
> that directly calls iio_channel_get_sys(), and I updated all the
> affected drivers with the new routine. I then no longer experienced
> any issues with the drivers on my devices.
> 
> Chris Morgan (2):
>   iio: core: Add devm_ API for iio_channel_get_sys
>   power: supply: axp20x: Use devm_iio_channel_get_sys() for iio chans
> 
>  drivers/iio/inkern.c                    | 18 ++++++++++++++++++
>  drivers/power/supply/axp20x_ac_power.c  |  4 ++--
>  drivers/power/supply/axp20x_battery.c   | 16 ++++++++--------
>  drivers/power/supply/axp20x_usb_power.c |  6 +++---
>  include/linux/iio/consumer.h            | 20 ++++++++++++++++++++
>  5 files changed, 51 insertions(+), 13 deletions(-)
>