[PATCH V2 4/9] ata/sata_mv: Remove conditional compilation of clk code

[PATCH V2 4/9] ata/sata_mv: Remove conditional compilation of clk code

[Andrew Lunn]

Hi Viresh

> With addition of dummy clk_*() calls for non CONFIG_HAVE_CLK cases in clk.h,
> there is no need to have clk code enclosed in #ifdef CONFIG_HAVE_CLK, #endif
> macros.
> 
> ...
> 
> -#if defined(CONFIG_HAVE_CLK)
>         hpriv->clk = clk_get(&pdev->dev, NULL);
>         if (IS_ERR(hpriv->clk))
>                 dev_notice(&pdev->dev, "cannot get clkdev\n");
>         else
>                 clk_enable(hpriv->clk);
> -#endif

I don't think this change is correct. With the old semantics, it was:

If we have CLK support, we expect there to be a clock for sata_mv, and
if there is no such clock, output a notice message, something is
probably wrong, i expected there to be a clock.

The new semantics are:

We expect there to be a clock for sata_mv, and if there is no such
clock, output a notice message, something is probably wrong, i
expected there to be a clock.

We are going to see this notice message much more, when it is not
expected.

	Andrew

Re: [PATCH V2 4/9] ata/sata_mv: Remove conditional compilation of clk code

[Viresh Kumar]

On 4/24/2012 12:34 PM, Andrew Lunn wrote:
> I don't think this change is correct. With the old semantics, it was:

Sorry. :(

> If we have CLK support, we expect there to be a clock for sata_mv, and
> if there is no such clock, output a notice message, something is
> probably wrong, i expected there to be a clock.
> 
> The new semantics are:
> 
> We expect there to be a clock for sata_mv, and if there is no such
> clock, output a notice message, something is probably wrong, i
> expected there to be a clock.
> 
> We are going to see this notice message much more, when it is not
> expected.

So, the only problem is this message?
How do you suggest to tackle this now. Have #ifdef,#endif around this print?

-- 
viresh

Re: [PATCH V2 4/9] ata/sata_mv: Remove conditional compilation of clk code

[Andrew Lunn]

On Tue, Apr 24, 2012 at 12:35:23PM +0530, Viresh Kumar wrote:
> On 4/24/2012 12:34 PM, Andrew Lunn wrote:
> > I don't think this change is correct. With the old semantics, it was:
> 
> Sorry. :(
> 
> > If we have CLK support, we expect there to be a clock for sata_mv, and
> > if there is no such clock, output a notice message, something is
> > probably wrong, i expected there to be a clock.
> > 
> > The new semantics are:
> > 
> > We expect there to be a clock for sata_mv, and if there is no such
> > clock, output a notice message, something is probably wrong, i
> > expected there to be a clock.
> > 
> > We are going to see this notice message much more, when it is not
> > expected.
> 
> So, the only problem is this message?
> How do you suggest to tackle this now. Have #ifdef,#endif around this print?

Well, adding #ifdef defeats the point of adding dummy implementations.

Maybe, rather than return -ENODEV, return -ENOTSUP.

IS_ERR() still returns true, so in most cases, no code needs
changing. However, when you need to differentiate between, "clock does
not exists" and "Dummy clock functions being used", you can tell the
difference. mv_sata could look like:

        hpriv->clk = clk_get(&pdev->dev, NULL);
        if (IS_ERR(hpriv->clk))
	        if (PTR_ERR(hpriv->clk) == -ENODEV)
		        dev_notice(&pdev->dev, "cannot get clkdev\n");
        else
                clk_prepare_enable(hpriv->clk);


You would however, need to look at all uses of clk_get and see if any
are looked for ENODEV, and not just IS_ERR(), and fix those....

	Andrew

Re: [PATCH V2 4/9] ata/sata_mv: Remove conditional compilation of clk code

[Russell King - ARM Linux]

On Tue, Apr 24, 2012 at 09:26:53AM +0200, Andrew Lunn wrote:
> On Tue, Apr 24, 2012 at 12:35:23PM +0530, Viresh Kumar wrote:
> > On 4/24/2012 12:34 PM, Andrew Lunn wrote:
> > > I don't think this change is correct. With the old semantics, it was:
> > 
> > Sorry. :(
> > 
> > > If we have CLK support, we expect there to be a clock for sata_mv, and
> > > if there is no such clock, output a notice message, something is
> > > probably wrong, i expected there to be a clock.
> > > 
> > > The new semantics are:
> > > 
> > > We expect there to be a clock for sata_mv, and if there is no such
> > > clock, output a notice message, something is probably wrong, i
> > > expected there to be a clock.
> > > 
> > > We are going to see this notice message much more, when it is not
> > > expected.
> > 
> > So, the only problem is this message?
> > How do you suggest to tackle this now. Have #ifdef,#endif around this print?
> 
> Well, adding #ifdef defeats the point of adding dummy implementations.
> 
> Maybe, rather than return -ENODEV, return -ENOTSUP.
> 
> IS_ERR() still returns true, so in most cases, no code needs
> changing. However, when you need to differentiate between, "clock does
> not exists" and "Dummy clock functions being used", you can tell the
> difference. mv_sata could look like:
> 
>         hpriv->clk = clk_get(&pdev->dev, NULL);
>         if (IS_ERR(hpriv->clk))
> 	        if (PTR_ERR(hpriv->clk) == -ENODEV)
> 		        dev_notice(&pdev->dev, "cannot get clkdev\n");
>         else
>                 clk_prepare_enable(hpriv->clk);
> 
> 
> You would however, need to look at all uses of clk_get and see if any
> are looked for ENODEV, and not just IS_ERR(), and fix those....

Why bother?

If you don't have the clk API configured, you have no clocks to control.
So, why not make clk_get() return NULL, and make the rest of the API
calls do nothing?  That's what you'll end up codifying in the drivers
anyway.

Re: [PATCH V2 4/9] ata/sata_mv: Remove conditional compilation of clk code

[Viresh Kumar]

On 4/24/2012 1:12 PM, Russell King - ARM Linux wrote:
> If you don't have the clk API configured, you have no clocks to control.
> So, why not make clk_get() return NULL, and make the rest of the API
> calls do nothing?  That's what you'll end up codifying in the drivers
> anyway.

Ok. We can return NULL from calls that return clk *. What about other
routines that return integers. Like, clk_enable().

Is returning 0 correct? Which would mean we were able to enable clk, but
actually we haven't.

-- 
viresh

Re: [PATCH V2 4/9] ata/sata_mv: Remove conditional compilation of clk code

[Russell King - ARM Linux]

On Tue, Apr 24, 2012 at 01:28:20PM +0530, Viresh Kumar wrote:
> On 4/24/2012 1:12 PM, Russell King - ARM Linux wrote:
> > If you don't have the clk API configured, you have no clocks to control.
> > So, why not make clk_get() return NULL, and make the rest of the API
> > calls do nothing?  That's what you'll end up codifying in the drivers
> > anyway.
> 
> Ok. We can return NULL from calls that return clk *. What about other
> routines that return integers. Like, clk_enable().
> 
> Is returning 0 correct? Which would mean we were able to enable clk, but
> actually we haven't.

Think about this case: if you don't have the means to control the clock
inputs to a device (for example, you don't support the clk API on your
CPU arch) then for the device to be functional, it must be supplied with
all its necessary clocks.  Therefore, the clock is already enabled.  It
makes sense for the clk API to stub-out to be completely transparent and
non-error inducing to the driver.

The problem comes with clk_get_rate().  I'd suggest merely returning zero
for that in this case.  If the clock rate is really required by a driver,
then the clk API would need to be enabled.

Re: [PATCH V2 4/9] ata/sata_mv: Remove conditional compilation of clk code

[Viresh Kumar]

On 4/24/2012 1:56 PM, Russell King - ARM Linux wrote:
> Think about this case: if you don't have the means to control the clock
> inputs to a device (for example, you don't support the clk API on your
> CPU arch) then for the device to be functional, it must be supplied with
> all its necessary clocks.  Therefore, the clock is already enabled.  It
> makes sense for the clk API to stub-out to be completely transparent and
> non-error inducing to the driver.
> 
> The problem comes with clk_get_rate().  I'd suggest merely returning zero
> for that in this case.  If the clock rate is really required by a driver,
> then the clk API would need to be enabled.

Ok. Will do as suggested. Will include the patches i dropped earlier, where
i removed macros for clk_*().

-- 
viresh

Re: [PATCH V2 4/9] ata/sata_mv: Remove conditional compilation of clk code

[Sergei Shtylyov]

Hello.

On 24-04-2012 11:05, Viresh Kumar wrote:

>> I don't think this change is correct. With the old semantics, it was:

> Sorry. :(

>> If we have CLK support, we expect there to be a clock for sata_mv, and
>> if there is no such clock, output a notice message, something is
>> probably wrong, i expected there to be a clock.

>> The new semantics are:

>> We expect there to be a clock for sata_mv, and if there is no such
>> clock, output a notice message, something is probably wrong, i
>> expected there to be a clock.

>> We are going to see this notice message much more, when it is not
>> expected.

> So, the only problem is this message?
> How do you suggest to tackle this now. Have #ifdef,#endif around this print?

    When there's no CONFIG_HAVE_CLK, if clk_get() returns NULL, not error, 
there'll be no message.

WBR, Sergei

[PATCH V2 4/9] ata/sata_mv: Remove conditional compilation of clk code

[Viresh Kumar]

With addition of dummy clk_*() calls for non CONFIG_HAVE_CLK cases in clk.h,
there is no need to have clk code enclosed in #ifdef CONFIG_HAVE_CLK, #endif
macros.

Signed-off-by: Viresh Kumar <viresh.kumar@st.com>
Cc: Jeff Garzik <jgarzik@redhat.com>
Cc: linux-ide@vger.kernel.org
---
 drivers/ata/sata_mv.c |   10 ----------
 1 files changed, 0 insertions(+), 10 deletions(-)

diff --git a/drivers/ata/sata_mv.c b/drivers/ata/sata_mv.c
index 7336d4a..37503b8 100644
--- a/drivers/ata/sata_mv.c
+++ b/drivers/ata/sata_mv.c
@@ -551,9 +551,7 @@ struct mv_host_priv {
 	u32			irq_mask_offset;
 	u32			unmask_all_irqs;
 
-#if defined(CONFIG_HAVE_CLK)
 	struct clk		*clk;
-#endif
 	/*
 	 * These consistent DMA memory pools give us guaranteed
 	 * alignment for hardware-accessed data structures,
@@ -4063,13 +4061,11 @@ static int mv_platform_probe(struct platform_device *pdev)
 				   resource_size(res));
 	hpriv->base -= SATAHC0_REG_BASE;
 
-#if defined(CONFIG_HAVE_CLK)
 	hpriv->clk = clk_get(&pdev->dev, NULL);
 	if (IS_ERR(hpriv->clk))
 		dev_notice(&pdev->dev, "cannot get clkdev\n");
 	else
 		clk_enable(hpriv->clk);
-#endif
 
 	/*
 	 * (Re-)program MBUS remapping windows if we are asked to.
@@ -4096,12 +4092,10 @@ static int mv_platform_probe(struct platform_device *pdev)
 		return 0;
 
 err:
-#if defined(CONFIG_HAVE_CLK)
 	if (!IS_ERR(hpriv->clk)) {
 		clk_disable(hpriv->clk);
 		clk_put(hpriv->clk);
 	}
-#endif
 
 	return rc;
 }
@@ -4117,17 +4111,13 @@ err:
 static int __devexit mv_platform_remove(struct platform_device *pdev)
 {
 	struct ata_host *host = platform_get_drvdata(pdev);
-#if defined(CONFIG_HAVE_CLK)
 	struct mv_host_priv *hpriv = host->private_data;
-#endif
 	ata_host_detach(host);
 
-#if defined(CONFIG_HAVE_CLK)
 	if (!IS_ERR(hpriv->clk)) {
 		clk_disable(hpriv->clk);
 		clk_put(hpriv->clk);
 	}
-#endif
 	return 0;
 }
 
-- 
1.7.9