[PATCH] serial: 8250_dw: Emit an error message if getting the baudclk failed

[PATCH] serial: 8250_dw: Emit an error message if getting the baudclk failed

[Uwe Kleine-König]

Instead of silently giving up, at least tell what the problem is.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/tty/serial/8250/8250_dw.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
index 2d1f350a4bea..94aa3dddb71e 100644
--- a/drivers/tty/serial/8250/8250_dw.c
+++ b/drivers/tty/serial/8250/8250_dw.c
@@ -597,7 +597,8 @@ static int dw8250_probe(struct platform_device *pdev)
 	if (data->clk == NULL)
 		data->clk = devm_clk_get_optional_enabled(dev, NULL);
 	if (IS_ERR(data->clk))
-		return PTR_ERR(data->clk);
+		return dev_err_probe(dev, PTR_ERR(data->clk),
+				     "failed to get baudclk\n");
 
 	INIT_WORK(&data->clk_work, dw8250_clk_work_cb);
 	data->clk_notifier.notifier_call = dw8250_clk_notifier_cb;
-- 
2.43.0

Re: [PATCH] serial: 8250_dw: Emit an error message if getting the baudclk failed

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 908 bytes --]

On Thu, 22 Feb 2024, Uwe Kleine-König wrote:

> Instead of silently giving up, at least tell what the problem is.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>  drivers/tty/serial/8250/8250_dw.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
> index 2d1f350a4bea..94aa3dddb71e 100644
> --- a/drivers/tty/serial/8250/8250_dw.c
> +++ b/drivers/tty/serial/8250/8250_dw.c
> @@ -597,7 +597,8 @@ static int dw8250_probe(struct platform_device *pdev)
>  	if (data->clk == NULL)
>  		data->clk = devm_clk_get_optional_enabled(dev, NULL);
>  	if (IS_ERR(data->clk))
> -		return PTR_ERR(data->clk);
> +		return dev_err_probe(dev, PTR_ERR(data->clk),
> +				     "failed to get baudclk\n");

Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

-- 
 i.

Re: [PATCH] serial: 8250_dw: Emit an error message if getting the baudclk failed

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 1292 bytes --]

Hello Ilpo,

On Thu, Feb 22, 2024 at 01:50:40PM +0200, Ilpo Järvinen wrote:
> On Thu, 22 Feb 2024, Uwe Kleine-König wrote:
> 
> > Instead of silently giving up, at least tell what the problem is.
> > 
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > ---
> >  drivers/tty/serial/8250/8250_dw.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
> > index 2d1f350a4bea..94aa3dddb71e 100644
> > --- a/drivers/tty/serial/8250/8250_dw.c
> > +++ b/drivers/tty/serial/8250/8250_dw.c
> > @@ -597,7 +597,8 @@ static int dw8250_probe(struct platform_device *pdev)
> >  	if (data->clk == NULL)
> >  		data->clk = devm_clk_get_optional_enabled(dev, NULL);
> >  	if (IS_ERR(data->clk))
> > -		return PTR_ERR(data->clk);
> > +		return dev_err_probe(dev, PTR_ERR(data->clk),
> > +				     "failed to get baudclk\n");
> 
> Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

Thanks. Is the expectation now with your tag that Greg (added to Cc:)
picks up this patch? 

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] serial: 8250_dw: Emit an error message if getting the baudclk failed

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 1378 bytes --]

On Thu, 22 Feb 2024, Uwe Kleine-König wrote:
> On Thu, Feb 22, 2024 at 01:50:40PM +0200, Ilpo Järvinen wrote:
> > On Thu, 22 Feb 2024, Uwe Kleine-König wrote:
> > 
> > > Instead of silently giving up, at least tell what the problem is.
> > > 
> > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > > ---
> > >  drivers/tty/serial/8250/8250_dw.c | 3 ++-
> > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
> > > index 2d1f350a4bea..94aa3dddb71e 100644
> > > --- a/drivers/tty/serial/8250/8250_dw.c
> > > +++ b/drivers/tty/serial/8250/8250_dw.c
> > > @@ -597,7 +597,8 @@ static int dw8250_probe(struct platform_device *pdev)
> > >  	if (data->clk == NULL)
> > >  		data->clk = devm_clk_get_optional_enabled(dev, NULL);
> > >  	if (IS_ERR(data->clk))
> > > -		return PTR_ERR(data->clk);
> > > +		return dev_err_probe(dev, PTR_ERR(data->clk),
> > > +				     "failed to get baudclk\n");
> > 
> > Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> 
> Thanks. Is the expectation now with your tag that Greg (added to Cc:)
> picks up this patch? 

Yes, Greg handles applying serial patches. There's just too low volume of 
patches for it be useful for me to setup some an intermediate tree for 
8250_dw/lpss only.

-- 
 i.

Re: [PATCH] serial: 8250_dw: Emit an error message if getting the baudclk failed

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 1810 bytes --]

Hello,

On Thu, Feb 22, 2024 at 03:48:48PM +0200, Ilpo Järvinen wrote:
> On Thu, 22 Feb 2024, Uwe Kleine-König wrote:
> > On Thu, Feb 22, 2024 at 01:50:40PM +0200, Ilpo Järvinen wrote:
> > > On Thu, 22 Feb 2024, Uwe Kleine-König wrote:
> > > 
> > > > Instead of silently giving up, at least tell what the problem is.
> > > > 
> > > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > > > ---
> > > >  drivers/tty/serial/8250/8250_dw.c | 3 ++-
> > > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
> > > > index 2d1f350a4bea..94aa3dddb71e 100644
> > > > --- a/drivers/tty/serial/8250/8250_dw.c
> > > > +++ b/drivers/tty/serial/8250/8250_dw.c
> > > > @@ -597,7 +597,8 @@ static int dw8250_probe(struct platform_device *pdev)
> > > >  	if (data->clk == NULL)
> > > >  		data->clk = devm_clk_get_optional_enabled(dev, NULL);
> > > >  	if (IS_ERR(data->clk))
> > > > -		return PTR_ERR(data->clk);
> > > > +		return dev_err_probe(dev, PTR_ERR(data->clk),
> > > > +				     "failed to get baudclk\n");
> > > 
> > > Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> > 
> > Thanks. Is the expectation now with your tag that Greg (added to Cc:)
> > picks up this patch? 
> 
> Yes, Greg handles applying serial patches. There's just too low volume of 
> patches for it be useful for me to setup some an intermediate tree for 
> 8250_dw/lpss only.

Yeah, I thought so, I just wondered if that also works if Greg isn't in
the list of recipents. But that should be fixed now, too.

Thanks
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] serial: 8250_dw: Emit an error message if getting the baudclk failed

[Andy Shevchenko]

On Thu, Feb 22, 2024 at 02:36:22PM +0100, Uwe Kleine-König wrote:
> On Thu, Feb 22, 2024 at 01:50:40PM +0200, Ilpo Järvinen wrote:

...

> Thanks. Is the expectation now with your tag that Greg (added to Cc:)
> picks up this patch? 

Why you haven't Cc'ed him to begin with? We have get_maintainer.pl script for
that. (You can see what script [1] I'm using to get patches published, another
approach can be switching to b4 relay.)

[1]: https://github.com/andy-shev/home-bin-tools/blob/master/ge2maintainer.sh

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] serial: 8250_dw: Emit an error message if getting the baudclk failed

[Andy Shevchenko]

On Thu, Feb 22, 2024 at 12:19:23PM +0100, Uwe Kleine-König wrote:
> Instead of silently giving up, at least tell what the problem is.

...

> -		return PTR_ERR(data->clk);
> +		return dev_err_probe(dev, PTR_ERR(data->clk),
> +				     "failed to get baudclk\n");

Why not on a single line? We are fine with relaxed limits.

...

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] serial: 8250_dw: Emit an error message if getting the baudclk failed

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 802 bytes --]

Hello Andy,

On Thu, Feb 22, 2024 at 05:11:46PM +0200, Andy Shevchenko wrote:
> On Thu, Feb 22, 2024 at 12:19:23PM +0100, Uwe Kleine-König wrote:
> > Instead of silently giving up, at least tell what the problem is.
> 
> ...
> 
> > -		return PTR_ERR(data->clk);
> > +		return dev_err_probe(dev, PTR_ERR(data->clk),
> > +				     "failed to get baudclk\n");
> 
> Why not on a single line? We are fine with relaxed limits.

At least around my change I didn't spot a line that makes use of the
relaxed limits, so I sticked to the overall style.

> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Thanks
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] serial: 8250_dw: Emit an error message if getting the baudclk failed

[VAMSHI GAJJELA]

On Thu, Feb 22, 2024 at 4:50 PM Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
>
> Instead of silently giving up, at least tell what the problem is.
>
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>  drivers/tty/serial/8250/8250_dw.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
> index 2d1f350a4bea..94aa3dddb71e 100644
> --- a/drivers/tty/serial/8250/8250_dw.c
> +++ b/drivers/tty/serial/8250/8250_dw.c
> @@ -597,7 +597,8 @@ static int dw8250_probe(struct platform_device *pdev)
>         if (data->clk == NULL)
>                 data->clk = devm_clk_get_optional_enabled(dev, NULL);
>         if (IS_ERR(data->clk))
> -               return PTR_ERR(data->clk);
> +               return dev_err_probe(dev, PTR_ERR(data->clk),
> +                                    "failed to get baudclk\n");

Not required IMO as the baudclk is optional, otherwise it might ask
for a similar change at apb_pclk.
Could you please provide some insight into the circumstances that lead
to encountering this error case?

Also the check is for IS_ERR(data->clk), data-clk could be NULL aswell.

I see any error should be caught at the following line, provided no
clock-frequency
```
/* If no clock rate is defined, fail. */
if (!p->uartclk)
        return dev_err_probe(dev, -EINVAL, "clock rate not defined\n");
```
>
>         INIT_WORK(&data->clk_work, dw8250_clk_work_cb);
>         data->clk_notifier.notifier_call = dw8250_clk_notifier_cb;
> --
> 2.43.0
>
>

Re: [PATCH] serial: 8250_dw: Emit an error message if getting the baudclk failed

[Andy Shevchenko]

On Mon, Feb 26, 2024 at 09:30:53PM +0530, VAMSHI GAJJELA wrote:
> On Thu, Feb 22, 2024 at 4:50 PM Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:

...

> >         if (data->clk == NULL)
> >                 data->clk = devm_clk_get_optional_enabled(dev, NULL);
> >         if (IS_ERR(data->clk))
> > -               return PTR_ERR(data->clk);
> > +               return dev_err_probe(dev, PTR_ERR(data->clk),
> > +                                    "failed to get baudclk\n");
> 
> Not required IMO as the baudclk is optional,

It adds verbosity to the cases when it's defined, but for some reason
can't be retrieved.

> otherwise it might ask
> for a similar change at apb_pclk.

If you need so, yes. Send a patch.

> Could you please provide some insight into the circumstances that lead
> to encountering this error case?
> 
> Also the check is for IS_ERR(data->clk), data-clk could be NULL aswell.

Yes, and that's the case of everything is fine as the clock is optional.
Do you see any problem with that check?

> I see any error should be caught at the following line, provided no
> clock-frequency
> ```
> /* If no clock rate is defined, fail. */
> if (!p->uartclk)
>         return dev_err_probe(dev, -EINVAL, "clock rate not defined\n");
> ```

Not sure what you meant by this. Even if we ask for an optional clock,
the error condition still might happen with many other reasons besides
the absence of the clock.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] serial: 8250_dw: Emit an error message if getting the baudclk failed

[VAMSHI GAJJELA]

On Mon, Feb 26, 2024 at 10:07 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Mon, Feb 26, 2024 at 09:30:53PM +0530, VAMSHI GAJJELA wrote:
> > On Thu, Feb 22, 2024 at 4:50 PM Uwe Kleine-König
> > <u.kleine-koenig@pengutronix.de> wrote:
>
> ...
>
> > >         if (data->clk == NULL)
> > >                 data->clk = devm_clk_get_optional_enabled(dev, NULL);
> > >         if (IS_ERR(data->clk))
> > > -               return PTR_ERR(data->clk);
> > > +               return dev_err_probe(dev, PTR_ERR(data->clk),
> > > +                                    "failed to get baudclk\n");
> >
> > Not required IMO as the baudclk is optional,
>
> It adds verbosity to the cases when it's defined, but for some reason
> can't be retrieved.
ack
>
> > otherwise it might ask
> > for a similar change at apb_pclk.
>
> If you need so, yes. Send a patch.
Noted
>
> > Could you please provide some insight into the circumstances that lead
> > to encountering this error case?
> >
> > Also the check is for IS_ERR(data->clk), data-clk could be NULL aswell.
>
> Yes, and that's the case of everything is fine as the clock is optional.
> Do you see any problem with that check?
No problem here
>
> > I see any error should be caught at the following line, provided no
> > clock-frequency
> > ```
> > /* If no clock rate is defined, fail. */
> > if (!p->uartclk)
> >         return dev_err_probe(dev, -EINVAL, "clock rate not defined\n");
> > ```
>
> Not sure what you meant by this. Even if we ask for an optional clock,
> the error condition still might happen with many other reasons besides
> the absence of the clock.
I was looking for a reason that the author has encountered. Error condition
on an optional clock might also fail `p->uartclk = clk_get_rate` and result
 "clock rate not defined\n" error.

>
> --
> With Best Regards,
> Andy Shevchenko
>
>

Re: [PATCH] serial: 8250_dw: Emit an error message if getting the baudclk failed

[Andy Shevchenko]

On Wed, Feb 28, 2024 at 11:15:23PM +0530, VAMSHI GAJJELA wrote:
> On Mon, Feb 26, 2024 at 10:07 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Mon, Feb 26, 2024 at 09:30:53PM +0530, VAMSHI GAJJELA wrote:
> > > On Thu, Feb 22, 2024 at 4:50 PM Uwe Kleine-König
> > > <u.kleine-koenig@pengutronix.de> wrote:

...

> > > I see any error should be caught at the following line, provided no
> > > clock-frequency
> > > ```
> > > /* If no clock rate is defined, fail. */
> > > if (!p->uartclk)
> > >         return dev_err_probe(dev, -EINVAL, "clock rate not defined\n");
> > > ```
> >
> > Not sure what you meant by this. Even if we ask for an optional clock,
> > the error condition still might happen with many other reasons besides
> > the absence of the clock.
> I was looking for a reason that the author has encountered. Error condition
> on an optional clock might also fail `p->uartclk = clk_get_rate` and result
>  "clock rate not defined\n" error.

Are you sure that CCF allows this to happen on real clocks?
I mean, the rate of (existing) clock can be 0 iff the clock provider is
powered off or unclocked itself (by parent clock).
But I'm not sure this is the case CCF handles.

-- 
With Best Regards,
Andy Shevchenko