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
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/devel
