Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1647776 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH/RFC] iio: hi8435: do not enable all events by default Nikita Yushchenko <nikita.yoush@cogentembedded.com> - 2017-05-23 10:20 +0200
    Re: [PATCH/RFC] iio: hi8435: do not enable all events by default Jonathan Cameron <jic23@kernel.org> - 2017-05-24 21:30 +0200
      Re: [PATCH/RFC] iio: hi8435: do not enable all events by default Nikita Yushchenko <nikita.yoush@cogentembedded.com> - 2017-05-25 07:50 +0200
        Re: [PATCH/RFC] iio: hi8435: do not enable all events by default Jonathan Cameron <jic23@kernel.org> - 2017-05-28 17:50 +0200
          Re: Fw: [PATCH/RFC] iio: hi8435: do not enable all events by default Chris Healy <cphealy@gmail.com> - 2017-05-28 19:10 +0200
          Re: [PATCH/RFC] iio: hi8435: do not enable all events by default Nikita Yushchenko <nikita.yoush@cogentembedded.com> - 2017-05-29 07:50 +0200
            Re: [PATCH/RFC] iio: hi8435: do not enable all events by default Jonathan Cameron <jic23@kernel.org> - 2017-06-03 11:10 +0200

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

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

[toc] | [next] | [standalone]

#1649884

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

[toc] | [prev] | [next] | [standalone]

#1650191

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.

[toc] | [prev] | [next] | [standalone]

#1652166

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

[toc] | [prev] | [next] | [standalone]

#1652181

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.

[toc] | [prev] | [next] | [standalone]

#1652240

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

[toc] | [prev] | [next] | [standalone]

#1656761

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web