Re: [PATCH 10/10] ARM: dts: imx6ulz: Add SPI display and touch overlay for BSH SMM-M2

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] ARM: dts: imx6ulz: active-high pendown-gpio breaks ads7846 touch
- [Medium] ARM: dts: imx6ulz: backlight pad mux placed on display@0 instead of backlight
- [Low] ARM: dts: imx6ulz: open-coded fragment and __overlay__ nodes in overlay
--

commit bebf64c08aefd84526e08136a534cf0657beb477
Author: Simon Holesch <simon.holesch@bshg.com>

ARM: dts: imx6ulz: Add SPI display and touch overlay for BSH SMM-M2

This commit adds a Devicetree overlay for an SPI-connected ILI9486 display
and ADS7846 touchscreen on the BSH SMM-M2 board.

> diff --git a/arch/arm/boot/dts/nxp/imx/imx6ulz-bsh-smm-m2-ws13506-ads7846.dtso b/arch/arm/boot/dts/nxp/imx/imx6ulz-bsh-smm-m2-ws13506-ads7846.dtso
> new file mode 100644
> index 0000000000000..869443dd648e8
> --- /dev/null
> +++ b/arch/arm/boot/dts/nxp/imx/imx6ulz-bsh-smm-m2-ws13506-ads7846.dtso
> @@ -0,0 +1,99 @@
> +// SPDX-License-Identifier: (GPL-2.0+)
> +/*
> + * Copyright 2024 BSH Hausgeraete GmbH
> + */
> +
> +/dts-v1/;
> +/plugin/;
> +
> +#include <dt-bindings/gpio/gpio.h>
> +#include <dt-bindings/interrupt-controller/arm-gic.h>
> +#include "imx6ul-pinfunc.h"
> +
> +/ {
> +	fragment@0 {
> +		target-path = "/";
> +		__overlay__ {

[Severity: Low]
This isn't a bug, but could this overlay use the standard DTC target
reference syntax (&{/}, &ecspi1, and &iomuxc) rather than open-coding
fragment@0, fragment@1, fragment@2 and __overlay__ nodes, matching
Documentation/devicetree/overlay-notes.rst and other .dtso files in
arch/arm/boot/dts/?

> +			backlight: backlight {
> +				compatible = "gpio-backlight";
> +				gpios = <&gpio3 21 GPIO_ACTIVE_HIGH>;
> +			};
> +		};
> +	};
> +
> +	fragment@1 {
> +		target = <&ecspi1>;
> +		__overlay__ {
> +			pinctrl-names = "default";
> +			pinctrl-0 = <&pinctrl_ecspi1>;
> +			cs-gpios = <&gpio3 26 GPIO_ACTIVE_LOW>,
> +				<&gpio2 15 GPIO_ACTIVE_LOW>;
> +			status = "okay";
> +
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +
> +			display@0 {
> +				compatible = "waveshare,rpi-lcd-35", "ilitek,ili9486";
> +				pinctrl-names = "default";
> +				pinctrl-0 = <&pinctrl_lcd>;
> +				reg = <0>;
> +				spi-max-frequency = <32000000>;
> +				dc-gpios = <&gpio1 31 GPIO_ACTIVE_HIGH>;
> +				reset-gpios = <&gpio3 6 GPIO_ACTIVE_HIGH>;
> +				backlight = <&backlight>;
> +				rotation = <180>;
> +			};
> +
> +			touchscreen@1 {
> +				compatible = "ti,ads7846";
> +				pinctrl-names = "default";
> +				pinctrl-0 = <&pinctrl_touchscreen>;
> +				reg = <1>;
> +				interrupt-parent = <&gpio3>;
> +				interrupts = <18 IRQ_TYPE_EDGE_FALLING>;
> +				pendown-gpio = <&gpio3 18 GPIO_ACTIVE_HIGH>;

[Severity: High]
Should pendown-gpio in touchscreen@1 use GPIO_ACTIVE_LOW instead of
GPIO_ACTIVE_HIGH?

The ADS7846 PENIRQ line is active-low (pulled low when touched), which
matches the 100k pull-up in pinctrl_touchscreen (0x1b0b0) and
IRQ_TYPE_EDGE_FALLING.

In drivers/input/touchscreen/ads7846.c, ads7846_setup_pendown() requests
the descriptor via devm_gpiod_get(&spi->dev, "pendown", GPIOD_IN), and
get_pendown_state() calls gpiod_get_value(), which applies the Devicetree
polarity flag:

drivers/input/touchscreen/ads7846.c:get_pendown_state() {
	if (ts->get_pendown_state)
		return ts->get_pendown_state();

	return gpiod_get_value(ts->gpio_pendown);
}

drivers/input/touchscreen/ads7846.c:ads7846_hard_irq() {
	struct ads7846 *ts = handle;

	return get_pendown_state(ts) ? IRQ_WAKE_THREAD : IRQ_HANDLED;
}

With GPIO_ACTIVE_HIGH, gpiod_get_value() returns 0 while GPIO3_IO18 is
physically asserted low on touch, causing ads7846_hard_irq() to return
IRQ_HANDLED without waking the threaded IRQ handler ads7846_irq().

[ ... ]

> +	fragment@2 {
> +		target = <&iomuxc>;
> +		__overlay__ {
> +			pinctrl_ecspi1: ecspi1grp {
> +				fsl,pins = <
> +					MX6UL_PAD_LCD_DATA20__ECSPI1_SCLK    0x100b1
> +					MX6UL_PAD_LCD_DATA22__ECSPI1_MOSI    0x100b1
> +					MX6UL_PAD_LCD_DATA23__ECSPI1_MISO    0x100b1
> +					MX6UL_PAD_LCD_DATA21__GPIO3_IO26     0x0b0b1     /* SPI CS.0 */
> +					MX6UL_PAD_ENET2_RX_ER__GPIO2_IO15    0x0b0b1     /* SPI CS.1 */
> +				>;
> +			};
> +
> +			pinctrl_lcd: lcdgrp {
> +				fsl,pins = <
> +					MX6UL_PAD_LCD_DATA01__GPIO3_IO06       0x180b1     /* LCD_RST */
> +					MX6UL_PAD_UART5_RX_DATA__GPIO1_IO31    0x180b1     /* LCD_DC */
> +					MX6UL_PAD_LCD_DATA16__GPIO3_IO21       0x180b1     /* backlight */
> +				>;
> +			};

[Severity: Medium]
Should MX6UL_PAD_LCD_DATA16__GPIO3_IO21 be moved into its own pinctrl group
and referenced via pinctrl-0 on the backlight node rather than on display@0?

In drivers/gpu/drm/tiny/ili9486.c, ili9486_probe() calls
devm_of_find_backlight(dev) and defers with -EPROBE_DEFER until the
backlight device has finished probing. On i.MX6UL, imx_pmx_ops in
drivers/pinctrl/freescale/pinctrl-imx.c does not implement
.gpio_request_enable, so requesting GPIO3_IO21 in gpio_backlight_probe()
does not mux the pad or apply its pad configuration.

As a result, when gpio_backlight_probe() in
drivers/video/backlight/gpio_backlight.c runs before display@0 binds
pinctrl_lcd:

drivers/video/backlight/gpio_backlight.c:gpio_backlight_probe() {
	...
	if (!of_node || !of_node->phandle)
		/* Not booted with device tree or no phandle link to the node */
		bl->props.power = def_value ? BACKLIGHT_POWER_ON
					    : BACKLIGHT_POWER_OFF;
	else if (gpiod_get_value_cansleep(gbl->gpiod) == 0)
		bl->props.power = BACKLIGHT_POWER_OFF;
	else
		bl->props.power = BACKLIGHT_POWER_ON;

	ret = gpiod_direction_output(gbl->gpiod, !bl->props.power);
	...
}

gpiod_get_value_cansleep() and gpiod_direction_output() execute while
MX6UL_PAD_LCD_DATA16 is still unmuxed and unconfigured.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261104-add-bsh-smm-m2-v1-0-369f0a77e021@cn.bosch.com?part=10