Re: [PATCH v7 5/9] serial: sc16is7xx: fix regression with GPIO configuration

[Hugo Villeneuve <hugo@xxxxxxxxxxx>]

On Sun, 4 Jun 2023 09:47:44 +0200
Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx> wrote:

> On Fri, Jun 02, 2023 at 11:26:21AM -0400, Hugo Villeneuve wrote:
> > From: Hugo Villeneuve <hvilleneuve@xxxxxxxxxxxx>
> > 
> > Commit 679875d1d880 ("sc16is7xx: Separate GPIOs from modem control lines")
> > and commit 21144bab4f11 ("sc16is7xx: Handle modem status lines")
> > changed the function of the GPIOs pins to act as modem control
> > lines without any possibility of selecting GPIO function.
> > 
> > As a consequence, applications that depends on GPIO lines configured
> > by default as GPIO pins no longer work as expected.
> > 
> > Also, the change to select modem control lines function was done only
> > for channel A of dual UART variants (752/762). This was not documented
> > in the log message.
> > 
> > Allow to specify GPIO or modem control line function in the device
> > tree, and for each of the ports (A or B).
> > 
> > Do so by using the new device-tree property named
> > "modem-control-line-ports" (property added in separate patch).
> > 
> > When registering GPIO chip controller, mask-out GPIO pins declared as
> > modem control lines according to this new "modem-control-line-ports"
> > DT property.
> > 
> > Boards that need to have GPIOS configured as modem control lines
> > should add that property to their device tree. Here is a list of
> > boards using the sc16is7xx driver in their device tree and that may
> > need to be modified:
> >     arm64/boot/dts/freescale/fsl-ls1012a-frdm.dts
> >     mips/boot/dts/ingenic/cu1830-neo.dts
> >     mips/boot/dts/ingenic/cu1000-neo.dts
> > 
> > Fixes: 679875d1d880 ("sc16is7xx: Separate GPIOs from modem control lines")
> > Fixes: 21144bab4f11 ("sc16is7xx: Handle modem status lines")
> > Cc: <stable@xxxxxxxxxxxxxxx> # 6.1.x: 35210b22 dt-bindings: sc16is7xx: Add property to change GPIO function
> > Cc: <stable@xxxxxxxxxxxxxxx> # 6.1.x: 7d61ca47 serial: sc16is7xx: refactor GPIO controller registration
> > Cc: <stable@xxxxxxxxxxxxxxx> # 6.1.x: 322470ed serial: sc16is7xx: mark IOCONTROL register as volatile
> > Cc: <stable@xxxxxxxxxxxxxxx> # 6.1.x: a0077362 serial: sc16is7xx: fix broken port 0 uart init
> > Cc: <stable@xxxxxxxxxxxxxxx> # 6.1.x
> > Signed-off-by: Hugo Villeneuve <hvilleneuve@xxxxxxxxxxxx>
> > Reviewed-by: Andy Shevchenko <andy.shevchenko@xxxxxxxxx>
> > ---
> >  drivers/tty/serial/sc16is7xx.c | 103 ++++++++++++++++++++++++++-------
> >  1 file changed, 82 insertions(+), 21 deletions(-)
> > 
> > diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
> > index 7d50674d2d0e..edc83f5f6340 100644
> > --- a/drivers/tty/serial/sc16is7xx.c
> > +++ b/drivers/tty/serial/sc16is7xx.c
> > @@ -236,7 +236,8 @@
> >  
> >  /* IOControl register bits (Only 750/760) */
> >  #define SC16IS7XX_IOCONTROL_LATCH_BIT	(1 << 0) /* Enable input latching */
> > -#define SC16IS7XX_IOCONTROL_MODEM_BIT	(1 << 1) /* Enable GPIO[7:4] as modem pins */
> > +#define SC16IS7XX_IOCONTROL_MODEM_A_BIT	(1 << 1) /* Enable GPIO[7:4] as modem A pins */
> > +#define SC16IS7XX_IOCONTROL_MODEM_B_BIT	(1 << 2) /* Enable GPIO[3:0] as modem B pins */
> >  #define SC16IS7XX_IOCONTROL_SRESET_BIT	(1 << 3) /* Software Reset */
> >  
> >  /* EFCR register bits */
> > @@ -301,12 +302,12 @@
> >  /* Misc definitions */
> >  #define SC16IS7XX_FIFO_SIZE		(64)
> >  #define SC16IS7XX_REG_SHIFT		2
> > +#define SC16IS7XX_GPIOS_PER_BANK	4
> >  
> >  struct sc16is7xx_devtype {
> >  	char	name[10];
> >  	int	nr_gpio;
> >  	int	nr_uart;
> > -	int	has_mctrl;
> >  };
> >  
> >  #define SC16IS7XX_RECONF_MD		(1 << 0)
> > @@ -336,6 +337,7 @@ struct sc16is7xx_port {
> >  	struct clk			*clk;
> >  #ifdef CONFIG_GPIOLIB
> >  	struct gpio_chip		gpio;
> > +	unsigned long			gpio_valid_mask;
> >  #endif
> >  	unsigned char			buf[SC16IS7XX_FIFO_SIZE];
> >  	struct kthread_worker		kworker;
> > @@ -447,35 +449,30 @@ static const struct sc16is7xx_devtype sc16is74x_devtype = {
> >  	.name		= "SC16IS74X",
> >  	.nr_gpio	= 0,
> >  	.nr_uart	= 1,
> > -	.has_mctrl	= 0,
> >  };
> >  
> >  static const struct sc16is7xx_devtype sc16is750_devtype = {
> >  	.name		= "SC16IS750",
> > -	.nr_gpio	= 4,
> > +	.nr_gpio	= 8,
> >  	.nr_uart	= 1,
> > -	.has_mctrl	= 1,
> >  };
> >  
> >  static const struct sc16is7xx_devtype sc16is752_devtype = {
> >  	.name		= "SC16IS752",
> > -	.nr_gpio	= 0,
> > +	.nr_gpio	= 8,
> >  	.nr_uart	= 2,
> > -	.has_mctrl	= 1,
> >  };
> >  
> >  static const struct sc16is7xx_devtype sc16is760_devtype = {
> >  	.name		= "SC16IS760",
> > -	.nr_gpio	= 4,
> > +	.nr_gpio	= 8,
> >  	.nr_uart	= 1,
> > -	.has_mctrl	= 1,
> >  };
> >  
> >  static const struct sc16is7xx_devtype sc16is762_devtype = {
> >  	.name		= "SC16IS762",
> > -	.nr_gpio	= 0,
> > +	.nr_gpio	= 8,
> >  	.nr_uart	= 2,
> > -	.has_mctrl	= 1,
> >  };
> >  
> >  static bool sc16is7xx_regmap_volatile(struct device *dev, unsigned int reg)
> > @@ -1350,16 +1347,45 @@ static int sc16is7xx_gpio_direction_output(struct gpio_chip *chip,
> >  	return 0;
> >  }
> >  
> > -static int sc16is7xx_setup_gpio_chip(struct device *dev)
> > +static int sc16is7xx_gpio_init_valid_mask(struct gpio_chip *chip,
> > +					  unsigned long *valid_mask,
> > +					  unsigned int ngpios)
> > +{
> > +	struct sc16is7xx_port *s = gpiochip_get_data(chip);
> > +
> > +	*valid_mask = s->gpio_valid_mask;
> > +
> > +	return 0;
> > +}
> > +
> > +static int sc16is7xx_setup_gpio_chip(struct device *dev, u8 mctrl_mask)
> >  {
> >  	struct sc16is7xx_port *s = dev_get_drvdata(dev);
> >  
> >  	if (!s->devtype->nr_gpio)
> >  		return 0;
> >  
> > +	switch (mctrl_mask) {
> > +	case 0:
> > +		s->gpio_valid_mask = GENMASK(7, 0);
> > +		break;
> > +	case SC16IS7XX_IOCONTROL_MODEM_A_BIT:
> > +		s->gpio_valid_mask = GENMASK(3, 0);
> > +		break;
> > +	case SC16IS7XX_IOCONTROL_MODEM_B_BIT:
> > +		s->gpio_valid_mask = GENMASK(7, 4);
> > +		break;
> > +	default:
> > +		break;
> > +	}
> > +
> > +	if (s->gpio_valid_mask == 0)
> > +		return 0;
> > +
> >  	s->gpio.owner		 = THIS_MODULE;
> >  	s->gpio.parent		 = dev;
> >  	s->gpio.label		 = dev_name(dev);
> > +	s->gpio.init_valid_mask	 = sc16is7xx_gpio_init_valid_mask;
> >  	s->gpio.direction_input	 = sc16is7xx_gpio_direction_input;
> >  	s->gpio.get		 = sc16is7xx_gpio_get;
> >  	s->gpio.direction_output = sc16is7xx_gpio_direction_output;
> > @@ -1371,6 +1397,44 @@ static int sc16is7xx_setup_gpio_chip(struct device *dev)
> >  }
> >  #endif
> >  
> > +static u8 sc16is7xx_setup_mctrl_ports(struct device *dev)
> 
> This returns what, mctrl?  If so, please document that, it doesn't look
> obvious.  And as the kernel test robot reported, you do nothing with the
> return value so why compute it?

Hi Greg,
I will  the following comment to document return value:

/*
 * Configure ports designated to operate as modem control lines.
 * Return mask of ports configured as modem control lines.
 */
static u8 sc16is7xx_setup_mctrl_ports(struct device *dev)


The kernel test robot identified a case, when CONFIG_GPIOLIB is not defined, where the value returned by sc16is7xx_setup_mctrl_ports() is not used. But the function sc16is7xx_setup_mctrl_ports() still need to be called to configure the modem status line ports correctly in that case.

And if CONFIG_GPIOLIB is defined, the value is definitely used (and needed).

Here is what I suggest to silence the warning:

	mctrl_mask = sc16is7xx_setup_mctrl_ports(dev);

#ifdef CONFIG_GPIOLIB
	ret = sc16is7xx_setup_gpio_chip(dev, mctrl_mask);
	if (ret)
		goto out_thread;
#else
	(void) mctrl_mask;
#endif

I could also store (define new variable) mctrl_mask directly inside struct sc16is7xx_port...


> And you have a real port here, no need to pass in a "raw" struct device,
> right?

The function operates globally on both ports (or nr_uart), not just a single port. That is why I pass the "raw" struct device, in order to extract the 
struct sc16is7xx_port from it:

    struct sc16is7xx_port *s = dev_get_drvdata(dev);

Inside the function, I also need the "raw" struc device. If we pass a struct sc16is7xx_port to the function, then I can get the "raw" struc device with this:

static u8 sc16is7xx_setup_mctrl_ports(struct sc16is7xx_port *s)
{
	struct device *dev = &s->p[0].port.dev;

But I find this more obfuscated and hard to understand than to simply pass a "raw" struct device...

Hugo.