Re: [PATCH] iio: chemical: sps30: allow changing self cleaning period

[Jonathan Cameron <jic23@jic23.retrosnub.co.uk>]

On Sun, 6 Jan 2019 11:57:31 +0100
Tomasz Duszynski <tduszyns@gmail.com> wrote:

> On Sat, Jan 05, 2019 at 04:54:47PM +0000, Jonathan Cameron wrote:
> > On Wed, 26 Dec 2018 20:30:35 +0100
> > Tomasz Duszynski <tduszyns@gmail.com> wrote:
> >  
> > > Sensor can periodically trigger self cleaning. Period can be changed by
> > > writing a new value to a dedicated attribute. Upon attribute read
> > > triplet representing respectively current, minimum and maximum period is
> > > returned.
> > >
> > > Signed-off-by: Tomasz Duszynski <tduszyns@gmail.com>  
> >
> > Code is fine, but to end up with predictable generic interface
> > that fits with the rest of IIO we need a different userspace interface I think...
> >
> > See below.
> >
> > Jonathan  
> > > ---
> > >  Documentation/ABI/testing/sysfs-bus-iio-sps30 |  11 ++
> > >  drivers/iio/chemical/sps30.c                  | 134 +++++++++++++++---
> > >  2 files changed, 127 insertions(+), 18 deletions(-)
> > >
> > > diff --git a/Documentation/ABI/testing/sysfs-bus-iio-sps30 b/Documentation/ABI/testing/sysfs-bus-iio-sps30
> > > index e7ce2c57635e..d83d9192a3e0 100644
> > > --- a/Documentation/ABI/testing/sysfs-bus-iio-sps30
> > > +++ b/Documentation/ABI/testing/sysfs-bus-iio-sps30
> > > @@ -6,3 +6,14 @@ Description:
> > >  		Writing 1 starts sensor self cleaning. Internal fan accelerates
> > >  		to its maximum speed and keeps spinning for about 10 seconds in
> > >  		order to blow out accumulated dust.
> > > +
> > > +What:		/sys/bus/iio/devices/iio:deviceX/cleaning_interval
> > > +Date:		December 2018
> > > +KernelVersion:	4.22
> > > +Contact:	linux-iio@vger.kernel.org
> > > +Description:
> > > +		Sensor is capable of triggering self cleaning periodically.
> > > +		Period can be changed by writing a new value here. Upon reading
> > > +		three values are returned representing respectively current,
> > > +		minimum and maximum period. All values are in seconds.
> > > +		Writing 0 here disables periodical self cleaning entirely.  
> > Hmm. The issue here is that the value isn't:
> > 1. Intuitive
> > 2. A single value (requirement for sysfs interfaces - we stretch the meaning
> > a bit where there the values really don't have any meaning on their own but
> > that isn't true here).
> >  
> 
> This is not uncommon in sysfs for attributes to list both available
> range and current value. Hence I though I could try to sneak that here.
> 
> Turned out that without luck ;).
Indeed, in general some drivers an subsystems do that.   We decided not
to a long time ago.  Mainly because we already had an interface for
the value itself without the extras and so couldn't change it without
breaking the userspace ABI.

Even though we are looking at new interfaces, we need to be consistent!

> 
> > We have a syntax in IIO use when we want to specify a range of acceptable
> > values.  It's done for 'core' attributes using the available callback
> > (I need to write some proper docs for it though...)
> >
> > cleaning_period - the actual value.
> > cleaning_period_available
> > The range version (rather than list of values) is formatted
> > by iio_format_avail_range which generates [min step max]
> >
> > Please do something along those lines for this control as well.
> >  
> 
> Agree.
> 

Thanks,

Jonathan