Date:   Thu, 10 Feb 2022 18:01:03 -0600
From:   Jeff LaBundy <jeff@...undy.com>
To:     Markuss Broks <markuss.broks@...il.com>
Cc:     linux-kernel@...r.kernel.org, phone-devel@...r.kernel.org,
        ~postmarketos/upstreaming@...ts.sr.ht,
        Dmitry Torokhov <dmitry.torokhov@...il.com>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski@...onical.com>,
        Henrik Rydberg <rydberg@...math.org>,
        Stephen Rothwell <sfr@...b.auug.org.au>,
        linux-input@...r.kernel.org, devicetree@...r.kernel.org
Subject: Re: [PATCH v2 2/2] Input: add Imagis touchscreen driver

Hi Markuss,

Neat little driver! Some humble feedback below.

On Thu, Feb 10, 2022 at 06:37:07PM +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@...il.com>
> ---
>  MAINTAINERS                        |   6 +
>  drivers/input/touchscreen/Kconfig  |  10 +
>  drivers/input/touchscreen/Makefile |   1 +
>  drivers/input/touchscreen/imagis.c | 329 +++++++++++++++++++++++++++++
>  4 files changed, 346 insertions(+)
>  create mode 100644 drivers/input/touchscreen/imagis.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index a899828a8d4e..f7f717ae926a 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -9405,6 +9405,12 @@ F:	Documentation/devicetree/bindings/iio/afe/current-sense-shunt.yaml
>  F:	Documentation/devicetree/bindings/iio/afe/voltage-divider.yaml
>  F:	drivers/iio/afe/iio-rescale.c
>  
> +IMAGIS TOUCHSCREEN DRIVER
> +M:	Markuss Broks <markuss.broks@...il.com>
> +S:	Maintained
> +F:	Documentation/devicetree/bindings/input/touchscreen/imagis,ist3038c.yaml
> +F:	drivers/input/touchscreen/imagis.c
> +
>  IKANOS/ADI EAGLE ADSL USB DRIVER
>  M:	Matthieu Castet <castet.matthieu@...e.fr>
>  M:	Stanislaw Gruszka <stf_xl@...pl>
> diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
> index 2f6adfb7b938..6810b4b094e8 100644
> --- a/drivers/input/touchscreen/Kconfig
> +++ b/drivers/input/touchscreen/Kconfig
> @@ -1367,4 +1367,14 @@ config TOUCHSCREEN_ZINITIX
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called zinitix.
>  
> +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.
> +
>  endif
> diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile
> index 39a8127cf6a5..989bb1d563d3 100644
> --- a/drivers/input/touchscreen/Makefile
> +++ b/drivers/input/touchscreen/Makefile
> @@ -115,3 +115,4 @@ obj-$(CONFIG_TOUCHSCREEN_ROHM_BU21023)	+= rohm_bu21023.o
>  obj-$(CONFIG_TOUCHSCREEN_RASPBERRYPI_FW)	+= raspberrypi-ts.o
>  obj-$(CONFIG_TOUCHSCREEN_IQS5XX)	+= iqs5xx.o
>  obj-$(CONFIG_TOUCHSCREEN_ZINITIX)	+= zinitix.o
> +obj-$(CONFIG_TOUCHSCREEN_IMAGIS)	+= imagis.o
> diff --git a/drivers/input/touchscreen/imagis.c b/drivers/input/touchscreen/imagis.c
> new file mode 100644
> index 000000000000..308f097a95c1
> --- /dev/null
> +++ b/drivers/input/touchscreen/imagis.c
> @@ -0,0 +1,329 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +
> +#include <linux/delay.h>
> +#include <linux/i2c.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/property.h>
> +#include <linux/input.h>
> +#include <linux/input/mt.h>
> +#include <linux/input/touchscreen.h>
> +
> +#define IST3038_ADDR_LEN		4
> +#define IST3038_DATA_LEN		4
> +#define IST3038_HIB_ACCESS		(0x800B << 16)
> +#define IST3038_DIRECT_ACCESS   BIT(31)
> +#define IST3038_REG_CHIPID      0x40001000
> +
> +#define IST3038_REG_HIB_BASE		(0x30000100)
> +#define IST3038_REG_TOUCH_STATUS        (IST3038_REG_HIB_BASE | IST3038_HIB_ACCESS)
> +#define IST3038_REG_TOUCH_COORD        (IST3038_REG_HIB_BASE | IST3038_HIB_ACCESS | 0x8)
> +#define IST3038_REG_INTR_MESSAGE        (IST3038_REG_HIB_BASE | IST3038_HIB_ACCESS | 0x4)
> +
> +#define IST3038C_WHOAMI			0x38c
> +#define CHIP_ON_DELAY				60 // ms
> +
> +#define I2C_RETRY_COUNT			3
> +
> +#define MAX_SUPPORTED_FINGER_NUM		10

Please prefix all #defines with a common namespace (e.g. IST3038C).

> +
> +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)
> +{
> +	unsigned int reg_be = __cpu_to_be32(reg);

Technically this type should be __be32. Also, why to use __cpu_to_be32 in
place of cpu_to_be32?

> +	struct i2c_msg msg[] = {
> +		{
> +			.addr = ts->client->addr,
> +			.flags = 0,
> +			.buf = (unsigned char *)&reg_be,
> +			.len = IST3038_ADDR_LEN,

I do not think we need these #defines; just use sizeof(reg_be).

> +		}, {
> +			.addr = ts->client->addr,
> +			.flags = I2C_M_RD,
> +			.buf = (unsigned char *)buffer,
> +			.len = IST3038_DATA_LEN,

Same here.

> +		},
> +	};
> +	int res;

For these return variables that may be positive or negative, it is more
common to use 'ret'.

> +	int error;
> +	int retry = I2C_RETRY_COUNT;
> +
> +	do {
> +		res = i2c_transfer(ts->client->adapter, msg, ARRAY_SIZE(msg));
> +		if (res == ARRAY_SIZE(msg)) {
> +			*buffer = __be32_to_cpu(*buffer);
> +			return 0;
> +		}
> +
> +		error = res < 0 ? res : -EIO;
> +		dev_err(&ts->client->dev,
> +			"%s - i2c_transfer failed: %d (%d)\n",
> +			__func__, error, res);

Does this controller suffer some sort of erratum that requires I2C reads
to be retried? If so, it would be handy to include a comment here as to
why we do not expect the read to be successful right away.

> +	} 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 ret, i, finger_count, finger_pressed;

Please use 'error' for return values that only return 0 or an -errno.

> +
> +	ret = imagis_i2c_read_reg(ts, IST3038_REG_INTR_MESSAGE, &intr_message);
> +	if (ret) {
> +		dev_err(&ts->client->dev, "failed to read the interrupt message\n");
> +		return IRQ_HANDLED;
> +	}
> +
> +	finger_count = (intr_message >> 12) & 0xF;
> +	finger_pressed = intr_message & 0x3FF;

Please #define bit shift and masks, with GENMASK used for the latter.

> +	if (finger_count > 10) {
> +		dev_err(&ts->client->dev, "finger count is more than maximum supported\n");
> +		return IRQ_HANDLED;
> +	}

You seem to have #defined the maximum finger count but it is not used here.

> +
> +	for (i = 0; i < finger_count; i++) {
> +		ret = imagis_i2c_read_reg(ts, IST3038_REG_TOUCH_COORD + (i * 4), &finger_status);
> +		if (ret) {
> +			dev_err(&ts->client->dev, "failed to read coordinates for finger %d\n", i);
> +			return IRQ_HANDLED;
> +		}

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.

> +		input_mt_slot(ts->input_dev, i);
> +		input_mt_report_slot_state(ts->input_dev, MT_TOOL_FINGER,
> +					   (finger_pressed & BIT(i)) ? 1 : 0);

No need for the ternary operator here; just pass the boolean as-is.

> +		touchscreen_report_pos(ts->input_dev, &ts->prop,
> +				       (finger_status >> 12) & 0xFFF, finger_status & 0xFFF, 1);
> +		input_report_abs(ts->input_dev, ABS_MT_TOUCH_MAJOR, (finger_status >> 24) & 0xFFF);
> +	}
> +	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(CHIP_ON_DELAY);
> +
> +	enable_irq(ts->client->irq);

This is going to cause unbalanced irq enable/disable because you're calling
this function from probe.

> +	return 0;
> +}
> +
> +static int imagis_stop(struct imagis_ts *ts)
> +{
> +	int error;
> +
> +	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;
> +	}
> +
> +	return 0;

This is largely personal preference, but this is shorter:

error = ...
if (error)
        dev_err(...);

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) {
> +		dev_err(&ts->client->dev,
> +			"Failed to allocate input device.");
> +		return -ENOMEM;
> +	}

No need for a message here.

> +
> +	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_WIDTH_MAJOR, 0, 255, 0, 0);

WIDTH is advertised here but never reported in the interrupt handler.

> +	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;
> +	}
> +
> +	error = input_mt_init_slots(input_dev, 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;
> +	}

I suggest using the device-managed version here, as you have no remove callback.

> +
> +	return 0;
> +}
> +
> +static int imagis_init_regulators(struct imagis_ts *ts)
> +{
> +	struct i2c_client *client = ts->client;
> +	int error;
> +
> +	ts->supplies[0].supply = "vdd";
> +	ts->supplies[1].supply = "vddio";

How does this work?

> +	error = devm_regulator_bulk_get(&client->dev,
> +					ARRAY_SIZE(ts->supplies),
> +					ts->supplies);
> +	if (error < 0) {
> +		dev_err(&client->dev, "Failed to get regulators: %d\n", error);
> +		return error;
> +	}
> +
> +	return 0;
> +}
> +
> +static int imagis_probe(struct i2c_client *i2c)
> +{
> +	struct device *dev;
> +	struct imagis_ts *ts;
> +	int chip_id, ret;
> +
> +	dev = &i2c->dev;
> +
> +	ts = devm_kzalloc(dev, sizeof(*ts), GFP_KERNEL);
> +	if (!ts)
> +		return -ENOMEM;
> +
> +	ts->client = i2c;
> +
> +	ret = devm_request_threaded_irq(dev, i2c->irq,
> +					NULL, imagis_interrupt,
> +					IRQF_ONESHOT | IRQF_TRIGGER_FALLING,
> +					"imagis-touchscreen", ts);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "IRQ allocation failure: %d\n", ret);

Are you sure it's safe to enable interrupts before the controller has
been powered?

> +
> +	ret = imagis_init_regulators(ts);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "regulator init error: %d\n", ret);
> +
> +	ret = imagis_start(ts);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "regulator enable error: %d\n", ret);
> +
> +	ret = imagis_i2c_read_reg(ts, IST3038_REG_CHIPID | IST3038_DIRECT_ACCESS, &chip_id);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "chip ID read failure: %d\n", ret);
> +
> +	if (chip_id == IST3038C_WHOAMI)
> +		dev_info(dev, "Detected IST3038C chip\n");

This should be dev_dbg, if at all.

> +	else
> +		return dev_err_probe(dev, -EINVAL, "unknown chip ID: 0x%x\n", chip_id);

Usually you want to ID the controller as early as possibe, to avoid wasting
time allocating resources if there is a problem.

> +
> +	ret = imagis_init_input_dev(ts);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "input subsystem init error: %d\n", ret);
> +

Just for my own understanding, this controller needs no configuration or
register writes after power-on?

> +	return 0;
> +}
> +
> +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);

I would prefer to salvage the return value and return it after mutex unlock.

> +
> +	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);
> +
> +	mutex_lock(&ts->input_dev->mutex);
> +
> +	if (input_device_enabled(ts->input_dev))
> +		imagis_start(ts);
> +
> +	mutex_unlock(&ts->input_dev->mutex);
> +
> +	return 0;
> +}
> +
> +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);
> +#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,
> +};
> +
> +module_i2c_driver(imagis_ts_driver);
> +
> +MODULE_DESCRIPTION("Imagis IST3038C Touchscreen Driver");
> +MODULE_AUTHOR("Markuss Broks <markuss.broks@...il.com>");
> +MODULE_LICENSE("GPL");
> -- 
> 2.35.0
> 

Kind regards,
Jeff LaBundy

Powered by blists - more mailing lists