[lkml]   [2021]   [Feb]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
SubjectRe: [PATCH net-next 7/9] net: phy: icplus: select page before writing control register

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


 \ /
  Last update: 2021-02-10 10:32    [W:16.679 / U:0.024 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site