* [PATCH] phy: rockchip-emmc: emmc_phy_init() always return 0 @ 2020-12-01 3:10 ` Chris Ruehl 0 siblings, 0 replies; 18+ messages in thread From: Chris Ruehl @ 2020-12-01 3:10 UTC (permalink / raw) Cc: Chris Ruehl, Kishon Vijay Abraham I, Vinod Koul, Heiko Stuebner, Douglas Anderson, Ulf Hansson, linux-kernel, linux-arm-kernel, linux-rockchip 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); dev_dbg(&phy->dev, "Error getting emmcclk: %d\n", ret); rk_phy->emmcclk = NULL; } -- 2.20.1 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH] phy: rockchip-emmc: emmc_phy_init() always return 0 @ 2020-12-01 3:10 ` Chris Ruehl 0 siblings, 0 replies; 18+ messages in thread From: Chris Ruehl @ 2020-12-01 3:10 UTC (permalink / raw) Cc: Ulf Hansson, Heiko Stuebner, linux-kernel, Douglas Anderson, Kishon Vijay Abraham I, linux-rockchip, Vinod Koul, Chris Ruehl, linux-arm-kernel 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); dev_dbg(&phy->dev, "Error getting emmcclk: %d\n", ret); rk_phy->emmcclk = NULL; } -- 2.20.1 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH] phy: rockchip-emmc: emmc_phy_init() always return 0 @ 2020-12-01 3:10 ` Chris Ruehl 0 siblings, 0 replies; 18+ messages in thread From: Chris Ruehl @ 2020-12-01 3:10 UTC (permalink / raw) Cc: Ulf Hansson, Heiko Stuebner, linux-kernel, Douglas Anderson, Kishon Vijay Abraham I, linux-rockchip, Vinod Koul, Chris Ruehl, linux-arm-kernel 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); dev_dbg(&phy->dev, "Error getting emmcclk: %d\n", ret); rk_phy->emmcclk = NULL; } -- 2.20.1 _______________________________________________ Linux-rockchip mailing list Linux-rockchip@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-rockchip ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH] phy: rockchip-emmc: emmc_phy_init() always return 0 2020-12-01 3:10 ` Chris Ruehl (?) @ 2020-12-01 16:05 ` Doug Anderson -1 siblings, 0 replies; 18+ messages in thread From: Doug Anderson @ 2020-12-01 16:05 UTC (permalink / raw) To: Chris Ruehl Cc: Kishon Vijay Abraham I, Vinod Koul, Heiko Stuebner, Ulf Hansson, LKML, Linux ARM, open list:ARM/Rockchip SoC... 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] phy: rockchip-emmc: emmc_phy_init() always return 0 @ 2020-12-01 16:05 ` Doug Anderson 0 siblings, 0 replies; 18+ messages in thread From: Doug Anderson @ 2020-12-01 16:05 UTC (permalink / raw) To: Chris Ruehl Cc: Ulf Hansson, Heiko Stuebner, LKML, Kishon Vijay Abraham I, open list:ARM/Rockchip SoC..., Vinod Koul, Linux ARM 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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] phy: rockchip-emmc: emmc_phy_init() always return 0 @ 2020-12-01 16:05 ` Doug Anderson 0 siblings, 0 replies; 18+ messages in thread From: Doug Anderson @ 2020-12-01 16:05 UTC (permalink / raw) To: Chris Ruehl Cc: Ulf Hansson, Heiko Stuebner, LKML, Kishon Vijay Abraham I, open list:ARM/Rockchip SoC..., Vinod Koul, Linux ARM 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 _______________________________________________ Linux-rockchip mailing list Linux-rockchip@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-rockchip ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] phy: rockchip-emmc: emmc_phy_init() always return 0 2020-12-01 16:05 ` Doug Anderson (?) @ 2020-12-02 8:36 ` Chris Ruehl -1 siblings, 0 replies; 18+ messages in thread From: Chris Ruehl @ 2020-12-02 8:36 UTC (permalink / raw) To: Doug Anderson Cc: Kishon Vijay Abraham I, Vinod Koul, Heiko Stuebner, Ulf Hansson, LKML, Linux ARM, open list:ARM/Rockchip SoC... 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] phy: rockchip-emmc: emmc_phy_init() always return 0 @ 2020-12-02 8:36 ` Chris Ruehl 0 siblings, 0 replies; 18+ messages in thread From: Chris Ruehl @ 2020-12-02 8:36 UTC (permalink / raw) To: Doug Anderson Cc: Ulf Hansson, Heiko Stuebner, LKML, Kishon Vijay Abraham I, open list:ARM/Rockchip SoC..., Vinod Koul, Linux ARM 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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] phy: rockchip-emmc: emmc_phy_init() always return 0 @ 2020-12-02 8:36 ` Chris Ruehl 0 siblings, 0 replies; 18+ messages in thread From: Chris Ruehl @ 2020-12-02 8:36 UTC (permalink / raw) To: Doug Anderson Cc: Ulf Hansson, Heiko Stuebner, LKML, Kishon Vijay Abraham I, open list:ARM/Rockchip SoC..., Vinod Koul, Linux ARM 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 _______________________________________________ Linux-rockchip mailing list Linux-rockchip@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-rockchip ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] phy: rockchip-emmc: emmc_phy_init() always return 0 2020-12-02 8:36 ` Chris Ruehl (?) @ 2020-12-04 3:01 ` Chris Ruehl -1 siblings, 0 replies; 18+ messages in thread From: Chris Ruehl @ 2020-12-04 3:01 UTC (permalink / raw) To: Doug Anderson Cc: Ulf Hansson, Heiko Stuebner, LKML, Kishon Vijay Abraham I, open list:ARM/Rockchip SoC..., Vinod Koul, Linux ARM 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] phy: rockchip-emmc: emmc_phy_init() always return 0 @ 2020-12-04 3:01 ` Chris Ruehl 0 siblings, 0 replies; 18+ messages in thread From: Chris Ruehl @ 2020-12-04 3:01 UTC (permalink / raw) To: Doug Anderson Cc: Ulf Hansson, Heiko Stuebner, LKML, Kishon Vijay Abraham I, open list:ARM/Rockchip SoC..., Vinod Koul, Linux ARM 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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] phy: rockchip-emmc: emmc_phy_init() always return 0 @ 2020-12-04 3:01 ` Chris Ruehl 0 siblings, 0 replies; 18+ messages in thread From: Chris Ruehl @ 2020-12-04 3:01 UTC (permalink / raw) To: Doug Anderson Cc: Ulf Hansson, Heiko Stuebner, LKML, Kishon Vijay Abraham I, open list:ARM/Rockchip SoC..., Vinod Koul, Linux ARM 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 _______________________________________________ Linux-rockchip mailing list Linux-rockchip@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-rockchip ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] phy: rockchip-emmc: emmc_phy_init() always return 0 2020-12-04 3:01 ` Chris Ruehl (?) @ 2020-12-04 17:53 ` Doug Anderson -1 siblings, 0 replies; 18+ messages in thread From: Doug Anderson @ 2020-12-04 17:53 UTC (permalink / raw) To: Chris Ruehl Cc: Ulf Hansson, Heiko Stuebner, LKML, Kishon Vijay Abraham I, open list:ARM/Rockchip SoC..., Vinod Koul, Linux ARM Hi, On Thu, Dec 3, 2020 at 7:01 PM Chris Ruehl <chris.ruehl@gtsys.com.hk> wrote: > > 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. If you want to change this I'd guess you'd want to change the bindings in a backward-incompatible way. The bindings (Documentation/devicetree/bindings/phy/rockchip-emmc-phy.txt) say: - clock-names: Should contain "emmcclk". Although this is listed as optional (because most boards can get basic functionality without having access to it), it is strongly suggested. Since it's officially listed as optional then the driver needs to function without it. It was a long time ago, but my guess about why I needed to list it as optional was for the whole "device tree purity" reasons. In other words the goal is always that old device trees should continue to work, so if you're adding new properties they need to be optional. It seems highly unlikely anyone actually has a device tree that doesn't specify this clock, though. If you want to cleanly go through and mark this as required everywhere I won't personally object. -Doug ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] phy: rockchip-emmc: emmc_phy_init() always return 0 @ 2020-12-04 17:53 ` Doug Anderson 0 siblings, 0 replies; 18+ messages in thread From: Doug Anderson @ 2020-12-04 17:53 UTC (permalink / raw) To: Chris Ruehl Cc: Ulf Hansson, Heiko Stuebner, LKML, Kishon Vijay Abraham I, open list:ARM/Rockchip SoC..., Vinod Koul, Linux ARM Hi, On Thu, Dec 3, 2020 at 7:01 PM Chris Ruehl <chris.ruehl@gtsys.com.hk> wrote: > > 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. If you want to change this I'd guess you'd want to change the bindings in a backward-incompatible way. The bindings (Documentation/devicetree/bindings/phy/rockchip-emmc-phy.txt) say: - clock-names: Should contain "emmcclk". Although this is listed as optional (because most boards can get basic functionality without having access to it), it is strongly suggested. Since it's officially listed as optional then the driver needs to function without it. It was a long time ago, but my guess about why I needed to list it as optional was for the whole "device tree purity" reasons. In other words the goal is always that old device trees should continue to work, so if you're adding new properties they need to be optional. It seems highly unlikely anyone actually has a device tree that doesn't specify this clock, though. If you want to cleanly go through and mark this as required everywhere I won't personally object. -Doug _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] phy: rockchip-emmc: emmc_phy_init() always return 0 @ 2020-12-04 17:53 ` Doug Anderson 0 siblings, 0 replies; 18+ messages in thread From: Doug Anderson @ 2020-12-04 17:53 UTC (permalink / raw) To: Chris Ruehl Cc: Ulf Hansson, Heiko Stuebner, LKML, Kishon Vijay Abraham I, open list:ARM/Rockchip SoC..., Vinod Koul, Linux ARM Hi, On Thu, Dec 3, 2020 at 7:01 PM Chris Ruehl <chris.ruehl@gtsys.com.hk> wrote: > > 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. If you want to change this I'd guess you'd want to change the bindings in a backward-incompatible way. The bindings (Documentation/devicetree/bindings/phy/rockchip-emmc-phy.txt) say: - clock-names: Should contain "emmcclk". Although this is listed as optional (because most boards can get basic functionality without having access to it), it is strongly suggested. Since it's officially listed as optional then the driver needs to function without it. It was a long time ago, but my guess about why I needed to list it as optional was for the whole "device tree purity" reasons. In other words the goal is always that old device trees should continue to work, so if you're adding new properties they need to be optional. It seems highly unlikely anyone actually has a device tree that doesn't specify this clock, though. If you want to cleanly go through and mark this as required everywhere I won't personally object. -Doug _______________________________________________ Linux-rockchip mailing list Linux-rockchip@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-rockchip ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] phy: rockchip-emmc: emmc_phy_init() always return 0 2020-12-04 17:53 ` Doug Anderson (?) (?) @ 2020-12-07 4:12 ` Chris Ruehl 2020-12-07 16:29 ` Doug Anderson -1 siblings, 1 reply; 18+ messages in thread From: Chris Ruehl @ 2020-12-07 4:12 UTC (permalink / raw) To: linux-rockchip, Doug Anderson On 5/12/2020 1:53 am, Doug Anderson wrote: > Hi, > > On Thu, Dec 3, 2020 at 7:01 PM Chris Ruehl <chris.ruehl@gtsys.com.hk> wrote: >> >> 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. > > If you want to change this I'd guess you'd want to change the bindings > in a backward-incompatible way. The bindings > (Documentation/devicetree/bindings/phy/rockchip-emmc-phy.txt) say: > > - clock-names: Should contain "emmcclk". Although this is listed as optional > (because most boards can get basic functionality without having > access to it), it is strongly suggested. > > Since it's officially listed as optional then the driver needs to > function without it. It was a long time ago, but my guess about why I > needed to list it as optional was for the whole "device tree purity" > reasons. In other words the goal is always that old device trees > should continue to work, so if you're adding new properties they need > to be optional. > > It seems highly unlikely anyone actually has a device tree that > doesn't specify this clock, though. If you want to cleanly go through > and mark this as required everywhere I won't personally object. > > -Doug > Doug, Yes, seems unlikely to me too, the definition for emmcclk is in rk3399.dtsi and therefore in all the rk3399 boards I find in the dts/rockchip/. To be backwards compatible, and keep it optional, I can check if clock-names is defined and if not, set emmcclk=null and return 0. But if emmcclk is defined and an error returned from clk_get(), escalate it. like below. diff --git a/drivers/phy/rockchip/phy-rockchip-emmc.c b/drivers/phy/rockchip/phy-rockchip-emmc.c index 75faee5c0d27..47126f9af31d 100644 --- a/drivers/phy/rockchip/phy-rockchip-emmc.c +++ b/drivers/phy/rockchip/phy-rockchip-emmc.c @@ -85,6 +85,7 @@ struct rockchip_emmc_phy { struct clk *emmcclk; unsigned int drive_impedance; unsigned int enable_strobe_pulldown; + bool hasclockname; }; static int rockchip_emmc_phy_power(struct phy *phy, bool on_off) @@ -251,10 +252,14 @@ static int rockchip_emmc_phy_init(struct phy *phy) * above expected use case, EPROBE_DEFER isn't sensible to expect, so * it's just like any other error. */ - rk_phy->emmcclk = clk_get(&phy->dev, "emmcclk"); - if (IS_ERR(rk_phy->emmcclk)) { - ret = PTR_ERR(rk_phy->emmcclk); - dev_dbg(&phy->dev, "Error getting emmcclk: %d\n", ret); + if (rk_phy->hasclockname) { + rk_phy->emmcclk = clk_get(&phy->dev, "emmcclk"); + if (IS_ERR(rk_phy->emmcclk)) { + ret = PTR_ERR(rk_phy->emmcclk); + dev_err(&phy->dev, "Error getting emmcclk: %d\n", ret); + rk_phy->emmcclk = NULL; + } + } else { rk_phy->emmcclk = NULL; } @@ -373,6 +378,7 @@ static int rockchip_emmc_phy_probe(struct platform_device *pdev) rk_phy->reg_base = grf; rk_phy->drive_impedance = PHYCTRL_DR_50OHM; rk_phy->enable_strobe_pulldown = PHYCTRL_REN_STRB_DISABLE; + rk_phy->hasclockname = of_property_read_bool(dev->of_node, "clock-names"); Chris _______________________________________________ Linux-rockchip mailing list Linux-rockchip@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-rockchip ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH] phy: rockchip-emmc: emmc_phy_init() always return 0 2020-12-07 4:12 ` Chris Ruehl @ 2020-12-07 16:29 ` Doug Anderson 2020-12-09 1:29 ` Chris Ruehl 0 siblings, 1 reply; 18+ messages in thread From: Doug Anderson @ 2020-12-07 16:29 UTC (permalink / raw) To: Chris Ruehl; +Cc: open list:ARM/Rockchip SoC... Hi, On Sun, Dec 6, 2020 at 8:13 PM Chris Ruehl <chris.ruehl@gtsys.com.hk> wrote: > > To be backwards compatible, and keep it optional, I can check if clock-names is > defined and if not, set emmcclk=null and return 0. > But if emmcclk is defined and an error returned from clk_get(), escalate it. ...but how is this different than just checking for the -ENODEV (or maybe -ENOENT?) error return (other than being more complicated and requiring more code?) ...or, perhaps even better, just use clk_get_optional()? -Doug _______________________________________________ Linux-rockchip mailing list Linux-rockchip@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-rockchip ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] phy: rockchip-emmc: emmc_phy_init() always return 0 2020-12-07 16:29 ` Doug Anderson @ 2020-12-09 1:29 ` Chris Ruehl 0 siblings, 0 replies; 18+ messages in thread From: Chris Ruehl @ 2020-12-09 1:29 UTC (permalink / raw) To: Doug Anderson; +Cc: open list:ARM/Rockchip SoC... On 8/12/2020 12:29 am, Doug Anderson wrote: > Hi, > > On Sun, Dec 6, 2020 at 8:13 PM Chris Ruehl <chris.ruehl@gtsys.com.hk> wrote: >> >> To be backwards compatible, and keep it optional, I can check if clock-names is >> defined and if not, set emmcclk=null and return 0. >> But if emmcclk is defined and an error returned from clk_get(), escalate it. > > ...but how is this different than just checking for the -ENODEV (or > maybe -ENOENT?) error return (other than being more complicated and > requiring more code?) ...or, perhaps even better, just use > clk_get_optional()? > > -Doug > Hi, yeah! good hint. Let me rewind, and use the optional function call. Chris _______________________________________________ Linux-rockchip mailing list Linux-rockchip@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-rockchip ^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2020-12-09 1:29 UTC | newest] Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-12-01 3:10 [PATCH] phy: rockchip-emmc: emmc_phy_init() always return 0 Chris Ruehl 2020-12-01 3:10 ` Chris Ruehl 2020-12-01 3:10 ` Chris Ruehl 2020-12-01 16:05 ` Doug Anderson 2020-12-01 16:05 ` Doug Anderson 2020-12-01 16:05 ` Doug Anderson 2020-12-02 8:36 ` Chris Ruehl 2020-12-02 8:36 ` Chris Ruehl 2020-12-02 8:36 ` Chris Ruehl 2020-12-04 3:01 ` Chris Ruehl 2020-12-04 3:01 ` Chris Ruehl 2020-12-04 3:01 ` Chris Ruehl 2020-12-04 17:53 ` Doug Anderson 2020-12-04 17:53 ` Doug Anderson 2020-12-04 17:53 ` Doug Anderson 2020-12-07 4:12 ` Chris Ruehl 2020-12-07 16:29 ` Doug Anderson 2020-12-09 1:29 ` Chris Ruehl
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.