[PATCH 0/2] iio: sx9310: Add debouncer-depth parameters

[PATCH 0/2] iio: sx9310: Add debouncer-depth parameters

[Gwendal Grignou]

Semtech SX9310 SAR sensor has a debouncer filter: only when N
measurements are above/below the far/close threshold an event is
sent to the host.
By default the debouncer is set to 2 events for the close to far
transition and 1 event (no debounce) for far to close.
It is a balance speed of detection and false positive avoidance.

On some chromebooks, the debouncer is set to a larger number.

This patch applies on top of commit 103d6ec82676 ("iio: sx9310: Support ACPI properties")

Gwendal Grignou (2):
  dt-bindings: iio: sx9310: Add close/far debouncer strength
  iio: sx9310: Add debouncer-depth parameters

 .../iio/proximity/semtech,sx9310.yaml         | 18 ++++++++++++++++
 drivers/iio/proximity/sx9310.c                | 21 ++++++++++++++++++-
 2 files changed, 38 insertions(+), 1 deletion(-)

-- 
2.31.0.291.g576ba9dcdaf-goog

[PATCH 1/2] dt-bindings: iio: sx9310: Add close/far debouncer strength

[Gwendal Grignou]

Debouncer are used to prevent false positive: only when N measurement
are above/below the far/close threshold an event is sent to the host.

Define 2 new entries: a close and far debouncer.

Signed-off-by: Gwendal Grignou <gwendal@chromium.org>
---
 .../bindings/iio/proximity/semtech,sx9310.yaml | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/Documentation/devicetree/bindings/iio/proximity/semtech,sx9310.yaml b/Documentation/devicetree/bindings/iio/proximity/semtech,sx9310.yaml
index ccfb163f3d341..f2656f87e2c44 100644
--- a/Documentation/devicetree/bindings/iio/proximity/semtech,sx9310.yaml
+++ b/Documentation/devicetree/bindings/iio/proximity/semtech,sx9310.yaml
@@ -97,6 +97,24 @@ properties:
       UINT_MAX (4294967295) represents infinite. Other values
       represent 1-1/N.
 
+  semtech,close-debouncer-depth:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    enum: [0, 2, 4, 8]
+    default: 0
+    description:
+      Number of samples need to trigger a transition from far to close event.
+      0 disables the debouncer, N is the number of samples needed under the
+      proximity threshold to trigger an event.
+
+  semtech,far-debouncer-depth:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    enum: [0, 2, 4, 8]
+    default: 0
+    description:
+      Number of samples need to trigger a transition from close to far event.
+      0 disables the debouncer, N is the number of samples needed above the
+      proximity threshold to trigger an event.
+
 required:
   - compatible
   - reg
-- 
2.31.0.291.g576ba9dcdaf-goog

[PATCH 2/2] iio: sx9310: Add debouncer-depth parameters

[Gwendal Grignou]

Semtech SX9310 SAR sensor has a debouncer filter: only when N
measurements are above/below the far/close threshold an event is
sent to the host.
By default the debouncer is set to 2 events for the close to far
transition and 1 event (no debounce) for far to close.
It is a balance speed of detection and false positive avoidance.

On some chromebooks, the debouncer is set to a larger number.

Signed-off-by: Gwendal Grignou <gwendal@chromium.org>
---
 drivers/iio/proximity/sx9310.c | 21 ++++++++++++++++++++-
 1 file changed, 20 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/proximity/sx9310.c b/drivers/iio/proximity/sx9310.c
index 1ed749190bff9..6f92cf18fac31 100644
--- a/drivers/iio/proximity/sx9310.c
+++ b/drivers/iio/proximity/sx9310.c
@@ -1219,8 +1219,8 @@ static const struct sx9310_reg_default *
 sx9310_get_default_reg(struct device *dev, int idx,
 		       struct sx9310_reg_default *reg_def)
 {
+	u32 start = 0, raw = 0, pos = 0, depth = 0;
 	u32 combined[SX9310_NUM_CHANNELS];
-	u32 start = 0, raw = 0, pos = 0;
 	unsigned long comb_mask = 0;
 	int ret, i, count;
 	const char *res;
@@ -1325,6 +1325,25 @@ sx9310_get_default_reg(struct device *dev, int idx,
 		reg_def->def |= FIELD_PREP(SX9310_REG_PROX_CTRL7_AVGPOSFILT_MASK,
 					   pos);
 		break;
+	case SX9310_REG_PROX_CTRL10:
+		ret = device_property_read_u32(dev, "semtech,close-debouncer-depth", &depth);
+		if (ret)
+			raw = FIELD_GET(SX9310_REG_PROX_CTRL10_CLOSE_DEBOUNCE_MASK,
+					reg_def->def);
+		else
+			raw = ilog2(depth);
+		reg_def->def &= ~SX9310_REG_PROX_CTRL10_CLOSE_DEBOUNCE_MASK;
+		reg_def->def |= FIELD_PREP(SX9310_REG_PROX_CTRL10_CLOSE_DEBOUNCE_MASK, raw);
+
+		ret = device_property_read_u32(dev, "semtech,far-debouncer-depth", &depth);
+		if (ret)
+			raw = FIELD_GET(SX9310_REG_PROX_CTRL10_FAR_DEBOUNCE_MASK,
+					reg_def->def);
+		else
+			raw = ilog2(depth);
+		reg_def->def &= ~SX9310_REG_PROX_CTRL10_FAR_DEBOUNCE_MASK;
+		reg_def->def |= FIELD_PREP(SX9310_REG_PROX_CTRL10_FAR_DEBOUNCE_MASK, raw);
+		break;
 	}
 
 	return reg_def;
-- 
2.31.0.291.g576ba9dcdaf-goog

Re: [PATCH 2/2] iio: sx9310: Add debouncer-depth parameters

[Andy Shevchenko]

On Sun, Mar 28, 2021 at 6:36 AM Gwendal Grignou <gwendal@chromium.org> wrote:
>
> Semtech SX9310 SAR sensor has a debouncer filter: only when N
> measurements are above/below the far/close threshold an event is
> sent to the host.
> By default the debouncer is set to 2 events for the close to far
> transition and 1 event (no debounce) for far to close.
> It is a balance speed of detection and false positive avoidance.
>
> On some chromebooks, the debouncer is set to a larger number.
...

> +               ret = device_property_read_u32(dev, "semtech,close-debouncer-depth", &depth);
> +               ret = device_property_read_u32(dev, "semtech,far-debouncer-depth", &depth);

Are they existing properties or new ones? To me sounds like the
latter. In such a case you missed DT bindings update.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/2] iio: sx9310: Add debouncer-depth parameters

[Stephen Boyd]

Quoting Andy Shevchenko (2021-03-28 05:32:36)
> On Sun, Mar 28, 2021 at 6:36 AM Gwendal Grignou <gwendal@chromium.org> wrote:
> >
> > Semtech SX9310 SAR sensor has a debouncer filter: only when N
> > measurements are above/below the far/close threshold an event is
> > sent to the host.
> > By default the debouncer is set to 2 events for the close to far
> > transition and 1 event (no debounce) for far to close.
> > It is a balance speed of detection and false positive avoidance.
> >
> > On some chromebooks, the debouncer is set to a larger number.
> ...
> 
> > +               ret = device_property_read_u32(dev, "semtech,close-debouncer-depth", &depth);
> > +               ret = device_property_read_u32(dev, "semtech,far-debouncer-depth", &depth);
> 
> Are they existing properties or new ones? To me sounds like the
> latter. In such a case you missed DT bindings update.

The bindings are part of patch #1.

Re: [PATCH 2/2] iio: sx9310: Add debouncer-depth parameters

[Andy Shevchenko]

On Mon, Mar 29, 2021 at 6:11 AM Stephen Boyd <swboyd@chromium.org> wrote:
> Quoting Andy Shevchenko (2021-03-28 05:32:36)
> > On Sun, Mar 28, 2021 at 6:36 AM Gwendal Grignou <gwendal@chromium.org> wrote:
> > >
> > > Semtech SX9310 SAR sensor has a debouncer filter: only when N
> > > measurements are above/below the far/close threshold an event is
> > > sent to the host.
> > > By default the debouncer is set to 2 events for the close to far
> > > transition and 1 event (no debounce) for far to close.
> > > It is a balance speed of detection and false positive avoidance.
> > >
> > > On some chromebooks, the debouncer is set to a larger number.
> > ...
> >
> > > +               ret = device_property_read_u32(dev, "semtech,close-debouncer-depth", &depth);
> > > +               ret = device_property_read_u32(dev, "semtech,far-debouncer-depth", &depth);
> >
> > Are they existing properties or new ones? To me sounds like the
> > latter. In such a case you missed DT bindings update.
>
> The bindings are part of patch #1.

Thanks, it means _I am_ missing them :-)

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 0/2] iio: sx9310: Add debouncer-depth parameters

[Stephen Boyd]

Quoting Gwendal Grignou (2021-03-27 20:36:37)
> Semtech SX9310 SAR sensor has a debouncer filter: only when N
> measurements are above/below the far/close threshold an event is
> sent to the host.
> By default the debouncer is set to 2 events for the close to far
> transition and 1 event (no debounce) for far to close.
> It is a balance speed of detection and false positive avoidance.
> 
> On some chromebooks, the debouncer is set to a larger number.
> 
> This patch applies on top of commit 103d6ec82676 ("iio: sx9310: Support ACPI properties")

The near/far debounce settings are already supported via sysfs. Can you
use those instead of putting this into DT/ACPI? See
sx9310_read_far_debounce() and associated code.

Re: [PATCH 0/2] iio: sx9310: Add debouncer-depth parameters

[Gwendal Grignou]

On Sun, Mar 28, 2021 at 8:12 PM Stephen Boyd <swboyd@chromium.org> wrote:
>
> Quoting Gwendal Grignou (2021-03-27 20:36:37)
> > Semtech SX9310 SAR sensor has a debouncer filter: only when N
> > measurements are above/below the far/close threshold an event is
> > sent to the host.
> > By default the debouncer is set to 2 events for the close to far
> > transition and 1 event (no debounce) for far to close.
> > It is a balance speed of detection and false positive avoidance.
> >
> > On some chromebooks, the debouncer is set to a larger number.
> >
> > This patch applies on top of commit 103d6ec82676 ("iio: sx9310: Support ACPI properties")
>
> The near/far debounce settings are already supported via sysfs. Can you
> use those instead of putting this into DT/ACPI? See
> sx9310_read_far_debounce() and associated code.
Stephen, I missed you already proposed these bindings earlier
[https://lore.kernel.org/linux-devicetree/20200906150247.3aaef3a3@archlinux/].
After Jonathan inputs, it is done via sysfs interface
[events/thresh_{falling|rising}_period].

The debounce parameters are in the same class as the other parameters
already present in the DT. They are calculated during tests to meet
FCC requirements, in particular the time it takes to detect a human
body near the antennas.
Depending on the SAR antenna design, it is a balance between lowering
the debouncer "period" to raise an event as soon as possible, and
increasing it to prevent false posible.

In addition to controlling it from sysfs, could it be acceptable to
have it in DT/ACPI as well?
In the meantime, I will make sure semtech,sx9310.yaml matches
in_proximityX_hardwaregain_available and connect to that attribute.

Gwendal.

Re: [PATCH 0/2] iio: sx9310: Add debouncer-depth parameters

[Jonathan Cameron]

On Tue, 30 Mar 2021 17:38:03 -0700
Gwendal Grignou <gwendal@chromium.org> wrote:

> On Sun, Mar 28, 2021 at 8:12 PM Stephen Boyd <swboyd@chromium.org> wrote:
> >
> > Quoting Gwendal Grignou (2021-03-27 20:36:37)  
> > > Semtech SX9310 SAR sensor has a debouncer filter: only when N
> > > measurements are above/below the far/close threshold an event is
> > > sent to the host.
> > > By default the debouncer is set to 2 events for the close to far
> > > transition and 1 event (no debounce) for far to close.
> > > It is a balance speed of detection and false positive avoidance.
> > >
> > > On some chromebooks, the debouncer is set to a larger number.
> > >
> > > This patch applies on top of commit 103d6ec82676 ("iio: sx9310: Support ACPI properties")  
> >
> > The near/far debounce settings are already supported via sysfs. Can you
> > use those instead of putting this into DT/ACPI? See
> > sx9310_read_far_debounce() and associated code.  
> Stephen, I missed you already proposed these bindings earlier
> [https://lore.kernel.org/linux-devicetree/20200906150247.3aaef3a3@archlinux/].
> After Jonathan inputs, it is done via sysfs interface
> [events/thresh_{falling|rising}_period].
> 
> The debounce parameters are in the same class as the other parameters
> already present in the DT. They are calculated during tests to meet
> FCC requirements, in particular the time it takes to detect a human
> body near the antennas.
> Depending on the SAR antenna design, it is a balance between lowering
> the debouncer "period" to raise an event as soon as possible, and
> increasing it to prevent false posible.
> 
> In addition to controlling it from sysfs, could it be acceptable to
> have it in DT/ACPI as well?
> In the meantime, I will make sure semtech,sx9310.yaml matches
> in_proximityX_hardwaregain_available and connect to that attribute.
> 
> Gwendal.


Ok, add something to make it clear that these effectively tied to the board
hardware because of this FCC requirement.

As long as that's clear the Rob is fine with the DT binding I don't mind.

Is there a requirement as a result of this FCC stuff that we should not
expose them to userspace control if they are provided in DT?

If so we should figure out a sensible way of doing that without breaking
the existing ABI.

Joanthan

Re: [PATCH 0/2] iio: sx9310: Add debouncer-depth parameters

[Stephen Boyd]

Quoting Jonathan Cameron (2021-04-01 06:19:35)
> On Tue, 30 Mar 2021 17:38:03 -0700
> Gwendal Grignou <gwendal@chromium.org> wrote:
> 
> > On Sun, Mar 28, 2021 at 8:12 PM Stephen Boyd <swboyd@chromium.org> wrote:
> > >
> > > Quoting Gwendal Grignou (2021-03-27 20:36:37)  
> > > > Semtech SX9310 SAR sensor has a debouncer filter: only when N
> > > > measurements are above/below the far/close threshold an event is
> > > > sent to the host.
> > > > By default the debouncer is set to 2 events for the close to far
> > > > transition and 1 event (no debounce) for far to close.
> > > > It is a balance speed of detection and false positive avoidance.
> > > >
> > > > On some chromebooks, the debouncer is set to a larger number.
> > > >
> > > > This patch applies on top of commit 103d6ec82676 ("iio: sx9310: Support ACPI properties")  
> > >
> > > The near/far debounce settings are already supported via sysfs. Can you
> > > use those instead of putting this into DT/ACPI? See
> > > sx9310_read_far_debounce() and associated code.  
> > Stephen, I missed you already proposed these bindings earlier
> > [https://lore.kernel.org/linux-devicetree/20200906150247.3aaef3a3@archlinux/].
> > After Jonathan inputs, it is done via sysfs interface
> > [events/thresh_{falling|rising}_period].
> > 
> > The debounce parameters are in the same class as the other parameters
> > already present in the DT. They are calculated during tests to meet
> > FCC requirements, in particular the time it takes to detect a human
> > body near the antennas.

The same could be said for the threshold values but those are in sysfs.

> > Depending on the SAR antenna design, it is a balance between lowering
> > the debouncer "period" to raise an event as soon as possible, and
> > increasing it to prevent false posible.
> > 
> > In addition to controlling it from sysfs, could it be acceptable to
> > have it in DT/ACPI as well?
> > In the meantime, I will make sure semtech,sx9310.yaml matches
> > in_proximityX_hardwaregain_available and connect to that attribute.
> > 

My understanding from the previous discussions with Jonathan was that
anything that could be delayed until later should be done through sysfs.
That's why only some properties are in DT, because they were used during
initial compensation of the device that happens when the device driver
probes. These other values like thresholds and debounce weren't required
for that so I put them into sysfs.

> 
> 
> Ok, add something to make it clear that these effectively tied to the board
> hardware because of this FCC requirement.
> 
> As long as that's clear the Rob is fine with the DT binding I don't mind.
> 
> Is there a requirement as a result of this FCC stuff that we should not
> expose them to userspace control if they are provided in DT?
> 
> If so we should figure out a sensible way of doing that without breaking
> the existing ABI.
>

Re: [PATCH 0/2] iio: sx9310: Add debouncer-depth parameters

[Jonathan Cameron]

On Thu, 01 Apr 2021 17:39:05 -0700
Stephen Boyd <swboyd@chromium.org> wrote:

> Quoting Jonathan Cameron (2021-04-01 06:19:35)
> > On Tue, 30 Mar 2021 17:38:03 -0700
> > Gwendal Grignou <gwendal@chromium.org> wrote:
> >   
> > > On Sun, Mar 28, 2021 at 8:12 PM Stephen Boyd <swboyd@chromium.org> wrote:  
> > > >
> > > > Quoting Gwendal Grignou (2021-03-27 20:36:37)    
> > > > > Semtech SX9310 SAR sensor has a debouncer filter: only when N
> > > > > measurements are above/below the far/close threshold an event is
> > > > > sent to the host.
> > > > > By default the debouncer is set to 2 events for the close to far
> > > > > transition and 1 event (no debounce) for far to close.
> > > > > It is a balance speed of detection and false positive avoidance.
> > > > >
> > > > > On some chromebooks, the debouncer is set to a larger number.
> > > > >
> > > > > This patch applies on top of commit 103d6ec82676 ("iio: sx9310: Support ACPI properties")    
> > > >
> > > > The near/far debounce settings are already supported via sysfs. Can you
> > > > use those instead of putting this into DT/ACPI? See
> > > > sx9310_read_far_debounce() and associated code.    
> > > Stephen, I missed you already proposed these bindings earlier
> > > [https://lore.kernel.org/linux-devicetree/20200906150247.3aaef3a3@archlinux/].
> > > After Jonathan inputs, it is done via sysfs interface
> > > [events/thresh_{falling|rising}_period].
> > > 
> > > The debounce parameters are in the same class as the other parameters
> > > already present in the DT. They are calculated during tests to meet
> > > FCC requirements, in particular the time it takes to detect a human
> > > body near the antennas.  
> 
> The same could be said for the threshold values but those are in sysfs.

Good point.

> 
> > > Depending on the SAR antenna design, it is a balance between lowering
> > > the debouncer "period" to raise an event as soon as possible, and
> > > increasing it to prevent false posible.
> > > 
> > > In addition to controlling it from sysfs, could it be acceptable to
> > > have it in DT/ACPI as well?
> > > In the meantime, I will make sure semtech,sx9310.yaml matches
> > > in_proximityX_hardwaregain_available and connect to that attribute.
> > >   
> 
> My understanding from the previous discussions with Jonathan was that
> anything that could be delayed until later should be done through sysfs.
> That's why only some properties are in DT, because they were used during
> initial compensation of the device that happens when the device driver
> probes. These other values like thresholds and debounce weren't required
> for that so I put them into sysfs.

My intent wasn't so much things that 'could' be delayed until later, but
rather the divide between policy (which should be in userspace control)
and hardware factors. We have a bit of an edge case here so not clear
cut.

I may well have been wrong in the past on this :(

Jonathan

> 
> > 
> > 
> > Ok, add something to make it clear that these effectively tied to the board
> > hardware because of this FCC requirement.
> > 
> > As long as that's clear the Rob is fine with the DT binding I don't mind.
> > 
> > Is there a requirement as a result of this FCC stuff that we should not
> > expose them to userspace control if they are provided in DT?
> > 
> > If so we should figure out a sensible way of doing that without breaking
> > the existing ABI.
> >  

Re: [PATCH 0/2] iio: sx9310: Add debouncer-depth parameters

[Gwendal Grignou]

On Fri, Apr 2, 2021 at 2:36 AM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Thu, 01 Apr 2021 17:39:05 -0700
> Stephen Boyd <swboyd@chromium.org> wrote:
>
> > Quoting Jonathan Cameron (2021-04-01 06:19:35)
> > > On Tue, 30 Mar 2021 17:38:03 -0700
> > > Gwendal Grignou <gwendal@chromium.org> wrote:
> > >
> > > > On Sun, Mar 28, 2021 at 8:12 PM Stephen Boyd <swboyd@chromium.org> wrote:
> > > > >
> > > > > Quoting Gwendal Grignou (2021-03-27 20:36:37)
> > > > > > Semtech SX9310 SAR sensor has a debouncer filter: only when N
> > > > > > measurements are above/below the far/close threshold an event is
> > > > > > sent to the host.
> > > > > > By default the debouncer is set to 2 events for the close to far
> > > > > > transition and 1 event (no debounce) for far to close.
> > > > > > It is a balance speed of detection and false positive avoidance.
> > > > > >
> > > > > > On some chromebooks, the debouncer is set to a larger number.
> > > > > >
> > > > > > This patch applies on top of commit 103d6ec82676 ("iio: sx9310: Support ACPI properties")
> > > > >
> > > > > The near/far debounce settings are already supported via sysfs. Can you
> > > > > use those instead of putting this into DT/ACPI? See
> > > > > sx9310_read_far_debounce() and associated code.
> > > > Stephen, I missed you already proposed these bindings earlier
> > > > [https://lore.kernel.org/linux-devicetree/20200906150247.3aaef3a3@archlinux/].
> > > > After Jonathan inputs, it is done via sysfs interface
> > > > [events/thresh_{falling|rising}_period].
> > > >
> > > > The debounce parameters are in the same class as the other parameters
> > > > already present in the DT. They are calculated during tests to meet
> > > > FCC requirements, in particular the time it takes to detect a human
> > > > body near the antennas.
> >
> > The same could be said for the threshold values but those are in sysfs.
>
> Good point.
>
> >
> > > > Depending on the SAR antenna design, it is a balance between lowering
> > > > the debouncer "period" to raise an event as soon as possible, and
> > > > increasing it to prevent false posible.
> > > >
> > > > In addition to controlling it from sysfs, could it be acceptable to
> > > > have it in DT/ACPI as well?
> > > > In the meantime, I will make sure semtech,sx9310.yaml matches
> > > > in_proximityX_hardwaregain_available and connect to that attribute.
> > > >
> >
> > My understanding from the previous discussions with Jonathan was that
> > anything that could be delayed until later should be done through sysfs.
> > That's why only some properties are in DT, because they were used during
> > initial compensation of the device that happens when the device driver
> > probes. These other values like thresholds and debounce weren't required
> > for that so I put them into sysfs.
>
> My intent wasn't so much things that 'could' be delayed until later, but
> rather the divide between policy (which should be in userspace control)
> and hardware factors. We have a bit of an edge case here so not clear
> cut.
>
> I may well have been wrong in the past on this :(

Looking at the code again, the current approach makes sense, and
having access through sysfs is nice since it allows easy
experimentation.
Let's forget this patchset, I will configure the required parameters from sysfs.

Gwendal.
>
> Jonathan
>
> >
> > >
> > >
> > > Ok, add something to make it clear that these effectively tied to the board
> > > hardware because of this FCC requirement.
> > >
> > > As long as that's clear the Rob is fine with the DT binding I don't mind.
> > >
> > > Is there a requirement as a result of this FCC stuff that we should not
> > > expose them to userspace control if they are provided in DT?
> > >
> > > If so we should figure out a sensible way of doing that without breaking
> > > the existing ABI.
> > >
>