Re: [PATCH/RFC 04/10] ravb: Add support for r8a7795 SoC

[Simon Horman <horms@verge.net.au>]

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
>