| Message ID | 20230606-topic-goodix-berlin-upstream-initial-v1-4-4a0741b8aefd@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | input: touchscreen: add initial support for Goodix Berlin touchscreen IC | expand |
Hi Neil, On Tue, Jun 06, 2023 at 04:31:59PM +0200, Neil Armstrong wrote: > Add initial support for the new Goodix "Berlin" touchscreen ICs > over the SPI interface. > > The driver doesn't use the regmap_spi code since the SPI messages > needs to be prefixed, thus this custom regmap code. > > This initial driver is derived from the Goodix goodix_ts_berlin > available at [1] and [2] and only supports the GT9916 IC > present on the Qualcomm SM8550 MTP & QRD touch panel. > > The current implementation only supports BerlinD, aka GT9916. > > [1] https://github.com/goodix/goodix_ts_berlin > [2] https://git.codelinaro.org/clo/la/platform/vendor/opensource/touch-drivers > > Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org> > --- > drivers/input/touchscreen/Kconfig | 14 ++ > drivers/input/touchscreen/Makefile | 1 + > drivers/input/touchscreen/goodix_berlin_spi.c | 183 ++++++++++++++++++++++++++ > 3 files changed, 198 insertions(+) > > diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig > index da6d5d75c42d..ffe0c0a4cd15 100644 > --- a/drivers/input/touchscreen/Kconfig > +++ b/drivers/input/touchscreen/Kconfig > @@ -435,6 +435,20 @@ config TOUCHSCREEN_GOODIX_BERLIN_I2C > To compile this driver as a module, choose M here: the > module will be called goodix_berlin_i2c. > > +config TOUCHSCREEN_GOODIX_BERLIN_SPI > + tristate "Goodix Berlin SPI touchscreen" > + depends on SPI_MASTER > + depends on REGMAP As TOUCHSCREEN_GOODIX_BERLIN_CORE already depends on REGMAP; is this line necessary? I was about to ask why not to select REGMAP_SPI; thank you for the additional information in the commit message. > + select TOUCHSCREEN_GOODIX_BERLIN_CORE > + help > + Say Y here if you have the a touchscreen connected to your > + system using the Goodix Berlin IC connection via SPI. Same comment here with regard to diction. > + > + If unsure, say N. > + > + To compile this driver as a module, choose M here: the > + module will be called goodix_berlin_spi. > + > config TOUCHSCREEN_HIDEEP > tristate "HiDeep Touch IC" > depends on I2C > diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile > index 921a2da0c2be..29524e8a83db 100644 > --- a/drivers/input/touchscreen/Makefile > +++ b/drivers/input/touchscreen/Makefile > @@ -49,6 +49,7 @@ obj-$(CONFIG_TOUCHSCREEN_FUJITSU) += fujitsu_ts.o > obj-$(CONFIG_TOUCHSCREEN_GOODIX) += goodix_ts.o > obj-$(CONFIG_TOUCHSCREEN_GOODIX_BERLIN_CORE) += goodix_berlin_core.o > obj-$(CONFIG_TOUCHSCREEN_GOODIX_BERLIN_I2C) += goodix_berlin_i2c.o > +obj-$(CONFIG_TOUCHSCREEN_GOODIX_BERLIN_SPI) += goodix_berlin_spi.o > obj-$(CONFIG_TOUCHSCREEN_HIDEEP) += hideep.o > obj-$(CONFIG_TOUCHSCREEN_HYNITRON_CSTXXX) += hynitron_cstxxx.o > obj-$(CONFIG_TOUCHSCREEN_ILI210X) += ili210x.o > diff --git a/drivers/input/touchscreen/goodix_berlin_spi.c b/drivers/input/touchscreen/goodix_berlin_spi.c > new file mode 100644 > index 000000000000..0f4f650fdf3f > --- /dev/null > +++ b/drivers/input/touchscreen/goodix_berlin_spi.c > @@ -0,0 +1,183 @@ > +// SPDX-License-Identifier: GPL-2.0-or-later > +/* > + * Goodix Berlin Touchscreen Driver > + * > + * Copyright (C) 2020 - 2021 Goodix, Inc. > + * Copyright (C) 2023 Linaro Ltd. > + * > + * Based on goodix_ts_berlin driver. > + */ > +#include <linux/kernel.h> > +#include <linux/module.h> > +#include <linux/spi/spi.h> > +#include <linux/regmap.h> Please alphabetize these to aid readability. > +#include <asm/unaligned.h> > + > +#include "goodix_berlin.h" > + > +#define SPI_TRANS_PREFIX_LEN 1 > +#define REGISTER_WIDTH 4 > +#define SPI_READ_DUMMY_LEN 3 > +#define SPI_READ_PREFIX_LEN (SPI_TRANS_PREFIX_LEN + REGISTER_WIDTH + SPI_READ_DUMMY_LEN) > +#define SPI_WRITE_PREFIX_LEN (SPI_TRANS_PREFIX_LEN + REGISTER_WIDTH) > + > +#define SPI_WRITE_FLAG 0xF0 > +#define SPI_READ_FLAG 0xF1 > + > +static int goodix_berlin_spi_read(void *context, const void *reg_buf, > + size_t reg_size, void *val_buf, > + size_t val_size) > +{ > + struct spi_device *spi = context; > + struct spi_transfer xfers; > + struct spi_message spi_msg; > + const u32 *reg = reg_buf; /* reg is stored as native u32 at start of buffer */ > + u8 *buf = NULL; > + int ret = 0; No need to initialize these, only to forcibly assign them later. > + > + if (reg_size != REGISTER_WIDTH) > + return -EINVAL; > + > + buf = kzalloc(SPI_READ_PREFIX_LEN + val_size, GFP_KERNEL); > + if (!buf) > + return -ENOMEM; > + > + spi_message_init(&spi_msg); > + memset(&xfers, 0, sizeof(xfers)); > + > + /* buffer format: 0xF1 + addr(4bytes) + dummy(3bytes) + data */ > + buf[0] = SPI_READ_FLAG; > + put_unaligned_be32(*reg, buf + SPI_TRANS_PREFIX_LEN); > + memset(buf + SPI_TRANS_PREFIX_LEN + REGISTER_WIDTH, 0xff, > + SPI_READ_DUMMY_LEN); > + > + xfers.tx_buf = buf; > + xfers.rx_buf = buf; > + xfers.len = SPI_READ_PREFIX_LEN + val_size; > + xfers.cs_change = 0; > + spi_message_add_tail(&xfers, &spi_msg); > + > + ret = spi_sync(spi, &spi_msg); > + if (ret < 0) { > + dev_err(&spi->dev, "transfer error:%d", ret); > + goto exit; > + } My comment is purely idiomatic, but this seems cleaner: ret = ... if (ret) dev_err(...); else memcpy(...); kfree(...); return ret; > + > + memcpy(val_buf, buf + SPI_READ_PREFIX_LEN, val_size); > +exit: > + kfree(buf); > + return ret; > +} > + > +static int goodix_berlin_spi_write(void *context, const void *data, > + size_t count) > +{ > + unsigned int len = count - REGISTER_WIDTH; > + struct spi_device *spi = context; > + struct spi_transfer xfers; > + struct spi_message spi_msg; > + const u32 *reg = data; /* reg is stored as native u32 at start of buffer */ > + u8 *buf = NULL; > + int ret = 0; Same comment here with regard to initialization. > + > + buf = kzalloc(SPI_WRITE_PREFIX_LEN + len, GFP_KERNEL); > + if (!buf) > + return -ENOMEM; > + > + spi_message_init(&spi_msg); > + memset(&xfers, 0, sizeof(xfers)); > + > + buf[0] = SPI_WRITE_FLAG; > + put_unaligned_be32(*reg, buf + SPI_TRANS_PREFIX_LEN); > + memcpy(buf + SPI_WRITE_PREFIX_LEN, data + REGISTER_WIDTH, len); > + > + xfers.tx_buf = buf; > + xfers.len = SPI_WRITE_PREFIX_LEN + len; > + xfers.cs_change = 0; > + spi_message_add_tail(&xfers, &spi_msg); > + > + ret = spi_sync(spi, &spi_msg); > + if (ret < 0) > + dev_err(&spi->dev, "transfer error:%d", ret); > + > + kfree(buf); > + return ret; > +} > + > +static const struct regmap_config goodix_berlin_spi_regmap_conf = { > + .reg_bits = 32, > + .val_bits = 8, > + .read = goodix_berlin_spi_read, > + .write = goodix_berlin_spi_write, > +}; > + > +static const struct input_id goodix_berlin_spi_input_id = { > + .bustype = BUS_SPI, > + .vendor = 0x0416, > + .product = 0x1001, After having seen these in the I2C counterpart; consider defining them in goodix_berlin.h. > +}; > + > +static int goodix_berlin_spi_probe(struct spi_device *spi) > +{ > + struct regmap_config *cfg; regmap_config > + struct regmap *map; regmap (see more examples in MFD where such dual-mode devices are common). > + size_t max_size; > + int ret = 0; > + > + cfg = devm_kmemdup(&spi->dev, &goodix_berlin_spi_regmap_conf, > + sizeof(*cfg), GFP_KERNEL); > + if (!cfg) > + return -ENOMEM; > + > + spi->mode = SPI_MODE_0; > + spi->bits_per_word = 8; > + ret = spi_setup(spi); > + if (ret) > + return ret; > + > + max_size = spi_max_transfer_size(spi); > + cfg->max_raw_read = max_size - SPI_READ_PREFIX_LEN; > + cfg->max_raw_write = max_size - SPI_WRITE_PREFIX_LEN; > + > + map = devm_regmap_init(&spi->dev, NULL, spi, cfg); > + if (IS_ERR(map)) > + return PTR_ERR(map); > + > + return goodix_berlin_probe(&spi->dev, spi->irq, > + &goodix_berlin_spi_input_id, map); > +} > + > +static void goodix_berlin_spi_remove(struct spi_device *spi) > +{ > + goodix_berlin_remove(&spi->dev); > +} > + > +static const struct of_device_id goodix_berlin_spi_of_match[] = { > + { > + .compatible = "goodix,gt9916", > + }, This format is different than its I2C counterpart. > + { }, Nit: same comment with regards to trailing commas. > +}; > +MODULE_DEVICE_TABLE(of, goodix_berlin_spi_of_match); > + > +static const struct spi_device_id goodix_berlin_spi_ids[] = { > + { "gt9916" }, > + { }, And here. > +}; > +MODULE_DEVICE_TABLE(spi, goodix_berlin_spi_ids); > + > +static struct spi_driver goodix_berlin_spi_driver = { > + .driver = { > + .name = "goodix-berlin-spi", > + .of_match_table = goodix_berlin_spi_of_match, > + .pm = pm_sleep_ptr(&goodix_berlin_pm_ops), > + }, > + .id_table = goodix_berlin_spi_ids, > + .probe = goodix_berlin_spi_probe, > + .remove = goodix_berlin_spi_remove, > +}; > +module_spi_driver(goodix_berlin_spi_driver); > + > +MODULE_LICENSE("GPL"); > +MODULE_DESCRIPTION("Goodix Berlin SPI Touchscreen driver"); > +MODULE_AUTHOR("Neil Armstrong <neil.armstrong@linaro.org>"); > > -- > 2.34.1 > Kind regards, Jeff LaBundy
On 12/06/2023 19:07, Jeff LaBundy wrote: > Hi Neil, > > [...] > >>>> +static const struct input_id goodix_berlin_spi_input_id = { >>>> + .bustype = BUS_SPI, >>>> + .vendor = 0x0416, >>>> + .product = 0x1001, >>> >>> After having seen these in the I2C counterpart; consider defining them >>> in goodix_berlin.h. >> >> To be honest, I blindly copied it from goodix.c because the vendor >> driver puts random values here. >> >> input_dev->id.product = 0xDEAD; >> input_dev->id.vendor = 0xBEEF; >> >> So what should I set ? > > If there is no explicit guidance from the vendor, I would simply leave > these unassigned; in theory one would imagine that this controller would > have a different product ID than other models. I'll leave those unassigned for now, and perhaps we could find some firmware info value that could be used here. Neil > > [...] > > Kind regards, > Jeff LaBundy
diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig index da6d5d75c42d..ffe0c0a4cd15 100644 --- a/drivers/input/touchscreen/Kconfig +++ b/drivers/input/touchscreen/Kconfig @@ -435,6 +435,20 @@ config TOUCHSCREEN_GOODIX_BERLIN_I2C To compile this driver as a module, choose M here: the module will be called goodix_berlin_i2c. +config TOUCHSCREEN_GOODIX_BERLIN_SPI + tristate "Goodix Berlin SPI touchscreen" + depends on SPI_MASTER + depends on REGMAP + select TOUCHSCREEN_GOODIX_BERLIN_CORE + help + Say Y here if you have the a touchscreen connected to your + system using the Goodix Berlin IC connection via SPI. + + If unsure, say N. + + To compile this driver as a module, choose M here: the + module will be called goodix_berlin_spi. + config TOUCHSCREEN_HIDEEP tristate "HiDeep Touch IC" depends on I2C diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile index 921a2da0c2be..29524e8a83db 100644 --- a/drivers/input/touchscreen/Makefile +++ b/drivers/input/touchscreen/Makefile @@ -49,6 +49,7 @@ obj-$(CONFIG_TOUCHSCREEN_FUJITSU) += fujitsu_ts.o obj-$(CONFIG_TOUCHSCREEN_GOODIX) += goodix_ts.o obj-$(CONFIG_TOUCHSCREEN_GOODIX_BERLIN_CORE) += goodix_berlin_core.o obj-$(CONFIG_TOUCHSCREEN_GOODIX_BERLIN_I2C) += goodix_berlin_i2c.o +obj-$(CONFIG_TOUCHSCREEN_GOODIX_BERLIN_SPI) += goodix_berlin_spi.o obj-$(CONFIG_TOUCHSCREEN_HIDEEP) += hideep.o obj-$(CONFIG_TOUCHSCREEN_HYNITRON_CSTXXX) += hynitron_cstxxx.o obj-$(CONFIG_TOUCHSCREEN_ILI210X) += ili210x.o diff --git a/drivers/input/touchscreen/goodix_berlin_spi.c b/drivers/input/touchscreen/goodix_berlin_spi.c new file mode 100644 index 000000000000..0f4f650fdf3f --- /dev/null +++ b/drivers/input/touchscreen/goodix_berlin_spi.c @@ -0,0 +1,183 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +/* + * Goodix Berlin Touchscreen Driver + * + * Copyright (C) 2020 - 2021 Goodix, Inc. + * Copyright (C) 2023 Linaro Ltd. + * + * Based on goodix_ts_berlin driver. + */ +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/spi/spi.h> +#include <linux/regmap.h> +#include <asm/unaligned.h> + +#include "goodix_berlin.h" + +#define SPI_TRANS_PREFIX_LEN 1 +#define REGISTER_WIDTH 4 +#define SPI_READ_DUMMY_LEN 3 +#define SPI_READ_PREFIX_LEN (SPI_TRANS_PREFIX_LEN + REGISTER_WIDTH + SPI_READ_DUMMY_LEN) +#define SPI_WRITE_PREFIX_LEN (SPI_TRANS_PREFIX_LEN + REGISTER_WIDTH) + +#define SPI_WRITE_FLAG 0xF0 +#define SPI_READ_FLAG 0xF1 + +static int goodix_berlin_spi_read(void *context, const void *reg_buf, + size_t reg_size, void *val_buf, + size_t val_size) +{ + struct spi_device *spi = context; + struct spi_transfer xfers; + struct spi_message spi_msg; + const u32 *reg = reg_buf; /* reg is stored as native u32 at start of buffer */ + u8 *buf = NULL; + int ret = 0; + + if (reg_size != REGISTER_WIDTH) + return -EINVAL; + + buf = kzalloc(SPI_READ_PREFIX_LEN + val_size, GFP_KERNEL); + if (!buf) + return -ENOMEM; + + spi_message_init(&spi_msg); + memset(&xfers, 0, sizeof(xfers)); + + /* buffer format: 0xF1 + addr(4bytes) + dummy(3bytes) + data */ + buf[0] = SPI_READ_FLAG; + put_unaligned_be32(*reg, buf + SPI_TRANS_PREFIX_LEN); + memset(buf + SPI_TRANS_PREFIX_LEN + REGISTER_WIDTH, 0xff, + SPI_READ_DUMMY_LEN); + + xfers.tx_buf = buf; + xfers.rx_buf = buf; + xfers.len = SPI_READ_PREFIX_LEN + val_size; + xfers.cs_change = 0; + spi_message_add_tail(&xfers, &spi_msg); + + ret = spi_sync(spi, &spi_msg); + if (ret < 0) { + dev_err(&spi->dev, "transfer error:%d", ret); + goto exit; + } + + memcpy(val_buf, buf + SPI_READ_PREFIX_LEN, val_size); +exit: + kfree(buf); + return ret; +} + +static int goodix_berlin_spi_write(void *context, const void *data, + size_t count) +{ + unsigned int len = count - REGISTER_WIDTH; + struct spi_device *spi = context; + struct spi_transfer xfers; + struct spi_message spi_msg; + const u32 *reg = data; /* reg is stored as native u32 at start of buffer */ + u8 *buf = NULL; + int ret = 0; + + buf = kzalloc(SPI_WRITE_PREFIX_LEN + len, GFP_KERNEL); + if (!buf) + return -ENOMEM; + + spi_message_init(&spi_msg); + memset(&xfers, 0, sizeof(xfers)); + + buf[0] = SPI_WRITE_FLAG; + put_unaligned_be32(*reg, buf + SPI_TRANS_PREFIX_LEN); + memcpy(buf + SPI_WRITE_PREFIX_LEN, data + REGISTER_WIDTH, len); + + xfers.tx_buf = buf; + xfers.len = SPI_WRITE_PREFIX_LEN + len; + xfers.cs_change = 0; + spi_message_add_tail(&xfers, &spi_msg); + + ret = spi_sync(spi, &spi_msg); + if (ret < 0) + dev_err(&spi->dev, "transfer error:%d", ret); + + kfree(buf); + return ret; +} + +static const struct regmap_config goodix_berlin_spi_regmap_conf = { + .reg_bits = 32, + .val_bits = 8, + .read = goodix_berlin_spi_read, + .write = goodix_berlin_spi_write, +}; + +static const struct input_id goodix_berlin_spi_input_id = { + .bustype = BUS_SPI, + .vendor = 0x0416, + .product = 0x1001, +}; + +static int goodix_berlin_spi_probe(struct spi_device *spi) +{ + struct regmap_config *cfg; + struct regmap *map; + size_t max_size; + int ret = 0; + + cfg = devm_kmemdup(&spi->dev, &goodix_berlin_spi_regmap_conf, + sizeof(*cfg), GFP_KERNEL); + if (!cfg) + return -ENOMEM; + + spi->mode = SPI_MODE_0; + spi->bits_per_word = 8; + ret = spi_setup(spi); + if (ret) + return ret; + + max_size = spi_max_transfer_size(spi); + cfg->max_raw_read = max_size - SPI_READ_PREFIX_LEN; + cfg->max_raw_write = max_size - SPI_WRITE_PREFIX_LEN; + + map = devm_regmap_init(&spi->dev, NULL, spi, cfg); + if (IS_ERR(map)) + return PTR_ERR(map); + + return goodix_berlin_probe(&spi->dev, spi->irq, + &goodix_berlin_spi_input_id, map); +} + +static void goodix_berlin_spi_remove(struct spi_device *spi) +{ + goodix_berlin_remove(&spi->dev); +} + +static const struct of_device_id goodix_berlin_spi_of_match[] = { + { + .compatible = "goodix,gt9916", + }, + { }, +}; +MODULE_DEVICE_TABLE(of, goodix_berlin_spi_of_match); + +static const struct spi_device_id goodix_berlin_spi_ids[] = { + { "gt9916" }, + { }, +}; +MODULE_DEVICE_TABLE(spi, goodix_berlin_spi_ids); + +static struct spi_driver goodix_berlin_spi_driver = { + .driver = { + .name = "goodix-berlin-spi", + .of_match_table = goodix_berlin_spi_of_match, + .pm = pm_sleep_ptr(&goodix_berlin_pm_ops), + }, + .id_table = goodix_berlin_spi_ids, + .probe = goodix_berlin_spi_probe, + .remove = goodix_berlin_spi_remove, +}; +module_spi_driver(goodix_berlin_spi_driver); + +MODULE_LICENSE("GPL"); +MODULE_DESCRIPTION("Goodix Berlin SPI Touchscreen driver"); +MODULE_AUTHOR("Neil Armstrong <neil.armstrong@linaro.org>");
Add initial support for the new Goodix "Berlin" touchscreen ICs over the SPI interface. The driver doesn't use the regmap_spi code since the SPI messages needs to be prefixed, thus this custom regmap code. This initial driver is derived from the Goodix goodix_ts_berlin available at [1] and [2] and only supports the GT9916 IC present on the Qualcomm SM8550 MTP & QRD touch panel. The current implementation only supports BerlinD, aka GT9916. [1] https://github.com/goodix/goodix_ts_berlin [2] https://git.codelinaro.org/clo/la/platform/vendor/opensource/touch-drivers Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org> --- drivers/input/touchscreen/Kconfig | 14 ++ drivers/input/touchscreen/Makefile | 1 + drivers/input/touchscreen/goodix_berlin_spi.c | 183 ++++++++++++++++++++++++++ 3 files changed, 198 insertions(+)