Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH v2] serial: amba-pl011: add RS485 support
To:     Greg KH <gregkh@linuxfoundation.org>
Cc:     linux@armlinux.org.uk, jirislaby@kernel.org,
        linux-serial@vger.kernel.org, linux-kernel@vger.kernel.org
References: <20210618145153.1906-1-LinoSanfilippo@gmx.de>
 <YNSA1H0cFKiPUn6N@kroah.com>
From:   Lino Sanfilippo <LinoSanfilippo@gmx.de>
Message-ID: <5d7a4351-2adc-ea31-3290-91d91bd5a5d4@gmx.de>
Date:   Fri, 25 Jun 2021 02:15:13 +0200
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: <YNSA1H0cFKiPUn6N@kroah.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: quoted-printable
Precedence: bulk


Hi,

On 24.06.21 at 14:55, Greg KH wrote:
>>
>> +static int pl011_rs485_tx_stop(struct uart_amba_port *uap)
>> +{
>> +	struct uart_port *port =3D &uap->port;
>> +	u32 cr;
>> +
>> +	/* Wait until hardware tx queue is empty */
>> +	while (!pl011_tx_empty(port))
>> +		udelay(uap->rs485_tx_drain_interval);
>
> No way out if the hardware doesn't ever empty?  Shouldn't you have an
> "upper bound" on this loop somehow?

Yes, indeed. I will fix this.

>
>> +
>> +	if (port->rs485.delay_rts_after_send)
>> +		mdelay(port->rs485.delay_rts_after_send);
>> +
>> +	cr =3D pl011_read(uap, REG_CR);
>> +
>> +	if (port->rs485.flags & SER_RS485_RTS_AFTER_SEND)
>> +		cr &=3D ~UART011_CR_RTS;
>> +	else
>> +		cr |=3D UART011_CR_RTS;
>
> Blank line here please.

Ok.

>
>> +	/* Disable the transmitter and reenable the transceiver */
>> +	cr &=3D ~UART011_CR_TXE;
>> +	cr |=3D UART011_CR_RXE;
>> +	pl011_write(cr, uap, REG_CR);
>> +
>> +	uap->rs485_tx_started =3D false;
>> +
>> +	return 0;
>
> Why does this function return a value if it can not fail and you do not
> check the return value of it?

>> +}
>> +
>>  static void pl011_stop_tx(struct uart_port *port)
>>  {
>>  	struct uart_amba_port *uap =3D
>> @@ -1290,6 +1322,9 @@ static void pl011_stop_tx(struct uart_port *port)
>>  	uap->im &=3D ~UART011_TXIM;
>>  	pl011_write(uap->im, uap, REG_IMSC);
>>  	pl011_dma_tx_stop(uap);
>> +
>> +	if ((port->rs485.flags & SER_RS485_ENABLED) && uap->rs485_tx_started)
>> +		pl011_rs485_tx_stop(uap);
>
> So, no check :(
>

Ah, right. The return value is a leftover from an earlier version of the f=
unction. I will
correct this in the next patch version.

>
>>  }
>>
>>  static bool pl011_tx_chars(struct uart_amba_port *uap, bool from_irq);
>> @@ -1380,6 +1415,31 @@ static bool pl011_tx_char(struct uart_amba_port =
*uap, unsigned char c,
>>  	return true;
>>  }
>>
>> +static void pl011_rs485_tx_start(struct uart_amba_port *uap)
>> +{
>> +	struct uart_port *port =3D &uap->port;
>> +	u32 cr;
>> +
>> +	/* Enable transmitter */
>> +	cr =3D pl011_read(uap, REG_CR);
>> +	cr |=3D UART011_CR_TXE;
>
> Blank line please.
>

Ok.

>> +
>>  	spin_lock_irqsave(&port->lock, flags);
>>
>>  	/*
>>  	 * Update the per-port timeout.
>>  	 */
>>  	uart_update_timeout(port, termios->c_cflag, baud);
>
> Blank line
>

Ok.

>>
>> +static int pl011_rs485_config(struct uart_port *port,
>> +			      struct serial_rs485 *rs485)
>> +{
>> +	struct uart_amba_port *uap =3D
>> +		container_of(port, struct uart_amba_port, port);
>> +
>> +	/* pick sane settings if the user hasn't */
>> +	if (!!(rs485->flags & SER_RS485_RTS_ON_SEND) =3D=3D
>
> Why the !! in an if statement?
>
>> +	    !!(rs485->flags & SER_RS485_RTS_AFTER_SEND)) {
>
> Same here, why?
>

This was copied from serial8250_em485_config(). But I think we can simply =
use

	if (rs485->flags & SER_RS485_RTS_AFTER_SEND)
		rs485->flags &=3D ~SER_RS485_RTS_ON_SEND;
	else
		rs485->flags |=3D SER_RS485_RTS_ON_SEND;

instead. I will adjust the code accordingly.

>> +
>> +	if (port->rs485.flags & SER_RS485_ENABLED)
>> +		pl011_rs485_tx_stop(uap);
>> +
>> +	/* Set new configuration */
>> +	port->rs485 =3D *rs485;
>
> Blank line please.
>

Ok.

>
> thanks,
>
> greg k-h
>

Thank you for the review!

Regards,
Lino