Re: [PATCH v7 3/3] serial: imx: get rid of imx_uart_rts_auto()

[Sergey Organov <sorganov@gmail.com>]

Uwe Kleine-König <u.kleine-koenig@pengutronix.de> writes:

> On Fri, Jul 26, 2019 at 09:52:41PM +0300, Sergey Organov wrote:
>> Called in only one place, for RS232, it only obscures things, as it
>> doesn't go well with 2 similar named functions,
>> imx_uart_rts_inactive() and imx_uart_rts_active(), that both are
>> RS485-specific.
>
> I don't share the critic. IMHO the name is fine. imx_uart_rts_inactive
> sets rts to its inactive level,
> imx_uart_rts_active() to its active level

Not exactly, in fact both do more than that, in a similar manner.

> imx_uart_rts_auto() lets the output drive automatically by the
> receiver.

And this one was different and it was rather confusing when I've tried
to grok the logic of the driver.

> The name started to be a bit wrong in patch 1 of the series however.

The function was different from first two even before the patch, as it
does not do any of those additional things the first two do.

> And I still object removing this function because with the semantic
> this function got in patch 1 it is suiteable to be used in
> imx_uart_set_mctrl().

It is not, as it does require change to be used there, as we've already
seen, and then it becomes very different function from what it was at
the beginning.

Even then, the end result I've shown you when attempting to somehow preserve
some re-incarnation of this function still seems more cumbersome to me
than the end result of these patches.

That said, this a matter of taste and style, not correctness, and could
be changed as a follow-up, not to risk breaking already tested patch
series.

Thanks,

-- Sergey

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel