| Message ID | 20221220214959.2289330-2-marten.lindahl@axis.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | iio: light: vcnl4000: Add vcnl4040 interrupt support | expand |
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?
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 >
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 > >
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 > > > >
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);
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(-)