Skip to contentSkip to conversation

Staging: d53155_drv.c: cleanup fbuffer usage

PATCH In review H Hartley Sweeten 2 participants 3 messages Jun 24, 2010 Staging

lore
Share & tools
lore

H Hartley Sweeten

author
#0 lore raw

PATCH 1/1 Staging: d53155_drv.c: cleanup fbuffer usage

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.

Signed-off-by H Hartley Sweeten <[email protected]>
Cc Greg Kroah-Hartman <[email protected]>
Cc Scott Smedley <[email protected]>

---

drivers/staging/dt3155/dt3155_drv.c +40 -45
index 88c9e59..f19cc1f 100644
@@ -135,6 +135,8 @@ u32 unique_tag = 0;;
  */
 static void quick_stop (int minor)
 {
+  struct dt3155_fbuffer *fb = &dt3155_status[minor].fbuffer;
+
   // TODO: scott was here
 #if 1
   int_csr_r.reg = readl(dt3155_lbase[minor] + INT_CSR);

@@ -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
 
 }

@@ -170,6 +172,7 @@ static void dt3155_isr(int irq, void *dev_id, struct pt_regs *regs)
   unsigned long flags;
   u32 buffer_addr;
   void __iomem *mmio;
+  struct dt3155_fbuffer *fb;
 
   /* find out who issued the interrupt */
   for (index = 0; index < ndevices; index++) {

@@ -187,6 +190,7 @@ static void dt3155_isr(int irq, void *dev_id, struct pt_regs *regs)
   }
 
   mmio = dt3155_lbase[minor];
+  fb = &dt3155_status[minor].fbuffer;
 
   /* Check for corruption and set a flag if so */
   csr1_r.reg = readl(mmio + CSR1);

@@ -209,7 +213,7 @@ static void dt3155_isr(int irq, void *dev_id, struct pt_regs *regs)
       if ((dt3155_status[minor].state & DT3155_STATE_MODE) ==
 	   DT3155_STATE_FLD)
 	{
-	  dt3155_fbuffer[minor]->frame_count++;
+	  fb->frame_count++;
 	}
 
       ReadI2C(mmio, EVEN_CSR, &i2c_even_csr.reg);

@@ -218,10 +222,10 @@ static void dt3155_isr(int irq, void *dev_id, struct pt_regs *regs)
       int_csr_r.fld.FLD_END_EVE = 1;
 
       /* disable the interrupt if last field */
-      if (dt3155_fbuffer[minor]->stop_acquire)
+      if (fb->stop_acquire)
 	{
 	  printk("dt3155:  even stopped.\n");
-	  dt3155_fbuffer[minor]->even_stopped = 1;
+	  fb->even_stopped = 1;
 	  if (i2c_even_csr.fld.SNGL_EVE)
 	    {
 	      int_csr_r.fld.FLD_END_EVE_EN = 0;

@@ -241,9 +245,8 @@ static void dt3155_isr(int irq, void *dev_id, struct pt_regs *regs)
 	  /* GCS (Aug 2, 2002) -- In field mode, dma the odd field
 	     into the lower half of the buffer */
 	  const u32 stride =  dt3155_status[minor].config.cols;
-	  buffer_addr = dt3155_fbuffer[minor]->
-	    frame_info[dt3155_fbuffer[minor]->active_buf].addr
-	    + (DT3155_MAX_ROWS / 2) * stride;
+	  buffer_addr = fb->frame_info[fb->active_buf].addr +
+			(DT3155_MAX_ROWS / 2) * stride;
 	  local_save_flags(flags);
 	  local_irq_disable();
 	  wake_up_interruptible(&dt3155_read_wait_queue[minor]);

@@ -262,13 +265,11 @@ static void dt3155_isr(int irq, void *dev_id, struct pt_regs *regs)
 
       /* Note that we actually saw an even field meaning  */
       /* that subsequent odd field complete the frame     */
-      dt3155_fbuffer[minor]->even_happened = 1;
+      fb->even_happened = 1;
 
       /* recording the time that the even field finished, this should be */
       /* about time in the middle of the frame */
-      do_gettimeofday(&(dt3155_fbuffer[minor]->
-			 frame_info[dt3155_fbuffer[minor]->
-				     active_buf].time));
+      do_gettimeofday(&fb->frame_info[fb->active_buf].time);
       return;
     }
 

@@ -280,15 +281,14 @@ static void dt3155_isr(int irq, void *dev_id, struct pt_regs *regs)
       /* Clear the interrupt? */
       int_csr_r.fld.FLD_END_ODD = 1;
 
-      if (dt3155_fbuffer[minor]->even_happened ||
+      if (fb->even_happened ||
 	  (dt3155_status[minor].state & DT3155_STATE_MODE) ==
 	  DT3155_STATE_FLD)
 	{
-	  dt3155_fbuffer[minor]->frame_count++;
+	  fb->frame_count++;
 	}
 
-      if (dt3155_fbuffer[minor]->stop_acquire &&
-	   dt3155_fbuffer[minor]->even_stopped)
+      if (fb->stop_acquire && fb->even_stopped)
 	{
 	  printk(KERN_DEBUG "dt3155:  stopping odd..\n");
 	  if (i2c_odd_csr.fld.SNGL_ODD)

@@ -299,8 +299,8 @@ static void dt3155_isr(int irq, void *dev_id, struct pt_regs *regs)
 
 	      /* 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;
 
 	      printk(KERN_DEBUG "dt3155:  state is now %x\n",
 		     dt3155_status[minor].state);

@@ -316,7 +316,7 @@ static void dt3155_isr(int irq, void *dev_id, struct pt_regs *regs)
       /* if the odd field has been acquired, then     */
       /* change the next dma location for both fields */
       /* and wake up the process if sleeping          */
-      if (dt3155_fbuffer[minor]->even_happened ||
+      if (fb->even_happened ||
 	   (dt3155_status[minor].state & DT3155_STATE_MODE) ==
 	   DT3155_STATE_FLD)
 	{

@@ -327,7 +327,7 @@ static void dt3155_isr(int irq, void *dev_id, struct pt_regs *regs)
 #ifdef DEBUG_QUES_B
 	  printques(minor);
 #endif
-	  if (dt3155_fbuffer[minor]->nbuffers > 2)
+	  if (fb->nbuffers > 2)
 	    {
 	      if (!are_empty_buffers(minor))
 		{

@@ -341,31 +341,26 @@ static void dt3155_isr(int irq, void *dev_id, struct pt_regs *regs)
 	      /* The ready_que can't be full, since we know
 	       * there is one active buffer right now, so it's safe
 	       * to push the active buf on the ready_que. */
-	      push_ready(minor, dt3155_fbuffer[minor]->active_buf);
+	      push_ready(minor, fb->active_buf);
 	      /* There's at least 1 empty -- make it active */
-	      dt3155_fbuffer[minor]->active_buf = pop_empty(minor);
-	      dt3155_fbuffer[minor]->
-		frame_info[dt3155_fbuffer[minor]->
-			    active_buf].tag = ++unique_tag;
+	      fb->active_buf = pop_empty(minor);
+	      fb->frame_info[fb->active_buf].tag = ++unique_tag;
 	    }
 	  else /* nbuffers == 2, special case */
 	    { /* There is 1 active buffer.
 	       * If there is a locked buffer, keep the active buffer
 	       * the same -- that means we drop a frame.
 	       */
-	      if (dt3155_fbuffer[minor]->locked_buf < 0)
+	      if (fb->locked_buf < 0)
 		{
-		  push_ready(minor,
-			      dt3155_fbuffer[minor]->active_buf);
+		  push_ready(minor, fb->active_buf);
 		  if (are_empty_buffers(minor))
 		    {
-		      dt3155_fbuffer[minor]->active_buf =
-			pop_empty(minor);
+		      fb->active_buf = pop_empty(minor);
 		    }
 		  else
 		    { /* no empty or locked buffers, so use a readybuf */
-		      dt3155_fbuffer[minor]->active_buf =
-			pop_ready(minor);
+		      fb->active_buf = pop_ready(minor);
 		    }
 		}
 	    }

@@ -374,7 +369,7 @@ static void dt3155_isr(int irq, void *dev_id, struct pt_regs *regs)
 	  printques(minor);
 #endif
 
-	  dt3155_fbuffer[minor]->even_happened = 0;
+	  fb->even_happened = 0;
 
 	  wake_up_interruptible(&dt3155_read_wait_queue[minor]);
 

@@ -383,8 +378,7 @@ static void dt3155_isr(int irq, void *dev_id, struct pt_regs *regs)
 
 
       /* Set up the DMA address for the next frame/field */
-      buffer_addr = dt3155_fbuffer[minor]->
-	frame_info[dt3155_fbuffer[minor]->active_buf].addr;
+      buffer_addr = fb->frame_info[fb->active_buf].addr;
       if ((dt3155_status[minor].state & DT3155_STATE_MODE) ==
 	   DT3155_STATE_FLD)
 	{

@@ -421,13 +415,13 @@ static void dt3155_init_isr(int minor)
 {
   const u32 stride =  dt3155_status[minor].config.cols;
   void __iomem *mmio = dt3155_lbase[minor];
+  struct dt3155_fbuffer *fb = &dt3155_status[minor].fbuffer;
 
   switch (dt3155_status[minor].state & DT3155_STATE_MODE)
     {
     case DT3155_STATE_FLD:
       {
-	even_dma_start_r  = dt3155_status[minor].
-	  fbuffer.frame_info[dt3155_status[minor].fbuffer.active_buf].addr;
+	even_dma_start_r  = fb->frame_info[fb->active_buf].addr;
 	even_dma_stride_r = 0;
 	odd_dma_stride_r  = 0;
 

@@ -440,8 +434,7 @@ static void dt3155_init_isr(int minor)
     case DT3155_STATE_FRAME:
     default:
       {
-	even_dma_start_r  = dt3155_status[minor].
-	  fbuffer.frame_info[dt3155_status[minor].fbuffer.active_buf].addr;
+	even_dma_start_r  = fb->frame_info[fb->active_buf].addr;
 	odd_dma_start_r   =  even_dma_start_r + stride;
 	even_dma_stride_r =  stride;
 	odd_dma_stride_r  =  stride;

@@ -514,6 +507,7 @@ static int dt3155_ioctl(struct inode *inode,
 {
   int minor = MINOR(inode->i_rdev); /* What device are we ioctl()'ing? */
   void __user *up = (void __user *)arg;
+  struct dt3155_fbuffer *fb = &dt3155_status[minor].fbuffer;
 
   if (minor >= MAXBOARDS || minor < 0)
     return -ENODEV;

@@ -572,7 +566,7 @@ static int dt3155_ioctl(struct inode *inode,
     case DT3155_STOP:
       {
 	if (dt3155_status[minor].state & DT3155_STATE_STOP ||
-	    dt3155_status[minor].fbuffer.stop_acquire)
+	    fb->stop_acquire)
 	  return -EBUSY;
 
 	if (dt3155_status[minor].state == DT3155_STATE_IDLE)

@@ -589,8 +583,8 @@ static int dt3155_ioctl(struct inode *inode,
 	if (dt3155_status[minor].state != DT3155_STATE_IDLE)
 	  return -EBUSY;
 
-	dt3155_status[minor].fbuffer.stop_acquire = 0;
-	dt3155_status[minor].fbuffer.frame_count = 0;
+	fb->stop_acquire = 0;
+	fb->frame_count = 0;
 
 	/* Set the MODE in the status -- we default to FRAME */
 	if (dt3155_status[minor].config.acq_mode == DT3155_MODE_FIELD)

@@ -750,6 +744,7 @@ static ssize_t dt3155_read(struct file *filep, char __user *buf,
   int		minor = MINOR(filep->f_dentry->d_inode->i_rdev);
   u32		offset;
   int		frame_index;
+  struct dt3155_fbuffer *fb = &dt3155_status[minor].fbuffer;
   struct frame_info	*frame_info;
 
   /* TODO: this should check the error flag and */

@@ -800,14 +795,14 @@ static ssize_t dt3155_read(struct file *filep, char __user *buf,
 	}
     }
 
-  frame_info = &dt3155_status[minor].fbuffer.frame_info[frame_index];
+  frame_info = &fb->frame_info[frame_index];
 
   /* make this an offset */
   offset = frame_info->addr - dt3155_status[minor].mem_addr;
 
   put_user(offset, (unsigned int __user *)buf);
   buf += sizeof(u32);
-  put_user(dt3155_status[minor].fbuffer.frame_count, (unsigned int __user *)buf);
+  put_user(fb->frame_count, (unsigned int __user *)buf);
   buf += sizeof(u32);
   put_user(dt3155_status[minor].state, (unsigned int __user *)buf);
   buf += sizeof(u32);
#1 lore raw
Quoted 4 lines · H Hartley Sweeten

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];

drivers/staging/dt3155/dt3155_drv.c static void quick_stop (int minor) · line 156

Quoted 13 lines
drivers/staging/dt3155/dt3155_drv.c +3 -3
@@ -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

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.

author
#2 lore raw
Quoted 28 lines · Joe Perches

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

Quoted 3 lines

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];

drivers/staging/dt3155/dt3155_drv.c static void quick_stop (int minor) · line 156

Quoted 13 lines
drivers/staging/dt3155/dt3155_drv.c +3 -3
@@ -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

Quoted 8 lines

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.

Quoted 26 lines

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

Quoted 3 lines

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�