Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
Subject: Re: [PATCH 1/9] serial: core: move RS485 configuration tasks from
 drivers into core
To:     Jiri Slaby <jirislaby@kernel.org>, gregkh@linuxfoundation.org
Cc:     linux@armlinux.org.uk, richard.genoud@gmail.com,
        nicolas.ferre@microchip.com, alexandre.belloni@bootlin.com,
        ludovic.desroches@microchip.com, shawnguo@kernel.org,
        s.hauer@pengutronix.de, kernel@pengutronix.de, festevam@gmail.com,
        linux-imx@nxp.com, mcoquelin.stm32@gmail.com,
        alexandre.torgue@foss.st.com, linux-serial@vger.kernel.org,
        linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
        linux-stm32@st-md-mailman.stormreply.com, lukas@wunner.de
References: <20220213222737.15709-1-LinoSanfilippo@gmx.de>
 <20220213222737.15709-2-LinoSanfilippo@gmx.de>
 <aa45fed9-7a40-7ac1-a000-18d2805d088f@kernel.org>
From:   Lino Sanfilippo <LinoSanfilippo@gmx.de>
Message-ID: <9791fb9a-0c27-bfeb-5ff8-fb70f1968048@gmx.de>
Date:   Mon, 14 Feb 2022 17:13:20 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.10.0
MIME-Version: 1.0
In-Reply-To: <aa45fed9-7a40-7ac1-a000-18d2805d088f@kernel.org>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: quoted-printable
Precedence: bulk


Hi,

On 14.02.22 at 06:41, Jiri Slaby wrote:
> On 13. 02. 22, 23:27, Lino Sanfilippo wrote:
>> Several drivers that support setting the RS485 configuration via usersp=
ace
>> implement on or more of the following tasks:
>>
>> - in case of an invalid RTS configuration (both RTS after send and RTS =
on
>> =C2=A0=C2=A0 send set or both unset) fall back to enable RTS on send an=
d disable RTS
>> =C2=A0=C2=A0 after send
>>
>> - nullify the padding field of the returned serial_rs485 struct
>>
>> - copy the configuration into the uart port struct
>>
>> - limit RTS delays to 100 ms
>>
>> Move these tasks into the serial core to make them generic and to provi=
de
>> a consistent beheviour among all drivers.
>>
>> Signed-off-by: Lino Sanfilippo <LinoSanfilippo@gmx.de>
>> ---
>> =C2=A0 drivers/tty/serial/serial_core.c | 13 +++++++++++++
>> =C2=A0 1 file changed, 13 insertions(+)
>>
>> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/seri=
al_core.c
>> index 846192a7b4bf..3fab4070359c 100644
>> --- a/drivers/tty/serial/serial_core.c
>> +++ b/drivers/tty/serial/serial_core.c
>> @@ -1282,8 +1282,21 @@ static int uart_set_rs485_config(struct uart_por=
t *port,
>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 if (copy_from_user(&rs485, rs485_user, s=
izeof(*rs485_user)))
>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 return -EFAULT;
>> =C2=A0 +=C2=A0=C2=A0=C2=A0 /* pick sane settings if the user hasn't */
>> +=C2=A0=C2=A0=C2=A0 if (!(rs485.flags & SER_RS485_RTS_ON_SEND) =3D=3D
>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 !(rs485.flags & SER_RS485_R=
TS_AFTER_SEND)) {
>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 rs485.flags |=3D SER_RS485_=
RTS_ON_SEND;
>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 rs485.flags &=3D ~SER_RS485=
_RTS_AFTER_SEND;
>> +=C2=A0=C2=A0=C2=A0 }
>> +=C2=A0=C2=A0=C2=A0 /* clamp the delays to [0, 100ms] */
>> +=C2=A0=C2=A0=C2=A0 rs485.delay_rts_before_send =3D min(rs485.delay_rts=
_before_send, 100U);
>> +=C2=A0=C2=A0=C2=A0 rs485.delay_rts_after_send =3D min(rs485.delay_rts_=
after_send, 100U);
>
> Why is this magic 100?

The only drivers that seem to care about a max value for the RTS delays us=
e 100 ms
(omap-serial, amba pl011, 8250) so I chose this to stay compatible with th=
e current
driver implementations. 100 ms also seems large enough to be used as a gen=
eral max value.

> Can we have that number somehow documented? You should define a macro fo=
r that anyway.

Ok, I will do so.

>
>> +=C2=A0=C2=A0=C2=A0 memset(rs485.padding, 0, sizeof(rs485.padding));
>
> What is this memset good for?

Drivers like max310x, amba-pl011, 8250_pci, 8250_fintek, 8250_lpc18xx seem=
 to care about
returning a serial_rs485 struct with cleared padding field to userspace. S=
o they all clear
that field on their own. Although not really necessary, to me this seems t=
o be a good
default behavior, so I added it to the serial core.

>
>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 spin_lock_irqsave(&port->lock, flags);
>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 ret =3D port->rs485_config(port, &rs485)=
;
>> +=C2=A0=C2=A0=C2=A0 if (!ret)
>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 port->rs485 =3D rs485;
>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 spin_unlock_irqrestore(&port->lock, flag=
s);
>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 if (ret)
>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 return ret;
>
> thanks,

Thanks for the review!

Regards,
Lino