[PATCH] staging: s5k3e2fx.c: simplify complexity by factoring

[PATCH] staging: s5k3e2fx.c: simplify complexity by factoring

[Justin Madru]

the code was looping, seting s_move[i] to the following calculations

if (actual_step >= 0)
        s_move[i] = ((((i + 1) * gain + 0x200) - (i * gain + 0x200)) / 0x400);
else
        s_move[i] = ((((i + 1) * gain - 0x200) - (i * gain - 0x200)) / 0x400);

but, this code redues to the expression
	s_move[i] = gain >> 10;

The reason for the complexity was to generate a step function with
integer division and rounding to land on specific values. But these calculations
can be simplified to the following code:

	gain = ((actual_step << 10) / 5) >> 10;
	for (i = 0; i <= 4; i++)
		s_move[i] = gain;
---
 drivers/staging/dream/camera/s5k3e2fx.c |   10 +++-------
 1 files changed, 3 insertions(+), 7 deletions(-)

diff --git a/drivers/staging/dream/camera/s5k3e2fx.c b/drivers/staging/dream/camera/s5k3e2fx.c
index edba198..66582af 100644
--- a/drivers/staging/dream/camera/s5k3e2fx.c
+++ b/drivers/staging/dream/camera/s5k3e2fx.c
@@ -1093,14 +1093,10 @@ static int32_t s5k3e2fx_move_focus(int direction, int32_t num_steps)
 
 	actual_step = step_direction * (int16_t)num_steps;
 	pos_offset = init_code + s5k3e2fx_ctrl->curr_lens_pos;
-	gain = actual_step * 0x400 / 5;
+	gain = ((actual_step << 10) / 5) >> 10;
 
-	for (i = 0; i <= 4; i++) {
-		if (actual_step >= 0)
-			s_move[i] = ((((i+1)*gain+0x200) - (i*gain+0x200))/0x400);
-		else
-			s_move[i] = ((((i+1)*gain-0x200) - (i*gain-0x200))/0x400);
-	}
+	for (i = 0; i <= 4; i++)
+		s_move[i] = gain;
 
 	/* Ring Damping Code */
 	for (i = 0; i <= 4; i++) {
-- 
1.6.5.6

Re: [PATCH] staging: s5k3e2fx.c: simplify complexity by factoring

[Ray Lee]

On Tue, Dec 15, 2009 at 12:52 PM, Justin Madru <jdm64@gawab.com> wrote:
> the code was looping, seting s_move[i] to the following calculations
>
> if (actual_step >= 0)
>        s_move[i] = ((((i + 1) * gain + 0x200) - (i * gain + 0x200)) / 0x400);
> else
>        s_move[i] = ((((i + 1) * gain - 0x200) - (i * gain - 0x200)) / 0x400);
>
> but, this code redues to the expression
>        s_move[i] = gain >> 10;
>
> The reason for the complexity was to generate a step function with
> integer division and rounding to land on specific values. But these calculations
> can be simplified to the following code:
>
>        gain = ((actual_step << 10) / 5) >> 10;
>        for (i = 0; i <= 4; i++)
>                s_move[i] = gain;
> ---
>  drivers/staging/dream/camera/s5k3e2fx.c |   10 +++-------
>  1 files changed, 3 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/staging/dream/camera/s5k3e2fx.c b/drivers/staging/dream/camera/s5k3e2fx.c
> index edba198..66582af 100644
> --- a/drivers/staging/dream/camera/s5k3e2fx.c
> +++ b/drivers/staging/dream/camera/s5k3e2fx.c
> @@ -1093,14 +1093,10 @@ static int32_t s5k3e2fx_move_focus(int direction, int32_t num_steps)
>
>        actual_step = step_direction * (int16_t)num_steps;
>        pos_offset = init_code + s5k3e2fx_ctrl->curr_lens_pos;
> -       gain = actual_step * 0x400 / 5;
> +       gain = ((actual_step << 10) / 5) >> 10;
>
> -       for (i = 0; i <= 4; i++) {
> -               if (actual_step >= 0)
> -                       s_move[i] = ((((i+1)*gain+0x200) - (i*gain+0x200))/0x400);
> -               else
> -                       s_move[i] = ((((i+1)*gain-0x200) - (i*gain-0x200))/0x400);
> -       }
> +       for (i = 0; i <= 4; i++)
> +               s_move[i] = gain;
>
>        /* Ring Damping Code */
>        for (i = 0; i <= 4; i++) {
> --
> 1.6.5.6

Okay, yes, that now generates the same numbers before and after.

More worryingly, however, it's now even more obvious that there's no
need for a five-element array here to hold the steps. They're all the
same size, so the array could have been removed along with the gain
variable and the expression ((actual_step << 10) / 5) >> 10 can be
used directly in at the top of the loop in the ring damping code
directly below, the only place that uses the s_move[5] array.

(There's no point in having an array of move values unless the
individual values differ, which makes the whole thing look like
there's a bug in the code compared to the original intent of, perhaps,
making large moves in the beginning and smaller toward the end. Dunno.
This is one of those 'ask the author' sorts of things.)

Regardless, that's not your fault, and not something that has to be
rolled into this patch, so feel free to add my

Reviewed-by: Ray Lee <ray-lk@madrabbit.org>

Re: [PATCH] staging: s5k3e2fx.c: simplify complexity by factoring

[Greg KH]

On Tue, Dec 15, 2009 at 12:52:10PM -0800, Justin Madru wrote:
> the code was looping, seting s_move[i] to the following calculations
> 
> if (actual_step >= 0)
>         s_move[i] = ((((i + 1) * gain + 0x200) - (i * gain + 0x200)) / 0x400);
> else
>         s_move[i] = ((((i + 1) * gain - 0x200) - (i * gain - 0x200)) / 0x400);
> 
> but, this code redues to the expression
> 	s_move[i] = gain >> 10;
> 
> The reason for the complexity was to generate a step function with
> integer division and rounding to land on specific values. But these calculations
> can be simplified to the following code:
> 
> 	gain = ((actual_step << 10) / 5) >> 10;
> 	for (i = 0; i <= 4; i++)
> 		s_move[i] = gain;

Care to resend this with a Signed-off-by: line, and add the reviewed-by
line that was requested by Ray?

thanks,

greg k-h

Re: [PATCH] staging: s5k3e2fx.c: simplify complexity by factoring

[Justin Madru]

On 12/18/2009 10:41 AM, Greg KH wrote:

> On Tue, Dec 15, 2009 at 12:52:10PM -0800, Justin Madru wrote:
>> the code was looping, seting s_move[i] to the following calculations
>>
>> if (actual_step>= 0)
>>          s_move[i] = ((((i + 1) * gain + 0x200) - (i * gain + 0x200)) / 0x400);
>> else
>>          s_move[i] = ((((i + 1) * gain - 0x200) - (i * gain - 0x200)) / 0x400);
>>
>> but, this code redues to the expression
>> 	s_move[i] = gain>>  10;
>>
>> The reason for the complexity was to generate a step function with
>> integer division and rounding to land on specific values. But these calculations
>> can be simplified to the following code:
>>
>> 	gain = ((actual_step<<  10) / 5)>>  10;
>> 	for (i = 0; i<= 4; i++)
>> 		s_move[i] = gain;
>
> Care to resend this with a Signed-off-by: line, and add the reviewed-by
> line that was requested by Ray?
>
> thanks,
>
> greg k-h
Sorry about that, new at submitting patches, don't completely know the flow yet.
Does the below format work for you? Hopefully my email client won't mess the patch up.

Justin Madru
------

From: Justin Madru<jdm64@gawab.com>
Date: Tue, 15 Dec 2009 12:27:31 -0800
Subject: [PATCH] staging: s5k3e2fx.c: simplify complexity by factoring

the code was looping, setting s_move[i] to the following calculations

if (actual_step>= 0)
         s_move[i] = ((((i + 1) * gain + 0x200) - (i * gain + 0x200)) / 0x400);
else
         s_move[i] = ((((i + 1) * gain - 0x200) - (i * gain - 0x200)) / 0x400);

but, this code reduces to the expression
	s_move[i] = gain>>  10;

The reason for the complexity was to generate a step function with
integer division and rounding to land on specific values. But these calculations
can be simplified to the following code:

	gain = ((actual_step<<  10) / 5)>>  10;
	for (i = 0; i<= 4; i++)
		s_move[i] = gain;

Signed-off-by: Justin Madru<jdm64@gawab.com>
Reviewed-by: Ray Lee<ray-lk@madrabbit.org>
---
  drivers/staging/dream/camera/s5k3e2fx.c |   10 +++-------
  1 files changed, 3 insertions(+), 7 deletions(-)

diff --git a/drivers/staging/dream/camera/s5k3e2fx.c b/drivers/staging/dream/camera/s5k3e2fx.c
index f0e49be..93162a0 100644
--- a/drivers/staging/dream/camera/s5k3e2fx.c
+++ b/drivers/staging/dream/camera/s5k3e2fx.c
@@ -1092,14 +1092,10 @@ static int32_t s5k3e2fx_move_focus(int direction, int32_t num_steps)

  	actual_step = step_direction * (int16_t)num_steps;
  	pos_offset = init_code + s5k3e2fx_ctrl->curr_lens_pos;
-	gain = actual_step * 0x400 / 5;
+	gain = ((actual_step<<  10) / 5)>>  10;

-	for (i = 0; i<= 4; i++) {
-		if (actual_step>= 0)
-			s_move[i] = ((((i+1)*gain+0x200) - (i*gain+0x200))/0x400);
-		else
-			s_move[i] = ((((i+1)*gain-0x200) - (i*gain-0x200))/0x400);
-	}
+	for (i = 0; i<= 4; i++)
+		s_move[i] = gain;

  	/* Ring Damping Code */
  	for (i = 0; i<= 4; i++) {
-- 
1.6.5.6