[PATCH] net: phy: dp83867: Do not check sgmii if rgmii is already used

[Grygorii Strashko]

On 13/02/2020 08:23, Michal Simek wrote:
> On 12. 02. 20 21:24, Grygorii Strashko wrote:
>>
>>
>> On 11/02/2020 10:11, Michal Simek wrote:
>>> On 10. 02. 20 13:07, Grygorii Strashko wrote:
>>>>
>>>>
>>>> On 07/02/2020 13:31, Michal Simek wrote:
>>>>> There is no reason to check sgmii branch again when it is clear that
>>>>> phy
>>>>> interface is rgmii.
>>>>>
>>>>> Signed-off-by: Michal Simek <michal.simek at xilinx.com>
>>>>> ---
>>>>>
>>>>>     drivers/net/phy/dp83867.c | 4 +---
>>>>>     1 file changed, 1 insertion(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/drivers/net/phy/dp83867.c b/drivers/net/phy/dp83867.c
>>>>> index 4d796e289c45..3178787ff1c7 100644
>>>>> --- a/drivers/net/phy/dp83867.c
>>>>> +++ b/drivers/net/phy/dp83867.c
>>>>> @@ -327,9 +327,7 @@ static int dp83867_config(struct phy_device
>>>>> *phydev)
>>>>>               phy_write_mmd(phydev, DP83867_DEVADDR,
>>>>>                       DP83867_RGMIIDCTL, delay);
>>>>> -    }
>>>>> -
>>>>> -    if (phy_interface_is_sgmii(phydev)) {
>>>>> +    } else if (phy_interface_is_sgmii(phydev)) {
>>>>>             phy_write(phydev, MDIO_DEVAD_NONE, MII_BMCR,
>>>>>                   (BMCR_ANENABLE | BMCR_FULLDPLX | BMCR_SPEED1000));
>>>>>    
>>>>
>>>>   From one side I have no objections, but from another - I'd prefer to
>>>> keep as is.
>>>
>>> Can you please be elaborate on this one more?
>>
>> - keep the same way as in the Kernel
> 
> If kernel does it in the same way it should be also fixed.
> 
> I have been checking yesterday dt binding docs in u-boot and in Linux
> and surprisingly they are different.
> 
> ti,dp83867-rxctrl-strap-quirk is supported in u-boot but not described
> 
> ti,clk-output-sel is supported but even in code is said that it is
> optional property.
> 
> ti,min-output-impedance, ti,max-output-impedance and ti,fifo-depth  are
> not documented in dt binding doc
> 
> ti,sgmii-ref-clock-output-enable is not supported in u-boot but it is in
> Linux and we are using this feature.
> 

My understanding is that u-boot goal is to have uboot-dt == kernel-dt and current
approach add DT+bindings to the kernel first.

So, if you check most of u-boot bindings are missed or obsolete.

> Can you please sync it if you want to keep it in the same was as is done
> in Linux?

So, may be it can be just deleted.

> 
>> - code readability
> 
> I don't think this is really changing code readability. For improving
> readability would be the best to move bodies of these ifs to separate
> functions and not have dp83867_config() ~140 lines long.

It really too minor change to fight for. But if you wish you can update kernel
driver as per-above your proposal and then port it in u-boot.

-- 
Best regards,
grygorii