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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAHp75VcyNDNfYyUvSk4kToLiLCVDWo5-Rz_1_fRtR7i8ZjGr7g@mail.gmail.com>
Date:   Mon, 21 Mar 2022 18:39:00 +0200
From:   Andy Shevchenko <andy.shevchenko@...il.com>
To:     frank zago <frank@...o.net>
Cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Bartosz Golaszewski <bgolaszewski@...libre.com>,
        Wolfram Sang <wsa@...nel.org>, Johan Hovold <johan@...nel.org>,
        USB <linux-usb@...r.kernel.org>,
        Lee Jones <lee.jones@...aro.org>,
        Linus Walleij <linus.walleij@...aro.org>,
        "open list:GPIO SUBSYSTEM" <linux-gpio@...r.kernel.org>,
        linux-i2c <linux-i2c@...r.kernel.org>
Subject: Re: [PATCH v4 1/3] mfd: ch341: add core driver for the WCH CH341 in
 I2C/GPIO mode

On Mon, Mar 21, 2022 at 11:17 AM frank zago <frank@...o.net> 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