| Message ID | 20241210104524.2466586-1-tmyu0@nuvoton.com |
|---|---|
| Headers | show |
| Series | Add Nuvoton NCT6694 MFD drivers | expand |
On 10/12/2024 11:45, Ming Yu wrote: > + nct6694->int_buffer = devm_kcalloc(dev, NCT6694_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) > + return -ENOMEM; > + > + nct6694->domain = irq_domain_add_simple(NULL, NCT6694_NR_IRQS, 0, > + &nct6694_irq_domain_ops, > + nct6694); > + if (!nct6694->domain) > + return -ENODEV; > + > + nct6694->udev = udev; > + nct6694->timeout = NCT6694_URB_TIMEOUT; /* Wait until urb complete */ > + nct6694->cmd_header = cmd_header; > + nct6694->response_header = response_header; > + > + mutex_init(&nct6694->access_lock); > + mutex_init(&nct6694->irq_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(dev, nct6694); > + usb_set_intfdata(iface, nct6694); > + > + ret = mfd_add_hotplug_devices(dev, nct6694_dev, ARRAY_SIZE(nct6694_dev)); > + if (ret) > + goto err_mfd; > + > + dev_info(dev, "Probed device: (%04X:%04X)\n", id->idVendor, id->idProduct); Drop. Duplicating existing messages and interfaces. Your driver is supposed to be silent on success. > + return 0; > + > +err_mfd: > + usb_kill_urb(nct6694->int_in_urb); > +err_urb: > + usb_free_urb(nct6694->int_in_urb); > + return dev_err_probe(dev, ret, "Probe failed\n"); No, this should go to individual call causing errors so this will be informative. Above is not informative at all and kernel already reports this, so drop. > +} > + > +static void nct6694_usb_disconnect(struct usb_interface *iface) > +{ > + struct usb_device *udev = interface_to_usbdev(iface); > + struct nct6694 *nct6694 = usb_get_intfdata(iface); Best regards, Krzysztof
On Tue, 10 Dec 2024 11:57:41 +0100 Mateusz Polchlopek wrote: > > +int nct6694_read_msg(struct nct6694 *nct6694, u8 mod, u16 offset, > > + u16 length, void *buf) > > +{ > > + struct nct6694_cmd_header *cmd_header = nct6694->cmd_header; > > + struct nct6694_response_header *response_header = nct6694->response_header; > > RCT violation This code is not under net not drivers/net As a general rule please focus on functional review, formatting and process issues are harder to judge unless you read all of the mailing list traffic.
On 12/11/2024 2:56 AM, Jakub Kicinski wrote: > On Tue, 10 Dec 2024 11:57:41 +0100 Mateusz Polchlopek wrote: >>> +int nct6694_read_msg(struct nct6694 *nct6694, u8 mod, u16 offset, >>> + u16 length, void *buf) >>> +{ >>> + struct nct6694_cmd_header *cmd_header = nct6694->cmd_header; >>> + struct nct6694_response_header *response_header = nct6694->response_header; >> >> RCT violation > > This code is not under net not drivers/net > As a general rule please focus on functional review, formatting and > process issues are harder to judge unless you read all of the mailing > list traffic. Okay, my bad. Thanks Kuba for explanation and I will focus on code next time
Le 10/12/2024 à 11:45, Ming Yu a écrit : > 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 request > interrupt that will be called when the USB device receives its > interrupt pipe. ... > +int nct6694_read_msg(struct nct6694 *nct6694, u8 mod, u16 offset, > + u16 length, void *buf) > +{ > + struct nct6694_cmd_header *cmd_header = nct6694->cmd_header; > + struct nct6694_response_header *response_header = nct6694->response_header; > + struct usb_device *udev = nct6694->udev; > + int tx_len, rx_len, ret; > + > + guard(mutex)(&nct6694->access_lock); Nitpick: This could be moved a few lines below, should it still comply with your coding style. > + > + /* Send command packet to USB device */ > + cmd_header->mod = mod; > + cmd_header->cmd = offset & 0xFF; > + cmd_header->sel = (offset >> 8) & 0xFF; > + cmd_header->hctrl = NCT6694_HCTRL_GET; > + cmd_header->len = length; > + > + ret = usb_bulk_msg(udev, usb_sndbulkpipe(udev, NCT6694_BULK_OUT_EP), > + cmd_header, NCT6694_CMD_PACKET_SZ, &tx_len, > + nct6694->timeout); > + if (ret) > + return ret; > + > + /* Receive response packet from USB device */ > + ret = usb_bulk_msg(udev, usb_rcvbulkpipe(udev, NCT6694_BULK_IN_EP), > + response_header, NCT6694_CMD_PACKET_SZ, &rx_len, > + nct6694->timeout); > + if (ret) > + return ret; > + > + ret = usb_bulk_msg(udev, usb_rcvbulkpipe(udev, NCT6694_BULK_IN_EP), > + buf, NCT6694_MAX_PACKET_SZ, &rx_len, nct6694->timeout); > + if (ret) > + return ret; > + > + return nct6694_response_err_handling(nct6694, response_header->sts); > +} > +EXPORT_SYMBOL(nct6694_read_msg); > + > +int nct6694_write_msg(struct nct6694 *nct6694, u8 mod, u16 offset, > + u16 length, void *buf) > +{ > + struct nct6694_cmd_header *cmd_header = nct6694->cmd_header; > + struct nct6694_response_header *response_header = nct6694->response_header; > + struct usb_device *udev = nct6694->udev; > + int tx_len, rx_len, ret; > + > + guard(mutex)(&nct6694->access_lock); Nitpick: This could be moved a few lines below, should it still comply with your coding style. > + > + /* Send command packet to USB device */ Nitpick: double space before */ > + cmd_header->mod = mod; > + cmd_header->cmd = offset & 0xFF; > + cmd_header->sel = (offset >> 8) & 0xFF; > + cmd_header->hctrl = NCT6694_HCTRL_SET; > + cmd_header->len = length; ... > +static struct irq_chip nct6694_irq_chip = { const? > + .name = "nct6694-irq", > + .flags = IRQCHIP_SKIP_SET_WAKE, > + .irq_bus_lock = nct6694_irq_lock, > + .irq_bus_sync_unlock = nct6694_irq_sync_unlock, > + .irq_enable = nct6694_irq_enable, > + .irq_disable = nct6694_irq_disable, > +}; ... > +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; > + struct nct6694_cmd_header *cmd_header; > + struct nct6694_response_header *response_header; > + int pipe, maxp; > + int ret; > + > + interface = iface->cur_altsetting; > + > + int_endpoint = &interface->endpoint[0].desc; > + if (!usb_endpoint_is_int_in(int_endpoint)) > + return -ENODEV; > + > + nct6694 = devm_kzalloc(dev, sizeof(*nct6694), GFP_KERNEL); > + if (!nct6694) > + return -ENOMEM; > + > + pipe = usb_rcvintpipe(udev, NCT6694_INT_IN_EP); > + maxp = usb_maxpacket(udev, pipe); > + > + cmd_header = devm_kzalloc(dev, sizeof(*cmd_header), > + GFP_KERNEL); > + if (!cmd_header) > + return -ENOMEM; > + > + response_header = devm_kzalloc(dev, sizeof(*response_header), > + GFP_KERNEL); > + if (!response_header) > + return -ENOMEM; > + > + nct6694->int_buffer = devm_kcalloc(dev, NCT6694_MAX_PACKET_SZ, > + sizeof(unsigned char), GFP_KERNEL); Why for cmd_header and response_header we use a temp variable, while here we update directly nct6694->int_buffer? It would save a few LoC do remove this temp var. > + if (!nct6694->int_buffer) > + return -ENOMEM; > + > + nct6694->int_in_urb = usb_alloc_urb(0, GFP_KERNEL); > + if (!nct6694->int_in_urb) > + return -ENOMEM; ... CJ
Dear Krzysztof, Thank you for your comments, Krzysztof Kozlowski <krzk@kernel.org> 於 2024年12月10日 週二 下午10:38寫道： > > > + > > + dev_set_drvdata(dev, nct6694); > > + usb_set_intfdata(iface, nct6694); > > + > > + ret = mfd_add_hotplug_devices(dev, nct6694_dev, ARRAY_SIZE(nct6694_dev)); > > + if (ret) > > + goto err_mfd; > > + > > + dev_info(dev, "Probed device: (%04X:%04X)\n", id->idVendor, id->idProduct); > > Drop. Duplicating existing messages and interfaces. Your driver is > supposed to be silent on success. > Okay, I will drop it in v4. > > + return 0; > > + > > +err_mfd: > > + usb_kill_urb(nct6694->int_in_urb); > > +err_urb: > > + usb_free_urb(nct6694->int_in_urb); > > + return dev_err_probe(dev, ret, "Probe failed\n"); > > No, this should go to individual call causing errors so this will be > informative. Above is not informative at all and kernel already reports > this, so drop. > Okay, I will drop it in v4. > > +} > > + > > +static void nct6694_usb_disconnect(struct usb_interface *iface) > > +{ > > + struct usb_device *udev = interface_to_usbdev(iface); > > + struct nct6694 *nct6694 = usb_get_intfdata(iface); > Best regards, Ming
Dear Guenter, Thank you for your comments, Guenter Roeck <linux@roeck-us.net> 於 2024年12月10日 週二 下午11:22寫道： > > > +static int nct6694_wdt_probe(struct platform_device *pdev) > > +{ > ... > > + wdev->timeout = timeout; > > + wdev->pretimeout = pretimeout; > > + if (timeout < pretimeout) { > > + dev_warn(data->dev, "pretimeout < timeout. Setting to zero\n"); > > + wdev->pretimeout = 0; > > + } > > + > > + wdev->min_timeout = 1; > > + wdev->max_timeout = 255; > > + > > + mutex_init(&data->lock); > > + > > + platform_set_drvdata(pdev, data); > > + > > + /* Register watchdog timer device to WDT framework */ > > + watchdog_set_drvdata(&data->wdev, data); > > + watchdog_init_timeout(&data->wdev, timeout, dev); > > This is pointless since timeout is pre-initialized with a value != 0. > That means a value provided through devicetree will never be used > unless the user sets timeout=0 as module parameter. But then the above > check for pretimeout is useless. > Understood! I will drop it in v4. Best regards, Ming
Dear Christophe, Thank you for your comments, Christophe JAILLET <christophe.jaillet@wanadoo.fr> 於 2024年12月12日 週四 上午12:44寫道： > > > +int nct6694_read_msg(struct nct6694 *nct6694, u8 mod, u16 offset, > > + u16 length, void *buf) > > +{ > > + struct nct6694_cmd_header *cmd_header = nct6694->cmd_header; > > + struct nct6694_response_header *response_header = nct6694->response_header; > > + struct usb_device *udev = nct6694->udev; > > + int tx_len, rx_len, ret; > > + > > + guard(mutex)(&nct6694->access_lock); > > Nitpick: This could be moved a few lines below, should it still comply > with your coding style. > I think the lock should be placed here to prevent the cmd_header from being overwritten by another caller. Could you share your perspective on this? > > + > > + /* Send command packet to USB device */ > > + cmd_header->mod = mod; > > + cmd_header->cmd = offset & 0xFF; > > + cmd_header->sel = (offset >> 8) & 0xFF; > > + cmd_header->hctrl = NCT6694_HCTRL_GET; > > + cmd_header->len = length; > > + > > + ret = usb_bulk_msg(udev, usb_sndbulkpipe(udev, NCT6694_BULK_OUT_EP), > > + cmd_header, NCT6694_CMD_PACKET_SZ, &tx_len, > > + nct6694->timeout); > > + if (ret) > > + return ret; > > + > > + /* Receive response packet from USB device */ > > + ret = usb_bulk_msg(udev, usb_rcvbulkpipe(udev, NCT6694_BULK_IN_EP), > > + response_header, NCT6694_CMD_PACKET_SZ, &rx_len, > > + nct6694->timeout); > > + if (ret) > > + return ret; > > + > > + ret = usb_bulk_msg(udev, usb_rcvbulkpipe(udev, NCT6694_BULK_IN_EP), > > + buf, NCT6694_MAX_PACKET_SZ, &rx_len, nct6694->timeout); > > + if (ret) > > + return ret; > > + > > + return nct6694_response_err_handling(nct6694, response_header->sts); > > +} > > +EXPORT_SYMBOL(nct6694_read_msg); > > + > > +int nct6694_write_msg(struct nct6694 *nct6694, u8 mod, u16 offset, > > + u16 length, void *buf) > > +{ > > + struct nct6694_cmd_header *cmd_header = nct6694->cmd_header; > > + struct nct6694_response_header *response_header = nct6694->response_header; > > + struct usb_device *udev = nct6694->udev; > > + int tx_len, rx_len, ret; > > + > > + guard(mutex)(&nct6694->access_lock); > > Nitpick: This could be moved a few lines below, should it still comply > with your coding style. > I think the lock should be placed here to prevent the cmd_header from being overwritten by another caller. Could you share your perspective on this? > > + > > + /* Send command packet to USB device */ > > Nitpick: double space before */ > Fix it in v4. > > + cmd_header->mod = mod; > > + cmd_header->cmd = offset & 0xFF; > > + cmd_header->sel = (offset >> 8) & 0xFF; > > + cmd_header->hctrl = NCT6694_HCTRL_SET; > > + cmd_header->len = length; > > ... > > > +static struct irq_chip nct6694_irq_chip = { > > const? > Fix it in v4. > > + .name = "nct6694-irq", > > + .flags = IRQCHIP_SKIP_SET_WAKE, > > + .irq_bus_lock = nct6694_irq_lock, > > + .irq_bus_sync_unlock = nct6694_irq_sync_unlock, > > + .irq_enable = nct6694_irq_enable, > > + .irq_disable = nct6694_irq_disable, > > +}; > > ... > > > +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; > > + struct nct6694_cmd_header *cmd_header; > > + struct nct6694_response_header *response_header; > > + int pipe, maxp; > > + int ret; > > + > > + interface = iface->cur_altsetting; > > + > > + int_endpoint = &interface->endpoint[0].desc; > > + if (!usb_endpoint_is_int_in(int_endpoint)) > > + return -ENODEV; > > + > > + nct6694 = devm_kzalloc(dev, sizeof(*nct6694), GFP_KERNEL); > > + if (!nct6694) > > + return -ENOMEM; > > + > > + pipe = usb_rcvintpipe(udev, NCT6694_INT_IN_EP); > > + maxp = usb_maxpacket(udev, pipe); > > + > > + cmd_header = devm_kzalloc(dev, sizeof(*cmd_header), > > + GFP_KERNEL); > > + if (!cmd_header) > > + return -ENOMEM; > > + > > + response_header = devm_kzalloc(dev, sizeof(*response_header), > > + GFP_KERNEL); > > + if (!response_header) > > + return -ENOMEM; > > + > > + nct6694->int_buffer = devm_kcalloc(dev, NCT6694_MAX_PACKET_SZ, > > + sizeof(unsigned char), GFP_KERNEL); > > Why for cmd_header and response_header we use a temp variable, while > here we update directly nct6694->int_buffer? > > It would save a few LoC do remove this temp var. > Fix it in v4. > > + if (!nct6694->int_buffer) > > + return -ENOMEM; > > + > > + nct6694->int_in_urb = usb_alloc_urb(0, GFP_KERNEL); > > + if (!nct6694->int_in_urb) > > + return -ENOMEM; > > ... > Best regards, Ming
Le 12/12/2024 à 08:01, Ming Yu a écrit : > Dear Christophe, > > Thank you for your comments, > > Christophe JAILLET <christophe.jaillet-39ZsbGIQGT5GWvitb5QawA@public.gmane.org> 於 2024年12月12日 週四 上午12:44寫道： >> >>> +int nct6694_read_msg(struct nct6694 *nct6694, u8 mod, u16 offset, >>> + u16 length, void *buf) >>> +{ >>> + struct nct6694_cmd_header *cmd_header = nct6694->cmd_header; >>> + struct nct6694_response_header *response_header = nct6694->response_header; >>> + struct usb_device *udev = nct6694->udev; >>> + int tx_len, rx_len, ret; >>> + >>> + guard(mutex)(&nct6694->access_lock); >> >> Nitpick: This could be moved a few lines below, should it still comply >> with your coding style. >> > > I think the lock should be placed here to prevent the cmd_header from > being overwritten by another caller. > Could you share your perspective on this? You are right, I misread the code :( (I though cmd_header was a local structure) > >>> + >>> + /* Send command packet to USB device */ >>> + cmd_header->mod = mod; >>> + cmd_header->cmd = offset & 0xFF; >>> + cmd_header->sel = (offset >> 8) & 0xFF; >>> + cmd_header->hctrl = NCT6694_HCTRL_GET; >>> + cmd_header->len = length; CJ