Re: [PATCH v3 2/5] Input: add driver for Focaltech FTS touchscreen

[Jeff LaBundy <jeff@labundy.com>]

Hi Joel,

On Thu, May 11, 2023 at 07:43:28PM -0500, Joel Selvaraj wrote:
> Hi Jeff LaBundy,
> 
> On 4/23/23 21:39, Jeff LaBundy wrote:
> > Hi Joel,
> > 
> > Great work so far! It's coming along nicely. Please find my latest
> > feedback below.
> 
> Sorry for the late reply, university semester end got me hooked up.
> I have a sad kind of good news... As pointed out by Hans de Goede, the
> edt-ft5x06 driver works out of the box for the fts8719 touchscreen in my
> device. So I think this patch series is no longer needed. I did have a look
> at the driver once when working on this patch series, but it looked
> different/not compatible at that time. After mentioned by Hans de Goede, I
> had a more closer look and the main touch buffer handling seems to be the
> same.
> 
> I am sorry as we put some considerable time in this patch series. I should
> have noted more carefully. Thank you though as I learnt things working on
> this patch series.

There is absolutely no need to apologize; the review process is just as
informative for the reviewer as it is the reviewee. The important thing
is that the most optimal solution lands. "Less code is the best code" :)

> 
> I guess I will send a different patch to add the compatible data to the
> existing edt-ft5x06 driver and dts changes to include that driver to my
> device.

Sounds great.

> 
> > On Fri, Apr 14, 2023 at 09:02:19PM -0500, Joel Selvaraj wrote:
> > > The Focaltech FTS driver supports several variants of focaltech
> > > touchscreens found in ~2018 era smartphones including variants found on
> > > the PocoPhone F1 and the SHIFT6mq which are already present in mainline.
> > > This driver is loosely based on the original driver from Focaltech
> > > but has been simplified and largely reworked.
> > > 
> > > Co-developed-by: Caleb Connolly <caleb@connolly.tech>
> > > Signed-off-by: Caleb Connolly <caleb@connolly.tech>
> > > Signed-off-by: Joel Selvaraj <joelselvaraj.oss@gmail.com>
> > > ---
> > >   MAINTAINERS                                   |   8 +
> > >   drivers/input/touchscreen/Kconfig             |  12 +
> > >   drivers/input/touchscreen/Makefile            |   1 +
> > >   drivers/input/touchscreen/focaltech_fts5452.c | 432 ++++++++++++++++++
> > >   4 files changed, 453 insertions(+)
> > >   create mode 100644 drivers/input/touchscreen/focaltech_fts5452.c
> > > 
> > > diff --git a/MAINTAINERS b/MAINTAINERS
> > > index 7ec4ce64f66d..1a3ea61e1f52 100644
> > > --- a/MAINTAINERS
> > > +++ b/MAINTAINERS
> > > @@ -8028,6 +8028,14 @@ L:	linux-input@vger.kernel.org
> > >   S:	Maintained
> > >   F:	drivers/input/joystick/fsia6b.c
> > > +FOCALTECH FTS5452 TOUCHSCREEN DRIVER
> > > +M:	Joel Selvaraj <joelselvaraj.oss@gmail.com>
> > > +M:	Caleb Connolly <caleb@connolly.tech>
> > > +L:	linux-input@vger.kernel.org
> > > +S:	Maintained
> > > +F:	Documentation/devicetree/bindings/input/touchscreen/focaltech,fts5452.yaml
> > > +F:	drivers/input/touchscreen/focaltech_fts5452.c
> > > +
> > >   FOCUSRITE SCARLETT GEN 2/3 MIXER DRIVER
> > >   M:	Geoffrey D. Bennett <g@b4.vu>
> > >   L:	alsa-devel@alsa-project.org (moderated for non-subscribers)
> > > diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
> > > index 1feecd7ed3cb..11af91504969 100644
> > > --- a/drivers/input/touchscreen/Kconfig
> > > +++ b/drivers/input/touchscreen/Kconfig
> > > @@ -388,6 +388,18 @@ config TOUCHSCREEN_EXC3000
> > >   	  To compile this driver as a module, choose M here: the
> > >   	  module will be called exc3000.
> > > +config TOUCHSCREEN_FOCALTECH_FTS5452
> > > +	tristate "Focaltech FTS Touchscreen"
> > > +	depends on I2C
> > > +	help
> > > +	  Say Y here to enable support for I2C connected Focaltech FTS
> > > +	  based touch panels, including the 5452 and 8917 panels.
> > 
> > This language is a bit misleading, as it seems to suggest three or more
> > models are supported. It seems the title should simply be "FocalTech
> > FTS5452 touchscreen controller" with the description as "...FocalTech
> > FTS5452 and compatible touchscreen controllers."
> > 
> > As more are found to be compatible (e.g. FTS8917), the compatible strings
> > can simply be appended.
> > 
> > > +
> > > +	  If unsure, say N.
> > > +
> > > +	  To compile this driver as a module, choose M here: the
> > > +	  module will be called focaltech_fts.
> > > +
> > >   config TOUCHSCREEN_FUJITSU
> > >   	tristate "Fujitsu serial touchscreen"
> > >   	select SERIO
> > > diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile
> > > index 159cd5136fdb..47d78c9cff21 100644
> > > --- a/drivers/input/touchscreen/Makefile
> > > +++ b/drivers/input/touchscreen/Makefile
> > > @@ -45,6 +45,7 @@ obj-$(CONFIG_TOUCHSCREEN_ELO)		+= elo.o
> > >   obj-$(CONFIG_TOUCHSCREEN_EGALAX)	+= egalax_ts.o
> > >   obj-$(CONFIG_TOUCHSCREEN_EGALAX_SERIAL)	+= egalax_ts_serial.o
> > >   obj-$(CONFIG_TOUCHSCREEN_EXC3000)	+= exc3000.o
> > > +obj-$(CONFIG_TOUCHSCREEN_FOCALTECH_FTS5452)	+= focaltech_fts5452.o
> > >   obj-$(CONFIG_TOUCHSCREEN_FUJITSU)	+= fujitsu_ts.o
> > >   obj-$(CONFIG_TOUCHSCREEN_GOODIX)	+= goodix_ts.o
> > >   obj-$(CONFIG_TOUCHSCREEN_HIDEEP)	+= hideep.o
> > > diff --git a/drivers/input/touchscreen/focaltech_fts5452.c b/drivers/input/touchscreen/focaltech_fts5452.c
> > > new file mode 100644
> > > index 000000000000..abf8a2f271ca
> > > --- /dev/null
> > > +++ b/drivers/input/touchscreen/focaltech_fts5452.c
> > > @@ -0,0 +1,432 @@
> > > +// SPDX-License-Identifier: GPL-2.0-only
> > > +/*
> > > + * FocalTech touchscreen driver.
> > > + *
> > > + * Copyright (c) 2010-2017, FocalTech Systems, Ltd., all rights reserved.
> > > + * Copyright (C) 2018 XiaoMi, Inc.
> > > + * Copyright (c) 2021 Caleb Connolly <caleb@connolly.tech>
> > > + * Copyright (c) 2023 Joel Selvaraj <joelselvaraj.oss@gmail.com>
> > > + */
> 
> [skip]
> 
> > > +static const struct of_device_id fts_of_match[] = {
> > > +	{ .compatible = "focaltech,fts5452", .data = &fts5452_chip_data },
> > > +	{ .compatible = "focaltech,fts8719", .data = &fts8719_chip_data },
> > > +	{ /* sentinel */ }
> > > +};
> > > +
> > > +MODULE_DEVICE_TABLE(of, fts_of_match);
> > > +
> > > +static struct i2c_driver fts_ts_driver = {
> > > +	.probe_new = fts_ts_probe,
> > > +	.id_table = fts_i2c_id,
> > > +	.driver = {
> > > +		.name = FTS_DRIVER_NAME,
> > > +		.pm = pm_sleep_ptr(&fts_dev_pm_ops),
> > > +		.of_match_table = fts_of_match,
> > > +	},
> > > +};
> > > +module_i2c_driver(fts_ts_driver);
> > > +
> > > +MODULE_AUTHOR("Joel Selvaraj <joelselvaraj.oss@gmail.com>");
> > > +MODULE_AUTHOR("Caleb Connolly <caleb@connolly.tech>");
> > > +MODULE_DESCRIPTION("Focaltech Touchscreen Driver");
> > 
> > Nit: mixing 'FocalTech' and 'Focaltech' throughout.
> > 
> > > +MODULE_LICENSE("GPL");
> > > -- 
> > > 2.40.0
> > > 
> > 
> > Kind regards,
> > Jeff LaBundy
> 
> Thank you,
> Joel Selvaraj

Kind regards,
Jeff LaBundy