Messages in this thread | | | From | Andy Shevchenko <> | Date | Mon, 21 Mar 2022 19:06:10 +0200 | Subject | Re: [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
| |