linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Chris Ruehl <chris.ruehl@gtsys.com.hk>
To: Doug Anderson <dianders@chromium.org>
Cc: Ulf Hansson <ulf.hansson@linaro.org>,
	Heiko Stuebner <heiko@sntech.de>,
	LKML <linux-kernel@vger.kernel.org>,
	Kishon Vijay Abraham I <kishon@ti.com>,
	"open list:ARM/Rockchip SoC..."
	<linux-rockchip@lists.infradead.org>,
	Vinod Koul <vkoul@kernel.org>,
	Linux ARM <linux-arm-kernel@lists.infradead.org>
Subject: Re: [PATCH] phy: rockchip-emmc: emmc_phy_init() always return 0
Date: Fri, 4 Dec 2020 11:01:40 +0800	[thread overview]
Message-ID: <e6d351f9-232c-ebab-2f4e-bf2ff4dc8238@gtsys.com.hk> (raw)
In-Reply-To: <f8a21465-91d0-9967-e559-d8f91f795028@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

  reply	other threads:[~2020-12-04  3:02 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-12-01  3:10 Chris Ruehl
2020-12-01 16:05 ` Doug Anderson
2020-12-02  8:36   ` Chris Ruehl
2020-12-04  3:01     ` Chris Ruehl [this message]
2020-12-04 17:53       ` Doug Anderson

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=e6d351f9-232c-ebab-2f4e-bf2ff4dc8238@gtsys.com.hk \
    --to=chris.ruehl@gtsys.com.hk \
    --cc=dianders@chromium.org \
    --cc=heiko@sntech.de \
    --cc=kishon@ti.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-rockchip@lists.infradead.org \
    --cc=ulf.hansson@linaro.org \
    --cc=vkoul@kernel.org \
    --subject='Re: [PATCH] phy: rockchip-emmc: emmc_phy_init() always return 0' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).