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

[Hugo Villeneuve]

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)...

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