Dear Vincent,
Thank you for your comments,
Vincent Mailhol <mailhol.vincent@xxxxxxxxxx> 於 2024年12月27日 週五 下午11:34寫道：
>
> +cc: linux-usb@xxxxxxxxxxxxxxx
>
...
> > +config MFD_NCT6694
> > + tristate "Nuvoton NCT6694 support"
> > + select MFD_CORE
> > + depends on USB
> > + help
> > + This adds support for Nuvoton USB device NCT6694 sharing peripherals
> > + This includes the USB devcie driver and core APIs.
> ^^^^^^
> device
>
Fix it in v5.
> > + Additional drivers must be enabled in order to use the functionality
> > + of the device.
> > +
> > config MFD_HI6421_PMIC
> > tristate "HiSilicon Hi6421 PMU/Codec IC"
> > depends on OF
> > diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> > index e057d6d6faef..9d0365ba6a26 100644
> > --- a/drivers/mfd/Makefile
> > +++ b/drivers/mfd/Makefile
> > @@ -117,6 +117,8 @@ obj-$(CONFIG_TWL6040_CORE) += twl6040.o
> >
> > obj-$(CONFIG_MFD_MX25_TSADC) += fsl-imx25-tsadc.o
> >
> > +obj-$(CONFIG_MFD_NCT6694) += nct6694.o
>
> Keep the alphabetic order. MFD_NCT6694 is after MFD_MC13XXX in the alphabet.
>
Fix it in v5.
> > obj-$(CONFIG_MFD_MC13XXX) += mc13xxx-core.o
> > obj-$(CONFIG_MFD_MC13XXX_SPI) += mc13xxx-spi.o
> > obj-$(CONFIG_MFD_MC13XXX_I2C) += mc13xxx-i2c.o
> > diff --git a/drivers/mfd/nct6694.c b/drivers/mfd/nct6694.c
> > new file mode 100644
> > index 000000000000..0f31489ef9fa
> > --- /dev/null
> > +++ b/drivers/mfd/nct6694.c
>
> If I understand correctly, your device is an USB device, so shouldn't it
> be under
>
> drivers/usb/mfd/nct6694.c
>
> ?
I understand, but there is no drivers/usb/mfd/ directory, I believe my
device is similar to dln2.c and viperboard.c, which is why I placed it
under drivers/mfd/
>
> At the moment, I see no USB maintainers in CC (this is why I added
> linux-usb myself). By putting it in the correct folder, the
> get_maintainers.pl will give you the correct list of persons to put in copy.
>
Okay, I will add CC to linux-usb from now on.
> The same comment applies to the other modules. For example, I would
> expect to see the CAN module under:
>
> drivers/net/can/usb/nct6694_canfd.c
>
Understood! I will move the can driver to drivers/net/can/usb/ in v5.
...
> > +static int nct6694_response_err_handling(struct nct6694 *nct6694,
> > + unsigned char err_status)
> > +{
> > + struct device *dev = &nct6694->udev->dev;
> > +
> > + switch (err_status) {
> > + case NCT6694_NO_ERROR:
> > + return err_status;
> > + case NCT6694_NOT_SUPPORT_ERROR:
> > + dev_dbg(dev, "%s: Command is not support!\n", __func__);
>
> Grammar: Command is not supported
>
Fix it in v5.
> > + break;
> > + case NCT6694_NO_RESPONSE_ERROR:
> > + dev_dbg(dev, "%s: Command is no response!\n", __func__);
>
> Not sure what you meant here. Maybe: command didn't get a response? But
> then, I do not see the nuance with the timeout.
>
The firmware engine returns an error response.
If it returns NO_RESPONSE_ERROR, it means the target device did not
respond(e.g., I2C slave NACK).
If it returns TIMEOUT_ERROR, it means the engine process the command timed out.
I will add the comments to describe these error status in v5.
> > + break;
> > + case NCT6694_TIMEOUT_ERROR:
> > + dev_dbg(dev, "%s: Command is timeout!\n", __func__);
>
> Grammar: Command timed out
Fix it in v5.
> > + break;
> > + case NCT6694_PENDING:
> > + dev_dbg(dev, "%s: Command is pending!\n", __func__);
> > + break;> + default:
> > + return -EINVAL;
> > + }
> > +
> > + return -EIO;
> > +}
> > +
> > +int nct6694_read_msg(struct nct6694 *nct6694, u8 mod, u16 offset,
> > + u16 length, void *buf)
> > +{
> > + union nct6694_usb_msg *msg = nct6694->usb_msg;
> > + int tx_len, rx_len, ret;
> > +
> > + guard(mutex)(&nct6694->access_lock);
> > +
> > + memset(msg, 0, sizeof(*msg));
> > +
> > + /* Send command packet to USB device */
> > + msg->cmd_header.mod = mod;
> > + msg->cmd_header.cmd = offset & 0xFF;
> > + msg->cmd_header.sel = (offset >> 8) & 0xFF;
>
> In the other modules, you have some macros to combine together the cmd
> and the sel (selector, I guess?). For example from nct6694_canfd.c:
>
> #define NCT6694_CAN_DELIVER(buf_cnt) \
> ((((buf_cnt) & 0xFF) << 8) | 0x10) /* CMD|SEL */
>
> And here, you split them again. So what was the point to combine those
> together in the first place?
>
Due to these two bytes may used to OFFSET in report channel for other
modules(gpio, hwmon), I will modify them below...
> Can't you just pass both the cmd and the sel as two separate argument?
> Those cmd and sel concatenation macros are too confusing.
>
> Also, if you are worried of having too many arguments in
> nct6694_read_msg(), you may just directly pass a pointer to a struct
> nct6694_cmd_header instead of all the arguments separately.
>
...
in mfd/nct6694.c
inline struct nct6694_cmd_header nct6694_init_cmd(u8 mod, u8 cmd, u8 sel,
u16 offset, u16 length)
{
struct nct6694_cmd_header header;
header.mod = mod;
header.cmd = cmd;
header.sel = sel;
header.offset = cpu_to_le16(offset);
header.len = cpu_to_le16(length);
return header;
}
EXPORT_SYMBOL(nct6694_init_cmd);
int nct6694_read_msg(struct nct6694 *nct6694, struct nct6694_cmd_header *header,
void *buf)
{
union nct6694_usb_msg *msg = nct6694->usb_msg;
...
msg->cmd_header.mod = header->mod;
msg->cmd_header.hctrl = NCT6694_HCTRL_GET;
msg->cmd_header.len = header->len;
if (msg->cmd_header.mod == 0xFF) {
msg->cmd_header.offset = header->offset;
} else {
msg->cmd_header.cmd = header->cmd;
msg->cmd_header.sel = header->sel;
}
...
}
(also apply to nct6694_write_msg)
in other drivers, for example: gpio-nct6694.c
struct nct6694_cmd_header cmd;
int ret;
guard(mutex)(&data->lock);
cmd = nct6694_init_cmd(NCT6694_GPIO_MOD, 0, 0,
NCT6694_GPO_DIR + data->group,
sizeof(data->reg_val));
ret = nct6694_read_msg(data->nct6694, &cmd, &data->reg_val);
if (ret < 0)
return ret;
Do you think this approach would be better?
> > + msg->cmd_header.hctrl = NCT6694_HCTRL_GET;
> > + msg->cmd_header.len = cpu_to_le16(length);
> > +
...
> > +int nct6694_write_msg(struct nct6694 *nct6694, u8 mod, u16 offset,
> > + u16 length, void *buf)
> > +{
> > + union nct6694_usb_msg *msg = nct6694->usb_msg;
> > + int tx_len, rx_len, ret;
> > +
> > + guard(mutex)(&nct6694->access_lock);
> > +
> > + memset(msg, 0, sizeof(*msg));
> > +
> > + /* Send command packet to USB device */
> > + msg->cmd_header.mod = mod;
> > + msg->cmd_header.cmd = offset & 0xFF;
> > + msg->cmd_header.sel = (offset >> 8) & 0xFF;
> > + msg->cmd_header.hctrl = NCT6694_HCTRL_SET;
> > + msg->cmd_header.len = cpu_to_le16(length);
> > +
> > + 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, length, &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, NCT6694_MAX_PACKET_SZ, &rx_len, nct6694->timeout);
>
> What if the object size of buf is smaller than NCT6694_MAX_PACKET_SZ?
>
I will fix it to le16_to_cpu(header->len) in the v5.
> > + if (ret)
> > + return ret;
> > +
> > + if (rx_len != length) {
> > + dev_dbg(&nct6694->udev->dev, "%s: Sent length is not match!\n",
> > + __func__);
> > + 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;
> > + struct device *dev = &nct6694->udev->dev;
> > + unsigned int *int_status = urb->transfer_buffer;
> > + int ret;
> > +
> > + switch (urb->status) {
> > + case 0:
> > + break;
> > + case -ECONNRESET:
> > + case -ENOENT:
> > + case -ESHUTDOWN:
> > + return;
> > + default:
> > + goto resubmit;
> > + }
> > +
> > + while (*int_status) {
> > + int irq = __ffs(*int_status);
> > +
> > + 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(dev, "%s: Failed to resubmit urb, status %d",
> > + __func__, ret);
>
> Print the error mnemotecnic instead of the error code:
>
> dev_dbg(dev, "%s: Failed to resubmit urb, status %pe",
> __func__, ERR_PTR(ret));
>
> (apply to other location where you print error).
>
Understood. Fix these in v5.
> > +}
> > +
...
> > + * nct6694_read_msg - Receive data from NCT6694 USB device
> > + *
> > + * @nct6694 - Nuvoton NCT6694 structure
> > + * @mod - Module byte
> > + * @offset - Offset byte or (Select byte | Command byte)
> > + * @length - Length byte
> > + * @buf - Read data from rx buffer
> > + *
> > + * USB Transaction format:
> > + *
> > + * OUT |RSV|MOD|CMD|SEL|HCTL|RSV|LEN_L|LEN_H|
> > + * OUT |SEQ|STS|RSV|RSV|RSV|RSV||LEN_L|LEN_H|
> > + * IN |-------D------A------D------A-------|
> > + * IN ......
> > + * IN |-------D------A------D------A-------|
>
> The structure already discribes this information pretty well. No need
> for this table.
>
Drop it in v5.
> > + */
Best regards,
Ming
