Re: [PATCH v12 2/4] Input: add core support for Goodix Berlin Touchscreen IC

[Dmitry Torokhov <dmitry.torokhov@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 1920 bytes --]

Hi Neil,

On Sat, Dec 09, 2023 at 08:33:40AM +0100, 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
> 
> Reviewed-by: Jeff LaBundy <jeff@labundy.com>
> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>

Thank you for resending the patch. I think there is an issue in how you
read and parse the data in case of more than 2 fingers. It looks like in
that case you are overwriting the checksum form the first 2 and then not
reading the new checksum but use some garbage past the touch data. I
might be mistaken though...

I also believe you are leaking afe_data in case of success. We have the
newfangled __free(kfree) from cleanup.h that should help there.

Another request - we should not have anything in goodix_berlin.h that is
not used by the I2C and SPI sub-drivers, so the only thing it should
contain is goodix_berlin_probe() declaration and dev_pm_ops. All other
defines and definitions should go to goodix_berlin_core.h.

I made a few more cosmetic changes in the attached patch, please
consider applying it.

Thanks.

-- 
Dmitry

[-- Attachment #2: tmp.patch --]
[-- Type: text/x-diff, Size: 21179 bytes --]

diff --git a/drivers/input/touchscreen/goodix_berlin.h b/drivers/input/touchscreen/goodix_berlin.h
index 235f44947a28..1fd77eb69c9a 100644
--- a/drivers/input/touchscreen/goodix_berlin.h
+++ b/drivers/input/touchscreen/goodix_berlin.h
@@ -10,146 +10,11 @@
 #ifndef __GOODIX_BERLIN_H_
 #define __GOODIX_BERLIN_H_
 
-#include <linux/gpio/consumer.h>
-#include <linux/input.h>
-#include <linux/input/touchscreen.h>
-#include <linux/regulator/consumer.h>
-#include <linux/sizes.h>
+#include <linux/pm.h>
 
-#define GOODIX_BERLIN_MAX_TOUCH			10
-
-#define GOODIX_BERLIN_NORMAL_RESET_DELAY_MS	100
-
-#define GOODIX_BERLIN_IRQ_EVENT_HEAD_LEN	8
-#define GOODIX_BERLIN_STATUS_OFFSET		0
-#define GOODIX_BERLIN_REQUEST_TYPE_OFFSET	2
-
-#define GOODIX_BERLIN_BYTES_PER_POINT		8
-#define GOODIX_BERLIN_COOR_DATA_CHECKSUM_SIZE	2
-#define GOODIX_BERLIN_COOR_DATA_CHECKSUM_MASK	GENMASK(15, 0)
-
-/* Read n finger events */
-#define GOODIX_BERLIN_IRQ_READ_LEN(n)		(GOODIX_BERLIN_IRQ_EVENT_HEAD_LEN + \
-						 (GOODIX_BERLIN_BYTES_PER_POINT * (n)) + \
-						 GOODIX_BERLIN_COOR_DATA_CHECKSUM_SIZE)
-
-#define GOODIX_BERLIN_TOUCH_EVENT		BIT(7)
-#define GOODIX_BERLIN_REQUEST_EVENT		BIT(6)
-#define GOODIX_BERLIN_TOUCH_COUNT_MASK		GENMASK(3, 0)
-
-#define GOODIX_BERLIN_REQUEST_CODE_RESET	3
-
-#define GOODIX_BERLIN_POINT_TYPE_MASK		GENMASK(3, 0)
-#define GOODIX_BERLIN_POINT_TYPE_STYLUS_HOVER	1
-#define GOODIX_BERLIN_POINT_TYPE_STYLUS		3
-
-#define GOODIX_BERLIN_TOUCH_ID_MASK		GENMASK(7, 4)
-
-#define GOODIX_BERLIN_DEV_CONFIRM_VAL		0xAA
-#define GOODIX_BERLIN_BOOTOPTION_ADDR		0x10000
-#define GOODIX_BERLIN_FW_VERSION_INFO_ADDR	0x10014
-
-#define GOODIX_BERLIN_IC_INFO_MAX_LEN		SZ_1K
-#define GOODIX_BERLIN_IC_INFO_ADDR		0x10070
-
-struct goodix_berlin_fw_version {
-	u8 rom_pid[6];
-	u8 rom_vid[3];
-	u8 rom_vid_reserved;
-	u8 patch_pid[8];
-	u8 patch_vid[4];
-	u8 patch_vid_reserved;
-	u8 sensor_id;
-	u8 reserved[2];
-	__le16 checksum;
-} __packed;
-
-struct goodix_berlin_ic_info_version {
-	u8 info_customer_id;
-	u8 info_version_id;
-	u8 ic_die_id;
-	u8 ic_version_id;
-	__le32 config_id;
-	u8 config_version;
-	u8 frame_data_customer_id;
-	u8 frame_data_version_id;
-	u8 touch_data_customer_id;
-	u8 touch_data_version_id;
-	u8 reserved[3];
-} __packed;
-
-struct goodix_berlin_ic_info_feature {
-	__le16 freqhop_feature;
-	__le16 calibration_feature;
-	__le16 gesture_feature;
-	__le16 side_touch_feature;
-	__le16 stylus_feature;
-} __packed;
-
-struct goodix_berlin_ic_info_misc {
-	__le32 cmd_addr;
-	__le16 cmd_max_len;
-	__le32 cmd_reply_addr;
-	__le16 cmd_reply_len;
-	__le32 fw_state_addr;
-	__le16 fw_state_len;
-	__le32 fw_buffer_addr;
-	__le16 fw_buffer_max_len;
-	__le32 frame_data_addr;
-	__le16 frame_data_head_len;
-	__le16 fw_attr_len;
-	__le16 fw_log_len;
-	u8 pack_max_num;
-	u8 pack_compress_version;
-	__le16 stylus_struct_len;
-	__le16 mutual_struct_len;
-	__le16 self_struct_len;
-	__le16 noise_struct_len;
-	__le32 touch_data_addr;
-	__le16 touch_data_head_len;
-	__le16 point_struct_len;
-	__le16 reserved1;
-	__le16 reserved2;
-	__le32 mutual_rawdata_addr;
-	__le32 mutual_diffdata_addr;
-	__le32 mutual_refdata_addr;
-	__le32 self_rawdata_addr;
-	__le32 self_diffdata_addr;
-	__le32 self_refdata_addr;
-	__le32 iq_rawdata_addr;
-	__le32 iq_refdata_addr;
-	__le32 im_rawdata_addr;
-	__le16 im_readata_len;
-	__le32 noise_rawdata_addr;
-	__le16 noise_rawdata_len;
-	__le32 stylus_rawdata_addr;
-	__le16 stylus_rawdata_len;
-	__le32 noise_data_addr;
-	__le32 esd_addr;
-} __packed;
-
-struct goodix_berlin_touch_data {
-	u8 id;
-	u8 unused;
-	__le16 x;
-	__le16 y;
-	__le16 w;
-} __packed;
-
-struct goodix_berlin_core {
-	struct device *dev;
-	struct regmap *regmap;
-	struct regulator *avdd;
-	struct regulator *iovdd;
-	struct gpio_desc *reset_gpio;
-	struct touchscreen_properties props;
-	struct goodix_berlin_fw_version fw_version;
-	struct input_dev *input_dev;
-	int irq;
-
-	/* Runtime parameters extracted from IC_INFO buffer  */
-	u32 touch_data_addr;
-};
+struct device;
+struct input_id;
+struct regmap;
 
 int goodix_berlin_probe(struct device *dev, int irq, const struct input_id *id,
 			struct regmap *regmap);
diff --git a/drivers/input/touchscreen/goodix_berlin_core.c b/drivers/input/touchscreen/goodix_berlin_core.c
index c66e2f0c6529..88a88e77d940 100644
--- a/drivers/input/touchscreen/goodix_berlin_core.c
+++ b/drivers/input/touchscreen/goodix_berlin_core.c
@@ -1,28 +1,17 @@
 // SPDX-License-Identifier: GPL-2.0-or-later
 /*
- * Goodix Touchscreen Driver
+ * Goodix "Berlin" Touchscreen IC driver
  * Copyright (C) 2020 - 2021 Goodix, Inc.
  * Copyright (C) 2023 Linaro Ltd.
  *
  * Based on goodix_ts_berlin driver.
- */
-#include <asm/unaligned.h>
-#include <linux/bitfield.h>
-#include <linux/input/mt.h>
-#include <linux/input/touchscreen.h>
-#include <linux/regmap.h>
-
-#include "goodix_berlin.h"
-
-/*
- * Goodix "Berlin" Touchscreen IC driver
  *
  * This driver is distinct from goodix.c since hardware interface
  * is different enough to require a new driver.
  * None of the register address or data structure are close enough
  * to the previous generations.
  *
- * Currently only handles Multitouch events with already
+ * Currently the driver only handles Multitouch events with already
  * programmed firmware and "config" for "Revision D" Berlin IC.
  *
  * Support is missing for:
@@ -34,6 +23,153 @@
  * - Support for older revisions (A & B)
  */
 
+#include <linux/bitfield.h>
+#include <linux/gpio/consumer.h>
+#include <linux/input.h>
+#include <linux/input/mt.h>
+#include <linux/input/touchscreen.h>
+#include <linux/regmap.h>
+#include <linux/regulator/consumer.h>
+#include <linux/sizes.h>
+#include <asm/unaligned.h>
+
+#include "goodix_berlin.h"
+
+#define GOODIX_BERLIN_MAX_TOUCH			10
+
+#define GOODIX_BERLIN_NORMAL_RESET_DELAY_MS	100
+
+#define GOODIX_BERLIN_IRQ_EVENT_HEAD_LEN	8
+#define GOODIX_BERLIN_STATUS_OFFSET		0
+#define GOODIX_BERLIN_REQUEST_TYPE_OFFSET	2
+
+#define GOODIX_BERLIN_BYTES_PER_POINT		8
+#define GOODIX_BERLIN_COOR_DATA_CHECKSUM_SIZE	2
+#define GOODIX_BERLIN_COOR_DATA_CHECKSUM_MASK	GENMASK(15, 0)
+
+/* Read n finger events */
+#define GOODIX_BERLIN_IRQ_READ_LEN(n)		(GOODIX_BERLIN_IRQ_EVENT_HEAD_LEN + \
+						 (GOODIX_BERLIN_BYTES_PER_POINT * (n)) + \
+						 GOODIX_BERLIN_COOR_DATA_CHECKSUM_SIZE)
+
+#define GOODIX_BERLIN_TOUCH_EVENT		BIT(7)
+#define GOODIX_BERLIN_REQUEST_EVENT		BIT(6)
+#define GOODIX_BERLIN_TOUCH_COUNT_MASK		GENMASK(3, 0)
+
+#define GOODIX_BERLIN_REQUEST_CODE_RESET	3
+
+#define GOODIX_BERLIN_POINT_TYPE_MASK		GENMASK(3, 0)
+#define GOODIX_BERLIN_POINT_TYPE_STYLUS_HOVER	1
+#define GOODIX_BERLIN_POINT_TYPE_STYLUS		3
+
+#define GOODIX_BERLIN_TOUCH_ID_MASK		GENMASK(7, 4)
+
+#define GOODIX_BERLIN_DEV_CONFIRM_VAL		0xAA
+#define GOODIX_BERLIN_BOOTOPTION_ADDR		0x10000
+#define GOODIX_BERLIN_FW_VERSION_INFO_ADDR	0x10014
+
+#define GOODIX_BERLIN_IC_INFO_MAX_LEN		SZ_1K
+#define GOODIX_BERLIN_IC_INFO_ADDR		0x10070
+
+struct goodix_berlin_fw_version {
+	u8 rom_pid[6];
+	u8 rom_vid[3];
+	u8 rom_vid_reserved;
+	u8 patch_pid[8];
+	u8 patch_vid[4];
+	u8 patch_vid_reserved;
+	u8 sensor_id;
+	u8 reserved[2];
+	__le16 checksum;
+} __packed;
+
+struct goodix_berlin_ic_info_version {
+	u8 info_customer_id;
+	u8 info_version_id;
+	u8 ic_die_id;
+	u8 ic_version_id;
+	__le32 config_id;
+	u8 config_version;
+	u8 frame_data_customer_id;
+	u8 frame_data_version_id;
+	u8 touch_data_customer_id;
+	u8 touch_data_version_id;
+	u8 reserved[3];
+} __packed;
+
+struct goodix_berlin_ic_info_feature {
+	__le16 freqhop_feature;
+	__le16 calibration_feature;
+	__le16 gesture_feature;
+	__le16 side_touch_feature;
+	__le16 stylus_feature;
+} __packed;
+
+struct goodix_berlin_ic_info_misc {
+	__le32 cmd_addr;
+	__le16 cmd_max_len;
+	__le32 cmd_reply_addr;
+	__le16 cmd_reply_len;
+	__le32 fw_state_addr;
+	__le16 fw_state_len;
+	__le32 fw_buffer_addr;
+	__le16 fw_buffer_max_len;
+	__le32 frame_data_addr;
+	__le16 frame_data_head_len;
+	__le16 fw_attr_len;
+	__le16 fw_log_len;
+	u8 pack_max_num;
+	u8 pack_compress_version;
+	__le16 stylus_struct_len;
+	__le16 mutual_struct_len;
+	__le16 self_struct_len;
+	__le16 noise_struct_len;
+	__le32 touch_data_addr;
+	__le16 touch_data_head_len;
+	__le16 point_struct_len;
+	__le16 reserved1;
+	__le16 reserved2;
+	__le32 mutual_rawdata_addr;
+	__le32 mutual_diffdata_addr;
+	__le32 mutual_refdata_addr;
+	__le32 self_rawdata_addr;
+	__le32 self_diffdata_addr;
+	__le32 self_refdata_addr;
+	__le32 iq_rawdata_addr;
+	__le32 iq_refdata_addr;
+	__le32 im_rawdata_addr;
+	__le16 im_readata_len;
+	__le32 noise_rawdata_addr;
+	__le16 noise_rawdata_len;
+	__le32 stylus_rawdata_addr;
+	__le16 stylus_rawdata_len;
+	__le32 noise_data_addr;
+	__le32 esd_addr;
+} __packed;
+
+struct goodix_berlin_touch_data {
+	u8 id;
+	u8 unused;
+	__le16 x;
+	__le16 y;
+	__le16 w;
+} __packed;
+
+struct goodix_berlin_core {
+	struct device *dev;
+	struct regmap *regmap;
+	struct regulator *avdd;
+	struct regulator *iovdd;
+	struct gpio_desc *reset_gpio;
+	struct touchscreen_properties props;
+	struct goodix_berlin_fw_version fw_version;
+	struct input_dev *input_dev;
+	int irq;
+
+	/* Runtime parameters extracted from IC_INFO buffer  */
+	u32 touch_data_addr;
+};
+
 static bool goodix_berlin_checksum_valid(const u8 *data, int size)
 {
 	u32 cal_checksum = 0;
@@ -46,9 +182,11 @@ static bool goodix_berlin_checksum_valid(const u8 *data, int size)
 	for (i = 0; i < size - GOODIX_BERLIN_COOR_DATA_CHECKSUM_SIZE; i++)
 		cal_checksum += data[i];
 
+	cal_checksum = FIELD_GET(GOODIX_BERLIN_COOR_DATA_CHECKSUM_MASK,
+				 cal_checksum);
 	r_checksum = get_unaligned_le16(&data[i]);
 
-	return FIELD_GET(GOODIX_BERLIN_COOR_DATA_CHECKSUM_MASK, cal_checksum) == r_checksum;
+	return cal_checksum == r_checksum;
 }
 
 static bool goodix_berlin_is_dummy_data(struct goodix_berlin_core *cd,
@@ -76,13 +214,15 @@ static int goodix_berlin_dev_confirm(struct goodix_berlin_core *cd)
 
 	memset(tx_buf, GOODIX_BERLIN_DEV_CONFIRM_VAL, sizeof(tx_buf));
 	while (retry--) {
-		error = regmap_raw_write(cd->regmap, GOODIX_BERLIN_BOOTOPTION_ADDR, tx_buf,
-					 sizeof(tx_buf));
+		error = regmap_raw_write(cd->regmap,
+					 GOODIX_BERLIN_BOOTOPTION_ADDR,
+					 tx_buf, sizeof(tx_buf));
 		if (error)
 			return error;
 
-		error = regmap_raw_read(cd->regmap, GOODIX_BERLIN_BOOTOPTION_ADDR, rx_buf,
-					sizeof(rx_buf));
+		error = regmap_raw_read(cd->regmap,
+					GOODIX_BERLIN_BOOTOPTION_ADDR,
+					rx_buf, sizeof(rx_buf));
 		if (error)
 			return error;
 
@@ -92,66 +232,70 @@ static int goodix_berlin_dev_confirm(struct goodix_berlin_core *cd)
 		usleep_range(5000, 5100);
 	}
 
-	dev_err(cd->dev, "device confirm failed, rx_buf: %*ph\n", 8, rx_buf);
+	dev_err(cd->dev, "device confirm failed, rx_buf: %*ph\n",
+		(int)sizeof(rx_buf), rx_buf);
 
 	return -EINVAL;
 }
 
-static int goodix_berlin_power_on(struct goodix_berlin_core *cd, bool on)
+static int goodix_berlin_power_on(struct goodix_berlin_core *cd)
 {
-	int error = 0;
+	int error;
 
-	if (on) {
-		error = regulator_enable(cd->iovdd);
-		if (error) {
-			dev_err(cd->dev, "Failed to enable iovdd: %d\n", error);
-			return error;
-		}
+	error = regulator_enable(cd->iovdd);
+	if (error) {
+		dev_err(cd->dev, "Failed to enable iovdd: %d\n", error);
+		return error;
+	}
 
-		/* Vendor waits 3ms for IOVDD to settle */
-		usleep_range(3000, 3100);
+	/* Vendor waits 3ms for IOVDD to settle */
+	usleep_range(3000, 3100);
 
-		error = regulator_enable(cd->avdd);
-		if (error) {
-			dev_err(cd->dev, "Failed to enable avdd: %d\n", error);
-			goto err_iovdd_disable;
-		}
+	error = regulator_enable(cd->avdd);
+	if (error) {
+		dev_err(cd->dev, "Failed to enable avdd: %d\n", error);
+		goto err_iovdd_disable;
+	}
 
-		/* Vendor waits 15ms for IOVDD to settle */
-		usleep_range(15000, 15100);
+	/* Vendor waits 15ms for IOVDD to settle */
+	usleep_range(15000, 15100);
 
-		gpiod_set_value(cd->reset_gpio, 0);
+	gpiod_set_value_cansleep(cd->reset_gpio, 0);
 
-		/* Vendor waits 4ms for Firmware to initialize */
-		usleep_range(4000, 4100);
+	/* Vendor waits 4ms for Firmware to initialize */
+	usleep_range(4000, 4100);
 
-		error = goodix_berlin_dev_confirm(cd);
-		if (error)
-			goto err_dev_reset;
+	error = goodix_berlin_dev_confirm(cd);
+	if (error)
+		goto err_dev_reset;
 
-		/* Vendor waits 100ms for Firmware to fully boot */
-		msleep(GOODIX_BERLIN_NORMAL_RESET_DELAY_MS);
+	/* Vendor waits 100ms for Firmware to fully boot */
+	msleep(GOODIX_BERLIN_NORMAL_RESET_DELAY_MS);
 
-		return 0;
-	}
+	return 0;
 
 err_dev_reset:
-	gpiod_set_value(cd->reset_gpio, 1);
-
+	gpiod_set_value_cansleep(cd->reset_gpio, 1);
 	regulator_disable(cd->avdd);
-
 err_iovdd_disable:
 	regulator_disable(cd->iovdd);
-
 	return error;
 }
 
+static void goodix_berlin_power_off(struct goodix_berlin_core *cd)
+{
+	gpiod_set_value_cansleep(cd->reset_gpio, 1);
+	regulator_disable(cd->avdd);
+	regulator_disable(cd->iovdd);
+}
+
 static int goodix_berlin_read_version(struct goodix_berlin_core *cd)
 {
 	u8 buf[sizeof(struct goodix_berlin_fw_version)];
 	int error;
 
-	error = regmap_raw_read(cd->regmap, GOODIX_BERLIN_FW_VERSION_INFO_ADDR, buf, sizeof(buf));
+	error = regmap_raw_read(cd->regmap, GOODIX_BERLIN_FW_VERSION_INFO_ADDR,
+				buf, sizeof(buf));
 	if (error) {
 		dev_err(cd->dev, "error reading fw version, %d\n", error);
 		return error;
@@ -235,8 +379,8 @@ static int goodix_berlin_convert_ic_info(struct goodix_berlin_core *cd,
 
 static int goodix_berlin_get_ic_info(struct goodix_berlin_core *cd)
 {
+	u8 *afe_data __free(kfree) = NULL;
 	__le16 length_raw;
-	u8 *afe_data;
 	u16 length;
 	int error;
 
@@ -248,53 +392,44 @@ static int goodix_berlin_get_ic_info(struct goodix_berlin_core *cd)
 				&length_raw, sizeof(length_raw));
 	if (error) {
 		dev_err(cd->dev, "failed get ic info length, %d\n", error);
-		goto free_afe_data;
+		return error;
 	}
 
 	length = le16_to_cpu(length_raw);
 	if (length >= GOODIX_BERLIN_IC_INFO_MAX_LEN) {
 		dev_err(cd->dev, "invalid ic info length %d\n", length);
-		error = -EINVAL;
-		goto free_afe_data;
+		return -EINVAL;
 	}
 
 	error = regmap_raw_read(cd->regmap, GOODIX_BERLIN_IC_INFO_ADDR,
 				afe_data, length);
 	if (error) {
 		dev_err(cd->dev, "failed get ic info data, %d\n", error);
-		goto free_afe_data;
+		return error;
 	}
 
 	/* 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;
+		return -EINVAL;
 	}
 
 	if (!goodix_berlin_checksum_valid(afe_data, length)) {
 		dev_err(cd->dev, "fw info checksum error\n");
-		error = -EINVAL;
-		goto free_afe_data;
+		return -EINVAL;
 	}
 
 	error = goodix_berlin_convert_ic_info(cd, afe_data, length);
 	if (error)
-		goto free_afe_data;
+		return error;
 
 	/* 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 -EINVAL;
 	}
 
 	return 0;
-
-free_afe_data:
-	kfree(afe_data);
-
-	return error;
 }
 
 static void goodix_berlin_parse_finger(struct goodix_berlin_core *cd,
@@ -305,12 +440,12 @@ static void goodix_berlin_parse_finger(struct goodix_berlin_core *cd,
 
 	/* Report finger touches */
 	for (i = 0; i < touch_num; i++) {
-		unsigned int id;
-
-		id = FIELD_GET(GOODIX_BERLIN_TOUCH_ID_MASK, touch_data[i].id);
+		unsigned int id = FIELD_GET(GOODIX_BERLIN_TOUCH_ID_MASK,
+					    touch_data[i].id);
 
 		if (id >= GOODIX_BERLIN_MAX_TOUCH) {
-			dev_warn_ratelimited(cd->dev, "invalid finger id %d\n", id);
+			dev_warn_ratelimited(cd->dev,
+					     "invalid finger id %d\n", id);
 			continue;
 		}
 
@@ -372,7 +507,7 @@ static void goodix_berlin_touch_handler(struct goodix_berlin_core *cd,
 
 		if (!goodix_berlin_checksum_valid(&buffer[GOODIX_BERLIN_IRQ_EVENT_HEAD_LEN],
 						  touch_num * GOODIX_BERLIN_BYTES_PER_POINT + 2)) {
-			dev_err(cd->dev, "touch data checksum error, data: %*ph\n",
+			dev_err(cd->dev, "touch data checksum error: %*ph\n",
 				touch_num * GOODIX_BERLIN_BYTES_PER_POINT + 2,
 				&buffer[GOODIX_BERLIN_IRQ_EVENT_HEAD_LEN]);
 			return;
@@ -385,16 +520,16 @@ static void goodix_berlin_touch_handler(struct goodix_berlin_core *cd,
 
 static int goodix_berlin_request_handle_reset(struct goodix_berlin_core *cd)
 {
-	gpiod_set_value(cd->reset_gpio, 1);
+	gpiod_set_value_cansleep(cd->reset_gpio, 1);
 	usleep_range(2000, 2100);
-	gpiod_set_value(cd->reset_gpio, 0);
+	gpiod_set_value_cansleep(cd->reset_gpio, 0);
 
 	msleep(GOODIX_BERLIN_NORMAL_RESET_DELAY_MS);
 
 	return 0;
 }
 
-static irqreturn_t goodix_berlin_threadirq_func(int irq, void *data)
+static irqreturn_t goodix_berlin_irq(int irq, void *data)
 {
 	struct goodix_berlin_core *cd = data;
 	u8 buf[GOODIX_BERLIN_IRQ_READ_LEN(2)];
@@ -405,7 +540,8 @@ static irqreturn_t goodix_berlin_threadirq_func(int irq, void *data)
 	error = regmap_raw_read(cd->regmap, cd->touch_data_addr, buf,
 				GOODIX_BERLIN_IRQ_READ_LEN(2));
 	if (error) {
-		dev_err_ratelimited(cd->dev, "failed get event head data, %d\n", error);
+		dev_warn_ratelimited(cd->dev,
+				     "failed get event head data: %d\n", error);
 		return IRQ_HANDLED;
 	}
 
@@ -413,7 +549,8 @@ static irqreturn_t goodix_berlin_threadirq_func(int irq, void *data)
 		return IRQ_HANDLED;
 
 	if (!goodix_berlin_checksum_valid(buf, GOODIX_BERLIN_IRQ_EVENT_HEAD_LEN)) {
-		dev_warn_ratelimited(cd->dev, "touch head checksum err : %*ph\n",
+		dev_warn_ratelimited(cd->dev,
+				     "touch head checksum error: %*ph\n",
 				     GOODIX_BERLIN_IRQ_EVENT_HEAD_LEN, buf);
 		return IRQ_HANDLED;
 	}
@@ -421,7 +558,8 @@ static irqreturn_t goodix_berlin_threadirq_func(int irq, void *data)
 	event_status = buf[GOODIX_BERLIN_STATUS_OFFSET];
 
 	if (event_status & GOODIX_BERLIN_TOUCH_EVENT)
-		goodix_berlin_touch_handler(cd, buf, GOODIX_BERLIN_IRQ_READ_LEN(2));
+		goodix_berlin_touch_handler(cd, buf,
+					    GOODIX_BERLIN_IRQ_READ_LEN(2));
 
 	if (event_status & GOODIX_BERLIN_REQUEST_EVENT) {
 		switch (buf[GOODIX_BERLIN_REQUEST_TYPE_OFFSET]) {
@@ -460,8 +598,10 @@ static int goodix_berlin_input_dev_config(struct goodix_berlin_core *cd,
 
 	input_dev->id = *id;
 
-	input_set_abs_params(cd->input_dev, ABS_MT_POSITION_X, 0, SZ_64K - 1, 0, 0);
-	input_set_abs_params(cd->input_dev, ABS_MT_POSITION_Y, 0, SZ_64K - 1, 0, 0);
+	input_set_abs_params(cd->input_dev, ABS_MT_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);
@@ -478,21 +618,22 @@ static int goodix_berlin_input_dev_config(struct goodix_berlin_core *cd,
 	return 0;
 }
 
-static int goodix_berlin_pm_suspend(struct device *dev)
+static int goodix_berlin_suspend(struct device *dev)
 {
 	struct goodix_berlin_core *cd = dev_get_drvdata(dev);
 
 	disable_irq(cd->irq);
+	goodix_berlin_power_off(cd);
 
-	return goodix_berlin_power_on(cd, false);
+	return 0;
 }
 
-static int goodix_berlin_pm_resume(struct device *dev)
+static int goodix_berlin_resume(struct device *dev)
 {
 	struct goodix_berlin_core *cd = dev_get_drvdata(dev);
 	int error;
 
-	error = goodix_berlin_power_on(cd, true);
+	error = goodix_berlin_power_on(cd);
 	if (error)
 		return error;
 
@@ -502,14 +643,13 @@ static int goodix_berlin_pm_resume(struct device *dev)
 }
 
 EXPORT_GPL_SIMPLE_DEV_PM_OPS(goodix_berlin_pm_ops,
-			     goodix_berlin_pm_suspend,
-			     goodix_berlin_pm_resume);
+			     goodix_berlin_suspend, goodix_berlin_resume);
 
-static void goodix_berlin_power_off(void *data)
+static void goodix_berlin_power_off_act(void *data)
 {
 	struct goodix_berlin_core *cd = data;
 
-	goodix_berlin_power_on(cd, false);
+	goodix_berlin_power_off(cd);
 }
 
 int goodix_berlin_probe(struct device *dev, int irq, const struct input_id *id,
@@ -546,13 +686,13 @@ int goodix_berlin_probe(struct device *dev, int irq, const struct input_id *id,
 		return dev_err_probe(dev, PTR_ERR(cd->iovdd),
 				     "Failed to request iovdd regulator\n");
 
-	error = goodix_berlin_power_on(cd, true);
+	error = goodix_berlin_power_on(cd);
 	if (error) {
 		dev_err(dev, "failed power on");
 		return error;
 	}
 
-	error = devm_add_action_or_reset(dev, goodix_berlin_power_off, cd);
+	error = devm_add_action_or_reset(dev, goodix_berlin_power_off_act, cd);
 	if (error)
 		return error;
 
@@ -574,8 +714,7 @@ int goodix_berlin_probe(struct device *dev, int irq, const struct input_id *id,
 		return error;
 	}
 
-	error = devm_request_threaded_irq(dev, irq, NULL,
-					  goodix_berlin_threadirq_func,
+	error = devm_request_threaded_irq(dev, cd->irq, NULL, goodix_berlin_irq,
 					  IRQF_ONESHOT, "goodix-berlin", cd);
 	if (error) {
 		dev_err(dev, "request threaded irq failed: %d\n", error);
@@ -584,7 +723,8 @@ int goodix_berlin_probe(struct device *dev, int irq, const struct input_id *id,
 
 	dev_set_drvdata(dev, cd);
 
-	dev_dbg(dev, "Goodix Berlin %s Touchscreen Controller", cd->fw_version.patch_pid);
+	dev_dbg(dev, "Goodix Berlin %s Touchscreen Controller",
+		cd->fw_version.patch_pid);
 
 	return 0;
 }