| Message ID | 20230515195922.617243-1-afd@ti.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | phy: ti: gmii-sel: Allow parent to not be syscon node | expand |
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); >
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);
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(-)