| Message ID | 20210328033639.1021599-1-gwendal@chromium.org (mailing list archive) |
|---|---|
| Headers | show |
| Series | iio: sx9310: Add debouncer-depth parameters | expand |
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.
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.
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
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. >
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. > >
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. > > > >
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(-)