| Message ID | 20240511121245.109644-4-felix@kaechele.ca (mailing list archive) |
|---|---|
| State | Mainlined |
| Commit | d731751dcfc135a9045431013d38de85db592f8f |
| Headers | show |
| Series | input: himax_hx83112b: add support for HX83100A | expand |
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 <felix@kaechele.ca> > --- > 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 >
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
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;
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 | 50 ++++++++++++++++++++-- 1 file changed, 47 insertions(+), 3 deletions(-)