[PATCH] iio: mma8452: add power_mode sysfs configuration

[PATCH] iio: mma8452: add power_mode sysfs configuration

[Martin Kepplinger]

This adds the power_mode sysfs interface to the device as documented in
sysfs-bus-iio.

---

Note that I explicitely don't sign off on this.

This is a starting point for anybody who can test it and check for correct
API usage, and ABI correctness, as documented in Documentation/ABI/testing/sys-bus-iio
(grep it for "power_mode"). The ABI doc probably would need an addition
too, if the 4 power modes here seem generally useful (there are only
 2 listed there)!

So, if you can test this, feel free to set up a proper patch or
two, and I'm happy to review.

Please note that this patch is quite old. It really should be that simple
as far as my understanding back then. We always list the available frequencies
of the given power mode we are in, for example, already, and everything
basically is in place except for the user interface.

thanks
                            martin



 drivers/iio/accel/mma8452.c | 37 +++++++++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
index bfd4bc806fc2..640bbd9872ab 100644
--- a/drivers/iio/accel/mma8452.c
+++ b/drivers/iio/accel/mma8452.c
@@ -1166,6 +1166,41 @@ static struct attribute_group mma8452_event_attribute_group = {
 	.attrs = mma8452_event_attributes,
 };
 
+static const char * const mma8452_power_modes[] = {"normal",
+						   "low_noise_low_power",
+						   "low_noise",
+						   "low_power"};
+
+static int mma8452_get_power_mode_iio_enum(struct iio_dev *indio_dev,
+					   const struct iio_chan_spec *chan)
+{
+	struct mma8452_data *data = iio_priv(indio_dev);
+
+	return mma8452_get_power_mode(data);
+}
+
+static int mma8452_set_power_mode_iio_enum(struct iio_dev *indio_dev,
+					   const struct iio_chan_spec *chan,
+					   unsigned int mode)
+{
+	struct mma8452_data *data = iio_priv(indio_dev);
+
+	return mma8452_set_power_mode(data, mode);
+}
+
+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_iio_enum,
+	.set = mma8452_set_power_mode_iio_enum,
+};
+
+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, \
@@ -1204,6 +1239,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) { \
@@ -1225,6 +1261,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.11.0

Re: [PATCH] iio: mma8452: add power_mode sysfs configuration

[harinath Nampally]

Hi Martin,

Thanks for publishing the patch.
I will work on it, but unfortunately I can't promise anything before 11/27.

Thanks,
Harinath

On Mon, Nov 6, 2017 at 2:19 AM, Martin Kepplinger <martink@posteo.de> wrote:
> This adds the power_mode sysfs interface to the device as documented in
> sysfs-bus-iio.
>
> ---
>
> Note that I explicitely don't sign off on this.
>
> This is a starting point for anybody who can test it and check for correct
> API usage, and ABI correctness, as documented in Documentation/ABI/testing/sys-bus-iio
> (grep it for "power_mode"). The ABI doc probably would need an addition
> too, if the 4 power modes here seem generally useful (there are only
>  2 listed there)!
>
> So, if you can test this, feel free to set up a proper patch or
> two, and I'm happy to review.
>
> Please note that this patch is quite old. It really should be that simple
> as far as my understanding back then. We always list the available frequencies
> of the given power mode we are in, for example, already, and everything
> basically is in place except for the user interface.
>
> thanks
>                             martin
>
>
>
>  drivers/iio/accel/mma8452.c | 37 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 37 insertions(+)
>
> diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
> index bfd4bc806fc2..640bbd9872ab 100644
> --- a/drivers/iio/accel/mma8452.c
> +++ b/drivers/iio/accel/mma8452.c
> @@ -1166,6 +1166,41 @@ static struct attribute_group mma8452_event_attribute_group = {
>         .attrs = mma8452_event_attributes,
>  };
>
> +static const char * const mma8452_power_modes[] = {"normal",
> +                                                  "low_noise_low_power",
> +                                                  "low_noise",
> +                                                  "low_power"};
> +
> +static int mma8452_get_power_mode_iio_enum(struct iio_dev *indio_dev,
> +                                          const struct iio_chan_spec *chan)
> +{
> +       struct mma8452_data *data = iio_priv(indio_dev);
> +
> +       return mma8452_get_power_mode(data);
> +}
> +
> +static int mma8452_set_power_mode_iio_enum(struct iio_dev *indio_dev,
> +                                          const struct iio_chan_spec *chan,
> +                                          unsigned int mode)
> +{
> +       struct mma8452_data *data = iio_priv(indio_dev);
> +
> +       return mma8452_set_power_mode(data, mode);
> +}
> +
> +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_iio_enum,
> +       .set = mma8452_set_power_mode_iio_enum,
> +};
> +
> +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, \
> @@ -1204,6 +1239,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) { \
> @@ -1225,6 +1261,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.11.0
>

Re: [PATCH] iio: mma8452: add power_mode sysfs configuration

[Martin Kepplinger]

Am 09.11.2017 04:19 schrieb harinath Nampally:
> Hi Martin,
> 
> Thanks for publishing the patch.
> I will work on it, but unfortunately I can't promise anything before 
> 11/27.
> 

perfectly fine, this patch has been lying around here for at least a 
year, so there's no
rush.

Re: [PATCH] iio: mma8452: add power_mode sysfs configuration

[Jonathan Cameron]

On Mon, 6 Nov 2017 08:19:58 +0100
Martin Kepplinger <martink@posteo.de> wrote:

> This adds the power_mode sysfs interface to the device as documented in
> sysfs-bus-iio.
> 
> ---
> 
> Note that I explicitely don't sign off on this.
> 
> This is a starting point for anybody who can test it and check for correct
> API usage, and ABI correctness, as documented in Documentation/ABI/testing/sys-bus-iio
> (grep it for "power_mode"). The ABI doc probably would need an addition
> too, if the 4 power modes here seem generally useful (there are only
>  2 listed there)!
> 
> So, if you can test this, feel free to set up a proper patch or
> two, and I'm happy to review.
> 
> Please note that this patch is quite old. It really should be that simple
> as far as my understanding back then. We always list the available frequencies
> of the given power mode we are in, for example, already, and everything
> basically is in place except for the user interface.

Hmm. A lot of devices support something along these lines.  The issue
has always been - how is userspace to figure out what to do with it?
It's all very vague...

Funnily enough - this used to be really common, but is becoming less so
now - presumably because no one was using it much (or maybe I am reading
too much into that ;)

Now the question is whether it can be tied to better defined things?

Here low noise restricts the range to 4g.  Issue is that we don't actually
have writeable _available attributes (which correspond to range in this case).

Low power mode... This one is apparently oversampling.  If possible support
it as that as we have well defined interfaces for that.

Jonathan.
> 
> thanks
>                             martin
> 
> 
> 
>  drivers/iio/accel/mma8452.c | 37 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 37 insertions(+)
> 
> diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
> index bfd4bc806fc2..640bbd9872ab 100644
> --- a/drivers/iio/accel/mma8452.c
> +++ b/drivers/iio/accel/mma8452.c
> @@ -1166,6 +1166,41 @@ static struct attribute_group mma8452_event_attribute_group = {
>  	.attrs = mma8452_event_attributes,
>  };
>  
> +static const char * const mma8452_power_modes[] = {"normal",
> +						   "low_noise_low_power",
> +						   "low_noise",
> +						   "low_power"};
> +
> +static int mma8452_get_power_mode_iio_enum(struct iio_dev *indio_dev,
> +					   const struct iio_chan_spec *chan)
> +{
> +	struct mma8452_data *data = iio_priv(indio_dev);
> +
> +	return mma8452_get_power_mode(data);
> +}
> +
> +static int mma8452_set_power_mode_iio_enum(struct iio_dev *indio_dev,
> +					   const struct iio_chan_spec *chan,
> +					   unsigned int mode)
> +{
> +	struct mma8452_data *data = iio_priv(indio_dev);
> +
> +	return mma8452_set_power_mode(data, mode);
> +}
> +
> +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_iio_enum,
> +	.set = mma8452_set_power_mode_iio_enum,
> +};
> +
> +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, \
> @@ -1204,6 +1239,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) { \
> @@ -1225,6 +1261,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[] = {

Re: [PATCH] iio: mma8452: add power_mode sysfs configuration

[Martin Kepplinger]

On 2017-11-11 01:33, Jonathan Cameron wrote:
> On Mon, 6 Nov 2017 08:19:58 +0100
> Martin Kepplinger <martink@posteo.de> wrote:
> 
>> This adds the power_mode sysfs interface to the device as documented in
>> sysfs-bus-iio.
>>
>> ---
>>
>> Note that I explicitely don't sign off on this.
>>
>> This is a starting point for anybody who can test it and check for correct
>> API usage, and ABI correctness, as documented in Documentation/ABI/testing/sys-bus-iio
>> (grep it for "power_mode"). The ABI doc probably would need an addition
>> too, if the 4 power modes here seem generally useful (there are only
>>  2 listed there)!
>>
>> So, if you can test this, feel free to set up a proper patch or
>> two, and I'm happy to review.
>>
>> Please note that this patch is quite old. It really should be that simple
>> as far as my understanding back then. We always list the available frequencies
>> of the given power mode we are in, for example, already, and everything
>> basically is in place except for the user interface.
> 
> Hmm. A lot of devices support something along these lines.  The issue
> has always been - how is userspace to figure out what to do with it?
> It's all very vague...
> 
> Funnily enough - this used to be really common, but is becoming less so
> now - presumably because no one was using it much (or maybe I am reading
> too much into that ;)
> 
> Now the question is whether it can be tied to better defined things?
> 
> Here low noise restricts the range to 4g.  Issue is that we don't actually
> have writeable _available attributes (which correspond to range in this case).
> 

Does it? Isn't it merely less oversampling.

> Low power mode... This one is apparently oversampling.  If possible support
> it as that as we have well defined interfaces for that.
> 
> Jonathan.

Ah, I remember; the oversampling settings was actually a reason why I
hadn't submitted the patch :) The oversampling API would definitely be
more accurate.

I would like "oversampling" more than this "power_mode" too. For this
driver it would be far more complicated to implement though. I doubt
that it'll be done. power_mode is basically already there implicitely,
and given that there *is* the ABI, we could offer it for free.

But given your concerns, I would strip down this patch to only offer the
already documented "low_noise" and "low_power" modes. It wouldn't be
worth it to extend the ABI just because of this!

Users would have a simple switch if they don't really *want* to know the
details. I think it can be useful to just say "I don't care about power
consuption. Be as accurate as possible" or "I just want this think to
work. Use a little power as possible." Sure it's vage, but would it be
useless?

Re: [PATCH] iio: mma8452: add power_mode sysfs configuration

[harinath Nampally]

Hi Martin,

> But given your concerns, I would strip down this patch to only offer the
> already documented "low_noise" and "low_power" modes. It wouldn't be
> worth it to extend the ABI just because of this!
OK then we can map 'low_noise' to high resolution mode. But I am afraid
I can't test the functionality because I don't have proper instruments to
measure the current draw(in microAmps) accurately.

> I would like "oversampling" more than this "power_mode" too. For this
> driver it would be far more complicated to implement though. I doubt
> that it'll be done. power_mode is basically already there implicitely,
> and given that there *is* the ABI, we could offer it for free.
I think 'oversampling' is already implemented, as I see
'case IIO_CHAN_INFO_OVERSAMPLING_RATIO:'
being handled which is basically setting the all 4 different power modes.
If we also add 'power_mode', I think it would be like having two
different user interfaces for
same functionality. So I don't see much of value adding 'power_mode' as well.
Please correct me if I am wrong.

Thanks,
Harinath

On Sun, Nov 12, 2017 at 7:28 AM, Martin Kepplinger <martink@posteo.de> wrote:
> On 2017-11-11 01:33, Jonathan Cameron wrote:
>> On Mon, 6 Nov 2017 08:19:58 +0100
>> Martin Kepplinger <martink@posteo.de> wrote:
>>
>>> This adds the power_mode sysfs interface to the device as documented in
>>> sysfs-bus-iio.
>>>
>>> ---
>>>
>>> Note that I explicitely don't sign off on this.
>>>
>>> This is a starting point for anybody who can test it and check for correct
>>> API usage, and ABI correctness, as documented in Documentation/ABI/testing/sys-bus-iio
>>> (grep it for "power_mode"). The ABI doc probably would need an addition
>>> too, if the 4 power modes here seem generally useful (there are only
>>>  2 listed there)!
>>>
>>> So, if you can test this, feel free to set up a proper patch or
>>> two, and I'm happy to review.
>>>
>>> Please note that this patch is quite old. It really should be that simple
>>> as far as my understanding back then. We always list the available frequencies
>>> of the given power mode we are in, for example, already, and everything
>>> basically is in place except for the user interface.
>>
>> Hmm. A lot of devices support something along these lines.  The issue
>> has always been - how is userspace to figure out what to do with it?
>> It's all very vague...
>>
>> Funnily enough - this used to be really common, but is becoming less so
>> now - presumably because no one was using it much (or maybe I am reading
>> too much into that ;)
>>
>> Now the question is whether it can be tied to better defined things?
>>
>> Here low noise restricts the range to 4g.  Issue is that we don't actually
>> have writeable _available attributes (which correspond to range in this case).
>>
>
> Does it? Isn't it merely less oversampling.
>
>> Low power mode... This one is apparently oversampling.  If possible support
>> it as that as we have well defined interfaces for that.
>>
>> Jonathan.
>
> Ah, I remember; the oversampling settings was actually a reason why I
> hadn't submitted the patch :) The oversampling API would definitely be
> more accurate.
>
> I would like "oversampling" more than this "power_mode" too. For this
> driver it would be far more complicated to implement though. I doubt
> that it'll be done. power_mode is basically already there implicitely,
> and given that there *is* the ABI, we could offer it for free.
>
> But given your concerns, I would strip down this patch to only offer the
> already documented "low_noise" and "low_power" modes. It wouldn't be
> worth it to extend the ABI just because of this!
>
> Users would have a simple switch if they don't really *want* to know the
> details. I think it can be useful to just say "I don't care about power
> consuption. Be as accurate as possible" or "I just want this think to
> work. Use a little power as possible." Sure it's vage, but would it be
> useless?

Re: [PATCH] iio: mma8452: add power_mode sysfs configuration

[Martin Kepplinger]

Am 14.11.2017 05:56 schrieb harinath Nampally:
> Hi Martin,
> 
>> But given your concerns, I would strip down this patch to only offer 
>> the
>> already documented "low_noise" and "low_power" modes. It wouldn't be
>> worth it to extend the ABI just because of this!
> OK then we can map 'low_noise' to high resolution mode. But I am afraid
> I can't test the functionality because I don't have proper instruments 
> to
> measure the current draw(in microAmps) accurately.
> 
>> I would like "oversampling" more than this "power_mode" too. For this
>> driver it would be far more complicated to implement though. I doubt
>> that it'll be done. power_mode is basically already there implicitely,
>> and given that there *is* the ABI, we could offer it for free.
> I think 'oversampling' is already implemented, as I see
> 'case IIO_CHAN_INFO_OVERSAMPLING_RATIO:'
> being handled which is basically setting the all 4 different power 
> modes.
> If we also add 'power_mode', I think it would be like having two
> different user interfaces for
> same functionality. So I don't see much of value adding 'power_mode' as 
> well.
> Please correct me if I am wrong.
> 
> Thanks,
> Harinath
> 

You're right. I should've looked more closely. oversampling is there and 
seems to
work. No need to blow up this driver or let alone extend an ABI now. 
Let's drop
this patch.

thanks
                                  martin


> On Sun, Nov 12, 2017 at 7:28 AM, Martin Kepplinger <martink@posteo.de> 
> wrote:
>> On 2017-11-11 01:33, Jonathan Cameron wrote:
>>> On Mon, 6 Nov 2017 08:19:58 +0100
>>> Martin Kepplinger <martink@posteo.de> wrote:
>>> 
>>>> This adds the power_mode sysfs interface to the device as documented 
>>>> in
>>>> sysfs-bus-iio.
>>>> 
>>>> ---
>>>> 
>>>> Note that I explicitely don't sign off on this.
>>>> 
>>>> This is a starting point for anybody who can test it and check for 
>>>> correct
>>>> API usage, and ABI correctness, as documented in 
>>>> Documentation/ABI/testing/sys-bus-iio
>>>> (grep it for "power_mode"). The ABI doc probably would need an 
>>>> addition
>>>> too, if the 4 power modes here seem generally useful (there are only
>>>>  2 listed there)!
>>>> 
>>>> So, if you can test this, feel free to set up a proper patch or
>>>> two, and I'm happy to review.
>>>> 
>>>> Please note that this patch is quite old. It really should be that 
>>>> simple
>>>> as far as my understanding back then. We always list the available 
>>>> frequencies
>>>> of the given power mode we are in, for example, already, and 
>>>> everything
>>>> basically is in place except for the user interface.
>>> 
>>> Hmm. A lot of devices support something along these lines.  The issue
>>> has always been - how is userspace to figure out what to do with it?
>>> It's all very vague...
>>> 
>>> Funnily enough - this used to be really common, but is becoming less 
>>> so
>>> now - presumably because no one was using it much (or maybe I am 
>>> reading
>>> too much into that ;)
>>> 
>>> Now the question is whether it can be tied to better defined things?
>>> 
>>> Here low noise restricts the range to 4g.  Issue is that we don't 
>>> actually
>>> have writeable _available attributes (which correspond to range in 
>>> this case).
>>> 
>> 
>> Does it? Isn't it merely less oversampling.
>> 
>>> Low power mode... This one is apparently oversampling.  If possible 
>>> support
>>> it as that as we have well defined interfaces for that.
>>> 
>>> Jonathan.
>> 
>> Ah, I remember; the oversampling settings was actually a reason why I
>> hadn't submitted the patch :) The oversampling API would definitely be
>> more accurate.
>> 
>> I would like "oversampling" more than this "power_mode" too. For this
>> driver it would be far more complicated to implement though. I doubt
>> that it'll be done. power_mode is basically already there implicitely,
>> and given that there *is* the ABI, we could offer it for free.
>> 
>> But given your concerns, I would strip down this patch to only offer 
>> the
>> already documented "low_noise" and "low_power" modes. It wouldn't be
>> worth it to extend the ABI just because of this!
>> 
>> Users would have a simple switch if they don't really *want* to know 
>> the
>> details. I think it can be useful to just say "I don't care about 
>> power
>> consuption. Be as accurate as possible" or "I just want this think to
>> work. Use a little power as possible." Sure it's vage, but would it be
>> useless?