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    [W:0.047 / U:0.264 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site