Message-ID: <CAOoeyxUhnyYG3p+DQJG-tvU5vc5WYQZLLqCXW=uPcXTjq2gVfw@mail.gmail.com>
Date: Fri, 25 Oct 2024 16:08:10 +0800
From: Ming Yu <a0282524688@...il.com>
To: Marc Kleine-Budde <mkl@...gutronix.de>
Cc: tmyu0@...oton.com, lee@...nel.org, linus.walleij@...aro.org, brgl@...ev.pl, 
	andi.shyti@...nel.org, 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, 
	jic23@...nel.org, lars@...afoo.de, ukleinek@...nel.org, 
	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-iio@...r.kernel.org, linux-pwm@...r.kernel.org, 
	linux-rtc@...r.kernel.org
Subject: Re: [PATCH v1 1/9] mfd: Add core driver for Nuvoton NCT6694

Sorry, resending this email in plain text format.

Marc Kleine-Budde <mkl@...gutronix.de> 於 2024年10月24日 週四 下午11:21寫道：
>
> On 24.10.2024 16:59:14, 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 driver implements USB device functionality and shares the
> > chip's peripherals as a child device.
> >
> > Each child device can use the USB functions nct6694_read_msg()
> > and nct6694_write_msg() to issue a command. They can also register
> > a handler function that will be called when the USB device receives
> > its interrupt pipe.
>
> [...]
>
> > diff --git a/drivers/mfd/nct6694.c b/drivers/mfd/nct6694.c
> > new file mode 100644
> > index 000000000000..9838c7be0b98
> > --- /dev/null
> > +++ b/drivers/mfd/nct6694.c
>
> [...]
>
> > +int nct6694_read_msg(struct nct6694 *nct6694, u8 mod, u16 offset, u16 length,
> > +                  u8 rd_idx, u8 rd_len, unsigned char *buf)
>
> why not make buf a void *?

[Ming] I'll change the type in the next patch.

>
> > +{
> > +     struct usb_device *udev = nct6694->udev;
> > +     unsigned char err_status;
> > +     int len, packet_len, tx_len, rx_len;
> > +     int i, ret;
> > +
> > +     mutex_lock(&nct6694->access_lock);
> > +
> > +     /* Send command packet to USB device */
> > +     nct6694->cmd_buffer[REQUEST_MOD_IDX] = mod;
> > +     nct6694->cmd_buffer[REQUEST_CMD_IDX] = offset & 0xFF;
> > +     nct6694->cmd_buffer[REQUEST_SEL_IDX] = (offset >> 8) & 0xFF;
> > +     nct6694->cmd_buffer[REQUEST_HCTRL_IDX] = HCTRL_GET;
> > +     nct6694->cmd_buffer[REQUEST_LEN_L_IDX] = length & 0xFF;
> > +     nct6694->cmd_buffer[REQUEST_LEN_H_IDX] = (length >> 8) & 0xFF;
> > +
> > +     ret = usb_bulk_msg(udev, usb_sndbulkpipe(udev, BULK_OUT_ENDPOINT),
> > +                        nct6694->cmd_buffer, CMD_PACKET_SZ, &tx_len,
> > +                        nct6694->timeout);
> > +     if (ret)
> > +             goto err;
> > +
> > +     /* Receive response packet from USB device */
> > +     ret = usb_bulk_msg(udev, usb_rcvbulkpipe(udev, BULK_IN_ENDPOINT),
> > +                        nct6694->rx_buffer, CMD_PACKET_SZ, &rx_len,
> > +                        nct6694->timeout);
> > +     if (ret)
> > +             goto err;
> > +
> > +     err_status = nct6694->rx_buffer[RESPONSE_STS_IDX];
> > +
> > +     /*
> > +      * Segmented reception of messages that exceed the size of USB bulk
> > +      * pipe packets.
> > +      */
>
> The Linux USB stack can receive bulk messages longer than the max packet size.

[Ming] Since NCT6694's bulk pipe endpoint size is 128 bytes for this MFD device.
The core will divide packet 256 bytes for high speed USB device, but
it is exceeds
the hardware limitation, so I am dividing it manually.

>
> > +     for (i = 0, len = length; len > 0; i++, len -= packet_len) {
> > +             if (len > nct6694->maxp)
> > +                     packet_len = nct6694->maxp;
> > +             else
> > +                     packet_len = len;
> > +
> > +             ret = usb_bulk_msg(udev, usb_rcvbulkpipe(udev, BULK_IN_ENDPOINT),
> > +                                nct6694->rx_buffer + nct6694->maxp * i,
> > +                                packet_len, &rx_len, nct6694->timeout);
> > +             if (ret)
> > +                     goto err;
> > +     }
> > +
> > +     for (i = 0; i < rd_len; i++)
> > +             buf[i] = nct6694->rx_buffer[i + rd_idx];
>
> memcpy()?
>
> Or why don't you directly receive data into the provided buffer? Copying
> of the data doesn't make it faster.
>
> On the other hand, receiving directly into the target buffer means the
> target buffer must not live on the stack.

[Ming] Okay! I'll change it to memcpy().
This is my perspective: the data is uniformly received by the rx_bffer held
by the MFD device. does it need to be changed?

>
> > +
> > +     if (err_status) {
> > +             pr_debug("%s: MSG CH status = %2Xh\n", __func__, err_status);
> > +             ret = -EIO;
> > +     }
> > +
> > +err:
> > +     mutex_unlock(&nct6694->access_lock);
> > +     return ret;
> > +}
> > +EXPORT_SYMBOL(nct6694_read_msg);
> > +
> > +int nct6694_write_msg(struct nct6694 *nct6694, u8 mod, u16 offset,
> > +                   u16 length, unsigned char *buf)
> > +{
> > +     struct usb_device *udev = nct6694->udev;
> > +     unsigned char err_status;
> > +     int len, packet_len, tx_len, rx_len;
> > +     int i, ret;
> > +
> > +     mutex_lock(&nct6694->access_lock);
> > +
> > +     /* Send command packet to USB device  */
> > +     nct6694->cmd_buffer[REQUEST_MOD_IDX] = mod;
> > +     nct6694->cmd_buffer[REQUEST_CMD_IDX] = offset & 0xFF;
> > +     nct6694->cmd_buffer[REQUEST_SEL_IDX] = (offset >> 8) & 0xFF;
> > +     nct6694->cmd_buffer[REQUEST_HCTRL_IDX] = HCTRL_SET;
> > +     nct6694->cmd_buffer[REQUEST_LEN_L_IDX] = length & 0xFF;
> > +     nct6694->cmd_buffer[REQUEST_LEN_H_IDX] = (length >> 8) & 0xFF;
>
> What about creating a struct that describes the cmd_buffer layout?

[Ming] I've thought about this before, thanks for your comments.

>
> > +
> > +     ret = usb_bulk_msg(udev, usb_sndbulkpipe(udev, BULK_OUT_ENDPOINT),
> > +                        nct6694->cmd_buffer, CMD_PACKET_SZ, &tx_len,
> > +                        nct6694->timeout);
> > +     if (ret)
> > +             goto err;
> > +
> > +     /*
> > +      * Segmented transmission of messages that exceed the size of USB bulk
> > +      * pipe packets.
> > +      */
>
> same as above
>
> > +     for (i = 0, len = length; len > 0; i++, len -= packet_len) {
> > +             if (len > nct6694->maxp)
> > +                     packet_len = nct6694->maxp;
> > +             else
> > +                     packet_len = len;
> > +
> > +             memcpy(nct6694->tx_buffer + nct6694->maxp * i,
> > +                    buf + nct6694->maxp * i, packet_len);
> > +
> > +             ret = usb_bulk_msg(udev, usb_sndbulkpipe(udev, BULK_OUT_ENDPOINT),
> > +                                nct6694->tx_buffer + nct6694->maxp * i,
> > +                                packet_len, &tx_len, nct6694->timeout);
> > +             if (ret)
> > +                     goto err;
> > +     }
> > +
> > +     /* Receive response packet from USB device */
> > +     ret = usb_bulk_msg(udev, usb_rcvbulkpipe(udev, BULK_IN_ENDPOINT),
> > +                        nct6694->rx_buffer, CMD_PACKET_SZ, &rx_len,
> > +                        nct6694->timeout);
> > +     if (ret)
> > +             goto err;
> > +
> > +     err_status = nct6694->rx_buffer[RESPONSE_STS_IDX];
> > +
> > +     /*
> > +      * Segmented reception of messages that exceed the size of USB bulk
> > +      * pipe packets.
> > +      */
>
> same as above
>
> > +     for (i = 0, len = length; len > 0; i++, len -= packet_len) {
> > +             if (len > nct6694->maxp)
> > +                     packet_len = nct6694->maxp;
> > +             else
> > +                     packet_len = len;
> > +
> > +             ret = usb_bulk_msg(udev, usb_rcvbulkpipe(udev, BULK_IN_ENDPOINT),
> > +                                nct6694->rx_buffer + nct6694->maxp * i,
> > +                                packet_len, &rx_len, nct6694->timeout);
> > +             if (ret)
> > +                     goto err;
> > +     }
> > +
> > +     memcpy(buf, nct6694->rx_buffer, length);
>
> why not rx into the destination buffer directly?
>
> > +
> > +     if (err_status) {
> > +             pr_debug("%s: MSG CH status = %2Xh\n", __func__, err_status);
> > +             ret = -EIO;
> > +     }
> > +
> > +err:
> > +     mutex_unlock(&nct6694->access_lock);
> > +     return ret;
> > +}
> > +EXPORT_SYMBOL(nct6694_write_msg);
>
> [...]
>
> > +static int nct6694_usb_probe(struct usb_interface *iface,
> > +                          const struct usb_device_id *id)
> > +{
> > +     struct usb_device *udev = interface_to_usbdev(iface);
> > +     struct device *dev = &udev->dev;
> > +     struct usb_host_interface *interface;
> > +     struct usb_endpoint_descriptor *int_endpoint;
> > +     struct nct6694 *nct6694;
> > +     int pipe, maxp, bulk_pipe;
> > +     int ret = EINVAL;
> > +
> > +     interface = iface->cur_altsetting;
> > +     /* Binding interface class : 0xFF */
> > +     if (interface->desc.bInterfaceClass != USB_CLASS_VENDOR_SPEC ||
> > +         interface->desc.bInterfaceSubClass != 0x00 ||
> > +         interface->desc.bInterfaceProtocol != 0x00)
> > +             return -ENODEV;
> > +
> > +     int_endpoint = &interface->endpoint[0].desc;
> > +     if (!usb_endpoint_is_int_in(int_endpoint))
> > +             return -ENODEV;
> > +
> > +     nct6694 = devm_kzalloc(&udev->dev, sizeof(*nct6694), GFP_KERNEL);
> > +     if (!nct6694)
> > +             return -ENOMEM;
> > +
> > +     pipe = usb_rcvintpipe(udev, INT_IN_ENDPOINT);
> > +     maxp = usb_maxpacket(udev, pipe);
>
> better move these 2 down to the usb_fill_int_urb().

[Ming] Okay! I'll move these in the next patch.

>
> > +
> > +     nct6694->cmd_buffer = devm_kcalloc(dev, CMD_PACKET_SZ,
> > +                                        sizeof(unsigned char), GFP_KERNEL);
> > +     if (!nct6694->cmd_buffer)
> > +             return -ENOMEM;
> > +     nct6694->rx_buffer = devm_kcalloc(dev, MAX_PACKET_SZ,
> > +                                       sizeof(unsigned char), GFP_KERNEL);
> > +     if (!nct6694->rx_buffer)
> > +             return -ENOMEM;
> > +     nct6694->tx_buffer = devm_kcalloc(dev, MAX_PACKET_SZ,
> > +                                       sizeof(unsigned char), GFP_KERNEL);
> > +     if (!nct6694->tx_buffer)
> > +             return -ENOMEM;
> > +     nct6694->int_buffer = devm_kcalloc(dev, MAX_PACKET_SZ,
> > +                                        sizeof(unsigned char), GFP_KERNEL);
> > +     if (!nct6694->int_buffer)
> > +             return -ENOMEM;
> > +
> > +     nct6694->int_in_urb = usb_alloc_urb(0, GFP_KERNEL);
> > +     if (!nct6694->int_in_urb) {
> > +             dev_err(&udev->dev, "Failed to allocate INT-in urb!\n");
> > +             return -ENOMEM;
> > +     }
> > +
> > +     /* Bulk pipe maximum packet for each transaction */
> > +     bulk_pipe = usb_sndbulkpipe(udev, BULK_OUT_ENDPOINT);
> > +     nct6694->maxp = usb_maxpacket(udev, bulk_pipe);
> > +
> > +     mutex_init(&nct6694->access_lock);
> > +     nct6694->udev = udev;
> > +     nct6694->timeout = URB_TIMEOUT; /* Wait until urb complete */
> > +
> > +     INIT_LIST_HEAD(&nct6694->handler_list);
> > +     spin_lock_init(&nct6694->lock);
> > +
> > +     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;
> > +
> > +     dev_set_drvdata(&udev->dev, nct6694);
> > +     usb_set_intfdata(iface, nct6694);
> > +
> > +     ret = mfd_add_hotplug_devices(&udev->dev, nct6694_dev,
> > +                                   ARRAY_SIZE(nct6694_dev));
> > +     if (ret) {
> > +             dev_err(&udev->dev, "Failed to add mfd's child device\n");
> > +             goto err_mfd;
> > +     }
> > +
> > +     nct6694->async_workqueue = alloc_ordered_workqueue("asyn_workqueue", 0);
>
> Where is the async_workqueue used?
>
> > +
> > +     dev_info(&udev->dev, "Probed device: (%04X:%04X)\n",
> > +              id->idVendor, id->idProduct);
> > +     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 usb_device *udev = interface_to_usbdev(iface);
> > +     struct nct6694 *nct6694 = usb_get_intfdata(iface);
> > +
> > +     mfd_remove_devices(&udev->dev);
> > +     flush_workqueue(nct6694->async_workqueue);
> > +     destroy_workqueue(nct6694->async_workqueue);
> > +     usb_set_intfdata(iface, NULL);
>
> I think this is not needed.

[Ming] I'll remove it in the next patch.

>
> > +     usb_kill_urb(nct6694->int_in_urb);
> > +     usb_free_urb(nct6694->int_in_urb);
> > +}
> > +
> > +static const struct usb_device_id nct6694_ids[] = {
> > +     { USB_DEVICE(NCT6694_VENDOR_ID, NCT6694_PRODUCT_ID)},
> > +     {},
> > +};
> > +MODULE_DEVICE_TABLE(usb, nct6694_ids);
> > +
> > +static struct usb_driver nct6694_usb_driver = {
> > +     .name   = DRVNAME,
> > +     .id_table = nct6694_ids,
> > +     .probe = nct6694_usb_probe,
> > +     .disconnect = nct6694_usb_disconnect,
> > +};
> > +
> > +module_usb_driver(nct6694_usb_driver);
> > +
> > +MODULE_DESCRIPTION("USB-MFD driver for NCT6694");
> > +MODULE_AUTHOR("Ming Yu <tmyu0@...oton.com>");
> > +MODULE_LICENSE("GPL");
> > diff --git a/include/linux/mfd/nct6694.h b/include/linux/mfd/nct6694.h
> > new file mode 100644
> > index 000000000000..0797564363be
> > --- /dev/null
> > +++ b/include/linux/mfd/nct6694.h
> > @@ -0,0 +1,168 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * Nuvoton NCT6694 USB transaction and data structure.
> > + *
> > + * Copyright (C) 2024 Nuvoton Technology Corp.
> > + */
> > +
> > +#ifndef __MFD_NCT6694_H
> > +#define __MFD_NCT6694_H
> > +
> > +#define NCT6694_DEV_GPIO             "nct6694-gpio"
> > +#define NCT6694_DEV_I2C                      "nct6694-i2c"
> > +#define NCT6694_DEV_CAN                      "nct6694-can"
> > +#define NCT6694_DEV_WDT                      "nct6694-wdt"
> > +#define NCT6694_DEV_IIO                      "nct6694-iio"
> > +#define NCT6694_DEV_HWMON            "nct6694-hwmon"
> > +#define NCT6694_DEV_PWM                      "nct6694-pwm"
> > +#define NCT6694_DEV_RTC                      "nct6694-rtc"
> > +
> > +#define NCT6694_VENDOR_ID            0x0416
> > +#define NCT6694_PRODUCT_ID           0x200B
> > +#define INT_IN_ENDPOINT                      0x81
> > +#define BULK_IN_ENDPOINT             0x82
>
> In Linux we don't add the 0x80 for IN endpoint, the framework does this
> for you.

[Ming] I'll change it in the next patch.

>
> > +#define BULK_OUT_ENDPOINT            0x03
> > +#define MAX_PACKET_SZ                        0x100
> > +
> > +#define CMD_PACKET_SZ                        0x8
> > +#define HCTRL_SET                    0x40
> > +#define HCTRL_GET                    0x80
> > +
> > +#define REQUEST_MOD_IDX                      0x01
> > +#define REQUEST_CMD_IDX                      0x02
> > +#define REQUEST_SEL_IDX                      0x03
> > +#define REQUEST_HCTRL_IDX            0x04
> > +#define REQUEST_LEN_L_IDX            0x06
> > +#define REQUEST_LEN_H_IDX            0x07
> > +
> > +#define RESPONSE_STS_IDX             0x01
> > +
> > +#define INT_IN_IRQ_IDX                       0x00
> > +#define GPIO_IRQ_STATUS                      BIT(0)
> > +#define CAN_IRQ_STATUS                       BIT(2)
> > +#define RTC_IRQ_STATUS                       BIT(3)
> > +
> > +#define URB_TIMEOUT                  1000
> > +
> > +/*
> > + * struct nct6694 - Nuvoton NCT6694 structure
> > + *
> > + * @udev: Pointer to the USB device
> > + * @int_in_urb: Interrupt pipe urb
> > + * @access_lock: USB transaction lock
> > + * @handler_list: List of registered handlers
> > + * @async_workqueue: Workqueue of processing asynchronous work
> > + * @tx_buffer: USB write message buffer
> > + * @rx_buffer: USB read message buffer
> > + * @cmd_buffer: USB send command message buffer
> > + * @int_buffer: USB receive interrupt message buffer
> > + * @lock: Handlers lock
> > + * @timeout: URB timeout
> > + * @maxp: Maximum packet of bulk pipe
> > + */
> > +struct nct6694 {
> > +     struct usb_device *udev;
> > +     struct urb *int_in_urb;
> > +     struct list_head handler_list;
> > +     struct workqueue_struct *async_workqueue;
> > +
> > +     /* Make sure that every USB transaction is not interrupted */
> > +     struct mutex access_lock;
> > +
> > +     unsigned char *tx_buffer;
> > +     unsigned char *rx_buffer;
> > +     unsigned char *cmd_buffer;
> > +     unsigned char *int_buffer;
> > +
> > +     /* Prevent races within handlers */
> > +     spinlock_t lock;
> > +
> > +     /* time in msec to wait for the urb to the complete */
> > +     long timeout;
> > +
> > +     /* Bulk pipe maximum packet for each transaction */
> > +     int maxp;
> > +};
> > +
> > +/*
> > + * struct nct6694_handler_entry - Stores the interrupt handling information
> > + * for each registered peripheral
> > + *
> > + * @irq_bit: The bit in irq_status[INT_IN_IRQ_IDX] representing interrupt
>                     ^^^
>
> I think this comment could be more precise, you can emphasize, that it's
> not the bit number but the bit mask.

[Ming] Okay! I'll change it in the next patch.

>
> > + * @handler: Function pointer to the interrupt handler of the peripheral
> > + * @private_data: Private data specific to the peripheral driver
> > + * @list: Node used to link to the handler_list
> > + */
> > +struct nct6694_handler_entry {
> > +     int irq_bit;
>
> the int_status you compare against in the IRQ callback ist a unsigned
> char. Better make all a u8.

[Ming] Okay! I'll change it in the next patch.

>
> > +     void (*handler)(void *private_data);
> > +     void *private_data;
> > +     struct list_head list;
> > +};
>
> regards,
> Marc
>
> --
> Pengutronix e.K.                 | Marc Kleine-Budde          |
> Embedded Linux                   | https://www.pengutronix.de |
> Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
> Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

Powered by blists - more mailing lists