linux-kernel - Re: [PATCH v3 0/9] fix reset line polarity for Goodix touchscreen controllers

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Mon, 5 Dec 2022 23:19:17 +0100
From:   Hans de Goede <hdegoede@...hat.com>
To:     Quentin Schulz <foss+kernel@...il.net>,
        Samuel Holland <samuel@...lland.org>,
        Bastien Nocera <hadess@...ess.net>,
        Guido Günther <agx@...xcpu.org>,
        Sascha Hauer <s.hauer@...gutronix.de>,
        Pengutronix Kernel Team <kernel@...gutronix.de>,
        Angus Ainslie <angus@...ea.ca>,
        Ondrej Jirman <megous@...ous.com>,
        Icenowy Zheng <icenowy@...c.io>,
        Andy Gross <agross@...nel.org>,
        Aleksei Mamlin <mamlinav@...il.com>,
        Fabio Estevam <festevam@...il.com>,
        David Jander <david@...tonic.nl>,
        Frieder Schrempf <frieder.schrempf@...tron.de>,
        Bjorn Andersson <andersson@...nel.org>,
        Konrad Dybcio <konrad.dybcio@...ainline.org>,
        Peter Geis <pgwipeout@...il.com>,
        Heiko Stuebner <heiko@...ech.de>,
        Shawn Guo <shawnguo@...nel.org>,
        Jernej Skrabec <jernej.skrabec@...il.com>,
        Lukasz Majewski <lukma@...x.de>,
        AngeloGioacchino Del Regno 
        <angelogioacchino.delregno@...ainline.org>,
        Chen-Yu Tsai <wens@...e.org>,
        Michael Riesch <michael.riesch@...fvision.net>,
        Rob Herring <robh+dt@...nel.org>,
        NXP Linux Team <linux-imx@....com>,
        Dmitry Torokhov <dmitry.torokhov@...il.com>,
        Jagan Teki <jagan@...rulasolutions.com>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>
Cc:     Quentin Schulz <quentin.schulz@...obroma-systems.com>,
        linux-input@...r.kernel.org, linux-arm-msm@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
        linux-sunxi@...ts.linux.dev, devicetree@...r.kernel.org,
        linux-rockchip@...ts.infradead.org
Subject: Re: [PATCH v3 0/9] fix reset line polarity for Goodix touchscreen
 controllers

Hi Quentin,

On 12/5/22 14:40, Quentin Schulz wrote:
> From: Quentin Schulz <quentin.schulz@...obroma-systems.com>
> 
> The Goodix touchscreen controller has a reset line active low. It happens to
> also be used to configure its i2c address at runtime. If the reset line is
> incorrectly asserted, the address will be wrongly configured. This cost me a few
> hours, trying to figure out why the touchscreen wouldn't work.
> 
> The driver is "asserting" this reset GPIO by setting its output to 0, probably
> to reflect the physical state of the line. However, this relies on the fact that
> the Device Tree node setting the reset line polarity to active high, which is
> incorrect since the reset is active low in hardware.
> 
> To fix this inconsistency, the polarity is inverted to not confuse the user
> about the reset line polarity. This obviously requires to fix the DT since most
> users had the "incorrect" value in there, it needs to be inverted.
> Note that the v2 highlighted that I was not the only one that got confused since
> PRT8MM board has the "correct" HW representation for this line in DT (which does
> not match what the driver was expecting).
> 
> This is marked as RFC because I can neither test ACPI support nor boards I don't
> own. Please test on the boards you have that are impacted by this patchset and
> give your Tested-By.

I have tested this on a x86/ACPI device where we actually need to reset
the controller at boot to get it to work and things still work fine there
after this series.

I've also reviewd patches 1-3 and they look good to me to.

So for patches 1-3 you may add my:

Tested-by: Hans de Goede <hdegoede@...hat.com>
Reviewed-by: Hans de Goede <hdegoede@...hat.com>

Regards,

Hans





> Do we also make this patch series only one patchset since the DT patches depend
> on the driver patch and vice-versa? In which tree would this go?
> 
> Thanks,
> Quentin
> 
> To: Bastien Nocera <hadess@...ess.net>
> To: Hans de Goede <hdegoede@...hat.com>
> To: Dmitry Torokhov <dmitry.torokhov@...il.com>
> To: Rob Herring <robh+dt@...nel.org>
> To: Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>
> To: Shawn Guo <shawnguo@...nel.org>
> To: Sascha Hauer <s.hauer@...gutronix.de>
> To: Pengutronix Kernel Team <kernel@...gutronix.de>
> To: Fabio Estevam <festevam@...il.com>
> To: NXP Linux Team <linux-imx@....com>
> To: Chen-Yu Tsai <wens@...e.org>
> To: Jernej Skrabec <jernej.skrabec@...il.com>
> To: Samuel Holland <samuel@...lland.org>
> To: Andy Gross <agross@...nel.org>
> To: Bjorn Andersson <andersson@...nel.org>
> To: Konrad Dybcio <konrad.dybcio@...ainline.org>
> To: Heiko Stuebner <heiko@...ech.de>
> To: David Jander <david@...tonic.nl>
> To: Angus Ainslie <angus@...ea.ca>
> To: Peter Geis <pgwipeout@...il.com>
> To: Michael Riesch <michael.riesch@...fvision.net>
> To: Konrad Dybcio <konrad.dybcio@...ainline.org>
> To: AngeloGioacchino Del Regno <angelogioacchino.delregno@...ainline.org>
> To: Guido Günther <agx@...xcpu.org>
> To: Jagan Teki <jagan@...rulasolutions.com>
> To: Ondrej Jirman <megous@...ous.com>
> To: Icenowy Zheng <icenowy@...c.io>
> To: Aleksei Mamlin <mamlinav@...il.com>
> To: Lukasz Majewski <lukma@...x.de>
> To: Frieder Schrempf <frieder.schrempf@...tron.de>
> Cc: linux-input@...r.kernel.org
> Cc: linux-kernel@...r.kernel.org
> Cc: devicetree@...r.kernel.org
> Cc: linux-arm-kernel@...ts.infradead.org
> Cc: linux-sunxi@...ts.linux.dev
> Cc: linux-arm-msm@...r.kernel.org
> Cc: linux-rockchip@...ts.infradead.org
> Signed-off-by: Quentin Schulz <quentin.schulz@...obroma-systems.com>
> ---
> Changes in v3:
> - Cc'ing people who contributed to DTS of impacted boards,
> - removed PRT8MM DTS change since it's been reported the polarity is actually
>   correct (goes through an inverter), keeping the appropriate folks in Cc though
>   since it'd be a good idea to check this patch series anyways,
> - added ACPI_GPIO_QUIRK_NO_IO_RESTRICTION to acpi_gpio_mapping quirks to make
>   gpiolib-acpi core respect GPIOD_ASIS flag in gpiod_get,
> - checked schematics of:
>   - pinephone: https://files.pine64.org/doc/PinePhone/PinePhone%20v1.2%20Released%20Schematic.pdf
>   - pinetab: https://files.pine64.org/doc/PineTab/PineTab%20Schematic%20v1.2-20191125.pdf
>   - px30 evb: https://opensource.rock-chips.com/images/d/db/Px30_mini_evb_v10_20180528.pdf
>   - rockpro64: https://files.pine64.org/doc/rockpro64/rockpro64_v21-SCH.pdf
>   - librem5 devkit: https://source.puri.sm/Librem5/dvk-mx8m-bsb/blob/master/dvk-mx8m-bsb.pdf
> 
>   All seems to be directly connected to the GPIO on the SoC side, without an
>   inverter on the line.
> - Link to v2: https://lore.kernel.org/r/20221103-upstream-goodix-reset-v2-0-2c38fb03a300@theobroma-systems.com
> 
> Changes in v2:
> - implemented ACPI support as suggested by Hans,
> - removed Qcom SC7180 Trogdor-based devices changes as they are not using this Goodix driver,
> - added comment on how to read gpiod_request_output and the GPIO DT polarity,
> - Link to v1: https://lore.kernel.org/r/20221103-upstream-goodix-reset-v1-0-87b49ae589f1@theobroma-systems.com
> 
> ---
> Quentin Schulz (9):
>       Input: goodix - add macro for gpio mapping
>       Input: goodix - make gpiod_get honor GPIOD_ASIS
>       Input: goodix - fix reset polarity
>       ARM: dts: imx: fix touchscreen reset GPIO polarity
>       ARM: dts: sunxi: fix touchscreen reset GPIO polarity on Wexler TAB7200 tablet
>       arm64: dts: allwinner: fix touchscreen reset GPIO polarity
>       arm64: dts: librem5: fix touchscreen reset GPIO polarity
>       arm64: dts: qcom: msm8998-fxtec: fix touchscreen reset GPIO polarity
>       arm64: dts: rockchip: fix touchscreen reset GPIO polarity
> 
>  arch/arm/boot/dts/imx6q-kp.dtsi                    |  2 +-
>  arch/arm/boot/dts/imx6ul-kontron-bl-43.dts         |  2 +-
>  arch/arm/boot/dts/sun7i-a20-wexler-tab7200.dts     |  2 +-
>  .../dts/allwinner/sun50i-a64-amarula-relic.dts     |  2 +-
>  .../allwinner/sun50i-a64-oceanic-5205-5inmfd.dts   |  2 +-
>  .../boot/dts/allwinner/sun50i-a64-pinephone.dtsi   |  2 +-
>  .../boot/dts/allwinner/sun50i-a64-pinetab.dts      |  2 +-
>  .../boot/dts/freescale/imx8mq-librem5-devkit.dts   |  2 +-
>  arch/arm64/boot/dts/qcom/msm8998-fxtec-pro1.dts    |  2 +-
>  arch/arm64/boot/dts/rockchip/px30-evb.dts          |  2 +-
>  arch/arm64/boot/dts/rockchip/rk3399-rockpro64.dtsi |  2 +-
>  arch/arm64/boot/dts/rockchip/rk3568-evb1-v10.dts   |  2 +-
>  drivers/input/touchscreen/goodix.c                 | 54 ++++++++++++++++++----
>  13 files changed, 56 insertions(+), 22 deletions(-)
> ---
> base-commit: 76dcd734eca23168cb008912c0f69ff408905235
> change-id: 20221103-upstream-goodix-reset-aa1c65994f57
> 
> Best regards,