lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Tue, 19 Oct 2021 16:12:57 +0300 From: Andy Shevchenko <andy.shevchenko@...il.com> To: Mauri Sandberg <maukka@....kapsi.fi> Cc: Mauri Sandberg <sandberg@...lfence.com>, Bartosz Golaszewski <bgolaszewski@...libre.com>, Geert Uytterhoeven <geert+renesas@...der.be>, Linus Walleij <linus.walleij@...aro.org>, "open list:GPIO SUBSYSTEM" <linux-gpio@...r.kernel.org>, Rob Herring <robh+dt@...nel.org>, devicetree <devicetree@...r.kernel.org>, Linux Kernel Mailing List <linux-kernel@...r.kernel.org>, Drew Fustini <drew@...gleboard.org> Subject: Re: [PATCH v6 2/2] gpio: gpio-cascade: add generic GPIO cascade On Tue, Oct 19, 2021 at 4:00 PM Mauri Sandberg <maukka@....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
Powered by blists - more mailing lists