Le ven. 1 nov. 2024 à 16:45, Jonathan Cameron <jic23@xxxxxxxxxx> a écrit : > > On Thu, 31 Oct 2024 11:27:45 -0500 > David Lechner <dlechner@xxxxxxxxxxxx> 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@xxxxxxxxxxxx> > > > --- > > > 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@xxxxxxxxxxxx> > 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@xxxxxxxxxxxx> > Link: https://patch.msgid.link/20241031-iio-fix-write-event-config-signature-v2-6-2bcacbb517a2@xxxxxxxxxxxx > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx> > --- > 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 > > > > > > > >