[PATCH 0/6] Raspberry Pi Sense HAT driver

[PATCH 0/6] Raspberry Pi Sense HAT driver

[Charles Mirabile]

This patch series adds a set of drivers for operating the Sense HAT
peripheral device. This board is an add on for the Raspberry Pi that is
designed to connect using the GPIO connector and communicate via I2C.

It features:
	- a joystick
	- an 8x8 RGB LED matrix display
	- a whole bunch of environmental sensors with their own drivers
	  (those are already in upstream Linux)

This is a refactor of the work of Serge Schneider, the author of a
version of this driver that is currently in the Raspberry Pi downstream
kernel. We modified his code to make it suitable for upstream Linux.

A couple of tests are available for the driver in the test folder in
this repo: https://github.com/underground-software/sensehat.git
	- sensehat_joystick_test logs the input events from the
	  joystick to the console
	- sensehat_display_test displays various solid colors on
	  the LED panel.
	- full_sensehat_test displays a single lit cell that can be
	  moved with the joystick. Pressing the joystick ends the
	  program.

For more information about the Sense HAT, visit:
https://www.raspberrypi.org/products/sense-hat/

Improvements since v5:
	- The sensehat core driver has been removed and its functionality
	  is now performed by simply adding the sensehat compatible string
	  to the simple_mfd_i2c driver.
	- the subdrivers now read information about their SMB registers and
	  irqs from their own device tree meaning that these values are now
	  configurable and no longer hard coded into the driver.
	- As a result of removing the core driver and making each subdriver
	  handle its own internal data itself, the drivers have finally become
	  completely independent and so now it is finally truly possible to
	  use more than one sensehat or more than one of its sub components
	  in theory. (though in practice this would require modifying the
	  firmware on the board).

Fixes since v5:
	- the joystick has been changed to use BTN_DPAD and BTN_SELECT
	  instead of the arrow keys and enter, and lots of small issues
	  with the joystick code have been fixed (thanks to Dmitry
	  Torokhov for these suggestions)
	- all forward declarations have been removed from the driver
	  (thanks to Miguel Ojeda and Stefan Wahren for this suggestion)
	- the userspace interface of the display driver has been changed to
	  remove the need to process and copy the data as much before it can
	  be sent to the device. (thanks to Matthias Brugger for this
	  suggestion)
	- fixed issue with interrupt parent property in device tree binding
	  (thanks to Rob Herring for this suggestion)

charliemirabile (6):
  drivers/mfd: sensehat: Add Raspberry Pi Sense HAT to simple_mfd_i2c
  drivers/input/joystick: sensehat: Raspberry Pi Sense HAT joystick
    driver
  drivers/auxdisplay: sensehat: Raspberry Pi Sense HAT display driver
  dt-bindings: mfd: sensehat: Add Raspberry Pi Sense HAT schema
  MAINTAINERS: Add sensehat driver authors to MAINTAINERS
  DO NOT MERGE: full sensehat device tree overlay for raspberry pi 4

 .../raspberrypi,sensehat-display.yaml         |  32 +++
 .../input/raspberrypi,sensehat-joystick.yaml  |  37 +++
 .../bindings/mfd/raspberrypi,sensehat.yaml    |  68 +++++
 MAINTAINERS                                   |  12 +
 drivers/auxdisplay/Kconfig                    |   8 +
 drivers/auxdisplay/Makefile                   |   1 +
 drivers/auxdisplay/sensehat-display.c         | 264 ++++++++++++++++++
 drivers/input/joystick/Kconfig                |  11 +
 drivers/input/joystick/Makefile               |   1 +
 drivers/input/joystick/sensehat-joystick.c    | 128 +++++++++
 drivers/mfd/simple-mfd-i2c.c                  |   1 +
 include/uapi/linux/sensehat.h                 |  28 ++
 sensehat.dtbs                                 |  56 ++++
 13 files changed, 647 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
 create mode 100644 drivers/auxdisplay/sensehat-display.c
 create mode 100644 drivers/input/joystick/sensehat-joystick.c
 create mode 100644 include/uapi/linux/sensehat.h
 create mode 100644 sensehat.dtbs

-- 
2.31.1

[PATCH 1/6] drivers/mfd: sensehat: Add Raspberry Pi Sense HAT to simple_mfd_i2c

[Charles Mirabile]

This patch adds the compatible string for the Sense HAT device to
the list of compatible strings in the simple_mfd_i2c driver so that
it can match against the device and load its children and their drivers

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/mfd/simple-mfd-i2c.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/mfd/simple-mfd-i2c.c b/drivers/mfd/simple-mfd-i2c.c
index 51536691ad9d..f916a9c84563 100644
--- a/drivers/mfd/simple-mfd-i2c.c
+++ b/drivers/mfd/simple-mfd-i2c.c
@@ -64,6 +64,7 @@ static int simple_mfd_i2c_probe(struct i2c_client *i2c)
 
 static const struct of_device_id simple_mfd_i2c_of_match[] = {
 	{ .compatible = "kontron,sl28cpld" },
+	{ .compatible = "raspberrypi,sensehat" },
 	{}
 };
 MODULE_DEVICE_TABLE(of, simple_mfd_i2c_of_match);
-- 
2.31.1

[PATCH 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: 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 | 128 +++++++++++++++++++++
 3 files changed, 140 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..a688cc8fbde3
--- /dev/null
+++ b/drivers/input/joystick/sensehat-joystick.c
@@ -0,0 +1,128 @@
+// 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/of_irq.h>
+#include <linux/property.h>
+
+struct sensehat_joystick {
+	struct platform_device *pdev;
+	struct input_dev *keys_dev;
+	unsigned long prev_states;
+	u32 joystick_register;
+};
+
+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;
+	struct regmap *regmap =
+		dev_get_regmap(sensehat_joystick->pdev->dev.parent, NULL);
+	unsigned long curr_states, changes;
+
+	error = regmap_read(regmap, sensehat_joystick->joystick_register,
+			    &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;
+	struct sensehat_joystick *sensehat_joystick = devm_kzalloc(
+		&pdev->dev, sizeof(*sensehat_joystick), GFP_KERNEL);
+
+	sensehat_joystick->pdev = pdev;
+	sensehat_joystick->keys_dev = devm_input_allocate_device(&pdev->dev);
+	if (!sensehat_joystick->keys_dev) {
+		dev_err(&pdev->dev, "Could not allocate input device.\n");
+		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.\n");
+		return error;
+	}
+
+	error = device_property_read_u32(&pdev->dev, "reg",
+					 &sensehat_joystick->joystick_register);
+	if (error) {
+		dev_err(&pdev->dev, "Could not read register property.\n");
+		return error;
+	}
+
+	error = devm_request_threaded_irq(&pdev->dev,
+					  of_irq_get(pdev->dev.of_node, 0),
+					  NULL, sensehat_joystick_report,
+					  IRQF_ONESHOT, "keys",
+					  sensehat_joystick);
+
+	if (error) {
+		dev_err(&pdev->dev, "IRQ request failed.\n");
+		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("Serge Schneider <serge@raspberrypi.org>");
+MODULE_LICENSE("GPL");
-- 
2.31.1

[PATCH 3/6] drivers/auxdisplay: sensehat: Raspberry Pi Sense HAT display driver

[Charles Mirabile]

This patch adds the driver for the 8x8 RGB LED matrix display
on the Sense HAT. It appears as a character device named sense-hat
in /dev/. That special file is 192 bytes large and contains one byte
for each color channel of each pixel. The overall layout is:

    col 1       col 8
      |           |
      v           v
0x00: R   . . .   R \
0x08: G   . . .   G |> row 1
0x10: B   . . .   B /
      .   .       .
...   .     .     .
      .       .   .
0xA8: R   . . .   R \
0xB0: G   . . .   G |> row 8
0xB8: G   . . .   G /

Each channel may have any value between 0 and 31 (larger values are
wrapped) and are translated by a gamma table before being send to the
device to provide a linear experience of brightness (the gamma table
can be modified or reset using an ioctl call on the file). The constants
for the ioctl are in the sensehat.h header also added in this patch.

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>
Co-developed-by: Daniel Bauman <dbauman@redhat.com>
Signed-off-by: Daniel Bauman <dbauman@redhat.com>
Signed-off-by: Charles Mirabile <cmirabil@redhat.com>
---
 drivers/auxdisplay/Kconfig            |   8 +
 drivers/auxdisplay/Makefile           |   1 +
 drivers/auxdisplay/sensehat-display.c | 264 ++++++++++++++++++++++++++
 include/uapi/linux/sensehat.h         |  28 +++
 4 files changed, 301 insertions(+)
 create mode 100644 drivers/auxdisplay/sensehat-display.c
 create mode 100644 include/uapi/linux/sensehat.h

diff --git a/drivers/auxdisplay/Kconfig b/drivers/auxdisplay/Kconfig
index 64012cda4d12..9bad1aade7a0 100644
--- a/drivers/auxdisplay/Kconfig
+++ b/drivers/auxdisplay/Kconfig
@@ -203,6 +203,14 @@ config ARM_CHARLCD
 	  line and the Linux version on the second line, but that's
 	  still useful.
 
+config SENSEHAT_DISPLAY
+	tristate "Raspberry Pi Sense HAT display driver"
+	depends on I2C
+	select MFD_SIMPLE_MFD_I2C
+	help
+	 This is a driver for the Raspberry Pi Sensehat 8x8 RBG-LED matrix
+	 you can access it as a misc device at /dev/sense-hat
+
 menuconfig PARPORT_PANEL
 	tristate "Parallel port LCD/Keypad Panel support"
 	depends on PARPORT
diff --git a/drivers/auxdisplay/Makefile b/drivers/auxdisplay/Makefile
index 6968ed4d3f0a..30b2b7934046 100644
--- a/drivers/auxdisplay/Makefile
+++ b/drivers/auxdisplay/Makefile
@@ -14,3 +14,4 @@ obj-$(CONFIG_HT16K33)		+= ht16k33.o
 obj-$(CONFIG_PARPORT_PANEL)	+= panel.o
 obj-$(CONFIG_LCD2S)		+= lcd2s.o
 obj-$(CONFIG_LINEDISP)		+= line-display.o
+obj-$(CONFIG_SENSEHAT_DISPLAY)	+= sensehat-display.o
diff --git a/drivers/auxdisplay/sensehat-display.c b/drivers/auxdisplay/sensehat-display.c
new file mode 100644
index 000000000000..08b397a544f0
--- /dev/null
+++ b/drivers/auxdisplay/sensehat-display.c
@@ -0,0 +1,264 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Raspberry Pi Sense HAT 8x8 LED matrix display 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/kernel.h>
+#include <linux/errno.h>
+#include <linux/string.h>
+#include <linux/mm.h>
+#include <linux/slab.h>
+#include <linux/uaccess.h>
+#include <linux/delay.h>
+#include <linux/init.h>
+#include <linux/platform_device.h>
+#include <linux/mod_devicetable.h>
+#include <linux/miscdevice.h>
+#include <linux/regmap.h>
+#include <linux/property.h>
+#include <linux/sensehat.h>
+
+#define GAMMA_SIZE 32
+#define VMEM_SIZE 192
+
+struct sensehat_display {
+	struct platform_device *pdev;
+	struct miscdevice mdev;
+	struct mutex rw_mtx;
+	u8 gamma[GAMMA_SIZE];
+	u8 vmem[VMEM_SIZE];
+	u32 display_register;
+};
+
+static bool lowlight;
+module_param(lowlight, bool, 0);
+MODULE_PARM_DESC(lowlight, "Reduce LED matrix brightness to one third");
+
+static const u8 gamma_presets[][GAMMA_SIZE] = {
+	[SENSEHAT_GAMMA_DEFAULT] = {
+		0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x01, 0x01,
+		0x02, 0x02, 0x03, 0x03, 0x04, 0x05, 0x06, 0x07,
+		0x08, 0x09, 0x0A, 0x0B, 0x0C, 0x0E, 0x0F, 0x11,
+		0x12, 0x14, 0x15, 0x17, 0x19, 0x1B, 0x1D, 0x1F,
+	},
+	[SENSEHAT_GAMMA_LOWLIGHT] = {
+		0x00, 0x01, 0x01, 0x01, 0x01, 0x01, 0x01, 0x01,
+		0x01, 0x01, 0x01, 0x01, 0x01, 0x02, 0x02, 0x02,
+		0x03, 0x03, 0x03, 0x04, 0x04, 0x05, 0x05, 0x06,
+		0x06, 0x07, 0x07, 0x08, 0x08, 0x09, 0x0A, 0x0A,
+	},
+};
+
+static void sensehat_update_display(struct sensehat_display *display)
+{
+	int i, ret;
+	struct regmap *regmap = dev_get_regmap(display->pdev->dev.parent, NULL);
+	u8 temp[VMEM_SIZE];
+
+	for (i = 0; i < VMEM_SIZE; ++i)
+		temp[i] = display->gamma[display->vmem[i] & 0x1f];
+
+	ret = regmap_bulk_write(regmap, display->display_register, temp,
+				VMEM_SIZE);
+	if (ret < 0)
+		dev_err(&display->pdev->dev,
+			"Update to 8x8 LED matrix display failed");
+}
+
+static loff_t sensehat_display_llseek(struct file *filp, loff_t offset,
+				      int whence)
+{
+	loff_t base, pos;
+
+	switch (whence) {
+	case SEEK_SET:
+		base = 0;
+		break;
+	case SEEK_CUR:
+		base = filp->f_pos;
+		break;
+	case SEEK_END:
+		base = VMEM_SIZE;
+		break;
+	default:
+		return -EINVAL;
+	}
+	pos = base + offset;
+	if (pos < 0 || pos >= VMEM_SIZE)
+		return -EINVAL;
+	filp->f_pos = pos;
+	return base;
+}
+
+static ssize_t sensehat_display_read(struct file *filp, char __user *buf,
+				     size_t count, loff_t *f_pos)
+{
+	struct sensehat_display *sensehat_display =
+		container_of(filp->private_data, struct sensehat_display, mdev);
+	ssize_t retval = -EFAULT;
+
+	if (*f_pos >= VMEM_SIZE)
+		return 0;
+	if (*f_pos + count > VMEM_SIZE)
+		count = VMEM_SIZE - *f_pos;
+	if (mutex_lock_interruptible(&sensehat_display->rw_mtx))
+		return -ERESTARTSYS;
+	if (copy_to_user(buf, sensehat_display->vmem + *f_pos, count))
+		goto out;
+	*f_pos += count;
+	retval = count;
+out:
+	mutex_unlock(&sensehat_display->rw_mtx);
+	return retval;
+}
+
+static ssize_t sensehat_display_write(struct file *filp, const char __user *buf,
+				      size_t count, loff_t *f_pos)
+{
+	struct sensehat_display *sensehat_display =
+		container_of(filp->private_data, struct sensehat_display, mdev);
+	int ret = count;
+
+	if (*f_pos >= VMEM_SIZE)
+		return -EFBIG;
+	if (*f_pos + count > VMEM_SIZE)
+		count = VMEM_SIZE - *f_pos;
+	if (mutex_lock_interruptible(&sensehat_display->rw_mtx))
+		return -ERESTARTSYS;
+	if (copy_from_user(sensehat_display->vmem + *f_pos, buf, count)) {
+		ret = -EFAULT;
+		goto out;
+	}
+	sensehat_update_display(sensehat_display);
+	*f_pos += count;
+out:
+	mutex_unlock(&sensehat_display->rw_mtx);
+	return ret;
+}
+
+static long sensehat_display_ioctl(struct file *filp, unsigned int cmd,
+				   unsigned long arg)
+{
+	struct sensehat_display *sensehat_display =
+		container_of(filp->private_data, struct sensehat_display, mdev);
+	void __user *user_ptr = (void __user *)arg;
+	int i, ret = 0;
+
+	if (mutex_lock_interruptible(&sensehat_display->rw_mtx))
+		return -ERESTARTSYS;
+	switch (cmd) {
+	case SENSEDISP_IOGET_GAMMA:
+		if (copy_to_user(user_ptr, sensehat_display->gamma, GAMMA_SIZE))
+			ret = -EFAULT;
+		goto no_update;
+	case SENSEDISP_IOSET_GAMMA:
+		if (copy_from_user(sensehat_display->gamma, user_ptr,
+				   GAMMA_SIZE))
+			ret = -EFAULT;
+		break;
+	case SENSEDISP_IORESET_GAMMA:
+		if (arg >= SENSEHAT_GAMMA_PRESET_COUNT) {
+			ret = -EINVAL;
+			goto no_update;
+		}
+		memcpy(sensehat_display->gamma, gamma_presets[arg], GAMMA_SIZE);
+		goto no_check;
+	default:
+		ret = -EINVAL;
+		break;
+	}
+	for (i = 0; i < GAMMA_SIZE; ++i)
+		sensehat_display->gamma[i] &= 0x1f;
+no_check:
+	sensehat_update_display(sensehat_display);
+no_update:
+	mutex_unlock(&sensehat_display->rw_mtx);
+	return ret;
+}
+
+static const struct file_operations sensehat_display_fops = {
+	.owner = THIS_MODULE,
+	.llseek = sensehat_display_llseek,
+	.read = sensehat_display_read,
+	.write = sensehat_display_write,
+	.unlocked_ioctl = sensehat_display_ioctl,
+};
+
+static int sensehat_display_probe(struct platform_device *pdev)
+{
+	int ret;
+
+	struct sensehat_display *sensehat_display =
+		devm_kzalloc(&pdev->dev, sizeof(*sensehat_display), GFP_KERNEL);
+
+	sensehat_display->pdev = pdev;
+
+	memcpy(sensehat_display->gamma, gamma_presets[lowlight], GAMMA_SIZE);
+
+	memset(sensehat_display->vmem, 0, VMEM_SIZE);
+
+	mutex_init(&sensehat_display->rw_mtx);
+
+	ret = device_property_read_u32(&pdev->dev, "reg",
+				       &sensehat_display->display_register);
+	if (ret) {
+		dev_err(&pdev->dev, "Could not read register property.\n");
+		return ret;
+	}
+
+	sensehat_display->mdev = (struct miscdevice){
+		.minor = MISC_DYNAMIC_MINOR,
+		.name = "sense-hat",
+		.mode = 0666,
+		.fops = &sensehat_display_fops,
+	};
+
+	ret = misc_register(&sensehat_display->mdev);
+	if (ret < 0) {
+		dev_err(&pdev->dev,
+			"Could not register 8x8 LED matrix display.\n");
+		return ret;
+	}
+
+	ret = devm_add_action(&pdev->dev, (void *)misc_deregister,
+			      &sensehat_display->mdev);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "Could not add misc device to devm\n");
+		return ret;
+	}
+
+	dev_info(&pdev->dev,
+		 "8x8 LED matrix display registered with minor number %i",
+		 sensehat_display->mdev.minor);
+
+	sensehat_update_display(sensehat_display);
+	return 0;
+}
+
+static const struct of_device_id sensehat_display_device_id[] = {
+	{ .compatible = "raspberrypi,sensehat-display" },
+	{},
+};
+MODULE_DEVICE_TABLE(of, sensehat_display_device_id);
+
+static struct platform_driver sensehat_display_driver = {
+	.probe = sensehat_display_probe,
+	.driver = {
+		.name = "sensehat-display",
+		.of_match_table = sensehat_display_device_id,
+	},
+};
+
+module_platform_driver(sensehat_display_driver);
+
+MODULE_DESCRIPTION("Raspberry Pi Sense HAT 8x8 LED matrix display driver");
+MODULE_AUTHOR("Serge Schneider <serge@raspberrypi.org>");
+MODULE_LICENSE("GPL");
diff --git a/include/uapi/linux/sensehat.h b/include/uapi/linux/sensehat.h
new file mode 100644
index 000000000000..cae07568a5eb
--- /dev/null
+++ b/include/uapi/linux/sensehat.h
@@ -0,0 +1,28 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later WITH Linux-syscall-note */
+/*
+ * Raspberry Pi Sense HAT core 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
+ */
+
+#ifndef _UAPILINUX_SENSEHAT_H_
+#define _UAPILINUX_SENSEHAT_H_
+
+#define SENSEDISP_IOC_MAGIC 0xF1
+
+#define SENSEDISP_IOGET_GAMMA _IO(SENSEDISP_IOC_MAGIC, 0)
+#define SENSEDISP_IOSET_GAMMA _IO(SENSEDISP_IOC_MAGIC, 1)
+#define SENSEDISP_IORESET_GAMMA _IO(SENSEDISP_IOC_MAGIC, 2)
+
+enum gamma_preset {
+	SENSEHAT_GAMMA_DEFAULT = 0,
+	SENSEHAT_GAMMA_LOWLIGHT,
+	SENSEHAT_GAMMA_PRESET_COUNT,
+};
+
+#endif
-- 
2.31.1

[PATCH 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         | 32 +++++++++
 .../input/raspberrypi,sensehat-joystick.yaml  | 37 ++++++++++
 .../bindings/mfd/raspberrypi,sensehat.yaml    | 68 +++++++++++++++++++
 3 files changed, 137 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..8937adba1c4b
--- /dev/null
+++ b/Documentation/devicetree/bindings/auxdisplay/raspberrypi,sensehat-display.yaml
@@ -0,0 +1,32 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+$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
+
+  reg:
+    items:
+      - description: |
+          smb register number for the first component of the fist
+          pixel in the range.
+
+required:
+  - compatible
+  - reg
+
+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..a6d95c903b04
--- /dev/null
+++ b/Documentation/devicetree/bindings/input/raspberrypi,sensehat-joystick.yaml
@@ -0,0 +1,37 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+$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
+
+  reg:
+    items:
+      - description: |
+          smb register number for accessing the state of the buttons
+
+  interrupts:
+    items:
+      - description: pin number for joystick interrupt
+
+required:
+  - compatible
+  - reg
+  - 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..89037be87c10
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/raspberrypi,sensehat.yaml
@@ -0,0 +1,68 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+$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
+
+patternProperties:
+  "^joystick(@[0-9a-f]+)?$":
+    $ref: ../input/raspberrypi,sensehat-joystick.yaml
+
+  "^display(@[0-9a-f]+)?$":
+    $ref: ../auxdisplay/raspberrypi,sensehat-display.yaml
+
+required:
+  - "#address-cells"
+  - "#size-cells"
+  - compatible
+  - reg
+
+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@0 {
+          compatible = "raspberrypi,sensehat-display";
+          reg = <0x0>;
+        };
+        joystick@f2 {
+          compatible = "raspberrypi,sensehat-joystick";
+          reg = <0x0>;
+          interrupts = <23 GPIO_ACTIVE_HIGH>;
+        };
+      };
+    };
-- 
2.31.1

[PATCH 5/6] MAINTAINERS: Add sensehat driver authors to MAINTAINERS

[Charles Mirabile]

This patch adds the driver authors to MAINAINERS.

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>
---
 MAINTAINERS | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index f41088418aae..338df96ca782 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -17389,6 +17389,18 @@ F:	Documentation/ABI/testing/sysfs-bus-iio-chemical-sunrise-co2
 F:	Documentation/devicetree/bindings/iio/chemical/senseair,sunrise.yaml
 F:	drivers/iio/chemical/sunrise_co2.c
 
+SENSEHAT DRIVER
+M:	Charles Mirabile <cmirabil@redhat.com>
+M:	Mwesigwa Guma <mguma@redhat.com>
+M:	Joel Savitz <jsavitz@redhat.com>
+S:	Maintained
+F:	Documentation/devicetree/bindings/auxdisplay/raspberrypi,sensehat-display.yaml
+F:	Documentation/devicetree/bindings/input/raspberrypi,sensehat-joystick.yaml
+F:	Documentation/devicetree/bindings/mfd/raspberrypi,sensehat.yaml
+F:	drivers/auxdisplay/sensehat-display.c
+F:	drivers/input/joystick/sensehat-joystick.c
+F:	include/linux/mfd/sensehat.h
+
 SENSIRION SCD30 CARBON DIOXIDE SENSOR DRIVER
 M:	Tomasz Duszynski <tomasz.duszynski@octakon.com>
 S:	Maintained
-- 
2.31.1

[PATCH 6/6] DO NOT MERGE: full sensehat device tree overlay for raspberry pi 4

[Charles Mirabile]

This patch shold not be merged - dtbs files are not stored in the
kernel tree. We just provide this file so the code can be tested.

This overlay is suitable for testing the driver, it can be compiled with
dtc and put in the /boot/overlays/ folder then specified in config.txt
by putting the lines:

dtoverlay=		#suppress loading of default overlay for HAT
dtoverlay=sensehat	#load custom overlay

at the beginning before any other lines in config.txt

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>
---
 sensehat.dtbs | 56 +++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 56 insertions(+)
 create mode 100644 sensehat.dtbs

diff --git a/sensehat.dtbs b/sensehat.dtbs
new file mode 100644
index 000000000000..5f1a9d62c9f4
--- /dev/null
+++ b/sensehat.dtbs
@@ -0,0 +1,56 @@
+/dts-v1/;
+/plugin/;
+
+/ {
+	compatible = "brcm,bcm2835";
+};
+
+&i2c1 {
+	#address-cells = <0x01>;
+	#size-cells = <0x00>;
+	status = "okay";
+
+	sensehat@46 {
+		#address-cells = <0x01>;
+		#size-cells = <0x00>;
+		compatible = "raspberrypi,sensehat";
+		reg = <0x46>;
+		interrupt-parent = <&gpio>;
+		status = "okay";
+		display@0 {
+			compatible = "raspberrypi,sensehat-display";
+			reg = <0x0>;
+			status = "okay";
+		};
+		joystick@f2 {
+			compatible = "raspberrypi,sensehat-joystick";
+			reg = <0xf2>;
+			interrupts = <23 1>;
+			status = "okay";
+		};
+	};
+
+	lsm9ds1-magn@1c {
+		compatible = "st,lsm9ds1-magn";
+		reg = <0x1c>;
+		status = "okay";
+	};
+
+	lsm9ds1-accel@6a {
+		compatible = "st,lsm9ds1-accel";
+		reg = <0x6a>;
+		status = "okay";
+	};
+
+	lps25h-press@5c {
+		compatible = "st,lps25h-press";
+		reg = <0x5c>;
+		status = "okay";
+	};
+
+	hts221-humid@5f {
+		compatible = "st,hts221-humid\0st,hts221";
+		reg = <0x5f>;
+		status = "okay";
+	};
+};
-- 
2.31.1

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

[Rob Herring]

On Wed, 02 Feb 2022 19:25:19 -0500, Charles Mirabile wrote:
> 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         | 32 +++++++++
>  .../input/raspberrypi,sensehat-joystick.yaml  | 37 ++++++++++
>  .../bindings/mfd/raspberrypi,sensehat.yaml    | 68 +++++++++++++++++++
>  3 files changed, 137 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
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:
./Documentation/devicetree/bindings/auxdisplay/raspberrypi,sensehat-display.yaml:2:1: [error] missing document start "---" (document-start)
./Documentation/devicetree/bindings/input/raspberrypi,sensehat-joystick.yaml:2:1: [error] missing document start "---" (document-start)
./Documentation/devicetree/bindings/mfd/raspberrypi,sensehat.yaml:2:1: [error] missing document start "---" (document-start)

dtschema/dtc warnings/errors:

doc reference errors (make refcheckdocs):

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

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.

Re: [PATCH 1/6] drivers/mfd: sensehat: Add Raspberry Pi Sense HAT to simple_mfd_i2c

[Nicolas Saenz Julienne]

On Wed, 2022-02-02 at 19:25 -0500, Charles Mirabile wrote:
> This patch adds the compatible string for the Sense HAT device to
> the list of compatible strings in the simple_mfd_i2c driver so that
> it can match against the device and load its children and their drivers
> 
> 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/mfd/simple-mfd-i2c.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/mfd/simple-mfd-i2c.c b/drivers/mfd/simple-mfd-i2c.c
> index 51536691ad9d..f916a9c84563 100644
> --- a/drivers/mfd/simple-mfd-i2c.c
> +++ b/drivers/mfd/simple-mfd-i2c.c
> @@ -64,6 +64,7 @@ static int simple_mfd_i2c_probe(struct i2c_client *i2c)
>  
>  static const struct of_device_id simple_mfd_i2c_of_match[] = {
>  	{ .compatible = "kontron,sl28cpld" },
> +	{ .compatible = "raspberrypi,sensehat" },

Nice!

>  	{}
>  };
>  MODULE_DEVICE_TABLE(of, simple_mfd_i2c_of_match);

-- 
Nicolás Sáenz

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

[Nicolas Saenz Julienne]

On Wed, 2022-02-02 at 19:25 -0500, Charles Mirabile wrote:
> 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: 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 | 128 +++++++++++++++++++++
>  3 files changed, 140 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..a688cc8fbde3
> --- /dev/null
> +++ b/drivers/input/joystick/sensehat-joystick.c
> @@ -0,0 +1,128 @@
> +// 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/of_irq.h>
> +#include <linux/property.h>
> +
> +struct sensehat_joystick {
> +	struct platform_device *pdev;
> +	struct input_dev *keys_dev;
> +	unsigned long prev_states;
> +	u32 joystick_register;
> +};
> +
> +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;
> +	struct regmap *regmap =
> +		dev_get_regmap(sensehat_joystick->pdev->dev.parent, NULL);

I think it's better to get this during probe and save it in 'struct
sensehat_joystick'. You'll avoid having to search for it everytime.

> +	unsigned long curr_states, changes;
> +
> +	error = regmap_read(regmap, sensehat_joystick->joystick_register,
> +			    &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;
> +	struct sensehat_joystick *sensehat_joystick = devm_kzalloc(
> +		&pdev->dev, sizeof(*sensehat_joystick), GFP_KERNEL);
> +
> +	sensehat_joystick->pdev = pdev;
> +	sensehat_joystick->keys_dev = devm_input_allocate_device(&pdev->dev);
> +	if (!sensehat_joystick->keys_dev) {
> +		dev_err(&pdev->dev, "Could not allocate input device.\n");
> +		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.\n");
> +		return error;
> +	}
> +
> +	error = device_property_read_u32(&pdev->dev, "reg",
> +					 &sensehat_joystick->joystick_register);
> +	if (error) {
> +		dev_err(&pdev->dev, "Could not read register property.\n");
> +		return error;
> +	}

I don't think you really need to pass this information through the devicetree.
It's just the address to a register, not a HW design detail (i.e. how the board
is layout). Using a define is good enough.

> +
> +	error = devm_request_threaded_irq(&pdev->dev,
> +					  of_irq_get(pdev->dev.of_node, 0),

Use platform_get_irq() and remember the function may fail, both unrecoverable
errors and EPROBE_DEFER when the irqchip isn't ready yet.

> +					  NULL, sensehat_joystick_report,
> +					  IRQF_ONESHOT, "keys",
> +					  sensehat_joystick);
> +
> +	if (error) {
> +		dev_err(&pdev->dev, "IRQ request failed.\n");
> +		return error;
> +	}

nit: space here

> +	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("Serge Schneider <serge@raspberrypi.org>");
> +MODULE_LICENSE("GPL");

-- 
Nicolás Sáenz

Re: [PATCH 3/6] drivers/auxdisplay: sensehat: Raspberry Pi Sense HAT display driver

[Nicolas Saenz Julienne]

On Wed, 2022-02-02 at 19:25 -0500, Charles Mirabile wrote:
> This patch adds the driver for the 8x8 RGB LED matrix display
> on the Sense HAT. It appears as a character device named sense-hat
> in /dev/. That special file is 192 bytes large and contains one byte
> for each color channel of each pixel. The overall layout is:
> 
>     col 1       col 8
>       |           |
>       v           v
> 0x00: R   . . .   R \
> 0x08: G   . . .   G |> row 1
> 0x10: B   . . .   B /
>       .   .       .
> ...   .     .     .
>       .       .   .
> 0xA8: R   . . .   R \
> 0xB0: G   . . .   G |> row 8
> 0xB8: G   . . .   G /
> 
> Each channel may have any value between 0 and 31 (larger values are
> wrapped) and are translated by a gamma table before being send to the
> device to provide a linear experience of brightness (the gamma table
> can be modified or reset using an ioctl call on the file). The constants
> for the ioctl are in the sensehat.h header also added in this patch.
> 
> 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>
> Co-developed-by: Daniel Bauman <dbauman@redhat.com>
> Signed-off-by: Daniel Bauman <dbauman@redhat.com>
> Signed-off-by: Charles Mirabile <cmirabil@redhat.com>
> ---
>  drivers/auxdisplay/Kconfig            |   8 +
>  drivers/auxdisplay/Makefile           |   1 +
>  drivers/auxdisplay/sensehat-display.c | 264 ++++++++++++++++++++++++++
>  include/uapi/linux/sensehat.h         |  28 +++
>  4 files changed, 301 insertions(+)
>  create mode 100644 drivers/auxdisplay/sensehat-display.c
>  create mode 100644 include/uapi/linux/sensehat.h
> 
> diff --git a/drivers/auxdisplay/Kconfig b/drivers/auxdisplay/Kconfig
> index 64012cda4d12..9bad1aade7a0 100644
> --- a/drivers/auxdisplay/Kconfig
> +++ b/drivers/auxdisplay/Kconfig
> @@ -203,6 +203,14 @@ config ARM_CHARLCD
>  	  line and the Linux version on the second line, but that's
>  	  still useful.
>  
> +config SENSEHAT_DISPLAY
> +	tristate "Raspberry Pi Sense HAT display driver"
> +	depends on I2C
> +	select MFD_SIMPLE_MFD_I2C
> +	help
> +	 This is a driver for the Raspberry Pi Sensehat 8x8 RBG-LED matrix
> +	 you can access it as a misc device at /dev/sense-hat
> +
>  menuconfig PARPORT_PANEL
>  	tristate "Parallel port LCD/Keypad Panel support"
>  	depends on PARPORT
> diff --git a/drivers/auxdisplay/Makefile b/drivers/auxdisplay/Makefile
> index 6968ed4d3f0a..30b2b7934046 100644
> --- a/drivers/auxdisplay/Makefile
> +++ b/drivers/auxdisplay/Makefile
> @@ -14,3 +14,4 @@ obj-$(CONFIG_HT16K33)		+= ht16k33.o
>  obj-$(CONFIG_PARPORT_PANEL)	+= panel.o
>  obj-$(CONFIG_LCD2S)		+= lcd2s.o
>  obj-$(CONFIG_LINEDISP)		+= line-display.o
> +obj-$(CONFIG_SENSEHAT_DISPLAY)	+= sensehat-display.o
> diff --git a/drivers/auxdisplay/sensehat-display.c b/drivers/auxdisplay/sensehat-display.c
> new file mode 100644
> index 000000000000..08b397a544f0
> --- /dev/null
> +++ b/drivers/auxdisplay/sensehat-display.c
> @@ -0,0 +1,264 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Raspberry Pi Sense HAT 8x8 LED matrix display 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/kernel.h>
> +#include <linux/errno.h>
> +#include <linux/string.h>
> +#include <linux/mm.h>
> +#include <linux/slab.h>
> +#include <linux/uaccess.h>
> +#include <linux/delay.h>
> +#include <linux/init.h>
> +#include <linux/platform_device.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/miscdevice.h>
> +#include <linux/regmap.h>
> +#include <linux/property.h>
> +#include <linux/sensehat.h>
> +
> +#define GAMMA_SIZE 32
> +#define VMEM_SIZE 192
> +
> +struct sensehat_display {
> +	struct platform_device *pdev;
> +	struct miscdevice mdev;
> +	struct mutex rw_mtx;
> +	u8 gamma[GAMMA_SIZE];
> +	u8 vmem[VMEM_SIZE];
> +	u32 display_register;
> +};
> +
> +static bool lowlight;
> +module_param(lowlight, bool, 0);
> +MODULE_PARM_DESC(lowlight, "Reduce LED matrix brightness to one third");
> +
> +static const u8 gamma_presets[][GAMMA_SIZE] = {
> +	[SENSEHAT_GAMMA_DEFAULT] = {
> +		0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x01, 0x01,
> +		0x02, 0x02, 0x03, 0x03, 0x04, 0x05, 0x06, 0x07,
> +		0x08, 0x09, 0x0A, 0x0B, 0x0C, 0x0E, 0x0F, 0x11,
> +		0x12, 0x14, 0x15, 0x17, 0x19, 0x1B, 0x1D, 0x1F,
> +	},
> +	[SENSEHAT_GAMMA_LOWLIGHT] = {
> +		0x00, 0x01, 0x01, 0x01, 0x01, 0x01, 0x01, 0x01,
> +		0x01, 0x01, 0x01, 0x01, 0x01, 0x02, 0x02, 0x02,
> +		0x03, 0x03, 0x03, 0x04, 0x04, 0x05, 0x05, 0x06,
> +		0x06, 0x07, 0x07, 0x08, 0x08, 0x09, 0x0A, 0x0A,
> +	},
> +};
> +
> +static void sensehat_update_display(struct sensehat_display *display)
> +{
> +	int i, ret;
> +	struct regmap *regmap = dev_get_regmap(display->pdev->dev.parent, NULL);

Same comment as with joystick.

> +	u8 temp[VMEM_SIZE];
> +
> +	for (i = 0; i < VMEM_SIZE; ++i)
> +		temp[i] = display->gamma[display->vmem[i] & 0x1f];
> +
> +	ret = regmap_bulk_write(regmap, display->display_register, temp,
> +				VMEM_SIZE);
> +	if (ret < 0)
> +		dev_err(&display->pdev->dev,
> +			"Update to 8x8 LED matrix display failed");
> +}
> +
> +static loff_t sensehat_display_llseek(struct file *filp, loff_t offset,
> +				      int whence)
> +{
> +	loff_t base, pos;
> +
> +	switch (whence) {
> +	case SEEK_SET:
> +		base = 0;
> +		break;
> +	case SEEK_CUR:
> +		base = filp->f_pos;
> +		break;
> +	case SEEK_END:
> +		base = VMEM_SIZE;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +	pos = base + offset;
> +	if (pos < 0 || pos >= VMEM_SIZE)
> +		return -EINVAL;
> +	filp->f_pos = pos;
> +	return base;
> +}
> +
> +static ssize_t sensehat_display_read(struct file *filp, char __user *buf,
> +				     size_t count, loff_t *f_pos)
> +{
> +	struct sensehat_display *sensehat_display =
> +		container_of(filp->private_data, struct sensehat_display, mdev);
> +	ssize_t retval = -EFAULT;
> +
> +	if (*f_pos >= VMEM_SIZE)
> +		return 0;
> +	if (*f_pos + count > VMEM_SIZE)
> +		count = VMEM_SIZE - *f_pos;
> +	if (mutex_lock_interruptible(&sensehat_display->rw_mtx))
> +		return -ERESTARTSYS;
> +	if (copy_to_user(buf, sensehat_display->vmem + *f_pos, count))
> +		goto out;
> +	*f_pos += count;
> +	retval = count;
> +out:
> +	mutex_unlock(&sensehat_display->rw_mtx);
> +	return retval;
> +}
> +
> +static ssize_t sensehat_display_write(struct file *filp, const char __user *buf,
> +				      size_t count, loff_t *f_pos)
> +{
> +	struct sensehat_display *sensehat_display =
> +		container_of(filp->private_data, struct sensehat_display, mdev);
> +	int ret = count;
> +
> +	if (*f_pos >= VMEM_SIZE)
> +		return -EFBIG;
> +	if (*f_pos + count > VMEM_SIZE)
> +		count = VMEM_SIZE - *f_pos;
> +	if (mutex_lock_interruptible(&sensehat_display->rw_mtx))
> +		return -ERESTARTSYS;
> +	if (copy_from_user(sensehat_display->vmem + *f_pos, buf, count)) {
> +		ret = -EFAULT;
> +		goto out;
> +	}
> +	sensehat_update_display(sensehat_display);
> +	*f_pos += count;
> +out:
> +	mutex_unlock(&sensehat_display->rw_mtx);
> +	return ret;
> +}
> +
> +static long sensehat_display_ioctl(struct file *filp, unsigned int cmd,
> +				   unsigned long arg)
> +{
> +	struct sensehat_display *sensehat_display =
> +		container_of(filp->private_data, struct sensehat_display, mdev);
> +	void __user *user_ptr = (void __user *)arg;
> +	int i, ret = 0;
> +
> +	if (mutex_lock_interruptible(&sensehat_display->rw_mtx))
> +		return -ERESTARTSYS;

nit: add space, here and after the switch statement.

> +	switch (cmd) {
> +	case SENSEDISP_IOGET_GAMMA:
> +		if (copy_to_user(user_ptr, sensehat_display->gamma, GAMMA_SIZE))
> +			ret = -EFAULT;
> +		goto no_update;
> +	case SENSEDISP_IOSET_GAMMA:
> +		if (copy_from_user(sensehat_display->gamma, user_ptr,
> +				   GAMMA_SIZE))
> +			ret = -EFAULT;
> +		break;
> +	case SENSEDISP_IORESET_GAMMA:
> +		if (arg >= SENSEHAT_GAMMA_PRESET_COUNT) {
> +			ret = -EINVAL;
> +			goto no_update;
> +		}
> +		memcpy(sensehat_display->gamma, gamma_presets[arg], GAMMA_SIZE);
> +		goto no_check;
> +	default:
> +		ret = -EINVAL;
> +		break;
> +	}
> +	for (i = 0; i < GAMMA_SIZE; ++i)
> +		sensehat_display->gamma[i] &= 0x1f;
> +no_check:
> +	sensehat_update_display(sensehat_display);
> +no_update:
> +	mutex_unlock(&sensehat_display->rw_mtx);
> +	return ret;
> +}
> +
> +static const struct file_operations sensehat_display_fops = {
> +	.owner = THIS_MODULE,
> +	.llseek = sensehat_display_llseek,
> +	.read = sensehat_display_read,
> +	.write = sensehat_display_write,
> +	.unlocked_ioctl = sensehat_display_ioctl,
> +};
> +
> +static int sensehat_display_probe(struct platform_device *pdev)
> +{
> +	int ret;
> +
> +	struct sensehat_display *sensehat_display =
> +		devm_kzalloc(&pdev->dev, sizeof(*sensehat_display), GFP_KERNEL);
> +
> +	sensehat_display->pdev = pdev;
> +
> +	memcpy(sensehat_display->gamma, gamma_presets[lowlight], GAMMA_SIZE);
> +
> +	memset(sensehat_display->vmem, 0, VMEM_SIZE);

You don't nee this, kzalloc already zeroes the memory.

> +
> +	mutex_init(&sensehat_display->rw_mtx);
> +
> +	ret = device_property_read_u32(&pdev->dev, "reg",
> +				       &sensehat_display->display_register);

Same comment as with joystick.

> +	if (ret) {
> +		dev_err(&pdev->dev, "Could not read register property.\n");
> +		return ret;
> +	}
> +
> +	sensehat_display->mdev = (struct miscdevice){
> +		.minor = MISC_DYNAMIC_MINOR,
> +		.name = "sense-hat",
> +		.mode = 0666,
> +		.fops = &sensehat_display_fops,
> +	};
> +
> +	ret = misc_register(&sensehat_display->mdev);
> +	if (ret < 0) {
> +		dev_err(&pdev->dev,
> +			"Could not register 8x8 LED matrix display.\n");
> +		return ret;
> +	}
> +
> +	ret = devm_add_action(&pdev->dev, (void *)misc_deregister,
> +			      &sensehat_display->mdev);
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "Could not add misc device to devm\n");
> +		return ret;
> +	}

Instead of doing this I'd add a .remove callback to the platform driver.

> +
> +	dev_info(&pdev->dev,
> +		 "8x8 LED matrix display registered with minor number %i",
> +		 sensehat_display->mdev.minor);
> +
> +	sensehat_update_display(sensehat_display);

This could potentially race with an app accessing the misc device?

> +	return 0;
> +}
> +
> +static const struct of_device_id sensehat_display_device_id[] = {
> +	{ .compatible = "raspberrypi,sensehat-display" },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, sensehat_display_device_id);
> +
> +static struct platform_driver sensehat_display_driver = {
> +	.probe = sensehat_display_probe,
> +	.driver = {
> +		.name = "sensehat-display",
> +		.of_match_table = sensehat_display_device_id,
> +	},
> +};
> +
> +module_platform_driver(sensehat_display_driver);
> +
> +MODULE_DESCRIPTION("Raspberry Pi Sense HAT 8x8 LED matrix display driver");
> +MODULE_AUTHOR("Serge Schneider <serge@raspberrypi.org>");
> +MODULE_LICENSE("GPL");
> diff --git a/include/uapi/linux/sensehat.h b/include/uapi/linux/sensehat.h
> new file mode 100644
> index 000000000000..cae07568a5eb
> --- /dev/null
> +++ b/include/uapi/linux/sensehat.h
> @@ -0,0 +1,28 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later WITH Linux-syscall-note */
> +/*
> + * Raspberry Pi Sense HAT core 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
> + */
> +
> +#ifndef _UAPILINUX_SENSEHAT_H_
> +#define _UAPILINUX_SENSEHAT_H_
> +
> +#define SENSEDISP_IOC_MAGIC 0xF1
> +
> +#define SENSEDISP_IOGET_GAMMA _IO(SENSEDISP_IOC_MAGIC, 0)
> +#define SENSEDISP_IOSET_GAMMA _IO(SENSEDISP_IOC_MAGIC, 1)
> +#define SENSEDISP_IORESET_GAMMA _IO(SENSEDISP_IOC_MAGIC, 2)
> +
> +enum gamma_preset {
> +	SENSEHAT_GAMMA_DEFAULT = 0,
> +	SENSEHAT_GAMMA_LOWLIGHT,
> +	SENSEHAT_GAMMA_PRESET_COUNT,
> +};
> +
> +#endif

-- 
Nicolás Sáenz

Re: [PATCH 3/6] drivers/auxdisplay: sensehat: Raspberry Pi Sense HAT display driver

[Miguel Ojeda]

On Thu, Feb 3, 2022 at 1:26 AM Charles Mirabile <cmirabil@redhat.com> wrote:
>
> This patch adds the driver for the 8x8 RGB LED matrix display
> on the Sense HAT. It appears as a character device named sense-hat
> in /dev/. That special file is 192 bytes large and contains one byte
> for each color channel of each pixel. The overall layout is:

A general comment about the commit message: I am not sure why the
format of the file is included here -- shouldn't this be placed in
comments in the code or in Documentation? Is it elsewhere in the
series?

>     col 1       col 8
>       |           |
>       v           v
> 0x00: R   . . .   R \
> 0x08: G   . . .   G |> row 1
> 0x10: B   . . .   B /
>       .   .       .
> ...   .     .     .
>       .       .   .
> 0xA8: R   . . .   R \
> 0xB0: G   . . .   G |> row 8
> 0xB8: G   . . .   G /

Do you mean B instead of G in row 8?

By the way, is there a particular reason for this layout vs., say, RGB tuples?

> Each channel may have any value between 0 and 31 (larger values are
> wrapped) and are translated by a gamma table before being send to the
> device to provide a linear experience of brightness (the gamma table
> can be modified or reset using an ioctl call on the file). The constants
> for the ioctl are in the sensehat.h header also added in this patch.

Even though there is a gamma table, it is unclear what the input
encoding is supposed to be. sRGB? Linear sRGB?

In any case, where the gamma tables' values come from? i.e. the
device's datasheet (or sRGB, or a combination of those), or hand-made,
...?

Also, should this conversion be handled by the driver? If yes, why not
use the full range 0..255 as input?

> +       for (i = 0; i < VMEM_SIZE; ++i)
> +               temp[i] = display->gamma[display->vmem[i] & 0x1f];

Please avoid hardcoded constants (and, in particular, this is in hex
while the related constant `GAMMA_SIZE` is in decimal, so it is even
harder to see). Or perhaps use a small helper function or macro.

> +       pos = base + offset;
> +       if (pos < 0 || pos >= VMEM_SIZE)
> +               return -EINVAL;
> +       filp->f_pos = pos;

Please check if one of the generic `llseek`'s would work here (e.g.
`fixed_size_llseek`).

> +       memcpy(sensehat_display->gamma, gamma_presets[lowlight], GAMMA_SIZE);

Here it is confusing which table you are copying due to the `lowlight`
name. I would write `lowlight ? SENSEHAT_GAMMA_LOWLIGHT :
SENSEHAT_GAMMA_DEFAULT` even if it is redundant.

(Given you created an enum for the presets, it may make sense to use
something else than a `bool`, by the way).

Cheers,
Miguel