Message ID | 20230704135936.14697-1-ddrokosov@sberdevices.ru |
---|---|
Headers | show |
Series | tty: serial: meson: support ttyS devname | expand |
On Tue, Jul 04, 2023 at 06:19:20PM +0300, Dmitry Rokosov wrote: > On Tue, Jul 04, 2023 at 04:46:40PM +0200, neil.armstrong@linaro.org wrote: > > On 04/07/2023 15:59, Dmitry Rokosov wrote: > > > Actually, the meson_uart module is already a platform_driver, but it is > > > currently registered manually and the uart core registration is run > > > outside the probe() scope, which results in some restrictions. For > > > instance, it is not possible to communicate with the OF subsystem > > > because it requires an initialized device object. > > > > > > To address this issue, apply module_platform_driver() instead of direct > > > module init/exit routines. Additionally, move uart_register_driver() to > > > the driver probe(), and destroy manual console registration because it's > > > already run in the uart_register_driver() flow. > > > > > > Signed-off-by: Dmitry Rokosov <ddrokosov@sberdevices.ru> > > > --- > > > drivers/tty/serial/meson_uart.c | 46 +++++++-------------------------- > > > 1 file changed, 10 insertions(+), 36 deletions(-) > > > > > > diff --git a/drivers/tty/serial/meson_uart.c b/drivers/tty/serial/meson_uart.c > > > index 169f028956ae..87c0eb5f2dba 100644 > > > --- a/drivers/tty/serial/meson_uart.c > > > +++ b/drivers/tty/serial/meson_uart.c > > > @@ -621,12 +621,6 @@ static struct console meson_serial_console = { > > > .data = &meson_uart_driver, > > > }; > > > -static int __init meson_serial_console_init(void) > > > -{ > > > - register_console(&meson_serial_console); > > > - return 0; > > > -} > > > - > > > static void meson_serial_early_console_write(struct console *co, > > > const char *s, > > > u_int count) > > > @@ -652,9 +646,6 @@ OF_EARLYCON_DECLARE(meson, "amlogic,meson-ao-uart", > > > #define MESON_SERIAL_CONSOLE (&meson_serial_console) > > > #else > > > -static int __init meson_serial_console_init(void) { > > > - return 0; > > > -} > > > #define MESON_SERIAL_CONSOLE NULL > > > #endif > > > @@ -738,6 +729,13 @@ static int meson_uart_probe(struct platform_device *pdev) > > > if (ret) > > > return ret; > > > + if (!meson_uart_driver.state) { > > > + ret = uart_register_driver(&meson_uart_driver); > > > + if (ret) > > > + return dev_err_probe(&pdev->dev, ret, > > > + "failed to register meson-uart driver\n"); > > > + } > > > > PL010 protects this in a mutex, and I think you should do the same otherwise > > if multiple uart probes at the same it will do weird stuff. > > > > Looks like that not all drivers protect this location with a specialized > mutex object. Firstly, I think it's important to verify parallel probe() > calling and implementing mutex protection at the platform core level. > For example, I've faced with the same problem during regmap mutex based > protection. > Upon examining the core logic in drivers/base/dd.c, I have observed that driver_probe_device() is consistently executed under the device_lock(). This lock is already based on a mutex, thus ensuring parallel execution protection: https://elixir.bootlin.com/linux/latest/source/include/linux/device.h#L835 > > > + > > > port->iotype = UPIO_MEM; > > > port->mapbase = res_mem->start; > > > port->mapsize = resource_size(res_mem); > > > @@ -776,6 +774,8 @@ static int meson_uart_remove(struct platform_device *pdev) > > > uart_remove_one_port(&meson_uart_driver, port); > > > meson_ports[pdev->id] = NULL; > > > + uart_unregister_driver(&meson_uart_driver); > > > + > > > > This is dangerous, it will remove the driver even if some uart are still attached to it. > > > > You should probably do like in pl010_remove() and remove only if the last one is removed. > > > > Indeed... multiple ports can be registered... > > > > return 0; > > > } > > > @@ -809,33 +809,7 @@ static struct platform_driver meson_uart_platform_driver = { > > > }, > > > }; > > > -static int __init meson_uart_init(void) > > > -{ > > > - int ret; > > > - > > > - ret = meson_serial_console_init(); > > > - if (ret) > > > - return ret; > > > - > > > - ret = uart_register_driver(&meson_uart_driver); > > > - if (ret) > > > - return ret; > > > - > > > - ret = platform_driver_register(&meson_uart_platform_driver); > > > - if (ret) > > > - uart_unregister_driver(&meson_uart_driver); > > > - > > > - return ret; > > > -} > > > - > > > -static void __exit meson_uart_exit(void) > > > -{ > > > - platform_driver_unregister(&meson_uart_platform_driver); > > > - uart_unregister_driver(&meson_uart_driver); > > > -} > > > - > > > -module_init(meson_uart_init); > > > -module_exit(meson_uart_exit); > > > +module_platform_driver(meson_uart_platform_driver); > > > > Only pl010 uses this scheme, and I don't know why... if it works then it's ok for me. > > From my point of view, the "scheme" is using uart driver registration > from the probe() routine. Many drivers are based on such approach: > samsung-tty, timbuart, sprd, max3100, etc. Some of them are platform > drivers as well. > > > > MODULE_AUTHOR("Carlo Caione <carlo@caione.org>"); > > > MODULE_DESCRIPTION("Amlogic Meson serial port driver"); > > > > -- > Thank you, > Dmitry