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

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

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@renesas.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.