Re: [PATCH 06/12] net: phy: add phy_device_atomic_register helper

[Marco Felsch]

On 23-04-05, Andrew Lunn wrote:
> > The current fwnode_mdio.c don't provide the proper helper functions yet.
> > Instead the parsing is spread between fwnode_mdiobus_register_phy() and
> > fwnode_mdiobus_phy_device_register(). Of course these can be extracted
> > and exported but I don't see the benefit. IMHO it just cause jumping
> > around files and since fwnode is a proper firmware abstraction we could
> > use is directly wihin core/lib files.
> 
> No, assuming fwnode is the proper firmware abstraction is wrong. You
> need to be very careful any time you convert of_ to fwnode_ and look
> at the history of every property. Look at the number of deprecated OF
> properties in Documentation/devicetree/bindings. They should never be
> moved to fwnode_ because then you are moving deprecated properties to
> ACPI, which never had them in the first place! 

The handling of deprecated properties is always a pain. Drivers handling
deprecated properties correctly for of_ should handle it correctly for
fwnode_ too. IMHO it would be driver bug if not existing deprecated
properties cause an error.  Of course there will be properties which
need special attention for ACPI case but I don't see a problem for
deprecated properties since those must be handled correctly for of_ case
too.

> You cannot assume DT and ACPI are the same thing, have the same
> binding. And the same is true, in theory, in the opposite direction.
> We don't want the DT properties polluted with ACPI only properties.
> Not that anybody takes ACPI seriously in networking.

My assumption was that ACPI is becoming more closer to OF and the
fwnode/device abstraction is the abstraction to have one driver
interacting correctly with OF and ACPI. As I said above there will be
some corner-cases which need special attention of course :/

Also while answering this mail, I noticed that there are already some
'small' fwnode/device_ helpers within phy_device.c. So why not bundling
everything within phy_device.c?

> > I know and I thought about adding the firmware parsing helpers first but
> > than I went this way. I can split this of course to make the patch
> > smaller.
> 
> Please do. Also, i read your commit message thinking it was a straight
> copy of the code, and hence i did not need to review the code. But in
> fact it is new code. So i need to take a close look at it.
> 
> But what i think is most important for this patchset is the
> justification for not fixing the current API. Why is it broken beyond
> repair?

Currently we have one API which creates/allocates the 'struct
phy_device' and intialize the state which is:
   - phy_device_create()

This function requests a driver based on the phy_id/c45_ids. The ID have
to come from somewhere if autodection is used. For autodetection case
   - get_phy_device()

is called. This function try to access the phy without taken possible
hardware dependencies into account. These dependecies can be reset-lines
(in my case), clocks, supplies, ...

For taking fwnode (and possible dependencies) into account fwnode_mdio.c
was written which provides two helpers:
   - fwnode_mdiobus_register_phy()
   - fwnode_mdiobus_phy_device_register().

The of_mdio.c and of_mdiobus_register_phy() is just a wrapper around
fwnode_mdiobus_register_phy().

fwnode_mdiobus_register_phy():
   1st) calls get_phy_device() in case of autodection or c45. If phy_id
        is provided and !c45 case phy_device_create() is called to get a
	'struct phy_device'
        - The autodection/c45 case try to access the PHYID registers
	  which is not possible, please see above.
   2nd) call fwnode_mdiobus_phy_device_register() or
        phy_device_register() directly.
	- phy_device_register() is the first time we taking the possible
	  hardware reset line into account, which is far to late.

fwnode_mdiobus_phy_device_register():
   - takes a 'struct phy_device' as parameter, again this have to come
     from somewhere.
   - calls phy_device_register() which is taken the possibel hardware
     reset into account, again to late.

Why do I need the autodection? Because PHYs can be changed due to EOL,
cheaper device, ... I don't wanna have a new devicetree/firmware for the
updated product, just let the magic happen :)

Why do I introduce a new API?
  1st) There are working users of get_phy_device() and I don't wanna
       break their systems, so I left this logic untouched. 
  2nd) The fwnode API is replaced by this new one, since it is
       broken (please see above). 
  3rd) IMHO the 'phy request/phy create' handling is far to complex
       therefore I introduced a single API which:
       - intialize all structures and states
       - prepare the device for interaction by using fwnode
       - initialize/detect the device and requests the coorect phy
	 module
       - applies the fixups
       - add the device to the kernel
       - finally return the 'struct phy_device' to the user, so the
	 driver can do $stuff.
  4th) The new 'struct phy_device_config' makes it easier to
       adapt/extend the API.

Thanks a lot for your fast response and feedback :)

Regards,
  Marco