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

[Archit Taneja <a0393947@ti.com>]

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
>