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

From: Hugo Villeneuve
Date: Tue May 16 2023 - 18:09:59 EST

On Tue, 16 May 2023 11:59:06 -0400
Hugo Villeneuve <hugo@xxxxxxxxxxx> wrote:

> On Tue, 16 May 2023 10:50:11 +0200
> Lech Perczak <lech.perczak@xxxxxxxxxxxxxxx> wrote:
>
> > Hi Hugo,
> >
> > Please see my answers inline.
> >
> > W dniu 15.05.2023 o 18:51, Hugo Villeneuve pisze:
> > > 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:
> > Just as you noticed, this was required to support full set of flow control lines on this device.
> > The commit you're trying to revert was a preparation for it. Disabling has_mctrl will break it.
> > I kindly suggest to suggest a fix, instead of hurrying a revert, and waiting for a proper fix later.
>
> Hi Lech,
> the [RFC] in the subject was there to discuss about a possible revert, and/or maybe a possible fix that would allow both modes to be supported. I am not hurrying anything and I am certainly not waiting for a later fix, as I very much want to help and maybe submit such a fix myself.
>
> But the reality is that commits 679875d1d880/21144bab4f11 broke userspace by forcing GPIOs as modem control lines. I understand that reverting these patches could also potentially break things for applications depending on these patches. I am simply wondering what is the proper course of action here: revert patches and work on a fix to support both modes, or skip revert and work on a fix (my preference)?
>
> > > @@ -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?
> > I think the proper solution here, is not to invent a new device tree property for every single use case.
> > I would start by looking for other drivers, if, and how they handle similar cases.
> > For example, imx-serial driver respects "uart-has-rtscts" property, as do a lot of other controllers built into SoC-s.
> > On the other hand, other devices which can also provide GPIOs, respect "gpio-controller" property.
>
> I think that testing the presence of the "uart-has-rtscts" to force GPIOs as modem control lines would make a lot of sense.
>
> > According to SC16IS752 datasheet [1], respecting one of those should be enough,
> > as GPIOs can be enabled in groups of four pins even for dual UART version.
> > Every group matches a single port, so probably this can be probably selected per UART even on dual-port versions.
>
> I am trying to see how we could set "uart-has-rtscts" for only UART channel A or B in the device tree, but cannot find any example or documentation about that. How do you propose to do it?
>
> From what I understand, the property "uart-has-rtscts" can be set only for the whole chip (channels A and B)...

After some analysis, I don't think that we should be using the property "uart-has-rtscts". For our chip, this doesn't make sense because RTS/CTS are dedicated pins. also, like I said, this property applies to the whole chip/device, not to indivual A or B channels (like sc16is752).

The way to go would be to define a new DT property similar to "irda-mode-ports" (for the same sc16is7xx driver). Defining a new property named "modem-control-line-ports" would allow us to specify an array that lists the indices of the port that should have shared GPIO lines configured as modem control lines.

I already implemented that as a proof of concept and it works great.

Hugo.


> > I'll be more than happy to assist with that.
> >
> > >
> > > Thank you,
> > > Hugo.
> > >
> > [1] https://www.nxp.com/docs/en/data-sheet/SC16IS752_SC16IS762.pdf
> >
> > --
> > Pozdrawiam/With kind regards,
> > Lech Perczak
> >
> > Sr. Software Engineer
> > Camlin Technologies Poland Limited Sp. z o.o.
> > Strzegomska 54,
> > 53-611 Wroclaw
> > Tel: (+48) 71 75 000 16
> > Email: lech.perczak@xxxxxxxxxxxxxxx
> > Website: http://www.camlingroup.com
> >
> >
>
>
> --
> Hugo Villeneuve
>


--
Hugo Villeneuve