Re: [PATCHv2] gianfar: Add gfar_change_carrier() for Fixed PHYs

[Joakim Tjernlund]

On Wed, 2018-12-12 at 13:10 +0000, Claudiu Manoil wrote:
> CAUTION: This email originated from outside of the organization. Do not click 
> links or open attachments unless you recognize the sender and know the 
> content is safe.
> 
> 
> > -----Original Message-----
> > From: Andrew Lunn <and...@lunn.ch>
> > Sent: Wednesday, December 12, 2018 2:43 PM
> > To: jo...@infinera.com <joakim.tjernl...@infinera.com>
> > Cc: netdev @ vger . kernel . org <netdev@vger.kernel.org>; Claudiu Manoil
> > <claudiu.man...@nxp.com>; Florian Fainelli <f.faine...@gmail.com>
> > Subject: Re: [PATCHv2] gianfar: Add gfar_change_carrier() for Fixed PHYs
> > 
> > On Wed, Dec 12, 2018 at 01:33:08PM +0100, Joakim Tjernlund wrote:
> > > This allows to control carrier from /sys/class/net/ethX/carrier for
> > > Fixed PHYs.
> > > 
> > > Signed-off-by: Joakim Tjernlund <joakim.tjernl...@infinera.com>
> > > ---
> > >  v2 - Only allow carrier changes for Fixed PHYs
> > > 
> > >  Florian: I have reimpl. this as I think you meant by registering
> > >           a Fixed PHY callback.
> > >  Andrew: Are happy with this as well?
> > 
> > Hi Joakim
> > 
> > The basic idea is O.K.
> > 
> > >  If this is OK I will sent the other 2 drivers.
> > 
> > Rather than replicating the code three times, how about putting all
> > the code in the fixed phy driver, exporting a fixed_phy_change_carrier
> > function which can be assigned to the .ndo.
> > 
> 
> I agree with Andrew on this. This code is not device (eTSEC) specific, it is 
> generic
> and only uses the gianfar driver to get a reference to its net_device.
> Thanks,
> Claudiu

fast check, would you be happy with this in fixed PHY:
--- a/drivers/net/phy/fixed_phy.c
+++ b/drivers/net/phy/fixed_phy.c
@@ -25,6 +25,7 @@
 #include <linux/gpio.h>
 #include <linux/seqlock.h>
 #include <linux/idr.h>
+#include <linux/netdevice.h>
 
 #include "swphy.h"
 
@@ -38,6 +39,7 @@ struct fixed_phy {
        struct phy_device *phydev;
        seqcount_t seqcount;
        struct fixed_phy_status status;
+       bool no_carrier;
        int (*link_update)(struct net_device *, struct fixed_phy_status *);
        struct list_head node;
        int link_gpio;
@@ -48,6 +50,24 @@ static struct fixed_mdio_bus platform_fmb = {
        .phys = LIST_HEAD_INIT(platform_fmb.phys),
 };
 
+int
+fixed_phy_change_carrier(struct net_device *dev, bool new_carrier)
+{
+       struct fixed_mdio_bus *fmb = &platform_fmb;
+       struct phy_device *phydev = dev->phydev;
+       struct fixed_phy *fp;
+
+       if (!phydev || !phydev->mdio.bus)
+               return -EINVAL;
+
+       list_for_each_entry(fp, &fmb->phys, node) {
+               if (fp->addr == phydev->mdio.addr) {
+                       fp->no_carrier = !new_carrier;
+                       return 0;
+               }
+       }
+       return -EINVAL;
+}
 static void fixed_phy_update(struct fixed_phy *fp)
 {
        if (gpio_is_valid(fp->link_gpio))
@@ -72,6 +92,12 @@ static int fixed_mdio_read(struct mii_bus *bus, int 
phy_addr, int reg_num)
                                                        &fp->status);
                                        fixed_phy_update(fp);
                                }
+                               if (fp->no_carrier) {
+                                       
fp->link_update(fp->phydev->attached_dev,
+                                                       &fp->status);
+                                       fp->status.link = 0;
+                               }
+
                                state = fp->status;
                        } while (read_seqcount_retry(&fp->seqcount, s));
 

Then one just set:
  +       .ndo_change_carrier = fixed_phy_change_carrier,
in each ethernet driver.

OK?

 Jocke