Re: [PATCH 1/3] serial: 8250: Handle UART without interrupt on TEMT using em485

[Eric Tremblay]

On 2021-01-29 6:22 a.m., Andy Shevchenko wrote:
> On Thu, Jan 28, 2021 at 06:36:27PM -0500, Eric Tremblay wrote:
>> The patch introduce the UART_CAP_TEMT capability which is by default
>> assigned to all 8250 UART since the code assume that device has the
>> interrupt on TEMT
> You have missed periods in the sentences here and there. Please, check the
> grammar and punctuation everywhere.
>
>> In the case where the device does not support it, we calculate the
>> maximum of time it could take for the transmitter to empty the
> maximum time
>
>> shift register. When we get in the situation where we get the
>> THRE interrupt but the TEMT bit is not set we start the timer
>> and recall __stop_tx after the delay
> __stop_tx()

I will review the grammar and spelling, thanks for mentioning it

>
> ...
>
>>  	/* initialize data */
>> -	up.capabilities = UART_CAP_FIFO | UART_CAP_MINI;
>> +	data->uart.capabilities = UART_CAP_FIFO | UART_CAP_MINI | UART_CAP_TEMT;
> I didn't get, if you state that CAP_TEMT is default on all UARTs, why you have
> this?

It's a merge mistake, sorry for that. The next version will use the 
reverse capability like Jiri Slaby suggested, there will be no needs to
modify other driver.

>
>> -	up.capabilities = UART_CAP_FIFO;
>> +	up.capabilities = UART_CAP_FIFO | UART_CAP_TEMT;
> And so this?
>
> ...
>
>> +static inline void serial8250_em485_update_temt_delay(struct uart_8250_port *p,
>> +			unsigned int cflag, unsigned int baud)
>> +{
>> +	unsigned int bits;
>> +
>> +	if (!p->em485)
>> +		return;
>> +
>> +	/* byte size and parity */
>> +	switch (cflag & CSIZE) {
>> +	case CS5:
>> +		bits = 7;
>> +		break;
>> +	case CS6:
>> +		bits = 8;
>> +		break;
>> +	case CS7:
>> +		bits = 9;
>> +		break;
>> +	default:
>> +		bits = 10;
>> +		break; /* CS8 */
>> +	}
>> +
>> +	if (cflag & CSTOPB)
>> +		bits++;
>> +	if (cflag & PARENB)
>> +		bits++;
> This is repetition of uart_update_timeout(). Find a way to deduplicate.
>
>> +	p->em485->no_temt_delay = bits*1000000/baud;
> Use spaces.
> Is this magic should be defined as HZ_PER_MHZ?
>
>> +}
> ...
>
>> +static void start_hrtimer_us(struct hrtimer *hrt, unsigned long usec)
>> +{
>> +	long sec = usec / 1000000;
>> +	long nsec = (usec % 1000000) * 1000;
>
> USEC_PER_SEC
> NSEC_PER_USEC
>
>> +	ktime_t t = ktime_set(sec, nsec);
>> +
>> +	hrtimer_start(hrt, t, HRTIMER_MODE_REL);
>> +}
> ...
>
>> +		if ((lsr & BOTH_EMPTY) != BOTH_EMPTY) {
>> +			/*
>> +			 * On devices with no interrupt on TEMT available
> "with no TEMT interrupt available"
>
>> +			 * start a timer for a byte time, the timer will recall
>> +			 * __stop_tx
> __stop_tx().
>
>> +			 */
>> +			if (!(p->capabilities & UART_CAP_TEMT) && (lsr & UART_LSR_THRE)) {
>> +				em485->active_timer = &em485->no_temt_timer;
>> +				start_hrtimer_us(&em485->no_temt_timer, em485->no_temt_delay);
>> +			}
> Perhaps
> 			if ((p->capabilities & UART_CAP_TEMT) && (lsr & UART_LSR_THRE))
> 				return;
>
> 			em485->active_timer = &em485->no_temt_timer;
> 			start_hrtimer_us(&em485->no_temt_timer, em485->no_temt_delay);
>
> ?

I also prefer that form, I will apply it in next version

>
>>  			return;
>> +		}