* [PATCH v2 0/3] Add Himax hx83112b touchscreen driver @ 2022-10-12 20:23 Job Noorman 2022-10-12 20:24 ` [PATCH v2 1/3] dt-bindings: touchscreen: add Himax hx83112b bindings Job Noorman 2022-10-12 20:24 ` [PATCH v2 2/3] Input: add driver for Himax hx83112b touchscreen devices Job Noorman 0 siblings, 2 replies; 8+ messages in thread From: Job Noorman @ 2022-10-12 20:23 UTC (permalink / raw) To: Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Andy Gross, Bjorn Andersson, Konrad Dybcio, Henrik Rydberg Cc: Luca Weiss, linux-input, devicetree, linux-kernel, linux-arm-msm Hi all, This series adds support for the Himax hx83112b. The hx83112b supports 10 point multitouch with hardware tracking of touch points. It is the touchschreen used by the Fairphone 3. Note that a datasheet was unavailable for this device, so it was built based on the Android driver that was tagged as GPLv2. This series is a complete rewrite, though, and the code bears no resemblence to the original implementation. It is expected that this driver can be made to work on other hx83xxx devices, especially the hx83112a used in the Fairphone 4. However, since we have been unable to verify this, this driver only declares compatibility with the hx83112b and uses very specific file names. Changes since v1: - Fix sparse warnings. Reported-by: kernel test robot <lkp@intel.com>. - Fix dt_binding_check. Best regards, Job Previous versions: - v1: https://lore.kernel.org/lkml/20221011190729.14747-1-job@noorman.info/ Job Noorman (3): dt-bindings: touchscreen: add Himax hx83112b bindings Input: add driver for Himax hx83112b touchscreen devices arm64: dts: qcom: sdm632: fairphone-fp3: add touchscreen .../input/touchscreen/himax,hx83112b.yaml | 61 +++ MAINTAINERS | 7 + .../boot/dts/qcom/sdm632-fairphone-fp3.dts | 14 + drivers/input/touchscreen/Kconfig | 11 + drivers/input/touchscreen/Makefile | 1 + drivers/input/touchscreen/himax_hx83112b.c | 377 ++++++++++++++++++ 6 files changed, 471 insertions(+) create mode 100644 Documentation/devicetree/bindings/input/touchscreen/himax,hx83112b.yaml create mode 100644 drivers/input/touchscreen/himax_hx83112b.c base-commit: d4a596eddb90114f5f5f32a440057a175517b090 -- 2.38.0 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v2 1/3] dt-bindings: touchscreen: add Himax hx83112b bindings 2022-10-12 20:23 [PATCH v2 0/3] Add Himax hx83112b touchscreen driver Job Noorman @ 2022-10-12 20:24 ` Job Noorman 2022-10-13 20:25 ` Rob Herring 2022-10-12 20:24 ` [PATCH v2 2/3] Input: add driver for Himax hx83112b touchscreen devices Job Noorman 1 sibling, 1 reply; 8+ messages in thread From: Job Noorman @ 2022-10-12 20:24 UTC (permalink / raw) To: Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski Cc: Luca Weiss, linux-input, devicetree, linux-kernel This patch adds device tree bindings for Himax 83112b touchscreen devices. Signed-off-by: Job Noorman <job@noorman.info> --- .../input/touchscreen/himax,hx83112b.yaml | 61 +++++++++++++++++++ MAINTAINERS | 6 ++ 2 files changed, 67 insertions(+) create mode 100644 Documentation/devicetree/bindings/input/touchscreen/himax,hx83112b.yaml diff --git a/Documentation/devicetree/bindings/input/touchscreen/himax,hx83112b.yaml b/Documentation/devicetree/bindings/input/touchscreen/himax,hx83112b.yaml new file mode 100644 index 000000000000..c10c82415401 --- /dev/null +++ b/Documentation/devicetree/bindings/input/touchscreen/himax,hx83112b.yaml @@ -0,0 +1,61 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/input/touchscreen/himax,hx83112b.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Himax hx83112b touchscreen controller bindings + +maintainers: + - Job Noorman <job@noorman.info> + +allOf: + - $ref: touchscreen.yaml# + +properties: + compatible: + enum: + - himax,hx83112b + + reg: + maxItems: 1 + + interrupts: + maxItems: 1 + + reset-gpios: + maxItems: 1 + + touchscreen-inverted-x: true + touchscreen-inverted-y: true + touchscreen-size-x: true + touchscreen-size-y: true + touchscreen-swapped-x-y: true + +additionalProperties: false + +required: + - compatible + - reg + - interrupts + - reset-gpios + +examples: + - | + #include <dt-bindings/interrupt-controller/irq.h> + #include <dt-bindings/gpio/gpio.h> + i2c { + #address-cells = <1>; + #size-cells = <0>; + touchscreen@48 { + compatible = "himax,hx83112b"; + reg = <0x48>; + interrupt-parent = <&tlmm>; + interrupts = <65 IRQ_TYPE_LEVEL_LOW>; + touchscreen-size-x = <1080>; + touchscreen-size-y = <2160>; + reset-gpios = <&tlmm 64 GPIO_ACTIVE_LOW>; + }; + }; + +... diff --git a/MAINTAINERS b/MAINTAINERS index 9ddcc242081c..2418bffe9187 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -9094,6 +9094,12 @@ W: http://www.highpoint-tech.com F: Documentation/scsi/hptiop.rst F: drivers/scsi/hptiop.c +HIMAX HX83112B TOUCHSCREEN SUPPORT +M: Job Noorman <job@noorman.info> +L: linux-input@vger.kernel.org +S: Maintained +F: Documentation/devicetree/bindings/input/touchscreen/himax,hx83112b.yaml + HIPPI M: Jes Sorensen <jes@trained-monkey.org> L: linux-hippi@sunsite.dk -- 2.38.0 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v2 1/3] dt-bindings: touchscreen: add Himax hx83112b bindings 2022-10-12 20:24 ` [PATCH v2 1/3] dt-bindings: touchscreen: add Himax hx83112b bindings Job Noorman @ 2022-10-13 20:25 ` Rob Herring 2022-10-14 7:24 ` Job Noorman 0 siblings, 1 reply; 8+ messages in thread From: Rob Herring @ 2022-10-13 20:25 UTC (permalink / raw) To: Job Noorman Cc: Krzysztof Kozlowski, linux-input, linux-kernel, Dmitry Torokhov, devicetree, Rob Herring, Luca Weiss On Wed, 12 Oct 2022 20:24:06 +0000, Job Noorman wrote: > This patch adds device tree bindings for Himax 83112b touchscreen > devices. > > Signed-off-by: Job Noorman <job@noorman.info> > --- > .../input/touchscreen/himax,hx83112b.yaml | 61 +++++++++++++++++++ > MAINTAINERS | 6 ++ > 2 files changed, 67 insertions(+) > create mode 100644 Documentation/devicetree/bindings/input/touchscreen/himax,hx83112b.yaml > Reviewed-by: Rob Herring <robh@kernel.org> ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 1/3] dt-bindings: touchscreen: add Himax hx83112b bindings 2022-10-13 20:25 ` Rob Herring @ 2022-10-14 7:24 ` Job Noorman 0 siblings, 0 replies; 8+ messages in thread From: Job Noorman @ 2022-10-14 7:24 UTC (permalink / raw) To: Rob Herring Cc: Krzysztof Kozlowski, linux-input, linux-kernel, Dmitry Torokhov, devicetree, Rob Herring, Luca Weiss Hi Rob, Thanks for you review! On Thu Oct 13, 2022 at 10:25 PM CEST, Rob Herring wrote: > On Wed, 12 Oct 2022 20:24:06 +0000, Job Noorman wrote: > > This patch adds device tree bindings for Himax 83112b touchscreen > > devices. > > > > Signed-off-by: Job Noorman <job@noorman.info> > > --- > > .../input/touchscreen/himax,hx83112b.yaml | 61 +++++++++++++++++++ > > MAINTAINERS | 6 ++ > > 2 files changed, 67 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/input/touchscreen/himax,hx83112b.yaml > > > > Reviewed-by: Rob Herring <robh@kernel.org> The next version of this patch will make the properties touchscreen-size-{x,y} required. Can I still attach your "Reviewed-by" tag to this updated patch or would you like to have a look at it first? Kind regards, Job ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v2 2/3] Input: add driver for Himax hx83112b touchscreen devices 2022-10-12 20:23 [PATCH v2 0/3] Add Himax hx83112b touchscreen driver Job Noorman 2022-10-12 20:24 ` [PATCH v2 1/3] dt-bindings: touchscreen: add Himax hx83112b bindings Job Noorman @ 2022-10-12 20:24 ` Job Noorman 2022-10-13 2:34 ` Jeff LaBundy 1 sibling, 1 reply; 8+ messages in thread From: Job Noorman @ 2022-10-12 20:24 UTC (permalink / raw) To: Dmitry Torokhov, Henrik Rydberg; +Cc: Luca Weiss, linux-kernel, linux-input This patch adds support for Himax hx83112b touchscreen devices. As there are no publicly available data sheets for these devices, the implementation is based on the driver of the downstream Android kernel used in the Fairphone 3. This patch is a complete rewrite, though, and the code bears no resemblence to the original implementation. The driver has been tested on the aforementioned phone. Signed-off-by: Job Noorman <job@noorman.info> --- MAINTAINERS | 1 + drivers/input/touchscreen/Kconfig | 11 + drivers/input/touchscreen/Makefile | 1 + drivers/input/touchscreen/himax_hx83112b.c | 377 +++++++++++++++++++++ 4 files changed, 390 insertions(+) create mode 100644 drivers/input/touchscreen/himax_hx83112b.c diff --git a/MAINTAINERS b/MAINTAINERS index 2418bffe9187..51a03f9586f1 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -9099,6 +9099,7 @@ M: Job Noorman <job@noorman.info> L: linux-input@vger.kernel.org S: Maintained F: Documentation/devicetree/bindings/input/touchscreen/himax,hx83112b.yaml +F: drivers/input/touchscreen/himax_hx83112b.c HIPPI M: Jes Sorensen <jes@trained-monkey.org> diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig index dc90a3ea51ee..fd710fd8cd53 100644 --- a/drivers/input/touchscreen/Kconfig +++ b/drivers/input/touchscreen/Kconfig @@ -1379,4 +1379,15 @@ config TOUCHSCREEN_ZINITIX To compile this driver as a module, choose M here: the module will be called zinitix. +config TOUCHSCREEN_HIMAX_HX83112B + tristate "Himax hx83112b touchscreen driver" + depends on I2C + help + Say Y here to enable support for Himax hx83112b touchscreens. + + If unsure, say N. + + To compile this driver as a module, choose M here: the + module will be called himax_hx83112b. + endif diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile index 557f84fd2075..0f8bf79e01fe 100644 --- a/drivers/input/touchscreen/Makefile +++ b/drivers/input/touchscreen/Makefile @@ -116,3 +116,4 @@ obj-$(CONFIG_TOUCHSCREEN_ROHM_BU21023) += rohm_bu21023.o obj-$(CONFIG_TOUCHSCREEN_RASPBERRYPI_FW) += raspberrypi-ts.o obj-$(CONFIG_TOUCHSCREEN_IQS5XX) += iqs5xx.o obj-$(CONFIG_TOUCHSCREEN_ZINITIX) += zinitix.o +obj-$(CONFIG_TOUCHSCREEN_HIMAX_HX83112B) += himax_hx83112b.o diff --git a/drivers/input/touchscreen/himax_hx83112b.c b/drivers/input/touchscreen/himax_hx83112b.c new file mode 100644 index 000000000000..a7afea795dd5 --- /dev/null +++ b/drivers/input/touchscreen/himax_hx83112b.c @@ -0,0 +1,377 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * Driver for Himax hx83112b touchscreens + * + * Copyright (C) 2022 Job Noorman <job@noorman.info> + * + * This code is based on "Himax Android Driver Sample Code for QCT platform": + * + * Copyright (C) 2017 Himax Corporation. + */ + +#include <asm/byteorder.h> +#include <linux/delay.h> +#include <linux/err.h> +#include <linux/gpio/consumer.h> +#include <linux/i2c.h> +#include <linux/input.h> +#include <linux/input/mt.h> +#include <linux/input/touchscreen.h> +#include <linux/interrupt.h> +#include <linux/kernel.h> +#include <linux/regmap.h> + +#define HIMAX_ID_83112B 0x83112b + +#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_CFG_PRODUCT_ID 0x900000d0 + +struct himax_event_point { + __be16 x; + __be16 y; +} __packed; + +struct himax_event { + struct himax_event_point points[HIMAX_MAX_POINTS]; + u8 majors[HIMAX_MAX_POINTS]; + u8 pad0[2]; + u8 num_points; + u8 pad1[2]; + u8 checksum_fix; +} __packed; + +static_assert(sizeof(struct himax_event) == 56); + +struct himax_ts_data { + struct gpio_desc *gpiod_rst; + struct input_dev *input_dev; + struct i2c_client *client; + struct regmap *regmap; + struct touchscreen_properties props; +}; + +static const struct regmap_config himax_regmap_config = { + .reg_bits = 8, + .val_bits = 32, + .val_format_endian = REGMAP_ENDIAN_LITTLE, +}; + +static int himax_read_config(struct himax_ts_data *ts, u32 address, u32 *dst) +{ + int error = 0; + + error = regmap_write(ts->regmap, HIMAX_REG_CFG_SET_ADDR, address); + if (error) + return error; + + error = regmap_write(ts->regmap, HIMAX_REG_CFG_INIT_READ, 0x0); + if (error) + return error; + + return regmap_read(ts->regmap, HIMAX_REG_CFG_READ_VALUE, dst); +} + +static int himax_read_input_event(struct himax_ts_data *ts, + struct himax_event *event) +{ + return regmap_raw_read(ts->regmap, HIMAX_REG_READ_EVENT, event, + sizeof(*event)); +} + +static void himax_reset(struct himax_ts_data *ts) +{ + gpiod_set_value(ts->gpiod_rst, 1); + msleep(20); + gpiod_set_value(ts->gpiod_rst, 0); +} + +static int himax_read_product_id(struct himax_ts_data *ts, u32 *product_id) +{ + int error = himax_read_config(ts, HIMAX_CFG_PRODUCT_ID, product_id); + + if (error) + return error; + + *product_id >>= 8; + return 0; +} + +static int himax_check_product_id(struct himax_ts_data *ts) +{ + int error; + u32 product_id; + + error = himax_read_product_id(ts, &product_id); + if (error) + return error; + + dev_dbg(&ts->client->dev, "Product id: %x\n", product_id); + + switch (product_id) { + case HIMAX_ID_83112B: + return 0; + + default: + return dev_err_probe(&ts->client->dev, -ENODEV, + "Unknown product id: %x\n", product_id); + } +} + +static int himax_setup_gpio(struct himax_ts_data *ts) +{ + ts->gpiod_rst = + devm_gpiod_get(&ts->client->dev, "reset", GPIOD_OUT_HIGH); + if (IS_ERR(ts->gpiod_rst)) { + return dev_err_probe(&ts->client->dev, PTR_ERR(ts->gpiod_rst), + "Failed to get reset GPIO\n"); + } + + return 0; +} + +static int himax_input_register(struct himax_ts_data *ts) +{ + int error; + + ts->input_dev = devm_input_allocate_device(&ts->client->dev); + if (!ts->input_dev) { + return dev_err_probe(&ts->client->dev, -ENOMEM, + "Failed to allocate input device\n"); + } + + ts->input_dev->name = "Himax Touchscreen"; + + input_set_capability(ts->input_dev, EV_ABS, ABS_MT_POSITION_X); + input_set_capability(ts->input_dev, EV_ABS, ABS_MT_POSITION_Y); + input_set_abs_params(ts->input_dev, ABS_MT_WIDTH_MAJOR, 0, 200, 0, 0); + input_set_abs_params(ts->input_dev, ABS_MT_TOUCH_MAJOR, 0, 200, 0, 0); + + touchscreen_parse_properties(ts->input_dev, true, &ts->props); + + error = input_mt_init_slots(ts->input_dev, HIMAX_MAX_POINTS, + INPUT_MT_DIRECT | INPUT_MT_DROP_UNUSED); + if (error) { + return dev_err_probe(&ts->client->dev, error, + "Failed to initialize MT slots"); + } + + error = input_register_device(ts->input_dev); + if (error) { + return dev_err_probe(&ts->client->dev, error, + "Failed to register input device"); + } + + return 0; +} + +static u8 himax_event_get_num_points(const struct himax_event *event) +{ + if (event->num_points == 0xff) + return 0; + else + return event->num_points & 0x0f; +} + +static u16 himax_event_point_get_x(const struct himax_event_point *point) +{ + return be16_to_cpu(point->x); +} + +static u16 himax_event_point_get_y(const struct himax_event_point *point) +{ + return be16_to_cpu(point->y); +} + +static bool himax_event_point_is_valid(const struct himax_event_point *point) +{ + return himax_event_point_get_x(point) != 0xffff && + himax_event_point_get_y(point) != 0xffff; +} + +static bool himax_process_event_point(struct himax_ts_data *ts, + const struct himax_event *event, + int point_index) +{ + const struct himax_event_point *point = &event->points[point_index]; + u16 x = himax_event_point_get_x(point); + u16 y = himax_event_point_get_y(point); + u8 w = event->majors[point_index]; + + if (!himax_event_point_is_valid(point)) + return false; + + input_mt_slot(ts->input_dev, point_index); + input_mt_report_slot_state(ts->input_dev, MT_TOOL_FINGER, true); + touchscreen_report_pos(ts->input_dev, &ts->props, x, y, true); + input_report_abs(ts->input_dev, ABS_MT_TOUCH_MAJOR, w); + input_report_abs(ts->input_dev, ABS_MT_WIDTH_MAJOR, w); + return true; +} + +static void himax_process_event(struct himax_ts_data *ts, + const struct himax_event *event) +{ + int i; + int num_points_left = himax_event_get_num_points(event); + + for (i = 0; i < HIMAX_MAX_POINTS && num_points_left > 0; i++) { + if (himax_process_event_point(ts, event, i)) + num_points_left--; + } + + input_mt_sync_frame(ts->input_dev); + input_sync(ts->input_dev); +} + +static bool himax_verify_checksum(struct himax_ts_data *ts, + const struct himax_event *event) +{ + u8 *data = (u8 *)event; + int i; + u16 checksum = 0; + + for (i = 0; i < sizeof(*event); i++) + checksum += data[i]; + + if ((checksum & 0x00ff) != 0) { + dev_err(&ts->client->dev, "Wrong event checksum: %04x\n", + checksum); + return false; + } + + return true; +} + +static void himax_handle_input(struct himax_ts_data *ts) +{ + int error; + struct himax_event event; + + error = himax_read_input_event(ts, &event); + if (error) { + dev_err(&ts->client->dev, "Failed to read input event: %d\n", + error); + return; + } + + if (!himax_verify_checksum(ts, &event)) + return; + + himax_process_event(ts, &event); +} + +static irqreturn_t himax_irq_handler(int irq, void *dev_id) +{ + struct himax_ts_data *ts = dev_id; + + himax_handle_input(ts); + return IRQ_HANDLED; +} + +static int himax_request_irq(struct himax_ts_data *ts) +{ + struct i2c_client *client = ts->client; + + return devm_request_threaded_irq(&client->dev, client->irq, NULL, + himax_irq_handler, IRQF_ONESHOT, + client->name, ts); +} + +static int himax_probe(struct i2c_client *client, + const struct i2c_device_id *id) +{ + int error; + struct device *dev = &client->dev; + struct himax_ts_data *ts; + + if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) { + return dev_err_probe(dev, -ENODEV, + "I2C check functionality failed\n"); + } + + ts = devm_kzalloc(dev, sizeof(*ts), GFP_KERNEL); + if (!ts) + return -ENOMEM; + + i2c_set_clientdata(client, ts); + ts->client = client; + + ts->regmap = devm_regmap_init_i2c(client, &himax_regmap_config); + if (IS_ERR(ts->regmap)) { + return dev_err_probe(&client->dev, PTR_ERR(ts->regmap), + "Failed to initialize regmap"); + } + + error = himax_setup_gpio(ts); + if (error) + return error; + + himax_reset(ts); + + error = himax_check_product_id(ts); + if (error) + return error; + + error = himax_input_register(ts); + if (error) + return error; + + error = himax_request_irq(ts); + if (error) + return error; + + return 0; +} + +static int himax_suspend(struct device *dev) +{ + struct himax_ts_data *ts = dev_get_drvdata(dev); + + disable_irq(ts->client->irq); + return 0; +} + +static int himax_resume(struct device *dev) +{ + struct himax_ts_data *ts = dev_get_drvdata(dev); + + enable_irq(ts->client->irq); + return 0; +} + +static SIMPLE_DEV_PM_OPS(himax_pm_ops, himax_suspend, himax_resume); + +static const struct i2c_device_id himax_ts_id[] = { + { "hx83112b", 0 }, + { /* sentinel */ }, +}; +MODULE_DEVICE_TABLE(i2c, himax_ts_id); + +#ifdef CONFIG_OF +static const struct of_device_id himax_of_match[] = { + { .compatible = "himax,hx83112b" }, + { /* sentinel */ }, +}; +MODULE_DEVICE_TABLE(of, himax_of_match); +#endif + +static struct i2c_driver himax_ts_driver = { + .probe = himax_probe, + .id_table = himax_ts_id, + .driver = { + .name = "Himax-hx83112b-TS", + .of_match_table = of_match_ptr(himax_of_match), + .pm = &himax_pm_ops, + }, +}; +module_i2c_driver(himax_ts_driver); + +MODULE_AUTHOR("Job Noorman <job@noorman.info>"); +MODULE_DESCRIPTION("Himax hx83112b touchscreen driver"); +MODULE_LICENSE("GPL"); -- 2.38.0 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v2 2/3] Input: add driver for Himax hx83112b touchscreen devices 2022-10-12 20:24 ` [PATCH v2 2/3] Input: add driver for Himax hx83112b touchscreen devices Job Noorman @ 2022-10-13 2:34 ` Jeff LaBundy 2022-10-13 14:57 ` Job Noorman 0 siblings, 1 reply; 8+ messages in thread From: Jeff LaBundy @ 2022-10-13 2:34 UTC (permalink / raw) To: Job Noorman Cc: Dmitry Torokhov, Henrik Rydberg, Luca Weiss, linux-kernel, linux-input Hi Job, Nicely done, just a few comments throughout. On Wed, Oct 12, 2022 at 08:24:17PM +0000, Job Noorman wrote: > This patch adds support for Himax hx83112b touchscreen devices. As there > are no publicly available data sheets for these devices, the > implementation is based on the driver of the downstream Android kernel > used in the Fairphone 3. This patch is a complete rewrite, though, and > the code bears no resemblence to the original implementation. > > The driver has been tested on the aforementioned phone. > > Signed-off-by: Job Noorman <job@noorman.info> > --- > MAINTAINERS | 1 + > drivers/input/touchscreen/Kconfig | 11 + > drivers/input/touchscreen/Makefile | 1 + > drivers/input/touchscreen/himax_hx83112b.c | 377 +++++++++++++++++++++ > 4 files changed, 390 insertions(+) > create mode 100644 drivers/input/touchscreen/himax_hx83112b.c > > diff --git a/MAINTAINERS b/MAINTAINERS > index 2418bffe9187..51a03f9586f1 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -9099,6 +9099,7 @@ M: Job Noorman <job@noorman.info> > L: linux-input@vger.kernel.org > S: Maintained > F: Documentation/devicetree/bindings/input/touchscreen/himax,hx83112b.yaml > +F: drivers/input/touchscreen/himax_hx83112b.c > > HIPPI > M: Jes Sorensen <jes@trained-monkey.org> > diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig > index dc90a3ea51ee..fd710fd8cd53 100644 > --- a/drivers/input/touchscreen/Kconfig > +++ b/drivers/input/touchscreen/Kconfig > @@ -1379,4 +1379,15 @@ config TOUCHSCREEN_ZINITIX > To compile this driver as a module, choose M here: the > module will be called zinitix. > > +config TOUCHSCREEN_HIMAX_HX83112B > + tristate "Himax hx83112b touchscreen driver" > + depends on I2C Because regmap is used here, we must select REGMAP_I2C. > + help > + Say Y here to enable support for Himax hx83112b touchscreens. > + > + If unsure, say N. > + > + To compile this driver as a module, choose M here: the > + module will be called himax_hx83112b. > + > endif > diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile > index 557f84fd2075..0f8bf79e01fe 100644 > --- a/drivers/input/touchscreen/Makefile > +++ b/drivers/input/touchscreen/Makefile > @@ -116,3 +116,4 @@ obj-$(CONFIG_TOUCHSCREEN_ROHM_BU21023) += rohm_bu21023.o > obj-$(CONFIG_TOUCHSCREEN_RASPBERRYPI_FW) += raspberrypi-ts.o > obj-$(CONFIG_TOUCHSCREEN_IQS5XX) += iqs5xx.o > obj-$(CONFIG_TOUCHSCREEN_ZINITIX) += zinitix.o > +obj-$(CONFIG_TOUCHSCREEN_HIMAX_HX83112B) += himax_hx83112b.o > diff --git a/drivers/input/touchscreen/himax_hx83112b.c b/drivers/input/touchscreen/himax_hx83112b.c > new file mode 100644 > index 000000000000..a7afea795dd5 > --- /dev/null > +++ b/drivers/input/touchscreen/himax_hx83112b.c > @@ -0,0 +1,377 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * Driver for Himax hx83112b touchscreens > + * > + * Copyright (C) 2022 Job Noorman <job@noorman.info> > + * > + * This code is based on "Himax Android Driver Sample Code for QCT platform": > + * > + * Copyright (C) 2017 Himax Corporation. > + */ > + > +#include <asm/byteorder.h> This is already included by linux/kernel.h; no need to explicitly include it here. > +#include <linux/delay.h> > +#include <linux/err.h> > +#include <linux/gpio/consumer.h> > +#include <linux/i2c.h> > +#include <linux/input.h> > +#include <linux/input/mt.h> > +#include <linux/input/touchscreen.h> > +#include <linux/interrupt.h> > +#include <linux/kernel.h> > +#include <linux/regmap.h> > + > +#define HIMAX_ID_83112B 0x83112b > + > +#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_CFG_PRODUCT_ID 0x900000d0 Please align these values for better readability. > + > +struct himax_event_point { > + __be16 x; > + __be16 y; > +} __packed; > + > +struct himax_event { > + struct himax_event_point points[HIMAX_MAX_POINTS]; > + u8 majors[HIMAX_MAX_POINTS]; > + u8 pad0[2]; > + u8 num_points; > + u8 pad1[2]; > + u8 checksum_fix; > +} __packed; > + > +static_assert(sizeof(struct himax_event) == 56); > + > +struct himax_ts_data { > + struct gpio_desc *gpiod_rst; > + struct input_dev *input_dev; > + struct i2c_client *client; > + struct regmap *regmap; > + struct touchscreen_properties props; > +}; > + > +static const struct regmap_config himax_regmap_config = { > + .reg_bits = 8, > + .val_bits = 32, > + .val_format_endian = REGMAP_ENDIAN_LITTLE, > +}; > + > +static int himax_read_config(struct himax_ts_data *ts, u32 address, u32 *dst) > +{ > + int error = 0; > + > + error = regmap_write(ts->regmap, HIMAX_REG_CFG_SET_ADDR, address); > + if (error) > + return error; > + > + error = regmap_write(ts->regmap, HIMAX_REG_CFG_INIT_READ, 0x0); > + if (error) > + return error; > + > + return regmap_read(ts->regmap, HIMAX_REG_CFG_READ_VALUE, dst); > +} > + > +static int himax_read_input_event(struct himax_ts_data *ts, > + struct himax_event *event) > +{ > + return regmap_raw_read(ts->regmap, HIMAX_REG_READ_EVENT, event, > + sizeof(*event)); > +} > + > +static void himax_reset(struct himax_ts_data *ts) > +{ > + gpiod_set_value(ts->gpiod_rst, 1); > + msleep(20); > + gpiod_set_value(ts->gpiod_rst, 0); > +} > + > +static int himax_read_product_id(struct himax_ts_data *ts, u32 *product_id) > +{ > + int error = himax_read_config(ts, HIMAX_CFG_PRODUCT_ID, product_id); > + > + if (error) > + return error; > + > + *product_id >>= 8; > + return 0; > +} > + > +static int himax_check_product_id(struct himax_ts_data *ts) > +{ > + int error; > + u32 product_id; > + > + error = himax_read_product_id(ts, &product_id); > + if (error) > + return error; > + > + dev_dbg(&ts->client->dev, "Product id: %x\n", product_id); > + > + switch (product_id) { > + case HIMAX_ID_83112B: > + return 0; > + > + default: > + return dev_err_probe(&ts->client->dev, -ENODEV, > + "Unknown product id: %x\n", product_id); Aside from the existing arguments against dev_err_probe(), there is no point in using it here as the error is fixed and never equal to -EPROBE_DEFER. I know the documentation specifically offers this but doing so seems unnecessarily convoluted; just my $.02. Also note that -ENODEV is not really the appropriate error code to use here. It is not that there was no device at this address, rather the device did not return a valid product ID. Therefore I think that -EINVAL is more appropriate; again this is just my $.02. > + } > +} > + > +static int himax_setup_gpio(struct himax_ts_data *ts) > +{ > + ts->gpiod_rst = > + devm_gpiod_get(&ts->client->dev, "reset", GPIOD_OUT_HIGH); This line break is hard to read. This would be better: ts->gpiod_rst = devm_gpiod_get(..., ...); > + if (IS_ERR(ts->gpiod_rst)) { > + return dev_err_probe(&ts->client->dev, PTR_ERR(ts->gpiod_rst), > + "Failed to get reset GPIO\n"); > + } > + > + return 0; > +} > + > +static int himax_input_register(struct himax_ts_data *ts) > +{ > + int error; > + > + ts->input_dev = devm_input_allocate_device(&ts->client->dev); > + if (!ts->input_dev) { > + return dev_err_probe(&ts->client->dev, -ENOMEM, > + "Failed to allocate input device\n"); > + } Same here, this call to dev_err_probe() really just becomes dev_err() followed by a return. That being said, use of dev_err_probe() is generally frowned upon in the input subsystem and I tend to agree. The argument against it is that resources that may not be ready in time should be responsible for the housekeeping done in dev_err_probe() rather than every possible consumer doing so through every possible error path. I only mention this because you will likely be asked to change even the "valid" calls to dev_err_probe(). > + > + ts->input_dev->name = "Himax Touchscreen"; > + > + input_set_capability(ts->input_dev, EV_ABS, ABS_MT_POSITION_X); > + input_set_capability(ts->input_dev, EV_ABS, ABS_MT_POSITION_Y); > + input_set_abs_params(ts->input_dev, ABS_MT_WIDTH_MAJOR, 0, 200, 0, 0); > + input_set_abs_params(ts->input_dev, ABS_MT_TOUCH_MAJOR, 0, 200, 0, 0); > + > + touchscreen_parse_properties(ts->input_dev, true, &ts->props); Since the driver is not reading any default resolution values from the device, it seems you are expecting touchscreen-size-x/y to be provided as in your example. Unless I have misunderstood, please consider promoting these properties to 'required' in the binding. > + > + error = input_mt_init_slots(ts->input_dev, HIMAX_MAX_POINTS, > + INPUT_MT_DIRECT | INPUT_MT_DROP_UNUSED); > + if (error) { > + return dev_err_probe(&ts->client->dev, error, > + "Failed to initialize MT slots"); > + } > + > + error = input_register_device(ts->input_dev); > + if (error) { > + return dev_err_probe(&ts->client->dev, error, > + "Failed to register input device"); > + } > + > + return 0; > +} > + > +static u8 himax_event_get_num_points(const struct himax_event *event) > +{ > + if (event->num_points == 0xff) > + return 0; > + else > + return event->num_points & 0x0f; > +} > + > +static u16 himax_event_point_get_x(const struct himax_event_point *point) > +{ > + return be16_to_cpu(point->x); > +} > + > +static u16 himax_event_point_get_y(const struct himax_event_point *point) > +{ > + return be16_to_cpu(point->y); > +} These two functions seem to add an unnecessary layer and probably get inlined anyway; I think it is cleaner to simply call be16_to_cpu(...) directly. > + > +static bool himax_event_point_is_valid(const struct himax_event_point *point) > +{ > + return himax_event_point_get_x(point) != 0xffff && > + himax_event_point_get_y(point) != 0xffff; > +} How about U16_MAX? > + > +static bool himax_process_event_point(struct himax_ts_data *ts, > + const struct himax_event *event, > + int point_index) > +{ > + const struct himax_event_point *point = &event->points[point_index]; > + u16 x = himax_event_point_get_x(point); > + u16 y = himax_event_point_get_y(point); > + u8 w = event->majors[point_index]; > + > + if (!himax_event_point_is_valid(point)) > + return false; > + > + input_mt_slot(ts->input_dev, point_index); > + input_mt_report_slot_state(ts->input_dev, MT_TOOL_FINGER, true); > + touchscreen_report_pos(ts->input_dev, &ts->props, x, y, true); > + input_report_abs(ts->input_dev, ABS_MT_TOUCH_MAJOR, w); > + input_report_abs(ts->input_dev, ABS_MT_WIDTH_MAJOR, w); > + return true; > +} > + > +static void himax_process_event(struct himax_ts_data *ts, > + const struct himax_event *event) > +{ > + int i; > + int num_points_left = himax_event_get_num_points(event); > + > + for (i = 0; i < HIMAX_MAX_POINTS && num_points_left > 0; i++) { > + if (himax_process_event_point(ts, event, i)) > + num_points_left--; > + } > + > + input_mt_sync_frame(ts->input_dev); > + input_sync(ts->input_dev); > +} > + > +static bool himax_verify_checksum(struct himax_ts_data *ts, > + const struct himax_event *event) > +{ > + u8 *data = (u8 *)event; > + int i; > + u16 checksum = 0; > + > + for (i = 0; i < sizeof(*event); i++) > + checksum += data[i]; > + > + if ((checksum & 0x00ff) != 0) { > + dev_err(&ts->client->dev, "Wrong event checksum: %04x\n", > + checksum); > + return false; > + } > + > + return true; > +} > + > +static void himax_handle_input(struct himax_ts_data *ts) > +{ > + int error; > + struct himax_event event; > + > + error = himax_read_input_event(ts, &event); > + if (error) { > + dev_err(&ts->client->dev, "Failed to read input event: %d\n", > + error); > + return; > + } > + > + if (!himax_verify_checksum(ts, &event)) > + return; > + > + himax_process_event(ts, &event); > +} > + > +static irqreturn_t himax_irq_handler(int irq, void *dev_id) > +{ > + struct himax_ts_data *ts = dev_id; > + > + himax_handle_input(ts); Is it accurate to assume that the act of reading the event status register(s) is what acknowledges the interrupt and de-asserts the interrupt pin? If so, I think it is safer to define himax_handle_input() with an integer return type, then return IRQ_NONE upon failure. If the I2C adapter goes south such that reads are never initiated and the pin is stuck low causing an interrupt storm, the handler would get cut off quickly. Just for my own understanding, _when_ does the pin get de-asserted? Is it early in the I2C read, or after the stop condition? In case of the latter, consider a delay to prevent the interrupt from being immediately triggered once more after the handler has returned, but the pin hasn't quite returned to a logic-high level. > + return IRQ_HANDLED; > +} > + > +static int himax_request_irq(struct himax_ts_data *ts) > +{ > + struct i2c_client *client = ts->client; > + > + return devm_request_threaded_irq(&client->dev, client->irq, NULL, > + himax_irq_handler, IRQF_ONESHOT, > + client->name, ts); > +} This wrapper seems unnecessary. > + > +static int himax_probe(struct i2c_client *client, > + const struct i2c_device_id *id) > +{ > + int error; > + struct device *dev = &client->dev; > + struct himax_ts_data *ts; > + > + if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) { > + return dev_err_probe(dev, -ENODEV, > + "I2C check functionality failed\n"); > + } No need for curly braces around if () blocks that only have one expression. > + > + ts = devm_kzalloc(dev, sizeof(*ts), GFP_KERNEL); > + if (!ts) > + return -ENOMEM; > + > + i2c_set_clientdata(client, ts); > + ts->client = client; > + > + ts->regmap = devm_regmap_init_i2c(client, &himax_regmap_config); > + if (IS_ERR(ts->regmap)) { > + return dev_err_probe(&client->dev, PTR_ERR(ts->regmap), > + "Failed to initialize regmap"); > + } And here. > + > + error = himax_setup_gpio(ts); > + if (error) > + return error; > + > + himax_reset(ts); It looks like we're expecting an I2C read to occur directly after reset is de-asserted. Understanding that no datasheet is available to specify how much time the device takes to exit reset, does the downstream driver at least include some delay? > + > + error = himax_check_product_id(ts); > + if (error) > + return error; > + > + error = himax_input_register(ts); > + if (error) > + return error; > + > + error = himax_request_irq(ts); > + if (error) > + return error; > + > + return 0; Nit: I think it is cleaner to simply return himax_request_irq(ts) here, or actually just devm_request_threaded_irq(). > +} > + > +static int himax_suspend(struct device *dev) > +{ > + struct himax_ts_data *ts = dev_get_drvdata(dev); > + > + disable_irq(ts->client->irq); > + return 0; > +} > + > +static int himax_resume(struct device *dev) > +{ > + struct himax_ts_data *ts = dev_get_drvdata(dev); > + > + enable_irq(ts->client->irq); > + return 0; > +} > + > +static SIMPLE_DEV_PM_OPS(himax_pm_ops, himax_suspend, himax_resume); > + > +static const struct i2c_device_id himax_ts_id[] = { > + { "hx83112b", 0 }, > + { /* sentinel */ }, Nit: no need for a trailing comma after the sentinel because no patch would ever add a line below it. > +}; > +MODULE_DEVICE_TABLE(i2c, himax_ts_id); > + > +#ifdef CONFIG_OF > +static const struct of_device_id himax_of_match[] = { > + { .compatible = "himax,hx83112b" }, > + { /* sentinel */ }, And here. > +}; > +MODULE_DEVICE_TABLE(of, himax_of_match); > +#endif > + > +static struct i2c_driver himax_ts_driver = { > + .probe = himax_probe, > + .id_table = himax_ts_id, > + .driver = { > + .name = "Himax-hx83112b-TS", > + .of_match_table = of_match_ptr(himax_of_match), > + .pm = &himax_pm_ops, > + }, > +}; > +module_i2c_driver(himax_ts_driver); > + > +MODULE_AUTHOR("Job Noorman <job@noorman.info>"); > +MODULE_DESCRIPTION("Himax hx83112b touchscreen driver"); > +MODULE_LICENSE("GPL"); > -- > 2.38.0 > > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 2/3] Input: add driver for Himax hx83112b touchscreen devices 2022-10-13 2:34 ` Jeff LaBundy @ 2022-10-13 14:57 ` Job Noorman 2022-10-14 1:39 ` Jeff LaBundy 0 siblings, 1 reply; 8+ messages in thread From: Job Noorman @ 2022-10-13 14:57 UTC (permalink / raw) To: Jeff LaBundy Cc: Dmitry Torokhov, Henrik Rydberg, Luca Weiss, linux-kernel, linux-input Hi Jeff, Thanks for the extensive review! I've addressed most of your comments. I will reply below to the ones I have not yet addressed, or have questions about. You can consider the comments I don't reply to as addressed in the next version. On Thu Oct 13, 2022 at 4:34 AM CEST, Jeff LaBundy wrote: > That being said, use of dev_err_probe() is generally frowned upon in > the input subsystem and I tend to agree. The argument against it is > that resources that may not be ready in time should be responsible for > the housekeeping done in dev_err_probe() rather than every possible > consumer doing so through every possible error path. > > I only mention this because you will likely be asked to change even > the "valid" calls to dev_err_probe(). I have removed all uses of dev_err_probe(). I followed the approach I saw in other drivers of only calling dev_err() when the error is not -EPROBE_DEFER, and calling it unconditionally when this is not a possible error. However, while I _think_ I managed to figure out for which functions this can never be returned (based mostly on the Goodix driver), I wouldn't mind if somebody could double-check this :) > > +static bool himax_event_point_is_valid(const struct himax_event_point *point) > > +{ > > + return himax_event_point_get_x(point) != 0xffff && > > + himax_event_point_get_y(point) != 0xffff; > > +} > > How about U16_MAX? This feels strange to me because conceptually, I don't want to know if these values are equal to the maximum u16 value, I want to know if they are invalid coordinates. It's incidental that invalid values correspond to U16_MAX. I created a #define HIMAX_INVALID_COORD for this. Would you agree with this approach? > > +static irqreturn_t himax_irq_handler(int irq, void *dev_id) > > +{ > > + struct himax_ts_data *ts = dev_id; > > + > > + himax_handle_input(ts); > > Is it accurate to assume that the act of reading the event status > register(s) is what acknowledges the interrupt and de-asserts the > interrupt pin? I assume so but without datasheets this is impossible to know for sure unfortunately. However, since this is the only interaction with the device during an IRQ, I cannot imagine what else could cause the de-assert. > If so, I think it is safer to define himax_handle_input() with an > integer return type, then return IRQ_NONE upon failure. If the I2C > adapter goes south such that reads are never initiated and the pin > is stuck low causing an interrupt storm, the handler would get cut > off quickly. Done! Two questions: - Do you think it's necessary to return IRQ_NONE on a checksum failure? - If I understand correctly, this means that once an error occurs, the driver becomes unusable. Would it make sense to try to reset the device after an error? > Just for my own understanding, _when_ does the pin get de-asserted? > Is it early in the I2C read, or after the stop condition? In case > of the latter, consider a delay to prevent the interrupt from being > immediately triggered once more after the handler has returned, but > the pin hasn't quite returned to a logic-high level. I don't know the answer to this question unfortunately. The downstream driver doesn't seem to have any delay after reading the registers though. > > + error = himax_setup_gpio(ts); > > + if (error) > > + return error; > > + > > + himax_reset(ts); > > It looks like we're expecting an I2C read to occur directly after reset > is de-asserted. Understanding that no datasheet is available to specify > how much time the device takes to exit reset, does the downstream driver > at least include some delay? The downstream driver has a 20ms delay between asserting and de-asserting, but not _after_ de-asserting. I have copied this behavior. Here's the funny thing: the I2C read you're talking about (to read the product id) happens _before_ the reset in the downstream driver and that also seems to work... So I'm not sure what to do here. It does feel safer to add an additional delay so shall we just do that? Kind regards, Job ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 2/3] Input: add driver for Himax hx83112b touchscreen devices 2022-10-13 14:57 ` Job Noorman @ 2022-10-14 1:39 ` Jeff LaBundy 0 siblings, 0 replies; 8+ messages in thread From: Jeff LaBundy @ 2022-10-14 1:39 UTC (permalink / raw) To: Job Noorman Cc: Dmitry Torokhov, Henrik Rydberg, Luca Weiss, linux-kernel, linux-input Hi Job, On Thu, Oct 13, 2022 at 02:57:55PM +0000, Job Noorman wrote: > Hi Jeff, > > Thanks for the extensive review! > > I've addressed most of your comments. I will reply below to the ones I > have not yet addressed, or have questions about. You can consider the > comments I don't reply to as addressed in the next version. > > On Thu Oct 13, 2022 at 4:34 AM CEST, Jeff LaBundy wrote: > > That being said, use of dev_err_probe() is generally frowned upon in > > the input subsystem and I tend to agree. The argument against it is > > that resources that may not be ready in time should be responsible for > > the housekeeping done in dev_err_probe() rather than every possible > > consumer doing so through every possible error path. > > > > I only mention this because you will likely be asked to change even > > the "valid" calls to dev_err_probe(). > > I have removed all uses of dev_err_probe(). I followed the approach I > saw in other drivers of only calling dev_err() when the error is not > -EPROBE_DEFER, and calling it unconditionally when this is not a > possible error. However, while I _think_ I managed to figure out for > which functions this can never be returned (based mostly on the Goodix > driver), I wouldn't mind if somebody could double-check this :) > > > > +static bool himax_event_point_is_valid(const struct himax_event_point *point) > > > +{ > > > + return himax_event_point_get_x(point) != 0xffff && > > > + himax_event_point_get_y(point) != 0xffff; > > > +} > > > > How about U16_MAX? > > This feels strange to me because conceptually, I don't want to know if > these values are equal to the maximum u16 value, I want to know if they > are invalid coordinates. It's incidental that invalid values correspond > to U16_MAX. > > I created a #define HIMAX_INVALID_COORD for this. Would you agree with > this approach? That sounds fine too. > > > > +static irqreturn_t himax_irq_handler(int irq, void *dev_id) > > > +{ > > > + struct himax_ts_data *ts = dev_id; > > > + > > > + himax_handle_input(ts); > > > > Is it accurate to assume that the act of reading the event status > > register(s) is what acknowledges the interrupt and de-asserts the > > interrupt pin? > > I assume so but without datasheets this is impossible to know for sure > unfortunately. However, since this is the only interaction with the > device during an IRQ, I cannot imagine what else could cause the > de-assert. > > > If so, I think it is safer to define himax_handle_input() with an > > integer return type, then return IRQ_NONE upon failure. If the I2C > > adapter goes south such that reads are never initiated and the pin > > is stuck low causing an interrupt storm, the handler would get cut > > off quickly. > > Done! Two questions: > - Do you think it's necessary to return IRQ_NONE on a checksum failure? I personally do not think so. In this case the problem is not that the interrupt went unhandled, but rather the driver handled the interrupt but decided not to trust the data and post an input event. > - If I understand correctly, this means that once an error occurs, the > driver becomes unusable. Would it make sense to try to reset the > device after an error? That's up to you; it's not terribly uncommon to wrap low-level register access in some kind of retry loop if there is reason to believe the device may be unresponsive for some reason. One could issue a hardware reset after some number of failed attempts. Personally I think that is a bit overkill for an initial submission if the device operates perfectly fine on your platform. > > > Just for my own understanding, _when_ does the pin get de-asserted? > > Is it early in the I2C read, or after the stop condition? In case > > of the latter, consider a delay to prevent the interrupt from being > > immediately triggered once more after the handler has returned, but > > the pin hasn't quite returned to a logic-high level. > > I don't know the answer to this question unfortunately. The downstream > driver doesn't seem to have any delay after reading the registers > though. > > > > + error = himax_setup_gpio(ts); > > > + if (error) > > > + return error; > > > + > > > + himax_reset(ts); > > > > It looks like we're expecting an I2C read to occur directly after reset > > is de-asserted. Understanding that no datasheet is available to specify > > how much time the device takes to exit reset, does the downstream driver > > at least include some delay? > > The downstream driver has a 20ms delay between asserting and > de-asserting, but not _after_ de-asserting. I have copied this behavior. > > Here's the funny thing: the I2C read you're talking about (to read the > product id) happens _before_ the reset in the downstream driver and that > also seems to work... Interesting; I'm guessing that's because the reset GPIO may be pulled up such that the device has been out of reset for quite some time before the vendor driver reads the product ID. > > So I'm not sure what to do here. It does feel safer to add an additional > delay so shall we just do that? Since reset is asserted for a whole 20 ms, it seems relatively harmless to insert something like usleep_range(1000, 1100) after reset is released. It adds some margin and doesn't noticeably increase the probe time. At the very least, it would be very obvious where in the code one would increase the delay if required in the future. Consider adding a comment that the delay is a guess to prevent future developers from believing it came from a datasheet. > > Kind regards, > Job > Kind regards, Jeff LaBundy ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2022-10-14 7:24 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-10-12 20:23 [PATCH v2 0/3] Add Himax hx83112b touchscreen driver Job Noorman 2022-10-12 20:24 ` [PATCH v2 1/3] dt-bindings: touchscreen: add Himax hx83112b bindings Job Noorman 2022-10-13 20:25 ` Rob Herring 2022-10-14 7:24 ` Job Noorman 2022-10-12 20:24 ` [PATCH v2 2/3] Input: add driver for Himax hx83112b touchscreen devices Job Noorman 2022-10-13 2:34 ` Jeff LaBundy 2022-10-13 14:57 ` Job Noorman 2022-10-14 1:39 ` Jeff LaBundy
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).