[v3] iio: sx9310: Fix semtech,avg-pos-strength setting when > 16

Commit Message

Stephen Boyd Dec. 2, 2020, 8:02 p.m. UTC

This DT property can be 0, 16, and then 64, but not 32. The math here
doesn't recognize this slight bump in the power of 2 numbers and
translates a DT property of 64 into the register value '3' when it
really should be '2'. Fix it by subtracting one more if the number being
translated is larger than 31. Also use clamp() because we're here.

Cc: Daniel Campello <[e-mail]>
Cc: Lars-Peter Clausen <[e-mail]>
Cc: Peter Meerwald-Stadler <[e-mail]>
Cc: Douglas Anderson <[e-mail]>
Cc: Gwendal Grignou <[e-mail]>
Cc: Evan Green <[e-mail]>
Signed-off-by: Stephen Boyd <[e-mail]>
---

Changes from v2 (https://lore.kernel.org/r/[e-mail]):                                                                                                         
 * Use clamp()
 * Add comment to clarify

Changes from v1 (https://lore.kernel.org/r/[e-mail]):                                                                                                         
 * Changed ternary to consider 17 to 31 as the same as 16   

 drivers/iio/proximity/sx9310.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)


base-commit: 5b19ca2c78a0838976064c0347e46a2c859b541d

Comments

Douglas Anderson Dec. 8, 2020, 10:24 p.m. UTC | #1

Hi,

On Wed, Dec 2, 2020 at 12:02 PM Stephen Boyd <[e-mail]> wrote:
>
> This DT property can be 0, 16, and then 64, but not 32. The math here
> doesn't recognize this slight bump in the power of 2 numbers and
> translates a DT property of 64 into the register value '3' when it
> really should be '2'. Fix it by subtracting one more if the number being
> translated is larger than 31. Also use clamp() because we're here.
>
> Cc: Daniel Campello <[e-mail]>
> Cc: Lars-Peter Clausen <[e-mail]>
> Cc: Peter Meerwald-Stadler <[e-mail]>
> Cc: Douglas Anderson <[e-mail]>
> Cc: Gwendal Grignou <[e-mail]>
> Cc: Evan Green <[e-mail]>
> Signed-off-by: Stephen Boyd <[e-mail]>
> ---
>
> Changes from v2 (https://lore.kernel.org/r/[e-mail]):
>  * Use clamp()
>  * Add comment to clarify
>
> Changes from v1 (https://lore.kernel.org/r/[e-mail]):
>  * Changed ternary to consider 17 to 31 as the same as 16
>
>  drivers/iio/proximity/sx9310.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)

Reviewed-by: Douglas Anderson <[e-mail]>

Stephen Boyd Jan. 11, 2021, 7:06 p.m. UTC | #2

Quoting Stephen Boyd (2020-12-02 12:02:52)
> This DT property can be 0, 16, and then 64, but not 32. The math here
> doesn't recognize this slight bump in the power of 2 numbers and
> translates a DT property of 64 into the register value '3' when it
> really should be '2'. Fix it by subtracting one more if the number being
> translated is larger than 31. Also use clamp() because we're here.
> 
> Cc: Daniel Campello <[e-mail]>
> Cc: Lars-Peter Clausen <[e-mail]>
> Cc: Peter Meerwald-Stadler <[e-mail]>
> Cc: Douglas Anderson <[e-mail]>
> Cc: Gwendal Grignou <[e-mail]>
> Cc: Evan Green <[e-mail]>
> Signed-off-by: Stephen Boyd <[e-mail]>
> ---

Hi Jonathan,

Happy new year! Can this be picked up? Or should I resend? 

> 
> Changes from v2 (https://lore.kernel.org/r/[e-mail]):                                                                                                         
>  * Use clamp()
>  * Add comment to clarify
> 
> Changes from v1 (https://lore.kernel.org/r/[e-mail]):                                                                                                         
>  * Changed ternary to consider 17 to 31 as the same as 16   
> 
>  drivers/iio/proximity/sx9310.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/proximity/sx9310.c b/drivers/iio/proximity/sx9310.c
> index a2f820997afc..0e6863e4d384 100644
> --- a/drivers/iio/proximity/sx9310.c
> +++ b/drivers/iio/proximity/sx9310.c
> @@ -1305,7 +1305,8 @@ sx9310_get_default_reg(struct sx9310_data *data, int i,
>                 if (ret)
>                         break;
>  
> -               pos = min(max(ilog2(pos), 3), 10) - 3;
> +               /* Powers of 2, except for a gap between 16 and 64 */
> +               pos = clamp(ilog2(pos), 3, 11) - (pos >= 32 ? 4 : 3);
>                 reg_def->def &= ~SX9310_REG_PROX_CTRL7_AVGPOSFILT_MASK;
>                 reg_def->def |= FIELD_PREP(SX9310_REG_PROX_CTRL7_AVGPOSFILT_MASK,
>                                            pos);
> 
> base-commit: 5b19ca2c78a0838976064c0347e46a2c859b541d

Jonathan Cameron Jan. 14, 2021, 10:15 p.m. UTC | #3

On Mon, 11 Jan 2021 11:06:41 -0800
Stephen Boyd <[e-mail]> wrote:

> Quoting Stephen Boyd (2020-12-02 12:02:52)
> > This DT property can be 0, 16, and then 64, but not 32. The math here
> > doesn't recognize this slight bump in the power of 2 numbers and
> > translates a DT property of 64 into the register value '3' when it
> > really should be '2'. Fix it by subtracting one more if the number being
> > translated is larger than 31. Also use clamp() because we're here.
> > 
> > Cc: Daniel Campello <[e-mail]>
> > Cc: Lars-Peter Clausen <[e-mail]>
> > Cc: Peter Meerwald-Stadler <[e-mail]>
> > Cc: Douglas Anderson <[e-mail]>
> > Cc: Gwendal Grignou <[e-mail]>
> > Cc: Evan Green <[e-mail]>
> > Signed-off-by: Stephen Boyd <[e-mail]>
> > ---  
> 
> Hi Jonathan,
> 
> Happy new year! Can this be picked up? Or should I resend? 
Happy new year to you as well!

Oops. Sorry - I lost this one somehow.

Anyhow, now applied and it's the pull request I just sent out.

Jonathan



> 
> > 
> > Changes from v2 (https://lore.kernel.org/r/[e-mail]):                                                                                                         
> >  * Use clamp()
> >  * Add comment to clarify
> > 
> > Changes from v1 (https://lore.kernel.org/r/[e-mail]):                                                                                                         
> >  * Changed ternary to consider 17 to 31 as the same as 16   
> > 
> >  drivers/iio/proximity/sx9310.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/iio/proximity/sx9310.c b/drivers/iio/proximity/sx9310.c
> > index a2f820997afc..0e6863e4d384 100644
> > --- a/drivers/iio/proximity/sx9310.c
> > +++ b/drivers/iio/proximity/sx9310.c
> > @@ -1305,7 +1305,8 @@ sx9310_get_default_reg(struct sx9310_data *data, int i,
> >                 if (ret)
> >                         break;
> >  
> > -               pos = min(max(ilog2(pos), 3), 10) - 3;
> > +               /* Powers of 2, except for a gap between 16 and 64 */
> > +               pos = clamp(ilog2(pos), 3, 11) - (pos >= 32 ? 4 : 3);
> >                 reg_def->def &= ~SX9310_REG_PROX_CTRL7_AVGPOSFILT_MASK;
> >                 reg_def->def |= FIELD_PREP(SX9310_REG_PROX_CTRL7_AVGPOSFILT_MASK,
> >                                            pos);
> > 
> > base-commit: 5b19ca2c78a0838976064c0347e46a2c859b541d

Patch

diff --git a/drivers/iio/proximity/sx9310.c b/drivers/iio/proximity/sx9310.c
index a2f820997afc..0e6863e4d384 100644
--- a/drivers/iio/proximity/sx9310.c
+++ b/drivers/iio/proximity/sx9310.c
@@ -1305,7 +1305,8 @@  sx9310_get_default_reg(struct sx9310_data *data, int i,
 		if (ret)
 			break;
 
-		pos = min(max(ilog2(pos), 3), 10) - 3;
+		/* Powers of 2, except for a gap between 16 and 64 */
+		pos = clamp(ilog2(pos), 3, 11) - (pos >= 32 ? 4 : 3);
 		reg_def->def &= ~SX9310_REG_PROX_CTRL7_AVGPOSFILT_MASK;
 		reg_def->def |= FIELD_PREP(SX9310_REG_PROX_CTRL7_AVGPOSFILT_MASK,
 					   pos);