On Sat, 2017-11-25 at 17:48 +0000, Jonathan Cameron wrote:
> On Tue, 21 Nov 2017 17:11:29 +0100
> Andreas Brauchli <[email protected]> wrote:
>
> > Support triggered buffer for use with e.g. hrtimer for automated
> > polling to ensure that the sensor's internal baseline is correctly
> > updated independently of the use-case.
>
> Given the really strict timing requirements for this device and slow
> read rates, I wouldn't involve a triggered buffer at all, but
> actually
> do it with a thread / timer within the initial driver. The
> sysfs interface is more than adequate for a 1Hz device so using
> the buffered option is just adding unnecessary complexity...
Thanks for the suggestions, I went with that in v2.
>
> I reviewed the code anyway. Key thing is though the file would be
> small, there should be a separate .c file for the buffered support
> if you are going to make it optional. That way we don't get ifdefs
> in the c file, but rather stubs provided in the header.
FWIW, I learned a few things
> You could decide to add stubs to include/linux/iio/triggered_buffer.h
> /buffer.h
>
> and then use __maybe_unusued to mark your trigger function.
> The compiler would drop it anyway and this would suppress build
> warnings.
>
> However, I don't think this device wants to be supported via the
> buffered interfaces at all. More smartness needed in the core
> driver to maintain the timing etc.
>
> Jonathan
> >
> > Triggered buffer support is only enabled when IIO_BUFFER is set.
> >
> > Signed-off-by: Andreas Brauchli <[email protected]>
> > ---
> > drivers/iio/chemical/Kconfig | 3 +++
> > drivers/iio/chemical/sgpxx.c | 38
> > ++++++++++++++++++++++++++++++++++++++
> > 2 files changed, 41 insertions(+)
> >
> > diff --git a/drivers/iio/chemical/Kconfig
> > b/drivers/iio/chemical/Kconfig
> > index 4574dd687513..6710fbfc6451 100644
> > --- a/drivers/iio/chemical/Kconfig
> > +++ b/drivers/iio/chemical/Kconfig
> > @@ -42,12 +42,15 @@ config SENSIRION_SGPXX
> > tristate "Sensirion SGPxx gas sensors"
> > depends on I2C
> > select CRC8
> > + select IIO_TRIGGERED_BUFFER if (IIO_BUFFER)
> > help
> > Say Y here to build I2C interface support for the
> > following
> > Sensirion SGP gas sensors:
> > * SGP30 gas sensor
> > * SGPC3 gas sensor
> >
> > + Also select IIO_BUFFER to enable triggered buffers.
> > +
> > To compile this driver as module, choose M here: the
> > module will be called sgpxx.
> >
> > diff --git a/drivers/iio/chemical/sgpxx.c
> > b/drivers/iio/chemical/sgpxx.c
> > index aea55e41d4cc..025206448f73 100644
> > --- a/drivers/iio/chemical/sgpxx.c
> > +++ b/drivers/iio/chemical/sgpxx.c
> > @@ -27,6 +27,10 @@
> > #include <linux/of_device.h>
> > #include <linux/iio/iio.h>
> > #include <linux/iio/buffer.h>
> > +#ifdef CONFIG_IIO_BUFFER
> > +#include <linux/iio/trigger_consumer.h>
> > +#include <linux/iio/triggered_buffer.h>
> > +#endif /* CONFIG_IIO_BUFFER */
> > #include <linux/iio/sysfs.h>
> >
> > #define SGP_WORD_LEN 2
> > @@ -789,6 +793,26 @@ static const struct of_device_id sgp_dt_ids[]
> > = {
> > { }
> > };
> >
> > +#ifdef CONFIG_IIO_BUFFER
>
> All this ifdef fun is rather messy.
> Split it out to a separate file like most other drivers with
> optional buffer support.
>
> General rule (more or less) of kernel drivers is that
> any optional ifdef stuff should be in headers to provide stubs
> when code isn't available, not down in the drivers making them
> harder to read.
>
> > +static irqreturn_t sgp_trigger_handler(int irq, void *p)
> > +{
> > + struct iio_poll_func *pf = p;
> > + struct iio_dev *indio_dev = pf->indio_dev;
> > + struct sgp_data *data = iio_priv(indio_dev);
> > + int ret;
> > +
> > + ret = sgp_get_measurement(data, data->measure_iaq_cmd,
> > + SGP_MEASURE_MODE_IAQ);
> > + if (!ret)
> > + iio_push_to_buffers_with_timestamp(indio_dev,
> > + &data-
> > >buffer.start,
> > + pf->timestamp);
> > +
> > + iio_trigger_notify_done(indio_dev->trig);
> > + return IRQ_HANDLED;
> > +}
> > +#endif /* CONFIG_IIO_BUFFER */
> > +
> > static int sgp_probe(struct i2c_client *client,
> > const struct i2c_device_id *id)
> > {
> > @@ -846,6 +870,17 @@ static int sgp_probe(struct i2c_client
> > *client,
> > indio_dev->channels = chip->channels;
> > indio_dev->num_channels = chip->num_channels;
> >
> > +#ifdef CONFIG_IIO_BUFFER
> > + ret = iio_triggered_buffer_setup(indio_dev,
> > + iio_pollfunc_store_time,
> > + sgp_trigger_handler,
> > + NULL);
> > + if (ret) {
> > + dev_err(&client->dev, "failed to setup iio
> > triggered buffer\n");
> > + goto fail_free;
> > + }
> > +#endif /* CONFIG_IIO_BUFFER */
> > +
> > ret = devm_iio_device_register(&client->dev, indio_dev);
> > if (!ret)
> > return ret;
> > @@ -863,6 +898,9 @@ static int sgp_remove(struct i2c_client
> > *client)
> > {
> > struct iio_dev *indio_dev = i2c_get_clientdata(client);
> >
> > +#ifdef CONFIG_IIO_BUFFER
> > + iio_triggered_buffer_cleanup(indio_dev);
> > +#endif /* CONFIG_IIO_BUFFER */
>
> I would prefer stubs being added to the header for this case to
> having
> ifdefs in here.
>
> > devm_iio_device_unregister(&client->dev, indio_dev);
> > return 0;
> > }
>
>