* Re: [PATCH v3 4/4] input: touchscreen: add SPI support for Goodix Berlin Touchscreen IC
From: Neil Armstrong @ 2023-06-26 7:02 UTC (permalink / raw)
To: Jeff LaBundy
Cc: Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Bastien Nocera, Hans de Goede, Henrik Rydberg, linux-input,
linux-arm-msm, devicetree, linux-kernel
In-Reply-To: <ZJiXopmFr4dPbqll@nixie71>
Hi Jeff,
On 25/06/2023 21:38, Jeff LaBundy wrote:
> Hi Neil,
>
> On Thu, Jun 22, 2023 at 04:29:02PM +0200, Neil Armstrong wrote:
>> Add initial support for the new Goodix "Berlin" touchscreen ICs
>> over the SPI interface.
>>
>> The driver doesn't use the regmap_spi code since the SPI messages
>> needs to be prefixed, thus this custom regmap code.
>>
>> This initial driver is derived from the Goodix goodix_ts_berlin
>> available at [1] and [2] and only supports the GT9916 IC
>> present on the Qualcomm SM8550 MTP & QRD touch panel.
>>
>> The current implementation only supports BerlinD, aka GT9916.
>>
>> [1] https://github.com/goodix/goodix_ts_berlin
>> [2] https://git.codelinaro.org/clo/la/platform/vendor/opensource/touch-drivers
>>
>> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
>> ---
>
> Just a few comments below, then feel free to add:
>
> Reviewed-by: Jeff LaBundy <jeff@labundy.com>
>
>> drivers/input/touchscreen/Kconfig | 13 ++
>> drivers/input/touchscreen/Makefile | 1 +
>> drivers/input/touchscreen/goodix_berlin_spi.c | 172 ++++++++++++++++++++++++++
>> 3 files changed, 186 insertions(+)
>>
>> diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
>> index 5e21cca6025d..2d86615e5090 100644
>> --- a/drivers/input/touchscreen/Kconfig
>> +++ b/drivers/input/touchscreen/Kconfig
>> @@ -435,6 +435,19 @@ config TOUCHSCREEN_GOODIX_BERLIN_I2C
>> To compile this driver as a module, choose M here: the
>> module will be called goodix_berlin_i2c.
>>
>> +config TOUCHSCREEN_GOODIX_BERLIN_SPI
>> + tristate "Goodix Berlin SPI touchscreen"
>> + depends on SPI_MASTER
>
> select REGMAP
>
> (keep "depends on SPI_MASTER")
Ack, indeed it looks cleaner to do that
>
>> + select TOUCHSCREEN_GOODIX_BERLIN_CORE
>> + help
>> + Say Y here if you have a Goodix Berlin IC connected to
>> + your system via SPI.
>> +
>> + If unsure, say N.
>> +
>> + To compile this driver as a module, choose M here: the
>> + module will be called goodix_berlin_spi.
>> +
>> config TOUCHSCREEN_HIDEEP
>> tristate "HiDeep Touch IC"
>> depends on I2C
>> diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile
>> index 921a2da0c2be..29524e8a83db 100644
>> --- a/drivers/input/touchscreen/Makefile
>> +++ b/drivers/input/touchscreen/Makefile
>> @@ -49,6 +49,7 @@ obj-$(CONFIG_TOUCHSCREEN_FUJITSU) += fujitsu_ts.o
>> obj-$(CONFIG_TOUCHSCREEN_GOODIX) += goodix_ts.o
>> obj-$(CONFIG_TOUCHSCREEN_GOODIX_BERLIN_CORE) += goodix_berlin_core.o
>> obj-$(CONFIG_TOUCHSCREEN_GOODIX_BERLIN_I2C) += goodix_berlin_i2c.o
>> +obj-$(CONFIG_TOUCHSCREEN_GOODIX_BERLIN_SPI) += goodix_berlin_spi.o
>> obj-$(CONFIG_TOUCHSCREEN_HIDEEP) += hideep.o
>> obj-$(CONFIG_TOUCHSCREEN_HYNITRON_CSTXXX) += hynitron_cstxxx.o
>> obj-$(CONFIG_TOUCHSCREEN_ILI210X) += ili210x.o
>> diff --git a/drivers/input/touchscreen/goodix_berlin_spi.c b/drivers/input/touchscreen/goodix_berlin_spi.c
>> new file mode 100644
>> index 000000000000..3a1bc251b32d
>> --- /dev/null
>> +++ b/drivers/input/touchscreen/goodix_berlin_spi.c
>> @@ -0,0 +1,172 @@
>> +// SPDX-License-Identifier: GPL-2.0-or-later
>> +/*
>> + * Goodix Berlin Touchscreen Driver
>> + *
>> + * Copyright (C) 2020 - 2021 Goodix, Inc.
>> + * Copyright (C) 2023 Linaro Ltd.
>> + *
>> + * Based on goodix_ts_berlin driver.
>> + */
>> +#include <asm/unaligned.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/regmap.h>
>> +#include <linux/spi/spi.h>
>> +
>> +#include "goodix_berlin.h"
>> +
>> +#define SPI_TRANS_PREFIX_LEN 1
>> +#define REGISTER_WIDTH 4
>> +#define SPI_READ_DUMMY_LEN 3
>> +#define SPI_READ_PREFIX_LEN (SPI_TRANS_PREFIX_LEN + REGISTER_WIDTH + SPI_READ_DUMMY_LEN)
>> +#define SPI_WRITE_PREFIX_LEN (SPI_TRANS_PREFIX_LEN + REGISTER_WIDTH)
>> +
>> +#define SPI_WRITE_FLAG 0xF0
>> +#define SPI_READ_FLAG 0xF1
>
> Please namespace all of these as you have done in the core driver.
Ack
>
>> +
>> +static int goodix_berlin_spi_read(void *context, const void *reg_buf,
>> + size_t reg_size, void *val_buf,
>> + size_t val_size)
>> +{
>> + struct spi_device *spi = context;
>> + struct spi_transfer xfers;
>> + struct spi_message spi_msg;
>> + const u32 *reg = reg_buf; /* reg is stored as native u32 at start of buffer */
>> + u8 *buf;
>> + int ret;
>
> int error;
>
>> +
>> + if (reg_size != REGISTER_WIDTH)
>> + return -EINVAL;
>> +
>> + buf = kzalloc(SPI_READ_PREFIX_LEN + val_size, GFP_KERNEL);
>> + if (!buf)
>> + return -ENOMEM;
>> +
>> + spi_message_init(&spi_msg);
>> + memset(&xfers, 0, sizeof(xfers));
>> +
>> + /* buffer format: 0xF1 + addr(4bytes) + dummy(3bytes) + data */
>> + buf[0] = SPI_READ_FLAG;
>> + put_unaligned_be32(*reg, buf + SPI_TRANS_PREFIX_LEN);
>> + memset(buf + SPI_TRANS_PREFIX_LEN + REGISTER_WIDTH, 0xff,
>> + SPI_READ_DUMMY_LEN);
>> +
>> + xfers.tx_buf = buf;
>> + xfers.rx_buf = buf;
>> + xfers.len = SPI_READ_PREFIX_LEN + val_size;
>> + xfers.cs_change = 0;
>> + spi_message_add_tail(&xfers, &spi_msg);
>> +
>> + ret = spi_sync(spi, &spi_msg);
>
> error = spi_sync(...);
>
>> + if (ret < 0)
>
> if (error)
>
>> + dev_err(&spi->dev, "transfer error:%d", ret);
>> + else
>> + memcpy(val_buf, buf + SPI_READ_PREFIX_LEN, val_size);
>> +
>> + kfree(buf);
>> + return ret;
>> +}
>> +
>> +static int goodix_berlin_spi_write(void *context, const void *data,
>> + size_t count)
>> +{
>> + unsigned int len = count - REGISTER_WIDTH;
>> + struct spi_device *spi = context;
>> + struct spi_transfer xfers;
>> + struct spi_message spi_msg;
>> + const u32 *reg = data; /* reg is stored as native u32 at start of buffer */
>> + u8 *buf;
>> + int ret;
>
> Same comments here regarding 'error' vs. 'ret'.
Seems I forgot to do the rename here, thanks for pointing it!
>
>> +
>> + buf = kzalloc(SPI_WRITE_PREFIX_LEN + len, GFP_KERNEL);
>> + if (!buf)
>> + return -ENOMEM;
>> +
>> + spi_message_init(&spi_msg);
>> + memset(&xfers, 0, sizeof(xfers));
>> +
>> + buf[0] = SPI_WRITE_FLAG;
>> + put_unaligned_be32(*reg, buf + SPI_TRANS_PREFIX_LEN);
>> + memcpy(buf + SPI_WRITE_PREFIX_LEN, data + REGISTER_WIDTH, len);
>> +
>> + xfers.tx_buf = buf;
>> + xfers.len = SPI_WRITE_PREFIX_LEN + len;
>> + xfers.cs_change = 0;
>> + spi_message_add_tail(&xfers, &spi_msg);
>> +
>> + ret = spi_sync(spi, &spi_msg);
>> + if (ret < 0)
>> + dev_err(&spi->dev, "transfer error:%d", ret);
>> +
>> + kfree(buf);
>> + return ret;
>> +}
>> +
>> +static const struct regmap_config goodix_berlin_spi_regmap_conf = {
>> + .reg_bits = 32,
>> + .val_bits = 8,
>> + .read = goodix_berlin_spi_read,
>> + .write = goodix_berlin_spi_write,
>> +};
>> +
>> +/* vendor & product left unassigned here, should probably be updated from fw info */
>> +static const struct input_id goodix_berlin_spi_input_id = {
>> + .bustype = BUS_SPI,
>> +};
>> +
>> +static int goodix_berlin_spi_probe(struct spi_device *spi)
>> +{
>> + struct regmap_config *regmap_config;
>> + struct regmap *regmap;
>> + size_t max_size;
>> + int error = 0;
>> +
>> + regmap_config = devm_kmemdup(&spi->dev, &goodix_berlin_spi_regmap_conf,
>> + sizeof(*regmap_config), GFP_KERNEL);
>> + if (!regmap_config)
>> + return -ENOMEM;
>
> Is there any reason we cannot simply pass goodix_berlin_spi_regmap_conf to
> devm_regmap_init() below? Why to duplicate and pass the copy?
>
> For reference, BMP280 in IIO is a similar example of a device with regmap
> sitting atop a bespoke SPI protocol; it does not seem to take this extra
> step.
The goodix_berlin_spi_regmap_conf copy is modified after with the correct
max raw read/write size, and I'm not a fan of modifying a global structure
that could be use for multiple probes, I can make a copy in a stack variable
if it feels simpler.
>
>> +
>> + spi->mode = SPI_MODE_0;
>> + spi->bits_per_word = 8;
>> + error = spi_setup(spi);
>> + if (error)
>> + return error;
>> +
>> + max_size = spi_max_transfer_size(spi);
>> + regmap_config->max_raw_read = max_size - SPI_READ_PREFIX_LEN;
>> + regmap_config->max_raw_write = max_size - SPI_WRITE_PREFIX_LEN;
>> +
>> + regmap = devm_regmap_init(&spi->dev, NULL, spi, regmap_config);
>> + if (IS_ERR(regmap))
>> + return PTR_ERR(regmap);
>> +
>> + return goodix_berlin_probe(&spi->dev, spi->irq,
>> + &goodix_berlin_spi_input_id, regmap);
>> +}
>> +
>> +static const struct spi_device_id goodix_berlin_spi_ids[] = {
>> + { "gt9916" },
>> + { },
>> +};
>> +MODULE_DEVICE_TABLE(spi, goodix_berlin_spi_ids);
>> +
>> +static const struct of_device_id goodix_berlin_spi_of_match[] = {
>> + { .compatible = "goodix,gt9916", },
>> + { }
>> +};
>> +MODULE_DEVICE_TABLE(of, goodix_berlin_spi_of_match);
>> +
>> +static struct spi_driver goodix_berlin_spi_driver = {
>> + .driver = {
>> + .name = "goodix-berlin-spi",
>> + .of_match_table = goodix_berlin_spi_of_match,
>> + .pm = pm_sleep_ptr(&goodix_berlin_pm_ops),
>> + },
>> + .probe = goodix_berlin_spi_probe,
>> + .id_table = goodix_berlin_spi_ids,
>> +};
>> +module_spi_driver(goodix_berlin_spi_driver);
>> +
>> +MODULE_LICENSE("GPL");
>> +MODULE_DESCRIPTION("Goodix Berlin SPI Touchscreen driver");
>> +MODULE_AUTHOR("Neil Armstrong <neil.armstrong@linaro.org>");
>>
>> --
>> 2.34.1
>>
>
> Kind regards,
> Jeff LaBundy
Thanks,
Neil
^ permalink raw reply
* Re: [PATCH v3 3/4] input: touchscreen: add I2C support for Goodix Berlin Touchscreen IC
From: Neil Armstrong @ 2023-06-26 6:58 UTC (permalink / raw)
To: Jeff LaBundy
Cc: Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Bastien Nocera, Hans de Goede, Henrik Rydberg, linux-input,
linux-arm-msm, devicetree, linux-kernel
In-Reply-To: <ZJiS37RV4ApshVxs@nixie71>
Hi Jeff,
On 25/06/2023 21:17, Jeff LaBundy wrote:
> Hi Neil,
>
> On Thu, Jun 22, 2023 at 04:29:01PM +0200, Neil Armstrong wrote:
>> Add initial support for the new Goodix "Berlin" touchscreen ICs
>> over the I2C interface.
>>
>> This initial driver is derived from the Goodix goodix_ts_berlin
>> available at [1] and [2] and only supports the GT9916 IC
>> present on the Qualcomm SM8550 MTP & QRD touch panel.
>>
>> The current implementation only supports BerlinD, aka GT9916.
>>
>> [1] https://github.com/goodix/goodix_ts_berlin
>> [2] https://git.codelinaro.org/clo/la/platform/vendor/opensource/touch-drivers
>>
>> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
>> ---
>
> Just one comment below, then feel free to add:
>
> Reviewed-by: Jeff LaBundy <jeff@labundy.com>
>
>> drivers/input/touchscreen/Kconfig | 14 ++++++
>> drivers/input/touchscreen/Makefile | 1 +
>> drivers/input/touchscreen/goodix_berlin_i2c.c | 69 +++++++++++++++++++++++++++
>> 3 files changed, 84 insertions(+)
>>
>> diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
>> index 1a6f6f6da991..5e21cca6025d 100644
>> --- a/drivers/input/touchscreen/Kconfig
>> +++ b/drivers/input/touchscreen/Kconfig
>> @@ -421,6 +421,20 @@ config TOUCHSCREEN_GOODIX_BERLIN_CORE
>> depends on REGMAP
>> tristate
>>
>> +config TOUCHSCREEN_GOODIX_BERLIN_I2C
>> + tristate "Goodix Berlin I2C touchscreen"
>> + depends on I2C
>> + depends on REGMAP_I2C
>
> select REGMAP_I2C
>
> (keep "depends on I2C")
Good point,
Thanks,
Neil
>
>> + select TOUCHSCREEN_GOODIX_BERLIN_CORE
>> + help
>> + Say Y here if you have a Goodix Berlin IC connected to
>> + your system via I2C.
>> +
>> + If unsure, say N.
>> +
>> + To compile this driver as a module, choose M here: the
>> + module will be called goodix_berlin_i2c.
>> +
>> config TOUCHSCREEN_HIDEEP
>> tristate "HiDeep Touch IC"
>> depends on I2C
>> diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile
>> index 29cdb042e104..921a2da0c2be 100644
>> --- a/drivers/input/touchscreen/Makefile
>> +++ b/drivers/input/touchscreen/Makefile
>> @@ -48,6 +48,7 @@ obj-$(CONFIG_TOUCHSCREEN_EXC3000) += exc3000.o
>> obj-$(CONFIG_TOUCHSCREEN_FUJITSU) += fujitsu_ts.o
>> obj-$(CONFIG_TOUCHSCREEN_GOODIX) += goodix_ts.o
>> obj-$(CONFIG_TOUCHSCREEN_GOODIX_BERLIN_CORE) += goodix_berlin_core.o
>> +obj-$(CONFIG_TOUCHSCREEN_GOODIX_BERLIN_I2C) += goodix_berlin_i2c.o
>> obj-$(CONFIG_TOUCHSCREEN_HIDEEP) += hideep.o
>> obj-$(CONFIG_TOUCHSCREEN_HYNITRON_CSTXXX) += hynitron_cstxxx.o
>> obj-$(CONFIG_TOUCHSCREEN_ILI210X) += ili210x.o
>> diff --git a/drivers/input/touchscreen/goodix_berlin_i2c.c b/drivers/input/touchscreen/goodix_berlin_i2c.c
>> new file mode 100644
>> index 000000000000..6407b2258eb1
>> --- /dev/null
>> +++ b/drivers/input/touchscreen/goodix_berlin_i2c.c
>> @@ -0,0 +1,69 @@
>> +// SPDX-License-Identifier: GPL-2.0-or-later
>> +/*
>> + * Goodix Berlin Touchscreen Driver
>> + *
>> + * Copyright (C) 2020 - 2021 Goodix, Inc.
>> + * Copyright (C) 2023 Linaro Ltd.
>> + *
>> + * Based on goodix_ts_berlin driver.
>> + */
>> +#include <linux/i2c.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/regmap.h>
>> +
>> +#include "goodix_berlin.h"
>> +
>> +#define I2C_MAX_TRANSFER_SIZE 256
>> +
>> +static const struct regmap_config goodix_berlin_i2c_regmap_conf = {
>> + .reg_bits = 32,
>> + .val_bits = 8,
>> + .max_raw_read = I2C_MAX_TRANSFER_SIZE,
>> + .max_raw_write = I2C_MAX_TRANSFER_SIZE,
>> +};
>> +
>> +/* vendor & product left unassigned here, should probably be updated from fw info */
>> +static const struct input_id goodix_berlin_i2c_input_id = {
>> + .bustype = BUS_I2C,
>> +};
>> +
>> +static int goodix_berlin_i2c_probe(struct i2c_client *client)
>> +{
>> + struct regmap *regmap;
>> +
>> + regmap = devm_regmap_init_i2c(client, &goodix_berlin_i2c_regmap_conf);
>> + if (IS_ERR(regmap))
>> + return PTR_ERR(regmap);
>> +
>> + return goodix_berlin_probe(&client->dev, client->irq,
>> + &goodix_berlin_i2c_input_id, regmap);
>> +}
>> +
>> +static const struct i2c_device_id goodix_berlin_i2c_id[] = {
>> + { "gt9916", 0 },
>> + { }
>> +};
>> +
>> +MODULE_DEVICE_TABLE(i2c, goodix_berlin_i2c_id);
>> +
>> +static const struct of_device_id goodix_berlin_i2c_of_match[] = {
>> + { .compatible = "goodix,gt9916", },
>> + { }
>> +};
>> +MODULE_DEVICE_TABLE(of, goodix_berlin_i2c_of_match);
>> +
>> +static struct i2c_driver goodix_berlin_i2c_driver = {
>> + .driver = {
>> + .name = "goodix-berlin-i2c",
>> + .of_match_table = goodix_berlin_i2c_of_match,
>> + .pm = pm_sleep_ptr(&goodix_berlin_pm_ops),
>> + },
>> + .probe = goodix_berlin_i2c_probe,
>> + .id_table = goodix_berlin_i2c_id,
>> +};
>> +module_i2c_driver(goodix_berlin_i2c_driver);
>> +
>> +MODULE_LICENSE("GPL");
>> +MODULE_DESCRIPTION("Goodix Berlin I2C Touchscreen driver");
>> +MODULE_AUTHOR("Neil Armstrong <neil.armstrong@linaro.org>");
>>
>> --
>> 2.34.1
>>
>
> Kind regards,
> Jeff LaBundy
^ permalink raw reply
* Re: [PATCH v3 2/4] input: touchscreen: add core support for Goodix Berlin Touchscreen IC
From: Neil Armstrong @ 2023-06-26 6:57 UTC (permalink / raw)
To: Jeff LaBundy
Cc: Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Bastien Nocera, Hans de Goede, Henrik Rydberg, linux-input,
linux-arm-msm, devicetree, linux-kernel
In-Reply-To: <ZJiSIyt1sg8NnPI9@nixie71>
On 25/06/2023 21:14, Jeff LaBundy wrote:
> Hi Neil,
>
> On Thu, Jun 22, 2023 at 04:29:00PM +0200, Neil Armstrong wrote:
>> Add initial support for the new Goodix "Berlin" touchscreen ICs.
>>
>> These touchscreen ICs support SPI, I2C and I3C interface, up to
>> 10 finger touch, stylus and gestures events.
>>
>> This initial driver is derived from the Goodix goodix_ts_berlin
>> available at [1] and [2] and only supports the GT9916 IC
>> present on the Qualcomm SM8550 MTP & QRD touch panel.
>>
>> The current implementation only supports BerlinD, aka GT9916.
>>
>> Support for advanced features like:
>> - Firmware & config update
>> - Stylus events
>> - Gestures events
>> - Previous revisions support (BerlinA or BerlinB)
>> is not included in current version.
>>
>> The current support will work with currently flashed firmware
>> and config, and bail out if firmware or config aren't flashed yet.
>>
>> [1] https://github.com/goodix/goodix_ts_berlin
>> [2] https://git.codelinaro.org/clo/la/platform/vendor/opensource/touch-drivers
>>
>> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
>> ---
>
> Great work; thank you for the productive discussion. I only had some
> minor cosmetic comments this last time; once those are addressed, feel
> free to add:
>
> Reviewed-by: Jeff LaBundy <jeff@labundy.com>
Thanks a lot for you reviews!
>
>> drivers/input/touchscreen/Kconfig | 5 +
>> drivers/input/touchscreen/Makefile | 1 +
>> drivers/input/touchscreen/goodix_berlin.h | 159 +++++++
>> drivers/input/touchscreen/goodix_berlin_core.c | 584 +++++++++++++++++++++++++
>> 4 files changed, 749 insertions(+)
>>
>> diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
>> index c2cbd332af1d..1a6f6f6da991 100644
>> --- a/drivers/input/touchscreen/Kconfig
>> +++ b/drivers/input/touchscreen/Kconfig
>> @@ -416,6 +416,11 @@ config TOUCHSCREEN_GOODIX
>> To compile this driver as a module, choose M here: the
>> module will be called goodix.
>>
>> +config TOUCHSCREEN_GOODIX_BERLIN_CORE
>> + depends on GPIOLIB || COMPILE_TEST
>
> My comment seems to have been missed; this driver does not actually depend
> on GPIOLIB and the dependency here can be dropped. However, this would leave
> the driver to be built only if COMPILE_TEST is set which is obviously not
> what we want. Therefore, this line can simply be dropped entirely.
Indeed, I forgot to change this thx for the reminder.
>
>> + depends on REGMAP
>
> Rather than depending on REGMAP here, you should simply select the appropriate
> transport from the I2C or SPI driver (which seems to have been missed in those
> patches as well; I will follow up there). In the meantime, this line can just
> be dropped.
>
> MFD has several examples of dual I2C/SPI devices that demonstrate the correct
> pattern; see any of those for reference.
Ack, I'll update this and the i2c/spi entries aswell as you pointed.
>
>> + tristate
>> +
>> config TOUCHSCREEN_HIDEEP
>> tristate "HiDeep Touch IC"
>> depends on I2C
>> diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile
>> index 159cd5136fdb..29cdb042e104 100644
>> --- a/drivers/input/touchscreen/Makefile
>> +++ b/drivers/input/touchscreen/Makefile
>> @@ -47,6 +47,7 @@ obj-$(CONFIG_TOUCHSCREEN_EGALAX_SERIAL) += egalax_ts_serial.o
>> obj-$(CONFIG_TOUCHSCREEN_EXC3000) += exc3000.o
>> obj-$(CONFIG_TOUCHSCREEN_FUJITSU) += fujitsu_ts.o
>> obj-$(CONFIG_TOUCHSCREEN_GOODIX) += goodix_ts.o
>> +obj-$(CONFIG_TOUCHSCREEN_GOODIX_BERLIN_CORE) += goodix_berlin_core.o
>> obj-$(CONFIG_TOUCHSCREEN_HIDEEP) += hideep.o
>> obj-$(CONFIG_TOUCHSCREEN_HYNITRON_CSTXXX) += hynitron_cstxxx.o
>> obj-$(CONFIG_TOUCHSCREEN_ILI210X) += ili210x.o
>> diff --git a/drivers/input/touchscreen/goodix_berlin.h b/drivers/input/touchscreen/goodix_berlin.h
>> new file mode 100644
>> index 000000000000..235f44947a28
>> --- /dev/null
>> +++ b/drivers/input/touchscreen/goodix_berlin.h
>> @@ -0,0 +1,159 @@
>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>> +/*
>> + * Goodix Touchscreen Driver
>> + * Copyright (C) 2020 - 2021 Goodix, Inc.
>> + * Copyright (C) 2023 Linaro Ltd.
>> + *
>> + * Based on goodix_berlin_berlin driver.
>> + */
>> +
>> +#ifndef __GOODIX_BERLIN_H_
>> +#define __GOODIX_BERLIN_H_
>> +
>> +#include <linux/gpio/consumer.h>
>> +#include <linux/input.h>
>> +#include <linux/input/touchscreen.h>
>> +#include <linux/regulator/consumer.h>
>> +#include <linux/sizes.h>
>> +
>> +#define GOODIX_BERLIN_MAX_TOUCH 10
>> +
>> +#define GOODIX_BERLIN_NORMAL_RESET_DELAY_MS 100
>> +
>> +#define GOODIX_BERLIN_IRQ_EVENT_HEAD_LEN 8
>> +#define GOODIX_BERLIN_STATUS_OFFSET 0
>> +#define GOODIX_BERLIN_REQUEST_TYPE_OFFSET 2
>> +
>> +#define GOODIX_BERLIN_BYTES_PER_POINT 8
>> +#define GOODIX_BERLIN_COOR_DATA_CHECKSUM_SIZE 2
>> +#define GOODIX_BERLIN_COOR_DATA_CHECKSUM_MASK GENMASK(15, 0)
>> +
>> +/* Read n finger events */
>> +#define GOODIX_BERLIN_IRQ_READ_LEN(n) (GOODIX_BERLIN_IRQ_EVENT_HEAD_LEN + \
>> + (GOODIX_BERLIN_BYTES_PER_POINT * (n)) + \
>> + GOODIX_BERLIN_COOR_DATA_CHECKSUM_SIZE)
>> +
>> +#define GOODIX_BERLIN_TOUCH_EVENT BIT(7)
>> +#define GOODIX_BERLIN_REQUEST_EVENT BIT(6)
>> +#define GOODIX_BERLIN_TOUCH_COUNT_MASK GENMASK(3, 0)
>> +
>> +#define GOODIX_BERLIN_REQUEST_CODE_RESET 3
>> +
>> +#define GOODIX_BERLIN_POINT_TYPE_MASK GENMASK(3, 0)
>> +#define GOODIX_BERLIN_POINT_TYPE_STYLUS_HOVER 1
>> +#define GOODIX_BERLIN_POINT_TYPE_STYLUS 3
>> +
>> +#define GOODIX_BERLIN_TOUCH_ID_MASK GENMASK(7, 4)
>> +
>> +#define GOODIX_BERLIN_DEV_CONFIRM_VAL 0xAA
>> +#define GOODIX_BERLIN_BOOTOPTION_ADDR 0x10000
>> +#define GOODIX_BERLIN_FW_VERSION_INFO_ADDR 0x10014
>> +
>> +#define GOODIX_BERLIN_IC_INFO_MAX_LEN SZ_1K
>> +#define GOODIX_BERLIN_IC_INFO_ADDR 0x10070
>> +
>> +struct goodix_berlin_fw_version {
>> + u8 rom_pid[6];
>> + u8 rom_vid[3];
>> + u8 rom_vid_reserved;
>> + u8 patch_pid[8];
>> + u8 patch_vid[4];
>> + u8 patch_vid_reserved;
>> + u8 sensor_id;
>> + u8 reserved[2];
>> + __le16 checksum;
>> +} __packed;
>> +
>> +struct goodix_berlin_ic_info_version {
>> + u8 info_customer_id;
>> + u8 info_version_id;
>> + u8 ic_die_id;
>> + u8 ic_version_id;
>> + __le32 config_id;
>> + u8 config_version;
>> + u8 frame_data_customer_id;
>> + u8 frame_data_version_id;
>> + u8 touch_data_customer_id;
>> + u8 touch_data_version_id;
>> + u8 reserved[3];
>> +} __packed;
>> +
>> +struct goodix_berlin_ic_info_feature {
>> + __le16 freqhop_feature;
>> + __le16 calibration_feature;
>> + __le16 gesture_feature;
>> + __le16 side_touch_feature;
>> + __le16 stylus_feature;
>> +} __packed;
>> +
>> +struct goodix_berlin_ic_info_misc {
>> + __le32 cmd_addr;
>> + __le16 cmd_max_len;
>> + __le32 cmd_reply_addr;
>> + __le16 cmd_reply_len;
>> + __le32 fw_state_addr;
>> + __le16 fw_state_len;
>> + __le32 fw_buffer_addr;
>> + __le16 fw_buffer_max_len;
>> + __le32 frame_data_addr;
>> + __le16 frame_data_head_len;
>> + __le16 fw_attr_len;
>> + __le16 fw_log_len;
>> + u8 pack_max_num;
>> + u8 pack_compress_version;
>> + __le16 stylus_struct_len;
>> + __le16 mutual_struct_len;
>> + __le16 self_struct_len;
>> + __le16 noise_struct_len;
>> + __le32 touch_data_addr;
>> + __le16 touch_data_head_len;
>> + __le16 point_struct_len;
>> + __le16 reserved1;
>> + __le16 reserved2;
>> + __le32 mutual_rawdata_addr;
>> + __le32 mutual_diffdata_addr;
>> + __le32 mutual_refdata_addr;
>> + __le32 self_rawdata_addr;
>> + __le32 self_diffdata_addr;
>> + __le32 self_refdata_addr;
>> + __le32 iq_rawdata_addr;
>> + __le32 iq_refdata_addr;
>> + __le32 im_rawdata_addr;
>> + __le16 im_readata_len;
>> + __le32 noise_rawdata_addr;
>> + __le16 noise_rawdata_len;
>> + __le32 stylus_rawdata_addr;
>> + __le16 stylus_rawdata_len;
>> + __le32 noise_data_addr;
>> + __le32 esd_addr;
>> +} __packed;
>> +
>> +struct goodix_berlin_touch_data {
>> + u8 id;
>> + u8 unused;
>> + __le16 x;
>> + __le16 y;
>> + __le16 w;
>> +} __packed;
>> +
>> +struct goodix_berlin_core {
>> + struct device *dev;
>> + struct regmap *regmap;
>> + struct regulator *avdd;
>> + struct regulator *iovdd;
>> + struct gpio_desc *reset_gpio;
>> + struct touchscreen_properties props;
>> + struct goodix_berlin_fw_version fw_version;
>> + struct input_dev *input_dev;
>> + int irq;
>> +
>> + /* Runtime parameters extracted from IC_INFO buffer */
>> + u32 touch_data_addr;
>> +};
>> +
>> +int goodix_berlin_probe(struct device *dev, int irq, const struct input_id *id,
>> + struct regmap *regmap);
>> +
>> +extern const struct dev_pm_ops goodix_berlin_pm_ops;
>> +
>> +#endif
>> diff --git a/drivers/input/touchscreen/goodix_berlin_core.c b/drivers/input/touchscreen/goodix_berlin_core.c
>> new file mode 100644
>> index 000000000000..af3e73bbb3ec
>> --- /dev/null
>> +++ b/drivers/input/touchscreen/goodix_berlin_core.c
>> @@ -0,0 +1,584 @@
>> +// SPDX-License-Identifier: GPL-2.0-or-later
>> +/*
>> + * Goodix Touchscreen Driver
>> + * Copyright (C) 2020 - 2021 Goodix, Inc.
>> + * Copyright (C) 2023 Linaro Ltd.
>> + *
>> + * Based on goodix_ts_berlin driver.
>> + */
>> +#include <asm/unaligned.h>
>> +#include <linux/input/mt.h>
>> +#include <linux/input/touchscreen.h>
>> +#include <linux/regmap.h>
>> +
>> +#include "goodix_berlin.h"
>> +
>> +/*
>> + * Goodix "Berlin" Touchscreen ID driver
>> + *
>> + * This driver is distinct from goodix.c since hardware interface
>> + * is different enough to require a new driver.
>> + * None of the register address or data structure are close enough
>> + * to the previous generations.
>> + *
>> + * Currently only handles Multitouch events with already
>> + * programmed firmware and "config" for "Revision D" Berlin IC.
>> + *
>> + * Support is missing for:
>> + * - ESD Management
>> + * - Firmware update/flashing
>> + * - "Config" update/flashing
>> + * - Stylus Events
>> + * - Gesture Events
>> + * - Support for older revisions (A & B)
>> + */
>> +
>> +static bool goodix_berlin_checksum_valid(const u8 *data, int size)
>> +{
>> + u32 cal_checksum = 0;
>> + u16 r_checksum;
>> + u32 i;
>> +
>> + if (size < GOODIX_BERLIN_COOR_DATA_CHECKSUM_SIZE)
>> + return false;
>> +
>> + for (i = 0; i < size - GOODIX_BERLIN_COOR_DATA_CHECKSUM_SIZE; i++)
>> + cal_checksum += data[i];
>> +
>> + r_checksum = get_unaligned_le16(&data[i]);
>> +
>> + return FIELD_GET(GOODIX_BERLIN_COOR_DATA_CHECKSUM_MASK, cal_checksum) == r_checksum;
>> +}
>> +
>> +static bool goodix_berlin_is_dummy_data(struct goodix_berlin_core *cd,
>> + const u8 *data, int size)
>> +{
>> + int i;
>> +
>> + /*
>> + * If the device is missing or doesn't respond the buffer
>> + * could be filled with bus default line state, 0x00 or 0xff,
>> + * so declare success the first time we encounter neither.
>> + */
>> + for (i = 0; i < size; i++)
>> + if (data[i] > 0 && data[i] < 0xff)
>> + return false;
>> +
>> + return true;
>> +}
>> +
>> +static int goodix_berlin_dev_confirm(struct goodix_berlin_core *cd)
>> +{
>> + u8 tx_buf[8], rx_buf[8];
>> + int retry = 3;
>> + int error;
>> +
>> + memset(tx_buf, GOODIX_BERLIN_DEV_CONFIRM_VAL, sizeof(tx_buf));
>> + while (retry--) {
>> + error = regmap_raw_write(cd->regmap, GOODIX_BERLIN_BOOTOPTION_ADDR, tx_buf,
>> + sizeof(tx_buf));
>> + if (error)
>> + return error;
>> +
>> + error = regmap_raw_read(cd->regmap, GOODIX_BERLIN_BOOTOPTION_ADDR, rx_buf,
>> + sizeof(rx_buf));
>> + if (error)
>> + return error;
>> +
>> + if (!memcmp(tx_buf, rx_buf, sizeof(tx_buf)))
>> + return 0;
>> +
>> + usleep_range(5000, 5100);
>> + }
>> +
>> + dev_err(cd->dev, "device confirm failed, rx_buf: %*ph\n", 8, rx_buf);
>> +
>> + return -EINVAL;
>> +}
>> +
>> +static int goodix_berlin_power_on(struct goodix_berlin_core *cd, bool on)
>> +{
>> + int error = 0;
>> +
>> + if (on) {
>> + error = regulator_enable(cd->iovdd);
>> + if (error) {
>> + dev_err(cd->dev, "Failed to enable iovdd: %d\n", error);
>> + return error;
>> + }
>> +
>> + /* Vendor waits 3ms for IOVDD to settle */
>> + usleep_range(3000, 3100);
>> +
>> + error = regulator_enable(cd->avdd);
>> + if (error) {
>> + dev_err(cd->dev, "Failed to enable avdd: %d\n", error);
>> + goto error_avdd_regulator;
>> + }
>> +
>> + /* Vendor waits 15ms for IOVDD to settle */
>> + usleep_range(15000, 15100);
>> +
>> + gpiod_set_value(cd->reset_gpio, 0);
>> +
>> + /* Vendor waits 4ms for Firmware to initialize */
>> + usleep_range(4000, 4100);
>> +
>> + error = goodix_berlin_dev_confirm(cd);
>> + if (error)
>> + goto error_dev_confirm;
>> +
>> + /* Vendor waits 100ms for Firmware to fully boot */
>> + msleep(GOODIX_BERLIN_NORMAL_RESET_DELAY_MS);
>> +
>> + return 0;
>> + }
>> +
>> +error_dev_confirm:
>> + gpiod_set_value(cd->reset_gpio, 1);
>
> These labels are still confusing in my opinion. Rather than name the label
> based on the code that could have gotten us here, it is better for the name
> to reflect the action that follows.
>
> How about err_dev_reset and err_iovdd_disable?
Ack, I wasn't sure in which way those should be names
>> +
>> + regulator_disable(cd->avdd);
>> +
>> +error_avdd_regulator:
>> + regulator_disable(cd->iovdd);
>> +
>> + return error;
>> +}
>> +
>> +static int goodix_berlin_read_version(struct goodix_berlin_core *cd)
>> +{
>> + u8 buf[sizeof(struct goodix_berlin_fw_version)];
>> + int error;
>> +
>> + error = regmap_raw_read(cd->regmap, GOODIX_BERLIN_FW_VERSION_INFO_ADDR, buf, sizeof(buf));
>> + if (error) {
>> + dev_err(cd->dev, "error reading fw version\n");
>
> It's handy to print the return value here as you do elsewhere.
Ack
>
>> + return error;
>> + }
>> +
>> + if (!goodix_berlin_checksum_valid(buf, sizeof(buf))) {
>> + dev_err(cd->dev, "invalid fw version: checksum error\n");
>> + return -EINVAL;
>> + }
>> +
>> + memcpy(&cd->fw_version, buf, sizeof(cd->fw_version));
>> +
>> + return 0;
>> +}
>> +
>> +/* Only extract necessary data for runtime */
>> +static int goodix_berlin_convert_ic_info(struct goodix_berlin_core *cd,
>> + const u8 *data, u16 length)
>> +{
>> + struct goodix_berlin_ic_info_misc misc;
>> + unsigned int offset = 0;
>> + u8 param_num;
>> +
>> + offset += sizeof(__le16); /* length */
>> + offset += sizeof(struct goodix_berlin_ic_info_version);
>> + offset += sizeof(struct goodix_berlin_ic_info_feature);
>> +
>> + /* IC_INFO Parameters, variable width structure */
>> + offset += 4 * sizeof(u8); /* drv_num, sen_num, button_num, force_num */
>> +
>> + if (offset >= length)
>> + goto invalid_offset;
>> +
>> + param_num = data[offset++]; /* active_scan_rate_num */
>> +
>> + offset += param_num * sizeof(__le16);
>> +
>> + if (offset >= length)
>> + goto invalid_offset;
>> +
>> + param_num = data[offset++]; /* mutual_freq_num*/
>> +
>> + offset += param_num * sizeof(__le16);
>> +
>> + if (offset >= length)
>> + goto invalid_offset;
>> +
>> + param_num = data[offset++]; /* self_tx_freq_num */
>> +
>> + offset += param_num * sizeof(__le16);
>> +
>> + if (offset >= length)
>> + goto invalid_offset;
>> +
>> + param_num = data[offset++]; /* self_rx_freq_num */
>> +
>> + offset += param_num * sizeof(__le16);
>> +
>> + if (offset >= length)
>> + goto invalid_offset;
>> +
>> + param_num = data[offset++]; /* stylus_freq_num */
>> +
>> + offset += param_num * sizeof(__le16);
>> +
>> + if (offset + sizeof(misc) > length)
>> + goto invalid_offset;
>> +
>> + /* goodix_berlin_ic_info_misc */
>> + memcpy(&misc, &data[offset], sizeof(misc));
>> +
>> + cd->touch_data_addr = le32_to_cpu(misc.touch_data_addr);
>> +
>> + return 0;
>> +
>> +invalid_offset:
>> + dev_err(cd->dev, "ic_info length is invalid (offset %d length %d)\n",
>> + offset, length);
>> + return -EINVAL;
>> +}
>> +
>> +static int goodix_berlin_get_ic_info(struct goodix_berlin_core *cd)
>> +{
>> + u8 afe_data[GOODIX_BERLIN_IC_INFO_MAX_LEN];
>> + __le16 length_raw;
>> + u16 length;
>> + int error;
>> +
>> + error = regmap_raw_read(cd->regmap, GOODIX_BERLIN_IC_INFO_ADDR,
>> + &length_raw, sizeof(length_raw));
>> + if (error) {
>> + dev_info(cd->dev, "failed get ic info length, %d\n", error);
>> + return error;
>> + }
>> +
>> + length = le16_to_cpu(length_raw);
>> + if (length >= GOODIX_BERLIN_IC_INFO_MAX_LEN) {
>> + dev_info(cd->dev, "invalid ic info length %d\n", length);
>> + return -EINVAL;
>> + }
>> +
>> + error = regmap_raw_read(cd->regmap, GOODIX_BERLIN_IC_INFO_ADDR,
>> + afe_data, length);
>> + if (error) {
>> + dev_info(cd->dev, "failed get ic info data, %d\n", error);
>> + return error;
>> + }
>> +
>> + /* check whether the data is valid (ex. bus default values) */
>> + if (goodix_berlin_is_dummy_data(cd, (const uint8_t *)afe_data, length)) {
>> + dev_err(cd->dev, "fw info data invalid\n");
>> + return -EINVAL;
>> + }
>> +
>> + if (!goodix_berlin_checksum_valid((const uint8_t *)afe_data, length)) {
>> + dev_info(cd->dev, "fw info checksum error\n");
>> + return -EINVAL;
>> + }
>> +
>> + error = goodix_berlin_convert_ic_info(cd, afe_data, length);
>> + if (error) {
>> + dev_err(cd->dev, "error converting ic info\n");
>
> This function already prints an error message upon failure; consider dropping
> one of the two.
Ack
>
>> + return error;
>> + }
>> +
>> + /* check some key info */
>> + if (!cd->touch_data_addr) {
>> + dev_err(cd->dev, "touch_data_addr is null\n");
>> + return -EINVAL;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static void goodix_berlin_parse_finger(struct goodix_berlin_core *cd,
>> + const void *buf, int touch_num)
>> +{
>> + const struct goodix_berlin_touch_data *touch_data = buf;
>> + int i;
>> +
>> + /* Check for data validity */
>> + for (i = 0; i < touch_num; i++) {
>> + unsigned int id;
>> +
>> + id = FIELD_GET(GOODIX_BERLIN_TOUCH_ID_MASK, touch_data[i].id);
>> +
>> + if (id >= GOODIX_BERLIN_MAX_TOUCH) {
>> + dev_warn(cd->dev, "invalid finger id %d\n", id);
>> + return;
>> + }
>> + }
>> +
>> + /* Report finger touches */
>> + for (i = 0; i < touch_num; i++) {
>> + input_mt_slot(cd->input_dev,
>> + FIELD_GET(GOODIX_BERLIN_TOUCH_ID_MASK,
>> + touch_data[i].id));
>> + input_mt_report_slot_state(cd->input_dev, MT_TOOL_FINGER, true);
>> +
>> + touchscreen_report_pos(cd->input_dev, &cd->props,
>> + __le16_to_cpu(touch_data[i].x),
>> + __le16_to_cpu(touch_data[i].y),
>> + true);
>> + input_report_abs(cd->input_dev, ABS_MT_TOUCH_MAJOR,
>> + __le16_to_cpu(touch_data[i].w));
>> + }
>> +
>> + input_mt_sync_frame(cd->input_dev);
>> + input_sync(cd->input_dev);
>> +}
>> +
>> +static void goodix_berlin_touch_handler(struct goodix_berlin_core *cd,
>> + const void *pre_buf, u32 pre_buf_len)
>> +{
>> + static u8 buffer[GOODIX_BERLIN_IRQ_READ_LEN(GOODIX_BERLIN_MAX_TOUCH)];
>> + u8 point_type = 0;
>> + u8 touch_num = 0;
>> + int error = 0;
>
> Nit: some more unnecessary initializations in here. All of this cleaned up
> quite nicely however.
Ack, forgot those in the refactor
>
>> +
>> + /* copy pre-data to buffer */
>> + memcpy(buffer, pre_buf, pre_buf_len);
>> +
>> + touch_num = FIELD_GET(GOODIX_BERLIN_TOUCH_COUNT_MASK,
>> + buffer[GOODIX_BERLIN_REQUEST_TYPE_OFFSET]);
>> +
>> + if (touch_num > GOODIX_BERLIN_MAX_TOUCH) {
>> + dev_warn(cd->dev, "invalid touch num %d\n", touch_num);
>> + return;
>> + }
>> +
>> + if (touch_num) {
>> + /* read more data if more than 2 touch events */
>> + if (unlikely(touch_num > 2)) {
>> + error = regmap_raw_read(cd->regmap,
>> + cd->touch_data_addr + pre_buf_len,
>> + &buffer[pre_buf_len],
>> + (touch_num - 2) * GOODIX_BERLIN_BYTES_PER_POINT);
>> + if (error) {
>> + dev_err_ratelimited(cd->dev, "failed get touch data\n");
>
> failed to get touch data: %d
>
> (insert the word "to" and print the return value)
Ack
>
>> + return;
>> + }
>> + }
>> +
>> + point_type = FIELD_GET(GOODIX_BERLIN_POINT_TYPE_MASK,
>> + buffer[GOODIX_BERLIN_IRQ_EVENT_HEAD_LEN]);
>> +
>> + if (point_type == GOODIX_BERLIN_POINT_TYPE_STYLUS ||
>> + point_type == GOODIX_BERLIN_POINT_TYPE_STYLUS_HOVER) {
>> + dev_warn_once(cd->dev, "Stylus event type not handled\n");
>> + return;
>> + }
>> +
>> + if (!goodix_berlin_checksum_valid(&buffer[GOODIX_BERLIN_IRQ_EVENT_HEAD_LEN],
>> + touch_num * GOODIX_BERLIN_BYTES_PER_POINT + 2)) {
>> + dev_dbg(cd->dev, "touch data checksum error\n");
>> + dev_dbg(cd->dev, "data: %*ph\n",
>> + touch_num * GOODIX_BERLIN_BYTES_PER_POINT + 2,
>> + &buffer[GOODIX_BERLIN_IRQ_EVENT_HEAD_LEN]);
>
> This case seems worth of a dev_err; the two messages can be combined as well.
Yep
>
>> + return;
>> + }
>> + }
>> +
>> + goodix_berlin_parse_finger(cd, &buffer[GOODIX_BERLIN_IRQ_EVENT_HEAD_LEN],
>> + touch_num);
>> +}
>> +
>> +static int goodix_berlin_request_handle_reset(struct goodix_berlin_core *cd)
>> +{
>> + gpiod_set_value(cd->reset_gpio, 1);
>> + usleep_range(2000, 2100);
>> + gpiod_set_value(cd->reset_gpio, 0);
>> +
>> + msleep(GOODIX_BERLIN_NORMAL_RESET_DELAY_MS);
>> +
>> + return 0;
>> +}
>> +
>> +static irqreturn_t goodix_berlin_threadirq_func(int irq, void *data)
>> +{
>> + struct goodix_berlin_core *cd = data;
>> + u8 buf[GOODIX_BERLIN_IRQ_READ_LEN(2)];
>> + u8 event_status;
>> + int error;
>> +
>> + /* First, read buffer with space for 2 touch events */
>> + error = regmap_raw_read(cd->regmap, cd->touch_data_addr, buf,
>> + GOODIX_BERLIN_IRQ_READ_LEN(2));
>> + if (error) {
>> + dev_err_ratelimited(cd->dev, "failed get event head data\n");
>
> failed to get event head data: %d
>
> (same comment as earlier)
>
Ack
>> + return IRQ_HANDLED;
>> + }
>> +
>> + if (buf[GOODIX_BERLIN_STATUS_OFFSET] == 0)
>> + return IRQ_HANDLED;
>> +
>> + if (!goodix_berlin_checksum_valid(buf, GOODIX_BERLIN_IRQ_EVENT_HEAD_LEN)) {
>> + dev_warn_ratelimited(cd->dev, "touch head checksum err : %*ph\n",
>> + GOODIX_BERLIN_IRQ_EVENT_HEAD_LEN, buf);
>> + return IRQ_HANDLED;
>> + }
>> +
>> + event_status = buf[GOODIX_BERLIN_STATUS_OFFSET];
>> +
>> + if (event_status & GOODIX_BERLIN_TOUCH_EVENT)
>> + goodix_berlin_touch_handler(cd, buf, GOODIX_BERLIN_IRQ_READ_LEN(2));
>> +
>> + if (event_status & GOODIX_BERLIN_REQUEST_EVENT) {
>> + switch (buf[GOODIX_BERLIN_REQUEST_TYPE_OFFSET]) {
>> + case GOODIX_BERLIN_REQUEST_CODE_RESET:
>> + goodix_berlin_request_handle_reset(cd);
>> + break;
>> +
>> + default:
>> + dev_warn(cd->dev, "unsupported request code 0x%x\n",
>> + buf[GOODIX_BERLIN_REQUEST_TYPE_OFFSET]);
>> + }
>> + }
>> +
>> + /* Clear up status field */
>> + regmap_write(cd->regmap, cd->touch_data_addr, 0);
>> +
>> + return IRQ_HANDLED;
>> +}
>> +
>> +static int goodix_berlin_input_dev_config(struct goodix_berlin_core *cd,
>> + const struct input_id *id)
>> +{
>> + struct input_dev *input_dev;
>> + int error;
>> +
>> + input_dev = devm_input_allocate_device(cd->dev);
>> + if (!input_dev)
>> + return -ENOMEM;
>> +
>> + cd->input_dev = input_dev;
>> + input_set_drvdata(input_dev, cd);
>> +
>> + input_dev->name = "Goodix Berlin Capacitive TouchScreen";
>> + input_dev->phys = "input/ts";
>> +
>> + input_dev->id = *id;
>> +
>> + input_set_abs_params(cd->input_dev, ABS_MT_POSITION_X, 0, SZ_64K - 1, 0, 0);
>> + input_set_abs_params(cd->input_dev, ABS_MT_POSITION_Y, 0, SZ_64K - 1, 0, 0);
>> + input_set_abs_params(cd->input_dev, ABS_MT_TOUCH_MAJOR, 0, 255, 0, 0);
>> +
>> + touchscreen_parse_properties(cd->input_dev, true, &cd->props);
>> +
>> + error = input_mt_init_slots(cd->input_dev, GOODIX_BERLIN_MAX_TOUCH,
>> + INPUT_MT_DIRECT | INPUT_MT_DROP_UNUSED);
>> + if (error)
>> + return error;
>> +
>> + return input_register_device(cd->input_dev);
>> +}
>> +
>> +static int goodix_berlin_pm_suspend(struct device *dev)
>> +{
>> + struct goodix_berlin_core *cd = dev_get_drvdata(dev);
>> +
>> + disable_irq(cd->irq);
>> +
>> + return goodix_berlin_power_on(cd, false);
>> +}
>> +
>> +static int goodix_berlin_pm_resume(struct device *dev)
>> +{
>> + struct goodix_berlin_core *cd = dev_get_drvdata(dev);
>> + int error;
>> +
>> + error = goodix_berlin_power_on(cd, true);
>> + if (error)
>> + return error;
>> +
>> + enable_irq(cd->irq);
>> +
>> + return 0;
>> +}
>> +
>> +EXPORT_GPL_SIMPLE_DEV_PM_OPS(goodix_berlin_pm_ops,
>> + goodix_berlin_pm_suspend,
>> + goodix_berlin_pm_resume);
>> +
>> +static void goodix_berlin_power_off(void *data)
>> +{
>> + struct goodix_berlin_core *cd = data;
>> +
>> + goodix_berlin_power_on(cd, false);
>> +}
>> +
>> +int goodix_berlin_probe(struct device *dev, int irq, const struct input_id *id,
>> + struct regmap *regmap)
>> +{
>> + struct goodix_berlin_core *cd;
>> + int error;
>> +
>> + if (irq <= 0) {
>> + dev_err(dev, "Missing interrupt number\n");
>> + return -EINVAL;
>> + }
>> +
>> + cd = devm_kzalloc(dev, sizeof(*cd), GFP_KERNEL);
>> + if (!cd)
>> + return -ENOMEM;
>> +
>> + cd->dev = dev;
>> + cd->regmap = regmap;
>> + cd->irq = irq;
>> +
>> + cd->reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
>> + if (IS_ERR(cd->reset_gpio))
>> + return dev_err_probe(dev, PTR_ERR(cd->reset_gpio),
>> + "Failed to request reset gpio\n");
>> +
>> + cd->avdd = devm_regulator_get(dev, "avdd");
>> + if (IS_ERR(cd->avdd))
>> + return dev_err_probe(dev, PTR_ERR(cd->avdd),
>> + "Failed to request avdd regulator\n");
>> +
>> + cd->iovdd = devm_regulator_get(dev, "iovdd");
>> + if (IS_ERR(cd->iovdd))
>> + return dev_err_probe(dev, PTR_ERR(cd->iovdd),
>> + "Failed to request iovdd regulator\n");
>> +
>> + error = goodix_berlin_power_on(cd, true);
>> + if (error) {
>> + dev_err(dev, "failed power on");
>> + return error;
>> + }
>> +
>> + error = devm_add_action_or_reset(dev, goodix_berlin_power_off, cd);
>> + if (error)
>> + return error;
>> +
>> + error = goodix_berlin_read_version(cd);
>> + if (error) {
>> + dev_err(dev, "failed to get version info");
>> + return error;
>> + }
>> +
>> + error = goodix_berlin_get_ic_info(cd);
>> + if (error) {
>> + dev_err(dev, "invalid ic info, abort");
>> + return error;
>> + }
>> +
>> + error = goodix_berlin_input_dev_config(cd, id);
>> + if (error) {
>> + dev_err(dev, "failed set input device");
>> + return error;
>> + }
>> +
>> + error = devm_request_threaded_irq(dev, irq, NULL,
>> + goodix_berlin_threadirq_func,
>> + IRQF_ONESHOT, "goodix-berlin", cd);
>> + if (error) {
>> + dev_err(dev, "request threaded irq failed: %d\n", error);
>> + return error;
>> + }
>> +
>> + dev_set_drvdata(dev, cd);
>> +
>> + dev_dbg(dev, "Goodix Berlin %s Touchscreen Controller", cd->fw_version.patch_pid);
>> +
>> + return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(goodix_berlin_probe);
>> +
>> +MODULE_LICENSE("GPL");
>> +MODULE_DESCRIPTION("Goodix Berlin Core Touchscreen driver");
>> +MODULE_AUTHOR("Neil Armstrong <neil.armstrong@linaro.org>");
>>
>> --
>> 2.34.1
>>
>
> Kind regards,
> Jeff LaBundy
Thanks,
Neil
^ permalink raw reply
* Re: [PATCH 00/24 v2] Documentation: correct lots of spelling errors (series 1)
From: Krzysztof Wilczyński @ 2023-06-26 6:56 UTC (permalink / raw)
To: Randy Dunlap
Cc: linux-kernel, Jonathan Corbet, Russell King, Jens Axboe,
Vladimir Oltean, Steffen Klassert, Daniel Jordan, Akinobu Mita,
Helge Deller, Dmitry Torokhov, Henrik Rydberg, Karsten Keil,
Jiri Kosina, Miroslav Benes, Petr Mladek, Josh Poimboeuf,
Peter Zijlstra, Ingo Molnar, Will Deacon, Jérôme Glisse,
Naoya Horiguchi, Miaohe Lin, Jonas Bonn, Stefan Kristiansson,
Stafford Horne, Bjorn Helgaas, Lorenzo Pieralisi, Marc Zyngier,
Michael Ellerman, Heiko Carstens, Vasily Gorbik,
Alexander Gordeev, Juri Lelli, Vincent Guittot, David Howells,
Jarkko Sakkinen, Paul Moore, James Morris, Serge E. Hallyn,
Daniel Bristot de Oliveira, Steven Rostedt, Masami Hiramatsu,
Mathieu Poirier, Suzuki K Poulose, Evgeniy Polyakov, Fenghua Yu,
Reinette Chatre, Thomas Gleixner, Borislav Petkov, Chris Zankel,
Max Filippov, coresight, dri-devel, keyrings, linux-block,
linux-crypto, linux-doc, linux-fbdev, linux-input, linux-pci,
linux-s390, linux-scsi, linux-sgx, linux-trace-devel,
linux-trace-kernel, live-patching, linux-security-module,
linux-usb, netdev, linux-mm, openrisc, linux-arm-kernel,
linux-xtensa, linuxppc-dev, x86
In-Reply-To: <20230209071400.31476-1-rdunlap@infradead.org>
Hello,
> Correct many spelling errors in Documentation/ as reported by codespell.
>
> Maintainers of specific kernel subsystems are only Cc-ed on their
> respective patches, not the entire series.
>
> These patches are based on linux-next-20230209.
>
[...]
> [PATCH 13/24] Documentation: PCI: correct spelling
[...]
Applied to misc, thank you!
[1/1] Documentation: PCI: correct spelling
https://git.kernel.org/pci/pci/c/b58d6d89ae02
Krzysztof
^ permalink raw reply
* Re: [PATCH v3 1/4] Input: ts-overlay - Add touchscreen overlay object handling
From: Jeff LaBundy @ 2023-06-26 2:35 UTC (permalink / raw)
To: Javier Carrasco
Cc: Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Henrik Rydberg, Bastian Hecht, Michael Riesch, linux-kernel,
linux-input, devicetree
In-Reply-To: <20230510-feature-ts_virtobj_patch-v3-1-b4fb7fc4bab7@wolfvision.net>
Hi Javier,
On Fri, Jun 16, 2023 at 09:28:51AM +0200, Javier Carrasco wrote:
> Some touchscreens provide mechanical overlays with different objects
> like buttons or clipped touchscreen surfaces.
>
> In order to support these objects, add a series of helper functions
> to the input subsystem to transform them into overlay objects via
> device tree nodes.
>
> These overlay objects consume the raw touch events and report the
> expected input events depending on the object properties.
>
> Signed-off-by: Javier Carrasco <javier.carrasco@wolfvision.net>
> ---
Excellent work; it's great to see this series move along.
> MAINTAINERS | 7 +
> drivers/input/touchscreen/Kconfig | 9 +
> drivers/input/touchscreen/Makefile | 1 +
> drivers/input/touchscreen/ts-overlay.c | 356 +++++++++++++++++++++++++++++++++
> include/linux/input/ts-overlay.h | 43 ++++
> 5 files changed, 416 insertions(+)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 7e0b87d5aa2e..db9427926a4c 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -21434,6 +21434,13 @@ W: https://github.com/srcres258/linux-doc
> T: git git://github.com/srcres258/linux-doc.git doc-zh-tw
> F: Documentation/translations/zh_TW/
>
> +TOUCHSCREEN OVERLAY OBJECTS
> +M: Javier Carrasco <javier.carrasco@wolfvision.net>
> +L: linux-input@vger.kernel.org
> +S: Maintained
> +F: drivers/input/touchscreen/ts-overlay.c
> +F: include/linux/input/ts-overlay.h
> +
> TTY LAYER
> M: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> M: Jiri Slaby <jirislaby@kernel.org>
> diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
> index 143ff43c67ae..8382a4d68326 100644
> --- a/drivers/input/touchscreen/Kconfig
> +++ b/drivers/input/touchscreen/Kconfig
> @@ -1388,4 +1388,13 @@ config TOUCHSCREEN_HIMAX_HX83112B
> To compile this driver as a module, choose M here: the
> module will be called himax_hx83112b.
>
> +config TOUCHSCREEN_TS_OVERLAY
> + bool "Touchscreen Overlay Objects"
> + help
> + Say Y here if you are using a touchscreen driver that supports
> + printed overlays with keys or a clipped touchscreen area.
> +
> + Should be selected by the touchscren drivers that support
> + this feature.
> +
> endif
> diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile
> index 159cd5136fdb..f554826706ff 100644
> --- a/drivers/input/touchscreen/Makefile
> +++ b/drivers/input/touchscreen/Makefile
> @@ -117,3 +117,4 @@ obj-$(CONFIG_TOUCHSCREEN_RASPBERRYPI_FW) += raspberrypi-ts.o
> obj-$(CONFIG_TOUCHSCREEN_IQS5XX) += iqs5xx.o
> obj-$(CONFIG_TOUCHSCREEN_ZINITIX) += zinitix.o
> obj-$(CONFIG_TOUCHSCREEN_HIMAX_HX83112B) += himax_hx83112b.o
> +obj-$(CONFIG_TOUCHSCREEN_TS_OVERLAY) += ts-overlay.o
It seems like this feature is useful for any two-dimensional touch surface
(e.g. trackpads) and not just touchscreens. For that reason, the touchscreen
helpers in touchscreen.c were moved out of input/touchscreen and into input/
such that they are not guarded by CONFIG_INPUT_TOUCHSCREEN. A growing number
of devices in input/misc are taking advantage of these.
Therefore, I think this feature should follow suit and be available to any
input device as is the case for touchscreen.c. As written, the newly updated
binding is misleading because one may believe that any device that includes
touchscreen.yaml can define an overlay child, but the code does not currently
support this.
To that end, it seems like touch-overlay would be a more descriptive name as
well. I understand that the name has changed once already, so hopefully this
feedback is not too annoying :)
> diff --git a/drivers/input/touchscreen/ts-overlay.c b/drivers/input/touchscreen/ts-overlay.c
> new file mode 100644
> index 000000000000..7afa77d86c1f
> --- /dev/null
> +++ b/drivers/input/touchscreen/ts-overlay.c
> @@ -0,0 +1,356 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Helper functions for overlay objects on touchscreens
> + *
> + * Copyright (c) 2023 Javier Carrasco <javier.carrasco@wolfvision.net>
> + */
> +
> +#include <linux/property.h>
> +#include <linux/input.h>
> +#include <linux/input/mt.h>
> +#include <linux/module.h>
> +#include <linux/input/ts-overlay.h>
> +
> +enum ts_overlay_valid_objects {
> + TOUCHSCREEN,
> + BUTTON,
Please namespace these, i.e. TOUCH_OVERLAY_*.
> +};
> +
> +static const char *const ts_overlay_names[] = {
> + [TOUCHSCREEN] = "overlay-touchscreen",
I'm a little confused why we need new code for this particular function; it's
what touchscreen-min-x/y and touchscreen-size-x/y were meant to define. Why
can't we keep using those?
> + [BUTTON] = "overlay-buttons",
> +};
> +
> +struct ts_overlay_shape {
> + u32 x_origin;
> + u32 y_origin;
> + u32 x_size;
> + u32 y_size;
> +};
> +
> +struct ts_overlay_button {
> + struct ts_overlay_shape shape;
> + u32 key;
> + bool pressed;
> + int slot;
> +};
> +
> +static int ts_overlay_get_shape_properties(struct fwnode_handle *child_node,
> + struct ts_overlay_shape *shape)
> +{
> + int rc;
In input, the common practice is to use 'error' for return values that are either
zero or negative. The reasoning is because the variable quite literally represents
an error, or lack thereof. And then:
error = ...
if (error)
return error;
> +
> + rc = fwnode_property_read_u32(child_node, "x-origin", &shape->x_origin);
> + if (rc < 0)
> + return rc;
It seems like all of these properties are required; if so, please update the
binding to make this clear.
If the binding is correct and these properties are in fact optional, then you
must evaluate fwnode_property_read_u32() against -EINVAL.
> +
> + rc = fwnode_property_read_u32(child_node, "y-origin", &shape->y_origin);
> + if (rc < 0)
> + return rc;
> +
> + rc = fwnode_property_read_u32(child_node, "x-size", &shape->x_size);
> + if (rc < 0)
> + return rc;
> +
> + rc = fwnode_property_read_u32(child_node, "y-size", &shape->y_size);
> + if (rc < 0)
> + return rc;
> +
> + return 0;
> +}
> +
> +static int ts_overlay_get_button_properties(struct device *dev,
> + struct fwnode_handle *child_node,
> + struct ts_overlay_button *btn)
> +{
> + struct fwnode_handle *child_btn;
> + int rc;
> + int j = 0;
> +
> + fwnode_for_each_child_node(child_node, child_btn) {
> + rc = ts_overlay_get_shape_properties(child_btn, &btn[j].shape);
> + if (rc < 0)
> + goto button_prop_cleanup;
> +
> + rc = fwnode_property_read_u32(child_btn, "linux,code",
> + &btn[j].key);
> + if (rc < 0)
> + goto button_prop_cleanup;
The binding needs to list this property as required, too.
> +
> + dev_info(dev, "Added button at (%u, %u), size %ux%u, code=%u\n",
> + btn[j].shape.x_origin, btn[j].shape.y_origin,
> + btn[j].shape.x_size, btn[j].shape.y_size, btn[j].key);
This seems like a dev_dbg() to me.
> + j++;
> + }
> +
> + return 0;
> +
> +button_prop_cleanup:
> + fwnode_handle_put(child_btn);
> + return rc;
> +}
> +
> +void ts_overlay_set_button_caps(struct ts_overlay_map *map,
> + struct input_dev *dev)
> +{
> + int i;
> +
> + for (i = 0; i < map->button_count; i++)
> + input_set_capability(dev, EV_KEY, map->buttons[i].key);
> +}
> +EXPORT_SYMBOL(ts_overlay_set_button_caps);
I don't see a need to expose this function and require participating drivers
to call it; we should just have one over-arching function for processing the
overlay(s), akin to touchscreen_parse_properties but for the button input
device in case the driver separates the button and touchscreen input devices.
That one function would then be responsible for parsing the overlay(s) and
calling input_set_capability() on each button.
> +
> +static int ts_overlay_count_buttons(struct device *dev)
> +{
> + struct fwnode_handle *child_node;
> + struct fwnode_handle *child_button;
These names confused me; they're both children, but only the second is aptly
named. How about child_overlay and child_button, or perhaps overlay_node and
button_node?
> + int count = 0;
> +
> + child_node = device_get_named_child_node(dev, ts_overlay_names[BUTTON]);
> + if (!child_node)
> + return 0;
> +
> + fwnode_for_each_child_node(child_node, child_button)
> + count++;
> + fwnode_handle_put(child_node);
> +
> + return count;
> +}
> +
> +static int ts_overlay_map_touchscreen(struct device *dev,
> + struct ts_overlay_map *map)
> +{
> + struct fwnode_handle *child;
Same here; there are two layers of children, so please use more descriptive
variable names.
> + int rc = 0;
> +
> + child = device_get_named_child_node(dev, ts_overlay_names[TOUCHSCREEN]);
> + if (!child)
> + goto touchscreen_ret;
I don't think we need a label here; just return 0 directly.
> +
> + map->touchscreen =
> + devm_kzalloc(dev, sizeof(*map->touchscreen), GFP_KERNEL);
> + if (!map->touchscreen) {
> + rc = -ENOMEM;
> + goto touchscreen_handle;
> + }
> + rc = ts_overlay_get_shape_properties(child, map->touchscreen);
> + if (rc < 0)
> + goto touchscreen_free;
> +
> + map->overlay_touchscreen = true;
> + dev_info(dev, "Added overlay touchscreen at (%u, %u), size %u x %u\n",
> + map->touchscreen->x_origin, map->touchscreen->y_origin,
> + map->touchscreen->x_size, map->touchscreen->y_size);
dev_dbg()
> +
> + rc = 0;
rc (error) can only be zero if we have gotten this far; I don't see a need
to assign it here.
> + goto touchscreen_handle;
Please think about whether this can be reorganized to prevent jumping over
small bits of code; I found it hard to follow. Maybe one or more tasks can
be offloaded to a helper function?
> +
> +touchscreen_free:
> + devm_kfree(dev, map->touchscreen);
This set off a red flag; it's unclear that it's necessary. Regardless of
whether the participating driver is smart enough to bail during probe()
if the overlay parsing fails, or it happily continues, this memory will
get freed when the driver tied to 'dev' is torn down.
Calling devm_kfree() is generally limited to special cases where you are
dynamically reallocating memory at runtime. In case I have misunderstood
the intent, please let me know.
> +touchscreen_handle:
> + fwnode_handle_put(child);
> +touchscreen_ret:
> + return rc;
> +}
> +
> +static int ts_overlay_map_buttons(struct device *dev,
> + struct ts_overlay_map *map,
> + struct input_dev *input)
> +{
> + struct fwnode_handle *child;
> + u32 button_count;
> + int rc = 0;
> +
> + button_count = ts_overlay_count_buttons(dev);
> + if (button_count) {
> + map->buttons = devm_kcalloc(dev, button_count,
> + sizeof(*map->buttons), GFP_KERNEL);
> + if (!map->buttons) {
> + rc = -ENOMEM;
> + goto map_buttons_ret;
> + }
> + child = device_get_named_child_node(dev,
> + ts_overlay_names[BUTTON]);
> + if (unlikely(!child))
> + goto map_buttons_free;
> +
> + rc = ts_overlay_get_button_properties(dev, child, map->buttons);
> + if (rc < 0)
> + goto map_buttons_free;
> +
> + map->button_count = button_count;
> + }
> +
> + return 0;
> +
> +map_buttons_free:
> + devm_kfree(dev, map->buttons);
> +map_buttons_ret:
> + return rc;
> +}
> +
> +static bool ts_overlay_defined_objects(struct device *dev)
> +{
> + struct fwnode_handle *child;
> + int i;
> +
> + for (i = 0; i < ARRAY_SIZE(ts_overlay_names); i++) {
> + child = device_get_named_child_node(dev, ts_overlay_names[i]);
> + if (child) {
> + fwnode_handle_put(child);
> + return true;
> + }
> + fwnode_handle_put(child);
> + }
> +
> + return false;
> +}
> +
> +struct ts_overlay_map *ts_overlay_map_objects(struct device *dev,
> + struct input_dev *input)
> +{
> + struct ts_overlay_map *map = NULL;
> + int rc;
> +
> + if (!ts_overlay_defined_objects(dev))
> + return NULL;
> +
> + map = devm_kzalloc(dev, sizeof(*map), GFP_KERNEL);
> + if (!map) {
> + rc = -ENOMEM;
> + goto objects_err;
> + }
> + rc = ts_overlay_map_touchscreen(dev, map);
> + if (rc < 0)
> + goto objects_free;
> +
> + rc = ts_overlay_map_buttons(dev, map, input);
> + if (rc < 0)
> + goto objects_free;
> +
> + return map;
> +
> +objects_free:
> + devm_kfree(dev, map);
> +objects_err:
> + return ERR_PTR(rc);
> +}
> +EXPORT_SYMBOL(ts_overlay_map_objects);
> +
> +void ts_overlay_get_touchscreen_abs(struct ts_overlay_map *map, u16 *x, u16 *y)
> +{
> + *x = map->touchscreen->x_size - 1;
> + *y = map->touchscreen->y_size - 1;
> +}
> +EXPORT_SYMBOL(ts_overlay_get_touchscreen_abs);
> +
> +static bool ts_overlay_shape_event(struct ts_overlay_shape *shape, u32 x, u32 y)
> +{
> + if (!shape)
> + return false;
> +
> + if (x >= shape->x_origin && x < (shape->x_origin + shape->x_size) &&
> + y >= shape->y_origin && y < (shape->y_origin + shape->y_size))
> + return true;
> +
> + return false;
> +}
> +
> +static bool ts_overlay_touchscreen_event(struct ts_overlay_shape *touchscreen,
> + u32 *x, u32 *y)
> +{
> + if (ts_overlay_shape_event(touchscreen, *x, *y)) {
> + *x -= touchscreen->x_origin;
> + *y -= touchscreen->y_origin;
> + return true;
> + }
> +
> + return false;
> +}
> +
> +bool ts_overlay_mapped_touchscreen(struct ts_overlay_map *map)
> +{
> + if (!map || !map->overlay_touchscreen)
> + return false;
> +
> + return true;
> +}
> +EXPORT_SYMBOL(ts_overlay_mapped_touchscreen);
> +
> +bool ts_overlay_mapped_buttons(struct ts_overlay_map *map)
> +{
> + if (!map || !map->button_count)
> + return false;
> +
> + return true;
> +}
> +EXPORT_SYMBOL(ts_overlay_mapped_buttons);
> +
> +bool ts_overlay_mt_on_touchscreen(struct ts_overlay_map *map, u32 *x, u32 *y)
> +{
> + if (!ts_overlay_mapped_touchscreen(map))
> + return true;
> +
> + if (!ts_overlay_touchscreen_event(map->touchscreen, x, y))
> + return false;
> +
> + return true;
> +}
> +EXPORT_SYMBOL(ts_overlay_mt_on_touchscreen);
> +
> +bool ts_overlay_button_press(struct ts_overlay_map *map,
> + struct input_dev *input, u32 x, u32 y, u32 slot)
> +{
> + int i;
> +
> + if (!ts_overlay_mapped_buttons(map))
> + return false;
> +
> + for (i = 0; i < map->button_count; i++) {
> + if (ts_overlay_shape_event(&map->buttons[i].shape, x, y)) {
> + input_report_key(input, map->buttons[i].key, 1);
> + map->buttons[i].pressed = true;
> + map->buttons[i].slot = slot;
> + return true;
> + }
> + }
> +
> + return false;
> +}
> +EXPORT_SYMBOL(ts_overlay_button_press);
The level of abstraction here does not seem quite right. Rather than force
each participating driver to call a press and release function, I think it
is better to expose something like touch_overlay_process_buttons() which
then handles the press and release events internally.
You're also relying on each individual driver to decide whether a touch
coordinate is inside or outside the overlay, and selectively call the press
and release functions OR report coordinates which is non-optimal.
To me, this says we actually need one wrapper function that accepts handles
to both the touchscreen and button input devices (which may be the same at
the driver's discretion) as well as the coordinates. If the coordinate is
within an overlay area, handle press/release; if not, call touchscreen_report_pos().
> +
> +bool ts_overlay_is_button_slot(struct ts_overlay_map *map, int slot)
> +{
> + int i;
> +
> + if (!map || !map->button_count)
> + return false;
> +
> + for (i = 0; i < map->button_count; i++) {
> + if (map->buttons[i].pressed && map->buttons[i].slot == slot)
> + return true;
> + }
> +
> + return false;
> +}
> +EXPORT_SYMBOL(ts_overlay_is_button_slot);
> +
> +void ts_overlay_button_release(struct ts_overlay_map *map,
> + struct input_dev *input, u32 slot)
> +{
> + int i;
> +
> + if (!map || !map->button_count)
> + return;
> +
> + for (i = 0; i < map->button_count; i++) {
> + if (map->buttons[i].pressed && map->buttons[i].slot == slot) {
> + input_report_key(input, map->buttons[i].key, 0);
> + map->buttons[i].pressed = false;
> + }
> + }
> +}
> +EXPORT_SYMBOL(ts_overlay_button_release);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("Helper functions for overlay objects on touchscreens");
> diff --git a/include/linux/input/ts-overlay.h b/include/linux/input/ts-overlay.h
> new file mode 100644
> index 000000000000..b75df0dec3ab
> --- /dev/null
> +++ b/include/linux/input/ts-overlay.h
> @@ -0,0 +1,43 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Copyright (c) 2023 Javier Carrasco <javier.carrasco@wolfvision.net>
> + */
> +
> +#ifndef _TS_OVERLAY
> +#define _TS_OVERLAY
> +
> +#include <linux/types.h>
> +
> +struct input_dev;
> +struct device;
> +
> +struct ts_overlay_map {
> + struct ts_overlay_shape *touchscreen;
> + bool overlay_touchscreen;
> + struct ts_overlay_button *buttons;
> + u32 button_count;
> +};
> +
> +struct ts_overlay_map *ts_overlay_map_objects(struct device *dev,
> + struct input_dev *input);
> +
> +void ts_overlay_get_touchscreen_abs(struct ts_overlay_map *map, u16 *x, u16 *y);
> +
> +bool ts_overlay_mapped_touchscreen(struct ts_overlay_map *map);
> +
> +bool ts_overlay_mapped_buttons(struct ts_overlay_map *map);
> +
> +bool ts_overlay_mt_on_touchscreen(struct ts_overlay_map *map, u32 *x, u32 *y);
> +
> +bool ts_overlay_button_press(struct ts_overlay_map *map,
> + struct input_dev *input, u32 x, u32 y, u32 slot);
> +
> +bool ts_overlay_is_button_slot(struct ts_overlay_map *map, int slot);
> +
> +void ts_overlay_button_release(struct ts_overlay_map *map,
> + struct input_dev *input, u32 slot);
> +
> +void ts_overlay_set_button_caps(struct ts_overlay_map *map,
> + struct input_dev *dev);
> +
> +#endif
>
> --
> 2.39.2
>
Kind regards,
Jeff LaBundy
^ permalink raw reply
* Re: [PATCH v3 4/4] input: touchscreen: add SPI support for Goodix Berlin Touchscreen IC
From: Jeff LaBundy @ 2023-06-25 19:38 UTC (permalink / raw)
To: Neil Armstrong
Cc: Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Bastien Nocera, Hans de Goede, Henrik Rydberg, linux-input,
linux-arm-msm, devicetree, linux-kernel
In-Reply-To: <20230606-topic-goodix-berlin-upstream-initial-v3-4-f0577cead709@linaro.org>
Hi Neil,
On Thu, Jun 22, 2023 at 04:29:02PM +0200, Neil Armstrong wrote:
> Add initial support for the new Goodix "Berlin" touchscreen ICs
> over the SPI interface.
>
> The driver doesn't use the regmap_spi code since the SPI messages
> needs to be prefixed, thus this custom regmap code.
>
> This initial driver is derived from the Goodix goodix_ts_berlin
> available at [1] and [2] and only supports the GT9916 IC
> present on the Qualcomm SM8550 MTP & QRD touch panel.
>
> The current implementation only supports BerlinD, aka GT9916.
>
> [1] https://github.com/goodix/goodix_ts_berlin
> [2] https://git.codelinaro.org/clo/la/platform/vendor/opensource/touch-drivers
>
> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
> ---
Just a few comments below, then feel free to add:
Reviewed-by: Jeff LaBundy <jeff@labundy.com>
> drivers/input/touchscreen/Kconfig | 13 ++
> drivers/input/touchscreen/Makefile | 1 +
> drivers/input/touchscreen/goodix_berlin_spi.c | 172 ++++++++++++++++++++++++++
> 3 files changed, 186 insertions(+)
>
> diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
> index 5e21cca6025d..2d86615e5090 100644
> --- a/drivers/input/touchscreen/Kconfig
> +++ b/drivers/input/touchscreen/Kconfig
> @@ -435,6 +435,19 @@ config TOUCHSCREEN_GOODIX_BERLIN_I2C
> To compile this driver as a module, choose M here: the
> module will be called goodix_berlin_i2c.
>
> +config TOUCHSCREEN_GOODIX_BERLIN_SPI
> + tristate "Goodix Berlin SPI touchscreen"
> + depends on SPI_MASTER
select REGMAP
(keep "depends on SPI_MASTER")
> + select TOUCHSCREEN_GOODIX_BERLIN_CORE
> + help
> + Say Y here if you have a Goodix Berlin IC connected to
> + your system via SPI.
> +
> + If unsure, say N.
> +
> + To compile this driver as a module, choose M here: the
> + module will be called goodix_berlin_spi.
> +
> config TOUCHSCREEN_HIDEEP
> tristate "HiDeep Touch IC"
> depends on I2C
> diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile
> index 921a2da0c2be..29524e8a83db 100644
> --- a/drivers/input/touchscreen/Makefile
> +++ b/drivers/input/touchscreen/Makefile
> @@ -49,6 +49,7 @@ obj-$(CONFIG_TOUCHSCREEN_FUJITSU) += fujitsu_ts.o
> obj-$(CONFIG_TOUCHSCREEN_GOODIX) += goodix_ts.o
> obj-$(CONFIG_TOUCHSCREEN_GOODIX_BERLIN_CORE) += goodix_berlin_core.o
> obj-$(CONFIG_TOUCHSCREEN_GOODIX_BERLIN_I2C) += goodix_berlin_i2c.o
> +obj-$(CONFIG_TOUCHSCREEN_GOODIX_BERLIN_SPI) += goodix_berlin_spi.o
> obj-$(CONFIG_TOUCHSCREEN_HIDEEP) += hideep.o
> obj-$(CONFIG_TOUCHSCREEN_HYNITRON_CSTXXX) += hynitron_cstxxx.o
> obj-$(CONFIG_TOUCHSCREEN_ILI210X) += ili210x.o
> diff --git a/drivers/input/touchscreen/goodix_berlin_spi.c b/drivers/input/touchscreen/goodix_berlin_spi.c
> new file mode 100644
> index 000000000000..3a1bc251b32d
> --- /dev/null
> +++ b/drivers/input/touchscreen/goodix_berlin_spi.c
> @@ -0,0 +1,172 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Goodix Berlin Touchscreen Driver
> + *
> + * Copyright (C) 2020 - 2021 Goodix, Inc.
> + * Copyright (C) 2023 Linaro Ltd.
> + *
> + * Based on goodix_ts_berlin driver.
> + */
> +#include <asm/unaligned.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/regmap.h>
> +#include <linux/spi/spi.h>
> +
> +#include "goodix_berlin.h"
> +
> +#define SPI_TRANS_PREFIX_LEN 1
> +#define REGISTER_WIDTH 4
> +#define SPI_READ_DUMMY_LEN 3
> +#define SPI_READ_PREFIX_LEN (SPI_TRANS_PREFIX_LEN + REGISTER_WIDTH + SPI_READ_DUMMY_LEN)
> +#define SPI_WRITE_PREFIX_LEN (SPI_TRANS_PREFIX_LEN + REGISTER_WIDTH)
> +
> +#define SPI_WRITE_FLAG 0xF0
> +#define SPI_READ_FLAG 0xF1
Please namespace all of these as you have done in the core driver.
> +
> +static int goodix_berlin_spi_read(void *context, const void *reg_buf,
> + size_t reg_size, void *val_buf,
> + size_t val_size)
> +{
> + struct spi_device *spi = context;
> + struct spi_transfer xfers;
> + struct spi_message spi_msg;
> + const u32 *reg = reg_buf; /* reg is stored as native u32 at start of buffer */
> + u8 *buf;
> + int ret;
int error;
> +
> + if (reg_size != REGISTER_WIDTH)
> + return -EINVAL;
> +
> + buf = kzalloc(SPI_READ_PREFIX_LEN + val_size, GFP_KERNEL);
> + if (!buf)
> + return -ENOMEM;
> +
> + spi_message_init(&spi_msg);
> + memset(&xfers, 0, sizeof(xfers));
> +
> + /* buffer format: 0xF1 + addr(4bytes) + dummy(3bytes) + data */
> + buf[0] = SPI_READ_FLAG;
> + put_unaligned_be32(*reg, buf + SPI_TRANS_PREFIX_LEN);
> + memset(buf + SPI_TRANS_PREFIX_LEN + REGISTER_WIDTH, 0xff,
> + SPI_READ_DUMMY_LEN);
> +
> + xfers.tx_buf = buf;
> + xfers.rx_buf = buf;
> + xfers.len = SPI_READ_PREFIX_LEN + val_size;
> + xfers.cs_change = 0;
> + spi_message_add_tail(&xfers, &spi_msg);
> +
> + ret = spi_sync(spi, &spi_msg);
error = spi_sync(...);
> + if (ret < 0)
if (error)
> + dev_err(&spi->dev, "transfer error:%d", ret);
> + else
> + memcpy(val_buf, buf + SPI_READ_PREFIX_LEN, val_size);
> +
> + kfree(buf);
> + return ret;
> +}
> +
> +static int goodix_berlin_spi_write(void *context, const void *data,
> + size_t count)
> +{
> + unsigned int len = count - REGISTER_WIDTH;
> + struct spi_device *spi = context;
> + struct spi_transfer xfers;
> + struct spi_message spi_msg;
> + const u32 *reg = data; /* reg is stored as native u32 at start of buffer */
> + u8 *buf;
> + int ret;
Same comments here regarding 'error' vs. 'ret'.
> +
> + buf = kzalloc(SPI_WRITE_PREFIX_LEN + len, GFP_KERNEL);
> + if (!buf)
> + return -ENOMEM;
> +
> + spi_message_init(&spi_msg);
> + memset(&xfers, 0, sizeof(xfers));
> +
> + buf[0] = SPI_WRITE_FLAG;
> + put_unaligned_be32(*reg, buf + SPI_TRANS_PREFIX_LEN);
> + memcpy(buf + SPI_WRITE_PREFIX_LEN, data + REGISTER_WIDTH, len);
> +
> + xfers.tx_buf = buf;
> + xfers.len = SPI_WRITE_PREFIX_LEN + len;
> + xfers.cs_change = 0;
> + spi_message_add_tail(&xfers, &spi_msg);
> +
> + ret = spi_sync(spi, &spi_msg);
> + if (ret < 0)
> + dev_err(&spi->dev, "transfer error:%d", ret);
> +
> + kfree(buf);
> + return ret;
> +}
> +
> +static const struct regmap_config goodix_berlin_spi_regmap_conf = {
> + .reg_bits = 32,
> + .val_bits = 8,
> + .read = goodix_berlin_spi_read,
> + .write = goodix_berlin_spi_write,
> +};
> +
> +/* vendor & product left unassigned here, should probably be updated from fw info */
> +static const struct input_id goodix_berlin_spi_input_id = {
> + .bustype = BUS_SPI,
> +};
> +
> +static int goodix_berlin_spi_probe(struct spi_device *spi)
> +{
> + struct regmap_config *regmap_config;
> + struct regmap *regmap;
> + size_t max_size;
> + int error = 0;
> +
> + regmap_config = devm_kmemdup(&spi->dev, &goodix_berlin_spi_regmap_conf,
> + sizeof(*regmap_config), GFP_KERNEL);
> + if (!regmap_config)
> + return -ENOMEM;
Is there any reason we cannot simply pass goodix_berlin_spi_regmap_conf to
devm_regmap_init() below? Why to duplicate and pass the copy?
For reference, BMP280 in IIO is a similar example of a device with regmap
sitting atop a bespoke SPI protocol; it does not seem to take this extra
step.
> +
> + spi->mode = SPI_MODE_0;
> + spi->bits_per_word = 8;
> + error = spi_setup(spi);
> + if (error)
> + return error;
> +
> + max_size = spi_max_transfer_size(spi);
> + regmap_config->max_raw_read = max_size - SPI_READ_PREFIX_LEN;
> + regmap_config->max_raw_write = max_size - SPI_WRITE_PREFIX_LEN;
> +
> + regmap = devm_regmap_init(&spi->dev, NULL, spi, regmap_config);
> + if (IS_ERR(regmap))
> + return PTR_ERR(regmap);
> +
> + return goodix_berlin_probe(&spi->dev, spi->irq,
> + &goodix_berlin_spi_input_id, regmap);
> +}
> +
> +static const struct spi_device_id goodix_berlin_spi_ids[] = {
> + { "gt9916" },
> + { },
> +};
> +MODULE_DEVICE_TABLE(spi, goodix_berlin_spi_ids);
> +
> +static const struct of_device_id goodix_berlin_spi_of_match[] = {
> + { .compatible = "goodix,gt9916", },
> + { }
> +};
> +MODULE_DEVICE_TABLE(of, goodix_berlin_spi_of_match);
> +
> +static struct spi_driver goodix_berlin_spi_driver = {
> + .driver = {
> + .name = "goodix-berlin-spi",
> + .of_match_table = goodix_berlin_spi_of_match,
> + .pm = pm_sleep_ptr(&goodix_berlin_pm_ops),
> + },
> + .probe = goodix_berlin_spi_probe,
> + .id_table = goodix_berlin_spi_ids,
> +};
> +module_spi_driver(goodix_berlin_spi_driver);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("Goodix Berlin SPI Touchscreen driver");
> +MODULE_AUTHOR("Neil Armstrong <neil.armstrong@linaro.org>");
>
> --
> 2.34.1
>
Kind regards,
Jeff LaBundy
^ permalink raw reply
* Re: [PATCH v3 3/4] input: touchscreen: add I2C support for Goodix Berlin Touchscreen IC
From: Jeff LaBundy @ 2023-06-25 19:17 UTC (permalink / raw)
To: Neil Armstrong
Cc: Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Bastien Nocera, Hans de Goede, Henrik Rydberg, linux-input,
linux-arm-msm, devicetree, linux-kernel
In-Reply-To: <20230606-topic-goodix-berlin-upstream-initial-v3-3-f0577cead709@linaro.org>
Hi Neil,
On Thu, Jun 22, 2023 at 04:29:01PM +0200, Neil Armstrong wrote:
> Add initial support for the new Goodix "Berlin" touchscreen ICs
> over the I2C interface.
>
> This initial driver is derived from the Goodix goodix_ts_berlin
> available at [1] and [2] and only supports the GT9916 IC
> present on the Qualcomm SM8550 MTP & QRD touch panel.
>
> The current implementation only supports BerlinD, aka GT9916.
>
> [1] https://github.com/goodix/goodix_ts_berlin
> [2] https://git.codelinaro.org/clo/la/platform/vendor/opensource/touch-drivers
>
> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
> ---
Just one comment below, then feel free to add:
Reviewed-by: Jeff LaBundy <jeff@labundy.com>
> drivers/input/touchscreen/Kconfig | 14 ++++++
> drivers/input/touchscreen/Makefile | 1 +
> drivers/input/touchscreen/goodix_berlin_i2c.c | 69 +++++++++++++++++++++++++++
> 3 files changed, 84 insertions(+)
>
> diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
> index 1a6f6f6da991..5e21cca6025d 100644
> --- a/drivers/input/touchscreen/Kconfig
> +++ b/drivers/input/touchscreen/Kconfig
> @@ -421,6 +421,20 @@ config TOUCHSCREEN_GOODIX_BERLIN_CORE
> depends on REGMAP
> tristate
>
> +config TOUCHSCREEN_GOODIX_BERLIN_I2C
> + tristate "Goodix Berlin I2C touchscreen"
> + depends on I2C
> + depends on REGMAP_I2C
select REGMAP_I2C
(keep "depends on I2C")
> + select TOUCHSCREEN_GOODIX_BERLIN_CORE
> + help
> + Say Y here if you have a Goodix Berlin IC connected to
> + your system via I2C.
> +
> + If unsure, say N.
> +
> + To compile this driver as a module, choose M here: the
> + module will be called goodix_berlin_i2c.
> +
> config TOUCHSCREEN_HIDEEP
> tristate "HiDeep Touch IC"
> depends on I2C
> diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile
> index 29cdb042e104..921a2da0c2be 100644
> --- a/drivers/input/touchscreen/Makefile
> +++ b/drivers/input/touchscreen/Makefile
> @@ -48,6 +48,7 @@ obj-$(CONFIG_TOUCHSCREEN_EXC3000) += exc3000.o
> obj-$(CONFIG_TOUCHSCREEN_FUJITSU) += fujitsu_ts.o
> obj-$(CONFIG_TOUCHSCREEN_GOODIX) += goodix_ts.o
> obj-$(CONFIG_TOUCHSCREEN_GOODIX_BERLIN_CORE) += goodix_berlin_core.o
> +obj-$(CONFIG_TOUCHSCREEN_GOODIX_BERLIN_I2C) += goodix_berlin_i2c.o
> obj-$(CONFIG_TOUCHSCREEN_HIDEEP) += hideep.o
> obj-$(CONFIG_TOUCHSCREEN_HYNITRON_CSTXXX) += hynitron_cstxxx.o
> obj-$(CONFIG_TOUCHSCREEN_ILI210X) += ili210x.o
> diff --git a/drivers/input/touchscreen/goodix_berlin_i2c.c b/drivers/input/touchscreen/goodix_berlin_i2c.c
> new file mode 100644
> index 000000000000..6407b2258eb1
> --- /dev/null
> +++ b/drivers/input/touchscreen/goodix_berlin_i2c.c
> @@ -0,0 +1,69 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Goodix Berlin Touchscreen Driver
> + *
> + * Copyright (C) 2020 - 2021 Goodix, Inc.
> + * Copyright (C) 2023 Linaro Ltd.
> + *
> + * Based on goodix_ts_berlin driver.
> + */
> +#include <linux/i2c.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/regmap.h>
> +
> +#include "goodix_berlin.h"
> +
> +#define I2C_MAX_TRANSFER_SIZE 256
> +
> +static const struct regmap_config goodix_berlin_i2c_regmap_conf = {
> + .reg_bits = 32,
> + .val_bits = 8,
> + .max_raw_read = I2C_MAX_TRANSFER_SIZE,
> + .max_raw_write = I2C_MAX_TRANSFER_SIZE,
> +};
> +
> +/* vendor & product left unassigned here, should probably be updated from fw info */
> +static const struct input_id goodix_berlin_i2c_input_id = {
> + .bustype = BUS_I2C,
> +};
> +
> +static int goodix_berlin_i2c_probe(struct i2c_client *client)
> +{
> + struct regmap *regmap;
> +
> + regmap = devm_regmap_init_i2c(client, &goodix_berlin_i2c_regmap_conf);
> + if (IS_ERR(regmap))
> + return PTR_ERR(regmap);
> +
> + return goodix_berlin_probe(&client->dev, client->irq,
> + &goodix_berlin_i2c_input_id, regmap);
> +}
> +
> +static const struct i2c_device_id goodix_berlin_i2c_id[] = {
> + { "gt9916", 0 },
> + { }
> +};
> +
> +MODULE_DEVICE_TABLE(i2c, goodix_berlin_i2c_id);
> +
> +static const struct of_device_id goodix_berlin_i2c_of_match[] = {
> + { .compatible = "goodix,gt9916", },
> + { }
> +};
> +MODULE_DEVICE_TABLE(of, goodix_berlin_i2c_of_match);
> +
> +static struct i2c_driver goodix_berlin_i2c_driver = {
> + .driver = {
> + .name = "goodix-berlin-i2c",
> + .of_match_table = goodix_berlin_i2c_of_match,
> + .pm = pm_sleep_ptr(&goodix_berlin_pm_ops),
> + },
> + .probe = goodix_berlin_i2c_probe,
> + .id_table = goodix_berlin_i2c_id,
> +};
> +module_i2c_driver(goodix_berlin_i2c_driver);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("Goodix Berlin I2C Touchscreen driver");
> +MODULE_AUTHOR("Neil Armstrong <neil.armstrong@linaro.org>");
>
> --
> 2.34.1
>
Kind regards,
Jeff LaBundy
^ permalink raw reply
* Re: [PATCH v3 2/4] input: touchscreen: add core support for Goodix Berlin Touchscreen IC
From: Jeff LaBundy @ 2023-06-25 19:14 UTC (permalink / raw)
To: Neil Armstrong
Cc: Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Bastien Nocera, Hans de Goede, Henrik Rydberg, linux-input,
linux-arm-msm, devicetree, linux-kernel
In-Reply-To: <20230606-topic-goodix-berlin-upstream-initial-v3-2-f0577cead709@linaro.org>
Hi Neil,
On Thu, Jun 22, 2023 at 04:29:00PM +0200, Neil Armstrong wrote:
> Add initial support for the new Goodix "Berlin" touchscreen ICs.
>
> These touchscreen ICs support SPI, I2C and I3C interface, up to
> 10 finger touch, stylus and gestures events.
>
> This initial driver is derived from the Goodix goodix_ts_berlin
> available at [1] and [2] and only supports the GT9916 IC
> present on the Qualcomm SM8550 MTP & QRD touch panel.
>
> The current implementation only supports BerlinD, aka GT9916.
>
> Support for advanced features like:
> - Firmware & config update
> - Stylus events
> - Gestures events
> - Previous revisions support (BerlinA or BerlinB)
> is not included in current version.
>
> The current support will work with currently flashed firmware
> and config, and bail out if firmware or config aren't flashed yet.
>
> [1] https://github.com/goodix/goodix_ts_berlin
> [2] https://git.codelinaro.org/clo/la/platform/vendor/opensource/touch-drivers
>
> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
> ---
Great work; thank you for the productive discussion. I only had some
minor cosmetic comments this last time; once those are addressed, feel
free to add:
Reviewed-by: Jeff LaBundy <jeff@labundy.com>
> drivers/input/touchscreen/Kconfig | 5 +
> drivers/input/touchscreen/Makefile | 1 +
> drivers/input/touchscreen/goodix_berlin.h | 159 +++++++
> drivers/input/touchscreen/goodix_berlin_core.c | 584 +++++++++++++++++++++++++
> 4 files changed, 749 insertions(+)
>
> diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
> index c2cbd332af1d..1a6f6f6da991 100644
> --- a/drivers/input/touchscreen/Kconfig
> +++ b/drivers/input/touchscreen/Kconfig
> @@ -416,6 +416,11 @@ config TOUCHSCREEN_GOODIX
> To compile this driver as a module, choose M here: the
> module will be called goodix.
>
> +config TOUCHSCREEN_GOODIX_BERLIN_CORE
> + depends on GPIOLIB || COMPILE_TEST
My comment seems to have been missed; this driver does not actually depend
on GPIOLIB and the dependency here can be dropped. However, this would leave
the driver to be built only if COMPILE_TEST is set which is obviously not
what we want. Therefore, this line can simply be dropped entirely.
> + depends on REGMAP
Rather than depending on REGMAP here, you should simply select the appropriate
transport from the I2C or SPI driver (which seems to have been missed in those
patches as well; I will follow up there). In the meantime, this line can just
be dropped.
MFD has several examples of dual I2C/SPI devices that demonstrate the correct
pattern; see any of those for reference.
> + tristate
> +
> config TOUCHSCREEN_HIDEEP
> tristate "HiDeep Touch IC"
> depends on I2C
> diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile
> index 159cd5136fdb..29cdb042e104 100644
> --- a/drivers/input/touchscreen/Makefile
> +++ b/drivers/input/touchscreen/Makefile
> @@ -47,6 +47,7 @@ obj-$(CONFIG_TOUCHSCREEN_EGALAX_SERIAL) += egalax_ts_serial.o
> obj-$(CONFIG_TOUCHSCREEN_EXC3000) += exc3000.o
> obj-$(CONFIG_TOUCHSCREEN_FUJITSU) += fujitsu_ts.o
> obj-$(CONFIG_TOUCHSCREEN_GOODIX) += goodix_ts.o
> +obj-$(CONFIG_TOUCHSCREEN_GOODIX_BERLIN_CORE) += goodix_berlin_core.o
> obj-$(CONFIG_TOUCHSCREEN_HIDEEP) += hideep.o
> obj-$(CONFIG_TOUCHSCREEN_HYNITRON_CSTXXX) += hynitron_cstxxx.o
> obj-$(CONFIG_TOUCHSCREEN_ILI210X) += ili210x.o
> diff --git a/drivers/input/touchscreen/goodix_berlin.h b/drivers/input/touchscreen/goodix_berlin.h
> new file mode 100644
> index 000000000000..235f44947a28
> --- /dev/null
> +++ b/drivers/input/touchscreen/goodix_berlin.h
> @@ -0,0 +1,159 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Goodix Touchscreen Driver
> + * Copyright (C) 2020 - 2021 Goodix, Inc.
> + * Copyright (C) 2023 Linaro Ltd.
> + *
> + * Based on goodix_berlin_berlin driver.
> + */
> +
> +#ifndef __GOODIX_BERLIN_H_
> +#define __GOODIX_BERLIN_H_
> +
> +#include <linux/gpio/consumer.h>
> +#include <linux/input.h>
> +#include <linux/input/touchscreen.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/sizes.h>
> +
> +#define GOODIX_BERLIN_MAX_TOUCH 10
> +
> +#define GOODIX_BERLIN_NORMAL_RESET_DELAY_MS 100
> +
> +#define GOODIX_BERLIN_IRQ_EVENT_HEAD_LEN 8
> +#define GOODIX_BERLIN_STATUS_OFFSET 0
> +#define GOODIX_BERLIN_REQUEST_TYPE_OFFSET 2
> +
> +#define GOODIX_BERLIN_BYTES_PER_POINT 8
> +#define GOODIX_BERLIN_COOR_DATA_CHECKSUM_SIZE 2
> +#define GOODIX_BERLIN_COOR_DATA_CHECKSUM_MASK GENMASK(15, 0)
> +
> +/* Read n finger events */
> +#define GOODIX_BERLIN_IRQ_READ_LEN(n) (GOODIX_BERLIN_IRQ_EVENT_HEAD_LEN + \
> + (GOODIX_BERLIN_BYTES_PER_POINT * (n)) + \
> + GOODIX_BERLIN_COOR_DATA_CHECKSUM_SIZE)
> +
> +#define GOODIX_BERLIN_TOUCH_EVENT BIT(7)
> +#define GOODIX_BERLIN_REQUEST_EVENT BIT(6)
> +#define GOODIX_BERLIN_TOUCH_COUNT_MASK GENMASK(3, 0)
> +
> +#define GOODIX_BERLIN_REQUEST_CODE_RESET 3
> +
> +#define GOODIX_BERLIN_POINT_TYPE_MASK GENMASK(3, 0)
> +#define GOODIX_BERLIN_POINT_TYPE_STYLUS_HOVER 1
> +#define GOODIX_BERLIN_POINT_TYPE_STYLUS 3
> +
> +#define GOODIX_BERLIN_TOUCH_ID_MASK GENMASK(7, 4)
> +
> +#define GOODIX_BERLIN_DEV_CONFIRM_VAL 0xAA
> +#define GOODIX_BERLIN_BOOTOPTION_ADDR 0x10000
> +#define GOODIX_BERLIN_FW_VERSION_INFO_ADDR 0x10014
> +
> +#define GOODIX_BERLIN_IC_INFO_MAX_LEN SZ_1K
> +#define GOODIX_BERLIN_IC_INFO_ADDR 0x10070
> +
> +struct goodix_berlin_fw_version {
> + u8 rom_pid[6];
> + u8 rom_vid[3];
> + u8 rom_vid_reserved;
> + u8 patch_pid[8];
> + u8 patch_vid[4];
> + u8 patch_vid_reserved;
> + u8 sensor_id;
> + u8 reserved[2];
> + __le16 checksum;
> +} __packed;
> +
> +struct goodix_berlin_ic_info_version {
> + u8 info_customer_id;
> + u8 info_version_id;
> + u8 ic_die_id;
> + u8 ic_version_id;
> + __le32 config_id;
> + u8 config_version;
> + u8 frame_data_customer_id;
> + u8 frame_data_version_id;
> + u8 touch_data_customer_id;
> + u8 touch_data_version_id;
> + u8 reserved[3];
> +} __packed;
> +
> +struct goodix_berlin_ic_info_feature {
> + __le16 freqhop_feature;
> + __le16 calibration_feature;
> + __le16 gesture_feature;
> + __le16 side_touch_feature;
> + __le16 stylus_feature;
> +} __packed;
> +
> +struct goodix_berlin_ic_info_misc {
> + __le32 cmd_addr;
> + __le16 cmd_max_len;
> + __le32 cmd_reply_addr;
> + __le16 cmd_reply_len;
> + __le32 fw_state_addr;
> + __le16 fw_state_len;
> + __le32 fw_buffer_addr;
> + __le16 fw_buffer_max_len;
> + __le32 frame_data_addr;
> + __le16 frame_data_head_len;
> + __le16 fw_attr_len;
> + __le16 fw_log_len;
> + u8 pack_max_num;
> + u8 pack_compress_version;
> + __le16 stylus_struct_len;
> + __le16 mutual_struct_len;
> + __le16 self_struct_len;
> + __le16 noise_struct_len;
> + __le32 touch_data_addr;
> + __le16 touch_data_head_len;
> + __le16 point_struct_len;
> + __le16 reserved1;
> + __le16 reserved2;
> + __le32 mutual_rawdata_addr;
> + __le32 mutual_diffdata_addr;
> + __le32 mutual_refdata_addr;
> + __le32 self_rawdata_addr;
> + __le32 self_diffdata_addr;
> + __le32 self_refdata_addr;
> + __le32 iq_rawdata_addr;
> + __le32 iq_refdata_addr;
> + __le32 im_rawdata_addr;
> + __le16 im_readata_len;
> + __le32 noise_rawdata_addr;
> + __le16 noise_rawdata_len;
> + __le32 stylus_rawdata_addr;
> + __le16 stylus_rawdata_len;
> + __le32 noise_data_addr;
> + __le32 esd_addr;
> +} __packed;
> +
> +struct goodix_berlin_touch_data {
> + u8 id;
> + u8 unused;
> + __le16 x;
> + __le16 y;
> + __le16 w;
> +} __packed;
> +
> +struct goodix_berlin_core {
> + struct device *dev;
> + struct regmap *regmap;
> + struct regulator *avdd;
> + struct regulator *iovdd;
> + struct gpio_desc *reset_gpio;
> + struct touchscreen_properties props;
> + struct goodix_berlin_fw_version fw_version;
> + struct input_dev *input_dev;
> + int irq;
> +
> + /* Runtime parameters extracted from IC_INFO buffer */
> + u32 touch_data_addr;
> +};
> +
> +int goodix_berlin_probe(struct device *dev, int irq, const struct input_id *id,
> + struct regmap *regmap);
> +
> +extern const struct dev_pm_ops goodix_berlin_pm_ops;
> +
> +#endif
> diff --git a/drivers/input/touchscreen/goodix_berlin_core.c b/drivers/input/touchscreen/goodix_berlin_core.c
> new file mode 100644
> index 000000000000..af3e73bbb3ec
> --- /dev/null
> +++ b/drivers/input/touchscreen/goodix_berlin_core.c
> @@ -0,0 +1,584 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Goodix Touchscreen Driver
> + * Copyright (C) 2020 - 2021 Goodix, Inc.
> + * Copyright (C) 2023 Linaro Ltd.
> + *
> + * Based on goodix_ts_berlin driver.
> + */
> +#include <asm/unaligned.h>
> +#include <linux/input/mt.h>
> +#include <linux/input/touchscreen.h>
> +#include <linux/regmap.h>
> +
> +#include "goodix_berlin.h"
> +
> +/*
> + * Goodix "Berlin" Touchscreen ID driver
> + *
> + * This driver is distinct from goodix.c since hardware interface
> + * is different enough to require a new driver.
> + * None of the register address or data structure are close enough
> + * to the previous generations.
> + *
> + * Currently only handles Multitouch events with already
> + * programmed firmware and "config" for "Revision D" Berlin IC.
> + *
> + * Support is missing for:
> + * - ESD Management
> + * - Firmware update/flashing
> + * - "Config" update/flashing
> + * - Stylus Events
> + * - Gesture Events
> + * - Support for older revisions (A & B)
> + */
> +
> +static bool goodix_berlin_checksum_valid(const u8 *data, int size)
> +{
> + u32 cal_checksum = 0;
> + u16 r_checksum;
> + u32 i;
> +
> + if (size < GOODIX_BERLIN_COOR_DATA_CHECKSUM_SIZE)
> + return false;
> +
> + for (i = 0; i < size - GOODIX_BERLIN_COOR_DATA_CHECKSUM_SIZE; i++)
> + cal_checksum += data[i];
> +
> + r_checksum = get_unaligned_le16(&data[i]);
> +
> + return FIELD_GET(GOODIX_BERLIN_COOR_DATA_CHECKSUM_MASK, cal_checksum) == r_checksum;
> +}
> +
> +static bool goodix_berlin_is_dummy_data(struct goodix_berlin_core *cd,
> + const u8 *data, int size)
> +{
> + int i;
> +
> + /*
> + * If the device is missing or doesn't respond the buffer
> + * could be filled with bus default line state, 0x00 or 0xff,
> + * so declare success the first time we encounter neither.
> + */
> + for (i = 0; i < size; i++)
> + if (data[i] > 0 && data[i] < 0xff)
> + return false;
> +
> + return true;
> +}
> +
> +static int goodix_berlin_dev_confirm(struct goodix_berlin_core *cd)
> +{
> + u8 tx_buf[8], rx_buf[8];
> + int retry = 3;
> + int error;
> +
> + memset(tx_buf, GOODIX_BERLIN_DEV_CONFIRM_VAL, sizeof(tx_buf));
> + while (retry--) {
> + error = regmap_raw_write(cd->regmap, GOODIX_BERLIN_BOOTOPTION_ADDR, tx_buf,
> + sizeof(tx_buf));
> + if (error)
> + return error;
> +
> + error = regmap_raw_read(cd->regmap, GOODIX_BERLIN_BOOTOPTION_ADDR, rx_buf,
> + sizeof(rx_buf));
> + if (error)
> + return error;
> +
> + if (!memcmp(tx_buf, rx_buf, sizeof(tx_buf)))
> + return 0;
> +
> + usleep_range(5000, 5100);
> + }
> +
> + dev_err(cd->dev, "device confirm failed, rx_buf: %*ph\n", 8, rx_buf);
> +
> + return -EINVAL;
> +}
> +
> +static int goodix_berlin_power_on(struct goodix_berlin_core *cd, bool on)
> +{
> + int error = 0;
> +
> + if (on) {
> + error = regulator_enable(cd->iovdd);
> + if (error) {
> + dev_err(cd->dev, "Failed to enable iovdd: %d\n", error);
> + return error;
> + }
> +
> + /* Vendor waits 3ms for IOVDD to settle */
> + usleep_range(3000, 3100);
> +
> + error = regulator_enable(cd->avdd);
> + if (error) {
> + dev_err(cd->dev, "Failed to enable avdd: %d\n", error);
> + goto error_avdd_regulator;
> + }
> +
> + /* Vendor waits 15ms for IOVDD to settle */
> + usleep_range(15000, 15100);
> +
> + gpiod_set_value(cd->reset_gpio, 0);
> +
> + /* Vendor waits 4ms for Firmware to initialize */
> + usleep_range(4000, 4100);
> +
> + error = goodix_berlin_dev_confirm(cd);
> + if (error)
> + goto error_dev_confirm;
> +
> + /* Vendor waits 100ms for Firmware to fully boot */
> + msleep(GOODIX_BERLIN_NORMAL_RESET_DELAY_MS);
> +
> + return 0;
> + }
> +
> +error_dev_confirm:
> + gpiod_set_value(cd->reset_gpio, 1);
These labels are still confusing in my opinion. Rather than name the label
based on the code that could have gotten us here, it is better for the name
to reflect the action that follows.
How about err_dev_reset and err_iovdd_disable?
> +
> + regulator_disable(cd->avdd);
> +
> +error_avdd_regulator:
> + regulator_disable(cd->iovdd);
> +
> + return error;
> +}
> +
> +static int goodix_berlin_read_version(struct goodix_berlin_core *cd)
> +{
> + u8 buf[sizeof(struct goodix_berlin_fw_version)];
> + int error;
> +
> + error = regmap_raw_read(cd->regmap, GOODIX_BERLIN_FW_VERSION_INFO_ADDR, buf, sizeof(buf));
> + if (error) {
> + dev_err(cd->dev, "error reading fw version\n");
It's handy to print the return value here as you do elsewhere.
> + return error;
> + }
> +
> + if (!goodix_berlin_checksum_valid(buf, sizeof(buf))) {
> + dev_err(cd->dev, "invalid fw version: checksum error\n");
> + return -EINVAL;
> + }
> +
> + memcpy(&cd->fw_version, buf, sizeof(cd->fw_version));
> +
> + return 0;
> +}
> +
> +/* Only extract necessary data for runtime */
> +static int goodix_berlin_convert_ic_info(struct goodix_berlin_core *cd,
> + const u8 *data, u16 length)
> +{
> + struct goodix_berlin_ic_info_misc misc;
> + unsigned int offset = 0;
> + u8 param_num;
> +
> + offset += sizeof(__le16); /* length */
> + offset += sizeof(struct goodix_berlin_ic_info_version);
> + offset += sizeof(struct goodix_berlin_ic_info_feature);
> +
> + /* IC_INFO Parameters, variable width structure */
> + offset += 4 * sizeof(u8); /* drv_num, sen_num, button_num, force_num */
> +
> + if (offset >= length)
> + goto invalid_offset;
> +
> + param_num = data[offset++]; /* active_scan_rate_num */
> +
> + offset += param_num * sizeof(__le16);
> +
> + if (offset >= length)
> + goto invalid_offset;
> +
> + param_num = data[offset++]; /* mutual_freq_num*/
> +
> + offset += param_num * sizeof(__le16);
> +
> + if (offset >= length)
> + goto invalid_offset;
> +
> + param_num = data[offset++]; /* self_tx_freq_num */
> +
> + offset += param_num * sizeof(__le16);
> +
> + if (offset >= length)
> + goto invalid_offset;
> +
> + param_num = data[offset++]; /* self_rx_freq_num */
> +
> + offset += param_num * sizeof(__le16);
> +
> + if (offset >= length)
> + goto invalid_offset;
> +
> + param_num = data[offset++]; /* stylus_freq_num */
> +
> + offset += param_num * sizeof(__le16);
> +
> + if (offset + sizeof(misc) > length)
> + goto invalid_offset;
> +
> + /* goodix_berlin_ic_info_misc */
> + memcpy(&misc, &data[offset], sizeof(misc));
> +
> + cd->touch_data_addr = le32_to_cpu(misc.touch_data_addr);
> +
> + return 0;
> +
> +invalid_offset:
> + dev_err(cd->dev, "ic_info length is invalid (offset %d length %d)\n",
> + offset, length);
> + return -EINVAL;
> +}
> +
> +static int goodix_berlin_get_ic_info(struct goodix_berlin_core *cd)
> +{
> + u8 afe_data[GOODIX_BERLIN_IC_INFO_MAX_LEN];
> + __le16 length_raw;
> + u16 length;
> + int error;
> +
> + error = regmap_raw_read(cd->regmap, GOODIX_BERLIN_IC_INFO_ADDR,
> + &length_raw, sizeof(length_raw));
> + if (error) {
> + dev_info(cd->dev, "failed get ic info length, %d\n", error);
> + return error;
> + }
> +
> + length = le16_to_cpu(length_raw);
> + if (length >= GOODIX_BERLIN_IC_INFO_MAX_LEN) {
> + dev_info(cd->dev, "invalid ic info length %d\n", length);
> + return -EINVAL;
> + }
> +
> + error = regmap_raw_read(cd->regmap, GOODIX_BERLIN_IC_INFO_ADDR,
> + afe_data, length);
> + if (error) {
> + dev_info(cd->dev, "failed get ic info data, %d\n", error);
> + return error;
> + }
> +
> + /* check whether the data is valid (ex. bus default values) */
> + if (goodix_berlin_is_dummy_data(cd, (const uint8_t *)afe_data, length)) {
> + dev_err(cd->dev, "fw info data invalid\n");
> + return -EINVAL;
> + }
> +
> + if (!goodix_berlin_checksum_valid((const uint8_t *)afe_data, length)) {
> + dev_info(cd->dev, "fw info checksum error\n");
> + return -EINVAL;
> + }
> +
> + error = goodix_berlin_convert_ic_info(cd, afe_data, length);
> + if (error) {
> + dev_err(cd->dev, "error converting ic info\n");
This function already prints an error message upon failure; consider dropping
one of the two.
> + return error;
> + }
> +
> + /* check some key info */
> + if (!cd->touch_data_addr) {
> + dev_err(cd->dev, "touch_data_addr is null\n");
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> +static void goodix_berlin_parse_finger(struct goodix_berlin_core *cd,
> + const void *buf, int touch_num)
> +{
> + const struct goodix_berlin_touch_data *touch_data = buf;
> + int i;
> +
> + /* Check for data validity */
> + for (i = 0; i < touch_num; i++) {
> + unsigned int id;
> +
> + id = FIELD_GET(GOODIX_BERLIN_TOUCH_ID_MASK, touch_data[i].id);
> +
> + if (id >= GOODIX_BERLIN_MAX_TOUCH) {
> + dev_warn(cd->dev, "invalid finger id %d\n", id);
> + return;
> + }
> + }
> +
> + /* Report finger touches */
> + for (i = 0; i < touch_num; i++) {
> + input_mt_slot(cd->input_dev,
> + FIELD_GET(GOODIX_BERLIN_TOUCH_ID_MASK,
> + touch_data[i].id));
> + input_mt_report_slot_state(cd->input_dev, MT_TOOL_FINGER, true);
> +
> + touchscreen_report_pos(cd->input_dev, &cd->props,
> + __le16_to_cpu(touch_data[i].x),
> + __le16_to_cpu(touch_data[i].y),
> + true);
> + input_report_abs(cd->input_dev, ABS_MT_TOUCH_MAJOR,
> + __le16_to_cpu(touch_data[i].w));
> + }
> +
> + input_mt_sync_frame(cd->input_dev);
> + input_sync(cd->input_dev);
> +}
> +
> +static void goodix_berlin_touch_handler(struct goodix_berlin_core *cd,
> + const void *pre_buf, u32 pre_buf_len)
> +{
> + static u8 buffer[GOODIX_BERLIN_IRQ_READ_LEN(GOODIX_BERLIN_MAX_TOUCH)];
> + u8 point_type = 0;
> + u8 touch_num = 0;
> + int error = 0;
Nit: some more unnecessary initializations in here. All of this cleaned up
quite nicely however.
> +
> + /* copy pre-data to buffer */
> + memcpy(buffer, pre_buf, pre_buf_len);
> +
> + touch_num = FIELD_GET(GOODIX_BERLIN_TOUCH_COUNT_MASK,
> + buffer[GOODIX_BERLIN_REQUEST_TYPE_OFFSET]);
> +
> + if (touch_num > GOODIX_BERLIN_MAX_TOUCH) {
> + dev_warn(cd->dev, "invalid touch num %d\n", touch_num);
> + return;
> + }
> +
> + if (touch_num) {
> + /* read more data if more than 2 touch events */
> + if (unlikely(touch_num > 2)) {
> + error = regmap_raw_read(cd->regmap,
> + cd->touch_data_addr + pre_buf_len,
> + &buffer[pre_buf_len],
> + (touch_num - 2) * GOODIX_BERLIN_BYTES_PER_POINT);
> + if (error) {
> + dev_err_ratelimited(cd->dev, "failed get touch data\n");
failed to get touch data: %d
(insert the word "to" and print the return value)
> + return;
> + }
> + }
> +
> + point_type = FIELD_GET(GOODIX_BERLIN_POINT_TYPE_MASK,
> + buffer[GOODIX_BERLIN_IRQ_EVENT_HEAD_LEN]);
> +
> + if (point_type == GOODIX_BERLIN_POINT_TYPE_STYLUS ||
> + point_type == GOODIX_BERLIN_POINT_TYPE_STYLUS_HOVER) {
> + dev_warn_once(cd->dev, "Stylus event type not handled\n");
> + return;
> + }
> +
> + if (!goodix_berlin_checksum_valid(&buffer[GOODIX_BERLIN_IRQ_EVENT_HEAD_LEN],
> + touch_num * GOODIX_BERLIN_BYTES_PER_POINT + 2)) {
> + dev_dbg(cd->dev, "touch data checksum error\n");
> + dev_dbg(cd->dev, "data: %*ph\n",
> + touch_num * GOODIX_BERLIN_BYTES_PER_POINT + 2,
> + &buffer[GOODIX_BERLIN_IRQ_EVENT_HEAD_LEN]);
This case seems worth of a dev_err; the two messages can be combined as well.
> + return;
> + }
> + }
> +
> + goodix_berlin_parse_finger(cd, &buffer[GOODIX_BERLIN_IRQ_EVENT_HEAD_LEN],
> + touch_num);
> +}
> +
> +static int goodix_berlin_request_handle_reset(struct goodix_berlin_core *cd)
> +{
> + gpiod_set_value(cd->reset_gpio, 1);
> + usleep_range(2000, 2100);
> + gpiod_set_value(cd->reset_gpio, 0);
> +
> + msleep(GOODIX_BERLIN_NORMAL_RESET_DELAY_MS);
> +
> + return 0;
> +}
> +
> +static irqreturn_t goodix_berlin_threadirq_func(int irq, void *data)
> +{
> + struct goodix_berlin_core *cd = data;
> + u8 buf[GOODIX_BERLIN_IRQ_READ_LEN(2)];
> + u8 event_status;
> + int error;
> +
> + /* First, read buffer with space for 2 touch events */
> + error = regmap_raw_read(cd->regmap, cd->touch_data_addr, buf,
> + GOODIX_BERLIN_IRQ_READ_LEN(2));
> + if (error) {
> + dev_err_ratelimited(cd->dev, "failed get event head data\n");
failed to get event head data: %d
(same comment as earlier)
> + return IRQ_HANDLED;
> + }
> +
> + if (buf[GOODIX_BERLIN_STATUS_OFFSET] == 0)
> + return IRQ_HANDLED;
> +
> + if (!goodix_berlin_checksum_valid(buf, GOODIX_BERLIN_IRQ_EVENT_HEAD_LEN)) {
> + dev_warn_ratelimited(cd->dev, "touch head checksum err : %*ph\n",
> + GOODIX_BERLIN_IRQ_EVENT_HEAD_LEN, buf);
> + return IRQ_HANDLED;
> + }
> +
> + event_status = buf[GOODIX_BERLIN_STATUS_OFFSET];
> +
> + if (event_status & GOODIX_BERLIN_TOUCH_EVENT)
> + goodix_berlin_touch_handler(cd, buf, GOODIX_BERLIN_IRQ_READ_LEN(2));
> +
> + if (event_status & GOODIX_BERLIN_REQUEST_EVENT) {
> + switch (buf[GOODIX_BERLIN_REQUEST_TYPE_OFFSET]) {
> + case GOODIX_BERLIN_REQUEST_CODE_RESET:
> + goodix_berlin_request_handle_reset(cd);
> + break;
> +
> + default:
> + dev_warn(cd->dev, "unsupported request code 0x%x\n",
> + buf[GOODIX_BERLIN_REQUEST_TYPE_OFFSET]);
> + }
> + }
> +
> + /* Clear up status field */
> + regmap_write(cd->regmap, cd->touch_data_addr, 0);
> +
> + return IRQ_HANDLED;
> +}
> +
> +static int goodix_berlin_input_dev_config(struct goodix_berlin_core *cd,
> + const struct input_id *id)
> +{
> + struct input_dev *input_dev;
> + int error;
> +
> + input_dev = devm_input_allocate_device(cd->dev);
> + if (!input_dev)
> + return -ENOMEM;
> +
> + cd->input_dev = input_dev;
> + input_set_drvdata(input_dev, cd);
> +
> + input_dev->name = "Goodix Berlin Capacitive TouchScreen";
> + input_dev->phys = "input/ts";
> +
> + input_dev->id = *id;
> +
> + input_set_abs_params(cd->input_dev, ABS_MT_POSITION_X, 0, SZ_64K - 1, 0, 0);
> + input_set_abs_params(cd->input_dev, ABS_MT_POSITION_Y, 0, SZ_64K - 1, 0, 0);
> + input_set_abs_params(cd->input_dev, ABS_MT_TOUCH_MAJOR, 0, 255, 0, 0);
> +
> + touchscreen_parse_properties(cd->input_dev, true, &cd->props);
> +
> + error = input_mt_init_slots(cd->input_dev, GOODIX_BERLIN_MAX_TOUCH,
> + INPUT_MT_DIRECT | INPUT_MT_DROP_UNUSED);
> + if (error)
> + return error;
> +
> + return input_register_device(cd->input_dev);
> +}
> +
> +static int goodix_berlin_pm_suspend(struct device *dev)
> +{
> + struct goodix_berlin_core *cd = dev_get_drvdata(dev);
> +
> + disable_irq(cd->irq);
> +
> + return goodix_berlin_power_on(cd, false);
> +}
> +
> +static int goodix_berlin_pm_resume(struct device *dev)
> +{
> + struct goodix_berlin_core *cd = dev_get_drvdata(dev);
> + int error;
> +
> + error = goodix_berlin_power_on(cd, true);
> + if (error)
> + return error;
> +
> + enable_irq(cd->irq);
> +
> + return 0;
> +}
> +
> +EXPORT_GPL_SIMPLE_DEV_PM_OPS(goodix_berlin_pm_ops,
> + goodix_berlin_pm_suspend,
> + goodix_berlin_pm_resume);
> +
> +static void goodix_berlin_power_off(void *data)
> +{
> + struct goodix_berlin_core *cd = data;
> +
> + goodix_berlin_power_on(cd, false);
> +}
> +
> +int goodix_berlin_probe(struct device *dev, int irq, const struct input_id *id,
> + struct regmap *regmap)
> +{
> + struct goodix_berlin_core *cd;
> + int error;
> +
> + if (irq <= 0) {
> + dev_err(dev, "Missing interrupt number\n");
> + return -EINVAL;
> + }
> +
> + cd = devm_kzalloc(dev, sizeof(*cd), GFP_KERNEL);
> + if (!cd)
> + return -ENOMEM;
> +
> + cd->dev = dev;
> + cd->regmap = regmap;
> + cd->irq = irq;
> +
> + cd->reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
> + if (IS_ERR(cd->reset_gpio))
> + return dev_err_probe(dev, PTR_ERR(cd->reset_gpio),
> + "Failed to request reset gpio\n");
> +
> + cd->avdd = devm_regulator_get(dev, "avdd");
> + if (IS_ERR(cd->avdd))
> + return dev_err_probe(dev, PTR_ERR(cd->avdd),
> + "Failed to request avdd regulator\n");
> +
> + cd->iovdd = devm_regulator_get(dev, "iovdd");
> + if (IS_ERR(cd->iovdd))
> + return dev_err_probe(dev, PTR_ERR(cd->iovdd),
> + "Failed to request iovdd regulator\n");
> +
> + error = goodix_berlin_power_on(cd, true);
> + if (error) {
> + dev_err(dev, "failed power on");
> + return error;
> + }
> +
> + error = devm_add_action_or_reset(dev, goodix_berlin_power_off, cd);
> + if (error)
> + return error;
> +
> + error = goodix_berlin_read_version(cd);
> + if (error) {
> + dev_err(dev, "failed to get version info");
> + return error;
> + }
> +
> + error = goodix_berlin_get_ic_info(cd);
> + if (error) {
> + dev_err(dev, "invalid ic info, abort");
> + return error;
> + }
> +
> + error = goodix_berlin_input_dev_config(cd, id);
> + if (error) {
> + dev_err(dev, "failed set input device");
> + return error;
> + }
> +
> + error = devm_request_threaded_irq(dev, irq, NULL,
> + goodix_berlin_threadirq_func,
> + IRQF_ONESHOT, "goodix-berlin", cd);
> + if (error) {
> + dev_err(dev, "request threaded irq failed: %d\n", error);
> + return error;
> + }
> +
> + dev_set_drvdata(dev, cd);
> +
> + dev_dbg(dev, "Goodix Berlin %s Touchscreen Controller", cd->fw_version.patch_pid);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(goodix_berlin_probe);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("Goodix Berlin Core Touchscreen driver");
> +MODULE_AUTHOR("Neil Armstrong <neil.armstrong@linaro.org>");
>
> --
> 2.34.1
>
Kind regards,
Jeff LaBundy
^ permalink raw reply
* [PATCH v4 24/24] Input: bu21029_ts - Use local 'client->dev' variable in probe()
From: Krzysztof Kozlowski @ 2023-06-25 16:28 UTC (permalink / raw)
To: Dmitry Torokhov, Hans de Goede, Linus Walleij, Bastien Nocera,
Sangwon Jee, Eugen Hristev, Mika Penttilä, linux-input,
linux-kernel, platform-driver-x86
Cc: Andi Shyti, Andy Shevchenko, Krzysztof Kozlowski, Andy Shevchenko
In-Reply-To: <20230625162817.100397-1-krzysztof.kozlowski@linaro.org>
'dev' is shorter and simpler than '&client->dev' and in few cases it
allows to skip line wrapping. Probe function uses '&client->dev' a lot,
so this improves readability slightly.
Suggested-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
---
Changes since v2:
1. New patch
---
drivers/input/touchscreen/bu21029_ts.c | 35 +++++++++++---------------
1 file changed, 15 insertions(+), 20 deletions(-)
diff --git a/drivers/input/touchscreen/bu21029_ts.c b/drivers/input/touchscreen/bu21029_ts.c
index 3d81ebe66b66..e1dfbd92ab64 100644
--- a/drivers/input/touchscreen/bu21029_ts.c
+++ b/drivers/input/touchscreen/bu21029_ts.c
@@ -333,6 +333,7 @@ static void bu21029_stop_chip(struct input_dev *dev)
static int bu21029_probe(struct i2c_client *client)
{
+ struct device *dev = &client->dev;
struct bu21029_ts_data *bu21029;
struct input_dev *in_dev;
int error;
@@ -341,37 +342,33 @@ static int bu21029_probe(struct i2c_client *client)
I2C_FUNC_SMBUS_WRITE_BYTE |
I2C_FUNC_SMBUS_WRITE_BYTE_DATA |
I2C_FUNC_SMBUS_READ_I2C_BLOCK)) {
- dev_err(&client->dev,
- "i2c functionality support is not sufficient\n");
+ dev_err(dev, "i2c functionality support is not sufficient\n");
return -EIO;
}
- bu21029 = devm_kzalloc(&client->dev, sizeof(*bu21029), GFP_KERNEL);
+ bu21029 = devm_kzalloc(dev, sizeof(*bu21029), GFP_KERNEL);
if (!bu21029)
return -ENOMEM;
- error = device_property_read_u32(&client->dev, "rohm,x-plate-ohms",
- &bu21029->x_plate_ohms);
+ error = device_property_read_u32(dev, "rohm,x-plate-ohms", &bu21029->x_plate_ohms);
if (error) {
- dev_err(&client->dev,
- "invalid 'x-plate-ohms' supplied: %d\n", error);
+ dev_err(dev, "invalid 'x-plate-ohms' supplied: %d\n", error);
return error;
}
- bu21029->vdd = devm_regulator_get(&client->dev, "vdd");
+ bu21029->vdd = devm_regulator_get(dev, "vdd");
if (IS_ERR(bu21029->vdd))
- return dev_err_probe(&client->dev, PTR_ERR(bu21029->vdd),
+ return dev_err_probe(dev, PTR_ERR(bu21029->vdd),
"failed to acquire 'vdd' supply\n");
- bu21029->reset_gpios = devm_gpiod_get_optional(&client->dev,
- "reset", GPIOD_OUT_HIGH);
+ bu21029->reset_gpios = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
if (IS_ERR(bu21029->reset_gpios))
- return dev_err_probe(&client->dev, PTR_ERR(bu21029->reset_gpios),
+ return dev_err_probe(dev, PTR_ERR(bu21029->reset_gpios),
"failed to acquire 'reset' gpio\n");
- in_dev = devm_input_allocate_device(&client->dev);
+ in_dev = devm_input_allocate_device(dev);
if (!in_dev) {
- dev_err(&client->dev, "unable to allocate input device\n");
+ dev_err(dev, "unable to allocate input device\n");
return -ENOMEM;
}
@@ -392,20 +389,18 @@ static int bu21029_probe(struct i2c_client *client)
input_set_drvdata(in_dev, bu21029);
- error = devm_request_threaded_irq(&client->dev, client->irq,
- NULL, bu21029_touch_soft_irq,
+ error = devm_request_threaded_irq(dev, client->irq, NULL,
+ bu21029_touch_soft_irq,
IRQF_ONESHOT | IRQF_NO_AUTOEN,
DRIVER_NAME, bu21029);
if (error) {
- dev_err(&client->dev,
- "unable to request touch irq: %d\n", error);
+ dev_err(dev, "unable to request touch irq: %d\n", error);
return error;
}
error = input_register_device(in_dev);
if (error) {
- dev_err(&client->dev,
- "unable to register input device: %d\n", error);
+ dev_err(dev, "unable to register input device: %d\n", error);
return error;
}
--
2.34.1
^ permalink raw reply related
* [PATCH v4 21/24] Input: sx8643 - Simplify with dev_err_probe()
From: Krzysztof Kozlowski @ 2023-06-25 16:28 UTC (permalink / raw)
To: Dmitry Torokhov, Hans de Goede, Linus Walleij, Bastien Nocera,
Sangwon Jee, Eugen Hristev, Mika Penttilä, linux-input,
linux-kernel, platform-driver-x86
Cc: Andi Shyti, Andy Shevchenko, Krzysztof Kozlowski, Andy Shevchenko
In-Reply-To: <20230625162817.100397-1-krzysztof.kozlowski@linaro.org>
Common pattern of handling deferred probe can be simplified with
dev_err_probe(). Less code and also it prints the error value.
Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
---
drivers/input/touchscreen/sx8654.c | 10 +++-------
1 file changed, 3 insertions(+), 7 deletions(-)
diff --git a/drivers/input/touchscreen/sx8654.c b/drivers/input/touchscreen/sx8654.c
index 0293c493bc79..f5c5881cef6b 100644
--- a/drivers/input/touchscreen/sx8654.c
+++ b/drivers/input/touchscreen/sx8654.c
@@ -323,13 +323,9 @@ static int sx8654_probe(struct i2c_client *client)
sx8654->gpio_reset = devm_gpiod_get_optional(&client->dev, "reset",
GPIOD_OUT_HIGH);
- if (IS_ERR(sx8654->gpio_reset)) {
- error = PTR_ERR(sx8654->gpio_reset);
- if (error != -EPROBE_DEFER)
- dev_err(&client->dev, "unable to get reset-gpio: %d\n",
- error);
- return error;
- }
+ if (IS_ERR(sx8654->gpio_reset))
+ return dev_err_probe(&client->dev, PTR_ERR(sx8654->gpio_reset),
+ "unable to get reset-gpio\n");
dev_dbg(&client->dev, "got GPIO reset pin\n");
sx8654->data = device_get_match_data(&client->dev);
--
2.34.1
^ permalink raw reply related
* [PATCH v4 17/24] Input: resistive-adc-touch - Simplify with dev_err_probe()
From: Krzysztof Kozlowski @ 2023-06-25 16:28 UTC (permalink / raw)
To: Dmitry Torokhov, Hans de Goede, Linus Walleij, Bastien Nocera,
Sangwon Jee, Eugen Hristev, Mika Penttilä, linux-input,
linux-kernel, platform-driver-x86
Cc: Andi Shyti, Andy Shevchenko, Krzysztof Kozlowski, Andy Shevchenko
In-Reply-To: <20230625162817.100397-1-krzysztof.kozlowski@linaro.org>
Common pattern of handling deferred probe can be simplified with
dev_err_probe(). Less code and also it prints the error value.
Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
---
drivers/input/touchscreen/resistive-adc-touch.c | 8 ++------
1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/drivers/input/touchscreen/resistive-adc-touch.c b/drivers/input/touchscreen/resistive-adc-touch.c
index 6f754a8d30b1..7e761ec73273 100644
--- a/drivers/input/touchscreen/resistive-adc-touch.c
+++ b/drivers/input/touchscreen/resistive-adc-touch.c
@@ -210,12 +210,8 @@ static int grts_probe(struct platform_device *pdev)
/* get the channels from IIO device */
st->iio_chans = devm_iio_channel_get_all(dev);
- if (IS_ERR(st->iio_chans)) {
- error = PTR_ERR(st->iio_chans);
- if (error != -EPROBE_DEFER)
- dev_err(dev, "can't get iio channels.\n");
- return error;
- }
+ if (IS_ERR(st->iio_chans))
+ return dev_err_probe(dev, PTR_ERR(st->iio_chans), "can't get iio channels\n");
if (!device_property_present(dev, "io-channel-names"))
return -ENODEV;
--
2.34.1
^ permalink raw reply related
* [PATCH v4 23/24] Input: bu21013_ts - Use local 'client->dev' variable in probe()
From: Krzysztof Kozlowski @ 2023-06-25 16:28 UTC (permalink / raw)
To: Dmitry Torokhov, Hans de Goede, Linus Walleij, Bastien Nocera,
Sangwon Jee, Eugen Hristev, Mika Penttilä, linux-input,
linux-kernel, platform-driver-x86
Cc: Andi Shyti, Andy Shevchenko, Krzysztof Kozlowski, Andy Shevchenko
In-Reply-To: <20230625162817.100397-1-krzysztof.kozlowski@linaro.org>
'dev' is shorter and simpler than '&client->dev' and in few cases it
allows to skip line wrapping. Probe function uses '&client->dev' a lot,
so this improves readability slightly.
Suggested-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
---
Changes since v3:
1. Correct suggested-by tag.
Changes since v2:
1. New patch
---
drivers/input/touchscreen/bu21013_ts.c | 61 ++++++++++++--------------
1 file changed, 28 insertions(+), 33 deletions(-)
diff --git a/drivers/input/touchscreen/bu21013_ts.c b/drivers/input/touchscreen/bu21013_ts.c
index f811677a59f7..652439a79e21 100644
--- a/drivers/input/touchscreen/bu21013_ts.c
+++ b/drivers/input/touchscreen/bu21013_ts.c
@@ -410,31 +410,32 @@ static int bu21013_probe(struct i2c_client *client)
struct input_dev *in_dev;
struct input_absinfo *info;
u32 max_x = 0, max_y = 0;
+ struct device *dev = &client->dev;
int error;
if (!i2c_check_functionality(client->adapter,
I2C_FUNC_SMBUS_BYTE_DATA)) {
- dev_err(&client->dev, "i2c smbus byte data not supported\n");
+ dev_err(dev, "i2c smbus byte data not supported\n");
return -EIO;
}
if (!client->irq) {
- dev_err(&client->dev, "No IRQ set up\n");
+ dev_err(dev, "No IRQ set up\n");
return -EINVAL;
}
- ts = devm_kzalloc(&client->dev, sizeof(*ts), GFP_KERNEL);
+ ts = devm_kzalloc(dev, sizeof(*ts), GFP_KERNEL);
if (!ts)
return -ENOMEM;
ts->client = client;
- ts->x_flip = device_property_read_bool(&client->dev, "rohm,flip-x");
- ts->y_flip = device_property_read_bool(&client->dev, "rohm,flip-y");
+ ts->x_flip = device_property_read_bool(dev, "rohm,flip-x");
+ ts->y_flip = device_property_read_bool(dev, "rohm,flip-y");
- in_dev = devm_input_allocate_device(&client->dev);
+ in_dev = devm_input_allocate_device(dev);
if (!in_dev) {
- dev_err(&client->dev, "device memory alloc failed\n");
+ dev_err(dev, "device memory alloc failed\n");
return -ENOMEM;
}
ts->in_dev = in_dev;
@@ -444,8 +445,8 @@ static int bu21013_probe(struct i2c_client *client)
in_dev->name = DRIVER_TP;
in_dev->id.bustype = BUS_I2C;
- device_property_read_u32(&client->dev, "rohm,touch-max-x", &max_x);
- device_property_read_u32(&client->dev, "rohm,touch-max-y", &max_y);
+ device_property_read_u32(dev, "rohm,touch-max-x", &max_x);
+ device_property_read_u32(dev, "rohm,touch-max-y", &max_y);
input_set_abs_params(in_dev, ABS_MT_POSITION_X, 0, max_x, 0, 0);
input_set_abs_params(in_dev, ABS_MT_POSITION_Y, 0, max_y, 0, 0);
@@ -454,14 +455,14 @@ static int bu21013_probe(struct i2c_client *client)
/* Adjust for the legacy "flip" properties, if present */
if (!ts->props.invert_x &&
- device_property_read_bool(&client->dev, "rohm,flip-x")) {
+ device_property_read_bool(dev, "rohm,flip-x")) {
info = &in_dev->absinfo[ABS_MT_POSITION_X];
info->maximum -= info->minimum;
info->minimum = 0;
}
if (!ts->props.invert_y &&
- device_property_read_bool(&client->dev, "rohm,flip-y")) {
+ device_property_read_bool(dev, "rohm,flip-y")) {
info = &in_dev->absinfo[ABS_MT_POSITION_Y];
info->maximum -= info->minimum;
info->minimum = 0;
@@ -471,50 +472,46 @@ static int bu21013_probe(struct i2c_client *client)
INPUT_MT_DIRECT | INPUT_MT_TRACK |
INPUT_MT_DROP_UNUSED);
if (error) {
- dev_err(&client->dev, "failed to initialize MT slots");
+ dev_err(dev, "failed to initialize MT slots");
return error;
}
- ts->regulator = devm_regulator_get(&client->dev, "avdd");
+ ts->regulator = devm_regulator_get(dev, "avdd");
if (IS_ERR(ts->regulator)) {
- dev_err(&client->dev, "regulator_get failed\n");
+ dev_err(dev, "regulator_get failed\n");
return PTR_ERR(ts->regulator);
}
error = regulator_enable(ts->regulator);
if (error) {
- dev_err(&client->dev, "regulator enable failed\n");
+ dev_err(dev, "regulator enable failed\n");
return error;
}
- error = devm_add_action_or_reset(&client->dev, bu21013_power_off, ts);
+ error = devm_add_action_or_reset(dev, bu21013_power_off, ts);
if (error) {
- dev_err(&client->dev, "failed to install power off handler\n");
+ dev_err(dev, "failed to install power off handler\n");
return error;
}
/* Named "CS" on the chip, DT binding is "reset" */
- ts->cs_gpiod = devm_gpiod_get(&client->dev, "reset", GPIOD_OUT_HIGH);
+ ts->cs_gpiod = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
if (IS_ERR(ts->cs_gpiod))
- return dev_err_probe(&client->dev, PTR_ERR(ts->cs_gpiod),
- "failed to get CS GPIO\n");
+ return dev_err_probe(dev, PTR_ERR(ts->cs_gpiod), "failed to get CS GPIO\n");
gpiod_set_consumer_name(ts->cs_gpiod, "BU21013 CS");
- error = devm_add_action_or_reset(&client->dev,
- bu21013_disable_chip, ts);
+ error = devm_add_action_or_reset(dev, bu21013_disable_chip, ts);
if (error) {
- dev_err(&client->dev,
- "failed to install chip disable handler\n");
+ dev_err(dev, "failed to install chip disable handler\n");
return error;
}
/* Named "INT" on the chip, DT binding is "touch" */
- ts->int_gpiod = devm_gpiod_get_optional(&client->dev,
- "touch", GPIOD_IN);
+ ts->int_gpiod = devm_gpiod_get_optional(dev, "touch", GPIOD_IN);
error = PTR_ERR_OR_ZERO(ts->int_gpiod);
if (error)
- return dev_err_probe(&client->dev, error, "failed to get INT GPIO\n");
+ return dev_err_probe(dev, error, "failed to get INT GPIO\n");
if (ts->int_gpiod)
gpiod_set_consumer_name(ts->int_gpiod, "BU21013 INT");
@@ -522,22 +519,20 @@ static int bu21013_probe(struct i2c_client *client)
/* configure the touch panel controller */
error = bu21013_init_chip(ts);
if (error) {
- dev_err(&client->dev, "error in bu21013 config\n");
+ dev_err(dev, "error in bu21013 config\n");
return error;
}
- error = devm_request_threaded_irq(&client->dev, client->irq,
- NULL, bu21013_gpio_irq,
+ error = devm_request_threaded_irq(dev, client->irq, NULL, bu21013_gpio_irq,
IRQF_ONESHOT, DRIVER_TP, ts);
if (error) {
- dev_err(&client->dev, "request irq %d failed\n",
- client->irq);
+ dev_err(dev, "request irq %d failed\n", client->irq);
return error;
}
error = input_register_device(in_dev);
if (error) {
- dev_err(&client->dev, "failed to register input device\n");
+ dev_err(dev, "failed to register input device\n");
return error;
}
--
2.34.1
^ permalink raw reply related
* [PATCH v4 20/24] Input: surface3_spi - Simplify with dev_err_probe()
From: Krzysztof Kozlowski @ 2023-06-25 16:28 UTC (permalink / raw)
To: Dmitry Torokhov, Hans de Goede, Linus Walleij, Bastien Nocera,
Sangwon Jee, Eugen Hristev, Mika Penttilä, linux-input,
linux-kernel, platform-driver-x86
Cc: Andi Shyti, Andy Shevchenko, Krzysztof Kozlowski, Andy Shevchenko
In-Reply-To: <20230625162817.100397-1-krzysztof.kozlowski@linaro.org>
Common pattern of handling deferred probe can be simplified with
dev_err_probe(). Less code and also it prints the error value.
Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
---
drivers/input/touchscreen/surface3_spi.c | 13 +++----------
1 file changed, 3 insertions(+), 10 deletions(-)
diff --git a/drivers/input/touchscreen/surface3_spi.c b/drivers/input/touchscreen/surface3_spi.c
index 31d140248f2e..7efbcd0fde4f 100644
--- a/drivers/input/touchscreen/surface3_spi.c
+++ b/drivers/input/touchscreen/surface3_spi.c
@@ -221,7 +221,6 @@ static void surface3_spi_power(struct surface3_ts_data *data, bool on)
*/
static int surface3_spi_get_gpio_config(struct surface3_ts_data *data)
{
- int error;
struct device *dev;
struct gpio_desc *gpiod;
int i;
@@ -231,15 +230,9 @@ static int surface3_spi_get_gpio_config(struct surface3_ts_data *data)
/* Get the reset lines GPIO pin number */
for (i = 0; i < 2; i++) {
gpiod = devm_gpiod_get_index(dev, NULL, i, GPIOD_OUT_LOW);
- if (IS_ERR(gpiod)) {
- error = PTR_ERR(gpiod);
- if (error != -EPROBE_DEFER)
- dev_err(dev,
- "Failed to get power GPIO %d: %d\n",
- i,
- error);
- return error;
- }
+ if (IS_ERR(gpiod))
+ return dev_err_probe(dev, PTR_ERR(gpiod),
+ "Failed to get power GPIO %d\n", i);
data->gpiod_rst[i] = gpiod;
}
--
2.34.1
^ permalink raw reply related
* [PATCH v4 22/24] Input: bcm-keypad - Simplify with dev_err_probe()
From: Krzysztof Kozlowski @ 2023-06-25 16:28 UTC (permalink / raw)
To: Dmitry Torokhov, Hans de Goede, Linus Walleij, Bastien Nocera,
Sangwon Jee, Eugen Hristev, Mika Penttilä, linux-input,
linux-kernel, platform-driver-x86
Cc: Andi Shyti, Andy Shevchenko, Krzysztof Kozlowski, Andy Shevchenko
In-Reply-To: <20230625162817.100397-1-krzysztof.kozlowski@linaro.org>
Common pattern of handling deferred probe can be simplified with
dev_err_probe() and devm_clk_get_optional(). Less code and the error
value gets printed.
Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
---
Changes since v1:
1. Use also devm_clk_get_optional()
---
drivers/input/keyboard/bcm-keypad.c | 14 ++++----------
1 file changed, 4 insertions(+), 10 deletions(-)
diff --git a/drivers/input/keyboard/bcm-keypad.c b/drivers/input/keyboard/bcm-keypad.c
index 56a919ec23b5..05b09066df84 100644
--- a/drivers/input/keyboard/bcm-keypad.c
+++ b/drivers/input/keyboard/bcm-keypad.c
@@ -365,17 +365,11 @@ static int bcm_kp_probe(struct platform_device *pdev)
return PTR_ERR(kp->base);
/* Enable clock */
- kp->clk = devm_clk_get(&pdev->dev, "peri_clk");
+ kp->clk = devm_clk_get_optional(&pdev->dev, "peri_clk");
if (IS_ERR(kp->clk)) {
- error = PTR_ERR(kp->clk);
- if (error != -ENOENT) {
- if (error != -EPROBE_DEFER)
- dev_err(&pdev->dev, "Failed to get clock\n");
- return error;
- }
- dev_dbg(&pdev->dev,
- "No clock specified. Assuming it's enabled\n");
- kp->clk = NULL;
+ return dev_err_probe(&pdev->dev, error, "Failed to get clock\n");
+ } else if (!kp->clk) {
+ dev_dbg(&pdev->dev, "No clock specified. Assuming it's enabled\n");
} else {
unsigned int desired_rate;
long actual_rate;
--
2.34.1
^ permalink raw reply related
* [PATCH v4 19/24] Input: sis_i2c - Simplify with dev_err_probe()
From: Krzysztof Kozlowski @ 2023-06-25 16:28 UTC (permalink / raw)
To: Dmitry Torokhov, Hans de Goede, Linus Walleij, Bastien Nocera,
Sangwon Jee, Eugen Hristev, Mika Penttilä, linux-input,
linux-kernel, platform-driver-x86
Cc: Andi Shyti, Andy Shevchenko, Krzysztof Kozlowski, Andy Shevchenko
In-Reply-To: <20230625162817.100397-1-krzysztof.kozlowski@linaro.org>
Common pattern of handling deferred probe can be simplified with
dev_err_probe(). Less code and also it prints the error value.
Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
---
drivers/input/touchscreen/sis_i2c.c | 20 ++++++--------------
1 file changed, 6 insertions(+), 14 deletions(-)
diff --git a/drivers/input/touchscreen/sis_i2c.c b/drivers/input/touchscreen/sis_i2c.c
index 426564d0fc39..ed56cb546f39 100644
--- a/drivers/input/touchscreen/sis_i2c.c
+++ b/drivers/input/touchscreen/sis_i2c.c
@@ -310,23 +310,15 @@ static int sis_ts_probe(struct i2c_client *client)
ts->attn_gpio = devm_gpiod_get_optional(&client->dev,
"attn", GPIOD_IN);
- if (IS_ERR(ts->attn_gpio)) {
- error = PTR_ERR(ts->attn_gpio);
- if (error != -EPROBE_DEFER)
- dev_err(&client->dev,
- "Failed to get attention GPIO: %d\n", error);
- return error;
- }
+ if (IS_ERR(ts->attn_gpio))
+ return dev_err_probe(&client->dev, PTR_ERR(ts->attn_gpio),
+ "Failed to get attention GPIO\n");
ts->reset_gpio = devm_gpiod_get_optional(&client->dev,
"reset", GPIOD_OUT_LOW);
- if (IS_ERR(ts->reset_gpio)) {
- error = PTR_ERR(ts->reset_gpio);
- if (error != -EPROBE_DEFER)
- dev_err(&client->dev,
- "Failed to get reset GPIO: %d\n", error);
- return error;
- }
+ if (IS_ERR(ts->reset_gpio))
+ return dev_err_probe(&client->dev, PTR_ERR(ts->reset_gpio),
+ "Failed to get reset GPIO\n");
sis_ts_reset(ts);
--
2.34.1
^ permalink raw reply related
* [PATCH v4 18/24] Input: silead - Simplify with dev_err_probe()
From: Krzysztof Kozlowski @ 2023-06-25 16:28 UTC (permalink / raw)
To: Dmitry Torokhov, Hans de Goede, Linus Walleij, Bastien Nocera,
Sangwon Jee, Eugen Hristev, Mika Penttilä, linux-input,
linux-kernel, platform-driver-x86
Cc: Andi Shyti, Andy Shevchenko, Krzysztof Kozlowski, Andy Shevchenko
In-Reply-To: <20230625162817.100397-1-krzysztof.kozlowski@linaro.org>
Common pattern of handling deferred probe can be simplified with
dev_err_probe(). Less code and also it prints the error value.
Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
---
drivers/input/touchscreen/silead.c | 8 +++-----
1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/drivers/input/touchscreen/silead.c b/drivers/input/touchscreen/silead.c
index 9e28f962e059..62f562ad5026 100644
--- a/drivers/input/touchscreen/silead.c
+++ b/drivers/input/touchscreen/silead.c
@@ -706,11 +706,9 @@ static int silead_ts_probe(struct i2c_client *client)
/* Power GPIO pin */
data->gpio_power = devm_gpiod_get_optional(dev, "power", GPIOD_OUT_LOW);
- if (IS_ERR(data->gpio_power)) {
- if (PTR_ERR(data->gpio_power) != -EPROBE_DEFER)
- dev_err(dev, "Shutdown GPIO request failed\n");
- return PTR_ERR(data->gpio_power);
- }
+ if (IS_ERR(data->gpio_power))
+ return dev_err_probe(dev, PTR_ERR(data->gpio_power),
+ "Shutdown GPIO request failed\n");
error = silead_ts_setup(client);
if (error)
--
2.34.1
^ permalink raw reply related
* [PATCH v4 15/24] Input: pixcir_i2c_ts - Simplify with dev_err_probe()
From: Krzysztof Kozlowski @ 2023-06-25 16:28 UTC (permalink / raw)
To: Dmitry Torokhov, Hans de Goede, Linus Walleij, Bastien Nocera,
Sangwon Jee, Eugen Hristev, Mika Penttilä, linux-input,
linux-kernel, platform-driver-x86
Cc: Andi Shyti, Andy Shevchenko, Krzysztof Kozlowski, Andy Shevchenko
In-Reply-To: <20230625162817.100397-1-krzysztof.kozlowski@linaro.org>
Common pattern of handling deferred probe can be simplified with
dev_err_probe(). Less code and also it prints the error value.
Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
---
drivers/input/touchscreen/pixcir_i2c_ts.c | 38 +++++++----------------
1 file changed, 12 insertions(+), 26 deletions(-)
diff --git a/drivers/input/touchscreen/pixcir_i2c_ts.c b/drivers/input/touchscreen/pixcir_i2c_ts.c
index 554e179c2e48..0b4576091dac 100644
--- a/drivers/input/touchscreen/pixcir_i2c_ts.c
+++ b/drivers/input/touchscreen/pixcir_i2c_ts.c
@@ -515,41 +515,27 @@ static int pixcir_i2c_ts_probe(struct i2c_client *client)
input_set_drvdata(input, tsdata);
tsdata->gpio_attb = devm_gpiod_get(dev, "attb", GPIOD_IN);
- if (IS_ERR(tsdata->gpio_attb)) {
- error = PTR_ERR(tsdata->gpio_attb);
- if (error != -EPROBE_DEFER)
- dev_err(dev, "Failed to request ATTB gpio: %d\n",
- error);
- return error;
- }
+ if (IS_ERR(tsdata->gpio_attb))
+ return dev_err_probe(dev, PTR_ERR(tsdata->gpio_attb),
+ "Failed to request ATTB gpio\n");
tsdata->gpio_reset = devm_gpiod_get_optional(dev, "reset",
GPIOD_OUT_LOW);
- if (IS_ERR(tsdata->gpio_reset)) {
- error = PTR_ERR(tsdata->gpio_reset);
- if (error != -EPROBE_DEFER)
- dev_err(dev, "Failed to request RESET gpio: %d\n",
- error);
- return error;
- }
+ if (IS_ERR(tsdata->gpio_reset))
+ return dev_err_probe(dev, PTR_ERR(tsdata->gpio_reset),
+ "Failed to request RESET gpio\n");
tsdata->gpio_wake = devm_gpiod_get_optional(dev, "wake",
GPIOD_OUT_HIGH);
- if (IS_ERR(tsdata->gpio_wake)) {
- error = PTR_ERR(tsdata->gpio_wake);
- if (error != -EPROBE_DEFER)
- dev_err(dev, "Failed to get wake gpio: %d\n", error);
- return error;
- }
+ if (IS_ERR(tsdata->gpio_wake))
+ return dev_err_probe(dev, PTR_ERR(tsdata->gpio_wake),
+ "Failed to get wake gpio\n");
tsdata->gpio_enable = devm_gpiod_get_optional(dev, "enable",
GPIOD_OUT_HIGH);
- if (IS_ERR(tsdata->gpio_enable)) {
- error = PTR_ERR(tsdata->gpio_enable);
- if (error != -EPROBE_DEFER)
- dev_err(dev, "Failed to get enable gpio: %d\n", error);
- return error;
- }
+ if (IS_ERR(tsdata->gpio_enable))
+ return dev_err_probe(dev, PTR_ERR(tsdata->gpio_enable),
+ "Failed to get enable gpio\n");
if (tsdata->gpio_enable)
msleep(100);
--
2.34.1
^ permalink raw reply related
* [PATCH v4 16/24] Input: raydium_i2c_ts - Simplify with dev_err_probe()
From: Krzysztof Kozlowski @ 2023-06-25 16:28 UTC (permalink / raw)
To: Dmitry Torokhov, Hans de Goede, Linus Walleij, Bastien Nocera,
Sangwon Jee, Eugen Hristev, Mika Penttilä, linux-input,
linux-kernel, platform-driver-x86
Cc: Andi Shyti, Andy Shevchenko, Krzysztof Kozlowski, Andy Shevchenko
In-Reply-To: <20230625162817.100397-1-krzysztof.kozlowski@linaro.org>
Common pattern of handling deferred probe can be simplified with
dev_err_probe(). Less code and also it prints the error value.
Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
---
drivers/input/touchscreen/raydium_i2c_ts.c | 30 +++++++---------------
1 file changed, 9 insertions(+), 21 deletions(-)
diff --git a/drivers/input/touchscreen/raydium_i2c_ts.c b/drivers/input/touchscreen/raydium_i2c_ts.c
index 76e7d62d5870..78dd3059d585 100644
--- a/drivers/input/touchscreen/raydium_i2c_ts.c
+++ b/drivers/input/touchscreen/raydium_i2c_ts.c
@@ -1087,32 +1087,20 @@ static int raydium_i2c_probe(struct i2c_client *client)
i2c_set_clientdata(client, ts);
ts->avdd = devm_regulator_get(&client->dev, "avdd");
- if (IS_ERR(ts->avdd)) {
- error = PTR_ERR(ts->avdd);
- if (error != -EPROBE_DEFER)
- dev_err(&client->dev,
- "Failed to get 'avdd' regulator: %d\n", error);
- return error;
- }
+ if (IS_ERR(ts->avdd))
+ return dev_err_probe(&client->dev, PTR_ERR(ts->avdd),
+ "Failed to get 'avdd' regulator\n");
ts->vccio = devm_regulator_get(&client->dev, "vccio");
- if (IS_ERR(ts->vccio)) {
- error = PTR_ERR(ts->vccio);
- if (error != -EPROBE_DEFER)
- dev_err(&client->dev,
- "Failed to get 'vccio' regulator: %d\n", error);
- return error;
- }
+ if (IS_ERR(ts->vccio))
+ return dev_err_probe(&client->dev, PTR_ERR(ts->vccio),
+ "Failed to get 'vccio' regulator\n");
ts->reset_gpio = devm_gpiod_get_optional(&client->dev, "reset",
GPIOD_OUT_LOW);
- if (IS_ERR(ts->reset_gpio)) {
- error = PTR_ERR(ts->reset_gpio);
- if (error != -EPROBE_DEFER)
- dev_err(&client->dev,
- "failed to get reset gpio: %d\n", error);
- return error;
- }
+ if (IS_ERR(ts->reset_gpio))
+ return dev_err_probe(&client->dev, PTR_ERR(ts->reset_gpio),
+ "Failed to get reset gpio\n");
error = raydium_i2c_power_on(ts);
if (error)
--
2.34.1
^ permalink raw reply related
* [PATCH v4 14/24] Input: melfas_mip4 - Simplify with dev_err_probe()
From: Krzysztof Kozlowski @ 2023-06-25 16:28 UTC (permalink / raw)
To: Dmitry Torokhov, Hans de Goede, Linus Walleij, Bastien Nocera,
Sangwon Jee, Eugen Hristev, Mika Penttilä, linux-input,
linux-kernel, platform-driver-x86
Cc: Andi Shyti, Andy Shevchenko, Krzysztof Kozlowski, Andy Shevchenko
In-Reply-To: <20230625162817.100397-1-krzysztof.kozlowski@linaro.org>
Common pattern of handling deferred probe can be simplified with
dev_err_probe(). Less code and also it prints the error value.
Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
---
drivers/input/touchscreen/melfas_mip4.c | 9 ++-------
1 file changed, 2 insertions(+), 7 deletions(-)
diff --git a/drivers/input/touchscreen/melfas_mip4.c b/drivers/input/touchscreen/melfas_mip4.c
index 32896e5085bd..2ac4483fbc25 100644
--- a/drivers/input/touchscreen/melfas_mip4.c
+++ b/drivers/input/touchscreen/melfas_mip4.c
@@ -1451,13 +1451,8 @@ static int mip4_probe(struct i2c_client *client)
ts->gpio_ce = devm_gpiod_get_optional(&client->dev,
"ce", GPIOD_OUT_LOW);
- if (IS_ERR(ts->gpio_ce)) {
- error = PTR_ERR(ts->gpio_ce);
- if (error != -EPROBE_DEFER)
- dev_err(&client->dev,
- "Failed to get gpio: %d\n", error);
- return error;
- }
+ if (IS_ERR(ts->gpio_ce))
+ return dev_err_probe(&client->dev, PTR_ERR(ts->gpio_ce), "Failed to get gpio\n");
error = mip4_power_on(ts);
if (error)
--
2.34.1
^ permalink raw reply related
* [PATCH v4 13/24] Input: goodix - Simplify with dev_err_probe()
From: Krzysztof Kozlowski @ 2023-06-25 16:28 UTC (permalink / raw)
To: Dmitry Torokhov, Hans de Goede, Linus Walleij, Bastien Nocera,
Sangwon Jee, Eugen Hristev, Mika Penttilä, linux-input,
linux-kernel, platform-driver-x86
Cc: Andi Shyti, Andy Shevchenko, Krzysztof Kozlowski, Andy Shevchenko
In-Reply-To: <20230625162817.100397-1-krzysztof.kozlowski@linaro.org>
Common pattern of handling deferred probe can be simplified with
dev_err_probe(). Less code and also it prints the error value.
Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
---
drivers/input/touchscreen/goodix.c | 40 ++++++++----------------------
1 file changed, 11 insertions(+), 29 deletions(-)
diff --git a/drivers/input/touchscreen/goodix.c b/drivers/input/touchscreen/goodix.c
index f5aa240739f9..85d4249f1065 100644
--- a/drivers/input/touchscreen/goodix.c
+++ b/drivers/input/touchscreen/goodix.c
@@ -935,7 +935,6 @@ static int goodix_add_acpi_gpio_mappings(struct goodix_ts_data *ts)
*/
static int goodix_get_gpio_config(struct goodix_ts_data *ts)
{
- int error;
struct device *dev;
struct gpio_desc *gpiod;
bool added_acpi_mappings = false;
@@ -951,33 +950,20 @@ static int goodix_get_gpio_config(struct goodix_ts_data *ts)
ts->gpiod_rst_flags = GPIOD_IN;
ts->avdd28 = devm_regulator_get(dev, "AVDD28");
- if (IS_ERR(ts->avdd28)) {
- error = PTR_ERR(ts->avdd28);
- if (error != -EPROBE_DEFER)
- dev_err(dev,
- "Failed to get AVDD28 regulator: %d\n", error);
- return error;
- }
+ if (IS_ERR(ts->avdd28))
+ return dev_err_probe(dev, PTR_ERR(ts->avdd28), "Failed to get AVDD28 regulator\n");
ts->vddio = devm_regulator_get(dev, "VDDIO");
- if (IS_ERR(ts->vddio)) {
- error = PTR_ERR(ts->vddio);
- if (error != -EPROBE_DEFER)
- dev_err(dev,
- "Failed to get VDDIO regulator: %d\n", error);
- return error;
- }
+ if (IS_ERR(ts->vddio))
+ return dev_err_probe(dev, PTR_ERR(ts->vddio), "Failed to get VDDIO regulator\n");
retry_get_irq_gpio:
/* Get the interrupt GPIO pin number */
gpiod = devm_gpiod_get_optional(dev, GOODIX_GPIO_INT_NAME, GPIOD_IN);
- if (IS_ERR(gpiod)) {
- error = PTR_ERR(gpiod);
- if (error != -EPROBE_DEFER)
- dev_err(dev, "Failed to get %s GPIO: %d\n",
- GOODIX_GPIO_INT_NAME, error);
- return error;
- }
+ if (IS_ERR(gpiod))
+ return dev_err_probe(dev, PTR_ERR(gpiod), "Failed to get %s GPIO\n",
+ GOODIX_GPIO_INT_NAME);
+
if (!gpiod && has_acpi_companion(dev) && !added_acpi_mappings) {
added_acpi_mappings = true;
if (goodix_add_acpi_gpio_mappings(ts) == 0)
@@ -988,13 +974,9 @@ static int goodix_get_gpio_config(struct goodix_ts_data *ts)
/* Get the reset line GPIO pin number */
gpiod = devm_gpiod_get_optional(dev, GOODIX_GPIO_RST_NAME, ts->gpiod_rst_flags);
- if (IS_ERR(gpiod)) {
- error = PTR_ERR(gpiod);
- if (error != -EPROBE_DEFER)
- dev_err(dev, "Failed to get %s GPIO: %d\n",
- GOODIX_GPIO_RST_NAME, error);
- return error;
- }
+ if (IS_ERR(gpiod))
+ return dev_err_probe(dev, PTR_ERR(gpiod), "Failed to get %s GPIO\n",
+ GOODIX_GPIO_RST_NAME);
ts->gpiod_rst = gpiod;
--
2.34.1
^ permalink raw reply related
* [PATCH v4 12/24] Input: elants_i2c - Simplify with dev_err_probe()
From: Krzysztof Kozlowski @ 2023-06-25 16:28 UTC (permalink / raw)
To: Dmitry Torokhov, Hans de Goede, Linus Walleij, Bastien Nocera,
Sangwon Jee, Eugen Hristev, Mika Penttilä, linux-input,
linux-kernel, platform-driver-x86
Cc: Andi Shyti, Andy Shevchenko, Krzysztof Kozlowski, Andy Shevchenko
In-Reply-To: <20230625162817.100397-1-krzysztof.kozlowski@linaro.org>
Common pattern of handling deferred probe can be simplified with
dev_err_probe(). Less code and also it prints the error value.
Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
---
drivers/input/touchscreen/elants_i2c.c | 22 ++++++----------------
1 file changed, 6 insertions(+), 16 deletions(-)
diff --git a/drivers/input/touchscreen/elants_i2c.c b/drivers/input/touchscreen/elants_i2c.c
index 2da1db64126d..a1af3de9f310 100644
--- a/drivers/input/touchscreen/elants_i2c.c
+++ b/drivers/input/touchscreen/elants_i2c.c
@@ -1438,24 +1438,14 @@ static int elants_i2c_probe(struct i2c_client *client)
i2c_set_clientdata(client, ts);
ts->vcc33 = devm_regulator_get(&client->dev, "vcc33");
- if (IS_ERR(ts->vcc33)) {
- error = PTR_ERR(ts->vcc33);
- if (error != -EPROBE_DEFER)
- dev_err(&client->dev,
- "Failed to get 'vcc33' regulator: %d\n",
- error);
- return error;
- }
+ if (IS_ERR(ts->vcc33))
+ return dev_err_probe(&client->dev, PTR_ERR(ts->vcc33),
+ "Failed to get 'vcc33' regulator\n");
ts->vccio = devm_regulator_get(&client->dev, "vccio");
- if (IS_ERR(ts->vccio)) {
- error = PTR_ERR(ts->vccio);
- if (error != -EPROBE_DEFER)
- dev_err(&client->dev,
- "Failed to get 'vccio' regulator: %d\n",
- error);
- return error;
- }
+ if (IS_ERR(ts->vccio))
+ return dev_err_probe(&client->dev, PTR_ERR(ts->vccio),
+ "Failed to get 'vccio' regulator\n");
ts->reset_gpio = devm_gpiod_get(&client->dev, "reset", GPIOD_OUT_HIGH);
if (IS_ERR(ts->reset_gpio)) {
--
2.34.1
^ permalink raw reply related
* [PATCH v4 11/24] Input: ektf2127 - Simplify with dev_err_probe()
From: Krzysztof Kozlowski @ 2023-06-25 16:28 UTC (permalink / raw)
To: Dmitry Torokhov, Hans de Goede, Linus Walleij, Bastien Nocera,
Sangwon Jee, Eugen Hristev, Mika Penttilä, linux-input,
linux-kernel, platform-driver-x86
Cc: Andi Shyti, Andy Shevchenko, Krzysztof Kozlowski, Andy Shevchenko
In-Reply-To: <20230625162817.100397-1-krzysztof.kozlowski@linaro.org>
Common pattern of handling deferred probe can be simplified with
dev_err_probe(). Less code and also it prints the error value.
Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
---
drivers/input/touchscreen/ektf2127.c | 8 ++------
1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/drivers/input/touchscreen/ektf2127.c b/drivers/input/touchscreen/ektf2127.c
index fd8724a3c19f..cc3103b9cbfb 100644
--- a/drivers/input/touchscreen/ektf2127.c
+++ b/drivers/input/touchscreen/ektf2127.c
@@ -264,12 +264,8 @@ static int ektf2127_probe(struct i2c_client *client)
/* This requests the gpio *and* turns on the touchscreen controller */
ts->power_gpios = devm_gpiod_get(dev, "power", GPIOD_OUT_HIGH);
- if (IS_ERR(ts->power_gpios)) {
- error = PTR_ERR(ts->power_gpios);
- if (error != -EPROBE_DEFER)
- dev_err(dev, "Error getting power gpio: %d\n", error);
- return error;
- }
+ if (IS_ERR(ts->power_gpios))
+ return dev_err_probe(dev, PTR_ERR(ts->power_gpios), "Error getting power gpio\n");
input = devm_input_allocate_device(dev);
if (!input)
--
2.34.1
^ permalink raw reply related
* [PATCH v4 10/24] Input: edf-ft5x06 - Simplify with dev_err_probe()
From: Krzysztof Kozlowski @ 2023-06-25 16:28 UTC (permalink / raw)
To: Dmitry Torokhov, Hans de Goede, Linus Walleij, Bastien Nocera,
Sangwon Jee, Eugen Hristev, Mika Penttilä, linux-input,
linux-kernel, platform-driver-x86
Cc: Andi Shyti, Andy Shevchenko, Krzysztof Kozlowski, Andy Shevchenko
In-Reply-To: <20230625162817.100397-1-krzysztof.kozlowski@linaro.org>
Common pattern of handling deferred probe can be simplified with
dev_err_probe(). Less code and also it prints the error value.
Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
---
drivers/input/touchscreen/edt-ft5x06.c | 10 +++-------
1 file changed, 3 insertions(+), 7 deletions(-)
diff --git a/drivers/input/touchscreen/edt-ft5x06.c b/drivers/input/touchscreen/edt-ft5x06.c
index 795c7dad22bf..457d53337fbb 100644
--- a/drivers/input/touchscreen/edt-ft5x06.c
+++ b/drivers/input/touchscreen/edt-ft5x06.c
@@ -1168,13 +1168,9 @@ static int edt_ft5x06_ts_probe(struct i2c_client *client)
tsdata->max_support_points = chip_data->max_support_points;
tsdata->vcc = devm_regulator_get(&client->dev, "vcc");
- if (IS_ERR(tsdata->vcc)) {
- error = PTR_ERR(tsdata->vcc);
- if (error != -EPROBE_DEFER)
- dev_err(&client->dev,
- "failed to request regulator: %d\n", error);
- return error;
- }
+ if (IS_ERR(tsdata->vcc))
+ return dev_err_probe(&client->dev, PTR_ERR(tsdata->vcc),
+ "failed to request regulator\n");
tsdata->iovcc = devm_regulator_get(&client->dev, "iovcc");
if (IS_ERR(tsdata->iovcc)) {
--
2.34.1
^ permalink raw reply related
* [PATCH v4 09/24] Input: cy8ctma140 - Simplify with dev_err_probe()
From: Krzysztof Kozlowski @ 2023-06-25 16:28 UTC (permalink / raw)
To: Dmitry Torokhov, Hans de Goede, Linus Walleij, Bastien Nocera,
Sangwon Jee, Eugen Hristev, Mika Penttilä, linux-input,
linux-kernel, platform-driver-x86
Cc: Andi Shyti, Andy Shevchenko, Krzysztof Kozlowski, Andy Shevchenko
In-Reply-To: <20230625162817.100397-1-krzysztof.kozlowski@linaro.org>
Common pattern of handling deferred probe can be simplified with
dev_err_probe(). Less code and also it prints the error value.
Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Acked-by: Linus Walleij <linus.walleij@linaro.org>
---
drivers/input/touchscreen/cy8ctma140.c | 8 ++------
1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/drivers/input/touchscreen/cy8ctma140.c b/drivers/input/touchscreen/cy8ctma140.c
index 967ecde23e83..ea3895167b82 100644
--- a/drivers/input/touchscreen/cy8ctma140.c
+++ b/drivers/input/touchscreen/cy8ctma140.c
@@ -258,12 +258,8 @@ static int cy8ctma140_probe(struct i2c_client *client)
ts->regulators[1].supply = "vdd";
error = devm_regulator_bulk_get(dev, ARRAY_SIZE(ts->regulators),
ts->regulators);
- if (error) {
- if (error != -EPROBE_DEFER)
- dev_err(dev, "Failed to get regulators %d\n",
- error);
- return error;
- }
+ if (error)
+ return dev_err_probe(dev, error, "Failed to get regulators\n");
error = cy8ctma140_power_up(ts);
if (error)
--
2.34.1
^ permalink raw reply related
* [PATCH v4 08/24] Input: chipone_icn8318 - Simplify with dev_err_probe()
From: Krzysztof Kozlowski @ 2023-06-25 16:28 UTC (permalink / raw)
To: Dmitry Torokhov, Hans de Goede, Linus Walleij, Bastien Nocera,
Sangwon Jee, Eugen Hristev, Mika Penttilä, linux-input,
linux-kernel, platform-driver-x86
Cc: Andi Shyti, Andy Shevchenko, Krzysztof Kozlowski, Andy Shevchenko
In-Reply-To: <20230625162817.100397-1-krzysztof.kozlowski@linaro.org>
Common pattern of handling deferred probe can be simplified with
dev_err_probe(). Less code and also it prints the error value.
Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
---
drivers/input/touchscreen/chipone_icn8318.c | 8 ++------
1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/drivers/input/touchscreen/chipone_icn8318.c b/drivers/input/touchscreen/chipone_icn8318.c
index 9fbeaf17f00b..d6876d10b252 100644
--- a/drivers/input/touchscreen/chipone_icn8318.c
+++ b/drivers/input/touchscreen/chipone_icn8318.c
@@ -191,12 +191,8 @@ static int icn8318_probe(struct i2c_client *client)
return -ENOMEM;
data->wake_gpio = devm_gpiod_get(dev, "wake", GPIOD_OUT_LOW);
- if (IS_ERR(data->wake_gpio)) {
- error = PTR_ERR(data->wake_gpio);
- if (error != -EPROBE_DEFER)
- dev_err(dev, "Error getting wake gpio: %d\n", error);
- return error;
- }
+ if (IS_ERR(data->wake_gpio))
+ return dev_err_probe(dev, PTR_ERR(data->wake_gpio), "Error getting wake gpio\n");
input = devm_input_allocate_device(dev);
if (!input)
--
2.34.1
^ 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