Messages in this thread | | | Date | Fri, 31 Mar 2023 15:07:42 +0200 | From | Andrew Lunn <> | Subject | Re: [RFC net-next] net: phy: introduce phy_reg_field interface |
| |
On Fri, Mar 31, 2023 at 03:32:59PM +0300, Radu Pirea (OSS) wrote: > Some PHYs can be heavily modified between revisions, and the addresses of > the registers are changed and the register fields are moved from one > register to another. > > To integrate more PHYs in the same driver with the same register fields, > but these register fields were located in different registers at > different offsets, I introduced the phy_reg_fied structure.
Maybe you are solving the wrong problem. Maybe you should be telling the hardware/firmware engineers not to do this!
How many drivers can actually use this? I don't really want to encourage vendors to make such a mess of their hardware, so i'm wondering if this should be hidden away in the driver, if there is only one driver which needs it. If there are multiple drivers which can use this, please do modify at least one other driver to use it, hence showing it is generic.
> +int phy_read_reg_field(struct phy_device *phydev, > + const struct phy_reg_field *reg_field) > +{ > + u16 mask; > + int ret; > + > + if (reg_field->size == 0) { > + phydev_warn(phydev, "Trying to read a reg field of size 0."); > + return -EINVAL; > + } > + > + phy_lock_mdio_bus(phydev); > + if (reg_field->mmd) > + ret = __phy_read_mmd(phydev, reg_field->devad, > + reg_field->reg); > + else > + ret = __phy_read(phydev, reg_field->reg); > + phy_unlock_mdio_bus(phydev); > +
Could you please explain the locking. It appears you are trying to protect reg_field->mmd? Does that really change? Especially since you have _const_ struct phy_reg_field *
Andrew
| |