lkml.org 
[lkml]   [2010]   [Jun]   [24]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRE: [PATCH] Staging: d53155_drv.c: cleanup fbuffer usage
On Thursday, June 24, 2010 10:38 AM, Joe Perches wrote:
> On Thu, 2010-06-24 at 09:31 -0700, H Hartley Sweeten wrote:
> The global symbol dt3155_fbuffer[], declared in dt3155_isr.c, is really
>> just a pointer to dt3155_status[].fbuffer. To improve readability, make
>> some of the really long lines shorter, and make the buffer access more
>> consistent, use &dt3155_status[].fbuffer to access the buffer structure.
>
> I think this would be more consistent/intelligible
> if the temporary wasn't
> struct dt3155_fbuffer *fb = &dt3155_status[minor].fbuffer;
> but was
> struct dt3155_status *dts = &dt3155_status[minor];
>
>> @@ -146,11 +148,11 @@ static void quick_stop (int minor)
>> dt3155_status[minor].state &= ~(DT3155_STATE_STOP|0xff);
>> /* mark the system stopped: */
>> dt3155_status[minor].state |= DT3155_STATE_IDLE;
>> - dt3155_fbuffer[minor]->stop_acquire = 0;
>> - dt3155_fbuffer[minor]->even_stopped = 0;
>> + fb->stop_acquire = 0;
>> + fb->even_stopped = 0;
>> #else
>> dt3155_status[minor].state |= DT3155_STATE_STOP;
>> - dt3155_status[minor].fbuffer.stop_acquire = 1;
>> + fb->stop_acquire = 1;
>> #endif
>
> becomes
>
> dts->state &= ~(DT3155_STATE_STOP|0xff);
> /* mark the system stopped: */
> dts->state |= DT3155_STATE_IDLE;
> dts->fbuffer.stop_acquire = 0;
> dts->fbuffer.even_stopped = 0;
> #else
> dts->fbuffer.stop_acquire = 1;
> #endif

I will be posting a follow up patch to remove the dt3155_status[minor] similar
to what you are proposing above. I just needed to start somewhere that was
still reviewable. I tried it all in one patch but it's a bit much to review.

Personally I prefer using the 'fb' dereference instead of 'dts->fbuffer' since
there are a number of places in the driver where it gets a bit hard to read.
Also, this file still needs to be reformatted to the kernel coding style but
it's a bit difficult due to the length of some of the lines.

This one for example around line 241:

buffer_addr = dt3155_fbuffer[minor]->
frame_info[dt3155_fbuffer[minor]->active_buf].addr
+ (DT3155_MAX_ROWS / 2) * stride;
With 'fb' it becomes:

buffer_addr = fb->frame_info[fb->active_buf].addr +
(DT3155_MAX_ROWS / 2) * stride;
With 'dts->fbuffer' it would be:

buffer_addr = dts->fbuffer.frame_info[dts->fbuffer.active_buf].addr +
(DT3155_MAX_ROWS / 2) * stride;
The one around line 341 is really bad right now:

dt3155_fbuffer[minor]->
frame_info[dt3155_fbuffer[minor]->
active_buf].tag = ++unique_tag;
With the 'fb' temporary it's reduced to:

fb->frame_info[fb->active_buf].tag = ++unique_tag;
Which you can easily read and see that the tag of the active buffer is being set.

But, I guess either way will work. I really just want to get rid of all the [minor]
stuff, it's a bit annoying to read.

> Another option might be to use dereferencing inlines.
>
> static inline struct dt3155_status *dts(int index)
> {
> return &dt3155_status[index];
> }
> static inline struct dt3155_fbuffer *fb(int index)
> {
> return &(dts(index)->fbuffer);
> }
>
> I think this isn't as good as the temporary because the
> compiler sometimes doesn't reuse the intermediate addresses
> and the inlines have file rather than function scope.
>
> It becomes something like:
>
> dts(minor)->state &= ~(DT3155_STATE_STOP|0xff);
> /* mark the system stopped: */
> dts(minor)->state |= DT3155_STATE_IDLE;
> fb(minor)->stop_acquire = 0;
> fb(minor)->even_stopped = 0;
> #else
> fb(minor)->stop_acquire = 1;
> #endif
>
> I think this isn't as good as the temporary because the
> compiler sometimes doesn't reuse the intermediate addresses
> and the macros have file rather than function scope.

I also don't like the inline approach.

Regards,
Hartley
\
 
 \ /
  Last update: 2010-06-24 19:57    [from the cache]
©2003-2011 Jasper Spaans