* [PATCH 0/2] input: Imagis: add support for the IST3032C touchscreen @ 2023-09-26 17:35 Karel Balej 2023-09-26 17:35 ` [PATCH 1/2] input: generalize the Imagis touchscreen driver Karel Balej ` (2 more replies) 0 siblings, 3 replies; 10+ messages in thread From: Karel Balej @ 2023-09-26 17:35 UTC (permalink / raw) To: Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Markuss Broks, linux-input, devicetree, linux-kernel, Duje Mihanović, ~postmarketos/upstreaming Cc: Karel Balej This patch series extends the Imagis driver to support the IST3032C touchscreen, which is used for instance with the samsung,coreprimevelte smartphone, with which this was tested. To use it with this model however, the regulator driver needs to be ported first. Support for this smartphone is not yet in-tree, upstreaming is ongoing at [1]. [1] https://lore.kernel.org/all/20230812-pxa1908-lkml-v5-0-a5d51937ee34@skole.hr/ Karel Balej (2): input: generalize the Imagis touchscreen driver input: Imagis: add support for the IST3032C touchscreen ...gis,ist3038c.yaml => imagis,ist30xxc.yaml} | 3 +- MAINTAINERS | 2 +- drivers/input/touchscreen/Kconfig | 4 +- drivers/input/touchscreen/imagis.c | 99 ++++++++++++------- 4 files changed, 66 insertions(+), 42 deletions(-) rename Documentation/devicetree/bindings/input/touchscreen/{imagis,ist3038c.yaml => imagis,ist30xxc.yaml} (97%) -- 2.42.0 ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 1/2] input: generalize the Imagis touchscreen driver 2023-09-26 17:35 [PATCH 0/2] input: Imagis: add support for the IST3032C touchscreen Karel Balej @ 2023-09-26 17:35 ` Karel Balej 2023-09-26 22:17 ` Conor Dooley 2023-09-27 1:48 ` Jeff LaBundy 2023-09-26 17:35 ` [PATCH 2/2] input: Imagis: add support for the IST3032C touchscreen Karel Balej 2023-09-26 22:16 ` [PATCH 0/2] " Conor Dooley 2 siblings, 2 replies; 10+ messages in thread From: Karel Balej @ 2023-09-26 17:35 UTC (permalink / raw) To: Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Markuss Broks, linux-input, devicetree, linux-kernel, Duje Mihanović, ~postmarketos/upstreaming Cc: Karel Balej This driver should work with other Imagis ICs of the IST30**C series. Make that more apparent. Co-developed-by: Duje Mihanović <duje.mihanovic@skole.hr> Signed-off-by: Duje Mihanović <duje.mihanovic@skole.hr> Signed-off-by: Karel Balej <balejk@matfyz.cz> --- ...gis,ist3038c.yaml => imagis,ist30xxc.yaml} | 2 +- MAINTAINERS | 2 +- drivers/input/touchscreen/Kconfig | 4 +- drivers/input/touchscreen/imagis.c | 86 +++++++++++-------- 4 files changed, 52 insertions(+), 42 deletions(-) rename Documentation/devicetree/bindings/input/touchscreen/{imagis,ist3038c.yaml => imagis,ist30xxc.yaml} (99%) diff --git a/Documentation/devicetree/bindings/input/touchscreen/imagis,ist3038c.yaml b/Documentation/devicetree/bindings/input/touchscreen/imagis,ist30xxc.yaml similarity index 99% rename from Documentation/devicetree/bindings/input/touchscreen/imagis,ist3038c.yaml rename to Documentation/devicetree/bindings/input/touchscreen/imagis,ist30xxc.yaml index 0d6b033fd5fb..09bf3a6acc5e 100644 --- a/Documentation/devicetree/bindings/input/touchscreen/imagis,ist3038c.yaml +++ b/Documentation/devicetree/bindings/input/touchscreen/imagis,ist30xxc.yaml @@ -1,7 +1,7 @@ # SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) %YAML 1.2 --- -$id: http://devicetree.org/schemas/input/touchscreen/imagis,ist3038c.yaml# +$id: http://devicetree.org/schemas/input/touchscreen/imagis,ist30xxc.yaml# $schema: http://devicetree.org/meta-schemas/core.yaml# title: Imagis IST30XXC family touchscreen controller diff --git a/MAINTAINERS b/MAINTAINERS index b19995690904..b23e76418d94 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -10209,7 +10209,7 @@ F: drivers/usb/atm/ueagle-atm.c IMAGIS TOUCHSCREEN DRIVER M: Markuss Broks <markuss.broks@gmail.com> S: Maintained -F: Documentation/devicetree/bindings/input/touchscreen/imagis,ist3038c.yaml +F: Documentation/devicetree/bindings/input/touchscreen/imagis,ist30xxc.yaml F: drivers/input/touchscreen/imagis.c IMGTEC ASCII LCD DRIVER diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig index e3e2324547b9..45503aa2653e 100644 --- a/drivers/input/touchscreen/Kconfig +++ b/drivers/input/touchscreen/Kconfig @@ -665,10 +665,10 @@ config TOUCHSCREEN_NOVATEK_NVT_TS module will be called novatek-nvt-ts. config TOUCHSCREEN_IMAGIS - tristate "Imagis touchscreen support" + tristate "Imagis IST30XXC touchscreen support" depends on I2C help - Say Y here if you have an Imagis IST30xxC touchscreen. + Say Y here if you have an Imagis IST30XXC touchscreen. If unsure, say N. To compile this driver as a module, choose M here: the diff --git a/drivers/input/touchscreen/imagis.c b/drivers/input/touchscreen/imagis.c index 07111ca24455..4456f1b4d527 100644 --- a/drivers/input/touchscreen/imagis.c +++ b/drivers/input/touchscreen/imagis.c @@ -11,25 +11,26 @@ #include <linux/property.h> #include <linux/regulator/consumer.h> -#define IST3038C_HIB_ACCESS (0x800B << 16) -#define IST3038C_DIRECT_ACCESS BIT(31) -#define IST3038C_REG_CHIPID 0x40001000 -#define IST3038C_REG_HIB_BASE 0x30000100 -#define IST3038C_REG_TOUCH_STATUS (IST3038C_REG_HIB_BASE | IST3038C_HIB_ACCESS) -#define IST3038C_REG_TOUCH_COORD (IST3038C_REG_HIB_BASE | IST3038C_HIB_ACCESS | 0x8) -#define IST3038C_REG_INTR_MESSAGE (IST3038C_REG_HIB_BASE | IST3038C_HIB_ACCESS | 0x4) +#define IST30XXC_HIB_ACCESS (0x800B << 16) +#define IST30XXC_DIRECT_ACCESS BIT(31) +#define IST30XXC_REG_CHIPID 0x40001000 +#define IST30XXC_REG_HIB_BASE 0x30000100 +#define IST30XXC_REG_TOUCH_STATUS (IST30XXC_REG_HIB_BASE | IST30XXC_HIB_ACCESS) +#define IST30XXC_REG_TOUCH_COORD (IST30XXC_REG_HIB_BASE | IST30XXC_HIB_ACCESS | 0x8) +#define IST30XXC_REG_INTR_MESSAGE (IST30XXC_REG_HIB_BASE | IST30XXC_HIB_ACCESS | 0x4) +#define IST30XXC_CHIP_ON_DELAY_MS 60 +#define IST30XXC_I2C_RETRY_COUNT 3 +#define IST30XXC_MAX_FINGER_NUM 10 +#define IST30XXC_X_MASK GENMASK(23, 12) +#define IST30XXC_X_SHIFT 12 +#define IST30XXC_Y_MASK GENMASK(11, 0) +#define IST30XXC_AREA_MASK GENMASK(27, 24) +#define IST30XXC_AREA_SHIFT 24 +#define IST30XXC_FINGER_COUNT_MASK GENMASK(15, 12) +#define IST30XXC_FINGER_COUNT_SHIFT 12 +#define IST30XXC_FINGER_STATUS_MASK GENMASK(9, 0) + #define IST3038C_WHOAMI 0x38c -#define IST3038C_CHIP_ON_DELAY_MS 60 -#define IST3038C_I2C_RETRY_COUNT 3 -#define IST3038C_MAX_FINGER_NUM 10 -#define IST3038C_X_MASK GENMASK(23, 12) -#define IST3038C_X_SHIFT 12 -#define IST3038C_Y_MASK GENMASK(11, 0) -#define IST3038C_AREA_MASK GENMASK(27, 24) -#define IST3038C_AREA_SHIFT 24 -#define IST3038C_FINGER_COUNT_MASK GENMASK(15, 12) -#define IST3038C_FINGER_COUNT_SHIFT 12 -#define IST3038C_FINGER_STATUS_MASK GENMASK(9, 0) struct imagis_ts { struct i2c_client *client; @@ -57,7 +58,7 @@ static int imagis_i2c_read_reg(struct imagis_ts *ts, }, }; int ret, error; - int retry = IST3038C_I2C_RETRY_COUNT; + int retry = IST30XXC_I2C_RETRY_COUNT; /* Retry in case the controller fails to respond */ do { @@ -84,7 +85,7 @@ static irqreturn_t imagis_interrupt(int irq, void *dev_id) int i; int error; - error = imagis_i2c_read_reg(ts, IST3038C_REG_INTR_MESSAGE, + error = imagis_i2c_read_reg(ts, IST30XXC_REG_INTR_MESSAGE, &intr_message); if (error) { dev_err(&ts->client->dev, @@ -92,20 +93,20 @@ static irqreturn_t imagis_interrupt(int irq, void *dev_id) goto out; } - finger_count = (intr_message & IST3038C_FINGER_COUNT_MASK) >> - IST3038C_FINGER_COUNT_SHIFT; - if (finger_count > IST3038C_MAX_FINGER_NUM) { + finger_count = (intr_message & IST30XXC_FINGER_COUNT_MASK) >> + IST30XXC_FINGER_COUNT_SHIFT; + if (finger_count > IST30XXC_MAX_FINGER_NUM) { dev_err(&ts->client->dev, "finger count %d is more than maximum supported\n", finger_count); goto out; } - finger_pressed = intr_message & IST3038C_FINGER_STATUS_MASK; + finger_pressed = intr_message & IST30XXC_FINGER_STATUS_MASK; for (i = 0; i < finger_count; i++) { error = imagis_i2c_read_reg(ts, - IST3038C_REG_TOUCH_COORD + (i * 4), + IST30XXC_REG_TOUCH_COORD + (i * 4), &finger_status); if (error) { dev_err(&ts->client->dev, @@ -118,12 +119,12 @@ static irqreturn_t imagis_interrupt(int irq, void *dev_id) input_mt_report_slot_state(ts->input_dev, MT_TOOL_FINGER, finger_pressed & BIT(i)); touchscreen_report_pos(ts->input_dev, &ts->prop, - (finger_status & IST3038C_X_MASK) >> - IST3038C_X_SHIFT, - finger_status & IST3038C_Y_MASK, 1); + (finger_status & IST30XXC_X_MASK) >> + IST30XXC_X_SHIFT, + finger_status & IST30XXC_Y_MASK, 1); input_report_abs(ts->input_dev, ABS_MT_TOUCH_MAJOR, - (finger_status & IST3038C_AREA_MASK) >> - IST3038C_AREA_SHIFT); + (finger_status & IST30XXC_AREA_MASK) >> + IST30XXC_AREA_SHIFT); } input_mt_sync_frame(ts->input_dev); @@ -148,7 +149,7 @@ static int imagis_power_on(struct imagis_ts *ts) if (error) return error; - msleep(IST3038C_CHIP_ON_DELAY_MS); + msleep(IST30XXC_CHIP_ON_DELAY_MS); return 0; } @@ -220,7 +221,7 @@ static int imagis_init_input_dev(struct imagis_ts *ts) } error = input_mt_init_slots(input_dev, - IST3038C_MAX_FINGER_NUM, + IST30XXC_MAX_FINGER_NUM, INPUT_MT_DIRECT | INPUT_MT_DROP_UNUSED); if (error) { dev_err(&ts->client->dev, @@ -253,7 +254,7 @@ static int imagis_probe(struct i2c_client *i2c) { struct device *dev = &i2c->dev; struct imagis_ts *ts; - int chip_id, error; + int chip_id, dt_chip_id, error; ts = devm_kzalloc(dev, sizeof(*ts), GFP_KERNEL); if (!ts) @@ -261,6 +262,8 @@ static int imagis_probe(struct i2c_client *i2c) ts->client = i2c; + dt_chip_id = (int)(uintptr_t)device_get_match_data(&i2c->dev); + error = imagis_init_regulators(ts); if (error) { dev_err(dev, "regulator init error: %d\n", error); @@ -280,15 +283,15 @@ static int imagis_probe(struct i2c_client *i2c) } error = imagis_i2c_read_reg(ts, - IST3038C_REG_CHIPID | IST3038C_DIRECT_ACCESS, + IST30XXC_REG_CHIPID | IST30XXC_DIRECT_ACCESS, &chip_id); if (error) { dev_err(dev, "chip ID read failure: %d\n", error); return error; } - if (chip_id != IST3038C_WHOAMI) { - dev_err(dev, "unknown chip ID: 0x%x\n", chip_id); + if (chip_id != dt_chip_id) { + dev_err(dev, "unknown or misconfigured chip ID: 0x%x\n", chip_id); return -EINVAL; } @@ -345,12 +348,18 @@ static DEFINE_SIMPLE_DEV_PM_OPS(imagis_pm_ops, imagis_suspend, imagis_resume); #ifdef CONFIG_OF static const struct of_device_id imagis_of_match[] = { - { .compatible = "imagis,ist3038c", }, + { .compatible = "imagis,ist3038c", .data = (void *)IST3038C_WHOAMI, }, { }, }; MODULE_DEVICE_TABLE(of, imagis_of_match); #endif +static const struct i2c_device_id imagis_ts_i2c_id[] = { + { "ist3038c", IST3038C_WHOAMI, }, + { }, +}; +MODULE_DEVICE_TABLE(i2c, imagis_ts_i2c_id); + static struct i2c_driver imagis_ts_driver = { .driver = { .name = "imagis-touchscreen", @@ -358,10 +367,11 @@ static struct i2c_driver imagis_ts_driver = { .of_match_table = of_match_ptr(imagis_of_match), }, .probe = imagis_probe, + .id_table = imagis_ts_i2c_id, }; module_i2c_driver(imagis_ts_driver); -MODULE_DESCRIPTION("Imagis IST3038C Touchscreen Driver"); +MODULE_DESCRIPTION("Imagis IST30XXC Touchscreen Driver"); MODULE_AUTHOR("Markuss Broks <markuss.broks@gmail.com>"); MODULE_LICENSE("GPL"); -- 2.42.0 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] input: generalize the Imagis touchscreen driver 2023-09-26 17:35 ` [PATCH 1/2] input: generalize the Imagis touchscreen driver Karel Balej @ 2023-09-26 22:17 ` Conor Dooley 2023-09-27 1:48 ` Jeff LaBundy 1 sibling, 0 replies; 10+ messages in thread From: Conor Dooley @ 2023-09-26 22:17 UTC (permalink / raw) To: Karel Balej Cc: Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Markuss Broks, linux-input, devicetree, linux-kernel, Duje Mihanović, ~postmarketos/upstreaming [-- Attachment #1: Type: text/plain, Size: 1162 bytes --] On Tue, Sep 26, 2023 at 07:35:23PM +0200, Karel Balej wrote: > This driver should work with other Imagis ICs of the IST30**C series. > Make that more apparent. > > Co-developed-by: Duje Mihanović <duje.mihanovic@skole.hr> > Signed-off-by: Duje Mihanović <duje.mihanovic@skole.hr> > Signed-off-by: Karel Balej <balejk@matfyz.cz> > --- > ...gis,ist3038c.yaml => imagis,ist30xxc.yaml} | 2 +- > MAINTAINERS | 2 +- > drivers/input/touchscreen/Kconfig | 4 +- > drivers/input/touchscreen/imagis.c | 86 +++++++++++-------- > 4 files changed, 52 insertions(+), 42 deletions(-) > rename Documentation/devicetree/bindings/input/touchscreen/{imagis,ist3038c.yaml => imagis,ist30xxc.yaml} (99%) > > diff --git a/Documentation/devicetree/bindings/input/touchscreen/imagis,ist3038c.yaml b/Documentation/devicetree/bindings/input/touchscreen/imagis,ist30xxc.yaml > similarity index 99% > rename from Documentation/devicetree/bindings/input/touchscreen/imagis,ist3038c.yaml > rename to Documentation/devicetree/bindings/input/touchscreen/imagis,ist30xxc.yaml This rename is pointless. [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 228 bytes --] ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] input: generalize the Imagis touchscreen driver 2023-09-26 17:35 ` [PATCH 1/2] input: generalize the Imagis touchscreen driver Karel Balej 2023-09-26 22:17 ` Conor Dooley @ 2023-09-27 1:48 ` Jeff LaBundy 1 sibling, 0 replies; 10+ messages in thread From: Jeff LaBundy @ 2023-09-27 1:48 UTC (permalink / raw) To: Karel Balej Cc: Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Markuss Broks, linux-input, devicetree, linux-kernel, Duje Mihanović, ~postmarketos/upstreaming Hi Karel, On Tue, Sep 26, 2023 at 07:35:23PM +0200, Karel Balej wrote: > This driver should work with other Imagis ICs of the IST30**C series. > Make that more apparent. > > Co-developed-by: Duje Mihanović <duje.mihanovic@skole.hr> > Signed-off-by: Duje Mihanović <duje.mihanovic@skole.hr> > Signed-off-by: Karel Balej <balejk@matfyz.cz> > --- > ...gis,ist3038c.yaml => imagis,ist30xxc.yaml} | 2 +- > MAINTAINERS | 2 +- > drivers/input/touchscreen/Kconfig | 4 +- > drivers/input/touchscreen/imagis.c | 86 +++++++++++-------- > 4 files changed, 52 insertions(+), 42 deletions(-) > rename Documentation/devicetree/bindings/input/touchscreen/{imagis,ist3038c.yaml => imagis,ist30xxc.yaml} (99%) > > diff --git a/Documentation/devicetree/bindings/input/touchscreen/imagis,ist3038c.yaml b/Documentation/devicetree/bindings/input/touchscreen/imagis,ist30xxc.yaml > similarity index 99% > rename from Documentation/devicetree/bindings/input/touchscreen/imagis,ist3038c.yaml > rename to Documentation/devicetree/bindings/input/touchscreen/imagis,ist30xxc.yaml > index 0d6b033fd5fb..09bf3a6acc5e 100644 > --- a/Documentation/devicetree/bindings/input/touchscreen/imagis,ist3038c.yaml > +++ b/Documentation/devicetree/bindings/input/touchscreen/imagis,ist30xxc.yaml > @@ -1,7 +1,7 @@ > # SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > %YAML 1.2 > --- > -$id: http://devicetree.org/schemas/input/touchscreen/imagis,ist3038c.yaml# > +$id: http://devicetree.org/schemas/input/touchscreen/imagis,ist30xxc.yaml# > $schema: http://devicetree.org/meta-schemas/core.yaml# > > title: Imagis IST30XXC family touchscreen controller > diff --git a/MAINTAINERS b/MAINTAINERS > index b19995690904..b23e76418d94 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -10209,7 +10209,7 @@ F: drivers/usb/atm/ueagle-atm.c > IMAGIS TOUCHSCREEN DRIVER > M: Markuss Broks <markuss.broks@gmail.com> > S: Maintained > -F: Documentation/devicetree/bindings/input/touchscreen/imagis,ist3038c.yaml > +F: Documentation/devicetree/bindings/input/touchscreen/imagis,ist30xxc.yaml > F: drivers/input/touchscreen/imagis.c > > IMGTEC ASCII LCD DRIVER > diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig > index e3e2324547b9..45503aa2653e 100644 > --- a/drivers/input/touchscreen/Kconfig > +++ b/drivers/input/touchscreen/Kconfig > @@ -665,10 +665,10 @@ config TOUCHSCREEN_NOVATEK_NVT_TS > module will be called novatek-nvt-ts. > > config TOUCHSCREEN_IMAGIS > - tristate "Imagis touchscreen support" > + tristate "Imagis IST30XXC touchscreen support" > depends on I2C > help > - Say Y here if you have an Imagis IST30xxC touchscreen. > + Say Y here if you have an Imagis IST30XXC touchscreen. > If unsure, say N. > > To compile this driver as a module, choose M here: the > diff --git a/drivers/input/touchscreen/imagis.c b/drivers/input/touchscreen/imagis.c > index 07111ca24455..4456f1b4d527 100644 > --- a/drivers/input/touchscreen/imagis.c > +++ b/drivers/input/touchscreen/imagis.c > @@ -11,25 +11,26 @@ > #include <linux/property.h> > #include <linux/regulator/consumer.h> > > -#define IST3038C_HIB_ACCESS (0x800B << 16) > -#define IST3038C_DIRECT_ACCESS BIT(31) > -#define IST3038C_REG_CHIPID 0x40001000 > -#define IST3038C_REG_HIB_BASE 0x30000100 > -#define IST3038C_REG_TOUCH_STATUS (IST3038C_REG_HIB_BASE | IST3038C_HIB_ACCESS) > -#define IST3038C_REG_TOUCH_COORD (IST3038C_REG_HIB_BASE | IST3038C_HIB_ACCESS | 0x8) > -#define IST3038C_REG_INTR_MESSAGE (IST3038C_REG_HIB_BASE | IST3038C_HIB_ACCESS | 0x4) > +#define IST30XXC_HIB_ACCESS (0x800B << 16) > +#define IST30XXC_DIRECT_ACCESS BIT(31) > +#define IST30XXC_REG_CHIPID 0x40001000 > +#define IST30XXC_REG_HIB_BASE 0x30000100 > +#define IST30XXC_REG_TOUCH_STATUS (IST30XXC_REG_HIB_BASE | IST30XXC_HIB_ACCESS) > +#define IST30XXC_REG_TOUCH_COORD (IST30XXC_REG_HIB_BASE | IST30XXC_HIB_ACCESS | 0x8) > +#define IST30XXC_REG_INTR_MESSAGE (IST30XXC_REG_HIB_BASE | IST30XXC_HIB_ACCESS | 0x4) > +#define IST30XXC_CHIP_ON_DELAY_MS 60 > +#define IST30XXC_I2C_RETRY_COUNT 3 > +#define IST30XXC_MAX_FINGER_NUM 10 > +#define IST30XXC_X_MASK GENMASK(23, 12) > +#define IST30XXC_X_SHIFT 12 > +#define IST30XXC_Y_MASK GENMASK(11, 0) > +#define IST30XXC_AREA_MASK GENMASK(27, 24) > +#define IST30XXC_AREA_SHIFT 24 > +#define IST30XXC_FINGER_COUNT_MASK GENMASK(15, 12) > +#define IST30XXC_FINGER_COUNT_SHIFT 12 > +#define IST30XXC_FINGER_STATUS_MASK GENMASK(9, 0) > + > #define IST3038C_WHOAMI 0x38c > -#define IST3038C_CHIP_ON_DELAY_MS 60 > -#define IST3038C_I2C_RETRY_COUNT 3 > -#define IST3038C_MAX_FINGER_NUM 10 > -#define IST3038C_X_MASK GENMASK(23, 12) > -#define IST3038C_X_SHIFT 12 > -#define IST3038C_Y_MASK GENMASK(11, 0) > -#define IST3038C_AREA_MASK GENMASK(27, 24) > -#define IST3038C_AREA_SHIFT 24 > -#define IST3038C_FINGER_COUNT_MASK GENMASK(15, 12) > -#define IST3038C_FINGER_COUNT_SHIFT 12 > -#define IST3038C_FINGER_STATUS_MASK GENMASK(9, 0) > There is no need to change all of the namespacing from IST3038C to IST30XXC; this is purely cosmetic and adds noise to the actual patch. It is perfectly acceptable for the driver to call itself IST3038C throughout while remaining compatible with other devices (e.g. IST3032C), in fact this flexibility is the intent of the compatible string in the first place. [...] > > -MODULE_DESCRIPTION("Imagis IST3038C Touchscreen Driver"); > +MODULE_DESCRIPTION("Imagis IST30XXC Touchscreen Driver"); > MODULE_AUTHOR("Markuss Broks <markuss.broks@gmail.com>"); > MODULE_LICENSE("GPL"); > -- > 2.42.0 > Kind regards, Jeff LaBundy ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 2/2] input: Imagis: add support for the IST3032C touchscreen 2023-09-26 17:35 [PATCH 0/2] input: Imagis: add support for the IST3032C touchscreen Karel Balej 2023-09-26 17:35 ` [PATCH 1/2] input: generalize the Imagis touchscreen driver Karel Balej @ 2023-09-26 17:35 ` Karel Balej 2023-09-27 1:40 ` Jeff LaBundy 2023-09-28 5:25 ` Krzysztof Kozlowski 2023-09-26 22:16 ` [PATCH 0/2] " Conor Dooley 2 siblings, 2 replies; 10+ messages in thread From: Karel Balej @ 2023-09-26 17:35 UTC (permalink / raw) To: Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Markuss Broks, linux-input, devicetree, linux-kernel, Duje Mihanović, ~postmarketos/upstreaming Cc: Karel Balej The downstream driver sets the regulator voltage to 3.1 V. Without this, the touchscreen generates random touches even after it is no longer being touched. It is unknown whether the same problem appears with other chips of the IST30**C series. Co-developed-by: Duje Mihanović <duje.mihanovic@skole.hr> Signed-off-by: Duje Mihanović <duje.mihanovic@skole.hr> Signed-off-by: Karel Balej <balejk@matfyz.cz> --- .../bindings/input/touchscreen/imagis,ist30xxc.yaml | 1 + drivers/input/touchscreen/imagis.c | 13 +++++++++++++ 2 files changed, 14 insertions(+) diff --git a/Documentation/devicetree/bindings/input/touchscreen/imagis,ist30xxc.yaml b/Documentation/devicetree/bindings/input/touchscreen/imagis,ist30xxc.yaml index 09bf3a6acc5e..d6f75bbfaec3 100644 --- a/Documentation/devicetree/bindings/input/touchscreen/imagis,ist30xxc.yaml +++ b/Documentation/devicetree/bindings/input/touchscreen/imagis,ist30xxc.yaml @@ -18,6 +18,7 @@ properties: compatible: enum: + - imagis,ist3032c - imagis,ist3038c reg: diff --git a/drivers/input/touchscreen/imagis.c b/drivers/input/touchscreen/imagis.c index 4456f1b4d527..df9a8fbf2c5f 100644 --- a/drivers/input/touchscreen/imagis.c +++ b/drivers/input/touchscreen/imagis.c @@ -30,6 +30,7 @@ #define IST30XXC_FINGER_COUNT_SHIFT 12 #define IST30XXC_FINGER_STATUS_MASK GENMASK(9, 0) +#define IST3032C_WHOAMI 0x32c #define IST3038C_WHOAMI 0x38c struct imagis_ts { @@ -295,6 +296,16 @@ static int imagis_probe(struct i2c_client *i2c) return -EINVAL; } + if (chip_id == IST3032C_WHOAMI) { + /* + * if the regulator voltage is not set like this, the touchscreen + * generates random touches without user interaction + */ + error = regulator_set_voltage(ts->supplies[0].consumer, 3100000, 3100000); + if (error) + dev_warn(dev, "failed to set regulator voltage\n"); + } + error = devm_request_threaded_irq(dev, i2c->irq, NULL, imagis_interrupt, IRQF_ONESHOT | IRQF_NO_AUTOEN, @@ -348,6 +359,7 @@ static DEFINE_SIMPLE_DEV_PM_OPS(imagis_pm_ops, imagis_suspend, imagis_resume); #ifdef CONFIG_OF static const struct of_device_id imagis_of_match[] = { + { .compatible = "imagis,ist3032c", .data = (void *)IST3032C_WHOAMI, }, { .compatible = "imagis,ist3038c", .data = (void *)IST3038C_WHOAMI, }, { }, }; @@ -355,6 +367,7 @@ MODULE_DEVICE_TABLE(of, imagis_of_match); #endif static const struct i2c_device_id imagis_ts_i2c_id[] = { + { "ist3032c", IST3032C_WHOAMI, }, { "ist3038c", IST3038C_WHOAMI, }, { }, }; -- 2.42.0 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] input: Imagis: add support for the IST3032C touchscreen 2023-09-26 17:35 ` [PATCH 2/2] input: Imagis: add support for the IST3032C touchscreen Karel Balej @ 2023-09-27 1:40 ` Jeff LaBundy 2023-09-28 17:56 ` Karel Balej 2023-09-28 5:25 ` Krzysztof Kozlowski 1 sibling, 1 reply; 10+ messages in thread From: Jeff LaBundy @ 2023-09-27 1:40 UTC (permalink / raw) To: Karel Balej Cc: Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Markuss Broks, linux-input, devicetree, linux-kernel, Duje Mihanović, ~postmarketos/upstreaming Hi Karel, On Tue, Sep 26, 2023 at 07:35:24PM +0200, Karel Balej wrote: > The downstream driver sets the regulator voltage to 3.1 V. Without this, > the touchscreen generates random touches even after it is no longer > being touched. It is unknown whether the same problem appears with other > chips of the IST30**C series. > > Co-developed-by: Duje Mihanović <duje.mihanovic@skole.hr> > Signed-off-by: Duje Mihanović <duje.mihanovic@skole.hr> > Signed-off-by: Karel Balej <balejk@matfyz.cz> > --- > .../bindings/input/touchscreen/imagis,ist30xxc.yaml | 1 + > drivers/input/touchscreen/imagis.c | 13 +++++++++++++ > 2 files changed, 14 insertions(+) > > diff --git a/Documentation/devicetree/bindings/input/touchscreen/imagis,ist30xxc.yaml b/Documentation/devicetree/bindings/input/touchscreen/imagis,ist30xxc.yaml > index 09bf3a6acc5e..d6f75bbfaec3 100644 > --- a/Documentation/devicetree/bindings/input/touchscreen/imagis,ist30xxc.yaml > +++ b/Documentation/devicetree/bindings/input/touchscreen/imagis,ist30xxc.yaml > @@ -18,6 +18,7 @@ properties: > > compatible: > enum: > + - imagis,ist3032c > - imagis,ist3038c > > reg: > diff --git a/drivers/input/touchscreen/imagis.c b/drivers/input/touchscreen/imagis.c > index 4456f1b4d527..df9a8fbf2c5f 100644 > --- a/drivers/input/touchscreen/imagis.c > +++ b/drivers/input/touchscreen/imagis.c > @@ -30,6 +30,7 @@ > #define IST30XXC_FINGER_COUNT_SHIFT 12 > #define IST30XXC_FINGER_STATUS_MASK GENMASK(9, 0) > > +#define IST3032C_WHOAMI 0x32c > #define IST3038C_WHOAMI 0x38c > > struct imagis_ts { > @@ -295,6 +296,16 @@ static int imagis_probe(struct i2c_client *i2c) > return -EINVAL; > } > > + if (chip_id == IST3032C_WHOAMI) { > + /* > + * if the regulator voltage is not set like this, the touchscreen > + * generates random touches without user interaction > + */ > + error = regulator_set_voltage(ts->supplies[0].consumer, 3100000, 3100000); > + if (error) > + dev_warn(dev, "failed to set regulator voltage\n"); > + } > + Opinions may vary, but mine is that this kind of hard-coded board-level policy does not belong in the driver. Surely the supply need not be equal to exactly 3.1 V and this is merely an upper or lower limit? Assuming so, what if the board designer opts to share this supply with another consumer that requires a specific voltage not equal to 3.1 V, but still within the safe range of IST3032C? To me, this restriction belongs in dts, specifically within the regulator child node referenced by the client which bears the new 'ist3032c' compatible string. Maybe a better solution is to simply note this presumed silicon erratum in the description of the vdd supply in the binding which, as Conor states, must not be clubbed with driver patches. > error = devm_request_threaded_irq(dev, i2c->irq, > NULL, imagis_interrupt, > IRQF_ONESHOT | IRQF_NO_AUTOEN, > @@ -348,6 +359,7 @@ static DEFINE_SIMPLE_DEV_PM_OPS(imagis_pm_ops, imagis_suspend, imagis_resume); > > #ifdef CONFIG_OF > static const struct of_device_id imagis_of_match[] = { > + { .compatible = "imagis,ist3032c", .data = (void *)IST3032C_WHOAMI, }, > { .compatible = "imagis,ist3038c", .data = (void *)IST3038C_WHOAMI, }, > { }, > }; > @@ -355,6 +367,7 @@ MODULE_DEVICE_TABLE(of, imagis_of_match); > #endif > > static const struct i2c_device_id imagis_ts_i2c_id[] = { > + { "ist3032c", IST3032C_WHOAMI, }, > { "ist3038c", IST3038C_WHOAMI, }, > { }, > }; > -- > 2.42.0 > Kind regards, Jeff LaBundy ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] input: Imagis: add support for the IST3032C touchscreen 2023-09-27 1:40 ` Jeff LaBundy @ 2023-09-28 17:56 ` Karel Balej 2023-10-22 17:17 ` Jeff LaBundy 0 siblings, 1 reply; 10+ messages in thread From: Karel Balej @ 2023-09-28 17:56 UTC (permalink / raw) To: Jeff LaBundy, Karel Balej Cc: Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Markuss Broks, linux-input, devicetree, linux-kernel, Duje Mihanović, ~postmarketos/upstreaming Hello, Jeff, thank you very much for your feedback. > > + if (chip_id == IST3032C_WHOAMI) { > > + /* > > + * if the regulator voltage is not set like this, the touchscreen > > + * generates random touches without user interaction > > + */ > > + error = regulator_set_voltage(ts->supplies[0].consumer, 3100000, 3100000); > > + if (error) > > + dev_warn(dev, "failed to set regulator voltage\n"); > > + } > > + > > Opinions may vary, but mine is that this kind of hard-coded board-level policy > does not belong in the driver. Surely the supply need not be equal to exactly > 3.1 V and this is merely an upper or lower limit? Assuming so, what if the board > designer opts to share this supply with another consumer that requires a specific > voltage not equal to 3.1 V, but still within the safe range of IST3032C? > > To me, this restriction belongs in dts, specifically within the regulator child > node referenced by the client which bears the new 'ist3032c' compatible string. > Maybe a better solution is to simply note this presumed silicon erratum in the > description of the vdd supply in the binding which, as Conor states, must not be > clubbed with driver patches. I agree that the voltage should not be hardcoded. I do not know what the safe range for the touchscreen is though, because the downstream driver does exactly this. I will try to test it with several values within the range allowed by the regulator and see if I can determine some limits on when the "ghost" touches do not appear. However I am not sure whether this setting should be moved to the regulator DT - it is my understanding that the DT for the regulator should list the min/max range *supported* by the regulator, not conform to requirements of its consumers, which should instead ask for the regulator to be set to a range they require themselves, via their driver - is it not so? The regulator driver is not mainlined yet (although I managed to get the downstream code working with mainline), however the downstream DT contains much wider range of supported voltage (compared to those 3.1 V used by the touchscreen) - an information which would get lost if I set the DT for the regulator by the requirements of the touchscreen, which I believe would have similiar implications as what you said regarding using this regulator with other consumers. What would seem a reasonable solution to me would be to move the voltage range values to the touchscreen DT (which incidentally is what the downstream driver does also, except it uses one value for both min and max), so that they would be set by the driver but not hardcoded in the code - what do you think about this? Best regards, K. B. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] input: Imagis: add support for the IST3032C touchscreen 2023-09-28 17:56 ` Karel Balej @ 2023-10-22 17:17 ` Jeff LaBundy 0 siblings, 0 replies; 10+ messages in thread From: Jeff LaBundy @ 2023-10-22 17:17 UTC (permalink / raw) To: Karel Balej Cc: Karel Balej, Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Markuss Broks, linux-input, devicetree, linux-kernel, Duje Mihanović, ~postmarketos/upstreaming Hi Karel, On Thu, Sep 28, 2023 at 07:56:45PM +0200, Karel Balej wrote: > Hello, Jeff, > > thank you very much for your feedback. > > > > + if (chip_id == IST3032C_WHOAMI) { > > > + /* > > > + * if the regulator voltage is not set like this, the touchscreen > > > + * generates random touches without user interaction > > > + */ > > > + error = regulator_set_voltage(ts->supplies[0].consumer, 3100000, 3100000); > > > + if (error) > > > + dev_warn(dev, "failed to set regulator voltage\n"); > > > + } > > > + > > > > Opinions may vary, but mine is that this kind of hard-coded board-level policy > > does not belong in the driver. Surely the supply need not be equal to exactly > > 3.1 V and this is merely an upper or lower limit? Assuming so, what if the board > > designer opts to share this supply with another consumer that requires a specific > > voltage not equal to 3.1 V, but still within the safe range of IST3032C? > > > > To me, this restriction belongs in dts, specifically within the regulator child > > node referenced by the client which bears the new 'ist3032c' compatible string. > > Maybe a better solution is to simply note this presumed silicon erratum in the > > description of the vdd supply in the binding which, as Conor states, must not be > > clubbed with driver patches. > > I agree that the voltage should not be hardcoded. I do not know what the > safe range for the touchscreen is though, because the downstream driver > does exactly this. I will try to test it with several values within the > range allowed by the regulator and see if I can determine some limits on > when the "ghost" touches do not appear. > > However I am not sure whether this setting should be moved to the > regulator DT - it is my understanding that the DT for the regulator > should list the min/max range *supported* by the regulator, not conform > to requirements of its consumers, which should instead ask for the > regulator to be set to a range they require themselves, via their driver > - is it not so? > > The regulator driver is not mainlined yet (although I managed to get the > downstream code working with mainline), however the downstream DT > contains much wider range of supported voltage (compared to those 3.1 V > used by the touchscreen) - an information which would get lost if I set > the DT for the regulator by the requirements of the touchscreen, which I > believe would have similiar implications as what you said regarding > using this regulator with other consumers. > > What would seem a reasonable solution to me would be to move the voltage > range values to the touchscreen DT (which incidentally is what the > downstream driver does also, except it uses one value for both min and > max), so that they would be set by the driver but not hardcoded in the > code - what do you think about this? I believe this has been clarified in the other thread with Markuss, but just to close this out: in general, individual drivers should not be setting the output value of a regulator. Instead, they should merely mark themselves as a consumer of a regulator, and increment or decrement its usage counter by enabling and disabling the regulator, respectively. You are correct that the regulator node typically specifies the minimum and maximum voltages supported by the regulator, but if your board has a stricter range because said regulator is tied to something such as this touchscreen controller, then you can override the maximum voltage with something smaller (e.g. 3.1 V). The regulator framework will then set the output voltage according to the ultimate range defined in the device tree. Often times the baseline regulator nodes are defined in a PMIC-specific .dtsi file, or part of a more generic board definition. Then, some nodes are overridden at a higher level in the heirarchy tailored to the board, or an overlay applied by the bootloader. > > Best regards, > K. B. Kind regards, Jeff LaBundy ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] input: Imagis: add support for the IST3032C touchscreen 2023-09-26 17:35 ` [PATCH 2/2] input: Imagis: add support for the IST3032C touchscreen Karel Balej 2023-09-27 1:40 ` Jeff LaBundy @ 2023-09-28 5:25 ` Krzysztof Kozlowski 1 sibling, 0 replies; 10+ messages in thread From: Krzysztof Kozlowski @ 2023-09-28 5:25 UTC (permalink / raw) To: Karel Balej, Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Markuss Broks, linux-input, devicetree, linux-kernel, Duje Mihanović, ~postmarketos/upstreaming On 26/09/2023 19:35, Karel Balej wrote: > The downstream driver sets the regulator voltage to 3.1 V. Without this, > the touchscreen generates random touches even after it is no longer > being touched. It is unknown whether the same problem appears with other > chips of the IST30**C series. > > Co-developed-by: Duje Mihanović <duje.mihanovic@skole.hr> > Signed-off-by: Duje Mihanović <duje.mihanovic@skole.hr> > Signed-off-by: Karel Balej <balejk@matfyz.cz> > --- > .../bindings/input/touchscreen/imagis,ist30xxc.yaml | 1 + Bindings are always separate patches. Always. Please run scripts/checkpatch.pl and fix reported warnings. Some warnings can be ignored, but the code here looks like it needs a fix. Feel free to get in touch if the warning is not clear. Please use subject prefixes matching the subsystem. You can get them for example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory your patch is touching. Best regards, Krzysztof ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 0/2] input: Imagis: add support for the IST3032C touchscreen 2023-09-26 17:35 [PATCH 0/2] input: Imagis: add support for the IST3032C touchscreen Karel Balej 2023-09-26 17:35 ` [PATCH 1/2] input: generalize the Imagis touchscreen driver Karel Balej 2023-09-26 17:35 ` [PATCH 2/2] input: Imagis: add support for the IST3032C touchscreen Karel Balej @ 2023-09-26 22:16 ` Conor Dooley 2 siblings, 0 replies; 10+ messages in thread From: Conor Dooley @ 2023-09-26 22:16 UTC (permalink / raw) To: Karel Balej Cc: Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Markuss Broks, linux-input, devicetree, linux-kernel, Duje Mihanović, ~postmarketos/upstreaming [-- Attachment #1: Type: text/plain, Size: 1201 bytes --] On Tue, Sep 26, 2023 at 07:35:22PM +0200, Karel Balej wrote: > This patch series extends the Imagis driver to support the IST3032C > touchscreen, which is used for instance with the samsung,coreprimevelte > smartphone, with which this was tested. To use it with this model > however, the regulator driver needs to be ported first. Support for this > smartphone is not yet in-tree, upstreaming is ongoing at [1]. > > [1] https://lore.kernel.org/all/20230812-pxa1908-lkml-v5-0-a5d51937ee34@skole.hr/ For both patches, changes to dt-bindings need to be in their own patches & not bundled with drivers. > > Karel Balej (2): > input: generalize the Imagis touchscreen driver > input: Imagis: add support for the IST3032C touchscreen > > ...gis,ist3038c.yaml => imagis,ist30xxc.yaml} | 3 +- > MAINTAINERS | 2 +- > drivers/input/touchscreen/Kconfig | 4 +- > drivers/input/touchscreen/imagis.c | 99 ++++++++++++------- > 4 files changed, 66 insertions(+), 42 deletions(-) > rename Documentation/devicetree/bindings/input/touchscreen/{imagis,ist3038c.yaml => imagis,ist30xxc.yaml} (97%) > > -- > 2.42.0 > [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 228 bytes --] ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2023-10-22 17:18 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-09-26 17:35 [PATCH 0/2] input: Imagis: add support for the IST3032C touchscreen Karel Balej 2023-09-26 17:35 ` [PATCH 1/2] input: generalize the Imagis touchscreen driver Karel Balej 2023-09-26 22:17 ` Conor Dooley 2023-09-27 1:48 ` Jeff LaBundy 2023-09-26 17:35 ` [PATCH 2/2] input: Imagis: add support for the IST3032C touchscreen Karel Balej 2023-09-27 1:40 ` Jeff LaBundy 2023-09-28 17:56 ` Karel Balej 2023-10-22 17:17 ` Jeff LaBundy 2023-09-28 5:25 ` Krzysztof Kozlowski 2023-09-26 22:16 ` [PATCH 0/2] " Conor Dooley
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.