linux-kernel - Re: [PATCH 1/2] input: generalize the Imagis touchscreen driver

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]
Message-ID: <ZROJ/G0hXV/bNIYX@nixie71>
Date:   Tue, 26 Sep 2023 20:48:44 -0500
From:   Jeff LaBundy <jeff@...undy.com>
To:     Karel Balej <balejk@...fyz.cz>
Cc:     Dmitry Torokhov <dmitry.torokhov@...il.com>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Conor Dooley <conor+dt@...nel.org>,
        Markuss Broks <markuss.broks@...il.com>,
        linux-input@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org,
        Duje Mihanović <duje.mihanovic@...le.hr>,
        ~postmarketos/upstreaming@...ts.sr.ht
Subject: Re: [PATCH 1/2] input: generalize the Imagis touchscreen driver

Hi Karel,

On Tue, Sep 26, 2023 at 07:35:23PM +0200, Karel Balej wrote:
> This driver should work with other Imagis ICs of the IST30**C series.
> Make that more apparent.
> 
> Co-developed-by: Duje Mihanović <duje.mihanovic@...le.hr>
> Signed-off-by: Duje Mihanović <duje.mihanovic@...le.hr>
> Signed-off-by: Karel Balej <balejk@...fyz.cz>
> ---
>  ...gis,ist3038c.yaml => imagis,ist30xxc.yaml} |  2 +-
>  MAINTAINERS                                   |  2 +-
>  drivers/input/touchscreen/Kconfig             |  4 +-
>  drivers/input/touchscreen/imagis.c            | 86 +++++++++++--------
>  4 files changed, 52 insertions(+), 42 deletions(-)
>  rename Documentation/devicetree/bindings/input/touchscreen/{imagis,ist3038c.yaml => imagis,ist30xxc.yaml} (99%)
> 
> diff --git a/Documentation/devicetree/bindings/input/touchscreen/imagis,ist3038c.yaml b/Documentation/devicetree/bindings/input/touchscreen/imagis,ist30xxc.yaml
> similarity index 99%
> rename from Documentation/devicetree/bindings/input/touchscreen/imagis,ist3038c.yaml
> rename to Documentation/devicetree/bindings/input/touchscreen/imagis,ist30xxc.yaml
> index 0d6b033fd5fb..09bf3a6acc5e 100644
> --- a/Documentation/devicetree/bindings/input/touchscreen/imagis,ist3038c.yaml
> +++ b/Documentation/devicetree/bindings/input/touchscreen/imagis,ist30xxc.yaml
> @@ -1,7 +1,7 @@
>  # SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>  %YAML 1.2
>  ---
> -$id: http://devicetree.org/schemas/input/touchscreen/imagis,ist3038c.yaml#
> +$id: http://devicetree.org/schemas/input/touchscreen/imagis,ist30xxc.yaml#
>  $schema: http://devicetree.org/meta-schemas/core.yaml#
>  
>  title: Imagis IST30XXC family touchscreen controller
> diff --git a/MAINTAINERS b/MAINTAINERS
> index b19995690904..b23e76418d94 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -10209,7 +10209,7 @@ F:	drivers/usb/atm/ueagle-atm.c
>  IMAGIS TOUCHSCREEN DRIVER
>  M:	Markuss Broks <markuss.broks@...il.com>
>  S:	Maintained
> -F:	Documentation/devicetree/bindings/input/touchscreen/imagis,ist3038c.yaml
> +F:	Documentation/devicetree/bindings/input/touchscreen/imagis,ist30xxc.yaml
>  F:	drivers/input/touchscreen/imagis.c
>  
>  IMGTEC ASCII LCD DRIVER
> diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
> index e3e2324547b9..45503aa2653e 100644
> --- a/drivers/input/touchscreen/Kconfig
> +++ b/drivers/input/touchscreen/Kconfig
> @@ -665,10 +665,10 @@ config TOUCHSCREEN_NOVATEK_NVT_TS
>  	  module will be called novatek-nvt-ts.
>  
>  config TOUCHSCREEN_IMAGIS
> -	tristate "Imagis touchscreen support"
> +	tristate "Imagis IST30XXC touchscreen support"
>  	depends on I2C
>  	help
> -	  Say Y here if you have an Imagis IST30xxC touchscreen.
> +	  Say Y here if you have an Imagis IST30XXC touchscreen.
>  	  If unsure, say N.
>  
>  	  To compile this driver as a module, choose M here: the
> diff --git a/drivers/input/touchscreen/imagis.c b/drivers/input/touchscreen/imagis.c
> index 07111ca24455..4456f1b4d527 100644
> --- a/drivers/input/touchscreen/imagis.c
> +++ b/drivers/input/touchscreen/imagis.c
> @@ -11,25 +11,26 @@
>  #include <linux/property.h>
>  #include <linux/regulator/consumer.h>
>  
> -#define IST3038C_HIB_ACCESS		(0x800B << 16)
> -#define IST3038C_DIRECT_ACCESS		BIT(31)
> -#define IST3038C_REG_CHIPID		0x40001000
> -#define IST3038C_REG_HIB_BASE		0x30000100
> -#define IST3038C_REG_TOUCH_STATUS	(IST3038C_REG_HIB_BASE | IST3038C_HIB_ACCESS)
> -#define IST3038C_REG_TOUCH_COORD	(IST3038C_REG_HIB_BASE | IST3038C_HIB_ACCESS | 0x8)
> -#define IST3038C_REG_INTR_MESSAGE	(IST3038C_REG_HIB_BASE | IST3038C_HIB_ACCESS | 0x4)
> +#define IST30XXC_HIB_ACCESS		(0x800B << 16)
> +#define IST30XXC_DIRECT_ACCESS		BIT(31)
> +#define IST30XXC_REG_CHIPID		0x40001000
> +#define IST30XXC_REG_HIB_BASE		0x30000100
> +#define IST30XXC_REG_TOUCH_STATUS	(IST30XXC_REG_HIB_BASE | IST30XXC_HIB_ACCESS)
> +#define IST30XXC_REG_TOUCH_COORD	(IST30XXC_REG_HIB_BASE | IST30XXC_HIB_ACCESS | 0x8)
> +#define IST30XXC_REG_INTR_MESSAGE	(IST30XXC_REG_HIB_BASE | IST30XXC_HIB_ACCESS | 0x4)
> +#define IST30XXC_CHIP_ON_DELAY_MS	60
> +#define IST30XXC_I2C_RETRY_COUNT	3
> +#define IST30XXC_MAX_FINGER_NUM		10
> +#define IST30XXC_X_MASK			GENMASK(23, 12)
> +#define IST30XXC_X_SHIFT		12
> +#define IST30XXC_Y_MASK			GENMASK(11, 0)
> +#define IST30XXC_AREA_MASK		GENMASK(27, 24)
> +#define IST30XXC_AREA_SHIFT		24
> +#define IST30XXC_FINGER_COUNT_MASK	GENMASK(15, 12)
> +#define IST30XXC_FINGER_COUNT_SHIFT	12
> +#define IST30XXC_FINGER_STATUS_MASK	GENMASK(9, 0)
> +
>  #define IST3038C_WHOAMI			0x38c
> -#define IST3038C_CHIP_ON_DELAY_MS	60
> -#define IST3038C_I2C_RETRY_COUNT	3
> -#define IST3038C_MAX_FINGER_NUM		10
> -#define IST3038C_X_MASK			GENMASK(23, 12)
> -#define IST3038C_X_SHIFT		12
> -#define IST3038C_Y_MASK			GENMASK(11, 0)
> -#define IST3038C_AREA_MASK		GENMASK(27, 24)
> -#define IST3038C_AREA_SHIFT		24
> -#define IST3038C_FINGER_COUNT_MASK	GENMASK(15, 12)
> -#define IST3038C_FINGER_COUNT_SHIFT	12
> -#define IST3038C_FINGER_STATUS_MASK	GENMASK(9, 0)
>  

There is no need to change all of the namespacing from IST3038C to IST30XXC;
this is purely cosmetic and adds noise to the actual patch.

It is perfectly acceptable for the driver to call itself IST3038C throughout
while remaining compatible with other devices (e.g. IST3032C), in fact this
flexibility is the intent of the compatible string in the first place.

[...]
>  
> -MODULE_DESCRIPTION("Imagis IST3038C Touchscreen Driver");
> +MODULE_DESCRIPTION("Imagis IST30XXC Touchscreen Driver");
>  MODULE_AUTHOR("Markuss Broks <markuss.broks@...il.com>");
>  MODULE_LICENSE("GPL");
> -- 
> 2.42.0
> 

Kind regards,
Jeff LaBundy