> -----Original Message----- > From: Thierry Reding <[email protected]> > Sent: Tuesday, August 13, 2019 3:50 PM > To: Krishna Yarlagadda <[email protected]> > Cc: [email protected]; [email protected]; > [email protected]; Jonathan Hunter <[email protected]>; Laxman > Dewangan <[email protected]>; [email protected]; linux- > [email protected]; [email protected]; linux- > [email protected]; [email protected]; Shardar Mohammed > <[email protected]> > Subject: Re: [PATCH 09/14] serial: tegra: set maximum num of uart > ports to 8 > > On Mon, Aug 12, 2019 at 04:58:18PM +0530, Krishna Yarlagadda wrote: > > From: Shardar Shariff Md <[email protected]> > > > > Set maximum number of UART ports to 8 as older chips have 7 ports > > and > > Tergra194 and later chips will have 8 ports. Add this info to chip > > data and register uart driver in platform driver probe. > > > > Signed-off-by: Shardar Shariff Md <[email protected]> > > Signed-off-by: Krishna Yarlagadda <[email protected]> > > --- > > drivers/tty/serial/serial-tegra.c | 21 +++++++++++++-------- > > 1 file changed, 13 insertions(+), 8 deletions(-) > > > > diff --git a/drivers/tty/serial/serial-tegra.c > > b/drivers/tty/serial/serial-tegra.c > > index e0379d9..329923c 100644 > > --- a/drivers/tty/serial/serial-tegra.c > > +++ b/drivers/tty/serial/serial-tegra.c > > @@ -62,7 +62,7 @@ > > #define TEGRA_UART_TX_TRIG_4B 0x20 > > #define TEGRA_UART_TX_TRIG_1B 0x30 > > > > -#define TEGRA_UART_MAXIMUM 5 > > +#define TEGRA_UART_MAXIMUM 8 > > > > /* Default UART setting when started: 115200 no parity, stop, 8 data bits > > */ > > #define TEGRA_UART_DEFAULT_BAUD 115200 > > @@ -87,6 +87,7 @@ struct tegra_uart_chip_data { > > bool allow_txfifo_reset_fifo_mode; > > bool support_clk_src_div; > > bool fifo_mode_enable_status; > > + int uart_max_port; > > }; > > > > struct tegra_uart_port { > > @@ -1323,6 +1324,7 @@ static struct tegra_uart_chip_data > tegra20_uart_chip_data = { > > .allow_txfifo_reset_fifo_mode = true, > > .support_clk_src_div = false, > > .fifo_mode_enable_status = false, > > + .uart_max_port = 5, > > }; > > > > static struct tegra_uart_chip_data tegra30_uart_chip_data = { @@ > > -1330,6 +1332,7 @@ static struct tegra_uart_chip_data > tegra30_uart_chip_data = { > > .allow_txfifo_reset_fifo_mode = false, > > .support_clk_src_div = true, > > .fifo_mode_enable_status = false, > > + .uart_max_port = 5, > > }; > > > > static struct tegra_uart_chip_data tegra186_uart_chip_data = { @@ > > -1337,6 +1340,7 @@ static struct tegra_uart_chip_data > tegra186_uart_chip_data = { > > .allow_txfifo_reset_fifo_mode = false, > > .support_clk_src_div = true, > > .fifo_mode_enable_status = true, > > + .uart_max_port = 5, > > You say in the commit message that the older chips have 7 ports, but > here you say they have 5. Which one is it? > > > }; > > > > static const struct of_device_id tegra_uart_of_match[] = { @@ > > -1386,6 > > +1390,7 @@ static int tegra_uart_probe(struct platform_device *pdev) > > u->type = PORT_TEGRA; > > u->fifosize = 32; > > tup->cdata = cdata; > > + tegra_uart_driver.nr = cdata->uart_max_port; > > > > platform_set_drvdata(pdev, tup); > > resource = platform_get_resource(pdev, IORESOURCE_MEM, 0); > @@ > > -1411,6 +1416,13 @@ static int tegra_uart_probe(struct > > platform_device > *pdev) > > return PTR_ERR(tup->rst); > > } > > > > + ret = uart_register_driver(&tegra_uart_driver); > > + if (ret < 0) { > > + pr_err("Could not register %s driver\n", > > + tegra_uart_driver.driver_name); > > + return ret; > > + } > > I don't think this is the right place for this. You're going to try to > register the driver once for each instance of the Tegra UART that will be > probed. > > I'm surprised that this works at all because there's a BUG_ON() early > in > uart_register_driver() that checks for the existence of drv->state, > which means that the second instance of tegra_uart_probe() should > trigger that and cause the kernel to crash. > > I think it's better to either create an additional of_device_id table > that is used to match on the top-level node's compatible string and > which only contains the maximum number of ports for the given SoC, or > you could add code to > tegra_uart_init() that counts the number of ports that do match and > initialize tegra_uart_driver.nr using that number. It would something like > this: > > unsigned int count = 0; > > for_each_matching_node(np, &tegra_uart_of_match) > count++; > > tegra_uart_driver.nr = count; > > You could also add additional checks in the loop, perhaps something > like: > > for_each_matching_node(np, &tegra_uart_of_match) > if (of_device_is_available(np)) > count++ > > Though that would prevent any UARTs from getting added via dynamic > device tree manipulation. > > Thierry > Multiple port entries does result in failures which I missed. I will fix this as suggested. KY > > + > > u->iotype = UPIO_MEM32; > > ret = platform_get_irq(pdev, 0); > > if (ret < 0) { > > @@ -1472,13 +1484,6 @@ static int __init tegra_uart_init(void) { > > int ret; > > > > - ret = uart_register_driver(&tegra_uart_driver); > > - if (ret < 0) { > > - pr_err("Could not register %s driver\n", > > - tegra_uart_driver.driver_name); > > - return ret; > > - } > > - > > ret = platform_driver_register(&tegra_uart_platform_driver); > > if (ret < 0) { > > pr_err("Uart platform driver register failed, e = %d\n", ret); > > -- > > 2.7.4 > >
