Re: [PATCH 4/4] OMAP: DSS2: Check for SDI HW before accessing SDI registers

[archit taneja <archit@ti.com>]

On Tuesday 01 March 2011 01:30 PM, Valkeinen, Tomi wrote:
> On Tue, 2011-03-01 at 01:46 -0600, Taneja, Archit wrote:
>> Hi,
>>
>> On Tuesday 01 March 2011 01:08 PM, Valkeinen, Tomi wrote:
>>> On Tue, 2011-03-01 at 00:49 -0600, Taneja, Archit wrote:
>>>> Hi,
>>>>
>>>> On Thursday 24 February 2011 07:04 PM, Valkeinen, Tomi wrote:
>>>>> Only OMAP 3430 hardware has SDI support. The availability of SDI HW can
>>>>> be found out by checking if the LCD channel supports SDI displays.
>>>>>
>>>>> This patch checks for SDI HW support before accessing SDI registers,
>>>>> which fixes a crash on OMAP4 when SDI SW support is compiled in.
>>>>>
>>>>> Signed-off-by: Tomi Valkeinen<tomi.valkeinen@ti.com>
>>>>> ---
>>>>>     drivers/video/omap2/dss/display.c |   10 ++++++++++
>>>>>     drivers/video/omap2/dss/dss.c     |   29 ++++++++++++++++++-----------
>>>>>     2 files changed, 28 insertions(+), 11 deletions(-)
>>>>>
>>>>> diff --git a/drivers/video/omap2/dss/display.c b/drivers/video/omap2/dss/display.c
>>>>> index 3f4fa0b..58459f4 100644
>>>>> --- a/drivers/video/omap2/dss/display.c
>>>>> +++ b/drivers/video/omap2/dss/display.c
>>>>> @@ -30,6 +30,7 @@
>>>>>
>>>>>     #include<plat/display.h>
>>>>>     #include "dss.h"
>>>>> +#include "dss_features.h"
>>>>>
>>>>>     static LIST_HEAD(display_list);
>>>>>
>>>>> @@ -392,6 +393,15 @@ void dss_init_device(struct platform_device *pdev,
>>>>>     	struct device_attribute *attr;
>>>>>     	int i;
>>>>>     	int r;
>>>>> +	enum omap_display_type supported;
>>>>> +
>>>>> +	supported = dss_feat_get_supported_displays(dssdev->channel);
>>>>> +
>>>>> +	if (!(supported&    dssdev->type)) {
>>>>> +		DSSERR("Unsupported display interface for display '%s'.\n",
>>>>> +				dssdev->name);
>>>>> +		return;
>>>>> +	}
>>>>
>>>> This would make it necessary to specify the channel in the board file,
>>>> especially digit. I think this patch should also add the channel
>>>> parameters for all board files which add a tv display.
>>>
>>> Argh. You're right, dssdev->channel is not what I thought it is. That is
>>> rather confusing. I remember this was discussed when that
>>> dssdev->channel was introduced.
>>>
>>> I believe this should work if I change dssdev->channel to
>>> dssdev->manager->id.
>>>
>>
>> I am not sure it will work, dss_init_device is called before
>> dss_recheck_connections which sets the manager for the display.
>>
>> Hence, dssdev->manager will be NULL.
>
> True.
>
>>> But this dssdev->channel has to be fixed somehow, I'm 100% sure this
>>> won't be the last time somebody tries to use it =).
>>>
>>
>> I think after the managers are set, we should somehow make the channel
>> parameter "void".
>
> Perhaps not. It is a valid piece of information, isn't it? It tells to
> which output channel this device is connected on the hardware level.
> That and display type is needed to describe how the panel is connected
> and how the DSS outputs should be set up.
>
> Looking at manager.c:omap_dss_set_device, which checks if a device can
> be attached to a manager, it only checks:
>
> 	if ((mgr->supported_displays&  dssdev->type) == 0) {
>
> Which means it doesn't work properly for OMAP4. Here we would also need
> the dssdev->channel information, wouldn't we?

We have already done the check with dssdev->channel before calling 
omap_dss_set_device in dss_recheck_connections in overlay.c, but since 
dss_recheck_connections is a one time affair, and set_device / 
unset_device can be called later on, I agree that it would be a better 
idea to move the dssdev->channel checks into omap_dss_set_device rather 
than keeping it recheck_connections.

>
> But this seems a bit confusing to me. I need to study this more.
>
> For the time being, I think I can just remove the piece of code above,
> as 1) it doesn't work right and 2) it's only needed in case the board
> file is set up wrong.
>

Okay.