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>
 <20211019125731.4327-1-maukka@ext.kapsi.fi> <20211019125731.4327-3-maukka@ext.kapsi.fi>
In-Reply-To: <20211019125731.4327-3-maukka@ext.kapsi.fi>
From:   Andy Shevchenko <andy.shevchenko@gmail.com>
Date:   Tue, 19 Oct 2021 16:12:57 +0300
Message-ID: <CAHp75VeOZY+YHfFsnTNdYfan=dxZx9ZEoCr8PAgOewAZX4khsQ@mail.gmail.com>
Subject: Re: [PATCH v6 2/2] gpio: gpio-cascade: add generic GPIO cascade
To:     Mauri Sandberg <maukka@ext.kapsi.fi>
Cc:     Mauri Sandberg <sandberg@mailfence.com>,
        Bartosz Golaszewski <bgolaszewski@baylibre.com>,
        Geert Uytterhoeven <geert+renesas@glider.be>,
        Linus Walleij <linus.walleij@linaro.org>,
        "open list:GPIO SUBSYSTEM" <linux-gpio@vger.kernel.org>,
        Rob Herring <robh+dt@kernel.org>,
        devicetree <devicetree@vger.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Drew Fustini <drew@beagleboard.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Tue, Oct 19, 2021 at 4:00 PM Mauri Sandberg <maukka@ext.kapsi.fi> wrote:
>
> Adds support for building cascades of GPIO lines. That is, it allows
> setups when there is one upstream line and multiple cascaded lines, out
> of which one can be chosen at a time. The status of the upstream line
> can be conveyed to the selected cascaded line or, vice versa, the status
> of the cascaded line can be conveyed to the upstream line.
>
> A multiplexer is being used to select, which cascaded GPIO line is being
> used at any given time.
>
> At the moment only input direction is supported. In future it should be
> possible to add support for output direction, too.

Thanks for an update! My comments below.

...

> +config GPIO_CASCADE
> +       tristate "General GPIO cascade"
> +       select MULTIPLEXER
> +       help
> +         Say yes here to enable support for generic GPIO cascade.
> +
> +         This allows building one-to-many cascades of GPIO lines using
> +         different types of multiplexers readily available. At the
> +         moment only input lines are supported.

Care to mention what will be the module name in the case of being
built as a module?
(Hint: there are plenty of existing examples in the kernel)

...

> +#include <linux/module.h>

> +#include <linux/gpio/consumer.h>
> +#include <linux/gpio/driver.h>

I would move this group...

> +#include <linux/slab.h>
> +#include <linux/platform_device.h>
> +#include <linux/mux/consumer.h>
> +

...to be somewhere here to explicitly show that this is the GPIO
subsystem related driver.

...

> +       mc = devm_mux_control_get(dev, NULL);
> +       if (IS_ERR(mc))

> +               return dev_err_probe(dev,
> +                                    PTR_ERR(mc),
> +                                    "unable to get mux-control\n");

Why not one line?

...

> +       upstream = devm_gpiod_get(dev, "upstream",  GPIOD_IN);
> +       if (IS_ERR(upstream)) {
> +               dev_err(dev, "unable to claim upstream GPIO line\n");
> +               return -ENODEV;

Why shadowing error code? What happens if it's deferred?
Hint: use dev_err_probe() here as well.

> +       }

...

> +       err = devm_gpiochip_add_data(dev, &cas->gpio_chip, NULL);
> +       if (err) {
> +               dev_err(dev, "unable to add gpio chip\n");
> +               return err;
> +       }
> +
> +       platform_set_drvdata(pdev, cas);
> +       return 0;

I would rather do

       platform_set_drvdata(pdev, cas);

       return devm_gpiochip_add_data(dev, &cas->gpio_chip, NULL);

-- 
With Best Regards,
Andy Shevchenko