| Message ID | 20230926173531.18715-1-balejk@matfyz.cz |
|---|---|
| Headers | show |
| Series | input: Imagis: add support for the IST3032C touchscreen | expand |
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 >
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
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.
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%)