> +static int regmap_dm9051_phy_reg_write(void *context, unsigned int reg, unsigned int val)
> +{
> + struct board_info *db = context;
> + int ret;
> +
> + regmap_write(db->regmap, DM9051_EPAR, DM9051_PHY | reg);
regmap_write() can return an error code. You should check for it, and
return it. The driver is full of code like this. Always check the
return code.
> + regmap_write(db->regmap, DM9051_EPDRL, val & 0xff);
> + regmap_write(db->regmap, DM9051_EPDRH, (val >> 8) && 0xff);
> + regmap_write(db->regmap, DM9051_EPCR, EPCR_EPOS | EPCR_ERPRW);
> + ret = dm9051_map_poll(db);
> + regmap_write(db->regmap, DM9051_EPCR, 0x0);
> +
> + if (reg == MII_BMCR && !(val & 0x0800))
Use the available defines, BMCR_RESET. This then makes a lot more
sense.
> + mdelay(1); /* need for if activate phyxcer */
However, the MAC driver should not be touching the PHY. The PHY driver
should be resetting the PHY. If the PHY driver uses
genphy_soft_reset(), phy_poll_reset() will poll until the BMCR_RESET
bit is cleared by the PHY indicating it is has completed reset. Or is
the PHY broken and needs longer?
> +static bool dm9051_phymap_writeable(struct device *dev, unsigned int reg)
> +{
> + if (reg == MII_BMSR || reg == MII_PHYSID1 || reg == MII_PHYSID2)
> + return false;
> + return true;
> +}
Do bad things actually happen if you write to these registers?
> +static u8 dm9051_map_read(struct board_info *db, u8 reg)
> +{
> + struct net_device *ndev = db->ndev;
> + unsigned int val = 0;
> + int ret;
> +
> + ret = regmap_read(db->regmap, reg, &val); /* read only one byte */
> + if (unlikely(ret))
> + netif_err(db, drv, ndev, "%s: error %d reading reg %02x\n",
> + __func__, ret, reg);
Don't discard the error, return it to the caller.
> + return val;
> +}
> +
> +static void dm9051_map_write(struct board_info *db, u8 reg, u16 val)
> +{
> + struct net_device *ndev = db->ndev;
> + int ret = regmap_write(db->regmap, reg, val);
> +
> + if (unlikely(ret))
> + netif_err(db, drv, ndev, "%s: error %d writing reg %02x=%04x\n",
> + __func__, ret, reg, val);
Return the error to the caller.
> +static int dm9051_dumpblk(struct board_info *db, unsigned int len)
> +{
> + int ret;
> + u8 rxb[1];
> +
> + while (len--) {
> + ret = hw_dm9051_spi_read(db, DM_SPI_MRCMD, rxb, 1);
> + if (ret < 0)
> + return ret;
> + }
> + return ret;
> +}
It would be good to have a comment why this function is needed. It
appears to be discarding whatever it reads. Why do you need to do
that?
> +static int dm9051_direct_phyread(struct board_info *db, int reg, int *pvalue)
> +{
> + u8 eph, epl;
> + int ret;
> +
> + ret = dm9051_direct_write(db, DM9051_EPAR, DM9051_PHY | reg);
> + if (ret < 0)
> + return ret;
> + ret = dm9051_direct_write(db, DM9051_EPCR, EPCR_ERPRR | EPCR_EPOS);
> + if (ret < 0)
> + return ret;
> +
> + ret = dm9051_direct_poll(db);
> + if (ret)
> + return ret;
> +
> + ret = dm9051_direct_write(db, DM9051_EPCR, 0x0);
> + if (ret < 0)
> + return ret;
> +
> + ret = dm9051_direct_read(db, DM9051_EPDRH, &eph);
> + if (ret < 0)
> + return ret;
> + ret = dm9051_direct_read(db, DM9051_EPDRL, &epl);
> + if (ret < 0)
> + return ret;
> +
> + *pvalue = (eph << 8) | epl;
> + return ret;
> +}
> +
> +static int dm9051_direct_phywrite(struct board_info *db, int reg, int value)
> +{
It is not clear why you need this. You already setup a regmap for
access to the PHY. Why are you not using it?
> +static int dm9051_mdio_read(struct mii_bus *mdiobus, int phy_id, int reg)
> +{
> + struct board_info *db = mdiobus->priv;
> + int val, ret;
> +
> + if (phy_id == DM9051_PHY_ID) {
> + mutex_lock(&db->addr_lock);
> + ret = dm9051_direct_phyread(db, reg, &val);
> + mutex_unlock(&db->addr_lock);
At some point, the locking needs a good looking at. The MDIO layer
provides a lock, so there will not be parallel MDIO operations. regmap
also has a lock. So i wonder if this lock is actually required?
> +static unsigned int dm9051_chipid(struct board_info *db)
> +{
> + struct device *dev = &db->spidev->dev;
> + unsigned int wpidh, wpidl;
> + u16 id = 0;
> +
> + regmap_read(db->regmap, DM9051_PIDH, &wpidh);
> + regmap_read(db->regmap, DM9051_PIDL, &wpidl);
I'm guessing this is one of the first accesses made to the hardware?
You definitely should be looking at the error codes these return.
> +static int dm9051_direct_reset_code(struct board_info *db)
> +{
> + int ret;
> +
> + mdelay(2); /* need before NCR_RST */
> + ret = dm9051_direct_write(db, DM9051_NCR, NCR_RST); /* NCR reset */
> + if (ret < 0)
> + return ret;
A pause before doing a reset? That is odd. What is actually happening
before dm9051_direct_reset_code() is called which means this pause is
required?
Andrew
