Re: [PATCH V1 1/2] gpio: inverter: Add virtual controller for gpio configuration

[Linus Walleij <linus.walleij@xxxxxxxxxx>]

Hi Harish,

thanks for your patch! We have discussed this a lot
and it's nice to see it materialize!

On Wed, Jun 12, 2019 at 6:50 AM Harish Jenny K N
<harish_kandiga@xxxxxxxxxx> wrote:

> Provides a new virtual gpio controller to configure the polarity
> of the gpio pins used by the userspace.

I think we should refrain from using both the terms
"virtual", as it is a real thing, just modeled like this and
any reference to userspace because kernel drivers may
need this just as much.

> When there is no kernel
> driver using the gpio pin, it becomes necessary for the userspace
> to configure the polarity of the gpio pin.

So sometimes this might be necessary also for kernelspace,
it models the inverter on the PCB very well, and the fact
that DTS files work around it by adding things like
active low flags are really just hacks.
>
> Signed-off-by: Balasubramani Vivekanandan <balasubramani_vivekanandan@xxxxxxxxxx>
> Signed-off-by: Harish Jenny K N <harish_kandiga@xxxxxxxxxx>
> ---
>  drivers/gpio/Kconfig         |   9 +++
>  drivers/gpio/Makefile        |   1 +
>  drivers/gpio/gpio-inverter.c | 144 +++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 154 insertions(+)
>  create mode 100644 drivers/gpio/gpio-inverter.c
>
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index acd40eb..15893dd 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -77,6 +77,15 @@ config GPIO_GENERIC
>         depends on HAS_IOMEM # Only for IOMEM drivers
>         tristate
>
> This driver enables the userspace to directly use the gpio pin

Instead of "userspace" write "consumers".

> without worrying about the hardware level polarity configuration.
> Polarity configuration will be done by the virtual gpio controller
> based on device tree information

Call it the "inverter GPIO controller wrapper" or something
like this.

> +config GPIO_INVERTER
> +       tristate "Virtual GPIO controller for configuring the gpio polarity"

"Inverter GPIO controller for handling hardware inverters"

> +++ b/drivers/gpio/gpio-inverter.c
> @@ -0,0 +1,144 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Virtual GPIO controller for configuring the gpio polarity

Text like for the Kconfig.

> +#include <linux/gpio.h>
> +struct gpio_inverter {
> +       struct gpio_chip gpiochip;
> +       int count;
> +       struct gpio_desc *gpios[0];
> +};
> +
As everyone already pointed out, drop this.

> +#include <linux/of_gpio.h>

Also drop this.

> +static int gpio_inverter_direction_input(struct gpio_chip *chip,
> +                                        unsigned int offset)
> +{
> +       struct gpio_inverter *virt = gpiochip_get_data(chip);

Don't call it "virt" just call it "inv" or something, it isn't
really virtual.

> +       gpio_chip = &virt->gpiochip;
> +       gpio_chip->direction_input = gpio_inverter_direction_input;
> +       gpio_chip->direction_output = gpio_inverter_direction_output;
> +       gpio_chip->get = gpio_inverter_get;
> +       gpio_chip->set = gpio_inverter_set;
> +       gpio_chip->label = dev_name(dev);
> +       gpio_chip->parent = dev;
> +       gpio_chip->owner = THIS_MODULE;
> +       gpio_chip->base = -1;
> +       gpio_chip->ngpio = count;

As has been pointed out, check if we can wrap all functions
also .get/set_multiple (remembering inversion of course) and
.set_config().

> +static int gpio_inverter_remove(struct platform_device *pdev)
> +{
> +       struct gpio_inverter *virt = platform_get_drvdata(pdev);
> +
> +       gpiochip_remove(&virt->gpiochip);
> +
> +       return 0;
> +}

Can't you just use devm_gpiochip_add_data() and get rid of
this remove() function?

> +static int __init gpio_inverter_init(void)
> +{
> +       return platform_driver_register(&gpio_inverter_driver);
> +}
> +late_initcall(gpio_inverter_init);
> +
> +static void __exit gpio_inverter_exit(void)
> +{
> +       platform_driver_unregister(&gpio_inverter_driver);
> +}
> +module_exit(gpio_inverter_exit);

Why can't you just use module_platform_driver() and
handle all at the driver init level? Providers should
defer.

Yours,
Linus Walleij