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

[archit taneja <archit@ti.com>]

Hi,

On Tuesday 01 March 2011 06:50 PM, DebBarma, Tarun Kanti wrote:
>> -----Original Message-----
>> From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-
>> owner@vger.kernel.org] On Behalf Of Taneja, Archit
>> Sent: Tuesday, March 01, 2011 6:02 PM
>> To: Valkeinen, Tomi
>> Cc: linux-omap@vger.kernel.org; Taneja, Archit
>> Subject: [PATCH 1/3] OMAP: DSS2: Functions to request/release DSI VCs
>>
>> 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_set_vc_id() takes the omap_dss_device pointer, the Virtual
>> Channel
>> number and the VC_ID that needs to be set for the specifed Virtual
>> Channel.
>>
>> omap_dsi_releae_vc() takes the omap_dss_device pointer and the Virtual
>> Channel
>> number that needs to be made free.
>>
>> Initialisation of VC parameters is done in dsi_init().
>>
>> Signed-off-by: Archit Taneja<archit@ti.com>

<snip>

>> +int omap_dsi_request_vc(struct omap_dss_device *dssdev, int *channel)
> Since this is an export function it might be good to have standard comment
> Header.

Its meant to be used by another folder which is also a part of DSS2, 
other functions exported in this file don't have comments on them 
either. Not totally sure if comments are needed for this or not.

>> +{
>> +	int i;
>> +
>> +	for (i = 0; i<  4; i++) {
> Does it make sense to use a constant instead of hard coding 4?

This is a property of DSI HW which is quite unlikely to change with 
later version of OMAPs.

>> +		if (!dsi.vc[i].dssdev) {
>> +			dsi.vc[i].dssdev = dssdev;
>> +			*channel = i;
>> +			return 0;
>> +		}
>> +	}
>> +
>> +	DSSERR("cannot get VC for display %s", dssdev->name);
>> +	return -ENOSPC;
>> +}
>> +EXPORT_SYMBOL(omap_dsi_request_vc);
>> +
>> +int omap_dsi_set_vc_id(struct omap_dss_device *dssdev, int channel, int
>> vc_id)
>> +{
>> +	if (vc_id<  0 || vc_id>  4) {
>> +		DSSERR("VC ID out of range\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	if (dsi.vc[channel].dssdev == dssdev) {
>> +		dsi.vc[channel].vc_id = vc_id;
>> +		return 0;
>> +	}
>> +
> Might be a good idea to print some type of error message here?

The user of this function checks for errors and prints an error message 
very similar to what we would print here. Thats why I decided to skip here.

>> +	return -EINVAL;
>> +}
>> +EXPORT_SYMBOL(omap_dsi_set_vc_id);
>> +
>> +void omap_dsi_release_vc(struct omap_dss_device *dssdev, int channel)
>> +{
>> +	if (dsi.vc[channel].dssdev == dssdev) {
>> +		dsi.vc[channel].dssdev = NULL;
> Don't we have to free dssdev? Unless it is getting freed else where?

Its not really allocated at boot time, its a static structure filled up 
in the board file, setting to NULL just lets omap_dsi_request_vc() reuse it.

Thanks,
Archit