* [PATCH 2/6] iio: accel: kxcjk-1013: Use try_reenable to ack intr
2014-07-17 0:42 [PATCH 1/6] iio: accel: kxcjk-1013: Fix setting frequency Srinivas Pandruvada
@ 2014-07-17 0:42 ` Srinivas Pandruvada
2014-07-20 15:20 ` Jonathan Cameron
2014-07-17 0:42 ` [PATCH 3/6] iio: accel: kxcjk-1013: support runtime pm Srinivas Pandruvada
` (4 subsequent siblings)
5 siblings, 1 reply; 21+ messages in thread
From: Srinivas Pandruvada @ 2014-07-17 0:42 UTC (permalink / raw)
To: jic23; +Cc: linux-iio, Srinivas Pandruvada
This chip needs explicit interrupt ack, introducing try_reenable
callback. Also removed separate function to ack interrupt as this
doesn't add any value.
Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
---
drivers/iio/accel/kxcjk-1013.c | 33 ++++++++++++++++-----------------
1 file changed, 16 insertions(+), 17 deletions(-)
diff --git a/drivers/iio/accel/kxcjk-1013.c b/drivers/iio/accel/kxcjk-1013.c
index 4702aeb..bff5161 100644
--- a/drivers/iio/accel/kxcjk-1013.c
+++ b/drivers/iio/accel/kxcjk-1013.c
@@ -138,19 +138,6 @@ static int kxcjk1013_set_mode(struct kxcjk1013_data *data,
return 0;
}
-static int kxcjk1013_chip_ack_intr(struct kxcjk1013_data *data)
-{
- int ret;
-
- ret = i2c_smbus_read_byte_data(data->client, KXCJK1013_REG_INT_REL);
- if (ret < 0) {
- dev_err(&data->client->dev, "Error writing reg_int_rel\n");
- return ret;
- }
-
- return ret;
-}
-
static int kxcjk1013_chip_init(struct kxcjk1013_data *data)
{
int ret;
@@ -498,15 +485,11 @@ static irqreturn_t kxcjk1013_trigger_handler(int irq, void *p)
indio_dev->masklength) {
ret = kxcjk1013_get_acc_reg(data, bit);
if (ret < 0) {
- kxcjk1013_chip_ack_intr(data);
mutex_unlock(&data->mutex);
goto err;
}
data->buffer[i++] = ret;
}
-
- kxcjk1013_chip_ack_intr(data);
-
mutex_unlock(&data->mutex);
iio_push_to_buffers_with_timestamp(indio_dev, data->buffer,
@@ -517,6 +500,21 @@ err:
return IRQ_HANDLED;
}
+static int kxcjk1013_trig_try_reen(struct iio_trigger *trig)
+{
+ struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
+ struct kxcjk1013_data *data = iio_priv(indio_dev);
+ int ret;
+
+ ret = i2c_smbus_read_byte_data(data->client, KXCJK1013_REG_INT_REL);
+ if (ret < 0) {
+ dev_err(&data->client->dev, "Error reading reg_int_rel\n");
+ return ret;
+ }
+
+ return 0;
+}
+
static int kxcjk1013_data_rdy_trigger_set_state(struct iio_trigger *trig,
bool state)
{
@@ -543,6 +541,7 @@ static int kxcjk1013_data_rdy_trigger_set_state(struct iio_trigger *trig,
static const struct iio_trigger_ops kxcjk1013_trigger_ops = {
.set_trigger_state = kxcjk1013_data_rdy_trigger_set_state,
+ .try_reenable = kxcjk1013_trig_try_reen,
.owner = THIS_MODULE,
};
--
1.7.11.7
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH 2/6] iio: accel: kxcjk-1013: Use try_reenable to ack intr
2014-07-17 0:42 ` [PATCH 2/6] iio: accel: kxcjk-1013: Use try_reenable to ack intr Srinivas Pandruvada
@ 2014-07-20 15:20 ` Jonathan Cameron
0 siblings, 0 replies; 21+ messages in thread
From: Jonathan Cameron @ 2014-07-20 15:20 UTC (permalink / raw)
To: Srinivas Pandruvada; +Cc: linux-iio
On 17/07/14 01:42, Srinivas Pandruvada wrote:
> This chip needs explicit interrupt ack, introducing try_reenable
> callback. Also removed separate function to ack interrupt as this
> doesn't add any value.
>
> Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
Definitely neater this way, though would have worked before as acked in the
trigger handler... Patch description seems to suggest it wouldn't?
Applied to the togreg branch of iio.git
J
> ---
> drivers/iio/accel/kxcjk-1013.c | 33 ++++++++++++++++-----------------
> 1 file changed, 16 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/iio/accel/kxcjk-1013.c b/drivers/iio/accel/kxcjk-1013.c
> index 4702aeb..bff5161 100644
> --- a/drivers/iio/accel/kxcjk-1013.c
> +++ b/drivers/iio/accel/kxcjk-1013.c
> @@ -138,19 +138,6 @@ static int kxcjk1013_set_mode(struct kxcjk1013_data *data,
> return 0;
> }
>
> -static int kxcjk1013_chip_ack_intr(struct kxcjk1013_data *data)
> -{
> - int ret;
> -
> - ret = i2c_smbus_read_byte_data(data->client, KXCJK1013_REG_INT_REL);
> - if (ret < 0) {
> - dev_err(&data->client->dev, "Error writing reg_int_rel\n");
> - return ret;
> - }
> -
> - return ret;
> -}
> -
> static int kxcjk1013_chip_init(struct kxcjk1013_data *data)
> {
> int ret;
> @@ -498,15 +485,11 @@ static irqreturn_t kxcjk1013_trigger_handler(int irq, void *p)
> indio_dev->masklength) {
> ret = kxcjk1013_get_acc_reg(data, bit);
> if (ret < 0) {
> - kxcjk1013_chip_ack_intr(data);
> mutex_unlock(&data->mutex);
> goto err;
> }
> data->buffer[i++] = ret;
> }
> -
> - kxcjk1013_chip_ack_intr(data);
> -
> mutex_unlock(&data->mutex);
>
> iio_push_to_buffers_with_timestamp(indio_dev, data->buffer,
> @@ -517,6 +500,21 @@ err:
> return IRQ_HANDLED;
> }
>
> +static int kxcjk1013_trig_try_reen(struct iio_trigger *trig)
> +{
> + struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
> + struct kxcjk1013_data *data = iio_priv(indio_dev);
> + int ret;
> +
> + ret = i2c_smbus_read_byte_data(data->client, KXCJK1013_REG_INT_REL);
> + if (ret < 0) {
> + dev_err(&data->client->dev, "Error reading reg_int_rel\n");
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> static int kxcjk1013_data_rdy_trigger_set_state(struct iio_trigger *trig,
> bool state)
> {
> @@ -543,6 +541,7 @@ static int kxcjk1013_data_rdy_trigger_set_state(struct iio_trigger *trig,
>
> static const struct iio_trigger_ops kxcjk1013_trigger_ops = {
> .set_trigger_state = kxcjk1013_data_rdy_trigger_set_state,
> + .try_reenable = kxcjk1013_trig_try_reen,
> .owner = THIS_MODULE,
> };
>
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 3/6] iio: accel: kxcjk-1013: support runtime pm
2014-07-17 0:42 [PATCH 1/6] iio: accel: kxcjk-1013: Fix setting frequency Srinivas Pandruvada
2014-07-17 0:42 ` [PATCH 2/6] iio: accel: kxcjk-1013: Use try_reenable to ack intr Srinivas Pandruvada
@ 2014-07-17 0:42 ` Srinivas Pandruvada
2014-07-20 15:37 ` Jonathan Cameron
2014-07-17 0:42 ` [PATCH 4/6] iio: accel: kxcjk-1013: Set adjustable range Srinivas Pandruvada
` (3 subsequent siblings)
5 siblings, 1 reply; 21+ messages in thread
From: Srinivas Pandruvada @ 2014-07-17 0:42 UTC (permalink / raw)
To: jic23; +Cc: linux-iio, Srinivas Pandruvada
In an effort to improve raw read performance and at the same time enter
low power state at every possible chance.
For raw reads, it will keep the system powered on for a user specified
max time, this will help read multiple samples without power on/off
sequence.
When runtime pm is not enabled, then it fallbacks to current scheme
of on/off after each read.
Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
---
drivers/iio/accel/kxcjk-1013.c | 198 +++++++++++++++++++++++++++++++++--------
1 file changed, 160 insertions(+), 38 deletions(-)
diff --git a/drivers/iio/accel/kxcjk-1013.c b/drivers/iio/accel/kxcjk-1013.c
index bff5161..4912143 100644
--- a/drivers/iio/accel/kxcjk-1013.c
+++ b/drivers/iio/accel/kxcjk-1013.c
@@ -21,6 +21,8 @@
#include <linux/string.h>
#include <linux/acpi.h>
#include <linux/gpio/consumer.h>
+#include <linux/pm.h>
+#include <linux/pm_runtime.h>
#include <linux/iio/iio.h>
#include <linux/iio/sysfs.h>
#include <linux/iio/buffer.h>
@@ -71,15 +73,21 @@
#define KXCJK1013_DATA_MASK_12_BIT 0x0FFF
#define KXCJK1013_MAX_STARTUP_TIME_US 100000
+#define KXCJK1013_SLEEP_DELAY_MS 2000
+static int kxcjk1013_power_off_delay_ms = KXCJK1013_SLEEP_DELAY_MS;
+module_param(kxcjk1013_power_off_delay_ms, int, 0644);
+MODULE_PARM_DESC(kxcjk1013_power_off_delay_ms,
+ "KXCJK1013 accelerometer power of delay in milli seconds.");
+
struct kxcjk1013_data {
struct i2c_client *client;
struct iio_trigger *trig;
bool trig_mode;
struct mutex mutex;
s16 buffer[8];
- int power_state;
u8 odr_bits;
bool active_high_intr;
+ bool trigger_on;
};
enum kxcjk1013_axis {
@@ -138,6 +146,25 @@ static int kxcjk1013_set_mode(struct kxcjk1013_data *data,
return 0;
}
+static int kxcjk1013_get_mode(struct kxcjk1013_data *data,
+ enum kxcjk1013_mode *mode)
+{
+ int ret;
+
+ ret = i2c_smbus_read_byte_data(data->client, KXCJK1013_REG_CTRL1);
+ if (ret < 0) {
+ dev_err(&data->client->dev, "Error reading reg_ctrl1\n");
+ return ret;
+ }
+
+ if (ret & KXCJK1013_REG_CTRL1_BIT_PC1)
+ *mode = OPERATION;
+ else
+ *mode = STANDBY;
+
+ return 0;
+}
+
static int kxcjk1013_chip_init(struct kxcjk1013_data *data)
{
int ret;
@@ -204,10 +231,53 @@ static int kxcjk1013_chip_init(struct kxcjk1013_data *data)
return 0;
}
+static int kxcjk1013_set_power_state(struct kxcjk1013_data *data, bool on)
+{
+ int ret;
+
+#ifdef CONFIG_PM_RUNTIME
+ if (on)
+ ret = pm_runtime_get_sync(&data->client->dev);
+ else {
+ pm_runtime_put_noidle(&data->client->dev);
+ ret = pm_schedule_suspend(&data->client->dev,
+ kxcjk1013_power_off_delay_ms);
+ }
+#else
+ if (on) {
+ ret = kxcjk1013_set_mode(data, OPERATION);
+ if (!ret) {
+ int sleep_val;
+
+ sleep_val = kxcjk1013_get_startup_times(data);
+ if (sleep_val < 20000)
+ usleep_range(sleep_val, 20000);
+ else
+ msleep_interruptible(sleep_val/1000);
+
+ }
+ } else
+ ret = kxcjk1013_set_mode(data, STANDBY);
+#endif
+
+ if (ret < 0) {
+ dev_err(&data->client->dev,
+ "Failed: kxcjk1013_set_power_state for %d\n", on);
+ return ret;
+ }
+
+ return 0;
+}
+
static int kxcjk1013_chip_setup_interrupt(struct kxcjk1013_data *data,
bool status)
{
int ret;
+ enum kxcjk1013_mode store_mode;
+
+ ret = kxcjk1013_get_mode(data, &store_mode);
+ if (ret < 0)
+ return ret;
/* This is requirement by spec to change state to STANDBY */
ret = kxcjk1013_set_mode(data, STANDBY);
@@ -250,7 +320,13 @@ static int kxcjk1013_chip_setup_interrupt(struct kxcjk1013_data *data,
return ret;
}
- return ret;
+ if (store_mode == OPERATION) {
+ ret = kxcjk1013_set_mode(data, OPERATION);
+ if (ret < 0)
+ return ret;
+ }
+
+ return 0;
}
static int kxcjk1013_convert_freq_to_bit(int val, int val2)
@@ -271,6 +347,11 @@ static int kxcjk1013_set_odr(struct kxcjk1013_data *data, int val, int val2)
{
int ret;
int odr_bits;
+ enum kxcjk1013_mode store_mode;
+
+ ret = kxcjk1013_get_mode(data, &store_mode);
+ if (ret < 0)
+ return ret;
odr_bits = kxcjk1013_convert_freq_to_bit(val, val2);
if (odr_bits < 0)
@@ -290,9 +371,7 @@ static int kxcjk1013_set_odr(struct kxcjk1013_data *data, int val, int val2)
data->odr_bits = odr_bits;
- /* Check, if the ODR is changed after data enable */
- if (data->power_state) {
- /* Set the state back to operation */
+ if (store_mode == OPERATION) {
ret = kxcjk1013_set_mode(data, OPERATION);
if (ret < 0)
return ret;
@@ -356,29 +435,25 @@ static int kxcjk1013_read_raw(struct iio_dev *indio_dev,
if (iio_buffer_enabled(indio_dev))
ret = -EBUSY;
else {
- int sleep_val;
-
- ret = kxcjk1013_set_mode(data, OPERATION);
+ ret = kxcjk1013_set_power_state(data, true);
if (ret < 0) {
mutex_unlock(&data->mutex);
return ret;
}
- ++data->power_state;
- sleep_val = kxcjk1013_get_startup_times(data);
- if (sleep_val < 20000)
- usleep_range(sleep_val, 20000);
- else
- msleep_interruptible(sleep_val/1000);
ret = kxcjk1013_get_acc_reg(data, chan->scan_index);
- if (--data->power_state == 0)
- kxcjk1013_set_mode(data, STANDBY);
+ if (ret < 0) {
+ kxcjk1013_set_power_state(data, false);
+ mutex_unlock(&data->mutex);
+ return ret;
+ }
+ *val = sign_extend32(ret >> 4, 11);
+ ret = kxcjk1013_set_power_state(data, false);
}
mutex_unlock(&data->mutex);
if (ret < 0)
return ret;
- *val = sign_extend32(ret >> 4, 11);
return IIO_VAL_INT;
case IIO_CHAN_INFO_SCALE:
@@ -520,20 +595,21 @@ static int kxcjk1013_data_rdy_trigger_set_state(struct iio_trigger *trig,
{
struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
struct kxcjk1013_data *data = iio_priv(indio_dev);
+ int ret;
+
+ if (state && data->trigger_on)
+ return 0;
mutex_lock(&data->mutex);
- if (state) {
- kxcjk1013_chip_setup_interrupt(data, true);
- kxcjk1013_set_mode(data, OPERATION);
- ++data->power_state;
- } else {
- if (--data->power_state) {
+ ret = kxcjk1013_chip_setup_interrupt(data, state);
+ if (!ret) {
+ ret = kxcjk1013_set_power_state(data, state);
+ if (ret < 0) {
mutex_unlock(&data->mutex);
- return 0;
+ return ret;
}
- kxcjk1013_chip_setup_interrupt(data, false);
- kxcjk1013_set_mode(data, STANDBY);
}
+ data->trigger_on = state;
mutex_unlock(&data->mutex);
return 0;
@@ -660,14 +736,22 @@ static int kxcjk1013_probe(struct i2c_client *client,
}
}
- ret = devm_iio_device_register(&client->dev, indio_dev);
+ ret = iio_device_register(indio_dev);
if (ret < 0) {
dev_err(&client->dev, "unable to register iio device\n");
goto err_buffer_cleanup;
}
+ ret = pm_runtime_set_active(&client->dev);
+ if (ret)
+ goto err_iio_unregister;
+
+ pm_runtime_enable(&client->dev);
+
return 0;
+err_iio_unregister:
+ iio_device_unregister(indio_dev);
err_buffer_cleanup:
if (data->trig_mode)
iio_triggered_buffer_cleanup(indio_dev);
@@ -686,6 +770,12 @@ static int kxcjk1013_remove(struct i2c_client *client)
struct iio_dev *indio_dev = i2c_get_clientdata(client);
struct kxcjk1013_data *data = iio_priv(indio_dev);
+ pm_runtime_disable(&client->dev);
+ pm_runtime_set_suspended(&client->dev);
+ pm_runtime_put_noidle(&client->dev);
+
+ iio_device_unregister(indio_dev);
+
if (data->trig_mode) {
iio_triggered_buffer_cleanup(indio_dev);
iio_trigger_unregister(data->trig);
@@ -704,35 +794,67 @@ static int kxcjk1013_suspend(struct device *dev)
{
struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
struct kxcjk1013_data *data = iio_priv(indio_dev);
+ int ret;
mutex_lock(&data->mutex);
- kxcjk1013_set_mode(data, STANDBY);
+ ret = kxcjk1013_set_mode(data, STANDBY);
mutex_unlock(&data->mutex);
- return 0;
+ return ret;
}
static int kxcjk1013_resume(struct device *dev)
{
struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
struct kxcjk1013_data *data = iio_priv(indio_dev);
+ int ret = 0;
mutex_lock(&data->mutex);
+ /* Check, if the suspend occured while active */
+ if (data->trigger_on)
+ ret = kxcjk1013_set_mode(data, OPERATION);
+ mutex_unlock(&data->mutex);
- if (data->power_state)
- kxcjk1013_set_mode(data, OPERATION);
+ return ret;
+}
+#endif
- mutex_unlock(&data->mutex);
+#ifdef CONFIG_PM_RUNTIME
+static int kxcjk1013_runtime_suspend(struct device *dev)
+{
+ struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
+ struct kxcjk1013_data *data = iio_priv(indio_dev);
- return 0;
+ return kxcjk1013_set_mode(data, STANDBY);
}
-static SIMPLE_DEV_PM_OPS(kxcjk1013_pm_ops, kxcjk1013_suspend, kxcjk1013_resume);
-#define KXCJK1013_PM_OPS (&kxcjk1013_pm_ops)
-#else
-#define KXCJK1013_PM_OPS NULL
+static int kxcjk1013_runtime_resume(struct device *dev)
+{
+ struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
+ struct kxcjk1013_data *data = iio_priv(indio_dev);
+ int ret;
+ int sleep_val;
+
+ ret = kxcjk1013_set_mode(data, OPERATION);
+ if (ret < 0)
+ return ret;
+
+ sleep_val = kxcjk1013_get_startup_times(data);
+ if (sleep_val < 20000)
+ usleep_range(sleep_val, 20000);
+ else
+ msleep_interruptible(sleep_val/1000);
+
+ return 0;
+}
#endif
+static const struct dev_pm_ops kxcjk1013_pm_ops = {
+ SET_SYSTEM_SLEEP_PM_OPS(kxcjk1013_suspend, kxcjk1013_resume)
+ SET_RUNTIME_PM_OPS(kxcjk1013_runtime_suspend,
+ kxcjk1013_runtime_resume, NULL)
+};
+
static const struct acpi_device_id kx_acpi_match[] = {
{"KXCJ1013", 0},
{ },
@@ -750,7 +872,7 @@ static struct i2c_driver kxcjk1013_driver = {
.driver = {
.name = KXCJK1013_DRV_NAME,
.acpi_match_table = ACPI_PTR(kx_acpi_match),
- .pm = KXCJK1013_PM_OPS,
+ .pm = &kxcjk1013_pm_ops,
},
.probe = kxcjk1013_probe,
.remove = kxcjk1013_remove,
--
1.7.11.7
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH 3/6] iio: accel: kxcjk-1013: support runtime pm
2014-07-17 0:42 ` [PATCH 3/6] iio: accel: kxcjk-1013: support runtime pm Srinivas Pandruvada
@ 2014-07-20 15:37 ` Jonathan Cameron
2014-07-22 1:23 ` Rafael J. Wysocki
0 siblings, 1 reply; 21+ messages in thread
From: Jonathan Cameron @ 2014-07-20 15:37 UTC (permalink / raw)
To: Srinivas Pandruvada; +Cc: linux-iio, rafael.j.wysocki
On 17/07/14 01:42, Srinivas Pandruvada wrote:
> In an effort to improve raw read performance and at the same time enter
> low power state at every possible chance.
> For raw reads, it will keep the system powered on for a user specified
> max time, this will help read multiple samples without power on/off
> sequence.
> When runtime pm is not enabled, then it fallbacks to current scheme
> of on/off after each read.
>
> Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
I'm happy with what you have here, but really don't feel confident enough
on the runtime_pm side of things to take it without someone with more
experience of that taking a quick look at it.
Hence, I've cc'd Rafael. Rafael, if you don't have time to look at this
feel free to say so and I'll rely on the docs rather than experience.
> ---
> drivers/iio/accel/kxcjk-1013.c | 198 +++++++++++++++++++++++++++++++++--------
> 1 file changed, 160 insertions(+), 38 deletions(-)
>
> diff --git a/drivers/iio/accel/kxcjk-1013.c b/drivers/iio/accel/kxcjk-1013.c
> index bff5161..4912143 100644
> --- a/drivers/iio/accel/kxcjk-1013.c
> +++ b/drivers/iio/accel/kxcjk-1013.c
> @@ -21,6 +21,8 @@
> #include <linux/string.h>
> #include <linux/acpi.h>
> #include <linux/gpio/consumer.h>
> +#include <linux/pm.h>
> +#include <linux/pm_runtime.h>
> #include <linux/iio/iio.h>
> #include <linux/iio/sysfs.h>
> #include <linux/iio/buffer.h>
> @@ -71,15 +73,21 @@
> #define KXCJK1013_DATA_MASK_12_BIT 0x0FFF
> #define KXCJK1013_MAX_STARTUP_TIME_US 100000
>
> +#define KXCJK1013_SLEEP_DELAY_MS 2000
> +static int kxcjk1013_power_off_delay_ms = KXCJK1013_SLEEP_DELAY_MS;
> +module_param(kxcjk1013_power_off_delay_ms, int, 0644);
> +MODULE_PARM_DESC(kxcjk1013_power_off_delay_ms,
> + "KXCJK1013 accelerometer power of delay in milli seconds.");
> +
> struct kxcjk1013_data {
> struct i2c_client *client;
> struct iio_trigger *trig;
> bool trig_mode;
> struct mutex mutex;
> s16 buffer[8];
> - int power_state;
> u8 odr_bits;
> bool active_high_intr;
> + bool trigger_on;
> };
>
> enum kxcjk1013_axis {
> @@ -138,6 +146,25 @@ static int kxcjk1013_set_mode(struct kxcjk1013_data *data,
> return 0;
> }
>
> +static int kxcjk1013_get_mode(struct kxcjk1013_data *data,
> + enum kxcjk1013_mode *mode)
> +{
> + int ret;
> +
> + ret = i2c_smbus_read_byte_data(data->client, KXCJK1013_REG_CTRL1);
> + if (ret < 0) {
> + dev_err(&data->client->dev, "Error reading reg_ctrl1\n");
> + return ret;
> + }
> +
> + if (ret & KXCJK1013_REG_CTRL1_BIT_PC1)
> + *mode = OPERATION;
> + else
> + *mode = STANDBY;
> +
> + return 0;
> +}
> +
> static int kxcjk1013_chip_init(struct kxcjk1013_data *data)
> {
> int ret;
> @@ -204,10 +231,53 @@ static int kxcjk1013_chip_init(struct kxcjk1013_data *data)
> return 0;
> }
>
> +static int kxcjk1013_set_power_state(struct kxcjk1013_data *data, bool on)
> +{
> + int ret;
> +
> +#ifdef CONFIG_PM_RUNTIME
> + if (on)
> + ret = pm_runtime_get_sync(&data->client->dev);
> + else {
> + pm_runtime_put_noidle(&data->client->dev);
> + ret = pm_schedule_suspend(&data->client->dev,
> + kxcjk1013_power_off_delay_ms);
> + }
> +#else
> + if (on) {
> + ret = kxcjk1013_set_mode(data, OPERATION);
> + if (!ret) {
> + int sleep_val;
> +
> + sleep_val = kxcjk1013_get_startup_times(data);
> + if (sleep_val < 20000)
> + usleep_range(sleep_val, 20000);
> + else
> + msleep_interruptible(sleep_val/1000);
> +
> + }
> + } else
> + ret = kxcjk1013_set_mode(data, STANDBY);
> +#endif
> +
> + if (ret < 0) {
> + dev_err(&data->client->dev,
> + "Failed: kxcjk1013_set_power_state for %d\n", on);
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> static int kxcjk1013_chip_setup_interrupt(struct kxcjk1013_data *data,
> bool status)
> {
> int ret;
> + enum kxcjk1013_mode store_mode;
> +
> + ret = kxcjk1013_get_mode(data, &store_mode);
> + if (ret < 0)
> + return ret;
>
> /* This is requirement by spec to change state to STANDBY */
> ret = kxcjk1013_set_mode(data, STANDBY);
> @@ -250,7 +320,13 @@ static int kxcjk1013_chip_setup_interrupt(struct kxcjk1013_data *data,
> return ret;
> }
>
> - return ret;
> + if (store_mode == OPERATION) {
> + ret = kxcjk1013_set_mode(data, OPERATION);
> + if (ret < 0)
> + return ret;
> + }
> +
> + return 0;
> }
>
> static int kxcjk1013_convert_freq_to_bit(int val, int val2)
> @@ -271,6 +347,11 @@ static int kxcjk1013_set_odr(struct kxcjk1013_data *data, int val, int val2)
> {
> int ret;
> int odr_bits;
> + enum kxcjk1013_mode store_mode;
> +
> + ret = kxcjk1013_get_mode(data, &store_mode);
> + if (ret < 0)
> + return ret;
>
> odr_bits = kxcjk1013_convert_freq_to_bit(val, val2);
> if (odr_bits < 0)
> @@ -290,9 +371,7 @@ static int kxcjk1013_set_odr(struct kxcjk1013_data *data, int val, int val2)
>
> data->odr_bits = odr_bits;
>
> - /* Check, if the ODR is changed after data enable */
> - if (data->power_state) {
> - /* Set the state back to operation */
> + if (store_mode == OPERATION) {
> ret = kxcjk1013_set_mode(data, OPERATION);
> if (ret < 0)
> return ret;
> @@ -356,29 +435,25 @@ static int kxcjk1013_read_raw(struct iio_dev *indio_dev,
> if (iio_buffer_enabled(indio_dev))
> ret = -EBUSY;
> else {
> - int sleep_val;
> -
> - ret = kxcjk1013_set_mode(data, OPERATION);
> + ret = kxcjk1013_set_power_state(data, true);
> if (ret < 0) {
> mutex_unlock(&data->mutex);
> return ret;
> }
> - ++data->power_state;
> - sleep_val = kxcjk1013_get_startup_times(data);
> - if (sleep_val < 20000)
> - usleep_range(sleep_val, 20000);
> - else
> - msleep_interruptible(sleep_val/1000);
> ret = kxcjk1013_get_acc_reg(data, chan->scan_index);
> - if (--data->power_state == 0)
> - kxcjk1013_set_mode(data, STANDBY);
> + if (ret < 0) {
> + kxcjk1013_set_power_state(data, false);
> + mutex_unlock(&data->mutex);
> + return ret;
> + }
> + *val = sign_extend32(ret >> 4, 11);
> + ret = kxcjk1013_set_power_state(data, false);
> }
> mutex_unlock(&data->mutex);
>
> if (ret < 0)
> return ret;
>
> - *val = sign_extend32(ret >> 4, 11);
> return IIO_VAL_INT;
>
> case IIO_CHAN_INFO_SCALE:
> @@ -520,20 +595,21 @@ static int kxcjk1013_data_rdy_trigger_set_state(struct iio_trigger *trig,
> {
> struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
> struct kxcjk1013_data *data = iio_priv(indio_dev);
> + int ret;
> +
> + if (state && data->trigger_on)
> + return 0;
>
> mutex_lock(&data->mutex);
> - if (state) {
> - kxcjk1013_chip_setup_interrupt(data, true);
> - kxcjk1013_set_mode(data, OPERATION);
> - ++data->power_state;
> - } else {
> - if (--data->power_state) {
> + ret = kxcjk1013_chip_setup_interrupt(data, state);
> + if (!ret) {
> + ret = kxcjk1013_set_power_state(data, state);
> + if (ret < 0) {
> mutex_unlock(&data->mutex);
> - return 0;
> + return ret;
> }
> - kxcjk1013_chip_setup_interrupt(data, false);
> - kxcjk1013_set_mode(data, STANDBY);
> }
> + data->trigger_on = state;
> mutex_unlock(&data->mutex);
>
> return 0;
> @@ -660,14 +736,22 @@ static int kxcjk1013_probe(struct i2c_client *client,
> }
> }
>
> - ret = devm_iio_device_register(&client->dev, indio_dev);
> + ret = iio_device_register(indio_dev);
> if (ret < 0) {
> dev_err(&client->dev, "unable to register iio device\n");
> goto err_buffer_cleanup;
> }
>
> + ret = pm_runtime_set_active(&client->dev);
> + if (ret)
> + goto err_iio_unregister;
> +
> + pm_runtime_enable(&client->dev);
> +
> return 0;
>
> +err_iio_unregister:
> + iio_device_unregister(indio_dev);
> err_buffer_cleanup:
> if (data->trig_mode)
> iio_triggered_buffer_cleanup(indio_dev);
> @@ -686,6 +770,12 @@ static int kxcjk1013_remove(struct i2c_client *client)
> struct iio_dev *indio_dev = i2c_get_clientdata(client);
> struct kxcjk1013_data *data = iio_priv(indio_dev);
>
> + pm_runtime_disable(&client->dev);
> + pm_runtime_set_suspended(&client->dev);
> + pm_runtime_put_noidle(&client->dev);
> +
> + iio_device_unregister(indio_dev);
> +
> if (data->trig_mode) {
> iio_triggered_buffer_cleanup(indio_dev);
> iio_trigger_unregister(data->trig);
> @@ -704,35 +794,67 @@ static int kxcjk1013_suspend(struct device *dev)
> {
> struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
> struct kxcjk1013_data *data = iio_priv(indio_dev);
> + int ret;
>
> mutex_lock(&data->mutex);
> - kxcjk1013_set_mode(data, STANDBY);
> + ret = kxcjk1013_set_mode(data, STANDBY);
> mutex_unlock(&data->mutex);
>
> - return 0;
> + return ret;
> }
>
> static int kxcjk1013_resume(struct device *dev)
> {
> struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
> struct kxcjk1013_data *data = iio_priv(indio_dev);
> + int ret = 0;
>
> mutex_lock(&data->mutex);
> + /* Check, if the suspend occured while active */
> + if (data->trigger_on)
> + ret = kxcjk1013_set_mode(data, OPERATION);
> + mutex_unlock(&data->mutex);
>
> - if (data->power_state)
> - kxcjk1013_set_mode(data, OPERATION);
> + return ret;
> +}
> +#endif
>
> - mutex_unlock(&data->mutex);
> +#ifdef CONFIG_PM_RUNTIME
> +static int kxcjk1013_runtime_suspend(struct device *dev)
> +{
> + struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
> + struct kxcjk1013_data *data = iio_priv(indio_dev);
>
> - return 0;
> + return kxcjk1013_set_mode(data, STANDBY);
> }
>
> -static SIMPLE_DEV_PM_OPS(kxcjk1013_pm_ops, kxcjk1013_suspend, kxcjk1013_resume);
> -#define KXCJK1013_PM_OPS (&kxcjk1013_pm_ops)
> -#else
> -#define KXCJK1013_PM_OPS NULL
> +static int kxcjk1013_runtime_resume(struct device *dev)
> +{
> + struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
> + struct kxcjk1013_data *data = iio_priv(indio_dev);
> + int ret;
> + int sleep_val;
> +
> + ret = kxcjk1013_set_mode(data, OPERATION);
> + if (ret < 0)
> + return ret;
> +
> + sleep_val = kxcjk1013_get_startup_times(data);
> + if (sleep_val < 20000)
> + usleep_range(sleep_val, 20000);
> + else
> + msleep_interruptible(sleep_val/1000);
> +
> + return 0;
> +}
> #endif
>
> +static const struct dev_pm_ops kxcjk1013_pm_ops = {
> + SET_SYSTEM_SLEEP_PM_OPS(kxcjk1013_suspend, kxcjk1013_resume)
> + SET_RUNTIME_PM_OPS(kxcjk1013_runtime_suspend,
> + kxcjk1013_runtime_resume, NULL)
> +};
> +
> static const struct acpi_device_id kx_acpi_match[] = {
> {"KXCJ1013", 0},
> { },
> @@ -750,7 +872,7 @@ static struct i2c_driver kxcjk1013_driver = {
> .driver = {
> .name = KXCJK1013_DRV_NAME,
> .acpi_match_table = ACPI_PTR(kx_acpi_match),
> - .pm = KXCJK1013_PM_OPS,
> + .pm = &kxcjk1013_pm_ops,
> },
> .probe = kxcjk1013_probe,
> .remove = kxcjk1013_remove,
>
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH 3/6] iio: accel: kxcjk-1013: support runtime pm
2014-07-20 15:37 ` Jonathan Cameron
@ 2014-07-22 1:23 ` Rafael J. Wysocki
2014-07-27 13:42 ` Jonathan Cameron
0 siblings, 1 reply; 21+ messages in thread
From: Rafael J. Wysocki @ 2014-07-22 1:23 UTC (permalink / raw)
To: Jonathan Cameron, Srinivas Pandruvada; +Cc: linux-iio
On 7/20/2014 5:37 PM, Jonathan Cameron wrote:
> On 17/07/14 01:42, Srinivas Pandruvada wrote:
>> In an effort to improve raw read performance and at the same time enter
>> low power state at every possible chance.
>> For raw reads, it will keep the system powered on for a user specified
>> max time, this will help read multiple samples without power on/off
>> sequence.
>> When runtime pm is not enabled, then it fallbacks to current scheme
>> of on/off after each read.
>>
>> Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> I'm happy with what you have here, but really don't feel confident enough
> on the runtime_pm side of things to take it without someone with more
> experience of that taking a quick look at it.
>
> Hence, I've cc'd Rafael. Rafael, if you don't have time to look at this
> feel free to say so and I'll rely on the docs rather than experience.
Well, quite frankly, that'd take me quite some time to review as I'm not
familiar with the driver, so I'm afraid I can't promise any reasonable
time frame.
Rafael
>> ---
>> drivers/iio/accel/kxcjk-1013.c | 198
>> +++++++++++++++++++++++++++++++++--------
>> 1 file changed, 160 insertions(+), 38 deletions(-)
>>
>> diff --git a/drivers/iio/accel/kxcjk-1013.c
>> b/drivers/iio/accel/kxcjk-1013.c
>> index bff5161..4912143 100644
>> --- a/drivers/iio/accel/kxcjk-1013.c
>> +++ b/drivers/iio/accel/kxcjk-1013.c
>> @@ -21,6 +21,8 @@
>> #include <linux/string.h>
>> #include <linux/acpi.h>
>> #include <linux/gpio/consumer.h>
>> +#include <linux/pm.h>
>> +#include <linux/pm_runtime.h>
>> #include <linux/iio/iio.h>
>> #include <linux/iio/sysfs.h>
>> #include <linux/iio/buffer.h>
>> @@ -71,15 +73,21 @@
>> #define KXCJK1013_DATA_MASK_12_BIT 0x0FFF
>> #define KXCJK1013_MAX_STARTUP_TIME_US 100000
>>
>> +#define KXCJK1013_SLEEP_DELAY_MS 2000
>> +static int kxcjk1013_power_off_delay_ms = KXCJK1013_SLEEP_DELAY_MS;
>> +module_param(kxcjk1013_power_off_delay_ms, int, 0644);
>> +MODULE_PARM_DESC(kxcjk1013_power_off_delay_ms,
>> + "KXCJK1013 accelerometer power of delay in milli seconds.");
>> +
>> struct kxcjk1013_data {
>> struct i2c_client *client;
>> struct iio_trigger *trig;
>> bool trig_mode;
>> struct mutex mutex;
>> s16 buffer[8];
>> - int power_state;
>> u8 odr_bits;
>> bool active_high_intr;
>> + bool trigger_on;
>> };
>>
>> enum kxcjk1013_axis {
>> @@ -138,6 +146,25 @@ static int kxcjk1013_set_mode(struct
>> kxcjk1013_data *data,
>> return 0;
>> }
>>
>> +static int kxcjk1013_get_mode(struct kxcjk1013_data *data,
>> + enum kxcjk1013_mode *mode)
>> +{
>> + int ret;
>> +
>> + ret = i2c_smbus_read_byte_data(data->client, KXCJK1013_REG_CTRL1);
>> + if (ret < 0) {
>> + dev_err(&data->client->dev, "Error reading reg_ctrl1\n");
>> + return ret;
>> + }
>> +
>> + if (ret & KXCJK1013_REG_CTRL1_BIT_PC1)
>> + *mode = OPERATION;
>> + else
>> + *mode = STANDBY;
>> +
>> + return 0;
>> +}
>> +
>> static int kxcjk1013_chip_init(struct kxcjk1013_data *data)
>> {
>> int ret;
>> @@ -204,10 +231,53 @@ static int kxcjk1013_chip_init(struct
>> kxcjk1013_data *data)
>> return 0;
>> }
>>
>> +static int kxcjk1013_set_power_state(struct kxcjk1013_data *data,
>> bool on)
>> +{
>> + int ret;
>> +
>> +#ifdef CONFIG_PM_RUNTIME
>> + if (on)
>> + ret = pm_runtime_get_sync(&data->client->dev);
>> + else {
>> + pm_runtime_put_noidle(&data->client->dev);
>> + ret = pm_schedule_suspend(&data->client->dev,
>> + kxcjk1013_power_off_delay_ms);
>> + }
>> +#else
>> + if (on) {
>> + ret = kxcjk1013_set_mode(data, OPERATION);
>> + if (!ret) {
>> + int sleep_val;
>> +
>> + sleep_val = kxcjk1013_get_startup_times(data);
>> + if (sleep_val < 20000)
>> + usleep_range(sleep_val, 20000);
>> + else
>> + msleep_interruptible(sleep_val/1000);
>> +
>> + }
>> + } else
>> + ret = kxcjk1013_set_mode(data, STANDBY);
>> +#endif
>> +
>> + if (ret < 0) {
>> + dev_err(&data->client->dev,
>> + "Failed: kxcjk1013_set_power_state for %d\n", on);
>> + return ret;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> static int kxcjk1013_chip_setup_interrupt(struct kxcjk1013_data *data,
>> bool status)
>> {
>> int ret;
>> + enum kxcjk1013_mode store_mode;
>> +
>> + ret = kxcjk1013_get_mode(data, &store_mode);
>> + if (ret < 0)
>> + return ret;
>>
>> /* This is requirement by spec to change state to STANDBY */
>> ret = kxcjk1013_set_mode(data, STANDBY);
>> @@ -250,7 +320,13 @@ static int kxcjk1013_chip_setup_interrupt(struct
>> kxcjk1013_data *data,
>> return ret;
>> }
>>
>> - return ret;
>> + if (store_mode == OPERATION) {
>> + ret = kxcjk1013_set_mode(data, OPERATION);
>> + if (ret < 0)
>> + return ret;
>> + }
>> +
>> + return 0;
>> }
>>
>> static int kxcjk1013_convert_freq_to_bit(int val, int val2)
>> @@ -271,6 +347,11 @@ static int kxcjk1013_set_odr(struct
>> kxcjk1013_data *data, int val, int val2)
>> {
>> int ret;
>> int odr_bits;
>> + enum kxcjk1013_mode store_mode;
>> +
>> + ret = kxcjk1013_get_mode(data, &store_mode);
>> + if (ret < 0)
>> + return ret;
>>
>> odr_bits = kxcjk1013_convert_freq_to_bit(val, val2);
>> if (odr_bits < 0)
>> @@ -290,9 +371,7 @@ static int kxcjk1013_set_odr(struct
>> kxcjk1013_data *data, int val, int val2)
>>
>> data->odr_bits = odr_bits;
>>
>> - /* Check, if the ODR is changed after data enable */
>> - if (data->power_state) {
>> - /* Set the state back to operation */
>> + if (store_mode == OPERATION) {
>> ret = kxcjk1013_set_mode(data, OPERATION);
>> if (ret < 0)
>> return ret;
>> @@ -356,29 +435,25 @@ static int kxcjk1013_read_raw(struct iio_dev
>> *indio_dev,
>> if (iio_buffer_enabled(indio_dev))
>> ret = -EBUSY;
>> else {
>> - int sleep_val;
>> -
>> - ret = kxcjk1013_set_mode(data, OPERATION);
>> + ret = kxcjk1013_set_power_state(data, true);
>> if (ret < 0) {
>> mutex_unlock(&data->mutex);
>> return ret;
>> }
>> - ++data->power_state;
>> - sleep_val = kxcjk1013_get_startup_times(data);
>> - if (sleep_val < 20000)
>> - usleep_range(sleep_val, 20000);
>> - else
>> - msleep_interruptible(sleep_val/1000);
>> ret = kxcjk1013_get_acc_reg(data, chan->scan_index);
>> - if (--data->power_state == 0)
>> - kxcjk1013_set_mode(data, STANDBY);
>> + if (ret < 0) {
>> + kxcjk1013_set_power_state(data, false);
>> + mutex_unlock(&data->mutex);
>> + return ret;
>> + }
>> + *val = sign_extend32(ret >> 4, 11);
>> + ret = kxcjk1013_set_power_state(data, false);
>> }
>> mutex_unlock(&data->mutex);
>>
>> if (ret < 0)
>> return ret;
>>
>> - *val = sign_extend32(ret >> 4, 11);
>> return IIO_VAL_INT;
>>
>> case IIO_CHAN_INFO_SCALE:
>> @@ -520,20 +595,21 @@ static int
>> kxcjk1013_data_rdy_trigger_set_state(struct iio_trigger *trig,
>> {
>> struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
>> struct kxcjk1013_data *data = iio_priv(indio_dev);
>> + int ret;
>> +
>> + if (state && data->trigger_on)
>> + return 0;
>>
>> mutex_lock(&data->mutex);
>> - if (state) {
>> - kxcjk1013_chip_setup_interrupt(data, true);
>> - kxcjk1013_set_mode(data, OPERATION);
>> - ++data->power_state;
>> - } else {
>> - if (--data->power_state) {
>> + ret = kxcjk1013_chip_setup_interrupt(data, state);
>> + if (!ret) {
>> + ret = kxcjk1013_set_power_state(data, state);
>> + if (ret < 0) {
>> mutex_unlock(&data->mutex);
>> - return 0;
>> + return ret;
>> }
>> - kxcjk1013_chip_setup_interrupt(data, false);
>> - kxcjk1013_set_mode(data, STANDBY);
>> }
>> + data->trigger_on = state;
>> mutex_unlock(&data->mutex);
>>
>> return 0;
>> @@ -660,14 +736,22 @@ static int kxcjk1013_probe(struct i2c_client
>> *client,
>> }
>> }
>>
>> - ret = devm_iio_device_register(&client->dev, indio_dev);
>> + ret = iio_device_register(indio_dev);
>> if (ret < 0) {
>> dev_err(&client->dev, "unable to register iio device\n");
>> goto err_buffer_cleanup;
>> }
>>
>> + ret = pm_runtime_set_active(&client->dev);
>> + if (ret)
>> + goto err_iio_unregister;
>> +
>> + pm_runtime_enable(&client->dev);
>> +
>> return 0;
>>
>> +err_iio_unregister:
>> + iio_device_unregister(indio_dev);
>> err_buffer_cleanup:
>> if (data->trig_mode)
>> iio_triggered_buffer_cleanup(indio_dev);
>> @@ -686,6 +770,12 @@ static int kxcjk1013_remove(struct i2c_client
>> *client)
>> struct iio_dev *indio_dev = i2c_get_clientdata(client);
>> struct kxcjk1013_data *data = iio_priv(indio_dev);
>>
>> + pm_runtime_disable(&client->dev);
>> + pm_runtime_set_suspended(&client->dev);
>> + pm_runtime_put_noidle(&client->dev);
>> +
>> + iio_device_unregister(indio_dev);
>> +
>> if (data->trig_mode) {
>> iio_triggered_buffer_cleanup(indio_dev);
>> iio_trigger_unregister(data->trig);
>> @@ -704,35 +794,67 @@ static int kxcjk1013_suspend(struct device *dev)
>> {
>> struct iio_dev *indio_dev =
>> i2c_get_clientdata(to_i2c_client(dev));
>> struct kxcjk1013_data *data = iio_priv(indio_dev);
>> + int ret;
>>
>> mutex_lock(&data->mutex);
>> - kxcjk1013_set_mode(data, STANDBY);
>> + ret = kxcjk1013_set_mode(data, STANDBY);
>> mutex_unlock(&data->mutex);
>>
>> - return 0;
>> + return ret;
>> }
>>
>> static int kxcjk1013_resume(struct device *dev)
>> {
>> struct iio_dev *indio_dev =
>> i2c_get_clientdata(to_i2c_client(dev));
>> struct kxcjk1013_data *data = iio_priv(indio_dev);
>> + int ret = 0;
>>
>> mutex_lock(&data->mutex);
>> + /* Check, if the suspend occured while active */
>> + if (data->trigger_on)
>> + ret = kxcjk1013_set_mode(data, OPERATION);
>> + mutex_unlock(&data->mutex);
>>
>> - if (data->power_state)
>> - kxcjk1013_set_mode(data, OPERATION);
>> + return ret;
>> +}
>> +#endif
>>
>> - mutex_unlock(&data->mutex);
>> +#ifdef CONFIG_PM_RUNTIME
>> +static int kxcjk1013_runtime_suspend(struct device *dev)
>> +{
>> + struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
>> + struct kxcjk1013_data *data = iio_priv(indio_dev);
>>
>> - return 0;
>> + return kxcjk1013_set_mode(data, STANDBY);
>> }
>>
>> -static SIMPLE_DEV_PM_OPS(kxcjk1013_pm_ops, kxcjk1013_suspend,
>> kxcjk1013_resume);
>> -#define KXCJK1013_PM_OPS (&kxcjk1013_pm_ops)
>> -#else
>> -#define KXCJK1013_PM_OPS NULL
>> +static int kxcjk1013_runtime_resume(struct device *dev)
>> +{
>> + struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
>> + struct kxcjk1013_data *data = iio_priv(indio_dev);
>> + int ret;
>> + int sleep_val;
>> +
>> + ret = kxcjk1013_set_mode(data, OPERATION);
>> + if (ret < 0)
>> + return ret;
>> +
>> + sleep_val = kxcjk1013_get_startup_times(data);
>> + if (sleep_val < 20000)
>> + usleep_range(sleep_val, 20000);
>> + else
>> + msleep_interruptible(sleep_val/1000);
>> +
>> + return 0;
>> +}
>> #endif
>>
>> +static const struct dev_pm_ops kxcjk1013_pm_ops = {
>> + SET_SYSTEM_SLEEP_PM_OPS(kxcjk1013_suspend, kxcjk1013_resume)
>> + SET_RUNTIME_PM_OPS(kxcjk1013_runtime_suspend,
>> + kxcjk1013_runtime_resume, NULL)
>> +};
>> +
>> static const struct acpi_device_id kx_acpi_match[] = {
>> {"KXCJ1013", 0},
>> { },
>> @@ -750,7 +872,7 @@ static struct i2c_driver kxcjk1013_driver = {
>> .driver = {
>> .name = KXCJK1013_DRV_NAME,
>> .acpi_match_table = ACPI_PTR(kx_acpi_match),
>> - .pm = KXCJK1013_PM_OPS,
>> + .pm = &kxcjk1013_pm_ops,
>> },
>> .probe = kxcjk1013_probe,
>> .remove = kxcjk1013_remove,
>>
>
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH 3/6] iio: accel: kxcjk-1013: support runtime pm
2014-07-22 1:23 ` Rafael J. Wysocki
@ 2014-07-27 13:42 ` Jonathan Cameron
2014-07-27 14:26 ` Srinivas Pandruvada
0 siblings, 1 reply; 21+ messages in thread
From: Jonathan Cameron @ 2014-07-27 13:42 UTC (permalink / raw)
To: Rafael J. Wysocki, Srinivas Pandruvada; +Cc: linux-iio
On 22/07/14 02:23, Rafael J. Wysocki wrote:
> On 7/20/2014 5:37 PM, Jonathan Cameron wrote:
>> On 17/07/14 01:42, Srinivas Pandruvada wrote:
>>> In an effort to improve raw read performance and at the same time enter
>>> low power state at every possible chance.
>>> For raw reads, it will keep the system powered on for a user specified
>>> max time, this will help read multiple samples without power on/off
>>> sequence.
>>> When runtime pm is not enabled, then it fallbacks to current scheme
>>> of on/off after each read.
>>>
>>> Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
>> I'm happy with what you have here, but really don't feel confident enough
>> on the runtime_pm side of things to take it without someone with more
>> experience of that taking a quick look at it.
>>
>> Hence, I've cc'd Rafael. Rafael, if you don't have time to look at this
>> feel free to say so and I'll rely on the docs rather than experience.
>
> Well, quite frankly, that'd take me quite some time to review as I'm
> not familiar with the driver, so I'm afraid I can't promise any
> reasonable time frame.
Thanks anyway,
> Rafael
>
Srinivas, from my initial review I was in the process of applying this and
got:
drivers/iio/accel/kxcjk-1013.c: In function ‘kxcjk1013_set_power_state’:
drivers/iio/accel/kxcjk-1013.c:252:4: error: implicit declaration of function ‘kxcjk1013_get_startup_times’ [-Werror=implicit-function-declaration]
sleep_val = kxcjk1013_get_startup_times(data);
^
drivers/iio/accel/kxcjk-1013.c: At top level:
drivers/iio/accel/kxcjk-1013.c:413:12: error: static declaration of ‘kxcjk1013_get_startup_times’ follows non-static declaration
static int kxcjk1013_get_startup_times(struct kxcjk1013_data *data)
^
drivers/iio/accel/kxcjk-1013.c:252:16: note: previous implicit declaration of ‘kxcjk1013_get_startup_times’ was here
sleep_val = kxcjk1013_get_startup_times(data);
^
drivers/iio/accel/kxcjk-1013.c:413:12: warning: ‘kxcjk1013_get_startup_times’ defined but not used [-Wunused-function]
static int kxcjk1013_get_startup_times(struct kxcjk1013_data *data)
^
Looks like a spot of reordering is needed.
Jonathan
>
>>> ---
>>> drivers/iio/accel/kxcjk-1013.c | 198 +++++++++++++++++++++++++++++++++--------
>>> 1 file changed, 160 insertions(+), 38 deletions(-)
>>>
>>> diff --git a/drivers/iio/accel/kxcjk-1013.c b/drivers/iio/accel/kxcjk-1013.c
>>> index bff5161..4912143 100644
>>> --- a/drivers/iio/accel/kxcjk-1013.c
>>> +++ b/drivers/iio/accel/kxcjk-1013.c
>>> @@ -21,6 +21,8 @@
>>> #include <linux/string.h>
>>> #include <linux/acpi.h>
>>> #include <linux/gpio/consumer.h>
>>> +#include <linux/pm.h>
>>> +#include <linux/pm_runtime.h>
>>> #include <linux/iio/iio.h>
>>> #include <linux/iio/sysfs.h>
>>> #include <linux/iio/buffer.h>
>>> @@ -71,15 +73,21 @@
>>> #define KXCJK1013_DATA_MASK_12_BIT 0x0FFF
>>> #define KXCJK1013_MAX_STARTUP_TIME_US 100000
>>>
>>> +#define KXCJK1013_SLEEP_DELAY_MS 2000
>>> +static int kxcjk1013_power_off_delay_ms = KXCJK1013_SLEEP_DELAY_MS;
>>> +module_param(kxcjk1013_power_off_delay_ms, int, 0644);
>>> +MODULE_PARM_DESC(kxcjk1013_power_off_delay_ms,
>>> + "KXCJK1013 accelerometer power of delay in milli seconds.");
>>> +
>>> struct kxcjk1013_data {
>>> struct i2c_client *client;
>>> struct iio_trigger *trig;
>>> bool trig_mode;
>>> struct mutex mutex;
>>> s16 buffer[8];
>>> - int power_state;
>>> u8 odr_bits;
>>> bool active_high_intr;
>>> + bool trigger_on;
>>> };
>>>
>>> enum kxcjk1013_axis {
>>> @@ -138,6 +146,25 @@ static int kxcjk1013_set_mode(struct kxcjk1013_data *data,
>>> return 0;
>>> }
>>>
>>> +static int kxcjk1013_get_mode(struct kxcjk1013_data *data,
>>> + enum kxcjk1013_mode *mode)
>>> +{
>>> + int ret;
>>> +
>>> + ret = i2c_smbus_read_byte_data(data->client, KXCJK1013_REG_CTRL1);
>>> + if (ret < 0) {
>>> + dev_err(&data->client->dev, "Error reading reg_ctrl1\n");
>>> + return ret;
>>> + }
>>> +
>>> + if (ret & KXCJK1013_REG_CTRL1_BIT_PC1)
>>> + *mode = OPERATION;
>>> + else
>>> + *mode = STANDBY;
>>> +
>>> + return 0;
>>> +}
>>> +
>>> static int kxcjk1013_chip_init(struct kxcjk1013_data *data)
>>> {
>>> int ret;
>>> @@ -204,10 +231,53 @@ static int kxcjk1013_chip_init(struct kxcjk1013_data *data)
>>> return 0;
>>> }
>>>
>>> +static int kxcjk1013_set_power_state(struct kxcjk1013_data *data, bool on)
>>> +{
>>> + int ret;
>>> +
>>> +#ifdef CONFIG_PM_RUNTIME
>>> + if (on)
>>> + ret = pm_runtime_get_sync(&data->client->dev);
>>> + else {
>>> + pm_runtime_put_noidle(&data->client->dev);
>>> + ret = pm_schedule_suspend(&data->client->dev,
>>> + kxcjk1013_power_off_delay_ms);
>>> + }
>>> +#else
>>> + if (on) {
>>> + ret = kxcjk1013_set_mode(data, OPERATION);
>>> + if (!ret) {
>>> + int sleep_val;
>>> +
>>> + sleep_val = kxcjk1013_get_startup_times(data);
>>> + if (sleep_val < 20000)
>>> + usleep_range(sleep_val, 20000);
>>> + else
>>> + msleep_interruptible(sleep_val/1000);
>>> +
>>> + }
>>> + } else
>>> + ret = kxcjk1013_set_mode(data, STANDBY);
>>> +#endif
>>> +
>>> + if (ret < 0) {
>>> + dev_err(&data->client->dev,
>>> + "Failed: kxcjk1013_set_power_state for %d\n", on);
>>> + return ret;
>>> + }
>>> +
>>> + return 0;
>>> +}
>>> +
>>> static int kxcjk1013_chip_setup_interrupt(struct kxcjk1013_data *data,
>>> bool status)
>>> {
>>> int ret;
>>> + enum kxcjk1013_mode store_mode;
>>> +
>>> + ret = kxcjk1013_get_mode(data, &store_mode);
>>> + if (ret < 0)
>>> + return ret;
>>>
>>> /* This is requirement by spec to change state to STANDBY */
>>> ret = kxcjk1013_set_mode(data, STANDBY);
>>> @@ -250,7 +320,13 @@ static int kxcjk1013_chip_setup_interrupt(struct kxcjk1013_data *data,
>>> return ret;
>>> }
>>>
>>> - return ret;
>>> + if (store_mode == OPERATION) {
>>> + ret = kxcjk1013_set_mode(data, OPERATION);
>>> + if (ret < 0)
>>> + return ret;
>>> + }
>>> +
>>> + return 0;
>>> }
>>>
>>> static int kxcjk1013_convert_freq_to_bit(int val, int val2)
>>> @@ -271,6 +347,11 @@ static int kxcjk1013_set_odr(struct kxcjk1013_data *data, int val, int val2)
>>> {
>>> int ret;
>>> int odr_bits;
>>> + enum kxcjk1013_mode store_mode;
>>> +
>>> + ret = kxcjk1013_get_mode(data, &store_mode);
>>> + if (ret < 0)
>>> + return ret;
>>>
>>> odr_bits = kxcjk1013_convert_freq_to_bit(val, val2);
>>> if (odr_bits < 0)
>>> @@ -290,9 +371,7 @@ static int kxcjk1013_set_odr(struct kxcjk1013_data *data, int val, int val2)
>>>
>>> data->odr_bits = odr_bits;
>>>
>>> - /* Check, if the ODR is changed after data enable */
>>> - if (data->power_state) {
>>> - /* Set the state back to operation */
>>> + if (store_mode == OPERATION) {
>>> ret = kxcjk1013_set_mode(data, OPERATION);
>>> if (ret < 0)
>>> return ret;
>>> @@ -356,29 +435,25 @@ static int kxcjk1013_read_raw(struct iio_dev *indio_dev,
>>> if (iio_buffer_enabled(indio_dev))
>>> ret = -EBUSY;
>>> else {
>>> - int sleep_val;
>>> -
>>> - ret = kxcjk1013_set_mode(data, OPERATION);
>>> + ret = kxcjk1013_set_power_state(data, true);
>>> if (ret < 0) {
>>> mutex_unlock(&data->mutex);
>>> return ret;
>>> }
>>> - ++data->power_state;
>>> - sleep_val = kxcjk1013_get_startup_times(data);
>>> - if (sleep_val < 20000)
>>> - usleep_range(sleep_val, 20000);
>>> - else
>>> - msleep_interruptible(sleep_val/1000);
>>> ret = kxcjk1013_get_acc_reg(data, chan->scan_index);
>>> - if (--data->power_state == 0)
>>> - kxcjk1013_set_mode(data, STANDBY);
>>> + if (ret < 0) {
>>> + kxcjk1013_set_power_state(data, false);
>>> + mutex_unlock(&data->mutex);
>>> + return ret;
>>> + }
>>> + *val = sign_extend32(ret >> 4, 11);
>>> + ret = kxcjk1013_set_power_state(data, false);
>>> }
>>> mutex_unlock(&data->mutex);
>>>
>>> if (ret < 0)
>>> return ret;
>>>
>>> - *val = sign_extend32(ret >> 4, 11);
>>> return IIO_VAL_INT;
>>>
>>> case IIO_CHAN_INFO_SCALE:
>>> @@ -520,20 +595,21 @@ static int kxcjk1013_data_rdy_trigger_set_state(struct iio_trigger *trig,
>>> {
>>> struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
>>> struct kxcjk1013_data *data = iio_priv(indio_dev);
>>> + int ret;
>>> +
>>> + if (state && data->trigger_on)
>>> + return 0;
>>>
>>> mutex_lock(&data->mutex);
>>> - if (state) {
>>> - kxcjk1013_chip_setup_interrupt(data, true);
>>> - kxcjk1013_set_mode(data, OPERATION);
>>> - ++data->power_state;
>>> - } else {
>>> - if (--data->power_state) {
>>> + ret = kxcjk1013_chip_setup_interrupt(data, state);
>>> + if (!ret) {
>>> + ret = kxcjk1013_set_power_state(data, state);
>>> + if (ret < 0) {
>>> mutex_unlock(&data->mutex);
>>> - return 0;
>>> + return ret;
>>> }
>>> - kxcjk1013_chip_setup_interrupt(data, false);
>>> - kxcjk1013_set_mode(data, STANDBY);
>>> }
>>> + data->trigger_on = state;
>>> mutex_unlock(&data->mutex);
>>>
>>> return 0;
>>> @@ -660,14 +736,22 @@ static int kxcjk1013_probe(struct i2c_client *client,
>>> }
>>> }
>>>
>>> - ret = devm_iio_device_register(&client->dev, indio_dev);
>>> + ret = iio_device_register(indio_dev);
>>> if (ret < 0) {
>>> dev_err(&client->dev, "unable to register iio device\n");
>>> goto err_buffer_cleanup;
>>> }
>>>
>>> + ret = pm_runtime_set_active(&client->dev);
>>> + if (ret)
>>> + goto err_iio_unregister;
>>> +
>>> + pm_runtime_enable(&client->dev);
>>> +
>>> return 0;
>>>
>>> +err_iio_unregister:
>>> + iio_device_unregister(indio_dev);
>>> err_buffer_cleanup:
>>> if (data->trig_mode)
>>> iio_triggered_buffer_cleanup(indio_dev);
>>> @@ -686,6 +770,12 @@ static int kxcjk1013_remove(struct i2c_client *client)
>>> struct iio_dev *indio_dev = i2c_get_clientdata(client);
>>> struct kxcjk1013_data *data = iio_priv(indio_dev);
>>>
>>> + pm_runtime_disable(&client->dev);
>>> + pm_runtime_set_suspended(&client->dev);
>>> + pm_runtime_put_noidle(&client->dev);
>>> +
>>> + iio_device_unregister(indio_dev);
>>> +
>>> if (data->trig_mode) {
>>> iio_triggered_buffer_cleanup(indio_dev);
>>> iio_trigger_unregister(data->trig);
>>> @@ -704,35 +794,67 @@ static int kxcjk1013_suspend(struct device *dev)
>>> {
>>> struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
>>> struct kxcjk1013_data *data = iio_priv(indio_dev);
>>> + int ret;
>>>
>>> mutex_lock(&data->mutex);
>>> - kxcjk1013_set_mode(data, STANDBY);
>>> + ret = kxcjk1013_set_mode(data, STANDBY);
>>> mutex_unlock(&data->mutex);
>>>
>>> - return 0;
>>> + return ret;
>>> }
>>>
>>> static int kxcjk1013_resume(struct device *dev)
>>> {
>>> struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
>>> struct kxcjk1013_data *data = iio_priv(indio_dev);
>>> + int ret = 0;
>>>
>>> mutex_lock(&data->mutex);
>>> + /* Check, if the suspend occured while active */
>>> + if (data->trigger_on)
>>> + ret = kxcjk1013_set_mode(data, OPERATION);
>>> + mutex_unlock(&data->mutex);
>>>
>>> - if (data->power_state)
>>> - kxcjk1013_set_mode(data, OPERATION);
>>> + return ret;
>>> +}
>>> +#endif
>>>
>>> - mutex_unlock(&data->mutex);
>>> +#ifdef CONFIG_PM_RUNTIME
>>> +static int kxcjk1013_runtime_suspend(struct device *dev)
>>> +{
>>> + struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
>>> + struct kxcjk1013_data *data = iio_priv(indio_dev);
>>>
>>> - return 0;
>>> + return kxcjk1013_set_mode(data, STANDBY);
>>> }
>>>
>>> -static SIMPLE_DEV_PM_OPS(kxcjk1013_pm_ops, kxcjk1013_suspend, kxcjk1013_resume);
>>> -#define KXCJK1013_PM_OPS (&kxcjk1013_pm_ops)
>>> -#else
>>> -#define KXCJK1013_PM_OPS NULL
>>> +static int kxcjk1013_runtime_resume(struct device *dev)
>>> +{
>>> + struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
>>> + struct kxcjk1013_data *data = iio_priv(indio_dev);
>>> + int ret;
>>> + int sleep_val;
>>> +
>>> + ret = kxcjk1013_set_mode(data, OPERATION);
>>> + if (ret < 0)
>>> + return ret;
>>> +
>>> + sleep_val = kxcjk1013_get_startup_times(data);
>>> + if (sleep_val < 20000)
>>> + usleep_range(sleep_val, 20000);
>>> + else
>>> + msleep_interruptible(sleep_val/1000);
>>> +
>>> + return 0;
>>> +}
>>> #endif
>>>
>>> +static const struct dev_pm_ops kxcjk1013_pm_ops = {
>>> + SET_SYSTEM_SLEEP_PM_OPS(kxcjk1013_suspend, kxcjk1013_resume)
>>> + SET_RUNTIME_PM_OPS(kxcjk1013_runtime_suspend,
>>> + kxcjk1013_runtime_resume, NULL)
>>> +};
>>> +
>>> static const struct acpi_device_id kx_acpi_match[] = {
>>> {"KXCJ1013", 0},
>>> { },
>>> @@ -750,7 +872,7 @@ static struct i2c_driver kxcjk1013_driver = {
>>> .driver = {
>>> .name = KXCJK1013_DRV_NAME,
>>> .acpi_match_table = ACPI_PTR(kx_acpi_match),
>>> - .pm = KXCJK1013_PM_OPS,
>>> + .pm = &kxcjk1013_pm_ops,
>>> },
>>> .probe = kxcjk1013_probe,
>>> .remove = kxcjk1013_remove,
>>>
>>
>
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH 3/6] iio: accel: kxcjk-1013: support runtime pm
2014-07-27 13:42 ` Jonathan Cameron
@ 2014-07-27 14:26 ` Srinivas Pandruvada
0 siblings, 0 replies; 21+ messages in thread
From: Srinivas Pandruvada @ 2014-07-27 14:26 UTC (permalink / raw)
To: Jonathan Cameron, Rafael J. Wysocki; +Cc: linux-iio
On 07/27/2014 06:42 AM, Jonathan Cameron wrote:
> On 22/07/14 02:23, Rafael J. Wysocki wrote:
>> On 7/20/2014 5:37 PM, Jonathan Cameron wrote:
>>> On 17/07/14 01:42, Srinivas Pandruvada wrote:
>>>> In an effort to improve raw read performance and at the same time
>>>> enter
>>>> low power state at every possible chance.
>>>> For raw reads, it will keep the system powered on for a user specified
>>>> max time, this will help read multiple samples without power on/off
>>>> sequence.
>>>> When runtime pm is not enabled, then it fallbacks to current scheme
>>>> of on/off after each read.
>>>>
>>>> Signed-off-by: Srinivas Pandruvada
>>>> <srinivas.pandruvada@linux.intel.com>
>>> I'm happy with what you have here, but really don't feel confident
>>> enough
>>> on the runtime_pm side of things to take it without someone with more
>>> experience of that taking a quick look at it.
>>>
>>> Hence, I've cc'd Rafael. Rafael, if you don't have time to look at
>>> this
>>> feel free to say so and I'll rely on the docs rather than experience.
>>
>> Well, quite frankly, that'd take me quite some time to review as I'm
>> not familiar with the driver, so I'm afraid I can't promise any
>> reasonable time frame.
> Thanks anyway,
>> Rafael
>>
> Srinivas, from my initial review I was in the process of applying this
> and
> got:
>
> drivers/iio/accel/kxcjk-1013.c: In function ‘kxcjk1013_set_power_state’:
> drivers/iio/accel/kxcjk-1013.c:252:4: error: implicit declaration of
> function ‘kxcjk1013_get_startup_times’
> [-Werror=implicit-function-declaration]
> sleep_val = kxcjk1013_get_startup_times(data);
> ^
> drivers/iio/accel/kxcjk-1013.c: At top level:
> drivers/iio/accel/kxcjk-1013.c:413:12: error: static declaration of
> ‘kxcjk1013_get_startup_times’ follows non-static declaration
> static int kxcjk1013_get_startup_times(struct kxcjk1013_data *data)
> ^
> drivers/iio/accel/kxcjk-1013.c:252:16: note: previous implicit
> declaration of ‘kxcjk1013_get_startup_times’ was here
> sleep_val = kxcjk1013_get_startup_times(data);
> ^
> drivers/iio/accel/kxcjk-1013.c:413:12: warning:
> ‘kxcjk1013_get_startup_times’ defined but not used [-Wunused-function]
> static int kxcjk1013_get_startup_times(struct kxcjk1013_data *data)
> ^
>
I will resubmit on the top of your latest tree.
Thanks,
Srinivas
> Looks like a spot of reordering is needed.
>
> Jonathan
>>
>>>> ---
>>>> drivers/iio/accel/kxcjk-1013.c | 198
>>>> +++++++++++++++++++++++++++++++++--------
>>>> 1 file changed, 160 insertions(+), 38 deletions(-)
>>>>
>>>> diff --git a/drivers/iio/accel/kxcjk-1013.c
>>>> b/drivers/iio/accel/kxcjk-1013.c
>>>> index bff5161..4912143 100644
>>>> --- a/drivers/iio/accel/kxcjk-1013.c
>>>> +++ b/drivers/iio/accel/kxcjk-1013.c
>>>> @@ -21,6 +21,8 @@
>>>> #include <linux/string.h>
>>>> #include <linux/acpi.h>
>>>> #include <linux/gpio/consumer.h>
>>>> +#include <linux/pm.h>
>>>> +#include <linux/pm_runtime.h>
>>>> #include <linux/iio/iio.h>
>>>> #include <linux/iio/sysfs.h>
>>>> #include <linux/iio/buffer.h>
>>>> @@ -71,15 +73,21 @@
>>>> #define KXCJK1013_DATA_MASK_12_BIT 0x0FFF
>>>> #define KXCJK1013_MAX_STARTUP_TIME_US 100000
>>>>
>>>> +#define KXCJK1013_SLEEP_DELAY_MS 2000
>>>> +static int kxcjk1013_power_off_delay_ms = KXCJK1013_SLEEP_DELAY_MS;
>>>> +module_param(kxcjk1013_power_off_delay_ms, int, 0644);
>>>> +MODULE_PARM_DESC(kxcjk1013_power_off_delay_ms,
>>>> + "KXCJK1013 accelerometer power of delay in milli seconds.");
>>>> +
>>>> struct kxcjk1013_data {
>>>> struct i2c_client *client;
>>>> struct iio_trigger *trig;
>>>> bool trig_mode;
>>>> struct mutex mutex;
>>>> s16 buffer[8];
>>>> - int power_state;
>>>> u8 odr_bits;
>>>> bool active_high_intr;
>>>> + bool trigger_on;
>>>> };
>>>>
>>>> enum kxcjk1013_axis {
>>>> @@ -138,6 +146,25 @@ static int kxcjk1013_set_mode(struct
>>>> kxcjk1013_data *data,
>>>> return 0;
>>>> }
>>>>
>>>> +static int kxcjk1013_get_mode(struct kxcjk1013_data *data,
>>>> + enum kxcjk1013_mode *mode)
>>>> +{
>>>> + int ret;
>>>> +
>>>> + ret = i2c_smbus_read_byte_data(data->client,
>>>> KXCJK1013_REG_CTRL1);
>>>> + if (ret < 0) {
>>>> + dev_err(&data->client->dev, "Error reading reg_ctrl1\n");
>>>> + return ret;
>>>> + }
>>>> +
>>>> + if (ret & KXCJK1013_REG_CTRL1_BIT_PC1)
>>>> + *mode = OPERATION;
>>>> + else
>>>> + *mode = STANDBY;
>>>> +
>>>> + return 0;
>>>> +}
>>>> +
>>>> static int kxcjk1013_chip_init(struct kxcjk1013_data *data)
>>>> {
>>>> int ret;
>>>> @@ -204,10 +231,53 @@ static int kxcjk1013_chip_init(struct
>>>> kxcjk1013_data *data)
>>>> return 0;
>>>> }
>>>>
>>>> +static int kxcjk1013_set_power_state(struct kxcjk1013_data *data,
>>>> bool on)
>>>> +{
>>>> + int ret;
>>>> +
>>>> +#ifdef CONFIG_PM_RUNTIME
>>>> + if (on)
>>>> + ret = pm_runtime_get_sync(&data->client->dev);
>>>> + else {
>>>> + pm_runtime_put_noidle(&data->client->dev);
>>>> + ret = pm_schedule_suspend(&data->client->dev,
>>>> + kxcjk1013_power_off_delay_ms);
>>>> + }
>>>> +#else
>>>> + if (on) {
>>>> + ret = kxcjk1013_set_mode(data, OPERATION);
>>>> + if (!ret) {
>>>> + int sleep_val;
>>>> +
>>>> + sleep_val = kxcjk1013_get_startup_times(data);
>>>> + if (sleep_val < 20000)
>>>> + usleep_range(sleep_val, 20000);
>>>> + else
>>>> + msleep_interruptible(sleep_val/1000);
>>>> +
>>>> + }
>>>> + } else
>>>> + ret = kxcjk1013_set_mode(data, STANDBY);
>>>> +#endif
>>>> +
>>>> + if (ret < 0) {
>>>> + dev_err(&data->client->dev,
>>>> + "Failed: kxcjk1013_set_power_state for %d\n", on);
>>>> + return ret;
>>>> + }
>>>> +
>>>> + return 0;
>>>> +}
>>>> +
>>>> static int kxcjk1013_chip_setup_interrupt(struct kxcjk1013_data
>>>> *data,
>>>> bool status)
>>>> {
>>>> int ret;
>>>> + enum kxcjk1013_mode store_mode;
>>>> +
>>>> + ret = kxcjk1013_get_mode(data, &store_mode);
>>>> + if (ret < 0)
>>>> + return ret;
>>>>
>>>> /* This is requirement by spec to change state to STANDBY */
>>>> ret = kxcjk1013_set_mode(data, STANDBY);
>>>> @@ -250,7 +320,13 @@ static int
>>>> kxcjk1013_chip_setup_interrupt(struct kxcjk1013_data *data,
>>>> return ret;
>>>> }
>>>>
>>>> - return ret;
>>>> + if (store_mode == OPERATION) {
>>>> + ret = kxcjk1013_set_mode(data, OPERATION);
>>>> + if (ret < 0)
>>>> + return ret;
>>>> + }
>>>> +
>>>> + return 0;
>>>> }
>>>>
>>>> static int kxcjk1013_convert_freq_to_bit(int val, int val2)
>>>> @@ -271,6 +347,11 @@ static int kxcjk1013_set_odr(struct
>>>> kxcjk1013_data *data, int val, int val2)
>>>> {
>>>> int ret;
>>>> int odr_bits;
>>>> + enum kxcjk1013_mode store_mode;
>>>> +
>>>> + ret = kxcjk1013_get_mode(data, &store_mode);
>>>> + if (ret < 0)
>>>> + return ret;
>>>>
>>>> odr_bits = kxcjk1013_convert_freq_to_bit(val, val2);
>>>> if (odr_bits < 0)
>>>> @@ -290,9 +371,7 @@ static int kxcjk1013_set_odr(struct
>>>> kxcjk1013_data *data, int val, int val2)
>>>>
>>>> data->odr_bits = odr_bits;
>>>>
>>>> - /* Check, if the ODR is changed after data enable */
>>>> - if (data->power_state) {
>>>> - /* Set the state back to operation */
>>>> + if (store_mode == OPERATION) {
>>>> ret = kxcjk1013_set_mode(data, OPERATION);
>>>> if (ret < 0)
>>>> return ret;
>>>> @@ -356,29 +435,25 @@ static int kxcjk1013_read_raw(struct iio_dev
>>>> *indio_dev,
>>>> if (iio_buffer_enabled(indio_dev))
>>>> ret = -EBUSY;
>>>> else {
>>>> - int sleep_val;
>>>> -
>>>> - ret = kxcjk1013_set_mode(data, OPERATION);
>>>> + ret = kxcjk1013_set_power_state(data, true);
>>>> if (ret < 0) {
>>>> mutex_unlock(&data->mutex);
>>>> return ret;
>>>> }
>>>> - ++data->power_state;
>>>> - sleep_val = kxcjk1013_get_startup_times(data);
>>>> - if (sleep_val < 20000)
>>>> - usleep_range(sleep_val, 20000);
>>>> - else
>>>> - msleep_interruptible(sleep_val/1000);
>>>> ret = kxcjk1013_get_acc_reg(data, chan->scan_index);
>>>> - if (--data->power_state == 0)
>>>> - kxcjk1013_set_mode(data, STANDBY);
>>>> + if (ret < 0) {
>>>> + kxcjk1013_set_power_state(data, false);
>>>> + mutex_unlock(&data->mutex);
>>>> + return ret;
>>>> + }
>>>> + *val = sign_extend32(ret >> 4, 11);
>>>> + ret = kxcjk1013_set_power_state(data, false);
>>>> }
>>>> mutex_unlock(&data->mutex);
>>>>
>>>> if (ret < 0)
>>>> return ret;
>>>>
>>>> - *val = sign_extend32(ret >> 4, 11);
>>>> return IIO_VAL_INT;
>>>>
>>>> case IIO_CHAN_INFO_SCALE:
>>>> @@ -520,20 +595,21 @@ static int
>>>> kxcjk1013_data_rdy_trigger_set_state(struct iio_trigger *trig,
>>>> {
>>>> struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
>>>> struct kxcjk1013_data *data = iio_priv(indio_dev);
>>>> + int ret;
>>>> +
>>>> + if (state && data->trigger_on)
>>>> + return 0;
>>>>
>>>> mutex_lock(&data->mutex);
>>>> - if (state) {
>>>> - kxcjk1013_chip_setup_interrupt(data, true);
>>>> - kxcjk1013_set_mode(data, OPERATION);
>>>> - ++data->power_state;
>>>> - } else {
>>>> - if (--data->power_state) {
>>>> + ret = kxcjk1013_chip_setup_interrupt(data, state);
>>>> + if (!ret) {
>>>> + ret = kxcjk1013_set_power_state(data, state);
>>>> + if (ret < 0) {
>>>> mutex_unlock(&data->mutex);
>>>> - return 0;
>>>> + return ret;
>>>> }
>>>> - kxcjk1013_chip_setup_interrupt(data, false);
>>>> - kxcjk1013_set_mode(data, STANDBY);
>>>> }
>>>> + data->trigger_on = state;
>>>> mutex_unlock(&data->mutex);
>>>>
>>>> return 0;
>>>> @@ -660,14 +736,22 @@ static int kxcjk1013_probe(struct i2c_client
>>>> *client,
>>>> }
>>>> }
>>>>
>>>> - ret = devm_iio_device_register(&client->dev, indio_dev);
>>>> + ret = iio_device_register(indio_dev);
>>>> if (ret < 0) {
>>>> dev_err(&client->dev, "unable to register iio device\n");
>>>> goto err_buffer_cleanup;
>>>> }
>>>>
>>>> + ret = pm_runtime_set_active(&client->dev);
>>>> + if (ret)
>>>> + goto err_iio_unregister;
>>>> +
>>>> + pm_runtime_enable(&client->dev);
>>>> +
>>>> return 0;
>>>>
>>>> +err_iio_unregister:
>>>> + iio_device_unregister(indio_dev);
>>>> err_buffer_cleanup:
>>>> if (data->trig_mode)
>>>> iio_triggered_buffer_cleanup(indio_dev);
>>>> @@ -686,6 +770,12 @@ static int kxcjk1013_remove(struct i2c_client
>>>> *client)
>>>> struct iio_dev *indio_dev = i2c_get_clientdata(client);
>>>> struct kxcjk1013_data *data = iio_priv(indio_dev);
>>>>
>>>> + pm_runtime_disable(&client->dev);
>>>> + pm_runtime_set_suspended(&client->dev);
>>>> + pm_runtime_put_noidle(&client->dev);
>>>> +
>>>> + iio_device_unregister(indio_dev);
>>>> +
>>>> if (data->trig_mode) {
>>>> iio_triggered_buffer_cleanup(indio_dev);
>>>> iio_trigger_unregister(data->trig);
>>>> @@ -704,35 +794,67 @@ static int kxcjk1013_suspend(struct device *dev)
>>>> {
>>>> struct iio_dev *indio_dev =
>>>> i2c_get_clientdata(to_i2c_client(dev));
>>>> struct kxcjk1013_data *data = iio_priv(indio_dev);
>>>> + int ret;
>>>>
>>>> mutex_lock(&data->mutex);
>>>> - kxcjk1013_set_mode(data, STANDBY);
>>>> + ret = kxcjk1013_set_mode(data, STANDBY);
>>>> mutex_unlock(&data->mutex);
>>>>
>>>> - return 0;
>>>> + return ret;
>>>> }
>>>>
>>>> static int kxcjk1013_resume(struct device *dev)
>>>> {
>>>> struct iio_dev *indio_dev =
>>>> i2c_get_clientdata(to_i2c_client(dev));
>>>> struct kxcjk1013_data *data = iio_priv(indio_dev);
>>>> + int ret = 0;
>>>>
>>>> mutex_lock(&data->mutex);
>>>> + /* Check, if the suspend occured while active */
>>>> + if (data->trigger_on)
>>>> + ret = kxcjk1013_set_mode(data, OPERATION);
>>>> + mutex_unlock(&data->mutex);
>>>>
>>>> - if (data->power_state)
>>>> - kxcjk1013_set_mode(data, OPERATION);
>>>> + return ret;
>>>> +}
>>>> +#endif
>>>>
>>>> - mutex_unlock(&data->mutex);
>>>> +#ifdef CONFIG_PM_RUNTIME
>>>> +static int kxcjk1013_runtime_suspend(struct device *dev)
>>>> +{
>>>> + struct iio_dev *indio_dev =
>>>> i2c_get_clientdata(to_i2c_client(dev));
>>>> + struct kxcjk1013_data *data = iio_priv(indio_dev);
>>>>
>>>> - return 0;
>>>> + return kxcjk1013_set_mode(data, STANDBY);
>>>> }
>>>>
>>>> -static SIMPLE_DEV_PM_OPS(kxcjk1013_pm_ops, kxcjk1013_suspend,
>>>> kxcjk1013_resume);
>>>> -#define KXCJK1013_PM_OPS (&kxcjk1013_pm_ops)
>>>> -#else
>>>> -#define KXCJK1013_PM_OPS NULL
>>>> +static int kxcjk1013_runtime_resume(struct device *dev)
>>>> +{
>>>> + struct iio_dev *indio_dev =
>>>> i2c_get_clientdata(to_i2c_client(dev));
>>>> + struct kxcjk1013_data *data = iio_priv(indio_dev);
>>>> + int ret;
>>>> + int sleep_val;
>>>> +
>>>> + ret = kxcjk1013_set_mode(data, OPERATION);
>>>> + if (ret < 0)
>>>> + return ret;
>>>> +
>>>> + sleep_val = kxcjk1013_get_startup_times(data);
>>>> + if (sleep_val < 20000)
>>>> + usleep_range(sleep_val, 20000);
>>>> + else
>>>> + msleep_interruptible(sleep_val/1000);
>>>> +
>>>> + return 0;
>>>> +}
>>>> #endif
>>>>
>>>> +static const struct dev_pm_ops kxcjk1013_pm_ops = {
>>>> + SET_SYSTEM_SLEEP_PM_OPS(kxcjk1013_suspend, kxcjk1013_resume)
>>>> + SET_RUNTIME_PM_OPS(kxcjk1013_runtime_suspend,
>>>> + kxcjk1013_runtime_resume, NULL)
>>>> +};
>>>> +
>>>> static const struct acpi_device_id kx_acpi_match[] = {
>>>> {"KXCJ1013", 0},
>>>> { },
>>>> @@ -750,7 +872,7 @@ static struct i2c_driver kxcjk1013_driver = {
>>>> .driver = {
>>>> .name = KXCJK1013_DRV_NAME,
>>>> .acpi_match_table = ACPI_PTR(kx_acpi_match),
>>>> - .pm = KXCJK1013_PM_OPS,
>>>> + .pm = &kxcjk1013_pm_ops,
>>>> },
>>>> .probe = kxcjk1013_probe,
>>>> .remove = kxcjk1013_remove,
>>>>
>>>
>>
>
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 4/6] iio: accel: kxcjk-1013: Set adjustable range
2014-07-17 0:42 [PATCH 1/6] iio: accel: kxcjk-1013: Fix setting frequency Srinivas Pandruvada
2014-07-17 0:42 ` [PATCH 2/6] iio: accel: kxcjk-1013: Use try_reenable to ack intr Srinivas Pandruvada
2014-07-17 0:42 ` [PATCH 3/6] iio: accel: kxcjk-1013: support runtime pm Srinivas Pandruvada
@ 2014-07-17 0:42 ` Srinivas Pandruvada
2014-07-20 15:45 ` Jonathan Cameron
2014-07-17 0:42 ` [PATCH 5/6] iio: accel: kxcjk-1013: Support thresholds Srinivas Pandruvada
` (2 subsequent siblings)
5 siblings, 1 reply; 21+ messages in thread
From: Srinivas Pandruvada @ 2014-07-17 0:42 UTC (permalink / raw)
To: jic23; +Cc: linux-iio, Srinivas Pandruvada
This chip can support 3 different ranges. Allowing range specification.
Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
---
drivers/iio/accel/kxcjk-1013.c | 80 +++++++++++++++++++++++++++++++++++++++++-
1 file changed, 79 insertions(+), 1 deletion(-)
diff --git a/drivers/iio/accel/kxcjk-1013.c b/drivers/iio/accel/kxcjk-1013.c
index 4912143..975f8a6 100644
--- a/drivers/iio/accel/kxcjk-1013.c
+++ b/drivers/iio/accel/kxcjk-1013.c
@@ -70,6 +70,10 @@
#define KXCJK1013_REG_INT_REG1_BIT_IEA BIT(4)
#define KXCJK1013_REG_INT_REG1_BIT_IEN BIT(5)
+#define KXCJK1013_RANGE_2G 0x00
+#define KXCJK1013_RANGE_4G 0x01
+#define KXCJK1013_RANGE_8G 0x02
+
#define KXCJK1013_DATA_MASK_12_BIT 0x0FFF
#define KXCJK1013_MAX_STARTUP_TIME_US 100000
@@ -86,6 +90,7 @@ struct kxcjk1013_data {
struct mutex mutex;
s16 buffer[8];
u8 odr_bits;
+ u8 range;
bool active_high_intr;
bool trigger_on;
};
@@ -120,6 +125,14 @@ static const struct {
{0x02, 21000}, {0x03, 11000}, {0x04, 6400},
{0x05, 3900}, {0x06, 2700}, {0x07, 2100} };
+static const struct {
+ u16 scale;
+ u8 gsel_0;
+ u8 gsel_1;
+} KXCJK1013_scale_table[] = { {9582, 0, 0},
+ {19163, 0, 1},
+ {38326, 1, 0} };
+
static int kxcjk1013_set_mode(struct kxcjk1013_data *data,
enum kxcjk1013_mode mode)
{
@@ -190,6 +203,7 @@ static int kxcjk1013_chip_init(struct kxcjk1013_data *data)
/* Setting range to 4G */
ret |= KXCJK1013_REG_CTRL1_BIT_GSEL0;
ret &= ~KXCJK1013_REG_CTRL1_BIT_GSEL1;
+ data->range = KXCJK1013_RANGE_4G;
/* Set 12 bit mode */
ret |= KXCJK1013_REG_CTRL1_BIT_RES;
@@ -422,6 +436,59 @@ static int kxcjk1013_get_startup_times(struct kxcjk1013_data *data)
return KXCJK1013_MAX_STARTUP_TIME_US;
}
+static int kxcjk1013_set_scale(struct kxcjk1013_data *data, int val)
+{
+ int ret, i;
+ enum kxcjk1013_mode store_mode;
+
+
+ for (i = 0; i < ARRAY_SIZE(KXCJK1013_scale_table); ++i) {
+ if (KXCJK1013_scale_table[i].scale == val) {
+
+ ret = kxcjk1013_get_mode(data, &store_mode);
+ if (ret < 0)
+ return ret;
+
+ ret = kxcjk1013_set_mode(data, STANDBY);
+ if (ret < 0)
+ return ret;
+
+ ret = i2c_smbus_read_byte_data(data->client,
+ KXCJK1013_REG_CTRL1);
+ if (ret < 0) {
+ dev_err(&data->client->dev,
+ "Error reading reg_ctrl1\n");
+ return ret;
+ }
+
+ ret |= (KXCJK1013_scale_table[i].gsel_0 << 3);
+ ret |= (KXCJK1013_scale_table[i].gsel_1 << 4);
+
+ ret = i2c_smbus_write_byte_data(data->client,
+ KXCJK1013_REG_CTRL1,
+ ret);
+ if (ret < 0) {
+ dev_err(&data->client->dev,
+ "Error writing reg_ctrl1\n");
+ return ret;
+ }
+
+ data->range = i;
+ dev_dbg(&data->client->dev, "New Scale range %d\n", i);
+
+ if (store_mode == OPERATION) {
+ ret = kxcjk1013_set_mode(data, OPERATION);
+ if (ret)
+ return ret;
+ }
+
+ return 0;
+ }
+ }
+
+ return -EINVAL;
+}
+
static int kxcjk1013_read_raw(struct iio_dev *indio_dev,
struct iio_chan_spec const *chan, int *val,
int *val2, long mask)
@@ -458,7 +525,7 @@ static int kxcjk1013_read_raw(struct iio_dev *indio_dev,
case IIO_CHAN_INFO_SCALE:
*val = 0;
- *val2 = 19163; /* range +-4g (4/2047*9.806650) */
+ *val2 = KXCJK1013_scale_table[data->range].scale;
return IIO_VAL_INT_PLUS_MICRO;
case IIO_CHAN_INFO_SAMP_FREQ:
@@ -485,6 +552,14 @@ static int kxcjk1013_write_raw(struct iio_dev *indio_dev,
ret = kxcjk1013_set_odr(data, val, val2);
mutex_unlock(&data->mutex);
break;
+ case IIO_CHAN_INFO_SCALE:
+ if (val)
+ return -EINVAL;
+
+ mutex_lock(&data->mutex);
+ ret = kxcjk1013_set_scale(data, val2);
+ mutex_unlock(&data->mutex);
+ break;
default:
ret = -EINVAL;
}
@@ -506,8 +581,11 @@ static int kxcjk1013_validate_trigger(struct iio_dev *indio_dev,
static IIO_CONST_ATTR_SAMP_FREQ_AVAIL(
"0.781000 1.563000 3.125000 6.250000 12.500000 25 50 100 200 400 800 1600");
+static IIO_CONST_ATTR(in_accel_scale_available, "0.009582 0.019163 0.038326");
+
static struct attribute *kxcjk1013_attributes[] = {
&iio_const_attr_sampling_frequency_available.dev_attr.attr,
+ &iio_const_attr_in_accel_scale_available.dev_attr.attr,
NULL,
};
--
1.7.11.7
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH 4/6] iio: accel: kxcjk-1013: Set adjustable range
2014-07-17 0:42 ` [PATCH 4/6] iio: accel: kxcjk-1013: Set adjustable range Srinivas Pandruvada
@ 2014-07-20 15:45 ` Jonathan Cameron
2014-07-24 23:04 ` Srinivas Pandruvada
0 siblings, 1 reply; 21+ messages in thread
From: Jonathan Cameron @ 2014-07-20 15:45 UTC (permalink / raw)
To: Srinivas Pandruvada; +Cc: linux-iio
On 17/07/14 01:42, Srinivas Pandruvada wrote:
> This chip can support 3 different ranges. Allowing range specification.
>
> Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
A few minor suggestions. Basically fine though.
> ---
> drivers/iio/accel/kxcjk-1013.c | 80 +++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 79 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/iio/accel/kxcjk-1013.c b/drivers/iio/accel/kxcjk-1013.c
> index 4912143..975f8a6 100644
> --- a/drivers/iio/accel/kxcjk-1013.c
> +++ b/drivers/iio/accel/kxcjk-1013.c
> @@ -70,6 +70,10 @@
> #define KXCJK1013_REG_INT_REG1_BIT_IEA BIT(4)
> #define KXCJK1013_REG_INT_REG1_BIT_IEN BIT(5)
>
> +#define KXCJK1013_RANGE_2G 0x00
> +#define KXCJK1013_RANGE_4G 0x01
> +#define KXCJK1013_RANGE_8G 0x02
These are effectively used as an enum? Hence I'd make them one.
> +
> #define KXCJK1013_DATA_MASK_12_BIT 0x0FFF
> #define KXCJK1013_MAX_STARTUP_TIME_US 100000
>
> @@ -86,6 +90,7 @@ struct kxcjk1013_data {
> struct mutex mutex;
> s16 buffer[8];
> u8 odr_bits;
> + u8 range;
with an enum above this becomes a little more obvious too.
> bool active_high_intr;
> bool trigger_on;
> };
> @@ -120,6 +125,14 @@ static const struct {
> {0x02, 21000}, {0x03, 11000}, {0x04, 6400},
> {0x05, 3900}, {0x06, 2700}, {0x07, 2100} };
>
> +static const struct {
> + u16 scale;
> + u8 gsel_0;
> + u8 gsel_1;
Could use a bitfield to make it clear these are single bits..
> +} KXCJK1013_scale_table[] = { {9582, 0, 0},
> + {19163, 0, 1},
> + {38326, 1, 0} };
> +
Once the _4G etc defines are an enum, you can explicitly set the values
in the table using [KXCJK1013_RANGE_2G] = etc
> static int kxcjk1013_set_mode(struct kxcjk1013_data *data,
> enum kxcjk1013_mode mode)
> {
> @@ -190,6 +203,7 @@ static int kxcjk1013_chip_init(struct kxcjk1013_data *data)
> /* Setting range to 4G */
> ret |= KXCJK1013_REG_CTRL1_BIT_GSEL0;
> ret &= ~KXCJK1013_REG_CTRL1_BIT_GSEL1;
> + data->range = KXCJK1013_RANGE_4G;
I'd almost be tempted to call the set function here instead of hand coding
this case. I know it will involve more hardware writes, but it will give
slightly more obviously correct code (one path to set it rather than two).
>
> /* Set 12 bit mode */
> ret |= KXCJK1013_REG_CTRL1_BIT_RES;
> @@ -422,6 +436,59 @@ static int kxcjk1013_get_startup_times(struct kxcjk1013_data *data)
> return KXCJK1013_MAX_STARTUP_TIME_US;
> }
>
> +static int kxcjk1013_set_scale(struct kxcjk1013_data *data, int val)
> +{
> + int ret, i;
> + enum kxcjk1013_mode store_mode;
> +
> +
> + for (i = 0; i < ARRAY_SIZE(KXCJK1013_scale_table); ++i) {
> + if (KXCJK1013_scale_table[i].scale == val) {
> +
> + ret = kxcjk1013_get_mode(data, &store_mode);
> + if (ret < 0)
> + return ret;
> +
> + ret = kxcjk1013_set_mode(data, STANDBY);
> + if (ret < 0)
> + return ret;
> +
> + ret = i2c_smbus_read_byte_data(data->client,
> + KXCJK1013_REG_CTRL1);
> + if (ret < 0) {
> + dev_err(&data->client->dev,
> + "Error reading reg_ctrl1\n");
> + return ret;
> + }
> +
> + ret |= (KXCJK1013_scale_table[i].gsel_0 << 3);
> + ret |= (KXCJK1013_scale_table[i].gsel_1 << 4);
> +
> + ret = i2c_smbus_write_byte_data(data->client,
> + KXCJK1013_REG_CTRL1,
> + ret);
> + if (ret < 0) {
> + dev_err(&data->client->dev,
> + "Error writing reg_ctrl1\n");
> + return ret;
> + }
> +
> + data->range = i;
> + dev_dbg(&data->client->dev, "New Scale range %d\n", i);
> +
> + if (store_mode == OPERATION) {
> + ret = kxcjk1013_set_mode(data, OPERATION);
> + if (ret)
> + return ret;
> + }
> +
> + return 0;
> + }
> + }
> +
> + return -EINVAL;
> +}
> +
> static int kxcjk1013_read_raw(struct iio_dev *indio_dev,
> struct iio_chan_spec const *chan, int *val,
> int *val2, long mask)
> @@ -458,7 +525,7 @@ static int kxcjk1013_read_raw(struct iio_dev *indio_dev,
>
> case IIO_CHAN_INFO_SCALE:
> *val = 0;
> - *val2 = 19163; /* range +-4g (4/2047*9.806650) */
> + *val2 = KXCJK1013_scale_table[data->range].scale;
> return IIO_VAL_INT_PLUS_MICRO;
>
> case IIO_CHAN_INFO_SAMP_FREQ:
> @@ -485,6 +552,14 @@ static int kxcjk1013_write_raw(struct iio_dev *indio_dev,
> ret = kxcjk1013_set_odr(data, val, val2);
> mutex_unlock(&data->mutex);
> break;
> + case IIO_CHAN_INFO_SCALE:
> + if (val)
> + return -EINVAL;
> +
> + mutex_lock(&data->mutex);
> + ret = kxcjk1013_set_scale(data, val2);
> + mutex_unlock(&data->mutex);
> + break;
> default:
> ret = -EINVAL;
> }
> @@ -506,8 +581,11 @@ static int kxcjk1013_validate_trigger(struct iio_dev *indio_dev,
> static IIO_CONST_ATTR_SAMP_FREQ_AVAIL(
> "0.781000 1.563000 3.125000 6.250000 12.500000 25 50 100 200 400 800 1600");
>
> +static IIO_CONST_ATTR(in_accel_scale_available, "0.009582 0.019163 0.038326");
> +
> static struct attribute *kxcjk1013_attributes[] = {
> &iio_const_attr_sampling_frequency_available.dev_attr.attr,
> + &iio_const_attr_in_accel_scale_available.dev_attr.attr,
> NULL,
> };
>
>
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH 4/6] iio: accel: kxcjk-1013: Set adjustable range
2014-07-20 15:45 ` Jonathan Cameron
@ 2014-07-24 23:04 ` Srinivas Pandruvada
2014-07-27 13:51 ` Jonathan Cameron
0 siblings, 1 reply; 21+ messages in thread
From: Srinivas Pandruvada @ 2014-07-24 23:04 UTC (permalink / raw)
To: Jonathan Cameron; +Cc: linux-iio
On 07/20/2014 08:45 AM, Jonathan Cameron wrote:
> On 17/07/14 01:42, Srinivas Pandruvada wrote:
>> This chip can support 3 different ranges. Allowing range specification.
>>
>> Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> A few minor suggestions. Basically fine though.
>> ---
>> drivers/iio/accel/kxcjk-1013.c | 80
>> +++++++++++++++++++++++++++++++++++++++++-
>> 1 file changed, 79 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/iio/accel/kxcjk-1013.c
>> b/drivers/iio/accel/kxcjk-1013.c
>> index 4912143..975f8a6 100644
>> --- a/drivers/iio/accel/kxcjk-1013.c
>> +++ b/drivers/iio/accel/kxcjk-1013.c
>> @@ -70,6 +70,10 @@
>> #define KXCJK1013_REG_INT_REG1_BIT_IEA BIT(4)
>> #define KXCJK1013_REG_INT_REG1_BIT_IEN BIT(5)
>>
>> +#define KXCJK1013_RANGE_2G 0x00
>> +#define KXCJK1013_RANGE_4G 0x01
>> +#define KXCJK1013_RANGE_8G 0x02
> These are effectively used as an enum? Hence I'd make them one.
>
>> +
>> #define KXCJK1013_DATA_MASK_12_BIT 0x0FFF
>> #define KXCJK1013_MAX_STARTUP_TIME_US 100000
>>
>> @@ -86,6 +90,7 @@ struct kxcjk1013_data {
>> struct mutex mutex;
>> s16 buffer[8];
>> u8 odr_bits;
>> + u8 range;
> with an enum above this becomes a little more obvious too.
>> bool active_high_intr;
>> bool trigger_on;
>> };
>> @@ -120,6 +125,14 @@ static const struct {
>> {0x02, 21000}, {0x03, 11000}, {0x04, 6400},
>> {0x05, 3900}, {0x06, 2700}, {0x07, 2100} };
>>
>> +static const struct {
>> + u16 scale;
>> + u8 gsel_0;
>> + u8 gsel_1;
> Could use a bitfield to make it clear these are single bits..
I thought bit fields is not recommended for portability concerns in kernel.
Hence avoided using bit fields.
Thanks,
Srinivas
>> +} KXCJK1013_scale_table[] = { {9582, 0, 0},
>> + {19163, 0, 1},
>> + {38326, 1, 0} };
>> +
> Once the _4G etc defines are an enum, you can explicitly set the values
> in the table using [KXCJK1013_RANGE_2G] = etc
>
>> static int kxcjk1013_set_mode(struct kxcjk1013_data *data,
>> enum kxcjk1013_mode mode)
>> {
>> @@ -190,6 +203,7 @@ static int kxcjk1013_chip_init(struct
>> kxcjk1013_data *data)
>> /* Setting range to 4G */
>> ret |= KXCJK1013_REG_CTRL1_BIT_GSEL0;
>> ret &= ~KXCJK1013_REG_CTRL1_BIT_GSEL1;
>> + data->range = KXCJK1013_RANGE_4G;
> I'd almost be tempted to call the set function here instead of hand
> coding
> this case. I know it will involve more hardware writes, but it will give
> slightly more obviously correct code (one path to set it rather than
> two).
>>
>> /* Set 12 bit mode */
>> ret |= KXCJK1013_REG_CTRL1_BIT_RES;
>> @@ -422,6 +436,59 @@ static int kxcjk1013_get_startup_times(struct
>> kxcjk1013_data *data)
>> return KXCJK1013_MAX_STARTUP_TIME_US;
>> }
>>
>> +static int kxcjk1013_set_scale(struct kxcjk1013_data *data, int val)
>> +{
>> + int ret, i;
>> + enum kxcjk1013_mode store_mode;
>> +
>> +
>> + for (i = 0; i < ARRAY_SIZE(KXCJK1013_scale_table); ++i) {
>> + if (KXCJK1013_scale_table[i].scale == val) {
>> +
>> + ret = kxcjk1013_get_mode(data, &store_mode);
>> + if (ret < 0)
>> + return ret;
>> +
>> + ret = kxcjk1013_set_mode(data, STANDBY);
>> + if (ret < 0)
>> + return ret;
>> +
>> + ret = i2c_smbus_read_byte_data(data->client,
>> + KXCJK1013_REG_CTRL1);
>> + if (ret < 0) {
>> + dev_err(&data->client->dev,
>> + "Error reading reg_ctrl1\n");
>> + return ret;
>> + }
>> +
>> + ret |= (KXCJK1013_scale_table[i].gsel_0 << 3);
>> + ret |= (KXCJK1013_scale_table[i].gsel_1 << 4);
>> +
>> + ret = i2c_smbus_write_byte_data(data->client,
>> + KXCJK1013_REG_CTRL1,
>> + ret);
>> + if (ret < 0) {
>> + dev_err(&data->client->dev,
>> + "Error writing reg_ctrl1\n");
>> + return ret;
>> + }
>> +
>> + data->range = i;
>> + dev_dbg(&data->client->dev, "New Scale range %d\n", i);
>> +
>> + if (store_mode == OPERATION) {
>> + ret = kxcjk1013_set_mode(data, OPERATION);
>> + if (ret)
>> + return ret;
>> + }
>> +
>> + return 0;
>> + }
>> + }
>> +
>> + return -EINVAL;
>> +}
>> +
>> static int kxcjk1013_read_raw(struct iio_dev *indio_dev,
>> struct iio_chan_spec const *chan, int *val,
>> int *val2, long mask)
>> @@ -458,7 +525,7 @@ static int kxcjk1013_read_raw(struct iio_dev
>> *indio_dev,
>>
>> case IIO_CHAN_INFO_SCALE:
>> *val = 0;
>> - *val2 = 19163; /* range +-4g (4/2047*9.806650) */
>> + *val2 = KXCJK1013_scale_table[data->range].scale;
>> return IIO_VAL_INT_PLUS_MICRO;
>>
>> case IIO_CHAN_INFO_SAMP_FREQ:
>> @@ -485,6 +552,14 @@ static int kxcjk1013_write_raw(struct iio_dev
>> *indio_dev,
>> ret = kxcjk1013_set_odr(data, val, val2);
>> mutex_unlock(&data->mutex);
>> break;
>> + case IIO_CHAN_INFO_SCALE:
>> + if (val)
>> + return -EINVAL;
>> +
>> + mutex_lock(&data->mutex);
>> + ret = kxcjk1013_set_scale(data, val2);
>> + mutex_unlock(&data->mutex);
>> + break;
>> default:
>> ret = -EINVAL;
>> }
>> @@ -506,8 +581,11 @@ static int kxcjk1013_validate_trigger(struct
>> iio_dev *indio_dev,
>> static IIO_CONST_ATTR_SAMP_FREQ_AVAIL(
>> "0.781000 1.563000 3.125000 6.250000 12.500000 25 50 100 200
>> 400 800 1600");
>>
>> +static IIO_CONST_ATTR(in_accel_scale_available, "0.009582 0.019163
>> 0.038326");
>> +
>> static struct attribute *kxcjk1013_attributes[] = {
>> &iio_const_attr_sampling_frequency_available.dev_attr.attr,
>> + &iio_const_attr_in_accel_scale_available.dev_attr.attr,
>> NULL,
>> };
>>
>>
>
>
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH 4/6] iio: accel: kxcjk-1013: Set adjustable range
2014-07-24 23:04 ` Srinivas Pandruvada
@ 2014-07-27 13:51 ` Jonathan Cameron
0 siblings, 0 replies; 21+ messages in thread
From: Jonathan Cameron @ 2014-07-27 13:51 UTC (permalink / raw)
To: Srinivas Pandruvada; +Cc: linux-iio
On 25/07/14 00:04, Srinivas Pandruvada wrote:
>
> On 07/20/2014 08:45 AM, Jonathan Cameron wrote:
>> On 17/07/14 01:42, Srinivas Pandruvada wrote:
>>> This chip can support 3 different ranges. Allowing range specification.
>>>
>>> Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
>> A few minor suggestions. Basically fine though.
>>> ---
>>> drivers/iio/accel/kxcjk-1013.c | 80 +++++++++++++++++++++++++++++++++++++++++-
>>> 1 file changed, 79 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/iio/accel/kxcjk-1013.c b/drivers/iio/accel/kxcjk-1013.c
>>> index 4912143..975f8a6 100644
>>> --- a/drivers/iio/accel/kxcjk-1013.c
>>> +++ b/drivers/iio/accel/kxcjk-1013.c
>>> @@ -70,6 +70,10 @@
>>> #define KXCJK1013_REG_INT_REG1_BIT_IEA BIT(4)
>>> #define KXCJK1013_REG_INT_REG1_BIT_IEN BIT(5)
>>>
>>> +#define KXCJK1013_RANGE_2G 0x00
>>> +#define KXCJK1013_RANGE_4G 0x01
>>> +#define KXCJK1013_RANGE_8G 0x02
>> These are effectively used as an enum? Hence I'd make them one.
>>
>>> +
>>> #define KXCJK1013_DATA_MASK_12_BIT 0x0FFF
>>> #define KXCJK1013_MAX_STARTUP_TIME_US 100000
>>>
>>> @@ -86,6 +90,7 @@ struct kxcjk1013_data {
>>> struct mutex mutex;
>>> s16 buffer[8];
>>> u8 odr_bits;
>>> + u8 range;
>> with an enum above this becomes a little more obvious too.
>>> bool active_high_intr;
>>> bool trigger_on;
>>> };
>>> @@ -120,6 +125,14 @@ static const struct {
>>> {0x02, 21000}, {0x03, 11000}, {0x04, 6400},
>>> {0x05, 3900}, {0x06, 2700}, {0x07, 2100} };
>>>
>>> +static const struct {
>>> + u16 scale;
>>> + u8 gsel_0;
>>> + u8 gsel_1;
>> Could use a bitfield to make it clear these are single bits..
> I thought bit fields is not recommended for portability concerns in kernel.
> Hence avoided using bit fields.
I believe that is more about anything that might be exposed to userspace rather
than there being any issues using them internally. Still, it's not something
I feel strongly about either way!
J
>
> Thanks,
> Srinivas
>
>>> +} KXCJK1013_scale_table[] = { {9582, 0, 0},
>>> + {19163, 0, 1},
>>> + {38326, 1, 0} };
>>> +
>> Once the _4G etc defines are an enum, you can explicitly set the values
>> in the table using [KXCJK1013_RANGE_2G] = etc
>>
>>> static int kxcjk1013_set_mode(struct kxcjk1013_data *data,
>>> enum kxcjk1013_mode mode)
>>> {
>>> @@ -190,6 +203,7 @@ static int kxcjk1013_chip_init(struct kxcjk1013_data *data)
>>> /* Setting range to 4G */
>>> ret |= KXCJK1013_REG_CTRL1_BIT_GSEL0;
>>> ret &= ~KXCJK1013_REG_CTRL1_BIT_GSEL1;
>>> + data->range = KXCJK1013_RANGE_4G;
>> I'd almost be tempted to call the set function here instead of hand coding
>> this case. I know it will involve more hardware writes, but it will give
>> slightly more obviously correct code (one path to set it rather than two).
>>>
>>> /* Set 12 bit mode */
>>> ret |= KXCJK1013_REG_CTRL1_BIT_RES;
>>> @@ -422,6 +436,59 @@ static int kxcjk1013_get_startup_times(struct kxcjk1013_data *data)
>>> return KXCJK1013_MAX_STARTUP_TIME_US;
>>> }
>>>
>>> +static int kxcjk1013_set_scale(struct kxcjk1013_data *data, int val)
>>> +{
>>> + int ret, i;
>>> + enum kxcjk1013_mode store_mode;
>>> +
>>> +
>>> + for (i = 0; i < ARRAY_SIZE(KXCJK1013_scale_table); ++i) {
>>> + if (KXCJK1013_scale_table[i].scale == val) {
>>> +
>>> + ret = kxcjk1013_get_mode(data, &store_mode);
>>> + if (ret < 0)
>>> + return ret;
>>> +
>>> + ret = kxcjk1013_set_mode(data, STANDBY);
>>> + if (ret < 0)
>>> + return ret;
>>> +
>>> + ret = i2c_smbus_read_byte_data(data->client,
>>> + KXCJK1013_REG_CTRL1);
>>> + if (ret < 0) {
>>> + dev_err(&data->client->dev,
>>> + "Error reading reg_ctrl1\n");
>>> + return ret;
>>> + }
>>> +
>>> + ret |= (KXCJK1013_scale_table[i].gsel_0 << 3);
>>> + ret |= (KXCJK1013_scale_table[i].gsel_1 << 4);
>>> +
>>> + ret = i2c_smbus_write_byte_data(data->client,
>>> + KXCJK1013_REG_CTRL1,
>>> + ret);
>>> + if (ret < 0) {
>>> + dev_err(&data->client->dev,
>>> + "Error writing reg_ctrl1\n");
>>> + return ret;
>>> + }
>>> +
>>> + data->range = i;
>>> + dev_dbg(&data->client->dev, "New Scale range %d\n", i);
>>> +
>>> + if (store_mode == OPERATION) {
>>> + ret = kxcjk1013_set_mode(data, OPERATION);
>>> + if (ret)
>>> + return ret;
>>> + }
>>> +
>>> + return 0;
>>> + }
>>> + }
>>> +
>>> + return -EINVAL;
>>> +}
>>> +
>>> static int kxcjk1013_read_raw(struct iio_dev *indio_dev,
>>> struct iio_chan_spec const *chan, int *val,
>>> int *val2, long mask)
>>> @@ -458,7 +525,7 @@ static int kxcjk1013_read_raw(struct iio_dev *indio_dev,
>>>
>>> case IIO_CHAN_INFO_SCALE:
>>> *val = 0;
>>> - *val2 = 19163; /* range +-4g (4/2047*9.806650) */
>>> + *val2 = KXCJK1013_scale_table[data->range].scale;
>>> return IIO_VAL_INT_PLUS_MICRO;
>>>
>>> case IIO_CHAN_INFO_SAMP_FREQ:
>>> @@ -485,6 +552,14 @@ static int kxcjk1013_write_raw(struct iio_dev *indio_dev,
>>> ret = kxcjk1013_set_odr(data, val, val2);
>>> mutex_unlock(&data->mutex);
>>> break;
>>> + case IIO_CHAN_INFO_SCALE:
>>> + if (val)
>>> + return -EINVAL;
>>> +
>>> + mutex_lock(&data->mutex);
>>> + ret = kxcjk1013_set_scale(data, val2);
>>> + mutex_unlock(&data->mutex);
>>> + break;
>>> default:
>>> ret = -EINVAL;
>>> }
>>> @@ -506,8 +581,11 @@ static int kxcjk1013_validate_trigger(struct iio_dev *indio_dev,
>>> static IIO_CONST_ATTR_SAMP_FREQ_AVAIL(
>>> "0.781000 1.563000 3.125000 6.250000 12.500000 25 50 100 200 400 800 1600");
>>>
>>> +static IIO_CONST_ATTR(in_accel_scale_available, "0.009582 0.019163 0.038326");
>>> +
>>> static struct attribute *kxcjk1013_attributes[] = {
>>> &iio_const_attr_sampling_frequency_available.dev_attr.attr,
>>> + &iio_const_attr_in_accel_scale_available.dev_attr.attr,
>>> NULL,
>>> };
>>>
>>>
>>
>>
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 5/6] iio: accel: kxcjk-1013: Support thresholds
2014-07-17 0:42 [PATCH 1/6] iio: accel: kxcjk-1013: Fix setting frequency Srinivas Pandruvada
` (2 preceding siblings ...)
2014-07-17 0:42 ` [PATCH 4/6] iio: accel: kxcjk-1013: Set adjustable range Srinivas Pandruvada
@ 2014-07-17 0:42 ` Srinivas Pandruvada
2014-07-20 16:04 ` Jonathan Cameron
2014-07-17 0:42 ` [PATCH 6/6] iio: accel: kxcjk-1013: Increment ref counter for indio_dev->trig Srinivas Pandruvada
2014-07-20 15:19 ` [PATCH 1/6] iio: accel: kxcjk-1013: Fix setting frequency Jonathan Cameron
5 siblings, 1 reply; 21+ messages in thread
From: Srinivas Pandruvada @ 2014-07-17 0:42 UTC (permalink / raw)
To: jic23; +Cc: linux-iio, Srinivas Pandruvada
This chip can operate in two modes. In one mode it can issue periodic
interrupts based on sample rate setting or when there is a motion.
Using events mechanism to allow configuration and receive events.
Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
---
drivers/iio/accel/kxcjk-1013.c | 238 +++++++++++++++++++++++++++++++++++++++--
1 file changed, 230 insertions(+), 8 deletions(-)
diff --git a/drivers/iio/accel/kxcjk-1013.c b/drivers/iio/accel/kxcjk-1013.c
index 975f8a6..f5bb682 100644
--- a/drivers/iio/accel/kxcjk-1013.c
+++ b/drivers/iio/accel/kxcjk-1013.c
@@ -27,6 +27,7 @@
#include <linux/iio/sysfs.h>
#include <linux/iio/buffer.h>
#include <linux/iio/trigger.h>
+#include <linux/iio/events.h>
#include <linux/iio/trigger_consumer.h>
#include <linux/iio/triggered_buffer.h>
#include <linux/iio/accel/kxcjk_1013.h>
@@ -92,6 +93,9 @@ struct kxcjk1013_data {
u8 odr_bits;
u8 range;
bool active_high_intr;
+ int ev_enable_state;
+ int wake_thres;
+ int wake_dur;
bool trigger_on;
};
@@ -116,6 +120,23 @@ static const struct {
{200, 0, 0x04}, {400, 0, 0x05}, {800, 0, 0x06},
{1600, 0, 0x07} };
+static const struct {
+ int val;
+ int val2;
+ int odr_bits;
+} wake_odr_data_rate_table[] = { {0, 781000, 0x00},
+ {1, 563000, 0x01},
+ {3, 125000, 0x02},
+ {6, 250000, 0x03},
+ {12, 500000, 0x04},
+ {25, 0, 0x05},
+ {50, 0, 0x06},
+ {100, 0, 0x06},
+ {200, 0, 0x06},
+ {400, 0, 0x06},
+ {800, 0, 0x06},
+ {1600, 0, 0x06} };
+
/* Refer to section 4 of the specification */
static const struct {
int odr_bits;
@@ -283,10 +304,36 @@ static int kxcjk1013_set_power_state(struct kxcjk1013_data *data, bool on)
return 0;
}
+static int kxcjk1013_chip_update_thresholds(struct kxcjk1013_data *data)
+{
+ int ret;
+
+ ret = i2c_smbus_write_byte_data(data->client,
+ KXCJK1013_REG_WAKE_TIMER,
+ data->wake_dur);
+ if (ret < 0) {
+ dev_err(&data->client->dev,
+ "Error writing reg_wake_timer\n");
+ return ret;
+ }
+
+ ret = i2c_smbus_write_byte_data(data->client,
+ KXCJK1013_REG_WAKE_THRES,
+ data->wake_thres);
+ if (ret < 0) {
+ dev_err(&data->client->dev,
+ "Error writing reg_wake_thres\n");
+ return ret;
+ }
+
+ return 0;
+}
+
static int kxcjk1013_chip_setup_interrupt(struct kxcjk1013_data *data,
bool status)
{
int ret;
+ u8 intr_bit;
enum kxcjk1013_mode store_mode;
ret = kxcjk1013_get_mode(data, &store_mode);
@@ -316,6 +363,16 @@ static int kxcjk1013_chip_setup_interrupt(struct kxcjk1013_data *data,
return ret;
}
+ if (data->wake_thres) {
+ if (status) {
+ ret = kxcjk1013_chip_update_thresholds(data);
+ if (ret < 0)
+ return ret;
+ }
+ intr_bit = KXCJK1013_REG_CTRL1_BIT_WUFE;
+ } else
+ intr_bit = KXCJK1013_REG_CTRL1_BIT_DRDY;
+
ret = i2c_smbus_read_byte_data(data->client, KXCJK1013_REG_CTRL1);
if (ret < 0) {
dev_err(&data->client->dev, "Error reading reg_ctrl1\n");
@@ -323,9 +380,10 @@ static int kxcjk1013_chip_setup_interrupt(struct kxcjk1013_data *data,
}
if (status)
- ret |= KXCJK1013_REG_CTRL1_BIT_DRDY;
+ ret |= intr_bit;
else
- ret &= ~KXCJK1013_REG_CTRL1_BIT_DRDY;
+ ret &= ~(KXCJK1013_REG_CTRL1_BIT_WUFE |
+ KXCJK1013_REG_CTRL1_BIT_DRDY);
ret = i2c_smbus_write_byte_data(data->client,
KXCJK1013_REG_CTRL1, ret);
@@ -357,6 +415,20 @@ static int kxcjk1013_convert_freq_to_bit(int val, int val2)
return -EINVAL;
}
+static int kxcjk1013_convert_wake_odr_to_bit(int val, int val2)
+{
+ int i;
+
+ for (i = 0; i < ARRAY_SIZE(wake_odr_data_rate_table); ++i) {
+ if (wake_odr_data_rate_table[i].val == val &&
+ wake_odr_data_rate_table[i].val2 == val2) {
+ return wake_odr_data_rate_table[i].odr_bits;
+ }
+ }
+
+ return -EINVAL;
+}
+
static int kxcjk1013_set_odr(struct kxcjk1013_data *data, int val, int val2)
{
int ret;
@@ -385,6 +457,17 @@ static int kxcjk1013_set_odr(struct kxcjk1013_data *data, int val, int val2)
data->odr_bits = odr_bits;
+ odr_bits = kxcjk1013_convert_wake_odr_to_bit(val, val2);
+ if (odr_bits < 0)
+ return odr_bits;
+
+ ret = i2c_smbus_write_byte_data(data->client, KXCJK1013_REG_CTRL2,
+ odr_bits);
+ if (ret < 0) {
+ dev_err(&data->client->dev, "Error writing reg_ctrl2\n");
+ return ret;
+ }
+
if (store_mode == OPERATION) {
ret = kxcjk1013_set_mode(data, OPERATION);
if (ret < 0)
@@ -567,6 +650,94 @@ static int kxcjk1013_write_raw(struct iio_dev *indio_dev,
return ret;
}
+static int kxcjk1013_read_event(struct iio_dev *indio_dev,
+ const struct iio_chan_spec *chan,
+ enum iio_event_type type,
+ enum iio_event_direction dir,
+ enum iio_event_info info,
+ int *val, int *val2)
+{
+ struct kxcjk1013_data *data = iio_priv(indio_dev);
+
+ *val2 = 0;
+ switch (info) {
+ case IIO_EV_INFO_VALUE:
+ *val = data->wake_thres;
+ break;
+ case IIO_EV_INFO_PERIOD:
+ *val = data->wake_dur;
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ return IIO_VAL_INT;
+}
+
+static int kxcjk1013_write_event(struct iio_dev *indio_dev,
+ const struct iio_chan_spec *chan,
+ enum iio_event_type type,
+ enum iio_event_direction dir,
+ enum iio_event_info info,
+ int val, int val2)
+{
+ struct kxcjk1013_data *data = iio_priv(indio_dev);
+
+ switch (info) {
+ case IIO_EV_INFO_VALUE:
+ data->wake_thres = val;
+ break;
+ case IIO_EV_INFO_PERIOD:
+ data->wake_dur = val;
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
+static int kxcjk1013_read_event_config(struct iio_dev *indio_dev,
+ const struct iio_chan_spec *chan,
+ enum iio_event_type type,
+ enum iio_event_direction dir)
+{
+
+ struct kxcjk1013_data *data = iio_priv(indio_dev);
+
+ return data->ev_enable_state;
+}
+
+static int kxcjk1013_write_event_config(struct iio_dev *indio_dev,
+ const struct iio_chan_spec *chan,
+ enum iio_event_type type,
+ enum iio_event_direction dir,
+ int state)
+{
+ struct kxcjk1013_data *data = iio_priv(indio_dev);
+ int ret;
+
+ if (data->trigger_on)
+ return -EAGAIN;
+
+ if (state && data->ev_enable_state)
+ return 0;
+
+ mutex_lock(&data->mutex);
+ ret = kxcjk1013_chip_setup_interrupt(data, state);
+ if (!ret) {
+ ret = kxcjk1013_set_power_state(data, state);
+ if (ret < 0) {
+ mutex_unlock(&data->mutex);
+ return ret;
+ }
+ }
+ data->ev_enable_state = state;
+ mutex_unlock(&data->mutex);
+
+ return 0;
+}
+
static int kxcjk1013_validate_trigger(struct iio_dev *indio_dev,
struct iio_trigger *trig)
{
@@ -593,6 +764,14 @@ static const struct attribute_group kxcjk1013_attrs_group = {
.attrs = kxcjk1013_attributes,
};
+static const struct iio_event_spec kxcjk1013_event = {
+ .type = IIO_EV_TYPE_THRESH,
+ .dir = IIO_EV_DIR_EITHER,
+ .mask_separate = BIT(IIO_EV_INFO_VALUE) |
+ BIT(IIO_EV_INFO_ENABLE) |
+ BIT(IIO_EV_INFO_PERIOD)
+};
+
#define KXCJK1013_CHANNEL(_axis) { \
.type = IIO_ACCEL, \
.modified = 1, \
@@ -608,6 +787,8 @@ static const struct attribute_group kxcjk1013_attrs_group = {
.shift = 4, \
.endianness = IIO_LE, \
}, \
+ .event_spec = &kxcjk1013_event, \
+ .num_event_specs = 1 \
}
static const struct iio_chan_spec kxcjk1013_channels[] = {
@@ -621,6 +802,10 @@ static const struct iio_info kxcjk1013_info = {
.attrs = &kxcjk1013_attrs_group,
.read_raw = kxcjk1013_read_raw,
.write_raw = kxcjk1013_write_raw,
+ .read_event_value = kxcjk1013_read_event,
+ .write_event_value = kxcjk1013_write_event,
+ .write_event_config = kxcjk1013_write_event_config,
+ .read_event_config = kxcjk1013_read_event_config,
.validate_trigger = kxcjk1013_validate_trigger,
.driver_module = THIS_MODULE,
};
@@ -675,6 +860,9 @@ static int kxcjk1013_data_rdy_trigger_set_state(struct iio_trigger *trig,
struct kxcjk1013_data *data = iio_priv(indio_dev);
int ret;
+ if (data->ev_enable_state)
+ return -EAGAIN;
+
if (state && data->trigger_on)
return 0;
@@ -699,6 +887,38 @@ static const struct iio_trigger_ops kxcjk1013_trigger_ops = {
.owner = THIS_MODULE,
};
+static irqreturn_t kxcjk1013_event_handler(int irq, void *private)
+{
+ struct iio_dev *indio_dev = private;
+ struct kxcjk1013_data *data = iio_priv(indio_dev);
+ int ret;
+
+ iio_push_event(indio_dev, IIO_MOD_EVENT_CODE(IIO_ACCEL,
+ 0,
+ IIO_MOD_X_OR_Y_OR_Z,
+ IIO_EV_TYPE_THRESH,
+ IIO_EV_DIR_EITHER),
+ iio_get_time_ns());
+
+ ret = i2c_smbus_read_byte_data(data->client, KXCJK1013_REG_INT_REL);
+ if (ret < 0)
+ dev_err(&data->client->dev, "Error reading reg_int_rel\n");
+
+ return IRQ_HANDLED;
+}
+
+static irqreturn_t kxcjk1013_data_rdy_trig_poll(int irq, void *private)
+{
+ struct iio_dev *indio_dev = private;
+ struct kxcjk1013_data *data = iio_priv(indio_dev);
+
+ if (data->trigger_on) {
+ iio_trigger_poll(data->trig);
+ return IRQ_HANDLED;
+ } else
+ return IRQ_WAKE_THREAD;
+}
+
static int kxcjk1013_acpi_gpio_probe(struct i2c_client *client,
struct kxcjk1013_data *data)
{
@@ -783,11 +1003,13 @@ static int kxcjk1013_probe(struct i2c_client *client,
data->trig_mode = true;
- ret = devm_request_irq(&client->dev, client->irq,
- iio_trigger_generic_data_rdy_poll,
- IRQF_TRIGGER_RISING,
- KXCJK1013_IRQ_NAME,
- trig);
+ ret = devm_request_threaded_irq(&client->dev, client->irq,
+ kxcjk1013_data_rdy_trig_poll,
+ kxcjk1013_event_handler,
+ IRQF_TRIGGER_RISING |
+ IRQF_ONESHOT,
+ KXCJK1013_IRQ_NAME,
+ indio_dev);
if (ret) {
dev_err(&client->dev, "unable to request IRQ\n");
goto err_trigger_free;
@@ -889,7 +1111,7 @@ static int kxcjk1013_resume(struct device *dev)
mutex_lock(&data->mutex);
/* Check, if the suspend occured while active */
- if (data->trigger_on)
+ if (data->trigger_on || data->ev_enable_state)
ret = kxcjk1013_set_mode(data, OPERATION);
mutex_unlock(&data->mutex);
--
1.7.11.7
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH 5/6] iio: accel: kxcjk-1013: Support thresholds
2014-07-17 0:42 ` [PATCH 5/6] iio: accel: kxcjk-1013: Support thresholds Srinivas Pandruvada
@ 2014-07-20 16:04 ` Jonathan Cameron
2014-07-20 16:34 ` Srinivas Pandruvada
0 siblings, 1 reply; 21+ messages in thread
From: Jonathan Cameron @ 2014-07-20 16:04 UTC (permalink / raw)
To: Srinivas Pandruvada; +Cc: linux-iio
On 17/07/14 01:42, Srinivas Pandruvada wrote:
> This chip can operate in two modes. In one mode it can issue periodic
> interrupts based on sample rate setting or when there is a motion.
But not both? This had me confused below.
Also, the event description is incorrect for conventional motion detection
(magnitude rising detection).
J
> Using events mechanism to allow configuration and receive events.
>
> Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> ---
> drivers/iio/accel/kxcjk-1013.c | 238 +++++++++++++++++++++++++++++++++++++++--
> 1 file changed, 230 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/iio/accel/kxcjk-1013.c b/drivers/iio/accel/kxcjk-1013.c
> index 975f8a6..f5bb682 100644
> --- a/drivers/iio/accel/kxcjk-1013.c
> +++ b/drivers/iio/accel/kxcjk-1013.c
> @@ -27,6 +27,7 @@
> #include <linux/iio/sysfs.h>
> #include <linux/iio/buffer.h>
> #include <linux/iio/trigger.h>
> +#include <linux/iio/events.h>
> #include <linux/iio/trigger_consumer.h>
> #include <linux/iio/triggered_buffer.h>
> #include <linux/iio/accel/kxcjk_1013.h>
> @@ -92,6 +93,9 @@ struct kxcjk1013_data {
> u8 odr_bits;
> u8 range;
> bool active_high_intr;
> + int ev_enable_state;
> + int wake_thres;
> + int wake_dur;
> bool trigger_on;
> };
>
> @@ -116,6 +120,23 @@ static const struct {
> {200, 0, 0x04}, {400, 0, 0x05}, {800, 0, 0x06},
> {1600, 0, 0x07} };
>
> +static const struct {
> + int val;
> + int val2;
> + int odr_bits;
> +} wake_odr_data_rate_table[] = { {0, 781000, 0x00},
> + {1, 563000, 0x01},
> + {3, 125000, 0x02},
> + {6, 250000, 0x03},
> + {12, 500000, 0x04},
> + {25, 0, 0x05},
> + {50, 0, 0x06},
> + {100, 0, 0x06},
> + {200, 0, 0x06},
> + {400, 0, 0x06},
> + {800, 0, 0x06},
> + {1600, 0, 0x06} };
> +
> /* Refer to section 4 of the specification */
> static const struct {
> int odr_bits;
> @@ -283,10 +304,36 @@ static int kxcjk1013_set_power_state(struct kxcjk1013_data *data, bool on)
> return 0;
> }
>
> +static int kxcjk1013_chip_update_thresholds(struct kxcjk1013_data *data)
> +{
> + int ret;
> +
> + ret = i2c_smbus_write_byte_data(data->client,
> + KXCJK1013_REG_WAKE_TIMER,
> + data->wake_dur);
> + if (ret < 0) {
> + dev_err(&data->client->dev,
> + "Error writing reg_wake_timer\n");
> + return ret;
> + }
> +
> + ret = i2c_smbus_write_byte_data(data->client,
> + KXCJK1013_REG_WAKE_THRES,
> + data->wake_thres);
> + if (ret < 0) {
> + dev_err(&data->client->dev,
> + "Error writing reg_wake_thres\n");
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> static int kxcjk1013_chip_setup_interrupt(struct kxcjk1013_data *data,
> bool status)
> {
> int ret;
> + u8 intr_bit;
> enum kxcjk1013_mode store_mode;
>
> ret = kxcjk1013_get_mode(data, &store_mode);
> @@ -316,6 +363,16 @@ static int kxcjk1013_chip_setup_interrupt(struct kxcjk1013_data *data,
> return ret;
> }
>
> + if (data->wake_thres) {
rather than ev_enable_state?
> + if (status) {
> + ret = kxcjk1013_chip_update_thresholds(data);
> + if (ret < 0)
> + return ret;
> + }
> + intr_bit = KXCJK1013_REG_CTRL1_BIT_WUFE;
> + } else
> + intr_bit = KXCJK1013_REG_CTRL1_BIT_DRDY;
> +
Does this mean the dataready interrupt is disabled whenever thresholds
are non zero? This needs some explanation.
> ret = i2c_smbus_read_byte_data(data->client, KXCJK1013_REG_CTRL1);
> if (ret < 0) {
> dev_err(&data->client->dev, "Error reading reg_ctrl1\n");
> @@ -323,9 +380,10 @@ static int kxcjk1013_chip_setup_interrupt(struct kxcjk1013_data *data,
> }
>
> if (status)
> - ret |= KXCJK1013_REG_CTRL1_BIT_DRDY;
> + ret |= intr_bit;
> else
> - ret &= ~KXCJK1013_REG_CTRL1_BIT_DRDY;
> + ret &= ~(KXCJK1013_REG_CTRL1_BIT_WUFE |
> + KXCJK1013_REG_CTRL1_BIT_DRDY);
>
> ret = i2c_smbus_write_byte_data(data->client,
> KXCJK1013_REG_CTRL1, ret);
> @@ -357,6 +415,20 @@ static int kxcjk1013_convert_freq_to_bit(int val, int val2)
> return -EINVAL;
> }
>
> +static int kxcjk1013_convert_wake_odr_to_bit(int val, int val2)
> +{
> + int i;
> +
> + for (i = 0; i < ARRAY_SIZE(wake_odr_data_rate_table); ++i) {
> + if (wake_odr_data_rate_table[i].val == val &&
> + wake_odr_data_rate_table[i].val2 == val2) {
> + return wake_odr_data_rate_table[i].odr_bits;
> + }
> + }
> +
> + return -EINVAL;
> +}
> +
> static int kxcjk1013_set_odr(struct kxcjk1013_data *data, int val, int val2)
> {
> int ret;
> @@ -385,6 +457,17 @@ static int kxcjk1013_set_odr(struct kxcjk1013_data *data, int val, int val2)
>
> data->odr_bits = odr_bits;
>
> + odr_bits = kxcjk1013_convert_wake_odr_to_bit(val, val2);
> + if (odr_bits < 0)
> + return odr_bits;
> +
> + ret = i2c_smbus_write_byte_data(data->client, KXCJK1013_REG_CTRL2,
> + odr_bits);
> + if (ret < 0) {
> + dev_err(&data->client->dev, "Error writing reg_ctrl2\n");
> + return ret;
> + }
> +
> if (store_mode == OPERATION) {
> ret = kxcjk1013_set_mode(data, OPERATION);
> if (ret < 0)
> @@ -567,6 +650,94 @@ static int kxcjk1013_write_raw(struct iio_dev *indio_dev,
> return ret;
> }
>
> +static int kxcjk1013_read_event(struct iio_dev *indio_dev,
> + const struct iio_chan_spec *chan,
> + enum iio_event_type type,
> + enum iio_event_direction dir,
> + enum iio_event_info info,
> + int *val, int *val2)
> +{
> + struct kxcjk1013_data *data = iio_priv(indio_dev);
> +
> + *val2 = 0;
> + switch (info) {
> + case IIO_EV_INFO_VALUE:
> + *val = data->wake_thres;
> + break;
> + case IIO_EV_INFO_PERIOD:
> + *val = data->wake_dur;
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + return IIO_VAL_INT;
> +}
> +
> +static int kxcjk1013_write_event(struct iio_dev *indio_dev,
> + const struct iio_chan_spec *chan,
> + enum iio_event_type type,
> + enum iio_event_direction dir,
> + enum iio_event_info info,
> + int val, int val2)
> +{
> + struct kxcjk1013_data *data = iio_priv(indio_dev);
> +
Perhaps check val2 == 0
> + switch (info) {
> + case IIO_EV_INFO_VALUE:
> + data->wake_thres = val;
> + break;
> + case IIO_EV_INFO_PERIOD:
> + data->wake_dur = val;
Double space above.
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> +static int kxcjk1013_read_event_config(struct iio_dev *indio_dev,
> + const struct iio_chan_spec *chan,
> + enum iio_event_type type,
> + enum iio_event_direction dir)
> +{
> +
> + struct kxcjk1013_data *data = iio_priv(indio_dev);
> +
> + return data->ev_enable_state;
> +}
> +
> +static int kxcjk1013_write_event_config(struct iio_dev *indio_dev,
> + const struct iio_chan_spec *chan,
> + enum iio_event_type type,
> + enum iio_event_direction dir,
> + int state)
> +{
> + struct kxcjk1013_data *data = iio_priv(indio_dev);
> + int ret;
> +
> + if (data->trigger_on)
> + return -EAGAIN;
> +
> + if (state && data->ev_enable_state)
> + return 0;
> +
> + mutex_lock(&data->mutex);
> + ret = kxcjk1013_chip_setup_interrupt(data, state);
> + if (!ret) {
> + ret = kxcjk1013_set_power_state(data, state);
> + if (ret < 0) {
> + mutex_unlock(&data->mutex);
> + return ret;
> + }
> + }
> + data->ev_enable_state = state;
> + mutex_unlock(&data->mutex);
> +
> + return 0;
> +}
> +
> static int kxcjk1013_validate_trigger(struct iio_dev *indio_dev,
> struct iio_trigger *trig)
> {
> @@ -593,6 +764,14 @@ static const struct attribute_group kxcjk1013_attrs_group = {
> .attrs = kxcjk1013_attributes,
> };
>
> +static const struct iio_event_spec kxcjk1013_event = {
> + .type = IIO_EV_TYPE_THRESH,
> + .dir = IIO_EV_DIR_EITHER,
As stated below I'm a little doubtful this is correct.
> + .mask_separate = BIT(IIO_EV_INFO_VALUE) |
> + BIT(IIO_EV_INFO_ENABLE) |
> + BIT(IIO_EV_INFO_PERIOD)
> +};
> +
> #define KXCJK1013_CHANNEL(_axis) { \
> .type = IIO_ACCEL, \
> .modified = 1, \
> @@ -608,6 +787,8 @@ static const struct attribute_group kxcjk1013_attrs_group = {
> .shift = 4, \
> .endianness = IIO_LE, \
> }, \
> + .event_spec = &kxcjk1013_event, \
> + .num_event_specs = 1 \
> }
>
> static const struct iio_chan_spec kxcjk1013_channels[] = {
> @@ -621,6 +802,10 @@ static const struct iio_info kxcjk1013_info = {
> .attrs = &kxcjk1013_attrs_group,
> .read_raw = kxcjk1013_read_raw,
> .write_raw = kxcjk1013_write_raw,
> + .read_event_value = kxcjk1013_read_event,
> + .write_event_value = kxcjk1013_write_event,
> + .write_event_config = kxcjk1013_write_event_config,
> + .read_event_config = kxcjk1013_read_event_config,
> .validate_trigger = kxcjk1013_validate_trigger,
> .driver_module = THIS_MODULE,
> };
> @@ -675,6 +860,9 @@ static int kxcjk1013_data_rdy_trigger_set_state(struct iio_trigger *trig,
> struct kxcjk1013_data *data = iio_priv(indio_dev);
> int ret;
>
> + if (data->ev_enable_state)
> + return -EAGAIN;
> +
> if (state && data->trigger_on)
> return 0;
>
> @@ -699,6 +887,38 @@ static const struct iio_trigger_ops kxcjk1013_trigger_ops = {
> .owner = THIS_MODULE,
> };
>
> +static irqreturn_t kxcjk1013_event_handler(int irq, void *private)
> +{
> + struct iio_dev *indio_dev = private;
> + struct kxcjk1013_data *data = iio_priv(indio_dev);
> + int ret;
> +
Slight preference for a comment here saying what the event is...
> + iio_push_event(indio_dev, IIO_MOD_EVENT_CODE(IIO_ACCEL,
> + 0,
> + IIO_MOD_X_OR_Y_OR_Z,
> + IIO_EV_TYPE_THRESH,
> + IIO_EV_DIR_EITHER),
Really? Note this means that a fall in the magnitude will trigger the event
as well as a rise? (e.g. stop of a previously existing motion).
It's possible but seems unlikely. I'm guessing you want IIO_EV_TYPE_MAG
and IIO_EV_DIR_RISING.
> + iio_get_time_ns());
> +
> + ret = i2c_smbus_read_byte_data(data->client, KXCJK1013_REG_INT_REL);
> + if (ret < 0)
> + dev_err(&data->client->dev, "Error reading reg_int_rel\n");
> +
> + return IRQ_HANDLED;
> +}
> +
> +static irqreturn_t kxcjk1013_data_rdy_trig_poll(int irq, void *private)
> +{
> + struct iio_dev *indio_dev = private;
> + struct kxcjk1013_data *data = iio_priv(indio_dev);
> +
> + if (data->trigger_on) {
> + iio_trigger_poll(data->trig);
> + return IRQ_HANDLED;
> + } else
> + return IRQ_WAKE_THREAD;
> +}
> +
> static int kxcjk1013_acpi_gpio_probe(struct i2c_client *client,
> struct kxcjk1013_data *data)
> {
> @@ -783,11 +1003,13 @@ static int kxcjk1013_probe(struct i2c_client *client,
>
> data->trig_mode = true;
>
> - ret = devm_request_irq(&client->dev, client->irq,
> - iio_trigger_generic_data_rdy_poll,
> - IRQF_TRIGGER_RISING,
> - KXCJK1013_IRQ_NAME,
> - trig);
> + ret = devm_request_threaded_irq(&client->dev, client->irq,
> + kxcjk1013_data_rdy_trig_poll,
> + kxcjk1013_event_handler,
> + IRQF_TRIGGER_RISING |
> + IRQF_ONESHOT,
> + KXCJK1013_IRQ_NAME,
> + indio_dev);
> if (ret) {
> dev_err(&client->dev, "unable to request IRQ\n");
> goto err_trigger_free;
> @@ -889,7 +1111,7 @@ static int kxcjk1013_resume(struct device *dev)
>
> mutex_lock(&data->mutex);
> /* Check, if the suspend occured while active */
> - if (data->trigger_on)
> + if (data->trigger_on || data->ev_enable_state)
> ret = kxcjk1013_set_mode(data, OPERATION);
> mutex_unlock(&data->mutex);
>
>
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH 5/6] iio: accel: kxcjk-1013: Support thresholds
2014-07-20 16:04 ` Jonathan Cameron
@ 2014-07-20 16:34 ` Srinivas Pandruvada
2014-07-20 17:01 ` Jonathan Cameron
0 siblings, 1 reply; 21+ messages in thread
From: Srinivas Pandruvada @ 2014-07-20 16:34 UTC (permalink / raw)
To: Jonathan Cameron; +Cc: linux-iio
On 07/20/2014 09:04 AM, Jonathan Cameron wrote:
> On 17/07/14 01:42, Srinivas Pandruvada wrote:
>> This chip can operate in two modes. In one mode it can issue periodic
>> interrupts based on sample rate setting or when there is a motion.
> But not both? This had me confused below.
>
The purpose of this change is so that CPU doesn't wake up to process
interrupt when there is no significant change, when user space application
can tolerate by using event thresholds. This tolerance depends on use case.
If you enable both, then this is not meaningful as data ready will generate
continuous interrupts anyway taking precedence.
> Also, the event description is incorrect for conventional motion
> detection
> (magnitude rising detection).
>
> J
>> Using events mechanism to allow configuration and receive events.
>>
>> Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
>> ---
>> drivers/iio/accel/kxcjk-1013.c | 238
>> +++++++++++++++++++++++++++++++++++++++--
>> 1 file changed, 230 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/iio/accel/kxcjk-1013.c
>> b/drivers/iio/accel/kxcjk-1013.c
>> index 975f8a6..f5bb682 100644
>> --- a/drivers/iio/accel/kxcjk-1013.c
>> +++ b/drivers/iio/accel/kxcjk-1013.c
>> @@ -27,6 +27,7 @@
>> #include <linux/iio/sysfs.h>
>> #include <linux/iio/buffer.h>
>> #include <linux/iio/trigger.h>
>> +#include <linux/iio/events.h>
>> #include <linux/iio/trigger_consumer.h>
>> #include <linux/iio/triggered_buffer.h>
>> #include <linux/iio/accel/kxcjk_1013.h>
>> @@ -92,6 +93,9 @@ struct kxcjk1013_data {
>> u8 odr_bits;
>> u8 range;
>> bool active_high_intr;
>> + int ev_enable_state;
>> + int wake_thres;
>> + int wake_dur;
>> bool trigger_on;
>> };
>>
>> @@ -116,6 +120,23 @@ static const struct {
>> {200, 0, 0x04}, {400, 0, 0x05}, {800, 0, 0x06},
>> {1600, 0, 0x07} };
>>
>> +static const struct {
>> + int val;
>> + int val2;
>> + int odr_bits;
>> +} wake_odr_data_rate_table[] = { {0, 781000, 0x00},
>> + {1, 563000, 0x01},
>> + {3, 125000, 0x02},
>> + {6, 250000, 0x03},
>> + {12, 500000, 0x04},
>> + {25, 0, 0x05},
>> + {50, 0, 0x06},
>> + {100, 0, 0x06},
>> + {200, 0, 0x06},
>> + {400, 0, 0x06},
>> + {800, 0, 0x06},
>> + {1600, 0, 0x06} };
>> +
>> /* Refer to section 4 of the specification */
>> static const struct {
>> int odr_bits;
>> @@ -283,10 +304,36 @@ static int kxcjk1013_set_power_state(struct
>> kxcjk1013_data *data, bool on)
>> return 0;
>> }
>>
>> +static int kxcjk1013_chip_update_thresholds(struct kxcjk1013_data
>> *data)
>> +{
>> + int ret;
>> +
>> + ret = i2c_smbus_write_byte_data(data->client,
>> + KXCJK1013_REG_WAKE_TIMER,
>> + data->wake_dur);
>> + if (ret < 0) {
>> + dev_err(&data->client->dev,
>> + "Error writing reg_wake_timer\n");
>> + return ret;
>> + }
>> +
>> + ret = i2c_smbus_write_byte_data(data->client,
>> + KXCJK1013_REG_WAKE_THRES,
>> + data->wake_thres);
>> + if (ret < 0) {
>> + dev_err(&data->client->dev,
>> + "Error writing reg_wake_thres\n");
>> + return ret;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> static int kxcjk1013_chip_setup_interrupt(struct kxcjk1013_data *data,
>> bool status)
>> {
>> int ret;
>> + u8 intr_bit;
>> enum kxcjk1013_mode store_mode;
>>
>> ret = kxcjk1013_get_mode(data, &store_mode);
>> @@ -316,6 +363,16 @@ static int kxcjk1013_chip_setup_interrupt(struct
>> kxcjk1013_data *data,
>> return ret;
>> }
>>
>> + if (data->wake_thres) {
> rather than ev_enable_state?
We don't want to depend on event enable when there is no threshold set,
events will have
no meaning.
>> + if (status) {
>> + ret = kxcjk1013_chip_update_thresholds(data);
>> + if (ret < 0)
>> + return ret;
>> + }
>> + intr_bit = KXCJK1013_REG_CTRL1_BIT_WUFE;
>> + } else
>> + intr_bit = KXCJK1013_REG_CTRL1_BIT_DRDY;
>> +
> Does this mean the dataready interrupt is disabled whenever thresholds
> are non zero? This needs some explanation.
>
Data ready will take precedence and will send periodic interrupts.
>> ret = i2c_smbus_read_byte_data(data->client, KXCJK1013_REG_CTRL1);
>> if (ret < 0) {
>> dev_err(&data->client->dev, "Error reading reg_ctrl1\n");
>> @@ -323,9 +380,10 @@ static int kxcjk1013_chip_setup_interrupt(struct
>> kxcjk1013_data *data,
>> }
>>
>> if (status)
>> - ret |= KXCJK1013_REG_CTRL1_BIT_DRDY;
>> + ret |= intr_bit;
>> else
>> - ret &= ~KXCJK1013_REG_CTRL1_BIT_DRDY;
>> + ret &= ~(KXCJK1013_REG_CTRL1_BIT_WUFE |
>> + KXCJK1013_REG_CTRL1_BIT_DRDY);
>>
>> ret = i2c_smbus_write_byte_data(data->client,
>> KXCJK1013_REG_CTRL1, ret);
>> @@ -357,6 +415,20 @@ static int kxcjk1013_convert_freq_to_bit(int
>> val, int val2)
>> return -EINVAL;
>> }
>>
>> +static int kxcjk1013_convert_wake_odr_to_bit(int val, int val2)
>> +{
>> + int i;
>> +
>> + for (i = 0; i < ARRAY_SIZE(wake_odr_data_rate_table); ++i) {
>> + if (wake_odr_data_rate_table[i].val == val &&
>> + wake_odr_data_rate_table[i].val2 == val2) {
>> + return wake_odr_data_rate_table[i].odr_bits;
>> + }
>> + }
>> +
>> + return -EINVAL;
>> +}
>> +
>> static int kxcjk1013_set_odr(struct kxcjk1013_data *data, int val,
>> int val2)
>> {
>> int ret;
>> @@ -385,6 +457,17 @@ static int kxcjk1013_set_odr(struct
>> kxcjk1013_data *data, int val, int val2)
>>
>> data->odr_bits = odr_bits;
>>
>> + odr_bits = kxcjk1013_convert_wake_odr_to_bit(val, val2);
>> + if (odr_bits < 0)
>> + return odr_bits;
>> +
>> + ret = i2c_smbus_write_byte_data(data->client, KXCJK1013_REG_CTRL2,
>> + odr_bits);
>> + if (ret < 0) {
>> + dev_err(&data->client->dev, "Error writing reg_ctrl2\n");
>> + return ret;
>> + }
>> +
>> if (store_mode == OPERATION) {
>> ret = kxcjk1013_set_mode(data, OPERATION);
>> if (ret < 0)
>> @@ -567,6 +650,94 @@ static int kxcjk1013_write_raw(struct iio_dev
>> *indio_dev,
>> return ret;
>> }
>>
>> +static int kxcjk1013_read_event(struct iio_dev *indio_dev,
>> + const struct iio_chan_spec *chan,
>> + enum iio_event_type type,
>> + enum iio_event_direction dir,
>> + enum iio_event_info info,
>> + int *val, int *val2)
>> +{
>> + struct kxcjk1013_data *data = iio_priv(indio_dev);
>> +
>> + *val2 = 0;
>> + switch (info) {
>> + case IIO_EV_INFO_VALUE:
>> + *val = data->wake_thres;
>> + break;
>> + case IIO_EV_INFO_PERIOD:
>> + *val = data->wake_dur;
>> + break;
>> + default:
>> + return -EINVAL;
>> + }
>> +
>> + return IIO_VAL_INT;
>> +}
>> +
>> +static int kxcjk1013_write_event(struct iio_dev *indio_dev,
>> + const struct iio_chan_spec *chan,
>> + enum iio_event_type type,
>> + enum iio_event_direction dir,
>> + enum iio_event_info info,
>> + int val, int val2)
>> +{
>> + struct kxcjk1013_data *data = iio_priv(indio_dev);
>> +
> Perhaps check val2 == 0
OK
>> + switch (info) {
>> + case IIO_EV_INFO_VALUE:
>> + data->wake_thres = val;
>> + break;
>> + case IIO_EV_INFO_PERIOD:
>> + data->wake_dur = val;
> Double space above.
>> + break;
>> + default:
>> + return -EINVAL;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int kxcjk1013_read_event_config(struct iio_dev *indio_dev,
>> + const struct iio_chan_spec *chan,
>> + enum iio_event_type type,
>> + enum iio_event_direction dir)
>> +{
>> +
>> + struct kxcjk1013_data *data = iio_priv(indio_dev);
>> +
>> + return data->ev_enable_state;
>> +}
>> +
>> +static int kxcjk1013_write_event_config(struct iio_dev *indio_dev,
>> + const struct iio_chan_spec *chan,
>> + enum iio_event_type type,
>> + enum iio_event_direction dir,
>> + int state)
>> +{
>> + struct kxcjk1013_data *data = iio_priv(indio_dev);
>> + int ret;
>> +
>> + if (data->trigger_on)
>> + return -EAGAIN;
>> +
>> + if (state && data->ev_enable_state)
>> + return 0;
>> +
>> + mutex_lock(&data->mutex);
>> + ret = kxcjk1013_chip_setup_interrupt(data, state);
>> + if (!ret) {
>> + ret = kxcjk1013_set_power_state(data, state);
>> + if (ret < 0) {
>> + mutex_unlock(&data->mutex);
>> + return ret;
>> + }
>> + }
>> + data->ev_enable_state = state;
>> + mutex_unlock(&data->mutex);
>> +
>> + return 0;
>> +}
>> +
>> static int kxcjk1013_validate_trigger(struct iio_dev *indio_dev,
>> struct iio_trigger *trig)
>> {
>> @@ -593,6 +764,14 @@ static const struct attribute_group
>> kxcjk1013_attrs_group = {
>> .attrs = kxcjk1013_attributes,
>> };
>>
>> +static const struct iio_event_spec kxcjk1013_event = {
>> + .type = IIO_EV_TYPE_THRESH,
>> + .dir = IIO_EV_DIR_EITHER,
> As stated below I'm a little doubtful this is correct.
>> + .mask_separate = BIT(IIO_EV_INFO_VALUE) |
>> + BIT(IIO_EV_INFO_ENABLE) |
>> + BIT(IIO_EV_INFO_PERIOD)
>> +};
>> +
>> #define KXCJK1013_CHANNEL(_axis) { \
>> .type = IIO_ACCEL, \
>> .modified = 1, \
>> @@ -608,6 +787,8 @@ static const struct attribute_group
>> kxcjk1013_attrs_group = {
>> .shift = 4, \
>> .endianness = IIO_LE, \
>> }, \
>> + .event_spec = &kxcjk1013_event, \
>> + .num_event_specs = 1 \
>> }
>>
>> static const struct iio_chan_spec kxcjk1013_channels[] = {
>> @@ -621,6 +802,10 @@ static const struct iio_info kxcjk1013_info = {
>> .attrs = &kxcjk1013_attrs_group,
>> .read_raw = kxcjk1013_read_raw,
>> .write_raw = kxcjk1013_write_raw,
>> + .read_event_value = kxcjk1013_read_event,
>> + .write_event_value = kxcjk1013_write_event,
>> + .write_event_config = kxcjk1013_write_event_config,
>> + .read_event_config = kxcjk1013_read_event_config,
>> .validate_trigger = kxcjk1013_validate_trigger,
>> .driver_module = THIS_MODULE,
>> };
>> @@ -675,6 +860,9 @@ static int
>> kxcjk1013_data_rdy_trigger_set_state(struct iio_trigger *trig,
>> struct kxcjk1013_data *data = iio_priv(indio_dev);
>> int ret;
>>
>> + if (data->ev_enable_state)
>> + return -EAGAIN;
>> +
>> if (state && data->trigger_on)
>> return 0;
>>
>> @@ -699,6 +887,38 @@ static const struct iio_trigger_ops
>> kxcjk1013_trigger_ops = {
>> .owner = THIS_MODULE,
>> };
>>
>> +static irqreturn_t kxcjk1013_event_handler(int irq, void *private)
>> +{
>> + struct iio_dev *indio_dev = private;
>> + struct kxcjk1013_data *data = iio_priv(indio_dev);
>> + int ret;
>> +
> Slight preference for a comment here saying what the event is...
>> + iio_push_event(indio_dev, IIO_MOD_EVENT_CODE(IIO_ACCEL,
>> + 0,
>> + IIO_MOD_X_OR_Y_OR_Z,
>> + IIO_EV_TYPE_THRESH,
>> + IIO_EV_DIR_EITHER),
> Really? Note this means that a fall in the magnitude will trigger the
> event
> as well as a rise? (e.g. stop of a previously existing motion).
>
> It's possible but seems unlikely. I'm guessing you want IIO_EV_TYPE_MAG
> and IIO_EV_DIR_RISING.
Makes sense. I interpret as x,y, z, change in any direction will result
in event.
I will correct this.
>> + iio_get_time_ns());
>> +
>> + ret = i2c_smbus_read_byte_data(data->client,
>> KXCJK1013_REG_INT_REL);
>> + if (ret < 0)
>> + dev_err(&data->client->dev, "Error reading reg_int_rel\n");
>> +
>> + return IRQ_HANDLED;
>> +}
>> +
>> +static irqreturn_t kxcjk1013_data_rdy_trig_poll(int irq, void *private)
>> +{
>> + struct iio_dev *indio_dev = private;
>> + struct kxcjk1013_data *data = iio_priv(indio_dev);
>> +
>> + if (data->trigger_on) {
>> + iio_trigger_poll(data->trig);
>> + return IRQ_HANDLED;
>> + } else
>> + return IRQ_WAKE_THREAD;
>> +}
>> +
>> static int kxcjk1013_acpi_gpio_probe(struct i2c_client *client,
>> struct kxcjk1013_data *data)
>> {
>> @@ -783,11 +1003,13 @@ static int kxcjk1013_probe(struct i2c_client
>> *client,
>>
>> data->trig_mode = true;
>>
>> - ret = devm_request_irq(&client->dev, client->irq,
>> - iio_trigger_generic_data_rdy_poll,
>> - IRQF_TRIGGER_RISING,
>> - KXCJK1013_IRQ_NAME,
>> - trig);
>> + ret = devm_request_threaded_irq(&client->dev, client->irq,
>> + kxcjk1013_data_rdy_trig_poll,
>> + kxcjk1013_event_handler,
>> + IRQF_TRIGGER_RISING |
>> + IRQF_ONESHOT,
>> + KXCJK1013_IRQ_NAME,
>> + indio_dev);
>> if (ret) {
>> dev_err(&client->dev, "unable to request IRQ\n");
>> goto err_trigger_free;
>> @@ -889,7 +1111,7 @@ static int kxcjk1013_resume(struct device *dev)
>>
>> mutex_lock(&data->mutex);
>> /* Check, if the suspend occured while active */
>> - if (data->trigger_on)
>> + if (data->trigger_on || data->ev_enable_state)
>> ret = kxcjk1013_set_mode(data, OPERATION);
>> mutex_unlock(&data->mutex);
>>
>>
>
>
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH 5/6] iio: accel: kxcjk-1013: Support thresholds
2014-07-20 16:34 ` Srinivas Pandruvada
@ 2014-07-20 17:01 ` Jonathan Cameron
0 siblings, 0 replies; 21+ messages in thread
From: Jonathan Cameron @ 2014-07-20 17:01 UTC (permalink / raw)
To: Srinivas Pandruvada; +Cc: linux-iio
On 20/07/14 17:34, Srinivas Pandruvada wrote:
>
> On 07/20/2014 09:04 AM, Jonathan Cameron wrote:
>> On 17/07/14 01:42, Srinivas Pandruvada wrote:
>>> This chip can operate in two modes. In one mode it can issue periodic
>>> interrupts based on sample rate setting or when there is a motion.
>> But not both? This had me confused below.
>>
> The purpose of this change is so that CPU doesn't wake up to process
> interrupt when there is no significant change, when user space application
> can tolerate by using event thresholds. This tolerance depends on use case.
>
> If you enable both, then this is not meaningful as data ready will generate
> continuous interrupts anyway taking precedence.
Fair enough I guess. As long as it outputs the events when an event has occurred
I guess having the additional interrupt enabled makes not difference.
I'd be tempted to enable it anyway just to keep things simple.
If you have triggers based on motion detection, I would prefer that this
is explicitly indicated to userspace. Perhaps export a second trigger to indicate
this is happening? Could have it a sysfs attribute of the trigger I guess, but that
is a bit ugly.
If dataready trigger is enabled, I would expect that to take precedence from the
point of view of interrupts. You can also check for events on that interrupt if
enabled.
If the events alone are enabled, then no triggering occurs.
If the motion detection trigger is enabled then you trigger on motion detection events
whether or not the event is enabled (to be provided to userspace).
Finally if motion detection trigger is in use and the event is in use, the interrupt
handler will trigger capture and send an event.
I think this makes it more apparent what is going on.
Motion detection trigger would obviously have to have a good name. Arguably
you'd also have to repeat the threshold control in the trigger directory as it
would then be non obvious that the one in the events directory was effecting the
trigger. Each of these sysfs attributes would change what was read from both
of them (which is fine).
Jonathan
>
>> Also, the event description is incorrect for conventional motion detection
>> (magnitude rising detection).
>>
>> J
>>> Using events mechanism to allow configuration and receive events.
>>>
>>> Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
>>> ---
>>> drivers/iio/accel/kxcjk-1013.c | 238 +++++++++++++++++++++++++++++++++++++++--
>>> 1 file changed, 230 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/drivers/iio/accel/kxcjk-1013.c b/drivers/iio/accel/kxcjk-1013.c
>>> index 975f8a6..f5bb682 100644
>>> --- a/drivers/iio/accel/kxcjk-1013.c
>>> +++ b/drivers/iio/accel/kxcjk-1013.c
>>> @@ -27,6 +27,7 @@
>>> #include <linux/iio/sysfs.h>
>>> #include <linux/iio/buffer.h>
>>> #include <linux/iio/trigger.h>
>>> +#include <linux/iio/events.h>
>>> #include <linux/iio/trigger_consumer.h>
>>> #include <linux/iio/triggered_buffer.h>
>>> #include <linux/iio/accel/kxcjk_1013.h>
>>> @@ -92,6 +93,9 @@ struct kxcjk1013_data {
>>> u8 odr_bits;
>>> u8 range;
>>> bool active_high_intr;
>>> + int ev_enable_state;
>>> + int wake_thres;
>>> + int wake_dur;
>>> bool trigger_on;
>>> };
>>>
>>> @@ -116,6 +120,23 @@ static const struct {
>>> {200, 0, 0x04}, {400, 0, 0x05}, {800, 0, 0x06},
>>> {1600, 0, 0x07} };
>>>
>>> +static const struct {
>>> + int val;
>>> + int val2;
>>> + int odr_bits;
>>> +} wake_odr_data_rate_table[] = { {0, 781000, 0x00},
>>> + {1, 563000, 0x01},
>>> + {3, 125000, 0x02},
>>> + {6, 250000, 0x03},
>>> + {12, 500000, 0x04},
>>> + {25, 0, 0x05},
>>> + {50, 0, 0x06},
>>> + {100, 0, 0x06},
>>> + {200, 0, 0x06},
>>> + {400, 0, 0x06},
>>> + {800, 0, 0x06},
>>> + {1600, 0, 0x06} };
>>> +
>>> /* Refer to section 4 of the specification */
>>> static const struct {
>>> int odr_bits;
>>> @@ -283,10 +304,36 @@ static int kxcjk1013_set_power_state(struct kxcjk1013_data *data, bool on)
>>> return 0;
>>> }
>>>
>>> +static int kxcjk1013_chip_update_thresholds(struct kxcjk1013_data *data)
>>> +{
>>> + int ret;
>>> +
>>> + ret = i2c_smbus_write_byte_data(data->client,
>>> + KXCJK1013_REG_WAKE_TIMER,
>>> + data->wake_dur);
>>> + if (ret < 0) {
>>> + dev_err(&data->client->dev,
>>> + "Error writing reg_wake_timer\n");
>>> + return ret;
>>> + }
>>> +
>>> + ret = i2c_smbus_write_byte_data(data->client,
>>> + KXCJK1013_REG_WAKE_THRES,
>>> + data->wake_thres);
>>> + if (ret < 0) {
>>> + dev_err(&data->client->dev,
>>> + "Error writing reg_wake_thres\n");
>>> + return ret;
>>> + }
>>> +
>>> + return 0;
>>> +}
>>> +
>>> static int kxcjk1013_chip_setup_interrupt(struct kxcjk1013_data *data,
>>> bool status)
>>> {
>>> int ret;
>>> + u8 intr_bit;
>>> enum kxcjk1013_mode store_mode;
>>>
>>> ret = kxcjk1013_get_mode(data, &store_mode);
>>> @@ -316,6 +363,16 @@ static int kxcjk1013_chip_setup_interrupt(struct kxcjk1013_data *data,
>>> return ret;
>>> }
>>>
>>> + if (data->wake_thres) {
>> rather than ev_enable_state?
> We don't want to depend on event enable when there is no threshold set, events will have
> no meaning.
I'd rather you checked them both and refused to turn on if not sensible. That way
the fallback to dataready doesn't hide the issue.
>>> + if (status) {
>>> + ret = kxcjk1013_chip_update_thresholds(data);
>>> + if (ret < 0)
>>> + return ret;
>>> + }
>>> + intr_bit = KXCJK1013_REG_CTRL1_BIT_WUFE;
>>> + } else
>>> + intr_bit = KXCJK1013_REG_CTRL1_BIT_DRDY;
>>> +
>> Does this mean the dataready interrupt is disabled whenever thresholds
>> are non zero? This needs some explanation.
>>
> Data ready will take precedence and will send periodic interrupts.
>>> ret = i2c_smbus_read_byte_data(data->client, KXCJK1013_REG_CTRL1);
>>> if (ret < 0) {
>>> dev_err(&data->client->dev, "Error reading reg_ctrl1\n");
>>> @@ -323,9 +380,10 @@ static int kxcjk1013_chip_setup_interrupt(struct kxcjk1013_data *data,
>>> }
>>>
>>> if (status)
>>> - ret |= KXCJK1013_REG_CTRL1_BIT_DRDY;
>>> + ret |= intr_bit;
>>> else
>>> - ret &= ~KXCJK1013_REG_CTRL1_BIT_DRDY;
>>> + ret &= ~(KXCJK1013_REG_CTRL1_BIT_WUFE |
>>> + KXCJK1013_REG_CTRL1_BIT_DRDY);
>>>
>>> ret = i2c_smbus_write_byte_data(data->client,
>>> KXCJK1013_REG_CTRL1, ret);
>>> @@ -357,6 +415,20 @@ static int kxcjk1013_convert_freq_to_bit(int val, int val2)
>>> return -EINVAL;
>>> }
>>>
>>> +static int kxcjk1013_convert_wake_odr_to_bit(int val, int val2)
>>> +{
>>> + int i;
>>> +
>>> + for (i = 0; i < ARRAY_SIZE(wake_odr_data_rate_table); ++i) {
>>> + if (wake_odr_data_rate_table[i].val == val &&
>>> + wake_odr_data_rate_table[i].val2 == val2) {
>>> + return wake_odr_data_rate_table[i].odr_bits;
>>> + }
>>> + }
>>> +
>>> + return -EINVAL;
>>> +}
>>> +
>>> static int kxcjk1013_set_odr(struct kxcjk1013_data *data, int val, int val2)
>>> {
>>> int ret;
>>> @@ -385,6 +457,17 @@ static int kxcjk1013_set_odr(struct kxcjk1013_data *data, int val, int val2)
>>>
>>> data->odr_bits = odr_bits;
>>>
>>> + odr_bits = kxcjk1013_convert_wake_odr_to_bit(val, val2);
>>> + if (odr_bits < 0)
>>> + return odr_bits;
>>> +
>>> + ret = i2c_smbus_write_byte_data(data->client, KXCJK1013_REG_CTRL2,
>>> + odr_bits);
>>> + if (ret < 0) {
>>> + dev_err(&data->client->dev, "Error writing reg_ctrl2\n");
>>> + return ret;
>>> + }
>>> +
>>> if (store_mode == OPERATION) {
>>> ret = kxcjk1013_set_mode(data, OPERATION);
>>> if (ret < 0)
>>> @@ -567,6 +650,94 @@ static int kxcjk1013_write_raw(struct iio_dev *indio_dev,
>>> return ret;
>>> }
>>>
>>> +static int kxcjk1013_read_event(struct iio_dev *indio_dev,
>>> + const struct iio_chan_spec *chan,
>>> + enum iio_event_type type,
>>> + enum iio_event_direction dir,
>>> + enum iio_event_info info,
>>> + int *val, int *val2)
>>> +{
>>> + struct kxcjk1013_data *data = iio_priv(indio_dev);
>>> +
>>> + *val2 = 0;
>>> + switch (info) {
>>> + case IIO_EV_INFO_VALUE:
>>> + *val = data->wake_thres;
>>> + break;
>>> + case IIO_EV_INFO_PERIOD:
>>> + *val = data->wake_dur;
>>> + break;
>>> + default:
>>> + return -EINVAL;
>>> + }
>>> +
>>> + return IIO_VAL_INT;
>>> +}
>>> +
>>> +static int kxcjk1013_write_event(struct iio_dev *indio_dev,
>>> + const struct iio_chan_spec *chan,
>>> + enum iio_event_type type,
>>> + enum iio_event_direction dir,
>>> + enum iio_event_info info,
>>> + int val, int val2)
>>> +{
>>> + struct kxcjk1013_data *data = iio_priv(indio_dev);
>>> +
>> Perhaps check val2 == 0
> OK
>>> + switch (info) {
>>> + case IIO_EV_INFO_VALUE:
>>> + data->wake_thres = val;
>>> + break;
>>> + case IIO_EV_INFO_PERIOD:
>>> + data->wake_dur = val;
>> Double space above.
>>> + break;
>>> + default:
>>> + return -EINVAL;
>>> + }
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int kxcjk1013_read_event_config(struct iio_dev *indio_dev,
>>> + const struct iio_chan_spec *chan,
>>> + enum iio_event_type type,
>>> + enum iio_event_direction dir)
>>> +{
>>> +
>>> + struct kxcjk1013_data *data = iio_priv(indio_dev);
>>> +
>>> + return data->ev_enable_state;
>>> +}
>>> +
>>> +static int kxcjk1013_write_event_config(struct iio_dev *indio_dev,
>>> + const struct iio_chan_spec *chan,
>>> + enum iio_event_type type,
>>> + enum iio_event_direction dir,
>>> + int state)
>>> +{
>>> + struct kxcjk1013_data *data = iio_priv(indio_dev);
>>> + int ret;
>>> +
>>> + if (data->trigger_on)
>>> + return -EAGAIN;
>>> +
>>> + if (state && data->ev_enable_state)
>>> + return 0;
>>> +
>>> + mutex_lock(&data->mutex);
>>> + ret = kxcjk1013_chip_setup_interrupt(data, state);
>>> + if (!ret) {
>>> + ret = kxcjk1013_set_power_state(data, state);
>>> + if (ret < 0) {
>>> + mutex_unlock(&data->mutex);
>>> + return ret;
>>> + }
>>> + }
>>> + data->ev_enable_state = state;
>>> + mutex_unlock(&data->mutex);
>>> +
>>> + return 0;
>>> +}
>>> +
>>> static int kxcjk1013_validate_trigger(struct iio_dev *indio_dev,
>>> struct iio_trigger *trig)
>>> {
>>> @@ -593,6 +764,14 @@ static const struct attribute_group kxcjk1013_attrs_group = {
>>> .attrs = kxcjk1013_attributes,
>>> };
>>>
>>> +static const struct iio_event_spec kxcjk1013_event = {
>>> + .type = IIO_EV_TYPE_THRESH,
>>> + .dir = IIO_EV_DIR_EITHER,
>> As stated below I'm a little doubtful this is correct.
>>> + .mask_separate = BIT(IIO_EV_INFO_VALUE) |
>>> + BIT(IIO_EV_INFO_ENABLE) |
>>> + BIT(IIO_EV_INFO_PERIOD)
>>> +};
>>> +
>>> #define KXCJK1013_CHANNEL(_axis) { \
>>> .type = IIO_ACCEL, \
>>> .modified = 1, \
>>> @@ -608,6 +787,8 @@ static const struct attribute_group kxcjk1013_attrs_group = {
>>> .shift = 4, \
>>> .endianness = IIO_LE, \
>>> }, \
>>> + .event_spec = &kxcjk1013_event, \
>>> + .num_event_specs = 1 \
>>> }
>>>
>>> static const struct iio_chan_spec kxcjk1013_channels[] = {
>>> @@ -621,6 +802,10 @@ static const struct iio_info kxcjk1013_info = {
>>> .attrs = &kxcjk1013_attrs_group,
>>> .read_raw = kxcjk1013_read_raw,
>>> .write_raw = kxcjk1013_write_raw,
>>> + .read_event_value = kxcjk1013_read_event,
>>> + .write_event_value = kxcjk1013_write_event,
>>> + .write_event_config = kxcjk1013_write_event_config,
>>> + .read_event_config = kxcjk1013_read_event_config,
>>> .validate_trigger = kxcjk1013_validate_trigger,
>>> .driver_module = THIS_MODULE,
>>> };
>>> @@ -675,6 +860,9 @@ static int kxcjk1013_data_rdy_trigger_set_state(struct iio_trigger *trig,
>>> struct kxcjk1013_data *data = iio_priv(indio_dev);
>>> int ret;
>>>
>>> + if (data->ev_enable_state)
>>> + return -EAGAIN;
>>> +
>>> if (state && data->trigger_on)
>>> return 0;
>>>
>>> @@ -699,6 +887,38 @@ static const struct iio_trigger_ops kxcjk1013_trigger_ops = {
>>> .owner = THIS_MODULE,
>>> };
>>>
>>> +static irqreturn_t kxcjk1013_event_handler(int irq, void *private)
>>> +{
>>> + struct iio_dev *indio_dev = private;
>>> + struct kxcjk1013_data *data = iio_priv(indio_dev);
>>> + int ret;
>>> +
>> Slight preference for a comment here saying what the event is...
>>> + iio_push_event(indio_dev, IIO_MOD_EVENT_CODE(IIO_ACCEL,
>>> + 0,
>>> + IIO_MOD_X_OR_Y_OR_Z,
>>> + IIO_EV_TYPE_THRESH,
>>> + IIO_EV_DIR_EITHER),
>> Really? Note this means that a fall in the magnitude will trigger the event
>> as well as a rise? (e.g. stop of a previously existing motion).
>>
>> It's possible but seems unlikely. I'm guessing you want IIO_EV_TYPE_MAG
>> and IIO_EV_DIR_RISING.
> Makes sense. I interpret as x,y, z, change in any direction will result in event.
> I will correct this.
>>> + iio_get_time_ns());
>>> +
>>> + ret = i2c_smbus_read_byte_data(data->client, KXCJK1013_REG_INT_REL);
>>> + if (ret < 0)
>>> + dev_err(&data->client->dev, "Error reading reg_int_rel\n");
>>> +
>>> + return IRQ_HANDLED;
>>> +}
>>> +
>>> +static irqreturn_t kxcjk1013_data_rdy_trig_poll(int irq, void *private)
>>> +{
>>> + struct iio_dev *indio_dev = private;
>>> + struct kxcjk1013_data *data = iio_priv(indio_dev);
>>> +
>>> + if (data->trigger_on) {
>>> + iio_trigger_poll(data->trig);
>>> + return IRQ_HANDLED;
>>> + } else
>>> + return IRQ_WAKE_THREAD;
>>> +}
>>> +
>>> static int kxcjk1013_acpi_gpio_probe(struct i2c_client *client,
>>> struct kxcjk1013_data *data)
>>> {
>>> @@ -783,11 +1003,13 @@ static int kxcjk1013_probe(struct i2c_client *client,
>>>
>>> data->trig_mode = true;
>>>
>>> - ret = devm_request_irq(&client->dev, client->irq,
>>> - iio_trigger_generic_data_rdy_poll,
>>> - IRQF_TRIGGER_RISING,
>>> - KXCJK1013_IRQ_NAME,
>>> - trig);
>>> + ret = devm_request_threaded_irq(&client->dev, client->irq,
>>> + kxcjk1013_data_rdy_trig_poll,
>>> + kxcjk1013_event_handler,
>>> + IRQF_TRIGGER_RISING |
>>> + IRQF_ONESHOT,
>>> + KXCJK1013_IRQ_NAME,
>>> + indio_dev);
>>> if (ret) {
>>> dev_err(&client->dev, "unable to request IRQ\n");
>>> goto err_trigger_free;
>>> @@ -889,7 +1111,7 @@ static int kxcjk1013_resume(struct device *dev)
>>>
>>> mutex_lock(&data->mutex);
>>> /* Check, if the suspend occured while active */
>>> - if (data->trigger_on)
>>> + if (data->trigger_on || data->ev_enable_state)
>>> ret = kxcjk1013_set_mode(data, OPERATION);
>>> mutex_unlock(&data->mutex);
>>>
>>>
>>
>>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 6/6] iio: accel: kxcjk-1013: Increment ref counter for indio_dev->trig
2014-07-17 0:42 [PATCH 1/6] iio: accel: kxcjk-1013: Fix setting frequency Srinivas Pandruvada
` (3 preceding siblings ...)
2014-07-17 0:42 ` [PATCH 5/6] iio: accel: kxcjk-1013: Support thresholds Srinivas Pandruvada
@ 2014-07-17 0:42 ` Srinivas Pandruvada
2014-07-20 11:29 ` Lars-Peter Clausen
2014-07-20 15:39 ` Jonathan Cameron
2014-07-20 15:19 ` [PATCH 1/6] iio: accel: kxcjk-1013: Fix setting frequency Jonathan Cameron
5 siblings, 2 replies; 21+ messages in thread
From: Srinivas Pandruvada @ 2014-07-17 0:42 UTC (permalink / raw)
To: jic23; +Cc: linux-iio, Srinivas Pandruvada
Assigning indio_dev->trig is not a good idea, as this can result in
wrong reference count for trigger device. If assigned, it is better to
increment reference counter by calling iio_trigger_get.
Refer to http://www.spinics.net/lists/linux-iio/msg13669.html for discussion
with Jonathan.
Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
---
drivers/iio/accel/kxcjk-1013.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/iio/accel/kxcjk-1013.c b/drivers/iio/accel/kxcjk-1013.c
index f5bb682..40ecf8b 100644
--- a/drivers/iio/accel/kxcjk-1013.c
+++ b/drivers/iio/accel/kxcjk-1013.c
@@ -1020,6 +1020,7 @@ static int kxcjk1013_probe(struct i2c_client *client,
iio_trigger_set_drvdata(trig, indio_dev);
data->trig = trig;
indio_dev->trig = trig;
+ iio_trigger_get(indio_dev->trig);
ret = iio_trigger_register(trig);
if (ret)
--
1.7.11.7
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH 6/6] iio: accel: kxcjk-1013: Increment ref counter for indio_dev->trig
2014-07-17 0:42 ` [PATCH 6/6] iio: accel: kxcjk-1013: Increment ref counter for indio_dev->trig Srinivas Pandruvada
@ 2014-07-20 11:29 ` Lars-Peter Clausen
2014-07-20 12:24 ` Jonathan Cameron
2014-07-20 15:39 ` Jonathan Cameron
1 sibling, 1 reply; 21+ messages in thread
From: Lars-Peter Clausen @ 2014-07-20 11:29 UTC (permalink / raw)
To: Srinivas Pandruvada, jic23; +Cc: linux-iio
On 07/17/2014 02:42 AM, Srinivas Pandruvada wrote:
> Assigning indio_dev->trig is not a good idea, as this can result in
> wrong reference count for trigger device. If assigned, it is better to
> increment reference counter by calling iio_trigger_get.
> Refer to http://www.spinics.net/lists/linux-iio/msg13669.html for discussion
> with Jonathan.
>
> Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> ---
> drivers/iio/accel/kxcjk-1013.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/iio/accel/kxcjk-1013.c b/drivers/iio/accel/kxcjk-1013.c
> index f5bb682..40ecf8b 100644
> --- a/drivers/iio/accel/kxcjk-1013.c
> +++ b/drivers/iio/accel/kxcjk-1013.c
> @@ -1020,6 +1020,7 @@ static int kxcjk1013_probe(struct i2c_client *client,
> iio_trigger_set_drvdata(trig, indio_dev);
> data->trig = trig;
> indio_dev->trig = trig;
> + iio_trigger_get(indio_dev->trig);
Hm, we should change the signature of iio_trigger_get() to return the
trigger, this gets it more in sync with other ..._get() APIs in the kernel.
Then the above two lines become indio_dev->trig = iio_trigger_get(trig).
- Lars
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 6/6] iio: accel: kxcjk-1013: Increment ref counter for indio_dev->trig
2014-07-20 11:29 ` Lars-Peter Clausen
@ 2014-07-20 12:24 ` Jonathan Cameron
0 siblings, 0 replies; 21+ messages in thread
From: Jonathan Cameron @ 2014-07-20 12:24 UTC (permalink / raw)
To: Lars-Peter Clausen, Srinivas Pandruvada; +Cc: linux-iio
On 20/07/14 12:29, Lars-Peter Clausen wrote:
> On 07/17/2014 02:42 AM, Srinivas Pandruvada wrote:
>> Assigning indio_dev->trig is not a good idea, as this can result in
>> wrong reference count for trigger device. If assigned, it is better to
>> increment reference counter by calling iio_trigger_get.
>> Refer to http://www.spinics.net/lists/linux-iio/msg13669.html for discussion
>> with Jonathan.
>>
>> Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
>> ---
>> drivers/iio/accel/kxcjk-1013.c | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/iio/accel/kxcjk-1013.c b/drivers/iio/accel/kxcjk-1013.c
>> index f5bb682..40ecf8b 100644
>> --- a/drivers/iio/accel/kxcjk-1013.c
>> +++ b/drivers/iio/accel/kxcjk-1013.c
>> @@ -1020,6 +1020,7 @@ static int kxcjk1013_probe(struct i2c_client *client,
>> iio_trigger_set_drvdata(trig, indio_dev);
>> data->trig = trig;
>> indio_dev->trig = trig;
>> + iio_trigger_get(indio_dev->trig);
>
> Hm, we should change the signature of iio_trigger_get() to return the
> trigger, this gets it more in sync with other ..._get() APIs in the
> kernel. Then the above two lines become indio_dev->trig =
> iio_trigger_get(trig).
Would make sense, but can occur after these fixes (more invasive) and these
ought to go in as fixes and stable etc.
J
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 6/6] iio: accel: kxcjk-1013: Increment ref counter for indio_dev->trig
2014-07-17 0:42 ` [PATCH 6/6] iio: accel: kxcjk-1013: Increment ref counter for indio_dev->trig Srinivas Pandruvada
2014-07-20 11:29 ` Lars-Peter Clausen
@ 2014-07-20 15:39 ` Jonathan Cameron
1 sibling, 0 replies; 21+ messages in thread
From: Jonathan Cameron @ 2014-07-20 15:39 UTC (permalink / raw)
To: Srinivas Pandruvada; +Cc: linux-iio, Lars-Peter Clausen
On 17/07/14 01:42, Srinivas Pandruvada wrote:
> Assigning indio_dev->trig is not a good idea, as this can result in
> wrong reference count for trigger device. If assigned, it is better to
> increment reference counter by calling iio_trigger_get.
> Refer to http://www.spinics.net/lists/linux-iio/msg13669.html for discussion
> with Jonathan.
>
> Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
I've parked the intervening patches to allow time for the runtime pm one
to sit on the list.
This one is a bug fix (and should have been earlier in the series!).
I've applied it to the togreg branch of iio.git.
Lars is of course correct that we could do this in a nicer fashion, but
I'd rather get the simple fix in place and tidy up later, than delay it
now.
Jonathan
> ---
> drivers/iio/accel/kxcjk-1013.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/iio/accel/kxcjk-1013.c b/drivers/iio/accel/kxcjk-1013.c
> index f5bb682..40ecf8b 100644
> --- a/drivers/iio/accel/kxcjk-1013.c
> +++ b/drivers/iio/accel/kxcjk-1013.c
> @@ -1020,6 +1020,7 @@ static int kxcjk1013_probe(struct i2c_client *client,
> iio_trigger_set_drvdata(trig, indio_dev);
> data->trig = trig;
> indio_dev->trig = trig;
> + iio_trigger_get(indio_dev->trig);
>
> ret = iio_trigger_register(trig);
> if (ret)
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 1/6] iio: accel: kxcjk-1013: Fix setting frequency
2014-07-17 0:42 [PATCH 1/6] iio: accel: kxcjk-1013: Fix setting frequency Srinivas Pandruvada
` (4 preceding siblings ...)
2014-07-17 0:42 ` [PATCH 6/6] iio: accel: kxcjk-1013: Increment ref counter for indio_dev->trig Srinivas Pandruvada
@ 2014-07-20 15:19 ` Jonathan Cameron
5 siblings, 0 replies; 21+ messages in thread
From: Jonathan Cameron @ 2014-07-20 15:19 UTC (permalink / raw)
To: Srinivas Pandruvada; +Cc: linux-iio
On 17/07/14 01:42, Srinivas Pandruvada wrote:
> Fix issue with setting of 12.5 and 6.25 HZ. The match of val2 fails.
>
> Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
Applied to the togreg branch of iio.git
Thanks,
> ---
> drivers/iio/accel/kxcjk-1013.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/iio/accel/kxcjk-1013.c b/drivers/iio/accel/kxcjk-1013.c
> index 72a6dbb..4702aeb 100644
> --- a/drivers/iio/accel/kxcjk-1013.c
> +++ b/drivers/iio/accel/kxcjk-1013.c
> @@ -98,7 +98,7 @@ static const struct {
> int val2;
> int odr_bits;
> } samp_freq_table[] = { {0, 781000, 0x08}, {1, 563000, 0x09},
> - {3, 125000, 0x0A}, {6, 25000, 0x0B}, {12, 5000, 0},
> + {3, 125000, 0x0A}, {6, 250000, 0x0B}, {12, 500000, 0},
> {25, 0, 0x01}, {50, 0, 0x02}, {100, 0, 0x03},
> {200, 0, 0x04}, {400, 0, 0x05}, {800, 0, 0x06},
> {1600, 0, 0x07} };
>
^ permalink raw reply [flat|nested] 21+ messages in thread