Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210325122832.119147-1-sandberg@mailfence.com>
 <20210530161333.3996-1-maukka@ext.kapsi.fi> <20210530161333.3996-3-maukka@ext.kapsi.fi>
In-Reply-To: <20210530161333.3996-3-maukka@ext.kapsi.fi>
From:   Linus Walleij <linus.walleij@linaro.org>
Date:   Tue, 1 Jun 2021 12:38:01 +0200
Message-ID: <CACRpkdYf06W2QDY6EN0OG3RjOnJ+AVE+Wd4M6Z9=B7aZ9rGfwA@mail.gmail.com>
Subject: Re: [PATCH v4 2/2] gpio: gpio-mux-input: add generic gpio input multiplexer
To:     Mauri Sandberg <maukka@ext.kapsi.fi>
Cc:     Mauri Sandberg <sandberg@mailfence.com>,
        Andy Shevchenko <andy.shevchenko@gmail.com>,
        Bartosz Golaszewski <bgolaszewski@baylibre.com>,
        Geert Uytterhoeven <geert+renesas@glider.be>,
        "open list:GPIO SUBSYSTEM" <linux-gpio@vger.kernel.org>,
        Rob Herring <robh+dt@kernel.org>,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@vger.kernel.org>,
        linux-kernel <linux-kernel@vger.kernel.org>,
        Drew Fustini <drew@beagleboard.org>,
        kernel test robot <lkp@intel.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Sun, May 30, 2021 at 6:16 PM Mauri Sandberg <maukka@ext.kapsi.fi> wrote:

> Adds support for a generic GPIO multiplexer. To drive the multiplexer a
> mux-controller is needed. The output pin of the multiplexer is a GPIO
> pin.
>
> Reported-by: kernel test robot <lkp@intel.com>
> Signed-off-by: Mauri Sandberg <maukka@ext.kapsi.fi>
> Tested-by: Drew Fustini <drew@beagleboard.org>
> Reviewed-by: Drew Fustini <drew@beagleboard.org>

The commit message and part of the driver becomes hard to
read and understand because the word pin is overused.
Switch to talk about "gpio lines" rather than pins.

Draw a simple ASCII image like this:

               /|---- Cascaded GPIO line 0
              |M|---- Cascaded GPIO line 1
GPIO line ----+U| .
              |X| .
           \|---- Cascaded GPIO line n

Maybe also as illustration in the driver and in the bindings.
Make things easy to understand.

Explain exactly why only input lines can be multiplexed.

I'm not sure it should be restricted to just input
in theory, but since that is all we can test, restrict it to
input in practice.

> +config GPIO_MUX_INPUT
> +       tristate "General GPIO input multiplexer"

Rename it just GPIO_MUX
  "General GPIO multiplexer"

Then clarify in the help description that it currently can only
handle input lines.

> +       depends on OF_GPIO
> +       select MULTIPLEXER
> +       select MUX_GPIO
> +       help
> +         Say yes here to enable support for generic GPIO input multiplexer.
> +
> +         This driver uses a mux-controller to drive the multiplexer and has a
> +         single output pin for reading the inputs to the mux. The driver can
> +         be used in situations when GPIO pins are used to select what
> +         multiplexer pin should be used for reading input and the output pin
> +         of the multiplexer is connected to a GPIO input pin.

Input output etc, this gets very hard to understand.

Switch terminology from "pin" to "GPIO lines", (or "GPIO rails").

Use the word "routing" as the GPIO line is routed through the
multiplexer. Maybe spell out multiplexer for clarity.

Explain why, for electrical reasons, output lines are harder
to multiplex like this, as the output will not maintain
state. Notice that "using open drain constructions, output
multiplexing may be possible, but it is currently not implemented."

> +static int gpio_mux_input_get_direction(struct gpio_chip *gc,
> +                                       unsigned int offset)
> +{
> +       return GPIO_LINE_DIRECTION_IN;
> +}

Explain why this is a restriction with a comment in the code.
Add comment that in the future we might be able to handle
also output.

> +static int gpio_mux_input_get_value(struct gpio_chip *gc, unsigned int offset)

This looks very nice!

We might have to extend this driver at some point.

Intuitively I'd say it takes some time and then someone
comes along and say "actually we have done this
for output as well, using some open drain and stuff"
but this is a good starting point anyway we need no
big upfront designs.

Yours,
Linus Walleij