On Mon, Jul 29, 2019 at 12:03:07PM +0300, Sergey Organov wrote: > Uwe Kleine-König <u.kleine-koenig@xxxxxxxxxxxxxx> 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. They both handle mctrl-gpio, the autorts stuff isn't available for that, so we could fix that by letting rts-auto set the RTS gpio to active. > > 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. *shrug* I stop caring here. Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | http://www.pengutronix.de/ |
