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@xxxxxxxxx> 
> wrote:
> > On 2017-11-11 01:33, Jonathan Cameron wrote:
> > > On Mon, 6 Nov 2017 08:19:58 +0100
> > > Martin Kepplinger <martink@xxxxxxxxx> 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?