Messages in this thread | | | Date | Mon, 20 Apr 2020 18:11:22 +0200 | From | Michael Walle <> | Subject | Re: [PATCH net-next 3/3] net: phy: bcm54140: add hwmon support |
| |
Am 2020-04-20 17:36, schrieb Andrew Lunn: >> Ok I see, but what locking do you have in mind? We could have >> something >> like >> >> __phy_package_write(struct phy_device *dev, u32 regnum, u16 val) >> { >> return __mdiobus_write(phydev->mdio.bus, phydev->shared->addr, >> regnum, val); >> } >> >> and its phy_package_write() equivalent. But that would just be >> convenience functions, nothing where you actually help the user with >> locking. Am I missing something? > > In general, drivers should not be using __foo functions. We want > drivers to make use of phy_package_write() which would do the bus > locking. Look at a typical PHY driver. There is no locking what so > ever. Just lots of phy_read() and phy write(). The locking is done by > the core and so should be correct.
Ok, but for example the BCM54140 uses indirect register access and thus need to lock the mdio bus itself in which case I need the __funcs.
>> > > > Get the core to do reference counting on the structure? >> > > > Add helpers phy_read_shared(), phy_write_shared(), etc, which does >> > > > MDIO accesses on the base device, taking care of the locking. >> > > > >> > > The "base" access is another thing, I guess, which has nothing to do >> > > with the shared structure. >> > > >> > I'm making the assumption that all global addresses are at the base >> > address. If we don't want to make that assumption, we need the change >> > the API above so you pass a cookie, and all PHYs need to use the same >> > cookie to identify the package. >> >> how would a phy driver deduce a common cookie? And how would that be a >> difference to using a PHY address. > > For a cookie, i don't care how the driver decides on the cookie. The > core never uses it, other than comparing cookies to combine individual > PHYs into a package. It could be a PHY address. It could be the PHY > address where the global registers are. Or it could be anything else. > >> > Maybe base is the wrong name, since MSCC can have the base as the high >> > address of the four, not the low? >> >> I'd say it might be any of the four addresses as long as it is the >> same >> across the PHYs in the same package. And in that case you can also >> have >> the phy_package_read/write() functions. > > Yes. That is the semantics which is think is most useful. But then we > don't have a cookie, the value has real significance, and we need to > document what is should mean.
Agreed.
I will post a RFC shortly.
-michael
| |