Messages in this thread | | | Date | Wed, 10 Feb 2021 10:14:59 +0100 | From | Michael Walle <> | Subject | 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: [..] >>>> + 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
| |