diff mbox series

[1/3] dt-bindings: input: sitronix-st1232: add compatible string for ST1633

Message ID 20190128084449.16070-1-martink@posteo.de
State Not Applicable, archived
Headers show
Series [1/3] dt-bindings: input: sitronix-st1232: add compatible string for ST1633 | expand

Checks

Context Check Description
robh/checkpatch success

Commit Message

Martin Kepplinger Jan. 28, 2019, 8:44 a.m. UTC 
From: Martin Kepplinger <martin.kepplinger@ginzinger.com>

The st1232 driver gains support for the ST1633 controller too; update
the bindings doc accordingly.

Signed-off-by: Martin Kepplinger <martin.kepplinger@ginzinger.com>
---
 .../bindings/input/touchscreen/sitronix-st1232.txt          | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Comments

Dmitry Torokhov Jan. 28, 2019, 7:03 p.m. UTC | #1 
Hi Martin,

On Mon, Jan 28, 2019 at 09:44:48AM +0100, Martin Kepplinger wrote:
> From: Martin Kepplinger <martin.kepplinger@ginzinger.com>
> 
> Add support for the Sitronix ST1633 touchscreen controller to the st1232
> driver. A protocol spec can be found here:
> www.ampdisplay.com/documents/pdf/AM-320480B6TZQW-TC0H.pdf
> 
> Signed-off-by: Martin Kepplinger <martin.kepplinger@ginzinger.com>
> ---
>  drivers/input/touchscreen/Kconfig  |   6 +-
>  drivers/input/touchscreen/st1232.c | 122 +++++++++++++++++++++--------
>  2 files changed, 94 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
> index 068dbbc610fc..7c597a49c265 100644
> --- a/drivers/input/touchscreen/Kconfig
> +++ b/drivers/input/touchscreen/Kconfig
> @@ -1168,11 +1168,11 @@ config TOUCHSCREEN_SIS_I2C
>  	  module will be called sis_i2c.
>  
>  config TOUCHSCREEN_ST1232
> -	tristate "Sitronix ST1232 touchscreen controllers"
> +	tristate "Sitronix ST1232 or ST1633 touchscreen controllers"
>  	depends on I2C
>  	help
> -	  Say Y here if you want to support Sitronix ST1232
> -	  touchscreen controller.
> +	  Say Y here if you want to support the Sitronix ST1232
> +	  or ST1633 touchscreen controller.
>  
>  	  If unsure, say N.
>  
> diff --git a/drivers/input/touchscreen/st1232.c b/drivers/input/touchscreen/st1232.c
> index 11ff32c68025..19a665d48dad 100644
> --- a/drivers/input/touchscreen/st1232.c
> +++ b/drivers/input/touchscreen/st1232.c
> @@ -23,13 +23,15 @@
>  #include <linux/types.h>
>  
>  #define ST1232_TS_NAME	"st1232-ts"
> +#define ST1633_TS_NAME	"st1633-ts"
> +
> +enum {
> +	st1232,
> +	st1633,
> +};

This enum seems no longer needed, I dropped it.

>  
>  #define MIN_X		0x00
>  #define MIN_Y		0x00

Given we no longer have constant MAX_X/Y I simply used 0 in
input_set_abs_params() and dropped MIN-X/Y.

> -#define MAX_X		0x31f	/* (800 - 1) */
> -#define MAX_Y		0x1df	/* (480 - 1) */
> -#define MAX_AREA	0xff
> -#define MAX_FINGERS	2
>  
>  struct st1232_ts_finger {
>  	u16 x;
> @@ -38,12 +40,24 @@ struct st1232_ts_finger {
>  	bool is_valid;
>  };
>  
> +struct st_chip_info {
> +	bool	have_z;
> +	u16	max_x;
> +	u16	max_y;
> +	u16	max_area;
> +	u16	max_fingers;
> +	u8	start_reg;
> +};
> +
>  struct st1232_ts_data {
>  	struct i2c_client *client;
>  	struct input_dev *input_dev;
> -	struct st1232_ts_finger finger[MAX_FINGERS];
>  	struct dev_pm_qos_request low_latency_req;
>  	int reset_gpio;

Could you please create an additional patch converting this to gpiod?
Instead of doing of_get_gpio()/gpio_is_valid()/devm_gpio_request() smply
do

	ts->reset_gpio = devm_gpiod_get_optional();
	if (IS_ERR(ts->reset_gpio)) {
		...
	}

and later

	if (ts->reset_gpio)
		...

Looking at the code it looks like reset is as usual active low, so you
will need to invert the logic as gpiod takes care of convertion logical
value to proper level (active low or high).

> +	const struct st_chip_info *chip_info;
> +	int read_buf_len;
> +	u8 *read_buf;
> +	struct st1232_ts_finger *finger;
>  };
>  
>  static int st1232_ts_read_data(struct st1232_ts_data *ts)
> @@ -52,40 +66,35 @@ static int st1232_ts_read_data(struct st1232_ts_data *ts)
>  	struct i2c_client *client = ts->client;
>  	struct i2c_msg msg[2];
>  	int error;
> -	u8 start_reg;
> -	u8 buf[10];
> +	int i, y;
> +	u8 start_reg = ts->chip_info->start_reg;
> +	u8 *buf = ts->read_buf;
>  
> -	/* read touchscreen data from ST1232 */
> +	/* read touchscreen data */
>  	msg[0].addr = client->addr;
>  	msg[0].flags = 0;
>  	msg[0].len = 1;
>  	msg[0].buf = &start_reg;
> -	start_reg = 0x10;
>  
>  	msg[1].addr = ts->client->addr;
>  	msg[1].flags = I2C_M_RD;
> -	msg[1].len = sizeof(buf);
> +	msg[1].len = ts->read_buf_len;
>  	msg[1].buf = buf;
>  
>  	error = i2c_transfer(client->adapter, msg, 2);
>  	if (error < 0)
>  		return error;
>  
> -	/* get "valid" bits */
> -	finger[0].is_valid = buf[2] >> 7;
> -	finger[1].is_valid = buf[5] >> 7;
> +	for (i = 0, y = 0; i < ts->chip_info->max_fingers; i++, y += 3) {
> +		finger[i].is_valid = buf[i + y] >> 7;
> +		if (finger[i].is_valid) {
> +			finger[i].x = ((buf[i + y] & 0x0070) << 4) | buf[i + 1];
> +			finger[i].y = ((buf[i + y] & 0x0007) << 8) | buf[i + 2];
>  
> -	/* get xy coordinate */
> -	if (finger[0].is_valid) {
> -		finger[0].x = ((buf[2] & 0x0070) << 4) | buf[3];
> -		finger[0].y = ((buf[2] & 0x0007) << 8) | buf[4];
> -		finger[0].t = buf[8];
> -	}
> -
> -	if (finger[1].is_valid) {
> -		finger[1].x = ((buf[5] & 0x0070) << 4) | buf[6];
> -		finger[1].y = ((buf[5] & 0x0007) << 8) | buf[7];
> -		finger[1].t = buf[9];
> +			/* st1232 includes a z-axis / touch strength */
> +			if (ts->chip_info->have_z)
> +				finger[i].t = buf[i + 6];
> +		}
>  	}
>  
>  	return 0;
> @@ -104,11 +113,14 @@ static irqreturn_t st1232_ts_irq_handler(int irq, void *dev_id)
>  		goto end;
>  
>  	/* multi touch protocol */
> -	for (i = 0; i < MAX_FINGERS; i++) {
> +	for (i = 0; i < ts->chip_info->max_fingers; i++) {
>  		if (!finger[i].is_valid)
>  			continue;
>  
> -		input_report_abs(input_dev, ABS_MT_TOUCH_MAJOR, finger[i].t);
> +		if (ts->chip_info->have_z)
> +			input_report_abs(input_dev, ABS_MT_TOUCH_MAJOR,
> +					 finger[i].t);
> +
>  		input_report_abs(input_dev, ABS_MT_POSITION_X, finger[i].x);
>  		input_report_abs(input_dev, ABS_MT_POSITION_Y, finger[i].y);
>  		input_mt_sync(input_dev);
> @@ -142,12 +154,40 @@ static void st1232_ts_power(struct st1232_ts_data *ts, bool poweron)
>  		gpio_direction_output(ts->reset_gpio, poweron);
>  }
>  
> +static const struct st_chip_info st1232_chip_info = {
> +		.have_z = true,
> +		.max_x = 0x31f, /* 800 - 1 */
> +		.max_y = 0x1df, /* 480 -1 */
> +		.max_area = 0xff,
> +		.max_fingers = 2,
> +		.start_reg = 0x12,
> +};
> +
> +static const struct st_chip_info st1633_chip_info = {
> +		.have_z = false,
> +		.max_x = 0x13f, /* 320 - 1 */
> +		.max_y = 0x1df, /* 480 -1 */
> +		.max_area = 0x00,
> +		.max_fingers = 5,
> +		.start_reg = 0x12,
> +};
> +
>  static int st1232_ts_probe(struct i2c_client *client,
>  			   const struct i2c_device_id *id)
>  {
>  	struct st1232_ts_data *ts;
> +	struct st1232_ts_finger *finger;
>  	struct input_dev *input_dev;
>  	int error;
> +	const struct st_chip_info *match = NULL;

There is no need to initialize with NULL given that we do unconditional
assignment below. I removed initialization.

> +
> +	match = device_get_match_data(&client->dev);
> +	if (!match && id)
> +		match = (const void *)id->driver_data;
> +	if (!match) {
> +		dev_err(&client->dev, "unknown device model\n");
> +		return -ENODEV;
> +	}
>  
>  	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
>  		dev_err(&client->dev, "need I2C_FUNC_I2C\n");
> @@ -163,6 +203,19 @@ static int st1232_ts_probe(struct i2c_client *client,
>  	if (!ts)
>  		return -ENOMEM;
>  
> +	ts->chip_info = match;
> +	ts->finger = devm_kzalloc(&client->dev,
> +				  sizeof(*finger) * ts->chip_info->max_fingers,
> +				  GFP_KERNEL);

I replaced it with devm_kcalloc(&client->dev,
				ts->chip_info->max_fingers, sizeof(*finger),
				GFP_KERNEL);

and applied.

> +	if (!ts->finger)
> +		return -ENOMEM;
> +
> +	/* allocate a buffer according to the number of registers to read */
> +	ts->read_buf_len = ts->chip_info->max_fingers * 4;
> +	ts->read_buf = devm_kzalloc(&client->dev, ts->read_buf_len, GFP_KERNEL);
> +	if (!ts->read_buf)
> +		return -ENOMEM;
> +
>  	input_dev = devm_input_allocate_device(&client->dev);
>  	if (!input_dev)
>  		return -ENOMEM;
> @@ -192,9 +245,14 @@ static int st1232_ts_probe(struct i2c_client *client,
>  	__set_bit(EV_KEY, input_dev->evbit);
>  	__set_bit(EV_ABS, input_dev->evbit);
>  
> -	input_set_abs_params(input_dev, ABS_MT_TOUCH_MAJOR, 0, MAX_AREA, 0, 0);
> -	input_set_abs_params(input_dev, ABS_MT_POSITION_X, MIN_X, MAX_X, 0, 0);
> -	input_set_abs_params(input_dev, ABS_MT_POSITION_Y, MIN_Y, MAX_Y, 0, 0);
> +	if (ts->chip_info->have_z)
> +		input_set_abs_params(input_dev, ABS_MT_TOUCH_MAJOR, 0,
> +				     ts->chip_info->max_area, 0, 0);
> +
> +	input_set_abs_params(input_dev, ABS_MT_POSITION_X,
> +			     MIN_X, ts->chip_info->max_x, 0, 0);
> +	input_set_abs_params(input_dev, ABS_MT_POSITION_Y,
> +			     MIN_Y, ts->chip_info->max_y, 0, 0);
>  
>  	error = devm_request_threaded_irq(&client->dev, client->irq,
>  					  NULL, st1232_ts_irq_handler,
> @@ -261,13 +319,15 @@ static SIMPLE_DEV_PM_OPS(st1232_ts_pm_ops,
>  			 st1232_ts_suspend, st1232_ts_resume);
>  
>  static const struct i2c_device_id st1232_ts_id[] = {
> -	{ ST1232_TS_NAME, 0 },
> +	{ ST1232_TS_NAME, (unsigned long)&st1232_chip_info },
> +	{ ST1633_TS_NAME, (unsigned long)&st1633_chip_info },
>  	{ }
>  };
>  MODULE_DEVICE_TABLE(i2c, st1232_ts_id);
>  
>  static const struct of_device_id st1232_ts_dt_ids[] = {
> -	{ .compatible = "sitronix,st1232", },
> +	{ .compatible = "sitronix,st1232", .data = &st1232_chip_info },
> +	{ .compatible = "sitronix,st1633", .data = &st1633_chip_info },
>  	{ }
>  };
>  MODULE_DEVICE_TABLE(of, st1232_ts_dt_ids);
> -- 
> 2.20.1
> 

Thanks.
Dmitry Torokhov Jan. 28, 2019, 7:04 p.m. UTC | #2 
On Mon, Jan 28, 2019 at 09:44:49AM +0100, Martin Kepplinger wrote:
> From: Martin Kepplinger <martin.kepplinger@ginzinger.com>
> 
> This adds myself as an author of the st1232 driver module as Tony's
> email address doesn't seem to work anymore.
> 
> Signed-off-by: Martin Kepplinger <martin.kepplinger@ginzinger.com>

Applied, thank you.

> ---
>  drivers/input/touchscreen/st1232.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/input/touchscreen/st1232.c b/drivers/input/touchscreen/st1232.c
> index 19a665d48dad..906b233970aa 100644
> --- a/drivers/input/touchscreen/st1232.c
> +++ b/drivers/input/touchscreen/st1232.c
> @@ -346,5 +346,6 @@ static struct i2c_driver st1232_ts_driver = {
>  module_i2c_driver(st1232_ts_driver);
>  
>  MODULE_AUTHOR("Tony SIM <chinyeow.sim.xt@renesas.com>");
> +MODULE_AUTHOR("Martin Kepplinger <martin.kepplinger@ginzinger.com>");
>  MODULE_DESCRIPTION("SITRONIX ST1232 Touchscreen Controller Driver");
>  MODULE_LICENSE("GPL v2");
> -- 
> 2.20.1
>
Martin Kepplinger Jan. 28, 2019, 7:10 p.m. UTC | #3 
Am 28.01.2019 20:03 schrieb Dmitry Torokhov:
> Hi Martin,
> 
> On Mon, Jan 28, 2019 at 09:44:48AM +0100, Martin Kepplinger wrote:
>> From: Martin Kepplinger <martin.kepplinger@ginzinger.com>
>> 
>> Add support for the Sitronix ST1633 touchscreen controller to the 
>> st1232
>> driver. A protocol spec can be found here:
>> www.ampdisplay.com/documents/pdf/AM-320480B6TZQW-TC0H.pdf
>> 
>> Signed-off-by: Martin Kepplinger <martin.kepplinger@ginzinger.com>
>> ---
>>  drivers/input/touchscreen/Kconfig  |   6 +-
>>  drivers/input/touchscreen/st1232.c | 122 
>> +++++++++++++++++++++--------
>>  2 files changed, 94 insertions(+), 34 deletions(-)
>> 
>> diff --git a/drivers/input/touchscreen/Kconfig 
>> b/drivers/input/touchscreen/Kconfig
>> index 068dbbc610fc..7c597a49c265 100644
>> --- a/drivers/input/touchscreen/Kconfig
>> +++ b/drivers/input/touchscreen/Kconfig
>> @@ -1168,11 +1168,11 @@ config TOUCHSCREEN_SIS_I2C
>>  	  module will be called sis_i2c.
>> 
>>  config TOUCHSCREEN_ST1232
>> -	tristate "Sitronix ST1232 touchscreen controllers"
>> +	tristate "Sitronix ST1232 or ST1633 touchscreen controllers"
>>  	depends on I2C
>>  	help
>> -	  Say Y here if you want to support Sitronix ST1232
>> -	  touchscreen controller.
>> +	  Say Y here if you want to support the Sitronix ST1232
>> +	  or ST1633 touchscreen controller.
>> 
>>  	  If unsure, say N.
>> 
>> diff --git a/drivers/input/touchscreen/st1232.c 
>> b/drivers/input/touchscreen/st1232.c
>> index 11ff32c68025..19a665d48dad 100644
>> --- a/drivers/input/touchscreen/st1232.c
>> +++ b/drivers/input/touchscreen/st1232.c
>> @@ -23,13 +23,15 @@
>>  #include <linux/types.h>
>> 
>>  #define ST1232_TS_NAME	"st1232-ts"
>> +#define ST1633_TS_NAME	"st1633-ts"
>> +
>> +enum {
>> +	st1232,
>> +	st1633,
>> +};
> 
> This enum seems no longer needed, I dropped it.
> 
>> 
>>  #define MIN_X		0x00
>>  #define MIN_Y		0x00
> 
> Given we no longer have constant MAX_X/Y I simply used 0 in
> input_set_abs_params() and dropped MIN-X/Y.
> 
>> -#define MAX_X		0x31f	/* (800 - 1) */
>> -#define MAX_Y		0x1df	/* (480 - 1) */
>> -#define MAX_AREA	0xff
>> -#define MAX_FINGERS	2
>> 
>>  struct st1232_ts_finger {
>>  	u16 x;
>> @@ -38,12 +40,24 @@ struct st1232_ts_finger {
>>  	bool is_valid;
>>  };
>> 
>> +struct st_chip_info {
>> +	bool	have_z;
>> +	u16	max_x;
>> +	u16	max_y;
>> +	u16	max_area;
>> +	u16	max_fingers;
>> +	u8	start_reg;
>> +};
>> +
>>  struct st1232_ts_data {
>>  	struct i2c_client *client;
>>  	struct input_dev *input_dev;
>> -	struct st1232_ts_finger finger[MAX_FINGERS];
>>  	struct dev_pm_qos_request low_latency_req;
>>  	int reset_gpio;
> 
> Could you please create an additional patch converting this to gpiod?
> Instead of doing of_get_gpio()/gpio_is_valid()/devm_gpio_request() 
> smply
> do
> 
> 	ts->reset_gpio = devm_gpiod_get_optional();
> 	if (IS_ERR(ts->reset_gpio)) {
> 		...
> 	}
> 
> and later
> 
> 	if (ts->reset_gpio)
> 		...
> 
> Looking at the code it looks like reset is as usual active low, so you
> will need to invert the logic as gpiod takes care of convertion logical
> value to proper level (active low or high).

I'll test your applied changes and get back to this tomorrow.

thanks.

> 
>> +	const struct st_chip_info *chip_info;
>> +	int read_buf_len;
>> +	u8 *read_buf;
>> +	struct st1232_ts_finger *finger;
>>  };
>> 
>>  static int st1232_ts_read_data(struct st1232_ts_data *ts)
>> @@ -52,40 +66,35 @@ static int st1232_ts_read_data(struct 
>> st1232_ts_data *ts)
>>  	struct i2c_client *client = ts->client;
>>  	struct i2c_msg msg[2];
>>  	int error;
>> -	u8 start_reg;
>> -	u8 buf[10];
>> +	int i, y;
>> +	u8 start_reg = ts->chip_info->start_reg;
>> +	u8 *buf = ts->read_buf;
>> 
>> -	/* read touchscreen data from ST1232 */
>> +	/* read touchscreen data */
>>  	msg[0].addr = client->addr;
>>  	msg[0].flags = 0;
>>  	msg[0].len = 1;
>>  	msg[0].buf = &start_reg;
>> -	start_reg = 0x10;
>> 
>>  	msg[1].addr = ts->client->addr;
>>  	msg[1].flags = I2C_M_RD;
>> -	msg[1].len = sizeof(buf);
>> +	msg[1].len = ts->read_buf_len;
>>  	msg[1].buf = buf;
>> 
>>  	error = i2c_transfer(client->adapter, msg, 2);
>>  	if (error < 0)
>>  		return error;
>> 
>> -	/* get "valid" bits */
>> -	finger[0].is_valid = buf[2] >> 7;
>> -	finger[1].is_valid = buf[5] >> 7;
>> +	for (i = 0, y = 0; i < ts->chip_info->max_fingers; i++, y += 3) {
>> +		finger[i].is_valid = buf[i + y] >> 7;
>> +		if (finger[i].is_valid) {
>> +			finger[i].x = ((buf[i + y] & 0x0070) << 4) | buf[i + 1];
>> +			finger[i].y = ((buf[i + y] & 0x0007) << 8) | buf[i + 2];
>> 
>> -	/* get xy coordinate */
>> -	if (finger[0].is_valid) {
>> -		finger[0].x = ((buf[2] & 0x0070) << 4) | buf[3];
>> -		finger[0].y = ((buf[2] & 0x0007) << 8) | buf[4];
>> -		finger[0].t = buf[8];
>> -	}
>> -
>> -	if (finger[1].is_valid) {
>> -		finger[1].x = ((buf[5] & 0x0070) << 4) | buf[6];
>> -		finger[1].y = ((buf[5] & 0x0007) << 8) | buf[7];
>> -		finger[1].t = buf[9];
>> +			/* st1232 includes a z-axis / touch strength */
>> +			if (ts->chip_info->have_z)
>> +				finger[i].t = buf[i + 6];
>> +		}
>>  	}
>> 
>>  	return 0;
>> @@ -104,11 +113,14 @@ static irqreturn_t st1232_ts_irq_handler(int 
>> irq, void *dev_id)
>>  		goto end;
>> 
>>  	/* multi touch protocol */
>> -	for (i = 0; i < MAX_FINGERS; i++) {
>> +	for (i = 0; i < ts->chip_info->max_fingers; i++) {
>>  		if (!finger[i].is_valid)
>>  			continue;
>> 
>> -		input_report_abs(input_dev, ABS_MT_TOUCH_MAJOR, finger[i].t);
>> +		if (ts->chip_info->have_z)
>> +			input_report_abs(input_dev, ABS_MT_TOUCH_MAJOR,
>> +					 finger[i].t);
>> +
>>  		input_report_abs(input_dev, ABS_MT_POSITION_X, finger[i].x);
>>  		input_report_abs(input_dev, ABS_MT_POSITION_Y, finger[i].y);
>>  		input_mt_sync(input_dev);
>> @@ -142,12 +154,40 @@ static void st1232_ts_power(struct 
>> st1232_ts_data *ts, bool poweron)
>>  		gpio_direction_output(ts->reset_gpio, poweron);
>>  }
>> 
>> +static const struct st_chip_info st1232_chip_info = {
>> +		.have_z = true,
>> +		.max_x = 0x31f, /* 800 - 1 */
>> +		.max_y = 0x1df, /* 480 -1 */
>> +		.max_area = 0xff,
>> +		.max_fingers = 2,
>> +		.start_reg = 0x12,
>> +};
>> +
>> +static const struct st_chip_info st1633_chip_info = {
>> +		.have_z = false,
>> +		.max_x = 0x13f, /* 320 - 1 */
>> +		.max_y = 0x1df, /* 480 -1 */
>> +		.max_area = 0x00,
>> +		.max_fingers = 5,
>> +		.start_reg = 0x12,
>> +};
>> +
>>  static int st1232_ts_probe(struct i2c_client *client,
>>  			   const struct i2c_device_id *id)
>>  {
>>  	struct st1232_ts_data *ts;
>> +	struct st1232_ts_finger *finger;
>>  	struct input_dev *input_dev;
>>  	int error;
>> +	const struct st_chip_info *match = NULL;
> 
> There is no need to initialize with NULL given that we do unconditional
> assignment below. I removed initialization.
> 
>> +
>> +	match = device_get_match_data(&client->dev);
>> +	if (!match && id)
>> +		match = (const void *)id->driver_data;
>> +	if (!match) {
>> +		dev_err(&client->dev, "unknown device model\n");
>> +		return -ENODEV;
>> +	}
>> 
>>  	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
>>  		dev_err(&client->dev, "need I2C_FUNC_I2C\n");
>> @@ -163,6 +203,19 @@ static int st1232_ts_probe(struct i2c_client 
>> *client,
>>  	if (!ts)
>>  		return -ENOMEM;
>> 
>> +	ts->chip_info = match;
>> +	ts->finger = devm_kzalloc(&client->dev,
>> +				  sizeof(*finger) * ts->chip_info->max_fingers,
>> +				  GFP_KERNEL);
> 
> I replaced it with devm_kcalloc(&client->dev,
> 				ts->chip_info->max_fingers, sizeof(*finger),
> 				GFP_KERNEL);
> 
> and applied.
> 
>> +	if (!ts->finger)
>> +		return -ENOMEM;
>> +
>> +	/* allocate a buffer according to the number of registers to read */
>> +	ts->read_buf_len = ts->chip_info->max_fingers * 4;
>> +	ts->read_buf = devm_kzalloc(&client->dev, ts->read_buf_len, 
>> GFP_KERNEL);
>> +	if (!ts->read_buf)
>> +		return -ENOMEM;
>> +
>>  	input_dev = devm_input_allocate_device(&client->dev);
>>  	if (!input_dev)
>>  		return -ENOMEM;
>> @@ -192,9 +245,14 @@ static int st1232_ts_probe(struct i2c_client 
>> *client,
>>  	__set_bit(EV_KEY, input_dev->evbit);
>>  	__set_bit(EV_ABS, input_dev->evbit);
>> 
>> -	input_set_abs_params(input_dev, ABS_MT_TOUCH_MAJOR, 0, MAX_AREA, 0, 
>> 0);
>> -	input_set_abs_params(input_dev, ABS_MT_POSITION_X, MIN_X, MAX_X, 0, 
>> 0);
>> -	input_set_abs_params(input_dev, ABS_MT_POSITION_Y, MIN_Y, MAX_Y, 0, 
>> 0);
>> +	if (ts->chip_info->have_z)
>> +		input_set_abs_params(input_dev, ABS_MT_TOUCH_MAJOR, 0,
>> +				     ts->chip_info->max_area, 0, 0);
>> +
>> +	input_set_abs_params(input_dev, ABS_MT_POSITION_X,
>> +			     MIN_X, ts->chip_info->max_x, 0, 0);
>> +	input_set_abs_params(input_dev, ABS_MT_POSITION_Y,
>> +			     MIN_Y, ts->chip_info->max_y, 0, 0);
>> 
>>  	error = devm_request_threaded_irq(&client->dev, client->irq,
>>  					  NULL, st1232_ts_irq_handler,
>> @@ -261,13 +319,15 @@ static SIMPLE_DEV_PM_OPS(st1232_ts_pm_ops,
>>  			 st1232_ts_suspend, st1232_ts_resume);
>> 
>>  static const struct i2c_device_id st1232_ts_id[] = {
>> -	{ ST1232_TS_NAME, 0 },
>> +	{ ST1232_TS_NAME, (unsigned long)&st1232_chip_info },
>> +	{ ST1633_TS_NAME, (unsigned long)&st1633_chip_info },
>>  	{ }
>>  };
>>  MODULE_DEVICE_TABLE(i2c, st1232_ts_id);
>> 
>>  static const struct of_device_id st1232_ts_dt_ids[] = {
>> -	{ .compatible = "sitronix,st1232", },
>> +	{ .compatible = "sitronix,st1232", .data = &st1232_chip_info },
>> +	{ .compatible = "sitronix,st1633", .data = &st1633_chip_info },
>>  	{ }
>>  };
>>  MODULE_DEVICE_TABLE(of, st1232_ts_dt_ids);
>> --
>> 2.20.1
>> 
> 
> Thanks.
Matthias Fend Jan. 29, 2019, 7:07 a.m. UTC | #4 
Hi Martin,


Matthias Fend
R&D Electronics 

Wolfvision GmbH
Oberes Ried 14 | 6833 Klaus | Austria 
Tel: +43 5523 52250 | Mail: Matthias.Fend@wolfvision.net 

Webpage: www.wolfvision.com | www.wolfvision.com/green
Firmenbuch / Commercial Register: FN283521v Feldkirch/Austria

> -----Ursprüngliche Nachricht-----
> Von: linux-input-owner@vger.kernel.org <linux-input-
> owner@vger.kernel.org> Im Auftrag von Martin Kepplinger
> Gesendet: Montag, 28. Jänner 2019 20:10
> An: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Cc: devicetree@vger.kernel.org; linux-input@vger.kernel.org;
> robh+dt@kernel.org; mark.rutland@arm.com; linux-kernel@vger.kernel.org;
> Martin Kepplinger <martin.kepplinger@ginzinger.com>
> Betreff: Re: [PATCH 2/3] Input: st1232 - add support for st1633
> 
> Am 28.01.2019 20:03 schrieb Dmitry Torokhov:
> > Hi Martin,
> >
> > On Mon, Jan 28, 2019 at 09:44:48AM +0100, Martin Kepplinger wrote:
> >> From: Martin Kepplinger <martin.kepplinger@ginzinger.com>
> >>
> >> Add support for the Sitronix ST1633 touchscreen controller to the
> >> st1232
> >> driver. A protocol spec can be found here:
> >>
> https://emea01.safelinks.protection.outlook.com/?url=www.ampdisplay.co
> m%2Fdocuments%2Fpdf%2FAM-320480B6TZQW-
> TC0H.pdf&amp;data=01%7C01%7CMatthias.Fend%40wolfvision.net%7C4f9d
> 56475f674b1e3b4008d685543b35%7Ce94ec9da9183471e83b351baa8eb804f%
> 7C0&amp;sdata=YTw33BPKKpdN%2BBFrufVk8e4YqXOzR6VQKuzRMOjcwWk
> %3D&amp;reserved=0
> >>
> >> Signed-off-by: Martin Kepplinger <martin.kepplinger@ginzinger.com>
> >> ---
> >>  drivers/input/touchscreen/Kconfig  |   6 +-
> >>  drivers/input/touchscreen/st1232.c | 122
> >> +++++++++++++++++++++--------
> >>  2 files changed, 94 insertions(+), 34 deletions(-)
> >>
> >> diff --git a/drivers/input/touchscreen/Kconfig
> >> b/drivers/input/touchscreen/Kconfig
> >> index 068dbbc610fc..7c597a49c265 100644
> >> --- a/drivers/input/touchscreen/Kconfig
> >> +++ b/drivers/input/touchscreen/Kconfig
> >> @@ -1168,11 +1168,11 @@ config TOUCHSCREEN_SIS_I2C
> >>  	  module will be called sis_i2c.
> >>
> >>  config TOUCHSCREEN_ST1232
> >> -	tristate "Sitronix ST1232 touchscreen controllers"
> >> +	tristate "Sitronix ST1232 or ST1633 touchscreen controllers"
> >>  	depends on I2C
> >>  	help
> >> -	  Say Y here if you want to support Sitronix ST1232
> >> -	  touchscreen controller.
> >> +	  Say Y here if you want to support the Sitronix ST1232
> >> +	  or ST1633 touchscreen controller.
> >>
> >>  	  If unsure, say N.
> >>
> >> diff --git a/drivers/input/touchscreen/st1232.c
> >> b/drivers/input/touchscreen/st1232.c
> >> index 11ff32c68025..19a665d48dad 100644
> >> --- a/drivers/input/touchscreen/st1232.c
> >> +++ b/drivers/input/touchscreen/st1232.c
> >> @@ -23,13 +23,15 @@
> >>  #include <linux/types.h>
> >>
> >>  #define ST1232_TS_NAME	"st1232-ts"
> >> +#define ST1633_TS_NAME	"st1633-ts"
> >> +
> >> +enum {
> >> +	st1232,
> >> +	st1633,
> >> +};
> >
> > This enum seems no longer needed, I dropped it.
> >
> >>
> >>  #define MIN_X		0x00
> >>  #define MIN_Y		0x00
> >
> > Given we no longer have constant MAX_X/Y I simply used 0 in
> > input_set_abs_params() and dropped MIN-X/Y.
> >
> >> -#define MAX_X		0x31f	/* (800 - 1) */
> >> -#define MAX_Y		0x1df	/* (480 - 1) */
> >> -#define MAX_AREA	0xff
> >> -#define MAX_FINGERS	2
> >>
> >>  struct st1232_ts_finger {
> >>  	u16 x;
> >> @@ -38,12 +40,24 @@ struct st1232_ts_finger {
> >>  	bool is_valid;
> >>  };
> >>
> >> +struct st_chip_info {
> >> +	bool	have_z;
> >> +	u16	max_x;
> >> +	u16	max_y;
> >> +	u16	max_area;
> >> +	u16	max_fingers;
> >> +	u8	start_reg;
> >> +};
> >> +
> >>  struct st1232_ts_data {
> >>  	struct i2c_client *client;
> >>  	struct input_dev *input_dev;
> >> -	struct st1232_ts_finger finger[MAX_FINGERS];
> >>  	struct dev_pm_qos_request low_latency_req;
> >>  	int reset_gpio;
> >
> > Could you please create an additional patch converting this to gpiod?
> > Instead of doing of_get_gpio()/gpio_is_valid()/devm_gpio_request()
> > smply
> > do
> >
> > 	ts->reset_gpio = devm_gpiod_get_optional();
> > 	if (IS_ERR(ts->reset_gpio)) {
> > 		...
> > 	}
> >
> > and later
> >
> > 	if (ts->reset_gpio)
> > 		...
> >
> > Looking at the code it looks like reset is as usual active low, so you
> > will need to invert the logic as gpiod takes care of convertion logical
> > value to proper level (active low or high).
> 
> I'll test your applied changes and get back to this tomorrow.
> 
> thanks.
> 
> >
> >> +	const struct st_chip_info *chip_info;
> >> +	int read_buf_len;
> >> +	u8 *read_buf;
> >> +	struct st1232_ts_finger *finger;
> >>  };
> >>
> >>  static int st1232_ts_read_data(struct st1232_ts_data *ts)
> >> @@ -52,40 +66,35 @@ static int st1232_ts_read_data(struct
> >> st1232_ts_data *ts)
> >>  	struct i2c_client *client = ts->client;
> >>  	struct i2c_msg msg[2];
> >>  	int error;
> >> -	u8 start_reg;
> >> -	u8 buf[10];
> >> +	int i, y;
> >> +	u8 start_reg = ts->chip_info->start_reg;
> >> +	u8 *buf = ts->read_buf;
> >>
> >> -	/* read touchscreen data from ST1232 */
> >> +	/* read touchscreen data */
> >>  	msg[0].addr = client->addr;
> >>  	msg[0].flags = 0;
> >>  	msg[0].len = 1;
> >>  	msg[0].buf = &start_reg;
> >> -	start_reg = 0x10;
> >>
> >>  	msg[1].addr = ts->client->addr;
> >>  	msg[1].flags = I2C_M_RD;
> >> -	msg[1].len = sizeof(buf);
> >> +	msg[1].len = ts->read_buf_len;
> >>  	msg[1].buf = buf;
> >>
> >>  	error = i2c_transfer(client->adapter, msg, 2);
> >>  	if (error < 0)
> >>  		return error;
> >>
> >> -	/* get "valid" bits */
> >> -	finger[0].is_valid = buf[2] >> 7;
> >> -	finger[1].is_valid = buf[5] >> 7;
> >> +	for (i = 0, y = 0; i < ts->chip_info->max_fingers; i++, y += 3) {
> >> +		finger[i].is_valid = buf[i + y] >> 7;
> >> +		if (finger[i].is_valid) {
> >> +			finger[i].x = ((buf[i + y] & 0x0070) << 4) | buf[i + 1];
> >> +			finger[i].y = ((buf[i + y] & 0x0007) << 8) | buf[i + 2];
> >>
> >> -	/* get xy coordinate */
> >> -	if (finger[0].is_valid) {
> >> -		finger[0].x = ((buf[2] & 0x0070) << 4) | buf[3];
> >> -		finger[0].y = ((buf[2] & 0x0007) << 8) | buf[4];
> >> -		finger[0].t = buf[8];
> >> -	}
> >> -
> >> -	if (finger[1].is_valid) {
> >> -		finger[1].x = ((buf[5] & 0x0070) << 4) | buf[6];
> >> -		finger[1].y = ((buf[5] & 0x0007) << 8) | buf[7];
> >> -		finger[1].t = buf[9];
> >> +			/* st1232 includes a z-axis / touch strength */
> >> +			if (ts->chip_info->have_z)
> >> +				finger[i].t = buf[i + 6];
> >> +		}
> >>  	}
> >>
> >>  	return 0;
> >> @@ -104,11 +113,14 @@ static irqreturn_t st1232_ts_irq_handler(int
> >> irq, void *dev_id)
> >>  		goto end;
> >>
> >>  	/* multi touch protocol */
> >> -	for (i = 0; i < MAX_FINGERS; i++) {
> >> +	for (i = 0; i < ts->chip_info->max_fingers; i++) {
> >>  		if (!finger[i].is_valid)
> >>  			continue;
> >>
> >> -		input_report_abs(input_dev, ABS_MT_TOUCH_MAJOR,
> finger[i].t);
> >> +		if (ts->chip_info->have_z)
> >> +			input_report_abs(input_dev,
> ABS_MT_TOUCH_MAJOR,
> >> +					 finger[i].t);
> >> +
> >>  		input_report_abs(input_dev, ABS_MT_POSITION_X,
> finger[i].x);
> >>  		input_report_abs(input_dev, ABS_MT_POSITION_Y,
> finger[i].y);
> >>  		input_mt_sync(input_dev);
> >> @@ -142,12 +154,40 @@ static void st1232_ts_power(struct
> >> st1232_ts_data *ts, bool poweron)
> >>  		gpio_direction_output(ts->reset_gpio, poweron);
> >>  }
> >>
> >> +static const struct st_chip_info st1232_chip_info = {
> >> +		.have_z = true,
> >> +		.max_x = 0x31f, /* 800 - 1 */
> >> +		.max_y = 0x1df, /* 480 -1 */
> >> +		.max_area = 0xff,
> >> +		.max_fingers = 2,
> >> +		.start_reg = 0x12,
> >> +};
> >> +
> >> +static const struct st_chip_info st1633_chip_info = {
> >> +		.have_z = false,
> >> +		.max_x = 0x13f, /* 320 - 1 */
> >> +		.max_y = 0x1df, /* 480 -1 */

I guess these values are the dimensions of the referenced TFT display and not any limits of the touch controller. I wonder which values are correct here?
Maybe it would also be easier to just use the decimal representation and drop the comment, what do you think?

Thanks,
 ~Matthias

> >> +		.max_area = 0x00,
> >> +		.max_fingers = 5,
> >> +		.start_reg = 0x12,
> >> +};
> >> +
> >>  static int st1232_ts_probe(struct i2c_client *client,
> >>  			   const struct i2c_device_id *id)
> >>  {
> >>  	struct st1232_ts_data *ts;
> >> +	struct st1232_ts_finger *finger;
> >>  	struct input_dev *input_dev;
> >>  	int error;
> >> +	const struct st_chip_info *match = NULL;
> >
> > There is no need to initialize with NULL given that we do unconditional
> > assignment below. I removed initialization.
> >
> >> +
> >> +	match = device_get_match_data(&client->dev);
> >> +	if (!match && id)
> >> +		match = (const void *)id->driver_data;
> >> +	if (!match) {
> >> +		dev_err(&client->dev, "unknown device model\n");
> >> +		return -ENODEV;
> >> +	}
> >>
> >>  	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
> >>  		dev_err(&client->dev, "need I2C_FUNC_I2C\n");
> >> @@ -163,6 +203,19 @@ static int st1232_ts_probe(struct i2c_client
> >> *client,
> >>  	if (!ts)
> >>  		return -ENOMEM;
> >>
> >> +	ts->chip_info = match;
> >> +	ts->finger = devm_kzalloc(&client->dev,
> >> +				  sizeof(*finger) * ts->chip_info-
> >max_fingers,
> >> +				  GFP_KERNEL);
> >
> > I replaced it with devm_kcalloc(&client->dev,
> > 				ts->chip_info->max_fingers, sizeof(*finger),
> > 				GFP_KERNEL);
> >
> > and applied.
> >
> >> +	if (!ts->finger)
> >> +		return -ENOMEM;
> >> +
> >> +	/* allocate a buffer according to the number of registers to read */
> >> +	ts->read_buf_len = ts->chip_info->max_fingers * 4;
> >> +	ts->read_buf = devm_kzalloc(&client->dev, ts->read_buf_len,
> >> GFP_KERNEL);
> >> +	if (!ts->read_buf)
> >> +		return -ENOMEM;
> >> +
> >>  	input_dev = devm_input_allocate_device(&client->dev);
> >>  	if (!input_dev)
> >>  		return -ENOMEM;
> >> @@ -192,9 +245,14 @@ static int st1232_ts_probe(struct i2c_client
> >> *client,
> >>  	__set_bit(EV_KEY, input_dev->evbit);
> >>  	__set_bit(EV_ABS, input_dev->evbit);
> >>
> >> -	input_set_abs_params(input_dev, ABS_MT_TOUCH_MAJOR, 0,
> MAX_AREA, 0,
> >> 0);
> >> -	input_set_abs_params(input_dev, ABS_MT_POSITION_X, MIN_X,
> MAX_X, 0,
> >> 0);
> >> -	input_set_abs_params(input_dev, ABS_MT_POSITION_Y, MIN_Y,
> MAX_Y, 0,
> >> 0);
> >> +	if (ts->chip_info->have_z)
> >> +		input_set_abs_params(input_dev,
> ABS_MT_TOUCH_MAJOR, 0,
> >> +				     ts->chip_info->max_area, 0, 0);
> >> +
> >> +	input_set_abs_params(input_dev, ABS_MT_POSITION_X,
> >> +			     MIN_X, ts->chip_info->max_x, 0, 0);
> >> +	input_set_abs_params(input_dev, ABS_MT_POSITION_Y,
> >> +			     MIN_Y, ts->chip_info->max_y, 0, 0);
> >>
> >>  	error = devm_request_threaded_irq(&client->dev, client->irq,
> >>  					  NULL, st1232_ts_irq_handler,
> >> @@ -261,13 +319,15 @@ static
> SIMPLE_DEV_PM_OPS(st1232_ts_pm_ops,
> >>  			 st1232_ts_suspend, st1232_ts_resume);
> >>
> >>  static const struct i2c_device_id st1232_ts_id[] = {
> >> -	{ ST1232_TS_NAME, 0 },
> >> +	{ ST1232_TS_NAME, (unsigned long)&st1232_chip_info },
> >> +	{ ST1633_TS_NAME, (unsigned long)&st1633_chip_info },
> >>  	{ }
> >>  };
> >>  MODULE_DEVICE_TABLE(i2c, st1232_ts_id);
> >>
> >>  static const struct of_device_id st1232_ts_dt_ids[] = {
> >> -	{ .compatible = "sitronix,st1232", },
> >> +	{ .compatible = "sitronix,st1232", .data = &st1232_chip_info },
> >> +	{ .compatible = "sitronix,st1633", .data = &st1633_chip_info },
> >>  	{ }
> >>  };
> >>  MODULE_DEVICE_TABLE(of, st1232_ts_dt_ids);
> >> --
> >> 2.20.1
> >>
> >
> > Thanks.
Rob Herring (Arm) Feb. 23, 2019, 12:43 a.m. UTC | #5 
On Mon, 28 Jan 2019 09:44:47 +0100, Martin Kepplinger wrote:
> From: Martin Kepplinger <martin.kepplinger@ginzinger.com>
> 
> The st1232 driver gains support for the ST1633 controller too; update
> the bindings doc accordingly.
> 
> Signed-off-by: Martin Kepplinger <martin.kepplinger@ginzinger.com>
> ---
>  .../bindings/input/touchscreen/sitronix-st1232.txt          | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 

Reviewed-by: Rob Herring <robh@kernel.org>
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/input/touchscreen/sitronix-st1232.txt b/Documentation/devicetree/bindings/input/touchscreen/sitronix-st1232.txt
index 64ad48b824a2..e73e826e0f2a 100644
--- a/Documentation/devicetree/bindings/input/touchscreen/sitronix-st1232.txt
+++ b/Documentation/devicetree/bindings/input/touchscreen/sitronix-st1232.txt
@@ -1,7 +1,9 @@ 
-* Sitronix st1232 touchscreen controller
+* Sitronix st1232 or st1633 touchscreen controller
 
 Required properties:
-- compatible: must be "sitronix,st1232"
+- compatible: must contain one of
+  * "sitronix,st1232"
+  * "sitronix,st1633"
 - reg: I2C address of the chip
 - interrupts: interrupt to which the chip is connected