iio: light: apds9960: Fix iio_event_spec structures

  There is only one interrupt enable option for both ALS low and high
thresholds, and one for both Proximity low and high thresholds.

Signed-off-by: Subhajit Ghosh <subhajit.ghosh@vixtechnology.com>
---
 drivers/iio/light/apds9960.c | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)
  

On Thu, Nov 10, 2022 at 6:13 PM Subhajit Ghosh
<subhajit.ghosh@vixtechnology.com> wrote:
>
> There is only one interrupt enable option for both ALS low and high
> thresholds, and one for both Proximity low and high thresholds.
>
> Signed-off-by: Subhajit Ghosh <subhajit.ghosh@vixtechnology.com>
> ---
>  drivers/iio/light/apds9960.c | 20 ++++++++++++--------
>  1 file changed, 12 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/iio/light/apds9960.c b/drivers/iio/light/apds9960.c
> index 4141c0fa7bc4..df9ccbcf0ffe 100644
> --- a/drivers/iio/light/apds9960.c
> +++ b/drivers/iio/light/apds9960.c
> @@ -223,14 +223,16 @@ static const struct iio_event_spec apds9960_pxs_event_spec[] = {
>         {
>                 .type = IIO_EV_TYPE_THRESH,
>                 .dir = IIO_EV_DIR_RISING,
> -               .mask_separate = BIT(IIO_EV_INFO_VALUE) |
> -                       BIT(IIO_EV_INFO_ENABLE),
> +               .mask_separate = BIT(IIO_EV_INFO_VALUE),

Probably more concise to use the following, and you won't need to add
an additional item to the structs.

   .mask_separate = BIT(IIO_EV_INFO_VALUE),
   .mask_shared_by_type = BIT(IIO_EV_INFO_ENABLE),

>         },
>         {
>                 .type = IIO_EV_TYPE_THRESH,
>                 .dir = IIO_EV_DIR_FALLING,
> -               .mask_separate = BIT(IIO_EV_INFO_VALUE) |
> -                       BIT(IIO_EV_INFO_ENABLE),
> +               .mask_separate = BIT(IIO_EV_INFO_VALUE),
> +       },
> +       {
> +               .type = IIO_EV_TYPE_THRESH,
> +               .mask_separate = BIT(IIO_EV_INFO_ENABLE),

Same here.

- Matt

>         },
>  };
>
> @@ -238,14 +240,16 @@ static const struct iio_event_spec apds9960_als_event_spec[] = {
>         {
>                 .type = IIO_EV_TYPE_THRESH,
>                 .dir = IIO_EV_DIR_RISING,
> -               .mask_separate = BIT(IIO_EV_INFO_VALUE) |
> -                       BIT(IIO_EV_INFO_ENABLE),
> +               .mask_separate = BIT(IIO_EV_INFO_VALUE),
>         },
>         {
>                 .type = IIO_EV_TYPE_THRESH,
>                 .dir = IIO_EV_DIR_FALLING,
> -               .mask_separate = BIT(IIO_EV_INFO_VALUE) |
> -                       BIT(IIO_EV_INFO_ENABLE),
> +               .mask_separate = BIT(IIO_EV_INFO_VALUE),
> +       },
> +       {
> +               .type = IIO_EV_TYPE_THRESH,
> +               .mask_separate = BIT(IIO_EV_INFO_ENABLE),
>         },
>  };
>
> --
> 2.34.1
>
>
> --
> This email is confidential. If you have received this email in error please
> notify us immediately by return email and delete this email and any
> attachments. Vix accepts no liability for any damage caused by this email
> or any attachments due to viruses, interference, interception, corruption
> or unauthorised access.
  

>>                 .type = IIO_EV_TYPE_THRESH,
>>                 .dir = IIO_EV_DIR_RISING,
>> -               .mask_separate = BIT(IIO_EV_INFO_VALUE) |
>> -                       BIT(IIO_EV_INFO_ENABLE),
>> +               .mask_separate = BIT(IIO_EV_INFO_VALUE),
> 
> Probably more concise to use the following, and you won't need to add
> an additional item to the structs.
> 
>    .mask_separate = BIT(IIO_EV_INFO_VALUE),
>    .mask_shared_by_type = BIT(IIO_EV_INFO_ENABLE),
> 

Above is the first thing I tried.

Current implementation:

root@stm32mp1:~# ls -1 /sys/bus/iio/devices/iio:device0/events/
in_intensity_clear_thresh_falling_en
in_intensity_clear_thresh_falling_value
in_intensity_clear_thresh_rising_en
in_intensity_clear_thresh_rising_value

in_proximity_thresh_falling_en
in_proximity_thresh_falling_value
in_proximity_thresh_rising_en
in_proximity_thresh_rising_value


First method (Which you are suggesting):
.mask_separate = BIT(IIO_EV_INFO_VALUE),
.mask_shared_by_type = BIT(IIO_EV_INFO_ENABLE),

root@stm32mp1:~# ls -1 /sys/bus/iio/devices/iio:device0/events/
in_intensity_clear_thresh_falling_value
in_intensity_clear_thresh_rising_value
in_intensity_thresh_falling_en
in_intensity_thresh_rising_en

The above says all channels with with the type IIO_INTENSITY has
the same enable but we require this particular channel (in_intensity_clear)
regardless of direction to have the same enable.
Using mask_shared_by_dir and mask_shared_by_all does not provide the logical
attribute name. 


This patch provides the below:

root@stm32mp1:~# ls -1 /sys/bus/iio/devices/iio:device0/events/
in_intensity_clear_thresh_either_en
in_intensity_clear_thresh_falling_value
in_intensity_clear_thresh_rising_value

in_proximity_thresh_either_en
in_proximity_thresh_falling_value
in_proximity_thresh_rising_value

Verified using iio_event_monitor:

root@stm32mp1:~# ./iio_event_monitor /dev/iio:device0
Event: time: 1647143384807582753, type: proximity, channel: 0, evtype: thresh, direction: either


Regards
Subhajit Ghosh
  

On Thu, Nov 10, 2022 at 10:45 PM Subhajit Ghosh
<subhajit.ghosh@vixtechnology.com> wrote:
>
>
> >>                 .type = IIO_EV_TYPE_THRESH,
> >>                 .dir = IIO_EV_DIR_RISING,
> >> -               .mask_separate = BIT(IIO_EV_INFO_VALUE) |
> >> -                       BIT(IIO_EV_INFO_ENABLE),
> >> +               .mask_separate = BIT(IIO_EV_INFO_VALUE),
> >
> > Probably more concise to use the following, and you won't need to add
> > an additional item to the structs.
> >
> >    .mask_separate = BIT(IIO_EV_INFO_VALUE),
> >    .mask_shared_by_type = BIT(IIO_EV_INFO_ENABLE),
> >
>
> Above is the first thing I tried.
>
> Current implementation:
>
> root@stm32mp1:~# ls -1 /sys/bus/iio/devices/iio:device0/events/
> in_intensity_clear_thresh_falling_en
> in_intensity_clear_thresh_falling_value
> in_intensity_clear_thresh_rising_en
> in_intensity_clear_thresh_rising_value
>
> in_proximity_thresh_falling_en
> in_proximity_thresh_falling_value
> in_proximity_thresh_rising_en
> in_proximity_thresh_rising_value
>
>
> First method (Which you are suggesting):
> .mask_separate = BIT(IIO_EV_INFO_VALUE),
> .mask_shared_by_type = BIT(IIO_EV_INFO_ENABLE),
>
> root@stm32mp1:~# ls -1 /sys/bus/iio/devices/iio:device0/events/
> in_intensity_clear_thresh_falling_value
> in_intensity_clear_thresh_rising_value
> in_intensity_thresh_falling_en
> in_intensity_thresh_rising_en
>
> The above says all channels with with the type IIO_INTENSITY has
> the same enable but we require this particular channel (in_intensity_clear)
> regardless of direction to have the same enable.
> Using mask_shared_by_dir and mask_shared_by_all does not provide the logical
> attribute name.
>
>
> This patch provides the below:
>
> root@stm32mp1:~# ls -1 /sys/bus/iio/devices/iio:device0/events/
> in_intensity_clear_thresh_either_en
> in_intensity_clear_thresh_falling_value
> in_intensity_clear_thresh_rising_value
>
> in_proximity_thresh_either_en
> in_proximity_thresh_falling_value
> in_proximity_thresh_rising_value
>
> Verified using iio_event_monitor:
>
> root@stm32mp1:~# ./iio_event_monitor /dev/iio:device0
> Event: time: 1647143384807582753, type: proximity, channel: 0, evtype: thresh, direction: either
>

Hmm maybe Jonathan will have some feedback on this (and if it is okay
to break the ABI interface). Been awhile since I've touched
this driver and a little rusty on iio events.  But I am guessing your
method makes sense since the event(s) has direction and a type, and
can't just have one of the .mask_shared_by_dir and mask_shared_by_type.

In any case:

Reviewed-by: Matt Ranostay <matt.ranostay@konsulko.com>


- Matt

>
> Regards
> Subhajit Ghosh
>
> --
> This email is confidential. If you have received this email in error please
> notify us immediately by return email and delete this email and any
> attachments. Vix accepts no liability for any damage caused by this email
> or any attachments due to viruses, interference, interception, corruption
> or unauthorised access.
  

>>
> 
> Hmm maybe Jonathan will have some feedback on this (and if it is okay
> to break the ABI interface). Been awhile since I've touched
> this driver and a little rusty on iio events.  But I am guessing your
> method makes sense since the event(s) has direction and a type, and
> can't just have one of the .mask_shared_by_dir and mask_shared_by_type.
> 
> In any case:
> 
> Reviewed-by: Matt Ranostay <matt.ranostay@konsulko.com>
> 

Thank you so much for your time and review Matt.

Apologies for the company disclaimer message which auto generates after
the message body. I am still working on proper submission skills.  

Regards,
Subhajit Ghosh
  

On Fri, 11 Nov 2022 10:50:35 +0800
Matt Ranostay <matt.ranostay@konsulko.com> wrote:

> On Thu, Nov 10, 2022 at 10:45 PM Subhajit Ghosh
> <subhajit.ghosh@vixtechnology.com> wrote:
> >
> >  
> > >>                 .type = IIO_EV_TYPE_THRESH,
> > >>                 .dir = IIO_EV_DIR_RISING,
> > >> -               .mask_separate = BIT(IIO_EV_INFO_VALUE) |
> > >> -                       BIT(IIO_EV_INFO_ENABLE),
> > >> +               .mask_separate = BIT(IIO_EV_INFO_VALUE),  
> > >
> > > Probably more concise to use the following, and you won't need to add
> > > an additional item to the structs.
> > >
> > >    .mask_separate = BIT(IIO_EV_INFO_VALUE),
> > >    .mask_shared_by_type = BIT(IIO_EV_INFO_ENABLE),
> > >  
> >
> > Above is the first thing I tried.
> >
> > Current implementation:
> >
> > root@stm32mp1:~# ls -1 /sys/bus/iio/devices/iio:device0/events/
> > in_intensity_clear_thresh_falling_en
> > in_intensity_clear_thresh_falling_value
> > in_intensity_clear_thresh_rising_en
> > in_intensity_clear_thresh_rising_value
> >
> > in_proximity_thresh_falling_en
> > in_proximity_thresh_falling_value
> > in_proximity_thresh_rising_en
> > in_proximity_thresh_rising_value
> >
> >
> > First method (Which you are suggesting):
> > .mask_separate = BIT(IIO_EV_INFO_VALUE),
> > .mask_shared_by_type = BIT(IIO_EV_INFO_ENABLE),
> >
> > root@stm32mp1:~# ls -1 /sys/bus/iio/devices/iio:device0/events/
> > in_intensity_clear_thresh_falling_value
> > in_intensity_clear_thresh_rising_value
> > in_intensity_thresh_falling_en
> > in_intensity_thresh_rising_en
> >
> > The above says all channels with with the type IIO_INTENSITY has
> > the same enable but we require this particular channel (in_intensity_clear)
> > regardless of direction to have the same enable.
> > Using mask_shared_by_dir and mask_shared_by_all does not provide the logical
> > attribute name.
> >
> >
> > This patch provides the below:
> >
> > root@stm32mp1:~# ls -1 /sys/bus/iio/devices/iio:device0/events/
> > in_intensity_clear_thresh_either_en
> > in_intensity_clear_thresh_falling_value
> > in_intensity_clear_thresh_rising_value
> >
> > in_proximity_thresh_either_en
> > in_proximity_thresh_falling_value
> > in_proximity_thresh_rising_value
> >
> > Verified using iio_event_monitor:
> >
> > root@stm32mp1:~# ./iio_event_monitor /dev/iio:device0
> > Event: time: 1647143384807582753, type: proximity, channel: 0, evtype: thresh, direction: either
> >  
> 
> Hmm maybe Jonathan will have some feedback on this (and if it is okay
> to break the ABI interface). Been awhile since I've touched
> this driver and a little rusty on iio events.  But I am guessing your
> method makes sense since the event(s) has direction and a type, and
> can't just have one of the .mask_shared_by_dir and mask_shared_by_type.
> 
> In any case:
> 
> Reviewed-by: Matt Ranostay <matt.ranostay@konsulko.com>

Hmm.  Given that event enables often cover a couple of different things
(as done here) it isn't unknown for those to not be as easily covered
as you have done.  As such, we have drivers were the ABI allows for
enabling one event to end up enabling several others (even though they
have separate enable attributes).  It's always been permitted for one
IIO attribute write to have an effect on other attributes simply because
we can't represent all dependencies.

Now the bigger complexity / surprise here is the return of the either
direction in response to enabling either rising or falling. 
That is going to rather surprise your average writer of userspace code.

So patch covers what we should definitely have had in the first place.
Hence it's a question of risk of someone running code that will be affected
by the ABI change.  One of those fingers crossed moments...

Jonathan

> 
> 
> - Matt
> 
> >
> > Regards
> > Subhajit Ghosh
> >
> > --
> > This email is confidential. If you have received this email in error please
> > notify us immediately by return email and delete this email and any
> > attachments. Vix accepts no liability for any damage caused by this email
> > or any attachments due to viruses, interference, interception, corruption
> > or unauthorised access.
  

> Hmm.  Given that event enables often cover a couple of different things
> (as done here) it isn't unknown for those to not be as easily covered
> as you have done.  As such, we have drivers were the ABI allows for
> enabling one event to end up enabling several others (even though they
> have separate enable attributes).  It's always been permitted for one
> IIO attribute write to have an effect on other attributes simply because
> we can't represent all dependencies.
> 
> Now the bigger complexity / surprise here is the return of the either
> direction in response to enabling either rising or falling. 
> That is going to rather surprise your average writer of userspace cod This is where the inconsistency was found. When an ALS threshold rising 
value was given and as soon as it was enabled, interrupts started firing
in low light conditions as there was some value present in the ALS falling 
threshold(reset value is not defined in the datasheet for this register), 
but falling threshold value was neither fed nor enabled!

> So patch covers what we should definitely have had in the first place.
> Hence it's a question of risk of someone running code that will be affected
> by the ABI change.  One of those fingers crossed moments...
I understand that breaking existing userspace applications is not the best
thing to do.

> 
> Jonathan

Thank you for your time and comments.

Regards,
Subhajit Ghosh
  

On Fri, 11 Nov 2022 21:44:48 +0800
Subhajit Ghosh <subhajit.ghosh@vixtechnology.com> wrote:

> > Hmm.  Given that event enables often cover a couple of different things
> > (as done here) it isn't unknown for those to not be as easily covered
> > as you have done.  As such, we have drivers were the ABI allows for
> > enabling one event to end up enabling several others (even though they
> > have separate enable attributes).  It's always been permitted for one
> > IIO attribute write to have an effect on other attributes simply because
> > we can't represent all dependencies.
> > 
> > Now the bigger complexity / surprise here is the return of the either
> > direction in response to enabling either rising or falling. 
> > That is going to rather surprise your average writer of userspace cod This is where the inconsistency was found. When an ALS threshold rising   
> value was given and as soon as it was enabled, interrupts started firing
> in low light conditions as there was some value present in the ALS falling 
> threshold(reset value is not defined in the datasheet for this register), 
> but falling threshold value was neither fed nor enabled!
> 
> > So patch covers what we should definitely have had in the first place.
> > Hence it's a question of risk of someone running code that will be affected
> > by the ABI change.  One of those fingers crossed moments...  
> I understand that breaking existing userspace applications is not the best
> thing to do.

Given the risks around this one, I'm going to apply it to the togreg
branch of iio.git which is lined up for next merge cycle.  That should
in theory provide more time before it hits upstream / gets back ported
to stable releases for people to notice the change.

Hopefully no one will though!

Applied to the togreg branch of iio.git and pushed out initially as testing
to let 0-day poke at it.

Thanks,

Jonathan

> 
> > 
> > Jonathan  
> 
> Thank you for your time and comments.
> 
> Regards,
> Subhajit Ghosh
>