netdev - Re: [PATCHi v2] net: sh_eth: Add support of device tree probe

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20130305004335.GF9439@verge.net.au>
Date:	Tue, 5 Mar 2013 09:43:35 +0900
From:	Simon Horman <horms@...ge.net.au>
To:	Mark Rutland <mark.rutland@....com>
Cc:	Nobuhiro Iwamatsu <nobuhiro.iwamatsu.yj@...esas.com>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"devicetree-discuss@...ts.ozlabs.org" 
	<devicetree-discuss@...ts.ozlabs.org>,
	"magnus.damm@...il.com" <magnus.damm@...il.com>,
	"kda@...ux-powerpc.org" <kda@...ux-powerpc.org>
Subject: Re: [PATCHi v2] net: sh_eth: Add support of device tree probe

On Fri, Feb 15, 2013 at 04:32:51PM +0000, Mark Rutland wrote:
> Hello,
> 
> I have a couple of comments on the binding and the way it's parsed.
> 
> On Thu, Feb 14, 2013 at 12:51:31AM +0000, Nobuhiro Iwamatsu wrote:
> > This adds support of device tree probe for Renesas sh-ether driver.
> > 
> > Signed-off-by: Nobuhiro Iwamatsu <nobuhiro.iwamatsu.yj@...esas.com>
> > 
> > ---
> > V2: - Removed ether_setup().
> >     - Fixed typo from "sh-etn" to "sh-eth".
> > 	- Removed "needs-init" and sh-eth,endian from definition of DT.
> > 	- Changed "sh-eth,edmac-endian" instead of "sh-eth,edmac-big-endain"
> > 	  in definition of DT.
> > 
> >  Documentation/devicetree/bindings/net/sh_ether.txt |   41 +++++
> >  drivers/net/ethernet/renesas/sh_eth.c              |  156 +++++++++++++++++---
> >  2 files changed, 173 insertions(+), 24 deletions(-)
> >  create mode 100644 Documentation/devicetree/bindings/net/sh_ether.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/net/sh_ether.txt b/Documentation/devicetree/bindings/net/sh_ether.txt
> > new file mode 100644
> > index 0000000..7415e54
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/net/sh_ether.txt
> > @@ -0,0 +1,41 @@
> > +* Renesas Electronics SuperH EMAC
> > +
> > +This file provides information, what the device node
> > +for the sh_eth interface contains.
> > +
> > +Required properties:
> > +- compatible:                   "renesas,sh-eth";
> > +- interrupt-parent:             The phandle for the interrupt controller that
> > +                                services interrupts for this device.
> 
> Is this really a required property?
> 
> As it's a standard property with a well-defined meaning, is it necessary to
> document?
> 
> > +- reg:                          Offset and length of the register set for the
> > +                                device.
> 
> The example below shows 2 reg values, yet this implies only one is necessary.
> 
> > +- interrupts:                   Interrupt mapping for the sh_eth interrupt
> > +                                sources (vector id).
> 
> How many interrupts are expected, what are they, and in what order should they be in?
> 
> > +- phy-mode:                     String, operation mode of the PHY interface.
> 
> What are the valid values for this?
> 
> If they're defined in another document, it'd be good to reference it.
> 
> > +- sh-eth,register-type:         String, register type of sh_eth.
> > +                                Please select "gigabit", "fast-sh4" or
> > +                                "fast-sh3-sh2".
> 
> Why are these not handled as separate versions using the compatible string to
> differentiate them?

Hi Iwamatasu-san,

Could you please address this review?
I believe it is still applicable as of v5 which you posted yesterday.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html