| 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
| ||
|
Message-ID: <20110704063850.GK15152@ponder.secretlab.ca>
Date: Mon, 4 Jul 2011 00:38:50 -0600
From: Grant Likely <grant.likely@...retlab.ca>
To: Shawn Guo <shawn.guo@...escale.com>
Cc: Shawn Guo <shawn.guo@...aro.org>, patches@...aro.org,
netdev@...r.kernel.org, devicetree-discuss@...ts.ozlabs.org,
Jason Liu <jason.hui@...aro.org>,
"David S. Miller" <davem@...emloft.net>,
linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH] net/fec: add device tree probe support
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@...aro.org>
> > > > > Signed-off-by: Shawn Guo <shawn.guo@...aro.org>
> > > > > Cc: David S. Miller <davem@...emloft.net>
> > > > > Cc: Grant Likely <grant.likely@...retlab.ca>
> > > >
> > > > Minor comments below. After addressing them you can add my:
> > > >
> > > > Acked-by: Grant Likely <grant.likely@...retlab.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.
--
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
Powered by blists - more mailing lists