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] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAOoeyxUgiTqtSksfHopEDhZHwNkUq9+d-ojo8ma3PX2dosuwyQ@mail.gmail.com>
Date: Mon, 17 Mar 2025 10:57:09 +0800
From: Ming Yu <a0282524688@...il.com>
To: Lee Jones <lee@...nel.org>
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

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.

> > +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
> > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> > index 6b0682af6e32..c97a2bdcea0b 100644
> > --- a/drivers/mfd/Kconfig
> > +++ b/drivers/mfd/Kconfig
> > @@ -1045,6 +1045,24 @@ config MFD_MENF21BMC
> >         This driver can also be built as a module. If so the module
> >         will be called menf21bmc.
> >
> > +config MFD_NCT6694
> > +     tristate "Nuvoton NCT6694 support"
> > +     select MFD_CORE
> > +     depends on USB
> > +     help
> > +       This enables support for the Nuvoton USB device NCT6694, which shares
> > +       peripherals.
> > +
> > +       This driver provides core APIs to access the NCT6694 hardware
> > +       monitoring and control features.
> > +
> > +       The NCT6694 is a versatile multi-function device that supports
>
> Please drop the term multi-function device and replace it what a proper
> description of the devices.
>

Fix it in v9.

> > +       functionalities such as GPIO, I2C, CAN, WDT, HWMON, and RTC
> > +       management.
>
> All of these line breaks should be removed.
>

Fix these in v9.

> > +       Additional drivers must be enabled to utilize the specific
> > +       functionalities of the device.
> > +
> >  config MFD_OCELOT
> >       tristate "Microsemi Ocelot External Control Support"
> >       depends on SPI_MASTER
> > diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> > index 9220eaf7cf12..7725b732e265 100644
> > --- a/drivers/mfd/Makefile
> > +++ b/drivers/mfd/Makefile
> > @@ -121,6 +121,8 @@ obj-$(CONFIG_MFD_MC13XXX) += mc13xxx-core.o
> >  obj-$(CONFIG_MFD_MC13XXX_SPI)        += mc13xxx-spi.o
> >  obj-$(CONFIG_MFD_MC13XXX_I2C)        += mc13xxx-i2c.o
> >
> > +obj-$(CONFIG_MFD_NCT6694)    += nct6694.o
> > +
> >  obj-$(CONFIG_MFD_CORE)               += mfd-core.o
> >
> >  ocelot-soc-objs                      := ocelot-core.o ocelot-spi.o
> > diff --git a/drivers/mfd/nct6694.c b/drivers/mfd/nct6694.c
> > new file mode 100644
> > index 000000000000..c82457679ca6
> > --- /dev/null
> > +++ b/drivers/mfd/nct6694.c
> > @@ -0,0 +1,378 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Nuvoton NCT6694 core driver using USB interface to provide
> > + * access to the NCT6694 hardware monitoring and control features.
> > + *
> > + * The NCT6694 is a versatile multi-function device that supports
>
> Here too.
>

Fix it in v9.

> > + * functionalities such as GPIO, I2C, CAN, WDT, HWMON and RTC
> > + * management.
> > + *
> > + * Copyright (C) 2024 Nuvoton Technology Corp.
>
> This goes at the top.
>

Fix it in v9.

> > + */
> > +
> > +#include <linux/bits.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/irq.h>
> > +#include <linux/irqdomain.h>
> > +#include <linux/kernel.h>
> > +#include <linux/mfd/core.h>
> > +#include <linux/mfd/nct6694.h>
> > +#include <linux/module.h>
> > +#include <linux/slab.h>
> > +#include <linux/usb.h>
> > +
> > +static const struct mfd_cell nct6694_dev[] = {
> > +     MFD_CELL_BASIC("gpio-nct6694", NULL, NULL, 0, 0x0),
>
> "-gpio" usually goes on the end.
>

Fix it in v9.

> > +     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.

> > +     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" ?

> > +     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()?

> > +};
> > +
> > +static int nct6694_response_err_handling(struct nct6694 *nct6694,
>
> > +{
> > +     switch (err_status) {
> > +     case NCT6694_NO_ERROR:
> > +             return err_status;
>
> This is odd since you already know this will be 0.
>

Fix it in v9.

> > +     case NCT6694_NOT_SUPPORT_ERROR:
> > +             dev_warn(nct6694->dev, "Command is not supported!\n");
>
> Why not dev_err()?
>

Fix it in v9.

> > +             break;
> > +     case NCT6694_NO_RESPONSE_ERROR:
> > +             dev_warn(nct6694->dev, "Command received no response!\n");
> > +             break;
> > +     case NCT6694_TIMEOUT_ERROR:
> > +             dev_warn(nct6694->dev, "Command timed out!\n");
> > +             break;
> > +     case NCT6694_PENDING:
> > +             dev_warn(nct6694->dev, "Command is pending!\n");
>
> Is this an error?
>

Yes, I will fix it to dev_err() in the next patch.

> > +             break;
> > +     default:
> > +             return -EINVAL;
> > +     }
> > +
> > +     return -EIO;
> > +}
> > +
> > +int nct6694_read_msg(struct nct6694 *nct6694, const struct nct6694_cmd_header *cmd_hd, void *buf)
> > +{
> > +     union nct6694_usb_msg *msg = nct6694->usb_msg;
> > +     int tx_len, rx_len, ret;
> > +
> > +     guard(mutex)(&nct6694->access_lock);
> > +
> > +     /* Send command packet to USB device */
>
> This doesn't really describe the next 2 lines.
>
> Move it down?
>

Fix it in v9.

> > +     memcpy(&msg->cmd_header, cmd_hd, sizeof(*cmd_hd));
> > +     msg->cmd_header.hctrl = NCT6694_HCTRL_GET;
> > +
> > +     ret = usb_bulk_msg(nct6694->udev,
>
> Since you use nct6694->udev a bunch - sometimes twice in the same call,
> it might be nicer to pull it into it's own variable instead of
> dereferencing it all the time.
>

Fix it in v9.

> > +                        usb_sndbulkpipe(nct6694->udev, NCT6694_BULK_OUT_EP),
> > +                        &msg->cmd_header, sizeof(*msg), &tx_len,
> > +                        nct6694->timeout);
> > +     if (ret)
> > +             return ret;
> > +
> > +     /* Receive response packet from USB device */
> > +     ret = usb_bulk_msg(nct6694->udev,
> > +                        usb_rcvbulkpipe(nct6694->udev, NCT6694_BULK_IN_EP),
> > +                        &msg->response_header, sizeof(*msg), &rx_len,
>
> How can you read sizeof(*msg) Bytes (22?) into the smaller
> response_header (16?) attribute?
>

The size of (*msg) is the same as command_header and response_header (8 bytes).

> > +                        nct6694->timeout);
> > +     if (ret)
> > +             return ret;
> > +
> > +     /* Receive data packet from USB device */
> > +     ret = usb_bulk_msg(nct6694->udev,
> > +                        usb_rcvbulkpipe(nct6694->udev, NCT6694_BULK_IN_EP),
> > +                        buf, le16_to_cpu(cmd_hd->len), &rx_len,
> > +                        nct6694->timeout);
> > +     if (ret)
> > +             return ret;
> > +
> > +     if (rx_len != le16_to_cpu(cmd_hd->len)) {
> > +             dev_err(nct6694->dev, "Expected received length %d, but got %d\n",
> > +                     le16_to_cpu(cmd_hd->len), rx_len);
> > +             return -EIO;
> > +     }
> > +
> > +     return nct6694_response_err_handling(nct6694, msg->response_header.sts);
> > +}
> > +EXPORT_SYMBOL(nct6694_read_msg);
> > +
> > +int nct6694_write_msg(struct nct6694 *nct6694, const struct nct6694_cmd_header *cmd_hd, void *buf)
> > +{
> > +     union nct6694_usb_msg *msg = nct6694->usb_msg;
> > +     int tx_len, rx_len, ret;
> > +
> > +     guard(mutex)(&nct6694->access_lock);
> > +
> > +     /* Send command packet to USB device */
> > +     memcpy(&msg->cmd_header, cmd_hd, sizeof(*cmd_hd));
> > +     msg->cmd_header.hctrl = NCT6694_HCTRL_SET;
> > +
> > +     ret = usb_bulk_msg(nct6694->udev,
> > +                        usb_sndbulkpipe(nct6694->udev, NCT6694_BULK_OUT_EP),
> > +                        &msg->cmd_header, sizeof(*msg), &tx_len,
> > +                        nct6694->timeout);
> > +     if (ret)
> > +             return ret;
> > +
> > +     /* Send data packet to USB device */
> > +     ret = usb_bulk_msg(nct6694->udev,
> > +                        usb_sndbulkpipe(nct6694->udev, NCT6694_BULK_OUT_EP),
> > +                        buf, le16_to_cpu(cmd_hd->len), &tx_len,
> > +                        nct6694->timeout);
> > +     if (ret)
> > +             return ret;
> > +
> > +     /* Receive response packet from USB device */
> > +     ret = usb_bulk_msg(nct6694->udev,
> > +                        usb_rcvbulkpipe(nct6694->udev, NCT6694_BULK_IN_EP),
> > +                        &msg->response_header, sizeof(*msg), &rx_len,
> > +                        nct6694->timeout);
> > +     if (ret)
> > +             return ret;
> > +
> > +     /* Receive data packet from USB device */
> > +     ret = usb_bulk_msg(nct6694->udev,
> > +                        usb_rcvbulkpipe(nct6694->udev, NCT6694_BULK_IN_EP),
> > +                        buf, le16_to_cpu(cmd_hd->len), &rx_len,
> > +                        nct6694->timeout);
> > +     if (ret)
> > +             return ret;
> > +
> > +     if (rx_len != le16_to_cpu(cmd_hd->len)) {
> > +             dev_err(nct6694->dev, "Expected transmitted length %d, but got %d\n",
> > +                     le16_to_cpu(cmd_hd->len), rx_len);
> > +             return -EIO;
> > +     }
> > +
> > +     return nct6694_response_err_handling(nct6694, msg->response_header.sts);
> > +}
> > +EXPORT_SYMBOL(nct6694_write_msg);
> > +
> > +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()?

> > +                     __func__, ERR_PTR(ret));
>
> Remove the __func__ part.
>

Understood.

> > +}
> > +
...
> > +static int nct6694_usb_probe(struct usb_interface *iface,
> > +                          const struct usb_device_id *id)
> > +{
> > +     struct usb_device *udev = interface_to_usbdev(iface);
> > +     struct usb_endpoint_descriptor *int_endpoint;
> > +     struct usb_host_interface *interface;
> > +     struct device *dev = &iface->dev;
> > +     struct nct6694 *nct6694;
> > +     int pipe, maxp;
> > +     int ret;
> > +
> > +     nct6694 = devm_kzalloc(dev, sizeof(*nct6694), GFP_KERNEL);
> > +     if (!nct6694)
> > +             return -ENOMEM;
> > +
> > +     pipe = usb_rcvintpipe(udev, NCT6694_INT_IN_EP);
> > +     maxp = usb_maxpacket(udev, pipe);
> > +
> > +     nct6694->usb_msg = devm_kzalloc(dev, sizeof(union nct6694_usb_msg), GFP_KERNEL);
> > +     if (!nct6694->usb_msg)
> > +             return -ENOMEM;
> > +
> > +     nct6694->int_buffer = devm_kzalloc(dev, maxp, GFP_KERNEL);
> > +     if (!nct6694->int_buffer)
> > +             return -ENOMEM;
> > +
> > +     nct6694->int_in_urb = usb_alloc_urb(0, GFP_KERNEL);
> > +     if (!nct6694->int_in_urb)
> > +             return -ENOMEM;
> > +
> > +     nct6694->domain = irq_domain_add_simple(NULL, NCT6694_NR_IRQS, 0,
> > +                                             &nct6694_irq_domain_ops,
> > +                                             nct6694);
> > +     if (!nct6694->domain) {
> > +             ret = -ENODEV;
> > +             goto err_urb;
> > +     }
> > +
> > +     nct6694->dev = dev;
> > +     nct6694->udev = udev;
> > +     nct6694->timeout = NCT6694_URB_TIMEOUT; /* Wait until URB completes */
>
> No need to save this known value.
>

Understood, I will drop it in the next patch.

> > +     ret = devm_mutex_init(dev, &nct6694->access_lock);
> > +     if (ret)
> > +             goto err_urb;
> > +
> > +     ret = devm_mutex_init(dev, &nct6694->irq_lock);
> > +     if (ret)
> > +             goto err_urb;
> > +
> > +     interface = iface->cur_altsetting;
> > +     int_endpoint = &interface->endpoint[0].desc;
> > +     if (!usb_endpoint_is_int_in(int_endpoint)) {
> > +             ret = -ENODEV;
> > +             goto err_urb;
> > +     }
> > +     usb_fill_int_urb(nct6694->int_in_urb, udev, pipe,
> > +                      nct6694->int_buffer, maxp, usb_int_callback,
> > +                      nct6694, int_endpoint->bInterval);
> > +     ret = usb_submit_urb(nct6694->int_in_urb, GFP_KERNEL);
> > +     if (ret)
> > +             goto err_urb;
>
> Please unsquash these calls - space them out.
>

Fix these in v9.

> > +
> > +     usb_set_intfdata(iface, nct6694);
> > +
> > +     ret = mfd_add_hotplug_devices(dev, nct6694_dev, ARRAY_SIZE(nct6694_dev));
> > +     if (ret)
> > +             goto err_mfd;
> > +
> > +     return 0;
> > +
> > +err_mfd:
> > +     usb_kill_urb(nct6694->int_in_urb);
> > +err_urb:
> > +     usb_free_urb(nct6694->int_in_urb);
> > +     return ret;
> > +}
> > +
> > +static void nct6694_usb_disconnect(struct usb_interface *iface)
> > +{
> > +     struct nct6694 *nct6694 = usb_get_intfdata(iface);
> > +
> > +     mfd_remove_devices(nct6694->dev);
> > +     usb_kill_urb(nct6694->int_in_urb);
> > +     usb_free_urb(nct6694->int_in_urb);
> > +}
> > +
> > +static const struct usb_device_id nct6694_ids[] = {
> > +     { USB_DEVICE_AND_INTERFACE_INFO(NCT6694_VENDOR_ID,
> > +                                     NCT6694_PRODUCT_ID,
> > +                                     0xFF, 0x00, 0x00)},
>
> This should fit on one line.  You can use up to 100-chars.
>

Fix it in v9.

> > +     {}
> > +};
> > +MODULE_DEVICE_TABLE(usb, nct6694_ids);
> > +
> > +static struct usb_driver nct6694_usb_driver = {
> > +     .name   = "nct6694",
>
> Odd spaces.
>

Fix it in v9.

> > +     .id_table = nct6694_ids,
> > +     .probe = nct6694_usb_probe,
> > +     .disconnect = nct6694_usb_disconnect,
> > +};
> > +
>
> Remove this line.
>

Fix it in v9.

> > +module_usb_driver(nct6694_usb_driver);
> > +
> > +MODULE_DESCRIPTION("USB core driver for NCT6694");
>
> This is not a USB driver.
>

Fix it in v9.

> > +MODULE_AUTHOR("Ming Yu <tmyu0@...oton.com>");
>
> Different to SoB.
>

Fix it in v9.

> > +MODULE_LICENSE("GPL");
> > diff --git a/include/linux/mfd/nct6694.h b/include/linux/mfd/nct6694.h
> > new file mode 100644
> > index 000000000000..8171f975761e
> > --- /dev/null
> > +++ b/include/linux/mfd/nct6694.h
> > @@ -0,0 +1,102 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * Nuvoton NCT6694 USB transaction and data structure.
> > + *
> > + * Copyright (C) 2024 Nuvoton Technology Corp.
>
> At the top.
>

Fix it in v9.

> > + */
> > +


Best regards,
Ming

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ