Re: [PATCH v4 1/3] mfd: ch341: add core driver for the WCH CH341 in I2C/GPIO mode
From: Andy Shevchenko
Date: Mon Mar 21 2022 - 12:40:16 EST
On Mon, Mar 21, 2022 at 11:17 AM frank zago <frank@xxxxxxxx> wrote:
>
> The CH341 is a multifunction chip, presenting 3 different USB PID. One
> of these functions is for I2C/SPI/GPIO. This new set of drivers will
> manage I2C and GPIO.
...
> +The driver doesn't support detection of I2C device present on the
devices
> +bus. Apparently when a device is not present at a given address, the
> +CH341 will return an extra byte of data, but the driver doesn't
> +support that. This may be addressed in the future.
...
> spear-pcie-gadget
> uacce
> xilinx_sdfec
> + ch341
Seems you broke the order.
...
> +config MFD_CH341
> + tristate "WinChipHead CH341 in I2C/SPI/GPIO mode"
(1)
> + depends on USB
> + help
> + If you say yes to this option, support for the CH341 series
> + of chips, running in I2C/SPI/GPIO mode will be included.
"chips running" (no comma needed)
> + The chip's SPI mode is not supported.
Maybe no need to include SPI in the (1) along with dropping this line?
> + This driver can also be built as a module. If so, the
> + module will be called ch341-core.
...
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/slab.h>
(2)
> +#include <linux/mfd/core.h>
> +
> +#include <linux/mfd/ch341.h>
Moving these two to (2) ?
...
> + dev = kzalloc(sizeof(*dev), GFP_KERNEL);
devm_kzalloc() ?
> + if (!dev)
> + return -ENOMEM;
...
> + rc = mfd_add_hotplug_devices(&iface->dev, ch341_devs,
> + ARRAY_SIZE(ch341_devs));
> + if (rc) {
> + dev_err(&iface->dev, "Failed to add mfd devices to core.");
> + goto free_dev;
return dev_err_probe(...); ?
> + }
...
> + usb_set_intfdata(dev->iface, NULL);
This has been done by device driver core for the past 10+ years.
...
> +static const struct usb_device_id ch341_usb_table[] = {
> + { USB_DEVICE(0x1a86, 0x5512) },
> + { }
> +};
> +
Redundant blank line.
> +MODULE_DEVICE_TABLE(usb, ch341_usb_table);
> +
> +static struct usb_driver ch341_usb_driver = {
> + .name = "ch341-mfd",
> + .id_table = ch341_usb_table,
> + .probe = ch341_usb_probe,
> + .disconnect = ch341_usb_disconnect
Keep a comma to avoid unneeded churn in the future.
> +};
> +
Redundant blank line.
> +module_usb_driver(ch341_usb_driver);
> +/*
> + * Definitions for CH341 MFD driver
> + */
One line?
...
> +#include <linux/usb.h>
No users of this header. Use forward declarations.
> +#include <linux/mutex.h>
Missed types.h.
...
> +#define DEFAULT_TIMEOUT 1000 /* 1s USB requests timeout */
Use proper suffix, i.e. _MS.
--
With Best Regards,
Andy Shevchenko