From: Jonathan Cameron <jic23@kernel.org>
To: Julien Stephan <jstephan@baylibre.com>
Cc: Mudit Sharma <muditsharma.info@gmail.com>,
Lars-Peter Clausen <lars@metafoo.de>,
Anshul Dalal <anshulusr@gmail.com>,
Javier Carrasco <javier.carrasco.cruz@gmail.com>,
Jean-Baptiste Maneyrol <jean-baptiste.maneyrol@tdk.com>,
Michael Hennerich <Michael.Hennerich@analog.com>,
Cosmin Tanislav <cosmin.tanislav@analog.com>,
Ramona Gradinariu <ramona.gradinariu@analog.com>,
Antoniu Miclaus <antoniu.miclaus@analog.com>,
Dan Robertson <dan@dlrobertson.com>,
Marcelo Schmitt <marcelo.schmitt@analog.com>,
Matteo Martelli <matteomartelli3@gmail.com>,
Anand Ashok Dumbre <anand.ashok.dumbre@xilinx.com>,
Michal Simek <michal.simek@amd.com>,
Mariel Tinaco <Mariel.Tinaco@analog.com>,
Jagath Jog J <jagathjog1996@gmail.com>,
Lorenzo Bianconi <lorenzo@kernel.org>,
Subhajit Ghosh <subhajit.ghosh@tweaklogic.com>,
Kevin Tsai <ktsai@capellamicro.com>,
Linus Walleij <linus.walleij@linaro.org>,
Benson Leung <bleung@chromium.org>,
Guenter Roeck <groeck@chromium.org>,
linux-iio@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org,
chrome-platform@lists.linux.dev,
Julia Lawall <julia.lawall@inria.fr>
Subject: Re: [PATCH 7/7] iio: fix write_event_config signature
Date: Sat, 26 Oct 2024 13:59:08 +0100 [thread overview]
Message-ID: <20241026135908.092c2736@jic23-huawei> (raw)
In-Reply-To: <20241024-iio-fix-write-event-config-signature-v1-7-7d29e5a31b00@baylibre.com>
On Thu, 24 Oct 2024 11:11:29 +0200
Julien Stephan <jstephan@baylibre.com> wrote:
> write_event_config callback use an int for state, 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.
>
> Fix signature and update all iio drivers to use the new signature.
>
> This patch has been partially written using coccinelle with the
> following script:
>
> $ cat iio-bool.cocci
> // Options: --all-includes
>
> virtual patch
>
> @c1@
> identifier iioinfo;
> identifier wecfunc;
> @@
> static const struct iio_info iioinfo = {
> ...,
> .write_event_config =
> (
> wecfunc
> |
> &wecfunc
> ),
> ...,
> };
>
> @@
> identifier c1.wecfunc;
> identifier indio_dev, chan, type, dir, state;
> @@
> int wecfunc(struct iio_dev *indio_dev, const struct iio_chan_spec *chan, enum iio_event_type type, enum iio_event_direction dir,
> -int
> +bool
> state) {
> ...
> }
>
> make coccicheck MODE=patch COCCI=iio-bool.cocci M=drivers/iio
>
> Unfortunately, this script didn't match all files:
> * all write_event_config callbacks using iio_device_claim_direct_scoped
> were not detected and not patched.
> * all files that do not assign and declare the write_event_config
> callback in the same file.
>
> iio.h was also manually updated.
>
> The patch was build tested using allmodconfig config.
>
> cc: Julia Lawall <julia.lawall@inria.fr>
> Signed-off-by: Julien Stephan <jstephan@baylibre.com>
Hi Julien,
It's a nice cleanup, but the challenge of this sort of change is the odd ways
various drivers use what you are changing.
So ever single case needs close inspection. I took a fairly broad look at it and there
is at least one case that I think is broken by the change :(
+ a bunch of others where the result needs tidying up to not end up confusing.
Jonathan
> diff --git a/drivers/iio/accel/adxl380.c b/drivers/iio/accel/adxl380.c
> index 9f6f0a45efce7981736c3a8f005f4fef83f05cfd..5d2bda1a6a783c3f5bf6dfa6830bc80cc7055e01 100644
> --- a/drivers/iio/accel/adxl380.c
> +++ b/drivers/iio/accel/adxl380.c
> @@ -1386,7 +1386,7 @@ static int adxl380_write_event_config(struct iio_dev *indio_dev,
> const struct iio_chan_spec *chan,
> enum iio_event_type type,
> enum iio_event_direction dir,
> - int state)
> + bool state)
> {
> struct adxl380_state *st = iio_priv(indio_dev);
> enum adxl380_axis axis;
> diff --git a/drivers/iio/accel/mma9551.c b/drivers/iio/accel/mma9551.c
> index fa1799b0b0dff35ccd811c4abee3883249c184da..0656e35a1c4e5c1268a4bbf7011fd9282053dbbf 100644
> --- a/drivers/iio/accel/mma9551.c
> +++ b/drivers/iio/accel/mma9551.c
> @@ -225,7 +225,7 @@ static int mma9551_write_event_config(struct iio_dev *indio_dev,
> const struct iio_chan_spec *chan,
> enum iio_event_type type,
> enum iio_event_direction dir,
> - int state)
> + bool state)
> {
> struct mma9551_data *data = iio_priv(indio_dev);
> int ret;
Good to push down into functions called.
> diff --git a/drivers/iio/accel/sca3000.c b/drivers/iio/accel/sca3000.c
> index 87c54e41f6ccd2f9311653b757b2d794df1dd5d4..36cbfcbba04d6920ba68bebd70d21bc3898a044d 100644
> --- a/drivers/iio/accel/sca3000.c
> +++ b/drivers/iio/accel/sca3000.c
> @@ -1253,7 +1253,7 @@ static int sca3000_write_event_config(struct iio_dev *indio_dev,
> const struct iio_chan_spec *chan,
> enum iio_event_type type,
> enum iio_event_direction dir,
> - int state)
> + bool state)
> {
> struct sca3000_state *st = iio_priv(indio_dev);
> int ret;
Good to push down the state into the functions called.
> diff --git a/drivers/iio/imu/bmi323/bmi323_core.c b/drivers/iio/imu/bmi323/bmi323_core.c
> index 1e6c083ea5c1bbddb878f08cd4f4be725c0e319f..76a88e1ccc1d89988eb52d6b1be8da0f5005f0e6 100644
> --- a/drivers/iio/imu/bmi323/bmi323_core.c
> +++ b/drivers/iio/imu/bmi323/bmi323_core.c
> @@ -785,7 +785,7 @@ static const struct attribute_group bmi323_event_attribute_group = {
> static int bmi323_write_event_config(struct iio_dev *indio_dev,
> const struct iio_chan_spec *chan,
> enum iio_event_type type,
> - enum iio_event_direction dir, int state)
> + enum iio_event_direction dir, bool state)
> {
> struct bmi323_data *data = iio_priv(indio_dev);
>
Mix of types used to store state. Would be good to chase the boolean though. But no
need to do that for this series.
> diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> index fb4c6c39ff2e157593f02513cc8e01cd7eea9bdf..caefa15e559b8808051ff1c7f82c3f36c947933c 100644
> --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> @@ -1959,7 +1959,7 @@ static int
> st_lsm6dsx_write_event_config(struct iio_dev *iio_dev,
> const struct iio_chan_spec *chan,
> enum iio_event_type type,
> - enum iio_event_direction dir, int state)
> + enum iio_event_direction dir, bool state)
> {
> struct st_lsm6dsx_sensor *sensor = iio_priv(iio_dev);
> struct st_lsm6dsx_hw *hw = sensor->hw;
state is stored in a mix of bool and int in this driver. Might be a nice to cleanup
thing but not strictly part of what you are doing here.
> diff --git a/drivers/iio/light/adux1020.c b/drivers/iio/light/adux1020.c
> index 2e0170be077aef9aa194fab51afbb33aec02e513..a36cdad113f774ef8733eb1aaef0f87d5d2532f1 100644
> --- a/drivers/iio/light/adux1020.c
> +++ b/drivers/iio/light/adux1020.c
> @@ -502,7 +502,8 @@ static int adux1020_write_raw(struct iio_dev *indio_dev,
> static int adux1020_write_event_config(struct iio_dev *indio_dev,
> const struct iio_chan_spec *chan,
> enum iio_event_type type,
> - enum iio_event_direction dir, int state)
> + enum iio_event_direction dir,
> + bool state)
This one is a bug, because state is used as an intermediate for other stuff.
Bad code, but needs fixing before you can do this.
> {
> struct adux1020_data *data = iio_priv(indio_dev);
> int ret, mask;
> diff --git a/drivers/iio/light/apds9300.c b/drivers/iio/light/apds9300.c
> index 11f2ab4ca261813aff04b8c25d4595a531fb43cb..95861b2a5b2d94011d894959289c5c4f06cc1efe 100644
> --- a/drivers/iio/light/apds9300.c
> +++ b/drivers/iio/light/apds9300.c
> @@ -321,7 +321,7 @@ static int apds9300_read_interrupt_config(struct iio_dev *indio_dev,
>
> static int apds9300_write_interrupt_config(struct iio_dev *indio_dev,
> const struct iio_chan_spec *chan, enum iio_event_type type,
> - enum iio_event_direction dir, int state)
> + enum iio_event_direction dir, bool state)
> {
> struct apds9300_data *data = iio_priv(indio_dev);
> int ret;
Tidying up the interrupt state to all be tracked with a bool in here would make this better, but
not strictly related to what you have here.
> diff --git a/drivers/iio/light/apds9306.c b/drivers/iio/light/apds9306.c
> index 079e02be1005210ddd30b384ffa1ff7feeb098d7..8adc74040db2bddf93fbb773e3519abcc726b9a6 100644
> --- a/drivers/iio/light/apds9306.c
> +++ b/drivers/iio/light/apds9306.c
> @@ -1071,7 +1071,7 @@ static int apds9306_write_event_config(struct iio_dev *indio_dev,
> const struct iio_chan_spec *chan,
> enum iio_event_type type,
> enum iio_event_direction dir,
> - int state)
> + bool state)
> {
> struct apds9306_data *data = iio_priv(indio_dev);
> struct apds9306_regfields *rf = &data->rf;
This has a rather silly regmap_field_write() related if statement you could clean up but
that's an optional extra.
> diff --git a/drivers/iio/light/apds9960.c b/drivers/iio/light/apds9960.c
> index 3a56eaae5a68f2891d061871c7013f0b5447bb47..a7f0cc99f236685900f89fbc48de3be0e9a40704 100644
> --- a/drivers/iio/light/apds9960.c
> +++ b/drivers/iio/light/apds9960.c
> @@ -757,7 +757,7 @@ static int apds9960_write_event_config(struct iio_dev *indio_dev,
> const struct iio_chan_spec *chan,
> enum iio_event_type type,
> enum iio_event_direction dir,
> - int state)
> + bool state)
> {
> struct apds9960_data *data = iio_priv(indio_dev);
> int ret;
Should change type of pxs_int etc to bool.
> diff --git a/drivers/iio/light/ltr390.c b/drivers/iio/light/ltr390.c
> index ff6b5d8b582b33eba60b769dff529caa00fb7244..b67c811cdf54af8de3dec3e1713a41312fb238d4 100644
> --- a/drivers/iio/light/ltr390.c
> +++ b/drivers/iio/light/ltr390.c
> @@ -553,7 +553,7 @@ static int ltr390_write_event_config(struct iio_dev *indio_dev,
> const struct iio_chan_spec *chan,
> enum iio_event_type type,
> enum iio_event_direction dir,
> - int state)
> + bool state)
> {
> struct ltr390_data *data = iio_priv(indio_dev);
> int ret;
See review of patch 2.
> diff --git a/drivers/iio/light/tsl2772.c b/drivers/iio/light/tsl2772.c
> index cab468a82b616a23394977da1d8822d29d8941d3..13086048ee5ffae77c26db8cbd3cb7002e1a72e2 100644
> --- a/drivers/iio/light/tsl2772.c
> +++ b/drivers/iio/light/tsl2772.c
> @@ -1081,7 +1081,7 @@ static int tsl2772_write_interrupt_config(struct iio_dev *indio_dev,
> const struct iio_chan_spec *chan,
> enum iio_event_type type,
> enum iio_event_direction dir,
> - int val)
> + bool val)
> {
> struct tsl2772_chip *chip = iio_priv(indio_dev);
>
val ? true : false
in here should go.
> diff --git a/drivers/iio/proximity/hx9023s.c b/drivers/iio/proximity/hx9023s.c
> index d8fb34060d3db88a3ba5ecdc209b14be8e42e8b9..8c2f1cf062033dd8580f8f75649543289fbf9fb7 100644
> --- a/drivers/iio/proximity/hx9023s.c
> +++ b/drivers/iio/proximity/hx9023s.c
> @@ -874,7 +874,7 @@ static int hx9023s_write_event_config(struct iio_dev *indio_dev,
> const struct iio_chan_spec *chan,
> enum iio_event_type type,
> enum iio_event_direction dir,
> - int state)
> + bool state)
> {
> struct hx9023s_data *data = iio_priv(indio_dev);
Has a !!state that should go.
>
> diff --git a/drivers/iio/proximity/irsd200.c b/drivers/iio/proximity/irsd200.c
> index 6e96b764fed8b577d71c3146210679b0b61d4c38..eded45a778737a699f8b69ad86a9909594e04b32 100644
> --- a/drivers/iio/proximity/irsd200.c
> +++ b/drivers/iio/proximity/irsd200.c
> @@ -648,7 +648,8 @@ static int irsd200_read_event_config(struct iio_dev *indio_dev,
This has a !!state that should go.
> static int irsd200_write_event_config(struct iio_dev *indio_dev,
> const struct iio_chan_spec *chan,
> enum iio_event_type type,
> - enum iio_event_direction dir, int state)
> + enum iio_event_direction dir,
> + bool state)
> {
> struct irsd200_data *data = iio_priv(indio_dev);
> unsigned int tmp;
> diff --git a/drivers/iio/proximity/sx9500.c b/drivers/iio/proximity/sx9500.c
> index 3f4eace05cfc6a4679fe82854dc059aa4a710d6d..e7da02c59b753158c30c7473bdeea8886a2977a3 100644
> --- a/drivers/iio/proximity/sx9500.c
> +++ b/drivers/iio/proximity/sx9500.c
> @@ -540,7 +540,7 @@ static int sx9500_write_event_config(struct iio_dev *indio_dev,
> const struct iio_chan_spec *chan,
> enum iio_event_type type,
> enum iio_event_direction dir,
> - int state)
> + bool state)
> {
This one treats state as an integer. We should change that before applying this patch.
> struct sx9500_data *data = iio_priv(indio_dev);
> int ret;
next prev parent reply other threads:[~2024-10-26 13:01 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-10-24 9:11 [PATCH 0/7] iio: fix write_event_config signature Julien Stephan
2024-10-24 9:11 ` [PATCH 1/7] iio: light: bh1745: simplify code in write_event_config callback Julien Stephan
2024-10-26 12:20 ` Jonathan Cameron
2024-10-24 9:11 ` [PATCH 2/7] iio: light: ltr390: " Julien Stephan
2024-10-26 12:22 ` Jonathan Cameron
2024-10-24 9:11 ` [PATCH 3/7] iio: light: ltr501: " Julien Stephan
2024-10-26 12:23 ` Jonathan Cameron
2024-10-24 9:11 ` [PATCH 4/7] iio: light: veml6030: " Julien Stephan
2024-10-26 12:25 ` Jonathan Cameron
2024-10-24 9:11 ` [PATCH 5/7] iio: imu: inv_mpu6050: " Julien Stephan
2024-10-24 12:16 ` Jean-Baptiste Maneyrol
2024-10-26 12:27 ` Jonathan Cameron
2024-10-24 9:11 ` [PATCH 6/7] iio: light: stk3310: " Julien Stephan
2024-10-26 12:30 ` Jonathan Cameron
2024-10-24 9:11 ` [PATCH 7/7] iio: fix write_event_config signature Julien Stephan
2024-10-26 12:59 ` Jonathan Cameron [this message]
2024-10-30 14:07 ` Julien Stephan
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20241026135908.092c2736@jic23-huawei \
--to=jic23@kernel.org \
--cc=Mariel.Tinaco@analog.com \
--cc=Michael.Hennerich@analog.com \
--cc=anand.ashok.dumbre@xilinx.com \
--cc=anshulusr@gmail.com \
--cc=antoniu.miclaus@analog.com \
--cc=bleung@chromium.org \
--cc=chrome-platform@lists.linux.dev \
--cc=cosmin.tanislav@analog.com \
--cc=dan@dlrobertson.com \
--cc=groeck@chromium.org \
--cc=jagathjog1996@gmail.com \
--cc=javier.carrasco.cruz@gmail.com \
--cc=jean-baptiste.maneyrol@tdk.com \
--cc=jstephan@baylibre.com \
--cc=julia.lawall@inria.fr \
--cc=ktsai@capellamicro.com \
--cc=lars@metafoo.de \
--cc=linus.walleij@linaro.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-iio@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=lorenzo@kernel.org \
--cc=marcelo.schmitt@analog.com \
--cc=matteomartelli3@gmail.com \
--cc=michal.simek@amd.com \
--cc=muditsharma.info@gmail.com \
--cc=ramona.gradinariu@analog.com \
--cc=subhajit.ghosh@tweaklogic.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox