Re: [PATCH net-next 3/3] net: phy: bcm54140: add hwmon support

[Michael Walle]

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