Re: [PATCH net v2 0/3] net: phy: mscc: support VSC8501

Andrew Lunn
2023-05-23T13:17:04Z

> - I left the mutex_lock(&phydev->lock) in the
> vsc85xx_update_rgmii_cntl() function, as I'm not sure whether it
> is required to repeatedly access phydev->interface and
> phy_interface_is_rgmii(phydev) in a consistent way.

Just adding to Russell comment.

As a general rule of thumb, if your driver is doing something which no
other driver is doing, you have to consider if it is correct. A PHY
driver taking phydev->lock is very unusual. So at minimum you should
be able to explain why it is needed. And when it comes to locking,
locking is hard, so you really should understand it.

Now the mscc is an odd device, because it has multiple PHYs in the
package, and a number of registers are shared between these PHYs. So
it does have different locking requirements to most PHYs. However, i
don't think that is involved here. Those oddities are hidden behind
phy_base_write() and phy_base_read().

Andrew