| Message ID | 20241031-iio-fix-write-event-config-signature-v2-6-2bcacbb517a2@baylibre.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | iio: fix write_event_config signature | expand |
On 10/31/24 10:27 AM, Julien Stephan wrote: > state parameter is currently an int, but it is actually a boolean. > iio_ev_state_store is actually using kstrtobool to check user input, > then gives the converted boolean value to write_event_config. The code > in adux1020_write_event_config re-uses state parameter to store an > integer value. To prepare for updating the write_event_config signature > to use a boolean for state, introduce a new local int variable. > > Signed-off-by: Julien Stephan <jstephan@baylibre.com> > --- > drivers/iio/light/adux1020.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/drivers/iio/light/adux1020.c b/drivers/iio/light/adux1020.c > index 2e0170be077aef9aa194fab51afbb33aec02e513..db57d84da616b91add8c5d1aba08a73ce18c367e 100644 > --- a/drivers/iio/light/adux1020.c > +++ b/drivers/iio/light/adux1020.c > @@ -505,7 +505,7 @@ static int adux1020_write_event_config(struct iio_dev *indio_dev, > enum iio_event_direction dir, int state) > { > struct adux1020_data *data = iio_priv(indio_dev); > - int ret, mask; > + int ret, mask, val; > > mutex_lock(&data->lock); > > @@ -526,12 +526,12 @@ static int adux1020_write_event_config(struct iio_dev *indio_dev, > mask = ADUX1020_PROX_OFF1_INT; > > if (state) > - state = 0; > + val = 0; > else > - state = mask; > + val = mask; > > ret = regmap_update_bits(data->regmap, ADUX1020_REG_INT_MASK, > - mask, state); > + mask, val); > if (ret < 0) > goto fail; > > Instead of introducing `val`, I would rewrite this as: if (state) ret = regmap_clear_bits(...); else ret = regmap_set_bits(...);
On Thu, 31 Oct 2024 11:27:45 -0500 David Lechner <dlechner@baylibre.com> wrote: > On 10/31/24 10:27 AM, Julien Stephan wrote: > > state parameter is currently an int, but it is actually a boolean. > > iio_ev_state_store is actually using kstrtobool to check user input, > > then gives the converted boolean value to write_event_config. The code > > in adux1020_write_event_config re-uses state parameter to store an > > integer value. To prepare for updating the write_event_config signature > > to use a boolean for state, introduce a new local int variable. > > > > Signed-off-by: Julien Stephan <jstephan@baylibre.com> > > --- > > drivers/iio/light/adux1020.c | 8 ++++---- > > 1 file changed, 4 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/iio/light/adux1020.c b/drivers/iio/light/adux1020.c > > index 2e0170be077aef9aa194fab51afbb33aec02e513..db57d84da616b91add8c5d1aba08a73ce18c367e 100644 > > --- a/drivers/iio/light/adux1020.c > > +++ b/drivers/iio/light/adux1020.c > > @@ -505,7 +505,7 @@ static int adux1020_write_event_config(struct iio_dev *indio_dev, > > enum iio_event_direction dir, int state) > > { > > struct adux1020_data *data = iio_priv(indio_dev); > > - int ret, mask; > > + int ret, mask, val; > > > > mutex_lock(&data->lock); > > > > @@ -526,12 +526,12 @@ static int adux1020_write_event_config(struct iio_dev *indio_dev, > > mask = ADUX1020_PROX_OFF1_INT; > > > > if (state) > > - state = 0; > > + val = 0; > > else > > - state = mask; > > + val = mask; > > > > ret = regmap_update_bits(data->regmap, ADUX1020_REG_INT_MASK, > > - mask, state); > > + mask, val); > > if (ret < 0) > > goto fail; > > > > > > Instead of introducing `val`, I would rewrite this as: > > if (state) > ret = regmap_clear_bits(...); > else > ret = regmap_set_bits(...); > Good idea. Rather than go around again and potentially stall the end of this series. I made that change whilst applying. Shout if either of you doesn't like the result. Diff doesn't do a perfect job on readability (it does if I add a line break but then the code looks worse in the end!) From 06a1ca816450d1b5524f6010581a83ab9935d51b Mon Sep 17 00:00:00 2001 From: Julien Stephan <jstephan@baylibre.com> Date: Thu, 31 Oct 2024 16:27:01 +0100 Subject: [PATCH] iio: light: adux1020: write_event_config: use local variable for interrupt value state parameter is currently an int, but it is actually a boolean. iio_ev_state_store is actually using kstrtobool to check user input, then gives the converted boolean value to write_event_config. The code in adux1020_write_event_config re-uses state parameter to store an integer value. To prepare for updating the write_event_config signature to use a boolean for state, introduce a new local int variable. Signed-off-by: Julien Stephan <jstephan@baylibre.com> Link: https://patch.msgid.link/20241031-iio-fix-write-event-config-signature-v2-6-2bcacbb517a2@baylibre.com Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> --- drivers/iio/light/adux1020.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/drivers/iio/light/adux1020.c b/drivers/iio/light/adux1020.c index 2e0170be077a..06d5bc1d246c 100644 --- a/drivers/iio/light/adux1020.c +++ b/drivers/iio/light/adux1020.c @@ -526,12 +526,11 @@ static int adux1020_write_event_config(struct iio_dev *indio_dev, mask = ADUX1020_PROX_OFF1_INT; if (state) - state = 0; + ret = regmap_clear_bits(data->regmap, + ADUX1020_REG_INT_MASK, mask); else - state = mask; - - ret = regmap_update_bits(data->regmap, ADUX1020_REG_INT_MASK, - mask, state); + ret = regmap_set_bits(data->regmap, + ADUX1020_REG_INT_MASK, mask); if (ret < 0) goto fail;
Le ven. 1 nov. 2024 à 16:45, Jonathan Cameron <jic23@kernel.org> a écrit : > > On Thu, 31 Oct 2024 11:27:45 -0500 > David Lechner <dlechner@baylibre.com> wrote: > > > On 10/31/24 10:27 AM, Julien Stephan wrote: > > > state parameter is currently an int, but it is actually a boolean. > > > iio_ev_state_store is actually using kstrtobool to check user input, > > > then gives the converted boolean value to write_event_config. The code > > > in adux1020_write_event_config re-uses state parameter to store an > > > integer value. To prepare for updating the write_event_config signature > > > to use a boolean for state, introduce a new local int variable. > > > > > > Signed-off-by: Julien Stephan <jstephan@baylibre.com> > > > --- > > > drivers/iio/light/adux1020.c | 8 ++++---- > > > 1 file changed, 4 insertions(+), 4 deletions(-) > > > > > > diff --git a/drivers/iio/light/adux1020.c b/drivers/iio/light/adux1020.c > > > index 2e0170be077aef9aa194fab51afbb33aec02e513..db57d84da616b91add8c5d1aba08a73ce18c367e 100644 > > > --- a/drivers/iio/light/adux1020.c > > > +++ b/drivers/iio/light/adux1020.c > > > @@ -505,7 +505,7 @@ static int adux1020_write_event_config(struct iio_dev *indio_dev, > > > enum iio_event_direction dir, int state) > > > { > > > struct adux1020_data *data = iio_priv(indio_dev); > > > - int ret, mask; > > > + int ret, mask, val; > > > > > > mutex_lock(&data->lock); > > > > > > @@ -526,12 +526,12 @@ static int adux1020_write_event_config(struct iio_dev *indio_dev, > > > mask = ADUX1020_PROX_OFF1_INT; > > > > > > if (state) > > > - state = 0; > > > + val = 0; > > > else > > > - state = mask; > > > + val = mask; > > > > > > ret = regmap_update_bits(data->regmap, ADUX1020_REG_INT_MASK, > > > - mask, state); > > > + mask, val); > > > if (ret < 0) > > > goto fail; > > > > > > > > > > Instead of introducing `val`, I would rewrite this as: > > > > if (state) > > ret = regmap_clear_bits(...); > > else > > ret = regmap_set_bits(...); > > > Good idea. Rather than go around again and potentially stall the end of this series. > I made that change whilst applying. Shout if either of you doesn't > like the result. Diff doesn't do a perfect job on readability (it does > if I add a line break but then the code looks worse in the end!) > Hello Jonathan, Looks fine to me. Thank you for doing the change yourself. Cheers Julien > From 06a1ca816450d1b5524f6010581a83ab9935d51b Mon Sep 17 00:00:00 2001 > From: Julien Stephan <jstephan@baylibre.com> > Date: Thu, 31 Oct 2024 16:27:01 +0100 > Subject: [PATCH] iio: light: adux1020: write_event_config: use local variable > for interrupt value > > state parameter is currently an int, but it is actually a boolean. > iio_ev_state_store is actually using kstrtobool to check user input, > then gives the converted boolean value to write_event_config. The code > in adux1020_write_event_config re-uses state parameter to store an > integer value. To prepare for updating the write_event_config signature > to use a boolean for state, introduce a new local int variable. > > Signed-off-by: Julien Stephan <jstephan@baylibre.com> > Link: https://patch.msgid.link/20241031-iio-fix-write-event-config-signature-v2-6-2bcacbb517a2@baylibre.com > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > --- > drivers/iio/light/adux1020.c | 9 ++++----- > 1 file changed, 4 insertions(+), 5 deletions(-) > > diff --git a/drivers/iio/light/adux1020.c b/drivers/iio/light/adux1020.c > index 2e0170be077a..06d5bc1d246c 100644 > --- a/drivers/iio/light/adux1020.c > +++ b/drivers/iio/light/adux1020.c > @@ -526,12 +526,11 @@ static int adux1020_write_event_config(struct iio_dev *indio_dev, > mask = ADUX1020_PROX_OFF1_INT; > > if (state) > - state = 0; > + ret = regmap_clear_bits(data->regmap, > + ADUX1020_REG_INT_MASK, mask); > else > - state = mask; > - > - ret = regmap_update_bits(data->regmap, ADUX1020_REG_INT_MASK, > - mask, state); > + ret = regmap_set_bits(data->regmap, > + ADUX1020_REG_INT_MASK, mask); > if (ret < 0) > goto fail; > > -- > 2.46.2 > > > > > > > >
diff --git a/drivers/iio/light/adux1020.c b/drivers/iio/light/adux1020.c index 2e0170be077aef9aa194fab51afbb33aec02e513..db57d84da616b91add8c5d1aba08a73ce18c367e 100644 --- a/drivers/iio/light/adux1020.c +++ b/drivers/iio/light/adux1020.c @@ -505,7 +505,7 @@ static int adux1020_write_event_config(struct iio_dev *indio_dev, enum iio_event_direction dir, int state) { struct adux1020_data *data = iio_priv(indio_dev); - int ret, mask; + int ret, mask, val; mutex_lock(&data->lock); @@ -526,12 +526,12 @@ static int adux1020_write_event_config(struct iio_dev *indio_dev, mask = ADUX1020_PROX_OFF1_INT; if (state) - state = 0; + val = 0; else - state = mask; + val = mask; ret = regmap_update_bits(data->regmap, ADUX1020_REG_INT_MASK, - mask, state); + mask, val); if (ret < 0) goto fail;
state parameter is currently an int, but it is actually a boolean. iio_ev_state_store is actually using kstrtobool to check user input, then gives the converted boolean value to write_event_config. The code in adux1020_write_event_config re-uses state parameter to store an integer value. To prepare for updating the write_event_config signature to use a boolean for state, introduce a new local int variable. Signed-off-by: Julien Stephan <jstephan@baylibre.com> --- drivers/iio/light/adux1020.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)