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

[Lee Jones <lee@xxxxxxxxxx>]

On Mon, 17 Mar 2025, Ming Yu wrote:

> Dear Lee,
> 
> Thank you for reviewing,
> 
> Lee Jones <lee@xxxxxxxxxx> 於 2025年3月7日 週五 上午9:15寫道：
> >
> > On Tue, 25 Feb 2025, Ming Yu wrote:
> >
> > > The Nuvoton NCT6694 is a peripheral expander with 16 GPIO chips,
> > > 6 I2C controllers, 2 CANfd controllers, 2 Watchdog timers, ADC,
> > > PWM, and RTC.
> >
> > This needs to go into the Kconfig help passage.
> >
> 
> Okay, I will move these to Kconfig in the next patch.
> 
> > > This driver implements USB device functionality and shares the
> > > chip's peripherals as a child device.
> >
> > This driver doesn't implement USB functionality.
> >
> 
> Fix it in v9.
> 
> > > Each child device can use the USB functions nct6694_read_msg()
> > > and nct6694_write_msg() to issue a command. They can also request
> > > interrupt that will be called when the USB device receives its
> > > interrupt pipe.
> > >
> > > Signed-off-by: Ming Yu <a0282524688@xxxxxxxxx>
> >
> > Why aren't you signing off with your work address?
> >
> 
> Fix it in v9.
> 
> > > ---
> > >  MAINTAINERS                 |   7 +
> > >  drivers/mfd/Kconfig         |  18 ++
> > >  drivers/mfd/Makefile        |   2 +
> > >  drivers/mfd/nct6694.c       | 378 ++++++++++++++++++++++++++++++++++++
> > >  include/linux/mfd/nct6694.h | 102 ++++++++++
> > >  5 files changed, 507 insertions(+)
> > >  create mode 100644 drivers/mfd/nct6694.c
> > >  create mode 100644 include/linux/mfd/nct6694.h
> > >
> > > diff --git a/MAINTAINERS b/MAINTAINERS
> > > index 873aa2cce4d7..c700a0b96960 100644
> > > --- a/MAINTAINERS
> > > +++ b/MAINTAINERS
> > > @@ -16918,6 +16918,13 @@ F:   drivers/nubus/
> > >  F:   include/linux/nubus.h
> > >  F:   include/uapi/linux/nubus.h
> > >
> > > +NUVOTON NCT6694 MFD DRIVER
> > > +M:   Ming Yu <tmyu0@xxxxxxxxxxx>
> > > +L:   linux-kernel@xxxxxxxxxxxxxxx
> >
> > This is the default list.  You shouldn't need to add that here.
> 
> Remove it in v9.

Please snip everything that you agree with.

> > > +S:   Supported
> > > +F:   drivers/mfd/nct6694.c
> > > +F:   include/linux/mfd/nct6694.h
> > > +
> > >  NVIDIA (rivafb and nvidiafb) FRAMEBUFFER DRIVER
> > >  M:   Antonino Daplas <adaplas@xxxxxxxxx>
> > >  L:   linux-fbdev@xxxxxxxxxxxxxxx

[...]

> > > +     MFD_CELL_BASIC("gpio-nct6694", NULL, NULL, 0, 0x1),
> >
> > IDs are usually given in base-10.
> >
> 
> Fix it in v9.
> 
> > Why are you manually adding the device IDs?
> >
> > PLATFORM_DEVID_AUTO doesn't work for you?
> >
> 
> I need to manage these IDs to ensure that child devices can be
> properly utilized within their respective modules.

How?  Please explain.

This numbering looks sequential and arbitrary.

What does PLATFORM_DEVID_AUTO do differently such that it is not useful?

> 
> > > +     MFD_CELL_BASIC("gpio-nct6694", NULL, NULL, 0, 0x2),
> > > +     MFD_CELL_BASIC("gpio-nct6694", NULL, NULL, 0, 0x3),
> > > +     MFD_CELL_BASIC("gpio-nct6694", NULL, NULL, 0, 0x4),
> > > +     MFD_CELL_BASIC("gpio-nct6694", NULL, NULL, 0, 0x5),
> > > +     MFD_CELL_BASIC("gpio-nct6694", NULL, NULL, 0, 0x6),
> > > +     MFD_CELL_BASIC("gpio-nct6694", NULL, NULL, 0, 0x7),
> > > +     MFD_CELL_BASIC("gpio-nct6694", NULL, NULL, 0, 0x8),
> > > +     MFD_CELL_BASIC("gpio-nct6694", NULL, NULL, 0, 0x9),
> > > +     MFD_CELL_BASIC("gpio-nct6694", NULL, NULL, 0, 0xA),
> > > +     MFD_CELL_BASIC("gpio-nct6694", NULL, NULL, 0, 0xB),
> > > +     MFD_CELL_BASIC("gpio-nct6694", NULL, NULL, 0, 0xC),
> > > +     MFD_CELL_BASIC("gpio-nct6694", NULL, NULL, 0, 0xD),
> > > +     MFD_CELL_BASIC("gpio-nct6694", NULL, NULL, 0, 0xE),
> > > +     MFD_CELL_BASIC("gpio-nct6694", NULL, NULL, 0, 0xF),

> > > +
> > > +     MFD_CELL_BASIC("i2c-nct6694", NULL, NULL, 0, 0x0),
> > > +     MFD_CELL_BASIC("i2c-nct6694", NULL, NULL, 0, 0x1),
> > > +     MFD_CELL_BASIC("i2c-nct6694", NULL, NULL, 0, 0x2),
> > > +     MFD_CELL_BASIC("i2c-nct6694", NULL, NULL, 0, 0x3),
> > > +     MFD_CELL_BASIC("i2c-nct6694", NULL, NULL, 0, 0x4),
> > > +     MFD_CELL_BASIC("i2c-nct6694", NULL, NULL, 0, 0x5),
> > > +
> > > +     MFD_CELL_BASIC("nct6694_canfd", NULL, NULL, 0, 0x0),
> >
> > Why has the naming convention changed here?
> >
> 
> I originally expected the child devices name to directly match its
> driver name. Do you think it would be better to standardize the naming
> as "nct6694-xxx" ?

Yes, that is the usual procedure.

> > > +     MFD_CELL_BASIC("nct6694_canfd", NULL, NULL, 0, 0x1),
> > > +
> > > +     MFD_CELL_BASIC("nct6694_wdt", NULL, NULL, 0, 0x0),
> > > +     MFD_CELL_BASIC("nct6694_wdt", NULL, NULL, 0, 0x1),
> > > +
> > > +     MFD_CELL_NAME("nct6694-hwmon"),
> > > +     MFD_CELL_NAME("rtc-nct6694"),
> >
> > There doesn't seem to be any consistency here.
> >
> 
> Do you think these two should be changed to use MFD_CELL_BASIC()?

No.  I mean with the device nomenclature.

[...]

> > > +static void usb_int_callback(struct urb *urb)
> > > +{
> > > +     struct nct6694 *nct6694 = urb->context;
> > > +     unsigned int *int_status = urb->transfer_buffer;
> > > +     int ret;
> > > +
> > > +     switch (urb->status) {
> > > +     case 0:
> > > +             break;
> > > +     case -ECONNRESET:
> > > +     case -ENOENT:
> > > +     case -ESHUTDOWN:
> > > +             return;
> > > +     default:
> > > +             generic_handle_irq_safe(irq_find_mapping(nct6694->domain, irq));
> > > +             *int_status &= ~BIT(irq);
> > > +     }
> > > +
> > > +resubmit:
> > > +     ret = usb_submit_urb(urb, GFP_ATOMIC);
> > > +     if (ret)
> > > +             dev_dbg(nct6694->dev, "%s: Failed to resubmit urb, status %pe",
> >
> > Why debug?
> >
> 
> Excuse me, do you think it should change to dev_err()?

Probably a dev_warn() since you are not propagating the error.

Is this okay by the way?  Is it okay to fail?

-- 
Lee Jones [李琼斯]