On Wed, 5 Mar 2025 20:19:43 -0600
Chris Morgan <macromorgan@xxxxxxxxxxx> 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@xxxxxxxxx> 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@xxxxxxxxx> 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@xxxxxxxxx> wrote:
> > > > > >
> > > > > > > From: Chris Morgan <macromorgan@xxxxxxxxxxx>
> > > > > > >
> > > > > > > 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(-)
> > > > > > >
> > > > > >
> > >
> >
