Hi Martin, Matthias Fend R&D Electronics Wolfvision GmbH Oberes Ried 14 | 6833 Klaus | Austria Tel: +43 5523 52250 | Mail: Matthias.Fend@xxxxxxxxxxxxxx Webpage: www.wolfvision.com | www.wolfvision.com/green Firmenbuch / Commercial Register: FN283521v Feldkirch/Austria > -----Ursprüngliche Nachricht----- > Von: linux-input-owner@xxxxxxxxxxxxxxx <linux-input- > owner@xxxxxxxxxxxxxxx> Im Auftrag von Martin Kepplinger > Gesendet: Montag, 28. Jänner 2019 20:10 > An: Dmitry Torokhov <dmitry.torokhov@xxxxxxxxx> > Cc: devicetree@xxxxxxxxxxxxxxx; linux-input@xxxxxxxxxxxxxxx; > robh+dt@xxxxxxxxxx; mark.rutland@xxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; > Martin Kepplinger <martin.kepplinger@xxxxxxxxxxxxx> > 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@xxxxxxxxxxxxx> > >> > >> 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&data=01%7C01%7CMatthias.Fend%40wolfvision.net%7C4f9d > 56475f674b1e3b4008d685543b35%7Ce94ec9da9183471e83b351baa8eb804f% > 7C0&sdata=YTw33BPKKpdN%2BBFrufVk8e4YqXOzR6VQKuzRMOjcwWk > %3D&reserved=0 > >> > >> Signed-off-by: Martin Kepplinger <martin.kepplinger@xxxxxxxxxxxxx> > >> --- > >> 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.
