* iio: mma8452: power saving features for v4.7
@ 2016-03-03 8:24 Martin Kepplinger
2016-03-03 8:24 ` [PATCH 1/4] iio: mma8452: coding style fixes Martin Kepplinger
` (3 more replies)
0 siblings, 4 replies; 11+ messages in thread
From: Martin Kepplinger @ 2016-03-03 8:24 UTC (permalink / raw)
To: jic23, knaack.h, lars, pmeerw, christoph.muellner, mfuzzey
Cc: linux-iio, linux-kernel
The accelerometers supported by mma8452 can use significantly less power if
configured accordingly. Let's make use of this:
* add support for runtime power management
(disabling the device after 2 seconds of inactivity), and
* add a "power_modes" property to IIO sysfs, that offers a "low_power"
*active* mode of operation that roughly consumes half the power of
the default mode.
If you don't have CONFIG_PM enabled, by default nothing changes for you.
Here's how it's done and tested on MMA8653FC:
[PATCH 1/4] iio: mma8452: coding style fixes
[PATCH 2/4] iio: mma8452: avoid switching to active because of config
[PATCH 3/4] iio: mma8452: add support for runtime power management
[PATCH 4/4] iio: mma8452: add low_power mode
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 1/4] iio: mma8452: coding style fixes
2016-03-03 8:24 iio: mma8452: power saving features for v4.7 Martin Kepplinger
@ 2016-03-03 8:24 ` Martin Kepplinger
2016-03-05 17:19 ` Jonathan Cameron
2016-03-03 8:24 ` [PATCH 2/4] iio: mma8452: avoid switching to active because of config change Martin Kepplinger
` (2 subsequent siblings)
3 siblings, 1 reply; 11+ messages in thread
From: Martin Kepplinger @ 2016-03-03 8:24 UTC (permalink / raw)
To: jic23, knaack.h, lars, pmeerw, christoph.muellner, mfuzzey
Cc: linux-iio, linux-kernel, Martin Kepplinger
fix checkpatch issues like "space before tabs", too long lines or alignment.
Signed-off-by: Martin Kepplinger <martink@posteo.de>
Signed-off-by: Christoph Muellner <christoph.muellner@theobroma-systems.com>
---
drivers/iio/accel/mma8452.c | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
index 7f4994f..17d72bc 100644
--- a/drivers/iio/accel/mma8452.c
+++ b/drivers/iio/accel/mma8452.c
@@ -357,7 +357,8 @@ static int mma8452_read_raw(struct iio_dev *indio_dev,
return IIO_VAL_INT_PLUS_MICRO;
case IIO_CHAN_INFO_CALIBBIAS:
ret = i2c_smbus_read_byte_data(data->client,
- MMA8452_OFF_X + chan->scan_index);
+ MMA8452_OFF_X +
+ chan->scan_index);
if (ret < 0)
return ret;
@@ -418,7 +419,7 @@ fail:
return ret;
}
-/* returns >0 if in freefall mode, 0 if not or <0 if an error occured */
+/* returns >0 if in freefall mode, 0 if not or <0 if an error occurred */
static int mma8452_freefall_mode_enabled(struct mma8452_data *data)
{
int val;
@@ -668,7 +669,8 @@ static int mma8452_read_event_config(struct iio_dev *indio_dev,
if (ret < 0)
return ret;
- return !!(ret & BIT(chan->scan_index + chip->ev_cfg_chan_shift));
+ return !!(ret & BIT(chan->scan_index +
+ chip->ev_cfg_chan_shift));
default:
return -EINVAL;
}
@@ -1003,7 +1005,7 @@ static const struct mma_chip_info mma_chip_info_table[] = {
* bit.
* The userspace interface uses m/s^2 and we declare micro units
* So scale factor for 12 bit here is given by:
- * g * N * 1000000 / 2048 for N = 2, 4, 8 and g=9.80665
+ * g * N * 1000000 / 2048 for N = 2, 4, 8 and g=9.80665
*/
.mma_scales = { {0, 2394}, {0, 4788}, {0, 9577} },
.ev_cfg = MMA8452_TRANSIENT_CFG,
--
2.1.4
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 2/4] iio: mma8452: avoid switching to active because of config change
2016-03-03 8:24 iio: mma8452: power saving features for v4.7 Martin Kepplinger
2016-03-03 8:24 ` [PATCH 1/4] iio: mma8452: coding style fixes Martin Kepplinger
@ 2016-03-03 8:24 ` Martin Kepplinger
2016-03-05 17:23 ` Jonathan Cameron
2016-03-03 8:24 ` [PATCH 3/4] iio: mma8452: add support for runtime power management Martin Kepplinger
2016-03-03 8:24 ` [PATCH 4/4] iio: mma8452: add low_power mode Martin Kepplinger
3 siblings, 1 reply; 11+ messages in thread
From: Martin Kepplinger @ 2016-03-03 8:24 UTC (permalink / raw)
To: jic23, knaack.h, lars, pmeerw, christoph.muellner, mfuzzey
Cc: linux-iio, linux-kernel, Martin Kepplinger
The devices' config registers can only be changed in standby mode.
Up until now the driver just held the device *always* active, so for
changing a config it was *always* necessary to switch to standby.
For upcoming support for runtime pm, the device can as well be in standby
mode. Instead of putting runtime pm functions in there, just keep the
device in standby if it already is. This section is protected by a lock
after all.
Signed-off-by: Martin Kepplinger <martink@posteo.de>
Signed-off-by: Christoph Muellner <christoph.muellner@theobroma-systems.com>
---
drivers/iio/accel/mma8452.c | 35 +++++++++++++++++++++++++++++------
1 file changed, 29 insertions(+), 6 deletions(-)
diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
index 17d72bc..9c4a84a 100644
--- a/drivers/iio/accel/mma8452.c
+++ b/drivers/iio/accel/mma8452.c
@@ -393,24 +393,47 @@ static int mma8452_active(struct mma8452_data *data)
data->ctrl_reg1);
}
+/* returns >0 if active, 0 if in standby and <0 on error */
+static int mma8452_is_active(struct mma8452_data *data)
+{
+ int reg;
+
+ reg = i2c_smbus_read_byte_data(data->client, MMA8452_CTRL_REG1);
+ if (reg < 0)
+ return reg;
+
+ return reg & MMA8452_CTRL_ACTIVE;
+}
+
static int mma8452_change_config(struct mma8452_data *data, u8 reg, u8 val)
{
int ret;
+ int is_active;
mutex_lock(&data->lock);
- /* config can only be changed when in standby */
- ret = mma8452_standby(data);
- if (ret < 0)
+ is_active = mma8452_is_active(data);
+ if (is_active < 0) {
+ ret = is_active;
goto fail;
+ }
+
+ /* config can only be changed when in standby */
+ if (is_active > 0) {
+ ret = mma8452_standby(data);
+ if (ret < 0)
+ goto fail;
+ }
ret = i2c_smbus_write_byte_data(data->client, reg, val);
if (ret < 0)
goto fail;
- ret = mma8452_active(data);
- if (ret < 0)
- goto fail;
+ if (is_active > 0) {
+ ret = mma8452_active(data);
+ if (ret < 0)
+ goto fail;
+ }
ret = 0;
fail:
--
2.1.4
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 3/4] iio: mma8452: add support for runtime power management
2016-03-03 8:24 iio: mma8452: power saving features for v4.7 Martin Kepplinger
2016-03-03 8:24 ` [PATCH 1/4] iio: mma8452: coding style fixes Martin Kepplinger
2016-03-03 8:24 ` [PATCH 2/4] iio: mma8452: avoid switching to active because of config change Martin Kepplinger
@ 2016-03-03 8:24 ` Martin Kepplinger
2016-03-06 12:59 ` Jonathan Cameron
2016-03-03 8:24 ` [PATCH 4/4] iio: mma8452: add low_power mode Martin Kepplinger
3 siblings, 1 reply; 11+ messages in thread
From: Martin Kepplinger @ 2016-03-03 8:24 UTC (permalink / raw)
To: jic23, knaack.h, lars, pmeerw, christoph.muellner, mfuzzey
Cc: linux-iio, linux-kernel, Martin Kepplinger
This adds support for runtime power management and, if configured, activates
automatic standby after 2 seconds of inactivity.
Inactivity means no read of acceleration values and no events triggered or
activated.
If CONFIG_PM is not set, this doesn't change anything for existing users.
Signed-off-by: Martin Kepplinger <martink@posteo.de>
Signed-off-by: Christoph Muellner <christoph.muellner@theobroma-systems.com>
---
drivers/iio/accel/mma8452.c | 118 ++++++++++++++++++++++++++++++++++++++++----
1 file changed, 108 insertions(+), 10 deletions(-)
diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
index 9c4a84a..5ca0d16 100644
--- a/drivers/iio/accel/mma8452.c
+++ b/drivers/iio/accel/mma8452.c
@@ -31,6 +31,7 @@
#include <linux/delay.h>
#include <linux/of_device.h>
#include <linux/of_irq.h>
+#include <linux/pm_runtime.h>
#define MMA8452_STATUS 0x00
#define MMA8452_STATUS_DRDY (BIT(2) | BIT(1) | BIT(0))
@@ -92,6 +93,8 @@
#define MMA8652_DEVICE_ID 0x4a
#define MMA8653_DEVICE_ID 0x5a
+#define MMA8452_AUTO_SUSPEND_DELAY_MS 2000
+
struct mma8452_data {
struct i2c_client *client;
struct mutex lock;
@@ -172,6 +175,31 @@ static int mma8452_drdy(struct mma8452_data *data)
return -EIO;
}
+static int mma8452_set_runtime_pm_state(struct i2c_client *client, bool on)
+{
+#ifdef CONFIG_PM
+ int ret;
+
+ if (on) {
+ ret = pm_runtime_get_sync(&client->dev);
+ } else {
+ pm_runtime_mark_last_busy(&client->dev);
+ ret = pm_runtime_put_autosuspend(&client->dev);
+ }
+
+ if (ret < 0) {
+ dev_err(&client->dev,
+ "failed to change power state to %d\n", on);
+ if (on)
+ pm_runtime_put_noidle(&client->dev);
+
+ return ret;
+ }
+#endif
+
+ return 0;
+}
+
static int mma8452_read(struct mma8452_data *data, __be16 buf[3])
{
int ret = mma8452_drdy(data);
@@ -179,8 +207,16 @@ static int mma8452_read(struct mma8452_data *data, __be16 buf[3])
if (ret < 0)
return ret;
- return i2c_smbus_read_i2c_block_data(data->client, MMA8452_OUT_X,
- 3 * sizeof(__be16), (u8 *)buf);
+ ret = mma8452_set_runtime_pm_state(data->client, true);
+ if (ret)
+ return ret;
+
+ ret = i2c_smbus_read_i2c_block_data(data->client, MMA8452_OUT_X,
+ 3 * sizeof(__be16), (u8 *)buf);
+
+ ret = mma8452_set_runtime_pm_state(data->client, false);
+
+ return ret;
}
static ssize_t mma8452_show_int_plus_micros(char *buf, const int (*vals)[2],
@@ -707,7 +743,11 @@ static int mma8452_write_event_config(struct iio_dev *indio_dev,
{
struct mma8452_data *data = iio_priv(indio_dev);
const struct mma_chip_info *chip = data->chip_info;
- int val;
+ int val, ret;
+
+ ret = mma8452_set_runtime_pm_state(data->client, state);
+ if (ret)
+ return ret;
switch (dir) {
case IIO_EV_DIR_FALLING:
@@ -1139,7 +1179,11 @@ static int mma8452_data_rdy_trigger_set_state(struct iio_trigger *trig,
{
struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
struct mma8452_data *data = iio_priv(indio_dev);
- int reg;
+ int reg, ret;
+
+ ret = mma8452_set_runtime_pm_state(data->client, state);
+ if (ret)
+ return ret;
reg = i2c_smbus_read_byte_data(data->client, MMA8452_CTRL_REG4);
if (reg < 0)
@@ -1365,6 +1409,15 @@ static int mma8452_probe(struct i2c_client *client,
goto buffer_cleanup;
}
+ ret = pm_runtime_set_active(&client->dev);
+ if (ret < 0)
+ goto buffer_cleanup;
+
+ pm_runtime_enable(&client->dev);
+ pm_runtime_set_autosuspend_delay(&client->dev,
+ MMA8452_AUTO_SUSPEND_DELAY_MS);
+ pm_runtime_use_autosuspend(&client->dev);
+
ret = iio_device_register(indio_dev);
if (ret < 0)
goto buffer_cleanup;
@@ -1389,6 +1442,11 @@ static int mma8452_remove(struct i2c_client *client)
struct iio_dev *indio_dev = i2c_get_clientdata(client);
iio_device_unregister(indio_dev);
+
+ pm_runtime_disable(&client->dev);
+ pm_runtime_set_suspended(&client->dev);
+ pm_runtime_put_noidle(&client->dev);
+
iio_triggered_buffer_cleanup(indio_dev);
mma8452_trigger_cleanup(indio_dev);
mma8452_standby(iio_priv(indio_dev));
@@ -1396,6 +1454,45 @@ static int mma8452_remove(struct i2c_client *client)
return 0;
}
+#ifdef CONFIG_PM
+static int mma8452_runtime_suspend(struct device *dev)
+{
+ struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
+ struct mma8452_data *data = iio_priv(indio_dev);
+ int ret;
+
+ mutex_lock(&data->lock);
+ ret = mma8452_standby(data);
+ mutex_unlock(&data->lock);
+ if (ret < 0) {
+ dev_err(&data->client->dev, "powering off device failed\n");
+ return -EAGAIN;
+ }
+
+ return 0;
+}
+
+static int mma8452_runtime_resume(struct device *dev)
+{
+ struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
+ struct mma8452_data *data = iio_priv(indio_dev);
+ int ret, sleep_val;
+
+ ret = mma8452_active(data);
+ if (ret < 0)
+ return ret;
+
+ ret = mma8452_get_odr_index(data);
+ sleep_val = 1000 / mma8452_samp_freq[ret][0];
+ if (sleep_val < 20)
+ usleep_range(sleep_val * 1000, 20000);
+ else
+ msleep_interruptible(sleep_val);
+
+ return 0;
+}
+#endif
+
#ifdef CONFIG_PM_SLEEP
static int mma8452_suspend(struct device *dev)
{
@@ -1408,13 +1505,14 @@ static int mma8452_resume(struct device *dev)
return mma8452_active(iio_priv(i2c_get_clientdata(
to_i2c_client(dev))));
}
-
-static SIMPLE_DEV_PM_OPS(mma8452_pm_ops, mma8452_suspend, mma8452_resume);
-#define MMA8452_PM_OPS (&mma8452_pm_ops)
-#else
-#define MMA8452_PM_OPS NULL
#endif
+static const struct dev_pm_ops mma8452_pm_ops = {
+ SET_SYSTEM_SLEEP_PM_OPS(mma8452_suspend, mma8452_resume)
+ SET_RUNTIME_PM_OPS(mma8452_runtime_suspend,
+ mma8452_runtime_resume, NULL)
+};
+
static const struct i2c_device_id mma8452_id[] = {
{ "mma8452", mma8452 },
{ "mma8453", mma8453 },
@@ -1428,7 +1526,7 @@ static struct i2c_driver mma8452_driver = {
.driver = {
.name = "mma8452",
.of_match_table = of_match_ptr(mma8452_dt_ids),
- .pm = MMA8452_PM_OPS,
+ .pm = &mma8452_pm_ops,
},
.probe = mma8452_probe,
.remove = mma8452_remove,
--
2.1.4
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 4/4] iio: mma8452: add low_power mode
2016-03-03 8:24 iio: mma8452: power saving features for v4.7 Martin Kepplinger
` (2 preceding siblings ...)
2016-03-03 8:24 ` [PATCH 3/4] iio: mma8452: add support for runtime power management Martin Kepplinger
@ 2016-03-03 8:24 ` Martin Kepplinger
2016-03-05 17:40 ` Jonathan Cameron
3 siblings, 1 reply; 11+ messages in thread
From: Martin Kepplinger @ 2016-03-03 8:24 UTC (permalink / raw)
To: jic23, knaack.h, lars, pmeerw, christoph.muellner, mfuzzey
Cc: linux-iio, linux-kernel, Martin Kepplinger
This adds a mode of operation that consumes less power by lesser
oversampling. It's exposed in IIO sysfs as in_accelX_power_mode, as
documented.
It consumes roughly half the power the default low_noise mode does.
See the datasheet for details.
Signed-off-by: Martin Kepplinger <martink@posteo.de>
Signed-off-by: Christoph Muellner <christoph.muellner@theobroma-systems.com>
---
drivers/iio/accel/mma8452.c | 55 +++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 55 insertions(+)
diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
index 5ca0d16..5b5abec 100644
--- a/drivers/iio/accel/mma8452.c
+++ b/drivers/iio/accel/mma8452.c
@@ -75,6 +75,9 @@
#define MMA8452_CTRL_DR_DEFAULT 0x4 /* 50 Hz sample frequency */
#define MMA8452_CTRL_REG2 0x2b
#define MMA8452_CTRL_REG2_RST BIT(6)
+#define MMA8452_CTRL_REG2_MODS_MASK 0x1b
+#define MMA8452_CTRL_REG2_MODS_NORMAL 0x00
+#define MMA8452_CTRL_REG2_MODS_LOW_POWER 0x1b
#define MMA8452_CTRL_REG4 0x2d
#define MMA8452_CTRL_REG5 0x2e
#define MMA8452_OFF_X 0x2f
@@ -950,6 +953,56 @@ static struct attribute_group mma8452_event_attribute_group = {
.attrs = mma8452_event_attributes,
};
+static const char * const mma8452_power_modes[] = {"low_noise", "low_power"};
+
+static int mma8452_get_power_mode(struct iio_dev *indio_dev,
+ const struct iio_chan_spec *chan)
+{
+ struct mma8452_data *data = iio_priv(indio_dev);
+ int reg;
+
+ reg = i2c_smbus_read_byte_data(data->client,
+ MMA8452_CTRL_REG2);
+ if (reg < 0)
+ return reg;
+
+ return !(reg & MMA8452_CTRL_REG2_MODS_MASK);
+}
+
+static int mma8452_set_power_mode(struct iio_dev *indio_dev,
+ const struct iio_chan_spec *chan,
+ unsigned int mode)
+{
+ struct mma8452_data *data = iio_priv(indio_dev);
+ int reg;
+
+ reg = i2c_smbus_read_byte_data(data->client,
+ MMA8452_CTRL_REG2);
+ if (reg < 0)
+ return reg;
+
+ reg &= ~MMA8452_CTRL_REG2_MODS_MASK;
+ if (mode)
+ reg |= MMA8452_CTRL_REG2_MODS_LOW_POWER;
+ else
+ reg |= MMA8452_CTRL_REG2_MODS_NORMAL;
+
+ return mma8452_change_config(data, MMA8452_CTRL_REG2, reg);
+}
+
+static const struct iio_enum mma8452_power_mode_enum = {
+ .items = mma8452_power_modes,
+ .num_items = ARRAY_SIZE(mma8452_power_modes),
+ .get = mma8452_get_power_mode,
+ .set = mma8452_set_power_mode,
+};
+
+static const struct iio_chan_spec_ext_info mma8452_ext_info[] = {
+ IIO_ENUM("power_mode", true, &mma8452_power_mode_enum),
+ IIO_ENUM_AVAILABLE("power_mode", &mma8452_power_mode_enum),
+ { },
+};
+
#define MMA8452_FREEFALL_CHANNEL(modifier) { \
.type = IIO_ACCEL, \
.modified = 1, \
@@ -987,6 +1040,7 @@ static struct attribute_group mma8452_event_attribute_group = {
}, \
.event_spec = mma8452_transient_event, \
.num_event_specs = ARRAY_SIZE(mma8452_transient_event), \
+ .ext_info = mma8452_ext_info, \
}
#define MMA8652_CHANNEL(axis, idx, bits) { \
@@ -1007,6 +1061,7 @@ static struct attribute_group mma8452_event_attribute_group = {
}, \
.event_spec = mma8452_motion_event, \
.num_event_specs = ARRAY_SIZE(mma8452_motion_event), \
+ .ext_info = mma8452_ext_info, \
}
static const struct iio_chan_spec mma8451_channels[] = {
--
2.1.4
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 1/4] iio: mma8452: coding style fixes
2016-03-03 8:24 ` [PATCH 1/4] iio: mma8452: coding style fixes Martin Kepplinger
@ 2016-03-05 17:19 ` Jonathan Cameron
0 siblings, 0 replies; 11+ messages in thread
From: Jonathan Cameron @ 2016-03-05 17:19 UTC (permalink / raw)
To: Martin Kepplinger, knaack.h, lars, pmeerw, christoph.muellner,
mfuzzey
Cc: linux-iio, linux-kernel
On 03/03/16 08:24, Martin Kepplinger wrote:
> fix checkpatch issues like "space before tabs", too long lines or alignment.
>
> Signed-off-by: Martin Kepplinger <martink@posteo.de>
> Signed-off-by: Christoph Muellner <christoph.muellner@theobroma-systems.com>
Applied to the togreg branch of iio.git - initially pushed out as testing.
Thanks,
Jonathan
> ---
> drivers/iio/accel/mma8452.c | 10 ++++++----
> 1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
> index 7f4994f..17d72bc 100644
> --- a/drivers/iio/accel/mma8452.c
> +++ b/drivers/iio/accel/mma8452.c
> @@ -357,7 +357,8 @@ static int mma8452_read_raw(struct iio_dev *indio_dev,
> return IIO_VAL_INT_PLUS_MICRO;
> case IIO_CHAN_INFO_CALIBBIAS:
> ret = i2c_smbus_read_byte_data(data->client,
> - MMA8452_OFF_X + chan->scan_index);
> + MMA8452_OFF_X +
> + chan->scan_index);
> if (ret < 0)
> return ret;
>
> @@ -418,7 +419,7 @@ fail:
> return ret;
> }
>
> -/* returns >0 if in freefall mode, 0 if not or <0 if an error occured */
> +/* returns >0 if in freefall mode, 0 if not or <0 if an error occurred */
> static int mma8452_freefall_mode_enabled(struct mma8452_data *data)
> {
> int val;
> @@ -668,7 +669,8 @@ static int mma8452_read_event_config(struct iio_dev *indio_dev,
> if (ret < 0)
> return ret;
>
> - return !!(ret & BIT(chan->scan_index + chip->ev_cfg_chan_shift));
> + return !!(ret & BIT(chan->scan_index +
> + chip->ev_cfg_chan_shift));
> default:
> return -EINVAL;
> }
> @@ -1003,7 +1005,7 @@ static const struct mma_chip_info mma_chip_info_table[] = {
> * bit.
> * The userspace interface uses m/s^2 and we declare micro units
> * So scale factor for 12 bit here is given by:
> - * g * N * 1000000 / 2048 for N = 2, 4, 8 and g=9.80665
> + * g * N * 1000000 / 2048 for N = 2, 4, 8 and g=9.80665
> */
> .mma_scales = { {0, 2394}, {0, 4788}, {0, 9577} },
> .ev_cfg = MMA8452_TRANSIENT_CFG,
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/4] iio: mma8452: avoid switching to active because of config change
2016-03-03 8:24 ` [PATCH 2/4] iio: mma8452: avoid switching to active because of config change Martin Kepplinger
@ 2016-03-05 17:23 ` Jonathan Cameron
0 siblings, 0 replies; 11+ messages in thread
From: Jonathan Cameron @ 2016-03-05 17:23 UTC (permalink / raw)
To: Martin Kepplinger, knaack.h, lars, pmeerw, christoph.muellner,
mfuzzey
Cc: linux-iio, linux-kernel
On 03/03/16 08:24, Martin Kepplinger wrote:
> The devices' config registers can only be changed in standby mode.
> Up until now the driver just held the device *always* active, so for
> changing a config it was *always* necessary to switch to standby.
>
> For upcoming support for runtime pm, the device can as well be in standby
> mode. Instead of putting runtime pm functions in there, just keep the
> device in standby if it already is. This section is protected by a lock
> after all.
>
> Signed-off-by: Martin Kepplinger <martink@posteo.de>
> Signed-off-by: Christoph Muellner <christoph.muellner@theobroma-systems.com>
Applied to the togreg branch of iio.git - initially pushed out as testing
Thanks,
Jonathan
> ---
> drivers/iio/accel/mma8452.c | 35 +++++++++++++++++++++++++++++------
> 1 file changed, 29 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
> index 17d72bc..9c4a84a 100644
> --- a/drivers/iio/accel/mma8452.c
> +++ b/drivers/iio/accel/mma8452.c
> @@ -393,24 +393,47 @@ static int mma8452_active(struct mma8452_data *data)
> data->ctrl_reg1);
> }
>
> +/* returns >0 if active, 0 if in standby and <0 on error */
> +static int mma8452_is_active(struct mma8452_data *data)
> +{
> + int reg;
> +
> + reg = i2c_smbus_read_byte_data(data->client, MMA8452_CTRL_REG1);
> + if (reg < 0)
> + return reg;
> +
> + return reg & MMA8452_CTRL_ACTIVE;
> +}
> +
> static int mma8452_change_config(struct mma8452_data *data, u8 reg, u8 val)
> {
> int ret;
> + int is_active;
>
> mutex_lock(&data->lock);
>
> - /* config can only be changed when in standby */
> - ret = mma8452_standby(data);
> - if (ret < 0)
> + is_active = mma8452_is_active(data);
> + if (is_active < 0) {
> + ret = is_active;
> goto fail;
> + }
> +
> + /* config can only be changed when in standby */
> + if (is_active > 0) {
> + ret = mma8452_standby(data);
> + if (ret < 0)
> + goto fail;
> + }
>
> ret = i2c_smbus_write_byte_data(data->client, reg, val);
> if (ret < 0)
> goto fail;
>
> - ret = mma8452_active(data);
> - if (ret < 0)
> - goto fail;
> + if (is_active > 0) {
> + ret = mma8452_active(data);
> + if (ret < 0)
> + goto fail;
> + }
>
> ret = 0;
> fail:
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 4/4] iio: mma8452: add low_power mode
2016-03-03 8:24 ` [PATCH 4/4] iio: mma8452: add low_power mode Martin Kepplinger
@ 2016-03-05 17:40 ` Jonathan Cameron
2016-03-06 15:49 ` Martin Kepplinger
0 siblings, 1 reply; 11+ messages in thread
From: Jonathan Cameron @ 2016-03-05 17:40 UTC (permalink / raw)
To: Martin Kepplinger, knaack.h, lars, pmeerw, christoph.muellner,
mfuzzey
Cc: linux-iio, linux-kernel
On 03/03/16 08:24, Martin Kepplinger wrote:
> This adds a mode of operation that consumes less power by lesser
> oversampling. It's exposed in IIO sysfs as in_accelX_power_mode, as
> documented.
>
> It consumes roughly half the power the default low_noise mode does.
> See the datasheet for details.
>
> Signed-off-by: Martin Kepplinger <martink@posteo.de>
> Signed-off-by: Christoph Muellner <christoph.muellner@theobroma-systems.com>
This is in the datasheet as oversampling. So why are we not using the oversampling
interface rather than adding a new one?
It's obvious that with the interactions with sampling rate that this won't be trivial
to support, but it doesn't look that hard. For a given sampling_frequency only
certain oversampling rations are possible, but that's not exactly unusual.
That way everything is explicit, whereas with this approach people will need to
read the datasheet to know if setting the power mode is having any effect at all.
That will also allow appropriate trade offs to use the low noise / low power version
if the oversampling ratio is appropriate. All this stuff is clearly given in
table 59 of the datasheet google gave me.
(actually as control mechanisms go for this sort of setting I think this one is
reasonably elegant as such hardware goes!)
Jonathan
> ---
> drivers/iio/accel/mma8452.c | 55 +++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 55 insertions(+)
>
> diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
> index 5ca0d16..5b5abec 100644
> --- a/drivers/iio/accel/mma8452.c
> +++ b/drivers/iio/accel/mma8452.c
> @@ -75,6 +75,9 @@
> #define MMA8452_CTRL_DR_DEFAULT 0x4 /* 50 Hz sample frequency */
> #define MMA8452_CTRL_REG2 0x2b
> #define MMA8452_CTRL_REG2_RST BIT(6)
> +#define MMA8452_CTRL_REG2_MODS_MASK 0x1b
> +#define MMA8452_CTRL_REG2_MODS_NORMAL 0x00
> +#define MMA8452_CTRL_REG2_MODS_LOW_POWER 0x1b
> #define MMA8452_CTRL_REG4 0x2d
> #define MMA8452_CTRL_REG5 0x2e
> #define MMA8452_OFF_X 0x2f
> @@ -950,6 +953,56 @@ static struct attribute_group mma8452_event_attribute_group = {
> .attrs = mma8452_event_attributes,
> };
>
> +static const char * const mma8452_power_modes[] = {"low_noise", "low_power"};
> +
> +static int mma8452_get_power_mode(struct iio_dev *indio_dev,
> + const struct iio_chan_spec *chan)
> +{
> + struct mma8452_data *data = iio_priv(indio_dev);
> + int reg;
> +
> + reg = i2c_smbus_read_byte_data(data->client,
> + MMA8452_CTRL_REG2);
> + if (reg < 0)
> + return reg;
> +
> + return !(reg & MMA8452_CTRL_REG2_MODS_MASK);
> +}
> +
> +static int mma8452_set_power_mode(struct iio_dev *indio_dev,
> + const struct iio_chan_spec *chan,
> + unsigned int mode)
> +{
> + struct mma8452_data *data = iio_priv(indio_dev);
> + int reg;
> +
> + reg = i2c_smbus_read_byte_data(data->client,
> + MMA8452_CTRL_REG2);
> + if (reg < 0)
> + return reg;
> +
> + reg &= ~MMA8452_CTRL_REG2_MODS_MASK;
> + if (mode)
> + reg |= MMA8452_CTRL_REG2_MODS_LOW_POWER;
> + else
> + reg |= MMA8452_CTRL_REG2_MODS_NORMAL;
> +
> + return mma8452_change_config(data, MMA8452_CTRL_REG2, reg);
> +}
> +
> +static const struct iio_enum mma8452_power_mode_enum = {
> + .items = mma8452_power_modes,
> + .num_items = ARRAY_SIZE(mma8452_power_modes),
> + .get = mma8452_get_power_mode,
> + .set = mma8452_set_power_mode,
> +};
> +
> +static const struct iio_chan_spec_ext_info mma8452_ext_info[] = {
> + IIO_ENUM("power_mode", true, &mma8452_power_mode_enum),
> + IIO_ENUM_AVAILABLE("power_mode", &mma8452_power_mode_enum),
> + { },
> +};
> +
> #define MMA8452_FREEFALL_CHANNEL(modifier) { \
> .type = IIO_ACCEL, \
> .modified = 1, \
> @@ -987,6 +1040,7 @@ static struct attribute_group mma8452_event_attribute_group = {
> }, \
> .event_spec = mma8452_transient_event, \
> .num_event_specs = ARRAY_SIZE(mma8452_transient_event), \
> + .ext_info = mma8452_ext_info, \
> }
>
> #define MMA8652_CHANNEL(axis, idx, bits) { \
> @@ -1007,6 +1061,7 @@ static struct attribute_group mma8452_event_attribute_group = {
> }, \
> .event_spec = mma8452_motion_event, \
> .num_event_specs = ARRAY_SIZE(mma8452_motion_event), \
> + .ext_info = mma8452_ext_info, \
> }
>
> static const struct iio_chan_spec mma8451_channels[] = {
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 3/4] iio: mma8452: add support for runtime power management
2016-03-03 8:24 ` [PATCH 3/4] iio: mma8452: add support for runtime power management Martin Kepplinger
@ 2016-03-06 12:59 ` Jonathan Cameron
0 siblings, 0 replies; 11+ messages in thread
From: Jonathan Cameron @ 2016-03-06 12:59 UTC (permalink / raw)
To: Martin Kepplinger, knaack.h, lars, pmeerw, christoph.muellner,
mfuzzey
Cc: linux-iio, linux-kernel
On 03/03/16 08:24, Martin Kepplinger wrote:
> This adds support for runtime power management and, if configured, activates
> automatic standby after 2 seconds of inactivity.
>
> Inactivity means no read of acceleration values and no events triggered or
> activated.
>
> If CONFIG_PM is not set, this doesn't change anything for existing users.
>
> Signed-off-by: Martin Kepplinger <martink@posteo.de>
> Signed-off-by: Christoph Muellner <christoph.muellner@theobroma-systems.com>
Looks good. Applied to the togreg branch of iio.git - initially pushed out
as testing for the autobuilders to play with it.
Thanks,
Jonathan
> ---
> drivers/iio/accel/mma8452.c | 118 ++++++++++++++++++++++++++++++++++++++++----
> 1 file changed, 108 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
> index 9c4a84a..5ca0d16 100644
> --- a/drivers/iio/accel/mma8452.c
> +++ b/drivers/iio/accel/mma8452.c
> @@ -31,6 +31,7 @@
> #include <linux/delay.h>
> #include <linux/of_device.h>
> #include <linux/of_irq.h>
> +#include <linux/pm_runtime.h>
>
> #define MMA8452_STATUS 0x00
> #define MMA8452_STATUS_DRDY (BIT(2) | BIT(1) | BIT(0))
> @@ -92,6 +93,8 @@
> #define MMA8652_DEVICE_ID 0x4a
> #define MMA8653_DEVICE_ID 0x5a
>
> +#define MMA8452_AUTO_SUSPEND_DELAY_MS 2000
> +
> struct mma8452_data {
> struct i2c_client *client;
> struct mutex lock;
> @@ -172,6 +175,31 @@ static int mma8452_drdy(struct mma8452_data *data)
> return -EIO;
> }
>
> +static int mma8452_set_runtime_pm_state(struct i2c_client *client, bool on)
> +{
> +#ifdef CONFIG_PM
> + int ret;
> +
> + if (on) {
> + ret = pm_runtime_get_sync(&client->dev);
> + } else {
> + pm_runtime_mark_last_busy(&client->dev);
> + ret = pm_runtime_put_autosuspend(&client->dev);
> + }
> +
> + if (ret < 0) {
> + dev_err(&client->dev,
> + "failed to change power state to %d\n", on);
> + if (on)
> + pm_runtime_put_noidle(&client->dev);
> +
> + return ret;
> + }
> +#endif
> +
> + return 0;
> +}
> +
> static int mma8452_read(struct mma8452_data *data, __be16 buf[3])
> {
> int ret = mma8452_drdy(data);
> @@ -179,8 +207,16 @@ static int mma8452_read(struct mma8452_data *data, __be16 buf[3])
> if (ret < 0)
> return ret;
>
> - return i2c_smbus_read_i2c_block_data(data->client, MMA8452_OUT_X,
> - 3 * sizeof(__be16), (u8 *)buf);
> + ret = mma8452_set_runtime_pm_state(data->client, true);
> + if (ret)
> + return ret;
> +
> + ret = i2c_smbus_read_i2c_block_data(data->client, MMA8452_OUT_X,
> + 3 * sizeof(__be16), (u8 *)buf);
> +
> + ret = mma8452_set_runtime_pm_state(data->client, false);
> +
> + return ret;
> }
>
> static ssize_t mma8452_show_int_plus_micros(char *buf, const int (*vals)[2],
> @@ -707,7 +743,11 @@ static int mma8452_write_event_config(struct iio_dev *indio_dev,
> {
> struct mma8452_data *data = iio_priv(indio_dev);
> const struct mma_chip_info *chip = data->chip_info;
> - int val;
> + int val, ret;
> +
> + ret = mma8452_set_runtime_pm_state(data->client, state);
> + if (ret)
> + return ret;
>
> switch (dir) {
> case IIO_EV_DIR_FALLING:
> @@ -1139,7 +1179,11 @@ static int mma8452_data_rdy_trigger_set_state(struct iio_trigger *trig,
> {
> struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
> struct mma8452_data *data = iio_priv(indio_dev);
> - int reg;
> + int reg, ret;
> +
> + ret = mma8452_set_runtime_pm_state(data->client, state);
> + if (ret)
> + return ret;
>
> reg = i2c_smbus_read_byte_data(data->client, MMA8452_CTRL_REG4);
> if (reg < 0)
> @@ -1365,6 +1409,15 @@ static int mma8452_probe(struct i2c_client *client,
> goto buffer_cleanup;
> }
>
> + ret = pm_runtime_set_active(&client->dev);
> + if (ret < 0)
> + goto buffer_cleanup;
> +
> + pm_runtime_enable(&client->dev);
> + pm_runtime_set_autosuspend_delay(&client->dev,
> + MMA8452_AUTO_SUSPEND_DELAY_MS);
> + pm_runtime_use_autosuspend(&client->dev);
> +
> ret = iio_device_register(indio_dev);
> if (ret < 0)
> goto buffer_cleanup;
> @@ -1389,6 +1442,11 @@ static int mma8452_remove(struct i2c_client *client)
> struct iio_dev *indio_dev = i2c_get_clientdata(client);
>
> iio_device_unregister(indio_dev);
> +
> + pm_runtime_disable(&client->dev);
> + pm_runtime_set_suspended(&client->dev);
> + pm_runtime_put_noidle(&client->dev);
> +
> iio_triggered_buffer_cleanup(indio_dev);
> mma8452_trigger_cleanup(indio_dev);
> mma8452_standby(iio_priv(indio_dev));
> @@ -1396,6 +1454,45 @@ static int mma8452_remove(struct i2c_client *client)
> return 0;
> }
>
> +#ifdef CONFIG_PM
> +static int mma8452_runtime_suspend(struct device *dev)
> +{
> + struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
> + struct mma8452_data *data = iio_priv(indio_dev);
> + int ret;
> +
> + mutex_lock(&data->lock);
> + ret = mma8452_standby(data);
> + mutex_unlock(&data->lock);
> + if (ret < 0) {
> + dev_err(&data->client->dev, "powering off device failed\n");
> + return -EAGAIN;
> + }
> +
> + return 0;
> +}
> +
> +static int mma8452_runtime_resume(struct device *dev)
> +{
> + struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
> + struct mma8452_data *data = iio_priv(indio_dev);
> + int ret, sleep_val;
> +
> + ret = mma8452_active(data);
> + if (ret < 0)
> + return ret;
> +
> + ret = mma8452_get_odr_index(data);
> + sleep_val = 1000 / mma8452_samp_freq[ret][0];
> + if (sleep_val < 20)
> + usleep_range(sleep_val * 1000, 20000);
> + else
> + msleep_interruptible(sleep_val);
> +
> + return 0;
> +}
> +#endif
> +
> #ifdef CONFIG_PM_SLEEP
> static int mma8452_suspend(struct device *dev)
> {
> @@ -1408,13 +1505,14 @@ static int mma8452_resume(struct device *dev)
> return mma8452_active(iio_priv(i2c_get_clientdata(
> to_i2c_client(dev))));
> }
> -
> -static SIMPLE_DEV_PM_OPS(mma8452_pm_ops, mma8452_suspend, mma8452_resume);
> -#define MMA8452_PM_OPS (&mma8452_pm_ops)
> -#else
> -#define MMA8452_PM_OPS NULL
> #endif
>
> +static const struct dev_pm_ops mma8452_pm_ops = {
> + SET_SYSTEM_SLEEP_PM_OPS(mma8452_suspend, mma8452_resume)
> + SET_RUNTIME_PM_OPS(mma8452_runtime_suspend,
> + mma8452_runtime_resume, NULL)
> +};
> +
> static const struct i2c_device_id mma8452_id[] = {
> { "mma8452", mma8452 },
> { "mma8453", mma8453 },
> @@ -1428,7 +1526,7 @@ static struct i2c_driver mma8452_driver = {
> .driver = {
> .name = "mma8452",
> .of_match_table = of_match_ptr(mma8452_dt_ids),
> - .pm = MMA8452_PM_OPS,
> + .pm = &mma8452_pm_ops,
> },
> .probe = mma8452_probe,
> .remove = mma8452_remove,
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 4/4] iio: mma8452: add low_power mode
2016-03-05 17:40 ` Jonathan Cameron
@ 2016-03-06 15:49 ` Martin Kepplinger
2016-03-09 21:17 ` Jonathan Cameron
0 siblings, 1 reply; 11+ messages in thread
From: Martin Kepplinger @ 2016-03-06 15:49 UTC (permalink / raw)
To: Jonathan Cameron, knaack.h, lars, pmeerw, christoph.muellner,
mfuzzey
Cc: linux-iio, linux-kernel
Am 2016-03-05 um 18:40 schrieb Jonathan Cameron:
> On 03/03/16 08:24, Martin Kepplinger wrote:
>> This adds a mode of operation that consumes less power by lesser
>> oversampling. It's exposed in IIO sysfs as in_accelX_power_mode, as
>> documented.
>>
>> It consumes roughly half the power the default low_noise mode does.
>> See the datasheet for details.
>>
>> Signed-off-by: Martin Kepplinger <martink@posteo.de>
>> Signed-off-by: Christoph Muellner <christoph.muellner@theobroma-systems.com>
> This is in the datasheet as oversampling. So why are we not using the oversampling
> interface rather than adding a new one?
>
> It's obvious that with the interactions with sampling rate that this won't be trivial
> to support, but it doesn't look that hard. For a given sampling_frequency only
> certain oversampling rations are possible, but that's not exactly unusual.
>
> That way everything is explicit, whereas with this approach people will need to
> read the datasheet to know if setting the power mode is having any effect at all.
>
> That will also allow appropriate trade offs to use the low noise / low power version
> if the oversampling ratio is appropriate. All this stuff is clearly given in
> table 59 of the datasheet google gave me.
> (actually as control mechanisms go for this sort of setting I think this one is
> reasonably elegant as such hardware goes!)
>
> Jonathan
>
True, that way a more accurate way of describing the hardware would be
possible. I figured power_mode already is an ABI and used in another
driver, and very convenient to use.
How about having both, so that the connection to saving power is obvious
for users? This way I would have to extend "power_modes_available" to
have 4 modes, but no reading of the data sheet would be necessary to
understand it all.
It would not be trivial but doable and I could redo this by supporting
oversampling ABI first, and try to add this power_mode knob seperately?
martin
>> ---
>> drivers/iio/accel/mma8452.c | 55 +++++++++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 55 insertions(+)
>>
>> diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
>> index 5ca0d16..5b5abec 100644
>> --- a/drivers/iio/accel/mma8452.c
>> +++ b/drivers/iio/accel/mma8452.c
>> @@ -75,6 +75,9 @@
>> #define MMA8452_CTRL_DR_DEFAULT 0x4 /* 50 Hz sample frequency */
>> #define MMA8452_CTRL_REG2 0x2b
>> #define MMA8452_CTRL_REG2_RST BIT(6)
>> +#define MMA8452_CTRL_REG2_MODS_MASK 0x1b
>> +#define MMA8452_CTRL_REG2_MODS_NORMAL 0x00
>> +#define MMA8452_CTRL_REG2_MODS_LOW_POWER 0x1b
>> #define MMA8452_CTRL_REG4 0x2d
>> #define MMA8452_CTRL_REG5 0x2e
>> #define MMA8452_OFF_X 0x2f
>> @@ -950,6 +953,56 @@ static struct attribute_group mma8452_event_attribute_group = {
>> .attrs = mma8452_event_attributes,
>> };
>>
>> +static const char * const mma8452_power_modes[] = {"low_noise", "low_power"};
>> +
>> +static int mma8452_get_power_mode(struct iio_dev *indio_dev,
>> + const struct iio_chan_spec *chan)
>> +{
>> + struct mma8452_data *data = iio_priv(indio_dev);
>> + int reg;
>> +
>> + reg = i2c_smbus_read_byte_data(data->client,
>> + MMA8452_CTRL_REG2);
>> + if (reg < 0)
>> + return reg;
>> +
>> + return !(reg & MMA8452_CTRL_REG2_MODS_MASK);
>> +}
>> +
>> +static int mma8452_set_power_mode(struct iio_dev *indio_dev,
>> + const struct iio_chan_spec *chan,
>> + unsigned int mode)
>> +{
>> + struct mma8452_data *data = iio_priv(indio_dev);
>> + int reg;
>> +
>> + reg = i2c_smbus_read_byte_data(data->client,
>> + MMA8452_CTRL_REG2);
>> + if (reg < 0)
>> + return reg;
>> +
>> + reg &= ~MMA8452_CTRL_REG2_MODS_MASK;
>> + if (mode)
>> + reg |= MMA8452_CTRL_REG2_MODS_LOW_POWER;
>> + else
>> + reg |= MMA8452_CTRL_REG2_MODS_NORMAL;
>> +
>> + return mma8452_change_config(data, MMA8452_CTRL_REG2, reg);
>> +}
>> +
>> +static const struct iio_enum mma8452_power_mode_enum = {
>> + .items = mma8452_power_modes,
>> + .num_items = ARRAY_SIZE(mma8452_power_modes),
>> + .get = mma8452_get_power_mode,
>> + .set = mma8452_set_power_mode,
>> +};
>> +
>> +static const struct iio_chan_spec_ext_info mma8452_ext_info[] = {
>> + IIO_ENUM("power_mode", true, &mma8452_power_mode_enum),
>> + IIO_ENUM_AVAILABLE("power_mode", &mma8452_power_mode_enum),
>> + { },
>> +};
>> +
>> #define MMA8452_FREEFALL_CHANNEL(modifier) { \
>> .type = IIO_ACCEL, \
>> .modified = 1, \
>> @@ -987,6 +1040,7 @@ static struct attribute_group mma8452_event_attribute_group = {
>> }, \
>> .event_spec = mma8452_transient_event, \
>> .num_event_specs = ARRAY_SIZE(mma8452_transient_event), \
>> + .ext_info = mma8452_ext_info, \
>> }
>>
>> #define MMA8652_CHANNEL(axis, idx, bits) { \
>> @@ -1007,6 +1061,7 @@ static struct attribute_group mma8452_event_attribute_group = {
>> }, \
>> .event_spec = mma8452_motion_event, \
>> .num_event_specs = ARRAY_SIZE(mma8452_motion_event), \
>> + .ext_info = mma8452_ext_info, \
>> }
>>
>> static const struct iio_chan_spec mma8451_channels[] = {
>>
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 4/4] iio: mma8452: add low_power mode
2016-03-06 15:49 ` Martin Kepplinger
@ 2016-03-09 21:17 ` Jonathan Cameron
0 siblings, 0 replies; 11+ messages in thread
From: Jonathan Cameron @ 2016-03-09 21:17 UTC (permalink / raw)
To: Martin Kepplinger, knaack.h, lars, pmeerw, christoph.muellner,
mfuzzey
Cc: linux-iio, linux-kernel
On 06/03/16 15:49, Martin Kepplinger wrote:
> Am 2016-03-05 um 18:40 schrieb Jonathan Cameron:
>> On 03/03/16 08:24, Martin Kepplinger wrote:
>>> This adds a mode of operation that consumes less power by lesser
>>> oversampling. It's exposed in IIO sysfs as in_accelX_power_mode, as
>>> documented.
>>>
>>> It consumes roughly half the power the default low_noise mode does.
>>> See the datasheet for details.
>>>
>>> Signed-off-by: Martin Kepplinger <martink@posteo.de>
>>> Signed-off-by: Christoph Muellner <christoph.muellner@theobroma-systems.com>
>> This is in the datasheet as oversampling. So why are we not using the oversampling
>> interface rather than adding a new one?
>>
>> It's obvious that with the interactions with sampling rate that this won't be trivial
>> to support, but it doesn't look that hard. For a given sampling_frequency only
>> certain oversampling rations are possible, but that's not exactly unusual.
>>
>> That way everything is explicit, whereas with this approach people will need to
>> read the datasheet to know if setting the power mode is having any effect at all.
>>
>> That will also allow appropriate trade offs to use the low noise / low power version
>> if the oversampling ratio is appropriate. All this stuff is clearly given in
>> table 59 of the datasheet google gave me.
>> (actually as control mechanisms go for this sort of setting I think this one is
>> reasonably elegant as such hardware goes!)
>>
>> Jonathan
>>
>
> True, that way a more accurate way of describing the hardware would be
> possible. I figured power_mode already is an ABI and used in another
> driver, and very convenient to use.
>
> How about having both, so that the connection to saving power is obvious
> for users? This way I would have to extend "power_modes_available" to
> have 4 modes, but no reading of the data sheet would be necessary to
> understand it all.
>
> It would not be trivial but doable and I could redo this by supporting
> oversampling ABI first, and try to add this power_mode knob seperately?
That indeed sounds like a good approach. There are kernel wide discussions
about such control knobs from time to time as the argument is that scattering
them everywhere just leads to them not being used, but I don't think any firm
conclusions on how to do it have been reached.
>
> martin
>
>>> ---
>>> drivers/iio/accel/mma8452.c | 55 +++++++++++++++++++++++++++++++++++++++++++++
>>> 1 file changed, 55 insertions(+)
>>>
>>> diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
>>> index 5ca0d16..5b5abec 100644
>>> --- a/drivers/iio/accel/mma8452.c
>>> +++ b/drivers/iio/accel/mma8452.c
>>> @@ -75,6 +75,9 @@
>>> #define MMA8452_CTRL_DR_DEFAULT 0x4 /* 50 Hz sample frequency */
>>> #define MMA8452_CTRL_REG2 0x2b
>>> #define MMA8452_CTRL_REG2_RST BIT(6)
>>> +#define MMA8452_CTRL_REG2_MODS_MASK 0x1b
>>> +#define MMA8452_CTRL_REG2_MODS_NORMAL 0x00
>>> +#define MMA8452_CTRL_REG2_MODS_LOW_POWER 0x1b
>>> #define MMA8452_CTRL_REG4 0x2d
>>> #define MMA8452_CTRL_REG5 0x2e
>>> #define MMA8452_OFF_X 0x2f
>>> @@ -950,6 +953,56 @@ static struct attribute_group mma8452_event_attribute_group = {
>>> .attrs = mma8452_event_attributes,
>>> };
>>>
>>> +static const char * const mma8452_power_modes[] = {"low_noise", "low_power"};
>>> +
>>> +static int mma8452_get_power_mode(struct iio_dev *indio_dev,
>>> + const struct iio_chan_spec *chan)
>>> +{
>>> + struct mma8452_data *data = iio_priv(indio_dev);
>>> + int reg;
>>> +
>>> + reg = i2c_smbus_read_byte_data(data->client,
>>> + MMA8452_CTRL_REG2);
>>> + if (reg < 0)
>>> + return reg;
>>> +
>>> + return !(reg & MMA8452_CTRL_REG2_MODS_MASK);
>>> +}
>>> +
>>> +static int mma8452_set_power_mode(struct iio_dev *indio_dev,
>>> + const struct iio_chan_spec *chan,
>>> + unsigned int mode)
>>> +{
>>> + struct mma8452_data *data = iio_priv(indio_dev);
>>> + int reg;
>>> +
>>> + reg = i2c_smbus_read_byte_data(data->client,
>>> + MMA8452_CTRL_REG2);
>>> + if (reg < 0)
>>> + return reg;
>>> +
>>> + reg &= ~MMA8452_CTRL_REG2_MODS_MASK;
>>> + if (mode)
>>> + reg |= MMA8452_CTRL_REG2_MODS_LOW_POWER;
>>> + else
>>> + reg |= MMA8452_CTRL_REG2_MODS_NORMAL;
>>> +
>>> + return mma8452_change_config(data, MMA8452_CTRL_REG2, reg);
>>> +}
>>> +
>>> +static const struct iio_enum mma8452_power_mode_enum = {
>>> + .items = mma8452_power_modes,
>>> + .num_items = ARRAY_SIZE(mma8452_power_modes),
>>> + .get = mma8452_get_power_mode,
>>> + .set = mma8452_set_power_mode,
>>> +};
>>> +
>>> +static const struct iio_chan_spec_ext_info mma8452_ext_info[] = {
>>> + IIO_ENUM("power_mode", true, &mma8452_power_mode_enum),
>>> + IIO_ENUM_AVAILABLE("power_mode", &mma8452_power_mode_enum),
>>> + { },
>>> +};
>>> +
>>> #define MMA8452_FREEFALL_CHANNEL(modifier) { \
>>> .type = IIO_ACCEL, \
>>> .modified = 1, \
>>> @@ -987,6 +1040,7 @@ static struct attribute_group mma8452_event_attribute_group = {
>>> }, \
>>> .event_spec = mma8452_transient_event, \
>>> .num_event_specs = ARRAY_SIZE(mma8452_transient_event), \
>>> + .ext_info = mma8452_ext_info, \
>>> }
>>>
>>> #define MMA8652_CHANNEL(axis, idx, bits) { \
>>> @@ -1007,6 +1061,7 @@ static struct attribute_group mma8452_event_attribute_group = {
>>> }, \
>>> .event_spec = mma8452_motion_event, \
>>> .num_event_specs = ARRAY_SIZE(mma8452_motion_event), \
>>> + .ext_info = mma8452_ext_info, \
>>> }
>>>
>>> static const struct iio_chan_spec mma8451_channels[] = {
>>>
>>
>
> --
> 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] 11+ messages in thread
end of thread, other threads:[~2016-03-09 21:17 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-03-03 8:24 iio: mma8452: power saving features for v4.7 Martin Kepplinger
2016-03-03 8:24 ` [PATCH 1/4] iio: mma8452: coding style fixes Martin Kepplinger
2016-03-05 17:19 ` Jonathan Cameron
2016-03-03 8:24 ` [PATCH 2/4] iio: mma8452: avoid switching to active because of config change Martin Kepplinger
2016-03-05 17:23 ` Jonathan Cameron
2016-03-03 8:24 ` [PATCH 3/4] iio: mma8452: add support for runtime power management Martin Kepplinger
2016-03-06 12:59 ` Jonathan Cameron
2016-03-03 8:24 ` [PATCH 4/4] iio: mma8452: add low_power mode Martin Kepplinger
2016-03-05 17:40 ` Jonathan Cameron
2016-03-06 15:49 ` Martin Kepplinger
2016-03-09 21:17 ` Jonathan Cameron
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).