On Tue, Oct 19, 2021 at 4:00 PM Mauri Sandberg <maukka@xxxxxxxxxxxx> 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
