[PATCH 1/3] iio: addac: ad74413r: use ngpio size when iterating over mask

[PATCH 1/3] iio: addac: ad74413r: use ngpio size when iterating over mask

[Cosmin Tanislav]

ngpio is the actual number of GPIOs handled by the GPIO chip,
as opposed to the max number of GPIOs.

Signed-off-by: Cosmin Tanislav <cosmin.tanislav@analog.com>
---
 drivers/iio/addac/ad74413r.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/addac/ad74413r.c b/drivers/iio/addac/ad74413r.c
index 5271073bb74e..6ea3cd933d05 100644
--- a/drivers/iio/addac/ad74413r.c
+++ b/drivers/iio/addac/ad74413r.c
@@ -288,7 +288,7 @@ static void ad74413r_gpio_set_multiple(struct gpio_chip *chip,
 	unsigned int offset = 0;
 	int ret;
 
-	for_each_set_bit_from(offset, mask, AD74413R_CHANNEL_MAX) {
+	for_each_set_bit_from(offset, mask, chip->ngpio) {
 		unsigned int real_offset = st->gpo_gpio_offsets[offset];
 
 		ret = ad74413r_set_gpo_config(st, real_offset,
@@ -334,7 +334,7 @@ static int ad74413r_gpio_get_multiple(struct gpio_chip *chip,
 	if (ret)
 		return ret;
 
-	for_each_set_bit_from(offset, mask, AD74413R_CHANNEL_MAX) {
+	for_each_set_bit_from(offset, mask, chip->ngpio) {
 		unsigned int real_offset = st->comp_gpio_offsets[offset];
 
 		if (val & BIT(real_offset))
-- 
2.34.1

[PATCH 2/3] iio: addac: ad74413r: for_each_set_bit_from -> for_each_set_bit

[Cosmin Tanislav]

The starting bit is always zero, it doesn't make much sense to
use for_each_set_bit_from. Replace it with for_each_set_bit
which doesn't start from a particular bit.

Signed-off-by: Cosmin Tanislav <cosmin.tanislav@analog.com>
---
 drivers/iio/addac/ad74413r.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/iio/addac/ad74413r.c b/drivers/iio/addac/ad74413r.c
index 6ea3cd933d05..3d089c0b6f7a 100644
--- a/drivers/iio/addac/ad74413r.c
+++ b/drivers/iio/addac/ad74413r.c
@@ -285,10 +285,10 @@ static void ad74413r_gpio_set_multiple(struct gpio_chip *chip,
 	struct ad74413r_state *st = gpiochip_get_data(chip);
 	unsigned long real_mask = 0;
 	unsigned long real_bits = 0;
-	unsigned int offset = 0;
+	unsigned int offset;
 	int ret;
 
-	for_each_set_bit_from(offset, mask, chip->ngpio) {
+	for_each_set_bit(offset, mask, chip->ngpio) {
 		unsigned int real_offset = st->gpo_gpio_offsets[offset];
 
 		ret = ad74413r_set_gpo_config(st, real_offset,
@@ -326,7 +326,7 @@ static int ad74413r_gpio_get_multiple(struct gpio_chip *chip,
 				      unsigned long *bits)
 {
 	struct ad74413r_state *st = gpiochip_get_data(chip);
-	unsigned int offset = 0;
+	unsigned int offset;
 	unsigned int val;
 	int ret;
 
@@ -334,7 +334,7 @@ static int ad74413r_gpio_get_multiple(struct gpio_chip *chip,
 	if (ret)
 		return ret;
 
-	for_each_set_bit_from(offset, mask, chip->ngpio) {
+	for_each_set_bit(offset, mask, chip->ngpio) {
 		unsigned int real_offset = st->comp_gpio_offsets[offset];
 
 		if (val & BIT(real_offset))
-- 
2.34.1

[PATCH 3/3] iio: addac: ad74413r: correct comparator gpio getters mask usage

[Cosmin Tanislav]

The value of the GPIOs is currently altered using offsets rather
than masks. Make use the BIT macro to turn the offsets into masks.

Signed-off-by: Cosmin Tanislav <cosmin.tanislav@analog.com>
---
 drivers/iio/addac/ad74413r.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/addac/ad74413r.c b/drivers/iio/addac/ad74413r.c
index 3d089c0b6f7a..a69dee667441 100644
--- a/drivers/iio/addac/ad74413r.c
+++ b/drivers/iio/addac/ad74413r.c
@@ -134,7 +134,6 @@ struct ad74413r_state {
 #define AD74413R_CH_EN_MASK(x)		BIT(x)
 
 #define AD74413R_REG_DIN_COMP_OUT		0x25
-#define AD74413R_DIN_COMP_OUT_SHIFT_X(x)	x
 
 #define AD74413R_REG_ADC_RESULT_X(x)	(0x26 + (x))
 #define AD74413R_ADC_RESULT_MAX		GENMASK(15, 0)
@@ -316,7 +315,7 @@ static int ad74413r_gpio_get(struct gpio_chip *chip, unsigned int offset)
 	if (ret)
 		return ret;
 
-	status &= AD74413R_DIN_COMP_OUT_SHIFT_X(real_offset);
+	status &= BIT(real_offset);
 
 	return status ? 1 : 0;
 }
@@ -334,11 +333,13 @@ static int ad74413r_gpio_get_multiple(struct gpio_chip *chip,
 	if (ret)
 		return ret;
 
+	bitmap_zero(bits, chip->ngpio);
+
 	for_each_set_bit(offset, mask, chip->ngpio) {
 		unsigned int real_offset = st->comp_gpio_offsets[offset];
 
 		if (val & BIT(real_offset))
-			*bits |= offset;
+			*bits |= BIT(offset);
 	}
 
 	return ret;
-- 
2.34.1

Re: [PATCH 1/3] iio: addac: ad74413r: use ngpio size when iterating over mask

[Andy Shevchenko]

On Fri, Jan 7, 2022 at 7:34 AM Cosmin Tanislav <demonsingur@gmail.com> wrote:
>
> ngpio is the actual number of GPIOs handled by the GPIO chip,
> as opposed to the max number of GPIOs.

Fixes?

> Signed-off-by: Cosmin Tanislav <cosmin.tanislav@analog.com>
> ---
>  drivers/iio/addac/ad74413r.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/iio/addac/ad74413r.c b/drivers/iio/addac/ad74413r.c
> index 5271073bb74e..6ea3cd933d05 100644
> --- a/drivers/iio/addac/ad74413r.c
> +++ b/drivers/iio/addac/ad74413r.c
> @@ -288,7 +288,7 @@ static void ad74413r_gpio_set_multiple(struct gpio_chip *chip,
>         unsigned int offset = 0;
>         int ret;
>
> -       for_each_set_bit_from(offset, mask, AD74413R_CHANNEL_MAX) {
> +       for_each_set_bit_from(offset, mask, chip->ngpio) {
>                 unsigned int real_offset = st->gpo_gpio_offsets[offset];
>
>                 ret = ad74413r_set_gpo_config(st, real_offset,
> @@ -334,7 +334,7 @@ static int ad74413r_gpio_get_multiple(struct gpio_chip *chip,
>         if (ret)
>                 return ret;
>
> -       for_each_set_bit_from(offset, mask, AD74413R_CHANNEL_MAX) {
> +       for_each_set_bit_from(offset, mask, chip->ngpio) {
>                 unsigned int real_offset = st->comp_gpio_offsets[offset];
>
>                 if (val & BIT(real_offset))
> --
> 2.34.1
>


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 3/3] iio: addac: ad74413r: correct comparator gpio getters mask usage

[Andy Shevchenko]

On Fri, Jan 7, 2022 at 7:34 AM Cosmin Tanislav <demonsingur@gmail.com> wrote:
>
> The value of the GPIOs is currently altered using offsets rather
> than masks. Make use the BIT macro to turn the offsets into masks.

of the

...

> -       status &= AD74413R_DIN_COMP_OUT_SHIFT_X(real_offset);
> +       status &= BIT(real_offset);

But this is completely different.

> +       bitmap_zero(bits, chip->ngpio);
> +
>         for_each_set_bit(offset, mask, chip->ngpio) {
>                 unsigned int real_offset = st->comp_gpio_offsets[offset];
>
>                 if (val & BIT(real_offset))
> -                       *bits |= offset;
> +                       *bits |= BIT(offset);

So, how was it working before? If it fixes, it should go with the
Fixes tag and before patch 2.

>         }

On top of that, you may try to see if one of bitmap_*() APIs can be
suitable here to perform the above in a more optimal way.
(At least this conditional can be replaced with __asign_bit() call,
but I think refactoring the entire loop may reveal a better approach)

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 3/3] iio: addac: ad74413r: correct comparator gpio getters mask usage

[Andy Shevchenko]

On Sun, Jan 9, 2022 at 2:13 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Fri, Jan 7, 2022 at 7:34 AM Cosmin Tanislav <demonsingur@gmail.com> wrote:

> > +       bitmap_zero(bits, chip->ngpio);

> (At least this conditional can be replaced with __asign_bit() call,
> but I think refactoring the entire loop may reveal a better approach)

Switching to it makes bitmap_zero() redundant.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 3/3] iio: addac: ad74413r: correct comparator gpio getters mask usage

[Cosmin Tanislav]

On 1/9/22 14:13, Andy Shevchenko wrote:
> On Fri, Jan 7, 2022 at 7:34 AM Cosmin Tanislav <demonsingur@gmail.com> wrote:
>>
>> The value of the GPIOs is currently altered using offsets rather
>> than masks. Make use the BIT macro to turn the offsets into masks.
> 
> of the
> 
> ...
> 
>> -       status &= AD74413R_DIN_COMP_OUT_SHIFT_X(real_offset);
>> +       status &= BIT(real_offset);
> 
> But this is completely different.

What do you mean by this is completely different?

It was broken before, it is fixed now. Indeed, I'm missing
the Fixes tag, if that's what you meant.

> 
>> +       bitmap_zero(bits, chip->ngpio);
>> +
>>          for_each_set_bit(offset, mask, chip->ngpio) {
>>                  unsigned int real_offset = st->comp_gpio_offsets[offset];
>>
>>                  if (val & BIT(real_offset))
>> -                       *bits |= offset;
>> +                       *bits |= BIT(offset);
> 
> So, how was it working before? If it fixes, it should go with the
> Fixes tag and before patch 2.
> 
>>          }
> 
> On top of that, you may try to see if one of bitmap_*() APIs can be
> suitable here to perform the above in a more optimal way.
> (At least this conditional can be replaced with __asign_bit() call,
> but I think refactoring the entire loop may reveal a better approach)
> 

I can replace the if and bitmap_zero with __assign_bit, as you
suggested. I'm not familiar with bitmap APIs, do you have a suggestion?

Re: [PATCH 3/3] iio: addac: ad74413r: correct comparator gpio getters mask usage

[Andy Shevchenko]

On Mon, Jan 10, 2022 at 6:55 PM Cosmin Tanislav <demonsingur@gmail.com> wrote:
> On 1/9/22 14:13, Andy Shevchenko wrote:
> > On Fri, Jan 7, 2022 at 7:34 AM Cosmin Tanislav <demonsingur@gmail.com> wrote:

...

> >> -       status &= AD74413R_DIN_COMP_OUT_SHIFT_X(real_offset);
> >> +       status &= BIT(real_offset);
> >
> > But this is completely different.
>
> What do you mean by this is completely different?
>
> It was broken before, it is fixed now. Indeed, I'm missing
> the Fixes tag, if that's what you meant.

Yeah, I explained myself below. I think you got the idea.

...

> >> +       bitmap_zero(bits, chip->ngpio);
> >> +
> >>          for_each_set_bit(offset, mask, chip->ngpio) {
> >>                  unsigned int real_offset = st->comp_gpio_offsets[offset];
> >>
> >>                  if (val & BIT(real_offset))
> >> -                       *bits |= offset;
> >> +                       *bits |= BIT(offset);
> >
> > So, how was it working before? If it fixes, it should go with the
> > Fixes tag and before patch 2.
> >
> > On top of that, you may try to see if one of bitmap_*() APIs can be
> > suitable here to perform the above in a more optimal way.
> > (At least this conditional can be replaced with __asign_bit() call,
> > but I think refactoring the entire loop may reveal a better approach)
>
> I can replace the if and bitmap_zero with __assign_bit, as you
> suggested. I'm not familiar with bitmap APIs, do you have a suggestion?

For now I'm lacking any new suggestions. If you don't see any better
approaches, let's go with __assign_bit().

-- 
With Best Regards,
Andy Shevchenko