Re: [PATCH 11/25] OMAPDSS: create custom pdevs for DSS omap_devices

[Tomi Valkeinen <tomi.valkeinen@ti.com>]

[-- Attachment #1: Type: text/plain, Size: 2912 bytes --]

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


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]