Re: [PATCH v4 2/3] gpio: ch341: add MFD cell driver for the CH341

[Andy Shevchenko <andy.shevchenko@xxxxxxxxx>]

On Mon, Mar 21, 2022 at 4:13 PM frank zago <frank@xxxxxxxx> wrote:
>
> The GPIO interface offers 16 GPIOs. 6 are read/write, and 10 are
> read-only.

We use terminology of output-only and input-only. Is it what you are
telling us? If it's something else, you have to elaborate much better
on what's going on with these GPIO lines.

...

> +config GPIO_CH341
> +       tristate "CH341 USB adapter in GPIO/I2C/SPI mode"

How is this driver related to either SPI or I²C modes?

> +       depends on MFD_CH341

Can't be compile tested?

> +       help
> +         If you say yes to this option, GPIO support will be included for the
> +         WCH CH341, a USB to I2C/SPI/GPIO interface.
> +
> +         This driver can also be built as a module.  If so, the module
> +         will be called gpio-ch341.

...

> +/* Notes.

Keep the proper (not network) style for multi-line comments.


> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/mfd/core.h>
> +#include <linux/gpio.h>

> +#include <linux/mfd/ch341.h>

If I got your intention with groups of headers, I would see rather

...ordered headers...
blank line
#include <linux/gpio.h>

But more importantly that gpio.h is the wrong header and must be
replaced with the appropriate one from the include/gpio/ folder.

Also you have missed headers, like types.h.

...

> +#define CH341_PARA_CMD_STS          0xA0  /* Get pins status */
> +#define CH341_CMD_UIO_STREAM        0xAB  /* UIO stream command */
> +
> +#define CH341_CMD_UIO_STM_OUT       0x80  /* UIO interface OUT command (D0~D5) */
> +#define CH341_CMD_UIO_STM_DIR       0x40  /* UIO interface DIR command (D0~D5) */
> +#define CH341_CMD_UIO_STM_END       0x20  /* UIO interface END command */

What does UIO mean here? If it is Userspace I/O in terms of Linux
kernel, it's no-go we want this. Otherwise needs to be explained
somewhere.

...

> +struct ch341_gpio {
> +       struct gpio_chip gpio;
> +       struct mutex gpio_lock;
> +       u16 gpio_dir;           /* 1 bit per pin, 0=IN, 1=OUT. */
> +       u16 gpio_last_read;     /* last GPIO values read */
> +       u16 gpio_last_written;  /* last GPIO values written */
> +       u8 gpio_buf[SEG_SIZE];
> +
> +       struct {
> +               char name[32];
> +               bool enabled;
> +               struct irq_chip irq;
> +               int num;
> +               struct urb *urb;
> +               struct usb_anchor urb_out;
> +               u8 buf[CH341_USB_MAX_INTR_SIZE];
> +       } gpio_irq;

We have a specific GPIO IRQ chip structure, what is the purpose of
semi-duplication of it?

> +
> +       struct ch341_device *ch341;
> +};

...

> +static void ch341_gpio_dbg_show(struct seq_file *s, struct gpio_chip *chip)
> +{
> +       struct ch341_gpio *dev = gpiochip_get_data(chip);
> +
> +       seq_printf(s, "pin config  : %04x  (0=IN, 1=OUT)\n", dev->gpio_dir);
> +       seq_printf(s, "last read   : %04x\n", dev->gpio_last_read);
> +       seq_printf(s, "last written: %04x\n", dev->gpio_last_written);

Multi-line debug output is quite non-standard among GPIO drivers.

> +}

> +{
> +       struct ch341_device *ch341 = dev->ch341;
> +       int actual;
> +       int rc;
> +
> +       mutex_lock(&ch341->usb_lock);
> +
> +       rc = usb_bulk_msg(ch341->usb_dev,
> +                         usb_sndbulkpipe(ch341->usb_dev, ch341->ep_out),
> +                         dev->gpio_buf, out_len,
> +                         &actual, DEFAULT_TIMEOUT);

> +       if (rc < 0)
> +               goto done;
> +
> +       if (in_len == 0) {
> +               rc = actual;
> +               goto done;
> +       }

You may do it better. See below.

> +       rc = usb_bulk_msg(ch341->usb_dev,
> +                         usb_rcvbulkpipe(ch341->usb_dev, ch341->ep_in),
> +                         dev->gpio_buf, SEG_SIZE, &actual, DEFAULT_TIMEOUT);
> +
> +       if (rc == 0)
> +               rc = actual;

> +done:

out_unlock: sounds better.

> +       mutex_unlock(&ch341->usb_lock);

> +       return rc;

if (rc < 0)
  return rc;

return actual;

> +}

...

> +       int result;

rc / result / etc... Please, become consistent in naming the return
code variable.

...

> +       if (result == 6)
> +               dev->gpio_last_read = le16_to_cpu(*(__le16 *)dev->gpio_buf);

So, it means you have the wrong type of gpio_but. Also you missed the
pointer versions of leXX_to_cpu() helpers.

...

> +       return (result != 6) ? result : 0;

Besides redundant parentheses, this can be optimized. I will leave it
for your homework (the hint is given at the top part of the review).

...

> +       return (dev->gpio_last_read & BIT(offset)) ? 1 : 0;

!! can be used. But it's up to you and maintainers, the compiler will
do its job anyway.

...

> +       dev->gpio_last_written &= ~*mask;
> +       dev->gpio_last_written |= (*bits & *mask);

Can be done in one line as it's a well established pattern in Linux
kernel for drivers.

...

> +       return (dev->gpio_dir & BIT(offset)) ? 0 : 1;

! will do the job.

...

> +       if (!(pin_can_output & mask))
> +               return -EINVAL;

I don't remember if we have a valid mask for this case.

...

> +       if (!urb->status) {

Will be much better to simply do:

if (urb_status) {
 ...
 return;
}

> +       } else {
> +               usb_unanchor_urb(dev->gpio_irq.urb);
> +       }

...

> +       if (data->irq != dev->gpio_irq.num || type != IRQ_TYPE_EDGE_RISING)
> +               return -EINVAL;

Usually we lock the handler type here while in ->probe() we assign a
bad handler by default in order to filter out spurious interrupts.

...

> +       dev->gpio_irq.enabled = true;

What is the purpose of this flag? Note there is a patch to add a
specific flag to the descriptor to do exactly this.

...

> +/* Convert the GPIO index to the IRQ number */
> +static int ch341_gpio_to_irq(struct gpio_chip *chip, unsigned int offset)
> +{
> +       struct ch341_gpio *dev = gpiochip_get_data(chip);
> +
> +       if (offset != CH341_GPIO_INT_LINE)
> +               return -ENXIO;
> +
> +       return dev->gpio_irq.num;

In the new code we will have the special field that limits the GPIO
IRQ lines (can be different to the ngpio).

> +}

...

> +       snprintf(dev->gpio_irq.name, sizeof(dev->gpio_irq.name),
> +                "ch341-%s-gpio", dev_name(&ch341->usb_dev->dev));

> +       dev->gpio_irq.name[sizeof(dev->gpio_irq.name) - 1] = 0;

This is redundant. Have you read the manual page on snprintf()?

...

> +       rc = devm_irq_alloc_desc(&pdev->dev, 0);
> +       if (rc < 0) {

> +               dev_err(&pdev->dev, "Cannot allocate an IRQ desc");
> +               return rc;

return dev_err_probe();

> +       }
> +
> +       dev->gpio_irq.num = rc;
> +       dev->gpio_irq.enabled = false;
> +
> +       irq_set_chip_data(dev->gpio_irq.num, dev);
> +       irq_set_chip_and_handler(dev->gpio_irq.num, &dev->gpio_irq.irq,
> +                                handle_simple_irq);

Oh là là. Can you use the latest and greatest approach of
instantiating the GPIO IRQ chip?

...

> +               dev_err(&pdev->dev, "Cannot allocate the int URB");
> +               return -ENOMEM;

return dev_err_probe();

...

> +       rc = gpiochip_add_data(gpio, dev);

Why not devm?

> +       if (rc) {
> +               dev_err(&pdev->dev, "Could not add GPIO\n");
> +               goto release_urb;

return dev_err_probe();

> +       }

...

> +static struct platform_driver ch341_gpio_driver = {
> +       .driver.name    = "ch341-gpio",
> +       .probe          = ch341_gpio_probe,
> +       .remove         = ch341_gpio_remove,
> +};
> +

Redundant blank line.

> +module_platform_driver(ch341_gpio_driver);

-- 
With Best Regards,
Andy Shevchenko