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:

quoted

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.

quoted

+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