Message-ID: <20241024-adventurous-imaginary-hornet-4d5c46-mkl@pengutronix.de>
Date: Thu, 24 Oct 2024 17:20:57 +0200
From: Marc Kleine-Budde <mkl@...gutronix.de>
To: Ming Yu <a0282524688@...il.com>
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

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 *?

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

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

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

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

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

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

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

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

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

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)

Powered by blists - more mailing lists