Re: [PATCH 11/25] OMAPDSS: create custom pdevs for DSS omap_devices
On Friday 04 May 2012 02:30 PM, Tomi Valkeinen wrote:
On Fri, 2012-05-04 at 13:47 +0530, Archit Taneja wrote:
On Thursday 03 May 2012 07:27 PM, Tomi Valkeinen wrote:
@@ -221,22 +279,24 @@ int __init omap_display_init(struct omap_dss_board_info
*board_data)
oh_count = ARRAY_SIZE(omap4_dss_hwmod_data);
}
- for (i = 0; i< oh_count; i++) {
- oh = omap_hwmod_lookup(curr_dss_hwmod[i].oh_name);
- if (!oh) {
- pr_err("Could not look up %s\n",
- curr_dss_hwmod[i].oh_name);
- return -ENODEV;
- }
+ dss_pdev = NULL;
- pdev = omap_device_build(curr_dss_hwmod[i].dev_name,
- curr_dss_hwmod[i].id, oh,
+ for (i = 0; i< oh_count; i++) {
+ pdev = create_dss_pdev(curr_dss_hwmod[i].dev_name,
+ curr_dss_hwmod[i].id,
+ curr_dss_hwmod[i].oh_name,
NULL, 0,
- NULL, 0, 0);
+ dss_pdev);
+
+ if (IS_ERR(pdev)) {
+ pr_err("Could not build omap_device for %s\n",
+ curr_dss_hwmod[i].oh_name);
+
+ return PTR_ERR(pdev);
+ }
- if (WARN((IS_ERR(pdev)), "Could not build omap_device for %s\n",
- curr_dss_hwmod[i].oh_name))
- return -ENODEV;
+ if (i == 0)
+ dss_pdev = pdev;
The above line is a bit tricky to understand, maybe something like this
may explain the parent-child setting better:
if (!strcmp(curr_dss_hwmod[i].oh_name, "dss_core"))
dss_pdev = pdev;
I agree that it's a bit confusing. But your suggestion is not very good
either, as the code does not work properly if the dss_core is not the
first one created. I'll look into it. Perhaps I can separate the code
into a small function, and then I can more easily do something like:
Right, my suggestion wont work either.
dss_pdev = create_the_device();
for () {
// create the rest of the devices
create_the_device();
}
and that would clarify what's going on.
Yes, or you could just add a comment saying i == 0 is the dss_core
hwmod, and we make sure dss_core is the first one on the list.
I had another general question about the parent-child series. What is
the use of the platform device omap_display_device (with the name
"omapdss"). Is it just a way to get the board data?
Originally, before hwmods, we had only omapdss device, which contained
all the dss code. Then came hwmods, and the omapdss was split into
smaller devices, but omapdss was still there.
As I see it, omapdss is currently a "virtual" higher level device
(virtual in the sense that it doesn't correspond directly to any HW),
and the code for omapdss is more or less in the core.c file. It's used
to pass the board data, but also has some generic dss stuff that all dss
subdevices can use.
I think in the long run we should remove omapdss device, and probably
handle the generic stuff in dss_core (dss.c), which, as a parent to
other subdevices, should fit fine for the role.
For the time being we can't remove it. It's the only simple way to pass
callbacks from the arch code with device tree.
Okay, makes sense now.
Thanks,
Archit
Tomi
--
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 11/25] OMAPDSS: create custom pdevs for DSS omap_devices
On Fri, 2012-05-04 at 13:47 +0530, Archit Taneja wrote:
> On Thursday 03 May 2012 07:27 PM, Tomi Valkeinen wrote:
> > @@ -221,22 +279,24 @@ int __init omap_display_init(struct
> > omap_dss_board_info *board_data)
> > oh_count = ARRAY_SIZE(omap4_dss_hwmod_data);
> > }
> >
> > - for (i = 0; i< oh_count; i++) {
> > - oh = omap_hwmod_lookup(curr_dss_hwmod[i].oh_name);
> > - if (!oh) {
> > - pr_err("Could not look up %s\n",
> > - curr_dss_hwmod[i].oh_name);
> > - return -ENODEV;
> > - }
> > + dss_pdev = NULL;
> >
> > - pdev = omap_device_build(curr_dss_hwmod[i].dev_name,
> > - curr_dss_hwmod[i].id, oh,
> > + for (i = 0; i< oh_count; i++) {
> > + pdev = create_dss_pdev(curr_dss_hwmod[i].dev_name,
> > + curr_dss_hwmod[i].id,
> > + curr_dss_hwmod[i].oh_name,
> > NULL, 0,
> > - NULL, 0, 0);
> > + dss_pdev);
> > +
> > + if (IS_ERR(pdev)) {
> > + pr_err("Could not build omap_device for %s\n",
> > + curr_dss_hwmod[i].oh_name);
> > +
> > + return PTR_ERR(pdev);
> > + }
> >
> > - if (WARN((IS_ERR(pdev)), "Could not build omap_device for %s\n",
> > - curr_dss_hwmod[i].oh_name))
> > - return -ENODEV;
> > + if (i == 0)
> > + dss_pdev = pdev;
>
> The above line is a bit tricky to understand, maybe something like this
> may explain the parent-child setting better:
>
> if (!strcmp(curr_dss_hwmod[i].oh_name, "dss_core"))
> dss_pdev = pdev;
I agree that it's a bit confusing. But your suggestion is not very good
either, as the code does not work properly if the dss_core is not the
first one created. I'll look into it. Perhaps I can separate the code
into a small function, and then I can more easily do something like:
dss_pdev = create_the_device();
for () {
// create the rest of the devices
create_the_device();
}
and that would clarify what's going on.
> I had another general question about the parent-child series. What is
> the use of the platform device omap_display_device (with the name
> "omapdss"). Is it just a way to get the board data?
Originally, before hwmods, we had only omapdss device, which contained
all the dss code. Then came hwmods, and the omapdss was split into
smaller devices, but omapdss was still there.
As I see it, omapdss is currently a "virtual" higher level device
(virtual in the sense that it doesn't correspond directly to any HW),
and the code for omapdss is more or less in the core.c file. It's used
to pass the board data, but also has some generic dss stuff that all dss
subdevices can use.
I think in the long run we should remove omapdss device, and probably
handle the generic stuff in dss_core (dss.c), which, as a parent to
other subdevices, should fit fine for the role.
For the time being we can't remove it. It's the only simple way to pass
callbacks from the arch code with device tree.
Tomi
signature.asc
Description: This is a digitally signed message part
Re: [PATCH 11/25] OMAPDSS: create custom pdevs for DSS omap_devices
On Fri, 2012-05-04 at 11:33 +0530, Archit Taneja wrote: > Hi, > > On Thursday 03 May 2012 07:27 PM, Tomi Valkeinen wrote: > > Instead of using omap_device_build() to create the omap_devices for DSS > > hwmods, create them with a custom function. This will allow us to create > > a parent-child hierarchy for the devices so that the omapdss_core device > > is parent for the rest of the dss hwmod devices. > > > > Signed-off-by: Tomi Valkeinen > > --- > > arch/arm/mach-omap2/display.c | 88 > > ++--- > > 1 file changed, 74 insertions(+), 14 deletions(-) > > > > diff --git a/arch/arm/mach-omap2/display.c b/arch/arm/mach-omap2/display.c > > index 07232fd..46d2a98 100644 > > --- a/arch/arm/mach-omap2/display.c > > +++ b/arch/arm/mach-omap2/display.c > > @@ -185,13 +185,71 @@ static int omap_dss_set_min_bus_tput(struct device > > *dev, unsigned long tput) > > return omap_pm_set_min_bus_tput(dev, OCP_INITIATOR_AGENT, tput); > > } > > > > +static struct platform_device *create_dss_pdev(const char *pdev_name, > > + int pdev_id, const char *oh_name, void *pdata, int pdata_len, > > + struct platform_device *parent) > > This function looks quite generic, it's like omap_device_build() but > with a parent associated. omap_device_build() seems to be a special case > of this function with parent passed as null. Won't this > function be needed by other devices too? > > Maybe we could modify omap_device_build_ss() to take a parent argument, > and make a function called omap_device_build_parent(), where both > omap_device_build() and omap_device_build_parent() call > omap_device_build_ss()? Yes, that sounds good to me. Paul, Kevin, what do you think, could the omap_device functions be extended to allow setting a parent device? Tomi signature.asc Description: This is a digitally signed message part
Re: [PATCH 11/25] OMAPDSS: create custom pdevs for DSS omap_devices
On Thursday 03 May 2012 07:27 PM, Tomi Valkeinen wrote:
Instead of using omap_device_build() to create the omap_devices for DSS
hwmods, create them with a custom function. This will allow us to create
a parent-child hierarchy for the devices so that the omapdss_core device
is parent for the rest of the dss hwmod devices.
Signed-off-by: Tomi Valkeinen
---
arch/arm/mach-omap2/display.c | 88 ++---
1 file changed, 74 insertions(+), 14 deletions(-)
diff --git a/arch/arm/mach-omap2/display.c b/arch/arm/mach-omap2/display.c
index 07232fd..46d2a98 100644
--- a/arch/arm/mach-omap2/display.c
+++ b/arch/arm/mach-omap2/display.c
@@ -185,13 +185,71 @@ static int omap_dss_set_min_bus_tput(struct device *dev,
unsigned long tput)
return omap_pm_set_min_bus_tput(dev, OCP_INITIATOR_AGENT, tput);
}
+static struct platform_device *create_dss_pdev(const char *pdev_name,
+ int pdev_id, const char *oh_name, void *pdata, int pdata_len,
+ struct platform_device *parent)
+{
+ struct platform_device *pdev;
+ struct omap_device *od;
+ struct omap_hwmod *ohs[1];
+ struct omap_hwmod *oh;
+ int r;
+
+ oh = omap_hwmod_lookup(oh_name);
+ if (!oh) {
+ pr_err("Could not look up %s\n", oh_name);
+ r = -ENODEV;
+ goto err;
+ }
+
+ pdev = platform_device_alloc(pdev_name, pdev_id);
+ if (!pdev) {
+ pr_err("Could not create pdev for %s\n", pdev_name);
+ r = -ENOMEM;
+ goto err;
+ }
+
+ if (parent != NULL)
+ pdev->dev.parent =&parent->dev;
+
+ if (pdev->id != -1)
+ dev_set_name(&pdev->dev, "%s.%d", pdev->name, pdev->id);
+ else
+ dev_set_name(&pdev->dev, "%s", pdev->name);
+
+ ohs[0] = oh;
+ od = omap_device_alloc(pdev, ohs, 1, NULL, 0);
+ if (!od) {
+ pr_err("Could not alloc omap_device for %s\n", pdev_name);
+ r = -ENOMEM;
+ goto err;
+ }
+
+ r = platform_device_add_data(pdev, pdata, pdata_len);
+ if (r) {
+ pr_err("Could not set pdata for %s\n", pdev_name);
+ goto err;
+ }
+
+ r = omap_device_register(pdev);
+ if (r) {
+ pr_err("Could not register omap_device for %s\n", pdev_name);
+ goto err;
+ }
+
+ return pdev;
+
+err:
+ return ERR_PTR(r);
+}
+
int __init omap_display_init(struct omap_dss_board_info *board_data)
{
int r = 0;
- struct omap_hwmod *oh;
struct platform_device *pdev;
int i, oh_count;
const struct omap_dss_hwmod_data *curr_dss_hwmod;
+ struct platform_device *dss_pdev;
/* create omapdss device */
@@ -221,22 +279,24 @@ int __init omap_display_init(struct omap_dss_board_info
*board_data)
oh_count = ARRAY_SIZE(omap4_dss_hwmod_data);
}
- for (i = 0; i< oh_count; i++) {
- oh = omap_hwmod_lookup(curr_dss_hwmod[i].oh_name);
- if (!oh) {
- pr_err("Could not look up %s\n",
- curr_dss_hwmod[i].oh_name);
- return -ENODEV;
- }
+ dss_pdev = NULL;
- pdev = omap_device_build(curr_dss_hwmod[i].dev_name,
- curr_dss_hwmod[i].id, oh,
+ for (i = 0; i< oh_count; i++) {
+ pdev = create_dss_pdev(curr_dss_hwmod[i].dev_name,
+ curr_dss_hwmod[i].id,
+ curr_dss_hwmod[i].oh_name,
NULL, 0,
- NULL, 0, 0);
+ dss_pdev);
+
+ if (IS_ERR(pdev)) {
+ pr_err("Could not build omap_device for %s\n",
+ curr_dss_hwmod[i].oh_name);
+
+ return PTR_ERR(pdev);
+ }
- if (WARN((IS_ERR(pdev)), "Could not build omap_device for %s\n",
- curr_dss_hwmod[i].oh_name))
- return -ENODEV;
+ if (i == 0)
+ dss_pdev = pdev;
The above line is a bit tricky to understand, maybe something like this
may explain the parent-child setting better:
if (!strcmp(curr_dss_hwmod[i].oh_name, "dss_core"))
dss_pdev = pdev;
I had another general question about the parent-child series. What is
the use of the platform device omap_display_device (with the name
"omapdss"). Is it just a way to get the board data?
Archit
}
return 0;
--
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 11/25] OMAPDSS: create custom pdevs for DSS omap_devices
Hi,
On Thursday 03 May 2012 07:27 PM, Tomi Valkeinen wrote:
Instead of using omap_device_build() to create the omap_devices for DSS
hwmods, create them with a custom function. This will allow us to create
a parent-child hierarchy for the devices so that the omapdss_core device
is parent for the rest of the dss hwmod devices.
Signed-off-by: Tomi Valkeinen
---
arch/arm/mach-omap2/display.c | 88 ++---
1 file changed, 74 insertions(+), 14 deletions(-)
diff --git a/arch/arm/mach-omap2/display.c b/arch/arm/mach-omap2/display.c
index 07232fd..46d2a98 100644
--- a/arch/arm/mach-omap2/display.c
+++ b/arch/arm/mach-omap2/display.c
@@ -185,13 +185,71 @@ static int omap_dss_set_min_bus_tput(struct device *dev,
unsigned long tput)
return omap_pm_set_min_bus_tput(dev, OCP_INITIATOR_AGENT, tput);
}
+static struct platform_device *create_dss_pdev(const char *pdev_name,
+ int pdev_id, const char *oh_name, void *pdata, int pdata_len,
+ struct platform_device *parent)
This function looks quite generic, it's like omap_device_build() but
with a parent associated. omap_device_build() seems to be a special case
of this function with parent passed as null. Won't this
function be needed by other devices too?
Maybe we could modify omap_device_build_ss() to take a parent argument,
and make a function called omap_device_build_parent(), where both
omap_device_build() and omap_device_build_parent() call
omap_device_build_ss()?
Archit
+{
+ struct platform_device *pdev;
+ struct omap_device *od;
+ struct omap_hwmod *ohs[1];
+ struct omap_hwmod *oh;
+ int r;
+
+ oh = omap_hwmod_lookup(oh_name);
+ if (!oh) {
+ pr_err("Could not look up %s\n", oh_name);
+ r = -ENODEV;
+ goto err;
+ }
+
+ pdev = platform_device_alloc(pdev_name, pdev_id);
+ if (!pdev) {
+ pr_err("Could not create pdev for %s\n", pdev_name);
+ r = -ENOMEM;
+ goto err;
+ }
+
+ if (parent != NULL)
+ pdev->dev.parent =&parent->dev;
+
+ if (pdev->id != -1)
+ dev_set_name(&pdev->dev, "%s.%d", pdev->name, pdev->id);
+ else
+ dev_set_name(&pdev->dev, "%s", pdev->name);
+
+ ohs[0] = oh;
+ od = omap_device_alloc(pdev, ohs, 1, NULL, 0);
+ if (!od) {
+ pr_err("Could not alloc omap_device for %s\n", pdev_name);
+ r = -ENOMEM;
+ goto err;
+ }
+
+ r = platform_device_add_data(pdev, pdata, pdata_len);
+ if (r) {
+ pr_err("Could not set pdata for %s\n", pdev_name);
+ goto err;
+ }
+
+ r = omap_device_register(pdev);
+ if (r) {
+ pr_err("Could not register omap_device for %s\n", pdev_name);
+ goto err;
+ }
+
+ return pdev;
+
+err:
+ return ERR_PTR(r);
+}
+
int __init omap_display_init(struct omap_dss_board_info *board_data)
{
int r = 0;
- struct omap_hwmod *oh;
struct platform_device *pdev;
int i, oh_count;
const struct omap_dss_hwmod_data *curr_dss_hwmod;
+ struct platform_device *dss_pdev;
/* create omapdss device */
@@ -221,22 +279,24 @@ int __init omap_display_init(struct omap_dss_board_info
*board_data)
oh_count = ARRAY_SIZE(omap4_dss_hwmod_data);
}
- for (i = 0; i< oh_count; i++) {
- oh = omap_hwmod_lookup(curr_dss_hwmod[i].oh_name);
- if (!oh) {
- pr_err("Could not look up %s\n",
- curr_dss_hwmod[i].oh_name);
- return -ENODEV;
- }
+ dss_pdev = NULL;
- pdev = omap_device_build(curr_dss_hwmod[i].dev_name,
- curr_dss_hwmod[i].id, oh,
+ for (i = 0; i< oh_count; i++) {
+ pdev = create_dss_pdev(curr_dss_hwmod[i].dev_name,
+ curr_dss_hwmod[i].id,
+ curr_dss_hwmod[i].oh_name,
NULL, 0,
- NULL, 0, 0);
+ dss_pdev);
+
+ if (IS_ERR(pdev)) {
+ pr_err("Could not build omap_device for %s\n",
+ curr_dss_hwmod[i].oh_name);
+
+ return PTR_ERR(pdev);
+ }
- if (WARN((IS_ERR(pdev)), "Could not build omap_device for %s\n",
- curr_dss_hwmod[i].oh_name))
- return -ENODEV;
+ if (i == 0)
+ dss_pdev = pdev;
}
return 0;
--
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/maj
