RE: [PATCH 06/13] OMAP: hwmod: add non-locking versions of enable and idle functions

2010-06-24 Thread Basak, Partha 


 -Original Message-
 From: linux-omap-ow...@vger.kernel.org [mailto:linux-omap-
 ow...@vger.kernel.org] On Behalf Of Paul Walmsley
 Sent: Thursday, June 24, 2010 10:39 AM
 To: Kevin Hilman
 Cc: linux-omap@vger.kernel.org
 Subject: Re: [PATCH 06/13] OMAP: hwmod: add non-locking versions of enable
 and idle functions
 
 Hi Kevin
 
 On Wed, 23 Jun 2010, Kevin Hilman wrote:
 
  Some hwmods may need to be idled/enabled in atomic context, so
  non-locking versions of these functions are required.
 
  Most users should not need these and usage of theses should be
  controlled to understand why access is being done in atomic context.
  For this reason, the non-locking functions are only exposed at the
  hwmod level and not at the omap-device level.
 
  The use-case that led to the need for the non-locking versions is
  hwmods that are enabled/idled from within the core idle/suspend path.
  Since interrupts are already disabled here, the mutex-based locking in
  hwmod can sleep and will cause potential deadlocks.
 
 I accept the use-case.  But maybe it would be preferable to rename
 _enable(), _idle(), _shutdown() to _omap_hwmod_{enable,idle,shutdown}() ?
 That would avoid the need to add new functions that just call the existing
 ones.
 
 
 - Paul
 
 
  Cc: Paul Walmsley p...@pwsan.com
  Signed-off-by: Kevin Hilman khil...@deeprootsystems.com
  ---
   arch/arm/mach-omap2/omap_hwmod.c |   32
 ++---
   arch/arm/plat-omap/include/plat/omap_hwmod.h |2 +
   2 files changed, 30 insertions(+), 4 deletions(-)
 
  diff --git a/arch/arm/mach-omap2/omap_hwmod.c b/arch/arm/mach-
 omap2/omap_hwmod.c
  index 3d11523..8b2b44a 100644
  --- a/arch/arm/mach-omap2/omap_hwmod.c
  +++ b/arch/arm/mach-omap2/omap_hwmod.c
  @@ -1287,6 +1287,18 @@ int omap_hwmod_unregister(struct omap_hwmod *oh)
   }
 
   /**
  + * __omap_hwmod_enable - enable an omap_hwmod (non-locking version)
  + * @oh: struct omap_hwmod *
  + *
  + * Enable an omap_hwomd @oh.  Intended to be called in rare cases
  + * where usage is required in atomic context.
  + */
  +int __omap_hwmod_enable(struct omap_hwmod *oh)
  +{
  +   return _enable(oh);
  +}
  +
  +/**
* omap_hwmod_enable - enable an omap_hwmod
* @oh: struct omap_hwmod *
*
  @@ -1301,12 +1313,26 @@ int omap_hwmod_enable(struct omap_hwmod *oh)
  return -EINVAL;
 
  mutex_lock(omap_hwmod_mutex);
  -   r = _enable(oh);
  +   r = __omap_hwmod_enable(oh);
  mutex_unlock(omap_hwmod_mutex);
 
  return r;
   }
 
  +
  +/**
  + * __omap_hwmod_idle - idle an omap_hwmod (non-locking)
  + * @oh: struct omap_hwmod *
  + *
  + * Idle an omap_hwomd @oh.  Intended to be called in rare instances
 where
  + * used in atomic context.
  + */
  +int __omap_hwmod_idle(struct omap_hwmod *oh)
  +{
  +   _idle(oh);
  +   return 0;
  +}
  +
   /**
* omap_hwmod_idle - idle an omap_hwmod
* @oh: struct omap_hwmod *
  @@ -1319,9 +1345,7 @@ int omap_hwmod_idle(struct omap_hwmod *oh)
  if (!oh)
  return -EINVAL;
 
  -   mutex_lock(omap_hwmod_mutex);
  -   _idle(oh);
  -   mutex_unlock(omap_hwmod_mutex);
  +   __omap_hwmod_idle(oh);
 
BTW: The mutex locks are missing. Typo?

  return 0;
   }
  diff --git a/arch/arm/plat-omap/include/plat/omap_hwmod.h
 b/arch/arm/plat-omap/include/plat/omap_hwmod.h
  index 0eccc09..9a3f4dc 100644
  --- a/arch/arm/plat-omap/include/plat/omap_hwmod.h
  +++ b/arch/arm/plat-omap/include/plat/omap_hwmod.h
  @@ -486,7 +486,9 @@ int omap_hwmod_for_each(int (*fn)(struct omap_hwmod
 *oh));
   int omap_hwmod_late_init(void);
 
   int omap_hwmod_enable(struct omap_hwmod *oh);
  +int __omap_hwmod_enable(struct omap_hwmod *oh);
   int omap_hwmod_idle(struct omap_hwmod *oh);
  +int __omap_hwmod_idle(struct omap_hwmod *oh);
   int omap_hwmod_shutdown(struct omap_hwmod *oh);
 
   int omap_hwmod_enable_clocks(struct omap_hwmod *oh);
  --
  1.7.0.2
 
 
 
 - Paul
 --
 To unsubscribe from this list: send the line unsubscribe linux-omap in
 the body of a message to majord...@vger.kernel.org
 More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 06/13] OMAP: hwmod: add non-locking versions of enable and idle functions

2010-06-24 Thread Kevin Hilman 
Paul Walmsley p...@pwsan.com writes:

 Hi Kevin

 On Wed, 23 Jun 2010, Kevin Hilman wrote:

 Some hwmods may need to be idled/enabled in atomic context, so
 non-locking versions of these functions are required.
 
 Most users should not need these and usage of theses should be
 controlled to understand why access is being done in atomic context.
 For this reason, the non-locking functions are only exposed at the
 hwmod level and not at the omap-device level.
 
 The use-case that led to the need for the non-locking versions is
 hwmods that are enabled/idled from within the core idle/suspend path.
 Since interrupts are already disabled here, the mutex-based locking in
 hwmod can sleep and will cause potential deadlocks.

 I accept the use-case.  But maybe it would be preferable to rename 
 _enable(), _idle(), _shutdown() to _omap_hwmod_{enable,idle,shutdown}() ?
 That would avoid the need to add new functions that just call the existing 
 ones.

OK, will make that change.

Kevin
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 06/13] OMAP: hwmod: add non-locking versions of enable and idle functions

2010-06-24 Thread Kevin Hilman 
Basak, Partha p-bas...@ti.com writes:

[...]

   /**
* omap_hwmod_idle - idle an omap_hwmod
* @oh: struct omap_hwmod *
  @@ -1319,9 +1345,7 @@ int omap_hwmod_idle(struct omap_hwmod *oh)
 if (!oh)
 return -EINVAL;
 
  -  mutex_lock(omap_hwmod_mutex);
  -  _idle(oh);
  -  mutex_unlock(omap_hwmod_mutex);
  +  __omap_hwmod_idle(oh);
 

 BTW: The mutex locks are missing. Typo?


Good catch Partha.  

I wish I could explain that as only a typo.  Unfortunately, it was just
a not paying close enough attention mistake.

Thanks,

Kevin
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 06/13] OMAP: hwmod: add non-locking versions of enable and idle functions

2010-06-23 Thread Paul Walmsley 
Hi Kevin

On Wed, 23 Jun 2010, Kevin Hilman wrote:

 Some hwmods may need to be idled/enabled in atomic context, so
 non-locking versions of these functions are required.
 
 Most users should not need these and usage of theses should be
 controlled to understand why access is being done in atomic context.
 For this reason, the non-locking functions are only exposed at the
 hwmod level and not at the omap-device level.
 
 The use-case that led to the need for the non-locking versions is
 hwmods that are enabled/idled from within the core idle/suspend path.
 Since interrupts are already disabled here, the mutex-based locking in
 hwmod can sleep and will cause potential deadlocks.

I accept the use-case.  But maybe it would be preferable to rename 
_enable(), _idle(), _shutdown() to _omap_hwmod_{enable,idle,shutdown}() ?
That would avoid the need to add new functions that just call the existing 
ones.


- Paul

 
 Cc: Paul Walmsley p...@pwsan.com
 Signed-off-by: Kevin Hilman khil...@deeprootsystems.com
 ---
  arch/arm/mach-omap2/omap_hwmod.c |   32 ++---
  arch/arm/plat-omap/include/plat/omap_hwmod.h |2 +
  2 files changed, 30 insertions(+), 4 deletions(-)
 
 diff --git a/arch/arm/mach-omap2/omap_hwmod.c 
 b/arch/arm/mach-omap2/omap_hwmod.c
 index 3d11523..8b2b44a 100644
 --- a/arch/arm/mach-omap2/omap_hwmod.c
 +++ b/arch/arm/mach-omap2/omap_hwmod.c
 @@ -1287,6 +1287,18 @@ int omap_hwmod_unregister(struct omap_hwmod *oh)
  }
  
  /**
 + * __omap_hwmod_enable - enable an omap_hwmod (non-locking version)
 + * @oh: struct omap_hwmod *
 + *
 + * Enable an omap_hwomd @oh.  Intended to be called in rare cases
 + * where usage is required in atomic context.
 + */
 +int __omap_hwmod_enable(struct omap_hwmod *oh)
 +{
 + return _enable(oh);
 +}
 +
 +/**
   * omap_hwmod_enable - enable an omap_hwmod
   * @oh: struct omap_hwmod *
   *
 @@ -1301,12 +1313,26 @@ int omap_hwmod_enable(struct omap_hwmod *oh)
   return -EINVAL;
  
   mutex_lock(omap_hwmod_mutex);
 - r = _enable(oh);
 + r = __omap_hwmod_enable(oh);
   mutex_unlock(omap_hwmod_mutex);
  
   return r;
  }
  
 +
 +/**
 + * __omap_hwmod_idle - idle an omap_hwmod (non-locking)
 + * @oh: struct omap_hwmod *
 + *
 + * Idle an omap_hwomd @oh.  Intended to be called in rare instances where
 + * used in atomic context.
 + */
 +int __omap_hwmod_idle(struct omap_hwmod *oh)
 +{
 + _idle(oh);
 + return 0;
 +}
 +
  /**
   * omap_hwmod_idle - idle an omap_hwmod
   * @oh: struct omap_hwmod *
 @@ -1319,9 +1345,7 @@ int omap_hwmod_idle(struct omap_hwmod *oh)
   if (!oh)
   return -EINVAL;
  
 - mutex_lock(omap_hwmod_mutex);
 - _idle(oh);
 - mutex_unlock(omap_hwmod_mutex);
 + __omap_hwmod_idle(oh);
  
   return 0;
  }
 diff --git a/arch/arm/plat-omap/include/plat/omap_hwmod.h 
 b/arch/arm/plat-omap/include/plat/omap_hwmod.h
 index 0eccc09..9a3f4dc 100644
 --- a/arch/arm/plat-omap/include/plat/omap_hwmod.h
 +++ b/arch/arm/plat-omap/include/plat/omap_hwmod.h
 @@ -486,7 +486,9 @@ int omap_hwmod_for_each(int (*fn)(struct omap_hwmod *oh));
  int omap_hwmod_late_init(void);
  
  int omap_hwmod_enable(struct omap_hwmod *oh);
 +int __omap_hwmod_enable(struct omap_hwmod *oh);
  int omap_hwmod_idle(struct omap_hwmod *oh);
 +int __omap_hwmod_idle(struct omap_hwmod *oh);
  int omap_hwmod_shutdown(struct omap_hwmod *oh);
  
  int omap_hwmod_enable_clocks(struct omap_hwmod *oh);
 -- 
 1.7.0.2
 


- Paul
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html