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
