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

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

[Chris Morgan]

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.

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(-)

-- 
2.43.0

[PATCH 1/2] iio: core: Add devm_ API for iio_channel_get_sys

[Chris Morgan]

From: Chris Morgan <macromorgan@hotmail.com>

Some kernel drivers use the IIO framework to get voltage and current
data via ADC or IIO HW driver. This is complicated by the fact that
the consumer of this data is not a child of the IIO HW which current
helpers depend on being the case.

Add resource managed version (devm_*) of the APIs so that the client
driver can call by name alone for the iio channel.

Signed-off-by: Chris Morgan <macromorgan@hotmail.com>
---
 drivers/iio/inkern.c         | 18 ++++++++++++++++++
 include/linux/iio/consumer.h | 20 ++++++++++++++++++++
 2 files changed, 38 insertions(+)

diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c
index 136b225b6bc8..5df9e272743f 100644
--- a/drivers/iio/inkern.c
+++ b/drivers/iio/inkern.c
@@ -418,6 +418,24 @@ struct iio_channel *devm_iio_channel_get(struct device *dev,
 }
 EXPORT_SYMBOL_GPL(devm_iio_channel_get);
 
+struct iio_channel *devm_iio_channel_get_sys(struct device *dev,
+					     const char *channel_name)
+{
+	struct iio_channel *channel;
+	int ret;
+
+	channel = iio_channel_get_sys(NULL, channel_name);
+	if (IS_ERR(channel))
+		return channel;
+
+	ret = devm_add_action_or_reset(dev, devm_iio_channel_free, channel);
+	if (ret)
+		return ERR_PTR(ret);
+
+	return channel;
+}
+EXPORT_SYMBOL_GPL(devm_iio_channel_get_sys);
+
 struct iio_channel *devm_fwnode_iio_channel_get_by_name(struct device *dev,
 							struct fwnode_handle *fwnode,
 							const char *channel_name)
diff --git a/include/linux/iio/consumer.h b/include/linux/iio/consumer.h
index 333d1d8ccb37..bde075b5fb65 100644
--- a/include/linux/iio/consumer.h
+++ b/include/linux/iio/consumer.h
@@ -64,6 +64,26 @@ void iio_channel_release(struct iio_channel *chan);
  */
 struct iio_channel *devm_iio_channel_get(struct device *dev,
 					 const char *consumer_channel);
+
+/**
+ * devm_iio_channel_get_sys() - Resource managed version of
+				iio_channel_get_sys().
+ * @dev:		Pointer to consumer device. Device name must match
+ *			the name of the device as provided in the iio_map
+ *			with which the desired provider to consumer mapping
+ *			was registered.
+ * @consumer_channel:	Unique name to identify the channel on the consumer
+ *			side. This typically describes the channels use within
+ *			the consumer. E.g. 'battery_voltage'
+ *
+ * Returns a pointer to negative errno if it is not able to get the iio channel
+ * otherwise returns valid pointer for iio channel.
+ *
+ * The allocated iio channel is automatically released when the device is
+ * unbound.
+ */
+struct iio_channel *devm_iio_channel_get_sys(struct device *dev,
+					     const char *consumer_channel);
 /**
  * iio_channel_get_all() - get all channels associated with a client
  * @dev:		Pointer to consumer device.
-- 
2.43.0

[PATCH 2/2] power: supply: axp20x: Use devm_iio_channel_get_sys() for iio chans

[Chris Morgan]

From: Chris Morgan <macromorgan@hotmail.com>

Since commit e37ec3218870 ("mfd: axp20x: Allow multiple regulators")
the drivers for AC, battery, and USB have failed to probe when the
option of CONFIG_AXP20X_ADC is enabled. This appears to be because
the existing function devm_iio_channel_get() no longer calls
iio_channel_get_sys(). Use the newly created function of
devm_iio_channel_get_sys() instead.

Fixes: e37ec3218870 ("mfd: axp20x: Allow multiple regulators")
Signed-off-by: Chris Morgan <macromorgan@hotmail.com>
---
 drivers/power/supply/axp20x_ac_power.c  |  4 ++--
 drivers/power/supply/axp20x_battery.c   | 16 ++++++++--------
 drivers/power/supply/axp20x_usb_power.c |  6 +++---
 3 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/drivers/power/supply/axp20x_ac_power.c b/drivers/power/supply/axp20x_ac_power.c
index e5733cb9e19e..07de889af16b 100644
--- a/drivers/power/supply/axp20x_ac_power.c
+++ b/drivers/power/supply/axp20x_ac_power.c
@@ -343,14 +343,14 @@ static int axp20x_ac_power_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	if (axp_data->acin_adc) {
-		power->acin_v = devm_iio_channel_get(&pdev->dev, "acin_v");
+		power->acin_v = devm_iio_channel_get_sys(&pdev->dev, "acin_v");
 		if (IS_ERR(power->acin_v)) {
 			if (PTR_ERR(power->acin_v) == -ENODEV)
 				return -EPROBE_DEFER;
 			return PTR_ERR(power->acin_v);
 		}
 
-		power->acin_i = devm_iio_channel_get(&pdev->dev, "acin_i");
+		power->acin_i = devm_iio_channel_get_sys(&pdev->dev, "acin_i");
 		if (IS_ERR(power->acin_i)) {
 			if (PTR_ERR(power->acin_i) == -ENODEV)
 				return -EPROBE_DEFER;
diff --git a/drivers/power/supply/axp20x_battery.c b/drivers/power/supply/axp20x_battery.c
index fa27195f074e..152e556a4417 100644
--- a/drivers/power/supply/axp20x_battery.c
+++ b/drivers/power/supply/axp20x_battery.c
@@ -906,23 +906,23 @@ static const struct power_supply_desc axp717_batt_ps_desc = {
 static int axp209_bat_cfg_iio_channels(struct platform_device *pdev,
 				       struct axp20x_batt_ps *axp_batt)
 {
-	axp_batt->batt_v = devm_iio_channel_get(&pdev->dev, "batt_v");
+	axp_batt->batt_v = devm_iio_channel_get_sys(&pdev->dev, "batt_v");
 	if (IS_ERR(axp_batt->batt_v)) {
 		if (PTR_ERR(axp_batt->batt_v) == -ENODEV)
 			return -EPROBE_DEFER;
 		return PTR_ERR(axp_batt->batt_v);
 	}
 
-	axp_batt->batt_chrg_i = devm_iio_channel_get(&pdev->dev,
-							"batt_chrg_i");
+	axp_batt->batt_chrg_i = devm_iio_channel_get_sys(&pdev->dev,
+							 "batt_chrg_i");
 	if (IS_ERR(axp_batt->batt_chrg_i)) {
 		if (PTR_ERR(axp_batt->batt_chrg_i) == -ENODEV)
 			return -EPROBE_DEFER;
 		return PTR_ERR(axp_batt->batt_chrg_i);
 	}
 
-	axp_batt->batt_dischrg_i = devm_iio_channel_get(&pdev->dev,
-							   "batt_dischrg_i");
+	axp_batt->batt_dischrg_i = devm_iio_channel_get_sys(&pdev->dev,
+							    "batt_dischrg_i");
 	if (IS_ERR(axp_batt->batt_dischrg_i)) {
 		if (PTR_ERR(axp_batt->batt_dischrg_i) == -ENODEV)
 			return -EPROBE_DEFER;
@@ -935,15 +935,15 @@ static int axp209_bat_cfg_iio_channels(struct platform_device *pdev,
 static int axp717_bat_cfg_iio_channels(struct platform_device *pdev,
 				       struct axp20x_batt_ps *axp_batt)
 {
-	axp_batt->batt_v = devm_iio_channel_get(&pdev->dev, "batt_v");
+	axp_batt->batt_v = devm_iio_channel_get_sys(&pdev->dev, "batt_v");
 	if (IS_ERR(axp_batt->batt_v)) {
 		if (PTR_ERR(axp_batt->batt_v) == -ENODEV)
 			return -EPROBE_DEFER;
 		return PTR_ERR(axp_batt->batt_v);
 	}
 
-	axp_batt->batt_chrg_i = devm_iio_channel_get(&pdev->dev,
-							"batt_chrg_i");
+	axp_batt->batt_chrg_i = devm_iio_channel_get_sys(&pdev->dev,
+							 "batt_chrg_i");
 	if (IS_ERR(axp_batt->batt_chrg_i)) {
 		if (PTR_ERR(axp_batt->batt_chrg_i) == -ENODEV)
 			return -EPROBE_DEFER;
diff --git a/drivers/power/supply/axp20x_usb_power.c b/drivers/power/supply/axp20x_usb_power.c
index 9722912268fe..c82f05cef379 100644
--- a/drivers/power/supply/axp20x_usb_power.c
+++ b/drivers/power/supply/axp20x_usb_power.c
@@ -577,14 +577,14 @@ static int axp717_usb_power_prop_writeable(struct power_supply *psy,
 static int axp20x_configure_iio_channels(struct platform_device *pdev,
 					 struct axp20x_usb_power *power)
 {
-	power->vbus_v = devm_iio_channel_get(&pdev->dev, "vbus_v");
+	power->vbus_v = devm_iio_channel_get_sys(&pdev->dev, "vbus_v");
 	if (IS_ERR(power->vbus_v)) {
 		if (PTR_ERR(power->vbus_v) == -ENODEV)
 			return -EPROBE_DEFER;
 		return PTR_ERR(power->vbus_v);
 	}
 
-	power->vbus_i = devm_iio_channel_get(&pdev->dev, "vbus_i");
+	power->vbus_i = devm_iio_channel_get_sys(&pdev->dev, "vbus_i");
 	if (IS_ERR(power->vbus_i)) {
 		if (PTR_ERR(power->vbus_i) == -ENODEV)
 			return -EPROBE_DEFER;
@@ -597,7 +597,7 @@ static int axp20x_configure_iio_channels(struct platform_device *pdev,
 static int axp717_configure_iio_channels(struct platform_device *pdev,
 					 struct axp20x_usb_power *power)
 {
-	power->vbus_v = devm_iio_channel_get(&pdev->dev, "vbus_v");
+	power->vbus_v = devm_iio_channel_get_sys(&pdev->dev, "vbus_v");
 	if (IS_ERR(power->vbus_v)) {
 		if (PTR_ERR(power->vbus_v) == -ENODEV)
 			return -EPROBE_DEFER;
-- 
2.43.0

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

[Jonathan Cameron]

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(-)
> 

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

[Jonathan Cameron]

On Wed, 11 Dec 2024 21:58:26 +0000
Jonathan Cameron <jic23@kernel.org> wrote:

> 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.

(note this driver is a little newer than that era, but used that old infrastructure
none the less)

> 
> 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.
> 

+CC Sasha.  Whilst looking for the problem patch, I noticed that fix
has gone into stable.  If this is a common problem, we may want to drop
that briefly until we have a fix in place for this side effect as
the original patch fixed a configuration that had (I think?) never worked,
but broke ones that did.  I've no idea how common the devices are in
each configuration!

Jonathan


> 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(-)
> >   
> 

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

[Chris Morgan]

On Wed, Dec 11, 2024 at 09:58:26PM +0000, Jonathan Cameron wrote:
> 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.

Is there a way to figure out the device number at runtime? The issue is
each time the device attempts to probe and fails, the device number
increments, making it a "hitting a moving target" problem.

Thank you,
Chris

> 
> 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(-)
> > 
> 

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

[Chen-Yu Tsai]

On Tue, Dec 17, 2024 at 2:01 AM Chris Morgan <macroalpha82@gmail.com> wrote:
>
> On Wed, Dec 11, 2024 at 09:58:26PM +0000, Jonathan Cameron wrote:
> > 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.
>
> Is there a way to figure out the device number at runtime? The issue is
> each time the device attempts to probe and fails, the device number
> increments, making it a "hitting a moving target" problem.

The ADC device is a mfd cell or child device of the PMIC mfd device.
So you should be able to use dev->parent to get it directly? We do
that at least for the regulator driver.

ChenYu

> Thank you,
> Chris
>
> >
> > 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(-)
> > >
> >

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

[Chris Morgan]

On Tue, Dec 17, 2024 at 02:04:37AM +0800, Chen-Yu Tsai wrote:
> On Tue, Dec 17, 2024 at 2:01 AM Chris Morgan <macroalpha82@gmail.com> wrote:
> >
> > On Wed, Dec 11, 2024 at 09:58:26PM +0000, Jonathan Cameron wrote:
> > > 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.
> >
> > Is there a way to figure out the device number at runtime? The issue is
> > each time the device attempts to probe and fails, the device number
> > increments, making it a "hitting a moving target" problem.
> 
> The ADC device is a mfd cell or child device of the PMIC mfd device.
> So you should be able to use dev->parent to get it directly? We do
> that at least for the regulator driver.

Sorry to dig up such an old thread. I'm taking a look at this one again
and can confirm passing pdev->dev.parent to devm_iio_channel_get() is
insufficient to get the driver to find the correct ADC channel. Would
there be another/better way to do this other than the
devm_iio_channel_get_sys() as proposed? Or should we be walking the
parent somehow looking for the named ADC channel on each child device
(which would also require a new symbol most likely)?

Thank you,
Chris

> 
> ChenYu
> 
> > Thank you,
> > Chris
> >
> > >
> > > 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(-)
> > > >
> > >

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

[Jonathan Cameron]

On Tue, 4 Mar 2025 12:18:10 -0600
Chris Morgan <macroalpha82@gmail.com> wrote:

> On Tue, Dec 17, 2024 at 02:04:37AM +0800, Chen-Yu Tsai wrote:
> > On Tue, Dec 17, 2024 at 2:01 AM Chris Morgan <macroalpha82@gmail.com> wrote:  
> > >
> > > On Wed, Dec 11, 2024 at 09:58:26PM +0000, Jonathan Cameron wrote:  
> > > > 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.  
> > >
> > > Is there a way to figure out the device number at runtime? The issue is
> > > each time the device attempts to probe and fails, the device number
> > > increments, making it a "hitting a moving target" problem.  
> > 
> > The ADC device is a mfd cell or child device of the PMIC mfd device.
> > So you should be able to use dev->parent to get it directly? We do
> > that at least for the regulator driver.  
> 
> Sorry to dig up such an old thread. I'm taking a look at this one again
> and can confirm passing pdev->dev.parent to devm_iio_channel_get() is
> insufficient to get the driver to find the correct ADC channel. Would
> there be another/better way to do this other than the
> devm_iio_channel_get_sys() as proposed? Or should we be walking the
> parent somehow looking for the named ADC channel on each child device
> (which would also require a new symbol most likely)?

Hi Chris

From what I recall the point was to get the number from pdev->dev.parent
rather than simply passing it to the devm_iio_channel_get() call.

That should allow building of channel maps that provide the correct
device name to allow the provider to be found.

Jonathan

> 
> Thank you,
> Chris
> 
> > 
> > ChenYu
> >   
> > > Thank you,
> > > Chris
> > >  
> > > >
> > > > 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(-)
> > > > >  
> > > >  
> 

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

[Chris Morgan]

On Tue, Mar 04, 2025 at 11:29:13PM +0000, Jonathan Cameron wrote:
> On Tue, 4 Mar 2025 12:18:10 -0600
> Chris Morgan <macroalpha82@gmail.com> wrote:
> 
> > On Tue, Dec 17, 2024 at 02:04:37AM +0800, Chen-Yu Tsai wrote:
> > > On Tue, Dec 17, 2024 at 2:01 AM Chris Morgan <macroalpha82@gmail.com> wrote:  
> > > >
> > > > On Wed, Dec 11, 2024 at 09:58:26PM +0000, Jonathan Cameron wrote:  
> > > > > 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.  
> > > >
> > > > Is there a way to figure out the device number at runtime? The issue is
> > > > each time the device attempts to probe and fails, the device number
> > > > increments, making it a "hitting a moving target" problem.  
> > > 
> > > The ADC device is a mfd cell or child device of the PMIC mfd device.
> > > So you should be able to use dev->parent to get it directly? We do
> > > that at least for the regulator driver.  
> > 
> > Sorry to dig up such an old thread. I'm taking a look at this one again
> > and can confirm passing pdev->dev.parent to devm_iio_channel_get() is
> > insufficient to get the driver to find the correct ADC channel. Would
> > there be another/better way to do this other than the
> > devm_iio_channel_get_sys() as proposed? Or should we be walking the
> > parent somehow looking for the named ADC channel on each child device
> > (which would also require a new symbol most likely)?
> 
> Hi Chris
> 
> From what I recall the point was to get the number from pdev->dev.parent
> rather than simply passing it to the devm_iio_channel_get() call.
> 
> That should allow building of channel maps that provide the correct
> device name to allow the provider to be found.
> 
> Jonathan

I don't suppose you have a few more tips, do you? I'm still kind of
lost. I was able to crawl the parent node until I gathered the correct
device struct pointer of the adc device, but even passing it to the
devm_iio_channel_get() still doesn't work. I'm thinking that maybe it's
because devm_iio_channel_get() then calls iio_channel_get() which then
calls fwnode_iio_channel_get_by_name() which then calls
__fwnode_iio_channel_get_by_name() which fails because there is no 
"io-channel-names" property in the device tree (because the driver hard
codes the channel names). Now a call to iio_channel_get_sys() would
work for me in this case, but I'm starting to wonder how it might work
if we had more than one PMIC (which is the whole point of what the
original fix and revert was trying to solve) so we might have to figure
out a more elegant solution..

Thank you,
Chris

> 
> > 
> > Thank you,
> > Chris
> > 
> > > 
> > > ChenYu
> > >   
> > > > Thank you,
> > > > Chris
> > > >  
> > > > >
> > > > > 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(-)
> > > > > >  
> > > > >  
> > 
> 

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

[Jonathan Cameron]

On Wed, 5 Mar 2025 20:19:43 -0600
Chris Morgan <macromorgan@hotmail.com> wrote:

> On Tue, Mar 04, 2025 at 11:29:13PM +0000, Jonathan Cameron wrote:
> > On Tue, 4 Mar 2025 12:18:10 -0600
> > Chris Morgan <macroalpha82@gmail.com> wrote:
> >   
> > > On Tue, Dec 17, 2024 at 02:04:37AM +0800, Chen-Yu Tsai wrote:  
> > > > On Tue, Dec 17, 2024 at 2:01 AM Chris Morgan <macroalpha82@gmail.com> wrote:    
> > > > >
> > > > > On Wed, Dec 11, 2024 at 09:58:26PM +0000, Jonathan Cameron wrote:    
> > > > > > 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.    
> > > > >
> > > > > Is there a way to figure out the device number at runtime? The issue is
> > > > > each time the device attempts to probe and fails, the device number
> > > > > increments, making it a "hitting a moving target" problem.    
> > > > 
> > > > The ADC device is a mfd cell or child device of the PMIC mfd device.
> > > > So you should be able to use dev->parent to get it directly? We do
> > > > that at least for the regulator driver.    
> > > 
> > > Sorry to dig up such an old thread. I'm taking a look at this one again
> > > and can confirm passing pdev->dev.parent to devm_iio_channel_get() is
> > > insufficient to get the driver to find the correct ADC channel. Would
> > > there be another/better way to do this other than the
> > > devm_iio_channel_get_sys() as proposed? Or should we be walking the
> > > parent somehow looking for the named ADC channel on each child device
> > > (which would also require a new symbol most likely)?  
> > 
> > Hi Chris
> > 
> > From what I recall the point was to get the number from pdev->dev.parent
> > rather than simply passing it to the devm_iio_channel_get() call.
> > 
> > That should allow building of channel maps that provide the correct
> > device name to allow the provider to be found.
> > 
> > Jonathan  
> 
> I don't suppose you have a few more tips, do you? I'm still kind of
> lost. I was able to crawl the parent node until I gathered the correct
> device struct pointer of the adc device, but even passing it to the
> devm_iio_channel_get() still doesn't work. I'm thinking that maybe it's
> because devm_iio_channel_get() then calls iio_channel_get() which then
> calls fwnode_iio_channel_get_by_name() which then calls
> __fwnode_iio_channel_get_by_name() which fails because there is no 
> "io-channel-names" property in the device tree (because the driver hard
> codes the channel names). Now a call to iio_channel_get_sys() would
> work for me in this case, but I'm starting to wonder how it might work
> if we had more than one PMIC (which is the whole point of what the
> original fix and revert was trying to solve) so we might have to figure
> out a more elegant solution..
> 
This isn't about doing things directly with the parent, but rather modifying the
map at creation time to contain the correct naming for the consumers.

So look at the existing struct iio_map arrays in the driver. You are going
to need to do a kmem_dup() to get your own copy to edit.  Then look up
the naming of the various child nodes via the parent (e.g. the mfd).
That will use device_for_each_child() to find a match to first part
of name and then the ID (which is available on the platform_device
structure).  Key is the naming will come from;
https://elixir.bootlin.com/linux/v6.13.5/source/drivers/base/platform.c#L686
(I think anyway) which is built up using the original node name
and the id (+ a suffix of .auto).

You can then fill those into the struct iio_map array copy.
Ugly corner is that we need the adc mfd child to be registered last
but that we should be able to manage.

Looking back at the thread, the issue you refer to on devices being
renumbered is a pain though.  without thinking that much about it,
that renaming feels like a bug to me as I'd really expect device
naming of the children to be static after the mfd has loaded.
Or does this renumbering only happen if the mfd probe fails?
If it's that case it won't matter for this work around.

Hope that gives you an idea.

The whole things is ugly but it is hard to set up the equivalent
of device tree pnode handles without the infrastructure so we
tend to end up with names like this code.
I'd be happy to see something else generic but not sure what it
would look like.

Jonathan

> Thank you,
> Chris
> 
> >   
> > > 
> > > Thank you,
> > > Chris
> > >   
> > > > 
> > > > ChenYu
> > > >     
> > > > > Thank you,
> > > > > Chris
> > > > >    
> > > > > >
> > > > > > 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(-)
> > > > > > >    
> > > > > >    
> > >   
> >