Message-ID: <CAOoeyxVuu-kKoQa84mGOX=thAc0hnzQU8L=MnycoRRhzoZMnNw@mail.gmail.com>
Date: Wed, 25 Jun 2025 22:24:13 +0800
From: Ming Yu <a0282524688@...il.com>
To: Lee Jones <lee@...nel.org>
Cc: linus.walleij@...aro.org, brgl@...ev.pl, andi.shyti@...nel.org, 
	mkl@...gutronix.de, mailhol.vincent@...adoo.fr, andrew+netdev@...n.ch, 
	davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org, pabeni@...hat.com, 
	wim@...ux-watchdog.org, linux@...ck-us.net, jdelvare@...e.com, 
	alexandre.belloni@...tlin.com, linux-kernel@...r.kernel.org, 
	linux-gpio@...r.kernel.org, linux-i2c@...r.kernel.org, 
	linux-can@...r.kernel.org, netdev@...r.kernel.org, 
	linux-watchdog@...r.kernel.org, linux-hwmon@...r.kernel.org, 
	linux-rtc@...r.kernel.org, linux-usb@...r.kernel.org, 
	Ming Yu <tmyu0@...oton.com>
Subject: Re: [PATCH v12 1/7] mfd: Add core driver for Nuvoton NCT6694

Lee Jones <lee@...nel.org> 於 2025年6月25日 週三 下午9:46寫道：
>
> On Wed, 25 Jun 2025, Ming Yu wrote:
>
> > Dear Greg and Lee,
> >
> > Thank you for your comments.
> > I've reviewed your suggestions, but would appreciate your feedback on
> > a few remaining points.
> >
> > Lee Jones <lee@...nel.org> 於 2025年6月25日 週三 下午5:01寫道：
> > >
> > > On Fri, 20 Jun 2025, Ming Yu wrote:
> > >
> > > > Lee Jones <lee@...nel.org> 於 2025年6月19日 週四 下午11:28寫道：
> > > > >
> > > > > On Thu, 19 Jun 2025, Ming Yu wrote:
> > > > >
> > > > > > Lee Jones <lee@...nel.org> 於 2025年6月19日 週四 下午7:53寫道：
> > > > > > >
> > > > > > > On Fri, 13 Jun 2025, Ming Yu wrote:
> > > > > > >
> > > > > > > > Lee Jones <lee@...nel.org> 於 2025年6月13日 週五 下午9:11寫道：
> > > > > > > > >
> > > > > > > > > On Fri, 13 Jun 2025, Ming Yu wrote:
> > > > > > > > >
> > > > > > > > > > Lee Jones <lee@...nel.org> 於 2025年6月12日 週四 下午11:23寫道：
> > > > > > > > > > >
> > > > > > > > > > > On Thu, 12 Jun 2025, Ming Yu wrote:
> > > > > > > > > > >
> > > > > > > > > > > > Dear Lee,
> > > > > > > > > > > >
> > > > > > > > > > > > Thank you for reviewing,
> > > > > > > > > > > >
> > > > > > > > > > > > Lee Jones <lee@...nel.org> 於 2025年6月12日 週四 下午10:00寫道：
> > > > > > > > > > > > >
> > > > > > > > > > > > ...
> > > > > > > > > > > > > > +static const struct mfd_cell nct6694_devs[] = {
> > > > > > > > > > > > > > +     MFD_CELL_BASIC("nct6694-gpio", NULL, NULL, 0, 0),
> > > > > > > > > > > > > > +     MFD_CELL_BASIC("nct6694-gpio", NULL, NULL, 0, 1),
> > > > > > > > > > > > > > +     MFD_CELL_BASIC("nct6694-gpio", NULL, NULL, 0, 2),
> > > > > > > > > > > > > > +     MFD_CELL_BASIC("nct6694-gpio", NULL, NULL, 0, 3),
> > > > > > > > > > > > > > +     MFD_CELL_BASIC("nct6694-gpio", NULL, NULL, 0, 4),
> > > > > > > > > > > > > > +     MFD_CELL_BASIC("nct6694-gpio", NULL, NULL, 0, 5),
> > > > > > > > > > > > > > +     MFD_CELL_BASIC("nct6694-gpio", NULL, NULL, 0, 6),
> > > > > > > > > > > > > > +     MFD_CELL_BASIC("nct6694-gpio", NULL, NULL, 0, 7),
> > > > > > > > > > > > > > +     MFD_CELL_BASIC("nct6694-gpio", NULL, NULL, 0, 8),
> > > > > > > > > > > > > > +     MFD_CELL_BASIC("nct6694-gpio", NULL, NULL, 0, 9),
> > > > > > > > > > > > > > +     MFD_CELL_BASIC("nct6694-gpio", NULL, NULL, 0, 10),
> > > > > > > > > > > > > > +     MFD_CELL_BASIC("nct6694-gpio", NULL, NULL, 0, 11),
> > > > > > > > > > > > > > +     MFD_CELL_BASIC("nct6694-gpio", NULL, NULL, 0, 12),
> > > > > > > > > > > > > > +     MFD_CELL_BASIC("nct6694-gpio", NULL, NULL, 0, 13),
> > > > > > > > > > > > > > +     MFD_CELL_BASIC("nct6694-gpio", NULL, NULL, 0, 14),
> > > > > > > > > > > > > > +     MFD_CELL_BASIC("nct6694-gpio", NULL, NULL, 0, 15),
> > > > > > > > > > > > > > +
> > > > > > > > > > > > > > +     MFD_CELL_BASIC("nct6694-i2c", NULL, NULL, 0, 0),
> > > > > > > > > > > > > > +     MFD_CELL_BASIC("nct6694-i2c", NULL, NULL, 0, 1),
> > > > > > > > > > > > > > +     MFD_CELL_BASIC("nct6694-i2c", NULL, NULL, 0, 2),
> > > > > > > > > > > > > > +     MFD_CELL_BASIC("nct6694-i2c", NULL, NULL, 0, 3),
> > > > > > > > > > > > > > +     MFD_CELL_BASIC("nct6694-i2c", NULL, NULL, 0, 4),
> > > > > > > > > > > > > > +     MFD_CELL_BASIC("nct6694-i2c", NULL, NULL, 0, 5),
> > > > > > > > > > > > >
> > > > > > > > > > > > > Why have we gone back to this silly numbering scheme?
> > > > > > > > > > > > >
> > > > > > > > > > > > > What happened to using IDA in the child driver?
> > > > > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > > > > In a previous version, I tried to maintain a static IDA in each
> > > > > > > > > > > > sub-driver. However, I didn’t consider the case where multiple NCT6694
> > > > > > > > > > > > devices are bound to the same driver — in that case, the IDs are not
> > > > > > > > > > > > fixed and become unusable for my purpose.
> > > > > > > > > > >
> > > > > > > > > > > Not sure I understand.
> > > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > As far as I know, if I maintain the IDA in the sub-drivers and use
> > > > > > > > > > multiple MFD_CELL_NAME("nct6694-gpio") entries in the MFD, the first
> > > > > > > > > > NCT6694 device bound to the GPIO driver will receive IDs 0~15.
> > > > > > > > > > However, when a second NCT6694 device is connected to the system, it
> > > > > > > > > > will receive IDs 16~31.
> > > > > > > > > > Because of this behavior, I switched back to using platform_device->id.
> > > > > > > > >
> > > > > > > > > Each of the devices will probe once.
> > > > > > > > >
> > > > > > > > > The first one will be given 0, the second will be given 1, etc.
> > > > > > > > >
> > > > > > > > > Why would you give multiple IDs to a single device bound to a driver?
> > > > > > > > >
> > > > > > > >
> > > > > > > > The device exposes multiple peripherals — 16 GPIO controllers, 6 I2C
> > > > > > > > adapters, 2 CAN FD controllers, and 2 watchdog timers. Each peripheral
> > > > > > > > is independently addressable, has its own register region, and can
> > > > > > > > operate in isolation. The IDs are used to distinguish between these
> > > > > > > > instances.
> > > > > > > > For example, the GPIO driver will be probed 16 times, allocating 16
> > > > > > > > separate gpio_chip instances to control 8 GPIO lines each.
> > > > > > > >
> > > > > > > > If another device binds to this driver, it is expected to expose
> > > > > > > > peripherals with the same structure and behavior.
> > > > > > >
> > > > > > > I still don't see why having a per-device IDA wouldn't render each
> > > > > > > probed device with its own ID.  Just as you have above.
> > > > > > >
> > > > > >
> > > > > > For example, when the MFD driver and the I2C sub-driver are loaded,
> > > > > > connecting the first NCT6694 USB device to the system results in 6
> > > > > > nct6694-i2c platform devices being created and bound to the
> > > > > > i2c-nct6694 driver. These devices receive IDs 0 through 5 via the IDA.
> > > > > >
> > > > > > However, when a second NCT6694 USB device is connected, its
> > > > > > corresponding nct6694-i2c platform devices receive IDs 6 through 11 —
> > > > > > instead of 0 through 5 as I originally expected.
> > > > > >
> > > > > > If I've misunderstood something, please feel free to correct me. Thank you!
> > > > >
> > > > > 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.


Thanks,
Ming

Powered by blists - more mailing lists