[PATCH] i2c: designware: Suppress error message if platform_get_irq() returns -EPROBE_DEFER

[PATCH] i2c: designware: Suppress error message if platform_get_irq() returns -EPROBE_DEFER

[Alexey Brodkin]

There's no point in printing error message if platform_get_irq()
returns -EPROBE_DEFER because probe deferring subsystem already outputs
message in bootlog like this:
 --->8---
 platform e001d000.i2c: Driver i2c_designware requests probe deferral
 --->8---

Moreover in case of probe deferral following message may mislead user:
 --->8---
 i2c_designware e001d000.i2c: no irq resource?
 --->8---
even though it's expected that platform_get_irq() may return
-EPROBE_DEFER.

Signed-off-by: Alexey Brodkin <abrodkin@synopsys.com>
Cc: Vineet Gupta <vgupta@synopsys.com>
Cc: Christian Ruppert <christian.ruppert@abilis.com>
Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
Cc: Wolfram Sang <wsa@the-dreams.de>
---
 drivers/i2c/busses/i2c-designware-platdrv.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
index c270f5f..01c1b17 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -144,7 +144,8 @@ static int dw_i2c_probe(struct platform_device *pdev)
 
 	irq = platform_get_irq(pdev, 0);
 	if (irq < 0) {
-		dev_err(&pdev->dev, "no irq resource?\n");
+		if (irq != -EPROBE_DEFER)
+			dev_err(&pdev->dev, "no irq resource?\n");
 		return irq; /* -ENXIO */
 	}
 
-- 
2.1.0

Re: [PATCH] i2c: designware: Suppress error message if platform_get_irq() returns -EPROBE_DEFER

[Christian Ruppert]

On 2015-03-03 16:27, Alexey Brodkin wrote:
> There's no point in printing error message if platform_get_irq()
> returns -EPROBE_DEFER because probe deferring subsystem already outputs
> message in bootlog like this:
>  --->8---
>  platform e001d000.i2c: Driver i2c_designware requests probe deferral
>  --->8---
> 
> Moreover in case of probe deferral following message may mislead user:
>  --->8---
>  i2c_designware e001d000.i2c: no irq resource?
>  --->8---
> even though it's expected that platform_get_irq() may return
> -EPROBE_DEFER.
> 
> Signed-off-by: Alexey Brodkin <abrodkin@synopsys.com>
> Cc: Vineet Gupta <vgupta@synopsys.com>
> Cc: Christian Ruppert <christian.ruppert@abilis.com>
> Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
> Cc: Wolfram Sang <wsa@the-dreams.de>
> ---
>  drivers/i2c/busses/i2c-designware-platdrv.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
> index c270f5f..01c1b17 100644
> --- a/drivers/i2c/busses/i2c-designware-platdrv.c
> +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
> @@ -144,7 +144,8 @@ static int dw_i2c_probe(struct platform_device *pdev)
>  
>  	irq = platform_get_irq(pdev, 0);
>  	if (irq < 0) {
> -		dev_err(&pdev->dev, "no irq resource?\n");
> +		if (irq != -EPROBE_DEFER)
> +			dev_err(&pdev->dev, "no irq resource?\n");

Presented like this I wonder if this merits being a dev_err at all.
Wouldn't dev_dbg be more adequate? This might remove the need for the
condition and also avoid bothering everyone if something in the platform
device structures or device tree is not right.

>  		return irq; /* -ENXIO */
>  	}
>  
> 

Re: [PATCH] i2c: designware: Suppress error message if platform_get_irq() returns -EPROBE_DEFER

[Alexey Brodkin]

Hi Christian,

On Tue, 2015-03-03 at 17:28 +0100, Christian Ruppert wrote:
> On 2015-03-03 16:27, Alexey Brodkin wrote:
> > There's no point in printing error message if platform_get_irq()
> > returns -EPROBE_DEFER because probe deferring subsystem already outputs
> > message in bootlog like this:
> >  --->8---
> >  platform e001d000.i2c: Driver i2c_designware requests probe deferral
> >  --->8---
> > 
> > Moreover in case of probe deferral following message may mislead user:
> >  --->8---
> >  i2c_designware e001d000.i2c: no irq resource?
> >  --->8---
> > even though it's expected that platform_get_irq() may return
> > -EPROBE_DEFER.
> >  
> >  	irq = platform_get_irq(pdev, 0);
> >  	if (irq < 0) {
> > -		dev_err(&pdev->dev, "no irq resource?\n");
> > +		if (irq != -EPROBE_DEFER)
> > +			dev_err(&pdev->dev, "no irq resource?\n");
> 
> Presented like this I wonder if this merits being a dev_err at all.
> Wouldn't dev_dbg be more adequate? This might remove the need for the
> condition and also avoid bothering everyone if something in the platform
> device structures or device tree is not right.
> 
> >  		return irq; /* -ENXIO */
> >  	}

We've just had similar discussion related to DW APB UART with Andy here
https://lkml.org/lkml/2015/3/3/412

So yes probably we may safely remove error message from here completely.

-Alexey

Re: [PATCH] i2c: designware: Suppress error message if platform_get_irq() returns -EPROBE_DEFER

[christian.ruppert]

Hello Alexey,

Alexey Brodkin <Alexey.Brodkin@synopsys.com> wrote on 03/03/2015 05:37:31 
PM:

> From:
> 
> Alexey Brodkin <Alexey.Brodkin@synopsys.com>
> 
> To:
> 
> "christian.ruppert@alitech.com" <christian.ruppert@alitech.com>, 
> 
> Cc:
> 
> "mika.westerberg@linux.intel.com" <mika.westerberg@linux.intel.com>,
> "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>, 
> "Vineet.Gupta1@synopsys.com" <Vineet.Gupta1@synopsys.com>, "wsa@the-
> dreams.de" <wsa@the-dreams.de>, "andriy.shevchenko@linux.intel.com" 
> <andriy.shevchenko@linux.intel.com>, "linux-i2c@vger.kernel.org" 
> <linux-i2c@vger.kernel.org>, "christian.ruppert@abilis.com" 
> <christian.ruppert@abilis.com>
> 
> Date:
> 
> 03/03/2015 05:38 PM
> 
> Subject:
> 
> Re: [PATCH] i2c: designware: Suppress error message if 
> platform_get_irq() returns -EPROBE_DEFER
> 
> Hi Christian,
> [...]
> > > 
> > >     irq = platform_get_irq(pdev, 0);
> > >     if (irq < 0) {
> > > -      dev_err(&pdev->dev, "no irq resource?\n");
> > > +      if (irq != -EPROBE_DEFER)
> > > +         dev_err(&pdev->dev, "no irq resource?\n");
> > 
> > Presented like this I wonder if this merits being a dev_err at all.
> > Wouldn't dev_dbg be more adequate? This might remove the need for the
> > condition and also avoid bothering everyone if something in the 
platform
> > device structures or device tree is not right.
> > 
> > >        return irq; /* -ENXIO */
> > >     }
> 
> We've just had similar discussion related to DW APB UART with Andy here
> https://lkml.org/lkml/2015/3/3/412
> 
> So yes probably we may safely remove error message from here completely.

I agree. Although you do have a point (in the other thread) when saying 
this
kind of messages can be useful in some situations. The process of 
debugging
device tree and platform device setup is definitely more painful for 
drivers
which omit this type of messages completely. Andy's proposal of 
centralising
this looks like a very good solution here (and on top of that removes many
useless strings from the kernel binary).

Greetings,
  Christian

Re: [PATCH] i2c: designware: Suppress error message if platform_get_irq() returns -EPROBE_DEFER

[Wolfram Sang]

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


> which omit this type of messages completely. Andy's proposal of
> centralising this looks like a very good solution here (and on top of
> that removes many useless strings from the kernel binary).

I am all for centralizing printouts. I recommended this at my ELCE talk
last year, too. However, you need to keep in mind that irqs are
sometimes optional and you don't want error messages for those irqs.
IMO worthwhile, but not a low hanging fruit...


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] i2c: designware: Suppress error message if platform_get_irq() returns -EPROBE_DEFER

[Christian Ruppert]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 2015-03-03 18:21, Wolfram Sang wrote:
> 
>> which omit this type of messages completely. Andy's proposal of 
>> centralising this looks like a very good solution here (and on
>> top of that removes many useless strings from the kernel
>> binary).
> 
> I am all for centralizing printouts. I recommended this at my ELCE
> talk last year, too. However, you need to keep in mind that irqs
> are sometimes optional and you don't want error messages for those
> irqs. IMO worthwhile, but not a low hanging fruit...

There is a lot of truth in that. Thus the initial dev_dbg() suggestion
to go half way. I still think that Andy's proposal (or a variation
thereof to catch the optional irqs case) should be the ultimate goal
but I agree that this is more than a quick patch and that it's
probably way out of scope here.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.14 (GNU/Linux)

iQIcBAEBAgAGBQJU9fONAAoJENOBSR1q+OdQLh0P/3f9NIBGyvpr3ao+UVhiW26f
8ikcd1lHezjGZuILPgsobb4bpePwyV0IYLoqediGtwOSlbe6IlxC5DTzwlcJhUe5
BDtker32yPTohA8uc27PFlnZ3secJ3xngmF43QwsP4waQxdLsufQ85YQ1/P4SXhQ
gkxZpE1J1T4jVshtrf4lWlMJ3okdN2n/aFILRkNPCGc5QV9FjAMDwzYJLhBJw8wj
0Xe0X6zflgLlbUGfIeUIPoS7Stn9JtLZt/ltRAPBfFdKwBLUWOvmqGWl5XBcgzRi
UCEEefQl7T/H5HXOEdUyLuCFNsa7rMLEGX8HVSpqb1516dMM4L4vQomISKQyW5ED
SHLaCwzIsQTLdaY4gfNdKjJQMkuZSktz5zG8k3SRAR1PSqzNn7isUcc12HmoyRUd
2lONuN79r3z2iM591FofG69uC64RAJc5DHBj9gHbyvPwp48SMOQThO2vUunZvDoZ
E9rfux3Dj0txpxeh4GqGUjtDXmIhcLm9uXMYQ2UBMH/vaz3adWcX3rH76MLTIZCS
a0ilv+7Jr8VDgJcik2hiFIsSURSZqKSub6bXefvUYc8jOscREa0SnBQvGT3xkl+p
n57+5d+lWpkT4a5LSVRqkhhAtG+g/Ti09YOfhR2Z3C2xvWxM7VyVbxY69UDPXW5Z
sBDHtNkqCZUcx5wdkPec
=cvA7
-----END PGP SIGNATURE-----

Re: [PATCH] i2c: designware: Suppress error message if platform_get_irq() returns -EPROBE_DEFER

[Andy Shevchenko]

On Tue, 2015-03-03 at 18:46 +0100, Christian Ruppert wrote:
> On 2015-03-03 18:21, Wolfram Sang wrote:
> > 
> >> which omit this type of messages completely. Andy's proposal of 
> >> centralising this looks like a very good solution here (and on
> >> top of that removes many useless strings from the kernel
> >> binary).
> > 
> > I am all for centralizing printouts. I recommended this at my ELCE
> > talk last year, too. However, you need to keep in mind that irqs
> > are sometimes optional and you don't want error messages for those
> > irqs. IMO worthwhile, but not a low hanging fruit...
> 
> There is a lot of truth in that. Thus the initial dev_dbg() suggestion
> to go half way. I still think that Andy's proposal (or a variation
> thereof to catch the optional irqs case) should be the ultimate goal
> but I agree that this is more than a quick patch and that it's
> probably way out of scope here.

Yes, I was thinking even about some wrapper on top of platform_get_irq()
since it seems there are no messaging done inside platform.c, though
devm_* functions usually have it.


-- 
Andy Shevchenko <andriy.shevchenko@intel.com>
Intel Finland Oy

Re: [PATCH] i2c: designware: Suppress error message if platform_get_irq() returns -EPROBE_DEFER

[Wolfram Sang]

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


> > > I am all for centralizing printouts. I recommended this at my ELCE
> > > talk last year, too. However, you need to keep in mind that irqs
> > > are sometimes optional and you don't want error messages for those
> > > irqs. IMO worthwhile, but not a low hanging fruit...
> > 
> > There is a lot of truth in that. Thus the initial dev_dbg() suggestion
> > to go half way. I still think that Andy's proposal (or a variation
> > thereof to catch the optional irqs case) should be the ultimate goal
> > but I agree that this is more than a quick patch and that it's
> > probably way out of scope here.
> 
> Yes, I was thinking even about some wrapper on top of platform_get_irq()
> since it seems there are no messaging done inside platform.c, though
> devm_* functions usually have it.

When I had a look a few months ago, the situation with devm_* was messy.
Some rightfully printed errors, some rightfully didn't, some vice versa,
some the other way around, and some did something else...

For driver authors, it is hard to see/remember which devm function does
it and which doesn't. IMO a good cleanup will get rid of this mess. I
started sketching something but especially clks and irqs are basically
everywhere and so it easily grew out of the fun-time project scale,
sadly.


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] i2c: designware: Suppress error message if platform_get_irq() returns -EPROBE_DEFER

[Andy Shevchenko]

On Tue, 2015-03-03 at 20:11 +0100, Wolfram Sang wrote:
> > Yes, I was thinking even about some wrapper on top of platform_get_irq()
> > since it seems there are no messaging done inside platform.c, though
> > devm_* functions usually have it.
> 
> When I had a look a few months ago, the situation with devm_* was messy.
> Some rightfully printed errors, some rightfully didn't, some vice versa,
> some the other way around, and some did something else...
> 
> For driver authors, it is hard to see/remember which devm function does
> it and which doesn't. IMO a good cleanup will get rid of this mess. I
> started sketching something but especially clks and irqs are basically
> everywhere and so it easily grew out of the fun-time project scale,
> sadly.

Yeah, same for me. I've checked the situation with platform_get_irq()
and estimate the amount of drivers about 300.

That's why I discourage to create another one that needs to be fixed in
the future.

-- 
Andy Shevchenko <andriy.shevchenko@intel.com>
Intel Finland Oy

Re: [PATCH] i2c: designware: Suppress error message if platform_get_irq() returns -EPROBE_DEFER

[Wolfram Sang]

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


> > Presented like this I wonder if this merits being a dev_err at all.
> > Wouldn't dev_dbg be more adequate? This might remove the need for the
> > condition and also avoid bothering everyone if something in the platform
> > device structures or device tree is not right.
> > 
> > >  		return irq; /* -ENXIO */
> > >  	}
> 
> We've just had similar discussion related to DW APB UART with Andy here
> https://lkml.org/lkml/2015/3/3/412
> 
> So yes probably we may safely remove error message from here completely.

So, do you want to send a V2?

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] i2c: designware: Suppress error message if platform_get_irq() returns -EPROBE_DEFER

[Alexey Brodkin]

Hi Wolfram,

On Sat, 2015-03-07 at 01:24 +0100, Wolfram Sang wrote:
> > > Presented like this I wonder if this merits being a dev_err at all.
> > > Wouldn't dev_dbg be more adequate? This might remove the need for the
> > > condition and also avoid bothering everyone if something in the platform
> > > device structures or device tree is not right.
> > > 
> > > >  		return irq; /* -ENXIO */
> > > >  	}
> > 
> > We've just had similar discussion related to DW APB UART with Andy here
> > https://lkml.org/lkml/2015/3/3/412
> > 
> > So yes probably we may safely remove error message from here completely.
> 
> So, do you want to send a V2?

Sorry for this delay - I was away for last few days.
I'm about to send v2 with removal of error message completely.

-Alexey