On Wed, May 06, 2020 at 08:29:43AM +0200, Lukas Wunner wrote: > On Tue, May 05, 2020 at 07:10:35PM +0300, Andy Shevchenko wrote: > > On Tue, May 05, 2020 at 04:42:04PM +0200, Lukas Wunner wrote: ... > > > + devm_gpiod_put(dev, port->rs485_term_gpio); > > > > > + port->rs485_term_gpio = devm_gpiod_get_optional(dev, "rs485-term", > > > > Using devm_*() in uart_get_rs485_mode() seems not right. > > Why do you need this? > > uart_get_rs485_mode() is called from a driver's ->probe() hook and we > do not have a corresponding function that is called from a ->remove() > hook where we'd be able to relinquish rs485 resources we've acquired > on probe. > > Of course I could add that but it would be more heavy-weight compared > to simply using devm_*(). Do you disagree? > > devm_gpiod_put() isn't strictly necessary here. It is only necessary > if one of the drivers would invoke uart_get_rs485_mode() multiple > times, which none of them does AFAICS. It's just a safety measure. > I can drop it if that is preferred. I think putting and re-requesting here is also racy. Somebody can request the very same GPIO in between (for example crazy user space tool). Setting the same value many times won't hurt. > > > + GPIOD_FLAGS_BIT_DIR_SET | GPIOD_FLAGS_BIT_DIR_OUT); > > > > Parameter has a specific macro GPIOD_OUT_HIGH. > > Good point. It's also occurred to me now that reading the GPIO's > value after changing its direction to output is nonsense. If anything > it ought to be read *before* changing the direction to output. It's not a complete nonsense, depends what you actually want to achieve here. > That would make sense in case the board has a pullup or pulldown on > the Termination Enable pin. In other cases the pin may just float > and the value will be unpredictable. However if I do not read the > pin, I'd have to choose either high or low as initial state. Hm. > Let me check back with our hardware engineers today and see what they > recommend. -- With Best Regards, Andy Shevchenko
