[PATCH 0/6] input: himax_hx83112b: add support for HX83100A

[PATCH 0/6] input: himax_hx83112b: add support for HX83100A

[Felix Kaechele]

This set of patches brings support for the Himax HX83100A touch
controller.
To properly bring the chip up, support for regulator handling is also
added.

I have no access to datasheets. So, like the original driver code
that's being extended here, this code is mostly based on the quite
convoluted, GPLv2 licensed manufacturer drivers for Android.
I included links to sources and references where appropriate.

A number of people tested this patch set on Lenovo ThinkSmart View
(CD-18781Y) devices. That device has a variant utilizing a Innolux
P080DDD-AB2 LCM. This LCM comes with the HX83100A.

I would really appreciate if people using HX83112B chips could give this
set a run to ensure nothing broke.

Thanks,
Felix

Felix Kaechele (6):
  dt-bindings: input: touchscreen: himax,hx83112b: add HX83100A
  input: himax_hx83112b: add regulator handling
  input: himax_hx83112b: use more descriptive register defines
  input: himax_hx83112b: implement MCU register reading
  input: himax_hx83112b: add himax_chip struct for multi-chip support
  input: himax_hx83112b: add support for HX83100A

 .../input/touchscreen/himax,hx83112b.yaml     |   9 +
 drivers/input/touchscreen/himax_hx83112b.c    | 187 +++++++++++++++---
 2 files changed, 166 insertions(+), 30 deletions(-)


base-commit: 7b4e0b39182cf5e677c1fc092a3ec40e621c25b6
-- 
2.44.0

[PATCH 1/6] dt-bindings: input: touchscreen: himax,hx83112b: add HX83100A

[Felix Kaechele]

This adds a compatible string for the Himax HX83100A touch controller
including the AVDD and VDD supply nodes used by this chip family.

Signed-off-by: Felix Kaechele <felix@kaechele.ca>
---
 .../bindings/input/touchscreen/himax,hx83112b.yaml       | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/Documentation/devicetree/bindings/input/touchscreen/himax,hx83112b.yaml b/Documentation/devicetree/bindings/input/touchscreen/himax,hx83112b.yaml
index f42b23d532eb..5809afedb9a2 100644
--- a/Documentation/devicetree/bindings/input/touchscreen/himax,hx83112b.yaml
+++ b/Documentation/devicetree/bindings/input/touchscreen/himax,hx83112b.yaml
@@ -15,6 +15,7 @@ allOf:
 properties:
   compatible:
     enum:
+      - himax,hx83100a
       - himax,hx83112b
 
   reg:
@@ -26,6 +27,12 @@ properties:
   reset-gpios:
     maxItems: 1
 
+  avdd-supply:
+    description: Analog power supply regulator
+
+  vdd-supply:
+    description: Digital power supply regulator
+
   touchscreen-inverted-x: true
   touchscreen-inverted-y: true
   touchscreen-size-x: true
@@ -54,6 +61,8 @@ examples:
         reg = <0x48>;
         interrupt-parent = <&tlmm>;
         interrupts = <65 IRQ_TYPE_LEVEL_LOW>;
+        avdd-supply = <&avdd_reg>;
+        vdd-supply = <&vdd_reg>;
         touchscreen-size-x = <1080>;
         touchscreen-size-y = <2160>;
         reset-gpios = <&tlmm 64 GPIO_ACTIVE_LOW>;
-- 
2.44.0

[PATCH 2/6] input: himax_hx83112b: add regulator handling

[Felix Kaechele]

Handle regulators used on this chip family, namely AVDD and VDD. These
definitions are taken from the GPLv2 licensed vendor driver.

Signed-off-by: Felix Kaechele <felix@kaechele.ca>
Link: https://github.com/HimaxSoftware/HX83112_Android_Driver
---
 drivers/input/touchscreen/himax_hx83112b.c | 48 ++++++++++++++++++++--
 1 file changed, 44 insertions(+), 4 deletions(-)

diff --git a/drivers/input/touchscreen/himax_hx83112b.c b/drivers/input/touchscreen/himax_hx83112b.c
index 4f6609dcdef3..0a797789e548 100644
--- a/drivers/input/touchscreen/himax_hx83112b.c
+++ b/drivers/input/touchscreen/himax_hx83112b.c
@@ -19,10 +19,12 @@
 #include <linux/interrupt.h>
 #include <linux/kernel.h>
 #include <linux/regmap.h>
+#include <linux/regulator/consumer.h>
 
 #define HIMAX_ID_83112B			0x83112b
 
 #define HIMAX_MAX_POINTS		10
+#define HIMAX_MAX_SUPPLIES		2
 
 #define HIMAX_REG_CFG_SET_ADDR		0x00
 #define HIMAX_REG_CFG_INIT_READ		0x0c
@@ -50,6 +52,7 @@ struct himax_event {
 static_assert(sizeof(struct himax_event) == 56);
 
 struct himax_ts_data {
+	struct regulator_bulk_data supplies[HIMAX_MAX_SUPPLIES];
 	struct gpio_desc *gpiod_rst;
 	struct input_dev *input_dev;
 	struct i2c_client *client;
@@ -63,6 +66,11 @@ static const struct regmap_config himax_regmap_config = {
 	.val_format_endian = REGMAP_ENDIAN_LITTLE,
 };
 
+static const char *const himax_supply_names[] = {
+	"avdd",
+	"vdd",
+};
+
 static int himax_read_config(struct himax_ts_data *ts, u32 address, u32 *dst)
 {
 	int error;
@@ -267,7 +275,7 @@ static irqreturn_t himax_irq_handler(int irq, void *dev_id)
 
 static int himax_probe(struct i2c_client *client)
 {
-	int error;
+	int error, i;
 	struct device *dev = &client->dev;
 	struct himax_ts_data *ts;
 
@@ -290,11 +298,31 @@ static int himax_probe(struct i2c_client *client)
 		return error;
 	}
 
+	int num_supplies = ARRAY_SIZE(himax_supply_names);
+
+	for (i = 0; i < num_supplies; i++)
+		ts->supplies[i].supply = himax_supply_names[i];
+
+	error = devm_regulator_bulk_get(dev,
+					num_supplies,
+					ts->supplies);
+	if (error) {
+		dev_err(dev, "Failed to get supplies: %d\n", error);
+		return error;
+	}
+
+	error = regulator_bulk_enable(num_supplies,
+				      ts->supplies);
+	if (error) {
+		dev_err(dev, "Failed to enable supplies: %d\n", error);
+		goto error_out;
+	}
+
 	ts->gpiod_rst = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
 	error = PTR_ERR_OR_ZERO(ts->gpiod_rst);
 	if (error) {
 		dev_err(dev, "Failed to get reset GPIO: %d\n", error);
-		return error;
+		goto error_out;
 	}
 
 	himax_reset(ts);
@@ -305,15 +333,26 @@ static int himax_probe(struct i2c_client *client)
 
 	error = himax_input_register(ts);
 	if (error)
-		return error;
+		goto error_out;
 
 	error = devm_request_threaded_irq(dev, client->irq, NULL,
 					  himax_irq_handler, IRQF_ONESHOT,
 					  client->name, ts);
 	if (error)
-		return error;
+		goto error_out;
 
 	return 0;
+
+error_out:
+	regulator_bulk_disable(num_supplies, ts->supplies);
+	return error;
+}
+
+static void himax_remove(struct i2c_client *client)
+{
+	struct himax_ts_data *ts = i2c_get_clientdata(client);
+
+	regulator_bulk_disable(ARRAY_SIZE(himax_supply_names), ts->supplies);
 }
 
 static int himax_suspend(struct device *dev)
@@ -350,6 +389,7 @@ MODULE_DEVICE_TABLE(of, himax_of_match);
 
 static struct i2c_driver himax_ts_driver = {
 	.probe = himax_probe,
+	.remove = himax_remove,
 	.id_table = himax_ts_id,
 	.driver = {
 		.name = "Himax-hx83112b-TS",
-- 
2.44.0

[PATCH 3/6] input: himax_hx83112b: use more descriptive register defines

[Felix Kaechele]

Himax uses an AHB-style bus to communicate with different parts of the
display driver and touch controller system.
Use more descriptive names for the register and address defines.
The names were taken from a driver submission for the similar HX83102J
chip.

Signed-off-by: Felix Kaechele <felix@kaechele.ca>
Link: https://lore.kernel.org/all/TY0PR06MB561105A3386E9D76F429110D9E0F2@TY0PR06MB5611.apcprd06.prod.outlook.com/
---
 drivers/input/touchscreen/himax_hx83112b.c | 23 ++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)

diff --git a/drivers/input/touchscreen/himax_hx83112b.c b/drivers/input/touchscreen/himax_hx83112b.c
index 0a797789e548..ba5442cc0a24 100644
--- a/drivers/input/touchscreen/himax_hx83112b.c
+++ b/drivers/input/touchscreen/himax_hx83112b.c
@@ -26,12 +26,14 @@
 #define HIMAX_MAX_POINTS		10
 #define HIMAX_MAX_SUPPLIES		2
 
-#define HIMAX_REG_CFG_SET_ADDR		0x00
-#define HIMAX_REG_CFG_INIT_READ		0x0c
-#define HIMAX_REG_CFG_READ_VALUE	0x08
-#define HIMAX_REG_READ_EVENT		0x30
+#define HIMAX_AHB_ADDR_BYTE_0			0x00
+#define HIMAX_AHB_ADDR_RDATA_BYTE_0		0x08
+#define HIMAX_AHB_ADDR_ACCESS_DIRECTION		0x0c
+#define HIMAX_AHB_ADDR_EVENT_STACK		0x30
 
-#define HIMAX_CFG_PRODUCT_ID		0x900000d0
+#define HIMAX_AHB_CMD_ACCESS_DIRECTION_READ	0x00
+
+#define HIMAX_REG_ADDR_ICID			0x900000d0
 
 #define HIMAX_INVALID_COORD		0xffff
 
@@ -75,15 +77,16 @@ static int himax_read_config(struct himax_ts_data *ts, u32 address, u32 *dst)
 {
 	int error;
 
-	error = regmap_write(ts->regmap, HIMAX_REG_CFG_SET_ADDR, address);
+	error = regmap_write(ts->regmap, HIMAX_AHB_ADDR_BYTE_0, address);
 	if (error)
 		return error;
 
-	error = regmap_write(ts->regmap, HIMAX_REG_CFG_INIT_READ, 0x0);
+	error = regmap_write(ts->regmap, HIMAX_AHB_ADDR_ACCESS_DIRECTION,
+					 HIMAX_AHB_CMD_ACCESS_DIRECTION_READ);
 	if (error)
 		return error;
 
-	error = regmap_read(ts->regmap, HIMAX_REG_CFG_READ_VALUE, dst);
+	error = regmap_read(ts->regmap, HIMAX_AHB_ADDR_RDATA_BYTE_0, dst);
 	if (error)
 		return error;
 
@@ -109,7 +112,7 @@ static int himax_read_product_id(struct himax_ts_data *ts, u32 *product_id)
 {
 	int error;
 
-	error = himax_read_config(ts, HIMAX_CFG_PRODUCT_ID, product_id);
+	error = himax_read_config(ts, HIMAX_REG_ADDR_ICID, product_id);
 	if (error)
 		return error;
 
@@ -243,7 +246,7 @@ static int himax_handle_input(struct himax_ts_data *ts)
 	int error;
 	struct himax_event event;
 
-	error = regmap_raw_read(ts->regmap, HIMAX_REG_READ_EVENT, &event,
+	error = regmap_raw_read(ts->regmap, HIMAX_AHB_ADDR_EVENT_STACK, &event,
 				sizeof(event));
 	if (error) {
 		dev_err(&ts->client->dev, "Failed to read input event: %d\n",
-- 
2.44.0

[PATCH 4/6] input: himax_hx83112b: implement MCU register reading

[Felix Kaechele]

Implement reading from the MCU in a more universal fashion. This allows
properly handling reads of more than 4 bytes using the AHB FIFO
implemented in the chip.

Signed-off-by: Felix Kaechele <felix@kaechele.ca>
---
 drivers/input/touchscreen/himax_hx83112b.c | 51 ++++++++++++++++++++--
 1 file changed, 48 insertions(+), 3 deletions(-)

diff --git a/drivers/input/touchscreen/himax_hx83112b.c b/drivers/input/touchscreen/himax_hx83112b.c
index ba5442cc0a24..0173ff394a99 100644
--- a/drivers/input/touchscreen/himax_hx83112b.c
+++ b/drivers/input/touchscreen/himax_hx83112b.c
@@ -29,9 +29,13 @@
 #define HIMAX_AHB_ADDR_BYTE_0			0x00
 #define HIMAX_AHB_ADDR_RDATA_BYTE_0		0x08
 #define HIMAX_AHB_ADDR_ACCESS_DIRECTION		0x0c
+#define HIMAX_AHB_ADDR_INCR4			0x0d
+#define HIMAX_AHB_ADDR_CONTI			0x13
 #define HIMAX_AHB_ADDR_EVENT_STACK		0x30
 
 #define HIMAX_AHB_CMD_ACCESS_DIRECTION_READ	0x00
+#define HIMAX_AHB_CMD_INCR4			0x10
+#define HIMAX_AHB_CMD_CONTI			0x31
 
 #define HIMAX_REG_ADDR_ICID			0x900000d0
 
@@ -73,10 +77,34 @@ static const char *const himax_supply_names[] = {
 	"vdd",
 };
 
-static int himax_read_config(struct himax_ts_data *ts, u32 address, u32 *dst)
+static int himax_bus_enable_burst(struct himax_ts_data *ts)
 {
 	int error;
 
+	error = regmap_write(ts->regmap, HIMAX_AHB_ADDR_CONTI,
+					 HIMAX_AHB_CMD_CONTI);
+	if (error)
+		return error;
+
+	error = regmap_write(ts->regmap, HIMAX_AHB_ADDR_INCR4,
+					 HIMAX_AHB_CMD_INCR4);
+	if (error)
+		return error;
+
+	return 0;
+}
+
+static int himax_bus_read(struct himax_ts_data *ts, u32 address, void *dst,
+			  size_t length)
+{
+	int error;
+
+	if (length > 4) {
+		error = himax_bus_enable_burst(ts);
+		if (error)
+			return error;
+	}
+
 	error = regmap_write(ts->regmap, HIMAX_AHB_ADDR_BYTE_0, address);
 	if (error)
 		return error;
@@ -86,7 +114,24 @@ static int himax_read_config(struct himax_ts_data *ts, u32 address, u32 *dst)
 	if (error)
 		return error;
 
-	error = regmap_read(ts->regmap, HIMAX_AHB_ADDR_RDATA_BYTE_0, dst);
+	if (length > 4)
+		error = regmap_noinc_read(ts->regmap,
+					  HIMAX_AHB_ADDR_RDATA_BYTE_0,
+					  dst, length);
+	else
+		error = regmap_read(ts->regmap,
+				    HIMAX_AHB_ADDR_RDATA_BYTE_0, dst);
+	if (error)
+		return error;
+
+	return 0;
+}
+
+static int himax_read_mcu(struct himax_ts_data *ts, u32 address, u32 *dst)
+{
+	int error;
+
+	error = himax_bus_read(ts, address, dst, sizeof(dst));
 	if (error)
 		return error;
 
@@ -112,7 +157,7 @@ static int himax_read_product_id(struct himax_ts_data *ts, u32 *product_id)
 {
 	int error;
 
-	error = himax_read_config(ts, HIMAX_REG_ADDR_ICID, product_id);
+	error = himax_read_mcu(ts, HIMAX_REG_ADDR_ICID, product_id);
 	if (error)
 		return error;
 
-- 
2.44.0

[PATCH 5/6] input: himax_hx83112b: add himax_chip struct for multi-chip support

[Felix Kaechele]

In preparation for HX83100A support allow defining separate functions
for specific chip operations.

Signed-off-by: Felix Kaechele <felix@kaechele.ca>
---
 drivers/input/touchscreen/himax_hx83112b.c | 53 +++++++++++++++-------
 1 file changed, 37 insertions(+), 16 deletions(-)

diff --git a/drivers/input/touchscreen/himax_hx83112b.c b/drivers/input/touchscreen/himax_hx83112b.c
index 0173ff394a99..667611c5a018 100644
--- a/drivers/input/touchscreen/himax_hx83112b.c
+++ b/drivers/input/touchscreen/himax_hx83112b.c
@@ -21,8 +21,6 @@
 #include <linux/regmap.h>
 #include <linux/regulator/consumer.h>
 
-#define HIMAX_ID_83112B			0x83112b
-
 #define HIMAX_MAX_POINTS		10
 #define HIMAX_MAX_SUPPLIES		2
 
@@ -57,7 +55,17 @@ struct himax_event {
 
 static_assert(sizeof(struct himax_event) == 56);
 
+struct himax_ts_data;
+struct himax_chip {
+	u32 id;
+	int (*check_id)(struct himax_ts_data *ts);
+	int (*read_events)(struct himax_ts_data *ts,
+			   struct himax_event *event,
+			   size_t length);
+};
+
 struct himax_ts_data {
+	const struct himax_chip *chip;
 	struct regulator_bulk_data supplies[HIMAX_MAX_SUPPLIES];
 	struct gpio_desc *gpiod_rst;
 	struct input_dev *input_dev;
@@ -176,15 +184,12 @@ static int himax_check_product_id(struct himax_ts_data *ts)
 
 	dev_dbg(&ts->client->dev, "Product id: %x\n", product_id);
 
-	switch (product_id) {
-	case HIMAX_ID_83112B:
+	if (product_id == ts->chip->id)
 		return 0;
 
-	default:
-		dev_err(&ts->client->dev,
-			"Unknown product id: %x\n", product_id);
-		return -EINVAL;
-	}
+	dev_err(&ts->client->dev, "Unknown product id: %x\n",
+		product_id);
+	return -EINVAL;
 }
 
 static int himax_input_register(struct himax_ts_data *ts)
@@ -286,13 +291,20 @@ static bool himax_verify_checksum(struct himax_ts_data *ts,
 	return true;
 }
 
+static int himax_read_events(struct himax_ts_data *ts,
+			     struct himax_event *event,
+			     size_t length)
+{
+	return regmap_raw_read(ts->regmap, HIMAX_AHB_ADDR_EVENT_STACK, event,
+			       length);
+}
+
 static int himax_handle_input(struct himax_ts_data *ts)
 {
 	int error;
 	struct himax_event event;
 
-	error = regmap_raw_read(ts->regmap, HIMAX_AHB_ADDR_EVENT_STACK, &event,
-				sizeof(event));
+	error = ts->chip->read_events(ts, &event, sizeof(event));
 	if (error) {
 		dev_err(&ts->client->dev, "Failed to read input event: %d\n",
 			error);
@@ -338,6 +350,7 @@ static int himax_probe(struct i2c_client *client)
 
 	i2c_set_clientdata(client, ts);
 	ts->client = client;
+	ts->chip = i2c_get_match_data(client);
 
 	ts->regmap = devm_regmap_init_i2c(client, &himax_regmap_config);
 	error = PTR_ERR_OR_ZERO(ts->regmap);
@@ -375,9 +388,11 @@ static int himax_probe(struct i2c_client *client)
 
 	himax_reset(ts);
 
-	error = himax_check_product_id(ts);
-	if (error)
-		return error;
+	if (ts->chip->check_id) {
+		error = himax_check_product_id(ts);
+		if (error)
+			return error;
+	}
 
 	error = himax_input_register(ts);
 	if (error)
@@ -421,15 +436,21 @@ static int himax_resume(struct device *dev)
 
 static DEFINE_SIMPLE_DEV_PM_OPS(himax_pm_ops, himax_suspend, himax_resume);
 
+static const struct himax_chip hx83112b_chip = {
+	.id = 0x83112b,
+	.check_id = himax_check_product_id,
+	.read_events = himax_read_events,
+};
+
 static const struct i2c_device_id himax_ts_id[] = {
-	{ "hx83112b", 0 },
+	{ "hx83112b", (kernel_ulong_t) &hx83112b_chip },
 	{ /* sentinel */ }
 };
 MODULE_DEVICE_TABLE(i2c, himax_ts_id);
 
 #ifdef CONFIG_OF
 static const struct of_device_id himax_of_match[] = {
-	{ .compatible = "himax,hx83112b" },
+	{ .compatible = "himax,hx83112b", .data = &hx83112b_chip },
 	{ /* sentinel */ }
 };
 MODULE_DEVICE_TABLE(of, himax_of_match);
-- 
2.44.0

[PATCH 6/6] input: himax_hx83112b: add support for HX83100A

[Felix Kaechele]

The HX83100A is a bit of an outlier in the Himax HX831xxx series of
touch controllers as it requires reading touch events through the AHB
interface of the MCU rather than providing a dedicated FIFO address like
the other chips do.
This patch implements the specific read function and introduces the
HX83100A chip with an appropriate i2c ID and DT compatible string.

The HX83100A doesn't have a straightforward way to do chip
identification, which is why it is not implemented in this patch.

Tested on: Lenovo ThinkSmart View (CD-18781Y) / Innolux P080DDD-AB2 LCM

Signed-off-by: Felix Kaechele <felix@kaechele.ca>
Tested-by: Paul Gale <paul@siliconpixel.com>
---
 drivers/input/touchscreen/himax_hx83112b.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/drivers/input/touchscreen/himax_hx83112b.c b/drivers/input/touchscreen/himax_hx83112b.c
index 667611c5a018..984baef495e1 100644
--- a/drivers/input/touchscreen/himax_hx83112b.c
+++ b/drivers/input/touchscreen/himax_hx83112b.c
@@ -4,6 +4,9 @@
  *
  * Copyright (C) 2022 Job Noorman <job@noorman.info>
  *
+ * HX83100A support
+ * Copyright (C) 2024 Felix Kaechele <felix@kaechele.ca>
+ *
  * This code is based on "Himax Android Driver Sample Code for QCT platform":
  *
  * Copyright (C) 2017 Himax Corporation.
@@ -37,6 +40,8 @@
 
 #define HIMAX_REG_ADDR_ICID			0x900000d0
 
+#define HX83100A_REG_FW_EVENT_STACK		0x90060000
+
 #define HIMAX_INVALID_COORD		0xffff
 
 struct himax_event_point {
@@ -291,6 +296,13 @@ static bool himax_verify_checksum(struct himax_ts_data *ts,
 	return true;
 }
 
+static int hx83100a_read_events(struct himax_ts_data *ts,
+				struct himax_event *event,
+				size_t length)
+{
+	return himax_bus_read(ts, HX83100A_REG_FW_EVENT_STACK, event, length);
+};
+
 static int himax_read_events(struct himax_ts_data *ts,
 			     struct himax_event *event,
 			     size_t length)
@@ -436,6 +448,10 @@ static int himax_resume(struct device *dev)
 
 static DEFINE_SIMPLE_DEV_PM_OPS(himax_pm_ops, himax_suspend, himax_resume);
 
+static const struct himax_chip hx83100a_chip = {
+	.read_events = hx83100a_read_events,
+};
+
 static const struct himax_chip hx83112b_chip = {
 	.id = 0x83112b,
 	.check_id = himax_check_product_id,
@@ -443,6 +459,7 @@ static const struct himax_chip hx83112b_chip = {
 };
 
 static const struct i2c_device_id himax_ts_id[] = {
+	{ "hx83100a", (kernel_ulong_t) &hx83100a_chip },
 	{ "hx83112b", (kernel_ulong_t) &hx83112b_chip },
 	{ /* sentinel */ }
 };
@@ -450,6 +467,7 @@ MODULE_DEVICE_TABLE(i2c, himax_ts_id);
 
 #ifdef CONFIG_OF
 static const struct of_device_id himax_of_match[] = {
+	{ .compatible = "himax,hx83100a", .data = &hx83100a_chip },
 	{ .compatible = "himax,hx83112b", .data = &hx83112b_chip },
 	{ /* sentinel */ }
 };
-- 
2.44.0

Re: [PATCH 1/6] dt-bindings: input: touchscreen: himax,hx83112b: add HX83100A

[Krzysztof Kozlowski]

On 04/05/2024 04:04, Felix Kaechele wrote:
> This adds a compatible string for the Himax HX83100A touch controller

Please do not use "This commit/patch/change", but imperative mood. See
longer explanation here:
https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-patches.rst#L95

> including the AVDD and VDD supply nodes used by this chip family.
> 
> Signed-off-by: Felix Kaechele <felix@kaechele.ca>
> ---
>  .../bindings/input/touchscreen/himax,hx83112b.yaml       | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/input/touchscreen/himax,hx83112b.yaml b/Documentation/devicetree/bindings/input/touchscreen/himax,hx83112b.yaml
> index f42b23d532eb..5809afedb9a2 100644
> --- a/Documentation/devicetree/bindings/input/touchscreen/himax,hx83112b.yaml
> +++ b/Documentation/devicetree/bindings/input/touchscreen/himax,hx83112b.yaml
> @@ -15,6 +15,7 @@ allOf:
>  properties:
>    compatible:
>      enum:
> +      - himax,hx83100a
>        - himax,hx83112b
>  
>    reg:
> @@ -26,6 +27,12 @@ properties:
>    reset-gpios:
>      maxItems: 1
>  
> +  avdd-supply:
> +    description: Analog power supply regulator
> +
> +  vdd-supply:
> +    description: Digital power supply regulator

These should not be allowed for other variant, so either you need
allOf:if:then disallowing them (: false) or just create another binding
file.

Best regards,
Krzysztof

Re: [PATCH 2/6] input: himax_hx83112b: add regulator handling

[Krzysztof Kozlowski]

On 04/05/2024 04:04, Felix Kaechele wrote:
> Handle regulators used on this chip family, namely AVDD and VDD. These
> definitions are taken from the GPLv2 licensed vendor driver.
> 
> Signed-off-by: Felix Kaechele <felix@kaechele.ca>
> Link: https://github.com/HimaxSoftware/HX83112_Android_Driver
> ---
>  drivers/input/touchscreen/himax_hx83112b.c | 48 ++++++++++++++++++++--
>  1 file changed, 44 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/input/touchscreen/himax_hx83112b.c b/drivers/input/touchscreen/himax_hx83112b.c
> index 4f6609dcdef3..0a797789e548 100644
> --- a/drivers/input/touchscreen/himax_hx83112b.c
> +++ b/drivers/input/touchscreen/himax_hx83112b.c
> @@ -19,10 +19,12 @@
>  #include <linux/interrupt.h>
>  #include <linux/kernel.h>
>  #include <linux/regmap.h>
> +#include <linux/regulator/consumer.h>
>  
>  #define HIMAX_ID_83112B			0x83112b
>  
>  #define HIMAX_MAX_POINTS		10
> +#define HIMAX_MAX_SUPPLIES		2
>  
>  #define HIMAX_REG_CFG_SET_ADDR		0x00
>  #define HIMAX_REG_CFG_INIT_READ		0x0c
> @@ -50,6 +52,7 @@ struct himax_event {
>  static_assert(sizeof(struct himax_event) == 56);
>  
>  struct himax_ts_data {
> +	struct regulator_bulk_data supplies[HIMAX_MAX_SUPPLIES];
>  	struct gpio_desc *gpiod_rst;
>  	struct input_dev *input_dev;
>  	struct i2c_client *client;
> @@ -63,6 +66,11 @@ static const struct regmap_config himax_regmap_config = {
>  	.val_format_endian = REGMAP_ENDIAN_LITTLE,
>  };
>  
> +static const char *const himax_supply_names[] = {
> +	"avdd",
> +	"vdd",
> +};
> +

That's confusing. Binding said only HX83100A family has regulators, but
you request for everyone.

>  static int himax_read_config(struct himax_ts_data *ts, u32 address, u32 *dst)
>  {
>  	int error;
> @@ -267,7 +275,7 @@ static irqreturn_t himax_irq_handler(int irq, void *dev_id)
>  
>  static int himax_probe(struct i2c_client *client)
>  {
> -	int error;
> +	int error, i;
>  	struct device *dev = &client->dev;
>  	struct himax_ts_data *ts;
>  
> @@ -290,11 +298,31 @@ static int himax_probe(struct i2c_client *client)
>  		return error;
>  	}
>  
> +	int num_supplies = ARRAY_SIZE(himax_supply_names);
> +
> +	for (i = 0; i < num_supplies; i++)
> +		ts->supplies[i].supply = himax_supply_names[i];
> +
> +	error = devm_regulator_bulk_get(dev,

devm_regulator_bulk_get_enable and drop rest of the code here.


> +					num_supplies,
> +					ts->supplies);

Wrap it properly at 80, not one argument in one line.

> +	if (error) {
> +		dev_err(dev, "Failed to get supplies: %d\n", error);

return dev_err_probe()

> +		return error;
> +	}
> +
> +	error = regulator_bulk_enable(num_supplies,
> +				      ts->supplies);
> +	if (error) {
> +		dev_err(dev, "Failed to enable supplies: %d\n", error);
> +		goto error_out;
> +	}
> +


Best regards,
Krzysztof

Re: [PATCH 2/6] input: himax_hx83112b: add regulator handling

[Felix Kaechele]

Thanks, Krzysztof! Really appreciate your input, as I'm currently not a 
regular contributor to the kernel.

On 2024-05-04 08:37, Krzysztof Kozlowski wrote:
> On 04/05/2024 04:04, Felix Kaechele wrote:
>> Handle regulators used on this chip family, namely AVDD and VDD. These
>> definitions are taken from the GPLv2 licensed vendor driver.
>>
>> Signed-off-by: Felix Kaechele <felix@kaechele.ca>
>> Link: https://github.com/HimaxSoftware/HX83112_Android_Driver
>> ---
>>   drivers/input/touchscreen/himax_hx83112b.c | 48 ++++++++++++++++++++--
>>   1 file changed, 44 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/input/touchscreen/himax_hx83112b.c b/drivers/input/touchscreen/himax_hx83112b.c
>> index 4f6609dcdef3..0a797789e548 100644
>> --- a/drivers/input/touchscreen/himax_hx83112b.c
>> +++ b/drivers/input/touchscreen/himax_hx83112b.c
>> @@ -19,10 +19,12 @@
>>   #include <linux/interrupt.h>
>>   #include <linux/kernel.h>
>>   #include <linux/regmap.h>
>> +#include <linux/regulator/consumer.h>
>>   
>>   #define HIMAX_ID_83112B			0x83112b
>>   
>>   #define HIMAX_MAX_POINTS		10
>> +#define HIMAX_MAX_SUPPLIES		2
>>   
>>   #define HIMAX_REG_CFG_SET_ADDR		0x00
>>   #define HIMAX_REG_CFG_INIT_READ		0x0c
>> @@ -50,6 +52,7 @@ struct himax_event {
>>   static_assert(sizeof(struct himax_event) == 56);
>>   
>>   struct himax_ts_data {
>> +	struct regulator_bulk_data supplies[HIMAX_MAX_SUPPLIES];
>>   	struct gpio_desc *gpiod_rst;
>>   	struct input_dev *input_dev;
>>   	struct i2c_client *client;
>> @@ -63,6 +66,11 @@ static const struct regmap_config himax_regmap_config = {
>>   	.val_format_endian = REGMAP_ENDIAN_LITTLE,
>>   };
>>   
>> +static const char *const himax_supply_names[] = {
>> +	"avdd",
>> +	"vdd",
>> +};
>> +
> 
> That's confusing. Binding said only HX83100A family has regulators, but
> you request for everyone.
> 

You're right. Looking at the vendor driver for each of models I could 
see that it defined AVDD and VDD in both drivers. So I thought it could 
make sense to offer it for the entire chip family, with which I meant 
all HX831xx in this case.

But it seems after some more testing (and with this touch IC family 
generally being a part of the display controller) the regulators are 
sufficiently handled by the panel driver / bootloader for the use case 
of having the touchscreen on when the display is on.
It wouldn't allow for waking the screen by using the touchscreen, and 
while I'd have to go back to the original OS on the device to verify 
this again, I don't remember that working on the original OS either.

So I'm thinking I may drop the whole regulator part of the patchset to 
keep it smaller.

>>   static int himax_read_config(struct himax_ts_data *ts, u32 address, u32 *dst)
>>   {
>>   	int error;
>> @@ -267,7 +275,7 @@ static irqreturn_t himax_irq_handler(int irq, void *dev_id)
>>   
>>   static int himax_probe(struct i2c_client *client)
>>   {
>> -	int error;
>> +	int error, i;
>>   	struct device *dev = &client->dev;
>>   	struct himax_ts_data *ts;
>>   
>> @@ -290,11 +298,31 @@ static int himax_probe(struct i2c_client *client)
>>   		return error;
>>   	}
>>   
>> +	int num_supplies = ARRAY_SIZE(himax_supply_names);
>> +
>> +	for (i = 0; i < num_supplies; i++)
>> +		ts->supplies[i].supply = himax_supply_names[i];
>> +
>> +	error = devm_regulator_bulk_get(dev,
> 
> devm_regulator_bulk_get_enable and drop rest of the code here.
> 

That's pretty neat. If I do decide to bring in regulator handling this 
removes quite a bit of boilerplate.

> 
>> +					num_supplies,
>> +					ts->supplies);
> 
> Wrap it properly at 80, not one argument in one line.
> 
>> +	if (error) {
>> +		dev_err(dev, "Failed to get supplies: %d\n", error);
> 
> return dev_err_probe()
> 

Same here. Thanks for the hint.

>> +		return error;
>> +	}
>> +
>> +	error = regulator_bulk_enable(num_supplies,
>> +				      ts->supplies);
>> +	if (error) {
>> +		dev_err(dev, "Failed to enable supplies: %d\n", error);
>> +		goto error_out;
>> +	}
>> +

I'll be sending a v2 shortly.

Thanks again,
Felix

Re: [PATCH 1/6] dt-bindings: input: touchscreen: himax,hx83112b: add HX83100A

[Rob Herring]

On Sat, May 04, 2024 at 02:34:08PM +0200, Krzysztof Kozlowski wrote:
> On 04/05/2024 04:04, Felix Kaechele wrote:
> > This adds a compatible string for the Himax HX83100A touch controller
> 
> Please do not use "This commit/patch/change", but imperative mood. See
> longer explanation here:
> https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-patches.rst#L95
> 
> > including the AVDD and VDD supply nodes used by this chip family.
> > 
> > Signed-off-by: Felix Kaechele <felix@kaechele.ca>
> > ---
> >  .../bindings/input/touchscreen/himax,hx83112b.yaml       | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/input/touchscreen/himax,hx83112b.yaml b/Documentation/devicetree/bindings/input/touchscreen/himax,hx83112b.yaml
> > index f42b23d532eb..5809afedb9a2 100644
> > --- a/Documentation/devicetree/bindings/input/touchscreen/himax,hx83112b.yaml
> > +++ b/Documentation/devicetree/bindings/input/touchscreen/himax,hx83112b.yaml
> > @@ -15,6 +15,7 @@ allOf:
> >  properties:
> >    compatible:
> >      enum:
> > +      - himax,hx83100a
> >        - himax,hx83112b
> >  
> >    reg:
> > @@ -26,6 +27,12 @@ properties:
> >    reset-gpios:
> >      maxItems: 1
> >  
> > +  avdd-supply:
> > +    description: Analog power supply regulator
> > +
> > +  vdd-supply:
> > +    description: Digital power supply regulator
> 
> These should not be allowed for other variant, so either you need
> allOf:if:then disallowing them (: false) or just create another binding
> file.

Or the commit message needs some explanation that the supplies also 
apply to the 83112b as the existing binding has no supplies. 

Rob