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

[Ming Yu <a0282524688@gmail.com>]

Dear Lee,

Thank you for reviewing.
I will fix the code which you mentioned.

Additionally, I will revert the mfd_cell changes to match version 10. (i.e.,
static const struct mfd_cell nct6694_dev[] = {
    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),
    ...
}; ), since I found that using devm_mfd_add_devices() with id field
set to 0 meets my needs. This allows me to use pdev->id in the
sub-drivers.


Best regards,
Ming

Lee Jones <lee@kernel.org> 於 2025年5月23日 週五 下午5:02寫道：
>
> On Tue, 20 May 2025, a0282524688@gmail.com wrote:
>
> > From: Ming Yu <tmyu0@nuvoton.com>
> >
> > The Nuvoton NCT6694 provides an USB interface to the host to
> > access its features.
> >
> > Sub-devices 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 <tmyu0@nuvoton.com>
> > ---
> >
> > Changes since version 10:
> > - Add change log for the patch
> > - Fix mfd_cell to MFD_CELL_NAME()
> > - Remove unnecessary blank line
> >
> > Changes since version 9:
> > - Add KernelDoc to exported functions
> >
> > Changes since version 8:
> > - Modify the signed-off-by with my work address
> > - Rename all MFD cell names to "nct6694-xxx"
> > - Fix some comments in nct6694.c and in nct6694.h
> >
> > Changes since version 7:
> > - Add error handling for devm_mutex_init()
> >
> > Changes since version 6:
> >
> > Changes since version 5:
> > - Fix mfd_cell to MFD_CELL_NAME() and MFD_CELL_BASIC()
> > - Drop unnecessary macros
> >
> > Changes since version 4:
> > - Modify arguments in read/write function to a pointer to cmd_header
> >
> > Changes since version 3:
> > - Modify array buffer to structure
> > - Fix defines and comments
> > - Add header <linux/bits.h> and use BIT macro
> > - Modify mutex_init() to devm_mutex_init()
> >
> > Changes since version 2:
> >
> > Changes since version 1:
> > - Implement IRQ domain to handle IRQ demux
> > - Modify USB_DEVICE to USB_DEVICE_AND_INTERFACE_INFO API
> > - Add command structure
> > - Fix USB functions
> > - Sort each driver's header files alphabetically
> >
> >  MAINTAINERS                 |   6 +
> >  drivers/mfd/Kconfig         |  15 ++
> >  drivers/mfd/Makefile        |   2 +
> >  drivers/mfd/nct6694.c       | 387 ++++++++++++++++++++++++++++++++++++
> >  include/linux/mfd/nct6694.h |  98 +++++++++
> >  5 files changed, 508 insertions(+)
> >  create mode 100644 drivers/mfd/nct6694.c
> >  create mode 100644 include/linux/mfd/nct6694.h
>
> [...]
>
> I was just going to fix this up for you when I applied the set, but
> seeing as it looks like you have to re-submit anyway ...
>
> > +static const struct usb_device_id nct6694_ids[] = {
> > +     { USB_DEVICE_AND_INTERFACE_INFO(NCT6694_VENDOR_ID, NCT6694_PRODUCT_ID, 0xFF, 0x00, 0x00)},
>
> You need a space before the '}'.
>
> > +     {}
> > +};
> > +MODULE_DEVICE_TABLE(usb, nct6694_ids);
> > +
> > +static struct usb_driver nct6694_usb_driver = {
> > +     .name           = "nct6694",
> > +     .id_table       = nct6694_ids,
> > +     .probe          = nct6694_usb_probe,
> > +     .disconnect     = nct6694_usb_disconnect,
> > +};
> > +module_usb_driver(nct6694_usb_driver);
> > +
> > +MODULE_DESCRIPTION("Nuvoton NCT6694 core driver");
> > +MODULE_AUTHOR("Ming Yu <tmyu0@nuvoton.com>");
> > +MODULE_LICENSE("GPL");
> > diff --git a/include/linux/mfd/nct6694.h b/include/linux/mfd/nct6694.h
> > new file mode 100644
> > index 000000000000..5e172609be3f
> > --- /dev/null
> > +++ b/include/linux/mfd/nct6694.h
> > @@ -0,0 +1,98 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * Copyright (C) 2025 Nuvoton Technology Corp.
> > + *
> > + * Nuvoton NCT6694 USB transaction and data structure.
> > + */
> > +
> > +#ifndef __MFD_NCT6694_H
> > +#define __MFD_NCT6694_H
> > +
> > +#define NCT6694_VENDOR_ID    0x0416
> > +#define NCT6694_PRODUCT_ID   0x200B
> > +#define NCT6694_INT_IN_EP    0x81
> > +#define NCT6694_BULK_IN_EP   0x02
> > +#define NCT6694_BULK_OUT_EP  0x03
> > +
> > +#define NCT6694_HCTRL_SET    0x40
> > +#define NCT6694_HCTRL_GET    0x80
> > +
> > +#define NCT6694_URB_TIMEOUT  1000
> > +
> > +enum nct6694_irq_id {
> > +     NCT6694_IRQ_GPIO0 = 0,
> > +     NCT6694_IRQ_GPIO1,
> > +     NCT6694_IRQ_GPIO2,
> > +     NCT6694_IRQ_GPIO3,
> > +     NCT6694_IRQ_GPIO4,
> > +     NCT6694_IRQ_GPIO5,
> > +     NCT6694_IRQ_GPIO6,
> > +     NCT6694_IRQ_GPIO7,
> > +     NCT6694_IRQ_GPIO8,
> > +     NCT6694_IRQ_GPIO9,
> > +     NCT6694_IRQ_GPIOA,
> > +     NCT6694_IRQ_GPIOB,
> > +     NCT6694_IRQ_GPIOC,
> > +     NCT6694_IRQ_GPIOD,
> > +     NCT6694_IRQ_GPIOE,
> > +     NCT6694_IRQ_GPIOF,
> > +     NCT6694_IRQ_CAN0,
> > +     NCT6694_IRQ_CAN1,
> > +     NCT6694_IRQ_RTC,
> > +     NCT6694_NR_IRQS,
> > +};
> > +
> > +enum nct6694_response_err_status {
> > +     NCT6694_NO_ERROR = 0,
> > +     NCT6694_FORMAT_ERROR,
> > +     NCT6694_RESERVED1,
> > +     NCT6694_RESERVED2,
> > +     NCT6694_NOT_SUPPORT_ERROR,
> > +     NCT6694_NO_RESPONSE_ERROR,
> > +     NCT6694_TIMEOUT_ERROR,
> > +     NCT6694_PENDING,
> > +};
> > +
> > +struct __packed nct6694_cmd_header {
> > +     u8 rsv1;
> > +     u8 mod;
> > +     union __packed {
> > +             __le16 offset;
> > +             struct __packed {
> > +                     u8 cmd;
> > +                     u8 sel;
> > +             };
> > +     };
> > +     u8 hctrl;
> > +     u8 rsv2;
> > +     __le16 len;
> > +};
> > +
> > +struct __packed nct6694_response_header {
> > +     u8 sequence_id;
> > +     u8 sts;
> > +     u8 reserved[4];
> > +     __le16 len;
> > +};
> > +
> > +union __packed nct6694_usb_msg {
> > +     struct nct6694_cmd_header cmd_header;
> > +     struct nct6694_response_header response_header;
> > +};
> > +
> > +struct nct6694 {
> > +     struct device *dev;
> > +     struct irq_domain *domain;
> > +     struct mutex access_lock;
> > +     struct mutex irq_lock;
> > +     struct urb *int_in_urb;
> > +     struct usb_device *udev;
> > +     union nct6694_usb_msg *usb_msg;
> > +     unsigned char *int_buffer;
> > +     unsigned int irq_enable;
> > +};
> > +
> > +int nct6694_read_msg(struct nct6694 *nct6694, const struct nct6694_cmd_header *cmd_hd, void *buf);
> > +int nct6694_write_msg(struct nct6694 *nct6694, const struct nct6694_cmd_header *cmd_hd, void *buf);
> > +
> > +#endif
> > --
> > 2.34.1
> >
>
> --
> Lee Jones [李琼斯]