Hi Greg,
On Mon, 15 May 2023 18:20:02 +0200
Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
> On Mon, May 15, 2023 at 12:02:07PM -0400, Hugo Villeneuve wrote:
> > From: Hugo Villeneuve <hvilleneuve@xxxxxxxxxxxx>
> >
> > 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@xxxxxxxxxxxx>
> > ---
> > 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.
