Re: [RFC PATCH] Revert "sc16is7xx: Separate GPIOs from modem control lines"

[Hugo Villeneuve <hugo@hugovil.com>]

Hi Greg,

On Mon, 15 May 2023 18:20:02 +0200
Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:

> On Mon, May 15, 2023 at 12:02:07PM -0400, Hugo Villeneuve wrote:
> > From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> > 
> > This reverts commit 679875d1d8802669590ef4d69b0e7d13207ebd61.
> > 
> > Because of this commit, it is no longer possible to use the 16 GPIO
> > lines as dedicated GPIOs on the SC16IS752.
> > 
> > Reverting it makes it work again.
> > 
> > The log message of the original commit states:
> >     "Export only the GPIOs that are not shared with hardware modem
> >     control lines"
> > 
> > But there is no explanation as to why this decision was taken to
> > permanently set the function of the GPIO lines as modem control
> > lines. AFAIK, there is no problem with using these lines as GPIO or modem
> > control lines.
> > 
> > Maybe after reverting this commit, we could define a new
> > device-tree property named, for example,
> > "use-modem-control-lines", so that both options can be supported.
> > 
> > Fixes: 679875d1d880 ("sc16is7xx: Separate GPIOs from modem control
> > lines")
> 
> Please do not line-wrap these lines.

Ok.

> > 
> 
> Nor is a blank line needed here.

Ok.

> > Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> > ---
> >  drivers/tty/serial/sc16is7xx.c | 14 ++++----------
> >  1 file changed, 4 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
> > index 5bd98e4316f5..25f1b2f6ec51 100644
> > --- a/drivers/tty/serial/sc16is7xx.c
> > +++ b/drivers/tty/serial/sc16is7xx.c
> > @@ -306,7 +306,6 @@ struct sc16is7xx_devtype {
> >  	char	name[10];
> >  	int	nr_gpio;
> >  	int	nr_uart;
> > -	int	has_mctrl;
> >  };
> >  
> >  #define SC16IS7XX_RECONF_MD		(1 << 0)
> > @@ -447,35 +446,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,
> 
> I think this one line change is all you really need here, right?  the
> otner changes do nothing in this patch, so you should just create a new
> one changing this value.  Oh, and this one:
> 
> >  	.nr_uart	= 1,
> > -	.has_mctrl	= 1,
> >  };
> >  
> >  static const struct sc16is7xx_devtype sc16is752_devtype = {
> >  	.name		= "SC16IS752",
> > -	.nr_gpio	= 0,
> > +	.nr_gpio	= 8,
> 
> right?
> 
> Don't mess with the has_mctrl stuff, that's not relevant here.

Sorry, I just noticed that simply reverting commit 679875d1d880 is not sufficient (and will not compile). We must also revert part of commit:
21144bab4f11 ("sc16is7xx: Handle modem status lines").

The problem is that the commit 679875d1d880 was incomplete, and it was (unfortunately) completed by integrating it in commit 21144bab4f11 ("sc16is7xx: Handle modem status lines"). The relevant change was only these 5 new lines, burried deeply into the second commit: 

@@ -1353,9 +1452,17 @@ static int sc16is7xx_probe(struct device *dev,
                sc16is7xx_port_write(&s->p[i].port, SC16IS7XX_EFCR_REG,
                                     SC16IS7XX_EFCR_RXDISABLE_BIT |
                                     SC16IS7XX_EFCR_TXDISABLE_BIT);
+
+               /* Use GPIO lines as modem status registers */
+               if (devtype->has_mctrl)
+                       sc16is7xx_port_write(&s->p[i].port,
+                                            SC16IS7XX_IOCONTROL_REG,
+                                            SC16IS7XX_IOCONTROL_MODEM_BIT);
+

Therefore, I should also remove these lines if we go forward with a revert of the patch (should I add another tag "Fixes..." in that case?).

And what do you think of my proposal to maybe replace has_mctrl with a device tree property so that both modes can be fully supported?

Thank you,
Hugo.