Messages in this thread | | | Date | Sun, 19 Apr 2020 18:47:30 +0200 | From | Michael Walle <> | Subject | Re: [PATCH net-next 3/3] net: phy: bcm54140: add hwmon support |
| |
Am 2020-04-19 18:29, schrieb Andrew Lunn: > On Sun, Apr 19, 2020 at 12:29:23PM +0200, Michael Walle wrote: >> Am 2020-04-17 23:28, schrieb Andrew Lunn: >> > On Fri, Apr 17, 2020 at 11:08:56PM +0200, Michael Walle wrote: >> > > Am 2020-04-17 22:13, schrieb Andrew Lunn: >> > > > > Correct, and this function was actually stolen from there ;) This was >> > > > > actually stolen from the mscc PHY ;) >> > > > >> > > > Which in itself indicates it is time to make it a helper :-) >> > > >> > > Sure, do you have any suggestions? >> > >> > mdiobus_get_phy() does the bit i was complaining about, the mdiobus >> > internal knowledge. >> >> But that doesn't address your other comment. > > Yes, you are right. But i don't think you can easily generalize the > rest. It needs knowledge of the driver private structure to reference > pkg_init. You would have to move that into phy_device. > >> >> > There is also the question of locking. What happens if the PHY devices >> > is unbound while you have an instance of its phydev? >> >> Is there any lock one could take to avoid that? > > phy_attach_direct() does a get_device(). That at least means the > struct device will not go away. I don't know the code well enough to > know if that will also stop the phy_device structure from being freed. > We might need mdiobus_get_phy() to also do a get_device(), and add a > mdiobus_put_phy() which does a put_device(). > >> > What happens if the base PHY is unbound? Are the three others then >> > unusable? >> >> In my case, this would mean the hwmon device is also removed. I don't >> see any other way to do it right now. I guess it would be better to >> have the hwmon device registered to some kind of parent device. > > The phydev structure might go away. But the hardware is still > there. You can access it via address on the bus. What you have to be > careful of is using the phydev for a different phy.
But the hwmon is registered to the device of the PHY which might be unbound. So it will also be removed, correct? FWIW I don't think that is likely to happen in my case ;)
> >> For the BCM54140 there are three different functions: >> (1) PHY functions accessible by the PHYs own address (ie PHY >> status/control) >> (2) PHY functions but only accessible by the global registers (ie >> interrupt enables per PHY of the shared interrupt pin) >> (3) global functions (like sensors, global configuration) >> >> (1) is already supported in the current PHY framework. (2) and (3) >> need the "hack" which uses mdiobus_read/write() with the base >> address. > > Is the _is_pkg_init() function the only place you need to access some > other phy_device structure.
yes.
> Maybe we need a phydev->shared structure, which all PHYs in one > package share?
That came to my mind too. But how could the PHY core find out which shared structure belongs to which phydev? I guess the phydev have to find out, but then how does it tell the PHY core that it wants such a shared structure. Have the (base) PHY address as an identifier?
> 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. Also I presume not every PHY has the base address as some global register access. Eg. this PHY also have "base + 4" (or depending on the configuration base + 3, that is the last PHY of the four) as a special register access.
> pkg_init > is a member of this shared structure. And have a void * priv in shared > for shared driver private data?
if you have a void *priv, why would you need pkg_init, which is an implementation detail of the phydev. I guess it is enough to just have a void *shared (I don't know about the locking for now).
-michael
| |