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

[Simon Glass]

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