[PATCH 0/2] iio: light: vcnl4000: Add vcnl4040 interrupt support

[PATCH 0/2] iio: light: vcnl4000: Add vcnl4040 interrupt support

[Mårten Lindahl]

Hi!

I'm sending two patches to add support for proximity sensor
interrupts with the vcnl4040 sensor.

The first patch is a minor restructuring of the current setup for
interrupts since the probe function hardcodes it for vcnl4010 only.

The second patch adds support to configure proximity sensor
interrupts and threshold limits for vcnl4040.

Kind regards
Mårten Lindahl

Mårten Lindahl (2):
  iio: light: vcnl4000: Make irq handling more generic
  iio: light: vcnl4000: Add interrupt support for vcnl4040

 drivers/iio/light/vcnl4000.c | 229 ++++++++++++++++++++++++++++++-----
 1 file changed, 201 insertions(+), 28 deletions(-)

-- 
2.30.2

[PATCH 1/2] iio: light: vcnl4000: Make irq handling more generic

[Mårten Lindahl]

This driver supports 4 chips, by which only one (vcnl4010) handles
interrupts and has support for triggered buffer. The setup of these
functions is hardcoded for vcnl4010 inside the generic vcnl4000_probe,
and thus ignores the chip specific configuration structure where all
other chip specific functions are specified.

This complicates adding interrupt handler and triggered buffer support
to chips which may have support for it.

Add members for irq threads and iio_buffer_setup_ops to the generic
vcnl4000_chip_spec struct, so that instead of checking a chip specific
boolean irq support, we check for a chip specific triggered buffer
handler, and/or a chip specific irq thread handler.

Signed-off-by: Mårten Lindahl <marten.lindahl@axis.com>
---
 drivers/iio/light/vcnl4000.c | 66 +++++++++++++++++++++---------------
 1 file changed, 38 insertions(+), 28 deletions(-)

diff --git a/drivers/iio/light/vcnl4000.c b/drivers/iio/light/vcnl4000.c
index cc1a2062e76d..142d1760f65d 100644
--- a/drivers/iio/light/vcnl4000.c
+++ b/drivers/iio/light/vcnl4000.c
@@ -150,11 +150,13 @@ struct vcnl4000_chip_spec {
 	struct iio_chan_spec const *channels;
 	const int num_channels;
 	const struct iio_info *info;
-	bool irq_support;
+	const struct iio_buffer_setup_ops *buffer_setup_ops;
 	int (*init)(struct vcnl4000_data *data);
 	int (*measure_light)(struct vcnl4000_data *data, int *val);
 	int (*measure_proximity)(struct vcnl4000_data *data, int *val);
 	int (*set_power_state)(struct vcnl4000_data *data, bool on);
+	irqreturn_t (*irq_thread)(int irq, void *priv);
+	irqreturn_t (*trig_buffer_func)(int irq, void *priv);
 };
 
 static const struct i2c_device_id vcnl4000_id[] = {
@@ -167,6 +169,11 @@ static const struct i2c_device_id vcnl4000_id[] = {
 };
 MODULE_DEVICE_TABLE(i2c, vcnl4000_id);
 
+static irqreturn_t vcnl4010_irq_thread(int irq, void *p);
+static irqreturn_t vcnl4010_trigger_handler(int irq, void *p);
+static int vcnl4010_buffer_postenable(struct iio_dev *indio_dev);
+static int vcnl4010_buffer_predisable(struct iio_dev *indio_dev);
+
 static int vcnl4000_set_power_state(struct vcnl4000_data *data, bool on)
 {
 	/* no suspend op */
@@ -983,6 +990,11 @@ static const struct iio_info vcnl4040_info = {
 	.read_avail = vcnl4040_read_avail,
 };
 
+static const struct iio_buffer_setup_ops vcnl4010_buffer_ops = {
+	.postenable = &vcnl4010_buffer_postenable,
+	.predisable = &vcnl4010_buffer_predisable,
+};
+
 static const struct vcnl4000_chip_spec vcnl4000_chip_spec_cfg[] = {
 	[VCNL4000] = {
 		.prod = "VCNL4000",
@@ -993,7 +1005,6 @@ static const struct vcnl4000_chip_spec vcnl4000_chip_spec_cfg[] = {
 		.channels = vcnl4000_channels,
 		.num_channels = ARRAY_SIZE(vcnl4000_channels),
 		.info = &vcnl4000_info,
-		.irq_support = false,
 	},
 	[VCNL4010] = {
 		.prod = "VCNL4010/4020",
@@ -1004,7 +1015,9 @@ static const struct vcnl4000_chip_spec vcnl4000_chip_spec_cfg[] = {
 		.channels = vcnl4010_channels,
 		.num_channels = ARRAY_SIZE(vcnl4010_channels),
 		.info = &vcnl4010_info,
-		.irq_support = true,
+		.irq_thread = vcnl4010_irq_thread,
+		.trig_buffer_func = vcnl4010_trigger_handler,
+		.buffer_setup_ops = &vcnl4010_buffer_ops,
 	},
 	[VCNL4040] = {
 		.prod = "VCNL4040",
@@ -1015,7 +1028,6 @@ static const struct vcnl4000_chip_spec vcnl4000_chip_spec_cfg[] = {
 		.channels = vcnl4040_channels,
 		.num_channels = ARRAY_SIZE(vcnl4040_channels),
 		.info = &vcnl4040_info,
-		.irq_support = false,
 	},
 	[VCNL4200] = {
 		.prod = "VCNL4200",
@@ -1026,7 +1038,6 @@ static const struct vcnl4000_chip_spec vcnl4000_chip_spec_cfg[] = {
 		.channels = vcnl4000_channels,
 		.num_channels = ARRAY_SIZE(vcnl4000_channels),
 		.info = &vcnl4000_info,
-		.irq_support = false,
 	},
 };
 
@@ -1153,11 +1164,6 @@ static int vcnl4010_buffer_predisable(struct iio_dev *indio_dev)
 	return i2c_smbus_write_byte_data(data->client, VCNL4000_COMMAND, 0);
 }
 
-static const struct iio_buffer_setup_ops vcnl4010_buffer_ops = {
-	.postenable = &vcnl4010_buffer_postenable,
-	.predisable = &vcnl4010_buffer_predisable,
-};
-
 static const struct iio_trigger_ops vcnl4010_trigger_ops = {
 	.validate_device = iio_trigger_validate_own_device,
 };
@@ -1214,26 +1220,30 @@ static int vcnl4000_probe(struct i2c_client *client)
 	indio_dev->name = VCNL4000_DRV_NAME;
 	indio_dev->modes = INDIO_DIRECT_MODE;
 
-	if (client->irq && data->chip_spec->irq_support) {
-		ret = devm_iio_triggered_buffer_setup(&client->dev, indio_dev,
-						      NULL,
-						      vcnl4010_trigger_handler,
-						      &vcnl4010_buffer_ops);
-		if (ret < 0) {
-			dev_err(&client->dev,
-				"unable to setup iio triggered buffer\n");
-			return ret;
+	if (client->irq) {
+		if (data->chip_spec->trig_buffer_func) {
+			ret = devm_iio_triggered_buffer_setup(&client->dev,
+							      indio_dev, NULL,
+							      data->chip_spec->trig_buffer_func,
+							      data->chip_spec->buffer_setup_ops);
+			if (ret < 0) {
+				dev_err(&client->dev,
+					"unable to setup iio triggered buffer\n");
+				return ret;
+			}
 		}
 
-		ret = devm_request_threaded_irq(&client->dev, client->irq,
-						NULL, vcnl4010_irq_thread,
-						IRQF_TRIGGER_FALLING |
-						IRQF_ONESHOT,
-						"vcnl4010_irq",
-						indio_dev);
-		if (ret < 0) {
-			dev_err(&client->dev, "irq request failed\n");
-			return ret;
+		if (data->chip_spec->irq_thread) {
+			ret = devm_request_threaded_irq(&client->dev,
+							client->irq, NULL,
+							data->chip_spec->irq_thread,
+							IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
+							"vcnl4000_irq",
+							indio_dev);
+			if (ret < 0) {
+				dev_err(&client->dev, "irq request failed\n");
+				return ret;
+			}
 		}
 
 		ret = vcnl4010_probe_trigger(indio_dev);
-- 
2.30.2

Re: [PATCH 1/2] iio: light: vcnl4000: Make irq handling more generic

[Jonathan Cameron]

On Tue, 20 Dec 2022 22:49:58 +0100
Mårten Lindahl <marten.lindahl@axis.com> wrote:

> This driver supports 4 chips, by which only one (vcnl4010) handles
> interrupts and has support for triggered buffer. The setup of these
> functions is hardcoded for vcnl4010 inside the generic vcnl4000_probe,
> and thus ignores the chip specific configuration structure where all
> other chip specific functions are specified.
> 
> This complicates adding interrupt handler and triggered buffer support
> to chips which may have support for it.
> 
> Add members for irq threads and iio_buffer_setup_ops to the generic
> vcnl4000_chip_spec struct, so that instead of checking a chip specific
> boolean irq support, we check for a chip specific triggered buffer
> handler, and/or a chip specific irq thread handler.
> 
> Signed-off-by: Mårten Lindahl <marten.lindahl@axis.com>
Hi Mårten,

A few comments inline.

Jonathan


> ---
>  drivers/iio/light/vcnl4000.c | 66 +++++++++++++++++++++---------------
>  1 file changed, 38 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/iio/light/vcnl4000.c b/drivers/iio/light/vcnl4000.c
> index cc1a2062e76d..142d1760f65d 100644
> --- a/drivers/iio/light/vcnl4000.c
> +++ b/drivers/iio/light/vcnl4000.c
> @@ -150,11 +150,13 @@ struct vcnl4000_chip_spec {
>  	struct iio_chan_spec const *channels;
>  	const int num_channels;
>  	const struct iio_info *info;
> -	bool irq_support;
> +	const struct iio_buffer_setup_ops *buffer_setup_ops;
>  	int (*init)(struct vcnl4000_data *data);
>  	int (*measure_light)(struct vcnl4000_data *data, int *val);
>  	int (*measure_proximity)(struct vcnl4000_data *data, int *val);
>  	int (*set_power_state)(struct vcnl4000_data *data, bool on);
> +	irqreturn_t (*irq_thread)(int irq, void *priv);
> +	irqreturn_t (*trig_buffer_func)(int irq, void *priv);
>  };
>  
>  static const struct i2c_device_id vcnl4000_id[] = {
> @@ -167,6 +169,11 @@ static const struct i2c_device_id vcnl4000_id[] = {
>  };
>  MODULE_DEVICE_TABLE(i2c, vcnl4000_id);
>  
> +static irqreturn_t vcnl4010_irq_thread(int irq, void *p);
> +static irqreturn_t vcnl4010_trigger_handler(int irq, void *p);
> +static int vcnl4010_buffer_postenable(struct iio_dev *indio_dev);
> +static int vcnl4010_buffer_predisable(struct iio_dev *indio_dev);

Does it makes sense to just move the chip_spec array later in the driver
to avoid this set of forwards definitions?  If you do, make that move
a precursor to this patch as otherwise things are going to get very
hard to read!

> +
>  static int vcnl4000_set_power_state(struct vcnl4000_data *data, bool on)
>  {
>  	/* no suspend op */
> @@ -983,6 +990,11 @@ static const struct iio_info vcnl4040_info = {
>  	.read_avail = vcnl4040_read_avail,
>  };
>  
> +static const struct iio_buffer_setup_ops vcnl4010_buffer_ops = {
> +	.postenable = &vcnl4010_buffer_postenable,
> +	.predisable = &vcnl4010_buffer_predisable,
> +};
> +
>  static const struct vcnl4000_chip_spec vcnl4000_chip_spec_cfg[] = {
>  	[VCNL4000] = {
>  		.prod = "VCNL4000",
> @@ -993,7 +1005,6 @@ static const struct vcnl4000_chip_spec vcnl4000_chip_spec_cfg[] = {
>  		.channels = vcnl4000_channels,
>  		.num_channels = ARRAY_SIZE(vcnl4000_channels),
>  		.info = &vcnl4000_info,
> -		.irq_support = false,
>  	},
>  	[VCNL4010] = {
>  		.prod = "VCNL4010/4020",
> @@ -1004,7 +1015,9 @@ static const struct vcnl4000_chip_spec vcnl4000_chip_spec_cfg[] = {
>  		.channels = vcnl4010_channels,
>  		.num_channels = ARRAY_SIZE(vcnl4010_channels),
>  		.info = &vcnl4010_info,
> -		.irq_support = true,
> +		.irq_thread = vcnl4010_irq_thread,
> +		.trig_buffer_func = vcnl4010_trigger_handler,
> +		.buffer_setup_ops = &vcnl4010_buffer_ops,
>  	},
>  	[VCNL4040] = {
>  		.prod = "VCNL4040",
> @@ -1015,7 +1028,6 @@ static const struct vcnl4000_chip_spec vcnl4000_chip_spec_cfg[] = {
>  		.channels = vcnl4040_channels,
>  		.num_channels = ARRAY_SIZE(vcnl4040_channels),
>  		.info = &vcnl4040_info,
> -		.irq_support = false,
>  	},
>  	[VCNL4200] = {
>  		.prod = "VCNL4200",
> @@ -1026,7 +1038,6 @@ static const struct vcnl4000_chip_spec vcnl4000_chip_spec_cfg[] = {
>  		.channels = vcnl4000_channels,
>  		.num_channels = ARRAY_SIZE(vcnl4000_channels),
>  		.info = &vcnl4000_info,
> -		.irq_support = false,
>  	},
>  };
>  
> @@ -1153,11 +1164,6 @@ static int vcnl4010_buffer_predisable(struct iio_dev *indio_dev)
>  	return i2c_smbus_write_byte_data(data->client, VCNL4000_COMMAND, 0);
>  }
>  
> -static const struct iio_buffer_setup_ops vcnl4010_buffer_ops = {
> -	.postenable = &vcnl4010_buffer_postenable,
> -	.predisable = &vcnl4010_buffer_predisable,
> -};
> -
>  static const struct iio_trigger_ops vcnl4010_trigger_ops = {
>  	.validate_device = iio_trigger_validate_own_device,
>  };
> @@ -1214,26 +1220,30 @@ static int vcnl4000_probe(struct i2c_client *client)
>  	indio_dev->name = VCNL4000_DRV_NAME;
>  	indio_dev->modes = INDIO_DIRECT_MODE;
>  
> -	if (client->irq && data->chip_spec->irq_support) {
> -		ret = devm_iio_triggered_buffer_setup(&client->dev, indio_dev,
> -						      NULL,
> -						      vcnl4010_trigger_handler,
> -						      &vcnl4010_buffer_ops);
> -		if (ret < 0) {
> -			dev_err(&client->dev,
> -				"unable to setup iio triggered buffer\n");
> -			return ret;
> +	if (client->irq) {
> +		if (data->chip_spec->trig_buffer_func) {

Whilst they may always go together, perhaps also check the buffer_setup_ops is
present.

> +			ret = devm_iio_triggered_buffer_setup(&client->dev,
> +							      indio_dev, NULL,
> +							      data->chip_spec->trig_buffer_func,
> +							      data->chip_spec->buffer_setup_ops);

As a general rule, the buffer ideally wouldn't be directly coupled to their being an
irq available. The driver only allows the trigger to be used with this device, but doesn't
prevent another trigger being used with the device (only one of the two validate functions
is there).  So I'd kind of expect this block outside of the if (client->irq)


> +			if (ret < 0) {
> +				dev_err(&client->dev,
> +					"unable to setup iio triggered buffer\n");
> +				return ret;
> +			}
>  		}
>  
> -		ret = devm_request_threaded_irq(&client->dev, client->irq,
> -						NULL, vcnl4010_irq_thread,
> -						IRQF_TRIGGER_FALLING |
> -						IRQF_ONESHOT,
> -						"vcnl4010_irq",
> -						indio_dev);
> -		if (ret < 0) {
> -			dev_err(&client->dev, "irq request failed\n");
> -			return ret;
> +		if (data->chip_spec->irq_thread) {
> +			ret = devm_request_threaded_irq(&client->dev,
> +							client->irq, NULL,
> +							data->chip_spec->irq_thread,
> +							IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
> +							"vcnl4000_irq",
> +							indio_dev);
> +			if (ret < 0) {
> +				dev_err(&client->dev, "irq request failed\n");
> +				return ret;
> +			}
>  		}
>  
>  		ret = vcnl4010_probe_trigger(indio_dev);
Does it make sense to add the trigger even if we have no irq_thread?

Re: [PATCH 1/2] iio: light: vcnl4000: Make irq handling more generic

[Marten Lindahl]

On Fri, Dec 23, 2022 at 04:53:13PM +0100, Jonathan Cameron wrote:
> On Tue, 20 Dec 2022 22:49:58 +0100
> Mårten Lindahl <marten.lindahl@axis.com> wrote:
> 
> > This driver supports 4 chips, by which only one (vcnl4010) handles
> > interrupts and has support for triggered buffer. The setup of these
> > functions is hardcoded for vcnl4010 inside the generic vcnl4000_probe,
> > and thus ignores the chip specific configuration structure where all
> > other chip specific functions are specified.
> > 
> > This complicates adding interrupt handler and triggered buffer support
> > to chips which may have support for it.
> > 
> > Add members for irq threads and iio_buffer_setup_ops to the generic
> > vcnl4000_chip_spec struct, so that instead of checking a chip specific
> > boolean irq support, we check for a chip specific triggered buffer
> > handler, and/or a chip specific irq thread handler.
> > 
> > Signed-off-by: Mårten Lindahl <marten.lindahl@axis.com>
> Hi Mårten,
> 
> A few comments inline.
> 
> Jonathan
> 
>

Hi Jonathan!

Thanks! Please see my reflections below.

> > ---
> >  drivers/iio/light/vcnl4000.c | 66 +++++++++++++++++++++---------------
> >  1 file changed, 38 insertions(+), 28 deletions(-)
> > 
> > diff --git a/drivers/iio/light/vcnl4000.c b/drivers/iio/light/vcnl4000.c
> > index cc1a2062e76d..142d1760f65d 100644
> > --- a/drivers/iio/light/vcnl4000.c
> > +++ b/drivers/iio/light/vcnl4000.c
> > @@ -150,11 +150,13 @@ struct vcnl4000_chip_spec {
> >  	struct iio_chan_spec const *channels;
> >  	const int num_channels;
> >  	const struct iio_info *info;
> > -	bool irq_support;
> > +	const struct iio_buffer_setup_ops *buffer_setup_ops;
> >  	int (*init)(struct vcnl4000_data *data);
> >  	int (*measure_light)(struct vcnl4000_data *data, int *val);
> >  	int (*measure_proximity)(struct vcnl4000_data *data, int *val);
> >  	int (*set_power_state)(struct vcnl4000_data *data, bool on);
> > +	irqreturn_t (*irq_thread)(int irq, void *priv);
> > +	irqreturn_t (*trig_buffer_func)(int irq, void *priv);
> >  };
> >  
> >  static const struct i2c_device_id vcnl4000_id[] = {
> > @@ -167,6 +169,11 @@ static const struct i2c_device_id vcnl4000_id[] = {
> >  };
> >  MODULE_DEVICE_TABLE(i2c, vcnl4000_id);
> >  
> > +static irqreturn_t vcnl4010_irq_thread(int irq, void *p);
> > +static irqreturn_t vcnl4010_trigger_handler(int irq, void *p);
> > +static int vcnl4010_buffer_postenable(struct iio_dev *indio_dev);
> > +static int vcnl4010_buffer_predisable(struct iio_dev *indio_dev);
> 
> Does it makes sense to just move the chip_spec array later in the driver
> to avoid this set of forwards definitions?  If you do, make that move
> a precursor to this patch as otherwise things are going to get very
> hard to read!
> 

Yes, I will do that.

> > +
> >  static int vcnl4000_set_power_state(struct vcnl4000_data *data, bool on)
> >  {
> >  	/* no suspend op */
> > @@ -983,6 +990,11 @@ static const struct iio_info vcnl4040_info = {
> >  	.read_avail = vcnl4040_read_avail,
> >  };
> >  
> > +static const struct iio_buffer_setup_ops vcnl4010_buffer_ops = {
> > +	.postenable = &vcnl4010_buffer_postenable,
> > +	.predisable = &vcnl4010_buffer_predisable,
> > +};
> > +
> >  static const struct vcnl4000_chip_spec vcnl4000_chip_spec_cfg[] = {
> >  	[VCNL4000] = {
> >  		.prod = "VCNL4000",
> > @@ -993,7 +1005,6 @@ static const struct vcnl4000_chip_spec vcnl4000_chip_spec_cfg[] = {
> >  		.channels = vcnl4000_channels,
> >  		.num_channels = ARRAY_SIZE(vcnl4000_channels),
> >  		.info = &vcnl4000_info,
> > -		.irq_support = false,
> >  	},
> >  	[VCNL4010] = {
> >  		.prod = "VCNL4010/4020",
> > @@ -1004,7 +1015,9 @@ static const struct vcnl4000_chip_spec vcnl4000_chip_spec_cfg[] = {
> >  		.channels = vcnl4010_channels,
> >  		.num_channels = ARRAY_SIZE(vcnl4010_channels),
> >  		.info = &vcnl4010_info,
> > -		.irq_support = true,
> > +		.irq_thread = vcnl4010_irq_thread,
> > +		.trig_buffer_func = vcnl4010_trigger_handler,
> > +		.buffer_setup_ops = &vcnl4010_buffer_ops,
> >  	},
> >  	[VCNL4040] = {
> >  		.prod = "VCNL4040",
> > @@ -1015,7 +1028,6 @@ static const struct vcnl4000_chip_spec vcnl4000_chip_spec_cfg[] = {
> >  		.channels = vcnl4040_channels,
> >  		.num_channels = ARRAY_SIZE(vcnl4040_channels),
> >  		.info = &vcnl4040_info,
> > -		.irq_support = false,
> >  	},
> >  	[VCNL4200] = {
> >  		.prod = "VCNL4200",
> > @@ -1026,7 +1038,6 @@ static const struct vcnl4000_chip_spec vcnl4000_chip_spec_cfg[] = {
> >  		.channels = vcnl4000_channels,
> >  		.num_channels = ARRAY_SIZE(vcnl4000_channels),
> >  		.info = &vcnl4000_info,
> > -		.irq_support = false,
> >  	},
> >  };
> >  
> > @@ -1153,11 +1164,6 @@ static int vcnl4010_buffer_predisable(struct iio_dev *indio_dev)
> >  	return i2c_smbus_write_byte_data(data->client, VCNL4000_COMMAND, 0);
> >  }
> >  
> > -static const struct iio_buffer_setup_ops vcnl4010_buffer_ops = {
> > -	.postenable = &vcnl4010_buffer_postenable,
> > -	.predisable = &vcnl4010_buffer_predisable,
> > -};
> > -
> >  static const struct iio_trigger_ops vcnl4010_trigger_ops = {
> >  	.validate_device = iio_trigger_validate_own_device,
> >  };
> > @@ -1214,26 +1220,30 @@ static int vcnl4000_probe(struct i2c_client *client)
> >  	indio_dev->name = VCNL4000_DRV_NAME;
> >  	indio_dev->modes = INDIO_DIRECT_MODE;
> >  
> > -	if (client->irq && data->chip_spec->irq_support) {
> > -		ret = devm_iio_triggered_buffer_setup(&client->dev, indio_dev,
> > -						      NULL,
> > -						      vcnl4010_trigger_handler,
> > -						      &vcnl4010_buffer_ops);
> > -		if (ret < 0) {
> > -			dev_err(&client->dev,
> > -				"unable to setup iio triggered buffer\n");
> > -			return ret;
> > +	if (client->irq) {
> > +		if (data->chip_spec->trig_buffer_func) {
> 
> Whilst they may always go together, perhaps also check the buffer_setup_ops is
> present.
> 

Will add the check.

> > +			ret = devm_iio_triggered_buffer_setup(&client->dev,
> > +							      indio_dev, NULL,
> > +							      data->chip_spec->trig_buffer_func,
> > +							      data->chip_spec->buffer_setup_ops);
> 
> As a general rule, the buffer ideally wouldn't be directly coupled to their being an
> irq available. The driver only allows the trigger to be used with this device, but doesn't
> prevent another trigger being used with the device (only one of the two validate functions
> is there).  So I'd kind of expect this block outside of the if (client->irq)
> 
Ok, I'll move it out of the if.
> 
> > +			if (ret < 0) {
> > +				dev_err(&client->dev,
> > +					"unable to setup iio triggered buffer\n");
> > +				return ret;
> > +			}
> >  		}
> >  
> > -		ret = devm_request_threaded_irq(&client->dev, client->irq,
> > -						NULL, vcnl4010_irq_thread,
> > -						IRQF_TRIGGER_FALLING |
> > -						IRQF_ONESHOT,
> > -						"vcnl4010_irq",
> > -						indio_dev);
> > -		if (ret < 0) {
> > -			dev_err(&client->dev, "irq request failed\n");
> > -			return ret;
> > +		if (data->chip_spec->irq_thread) {
> > +			ret = devm_request_threaded_irq(&client->dev,
> > +							client->irq, NULL,
> > +							data->chip_spec->irq_thread,
> > +							IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
> > +							"vcnl4000_irq",
> > +							indio_dev);
> > +			if (ret < 0) {
> > +				dev_err(&client->dev, "irq request failed\n");
> > +				return ret;
> > +			}
> >  		}
> >  
> >  		ret = vcnl4010_probe_trigger(indio_dev);
> Does it make sense to add the trigger even if we have no irq_thread?
> 
The irq_thread is dependent on the iio_event_interface, but I can not see that
the trigger is dependent on the irq_thread. I am not sure of this.

Kind regards
Mårten
> 

Re: [PATCH 1/2] iio: light: vcnl4000: Make irq handling more generic

[Jonathan Cameron]

On Mon, 9 Jan 2023 12:32:10 +0100
Marten Lindahl <martenli@axis.com> wrote:
> > > +			if (ret < 0) {
> > > +				dev_err(&client->dev,
> > > +					"unable to setup iio triggered buffer\n");
> > > +				return ret;
> > > +			}
> > >  		}
> > >  
> > > -		ret = devm_request_threaded_irq(&client->dev, client->irq,
> > > -						NULL, vcnl4010_irq_thread,
> > > -						IRQF_TRIGGER_FALLING |
> > > -						IRQF_ONESHOT,
> > > -						"vcnl4010_irq",
> > > -						indio_dev);
> > > -		if (ret < 0) {
> > > -			dev_err(&client->dev, "irq request failed\n");
> > > -			return ret;
> > > +		if (data->chip_spec->irq_thread) {
> > > +			ret = devm_request_threaded_irq(&client->dev,
> > > +							client->irq, NULL,
> > > +							data->chip_spec->irq_thread,
> > > +							IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
> > > +							"vcnl4000_irq",
> > > +							indio_dev);
> > > +			if (ret < 0) {
> > > +				dev_err(&client->dev, "irq request failed\n");
> > > +				return ret;
> > > +			}
> > >  		}
> > >  
> > >  		ret = vcnl4010_probe_trigger(indio_dev);  
> > Does it make sense to add the trigger even if we have no irq_thread?
> >   
> The irq_thread is dependent on the iio_event_interface, but I can not see that
> the trigger is dependent on the irq_thread. I am not sure of this.

The trigger sets up the infrastructure (under the hood it's a software
only demux of interrupts) to route to the registered consumers of the trigger.
That happens via iio_trigger_poll[_chained]() - the call in question is in the
irq handler, so whilst you can register the trigger without the irq_thread, it
won't do anything useful (hence I would not register it).

Jonathan

> 
> Kind regards
> Mårten
> >   

Re: [PATCH 1/2] iio: light: vcnl4000: Make irq handling more generic

[Marten Lindahl]

On Mon, Jan 09, 2023 at 04:30:16PM +0100, Jonathan Cameron wrote:
> On Mon, 9 Jan 2023 12:32:10 +0100
> Marten Lindahl <martenli@axis.com> wrote:
> > > > +			if (ret < 0) {
> > > > +				dev_err(&client->dev,
> > > > +					"unable to setup iio triggered buffer\n");
> > > > +				return ret;
> > > > +			}
> > > >  		}
> > > >  
> > > > -		ret = devm_request_threaded_irq(&client->dev, client->irq,
> > > > -						NULL, vcnl4010_irq_thread,
> > > > -						IRQF_TRIGGER_FALLING |
> > > > -						IRQF_ONESHOT,
> > > > -						"vcnl4010_irq",
> > > > -						indio_dev);
> > > > -		if (ret < 0) {
> > > > -			dev_err(&client->dev, "irq request failed\n");
> > > > -			return ret;
> > > > +		if (data->chip_spec->irq_thread) {
> > > > +			ret = devm_request_threaded_irq(&client->dev,
> > > > +							client->irq, NULL,
> > > > +							data->chip_spec->irq_thread,
> > > > +							IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
> > > > +							"vcnl4000_irq",
> > > > +							indio_dev);
> > > > +			if (ret < 0) {
> > > > +				dev_err(&client->dev, "irq request failed\n");
> > > > +				return ret;
> > > > +			}
> > > >  		}
> > > >  
> > > >  		ret = vcnl4010_probe_trigger(indio_dev);  
> > > Does it make sense to add the trigger even if we have no irq_thread?
> > >   
> > The irq_thread is dependent on the iio_event_interface, but I can not see that
> > the trigger is dependent on the irq_thread. I am not sure of this.
> 
> The trigger sets up the infrastructure (under the hood it's a software
> only demux of interrupts) to route to the registered consumers of the trigger.
> That happens via iio_trigger_poll[_chained]() - the call in question is in the
> irq handler, so whilst you can register the trigger without the irq_thread, it
> won't do anything useful (hence I would not register it).
> 
> Jonathan

Thanks for clarifying this. I will bind it to the irq_thread then.

Kind regards
Mårten
> 
> > 
> > Kind regards
> > Mårten
> > >   
> 

[PATCH 2/2] iio: light: vcnl4000: Add interrupt support for vcnl4040

[Mårten Lindahl]

Add support to configure proximity sensor interrupts and threshold
limits for vcnl4040. If an interrupt is detected an event will be
pushed to the event interface.

Signed-off-by: Mårten Lindahl <marten.lindahl@axis.com>
---
 drivers/iio/light/vcnl4000.c | 163 +++++++++++++++++++++++++++++++++++
 1 file changed, 163 insertions(+)

diff --git a/drivers/iio/light/vcnl4000.c b/drivers/iio/light/vcnl4000.c
index 142d1760f65d..61d18c404ea6 100644
--- a/drivers/iio/light/vcnl4000.c
+++ b/drivers/iio/light/vcnl4000.c
@@ -60,8 +60,11 @@
 
 #define VCNL4200_AL_CONF	0x00 /* Ambient light configuration */
 #define VCNL4200_PS_CONF1	0x03 /* Proximity configuration */
+#define VCNL4040_PS_THDL_LM	0x06 /* Proximity threshold low */
+#define VCNL4040_PS_THDH_LM	0x07 /* Proximity threshold high */
 #define VCNL4200_PS_DATA	0x08 /* Proximity data */
 #define VCNL4200_AL_DATA	0x09 /* Ambient light data */
+#define VCNL4040_INT_FLAGS	0x0b /* Interrupt register */
 #define VCNL4200_DEV_ID		0x0e /* Device ID, slave address and version */
 
 #define VCNL4040_DEV_ID		0x0c /* Device ID and version */
@@ -78,6 +81,9 @@
 #define VCNL4040_ALS_CONF_ALS_SHUTDOWN	BIT(0)
 #define VCNL4040_PS_CONF1_PS_SHUTDOWN	BIT(0)
 #define VCNL4040_PS_CONF2_PS_IT	GENMASK(3, 1) /* Proximity integration time */
+#define VCNL4040_PS_CONF2_PS_INT	GENMASK(9, 8) /* Proximity interrupt mode */
+#define VCNL4040_PS_IF_AWAY		BIT(8) /* Proximity event cross low threshold */
+#define VCNL4040_PS_IF_CLOSE		BIT(9) /* Proximity event cross high threshold */
 
 /* Bit masks for interrupt registers. */
 #define VCNL4010_INT_THR_SEL	BIT(0) /* Select threshold interrupt source */
@@ -138,6 +144,7 @@ struct vcnl4000_data {
 	enum vcnl4000_device_ids id;
 	int rev;
 	int al_scale;
+	int ps_int;
 	const struct vcnl4000_chip_spec *chip_spec;
 	struct mutex vcnl4000_lock;
 	struct vcnl4200_channel vcnl4200_al;
@@ -261,6 +268,10 @@ static int vcnl4200_set_power_state(struct vcnl4000_data *data, bool on)
 {
 	int ret;
 
+	/* Do not power down if interrupts are enabled */
+	if (!on && data->ps_int)
+		return 0;
+
 	ret = vcnl4000_write_als_enable(data, on);
 	if (ret < 0)
 		return ret;
@@ -302,6 +313,7 @@ static int vcnl4200_init(struct vcnl4000_data *data)
 	dev_dbg(&data->client->dev, "device id 0x%x", id);
 
 	data->rev = (ret >> 8) & 0xf;
+	data->ps_int = 0;
 
 	data->vcnl4200_al.reg = VCNL4200_AL_DATA;
 	data->vcnl4200_ps.reg = VCNL4200_PS_DATA;
@@ -802,6 +814,64 @@ static int vcnl4010_write_event(struct iio_dev *indio_dev,
 	}
 }
 
+static int vcnl4040_read_event(struct iio_dev *indio_dev,
+			       const struct iio_chan_spec *chan,
+			       enum iio_event_type type,
+			       enum iio_event_direction dir,
+			       enum iio_event_info info,
+			       int *val, int *val2)
+{
+	int ret;
+	struct vcnl4000_data *data = iio_priv(indio_dev);
+
+	switch (dir) {
+	case IIO_EV_DIR_RISING:
+		ret = i2c_smbus_read_word_data(data->client,
+					       VCNL4040_PS_THDH_LM);
+		if (ret < 0)
+			return ret;
+		*val = ret;
+		return IIO_VAL_INT;
+	case IIO_EV_DIR_FALLING:
+		ret = i2c_smbus_read_word_data(data->client,
+					       VCNL4040_PS_THDL_LM);
+		if (ret < 0)
+			return ret;
+		*val = ret;
+		return IIO_VAL_INT;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int vcnl4040_write_event(struct iio_dev *indio_dev,
+				const struct iio_chan_spec *chan,
+				enum iio_event_type type,
+				enum iio_event_direction dir,
+				enum iio_event_info info,
+				int val, int val2)
+{
+	int ret;
+	struct vcnl4000_data *data = iio_priv(indio_dev);
+
+	switch (dir) {
+	case IIO_EV_DIR_RISING:
+		ret = i2c_smbus_write_word_data(data->client,
+						VCNL4040_PS_THDH_LM, val);
+		if (ret < 0)
+			return ret;
+		return IIO_VAL_INT;
+	case IIO_EV_DIR_FALLING:
+		ret = i2c_smbus_write_word_data(data->client,
+						VCNL4040_PS_THDL_LM, val);
+		if (ret < 0)
+			return ret;
+		return IIO_VAL_INT;
+	default:
+		return -EINVAL;
+	}
+}
+
 static bool vcnl4010_is_thr_enabled(struct vcnl4000_data *data)
 {
 	int ret;
@@ -884,6 +954,80 @@ static int vcnl4010_write_event_config(struct iio_dev *indio_dev,
 	}
 }
 
+static int vcnl4040_read_event_config(struct iio_dev *indio_dev,
+				      const struct iio_chan_spec *chan,
+				      enum iio_event_type type,
+				      enum iio_event_direction dir)
+{
+	struct vcnl4000_data *data = iio_priv(indio_dev);
+
+	return (dir == IIO_EV_DIR_RISING) ?
+	    (data->ps_int & 0x01) : (data->ps_int & 0x02) >> 1;
+}
+
+static int vcnl4040_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)
+{
+	int ret;
+	u16 val;
+	struct vcnl4000_data *data = iio_priv(indio_dev);
+
+	mutex_lock(&data->vcnl4000_lock);
+
+	ret = i2c_smbus_read_word_data(data->client, VCNL4200_PS_CONF1);
+	if (ret < 0)
+		goto out;
+
+	val = FIELD_GET(VCNL4040_PS_CONF2_PS_INT, ret);
+
+	if (dir == IIO_EV_DIR_RISING)
+		val = state ? (val | 0x1) : (val & 0x2);
+	else
+		val = state ? (val | 0x2) : (val & 0x1);
+
+	data->ps_int = val;
+	val = (ret & ~VCNL4040_PS_CONF2_PS_INT) |
+	    FIELD_PREP(VCNL4040_PS_CONF2_PS_INT, val);
+	ret = i2c_smbus_write_word_data(data->client, VCNL4200_PS_CONF1, val);
+
+out:
+	mutex_unlock(&data->vcnl4000_lock);
+	data->chip_spec->set_power_state(data, (bool)data->ps_int);
+
+	return ret;
+}
+
+static irqreturn_t vcnl4040_irq_thread(int irq, void *p)
+{
+	struct iio_dev *indio_dev = p;
+	struct vcnl4000_data *data = iio_priv(indio_dev);
+	int ret;
+
+	ret = i2c_smbus_read_word_data(data->client, VCNL4040_INT_FLAGS);
+	if (ret < 0)
+		return IRQ_HANDLED;
+
+	if (ret & VCNL4040_PS_IF_CLOSE) {
+		iio_push_event(indio_dev,
+			       IIO_UNMOD_EVENT_CODE(IIO_PROXIMITY, 0,
+						    IIO_EV_TYPE_THRESH,
+						    IIO_EV_DIR_RISING),
+			       iio_get_time_ns(indio_dev));
+	}
+
+	if (ret & VCNL4040_PS_IF_AWAY) {
+		iio_push_event(indio_dev,
+			       IIO_UNMOD_EVENT_CODE(IIO_PROXIMITY, 0,
+						    IIO_EV_TYPE_THRESH,
+						    IIO_EV_DIR_FALLING),
+			       iio_get_time_ns(indio_dev));
+	}
+
+	return IRQ_HANDLED;
+}
+
 static ssize_t vcnl4000_read_near_level(struct iio_dev *indio_dev,
 					uintptr_t priv,
 					const struct iio_chan_spec *chan,
@@ -919,6 +1063,18 @@ static const struct iio_event_spec vcnl4000_event_spec[] = {
 	}
 };
 
+static const struct iio_event_spec vcnl4040_event_spec[] = {
+	{
+		.type = IIO_EV_TYPE_THRESH,
+		.dir = IIO_EV_DIR_RISING,
+		.mask_separate = BIT(IIO_EV_INFO_VALUE) | BIT(IIO_EV_INFO_ENABLE),
+	}, {
+		.type = IIO_EV_TYPE_THRESH,
+		.dir = IIO_EV_DIR_FALLING,
+		.mask_separate = BIT(IIO_EV_INFO_VALUE) | BIT(IIO_EV_INFO_ENABLE),
+	},
+};
+
 static const struct iio_chan_spec vcnl4000_channels[] = {
 	{
 		.type = IIO_LIGHT,
@@ -967,6 +1123,8 @@ static const struct iio_chan_spec vcnl4040_channels[] = {
 			BIT(IIO_CHAN_INFO_INT_TIME),
 		.info_mask_separate_available = BIT(IIO_CHAN_INFO_INT_TIME),
 		.ext_info = vcnl4000_ext_info,
+		.event_spec = vcnl4040_event_spec,
+		.num_event_specs = ARRAY_SIZE(vcnl4040_event_spec),
 	}
 };
 
@@ -987,6 +1145,10 @@ static const struct iio_info vcnl4010_info = {
 static const struct iio_info vcnl4040_info = {
 	.read_raw = vcnl4000_read_raw,
 	.write_raw = vcnl4040_write_raw,
+	.read_event_value = vcnl4040_read_event,
+	.write_event_value = vcnl4040_write_event,
+	.read_event_config = vcnl4040_read_event_config,
+	.write_event_config = vcnl4040_write_event_config,
 	.read_avail = vcnl4040_read_avail,
 };
 
@@ -1028,6 +1190,7 @@ static const struct vcnl4000_chip_spec vcnl4000_chip_spec_cfg[] = {
 		.channels = vcnl4040_channels,
 		.num_channels = ARRAY_SIZE(vcnl4040_channels),
 		.info = &vcnl4040_info,
+		.irq_thread = vcnl4040_irq_thread,
 	},
 	[VCNL4200] = {
 		.prod = "VCNL4200",
-- 
2.30.2

Re: [PATCH 2/2] iio: light: vcnl4000: Add interrupt support for vcnl4040

[Jonathan Cameron]

On Tue, 20 Dec 2022 22:49:59 +0100
Mårten Lindahl <marten.lindahl@axis.com> wrote:

> Add support to configure proximity sensor interrupts and threshold
> limits for vcnl4040. If an interrupt is detected an event will be
> pushed to the event interface.
> 
> Signed-off-by: Mårten Lindahl <marten.lindahl@axis.com>
Hi,

Code looks good in general. A few readability related suggestions inline.

Thanks,

Jonathan

> ---
>  drivers/iio/light/vcnl4000.c | 163 +++++++++++++++++++++++++++++++++++
>  1 file changed, 163 insertions(+)
> 
> diff --git a/drivers/iio/light/vcnl4000.c b/drivers/iio/light/vcnl4000.c
> index 142d1760f65d..61d18c404ea6 100644
> --- a/drivers/iio/light/vcnl4000.c
> +++ b/drivers/iio/light/vcnl4000.c
> @@ -60,8 +60,11 @@

...

>  /* Bit masks for interrupt registers. */
>  #define VCNL4010_INT_THR_SEL	BIT(0) /* Select threshold interrupt source */
> @@ -138,6 +144,7 @@ struct vcnl4000_data {
>  	enum vcnl4000_device_ids id;
>  	int rev;
>  	int al_scale;
> +	int ps_int;

Bit big for 2 bits ;)  Maybe size it same as register size.

Also, probably benefit from a comment as ps_int isn't a particularly obviously name.

>  	const struct vcnl4000_chip_spec *chip_spec;
>  	struct mutex vcnl4000_lock;
>  	struct vcnl4200_channel vcnl4200_al;


...

>  
> +static int vcnl4040_read_event_config(struct iio_dev *indio_dev,
> +				      const struct iio_chan_spec *chan,
> +				      enum iio_event_type type,
> +				      enum iio_event_direction dir)
> +{
> +	struct vcnl4000_data *data = iio_priv(indio_dev);
> +
> +	return (dir == IIO_EV_DIR_RISING) ?
> +	    (data->ps_int & 0x01) : (data->ps_int & 0x02) >> 1;

Add some field definitions and FIELD_GET() to extract them.

> +}
> +
> +static int vcnl4040_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)
> +{
> +	int ret;
> +	u16 val;
> +	struct vcnl4000_data *data = iio_priv(indio_dev);
> +
> +	mutex_lock(&data->vcnl4000_lock);
> +
> +	ret = i2c_smbus_read_word_data(data->client, VCNL4200_PS_CONF1);
> +	if (ret < 0)
> +		goto out;
> +
> +	val = FIELD_GET(VCNL4040_PS_CONF2_PS_INT, ret);
> +
> +	if (dir == IIO_EV_DIR_RISING)
> +		val = state ? (val | 0x1) : (val & 0x2);

Whilst I'm sure this is correct, it's not easy to follow. Perhaps
		val = state ? (val | 0x1) : (val & ~0x1);
to make it clear you are turning on an off one bit?
Also as above, some field definitions may make this easier to follow.

> +	else
> +		val = state ? (val | 0x2) : (val & 0x1);
> +
> +	data->ps_int = val;
> +	val = (ret & ~VCNL4040_PS_CONF2_PS_INT) |

It's been quite a few lines. Probably better to put that ret into
a reg_val or similarly named field to make it slightly easier to see it
is retained from above.

> +	    FIELD_PREP(VCNL4040_PS_CONF2_PS_INT, val);
> +	ret = i2c_smbus_write_word_data(data->client, VCNL4200_PS_CONF1, val);
> +
> +out:
> +	mutex_unlock(&data->vcnl4000_lock);
> +	data->chip_spec->set_power_state(data, (bool)data->ps_int);
the bool cast is a little nasty.  Perhaps != 0 is clearer?

> +
> +	return ret;
> +}
> +

Re: [PATCH 2/2] iio: light: vcnl4000: Add interrupt support for vcnl4040

[Marten Lindahl]

On Fri, Dec 23, 2022 at 05:00:54PM +0100, Jonathan Cameron wrote:
> On Tue, 20 Dec 2022 22:49:59 +0100
> Mårten Lindahl <marten.lindahl@axis.com> wrote:
> 
> > Add support to configure proximity sensor interrupts and threshold
> > limits for vcnl4040. If an interrupt is detected an event will be
> > pushed to the event interface.
> > 
> > Signed-off-by: Mårten Lindahl <marten.lindahl@axis.com>
> Hi,
> 
> Code looks good in general. A few readability related suggestions inline.
> 
> Thanks,
> 
> Jonathan

Hi Jonathan!

Thank you. Please see my reflections below.
> 
> > ---
> >  drivers/iio/light/vcnl4000.c | 163 +++++++++++++++++++++++++++++++++++
> >  1 file changed, 163 insertions(+)
> > 
> > diff --git a/drivers/iio/light/vcnl4000.c b/drivers/iio/light/vcnl4000.c
> > index 142d1760f65d..61d18c404ea6 100644
> > --- a/drivers/iio/light/vcnl4000.c
> > +++ b/drivers/iio/light/vcnl4000.c
> > @@ -60,8 +60,11 @@
> 
> ...
> 
> >  /* Bit masks for interrupt registers. */
> >  #define VCNL4010_INT_THR_SEL	BIT(0) /* Select threshold interrupt source */
> > @@ -138,6 +144,7 @@ struct vcnl4000_data {
> >  	enum vcnl4000_device_ids id;
> >  	int rev;
> >  	int al_scale;
> > +	int ps_int;
> 
> Bit big for 2 bits ;)  Maybe size it same as register size.
> 
> Also, probably benefit from a comment as ps_int isn't a particularly obviously name.

Ok, I'll do so.
> 
> >  	const struct vcnl4000_chip_spec *chip_spec;
> >  	struct mutex vcnl4000_lock;
> >  	struct vcnl4200_channel vcnl4200_al;
> 
> 
> ...
> 
> >  
> > +static int vcnl4040_read_event_config(struct iio_dev *indio_dev,
> > +				      const struct iio_chan_spec *chan,
> > +				      enum iio_event_type type,
> > +				      enum iio_event_direction dir)
> > +{
> > +	struct vcnl4000_data *data = iio_priv(indio_dev);
> > +
> > +	return (dir == IIO_EV_DIR_RISING) ?
> > +	    (data->ps_int & 0x01) : (data->ps_int & 0x02) >> 1;
> 
> Add some field definitions and FIELD_GET() to extract them.

I will do that.
> 
> > +}
> > +
> > +static int vcnl4040_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)
> > +{
> > +	int ret;
> > +	u16 val;
> > +	struct vcnl4000_data *data = iio_priv(indio_dev);
> > +
> > +	mutex_lock(&data->vcnl4000_lock);
> > +
> > +	ret = i2c_smbus_read_word_data(data->client, VCNL4200_PS_CONF1);
> > +	if (ret < 0)
> > +		goto out;
> > +
> > +	val = FIELD_GET(VCNL4040_PS_CONF2_PS_INT, ret);
> > +
> > +	if (dir == IIO_EV_DIR_RISING)
> > +		val = state ? (val | 0x1) : (val & 0x2);
> 
> Whilst I'm sure this is correct, it's not easy to follow. Perhaps
> 		val = state ? (val | 0x1) : (val & ~0x1);
> to make it clear you are turning on an off one bit?
> Also as above, some field definitions may make this easier to follow.

I will do that to make it more clear.
> 
> > +	else
> > +		val = state ? (val | 0x2) : (val & 0x1);
> > +
> > +	data->ps_int = val;
> > +	val = (ret & ~VCNL4040_PS_CONF2_PS_INT) |
> 
> It's been quite a few lines. Probably better to put that ret into
> a reg_val or similarly named field to make it slightly easier to see it
> is retained from above.
> 
> > +	    FIELD_PREP(VCNL4040_PS_CONF2_PS_INT, val);
> > +	ret = i2c_smbus_write_word_data(data->client, VCNL4200_PS_CONF1, val);
> > +
> > +out:
> > +	mutex_unlock(&data->vcnl4000_lock);
> > +	data->chip_spec->set_power_state(data, (bool)data->ps_int);
> the bool cast is a little nasty.  Perhaps != 0 is clearer?

I will rework these lines using field definitions. I'll send v2 shortly.

Kind regards
Mårten

> 
> > +
> > +	return ret;
> > +}
> > +
> 
>