by Matthias Brugger

[permalink] [raw]

Subject: Re: [PATCH 3/5] dt-bindings: mfd: sensehat: Add Raspberry Pi Sense HAT schema

On 23/08/2022 19:41, 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: Joel Savitz <[email protected]>
> Signed-off-by: Joel Savitz <[email protected]>
> Signed-off-by: Charles Mirabile <[email protected]>
> Reviewed-by: Krzysztof Kozlowski <[email protected]>

When doing a v11, maybe decide if you want to split this patch up. This way
every maintainer (MFD, auxdisplay, input) can take his part of the binding
description through his tree. Otherwise you will need Acked-by tags from them
and one maintainer to volunteer, maybe provide a stable tag... Having it split
up seems easier to me.

Regards,
Matthias

> ---
> .../raspberrypi,sensehat-display.yaml | 26 +++++++++
> .../input/raspberrypi,sensehat-joystick.yaml | 31 ++++++++++
> .../bindings/mfd/raspberrypi,sensehat.yaml | 57 +++++++++++++++++++
> 3 files changed, 114 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..0cf59b0a519c
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/auxdisplay/raspberrypi,sensehat-display.yaml
> @@ -0,0 +1,26 @@
> +# 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 <[email protected]>
> + - Joel Savitz <[email protected]>
> +
> +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..98b03878a570
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/input/raspberrypi,sensehat-joystick.yaml
> @@ -0,0 +1,31 @@
> +# 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 <[email protected]>
> + - Joel Savitz <[email protected]>
> +
> +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:
> + 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..bc53834fbaa7
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mfd/raspberrypi,sensehat.yaml
> @@ -0,0 +1,57 @@
> +# 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 <[email protected]>
> + - Joel Savitz <[email protected]>
> +
> +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:
> + maxItems: 1
> +
> + joystick:
> + $ref: /schemas/input/raspberrypi,sensehat-joystick.yaml#
> +
> + display:
> + $ref: /schemas/auxdisplay/raspberrypi,sensehat-display.yaml#
> +
> +required:
> + - compatible
> + - reg
> + - joystick
> + - display
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> + #include <dt-bindings/interrupt-controller/irq.h>
> + i2c {
> + #address-cells = <1>;
> + #size-cells = <0>;
> + hat@46 {
> + compatible = "raspberrypi,sensehat";
> + reg = <0x46>;
> + display {
> + compatible = "raspberrypi,sensehat-display";
> + };
> + joystick {
> + compatible = "raspberrypi,sensehat-joystick";
> + interrupts = <23 IRQ_TYPE_EDGE_RISING>;
> + };
> + };
> + };

2022-08-24 15:52:47

On Tue, Aug 23, 2022 at 01:41:53PM -0400, Charles Mirabile wrote:
> 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.
>

Better say "Add a set of drivers for Sense HAT peripheral add-on device
for Raspberry Pi. The device is connected using GPIO connector and
communicate via I2C".

> 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.
>

s/We modified his code/The driver code is modified/

Also, I see that description of individual patches is written in
descriptive mood. However, it is preferable to write in imperative
instead (see "Describe your changes" in
Documentation/process/submitting-patches.rst). The specific suggested
wordings are in reply to each patch.

Thanks.

--
An old man doll... just what I always wanted! - Clara

Attachments:

(No filename) (1.07 kB)
signature.asc (235.00 B)
Download all attachments

2022-09-06 02:35:38

by Bagas Sanjaya

[permalink] [raw]

On Tue, Aug 23, 2022 at 01:41:55PM -0400, 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 64
> RGB triplets (3 bytes each) one for each pixel in row major order.
>
> Co-developed-by: Joel Savitz <[email protected]>
> Signed-off-by: Joel Savitz <[email protected]>
> Signed-off-by: Charles Mirabile <[email protected]>
> ---
> drivers/auxdisplay/Kconfig | 8 +
> drivers/auxdisplay/Makefile | 1 +
> drivers/auxdisplay/sensehat-display.c | 205 ++++++++++++++++++++++++++
> 3 files changed, 214 insertions(+)
> create mode 100644 drivers/auxdisplay/sensehat-display.c
>
> 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..08c0c4338f3e
> --- /dev/null
> +++ b/drivers/auxdisplay/sensehat-display.c
> @@ -0,0 +1,205 @@
> +// 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, Joel Savitz
> + *
> + * Original Author: Serge Schneider
> + * Revised for upstream Linux by: Charles Mirabile, 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>
> +
> +#define DISPLAY_SMB_REG 0x00
> +#define RGB_555_MASK 0x1f
> +#define NUM_LEDS 8
> +#define NUM_CHANNELS 3
> +
> +struct sensehat_display {
> + struct platform_device *pdev;
> + struct miscdevice mdev;
> + struct mutex rw_mtx;

Calling this rw_mtx is misleading, since it is not a rw_mutex. We should
rename it to mtx.

> + u8 vmem[NUM_LEDS][NUM_LEDS][NUM_CHANNELS];
> + struct regmap *regmap;
> +};
> +
> +#define VMEM_SIZE sizeof_field(struct sensehat_display, vmem)
> +
> +
> +static int sensehat_update_display(struct sensehat_display *display)
> +{
> + int i, j, k, ret;
> + u8 buff[NUM_LEDS][NUM_CHANNELS][NUM_LEDS];
> +
> + for (i = 0; i < NUM_LEDS; ++i)
> + for (j = 0; j < NUM_LEDS; ++j)
> + for (k = 0; k < NUM_CHANNELS; ++k)
> + buff[i][k][j] =
> + display->vmem[i][j][k] & RGB_555_MASK;
> +
> + ret = regmap_bulk_write(display->regmap, DISPLAY_SMB_REG, buff,
> + VMEM_SIZE);
> + if (ret < 0)
> + dev_err(&display->pdev->dev,
> + "Update to 8x8 LED matrix display failed");
> + return ret;
> +}
> +
> +static loff_t sensehat_display_llseek(struct file *filp, loff_t offset,
> + int whence)
> +{
> + return fixed_size_llseek(filp, offset, whence, VMEM_SIZE);
> +}
> +
> +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 ret = -EFAULT;
> +
> + if (*f_pos < 0 || *f_pos >= VMEM_SIZE)
> + return 0;

Maybe it should return -EINVAL?

> + count = min_t(size_t, count, VMEM_SIZE - *f_pos);
> +
> + if (mutex_lock_interruptible(&sensehat_display->rw_mtx))
> + return -ERESTARTSYS;
> + if (copy_to_user(buf, *f_pos + (u8 *)sensehat_display->vmem, count))
> + goto out;
> + *f_pos += count;
> + ret = count;
> +out:
> + mutex_unlock(&sensehat_display->rw_mtx);
> + return ret;
> +}
> +
> +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 = -EFAULT;
> +
> + if (*f_pos < 0 || *f_pos >= VMEM_SIZE)
> + return -EFBIG;
> + count = min_t(size_t, count, VMEM_SIZE - *f_pos);
> +
> + if (mutex_lock_interruptible(&sensehat_display->rw_mtx))
> + return -ERESTARTSYS;
> + if (copy_from_user(*f_pos + (u8 *)sensehat_display->vmem, buf, count))
> + goto out;
> + ret = sensehat_update_display(sensehat_display);
> + if (ret < 0) {
> + ret = -EIO;
> + goto out;
> + }
> + *f_pos += count;
> + ret = count;
> +out:
> + 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,
> +};
> +
> +static int sensehat_display_probe(struct platform_device *pdev)
> +{
> + int ret;
> +
> + struct sensehat_display *sensehat_display =
> + devm_kmalloc(&pdev->dev, sizeof(*sensehat_display), GFP_KERNEL);
> + if (!sensehat_display)
> + return -ENOMEM;
> +
> + sensehat_display->pdev = pdev;
> +
> + dev_set_drvdata(&pdev->dev, sensehat_display);
> +
> + sensehat_display->regmap = dev_get_regmap(pdev->dev.parent, NULL);
> + if (!sensehat_display->regmap) {
> + dev_err(&pdev->dev,
> + "unable to get sensehat regmap");
> + return -ENODEV;
> + }
> +
> + memset(sensehat_display->vmem, 0, VMEM_SIZE);
> +
> + mutex_init(&sensehat_display->rw_mtx);
> +
> + ret = sensehat_update_display(sensehat_display);
> + if (ret < 0) {
> + dev_err(&pdev->dev,
> + "Could not communicate with sensehat");
> + 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.");
> + return ret;
> + }
> +
> + dev_info(&pdev->dev,
> + "8x8 LED matrix display registered with minor number %i",
> + sensehat_display->mdev.minor);
> +
> + return 0;
> +}
> +
> +static int sensehat_display_remove(struct platform_device *pdev)
> +{
> + struct sensehat_display *sensehat_display = dev_get_drvdata(&pdev->dev);
> +
> + misc_deregister(&sensehat_display->mdev);
> + 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,
> + .remove = sensehat_display_remove,
> + .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("Charles Mirabile <[email protected]>");
> +MODULE_AUTHOR("Serge Schneider <[email protected]>");
> +MODULE_LICENSE("GPL");
> --
> 2.31.1
>
>