Message-Id: <20230517095831.5bde5dc2671e98c36631888a@hugovil.com>
Date:   Wed, 17 May 2023 09:58:31 -0400
From:   Hugo Villeneuve <hugo@...ovil.com>
To:     Lech Perczak <lech.perczak@...lingroup.com>
Cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Jiri Slaby <jirislaby@...nel.org>,
        Lech Perczak <l.perczak@...lintechnologies.com>,
        Tomasz Moń <tomasz.mon@...lingroup.com>,
        Hugo Villeneuve <hvilleneuve@...onoff.com>,
        linux-serial@...r.kernel.org, linux-kernel@...r.kernel.org,
        Krzysztof Drobiński 
        <krzysztof.drobinski@...lingroup.com>, hugo@...ovil.com
Subject: Re: [RFC PATCH] Revert
 "sc16is7xx: Separate GPIOs from modem control lines"

On Wed, 17 May 2023 11:18:57 +0200
Lech Perczak <lech.perczak@...lingroup.com> wrote:

> 
> W dniu 17.05.2023 o 00:09, Hugo Villeneuve pisze:
> > On Tue, 16 May 2023 11:59:06 -0400
> > Hugo Villeneuve <hugo@...ovil.com> wrote:
> >
> > > On Tue, 16 May 2023 10:50:11 +0200
> > > Lech Perczak <lech.perczak@...lingroup.com> 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@...uxfoundation.org> wrote:
> > > > >
> > > > >> On Mon, May 15, 2023 at 12:02:07PM -0400, Hugo Villeneuve wrote:
> > > > >>> From: Hugo Villeneuve <hvilleneuve@...onoff.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@...onoff.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:
> > > > 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).
> 
> Hi Hugo,
> 
> That's correct, my idea was to analyze what is available and pick the best one, if at all possible.
> "gpio-controller" could also be used - in theory - but it isn't a great choice either, because it doesn't allow us to specify, which pin groups should be used as GPIOs.
> >
> > 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.
> 
> There is nothing wrong per se in adding new device tree property, I'd just like to avoid jumping the gun in inventing one.
> After quickly reviewing documentation of available bindings I see that it's most likely unavoidable.
> The "modem-control-line-ports" proposal with a mask of ports sounds very reasonable - please share your PoC,
> it will be easier to discuss having a concrete example.
> 
> The general approach I noticed in other places (for example, the WF8960 audio codec) is setting a register value directly.
> This would allow us to control the IOLATCH bit in IOControl register, to make inputs register behave as interrupt flag register,
> but I think that if we ever need it, it would be cleaner to set it with a separate boolean property - I'm in favor of modem-control-line-ports.

Hi Lech,
I think that control of the IOLATCH bit should be part of a separate discussion (patch) if we want to change it. For the moment, I just want to restore the GPIO function as it was before your patches.

> I think it would be a good idea to include DT and GPIO maintainers and mailing lists as well.
> Especially the DT maintainers - they would like to see this property documented. They however, are not concerned with the code changes themselves.

Ok, I will submit my PoC soon.

Thank you, Hugo.

> BTW, the commit split between adding has_mctrl property and the rest of implementation warrants some explanation - this was based on my previous patches,
> which Tomasz reworked and submitted. The split was kept to split up the changes to minimal, logical chunks, and to maintain correct authorship of the changes.
> 
> With kind regards,
> Lech
> >
> > 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 <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@...lingroup.com
> > > > Website: http://www.camlingroup.com <http://www.camlingroup.com>
> > > >
> > > >
> > >
> > >
> > > --
> > > Hugo Villeneuve
> > >
> >
> >
> > -- 
> > Hugo Villeneuve
> 
> 

Powered by blists - more mailing lists