| Message ID | 20210325122832.119147-1-sandberg@mailfence.com |
|---|---|
| Headers | show |
| Series | gpio: add generic gpio input multiplexer | expand |
On Thu, Mar 25, 2021 at 02:28:32PM +0200, Mauri Sandberg wrote: > Suppport for a general GPIO multiplexer. To drive the multiplexer a > mux-controller is needed. The output pin of the multiplexer is a GPIO > pin > > Signed-off-by: Mauri Sandberg <sandberg@mailfence.com> Thanks for posting the RFC so we can take a look at the code and discuss how it works. > --- > drivers/gpio/Kconfig | 11 +++ > drivers/gpio/Makefile | 1 + > drivers/gpio/gpio-mux-input.c | 143 ++++++++++++++++++++++++++++++++++ > 3 files changed, 155 insertions(+) > create mode 100644 drivers/gpio/gpio-mux-input.c > > diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig > index c70f46e80a3b..41062d8f7d93 100644 > --- a/drivers/gpio/Kconfig > +++ b/drivers/gpio/Kconfig > @@ -1641,4 +1641,15 @@ config GPIO_MOCKUP > > endmenu > > +comment "Other GPIO expanders" > + > +config GPIO_MUX_INPUT > + tristate "General GPIO input multiplexer" > + select MULTIPLEXER > + select MUX_GPIO > + depends on OF_GPIO > + help > + Say yes here to enable support for generic GPIO input multiplexer. This > + needs a multiplexer controller to drive the select pins. > + > endif > diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile > index 35e3b6026665..00f7576ce23f 100644 > --- a/drivers/gpio/Makefile > +++ b/drivers/gpio/Makefile > @@ -105,6 +105,7 @@ obj-$(CONFIG_GPIO_MPC8XXX) += gpio-mpc8xxx.o > obj-$(CONFIG_GPIO_MSC313) += gpio-msc313.o > obj-$(CONFIG_GPIO_MSIC) += gpio-msic.o > obj-$(CONFIG_GPIO_MT7621) += gpio-mt7621.o > +obj-$(CONFIG_GPIO_MUX_INPUT) += gpio-mux-input.o > obj-$(CONFIG_GPIO_MVEBU) += gpio-mvebu.o > obj-$(CONFIG_GPIO_MXC) += gpio-mxc.o > obj-$(CONFIG_GPIO_MXS) += gpio-mxs.o This does not apply to mainline. I've added it manually to my drivers/gpio/Makefile but something to fix in v2. > diff --git a/drivers/gpio/gpio-mux-input.c b/drivers/gpio/gpio-mux-input.c > new file mode 100644 > index 000000000000..ec0c7acbab2f > --- /dev/null > +++ b/drivers/gpio/gpio-mux-input.c > @@ -0,0 +1,143 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * A generic GPIO input multiplexer driver > + * > + * Copyright (C) 2021 Mauri Sandberg <sandberg@mailfence.com> > + * > + */ > + > +#include <linux/module.h> > +#include <linux/gpio/consumer.h> > +#include <linux/gpio/driver.h> > +#include <linux/slab.h> > +#include <linux/platform_device.h> > +#include <linux/mux/consumer.h> > + > +struct gpio_mux_input { > + struct device *parent; > + struct gpio_chip gpio_chip; > + struct mux_control *mux_control; > + struct gpio_desc *mux_pin; > +}; > + > +static struct gpio_mux_input *gpio_to_mux(struct gpio_chip *gc) > +{ > + return container_of(gc, struct gpio_mux_input, gpio_chip); > +} > + > +static int gpio_mux_input_direction_input(struct gpio_chip *gc, > + unsigned int offset) > +{ > + return 0; > +} > + > +static int gpio_mux_input_direction_output(struct gpio_chip *gc, > + unsigned int offset, int val) > +{ > + return -EINVAL; > +} > + > +static int gpio_mux_input_get_value(struct gpio_chip *gc, unsigned int offset) > +{ > + struct gpio_mux_input *mux; > + int ret; > + > + mux = gpio_to_mux(gc); > + ret = mux_control_select(mux->mux_control, offset); > + if (ret) > + return ret; > + > + ret = gpiod_get_value(mux->mux_pin); I'm not too familiar with how mux_control works but does there need to be locking here? Or is not possible for mux_pin to change to another offset before if gpiod_get_value() if gpio_mux_input_get_value() runs concurrently? > + mux_control_deselect(mux->mux_control); > + return ret; > +} > + > +static void gpio_mux_input_set_value(struct gpio_chip *gc, > + unsigned int offset, int val) > +{ > + /* not supported */ I'm not sure but maybe it is better not to define gc->set in the probe? > +} > + > +static int gpio_mux_input_probe(struct platform_device *pdev) > +{ > + struct device_node *np = pdev->dev.of_node; > + struct gpio_mux_input *mux; > + struct gpio_chip *gc; > + struct mux_control *mc; > + struct gpio_desc *pin; > + int err; > + > + mux = kzalloc(sizeof(struct gpio_mux_input), GFP_KERNEL); > + if (mux == NULL) > + return -ENOMEM; > + > + mc = mux_control_get(&pdev->dev, NULL); > + if (IS_ERR(mc)) { > + err = (int) PTR_ERR(mc); > + if (err != -EPROBE_DEFER) > + dev_err(&pdev->dev, "unable to get mux-control: %d\n", > + err); > + goto err_free_mux; > + } > + > + mux->mux_control = mc; > + pin = gpiod_get(&pdev->dev, "pin", GPIOD_IN); > + if (IS_ERR(pin)) { > + err = (int) PTR_ERR(pin); > + dev_err(&pdev->dev, "unable to claim pin GPIOs: %d\n", err); > + goto err_free_mc; > + } > + > + mux->mux_pin = pin; > + mux->parent = &pdev->dev; > + > + gc = &mux->gpio_chip; > + gc->direction_input = gpio_mux_input_direction_input; > + gc->direction_output = gpio_mux_input_direction_output; > + gc->get = gpio_mux_input_get_value; > + gc->set = gpio_mux_input_set_value; > + gc->can_sleep = 1; > + > + gc->base = -1; > + gc->ngpio = mux_control_states(mc); > + gc->label = dev_name(mux->parent); > + gc->parent = mux->parent; > + gc->owner = THIS_MODULE; > + gc->of_node = np; > + > + err = gpiochip_add(&mux->gpio_chip); > + if (err) { > + dev_err(&pdev->dev, "unable to add gpio chip, err=%d\n", err); > + goto err_free_pin; > + } > + > + platform_set_drvdata(pdev, mux); > + return 0; > + > +err_free_pin: > + gpiod_put(pin); > +err_free_mc: > + mux_control_put(mc); > +err_free_mux: > + kfree(mux); > + return err; > +} > + > +static const struct of_device_id gpio_mux_input_id[] = { > + { > + .compatible = "gpio-mux-input", > + .data = NULL, > + }, > + { /* sentinel */ } > +}; > +MODULE_DEVICE_TABLE(of, gpio_mux_input_id); > + > +static struct platform_driver gpio_mux_input_driver = { > + .driver = { > + .name = "gpio-mux-input", > + .owner = THIS_MODULE, > + .of_match_table = gpio_mux_input_id, > + }, > + .probe = gpio_mux_input_probe, > +}; > +module_platform_driver(gpio_mux_input_driver); I believe you need to add: MODULE_AUTHOR("..."); MODULE_DESCRIPTION("..."); MODULE_LICENSE("GPL"); My build failed with: ERROR: modpost: missing MODULE_LICENSE() in drivers/gpio/gpio-mux-input.o LZMA arch/arm/boot/compressed/piggy_data make[1]: *** [scripts/Makefile.modpost:132: Module.symvers] Error 1 make[1]: *** Deleting file 'Module.symvers' make: *** [Makefile:1442: modules] Error 2 make: *** Waiting for unfinished jobs.... AS arch/arm/boot/compressed/piggy.o LD arch/arm/boot/compressed/vmlinux OBJCOPY arch/arm/boot/zImage Kernel: arch/arm/boot/zImage is ready I added those lines and it compiled successfully. -Drew
> ---------------------------------------- > From: Drew Fustini <drew@beagleboard.org> > Sent: Fri Mar 26 07:59:44 CET 2021 > To: Mauri Sandberg <sandberg@mailfence.com> > > diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile > > index 35e3b6026665..00f7576ce23f 100644 > > --- a/drivers/gpio/Makefile > > +++ b/drivers/gpio/Makefile > > @@ -105,6 +105,7 @@ obj-$(CONFIG_GPIO_MPC8XXX) += gpio-mpc8xxx.o > > obj-$(CONFIG_GPIO_MSC313) += gpio-msc313.o > > obj-$(CONFIG_GPIO_MSIC) += gpio-msic.o > > obj-$(CONFIG_GPIO_MT7621) += gpio-mt7621.o > > +obj-$(CONFIG_GPIO_MUX_INPUT) += gpio-mux-input.o > > obj-$(CONFIG_GPIO_MVEBU) += gpio-mvebu.o > > obj-$(CONFIG_GPIO_MXC) += gpio-mxc.o > > obj-$(CONFIG_GPIO_MXS) += gpio-mxs.o > > This does not apply to mainline. I've added it manually to my > drivers/gpio/Makefile but something to fix in v2. I was developing against gpio tree's [1] 'for-next' branch but should I go against mainline? > > diff --git a/drivers/gpio/gpio-mux-input.c b/drivers/gpio/gpio-mux-input.c > > +static int gpio_mux_input_get_value(struct gpio_chip *gc, unsigned int offset) > > +{ > > + struct gpio_mux_input *mux; > > + int ret; > > + > > + mux = gpio_to_mux(gc); > > + ret = mux_control_select(mux->mux_control, offset); > > + if (ret) > > + return ret; > > + > > + ret = gpiod_get_value(mux->mux_pin); > > I'm not too familiar with how mux_control works but does there need to > be locking here? > > Or is not possible for mux_pin to change to another offset before if > gpiod_get_value() if gpio_mux_input_get_value() runs concurrently? According to mux documentation [2] successfully selecting a state locks the mux until it is deselected. So I reckon no extra locking is needed. > > + > > +static void gpio_mux_input_set_value(struct gpio_chip *gc, > > + unsigned int offset, int val) > > +{ > > + /* not supported */ > > I'm not sure but maybe it is better not to define gc->set in the probe? I will give it a go. > I believe you need to add: > > MODULE_AUTHOR("..."); > MODULE_DESCRIPTION("..."); > MODULE_LICENSE("GPL"); > > My build failed with: > > ERROR: modpost: missing MODULE_LICENSE() in drivers/gpio/gpio-mux-input.o I will add them, thanks. How do you do your build? Mine does not complain pretty much about anything. Also a bot gave me a warning and I would like to run those tests manually before submitting anything for review. Cheers, Mauri [1] https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-gpio.git [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/mux/core.c#n322
On Fri, Mar 26, 2021 at 12:32 PM Mauri Sandberg <sandberg@mailfence.com> wrote: > > From: Drew Fustini <drew@beagleboard.org> > > Sent: Fri Mar 26 07:59:44 CET 2021 ... > > This does not apply to mainline. I've added it manually to my > > drivers/gpio/Makefile but something to fix in v2. > > I was developing against gpio tree's [1] 'for-next' branch but should I go against mainline? It's a Bart's tree nowadays [3], gpio/for-next branch. > > > + ret = gpiod_get_value(mux->mux_pin); > > > > I'm not too familiar with how mux_control works but does there need to > > be locking here? > > > > Or is not possible for mux_pin to change to another offset before if > > gpiod_get_value() if gpio_mux_input_get_value() runs concurrently? > > According to mux documentation [2] successfully selecting a state locks the mux > until it is deselected. So I reckon no extra locking is needed. ... > [1] https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-gpio.git > [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/mux/core.c#n322 [3]: https://git.kernel.org/pub/scm/linux/kernel/git/brgl/linux.git/ -- With Best Regards, Andy Shevchenko
On Mon, May 17, 2021 at 07:58:45PM +0300, Mauri Sandberg wrote: > Hello all! > > This patch set is closely related to another thread at [4], which I abandoned > against better judgement and created this one. > > Here I am sending revised versions of the patches. It builds on v2 and adopts > managed device resources as suggested by Andy on the thread mentioned > above [5]. > > I have tested the functionality on a NXP 74HC153 dual 4-way muxer. Drew, did > you find the time to have a go with this [6] and if so, did it work as expected? I ordered the parts but did not get around to trying it. I will try it out tomorrow with this patch series. Thanks, Drew
For some reason Bart did not get the patches with the previous email. So resending. -- Mauri
On Mon, May 24, 2021 at 07:29:04PM +0300, Mauri Sandberg wrote: > For some reason Bart did not get the patches with the previous email. So > resending. > > -- Mauri > > Tested-by: Drew Fustini <drew@beagleboard.org> Reviewed-by: Drew Fustini <drew@beagleboard.org> I just replied to the original v3 cover letter with my test results [1]. -Drew [1] https://lore.kernel.org/linux-gpio/20210524212538.GA3756746@x1/
I can't see the mails in this RFC series but it looks interesting and quite useful. Looking forward to the next posting. Yours, Linus Walleij
On Thu, May 27, 2021 at 5:23 PM Linus Walleij <linus.walleij@linaro.org> wrote: > > I can't see the mails in this RFC series but it looks interesting and quite > useful. > > Looking forward to the next posting. > > Yours, > Linus Walleij That is too bad. Hopefully Mauri can repost. Here it is on lore: https://lore.kernel.org/linux-gpio/20210325122832.119147-1-sandberg@mailfence.com/ I tested it successfully on a BeagleBone Black with a TI CD74HC153E: https://lore.kernel.org/linux-gpio/20210524212538.GA3756746@x1/ thanks, drew
Hello all!
I changed my email setup because there were serious issues with it
previously and patches were not delivered to people. Hopefully it's working
now. Because of that I am sending updated patches in v4, which consist of
the same functionality as in v3 added with cosmetic changes to Kconfig and
updated author email address.
Drew gave acked and tested-by tags in [1] so I took the liberty to include
them in the patches.
For convenience I am including also rangediff between v3 and v4 below.
Thanks,
Mauri
[1] https://www.spinics.net/lists/linux-gpio/msg61277.html
1: 1ca26bb53ab6 ! 1: 496558967cd8 dt-bindings: gpio-mux-input: add documentation
@@
## Metadata ##
-Author: Mauri Sandberg <sandberg@mailfence.com>
+Author: Mauri Sandberg <maukka@ext.kapsi.fi>
## Commit message ##
dt-bindings: gpio-mux-input: add documentation
Add documentation for a general GPIO multiplexer.
- Signed-off-by: Mauri Sandberg <sandberg@mailfence.com>
+ Signed-off-by: Mauri Sandberg <maukka@ext.kapsi.fi>
+ Tested-by: Drew Fustini <drew@beagleboard.org>
+ Reviewed-by: Drew Fustini <drew@beagleboard.org>
---
+ v3 -> v4:
+ - Changed author email
+ - Included Tested-by and Reviewed-by from Drew
v2 -> v3: added a complete example on dual 4-way multiplexer
v1 -> v2: added a little bit more text in the binding documenation
@@ Documentation/devicetree/bindings/gpio/gpio-mux-input.yaml (new)
+title: Generic GPIO input multiplexer
+
+maintainers:
-+ - Mauri Sandberg <sandberg@mailfence.com>
++ - Mauri Sandberg <maukka@ext.kapsi.fi>
+
+description: |
+ A generic GPIO based input multiplexer
2: dcd76ada9d34 ! 2: 782e009ba54b gpio: gpio-mux-input: add generic gpio input multiplexer
@@
## Metadata ##
-Author: Mauri Sandberg <sandberg@mailfence.com>
+Author: Mauri Sandberg <maukka@ext.kapsi.fi>
## Commit message ##
gpio: gpio-mux-input: add generic gpio input multiplexer
@@ Commit message
pin.
Reported-by: kernel test robot <lkp@intel.com>
- Signed-off-by: Mauri Sandberg <sandberg@mailfence.com>
+ Signed-off-by: Mauri Sandberg <maukka@ext.kapsi.fi>
+ Tested-by: Drew Fustini <drew@beagleboard.org>
+ Reviewed-by: Drew Fustini <drew@beagleboard.org>
---
+ v3 -> v4:
+ - Changed author email
+ - Included Tested-by and Reviewed-by from Drew
v2 -> v3:
- use managed device resources
- update Kconfig description
@@ drivers/gpio/Kconfig: config GPIO_MOCKUP
+ help
+ Say yes here to enable support for generic GPIO input multiplexer.
+
-+ This driver uses a mux-controller to drive the multiplexer and has a
-+ single output pin for reading the inputs to the mux. The driver can be
-+ used in situations when GPIO pins are used to select what multiplexer
-+ pin should be used for reading input and the output pin of the
-+ multiplexer is connected to a GPIO input pin.
++ This driver uses a mux-controller to drive the multiplexer and has a
++ single output pin for reading the inputs to the mux. The driver can
++ be used in situations when GPIO pins are used to select what
++ multiplexer pin should be used for reading input and the output pin
++ of the multiplexer is connected to a GPIO input pin.
+
endif
@@ drivers/gpio/gpio-mux-input.c (new)
+/*
+ * A generic GPIO input multiplexer driver
+ *
-+ * Copyright (C) 2021 Mauri Sandberg <sandberg@mailfence.com>
++ * Copyright (C) 2021 Mauri Sandberg <maukka@ext.kapsi.fi>
+ *
+ */
+
@@ drivers/gpio/gpio-mux-input.c (new)
+};
+module_platform_driver(gpio_mux_input_driver);
+
-+MODULE_AUTHOR("Mauri Sandberg <sandberg@mailfence.com>");
++MODULE_AUTHOR("Mauri Sandberg <maukka@ext.kapsi.fi>");
+MODULE_DESCRIPTION("Generic GPIO input multiplexer");
+MODULE_LICENSE("GPL");
Mauri Sandberg (2):
dt-bindings: gpio-mux-input: add documentation
gpio: gpio-mux-input: add generic gpio input multiplexer
.../bindings/gpio/gpio-mux-input.yaml | 75 +++++++++++
drivers/gpio/Kconfig | 16 +++
drivers/gpio/Makefile | 1 +
drivers/gpio/gpio-mux-input.c | 124 ++++++++++++++++++
4 files changed, 216 insertions(+)
create mode 100644 Documentation/devicetree/bindings/gpio/gpio-mux-input.yaml
create mode 100644 drivers/gpio/gpio-mux-input.c
base-commit: c354c29524eeabba63da51f30a09b85ec9dc853a
On Sun, May 30, 2021 at 7:16 PM Mauri Sandberg <maukka@ext.kapsi.fi> wrote: > > Adds support for a generic GPIO multiplexer. To drive the multiplexer a > mux-controller is needed. The output pin of the multiplexer is a GPIO > pin. > > Reported-by: kernel test robot <lkp@intel.com> Is it a fix? Shall we add the Fixes tag? > Signed-off-by: Mauri Sandberg <maukka@ext.kapsi.fi> > Tested-by: Drew Fustini <drew@beagleboard.org> > Reviewed-by: Drew Fustini <drew@beagleboard.org> -- With Best Regards, Andy Shevchenko
On 30.5.2021 21.09, Andy Shevchenko wrote: > On Sun, May 30, 2021 at 7:16 PM Mauri Sandberg <maukka@ext.kapsi.fi> wrote: >> Adds support for a generic GPIO multiplexer. To drive the multiplexer a >> mux-controller is needed. The output pin of the multiplexer is a GPIO >> pin. >> >> Reported-by: kernel test robot <lkp@intel.com> > Is it a fix? Shall we add the Fixes tag? In the v1 a build bot complained about .owner along these lines: --- snip ---- If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot <lkp@intel.com> cocci warnings: (new ones prefixed by >>) >> drivers/gpio/gpio-mux-input.c:138:3-8: No need to set .owner here. The core will do it. Please review and possibly fold the followup patch. --- snip --- I removed the .owner attribute in v2 as requested but wasn't really sure whether it was "appropriate" to add the tag so I put it there anyhow. Technically, this does not fix any previous commit. >> Signed-off-by: Mauri Sandberg <maukka@ext.kapsi.fi> >> Tested-by: Drew Fustini <drew@beagleboard.org> >> Reviewed-by: Drew Fustini <drew@beagleboard.org> >
On Sun, May 30, 2021 at 10:02 PM Mauri Sandberg <maukka@ext.kapsi.fi> wrote: > On 30.5.2021 21.09, Andy Shevchenko wrote: > > On Sun, May 30, 2021 at 7:16 PM Mauri Sandberg <maukka@ext.kapsi.fi> wrote: > >> Reported-by: kernel test robot <lkp@intel.com> > > Is it a fix? Shall we add the Fixes tag? > > In the v1 a build bot complained about .owner along these lines: > > --- snip ---- > If you fix the issue, kindly add following tag as appropriate > Reported-by: kernel test robot <lkp@intel.com> > > > cocci warnings: (new ones prefixed by >>) > >> drivers/gpio/gpio-mux-input.c:138:3-8: No need to set .owner here. > The core will do it. > > Please review and possibly fold the followup patch. > --- snip --- > > I removed the .owner attribute in v2 as requested but wasn't really sure > whether it was "appropriate" > to add the tag so I put it there anyhow. Technically, this does not fix > any previous commit. For this kind of thing you may attribute the reporter(s) by mentioning them in the comment lines / cover letter. -- With Best Regards, Andy Shevchenko
On 30.5.2021 22.38, Andy Shevchenko wrote: > On Sun, May 30, 2021 at 10:02 PM Mauri Sandberg <maukka@ext.kapsi.fi> wrote: >> On 30.5.2021 21.09, Andy Shevchenko wrote: >>> On Sun, May 30, 2021 at 7:16 PM Mauri Sandberg <maukka@ext.kapsi.fi> wrote: >>>> Reported-by: kernel test robot <lkp@intel.com> >>> Is it a fix? Shall we add the Fixes tag? >> In the v1 a build bot complained about .owner along these lines: >> >> --- snip ---- >> If you fix the issue, kindly add following tag as appropriate >> Reported-by: kernel test robot <lkp@intel.com> >> >> >> cocci warnings: (new ones prefixed by >>) >> >> drivers/gpio/gpio-mux-input.c:138:3-8: No need to set .owner here. >> The core will do it. >> >> Please review and possibly fold the followup patch. >> --- snip --- >> >> I removed the .owner attribute in v2 as requested but wasn't really sure >> whether it was "appropriate" >> to add the tag so I put it there anyhow. Technically, this does not fix >> any previous commit. > For this kind of thing you may attribute the reporter(s) by mentioning > them in the comment lines / cover letter. It's there in the patch version notes so the 'Reported-by' was unnecessary. Should it be removed? That is, is there a tool sitting somwhere that tries to match reports and their fixes?
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
On 21.06.21 19:43, Andy Shevchenko wrote: > Since in parallel there is a discussion about the virtio-gpio > interface, how will this work with it? Haven't really understood what this is actually doing. A multiplexer where only external line is connected to some actual gpio at a time ? Or does it merge multiple inputs into one (eg. logical OR) ? Is that about real hardware mux chips or just a software only ? What is the actual use case ? For now, I don't see any relation to virtio-gpio. Correct me if I'm wrong. --mtx -- --- Hinweis: unverschlüsselte E-Mails können leicht abgehört und manipuliert werden ! Für eine vertrauliche Kommunikation senden Sie bitte ihren GPG/PGP-Schlüssel zu. --- Enrico Weigelt, metux IT consult Free software and Linux embedded engineering info@metux.net -- +49-151-27565287
On Mon, 21 Jun 2021 20:20:52 +0300, Mauri Sandberg wrote: > Add documentation for a general GPIO cascade. It allows building > one-to-many cascades of GPIO lines using multiplexer to choose > the cascaded line. > > Signed-off-by: Mauri Sandberg <maukka@ext.kapsi.fi> > --- > v4 -> v5: > - renamed gpio-mux-input -> gpio-cascade > - changed vague term 'pin' to 'upstream line' > - added more verbose description for the module > - added missing 'mux-controls' entry > - dropped Tested-by and Reviewed-by due to changes in bindings > v3 -> v4: > - Changed author email > - Included Tested-by and Reviewed-by from Drew > v2 -> v3: added a complete example on dual 4-way multiplexer > v1 -> v2: added a little bit more text in the binding documenation > --- > .../bindings/gpio/gpio-cascade.yaml | 103 ++++++++++++++++++ > 1 file changed, 103 insertions(+) > create mode 100644 Documentation/devicetree/bindings/gpio/gpio-cascade.yaml > Reviewed-by: Rob Herring <robh@kernel.org>