Linux Input/HID development
 help / color / mirror / Atom feed

* Re: [PATCH 2/5] Input: add driver for Focaltech FTS touchscreen
From: Jeff LaBundy @ 2023-03-14  3:42 UTC (permalink / raw)
  To: Joel Selvaraj
  Cc: Caleb Connolly, Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski,
	Andy Gross, Bjorn Andersson, Konrad Dybcio, Henrik Rydberg,
	Arnd Bergmann, Robert Jarzmik, Neil Armstrong, Markuss Broks,
	Jean Delvare, Max Krummenacher, Job Noorman, Alistair Francis,
	Chris Morgan, linux-input, devicetree, linux-kernel,
	linux-arm-msm, ~postmarketos/upstreaming, phone-devel
In-Reply-To: <20230312093249.1846993-3-joelselvaraj.oss@gmail.com>

Hi Joel (and Caleb),

Great work! It's nice to see these devices get one step closer to
upstream. In addition to Markuss' valuable feedback, I have left a
few comments throughout.

On Sun, Mar 12, 2023 at 04:32:46AM -0500, Joel Selvaraj wrote:
> The Focaltech FTS driver supports several variants of focaltech
> touchscreens found in ~2018 era smartphones including variants found on
> the PocoPhone F1 and the SHIFT6mq which are already present in mainline.
> This driver is loosely based on the original driver from Focaltech
> but has been simplified and largely reworked.
> 
> Co-developed-by: Caleb Connolly <caleb@connolly.tech>
> Signed-off-by: Caleb Connolly <caleb@connolly.tech>
> Signed-off-by: Joel Selvaraj <joelselvaraj.oss@gmail.com>
> ---
>  MAINTAINERS                               |   8 +
>  drivers/input/touchscreen/Kconfig         |  12 +
>  drivers/input/touchscreen/Makefile        |   1 +
>  drivers/input/touchscreen/focaltech_fts.c | 448 ++++++++++++++++++++++
>  4 files changed, 469 insertions(+)
>  create mode 100644 drivers/input/touchscreen/focaltech_fts.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 2892858cb040..deb561c356f2 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -7993,6 +7993,14 @@ L:	linux-input@vger.kernel.org
>  S:	Maintained
>  F:	drivers/input/joystick/fsia6b.c
>  
> +FOCALTECH FTS TOUCHSCREEN DRIVER
> +M:	Joel Selvaraj <joelselvaraj.oss@gmail.com>
> +M:	Caleb Connolly <caleb@connolly.tech>
> +L:	linux-input@vger.kernel.org
> +S:	Maintained
> +F:	Documentation/devicetree/bindings/input/touchscreen/focaltech,fts.yaml
> +F:	drivers/input/touchscreen/focaltech_fts.c
> +
>  FOCUSRITE SCARLETT GEN 2/3 MIXER DRIVER
>  M:	Geoffrey D. Bennett <g@b4.vu>
>  L:	alsa-devel@alsa-project.org (moderated for non-subscribers)
> diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
> index 1a2049b336a6..320925bac3a1 100644
> --- a/drivers/input/touchscreen/Kconfig
> +++ b/drivers/input/touchscreen/Kconfig
> @@ -388,6 +388,18 @@ config TOUCHSCREEN_EXC3000
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called exc3000.
>  
> +config TOUCHSCREEN_FOCALTECH_FTS
> +	tristate "Focaltech FTS Touchscreen"
> +	depends on I2C
> +	help
> +	  Say Y here to enable support for I2C connected Focaltech FTS
> +	  based touch panels, including the 5452 and 8917 panels.
> +
> +	  If unsure, say N.
> +
> +	  To compile this driver as a module, choose M here: the
> +	  module will be called focaltech_fts.
> +
>  config TOUCHSCREEN_FUJITSU
>  	tristate "Fujitsu serial touchscreen"
>  	select SERIO
> diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile
> index f2fd28cc34a6..83ea2e3ce754 100644
> --- a/drivers/input/touchscreen/Makefile
> +++ b/drivers/input/touchscreen/Makefile
> @@ -45,6 +45,7 @@ obj-$(CONFIG_TOUCHSCREEN_ELO)		+= elo.o
>  obj-$(CONFIG_TOUCHSCREEN_EGALAX)	+= egalax_ts.o
>  obj-$(CONFIG_TOUCHSCREEN_EGALAX_SERIAL)	+= egalax_ts_serial.o
>  obj-$(CONFIG_TOUCHSCREEN_EXC3000)	+= exc3000.o
> +obj-$(CONFIG_TOUCHSCREEN_FOCALTECH_FTS)	+= focaltech_fts.o
>  obj-$(CONFIG_TOUCHSCREEN_FUJITSU)	+= fujitsu_ts.o
>  obj-$(CONFIG_TOUCHSCREEN_GOODIX)	+= goodix_ts.o
>  obj-$(CONFIG_TOUCHSCREEN_HIDEEP)	+= hideep.o
> diff --git a/drivers/input/touchscreen/focaltech_fts.c b/drivers/input/touchscreen/focaltech_fts.c
> new file mode 100644
> index 000000000000..d389c8b88944
> --- /dev/null
> +++ b/drivers/input/touchscreen/focaltech_fts.c
> @@ -0,0 +1,448 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + *
> + * FocalTech touchscreen driver.
> + *
> + * Copyright (c) 2010-2017, FocalTech Systems, Ltd., all rights reserved.
> + * Copyright (C) 2018 XiaoMi, Inc.
> + * Copyright (c) 2021 Caleb Connolly <caleb@connolly.tech>
> + * Copyright (c) 2023 Joel Selvaraj <joelselvaraj.oss@gmail.com>
> + *
> + * This software is licensed under the terms of the GNU General Public
> + * License version 2, as published by the Free Software Foundation, and
> + * may be copied, distributed, and modified under those terms.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.

The SPDX tag relieves you of the need to repeat any license terms. These
two paragraphs can be dropped.

> + *
> + */
> +
> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +#include <linux/input.h>
> +#include <linux/input/mt.h>
> +#include <linux/input/touchscreen.h>
> +#include <linux/slab.h>
> +#include <linux/interrupt.h>
> +#include <linux/i2c.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/delay.h>
> +#include <linux/regmap.h>
> +#include <linux/mutex.h>
> +
> +#define FTS_REG_CHIP_ID_H 0xA3
> +#define FTS_REG_CHIP_ID_L 0x9F
> +
> +#define FTS_MAX_POINTS_SUPPORT 10
> +#define FTS_ONE_TOUCH_LEN 6
> +
> +#define FTS_TOUCH_X_H_OFFSET 3
> +#define FTS_TOUCH_X_L_OFFSET 4
> +#define FTS_TOUCH_Y_H_OFFSET 5
> +#define FTS_TOUCH_Y_L_OFFSET 6
> +#define FTS_TOUCH_PRESSURE_OFFSET 7
> +#define FTS_TOUCH_AREA_OFFSET 8
> +#define FTS_TOUCH_TYPE_OFFSET 3
> +#define FTS_TOUCH_ID_OFFSET 5
> +
> +#define FTS_TOUCH_DOWN 0
> +#define FTS_TOUCH_UP 1
> +#define FTS_TOUCH_CONTACT 2
> +
> +#define FTS_DRIVER_NAME "fts-i2c"
> +#define INTERVAL_READ_REG 100 /* unit:ms */
> +#define TIMEOUT_READ_REG 2000 /* unit:ms */

In addition to what Markuss has mentioned, please namespace all #defines
(i.e. FTS_xxx). I also find these easier to read if they are all aligned
horizontally.

> +
> +#define CHIP_TYPE_5452 0x5452
> +#define CHIP_TYPE_8719 0x8719

A #define with the name equal to the value is a bit unnecessary; below I
will suggest an alternative.

> +
> +struct fts_ts_data {
> +	struct i2c_client *client;
> +	struct input_dev *input_dev;
> +	struct touchscreen_properties prop;
> +
> +	struct regmap *regmap;
> +	int irq;
> +
> +	struct regulator_bulk_data regulators[2];
> +
> +	/* Touch data */
> +	u8 max_touch_number;
> +	u8 *point_buf;
> +	int point_buf_size;
> +
> +	/* DT data */
> +	struct gpio_desc *reset_gpio;
> +};

Technically the touchscreen_properties can be DT data too. I think the
comments and newlines in the struct definition are unnecessary.

> +
> +static const struct regmap_config fts_ts_i2c_regmap_config = {
> +	.reg_bits = 8,
> +	.val_bits = 8,
> +};

I personally find the driver easier to read if this definition is closer
to probe, where it is used.

> +
> +static bool fts_chip_is_valid(struct fts_ts_data *data, u16 id)
> +{
> +	if (id != CHIP_TYPE_5452 && id != CHIP_TYPE_8719)
> +		return false;
> +
> +	return true;
> +}

This isn't particularly scalable; we would end up with a massive if block
as we add compatible devices. Instead, define an array toward the beginning
of the driver as follows:

static const u16 fts_chip_types[] = {
        0x5452,
        0x8719,
};

And then this function could turn into something like this:

        for (i = 0; i < ARRAY_SIZE(fts_chip_types); i++)
                if (id = fts_chip_types[i])
                        return true;

        return false;

This would also get rid of those obvious #defines.

> +
> +int fts_check_status(struct fts_ts_data *data)
> +{
> +	int count = 0;
> +	unsigned int val, id;
> +
> +	do {
> +		regmap_read(data->regmap, FTS_REG_CHIP_ID_H, &val);
> +		id = val << 8;
> +		regmap_read(data->regmap, FTS_REG_CHIP_ID_L, &val);
> +		id |= val;

There are cleaner alternatives for this, such as:

        __be16 val;

        do {
                error = regmap_raw_read(data->regmap, FTS_REG_CHIP_ID_H,
                                        &val, sizeof(val));
                if (error)
                        return error;

                if (fts_chip_is_valid(data, be16_to_cpu(id)) {
                        dev_dbg(...);
                        return 0;
                }

                /* ... */
        }

Please also note that there is no reason for fts_chip_is_valid() to
accept 'data' as an argument. One could also argue this function is
not doing enough work or being re-used enough to warrant being its
own function.

> +
> +		if (fts_chip_is_valid(data, id)) {
> +			dev_dbg(&data->client->dev, "TS Ready: Chip ID = 0x%x", id);
> +			return 0;
> +		}
> +
> +		count++;
> +		msleep(INTERVAL_READ_REG);
> +	} while ((count * INTERVAL_READ_REG) < TIMEOUT_READ_REG);
> +
> +	return -EIO;
> +}
> +
> +static void fts_report_touch(struct fts_ts_data *data)
> +{
> +	struct input_dev *input_dev = data->input_dev;
> +	int base;
> +	unsigned int x, y, z, maj;
> +	u8 slot, type;
> +	int error, i = 0;
> +
> +	u8 *buf = data->point_buf;
> +
> +	memset(buf, 0, data->point_buf_size);
> +
> +	error = regmap_bulk_read(data->regmap, 0, buf, data->point_buf_size);
> +	if (error) {
> +		dev_err_ratelimited(&data->client->dev, "I2C read failed: %d\n",
> +				    error);
> +		return;
> +	}
> +
> +	for (i = 0; i < data->max_touch_number; i++) {
> +		base = FTS_ONE_TOUCH_LEN * i;
> +
> +		slot = (buf[base + FTS_TOUCH_ID_OFFSET]) >> 4;
> +		if (slot >= data->max_touch_number)
> +			break;
> +
> +		x = ((buf[base + FTS_TOUCH_X_H_OFFSET] & 0x0F) << 8) +
> +			 (buf[base + FTS_TOUCH_X_L_OFFSET] & 0xFF);
> +		y = ((buf[base + FTS_TOUCH_Y_H_OFFSET] & 0x0F) << 8) +
> +			 (buf[base + FTS_TOUCH_Y_L_OFFSET] & 0xFF);

It seems that the interrupt status registers can be represented with a packed
struct and then unpacked with be16_to_cpu().

> +
> +		z = buf[base + FTS_TOUCH_PRESSURE_OFFSET];
> +		if (z <= 0)
> +			z = 0x3f;

z is unsigned; how can this happen?

> +
> +		maj = buf[base + FTS_TOUCH_AREA_OFFSET] >> 4;
> +		if (maj <= 0)
> +			maj = 0x09;
> +
> +		type = buf[base + FTS_TOUCH_TYPE_OFFSET] >> 6;
> +
> +		input_mt_slot(input_dev, slot);
> +		if (type == FTS_TOUCH_DOWN || type == FTS_TOUCH_CONTACT) {
> +			input_mt_report_slot_state(input_dev, MT_TOOL_FINGER, true);
> +			touchscreen_report_pos(data->input_dev, &data->prop, x, y, true);
> +			input_report_abs(input_dev, ABS_MT_PRESSURE, z);
> +			input_report_abs(input_dev, ABS_MT_TOUCH_MAJOR, maj);
> +			input_report_key(data->input_dev, BTN_TOUCH, 1);
> +		} else {
> +			input_report_key(data->input_dev, BTN_TOUCH, 0);
> +			input_mt_report_slot_inactive(input_dev);
> +		}
> +	}
> +	input_sync(input_dev);

We need to call input_mt_sync_frame() as well.

> +}
> +
> +static irqreturn_t fts_ts_interrupt(int irq, void *dev_id)
> +{
> +	struct fts_ts_data *data = dev_id;
> +
> +	fts_report_touch(data);

Assuming the act of reading the interrupt status registers is what prompts the
device to deassert its interrupt pin, I recommend promoting fts_report_touch()
to int type and evaluating its return value as follows:

        return fts_report_touch(data) ? IRQ_NONE : IRQ_HANDLED;

This way if the register read fails and the interrupt is in theory not serviced,
the IRQ subsystem can mute the device and prevent an interrupt storm.

> +
> +	return IRQ_HANDLED;
> +}
> +
> +static void fts_power_off(void *d)
> +{
> +	struct fts_ts_data *data = d;
> +
> +	regulator_bulk_disable(ARRAY_SIZE(data->regulators),
> +			       data->regulators);
> +}
> +
> +static int fts_start(struct fts_ts_data *data)
> +{
> +	int error;
> +
> +	error = regulator_bulk_enable(ARRAY_SIZE(data->regulators),
> +				      data->regulators);
> +	if (error) {
> +		dev_err(&data->client->dev, "failed to enable regulators\n");
> +		return error;
> +	}
> +
> +	gpiod_set_value_cansleep(data->reset_gpio, 0);
> +	msleep(200);
> +
> +	enable_irq(data->irq);
> +
> +	return 0;
> +}
> +
> +static int fts_stop(struct fts_ts_data *data)
> +{
> +	disable_irq(data->irq);
> +	gpiod_set_value_cansleep(data->reset_gpio, 1);
> +	fts_power_off(data);
> +
> +	return 0;
> +}
> +
> +static int fts_input_open(struct input_dev *dev)
> +{
> +	struct fts_ts_data *data = input_get_drvdata(dev);
> +	int error;
> +
> +	error = fts_start(data);
> +	if (error)
> +		return error;
> +
> +	error = fts_check_status(data);
> +	if (error) {
> +		dev_err(&data->client->dev, "Failed to start or unsupported chip");
> +		return error;
> +	}
> +
> +	return 0;
> +}
> +
> +static void fts_input_close(struct input_dev *dev)
> +{
> +	struct fts_ts_data *data = input_get_drvdata(dev);
> +
> +	fts_stop(data);
> +}
> +
> +static int fts_input_init(struct fts_ts_data *data)
> +{
> +	struct device *dev = &data->client->dev;
> +	struct input_dev *input_dev;
> +	int error = 0;
> +
> +	input_dev = devm_input_allocate_device(dev);
> +	if (!input_dev)
> +		return -ENOMEM;
> +
> +	data->input_dev = input_dev;
> +
> +	/* Init and register Input device */

This comment is obvious and not entirely true, as we're not actually registering
the device until farther down.

> +	input_dev->name = FTS_DRIVER_NAME;
> +	input_dev->id.bustype = BUS_I2C;
> +	input_dev->dev.parent = dev;
> +	input_dev->open = fts_input_open;
> +	input_dev->close = fts_input_close;
> +
> +	input_set_drvdata(input_dev, data);
> +
> +	__set_bit(EV_SYN, input_dev->evbit);

No need to set EV_SYN; input_register_device() does that for us.

> +	__set_bit(EV_ABS, input_dev->evbit);

No need to set EV_ABS; input_set_capability(..., EV_ABS, ...) does that for us.

> +	__set_bit(INPUT_PROP_DIRECT, input_dev->propbit);

Same here; input_mt_init_slots() is already doing this.

> +
> +	input_mt_init_slots(input_dev, data->max_touch_number,
> +			    INPUT_MT_DIRECT);

Please check the return value of input_mt_init_slots(), and move this call
_after_ your axes have been defined. Otherwise, this function cannot copy
the multitouch axes to single-touch axes, and drivers such as libinput will
ignore this device.

> +	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);
> +	input_set_abs_params(input_dev, ABS_MT_PRESSURE, 0, 255, 0, 0);
> +
> +	touchscreen_parse_properties(input_dev, true, &data->prop);
> +	if (!data->prop.max_x || !data->prop.max_y) {
> +		dev_err(dev,
> +			"touchscreen-size-x and/or touchscreen-size-y not set in dts\n");

Note that dts may not always be the source of these (see comments below).

> +		return -EINVAL;
> +	}
> +
> +	data->point_buf_size = (data->max_touch_number * FTS_ONE_TOUCH_LEN) + 3;
> +	data->point_buf = devm_kzalloc(dev, data->point_buf_size, GFP_KERNEL);
> +	if (!data->point_buf) {
> +		dev_err(dev, "Failed to alloc memory for point buffer\n");

Did checkpatch not gripe about this? No need to print a warning for failed
memory allocation.

> +		return -ENOMEM;
> +	}
> +
> +	error = input_register_device(input_dev);
> +	if (error) {
> +		dev_err(dev, "Failed to register input device\n");
> +		return error;
> +	}
> +
> +	return 0;
> +}
> +
> +static int fts_parse_dt(struct fts_ts_data *data)
> +{
> +	int error = 0;
> +	struct device *dev = &data->client->dev;
> +	struct device_node *np = dev->of_node;
> +	u32 val;
> +
> +	error = of_property_read_u32(np, "focaltech,max-touch-number", &val);
> +	if (error) {
> +		dev_err(dev, "Unable to read property 'focaltech,max-touch-number'");
> +		return -EINVAL;

Just return 'error' rather than papering over the actual error code.

> +	}

Please switch to the device_property API to allow easy adoption of ACPI
at a later time. To that end, it is not necessarily appropriate to use
the term 'dt' in the name of this function.

That being said, I do agree with Krzysztof's point in the binding review;
it seems you are relying on the user to know the hardware limitation. In
my opinion this is the driver's responsibility.

I think it is OK if you want the user to have the option to artificially
limit the number of points, but in that case the property should be optional.

> +	if (val < 2 || val > FTS_MAX_POINTS_SUPPORT) {
> +		dev_err(dev, "'focaltech,max-touch-number' out of range [2, %d]",
> +			FTS_MAX_POINTS_SUPPORT);
> +		return -EINVAL;
> +	}

Since FTS_MAX_POINTS_SUPPORT exists, it seems FTS_MIN_POINTS_SUPPORT should too.

That being said, why not to support single-touch applications? It does not seem
you write this value back to a device register, so will the device not simply
report one contact if that is all that exists? input_mt_init_slots() is perfectly
happy with one slot.

> +	data->max_touch_number = val;
> +
> +	data->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
> +	if (IS_ERR(data->reset_gpio)) {
> +		error = PTR_ERR(data->reset_gpio);
> +		dev_err(dev, "Failed to request reset gpio, error %d\n", error);
> +		return error;
> +	}

Having a hardware reset pin is nice, but often it is given up in case the board
designer runs out of GPIO and pulls the reset pin up locally. Can it be optional?

> +
> +	return 0;
> +}
> +
> +static int fts_ts_probe(struct i2c_client *client)
> +{
> +	int error = 0;
> +	struct fts_ts_data *data;
> +
> +	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
> +		dev_err(&client->dev, "I2C not supported");
> +		return -ENODEV;
> +	}
> +
> +	if (!client->irq) {
> +		dev_err(&client->dev, "No irq specified\n");
> +		return -EINVAL;
> +	}
> +
> +	data = devm_kzalloc(&client->dev, sizeof(*data), GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;
> +
> +	data->client = client;
> +
> +	error = fts_parse_dt(data);
> +	if (error)
> +		return error;
> +
> +	i2c_set_clientdata(client, data);
> +
> +	data->regmap = devm_regmap_init_i2c(client, &fts_ts_i2c_regmap_config);
> +	if (IS_ERR(data->regmap)) {
> +		dev_err(&client->dev, "regmap allocation failed\n");

It would be nice to save and print the error code as you do above for the
reset GPIO.

> +		return PTR_ERR(data->regmap);
> +	}
> +
> +	/*
> +	 * AVDD is the analog voltage supply (2.6V to 3.3V)
> +	 * VDDIO is the digital voltage supply (1.8V)
> +	 */
> +	data->regulators[0].supply = "avdd";
> +	data->regulators[1].supply = "vddio";
> +	error = devm_regulator_bulk_get(&client->dev, ARRAY_SIZE(data->regulators),
> +				      data->regulators);
> +	if (error) {
> +		dev_err(&client->dev, "Failed to get regulators %d\n", error);
> +		return error;
> +	}
> +
> +	error = devm_add_action_or_reset(&client->dev, fts_power_off, data);
> +	if (error) {
> +		dev_err(&client->dev, "failed to install power off handler\n");
> +		return error;
> +	}
> +
> +	error = devm_request_threaded_irq(&client->dev, client->irq, NULL,
> +					fts_ts_interrupt, IRQF_ONESHOT,
> +					client->name, data);

Nit: alignment

> +	if (error) {
> +		dev_err(&client->dev, "Failed to request IRQ: %d\n", error);
> +		return error;
> +	}
> +
> +	error = fts_input_init(data);
> +	if (error)
> +		return error;
> +
> +	return 0;
> +}
> +
> +static int fts_pm_suspend(struct device *dev)
> +{
> +	struct fts_ts_data *data = dev_get_drvdata(dev);
> +
> +	mutex_lock(&data->input_dev->mutex);
> +
> +	if (input_device_enabled(data->input_dev))
> +		fts_stop(data);
> +
> +	mutex_unlock(&data->input_dev->mutex);
> +
> +	return 0;
> +}
> +
> +static int fts_pm_resume(struct device *dev)
> +{
> +	struct fts_ts_data *data = dev_get_drvdata(dev);
> +	int error = 0;
> +
> +	mutex_lock(&data->input_dev->mutex);
> +
> +	if (input_device_enabled(data->input_dev))
> +		error = fts_start(data);
> +
> +	mutex_unlock(&data->input_dev->mutex);
> +
> +	return error;
> +}
> +
> +static const struct dev_pm_ops fts_dev_pm_ops = {
> +	.suspend = fts_pm_suspend,
> +	.resume = fts_pm_resume,
> +};

Please use the new DEFINE_SIMPLE_DEV_PM_OPS and pm_sleep_ptr().

> +
> +static const struct of_device_id fts_match_table[] = {
> +	{ .compatible = "focaltech,fts5452", },
> +	{ .compatible = "focaltech,fts8719", },
> +	{ /* sentinel */ },

Nit: no need for a trailing comma after the sentinel, as no patch would
ever add a line below it.

> +};
> +
> +MODULE_DEVICE_TABLE(of, fts_match_table);
> +
> +static struct i2c_driver fts_ts_driver = {
> +	.probe_new = fts_ts_probe,
> +	.driver = {
> +		.name = FTS_DRIVER_NAME,
> +		.pm = &fts_dev_pm_ops,
> +		.of_match_table = fts_match_table,
> +	},
> +};
> +module_i2c_driver(fts_ts_driver);
> +
> +MODULE_AUTHOR("Joel Selvaraj <joelselvaraj.oss@gmail.com>");
> +MODULE_AUTHOR("Caleb Connolly <caleb@connolly.tech>");
> +MODULE_DESCRIPTION("FocalTech touchscreen Driver");

Nit: inconsistent capitalization

> +MODULE_LICENSE("GPL");
> -- 
> 2.39.2
> 

Kind regards,
Jeff LaBundy

^ permalink raw reply

* Re: [PATCH v2] Fix buffer overrun in HID-SENSOR name string
From: srinivas pandruvada @ 2023-03-14  1:07 UTC (permalink / raw)
  To: todd.e.brandt, Andi Shyti, Todd Brandt
  Cc: linux-input, linux-iio, linux-kernel, jic23, jikos, p.jungkamp
In-Reply-To: <c5f67e5c49aee0ce48545a79b7295c7af779347f.camel@linux.intel.com>

On Mon, 2023-03-13 at 16:37 -0700, Todd Brandt wrote:
> On Tue, 2023-03-14 at 00:07 +0100, Andi Shyti wrote:
> > Hi Todd,
> > 
> > On Mon, Mar 13, 2023 at 03:06:53PM -0700, Todd Brandt wrote:
> > > On some platforms there are some platform devices created with
> > > invalid names. For example: "HID-SENSOR-INT-020b?.39.auto"
> > > instead
> > > of "HID-SENSOR-INT-020b.39.auto"
> > > 
> > > This string include some invalid characters, hence it will fail
> > > to
> > > properly load the driver which will handle this custom sensor.
> > > Also
> > > it is a problem for some user space tools, which parse
s parse/parses

> > >  the device
> > > names from ftrace and dmesg.
> > > 
> > > This is because the string, real_usage, is not NULL terminated
> > > and
> > > printed with %s to form device name.
> > > 
> > > To address this, we
Remove "we"

> > >  initialize the real_usage string with 0s.
> > > 
> > > Philipp Jungkamp created this fix, I'm simply submitting it. I've
> > > verified it fixes bugzilla issue 217169
> > > 
You don't need the above two lines. You can add

Original-by: Philipp Jungkamp <p.jungkamp@gmx.net>

Before your SOB.

> > > Reported-and-tested-by: Todd Brandt
> > > <todd.e.brandt@linux.intel.com>
> > > Link: https://bugzilla.kernel.org/show_bug.cgi?id=217169
> > > Signed-off-by: Todd Brandt <todd.e.brandt@intel.com>
> > 
> > Why is not Philip in the SoB list?
> > 
> Oh, sorry, I got feedback and assumed it needed more work on the
> changelog. I also forgot to copy over Phillip's SoB line so this one
> is
> probably worse than the original. The original was just fine, please
> ignore this one.
> 
> > Anyway the original patch made it to stable, so:
> > 
> > Fixes: 98c062e82451 ("HID: hid-sensor-custom: Allow more custom iio
> > sensors")
> > Cc: stable@vger.kernel.org
> > 
> > and with those you can add:
> > 
> > Reviewed-by: Andi Shyti <andi.shyti@kernel.org>
> > 
> > Andi
> 


^ permalink raw reply

* Re: [PATCH v5 3/3] arm64: dts: imx93: Add the bbnsm dts node
From: Shawn Guo @ 2023-03-14  1:01 UTC (permalink / raw)
  To: Jacky Bai
  Cc: lee, robh+dt, krzysztof.kozlowski+dt, s.hauer, dmitry.torokhov,
	a.zummo, alexandre.belloni, devicetree, linux-arm-kernel,
	linux-input, linux-rtc, kernel, linux-imx, festevam
In-Reply-To: <20230215024117.3357341-4-ping.bai@nxp.com>

On Wed, Feb 15, 2023 at 10:41:17AM +0800, Jacky Bai wrote:
> Add the bbnsm node for RTC & ON/OFF button support
> 
> Signed-off-by: Jacky Bai <ping.bai@nxp.com>

Applied, thanks!

^ permalink raw reply

* Re: [PATCH v2] Fix buffer overrun in HID-SENSOR name string
From: Todd Brandt @ 2023-03-13 23:37 UTC (permalink / raw)
  To: Andi Shyti, Todd Brandt
  Cc: linux-input, linux-iio, linux-kernel, srinivas.pandruvada, jic23,
	jikos, p.jungkamp
In-Reply-To: <20230313230712.6xboy3w5ocrvj3vn@intel.intel>

On Tue, 2023-03-14 at 00:07 +0100, Andi Shyti wrote:
> Hi Todd,
> 
> On Mon, Mar 13, 2023 at 03:06:53PM -0700, Todd Brandt wrote:
> > On some platforms there are some platform devices created with
> > invalid names. For example: "HID-SENSOR-INT-020b?.39.auto" instead
> > of "HID-SENSOR-INT-020b.39.auto"
> > 
> > This string include some invalid characters, hence it will fail to
> > properly load the driver which will handle this custom sensor. Also
> > it is a problem for some user space tools, which parse the device
> > names from ftrace and dmesg.
> > 
> > This is because the string, real_usage, is not NULL terminated and
> > printed with %s to form device name.
> > 
> > To address this, we initialize the real_usage string with 0s.
> > 
> > Philipp Jungkamp created this fix, I'm simply submitting it. I've
> > verified it fixes bugzilla issue 217169
> > 
> > Reported-and-tested-by: Todd Brandt <todd.e.brandt@linux.intel.com>
> > Link: https://bugzilla.kernel.org/show_bug.cgi?id=217169
> > Signed-off-by: Todd Brandt <todd.e.brandt@intel.com>
> 
> Why is not Philip in the SoB list?
> 
Oh, sorry, I got feedback and assumed it needed more work on the
changelog. I also forgot to copy over Phillip's SoB line so this one is
probably worse than the original. The original was just fine, please
ignore this one.

> Anyway the original patch made it to stable, so:
> 
> Fixes: 98c062e82451 ("HID: hid-sensor-custom: Allow more custom iio
> sensors")
> Cc: stable@vger.kernel.org
> 
> and with those you can add:
> 
> Reviewed-by: Andi Shyti <andi.shyti@kernel.org>
> 
> Andi


^ permalink raw reply

* Re: [PATCH v2] Fix buffer overrun in HID-SENSOR name string
From: Andi Shyti @ 2023-03-13 23:07 UTC (permalink / raw)
  To: Todd Brandt
  Cc: linux-input, linux-iio, linux-kernel, todd.e.brandt,
	srinivas.pandruvada, jic23, jikos, p.jungkamp
In-Reply-To: <20230313220653.3996-1-todd.e.brandt@intel.com>

Hi Todd,

On Mon, Mar 13, 2023 at 03:06:53PM -0700, Todd Brandt wrote:
> On some platforms there are some platform devices created with
> invalid names. For example: "HID-SENSOR-INT-020b?.39.auto" instead
> of "HID-SENSOR-INT-020b.39.auto"
> 
> This string include some invalid characters, hence it will fail to
> properly load the driver which will handle this custom sensor. Also
> it is a problem for some user space tools, which parse the device
> names from ftrace and dmesg.
> 
> This is because the string, real_usage, is not NULL terminated and
> printed with %s to form device name.
> 
> To address this, we initialize the real_usage string with 0s.
> 
> Philipp Jungkamp created this fix, I'm simply submitting it. I've
> verified it fixes bugzilla issue 217169
> 
> Reported-and-tested-by: Todd Brandt <todd.e.brandt@linux.intel.com>
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=217169
> Signed-off-by: Todd Brandt <todd.e.brandt@intel.com>

Why is not Philip in the SoB list?

Anyway the original patch made it to stable, so:

Fixes: 98c062e82451 ("HID: hid-sensor-custom: Allow more custom iio sensors")
Cc: stable@vger.kernel.org

and with those you can add:

Reviewed-by: Andi Shyti <andi.shyti@kernel.org>

Andi

^ permalink raw reply

* [PATCH v2] Fix buffer overrun in HID-SENSOR name string
From: Todd Brandt @ 2023-03-13 22:06 UTC (permalink / raw)
  To: linux-input, linux-iio, linux-kernel
  Cc: todd.e.brandt, todd.e.brandt, srinivas.pandruvada, jic23, jikos,
	p.jungkamp

On some platforms there are some platform devices created with
invalid names. For example: "HID-SENSOR-INT-020b?.39.auto" instead
of "HID-SENSOR-INT-020b.39.auto"

This string include some invalid characters, hence it will fail to
properly load the driver which will handle this custom sensor. Also
it is a problem for some user space tools, which parse the device
names from ftrace and dmesg.

This is because the string, real_usage, is not NULL terminated and
printed with %s to form device name.

To address this, we initialize the real_usage string with 0s.

Philipp Jungkamp created this fix, I'm simply submitting it. I've
verified it fixes bugzilla issue 217169

Reported-and-tested-by: Todd Brandt <todd.e.brandt@linux.intel.com>
Link: https://bugzilla.kernel.org/show_bug.cgi?id=217169
Signed-off-by: Todd Brandt <todd.e.brandt@intel.com>
---
 drivers/hid/hid-sensor-custom.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/hid/hid-sensor-custom.c b/drivers/hid/hid-sensor-custom.c
index 3e3f89e01d81..d85398721659 100644
--- a/drivers/hid/hid-sensor-custom.c
+++ b/drivers/hid/hid-sensor-custom.c
@@ -940,7 +940,7 @@ hid_sensor_register_platform_device(struct platform_device *pdev,
 				    struct hid_sensor_hub_device *hsdev,
 				    const struct hid_sensor_custom_match *match)
 {
-	char real_usage[HID_SENSOR_USAGE_LENGTH];
+	char real_usage[HID_SENSOR_USAGE_LENGTH] = { 0 };
 	struct platform_device *custom_pdev;
 	const char *dev_name;
 	char *c;
-- 
2.17.1


^ permalink raw reply related

* Re: [PATCH] Fix buffer overrun in HID-SENSOR name.
From: srinivas pandruvada @ 2023-03-13 16:56 UTC (permalink / raw)
  To: Todd Brandt, linux-input, linux-iio, linux-kernel
  Cc: todd.e.brandt, jic23, jikos, p.jungkamp
In-Reply-To: <20230310235414.12467-1-todd.e.brandt@intel.com>

On Fri, 2023-03-10 at 15:54 -0800, Todd Brandt wrote:
> Philipp Jungkamp created this fix, I'm simply submitting it. I've
> verified it fixes bugzilla issue 217169.
> 
Not the correct change log. Something like below:

On some platforms there are some platform device is created with
invalid name. For example:
"HID-SENSOR-INT-020b?.39.auto" instead of "HID-SENSOR-INT-020b.39.auto"

This string include some invalid character, hence it will fail to
properly load the driver which will handle this custom sensor. Also
it is a problem for some user space tools, which parses the device
names.

This is because the string real_usage is not NULL terminated and
printed with %s to form device name.

To address this initialize the real_usage string with 0s.

> Reported-and-tested-by: Todd Brandt <todd.e.brandt@linux.intel.com>
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=217169



Fixes: 98c062e82451 ("HID: hid-sensor-custom: Allow more custom iio
sensors")

Suggest-by: Philipp Jungkamp p.jungkamp@gmx.net

> Signed-off-by: Todd Brandt <todd.e.brandt@intel.com>

Thanks,
Srinivas

> ---
>  drivers/hid/hid-sensor-custom.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/hid/hid-sensor-custom.c b/drivers/hid/hid-
> sensor-custom.c
> index 3e3f89e01d81..d85398721659 100644
> --- a/drivers/hid/hid-sensor-custom.c
> +++ b/drivers/hid/hid-sensor-custom.c
> @@ -940,7 +940,7 @@ hid_sensor_register_platform_device(struct
> platform_device *pdev,
>                                     struct hid_sensor_hub_device
> *hsdev,
>                                     const struct
> hid_sensor_custom_match *match)
>  {
> -       char real_usage[HID_SENSOR_USAGE_LENGTH];
> +       char real_usage[HID_SENSOR_USAGE_LENGTH] = { 0 };
>         struct platform_device *custom_pdev;
>         const char *dev_name;
>         char *c;


^ permalink raw reply

* [regression] Bug 217182 - Dell Latitude E7450 Trackpoint not working as expected
From: Linux regression tracking (Thorsten Leemhuis) @ 2023-03-13 10:20 UTC (permalink / raw)
  To: Pali Rohár
  Cc: Steffen, Linux kernel regressions list, Dmitry Torokhov,
	open list:HID CORE LAYER, LKML


Hi, Thorsten here, the Linux kernel's regression tracker.

I noticed a regression report in bugzilla.kernel.org. As many (most?)
kernel developer don't keep an eye on it, I decided to forward it by
mail. Quoting from https://bugzilla.kernel.org/show_bug.cgi?id=217182 :

>  Steffen 2023-03-12 11:17:02 UTC
> 
> Hi,
> I'm new to this bug tracking system and just a user, not a developer.
> 
> I use a Dell Latitude E7450 running with the Debian unstable based
> distribution Siduction.
> After upgrading to Linux kernel 6.2.x the Trackpoint does not work
> anymore as expected. It uses an Alps PS/2 DualPoint Stick.
> Even slight touches in any direction send the cursor to the top right
> corner of the screen where it stays regardless of actual pointer
> movement. This also appears in console mode. One can retrieve the
> pointer again with the touchpad, but I'm more precise with the
> Trackpoint and prefer using that.
> The Trackpoint works well in kernel 6.1.14
> Can somebody help?

See the ticket for more details.


[TLDR for the rest of this mail: I'm adding this report to the list of
tracked Linux kernel regressions; the text you find below is based on a
few templates paragraphs you might have encountered already in similar
form.]

BTW, let me use this mail to also add the report to the list of tracked
regressions to ensure it's doesn't fall through the cracks:

#regzbot introduced: v6.1..v6.2
https://bugzilla.kernel.org/show_bug.cgi?id=217182
#regzbot title: input: alps: Dell Latitude E7450 Trackpoint acting up
#regzbot ignore-activity

This isn't a regression? This issue or a fix for it are already
discussed somewhere else? It was fixed already? You want to clarify when
the regression started to happen? Or point out I got the title or
something else totally wrong? Then just reply and tell me -- ideally
while also telling regzbot about it, as explained by the page listed in
the footer of this mail.

Developers: When fixing the issue, remember to add 'Link:' tags pointing
to the report (e.g. the buzgzilla ticket and maybe this mail as well, if
this thread sees some discussion). See page linked in footer for details.

Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat)
--
Everything you wanna know about Linux kernel regression tracking:
https://linux-regtracking.leemhuis.info/about/#tldr
If I did something stupid, please tell me, as explained on that page.

^ permalink raw reply

* Re: [PATCH 1/5] dt-bindings: input: touchscreen: add bindings for focaltech,fts
From: Joel Selvaraj @ 2023-03-13  6:53 UTC (permalink / raw)
  To: Krzysztof Kozlowski, Caleb Connolly, Dmitry Torokhov, Rob Herring,
	Krzysztof Kozlowski, Andy Gross, Bjorn Andersson, Konrad Dybcio,
	Henrik Rydberg, Arnd Bergmann, Robert Jarzmik, Jeff LaBundy,
	Neil Armstrong, Markuss Broks, Jean Delvare, Max Krummenacher,
	Job Noorman, Alistair Francis, Chris Morgan
  Cc: linux-input, devicetree, linux-kernel, linux-arm-msm,
	~postmarketos/upstreaming, phone-devel
In-Reply-To: <4f864115-ef66-6940-c1c3-6b296de4cab5@linaro.org>

Hi Krzysztof,

On 13/03/23 01:42, Krzysztof Kozlowski wrote:
> Downstream DTS is never a guideline on design of upstream bindings. They
> violate DT binding rules so many times so much, that I don't treat it as
> argument.
> 
> The property does not look board but device specific, so you should
> infer it from compatible.

Understood. Will do so in v2.

Thanks,
Joel

^ permalink raw reply

* Re: [PATCH 1/5] dt-bindings: input: touchscreen: add bindings for focaltech,fts
From: Krzysztof Kozlowski @ 2023-03-13  6:42 UTC (permalink / raw)
  To: Joel Selvaraj, Caleb Connolly, Dmitry Torokhov, Rob Herring,
	Krzysztof Kozlowski, Andy Gross, Bjorn Andersson, Konrad Dybcio,
	Henrik Rydberg, Arnd Bergmann, Robert Jarzmik, Jeff LaBundy,
	Neil Armstrong, Markuss Broks, Jean Delvare, Max Krummenacher,
	Job Noorman, Alistair Francis, Chris Morgan
  Cc: linux-input, devicetree, linux-kernel, linux-arm-msm,
	~postmarketos/upstreaming, phone-devel
In-Reply-To: <02012975-8b4c-58ba-b002-d2983c0e1500@gmail.com>

On 13/03/2023 01:21, Joel Selvaraj wrote:
> Hi Krzysztof,
> 
> Thanks for the review! I agree with most of your comments and will
> fix them in v2. I have a few doubts as discussed below.
> 
> On 12/03/23 15:47, Krzysztof Kozlowski wrote:
>> I have doubts you will cover here all possible FTS controllers, so
>> filename should be more specific, e.g. choose the oldest device compatible.
> 
> The driver is kind of widely used and can actually support 49 touch
> panel variants as per the downstream code [1]. With some slight
> modifications, the other touch panels can be supported too. However, in
> real world, we have only tested the driver against the two panel we have
> access to (FT8719 - Poco F1 Phone and FT5452 - Shiftmq6 Phone).
> 
> Although its very generic and widely used, I agree we don't know that
> will be the case forever. So I am ok with changing it to more specific
> one. But I don't think the panel chip number denote which is older and
> which newer. Shall I just go with focaltech,fts5452, as that's the
> lowest number panel that we have tested so far and is supported?
> 
> Or do I just keep it generic as it can potentially support a lot of
> variants?
> 
>>> +  focaltech,max-touch-number:
>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>> +    description: max number of fingers supported
>>
>> Why this is not implied from compatible? IOW, why this differs between
>> boards?
> 
> Without proper datasheet it is kind of hard to say if this is the
> maximum supported touch points by hardware or just a vendor specified
> one. Because, downstream has it as devicetree property and we only know
> what's set in that from each vendor tree. The FT8719 used in Poco F1
> specifies 10 touch points in downstrean devicetree. But, if I specify it
> as 2, it will still work fine. The FT5452 used in shiftmq6 specifies 5
> touch points in downstream devicetree, but we won't know if that is the
> maximum possible, unless we try to increase it upto 10 and confirm.
> 
> So, yeah without the datasheet, we will be just kind of assuming that is
> is the maximum possible number of touch points by the hardware. I am not
> sure if we wanna hard code that in the driver. Is it okay if we let this
> configurable? Boards/Phones can use the max touch number their vendor
> driver points too or if they have a datasheet, they can specify maximum
> supported one too.

Downstream DTS is never a guideline on design of upstream bindings. They
violate DT binding rules so many times so much, that I don't treat it as
argument.

The property does not look board but device specific, so you should
infer it from compatible.


Best regards,
Krzysztof


^ permalink raw reply

* Re: [PATCH v2] input: touchscreen: goodix: Hold controller in reset during suspend
From: Jarrah @ 2023-03-13  0:44 UTC (permalink / raw)
  To: Jan Jasper de Kroon; +Cc: linux-input, megi
In-Reply-To: <20230311131534.484700-1-jajadekroon@gmail.com>


On 3/12/23 00:15, Jan Jasper de Kroon wrote:
> From: Ondřej Jirman <megi@xff.cz>
>
> The Goodix touchscreen controller used in PinePhone is not properly
> suspended during system sleep, leading to high power consumption. This
> commit modifies the driver to hold the controller in reset during
> system sleep, reducing power consumption by around 40mW.
>
> The original patch also disabled the regulators during system sleep, but
> this could cause issues with other peripherals attached to the same power
> supplies. Hence, this commit only holds the controller in reset during
> system sleep and does not attempt to power it off.
>
> A separate commit adds a new device tree property "hold-in-reset-in-suspend"
> to control the hold-in-reset behavior of the controller during system sleep.
>
> Signed-off-by: Ondrej Jirman <megi@xff.cz>
> Signed-off-by: Jan Jasper de Kroon <jajadekroon@gmail.com>


Tested-by: Jarrah Gosbell <kernel@undef.tools>


Thank you for upstreaming these,

Jarrah.


^ permalink raw reply

* Re: [PATCH 1/5] dt-bindings: input: touchscreen: add bindings for focaltech,fts
From: Joel Selvaraj @ 2023-03-13  0:21 UTC (permalink / raw)
  To: Krzysztof Kozlowski, Caleb Connolly, Dmitry Torokhov, Rob Herring,
	Krzysztof Kozlowski, Andy Gross, Bjorn Andersson, Konrad Dybcio,
	Henrik Rydberg, Arnd Bergmann, Robert Jarzmik, Jeff LaBundy,
	Neil Armstrong, Markuss Broks, Jean Delvare, Max Krummenacher,
	Job Noorman, Alistair Francis, Chris Morgan
  Cc: linux-input, devicetree, linux-kernel, linux-arm-msm,
	~postmarketos/upstreaming, phone-devel
In-Reply-To: <c9db83e8-f87d-b94d-0c23-8114adb312e1@linaro.org>

Hi Krzysztof,

Thanks for the review! I agree with most of your comments and will
fix them in v2. I have a few doubts as discussed below.

On 12/03/23 15:47, Krzysztof Kozlowski wrote:
> I have doubts you will cover here all possible FTS controllers, so
> filename should be more specific, e.g. choose the oldest device compatible.

The driver is kind of widely used and can actually support 49 touch
panel variants as per the downstream code [1]. With some slight
modifications, the other touch panels can be supported too. However, in
real world, we have only tested the driver against the two panel we have
access to (FT8719 - Poco F1 Phone and FT5452 - Shiftmq6 Phone).

Although its very generic and widely used, I agree we don't know that
will be the case forever. So I am ok with changing it to more specific
one. But I don't think the panel chip number denote which is older and
which newer. Shall I just go with focaltech,fts5452, as that's the
lowest number panel that we have tested so far and is supported?

Or do I just keep it generic as it can potentially support a lot of
variants?

>> +  focaltech,max-touch-number:
>> +    $ref: /schemas/types.yaml#/definitions/uint32
>> +    description: max number of fingers supported
> 
> Why this is not implied from compatible? IOW, why this differs between
> boards?

Without proper datasheet it is kind of hard to say if this is the
maximum supported touch points by hardware or just a vendor specified
one. Because, downstream has it as devicetree property and we only know
what's set in that from each vendor tree. The FT8719 used in Poco F1
specifies 10 touch points in downstrean devicetree. But, if I specify it
as 2, it will still work fine. The FT5452 used in shiftmq6 specifies 5
touch points in downstream devicetree, but we won't know if that is the
maximum possible, unless we try to increase it upto 10 and confirm.

So, yeah without the datasheet, we will be just kind of assuming that is
is the maximum possible number of touch points by the hardware. I am not
sure if we wanna hard code that in the driver. Is it okay if we let this
configurable? Boards/Phones can use the max touch number their vendor
driver points too or if they have a datasheet, they can specify maximum
supported one too.

Kindly let me know your thoughts on this.

[1]
https://github.com/LineageOS/android_kernel_xiaomi_sdm845/blob/f1977d9edd01cff3fc9a12e09cd6a5a8052fc8ca/drivers/input/touchscreen/focaltech_touch/focaltech_config.h#L37

> Best regards,
> Krzysztof

Thanks,
Joel

^ permalink raw reply

* Re: [PATCH 2/5] Input: add driver for Focaltech FTS touchscreen
From: Joel Selvaraj @ 2023-03-12 22:21 UTC (permalink / raw)
  To: Markuss Broks, Caleb Connolly, Dmitry Torokhov, Rob Herring,
	Krzysztof Kozlowski, Andy Gross, Bjorn Andersson, Konrad Dybcio,
	Henrik Rydberg, Arnd Bergmann, Robert Jarzmik, Jeff LaBundy,
	Neil Armstrong, Jean Delvare, Max Krummenacher, Job Noorman,
	Alistair Francis, Chris Morgan
  Cc: linux-input, devicetree, linux-kernel, linux-arm-msm,
	~postmarketos/upstreaming, phone-devel
In-Reply-To: <68b05c43-5808-5792-9b57-aeafffe84149@gmail.com>

Hi Markuss,

Thanks for the quick review! I agree with most of your comments and will
fix them in a v2 soon. I have a few doubts as discussed below.

On 12/03/23 15:40, Markuss Broks wrote:

> Why is the _ratelimited variant necessary?

I assumed in case of the interrupt working, but i2c reads fail for some
reason, it would spam a lot of error messages if the user touches the
screen continuously, like a swipe up gesture or something.

I referred to ad7879 touchscreen's irq handling code [1] and thought
it's probably best to do this, to be on the safe side. I will remove
this if it's not needed in v2.

> Overall, I think it's better to cast the data type to a struct, which
> would make this seem with less random.

Sorry, I am not sure I got this right. Do you mean I create an array of
struct called say "fts_point" that stores the x, y, type, etc. info of
all the points, then report it separately. Like similar to something
done by the auo-pixcir touchscreen driver [2]?

If I didn't get this correctly, can you show me some code in mainline,
that does it? It would be very helpful.

[1]
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/input/touchscreen/ad7879.c?h=v6.3-rc1#n250
[2]
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/input/touchscreen/auo-pixcir-ts.c?h=v6.3-rc1#n162

> - Markuss
Thanks,
Joel

^ permalink raw reply

* Re: [PATCH 5/5] arm64: dts: qcom: sdm845-shift-axolotl: update focaltech touchscreen properties
From: Krzysztof Kozlowski @ 2023-03-12 20:49 UTC (permalink / raw)
  To: Joel Selvaraj, Caleb Connolly, Dmitry Torokhov, Rob Herring,
	Krzysztof Kozlowski, Andy Gross, Bjorn Andersson, Konrad Dybcio,
	Henrik Rydberg, Arnd Bergmann, Robert Jarzmik, Jeff LaBundy,
	Neil Armstrong, Markuss Broks, Jean Delvare, Max Krummenacher,
	Job Noorman, Alistair Francis, Chris Morgan
  Cc: linux-input, devicetree, linux-kernel, linux-arm-msm,
	~postmarketos/upstreaming, phone-devel
In-Reply-To: <20230312093249.1846993-6-joelselvaraj.oss@gmail.com>

On 12/03/2023 10:32, Joel Selvaraj wrote:
> The touchscreen nodes were added before the driver patches were merged.
> Update the focaltech touchscreen properties to match with the upstreamed
> focaltech driver. Also, the touchscreen used is in axolotl is fts5452
> and not fts8719.
> 
> Signed-off-by: Joel Selvaraj <joelselvaraj.oss@gmail.com>
> ---
>  .../boot/dts/qcom/sdm845-shift-axolotl.dts      | 17 +++++++++--------
>  1 file changed, 9 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sdm845-shift-axolotl.dts b/arch/arm64/boot/dts/qcom/sdm845-shift-axolotl.dts
> index b54e304abf71..39f59ee3612a 100644
> --- a/arch/arm64/boot/dts/qcom/sdm845-shift-axolotl.dts
> +++ b/arch/arm64/boot/dts/qcom/sdm845-shift-axolotl.dts
> @@ -473,21 +473,22 @@ zap-shader {
>  &i2c5 {
>  	status = "okay";
>  
> -	touchscreen@38 {
> -		compatible = "focaltech,fts8719";
> +	touchscreen: focaltech@38 {

So you had good name and you replace it to something wrong... Drop the
label. This actually applies to all other patches, unless you need it.

Best regards,
Krzysztof


^ permalink raw reply

* Re: [PATCH 4/5] arm64: dts: qcom: sdm845-xiaomi-beryllium-ebbg: introduce support for fts touchscreen
From: Krzysztof Kozlowski @ 2023-03-12 20:48 UTC (permalink / raw)
  To: Joel Selvaraj, Caleb Connolly, Dmitry Torokhov, Rob Herring,
	Krzysztof Kozlowski, Andy Gross, Bjorn Andersson, Konrad Dybcio,
	Henrik Rydberg, Arnd Bergmann, Robert Jarzmik, Jeff LaBundy,
	Neil Armstrong, Markuss Broks, Jean Delvare, Max Krummenacher,
	Job Noorman, Alistair Francis, Chris Morgan
  Cc: linux-input, devicetree, linux-kernel, linux-arm-msm,
	~postmarketos/upstreaming, phone-devel
In-Reply-To: <20230312093249.1846993-5-joelselvaraj.oss@gmail.com>

On 12/03/2023 10:32, Joel Selvaraj wrote:
> The Poco F1 EBBG variant uses Focaltech FTS touchscreen. Introduce
> support for it.
> 
> Signed-off-by: Joel Selvaraj <joelselvaraj.oss@gmail.com>
> ---
>  .../dts/qcom/sdm845-xiaomi-beryllium-ebbg.dts | 27 +++++++++++++++++++
>  1 file changed, 27 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sdm845-xiaomi-beryllium-ebbg.dts b/arch/arm64/boot/dts/qcom/sdm845-xiaomi-beryllium-ebbg.dts
> index 76931ebad065..a23be4c8e1bb 100644
> --- a/arch/arm64/boot/dts/qcom/sdm845-xiaomi-beryllium-ebbg.dts
> +++ b/arch/arm64/boot/dts/qcom/sdm845-xiaomi-beryllium-ebbg.dts
> @@ -13,3 +13,30 @@ &display_panel {
>  	compatible = "ebbg,ft8719";
>  	status = "okay";
>  };
> +
> +&i2c14 {
> +	status = "okay";
> +
> +	dmas =  <&gpi_dma1 0 6 QCOM_GPI_I2C>,
> +		<&gpi_dma1 1 6 QCOM_GPI_I2C>;
> +	dma-names = "tx", "rx";
> +
> +	touchscreen: focaltech@38 {

Node names should be generic.
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation



Best regards,
Krzysztof


^ permalink raw reply

* Re: [PATCH 1/5] dt-bindings: input: touchscreen: add bindings for focaltech,fts
From: Krzysztof Kozlowski @ 2023-03-12 20:47 UTC (permalink / raw)
  To: Joel Selvaraj, Caleb Connolly, Dmitry Torokhov, Rob Herring,
	Krzysztof Kozlowski, Andy Gross, Bjorn Andersson, Konrad Dybcio,
	Henrik Rydberg, Arnd Bergmann, Robert Jarzmik, Jeff LaBundy,
	Neil Armstrong, Markuss Broks, Jean Delvare, Max Krummenacher,
	Job Noorman, Alistair Francis, Chris Morgan
  Cc: linux-input, devicetree, linux-kernel, linux-arm-msm,
	~postmarketos/upstreaming, phone-devel
In-Reply-To: <20230312093249.1846993-2-joelselvaraj.oss@gmail.com>

On 12/03/2023 10:32, Joel Selvaraj wrote:
> Add devicetree bindings for the Focaltech FTS touchscreen drivers.
> 
> Signed-off-by: Joel Selvaraj <joelselvaraj.oss@gmail.com>
> Signed-off-by: Caleb Connolly <caleb@connolly.tech>
> ---
>  .../input/touchscreen/focaltech,fts.yaml      | 81 +++++++++++++++++++
>  1 file changed, 81 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/input/touchscreen/focaltech,fts.yaml
> 
> diff --git a/Documentation/devicetree/bindings/input/touchscreen/focaltech,fts.yaml b/Documentation/devicetree/bindings/input/touchscreen/focaltech,fts.yaml
> new file mode 100644
> index 000000000000..07fe595cc9ed
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/input/touchscreen/focaltech,fts.yaml

I have doubts you will cover here all possible FTS controllers, so
filename should be more specific, e.g. choose the oldest device compatible.

> @@ -0,0 +1,81 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/input/touchscreen/focaltech,fts.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Focaltech FTS I2C Touchscreen Controller
> +
> +maintainers:
> +  - Joel Selvaraj <joelselvaraj.oss@gmail.com>
> +  - Caleb Connolly <caleb@connolly.tech>
> +
> +allOf:
> +  - $ref: touchscreen.yaml#
> +
> +properties:
> +  compatible:
> +    enum:
> +      - focaltech,fts5452
> +      - focaltech,fts8719

Missing blank line

> +  reg:
> +    const: 0x38
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  reset-gpios:
> +    maxItems: 1
> +
> +  avdd-supply:
> +    description: a phandle for the regulator supplying analog power (2.6V to 3.3V).

Drop "a phandle for the"

> +
> +  vddio-supply:
> +    description: a phandle for the regulator supplying IO power (1.8V).

Ditto

> +
> +  focaltech,max-touch-number:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description: max number of fingers supported

Why this is not implied from compatible? IOW, why this differs between
boards?

If this property stays, then anyway "focaltech,max-touch", not number.
There is no such unit suffix as number.

> +    minimum: 2
> +    maximum: 10
> +
> +  touchscreen-size-x: true
> +  touchscreen-size-y: true

Drop these two

> +
> +additionalProperties: false

and then use unevaluatedProperties: false
so all properties from common schema apply. Unless these are not really
valid for the *device*?

> +
> +required:
> +  - compatible
> +  - reg
> +  - reset-gpios
> +  - focaltech,max-touch-number
> +  - touchscreen-size-x
> +  - touchscreen-size-y
> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +    #include <dt-bindings/gpio/gpio.h>
> +    &i2c5 {

i2c

> +      status="okay";

Drop status

Anyway this should pop warnings... Please run `make dt_binding_check`
(see Documentation/devicetree/bindings/writing-schema.rst for instructions).

> +
> +      touchscreen: focaltech@38 {

Node names should be generic.
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation

Also, drop label touchscreen.

> +        compatible = "focaltech,fts8719";
> +        reg = <0x38>;
> +        interrupt-parent = <&tlmm>;
> +        interrupts = <31 IRQ_TYPE_EDGE_RISING>;
> +


Best regards,
Krzysztof


^ permalink raw reply

* Re: [PATCH 2/5] Input: add driver for Focaltech FTS touchscreen
From: Markuss Broks @ 2023-03-12 20:40 UTC (permalink / raw)
  To: Joel Selvaraj, Caleb Connolly, Dmitry Torokhov, Rob Herring,
	Krzysztof Kozlowski, Andy Gross, Bjorn Andersson, Konrad Dybcio,
	Henrik Rydberg, Arnd Bergmann, Robert Jarzmik, Jeff LaBundy,
	Neil Armstrong, Jean Delvare, Max Krummenacher, Job Noorman,
	Alistair Francis, Chris Morgan
  Cc: linux-input, devicetree, linux-kernel, linux-arm-msm,
	~postmarketos/upstreaming, phone-devel
In-Reply-To: <20230312093249.1846993-3-joelselvaraj.oss@gmail.com>

Hi Joel,

Some comments about the driver below,

On 3/12/23 11:32, Joel Selvaraj wrote:
> The Focaltech FTS driver supports several variants of focaltech
> touchscreens found in ~2018 era smartphones including variants found on
> the PocoPhone F1 and the SHIFT6mq which are already present in mainline.
> This driver is loosely based on the original driver from Focaltech
> but has been simplified and largely reworked.
>
> Co-developed-by: Caleb Connolly <caleb@connolly.tech>
> Signed-off-by: Caleb Connolly <caleb@connolly.tech>
> Signed-off-by: Joel Selvaraj <joelselvaraj.oss@gmail.com>
> ---
>   MAINTAINERS                               |   8 +
>   drivers/input/touchscreen/Kconfig         |  12 +
>   drivers/input/touchscreen/Makefile        |   1 +
>   drivers/input/touchscreen/focaltech_fts.c | 448 ++++++++++++++++++++++
>   4 files changed, 469 insertions(+)
>   create mode 100644 drivers/input/touchscreen/focaltech_fts.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 2892858cb040..deb561c356f2 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -7993,6 +7993,14 @@ L:	linux-input@vger.kernel.org
>   S:	Maintained
>   F:	drivers/input/joystick/fsia6b.c
>   
> +FOCALTECH FTS TOUCHSCREEN DRIVER
> +M:	Joel Selvaraj <joelselvaraj.oss@gmail.com>
> +M:	Caleb Connolly <caleb@connolly.tech>
> +L:	linux-input@vger.kernel.org
> +S:	Maintained
> +F:	Documentation/devicetree/bindings/input/touchscreen/focaltech,fts.yaml
> +F:	drivers/input/touchscreen/focaltech_fts.c
> +
>   FOCUSRITE SCARLETT GEN 2/3 MIXER DRIVER
>   M:	Geoffrey D. Bennett <g@b4.vu>
>   L:	alsa-devel@alsa-project.org (moderated for non-subscribers)
> diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
> index 1a2049b336a6..320925bac3a1 100644
> --- a/drivers/input/touchscreen/Kconfig
> +++ b/drivers/input/touchscreen/Kconfig
> @@ -388,6 +388,18 @@ config TOUCHSCREEN_EXC3000
>   	  To compile this driver as a module, choose M here: the
>   	  module will be called exc3000.
>   
> +config TOUCHSCREEN_FOCALTECH_FTS
> +	tristate "Focaltech FTS Touchscreen"
> +	depends on I2C
> +	help
> +	  Say Y here to enable support for I2C connected Focaltech FTS
> +	  based touch panels, including the 5452 and 8917 panels.
> +
> +	  If unsure, say N.
> +
> +	  To compile this driver as a module, choose M here: the
> +	  module will be called focaltech_fts.
> +
>   config TOUCHSCREEN_FUJITSU
>   	tristate "Fujitsu serial touchscreen"
>   	select SERIO
> diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile
> index f2fd28cc34a6..83ea2e3ce754 100644
> --- a/drivers/input/touchscreen/Makefile
> +++ b/drivers/input/touchscreen/Makefile
> @@ -45,6 +45,7 @@ obj-$(CONFIG_TOUCHSCREEN_ELO)		+= elo.o
>   obj-$(CONFIG_TOUCHSCREEN_EGALAX)	+= egalax_ts.o
>   obj-$(CONFIG_TOUCHSCREEN_EGALAX_SERIAL)	+= egalax_ts_serial.o
>   obj-$(CONFIG_TOUCHSCREEN_EXC3000)	+= exc3000.o
> +obj-$(CONFIG_TOUCHSCREEN_FOCALTECH_FTS)	+= focaltech_fts.o
>   obj-$(CONFIG_TOUCHSCREEN_FUJITSU)	+= fujitsu_ts.o
>   obj-$(CONFIG_TOUCHSCREEN_GOODIX)	+= goodix_ts.o
>   obj-$(CONFIG_TOUCHSCREEN_HIDEEP)	+= hideep.o
> diff --git a/drivers/input/touchscreen/focaltech_fts.c b/drivers/input/touchscreen/focaltech_fts.c
> new file mode 100644
> index 000000000000..d389c8b88944
> --- /dev/null
> +++ b/drivers/input/touchscreen/focaltech_fts.c
> @@ -0,0 +1,448 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + *
> + * FocalTech touchscreen driver.
> + *
> + * Copyright (c) 2010-2017, FocalTech Systems, Ltd., all rights reserved.
> + * Copyright (C) 2018 XiaoMi, Inc.
> + * Copyright (c) 2021 Caleb Connolly <caleb@connolly.tech>
> + * Copyright (c) 2023 Joel Selvaraj <joelselvaraj.oss@gmail.com>
> + *
> + * This software is licensed under the terms of the GNU General Public
> + * License version 2, as published by the Free Software Foundation, and
> + * may be copied, distributed, and modified under those terms.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + */
> +
> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +#include <linux/input.h>
> +#include <linux/input/mt.h>
> +#include <linux/input/touchscreen.h>
> +#include <linux/slab.h>
> +#include <linux/interrupt.h>
> +#include <linux/i2c.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/delay.h>
> +#include <linux/regmap.h>
> +#include <linux/mutex.h>
Includes must be sorted alphabetically.
> +
> +#define FTS_REG_CHIP_ID_H 0xA3
> +#define FTS_REG_CHIP_ID_L 0x9F
> +
> +#define FTS_MAX_POINTS_SUPPORT 10
> +#define FTS_ONE_TOUCH_LEN 6
> +
> +#define FTS_TOUCH_X_H_OFFSET 3
> +#define FTS_TOUCH_X_L_OFFSET 4
> +#define FTS_TOUCH_Y_H_OFFSET 5
> +#define FTS_TOUCH_Y_L_OFFSET 6
> +#define FTS_TOUCH_PRESSURE_OFFSET 7
> +#define FTS_TOUCH_AREA_OFFSET 8
> +#define FTS_TOUCH_TYPE_OFFSET 3
> +#define FTS_TOUCH_ID_OFFSET 5
> +
> +#define FTS_TOUCH_DOWN 0
> +#define FTS_TOUCH_UP 1
> +#define FTS_TOUCH_CONTACT 2
> +
> +#define FTS_DRIVER_NAME "fts-i2c"
> +#define INTERVAL_READ_REG 100 /* unit:ms */
> +#define TIMEOUT_READ_REG 2000 /* unit:ms */
Instead of using those comments, it's better to use the _MS suffix.
> +
> +#define CHIP_TYPE_5452 0x5452
> +#define CHIP_TYPE_8719 0x8719
> +
> +struct fts_ts_data {
> +	struct i2c_client *client;
> +	struct input_dev *input_dev;
> +	struct touchscreen_properties prop;
> +
> +	struct regmap *regmap;
> +	int irq;
> +
> +	struct regulator_bulk_data regulators[2];
> +
> +	/* Touch data */
> +	u8 max_touch_number;
> +	u8 *point_buf;
> +	int point_buf_size;
> +
> +	/* DT data */
> +	struct gpio_desc *reset_gpio;
> +};
> +
> +static const struct regmap_config fts_ts_i2c_regmap_config = {
> +	.reg_bits = 8,
> +	.val_bits = 8,
> +};
> +
> +static bool fts_chip_is_valid(struct fts_ts_data *data, u16 id)
> +{
> +	if (id != CHIP_TYPE_5452 && id != CHIP_TYPE_8719)
> +		return false;
> +
> +	return true;
> +}
> +
> +int fts_check_status(struct fts_ts_data *data)
> +{
> +	int count = 0;
> +	unsigned int val, id;
> +
> +	do {
> +		regmap_read(data->regmap, FTS_REG_CHIP_ID_H, &val);
> +		id = val << 8;
> +		regmap_read(data->regmap, FTS_REG_CHIP_ID_L, &val);
> +		id |= val;
That register layout is surely... weird. Anyway, I'd probably do
```
         regmap_read(data->regmap, FTS_REG_CHIP_ID_L, &id);
         regmap_read(data->regmap, FTS_REG_CHIP_ID_H, &val);
         id |= val << 8;
```
to make it less actions. Also, this lacks error checking for the regmap 
calls.
> +
> +		if (fts_chip_is_valid(data, id)) {
> +			dev_dbg(&data->client->dev, "TS Ready: Chip ID = 0x%x", id);
> +			return 0;
> +		}
> +
> +		count++;
> +		msleep(INTERVAL_READ_REG);
> +	} while ((count * INTERVAL_READ_REG) < TIMEOUT_READ_REG);
> +
> +	return -EIO;
> +}
> +
> +static void fts_report_touch(struct fts_ts_data *data)
> +{
> +	struct input_dev *input_dev = data->input_dev;
> +	int base;
> +	unsigned int x, y, z, maj;
> +	u8 slot, type;
> +	int error, i = 0;
> +
> +	u8 *buf = data->point_buf;
> +
> +	memset(buf, 0, data->point_buf_size);
> +
> +	error = regmap_bulk_read(data->regmap, 0, buf, data->point_buf_size);
> +	if (error) {
> +		dev_err_ratelimited(&data->client->dev, "I2C read failed: %d\n",
> +				    error);
Why is the _ratelimited variant necessary?
> +		return;
> +	}
> +
> +	for (i = 0; i < data->max_touch_number; i++) {
> +		base = FTS_ONE_TOUCH_LEN * i;
> +
> +		slot = (buf[base + FTS_TOUCH_ID_OFFSET]) >> 4;
I believe parentheses aren't needed?
> +		if (slot >= data->max_touch_number)
> +			break;
> +
> +		x = ((buf[base + FTS_TOUCH_X_H_OFFSET] & 0x0F) << 8) +
> +			 (buf[base + FTS_TOUCH_X_L_OFFSET] & 0xFF);
> +		y = ((buf[base + FTS_TOUCH_Y_H_OFFSET] & 0x0F) << 8) +
> +			 (buf[base + FTS_TOUCH_Y_L_OFFSET] & 0xFF);
> +
> +		z = buf[base + FTS_TOUCH_PRESSURE_OFFSET];
> +		if (z <= 0)
> +			z = 0x3f;
> +
> +		maj = buf[base + FTS_TOUCH_AREA_OFFSET] >> 4;
> +		if (maj <= 0)
> +			maj = 0x09;
> +
> +		type = buf[base + FTS_TOUCH_TYPE_OFFSET] >> 6;
> +
> +		input_mt_slot(input_dev, slot);
> +		if (type == FTS_TOUCH_DOWN || type == FTS_TOUCH_CONTACT) {
> +			input_mt_report_slot_state(input_dev, MT_TOOL_FINGER, true);
> +			touchscreen_report_pos(data->input_dev, &data->prop, x, y, true);
> +			input_report_abs(input_dev, ABS_MT_PRESSURE, z);
> +			input_report_abs(input_dev, ABS_MT_TOUCH_MAJOR, maj);
> +			input_report_key(data->input_dev, BTN_TOUCH, 1);
> +		} else {
> +			input_report_key(data->input_dev, BTN_TOUCH, 0);
> +			input_mt_report_slot_inactive(input_dev);
> +		}
> +	}
Overall, I think it's better to cast the data type to a struct, which 
would make this seem with less random.
> +	input_sync(input_dev);
> +}
> +
> +static irqreturn_t fts_ts_interrupt(int irq, void *dev_id)
> +{
> +	struct fts_ts_data *data = dev_id;
> +
> +	fts_report_touch(data);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static void fts_power_off(void *d)
> +{
> +	struct fts_ts_data *data = d;
> +
> +	regulator_bulk_disable(ARRAY_SIZE(data->regulators),
> +			       data->regulators);
> +}
> +
> +static int fts_start(struct fts_ts_data *data)
> +{
> +	int error;
> +
> +	error = regulator_bulk_enable(ARRAY_SIZE(data->regulators),
> +				      data->regulators);
> +	if (error) {
> +		dev_err(&data->client->dev, "failed to enable regulators\n");
> +		return error;
> +	}
> +
> +	gpiod_set_value_cansleep(data->reset_gpio, 0);
> +	msleep(200);
> +
> +	enable_irq(data->irq);
> +
> +	return 0;
> +}
> +
> +static int fts_stop(struct fts_ts_data *data)
> +{
> +	disable_irq(data->irq);
> +	gpiod_set_value_cansleep(data->reset_gpio, 1);
> +	fts_power_off(data);
> +
> +	return 0;
> +}
> +
> +static int fts_input_open(struct input_dev *dev)
> +{
> +	struct fts_ts_data *data = input_get_drvdata(dev);
> +	int error;
> +
> +	error = fts_start(data);
> +	if (error)
> +		return error;
> +
> +	error = fts_check_status(data);
> +	if (error) {
> +		dev_err(&data->client->dev, "Failed to start or unsupported chip");
> +		return error;
> +	}
> +
> +	return 0;
> +}
> +
> +static void fts_input_close(struct input_dev *dev)
> +{
> +	struct fts_ts_data *data = input_get_drvdata(dev);
> +
> +	fts_stop(data);
> +}
> +
> +static int fts_input_init(struct fts_ts_data *data)
> +{
> +	struct device *dev = &data->client->dev;
> +	struct input_dev *input_dev;
> +	int error = 0;
> +
> +	input_dev = devm_input_allocate_device(dev);
> +	if (!input_dev)
> +		return -ENOMEM;
> +
> +	data->input_dev = input_dev;
> +
> +	/* Init and register Input device */
> +	input_dev->name = FTS_DRIVER_NAME;
> +	input_dev->id.bustype = BUS_I2C;
> +	input_dev->dev.parent = dev;
> +	input_dev->open = fts_input_open;
> +	input_dev->close = fts_input_close;
> +
> +	input_set_drvdata(input_dev, data);
> +
> +	__set_bit(EV_SYN, input_dev->evbit);
> +	__set_bit(EV_ABS, input_dev->evbit);
> +	__set_bit(INPUT_PROP_DIRECT, input_dev->propbit);
> +
> +	input_mt_init_slots(input_dev, data->max_touch_number,
> +			    INPUT_MT_DIRECT);
> +	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);
> +	input_set_abs_params(input_dev, ABS_MT_PRESSURE, 0, 255, 0, 0);
> +
> +	touchscreen_parse_properties(input_dev, true, &data->prop);
> +	if (!data->prop.max_x || !data->prop.max_y) {
> +		dev_err(dev,
> +			"touchscreen-size-x and/or touchscreen-size-y not set in dts\n");
> +		return -EINVAL;
> +	}
> +
> +	data->point_buf_size = (data->max_touch_number * FTS_ONE_TOUCH_LEN) + 3;
> +	data->point_buf = devm_kzalloc(dev, data->point_buf_size, GFP_KERNEL);
> +	if (!data->point_buf) {
> +		dev_err(dev, "Failed to alloc memory for point buffer\n");
> +		return -ENOMEM;
> +	}
> +
> +	error = input_register_device(input_dev);
> +	if (error) {
> +		dev_err(dev, "Failed to register input device\n");
> +		return error;
> +	}
> +
> +	return 0;
> +}
> +
> +static int fts_parse_dt(struct fts_ts_data *data)
> +{
> +	int error = 0;
> +	struct device *dev = &data->client->dev;
> +	struct device_node *np = dev->of_node;
> +	u32 val;
> +
> +	error = of_property_read_u32(np, "focaltech,max-touch-number", &val);
> +	if (error) {
> +		dev_err(dev, "Unable to read property 'focaltech,max-touch-number'");
> +		return -EINVAL;
> +	}
> +	if (val < 2 || val > FTS_MAX_POINTS_SUPPORT) {
> +		dev_err(dev, "'focaltech,max-touch-number' out of range [2, %d]",
> +			FTS_MAX_POINTS_SUPPORT);
> +		return -EINVAL;
> +	}
> +	data->max_touch_number = val;
> +
> +	data->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
> +	if (IS_ERR(data->reset_gpio)) {
> +		error = PTR_ERR(data->reset_gpio);
> +		dev_err(dev, "Failed to request reset gpio, error %d\n", error);
> +		return error;
> +	}
> +
> +	return 0;
> +}
> +
> +static int fts_ts_probe(struct i2c_client *client)
> +{
> +	int error = 0;
> +	struct fts_ts_data *data;
> +
> +	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
> +		dev_err(&client->dev, "I2C not supported");
> +		return -ENODEV;
> +	}
> +
> +	if (!client->irq) {
> +		dev_err(&client->dev, "No irq specified\n");
> +		return -EINVAL;
> +	}
> +
> +	data = devm_kzalloc(&client->dev, sizeof(*data), GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;
> +
> +	data->client = client;
> +
> +	error = fts_parse_dt(data);
> +	if (error)
> +		return error;
> +
> +	i2c_set_clientdata(client, data);
> +
> +	data->regmap = devm_regmap_init_i2c(client, &fts_ts_i2c_regmap_config);
> +	if (IS_ERR(data->regmap)) {
> +		dev_err(&client->dev, "regmap allocation failed\n");
> +		return PTR_ERR(data->regmap);
> +	}
> +
> +	/*
> +	 * AVDD is the analog voltage supply (2.6V to 3.3V)
> +	 * VDDIO is the digital voltage supply (1.8V)
> +	 */
> +	data->regulators[0].supply = "avdd";
> +	data->regulators[1].supply = "vddio";
> +	error = devm_regulator_bulk_get(&client->dev, ARRAY_SIZE(data->regulators),
> +				      data->regulators);
> +	if (error) {
> +		dev_err(&client->dev, "Failed to get regulators %d\n", error);
> +		return error;
> +	}
> +
> +	error = devm_add_action_or_reset(&client->dev, fts_power_off, data);
> +	if (error) {
> +		dev_err(&client->dev, "failed to install power off handler\n");
> +		return error;
> +	}
> +
> +	error = devm_request_threaded_irq(&client->dev, client->irq, NULL,
> +					fts_ts_interrupt, IRQF_ONESHOT,
> +					client->name, data);
> +	if (error) {
> +		dev_err(&client->dev, "Failed to request IRQ: %d\n", error);
> +		return error;
> +	}
> +
> +	error = fts_input_init(data);
> +	if (error)
> +		return error;
> +
> +	return 0;
> +}
> +
> +static int fts_pm_suspend(struct device *dev)
> +{
> +	struct fts_ts_data *data = dev_get_drvdata(dev);
> +
> +	mutex_lock(&data->input_dev->mutex);
> +
> +	if (input_device_enabled(data->input_dev))
> +		fts_stop(data);
> +
> +	mutex_unlock(&data->input_dev->mutex);
> +
> +	return 0;
> +}
> +
> +static int fts_pm_resume(struct device *dev)
> +{
> +	struct fts_ts_data *data = dev_get_drvdata(dev);
> +	int error = 0;
> +
> +	mutex_lock(&data->input_dev->mutex);
> +
> +	if (input_device_enabled(data->input_dev))
> +		error = fts_start(data);
> +
> +	mutex_unlock(&data->input_dev->mutex);
> +
> +	return error;
> +}
> +
> +static const struct dev_pm_ops fts_dev_pm_ops = {
> +	.suspend = fts_pm_suspend,
> +	.resume = fts_pm_resume,
> +};
> +
> +static const struct of_device_id fts_match_table[] = {
> +	{ .compatible = "focaltech,fts5452", },
> +	{ .compatible = "focaltech,fts8719", },
> +	{ /* sentinel */ },
> +};
> +
> +MODULE_DEVICE_TABLE(of, fts_match_table);
> +
> +static struct i2c_driver fts_ts_driver = {
> +	.probe_new = fts_ts_probe,
> +	.driver = {
> +		.name = FTS_DRIVER_NAME,
> +		.pm = &fts_dev_pm_ops,
> +		.of_match_table = fts_match_table,
> +	},
> +};
> +module_i2c_driver(fts_ts_driver);
> +
> +MODULE_AUTHOR("Joel Selvaraj <joelselvaraj.oss@gmail.com>");
> +MODULE_AUTHOR("Caleb Connolly <caleb@connolly.tech>");
> +MODULE_DESCRIPTION("FocalTech touchscreen Driver");
> +MODULE_LICENSE("GPL");
- Markuss

^ permalink raw reply

* Re: [PATCH v3 2/2] input: touchscreen: goodix: Hold controller in reset during suspend
From: Hans de Goede @ 2023-03-12 18:59 UTC (permalink / raw)
  To: Jan Jasper de Kroon
  Cc: linux-input, megi, hadess, dmitry.torokhov, linux-kernel
In-Reply-To: <20230312184058.552305-1-jajadekroon@gmail.com>

Hi,

On 3/12/23 19:40, Jan Jasper de Kroon wrote:
> From: Ondřej Jirman <megi@xff.cz>
> 
> The Goodix touchscreen controller used in PinePhone is not properly
> suspended during system sleep, leading to high power consumption. This
> commit modifies the driver to hold the controller in reset during
> system sleep, reducing power consumption by around 40mW.
> 
> The original patch also disabled the regulators during system sleep, but
> this could cause issues with other peripherals attached to the same power
> supplies. Hence, this commit only holds the controller in reset during
> system sleep and does not attempt to power it off.
> 
> Separate commit adds a new device tree property "hold-in-reset-in-suspend"
> to control the hold-in-reset behavior of the controller during system
> sleep.
> 
> Signed-off-by: Ondřej Jirman <megi@xff.cz>
> Signed-off-by: Jan Jasper de Kroon <jajadekroon@gmail.com>

Thanks, patch looks good to me:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans



> ---
> V1 -> V2: 
> - Rewrote the commit message and function name to better reflect it's 
>   behavior.
> v2 -> V3: Some patch style cleaning up, to confirm to submission standard.
> 
>  drivers/input/touchscreen/goodix.c | 23 +++++++++++++++++++++++
>  drivers/input/touchscreen/goodix.h |  1 +
>  2 files changed, 24 insertions(+)
> 
> diff --git a/drivers/input/touchscreen/goodix.c b/drivers/input/touchscreen/goodix.c
> index b348172f19c3..a35a203e04bf 100644
> --- a/drivers/input/touchscreen/goodix.c
> +++ b/drivers/input/touchscreen/goodix.c
> @@ -1284,6 +1284,7 @@ static void goodix_disable_regulators(void *arg)
>  
>  static int goodix_ts_probe(struct i2c_client *client)
>  {
> +	struct device_node *np = client->dev.of_node;
>  	struct goodix_ts_data *ts;
>  	const char *cfg_name;
>  	int error;
> @@ -1303,6 +1304,7 @@ static int goodix_ts_probe(struct i2c_client *client)
>  	i2c_set_clientdata(client, ts);
>  	init_completion(&ts->firmware_loading_complete);
>  	ts->contact_size = GOODIX_CONTACT_SIZE;
> +	ts->hold_in_reset_in_suspend = of_property_read_bool(np, "hold-in-reset-in-suspend");
>  
>  	error = goodix_get_gpio_config(ts);
>  	if (error)
> @@ -1410,6 +1412,13 @@ static int goodix_suspend(struct device *dev)
>  	if (ts->load_cfg_from_disk)
>  		wait_for_completion(&ts->firmware_loading_complete);
>  
> +	if (ts->hold_in_reset_in_suspend) {
> +		goodix_free_irq(ts);
> +		goodix_irq_direction_output(ts, 0);
> +		gpiod_direction_output(ts->gpiod_rst, 0);
> +		return 0;
> +	}
> +
>  	/* We need gpio pins to suspend/resume */
>  	if (ts->irq_pin_access_method == IRQ_PIN_ACCESS_NONE) {
>  		disable_irq(client->irq);
> @@ -1455,6 +1464,20 @@ static int goodix_resume(struct device *dev)
>  	u8 config_ver;
>  	int error;
>  
> +	if (ts->hold_in_reset_in_suspend) {
> +		error = goodix_reset(ts);
> +		if (error) {
> +			dev_err(dev, "Controller reset failed.\n");
> +			return error;
> +		}
> +
> +		error = goodix_request_irq(ts);
> +		if (error)
> +			return error;
> +
> +		return 0;
> +	}
> +
>  	if (ts->irq_pin_access_method == IRQ_PIN_ACCESS_NONE) {
>  		enable_irq(client->irq);
>  		return 0;
> diff --git a/drivers/input/touchscreen/goodix.h b/drivers/input/touchscreen/goodix.h
> index 87797cc88b32..c515d03b742c 100644
> --- a/drivers/input/touchscreen/goodix.h
> +++ b/drivers/input/touchscreen/goodix.h
> @@ -104,6 +104,7 @@ struct goodix_ts_data {
>  	u8 main_clk[GOODIX_MAIN_CLK_LEN];
>  	int bak_ref_len;
>  	u8 *bak_ref;
> +	bool hold_in_reset_in_suspend;
>  };
>  
>  int goodix_i2c_read(struct i2c_client *client, u16 reg, u8 *buf, int len);


^ permalink raw reply

* [PATCH v3 2/2] input: touchscreen: goodix: Hold controller in reset during suspend
From: Jan Jasper de Kroon @ 2023-03-12 18:40 UTC (permalink / raw)
  To: jajadekroon
  Cc: linux-input, megi, hadess, hdegoede, dmitry.torokhov,
	linux-kernel
In-Reply-To: <20230311131534.484700-1-jajadekroon@gmail.com>

From: Ondřej Jirman <megi@xff.cz>

The Goodix touchscreen controller used in PinePhone is not properly
suspended during system sleep, leading to high power consumption. This
commit modifies the driver to hold the controller in reset during
system sleep, reducing power consumption by around 40mW.

The original patch also disabled the regulators during system sleep, but
this could cause issues with other peripherals attached to the same power
supplies. Hence, this commit only holds the controller in reset during
system sleep and does not attempt to power it off.

Separate commit adds a new device tree property "hold-in-reset-in-suspend"
to control the hold-in-reset behavior of the controller during system
sleep.

Signed-off-by: Ondřej Jirman <megi@xff.cz>
Signed-off-by: Jan Jasper de Kroon <jajadekroon@gmail.com>
---
V1 -> V2: 
- Rewrote the commit message and function name to better reflect it's 
  behavior.
v2 -> V3: Some patch style cleaning up, to confirm to submission standard.

 drivers/input/touchscreen/goodix.c | 23 +++++++++++++++++++++++
 drivers/input/touchscreen/goodix.h |  1 +
 2 files changed, 24 insertions(+)

diff --git a/drivers/input/touchscreen/goodix.c b/drivers/input/touchscreen/goodix.c
index b348172f19c3..a35a203e04bf 100644
--- a/drivers/input/touchscreen/goodix.c
+++ b/drivers/input/touchscreen/goodix.c
@@ -1284,6 +1284,7 @@ static void goodix_disable_regulators(void *arg)
 
 static int goodix_ts_probe(struct i2c_client *client)
 {
+	struct device_node *np = client->dev.of_node;
 	struct goodix_ts_data *ts;
 	const char *cfg_name;
 	int error;
@@ -1303,6 +1304,7 @@ static int goodix_ts_probe(struct i2c_client *client)
 	i2c_set_clientdata(client, ts);
 	init_completion(&ts->firmware_loading_complete);
 	ts->contact_size = GOODIX_CONTACT_SIZE;
+	ts->hold_in_reset_in_suspend = of_property_read_bool(np, "hold-in-reset-in-suspend");
 
 	error = goodix_get_gpio_config(ts);
 	if (error)
@@ -1410,6 +1412,13 @@ static int goodix_suspend(struct device *dev)
 	if (ts->load_cfg_from_disk)
 		wait_for_completion(&ts->firmware_loading_complete);
 
+	if (ts->hold_in_reset_in_suspend) {
+		goodix_free_irq(ts);
+		goodix_irq_direction_output(ts, 0);
+		gpiod_direction_output(ts->gpiod_rst, 0);
+		return 0;
+	}
+
 	/* We need gpio pins to suspend/resume */
 	if (ts->irq_pin_access_method == IRQ_PIN_ACCESS_NONE) {
 		disable_irq(client->irq);
@@ -1455,6 +1464,20 @@ static int goodix_resume(struct device *dev)
 	u8 config_ver;
 	int error;
 
+	if (ts->hold_in_reset_in_suspend) {
+		error = goodix_reset(ts);
+		if (error) {
+			dev_err(dev, "Controller reset failed.\n");
+			return error;
+		}
+
+		error = goodix_request_irq(ts);
+		if (error)
+			return error;
+
+		return 0;
+	}
+
 	if (ts->irq_pin_access_method == IRQ_PIN_ACCESS_NONE) {
 		enable_irq(client->irq);
 		return 0;
diff --git a/drivers/input/touchscreen/goodix.h b/drivers/input/touchscreen/goodix.h
index 87797cc88b32..c515d03b742c 100644
--- a/drivers/input/touchscreen/goodix.h
+++ b/drivers/input/touchscreen/goodix.h
@@ -104,6 +104,7 @@ struct goodix_ts_data {
 	u8 main_clk[GOODIX_MAIN_CLK_LEN];
 	int bak_ref_len;
 	u8 *bak_ref;
+	bool hold_in_reset_in_suspend;
 };
 
 int goodix_i2c_read(struct i2c_client *client, u16 reg, u8 *buf, int len);
-- 
2.34.3


^ permalink raw reply related

* [PATCH v2 1/2] dt-bindings: input: touchscreen: Add 'hold-in-reset-in-suspend' property to goodix
From: Jan Jasper de Kroon @ 2023-03-12 18:31 UTC (permalink / raw)
  To: jajadekroon
  Cc: devicetree, dmitry.torokhov, robh+dt, krzysztof.kozlowski+dt,
	broonie, alexandre.belloni, kernel, linux-input
In-Reply-To: <20230311134655.486973-1-jajadekroon@gmail.com>

This patch adds a new optional property, 'hold-in-reset-in-suspend', to the
Goodix touchscreen device tree binding. When set to true, the touchscreen
controller will be held in reset mode during system suspend, reducing
power consumption. If not present, the property defaults to false.

I am submitting this patch to the Device Tree mailing list to add a new
property to the Goodix touchscreen device tree binding. This patch
supplements a related patch sent to the linux-input mailing list, which
updates the Goodix touchscreen driver to use this new property.The
linux-input patch can be found at:
https://lore.kernel.org/all/20230311131534.484700-1-jajadekroon@gmail.com/

Signed-off-by: Jan Jasper de Kroon <jajadekroon@gmail.com>
---
V1 -> V2:
- Updated subject prefix to match subsystem
- Added more detailed description of the change
- Fixed formatting issues in commit message
 .../devicetree/bindings/input/touchscreen/goodix.yaml  | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/Documentation/devicetree/bindings/input/touchscreen/goodix.yaml b/Documentation/devicetree/bindings/input/touchscreen/goodix.yaml
index 3d016b87c8df..cd4dd3d25364 100644
--- a/Documentation/devicetree/bindings/input/touchscreen/goodix.yaml
+++ b/Documentation/devicetree/bindings/input/touchscreen/goodix.yaml
@@ -56,6 +56,15 @@ properties:
   touchscreen-size-y: true
   touchscreen-swapped-x-y: true
 
+  hold-in-reset-in-suspend:
+    type: boolean
+    default: false
+    description: |
+      When set to true, the touchscreen controller will be held in reset mode
+      during system suspend. This can help reduce power consumption, but may
+      cause the touchscreen to take longer to resume when the system is woken
+      up from suspend. Defaults to false if not present.
+
 additionalProperties: false
 
 required:
@@ -75,6 +84,7 @@ examples:
         interrupts = <0 0>;
         irq-gpios = <&gpio1 0 0>;
         reset-gpios = <&gpio1 1 0>;
+        hold-in-reset-in-suspend;
       };
     };
 
-- 
2.34.3


^ permalink raw reply related

* [PATCH 4/4] Input: bcm_iproc_tsc: drop of_match_ptr for ID table
From: Krzysztof Kozlowski @ 2023-03-12 13:15 UTC (permalink / raw)
  To: Dmitry Torokhov, Ray Jui, Scott Branden,
	Broadcom internal kernel review list, Chen-Yu Tsai,
	Jernej Skrabec, Samuel Holland, linux-input, linux-kernel,
	linux-arm-kernel, linux-sunxi
  Cc: Krzysztof Kozlowski
In-Reply-To: <20230312131514.351603-1-krzysztof.kozlowski@linaro.org>

The driver can match only via the DT table so the table should be always
used and the of_match_ptr does not have any sense (this also allows ACPI
matching via PRP0001, even though it might not be relevant here).  This
also fixes !CONFIG_OF error:

Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
 drivers/input/touchscreen/bcm_iproc_tsc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/input/touchscreen/bcm_iproc_tsc.c b/drivers/input/touchscreen/bcm_iproc_tsc.c
index 35e2fe9911a4..9c84235327bf 100644
--- a/drivers/input/touchscreen/bcm_iproc_tsc.c
+++ b/drivers/input/touchscreen/bcm_iproc_tsc.c
@@ -511,7 +511,7 @@ static struct platform_driver iproc_ts_driver = {
 	.probe = iproc_ts_probe,
 	.driver = {
 		.name	= IPROC_TS_NAME,
-		.of_match_table = of_match_ptr(iproc_ts_of_match),
+		.of_match_table = iproc_ts_of_match,
 	},
 };
 
-- 
2.34.1


^ permalink raw reply related

* [PATCH 3/4] Input: sun4i-ts: drop of_match_ptr for ID table
From: Krzysztof Kozlowski @ 2023-03-12 13:15 UTC (permalink / raw)
  To: Dmitry Torokhov, Ray Jui, Scott Branden,
	Broadcom internal kernel review list, Chen-Yu Tsai,
	Jernej Skrabec, Samuel Holland, linux-input, linux-kernel,
	linux-arm-kernel, linux-sunxi
  Cc: Krzysztof Kozlowski
In-Reply-To: <20230312131514.351603-1-krzysztof.kozlowski@linaro.org>

The driver can match only via the DT table so the table should be always
used and the of_match_ptr does not have any sense (this also allows ACPI
matching via PRP0001, even though it might not be relevant here).  This
also fixes !CONFIG_OF error:

  drivers/input/touchscreen/sun4i-ts.c:392:34: error: ‘sun4i_ts_of_match’ defined but not used [-Werror=unused-const-variable=]

Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
 drivers/input/touchscreen/sun4i-ts.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/input/touchscreen/sun4i-ts.c b/drivers/input/touchscreen/sun4i-ts.c
index 1117fba30020..577c75c83e25 100644
--- a/drivers/input/touchscreen/sun4i-ts.c
+++ b/drivers/input/touchscreen/sun4i-ts.c
@@ -400,7 +400,7 @@ MODULE_DEVICE_TABLE(of, sun4i_ts_of_match);
 static struct platform_driver sun4i_ts_driver = {
 	.driver = {
 		.name	= "sun4i-ts",
-		.of_match_table = of_match_ptr(sun4i_ts_of_match),
+		.of_match_table = sun4i_ts_of_match,
 	},
 	.probe	= sun4i_ts_probe,
 	.remove	= sun4i_ts_remove,
-- 
2.34.1


^ permalink raw reply related

* [PATCH 2/4] Input: tm2-touchkey: drop of_match_ptr for ID table
From: Krzysztof Kozlowski @ 2023-03-12 13:15 UTC (permalink / raw)
  To: Dmitry Torokhov, Ray Jui, Scott Branden,
	Broadcom internal kernel review list, Chen-Yu Tsai,
	Jernej Skrabec, Samuel Holland, linux-input, linux-kernel,
	linux-arm-kernel, linux-sunxi
  Cc: Krzysztof Kozlowski
In-Reply-To: <20230312131514.351603-1-krzysztof.kozlowski@linaro.org>

The driver will match mostly by DT table (even thought there is regular
ID table) so there is little benefit in of_match_ptr (this also allows
ACPI matching via PRP0001, even though it might not be relevant here).
This also fixes !CONFIG_OF error:

  drivers/input/keyboard/tm2-touchkey.c:335:34: error: ‘tm2_touchkey_of_match’ defined but not used [-Werror=unused-const-variable=]

Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
 drivers/input/keyboard/tm2-touchkey.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/input/keyboard/tm2-touchkey.c b/drivers/input/keyboard/tm2-touchkey.c
index 6627e65f06e5..4e20571cb4c3 100644
--- a/drivers/input/keyboard/tm2-touchkey.c
+++ b/drivers/input/keyboard/tm2-touchkey.c
@@ -354,7 +354,7 @@ static struct i2c_driver tm2_touchkey_driver = {
 	.driver = {
 		.name = TM2_TOUCHKEY_DEV_NAME,
 		.pm = pm_sleep_ptr(&tm2_touchkey_pm_ops),
-		.of_match_table = of_match_ptr(tm2_touchkey_of_match),
+		.of_match_table = tm2_touchkey_of_match,
 	},
 	.probe_new = tm2_touchkey_probe,
 	.id_table = tm2_touchkey_id_table,
-- 
2.34.1


^ permalink raw reply related

* [PATCH 1/4] Input: st-keyscan: drop of_match_ptr for ID table
From: Krzysztof Kozlowski @ 2023-03-12 13:15 UTC (permalink / raw)
  To: Dmitry Torokhov, Ray Jui, Scott Branden,
	Broadcom internal kernel review list, Chen-Yu Tsai,
	Jernej Skrabec, Samuel Holland, linux-input, linux-kernel,
	linux-arm-kernel, linux-sunxi
  Cc: Krzysztof Kozlowski

The driver can match only via the DT table so the table should be always
used and the of_match_ptr does not have any sense (this also allows ACPI
matching via PRP0001, even though it might not be relevant here).  This
also fixes !CONFIG_OF error:

  drivers/input/keyboard/st-keyscan.c:251:34: error: ‘keyscan_of_match’ defined but not used [-Werror=unused-const-variable=]

Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
 drivers/input/keyboard/st-keyscan.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/input/keyboard/st-keyscan.c b/drivers/input/keyboard/st-keyscan.c
index b6e83324f97a..0d27324af809 100644
--- a/drivers/input/keyboard/st-keyscan.c
+++ b/drivers/input/keyboard/st-keyscan.c
@@ -259,7 +259,7 @@ static struct platform_driver keyscan_device_driver = {
 	.driver		= {
 		.name	= "st-keyscan",
 		.pm	= pm_sleep_ptr(&keyscan_dev_pm_ops),
-		.of_match_table = of_match_ptr(keyscan_of_match),
+		.of_match_table = keyscan_of_match,
 	}
 };
 
-- 
2.34.1


^ permalink raw reply related

* [PATCH 5/5] arm64: dts: qcom: sdm845-shift-axolotl: update focaltech touchscreen properties
From: Joel Selvaraj @ 2023-03-12  9:32 UTC (permalink / raw)
  To: Caleb Connolly, Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski,
	Andy Gross, Bjorn Andersson, Konrad Dybcio, Henrik Rydberg,
	Arnd Bergmann, Robert Jarzmik, Jeff LaBundy, Neil Armstrong,
	Markuss Broks, Jean Delvare, Max Krummenacher, Job Noorman,
	Alistair Francis, Chris Morgan
  Cc: linux-input, devicetree, linux-kernel, linux-arm-msm,
	~postmarketos/upstreaming, phone-devel, Joel Selvaraj
In-Reply-To: <20230312093249.1846993-1-joelselvaraj.oss@gmail.com>

The touchscreen nodes were added before the driver patches were merged.
Update the focaltech touchscreen properties to match with the upstreamed
focaltech driver. Also, the touchscreen used is in axolotl is fts5452
and not fts8719.

Signed-off-by: Joel Selvaraj <joelselvaraj.oss@gmail.com>
---
 .../boot/dts/qcom/sdm845-shift-axolotl.dts      | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/arch/arm64/boot/dts/qcom/sdm845-shift-axolotl.dts b/arch/arm64/boot/dts/qcom/sdm845-shift-axolotl.dts
index b54e304abf71..39f59ee3612a 100644
--- a/arch/arm64/boot/dts/qcom/sdm845-shift-axolotl.dts
+++ b/arch/arm64/boot/dts/qcom/sdm845-shift-axolotl.dts
@@ -473,21 +473,22 @@ zap-shader {
 &i2c5 {
 	status = "okay";
 
-	touchscreen@38 {
-		compatible = "focaltech,fts8719";
+	touchscreen: focaltech@38 {
+		compatible = "focaltech,fts5452";
 		reg = <0x38>;
-		wakeup-source;
+
 		interrupt-parent = <&tlmm>;
-		interrupts = <125 0x2>;
-		vdd-supply = <&vreg_l28a_3p0>;
-		vcc-i2c-supply = <&vreg_l14a_1p88>;
+		interrupts = <125 IRQ_TYPE_EDGE_FALLING>;
+
+		avdd-supply = <&vreg_l28a_3p0>;
+		vddio-supply = <&vreg_l14a_1p88>;
 
 		pinctrl-names = "default", "suspend";
 		pinctrl-0 = <&ts_int_active &ts_reset_active>;
 		pinctrl-1 = <&ts_int_suspend &ts_reset_suspend>;
 
-		reset-gpio = <&tlmm 99 GPIO_ACTIVE_HIGH>;
-		irq-gpio = <&tlmm 125 GPIO_TRANSITORY>;
+		reset-gpios = <&tlmm 99 GPIO_ACTIVE_LOW>;
+
 		touchscreen-size-x = <1080>;
 		touchscreen-size-y = <2160>;
 		focaltech,max-touch-number = <5>;
-- 
2.39.2


^ permalink raw reply related

page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox