Re: [PATCH v4 1/3] mfd: ch341: add core driver for the WCH CH341 in I2C/GPIO mode

[Andy Shevchenko <andy.shevchenko@xxxxxxxxx>]

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