Re: [PATCH v12 1/7] mfd: Add core driver for Nuvoton NCT6694

[Lee Jones <lee@kernel.org>]

[...]

> > > > > > In the code above you register 6 I2C devices.  Each device will be
> > > > > > assigned a platform ID 0 through 5. The .probe() function in the I2C
> > > > > > driver will be executed 6 times.  In each of those calls to .probe(),
> > > > > > instead of pre-allocating a contiguous assignment of IDs here, you
> > > > > > should be able to use IDA in .probe() to allocate those same device IDs
> > > > > > 0 through 5.
> > > > > >
> > > > > > What am I missing here?
> > > > > >
> > > > >
> > > > > You're absolutely right in the scenario where a single NCT6694 device
> > > > > is present. However, I’m wondering how we should handle the case where
> > > > > a second or even third NCT6694 device is bound to the same MFD driver.
> > > > > In that situation, the sub-drivers using a static IDA will continue
> > > > > allocating increasing IDs, rather than restarting from 0 for each
> > > > > device. How should this be handled?
> > > >
> > > > I'd like to see the implementation of this before advising.
> > > >
> > > > In such a case, I assume there would be a differentiating factor between
> > > > the two (or three) devices.  You would then use that to decide which IDA
> > > > would need to be incremented.
> > > >
> > > > However, Greg is correct.  Hard-coding look-ups for userspace to use
> > > > sounds like a terrible idea.
> > > >
> > >
> > > I understand.
> > > Do you think it would be better to pass the index via platform_data
> > > and use PLATFORM_DEVID_AUTO together with mfd_add_hotplug_devices()
> > > instead?
> > > For example:
> > > struct nct6694_platform_data {
> > >     int index;
> > > };
> > >
> > > static struct nct6694_platform_data i2c_data[] = {
> > >     { .index = 0 }, { .index = 1 }, { .index = 2 }, { .index = 3 }, {
> > > .index = 4 }, { .index = 5 },
> > > };
> > >
> > > static const struct mfd_cell nct6694_devs[] = {
> > >     MFD_CELL_BASIC("nct6694-i2c", NULL, &i2c_data[0], sizeof(struct
> > > nct6694_platform_data), 0),
> > >     MFD_CELL_BASIC("nct6694-i2c", NULL, &i2c_data[1], sizeof(struct
> > > nct6694_platform_data), 0),
> > >     MFD_CELL_BASIC("nct6694-i2c", NULL, &i2c_data[2], sizeof(struct
> > > nct6694_platform_data), 0),
> > >     MFD_CELL_BASIC("nct6694-i2c", NULL, &i2c_data[3], sizeof(struct
> > > nct6694_platform_data), 0),
> > >     MFD_CELL_BASIC("nct6694-i2c", NULL, &i2c_data[4], sizeof(struct
> > > nct6694_platform_data), 0),
> > >     MFD_CELL_BASIC("nct6694-i2c", NULL, &i2c_data[5], sizeof(struct
> > > nct6694_platform_data), 0),
> > > };
> > > ...
> > > mfd_add_hotplug_devices(dev, nct6694_devs, ARRAY_SIZE(nct6694_devs));
> > > ...
> >
> > No, that's clearly way worse.  =:-)
> >
> > The clean-up that this provides is probably not worth all of this
> > discussion.  I _still_ think this enumeration should be done in the
> > driver.  But if you really can't make it work, I'll accept the .id
> > patch.
> >
> 
> Okay, I would like to ask for your advice regarding the implementation of IDA.
> 
> Using a global IDA in the sub-driver like this:
> (in i2c-nct6694.c)
> static DEFINE_IDA(nct6694_i2c_ida);
> 
> static int nct6694_i2c_probe(struct platform_device *pdev)
> {
>     ida_alloc(&nct6694_i2c_ida, GFP_KERNEL);
>     ...
> }
> 
> causes IDs to be globally incremented across all devices. For example,
> the first NCT6694 device gets probed 6 times and receives IDs 0–5, but
> when a second NCT6694 device is added, it receives IDs starting from
> 6, rather than starting again from 0. This makes per-device ID mapping
> unreliable.
> 
> To solve this, I believe the right approach is to have each NCT6694
> instance maintain its own IDA, managed by the MFD driver's private
> data. As mentioned earlier, for example:
> (in nct6694.c)
> struct nct6694 {
>     struct device *dev;
>     struct ida i2c_ida;
> };
> 
> static int nct6694_probe(struct platform_device *pdev)
> {
>     ...
>     ida_init(&nct6694->i2c_ida);
>     ...
> }
> 
> (in i2c-nct6694.c)
> static int nct6694_i2c_probe(struct platform_device *pdev)
> {
>     id = ida_alloc(&nct6694->i2c_ida, GFP_KERNEL);
> }
> 
> This way, each device allocates IDs independently, and each set of
> I2C/GPIO instances gets predictable IDs starting from 0 per device. I
> think this resolves the original issue without relying on hardcoded
> platform IDs.
> Please let me know if this implementation aligns with what you had in mind.

This sounds like an acceptable way forward.

-- 
Lee Jones [李琼斯]