| Message ID | 20230110134323.543123-3-marten.lindahl@axis.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | iio: light: vcnl4000: Add vcnl4040 interrupt support | expand |
Tue, Jan 10, 2023 at 02:43:22PM +0100, Mårten Lindahl kirjoitti: > 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. ... > - ret = devm_iio_triggered_buffer_setup(&client->dev, indio_dev, > - NULL, > + ret = devm_iio_triggered_buffer_setup(&client->dev, > + indio_dev, NULL, What the point of this part of the hunk? ... > + if (client->irq) { > + if (data->chip_spec->irq_thread) { I have checked the rest of the series and found nothing that prevents this to be written as if (client->irq && data->chip_spec->irq_thread) { This will reduce the noise in the patch. > }
On 1/15/23 18:41, andriy.shechenko@saunalahti.fi wrote: > Tue, Jan 10, 2023 at 02:43:22PM +0100, Mårten Lindahl kirjoitti: >> 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. > ... > >> - ret = devm_iio_triggered_buffer_setup(&client->dev, indio_dev, >> - NULL, > >> + ret = devm_iio_triggered_buffer_setup(&client->dev, >> + indio_dev, NULL, > What the point of this part of the hunk? Hi Andy! If you mean why these two lines are changed, I blame Lindent. But yes, I will change it back to reduce the noise. > > ... > >> + if (client->irq) { >> + if (data->chip_spec->irq_thread) { > I have checked the rest of the series and found nothing that prevents this to > be written as > > if (client->irq && data->chip_spec->irq_thread) { > > This will reduce the noise in the patch. No problem. I see your point. I'll send a new version. Thanks! Kind regards Mårten > >> }
diff --git a/drivers/iio/light/vcnl4000.c b/drivers/iio/light/vcnl4000.c index 11b54b57e592..792fc7f9c36e 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[] = { @@ -1121,7 +1123,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", @@ -1132,7 +1133,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", @@ -1143,7 +1146,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", @@ -1154,7 +1156,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, }, }; @@ -1214,31 +1215,36 @@ 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 (data->chip_spec->trig_buffer_func && + data->chip_spec->buffer_setup_ops) { + 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 (client->irq) { + 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); - if (ret < 0) - return ret; + ret = vcnl4010_probe_trigger(indio_dev); + if (ret < 0) + return ret; + } } ret = pm_runtime_set_active(&client->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 | 52 ++++++++++++++++++++---------------- 1 file changed, 29 insertions(+), 23 deletions(-)