lkml.org 
[lkml]   [2022]   [Mar]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [PATCH v4 2/3] gpio: ch341: add MFD cell driver for the CH341
On Mon, Mar 21, 2022 at 4:13 PM frank zago <frank@zago.net> 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

\
 
 \ /
  Last update: 2022-03-21 18:08    [W:1.093 / U:0.012 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site