Re: [PATCH v5 2/2] gpio: gpio-cascade: add generic GPIO cascade

[Andy Shevchenko <andy.shevchenko@gmail.com>]

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 offset)
> +{
> +       struct gpio_cascade *cas;
> +       int ret;

> +       cas = chip_to_cascade(gc);

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

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

...

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

Nope, see below.

...

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

sizeof(*cas)

> +       if (cas == NULL)

if (!cas)

> +               return -ENOMEM;

...

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

Oh là là! No, the explicit castings are bad. besides the fact that all
above can be replaced by

  return dev_err_probe(...);

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

> +               err = (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 = np;

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

...

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

Why not the devm variant?

> +       if (err) {
> +               dev_err(dev, "unable to add gpio chip, err=%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[] = {
> +       {
> +               .compatible = "gpio-cascade",

> +               .data = NULL,

Redundant.

> +       },

All above may consume only a single LOC.

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

-- 
With Best Regards,
Andy Shevchenko