Hi,
On Wednesday 10 October 2018 04:13 AM, Grygorii Strashko wrote:
>
>
> On 10/09/2018 12:22 AM, Kishon Vijay Abraham I wrote:
>> Hi Grygorii,
>>
>> On Tuesday 09 October 2018 05:19 AM, Grygorii Strashko wrote:
>>> Add new API phy_set_netif_mode(struct phy *phy, phy_interface_t mode) and
>>> new PHY operation callback .set_netif_mode() which intended to be implemnte
>>> by PHY drivers which supports Network interrfaces mode selection. Both
>>> accepts phy_interface_t vlaue as input parameter.
>>>
>>> Cc: Kishon Vijay Abraham I <kishon@xxxxxx>
>>> Cc: Tony Lindgren <tony@xxxxxxxxxxx>
>>> Signed-off-by: Grygorii Strashko <grygorii.strashko@xxxxxx>
>>> ---
>>> drivers/phy/phy-core.c | 15 +++++++++++++++
>>> include/linux/phy/phy.h | 12 ++++++++++++
>>> 2 files changed, 27 insertions(+)
>>>
>>> diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c
>>> index 35fd38c..d9aba1a 100644
>>> --- a/drivers/phy/phy-core.c
>>> +++ b/drivers/phy/phy-core.c
>>> @@ -377,6 +377,21 @@ int phy_set_mode(struct phy *phy, enum phy_mode mode)
>>> }
>>> EXPORT_SYMBOL_GPL(phy_set_mode);
>>>
>>> +int phy_set_netif_mode(struct phy *phy, phy_interface_t mode)
>>> +{
>>> + int ret;
>>> +
>>> + if (!phy || !phy->ops->set_netif_mode)
>>> + return 0;
>>> +
>>> + mutex_lock(&phy->mutex);
>>> + ret = phy->ops->set_netif_mode(phy, mode);
>>> + mutex_unlock(&phy->mutex);
>>> +
>>> + return ret;
>>> +}
>>> +EXPORT_SYMBOL_GPL(phy_set_netif_mode);
>>
>> We should try to add only generic PHY APIs and not subsystem specific APIs. In
>> this case I think phy_set_mode should suffice.
>
>
> This is what I've had in mind first, but all my guts argued against it after I've tried:
>
> diff --git a/include/linux/phy/phy.h b/include/linux/phy/phy.h
> index bc73d2b..961b156 100644
> --- a/include/linux/phy/phy.h
> +++ b/include/linux/phy/phy.h
> @@ -41,6 +41,14 @@ enum phy_mode {
> PHY_MODE_10GKR,
> PHY_MODE_UFS_HS_A,
> PHY_MODE_UFS_HS_B,
> + PHY_MODE_MODE_MII,
> + PHY_MODE_MODE_GMII,
> + PHY_MODE_MODE_SGMII,
> + PHY_MODE_MODE_RMII,
> + PHY_MODE_MODE_RGMII,
> + PHY_MODE_MODE_RGMII_ID,
> + PHY_MODE_MODE_RGMII_RXID,
> + PHY_MODE_MODE_RGMII_TXID,
> };
>
> above introduces ugly constants duplication and required every network phy driver
> to maintain conversation table phy_interface_t -> enum phy_mode.
> More over, if above change happens third time (first time PHY_MODE_SGMII/PHY_MODE_10GKR were added,
> second - PHY_MODE_2500SGMII) it will never ends (there are ~15 more items only in phy_interface_t).
> As result, enum phy_mode might became a un-maintainable monster.
>
> So, as per above, and considering that Network subsystem is based on standards (phy_interface_t lists standard intf)
> I've tried to add separate PHY API.
>
> As an idea:
> - seems it could be reasonable to introduce PHY_MODE_NETWORK (or PHY_MODE_ETHERNET) and
> add generic phy_set_submode(struct phy *phy, long submode).
>
> So, single functional PHY device can just use phy_set_submode() and
> multi-functional devices (like serdes which can be muxed between PCIe, USB, NET), can use:
>
> phy_set_mode(PHY_MODE_ETHERNET)
> phy_set_submode(X);
Agreed on the constant duplication comment above. We can modify set_mode to
take submode as an additional parameter and fix all the users of phy_set_mode.
int phy_set_mode(struct phy *phy, enum phy_mode mode, int submode)
Thanks
Kishon
