iio: chemical: sgpxx: triggered buffer support
PATCH 2/2 iio: chemical: sgpxx: triggered buffer support
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.
Triggered buffer support is only enabled when IIO_BUFFER is set.
--- drivers/iio/chemical/Kconfig | 3 +++ drivers/iio/chemical/sgpxx.c | 38 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 41 insertions(+)
drivers/iio/chemical/Kconfig +3 -0
@@ -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.
drivers/iio/chemical/sgpxx.c +38 -0
@@ -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 +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 */ devm_iio_device_unregister (&client->dev, indio_dev);return 0 ; }
Signature · 1 line
2.14.1
Jonathan Cameron
maintainerQuoted 3 lines · On Tue, 21 Nov 2017 17:11:29 +0100 Andreas Brauchli…
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...
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.
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
Quoted 45 lines
Triggered buffer support is only enabled when
IIO_BUFFERis set.Signed-off-by Andreas Brauchli <[email protected]>--- drivers/iio/chemical/Kconfig | 3 +++ drivers/iio/chemical/sgpxx.c | 38 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 41 insertions(+)drivers/iio/chemical/Kconfig +3
-0@@ -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.drivers/iio/chemical/sgpxx.c +4
-0@@ -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
drivers/iio/chemical/sgpxx.c static const struct of_device_id sgp_dt_ids[] = { · line 796
Quoted 4 lines
drivers/iio/chemical/sgpxx.c +1
-0@@ -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.
Quoted 41 lines
drivers/iio/chemical/sgpxx.c +30
-0+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;
drivers/iio/chemical/sgpxx.c static int sgp_remove(struct i2c_client *client) · line 903
Quoted 6 lines
drivers/iio/chemical/sgpxx.c +3
-0@@ -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.
Quoted 3 lines
drivers/iio/chemical/sgpxx.c +0
-0devm_iio_device_unregister (&client->dev, indio_dev);return 0 ; }
Quoted 7 lines · Jonathan Cameron
Quoted 3 lines · On Tue, 21 Nov 2017 17:11:29 +0100 Andreas Brauchli…
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.
Quoted 6 lines
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.
Quoted 5 lines
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
Quoted 137 lines
You could decide to add stubs to
include/linux/iio/triggered_buffer.h/buffer.hand then use
__maybe_unusuedto 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
Quoted 49 lines
Triggered buffer support is only enabled when
IIO_BUFFERis set.Signed-off-by Andreas Brauchli <[email protected]>--- drivers/iio/chemical/Kconfig | 3 +++ drivers/iio/chemical/sgpxx.c | 38 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 41 insertions(+)drivers/iio/chemical/Kconfig +1
-0@@ -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 thefollowing
drivers/iio/chemical/Kconfig +2
-0Sensirion 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.drivers/iio/chemical/sgpxx.c +4
-0@@ -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 2drivers/iio/chemical/sgpxx.c static const struct of_device_id sgp_dt_ids[] · line 797
Quoted 5 lines
drivers/iio/chemical/sgpxx.c +1
-0@@ -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.
Quoted 44 lines
drivers/iio/chemical/sgpxx.c +19
-0+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,drivers/iio/chemical/sgpxx.c +11
-0const 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);drivers/iio/chemical/sgpxx.c +0
-0if (!ret)return ret;drivers/iio/chemical/sgpxx.c static int sgp_remove(struct i2c_client · line 904
Quoted 7 lines
drivers/iio/chemical/sgpxx.c +3
-0@@ -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.Quoted 3 lines
drivers/iio/chemical/sgpxx.c +0
-0devm_iio_device_unregister (&client->dev, indio_dev);return 0 ;}