Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1325984 > unrolled thread

[PATCH] net: ethernet: davicom: fix devicetree irq resource

Back to article view | Back to linux.kernel

Contents

  [PATCH] net: ethernet: davicom: fix devicetree irq resource Robert Jarzmik <robert.jarzmik@free.fr> - 2016-02-03 22:50 +0100
    Re: [PATCH] net: ethernet: davicom: fix devicetree irq resource Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> - 2016-02-04 18:20 +0100
      Re: [PATCH] net: ethernet: davicom: fix devicetree irq resource Robert Jarzmik <robert.jarzmik@free.fr> - 2016-02-04 21:50 +0100
        Re: [PATCH] net: ethernet: davicom: fix devicetree irq resource Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> - 2016-02-04 22:00 +0100

#1325984 — [PATCH] net: ethernet: davicom: fix devicetree irq resource

The dm9000 driver doesn't work in at least one device-tree
configuration, spitting an error message on irq resource :
[    1.062495] dm9000 8000000.ethernet: insufficient resources
[    1.068439] dm9000 8000000.ethernet: not found (-2).
[    1.073451] dm9000: probe of 8000000.ethernet failed with error -2

The reason behind is that the interrupt might be provided by a gpio
controller, not probed when dm9000 is probed, and needing the probe
deferral mechanism to apply.

Currently, the interrupt is directly taken from resources. This patch
changes this to use the more generic platform_get_irq(), which handles
the deferral.

Moreover, since commit Fixes: 7085a7401ba5 ("drivers: platform: parse
IRQ flags from resources"), the interrupt trigger flags are honored in
platform_get_irq(), so remove the needless code in dm9000.

Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr>
Acked-by: Marcel Ziswiler <marcel@ziswiler.com>
---
 drivers/net/ethernet/davicom/dm9000.c | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/drivers/net/ethernet/davicom/dm9000.c b/drivers/net/ethernet/davicom/dm9000.c
index cf94b72dbacd..6c527bde9edb 100644
--- a/drivers/net/ethernet/davicom/dm9000.c
+++ b/drivers/net/ethernet/davicom/dm9000.c
@@ -128,7 +128,6 @@ struct board_info {
 	struct resource *data_res;
 	struct resource	*addr_req;   /* resources requested */
 	struct resource *data_req;
-	struct resource *irq_res;
 
 	int		 irq_wake;
 
@@ -1300,18 +1299,14 @@ static int
 dm9000_open(struct net_device *dev)
 {
 	struct board_info *db = netdev_priv(dev);
-	unsigned long irqflags = db->irq_res->flags & IRQF_TRIGGER_MASK;
+	unsigned long irqflags = 0;
 
 	if (netif_msg_ifup(db))
 		dev_dbg(db->dev, "enabling %s\n", dev->name);
 
-	/* If there is no IRQ type specified, default to something that
-	 * may work, and tell the user that this is a problem */
-
-	if (irqflags == IRQF_TRIGGER_NONE)
-		irqflags = irq_get_trigger_type(dev->irq);
-
-	if (irqflags == IRQF_TRIGGER_NONE)
+	/* If there is no IRQ type specified, tell the user that this is a
+	 * problem */
+	if (irq_get_trigger_type(dev->irq) == IRQF_TRIGGER_NONE)
 		dev_warn(db->dev, "WARNING: no IRQ resource flags set.\n");
 
 	irqflags |= IRQF_SHARED;
@@ -1500,15 +1495,21 @@ dm9000_probe(struct platform_device *pdev)
 
 	db->addr_res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	db->data_res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
-	db->irq_res  = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
 
-	if (db->addr_res == NULL || db->data_res == NULL ||
-	    db->irq_res == NULL) {
-		dev_err(db->dev, "insufficient resources\n");
+	if (db->addr_res == NULL || db->data_res == NULL) {
+		dev_err(db->dev, "insufficient resources addr=%p data=%p\n",
+			db->addr_res, db->data_res);
 		ret = -ENOENT;
 		goto out;
 	}
 
+	ndev->irq = platform_get_irq(pdev, 0);
+	if (ndev->irq <= 0) {
+		dev_err(db->dev, "interrupt resource unavailable: %d\n",
+			ndev->irq);
+		return ndev->irq;
+	}
+
 	db->irq_wake = platform_get_irq(pdev, 1);
 	if (db->irq_wake >= 0) {
 		dev_dbg(db->dev, "wakeup irq %d\n", db->irq_wake);
@@ -1570,7 +1571,6 @@ dm9000_probe(struct platform_device *pdev)
 
 	/* fill in parameters for net-dev structure */
 	ndev->base_addr = (unsigned long)db->io_addr;
-	ndev->irq	= db->irq_res->start;
 
 	/* ensure at least we have a default set of IO routines */
 	dm9000_set_io(db, iosize);
-- 
2.1.4

[toc] | [next] | [standalone]

#1327033

Hello.

    Your patch summary prefixes are too verbose, it was enough to say only 
"dm9000: ".

On 02/04/2016 12:40 AM, Robert Jarzmik wrote:

> The dm9000 driver doesn't work in at least one device-tree
> configuration, spitting an error message on irq resource :
> [    1.062495] dm9000 8000000.ethernet: insufficient resources
> [    1.068439] dm9000 8000000.ethernet: not found (-2).
> [    1.073451] dm9000: probe of 8000000.ethernet failed with error -2
>
> The reason behind is that the interrupt might be provided by a gpio
> controller, not probed when dm9000 is probed, and needing the probe
> deferral mechanism to apply.
>
> Currently, the interrupt is directly taken from resources. This patch
> changes this to use the more generic platform_get_irq(), which handles
> the deferral.
>
> Moreover, since commit Fixes: 7085a7401ba5 ("drivers: platform: parse
> IRQ flags from resources"), the interrupt trigger flags are honored in
> platform_get_irq(), so remove the needless code in dm9000.
>
> Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr>
> Acked-by: Marcel Ziswiler <marcel@ziswiler.com>
> ---
>   drivers/net/ethernet/davicom/dm9000.c | 28 ++++++++++++++--------------
>   1 file changed, 14 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/net/ethernet/davicom/dm9000.c b/drivers/net/ethernet/davicom/dm9000.c
> index cf94b72dbacd..6c527bde9edb 100644
> --- a/drivers/net/ethernet/davicom/dm9000.c
> +++ b/drivers/net/ethernet/davicom/dm9000.c
[...]
> @@ -1300,18 +1299,14 @@ static int
>   dm9000_open(struct net_device *dev)
>   {
>   	struct board_info *db = netdev_priv(dev);
> -	unsigned long irqflags = db->irq_res->flags & IRQF_TRIGGER_MASK;
> +	unsigned long irqflags = 0;
>
>   	if (netif_msg_ifup(db))
>   		dev_dbg(db->dev, "enabling %s\n", dev->name);
>
> -	/* If there is no IRQ type specified, default to something that
> -	 * may work, and tell the user that this is a problem */
> -
> -	if (irqflags == IRQF_TRIGGER_NONE)
> -		irqflags = irq_get_trigger_type(dev->irq);
> -
> -	if (irqflags == IRQF_TRIGGER_NONE)
> +	/* If there is no IRQ type specified, tell the user that this is a
> +	 * problem */

    The networking code formats comments this way:

/* foo
  * bar
  */

> +	if (irq_get_trigger_type(dev->irq) == IRQF_TRIGGER_NONE)
>   		dev_warn(db->dev, "WARNING: no IRQ resource flags set.\n");
>
>   	irqflags |= IRQF_SHARED;
> @@ -1500,15 +1495,21 @@ dm9000_probe(struct platform_device *pdev)
>
>   	db->addr_res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>   	db->data_res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> -	db->irq_res  = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
>
> -	if (db->addr_res == NULL || db->data_res == NULL ||
> -	    db->irq_res == NULL) {
> -		dev_err(db->dev, "insufficient resources\n");
> +	if (db->addr_res == NULL || db->data_res == NULL) {
> +		dev_err(db->dev, "insufficient resources addr=%p data=%p\n",
> +			db->addr_res, db->data_res);
>   		ret = -ENOENT;
>   		goto out;
>   	}
>
> +	ndev->irq = platform_get_irq(pdev, 0);
> +	if (ndev->irq <= 0) {

    I don't recommend checking for 0 and returning early in this case -- 
you'll signal a probe success this way. Either ignore 0 or return -E<smth>
in this case. Unfortunately, platform_get_irq() is so sloppily coded now that 
it *can* return 0 on error. :-(

> +		dev_err(db->dev, "interrupt resource unavailable: %d\n",
> +			ndev->irq);
> +		return ndev->irq;
> +	}
> +
>   	db->irq_wake = platform_get_irq(pdev, 1);
>   	if (db->irq_wake >= 0) {
>   		dev_dbg(db->dev, "wakeup irq %d\n", db->irq_wake);
[...]

MBR, Sergei

[toc] | [prev] | [next] | [standalone]

#1327174

Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> writes:

> Hello.
>
>    Your patch summary prefixes are too verbose, it was enough to say only
> "dm9000: ".
Well, I don't agree here. The subsystem should be fully specified, at least this
is something I require in pxa, something that is also required in sound/*, etc
... If David doesn't object, I'll keep it that way. As it's his tree, his
decision in the end, so let's have him decide.

>> -	/* If there is no IRQ type specified, default to something that
>> -	 * may work, and tell the user that this is a problem */
>> -
>> -	if (irqflags == IRQF_TRIGGER_NONE)
>> -		irqflags = irq_get_trigger_type(dev->irq);
>> -
>> -	if (irqflags == IRQF_TRIGGER_NONE)
>> +	/* If there is no IRQ type specified, tell the user that this is a
>> +	 * problem */
>
>    The networking code formats comments this way:
>
> /* foo
>  * bar
>  */
May I know where this is documented ?
I'm asking because I didn't find it, because I parsed drivers/net/*.c files, and
the standard kernel comment style was there, ie:
/*
 * foo
 * bar
 */

I was reusing the previous comment style, but I will change it for the standard
kernel style if you wish.

>>
>> +	ndev->irq = platform_get_irq(pdev, 0);
>> +	if (ndev->irq <= 0) {
>
>    I don't recommend checking for 0 and returning early in this case -- 
> you'll signal a probe success this way. Either ignore 0 or return -E<smth>
> in this case. Unfortunately, platform_get_irq() is so sloppily coded now that it
> *can* return 0 on error. :-(
Ah we had that discussion not very long ago, didn't we ? :)
And I think I'll reuse the "if (ndev->irq < 0) {" solution to be consistent with
myself.

Thanks for the review.

-- 
Robert

[toc] | [prev] | [next] | [standalone]

#1327183

On 02/04/2016 11:42 PM, Robert Jarzmik wrote:

>>     Your patch summary prefixes are too verbose, it was enough to say only
>> "dm9000: ".

    Or "davicom: dm9000: ". Missing the driver name itself doesn't look very 
consistent. :-)

> Well, I don't agree here. The subsystem should be fully specified, at least this
> is something I require in pxa, something that is also required in sound/*, etc
> ... If David doesn't object, I'll keep it that way. As it's his tree, his
> decision in the end, so let's have him decide.

    I expect that he disagrees with you. Let's wait...

>>> -	/* If there is no IRQ type specified, default to something that
>>> -	 * may work, and tell the user that this is a problem */
>>> -
>>> -	if (irqflags == IRQF_TRIGGER_NONE)
>>> -		irqflags = irq_get_trigger_type(dev->irq);
>>> -
>>> -	if (irqflags == IRQF_TRIGGER_NONE)
>>> +	/* If there is no IRQ type specified, tell the user that this is a
>>> +	 * problem */
>>
>>     The networking code formats comments this way:
>>
>> /* foo
>>   * bar
>>   */
> May I know where this is documented ?

    Documentation/CodingStyle, chapter 8. Have you run your patch thru 
scripts/checkpatch.pl?

> I'm asking because I didn't find it, because I parsed drivers/net/*.c files, and
> the standard kernel comment style was there, ie:
> /*
>   * foo
>   * bar
>   */

    But you didn't follow it as well?

> I was reusing the previous comment style,

    Ah...

> but I will change it for the standard
> kernel style if you wish.

    Yes, I think checkpatch.pl checks for that, --strict is forced for the 
networking code.


>>> +	ndev->irq = platform_get_irq(pdev, 0);
>>> +	if (ndev->irq <= 0) {
>>
>>     I don't recommend checking for 0 and returning early in this case --
>> you'll signal a probe success this way. Either ignore 0 or return -E<smth>
>> in this case. Unfortunately, platform_get_irq() is so sloppily coded now that it
>> *can* return 0 on error. :-(

    I'll try looking into this issue once I get more free time...

> Ah we had that discussion not very long ago, didn't we ? :)

    Yeah, I remembered that just after hitting <Send>. :-)

> And I think I'll reuse the "if (ndev->irq < 0) {" solution to be consistent with
> myself.

> Thanks for the review.

    My pleasure. :-)

MBR, Sergei

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web