Re: [RFC: PATCH] Fix to support multiple HWMODS for a single device
Thanks Benoit for your review comments.
On Monday 23 August 2010 05:31 PM, Datta, Shubhrajyoti wrote:
-Original Message-
From: linux-omap-ow...@vger.kernel.org [mailto:linux-omap-
ow...@vger.kernel.org] On Behalf Of Cousson, Benoit
Sent: Monday, August 23, 2010 5:20 PM
To: ABRAHAM, KISHON VIJAY
Cc: linux-omap@vger.kernel.org; khil...@deeprootsystems.com;
p...@pwsan.com
Subject: Re: [RFC: PATCH] Fix to support multiple HWMODS for a single
device
Hi Vijay,
On 8/23/2010 12:46 PM, ABRAHAM, KISHON VIJAY wrote:
The current HWMOD code expects multiple HWMODS to be filled in
consecutive
memory location before passing to omap_device_build_ss().
Just a minor comment: this is not the "The current HWMOD code" but the
omap_device core code.
Ignoring this
will result in incorrect HWMOD data being extracted. This means before
calling
omap_device_build_ss() the user has to create memory chunks, copy all
the HWMOD
structures to it taking care of the mutex.
I don't think it was an expectation, this is simply a bug :-)
So copying hwmod structures before calling this API is clearly not the
right way to do fix that...
This fix uses the pointer to pointer to OMAP_HWMOD structure passed to
omap_device_build_ss() to correctly extract the appropriate
OMAP_HWMOD structure.
Yes, this is the proper way of fixing that.
This patch is created on top of origin/pm-wip/hwmods-omap4.
You can do it on the mainline too, there is no dependency with lo/master
or pm-wip for that one. That fix will be able to go to mainline faster.
Signed-off-by: Kishon Vijay Abraham I
---
arch/arm/plat-omap/omap_device.c | 42 +++---
1 files changed, 21 insertions(+), 21 deletions(-)
diff --git a/arch/arm/plat-omap/omap_device.c b/arch/arm/plat-
omap/omap_device.c
index d2b1609..e94bd7a 100644
--- a/arch/arm/plat-omap/omap_device.c
+++ b/arch/arm/plat-omap/omap_device.c
@@ -257,12 +257,12 @@ static inline struct omap_device
*_find_by_pdev(struct platform_device *pdev)
*/
int omap_device_count_resources(struct omap_device *od)
{
- struct omap_hwmod *oh;
+ struct omap_hwmod **oh;
int c = 0;
int i;
- for (i = 0, oh = *od->hwmods; i< od->hwmods_cnt; i++, oh++)
- c += omap_hwmod_count_resources(oh);
+ for (i = 0, oh = od->hwmods; i< od->hwmods_cnt; i++, oh++)
+ c += omap_hwmod_count_resources(*oh);
In that case, you might prefer using array type of access in order to
make the code more readable (i.e. oh[i]). Or even avoid the oh variable.
+c += omap_hwmod_count_resources(od->hwmods[i]);
Just for my information, what kind of device are you working on that
require multiple hwmods?
The McBSP qualifies for 2 hwmods and 1 device as OMAP3 has sidetone support
also.
Otherwise, this is a good catch. Thanks for fixing that.
Regards,
Benoit
--
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: [RFC: PATCH] Fix to support multiple HWMODS for a single device
> -Original Message-
> From: linux-omap-ow...@vger.kernel.org [mailto:linux-omap-
> ow...@vger.kernel.org] On Behalf Of Cousson, Benoit
> Sent: Monday, August 23, 2010 5:20 PM
> To: ABRAHAM, KISHON VIJAY
> Cc: linux-omap@vger.kernel.org; khil...@deeprootsystems.com;
> p...@pwsan.com
> Subject: Re: [RFC: PATCH] Fix to support multiple HWMODS for a single
> device
>
> Hi Vijay,
>
> On 8/23/2010 12:46 PM, ABRAHAM, KISHON VIJAY wrote:
> > The current HWMOD code expects multiple HWMODS to be filled in
> consecutive
> > memory location before passing to omap_device_build_ss().
>
> Just a minor comment: this is not the "The current HWMOD code" but the
> omap_device core code.
>
> > Ignoring this
> > will result in incorrect HWMOD data being extracted. This means before
> calling
> > omap_device_build_ss() the user has to create memory chunks, copy all
> the HWMOD
> > structures to it taking care of the mutex.
>
> I don't think it was an expectation, this is simply a bug :-)
> So copying hwmod structures before calling this API is clearly not the
> right way to do fix that...
>
> > This fix uses the pointer to pointer to OMAP_HWMOD structure passed to
> > omap_device_build_ss() to correctly extract the appropriate
> > OMAP_HWMOD structure.
>
> Yes, this is the proper way of fixing that.
>
> > This patch is created on top of origin/pm-wip/hwmods-omap4.
>
> You can do it on the mainline too, there is no dependency with lo/master
> or pm-wip for that one. That fix will be able to go to mainline faster.
>
> >
> > Signed-off-by: Kishon Vijay Abraham I
> > ---
> > arch/arm/plat-omap/omap_device.c | 42 +++---
>
> > 1 files changed, 21 insertions(+), 21 deletions(-)
> >
> > diff --git a/arch/arm/plat-omap/omap_device.c b/arch/arm/plat-
> omap/omap_device.c
> > index d2b1609..e94bd7a 100644
> > --- a/arch/arm/plat-omap/omap_device.c
> > +++ b/arch/arm/plat-omap/omap_device.c
> > @@ -257,12 +257,12 @@ static inline struct omap_device
> *_find_by_pdev(struct platform_device *pdev)
> >*/
> > int omap_device_count_resources(struct omap_device *od)
> > {
> > - struct omap_hwmod *oh;
> > + struct omap_hwmod **oh;
> > int c = 0;
> > int i;
> >
> > - for (i = 0, oh = *od->hwmods; i< od->hwmods_cnt; i++, oh++)
> > - c += omap_hwmod_count_resources(oh);
> > + for (i = 0, oh = od->hwmods; i< od->hwmods_cnt; i++, oh++)
> > + c += omap_hwmod_count_resources(*oh);
>
> In that case, you might prefer using array type of access in order to
> make the code more readable (i.e. oh[i]). Or even avoid the oh variable.
>
> + c += omap_hwmod_count_resources(od->hwmods[i]);
>
> Just for my information, what kind of device are you working on that
> require multiple hwmods?
The McBSP qualifies for 2 hwmods and 1 device as OMAP3 has sidetone support
also.
>
> Otherwise, this is a good catch. Thanks for fixing that.
>
> Regards,
> Benoit
>
>
> --
> 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: [RFC: PATCH] Fix to support multiple HWMODS for a single device
Hi Vijay,
On 8/23/2010 12:46 PM, ABRAHAM, KISHON VIJAY wrote:
The current HWMOD code expects multiple HWMODS to be filled in consecutive
memory location before passing to omap_device_build_ss().
Just a minor comment: this is not the "The current HWMOD code" but the
omap_device core code.
Ignoring this
will result in incorrect HWMOD data being extracted. This means before calling
omap_device_build_ss() the user has to create memory chunks, copy all the HWMOD
structures to it taking care of the mutex.
I don't think it was an expectation, this is simply a bug :-)
So copying hwmod structures before calling this API is clearly not the
right way to do fix that...
This fix uses the pointer to pointer to OMAP_HWMOD structure passed to
omap_device_build_ss() to correctly extract the appropriate
OMAP_HWMOD structure.
Yes, this is the proper way of fixing that.
This patch is created on top of origin/pm-wip/hwmods-omap4.
You can do it on the mainline too, there is no dependency with lo/master
or pm-wip for that one. That fix will be able to go to mainline faster.
Signed-off-by: Kishon Vijay Abraham I
---
arch/arm/plat-omap/omap_device.c | 42 +++---
1 files changed, 21 insertions(+), 21 deletions(-)
diff --git a/arch/arm/plat-omap/omap_device.c b/arch/arm/plat-omap/omap_device.c
index d2b1609..e94bd7a 100644
--- a/arch/arm/plat-omap/omap_device.c
+++ b/arch/arm/plat-omap/omap_device.c
@@ -257,12 +257,12 @@ static inline struct omap_device *_find_by_pdev(struct
platform_device *pdev)
*/
int omap_device_count_resources(struct omap_device *od)
{
- struct omap_hwmod *oh;
+ struct omap_hwmod **oh;
int c = 0;
int i;
- for (i = 0, oh = *od->hwmods; i< od->hwmods_cnt; i++, oh++)
- c += omap_hwmod_count_resources(oh);
+ for (i = 0, oh = od->hwmods; i< od->hwmods_cnt; i++, oh++)
+ c += omap_hwmod_count_resources(*oh);
In that case, you might prefer using array type of access in order to
make the code more readable (i.e. oh[i]). Or even avoid the oh variable.
+ c += omap_hwmod_count_resources(od->hwmods[i]);
Just for my information, what kind of device are you working on that
require multiple hwmods?
Otherwise, this is a good catch. Thanks for fixing that.
Regards,
Benoit
--
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
[RFC: PATCH] Fix to support multiple HWMODS for a single device
The current HWMOD code expects multiple HWMODS to be filled in consecutive
memory location before passing to omap_device_build_ss(). Ignoring this
will result in incorrect HWMOD data being extracted. This means before calling
omap_device_build_ss() the user has to create memory chunks, copy all the HWMOD
structures to it taking care of the mutex.
This fix uses the pointer to pointer to OMAP_HWMOD structure passed to
omap_device_build_ss() to correctly extract the appropriate
OMAP_HWMOD structure.
This patch is created on top of origin/pm-wip/hwmods-omap4.
Signed-off-by: Kishon Vijay Abraham I
---
arch/arm/plat-omap/omap_device.c | 42 +++---
1 files changed, 21 insertions(+), 21 deletions(-)
diff --git a/arch/arm/plat-omap/omap_device.c b/arch/arm/plat-omap/omap_device.c
index d2b1609..e94bd7a 100644
--- a/arch/arm/plat-omap/omap_device.c
+++ b/arch/arm/plat-omap/omap_device.c
@@ -257,12 +257,12 @@ static inline struct omap_device *_find_by_pdev(struct
platform_device *pdev)
*/
int omap_device_count_resources(struct omap_device *od)
{
- struct omap_hwmod *oh;
+ struct omap_hwmod **oh;
int c = 0;
int i;
- for (i = 0, oh = *od->hwmods; i < od->hwmods_cnt; i++, oh++)
- c += omap_hwmod_count_resources(oh);
+ for (i = 0, oh = od->hwmods; i < od->hwmods_cnt; i++, oh++)
+ c += omap_hwmod_count_resources(*oh);
pr_debug("omap_device: %s: counted %d total resources across %d "
"hwmods\n", od->pdev.name, c, od->hwmods_cnt);
@@ -289,12 +289,12 @@ int omap_device_count_resources(struct omap_device *od)
*/
int omap_device_fill_resources(struct omap_device *od, struct resource *res)
{
- struct omap_hwmod *oh;
+ struct omap_hwmod **oh;
int c = 0;
int i, r;
- for (i = 0, oh = *od->hwmods; i < od->hwmods_cnt; i++, oh++) {
- r = omap_hwmod_fill_resources(oh, res);
+ for (i = 0, oh = od->hwmods; i < od->hwmods_cnt; i++, oh++) {
+ r = omap_hwmod_fill_resources(*oh, res);
res += r;
c += r;
}
@@ -566,7 +566,7 @@ int omap_device_shutdown(struct platform_device *pdev)
{
int ret, i;
struct omap_device *od;
- struct omap_hwmod *oh;
+ struct omap_hwmod **oh;
od = _find_by_pdev(pdev);
@@ -579,8 +579,8 @@ int omap_device_shutdown(struct platform_device *pdev)
ret = _omap_device_deactivate(od, IGNORE_WAKEUP_LAT);
- for (i = 0, oh = *od->hwmods; i < od->hwmods_cnt; i++, oh++)
- omap_hwmod_shutdown(oh);
+ for (i = 0, oh = od->hwmods; i < od->hwmods_cnt; i++, oh++)
+ omap_hwmod_shutdown(*oh);
od->_state = OMAP_DEVICE_STATE_SHUTDOWN;
@@ -692,11 +692,11 @@ void __iomem *omap_device_get_rt_va(struct omap_device
*od)
*/
int omap_device_enable_hwmods(struct omap_device *od)
{
- struct omap_hwmod *oh;
+ struct omap_hwmod **oh;
int i;
- for (i = 0, oh = *od->hwmods; i < od->hwmods_cnt; i++, oh++)
- omap_hwmod_enable(oh);
+ for (i = 0, oh = od->hwmods; i < od->hwmods_cnt; i++, oh++)
+ omap_hwmod_enable(*oh);
/* XXX pass along return value here? */
return 0;
@@ -710,11 +710,11 @@ int omap_device_enable_hwmods(struct omap_device *od)
*/
int omap_device_idle_hwmods(struct omap_device *od)
{
- struct omap_hwmod *oh;
+ struct omap_hwmod **oh;
int i;
- for (i = 0, oh = *od->hwmods; i < od->hwmods_cnt; i++, oh++)
- omap_hwmod_idle(oh);
+ for (i = 0, oh = od->hwmods; i < od->hwmods_cnt; i++, oh++)
+ omap_hwmod_idle(*oh);
/* XXX pass along return value here? */
return 0;
@@ -729,11 +729,11 @@ int omap_device_idle_hwmods(struct omap_device *od)
*/
int omap_device_disable_clocks(struct omap_device *od)
{
- struct omap_hwmod *oh;
+ struct omap_hwmod **oh;
int i;
- for (i = 0, oh = *od->hwmods; i < od->hwmods_cnt; i++, oh++)
- omap_hwmod_disable_clocks(oh);
+ for (i = 0, oh = od->hwmods; i < od->hwmods_cnt; i++, oh++)
+ omap_hwmod_disable_clocks(*oh);
/* XXX pass along return value here? */
return 0;
@@ -748,11 +748,11 @@ int omap_device_disable_clocks(struct omap_device *od)
*/
int omap_device_enable_clocks(struct omap_device *od)
{
- struct omap_hwmod *oh;
+ struct omap_hwmod **oh;
int i;
- for (i = 0, oh = *od->hwmods; i < od->hwmods_cnt; i++, oh++)
- omap_hwmod_enable_clocks(oh);
+ for (i = 0, oh = od->hwmods; i < od->hwmods_cnt; i++, oh++)
+ omap_hwmod_enable_clocks(*oh);
/* XXX pass along return value here? */
return 0;
--
1.7.0.4
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majord
[RFC/PATCH] Fix to support multiple HWMODS for a single device
>From 24ffaa67c44065a9caa3ee3525dd863d32c67269 Mon Sep 17 00:00:00 2001
From: Kishon Vijay Abraham I
Date: Sun, 22 Aug 2010 15:06:42 +0530
Subject: [RFC/PATCH] Fix to support multiple HWMODS for a single device
The current code expects multiple HWMODS to be filled in consecutive
memory location before passing to omap_device_build_ss(). Ignoring this
will result in incorrect data being populated in omap_hwmod_fill_resources().
This means before calling omap_device_build_ss() the user has to create memory
chunks, copy all the HWMOD structures to it taking care of the mutex
variable as well.
This fix does some pointer calculations to the existing function
omap_device_fill_resources() in order to fill the resources properly.
Signed-off-by: Kishon Vijay Abraham I
---
arch/arm/plat-omap/omap_device.c |7 +++
1 files changed, 3 insertions(+), 4 deletions(-)
mode change 100644 => 100755 arch/arm/plat-omap/omap_device.c
diff --git a/arch/arm/plat-omap/omap_device.c b/arch/arm/plat-omap/omap_device.c
old mode 100644
new mode 100755
index d2b1609..8013292
--- a/arch/arm/plat-omap/omap_device.c
+++ b/arch/arm/plat-omap/omap_device.c
@@ -289,12 +289,12 @@ int omap_device_count_resources(struct omap_device *od)
*/
int omap_device_fill_resources(struct omap_device *od, struct resource *res)
{
- struct omap_hwmod *oh;
+ struct omap_hwmod **oh;
int c = 0;
int i, r;
- for (i = 0, oh = *od->hwmods; i < od->hwmods_cnt; i++, oh++) {
- r = omap_hwmod_fill_resources(oh, res);
+ for (i = 0, oh = od->hwmods; i < od->hwmods_cnt; i++, oh++) {
+ r = omap_hwmod_fill_resources(*oh, res);
res += r;
c += r;
}
@@ -694,7 +694,6 @@ int omap_device_enable_hwmods(struct omap_device *od)
{
struct omap_hwmod *oh;
int i;
-
for (i = 0, oh = *od->hwmods; i < od->hwmods_cnt; i++, oh++)
omap_hwmod_enable(oh);
--
1.7.0.4
