Re: [PATCH 3/6] iio: accel: kxcjk-1013: support runtime pm

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 

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

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,







--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
[Index of Archives]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Input]     [Linux Kernel]     [Linux SCSI]     [X.org]

  Powered by Linux