| Message ID | 1430132450-4496-1-git-send-email-niklass@axis.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
On Mon, Apr 27, 2015 at 01:00:50PM +0200, Niklas Cassel wrote: > Since NULL is a valid clock, we shouldn't use > IS_ERR_OR_NULL. > > Implemented as Russell King suggested in: > > http://lkml.kernel.org/r/20150207172949.GE8656@n2100.arm.linux.org.uk > > Signed-off-by: Niklas Cassel <niklass@axis.com> > --- > drivers/net/phy/micrel.c | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c > index 1190fd8..d958d13 100644 > --- a/drivers/net/phy/micrel.c > +++ b/drivers/net/phy/micrel.c > @@ -522,6 +522,7 @@ static int kszphy_probe(struct phy_device *phydev) > struct device_node *np = phydev->dev.of_node; > struct kszphy_priv *priv; > struct clk *clk; > + unsigned long rate = 0; > int ret; > > priv = devm_kzalloc(&phydev->dev, sizeof(*priv), GFP_KERNEL); > @@ -548,8 +549,10 @@ static int kszphy_probe(struct phy_device *phydev) > } > > clk = devm_clk_get(&phydev->dev, "rmii-ref"); > - if (!IS_ERR(clk)) { > - unsigned long rate = clk_get_rate(clk); > + if (!IS_ERR(clk)) > + rate = clk_get_rate(clk); > + > + if (rate) { And how do you expect this to work as clk_get_rate returns -EINVAL when clk is NULL? Have you not tested your patch? > bool rmii_ref_clk_sel_25_mhz; > > priv->rmii_ref_clk_sel = type->has_rmii_ref_clk_sel; Johan -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Apr 27, 2015 at 01:05:17PM +0200, Johan Hovold wrote: > On Mon, Apr 27, 2015 at 01:00:50PM +0200, Niklas Cassel wrote: > > Since NULL is a valid clock, we shouldn't use > > IS_ERR_OR_NULL. > > > > Implemented as Russell King suggested in: > > > > http://lkml.kernel.org/r/20150207172949.GE8656@n2100.arm.linux.org.uk > > > > Signed-off-by: Niklas Cassel <niklass@axis.com> > > --- > > drivers/net/phy/micrel.c | 7 +++++-- > > 1 file changed, 5 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c > > index 1190fd8..d958d13 100644 > > --- a/drivers/net/phy/micrel.c > > +++ b/drivers/net/phy/micrel.c > > @@ -522,6 +522,7 @@ static int kszphy_probe(struct phy_device *phydev) > > struct device_node *np = phydev->dev.of_node; > > struct kszphy_priv *priv; > > struct clk *clk; > > + unsigned long rate = 0; > > int ret; > > > > priv = devm_kzalloc(&phydev->dev, sizeof(*priv), GFP_KERNEL); > > @@ -548,8 +549,10 @@ static int kszphy_probe(struct phy_device *phydev) > > } > > > > clk = devm_clk_get(&phydev->dev, "rmii-ref"); > > - if (!IS_ERR(clk)) { > > - unsigned long rate = clk_get_rate(clk); > > + if (!IS_ERR(clk)) > > + rate = clk_get_rate(clk); > > + > > + if (rate) { > > And how do you expect this to work as clk_get_rate returns -EINVAL when > clk is NULL? > > Have you not tested your patch? Nevermind, it is 0 if !CONFIG_HAVE_CLK, but still does not look like the right fix. Johan -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Johan Hovold <johan@kernel.org> Date: Mon, 27 Apr 2015 13:08:07 +0200 > On Mon, Apr 27, 2015 at 01:05:17PM +0200, Johan Hovold wrote: >> On Mon, Apr 27, 2015 at 01:00:50PM +0200, Niklas Cassel wrote: >> > Since NULL is a valid clock, we shouldn't use >> > IS_ERR_OR_NULL. >> > >> > Implemented as Russell King suggested in: >> > >> > http://lkml.kernel.org/r/20150207172949.GE8656@n2100.arm.linux.org.uk >> > >> > Signed-off-by: Niklas Cassel <niklass@axis.com> >> > --- >> > drivers/net/phy/micrel.c | 7 +++++-- >> > 1 file changed, 5 insertions(+), 2 deletions(-) >> > >> > diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c >> > index 1190fd8..d958d13 100644 >> > --- a/drivers/net/phy/micrel.c >> > +++ b/drivers/net/phy/micrel.c >> > @@ -522,6 +522,7 @@ static int kszphy_probe(struct phy_device *phydev) >> > struct device_node *np = phydev->dev.of_node; >> > struct kszphy_priv *priv; >> > struct clk *clk; >> > + unsigned long rate = 0; >> > int ret; >> > >> > priv = devm_kzalloc(&phydev->dev, sizeof(*priv), GFP_KERNEL); >> > @@ -548,8 +549,10 @@ static int kszphy_probe(struct phy_device *phydev) >> > } >> > >> > clk = devm_clk_get(&phydev->dev, "rmii-ref"); >> > - if (!IS_ERR(clk)) { >> > - unsigned long rate = clk_get_rate(clk); >> > + if (!IS_ERR(clk)) >> > + rate = clk_get_rate(clk); >> > + >> > + if (rate) { >> >> And how do you expect this to work as clk_get_rate returns -EINVAL when >> clk is NULL? >> >> Have you not tested your patch? > > Nevermind, it is 0 if !CONFIG_HAVE_CLK, but still does not look like the > right fix. Agreed, this doesn't look right to me either. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Apr 27, 2015 at 8:00 AM, Niklas Cassel <niklas.cassel@axis.com> wrote: > Since NULL is a valid clock, we shouldn't use > IS_ERR_OR_NULL. Yes, but this code is not using IS_ERR_OR_NULL. It seems that you are not describing the problem you are trying to solve. What is the exact issue you are seeing? > clk = devm_clk_get(&phydev->dev, "rmii-ref"); You need to provide the 'rmii-ref' in your board file or dts. Are you doing this? -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 04/27/2015 07:59 PM, Fabio Estevam wrote: > On Mon, Apr 27, 2015 at 8:00 AM, Niklas Cassel <niklas.cassel@axis.com> wrote: > >> Since NULL is a valid clock, we shouldn't use >> IS_ERR_OR_NULL. > > Yes, but this code is not using IS_ERR_OR_NULL. > > It seems that you are not describing the problem you are trying to solve. > > What is the exact issue you are seeing? > >> clk = devm_clk_get(&phydev->dev, "rmii-ref"); > > You need to provide the 'rmii-ref' in your board file or dts. > > Are you doing this? > I'm sorry, my commit message didn't really describe the problem properly. When you compile your kernel without the clk.h API (CONFIG_HAVE_CLK), devm_clk_get returns NULL (which also happens to be a valid clock in the clk.h API). So it's not a matter of DT or board-files. This is on a mips platform without DT support. I simply want the drivers/net/phy/micrel.c driver to also work without CONFIG_HAVE_CLK, like it did before commit 63f44b2bfccdd98193bbd602747f780c0fae0f02 net: phy: micrel: add generic clock-mode-select support Now I get an error every time I boot with "Clock rate out of range: 0", since clk_get_rate returns 0 when compiling with !CONFIG_HAVE_CLK. It is confusing that devm_clk_get returns a valid clock in the clk.h API (NULL), when compiling with !CONFIG_HAVE_CLK, but according to Russell King in: http://lkml.kernel.org/r/20150207172949.GE8656@n2100.arm.linux.org.uk this was intentional. (explained in the lkml mail above.) In the same mail Russell King also suggests how a driver can support both !CONFIG_HAVE_CLK and CONFIG_HAVE_CLK. This is the suggestion I followed when writing my patch. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, May 04, 2015 at 01:30:58PM +0200, Niklas Cassel wrote: > On 04/27/2015 07:59 PM, Fabio Estevam wrote: > > On Mon, Apr 27, 2015 at 8:00 AM, Niklas Cassel <niklas.cassel@axis.com> wrote: > > > >> Since NULL is a valid clock, we shouldn't use > >> IS_ERR_OR_NULL. > > > > Yes, but this code is not using IS_ERR_OR_NULL. > > > > It seems that you are not describing the problem you are trying to solve. > > > > What is the exact issue you are seeing? > > > >> clk = devm_clk_get(&phydev->dev, "rmii-ref"); > > > > You need to provide the 'rmii-ref' in your board file or dts. > > > > Are you doing this? > > > > I'm sorry, my commit message didn't really describe the problem properly. > > When you compile your kernel without the clk.h API (CONFIG_HAVE_CLK), > devm_clk_get returns NULL (which also happens to be a valid clock in the clk.h API). > So it's not a matter of DT or board-files. This is on a mips platform without DT support. > > I simply want the drivers/net/phy/micrel.c driver to also work without CONFIG_HAVE_CLK, > like it did before commit > > 63f44b2bfccdd98193bbd602747f780c0fae0f02 > net: phy: micrel: add generic clock-mode-select support > > Now I get an error every time I boot with "Clock rate out of range: 0", > since clk_get_rate returns 0 when compiling with !CONFIG_HAVE_CLK. You should include this information in the commit message when you send the next version of the patch (and remember to include the patch revision in the subject, that is, PATCH v3). As this is a regression that prevents the device from being probed (mention this in the commit message as well) we should address this promptly. Add a Fixes and CC-stable tag as well as the offending commit went into 3.18. > It is confusing that devm_clk_get returns a valid clock in the clk.h API (NULL), > when compiling with !CONFIG_HAVE_CLK, but according to Russell King in: > > http://lkml.kernel.org/r/20150207172949.GE8656@n2100.arm.linux.org.uk > > this was intentional. (explained in the lkml mail above.) > > In the same mail Russell King also suggests how a driver can support both > !CONFIG_HAVE_CLK and CONFIG_HAVE_CLK. > > This is the suggestion I followed when writing my patch. I'm actually leaning towards using !IS_ERR_OR_NULL as you did in your first patch and to add a comment explaining why it's there (rather than skipping the ref_clk configuration if the clock rate is 0). The rmii-ref clock is already optional and is only used to set one configuration bit when present. We should of course continue to support board-file configuration as well. Thanks, Johan -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c index 1190fd8..d958d13 100644 --- a/drivers/net/phy/micrel.c +++ b/drivers/net/phy/micrel.c @@ -522,6 +522,7 @@ static int kszphy_probe(struct phy_device *phydev) struct device_node *np = phydev->dev.of_node; struct kszphy_priv *priv; struct clk *clk; + unsigned long rate = 0; int ret; priv = devm_kzalloc(&phydev->dev, sizeof(*priv), GFP_KERNEL); @@ -548,8 +549,10 @@ static int kszphy_probe(struct phy_device *phydev) } clk = devm_clk_get(&phydev->dev, "rmii-ref"); - if (!IS_ERR(clk)) { - unsigned long rate = clk_get_rate(clk); + if (!IS_ERR(clk)) + rate = clk_get_rate(clk); + + if (rate) { bool rmii_ref_clk_sel_25_mhz; priv->rmii_ref_clk_sel = type->has_rmii_ref_clk_sel;
Since NULL is a valid clock, we shouldn't use IS_ERR_OR_NULL. Implemented as Russell King suggested in: http://lkml.kernel.org/r/20150207172949.GE8656@n2100.arm.linux.org.uk Signed-off-by: Niklas Cassel <niklass@axis.com> --- drivers/net/phy/micrel.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)