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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <20250320145042.GS3890718@google.com>
Date: Thu, 20 Mar 2025 14:50:42 +0000
From: Lee Jones <lee@...nel.org>
To: Ming Yu <a0282524688@...il.com>
Cc: tmyu0@...oton.com, 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
Subject: Re: [PATCH v8 1/7] mfd: Add core driver for Nuvoton NCT6694

On Mon, 17 Mar 2025, Ming Yu wrote:

> Dear Lee,
> 
> Thank you for reviewing,
> 
> Lee Jones <lee@...nel.org> 於 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@...il.com>
> >
> > 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@...oton.com>
> > > +L:   linux-kernel@...r.kernel.org
> >
> > 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@...il.com>
> > >  L:   linux-fbdev@...r.kernel.org

[...]

> > > +     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 [李琼斯]