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@xxxxxxxx> 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@xxxxxxxx>
> 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
>
