Byungho An <bh74.an@xxxxxxxxxxx> :
[...]
> > Nit: you may consider reorganizing the variables in an inverted xmas tree
> > fashion at some point.
> Does it look better? No problem.
Marginally if not more. Consider it a guideline to avoid unusual or ugly
layout.
[...]
> > > + priv->ioaddr + SXGBE_MDIO_CLAUSE22_PORT_REG);
> > > + /* set mdio address register */
> > > + reg_val = (phyaddr << 16) | (phyreg & 0x1F);
> > > + writel(reg_val, priv->ioaddr + mii_addr);
> > > +
> > > + /* set mdio control/data register */
> > > + reg_val = ((SXGBE_SMA_READ_CMD << 16) |
> > SXGBE_SMA_SKIP_ADDRFRM |
> > > + ((priv->clk_csr & 0x7) << 19) | SXGBE_MII_BUSY);
> > > + writel(reg_val, priv->ioaddr + mii_data);
> > > + }
> >
> > The whole 'if (phyreg & MII_ADDR_C45) { ... } else { ... }' chunk is shared
> with
> > sxgbe_mdio_write(..., phydata = 0). Factor out ?
> Not exactly same.
Almost :o)
static void sxgbe_mdio_ctrl_data(struct spgbe_priv_data *sp, u32 cmd,
u16 phydata)
{
u32 reg = phydata;
reg |= (cmd << 16) | SXGBE_SMA_SKIP_ADDRFRM |
((sp->clk_csr & 0x7) << 19) | SXGBE_MII_BUSY;
writel(reg, sp->ioaddr + sp->hw->mii.data);
}
static void sxgbe_mdio_c45(struct spgbe_priv_data *sp, u32 cmd, int phyaddr,
int phyreg, u16 phydata)
{
u32 reg;
/* set mdio address register */
reg = ((phyreg >> 16) & 0x1f) << 21;
reg |= (phyaddr << 16) | (phyreg & 0xffff);
writel(reg, sp->ioaddr + sp->hw->mii.addr);
sxgbe_mdio_ctrl_data(sp, cmd, phydata);
}
static int sxgbe_mdio_c22(struct spgbe_priv_data *sp, u32 cmd, int phyaddr,
int phyreg, u16 phydata)
{
u32 reg
writel(1 << phyaddr, ioaddr + SXGBE_MDIO_CLAUSE22_PORT_REG);
/* set mdio address register */
reg = (phyaddr << 16) | (phyreg & 0x1f);
writel(reg, sp->ioaddr + sp->hw->mii.addr);
sxgbe_mdio_ctrl_data(sp, cmd, phydata);
}
static int spgbe_mdio_access(struct spgbe_priv_data *sp, u32 cmd, int phyaddr,
int phyreg, u16 phydata)
{
const struct mii_regs *mii = &sp->hw->mii;
int rc;
rc = spgbe_mdio_busy_wait(sp->ioaddr, mii->data);
if (rc < 0)
return rc;
if (phyreg & MII_ADDR_C45) {
spgbe_mdio_c45(sp, cmd, phyaddr, phyreg, phydata);
} else {
/* Ports 0-3 only support C22. */
if (phyaddr >= 4)
return -ENODEV;
spgbe_mdio_c22(sp, cmd, phyaddr, phyreg, phydata);
}
return spgbe_mdio_busy_wait(sp->ioaddr, mii->data);
}
static int sxgbe_mdio_read(struct mii_bus *bus, int phyaddr, int phyreg)
{
struct net_device *ndev = bus->priv;
struct sxgbe_priv_data *priv = netdev_priv(ndev);
int rc;
rc = sxgbe_mdio_access(priv, SXGBE_SMA_READ_CMD, phyaddr, phyreg, 0);
if (rc < 0)
return rc;
return readl(priv->ioaddr + priv->hw->mii.data) & 0xffff;
}
static int sxgbe_mdio_write(struct mii_bus *bus, int phyaddr, int phyreg,
u16 phydata)
{
struct net_device *ndev = bus->priv;
struct sxgbe_priv_data *priv = netdev_priv(ndev);
return sxgbe_mdio_access(priv, SXGBE_SMA_WRITE_CMD, phyaddr, phyreg,
phydata);
}
It fixes an unchecked sxgbe_mdio_busy_wait in sxgbe_mdio_write btw.
sxgbe_mdio_read and sxgbe_mdio_write are mostly the same. Their core is
imho worth sharing. You're of course free to rewrite or used the code above
as fits your needs.
sxgbe_priv_data should probably be sxgbe_priv, sxgbe or sx. It's everywhere
and it's a first class type in the code. It's exceedingly litterate whereas
common drivers choose to be more lean.
--
Ueimor
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
