Re: [PATCH V2 2/2] gpio: inverter: document the inverter bindings

[Linus Walleij <linus.walleij@xxxxxxxxxx>]

Hi Harish!

This is interesting development. I have some comments:

On Mon, Jun 17, 2019 at 5:14 PM Harish Jenny K N
<harish_kandiga@xxxxxxxxxx> wrote:

> Document the device tree binding for the inverter gpio
> controller to configure the polarity of the gpio pins
> used by the consumers.
>
> Signed-off-by: Harish Jenny K N <harish_kandiga@xxxxxxxxxx>

(...)

> +Required properties:
> +- compatible : "gpio-inverter".
> +- gpio-controller: Marks the port as GPIO controller.
> +- #gpio-cells: Two. The first cell is the pin number and
> +   the second cell is used to specify the gpio polarity as defined in
> +   defined in <dt-bindings/gpio/gpio.h>:
> +      0 = GPIO_ACTIVE_HIGH
> +      1 = GPIO_ACTIVE_LOW

Since this is an inverter I do not see the point of supporting anything
else than 0 (GPIO_ACTIVE_HIGH) here.

This is  consumer flag and as a *consumer* the inverter definately
does not require anything active low, it *produces* the inverse
semantic.

> +- mapped-gpios: Array of GPIO pins required from consumers, whose polarity has
> +  to be configured in the driver.

Why "mapped-"?

Just call them "gpios".

> +- gpio-line-names: This is an array of strings defining the names for the
> +  mapped-gpios correspondingly. Name should be defined for each gpio pin.

Just reference the generic gpio documentation here.

> +gpio_inv: gpio-inv {
> +       compatible = "gpio-inverter";
> +       gpio-controller;
> +       #gpio-cell = <2>;
> +       mapped-gpios = <&gpio5 24 1>, <&gpio7 0 1>, <&gpio7 1 1>;

Use predefined GPIO_ACTIVE_HIGH on all three.

I think this is using GPIO_ACTIVE_LOW just to get free inversion inside
GPIOLIB. That's not OK really, this is a hardware description problem and
totally different from the implementation problem inside the driver.

Yours,
Linus Walleij