Re: [PATCH 1/2] OMAP_VOUT: CLEANUP: Move some functions and macros from omap_vout

[Archit Taneja <archit@ti.com>]

Hi,

On Tuesday 07 June 2011 02:35 PM, Hiremath, Vaibhav wrote:
>> -----Original Message-----
>> From: Taneja, Archit
>> Sent: Friday, May 27, 2011 12:31 PM
>> To: linux-media@vger.kernel.org
>> Cc: Hiremath, Vaibhav; Taneja, Archit
>> Subject: [PATCH 1/2] OMAP_VOUT: CLEANUP: Move some functions and macros
>> from omap_vout
>>
> [Hiremath, Vaibhav] You may want to give patch revision here.

I don't think it makes sense to give the old revisions anymore, this 
patch set had been dormant since last year. I'll add revisions for the 
later versions of this set.

> Cosmetic comment -
>
> Consider changing the subject line to something -
>
> OMAP_VOUT: CLEANUP: Move generic functions and macros to common files
>
>
>> Move some inline functions from omap_vout.c to omap_voutdef.h and
>> independent
>> functions like omap_vout_alloc_buffer/omap_vout_free_buffer to
>> omap_voutlib.c.
>>
> [Hiremath, Vaibhav] Ditto here, word "some" doesn't convey anything.

Okay.

>

<snip>

>>
>>   /*
>> - * Return true if rotation is 90 or 270
>> - */
>> -static inline int rotate_90_or_270(const struct omap_vout_device *vout)
>> -{
>> -	return (vout->rotation == dss_rotation_90_degree ||
>> -			vout->rotation == dss_rotation_270_degree);
>> -}
>> -
>> -/*
>> - * Return true if rotation is enabled
>> - */
>> -static inline int rotation_enabled(const struct omap_vout_device *vout)
>> -{
>> -	return vout->rotation || vout->mirror;
>> -}
>> -
> [Hiremath, Vaibhav] As part of this cleanup I would suggest to rename these API's to self descriptive, something like -
>
> rotation_enabled =>  is_rotation_enabled
> rotate_90_or_270 =>  is_rotation_90_or_270

This patch just moves these functions. Moving it to another file and 
then changing the names in the same patch will make things messy. I'll 
do this in a separate patch in the same patch set.

>
>
>> -/*

<snip>

>> diff --git a/drivers/media/video/omap/omap_voutlib.h
>> b/drivers/media/video/omap/omap_voutlib.h
>> index a60b16e..1d722be 100644
>> --- a/drivers/media/video/omap/omap_voutlib.h
>> +++ b/drivers/media/video/omap/omap_voutlib.h
>> @@ -30,5 +30,7 @@ extern int omap_vout_new_window(struct v4l2_rect *crop,
>>   extern void omap_vout_new_format(struct v4l2_pix_format *pix,
>>   		struct v4l2_framebuffer *fbuf, struct v4l2_rect *crop,
>>   		struct v4l2_window *win);
>> +extern unsigned long omap_vout_alloc_buffer(u32 buf_size, u32
>> *phys_addr);
>> +extern void omap_vout_free_buffer(unsigned long virtaddr, u32 buf_size);
>>   #endif	/* #ifndef OMAP_VOUTLIB_H */
>>
> [Hiremath, Vaibhav] We do not need to use externs here; this should be another cleanup candidate which can be done with this patch series.

Will fix this.

Archit