[PATCH 0/3] OMAP2+ hwmod fixes

[PATCH 0/3] OMAP2+ hwmod fixes

[Rajendra Nayak]

This series fixes some hwmod api return values
and also adds some state checks.
The hwmod iterator functions are made to
continue and not break if one of the
callback functions ends up with an error.

Series applies on 2.6.38-rc4 and is boot
tested on OMAP3430SDP and OMAP4430SDP
platforms.

Rajendra Nayak (3):
  OMAP2+: hwmod: Avoid setup if clock lookup failed
  OMAP2+: hwmod: Fix what _init_clock returns
  OMAP2+: hwmod: Do not break iterator fn's if one fails

 arch/arm/mach-omap2/omap_hwmod.c |   31 +++++++++++++++----------------
 1 files changed, 15 insertions(+), 16 deletions(-)

[PATCH 0/3] OMAP2+ hwmod fixes

[Rajendra Nayak]

This series fixes some hwmod api return values
and also adds some state checks.
The hwmod iterator functions are made to
continue and not break if one of the
callback functions ends up with an error.

Series applies on 2.6.38-rc4 and is boot
tested on OMAP3430SDP and OMAP4430SDP
platforms.

Rajendra Nayak (3):
  OMAP2+: hwmod: Avoid setup if clock lookup failed
  OMAP2+: hwmod: Fix what _init_clock returns
  OMAP2+: hwmod: Do not break iterator fn's if one fails

 arch/arm/mach-omap2/omap_hwmod.c |   31 +++++++++++++++----------------
 1 files changed, 15 insertions(+), 16 deletions(-)

[PATCH 1/3] OMAP2+: hwmod: Avoid setup if clock lookup failed

[Rajendra Nayak]

Add a hwmod state check in the _setup function
to avoid setting up hwmods' for which clock
lookup has failed.

Signed-off-by: Rajendra Nayak <rnayak@ti.com>
---
 arch/arm/mach-omap2/omap_hwmod.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/arch/arm/mach-omap2/omap_hwmod.c b/arch/arm/mach-omap2/omap_hwmod.c
index e282e35..cd9dcde 100644
--- a/arch/arm/mach-omap2/omap_hwmod.c
+++ b/arch/arm/mach-omap2/omap_hwmod.c
@@ -1362,6 +1362,12 @@ static int _setup(struct omap_hwmod *oh, void *data)
 	int i, r;
 	u8 postsetup_state;
 
+	if (oh->_state != _HWMOD_STATE_CLKS_INITED) {
+		WARN(1, "omap_hwmod: %s: _setup failed as one or more"
+		     "clock lookups' have failed\n", oh->name);
+		return -EINVAL;
+	}
+
 	/* Set iclk autoidle mode */
 	if (oh->slaves_cnt > 0) {
 		for (i = 0; i < oh->slaves_cnt; i++) {
-- 
1.7.0.4

[PATCH 1/3] OMAP2+: hwmod: Avoid setup if clock lookup failed

[Rajendra Nayak]

Add a hwmod state check in the _setup function
to avoid setting up hwmods' for which clock
lookup has failed.

Signed-off-by: Rajendra Nayak <rnayak@ti.com>
---
 arch/arm/mach-omap2/omap_hwmod.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/arch/arm/mach-omap2/omap_hwmod.c b/arch/arm/mach-omap2/omap_hwmod.c
index e282e35..cd9dcde 100644
--- a/arch/arm/mach-omap2/omap_hwmod.c
+++ b/arch/arm/mach-omap2/omap_hwmod.c
@@ -1362,6 +1362,12 @@ static int _setup(struct omap_hwmod *oh, void *data)
 	int i, r;
 	u8 postsetup_state;
 
+	if (oh->_state != _HWMOD_STATE_CLKS_INITED) {
+		WARN(1, "omap_hwmod: %s: _setup failed as one or more"
+		     "clock lookups' have failed\n", oh->name);
+		return -EINVAL;
+	}
+
 	/* Set iclk autoidle mode */
 	if (oh->slaves_cnt > 0) {
 		for (i = 0; i < oh->slaves_cnt; i++) {
-- 
1.7.0.4

[PATCH 2/3] OMAP2+: hwmod: Fix what _init_clock returns

[Rajendra Nayak]

_init_clock always returns 0 and does
not propogate the error (in case of failure)
back to the caller, causing _init_clocks to
fail silently.

Signed-off-by: Rajendra Nayak <rnayak@ti.com>
---
 arch/arm/mach-omap2/omap_hwmod.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/arch/arm/mach-omap2/omap_hwmod.c b/arch/arm/mach-omap2/omap_hwmod.c
index cd9dcde..960461f 100644
--- a/arch/arm/mach-omap2/omap_hwmod.c
+++ b/arch/arm/mach-omap2/omap_hwmod.c
@@ -926,7 +926,7 @@ static int _init_clocks(struct omap_hwmod *oh, void *data)
 	if (!ret)
 		oh->_state = _HWMOD_STATE_CLKS_INITED;
 
-	return 0;
+	return ret;
 }
 
 /**
-- 
1.7.0.4

[PATCH 2/3] OMAP2+: hwmod: Fix what _init_clock returns

[Rajendra Nayak]

_init_clock always returns 0 and does
not propogate the error (in case of failure)
back to the caller, causing _init_clocks to
fail silently.

Signed-off-by: Rajendra Nayak <rnayak@ti.com>
---
 arch/arm/mach-omap2/omap_hwmod.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/arch/arm/mach-omap2/omap_hwmod.c b/arch/arm/mach-omap2/omap_hwmod.c
index cd9dcde..960461f 100644
--- a/arch/arm/mach-omap2/omap_hwmod.c
+++ b/arch/arm/mach-omap2/omap_hwmod.c
@@ -926,7 +926,7 @@ static int _init_clocks(struct omap_hwmod *oh, void *data)
 	if (!ret)
 		oh->_state = _HWMOD_STATE_CLKS_INITED;
 
-	return 0;
+	return ret;
 }
 
 /**
-- 
1.7.0.4

[PATCH 3/3] OMAP2+: hwmod: Do not break iterator fn's if one fails

[Rajendra Nayak]

The iterator functions used to iterate over all
registered hwmods and all hwmods of a given class
break if one of the iterator functions fail.
Instead iterate over all the functions and return
an ORed return value back to the user.

Signed-off-by: Rajendra Nayak <rnayak@ti.com>
---
 arch/arm/mach-omap2/omap_hwmod.c |   23 ++++++++---------------
 1 files changed, 8 insertions(+), 15 deletions(-)

diff --git a/arch/arm/mach-omap2/omap_hwmod.c b/arch/arm/mach-omap2/omap_hwmod.c
index 960461f..3cab82e 100644
--- a/arch/arm/mach-omap2/omap_hwmod.c
+++ b/arch/arm/mach-omap2/omap_hwmod.c
@@ -1568,25 +1568,21 @@ struct omap_hwmod *omap_hwmod_lookup(const char *name)
  *
  * Call @fn for each registered omap_hwmod, passing @data to each
  * function.  @fn must return 0 for success or any other value for
- * failure.  If @fn returns non-zero, the iteration across omap_hwmods
- * will stop and the non-zero return value will be passed to the
- * caller of omap_hwmod_for_each().  @fn is called with
+ * failure. Return value of all callback functions is OR'd and the
+ * value is passed back to the caller. @fn is called with
  * omap_hwmod_for_each() held.
  */
 int omap_hwmod_for_each(int (*fn)(struct omap_hwmod *oh, void *data),
 			void *data)
 {
 	struct omap_hwmod *temp_oh;
-	int ret;
+	int ret = 0;
 
 	if (!fn)
 		return -EINVAL;
 
-	list_for_each_entry(temp_oh, &omap_hwmod_list, node) {
-		ret = (*fn)(temp_oh, data);
-		if (ret)
-			break;
-	}
+	list_for_each_entry(temp_oh, &omap_hwmod_list, node)
+		ret |= (*fn)(temp_oh, data);
 
 	return ret;
 }
@@ -2127,8 +2123,7 @@ int omap_hwmod_read_hardreset(struct omap_hwmod *oh, const char *name)
  * @user: arbitrary context data to pass to the callback function
  *
  * For each omap_hwmod of class @classname, call @fn.
- * If the callback function returns something other than
- * zero, the iterator is terminated, and the callback function's return
+ * Return value of all callback functions is OR'd and the
  * value is passed back to the caller.  Returns 0 upon success, -EINVAL
  * if @classname or @fn are NULL, or passes back the error code from @fn.
  */
@@ -2150,14 +2145,12 @@ int omap_hwmod_for_each_by_class(const char *classname,
 		if (!strcmp(temp_oh->class->name, classname)) {
 			pr_debug("omap_hwmod: %s: %s: calling callback fn\n",
 				 __func__, temp_oh->name);
-			ret = (*fn)(temp_oh, user);
-			if (ret)
-				break;
+			ret |= (*fn)(temp_oh, user);
 		}
 	}
 
 	if (ret)
-		pr_debug("omap_hwmod: %s: iterator terminated early: %d\n",
+		pr_debug("omap_hwmod: %s: one or more callback fn failed: %d\n",
 			 __func__, ret);
 
 	return ret;
-- 
1.7.0.4

[PATCH 3/3] OMAP2+: hwmod: Do not break iterator fn's if one fails

[Rajendra Nayak]

The iterator functions used to iterate over all
registered hwmods and all hwmods of a given class
break if one of the iterator functions fail.
Instead iterate over all the functions and return
an ORed return value back to the user.

Signed-off-by: Rajendra Nayak <rnayak@ti.com>
---
 arch/arm/mach-omap2/omap_hwmod.c |   23 ++++++++---------------
 1 files changed, 8 insertions(+), 15 deletions(-)

diff --git a/arch/arm/mach-omap2/omap_hwmod.c b/arch/arm/mach-omap2/omap_hwmod.c
index 960461f..3cab82e 100644
--- a/arch/arm/mach-omap2/omap_hwmod.c
+++ b/arch/arm/mach-omap2/omap_hwmod.c
@@ -1568,25 +1568,21 @@ struct omap_hwmod *omap_hwmod_lookup(const char *name)
  *
  * Call @fn for each registered omap_hwmod, passing @data to each
  * function.  @fn must return 0 for success or any other value for
- * failure.  If @fn returns non-zero, the iteration across omap_hwmods
- * will stop and the non-zero return value will be passed to the
- * caller of omap_hwmod_for_each().  @fn is called with
+ * failure. Return value of all callback functions is OR'd and the
+ * value is passed back to the caller. @fn is called with
  * omap_hwmod_for_each() held.
  */
 int omap_hwmod_for_each(int (*fn)(struct omap_hwmod *oh, void *data),
 			void *data)
 {
 	struct omap_hwmod *temp_oh;
-	int ret;
+	int ret = 0;
 
 	if (!fn)
 		return -EINVAL;
 
-	list_for_each_entry(temp_oh, &omap_hwmod_list, node) {
-		ret = (*fn)(temp_oh, data);
-		if (ret)
-			break;
-	}
+	list_for_each_entry(temp_oh, &omap_hwmod_list, node)
+		ret |= (*fn)(temp_oh, data);
 
 	return ret;
 }
@@ -2127,8 +2123,7 @@ int omap_hwmod_read_hardreset(struct omap_hwmod *oh, const char *name)
  * @user: arbitrary context data to pass to the callback function
  *
  * For each omap_hwmod of class @classname, call @fn.
- * If the callback function returns something other than
- * zero, the iterator is terminated, and the callback function's return
+ * Return value of all callback functions is OR'd and the
  * value is passed back to the caller.  Returns 0 upon success, -EINVAL
  * if @classname or @fn are NULL, or passes back the error code from @fn.
  */
@@ -2150,14 +2145,12 @@ int omap_hwmod_for_each_by_class(const char *classname,
 		if (!strcmp(temp_oh->class->name, classname)) {
 			pr_debug("omap_hwmod: %s: %s: calling callback fn\n",
 				 __func__, temp_oh->name);
-			ret = (*fn)(temp_oh, user);
-			if (ret)
-				break;
+			ret |= (*fn)(temp_oh, user);
 		}
 	}
 
 	if (ret)
-		pr_debug("omap_hwmod: %s: iterator terminated early: %d\n",
+		pr_debug("omap_hwmod: %s: one or more callback fn failed: %d\n",
 			 __func__, ret);
 
 	return ret;
-- 
1.7.0.4

Re: [PATCH 1/3] OMAP2+: hwmod: Avoid setup if clock lookup failed

[Sergei Shtylyov]

Hello.

On 16.02.2011 15:11, Rajendra Nayak wrote:

> Add a hwmod state check in the _setup function
> to avoid setting up hwmods' for which clock
> lookup has failed.

> Signed-off-by: Rajendra Nayak<rnayak@ti.com>
[...]

> diff --git a/arch/arm/mach-omap2/omap_hwmod.c b/arch/arm/mach-omap2/omap_hwmod.c
> index e282e35..cd9dcde 100644
> --- a/arch/arm/mach-omap2/omap_hwmod.c
> +++ b/arch/arm/mach-omap2/omap_hwmod.c
> @@ -1362,6 +1362,12 @@ static int _setup(struct omap_hwmod *oh, void *data)
>   	int i, r;
>   	u8 postsetup_state;
>
> +	if (oh->_state != _HWMOD_STATE_CLKS_INITED) {
> +		WARN(1, "omap_hwmod: %s: _setup failed as one or more"

    You forgot space bafore " -- "moreclock" will be printed.

> +		     "clock lookups' have failed\n", oh->name);

    Why there's apostrophe here?

WBR, Sergei

[PATCH 1/3] OMAP2+: hwmod: Avoid setup if clock lookup failed

[Sergei Shtylyov]

Hello.

On 16.02.2011 15:11, Rajendra Nayak wrote:

> Add a hwmod state check in the _setup function
> to avoid setting up hwmods' for which clock
> lookup has failed.

> Signed-off-by: Rajendra Nayak<rnayak@ti.com>
[...]

> diff --git a/arch/arm/mach-omap2/omap_hwmod.c b/arch/arm/mach-omap2/omap_hwmod.c
> index e282e35..cd9dcde 100644
> --- a/arch/arm/mach-omap2/omap_hwmod.c
> +++ b/arch/arm/mach-omap2/omap_hwmod.c
> @@ -1362,6 +1362,12 @@ static int _setup(struct omap_hwmod *oh, void *data)
>   	int i, r;
>   	u8 postsetup_state;
>
> +	if (oh->_state != _HWMOD_STATE_CLKS_INITED) {
> +		WARN(1, "omap_hwmod: %s: _setup failed as one or more"

    You forgot space bafore " -- "moreclock" will be printed.

> +		     "clock lookups' have failed\n", oh->name);

    Why there's apostrophe here?

WBR, Sergei

Re: [PATCH 0/3] OMAP2+ hwmod fixes

[Cousson, Benoit]

Hi Rajendra,

On 2/16/2011 1:11 PM, Nayak, Rajendra wrote:
> This series fixes some hwmod api return values
> and also adds some state checks.
> The hwmod iterator functions are made to
> continue and not break if one of the
> callback functions ends up with an error.

By doing that, you change the behavior of this function.
I'm not sure I fully understand why.
Could you elaborate on the use case?

To avoid that behavior in the past, I was just returning
0 in case of failure to avoid stopping the iteration.
It looks like you do not want to stop the iteration but still
retrieve the error.
I do not see in this series what you plan to do with the
error at the end of the iteration.

Thanks,
Benoit

[PATCH 0/3] OMAP2+ hwmod fixes

[Cousson, Benoit]

Hi Rajendra,

On 2/16/2011 1:11 PM, Nayak, Rajendra wrote:
> This series fixes some hwmod api return values
> and also adds some state checks.
> The hwmod iterator functions are made to
> continue and not break if one of the
> callback functions ends up with an error.

By doing that, you change the behavior of this function.
I'm not sure I fully understand why.
Could you elaborate on the use case?

To avoid that behavior in the past, I was just returning
0 in case of failure to avoid stopping the iteration.
It looks like you do not want to stop the iteration but still
retrieve the error.
I do not see in this series what you plan to do with the
error at the end of the iteration.

Thanks,
Benoit

RE: [PATCH 0/3] OMAP2+ hwmod fixes

[Rajendra Nayak]

Hi Benoit,

> -----Original Message-----
> From: Cousson, Benoit [mailto:b-cousson@ti.com]
> Sent: Wednesday, February 16, 2011 6:37 PM
> To: Nayak, Rajendra
> Cc: linux-omap@vger.kernel.org; paul@pwsan.com;
linux-arm-kernel@lists.infradead.org
> Subject: Re: [PATCH 0/3] OMAP2+ hwmod fixes
>
> Hi Rajendra,
>
> On 2/16/2011 1:11 PM, Nayak, Rajendra wrote:
> > This series fixes some hwmod api return values
> > and also adds some state checks.
> > The hwmod iterator functions are made to
> > continue and not break if one of the
> > callback functions ends up with an error.
>
> By doing that, you change the behavior of this function.
> I'm not sure I fully understand why.
> Could you elaborate on the use case?

Since these functions iterate over all hwmods
calling a common callback function, there might
be cases wherein the callback function for *some*
hwmods might fail. For instance, if you run through
all hwmods and try to clear the context registers
for all of them, for some hwmods which might
not have context registers the callback function
might return a -EINVAL, however that should not
stop you from attempting to clear the context
registers for the rest of the hwmods which have
them and abort the function midway, no?
This is more hypothetical, however the real usecase
that prompted me with this patch was when I
had some wrong state check in _setup function,
and the iterator would stop with the first failure
and not even attempt to setup the rest of the
hwmods.

>
> To avoid that behavior in the past, I was just returning
> 0 in case of failure to avoid stopping the iteration.
> It looks like you do not want to stop the iteration but still
> retrieve the error.
> I do not see in this series what you plan to do with the
> error at the end of the iteration.

Most users of these iterators would just use the non-zero
return value to throw an error/warning out stating there
were failures running through all the callback functions.
That does not change with this patch, and they can still
do it.

Regards,
Rajendra

>
> Thanks,
> Benoit

[PATCH 0/3] OMAP2+ hwmod fixes

[Rajendra Nayak]

Hi Benoit,

> -----Original Message-----
> From: Cousson, Benoit [mailto:b-cousson at ti.com]
> Sent: Wednesday, February 16, 2011 6:37 PM
> To: Nayak, Rajendra
> Cc: linux-omap at vger.kernel.org; paul at pwsan.com;
linux-arm-kernel at lists.infradead.org
> Subject: Re: [PATCH 0/3] OMAP2+ hwmod fixes
>
> Hi Rajendra,
>
> On 2/16/2011 1:11 PM, Nayak, Rajendra wrote:
> > This series fixes some hwmod api return values
> > and also adds some state checks.
> > The hwmod iterator functions are made to
> > continue and not break if one of the
> > callback functions ends up with an error.
>
> By doing that, you change the behavior of this function.
> I'm not sure I fully understand why.
> Could you elaborate on the use case?

Since these functions iterate over all hwmods
calling a common callback function, there might
be cases wherein the callback function for *some*
hwmods might fail. For instance, if you run through
all hwmods and try to clear the context registers
for all of them, for some hwmods which might
not have context registers the callback function
might return a -EINVAL, however that should not
stop you from attempting to clear the context
registers for the rest of the hwmods which have
them and abort the function midway, no?
This is more hypothetical, however the real usecase
that prompted me with this patch was when I
had some wrong state check in _setup function,
and the iterator would stop with the first failure
and not even attempt to setup the rest of the
hwmods.

>
> To avoid that behavior in the past, I was just returning
> 0 in case of failure to avoid stopping the iteration.
> It looks like you do not want to stop the iteration but still
> retrieve the error.
> I do not see in this series what you plan to do with the
> error at the end of the iteration.

Most users of these iterators would just use the non-zero
return value to throw an error/warning out stating there
were failures running through all the callback functions.
That does not change with this patch, and they can still
do it.

Regards,
Rajendra

>
> Thanks,
> Benoit

Re: [PATCH 0/3] OMAP2+ hwmod fixes

[Cousson, Benoit]

Hi Rajendra,

On 2/16/2011 2:43 PM, Nayak, Rajendra wrote:
> Hi Benoit,
>
>> -----Original Message-----
>> From: Cousson, Benoit [mailto:b-cousson@ti.com]
>> Sent: Wednesday, February 16, 2011 6:37 PM
>> To: Nayak, Rajendra
>> Cc: linux-omap@vger.kernel.org; paul@pwsan.com;
> linux-arm-kernel@lists.infradead.org
>> Subject: Re: [PATCH 0/3] OMAP2+ hwmod fixes
>>
>> Hi Rajendra,
>>
>> On 2/16/2011 1:11 PM, Nayak, Rajendra wrote:
>>> This series fixes some hwmod api return values
>>> and also adds some state checks.
>>> The hwmod iterator functions are made to
>>> continue and not break if one of the
>>> callback functions ends up with an error.
>>
>> By doing that, you change the behavior of this function.
>> I'm not sure I fully understand why.
>> Could you elaborate on the use case?
>
> Since these functions iterate over all hwmods
> calling a common callback function, there might
> be cases wherein the callback function for *some*
> hwmods might fail. For instance, if you run through
> all hwmods and try to clear the context registers
> for all of them, for some hwmods which might
> not have context registers the callback function
> might return a -EINVAL, however that should not
> stop you from attempting to clear the context
> registers for the rest of the hwmods which have
> them and abort the function midway, no?
> This is more hypothetical, however the real usecase
> that prompted me with this patch was when I
> had some wrong state check in _setup function,
> and the iterator would stop with the first failure
> and not even attempt to setup the rest of the
> hwmods.

Yeah, but by using that function you implicitly accept that an error 
will break the loop, so the function you pass to the iterator should be 
written for that. Meaning if you do not want to exit on error you should 
not report an error.

>> To avoid that behavior in the past, I was just returning
>> 0 in case of failure to avoid stopping the iteration.
>> It looks like you do not want to stop the iteration but still
>> retrieve the error.
>> I do not see in this series what you plan to do with the
>> error at the end of the iteration.
>
> Most users of these iterators would just use the non-zero
> return value to throw an error/warning out stating there
> were failures running through all the callback functions.
> That does not change with this patch, and they can still
> do it.

Except that now, the iterator is not able anymore to stop on error if 
needed potentially.
My point is that you are changing the behavior of this function without 
maintaining the legacy.

So maybe creating a new iterator is a better approach.
Even is this feature is not used today I have some secret plan for this 
behavior in the near future :-)

But my initial comment is still valid, if you do not care about the 
final error status after the iteration, you'd better not return any 
error at the beginning.
This was the behavior or the _setup until now.

Regards,
Benoit

[PATCH 0/3] OMAP2+ hwmod fixes

[Cousson, Benoit]

Hi Rajendra,

On 2/16/2011 2:43 PM, Nayak, Rajendra wrote:
> Hi Benoit,
>
>> -----Original Message-----
>> From: Cousson, Benoit [mailto:b-cousson at ti.com]
>> Sent: Wednesday, February 16, 2011 6:37 PM
>> To: Nayak, Rajendra
>> Cc: linux-omap at vger.kernel.org; paul at pwsan.com;
> linux-arm-kernel at lists.infradead.org
>> Subject: Re: [PATCH 0/3] OMAP2+ hwmod fixes
>>
>> Hi Rajendra,
>>
>> On 2/16/2011 1:11 PM, Nayak, Rajendra wrote:
>>> This series fixes some hwmod api return values
>>> and also adds some state checks.
>>> The hwmod iterator functions are made to
>>> continue and not break if one of the
>>> callback functions ends up with an error.
>>
>> By doing that, you change the behavior of this function.
>> I'm not sure I fully understand why.
>> Could you elaborate on the use case?
>
> Since these functions iterate over all hwmods
> calling a common callback function, there might
> be cases wherein the callback function for *some*
> hwmods might fail. For instance, if you run through
> all hwmods and try to clear the context registers
> for all of them, for some hwmods which might
> not have context registers the callback function
> might return a -EINVAL, however that should not
> stop you from attempting to clear the context
> registers for the rest of the hwmods which have
> them and abort the function midway, no?
> This is more hypothetical, however the real usecase
> that prompted me with this patch was when I
> had some wrong state check in _setup function,
> and the iterator would stop with the first failure
> and not even attempt to setup the rest of the
> hwmods.

Yeah, but by using that function you implicitly accept that an error 
will break the loop, so the function you pass to the iterator should be 
written for that. Meaning if you do not want to exit on error you should 
not report an error.

>> To avoid that behavior in the past, I was just returning
>> 0 in case of failure to avoid stopping the iteration.
>> It looks like you do not want to stop the iteration but still
>> retrieve the error.
>> I do not see in this series what you plan to do with the
>> error at the end of the iteration.
>
> Most users of these iterators would just use the non-zero
> return value to throw an error/warning out stating there
> were failures running through all the callback functions.
> That does not change with this patch, and they can still
> do it.

Except that now, the iterator is not able anymore to stop on error if 
needed potentially.
My point is that you are changing the behavior of this function without 
maintaining the legacy.

So maybe creating a new iterator is a better approach.
Even is this feature is not used today I have some secret plan for this 
behavior in the near future :-)

But my initial comment is still valid, if you do not care about the 
final error status after the iteration, you'd better not return any 
error at the beginning.
This was the behavior or the _setup until now.

Regards,
Benoit

Re: [PATCH 2/3] OMAP2+: hwmod: Fix what _init_clock returns

[Cousson, Benoit]

On 2/16/2011 1:11 PM, Nayak, Rajendra wrote:
> _init_clock always returns 0 and does
> not propogate the error (in case of failure)
> back to the caller, causing _init_clocks to
> fail silently.
>
> Signed-off-by: Rajendra Nayak<rnayak@ti.com>
> ---
>   arch/arm/mach-omap2/omap_hwmod.c |    2 +-
>   1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/omap_hwmod.c b/arch/arm/mach-omap2/omap_hwmod.c
> index cd9dcde..960461f 100644
> --- a/arch/arm/mach-omap2/omap_hwmod.c
> +++ b/arch/arm/mach-omap2/omap_hwmod.c
> @@ -926,7 +926,7 @@ static int _init_clocks(struct omap_hwmod *oh, void *data)
>   	if (!ret)
>   		oh->_state = _HWMOD_STATE_CLKS_INITED;
>
> -	return 0;
> +	return ret;
>   }
>
>   /**

This is correct and that makes kerneldoc accurate : "Returns ... a 
non-zero error on failure."

I'll queue it for 2.6.39.

Thanks,
Benoit

[PATCH 2/3] OMAP2+: hwmod: Fix what _init_clock returns

[Cousson, Benoit]

On 2/16/2011 1:11 PM, Nayak, Rajendra wrote:
> _init_clock always returns 0 and does
> not propogate the error (in case of failure)
> back to the caller, causing _init_clocks to
> fail silently.
>
> Signed-off-by: Rajendra Nayak<rnayak@ti.com>
> ---
>   arch/arm/mach-omap2/omap_hwmod.c |    2 +-
>   1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/omap_hwmod.c b/arch/arm/mach-omap2/omap_hwmod.c
> index cd9dcde..960461f 100644
> --- a/arch/arm/mach-omap2/omap_hwmod.c
> +++ b/arch/arm/mach-omap2/omap_hwmod.c
> @@ -926,7 +926,7 @@ static int _init_clocks(struct omap_hwmod *oh, void *data)
>   	if (!ret)
>   		oh->_state = _HWMOD_STATE_CLKS_INITED;
>
> -	return 0;
> +	return ret;
>   }
>
>   /**

This is correct and that makes kerneldoc accurate : "Returns ... a 
non-zero error on failure."

I'll queue it for 2.6.39.

Thanks,
Benoit

Re: [PATCH 1/3] OMAP2+: hwmod: Avoid setup if clock lookup failed

[Cousson, Benoit]

On 2/16/2011 1:11 PM, Nayak, Rajendra wrote:
> Add a hwmod state check in the _setup function
> to avoid setting up hwmods' for which clock
> lookup has failed.
>
> Signed-off-by: Rajendra Nayak<rnayak@ti.com>
> ---
>   arch/arm/mach-omap2/omap_hwmod.c |    6 ++++++
>   1 files changed, 6 insertions(+), 0 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/omap_hwmod.c b/arch/arm/mach-omap2/omap_hwmod.c
> index e282e35..cd9dcde 100644
> --- a/arch/arm/mach-omap2/omap_hwmod.c
> +++ b/arch/arm/mach-omap2/omap_hwmod.c
> @@ -1362,6 +1362,12 @@ static int _setup(struct omap_hwmod *oh, void *data)
>   	int i, r;
>   	u8 postsetup_state;
>
> +	if (oh->_state != _HWMOD_STATE_CLKS_INITED) {
> +		WARN(1, "omap_hwmod: %s: _setup failed as one or more"
> +		     "clock lookups' have failed\n", oh->name);

Maybe a pr_warning will be enough for that?

> +		return -EINVAL;

As discussed previously I'd prefer to return 0 here since we do not want 
to break the iteration.

> +	}
> +
>   	/* Set iclk autoidle mode */
>   	if (oh->slaves_cnt>  0) {
>   		for (i = 0; i<  oh->slaves_cnt; i++) {

Otherwise it is fine for me.

Thanks,
Benoit

[PATCH 1/3] OMAP2+: hwmod: Avoid setup if clock lookup failed

[Cousson, Benoit]

On 2/16/2011 1:11 PM, Nayak, Rajendra wrote:
> Add a hwmod state check in the _setup function
> to avoid setting up hwmods' for which clock
> lookup has failed.
>
> Signed-off-by: Rajendra Nayak<rnayak@ti.com>
> ---
>   arch/arm/mach-omap2/omap_hwmod.c |    6 ++++++
>   1 files changed, 6 insertions(+), 0 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/omap_hwmod.c b/arch/arm/mach-omap2/omap_hwmod.c
> index e282e35..cd9dcde 100644
> --- a/arch/arm/mach-omap2/omap_hwmod.c
> +++ b/arch/arm/mach-omap2/omap_hwmod.c
> @@ -1362,6 +1362,12 @@ static int _setup(struct omap_hwmod *oh, void *data)
>   	int i, r;
>   	u8 postsetup_state;
>
> +	if (oh->_state != _HWMOD_STATE_CLKS_INITED) {
> +		WARN(1, "omap_hwmod: %s: _setup failed as one or more"
> +		     "clock lookups' have failed\n", oh->name);

Maybe a pr_warning will be enough for that?

> +		return -EINVAL;

As discussed previously I'd prefer to return 0 here since we do not want 
to break the iteration.

> +	}
> +
>   	/* Set iclk autoidle mode */
>   	if (oh->slaves_cnt>  0) {
>   		for (i = 0; i<  oh->slaves_cnt; i++) {

Otherwise it is fine for me.

Thanks,
Benoit

Re: [PATCH 0/3] OMAP2+ hwmod fixes

[Paul Walmsley]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 4960 bytes --]

Hello Benoît, Rajendra,

On Fri, 18 Feb 2011, Cousson, Benoit wrote:

> On 2/16/2011 2:43 PM, Nayak, Rajendra wrote:
> > > -----Original Message-----
> > > From: Cousson, Benoit [mailto:b-cousson@ti.com]
> > > Sent: Wednesday, February 16, 2011 6:37 PM
> > > To: Nayak, Rajendra
> > > Cc: linux-omap@vger.kernel.org; paul@pwsan.com;
> > linux-arm-kernel@lists.infradead.org
> > > Subject: Re: [PATCH 0/3] OMAP2+ hwmod fixes
> > > 
> > > Hi Rajendra,
> > > 
> > > On 2/16/2011 1:11 PM, Nayak, Rajendra wrote:
> > > > This series fixes some hwmod api return values
> > > > and also adds some state checks.
> > > > The hwmod iterator functions are made to
> > > > continue and not break if one of the
> > > > callback functions ends up with an error.
> > > 
> > > By doing that, you change the behavior of this function.
> > > I'm not sure I fully understand why.
> > > Could you elaborate on the use case?
> > 
> > Since these functions iterate over all hwmods
> > calling a common callback function, there might
> > be cases wherein the callback function for *some*
> > hwmods might fail. For instance, if you run through
> > all hwmods and try to clear the context registers
> > for all of them, for some hwmods which might
> > not have context registers the callback function
> > might return a -EINVAL, however that should not
> > stop you from attempting to clear the context
> > registers for the rest of the hwmods which have
> > them and abort the function midway, no?
> > This is more hypothetical, however the real usecase
> > that prompted me with this patch was when I
> > had some wrong state check in _setup function,
> > and the iterator would stop with the first failure
> > and not even attempt to setup the rest of the
> > hwmods.
> 
> Yeah, but by using that function you implicitly accept that an error will
> break the loop, so the function you pass to the iterator should be written for
> that. Meaning if you do not want to exit on error you should not report an
> error.
> 
> > > To avoid that behavior in the past, I was just returning
> > > 0 in case of failure to avoid stopping the iteration.
> > > It looks like you do not want to stop the iteration but still
> > > retrieve the error.
> > > I do not see in this series what you plan to do with the
> > > error at the end of the iteration.
> > 
> > Most users of these iterators would just use the non-zero
> > return value to throw an error/warning out stating there
> > were failures running through all the callback functions.
> > That does not change with this patch, and they can still
> > do it.
> 
> Except that now, the iterator is not able anymore to stop on error if needed
> potentially.
> My point is that you are changing the behavior of this function without
> maintaining the legacy.
> 
> So maybe creating a new iterator is a better approach.
> Even is this feature is not used today I have some secret plan for this
> behavior in the near future :-)
> 
> But my initial comment is still valid, if you do not care about the final
> error status after the iteration, you'd better not return any error at the
> beginning.
> This was the behavior or the _setup until now.

The original behavior of the iterator was intentional: loops over 
functions like _init_clocks() and _setup() should terminate upon 
encountering an error.  This is because the rest of that code currently 
relies on the hwmod/clock data to be accurate in order to work.  There is 
no provision in the code for a hwmod to fail initialization due to data 
errors.  The idea was that if the data is inaccurate, the data should be 
fixed first before doing anything else.

That said, I suppose that it's possible to enhance the code such that 
hwmods could be allowed to fail initialization, and simply brick 
themselves, rather than prevent the rest of the hwmods from initializing.  
Probably that would need a new _HWMOD_STATE_INIT_FAILED, or something 
similar.  If a hwmod would end up in that state, it must not be used by 
any other code, and the code should complain loudly.

The broader issue of whether the iterators should return immediately upon 
failure (as the current code does), or continue through the rest of the 
list, is a separate one.  I'd suggest one of two approaches:

1. If the rest of the code can be changed to gracefully handle cases where 
hwmod initialization fails, and if Benoît agrees, I don't have a problem 
with changing the iterator behavior to ignore failures as you describe.  
Of course, all of the current users of omap_hwmod_for_each*() would need 
to be checked to ensure that this behavior makes sense for them.

... or ...

2. The iterators could take an extra parameter that would control the 
behavior upon encountering an error: terminate, or continue.  But I am not 
sure that both cases are needed.  Ideas, feedback here are welcome.


- Paul

[PATCH 0/3] OMAP2+ hwmod fixes

[Paul Walmsley]

Hello Beno?t, Rajendra,

On Fri, 18 Feb 2011, Cousson, Benoit wrote:

> On 2/16/2011 2:43 PM, Nayak, Rajendra wrote:
> > > -----Original Message-----
> > > From: Cousson, Benoit [mailto:b-cousson at ti.com]
> > > Sent: Wednesday, February 16, 2011 6:37 PM
> > > To: Nayak, Rajendra
> > > Cc: linux-omap at vger.kernel.org; paul at pwsan.com;
> > linux-arm-kernel at lists.infradead.org
> > > Subject: Re: [PATCH 0/3] OMAP2+ hwmod fixes
> > > 
> > > Hi Rajendra,
> > > 
> > > On 2/16/2011 1:11 PM, Nayak, Rajendra wrote:
> > > > This series fixes some hwmod api return values
> > > > and also adds some state checks.
> > > > The hwmod iterator functions are made to
> > > > continue and not break if one of the
> > > > callback functions ends up with an error.
> > > 
> > > By doing that, you change the behavior of this function.
> > > I'm not sure I fully understand why.
> > > Could you elaborate on the use case?
> > 
> > Since these functions iterate over all hwmods
> > calling a common callback function, there might
> > be cases wherein the callback function for *some*
> > hwmods might fail. For instance, if you run through
> > all hwmods and try to clear the context registers
> > for all of them, for some hwmods which might
> > not have context registers the callback function
> > might return a -EINVAL, however that should not
> > stop you from attempting to clear the context
> > registers for the rest of the hwmods which have
> > them and abort the function midway, no?
> > This is more hypothetical, however the real usecase
> > that prompted me with this patch was when I
> > had some wrong state check in _setup function,
> > and the iterator would stop with the first failure
> > and not even attempt to setup the rest of the
> > hwmods.
> 
> Yeah, but by using that function you implicitly accept that an error will
> break the loop, so the function you pass to the iterator should be written for
> that. Meaning if you do not want to exit on error you should not report an
> error.
> 
> > > To avoid that behavior in the past, I was just returning
> > > 0 in case of failure to avoid stopping the iteration.
> > > It looks like you do not want to stop the iteration but still
> > > retrieve the error.
> > > I do not see in this series what you plan to do with the
> > > error at the end of the iteration.
> > 
> > Most users of these iterators would just use the non-zero
> > return value to throw an error/warning out stating there
> > were failures running through all the callback functions.
> > That does not change with this patch, and they can still
> > do it.
> 
> Except that now, the iterator is not able anymore to stop on error if needed
> potentially.
> My point is that you are changing the behavior of this function without
> maintaining the legacy.
> 
> So maybe creating a new iterator is a better approach.
> Even is this feature is not used today I have some secret plan for this
> behavior in the near future :-)
> 
> But my initial comment is still valid, if you do not care about the final
> error status after the iteration, you'd better not return any error at the
> beginning.
> This was the behavior or the _setup until now.

The original behavior of the iterator was intentional: loops over 
functions like _init_clocks() and _setup() should terminate upon 
encountering an error.  This is because the rest of that code currently 
relies on the hwmod/clock data to be accurate in order to work.  There is 
no provision in the code for a hwmod to fail initialization due to data 
errors.  The idea was that if the data is inaccurate, the data should be 
fixed first before doing anything else.

That said, I suppose that it's possible to enhance the code such that 
hwmods could be allowed to fail initialization, and simply brick 
themselves, rather than prevent the rest of the hwmods from initializing.  
Probably that would need a new _HWMOD_STATE_INIT_FAILED, or something 
similar.  If a hwmod would end up in that state, it must not be used by 
any other code, and the code should complain loudly.

The broader issue of whether the iterators should return immediately upon 
failure (as the current code does), or continue through the rest of the 
list, is a separate one.  I'd suggest one of two approaches:

1. If the rest of the code can be changed to gracefully handle cases where 
hwmod initialization fails, and if Beno?t agrees, I don't have a problem 
with changing the iterator behavior to ignore failures as you describe.  
Of course, all of the current users of omap_hwmod_for_each*() would need 
to be checked to ensure that this behavior makes sense for them.

... or ...

2. The iterators could take an extra parameter that would control the 
behavior upon encountering an error: terminate, or continue.  But I am not 
sure that both cases are needed.  Ideas, feedback here are welcome.


- Paul

RE: [PATCH 0/3] OMAP2+ hwmod fixes

[Rajendra Nayak]

Hi Paul,

> -----Original Message-----
> From: Paul Walmsley [mailto:paul@pwsan.com]
> Sent: Monday, February 21, 2011 10:25 PM
> To: Cousson, Benoit; Nayak, Rajendra
> Cc: linux-omap@vger.kernel.org; linux-arm-kernel@lists.infradead.org
> Subject: Re: [PATCH 0/3] OMAP2+ hwmod fixes
>
> Hello Benoît, Rajendra,
>
> On Fri, 18 Feb 2011, Cousson, Benoit wrote:
>
> > On 2/16/2011 2:43 PM, Nayak, Rajendra wrote:
> > > > -----Original Message-----
> > > > From: Cousson, Benoit [mailto:b-cousson@ti.com]
> > > > Sent: Wednesday, February 16, 2011 6:37 PM
> > > > To: Nayak, Rajendra
> > > > Cc: linux-omap@vger.kernel.org; paul@pwsan.com;
> > > linux-arm-kernel@lists.infradead.org
> > > > Subject: Re: [PATCH 0/3] OMAP2+ hwmod fixes
> > > >
> > > > Hi Rajendra,
> > > >
> > > > On 2/16/2011 1:11 PM, Nayak, Rajendra wrote:
> > > > > This series fixes some hwmod api return values
> > > > > and also adds some state checks.
> > > > > The hwmod iterator functions are made to
> > > > > continue and not break if one of the
> > > > > callback functions ends up with an error.
> > > >
> > > > By doing that, you change the behavior of this function.
> > > > I'm not sure I fully understand why.
> > > > Could you elaborate on the use case?
> > >
> > > Since these functions iterate over all hwmods
> > > calling a common callback function, there might
> > > be cases wherein the callback function for *some*
> > > hwmods might fail. For instance, if you run through
> > > all hwmods and try to clear the context registers
> > > for all of them, for some hwmods which might
> > > not have context registers the callback function
> > > might return a -EINVAL, however that should not
> > > stop you from attempting to clear the context
> > > registers for the rest of the hwmods which have
> > > them and abort the function midway, no?
> > > This is more hypothetical, however the real usecase
> > > that prompted me with this patch was when I
> > > had some wrong state check in _setup function,
> > > and the iterator would stop with the first failure
> > > and not even attempt to setup the rest of the
> > > hwmods.
> >
> > Yeah, but by using that function you implicitly accept that an error
will
> > break the loop, so the function you pass to the iterator should be
written for
> > that. Meaning if you do not want to exit on error you should not
report an
> > error.
> >
> > > > To avoid that behavior in the past, I was just returning
> > > > 0 in case of failure to avoid stopping the iteration.
> > > > It looks like you do not want to stop the iteration but still
> > > > retrieve the error.
> > > > I do not see in this series what you plan to do with the
> > > > error at the end of the iteration.
> > >
> > > Most users of these iterators would just use the non-zero
> > > return value to throw an error/warning out stating there
> > > were failures running through all the callback functions.
> > > That does not change with this patch, and they can still
> > > do it.
> >
> > Except that now, the iterator is not able anymore to stop on error if
needed
> > potentially.
> > My point is that you are changing the behavior of this function
without
> > maintaining the legacy.
> >
> > So maybe creating a new iterator is a better approach.
> > Even is this feature is not used today I have some secret plan for
this
> > behavior in the near future :-)
> >
> > But my initial comment is still valid, if you do not care about the
final
> > error status after the iteration, you'd better not return any error at
the
> > beginning.
> > This was the behavior or the _setup until now.
>
> The original behavior of the iterator was intentional: loops over
> functions like _init_clocks() and _setup() should terminate upon
> encountering an error.  This is because the rest of that code currently
> relies on the hwmod/clock data to be accurate in order to work.  There
is
> no provision in the code for a hwmod to fail initialization due to data
> errors.  The idea was that if the data is inaccurate, the data should be
> fixed first before doing anything else.

The original behavior of the iterators, to terminate upon
encountering an error, seems fine to me. The only problem
I faced was that they fail silently and go undetected, unless
their user catches the return value and WARN's, which I found
was not the case with most users, mainly those of
omap_hwmod_for_each_by_class.
I was thinking of keeping the behaviour of these iterators
same for now and add WARN's in these iterators itself upon
an error, so its seen even if the user fails to catch it.

Regards,
Rajendra

>
> That said, I suppose that it's possible to enhance the code such that
> hwmods could be allowed to fail initialization, and simply brick
> themselves, rather than prevent the rest of the hwmods from
initializing.
> Probably that would need a new _HWMOD_STATE_INIT_FAILED, or something
> similar.  If a hwmod would end up in that state, it must not be used by
> any other code, and the code should complain loudly.
>
> The broader issue of whether the iterators should return immediately
upon
> failure (as the current code does), or continue through the rest of the
> list, is a separate one.  I'd suggest one of two approaches:
>
> 1. If the rest of the code can be changed to gracefully handle cases
where
> hwmod initialization fails, and if Benoît agrees, I don't have a problem
> with changing the iterator behavior to ignore failures as you describe.
> Of course, all of the current users of omap_hwmod_for_each*() would need
> to be checked to ensure that this behavior makes sense for them.
>
> ... or ...
>
> 2. The iterators could take an extra parameter that would control the
> behavior upon encountering an error: terminate, or continue.  But I am
not
> sure that both cases are needed.  Ideas, feedback here are welcome.
>
>
> - Paul
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 0/3] OMAP2+ hwmod fixes

[Rajendra Nayak]

Hi Paul,

> -----Original Message-----
> From: Paul Walmsley [mailto:paul at pwsan.com]
> Sent: Monday, February 21, 2011 10:25 PM
> To: Cousson, Benoit; Nayak, Rajendra
> Cc: linux-omap at vger.kernel.org; linux-arm-kernel at lists.infradead.org
> Subject: Re: [PATCH 0/3] OMAP2+ hwmod fixes
>
> Hello Beno?t, Rajendra,
>
> On Fri, 18 Feb 2011, Cousson, Benoit wrote:
>
> > On 2/16/2011 2:43 PM, Nayak, Rajendra wrote:
> > > > -----Original Message-----
> > > > From: Cousson, Benoit [mailto:b-cousson at ti.com]
> > > > Sent: Wednesday, February 16, 2011 6:37 PM
> > > > To: Nayak, Rajendra
> > > > Cc: linux-omap at vger.kernel.org; paul at pwsan.com;
> > > linux-arm-kernel at lists.infradead.org
> > > > Subject: Re: [PATCH 0/3] OMAP2+ hwmod fixes
> > > >
> > > > Hi Rajendra,
> > > >
> > > > On 2/16/2011 1:11 PM, Nayak, Rajendra wrote:
> > > > > This series fixes some hwmod api return values
> > > > > and also adds some state checks.
> > > > > The hwmod iterator functions are made to
> > > > > continue and not break if one of the
> > > > > callback functions ends up with an error.
> > > >
> > > > By doing that, you change the behavior of this function.
> > > > I'm not sure I fully understand why.
> > > > Could you elaborate on the use case?
> > >
> > > Since these functions iterate over all hwmods
> > > calling a common callback function, there might
> > > be cases wherein the callback function for *some*
> > > hwmods might fail. For instance, if you run through
> > > all hwmods and try to clear the context registers
> > > for all of them, for some hwmods which might
> > > not have context registers the callback function
> > > might return a -EINVAL, however that should not
> > > stop you from attempting to clear the context
> > > registers for the rest of the hwmods which have
> > > them and abort the function midway, no?
> > > This is more hypothetical, however the real usecase
> > > that prompted me with this patch was when I
> > > had some wrong state check in _setup function,
> > > and the iterator would stop with the first failure
> > > and not even attempt to setup the rest of the
> > > hwmods.
> >
> > Yeah, but by using that function you implicitly accept that an error
will
> > break the loop, so the function you pass to the iterator should be
written for
> > that. Meaning if you do not want to exit on error you should not
report an
> > error.
> >
> > > > To avoid that behavior in the past, I was just returning
> > > > 0 in case of failure to avoid stopping the iteration.
> > > > It looks like you do not want to stop the iteration but still
> > > > retrieve the error.
> > > > I do not see in this series what you plan to do with the
> > > > error at the end of the iteration.
> > >
> > > Most users of these iterators would just use the non-zero
> > > return value to throw an error/warning out stating there
> > > were failures running through all the callback functions.
> > > That does not change with this patch, and they can still
> > > do it.
> >
> > Except that now, the iterator is not able anymore to stop on error if
needed
> > potentially.
> > My point is that you are changing the behavior of this function
without
> > maintaining the legacy.
> >
> > So maybe creating a new iterator is a better approach.
> > Even is this feature is not used today I have some secret plan for
this
> > behavior in the near future :-)
> >
> > But my initial comment is still valid, if you do not care about the
final
> > error status after the iteration, you'd better not return any error at
the
> > beginning.
> > This was the behavior or the _setup until now.
>
> The original behavior of the iterator was intentional: loops over
> functions like _init_clocks() and _setup() should terminate upon
> encountering an error.  This is because the rest of that code currently
> relies on the hwmod/clock data to be accurate in order to work.  There
is
> no provision in the code for a hwmod to fail initialization due to data
> errors.  The idea was that if the data is inaccurate, the data should be
> fixed first before doing anything else.

The original behavior of the iterators, to terminate upon
encountering an error, seems fine to me. The only problem
I faced was that they fail silently and go undetected, unless
their user catches the return value and WARN's, which I found
was not the case with most users, mainly those of
omap_hwmod_for_each_by_class.
I was thinking of keeping the behaviour of these iterators
same for now and add WARN's in these iterators itself upon
an error, so its seen even if the user fails to catch it.

Regards,
Rajendra

>
> That said, I suppose that it's possible to enhance the code such that
> hwmods could be allowed to fail initialization, and simply brick
> themselves, rather than prevent the rest of the hwmods from
initializing.
> Probably that would need a new _HWMOD_STATE_INIT_FAILED, or something
> similar.  If a hwmod would end up in that state, it must not be used by
> any other code, and the code should complain loudly.
>
> The broader issue of whether the iterators should return immediately
upon
> failure (as the current code does), or continue through the rest of the
> list, is a separate one.  I'd suggest one of two approaches:
>
> 1. If the rest of the code can be changed to gracefully handle cases
where
> hwmod initialization fails, and if Beno?t agrees, I don't have a problem
> with changing the iterator behavior to ignore failures as you describe.
> Of course, all of the current users of omap_hwmod_for_each*() would need
> to be checked to ensure that this behavior makes sense for them.
>
> ... or ...
>
> 2. The iterators could take an extra parameter that would control the
> behavior upon encountering an error: terminate, or continue.  But I am
not
> sure that both cases are needed.  Ideas, feedback here are welcome.
>
>
> - Paul

RE: [PATCH 0/3] OMAP2+ hwmod fixes

[Paul Walmsley]

Hi Rajendra

On Tue, 22 Feb 2011, Rajendra Nayak wrote:

> The original behavior of the iterators, to terminate upon
> encountering an error, seems fine to me. The only problem
> I faced was that they fail silently and go undetected, unless
> their user catches the return value and WARN's, which I found
> was not the case with most users, mainly those of
> omap_hwmod_for_each_by_class.
> I was thinking of keeping the behaviour of these iterators
> same for now and add WARN's in these iterators itself upon
> an error, so its seen even if the user fails to catch it.

What's your opinion on adding the pr_err() or WARN() into the code that 
the iterator calls for each hwmod?  That code should know why something 
fails, so it should be able to provide a more detailed error message.  Of 
course, it is not as general a solution...


- Paul

[PATCH 0/3] OMAP2+ hwmod fixes

[Paul Walmsley]

Hi Rajendra

On Tue, 22 Feb 2011, Rajendra Nayak wrote:

> The original behavior of the iterators, to terminate upon
> encountering an error, seems fine to me. The only problem
> I faced was that they fail silently and go undetected, unless
> their user catches the return value and WARN's, which I found
> was not the case with most users, mainly those of
> omap_hwmod_for_each_by_class.
> I was thinking of keeping the behaviour of these iterators
> same for now and add WARN's in these iterators itself upon
> an error, so its seen even if the user fails to catch it.

What's your opinion on adding the pr_err() or WARN() into the code that 
the iterator calls for each hwmod?  That code should know why something 
fails, so it should be able to provide a more detailed error message.  Of 
course, it is not as general a solution...


- Paul

RE: [PATCH 0/3] OMAP2+ hwmod fixes

[Rajendra Nayak]

Hi Paul,

> -----Original Message-----
> From: Paul Walmsley [mailto:paul@pwsan.com]
> Sent: Wednesday, February 23, 2011 12:40 AM
> To: Rajendra Nayak; Cousson, Benoit
> Cc: linux-omap@vger.kernel.org; linux-arm-kernel@lists.infradead.org
> Subject: RE: [PATCH 0/3] OMAP2+ hwmod fixes
>
> Hi Rajendra
>
> On Tue, 22 Feb 2011, Rajendra Nayak wrote:
>
> > The original behavior of the iterators, to terminate upon
> > encountering an error, seems fine to me. The only problem
> > I faced was that they fail silently and go undetected, unless
> > their user catches the return value and WARN's, which I found
> > was not the case with most users, mainly those of
> > omap_hwmod_for_each_by_class.
> > I was thinking of keeping the behaviour of these iterators
> > same for now and add WARN's in these iterators itself upon
> > an error, so its seen even if the user fails to catch it.
>
> What's your opinion on adding the pr_err() or WARN() into the code that
> the iterator calls for each hwmod?  That code should know why something
> fails, so it should be able to provide a more detailed error message.
Of
> course, it is not as general a solution...

I agree, if the callback functions are written with proper errors
or WARN's, they are the right place where most of the details'
exist. So maybe we don't need these in the iterator's after all.

Regards,
Rajendra

>
>
> - Paul

[PATCH 0/3] OMAP2+ hwmod fixes

[Rajendra Nayak]

Hi Paul,

> -----Original Message-----
> From: Paul Walmsley [mailto:paul at pwsan.com]
> Sent: Wednesday, February 23, 2011 12:40 AM
> To: Rajendra Nayak; Cousson, Benoit
> Cc: linux-omap at vger.kernel.org; linux-arm-kernel at lists.infradead.org
> Subject: RE: [PATCH 0/3] OMAP2+ hwmod fixes
>
> Hi Rajendra
>
> On Tue, 22 Feb 2011, Rajendra Nayak wrote:
>
> > The original behavior of the iterators, to terminate upon
> > encountering an error, seems fine to me. The only problem
> > I faced was that they fail silently and go undetected, unless
> > their user catches the return value and WARN's, which I found
> > was not the case with most users, mainly those of
> > omap_hwmod_for_each_by_class.
> > I was thinking of keeping the behaviour of these iterators
> > same for now and add WARN's in these iterators itself upon
> > an error, so its seen even if the user fails to catch it.
>
> What's your opinion on adding the pr_err() or WARN() into the code that
> the iterator calls for each hwmod?  That code should know why something
> fails, so it should be able to provide a more detailed error message.
Of
> course, it is not as general a solution...

I agree, if the callback functions are written with proper errors
or WARN's, they are the right place where most of the details'
exist. So maybe we don't need these in the iterator's after all.

Regards,
Rajendra

>
>
> - Paul

Re: [PATCH 0/3] OMAP2+ hwmod fixes

[Cousson, Benoit]

Hi Paul,

On 2/23/2011 11:05 AM, Nayak, Rajendra wrote:
> Hi Paul,
>
>> From: Paul Walmsley [mailto:paul@pwsan.com]
>> Sent: Wednesday, February 23, 2011 12:40 AM
>>
>> Hi Rajendra
>>
>> On Tue, 22 Feb 2011, Rajendra Nayak wrote:
>>
>>> The original behavior of the iterators, to terminate upon
>>> encountering an error, seems fine to me. The only problem
>>> I faced was that they fail silently and go undetected, unless
>>> their user catches the return value and WARN's, which I found
>>> was not the case with most users, mainly those of
>>> omap_hwmod_for_each_by_class.
>>> I was thinking of keeping the behaviour of these iterators
>>> same for now and add WARN's in these iterators itself upon
>>> an error, so its seen even if the user fails to catch it.
>>
>> What's your opinion on adding the pr_err() or WARN() into the code that
>> the iterator calls for each hwmod?  That code should know why something
>> fails, so it should be able to provide a more detailed error message.
> Of
>> course, it is not as general a solution...
>
> I agree, if the callback functions are written with proper errors
> or WARN's, they are the right place where most of the details'
> exist. So maybe we don't need these in the iterator's after all.

So to conclude, I will drop the #3 and just push #1 and #2.

#1 is fine with addition of the WARN.
#2 does return an error but does not print anything, but since each call 
(_init_main_clk, _init_interface_clks, _init_opt_clks) does report
some pr_warn in case of error, this is fine.


Regards,
Benoit

[PATCH 0/3] OMAP2+ hwmod fixes

[Cousson, Benoit]

Hi Paul,

On 2/23/2011 11:05 AM, Nayak, Rajendra wrote:
> Hi Paul,
>
>> From: Paul Walmsley [mailto:paul at pwsan.com]
>> Sent: Wednesday, February 23, 2011 12:40 AM
>>
>> Hi Rajendra
>>
>> On Tue, 22 Feb 2011, Rajendra Nayak wrote:
>>
>>> The original behavior of the iterators, to terminate upon
>>> encountering an error, seems fine to me. The only problem
>>> I faced was that they fail silently and go undetected, unless
>>> their user catches the return value and WARN's, which I found
>>> was not the case with most users, mainly those of
>>> omap_hwmod_for_each_by_class.
>>> I was thinking of keeping the behaviour of these iterators
>>> same for now and add WARN's in these iterators itself upon
>>> an error, so its seen even if the user fails to catch it.
>>
>> What's your opinion on adding the pr_err() or WARN() into the code that
>> the iterator calls for each hwmod?  That code should know why something
>> fails, so it should be able to provide a more detailed error message.
> Of
>> course, it is not as general a solution...
>
> I agree, if the callback functions are written with proper errors
> or WARN's, they are the right place where most of the details'
> exist. So maybe we don't need these in the iterator's after all.

So to conclude, I will drop the #3 and just push #1 and #2.

#1 is fine with addition of the WARN.
#2 does return an error but does not print anything, but since each call 
(_init_main_clk, _init_interface_clks, _init_opt_clks) does report
some pr_warn in case of error, this is fine.


Regards,
Benoit

Re: [PATCH 0/3] OMAP2+ hwmod fixes

[Paul Walmsley]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 536 bytes --]

Hi Benoît,

On Tue, 1 Mar 2011, Cousson, Benoit wrote:

> So to conclude, I will drop the #3 and just push #1 and #2.
> 
> #1 is fine with addition of the WARN.

We should probably drop this one now that _setup() is allowed to be called 
on hwmods that have already been setup (commit 48d54f3f).

> #2 does return an error but does not print anything, but since each call
> (_init_main_clk, _init_interface_clks, _init_opt_clks) does report
> some pr_warn in case of error, this is fine.

#2 looks fine to me.


- Paul

[PATCH 0/3] OMAP2+ hwmod fixes

[Paul Walmsley]

Hi Beno?t,

On Tue, 1 Mar 2011, Cousson, Benoit wrote:

> So to conclude, I will drop the #3 and just push #1 and #2.
> 
> #1 is fine with addition of the WARN.

We should probably drop this one now that _setup() is allowed to be called 
on hwmods that have already been setup (commit 48d54f3f).

> #2 does return an error but does not print anything, but since each call
> (_init_main_clk, _init_interface_clks, _init_opt_clks) does report
> some pr_warn in case of error, this is fine.

#2 looks fine to me.


- Paul