[PATCH/RFC] iio: hi8435: do not enable all events by default

[PATCH/RFC] iio: hi8435: do not enable all events by default

[Nikita Yushchenko]

Having all events enabled by default is misleading.
Userspace should explicitly enable events they want to receive.

Signed-off-by: Nikita Yushchenko <nikita.yoush@cogentembedded.com>
---
 drivers/iio/adc/hi8435.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/iio/adc/hi8435.c b/drivers/iio/adc/hi8435.c
index ef5c286c8e67..0331739f235c 100644
--- a/drivers/iio/adc/hi8435.c
+++ b/drivers/iio/adc/hi8435.c
@@ -506,8 +506,6 @@ static int hi8435_probe(struct spi_device *spi)
 	idev->channels		= hi8435_channels;
 	idev->num_channels	= ARRAY_SIZE(hi8435_channels);
 
-	/* unmask all events */
-	priv->event_scan_mask = ~(0);
 	/*
 	 * There is a restriction in the chip - the hysteresis can not be odd.
 	 * If the hysteresis is set to odd value then chip gets into lock state
-- 
2.11.0

Re: [PATCH/RFC] iio: hi8435: do not enable all events by default

[Jonathan Cameron]

On Tue, 23 May 2017 11:08:30 +0300
Nikita Yushchenko <nikita.yoush@cogentembedded.com> wrote:

> Having all events enabled by default is misleading.
> Userspace should explicitly enable events they want to receive.
> 
> Signed-off-by: Nikita Yushchenko <nikita.yoush@cogentembedded.com>
I agree in principle, but this is a userspace ABI change.  Sadly we
can't do it with out risking breaking userspace code...

One of those we should have caught in review, but now it's there
we can't actually do anything about it unless we are absolutely
sure no one will notice!

Jonathan
> ---
>  drivers/iio/adc/hi8435.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/iio/adc/hi8435.c b/drivers/iio/adc/hi8435.c
> index ef5c286c8e67..0331739f235c 100644
> --- a/drivers/iio/adc/hi8435.c
> +++ b/drivers/iio/adc/hi8435.c
> @@ -506,8 +506,6 @@ static int hi8435_probe(struct spi_device *spi)
>  	idev->channels		= hi8435_channels;
>  	idev->num_channels	= ARRAY_SIZE(hi8435_channels);
>  
> -	/* unmask all events */
> -	priv->event_scan_mask = ~(0);
>  	/*
>  	 * There is a restriction in the chip - the hysteresis can not be odd.
>  	 * If the hysteresis is set to odd value then chip gets into lock state

Re: [PATCH/RFC] iio: hi8435: do not enable all events by default

[Nikita Yushchenko]

24.05.2017 22:27, Jonathan Cameron wrote:
> On Tue, 23 May 2017 11:08:30 +0300
> Nikita Yushchenko <nikita.yoush@cogentembedded.com> wrote:
> 
>> Having all events enabled by default is misleading.
>> Userspace should explicitly enable events they want to receive.
>>
>> Signed-off-by: Nikita Yushchenko <nikita.yoush@cogentembedded.com>
> I agree in principle, but this is a userspace ABI change.  Sadly we
> can't do it with out risking breaking userspace code...
> 
> One of those we should have caught in review, but now it's there
> we can't actually do anything about it unless we are absolutely
> sure no one will notice!

I see your point.

Still, isn't there subsystem-level default that all events are disabled
by default?  If such, then current hi8435 state breaks subsystem-level
rules, which is a [userspace-visible] bug.  I'm not sure how far should
we go in bug compatibility.

One crazy idea could be - make default selectable via device tree (with
default set to all-enabled to keep bug-compatibility).  But perhaps
that's over-reaction.

Re: [PATCH/RFC] iio: hi8435: do not enable all events by default

[Jonathan Cameron]

On Thu, 25 May 2017 08:47:47 +0300
Nikita Yushchenko <nikita.yoush@cogentembedded.com> wrote:

> 24.05.2017 22:27, Jonathan Cameron wrote:
> > On Tue, 23 May 2017 11:08:30 +0300
> > Nikita Yushchenko <nikita.yoush@cogentembedded.com> wrote:
> >   
> >> Having all events enabled by default is misleading.
> >> Userspace should explicitly enable events they want to receive.
> >>
> >> Signed-off-by: Nikita Yushchenko <nikita.yoush@cogentembedded.com>  
> > I agree in principle, but this is a userspace ABI change.  Sadly we
> > can't do it with out risking breaking userspace code...
> > 
> > One of those we should have caught in review, but now it's there
> > we can't actually do anything about it unless we are absolutely
> > sure no one will notice!  
> 
> I see your point.
> 
> Still, isn't there subsystem-level default that all events are disabled
> by default?  If such, then current hi8435 state breaks subsystem-level
> rules, which is a [userspace-visible] bug.  I'm not sure how far should
> we go in bug compatibility.
It is indeed the subsystem default (as much as we have one)

This is a moderately obscure chip for linux systems, do we have a good handle
on where it is being used - i.e. are most of the devices under control of
people we can discuss this with?
> 
> One crazy idea could be - make default selectable via device tree (with
> default set to all-enabled to keep bug-compatibility).  But perhaps
> that's over-reaction.
Yeah, wouldn't fly with the devicetree binding maintainers..

Jonathan

Re: Fw: [PATCH/RFC] iio: hi8435: do not enable all events by default

[Chris Healy]

On Sun, May 28, 2017 at 9:50 AM, Chris Healy <Chris.Healy@zii.aero> wrote:
>
>
>
> ________________________________
> From: Jonathan Cameron <jic23@kernel.org>
> Sent: Sunday, May 28, 2017 8:48 AM
> To: Nikita Yoush
> Cc: Hartmut Knaack; Lars-Peter Clausen; Peter Meerwald-Stadler; Sanchayan
> Maity; Gregor Boirie; Matt Ranostay; linux-iio@vger.kernel.org;
> linux-kernel@vger.kernel.org; Chris Healy; Jeff White; Vladimir Barinov
> Subject: Re: [PATCH/RFC] iio: hi8435: do not enable all events by default
>
> On Thu, 25 May 2017 08:47:47 +0300
> Nikita Yushchenko <nikita.yoush@cogentembedded.com> wrote:
>
>> 24.05.2017 22:27, Jonathan Cameron wrote:
>> > On Tue, 23 May 2017 11:08:30 +0300
>> > Nikita Yushchenko <nikita.yoush@cogentembedded.com> wrote:
>> >
>> >> Having all events enabled by default is misleading.
>> >> Userspace should explicitly enable events they want to receive.
>> >>
>> >> Signed-off-by: Nikita Yushchenko <nikita.yoush@cogentembedded.com>
>> > I agree in principle, but this is a userspace ABI change.  Sadly we
>> > can't do it with out risking breaking userspace code...
>> >
>> > One of those we should have caught in review, but now it's there
>> > we can't actually do anything about it unless we are absolutely
>> > sure no one will notice!
>>
>> I see your point.
>>
>> Still, isn't there subsystem-level default that all events are disabled
>> by default?  If such, then current hi8435 state breaks subsystem-level
>> rules, which is a [userspace-visible] bug.  I'm not sure how far should
>> we go in bug compatibility.
> It is indeed the subsystem default (as much as we have one)
>
> This is a moderately obscure chip for linux systems, do we have a good
> handle
> on where it is being used - i.e. are most of the devices under control of
> people we can discuss this with?

The company I work for funded the initial work to get support for the
this Holt HI-8435 upstream.  We are presently using this driver and
can accommodate this proposed userspace ABI change as we can adjust
userspace if the ABI changes.  Unfortunately, this doesn't answer the
question of the drivers adoption elsewhere.

>>
>> One crazy idea could be - make default selectable via device tree (with
>> default set to all-enabled to keep bug-compatibility).  But perhaps
>> that's over-reaction.
> Yeah, wouldn't fly with the devicetree binding maintainers..
>
> Jonathan
>
> ________________________________
>
>
> This email and any files transmitted with it are confidential & proprietary
> to Zodiac Inflight Innovations. This information is intended solely for the
> use of the individual or entity to which it is addressed. Access or
> transmittal of the information contained in this e-mail, in full or in part,
> to any other organization or persons is not authorized.

Re: [PATCH/RFC] iio: hi8435: do not enable all events by default

[Nikita Yushchenko]

>> Still, isn't there subsystem-level default that all events are disabled
>> by default?  If such, then current hi8435 state breaks subsystem-level
>> rules, which is a [userspace-visible] bug.  I'm not sure how far should
>> we go in bug compatibility.
>
> It is indeed the subsystem default (as much as we have one)
> 
> This is a moderately obscure chip for linux systems, do we have a good handle
> on where it is being used - i.e. are most of the devices under control of
> people we can discuss this with?

Company I work with, uses this chip in several boards; what they need is
a service that monitors all connected chip's outputs and detects
changes.  They originally wanted gpio-style access to use with userspace
polling, and were not pleased with entire IIO thing. However it's
important for them to minimize required kernel patches against mainline,
thus if mainline supports this chip as IIO device that's ok for them.

Questions like default event enable state has little practical impact.
It's more about keeping architecture clean.

Nikita

Re: [PATCH/RFC] iio: hi8435: do not enable all events by default

[Jonathan Cameron]

On Mon, 29 May 2017 08:40:26 +0300
Nikita Yushchenko <nikita.yoush@cogentembedded.com> wrote:

> >> Still, isn't there subsystem-level default that all events are disabled
> >> by default?  If such, then current hi8435 state breaks subsystem-level
> >> rules, which is a [userspace-visible] bug.  I'm not sure how far should
> >> we go in bug compatibility.  
> >
> > It is indeed the subsystem default (as much as we have one)
> > 
> > This is a moderately obscure chip for linux systems, do we have a good handle
> > on where it is being used - i.e. are most of the devices under control of
> > people we can discuss this with?  
> 
> Company I work with, uses this chip in several boards; what they need is
> a service that monitors all connected chip's outputs and detects
> changes.  They originally wanted gpio-style access to use with userspace
> polling, and were not pleased with entire IIO thing. However it's
> important for them to minimize required kernel patches against mainline,
> thus if mainline supports this chip as IIO device that's ok for them.
Was always an odd corner case - and I'll admit it wasn't one that made
me very comfortable either. One option would be to put a GPIO bridge
driver in place so that both interfaces are available.  I know this is
something Linus and I have thought about in the past for various
use cases.
> 
> Questions like default event enable state has little practical impact.
> It's more about keeping architecture clean.
I'm going to go with not changing this, even though it matters little to
either of you.  Simply because we clearly do have a non trivial number of
users and this would be ABI breakage.

Jonathan

> 
> Nikita
> --
> 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

Re: [PATCH/RFC] iio: hi8435: do not enable all events by default

[Linus Walleij]

On Mon, May 29, 2017 at 7:40 AM, Nikita Yushchenko
<nikita.yoush@cogentembedded.com> wrote:

> They originally wanted gpio-style access to use with userspace
> polling, and were not pleased with entire IIO thing.

The GPIO userspace ABI supports events with timestamps.
tools/gpio/gpio-event-mon.c

Yours,
Linus Walleij