* Re: [PATCH v10 2/4] Input: add core support for Goodix Berlin Touchscreen IC
From: Jeff LaBundy @ 2023-10-26 5:05 UTC (permalink / raw)
To: Neil Armstrong
Cc: Dmitry Torokhov, linux-input, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Bastien Nocera, Hans de Goede, Henrik Rydberg,
linux-arm-msm, devicetree, linux-kernel
In-Reply-To: <20231023-topic-goodix-berlin-upstream-initial-v10-2-88eec2e51c0b@linaro.org>
Hi Neil,
On Mon, Oct 23, 2023 at 05:03:46PM +0200, Neil Armstrong wrote:
[...]
> +static int goodix_berlin_get_ic_info(struct goodix_berlin_core *cd)
> +{
> + __le16 length_raw;
> + u8 *afe_data;
> + u16 length;
> + int error;
> +
> + afe_data = kzalloc(GOODIX_BERLIN_IC_INFO_MAX_LEN, GFP_KERNEL);
> + if (!afe_data)
> + return -ENOMEM;
> +
> + 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);
This should be dev_err().
> + goto free_afe_data;
> + }
> +
> + 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);
And here.
> + error = -EINVAL;
> + goto free_afe_data;
> + }
> +
> + 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;
> + goto free_afe_data;
> + }
This return statement is left over from v9; the print should also be dev_err().
> +
> + /* check whether the data is valid (ex. bus default values) */
> + if (goodix_berlin_is_dummy_data(cd, afe_data, length)) {
> + dev_err(cd->dev, "fw info data invalid\n");
> + error = -EINVAL;
> + goto free_afe_data;
> + }
> +
> + if (!goodix_berlin_checksum_valid(afe_data, length)) {
> + dev_info(cd->dev, "fw info checksum error\n");
And here.
> + error = -EINVAL;
> + goto free_afe_data;
> + }
> +
> + error = goodix_berlin_convert_ic_info(cd, afe_data, length);
> + if (error)
> + goto free_afe_data;
> +
> + /* check some key info */
> + if (!cd->touch_data_addr) {
> + dev_err(cd->dev, "touch_data_addr is null\n");
> + error = -EINVAL;
> + goto free_afe_data;
> + }
> +
> + return 0;
> +
> +free_afe_data:
> + kfree(afe_data);
> +
> + return error;
> +}
[...]
> +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);
I see that now, this function is only called if the reset GPIO is defined,
but there used to be another msleep() here that has since been dropped. Is
that intentional?
> +
> + return 0;
> +}
Kind regards,
Jeff LaBundy
^ permalink raw reply
* Re: [PATCH v3 2/2] Input: add Himax HX852x(ES) touchscreen driver
From: Jeff LaBundy @ 2023-10-26 4:49 UTC (permalink / raw)
To: Stephan Gerhold
Cc: Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Henrik Rydberg, linux-input, devicetree, linux-kernel,
Christophe JAILLET, Jonathan Albrieux
In-Reply-To: <20231024-hx852x-v3-2-a1890d3a81e9@gerhold.net>
Hi Stephan,
On Tue, Oct 24, 2023 at 08:35:46PM +0200, Stephan Gerhold wrote:
> From: Jonathan Albrieux <jonathan.albrieux@gmail.com>
>
> Add a simple driver for the Himax HX852x(ES) touch panel controller,
> with support for multi-touch and capacitive touch keys.
>
> The driver is somewhat based on sample code from Himax. However, that
> code was so extremely confusing that we spent a significant amount of
> time just trying to understand the packet format and register commands.
> In this driver they are described with clean structs and defines rather
> than lots of magic numbers and offset calculations.
>
> Signed-off-by: Jonathan Albrieux <jonathan.albrieux@gmail.com>
> Co-developed-by: Stephan Gerhold <stephan@gerhold.net>
> Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
Reviewed-by: Jeff LaBundy <jeff@labundy.com>
Many thanks for the productive discussion.
> ---
> MAINTAINERS | 7 +
> drivers/input/touchscreen/Kconfig | 10 +
> drivers/input/touchscreen/Makefile | 1 +
> drivers/input/touchscreen/himax_hx852x.c | 500 +++++++++++++++++++++++++++++++
> 4 files changed, 518 insertions(+)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 4cc6bf79fdd8..c0004b25b524 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -9264,6 +9264,13 @@ S: Maintained
> F: Documentation/devicetree/bindings/input/touchscreen/himax,hx83112b.yaml
> F: drivers/input/touchscreen/himax_hx83112b.c
>
> +HIMAX HX852X TOUCHSCREEN DRIVER
> +M: Stephan Gerhold <stephan@gerhold.net>
> +L: linux-input@vger.kernel.org
> +S: Maintained
> +F: Documentation/devicetree/bindings/input/touchscreen/himax,hx852es.yaml
> +F: drivers/input/touchscreen/himax_hx852x.c
> +
> HIPPI
> M: Jes Sorensen <jes@trained-monkey.org>
> L: linux-hippi@sunsite.dk
> diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
> index e3e2324547b9..8e5667ae5dab 100644
> --- a/drivers/input/touchscreen/Kconfig
> +++ b/drivers/input/touchscreen/Kconfig
> @@ -427,6 +427,16 @@ config TOUCHSCREEN_HIDEEP
> To compile this driver as a module, choose M here : the
> module will be called hideep_ts.
>
> +config TOUCHSCREEN_HIMAX_HX852X
> + tristate "Himax HX852x(ES) touchscreen"
> + depends on I2C
> + help
> + Say Y here if you have a Himax HX852x(ES) touchscreen.
> + If unsure, say N.
> +
> + To compile this driver as a module, choose M here: the module
> + will be called himax_hx852x.
> +
> config TOUCHSCREEN_HYCON_HY46XX
> tristate "Hycon hy46xx touchscreen support"
> depends on I2C
> diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile
> index 62bd24f3ac8e..f42a87faa86c 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_HIDEEP) += hideep.o
> +obj-$(CONFIG_TOUCHSCREEN_HIMAX_HX852X) += himax_hx852x.o
> obj-$(CONFIG_TOUCHSCREEN_HYNITRON_CSTXXX) += hynitron_cstxxx.o
> obj-$(CONFIG_TOUCHSCREEN_ILI210X) += ili210x.o
> obj-$(CONFIG_TOUCHSCREEN_ILITEK) += ilitek_ts_i2c.o
> diff --git a/drivers/input/touchscreen/himax_hx852x.c b/drivers/input/touchscreen/himax_hx852x.c
> new file mode 100644
> index 000000000000..6aa39f02829d
> --- /dev/null
> +++ b/drivers/input/touchscreen/himax_hx852x.c
> @@ -0,0 +1,500 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Himax HX852x(ES) Touchscreen Driver
> + * Copyright (c) 2020-2023 Stephan Gerhold <stephan@gerhold.net>
> + * Copyright (c) 2020 Jonathan Albrieux <jonathan.albrieux@gmail.com>
> + *
> + * Based on the Himax Android Driver Sample Code Ver 0.3 for HMX852xES chipset:
> + * Copyright (c) 2014 Himax Corporation.
> + */
> +
> +#include <asm/unaligned.h>
> +#include <linux/delay.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/i2c.h>
> +#include <linux/input.h>
> +#include <linux/input/mt.h>
> +#include <linux/input/touchscreen.h>
> +#include <linux/interrupt.h>
> +#include <linux/kernel.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/regulator/consumer.h>
> +
> +#define HX852X_COORD_SIZE(fingers) ((fingers) * sizeof(struct hx852x_coord))
> +#define HX852X_WIDTH_SIZE(fingers) ALIGN(fingers, 4)
> +#define HX852X_BUF_SIZE(fingers) (HX852X_COORD_SIZE(fingers) + \
> + HX852X_WIDTH_SIZE(fingers) + \
> + sizeof(struct hx852x_touch_info))
> +
> +#define HX852X_MAX_FINGERS 12
> +#define HX852X_MAX_KEY_COUNT 4
> +#define HX852X_MAX_BUF_SIZE HX852X_BUF_SIZE(HX852X_MAX_FINGERS)
> +
> +#define HX852X_TS_SLEEP_IN 0x80
> +#define HX852X_TS_SLEEP_OUT 0x81
> +#define HX852X_TS_SENSE_OFF 0x82
> +#define HX852X_TS_SENSE_ON 0x83
> +#define HX852X_READ_ONE_EVENT 0x85
> +#define HX852X_READ_ALL_EVENTS 0x86
> +#define HX852X_READ_LATEST_EVENT 0x87
> +#define HX852X_CLEAR_EVENT_STACK 0x88
> +
> +#define HX852X_REG_SRAM_SWITCH 0x8c
> +#define HX852X_REG_SRAM_ADDR 0x8b
> +#define HX852X_REG_FLASH_RPLACE 0x5a
> +
> +#define HX852X_SRAM_SWITCH_TEST_MODE 0x14
> +#define HX852X_SRAM_ADDR_CONFIG 0x7000
> +
> +struct hx852x {
> + struct i2c_client *client;
> + struct input_dev *input_dev;
> + struct touchscreen_properties props;
> + struct gpio_desc *reset_gpiod;
> + struct regulator_bulk_data supplies[2];
> + unsigned int max_fingers;
> + unsigned int keycount;
> + unsigned int keycodes[HX852X_MAX_KEY_COUNT];
> +};
> +
> +struct hx852x_config {
> + u8 rx_num;
> + u8 tx_num;
> + u8 max_pt;
> + u8 padding1[3];
> + __be16 x_res;
> + __be16 y_res;
> + u8 padding2[2];
> +} __packed __aligned(4);
> +
> +struct hx852x_coord {
> + __be16 x;
> + __be16 y;
> +} __packed __aligned(4);
> +
> +struct hx852x_touch_info {
> + u8 finger_num;
> + __le16 finger_pressed;
> + u8 padding;
> +} __packed __aligned(4);
> +
> +static int hx852x_i2c_read(struct hx852x *hx, u8 cmd, void *data, u16 len)
> +{
> + struct i2c_client *client = hx->client;
> + int ret;
> +
> + struct i2c_msg msg[] = {
> + {
> + .addr = client->addr,
> + .flags = 0,
> + .len = 1,
> + .buf = &cmd,
> + },
> + {
> + .addr = client->addr,
> + .flags = I2C_M_RD,
> + .len = len,
> + .buf = data,
> + },
> + };
> +
> + ret = i2c_transfer(client->adapter, msg, ARRAY_SIZE(msg));
> + if (ret != ARRAY_SIZE(msg)) {
> + dev_err(&client->dev, "failed to read %#x: %d\n", cmd, ret);
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static int hx852x_power_on(struct hx852x *hx)
> +{
> + struct device *dev = &hx->client->dev;
> + int error;
> +
> + error = regulator_bulk_enable(ARRAY_SIZE(hx->supplies), hx->supplies);
> + if (error) {
> + dev_err(dev, "failed to enable regulators: %d\n", error);
> + return error;
> + }
> +
> + gpiod_set_value_cansleep(hx->reset_gpiod, 1);
> + msleep(20);
> + gpiod_set_value_cansleep(hx->reset_gpiod, 0);
> + msleep(20);
> +
> + return 0;
> +}
> +
> +static int hx852x_start(struct hx852x *hx)
> +{
> + struct device *dev = &hx->client->dev;
> + int error;
> +
> + error = i2c_smbus_write_byte(hx->client, HX852X_TS_SLEEP_OUT);
> + if (error) {
> + dev_err(dev, "failed to send TS_SLEEP_OUT: %d\n", error);
> + return error;
> + }
> + msleep(30);
> +
> + error = i2c_smbus_write_byte(hx->client, HX852X_TS_SENSE_ON);
> + if (error) {
> + dev_err(dev, "failed to send TS_SENSE_ON: %d\n", error);
> + return error;
> + }
> + msleep(20);
> +
> + return 0;
> +}
> +
> +static int hx852x_stop(struct hx852x *hx)
> +{
> + struct device *dev = &hx->client->dev;
> + int error;
> +
> + error = i2c_smbus_write_byte(hx->client, HX852X_TS_SENSE_OFF);
> + if (error) {
> + dev_err(dev, "failed to send TS_SENSE_OFF: %d\n", error);
> + return error;
> + }
> + msleep(20);
> +
> + error = i2c_smbus_write_byte(hx->client, HX852X_TS_SLEEP_IN);
> + if (error) {
> + dev_err(dev, "failed to send TS_SLEEP_IN: %d\n", error);
> + return error;
> + }
> + msleep(30);
> +
> + return 0;
> +}
> +
> +static int hx852x_power_off(struct hx852x *hx)
> +{
> + struct device *dev = &hx->client->dev;
> + int error;
> +
> + error = regulator_bulk_disable(ARRAY_SIZE(hx->supplies), hx->supplies);
> + if (error) {
> + dev_err(dev, "failed to disable regulators: %d\n", error);
> + return error;
> + }
> +
> + return 0;
> +}
> +
> +static int hx852x_read_config(struct hx852x *hx)
> +{
> + struct device *dev = &hx->client->dev;
> + struct hx852x_config conf;
> + int x_res, y_res;
> + int error;
> +
> + error = hx852x_power_on(hx);
> + if (error)
> + return error;
> +
> + /* Sensing must be turned on briefly to load the config */
> + error = hx852x_start(hx);
> + if (error)
> + goto err_power_off;
> +
> + error = hx852x_stop(hx);
> + if (error)
> + goto err_power_off;
> +
> + error = i2c_smbus_write_byte_data(hx->client, HX852X_REG_SRAM_SWITCH,
> + HX852X_SRAM_SWITCH_TEST_MODE);
> + if (error)
> + goto err_power_off;
> +
> + error = i2c_smbus_write_word_data(hx->client, HX852X_REG_SRAM_ADDR,
> + HX852X_SRAM_ADDR_CONFIG);
> + if (error)
> + goto err_test_mode;
> +
> + error = hx852x_i2c_read(hx, HX852X_REG_FLASH_RPLACE, &conf, sizeof(conf));
> + if (error)
> + goto err_test_mode;
> +
> + x_res = be16_to_cpu(conf.x_res);
> + y_res = be16_to_cpu(conf.y_res);
> + hx->max_fingers = (conf.max_pt & 0xf0) >> 4;
> + dev_dbg(dev, "x res: %u, y res: %u, max fingers: %u\n",
> + x_res, y_res, hx->max_fingers);
> +
> + if (hx->max_fingers > HX852X_MAX_FINGERS) {
> + dev_err(dev, "max supported fingers: %u, found: %u\n",
> + HX852X_MAX_FINGERS, hx->max_fingers);
> + error = -EINVAL;
> + goto err_test_mode;
> + }
> +
> + if (x_res && y_res) {
> + input_set_abs_params(hx->input_dev, ABS_MT_POSITION_X, 0, x_res - 1, 0, 0);
> + input_set_abs_params(hx->input_dev, ABS_MT_POSITION_Y, 0, y_res - 1, 0, 0);
> + }
> +
> +err_test_mode:
> + error = i2c_smbus_write_byte_data(hx->client, HX852X_REG_SRAM_SWITCH, 0) ? : error;
> +err_power_off:
> + return hx852x_power_off(hx) ? : error;
> +}
> +
> +static int hx852x_handle_events(struct hx852x *hx)
> +{
> + /*
> + * The event packets have variable size, depending on the amount of
> + * supported fingers (hx->max_fingers). They are laid out as follows:
> + * - struct hx852x_coord[hx->max_fingers]: Coordinates for each finger
> + * - u8[ALIGN(hx->max_fingers, 4)]: Touch width for each finger
> + * with padding for 32-bit alignment
> + * - struct hx852x_touch_info
> + *
> + * Load everything into a 32-bit aligned buffer so the coordinates
> + * can be assigned directly, without using get_unaligned_*().
> + */
> + u8 buf[HX852X_MAX_BUF_SIZE] __aligned(4);
> + struct hx852x_coord *coord = (struct hx852x_coord *)buf;
> + u8 *width = &buf[HX852X_COORD_SIZE(hx->max_fingers)];
> + struct hx852x_touch_info *info = (struct hx852x_touch_info *)
> + &width[HX852X_WIDTH_SIZE(hx->max_fingers)];
> + unsigned long finger_pressed, key_pressed;
> + unsigned int i, x, y, w;
> + int error;
> +
> + error = hx852x_i2c_read(hx, HX852X_READ_ALL_EVENTS, buf,
> + HX852X_BUF_SIZE(hx->max_fingers));
> + if (error)
> + return error;
> +
> + finger_pressed = get_unaligned_le16(&info->finger_pressed);
> + key_pressed = finger_pressed >> HX852X_MAX_FINGERS;
> +
> + /* All bits are set when no touch is detected */
> + if (info->finger_num == 0xff || !(info->finger_num & 0x0f))
> + finger_pressed = 0;
> + if (key_pressed == 0xf)
> + key_pressed = 0;
> +
> + for_each_set_bit(i, &finger_pressed, hx->max_fingers) {
> + x = be16_to_cpu(coord[i].x);
> + y = be16_to_cpu(coord[i].y);
> + w = width[i];
> +
> + input_mt_slot(hx->input_dev, i);
> + input_mt_report_slot_state(hx->input_dev, MT_TOOL_FINGER, 1);
> + touchscreen_report_pos(hx->input_dev, &hx->props, x, y, true);
> + input_report_abs(hx->input_dev, ABS_MT_TOUCH_MAJOR, w);
> + }
> + input_mt_sync_frame(hx->input_dev);
> +
> + for (i = 0; i < hx->keycount; i++)
> + input_report_key(hx->input_dev, hx->keycodes[i], key_pressed & BIT(i));
> +
> + input_sync(hx->input_dev);
> + return 0;
> +}
> +
> +static irqreturn_t hx852x_interrupt(int irq, void *ptr)
> +{
> + struct hx852x *hx = ptr;
> + int error;
> +
> + error = hx852x_handle_events(hx);
> + if (error) {
> + dev_err_ratelimited(&hx->client->dev, "failed to handle events: %d\n", error);
> + return IRQ_NONE;
> + }
> +
> + return IRQ_HANDLED;
> +}
> +
> +static int hx852x_input_open(struct input_dev *dev)
> +{
> + struct hx852x *hx = input_get_drvdata(dev);
> + int error;
> +
> + error = hx852x_power_on(hx);
> + if (error)
> + return error;
> +
> + error = hx852x_start(hx);
> + if (error) {
> + hx852x_power_off(hx);
> + return error;
> + }
> +
> + enable_irq(hx->client->irq);
> + return 0;
> +}
> +
> +static void hx852x_input_close(struct input_dev *dev)
> +{
> + struct hx852x *hx = input_get_drvdata(dev);
> +
> + hx852x_stop(hx);
> + disable_irq(hx->client->irq);
> + hx852x_power_off(hx);
> +}
> +
> +static int hx852x_parse_properties(struct hx852x *hx)
> +{
> + struct device *dev = &hx->client->dev;
> + int error, count;
> +
> + count = device_property_count_u32(dev, "linux,keycodes");
> + if (count == -EINVAL) {
> + /* Property does not exist, keycodes are optional */
> + return 0;
> + } else if (count < 0) {
> + dev_err(dev, "Failed to read linux,keycodes: %d\n", count);
> + return count;
> + } else if (count > HX852X_MAX_KEY_COUNT) {
> + dev_err(dev, "max supported keys: %u, found: %u\n",
> + HX852X_MAX_KEY_COUNT, hx->keycount);
> + return -EINVAL;
> + }
> + hx->keycount = count;
> +
> + error = device_property_read_u32_array(dev, "linux,keycodes",
> + hx->keycodes, hx->keycount);
> + if (error) {
> + dev_err(dev, "failed to read linux,keycodes: %d\n", error);
> + return error;
> + }
> +
> + return 0;
> +}
> +
> +static int hx852x_probe(struct i2c_client *client)
> +{
> + struct device *dev = &client->dev;
> + struct hx852x *hx;
> + int error, i;
> +
> + if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C |
> + I2C_FUNC_SMBUS_WRITE_BYTE |
> + I2C_FUNC_SMBUS_WRITE_BYTE_DATA |
> + I2C_FUNC_SMBUS_WRITE_WORD_DATA)) {
> + dev_err(dev, "not all required i2c functionality supported\n");
> + return -ENXIO;
> + }
> +
> + hx = devm_kzalloc(dev, sizeof(*hx), GFP_KERNEL);
> + if (!hx)
> + return -ENOMEM;
> +
> + hx->client = client;
> + hx->input_dev = devm_input_allocate_device(dev);
> + if (!hx->input_dev)
> + return -ENOMEM;
> +
> + hx->input_dev->name = "Himax HX852x";
> + hx->input_dev->id.bustype = BUS_I2C;
> + hx->input_dev->open = hx852x_input_open;
> + hx->input_dev->close = hx852x_input_close;
> +
> + i2c_set_clientdata(client, hx);
> + input_set_drvdata(hx->input_dev, hx);
> +
> + hx->supplies[0].supply = "vcca";
> + hx->supplies[1].supply = "vccd";
> + error = devm_regulator_bulk_get(dev, ARRAY_SIZE(hx->supplies), hx->supplies);
> + if (error)
> + return dev_err_probe(dev, error, "failed to get regulators\n");
> +
> + hx->reset_gpiod = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
> + if (IS_ERR(hx->reset_gpiod))
> + return dev_err_probe(dev, PTR_ERR(hx->reset_gpiod),
> + "failed to get reset gpio\n");
> +
> + error = devm_request_threaded_irq(dev, client->irq, NULL, hx852x_interrupt,
> + IRQF_ONESHOT | IRQF_NO_AUTOEN, NULL, hx);
> + if (error)
> + return dev_err_probe(dev, error, "failed to request irq %d", client->irq);
> +
> + error = hx852x_read_config(hx);
> + if (error)
> + return error;
> +
> + input_set_capability(hx->input_dev, EV_ABS, ABS_MT_POSITION_X);
> + input_set_capability(hx->input_dev, EV_ABS, ABS_MT_POSITION_Y);
> + input_set_abs_params(hx->input_dev, ABS_MT_TOUCH_MAJOR, 0, 255, 0, 0);
> +
> + touchscreen_parse_properties(hx->input_dev, true, &hx->props);
> + error = hx852x_parse_properties(hx);
> + if (error)
> + return error;
> +
> + hx->input_dev->keycode = hx->keycodes;
> + hx->input_dev->keycodemax = hx->keycount;
> + hx->input_dev->keycodesize = sizeof(hx->keycodes[0]);
> + for (i = 0; i < hx->keycount; i++)
> + input_set_capability(hx->input_dev, EV_KEY, hx->keycodes[i]);
> +
> + error = input_mt_init_slots(hx->input_dev, hx->max_fingers,
> + INPUT_MT_DIRECT | INPUT_MT_DROP_UNUSED);
> + if (error)
> + return dev_err_probe(dev, error, "failed to init MT slots\n");
> +
> + error = input_register_device(hx->input_dev);
> + if (error)
> + return dev_err_probe(dev, error, "failed to register input device\n");
> +
> + return 0;
> +}
> +
> +static int hx852x_suspend(struct device *dev)
> +{
> + struct hx852x *hx = dev_get_drvdata(dev);
> + int error = 0;
> +
> + mutex_lock(&hx->input_dev->mutex);
> + if (input_device_enabled(hx->input_dev))
> + error = hx852x_stop(hx);
> + mutex_unlock(&hx->input_dev->mutex);
> +
> + return error;
> +}
> +
> +static int hx852x_resume(struct device *dev)
> +{
> + struct hx852x *hx = dev_get_drvdata(dev);
> + int error = 0;
> +
> + mutex_lock(&hx->input_dev->mutex);
> + if (input_device_enabled(hx->input_dev))
> + error = hx852x_start(hx);
> + mutex_unlock(&hx->input_dev->mutex);
> +
> + return error;
> +}
> +
> +static DEFINE_SIMPLE_DEV_PM_OPS(hx852x_pm_ops, hx852x_suspend, hx852x_resume);
> +
> +#ifdef CONFIG_OF
> +static const struct of_device_id hx852x_of_match[] = {
> + { .compatible = "himax,hx852es" },
> + { }
> +};
> +MODULE_DEVICE_TABLE(of, hx852x_of_match);
> +#endif
> +
> +static struct i2c_driver hx852x_driver = {
> + .probe = hx852x_probe,
> + .driver = {
> + .name = "himax_hx852x",
> + .pm = pm_sleep_ptr(&hx852x_pm_ops),
> + .of_match_table = of_match_ptr(hx852x_of_match),
> + },
> +};
> +module_i2c_driver(hx852x_driver);
> +
> +MODULE_DESCRIPTION("Himax HX852x(ES) Touchscreen Driver");
> +MODULE_AUTHOR("Jonathan Albrieux <jonathan.albrieux@gmail.com>");
> +MODULE_AUTHOR("Stephan Gerhold <stephan@gerhold.net>");
> +MODULE_LICENSE("GPL");
>
> --
> 2.42.0
>
Kind regards,
Jeff LaBundy
^ permalink raw reply
* Re: [PATCH v5 2/2] input: joystick: driver for Adafruit Seesaw Gamepad
From: Jeff LaBundy @ 2023-10-26 4:38 UTC (permalink / raw)
To: Anshul Dalal
Cc: linux-input, devicetree, Dmitry Torokhov, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Thomas Weißschuh,
Shuah Khan, linux-kernel-mentees, linux-kernel
In-Reply-To: <c4eed4e8-77e1-4129-ab6c-cc76ee4197db@gmail.com>
Hi Anshul,
On Wed, Oct 25, 2023 at 03:50:04PM +0530, Anshul Dalal wrote:
> Hello Jeff,
> Thanks for the review, I plan on addressing the changes you requested in
> the next version of the patch. Though I had a few questions:
Sounds great; thank you for the informative discussion.
>
> On 10/23/23 05:12, Jeff LaBundy wrote:
> > Hi Anshul,
> >
> > On Tue, Oct 17, 2023 at 09:13:45AM +0530, Anshul Dalal wrote:
> >> Adds a driver for a mini gamepad that communicates over i2c, the gamepad
> >> has bidirectional thumb stick input and six buttons.
> >>
> >> The gamepad chip utilizes the open framework from Adafruit called 'Seesaw'
> >> to transmit the ADC data for the joystick and digital pin state for the
> >> buttons. I have only implemented the functionality required to receive the
> >> thumb stick and button state.
> >>
> >> Steps in reading the gamepad state over i2c:
> >> 1. Reset the registers
> >> 2. Set the pin mode of the pins specified by the `BUTTON_MASK` to input
> >> `BUTTON_MASK`: A bit-map for the six digital pins internally
> >> connected to the joystick buttons.
> >> 3. Enable internal pullup resistors for the `BUTTON_MASK`
> >> 4. Bulk set the pin state HIGH for `BUTTON_MASK`
> >> 5. Poll the device for button and joystick state done by:
> >> `seesaw_read_data(struct i2c_client *client, struct seesaw_data *data)`
> >>
> >> Product page:
> >> https://www.adafruit.com/product/5743
> >> Arduino driver:
> >> https://github.com/adafruit/Adafruit_Seesaw
> >>
> >> Driver tested on RPi Zero 2W
> >>
> >> Reviewed-by: Thomas Weißschuh <linux@weissschuh.net>
> >> Signed-off-by: Anshul Dalal <anshulusr@gmail.com>
> >> ---
> >>
> >> Changes for v5:
> >> - Added link to the datasheet
> >> - Added debug log message when `seesaw_read_data` fails
> >>
> >> Changes for v4:
> >> - Changed `1UL << BUTTON_` to BIT(BUTTON_)
> >> - Removed `hardware_id` field from `struct seesaw_gamepad`
> >> - Removed redundant checks for the number of bytes written and received by
> >> `i2c_master_send` and `i2c_master_recv`
> >> - Used `get_unaligned_be32` to instantiate `u32 result` from `read_buf`
> >> - Changed `result & (1UL << BUTTON_)` to
> >> `test_bit(BUTTON_, (long *)&result)`
> >> - Changed `KBUILD_MODNAME` in id-tables to `SEESAW_DEVICE_NAME`
> >> - Fixed formatting issues
> >> - Changed button reporting:
> >> Since the gamepad had the action buttons in a non-standard layout:
> >> (X)
> >> (Y) (A)
> >> (B)
> >> Therefore moved to using generic directional action button event codes
> >> instead of BTN_[ABXY].
> >>
> >> Changes for v3:
> >> - no updates
> >>
> >> Changes for v2:
> >> adafruit-seesaw.c:
> >> - Renamed file from 'adafruit_seesaw.c'
> >> - Changed device name from 'seesaw_gamepad' to 'seesaw-gamepad'
> >> - Changed count parameter for receiving joystick x on line 118:
> >> `2` to `sizeof(write_buf)`
> >> - Fixed invalid buffer size on line 123 and 126:
> >> `data->y` to `sizeof(data->y)`
> >> - Added comment for the `mdelay(10)` on line 169
> >> - Changed inconsistent indentation on line 271
> >> Kconfig:
> >> - Fixed indentation for the help text
> >> - Updated module name
> >> Makefile:
> >> - Updated module object file name
> >> MAINTAINERS:
> >> - Updated file name for the driver and bindings
> >>
> >> MAINTAINERS | 7 +
> >> drivers/input/joystick/Kconfig | 9 +
> >> drivers/input/joystick/Makefile | 1 +
> >> drivers/input/joystick/adafruit-seesaw.c | 273 +++++++++++++++++++++++
> >> 4 files changed, 290 insertions(+)
> >> create mode 100644 drivers/input/joystick/adafruit-seesaw.c
> >>
> >> diff --git a/MAINTAINERS b/MAINTAINERS
> >> index 6c4cce45a09d..a314f9b48e21 100644
> >> --- a/MAINTAINERS
> >> +++ b/MAINTAINERS
> >> @@ -441,6 +441,13 @@ W: http://wiki.analog.com/AD7879
> >> W: https://ez.analog.com/linux-software-drivers
> >> F: drivers/input/touchscreen/ad7879.c
> >>
> >> +ADAFRUIT MINI I2C GAMEPAD
> >> +M: Anshul Dalal <anshulusr@gmail.com>
> >> +L: linux-input@vger.kernel.org
> >> +S: Maintained
> >> +F: Documentation/devicetree/bindings/input/adafruit,seesaw-gamepad.yaml
> >> +F: drivers/input/joystick/adafruit-seesaw.c
> >> +
> >> ADDRESS SPACE LAYOUT RANDOMIZATION (ASLR)
> >> M: Jiri Kosina <jikos@kernel.org>
> >> S: Maintained
> >> diff --git a/drivers/input/joystick/Kconfig b/drivers/input/joystick/Kconfig
> >> index ac6925ce8366..df9cd1830b29 100644
> >> --- a/drivers/input/joystick/Kconfig
> >> +++ b/drivers/input/joystick/Kconfig
> >> @@ -412,4 +412,13 @@ config JOYSTICK_SENSEHAT
> >> To compile this driver as a module, choose M here: the
> >> module will be called sensehat_joystick.
> >>
> >> +config JOYSTICK_SEESAW
> >> + tristate "Adafruit Mini I2C Gamepad with Seesaw"
> >> + depends on I2C
> >> + help
> >> + Say Y here if you want to use the Adafruit Mini I2C Gamepad.
> >> +
> >> + To compile this driver as a module, choose M here: the module will be
> >> + called adafruit-seesaw.
> >> +
> >> endif
> >> diff --git a/drivers/input/joystick/Makefile b/drivers/input/joystick/Makefile
> >> index 3937535f0098..9976f596a920 100644
> >> --- a/drivers/input/joystick/Makefile
> >> +++ b/drivers/input/joystick/Makefile
> >> @@ -28,6 +28,7 @@ obj-$(CONFIG_JOYSTICK_N64) += n64joy.o
> >> obj-$(CONFIG_JOYSTICK_PSXPAD_SPI) += psxpad-spi.o
> >> obj-$(CONFIG_JOYSTICK_PXRC) += pxrc.o
> >> obj-$(CONFIG_JOYSTICK_QWIIC) += qwiic-joystick.o
> >> +obj-$(CONFIG_JOYSTICK_SEESAW) += adafruit-seesaw.o
> >> obj-$(CONFIG_JOYSTICK_SENSEHAT) += sensehat-joystick.o
> >> obj-$(CONFIG_JOYSTICK_SIDEWINDER) += sidewinder.o
> >> obj-$(CONFIG_JOYSTICK_SPACEBALL) += spaceball.o
> >> diff --git a/drivers/input/joystick/adafruit-seesaw.c b/drivers/input/joystick/adafruit-seesaw.c
> >> new file mode 100644
> >> index 000000000000..2a1eae8d2861
> >> --- /dev/null
> >> +++ b/drivers/input/joystick/adafruit-seesaw.c
> >> @@ -0,0 +1,273 @@
> >> +// SPDX-License-Identifier: GPL-2.0-or-later
> >> +/*
> >> + * Copyright (C) 2023 Anshul Dalal <anshulusr@gmail.com>
> >> + *
> >> + * Driver for Adafruit Mini I2C Gamepad
> >> + *
> >> + * Based on the work of:
> >> + * Oleh Kravchenko (Sparkfun Qwiic Joystick driver)
> >> + *
> >> + * Datasheet: https://cdn-learn.adafruit.com/downloads/pdf/gamepad-qt.pdf
> >> + * Product page: https://www.adafruit.com/product/5743
> >> + * Firmware and hardware sources: https://github.com/adafruit/Adafruit_Seesaw
> >> + */
> >> +
> >> +#include <asm-generic/unaligned.h>
> >> +#include <linux/bits.h>
> >> +#include <linux/delay.h>
> >> +#include <linux/i2c.h>
> >> +#include <linux/input.h>
> >> +#include <linux/kernel.h>
> >> +#include <linux/module.h>
> >> +
> >> +/* clang-format off */
> >
> > I don't think we need this directive; at least, no other input drivers have
> > it, or really any drivers for that matter.
> >
> >> +#define SEESAW_DEVICE_NAME "seesaw-gamepad"
> >> +
> >> +#define SEESAW_STATUS_BASE 0
> >> +#define SEESAW_GPIO_BASE 1
> >> +#define SEESAW_ADC_BASE 9
> >> +
> >> +#define SEESAW_GPIO_DIRCLR_BULK 3
> >> +#define SEESAW_GPIO_BULK 4
> >> +#define SEESAW_GPIO_BULK_SET 5
> >> +#define SEESAW_GPIO_PULLENSET 11
> >> +
> >> +#define SEESAW_STATUS_HW_ID 1
> >> +#define SEESAW_STATUS_SWRST 127
> >> +
> >> +#define SEESAW_ADC_OFFSET 7
> >> +
> >> +#define BUTTON_A 5
> >> +#define BUTTON_B 1
> >> +#define BUTTON_X 6
> >> +#define BUTTON_Y 2
> >> +#define BUTTON_START 16
> >> +#define BUTTON_SELECT 0
> >
> > Please namespace these (e.g. SEESAW_BUTTON_A) to make it clear they refer
> > to device-specific bits and not standard keycodes (e.g. BTN_A). In fact,
> > these seem better off as part of an array of structs; more on that below.
> >
> >> +
> >> +#define ANALOG_X 14
> >> +#define ANALOG_Y 15
> >
> > Please namespace these as well.
> >
> >> +
> >> +#define SEESAW_JOYSTICK_MAX_AXIS 1023
> >> +#define SEESAW_JOYSTICK_FUZZ 2
> >> +#define SEESAW_JOYSTICK_FLAT 4
> >> +
> >> +#define SEESAW_GAMEPAD_POLL_INTERVAL 16
> >> +#define SEESAW_GAMEPAD_POLL_MIN 8
> >> +#define SEESAW_GAMEPAD_POLL_MAX 32
> >> +/* clang-format on */
> >> +
> >> +u32 BUTTON_MASK = BIT(BUTTON_A) | BIT(BUTTON_B) | BIT(BUTTON_X) |
> >> + BIT(BUTTON_Y) | BIT(BUTTON_START) | BIT(BUTTON_SELECT);
> >> +
> >> +struct seesaw_gamepad {
> >> + char physical_path[32];
> >> + struct input_dev *input_dev;
> >> + struct i2c_client *i2c_client;
> >> +};
> >> +
> >> +struct seesaw_data {
> >> + __be16 x;
> >> + __be16 y;
> >> + u8 button_a, button_b, button_x, button_y, button_start, button_select;
> >
> > Please keep these each on a separate line.
> >
> >> +};
> >
> > Please declare this struct as __packed, as that is how it appears to be used.
> >
> >> +
> >> +static int seesaw_read_data(struct i2c_client *client, struct seesaw_data *data)
> >> +{
> >> + int err;
> >
> > Please use 'ret' for return variables that can indicate a positive value on success.
> >
> >> + unsigned char write_buf[2] = { SEESAW_GPIO_BASE, SEESAW_GPIO_BULK };
> >> + unsigned char read_buf[4];
> >
> > Please use standard kernel type definitions (i.e. u8 in this case).
> >
> >> +
> >> + err = i2c_master_send(client, write_buf, sizeof(write_buf));
> >> + if (err < 0)
> >> + return err;
> >
> > You correctly return err (or rather, ret) for negative values, but you should also
> > check that ret matches the size of the data sent. For 0 <= ret < sizeof(writebuf),
> > return -EIO.
> >
> >> + err = i2c_master_recv(client, read_buf, sizeof(read_buf));
> >> + if (err < 0)
> >> + return err;
> >
> > And here.
> >
> >> +
> >> + u32 result = get_unaligned_be32(&read_buf);
> >
> > Please do not mix declarations and code; all declarations must be at the
> > top of the function.
> >
> >> +
> >> + data->button_a = !test_bit(BUTTON_A, (long *)&result);
> >> + data->button_b = !test_bit(BUTTON_B, (long *)&result);
> >> + data->button_x = !test_bit(BUTTON_X, (long *)&result);
> >> + data->button_y = !test_bit(BUTTON_Y, (long *)&result);
> >> + data->button_start = !test_bit(BUTTON_START, (long *)&result);
> >> + data->button_select = !test_bit(BUTTON_SELECT, (long *)&result);
> >> +
> >> + write_buf[0] = SEESAW_ADC_BASE;
> >> + write_buf[1] = SEESAW_ADC_OFFSET + ANALOG_X;
> >> + err = i2c_master_send(client, write_buf, sizeof(write_buf));
> >> + if (err < 0)
> >> + return err;
> >> + err = i2c_master_recv(client, (char *)&data->x, sizeof(data->x));
> >> + if (err < 0)
> >> + return err;
> >
> > This is starting to look like a 16-bit register map. To that end, please
> > consider using regmap instead of open-coding each of these standard write-
> > then-read operations.
> >
> > Using regmap would also save you the trouble of managing the endianness
> > yourself, as well as having to check for incomplete transfers since its
> > functions return zero or a negative error code only.
> >
> In this driver there are only two places a 16-bit regmap could be used,
> for getting the joystick X and Y values. I see minimal utility in adding
> the boilerplate necessary to use the more sophisticated regmap API in
> this case.
I counted a total of three sequences that write two bytes (i.e. a 16-bit
"address"), send a stop condition, then read back a modulo-2 number of
bytes. If the hardware can tolerate a repeated start in between the write
and read operations, which is quite common, all of these can be replaced
with a single call to regmap_read().
A fourth sequence reads back a single byte after the 16-bit "address",
while a fifth writes a single byte after the 16-bit "address." Those two
admittedly break the model.
Given those two oddballs in seesaw_probe(), maybe regmap is not the best
solution after all. You could, however, mix the two and use regmap where
it works, and roll your own where it doesn't.
>
> As for the handling of endianness, if I am not mistaken the
> `be16_to_cpu` macro should manage it.
Right, what I mean to say is that regmap calls be16_to_cpu() for you. You
do not need to do any extra operations on the values returned by regmap.
>
> If you prefer I could add the following function to reduce code duplication:
>
> int seesaw_get_analog(int pin) {
> __be16 result;
> u8 write_buf[2] = { SEESAW_ADC_BASE, SEESAW_ADC_OFFSET + pin };
> int ret;
> ret = i2c_master_send(client, write_buf, sizeof(write_buf));
> if (ret < 0)
> return ret;
> ret = i2c_master_recv(client, (char *)&result, sizeof(result));
> if (ret < 0)
> return ret;
> return result;
> }
Assuming regmap is out of the picture, I'd like to see something even more
generic like the following:
static int seesaw_register_read(struct i2c_client *client,
u16 reg, void *val, u16 len)
{
__be16 reg_buf = cpu_to_be16(reg);
int ret;
ret = i2c_master_send(client, (char *)®_buf, sizeof(reg_buf));
if (ret < 0)
return ret;
ret = i2c_master_recv(client, (char *)&val, len);
if (ret < 0)
return ret;
return 0;
}
A call to seesaw_register_read() might look like the following:
int error;
__be16 val;
error = seesaw_register_read(client,
SEESAW_ADC_BASE + SEESAW_ADC_OFFSET + ANALOG_X,
&val, sizeof(val);
if (error)
return error;
input_report_abs(input, ABS_X, be16_to_cpu(val));
Last but not least:
static int seesaw_register_write(struct i2c_client *client, u16 reg, u8 val)
{
u8 buf[sizeof(reg) + sizeof(val)];
int ret;
put_unaligned_be16(reg, buf);
*(buf + sizeof(reg)) = val;
ret = i2c_master_send(client, (char *)&buf, sizeof(buf));
if (ret < 0)
return ret;
return 0;
}
And to reset the device:
error = seesaw_register_write(client,
SEESAW_STATUS_BASE + SEESAW_STATUS_SWRST, 0xFF);
if (error)
return error;
You can extend this as necessary to support the pin configuration registers
discussed below. Does this seem like a reasonable compromise?
>
> >> + /*
> >> + * ADC reads left as max and right as 0, must be reversed since kernel
> >> + * expects reports in opposite order.
> >> + */
> >> + data->x = SEESAW_JOYSTICK_MAX_AXIS - be16_to_cpu(data->x);
> >> +
> >> + write_buf[1] = SEESAW_ADC_OFFSET + ANALOG_Y;
> >> + err = i2c_master_send(client, write_buf, sizeof(write_buf));
> >> + if (err < 0)
> >> + return err;
> >> + err = i2c_master_recv(client, (char *)&data->y, sizeof(data->y));
> >> + if (err < 0)
> >> + return err;
> >> + data->y = be16_to_cpu(data->y);
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +static void seesaw_poll(struct input_dev *input)
> >> +{
> >> + struct seesaw_gamepad *private = input_get_drvdata(input);
> >> + struct seesaw_data data;
> >> + int err;
> >> +
> >> + err = seesaw_read_data(private->i2c_client, &data);
> >> + if (err != 0) {
> >> + dev_dbg(&input->dev, "failed to read joystick state: %d\n",
> >> + err);
> >
> > This should be dev_err_ratelimited().
> >
> >> + return;
> >> + }
> >> +
> >> + input_report_abs(input, ABS_X, data.x);
> >> + input_report_abs(input, ABS_Y, data.y);
> >> + input_report_key(input, BTN_EAST, data.button_a);
> >> + input_report_key(input, BTN_SOUTH, data.button_b);
> >> + input_report_key(input, BTN_NORTH, data.button_x);
> >> + input_report_key(input, BTN_WEST, data.button_y);
> >> + input_report_key(input, BTN_START, data.button_start);
> >> + input_report_key(input, BTN_SELECT, data.button_select);
> >
> > I think you can make this much cleaner and smaller by defining an array
> > of structs, each with a key code and bit position. You can then simply
> > iterate over the array and call input_report_key() once per element as
> > in the following:
> >
> > struct seesaw_btn_desc {
> > unsigned int code;
> > unsigned int shift;
> > };
> >
> > static const struct seesaw_btn_desc seesaw_btns[] = {
> > {
> > .code = BTN_EAST,
> > .mask = 5,
> > },
> > [...]
> > };
> >
> > And then:
> >
> > btn_status = ...;
> >
> > for (i = 0; i < ARRAY_SIZE(seesaw_btns); i++)
> > input_report_key(input, seesaw_btns[i].code,
> > btn_status & seesaw_btns[i].mask);
> >
> > This would also make it easier to quickly discern what keycodes are mapped
> > to which bits in the register.
> >
> >> + input_sync(input);
> >> +}
> >> +
> >> +static int seesaw_probe(struct i2c_client *client)
> >> +{
> >> + int err;
> >> + struct seesaw_gamepad *private;
> >
> > I'd rather this be called something like 'seesaw' rather than private.
> >
> >> + unsigned char register_reset[] = { SEESAW_STATUS_BASE,
> >> + SEESAW_STATUS_SWRST, 0xFF };
> >> + unsigned char get_hw_id[] = { SEESAW_STATUS_BASE, SEESAW_STATUS_HW_ID };
> >> +
> >> + err = i2c_master_send(client, register_reset, sizeof(register_reset));
> >> + if (err < 0)
> >> + return err;
> >> +
> >> + /* Wait for the registers to reset before proceeding */
> >> + mdelay(10);
> >> +
> >> + private = devm_kzalloc(&client->dev, sizeof(*private), GFP_KERNEL);
> >> + if (!private)
> >> + return -ENOMEM;
> >> +
> >> + err = i2c_master_send(client, get_hw_id, sizeof(get_hw_id));
> >> + if (err < 0)
> >> + return err;
> >> +
> >> + unsigned char hardware_id;
> >
> > Same comment as earlier with regard to mixed declarations.
> >
> >> +
> >> + err = i2c_master_recv(client, &hardware_id, 1);
> >> + if (err < 0)
> >> + return err;
> >> +
> >> + dev_dbg(&client->dev, "Adafruit Seesaw Gamepad, Hardware ID: %02x\n",
> >> + hardware_id);
> >> +
> >> + private->i2c_client = client;
> >> + scnprintf(private->physical_path, sizeof(private->physical_path),
> >> + "i2c/%s", dev_name(&client->dev));
> >
> > This seems overly complex; can we not simply set input_dev->phys to the
> > literal "i2c/seesaw-gamepad"? Why to copy at runtime and incur the cost
> > of carrying 'physical_path' throughout the life of the module?
> >
> >> + i2c_set_clientdata(client, private);
> >> +
> >> + private->input_dev = devm_input_allocate_device(&client->dev);
> >> + if (!private->input_dev)
> >> + return -ENOMEM;
> >> +
> >> + private->input_dev->id.bustype = BUS_I2C;
> >> + private->input_dev->name = "Adafruit Seesaw Gamepad";
> >> + private->input_dev->phys = private->physical_path;
> >> + input_set_drvdata(private->input_dev, private);
> >> + input_set_abs_params(private->input_dev, ABS_X, 0,
> >> + SEESAW_JOYSTICK_MAX_AXIS, SEESAW_JOYSTICK_FUZZ,
> >> + SEESAW_JOYSTICK_FLAT);
> >> + input_set_abs_params(private->input_dev, ABS_Y, 0,
> >> + SEESAW_JOYSTICK_MAX_AXIS, SEESAW_JOYSTICK_FUZZ,
> >> + SEESAW_JOYSTICK_FLAT);
> >> + input_set_capability(private->input_dev, EV_KEY, BTN_EAST);
> >> + input_set_capability(private->input_dev, EV_KEY, BTN_SOUTH);
> >> + input_set_capability(private->input_dev, EV_KEY, BTN_NORTH);
> >> + input_set_capability(private->input_dev, EV_KEY, BTN_WEST);
> >> + input_set_capability(private->input_dev, EV_KEY, BTN_START);
> >> + input_set_capability(private->input_dev, EV_KEY, BTN_SELECT);
> >
> > Same comment with regard to creating an array of structs, and hence only
> > having to call input_set_capability() from within a small loop.
> >
> >> +
> >> + err = input_setup_polling(private->input_dev, seesaw_poll);
> >> + if (err) {
> >> + dev_err(&client->dev, "failed to set up polling: %d\n", err);
> >> + return err;
> >> + }
> >> +
> >> + input_set_poll_interval(private->input_dev,
> >> + SEESAW_GAMEPAD_POLL_INTERVAL);
> >> + input_set_max_poll_interval(private->input_dev,
> >> + SEESAW_GAMEPAD_POLL_MAX);
> >> + input_set_min_poll_interval(private->input_dev,
> >> + SEESAW_GAMEPAD_POLL_MIN);
> >> +
> >> + err = input_register_device(private->input_dev);
> >> + if (err) {
> >> + dev_err(&client->dev, "failed to register joystick: %d\n", err);
> >> + return err;
> >> + }
> >> +
> >> + /* Set Pin Mode to input and enable pull-up resistors */
> >> + unsigned char pin_mode[] = { SEESAW_GPIO_BASE, SEESAW_GPIO_DIRCLR_BULK,
> >> + BUTTON_MASK >> 24, BUTTON_MASK >> 16,
> >> + BUTTON_MASK >> 8, BUTTON_MASK };
> >> + err = i2c_master_send(client, pin_mode, sizeof(pin_mode));
> >> + if (err < 0)
> >> + return err;
> >> + pin_mode[1] = SEESAW_GPIO_PULLENSET;
> >> + err = i2c_master_send(client, pin_mode, sizeof(pin_mode));
> >> + if (err < 0)
> >> + return err;
> >> + pin_mode[1] = SEESAW_GPIO_BULK_SET;
> >> + err = i2c_master_send(client, pin_mode, sizeof(pin_mode));
> >> + if (err < 0)
> >> + return err;
> >
> > Please configure the HW before the input device is live and being polled.
> >
>
> Could you elaborate on what you meant by this. To my knowledge, the
> device is ready to be polled right after the pin state for the
> `BUTTON_MASK` is configured. That is also how it's done in the Arduino
> driver provided by the manufacturer. Please clarify if I'm missing
> something here.
Normally, we want to do the following:
1. Configure the hardware.
2. Register the input device.
3. Request an interrupt line or enable polling.
Here, we have placed step (1) at the end of the sequence, which is dangerous
for two reasons:
1. For a brief moment, the device is availing button status to the input core
while the pull-up resistors are not yet enabled, and the buttons are in an
undefined state. Any kind of electrical noise or disturbance may trigger a
false button event.
2. The input poller is reading registers and changing the device's internal
address pointer at the same time probe() is still writing some registers.
This is a concurrency problem.
If what is shown is how the Arduino reference design operates, then I would
argue the reference design is mistaken, or not subject to the same constraints
and behaviors as a Linux input driver. Therefore, please set the pin mode much
earlier in probe().
>
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +#ifdef CONFIG_OF
> >> +static const struct of_device_id of_seesaw_match[] = {
> >> + {
> >> + .compatible = "adafruit,seesaw-gamepad",
> >> + },
> >> + { /* Sentinel */ }
> >> +};
> >> +MODULE_DEVICE_TABLE(of, of_seesaw_match);
> >> +#endif /* CONFIG_OF */
> >
> > Please correct me if I am wrong, but it does not seem that OF support is
> > required by this driver. There are no properties beyond the standard ones
> > understood by the I2C core, which can match based on the ID table below.
> >
> >> +
> >> +/* clang-format off */
> >> +static const struct i2c_device_id seesaw_id_table[] = {
> >> + { SEESAW_DEVICE_NAME, 0 },
> >> + { /* Sentinel */ }
> >> +};
> >> +/* clang-format on */
> >
> > Again, I don't see any need for these directives.
> >
> >> +
> >
> > Nit: unnecessary NL.
> >
> >> +MODULE_DEVICE_TABLE(i2c, seesaw_id_table);
> >> +
> >> +static struct i2c_driver seesaw_driver = {
> >> + .driver = {
> >> + .name = SEESAW_DEVICE_NAME,
> >> + .of_match_table = of_match_ptr(of_seesaw_match),
> >> + },
> >> + .id_table = seesaw_id_table,
> >> + .probe = seesaw_probe,
> >> +};
> >> +module_i2c_driver(seesaw_driver);
> >> +
> >> +MODULE_AUTHOR("Anshul Dalal <anshulusr@gmail.com>");
> >> +MODULE_DESCRIPTION("Adafruit Mini I2C Gamepad driver");
> >> +MODULE_LICENSE("GPL");
> >> --
> >> 2.42.0
> >>
> >
> > Kind regards,
> > Jeff LaBundy
>
> Regards,
> Anshul Dalal
Kind regards,
Jeff LaBundy
^ permalink raw reply
* Re: [PATCH v3 00/12] HID: logitech-hidpp: Avoid hidpp_connect_event() running while probe() restarts IO
From: Benjamin Tissoires @ 2023-10-25 19:03 UTC (permalink / raw)
To: Filipe Laíns, Bastien Nocera, Jiri Kosina,
Benjamin Tissoires, Hans de Goede
Cc: linux-input
In-Reply-To: <20231010102029.111003-1-hdegoede@redhat.com>
On Tue, 10 Oct 2023 12:20:17 +0200, Hans de Goede wrote:
> As dicussed here is v3 of my series to rework / cleanup the hidpp
> probing code.
>
> Note the $subject of the cover-letter is not entirely accurate anymore,
> but I kept it the same since this is the successor of series with
> the same subject.
>
> [...]
Applied to https://git.kernel.org/pub/scm/linux/kernel/git/hid/hid.git (for-6.7/logitech), thanks!
[01/12] HID: logitech-hidpp: Don't restart IO, instead defer hid_connect() only
https://git.kernel.org/hid/hid/c/11ca0322a419
[02/12] HID: logitech-hidpp: Revert "Don't restart communication if not necessary"
https://git.kernel.org/hid/hid/c/55bf70362ffc
[03/12] HID: logitech-hidpp: Move get_wireless_feature_index() check to hidpp_connect_event()
https://git.kernel.org/hid/hid/c/ba9de3505095
[04/12] HID: logitech-hidpp: Remove wtp_get_config() call from probe()
https://git.kernel.org/hid/hid/c/a3643036d7a8
[05/12] HID: logitech-hidpp: Move g920_get_config() to just before hidpp_ff_init()
https://git.kernel.org/hid/hid/c/219ccfb60003
[06/12] HID: logitech-hidpp: Move hidpp_overwrite_name() to before connect check
https://git.kernel.org/hid/hid/c/8954dac18c68
[07/12] HID: logitech-hidpp: Add hidpp_non_unifying_init() helper
https://git.kernel.org/hid/hid/c/c14f1485c605
[08/12] HID: logitech-hidpp: Remove connected check for non-unifying devices
https://git.kernel.org/hid/hid/c/6f335b47adc3
[09/12] HID: logitech-hidpp: Remove unused connected param from *_connect()
https://git.kernel.org/hid/hid/c/bb17b2c6dd87
[10/12] HID: logitech-hidpp: Fix connect event race
https://git.kernel.org/hid/hid/c/680ee411a98e
[11/12] HID: logitech-hidpp: Drop delayed_work_cb()
https://git.kernel.org/hid/hid/c/f3c4ee7166f2
[12/12] HID: logitech-hidpp: Drop HIDPP_QUIRK_UNIFYING
https://git.kernel.org/hid/hid/c/9ce363aa009c
Cheers,
--
Benjamin Tissoires <bentiss@kernel.org>
^ permalink raw reply
* Re: [PATCH v3 01/12] HID: logitech-hidpp: Don't restart IO, instead defer hid_connect() only
From: Hans de Goede @ 2023-10-25 19:02 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: Filipe Laíns, Bastien Nocera, Jiri Kosina,
Benjamin Tissoires, linux-input
In-Reply-To: <5xlyq6eoj2twunztyi6u63hw7hezljtt6jacg3hcuwp63rd4g6@uffuqovrugk4>
Hi,
On 10/25/23 18:43, Benjamin Tissoires wrote:
> On Oct 18 2023, Benjamin Tissoires wrote:
>> Hi Hans,
>>
>> FWIW, your series looks good, but I came accross a small nitpick here:
>>
>> On Oct 10 2023, Hans de Goede wrote:
>>> Restarting IO causes 2 problems:
>>>
>>> 1. Some devices do not like IO being restarted this was addressed in
>>> commit 498ba2069035 ("HID: logitech-hidpp: Don't restart communication
>>> if not necessary"), but that change has issues of its own and needs to
>>> be reverted.
>>>
>>> 2. Restarting IO and specifically calling hid_device_io_stop() causes
>>> received packets to be missed, which may cause connect-events to
>>> get missed.
>>>
>>> Restarting IO was introduced in commit 91cf9a98ae41 ("HID: logitech-hidpp:
>>> make .probe usbhid capable") to allow to retrieve the device's name and
>>> serial number and store these in hdev->name and hdev->uniq before
>>> connecting any hid subdrivers (hid-input, hidraw) exporting this info
>>> to userspace.
>>>
>>> But this does not require restarting IO, this merely requires deferring
>>> calling hid_connect(). Calling hid_hw_start() with a connect-mask of
>>> 0 makes it skip calling hid_connect(), so hidpp_probe() can simply call
>>> hid_connect() later without needing to restart IO.
>>>
>>> Remove the stop + restart of IO and instead just call hid_connect() later
>>> to avoid the issues caused by restarting IO.
>>>
>>> Now that IO is no longer stopped, hid_hw_close() must be called at the end
>>> of probe() to balance the hid_hw_open() done at the beginning probe().
>>>
>>> This series has been tested on the following devices:
>>> Logitech Bluetooth Laser Travel Mouse (bluetooth, HID++ 1.0)
>>> Logitech M720 Triathlon (bluetooth, HID++ 4.5)
>>> Logitech M720 Triathlon (unifying, HID++ 4.5)
>>> Logitech K400 Pro (unifying, HID++ 4.1)
>>> Logitech K270 (eQUAD nano Lite, HID++ 2.0)
>>> Logitech M185 (eQUAD nano Lite, HID++ 4.5)
>>> Logitech LX501 keyboard (27 Mhz, HID++ builtin scroll-wheel, HID++ 1.0)
>>> Logitech M-RAZ105 mouse (27 Mhz, HID++ extra mouse buttons, HID++ 1.0)
>>>
>>> Fixes: 498ba2069035 ("HID: logitech-hidpp: Don't restart communication if not necessary")
>>> Suggested-by: Benjamin Tissoires <bentiss@kernel.org>
>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>> ---
>>> drivers/hid/hid-logitech-hidpp.c | 22 ++++++++++++----------
>>> 1 file changed, 12 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
>>> index a209d51bd247..aa4f232c4518 100644
>>> --- a/drivers/hid/hid-logitech-hidpp.c
>>> +++ b/drivers/hid/hid-logitech-hidpp.c
>>> @@ -4460,8 +4460,10 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
>>> hdev->name);
>>>
>>> /*
>>> - * Plain USB connections need to actually call start and open
>>> - * on the transport driver to allow incoming data.
>>> + * First call hid_hw_start(hdev, 0) to allow IO without connecting any
>>> + * hid subdrivers (hid-input, hidraw). This allows retrieving the dev's
>>> + * name and serial number and store these in hdev->name and hdev->uniq,
>>> + * before the hid-input and hidraw drivers expose these to userspace.
>>> */
>>> ret = hid_hw_start(hdev, will_restart ? 0 : connect_mask);
>>> if (ret) {
>>> @@ -4519,19 +4521,14 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
>>> flush_work(&hidpp->work);
>>>
>>> if (will_restart) {
>>> - /* Reset the HID node state */
>>> - hid_device_io_stop(hdev);
>>> - hid_hw_close(hdev);
>>> - hid_hw_stop(hdev);
>>> -
>>> if (hidpp->quirks & HIDPP_QUIRK_DELAYED_INIT)
>>> connect_mask &= ~HID_CONNECT_HIDINPUT;
>>>
>>> /* Now export the actual inputs and hidraw nodes to the world */
>>> - ret = hid_hw_start(hdev, connect_mask);
>>> + ret = hid_connect(hdev, connect_mask);
>>
>> On plain USB devices, we get a new warning here "io already started".
>>
>> This is because hid_connect() will call hid_pidff_init() from
>> drivers/hid/usbhid/hid-pidff.c if connect_mask has the `HID_CONNECT_FF`
>> bit set.
>>
>> And hid_pidff_init() blindly calls hid_device_io_start() then
>> hid_device_io_stop().
>>
>> It's not a big issue, but still it's a new warning we have to tackly on.
>>
>> I see 3 possible solutions:
>> - teach hid_pidff_init() to only start/stop the IOs if it's not already
>> done so
>> - if a device is actually connected through USB, call
>> hid_device_io_stop() before hid_connect()
>> - unconditionally call hid_device_io_stop() before hid_connect()
>>
>> The latter has my current preference as we won't get biten in the future
>> if something else decides to change the io state.
>>
>> However, do you thing it'll be an issue to disable IOs there?
Not really an issue, but if we disable IOs then we may loose
incoming packets with a connect event in there.
>> And maybe we should re-enable them after?
If we disable IOs before hid_connect() (or at any point
after enabling them) then connect events may be lost
so we must re-enable IOs then and move the hidpp_connect_event()
work queuing, which polls for already connected devices to
after the re-enabling.
Also IOs need to be re-enabled for the g920_get_config()
call later during hidpp_probe().
I have just written a patch for this and submitted it upstream :)
>> If it's fine to simply disable the IOs, we can add an extra patch on top
>> of this series to fix that warning in the USB case.
>>
>> As mentioned above, I have tested with the T650, T651 that were likely to
>> be a problem, and they are working just fine :)
>>
>> So with that minor fix, we should be able to stage this series.
>
> The merge window is coming very soon. So I'm taking this series as it is
> (I just added the few devices I made the tests), and we can work on an
> extra patch to remove that warning.
Extra patch submitted :)
Regards,
Hans
^ permalink raw reply
* [PATCH] HID: logitech-hidpp: Stop IO before calling hid_connect()
From: Hans de Goede @ 2023-10-25 19:01 UTC (permalink / raw)
To: Filipe Laíns, Bastien Nocera, Jiri Kosina,
Benjamin Tissoires
Cc: Hans de Goede, linux-input
hid_connect() will call hid_pidff_init() which does
hid_device_io_start() leading to an "io already started" warning.
To fix this call hid_device_io_stop() before calling hid_connect(),
stopping IO means that connect events may be lost while hid_connect()
runs, re-enable IO and move the hidpp_connect_event() work queuing
after the hid_connect().
Note re-enabling IO is also necessary for the g920_get_config()
call later during hidpp_probe().
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
drivers/hid/hid-logitech-hidpp.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index c1bc89560612..fd6d8f1d9b8f 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -4461,19 +4461,22 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
else
hidpp_non_unifying_init(hidpp);
- schedule_work(&hidpp->work);
- flush_work(&hidpp->work);
-
if (hidpp->quirks & HIDPP_QUIRK_DELAYED_INIT)
connect_mask &= ~HID_CONNECT_HIDINPUT;
/* Now export the actual inputs and hidraw nodes to the world */
+ hid_device_io_stop(hdev);
ret = hid_connect(hdev, connect_mask);
if (ret) {
hid_err(hdev, "%s:hid_connect returned error %d\n", __func__, ret);
goto hid_hw_init_fail;
}
+ /* Check for connected devices now that incoming packets will not be disabled again */
+ hid_device_io_start(hdev);
+ schedule_work(&hidpp->work);
+ flush_work(&hidpp->work);
+
if (hidpp->quirks & HIDPP_QUIRK_CLASS_G920) {
struct hidpp_ff_private_data data;
--
2.41.0
^ permalink raw reply related
* Re: [PATCH 0/4] HID: remove #ifdef CONFIG_PM
From: Benjamin Tissoires @ 2023-10-25 19:02 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires, Thomas Weißschuh
Cc: linux-input, linux-kernel, linux-usb
In-Reply-To: <20231012-hid-pm_ptr-v1-0-0a71531ca93b@weissschuh.net>
On Thu, 12 Oct 2023 12:23:37 +0200, Thomas Weißschuh wrote:
> Through the usage of pm_ptr() the CONFIG_PM-dependent code will always be
> compiled, protecting against bitrot.
> The linker will then garbage-collect the unused function avoiding any overhead.
>
> This series only converts three users of CONFIG_PM in drivers/hid/ but
> most of the others should be convertible, too.
>
> [...]
Applied to https://git.kernel.org/pub/scm/linux/kernel/git/hid/hid.git (for-6.7/config_pm), thanks!
[1/4] HID: core: remove #ifdef CONFIG_PM from hid_driver
https://git.kernel.org/hid/hid/c/df8b030d82dd
[2/4] HID: usbhid: remove #ifdef CONFIG_PM
https://git.kernel.org/hid/hid/c/f354872108eb
[3/4] HID: multitouch: remove #ifdef CONFIG_PM
https://git.kernel.org/hid/hid/c/fc2543414c3e
[4/4] HID: rmi: remove #ifdef CONFIG_PM
https://git.kernel.org/hid/hid/c/eeebfe6259ba
Cheers,
--
Benjamin Tissoires <bentiss@kernel.org>
^ permalink raw reply
* Re: [PATCH] hid: lenovo: Resend all settings on reset_resume for compact keyboards
From: Benjamin Tissoires @ 2023-10-25 19:01 UTC (permalink / raw)
To: jikos, benjamin.tissoires, jm, linux-kernel, Martin Kepplinger
Cc: linux-input, stable
In-Reply-To: <20231002150914.22101-1-martink@posteo.de>
On Mon, 02 Oct 2023 15:09:14 +0000, Martin Kepplinger wrote:
> The USB Compact Keyboard variant requires a reset_resume function to
> restore keyboard configuration after a suspend in some situations. Move
> configuration normally done on probe to lenovo_features_set_cptkbd(), then
> recycle this for use on reset_resume.
>
> Without, the keyboard and driver would end up in an inconsistent state,
> breaking middle-button scrolling amongst other problems, and twiddling
> sysfs values wouldn't help as the middle-button mode won't be set until
> the driver is reloaded.
>
> [...]
Applied to hid/hid.git (for-6.7/lenovo), thanks!
[1/1] hid: lenovo: Resend all settings on reset_resume for compact keyboards
https://git.kernel.org/hid/hid/c/2f2bd7cbd1d1
Cheers,
--
Benjamin Tissoires <bentiss@kernel.org>
^ permalink raw reply
* Re: [PATCH v3 01/12] HID: logitech-hidpp: Don't restart IO, instead defer hid_connect() only
From: Benjamin Tissoires @ 2023-10-25 16:43 UTC (permalink / raw)
To: Hans de Goede
Cc: Filipe Laíns, Bastien Nocera, Jiri Kosina,
Benjamin Tissoires, linux-input
In-Reply-To: <ha4irm6pz34zepdfpvs3vuo6ffvalkvfp4ase3githdtn4qlrz@tgl4lwzcpdee>
On Oct 18 2023, Benjamin Tissoires wrote:
> Hi Hans,
>
> FWIW, your series looks good, but I came accross a small nitpick here:
>
> On Oct 10 2023, Hans de Goede wrote:
> > Restarting IO causes 2 problems:
> >
> > 1. Some devices do not like IO being restarted this was addressed in
> > commit 498ba2069035 ("HID: logitech-hidpp: Don't restart communication
> > if not necessary"), but that change has issues of its own and needs to
> > be reverted.
> >
> > 2. Restarting IO and specifically calling hid_device_io_stop() causes
> > received packets to be missed, which may cause connect-events to
> > get missed.
> >
> > Restarting IO was introduced in commit 91cf9a98ae41 ("HID: logitech-hidpp:
> > make .probe usbhid capable") to allow to retrieve the device's name and
> > serial number and store these in hdev->name and hdev->uniq before
> > connecting any hid subdrivers (hid-input, hidraw) exporting this info
> > to userspace.
> >
> > But this does not require restarting IO, this merely requires deferring
> > calling hid_connect(). Calling hid_hw_start() with a connect-mask of
> > 0 makes it skip calling hid_connect(), so hidpp_probe() can simply call
> > hid_connect() later without needing to restart IO.
> >
> > Remove the stop + restart of IO and instead just call hid_connect() later
> > to avoid the issues caused by restarting IO.
> >
> > Now that IO is no longer stopped, hid_hw_close() must be called at the end
> > of probe() to balance the hid_hw_open() done at the beginning probe().
> >
> > This series has been tested on the following devices:
> > Logitech Bluetooth Laser Travel Mouse (bluetooth, HID++ 1.0)
> > Logitech M720 Triathlon (bluetooth, HID++ 4.5)
> > Logitech M720 Triathlon (unifying, HID++ 4.5)
> > Logitech K400 Pro (unifying, HID++ 4.1)
> > Logitech K270 (eQUAD nano Lite, HID++ 2.0)
> > Logitech M185 (eQUAD nano Lite, HID++ 4.5)
> > Logitech LX501 keyboard (27 Mhz, HID++ builtin scroll-wheel, HID++ 1.0)
> > Logitech M-RAZ105 mouse (27 Mhz, HID++ extra mouse buttons, HID++ 1.0)
> >
> > Fixes: 498ba2069035 ("HID: logitech-hidpp: Don't restart communication if not necessary")
> > Suggested-by: Benjamin Tissoires <bentiss@kernel.org>
> > Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> > ---
> > drivers/hid/hid-logitech-hidpp.c | 22 ++++++++++++----------
> > 1 file changed, 12 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
> > index a209d51bd247..aa4f232c4518 100644
> > --- a/drivers/hid/hid-logitech-hidpp.c
> > +++ b/drivers/hid/hid-logitech-hidpp.c
> > @@ -4460,8 +4460,10 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
> > hdev->name);
> >
> > /*
> > - * Plain USB connections need to actually call start and open
> > - * on the transport driver to allow incoming data.
> > + * First call hid_hw_start(hdev, 0) to allow IO without connecting any
> > + * hid subdrivers (hid-input, hidraw). This allows retrieving the dev's
> > + * name and serial number and store these in hdev->name and hdev->uniq,
> > + * before the hid-input and hidraw drivers expose these to userspace.
> > */
> > ret = hid_hw_start(hdev, will_restart ? 0 : connect_mask);
> > if (ret) {
> > @@ -4519,19 +4521,14 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
> > flush_work(&hidpp->work);
> >
> > if (will_restart) {
> > - /* Reset the HID node state */
> > - hid_device_io_stop(hdev);
> > - hid_hw_close(hdev);
> > - hid_hw_stop(hdev);
> > -
> > if (hidpp->quirks & HIDPP_QUIRK_DELAYED_INIT)
> > connect_mask &= ~HID_CONNECT_HIDINPUT;
> >
> > /* Now export the actual inputs and hidraw nodes to the world */
> > - ret = hid_hw_start(hdev, connect_mask);
> > + ret = hid_connect(hdev, connect_mask);
>
> On plain USB devices, we get a new warning here "io already started".
>
> This is because hid_connect() will call hid_pidff_init() from
> drivers/hid/usbhid/hid-pidff.c if connect_mask has the `HID_CONNECT_FF`
> bit set.
>
> And hid_pidff_init() blindly calls hid_device_io_start() then
> hid_device_io_stop().
>
> It's not a big issue, but still it's a new warning we have to tackly on.
>
> I see 3 possible solutions:
> - teach hid_pidff_init() to only start/stop the IOs if it's not already
> done so
> - if a device is actually connected through USB, call
> hid_device_io_stop() before hid_connect()
> - unconditionally call hid_device_io_stop() before hid_connect()
>
> The latter has my current preference as we won't get biten in the future
> if something else decides to change the io state.
>
> However, do you thing it'll be an issue to disable IOs there?
> And maybe we should re-enable them after?
>
> If it's fine to simply disable the IOs, we can add an extra patch on top
> of this series to fix that warning in the USB case.
>
> As mentioned above, I have tested with the T650, T651 that were likely to
> be a problem, and they are working just fine :)
>
> So with that minor fix, we should be able to stage this series.
The merge window is coming very soon. So I'm taking this series as it is
(I just added the few devices I made the tests), and we can work on an
extra patch to remove that warning.
Cheers,
Benjamin
>
> Thanks again for your hard work
>
> Cheers,
> Benjamin
>
> > if (ret) {
> > - hid_err(hdev, "%s:hid_hw_start returned error\n", __func__);
> > - goto hid_hw_start_fail;
> > + hid_err(hdev, "%s:hid_connect returned error %d\n", __func__, ret);
> > + goto hid_hw_init_fail;
> > }
> > }
> >
> > @@ -4543,6 +4540,11 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
> > ret);
> > }
> >
> > + /*
> > + * This relies on logi_dj_ll_close() being a no-op so that DJ connection
> > + * events will still be received.
> > + */
> > + hid_hw_close(hdev);
> > return ret;
> >
> > hid_hw_init_fail:
> > --
> > 2.41.0
> >
^ permalink raw reply
* Re: [PATCH v2 0/2] HID: uclogic: Fix two bugs in uclogic
From: Benjamin Tissoires @ 2023-10-25 16:03 UTC (permalink / raw)
To: jikos, benjamin.tissoires, linux-input, linux-kernel,
jose.exposito89, Jinjie Ruan
In-Reply-To: <20231009064245.3573397-1-ruanjinjie@huawei.com>
On Mon, 09 Oct 2023 14:42:43 +0800, Jinjie Ruan wrote:
> When CONFIG_HID_UCLOGIC=y and CONFIG_KUNIT_ALL_TESTS=y, launch
> kernel and then there are a user-memory-access bug and a work->entry
> not empty bug. This patchset fix these issues.
>
> Changes in v2:
> - Use kunit_kzalloc() instead of kzalloc().
> - Add KUNIT_ASSERT_NOT_ERR_OR_NULL() for kunit_kzalloc().
> - Add Reviewed-by.
>
> [...]
Applied to https://git.kernel.org/pub/scm/linux/kernel/git/hid/hid.git (for-6.7/uclogic), thanks!
[1/2] HID: uclogic: Fix user-memory-access bug in uclogic_params_ugee_v2_init_event_hooks()
https://git.kernel.org/hid/hid/c/91cfe0bbaa1c
[2/2] HID: uclogic: Fix a work->entry not empty bug in __queue_work()
https://git.kernel.org/hid/hid/c/d45f72b3c275
Cheers,
--
Benjamin Tissoires <bentiss@kernel.org>
^ permalink raw reply
* [PATCH] Add support for touch screens using the General Touch ST6001S controller.
From: Gareth Randall @ 2023-10-25 15:51 UTC (permalink / raw)
To: Benjamin Tissoires, Dmitry Torokhov; +Cc: linux-input
Add support for touch screens using the General Touch ST6001S controller,
as found in the GPEG model AOD22WZ-ST monitor. This controller can output
the ELO 10-byte protocol, but requires different initialisation.
Signed-off-by: Gareth Randall <gareth@garethrandall.com>
---
drivers/input/touchscreen/elo.c | 81 ++++++++++++++++++++++++++++++++-
1 file changed, 80 insertions(+), 1 deletion(-)
diff --git a/drivers/input/touchscreen/elo.c b/drivers/input/touchscreen/elo.c
index 96173232e53f..b233869ffa2a 100644
--- a/drivers/input/touchscreen/elo.c
+++ b/drivers/input/touchscreen/elo.c
@@ -26,6 +26,27 @@ MODULE_AUTHOR("Vojtech Pavlik <vojtech@ucw.cz>");
MODULE_DESCRIPTION(DRIVER_DESC);
MODULE_LICENSE("GPL");
+static uint gt_abs_x_min;
+module_param(gt_abs_x_min, uint, 0444);
+MODULE_PARM_DESC(gt_abs_x_min, "abs_x min value in General Touch mode (default: 0)");
+
+static uint gt_abs_x_max = 4095;
+module_param(gt_abs_x_max, uint, 0444);
+MODULE_PARM_DESC(gt_abs_x_max, "abs_x max value in General Touch mode (default: 4095)");
+
+static uint gt_abs_y_min;
+module_param(gt_abs_y_min, uint, 0444);
+MODULE_PARM_DESC(gt_abs_y_min, "abs_y min value in General Touch mode (default: 0)");
+
+static uint gt_abs_y_max = 4095;
+module_param(gt_abs_y_max, uint, 0444);
+MODULE_PARM_DESC(gt_abs_y_max, "abs_y max value in General Touch mode (default: 4095)");
+
+static bool gt_mode_override;
+module_param(gt_mode_override, bool, 0444);
+MODULE_PARM_DESC(gt_mode_override, "force the use of General Touch mode (default: false)");
+
+
/*
* Definitions & global arrays.
*/
@@ -44,6 +65,8 @@ MODULE_LICENSE("GPL");
#define ELO10_ACK_PACKET 'A'
#define ELI10_ID_PACKET 'I'
+#define ELO_GT_INIT_PACKET "\001XfE\r"
+
/*
* Per-touchscreen data.
*/
@@ -201,6 +224,7 @@ static irqreturn_t elo_interrupt(struct serio *serio,
switch (elo->id) {
case 0:
+ case 4:
elo_process_data_10(elo, data);
break;
@@ -255,6 +279,50 @@ static int elo_command_10(struct elo *elo, unsigned char *packet)
return rc;
}
+/*
+ * Initialise the General Touch ST6001S controller.
+ */
+static int elo_command_10_gt(struct elo *elo)
+{
+ int rc = -1;
+ int i;
+ unsigned char *packet = ELO_GT_INIT_PACKET;
+
+ mutex_lock(&elo->cmd_mutex);
+
+ serio_pause_rx(elo->serio);
+ init_completion(&elo->cmd_done);
+ serio_continue_rx(elo->serio);
+
+ for (i = 0; i < (int)strlen(packet); i++) {
+ if (serio_write(elo->serio, packet[i]))
+ goto out;
+ }
+
+ wait_for_completion_timeout(&elo->cmd_done, HZ);
+ rc = 0;
+
+ out:
+ mutex_unlock(&elo->cmd_mutex);
+ return rc;
+}
+
+static int elo_setup_10_gt(struct elo *elo)
+{
+ struct input_dev *dev = elo->dev;
+
+ if (elo_command_10_gt(elo))
+ return -EIO;
+
+ input_set_abs_params(dev, ABS_X, gt_abs_x_min, gt_abs_x_max, 0, 0);
+ input_set_abs_params(dev, ABS_Y, gt_abs_y_min, gt_abs_y_max, 0, 0);
+
+ dev_info(&elo->serio->dev,
+ "GeneralTouch ST6001S touchscreen");
+
+ return 0;
+}
+
static int elo_setup_10(struct elo *elo)
{
static const char *elo_types[] = { "Accu", "Dura", "Intelli", "Carroll" };
@@ -273,7 +341,7 @@ static int elo_setup_10(struct elo *elo)
dev_info(&elo->serio->dev,
"%sTouch touchscreen, fw: %02x.%02x, features: 0x%02x, controller: 0x%02x\n",
- elo_types[(packet[1] -'0') & 0x03],
+ elo_types[(packet[1] - '0') & 0x03],
packet[5], packet[4], packet[3], packet[7]);
return 0;
@@ -332,12 +400,16 @@ static int elo_connect(struct serio *serio, struct serio_driver *drv)
input_dev->evbit[0] = BIT_MASK(EV_KEY) | BIT_MASK(EV_ABS);
input_dev->keybit[BIT_WORD(BTN_TOUCH)] = BIT_MASK(BTN_TOUCH);
+ __set_bit(INPUT_PROP_DIRECT, input_dev->propbit);
serio_set_drvdata(serio, elo);
err = serio_open(serio, drv);
if (err)
goto fail2;
+ if (gt_mode_override)
+ elo->id = 4;
+
switch (elo->id) {
case 0: /* 10-byte protocol */
@@ -361,6 +433,13 @@ static int elo_connect(struct serio *serio, struct serio_driver *drv)
input_set_abs_params(input_dev, ABS_X, 0, 255, 0, 0);
input_set_abs_params(input_dev, ABS_Y, 0, 255, 0, 0);
break;
+
+ case 4: /* 10-byte protocol with General Touch initialisation */
+ if (elo_setup_10_gt(elo)) {
+ err = -EIO;
+ goto fail3;
+ }
+ break;
}
err = input_register_device(elo->dev);
--
2.27.0
^ permalink raw reply related
* Re: [PATCH 1/2] dt-bindings: touchscreen: Add Novatek NT519XX series bindings
From: Rob Herring @ 2023-10-25 14:25 UTC (permalink / raw)
To: Wei-Shih Lin
Cc: robh+dt, krzysztof.kozlowski+dt, linux-input, conor+dt,
devicetree, linux-kernel, dmitry.torokhov
In-Reply-To: <20231025082054.1190-2-Weishih_Lin@novatek.com.tw>
On Wed, 25 Oct 2023 16:20:53 +0800, Wei-Shih Lin wrote:
> This patch adds device tree bindings for Novatek NT519XX series
> touchscreen devices.
>
> Signed-off-by: Wei-Shih Lin <Weishih_Lin@novatek.com.tw>
> ---
> .../input/touchscreen/novatek,nt519xx.yaml | 60 +++++++++++++++++++
> MAINTAINERS | 9 +++
> 2 files changed, 69 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/input/touchscreen/novatek,nt519xx.yaml
>
My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):
yamllint warnings/errors:
dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/input/touchscreen/novatek,nt519xx.yaml: title: 'Novatek nt519xx touchscreen controller bindings' should not be valid under {'pattern': '([Bb]inding| [Ss]chema)'}
hint: Everything is a binding/schema, no need to say it. Describe what hardware the binding is for.
from schema $id: http://devicetree.org/meta-schemas/base.yaml#
Error: Documentation/devicetree/bindings/input/touchscreen/novatek,nt519xx.example.dts:25.40-41 syntax error
FATAL ERROR: Unable to parse input tree
make[2]: *** [scripts/Makefile.lib:419: Documentation/devicetree/bindings/input/touchscreen/novatek,nt519xx.example.dtb] Error 1
make[2]: *** Waiting for unfinished jobs....
make[1]: *** [/builds/robherring/dt-review-ci/linux/Makefile:1427: dt_binding_check] Error 2
make: *** [Makefile:234: __sub-make] Error 2
doc reference errors (make refcheckdocs):
See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20231025082054.1190-2-Weishih_Lin@novatek.com.tw
The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.
If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:
pip3 install dtschema --upgrade
Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.
^ permalink raw reply
* Re: [PATCH v2] hid: cp2112: Fix IRQ shutdown stopping polling for all IRQs on chip
From: Benjamin Tissoires @ 2023-10-25 14:17 UTC (permalink / raw)
To: jikos, benjamin.tissoires, andriy.shevchenko, Danny Kaehn
Cc: linux-input, ethan.twardy
In-Reply-To: <20231011182317.1053344-1-danny.kaehn@plexus.com>
On Wed, 11 Oct 2023 13:23:17 -0500, Danny Kaehn wrote:
> Previously cp2112_gpio_irq_shutdown() always cancelled the
> gpio_poll_worker, even if other IRQs were still active, and did not set
> the gpio_poll flag to false. This resulted in any call to _shutdown()
> resulting in interrupts no longer functioning on the chip until a
> _remove() occurred (a.e. the cp2112 is unplugged or system rebooted).
>
> Only cancel polling if all IRQs are disabled/masked, and correctly set
> the gpio_poll flag, allowing polling to restart when an interrupt is
> next enabled.
>
> [...]
Applied to hid/hid.git (for-6.7/cp2112), thanks!
[1/1] hid: cp2112: Fix IRQ shutdown stopping polling for all IRQs on chip
https://git.kernel.org/hid/hid/c/dc3115e6c5d9
Cheers,
--
Benjamin Tissoires <bentiss@kernel.org>
^ permalink raw reply
* Re: [PATCH v3 1/3] pwm: make it possible to apply pwm changes in atomic context
From: Uwe Kleine-König @ 2023-10-25 10:47 UTC (permalink / raw)
To: Sean Young
Cc: Daniel Thompson, Hans de Goede, linux-media, linux-pwm,
Ivaylo Dimitrov, Thierry Reding, Jonathan Corbet, Jani Nikula,
Joonas Lahtinen, Rodrigo Vivi, Tvrtko Ursulin, David Airlie,
Daniel Vetter, Javier Martinez Canillas, Jean Delvare,
Guenter Roeck, Support Opensource, Dmitry Torokhov, Pavel Machek,
Lee Jones, Mauro Carvalho Chehab, Ilpo Järvinen, Mark Gross,
Liam Girdwood, Mark Brown, Jingoo Han, Helge Deller, linux-doc,
linux-kernel, intel-gfx, dri-devel, linux-hwmon, linux-input,
linux-leds, platform-driver-x86, linux-arm-kernel, linux-fbdev
In-Reply-To: <ZTjll7oTNVWqygbD@gofer.mess.org>
[-- Attachment #1: Type: text/plain, Size: 3932 bytes --]
Hello,
On Wed, Oct 25, 2023 at 10:53:27AM +0100, Sean Young wrote:
> On Mon, Oct 23, 2023 at 02:34:17PM +0100, Daniel Thompson wrote:
> > On Sun, Oct 22, 2023 at 11:46:22AM +0100, Sean Young wrote:
> > > On Sat, Oct 21, 2023 at 11:08:22AM +0200, Hans de Goede wrote:
> > > > On 10/19/23 12:51, Uwe Kleine-König wrote:
> > > > > On Wed, Oct 18, 2023 at 03:57:48PM +0200, Hans de Goede wrote:
> > > > >> On 10/17/23 11:17, Sean Young wrote:
> > > > > I think it's very subjective if you consider this
> > > > > churn or not.
> > > >
> > > > I consider it churn because I don't think adding a postfix
> > > > for what is the default/expected behavior is a good idea
> > > > (with GPIOs not sleeping is the expected behavior).
> > > >
> > > > I agree that this is very subjective and very much goes
> > > > into the territory of bikeshedding. So please consider
> > > > the above my 2 cents on this and lets leave it at that.
> > >
> > > You have a valid point. Let's focus on having descriptive function names.
> >
> > For a couple of days I've been trying to resist the bikeshedding (esp.
> > given the changes to backlight are tiny) so I'll try to keep it as
> > brief as I can:
> >
> > 1. I dislike the do_it() and do_it_cansleep() pairing. It is
> > difficult to detect when a client driver calls do_it() by mistake.
> > In fact a latent bug of this nature can only be detected by runtime
> > testing with the small number of PWMs that do not support
> > configuration from an atomic context.
> >
> > In contrast do_it() and do_it_atomic()[*] means that although
> > incorrectly calling do_it() from an atomic context can be pretty
> > catastrophic it is also trivially detected (with any PWM driver)
> > simply by running with CONFIG_DEBUG_ATOMIC_SLEEP.
Wrongly calling the atomic variant (no matter how it's named) in a
context where sleeping is possible is only a minor issue. Being faster
than necessary is hardly a problem, so it only hurts by not being an
preemption point with PREEMPT_VOLUNTARY which might not even be relevant
because we're near to a system call anyhow.
For me the naming is only very loosely related to the possible bugs. I
think calling the wrong function happens mainly because the driver author
isn't aware in which context the call happens and not because of wrong
assumptions about the sleepiness of a certain function call.
If you consider this an argument however, do_it + do_it_cansleep is
better than do_it_atomic + do_it as wrongly assuming do_it would sleep
is less bad than wrongly assuming do_it wouldn't sleep. (The latter is
catched by CONFIG_DEBUG_ATOMIC_SLEEP, but only if it's enabled.)
Having said that while my subjective preference ordering is (with first
= best):
do_it + do_it_cansleep
do_it_atomic + do_it_cansleep
do_it_atomic + do_it
wi(th a _might_sleep or _mightsleep suffix ranging below _cansleep)
I wouldn't get sleepless nights when I get overruled here
(uwe_cansleep :-).
> > No objections (beyond churn) to fully spelt out pairings such as
> > do_it_cansleep() and do_it_atomic()[*]!
>
> I must say I do like the look of this. Uwe, how do you feel about:
> pwm_apply_cansleep() and pwm_apply_atomic()? I know we've talked about
> pwm_apply_atomic in the past, however I think this this the best
> option I've seen so far.
>
> > 2. If there is an API rename can we make sure the patch contains no
> > other changes (e.g. don't introduce any new API in the same patch).
> > Seperating renames makes the patches easier to review!
> > It makes each one smaller and easier to review!
>
> Yes, this should have been separated out. Will fix for next version.
+1
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* Re: [PATCH v5 2/2] input: joystick: driver for Adafruit Seesaw Gamepad
From: Anshul Dalal @ 2023-10-25 10:20 UTC (permalink / raw)
To: Jeff LaBundy
Cc: linux-input, devicetree, Dmitry Torokhov, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Thomas Weißschuh,
Shuah Khan, linux-kernel-mentees, linux-kernel
In-Reply-To: <ZTWza+S+t+UZKlwu@nixie71>
Hello Jeff,
Thanks for the review, I plan on addressing the changes you requested in
the next version of the patch. Though I had a few questions:
On 10/23/23 05:12, Jeff LaBundy wrote:
> Hi Anshul,
>
> On Tue, Oct 17, 2023 at 09:13:45AM +0530, Anshul Dalal wrote:
>> Adds a driver for a mini gamepad that communicates over i2c, the gamepad
>> has bidirectional thumb stick input and six buttons.
>>
>> The gamepad chip utilizes the open framework from Adafruit called 'Seesaw'
>> to transmit the ADC data for the joystick and digital pin state for the
>> buttons. I have only implemented the functionality required to receive the
>> thumb stick and button state.
>>
>> Steps in reading the gamepad state over i2c:
>> 1. Reset the registers
>> 2. Set the pin mode of the pins specified by the `BUTTON_MASK` to input
>> `BUTTON_MASK`: A bit-map for the six digital pins internally
>> connected to the joystick buttons.
>> 3. Enable internal pullup resistors for the `BUTTON_MASK`
>> 4. Bulk set the pin state HIGH for `BUTTON_MASK`
>> 5. Poll the device for button and joystick state done by:
>> `seesaw_read_data(struct i2c_client *client, struct seesaw_data *data)`
>>
>> Product page:
>> https://www.adafruit.com/product/5743
>> Arduino driver:
>> https://github.com/adafruit/Adafruit_Seesaw
>>
>> Driver tested on RPi Zero 2W
>>
>> Reviewed-by: Thomas Weißschuh <linux@weissschuh.net>
>> Signed-off-by: Anshul Dalal <anshulusr@gmail.com>
>> ---
>>
>> Changes for v5:
>> - Added link to the datasheet
>> - Added debug log message when `seesaw_read_data` fails
>>
>> Changes for v4:
>> - Changed `1UL << BUTTON_` to BIT(BUTTON_)
>> - Removed `hardware_id` field from `struct seesaw_gamepad`
>> - Removed redundant checks for the number of bytes written and received by
>> `i2c_master_send` and `i2c_master_recv`
>> - Used `get_unaligned_be32` to instantiate `u32 result` from `read_buf`
>> - Changed `result & (1UL << BUTTON_)` to
>> `test_bit(BUTTON_, (long *)&result)`
>> - Changed `KBUILD_MODNAME` in id-tables to `SEESAW_DEVICE_NAME`
>> - Fixed formatting issues
>> - Changed button reporting:
>> Since the gamepad had the action buttons in a non-standard layout:
>> (X)
>> (Y) (A)
>> (B)
>> Therefore moved to using generic directional action button event codes
>> instead of BTN_[ABXY].
>>
>> Changes for v3:
>> - no updates
>>
>> Changes for v2:
>> adafruit-seesaw.c:
>> - Renamed file from 'adafruit_seesaw.c'
>> - Changed device name from 'seesaw_gamepad' to 'seesaw-gamepad'
>> - Changed count parameter for receiving joystick x on line 118:
>> `2` to `sizeof(write_buf)`
>> - Fixed invalid buffer size on line 123 and 126:
>> `data->y` to `sizeof(data->y)`
>> - Added comment for the `mdelay(10)` on line 169
>> - Changed inconsistent indentation on line 271
>> Kconfig:
>> - Fixed indentation for the help text
>> - Updated module name
>> Makefile:
>> - Updated module object file name
>> MAINTAINERS:
>> - Updated file name for the driver and bindings
>>
>> MAINTAINERS | 7 +
>> drivers/input/joystick/Kconfig | 9 +
>> drivers/input/joystick/Makefile | 1 +
>> drivers/input/joystick/adafruit-seesaw.c | 273 +++++++++++++++++++++++
>> 4 files changed, 290 insertions(+)
>> create mode 100644 drivers/input/joystick/adafruit-seesaw.c
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 6c4cce45a09d..a314f9b48e21 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -441,6 +441,13 @@ W: http://wiki.analog.com/AD7879
>> W: https://ez.analog.com/linux-software-drivers
>> F: drivers/input/touchscreen/ad7879.c
>>
>> +ADAFRUIT MINI I2C GAMEPAD
>> +M: Anshul Dalal <anshulusr@gmail.com>
>> +L: linux-input@vger.kernel.org
>> +S: Maintained
>> +F: Documentation/devicetree/bindings/input/adafruit,seesaw-gamepad.yaml
>> +F: drivers/input/joystick/adafruit-seesaw.c
>> +
>> ADDRESS SPACE LAYOUT RANDOMIZATION (ASLR)
>> M: Jiri Kosina <jikos@kernel.org>
>> S: Maintained
>> diff --git a/drivers/input/joystick/Kconfig b/drivers/input/joystick/Kconfig
>> index ac6925ce8366..df9cd1830b29 100644
>> --- a/drivers/input/joystick/Kconfig
>> +++ b/drivers/input/joystick/Kconfig
>> @@ -412,4 +412,13 @@ config JOYSTICK_SENSEHAT
>> To compile this driver as a module, choose M here: the
>> module will be called sensehat_joystick.
>>
>> +config JOYSTICK_SEESAW
>> + tristate "Adafruit Mini I2C Gamepad with Seesaw"
>> + depends on I2C
>> + help
>> + Say Y here if you want to use the Adafruit Mini I2C Gamepad.
>> +
>> + To compile this driver as a module, choose M here: the module will be
>> + called adafruit-seesaw.
>> +
>> endif
>> diff --git a/drivers/input/joystick/Makefile b/drivers/input/joystick/Makefile
>> index 3937535f0098..9976f596a920 100644
>> --- a/drivers/input/joystick/Makefile
>> +++ b/drivers/input/joystick/Makefile
>> @@ -28,6 +28,7 @@ obj-$(CONFIG_JOYSTICK_N64) += n64joy.o
>> obj-$(CONFIG_JOYSTICK_PSXPAD_SPI) += psxpad-spi.o
>> obj-$(CONFIG_JOYSTICK_PXRC) += pxrc.o
>> obj-$(CONFIG_JOYSTICK_QWIIC) += qwiic-joystick.o
>> +obj-$(CONFIG_JOYSTICK_SEESAW) += adafruit-seesaw.o
>> obj-$(CONFIG_JOYSTICK_SENSEHAT) += sensehat-joystick.o
>> obj-$(CONFIG_JOYSTICK_SIDEWINDER) += sidewinder.o
>> obj-$(CONFIG_JOYSTICK_SPACEBALL) += spaceball.o
>> diff --git a/drivers/input/joystick/adafruit-seesaw.c b/drivers/input/joystick/adafruit-seesaw.c
>> new file mode 100644
>> index 000000000000..2a1eae8d2861
>> --- /dev/null
>> +++ b/drivers/input/joystick/adafruit-seesaw.c
>> @@ -0,0 +1,273 @@
>> +// SPDX-License-Identifier: GPL-2.0-or-later
>> +/*
>> + * Copyright (C) 2023 Anshul Dalal <anshulusr@gmail.com>
>> + *
>> + * Driver for Adafruit Mini I2C Gamepad
>> + *
>> + * Based on the work of:
>> + * Oleh Kravchenko (Sparkfun Qwiic Joystick driver)
>> + *
>> + * Datasheet: https://cdn-learn.adafruit.com/downloads/pdf/gamepad-qt.pdf
>> + * Product page: https://www.adafruit.com/product/5743
>> + * Firmware and hardware sources: https://github.com/adafruit/Adafruit_Seesaw
>> + */
>> +
>> +#include <asm-generic/unaligned.h>
>> +#include <linux/bits.h>
>> +#include <linux/delay.h>
>> +#include <linux/i2c.h>
>> +#include <linux/input.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +
>> +/* clang-format off */
>
> I don't think we need this directive; at least, no other input drivers have
> it, or really any drivers for that matter.
>
>> +#define SEESAW_DEVICE_NAME "seesaw-gamepad"
>> +
>> +#define SEESAW_STATUS_BASE 0
>> +#define SEESAW_GPIO_BASE 1
>> +#define SEESAW_ADC_BASE 9
>> +
>> +#define SEESAW_GPIO_DIRCLR_BULK 3
>> +#define SEESAW_GPIO_BULK 4
>> +#define SEESAW_GPIO_BULK_SET 5
>> +#define SEESAW_GPIO_PULLENSET 11
>> +
>> +#define SEESAW_STATUS_HW_ID 1
>> +#define SEESAW_STATUS_SWRST 127
>> +
>> +#define SEESAW_ADC_OFFSET 7
>> +
>> +#define BUTTON_A 5
>> +#define BUTTON_B 1
>> +#define BUTTON_X 6
>> +#define BUTTON_Y 2
>> +#define BUTTON_START 16
>> +#define BUTTON_SELECT 0
>
> Please namespace these (e.g. SEESAW_BUTTON_A) to make it clear they refer
> to device-specific bits and not standard keycodes (e.g. BTN_A). In fact,
> these seem better off as part of an array of structs; more on that below.
>
>> +
>> +#define ANALOG_X 14
>> +#define ANALOG_Y 15
>
> Please namespace these as well.
>
>> +
>> +#define SEESAW_JOYSTICK_MAX_AXIS 1023
>> +#define SEESAW_JOYSTICK_FUZZ 2
>> +#define SEESAW_JOYSTICK_FLAT 4
>> +
>> +#define SEESAW_GAMEPAD_POLL_INTERVAL 16
>> +#define SEESAW_GAMEPAD_POLL_MIN 8
>> +#define SEESAW_GAMEPAD_POLL_MAX 32
>> +/* clang-format on */
>> +
>> +u32 BUTTON_MASK = BIT(BUTTON_A) | BIT(BUTTON_B) | BIT(BUTTON_X) |
>> + BIT(BUTTON_Y) | BIT(BUTTON_START) | BIT(BUTTON_SELECT);
>> +
>> +struct seesaw_gamepad {
>> + char physical_path[32];
>> + struct input_dev *input_dev;
>> + struct i2c_client *i2c_client;
>> +};
>> +
>> +struct seesaw_data {
>> + __be16 x;
>> + __be16 y;
>> + u8 button_a, button_b, button_x, button_y, button_start, button_select;
>
> Please keep these each on a separate line.
>
>> +};
>
> Please declare this struct as __packed, as that is how it appears to be used.
>
>> +
>> +static int seesaw_read_data(struct i2c_client *client, struct seesaw_data *data)
>> +{
>> + int err;
>
> Please use 'ret' for return variables that can indicate a positive value on success.
>
>> + unsigned char write_buf[2] = { SEESAW_GPIO_BASE, SEESAW_GPIO_BULK };
>> + unsigned char read_buf[4];
>
> Please use standard kernel type definitions (i.e. u8 in this case).
>
>> +
>> + err = i2c_master_send(client, write_buf, sizeof(write_buf));
>> + if (err < 0)
>> + return err;
>
> You correctly return err (or rather, ret) for negative values, but you should also
> check that ret matches the size of the data sent. For 0 <= ret < sizeof(writebuf),
> return -EIO.
>
>> + err = i2c_master_recv(client, read_buf, sizeof(read_buf));
>> + if (err < 0)
>> + return err;
>
> And here.
>
>> +
>> + u32 result = get_unaligned_be32(&read_buf);
>
> Please do not mix declarations and code; all declarations must be at the
> top of the function.
>
>> +
>> + data->button_a = !test_bit(BUTTON_A, (long *)&result);
>> + data->button_b = !test_bit(BUTTON_B, (long *)&result);
>> + data->button_x = !test_bit(BUTTON_X, (long *)&result);
>> + data->button_y = !test_bit(BUTTON_Y, (long *)&result);
>> + data->button_start = !test_bit(BUTTON_START, (long *)&result);
>> + data->button_select = !test_bit(BUTTON_SELECT, (long *)&result);
>> +
>> + write_buf[0] = SEESAW_ADC_BASE;
>> + write_buf[1] = SEESAW_ADC_OFFSET + ANALOG_X;
>> + err = i2c_master_send(client, write_buf, sizeof(write_buf));
>> + if (err < 0)
>> + return err;
>> + err = i2c_master_recv(client, (char *)&data->x, sizeof(data->x));
>> + if (err < 0)
>> + return err;
>
> This is starting to look like a 16-bit register map. To that end, please
> consider using regmap instead of open-coding each of these standard write-
> then-read operations.
>
> Using regmap would also save you the trouble of managing the endianness
> yourself, as well as having to check for incomplete transfers since its
> functions return zero or a negative error code only.
>
In this driver there are only two places a 16-bit regmap could be used,
for getting the joystick X and Y values. I see minimal utility in adding
the boilerplate necessary to use the more sophisticated regmap API in
this case.
As for the handling of endianness, if I am not mistaken the
`be16_to_cpu` macro should manage it.
If you prefer I could add the following function to reduce code duplication:
int seesaw_get_analog(int pin) {
__be16 result;
u8 write_buf[2] = { SEESAW_ADC_BASE, SEESAW_ADC_OFFSET + pin };
int ret;
ret = i2c_master_send(client, write_buf, sizeof(write_buf));
if (ret < 0)
return ret;
ret = i2c_master_recv(client, (char *)&result, sizeof(result));
if (ret < 0)
return ret;
return result;
}
>> + /*
>> + * ADC reads left as max and right as 0, must be reversed since kernel
>> + * expects reports in opposite order.
>> + */
>> + data->x = SEESAW_JOYSTICK_MAX_AXIS - be16_to_cpu(data->x);
>> +
>> + write_buf[1] = SEESAW_ADC_OFFSET + ANALOG_Y;
>> + err = i2c_master_send(client, write_buf, sizeof(write_buf));
>> + if (err < 0)
>> + return err;
>> + err = i2c_master_recv(client, (char *)&data->y, sizeof(data->y));
>> + if (err < 0)
>> + return err;
>> + data->y = be16_to_cpu(data->y);
>> +
>> + return 0;
>> +}
>> +
>> +static void seesaw_poll(struct input_dev *input)
>> +{
>> + struct seesaw_gamepad *private = input_get_drvdata(input);
>> + struct seesaw_data data;
>> + int err;
>> +
>> + err = seesaw_read_data(private->i2c_client, &data);
>> + if (err != 0) {
>> + dev_dbg(&input->dev, "failed to read joystick state: %d\n",
>> + err);
>
> This should be dev_err_ratelimited().
>
>> + return;
>> + }
>> +
>> + input_report_abs(input, ABS_X, data.x);
>> + input_report_abs(input, ABS_Y, data.y);
>> + input_report_key(input, BTN_EAST, data.button_a);
>> + input_report_key(input, BTN_SOUTH, data.button_b);
>> + input_report_key(input, BTN_NORTH, data.button_x);
>> + input_report_key(input, BTN_WEST, data.button_y);
>> + input_report_key(input, BTN_START, data.button_start);
>> + input_report_key(input, BTN_SELECT, data.button_select);
>
> I think you can make this much cleaner and smaller by defining an array
> of structs, each with a key code and bit position. You can then simply
> iterate over the array and call input_report_key() once per element as
> in the following:
>
> struct seesaw_btn_desc {
> unsigned int code;
> unsigned int shift;
> };
>
> static const struct seesaw_btn_desc seesaw_btns[] = {
> {
> .code = BTN_EAST,
> .mask = 5,
> },
> [...]
> };
>
> And then:
>
> btn_status = ...;
>
> for (i = 0; i < ARRAY_SIZE(seesaw_btns); i++)
> input_report_key(input, seesaw_btns[i].code,
> btn_status & seesaw_btns[i].mask);
>
> This would also make it easier to quickly discern what keycodes are mapped
> to which bits in the register.
>
>> + input_sync(input);
>> +}
>> +
>> +static int seesaw_probe(struct i2c_client *client)
>> +{
>> + int err;
>> + struct seesaw_gamepad *private;
>
> I'd rather this be called something like 'seesaw' rather than private.
>
>> + unsigned char register_reset[] = { SEESAW_STATUS_BASE,
>> + SEESAW_STATUS_SWRST, 0xFF };
>> + unsigned char get_hw_id[] = { SEESAW_STATUS_BASE, SEESAW_STATUS_HW_ID };
>> +
>> + err = i2c_master_send(client, register_reset, sizeof(register_reset));
>> + if (err < 0)
>> + return err;
>> +
>> + /* Wait for the registers to reset before proceeding */
>> + mdelay(10);
>> +
>> + private = devm_kzalloc(&client->dev, sizeof(*private), GFP_KERNEL);
>> + if (!private)
>> + return -ENOMEM;
>> +
>> + err = i2c_master_send(client, get_hw_id, sizeof(get_hw_id));
>> + if (err < 0)
>> + return err;
>> +
>> + unsigned char hardware_id;
>
> Same comment as earlier with regard to mixed declarations.
>
>> +
>> + err = i2c_master_recv(client, &hardware_id, 1);
>> + if (err < 0)
>> + return err;
>> +
>> + dev_dbg(&client->dev, "Adafruit Seesaw Gamepad, Hardware ID: %02x\n",
>> + hardware_id);
>> +
>> + private->i2c_client = client;
>> + scnprintf(private->physical_path, sizeof(private->physical_path),
>> + "i2c/%s", dev_name(&client->dev));
>
> This seems overly complex; can we not simply set input_dev->phys to the
> literal "i2c/seesaw-gamepad"? Why to copy at runtime and incur the cost
> of carrying 'physical_path' throughout the life of the module?
>
>> + i2c_set_clientdata(client, private);
>> +
>> + private->input_dev = devm_input_allocate_device(&client->dev);
>> + if (!private->input_dev)
>> + return -ENOMEM;
>> +
>> + private->input_dev->id.bustype = BUS_I2C;
>> + private->input_dev->name = "Adafruit Seesaw Gamepad";
>> + private->input_dev->phys = private->physical_path;
>> + input_set_drvdata(private->input_dev, private);
>> + input_set_abs_params(private->input_dev, ABS_X, 0,
>> + SEESAW_JOYSTICK_MAX_AXIS, SEESAW_JOYSTICK_FUZZ,
>> + SEESAW_JOYSTICK_FLAT);
>> + input_set_abs_params(private->input_dev, ABS_Y, 0,
>> + SEESAW_JOYSTICK_MAX_AXIS, SEESAW_JOYSTICK_FUZZ,
>> + SEESAW_JOYSTICK_FLAT);
>> + input_set_capability(private->input_dev, EV_KEY, BTN_EAST);
>> + input_set_capability(private->input_dev, EV_KEY, BTN_SOUTH);
>> + input_set_capability(private->input_dev, EV_KEY, BTN_NORTH);
>> + input_set_capability(private->input_dev, EV_KEY, BTN_WEST);
>> + input_set_capability(private->input_dev, EV_KEY, BTN_START);
>> + input_set_capability(private->input_dev, EV_KEY, BTN_SELECT);
>
> Same comment with regard to creating an array of structs, and hence only
> having to call input_set_capability() from within a small loop.
>
>> +
>> + err = input_setup_polling(private->input_dev, seesaw_poll);
>> + if (err) {
>> + dev_err(&client->dev, "failed to set up polling: %d\n", err);
>> + return err;
>> + }
>> +
>> + input_set_poll_interval(private->input_dev,
>> + SEESAW_GAMEPAD_POLL_INTERVAL);
>> + input_set_max_poll_interval(private->input_dev,
>> + SEESAW_GAMEPAD_POLL_MAX);
>> + input_set_min_poll_interval(private->input_dev,
>> + SEESAW_GAMEPAD_POLL_MIN);
>> +
>> + err = input_register_device(private->input_dev);
>> + if (err) {
>> + dev_err(&client->dev, "failed to register joystick: %d\n", err);
>> + return err;
>> + }
>> +
>> + /* Set Pin Mode to input and enable pull-up resistors */
>> + unsigned char pin_mode[] = { SEESAW_GPIO_BASE, SEESAW_GPIO_DIRCLR_BULK,
>> + BUTTON_MASK >> 24, BUTTON_MASK >> 16,
>> + BUTTON_MASK >> 8, BUTTON_MASK };
>> + err = i2c_master_send(client, pin_mode, sizeof(pin_mode));
>> + if (err < 0)
>> + return err;
>> + pin_mode[1] = SEESAW_GPIO_PULLENSET;
>> + err = i2c_master_send(client, pin_mode, sizeof(pin_mode));
>> + if (err < 0)
>> + return err;
>> + pin_mode[1] = SEESAW_GPIO_BULK_SET;
>> + err = i2c_master_send(client, pin_mode, sizeof(pin_mode));
>> + if (err < 0)
>> + return err;
>
> Please configure the HW before the input device is live and being polled.
>
Could you elaborate on what you meant by this. To my knowledge, the
device is ready to be polled right after the pin state for the
`BUTTON_MASK` is configured. That is also how it's done in the Arduino
driver provided by the manufacturer. Please clarify if I'm missing
something here.
>> +
>> + return 0;
>> +}
>> +
>> +#ifdef CONFIG_OF
>> +static const struct of_device_id of_seesaw_match[] = {
>> + {
>> + .compatible = "adafruit,seesaw-gamepad",
>> + },
>> + { /* Sentinel */ }
>> +};
>> +MODULE_DEVICE_TABLE(of, of_seesaw_match);
>> +#endif /* CONFIG_OF */
>
> Please correct me if I am wrong, but it does not seem that OF support is
> required by this driver. There are no properties beyond the standard ones
> understood by the I2C core, which can match based on the ID table below.
>
>> +
>> +/* clang-format off */
>> +static const struct i2c_device_id seesaw_id_table[] = {
>> + { SEESAW_DEVICE_NAME, 0 },
>> + { /* Sentinel */ }
>> +};
>> +/* clang-format on */
>
> Again, I don't see any need for these directives.
>
>> +
>
> Nit: unnecessary NL.
>
>> +MODULE_DEVICE_TABLE(i2c, seesaw_id_table);
>> +
>> +static struct i2c_driver seesaw_driver = {
>> + .driver = {
>> + .name = SEESAW_DEVICE_NAME,
>> + .of_match_table = of_match_ptr(of_seesaw_match),
>> + },
>> + .id_table = seesaw_id_table,
>> + .probe = seesaw_probe,
>> +};
>> +module_i2c_driver(seesaw_driver);
>> +
>> +MODULE_AUTHOR("Anshul Dalal <anshulusr@gmail.com>");
>> +MODULE_DESCRIPTION("Adafruit Mini I2C Gamepad driver");
>> +MODULE_LICENSE("GPL");
>> --
>> 2.42.0
>>
>
> Kind regards,
> Jeff LaBundy
Regards,
Anshul Dalal
^ permalink raw reply
* Re: [RESEND PATCH v6 3/3] input: pm8xxx-vibrator: add new SPMI vibrator support
From: Fenglin Wu @ 2023-10-25 9:54 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Dmitry Baryshkov, linux-arm-msm, linux-kernel,
krzysztof.kozlowski+dt, robh+dt, agross, andersson, Konrad Dybcio,
linux-input, quic_collinsd, quic_subbaram, quic_kamalw, jestar,
Luca Weiss
In-Reply-To: <8697d115-9aa7-2a1c-4d96-25b15adb5cca@quicinc.com>
On 10/9/2023 12:01 PM, Fenglin Wu wrote:
>
>
> On 10/1/2023 12:17 AM, Dmitry Torokhov wrote:
>> On Mon, Sep 25, 2023 at 10:54:45AM +0800, Fenglin Wu wrote:
>>>
>>>
>>> On 9/24/2023 3:07 AM, Dmitry Baryshkov wrote:
>>>>> +
>>>>> + switch (vib->data->hw_type) {
>>>>> + case SSBI_VIB:
>>>>> mask = SSBI_VIB_DRV_LEVEL_MASK;
>>>>> shift = SSBI_VIB_DRV_SHIFT;
>>>>> + break;
>>>>> + case SPMI_VIB:
>>>>> + mask = SPMI_VIB_DRV_LEVEL_MASK;
>>>>> + shift = SPMI_VIB_DRV_SHIFT;
>>>>> + break;
>>>>> + case SPMI_VIB_GEN2:
>>>>> + mask = SPMI_VIB_GEN2_DRV_MASK;
>>>>> + shift = SPMI_VIB_GEN2_DRV_SHIFT;
>>>>> + break;
>>>>> + default:
>>>>> + return -EINVAL;
>>>> Could you please move the switch to the previous patch? Then it would
>>>> be more obvious that you are just adding the SPMI_VIB_GEN2 here.
>>>>
>>>> Other than that LGTM.
>>>
>>> Sure, I can move the switch to the previous refactoring patch.
>>
>> Actually, the idea of having a const "reg" or "chip", etc. structure is
>> to avoid this kind of runtime checks based on hardware type and instead
>> use common computation. I believe you need to move mask and shift into
>> the chip-specific structure and avoid defining hw_type.
>>
>
> Actually, the main motivation for adding 'hw_type' is to avoid reading
> 'reg_base' from DT for SSBI_VIB. It can also help to simplify the
> 'pm8xxx_vib_data' structure and make following code logic more
> straightforward and easier to understand(check hw_type instead of
> checking specific constant reg/mask value), it has been used in
> following places:
>
> 1) Avoid reading 'reg_base' from DT for SSBI_VIB.
> 2) Only do manual-mode-mask-write for SSBI_VIB. Previously, it was
> achieved by giving a valid 'drv_en_manual_mask' value only for SSBI_VIB,
> having hw_type make it more straightforward.
> 3) Not writing VIB_EN register for SSBI_VIB. A similar strategy was
> used previously, only write VIB_EN register when 'enable_mask' is valid,
> checking hw_type make it more straightforward.
> 4) To achieve different drive step size for SPMI_VIB （100mV per
> step) and SPMI_VIB_GEN2 (1mV per step).
> 5) Do different VIB_DRV mask and shift assignment for SPMI_VIB and
> SPMI_VIB_GEN2
> 6) Only write VIB_DRV2 for SPMI_VIB_GEN2.
>
Hi Dmitry,
Can you please help to comment if this looks good for you?
I actually have pushed a V7 to address your last comment before you made
this one.
V7 change:
https://lore.kernel.org/linux-arm-msm/20230927-pm8xxx-vibrator-v7-1-b5d8c92ce818@quicinc.com/,
just want to know how to move forward.
Thanks
Fenglin
>
>> Thanks.
>>
^ permalink raw reply
* Re: [PATCH v3 1/3] pwm: make it possible to apply pwm changes in atomic context
From: Sean Young @ 2023-10-25 9:53 UTC (permalink / raw)
To: Daniel Thompson
Cc: Hans de Goede, Uwe Kleine-König, linux-media, linux-pwm,
Ivaylo Dimitrov, Thierry Reding, Jonathan Corbet, Jani Nikula,
Joonas Lahtinen, Rodrigo Vivi, Tvrtko Ursulin, David Airlie,
Daniel Vetter, Javier Martinez Canillas, Jean Delvare,
Guenter Roeck, Support Opensource, Dmitry Torokhov, Pavel Machek,
Lee Jones, Mauro Carvalho Chehab, Ilpo Järvinen, Mark Gross,
Liam Girdwood, Mark Brown, Jingoo Han, Helge Deller, linux-doc,
linux-kernel, intel-gfx, dri-devel, linux-hwmon, linux-input,
linux-leds, platform-driver-x86, linux-arm-kernel, linux-fbdev
In-Reply-To: <20231023133417.GE49511@aspen.lan>
On Mon, Oct 23, 2023 at 02:34:17PM +0100, Daniel Thompson wrote:
> On Sun, Oct 22, 2023 at 11:46:22AM +0100, Sean Young wrote:
> > On Sat, Oct 21, 2023 at 11:08:22AM +0200, Hans de Goede wrote:
> > > On 10/19/23 12:51, Uwe Kleine-König wrote:
> > > > On Wed, Oct 18, 2023 at 03:57:48PM +0200, Hans de Goede wrote:
> > > >> On 10/17/23 11:17, Sean Young wrote:
> > > > I think it's very subjective if you consider this
> > > > churn or not.
> > >
> > > I consider it churn because I don't think adding a postfix
> > > for what is the default/expected behavior is a good idea
> > > (with GPIOs not sleeping is the expected behavior).
> > >
> > > I agree that this is very subjective and very much goes
> > > into the territory of bikeshedding. So please consider
> > > the above my 2 cents on this and lets leave it at that.
> >
> > You have a valid point. Let's focus on having descriptive function names.
>
> For a couple of days I've been trying to resist the bikeshedding (esp.
> given the changes to backlight are tiny) so I'll try to keep it as
> brief as I can:
>
> 1. I dislike the do_it() and do_it_cansleep() pairing. It is
> difficult to detect when a client driver calls do_it() by mistake.
> In fact a latent bug of this nature can only be detected by runtime
> testing with the small number of PWMs that do not support
> configuration from an atomic context.
>
> In contrast do_it() and do_it_atomic()[*] means that although
> incorrectly calling do_it() from an atomic context can be pretty
> catastrophic it is also trivially detected (with any PWM driver)
> simply by running with CONFIG_DEBUG_ATOMIC_SLEEP.
>
> No objections (beyond churn) to fully spelt out pairings such as
> do_it_cansleep() and do_it_atomic()[*]!
I must say I do like the look of this. Uwe, how do you feel about:
pwm_apply_cansleep() and pwm_apply_atomic()? I know we've talked about
pwm_apply_atomic in the past, however I think this this the best
option I've seen so far.
> 2. If there is an API rename can we make sure the patch contains no
> other changes (e.g. don't introduce any new API in the same patch).
> Seperating renames makes the patches easier to review!
> It makes each one smaller and easier to review!
Yes, this should have been separated out. Will fix for next version.
Thanks,
Sean
^ permalink raw reply
* Re: [PATCH v4 3/4] mfd: cs40l50: Add support for CS40L50 core driver
From: Lee Jones @ 2023-10-25 9:26 UTC (permalink / raw)
To: Jeff LaBundy
Cc: James Ogletree, James Ogletree, Dmitry Torokhov, Rob Herring,
Krzysztof Kozlowski, Fred Treven, Ben Bright,
linux-input@vger.kernel.org, devicetree@vger.kernel.org,
linux-kernel@vger.kernel.org
In-Reply-To: <ZTiJYxAvqfBMMJ1S@nixie71>
On Tue, 24 Oct 2023, Jeff LaBundy wrote:
> Hi James,
>
> Just a few trailing comments on top of Lee's feedback.
>
> On Wed, Oct 18, 2023 at 05:57:24PM +0000, James Ogletree wrote:
> > From: James Ogletree <james.ogletree@cirrus.com>
> >
> > Introduce support for Cirrus Logic Device CS40L50: a
> > haptic driver with waveform memory, integrated DSP,
> > and closed-loop algorithms.
> >
> > The MFD component registers and initializes the device.
> >
> > Signed-off-by: James Ogletree <james.ogletree@cirrus.com>
> > ---
> > MAINTAINERS | 2 +
> > drivers/mfd/Kconfig | 30 +++
> > drivers/mfd/Makefile | 4 +
> > drivers/mfd/cs40l50-core.c | 443 ++++++++++++++++++++++++++++++++++++
> > drivers/mfd/cs40l50-i2c.c | 69 ++++++
> > drivers/mfd/cs40l50-spi.c | 68 ++++++
> > include/linux/mfd/cs40l50.h | 198 ++++++++++++++++
> > 7 files changed, 814 insertions(+)
> > create mode 100644 drivers/mfd/cs40l50-core.c
> > create mode 100644 drivers/mfd/cs40l50-i2c.c
> > create mode 100644 drivers/mfd/cs40l50-spi.c
> > create mode 100644 include/linux/mfd/cs40l50.h
Just popping along to say; these are excellent comments Jeff.
Thank you for your review.
--
Lee Jones [李琼斯]
^ permalink raw reply
* [PATCH 2/2] Input: Add driver for Novatek NT519XX series touchscreen devices
From: Wei-Shih Lin @ 2023-10-25 8:20 UTC (permalink / raw)
To: dmitry.torokhov, robh+dt, krzysztof.kozlowski+dt, conor+dt
Cc: linux-input, devicetree, linux-kernel
In-Reply-To: <20231025082054.1190-1-Weishih_Lin@novatek.com.tw>
This patch adds support for Novatek NT519XX series touchscreen devices.
Existing Novatek touchscreen driver code developed for Acer Iconia One 7
B1-750 tablet with Novatek IC NT11205 is novatek-nvt-ts.c in the path
drivers/input/touchscreen/. However, this patch supports touch features
for automotive display with Novatek TDDI NT519XX.
Signed-off-by: Wei-Shih Lin <Weishih_Lin@novatek.com.tw>
---
drivers/input/touchscreen/Kconfig | 12 +
drivers/input/touchscreen/Makefile | 1 +
drivers/input/touchscreen/nt519xx.c | 995 ++++++++++++++++++++
drivers/input/touchscreen/nt519xx.h | 130 +++
drivers/input/touchscreen/nt519xx_mem_map.h | 262 ++++++
5 files changed, 1400 insertions(+)
create mode 100644 drivers/input/touchscreen/nt519xx.c
create mode 100644 drivers/input/touchscreen/nt519xx.h
create mode 100644 drivers/input/touchscreen/nt519xx_mem_map.h
diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
index c2cbd332af1d..6f84e31e7dbd 100644
--- a/drivers/input/touchscreen/Kconfig
+++ b/drivers/input/touchscreen/Kconfig
@@ -1389,4 +1389,16 @@ config TOUCHSCREEN_HIMAX_HX83112B
To compile this driver as a module, choose M here: the
module will be called himax_hx83112b.
+config TOUCHSCREEN_NT519XX
+ tristate "Novatek NT519XX based touchscreens"
+ depends on I2C
+ help
+ Say Y here if you have a Novatek NT519XX based touchscreen
+ controller.
+
+ If unsure, say N.
+
+ To compile this driver as a module, choose M here: the
+ module will be called nt519xx.
+
endif
diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile
index 159cd5136fdb..598d38e4e06a 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_NT519XX) += nt519xx.o
diff --git a/drivers/input/touchscreen/nt519xx.c b/drivers/input/touchscreen/nt519xx.c
new file mode 100644
index 000000000000..f3cc884c65e9
--- /dev/null
+++ b/drivers/input/touchscreen/nt519xx.c
@@ -0,0 +1,995 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Driver for Novatek NT519xx touchscreens
+ *
+ * Copyright (C) 2023 Wei-Shih Lin <Weishih_Lin@novatek.com.tw>
+ * Copyright (C) 2023 Leo LS Chang <Leo_LS_Chang@novatek.com.tw>
+ *
+ * Copyright (C) 2010 - 2023 Novatek, Inc.
+ *
+ */
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/interrupt.h>
+#include <linux/irq.h>
+#include <linux/gpio.h>
+#include <linux/proc_fs.h>
+#include <linux/input/mt.h>
+#include <linux/of_gpio.h>
+#include <linux/of_irq.h>
+
+#include <linux/notifier.h>
+#include <linux/fb.h>
+
+#include "nt519xx.h"
+
+struct nvt_ts_data *ts;
+
+static int nvt_fb_notifier_callback(struct notifier_block *self,
+ unsigned long event, void *data);
+
+static uint8_t bTouchIsAwake;
+static uint8_t debug_log;
+
+static void nvt_irq_enable(bool enable)
+{
+ struct irq_desc *desc;
+
+ if (enable) {
+ if (!ts->irq_enabled) {
+ enable_irq(ts->client->irq);
+ ts->irq_enabled = true;
+ }
+ } else {
+ if (ts->irq_enabled) {
+ disable_irq(ts->client->irq);
+ ts->irq_enabled = false;
+ }
+ }
+
+ desc = irq_to_desc(ts->client->irq);
+ NVT_LOG("enable=%d, desc->depth=%d\n", enable, desc->depth);
+}
+
+int32_t CTP_I2C_READ(struct i2c_client *client, uint16_t address, uint8_t *buf,
+ uint16_t len)
+{
+ struct i2c_msg msgs[2];
+ int32_t ret = -1;
+ int32_t retries = 0;
+
+ mutex_lock(&ts->xbuf_lock);
+
+ msgs[0].flags = !I2C_M_RD;
+ msgs[0].addr = address;
+ msgs[0].len = 1;
+ msgs[0].buf = &buf[0];
+
+ msgs[1].flags = I2C_M_RD;
+ msgs[1].addr = address;
+ msgs[1].len = len - 1;
+ msgs[1].buf = ts->xbuf;
+
+ while (retries < 5) {
+ ret = i2c_transfer(client->adapter, msgs, 2);
+ if (ret == 2)
+ break;
+ retries++;
+ }
+
+ if (unlikely(retries == 5)) {
+ NVT_ERR("ret=%d\n", ret);
+ ret = -EIO;
+ }
+
+ memcpy(buf + 1, ts->xbuf, len - 1);
+
+ mutex_unlock(&ts->xbuf_lock);
+
+ return ret;
+}
+
+int32_t CTP_I2C_WRITE(struct i2c_client *client, uint16_t address, uint8_t *buf,
+ uint16_t len)
+{
+ struct i2c_msg msg;
+ int32_t ret = -1;
+ int32_t retries = 0;
+
+ mutex_lock(&ts->xbuf_lock);
+
+ msg.flags = !I2C_M_RD;
+ msg.addr = address;
+ msg.len = len;
+ memcpy(ts->xbuf, buf, len);
+ msg.buf = ts->xbuf;
+
+ while (retries < 5) {
+ ret = i2c_transfer(client->adapter, &msg, 1);
+ if (ret == 1)
+ break;
+ retries++;
+ }
+
+ if (unlikely(retries == 5)) {
+ NVT_ERR("ret=%d\n", ret);
+ ret = -EIO;
+ }
+
+ mutex_unlock(&ts->xbuf_lock);
+
+ return ret;
+}
+
+int32_t nvt_set_page(uint16_t i2c_addr, uint32_t addr)
+{
+ uint8_t buf[4] = { 0 };
+
+ buf[0] = 0xFF; // novatek set page command
+ buf[1] = (addr >> 16) & 0xFF;
+ buf[2] = (addr >> 8) & 0xFF;
+
+ return CTP_I2C_WRITE(ts->client, i2c_addr, buf, 3);
+}
+
+int32_t nvt_write_addr(uint32_t addr, uint8_t data)
+{
+ int32_t ret = 0;
+ uint8_t buf[2] = { 0 };
+
+ ret = nvt_set_page(I2C_FW_ADDRESS, addr);
+ if (ret < 0) {
+ NVT_ERR("Set page 0x%06X failed! ret=%d\n", addr, ret);
+ return ret;
+ }
+
+ buf[0] = addr & 0xFF;
+ buf[1] = data;
+ ret = CTP_I2C_WRITE(ts->client, I2C_FW_ADDRESS, buf, 2);
+ if (ret < 0) {
+ NVT_ERR("Write data to 0x%06X failed! ret=%d\n", addr, ret);
+ return ret;
+ }
+ ret = 0;
+
+ return ret;
+}
+
+int32_t nvt_read_reg(struct nvt_ts_reg reg, uint8_t *val)
+{
+ int32_t ret = 0;
+ uint32_t addr = 0;
+ uint8_t bitmask = 0;
+ uint8_t shift = 0;
+ uint8_t buf[8] = { 0 };
+ uint8_t temp = 0;
+
+ addr = reg.addr;
+ bitmask = reg.bitmask;
+ temp = reg.bitmask;
+ shift = 0;
+ while (1) {
+ if ((temp >> shift) & 0x01)
+ break;
+ if (shift == 8) {
+ NVT_ERR("bitmask all bits zero!\n");
+ ret = -1;
+ break;
+ }
+ shift++;
+ }
+
+ nvt_set_page(I2C_FW_ADDRESS, addr);
+ buf[0] = addr & 0xFF;
+ buf[1] = 0x00;
+ ret = CTP_I2C_READ(ts->client, I2C_FW_ADDRESS, buf, 2);
+ if (ret < 0) {
+ NVT_ERR("CTP_I2C_READ failed! ret=%d\n", ret);
+ goto nvt_read_register_exit;
+ }
+ *val = (buf[1] & bitmask) >> shift;
+
+nvt_read_register_exit:
+ return ret;
+}
+
+void nvt_sw_reset_idle(void)
+{
+ nvt_write_addr(SWRST_SIF_ADDR, 0xAA);
+
+ usleep_range(15000, 16000);
+
+ nvt_clear_crc_err_flag();
+}
+
+void nvt_bootloader_reset(void)
+{
+ NVT_LOG("start\n");
+
+ nvt_write_addr(SWRST_SIF_ADDR, 0x69);
+
+ msleep(35);
+
+ NVT_LOG("end\n");
+}
+
+bool nvt_check_fw_reset_state(enum rst_complete_state check_reset_state)
+{
+ uint8_t buf[8] = { 0 };
+ bool ret = true;
+ int32_t retry = 0;
+
+ nvt_set_page(I2C_FW_ADDRESS, ts->mmap->EVENT_BUF_ADDR);
+
+ while (1) {
+ usleep_range(10000, 11000);
+
+ buf[0] = EVENT_MAP_RESET_COMPLETE;
+ buf[1] = 0x00;
+ CTP_I2C_READ(ts->client, I2C_FW_ADDRESS, buf, 6);
+
+ if ((buf[1] >= check_reset_state) &&
+ (buf[1] <= RESET_STATE_MAX)) {
+ ret = true;
+ break;
+ }
+
+ retry++;
+ if (unlikely(retry > 100)) {
+ NVT_ERR("retry=%d, buf[1]=0x%02X\n", retry, buf[1]);
+ ret = false;
+ break;
+ }
+ }
+
+ return ret;
+}
+
+int32_t nvt_check_cascade_chip_numbers(void)
+{
+ uint8_t enb_casc = 0;
+ uint8_t sdloc = 0;
+ uint8_t ic_num = 0;
+ int32_t ret = 0;
+
+ if (ts->cascade_type) {
+ if (ts->cascade_type == CASCADE_ENB_CASC) {
+ if (!ts->mmap->ENB_CASC_REG.addr) {
+ NVT_ERR("ENB_CASC_REG.addr is missing!\n");
+ return (-EIO);
+ }
+
+ nvt_set_page(I2C_FW_ADDRESS,
+ ts->mmap->ENB_CASC_REG.addr);
+ nvt_read_reg(ts->mmap->ENB_CASC_REG, &enb_casc);
+
+ if (enb_casc == ENB_CASC_1CHIP)
+ ts->cascade_num = CASCADE_1CHIP;
+ else
+ ts->cascade_num = CASCADE_2CHIP;
+
+ } else if (ts->cascade_type == CASCADE_SDLOC) {
+ if (!ts->mmap->SDLOC_REG.addr) {
+ NVT_ERR("SDLOC_REG.addr is missing!\n");
+ return (-EIO);
+ }
+
+ nvt_set_page(I2C_FW_ADDRESS, ts->mmap->SDLOC_REG.addr);
+ nvt_read_reg(ts->mmap->SDLOC_REG, &sdloc);
+
+ switch (sdloc) {
+ case SDLOC_1CHIP:
+ ts->cascade_num = CASCADE_1CHIP;
+ break;
+ case SDLOC_2CHIP:
+ ts->cascade_num = CASCADE_2CHIP;
+ break;
+ case SDLOC_3CHIP_AND_ABOVE:
+ ts->cascade_num = CASCADE_3CHIP;
+ break;
+ default:
+ ts->cascade_num = CASCADE_CHECK_ERR;
+ NVT_ERR("Undefined CASCADE_TYPE_SDLOC: 0x%X!\n",
+ sdloc);
+ ret = (-EIO);
+ break;
+ }
+ } else if (ts->cascade_type == CASCADE_IC_NUM) {
+ if (!ts->mmap->IC_NUM_REG.addr) {
+ NVT_ERR("IC_NUM_REG.addr is missing!\n");
+ return (-EIO);
+ }
+
+ nvt_set_page(I2C_FW_ADDRESS, ts->mmap->IC_NUM_REG.addr);
+ nvt_read_reg(ts->mmap->IC_NUM_REG, &ic_num);
+
+ switch (ic_num) {
+ case IC_NUM_1CHIP:
+ ts->cascade_num = CASCADE_1CHIP;
+ break;
+ case IC_NUM_2CHIP:
+ ts->cascade_num = CASCADE_2CHIP;
+ break;
+ case IC_NUM_3CHIP:
+ case IC_NUM_3CHIP_AND_ABOVE:
+ ts->cascade_num = CASCADE_3CHIP;
+ break;
+ default:
+ ts->cascade_num = CASCADE_CHECK_ERR;
+ NVT_ERR("Undefined CASCADE_TYPE_IC_NUM: 0x%X!\n",
+ ic_num);
+ ret = (-EIO);
+ break;
+ }
+ }
+ } else {
+ ts->cascade_num = NONE_CASCADE_CASE;
+ NVT_LOG("CASCADE_NONE cascade_type: %d, cascade_num: %d\n",
+ ts->cascade_type, ts->cascade_num);
+ }
+
+ return ret;
+}
+
+int32_t nvt_get_fw_info(void)
+{
+ uint8_t buf[64] = { 0 };
+ uint8_t retry_count = 2;
+ int32_t ret = 0;
+
+info_retry:
+ nvt_set_page(I2C_FW_ADDRESS,
+ ts->mmap->EVENT_BUF_ADDR | EVENT_MAP_FWINFO);
+
+ buf[0] = EVENT_MAP_FWINFO;
+ CTP_I2C_READ(ts->client, I2C_FW_ADDRESS, buf, 17);
+ if ((buf[1] + buf[2]) != 0xFF) {
+ NVT_ERR("FW info is broken! fw_ver=0x%02X, ~fw_ver=0x%02X\n",
+ buf[1], buf[2]);
+ if (retry_count < 3) {
+ retry_count++;
+ NVT_ERR("retry_count=%d\n", retry_count);
+ goto info_retry;
+ } else {
+ ts->fw_ver = 0;
+ NVT_ERR("Set default fw_ver=0x%02X\n", ts->fw_ver);
+ ret = -1;
+ goto out;
+ }
+ }
+ ts->fw_ver = buf[1];
+ ts->x_num = buf[3];
+ ts->y_num = buf[4];
+ ts->nvt_pid = (uint16_t)((buf[36] << 8) | buf[35]);
+
+ NVT_LOG("fw_ver=0x%02X, x_num=%0d, y_num=%0d, fw_type=0x%02X, PID=0x%04X\n",
+ ts->fw_ver, ts->x_num, ts->y_num, buf[14], ts->nvt_pid);
+
+ ret = 0;
+out:
+ return ret;
+}
+
+#ifdef CONFIG_OF
+static void nvt_parse_dt(struct device *dev)
+{
+ struct device_node *np = dev->of_node;
+
+ ts->irq_gpio = of_get_named_gpio_flags(np, "novatek,irq-gpio", 0,
+ &ts->irq_flags);
+ NVT_LOG("novatek,irq-gpio=%d\n", ts->irq_gpio);
+}
+#else /* CONFIG_OF */
+static void nvt_parse_dt(struct device *dev)
+{
+ ts->irq_gpio = NVTTOUCH_INT_PIN;
+}
+#endif /* CONFIG_OF */
+
+static int nvt_gpio_config(struct nvt_ts_data *ts)
+{
+ int32_t ret = 0;
+
+ if (gpio_is_valid(ts->irq_gpio)) {
+ ret = gpio_request_one(ts->irq_gpio, GPIOF_IN, "NVT-int");
+ if (ret) {
+ NVT_ERR("Failed to request NVT-int GPIO!\n");
+ goto err_request_irq_gpio;
+ }
+ }
+
+ return ret;
+
+err_request_irq_gpio:
+ return ret;
+}
+
+static void nvt_gpio_deconfig(struct nvt_ts_data *ts)
+{
+ if (gpio_is_valid(ts->irq_gpio))
+ gpio_free(ts->irq_gpio);
+}
+
+static uint8_t nvt_calculate_crc8(uint8_t *buf, uint8_t length)
+{
+ uint8_t i = 0, j = 0;
+ uint8_t crc8 = 0;
+ uint8_t poly = 0x1D; // x8 + x4 + x3 + x2 + 1
+
+ for (i = 0; i < length; i++) {
+ crc8 ^= *(buf + i);
+
+ for (j = 0; j < 8; j++) {
+ if (crc8 & 0x80)
+ crc8 = (crc8 << 1) ^ poly;
+ else
+ crc8 = (crc8 << 1);
+ }
+ }
+
+ crc8 ^= (uint8_t)0x00;
+ return crc8;
+}
+
+static int32_t nvt_ts_event_crc8(uint8_t *buf, uint8_t length)
+{
+ uint8_t crc8 = nvt_calculate_crc8(buf, length - 1);
+ uint32_t i = 0;
+
+ /* The lastest item is CRC8 which is reported by FW */
+ if (buf[length - 1] != crc8) {
+ NVT_ERR("CRC8 not match, FW reported: 0x%02X, Calulated CRC8: 0x%02X\n",
+ buf[length - 1], crc8);
+ if (debug_log) {
+ NVT_ERR("length=%d\n", length);
+ for (i = 0; i < length; i++) {
+ NVT_LOG("%02X ", buf[i]);
+ if (i % POINT_DATA_LEN == 0)
+ NVT_LOG("\n");
+ }
+ NVT_LOG("\n");
+ }
+ return -1;
+ }
+ return crc8;
+}
+
+static irqreturn_t nvt_ts_work_func(int irq, void *data)
+{
+ int32_t ret = -1;
+ uint8_t touch_event[DUMMY_BYTES + TOUCH_EVENT_CRC_LEN] = { 0 };
+ uint32_t event_size = ARRAY_SIZE(touch_event);
+ uint32_t position = 0;
+ uint32_t input_x = 0;
+ uint32_t input_y = 0;
+ uint8_t input_id = 0;
+ uint8_t press_id[TOUCH_MAX_FINGER_NUM] = { 0 };
+ int32_t i = 0;
+ int32_t finger_cnt = 0;
+
+ mutex_lock(&ts->lock);
+
+ ret = CTP_I2C_READ(ts->client, I2C_FW_ADDRESS, touch_event, event_size);
+ if (ret < 0) {
+ NVT_ERR("CTP_I2C_READ failed! ret=%d\n", ret);
+ goto nvt_ts_work_func_exit;
+ }
+
+ if (debug_log) {
+ NVT_LOG("Touch event buffer length: %d\n", event_size);
+ for (i = DUMMY_BYTES; i < event_size; i++) {
+ NVT_LOG("%02X ", touch_event[i]);
+ if (i % POINT_DATA_LEN == 0)
+ NVT_LOG("\n");
+ }
+ NVT_LOG("\n");
+ }
+
+ ret = nvt_ts_event_crc8(touch_event + DUMMY_BYTES,
+ event_size - DUMMY_BYTES);
+ if (ret < 0)
+ goto nvt_ts_work_func_exit;
+
+ finger_cnt = 0;
+
+ for (i = 0; i < ts->max_touch_num; i++) {
+ position = DUMMY_BYTES + ASIL_FLAG_LEN + POINT_DATA_LEN * i;
+ input_id = (uint8_t)(touch_event[position] >> 4);
+
+ if ((input_id == 0) || (input_id > ts->max_touch_num))
+ continue;
+
+ /* Finger down (enter or moving) */
+ if (((touch_event[position] & 0x07) == 0x01) ||
+ ((touch_event[position] & 0x07) == 0x02)) {
+ input_x = (uint32_t)(touch_event[position + 1] << 8) +
+ (uint32_t)(touch_event[position + 2]);
+ input_y = (uint32_t)(touch_event[position + 3] << 8) +
+ (uint32_t)(touch_event[position + 4]);
+
+ if ((input_x > TOUCH_MAX_WIDTH) ||
+ (input_y > TOUCH_MAX_HEIGHT))
+ continue;
+
+ press_id[input_id - 1] = 1;
+ input_mt_slot(ts->input_dev, input_id - 1);
+ input_mt_report_slot_state(ts->input_dev,
+ MT_TOOL_FINGER, true);
+
+ input_report_abs(ts->input_dev, ABS_MT_POSITION_X,
+ input_x);
+ input_report_abs(ts->input_dev, ABS_MT_POSITION_Y,
+ input_y);
+
+ finger_cnt++;
+ }
+ }
+
+ for (i = 0; i < ts->max_touch_num; i++) {
+ if (press_id[i] != 1) {
+ input_mt_slot(ts->input_dev, i);
+ input_mt_report_slot_state(ts->input_dev,
+ MT_TOOL_FINGER, false);
+ }
+ }
+
+ input_report_key(ts->input_dev, BTN_TOUCH, (finger_cnt > 0));
+
+ input_sync(ts->input_dev);
+
+nvt_ts_work_func_exit:
+ mutex_unlock(&ts->lock);
+
+ return IRQ_HANDLED;
+}
+
+int32_t nvt_clear_crc_err_flag(void)
+{
+ uint8_t buf[2] = { 0x00 };
+ int ret = 0;
+
+ nvt_set_page(I2C_FW_ADDRESS, (uint32_t)BLD_SPE_PUPS_ADDR);
+
+ buf[0] = (uint32_t)BLD_SPE_PUPS_ADDR & 0xFF;
+ buf[1] = 0xA5;
+ ret = CTP_I2C_WRITE(ts->client, I2C_FW_ADDRESS, buf, 2);
+ if (ret < 0) {
+ NVT_LOG("Write 0x%02X to BLD_SPE_PUPS(0x%X) failed, ret=%d!\n",
+ (uint32_t)BLD_SPE_PUPS_ADDR, buf[1], ret);
+ return ret;
+ }
+
+ buf[0] = (uint32_t)BLD_SPE_PUPS_ADDR & 0xFF;
+ buf[1] = 0x00;
+ ret = CTP_I2C_READ(ts->client, I2C_FW_ADDRESS, buf, 2);
+ if (ret < 0) {
+ NVT_LOG("Read BLD_SPE_PUPS (0x%X) failed, ret=%d!\n",
+ (uint32_t)BLD_SPE_PUPS_ADDR, ret);
+ return ret;
+ }
+
+ NVT_LOG("BLD_SPE_PUPS(0x%X)=0x%02X\n", (uint32_t)BLD_SPE_PUPS_ADDR,
+ buf[1]);
+
+ if (buf[1] != 0xA5)
+ return (-EIO);
+
+ return ret;
+}
+
+void nvt_stop_crc_reboot(void)
+{
+ uint8_t buf[4] = { 0 };
+ int32_t i = 0;
+
+ /* Read dummy buffer to check CRC fail reboot is happening or not */
+ nvt_set_page(I2C_FW_ADDRESS, (uint32_t)CHIP_VER_TRIM_ADDR);
+
+ /* Read to check if buf is 0xFC which means IC is in CRC reboot */
+ buf[0] = (uint32_t)CHIP_VER_TRIM_ADDR & 0xFF;
+ CTP_I2C_READ(ts->client, I2C_FW_ADDRESS, buf, 4);
+
+ if (((buf[1] == 0xFB) && (buf[2] == 0xFB) && (buf[3] == 0xFB)) ||
+ ((buf[1] == 0xFC) && (buf[2] == 0xFC) && (buf[3] == 0xFC)) ||
+ ((buf[1] == 0xFF) && (buf[2] == 0xFF) && (buf[3] == 0xFF))) {
+ /* IC is in CRC fail reboot loop, needs to be stopped! */
+ for (i = 5; i > 0; i--) {
+ /*
+ * Reset idle twice to ensure that the cmd will be
+ * executed in CRC reboot process
+ */
+ nvt_sw_reset_idle();
+
+ nvt_sw_reset_idle();
+ usleep_range(1000, 2000);
+
+ if (nvt_clear_crc_err_flag() >= 0)
+ break;
+ }
+ if (i == 0)
+ NVT_ERR("CRC auto reboot is not able to be stopped! buf[1]=0x%02X\n",
+ buf[1]);
+ }
+}
+
+static int8_t nvt_ts_check_chip_ver_trim(void)
+{
+ uint8_t buf[8] = { 0 };
+ int32_t i = 0;
+ int32_t j = 0;
+ int32_t k = 0;
+ int32_t found_nvt_chip = 0;
+ int32_t ret = -1;
+
+ nvt_bootloader_reset();
+
+ for (i = 5; i > 0; i--) {
+ nvt_sw_reset_idle();
+
+ nvt_set_page(I2C_FW_ADDRESS, (uint32_t)CHIP_VER_TRIM_ADDR);
+
+ buf[0] = ((uint32_t)CHIP_VER_TRIM_ADDR & 0xFF);
+ buf[1] = 0x00;
+ buf[2] = 0x00;
+ buf[3] = 0x00;
+ buf[4] = 0x00;
+ buf[5] = 0x00;
+ buf[6] = 0x00;
+ CTP_I2C_READ(ts->client, I2C_FW_ADDRESS, buf, 7);
+ NVT_LOG("EXT_CHIP_ID_RD(0x%X)=0x%02X, 0x%02X, 0x%02X, 0x%02X, 0x%02X, 0x%02X\n",
+ (uint32_t)CHIP_VER_TRIM_ADDR, buf[1], buf[2], buf[3],
+ buf[4], buf[5], buf[6]);
+
+ /* Stop CRC check to prevent IC auto reboot */
+ if (((buf[1] == 0xFB) && (buf[2] == 0xFB) &&
+ (buf[3] == 0xFB)) ||
+ ((buf[1] == 0xFC) && (buf[2] == 0xFC) &&
+ (buf[3] == 0xFC)) ||
+ ((buf[1] == 0xFF) && (buf[2] == 0xFF) &&
+ (buf[3] == 0xFF))) {
+ nvt_stop_crc_reboot();
+ continue;
+ }
+
+ /* Compare read chip id on supported list */
+ for (j = 0; j < ARRAY_SIZE(trim_id_table); j++) {
+ found_nvt_chip = 0;
+
+ /* Compare each byte */
+ for (k = 0; k < NVT_ID_BYTE_MAX; k++) {
+ if (trim_id_table[j].mask[k]) {
+ if (buf[k + 1] !=
+ trim_id_table[j].id[k])
+ break;
+ }
+ }
+
+ if (k == NVT_ID_BYTE_MAX)
+ found_nvt_chip = 1;
+
+ if (found_nvt_chip) {
+ ts->mmap = trim_id_table[j].mmap;
+ ts->cascade_type =
+ trim_id_table[j].hwinfo->cascade_type;
+
+ ret = nvt_check_cascade_chip_numbers();
+ if (ret < 0)
+ NVT_ERR("Get cascade chip numbers failed! ret=%d\n",
+ ret);
+
+ /* Find out the memory map of target chip numbers */
+ if (ts->cascade_num ==
+ trim_id_table[j].hwinfo->cascade_num) {
+ if (ts->cascade_type ==
+ CASCADE_ENB_CASC) {
+ NVT_LOG("ENB_CASC_REG.addr(0x%X)\n",
+ ts->mmap->ENB_CASC_REG
+ .addr);
+ } else if (ts->cascade_type ==
+ CASCADE_IC_NUM) {
+ NVT_LOG("IC_NUM_REG.addr(0x%X)\n",
+ ts->mmap->IC_NUM_REG.addr;
+ } else {
+ NVT_LOG("Non cascade case\n");
+ }
+ NVT_LOG("trim_id_table[%d] is matched.\n",
+ j);
+ NVT_LOG("This is NVT touch IC.\n");
+ goto out;
+ }
+ }
+ }
+ usleep_range(10000, 11000);
+ }
+
+ ts->mmap = NULL;
+ ret = -1;
+
+out:
+ return ret;
+}
+
+static int32_t nvt_ts_probe(struct i2c_client *client)
+{
+ int32_t ret = 0;
+
+ ts = kzalloc(sizeof(struct nvt_ts_data), GFP_KERNEL);
+ if (ts == NULL) {
+ NVT_ERR("Failed to allocated memory for nvt ts data!\n");
+ return -ENOMEM;
+ }
+
+ ts->xbuf = kzalloc(NVT_XBUF_LEN, GFP_KERNEL);
+ if (ts->xbuf == NULL) {
+ NVT_ERR("kzalloc for xbuf failed!\n");
+ ret = -ENOMEM;
+ goto err_malloc_xbuf;
+ }
+
+ ts->client = client;
+ i2c_set_clientdata(client, ts);
+
+ nvt_parse_dt(&client->dev);
+
+ ret = nvt_gpio_config(ts);
+ if (ret) {
+ NVT_ERR("Gpio config error!\n");
+ goto err_gpio_config_failed;
+ }
+
+ if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
+ NVT_ERR("i2c_check_functionality() failed! (no I2C_FUNC_I2C)\n");
+ ret = -ENODEV;
+ goto err_check_functionality_failed;
+ }
+
+ mutex_init(&ts->lock);
+ mutex_init(&ts->xbuf_lock);
+
+ /* Need 10ms delay after POR (power on reset) */
+ usleep_range(10000, 11000);
+
+ ret = nvt_ts_check_chip_ver_trim();
+ if (ret) {
+ NVT_ERR("This chip is not identified in mapping table!\n");
+ ret = -EINVAL;
+ goto err_chipvertrim_failed;
+ }
+
+ nvt_bootloader_reset();
+ nvt_check_fw_reset_state(RESET_STATE_INIT);
+ nvt_get_fw_info();
+
+ ts->input_dev = input_allocate_device();
+ if (ts->input_dev == NULL) {
+ NVT_ERR("Allocate input device failed!\n");
+ ret = -ENOMEM;
+ goto err_input_dev_alloc_failed;
+ }
+
+ ts->max_touch_num = TOUCH_MAX_FINGER_NUM;
+
+ ts->int_trigger_type = INT_TRIGGER_TYPE;
+ ts->input_dev->evbit[0] = BIT_MASK(EV_SYN) | BIT_MASK(EV_KEY) |
+ BIT_MASK(EV_ABS);
+ ts->input_dev->keybit[BIT_WORD(BTN_TOUCH)] = BIT_MASK(BTN_TOUCH);
+ ts->input_dev->propbit[0] = BIT(INPUT_PROP_DIRECT);
+
+ input_mt_init_slots(ts->input_dev, ts->max_touch_num, 0);
+
+#if TOUCH_MAX_FINGER_NUM > 1
+ input_set_abs_params(ts->input_dev, ABS_MT_POSITION_X, 0,
+ TOUCH_MAX_WIDTH, 0, 0);
+ input_set_abs_params(ts->input_dev, ABS_MT_POSITION_Y, 0,
+ TOUCH_MAX_HEIGHT, 0, 0);
+#endif
+
+ sprintf(ts->phys, "input/ts");
+ ts->input_dev->name = NVT_TS_NAME;
+ ts->input_dev->phys = ts->phys;
+ ts->input_dev->id.bustype = BUS_I2C;
+
+ ret = input_register_device(ts->input_dev);
+ if (ret) {
+ NVT_ERR("Register input device (%s) failed! ret=%d\n",
+ ts->input_dev->name, ret);
+ goto err_input_register_device_failed;
+ }
+
+ client->irq = gpio_to_irq(ts->irq_gpio);
+ if (client->irq) {
+ NVT_LOG("int_trigger_type=%d\n", ts->int_trigger_type);
+ ts->irq_enabled = true;
+ ret = request_threaded_irq(client->irq, NULL, nvt_ts_work_func,
+ ts->int_trigger_type | IRQF_ONESHOT,
+ NVT_I2C_NAME, ts);
+ if (ret != 0) {
+ NVT_ERR("Request irq failed! ret=%d\n", ret);
+ goto err_int_request_failed;
+ } else {
+ nvt_irq_enable(false);
+ NVT_LOG("Request irq %d succeed.\n", client->irq);
+ }
+ }
+
+ ts->fb_notif.notifier_call = nvt_fb_notifier_callback;
+ ret = fb_register_client(&ts->fb_notif);
+ if (ret) {
+ NVT_ERR("Register fb_notifier failed! ret=%d\n", ret);
+ goto err_register_fb_notif_failed;
+ }
+
+ bTouchIsAwake = 1;
+ NVT_LOG("end\n");
+
+ nvt_irq_enable(true);
+
+ return 0;
+
+ if (fb_unregister_client(&ts->fb_notif))
+ NVT_ERR("Error occurred while unregistering fb_notifier!\n");
+err_register_fb_notif_failed:
+ free_irq(client->irq, ts);
+err_int_request_failed:
+ input_unregister_device(ts->input_dev);
+ ts->input_dev = NULL;
+err_input_register_device_failed:
+ if (ts->input_dev) {
+ input_free_device(ts->input_dev);
+ ts->input_dev = NULL;
+ }
+err_input_dev_alloc_failed:
+err_chipvertrim_failed:
+ mutex_destroy(&ts->xbuf_lock);
+ mutex_destroy(&ts->lock);
+err_check_functionality_failed:
+ nvt_gpio_deconfig(ts);
+err_gpio_config_failed:
+ i2c_set_clientdata(client, NULL);
+ kfree(ts->xbuf);
+ ts->xbuf = NULL;
+err_malloc_xbuf:
+ kfree(ts);
+ ts = NULL;
+
+ return ret;
+}
+
+static void nvt_ts_remove(struct i2c_client *client)
+{
+ NVT_LOG("Removing driver...\n");
+
+ if (fb_unregister_client(&ts->fb_notif))
+ NVT_ERR("Error occurred while unregistering fb_notifier!\n");
+
+ nvt_irq_enable(false);
+ free_irq(client->irq, ts);
+
+ mutex_destroy(&ts->xbuf_lock);
+ mutex_destroy(&ts->lock);
+
+ nvt_gpio_deconfig(ts);
+
+ if (ts->input_dev) {
+ input_unregister_device(ts->input_dev);
+ ts->input_dev = NULL;
+ }
+
+ i2c_set_clientdata(client, NULL);
+
+ kfree(ts->xbuf);
+ ts->xbuf = NULL;
+
+ kfree(ts);
+ ts = NULL;
+}
+
+static void nvt_ts_shutdown(struct i2c_client *client)
+{
+ NVT_LOG("Shutdown driver...\n");
+
+ nvt_irq_enable(false);
+
+ if (fb_unregister_client(&ts->fb_notif))
+ NVT_ERR("Error occurred while unregistering fb_notifier!\n");
+}
+
+static int32_t nvt_ts_resume(struct device *dev)
+{
+ if (bTouchIsAwake) {
+ NVT_LOG("Touch is already resume.\n");
+ return 0;
+ }
+
+ mutex_lock(&ts->lock);
+
+ NVT_LOG("start\n");
+
+ nvt_bootloader_reset();
+ nvt_check_fw_reset_state(RESET_STATE_NORMAL_RUN);
+
+ nvt_irq_enable(true);
+
+ bTouchIsAwake = 1;
+
+ mutex_unlock(&ts->lock);
+
+ NVT_LOG("end\n");
+
+ return 0;
+}
+
+static int nvt_fb_notifier_callback(struct notifier_block *self,
+ unsigned long event, void *data)
+{
+ struct fb_event *evdata = data;
+ int *blank;
+ struct nvt_ts_data *ts =
+ container_of(self, struct nvt_ts_data, fb_notif);
+
+ if (evdata && evdata->data && event == FB_EVENT_BLANK) {
+ blank = evdata->data;
+ if (*blank == FB_BLANK_UNBLANK) {
+ NVT_LOG("event=%lu, *blank=%d\n", event, *blank);
+ nvt_ts_resume(&ts->client->dev);
+ }
+ }
+
+ return 0;
+}
+
+static const struct i2c_device_id nvt_ts_id[] = { { NVT_I2C_NAME, 0 }, {} };
+
+#ifdef CONFIG_OF
+static const struct of_device_id nvt_match_table[] = {
+ {
+ .compatible = "novatek,NVT-ts",
+ },
+ {},
+};
+#endif
+
+static struct i2c_driver nvt_i2c_driver = {
+ .probe = nvt_ts_probe,
+ .remove = nvt_ts_remove,
+ .shutdown = nvt_ts_shutdown,
+ .id_table = nvt_ts_id,
+ .driver = {
+ .name = NVT_I2C_NAME,
+ .owner = THIS_MODULE,
+#ifdef CONFIG_OF
+ .of_match_table = nvt_match_table,
+#endif
+ },
+};
+
+static int32_t __init nvt_driver_init(void)
+{
+ int32_t ret = 0;
+
+ NVT_LOG("start\n");
+
+ bTouchIsAwake = 0;
+
+ ret = i2c_add_driver(&nvt_i2c_driver);
+ if (ret) {
+ NVT_ERR("Failed to add i2c driver!");
+ goto err_driver;
+ }
+
+ NVT_LOG("end\n");
+
+err_driver:
+ return ret;
+}
+
+static void __exit nvt_driver_exit(void)
+{
+ i2c_del_driver(&nvt_i2c_driver);
+}
+
+module_init(nvt_driver_init);
+module_exit(nvt_driver_exit);
+
+MODULE_DESCRIPTION("Novatek Touchscreen Driver");
+MODULE_LICENSE("GPL");
diff --git a/drivers/input/touchscreen/nt519xx.h b/drivers/input/touchscreen/nt519xx.h
new file mode 100644
index 000000000000..27521416cc6c
--- /dev/null
+++ b/drivers/input/touchscreen/nt519xx.h
@@ -0,0 +1,130 @@
+/* SPDX-License-Identifier: GPL-2.0-only
+ *
+ * Driver for Novatek NT519xx touchscreens
+ *
+ * Copyright (C) 2023 Wei-Shih Lin <Weishih_Lin@novatek.com.tw>
+ * Copyright (C) 2023 Leo LS Chang <Leo_LS_Chang@novatek.com.tw>
+ *
+ * Copyright (C) 2010 - 2023 Novatek, Inc.
+ *
+ */
+#ifndef _LINUX_NVT_TOUCH_H
+#define _LINUX_NVT_TOUCH_H
+
+#include <linux/delay.h>
+#include <linux/i2c.h>
+#include <linux/input.h>
+#include <linux/uaccess.h>
+#include <linux/slab.h>
+
+#include "nt519xx_mem_map.h"
+
+#define NVT_DEBUG (1)
+
+/* GPIO Number */
+#define NVTTOUCH_RST_PIN (980)
+#define NVTTOUCH_INT_PIN (943)
+
+/* INT Trigger Mode */
+#define INT_TRIGGER_TYPE (IRQ_TYPE_EDGE_RISING)
+
+/* I2C Driver Info */
+#define NVT_I2C_NAME "NVT-ts"
+#define I2C_FW_ADDRESS (0x01)
+#define I2C_HW_ADDRESS (0x62)
+
+#if NVT_DEBUG
+#define NVT_LOG(fmt, args...) \
+ dev_err(&ts->client->dev, "[%s] %s %d: " fmt, NVT_I2C_NAME, __func__, \
+ __LINE__, ##args)
+#else
+#define NVT_LOG(fmt, args...) \
+ dev_info(&ts->client->dev, "[%s] %s %d: " fmt, NVT_I2C_NAME, __func__, \
+ __LINE__, ##args)
+#endif
+#define NVT_ERR(fmt, args...) \
+ dev_err(&ts->client->dev, "[ERROR][%s] %s %d: " fmt, NVT_I2C_NAME, \
+ __func__, __LINE__, ##args)
+
+/* Input Device Info */
+#define NVT_TS_NAME "NVTCapacitiveTouchScreen"
+
+/* Touch Info */
+#define TOUCH_MAX_WIDTH (1920)
+#define TOUCH_MAX_HEIGHT (1080)
+#define TOUCH_MAX_FINGER_NUM (10)
+
+/* Each data length in event buffer (unit: byte) */
+#define ASIL_FLAG_LEN (1)
+#define POINT_DATA_LEN (7)
+#define BUTTON_STATUS_LEN (2)
+#define TOUCH_EVENT_LEN \
+ (ASIL_FLAG_LEN + (TOUCH_MAX_FINGER_NUM * POINT_DATA_LEN) + \
+ BUTTON_STATUS_LEN)
+#define ASIL_INFO_LEN (6)
+#define CRC_VALUE_LEN (1)
+#define TOUCH_EVENT_CRC_LEN (TOUCH_EVENT_LEN + ASIL_INFO_LEN + CRC_VALUE_LEN)
+
+#define DUMMY_BYTES (1)
+#define NVT_XBUF_LEN (1025)
+
+struct nvt_ts_data {
+ struct i2c_client *client;
+ struct input_dev *input_dev;
+ int8_t phys[32];
+ struct notifier_block fb_notif;
+ uint8_t fw_ver;
+ uint8_t x_num;
+ uint8_t y_num;
+ uint8_t max_touch_num;
+ uint32_t int_trigger_type;
+ int32_t irq_gpio;
+ uint32_t irq_flags;
+ int32_t reset_gpio;
+ uint32_t reset_flags;
+ struct mutex lock;
+ const struct nvt_ts_mem_map *mmap;
+ enum cascade_type cascade_type;
+ enum cascade_chip_num cascade_num;
+ uint16_t nvt_pid;
+ uint8_t *xbuf;
+ struct mutex xbuf_lock;
+ bool irq_enabled;
+};
+
+enum rst_complete_state {
+ RESET_STATE_INIT = 0xA0,
+ RESET_STATE_REK,
+ RESET_STATE_REK_FINISH,
+ RESET_STATE_NORMAL_RUN,
+ RESET_STATE_MAX = 0xAF
+};
+
+enum {
+ EVENT_MAP_HOST_CMD = 0x50,
+ EVENT_MAP_HANDSHAKING_or_SUB_CMD_BYTE = 0x51,
+ EVENT_MAP_RESET_COMPLETE = 0x60,
+ EVENT_MAP_FWINFO = 0x78,
+ EVENT_MAP_CASCADE_CHIP_NUMBERS = 0x99,
+ EVENT_MAP_PROJECTID = 0x9A,
+};
+
+extern struct nvt_ts_data *ts;
+
+int32_t CTP_I2C_READ(struct i2c_client *client, uint16_t address, uint8_t *buf,
+ uint16_t len);
+int32_t CTP_I2C_WRITE(struct i2c_client *client, uint16_t address, uint8_t *buf,
+ uint16_t len);
+void nvt_bootloader_reset(void);
+void nvt_sw_reset_idle(void);
+int32_t nvt_clear_crc_err_flag(void);
+bool nvt_check_fw_reset_state(enum rst_complete_state check_reset_state);
+int32_t nvt_get_fw_info(void);
+int32_t nvt_check_cascade_chip_numbers(void);
+int32_t nvt_set_page(uint16_t i2c_addr, uint32_t addr);
+int32_t nvt_write_addr(uint32_t addr, uint8_t data);
+int32_t nvt_read_reg(struct nvt_ts_reg reg, uint8_t *val);
+
+void nvt_stop_crc_reboot(void);
+
+#endif /* _LINUX_NVT_TOUCH_H */
diff --git a/drivers/input/touchscreen/nt519xx_mem_map.h b/drivers/input/touchscreen/nt519xx_mem_map.h
new file mode 100644
index 000000000000..9e9847d73292
--- /dev/null
+++ b/drivers/input/touchscreen/nt519xx_mem_map.h
@@ -0,0 +1,262 @@
+/* SPDX-License-Identifier: GPL-2.0-only
+ *
+ * Driver for Novatek NT519xx touchscreens
+ *
+ * Copyright (C) 2023 Wei-Shih Lin <Weishih_Lin@novatek.com.tw>
+ * Copyright (C) 2023 Leo LS Chang <Leo_LS_Chang@novatek.com.tw>
+ *
+ * Copyright (C) 2010 - 2023 Novatek, Inc.
+ *
+ */
+
+#define CHIP_VER_TRIM_ADDR (0xFF004)
+#define SWRST_SIF_ADDR (0xFF0FE)
+#define BLD_SPE_PUPS_ADDR (0xFF135)
+
+enum {
+ BIT0 = 0x01,
+ BIT1 = 0x02,
+ BIT2 = 0x04,
+ BIT3 = 0x08,
+ BIT4 = 0x10,
+ BIT5 = 0x20,
+ BIT6 = 0x40,
+ BIT7 = 0x80
+};
+
+enum cascade_type {
+ CASCADE_NONE = 0x00,
+ CASCADE_ENB_CASC = 0x01,
+ CASCADE_SDLOC = 0x02,
+ CASCADE_IC_NUM = 0x03
+};
+
+enum { ENB_CASC_2CHIP = 0x00, ENB_CASC_1CHIP = 0x01 };
+
+enum { SDLOC_1CHIP = 0x04, SDLOC_2CHIP = 0x06, SDLOC_3CHIP_AND_ABOVE = 0x07 };
+
+enum {
+ IC_NUM_3CHIP = 0x00,
+ IC_NUM_1CHIP = 0x01,
+ IC_NUM_2CHIP = 0x02,
+ IC_NUM_3CHIP_AND_ABOVE = 0x03
+};
+
+enum cascade_chip_num {
+ CASCADE_CHECK_ERR = 0,
+ NONE_CASCADE_CASE = 1,
+ CASCADE_1CHIP = NONE_CASCADE_CASE,
+ CASCADE_2CHIP = 2,
+ CASCADE_3CHIP = 3,
+ CASCADE_4CHIP = 4,
+ CASCADE_5CHIP = 5,
+ CASCADE_CHIP_MAX = CASCADE_5CHIP
+};
+
+struct nvt_ts_reg {
+ uint32_t addr;
+ uint8_t bitmask;
+};
+
+struct nvt_ts_mem_map {
+ uint32_t EVENT_BUF_ADDR;
+ struct nvt_ts_reg ENB_CASC_REG;
+ struct nvt_ts_reg SDLOC_REG;
+ struct nvt_ts_reg IC_NUM_REG;
+};
+
+struct nvt_ts_hw_info {
+ enum cascade_type cascade_type;
+ enum cascade_chip_num cascade_num;
+ uint8_t default_x_num;
+ uint8_t default_y_num;
+ uint16_t default_abs_x_max;
+ uint16_t default_abs_y_max;
+ uint8_t default_max_touch_num;
+ uint8_t default_max_button_num;
+};
+
+static const struct nvt_ts_mem_map NT51900_memory_map = {
+ .EVENT_BUF_ADDR = 0x2A800,
+ .ENB_CASC_REG = { .addr = 0x3F02C, .bitmask = BIT0 },
+};
+
+static const struct nvt_ts_mem_map NT51920_1chip_memory_map = {
+ .EVENT_BUF_ADDR = 0x30500,
+ .ENB_CASC_REG = { .addr = 0x3F02C, .bitmask = BIT0 },
+};
+
+static const struct nvt_ts_mem_map NT51920_2chip_memory_map = {
+ .EVENT_BUF_ADDR = 0x30500,
+ .ENB_CASC_REG = { .addr = 0x3F02C, .bitmask = BIT0 },
+};
+
+static const struct nvt_ts_mem_map NT51923_1chip_memory_map = {
+ .EVENT_BUF_ADDR = 0x94000,
+ .SDLOC_REG = { .addr = 0xFF02C, .bitmask = (BIT2 | BIT1 | BIT0) },
+};
+
+static const struct nvt_ts_mem_map NT51923_2chip_memory_map = {
+ .EVENT_BUF_ADDR = 0x94000,
+ .SDLOC_REG = { .addr = 0xFF02C, .bitmask = (BIT2 | BIT1 | BIT0) },
+};
+
+static const struct nvt_ts_mem_map NT51923_3chip_memory_map = {
+ .EVENT_BUF_ADDR = 0x94000,
+ .SDLOC_REG = { .addr = 0xFF02C, .bitmask = (BIT2 | BIT1 | BIT0) },
+};
+
+static const struct nvt_ts_mem_map NT51926_1chip_memory_map = {
+ .EVENT_BUF_ADDR = 0x96A00,
+ .IC_NUM_REG = { .addr = 0xFF02C, .bitmask = (BIT3 | BIT2) },
+};
+
+static const struct nvt_ts_mem_map NT51926_2chip_memory_map = {
+ .EVENT_BUF_ADDR = 0x96A00,
+ .IC_NUM_REG = { .addr = 0xFF02C, .bitmask = (BIT3 | BIT2) },
+};
+
+static const struct nvt_ts_mem_map NT51926_3chip_memory_map = {
+ .EVENT_BUF_ADDR = 0x96A00,
+ .IC_NUM_REG = { .addr = 0xFF02C, .bitmask = (BIT3 | BIT2) },
+};
+
+static struct nvt_ts_hw_info NT51900_hw_info = {
+ .cascade_type = CASCADE_NONE,
+ .cascade_num = NONE_CASCADE_CASE,
+
+ .default_x_num = 40,
+ .default_y_num = 24,
+ .default_abs_x_max = 1280,
+ .default_abs_y_max = 720,
+ .default_max_button_num = 0,
+};
+
+static struct nvt_ts_hw_info NT51920_1chip_hw_info = {
+ .cascade_type = CASCADE_ENB_CASC,
+ .cascade_num = CASCADE_1CHIP,
+
+ .default_x_num = 48,
+ .default_y_num = 20,
+ .default_abs_x_max = 960,
+ .default_abs_y_max = 540,
+ .default_max_button_num = 0,
+};
+
+static struct nvt_ts_hw_info NT51920_2chip_hw_info = {
+ .cascade_type = CASCADE_ENB_CASC,
+ .cascade_num = CASCADE_2CHIP,
+
+ .default_x_num = 56,
+ .default_y_num = 21,
+ .default_abs_x_max = 2880,
+ .default_abs_y_max = 1080,
+ .default_max_button_num = 0,
+};
+
+static struct nvt_ts_hw_info NT51923_1chip_hw_info = {
+ .cascade_type = CASCADE_SDLOC,
+ .cascade_num = CASCADE_1CHIP,
+
+ .default_x_num = 24,
+ .default_y_num = 60,
+ .default_abs_x_max = 2880,
+ .default_abs_y_max = 1080,
+ .default_max_button_num = 0,
+};
+
+static struct nvt_ts_hw_info NT51923_2chip_hw_info = {
+ .cascade_type = CASCADE_SDLOC,
+ .cascade_num = CASCADE_2CHIP,
+
+ .default_x_num = 48,
+ .default_y_num = 60,
+ .default_abs_x_max = 2880,
+ .default_abs_y_max = 1080,
+ .default_max_button_num = 0,
+};
+
+static struct nvt_ts_hw_info NT51923_3chip_hw_info = {
+ .cascade_type = CASCADE_SDLOC,
+ .cascade_num = CASCADE_3CHIP,
+
+ .default_x_num = 72,
+ .default_y_num = 40,
+ .default_abs_x_max = 2880,
+ .default_abs_y_max = 1620,
+ .default_max_button_num = 0,
+};
+
+static struct nvt_ts_hw_info NT51926_1chip_hw_info = {
+ .cascade_type = CASCADE_IC_NUM,
+ .cascade_num = CASCADE_1CHIP,
+ .default_x_num = 16,
+ .default_y_num = 60,
+ .default_abs_x_max = 2880,
+ .default_abs_y_max = 1080,
+ .default_max_button_num = 0,
+};
+static struct nvt_ts_hw_info NT51926_2chip_hw_info = {
+ .cascade_type = CASCADE_IC_NUM,
+ .cascade_num = CASCADE_2CHIP,
+ .default_x_num = 32,
+ .default_y_num = 60,
+ .default_abs_x_max = 1768,
+ .default_abs_y_max = 828,
+ .default_max_button_num = 0,
+};
+static struct nvt_ts_hw_info NT51926_3chip_hw_info = {
+ .cascade_type = CASCADE_IC_NUM,
+ .cascade_num = CASCADE_3CHIP,
+ .default_x_num = 48,
+ .default_y_num = 60,
+ .default_abs_x_max = 2880,
+ .default_abs_y_max = 1080,
+ .default_max_button_num = 0,
+};
+#define NVT_ID_BYTE_MAX 6
+struct nvt_ts_trim_id_table {
+ uint8_t id[NVT_ID_BYTE_MAX];
+ uint8_t mask[NVT_ID_BYTE_MAX];
+ const struct nvt_ts_mem_map *mmap;
+ const struct nvt_ts_hw_info *hwinfo;
+};
+
+static const struct nvt_ts_trim_id_table trim_id_table[] = {
+ { .id = { 0x00, 0x00, 0x00, 0x00, 0x19, 0x05 },
+ .mask = { 0, 0, 0, 1, 1, 1 },
+ .mmap = &NT51900_memory_map,
+ .hwinfo = &NT51900_hw_info },
+ { .id = { 0x00, 0x00, 0x01, 0x20, 0x19, 0x05 },
+ .mask = { 0, 0, 0, 1, 1, 1 },
+ .mmap = &NT51920_1chip_memory_map,
+ .hwinfo = &NT51920_1chip_hw_info },
+ { .id = { 0x00, 0x00, 0x01, 0x20, 0x19, 0x05 },
+ .mask = { 0, 0, 0, 1, 1, 1 },
+ .mmap = &NT51920_2chip_memory_map,
+ .hwinfo = &NT51920_2chip_hw_info },
+ { .id = { 0x00, 0x00, 0x00, 0x23, 0x19, 0x05 },
+ .mask = { 0, 0, 0, 1, 1, 1 },
+ .mmap = &NT51923_1chip_memory_map,
+ .hwinfo = &NT51923_1chip_hw_info },
+ { .id = { 0x00, 0x00, 0x00, 0x23, 0x19, 0x05 },
+ .mask = { 0, 0, 0, 1, 1, 1 },
+ .mmap = &NT51923_2chip_memory_map,
+ .hwinfo = &NT51923_2chip_hw_info },
+ { .id = { 0x00, 0x00, 0x00, 0x23, 0x19, 0x05 },
+ .mask = { 0, 0, 0, 1, 1, 1 },
+ .mmap = &NT51923_3chip_memory_map,
+ .hwinfo = &NT51923_3chip_hw_info },
+ { .id = { 0x00, 0x00, 0x00, 0x26, 0x19, 0x05 },
+ .mask = { 0, 0, 0, 1, 1, 1 },
+ .mmap = &NT51926_1chip_memory_map,
+ .hwinfo = &NT51926_1chip_hw_info },
+ { .id = { 0x00, 0x00, 0x00, 0x26, 0x19, 0x05 },
+ .mask = { 0, 0, 0, 1, 1, 1 },
+ .mmap = &NT51926_2chip_memory_map,
+ .hwinfo = &NT51926_2chip_hw_info },
+ { .id = { 0x00, 0x00, 0x00, 0x26, 0x19, 0x05 },
+ .mask = { 0, 0, 0, 1, 1, 1 },
+ .mmap = &NT51926_3chip_memory_map,
+ .hwinfo = &NT51926_3chip_hw_info },
+};
--
2.26.1
^ permalink raw reply related
* [PATCH 1/2] dt-bindings: touchscreen: Add Novatek NT519XX series bindings
From: Wei-Shih Lin @ 2023-10-25 8:20 UTC (permalink / raw)
To: dmitry.torokhov, robh+dt, krzysztof.kozlowski+dt, conor+dt
Cc: linux-input, devicetree, linux-kernel
In-Reply-To: <20231025082054.1190-1-Weishih_Lin@novatek.com.tw>
This patch adds device tree bindings for Novatek NT519XX series
touchscreen devices.
Signed-off-by: Wei-Shih Lin <Weishih_Lin@novatek.com.tw>
---
.../input/touchscreen/novatek,nt519xx.yaml | 60 +++++++++++++++++++
MAINTAINERS | 9 +++
2 files changed, 69 insertions(+)
create mode 100644 Documentation/devicetree/bindings/input/touchscreen/novatek,nt519xx.yaml
diff --git a/Documentation/devicetree/bindings/input/touchscreen/novatek,nt519xx.yaml b/Documentation/devicetree/bindings/input/touchscreen/novatek,nt519xx.yaml
new file mode 100644
index 000000000000..00912e265197
--- /dev/null
+++ b/Documentation/devicetree/bindings/input/touchscreen/novatek,nt519xx.yaml
@@ -0,0 +1,60 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/input/touchscreen/novatek,nt519xx.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Novatek nt519xx touchscreen controller bindings
+
+maintainers:
+ - Wei-Shih Lin <Weishih_Lin@novatek.com.tw>
+ - Leo LS Chang <Leo_LS_Chang@novatek.com.tw>
+
+allOf:
+ - $ref: touchscreen.yaml#
+
+properties:
+ compatible:
+ enum:
+ - novatek,NVT-ts
+
+ reg:
+ maxItems: 1
+
+ novatek,irq-gpio:
+ maxItems: 1
+
+ novatek,reset-gpio:
+ maxItems: 1
+
+ touchscreen-size-x: true
+ touchscreen-size-y: true
+
+required:
+ - compatible
+ - reg
+ - novatek,irq-gpio
+ - novatek,reset-gpio
+ - touchscreen-size-x
+ - touchscreen-size-y
+
+additionalProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/interrupt-controller/irq.h>
+ #include <dt-bindings/gpio/gpio.h>
+ i2c {
+ novatek@62 {
+ compatible = "novatek,NVT-ts";
+ reg = <0x62>;
+
+ novatek,irq-gpio = <&gpio5 GPIO_C3 IRQ_TYPE_EDGE_RISING>;
+ novatek,reset-gpio = <&gpio7 GPIO_B1 GPIO_ACTIVE_LOW>;
+
+ touchscreen-size-x = <1920>;
+ touchscreen-size-y = <1080>;
+ };
+ };
+
+...
diff --git a/MAINTAINERS b/MAINTAINERS
index 4cc6bf79fdd8..25c88bf7333e 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -15009,6 +15009,15 @@ L: linux-input@vger.kernel.org
S: Maintained
F: drivers/input/touchscreen/novatek-nvt-ts.c
+NOVATEK NT519XX I2C TOUCHSCREEN DRIVER
+M: Wei-Shih Lin <Weishih_Lin@novatek.com.tw>
+L: linux-input@vger.kernel.org
+S: Maintained
+F: Documentation/devicetree/bindings/input/touchscreen/novatek,nt519xx.yaml
+F: drivers/input/touchscreen/nt519xx.c
+F: drivers/input/touchscreen/nt519xx.h
+F: drivers/input/touchscreen/nt519xx_mem_map.h
+
NSDEPS
M: Matthias Maennich <maennich@google.com>
S: Maintained
--
2.26.1
^ permalink raw reply related
* [PATCH 0/2] Add Novatek NT519XX touchcreen driver
From: Wei-Shih Lin @ 2023-10-25 8:20 UTC (permalink / raw)
To: dmitry.torokhov, robh+dt, krzysztof.kozlowski+dt, conor+dt
Cc: linux-input, devicetree, linux-kernel
This series adds a driver for Novatek TDDI NT519XX which mainly used in
automotive display products. This driver is different from the existing
driver named as novatek-nvt-ts.c in the path drivers/input/touchscreen/.
The existing driver supports another Novatek IC NT11205 used in Acer
Iconia One 7 B1-750 tablet.
Wei-Shih Lin (2):
dt-bindings: touchscreen: Add Novatek NT519XX series bindings
Input: Add driver for Novatek NT519XX series touchscreen devices
.../input/touchscreen/novatek,nt519xx.yaml | 60 ++
MAINTAINERS | 9 +
drivers/input/touchscreen/Kconfig | 12 +
drivers/input/touchscreen/Makefile | 1 +
drivers/input/touchscreen/nt519xx.c | 995 ++++++++++++++++++
drivers/input/touchscreen/nt519xx.h | 130 +++
drivers/input/touchscreen/nt519xx_mem_map.h | 262 +++++
7 files changed, 1469 insertions(+)
create mode 100644 Documentation/devicetree/bindings/input/touchscreen/novatek,nt519xx.yaml
create mode 100644 drivers/input/touchscreen/nt519xx.c
create mode 100644 drivers/input/touchscreen/nt519xx.h
create mode 100644 drivers/input/touchscreen/nt519xx_mem_map.h
--
2.26.1
^ permalink raw reply
* [PATCH 5/5] HID: mcp2221: Handle reads greater than 60 bytes
From: Hamish Martin @ 2023-10-25 3:55 UTC (permalink / raw)
To: gupt21, jikos, benjamin.tissoires, Enrik.Berkhan, sven.zuehlsdorf
Cc: linux-i2c, linux-input, Hamish Martin
In-Reply-To: <20231025035514.3450123-1-hamish.martin@alliedtelesis.co.nz>
When a user requests more than 60 bytes of data the MCP2221 must chunk
the data in chunks up to 60 bytes long (see command/response code 0x40
in the datasheet).
In order to signal that the device has more data the (undocumented) byte
at byte index 2 of the Get I2C Data response uses the value 0x54. This
contrasts with the case for the final data chunk where the value
returned is 0x55 (MCP2221_I2C_READ_COMPL). The fact that 0x55 was not
returned in the response was interpreted by the driver as a failure
meaning that all reads of more than 60 bytes would fail.
Add support for reads that are split over multiple chunks by looking for
the response code indicating that more data is expected and continuing
the read as the code intended. Some timing delays are required to ensure
the chip has time to refill its FIFO as data is read in from the I2C
bus. This timing has been tested in my system when configured for bus
speeds of 50KHz, 100KHz, and 400KHz and operates well.
Signed-off-by: Hamish Martin <hamish.martin@alliedtelesis.co.nz>
---
drivers/hid/hid-mcp2221.c | 32 +++++++++++++++++++++++---------
1 file changed, 23 insertions(+), 9 deletions(-)
diff --git a/drivers/hid/hid-mcp2221.c b/drivers/hid/hid-mcp2221.c
index d0dd14cb4156..f9cceaeffd08 100644
--- a/drivers/hid/hid-mcp2221.c
+++ b/drivers/hid/hid-mcp2221.c
@@ -49,6 +49,7 @@ enum {
MCP2221_I2C_MASK_ADDR_NACK = 0x40,
MCP2221_I2C_WRADDRL_SEND = 0x21,
MCP2221_I2C_ADDR_NACK = 0x25,
+ MCP2221_I2C_READ_PARTIAL = 0x54,
MCP2221_I2C_READ_COMPL = 0x55,
MCP2221_ALT_F_NOT_GPIOV = 0xEE,
MCP2221_ALT_F_NOT_GPIOD = 0xEF,
@@ -297,6 +298,7 @@ static int mcp_i2c_smbus_read(struct mcp2221 *mcp,
{
int ret;
u16 total_len;
+ int retries = 0;
mcp->txbuf[0] = type;
if (msg) {
@@ -320,20 +322,31 @@ static int mcp_i2c_smbus_read(struct mcp2221 *mcp,
mcp->rxbuf_idx = 0;
do {
+ /* Wait for the data to be read by the device */
+ usleep_range(980, 1000);
+
memset(mcp->txbuf, 0, 4);
mcp->txbuf[0] = MCP2221_I2C_GET_DATA;
ret = mcp_send_data_req_status(mcp, mcp->txbuf, 1);
- if (ret)
- return ret;
-
- ret = mcp_chk_last_cmd_status_free_bus(mcp);
- if (ret)
- return ret;
-
- usleep_range(980, 1000);
+ if (ret) {
+ if (retries < 5) {
+ /* The data wasn't ready to read.
+ * Wait a bit longer and try again.
+ */
+ usleep_range(90, 100);
+ retries++;
+ } else {
+ return ret;
+ }
+ } else {
+ retries = 0;
+ }
} while (mcp->rxbuf_idx < total_len);
+ usleep_range(980, 1000);
+ ret = mcp_chk_last_cmd_status_free_bus(mcp);
+
return ret;
}
@@ -799,7 +812,8 @@ static int mcp2221_raw_event(struct hid_device *hdev,
mcp->status = -EIO;
break;
}
- if (data[2] == MCP2221_I2C_READ_COMPL) {
+ if (data[2] == MCP2221_I2C_READ_COMPL ||
+ data[2] == MCP2221_I2C_READ_PARTIAL) {
buf = mcp->rxbuf;
memcpy(&buf[mcp->rxbuf_idx], &data[4], data[3]);
mcp->rxbuf_idx = mcp->rxbuf_idx + data[3];
--
2.42.0
^ permalink raw reply related
* [PATCH 4/5] HID: mcp2221: Don't set bus speed on every transfer
From: Hamish Martin @ 2023-10-25 3:55 UTC (permalink / raw)
To: gupt21, jikos, benjamin.tissoires, Enrik.Berkhan, sven.zuehlsdorf
Cc: linux-i2c, linux-input, Hamish Martin
In-Reply-To: <20231025035514.3450123-1-hamish.martin@alliedtelesis.co.nz>
Since the initial commit of this driver the I2C bus speed has been
reconfigured for every single transfer. This is despite the fact that we
never change the speed and it is never "lost" by the chip.
Upon investigation we find that what was really happening was that the
setting of the bus speed had the side effect of cancelling a previous
failed command if there was one, thereby freeing the bus. This is the
part that was actually required to keep the bus operational in the face
of failed commands.
Instead of always setting the speed, we now correctly cancel any failed
commands as they are detected. This means we can just set the bus speed
at probe time and remove the previous speed sets on each transfer.
This has the effect of improving performance and reducing the number of
commands required to complete transfers.
Signed-off-by: Hamish Martin <hamish.martin@alliedtelesis.co.nz>
---
drivers/hid/hid-mcp2221.c | 41 ++++++++++++++++++++++++++-------------
1 file changed, 27 insertions(+), 14 deletions(-)
diff --git a/drivers/hid/hid-mcp2221.c b/drivers/hid/hid-mcp2221.c
index a219cd2e3309..d0dd14cb4156 100644
--- a/drivers/hid/hid-mcp2221.c
+++ b/drivers/hid/hid-mcp2221.c
@@ -187,6 +187,25 @@ static int mcp_cancel_last_cmd(struct mcp2221 *mcp)
return mcp_send_data_req_status(mcp, mcp->txbuf, 8);
}
+/* Check if the last command succeeded or failed and return the result.
+ * If the command did fail, cancel that command which will free the i2c bus.
+ */
+static int mcp_chk_last_cmd_status_free_bus(struct mcp2221 *mcp)
+{
+ int ret;
+
+ ret = mcp_chk_last_cmd_status(mcp);
+ if (ret) {
+ /* The last command was a failure.
+ * Send a cancel which will also free the bus.
+ */
+ usleep_range(980, 1000);
+ mcp_cancel_last_cmd(mcp);
+ }
+
+ return ret;
+}
+
static int mcp_set_i2c_speed(struct mcp2221 *mcp)
{
int ret;
@@ -241,7 +260,7 @@ static int mcp_i2c_write(struct mcp2221 *mcp,
usleep_range(980, 1000);
if (last_status) {
- ret = mcp_chk_last_cmd_status(mcp);
+ ret = mcp_chk_last_cmd_status_free_bus(mcp);
if (ret)
return ret;
}
@@ -308,7 +327,7 @@ static int mcp_i2c_smbus_read(struct mcp2221 *mcp,
if (ret)
return ret;
- ret = mcp_chk_last_cmd_status(mcp);
+ ret = mcp_chk_last_cmd_status_free_bus(mcp);
if (ret)
return ret;
@@ -328,11 +347,6 @@ static int mcp_i2c_xfer(struct i2c_adapter *adapter,
mutex_lock(&mcp->lock);
- /* Setting speed before every transaction is required for mcp2221 */
- ret = mcp_set_i2c_speed(mcp);
- if (ret)
- goto exit;
-
if (num == 1) {
if (msgs->flags & I2C_M_RD) {
ret = mcp_i2c_smbus_read(mcp, msgs, MCP2221_I2C_RD_DATA,
@@ -417,9 +431,7 @@ static int mcp_smbus_write(struct mcp2221 *mcp, u16 addr,
if (last_status) {
usleep_range(980, 1000);
- ret = mcp_chk_last_cmd_status(mcp);
- if (ret)
- return ret;
+ ret = mcp_chk_last_cmd_status_free_bus(mcp);
}
return ret;
@@ -437,10 +449,6 @@ static int mcp_smbus_xfer(struct i2c_adapter *adapter, u16 addr,
mutex_lock(&mcp->lock);
- ret = mcp_set_i2c_speed(mcp);
- if (ret)
- goto exit;
-
switch (size) {
case I2C_SMBUS_QUICK:
@@ -1150,6 +1158,11 @@ static int mcp2221_probe(struct hid_device *hdev,
if (i2c_clk_freq < 50)
i2c_clk_freq = 50;
mcp->cur_i2c_clk_div = (12000000 / (i2c_clk_freq * 1000)) - 3;
+ ret = mcp_set_i2c_speed(mcp);
+ if (ret) {
+ hid_err(hdev, "can't set i2c speed: %d\n", ret);
+ return ret;
+ }
mcp->adapter.owner = THIS_MODULE;
mcp->adapter.class = I2C_CLASS_HWMON;
--
2.42.0
^ permalink raw reply related
* [PATCH 3/5] HID: mcp2221: Set ACPI companion
From: Hamish Martin @ 2023-10-25 3:55 UTC (permalink / raw)
To: gupt21, jikos, benjamin.tissoires, Enrik.Berkhan, sven.zuehlsdorf
Cc: linux-i2c, linux-input, Hamish Martin
In-Reply-To: <20231025035514.3450123-1-hamish.martin@alliedtelesis.co.nz>
In scenarios where an I2C device tree is defined in ACPI and exists off
the MCP2221 I2C bus, the devices could not be instantiated.
Mark the USB port that the MCP2221 is connected to as its ACPI companion
so that the USB device can be bound to the ACPI tree when enumerated.
With this change the downstream I2C tree devices can be instantiated on
ACPI systems.
Signed-off-by: Hamish Martin <hamish.martin@alliedtelesis.co.nz>
---
drivers/hid/hid-mcp2221.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/hid/hid-mcp2221.c b/drivers/hid/hid-mcp2221.c
index aef0785c91cc..a219cd2e3309 100644
--- a/drivers/hid/hid-mcp2221.c
+++ b/drivers/hid/hid-mcp2221.c
@@ -1156,6 +1156,7 @@ static int mcp2221_probe(struct hid_device *hdev,
mcp->adapter.algo = &mcp_i2c_algo;
mcp->adapter.retries = 1;
mcp->adapter.dev.parent = &hdev->dev;
+ ACPI_COMPANION_SET(&mcp->adapter.dev, ACPI_COMPANION(hdev->dev.parent));
snprintf(mcp->adapter.name, sizeof(mcp->adapter.name),
"MCP2221 usb-i2c bridge");
--
2.42.0
^ permalink raw reply related
* [PATCH 2/5] HID: mcp2221: Allow IO to start during probe
From: Hamish Martin @ 2023-10-25 3:55 UTC (permalink / raw)
To: gupt21, jikos, benjamin.tissoires, Enrik.Berkhan, sven.zuehlsdorf
Cc: linux-i2c, linux-input, Hamish Martin
In-Reply-To: <20231025035514.3450123-1-hamish.martin@alliedtelesis.co.nz>
During the probe we add an I2C adapter and as soon as we add that adapter
it may be used for a transfer (e.g via the code in i2cdetect()).
Those transfers are not able to complete and time out. This is because the
HID raw_event callback (mcp2221_raw_event) will not be invoked until the
HID device's 'driver_input_lock' is marked up at the completion of the
probe in hid_device_probe(). This starves the driver of the responses it
is waiting for.
In order to allow the I2C transfers to complete while we are still in the
probe, start the IO once we have completed init of the HID device.
This issue seems to have been seen before and a patch was submitted but
it seems it was never accepted. See:
https://lore.kernel.org/all/20221103222714.21566-3-Enrik.Berkhan@inka.de/
Signed-off-by: Hamish Martin <hamish.martin@alliedtelesis.co.nz>
---
drivers/hid/hid-mcp2221.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/hid/hid-mcp2221.c b/drivers/hid/hid-mcp2221.c
index b95f31cf0fa2..aef0785c91cc 100644
--- a/drivers/hid/hid-mcp2221.c
+++ b/drivers/hid/hid-mcp2221.c
@@ -1142,6 +1142,8 @@ static int mcp2221_probe(struct hid_device *hdev,
if (ret)
return ret;
+ hid_device_io_start(hdev);
+
/* Set I2C bus clock diviser */
if (i2c_clk_freq > 400)
i2c_clk_freq = 400;
--
2.42.0
^ 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