[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

[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

[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

[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.

[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

[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.

[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

[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 0/9] clk: Add non CONFIG_HAVE_CLK routines

[Viresh Kumar]

Many drivers are shared between architectures that may or may not have HAVE_CLK
selected for them. To remove compilation errors for them we enclose clk_*()
calls in these drivers within #ifdef CONFIG_HAVE_CLK, #endif.

This patchset removes the need of these CONFIG_HAVE_CLK statements, by
introducing dummy routines when HAVE_CLK is not selected by platforms. So,
definition of these routines will always be available. These calls will return
error for platforms that don't select HAVE_CLK.

V1->V2:
- Removed few patches as they might break working drivers
- Updated 1st patch, as it doesn't apply cleanly on latest linux-next after this
  got applied.

  commit a8a97db984bdc5e89d42e41891543d2daaf314cb
  Author: Mark Brown <broonie@sirena.org.uk>
  Date:   Thu Apr 5 11:42:09 2012 +0100

      ARM: 7376/1: clkdev: Implement managed clk_get()

- Similarly, updated stmmac patch as there were updates for it too.

Viresh Kumar (9):
  clk: Add non CONFIG_HAVE_CLK routines
  clk: Remove redundant depends on from drivers/Kconfig
  ata/pata_arasan: Remove conditional compilation of clk code
  ata/sata_mv: Remove conditional compilation of clk code
  net/c_can: Remove conditional compilation of clk code
  net/stmmac: Remove conditional compilation of clk code
  gadget/m66592: Remove conditional compilation of clk code
  gadget/r8a66597: Remove conditional compilation of clk code
  usb/host/r8a66597: Remove conditional compilation of clk code

 drivers/ata/pata_arasan_cf.c                      |   39 ++---
 drivers/ata/sata_mv.c                             |   10 --
 drivers/clk/Kconfig                               |    2 -
 drivers/net/can/c_can/c_can_platform.c            |   28 ++--
 drivers/net/ethernet/stmicro/stmmac/stmmac.h      |   41 -----
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c |   31 +++--
 drivers/usb/gadget/m66592-udc.c                   |   19 +--
 drivers/usb/gadget/m66592-udc.h                   |    5 -
 drivers/usb/gadget/r8a66597-udc.c                 |   21 +--
 drivers/usb/gadget/r8a66597-udc.h                 |    5 -
 drivers/usb/host/r8a66597-hcd.c                   |   28 +---
 drivers/usb/host/r8a66597.h                       |    5 -
 include/linux/clk.h                               |  168 +++++++++++++-------
 13 files changed, 176 insertions(+), 226 deletions(-)

-- 
1.7.9

[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 at 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