Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <0d28eb47-a564-2155-875b-5c8dc8aa806e@ti.com>
Date:   Mon, 10 Jul 2023 09:45:30 -0500
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.11.0
Subject: Re: [PATCH] phy: ti: gmii-sel: Allow parent to not be syscon node
Content-Language: en-US
To:     Roger Quadros <rogerq@kernel.org>, Vinod Koul <vkoul@kernel.org>,
        Kishon Vijay Abraham I <kishon@kernel.org>,
        Siddharth Vadapalli <s-vadapalli@ti.com>
CC:     <linux-phy@lists.infradead.org>, <linux-kernel@vger.kernel.org>
References: <20230515195922.617243-1-afd@ti.com>
 <3c7d7d0b-b859-8921-952c-870c9474969c@kernel.org>
From:   Andrew Davis <afd@ti.com>
In-Reply-To: <3c7d7d0b-b859-8921-952c-870c9474969c@kernel.org>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

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);
>