Hi Jeff, On 2/21/23 04:39, Jeff LaBundy wrote: > Hi Hans, > > On Fri, Feb 17, 2023 at 04:07:49PM +0100, Hans de Goede wrote: >> Add a new driver for the Novatek i2c touchscreen controller as found >> on the Acer Iconia One 7 B1-750 tablet. Unfortunately the touchscreen >> controller model-number is unknown. Even with the tablet opened up it >> is impossible to read the model-number. >> >> Android calls this a "NVT-ts" touchscreen, but that may apply to other >> Novatek controller models too. >> >> This appears to be the same controller as the one supported by >> https://github.com/advx9600/android/blob/master/touchscreen/NVTtouch_Android4.0/NVTtouch.c >> but unfortunately that does not give us a model-number either. >> >> Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx> > > This is a great driver; I have only a few comments below. Thanks and thank you for the review! >> --- >> MAINTAINERS | 6 + >> drivers/input/touchscreen/Kconfig | 10 + >> drivers/input/touchscreen/Makefile | 1 + >> drivers/input/touchscreen/novatek-nvt-ts.c | 288 +++++++++++++++++++++ >> 4 files changed, 305 insertions(+) >> create mode 100644 drivers/input/touchscreen/novatek-nvt-ts.c >> >> diff --git a/MAINTAINERS b/MAINTAINERS >> index 60c0ded06e3f..0c051a973e6b 100644 >> --- a/MAINTAINERS >> +++ b/MAINTAINERS >> @@ -14835,6 +14835,12 @@ T: git git://git.kernel.org/pub/scm/linux/kernel/git/wtarreau/nolibc.git >> F: tools/include/nolibc/ >> F: tools/testing/selftests/nolibc/ >> >> +NOVATEK NVT-TS I2C TOUCHSCREEN DRIVER >> +M: Hans de Goede <hdegoede@xxxxxxxxxx> >> +L: linux-input@xxxxxxxxxxxxxxx >> +S: Maintained >> +F: drivers/input/touchscreen/novatek-nvt-ts.c >> + >> NSDEPS >> M: Matthias Maennich <maennich@xxxxxxxxxx> >> S: Maintained >> diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig >> index 68d99a112e14..59ca8bfe9a95 100644 >> --- a/drivers/input/touchscreen/Kconfig >> +++ b/drivers/input/touchscreen/Kconfig >> @@ -666,6 +666,16 @@ config TOUCHSCREEN_MTOUCH >> To compile this driver as a module, choose M here: the >> module will be called mtouch. >> >> +config TOUCHSCREEN_NOVATEK_NVT_TS >> + tristate "Novatek NVT-ts touchscreen support" >> + depends on I2C >> + help >> + Say Y here if you have a Novatek NVT-ts touchscreen. >> + If unsure, say N. >> + >> + To compile this driver as a module, choose M here: the >> + module will be called novatek-nvt-ts. >> + >> config TOUCHSCREEN_IMAGIS >> tristate "Imagis touchscreen support" >> depends on I2C >> diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile >> index 4968c370479a..41654239f89c 100644 >> --- a/drivers/input/touchscreen/Makefile >> +++ b/drivers/input/touchscreen/Makefile >> @@ -67,6 +67,7 @@ obj-$(CONFIG_TOUCHSCREEN_MMS114) += mms114.o >> obj-$(CONFIG_TOUCHSCREEN_MSG2638) += msg2638.o >> obj-$(CONFIG_TOUCHSCREEN_MTOUCH) += mtouch.o >> obj-$(CONFIG_TOUCHSCREEN_MK712) += mk712.o >> +obj-$(CONFIG_TOUCHSCREEN_NOVATEK_NVT_TS) += novatek-nvt-ts.o >> obj-$(CONFIG_TOUCHSCREEN_HP600) += hp680_ts_input.o >> obj-$(CONFIG_TOUCHSCREEN_HP7XX) += jornada720_ts.o >> obj-$(CONFIG_TOUCHSCREEN_IPAQ_MICRO) += ipaq-micro-ts.o >> diff --git a/drivers/input/touchscreen/novatek-nvt-ts.c b/drivers/input/touchscreen/novatek-nvt-ts.c >> new file mode 100644 >> index 000000000000..2464c758ca14 >> --- /dev/null >> +++ b/drivers/input/touchscreen/novatek-nvt-ts.c >> @@ -0,0 +1,288 @@ >> +// SPDX-License-Identifier: GPL-2.0-or-later >> +/* >> + * Driver for Novatek i2c touchscreen controller as found on >> + * the Acer Iconia One 7 B1-750 tablet. The Touchscreen controller >> + * model-number is unknown. Android calls this a "NVT-ts" touchscreen, >> + * but that may apply to other Novatek controller models too. >> + * >> + * Copyright (c) 2023 Hans de Goede <hdegoede@xxxxxxxxxx> >> + */ >> + >> +#include <linux/delay.h> >> +#include <linux/gpio/consumer.h> >> +#include <linux/interrupt.h> >> +#include <linux/i2c.h> >> +#include <linux/input.h> >> +#include <linux/input/mt.h> >> +#include <linux/input/touchscreen.h> >> +#include <linux/module.h> >> + >> +#include <asm/unaligned.h> >> + >> +#define NVT_TS_TOUCH_START 0x00 >> +#define NVT_TS_TOUCH_SIZE 6 >> + >> +#define NVT_TS_PARAMETERS_START 0x78 >> +/* These are offsets from NVT_TS_PARAMETERS_START */ >> +#define NVT_TS_PARAMS_WIDTH 0x04 >> +#define NVT_TS_PARAMS_HEIGHT 0x06 >> +#define NVT_TS_PARAMS_MAX_TOUCH 0x09 >> +#define NVT_TS_PARAMS_MAX_BUTTONS 0x0a >> +#define NVT_TS_PARAMS_IRQ_TYPE 0x0b >> +#define NVT_TS_PARAMS_WAKE_TYPE 0x0c >> +#define NVT_TS_PARAMS_CHIP_ID 0x0e >> +#define NVT_TS_PARAMS_SIZE 0x0f >> + >> +#define NVT_TS_SUPPORTED_WAKE_TYPE 0x05 >> +#define NVT_TS_SUPPORTED_CHIP_ID 0x05 >> + >> +#define NVT_TS_MAX_TOUCHES 10 >> +#define NVT_TS_MAX_SIZE 4096 >> + >> +#define NVT_TS_TOUCH_NEW 1 >> +#define NVT_TS_TOUCH_UPDATE 2 >> +#define NVT_TS_TOUCH_RELEASE 3 >> + >> +static const int nvt_ts_irq_type[4] = { >> + IRQF_TRIGGER_RISING, >> + IRQF_TRIGGER_FALLING, >> + IRQF_TRIGGER_LOW, >> + IRQF_TRIGGER_HIGH >> +}; >> + >> +struct nvt_ts_data { >> + struct i2c_client *client; >> + struct input_dev *input; >> + struct gpio_desc *reset_gpio; >> + struct touchscreen_properties prop; >> + int max_touches; >> + u8 buf[NVT_TS_TOUCH_SIZE * NVT_TS_MAX_TOUCHES]; >> +}; >> + >> +static int nvt_ts_read_data(struct i2c_client *client, u8 reg, u8 *data, int count) >> +{ >> + struct i2c_msg msg[2] = { >> + { >> + .addr = client->addr, >> + .len = 1, >> + .buf = ® > > Nit: there is no trailing comma here, yet one trails 'buf' below. Ack, I'll add the trailing comma here too. >> + }, >> + { >> + .addr = client->addr, >> + .flags = I2C_M_RD, >> + .len = count, >> + .buf = data, >> + } >> + }; >> + int ret; >> + >> + ret = i2c_transfer(client->adapter, msg, 2); >> + if (ret != 2) { >> + dev_err(&client->dev, "Error reading from 0x%02x: %d\n", reg, ret); >> + return (ret < 0) ? ret : -EIO; >> + } > > This is idiomatic, but I feel it is clearer to write ARRAY_SIZE(msg) instead > of 2 throughout; this way the length is hard-coded only once. Sure, will update for v2. >> + >> + return 0; >> +} >> + >> +static irqreturn_t nvt_ts_irq(int irq, void *dev_id) >> +{ >> + struct nvt_ts_data *data = dev_id; >> + struct device *dev = &data->client->dev; >> + int i, ret, slot, x, y; > > In input, return values for functions that only return zero on success tend to > be named 'error'. Ack, will update for v2. >> + bool active; >> + u8 *touch; >> + >> + ret = nvt_ts_read_data(data->client, NVT_TS_TOUCH_START, data->buf, >> + data->max_touches * NVT_TS_TOUCH_SIZE); >> + if (ret) >> + return IRQ_HANDLED; >> + >> + for (i = 0; i < data->max_touches; i++) { >> + touch = &data->buf[i * NVT_TS_TOUCH_SIZE]; >> + >> + /* 0xff means no touch */ >> + if (touch[0] == 0xff) >> + continue; >> + >> + slot = touch[0] >> 3; >> + if (slot < 1 || slot > data->max_touches) { >> + dev_warn(dev, "slot %d out of range, ignoring\n", slot); >> + continue; >> + } >> + >> + switch (touch[0] & 7) { > > With all other fields and values defined so nicely, it seems most clear to > also define the bit field name in this case. Ack, I'll add a define for this. > >> + case NVT_TS_TOUCH_NEW: >> + case NVT_TS_TOUCH_UPDATE: >> + active = true; >> + break; >> + case NVT_TS_TOUCH_RELEASE: >> + active = false; >> + break; >> + default: >> + dev_warn(dev, "slot %d unknown state %d\n", slot, touch[0] & 7); >> + continue; >> + } >> + >> + slot--; >> + x = (touch[1] << 4) | (touch[3] >> 4); >> + y = (touch[2] << 4) | (touch[3] & 0x0f); >> + >> + input_mt_slot(data->input, slot); >> + input_mt_report_slot_state(data->input, MT_TOOL_FINGER, active); >> + touchscreen_report_pos(data->input, &data->prop, x, y, true); >> + } >> + >> + input_mt_sync_frame(data->input); >> + input_sync(data->input); >> + >> + return IRQ_HANDLED; >> +} >> + >> +static int nvt_ts_start(struct input_dev *dev) >> +{ >> + struct nvt_ts_data *data = input_get_drvdata(dev); >> + >> + enable_irq(data->client->irq); >> + gpiod_set_value_cansleep(data->reset_gpio, 0); >> + >> + return 0; >> +} >> + >> +static void nvt_ts_stop(struct input_dev *dev) >> +{ >> + struct nvt_ts_data *data = input_get_drvdata(dev); >> + >> + disable_irq(data->client->irq); >> + gpiod_set_value_cansleep(data->reset_gpio, 1); >> +} >> + >> +static int nvt_ts_suspend(struct device *dev) >> +{ >> + struct nvt_ts_data *data = i2c_get_clientdata(to_i2c_client(dev)); >> + >> + mutex_lock(&data->input->mutex); >> + if (input_device_enabled(data->input)) >> + nvt_ts_stop(data->input); >> + mutex_unlock(&data->input->mutex); >> + >> + return 0; >> +} >> + >> +static int nvt_ts_resume(struct device *dev) >> +{ >> + struct nvt_ts_data *data = i2c_get_clientdata(to_i2c_client(dev)); >> + >> + mutex_lock(&data->input->mutex); >> + if (input_device_enabled(data->input)) >> + nvt_ts_start(data->input); >> + mutex_unlock(&data->input->mutex); >> + >> + return 0; >> +} >> + >> +static DEFINE_SIMPLE_DEV_PM_OPS(nvt_ts_pm_ops, nvt_ts_suspend, nvt_ts_resume); >> + >> +static int nvt_ts_probe(struct i2c_client *client) >> +{ >> + struct device *dev = &client->dev; >> + int ret, width, height, irq_type; >> + struct nvt_ts_data *data; >> + struct input_dev *input; >> + >> + if (!client->irq) { >> + dev_err(dev, "Error no irq specified\n"); >> + return -EINVAL; >> + } >> + >> + data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL); >> + if (!data) >> + return -ENOMEM; >> + >> + data->client = client; >> + i2c_set_clientdata(client, data); >> + >> + data->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW); >> + if (IS_ERR(data->reset_gpio)) >> + return dev_err_probe(dev, PTR_ERR(data->reset_gpio), "requesting reset GPIO\n"); >> + >> + /* Wait for controller to come out of reset before params read */ >> + msleep(100); >> + ret = nvt_ts_read_data(data->client, NVT_TS_PARAMETERS_START, data->buf, >> + NVT_TS_PARAMS_SIZE); >> + gpiod_set_value_cansleep(data->reset_gpio, 1); /* Put back in reset */ >> + if (ret) >> + return ret; >> + >> + width = get_unaligned_be16(&data->buf[NVT_TS_PARAMS_WIDTH]); >> + height = get_unaligned_be16(&data->buf[NVT_TS_PARAMS_HEIGHT]); >> + data->max_touches = data->buf[NVT_TS_PARAMS_MAX_TOUCH]; >> + irq_type = data->buf[NVT_TS_PARAMS_IRQ_TYPE]; >> + >> + if (width > NVT_TS_MAX_SIZE || height >= NVT_TS_MAX_SIZE || >> + data->max_touches > NVT_TS_MAX_TOUCHES || >> + irq_type >= ARRAY_SIZE(nvt_ts_irq_type) || >> + data->buf[NVT_TS_PARAMS_WAKE_TYPE] != NVT_TS_SUPPORTED_WAKE_TYPE || >> + data->buf[NVT_TS_PARAMS_CHIP_ID] != NVT_TS_SUPPORTED_CHIP_ID) { >> + dev_err(dev, "Unsupported touchscreen parameters: %*ph\n", >> + NVT_TS_PARAMS_SIZE, data->buf); >> + return -EIO; > > Nit: because there was no I/O error here necessarily, but rather invalid or > unacceptable values, I think -EINVAL is better here. AFAIK -EINVAL is reserved for invalid function parameters, typically on a syscall / ioctl. Where as here we are receiving invalid data, which is more like a a checksum/crc error which is an IO error. With that said I have no strong preference either way, so let me know if you want me to switch to EINVAL for v2 or not. > >> + } >> + >> + dev_info(dev, "Detected %dx%d touchscreen with %d max touches\n", >> + width, height, data->max_touches); > > This is also idiomatic, but this seems better as dev_dbg. Ack, this was mostly useful during development, I'll change this to dev_dbg() for v2. > >> + >> + if (data->buf[NVT_TS_PARAMS_MAX_BUTTONS]) >> + dev_warn(dev, "Touchscreen buttons are not supported\n"); >> + >> + input = devm_input_allocate_device(dev); >> + if (!input) >> + return -ENOMEM; >> + >> + input->name = client->name; >> + input->id.bustype = BUS_I2C; >> + input->open = nvt_ts_start; >> + input->close = nvt_ts_stop; >> + input->dev.parent = dev; > > devm_input_allocate_device() already sets the parent for us. Ah, nice. I'll drop this line then. > >> + >> + input_set_abs_params(input, ABS_MT_POSITION_X, 0, width - 1, 0, 0); >> + input_set_abs_params(input, ABS_MT_POSITION_Y, 0, height - 1, 0, 0); >> + touchscreen_parse_properties(input, true, &data->prop); >> + >> + ret = input_mt_init_slots(input, data->max_touches, >> + INPUT_MT_DIRECT | INPUT_MT_DROP_UNUSED); >> + if (ret) >> + return ret; >> + >> + data->input = input; >> + input_set_drvdata(input, data); >> + >> + ret = devm_request_threaded_irq(dev, client->irq, NULL, nvt_ts_irq, >> + IRQF_ONESHOT | IRQF_NO_AUTOEN | nvt_ts_irq_type[irq_type], >> + client->name, data); > > Interesting, it seems interrupt polarity is configurable? On the controller-side, yes. The goodix touchscreens have much the same thing. > For my own > understanding, what if there is an inverter on the board? Then things break I guess since we program the GPIO input IRQs polarity to match the controller polarity when then will be wrong. Luckily this has never happened so far AFAIK (mostly talking goodix here, since I know only 1 device with this new touchscreen). > Is the > expectation that the customer reprograms the controller's firmware? > >> + if (ret) >> + return dev_err_probe(dev, ret, "requesting irq\n"); > > dev_err_probe() tends not to be accepted in input, the argument being > that the callers who can return EPROBE_DEFER be responsible for setting > the reason as opposed to every driver calling a separate function that > does so. To me dev_err_probe() is not so much about setting the probe-defer reason, it is is very useful because: 1) It deals with not logging afalse-postivive error msg on -EPROBE_DEFER and you can return its return value, leading to much more compact code and thus IMOH more readable code 2) It leads to a consistent format for the printed errors To illustrate 1. without dev_err_probe() the reset_gpio request error handling turns from this: if (IS_ERR(data->reset_gpio)) return dev_err_probe(dev, PTR_ERR(data->reset_gpio), "requesting reset GPIO\n"); into: if (IS_ERR(data->reset_gpio)) { error = PTR_ERR(data->reset_gpio); if (error == -EPROBE_DEFER) return error; dev_err(dev, "Error %d requesting reset GPIO\n", error); return error; } Which is 7 lines vs 2 lines when using dev_err_probe() and more importantly IMHO the error handling code using using dev_err_probe() is just much more readable and thus more maintainable IMHO. Which is why IMHO using dev_err_probe() for errors getting resources is just much better. So unless you feel really strongly about not using this I would prefer to keep using dev_err_probe(). >> + >> + return input_register_device(input); >> +} >> + >> +static const struct i2c_device_id nvt_ts_i2c_id[] = { >> + { "NVT-ts" }, >> + { } >> +}; >> +MODULE_DEVICE_TABLE(i2c, nvt_ts_i2c_id); >> + >> +static struct i2c_driver nvt_ts_driver = { >> + .driver = { >> + .name = "novatek-nvt-ts", >> + .pm = &nvt_ts_pm_ops, > > I believe we need pm_sleep_ptr() here now. Ack, will fix for v2. >> + }, >> + .probe_new = nvt_ts_probe, >> + .id_table = nvt_ts_i2c_id, >> +}; >> + >> +module_i2c_driver(nvt_ts_driver); >> + >> +MODULE_DESCRIPTION("Novatek NVT-ts touchscreen driver"); >> +MODULE_AUTHOR("Hans de Goede <hdegoede@xxxxxxxxxx>"); >> +MODULE_LICENSE("GPL"); >> -- >> 2.39.1 >> Once more thank you for the review. If you can clarify what you want for the EINVAL vs EIO and for the dev_err_probe() review remarks then I'll prepare a v2. Regards, Hans
