Re: [PATCH v3] iio: light: apds9306: Refactor threshold get/set functions to use helper

[Subhajit Ghosh <subhajit.ghosh@tweaklogic.com>]

Hi Nattan, Lucas,

> Refactor the apds9306_event_thresh_get and apds9306_event_thresh_set
> functions to use a helper function (apds9306_get_thresh_reg) for obtaining the
> correct register based on the direction of the event. This improves code
> readability,minimize the number of lines  and maintains consistency
It actually adds four more lines to the driver file. Rephrase maybe.

>   drivers/iio/light/apds9306.c | 36 ++++++++++++++++++++----------------
>   1 file changed, 20 insertions(+), 16 deletions(-)
20 additions and 16 deletions.
   
> @@ -769,22 +776,19 @@ static int apds9306_event_thresh_get(struct apds9306_data *data, int dir,
>   static int apds9306_event_thresh_set(struct apds9306_data *data, int dir,
>   				     int val)
>   {
> -	int var;
> +	int reg;
I like the name changed from 'var' to reg', makes much more sense.
>   	u8 buff[3];
>
There is always a balance/trade-off between modularity and execution speed.
I agree with Marcelo's reply in the first patch and I also think that separate function for this does not add much value.

Overall the patch looks good to me.

With the above:
Acked-by: Subhajit Ghosh <subhajit.ghosh@tweaklogic.com>

Regards,
Subhajit Ghosh