Resent, due to being caught in the spam filter.
This set of patches brings support for the Himax HX83100A touch
controller.
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
Changes in v2:
- removed regulator handling, my test device works fine without it
- some minor formatting fixes
Felix Kaechele (5):
dt-bindings: input: touchscreen: himax,hx83112b: add HX83100A
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 | 1 +
drivers/input/touchscreen/himax_hx83112b.c | 135 ++++++++++++++----
2 files changed, 110 insertions(+), 26 deletions(-)
base-commit: 5128de84d8fc849400d00f7a6982711f129699ea
--
2.45.0
Add a compatible string for the Himax HX83100A touch controller.
Signed-off-by: Felix Kaechele <[email protected]>
---
.../devicetree/bindings/input/touchscreen/himax,hx83112b.yaml | 1 +
1 file changed, 1 insertion(+)
diff --git a/Documentation/devicetree/bindings/input/touchscreen/himax,hx83112b.yaml b/Documentation/devicetree/bindings/input/touchscreen/himax,hx83112b.yaml
index f42b23d532eb..f5cfacb5e966 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:
--
2.45.0
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 4f6609dcdef3..2da2920d43f9 100644
--- a/drivers/input/touchscreen/himax_hx83112b.c
+++ b/drivers/input/touchscreen/himax_hx83112b.c
@@ -24,12 +24,14 @@
#define HIMAX_MAX_POINTS 10
-#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
@@ -67,15 +69,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;
@@ -101,7 +104,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;
@@ -235,7 +238,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.45.0
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 | 50 ++++++++++++++++++++--
1 file changed, 47 insertions(+), 3 deletions(-)
diff --git a/drivers/input/touchscreen/himax_hx83112b.c b/drivers/input/touchscreen/himax_hx83112b.c
index 2da2920d43f9..67ef3255cc8b 100644
--- a/drivers/input/touchscreen/himax_hx83112b.c
+++ b/drivers/input/touchscreen/himax_hx83112b.c
@@ -27,9 +27,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
@@ -65,10 +69,34 @@ static const struct regmap_config himax_regmap_config = {
.val_format_endian = REGMAP_ENDIAN_LITTLE,
};
-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;
@@ -78,7 +106,23 @@ 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;
@@ -104,7 +148,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.45.0
In preparation for HX83100A support allow defining separate functions
for specific chip operations.
Signed-off-by: Felix Kaechele <[email protected]>
---
drivers/input/touchscreen/himax_hx83112b.c | 51 +++++++++++++++-------
1 file changed, 35 insertions(+), 16 deletions(-)
diff --git a/drivers/input/touchscreen/himax_hx83112b.c b/drivers/input/touchscreen/himax_hx83112b.c
index 67ef3255cc8b..5092a357c332 100644
--- a/drivers/input/touchscreen/himax_hx83112b.c
+++ b/drivers/input/touchscreen/himax_hx83112b.c
@@ -20,8 +20,6 @@
#include <linux/kernel.h>
#include <linux/regmap.h>
-#define HIMAX_ID_83112B 0x83112b
-
#define HIMAX_MAX_POINTS 10
#define HIMAX_AHB_ADDR_BYTE_0 0x00
@@ -55,7 +53,16 @@ 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 gpio_desc *gpiod_rst;
struct input_dev *input_dev;
struct i2c_client *client;
@@ -167,15 +174,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)
@@ -277,13 +281,19 @@ 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);
@@ -329,6 +339,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);
@@ -346,9 +357,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)
@@ -381,15 +394,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.45.0
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 <[email protected]>
Tested-by: Paul Gale <[email protected]>
---
drivers/input/touchscreen/himax_hx83112b.c | 17 +++++++++++++++++
1 file changed, 17 insertions(+)
diff --git a/drivers/input/touchscreen/himax_hx83112b.c b/drivers/input/touchscreen/himax_hx83112b.c
index 5092a357c332..9ed3bccde4ac 100644
--- a/drivers/input/touchscreen/himax_hx83112b.c
+++ b/drivers/input/touchscreen/himax_hx83112b.c
@@ -4,6 +4,9 @@
*
* Copyright (C) 2022 Job Noorman <[email protected]>
*
+ * HX83100A support
+ * Copyright (C) 2024 Felix Kaechele <[email protected]>
+ *
* This code is based on "Himax Android Driver Sample Code for QCT platform":
*
* Copyright (C) 2017 Himax Corporation.
@@ -35,6 +38,8 @@
#define HIMAX_REG_ADDR_ICID 0x900000d0
+#define HX83100A_REG_FW_EVENT_STACK 0x90060000
+
#define HIMAX_INVALID_COORD 0xffff
struct himax_event_point {
@@ -288,6 +293,12 @@ static int himax_read_events(struct himax_ts_data *ts,
length);
}
+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_handle_input(struct himax_ts_data *ts)
{
int error;
@@ -394,6 +405,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,
@@ -401,6 +416,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 */ }
};
@@ -408,6 +424,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.45.0
On Sat, May 11, 2024 at 08:12:22AM -0400, Felix Kaechele wrote:
> Add a compatible string for the Himax HX83100A touch controller.
>
> Signed-off-by: Felix Kaechele <[email protected]>
Commit message should mention what makes this device incompatible with
the existing device.
Thanks,
Conor.
> ---
> .../devicetree/bindings/input/touchscreen/himax,hx83112b.yaml | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/Documentation/devicetree/bindings/input/touchscreen/himax,hx83112b.yaml b/Documentation/devicetree/bindings/input/touchscreen/himax,hx83112b.yaml
> index f42b23d532eb..f5cfacb5e966 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:
> --
> 2.45.0
>
On 2024-05-11 08:38, Conor Dooley wrote:
> On Sat, May 11, 2024 at 08:12:22AM -0400, Felix Kaechele wrote:
>> Add a compatible string for the Himax HX83100A touch controller.
>>
>> Signed-off-by: Felix Kaechele <[email protected]>
>
> Commit message should mention what makes this device incompatible with
> the existing device.
Thanks!
I have added this note in the commit message which will be part of a v3,
coming after I hopefully receive a few more comments on the other parts
of this change:
The HX83100A presents touch events on its internal bus rather than
offering a dedicated event register like the other chips in this family do.
Felix
On Sat, May 11, 2024 at 10:10:08AM -0400, Felix Kaechele wrote:
> On 2024-05-11 08:38, Conor Dooley wrote:
> > On Sat, May 11, 2024 at 08:12:22AM -0400, Felix Kaechele wrote:
> > > Add a compatible string for the Himax HX83100A touch controller.
> > >
> > > Signed-off-by: Felix Kaechele <[email protected]>
> >
> > Commit message should mention what makes this device incompatible with
> > the existing device.
>
> Thanks!
>
> I have added this note in the commit message which will be part of a v3,
> coming after I hopefully receive a few more comments on the other parts of
> this change:
>
> The HX83100A presents touch events on its internal bus rather than offering
> a dedicated event register like the other chips in this family do.
Ye, that sounds good. W/ that,
Acked-by: Conor Dooley <[email protected]>
Cheers,
Conor.
On Sat, May 11, 2024 at 08:12:24AM -0400, Felix Kaechele wrote:
> 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.
Mark, do we have anything in regmap to support this better or having a
wrapper is the best solution here?
Thanks!
>
> Signed-off-by: Felix Kaechele <[email protected]>
> ---
> drivers/input/touchscreen/himax_hx83112b.c | 50 ++++++++++++++++++++--
> 1 file changed, 47 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/input/touchscreen/himax_hx83112b.c b/drivers/input/touchscreen/himax_hx83112b.c
> index 2da2920d43f9..67ef3255cc8b 100644
> --- a/drivers/input/touchscreen/himax_hx83112b.c
> +++ b/drivers/input/touchscreen/himax_hx83112b.c
> @@ -27,9 +27,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
>
> @@ -65,10 +69,34 @@ static const struct regmap_config himax_regmap_config = {
> .val_format_endian = REGMAP_ENDIAN_LITTLE,
> };
>
> -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;
> @@ -78,7 +106,23 @@ 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;
>
> @@ -104,7 +148,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.45.0
>
--
Dmitry
On Mon, May 13, 2024 at 04:01:59PM -0700, Dmitry Torokhov wrote:
> On Sat, May 11, 2024 at 08:12:24AM -0400, Felix Kaechele wrote:
> > 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.
> Mark, do we have anything in regmap to support this better or having a
> wrapper is the best solution here?
No, I've not seen something that explicitly requires toggling a burst
mode on and off to do a bulk operation. Off the top of my head I'd
suggest just always leaving the burst mode enabled but I assume there's
some downside to doing that. We could add something but I'm not sure if
it's worth it without having seen any other devices with the same need.
On 2024-05-14 05:46, Mark Brown wrote:
> On Mon, May 13, 2024 at 04:01:59PM -0700, Dmitry Torokhov wrote:
>> On Sat, May 11, 2024 at 08:12:24AM -0400, Felix Kaechele wrote:
>>> 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.
>
>> Mark, do we have anything in regmap to support this better or having a
>> wrapper is the best solution here?
>
> No, I've not seen something that explicitly requires toggling a burst
> mode on and off to do a bulk operation. Off the top of my head I'd
> suggest just always leaving the burst mode enabled but I assume there's
> some downside to doing that. We could add something but I'm not sure if
> it's worth it without having seen any other devices with the same need.
I can experiment some more with just leaving burst mode enabled.
Since the vendor driver invariably enables burst mode before any
transaction of more than 4 bytes I'll have to see if burst mode does
remain enabled under all circumstances of normal operation.
Since I don't have access to the datasheet I cannot know what the
intended behaviour and/or downsides are. Due to that I played it safe
and mimicked the behaviour of the vendor driver.
I'm guessing not having to enable burst mode on every touch event
interrupt could also mean an improvement in latency, since we save two
write cycles on the bus for each fetching of the event data.
Not sure how measurable that would be though.
I'm thinking to myself Himax at some point recognized this and that is
why we see a dedicated touch event read register on later models in this
chip family.
Regards,
Felix
On 2024-05-14 10:01, Felix Kaechele wrote:
> On 2024-05-14 05:46, Mark Brown wrote:
>> On Mon, May 13, 2024 at 04:01:59PM -0700, Dmitry Torokhov wrote:
>>> On Sat, May 11, 2024 at 08:12:24AM -0400, Felix Kaechele wrote:
>>>> 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.
>>
>>> Mark, do we have anything in regmap to support this better or having a
>>> wrapper is the best solution here?
>>
>> No, I've not seen something that explicitly requires toggling a burst
>> mode on and off to do a bulk operation. Off the top of my head I'd
>> suggest just always leaving the burst mode enabled but I assume there's
>> some downside to doing that. We could add something but I'm not sure if
>> it's worth it without having seen any other devices with the same need.
>
> I can experiment some more with just leaving burst mode enabled.
I've done some testing now and can confirm that, unfortunately, not
enabling burst mode for every read of the FIFO register results in
unreliable touchscreen operation.
For testing purposes I only enabled burst mode once at both probe and
resume.
The touchscreen will stop working both randomly in normal operation and
reproducibly after returning from screen blanking.
My wild guess is that DSI commands (e.g. for re-initializing the panel)
alter the state of the IC such that the burst mode on the I2C interface
ends up disabled again.
That means the bus read function from this current v2 series could be
used as-is.
I have a v3 in the pipeline to address the comments Conor made on
another patch in the series.
So far those are the only changes compared to this v2. If you have any
other ideas for what I could test in regards to this, please let me
know. Otherwise I'll be sending v3 in the next few days.
Regards,
Felix