Messages in this thread | | | From | "Ardelean, Alexandru" <> | Subject | Re: [PATCH v4 13/14] net: phy: adin: add ethtool get_stats support | Date | Tue, 13 Aug 2019 05:48:53 +0000 |
| |
On Mon, 2019-08-12 at 16:33 +0200, Andrew Lunn wrote: > [External] > > > +static int adin_read_mmd_stat_regs(struct phy_device *phydev, > > + struct adin_hw_stat *stat, > > + u32 *val) > > +{ > > + int ret; > > + > > + ret = phy_read_mmd(phydev, MDIO_MMD_VEND1, stat->reg1); > > + if (ret < 0) > > + return ret; > > + > > + *val = (ret & 0xffff); > > + > > + if (stat->reg2 == 0) > > + return 0; > > + > > + ret = phy_read_mmd(phydev, MDIO_MMD_VEND1, stat->reg2); > > + if (ret < 0) > > + return ret; > > + > > + *val <<= 16; > > + *val |= (ret & 0xffff); > > + > > + return 0; > > +} > > It still looks like you have not dealt with overflow from the LSB into > the MSB between the two reads.
Apologies for forgetting to address this. I did not intentionally leave it out; this item got lost after V1 [which had the most remarks]. Changelog V1 -> V2 was quite bulky, and I did not look at V1 remarks after I finished V2.
Thanks for snippet.
> > do { > hi1 = phy_read_mmd(phydev, MDIO_MMD_VEND1, stat->reg2); > if (hi1 < 0) > return hi1; > > low = phy_read_mmd(phydev, MDIO_MMD_VEND1, stat->reg1); > if (low < 0) > return low; > > hi2 = phy_read_mmd(phydev, MDIO_MMD_VEND1, stat->reg2); > if (hi2 < 0) > return hi1; > } while (hi1 != hi2) > > return low | (hi << 16); > > Andrew
| |