Re: [PATCH] phy: rockchip-emmc: emmc_phy_init() always return 0

[Chris Ruehl <chris.ruehl@gtsys.com.hk>]

Hi,

On 2/12/2020 4:36 pm, Chris Ruehl wrote:
> 
> On 2/12/2020 12:05 am, Doug Anderson wrote:
>> Hi,
>>
>> On Mon, Nov 30, 2020 at 7:10 PM Chris Ruehl <chris.ruehl@gtsys.com.hk> wrote:
>>>
>>> rockchip_emmc_phy_init() return variable is not set with the error value
>>> if clk_get() failed. The debug message print 0 on error and the function
>>> always return 0.
>>> Fix it using PTR_ERR().
>>>
>>> Fixes: 52c0624a10cce phy: rockchip-emmc: Set phyctrl_frqsel based on card clock
>>>
>>> Signed-off-by: Chris Ruehl <chris.ruehl@gtsys.com.hk>
>>> ---
>>>   drivers/phy/rockchip/phy-rockchip-emmc.c | 1 +
>>>   1 file changed, 1 insertion(+)
>>>
>>> diff --git a/drivers/phy/rockchip/phy-rockchip-emmc.c 
>>> b/drivers/phy/rockchip/phy-rockchip-emmc.c
>>> index 48e2d75b1004..75faee5c0d27 100644
>>> --- a/drivers/phy/rockchip/phy-rockchip-emmc.c
>>> +++ b/drivers/phy/rockchip/phy-rockchip-emmc.c
>>> @@ -253,6 +253,7 @@ static int rockchip_emmc_phy_init(struct phy *phy)
>>>           */
>>>          rk_phy->emmcclk = clk_get(&phy->dev, "emmcclk");
>>>          if (IS_ERR(rk_phy->emmcclk)) {
>>> +               ret = PTR_ERR(rk_phy->emmcclk);
>>
>> I'm pretty sure your patch isn't correct and it would break use cases.
>> Is it fixing some bug that you're aware of, or you found it via code
>> inspection?
>>
>> Specifically:
>>
>> * The big comment block in this function says that the clock is
>> optional and that we're ignoring errors.
>>
>> * The printout in this function is "dbg" level, which is an extra
>> indication that we aren't concerned with these errors.
>>
>> Arguably the code could be made better.  If you want to improve it,
>> you could check for just the error we expect if the clock isn't
>> specified (probably -ENODEV, but you should check) and treat all other
>> failures as real errors.
>>
>>
>> -Doug
>>
> 
> Hi Doug,
>   I reviewed the code while hunting behind an other bug, with hs400
> and yes I saw the comment that they don't care about the problem
> if the clk_get() return an error, and set the rk_phy->emmcclk = NULL
> regardless, not using the ret variable but define it isn't useful.
> 
> If return a error code break something on the other hand, better it
> hit it rather then suppress it in IMHO.
> 
> Let me follow the caller of the function and see how they treat the
> err != 0.
> 
> If something is in danger, I will be effected with my rk3399 rollout :)
> 
> Chris
> 

I check my case, the dts properties emmcclk is defined for the rk3399.
(checked it I do not have an error, clk_get() works)
If clk_get() failed and we propagate error<0 to the phy-core.c which then
not increase the phy->init_count, but throw error message that something goes
wrong.

Someone should explain to me, why we should cover up an error.

Chris


Otherwise