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>
 <20210621172053.107045-1-maukka@ext.kapsi.fi> <20210621172053.107045-3-maukka@ext.kapsi.fi>
In-Reply-To: <20210621172053.107045-3-maukka@ext.kapsi.fi>
From:   Andy Shevchenko <andy.shevchenko@gmail.com>
Date:   Mon, 21 Jun 2021 20:43:21 +0300
Message-ID: <CAHp75VcjGpveAHNAW7Xf7d_Zf6LGSSyD6+qBiF9xxvb+EKs3tg@mail.gmail.com>
Subject: Re: [PATCH v5 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"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Mon, Jun 21, 2021 at 8:25 PM Mauri Sandberg <maukka@ext.kapsi.fi> wrote:
>
> Adds support for a building cascades of GPIO lines. That is, it allows

for building

> 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 conveyd to the selected cascaded line or, vice versa, the status

conveyed

> of the cascaded line can be conveyed to the upstream line.
>
> A gpio-mux 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.

Since in parallel there is a discussion about the virtio-gpio
interface, how will this work with it?

> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + *  A generic GPIO cascade driver
> + *
> + *  Copyright (C) 2021 Mauri Sandberg <maukka@ext.kapsi.fi>
> + *
> + * This allows building cascades of GPIO lines in a manner illustrated
> + * below:
> + *
> + *                 /|---- Cascaded GPIO line 0
> + *  Upstream      | |---- Cascaded GPIO line 1
> + *  GPIO line ----+ | .
> + *                | | .
> + *                 \|---- Cascaded GPIO line n
> + *
> + * A gpio-mux is being used to select, which cascaded line is being
> + * addressed at any given time.
> + *
> + * At the moment only input mode is supported due to lack of means for
> + * testing output functionality. At least theoretically output should be
> + * possible with an open drain constructions.
> + */

...

> +static int gpio_cascade_get_value(struct gpio_chip *gc, unsigned int off=
set)
> +{
> +       struct gpio_cascade *cas;
> +       int ret;

> +       cas =3D chip_to_cascade(gc);

Doing this in the definition block above will save a LOC.

> +       ret =3D mux_control_select(cas->mux_control, offset);
> +       if (ret)
> +               return ret;
> +
> +       ret =3D gpiod_get_value(cas->upstream_line);
> +       mux_control_deselect(cas->mux_control);
> +       return ret;
> +}

...

> +       struct device_node *np =3D pdev->dev.of_node;

Nope, see below.

...

> +       cas =3D devm_kzalloc(dev, sizeof(struct gpio_cascade), GFP_KERNEL=
);

sizeof(*cas)

> +       if (cas =3D=3D NULL)

if (!cas)

> +               return -ENOMEM;

...

> +       mc =3D devm_mux_control_get(dev, NULL);
> +       if (IS_ERR(mc)) {
> +               err =3D (int) PTR_ERR(mc);
> +               if (err !=3D -EPROBE_DEFER)
> +                       dev_err(dev, "unable to get mux-control: %d\n", e=
rr);
> +               return err;

Oh l=C3=A0 l=C3=A0! No, the explicit castings are bad. besides the fact tha=
t all
above can be replaced by

  return dev_err_probe(...);

> +       }
> +
> +       cas->mux_control =3D mc;
> +       upstream =3D devm_gpiod_get(dev, "upstream",  GPIOD_IN);
> +       if (IS_ERR(upstream)) {

> +               err =3D (int) PTR_ERR(upstream);
> +               dev_err(dev, "unable to claim upstream GPIO line: %d\n", =
err);

No castings. Use proper printf() specifiers.

> +               return err;
> +       }

...

> +       gc->of_node =3D np;

This should be guarded by CONFIG_OF_GPIO.
And no need to use the np temporary variable for one use like this.

...

> +       err =3D gpiochip_add(&cas->gpio_chip);

Why not the devm variant?

> +       if (err) {
> +               dev_err(dev, "unable to add gpio chip, err=3D%d\n", err);
> +               return err;
> +       }

...

> +       dev_info(dev, "registered %u cascaded GPIO lines\n", gc->ngpio);

No, we don't pollute logs when everything is fine.

...

> +static const struct of_device_id gpio_cascade_id[] =3D {
> +       {
> +               .compatible =3D "gpio-cascade",

> +               .data =3D NULL,

Redundant.

> +       },

All above may consume only a single LOC.

> +       { /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, gpio_cascade_id);

--=20
With Best Regards,
Andy Shevchenko