Francois Romieu <romieu@xxxxxxxxxxxxx> wrote:
> 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.
OK. I'll consider it.
>
> [...]
> > > > + 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.
OK.
>
> 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.
OK.
>
> --
> 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
