| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Tue, 20 Jun 2023 20:38:20 +0200
From: Neil Armstrong <neil.armstrong@...aro.org>
To: Jeff LaBundy <jeff@...undy.com>
Cc: Dmitry Torokhov <dmitry.torokhov@...il.com>,
Rob Herring <robh+dt@...nel.org>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
Conor Dooley <conor+dt@...nel.org>,
Bastien Nocera <hadess@...ess.net>,
Hans de Goede <hdegoede@...hat.com>,
Henrik Rydberg <rydberg@...math.org>,
linux-input@...r.kernel.org, linux-arm-msm@...r.kernel.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 2/4] input: touchscreen: add core support for Goodix
Berlin Touchscreen IC
On 16/06/2023 05:14, Jeff LaBundy wrote:
> Hi Neil,
>
> Great work; this driver cleaned up quite nicely. With the previous
> noise out of the way, I have left some more detailed comments which
> are relatively minor for the most part.
>
> On Thu, Jun 15, 2023 at 12:27:01PM +0200, Neil Armstrong wrote:
>> Add initial support for the new Goodix "Berlin" touchscreen ICs.
>>
>> These touchscreen ICs support SPI, I2C and I3C interface, up to
>> 10 finger touch, stylus and gestures events.
>>
>> This initial driver is derived from the Goodix goodix_ts_berlin
>> available at [1] and [2] and only supports the GT9916 IC
>> present on the Qualcomm SM8550 MTP & QRD touch panel.
>>
>> The current implementation only supports BerlinD, aka GT9916.
>>
>> Support for advanced features like:
>> - Firmware & config update
>> - Stylus events
>> - Gestures events
>> - Previous revisions support (BerlinA or BerlinB)
>> is not included in current version.
>>
>> The current support will work with currently flashed firmware
>> and config, and bail out if firmware or config aren't flashed yet.
>>
>> [1] https://github.com/goodix/goodix_ts_berlin
>> [2] https://git.codelinaro.org/clo/la/platform/vendor/opensource/touch-drivers
>>
>> Signed-off-by: Neil Armstrong <neil.armstrong@...aro.org>
>> ---
>> drivers/input/touchscreen/Kconfig | 5 +
>> drivers/input/touchscreen/Makefile | 1 +
>> drivers/input/touchscreen/goodix_berlin.h | 178 +++++++
>> drivers/input/touchscreen/goodix_berlin_core.c | 681 +++++++++++++++++++++++++
>> 4 files changed, 865 insertions(+)
>>
>> diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
>> index c2cbd332af1d..1a6f6f6da991 100644
>> --- a/drivers/input/touchscreen/Kconfig
>> +++ b/drivers/input/touchscreen/Kconfig
>> @@ -416,6 +416,11 @@ config TOUCHSCREEN_GOODIX
>> To compile this driver as a module, choose M here: the
>> module will be called goodix.
>>
>> +config TOUCHSCREEN_GOODIX_BERLIN_CORE
>> + depends on GPIOLIB || COMPILE_TEST
>
> No need to depend on GPIOLIB; all gpiod calls used in this driver define dummy
> functions for the !CONFIG_GPIOLIB case.
Ack
>
>> + depends on REGMAP
>> + tristate
>> +
>> config TOUCHSCREEN_HIDEEP
>> tristate "HiDeep Touch IC"
>> depends on I2C
>> diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile
>> index 159cd5136fdb..29cdb042e104 100644
>> --- a/drivers/input/touchscreen/Makefile
>> +++ b/drivers/input/touchscreen/Makefile
>> @@ -47,6 +47,7 @@ obj-$(CONFIG_TOUCHSCREEN_EGALAX_SERIAL) += egalax_ts_serial.o
>> obj-$(CONFIG_TOUCHSCREEN_EXC3000) += exc3000.o
>> obj-$(CONFIG_TOUCHSCREEN_FUJITSU) += fujitsu_ts.o
>> obj-$(CONFIG_TOUCHSCREEN_GOODIX) += goodix_ts.o
>> +obj-$(CONFIG_TOUCHSCREEN_GOODIX_BERLIN_CORE) += goodix_berlin_core.o
>> obj-$(CONFIG_TOUCHSCREEN_HIDEEP) += hideep.o
>> obj-$(CONFIG_TOUCHSCREEN_HYNITRON_CSTXXX) += hynitron_cstxxx.o
>> obj-$(CONFIG_TOUCHSCREEN_ILI210X) += ili210x.o
>> diff --git a/drivers/input/touchscreen/goodix_berlin.h b/drivers/input/touchscreen/goodix_berlin.h
>> new file mode 100644
>> index 000000000000..56c110d94dff
>> --- /dev/null
>> +++ b/drivers/input/touchscreen/goodix_berlin.h
>> @@ -0,0 +1,178 @@
>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>> +/*
>> + * Goodix Touchscreen Driver
>> + * Copyright (C) 2020 - 2021 Goodix, Inc.
>> + * Copyright (C) 2023 Linaro Ltd.
>> + *
>> + * Based on goodix_berlin_berlin driver.
>> + */
>> +
>> +#ifndef __GOODIX_BERLIN_H_
>> +#define __GOODIX_BERLIN_H_
>> +
>> +#include <linux/input.h>
>> +#include <linux/of_gpio.h>
>
> I believe this should have been <linux/gpio/consumer.h>.
Ack
>
>> +#include <linux/input/touchscreen.h>
>> +#include <linux/regulator/consumer.h>
>> +
>> +#define GOODIX_MAX_TOUCH 10
>> +
>> +#define GOODIX_NORMAL_RESET_DELAY_MS 100
>> +
>> +#define MAX_SCAN_FREQ_NUM 8
>> +#define MAX_SCAN_RATE_NUM 8
>> +#define MAX_FREQ_NUM_STYLUS 8
>> +
>> +#define IRQ_EVENT_HEAD_LEN 8
>> +#define BYTES_PER_POINT 8
>> +#define COOR_DATA_CHECKSUM_SIZE 2
>> +
>> +#define GOODIX_TOUCH_EVENT BIT(7)
>> +#define GOODIX_REQUEST_EVENT BIT(6)
>> +
>> +#define GOODIX_REQUEST_CODE_RESET 3
>> +
>> +#define POINT_TYPE_STYLUS_HOVER 0x01
>> +#define POINT_TYPE_STYLUS 0x03
>> +
>> +#define DEV_CONFIRM_VAL 0xAA
>> +#define BOOTOPTION_ADDR 0x10000
>> +#define FW_VERSION_INFO_ADDR 0x10014
>> +
>> +#define GOODIX_IC_INFO_MAX_LEN 1024
>> +#define GOODIX_IC_INFO_ADDR 0x10070
>> +
>> +struct goodix_berlin_fw_version {
>> + u8 rom_pid[6];
>> + u8 rom_vid[3];
>> + u8 rom_vid_reserved;
>> + u8 patch_pid[8];
>> + u8 patch_vid[4];
>> + u8 patch_vid_reserved;
>> + u8 sensor_id;
>> + u8 reserved[2];
>> + u16 checksum;
>> +} __packed;
>> +
>> +struct goodix_berlin_ic_info_version {
>> + u8 info_customer_id;
>> + u8 info_version_id;
>> + u8 ic_die_id;
>> + u8 ic_version_id;
>> + u32 config_id;
>> + u8 config_version;
>> + u8 frame_data_customer_id;
>> + u8 frame_data_version_id;
>> + u8 touch_data_customer_id;
>> + u8 touch_data_version_id;
>> + u8 reserved[3];
>> +} __packed;
>> +
>> +struct goodix_berlin_ic_info_feature {
>> + u16 freqhop_feature;
>> + u16 calibration_feature;
>> + u16 gesture_feature;
>> + u16 side_touch_feature;
>> + u16 stylus_feature;
>> +} __packed;
>> +
>> +struct goodix_berlin_ic_info_misc {
>> + u32 cmd_addr;
>> + u16 cmd_max_len;
>> + u32 cmd_reply_addr;
>> + u16 cmd_reply_len;
>> + u32 fw_state_addr;
>> + u16 fw_state_len;
>> + u32 fw_buffer_addr;
>> + u16 fw_buffer_max_len;
>> + u32 frame_data_addr;
>> + u16 frame_data_head_len;
>> + u16 fw_attr_len;
>> + u16 fw_log_len;
>> + u8 pack_max_num;
>> + u8 pack_compress_version;
>> + u16 stylus_struct_len;
>> + u16 mutual_struct_len;
>> + u16 self_struct_len;
>> + u16 noise_struct_len;
>> + u32 touch_data_addr;
>> + u16 touch_data_head_len;
>> + u16 point_struct_len;
>> + u16 reserved1;
>> + u16 reserved2;
>> + u32 mutual_rawdata_addr;
>> + u32 mutual_diffdata_addr;
>> + u32 mutual_refdata_addr;
>> + u32 self_rawdata_addr;
>> + u32 self_diffdata_addr;
>> + u32 self_refdata_addr;
>> + u32 iq_rawdata_addr;
>> + u32 iq_refdata_addr;
>> + u32 im_rawdata_addr;
>> + u16 im_readata_len;
>> + u32 noise_rawdata_addr;
>> + u16 noise_rawdata_len;
>> + u32 stylus_rawdata_addr;
>> + u16 stylus_rawdata_len;
>> + u32 noise_data_addr;
>> + u32 esd_addr;
>> +} __packed;
>> +
>> +enum goodix_berlin_ts_event_type {
>> + GOODIX_BERLIN_EVENT_INVALID,
>> + GOODIX_BERLIN_EVENT_TOUCH,
>> + GOODIX_BERLIN_EVENT_REQUEST,
>> +};
>> +
>> +enum goodix_berlin_ts_request_type {
>> + GOODIX_BERLIN_REQUEST_TYPE_INVALID,
>> + GOODIX_BERLIN_REQUEST_TYPE_RESET,
>> +};
>> +
>> +enum goodix_berlin_touch_point_status {
>> + GOODIX_BERLIN_TS_NONE,
>> + GOODIX_BERLIN_TS_TOUCH,
>> +};
>> +
>> +struct goodix_berlin_coords {
>> + enum goodix_berlin_touch_point_status status;
>> + unsigned int x, y, w, p;
>> +};
>> +
>> +struct goodix_berlin_touch_data {
>> + int touch_num;
>> + struct goodix_berlin_coords coords[GOODIX_MAX_TOUCH];
>> +};
>> +
>> +struct goodix_berlin_event {
>> + enum goodix_berlin_ts_event_type event_type;
>> + enum goodix_berlin_ts_request_type request_code;
>> + struct goodix_berlin_touch_data touch_data;
>> +};
>> +
>> +/* Runtime parameters extracted from goodix_berlin_ic_info */
>> +struct goodix_berlin_params {
>> + u32 touch_data_addr;
>> +};
>
> Is there any reason to wrap this single member in a struct? It seems like
> touch_data_addr can simply be a member of struct goodix_berlin_core; this
> would shorten references to it throughout as well.
I did it first but then moved it in a separate struct for future additions,
but I'll move it in the core struct.
>
>> +
>> +struct goodix_berlin_core {
>> + struct device *dev;
>> + struct regmap *regmap;
>> + struct regulator *avdd;
>> + struct regulator *iovdd;
>> + struct gpio_desc *reset_gpio;
>> + struct touchscreen_properties props;
>> + struct goodix_berlin_fw_version fw_version;
>> + struct goodix_berlin_params params;
>> + struct input_dev *input_dev;
>> + struct goodix_berlin_event ts_event;
>> + int irq;
>> + struct dentry *debugfs_root;
>
> This last member appears to be unused.
Oops, forgot this one.
>
>> +};
>> +
>> +int goodix_berlin_probe(struct device *dev, int irq, const struct input_id *id,
>> + struct regmap *regmap);
>> +
>> +extern const struct dev_pm_ops goodix_berlin_pm_ops;
>> +
>> +#endif
>> diff --git a/drivers/input/touchscreen/goodix_berlin_core.c b/drivers/input/touchscreen/goodix_berlin_core.c
>> new file mode 100644
>> index 000000000000..11788662722a
>> --- /dev/null
>> +++ b/drivers/input/touchscreen/goodix_berlin_core.c
>> @@ -0,0 +1,681 @@
>> +// SPDX-License-Identifier: GPL-2.0-or-later
>> +/*
>> + * Goodix Touchscreen Driver
>> + * Copyright (C) 2020 - 2021 Goodix, Inc.
>> + * Copyright (C) 2023 Linaro Ltd.
>> + *
>> + * Based on goodix_ts_berlin driver.
>> + */
>> +#include <linux/input/mt.h>
>> +#include <linux/input/touchscreen.h>
>> +#include <linux/regmap.h>
>> +
>> +#include "goodix_berlin.h"
>> +
>> +/*
>> + * Goodix "Berlin" Touchscreen ID driver
>> + *
>> + * Currently only handles Multitouch events with already
>> + * programmed firmware and "config" for "Revision D" Berlin IC.
>> + *
>> + * Support is missing for:
>> + * - ESD Management
>> + * - Firmware update/flashing
>> + * - "Config" update/flashing
>> + * - Stylus Events
>> + * - Gesture Events
>> + * - Support for older revisions (A, B & C)
>> + */
>> +
>> +static bool goodix_berlin_check_checksum(const u8 *data, int size)
>
> Based on how this function is used later, something along the lines of
> goodix_berlin_checksum_valid() may be more descriptive.
Ack
>
>> +{
>> + u32 cal_checksum = 0;
>> + u32 r_checksum = 0;
>
> This can probably just be:
>
> u16 r_checksum;
>
>> + u32 i;
>> +
>> + if (size < COOR_DATA_CHECKSUM_SIZE)
>> + return false;
>> +
>> + for (i = 0; i < size - COOR_DATA_CHECKSUM_SIZE; i++)
>> + cal_checksum += data[i];
>> +
>> + r_checksum += data[i++];
>> + r_checksum += (data[i] << 8);
>
> And then:
>
> r_checksum = put_unaligned_le16(data[i]);
>
> In which case you must #include <asm/unaligned.h>.
Ack
>
>> +
>> + return (cal_checksum & 0xFFFF) == r_checksum;
>> +}
>> +
>> +static bool goodix_berlin_is_dummy_data(struct goodix_berlin_core *cd,
>> + const u8 *data, int size)
>> +{
>> + int zero_count = 0;
>> + int ff_count = 0;
>
> 'zero_count' and 'ff_count' seem like odd variable names; the following
> seems cleaner:
>
> static bool goodix_berlin_is_dummy_data(...)
> {
> int i;
>
> /*
> * 0 and 0xff represent ____, so declare success the first time
> * we encounter neither.
> */
> for (i = 0; i < size; i++)
> if (data[i] > 0 && data[i] < 0xff)
> return false;
>
> return true;
> }
>
> ...with the comment filled in to clarify what is the significance of dummy
> data, if possible. Note that the caller already prints a message when this
> fails.
Ack
>
>> + int i;
>> +
>> + for (i = 0; i < size; i++) {
>> + if (data[i] == 0)
>> + zero_count++;
>> + else if (data[i] == 0xff)
>> + ff_count++;
>> + }
>> + if (zero_count == size || ff_count == size) {
>> + dev_warn(cd->dev, "warning data is all %s\n",
>> + zero_count == size ? "zero" : "0xff");
>> + return true;
>> + }
>> +
>> + return false;
>> +}
>> +
>> +static int goodix_berlin_dev_confirm(struct goodix_berlin_core *cd)
>> +{
>> + u8 tx_buf[8], rx_buf[8];
>> + int retry = 3;
>> + int error;
>> +
>> + memset(tx_buf, DEV_CONFIRM_VAL, sizeof(tx_buf));
>
> Please namespace this #define as well as the one below.
Ack
>
>> + while (retry--) {
>> + error = regmap_raw_write(cd->regmap, BOOTOPTION_ADDR, tx_buf,
>> + sizeof(tx_buf));
>> + if (error < 0)
>> + return error;
>
> This should just be:
>
> if (error)
> return error;
>
>> +
>> + error = regmap_raw_read(cd->regmap, BOOTOPTION_ADDR, rx_buf,
>> + sizeof(rx_buf));
>> + if (error < 0)
>> + return error;
>
> And here, as well as a few other places throughout.
Ack will do a complete check
>
>> +
>> + if (!memcmp(tx_buf, rx_buf, sizeof(tx_buf)))
>> + return 0;
>> +
>> + usleep_range(5000, 5100);
>> + }
>> +
>> + dev_err(cd->dev, "device confirm failed, rx_buf: %*ph\n", 8, rx_buf);
>> +
>> + return -EINVAL;
>> +}
>> +
>> +static int goodix_berlin_power_on(struct goodix_berlin_core *cd, bool on)
>> +{
>> + int error = 0;
>
> No need to initialize 'error' here.
Th refactor I did needs it to be initialized at 0 because the if() always calls return,
but yeah it's kind of ugly.
>
>> +
>> + if (on) {
>> + error = regulator_enable(cd->iovdd);
>> + if (error < 0) {
>> + dev_err(cd->dev, "Failed to enable iovdd: %d\n", error);
>> + return error;
>> + }
>> +
>> + /* Vendor waits 3ms for IOVDD to settle */
>> + usleep_range(3000, 3100);
>> +
>> + error = regulator_enable(cd->avdd);
>> + if (error < 0) {
>> + dev_err(cd->dev, "Failed to enable avdd: %d\n", error);
>> + goto power_off_iovdd;
>> + }
>> +
>> + /* Vendor waits 15ms for IOVDD to settle */
>> + usleep_range(15000, 15100);
>> +
>> + gpiod_set_value(cd->reset_gpio, 0);
>> +
>> + /* Vendor waits 4ms for Firmware to initialize */
>> + usleep_range(4000, 4100);
>> +
>> + error = goodix_berlin_dev_confirm(cd);
>> + if (error < 0)
>> + goto power_off_gpio;
>
> All of this cleaned up nicely. The following comment is idiomatic, but I feel
> the goto can be easily eliminated as follows:
>
> error = goodix_berlin_dev_confirm(cd);
> if (error)
> break;
Break ? in an if ?
>
> If you feel strongly otherwise, please consider a different label name beside
> 'power_off_gpio' as it is a bit confusing.
Ack
>
>> +
>> + /* Vendor waits 100ms for Firmware to fully boot */
>> + msleep(GOODIX_NORMAL_RESET_DELAY_MS);
>> +
>> + return 0;
>> + }
>> +
>> +power_off_gpio:
>> + gpiod_set_value(cd->reset_gpio, 1);
>> +
>> + regulator_disable(cd->avdd);
>> +
>> +power_off_iovdd:
>> + regulator_disable(cd->iovdd);
>> +
>> + return error;
>> +}
>> +
>> +static int goodix_berlin_read_version(struct goodix_berlin_core *cd)
>> +{
>> + u8 buf[sizeof(struct goodix_berlin_fw_version)];
>> + int retry = 2;
>> + int error;
>> +
>> + while (retry--) {
>> + error = regmap_raw_read(cd->regmap, FW_VERSION_INFO_ADDR, buf, sizeof(buf));
>> + if (error) {
>> + dev_dbg(cd->dev, "read fw version: %d, retry %d\n", error, retry);
>> + usleep_range(5000, 5100);
>> + continue;
>> + }
>> +
>> + if (goodix_berlin_check_checksum(buf, sizeof(buf)))
>> + break;
>> +
>> + dev_dbg(cd->dev, "invalid fw version: checksum error\n");
>> +
>> + error = -EINVAL;
>> +
>> + /* Do not sleep on the last try */
>> + if (retry)
>> + usleep_range(10000, 11000);
>
> This works, but do you reasonably expect to continue if the checksum is bad?
> Perhaps the device can still respond over I2C/SPI, but returns garbage data
> if the communication happens in the process of the device waking up?
Honestly I personnaly wouldn't retry, but the vendor code did this.
I personally would remove all the retries since the FW should respond
correctly at first time.
> If so, this may be even cleaner:
>
> if (retry)
> usleep_range(...);
> else
> error = -EINVAL;
>
>> + }
>> +
>> + if (error) {
>> + dev_err(cd->dev, "failed to get fw version");
>> + return error;
>> + }
>
> Again the following comment is idiomatic, but this seems cleaner:
>
> if (error)
> dev_err(...);
> else
> memcpy(...);
>
> return error;
>
> I do not feel strongly about it if you prefer the existing method.
>
>> +
>> + memcpy(&cd->fw_version, buf, sizeof(cd->fw_version));
>> +
>> + return 0;
>> +}
>> +
>> +/* Only extract necessary data for runtime */
>> +static int goodix_berlin_convert_ic_info(struct goodix_berlin_core *cd,
>> + const u8 *data, u16 length)
>
> Nit: these arguments do not seem aligned as below:
Forgot this one...
>
> +static int goodix_berlin_convert_ic_info(...,
> ...)
>
>> +{
>> + struct goodix_berlin_ic_info_misc misc;
>> + unsigned int offset = 0;
>> + u8 param_num;
>> +
>> + offset += sizeof(__le16); /* length */
>> + offset += sizeof(struct goodix_berlin_ic_info_version);
>> + offset += sizeof(struct goodix_berlin_ic_info_feature);
>> +
>> + /* goodix_berlin_ic_info_param, variable width structure */
>
> I don't see any need to make up this name 'goodix_berlin_ic_info_param' which
> is not defined anywhere else; a generic text description of what this area of
> memory represents seems fine.
Ack
>
>> + offset += 4 * sizeof(u8); /* drv_num, sen_num, button_num, force_num */
>> +
>> + if (offset >= length)
>> + goto invalid_offset;
>> +
>> + param_num = data[offset++]; /* active_scan_rate_num */
>> +
>> + offset += param_num * sizeof(__le16);
>> +
>> + if (offset >= length)
>> + goto invalid_offset;
>
> Do you actually need this check after every operation? It seems the error
> would be cumulative such that you could perform one check at the end, and
> potentially avoid a goto statement like below:
>
> offset += ...; /* foo */
>
> offset += ---; /* bar */
>
> if (offset > length) {
> dev_err(...);
> return -EINVAL);
> }
>
> return 0;
>
> In case I have misunderstood, please let me know.
Yes because the data is a follow-up of size followed by the elements,
so I need to read the size, to know where is the next size... and each
size can be garbage and lead to a buffer overflow.
>
>> +
>> + param_num = data[offset++]; /* mutual_freq_num*/
>> +
>> + offset += param_num * sizeof(__le16);
>> +
>> + if (offset >= length)
>> + goto invalid_offset;
>> +
>> + param_num = data[offset++]; /* self_tx_freq_num */
>> +
>> + offset += param_num * sizeof(__le16);
>> +
>> + if (offset >= length)
>> + goto invalid_offset;
>> +
>> + param_num = data[offset++]; /* self_rx_freq_num */
>> +
>> + offset += param_num * sizeof(__le16);
>> +
>> + if (offset >= length)
>> + goto invalid_offset;
>> +
>> + param_num = data[offset++]; /* stylus_freq_num */
>> +
>> + offset += param_num * sizeof(__le16);
>> +
>> + if (offset + sizeof(misc) > length)
>> + goto invalid_offset;
>> +
>> + /* goodix_berlin_ic_info_misc */
>> + memcpy(&misc, &data[offset], sizeof(misc));
>> +
>> + cd->params.touch_data_addr = le32_to_cpu(misc.touch_data_addr);
>> +
>> + return 0;
>> +
>> +invalid_offset:
>> + dev_err(cd->dev, "ic_info length is invalid (offset %d length %d)\n",
>> + offset, length);
>> + return -EINVAL;
>> +}
>> +
>> +static int goodix_berlin_get_ic_info(struct goodix_berlin_core *cd)
>> +{
>> + int error, i;
>> + u16 length = 0;
>> + u32 ic_addr;
>> + u8 afe_data[GOODIX_IC_INFO_MAX_LEN] = { 0 };
>
> No need to initialize this array.
Ack
>
>> +
>> + ic_addr = GOODIX_IC_INFO_ADDR;
>> +
>> + for (i = 0; i < 3; i++) {
>> + error = regmap_raw_read(cd->regmap, ic_addr, (u8 *)&length, sizeof(length));
>> + if (error) {
>> + dev_info(cd->dev, "failed get ic info length, %d\n", error);
>> + usleep_range(5000, 5100);
>> + continue;
>> + }
>> +
>> + length = le16_to_cpu(length);
>
> This seems incorrect; it seems like you mean to initialize the following:
>
> __le16 length_raw;
> u16 length;
>
> And then this becomes:
>
> length = le16_to_cpu(length_raw);
>
>> + if (length >= GOODIX_IC_INFO_MAX_LEN) {
>> + dev_info(cd->dev, "invalid ic info length %d, retry %d\n", length, i);
>> + continue;
>> + }
>> +
>> + error = regmap_raw_read(cd->regmap, ic_addr, afe_data, length);
>> + if (error) {
>> + dev_info(cd->dev, "failed get ic info data, %d\n", error);
>> + usleep_range(5000, 5100);
>> + continue;
>> + }
>> +
>> + /* check whether the data is valid (ex. bus default values) */
>> + if (goodix_berlin_is_dummy_data(cd, (const uint8_t *)afe_data, length)) {
>> + dev_info(cd->dev, "fw info data invalid\n");
>> + usleep_range(5000, 5100);
>> + continue;
>> + }
>> +
>> + if (!goodix_berlin_check_checksum((const uint8_t *)afe_data, length)) {
>> + dev_info(cd->dev, "fw info checksum error\n");
>> + usleep_range(5000, 5100);
>> + continue;
>> + }
>> +
>> + break;
>> + }
>
> Even after the loop gives up, we will still sleep for 5 ms; it seems we can
> simplify this. One option may be to offload the function calls into a helper
> like below:
>
> static int __goodix_berlin_get_ic_info(...);
> {
> /* ... */
>
> error = regmap_raw_read(...);
> if (error)
> return error;
>
> length = le16_to_cpu(...);
> if (...)
> return -EINVAL;
>
> if (goodix_berlin_is_dummy_data(...))
> return -EAGAIN;
>
> if (!goodix_berlin_check_checksum(...));
> return -EAGAIN;
>
> return 0;
> }
>
> ...and then:
>
> static int goodix_berlin_get_ic_info(...)
> {
> /* ... */
>
> for (i = 0; i = GOODIX_BERLIN_INFO_RETRIES; i++) {
> error = __goodix_berlin_get_ic_info(...);
> if (!error)
> break;
> elseif (error == -EINVAL)
> return error;
>
> usleep_range(...);
> }
>
> if (i == GOODIX_BERLIN_INFO_RETRIES) {
> dev_err(...);
> return -ETIMEDOUT;
> }
>
> /* ... */
>
> }
This looks better, I'll see what inspire me the most
>
>> + if (i == 3) {
>> + dev_err(cd->dev, "failed get ic info\n");
>> + return -EINVAL;
>> + }
>> +
>> + error = goodix_berlin_convert_ic_info(cd, afe_data, length);
>> + if (error) {
>> + dev_err(cd->dev, "error converting ic info\n");
>> + return error;
>> + }
>> +
>> + /* check some key info */
>> + if (!cd->params.touch_data_addr) {
>> + dev_err(cd->dev, "touch_data_addr is null\n");
>> + return -EINVAL;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int goodix_berlin_after_event_handler(struct goodix_berlin_core *cd)
>> +{
>> + u8 sync_clean = 0;
>> +
>> + return regmap_raw_write(cd->regmap, cd->params.touch_data_addr, &sync_clean, 1);
>
> Since your register data width is 8 bits, can this not simply be:
>
> regmap_write(cd->regmap, params.touch_data_addr, 0);
Indeed, let me check
>
>> +}
>> +
>> +static void goodix_berlin_parse_finger(struct goodix_berlin_core *cd,
>> + struct goodix_berlin_touch_data *touch_data,
>> + u8 *buf, int touch_num)
>> +{
>> + unsigned int id = 0, x = 0, y = 0, w = 0;
>
> No need to initialize these.
>
>> + u8 *coor_data;
>> + int i;
>> +
>> + coor_data = &buf[IRQ_EVENT_HEAD_LEN];
>> +
>> + for (i = 0; i < touch_num; i++) {
>> + id = (coor_data[0] >> 4) & 0x0F;
>> +
>> + if (id >= GOODIX_MAX_TOUCH) {
>> + dev_warn(cd->dev, "invalid finger id %d\n", id);
>> +
>> + touch_data->touch_num = 0;
>> + return;
>> + }
>
> It does not seem this check needs to happen in the loop.
>
>> +
>> + x = le16_to_cpup((__le16 *)(coor_data + 2));
>> + y = le16_to_cpup((__le16 *)(coor_data + 4));
>> + w = le16_to_cpup((__le16 *)(coor_data + 6));
>
> I got a bit lost here; this almost seems like there is an opportunity
> to define the coordinate layout as a __packed struct.
>
> It's also a bit confusing that all of this information is stored in
> the driver's private data and then passed around multiple functions.
> Please consider whether this logic can be simplified by consolidating
> it into one homogenous interrupt handler.
Yep, the whole interrupt handling needs a refactoring to simplify it,
there's no need to convert those data and pass them back and forth.
>
>> +
>> + touch_data->coords[id].status = GOODIX_BERLIN_TS_TOUCH;
>> + touch_data->coords[id].x = x;
>> + touch_data->coords[id].y = y;
>> + touch_data->coords[id].w = w;
>> +
>> + coor_data += BYTES_PER_POINT;
>> + }
>> +
>> + touch_data->touch_num = touch_num;
>> +}
>> +
>> +static int goodix_berlin_touch_handler(struct goodix_berlin_core *cd,
>> + u8 *pre_buf, u32 pre_buf_len)
>> +{
>> + static u8 buffer[IRQ_EVENT_HEAD_LEN + BYTES_PER_POINT * GOODIX_MAX_TOUCH + 2];
>> + struct goodix_berlin_touch_data *touch_data = &cd->ts_event.touch_data;
>> + u8 point_type = 0;
>> + u8 touch_num = 0;
>> + int error = 0;
>> +
>> + memset(&cd->ts_event, 0, sizeof(cd->ts_event));
>> +
>> + /* copy pre-data to buffer */
>> + memcpy(buffer, pre_buf, pre_buf_len);
>> +
>> + touch_num = buffer[2] & 0x0F;
>> +
>> + if (touch_num > GOODIX_MAX_TOUCH) {
>> + dev_warn(cd->dev, "invalid touch num %d\n", touch_num);
>> + return -EINVAL;
>> + }
>> +
>> + /* read more data if more than 2 touch events */
>> + if (unlikely(touch_num > 2)) {
>> + error = regmap_raw_read(cd->regmap,
>> + cd->params.touch_data_addr + pre_buf_len,
>> + &buffer[pre_buf_len],
>> + (touch_num - 2) * BYTES_PER_POINT);
>> + if (error) {
>> + dev_err(cd->dev, "failed get touch data\n");
>> + return error;
>> + }
>> + }
>> +
>> + goodix_berlin_after_event_handler(cd);
>> +
>> + if (!touch_num)
>> + goto out_touch_handler;
>> +
>> + point_type = buffer[IRQ_EVENT_HEAD_LEN] & 0x0F;
>> +
>> + if (point_type == POINT_TYPE_STYLUS || point_type == POINT_TYPE_STYLUS_HOVER) {
>> + dev_warn_once(cd->dev, "Stylus event type not handled\n");
>> + return 0;
>> + }
>> +
>> + if (!goodix_berlin_check_checksum(&buffer[IRQ_EVENT_HEAD_LEN],
>> + touch_num * BYTES_PER_POINT + 2)) {
>> + dev_dbg(cd->dev, "touch data checksum error\n");
>> + dev_dbg(cd->dev, "data: %*ph\n",
>> + touch_num * BYTES_PER_POINT + 2, &buffer[IRQ_EVENT_HEAD_LEN]);
>> + return -EINVAL;
>> + }
>> +
>> +out_touch_handler:
>> + cd->ts_event.event_type = GOODIX_BERLIN_EVENT_TOUCH;
>> + goodix_berlin_parse_finger(cd, touch_data, buffer, touch_num);
>> +
>> + return 0;
>> +}
>> +
>> +static int goodix_berlin_event_handler(struct goodix_berlin_core *cd)
>> +{
>> + int pre_read_len;
>> + u8 pre_buf[32];
>> + u8 event_status;
>> + int error;
>> +
>> + pre_read_len = IRQ_EVENT_HEAD_LEN + BYTES_PER_POINT * 2 +
>> + COOR_DATA_CHECKSUM_SIZE;
>> +
>> + error = regmap_raw_read(cd->regmap, cd->params.touch_data_addr, pre_buf,
>> + pre_read_len);
>> + if (error) {
>> + dev_err(cd->dev, "failed get event head data\n");
>> + return error;
>> + }
>> +
>> + if (pre_buf[0] == 0x00)
>> + return -EINVAL;
>> +
>> + if (!goodix_berlin_check_checksum(pre_buf, IRQ_EVENT_HEAD_LEN)) {
>> + dev_warn(cd->dev, "touch head checksum err : %*ph\n",
>> + IRQ_EVENT_HEAD_LEN, pre_buf);
>> + return -EINVAL;
>> + }
>> +
>> + event_status = pre_buf[0];
>> + if (event_status & GOODIX_TOUCH_EVENT)
>> + return goodix_berlin_touch_handler(cd, pre_buf, pre_read_len);
>> +
>> + if (event_status & GOODIX_REQUEST_EVENT) {
>> + cd->ts_event.event_type = GOODIX_BERLIN_EVENT_REQUEST;
>> + if (pre_buf[2] == GOODIX_REQUEST_CODE_RESET)
>> + cd->ts_event.request_code = GOODIX_BERLIN_REQUEST_TYPE_RESET;
>> + else
>> + dev_warn(cd->dev, "unsupported request code 0x%x\n", pre_buf[2]);
>> + }
>> +
>> + goodix_berlin_after_event_handler(cd);
>> +
>> + return 0;
>> +}
>> +
>> +static void goodix_berlin_report_finger(struct goodix_berlin_core *cd)
>> +{
>> + struct goodix_berlin_touch_data *touch_data = &cd->ts_event.touch_data;
>> + int i;
>> +
>> + mutex_lock(&cd->input_dev->mutex);
>
> I do not see any need for a mutex here; this function is only ever called from
> a threaded handler declared with IRQF_ONESHOT; it cannot become re-entrant. In
> case I have misunderstood, please let me know.
Ack
>
>> +
>> + for (i = 0; i < GOODIX_MAX_TOUCH; i++) {
>> + bool pressed = touch_data->coords[i].status == GOODIX_BERLIN_TS_TOUCH;
>> +
>> + input_mt_slot(cd->input_dev, i);
>> + input_mt_report_slot_state(cd->input_dev, MT_TOOL_FINGER, pressed);
>> +
>> + if (touch_data->coords[i].status == GOODIX_BERLIN_TS_TOUCH) {
>> + dev_dbg(cd->dev, "report: id[%d], x %d, y %d, w %d\n", i,
>> + touch_data->coords[i].x,
>> + touch_data->coords[i].y,
>> + touch_data->coords[i].w);
>> +
>> + touchscreen_report_pos(cd->input_dev, &cd->props,
>> + touch_data->coords[i].x,
>> + touch_data->coords[i].y, true);
>> + input_report_abs(cd->input_dev, ABS_MT_TOUCH_MAJOR,
>> + touch_data->coords[i].w);
>> + }
>> + }
>> +
>> + input_mt_sync_frame(cd->input_dev);
>> + input_sync(cd->input_dev);
>> +
>> + mutex_unlock(&cd->input_dev->mutex);
>> +}
>> +
>> +static int goodix_berlin_request_handle(struct goodix_berlin_core *cd)
>> +{
>> + /* TOFIX: Handle more request codes */
>> + if (cd->ts_event.request_code != GOODIX_BERLIN_REQUEST_TYPE_RESET) {
>> + dev_info(cd->dev, "can't handle request type 0x%x\n",
>> + cd->ts_event.request_code);
>> + return -EINVAL;
>> + }
>> +
>> + gpiod_set_value(cd->reset_gpio, 1);
>> + usleep_range(2000, 2100);
>> + gpiod_set_value(cd->reset_gpio, 0);
>> +
>> + msleep(GOODIX_NORMAL_RESET_DELAY_MS);
>> +
>> + return 0;
>> +}
>> +
>> +static irqreturn_t goodix_berlin_threadirq_func(int irq, void *data)
>> +{
>> + struct goodix_berlin_core *cd = data;
>> + int error;
>> +
>> + error = goodix_berlin_event_handler(cd);
>> + if (likely(!error)) {
>> + switch (cd->ts_event.event_type) {
>> + case GOODIX_BERLIN_EVENT_TOUCH:
>> + goodix_berlin_report_finger(cd);
>> + break;
>> +
>> + case GOODIX_BERLIN_EVENT_REQUEST:
>> + goodix_berlin_request_handle(cd);
>> + break;
>> +
>> + /* TOFIX: Handle more request types */
>> + case GOODIX_BERLIN_EVENT_INVALID:
>> + break;
>> + }
>> + }
>> +
>> + return IRQ_HANDLED;
>> +}
>> +
>> +static int goodix_berlin_input_dev_config(struct goodix_berlin_core *cd,
>> + const struct input_id *id)
>> +{
>> + struct input_dev *input_dev;
>> + int error;
>> +
>> + input_dev = devm_input_allocate_device(cd->dev);
>> + if (!input_dev)
>> + return -ENOMEM;
>> +
>> + cd->input_dev = input_dev;
>> + input_set_drvdata(input_dev, cd);
>> +
>> + input_dev->name = "Goodix Berlin Capacitive TouchScreen";
>> + input_dev->phys = "input/ts";
>> + input_dev->dev.parent = cd->dev;
>
> This last line is not necessary as devm_input_allocate_device() already
> does it for us.
>
>> +
>> + memcpy(&input_dev->id, id, sizeof(*id));
>
> I do not think deep copy is necessary here; please let me know in case I
> have misunderstood.
Hmm, ok yeah
>
>> +
>> + /* Set input parameters */
>
> This comment is not necessary.
>
>> + input_set_abs_params(cd->input_dev, ABS_MT_POSITION_X, 0, SZ_64K - 1, 0, 0);
>> + input_set_abs_params(cd->input_dev, ABS_MT_POSITION_Y, 0, SZ_64K - 1, 0, 0);
>> + input_set_abs_params(cd->input_dev, ABS_MT_TOUCH_MAJOR, 0, 255, 0, 0);
>> +
>> + touchscreen_parse_properties(cd->input_dev, true, &cd->props);
>> +
>> + error = input_mt_init_slots(cd->input_dev, GOODIX_MAX_TOUCH, INPUT_MT_DIRECT);
>> + if (error)
>> + return error;
>> +
>> + return input_register_device(cd->input_dev);
>> +}
>> +
>> +static int goodix_berlin_pm_suspend(struct device *dev)
>> +{
>> + struct goodix_berlin_core *cd = dev_get_drvdata(dev);
>> +
>> + disable_irq(cd->irq);
>> +
>> + return goodix_berlin_power_on(cd, false);
>> +}
>> +
>> +static int goodix_berlin_pm_resume(struct device *dev)
>> +{
>> + struct goodix_berlin_core *cd = dev_get_drvdata(dev);
>> + int error;
>> +
>> + error = goodix_berlin_power_on(cd, true);
>> + if (error)
>> + return error;
>> +
>> + enable_irq(cd->irq);
>> +
>> + return 0;
>> +}
>> +
>> +EXPORT_GPL_SIMPLE_DEV_PM_OPS(goodix_berlin_pm_ops,
>> + goodix_berlin_pm_suspend,
>> + goodix_berlin_pm_resume);
>> +
>> +static void goodix_berlin_power_off(void *data)
>> +{
>> + struct goodix_berlin_core *cd = data;
>> +
>> + goodix_berlin_power_on(cd, false);
>> +}
>> +
>> +int goodix_berlin_probe(struct device *dev, int irq, const struct input_id *id,
>> + struct regmap *regmap)
>> +{
>> + struct goodix_berlin_core *cd;
>> + int error;
>> +
>> + if (irq <= 0)
>> + return -EINVAL;
>
> Please include a dev_err() here.
Ack
>
>> +
>> + cd = devm_kzalloc(dev, sizeof(*cd), GFP_KERNEL);
>> + if (!cd)
>> + return -ENOMEM;
>> +
>> + cd->dev = dev;
>> + cd->regmap = regmap;
>> + cd->irq = irq;
>> +
>> + cd->reset_gpio = devm_gpiod_get_optional(cd->dev, "reset", GPIOF_OUT_INIT_HIGH);
>
> You can simply use 'dev' instead of 'cd->dev' here and throughout.
Ack
>
>> + if (IS_ERR(cd->reset_gpio))
>> + return dev_err_probe(cd->dev, PTR_ERR(cd->reset_gpio),
>> + "Failed to request reset gpio\n");
>> +
>> + cd->avdd = devm_regulator_get(cd->dev, "avdd");
>> + if (IS_ERR(cd->avdd))
>> + return dev_err_probe(cd->dev, PTR_ERR(cd->avdd),
>> + "Failed to request avdd regulator\n");
>> +
>> + cd->iovdd = devm_regulator_get(cd->dev, "iovdd");
>> + if (IS_ERR(cd->iovdd))
>> + return dev_err_probe(cd->dev, PTR_ERR(cd->iovdd),
>> + "Failed to request iovdd regulator\n");
>> +
>> + error = goodix_berlin_input_dev_config(cd, id);
>> + if (error < 0) {
>> + dev_err(cd->dev, "failed set input device");
>> + return error;
>> + }
>
> I recommend calling this after goodix_berlin_get_ic_info(); no need to go
> through all this trouble if the device is not there.
Ack
>
>> +
>> + error = devm_request_threaded_irq(dev, irq, NULL,
>> + goodix_berlin_threadirq_func,
>> + IRQF_ONESHOT, "goodix-berlin", cd);
>> + if (error) {
>> + dev_err(dev, "request threaded irq failed: %d\n", error);
>> + return error;
>> + }
>
> Typically, the interrupt should be the last resource to be registered. At
> this stage the device is not powered and the IRQ pin may be in an invalid
> state.
Ack
>
>> +
>> + dev_set_drvdata(dev, cd);
>> +
>> + error = goodix_berlin_power_on(cd, true);
>> + if (error) {
>> + dev_err(cd->dev, "failed power on");
>> + return error;
>> + }
>> +
>> + error = devm_add_action_or_reset(dev, goodix_berlin_power_off, cd);
>> + if (error)
>> + return error;
>> +
>> + error = goodix_berlin_read_version(cd);
>> + if (error < 0) {
>
> if (error)
>
>> + dev_err(cd->dev, "failed to get version info");
>> + return error;
>> + }
>> +
>> + error = goodix_berlin_get_ic_info(cd);
>> + if (error) {
>> + dev_err(cd->dev, "invalid ic info, abort");
>> + return error;
>> + }
>> +
>> + dev_dbg(cd->dev, "Goodix Berlin %s Touchscreen Controller", cd->fw_version.patch_pid);
>> +
>> + return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(goodix_berlin_probe);
>> +
>> +MODULE_LICENSE("GPL");
>> +MODULE_DESCRIPTION("Goodix Berlin Core Touchscreen driver");
>> +MODULE_AUTHOR("Neil Armstrong <neil.armstrong@...aro.org>");
>>
>> --
>> 2.34.1
>>
>
> Kind regards,
> Jeff LaBundy
Thanks for the in-depth review !
Neil
Powered by blists - more mailing lists