Re: [PATCH 1/3] OMAP: DSS2: Functions to request/release DSI VCs

[archit taneja <archit@ti.com>]

Hi,

On Monday 28 February 2011 06:21 PM, Valkeinen, Tomi wrote:
> On Mon, 2011-02-28 at 02:47 -0600, Taneja, Archit wrote:
>> Introduce functions which request and release VC's. This will be used in panel
>> drivers in their probes.
>>
>> omap_dsi_request_vc() takes in the pointer to the omap_dss_device, the VC_ID
>> parameter which goes into the header of the DSI packets, and returns a Virtual
>> channel number (or virtual channel register set) which it can use.
>>
>> omap_dsi_releae_vc() takes the omap_dss_device pointer and frees all VCs which
>> were used by that device.
>>
>> Initialisation of VC parameters is done in dsi_init().
>>
>> Signed-off-by: Archit Taneja<archit@ti.com>
>> ---
>>   arch/arm/plat-omap/include/plat/display.h |    3 ++
>>   drivers/video/omap2/dss/dsi.c             |   55 ++++++++++++++++++++++++++---
>>   2 files changed, 53 insertions(+), 5 deletions(-)
>>
>> diff --git a/arch/arm/plat-omap/include/plat/display.h b/arch/arm/plat-omap/include/plat/display.h
>> index d45f107..0057259 100644
>> --- a/arch/arm/plat-omap/include/plat/display.h
>> +++ b/arch/arm/plat-omap/include/plat/display.h
>> @@ -560,6 +560,9 @@ int omap_dsi_update(struct omap_dss_device *dssdev,
>>   		int channel,
>>   		u16 x, u16 y, u16 w, u16 h,
>>   		void (*callback)(int, void *), void *data);
>> +int omap_dsi_request_vc(struct omap_dss_device *dssdev, int vc_id,
>> +		int *channel);
>> +void omap_dsi_release_vc(struct omap_dss_device *dssdev);
>
> The release function is misnamed. It's actually release_all_vcs.
> However, I think the request and release functions should match:
> request/release one vc.
>
> So perhaps something like:
>
> void omap_dsi_release_vc(struct omap_dss_device *dssdev, int channel);
>
> dssdev is not really needed there, but can be used for checking validity
> of the release call.

Yes, this is better.

>
> Also, about the returned channel value. It is something that the panel
> driver should not use for anything else than to pass to DSI functions.
> Perhaps this would be a good case for typedeffing (see CodingStyle,
> chapter 5, part a). dsi_channel_t? Well, we can think about this later.
>
> In the future we could possibly encode the DSI module number in the
> channel also.
>

That's a good idea.


<snip>

>>   }
>>
>> +int omap_dsi_request_vc(struct omap_dss_device *dssdev, int vc_id, int *channel)
>> +{
>> +	int p = dsi.num_vc_used;
>> +
>> +	if (p>= 4) {
>> +		DSSERR("cannot get VC for display %s", dssdev->name);
>> +		return -EINVAL;
>> +	}
>> +
>> +	dsi.vc[p].dssdev = dssdev;
>> +	dsi.vc[p].vc_id = vc_id;
>> +	*channel = p;
>> +
>> +	dsi.num_vc_used += 1;
>> +
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL(omap_dsi_request_vc);
>> +
>> +void omap_dsi_release_vc(struct omap_dss_device *dssdev)
>> +{
>> +	int i, num_vc_used_disp = 0;
>> +
>> +	for (i = 0; i<  4; i++) {
>> +		if (dsi.vc[i].dssdev == dssdev) {
>> +			dsi.vc[i].dssdev = NULL;
>> +			dsi.vc[i].vc_id = 0;
>> +			dsi.vc[i].mode = DSI_VC_MODE_L4;
>> +			num_vc_used_disp++;
>> +		}
>> +	}
>> +
>> +	dsi.num_vc_used -= num_vc_used_disp;
>> +}
>> +EXPORT_SYMBOL(omap_dsi_release_vc);
>
> I don't think these work quite right. What happens if there are two
> panel drivers, both have allocated one channel. Then the first channel
> is released. At this point num_vc_used is 1. Next request would allocate
> channel number 1, which is still in use.
>

This is quite silly of me. I think I made this thinking that the 
sequence of releases by panels will happen in opposite order to sequence 
of requests. But that won't be the case at all when the panels are 
inserted as modules

> I think the code will be cleaner and simpler if you removed num_vc_used,
> and loop through the 4 channels when allocating, trying to find a free
> one. It's just 4 iterations.
>

I will make this change.

Archit