[U-Boot,03/11] drivers: phy: add generic PHY framework

The PHY framework provides a set of APIs to control a PHY. This API is
derived from the linux version of the generic PHY framework.
Currently the API supports init(), deinit(), power_on, power_off() and
reset(). The framework provides a way to get a reference to a phy from the
device-tree.

Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>
---
 drivers/Kconfig          |  2 ++
 drivers/Makefile         |  1 +
 drivers/phy/Kconfig      | 32 ++++++++++++++++++++++++
 drivers/phy/Makefile     |  2 ++
 drivers/phy/phy-uclass.c | 64 ++++++++++++++++++++++++++++++++++++++++++++++++
 include/dm/uclass-id.h   |  1 +
 include/generic-phy.h    | 36 +++++++++++++++++++++++++++
 7 files changed, 138 insertions(+)
 create mode 100644 drivers/phy/Kconfig
 create mode 100644 drivers/phy/Makefile
 create mode 100644 drivers/phy/phy-uclass.c
 create mode 100644 include/generic-phy.h

Hi Jean-Jacques,

On 14 April 2017 at 05:08, Jean-Jacques Hiblot <jjhiblot@ti.com> wrote:
> The PHY framework provides a set of APIs to control a PHY. This API is
> derived from the linux version of the generic PHY framework.
> Currently the API supports init(), deinit(), power_on, power_off() and
> reset(). The framework provides a way to get a reference to a phy from the
> device-tree.
>
> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>
> ---
>  drivers/Kconfig          |  2 ++
>  drivers/Makefile         |  1 +
>  drivers/phy/Kconfig      | 32 ++++++++++++++++++++++++
>  drivers/phy/Makefile     |  2 ++
>  drivers/phy/phy-uclass.c | 64 ++++++++++++++++++++++++++++++++++++++++++++++++
>  include/dm/uclass-id.h   |  1 +
>  include/generic-phy.h    | 36 +++++++++++++++++++++++++++
>  7 files changed, 138 insertions(+)
>  create mode 100644 drivers/phy/Kconfig
>  create mode 100644 drivers/phy/Makefile
>  create mode 100644 drivers/phy/phy-uclass.c
>  create mode 100644 include/generic-phy.h

I mostly have minor things at this point. Note that I've applied the
patches from your series that I can, so please rebase on master.

Also can you please:
- check your version number (I think you might be up to v3 now)
- include a change log with each patch (patman might help you)
- rebase on u-boot-dm/master

I'm sorry if this comes across as a bit pedantic. But you are creating
a new uclass which I think will be quite important in U-Boot. I
suspect it will be used by USB, perhaps Ethernet and other systems, so
careful design and documentation is pretty important.

>
> diff --git a/drivers/Kconfig b/drivers/Kconfig
> index 0e5d97d..a90ceca 100644
> --- a/drivers/Kconfig
> +++ b/drivers/Kconfig
> @@ -88,6 +88,8 @@ source "drivers/video/Kconfig"
>
>  source "drivers/watchdog/Kconfig"
>
> +source "drivers/phy/Kconfig"
> +
>  config PHYS_TO_BUS
>         bool "Custom physical to bus address mapping"
>         help
> diff --git a/drivers/Makefile b/drivers/Makefile
> index 5d8baa5..0be0624 100644
> --- a/drivers/Makefile
> +++ b/drivers/Makefile
> @@ -7,6 +7,7 @@ obj-$(CONFIG_$(SPL_)CLK)        += clk/
>  obj-$(CONFIG_$(SPL_)LED)       += led/
>  obj-$(CONFIG_$(SPL_)PINCTRL)   += pinctrl/
>  obj-$(CONFIG_$(SPL_)RAM)       += ram/
> +obj-$(CONFIG_$(SPL_)GENERIC_PHY) += phy/

Please maintain the ordering here

>
>  ifdef CONFIG_SPL_BUILD
>
> diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
> new file mode 100644
> index 0000000..d66c9e3
> --- /dev/null
> +++ b/drivers/phy/Kconfig
> @@ -0,0 +1,32 @@
> +
> +menu "PHY Subsystem"
> +
> +config GENERIC_PHY
> +       bool "PHY Core"
> +       depends on DM
> +       help
> +         Generic PHY support.
> +
> +         This framework is designed to provide a generic interface for PHY

PHY means?

> +         devices. PHYs are commonly used for high speed interfaces such as
> +         SATA or PCIe.

Please write out SATA and PCIe in full at least once. This is help so
we should not assume much.

> +         The API provides functions to initialize/deinitialize the
> +         phy, power on/off the phy, and reset the phy. It's meant to be as

Is it PHY or phy?

> +         compatible as possible with the equivalent framework found in the
> +         linux kernel.
> +
> +config SPL_GENERIC_PHY
> +       bool "PHY Core in SPL"
> +       depends on DM
> +       help
> +         Generic PHY support in SPL.
> +
> +         This framework is designed to provide a generic interface for PHY
> +         devices. PHYs are commonly used for high speed interfaces such as
> +         SATA or PCIe.
> +         The API provides functions to initialize/deinitialize the
> +         phy, power on/off the phy, and reset the phy. It's meant to be as
> +         compatible as possible with the equivalent framework found in the
> +         linux kernel.
> +
> +endmenu
> diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile
> new file mode 100644
> index 0000000..b29a8b9
> --- /dev/null
> +++ b/drivers/phy/Makefile
> @@ -0,0 +1,2 @@
> +obj-$(CONFIG_$(SPL_)GENERIC_PHY) += phy-uclass.o
> +
> diff --git a/drivers/phy/phy-uclass.c b/drivers/phy/phy-uclass.c
> new file mode 100644
> index 0000000..e15ed43
> --- /dev/null
> +++ b/drivers/phy/phy-uclass.c
> @@ -0,0 +1,64 @@
> +/*
> + * Copyright (C) 2017 Texas Instruments Incorporated - http://www.ti.com/
> + * Written by Jean-Jacques Hiblot  <jjhiblot@ti.com>
> + *
> + * SPDX-License-Identifier:    GPL-2.0+
> + */
> +
> +#include <common.h>
> +#include <dm.h>
> +#include <generic-phy.h>
> +
> +struct udevice *dm_generic_phy_get(struct udevice *parent, const char *string)

Can you make this return an error code instead, and the device pointer
as a parameter? This is how it is generally down in DM. See
uclass_first_device() for example.

Also I think you should drop the dm_ in the name. That prefix is used
for core driver model functions, and DM versions of function that have
a non-DM analogue.

Also I think 'get' is too short. We normally use that for getting by
index. How about generic_phy_get_by_name() or, phy_get_by_name() if
you rename it?

> +{
> +       struct udevice *dev;
> +
> +       int rc = uclass_get_device_by_phandle(UCLASS_PHY, parent,
> +                                          string, &dev);
> +       if (rc) {
> +               debug("unable to find generic_phy device (err: %d)\n", rc);
> +               return ERR_PTR(rc);
> +       }
> +
> +       return dev;
> +}
> +
> +int generic_phy_init(struct udevice *dev)
> +{
> +       struct generic_phy_ops const *ops = dev->driver->ops;

Please use a header-file macro to access ops. See clk-uclass.c for an example.

> +
> +       return (ops && ops->init) ? ops->init(dev) : 0;

You don't need to check ops since it is invalid not to have one. So:

return ops->init ? ops->init(dev) : 0;

> +}
> +
> +int generic_phy_reset(struct udevice *dev)
> +{
> +       struct generic_phy_ops const *ops = dev->driver->ops;
> +
> +       return (ops && ops->reset) ? ops->reset(dev) : 0;
> +}
> +
> +int generic_phy_exit(struct udevice *dev)
> +{
> +       struct generic_phy_ops const *ops = dev->driver->ops;
> +
> +       return (ops && ops->exit) ? ops->exit(dev) : 0;
> +}
> +
> +int generic_phy_power_on(struct udevice *dev)
> +{
> +       struct generic_phy_ops const *ops = dev->driver->ops;
> +
> +       return (ops && ops->power_on) ? ops->power_on(dev) : 0;
> +}
> +
> +int generic_phy_power_off(struct udevice *dev)
> +{
> +       struct generic_phy_ops const *ops = dev->driver->ops;
> +
> +       return (ops && ops->power_off) ? ops->power_off(dev) : 0;
> +}
> +
> +UCLASS_DRIVER(generic_phy) = {
> +       .id             = UCLASS_PHY,

I would like the uclass name to match the header file and the uclass
name. So if you are calling this generic_phy, then the uclass should
be named this too. Same with the directory drivers/phy. If you want to
rename it all to 'phy' then that is fine too.

> +       .name           = "generic_phy",
> +};
> diff --git a/include/dm/uclass-id.h b/include/dm/uclass-id.h
> index 8c92d0b..9d34a32 100644
> --- a/include/dm/uclass-id.h
> +++ b/include/dm/uclass-id.h
> @@ -83,6 +83,7 @@ enum uclass_id {
>         UCLASS_VIDEO,           /* Video or LCD device */
>         UCLASS_VIDEO_BRIDGE,    /* Video bridge, e.g. DisplayPort to LVDS */
>         UCLASS_VIDEO_CONSOLE,   /* Text console driver for video device */
> +       UCLASS_PHY,             /* generic PHY device */

Physical layer device? Can you make your comment a bit more useful?

>
>         UCLASS_COUNT,
>         UCLASS_INVALID = -1,
> diff --git a/include/generic-phy.h b/include/generic-phy.h
> new file mode 100644
> index 0000000..24475f0
> --- /dev/null
> +++ b/include/generic-phy.h
> @@ -0,0 +1,36 @@
> +/*
> + * Copyright (C) 2017 Texas Instruments Incorporated - http://www.ti.com/
> + * Written by Jean-Jacques Hiblot  <jjhiblot@ti.com>
> + *
> + * SPDX-License-Identifier:    GPL-2.0+
> + */
> +
> +#ifndef __GENERIC_PHY_H
> +#define __GENERIC_PHY_H
> +
> +/*
> + * struct udevice_ops - set of function pointers for phy operations
> + * @init: operation to be performed for initializing phy (optionnal)

optional

Can you please put these comments in front of each function in struct
generic_phy_ops as is done with other uclasses? Your description
should explain what the operation does. For example, what happens for
init()? Since the phy has already been probed, what should you not do
in probe() but do in init()?

> + * @exit: operation to be performed while exiting (optionnal)

exiting what? Please add some more detail.

> + * @reset: reset the phy (optionnal).
> + * @power_on: powering on the phy (optionnal)
> + * @power_off: powering off the phy (optionnal)

Are these optional because the phy might be powered on during one of
the other operations? Otherwise it doesn't seem helpful to have the
thing not ever power on.

> + */
> +struct generic_phy_ops {
> +       int     (*init)(struct udevice *phy);
> +       int     (*exit)(struct udevice *phy);
> +       int     (*reset)(struct udevice *phy);
> +       int     (*power_on)(struct udevice *phy);
> +       int     (*power_off)(struct udevice *phy);
> +};
> +
> +
> +int generic_phy_init(struct udevice *phy);

Here you need to repeat your comments from above - see other uclasses
for an example.

> +int generic_phy_reset(struct udevice *phy);
> +int generic_phy_exit(struct udevice *phy);
> +int generic_phy_power_on(struct udevice *phy);
> +int generic_phy_power_off(struct udevice *phy);
> +
> +struct udevice *dm_generic_phy_get(struct udevice *parent, const char *string);

This function need a comment. Also instead of string can you think of
a descriptive name, e.g. phy_name?

> +
> +#endif /*__GENERIC_PHY_H */
> --
> 1.9.1
>

Regards,
Simon

On 15/04/2017 19:10, Simon Glass wrote:
> Hi Jean-Jacques,
>
> On 14 April 2017 at 05:08, Jean-Jacques Hiblot <jjhiblot@ti.com> wrote:
>> The PHY framework provides a set of APIs to control a PHY. This API is
>> derived from the linux version of the generic PHY framework.
>> Currently the API supports init(), deinit(), power_on, power_off() and
>> reset(). The framework provides a way to get a reference to a phy from the
>> device-tree.
>>
>> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>
>> ---
>>   drivers/Kconfig          |  2 ++
>>   drivers/Makefile         |  1 +
>>   drivers/phy/Kconfig      | 32 ++++++++++++++++++++++++
>>   drivers/phy/Makefile     |  2 ++
>>   drivers/phy/phy-uclass.c | 64 ++++++++++++++++++++++++++++++++++++++++++++++++
>>   include/dm/uclass-id.h   |  1 +
>>   include/generic-phy.h    | 36 +++++++++++++++++++++++++++
>>   7 files changed, 138 insertions(+)
>>   create mode 100644 drivers/phy/Kconfig
>>   create mode 100644 drivers/phy/Makefile
>>   create mode 100644 drivers/phy/phy-uclass.c
>>   create mode 100644 include/generic-phy.h
> I mostly have minor things at this point. Note that I've applied the
> patches from your series that I can, so please rebase on master.
>
> Also can you please:
> - check your version number (I think you might be up to v3 now)
> - include a change log with each patch (patman might help you)
> - rebase on u-boot-dm/master
>
> I'm sorry if this comes across as a bit pedantic. But you are creating
> a new uclass which I think will be quite important in U-Boot. I
> suspect it will be used by USB, perhaps Ethernet and other systems, so
> careful design and documentation is pretty important.
>
>> diff --git a/drivers/Kconfig b/drivers/Kconfig
>> index 0e5d97d..a90ceca 100644
>> --- a/drivers/Kconfig
>> +++ b/drivers/Kconfig
>> @@ -88,6 +88,8 @@ source "drivers/video/Kconfig"
>>
>>   source "drivers/watchdog/Kconfig"
>>
>> +source "drivers/phy/Kconfig"
>> +
>>   config PHYS_TO_BUS
>>          bool "Custom physical to bus address mapping"
>>          help
>> diff --git a/drivers/Makefile b/drivers/Makefile
>> index 5d8baa5..0be0624 100644
>> --- a/drivers/Makefile
>> +++ b/drivers/Makefile
>> @@ -7,6 +7,7 @@ obj-$(CONFIG_$(SPL_)CLK)        += clk/
>>   obj-$(CONFIG_$(SPL_)LED)       += led/
>>   obj-$(CONFIG_$(SPL_)PINCTRL)   += pinctrl/
>>   obj-$(CONFIG_$(SPL_)RAM)       += ram/
>> +obj-$(CONFIG_$(SPL_)GENERIC_PHY) += phy/
> Please maintain the ordering here
>
>>   ifdef CONFIG_SPL_BUILD
>>
>> diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
>> new file mode 100644
>> index 0000000..d66c9e3
>> --- /dev/null
>> +++ b/drivers/phy/Kconfig
>> @@ -0,0 +1,32 @@
>> +
>> +menu "PHY Subsystem"
>> +
>> +config GENERIC_PHY
>> +       bool "PHY Core"
>> +       depends on DM
>> +       help
>> +         Generic PHY support.
>> +
>> +         This framework is designed to provide a generic interface for PHY
> PHY means?
>
>> +         devices. PHYs are commonly used for high speed interfaces such as
>> +         SATA or PCIe.
> Please write out SATA and PCIe in full at least once. This is help so
> we should not assume much.
>
>> +         The API provides functions to initialize/deinitialize the
>> +         phy, power on/off the phy, and reset the phy. It's meant to be as
> Is it PHY or phy?
>
>> +         compatible as possible with the equivalent framework found in the
>> +         linux kernel.
>> +
>> +config SPL_GENERIC_PHY
>> +       bool "PHY Core in SPL"
>> +       depends on DM
>> +       help
>> +         Generic PHY support in SPL.
>> +
>> +         This framework is designed to provide a generic interface for PHY
>> +         devices. PHYs are commonly used for high speed interfaces such as
>> +         SATA or PCIe.
>> +         The API provides functions to initialize/deinitialize the
>> +         phy, power on/off the phy, and reset the phy. It's meant to be as
>> +         compatible as possible with the equivalent framework found in the
>> +         linux kernel.
>> +
>> +endmenu
>> diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile
>> new file mode 100644
>> index 0000000..b29a8b9
>> --- /dev/null
>> +++ b/drivers/phy/Makefile
>> @@ -0,0 +1,2 @@
>> +obj-$(CONFIG_$(SPL_)GENERIC_PHY) += phy-uclass.o
>> +
>> diff --git a/drivers/phy/phy-uclass.c b/drivers/phy/phy-uclass.c
>> new file mode 100644
>> index 0000000..e15ed43
>> --- /dev/null
>> +++ b/drivers/phy/phy-uclass.c
>> @@ -0,0 +1,64 @@
>> +/*
>> + * Copyright (C) 2017 Texas Instruments Incorporated - http://www.ti.com/
>> + * Written by Jean-Jacques Hiblot  <jjhiblot@ti.com>
>> + *
>> + * SPDX-License-Identifier:    GPL-2.0+
>> + */
>> +
>> +#include <common.h>
>> +#include <dm.h>
>> +#include <generic-phy.h>
>> +
>> +struct udevice *dm_generic_phy_get(struct udevice *parent, const char *string)
> Can you make this return an error code instead, and the device pointer
> as a parameter? This is how it is generally down in DM. See
> uclass_first_device() for example.
>
> Also I think you should drop the dm_ in the name. That prefix is used
> for core driver model functions, and DM versions of function that have
> a non-DM analogue.
>
> Also I think 'get' is too short. We normally use that for getting by
> index. How about generic_phy_get_by_name() or, phy_get_by_name() if
> you rename it?
>
>> +{
>> +       struct udevice *dev;
>> +
>> +       int rc = uclass_get_device_by_phandle(UCLASS_PHY, parent,
>> +                                          string, &dev);
>> +       if (rc) {
>> +               debug("unable to find generic_phy device (err: %d)\n", rc);
>> +               return ERR_PTR(rc);
>> +       }
>> +
>> +       return dev;
>> +}
>> +
>> +int generic_phy_init(struct udevice *dev)
>> +{
>> +       struct generic_phy_ops const *ops = dev->driver->ops;
> Please use a header-file macro to access ops. See clk-uclass.c for an example.
>
>> +
>> +       return (ops && ops->init) ? ops->init(dev) : 0;
> You don't need to check ops since it is invalid not to have one. So:
>
> return ops->init ? ops->init(dev) : 0;
>
>> +}
>> +
>> +int generic_phy_reset(struct udevice *dev)
>> +{
>> +       struct generic_phy_ops const *ops = dev->driver->ops;
>> +
>> +       return (ops && ops->reset) ? ops->reset(dev) : 0;
>> +}
>> +
>> +int generic_phy_exit(struct udevice *dev)
>> +{
>> +       struct generic_phy_ops const *ops = dev->driver->ops;
>> +
>> +       return (ops && ops->exit) ? ops->exit(dev) : 0;
>> +}
>> +
>> +int generic_phy_power_on(struct udevice *dev)
>> +{
>> +       struct generic_phy_ops const *ops = dev->driver->ops;
>> +
>> +       return (ops && ops->power_on) ? ops->power_on(dev) : 0;
>> +}
>> +
>> +int generic_phy_power_off(struct udevice *dev)
>> +{
>> +       struct generic_phy_ops const *ops = dev->driver->ops;
>> +
>> +       return (ops && ops->power_off) ? ops->power_off(dev) : 0;
>> +}
>> +
>> +UCLASS_DRIVER(generic_phy) = {
>> +       .id             = UCLASS_PHY,
> I would like the uclass name to match the header file and the uclass
> name. So if you are calling this generic_phy, then the uclass should
> be named this too. Same with the directory drivers/phy. If you want to
> rename it all to 'phy' then that is fine too.
IMO 'phy' would be the best option.
Unfortunately there are already tons of functions starting with 'phy_' 
and they're used for the ethernet phy. So I would propose to use 'phy' 
everywhere except for the API where 'generic_phy_' can be used to prefix 
the functions.
What do you think ?
>> +       .name           = "generic_phy",
>> +};
>> diff --git a/include/dm/uclass-id.h b/include/dm/uclass-id.h
>> index 8c92d0b..9d34a32 100644
>> --- a/include/dm/uclass-id.h
>> +++ b/include/dm/uclass-id.h
>> @@ -83,6 +83,7 @@ enum uclass_id {
>>          UCLASS_VIDEO,           /* Video or LCD device */
>>          UCLASS_VIDEO_BRIDGE,    /* Video bridge, e.g. DisplayPort to LVDS */
>>          UCLASS_VIDEO_CONSOLE,   /* Text console driver for video device */
>> +       UCLASS_PHY,             /* generic PHY device */
> Physical layer device? Can you make your comment a bit more useful?
>
>>          UCLASS_COUNT,
>>          UCLASS_INVALID = -1,
>> diff --git a/include/generic-phy.h b/include/generic-phy.h
>> new file mode 100644
>> index 0000000..24475f0
>> --- /dev/null
>> +++ b/include/generic-phy.h
>> @@ -0,0 +1,36 @@
>> +/*
>> + * Copyright (C) 2017 Texas Instruments Incorporated - http://www.ti.com/
>> + * Written by Jean-Jacques Hiblot  <jjhiblot@ti.com>
>> + *
>> + * SPDX-License-Identifier:    GPL-2.0+
>> + */
>> +
>> +#ifndef __GENERIC_PHY_H
>> +#define __GENERIC_PHY_H
>> +
>> +/*
>> + * struct udevice_ops - set of function pointers for phy operations
>> + * @init: operation to be performed for initializing phy (optionnal)
> optional
>
> Can you please put these comments in front of each function in struct
> generic_phy_ops as is done with other uclasses? Your description
> should explain what the operation does. For example, what happens for
> init()? Since the phy has already been probed, what should you not do
> in probe() but do in init()?
I'll work on documenting this. Too bad that linux also lacks this 
documentation.
The core idea is that probe() does not do much hardware-wise, it sets up 
everyting (irqs, mapping, clocks) but the actual initialization of the 
hardware is done in init().

Jean-Jacques

>> + * @exit: operation to be performed while exiting (optionnal)
> exiting what? Please add some more detail.
>
>> + * @reset: reset the phy (optionnal).
>> + * @power_on: powering on the phy (optionnal)
>> + * @power_off: powering off the phy (optionnal)
> Are these optional because the phy might be powered on during one of
> the other operations? Otherwise it doesn't seem helpful to have the
> thing not ever power on.
>
>> + */
>> +struct generic_phy_ops {
>> +       int     (*init)(struct udevice *phy);
>> +       int     (*exit)(struct udevice *phy);
>> +       int     (*reset)(struct udevice *phy);
>> +       int     (*power_on)(struct udevice *phy);
>> +       int     (*power_off)(struct udevice *phy);
>> +};
>> +
>> +
>> +int generic_phy_init(struct udevice *phy);
> Here you need to repeat your comments from above - see other uclasses
> for an example.
>
>> +int generic_phy_reset(struct udevice *phy);
>> +int generic_phy_exit(struct udevice *phy);
>> +int generic_phy_power_on(struct udevice *phy);
>> +int generic_phy_power_off(struct udevice *phy);
>> +
>> +struct udevice *dm_generic_phy_get(struct udevice *parent, const char *string);
> This function need a comment. Also instead of string can you think of
> a descriptive name, e.g. phy_name?
>
>> +
>> +#endif /*__GENERIC_PHY_H */
>> --
>> 1.9.1
>>
> Regards,
> Simon
>

Hi

,[..]

>>> +UCLASS_DRIVER(generic_phy) = {
>>> +       .id             = UCLASS_PHY,
>>
>> I would like the uclass name to match the header file and the uclass
>> name. So if you are calling this generic_phy, then the uclass should
>> be named this too. Same with the directory drivers/phy. If you want to
>> rename it all to 'phy' then that is fine too.
>
> IMO 'phy' would be the best option.
> Unfortunately there are already tons of functions starting with 'phy_' and
> they're used for the ethernet phy. So I would propose to use 'phy'
> everywhere except for the API where 'generic_phy_' can be used to prefix the
> functions.
> What do you think ?

Sounds good. That would be easy to clean up later once Ethernet Phy is done.

>
>>> +       .name           = "generic_phy",
>>> +};
>>> diff --git a/include/dm/uclass-id.h b/include/dm/uclass-id.h
>>> index 8c92d0b..9d34a32 100644
>>> --- a/include/dm/uclass-id.h
>>> +++ b/include/dm/uclass-id.h
>>> @@ -83,6 +83,7 @@ enum uclass_id {
>>>          UCLASS_VIDEO,           /* Video or LCD device */
>>>          UCLASS_VIDEO_BRIDGE,    /* Video bridge, e.g. DisplayPort to
>>> LVDS */
>>>          UCLASS_VIDEO_CONSOLE,   /* Text console driver for video device
>>> */
>>> +       UCLASS_PHY,             /* generic PHY device */
>>
>> Physical layer device? Can you make your comment a bit more useful?
>>
>>>          UCLASS_COUNT,
>>>          UCLASS_INVALID = -1,
>>> diff --git a/include/generic-phy.h b/include/generic-phy.h
>>> new file mode 100644
>>> index 0000000..24475f0
>>> --- /dev/null
>>> +++ b/include/generic-phy.h
>>> @@ -0,0 +1,36 @@
>>> +/*
>>> + * Copyright (C) 2017 Texas Instruments Incorporated -
>>> http://www.ti.com/
>>> + * Written by Jean-Jacques Hiblot  <jjhiblot@ti.com>
>>> + *
>>> + * SPDX-License-Identifier:    GPL-2.0+
>>> + */
>>> +
>>> +#ifndef __GENERIC_PHY_H
>>> +#define __GENERIC_PHY_H
>>> +
>>> +/*
>>> + * struct udevice_ops - set of function pointers for phy operations
>>> + * @init: operation to be performed for initializing phy (optionnal)
>>
>> optional
>>
>> Can you please put these comments in front of each function in struct
>> generic_phy_ops as is done with other uclasses? Your description
>> should explain what the operation does. For example, what happens for
>> init()? Since the phy has already been probed, what should you not do
>> in probe() but do in init()?
>
> I'll work on documenting this. Too bad that linux also lacks this
> documentation.
> The core idea is that probe() does not do much hardware-wise, it sets up
> everyting (irqs, mapping, clocks) but the actual initialization of the
> hardware is done in init().

OK sounds good.

Regards,
Simon