Messages in this thread | | | Subject | Re: [RFC net-next] net: phy: introduce phy_reg_field interface | From | "Radu Nicolae Pirea (OSS)" <> | Date | Fri, 31 Mar 2023 17:38:48 +0300 |
| |
On Fri, 2023-03-31 at 15:07 +0200, Andrew Lunn wrote: > 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! I agree with this. I am trying to solve the wrong problem.
> > 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. The nxp-c45-tja11xx driver will be the user of this kind of abstraction layer. I was looking to get a quick review on this, before sending it integrated into a patch series.
> > > +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 * I am trying to protect the __phy_read_mmd and __phy_read calls, not the reg_field->mmd.
Radu P. > > Andrew
| |