Sorry, resending this email in plain text format.
Marc Kleine-Budde <mkl@xxxxxxxxxxxxxx> 於 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@xxxxxxxxxxx>");
> > +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 |
