[PATCH] net/fec: add device tree probe support

[Grant Likely]

On Mon, Jul 04, 2011 at 02:25:22PM +0800, Shawn Guo wrote:
> On Sun, Jul 03, 2011 at 11:59:56PM -0600, Grant Likely wrote:
> > On Mon, Jul 04, 2011 at 01:50:22PM +0800, Shawn Guo wrote:
> > > On Sun, Jul 03, 2011 at 03:23:12PM -0600, Grant Likely wrote:
> > > > On Sun, Jul 03, 2011 at 04:06:41PM +0800, Shawn Guo wrote:
> > > > > It adds device tree probe support for fec driver.
> > > > > 
> > > > > Signed-off-by: Jason Liu <jason.hui at linaro.org>
> > > > > Signed-off-by: Shawn Guo <shawn.guo at linaro.org>
> > > > > Cc: David S. Miller <davem at davemloft.net>
> > > > > Cc: Grant Likely <grant.likely at secretlab.ca>
> > > > 
> > > > Minor comments below.  After addressing them you can add my:
> > > > 
> > > > Acked-by: Grant Likely <grant.likely at secretlab.ca>
> > > > 
> > > > I don't see any reason not to merge this one in v3.1
> > > > 
> > > > g.
> > > > 
> > > > > ---
> > > > >  Documentation/devicetree/bindings/net/fsl-fec.txt |   24 ++++
> > > > >  drivers/net/fec.c                                 |  120 ++++++++++++++++++++-
> > > > >  2 files changed, 139 insertions(+), 5 deletions(-)
> > > > >  create mode 100644 Documentation/devicetree/bindings/net/fsl-fec.txt
> > > > > 
> > > > > diff --git a/Documentation/devicetree/bindings/net/fsl-fec.txt b/Documentation/devicetree/bindings/net/fsl-fec.txt
> > > > > new file mode 100644
> > > > > index 0000000..1dad888
> > > > > --- /dev/null
> > > > > +++ b/Documentation/devicetree/bindings/net/fsl-fec.txt
> > > > > @@ -0,0 +1,24 @@
> > > > > +* Freescale Fast Ethernet Controller (FEC)
> > > > > +
> > > > > +Required properties:
> > > > > +- compatible : Should be "fsl,<soc>-fec"
> > > > > +- reg : Address and length of the register set for the device
> > > > > +- interrupts : Should contain fec interrupt
> > > > > +- phy-mode : String, operation mode of the PHY interface.
> > > > > +  Supported values are: "mii", "gmii", "sgmii", "tbi", "rmii",
> > > > > +  "rgmii", "rgmii-id", "rgmii-rxid", "rgmii-txid", "rtbi".
> > > > 
> > > > We don't have a common binding for this yet.  Should be
> > > > "fsl,phy-mode".
> > > > 
> > > There is nothing really fsl specific.
> > 
> > That's not really the point.  It might be that phy-mode as you're
> > defining it here will not be sufficient for other ethernet devices.
> 
> I'm not defining the phy mode.  Instead, it's just a direct mapping
> of 'enum phy_interface_t' found in include/linux/phy.h, which is
> something common and generic, I think?
> 
> > You should avoid creating 'generic' property definitions, unless
> > you're doing the leg work of figuring out which devices would use this
> > property and creating multiple users.
> > 
> 
> Any net device driver currently referring to 'enum phy_interface_t'
> is going to need this property when migrating to dt, I guess.
> 
> > So, you may be right that this isn't fsl specific, but it is still
> > good practise to use that "fsl," prefix.
> > 
> > 
> > > How does thn following patch
> > > look to you?
> > > 
> > > ---8<---------
> > > diff --git a/drivers/of/of_net.c b/drivers/of/of_net.c
> > > index 86f334a..cc117db 100644
> > > --- a/drivers/of/of_net.c
> > > +++ b/drivers/of/of_net.c
> > > @@ -8,6 +8,49 @@
> > >  #include <linux/etherdevice.h>
> > >  #include <linux/kernel.h>
> > >  #include <linux/of_net.h>
> > > +#include <linux/phy.h>
> > > +
> > > +/**
> > > + * It maps 'enum phy_interface_t' found in include/linux/phy.h
> > > + * into the device tree binding of 'phy-mode', so that Ethernet
> > > + * device driver can get phy interface from device tree.
> > > + */
> > > +static const char *phy_modes[] = {
> > > +       [PHY_INTERFACE_MODE_MII]        = "mii",
> > > +       [PHY_INTERFACE_MODE_GMII]       = "gmii",
> > > +       [PHY_INTERFACE_MODE_SGMII]      = "sgmii",
> > > +       [PHY_INTERFACE_MODE_TBI]        = "tbi",
> > > +       [PHY_INTERFACE_MODE_RMII]       = "rmii",
> > > +       [PHY_INTERFACE_MODE_RGMII]      = "rgmii",
> > > +       [PHY_INTERFACE_MODE_RGMII_ID]   = "rgmii-id",
> > > +       [PHY_INTERFACE_MODE_RGMII_RXID] = "rgmii-rxid",
> > > +       [PHY_INTERFACE_MODE_RGMII_TXID] = "rgmii-txid",
> > > +       [PHY_INTERFACE_MODE_RTBI]       = "rtbi",
> > > +};
> > > +
> > > +/**
> > > + * of_get_phy_mode - Get phy mode for given device_node
> > > + * @np:        Pointer to the given device_node
> > > + *
> > > + * The function gets phy interface string from property 'phy-mode',
> > > + * and return its index in phy_modes table, or errno in error case.
> > > + */
> > > +const int of_get_phy_mode(struct device_node *np)
> > > +{
> > > +       const char *pm;
> > > +       int err, i;
> > > +
> > > +       err = of_property_read_string(np, "phy-mode", &pm);
> > > +       if (err < 0)
> > > +               return err;
> > > +
> > > +       for (i = 0; i < ARRAY_SIZE(phy_modes); i++)
> > > +               if (!strcasecmp(pm, phy_modes[i]))
> > > +                       return i;
> > > +
> > > +       return -ENODEV;
> > > +}
> > > +EXPORT_SYMBOL_GPL(of_get_phy_mode);
> > 
> > The code looks fine, but I'm just not keen on this until I see
> > multiple users.  What are the other DT-aware network drivers doing
> > right now?
> > 
> You can find a bunch of examples using 'phy-mode' in this way with
> the grep below.
> 
> $ grep phy-mode arch/powerpc/boot/dts/*
> 
> The code I can find is drivers/net/ibm_newemac/core.c, function
> emac_init_config.  And I think it should use 'enum phy_interface_t'
> than defining its own PHY_MODE_*.

Fair enough.  Go ahead and create the common definition.  Can you
craft a patch to convert the ibm_newemac driver to use your common
function too?

g.