Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1350954 > unrolled thread
| Started by | Jonathan Cameron <jic23@kernel.org> |
|---|---|
| First post | 2016-03-05 18:50 +0100 |
| Last post | 2016-03-09 22:20 +0100 |
| Articles | 3 — 2 participants |
Back to article view | Back to linux.kernel
This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by
below is the oldest one visible, not the original post.
Re: [PATCH 4/4] iio: mma8452: add low_power mode Jonathan Cameron <jic23@kernel.org> - 2016-03-05 18:50 +0100
Re: [PATCH 4/4] iio: mma8452: add low_power mode Martin Kepplinger <martink@posteo.de> - 2016-03-06 17:00 +0100
Re: [PATCH 4/4] iio: mma8452: add low_power mode Jonathan Cameron <jic23@kernel.org> - 2016-03-09 22:20 +0100
| From | Jonathan Cameron <jic23@kernel.org> |
|---|---|
| Date | 2016-03-05 18:50 +0100 |
| Subject | Re: [PATCH 4/4] iio: mma8452: add low_power mode |
| Message-ID | <r9ouZ-18t-9@gated-at.bofh.it> |
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[] = {
>
[toc] | [next] | [standalone]
| From | Martin Kepplinger <martink@posteo.de> |
|---|---|
| Date | 2016-03-06 17:00 +0100 |
| Message-ID | <r9Jg6-6Zm-13@gated-at.bofh.it> |
| In reply to | #1350954 |
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[] = {
>>
>
[toc] | [prev] | [next] | [standalone]
| From | Jonathan Cameron <jic23@kernel.org> |
|---|---|
| Date | 2016-03-09 22:20 +0100 |
| Message-ID | <raTGp-5aL-1@gated-at.bofh.it> |
| In reply to | #1351143 |
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
>
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web