lkml.org 
[lkml]   [2022]   [Feb]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
SubjectRe: [PATCH v5 2/2] Input: add Imagis touchscreen driver
From
>
> 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 *)&reg_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

\
 
 \ /
  Last update: 2022-02-20 20:27    [W:1.102 / U:0.340 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site