Re: [RFC PATCH 01/11] phy: core add phy_set_netif_mode() api

[Kishon Vijay Abraham I]

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