Re: [PATCH net-next 7/9] net: phy: icplus: select page before writing control register

[Michael Walle]

Hi,

Am 2021-02-10 10:03, schrieb Heiner Kallweit:
[..]
> > > > +    return phy_restore_page(phydev, oldpage, err);
> > >
> > > If a random page was set before entering config_init, do we actually 
> > > want
> > > to restore it? Or wouldn't it be better to set the default page as 
> > > part
> > > of initialization?
> >
> > First, I want to convert this to the match_phy_device() and while at 
> > it,
> > I noticed that there is this one "problem". Short summary: the IP101A 
> > isn't
> > paged, the IP101G has serveral and if page 16 is selected it is more 
> > or
> > less compatible with the IP101A. My problem here is now how to share 
> > the
> > functions for both PHYs without duplicating all the code. Eg. the 
> > IP101A
> > will phy_read/phy_write/phy_modify(), that is, all the locked 
> > versions.
> > For the IP101G I'd either need the _paged() versions or the __phy ones
> > which don't take the mdio_bus lock.
> >
> > Here is what I came up with:
> > (1) provide a common function which uses the __phy ones, then the
> >     callback for the A version will take the mdio_bus lock and calls
> >     the common one. The G version will use 
> > phy_{select,restore}_page().
> > (2) the phy_driver ops for A will also provde a .read/write_page()
> >     callback which is just a no-op. So A can just use the G versions.
> > (3) What Heiner mentioned here, just set the default page in
> >     config_init().
> >
> > (1) will still bloat the code; (3) has the disadvantage, that the
> > userspace might fiddle around with the page register and then the
> > whole PHY driver goes awry. I don't know if we have to respect that
> > use case in general. I know there is an API to read/write the PHY
> > registers and it could happen.
>
> The potential issue you mention here we have with all PHY's using
> pages. As one example, the genphy functions rely on the PHY being
> set to the default page. In general userspace can write PHY register
> values that break processing, independent of paging.
> I'm not aware of any complaints regarding this behavior, therefore
> wouldn't be too concerned here.

I'm fine with that, that will make the driver easier.

> Regarding (2) I'd like to come back to my proposal from yesterday,
> implement match_phy_device to completely decouple the A and G versions.
> Did you consider this option?

Yes, that is what I was talking about above: "First, I want to convert
this to the match_phy_device()" ;) And then I stumbled across that 
problem
I was describing above.

It will likely go away if I just set the page to the default page.

> > That being said, I'm either fine with (2) and (3) but I'm preferring
> > (2).
> >
> > BTW, this patch is still missing read/writes to the interrupt status
> > and control registers which is also paged.

--
-michael