Add support for reTerminal, a Raspberry Pi 4 CM carrier board #4622

AIWintermuteAI · 2021-10-08T08:58:10Z

This PR adds a seeed-reterminal-core overlay and enables building necessary drivers as modules to support reTerminal, a Raspberry Pi 4 CM carrier board.

Seeed Studio reTerminal is a Human-Machine Interface (HMI) carrier board
for Raspberry Pi 4 CM with 5-inch LCD screen, accelerometer,
light sensor, GPIO buttons and user LEDs. reTerminal can easily and
efficiently work with IoT and cloud systems to unlock endless scenarios
at the edge.

LTR light sensor driver was modified to make it compatible with LTR303.
NXP PCA953xx I2C Mux/switch support was added
LIS331DLH accelerometer driver is unchanged, enabled building as a module
added display/touchscreen driver

Signed-off-by: Maslov Dmitry [email protected]

pelwell

There are several problems with this PR:

It contains several drivers (and changes to existing drivers) that aren't Pi-specific and should be upstreamed - we won't take on the support burden.
The overlay and README have a few stylistic issues - see the inline comments.
The defconfig patches can be squashed into a single one - we will do that eventually, so they might as well start out that way.

arch/arm/boot/dts/overlays/README

arch/arm/boot/dts/overlays/seeed-reterminal-core-overlay.dts

drivers/iio/light/Kconfig

drivers/iio/light/ltr.c

drivers/i2c/muxes/Kconfig

arch/arm/boot/dts/overlays/README

arch/arm/boot/dts/overlays/seeed-reterminal-core-overlay.dts

lategoodbye · 2021-10-08T13:23:01Z

@AIWintermuteAI FWIW CM4 and CM4 IO support will land in Linux 5.16. The DTS approach differs a little bit from this tree, because of missing full overlay support in upstream.

AIWintermuteAI · 2021-10-09T20:51:32Z

@pelwell Thanks for detailed review!
Alright, so from what I see now, it seems the best way is to:

do the rebase on commit branch and drop commits related to ltr501 and NXP PCA953xx drivers, since these need to be sent to Linux kernel
fix the empty lines and some issues in overlay
leave touch panel as-is
squash defconfig patches to one (namely for now that only would be the touch panel and acceleromter).
Is that correct?

AIWintermuteAI · 2021-10-09T20:53:06Z

@lategoodbye Thanks for the heads-up! Do you mean that the overlay in my PR needs to be modified because of that?

lategoodbye · 2021-10-10T16:25:42Z

@AIWintermuteAI There is no need to modify the overlay in this PR.

pelwell · 2021-10-11T14:17:01Z

I don't see why the touchpanel driver should be treated any differently - it's also generic code that could be useful for other platforms, so it should go upstream.

6by9

This feels like a bit of a mismash with multiple functions crammed into one driver.

ILI9881D would appear to be a standalone panel, although it does need GPIO control for reset control and/or regulators.
https://github.com/raspberrypi/linux/blob/rpi-5.10.y/arch/arm/boot/dts/overlays/vc4-kms-dsi-7inch-overlay.dts has been updated to use the newer drivers which split this functionality for the Pi panel into independent modules and stayed within the normal frameworks.

We also seem to have some new custom device tree properties which have no documentation.

6by9 · 2021-10-11T17:10:16Z

drivers/i2c/muxes/i2c-mux-pca953xx.c

@@ -0,0 +1,1413 @@
+// SPDX-License-Identifier: GPL-2.0-only


You've added this to drivers/i2c/muxes, but it's an I2C GPIO expander. It also appears to be a copy of /driver/gpio/gpio-pca953x, including duplicating a large number of the compatible strings.

6by9 · 2021-10-11T17:18:50Z

drivers/gpu/drm/panel/panel-ilitek-ili9881d.c

+	.vdisplay	= 1280,
+	.vsync_start= 1280 + 10,
+	.vsync_end	= 1280 + 10 + 20,
+	.vtotal		= 1280 + 10 + 20 + 30,


This timing looks identical to lhr050h41_default_mode in drivers/gpu/drm/panel/panel-ilitek-9881c.c
Are they actually any different?

6by9 · 2021-10-11T17:19:32Z

drivers/gpu/drm/panel/panel-ilitek-ili9881d.c

+#include "panel-reTerminal.h"
+
+
+static struct mipi_dsi_device *ili9881d_dsi = NULL;


One static variable, so how would I add 2 of these to a device?

6by9 · 2021-10-11T17:40:48Z

drivers/gpu/drm/panel/panel-ilitek-ili9881d.c

+#define IILI9881_COMMAND(_cmd,_data...)		DSI_DCS_WRITE(dsi,_cmd,_data)
+static int ili9881d_prepare(struct drm_panel *panel)
+{
+	struct mipi_dsi_device *dsi = ili9881d_dsi;


struct i2c_mipi_dsi *md = panel_to_md(panel); struct mipi_dsi_device *dsi = md->dsi;

Gets rid of ili9881d_dsi

6by9 · 2021-10-11T17:55:26Z

Looking at it, your commit text says of the touchscreen

however it is connected to the carrier board through a STM32G030 MCU,
which makes a separate driver necessary.

If it is the vanilla Goodix touch-controller, it'd seem highly plausible to write a very simple wrapper that looked like an I2C controller to take requests from the existing driver and encapsulate them for the MCU. It largely looks like you're splitting the write and read into two transactions with a delay, so that should be pretty trivial to implement.
Touchscreen rotation isn't normally handled in the driver, so your x_y_rotate function looks dubious.

arch/arm/configs/bcm2711_defconfig

AIWintermuteAI · 2021-10-13T10:44:12Z

@pelwell I dropped all the commits relating to the modules that you said need to go to upstream. What's left now is overlay and enabling building LIS331DLH driver as a module.
I'll review the code for the rest of devices and prepare the PRs to upstream. That might take a while of course.
@6by9 Thank you for the comments. Yes, I agree, it does seem the touch-screen panel driver needs some work. Can you point to some examples, where similar driver was implemented in the way that would conform to Raspberry Pi foundation/Linux kernel standards?

pelwell

If merging this now is useful to you then fix the hard TAB, squash down to two commits - overlay and config - and update the PR one more time.

arch/arm/boot/dts/overlays/README

6by9 · 2021-10-13T15:28:45Z

Can you point to some examples, where similar driver was implemented in the way that would conform to Raspberry Pi foundation/Linux kernel standards?

https://github.com/raspberrypi/linux/blob/rpi-5.10.y/arch/arm/boot/dts/overlays/vc4-kms-dsi-7inch-overlay.dts is the closest as it uses an Atmel as backlight, regulator, and GPIO controller. Remove the TC358762 bridge and panel_disp1, moving to a single ILI9881 panel under &dsi1 instead.

ILI9881C already has a driver at https://github.com/raspberrypi/linux/blob/rpi-5.10.y/drivers/gpu/drm/panel/panel-ilitek-ili9881c.c
I don't know how close to ILI9881D that is, but if your device can be supported by just adding an extra compatible string and init sequence, then so much the better.

The I2C wrapper for the MCU should be relatively simple in taking a parent I2C device and creating a new one. The transfer function then takes in the request and issues multiple transfers to the parent, with delays between each.

6by9 · 2021-10-13T15:31:16Z

DRM requires all components to be available before it binds them all. I suspect with dsi1 enabled but no panel driver in the build, adding this overlay will lead to all displays going blank as DRM can never bind. Is that actually useful?

pelwell · 2021-10-13T18:56:46Z

Apart from @6by9's question ("Is [all display's blank] actually useful?") I'm ready to merge. I'm also happy to park this until progress has been made on getting the necessary drivers into the kernel.

6by9 · 2021-10-15T11:05:59Z

By coincidence a patch has just been submitted to dri-devel that claims to add an ILI9881D panel to the ILI9881C driver. That implies that they are close enough to reuse the same driver (or it's a typo!)
https://lists.freedesktop.org/archives/dri-devel/2021-October/327570.html

AIWintermuteAI · 2021-10-18T07:31:59Z

@pelwell I discussed it with other members on the team and we decided it's better to park PR for a time being, while we work on sending other drivers upstream.
@6by9 Thanks for the information, I'll have a look at the patch!

This allows using reTerminal carrier board built-in sensors and LCD display/touch screen. Seeed Studio reTerminal is a Human-Machine Interface (HMI) carrier board for Raspberry Pi 4 CM with 5-inch LCD screen, accelerometer, light sensor, GPIO buttons and user LEDs. reTerminal can easily and efficiently work with IoT and cloud systems to unlock endless scenarios at the edge. Signed-off-by: Maslov Dmitry <[email protected]>

Enabled building LIS331DLH drivers as module, used in Seeed Studio reTerminal carrier board for Raspberry Pi CM 4. Signed-off-by: Maslov Dmitry <[email protected]>

pelwell requested changes Oct 8, 2021

View reviewed changes

6by9 reviewed Oct 11, 2021

View reviewed changes

AIWintermuteAI force-pushed the rpi-5.10.y branch from 5524210 to 2d34f9a Compare October 13, 2021 09:26

pelwell reviewed Oct 13, 2021

View reviewed changes

arch/arm/configs/bcm2711_defconfig Outdated Show resolved Hide resolved

AIWintermuteAI force-pushed the rpi-5.10.y branch from 2d34f9a to 542eb1b Compare October 13, 2021 10:36

pelwell reviewed Oct 13, 2021

View reviewed changes

arch/arm/boot/dts/overlays/README Outdated Show resolved Hide resolved

AIWintermuteAI force-pushed the rpi-5.10.y branch from 542eb1b to 1403669 Compare October 13, 2021 18:02

pelwell approved these changes Oct 13, 2021

View reviewed changes

pelwell force-pushed the rpi-5.10.y branch from 1403669 to 4a669d1 Compare October 13, 2021 19:11

pelwell added the Waiting for external input Waiting for a comment from the originator of the issue, or a collaborator. label Oct 18, 2021

AIWintermuteAI force-pushed the rpi-5.10.y branch from 4a669d1 to d6afe37 Compare October 26, 2021 18:05

configs: enabled building LIS331DLH as module

6ef7328

Enabled building LIS331DLH drivers as module, used in Seeed Studio reTerminal carrier board for Raspberry Pi CM 4. Signed-off-by: Maslov Dmitry <[email protected]>

		#include "panel-reTerminal.h"


		static struct mipi_dsi_device *ili9881d_dsi = NULL;

Add support for reTerminal, a Raspberry Pi 4 CM carrier board #4622

Are you sure you want to change the base?

Add support for reTerminal, a Raspberry Pi 4 CM carrier board #4622

Uh oh!

Conversation

AIWintermuteAI commented Oct 8, 2021

Uh oh!

pelwell left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lategoodbye commented Oct 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AIWintermuteAI commented Oct 9, 2021

Uh oh!

AIWintermuteAI commented Oct 9, 2021

Uh oh!

lategoodbye commented Oct 10, 2021

Uh oh!

pelwell commented Oct 11, 2021

Uh oh!

6by9 left a comment

Choose a reason for hiding this comment

Uh oh!

6by9 Oct 11, 2021

Choose a reason for hiding this comment

Uh oh!

6by9 Oct 11, 2021

Choose a reason for hiding this comment

Uh oh!

6by9 Oct 11, 2021

Choose a reason for hiding this comment

Uh oh!

6by9 Oct 11, 2021

Choose a reason for hiding this comment

Uh oh!

6by9 commented Oct 11, 2021

Uh oh!

Uh oh!

AIWintermuteAI commented Oct 13, 2021

Uh oh!

pelwell left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

6by9 commented Oct 13, 2021

Uh oh!

6by9 commented Oct 13, 2021

Uh oh!

pelwell commented Oct 13, 2021

Uh oh!

6by9 commented Oct 15, 2021

Uh oh!

AIWintermuteAI commented Oct 18, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

lategoodbye commented Oct 8, 2021 •

edited

Loading