On Mon, Jun 21, 2021 at 8:25 PM Mauri Sandberg [off-list ref] 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 [off-list ref]
+ *
+ * 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