diff mbox series

[2/2] net: gmii2rgmii: Switch priv field in mdio device structure

Message ID 1564565779-29537-3-git-send-email-harini.katakam@xilinx.com (mailing list archive)
State New, archived
Headers show
Series Fix GMII2RGMII private field | expand

Commit Message

Harini Katakam July 31, 2019, 9:36 a.m. UTC 
Use the priv field in mdio device structure instead of the one in
phy device structure. The phy device priv field may be used by the
external phy driver and should not be overwritten.

Signed-off-by: Harini Katakam <harini.katakam@xilinx.com>
Reviewed-by: Radhey Shyam Pandey <radhey.shyam.pandey@xilinx.com>
---
 drivers/net/phy/xilinx_gmii2rgmii.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Andrew Lunn Aug. 1, 2019, 4:06 a.m. UTC | #1 
On Wed, Jul 31, 2019 at 03:06:19PM +0530, Harini Katakam wrote:
> Use the priv field in mdio device structure instead of the one in
> phy device structure. The phy device priv field may be used by the
> external phy driver and should not be overwritten.

Hi Harini

I _think_ you could use dev_set_drvdata(&mdiodev->dev) in xgmiitorgmii_probe() and
dev_get_drvdata(&phydev->mdiomdio.dev) in _read_status()

       Andrew
Harini Katakam Aug. 13, 2019, 11:16 a.m. UTC | #2 
Hi Andrew,

On Thu, Aug 1, 2019 at 9:36 AM Andrew Lunn <andrew@lunn.ch> wrote:
>
> On Wed, Jul 31, 2019 at 03:06:19PM +0530, Harini Katakam wrote:
> > Use the priv field in mdio device structure instead of the one in
> > phy device structure. The phy device priv field may be used by the
> > external phy driver and should not be overwritten.
>
> Hi Harini
>
> I _think_ you could use dev_set_drvdata(&mdiodev->dev) in xgmiitorgmii_probe() and
> dev_get_drvdata(&phydev->mdiomdio.dev) in _read_status()

Thanks for the review. This works if I do:
dev_set_drvdata(&priv->phy_dev->mdio.dev->dev) in probe
and then
dev_get_drvdata(&phydev->mdio.dev) in _read_status()

i.e mdiodev in gmii2rgmii probe and priv->phy_dev->mdio are not the same.

If this is acceptable, I can send a v2.

Regards,
Harini
Andrew Lunn Aug. 13, 2019, 1:23 p.m. UTC | #3 
On Tue, Aug 13, 2019 at 04:46:40PM +0530, Harini Katakam wrote:
> Hi Andrew,
> 
> On Thu, Aug 1, 2019 at 9:36 AM Andrew Lunn <andrew@lunn.ch> wrote:
> >
> > On Wed, Jul 31, 2019 at 03:06:19PM +0530, Harini Katakam wrote:
> > > Use the priv field in mdio device structure instead of the one in
> > > phy device structure. The phy device priv field may be used by the
> > > external phy driver and should not be overwritten.
> >
> > Hi Harini
> >
> > I _think_ you could use dev_set_drvdata(&mdiodev->dev) in xgmiitorgmii_probe() and
> > dev_get_drvdata(&phydev->mdiomdio.dev) in _read_status()
> 
> Thanks for the review. This works if I do:
> dev_set_drvdata(&priv->phy_dev->mdio.dev->dev) in probe
> and then
> dev_get_drvdata(&phydev->mdio.dev) in _read_status()
> 
> i.e mdiodev in gmii2rgmii probe and priv->phy_dev->mdio are not the same.
> 
> If this is acceptable, I can send a v2.

Hi Harini

I think this is better, making use of the central driver
infrastructure, rather than inventing something new.

The kernel does have a few helper, spi_get_drvdata, pci_get_drvdata,
hci_get_drvdata. So maybe had add phydev_get_drvdata(struct phy_device
*phydev)?

	Thanks
		Andrew
Harini Katakam Aug. 13, 2019, 3:13 p.m. UTC | #4 
Hi Andrew,

On Tue, Aug 13, 2019 at 6:54 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> On Tue, Aug 13, 2019 at 04:46:40PM +0530, Harini Katakam wrote:
> > Hi Andrew,
> >
> > On Thu, Aug 1, 2019 at 9:36 AM Andrew Lunn <andrew@lunn.ch> wrote:
> > >
> > > On Wed, Jul 31, 2019 at 03:06:19PM +0530, Harini Katakam wrote:
> > > > Use the priv field in mdio device structure instead of the one in
> > > > phy device structure. The phy device priv field may be used by the
> > > > external phy driver and should not be overwritten.
> > >
> > > Hi Harini
> > >
> > > I _think_ you could use dev_set_drvdata(&mdiodev->dev) in xgmiitorgmii_probe() and
> > > dev_get_drvdata(&phydev->mdiomdio.dev) in _read_status()
> >
> > Thanks for the review. This works if I do:
> > dev_set_drvdata(&priv->phy_dev->mdio.dev->dev) in probe
> > and then
> > dev_get_drvdata(&phydev->mdio.dev) in _read_status()
> >
> > i.e mdiodev in gmii2rgmii probe and priv->phy_dev->mdio are not the same.
> >
> > If this is acceptable, I can send a v2.
>
> Hi Harini
>
> I think this is better, making use of the central driver
> infrastructure, rather than inventing something new.

Ok sure.

>
> The kernel does have a few helper, spi_get_drvdata, pci_get_drvdata,
> hci_get_drvdata. So maybe had add phydev_get_drvdata(struct phy_device
> *phydev)?

Maybe phydev_mdio_get_drvdata? Because the driver data member available is
phydev->mdio.dev.driver_data.

Regards,
Harini
Andrew Lunn Aug. 13, 2019, 3:38 p.m. UTC | #5 
> > The kernel does have a few helper, spi_get_drvdata, pci_get_drvdata,
> > hci_get_drvdata. So maybe had add phydev_get_drvdata(struct phy_device
> > *phydev)?
> 
> Maybe phydev_mdio_get_drvdata? Because the driver data member available is
> phydev->mdio.dev.driver_data.

I still prefer phydev_get_drvdata(). It fits with the X_get_drvdata()
pattern, where X is the type of parameter passed to the call, spi,
pci, hci.

We can also add mdiodev_get_drvdata(mdiodev). A few DSA drivers could
use that.

   Andrew
Harini Katakam Sept. 4, 2019, 2:11 p.m. UTC | #6 
Hi Andrew,

On Tue, Aug 13, 2019 at 9:40 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> > > The kernel does have a few helper, spi_get_drvdata, pci_get_drvdata,
> > > hci_get_drvdata. So maybe had add phydev_get_drvdata(struct phy_device
> > > *phydev)?
> >
> > Maybe phydev_mdio_get_drvdata? Because the driver data member available is
> > phydev->mdio.dev.driver_data.
>
> I still prefer phydev_get_drvdata(). It fits with the X_get_drvdata()
> pattern, where X is the type of parameter passed to the call, spi,
> pci, hci.
>
> We can also add mdiodev_get_drvdata(mdiodev). A few DSA drivers could
> use that.

Sorry for the late reply. I just sent a v2 adding
mdiodev_get/set_drvdata helpers
and using them in gmii2rgmii driver.
I did not add a corresponding phydev helper because there is no "struct dev" in
"struct phy_device" and I dint know if there were any users to add the member
and then a helper for driver data. Also,
strutct phy_device { struct mdio_device { struct device }}
is already available and it seemed logical to use that field to
set/get driver data
for gmii2rgmii. Please let me know if v2 is okay.

Regards,
Harini
diff mbox series

Patch

diff --git a/drivers/net/phy/xilinx_gmii2rgmii.c b/drivers/net/phy/xilinx_gmii2rgmii.c
index 2d14493..ba31b5c3 100644
--- a/drivers/net/phy/xilinx_gmii2rgmii.c
+++ b/drivers/net/phy/xilinx_gmii2rgmii.c
@@ -29,7 +29,7 @@  struct gmii2rgmii {
 
 static int xgmiitorgmii_read_status(struct phy_device *phydev)
 {
-	struct gmii2rgmii *priv = phydev->priv;
+	struct gmii2rgmii *priv = phydev->mdio.priv;
 	struct mii_bus *bus = priv->mdio->bus;
 	int addr = priv->mdio->addr;
 	u16 val = 0;
@@ -90,7 +90,7 @@  static int xgmiitorgmii_probe(struct mdio_device *mdiodev)
 	memcpy(&priv->conv_phy_drv, priv->phy_dev->drv,
 	       sizeof(struct phy_driver));
 	priv->conv_phy_drv.read_status = xgmiitorgmii_read_status;
-	priv->phy_dev->priv = priv;
+	priv->phy_dev->mdio.priv = priv;
 	priv->phy_dev->drv = &priv->conv_phy_drv;
 
 	return 0;