[PATCH 0/7] Input: cyttsp - First round of modernizations

[PATCH 0/7] Input: cyttsp - First round of modernizations

[Linus Walleij]

This fixes some issues I had when getting the CYTTSP
(also known as CY8CTMA340) to work with a modern device
tree and userspace.

The device stands out as there are no in-kernel users of
either platform data nor device tree.

I am going to submit a device tree client, if you have
other concerns, then step forward now.

With these patches the touchscreen works fine with
PostmarketOS on the Ux500 HREF using Wayland and Phosh
as UI.

This work is really being done to get a baseline for
refurbishing the PL022 SPI driver but sometimes you
have to fix a bunch of stuff.

Linus Walleij (7):
  Input: cyttsp - Probe from compatibles
  Input: cyttsp - Obtain regulators
  Input: cyttsp - Error message on boot mode exit error
  Input: cyttsp - Reduce reset pulse timings
  Input: cyttsp - Drop the phys path
  Input: cyttsp - Set abs params for ABS_MT_TOUCH_MAJOR
  Input: cyttsp - Flag the device properly

 drivers/input/touchscreen/cyttsp_core.c | 52 +++++++++++++++++++++----
 drivers/input/touchscreen/cyttsp_core.h |  3 +-
 drivers/input/touchscreen/cyttsp_i2c.c  |  7 ++++
 drivers/input/touchscreen/cyttsp_spi.c  |  7 ++++
 4 files changed, 60 insertions(+), 9 deletions(-)

-- 
2.29.2

[PATCH 1/7] Input: cyttsp - Probe from compatibles

[Linus Walleij]

The driver (both SPI and I2C interface) should probe from
the compatible strings, cypress,cy8ctma340 etc when using
device tree, not as now, where it is probing implicitly from
the I2C/SPI node name "cypress,cyttsp-i2c" etc.

Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/input/touchscreen/cyttsp_i2c.c | 7 +++++++
 drivers/input/touchscreen/cyttsp_spi.c | 7 +++++++
 2 files changed, 14 insertions(+)

diff --git a/drivers/input/touchscreen/cyttsp_i2c.c b/drivers/input/touchscreen/cyttsp_i2c.c
index 061debf64a2b..0f097f34832a 100644
--- a/drivers/input/touchscreen/cyttsp_i2c.c
+++ b/drivers/input/touchscreen/cyttsp_i2c.c
@@ -52,10 +52,17 @@ static const struct i2c_device_id cyttsp_i2c_id[] = {
 };
 MODULE_DEVICE_TABLE(i2c, cyttsp_i2c_id);
 
+static const struct of_device_id cyttsp_of_i2c_match[] = {
+	{ .compatible = "cypress,cy8ctma340", },
+	{ .compatible = "cypress,cy8ctst341", },
+	{ /* sentinel */ }
+};
+
 static struct i2c_driver cyttsp_i2c_driver = {
 	.driver = {
 		.name	= CY_I2C_NAME,
 		.pm	= &cyttsp_pm_ops,
+		.of_match_table = cyttsp_of_i2c_match,
 	},
 	.probe		= cyttsp_i2c_probe,
 	.id_table	= cyttsp_i2c_id,
diff --git a/drivers/input/touchscreen/cyttsp_spi.c b/drivers/input/touchscreen/cyttsp_spi.c
index 54e410921d53..ad8b3c6c4d3e 100644
--- a/drivers/input/touchscreen/cyttsp_spi.c
+++ b/drivers/input/touchscreen/cyttsp_spi.c
@@ -160,10 +160,17 @@ static int cyttsp_spi_probe(struct spi_device *spi)
 	return 0;
 }
 
+static const struct of_device_id cyttsp_of_spi_match[] = {
+	{ .compatible = "cypress,cy8ctma340", },
+	{ .compatible = "cypress,cy8ctst341", },
+	{ /* sentinel */ }
+};
+
 static struct spi_driver cyttsp_spi_driver = {
 	.driver = {
 		.name	= CY_SPI_NAME,
 		.pm	= &cyttsp_pm_ops,
+		.of_match_table = cyttsp_of_spi_match,
 	},
 	.probe  = cyttsp_spi_probe,
 };
-- 
2.29.2

Re: [PATCH 1/7] Input: cyttsp - Probe from compatibles

[Javier Martinez Canillas]

Hello Linus,

On Tue, Mar 30, 2021 at 10:54 AM Linus Walleij <linus.walleij@linaro.org> wrote:
>
> The driver (both SPI and I2C interface) should probe from
> the compatible strings, cypress,cy8ctma340 etc when using
> device tree, not as now, where it is probing implicitly from
> the I2C/SPI node name "cypress,cyttsp-i2c" etc.
>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
>  drivers/input/touchscreen/cyttsp_i2c.c | 7 +++++++
>  drivers/input/touchscreen/cyttsp_spi.c | 7 +++++++
>  2 files changed, 14 insertions(+)
>
> diff --git a/drivers/input/touchscreen/cyttsp_i2c.c b/drivers/input/touchscreen/cyttsp_i2c.c
> index 061debf64a2b..0f097f34832a 100644
> --- a/drivers/input/touchscreen/cyttsp_i2c.c
> +++ b/drivers/input/touchscreen/cyttsp_i2c.c
> @@ -52,10 +52,17 @@ static const struct i2c_device_id cyttsp_i2c_id[] = {
>  };
>  MODULE_DEVICE_TABLE(i2c, cyttsp_i2c_id);
>
> +static const struct of_device_id cyttsp_of_i2c_match[] = {
> +       { .compatible = "cypress,cy8ctma340", },
> +       { .compatible = "cypress,cy8ctst341", },
> +       { /* sentinel */ }
> +};
> +

I think we want to export this as modalias with
MODULE_DEVICE_TABLE(of, cyttsp_of_i2c_match) so the driver could be
autoloaded if built as a module ?

>  static struct i2c_driver cyttsp_i2c_driver = {
>         .driver = {
>                 .name   = CY_I2C_NAME,
>                 .pm     = &cyttsp_pm_ops,
> +               .of_match_table = cyttsp_of_i2c_match,
>         },
>         .probe          = cyttsp_i2c_probe,
>         .id_table       = cyttsp_i2c_id,
> diff --git a/drivers/input/touchscreen/cyttsp_spi.c b/drivers/input/touchscreen/cyttsp_spi.c
> index 54e410921d53..ad8b3c6c4d3e 100644
> --- a/drivers/input/touchscreen/cyttsp_spi.c
> +++ b/drivers/input/touchscreen/cyttsp_spi.c
> @@ -160,10 +160,17 @@ static int cyttsp_spi_probe(struct spi_device *spi)
>         return 0;
>  }
>
> +static const struct of_device_id cyttsp_of_spi_match[] = {
> +       { .compatible = "cypress,cy8ctma340", },
> +       { .compatible = "cypress,cy8ctst341", },
> +       { /* sentinel */ }
> +};
> +

And same here.

Other than that, the patch looks good to me.

Reviewed-by: Javier Martinez Canillas <javier@dowhile0.org>

Best regards,
Javier

[PATCH 2/7] Input: cyttsp - Obtain regulators

[Linus Walleij]

The CYTTSP TMA340 chips have two supplies: VCPIN and
VDD for analog and digital voltage respectively.
Add some minimal code to obtain and enable these
regulators if need be.

Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/input/touchscreen/cyttsp_core.c | 30 +++++++++++++++++++++++--
 drivers/input/touchscreen/cyttsp_core.h |  2 ++
 2 files changed, 30 insertions(+), 2 deletions(-)

diff --git a/drivers/input/touchscreen/cyttsp_core.c b/drivers/input/touchscreen/cyttsp_core.c
index b9772192b5ea..a19d7cce95ca 100644
--- a/drivers/input/touchscreen/cyttsp_core.c
+++ b/drivers/input/touchscreen/cyttsp_core.c
@@ -22,6 +22,7 @@
 #include <linux/slab.h>
 #include <linux/property.h>
 #include <linux/gpio/consumer.h>
+#include <linux/regulator/consumer.h>
 
 #include "cyttsp_core.h"
 
@@ -621,6 +622,19 @@ struct cyttsp *cyttsp_probe(const struct cyttsp_bus_ops *bus_ops,
 	ts->bus_ops = bus_ops;
 	ts->irq = irq;
 
+	/*
+	 * VCPIN is the analog voltage supply
+	 * VDD is the digital voltage supply
+	 */
+	ts->regulators[0].supply = "vcpin";
+	ts->regulators[1].supply = "vdd";
+	error = devm_regulator_bulk_get(dev, ARRAY_SIZE(ts->regulators),
+					ts->regulators);
+	if (error) {
+		dev_err(dev, "Failed to get regulators: %d\n", error);
+		return ERR_PTR(error);
+	}
+
 	ts->reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
 	if (IS_ERR(ts->reset_gpio)) {
 		error = PTR_ERR(ts->reset_gpio);
@@ -666,20 +680,32 @@ struct cyttsp *cyttsp_probe(const struct cyttsp_bus_ops *bus_ops,
 
 	disable_irq(ts->irq);
 
+	error = regulator_bulk_enable(ARRAY_SIZE(ts->regulators),
+				      ts->regulators);
+	if (error) {
+		dev_err(dev, "Cannot enable regulators: %d\n", error);
+		return ERR_PTR(error);
+	}
+
 	cyttsp_hard_reset(ts);
 
 	error = cyttsp_power_on(ts);
 	if (error)
-		return ERR_PTR(error);
+		goto err_dis_reg;
 
 	error = input_register_device(input_dev);
 	if (error) {
 		dev_err(ts->dev, "failed to register input device: %d\n",
 			error);
-		return ERR_PTR(error);
+		goto err_dis_reg;
 	}
 
 	return ts;
+
+err_dis_reg:
+	regulator_bulk_disable(ARRAY_SIZE(ts->regulators),
+			       ts->regulators);
+	return ERR_PTR(error);
 }
 EXPORT_SYMBOL_GPL(cyttsp_probe);
 
diff --git a/drivers/input/touchscreen/cyttsp_core.h b/drivers/input/touchscreen/cyttsp_core.h
index 8c651336ac12..c102a094e888 100644
--- a/drivers/input/touchscreen/cyttsp_core.h
+++ b/drivers/input/touchscreen/cyttsp_core.h
@@ -23,6 +23,7 @@
 #include <linux/types.h>
 #include <linux/device.h>
 #include <linux/input/cyttsp.h>
+#include <linux/regulator/consumer.h>
 
 #define CY_NUM_RETRY		16 /* max number of retries for read ops */
 
@@ -123,6 +124,7 @@ struct cyttsp {
 	enum cyttsp_state state;
 	bool suspended;
 
+	struct regulator_bulk_data regulators[2];
 	struct gpio_desc *reset_gpio;
 	bool use_hndshk;
 	u8 act_dist;
-- 
2.29.2

Re: [PATCH 2/7] Input: cyttsp - Obtain regulators

[Javier Martinez Canillas]

On Tue, Mar 30, 2021 at 10:54 AM Linus Walleij <linus.walleij@linaro.org> wrote:

[snip]

>
> +       struct regulator_bulk_data regulators[2];
>         struct gpio_desc *reset_gpio;
>         bool use_hndshk;
>         u8 act_dist;

Wonder if it is worth adding a constant macro for the 2.

Patch looks good to me:

Reviewed-by: Javier Martinez Canillas <javier@dowhile0.org>

Best regards,
Javier

[PATCH 3/7] Input: cyttsp - Error message on boot mode exit error

[Linus Walleij]

Provide a proper error message when attempting to exit
boot loader mode and failing, which is something that
happened to me.

Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/input/touchscreen/cyttsp_core.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/input/touchscreen/cyttsp_core.c b/drivers/input/touchscreen/cyttsp_core.c
index a19d7cce95ca..91f8f38b3f06 100644
--- a/drivers/input/touchscreen/cyttsp_core.c
+++ b/drivers/input/touchscreen/cyttsp_core.c
@@ -410,8 +410,10 @@ static int cyttsp_power_on(struct cyttsp *ts)
 	if (GET_BOOTLOADERMODE(ts->bl_data.bl_status) &&
 	    IS_VALID_APP(ts->bl_data.bl_status)) {
 		error = cyttsp_exit_bl_mode(ts);
-		if (error)
+		if (error) {
+			dev_err(ts->dev, "failed to exit bootloader mode\n");
 			return error;
+		}
 	}
 
 	if (GET_HSTMODE(ts->bl_data.bl_file) != CY_OPERATE_MODE ||
-- 
2.29.2

Re: [PATCH 3/7] Input: cyttsp - Error message on boot mode exit error

[Javier Martinez Canillas]

On Tue, Mar 30, 2021 at 10:54 AM Linus Walleij <linus.walleij@linaro.org> wrote:
>
> Provide a proper error message when attempting to exit
> boot loader mode and failing, which is something that
> happened to me.
>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

Reviewed-by: Javier Martinez Canillas <javier@dowhile0.org>

[PATCH 4/7] Input: cyttsp - Reduce reset pulse timings

[Linus Walleij]

The data sheet for CY8CTMA340 specifies that the reset pulse
shall be at least 1 ms. Specify 1-2 ms with usleep_range()
to cut some slack for the scheduler.

Curiously the datasheet does not specify how long we have to
wait after a hard reset until the chip is up, but I have found
a vendor tree (Samsung GT-S7710) that has code for this touch
screen and there this is set to 5 ms so I use this with
the same 1 ms fuzz.

Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/input/touchscreen/cyttsp_core.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/input/touchscreen/cyttsp_core.c b/drivers/input/touchscreen/cyttsp_core.c
index 91f8f38b3f06..84f4f1c1a220 100644
--- a/drivers/input/touchscreen/cyttsp_core.c
+++ b/drivers/input/touchscreen/cyttsp_core.c
@@ -230,10 +230,16 @@ static int cyttsp_set_sysinfo_regs(struct cyttsp *ts)
 static void cyttsp_hard_reset(struct cyttsp *ts)
 {
 	if (ts->reset_gpio) {
+		/*
+		 * According to the CY8CTMA340 datasheet page 21, the external
+		 * reset pulse width should be >= 1 ms. The datasheet does not
+		 * specify how long we have to wait after reset but a vendor
+		 * tree specifies 5 ms here.
+		 */
 		gpiod_set_value_cansleep(ts->reset_gpio, 1);
-		msleep(CY_DELAY_DFLT);
+		usleep_range(1000, 2000);
 		gpiod_set_value_cansleep(ts->reset_gpio, 0);
-		msleep(CY_DELAY_DFLT);
+		usleep_range(5000, 6000);
 	}
 }
 
-- 
2.29.2

Re: [PATCH 4/7] Input: cyttsp - Reduce reset pulse timings

[Javier Martinez Canillas]

On Tue, Mar 30, 2021 at 10:54 AM Linus Walleij <linus.walleij@linaro.org> wrote:
>
> The data sheet for CY8CTMA340 specifies that the reset pulse
> shall be at least 1 ms. Specify 1-2 ms with usleep_range()
> to cut some slack for the scheduler.
>
> Curiously the datasheet does not specify how long we have to
> wait after a hard reset until the chip is up, but I have found
> a vendor tree (Samsung GT-S7710) that has code for this touch
> screen and there this is set to 5 ms so I use this with
> the same 1 ms fuzz.
>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---

Makes sense.

Reviewed-by: Javier Martinez Canillas <javier@dowhile0.org>

[PATCH 5/7] Input: cyttsp - Drop the phys path

[Linus Walleij]

When I test to use the CY8CTMA340 with PostmarketOS I don't
have any problem whatsoever in dropping this phys path,
it finds and uses the touchscreen just as well. I suppose
it is because userspace is using modern input libraries.

I challenge the maintainers to point out a valid and still
used userspace that actually need this. I say we drop it.

Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/input/touchscreen/cyttsp_core.c | 2 --
 drivers/input/touchscreen/cyttsp_core.h | 1 -
 2 files changed, 3 deletions(-)

diff --git a/drivers/input/touchscreen/cyttsp_core.c b/drivers/input/touchscreen/cyttsp_core.c
index 84f4f1c1a220..8ae9f00e5e31 100644
--- a/drivers/input/touchscreen/cyttsp_core.c
+++ b/drivers/input/touchscreen/cyttsp_core.c
@@ -655,10 +655,8 @@ struct cyttsp *cyttsp_probe(const struct cyttsp_bus_ops *bus_ops,
 		return ERR_PTR(error);
 
 	init_completion(&ts->bl_ready);
-	snprintf(ts->phys, sizeof(ts->phys), "%s/input0", dev_name(dev));
 
 	input_dev->name = "Cypress TTSP TouchScreen";
-	input_dev->phys = ts->phys;
 	input_dev->id.bustype = bus_ops->bustype;
 	input_dev->dev.parent = ts->dev;
 
diff --git a/drivers/input/touchscreen/cyttsp_core.h b/drivers/input/touchscreen/cyttsp_core.h
index c102a094e888..8eba9d8ba74a 100644
--- a/drivers/input/touchscreen/cyttsp_core.h
+++ b/drivers/input/touchscreen/cyttsp_core.h
@@ -115,7 +115,6 @@ struct cyttsp {
 	struct device *dev;
 	int irq;
 	struct input_dev *input;
-	char phys[32];
 	const struct cyttsp_bus_ops *bus_ops;
 	struct cyttsp_bootloader_data bl_data;
 	struct cyttsp_sysinfo_data sysinfo_data;
-- 
2.29.2

Re: [PATCH 5/7] Input: cyttsp - Drop the phys path

[Javier Martinez Canillas]

On Tue, Mar 30, 2021 at 10:54 AM Linus Walleij <linus.walleij@linaro.org> wrote:
>
> When I test to use the CY8CTMA340 with PostmarketOS I don't
> have any problem whatsoever in dropping this phys path,
> it finds and uses the touchscreen just as well. I suppose
> it is because userspace is using modern input libraries.
>
> I challenge the maintainers to point out a valid and still
> used userspace that actually need this. I say we drop it.
>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---

Agreed. Let's just drop it and if someone needs this then can bring it
back from the git history.

Reviewed-by: Javier Martinez Canillas <javier@dowhile0.org>

[PATCH 6/7] Input: cyttsp - Set abs params for ABS_MT_TOUCH_MAJOR

[Linus Walleij]

The driver is certainly reporting pressure in
cyttsp_report_tchdata() with
input_report_abs(input, ABS_MT_TOUCH_MAJOR, tch->z);
so we should also advertise this capability.

Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/input/touchscreen/cyttsp_core.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/input/touchscreen/cyttsp_core.c b/drivers/input/touchscreen/cyttsp_core.c
index 8ae9f00e5e31..ac412bcb15d8 100644
--- a/drivers/input/touchscreen/cyttsp_core.c
+++ b/drivers/input/touchscreen/cyttsp_core.c
@@ -667,6 +667,9 @@ struct cyttsp *cyttsp_probe(const struct cyttsp_bus_ops *bus_ops,
 
 	input_set_capability(input_dev, EV_ABS, ABS_MT_POSITION_X);
 	input_set_capability(input_dev, EV_ABS, ABS_MT_POSITION_Y);
+	/* One byte for width 0..255 so this is the limit */
+	input_set_abs_params(input_dev, ABS_MT_TOUCH_MAJOR, 0, 255, 0, 0);
+
 	touchscreen_parse_properties(input_dev, true, NULL);
 
 	error = input_mt_init_slots(input_dev, CY_MAX_ID, 0);
-- 
2.29.2

Re: [PATCH 6/7] Input: cyttsp - Set abs params for ABS_MT_TOUCH_MAJOR

[Javier Martinez Canillas]

On Tue, Mar 30, 2021 at 10:54 AM Linus Walleij <linus.walleij@linaro.org> wrote:
>
> The driver is certainly reporting pressure in
> cyttsp_report_tchdata() with
> input_report_abs(input, ABS_MT_TOUCH_MAJOR, tch->z);
> so we should also advertise this capability.
>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---

Reviewed-by: Javier Martinez Canillas <javier@dowhile0.org>

[PATCH 7/7] Input: cyttsp - Flag the device properly

[Linus Walleij]

This device is certainly a very simple touchscreen so
we set INPUT_MT_DIRECT.

The sibling driver for CY8CTMA140 also sets
INPUT_MT_DROP_UNUSED and experimenting with this driver
it clearly does not hurt: the touchscreen is working just
fine so let's set it for this one as well.

Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/input/touchscreen/cyttsp_core.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/input/touchscreen/cyttsp_core.c b/drivers/input/touchscreen/cyttsp_core.c
index ac412bcb15d8..fb71cd0d2070 100644
--- a/drivers/input/touchscreen/cyttsp_core.c
+++ b/drivers/input/touchscreen/cyttsp_core.c
@@ -672,7 +672,8 @@ struct cyttsp *cyttsp_probe(const struct cyttsp_bus_ops *bus_ops,
 
 	touchscreen_parse_properties(input_dev, true, NULL);
 
-	error = input_mt_init_slots(input_dev, CY_MAX_ID, 0);
+	error = input_mt_init_slots(input_dev, CY_MAX_ID,
+				    INPUT_MT_DIRECT | INPUT_MT_DROP_UNUSED);
 	if (error) {
 		dev_err(dev, "Unable to init MT slots.\n");
 		return ERR_PTR(error);
-- 
2.29.2

Re: [PATCH 7/7] Input: cyttsp - Flag the device properly

[Javier Martinez Canillas]

On Tue, Mar 30, 2021 at 10:54 AM Linus Walleij <linus.walleij@linaro.org> wrote:
>
> This device is certainly a very simple touchscreen so
> we set INPUT_MT_DIRECT.
>
> The sibling driver for CY8CTMA140 also sets
> INPUT_MT_DROP_UNUSED and experimenting with this driver
> it clearly does not hurt: the touchscreen is working just
> fine so let's set it for this one as well.
>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---

Reviewed-by: Javier Martinez Canillas <javier@dowhile0.org>