| Message ID | 20220106062255.3208817-3-cosmin.tanislav@analog.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/3] iio: addac: ad74413r: use ngpio size when iterating over mask | expand |
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)
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.
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?
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().
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;
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(-)