| Message ID | b7687459-1665-eb1c-b8ad-2bb37b7136ac@virgin.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add support for touch screens using the General Touch ST6001S controller. | expand |
On 03/10/2021 22:54, Gareth Randall wrote: > Add support for touch screens using the General Touch ST6001S > controller, as found in the GPEG model AOD22WZ-ST monitor. > This controller can output the ELO 10-byte protocol, > but requires different initialisation. > > Signed-off-by: Gareth Randall <gareth@garethrandall.com> > --- > drivers/input/touchscreen/elo.c | 58 +++++++++++++++++++++++++++++++++ > 1 file changed, 58 insertions(+) Hi, I'm seeking feedback on this patch. I just wondered whether I have made any mistakes in the submission, or whether the maintainers are just very busy (most likely explanation!) I know that this does not appear properly in patchwork even though I have attempted to recreate the exact format of a [PATCH] email, but I don't want to send duplicate attempts. I've checked that there are no wrapped lines in the email message. Is there a test suite that is failing that I'm not aware of, or conventions I'm not following? Thanks for any feedback. Yours, Gareth
Dear Dmitry, I wonder if you have had a chance to look at this patch. It was originally sent on 26 Sep 2021, then resent on 3 Oct 2021 to try to make it come out in Patchwork properly. This is my first patch submission so any feedback on both the patch and whether I'm following the process properly would be very useful. Thanks very much. Yours, Gareth On 15/10/2021 10:04, Gareth Randall wrote: > On 03/10/2021 22:54, Gareth Randall wrote: >> Add support for touch screens using the General Touch ST6001S >> controller, as found in the GPEG model AOD22WZ-ST monitor. >> This controller can output the ELO 10-byte protocol, >> but requires different initialisation. >> >> Signed-off-by: Gareth Randall <gareth@garethrandall.com> >> --- >> drivers/input/touchscreen/elo.c | 58 +++++++++++++++++++++++++++++++++ >> 1 file changed, 58 insertions(+) > > Hi, > > I'm seeking feedback on this patch. I just wondered whether I have made > any mistakes in the submission, or whether the maintainers are just very > busy (most likely explanation!) > > I know that this does not appear properly in patchwork even though I > have attempted to recreate the exact format of a [PATCH] email, but I > don't want to send duplicate attempts. I've checked that there are no > wrapped lines in the email message. Is there a test suite that is > failing that I'm not aware of, or conventions I'm not following? > > Thanks for any feedback. > > Yours, > > Gareth
Hi Gareth, On Sun, Oct 03, 2021 at 10:54:21PM +0100, Gareth Randall wrote: > Add support for touch screens using the General Touch ST6001S > controller, as found in the GPEG model AOD22WZ-ST monitor. > This controller can output the ELO 10-byte protocol, > but requires different initialisation. > > Signed-off-by: Gareth Randall <gareth@garethrandall.com> > --- > drivers/input/touchscreen/elo.c | 58 +++++++++++++++++++++++++++++++++ > 1 file changed, 58 insertions(+) > > diff --git a/drivers/input/touchscreen/elo.c > b/drivers/input/touchscreen/elo.c > index 96173232e53f..8c15e0eea6b4 100644 > --- a/drivers/input/touchscreen/elo.c > +++ b/drivers/input/touchscreen/elo.c > @@ -44,6 +44,8 @@ MODULE_LICENSE("GPL"); > #define ELO10_ACK_PACKET 'A' > #define ELI10_ID_PACKET 'I' > > +#define ELO_GT_INIT_PACKET "\001XfE\r" > + > /* > * Per-touchscreen data. > */ > @@ -201,6 +203,7 @@ static irqreturn_t elo_interrupt(struct serio *serio, > > switch (elo->id) { > case 0: > + case 4: > elo_process_data_10(elo, data); > break; > > @@ -255,6 +258,54 @@ static int elo_command_10(struct elo *elo, unsigned > char *packet) > return rc; > } > > +/* > + * Initialise the General Touch ST6001S controller. > + */ > +static int elo_command_10_gt(struct elo *elo) > +{ > + int rc = -1; > + int i; > + unsigned char *packet = ELO_GT_INIT_PACKET; > + > + mutex_lock(&elo->cmd_mutex); > + > + serio_pause_rx(elo->serio); > + init_completion(&elo->cmd_done); > + serio_continue_rx(elo->serio); > + > + for (i = 0; i < (int)strlen(packet); i++) { > + if (serio_write(elo->serio, packet[i])) > + goto out; > + } > + > + wait_for_completion_timeout(&elo->cmd_done, HZ); > + rc = 0; > + > + out: > + mutex_unlock(&elo->cmd_mutex); > + return rc; > +} > + > +static int elo_setup_10_gt(struct elo *elo) > +{ > + struct input_dev *dev = elo->dev; > + > + if (elo_command_10_gt(elo)) > + return -1; return -EIO; > + > + __set_bit(INPUT_PROP_DIRECT, dev->propbit); Please make this a separate patch that would set this property for all variants of ELO touchscreens (i.e. move it into elo_connect()). > + > + // Values taken from a GPEG model AOD22WZ-ST monitor > + input_set_abs_params(dev, ABS_X, 1365, 5828, 0, 0); I believe the datasheet says that the touch resolution is 4096 x 4096: http://www.boardcon.com/download/LCD_datasheet/15inch_SAW_LCD/Serial%20Controller%20ST6001S%20SPEC.pdf > + // max and min inverted because screen axis is inverted > + input_set_abs_params(dev, ABS_Y, 5013, 2260, 0, 0); I dont think this changes anything for reported coordinates by the driver. > + > + dev_info(&elo->serio->dev, > + "GeneralTouch ST6001S touchscreen"); > + > + return 0; > +} > + > static int elo_setup_10(struct elo *elo) > { > static const char *elo_types[] = { "Accu", "Dura", "Intelli", "Carroll" }; > @@ -361,6 +412,13 @@ static int elo_connect(struct serio *serio, struct > serio_driver *drv) > input_set_abs_params(input_dev, ABS_X, 0, 255, 0, 0); > input_set_abs_params(input_dev, ABS_Y, 0, 255, 0, 0); > break; > + > + case 4: /* 10-byte protocol with General Touch initialisation */ > + if (elo_setup_10_gt(elo)) { > + err = -EIO; > + goto fail3; > + } > + break; > } > > err = input_register_device(elo->dev); > -- > 2.27.0 Thanks.
Hi Dmitry, Thanks very much for the feedback. Apologies for the long delay in getting back to you on this. Updated patch is below, and apologies for the email layout. I've also added comments to your points in the quoted message. Add support for touch screens using the General Touch ST6001S controller, as found in the GPEG model AOD22WZ-ST monitor. This controller can output the ELO 10-byte protocol, but requires different initialisation. Signed-off-by: Gareth Randall <gareth@garethrandall.com> diff --git a/drivers/input/touchscreen/elo.c b/drivers/input/touchscreen/elo.c index 96173232e53f..53ba056173df 100644 --- a/drivers/input/touchscreen/elo.c +++ b/drivers/input/touchscreen/elo.c @@ -26,6 +26,27 @@ MODULE_AUTHOR("Vojtech Pavlik <vojtech@ucw.cz>"); MODULE_DESCRIPTION(DRIVER_DESC); MODULE_LICENSE("GPL"); +static uint gt_abs_x_min = 0; +module_param(gt_abs_x_min, uint, S_IRUSR | S_IRGRP | S_IROTH); +MODULE_PARM_DESC(gt_abs_x_min, "abs_x min value in General Touch mode (default: 0)"); + +static uint gt_abs_x_max = 4095; +module_param(gt_abs_x_max, uint, S_IRUSR | S_IRGRP | S_IROTH); +MODULE_PARM_DESC(gt_abs_x_max, "abs_x max value in General Touch mode (default: 4095)"); + +static uint gt_abs_y_min = 0; +module_param(gt_abs_y_min, uint, S_IRUSR | S_IRGRP | S_IROTH); +MODULE_PARM_DESC(gt_abs_y_min, "abs_y min value in General Touch mode (default: 0)"); + +static uint gt_abs_y_max = 4095; +module_param(gt_abs_y_max, uint, S_IRUSR | S_IRGRP | S_IROTH); +MODULE_PARM_DESC(gt_abs_y_max, "abs_y max value in General Touch mode (default: 4095)"); + +static bool gt_mode_override = false; +module_param(gt_mode_override, bool, S_IRUSR | S_IRGRP | S_IROTH); +MODULE_PARM_DESC(gt_mode_override, "force the use of General Touch mode (default: false)"); + + /* * Definitions & global arrays. */ @@ -44,6 +65,8 @@ MODULE_LICENSE("GPL"); #define ELO10_ACK_PACKET 'A' #define ELI10_ID_PACKET 'I' +#define ELO_GT_INIT_PACKET "\001XfE\r" + /* * Per-touchscreen data. */ @@ -201,6 +224,7 @@ static irqreturn_t elo_interrupt(struct serio *serio, switch (elo->id) { case 0: + case 4: elo_process_data_10(elo, data); break; @@ -255,6 +279,50 @@ static int elo_command_10(struct elo *elo, unsigned char *packet) return rc; } +/* + * Initialise the General Touch ST6001S controller. + */ +static int elo_command_10_gt(struct elo *elo) +{ + int rc = -1; + int i; + unsigned char *packet = ELO_GT_INIT_PACKET; + + mutex_lock(&elo->cmd_mutex); + + serio_pause_rx(elo->serio); + init_completion(&elo->cmd_done); + serio_continue_rx(elo->serio); + + for (i = 0; i < (int)strlen(packet); i++) { + if (serio_write(elo->serio, packet[i])) + goto out; + } + + wait_for_completion_timeout(&elo->cmd_done, HZ); + rc = 0; + + out: + mutex_unlock(&elo->cmd_mutex); + return rc; +} + +static int elo_setup_10_gt(struct elo *elo) +{ + struct input_dev *dev = elo->dev; + + if (elo_command_10_gt(elo)) + return -EIO; + + input_set_abs_params(dev, ABS_X, gt_abs_x_min, gt_abs_x_max, 0, 0); + input_set_abs_params(dev, ABS_Y, gt_abs_y_min, gt_abs_y_max, 0, 0); + + dev_info(&elo->serio->dev, + "GeneralTouch ST6001S touchscreen"); + + return 0; +} + static int elo_setup_10(struct elo *elo) { static const char *elo_types[] = { "Accu", "Dura", "Intelli", "Carroll" }; @@ -332,12 +400,16 @@ static int elo_connect(struct serio *serio, struct serio_driver *drv) input_dev->evbit[0] = BIT_MASK(EV_KEY) | BIT_MASK(EV_ABS); input_dev->keybit[BIT_WORD(BTN_TOUCH)] = BIT_MASK(BTN_TOUCH); + __set_bit(INPUT_PROP_DIRECT, input_dev->propbit); serio_set_drvdata(serio, elo); err = serio_open(serio, drv); if (err) goto fail2; + if (gt_mode_override) + elo->id = 4; + switch (elo->id) { case 0: /* 10-byte protocol */ @@ -361,6 +433,13 @@ static int elo_connect(struct serio *serio, struct serio_driver *drv) input_set_abs_params(input_dev, ABS_X, 0, 255, 0, 0); input_set_abs_params(input_dev, ABS_Y, 0, 255, 0, 0); break; + + case 4: /* 10-byte protocol with General Touch initialisation */ + if (elo_setup_10_gt(elo)) { + err = -EIO; + goto fail3; + } + break; } err = input_register_device(elo->dev); On 08/11/2021 05:56, Dmitry Torokhov wrote: > Hi Gareth, > > On Sun, Oct 03, 2021 at 10:54:21PM +0100, Gareth Randall wrote: >> Add support for touch screens using the General Touch ST6001S >> controller, as found in the GPEG model AOD22WZ-ST monitor. >> This controller can output the ELO 10-byte protocol, >> but requires different initialisation. >> >> Signed-off-by: Gareth Randall <gareth@garethrandall.com> >> --- >> drivers/input/touchscreen/elo.c | 58 +++++++++++++++++++++++++++++++++ >> 1 file changed, 58 insertions(+) >> >> diff --git a/drivers/input/touchscreen/elo.c >> b/drivers/input/touchscreen/elo.c >> index 96173232e53f..8c15e0eea6b4 100644 >> --- a/drivers/input/touchscreen/elo.c >> +++ b/drivers/input/touchscreen/elo.c >> @@ -44,6 +44,8 @@ MODULE_LICENSE("GPL"); >> #define ELO10_ACK_PACKET 'A' >> #define ELI10_ID_PACKET 'I' >> >> +#define ELO_GT_INIT_PACKET "\001XfE\r" >> + >> /* >> * Per-touchscreen data. >> */ >> @@ -201,6 +203,7 @@ static irqreturn_t elo_interrupt(struct serio *serio, >> >> switch (elo->id) { >> case 0: >> + case 4: >> elo_process_data_10(elo, data); >> break; >> >> @@ -255,6 +258,54 @@ static int elo_command_10(struct elo *elo, unsigned >> char *packet) >> return rc; >> } >> >> +/* >> + * Initialise the General Touch ST6001S controller. >> + */ >> +static int elo_command_10_gt(struct elo *elo) >> +{ >> + int rc = -1; >> + int i; >> + unsigned char *packet = ELO_GT_INIT_PACKET; >> + >> + mutex_lock(&elo->cmd_mutex); >> + >> + serio_pause_rx(elo->serio); >> + init_completion(&elo->cmd_done); >> + serio_continue_rx(elo->serio); >> + >> + for (i = 0; i < (int)strlen(packet); i++) { >> + if (serio_write(elo->serio, packet[i])) >> + goto out; >> + } >> + >> + wait_for_completion_timeout(&elo->cmd_done, HZ); >> + rc = 0; >> + >> + out: >> + mutex_unlock(&elo->cmd_mutex); >> + return rc; >> +} >> + >> +static int elo_setup_10_gt(struct elo *elo) >> +{ >> + struct input_dev *dev = elo->dev; >> + >> + if (elo_command_10_gt(elo)) >> + return -1; > > return -EIO; > >> + >> + __set_bit(INPUT_PROP_DIRECT, dev->propbit); > > Please make this a separate patch that would set this property for all > variants of ELO touchscreens (i.e. move it into elo_connect()). > >> + >> + // Values taken from a GPEG model AOD22WZ-ST monitor >> + input_set_abs_params(dev, ABS_X, 1365, 5828, 0, 0); > > I believe the datasheet says that the touch resolution is 4096 x 4096: > http://www.boardcon.com/download/LCD_datasheet/15inch_SAW_LCD/Serial%20Controller%20ST6001S%20SPEC.pdf I never felt comfortable with the hard-coded values that I'd put in my patch, so I've changed them to your suggestion (0 and 4095) but with the option to override using kernel parameters. I hope this is okay. (My understanding of input_set_abs_params is that the input device has to be able to reach the values, even if it can go above the max and below the min. When moving my finger on the screen concerned, 1365 was the lowest it would comfortably go and 5828 was the highest, meaning that it would never be able to reach 0.) > > >> + // max and min inverted because screen axis is inverted >> + input_set_abs_params(dev, ABS_Y, 5013, 2260, 0, 0); > > I dont think this changes anything for reported coordinates by the > driver. When experimenting with the hardware the pointer would move correctly with "inverted" max and min, but allowing these to be overridden through kernel parameters will be fine. >> + >> + dev_info(&elo->serio->dev, >> + "GeneralTouch ST6001S touchscreen"); >> + >> + return 0; >> +} >> + >> static int elo_setup_10(struct elo *elo) >> { >> static const char *elo_types[] = { "Accu", "Dura", "Intelli", "Carroll" }; >> @@ -361,6 +412,13 @@ static int elo_connect(struct serio *serio, struct >> serio_driver *drv) >> input_set_abs_params(input_dev, ABS_X, 0, 255, 0, 0); >> input_set_abs_params(input_dev, ABS_Y, 0, 255, 0, 0); >> break; >> + >> + case 4: /* 10-byte protocol with General Touch initialisation */ >> + if (elo_setup_10_gt(elo)) { >> + err = -EIO; >> + goto fail3; >> + } >> + break; >> } >> >> err = input_register_device(elo->dev); >> -- >> 2.27.0 > > Thanks. > Thanks very much for your feedback. Yours, Gareth
Dear Dmitry, I've noticed that this update from a couple of weeks ago hasn't come up in the Patchwork system, probably because the original post was too long ago. I'll happily resubmit this if you (or any other admins) would like me to. Thanks. Gareth On 03/10/2023 19:32, Gareth Randall wrote: > Hi Dmitry, > > Thanks very much for the feedback. Apologies for the long delay in > getting back to you on this. Updated patch is below, and apologies for > the email layout. I've also added comments to your points in the quoted > message. [snip]
diff --git a/drivers/input/touchscreen/elo.c b/drivers/input/touchscreen/elo.c index 96173232e53f..8c15e0eea6b4 100644 --- a/drivers/input/touchscreen/elo.c +++ b/drivers/input/touchscreen/elo.c @@ -44,6 +44,8 @@ MODULE_LICENSE("GPL"); #define ELO10_ACK_PACKET 'A' #define ELI10_ID_PACKET 'I' +#define ELO_GT_INIT_PACKET "\001XfE\r" + /* * Per-touchscreen data. */ @@ -201,6 +203,7 @@ static irqreturn_t elo_interrupt(struct serio *serio, switch (elo->id) { case 0: + case 4: elo_process_data_10(elo, data); break; @@ -255,6 +258,54 @@ static int elo_command_10(struct elo *elo, unsigned char *packet) return rc; } +/* + * Initialise the General Touch ST6001S controller. + */ +static int elo_command_10_gt(struct elo *elo) +{ + int rc = -1; + int i; + unsigned char *packet = ELO_GT_INIT_PACKET; + + mutex_lock(&elo->cmd_mutex); + + serio_pause_rx(elo->serio); + init_completion(&elo->cmd_done); + serio_continue_rx(elo->serio); + + for (i = 0; i < (int)strlen(packet); i++) { + if (serio_write(elo->serio, packet[i])) + goto out; + } + + wait_for_completion_timeout(&elo->cmd_done, HZ); + rc = 0; + + out: + mutex_unlock(&elo->cmd_mutex); + return rc; +} + +static int elo_setup_10_gt(struct elo *elo) +{ + struct input_dev *dev = elo->dev; + + if (elo_command_10_gt(elo)) + return -1; + + __set_bit(INPUT_PROP_DIRECT, dev->propbit); + + // Values taken from a GPEG model AOD22WZ-ST monitor + input_set_abs_params(dev, ABS_X, 1365, 5828, 0, 0); + // max and min inverted because screen axis is inverted + input_set_abs_params(dev, ABS_Y, 5013, 2260, 0, 0); + + dev_info(&elo->serio->dev, + "GeneralTouch ST6001S touchscreen"); + + return 0; +} + static int elo_setup_10(struct elo *elo) {
Add support for touch screens using the General Touch ST6001S controller, as found in the GPEG model AOD22WZ-ST monitor. This controller can output the ELO 10-byte protocol, but requires different initialisation. Signed-off-by: Gareth Randall <gareth@garethrandall.com> --- drivers/input/touchscreen/elo.c | 58 +++++++++++++++++++++++++++++++++ 1 file changed, 58 insertions(+) static const char *elo_types[] = { "Accu", "Dura", "Intelli", "Carroll" }; @@ -361,6 +412,13 @@ static int elo_connect(struct serio *serio, struct serio_driver *drv) input_set_abs_params(input_dev, ABS_X, 0, 255, 0, 0); input_set_abs_params(input_dev, ABS_Y, 0, 255, 0, 0); break; + + case 4: /* 10-byte protocol with General Touch initialisation */ + if (elo_setup_10_gt(elo)) { + err = -EIO; + goto fail3; + } + break; } err = input_register_device(elo->dev);