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
���{.n�+�������+%�����ݶ��w��{.n�+����{��G�����{ay�ʇڙ�,j��f���h��������z_�(�階�ݢj"���m������G������?����&���~��iO��z��v�^�m����������?�I�