| Message ID | 1440667450-3513-5-git-send-email-horms+renesas@verge.net.au (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Simon Horman |
| Headers | show |
On Thu, Aug 27, 2015 at 11:24 AM, Simon Horman <horms+renesas@verge.net.au> wrote: > --- a/Documentation/devicetree/bindings/net/renesas,ravb.txt > +++ b/Documentation/devicetree/bindings/net/renesas,ravb.txt > @@ -6,8 +6,12 @@ interface contains. > Required properties: > - compatible: "renesas,etheravb-r8a7790" if the device is a part of R8A7790 SoC. > "renesas,etheravb-r8a7794" if the device is a part of R8A7794 SoC. > + "renesas,etheravb-r8a7795" if the device is a part of R8A7795 SoC. > - reg: offset and length of (1) the register block and (2) the stream buffer. > -- interrupts: interrupt specifier for the sole interrupt. > +- interrupts: if the device is a part of R8A7790/R8A7794 SoC > + interrupt specifier for the sole interrupt. > + if the device is a part of R8A7795 SoC > + interrupt specifier for the two interrupts. If there are multiple interrupts, you best make them named interrupts, i.e. requiring "interrupt-names", too. Why are there only 2 interrupts? The datasheet mentions 25, for ch0-ch24. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 8/27/2015 12:24 PM, Simon Horman wrote: > From: Kazuya Mizuguchi <kazuya.mizuguchi.ks@renesas.com> > > Signed-off-by: Kazuya Mizuguchi <kazuya.mizuguchi.ks@renesas.com> > [horms: updated changelog] I don't see any. ;-) > Signed-off-by: Simon Horman <horms+renesas@verge.net.au> > --- > .../devicetree/bindings/net/renesas,ravb.txt | 6 ++- > drivers/net/ethernet/renesas/ravb.h | 1 + > drivers/net/ethernet/renesas/ravb_main.c | 47 +++++++++++++++++++--- > 3 files changed, 47 insertions(+), 7 deletions(-) > > diff --git a/Documentation/devicetree/bindings/net/renesas,ravb.txt b/Documentation/devicetree/bindings/net/renesas,ravb.txt > index 1fd8831437bf..0b4ec02c35a4 100644 > --- a/Documentation/devicetree/bindings/net/renesas,ravb.txt > +++ b/Documentation/devicetree/bindings/net/renesas,ravb.txt > @@ -6,8 +6,12 @@ interface contains. > Required properties: > - compatible: "renesas,etheravb-r8a7790" if the device is a part of R8A7790 SoC. > "renesas,etheravb-r8a7794" if the device is a part of R8A7794 SoC. > + "renesas,etheravb-r8a7795" if the device is a part of R8A7795 SoC. > - reg: offset and length of (1) the register block and (2) the stream buffer. > -- interrupts: interrupt specifier for the sole interrupt. > +- interrupts: if the device is a part of R8A7790/R8A7794 SoC > + interrupt specifier for the sole interrupt. > + if the device is a part of R8A7795 SoC "If" since it starts the statement? > + interrupt specifier for the two interrupts. You need to be more verbose here: what two interrupts? Perhaps need to use "interrupt-names"? [...] > diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h > index a157aaaaff6a..1832737063f3 100644 > --- a/drivers/net/ethernet/renesas/ravb.h > +++ b/drivers/net/ethernet/renesas/ravb.h > @@ -809,6 +809,7 @@ struct ravb_private { > > unsigned no_avb_link:1; > unsigned avb_link_active_low:1; > + int emac_irq; > }; Why not add it above the bit fields? [...] > diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c > index 026d98435d87..bf604a869458 100644 > --- a/drivers/net/ethernet/renesas/ravb_main.c > +++ b/drivers/net/ethernet/renesas/ravb_main.c > @@ -1185,16 +1185,36 @@ static const struct ethtool_ops ravb_ethtool_ops = { > static int ravb_open(struct net_device *ndev) > { > struct ravb_private *priv = netdev_priv(ndev); > + struct platform_device *pdev = priv->pdev; You hardly need this variable. > + struct device *dev = &pdev->dev; > int error; > > napi_enable(&priv->napi[RAVB_BE]); > napi_enable(&priv->napi[RAVB_NC]); > > - error = request_irq(ndev->irq, ravb_interrupt, IRQF_SHARED, ndev->name, > - ndev); > - if (error) { > - netdev_err(ndev, "cannot request IRQ\n"); > - goto out_napi_off; > + if (of_device_is_compatible(dev->of_node, Are you going to add more calls as the support for more SoCs get added? > + "renesas,etheravb-r8a7795")) { Please respect the networking code alignment rules, start this line right under 'dev'. > + error = request_irq(ndev->irq, > + ravb_interrupt, IRQF_SHARED, ndev->name, ndev); Likewise. > + if (error) { > + netdev_err(ndev, "cannot request IRQ\n"); > + goto out_napi_off; > + } > + error = request_irq(priv->emac_irq, > + ravb_interrupt, IRQF_SHARED, ndev->name, ndev); Likewise. And using the same handler for both interrupts doesn't look good. > + if (error) { > + netdev_err(ndev, "cannot request IRQ\n"); > + free_irq(ndev->irq, ndev); > + goto out_napi_off; > + } > + } > + else { > + error = request_irq(ndev->irq, ravb_interrupt, IRQF_SHARED, > + ndev->name, ndev); Please respect the alignment rules. > + if (error) { > + netdev_err(ndev, "cannot request IRQ\n"); > + goto out_napi_off; > + } I don't understand why you have to duplicate this call in both branches. > } > > /* Device init */ > @@ -1219,7 +1239,11 @@ out_ptp_stop: > /* Stop PTP Clock driver */ > ravb_ptp_stop(ndev); > out_free_irq: > - free_irq(ndev->irq, ndev); > + if (of_device_is_compatible(dev->of_node, > + "renesas,etheravb-r8a7795")) > + free_irq(priv->emac_irq, ndev); What about the AVB-DMAC interrupt in this case? > + else > + free_irq(ndev->irq, ndev); This should be done regardless. [...] > @@ -1688,6 +1712,16 @@ static int ravb_probe(struct platform_device *pdev) > priv->avb_link_active_low = > of_property_read_bool(np, "renesas,ether-link-active-low"); > > + if (of_device_is_compatible(np, > + "renesas,etheravb-r8a7795")) { Please respect the rules. And do you really need the break the line here? > + irq = platform_get_irq(pdev, 1); > + if (irq < 0) { > + error = -ENODEV; Please propagate the error instead. > + goto out_release; > + } > + priv->emac_irq = irq; > + } > + [...] MBR, Sergei -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Aug 27, 2015 at 01:01:50PM +0200, Geert Uytterhoeven wrote: > On Thu, Aug 27, 2015 at 11:24 AM, Simon Horman > <horms+renesas@verge.net.au> wrote: > > --- a/Documentation/devicetree/bindings/net/renesas,ravb.txt > > +++ b/Documentation/devicetree/bindings/net/renesas,ravb.txt > > @@ -6,8 +6,12 @@ interface contains. > > Required properties: > > - compatible: "renesas,etheravb-r8a7790" if the device is a part of R8A7790 SoC. > > "renesas,etheravb-r8a7794" if the device is a part of R8A7794 SoC. > > + "renesas,etheravb-r8a7795" if the device is a part of R8A7795 SoC. > > - reg: offset and length of (1) the register block and (2) the stream buffer. > > -- interrupts: interrupt specifier for the sole interrupt. > > +- interrupts: if the device is a part of R8A7790/R8A7794 SoC > > + interrupt specifier for the sole interrupt. > > + if the device is a part of R8A7795 SoC > > + interrupt specifier for the two interrupts. > > If there are multiple interrupts, you best make them named interrupts, > i.e. requiring "interrupt-names", too. Thanks, I will look into that. > Why are there only 2 interrupts? The datasheet mentions 25, for ch0-ch24. Thanks for bringing that up. I think it has also come up elsewhere in this thread. I'd like to focus on addressing it here rather than spreading the discussion around any further. My understanding is that on the R-Car Gen3 r8a7795 SoC the EthernetAVB hardware may function in one of two modes. 1. A mode which is "mostly" compatible with R-Car Gen2 (e.g. r8a7790, r8a7790). In this mode only ch22 and ch24 are used. I note that the emac interrupt also appears to be documented for Gen-2. Its not clear to me at this time why it isn't appropriate to use it on those SoCs. 2. A mode which is new in Gen 3. In this mode ch0 - ch24 are used. I believe that in this mode there are per DMA queue interrupts. -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Aug 27, 2015 at 02:03:37PM +0300, Sergei Shtylyov wrote: > On 8/27/2015 12:24 PM, Simon Horman wrote: > > >From: Kazuya Mizuguchi <kazuya.mizuguchi.ks@renesas.com> > > > >Signed-off-by: Kazuya Mizuguchi <kazuya.mizuguchi.ks@renesas.com> > >[horms: updated changelog] > > I don't see any. ;-) > > >Signed-off-by: Simon Horman <horms+renesas@verge.net.au> > >--- > > .../devicetree/bindings/net/renesas,ravb.txt | 6 ++- > > drivers/net/ethernet/renesas/ravb.h | 1 + > > drivers/net/ethernet/renesas/ravb_main.c | 47 +++++++++++++++++++--- > > 3 files changed, 47 insertions(+), 7 deletions(-) > > > >diff --git a/Documentation/devicetree/bindings/net/renesas,ravb.txt b/Documentation/devicetree/bindings/net/renesas,ravb.txt > >index 1fd8831437bf..0b4ec02c35a4 100644 > >--- a/Documentation/devicetree/bindings/net/renesas,ravb.txt > >+++ b/Documentation/devicetree/bindings/net/renesas,ravb.txt > >@@ -6,8 +6,12 @@ interface contains. > > Required properties: > > - compatible: "renesas,etheravb-r8a7790" if the device is a part of R8A7790 SoC. > > "renesas,etheravb-r8a7794" if the device is a part of R8A7794 SoC. > >+ "renesas,etheravb-r8a7795" if the device is a part of R8A7795 SoC. > > - reg: offset and length of (1) the register block and (2) the stream buffer. > >-- interrupts: interrupt specifier for the sole interrupt. > >+- interrupts: if the device is a part of R8A7790/R8A7794 SoC > >+ interrupt specifier for the sole interrupt. > >+ if the device is a part of R8A7795 SoC > > "If" since it starts the statement? > > >+ interrupt specifier for the two interrupts. > > You need to be more verbose here: what two interrupts? Perhaps need to > use "interrupt-names"? Thanks, I will look into that. > [...] > >diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h > >index a157aaaaff6a..1832737063f3 100644 > >--- a/drivers/net/ethernet/renesas/ravb.h > >+++ b/drivers/net/ethernet/renesas/ravb.h > >@@ -809,6 +809,7 @@ struct ravb_private { > > > > unsigned no_avb_link:1; > > unsigned avb_link_active_low:1; > >+ int emac_irq; > > }; > > Why not add it above the bit fields? Using an int seems reasonable to me as it is used to hold an integer value returned by request_irq(). > [...] > >diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c > >index 026d98435d87..bf604a869458 100644 > >--- a/drivers/net/ethernet/renesas/ravb_main.c > >+++ b/drivers/net/ethernet/renesas/ravb_main.c > >@@ -1185,16 +1185,36 @@ static const struct ethtool_ops ravb_ethtool_ops = { > > static int ravb_open(struct net_device *ndev) > > { > > struct ravb_private *priv = netdev_priv(ndev); > >+ struct platform_device *pdev = priv->pdev; > > You hardly need this variable. > > >+ struct device *dev = &pdev->dev; > > int error; > > > > napi_enable(&priv->napi[RAVB_BE]); > > napi_enable(&priv->napi[RAVB_NC]); > > > >- error = request_irq(ndev->irq, ravb_interrupt, IRQF_SHARED, ndev->name, > >- ndev); > >- if (error) { > >- netdev_err(ndev, "cannot request IRQ\n"); > >- goto out_napi_off; > >+ if (of_device_is_compatible(dev->of_node, > > Are you going to add more calls as the support for more SoCs get added? I think it is reasonable to assume that is likely that support for more SoCs will be added. However, I am not aware of any immediate plans to do so. > > >+ "renesas,etheravb-r8a7795")) { > > Please respect the networking code alignment rules, start this line right > under 'dev'. Sure, will do. Likewise elsewhere in this patch. > >+ error = request_irq(ndev->irq, > >+ ravb_interrupt, IRQF_SHARED, ndev->name, ndev); > > Likewise. > > >+ if (error) { > >+ netdev_err(ndev, "cannot request IRQ\n"); > >+ goto out_napi_off; > >+ } > >+ error = request_irq(priv->emac_irq, > >+ ravb_interrupt, IRQF_SHARED, ndev->name, ndev); > > Likewise. > And using the same handler for both interrupts doesn't look good. It does seem a little odd. I will confirm that detail. > >+ if (error) { > >+ netdev_err(ndev, "cannot request IRQ\n"); > >+ free_irq(ndev->irq, ndev); > >+ goto out_napi_off; > >+ } > >+ } > >+ else { > >+ error = request_irq(ndev->irq, ravb_interrupt, IRQF_SHARED, > >+ ndev->name, ndev); > > Please respect the alignment rules. > > >+ if (error) { > >+ netdev_err(ndev, "cannot request IRQ\n"); > >+ goto out_napi_off; > >+ } > > I don't understand why you have to duplicate this call in both branches. Thanks, I'll see about cleaning that up. > > } > > > > /* Device init */ > >@@ -1219,7 +1239,11 @@ out_ptp_stop: > > /* Stop PTP Clock driver */ > > ravb_ptp_stop(ndev); > > out_free_irq: > >- free_irq(ndev->irq, ndev); > >+ if (of_device_is_compatible(dev->of_node, > >+ "renesas,etheravb-r8a7795")) > >+ free_irq(priv->emac_irq, ndev); > > What about the AVB-DMAC interrupt in this case? > > >+ else > >+ free_irq(ndev->irq, ndev); > > This should be done regardless. Thanks, that looks wrong. I'll see about fixing it. > [...] > >@@ -1688,6 +1712,16 @@ static int ravb_probe(struct platform_device *pdev) > > priv->avb_link_active_low = > > of_property_read_bool(np, "renesas,ether-link-active-low"); > > > >+ if (of_device_is_compatible(np, > >+ "renesas,etheravb-r8a7795")) { > > Please respect the rules. And do you really need the break the line here? > > >+ irq = platform_get_irq(pdev, 1); > >+ if (irq < 0) { > >+ error = -ENODEV; > > Please propagate the error instead. Sure, will do. > > >+ goto out_release; > >+ } > >+ priv->emac_irq = irq; > >+ } > >+ > [...] > > MBR, Sergei > -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Aug 28, 2015 at 11:01:20AM +0900, Simon Horman wrote: > On Thu, Aug 27, 2015 at 02:03:37PM +0300, Sergei Shtylyov wrote: > > On 8/27/2015 12:24 PM, Simon Horman wrote: [snip] > > >@@ -1688,6 +1712,16 @@ static int ravb_probe(struct platform_device *pdev) > > > priv->avb_link_active_low = > > > of_property_read_bool(np, "renesas,ether-link-active-low"); > > > > > >+ if (of_device_is_compatible(np, > > >+ "renesas,etheravb-r8a7795")) { > > > > Please respect the rules. And do you really need the break the line here? > > > > >+ irq = platform_get_irq(pdev, 1); > > >+ if (irq < 0) { > > >+ error = -ENODEV; > > > > Please propagate the error instead. > > Sure, will do. On closer examination the above is consistent with the existing error handling for another call to platform_get_irq() in the same function. Shall we fix that first? -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 8/28/2015 5:01 AM, Simon Horman wrote: >>> From: Kazuya Mizuguchi <kazuya.mizuguchi.ks@renesas.com> >>> >>> Signed-off-by: Kazuya Mizuguchi <kazuya.mizuguchi.ks@renesas.com> >>> [horms: updated changelog] >> >> I don't see any. ;-) >> >>> Signed-off-by: Simon Horman <horms+renesas@verge.net.au> >>> --- >>> .../devicetree/bindings/net/renesas,ravb.txt | 6 ++- >>> drivers/net/ethernet/renesas/ravb.h | 1 + >>> drivers/net/ethernet/renesas/ravb_main.c | 47 +++++++++++++++++++--- >>> 3 files changed, 47 insertions(+), 7 deletions(-) >>> >>> diff --git a/Documentation/devicetree/bindings/net/renesas,ravb.txt b/Documentation/devicetree/bindings/net/renesas,ravb.txt >>> index 1fd8831437bf..0b4ec02c35a4 100644 >>> --- a/Documentation/devicetree/bindings/net/renesas,ravb.txt >>> +++ b/Documentation/devicetree/bindings/net/renesas,ravb.txt [...] >>> diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h >>> index a157aaaaff6a..1832737063f3 100644 >>> --- a/drivers/net/ethernet/renesas/ravb.h >>> +++ b/drivers/net/ethernet/renesas/ravb.h >>> @@ -809,6 +809,7 @@ struct ravb_private { >>> >>> unsigned no_avb_link:1; >>> unsigned avb_link_active_low:1; >>> + int emac_irq; >>> }; >> Why not add it above the bit fields? > Using an int seems reasonable to me as it is used to hold > an integer value returned by request_irq(). No question about the field type, only about its placement. [...] >>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c >>> index 026d98435d87..bf604a869458 100644 >>> --- a/drivers/net/ethernet/renesas/ravb_main.c >>> +++ b/drivers/net/ethernet/renesas/ravb_main.c >>> @@ -1185,16 +1185,36 @@ static const struct ethtool_ops ravb_ethtool_ops = { [...] >>> + if (error) { >>> + netdev_err(ndev, "cannot request IRQ\n"); >>> + goto out_napi_off; >>> + } >>> + error = request_irq(priv->emac_irq, >>> + ravb_interrupt, IRQF_SHARED, ndev->name, ndev); >> >> Likewise. >> And using the same handler for both interrupts doesn't look good. > It does seem a little odd. I will confirm that detail. If the EMAC indeed uses a separate interrupt, please specify its handler directly. The same for the separate AVB-DMAC interrupt (I somewhat doubt that we really need it in the current driver though). [...] MBR, Sergei -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello. On 8/28/2015 5:13 AM, Simon Horman wrote: >>>> @@ -1688,6 +1712,16 @@ static int ravb_probe(struct platform_device *pdev) >>>> priv->avb_link_active_low = >>>> of_property_read_bool(np, "renesas,ether-link-active-low"); >>>> >>>> + if (of_device_is_compatible(np, >>>> + "renesas,etheravb-r8a7795")) { >>> >>> Please respect the rules. And do you really need the break the line here? >>> >>>> + irq = platform_get_irq(pdev, 1); >>>> + if (irq < 0) { >>>> + error = -ENODEV; >>> >>> Please propagate the error instead. >> Sure, will do. > On closer examination the above is consistent with the existing > error handling for another call to platform_get_irq() in the same function. > Shall we fix that first? Yes, and I'll look into that. WBR, Sergei -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Aug 28, 2015 at 01:20:49PM +0300, Sergei Shtylyov wrote: > On 8/28/2015 5:01 AM, Simon Horman wrote: > > >>>From: Kazuya Mizuguchi <kazuya.mizuguchi.ks@renesas.com> > >>> > >>>Signed-off-by: Kazuya Mizuguchi <kazuya.mizuguchi.ks@renesas.com> > >>>[horms: updated changelog] > >> > >> I don't see any. ;-) > >> > >>>Signed-off-by: Simon Horman <horms+renesas@verge.net.au> > >>>--- > >>> .../devicetree/bindings/net/renesas,ravb.txt | 6 ++- > >>> drivers/net/ethernet/renesas/ravb.h | 1 + > >>> drivers/net/ethernet/renesas/ravb_main.c | 47 +++++++++++++++++++--- > >>> 3 files changed, 47 insertions(+), 7 deletions(-) > >>> > >>>diff --git a/Documentation/devicetree/bindings/net/renesas,ravb.txt b/Documentation/devicetree/bindings/net/renesas,ravb.txt > >>>index 1fd8831437bf..0b4ec02c35a4 100644 > >>>--- a/Documentation/devicetree/bindings/net/renesas,ravb.txt > >>>+++ b/Documentation/devicetree/bindings/net/renesas,ravb.txt > [...] > >>>diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h > >>>index a157aaaaff6a..1832737063f3 100644 > >>>--- a/drivers/net/ethernet/renesas/ravb.h > >>>+++ b/drivers/net/ethernet/renesas/ravb.h > >>>@@ -809,6 +809,7 @@ struct ravb_private { > >>> > >>> unsigned no_avb_link:1; > >>> unsigned avb_link_active_low:1; > >>>+ int emac_irq; > >>> }; > > >> Why not add it above the bit fields? > > >Using an int seems reasonable to me as it is used to hold > >an integer value returned by request_irq(). > > No question about the field type, only about its placement. Sorry, I miss read your comment. I'll move the field if you like. > [...] > >>>diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c > >>>index 026d98435d87..bf604a869458 100644 > >>>--- a/drivers/net/ethernet/renesas/ravb_main.c > >>>+++ b/drivers/net/ethernet/renesas/ravb_main.c > >>>@@ -1185,16 +1185,36 @@ static const struct ethtool_ops ravb_ethtool_ops = { > [...] > >>>+ if (error) { > >>>+ netdev_err(ndev, "cannot request IRQ\n"); > >>>+ goto out_napi_off; > >>>+ } > >>>+ error = request_irq(priv->emac_irq, > >>>+ ravb_interrupt, IRQF_SHARED, ndev->name, ndev); > >> > >> Likewise. > >> And using the same handler for both interrupts doesn't look good. > > >It does seem a little odd. I will confirm that detail. > > If the EMAC indeed uses a separate interrupt, please specify its handler > directly. The same for the separate AVB-DMAC interrupt (I somewhat doubt > that we really need it in the current driver though). Ok, I will look into making it so. -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 08/28/2015 01:30 PM, Sergei Shtylyov wrote: >>>>> @@ -1688,6 +1712,16 @@ static int ravb_probe(struct platform_device *pdev) >>>>> priv->avb_link_active_low = >>>>> of_property_read_bool(np, "renesas,ether-link-active-low"); >>>>> >>>>> + if (of_device_is_compatible(np, >>>>> + "renesas,etheravb-r8a7795")) { >>>> >>>> Please respect the rules. And do you really need the break the line here? >>>> >>>>> + irq = platform_get_irq(pdev, 1); >>>>> + if (irq < 0) { >>>>> + error = -ENODEV; >>>> >>>> Please propagate the error instead. > >>> Sure, will do. > >> On closer examination the above is consistent with the existing >> error handling for another call to platform_get_irq() in the same function. >> Shall we fix that first? > Yes, and I'll look into that. Accidentally spotted the same error in sh_eth.c... really old one. :-) Will fix both. WBR, Sergei -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Aug 28, 2015 at 04:21:14PM +0300, Sergei Shtylyov wrote: > On 08/28/2015 01:30 PM, Sergei Shtylyov wrote: > > >>>>>@@ -1688,6 +1712,16 @@ static int ravb_probe(struct platform_device *pdev) > >>>>> priv->avb_link_active_low = > >>>>> of_property_read_bool(np, "renesas,ether-link-active-low"); > >>>>> > >>>>>+ if (of_device_is_compatible(np, > >>>>>+ "renesas,etheravb-r8a7795")) { > >>>> > >>>> Please respect the rules. And do you really need the break the line here? > >>>> > >>>>>+ irq = platform_get_irq(pdev, 1); > >>>>>+ if (irq < 0) { > >>>>>+ error = -ENODEV; > >>>> > >>>> Please propagate the error instead. > > > >>>Sure, will do. > > > >>On closer examination the above is consistent with the existing > >>error handling for another call to platform_get_irq() in the same function. > >>Shall we fix that first? > > > Yes, and I'll look into that. > > Accidentally spotted the same error in sh_eth.c... really old one. :-) > Will fix both. Thanks, I see both of those changes went into net-next. I'll update this patch accordingly. -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/Documentation/devicetree/bindings/net/renesas,ravb.txt b/Documentation/devicetree/bindings/net/renesas,ravb.txt index 1fd8831437bf..0b4ec02c35a4 100644 --- a/Documentation/devicetree/bindings/net/renesas,ravb.txt +++ b/Documentation/devicetree/bindings/net/renesas,ravb.txt @@ -6,8 +6,12 @@ interface contains. Required properties: - compatible: "renesas,etheravb-r8a7790" if the device is a part of R8A7790 SoC. "renesas,etheravb-r8a7794" if the device is a part of R8A7794 SoC. + "renesas,etheravb-r8a7795" if the device is a part of R8A7795 SoC. - reg: offset and length of (1) the register block and (2) the stream buffer. -- interrupts: interrupt specifier for the sole interrupt. +- interrupts: if the device is a part of R8A7790/R8A7794 SoC + interrupt specifier for the sole interrupt. + if the device is a part of R8A7795 SoC + interrupt specifier for the two interrupts. - phy-mode: see ethernet.txt file in the same directory. - phy-handle: see ethernet.txt file in the same directory. - #address-cells: number of address cells for the MDIO bus, must be equal to 1. diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h index a157aaaaff6a..1832737063f3 100644 --- a/drivers/net/ethernet/renesas/ravb.h +++ b/drivers/net/ethernet/renesas/ravb.h @@ -809,6 +809,7 @@ struct ravb_private { unsigned no_avb_link:1; unsigned avb_link_active_low:1; + int emac_irq; }; static inline u32 ravb_read(struct net_device *ndev, enum ravb_reg reg) diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c index 026d98435d87..bf604a869458 100644 --- a/drivers/net/ethernet/renesas/ravb_main.c +++ b/drivers/net/ethernet/renesas/ravb_main.c @@ -1185,16 +1185,36 @@ static const struct ethtool_ops ravb_ethtool_ops = { static int ravb_open(struct net_device *ndev) { struct ravb_private *priv = netdev_priv(ndev); + struct platform_device *pdev = priv->pdev; + struct device *dev = &pdev->dev; int error; napi_enable(&priv->napi[RAVB_BE]); napi_enable(&priv->napi[RAVB_NC]); - error = request_irq(ndev->irq, ravb_interrupt, IRQF_SHARED, ndev->name, - ndev); - if (error) { - netdev_err(ndev, "cannot request IRQ\n"); - goto out_napi_off; + if (of_device_is_compatible(dev->of_node, + "renesas,etheravb-r8a7795")) { + error = request_irq(ndev->irq, + ravb_interrupt, IRQF_SHARED, ndev->name, ndev); + if (error) { + netdev_err(ndev, "cannot request IRQ\n"); + goto out_napi_off; + } + error = request_irq(priv->emac_irq, + ravb_interrupt, IRQF_SHARED, ndev->name, ndev); + if (error) { + netdev_err(ndev, "cannot request IRQ\n"); + free_irq(ndev->irq, ndev); + goto out_napi_off; + } + } + else { + error = request_irq(ndev->irq, ravb_interrupt, IRQF_SHARED, + ndev->name, ndev); + if (error) { + netdev_err(ndev, "cannot request IRQ\n"); + goto out_napi_off; + } } /* Device init */ @@ -1219,7 +1239,11 @@ out_ptp_stop: /* Stop PTP Clock driver */ ravb_ptp_stop(ndev); out_free_irq: - free_irq(ndev->irq, ndev); + if (of_device_is_compatible(dev->of_node, + "renesas,etheravb-r8a7795")) + free_irq(priv->emac_irq, ndev); + else + free_irq(ndev->irq, ndev); out_napi_off: napi_disable(&priv->napi[RAVB_NC]); napi_disable(&priv->napi[RAVB_BE]); @@ -1688,6 +1712,16 @@ static int ravb_probe(struct platform_device *pdev) priv->avb_link_active_low = of_property_read_bool(np, "renesas,ether-link-active-low"); + if (of_device_is_compatible(np, + "renesas,etheravb-r8a7795")) { + irq = platform_get_irq(pdev, 1); + if (irq < 0) { + error = -ENODEV; + goto out_release; + } + priv->emac_irq = irq; + } + /* Set function */ ndev->netdev_ops = &ravb_netdev_ops; ndev->ethtool_ops = &ravb_ethtool_ops; @@ -1821,6 +1855,7 @@ static const struct dev_pm_ops ravb_dev_pm_ops = { static const struct of_device_id ravb_match_table[] = { { .compatible = "renesas,etheravb-r8a7790" }, { .compatible = "renesas,etheravb-r8a7794" }, + { .compatible = "renesas,etheravb-r8a7795" }, { } }; MODULE_DEVICE_TABLE(of, ravb_match_table);