Re: [PATCH] net: phy: smsc: disable energy detect mode

[Heiko Schocher]

Hello Florian,

Am 14.10.2015 um 06:17 schrieb Heiko Schocher:
> Hello Florian,
>
> Am 13.10.2015 um 21:26 schrieb Florian Fainelli:
> > On 12/10/15 22:13, Heiko Schocher wrote:
> > > On some boards the energy enable detect mode leads in
> > > trouble with some switches, so make the enabling of
> > > this mode configurable through DT.
> > >
> > > Signed-off-by: Heiko Schocher <hs@xxxxxxx>
> > > ---
> > >
> > >   .../devicetree/bindings/net/smsc-lan87xx.txt       | 19 +++++++++++++++++
> > >   drivers/net/phy/smsc.c                             | 24 +++++++++++++++++-----
> > >   2 files changed, 38 insertions(+), 5 deletions(-)
> > >   create mode 100644 Documentation/devicetree/bindings/net/smsc-lan87xx.txt
> > >
> > > diff --git a/Documentation/devicetree/bindings/net/smsc-lan87xx.txt
> > > b/Documentation/devicetree/bindings/net/smsc-lan87xx.txt
> > > new file mode 100644
> > > index 0000000..39aa1dc
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/net/smsc-lan87xx.txt
> > > @@ -0,0 +1,19 @@
> > > +SMSC LAN87xx Ethernet PHY
> > > +
> > > +Some boards require special tuning values. Configure them
> > > +through an Ethernet OF device node.
> > > +
> > > +Optional properties:
> > > +
> > > +- disable-energy-detect:
> > > +  If set, do not enable energy detect mode for the SMSC phy.
> > > +  default: enable energy detect mode
> >
> > Although energy detection is something that is implemented by many PHYs,
> > I am not sure a generic property is suitable here, I would prefix that
> > with the SMSC vendor prefix here to make it clear this only applies to
> > this PHY.
>
> Hmm... but all PHYs should be able to enable, disable it in some way, or?

ping?

> > Would not you want to make it a reverse property here though, something
> > like this:
> >
> > smsc,energy-detect: boolean, when present indicates the PHY reliably
> > supports energy detection
>
> Yes, that was also my first thought, but currently, on this PHYs
> energy detect mode is on ... and if I introduce such a property,
> it will disable it for all existing boards, because property is
> missing ... so, maybe I break boards ...

I have a v2 prepared, but what to do here?

> > > +
> > > +Examples:
> > > +
> > > +    /* Attach to an Ethernet device with autodetected PHY */
> > > +    &cpsw_emac0 {
> > > +        phy_id = <&davinci_mdio>, <0>;
> > > +        phy-mode = "mii";
> > > +        disable-energy-detect;
> > > +    };
> > > diff --git a/drivers/net/phy/smsc.c b/drivers/net/phy/smsc.c
> > > index 70b0895..f90fbf3 100644
> > > --- a/drivers/net/phy/smsc.c
> > > +++ b/drivers/net/phy/smsc.c
> > > @@ -43,16 +43,30 @@ static int smsc_phy_ack_interrupt(struct phy_device *phydev)
> > >
> > >   static int smsc_phy_config_init(struct phy_device *phydev)
> > >   {
> > > +#ifdef CONFIG_OF
> > > +    int len;
> > > +    struct device *dev = &phydev->dev;
> > > +    struct device_node *of_node = dev->of_node;
> >
> > That does not need to be ifdefd out, at best annontate with __maybe_unused?
>
> Yes, I try it.

removed.

> > > +#endif
> > >       int rc = phy_read(phydev, MII_LAN83C185_CTRL_STATUS);
> > > +    int enable_energy = 1;
> > >
> > >       if (rc < 0)
> > >           return rc;
> > >
> > > -    /* Enable energy detect mode for this SMSC Transceivers */
> > > -    rc = phy_write(phydev, MII_LAN83C185_CTRL_STATUS,
> > > -               rc | MII_LAN83C185_EDPWRDOWN);
> > > -    if (rc < 0)
> > > -        return rc;
> > > +#ifdef CONFIG_OF
> > > +    if (!of_node && dev->parent->of_node)
> > > +        of_node = dev->parent->of_node;
> >
> > That looks strange, why would the property be placed at the parent level
> > when this is a PHY device tree node property?
>
> Hmm.. I recheck this.

Hmm.. the drivers/net/ethernet/ti/cpsw.c has no phy-handle
support ... I added this, so my v2 is now a patchset of 2 patches.

bye,
Heiko
--
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/