| Message ID | 20210530161333.3996-3-maukka@ext.kapsi.fi |
|---|---|
| State | New |
| Headers | show |
| Series | gpio: add generic gpio input multiplexer | expand |
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 Sun, May 30, 2021 at 6: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> > Signed-off-by: Mauri Sandberg <maukka@ext.kapsi.fi> > Tested-by: Drew Fustini <drew@beagleboard.org> > Reviewed-by: Drew Fustini <drew@beagleboard.org> The commit message and part of the driver becomes hard to read and understand because the word pin is overused. Switch to talk about "gpio lines" rather than pins. Draw a simple ASCII image like this: /|---- Cascaded GPIO line 0 |M|---- Cascaded GPIO line 1 GPIO line ----+U| . |X| . \|---- Cascaded GPIO line n Maybe also as illustration in the driver and in the bindings. Make things easy to understand. Explain exactly why only input lines can be multiplexed. I'm not sure it should be restricted to just input in theory, but since that is all we can test, restrict it to input in practice. > +config GPIO_MUX_INPUT > + tristate "General GPIO input multiplexer" Rename it just GPIO_MUX "General GPIO multiplexer" Then clarify in the help description that it currently can only handle input lines. > + depends on OF_GPIO > + select MULTIPLEXER > + select MUX_GPIO > + 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. Input output etc, this gets very hard to understand. Switch terminology from "pin" to "GPIO lines", (or "GPIO rails"). Use the word "routing" as the GPIO line is routed through the multiplexer. Maybe spell out multiplexer for clarity. Explain why, for electrical reasons, output lines are harder to multiplex like this, as the output will not maintain state. Notice that "using open drain constructions, output multiplexing may be possible, but it is currently not implemented." > +static int gpio_mux_input_get_direction(struct gpio_chip *gc, > + unsigned int offset) > +{ > + return GPIO_LINE_DIRECTION_IN; > +} Explain why this is a restriction with a comment in the code. Add comment that in the future we might be able to handle also output. > +static int gpio_mux_input_get_value(struct gpio_chip *gc, unsigned int offset) This looks very nice! We might have to extend this driver at some point. Intuitively I'd say it takes some time and then someone comes along and say "actually we have done this for output as well, using some open drain and stuff" but this is a good starting point anyway we need no big upfront designs. Yours, Linus Walleij
diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig index 1dd0ec6727fd..8a41a283ba42 100644 --- a/drivers/gpio/Kconfig +++ b/drivers/gpio/Kconfig @@ -1657,4 +1657,20 @@ config GPIO_MOCKUP endmenu +comment "Other GPIO expanders" + +config GPIO_MUX_INPUT + tristate "General GPIO input multiplexer" + depends on OF_GPIO + select MULTIPLEXER + select MUX_GPIO + 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. + endif diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile index d7c81e1611a4..ff2b530d8ef4 100644 --- a/drivers/gpio/Makefile +++ b/drivers/gpio/Makefile @@ -104,6 +104,7 @@ obj-$(CONFIG_GPIO_MPC5200) += gpio-mpc5200.o obj-$(CONFIG_GPIO_MPC8XXX) += gpio-mpc8xxx.o obj-$(CONFIG_GPIO_MSC313) += gpio-msc313.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 diff --git a/drivers/gpio/gpio-mux-input.c b/drivers/gpio/gpio-mux-input.c new file mode 100644 index 000000000000..e0739640541e --- /dev/null +++ b/drivers/gpio/gpio-mux-input.c @@ -0,0 +1,124 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * A generic GPIO input multiplexer driver + * + * Copyright (C) 2021 Mauri Sandberg <maukka@ext.kapsi.fi> + * + */ + +#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_get_direction(struct gpio_chip *gc, + unsigned int offset) +{ + return GPIO_LINE_DIRECTION_IN; +} + +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); + mux_control_deselect(mux->mux_control); + return ret; +} + +static int gpio_mux_input_probe(struct platform_device *pdev) +{ + struct device_node *np = pdev->dev.of_node; + struct device *dev = &pdev->dev; + struct gpio_mux_input *mux; + struct mux_control *mc; + struct gpio_desc *pin; + struct gpio_chip *gc; + int err; + + mux = devm_kzalloc(dev, sizeof(struct gpio_mux_input), GFP_KERNEL); + if (mux == NULL) + 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; + } + + mux->mux_control = mc; + pin = devm_gpiod_get(dev, "pin", GPIOD_IN); + if (IS_ERR(pin)) { + err = (int) PTR_ERR(pin); + dev_err(dev, "unable to claim output pin: %d\n", err); + return err; + } + + mux->mux_pin = pin; + mux->parent = dev; + + gc = &mux->gpio_chip; + gc->get = gpio_mux_input_get_value; + gc->get_direction = gpio_mux_input_get_direction; + + 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(dev, "unable to add gpio chip, err=%d\n", err); + return err; + } + + platform_set_drvdata(pdev, mux); + dev_info(dev, "registered %u input GPIOs\n", gc->ngpio); + return 0; +} + +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", + .of_match_table = gpio_mux_input_id, + }, + .probe = gpio_mux_input_probe, +}; +module_platform_driver(gpio_mux_input_driver); + +MODULE_AUTHOR("Mauri Sandberg <maukka@ext.kapsi.fi>"); +MODULE_DESCRIPTION("Generic GPIO input multiplexer"); +MODULE_LICENSE("GPL");