[PATCH v8 2/6] drivers/input/joystick: sensehat: Raspberry Pi Sense HAT joystick driver

[PATCH v8 2/6] drivers/input/joystick: sensehat: Raspberry Pi Sense HAT joystick driver

[Charles Mirabile]

This patch adds the driver for the Sense HAT joystick. It outputs BTN_DPAD
key events when moved in any of the four directions and the BTN_SELECT
event when depressed.

Co-developed-by: Daniel Bauman <dbauman@redhat.com>
Signed-off-by: Daniel Bauman <dbauman@redhat.com>
Co-developed-by: Mwesigwa Guma <mguma@redhat.com>
Signed-off-by: Mwesigwa Guma <mguma@redhat.com>
Co-developed-by: Joel Savitz <jsavitz@redhat.com>
Signed-off-by: Joel Savitz <jsavitz@redhat.com>
Signed-off-by: Charles Mirabile <cmirabil@redhat.com>
---
 drivers/input/joystick/Kconfig             |  11 ++
 drivers/input/joystick/Makefile            |   1 +
 drivers/input/joystick/sensehat-joystick.c | 137 +++++++++++++++++++++
 3 files changed, 149 insertions(+)
 create mode 100644 drivers/input/joystick/sensehat-joystick.c

diff --git a/drivers/input/joystick/Kconfig b/drivers/input/joystick/Kconfig
index 3b23078bc7b5..505a032e2786 100644
--- a/drivers/input/joystick/Kconfig
+++ b/drivers/input/joystick/Kconfig
@@ -399,4 +399,15 @@ config JOYSTICK_N64
 	  Say Y here if you want enable support for the four
 	  built-in controller ports on the Nintendo 64 console.
 
+config JOYSTICK_SENSEHAT
+	tristate "Raspberry Pi Sense HAT joystick"
+	depends on INPUT && I2C
+	select MFD_SIMPLE_MFD_I2C
+	help
+	  Say Y here if you want to enable the driver for the
+	  the Raspberry Pi Sense HAT.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called sensehat_joystick.
+
 endif
diff --git a/drivers/input/joystick/Makefile b/drivers/input/joystick/Makefile
index 5174b8aba2dd..39c8b5c6e5ae 100644
--- a/drivers/input/joystick/Makefile
+++ b/drivers/input/joystick/Makefile
@@ -28,6 +28,7 @@ obj-$(CONFIG_JOYSTICK_N64)		+= n64joy.o
 obj-$(CONFIG_JOYSTICK_PSXPAD_SPI)	+= psxpad-spi.o
 obj-$(CONFIG_JOYSTICK_PXRC)		+= pxrc.o
 obj-$(CONFIG_JOYSTICK_QWIIC)		+= qwiic-joystick.o
+obj-$(CONFIG_JOYSTICK_SENSEHAT)         += sensehat-joystick.o
 obj-$(CONFIG_JOYSTICK_SIDEWINDER)	+= sidewinder.o
 obj-$(CONFIG_JOYSTICK_SPACEBALL)	+= spaceball.o
 obj-$(CONFIG_JOYSTICK_SPACEORB)		+= spaceorb.o
diff --git a/drivers/input/joystick/sensehat-joystick.c b/drivers/input/joystick/sensehat-joystick.c
new file mode 100644
index 000000000000..6fed6004f464
--- /dev/null
+++ b/drivers/input/joystick/sensehat-joystick.c
@@ -0,0 +1,137 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Raspberry Pi Sense HAT joystick driver
+ * http://raspberrypi.org
+ *
+ * Copyright (C) 2015 Raspberry Pi
+ * Copyright (C) 2021 Charles Mirabile, Mwesigwa Guma, Joel Savitz
+ *
+ * Original Author: Serge Schneider
+ * Revised for upstream Linux by: Charles Mirabile, Mwesigwa Guma, Joel Savitz
+ */
+
+#include <linux/module.h>
+#include <linux/input.h>
+#include <linux/i2c.h>
+#include <linux/interrupt.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/property.h>
+
+#define JOYSTICK_SMB_REG 0xf2
+
+struct sensehat_joystick {
+	struct platform_device *pdev;
+	struct input_dev *keys_dev;
+	unsigned long prev_states;
+	struct regmap *regmap;
+};
+
+static const unsigned int keymap[] = {
+	BTN_DPAD_DOWN, BTN_DPAD_RIGHT, BTN_DPAD_UP, BTN_SELECT, BTN_DPAD_LEFT,
+};
+
+static irqreturn_t sensehat_joystick_report(int n, void *cookie)
+{
+	int i, error, keys;
+	struct sensehat_joystick *sensehat_joystick = cookie;
+	unsigned long curr_states, changes;
+
+	error = regmap_read(sensehat_joystick->regmap, JOYSTICK_SMB_REG, &keys);
+	if (error < 0) {
+		dev_err(&sensehat_joystick->pdev->dev,
+			"Failed to read joystick state: %d", error);
+		return IRQ_NONE;
+	}
+	curr_states = keys;
+	bitmap_xor(&changes, &curr_states, &sensehat_joystick->prev_states,
+		   ARRAY_SIZE(keymap));
+
+	for_each_set_bit(i, &changes, ARRAY_SIZE(keymap)) {
+		input_report_key(sensehat_joystick->keys_dev, keymap[i],
+				 curr_states & BIT(i));
+	}
+
+	input_sync(sensehat_joystick->keys_dev);
+	sensehat_joystick->prev_states = keys;
+	return IRQ_HANDLED;
+}
+
+static int sensehat_joystick_probe(struct platform_device *pdev)
+{
+	int error, i, irq;
+	struct sensehat_joystick *sensehat_joystick = devm_kzalloc(
+		&pdev->dev, sizeof(*sensehat_joystick), GFP_KERNEL);
+	if (!sensehat_joystick)
+		return -ENOMEM;
+
+	sensehat_joystick->pdev = pdev;
+
+	sensehat_joystick->regmap = dev_get_regmap(sensehat_joystick->pdev->dev.parent, NULL);
+	if (!sensehat_joystick->regmap) {
+		dev_err(&pdev->dev,
+			"unable to get sensehat regmap");
+		return -ENODEV;
+	}
+
+
+	sensehat_joystick->keys_dev = devm_input_allocate_device(&pdev->dev);
+	if (!sensehat_joystick->keys_dev) {
+		dev_err(&pdev->dev, "Could not allocate input device.");
+		return -ENOMEM;
+	}
+
+	for (i = 0; i < ARRAY_SIZE(keymap); i++)
+		set_bit(keymap[i], sensehat_joystick->keys_dev->keybit);
+
+	sensehat_joystick->keys_dev->name = "Raspberry Pi Sense HAT Joystick";
+	sensehat_joystick->keys_dev->phys = "sensehat-joystick/input0";
+	sensehat_joystick->keys_dev->id.bustype = BUS_I2C;
+	sensehat_joystick->keys_dev->evbit[0] =
+		BIT_MASK(EV_KEY) | BIT_MASK(EV_REP);
+
+	error = input_register_device(sensehat_joystick->keys_dev);
+	if (error) {
+		dev_err(&pdev->dev, "Could not register input device.");
+		return error;
+	}
+
+	irq = platform_get_irq(pdev, 0);
+	if (irq < 0) {
+		dev_err(&pdev->dev, "Could not retrieve interrupt request.");
+		return irq;
+	}
+
+	error = devm_request_threaded_irq(&pdev->dev, irq, NULL,
+					  sensehat_joystick_report,
+					  IRQF_ONESHOT, "keys",
+					  sensehat_joystick);
+
+	if (error) {
+		dev_err(&pdev->dev, "IRQ request failed.");
+		return error;
+	}
+
+	return 0;
+}
+
+static const struct of_device_id sensehat_joystick_device_id[] = {
+	{ .compatible = "raspberrypi,sensehat-joystick" },
+	{},
+};
+MODULE_DEVICE_TABLE(of, sensehat_joystick_device_id);
+
+static struct platform_driver sensehat_joystick_driver = {
+	.probe = sensehat_joystick_probe,
+	.driver = {
+		.name = "sensehat-joystick",
+		.of_match_table = sensehat_joystick_device_id,
+	},
+};
+
+module_platform_driver(sensehat_joystick_driver);
+
+MODULE_DESCRIPTION("Raspberry Pi Sense HAT joystick driver");
+MODULE_AUTHOR("Charles Mirabile <cmirabil@redhat.com>");
+MODULE_AUTHOR("Serge Schneider <serge@raspberrypi.org>");
+MODULE_LICENSE("GPL");
-- 
2.31.1

[PATCH v8 4/6] dt-bindings: mfd: sensehat: Add Raspberry Pi Sense HAT schema

[Charles Mirabile]

This patch adds the device tree bindings for the Sense HAT
and each of its children devices in yaml form.

Co-developed-by: Mwesigwa Guma <mguma@redhat.com>
Signed-off-by: Mwesigwa Guma <mguma@redhat.com>
Co-developed-by: Joel Savitz <jsavitz@redhat.com>
Signed-off-by: Joel Savitz <jsavitz@redhat.com>
Signed-off-by: Charles Mirabile <cmirabil@redhat.com>
---
 .../raspberrypi,sensehat-display.yaml         | 27 ++++++++
 .../input/raspberrypi,sensehat-joystick.yaml  | 33 +++++++++
 .../bindings/mfd/raspberrypi,sensehat.yaml    | 69 +++++++++++++++++++
 3 files changed, 129 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/auxdisplay/raspberrypi,sensehat-display.yaml
 create mode 100644 Documentation/devicetree/bindings/input/raspberrypi,sensehat-joystick.yaml
 create mode 100644 Documentation/devicetree/bindings/mfd/raspberrypi,sensehat.yaml

diff --git a/Documentation/devicetree/bindings/auxdisplay/raspberrypi,sensehat-display.yaml b/Documentation/devicetree/bindings/auxdisplay/raspberrypi,sensehat-display.yaml
new file mode 100644
index 000000000000..5e41d6b7817d
--- /dev/null
+++ b/Documentation/devicetree/bindings/auxdisplay/raspberrypi,sensehat-display.yaml
@@ -0,0 +1,27 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/auxdisplay/raspberrypi,sensehat-display.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Raspberry Pi Sensehat Display
+
+maintainers:
+  - Charles Mirabile <cmirabil@redhat.com>
+  - Mwesigwa Guma <mguma@redhat.com>
+  - Joel Savitz <jsavitz@redhat.com>
+
+description:
+  This device is part of the sensehat multi function device.
+  For more information see ../mfd/raspberrypi,sensehat.yaml.
+
+  This device features a programmable 8x8 RGB LED matrix.
+
+properties:
+  compatible:
+    const: raspberrypi,sensehat-display
+
+required:
+  - compatible
+
+additionalProperties: false
diff --git a/Documentation/devicetree/bindings/input/raspberrypi,sensehat-joystick.yaml b/Documentation/devicetree/bindings/input/raspberrypi,sensehat-joystick.yaml
new file mode 100644
index 000000000000..c97cd1d8eac6
--- /dev/null
+++ b/Documentation/devicetree/bindings/input/raspberrypi,sensehat-joystick.yaml
@@ -0,0 +1,33 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/input/raspberrypi,sensehat-joystick.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Raspberry Pi Sensehat Joystick
+
+maintainers:
+  - Charles Mirabile <cmirabil@redhat.com>
+  - Mwesigwa Guma <mguma@redhat.com>
+  - Joel Savitz <jsavitz@redhat.com>
+
+description:
+  This device is part of the sensehat multi function device.
+  For more information see ../mfd/raspberrypi,sensehat.yaml.
+
+  This device features a five button joystick (up, down,left,
+  right, click)
+
+properties:
+  compatible:
+    const: raspberrypi,sensehat-joystick
+
+  interrupts:
+    items:
+      - description: pin number for joystick interrupt
+
+required:
+  - compatible
+  - interrupts
+
+additionalProperties: false
diff --git a/Documentation/devicetree/bindings/mfd/raspberrypi,sensehat.yaml b/Documentation/devicetree/bindings/mfd/raspberrypi,sensehat.yaml
new file mode 100644
index 000000000000..2484ec91b430
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/raspberrypi,sensehat.yaml
@@ -0,0 +1,69 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/mfd/raspberrypi,sensehat.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Raspberry Pi Sensehat
+
+maintainers:
+  - Charles Mirabile <cmirabil@redhat.com>
+  - Mwesigwa Guma <mguma@redhat.com>
+  - Joel Savitz <jsavitz@redhat.com>
+
+description:
+  The Raspberry Pi Sensehat is an addon board originally developed
+  for the Raspberry Pi that has a joystick and an 8x8 RGB LED display
+  as well as several environmental sensors. It connects via i2c and
+  a gpio for irq.
+
+properties:
+  compatible:
+    const: raspberrypi,sensehat
+
+  reg:
+    items:
+      - description: i2c device address
+
+  "#address-cells":
+    const: 1
+
+  "#size-cells":
+    const: 0
+
+  "joystick":
+    $ref: ../input/raspberrypi,sensehat-joystick.yaml
+
+  "display":
+    $ref: ../auxdisplay/raspberrypi,sensehat-display.yaml
+
+required:
+  - compatible
+  - reg
+  - "#address-cells"
+  - "#size-cells"
+  - joystick
+  - display
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+    i2c {
+      #address-cells = <1>;
+      #size-cells = <0>;
+      sensehat@46 {
+        #address-cells = <1>;
+        #size-cells = <0>;
+        compatible = "raspberrypi,sensehat";
+        reg = <0x46>;
+        display {
+          compatible = "raspberrypi,sensehat-display";
+        };
+        joystick {
+          compatible = "raspberrypi,sensehat-joystick";
+          interrupts = <23 GPIO_ACTIVE_HIGH>;
+        };
+      };
+    };
-- 
2.31.1

Re: [PATCH v8 4/6] dt-bindings: mfd: sensehat: Add Raspberry Pi Sense HAT schema

[Krzysztof Kozlowski]

On 12/04/2022 22:13, Charles Mirabile wrote:
> This patch adds the device tree bindings for the Sense HAT
> and each of its children devices in yaml form.
>

Thank you for your patch. There is something to discuss/improve.

> +description:
> +  This device is part of the sensehat multi function device.
> +  For more information see ../mfd/raspberrypi,sensehat.yaml.
> +
> +  This device features a programmable 8x8 RGB LED matrix.
> +
> +properties:
> +  compatible:
> +    const: raspberrypi,sensehat-display

This binding is practically empty, so I wonder what's is purpose? Do you
plan to grow it? If this was already explained, sorry for bringing it up
again... :)

> +
> +required:
> +  - compatible
> +
> +additionalProperties: false
> diff --git a/Documentation/devicetree/bindings/input/raspberrypi,sensehat-joystick.yaml b/Documentation/devicetree/bindings/input/raspberrypi,sensehat-joystick.yaml
> new file mode 100644
> index 000000000000..c97cd1d8eac6
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/input/raspberrypi,sensehat-joystick.yaml
> @@ -0,0 +1,33 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/input/raspberrypi,sensehat-joystick.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Raspberry Pi Sensehat Joystick
> +
> +maintainers:
> +  - Charles Mirabile <cmirabil@redhat.com>
> +  - Mwesigwa Guma <mguma@redhat.com>
> +  - Joel Savitz <jsavitz@redhat.com>
> +
> +description:
> +  This device is part of the sensehat multi function device.
> +  For more information see ../mfd/raspberrypi,sensehat.yaml.
> +
> +  This device features a five button joystick (up, down,left,
> +  right, click)
> +
> +properties:
> +  compatible:
> +    const: raspberrypi,sensehat-joystick
> +
> +  interrupts:
> +    items:
> +      - description: pin number for joystick interrupt

Description is obvious, so just "maxItems: 1"

> +
> +required:
> +  - compatible
> +  - interrupts
> +
> +additionalProperties: false
> diff --git a/Documentation/devicetree/bindings/mfd/raspberrypi,sensehat.yaml b/Documentation/devicetree/bindings/mfd/raspberrypi,sensehat.yaml
> new file mode 100644
> index 000000000000..2484ec91b430
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mfd/raspberrypi,sensehat.yaml
> @@ -0,0 +1,69 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/mfd/raspberrypi,sensehat.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Raspberry Pi Sensehat
> +
> +maintainers:
> +  - Charles Mirabile <cmirabil@redhat.com>
> +  - Mwesigwa Guma <mguma@redhat.com>
> +  - Joel Savitz <jsavitz@redhat.com>
> +
> +description:
> +  The Raspberry Pi Sensehat is an addon board originally developed
> +  for the Raspberry Pi that has a joystick and an 8x8 RGB LED display
> +  as well as several environmental sensors. It connects via i2c and
> +  a gpio for irq.
> +
> +properties:
> +  compatible:
> +    const: raspberrypi,sensehat
> +
> +  reg:
> +    items:
> +      - description: i2c device address

Description is obvious, so just "maxItems: 1"

> +
> +  "#address-cells":
> +    const: 1
> +
> +  "#size-cells":
> +    const: 0

Why do you need these? You are not allowing any children with unit
addresses.

> +
> +  "joystick":
> +    $ref: ../input/raspberrypi,sensehat-joystick.yaml

Full path, so "/schemas/input/raspberrypi,sensehat-joystick.yaml#"

> +
> +  "display":
> +    $ref: ../auxdisplay/raspberrypi,sensehat-display.yaml

The same.

> +
> +required:
> +  - compatible
> +  - reg
> +  - "#address-cells"
> +  - "#size-cells"
> +  - joystick
> +  - display
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/gpio/gpio.h>
> +    i2c {
> +      #address-cells = <1>;
> +      #size-cells = <0>;
> +      sensehat@46 {

Generic node names please, so "hat".

> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +        compatible = "raspberrypi,sensehat";
> +        reg = <0x46>;

Could you put compatible and then reg at beginning of properties (before
address/size)? It is more common convention and these are more important
properties.

> +        display {
> +          compatible = "raspberrypi,sensehat-display";
> +        };
> +        joystick {
> +          compatible = "raspberrypi,sensehat-joystick";
> +          interrupts = <23 GPIO_ACTIVE_HIGH>;

This does not look like proper interrupt flag. Wrong constant.

> +        };
> +      };
> +    };


Best regards,
Krzysztof

Re: [PATCH v8 4/6] dt-bindings: mfd: sensehat: Add Raspberry Pi Sense HAT schema

[Krzysztof Kozlowski]

On 13/04/2022 15:38, Krzysztof Kozlowski wrote:
> On 12/04/2022 22:13, Charles Mirabile wrote:
>> This patch adds the device tree bindings for the Sense HAT
>> and each of its children devices in yaml form.
>>
> 
> Thank you for your patch. There is something to discuss/improve.
> 
>> +description:
>> +  This device is part of the sensehat multi function device.
>> +  For more information see ../mfd/raspberrypi,sensehat.yaml.
>> +
>> +  This device features a programmable 8x8 RGB LED matrix.
>> +
>> +properties:
>> +  compatible:
>> +    const: raspberrypi,sensehat-display
> 
> This binding is practically empty, so I wonder what's is purpose? Do you
> plan to grow it? If this was already explained, sorry for bringing it up
> again... :)
> 

BTW, I see now you got review from Rob. What changed here that you did
not include it? It is expected to document such case in patch changelog,
so the reviewer will know he wasn't ignored. :)

Best regards,
Krzysztof