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

[Marten Lindahl <martenli@axis.com>]

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
>