phy: ti: gmii-sel: Allow parent to not be syscon node

  If the parent node is not a syscon type, then fallback and check
if we can get a regmap from our own node. This no longer forces
us to make the parent of this node a syscon node when that might
not be appropriate.

Signed-off-by: Andrew Davis <afd@ti.com>
---
 drivers/phy/ti/phy-gmii-sel.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)
  

Andrew,

On 16/05/23 01:29, Andrew Davis wrote:
> If the parent node is not a syscon type, then fallback and check
> if we can get a regmap from our own node. This no longer forces
> us to make the parent of this node a syscon node when that might
> not be appropriate.

Could you please let me know in which cases it is appropriate versus in which
cases it isn't? Is syscon_node_to_regmap() being retained for backward
compatibility until the device-tree nodes are cleaned up across all devices?

> 
> Signed-off-by: Andrew Davis <afd@ti.com>
> ---
>  drivers/phy/ti/phy-gmii-sel.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/phy/ti/phy-gmii-sel.c b/drivers/phy/ti/phy-gmii-sel.c
> index 8c667819c39a..1e67ed9a5cf6 100644
> --- a/drivers/phy/ti/phy-gmii-sel.c
> +++ b/drivers/phy/ti/phy-gmii-sel.c
> @@ -435,9 +435,12 @@ static int phy_gmii_sel_probe(struct platform_device *pdev)
>  
>  	priv->regmap = syscon_node_to_regmap(node->parent);
>  	if (IS_ERR(priv->regmap)) {
> -		ret = PTR_ERR(priv->regmap);
> -		dev_err(dev, "Failed to get syscon %d\n", ret);
> -		return ret;
> +		priv->regmap = device_node_to_regmap(node);

If device_node_to_regmap() can always be used with the corresponding changes
made to the device-tree nodes, wouldn't it be better to use it directly instead
of using it as a fallback? (This is based on the assumption that
syscon_node_to_regmap() is only being retained until the device-tree nodes are
cleaned up to work with device_node_to_regmap()).

> +		if (IS_ERR(priv->regmap)) {
> +			ret = PTR_ERR(priv->regmap);
> +			dev_err(dev, "Failed to get syscon %d\n", ret);
> +			return ret;
> +		}
>  	}
>  
>  	ret = phy_gmii_sel_init_ports(priv);
  

Hi Andrew,

On 15/05/2023 22:59, Andrew Davis wrote:
> If the parent node is not a syscon type, then fallback and check
> if we can get a regmap from our own node. This no longer forces
> us to make the parent of this node a syscon node when that might
> not be appropriate.

Trying to understand the motive for this and if it is better to
introduce a "syscon = <&syscon_node>" property instead which
makes it fool proof for all cases.

> 
> Signed-off-by: Andrew Davis <afd@ti.com>
> ---
>  drivers/phy/ti/phy-gmii-sel.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/phy/ti/phy-gmii-sel.c b/drivers/phy/ti/phy-gmii-sel.c
> index 8c667819c39a..1e67ed9a5cf6 100644
> --- a/drivers/phy/ti/phy-gmii-sel.c
> +++ b/drivers/phy/ti/phy-gmii-sel.c
> @@ -435,9 +435,12 @@ static int phy_gmii_sel_probe(struct platform_device *pdev)
>  
>  	priv->regmap = syscon_node_to_regmap(node->parent);
>  	if (IS_ERR(priv->regmap)) {
> -		ret = PTR_ERR(priv->regmap);
> -		dev_err(dev, "Failed to get syscon %d\n", ret);
> -		return ret;
> +		priv->regmap = device_node_to_regmap(node);
> +		if (IS_ERR(priv->regmap)) {
> +			ret = PTR_ERR(priv->regmap);
> +			dev_err(dev, "Failed to get syscon %d\n", ret);
> +			return ret;
> +		}
>  	}
>  
>  	ret = phy_gmii_sel_init_ports(priv);
  

On 5/15/23 11:05 PM, Siddharth Vadapalli wrote:
> Andrew,
> 
> On 16/05/23 01:29, Andrew Davis wrote:
>> If the parent node is not a syscon type, then fallback and check
>> if we can get a regmap from our own node. This no longer forces
>> us to make the parent of this node a syscon node when that might
>> not be appropriate.
> 
> Could you please let me know in which cases it is appropriate versus in which
> cases it isn't?

Use of syscon nodes should be discouraged, in most cases they can and should be
avoided. The only time we would need to have a syscon parent is when the register
space contains multiple sub-devices with bit level interleaving.

Most devices should never need syscon, we overuse syscon due to driver like
this that have no other option than to make the parent device a syscon node.

> Is syscon_node_to_regmap() being retained for backward
> compatibility until the device-tree nodes are cleaned up across all devices?
> 

Yes, the old way is left only for backwards compatibility.

>>
>> Signed-off-by: Andrew Davis <afd@ti.com>
>> ---
>>   drivers/phy/ti/phy-gmii-sel.c | 9 ++++++---
>>   1 file changed, 6 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/phy/ti/phy-gmii-sel.c b/drivers/phy/ti/phy-gmii-sel.c
>> index 8c667819c39a..1e67ed9a5cf6 100644
>> --- a/drivers/phy/ti/phy-gmii-sel.c
>> +++ b/drivers/phy/ti/phy-gmii-sel.c
>> @@ -435,9 +435,12 @@ static int phy_gmii_sel_probe(struct platform_device *pdev)
>>   
>>   	priv->regmap = syscon_node_to_regmap(node->parent);
>>   	if (IS_ERR(priv->regmap)) {
>> -		ret = PTR_ERR(priv->regmap);
>> -		dev_err(dev, "Failed to get syscon %d\n", ret);
>> -		return ret;
>> +		priv->regmap = device_node_to_regmap(node);
> 
> If device_node_to_regmap() can always be used with the corresponding changes
> made to the device-tree nodes, wouldn't it be better to use it directly instead
> of using it as a fallback? (This is based on the assumption that
> syscon_node_to_regmap() is only being retained until the device-tree nodes are
> cleaned up to work with device_node_to_regmap()).
> 

Yes, that could work too. I was trying to make the minimal changes here, but
if we feel it works better we can have the default be to check the self node first.

Andrew

>> +		if (IS_ERR(priv->regmap)) {
>> +			ret = PTR_ERR(priv->regmap);
>> +			dev_err(dev, "Failed to get syscon %d\n", ret);
>> +			return ret;
>> +		}
>>   	}
>>   
>>   	ret = phy_gmii_sel_init_ports(priv);
>
  

On 5/16/23 1:33 PM, Roger Quadros wrote:
> Hi Andrew,
> 
> On 15/05/2023 22:59, Andrew Davis wrote:
>> If the parent node is not a syscon type, then fallback and check
>> if we can get a regmap from our own node. This no longer forces
>> us to make the parent of this node a syscon node when that might
>> not be appropriate.
> 
> Trying to understand the motive for this and if it is better to
> introduce a "syscon = <&syscon_node>" property instead which
> makes it fool proof for all cases.
> 

My motivation is to reduce our overuse of syscon nodes, IMHO syscon
is almost always a broken design in DT and goes against the standard
usage.

Some drivers like this one force us to make the parent node a syscon
device, even what that is not needed otherwise (the register space is
standalone and the standard DT "reg" property can be used to describe
the device register space).

Using "syscon = <&syscon_node>" could be a useful option for devices
when syscon is actually needed. But I think that should only be used
when the whole node itself cannot be made a child of the syscon node,
making it a child when we can is better for DT organization vs. having
a bunch of top-level nodes that point around to their register spaces
with phandles.

Andrew

>>
>> Signed-off-by: Andrew Davis <afd@ti.com>
>> ---
>>   drivers/phy/ti/phy-gmii-sel.c | 9 ++++++---
>>   1 file changed, 6 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/phy/ti/phy-gmii-sel.c b/drivers/phy/ti/phy-gmii-sel.c
>> index 8c667819c39a..1e67ed9a5cf6 100644
>> --- a/drivers/phy/ti/phy-gmii-sel.c
>> +++ b/drivers/phy/ti/phy-gmii-sel.c
>> @@ -435,9 +435,12 @@ static int phy_gmii_sel_probe(struct platform_device *pdev)
>>   
>>   	priv->regmap = syscon_node_to_regmap(node->parent);
>>   	if (IS_ERR(priv->regmap)) {
>> -		ret = PTR_ERR(priv->regmap);
>> -		dev_err(dev, "Failed to get syscon %d\n", ret);
>> -		return ret;
>> +		priv->regmap = device_node_to_regmap(node);
>> +		if (IS_ERR(priv->regmap)) {
>> +			ret = PTR_ERR(priv->regmap);
>> +			dev_err(dev, "Failed to get syscon %d\n", ret);
>> +			return ret;
>> +		}
>>   	}
>>   
>>   	ret = phy_gmii_sel_init_ports(priv);
>