2024-05-04 02:15:40

by Felix Kaechele

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

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



2024-05-04 02:15:42

by Felix Kaechele

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

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 <[email protected]>
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


2024-05-04 02:15:43

by Felix Kaechele

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

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 <[email protected]>
---
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


2024-05-04 02:15:52

by Felix Kaechele

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

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 <[email protected]>
---
.../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


2024-05-04 02:22:06

by Felix Kaechele

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

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 <[email protected]>
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


2024-05-04 12:34:24

by Krzysztof Kozlowski

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

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 <[email protected]>
> ---
> .../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


2024-05-04 12:37:44

by Krzysztof Kozlowski

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

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 <[email protected]>
> 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


2024-05-07 14:33:24

by Felix Kaechele

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

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 <[email protected]>
>> 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

2024-05-07 19:06:00

by Rob Herring (Arm)

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

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 <[email protected]>
> > ---
> > .../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