[PATCH v2 1/2] serial: sprd: getting port index via serial alias only

[PATCH v2 1/2] serial: sprd: getting port index via serial alias only

[Chunyan Zhang]

From: Chunyan Zhang <chunyan.zhang@unisoc.com>

This patch simplifies the process of getting serial port number, with
this patch, serial devices must have alias configured in devicetree.

The serial port searched out via sprd_port array maybe wrong if we don't
have serial alias defined in devicetree, and specify console with command
line, we would get the wrong port number if other serial ports probe
failed before console's. So using alias is mandatory.

Signed-off-by: Chunyan Zhang <chunyan.zhang@unisoc.com>
---
 drivers/tty/serial/sprd_serial.c | 36 +++++---------------------------
 1 file changed, 5 insertions(+), 31 deletions(-)

diff --git a/drivers/tty/serial/sprd_serial.c b/drivers/tty/serial/sprd_serial.c
index 914862844790..9f8c14ff6454 100644
--- a/drivers/tty/serial/sprd_serial.c
+++ b/drivers/tty/serial/sprd_serial.c
@@ -1102,29 +1102,6 @@ static struct uart_driver sprd_uart_driver = {
 	.cons = SPRD_CONSOLE,
 };
 
-static int sprd_probe_dt_alias(int index, struct device *dev)
-{
-	struct device_node *np;
-	int ret = index;
-
-	if (!IS_ENABLED(CONFIG_OF))
-		return ret;
-
-	np = dev->of_node;
-	if (!np)
-		return ret;
-
-	ret = of_alias_get_id(np, "serial");
-	if (ret < 0)
-		ret = index;
-	else if (ret >= ARRAY_SIZE(sprd_port) || sprd_port[ret] != NULL) {
-		dev_warn(dev, "requested serial port %d not available.\n", ret);
-		ret = index;
-	}
-
-	return ret;
-}
-
 static int sprd_remove(struct platform_device *dev)
 {
 	struct sprd_uart_port *sup = platform_get_drvdata(dev);
@@ -1204,14 +1181,11 @@ static int sprd_probe(struct platform_device *pdev)
 	int index;
 	int ret;
 
-	for (index = 0; index < ARRAY_SIZE(sprd_port); index++)
-		if (sprd_port[index] == NULL)
-			break;
-
-	if (index == ARRAY_SIZE(sprd_port))
-		return -EBUSY;
-
-	index = sprd_probe_dt_alias(index, &pdev->dev);
+	index = of_alias_get_id(pdev->dev.of_node, "serial");
+	if (index < 0 || index >= ARRAY_SIZE(sprd_port)) {
+		dev_err(&pdev->dev, "got a wrong serial alias id %d\n", index);
+		return index;
+	}
 
 	sprd_port[index] = devm_kzalloc(&pdev->dev, sizeof(*sprd_port[index]),
 					GFP_KERNEL);
-- 
2.20.1

[PATCH v2 2/2] serial: sprd: cleanup the sprd_port for error case

[Chunyan Zhang]

From: Chunyan Zhang <chunyan.zhang@unisoc.com>

It would be better to cleanup the sprd_port for the device before
return error.

Signed-off-by: Chunyan Zhang <chunyan.zhang@unisoc.com>
---
 drivers/tty/serial/sprd_serial.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serial/sprd_serial.c b/drivers/tty/serial/sprd_serial.c
index 9f8c14ff6454..54477de9822f 100644
--- a/drivers/tty/serial/sprd_serial.c
+++ b/drivers/tty/serial/sprd_serial.c
@@ -1204,8 +1204,10 @@ static int sprd_probe(struct platform_device *pdev)
 	up->has_sysrq = IS_ENABLED(CONFIG_SERIAL_SPRD_CONSOLE);
 
 	ret = sprd_clk_init(up);
-	if (ret)
+	if (ret) {
+		sprd_port[index] = NULL;
 		return ret;
+	}
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	up->membase = devm_ioremap_resource(&pdev->dev, res);
-- 
2.20.1

Re: [PATCH v2 2/2] serial: sprd: cleanup the sprd_port for error case

[Baolin Wang]

On Wed, Mar 18, 2020 at 4:31 PM Chunyan Zhang <zhang.lyra@gmail.com> wrote:
>
> From: Chunyan Zhang <chunyan.zhang@unisoc.com>
>
> It would be better to cleanup the sprd_port for the device before
> return error.
>
> Signed-off-by: Chunyan Zhang <chunyan.zhang@unisoc.com>
> ---
>  drivers/tty/serial/sprd_serial.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/tty/serial/sprd_serial.c b/drivers/tty/serial/sprd_serial.c
> index 9f8c14ff6454..54477de9822f 100644
> --- a/drivers/tty/serial/sprd_serial.c
> +++ b/drivers/tty/serial/sprd_serial.c
> @@ -1204,8 +1204,10 @@ static int sprd_probe(struct platform_device *pdev)
>         up->has_sysrq = IS_ENABLED(CONFIG_SERIAL_SPRD_CONSOLE);
>
>         ret = sprd_clk_init(up);
> -       if (ret)
> +       if (ret) {
> +               sprd_port[index] = NULL;

如果我们强制使用alias, 则这里应该也无需清除了，因为一进probe就会给它重新赋值。 还是我漏了什么？


>                 return ret;
> +       }
>
>         res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>         up->membase = devm_ioremap_resource(&pdev->dev, res);
> --
> 2.20.1
>


--
Baolin Wang

Re: [PATCH v2 2/2] serial: sprd: cleanup the sprd_port for error case

[Chunyan Zhang]

On Wed, 18 Mar 2020 at 17:16, Baolin Wang <baolin.wang7@gmail.com> wrote:
>
> On Wed, Mar 18, 2020 at 4:31 PM Chunyan Zhang <zhang.lyra@gmail.com> wrote:
> >
> > From: Chunyan Zhang <chunyan.zhang@unisoc.com>
> >
> > It would be better to cleanup the sprd_port for the device before
> > return error.
> >
> > Signed-off-by: Chunyan Zhang <chunyan.zhang@unisoc.com>
> > ---
> >  drivers/tty/serial/sprd_serial.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/tty/serial/sprd_serial.c b/drivers/tty/serial/sprd_serial.c
> > index 9f8c14ff6454..54477de9822f 100644
> > --- a/drivers/tty/serial/sprd_serial.c
> > +++ b/drivers/tty/serial/sprd_serial.c
> > @@ -1204,8 +1204,10 @@ static int sprd_probe(struct platform_device *pdev)
> >         up->has_sysrq = IS_ENABLED(CONFIG_SERIAL_SPRD_CONSOLE);
> >
> >         ret = sprd_clk_init(up);
> > -       if (ret)
> > +       if (ret) {
> > +               sprd_port[index] = NULL;
>
> 如果我们强制使用alias, 则这里应该也无需清除了，因为一进probe就会给它重新赋值。 还是我漏了什么？

是不需要, 所以我comment message里写的是it would be better...

我觉得是下面返回的地方都清理了, 这里也清理掉

要么都去掉?


>
>
> >                 return ret;
> > +       }
> >
> >         res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> >         up->membase = devm_ioremap_resource(&pdev->dev, res);
> > --
> > 2.20.1
> >
>
>
> --
> Baolin Wang

Re: [PATCH v2 2/2] serial: sprd: cleanup the sprd_port for error case

[Baolin Wang]

On Wed, Mar 18, 2020 at 5:48 PM Chunyan Zhang <zhang.lyra@gmail.com> wrote:
>
> On Wed, 18 Mar 2020 at 17:16, Baolin Wang <baolin.wang7@gmail.com> wrote:
> >
> > On Wed, Mar 18, 2020 at 4:31 PM Chunyan Zhang <zhang.lyra@gmail.com> wrote:
> > >
> > > From: Chunyan Zhang <chunyan.zhang@unisoc.com>
> > >
> > > It would be better to cleanup the sprd_port for the device before
> > > return error.
> > >
> > > Signed-off-by: Chunyan Zhang <chunyan.zhang@unisoc.com>
> > > ---
> > >  drivers/tty/serial/sprd_serial.c | 4 +++-
> > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/tty/serial/sprd_serial.c b/drivers/tty/serial/sprd_serial.c
> > > index 9f8c14ff6454..54477de9822f 100644
> > > --- a/drivers/tty/serial/sprd_serial.c
> > > +++ b/drivers/tty/serial/sprd_serial.c
> > > @@ -1204,8 +1204,10 @@ static int sprd_probe(struct platform_device *pdev)
> > >         up->has_sysrq = IS_ENABLED(CONFIG_SERIAL_SPRD_CONSOLE);
> > >
> > >         ret = sprd_clk_init(up);
> > > -       if (ret)
> > > +       if (ret) {
> > > +               sprd_port[index] = NULL;
> >
> > 如果我们强制使用alias, 则这里应该也无需清除了，因为一进probe就会给它重新赋值。 还是我漏了什么？
>
> 是不需要, 所以我comment message里写的是it would be better...
>
> 我觉得是下面返回的地方都清理了, 这里也清理掉
>
> 要么都去掉?

我觉得删掉吧，代码能少一行是一行 ：）


> >
> >
> > >                 return ret;
> > > +       }
> > >
> > >         res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > >         up->membase = devm_ioremap_resource(&pdev->dev, res);
> > > --
> > > 2.20.1
> > >
> >
> >
> > --
> > Baolin Wang



-- 
Baolin Wang

Re: [PATCH v2 2/2] serial: sprd: cleanup the sprd_port for error case

[Baolin Wang]

On Wed, Mar 18, 2020 at 5:16 PM Baolin Wang <baolin.wang7@gmail.com> wrote:
>
> On Wed, Mar 18, 2020 at 4:31 PM Chunyan Zhang <zhang.lyra@gmail.com> wrote:
> >
> > From: Chunyan Zhang <chunyan.zhang@unisoc.com>
> >
> > It would be better to cleanup the sprd_port for the device before
> > return error.
> >
> > Signed-off-by: Chunyan Zhang <chunyan.zhang@unisoc.com>
> > ---
> >  drivers/tty/serial/sprd_serial.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/tty/serial/sprd_serial.c b/drivers/tty/serial/sprd_serial.c
> > index 9f8c14ff6454..54477de9822f 100644
> > --- a/drivers/tty/serial/sprd_serial.c
> > +++ b/drivers/tty/serial/sprd_serial.c
> > @@ -1204,8 +1204,10 @@ static int sprd_probe(struct platform_device *pdev)
> >         up->has_sysrq = IS_ENABLED(CONFIG_SERIAL_SPRD_CONSOLE);
> >
> >         ret = sprd_clk_init(up);
> > -       if (ret)
> > +       if (ret) {
> > +               sprd_port[index] = NULL;
>
> 如果我们强制使用alias, 则这里应该也无需清除了，因为一进probe就会给它重新赋值。 还是我漏了什么？

Sorry, please ignore my previsous comment. I made a stupid mistake
when talking with Chunyan.

So what I mean is we should not add this clean up, cause we will
always get the correct index with aliases, and it will be overlapped
when probing again.

>
>
> >                 return ret;
> > +       }
> >
> >         res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> >         up->membase = devm_ioremap_resource(&pdev->dev, res);
> > --
> > 2.20.1
> >
>
>
> --
> Baolin Wang



--
Baolin Wang

Re: [PATCH v2 2/2] serial: sprd: cleanup the sprd_port for error case

[Greg Kroah-Hartman]

On Wed, Mar 18, 2020 at 06:06:05PM +0800, Baolin Wang wrote:
> On Wed, Mar 18, 2020 at 5:16 PM Baolin Wang <baolin.wang7@gmail.com> wrote:
> >
> > On Wed, Mar 18, 2020 at 4:31 PM Chunyan Zhang <zhang.lyra@gmail.com> wrote:
> > >
> > > From: Chunyan Zhang <chunyan.zhang@unisoc.com>
> > >
> > > It would be better to cleanup the sprd_port for the device before
> > > return error.
> > >
> > > Signed-off-by: Chunyan Zhang <chunyan.zhang@unisoc.com>
> > > ---
> > >  drivers/tty/serial/sprd_serial.c | 4 +++-
> > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/tty/serial/sprd_serial.c b/drivers/tty/serial/sprd_serial.c
> > > index 9f8c14ff6454..54477de9822f 100644
> > > --- a/drivers/tty/serial/sprd_serial.c
> > > +++ b/drivers/tty/serial/sprd_serial.c
> > > @@ -1204,8 +1204,10 @@ static int sprd_probe(struct platform_device *pdev)
> > >         up->has_sysrq = IS_ENABLED(CONFIG_SERIAL_SPRD_CONSOLE);
> > >
> > >         ret = sprd_clk_init(up);
> > > -       if (ret)
> > > +       if (ret) {
> > > +               sprd_port[index] = NULL;
> >
> > 如果我们强制使用alias, 则这里应该也无需清除了，因为一进probe就会给它重新赋值。 还是我漏了什么？
> 
> Sorry, please ignore my previsous comment. I made a stupid mistake
> when talking with Chunyan.
> 
> So what I mean is we should not add this clean up, cause we will
> always get the correct index with aliases, and it will be overlapped
> when probing again.

So ignore this patch and only take patch 1/2?  If so, can I get your
acked-by for it?

thanks,

greg k-h

Re: [PATCH v2 2/2] serial: sprd: cleanup the sprd_port for error case

[Chunyan Zhang]

On Wed, 18 Mar 2020 at 18:13, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Wed, Mar 18, 2020 at 06:06:05PM +0800, Baolin Wang wrote:
> > On Wed, Mar 18, 2020 at 5:16 PM Baolin Wang <baolin.wang7@gmail.com> wrote:
> > >
> > > On Wed, Mar 18, 2020 at 4:31 PM Chunyan Zhang <zhang.lyra@gmail.com> wrote:
> > > >
> > > > From: Chunyan Zhang <chunyan.zhang@unisoc.com>
> > > >
> > > > It would be better to cleanup the sprd_port for the device before
> > > > return error.
> > > >
> > > > Signed-off-by: Chunyan Zhang <chunyan.zhang@unisoc.com>
> > > > ---
> > > >  drivers/tty/serial/sprd_serial.c | 4 +++-
> > > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/tty/serial/sprd_serial.c b/drivers/tty/serial/sprd_serial.c
> > > > index 9f8c14ff6454..54477de9822f 100644
> > > > --- a/drivers/tty/serial/sprd_serial.c
> > > > +++ b/drivers/tty/serial/sprd_serial.c
> > > > @@ -1204,8 +1204,10 @@ static int sprd_probe(struct platform_device *pdev)
> > > >         up->has_sysrq = IS_ENABLED(CONFIG_SERIAL_SPRD_CONSOLE);
> > > >
> > > >         ret = sprd_clk_init(up);
> > > > -       if (ret)
> > > > +       if (ret) {
> > > > +               sprd_port[index] = NULL;
> > >
> > > 如果我们强制使用alias, 则这里应该也无需清除了，因为一进probe就会给它重新赋值。 还是我漏了什么？
> >
> > Sorry, please ignore my previsous comment. I made a stupid mistake
> > when talking with Chunyan.
> >
> > So what I mean is we should not add this clean up, cause we will
> > always get the correct index with aliases, and it will be overlapped
> > when probing again.
>
> So ignore this patch and only take patch 1/2?  If so, can I get your
> acked-by for it?

Hi Greg,

There's something I need to modify on 1/2 as well, I will send the
whole patch-set later, please ignore these two patches.

Sorry for the noise!

Thanks,
Chunyan

>
> thanks,
>
> greg k-h

Re: [PATCH v2 2/2] serial: sprd: cleanup the sprd_port for error case

[Greg Kroah-Hartman]

On Wed, Mar 18, 2020 at 06:16:43PM +0800, Chunyan Zhang wrote:
> On Wed, 18 Mar 2020 at 18:13, Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> >
> > On Wed, Mar 18, 2020 at 06:06:05PM +0800, Baolin Wang wrote:
> > > On Wed, Mar 18, 2020 at 5:16 PM Baolin Wang <baolin.wang7@gmail.com> wrote:
> > > >
> > > > On Wed, Mar 18, 2020 at 4:31 PM Chunyan Zhang <zhang.lyra@gmail.com> wrote:
> > > > >
> > > > > From: Chunyan Zhang <chunyan.zhang@unisoc.com>
> > > > >
> > > > > It would be better to cleanup the sprd_port for the device before
> > > > > return error.
> > > > >
> > > > > Signed-off-by: Chunyan Zhang <chunyan.zhang@unisoc.com>
> > > > > ---
> > > > >  drivers/tty/serial/sprd_serial.c | 4 +++-
> > > > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/drivers/tty/serial/sprd_serial.c b/drivers/tty/serial/sprd_serial.c
> > > > > index 9f8c14ff6454..54477de9822f 100644
> > > > > --- a/drivers/tty/serial/sprd_serial.c
> > > > > +++ b/drivers/tty/serial/sprd_serial.c
> > > > > @@ -1204,8 +1204,10 @@ static int sprd_probe(struct platform_device *pdev)
> > > > >         up->has_sysrq = IS_ENABLED(CONFIG_SERIAL_SPRD_CONSOLE);
> > > > >
> > > > >         ret = sprd_clk_init(up);
> > > > > -       if (ret)
> > > > > +       if (ret) {
> > > > > +               sprd_port[index] = NULL;
> > > >
> > > > 如果我们强制使用alias, 则这里应该也无需清除了，因为一进probe就会给它重新赋值。 还是我漏了什么？
> > >
> > > Sorry, please ignore my previsous comment. I made a stupid mistake
> > > when talking with Chunyan.
> > >
> > > So what I mean is we should not add this clean up, cause we will
> > > always get the correct index with aliases, and it will be overlapped
> > > when probing again.
> >
> > So ignore this patch and only take patch 1/2?  If so, can I get your
> > acked-by for it?
> 
> Hi Greg,
> 
> There's something I need to modify on 1/2 as well, I will send the
> whole patch-set later, please ignore these two patches.
> 
> Sorry for the noise!

Discussion about patches is _never_ noise, that's what this list/group
is for :)

Re: [PATCH v2 2/2] serial: sprd: cleanup the sprd_port for error case

[Chunyan Zhang]

On Wed, Mar 18, 2020 at 6:48 PM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Wed, Mar 18, 2020 at 06:16:43PM +0800, Chunyan Zhang wrote:
> > On Wed, 18 Mar 2020 at 18:13, Greg Kroah-Hartman
> > <gregkh@linuxfoundation.org> wrote:
> > >
> > > On Wed, Mar 18, 2020 at 06:06:05PM +0800, Baolin Wang wrote:
> > > > On Wed, Mar 18, 2020 at 5:16 PM Baolin Wang <baolin.wang7@gmail.com> wrote:
> > > > >
> > > > > On Wed, Mar 18, 2020 at 4:31 PM Chunyan Zhang <zhang.lyra@gmail.com> wrote:
> > > > > >
> > > > > > From: Chunyan Zhang <chunyan.zhang@unisoc.com>
> > > > > >
> > > > > > It would be better to cleanup the sprd_port for the device before
> > > > > > return error.
> > > > > >
> > > > > > Signed-off-by: Chunyan Zhang <chunyan.zhang@unisoc.com>
> > > > > > ---
> > > > > >  drivers/tty/serial/sprd_serial.c | 4 +++-
> > > > > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git a/drivers/tty/serial/sprd_serial.c b/drivers/tty/serial/sprd_serial.c
> > > > > > index 9f8c14ff6454..54477de9822f 100644
> > > > > > --- a/drivers/tty/serial/sprd_serial.c
> > > > > > +++ b/drivers/tty/serial/sprd_serial.c
> > > > > > @@ -1204,8 +1204,10 @@ static int sprd_probe(struct platform_device *pdev)
> > > > > >         up->has_sysrq = IS_ENABLED(CONFIG_SERIAL_SPRD_CONSOLE);
> > > > > >
> > > > > >         ret = sprd_clk_init(up);
> > > > > > -       if (ret)
> > > > > > +       if (ret) {
> > > > > > +               sprd_port[index] = NULL;
> > > > >
> > > > > 如果我们强制使用alias, 则这里应该也无需清除了，因为一进probe就会给它重新赋值。 还是我漏了什么？
> > > >
> > > > Sorry, please ignore my previsous comment. I made a stupid mistake
> > > > when talking with Chunyan.
> > > >
> > > > So what I mean is we should not add this clean up, cause we will
> > > > always get the correct index with aliases, and it will be overlapped
> > > > when probing again.
> > >
> > > So ignore this patch and only take patch 1/2?  If so, can I get your
> > > acked-by for it?
> >
> > Hi Greg,
> >
> > There's something I need to modify on 1/2 as well, I will send the
> > whole patch-set later, please ignore these two patches.
> >
> > Sorry for the noise!
>
> Discussion about patches is _never_ noise, that's what this list/group
> is for :)

Ok, thanks for your nice words, at least brought you some trouble,
that's what I mean :)

I've sent out a new version of patches just now.

Thanks again,
Chunyan