Re: [RFC gpio/for-next 2/2] gpio: gpio-mux-input: add generic gpio input multiplexer

[Mauri Sandberg <sandberg@xxxxxxxxxxxxx>]

> ----------------------------------------
> From: Drew Fustini <drew@xxxxxxxxxxxxxxx>
> Sent: Fri Mar 26 07:59:44 CET 2021
> To: Mauri Sandberg <sandberg@xxxxxxxxxxxxx>
> > 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