On Thu, 15 Jun 2017 11:54:24 +0300
Mikko Koivunen <mikko.koivunen@xxxxxxxxxxxxxxxxx> wrote:
> Set up trigger producer and triggered buffer if there is irq defined for
> device in device tree. Trigger producer triggers from rpr0521 drdy
> interrupt line. Trigger consumer reads rpr0521 data to scan buffer.
> Depends on previous commits of _scale and _offset.
>
> Signed-off-by: Mikko Koivunen <mikko.koivunen@xxxxxxxxxxxxxxxxx>
One more issue inline. Sorry, failed to notice this before I guess.
You can't call iio_trigger_poll in an interrupt thread. It 'kind'
of works, but will sometimes cause issues. A bug we've had to fix
a few times in the past when it hasn't been picked up in review.
Jonathan
> ---
> Patch v2->v3 changes:
> .shift = 0 removed
> reordered functions to remove forward declarations
> whitespace changes
> refactored some update_bits->write, no "err = err || *"-pattern
> refactored trigger_consumer_handler
> reordered _probe() and _remove()
> added int clear on poweroff()
> checkpatch.pl OK
> Tested on LeMaker HiKey with AOSP7.1 kernel 4.4.
> Builds ok with torvalds/linux feb 27.
>
> Patch v3->v4 changes:
> Moved sensor on/off commands to buffer preenable and postdisable
> - Since drdy happens only on measurement data ready and register writes
> are cached, the trigger producer doesn't care of suspend/resume state.
> available_scan_masks added
> indent fix
> checkpatch.pl OK
> Lightly re-tested on LeMaker HiKey with AOSP7.1 kernel 4.4.
> Builds ok with torvalds/linux feb 27.
>
> Patch v5->v6 changes:
> Changed base from kernel 4.4 to 4.9
> - Using iio_device_claim_direct_mode()
> - Using iio_trigger_using_own()
> - Changed trigger/buffer to devm_
> Added shared irq support
> - divided trigger producer to h/thread
> - divided trigger consumer to h/thread
> - Using irq_timestamp when available
> Removed default trigger
> checkpatch.pl OK
> Lightly re-tested on LeMaker HiKey with AOSP7.1 kernel 4.9.29
> Builds ok with torvalds/linux feb 27.
>
> drivers/iio/light/rpr0521.c | 332 ++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 325 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/iio/light/rpr0521.c b/drivers/iio/light/rpr0521.c
> index 9d0c2e8..ce576be 100644
> --- a/drivers/iio/light/rpr0521.c
> +++ b/drivers/iio/light/rpr0521.c
> @@ -9,7 +9,7 @@
> *
> * IIO driver for RPR-0521RS (7-bit I2C slave address 0x38).
> *
> - * TODO: illuminance channel, buffer
> + * TODO: illuminance channel
> */
>
> #include <linux/module.h>
> @@ -20,6 +20,10 @@
> #include <linux/acpi.h>
>
> #include <linux/iio/iio.h>
> +#include <linux/iio/buffer.h>
> +#include <linux/iio/trigger.h>
> +#include <linux/iio/trigger_consumer.h>
> +#include <linux/iio/triggered_buffer.h>
> #include <linux/iio/sysfs.h>
> #include <linux/pm_runtime.h>
>
> @@ -30,6 +34,7 @@
> #define RPR0521_REG_PXS_DATA 0x44 /* 16-bit, little endian */
> #define RPR0521_REG_ALS_DATA0 0x46 /* 16-bit, little endian */
> #define RPR0521_REG_ALS_DATA1 0x48 /* 16-bit, little endian */
> +#define RPR0521_REG_INTERRUPT 0x4A
> #define RPR0521_REG_PS_OFFSET_LSB 0x53
> #define RPR0521_REG_ID 0x92
>
> @@ -42,16 +47,31 @@
> #define RPR0521_ALS_DATA1_GAIN_SHIFT 2
> #define RPR0521_PXS_GAIN_MASK GENMASK(5, 4)
> #define RPR0521_PXS_GAIN_SHIFT 4
> +#define RPR0521_PXS_PERSISTENCE_MASK GENMASK(3, 0)
> +#define RPR0521_INTERRUPT_INT_TRIG_PS_MASK BIT(0)
> +#define RPR0521_INTERRUPT_INT_TRIG_ALS_MASK BIT(1)
> +#define RPR0521_INTERRUPT_INT_REASSERT_MASK BIT(3)
> +#define RPR0521_INTERRUPT_ALS_INT_STATUS_MASK BIT(6)
> +#define RPR0521_INTERRUPT_PS_INT_STATUS_MASK BIT(7)
>
> #define RPR0521_MODE_ALS_ENABLE BIT(7)
> #define RPR0521_MODE_ALS_DISABLE 0x00
> #define RPR0521_MODE_PXS_ENABLE BIT(6)
> #define RPR0521_MODE_PXS_DISABLE 0x00
> +#define RPR0521_PXS_PERSISTENCE_DRDY 0x00
> +
> +#define RPR0521_INTERRUPT_INT_TRIG_PS_ENABLE BIT(0)
> +#define RPR0521_INTERRUPT_INT_TRIG_PS_DISABLE 0x00
> +#define RPR0521_INTERRUPT_INT_TRIG_ALS_ENABLE BIT(1)
> +#define RPR0521_INTERRUPT_INT_TRIG_ALS_DISABLE 0x00
> +#define RPR0521_INTERRUPT_INT_REASSERT_ENABLE BIT(3)
> +#define RPR0521_INTERRUPT_INT_REASSERT_DISABLE 0x00
>
> #define RPR0521_MANUFACT_ID 0xE0
> #define RPR0521_DEFAULT_MEAS_TIME 0x06 /* ALS - 100ms, PXS - 100ms */
>
> #define RPR0521_DRV_NAME "RPR0521"
> +#define RPR0521_IRQ_NAME "rpr0521_event"
> #define RPR0521_REGMAP_NAME "rpr0521_regmap"
>
> #define RPR0521_SLEEP_DELAY_MS 2000
> @@ -167,6 +187,9 @@ struct rpr0521_data {
> bool als_dev_en;
> bool pxs_dev_en;
>
> + struct iio_trigger *drdy_trigger0;
> + s64 irq_timestamp;
> +
> /* optimize runtime pm ops - enable/disable device only if needed */
> bool als_ps_need_en;
> bool pxs_ps_need_en;
> @@ -196,6 +219,19 @@ static const struct attribute_group rpr0521_attribute_group = {
> .attrs = rpr0521_attributes,
> };
>
> +/* Order of the channel data in buffer */
> +enum rpr0521_scan_index_order {
> + RPR0521_CHAN_INDEX_PXS,
> + RPR0521_CHAN_INDEX_BOTH,
> + RPR0521_CHAN_INDEX_IR,
> +};
> +
> +static const unsigned long rpr0521_available_scan_masks[] = {
> + BIT(RPR0521_CHAN_INDEX_PXS) | BIT(RPR0521_CHAN_INDEX_BOTH) |
> + BIT(RPR0521_CHAN_INDEX_IR),
> + 0
> +};
> +
> static const struct iio_chan_spec rpr0521_channels[] = {
> {
> .type = IIO_PROXIMITY,
> @@ -204,6 +240,13 @@ static const struct iio_chan_spec rpr0521_channels[] = {
> BIT(IIO_CHAN_INFO_OFFSET) |
> BIT(IIO_CHAN_INFO_SCALE),
> .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),
> + .scan_index = RPR0521_CHAN_INDEX_PXS,
> + .scan_type = {
> + .sign = 'u',
> + .realbits = 16,
> + .storagebits = 16,
> + .endianness = IIO_LE,
> + },
> },
> {
> .type = IIO_INTENSITY,
> @@ -213,6 +256,13 @@ static const struct iio_chan_spec rpr0521_channels[] = {
> .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> BIT(IIO_CHAN_INFO_SCALE),
> .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),
> + .scan_index = RPR0521_CHAN_INDEX_BOTH,
> + .scan_type = {
> + .sign = 'u',
> + .realbits = 16,
> + .storagebits = 16,
> + .endianness = IIO_LE,
> + },
> },
> {
> .type = IIO_INTENSITY,
> @@ -222,6 +272,13 @@ static const struct iio_chan_spec rpr0521_channels[] = {
> .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> BIT(IIO_CHAN_INFO_SCALE),
> .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),
> + .scan_index = RPR0521_CHAN_INDEX_IR,
> + .scan_type = {
> + .sign = 'u',
> + .realbits = 16,
> + .storagebits = 16,
> + .endianness = IIO_LE,
> + },
> },
> };
>
> @@ -330,6 +387,194 @@ static int rpr0521_set_power_state(struct rpr0521_data *data, bool on,
> return 0;
> }
>
> +/* Interrupt register tells if this sensor caused the interrupt or not. */
> +static inline bool rpr0521_is_triggered(struct rpr0521_data *data)
> +{
> + int ret;
> + int reg;
> +
> + ret = regmap_read(data->regmap, RPR0521_REG_INTERRUPT, ®);
> + if (ret < 0)
> + return false; /* Reg read failed. */
> + if (reg &
> + (RPR0521_INTERRUPT_ALS_INT_STATUS_MASK |
> + RPR0521_INTERRUPT_PS_INT_STATUS_MASK))
> + return true;
> + else
> + return false; /* Int not from this sensor. */
> +}
> +
> +/* IRQ to trigger handler */
> +static irqreturn_t rpr0521_drdy_irq_handler(int irq, void *private)
> +{
> + struct iio_dev *indio_dev = private;
> + struct rpr0521_data *data = iio_priv(indio_dev);
> +
> + data->irq_timestamp = iio_get_time_ns(indio_dev);
> + /*
> + * We need to wake the thread to read the interrupt reg. It
> + * is not possible to do that here because regmap_read takes a
> + * mutex.
> + */
> +
> + return IRQ_WAKE_THREAD;
> +}
> +
> +static irqreturn_t rpr0521_drdy_irq_thread(int irq, void *private)
> +{
> + struct iio_dev *indio_dev = private;
> + struct rpr0521_data *data = iio_priv(indio_dev);
> +
> + if (rpr0521_is_triggered(data)) {
> + iio_trigger_poll(data->drdy_trigger0);
When called from a thread, you need to use (the rather missnamed)
iio_trigger_poll_chained
Note it won't then call the top half handler at all, but it's the
best we can do without a really nasty and expensive dance to get
back into interrupt context.
> + return IRQ_HANDLED;
> + }
> + data->irq_timestamp = 0;
> +
> + return IRQ_NONE;
> +}
> +
> +static irqreturn_t rpr0521_trigger_consumer_store_time(int irq, void *p)
> +{
> + struct iio_poll_func *pf = p;
> + struct iio_dev *indio_dev = pf->indio_dev;
> + struct rpr0521_data *data = iio_priv(indio_dev);
> +
> + /* Store time if not already stored. */
> + if (iio_trigger_using_own(indio_dev) && data->irq_timestamp) {
This won't be called if we are calling from the threaded call.
> + pf->timestamp = data->irq_timestamp;
> + data->irq_timestamp = 0;
> + } else {
> + pf->timestamp = iio_get_time_ns(indio_dev);
> + }
> +
> + return IRQ_WAKE_THREAD;
> +}
> +
> +static irqreturn_t rpr0521_trigger_consumer_handler(int irq, void *p)
> +{
> + struct iio_poll_func *pf = p;
> + struct iio_dev *indio_dev = pf->indio_dev;
> + struct rpr0521_data *data = iio_priv(indio_dev);
> + int err;
> +
> + u8 buffer[16]; /* 3 16-bit channels + padding + ts */
> +
> + err = regmap_bulk_read(data->regmap, RPR0521_REG_PXS_DATA,
> + &buffer,
> + (3 * 2) + 1); /* 3 * 16-bit + (discarded) int clear reg. */
> + if (!err)
> + iio_push_to_buffers_with_timestamp(indio_dev,
> + buffer, pf->timestamp);
> + else
> + dev_err(&data->client->dev,
> + "Trigger consumer can't read from sensor.\n");
> +
> + iio_trigger_notify_done(indio_dev->trig);
> +
> + return IRQ_HANDLED;
> +}
> +
> +static int rpr0521_write_int_enable(struct rpr0521_data *data)
> +{
> + int err;
> +
> + /* Interrupt after each measurement */
> + err = regmap_update_bits(data->regmap, RPR0521_REG_PXS_CTRL,
> + RPR0521_PXS_PERSISTENCE_MASK,
> + RPR0521_PXS_PERSISTENCE_DRDY);
> + if (err) {
> + dev_err(&data->client->dev, "PS control reg write fail.\n");
> + return -EBUSY;
> + }
> +
> + /* Ignore latch and mode because of drdy */
> + err = regmap_write(data->regmap, RPR0521_REG_INTERRUPT,
> + RPR0521_INTERRUPT_INT_REASSERT_DISABLE |
> + RPR0521_INTERRUPT_INT_TRIG_ALS_DISABLE |
> + RPR0521_INTERRUPT_INT_TRIG_PS_ENABLE
> + );
> + if (err) {
> + dev_err(&data->client->dev, "Interrupt setup write fail.\n");
> + return -EBUSY;
> + }
> +
> + return 0;
> +}
> +
> +static int rpr0521_write_int_disable(struct rpr0521_data *data)
> +{
> + /* Don't care of clearing mode, assert and latch. */
> + return regmap_write(data->regmap, RPR0521_REG_INTERRUPT,
> + RPR0521_INTERRUPT_INT_TRIG_ALS_DISABLE |
> + RPR0521_INTERRUPT_INT_TRIG_PS_DISABLE
> + );
> +}
> +
> +/*
> + * Trigger producer enable / disable. Note that there will be trigs only when
> + * measurement data is ready to be read.
> + */
> +static int rpr0521_pxs_drdy_set_state(struct iio_trigger *trigger,
> + bool enable_drdy)
> +{
> + struct iio_dev *indio_dev = iio_trigger_get_drvdata(trigger);
> + struct rpr0521_data *data = iio_priv(indio_dev);
> + int err;
> +
> + if (enable_drdy)
> + err = rpr0521_write_int_enable(data);
> + else
> + err = rpr0521_write_int_disable(data);
> + if (err)
> + dev_err(&data->client->dev, "rpr0521_pxs_drdy_set_state failed\n");
> +
> + return err;
> +}
> +
> +static const struct iio_trigger_ops rpr0521_trigger_ops = {
> + .set_trigger_state = rpr0521_pxs_drdy_set_state,
> + .owner = THIS_MODULE,
> + };
> +
> +
> +static int rpr0521_buffer_preenable(struct iio_dev *indio_dev)
> +{
> + int err;
> + struct rpr0521_data *data = iio_priv(indio_dev);
> +
> + mutex_lock(&data->lock);
> + err = rpr0521_set_power_state(data, true,
> + (RPR0521_MODE_PXS_MASK | RPR0521_MODE_ALS_MASK));
> + mutex_unlock(&data->lock);
> + if (err)
> + dev_err(&data->client->dev, "_buffer_preenable fail\n");
> +
> + return err;
> +}
> +
> +static int rpr0521_buffer_postdisable(struct iio_dev *indio_dev)
> +{
> + int err;
> + struct rpr0521_data *data = iio_priv(indio_dev);
> +
> + mutex_lock(&data->lock);
> + err = rpr0521_set_power_state(data, false,
> + (RPR0521_MODE_PXS_MASK | RPR0521_MODE_ALS_MASK));
> + mutex_unlock(&data->lock);
> + if (err)
> + dev_err(&data->client->dev, "_buffer_postdisable fail\n");
> +
> + return err;
> +}
> +
> +static const struct iio_buffer_setup_ops rpr0521_buffer_setup_ops = {
> + .preenable = rpr0521_buffer_preenable,
> + .postenable = iio_triggered_buffer_postenable,
> + .predisable = iio_triggered_buffer_predisable,
> + .postdisable = rpr0521_buffer_postdisable,
> +};
> +
> static int rpr0521_get_gain(struct rpr0521_data *data, int chan,
> int *val, int *val2)
> {
> @@ -473,34 +718,39 @@ static int rpr0521_read_raw(struct iio_dev *indio_dev,
> {
> struct rpr0521_data *data = iio_priv(indio_dev);
> int ret;
> + int busy;
> u8 device_mask;
> __le16 raw_data;
>
> switch (mask) {
> +
Shouldn't be seeing whitespace changes in here...
> case IIO_CHAN_INFO_RAW:
> if (chan->type != IIO_INTENSITY && chan->type != IIO_PROXIMITY)
> return -EINVAL;
>
> + busy = iio_device_claim_direct_mode(indio_dev);
> + if (busy)
> + return -EBUSY;
> device_mask = rpr0521_data_reg[chan->address].device_mask;
>
> mutex_lock(&data->lock);
> ret = rpr0521_set_power_state(data, true, device_mask);
> - if (ret < 0) {
> - mutex_unlock(&data->lock);
> - return ret;
> - }
> + if (ret < 0)
> + goto rpr0521_read_raw_out;
>
> ret = regmap_bulk_read(data->regmap,
> rpr0521_data_reg[chan->address].address,
> &raw_data, sizeof(raw_data));
> if (ret < 0) {
> rpr0521_set_power_state(data, false, device_mask);
> - mutex_unlock(&data->lock);
> - return ret;
> + goto rpr0521_read_raw_out;
> }
>
> ret = rpr0521_set_power_state(data, false, device_mask);
> +
> +rpr0521_read_raw_out:
> mutex_unlock(&data->lock);
> + iio_device_release_direct_mode(indio_dev);
> if (ret < 0)
> return ret;
>
> @@ -617,12 +867,15 @@ static int rpr0521_init(struct rpr0521_data *data)
> return ret;
> #endif
>
> + data->irq_timestamp = 0;
> +
> return 0;
> }
>
> static int rpr0521_poweroff(struct rpr0521_data *data)
> {
> int ret;
> + int tmp;
>
> ret = regmap_update_bits(data->regmap, RPR0521_REG_MODE_CTRL,
> RPR0521_MODE_ALS_MASK |
> @@ -635,6 +888,16 @@ static int rpr0521_poweroff(struct rpr0521_data *data)
> data->als_dev_en = false;
> data->pxs_dev_en = false;
>
> + /*
> + * Int pin keeps state after power off. Set pin to high impedance
> + * mode to prevent power drain.
> + */
> + ret = regmap_read(data->regmap, RPR0521_REG_INTERRUPT, &tmp);
> + if (ret) {
> + dev_err(&data->client->dev, "Failed to reset int pin.\n");
> + return ret;
> + }
> +
> return 0;
> }
>
> @@ -707,6 +970,61 @@ static int rpr0521_probe(struct i2c_client *client,
> pm_runtime_set_autosuspend_delay(&client->dev, RPR0521_SLEEP_DELAY_MS);
> pm_runtime_use_autosuspend(&client->dev);
>
> + /*
> + * If sensor write/read is needed in _probe after _use_autosuspend,
> + * sensor needs to be _resumed first using rpr0521_set_power_state().
> + */
> +
> + /* IRQ to trigger setup */
> + if (client->irq) {
> + /* Trigger0 producer setup */
> + data->drdy_trigger0 = devm_iio_trigger_alloc(
> + indio_dev->dev.parent,
> + "%s-dev%d", indio_dev->name, indio_dev->id);
> + if (!data->drdy_trigger0) {
> + ret = -ENOMEM;
> + goto err_pm_disable;
> + }
> + data->drdy_trigger0->dev.parent = indio_dev->dev.parent;
> + data->drdy_trigger0->ops = &rpr0521_trigger_ops;
> + indio_dev->available_scan_masks = rpr0521_available_scan_masks;
> + iio_trigger_set_drvdata(data->drdy_trigger0, indio_dev);
> +
> + /* Ties irq to trigger producer handler. */
> + ret = devm_request_threaded_irq(&client->dev, client->irq,
> + rpr0521_drdy_irq_handler, rpr0521_drdy_irq_thread,
> + IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
> + RPR0521_IRQ_NAME, indio_dev);
> + if (ret < 0) {
> + dev_err(&client->dev, "request irq %d for trigger0 failed\n",
> + client->irq);
> + goto err_pm_disable;
> + }
> +
> + ret = devm_iio_trigger_register(indio_dev->dev.parent,
> + data->drdy_trigger0);
> + if (ret) {
> + dev_err(&client->dev, "iio trigger register failed\n");
> + goto err_pm_disable;
> + }
> +
> + /*
> + * Now whole pipe from physical interrupt (irq defined by
> + * devicetree to device) to trigger0 output is set up.
> + */
> +
> + /* Trigger consumer setup */
> + ret = devm_iio_triggered_buffer_setup(indio_dev->dev.parent,
> + indio_dev,
> + rpr0521_trigger_consumer_store_time,
> + rpr0521_trigger_consumer_handler,
> + &rpr0521_buffer_setup_ops);
> + if (ret < 0) {
> + dev_err(&client->dev, "iio triggered buffer setup failed\n");
> + goto err_pm_disable;
> + }
> + }
> +
> ret = iio_device_register(indio_dev);
> if (ret)
> goto err_pm_disable;
--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
