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

[Lech Perczak]

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

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'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