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;
>> + }
