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

Hi,

Am 2021-02-10 10:03, schrieb Heiner Kallweit:
[..]

quoted

quoted

quoted

+    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.

quoted

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