Messages in this thread Patch in this message | | | Date | Sun, 20 Feb 2022 21:26:59 +0200 | Subject | Re: [PATCH v5 2/2] Input: add Imagis touchscreen driver | From | Markuss Broks <> |
| |
> > Did checkpatch not complain about the C++ style comment? > I always run "./scripts/checkpatch.pl --strict patches/v5/*.patch" before sending, and it hasn't reported any issues with that.
> Please follow the kernel style guidelines for comments. I was also > hoping to see more detail as to why this is necessary. The loop in > and of itself is enough to suggest the controller may fail, but why? > > Does the datasheet or an errata document ask for this?
The only source of information on the touchscreen driver I have is the downstream driver, and it has retries for i2c reads/writes. However, during testing I've never got an error reading any of the registers, and I wonder if I can just drop it.
> Can the controller actually report more than 10 contacts? > > If so, what is the reason to bail? Is this the controller's way of saying > that the remaining register contents may be garbage? > > Or, can the controller report more contacts than it has available registers > for X/Y? That seems odd. > > These kind of controller-specific properties are handy to accompany with a > comment.
I added that as a guard against reporting more touches than possible, because it potentially can cause kernel oops. Is it not needed? I can't check how many touches it can actually report because the touchscreen used for testing only supports two touch points.
> Just confirming: does this controller require you to write the resolution > back to any internal scaling registers?
I believe it doesn't: the downstream driver does nothing of that sort. There is a register where x and y resolution is stored, but the downstream driver never uses it to set the resolution.
> You haven't responded to my question about this pair of statements, as > well as a couple other questions from my previous review.
I re-used this code from zinitix touchscreen driver to get both regulators for turning them on or off at the same time.
> Can this not be done in a bulk read so as to save up to 10 stop/starts? > > Maybe it makes sense to define a bulk read function, with imagis_i2c_read > simply calling the bulk read function with a fixed length.
Regarding this, I'm not sure how do I do this, since for each finger it's read from different i2c registers. It's not possible with one single i2c bulk read, so do I make it 10 separate i2c reads? Wouldn't this go against the purpose of having a bulk read function?
I'm sorry if I haven't responded to any other of your questions.
- Markuss (re-sent in plain text)
On 2/17/22 05:35, Jeff LaBundy wrote: > Hi Markuss, > > On Wed, Feb 16, 2022 at 03:43:01PM +0200, Markuss Broks wrote: >> Add support for the IST3038C touchscreen IC from Imagis, based on >> downstream driver. The driver supports multi-touch (10 touch points) >> The IST3038C IC supports touch keys, but the support isn't added >> because the touch screen used for testing doesn't utilize touch keys. >> Looking at the downstream driver, it is possible to add support >> for other Imagis ICs of IST30**C series. >> >> Signed-off-by: Markuss Broks <markuss.broks@gmail.com> >> --- >> MAINTAINERS | 6 + >> drivers/input/touchscreen/Kconfig | 10 + >> drivers/input/touchscreen/Makefile | 1 + >> drivers/input/touchscreen/imagis.c | 341 +++++++++++++++++++++++++++++ >> 4 files changed, 358 insertions(+) >> create mode 100644 drivers/input/touchscreen/imagis.c >> >> diff --git a/MAINTAINERS b/MAINTAINERS >> index a899828a8d4e..3b99c60e9f4b 100644 >> --- a/MAINTAINERS >> +++ b/MAINTAINERS >> @@ -9411,6 +9411,12 @@ M: Stanislaw Gruszka <stf_xl@wp.pl> >> S: Maintained >> F: drivers/usb/atm/ueagle-atm.c >> >> +IMAGIS TOUCHSCREEN DRIVER >> +M: Markuss Broks <markuss.broks@gmail.com> >> +S: Maintained >> +F: Documentation/devicetree/bindings/input/touchscreen/imagis,ist3038c.yaml >> +F: drivers/input/touchscreen/imagis.c >> + >> IMGTEC ASCII LCD DRIVER >> M: Paul Burton <paulburton@kernel.org> >> S: Maintained >> diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig >> index 2f6adfb7b938..a0f929506228 100644 >> --- a/drivers/input/touchscreen/Kconfig >> +++ b/drivers/input/touchscreen/Kconfig >> @@ -638,6 +638,16 @@ config TOUCHSCREEN_MTOUCH >> To compile this driver as a module, choose M here: the >> module will be called mtouch. >> >> +config TOUCHSCREEN_IMAGIS >> + tristate "Imagis touchscreen support" >> + depends on I2C >> + help >> + Say Y here if you have an Imagis IST30xxC touchscreen. >> + If unsure, say N. >> + >> + To compile this driver as a module, choose M here: the >> + module will be called imagis. > Please adhere to the indentation scheme used throughout the rest of > this file. > >> + >> config TOUCHSCREEN_IMX6UL_TSC >> tristate "Freescale i.MX6UL touchscreen controller" >> depends on ((OF && GPIOLIB) || COMPILE_TEST) && HAS_IOMEM >> diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile >> index 39a8127cf6a5..557f84fd2075 100644 >> --- a/drivers/input/touchscreen/Makefile >> +++ b/drivers/input/touchscreen/Makefile >> @@ -49,6 +49,7 @@ obj-$(CONFIG_TOUCHSCREEN_GOODIX) += goodix_ts.o >> obj-$(CONFIG_TOUCHSCREEN_HIDEEP) += hideep.o >> obj-$(CONFIG_TOUCHSCREEN_ILI210X) += ili210x.o >> obj-$(CONFIG_TOUCHSCREEN_ILITEK) += ilitek_ts_i2c.o >> +obj-$(CONFIG_TOUCHSCREEN_IMAGIS) += imagis.o >> obj-$(CONFIG_TOUCHSCREEN_IMX6UL_TSC) += imx6ul_tsc.o >> obj-$(CONFIG_TOUCHSCREEN_INEXIO) += inexio.o >> obj-$(CONFIG_TOUCHSCREEN_IPROC) += bcm_iproc_tsc.o >> diff --git a/drivers/input/touchscreen/imagis.c b/drivers/input/touchscreen/imagis.c >> new file mode 100644 >> index 000000000000..df26b90371c3 >> --- /dev/null >> +++ b/drivers/input/touchscreen/imagis.c >> @@ -0,0 +1,341 @@ >> +// SPDX-License-Identifier: GPL-2.0-only >> + > Please explicitly include bits.h.
>> +#include <linux/delay.h> >> +#include <linux/i2c.h> >> +#include <linux/input.h> >> +#include <linux/input/mt.h> >> +#include <linux/input/touchscreen.h> >> +#include <linux/kernel.h> >> +#include <linux/module.h> >> +#include <linux/property.h> >> +#include <linux/regulator/consumer.h> >> + >> +#define IST3038C_HIB_ACCESS (0x800B << 16) >> +#define IST3038C_DIRECT_ACCESS BIT(31) >> +#define IST3038C_REG_CHIPID 0x40001000 >> +#define IST3038C_REG_HIB_BASE 0x30000100 >> +#define IST3038C_REG_TOUCH_STATUS (IST3038C_REG_HIB_BASE | IST3038C_HIB_ACCESS) >> +#define IST3038C_REG_TOUCH_COORD (IST3038C_REG_HIB_BASE | IST3038C_HIB_ACCESS | 0x8) >> +#define IST3038C_REG_INTR_MESSAGE (IST3038C_REG_HIB_BASE | IST3038C_HIB_ACCESS | 0x4) >> +#define IST3038C_WHOAMI 0x38c >> +#define IST3038C_CHIP_ON_DELAY 60 // ms > IST3038C_CHIP_ON_DELAY_MS > > Did checkpatch not complain about the C++ style comment? > >> +#define IST3038C_I2C_RETRY_COUNT 3 >> +#define IST3038C_MAX_SUPPORTED_FINGER_NUM 10 >> +#define IST3038C_X_MASK GENMASK(23, 12) >> +#define IST3038C_X_SHIFT 12 >> +#define IST3038C_Y_MASK GENMASK(11, 0) >> +#define IST3038C_AREA_MASK GENMASK(27, 24) >> +#define IST3038C_AREA_SHIFT 24 >> +#define IST3038C_FINGER_COUNT_MASK GENMASK(15, 12) >> +#define IST3038C_FINGER_COUNT_SHIFT 12 >> +#define IST3038C_FINGER_STATUS_MASK GENMASK(9, 0) >> + >> +struct imagis_ts { >> + struct i2c_client *client; >> + struct input_dev *input_dev; >> + struct touchscreen_properties prop; >> + struct regulator_bulk_data supplies[2]; >> +}; >> + >> +static int imagis_i2c_read_reg(struct imagis_ts *ts, >> + unsigned int reg, unsigned int *buffer) >> +{ >> + __be32 reg_be = cpu_to_be32(reg); >> + struct i2c_msg msg[] = { >> + { >> + .addr = ts->client->addr, >> + .flags = 0, >> + .buf = (unsigned char *)®_be, >> + .len = sizeof(reg_be), >> + }, { >> + .addr = ts->client->addr, >> + .flags = I2C_M_RD, >> + .buf = (unsigned char *)buffer, >> + .len = sizeof(reg_be), >> + }, >> + }; >> + int ret, error; >> + int retry = IST3038C_I2C_RETRY_COUNT; >> + >> + do { // The controller might need several reads until it returns a value >> + ret = i2c_transfer(ts->client->adapter, msg, ARRAY_SIZE(msg)); >> + if (ret == ARRAY_SIZE(msg)) { >> + *buffer = be32_to_cpu(*buffer); >> + return 0; >> + } > Please follow the kernel style guidelines for comments. I was also > hoping to see more detail as to why this is necessary. The loop in > and of itself is enough to suggest the controller may fail, but why? > > Does the datasheet or an errata document ask for this? > >> + >> + error = ret < 0 ? ret : -EIO; >> + dev_err(&ts->client->dev, >> + "%s - i2c_transfer failed: %d (%d)\n", >> + __func__, error, ret); >> + } while (--retry); >> + >> + return error; >> +} >> + >> +static irqreturn_t imagis_interrupt(int irq, void *dev_id) >> +{ >> + struct imagis_ts *ts = dev_id; >> + unsigned int finger_status, intr_message; >> + int error, i, finger_count, finger_pressed; >> + >> + error = imagis_i2c_read_reg(ts, IST3038C_REG_INTR_MESSAGE, &intr_message); >> + if (error) { >> + dev_err(&ts->client->dev, "failed to read the interrupt message\n"); >> + return IRQ_HANDLED; >> + } >> + >> + finger_count = (intr_message & IST3038C_FINGER_COUNT_MASK) >> IST3038C_FINGER_COUNT_SHIFT; >> + finger_pressed = intr_message & IST3038C_FINGER_STATUS_MASK; >> + if (finger_count > IST3038C_MAX_SUPPORTED_FINGER_NUM) { >> + dev_err(&ts->client->dev, "finger count is more than maximum supported\n"); >> + return IRQ_HANDLED; >> + } > Can the controller actually report more than 10 contacts? > > If so, what is the reason to bail? Is this the controller's way of saying > that the remaining register contents may be garbage? > > Or, can the controller report more contacts than it has available registers > for X/Y? That seems odd. > > These kind of controller-specific properties are handy to accompany with a > comment. > >> + >> + for (i = 0; i < finger_count; i++) { >> + error = imagis_i2c_read_reg(ts, IST3038C_REG_TOUCH_COORD + (i * 4), &finger_status); >> + if (error) { >> + dev_err(&ts->client->dev, "failed to read coordinates for finger %d\n", i); >> + return IRQ_HANDLED; >> + } >> + input_mt_slot(ts->input_dev, i); >> + input_mt_report_slot_state(ts->input_dev, MT_TOOL_FINGER, >> + finger_pressed & BIT(i)); >> + touchscreen_report_pos(ts->input_dev, &ts->prop, >> + (finger_status & IST3038C_X_MASK) >> IST3038C_X_SHIFT, >> + finger_status & IST3038C_Y_MASK, 1); >> + input_report_abs(ts->input_dev, ABS_MT_TOUCH_MAJOR, >> + (finger_status & IST3038C_AREA_MASK) >> IST3038C_AREA_SHIFT); >> + } >> + input_mt_sync_frame(ts->input_dev); >> + input_sync(ts->input_dev); >> + >> + return IRQ_HANDLED; >> +} >> + >> +static int imagis_start(struct imagis_ts *ts) >> +{ >> + int error; >> + >> + error = regulator_bulk_enable(ARRAY_SIZE(ts->supplies), >> + ts->supplies); >> + if (error) { >> + dev_err(&ts->client->dev, >> + "Failed to enable regulators: %d\n", error); >> + return error; >> + } >> + >> + msleep(IST3038C_CHIP_ON_DELAY); >> + >> + enable_irq(ts->client->irq); >> + return 0; >> +} >> + >> +static int imagis_stop(struct imagis_ts *ts) >> +{ >> + int error = 0; > Nit: any reason to initialize this to zero, then immediately change it below? > >> + >> + disable_irq(ts->client->irq); >> + >> + error = regulator_bulk_disable(ARRAY_SIZE(ts->supplies), >> + ts->supplies); >> + if (error) >> + dev_err(&ts->client->dev, >> + "Failed to disable regulators: %d\n", error); >> + return error; >> +} >> + >> +static int imagis_input_open(struct input_dev *dev) >> +{ >> + struct imagis_ts *ts = input_get_drvdata(dev); >> + >> + return imagis_start(ts); >> +} >> + >> +static void imagis_input_close(struct input_dev *dev) >> +{ >> + struct imagis_ts *ts = input_get_drvdata(dev); >> + >> + imagis_stop(ts); >> +} >> + >> +static int imagis_init_input_dev(struct imagis_ts *ts) >> +{ >> + struct input_dev *input_dev; >> + int error; >> + >> + input_dev = devm_input_allocate_device(&ts->client->dev); >> + if (!input_dev) >> + return -ENOMEM; >> + >> + ts->input_dev = input_dev; >> + >> + input_dev->name = "Imagis capacitive touchscreen"; >> + input_dev->phys = "input/ts"; >> + input_dev->id.bustype = BUS_I2C; >> + input_dev->open = imagis_input_open; >> + input_dev->close = imagis_input_close; >> + >> + input_set_drvdata(input_dev, ts); >> + >> + input_set_capability(input_dev, EV_ABS, ABS_MT_POSITION_X); >> + input_set_capability(input_dev, EV_ABS, ABS_MT_POSITION_Y); >> + input_set_abs_params(input_dev, ABS_MT_TOUCH_MAJOR, 0, 255, 0, 0); >> + >> + touchscreen_parse_properties(input_dev, true, &ts->prop); >> + if (!ts->prop.max_x || !ts->prop.max_y) { >> + dev_err(&ts->client->dev, >> + "Touchscreen-size-x and/or touchscreen-size-y not set in dts\n"); >> + return -EINVAL; >> + } > Just confirming: does this controller require you to write the resolution > back to any internal scaling registers? > >> + >> + error = input_mt_init_slots(input_dev, IST3038C_MAX_SUPPORTED_FINGER_NUM, >> + INPUT_MT_DIRECT | INPUT_MT_DROP_UNUSED); >> + if (error) { >> + dev_err(&ts->client->dev, >> + "Failed to initialize MT slots: %d", error); >> + return error; >> + } >> + >> + error = input_register_device(input_dev); >> + if (error) { >> + dev_err(&ts->client->dev, >> + "Failed to register input device: %d", error); >> + return error; >> + } >> + >> + return 0; >> +} >> + >> +static int imagis_init_regulators(struct imagis_ts *ts) >> +{ >> + struct i2c_client *client = ts->client; >> + int error = 0; >> + >> + ts->supplies[0].supply = "vdd"; >> + ts->supplies[1].supply = "vddio"; > You haven't responded to my question about this pair of statements, as > well as a couple other questions from my previous review. > >> + error = devm_regulator_bulk_get(&client->dev, >> + ARRAY_SIZE(ts->supplies), >> + ts->supplies); >> + >> + return error; >> +} >> + >> +static int imagis_probe(struct i2c_client *i2c) >> +{ >> + struct device *dev; >> + struct imagis_ts *ts; >> + int chip_id, ret, error; >> + >> + dev = &i2c->dev; >> + >> + ts = devm_kzalloc(dev, sizeof(*ts), GFP_KERNEL); >> + if (!ts) >> + return -ENOMEM; >> + >> + ts->client = i2c; >> + >> + ret = imagis_init_regulators(ts); >> + if (ret) >> + return dev_err_probe(dev, ret, "regulator init error: %d\n", ret); >> + > As mentioned earlier, please use 'error' for return variables that only > return zero or negative values. > >> + ret = regulator_bulk_enable(ARRAY_SIZE(ts->supplies), >> + ts->supplies); >> + if (ret) >> + return dev_err_probe(dev, ret, "failed to enable regulators: %d\n", ret); >> + >> + msleep(IST3038C_CHIP_ON_DELAY); >> + >> + ret = imagis_i2c_read_reg(ts, IST3038C_REG_CHIPID | IST3038C_DIRECT_ACCESS, &chip_id); >> + if (ret) { >> + dev_err(dev, "chip ID read failure: %d\n", ret); >> + goto error_probe; >> + } >> + >> + if (chip_id == IST3038C_WHOAMI) { >> + dev_dbg(dev, "Detected IST3038C chip\n"); >> + } else { >> + dev_err(dev, "unknown chip ID: 0x%x\n", chip_id); >> + ret = -EINVAL; >> + goto error_probe; >> + } > It's personal preference, but this seems cleaner: > > if (chip_id != ...) { > /* ... */ > goto ... > } > > dev_dbg(...); > > And again, it's personal preference, but the dev_dbg is a bit pointless. > The presence of the device in /dev/input is enough to show the device > made it through probe. > >> + >> + ret = devm_request_threaded_irq(dev, i2c->irq, >> + NULL, imagis_interrupt, >> + IRQF_ONESHOT | IRQF_NO_AUTOEN, >> + "imagis-touchscreen", ts); >> + if (ret) { >> + dev_err(dev, "IRQ allocation failure: %d\n", ret); >> + goto error_probe; >> + } > I think you have unbalanced regulator enable/disable here. Based on > the above flags, it seems you are relying on the open callback to > call imagis_start which then enables interrupts. > > However, that also enables regulators once more which will increase > the refcount. Even if the close callback runs before the driver > is detached, the regulators will be left enabled. > > Maybe you need to add a device-managed action to disable regulators; > this will also allow you to get rid of the teardown path below the > error_probe label as well as a second return variable. > >> + >> + ret = imagis_init_input_dev(ts); >> + if (ret) { >> + dev_err(dev, "input subsystem init error: %d\n", ret); >> + goto error_probe; >> + } >> + return 0; >> + >> +error_probe: >> + error = regulator_bulk_disable(ARRAY_SIZE(ts->supplies), >> + ts->supplies); >> + if (error) >> + dev_err(dev, "Failed to disable regulators: %d\n", error); >> + return ret; >> +} >> + >> +static int __maybe_unused imagis_suspend(struct device *dev) >> +{ >> + struct i2c_client *client = to_i2c_client(dev); >> + struct imagis_ts *ts = i2c_get_clientdata(client); >> + >> + mutex_lock(&ts->input_dev->mutex); >> + >> + if (input_device_enabled(ts->input_dev)) >> + imagis_stop(ts); > Again, please pass the return value as you have done below. > >> + >> + mutex_unlock(&ts->input_dev->mutex); >> + >> + return 0; >> +} >> + >> +static int __maybe_unused imagis_resume(struct device *dev) >> +{ >> + struct i2c_client *client = to_i2c_client(dev); >> + struct imagis_ts *ts = i2c_get_clientdata(client); >> + int ret = 0; > Again, please use 'error' here. > >> + >> + mutex_lock(&ts->input_dev->mutex); >> + >> + if (input_device_enabled(ts->input_dev)) >> + ret = imagis_start(ts); >> + >> + mutex_unlock(&ts->input_dev->mutex); >> + >> + return ret; >> +} >> + >> +static SIMPLE_DEV_PM_OPS(imagis_pm_ops, imagis_suspend, imagis_resume); >> + >> +#ifdef CONFIG_OF >> +static const struct of_device_id imagis_of_match[] = { >> + { .compatible = "imagis,ist3038c", }, >> + { }, >> +}; >> +MODULE_DEVICE_TABLE(i2c, imagis_of_match); > MODULE_DEVICE_TABLE(of, ...); > >> +#endif >> + >> +static struct i2c_driver imagis_ts_driver = { >> + .driver = { >> + .name = "imagis-touchscreen", >> + .pm = &imagis_pm_ops, >> + .of_match_table = of_match_ptr(imagis_of_match), >> + }, >> + .probe_new = imagis_probe, > No extraneous spaces please. > >> +}; >> + >> +module_i2c_driver(imagis_ts_driver); >> + >> +MODULE_DESCRIPTION("Imagis IST3038C Touchscreen Driver"); >> +MODULE_AUTHOR("Markuss Broks <markuss.broks@gmail.com>"); >> +MODULE_LICENSE("GPL"); >> -- >> 2.35.0 >> > Kind regards, > Jeff LaBundy
| |