Re: [PATCH v2 07/15] iio: fix write_event_config signature

[Jonathan Cameron <jic23@kernel.org>]

On Thu, 31 Oct 2024 16:27:02 +0100
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,

I went through these by hand.  There are somewhere maybe it is worth
passing booleans down into leaf functions and only convert to int
right at the end (for a field write) but I don't think we care.
The cases here are more of the variety of converting a bool to an int
to use it as a bool (like the ones you clear up later in this series).

Neither is wrong, just inefficient, so applied and pushed out as testing
for 0-day to take a look.
Fingers crossed nothing got missed!

Jonathan

> diff --git a/drivers/iio/accel/fxls8962af-core.c b/drivers/iio/accel/fxls8962af-core.c
> index ab427f3461dbbef535c2ec2cf2982202ca97bb82..f07fba17048e7b5c1958807b14d4bcb3ff87e26d 100644
> --- a/drivers/iio/accel/fxls8962af-core.c
> +++ b/drivers/iio/accel/fxls8962af-core.c
> @@ -617,7 +617,7 @@ static int
>  fxls8962af_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 fxls8962af_data *data = iio_priv(indio_dev);
>  	u8 enable_event, enable_bits;
This passes the state variable into fxls8962af_event_setup() as an integer
and uses it as a boolean.  Might as well be bool all the way.


> diff --git a/drivers/iio/light/gp2ap020a00f.c b/drivers/iio/light/gp2ap020a00f.c
> index 81e718cdeae32d60581cb490148f4f1c0bd695c7..1a352c88598e5d701256aa8659a7f9683bce56f9 100644
> --- a/drivers/iio/light/gp2ap020a00f.c
> +++ b/drivers/iio/light/gp2ap020a00f.c
> @@ -1159,7 +1159,7 @@ static int gp2ap020a00f_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 could do with a follow up as the state variable is passed as an integer
to another function that then uses it as a bool.

>  {
>  	struct gp2ap020a00f_data *data = iio_priv(indio_dev);
>  	enum gp2ap020a00f_cmd cmd;