[PATCH 0/3] iio: Add a ChromeOS EC MKBP proximity driver

[PATCH 0/3] iio: Add a ChromeOS EC MKBP proximity driver

[Stephen Boyd]

This is a different approach to [1] where I tried to add this proximity
sensor logic to the input subsystem. Instead, we'll take the approach of
making a small IIO proximity driver that parses the EC switch bitmap to
find out if the front proximity sensor is detecting something or not.
This allows us to treat proximity sensors as IIO devices all the time in
userspace instead of handling this switch on the EC via the input
subsystem and then other proximity sensors via IIO.

[1] https://lore.kernel.org/r/20201205004709.3126266-1-swboyd@chromium.org

Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: Benson Leung <bleung@chromium.org>
Cc: Guenter Roeck <groeck@chromium.org>
Cc: Douglas Anderson <dianders@chromium.org>
Cc: Gwendal Grignou <gwendal@chromium.org>
Cc: <devicetree@vger.kernel.org>
Cc: Rob Herring <robh+dt@kernel.org>

Stephen Boyd (3):
  platform/chrome: cros_ec: Add SW_FRONT_PROXIMITY MKBP define
  dt-bindings: iio: Add cros ec proximity yaml doc
  iio: proximity: Add a ChromeOS EC MKBP proximity driver

 .../proximity/google,cros-ec-proximity.yaml   |  37 +++
 drivers/iio/proximity/Kconfig                 |  11 +
 drivers/iio/proximity/Makefile                |   1 +
 drivers/iio/proximity/cros_ec_proximity.c     | 252 ++++++++++++++++++
 .../linux/platform_data/cros_ec_commands.h    |   1 +
 5 files changed, 302 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/proximity/google,cros-ec-proximity.yaml
 create mode 100644 drivers/iio/proximity/cros_ec_proximity.c


base-commit: 19c329f6808995b142b3966301f217c831e7cf31
-- 
https://chromeos.dev

[PATCH 1/3] platform/chrome: cros_ec: Add SW_FRONT_PROXIMITY MKBP define

[Stephen Boyd]

Some cros ECs support a front proximity MKBP event via
'EC_MKBP_FRONT_PROXIMITY'. Add this define so it can be used in a
future patch.

Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: Benson Leung <bleung@chromium.org>
Cc: Guenter Roeck <groeck@chromium.org>
Cc: Douglas Anderson <dianders@chromium.org>
Cc: Gwendal Grignou <gwendal@chromium.org>
Signed-off-by: Stephen Boyd <swboyd@chromium.org>
---
 include/linux/platform_data/cros_ec_commands.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/linux/platform_data/cros_ec_commands.h b/include/linux/platform_data/cros_ec_commands.h
index 86376779ab31..776e0b2be0e9 100644
--- a/include/linux/platform_data/cros_ec_commands.h
+++ b/include/linux/platform_data/cros_ec_commands.h
@@ -3457,6 +3457,7 @@ struct ec_response_get_next_event_v1 {
 #define EC_MKBP_LID_OPEN	0
 #define EC_MKBP_TABLET_MODE	1
 #define EC_MKBP_BASE_ATTACHED	2
+#define EC_MKBP_FRONT_PROXIMITY	3
 
 /* Run keyboard factory test scanning */
 #define EC_CMD_KEYBOARD_FACTORY_TEST 0x0068
-- 
https://chromeos.dev

[PATCH 2/3] dt-bindings: iio: Add cros ec proximity yaml doc

[Stephen Boyd]

Some cros ECs support a front proximity MKBP event via
'EC_MKBP_FRONT_PROXIMITY'. Add a DT binding to document this feature via
a node that is a child of the main cros_ec device node. Devices that
have this ability will describe this in firmware.

Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: Benson Leung <bleung@chromium.org>
Cc: Guenter Roeck <groeck@chromium.org>
Cc: Douglas Anderson <dianders@chromium.org>
Cc: Gwendal Grignou <gwendal@chromium.org>
Cc: <devicetree@vger.kernel.org>
Cc: Rob Herring <robh+dt@kernel.org>
Signed-off-by: Stephen Boyd <swboyd@chromium.org>
---
 .../proximity/google,cros-ec-proximity.yaml   | 37 +++++++++++++++++++
 1 file changed, 37 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/proximity/google,cros-ec-proximity.yaml

diff --git a/Documentation/devicetree/bindings/iio/proximity/google,cros-ec-proximity.yaml b/Documentation/devicetree/bindings/iio/proximity/google,cros-ec-proximity.yaml
new file mode 100644
index 000000000000..c0a34bdfe4fd
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/proximity/google,cros-ec-proximity.yaml
@@ -0,0 +1,37 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+
+$id: http://devicetree.org/schemas/iio/proximity/google,cros-ec-proximity.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: ChromeOS EC MKBP Proximity Sensor
+
+maintainers:
+  - Stephen Boyd <swboyd@chromium.org>
+  - Benson Leung <bleung@chromium.org>
+  - Enric Balletbo i Serra <enric.balletbo@collabora.com>
+
+description: |
+  Google's ChromeOS EC sometimes has the ability to detect user proximity.
+  This is implemented on the EC as near/far logic and exposed to the OS
+  via an MKBP switch bit.
+
+properties:
+  compatible:
+    const: google,cros-ec-proximity
+
+  label:
+    description: Name for proximity sensor
+
+required:
+  - compatible
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    proximity {
+        compatible = "google,cros-ec-proximity";
+        label = "proximity-wifi-lte";
+    };
-- 
https://chromeos.dev

Re: [PATCH 2/3] dt-bindings: iio: Add cros ec proximity yaml doc

[Jonathan Cameron]

On Fri, 22 Jan 2021 14:54:42 -0800
Stephen Boyd <swboyd@chromium.org> wrote:

> Some cros ECs support a front proximity MKBP event via
> 'EC_MKBP_FRONT_PROXIMITY'. Add a DT binding to document this feature via
> a node that is a child of the main cros_ec device node. Devices that
> have this ability will describe this in firmware.
> 
> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Cc: Benson Leung <bleung@chromium.org>
> Cc: Guenter Roeck <groeck@chromium.org>
> Cc: Douglas Anderson <dianders@chromium.org>
> Cc: Gwendal Grignou <gwendal@chromium.org>
> Cc: <devicetree@vger.kernel.org>
> Cc: Rob Herring <robh+dt@kernel.org>
> Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> ---
>  .../proximity/google,cros-ec-proximity.yaml   | 37 +++++++++++++++++++
>  1 file changed, 37 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/proximity/google,cros-ec-proximity.yaml
> 
> diff --git a/Documentation/devicetree/bindings/iio/proximity/google,cros-ec-proximity.yaml b/Documentation/devicetree/bindings/iio/proximity/google,cros-ec-proximity.yaml
> new file mode 100644
> index 000000000000..c0a34bdfe4fd
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/proximity/google,cros-ec-proximity.yaml
> @@ -0,0 +1,37 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +
> +$id: http://devicetree.org/schemas/iio/proximity/google,cros-ec-proximity.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: ChromeOS EC MKBP Proximity Sensor
> +
> +maintainers:
> +  - Stephen Boyd <swboyd@chromium.org>
> +  - Benson Leung <bleung@chromium.org>
> +  - Enric Balletbo i Serra <enric.balletbo@collabora.com>
> +
> +description: |
> +  Google's ChromeOS EC sometimes has the ability to detect user proximity.
> +  This is implemented on the EC as near/far logic and exposed to the OS
> +  via an MKBP switch bit.
> +
> +properties:
> +  compatible:
> +    const: google,cros-ec-proximity
> +
> +  label:
> +    description: Name for proximity sensor
> +
> +required:
> +  - compatible
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    proximity {

Can we at least have the example making it clear this is a child of the
cros_ec device?

> +        compatible = "google,cros-ec-proximity";
> +        label = "proximity-wifi-lte";
> +    };

Re: [PATCH 2/3] dt-bindings: iio: Add cros ec proximity yaml doc

[Gwendal Grignou]

On Sun, Jan 24, 2021 at 9:28 AM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Fri, 22 Jan 2021 14:54:42 -0800
> Stephen Boyd <swboyd@chromium.org> wrote:
>
> > Some cros ECs support a front proximity MKBP event via
> > 'EC_MKBP_FRONT_PROXIMITY'. Add a DT binding to document this feature via
> > a node that is a child of the main cros_ec device node. Devices that
> > have this ability will describe this in firmware.
> >
> > Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > Cc: Benson Leung <bleung@chromium.org>
> > Cc: Guenter Roeck <groeck@chromium.org>
> > Cc: Douglas Anderson <dianders@chromium.org>
> > Cc: Gwendal Grignou <gwendal@chromium.org>
> > Cc: <devicetree@vger.kernel.org>
> > Cc: Rob Herring <robh+dt@kernel.org>
> > Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> > ---
> >  .../proximity/google,cros-ec-proximity.yaml   | 37 +++++++++++++++++++
> >  1 file changed, 37 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/iio/proximity/google,cros-ec-proximity.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/iio/proximity/google,cros-ec-proximity.yaml b/Documentation/devicetree/bindings/iio/proximity/google,cros-ec-proximity.yaml
> > new file mode 100644
> > index 000000000000..c0a34bdfe4fd
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/iio/proximity/google,cros-ec-proximity.yaml
> > @@ -0,0 +1,37 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +
> > +$id: http://devicetree.org/schemas/iio/proximity/google,cros-ec-proximity.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: ChromeOS EC MKBP Proximity Sensor
> > +
> > +maintainers:
> > +  - Stephen Boyd <swboyd@chromium.org>
> > +  - Benson Leung <bleung@chromium.org>
> > +  - Enric Balletbo i Serra <enric.balletbo@collabora.com>
> > +
> > +description: |
> > +  Google's ChromeOS EC sometimes has the ability to detect user proximity.
> > +  This is implemented on the EC as near/far logic and exposed to the OS
> > +  via an MKBP switch bit.
> > +
> > +properties:
> > +  compatible:
> > +    const: google,cros-ec-proximity
Given we have proximity detection in cros_ec via specific sensor (si
1141) or algorithm (on-body/off-body via
MOTIONSENSE_ACTIVITY_BODY_DETECTION), can we name the property
cros-ec-mkbp-proximity?
> > +
> > +  label:
> > +    description: Name for proximity sensor
> > +
> > +required:
> > +  - compatible
> > +
> > +unevaluatedProperties: false
> > +
> > +examples:
> > +  - |
> > +    proximity {
>
> Can we at least have the example making it clear this is a child of the
> cros_ec device?
>
> > +        compatible = "google,cros-ec-proximity";
> > +        label = "proximity-wifi-lte";
> > +    };
>

Re: [PATCH 2/3] dt-bindings: iio: Add cros ec proximity yaml doc

[Stephen Boyd]

Quoting Gwendal Grignou (2021-01-24 12:42:56)
> On Sun, Jan 24, 2021 at 9:28 AM Jonathan Cameron <jic23@kernel.org> wrote:
> > On Fri, 22 Jan 2021 14:54:42 -0800
> > Stephen Boyd <swboyd@chromium.org> wrote:
> > > +
> > > +properties:
> > > +  compatible:
> > > +    const: google,cros-ec-proximity
> Given we have proximity detection in cros_ec via specific sensor (si
> 1141) or algorithm (on-body/off-body via
> MOTIONSENSE_ACTIVITY_BODY_DETECTION), can we name the property
> cros-ec-mkbp-proximity?

Sure that works for me.

Re: [PATCH 2/3] dt-bindings: iio: Add cros ec proximity yaml doc

[Rob Herring]

On Sun, Jan 24, 2021 at 05:27:56PM +0000, Jonathan Cameron wrote:
> On Fri, 22 Jan 2021 14:54:42 -0800
> Stephen Boyd <swboyd@chromium.org> wrote:
> 
> > Some cros ECs support a front proximity MKBP event via
> > 'EC_MKBP_FRONT_PROXIMITY'. Add a DT binding to document this feature via
> > a node that is a child of the main cros_ec device node. Devices that
> > have this ability will describe this in firmware.
> > 
> > Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > Cc: Benson Leung <bleung@chromium.org>
> > Cc: Guenter Roeck <groeck@chromium.org>
> > Cc: Douglas Anderson <dianders@chromium.org>
> > Cc: Gwendal Grignou <gwendal@chromium.org>
> > Cc: <devicetree@vger.kernel.org>
> > Cc: Rob Herring <robh+dt@kernel.org>
> > Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> > ---
> >  .../proximity/google,cros-ec-proximity.yaml   | 37 +++++++++++++++++++
> >  1 file changed, 37 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/iio/proximity/google,cros-ec-proximity.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/iio/proximity/google,cros-ec-proximity.yaml b/Documentation/devicetree/bindings/iio/proximity/google,cros-ec-proximity.yaml
> > new file mode 100644
> > index 000000000000..c0a34bdfe4fd
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/iio/proximity/google,cros-ec-proximity.yaml
> > @@ -0,0 +1,37 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +
> > +$id: http://devicetree.org/schemas/iio/proximity/google,cros-ec-proximity.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: ChromeOS EC MKBP Proximity Sensor
> > +
> > +maintainers:
> > +  - Stephen Boyd <swboyd@chromium.org>
> > +  - Benson Leung <bleung@chromium.org>
> > +  - Enric Balletbo i Serra <enric.balletbo@collabora.com>
> > +
> > +description: |
> > +  Google's ChromeOS EC sometimes has the ability to detect user proximity.
> > +  This is implemented on the EC as near/far logic and exposed to the OS
> > +  via an MKBP switch bit.
> > +
> > +properties:
> > +  compatible:
> > +    const: google,cros-ec-proximity
> > +
> > +  label:
> > +    description: Name for proximity sensor
> > +
> > +required:
> > +  - compatible
> > +
> > +unevaluatedProperties: false

additionalProperties: false

> > +
> > +examples:
> > +  - |
> > +    proximity {
> 
> Can we at least have the example making it clear this is a child of the
> cros_ec device?

Move this to the core Cros EC binding. The core binding needs to define 
'proximity' and reference this binding ($ref).

> 
> > +        compatible = "google,cros-ec-proximity";
> > +        label = "proximity-wifi-lte";
> > +    };
> 

Re: [PATCH 2/3] dt-bindings: iio: Add cros ec proximity yaml doc

[Stephen Boyd]

Quoting Rob Herring (2021-01-25 07:02:03)
> On Sun, Jan 24, 2021 at 05:27:56PM +0000, Jonathan Cameron wrote:
> > On Fri, 22 Jan 2021 14:54:42 -0800
> > Stephen Boyd <swboyd@chromium.org> wrote:
> > 
> > > Some cros ECs support a front proximity MKBP event via
> > > 'EC_MKBP_FRONT_PROXIMITY'. Add a DT binding to document this feature via
> > > a node that is a child of the main cros_ec device node. Devices that
> > > have this ability will describe this in firmware.
> > > 
> > > Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > > Cc: Benson Leung <bleung@chromium.org>
> > > Cc: Guenter Roeck <groeck@chromium.org>
> > > Cc: Douglas Anderson <dianders@chromium.org>
> > > Cc: Gwendal Grignou <gwendal@chromium.org>
> > > Cc: <devicetree@vger.kernel.org>
> > > Cc: Rob Herring <robh+dt@kernel.org>
> > > Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> > > ---
> > >  .../proximity/google,cros-ec-proximity.yaml   | 37 +++++++++++++++++++
> > >  1 file changed, 37 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/iio/proximity/google,cros-ec-proximity.yaml
> > > 
> > > diff --git a/Documentation/devicetree/bindings/iio/proximity/google,cros-ec-proximity.yaml b/Documentation/devicetree/bindings/iio/proximity/google,cros-ec-proximity.yaml
> > > new file mode 100644
> > > index 000000000000..c0a34bdfe4fd
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/iio/proximity/google,cros-ec-proximity.yaml
> > > @@ -0,0 +1,37 @@
> > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > > +%YAML 1.2
> > > +---
> > > +
> > > +$id: http://devicetree.org/schemas/iio/proximity/google,cros-ec-proximity.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: ChromeOS EC MKBP Proximity Sensor
> > > +
> > > +maintainers:
> > > +  - Stephen Boyd <swboyd@chromium.org>
> > > +  - Benson Leung <bleung@chromium.org>
> > > +  - Enric Balletbo i Serra <enric.balletbo@collabora.com>
> > > +
> > > +description: |
> > > +  Google's ChromeOS EC sometimes has the ability to detect user proximity.
> > > +  This is implemented on the EC as near/far logic and exposed to the OS
> > > +  via an MKBP switch bit.
> > > +
> > > +properties:
> > > +  compatible:
> > > +    const: google,cros-ec-proximity
> > > +
> > > +  label:
> > > +    description: Name for proximity sensor
> > > +
> > > +required:
> > > +  - compatible
> > > +
> > > +unevaluatedProperties: false
> 
> additionalProperties: false
> 
> > > +
> > > +examples:
> > > +  - |
> > > +    proximity {
> > 
> > Can we at least have the example making it clear this is a child of the
> > cros_ec device?
> 
> Move this to the core Cros EC binding. The core binding needs to define 
> 'proximity' and reference this binding ($ref).

Will do.

Re: [PATCH 2/3] dt-bindings: iio: Add cros ec proximity yaml doc

[Rob Herring]

On Fri, 22 Jan 2021 14:54:42 -0800, Stephen Boyd wrote:
> Some cros ECs support a front proximity MKBP event via
> 'EC_MKBP_FRONT_PROXIMITY'. Add a DT binding to document this feature via
> a node that is a child of the main cros_ec device node. Devices that
> have this ability will describe this in firmware.
> 
> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Cc: Benson Leung <bleung@chromium.org>
> Cc: Guenter Roeck <groeck@chromium.org>
> Cc: Douglas Anderson <dianders@chromium.org>
> Cc: Gwendal Grignou <gwendal@chromium.org>
> Cc: <devicetree@vger.kernel.org>
> Cc: Rob Herring <robh+dt@kernel.org>
> Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> ---
>  .../proximity/google,cros-ec-proximity.yaml   | 37 +++++++++++++++++++
>  1 file changed, 37 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/proximity/google,cros-ec-proximity.yaml
> 

My bot found errors running 'make dt_binding_check' on your patch:

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/iio/proximity/google,cros-ec-proximity.yaml: 'additionalProperties' is a required property
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/iio/proximity/google,cros-ec-proximity.yaml: ignoring, error in schema: 
warning: no schema found in file: ./Documentation/devicetree/bindings/iio/proximity/google,cros-ec-proximity.yaml

See https://patchwork.ozlabs.org/patch/1430611

This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit.

[PATCH 3/3] iio: proximity: Add a ChromeOS EC MKBP proximity driver

[Stephen Boyd]

Add support for a ChromeOS EC proximity driver that exposes a "front"
proximity sensor via the IIO subsystem. The EC decides when front
proximity is near and sets an MKBP switch 'EC_MKBP_FRONT_PROXIMITY' to
notify the kernel of proximity. Similarly, when proximity detects
something far away it sets the switch bit to 0. For now this driver
exposes a single sensor, but it could be expanded in the future via more
MKBP bits if desired.

Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: Benson Leung <bleung@chromium.org>
Cc: Guenter Roeck <groeck@chromium.org>
Cc: Douglas Anderson <dianders@chromium.org>
Cc: Gwendal Grignou <gwendal@chromium.org>
Signed-off-by: Stephen Boyd <swboyd@chromium.org>
---
 drivers/iio/proximity/Kconfig             |  11 +
 drivers/iio/proximity/Makefile            |   1 +
 drivers/iio/proximity/cros_ec_proximity.c | 252 ++++++++++++++++++++++
 3 files changed, 264 insertions(+)
 create mode 100644 drivers/iio/proximity/cros_ec_proximity.c

diff --git a/drivers/iio/proximity/Kconfig b/drivers/iio/proximity/Kconfig
index 12672a0e89ed..35a04e9ede7d 100644
--- a/drivers/iio/proximity/Kconfig
+++ b/drivers/iio/proximity/Kconfig
@@ -21,6 +21,17 @@ endmenu
 
 menu "Proximity and distance sensors"
 
+config CROS_EC_PROXIMITY
+	tristate "ChromeOS EC MKBP Proximity sensor"
+	depends on CROS_EC
+	help
+	  Say Y here to enable the proximity sensor implemented via the ChromeOS EC MKBP
+	  switches protocol. You must enable one bus option (CROS_EC_I2C or CROS_EC_SPI)
+	  to use this.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called cros_ec_prox.
+
 config ISL29501
 	tristate "Intersil ISL29501 Time Of Flight sensor"
 	depends on I2C
diff --git a/drivers/iio/proximity/Makefile b/drivers/iio/proximity/Makefile
index 9c1aca1a8b79..b1330dd8e212 100644
--- a/drivers/iio/proximity/Makefile
+++ b/drivers/iio/proximity/Makefile
@@ -5,6 +5,7 @@
 
 # When adding new entries keep the list in alphabetical order
 obj-$(CONFIG_AS3935)		+= as3935.o
+obj-$(CONFIG_CROS_EC_PROXIMITY)	+= cros_ec_proximity.o
 obj-$(CONFIG_ISL29501)		+= isl29501.o
 obj-$(CONFIG_LIDAR_LITE_V2)	+= pulsedlight-lidar-lite-v2.o
 obj-$(CONFIG_MB1232)		+= mb1232.o
diff --git a/drivers/iio/proximity/cros_ec_proximity.c b/drivers/iio/proximity/cros_ec_proximity.c
new file mode 100644
index 000000000000..a3aef911e3cc
--- /dev/null
+++ b/drivers/iio/proximity/cros_ec_proximity.c
@@ -0,0 +1,252 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Driver for cros-ec proximity sensor exposed through MKBP switch
+ *
+ * Copyright 2021 Google LLC.
+ */
+
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/kernel.h>
+#include <linux/notifier.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+#include <linux/types.h>
+
+#include <linux/platform_data/cros_ec_commands.h>
+#include <linux/platform_data/cros_ec_proto.h>
+
+#include <linux/iio/events.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
+
+#include <asm/unaligned.h>
+
+struct cros_ec_proximity_data {
+	struct cros_ec_device *ec;
+	struct iio_dev *indio_dev;
+	struct mutex lock;
+	struct notifier_block notifier;
+	bool enabled;
+};
+
+static const struct iio_event_spec cros_ec_prox_events[] = {
+	{
+		.type = IIO_EV_TYPE_THRESH,
+		.dir = IIO_EV_DIR_EITHER,
+		.mask_separate = BIT(IIO_EV_INFO_ENABLE),
+	},
+};
+
+static const struct iio_chan_spec cros_ec_prox_chan_spec[] = {
+	{
+		.type = IIO_PROXIMITY,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+		.event_spec = cros_ec_prox_events,
+		.num_event_specs = ARRAY_SIZE(cros_ec_prox_events),
+	},
+};
+
+static int cros_ec_proximity_parse_state(const void *data)
+{
+	u32 switches = get_unaligned_le32(data);
+
+	return !!(switches & BIT(EC_MKBP_FRONT_PROXIMITY));
+}
+
+static int cros_ec_proximity_query(struct cros_ec_device *ec_dev, int *state)
+{
+	struct ec_params_mkbp_info *params;
+	struct cros_ec_command *msg;
+	int ret;
+
+	msg = kzalloc(sizeof(*msg) + max(sizeof(u32), sizeof(*params)),
+		      GFP_KERNEL);
+	if (!msg)
+		return -ENOMEM;
+
+	msg->command = EC_CMD_MKBP_INFO;
+	msg->version = 1;
+	msg->outsize = sizeof(*params);
+	msg->insize = sizeof(u32);
+	params = (struct ec_params_mkbp_info *)msg->data;
+	params->info_type = EC_MKBP_INFO_CURRENT;
+	params->event_type = EC_MKBP_EVENT_SWITCH;
+
+	ret = cros_ec_cmd_xfer_status(ec_dev, msg);
+	if (ret >= 0) {
+		if (ret != sizeof(u32)) {
+			dev_warn(ec_dev->dev, "wrong result size: %d != %zu\n",
+				 ret, sizeof(u32));
+			ret = -EPROTO;
+		} else {
+			*state = cros_ec_proximity_parse_state(msg->data);
+			ret = 0;
+		}
+	}
+
+	kfree(msg);
+
+	return ret;
+}
+
+static int cros_ec_proximity_notify(struct notifier_block *nb,
+			     unsigned long queued_during_suspend, void *_ec)
+{
+	struct cros_ec_proximity_data *data;
+	struct cros_ec_device *ec = _ec;
+	u8 event_type = ec->event_data.event_type & EC_MKBP_EVENT_TYPE_MASK;
+	void *switches = &ec->event_data.data.switches;
+	struct iio_dev *indio_dev;
+	s64 timestamp;
+	int state, dir;
+	u64 ev;
+
+	if (event_type == EC_MKBP_EVENT_SWITCH) {
+		data = container_of(nb, struct cros_ec_proximity_data, notifier);
+		indio_dev = data->indio_dev;
+
+		mutex_lock(&data->lock);
+		if (data->enabled) {
+			timestamp = iio_get_time_ns(indio_dev);
+			state = cros_ec_proximity_parse_state(switches);
+			dir = state ? IIO_EV_DIR_FALLING : IIO_EV_DIR_RISING;
+
+			ev = IIO_UNMOD_EVENT_CODE(IIO_PROXIMITY, 0,
+						  IIO_EV_TYPE_THRESH, dir);
+			iio_push_event(indio_dev, ev, timestamp);
+		}
+		mutex_unlock(&data->lock);
+	}
+
+	return NOTIFY_OK;
+}
+
+static int cros_ec_proximity_read_raw(struct iio_dev *indio_dev,
+			   const struct iio_chan_spec *chan, int *val,
+			   int *val2, long mask)
+{
+	struct cros_ec_proximity_data *data = iio_priv(indio_dev);
+	struct cros_ec_device *ec = data->ec;
+	int ret;
+
+	if (chan->type != IIO_PROXIMITY)
+		return -EINVAL;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		ret = iio_device_claim_direct_mode(indio_dev);
+		if (ret)
+			return ret;
+
+		ret = cros_ec_proximity_query(ec, val);
+		iio_device_release_direct_mode(indio_dev);
+		if (ret)
+			return ret;
+
+		return IIO_VAL_INT;
+	}
+
+	return -EINVAL;
+}
+
+static int cros_ec_proximity_read_event_config(struct iio_dev *indio_dev,
+				    const struct iio_chan_spec *chan,
+				    enum iio_event_type type,
+				    enum iio_event_direction dir)
+{
+	struct cros_ec_proximity_data *data = iio_priv(indio_dev);
+
+	return data->enabled;
+}
+
+static int cros_ec_proximity_write_event_config(struct iio_dev *indio_dev,
+				     const struct iio_chan_spec *chan,
+				     enum iio_event_type type,
+				     enum iio_event_direction dir, int state)
+{
+	struct cros_ec_proximity_data *data = iio_priv(indio_dev);
+
+	mutex_lock(&data->lock);
+	data->enabled = state;
+	mutex_unlock(&data->lock);
+
+	return 0;
+}
+
+static const struct iio_info cros_ec_proximity_info = {
+	.read_raw = cros_ec_proximity_read_raw,
+	.read_event_config = cros_ec_proximity_read_event_config,
+	.write_event_config = cros_ec_proximity_write_event_config,
+};
+
+static int cros_ec_proximity_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct cros_ec_device *ec = dev_get_drvdata(dev->parent);
+	struct iio_dev *indio_dev;
+	struct cros_ec_proximity_data *data;
+	int ret;
+
+	indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	data = iio_priv(indio_dev);
+	data->ec = ec;
+	data->indio_dev = indio_dev;
+	mutex_init(&data->lock);
+	platform_set_drvdata(pdev, data);
+
+	indio_dev->name = "cros_ec_proximity";
+	indio_dev->dev.parent = dev;
+	indio_dev->info = &cros_ec_proximity_info;
+	indio_dev->modes = INDIO_DIRECT_MODE;
+	indio_dev->channels = cros_ec_prox_chan_spec;
+	indio_dev->num_channels = ARRAY_SIZE(cros_ec_prox_chan_spec);
+
+	ret = devm_iio_device_register(dev, indio_dev);
+	if (ret)
+		return ret;
+
+	data->notifier.notifier_call = cros_ec_proximity_notify;
+	ret = blocking_notifier_chain_register(&ec->event_notifier,
+					       &data->notifier);
+	if (ret)
+		dev_err(dev, "cannot register notifier: %d\n", ret);
+
+	return ret;
+}
+
+static int cros_ec_proximity_remove(struct platform_device *pdev)
+{
+	struct cros_ec_proximity_data *data = platform_get_drvdata(pdev);
+	struct cros_ec_device *ec = data->ec;
+
+	blocking_notifier_chain_unregister(&ec->event_notifier,
+					   &data->notifier);
+
+	return 0;
+}
+
+#ifdef CONFIG_OF
+static const struct of_device_id cros_ec_proximity_of_match[] = {
+	{ .compatible = "google,cros-ec-proximity" },
+	{}
+};
+MODULE_DEVICE_TABLE(of, cros_ec_proximity_of_match);
+#endif
+
+static struct platform_driver cros_ec_proximity_driver = {
+	.driver = {
+		.name = "cros-ec-proximity",
+		.of_match_table = of_match_ptr(cros_ec_proximity_of_match),
+	},
+	.probe = cros_ec_proximity_probe,
+	.remove = cros_ec_proximity_remove,
+};
+module_platform_driver(cros_ec_proximity_driver);
+
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("ChromeOS EC MKBP proximity sensor driver");
-- 
https://chromeos.dev

Re: [PATCH 3/3] iio: proximity: Add a ChromeOS EC MKBP proximity driver

[Jonathan Cameron]

On Fri, 22 Jan 2021 14:54:43 -0800
Stephen Boyd <swboyd@chromium.org> wrote:

> Add support for a ChromeOS EC proximity driver that exposes a "front"
> proximity sensor via the IIO subsystem. The EC decides when front
> proximity is near and sets an MKBP switch 'EC_MKBP_FRONT_PROXIMITY' to
> notify the kernel of proximity. Similarly, when proximity detects
> something far away it sets the switch bit to 0. For now this driver
> exposes a single sensor, but it could be expanded in the future via more
> MKBP bits if desired.
> 
> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Cc: Benson Leung <bleung@chromium.org>
> Cc: Guenter Roeck <groeck@chromium.org>
> Cc: Douglas Anderson <dianders@chromium.org>
> Cc: Gwendal Grignou <gwendal@chromium.org>
> Signed-off-by: Stephen Boyd <swboyd@chromium.org>

Hi Stephen,

Looks more or less fine to me.  My main concern is potential confusion
in naming with the cros_ec_prox_light driver that we already have.

A few other minor bits and bobs inline.

thanks,

Jonathan


> ---
>  drivers/iio/proximity/Kconfig             |  11 +
>  drivers/iio/proximity/Makefile            |   1 +
>  drivers/iio/proximity/cros_ec_proximity.c | 252 ++++++++++++++++++++++
>  3 files changed, 264 insertions(+)
>  create mode 100644 drivers/iio/proximity/cros_ec_proximity.c
> 
> diff --git a/drivers/iio/proximity/Kconfig b/drivers/iio/proximity/Kconfig
> index 12672a0e89ed..35a04e9ede7d 100644
> --- a/drivers/iio/proximity/Kconfig
> +++ b/drivers/iio/proximity/Kconfig
> @@ -21,6 +21,17 @@ endmenu
>  
>  menu "Proximity and distance sensors"
>  
> +config CROS_EC_PROXIMITY
> +	tristate "ChromeOS EC MKBP Proximity sensor"
> +	depends on CROS_EC
> +	help
> +	  Say Y here to enable the proximity sensor implemented via the ChromeOS EC MKBP
> +	  switches protocol. You must enable one bus option (CROS_EC_I2C or CROS_EC_SPI)
> +	  to use this.
> +
> +	  To compile this driver as a module, choose M here: the
> +	  module will be called cros_ec_prox.
> +
>  config ISL29501
>  	tristate "Intersil ISL29501 Time Of Flight sensor"
>  	depends on I2C
> diff --git a/drivers/iio/proximity/Makefile b/drivers/iio/proximity/Makefile
> index 9c1aca1a8b79..b1330dd8e212 100644
> --- a/drivers/iio/proximity/Makefile
> +++ b/drivers/iio/proximity/Makefile
> @@ -5,6 +5,7 @@
>  
>  # When adding new entries keep the list in alphabetical order
>  obj-$(CONFIG_AS3935)		+= as3935.o
> +obj-$(CONFIG_CROS_EC_PROXIMITY)	+= cros_ec_proximity.o
>  obj-$(CONFIG_ISL29501)		+= isl29501.o
>  obj-$(CONFIG_LIDAR_LITE_V2)	+= pulsedlight-lidar-lite-v2.o
>  obj-$(CONFIG_MB1232)		+= mb1232.o
> diff --git a/drivers/iio/proximity/cros_ec_proximity.c b/drivers/iio/proximity/cros_ec_proximity.c
> new file mode 100644
> index 000000000000..a3aef911e3cc
> --- /dev/null
> +++ b/drivers/iio/proximity/cros_ec_proximity.c
> @@ -0,0 +1,252 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Driver for cros-ec proximity sensor exposed through MKBP switch
> + *
> + * Copyright 2021 Google LLC.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/kernel.h>

Slight preference for alphabetical order though keeping specific
sections separate as done here is fine.

> +#include <linux/notifier.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +#include <linux/types.h>
> +
> +#include <linux/platform_data/cros_ec_commands.h>
> +#include <linux/platform_data/cros_ec_proto.h>
> +
> +#include <linux/iio/events.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
> +
> +#include <asm/unaligned.h>
> +
> +struct cros_ec_proximity_data {
> +	struct cros_ec_device *ec;
> +	struct iio_dev *indio_dev;
> +	struct mutex lock;
> +	struct notifier_block notifier;
> +	bool enabled;
> +};
> +
> +static const struct iio_event_spec cros_ec_prox_events[] = {
> +	{
> +		.type = IIO_EV_TYPE_THRESH,
> +		.dir = IIO_EV_DIR_EITHER,
> +		.mask_separate = BIT(IIO_EV_INFO_ENABLE),
> +	},
> +};
> +
> +static const struct iio_chan_spec cros_ec_prox_chan_spec[] = {
> +	{
> +		.type = IIO_PROXIMITY,
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> +		.event_spec = cros_ec_prox_events,
> +		.num_event_specs = ARRAY_SIZE(cros_ec_prox_events),
> +	},
> +};
> +
> +static int cros_ec_proximity_parse_state(const void *data)
> +{
> +	u32 switches = get_unaligned_le32(data);
> +
> +	return !!(switches & BIT(EC_MKBP_FRONT_PROXIMITY));
> +}
> +
> +static int cros_ec_proximity_query(struct cros_ec_device *ec_dev, int *state)
> +{
> +	struct ec_params_mkbp_info *params;
> +	struct cros_ec_command *msg;
> +	int ret;
> +
> +	msg = kzalloc(sizeof(*msg) + max(sizeof(u32), sizeof(*params)),
> +		      GFP_KERNEL);

Given this is known at build time, perhaps better to add it to the 
iio_priv() accessed structure and avoid having to handle allocations
separately.

> +	if (!msg)
> +		return -ENOMEM;
> +
> +	msg->command = EC_CMD_MKBP_INFO;
> +	msg->version = 1;
> +	msg->outsize = sizeof(*params);
> +	msg->insize = sizeof(u32);
> +	params = (struct ec_params_mkbp_info *)msg->data;
> +	params->info_type = EC_MKBP_INFO_CURRENT;
> +	params->event_type = EC_MKBP_EVENT_SWITCH;
> +
> +	ret = cros_ec_cmd_xfer_status(ec_dev, msg);
> +	if (ret >= 0) {
> +		if (ret != sizeof(u32)) {
> +			dev_warn(ec_dev->dev, "wrong result size: %d != %zu\n",
> +				 ret, sizeof(u32));
> +			ret = -EPROTO;
> +		} else {
> +			*state = cros_ec_proximity_parse_state(msg->data);
> +			ret = 0;
If you move the allocation as suggested above, this can become

if (ret < 0)
	return ret;

if (ret != sizeof(u32)) {
	...
	return -EPROTO;
}

*state = cros_..
return 0;

Which will be neater and not as deeply nested.

> +		}
> +	}
> +
> +	kfree(msg);
> +
> +	return ret;
> +}
> +
> +static int cros_ec_proximity_notify(struct notifier_block *nb,
> +			     unsigned long queued_during_suspend, void *_ec)
> +{
> +	struct cros_ec_proximity_data *data;
> +	struct cros_ec_device *ec = _ec;
> +	u8 event_type = ec->event_data.event_type & EC_MKBP_EVENT_TYPE_MASK;
> +	void *switches = &ec->event_data.data.switches;
> +	struct iio_dev *indio_dev;
> +	s64 timestamp;
> +	int state, dir;
> +	u64 ev;
> +
> +	if (event_type == EC_MKBP_EVENT_SWITCH) {
> +		data = container_of(nb, struct cros_ec_proximity_data, notifier);
> +		indio_dev = data->indio_dev;
> +
> +		mutex_lock(&data->lock);
> +		if (data->enabled) {
> +			timestamp = iio_get_time_ns(indio_dev);
> +			state = cros_ec_proximity_parse_state(switches);
> +			dir = state ? IIO_EV_DIR_FALLING : IIO_EV_DIR_RISING;
> +
> +			ev = IIO_UNMOD_EVENT_CODE(IIO_PROXIMITY, 0,
> +						  IIO_EV_TYPE_THRESH, dir);
> +			iio_push_event(indio_dev, ev, timestamp);
> +		}
> +		mutex_unlock(&data->lock);
> +	}
> +
> +	return NOTIFY_OK;
> +}
> +
> +static int cros_ec_proximity_read_raw(struct iio_dev *indio_dev,
> +			   const struct iio_chan_spec *chan, int *val,
> +			   int *val2, long mask)
> +{
> +	struct cros_ec_proximity_data *data = iio_priv(indio_dev);
> +	struct cros_ec_device *ec = data->ec;
> +	int ret;
> +
> +	if (chan->type != IIO_PROXIMITY)
> +		return -EINVAL;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		ret = iio_device_claim_direct_mode(indio_dev);

Normally we only introduce these protections when adding the ability
to change the state from direct to buffered (which these prevent).
This driver doesn't yet support any other modes so I don't think
there is any benefit in having these.

If the aim is more local protection then should use a local lock
as the semantics fo these functions might change in future.


> +		if (ret)
> +			return ret;
> +
> +		ret = cros_ec_proximity_query(ec, val);
> +		iio_device_release_direct_mode(indio_dev);
> +		if (ret)
> +			return ret;
> +
> +		return IIO_VAL_INT;
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +static int cros_ec_proximity_read_event_config(struct iio_dev *indio_dev,
> +				    const struct iio_chan_spec *chan,
> +				    enum iio_event_type type,
> +				    enum iio_event_direction dir)
> +{
> +	struct cros_ec_proximity_data *data = iio_priv(indio_dev);
> +
> +	return data->enabled;
> +}
> +
> +static int cros_ec_proximity_write_event_config(struct iio_dev *indio_dev,
> +				     const struct iio_chan_spec *chan,
> +				     enum iio_event_type type,
> +				     enum iio_event_direction dir, int state)
> +{
> +	struct cros_ec_proximity_data *data = iio_priv(indio_dev);
> +
> +	mutex_lock(&data->lock);
> +	data->enabled = state;
> +	mutex_unlock(&data->lock);
> +
> +	return 0;
> +}
> +
> +static const struct iio_info cros_ec_proximity_info = {
> +	.read_raw = cros_ec_proximity_read_raw,
> +	.read_event_config = cros_ec_proximity_read_event_config,
> +	.write_event_config = cros_ec_proximity_write_event_config,
> +};
> +
> +static int cros_ec_proximity_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct cros_ec_device *ec = dev_get_drvdata(dev->parent);
> +	struct iio_dev *indio_dev;
> +	struct cros_ec_proximity_data *data;
> +	int ret;
> +
> +	indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	data = iio_priv(indio_dev);
> +	data->ec = ec;
> +	data->indio_dev = indio_dev;
> +	mutex_init(&data->lock);
> +	platform_set_drvdata(pdev, data);
> +
> +	indio_dev->name = "cros_ec_proximity";
> +	indio_dev->dev.parent = dev;
> +	indio_dev->info = &cros_ec_proximity_info;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->channels = cros_ec_prox_chan_spec;
> +	indio_dev->num_channels = ARRAY_SIZE(cros_ec_prox_chan_spec);
> +
> +	ret = devm_iio_device_register(dev, indio_dev);
> +	if (ret)
> +		return ret;
> +
> +	data->notifier.notifier_call = cros_ec_proximity_notify;
> +	ret = blocking_notifier_chain_register(&ec->event_notifier,
> +					       &data->notifier);
> +	if (ret)
> +		dev_err(dev, "cannot register notifier: %d\n", ret);
> +
> +	return ret;
> +}
> +
> +static int cros_ec_proximity_remove(struct platform_device *pdev)
> +{
> +	struct cros_ec_proximity_data *data = platform_get_drvdata(pdev);
> +	struct cros_ec_device *ec = data->ec;
> +
> +	blocking_notifier_chain_unregister(&ec->event_notifier,
> +					   &data->notifier);
> +
> +	return 0;
> +}
> +
> +#ifdef CONFIG_OF

As a general rule, we are trying to clear out protections on CONFIG_OF etc
and use of of_match_ptr() on the basis they don't really gain us anything
and prevent use of some other firmware types.  Here I guess you know what
your firmware looks like, but I'm still keen to drop it in the interests
of there being fewer places to copy it from.

It may be a good idea to give this a more specific name as well given
we already have cros-ec-prox as a platform device id from
the cros_ec_light_prox driver.


> +static const struct of_device_id cros_ec_proximity_of_match[] = {
> +	{ .compatible = "google,cros-ec-proximity" },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, cros_ec_proximity_of_match);
> +#endif
> +
> +static struct platform_driver cros_ec_proximity_driver = {
> +	.driver = {
> +		.name = "cros-ec-proximity",
> +		.of_match_table = of_match_ptr(cros_ec_proximity_of_match),
> +	},
> +	.probe = cros_ec_proximity_probe,
> +	.remove = cros_ec_proximity_remove,
> +};
> +module_platform_driver(cros_ec_proximity_driver);
> +
> +MODULE_LICENSE("GPL v2");
> +MODULE_DESCRIPTION("ChromeOS EC MKBP proximity sensor driver");

Re: [PATCH 3/3] iio: proximity: Add a ChromeOS EC MKBP proximity driver

[Gwendal Grignou]

On Sun, Jan 24, 2021 at 9:38 AM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Fri, 22 Jan 2021 14:54:43 -0800
> Stephen Boyd <swboyd@chromium.org> wrote:
>
> > Add support for a ChromeOS EC proximity driver that exposes a "front"
> > proximity sensor via the IIO subsystem. The EC decides when front
> > proximity is near and sets an MKBP switch 'EC_MKBP_FRONT_PROXIMITY' to
> > notify the kernel of proximity. Similarly, when proximity detects
> > something far away it sets the switch bit to 0. For now this driver
> > exposes a single sensor, but it could be expanded in the future via more
> > MKBP bits if desired.
> >
> > Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > Cc: Benson Leung <bleung@chromium.org>
> > Cc: Guenter Roeck <groeck@chromium.org>
> > Cc: Douglas Anderson <dianders@chromium.org>
> > Cc: Gwendal Grignou <gwendal@chromium.org>
> > Signed-off-by: Stephen Boyd <swboyd@chromium.org>
>
> Hi Stephen,
>
> Looks more or less fine to me.  My main concern is potential confusion
> in naming with the cros_ec_prox_light driver that we already have.
>
> A few other minor bits and bobs inline.
>
> thanks,
>
> Jonathan
>
>
> > ---
> >  drivers/iio/proximity/Kconfig             |  11 +
> >  drivers/iio/proximity/Makefile            |   1 +
> >  drivers/iio/proximity/cros_ec_proximity.c | 252 ++++++++++++++++++++++
> >  3 files changed, 264 insertions(+)
> >  create mode 100644 drivers/iio/proximity/cros_ec_proximity.c
> >
> > diff --git a/drivers/iio/proximity/Kconfig b/drivers/iio/proximity/Kconfig
> > index 12672a0e89ed..35a04e9ede7d 100644
> > --- a/drivers/iio/proximity/Kconfig
> > +++ b/drivers/iio/proximity/Kconfig
> > @@ -21,6 +21,17 @@ endmenu
> >
> >  menu "Proximity and distance sensors"
> >
> > +config CROS_EC_PROXIMITY
> > +     tristate "ChromeOS EC MKBP Proximity sensor"
> > +     depends on CROS_EC
> > +     help
> > +       Say Y here to enable the proximity sensor implemented via the ChromeOS EC MKBP
> > +       switches protocol. You must enable one bus option (CROS_EC_I2C or CROS_EC_SPI)
> > +       to use this.
> > +
> > +       To compile this driver as a module, choose M here: the
> > +       module will be called cros_ec_prox.
> > +
> >  config ISL29501
> >       tristate "Intersil ISL29501 Time Of Flight sensor"
> >       depends on I2C
> > diff --git a/drivers/iio/proximity/Makefile b/drivers/iio/proximity/Makefile
> > index 9c1aca1a8b79..b1330dd8e212 100644
> > --- a/drivers/iio/proximity/Makefile
> > +++ b/drivers/iio/proximity/Makefile
> > @@ -5,6 +5,7 @@
> >
> >  # When adding new entries keep the list in alphabetical order
> >  obj-$(CONFIG_AS3935)         += as3935.o
> > +obj-$(CONFIG_CROS_EC_PROXIMITY)      += cros_ec_proximity.o
> >  obj-$(CONFIG_ISL29501)               += isl29501.o
> >  obj-$(CONFIG_LIDAR_LITE_V2)  += pulsedlight-lidar-lite-v2.o
> >  obj-$(CONFIG_MB1232)         += mb1232.o
> > diff --git a/drivers/iio/proximity/cros_ec_proximity.c b/drivers/iio/proximity/cros_ec_proximity.c
> > new file mode 100644
> > index 000000000000..a3aef911e3cc
> > --- /dev/null
> > +++ b/drivers/iio/proximity/cros_ec_proximity.c
> > @@ -0,0 +1,252 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Driver for cros-ec proximity sensor exposed through MKBP switch
> > + *
> > + * Copyright 2021 Google LLC.
> > + */
> > +
> > +#include <linux/module.h>
> > +#include <linux/mutex.h>
> > +#include <linux/kernel.h>
>
> Slight preference for alphabetical order though keeping specific
> sections separate as done here is fine.
>
> > +#include <linux/notifier.h>
> > +#include <linux/of.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/slab.h>
> > +#include <linux/types.h>
> > +
> > +#include <linux/platform_data/cros_ec_commands.h>
> > +#include <linux/platform_data/cros_ec_proto.h>
> > +
> > +#include <linux/iio/events.h>
> > +#include <linux/iio/iio.h>
> > +#include <linux/iio/sysfs.h>
> > +
> > +#include <asm/unaligned.h>
> > +
> > +struct cros_ec_proximity_data {
> > +     struct cros_ec_device *ec;
> > +     struct iio_dev *indio_dev;
> > +     struct mutex lock;
> > +     struct notifier_block notifier;
> > +     bool enabled;
> > +};
> > +
> > +static const struct iio_event_spec cros_ec_prox_events[] = {
> > +     {
> > +             .type = IIO_EV_TYPE_THRESH,
> > +             .dir = IIO_EV_DIR_EITHER,
> > +             .mask_separate = BIT(IIO_EV_INFO_ENABLE),
> > +     },
> > +};
> > +
> > +static const struct iio_chan_spec cros_ec_prox_chan_spec[] = {
> > +     {
> > +             .type = IIO_PROXIMITY,
> > +             .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> > +             .event_spec = cros_ec_prox_events,
> > +             .num_event_specs = ARRAY_SIZE(cros_ec_prox_events),
> > +     },
> > +};
> > +
> > +static int cros_ec_proximity_parse_state(const void *data)
> > +{
> > +     u32 switches = get_unaligned_le32(data);
> > +
> > +     return !!(switches & BIT(EC_MKBP_FRONT_PROXIMITY));
> > +}
> > +
> > +static int cros_ec_proximity_query(struct cros_ec_device *ec_dev, int *state)
> > +{
> > +     struct ec_params_mkbp_info *params;
> > +     struct cros_ec_command *msg;
> > +     int ret;
> > +
> > +     msg = kzalloc(sizeof(*msg) + max(sizeof(u32), sizeof(*params)),
> > +                   GFP_KERNEL);
>
> Given this is known at build time, perhaps better to add it to the
> iio_priv() accessed structure and avoid having to handle allocations
> separately.
As Jonathan said, it can be preallocated in iio private structure. We
can also use the stack, given the response size is known beforehand.
See cros_ec_cec_set_log_addr() or cros_ec_pwm_get_duty() for example.
>
> > +     if (!msg)
> > +             return -ENOMEM;
> > +
> > +     msg->command = EC_CMD_MKBP_INFO;
> > +     msg->version = 1;
> > +     msg->outsize = sizeof(*params);
> > +     msg->insize = sizeof(u32);
> > +     params = (struct ec_params_mkbp_info *)msg->data;
> > +     params->info_type = EC_MKBP_INFO_CURRENT;
> > +     params->event_type = EC_MKBP_EVENT_SWITCH;
> > +
> > +     ret = cros_ec_cmd_xfer_status(ec_dev, msg);
> > +     if (ret >= 0) {
> > +             if (ret != sizeof(u32)) {
> > +                     dev_warn(ec_dev->dev, "wrong result size: %d != %zu\n",
> > +                              ret, sizeof(u32));
> > +                     ret = -EPROTO;
> > +             } else {
> > +                     *state = cros_ec_proximity_parse_state(msg->data);
> > +                     ret = 0;
> If you move the allocation as suggested above, this can become
>
> if (ret < 0)
>         return ret;
>
> if (ret != sizeof(u32)) {
>         ...
>         return -EPROTO;
> }
>
> *state = cros_..
> return 0;
>
> Which will be neater and not as deeply nested.
>
> > +             }
> > +     }
> > +
> > +     kfree(msg);
> > +
> > +     return ret;
> > +}
> > +
> > +static int cros_ec_proximity_notify(struct notifier_block *nb,
> > +                          unsigned long queued_during_suspend, void *_ec)
> > +{
> > +     struct cros_ec_proximity_data *data;
> > +     struct cros_ec_device *ec = _ec;
> > +     u8 event_type = ec->event_data.event_type & EC_MKBP_EVENT_TYPE_MASK;
> > +     void *switches = &ec->event_data.data.switches;
> > +     struct iio_dev *indio_dev;
> > +     s64 timestamp;
> > +     int state, dir;
> > +     u64 ev;
> > +
> > +     if (event_type == EC_MKBP_EVENT_SWITCH) {
> > +             data = container_of(nb, struct cros_ec_proximity_data, notifier);
> > +             indio_dev = data->indio_dev;
> > +
> > +             mutex_lock(&data->lock);
> > +             if (data->enabled) {
> > +                     timestamp = iio_get_time_ns(indio_dev);
For Android, given the timestamp must be time it happens, not reported
[https://source.android.com/devices/sensors/sensors-hal2] """The
timestamp must be accurate and correspond to the time at which the
event physically happened, not the time it was reported.""", consider
using ec_dev->last_event_time and apply a delta if the iio clock base
is different from CLOCK_BOOTTIME.
> > +                     state = cros_ec_proximity_parse_state(switches);
> > +                     dir = state ? IIO_EV_DIR_FALLING : IIO_EV_DIR_RISING;
> > +
> > +                     ev = IIO_UNMOD_EVENT_CODE(IIO_PROXIMITY, 0,
> > +                                               IIO_EV_TYPE_THRESH, dir);
> > +                     iio_push_event(indio_dev, ev, timestamp);
> > +             }
> > +             mutex_unlock(&data->lock);
> > +     }
> > +
> > +     return NOTIFY_OK;
> > +}
> > +
> > +static int cros_ec_proximity_read_raw(struct iio_dev *indio_dev,
> > +                        const struct iio_chan_spec *chan, int *val,
> > +                        int *val2, long mask)
> > +{
> > +     struct cros_ec_proximity_data *data = iio_priv(indio_dev);
> > +     struct cros_ec_device *ec = data->ec;
> > +     int ret;
> > +
> > +     if (chan->type != IIO_PROXIMITY)
> > +             return -EINVAL;
> > +
> > +     switch (mask) {
> > +     case IIO_CHAN_INFO_RAW:
> > +             ret = iio_device_claim_direct_mode(indio_dev);
>
> Normally we only introduce these protections when adding the ability
> to change the state from direct to buffered (which these prevent).
> This driver doesn't yet support any other modes so I don't think
> there is any benefit in having these.
>
> If the aim is more local protection then should use a local lock
> as the semantics fo these functions might change in future.
>
>
> > +             if (ret)
> > +                     return ret;
> > +
> > +             ret = cros_ec_proximity_query(ec, val);
> > +             iio_device_release_direct_mode(indio_dev);
> > +             if (ret)
> > +                     return ret;
> > +
> > +             return IIO_VAL_INT;
> > +     }
> > +
> > +     return -EINVAL;
> > +}
> > +
> > +static int cros_ec_proximity_read_event_config(struct iio_dev *indio_dev,
> > +                                 const struct iio_chan_spec *chan,
> > +                                 enum iio_event_type type,
> > +                                 enum iio_event_direction dir)
> > +{
> > +     struct cros_ec_proximity_data *data = iio_priv(indio_dev);
> > +
> > +     return data->enabled;
> > +}
> > +
> > +static int cros_ec_proximity_write_event_config(struct iio_dev *indio_dev,
> > +                                  const struct iio_chan_spec *chan,
> > +                                  enum iio_event_type type,
> > +                                  enum iio_event_direction dir, int state)
> > +{
> > +     struct cros_ec_proximity_data *data = iio_priv(indio_dev);
> > +
> > +     mutex_lock(&data->lock);
> > +     data->enabled = state;
> > +     mutex_unlock(&data->lock);
> > +
> > +     return 0;
> > +}
> > +
> > +static const struct iio_info cros_ec_proximity_info = {
> > +     .read_raw = cros_ec_proximity_read_raw,
> > +     .read_event_config = cros_ec_proximity_read_event_config,
> > +     .write_event_config = cros_ec_proximity_write_event_config,
> > +};
> > +
> > +static int cros_ec_proximity_probe(struct platform_device *pdev)
> > +{
> > +     struct device *dev = &pdev->dev;
> > +     struct cros_ec_device *ec = dev_get_drvdata(dev->parent);
> > +     struct iio_dev *indio_dev;
> > +     struct cros_ec_proximity_data *data;
> > +     int ret;
> > +
> > +     indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
> > +     if (!indio_dev)
> > +             return -ENOMEM;
> > +
> > +     data = iio_priv(indio_dev);
> > +     data->ec = ec;
> > +     data->indio_dev = indio_dev;
> > +     mutex_init(&data->lock);
> > +     platform_set_drvdata(pdev, data);
> > +
> > +     indio_dev->name = "cros_ec_proximity";
Define a constant CROS_EC_[MKBP_]PROXIMITY_DRIVER_NAME and use it here
and in struct platform_driver cros_ec_proximity_driver.
> > +     indio_dev->dev.parent = dev;
Not needed, done by iio_device_alloc(), called by devm_iio_device_alloc().
> > +     indio_dev->info = &cros_ec_proximity_info;
> > +     indio_dev->modes = INDIO_DIRECT_MODE;
> > +     indio_dev->channels = cros_ec_prox_chan_spec;
> > +     indio_dev->num_channels = ARRAY_SIZE(cros_ec_prox_chan_spec);
> > +
> > +     ret = devm_iio_device_register(dev, indio_dev);
> > +     if (ret)
> > +             return ret;
> > +
> > +     data->notifier.notifier_call = cros_ec_proximity_notify;
> > +     ret = blocking_notifier_chain_register(&ec->event_notifier,
> > +                                            &data->notifier);
> > +     if (ret)
> > +             dev_err(dev, "cannot register notifier: %d\n", ret);
> > +
> > +     return ret;
> > +}
> > +
> > +static int cros_ec_proximity_remove(struct platform_device *pdev)
> > +{
> > +     struct cros_ec_proximity_data *data = platform_get_drvdata(pdev);
> > +     struct cros_ec_device *ec = data->ec;
> > +
> > +     blocking_notifier_chain_unregister(&ec->event_notifier,
> > +                                        &data->notifier);
> > +
> > +     return 0;
> > +}
> > +
> > +#ifdef CONFIG_OF
>
> As a general rule, we are trying to clear out protections on CONFIG_OF etc
> and use of of_match_ptr() on the basis they don't really gain us anything
> and prevent use of some other firmware types.  Here I guess you know what
> your firmware looks like, but I'm still keen to drop it in the interests
> of there being fewer places to copy it from.
>
> It may be a good idea to give this a more specific name as well given
> we already have cros-ec-prox as a platform device id from
> the cros_ec_light_prox driver.
>
>
> > +static const struct of_device_id cros_ec_proximity_of_match[] = {
> > +     { .compatible = "google,cros-ec-proximity" },
> > +     {}
> > +};
> > +MODULE_DEVICE_TABLE(of, cros_ec_proximity_of_match);
> > +#endif
> > +
> > +static struct platform_driver cros_ec_proximity_driver = {
> > +     .driver = {
> > +             .name = "cros-ec-proximity",
> > +             .of_match_table = of_match_ptr(cros_ec_proximity_of_match),
Add a ACPI match table to match.
> > +     },
> > +     .probe = cros_ec_proximity_probe,
> > +     .remove = cros_ec_proximity_remove,
> > +};
> > +module_platform_driver(cros_ec_proximity_driver);
> > +
> > +MODULE_LICENSE("GPL v2");
> > +MODULE_DESCRIPTION("ChromeOS EC MKBP proximity sensor driver");
>

Re: [PATCH 3/3] iio: proximity: Add a ChromeOS EC MKBP proximity driver

[Stephen Boyd]

Quoting Gwendal Grignou (2021-01-24 13:41:44)
> On Sun, Jan 24, 2021 at 9:38 AM Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > On Fri, 22 Jan 2021 14:54:43 -0800
> > Stephen Boyd <swboyd@chromium.org> wrote:
> >
> > > ---
> > >  drivers/iio/proximity/Kconfig             |  11 +
> > >  drivers/iio/proximity/Makefile            |   1 +
> > >  drivers/iio/proximity/cros_ec_proximity.c | 252 ++++++++++++++++++++++

I suppose I'll change this to cros_ec_mkbp_proximity as well.

> > > diff --git a/drivers/iio/proximity/cros_ec_proximity.c b/drivers/iio/proximity/cros_ec_proximity.c
> > > new file mode 100644
> > > index 000000000000..a3aef911e3cc
> > > --- /dev/null
> > > +++ b/drivers/iio/proximity/cros_ec_proximity.c
> > > @@ -0,0 +1,252 @@
[...]
> > > +
> > > +static int cros_ec_proximity_query(struct cros_ec_device *ec_dev, int *state)
> > > +{
> > > +     struct ec_params_mkbp_info *params;
> > > +     struct cros_ec_command *msg;
> > > +     int ret;
> > > +
> > > +     msg = kzalloc(sizeof(*msg) + max(sizeof(u32), sizeof(*params)),
> > > +                   GFP_KERNEL);
> >
> > Given this is known at build time, perhaps better to add it to the
> > iio_priv() accessed structure and avoid having to handle allocations
> > separately.
> As Jonathan said, it can be preallocated in iio private structure. We
> can also use the stack, given the response size is known beforehand.
> See cros_ec_cec_set_log_addr() or cros_ec_pwm_get_duty() for example.

I suppose stack is even simpler. I'll try that.

> > > +
> > > +static int cros_ec_proximity_notify(struct notifier_block *nb,
> > > +                          unsigned long queued_during_suspend, void *_ec)
> > > +{
> > > +     struct cros_ec_proximity_data *data;
> > > +     struct cros_ec_device *ec = _ec;
> > > +     u8 event_type = ec->event_data.event_type & EC_MKBP_EVENT_TYPE_MASK;
> > > +     void *switches = &ec->event_data.data.switches;
> > > +     struct iio_dev *indio_dev;
> > > +     s64 timestamp;
> > > +     int state, dir;
> > > +     u64 ev;
> > > +
> > > +     if (event_type == EC_MKBP_EVENT_SWITCH) {
> > > +             data = container_of(nb, struct cros_ec_proximity_data, notifier);
> > > +             indio_dev = data->indio_dev;
> > > +
> > > +             mutex_lock(&data->lock);
> > > +             if (data->enabled) {
> > > +                     timestamp = iio_get_time_ns(indio_dev);
> For Android, given the timestamp must be time it happens, not reported
> [https://source.android.com/devices/sensors/sensors-hal2] """The
> timestamp must be accurate and correspond to the time at which the
> event physically happened, not the time it was reported.""", consider
> using ec_dev->last_event_time and apply a delta if the iio clock base
> is different from CLOCK_BOOTTIME.

Ah alright. Is there a reason why cros_ec_get_time_ns() is using
boottime instead of plain ktime_get(), i.e. CLOCK_MONOTONIC? Otherwise I
suppose some sort of cros_ec API should be exposed to convert the
last_event_time to whatever clock base is desired. Does that exist?

> > > +static int cros_ec_proximity_probe(struct platform_device *pdev)
> > > +{
> > > +     struct device *dev = &pdev->dev;
> > > +     struct cros_ec_device *ec = dev_get_drvdata(dev->parent);
> > > +     struct iio_dev *indio_dev;
> > > +     struct cros_ec_proximity_data *data;
> > > +     int ret;
> > > +
> > > +     indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
> > > +     if (!indio_dev)
> > > +             return -ENOMEM;
> > > +
> > > +     data = iio_priv(indio_dev);
> > > +     data->ec = ec;
> > > +     data->indio_dev = indio_dev;
> > > +     mutex_init(&data->lock);
> > > +     platform_set_drvdata(pdev, data);
> > > +
> > > +     indio_dev->name = "cros_ec_proximity";
> Define a constant CROS_EC_[MKBP_]PROXIMITY_DRIVER_NAME and use it here
> and in struct platform_driver cros_ec_proximity_driver.

I used dev->driver->name instead. Yay for no define!

> > > +     indio_dev->dev.parent = dev;
> Not needed, done by iio_device_alloc(), called by devm_iio_device_alloc().

Ok.

> > > +static const struct of_device_id cros_ec_proximity_of_match[] = {
> > > +     { .compatible = "google,cros-ec-proximity" },
> > > +     {}
> > > +};
> > > +MODULE_DEVICE_TABLE(of, cros_ec_proximity_of_match);
> > > +#endif
> > > +
> > > +static struct platform_driver cros_ec_proximity_driver = {
> > > +     .driver = {
> > > +             .name = "cros-ec-proximity",
> > > +             .of_match_table = of_match_ptr(cros_ec_proximity_of_match),
> Add a ACPI match table to match.

I don't have an ACPI system in hand. What should the ACPI table look
like? Can ACPI use the of_match_table logic?

> > > +     },
> > > +     .probe = cros_ec_proximity_probe,
> > > +     .remove = cros_ec_proximity_remove,
> > > +};

Re: [PATCH 3/3] iio: proximity: Add a ChromeOS EC MKBP proximity driver

[Gwendal Grignou]

On Mon, Jan 25, 2021 at 10:52 AM Stephen Boyd <swboyd@chromium.org> wrote:
>
> Quoting Gwendal Grignou (2021-01-24 13:41:44)
> > On Sun, Jan 24, 2021 at 9:38 AM Jonathan Cameron <jic23@kernel.org> wrote:
> > >
> > > On Fri, 22 Jan 2021 14:54:43 -0800
> > > Stephen Boyd <swboyd@chromium.org> wrote:
> > >
> > > > ---
> > > >  drivers/iio/proximity/Kconfig             |  11 +
> > > >  drivers/iio/proximity/Makefile            |   1 +
> > > >  drivers/iio/proximity/cros_ec_proximity.c | 252 ++++++++++++++++++++++
>
> I suppose I'll change this to cros_ec_mkbp_proximity as well.
>
> > > > diff --git a/drivers/iio/proximity/cros_ec_proximity.c b/drivers/iio/proximity/cros_ec_proximity.c
> > > > new file mode 100644
> > > > index 000000000000..a3aef911e3cc
> > > > --- /dev/null
> > > > +++ b/drivers/iio/proximity/cros_ec_proximity.c
> > > > @@ -0,0 +1,252 @@
> [...]
> > > > +
> > > > +static int cros_ec_proximity_query(struct cros_ec_device *ec_dev, int *state)
> > > > +{
> > > > +     struct ec_params_mkbp_info *params;
> > > > +     struct cros_ec_command *msg;
> > > > +     int ret;
> > > > +
> > > > +     msg = kzalloc(sizeof(*msg) + max(sizeof(u32), sizeof(*params)),
> > > > +                   GFP_KERNEL);
> > >
> > > Given this is known at build time, perhaps better to add it to the
> > > iio_priv() accessed structure and avoid having to handle allocations
> > > separately.
> > As Jonathan said, it can be preallocated in iio private structure. We
> > can also use the stack, given the response size is known beforehand.
> > See cros_ec_cec_set_log_addr() or cros_ec_pwm_get_duty() for example.
>
> I suppose stack is even simpler. I'll try that.
>
> > > > +
> > > > +static int cros_ec_proximity_notify(struct notifier_block *nb,
> > > > +                          unsigned long queued_during_suspend, void *_ec)
> > > > +{
> > > > +     struct cros_ec_proximity_data *data;
> > > > +     struct cros_ec_device *ec = _ec;
> > > > +     u8 event_type = ec->event_data.event_type & EC_MKBP_EVENT_TYPE_MASK;
> > > > +     void *switches = &ec->event_data.data.switches;
> > > > +     struct iio_dev *indio_dev;
> > > > +     s64 timestamp;
> > > > +     int state, dir;
> > > > +     u64 ev;
> > > > +
> > > > +     if (event_type == EC_MKBP_EVENT_SWITCH) {
> > > > +             data = container_of(nb, struct cros_ec_proximity_data, notifier);
> > > > +             indio_dev = data->indio_dev;
> > > > +
> > > > +             mutex_lock(&data->lock);
> > > > +             if (data->enabled) {
> > > > +                     timestamp = iio_get_time_ns(indio_dev);
> > For Android, given the timestamp must be time it happens, not reported
> > [https://source.android.com/devices/sensors/sensors-hal2] """The
> > timestamp must be accurate and correspond to the time at which the
> > event physically happened, not the time it was reported.""", consider
> > using ec_dev->last_event_time and apply a delta if the iio clock base
> > is different from CLOCK_BOOTTIME.
>
> Ah alright. Is there a reason why cros_ec_get_time_ns() is using
> boottime instead of plain ktime_get(), i.e. CLOCK_MONOTONIC? Otherwise I
> suppose some sort of cros_ec API should be exposed to convert the
> last_event_time to whatever clock base is desired. Does that exist?
CLOCK_BOOTTIME was chosen to be Android compliant, as it includes
suspend time and match elapsedRealtime() [see
https://developer.android.com/reference/android/os/SystemClock#elapsedRealtime()]
Chromebook set iio clock reference for all sensor to CLOCK_BOOTTIME
(see https://chromium.googlesource.com/chromiumos/platform2/+/HEAD/mems_setup/configuration.cc#127).
In case the iio device clock_id is not CLOCK_BOOTTIME/"bootime", we
need to add a delta, like in cros_ec_sensors_push_data()
[https://elixir.bootlin.com/linux/latest/source/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c#L210]
>
> > > > +static int cros_ec_proximity_probe(struct platform_device *pdev)
> > > > +{
> > > > +     struct device *dev = &pdev->dev;
> > > > +     struct cros_ec_device *ec = dev_get_drvdata(dev->parent);
> > > > +     struct iio_dev *indio_dev;
> > > > +     struct cros_ec_proximity_data *data;
> > > > +     int ret;
> > > > +
> > > > +     indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
> > > > +     if (!indio_dev)
> > > > +             return -ENOMEM;
> > > > +
> > > > +     data = iio_priv(indio_dev);
> > > > +     data->ec = ec;
> > > > +     data->indio_dev = indio_dev;
> > > > +     mutex_init(&data->lock);
> > > > +     platform_set_drvdata(pdev, data);
> > > > +
> > > > +     indio_dev->name = "cros_ec_proximity";
> > Define a constant CROS_EC_[MKBP_]PROXIMITY_DRIVER_NAME and use it here
> > and in struct platform_driver cros_ec_proximity_driver.
>
> I used dev->driver->name instead. Yay for no define!
>
> > > > +     indio_dev->dev.parent = dev;
> > Not needed, done by iio_device_alloc(), called by devm_iio_device_alloc().
>
> Ok.
>
> > > > +static const struct of_device_id cros_ec_proximity_of_match[] = {
> > > > +     { .compatible = "google,cros-ec-proximity" },
> > > > +     {}
> > > > +};
> > > > +MODULE_DEVICE_TABLE(of, cros_ec_proximity_of_match);
> > > > +#endif
> > > > +
> > > > +static struct platform_driver cros_ec_proximity_driver = {
> > > > +     .driver = {
> > > > +             .name = "cros-ec-proximity",
> > > > +             .of_match_table = of_match_ptr(cros_ec_proximity_of_match),
> > Add a ACPI match table to match.
>
> I don't have an ACPI system in hand. What should the ACPI table look
> like? Can ACPI use the of_match_table logic?
AFAIK, ACPI uses .acpi_match_table, see
drivers/iio/magnetometer/ak8975.c for a simple example.


>
> > > > +     },
> > > > +     .probe = cros_ec_proximity_probe,
> > > > +     .remove = cros_ec_proximity_remove,
> > > > +};

Re: [PATCH 3/3] iio: proximity: Add a ChromeOS EC MKBP proximity driver

[Stephen Boyd]

Quoting Gwendal Grignou (2021-01-25 14:28:46)
> On Mon, Jan 25, 2021 at 10:52 AM Stephen Boyd <swboyd@chromium.org> wrote:
> >
> > Quoting Gwendal Grignou (2021-01-24 13:41:44)
> > > On Sun, Jan 24, 2021 at 9:38 AM Jonathan Cameron <jic23@kernel.org> wrote:
> > > >
> > > > On Fri, 22 Jan 2021 14:54:43 -0800
> > > > Stephen Boyd <swboyd@chromium.org> wrote:
> > > > > +     if (event_type == EC_MKBP_EVENT_SWITCH) {
> > > > > +             data = container_of(nb, struct cros_ec_proximity_data, notifier);
> > > > > +             indio_dev = data->indio_dev;
> > > > > +
> > > > > +             mutex_lock(&data->lock);
> > > > > +             if (data->enabled) {
> > > > > +                     timestamp = iio_get_time_ns(indio_dev);
> > > For Android, given the timestamp must be time it happens, not reported
> > > [https://source.android.com/devices/sensors/sensors-hal2] """The
> > > timestamp must be accurate and correspond to the time at which the
> > > event physically happened, not the time it was reported.""", consider
> > > using ec_dev->last_event_time and apply a delta if the iio clock base
> > > is different from CLOCK_BOOTTIME.
> >
> > Ah alright. Is there a reason why cros_ec_get_time_ns() is using
> > boottime instead of plain ktime_get(), i.e. CLOCK_MONOTONIC? Otherwise I
> > suppose some sort of cros_ec API should be exposed to convert the
> > last_event_time to whatever clock base is desired. Does that exist?
> CLOCK_BOOTTIME was chosen to be Android compliant, as it includes
> suspend time and match elapsedRealtime() [see
> https://developer.android.com/reference/android/os/SystemClock#elapsedRealtime()]
> Chromebook set iio clock reference for all sensor to CLOCK_BOOTTIME
> (see https://chromium.googlesource.com/chromiumos/platform2/+/HEAD/mems_setup/configuration.cc#127).
> In case the iio device clock_id is not CLOCK_BOOTTIME/"bootime", we
> need to add a delta, like in cros_ec_sensors_push_data()
> [https://elixir.bootlin.com/linux/latest/source/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c#L210]

The delta may help but what if the clock is adjusted in the time between
the event is timestamped and this driver reading the timestamp? That
could lead to some odd behavior where the timestamp is in the future
because we don't know what sort of adjustment was made, e.g. the
realtime clock being moved back in time.

I'd rather have a way to request that cros_ec core timestamp the event
with some clock base that this driver desires, instead of having to do
an offset after the fact. For now I'll use ec_dev->last_event_time
because this is only used on chromeos and that should work until
userspace is changed, but in the future I think we'll need to have a way
for this IIO device to be notified when the clock base changes in
iio_device_set_clock() and then have this driver call into cros_ec to
request that such a timestamp be made when this event is seen. Or even
better have a way to request that cros_ec timestamp the event itself on
the EC side, but I don't know if that's possible.

> > > > > +static const struct of_device_id cros_ec_proximity_of_match[] = {
> > > > > +     { .compatible = "google,cros-ec-proximity" },
> > > > > +     {}
> > > > > +};
> > > > > +MODULE_DEVICE_TABLE(of, cros_ec_proximity_of_match);
> > > > > +#endif
> > > > > +
> > > > > +static struct platform_driver cros_ec_proximity_driver = {
> > > > > +     .driver = {
> > > > > +             .name = "cros-ec-proximity",
> > > > > +             .of_match_table = of_match_ptr(cros_ec_proximity_of_match),
> > > Add a ACPI match table to match.
> >
> > I don't have an ACPI system in hand. What should the ACPI table look
> > like? Can ACPI use the of_match_table logic?
> AFAIK, ACPI uses .acpi_match_table, see
> drivers/iio/magnetometer/ak8975.c for a simple example.

Ok. I'm leaning towards punting on this. I don't have an ACPI system to
test and I don't know what the ACPI match table should have in it. If
you can tell me what to put in the acpi_match_table then I can add it.

Re: [PATCH 3/3] iio: proximity: Add a ChromeOS EC MKBP proximity driver

[Gwendal Grignou]

On Mon, Jan 25, 2021 at 3:44 PM Stephen Boyd <swboyd@chromium.org> wrote:
>
> Quoting Gwendal Grignou (2021-01-25 14:28:46)
> > On Mon, Jan 25, 2021 at 10:52 AM Stephen Boyd <swboyd@chromium.org> wrote:
> > >
> > > Quoting Gwendal Grignou (2021-01-24 13:41:44)
> > > > On Sun, Jan 24, 2021 at 9:38 AM Jonathan Cameron <jic23@kernel.org> wrote:
> > > > >
> > > > > On Fri, 22 Jan 2021 14:54:43 -0800
> > > > > Stephen Boyd <swboyd@chromium.org> wrote:
> > > > > > +     if (event_type == EC_MKBP_EVENT_SWITCH) {
> > > > > > +             data = container_of(nb, struct cros_ec_proximity_data, notifier);
> > > > > > +             indio_dev = data->indio_dev;
> > > > > > +
> > > > > > +             mutex_lock(&data->lock);
> > > > > > +             if (data->enabled) {
> > > > > > +                     timestamp = iio_get_time_ns(indio_dev);
> > > > For Android, given the timestamp must be time it happens, not reported
> > > > [https://source.android.com/devices/sensors/sensors-hal2] """The
> > > > timestamp must be accurate and correspond to the time at which the
> > > > event physically happened, not the time it was reported.""", consider
> > > > using ec_dev->last_event_time and apply a delta if the iio clock base
> > > > is different from CLOCK_BOOTTIME.
> > >
> > > Ah alright. Is there a reason why cros_ec_get_time_ns() is using
> > > boottime instead of plain ktime_get(), i.e. CLOCK_MONOTONIC? Otherwise I
> > > suppose some sort of cros_ec API should be exposed to convert the
> > > last_event_time to whatever clock base is desired. Does that exist?
> > CLOCK_BOOTTIME was chosen to be Android compliant, as it includes
> > suspend time and match elapsedRealtime() [see
> > https://developer.android.com/reference/android/os/SystemClock#elapsedRealtime()]
> > Chromebook set iio clock reference for all sensor to CLOCK_BOOTTIME
> > (see https://chromium.googlesource.com/chromiumos/platform2/+/HEAD/mems_setup/configuration.cc#127).
> > In case the iio device clock_id is not CLOCK_BOOTTIME/"bootime", we
> > need to add a delta, like in cros_ec_sensors_push_data()
> > [https://elixir.bootlin.com/linux/latest/source/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c#L210]
>
> The delta may help but what if the clock is adjusted in the time between
> the event is timestamped and this driver reading the timestamp? That
> could lead to some odd behavior where the timestamp is in the future
> because we don't know what sort of adjustment was made, e.g. the
> realtime clock being moved back in time.
>
> I'd rather have a way to request that cros_ec core timestamp the event
> with some clock base that this driver desires,
ec_dev->last_event_time is collected outside of the IIO stack, and can
be used by multiple drivers.

> instead of having to do
> an offset after the fact. For now I'll use ec_dev->last_event_time
> because this is only used on chromeos and that should work until
> userspace is changed, but in the future I think we'll need to have a way
> for this IIO device to be notified when the clock base changes in
> iio_device_set_clock() and then have this driver call into cros_ec to
> request that such a timestamp be made when this event is seen. Or even
> better have a way to request that cros_ec timestamp the event itself on
> the EC side, but I don't know if that's possible.
One way would be use the EC sensor stack that collect such timestamp,
but that would be more firmware changes.

On second thought, to keep it simple and consistent with other IIO
drivers, I suggest to keep using iio_get_time_ns() when the sensor
clock is not CLOCK_BOOTTIME, ec_dev->last_event_time when it is.
>
> > > > > > +static const struct of_device_id cros_ec_proximity_of_match[] = {
> > > > > > +     { .compatible = "google,cros-ec-proximity" },
> > > > > > +     {}
> > > > > > +};
> > > > > > +MODULE_DEVICE_TABLE(of, cros_ec_proximity_of_match);
> > > > > > +#endif
> > > > > > +
> > > > > > +static struct platform_driver cros_ec_proximity_driver = {
> > > > > > +     .driver = {
> > > > > > +             .name = "cros-ec-proximity",
> > > > > > +             .of_match_table = of_match_ptr(cros_ec_proximity_of_match),
> > > > Add a ACPI match table to match.
> > >
> > > I don't have an ACPI system in hand. What should the ACPI table look
> > > like? Can ACPI use the of_match_table logic?
> > AFAIK, ACPI uses .acpi_match_table, see
> > drivers/iio/magnetometer/ak8975.c for a simple example.
>
> Ok. I'm leaning towards punting on this. I don't have an ACPI system to
> test and I don't know what the ACPI match table should have in it. If
> you can tell me what to put in the acpi_match_table then I can add it.

Re: [PATCH 3/3] iio: proximity: Add a ChromeOS EC MKBP proximity driver

[Stephen Boyd]

Quoting Gwendal Grignou (2021-01-26 12:59:50)
> On Mon, Jan 25, 2021 at 3:44 PM Stephen Boyd <swboyd@chromium.org> wrote:
> >
> > instead of having to do
> > an offset after the fact. For now I'll use ec_dev->last_event_time
> > because this is only used on chromeos and that should work until
> > userspace is changed, but in the future I think we'll need to have a way
> > for this IIO device to be notified when the clock base changes in
> > iio_device_set_clock() and then have this driver call into cros_ec to
> > request that such a timestamp be made when this event is seen. Or even
> > better have a way to request that cros_ec timestamp the event itself on
> > the EC side, but I don't know if that's possible.
> One way would be use the EC sensor stack that collect such timestamp,
> but that would be more firmware changes.
> 
> On second thought, to keep it simple and consistent with other IIO
> drivers, I suggest to keep using iio_get_time_ns() when the sensor
> clock is not CLOCK_BOOTTIME, ec_dev->last_event_time when it is.

Ok I will make that change and send v3.

Re: [PATCH 3/3] iio: proximity: Add a ChromeOS EC MKBP proximity driver

[Stephen Boyd]

Quoting Jonathan Cameron (2021-01-24 09:38:20)
> On Fri, 22 Jan 2021 14:54:43 -0800
> Stephen Boyd <swboyd@chromium.org> wrote:
> 
> > Add support for a ChromeOS EC proximity driver that exposes a "front"
> > proximity sensor via the IIO subsystem. The EC decides when front
> > proximity is near and sets an MKBP switch 'EC_MKBP_FRONT_PROXIMITY' to
> > notify the kernel of proximity. Similarly, when proximity detects
> > something far away it sets the switch bit to 0. For now this driver
> > exposes a single sensor, but it could be expanded in the future via more
> > MKBP bits if desired.
> > 
> > Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > Cc: Benson Leung <bleung@chromium.org>
> > Cc: Guenter Roeck <groeck@chromium.org>
> > Cc: Douglas Anderson <dianders@chromium.org>
> > Cc: Gwendal Grignou <gwendal@chromium.org>
> > Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> 
> Hi Stephen,
> 
> Looks more or less fine to me.  My main concern is potential confusion
> in naming with the cros_ec_prox_light driver that we already have.
> 
> A few other minor bits and bobs inline.
> 

Cool thanks.

> > diff --git a/drivers/iio/proximity/Makefile b/drivers/iio/proximity/Makefile
> > index 9c1aca1a8b79..b1330dd8e212 100644
> > --- a/drivers/iio/proximity/Makefile
> > +++ b/drivers/iio/proximity/Makefile
> > @@ -5,6 +5,7 @@
> >  
> >  # When adding new entries keep the list in alphabetical order
> >  obj-$(CONFIG_AS3935)         += as3935.o
> > +obj-$(CONFIG_CROS_EC_PROXIMITY)      += cros_ec_proximity.o
> >  obj-$(CONFIG_ISL29501)               += isl29501.o
> >  obj-$(CONFIG_LIDAR_LITE_V2)  += pulsedlight-lidar-lite-v2.o
> >  obj-$(CONFIG_MB1232)         += mb1232.o
> > diff --git a/drivers/iio/proximity/cros_ec_proximity.c b/drivers/iio/proximity/cros_ec_proximity.c
> > new file mode 100644
> > index 000000000000..a3aef911e3cc
> > --- /dev/null
> > +++ b/drivers/iio/proximity/cros_ec_proximity.c
> > @@ -0,0 +1,252 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Driver for cros-ec proximity sensor exposed through MKBP switch
> > + *
> > + * Copyright 2021 Google LLC.
> > + */
> > +
> > +#include <linux/module.h>
> > +#include <linux/mutex.h>
> > +#include <linux/kernel.h>
> 
> Slight preference for alphabetical order though keeping specific
> sections separate as done here is fine.

Will fix.

> 
> > +#include <linux/notifier.h>
> > +#include <linux/of.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/slab.h>
> > +#include <linux/types.h>
> > +
> > +#include <linux/platform_data/cros_ec_commands.h>
> > +#include <linux/platform_data/cros_ec_proto.h>
> > +
> > +#include <linux/iio/events.h>
> > +#include <linux/iio/iio.h>
> > +#include <linux/iio/sysfs.h>
> > +
> > +#include <asm/unaligned.h>
> > +
> > +struct cros_ec_proximity_data {
> > +     struct cros_ec_device *ec;
> > +     struct iio_dev *indio_dev;
> > +     struct mutex lock;
> > +     struct notifier_block notifier;
> > +     bool enabled;
> > +};
> > +
> > +static const struct iio_event_spec cros_ec_prox_events[] = {
> > +     {
> > +             .type = IIO_EV_TYPE_THRESH,
> > +             .dir = IIO_EV_DIR_EITHER,
> > +             .mask_separate = BIT(IIO_EV_INFO_ENABLE),
> > +     },
> > +};
> > +
> > +static const struct iio_chan_spec cros_ec_prox_chan_spec[] = {
> > +     {
> > +             .type = IIO_PROXIMITY,
> > +             .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> > +             .event_spec = cros_ec_prox_events,
> > +             .num_event_specs = ARRAY_SIZE(cros_ec_prox_events),
> > +     },
> > +};
> > +
> > +static int cros_ec_proximity_parse_state(const void *data)
> > +{
> > +     u32 switches = get_unaligned_le32(data);
> > +
> > +     return !!(switches & BIT(EC_MKBP_FRONT_PROXIMITY));
> > +}
> > +
> > +static int cros_ec_proximity_query(struct cros_ec_device *ec_dev, int *state)
> > +{
> > +     struct ec_params_mkbp_info *params;
> > +     struct cros_ec_command *msg;
> > +     int ret;
> > +
> > +     msg = kzalloc(sizeof(*msg) + max(sizeof(u32), sizeof(*params)),
> > +                   GFP_KERNEL);
> 
> Given this is known at build time, perhaps better to add it to the 
> iio_priv() accessed structure and avoid having to handle allocations
> separately.

Ok.

> 
> > +     if (!msg)
> > +             return -ENOMEM;
[...]
> > +static int cros_ec_proximity_read_raw(struct iio_dev *indio_dev,
> > +                        const struct iio_chan_spec *chan, int *val,
> > +                        int *val2, long mask)
> > +{
> > +     struct cros_ec_proximity_data *data = iio_priv(indio_dev);
> > +     struct cros_ec_device *ec = data->ec;
> > +     int ret;
> > +
> > +     if (chan->type != IIO_PROXIMITY)
> > +             return -EINVAL;
> > +
> > +     switch (mask) {
> > +     case IIO_CHAN_INFO_RAW:
> > +             ret = iio_device_claim_direct_mode(indio_dev);
> 
> Normally we only introduce these protections when adding the ability
> to change the state from direct to buffered (which these prevent).
> This driver doesn't yet support any other modes so I don't think
> there is any benefit in having these.
> 
> If the aim is more local protection then should use a local lock
> as the semantics fo these functions might change in future.

Alright I'll drop it.

> 
> 
> > +             if (ret)
> > +                     return ret;
> > +
> > +             ret = cros_ec_proximity_query(ec, val);
> > +             iio_device_release_direct_mode(indio_dev);
> > +             if (ret)
> > +                     return ret;
> > +
> > +             return IIO_VAL_INT;
> > +     }
> > +
> > +     return -EINVAL;
> > +}
> > +
> > +static int cros_ec_proximity_read_event_config(struct iio_dev *indio_dev,
> > +                                 const struct iio_chan_spec *chan,
> > +                                 enum iio_event_type type,
> > +                                 enum iio_event_direction dir)
> > +{
> > +     struct cros_ec_proximity_data *data = iio_priv(indio_dev);
> > +
> > +     return data->enabled;
> > +}
> > +
> > +static int cros_ec_proximity_write_event_config(struct iio_dev *indio_dev,
> > +                                  const struct iio_chan_spec *chan,
> > +                                  enum iio_event_type type,
> > +                                  enum iio_event_direction dir, int state)
> > +{
> > +     struct cros_ec_proximity_data *data = iio_priv(indio_dev);
> > +
> > +     mutex_lock(&data->lock);
> > +     data->enabled = state;
> > +     mutex_unlock(&data->lock);
> > +
> > +     return 0;
> > +}
> > +
> > +static const struct iio_info cros_ec_proximity_info = {
> > +     .read_raw = cros_ec_proximity_read_raw,
> > +     .read_event_config = cros_ec_proximity_read_event_config,
> > +     .write_event_config = cros_ec_proximity_write_event_config,
> > +};
> > +
> > +static int cros_ec_proximity_probe(struct platform_device *pdev)
> > +{
> > +     struct device *dev = &pdev->dev;
> > +     struct cros_ec_device *ec = dev_get_drvdata(dev->parent);
> > +     struct iio_dev *indio_dev;
> > +     struct cros_ec_proximity_data *data;
> > +     int ret;
> > +
> > +     indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
> > +     if (!indio_dev)
> > +             return -ENOMEM;
> > +
> > +     data = iio_priv(indio_dev);
> > +     data->ec = ec;
> > +     data->indio_dev = indio_dev;
> > +     mutex_init(&data->lock);
> > +     platform_set_drvdata(pdev, data);
> > +
> > +     indio_dev->name = "cros_ec_proximity";
> > +     indio_dev->dev.parent = dev;
> > +     indio_dev->info = &cros_ec_proximity_info;
> > +     indio_dev->modes = INDIO_DIRECT_MODE;
> > +     indio_dev->channels = cros_ec_prox_chan_spec;
> > +     indio_dev->num_channels = ARRAY_SIZE(cros_ec_prox_chan_spec);
> > +
> > +     ret = devm_iio_device_register(dev, indio_dev);
> > +     if (ret)
> > +             return ret;
> > +
> > +     data->notifier.notifier_call = cros_ec_proximity_notify;
> > +     ret = blocking_notifier_chain_register(&ec->event_notifier,
> > +                                            &data->notifier);
> > +     if (ret)
> > +             dev_err(dev, "cannot register notifier: %d\n", ret);
> > +
> > +     return ret;
> > +}
> > +
> > +static int cros_ec_proximity_remove(struct platform_device *pdev)
> > +{
> > +     struct cros_ec_proximity_data *data = platform_get_drvdata(pdev);
> > +     struct cros_ec_device *ec = data->ec;
> > +
> > +     blocking_notifier_chain_unregister(&ec->event_notifier,
> > +                                        &data->notifier);
> > +
> > +     return 0;
> > +}
> > +
> > +#ifdef CONFIG_OF
> 
> As a general rule, we are trying to clear out protections on CONFIG_OF etc
> and use of of_match_ptr() on the basis they don't really gain us anything
> and prevent use of some other firmware types.  Here I guess you know what
> your firmware looks like, but I'm still keen to drop it in the interests
> of there being fewer places to copy it from.
> 
> It may be a good idea to give this a more specific name as well given
> we already have cros-ec-prox as a platform device id from
> the cros_ec_light_prox driver.

Alright. I renamed it to cros_ec_mkbp_proximity throughout this driver.
I'm concerned about dropping CONFIG_OF because of_match_ptr() and
CONFIG_OF=n makes it unused but I suppose that will be OK as long as
compilation passes.

> 
> 
> > +static const struct of_device_id cros_ec_proximity_of_match[] = {
> > +     { .compatible = "google,cros-ec-proximity" },
> > +     {}
> > +};
> > +MODULE_DEVICE_TABLE(of, cros_ec_proximity_of_match);
> > +#endif
> > +
> > +static struct platform_driver cros_ec_proximity_driver = {
> > +     .driver = {
> > +             .name = "cros-ec-proximity",
> > +             .of_match_table = of_match_ptr(cros_ec_proximity_of_match),
> > +     },
> > +     .probe = cros_ec_proximity_probe,
> > +     .remove = cros_ec_proximity_remove,
> > +};
> > +module_platform_driver(cros_ec_proximity_driver);
> > +