[PATCH] net: phy: micrel: support !CONFIG_HAVE_CLK

[PATCH] net: phy: micrel: support !CONFIG_HAVE_CLK

[Niklas Cassel]

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) {
 		bool rmii_ref_clk_sel_25_mhz;
 
 		priv->rmii_ref_clk_sel = type->has_rmii_ref_clk_sel;
-- 
2.1.4

Re: [PATCH] net: phy: micrel: support !CONFIG_HAVE_CLK

[Johan Hovold]

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

Re: [PATCH] net: phy: micrel: support !CONFIG_HAVE_CLK

[Johan Hovold]

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

Re: [PATCH] net: phy: micrel: support !CONFIG_HAVE_CLK

[David Miller]

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.

Re: [PATCH] net: phy: micrel: support !CONFIG_HAVE_CLK

[Fabio Estevam]

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?

Re: [PATCH] net: phy: micrel: support !CONFIG_HAVE_CLK

[Niklas Cassel]

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.

Re: [PATCH] net: phy: micrel: support !CONFIG_HAVE_CLK

[Johan Hovold]

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