| Message ID | 20241210224859.58917-1-macroalpha82@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Fix Regression with AXP20X for 6.13-rc1 | expand |
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(-) >
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(-) > > >
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(-) > > >
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(-) > > > > >
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(-)