input: Introduce QPNP (Qualcomm Plug n Play) haptics found on qcom PMICs

input: Introduce QPNP (Qualcomm Plug n Play) haptics found on qcom PMICs

[Caleb Connolly]

This series introduces a driver for the QPNP haptics hardware block
found in Qualcomm PMICs. QPNP haptics support LRA (Linear Resonant
Actuator) style haptics, as well as ERM (Eccentric Rotating Mass).
It also supports several modes of driving the haptics, e.g. by loading
the pattern to play into an internal buffer, or using PWM.

More information about the hardware can be found here:
        https://gitlab.com/sdm845-mainline/linux/-/wikis/PMI8998-QPNP-Haptics

This driver has been written based on downstream sources as no public
documentation is available. It includes initial support for LRA haptics
in buffer mode, this combination seems to be the most common and will
enable haptics on the OnePlus 6 and 6T, PocoPhone F1, OnePlus 5 and
several other Qualcomm devices with mainline kernel support.

The driver is implemented using the ff-memless (forcefeedback) input
framework and makes an attempt to control the strength of vibration relative
to the magnitude set from userspace.

 ~ Caleb

[PATCH 1/4] dt-bindings: input: add Qualcomm QPNP haptics driver

[Caleb Connolly]

Add bindings for pmi8998 qpnp haptics driver.

Signed-off-by: Caleb Connolly <caleb@connolly.tech>
---
 .../bindings/input/qcom,qpnp-haptics.yaml     | 123 ++++++++++++++++++
 include/dt-bindings/input/qcom,qpnp-haptics.h |  31 +++++
 2 files changed, 154 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/input/qcom,qpnp-haptics.yaml
 create mode 100644 include/dt-bindings/input/qcom,qpnp-haptics.h

diff --git a/Documentation/devicetree/bindings/input/qcom,qpnp-haptics.yaml b/Documentation/devicetree/bindings/input/qcom,qpnp-haptics.yaml
new file mode 100644
index 000000000000..81dd8d6321f0
--- /dev/null
+++ b/Documentation/devicetree/bindings/input/qcom,qpnp-haptics.yaml
@@ -0,0 +1,123 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+# Copyright 2020 Unisoc Inc.
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/input/qcom,qpnp-haptics.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Qualcomm Technologies Inc PMI8998 QPNP haptics
+
+maintainers:
+  - Caleb Connolly <caleb@connolly.tech>
+
+description: |
+  QPNP (Qualcomm Technologies, Inc. Plug N Play) Haptics is a peripheral on some
+  QTI PMICs. It supports linear resonant actuators commonly found in mobile devices.
+
+properties:
+  compatible:
+    enum:
+      - qcom,qpnp-haptics
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    minItems: 2
+    items:
+      - description: short circuit interrupt
+      - description: play interrupt
+
+  interrupt-names:
+    minItems: 2
+    items:
+      - const: short
+      - const: play
+
+  qcom,actuator-type:
+    description: |
+      The type of actuator attached to the hardware.
+      Allowed values are,
+        0 - HAP_TYPE_LRA
+        1 - HAP_TYPE_ERM
+    $ref: /schemas/types.yaml#/definitions/uint32
+    enum: [ 0, 1 ]
+    default: 0
+
+  qcom,wave-shape:
+    description: |
+      Selects the wave shape to use.
+      Allowed values are,
+        0 - HAP_WAVE_SINE
+        1 - HAP_WAVE_SQUARE
+    $ref: /schemas/types.yaml#/definitions/uint32
+    enum: [ 0, 1 ]
+    default: 0
+
+  qcom,play-mode:
+    description: |
+      Selects the play mode to use.
+      Allowed values are,
+        0 - HAP_PLAY_DIRECT
+        1 - HAP_PLAY_BUFFER
+        2 - HAP_PLAY_AUDIO
+        3 - HAP_PLAY_PWM
+    $ref: /schemas/types.yaml#/definitions/uint32
+    enum: [ 0, 1, 2, 3 ]
+    default: 2
+
+  qcom,wave-play-rate-us:
+    description: |
+      Wave sample durection in microseconds, 1/f where f
+      is the resonant frequency of the actuator.
+    $ref: /schemas/types.yaml#/definitions/uint32
+    minimum: 0
+    maximum: 20475
+
+  qcom,brake-pattern:
+    minItems: 4
+    description: |
+      The brake pattern are the strengths of the pattern
+      used to brake the haptics. Allowed values are,
+        0 - 0V
+        1 - Vmax/4
+        2 - Vmax/2
+        3 - Vmax
+    $ref: /schemas/types.yaml#/definitions/uint32-array
+    default: [0x3, 0x3, 0x2, 0x1]
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - qcom,wave-play-rate-us
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/spmi/spmi.h>
+    #include <dt-bindings/interrupt-controller/irq.h>
+    #include <dt-bindings/input/qcom,qpnp-haptics.h>
+    pmi8998_lsid1: pmic@3 {
+      compatible = "qcom,pmi8998", "qcom,spmi-pmic";
+      reg = <0x3 SPMI_USID>;
+      #address-cells = <1>;
+      #size-cells = <0>;
+
+      qpnp_haptics: qcom,haptics@c000 {
+        compatible = "qcom,qpnp-haptics";
+        reg = <0xc000 0x100>;
+
+        interrupts = <0x3 0xc0 0x0 IRQ_TYPE_EDGE_BOTH>,
+              <0x3 0xc0 0x1 IRQ_TYPE_EDGE_BOTH>;
+        interrupt-names = "short", "play";
+
+        qcom,actuator-type = <HAP_TYPE_LRA>;
+        qcom,wave-shape = <HAP_WAVE_SINE>;
+        qcom,play-mode = <HAP_PLAY_BUFFER>;
+        qcom,brake-pattern = <0x3 0x3 0x2 0x1>;
+
+        qcom,wave-play-rate-us = <4255>;
+      };
+    };
diff --git a/include/dt-bindings/input/qcom,qpnp-haptics.h b/include/dt-bindings/input/qcom,qpnp-haptics.h
new file mode 100644
index 000000000000..bfbec041484c
--- /dev/null
+++ b/include/dt-bindings/input/qcom,qpnp-haptics.h
@@ -0,0 +1,31 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * This header provides constants for pmi8998 qpnp haptics options.
+ */
+
+#ifndef _DT_BINDINGS_QCOM_PMIC_QPNP_HAPTICS_
+#define _DT_BINDINGS_QCOM_PMIC_QPNP_HAPTICS_
+
+// Actuator types
+#define HAP_TYPE_LRA		0
+#define HAP_TYPE_ERM		1
+
+// LRA Wave type
+#define HAP_WAVE_SINE		0
+#define HAP_WAVE_SQUARE		1
+
+// Play modes
+#define HAP_PLAY_DIRECT		0
+#define HAP_PLAY_BUFFER		1
+#define HAP_PLAY_AUDIO		2
+#define HAP_PLAY_PWM		3
+
+#define HAP_PLAY_MAX		HAP_PLAY_PWM
+
+#define HAP_AUTO_RES_NONE	0
+#define HAP_AUTO_RES_ZXD	1
+#define HAP_AUTO_RES_QWD	2
+#define HAP_AUTO_RES_MAX_QWD	3
+#define HAP_AUTO_RES_ZXD_EOP	4
+
+#endif /* _DT_BINDINGS_QCOM_PMIC_QPNP_HAPTICS_ */
--
2.31.1

[PATCH 2/4] input: add Qualcomm QPNP haptics driver

[Caleb Connolly]

Add support for the haptics found in pmi8998 and related PMICs.
Based on the ff-memless interface. Currently this driver provides
a partial implementation of hardware features.

This driver only supports LRAs (Linear Resonant Actuators) in the "buffer"
mode with a single wave pattern.

Signed-off-by: Caleb Connolly <caleb@connolly.tech>
---
 drivers/input/misc/Kconfig        |   11 +
 drivers/input/misc/Makefile       |    1 +
 drivers/input/misc/qpnp-haptics.c | 1022 +++++++++++++++++++++++++++++
 3 files changed, 1034 insertions(+)
 create mode 100644 drivers/input/misc/qpnp-haptics.c

diff --git a/drivers/input/misc/Kconfig b/drivers/input/misc/Kconfig
index 498cde376981..b5ba03e6cf58 100644
--- a/drivers/input/misc/Kconfig
+++ b/drivers/input/misc/Kconfig
@@ -186,6 +186,17 @@ config INPUT_PMIC8XXX_PWRKEY
 	  To compile this driver as a module, choose M here: the
 	  module will be called pmic8xxx-pwrkey.

+config INPUT_QPNP_HAPTICS
+	tristate "Qualcomm QPNP HAPTICS"
+	depends on ARCH_QCOM
+	select INPUT_FF_MEMLESS
+	help
+	  This option enables support for the haptics found in pmi8998 and
+	  related PMICs. Based on the ff-memless interface.
+
+	  To compile this driver as module, choose M here: the
+	  module will be called qpnp-haptics.
+
 config INPUT_SPARCSPKR
 	tristate "SPARC Speaker support"
 	depends on PCI && SPARC64
diff --git a/drivers/input/misc/Makefile b/drivers/input/misc/Makefile
index f593beed7e05..c43290163db0 100644
--- a/drivers/input/misc/Makefile
+++ b/drivers/input/misc/Makefile
@@ -65,6 +65,7 @@ obj-$(CONFIG_INPUT_PMIC8XXX_PWRKEY)	+= pmic8xxx-pwrkey.o
 obj-$(CONFIG_INPUT_POWERMATE)		+= powermate.o
 obj-$(CONFIG_INPUT_PWM_BEEPER)		+= pwm-beeper.o
 obj-$(CONFIG_INPUT_PWM_VIBRA)		+= pwm-vibra.o
+obj-$(CONFIG_INPUT_QPNP_HAPTICS)	+= qpnp-haptics.o
 obj-$(CONFIG_INPUT_RAVE_SP_PWRBUTTON)	+= rave-sp-pwrbutton.o
 obj-$(CONFIG_INPUT_RB532_BUTTON)	+= rb532_button.o
 obj-$(CONFIG_INPUT_REGULATOR_HAPTIC)	+= regulator-haptic.o
diff --git a/drivers/input/misc/qpnp-haptics.c b/drivers/input/misc/qpnp-haptics.c
new file mode 100644
index 000000000000..daa7a18ffc7d
--- /dev/null
+++ b/drivers/input/misc/qpnp-haptics.c
@@ -0,0 +1,1022 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) 2021, Caleb Connolly <caleb@connolly.tech>
+ * Qualcomm Plug and Play haptics driver for pmi8998 and related PMICs.
+ * Based on ./pm8xxx-vibrator.c
+ */
+
+#include <dt-bindings/input/qcom,qpnp-haptics.h>
+
+#include <linux/atomic.h>
+#include <linux/bits.h>
+#include <linux/errno.h>
+#include <linux/input.h>
+#include <linux/interrupt.h>
+#include <linux/kernel.h>
+#include <linux/log2.h>
+#include <linux/minmax.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/slab.h>
+#include <linux/time.h>
+#include <linux/types.h>
+
+/*
+ * Register and bit definitions
+ */
+
+#define HAP_STATUS_1_REG(chip)		(chip->base + 0x0A)
+#define HAP_BUSY_BIT			BIT(1)
+#define SC_FLAG_BIT			BIT(3)
+#define AUTO_RES_ERROR_BIT		BIT(4)
+
+#define HAP_LRA_AUTO_RES_LO_REG(chip)	(chip->base + 0x0B)
+#define HAP_LRA_AUTO_RES_HI_REG(chip)	(chip->base + 0x0C)
+
+#define HAP_EN_CTL_REG(chip)		(chip->base + 0x46)
+#define HAP_EN_BIT			BIT(7)
+
+#define HAP_EN_CTL2_REG(chip)		(chip->base + 0x48)
+#define BRAKE_EN_BIT			BIT(0)
+
+#define HAP_AUTO_RES_CTRL_REG(chip)	(chip->base + 0x4B)
+#define AUTO_RES_EN_BIT			BIT(7)
+#define AUTO_RES_ERR_RECOVERY_BIT	BIT(3)
+#define AUTO_RES_EN_FLAG_BIT	BIT(0)
+
+#define HAP_CFG1_REG(chip)		(chip->base + 0x4C)
+#define HAP_ACT_TYPE_MASK		BIT(0)
+
+#define HAP_CFG2_REG(chip)		(chip->base + 0x4D)
+#define HAP_LRA_RES_TYPE_MASK		BIT(0)
+
+#define HAP_SEL_REG(chip)		(chip->base + 0x4E)
+#define HAP_WF_SOURCE_MASK		GENMASK(5, 4)
+#define HAP_WF_SOURCE_SHIFT		4
+
+#define HAP_LRA_AUTO_RES_REG(chip)	(chip->base + 0x4F)
+#define LRA_AUTO_RES_MODE_MASK		GENMASK(6, 4)
+#define LRA_AUTO_RES_MODE_SHIFT		4
+#define LRA_HIGH_Z_MASK			GENMASK(3, 2)
+#define LRA_HIGH_Z_SHIFT		2
+#define LRA_RES_CAL_MASK		GENMASK(1, 0)
+#define HAP_RES_CAL_PERIOD_MIN		4
+#define HAP_RES_CAL_PERIOD_MAX		32
+
+#define HAP_VMAX_CFG_REG(chip)		(chip->base + 0x51)
+#define HAP_VMAX_OVD_BIT		BIT(6)
+#define HAP_VMAX_MASK			GENMASK(5, 1)
+#define HAP_VMAX_SHIFT			1
+
+#define HAP_ILIM_CFG_REG(chip)		(chip->base + 0x52)
+#define HAP_ILIM_SEL_MASK		BIT(0)
+#define HAP_ILIM_400_MA			0
+#define HAP_ILIM_800_MA			1
+
+#define HAP_SC_DEB_REG(chip)		(chip->base + 0x53)
+#define HAP_SC_DEB_MASK			GENMASK(2, 0)
+#define HAP_SC_DEB_CYCLES_MIN		0
+#define HAP_DEF_SC_DEB_CYCLES		8
+#define HAP_SC_DEB_CYCLES_MAX		32
+
+#define HAP_RATE_CFG1_REG(chip)		(chip->base + 0x54)
+#define HAP_RATE_CFG1_MASK		GENMASK(7, 0)
+#define HAP_RATE_CFG2_SHIFT		8 // As CFG2 is the most significant byte
+
+#define HAP_RATE_CFG2_REG(chip)		(chip->base + 0x55)
+#define HAP_RATE_CFG2_MASK		GENMASK(3, 0)
+
+#define HAP_SC_CLR_REG(chip)		(chip->base + 0x59)
+#define SC_CLR_BIT			BIT(0)
+
+#define HAP_BRAKE_REG(chip)		(chip->base + 0x5C)
+#define HAP_BRAKE_PAT_MASK		0x3
+
+#define HAP_WF_REPEAT_REG(chip)		(chip->base + 0x5E)
+#define WF_REPEAT_MASK			GENMASK(6, 4)
+#define WF_REPEAT_SHIFT			4
+#define WF_REPEAT_MIN			1
+#define WF_REPEAT_MAX			128
+#define WF_S_REPEAT_MASK		GENMASK(1, 0)
+#define WF_S_REPEAT_MIN			1
+#define WF_S_REPEAT_MAX			8
+
+#define HAP_WF_S1_REG(chip)		(chip->base + 0x60)
+#define HAP_WF_SIGN_BIT			BIT(7)
+#define HAP_WF_OVD_BIT			BIT(6)
+#define HAP_WF_SAMP_MAX			GENMASK(5, 1)
+#define HAP_WF_SAMPLE_LEN		8
+
+#define HAP_PLAY_REG(chip)		(chip->base + 0x70)
+#define HAP_PLAY_BIT			BIT(7)
+#define HAP_PAUSE_BIT			BIT(0)
+
+#define HAP_SEC_ACCESS_REG(chip)	(chip->base + 0xD0)
+#define HAP_SEC_ACCESS_UNLOCK		0xA5
+
+#define HAP_TEST2_REG(chip)		(chip->base + 0xE3)
+
+// Values
+#define HAP_VMAX_MIN_MV			116
+#define HAP_VMAX_MAX_MV			3596
+#define HAP_VMAX_MAX_MV_STRONG		3596
+
+#define HAP_WAVE_PLAY_RATE_MIN_US	0
+#define HAP_WAVE_PLAY_RATE_MAX_US	20475
+#define HAP_WAVE_PLAY_TIME_MAX_MS	15000
+
+#define AUTO_RES_ERR_POLL_TIME_NS	(20 * NSEC_PER_MSEC)
+#define HAPTICS_BACK_EMF_DELAY_US	20000
+
+#define HAP_BRAKE_PAT_LEN		4
+#define HAP_WAVE_SAMP_LEN		8
+#define NUM_WF_SET			4
+#define HAP_WAVE_SAMP_SET_LEN		(HAP_WAVE_SAMP_LEN * NUM_WF_SET)
+#define HAP_RATE_CFG_STEP_US		5
+
+#define SC_MAX_COUNT			5
+#define SC_COUNT_RST_DELAY_US		1000000
+
+enum hap_play_control {
+	HAP_STOP,
+	HAP_PAUSE,
+	HAP_PLAY,
+};
+
+/**
+ * struct qpnp_haptics - struct for qpnp haptics data.
+ *
+ * @pdev: The platform device responsible for the haptics.
+ * @regmap: Register map for the hardware block.
+ * @input_dev: The input device used to receive events.
+ * @work: Work struct to play effects.
+ * @base: Base address of the regmap.
+ * @active: Atomic value used to track if haptics are currently playing.
+ * @play_irq: Fired to load the next wave pattern.
+ * @sc_irq: Short circuit irq.
+ * @last_sc_time: Time since the short circuit IRQ last fired.
+ * @sc_count: Number of times the short circuit IRQ has fired in this interval.
+ * @actuator_type: The type of actuator in use.
+ * @wave_shape: The shape of the waves to use (sine or square).
+ * @play_mode: The play mode to use (direct, buffer, pwm, audio).
+ * @magnitude: The strength we should be playing at.
+ * @vmax: Max voltage to use when playing.
+ * @current_limit: The current limit for this hardware (400mA or 800mA).
+ * @play_wave_rate: The wave rate to use for this hardware.
+ * @wave_samp: The array of wave samples to write for buffer mode.
+ * @brake_pat: The pattern to apply when braking.
+ * @play_lock: Lock to be held when updating the hardware state.
+ */
+struct qpnp_haptics {
+	struct platform_device *pdev;
+	struct regmap *regmap;
+	struct input_dev *haptics_input_dev;
+	struct work_struct work;
+	u32 base;
+
+	atomic_t active;
+
+	int play_irq;
+	int sc_irq;
+	ktime_t last_sc_time;
+	u8 sc_count;
+
+	u8 actuator_type;
+	u8 wave_shape;
+	u8 play_mode;
+	int magnitude;
+	u32 vmax;
+	u32 current_limit;
+	u32 play_wave_rate;
+
+	u32 wave_samp[HAP_WAVE_SAMP_SET_LEN];
+	u8 brake_pat[HAP_BRAKE_PAT_LEN];
+
+	struct mutex play_lock;
+};
+
+static inline bool is_secure_addr(u16 addr)
+{
+	return (addr & 0xFF) > 0xD0;
+}
+
+static int qpnp_haptics_read(struct qpnp_haptics *haptics,
+	u16 addr, u8 *val, int len)
+{
+	int ret;
+
+	ret = regmap_bulk_read(haptics->regmap, addr, val, len);
+	if (ret < 0)
+		pr_err("Error reading address: 0x%x, ret %d\n", addr, ret);
+
+	return ret;
+}
+
+static int qpnp_haptics_write(struct qpnp_haptics *haptics,
+	u16 addr, u8 *val, int len)
+{
+	int ret, i;
+
+	if (is_secure_addr(addr)) {
+		for (i = 0; i < len; i++) {
+			pr_info("%s: unlocking for addr: 0x%x, val: 0x%x", __func__,
+				addr, val[i]);
+			ret = regmap_write(haptics->regmap,
+				HAP_SEC_ACCESS_REG(haptics), HAP_SEC_ACCESS_UNLOCK);
+			if (ret < 0) {
+				pr_err("Error writing unlock code, ret %d\n",
+					ret);
+				return ret;
+			}
+
+			ret = regmap_write(haptics->regmap, addr + i, val[i]);
+			if (ret < 0) {
+				pr_err("Error writing address 0x%x, ret %d\n",
+					addr + i, ret);
+				return ret;
+			}
+		}
+	} else {
+		if (len > 1)
+			ret = regmap_bulk_write(haptics->regmap, addr, val, len);
+		else
+			ret = regmap_write(haptics->regmap, addr, *val);
+	}
+
+	if (ret < 0)
+		pr_err("%s: Error writing address: 0x%x, ret %d\n", __func__, addr, ret);
+
+	return ret;
+}
+
+static int qpnp_haptics_write_masked(struct qpnp_haptics *haptics,
+	u16 addr, u8 mask, u8 val)
+{
+	int ret;
+
+	if (is_secure_addr(addr)) {
+		ret = regmap_write(haptics->regmap,
+			HAP_SEC_ACCESS_REG(haptics), HAP_SEC_ACCESS_UNLOCK);
+		if (ret < 0) {
+			pr_err("Error writing unlock code - ret %d\n", ret);
+			return ret;
+		}
+	}
+
+	ret = regmap_update_bits(haptics->regmap, addr, mask, val);
+	if (ret < 0)
+		pr_err("Error writing address: 0x%x - ret %d\n", addr, ret);
+
+	return ret;
+}
+
+static bool is_haptics_idle(struct qpnp_haptics *haptics)
+{
+	int ret;
+	u8 val;
+
+	if (haptics->play_mode == HAP_PLAY_DIRECT ||
+			haptics->play_mode == HAP_PLAY_PWM)
+		return true;
+
+	ret = qpnp_haptics_read(haptics, HAP_STATUS_1_REG(haptics), &val, 1);
+	if (ret < 0 || (val & HAP_BUSY_BIT))
+		return false;
+
+	return true;
+}
+
+static int qpnp_haptics_module_enable(struct qpnp_haptics *haptics, bool enable)
+{
+	u8 val;
+	int rc;
+
+	dev_dbg(&haptics->pdev->dev, "Setting module enable: %d", enable);
+
+	val = enable ? HAP_EN_BIT : 0;
+	rc = qpnp_haptics_write(haptics, HAP_EN_CTL_REG(haptics), &val, 1);
+	if (rc < 0)
+		return rc;
+
+	return 0;
+}
+
+static int qpnp_haptics_write_vmax(struct qpnp_haptics *haptics)
+{
+	u8 val = 0;
+	int ret;
+	u32 vmax_mv = haptics->vmax;
+
+	vmax_mv = clamp_t(u32, vmax_mv, HAP_VMAX_MIN_MV, HAP_VMAX_MAX_MV);
+
+	dev_dbg(&haptics->pdev->dev, "Setting vmax to: %d", vmax_mv);
+
+	val = DIV_ROUND_CLOSEST(vmax_mv, HAP_VMAX_MIN_MV);
+	val <<= HAP_VMAX_SHIFT;
+	// overdrive is only supported on pm660
+	val &= ~HAP_VMAX_OVD_BIT;
+
+	ret = qpnp_haptics_write_masked(haptics, HAP_VMAX_CFG_REG(haptics),
+			HAP_VMAX_MASK | HAP_VMAX_OVD_BIT, val);
+	return ret;
+}
+
+static int qpnp_haptics_write_current_limit(struct qpnp_haptics *haptics)
+{
+	int ret;
+
+	haptics->current_limit = clamp_t(u32, haptics->current_limit,
+					 HAP_ILIM_400_MA, HAP_ILIM_800_MA);
+
+	dev_dbg(&haptics->pdev->dev, "Setting current_limit to: 0x%x", haptics->current_limit);
+
+	ret = qpnp_haptics_write_masked(haptics, HAP_ILIM_CFG_REG(haptics),
+			HAP_ILIM_SEL_MASK, haptics->current_limit);
+	return ret;
+}
+
+static int qpnp_haptics_write_play_mode(struct qpnp_haptics *haptics)
+{
+	u8 val = 0;
+	int ret;
+
+	if (!is_haptics_idle(haptics))
+		return -EBUSY;
+
+	dev_dbg(&haptics->pdev->dev, "Setting play_mode to: 0x%x", haptics->play_mode);
+
+	val = haptics->play_mode << HAP_WF_SOURCE_SHIFT;
+	ret = qpnp_haptics_write_masked(haptics, HAP_SEL_REG(haptics),
+			HAP_WF_SOURCE_MASK, val);
+
+	return ret;
+}
+
+static int qpnp_haptics_write_play_rate(struct qpnp_haptics *haptics, u16 play_rate)
+{
+	int rc;
+	u8 val[2];
+
+	dev_dbg(&haptics->pdev->dev, "Setting play_rate to: %d", play_rate);
+
+	val[0] = play_rate & HAP_RATE_CFG1_MASK;
+	val[1] = (play_rate >> HAP_RATE_CFG2_SHIFT) & HAP_RATE_CFG2_MASK;
+	rc = qpnp_haptics_write(haptics, HAP_RATE_CFG1_REG(haptics), val, 2);
+	if (rc < 0)
+		return rc;
+
+	return 0;
+}
+
+/*
+ * qpnp_haptics_set_auto_res() - Auto resonance
+ * allows the haptics to automatically adjust the
+ * speed of the oscillation in order to maintain
+ * the resonant frequency.
+ */
+static int qpnp_haptics_set_auto_res(struct qpnp_haptics *haptics, bool enable)
+{
+	int rc = 0;
+	u8 val;
+
+	// LRAs are the only type to support auto res
+	if (haptics->actuator_type != HAP_TYPE_LRA)
+		return 0;
+
+	val = enable ? AUTO_RES_EN_BIT : 0;
+
+	rc = qpnp_haptics_write_masked(haptics, HAP_TEST2_REG(haptics),
+			AUTO_RES_EN_BIT, val);
+	if (rc < 0)
+		return rc;
+
+	dev_dbg(&haptics->pdev->dev, "Auto resonance enabled: %d", enable);
+	return rc;
+}
+
+static int qpnp_haptics_write_brake(struct qpnp_haptics *haptics)
+{
+	int ret, i;
+	u32 temp;
+	u8 val;
+
+	dev_dbg(&haptics->pdev->dev, "Configuring brake pattern");
+
+	ret = qpnp_haptics_write_masked(haptics, HAP_EN_CTL2_REG(haptics),
+			BRAKE_EN_BIT, 1);
+	if (ret < 0)
+		return ret;
+
+	for (i = HAP_BRAKE_PAT_LEN - 1, val = 0; i >= 0; i--) {
+		u8 p = haptics->brake_pat[i] & HAP_BRAKE_PAT_MASK;
+
+		temp = i << 1;
+		val |= p << temp;
+	}
+
+	ret = qpnp_haptics_write(haptics, HAP_BRAKE_REG(haptics), &val, 1);
+	if (ret < 0)
+		return ret;
+
+	return 0;
+}
+
+static int qpnp_haptics_write_buffer_config(struct qpnp_haptics *haptics)
+{
+	u8 buf[HAP_WAVE_SAMP_LEN];
+	int rc, i;
+
+	dev_dbg(&haptics->pdev->dev, "Writing buffer config");
+
+	for (i = 0; i < HAP_WAVE_SAMP_LEN; i++)
+		buf[i] = haptics->wave_samp[i];
+
+	rc = qpnp_haptics_write(haptics, HAP_WF_S1_REG(haptics), buf,
+			HAP_WAVE_SAMP_LEN);
+
+	return rc;
+}
+
+/**
+ * qpnp_haptics_write_wave_repeat() - write wave repeat values.
+ */
+static int qpnp_haptics_write_wave_repeat(struct qpnp_haptics *haptics)
+{
+	int ret;
+	u8 val = 0, mask = 0;
+
+	// The number of times to repeat each wave
+	mask = WF_REPEAT_MASK;
+	val = ilog2(1) << WF_REPEAT_SHIFT; // Currently hard coded to default of 1
+
+	// the number of times to repeat each wave sample (group of waves)
+	mask |= WF_S_REPEAT_MASK;
+	val |= ilog2(1);
+
+	ret = qpnp_haptics_write_masked(haptics, HAP_WF_REPEAT_REG(haptics),
+			mask, val);
+	return ret;
+}
+
+static int qpnp_haptics_write_play_control(struct qpnp_haptics *haptics,
+						enum hap_play_control ctrl)
+{
+	u8 val;
+	int rc;
+
+	switch (ctrl) {
+	case HAP_STOP:
+		val = 0;
+		break;
+	case HAP_PAUSE:
+		val = HAP_PAUSE_BIT;
+		break;
+	case HAP_PLAY:
+		val = HAP_PLAY_BIT;
+		break;
+	default:
+		return 0;
+	}
+
+	rc = qpnp_haptics_write(haptics, HAP_PLAY_REG(haptics), &val, 1);
+	if (rc < 0) {
+		pr_err("Error writing to PLAY_REG, rc=%d\n", rc);
+		return rc;
+	}
+
+	dev_dbg(&haptics->pdev->dev, "haptics play ctrl: %d\n", ctrl);
+	return rc;
+}
+
+/*
+ * This IRQ is fired to tell us to load the next wave sample set.
+ * As we only currently support a single sample set, it's unused.
+ */
+static irqreturn_t qpnp_haptics_play_irq_handler(int irq, void *data)
+{
+	struct qpnp_haptics *haptics = data;
+
+	dev_dbg(&haptics->pdev->dev, "play_irq triggered");
+
+	return IRQ_HANDLED;
+}
+
+/*
+ * Fires every ~50ms whilst the haptics are active.
+ * If the SC_FLAG_BIT is set then that means there isn't a short circuit
+ * and we just need to clear the IRQ to indicate that the device should
+ * keep vibrating.
+ *
+ * Otherwise, it means a short circuit situation has occurred.
+ */
+static irqreturn_t qpnp_haptics_sc_irq_handler(int irq, void *data)
+{
+	struct qpnp_haptics *haptics = data;
+	int ret;
+	u8 val;
+	s64 sc_delta_time_us;
+	ktime_t temp;
+
+	ret = qpnp_haptics_read(haptics, HAP_STATUS_1_REG(haptics), &val, 1);
+	if (ret < 0)
+		return IRQ_HANDLED;
+
+	if (!(val & SC_FLAG_BIT)) {
+		haptics->sc_count = 0;
+		return IRQ_HANDLED;
+	}
+
+	temp = ktime_get();
+	sc_delta_time_us = ktime_us_delta(temp, haptics->last_sc_time);
+	haptics->last_sc_time = temp;
+
+	if (sc_delta_time_us > SC_COUNT_RST_DELAY_US)
+		haptics->sc_count = 0;
+	else
+		haptics->sc_count++;
+
+	// Clear the interrupt flag
+	val = SC_CLR_BIT;
+	ret = qpnp_haptics_write(haptics, HAP_SC_CLR_REG(haptics), &val, 1);
+	if (ret < 0) {
+		pr_err("Error writing to SC_CLR_REG, rc=%d\n", ret);
+		return IRQ_HANDLED;
+	}
+
+	if (haptics->sc_count > SC_MAX_COUNT) {
+		pr_crit("Short circuit persists, disabling haptics\n");
+		ret = qpnp_haptics_module_enable(haptics, false);
+		if (ret < 0) {
+			pr_err("Error disabling module, rc=%d\n", ret);
+			return IRQ_HANDLED;
+		}
+	}
+
+	return IRQ_HANDLED;
+}
+
+
+/**
+ * qpnp_haptics_init() - Initialise haptics hardware for use
+ * @haptics: haptics device
+ */
+static int qpnp_haptics_init(struct qpnp_haptics *haptics)
+{
+	int ret;
+	u8 val, mask;
+	u16 auto_res_mode;
+	u16 play_rate = 0;
+
+	ret = qpnp_haptics_write_masked(haptics, HAP_CFG1_REG(haptics),
+		HAP_ACT_TYPE_MASK, haptics->actuator_type);
+	if (ret < 0)
+		return ret;
+
+	// Configure auto resonance
+	// see qpnp_haptics_lra_auto_res_config downstream
+	// This is greatly simplified.
+	auto_res_mode = HAP_AUTO_RES_ZXD_EOP << LRA_AUTO_RES_MODE_SHIFT;
+
+	//Default LRA calibration period
+	val = ilog2(32 / HAP_RES_CAL_PERIOD_MIN);
+
+	val |= (auto_res_mode << LRA_AUTO_RES_MODE_SHIFT);
+	// The 1 here is for OPT2 "High Z configuration", there are 3
+	// options and no documentation indicating the difference
+	val |= (1 << LRA_HIGH_Z_SHIFT);
+	mask = LRA_AUTO_RES_MODE_MASK | LRA_HIGH_Z_MASK | LRA_RES_CAL_MASK;
+
+	ret = qpnp_haptics_write_masked(haptics, HAP_LRA_AUTO_RES_REG(haptics),
+			mask, val);
+
+	dev_dbg(&haptics->pdev->dev, "%s: auto_res_mode: %d\n", __func__,
+		auto_res_mode);
+
+	/* Configure the PLAY MODE register */
+	ret = qpnp_haptics_write_play_mode(haptics);
+	if (ret < 0)
+		return ret;
+
+	ret = qpnp_haptics_write_vmax(haptics);
+	if (ret < 0)
+		return ret;
+
+	/* Configure the ILIM register */
+	ret = qpnp_haptics_write_current_limit(haptics);
+	if (ret < 0)
+		return ret;
+
+	// Configure the debounce for short-circuit detection.
+	val = HAP_SC_DEB_CYCLES_MAX;
+	ret = qpnp_haptics_write_masked(haptics, HAP_SC_DEB_REG(haptics),
+			HAP_SC_DEB_MASK, HAP_SC_DEB_CYCLES_MAX);
+	if (ret < 0)
+		return ret;
+
+	// write the wave shape
+	ret = qpnp_haptics_write_masked(haptics, HAP_CFG2_REG(haptics),
+			HAP_LRA_RES_TYPE_MASK, haptics->wave_shape);
+	if (ret < 0)
+		return ret;
+
+	play_rate = haptics->play_wave_rate / HAP_RATE_CFG_STEP_US;
+
+	/*
+	 * Configure RATE_CFG1 and RATE_CFG2 registers.
+	 * Note: For ERM these registers act as play rate and
+	 * for LRA these represent resonance period
+	 */
+	ret = qpnp_haptics_write_play_rate(haptics, play_rate);
+
+	ret = qpnp_haptics_write_brake(haptics);
+	if (ret < 0)
+		return ret;
+
+	if (haptics->play_mode == HAP_PLAY_BUFFER) {
+		ret = qpnp_haptics_write_wave_repeat(haptics);
+		if (ret < 0)
+			return ret;
+
+		ret = qpnp_haptics_write_buffer_config(haptics);
+	}
+
+	if (haptics->play_irq >= 0) {
+		dev_dbg(&haptics->pdev->dev, "%s: Requesting play IRQ, irq: %d", __func__,
+			haptics->play_irq);
+		ret = devm_request_threaded_irq(&haptics->pdev->dev, haptics->play_irq,
+			NULL, qpnp_haptics_play_irq_handler, IRQF_ONESHOT,
+			"haptics_play_irq", haptics);
+
+		if (ret < 0) {
+			pr_err("Unable to request play IRQ ret=%d\n", ret);
+			return ret;
+		}
+
+		/* use play_irq only for buffer mode */
+		if (haptics->play_mode != HAP_PLAY_BUFFER)
+			disable_irq(haptics->play_irq);
+	}
+
+	if (haptics->sc_irq >= 0) {
+		dev_dbg(&haptics->pdev->dev, "%s: Requesting play IRQ, irq: %d", __func__,
+			haptics->play_irq);
+		ret = devm_request_threaded_irq(&haptics->pdev->dev, haptics->sc_irq,
+			NULL, qpnp_haptics_sc_irq_handler, IRQF_ONESHOT,
+			"haptics_sc_irq", haptics);
+
+		if (ret < 0) {
+			pr_err("Unable to request sc IRQ ret=%d\n", ret);
+			return ret;
+		}
+	}
+
+	return ret;
+}
+
+/**
+ * qpnp_haptics_set - handler to start/stop vibration
+ * @haptics: pointer to haptics struct
+ * @enable: state to set
+ */
+static int qpnp_haptics_set(struct qpnp_haptics *haptics, bool enable)
+{
+	int ret;
+
+	mutex_lock(&haptics->play_lock);
+
+	if (enable) {
+		if (haptics->sc_count > SC_MAX_COUNT) {
+			pr_err("Can't play while in short circuit");
+			ret = -1;
+			goto out;
+		}
+		ret = qpnp_haptics_set_auto_res(haptics, false);
+		if (ret < 0) {
+			pr_err("Error disabling auto_res, ret=%d\n", ret);
+			goto out;
+		}
+
+		ret = qpnp_haptics_module_enable(haptics, true);
+		if (ret < 0) {
+			pr_err("Error enabling module, ret=%d\n", ret);
+			goto out;
+		}
+
+		ret = qpnp_haptics_write_play_control(haptics, HAP_PLAY);
+		if (ret < 0) {
+			pr_err("Error enabling play, ret=%d\n", ret);
+			goto out;
+		}
+
+		ret = qpnp_haptics_set_auto_res(haptics, true);
+		if (ret < 0) {
+			pr_err("Error enabling auto_res, ret=%d\n", ret);
+			goto out;
+		}
+	} else {
+		ret = qpnp_haptics_write_play_control(haptics, HAP_STOP);
+		if (ret < 0) {
+			pr_err("Error disabling play, ret=%d\n", ret);
+			goto out;
+		}
+
+		ret = qpnp_haptics_module_enable(haptics, false);
+		if (ret < 0) {
+			pr_err("Error disabling module, ret=%d\n", ret);
+			goto out;
+		}
+	}
+
+out:
+	mutex_unlock(&haptics->play_lock);
+	return ret;
+}
+
+/*
+ * Threaded function to update the haptics state.
+ */
+static void qpnp_haptics_work(struct work_struct *work)
+{
+	struct qpnp_haptics *haptics = container_of(work, struct qpnp_haptics, work);
+
+	int ret;
+	bool enable;
+
+	enable = atomic_read(&haptics->active);
+	dev_dbg(&haptics->pdev->dev, "%s: state: %d\n", __func__, enable);
+
+	ret = qpnp_haptics_set(haptics, enable);
+	if (ret < 0)
+		pr_err("Error setting haptics, ret=%d", ret);
+}
+
+/**
+ * qpnp_haptics_close - callback for input device close
+ * @dev: input device pointer
+ *
+ * Turns off the vibrator.
+ */
+static void qpnp_haptics_close(struct input_dev *dev)
+{
+	struct qpnp_haptics *haptics = input_get_drvdata(dev);
+
+	cancel_work_sync(&haptics->work);
+	if (atomic_read(&haptics->active)) {
+		atomic_set(&haptics->active, 0);
+		schedule_work(&haptics->work);
+	}
+}
+
+/**
+ * qpnp_haptics_play_effect - play haptics effects
+ * @dev: input device pointer
+ * @data: data of effect
+ * @effect: effect to play
+ */
+static int qpnp_haptics_play_effect(struct input_dev *dev, void *data,
+					struct ff_effect *effect)
+{
+	struct qpnp_haptics *haptics = input_get_drvdata(dev);
+
+	dev_dbg(&haptics->pdev->dev, "%s: Rumbling with strong: %d and weak: %d", __func__,
+		effect->u.rumble.strong_magnitude, effect->u.rumble.weak_magnitude);
+
+	haptics->magnitude = effect->u.rumble.strong_magnitude >> 8;
+	if (!haptics->magnitude)
+		haptics->magnitude = effect->u.rumble.weak_magnitude >> 10;
+
+	if (!haptics->magnitude) {
+		atomic_set(&haptics->active, 0);
+		goto end;
+	}
+
+	atomic_set(&haptics->active, 1);
+
+	haptics->vmax = ((HAP_VMAX_MAX_MV - HAP_VMAX_MIN_MV) * haptics->magnitude) / 100 +
+					HAP_VMAX_MIN_MV;
+
+	dev_dbg(&haptics->pdev->dev, "%s: magnitude: %d, vmax: %d", __func__,
+		haptics->magnitude, haptics->vmax);
+
+	qpnp_haptics_write_vmax(haptics);
+
+end:
+	schedule_work(&haptics->work);
+
+	return 0;
+}
+
+static int qpnp_haptics_probe(struct platform_device *pdev)
+{
+	struct qpnp_haptics *haptics;
+	struct device_node *node;
+	struct input_dev *input_dev;
+	int ret;
+	u32 val;
+	int i;
+
+	haptics = devm_kzalloc(&pdev->dev, sizeof(*haptics), GFP_KERNEL);
+	if (!haptics)
+		return -ENOMEM;
+
+	haptics->regmap = dev_get_regmap(pdev->dev.parent, NULL);
+	if (!haptics->regmap)
+		return -ENODEV;
+
+	node = pdev->dev.of_node;
+
+	haptics->pdev = pdev;
+
+	ret = of_property_read_u32(node, "reg", &haptics->base);
+	if (ret < 0) {
+		pr_err("Couldn't find reg in node = %s ret = %d\n",
+			node->full_name, ret);
+		return ret;
+	}
+
+	if (haptics->base <= 0) {
+		pr_err("Invalid base address: 0x%x\n", haptics->base);
+		return -EINVAL;
+	}
+
+	haptics->play_irq = platform_get_irq_byname(pdev, "play");
+	if (haptics->play_irq < 0) {
+		dev_err(&pdev->dev, "Unable to get play irq\n");
+		ret = haptics->play_irq;
+		goto register_fail;
+	}
+
+	haptics->sc_irq = platform_get_irq_byname(pdev, "short");
+	if (haptics->sc_irq < 0) {
+		dev_err(&pdev->dev, "Unable to get sc irq\n");
+		ret = haptics->sc_irq;
+		goto register_fail;
+	}
+
+	// We only support LRAs for now
+	haptics->actuator_type = HAP_TYPE_LRA;
+	ret = of_property_read_u32(node, "qcom,actuator-type", &val);
+	if (!ret) {
+		if (val != HAP_TYPE_LRA) {
+			dev_err(&pdev->dev, "qcom,actuator-type (%d) isn't supported\n", val);
+			ret = -EINVAL;
+			goto register_fail;
+		}
+		haptics->actuator_type = val;
+	}
+
+	// Only buffer mode is currently supported
+	haptics->play_mode = HAP_PLAY_BUFFER;
+	ret = of_property_read_u32(node, "qcom,play-mode", &val);
+	if (!ret) {
+		if (val != HAP_PLAY_BUFFER) {
+			dev_err(&pdev->dev, "qcom,play-mode (%d) isn't supported\n", val);
+			ret = -EINVAL;
+			goto register_fail;
+		}
+		haptics->play_mode = val;
+	}
+
+	ret = of_property_read_u32(node,
+			"qcom,wave-play-rate-us", &val);
+	if (!ret) {
+		haptics->play_wave_rate = val;
+	} else if (ret != -EINVAL) {
+		pr_err("Unable to read play rate ret=%d\n", ret);
+		goto register_fail;
+	}
+
+	haptics->wave_shape = HAP_WAVE_SINE;
+	ret = of_property_read_u32(node, "qcom,wave-shape", &val);
+	if (!ret) {
+		if (val != HAP_WAVE_SINE && val != HAP_WAVE_SQUARE) {
+			dev_err(&pdev->dev, "qcom,wave-shape is invalid: %d\n", val);
+			ret = -EINVAL;
+			goto register_fail;
+		}
+		haptics->wave_shape = val;
+	}
+
+	ret = of_property_read_u8_array(node, "qcom,brake-pattern", haptics->brake_pat, 4);
+	// Default if not set
+	if (ret == -EINVAL) {
+		haptics->brake_pat[0] = 0x3;
+		haptics->brake_pat[1] = 0x3;
+		haptics->brake_pat[2] = 0x2;
+		haptics->brake_pat[3] = 0x1;
+	} else if (ret) {
+		dev_err(&pdev->dev, "qcom,brake-pattern is invalid, ret = %d\n", ret);
+	}
+
+	haptics->current_limit = HAP_ILIM_400_MA;
+
+	for (i = 0; i < HAP_WAVE_SAMP_LEN; i++)
+		haptics->wave_samp[i] = HAP_WF_SAMP_MAX;
+
+	haptics->play_wave_rate =
+		clamp_t(u32, haptics->play_wave_rate,
+		HAP_WAVE_PLAY_RATE_MIN_US, HAP_WAVE_PLAY_RATE_MAX_US);
+
+	ret = qpnp_haptics_init(haptics);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "Error initialising haptics, ret=%d\n",
+			ret);
+		goto register_fail;
+	}
+
+	platform_set_drvdata(pdev, haptics);
+
+	input_dev = devm_input_allocate_device(&pdev->dev);
+	if (!input_dev)
+		return -ENOMEM;
+
+	INIT_WORK(&haptics->work, qpnp_haptics_work);
+	haptics->haptics_input_dev = input_dev;
+
+	input_dev->name = "qpnp_haptics";
+	input_dev->id.version = 1;
+	input_dev->close = qpnp_haptics_close;
+	input_set_drvdata(input_dev, haptics);
+	// Figure out how to make this FF_PERIODIC
+	input_set_capability(haptics->haptics_input_dev, EV_FF, FF_RUMBLE);
+
+	ret = input_ff_create_memless(input_dev, NULL,
+					qpnp_haptics_play_effect);
+	if (ret) {
+		dev_err(&pdev->dev,
+			"couldn't register vibrator as FF device\n");
+		goto register_fail;
+	}
+
+	ret = input_register_device(input_dev);
+	if (ret) {
+		dev_err(&pdev->dev, "couldn't register input device\n");
+		goto register_fail;
+	}
+
+	return 0;
+
+register_fail:
+	cancel_work_sync(&haptics->work);
+	mutex_destroy(&haptics->play_lock);
+
+	return ret;
+}
+
+static int __maybe_unused qpnp_haptics_suspend(struct device *dev)
+{
+	struct qpnp_haptics *haptics = dev_get_drvdata(dev);
+
+	cancel_work_sync(&haptics->work);
+	qpnp_haptics_set(haptics, false);
+
+	return 0;
+}
+
+static SIMPLE_DEV_PM_OPS(qpnp_haptics_pm_ops, qpnp_haptics_suspend, NULL);
+
+static int qpnp_haptics_remove(struct platform_device *pdev)
+{
+	struct qpnp_haptics *haptics = dev_get_drvdata(&pdev->dev);
+
+	cancel_work_sync(&haptics->work);
+	mutex_destroy(&haptics->play_lock);
+	dev_set_drvdata(&pdev->dev, NULL);
+
+	return 0;
+}
+
+static void qpnp_haptics_shutdown(struct platform_device *pdev)
+{
+	struct qpnp_haptics *haptics = dev_get_drvdata(&pdev->dev);
+
+	cancel_work_sync(&haptics->work);
+
+	qpnp_haptics_set(haptics, false);
+}
+
+static const struct of_device_id qpnp_haptics_match_table[] = {
+	{ .compatible = "qcom,qpnp-haptics" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, qpnp_haptics_match_table);
+
+static struct platform_driver qpnp_haptics_driver = {
+	.probe		= qpnp_haptics_probe,
+	.remove		= qpnp_haptics_remove,
+	.shutdown	= qpnp_haptics_shutdown,
+	.driver		= {
+		.name	= "qpnp-haptics",
+		.pm	= &qpnp_haptics_pm_ops,
+		.of_match_table = qpnp_haptics_match_table,
+	},
+};
+module_platform_driver(qpnp_haptics_driver);
+
+MODULE_ALIAS("platform:qpnp_haptics");
+MODULE_DESCRIPTION("QPNP haptics driver using ff-memless framework");
+MODULE_LICENSE("GPL v2");
+MODULE_AUTHOR("Caleb Connolly <caleb@connolly.tech>");
--
2.31.1

Re: [PATCH 2/4] input: add Qualcomm QPNP haptics driver

[Bjorn Andersson]

On Sat 12 Jun 15:54 CDT 2021, Caleb Connolly wrote:

> Add support for the haptics found in pmi8998 and related PMICs.
> Based on the ff-memless interface. Currently this driver provides
> a partial implementation of hardware features.
> 
> This driver only supports LRAs (Linear Resonant Actuators) in the "buffer"
> mode with a single wave pattern.
> 
> Signed-off-by: Caleb Connolly <caleb@connolly.tech>

Please use a cover letter when posting multiple patches.

> ---
>  drivers/input/misc/Kconfig        |   11 +
>  drivers/input/misc/Makefile       |    1 +
>  drivers/input/misc/qpnp-haptics.c | 1022 +++++++++++++++++++++++++++++
>  3 files changed, 1034 insertions(+)
>  create mode 100644 drivers/input/misc/qpnp-haptics.c
> 
> diff --git a/drivers/input/misc/Kconfig b/drivers/input/misc/Kconfig
> index 498cde376981..b5ba03e6cf58 100644
> --- a/drivers/input/misc/Kconfig
> +++ b/drivers/input/misc/Kconfig
> @@ -186,6 +186,17 @@ config INPUT_PMIC8XXX_PWRKEY
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called pmic8xxx-pwrkey.
> 
> +config INPUT_QPNP_HAPTICS

No-one knows what "QPNP" really is, so please name this
"INPUT_QCOM_SPMI_HAPTICS"

> +	tristate "Qualcomm QPNP HAPTICS"
> +	depends on ARCH_QCOM

"depends on SPMI" makes sense here.

> +	select INPUT_FF_MEMLESS
> +	help
> +	  This option enables support for the haptics found in pmi8998 and
> +	  related PMICs. Based on the ff-memless interface.
> +
> +	  To compile this driver as module, choose M here: the
> +	  module will be called qpnp-haptics.
> +
>  config INPUT_SPARCSPKR
>  	tristate "SPARC Speaker support"
>  	depends on PCI && SPARC64
> diff --git a/drivers/input/misc/Makefile b/drivers/input/misc/Makefile
> index f593beed7e05..c43290163db0 100644
> --- a/drivers/input/misc/Makefile
> +++ b/drivers/input/misc/Makefile
> @@ -65,6 +65,7 @@ obj-$(CONFIG_INPUT_PMIC8XXX_PWRKEY)	+= pmic8xxx-pwrkey.o
>  obj-$(CONFIG_INPUT_POWERMATE)		+= powermate.o
>  obj-$(CONFIG_INPUT_PWM_BEEPER)		+= pwm-beeper.o
>  obj-$(CONFIG_INPUT_PWM_VIBRA)		+= pwm-vibra.o
> +obj-$(CONFIG_INPUT_QPNP_HAPTICS)	+= qpnp-haptics.o
>  obj-$(CONFIG_INPUT_RAVE_SP_PWRBUTTON)	+= rave-sp-pwrbutton.o
>  obj-$(CONFIG_INPUT_RB532_BUTTON)	+= rb532_button.o
>  obj-$(CONFIG_INPUT_REGULATOR_HAPTIC)	+= regulator-haptic.o
> diff --git a/drivers/input/misc/qpnp-haptics.c b/drivers/input/misc/qpnp-haptics.c

Again, qcom-spmi-haptics.c

> new file mode 100644
> index 000000000000..daa7a18ffc7d
> --- /dev/null
> +++ b/drivers/input/misc/qpnp-haptics.c
> @@ -0,0 +1,1022 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2021, Caleb Connolly <caleb@connolly.tech>
> + * Qualcomm Plug and Play haptics driver for pmi8998 and related PMICs.
> + * Based on ./pm8xxx-vibrator.c

Enough "based on" to warrant carrying some copyrights?

> + */
> +
> +#include <dt-bindings/input/qcom,qpnp-haptics.h>
> +
> +#include <linux/atomic.h>
> +#include <linux/bits.h>
> +#include <linux/errno.h>
> +#include <linux/input.h>
> +#include <linux/interrupt.h>
> +#include <linux/kernel.h>
> +#include <linux/log2.h>
> +#include <linux/minmax.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +#include <linux/slab.h>
> +#include <linux/time.h>
> +#include <linux/types.h>
> +
> +/*
> + * Register and bit definitions
> + */

This comment doesn't add much value.

> +
> +#define HAP_STATUS_1_REG(chip)		(chip->base + 0x0A)

This style is somewhat unusual, please define the offset here and do the
math in qpnp_haptics_read() et al.

> +#define HAP_BUSY_BIT			BIT(1)
> +#define SC_FLAG_BIT			BIT(3)
> +#define AUTO_RES_ERROR_BIT		BIT(4)
> +
> +#define HAP_LRA_AUTO_RES_LO_REG(chip)	(chip->base + 0x0B)
> +#define HAP_LRA_AUTO_RES_HI_REG(chip)	(chip->base + 0x0C)
> +
> +#define HAP_EN_CTL_REG(chip)		(chip->base + 0x46)
> +#define HAP_EN_BIT			BIT(7)
> +
> +#define HAP_EN_CTL2_REG(chip)		(chip->base + 0x48)
> +#define BRAKE_EN_BIT			BIT(0)
> +
> +#define HAP_AUTO_RES_CTRL_REG(chip)	(chip->base + 0x4B)
> +#define AUTO_RES_EN_BIT			BIT(7)
> +#define AUTO_RES_ERR_RECOVERY_BIT	BIT(3)
> +#define AUTO_RES_EN_FLAG_BIT	BIT(0)
> +
> +#define HAP_CFG1_REG(chip)		(chip->base + 0x4C)
> +#define HAP_ACT_TYPE_MASK		BIT(0)
> +
> +#define HAP_CFG2_REG(chip)		(chip->base + 0x4D)
> +#define HAP_LRA_RES_TYPE_MASK		BIT(0)
> +
> +#define HAP_SEL_REG(chip)		(chip->base + 0x4E)
> +#define HAP_WF_SOURCE_MASK		GENMASK(5, 4)
> +#define HAP_WF_SOURCE_SHIFT		4
> +
> +#define HAP_LRA_AUTO_RES_REG(chip)	(chip->base + 0x4F)
> +#define LRA_AUTO_RES_MODE_MASK		GENMASK(6, 4)
> +#define LRA_AUTO_RES_MODE_SHIFT		4
> +#define LRA_HIGH_Z_MASK			GENMASK(3, 2)
> +#define LRA_HIGH_Z_SHIFT		2
> +#define LRA_RES_CAL_MASK		GENMASK(1, 0)
> +#define HAP_RES_CAL_PERIOD_MIN		4
> +#define HAP_RES_CAL_PERIOD_MAX		32
> +
> +#define HAP_VMAX_CFG_REG(chip)		(chip->base + 0x51)
> +#define HAP_VMAX_OVD_BIT		BIT(6)
> +#define HAP_VMAX_MASK			GENMASK(5, 1)
> +#define HAP_VMAX_SHIFT			1
> +
> +#define HAP_ILIM_CFG_REG(chip)		(chip->base + 0x52)
> +#define HAP_ILIM_SEL_MASK		BIT(0)
> +#define HAP_ILIM_400_MA			0
> +#define HAP_ILIM_800_MA			1
> +
> +#define HAP_SC_DEB_REG(chip)		(chip->base + 0x53)
> +#define HAP_SC_DEB_MASK			GENMASK(2, 0)
> +#define HAP_SC_DEB_CYCLES_MIN		0
> +#define HAP_DEF_SC_DEB_CYCLES		8
> +#define HAP_SC_DEB_CYCLES_MAX		32
> +
> +#define HAP_RATE_CFG1_REG(chip)		(chip->base + 0x54)
> +#define HAP_RATE_CFG1_MASK		GENMASK(7, 0)
> +#define HAP_RATE_CFG2_SHIFT		8 // As CFG2 is the most significant byte
> +
> +#define HAP_RATE_CFG2_REG(chip)		(chip->base + 0x55)
> +#define HAP_RATE_CFG2_MASK		GENMASK(3, 0)
> +
> +#define HAP_SC_CLR_REG(chip)		(chip->base + 0x59)
> +#define SC_CLR_BIT			BIT(0)
> +
> +#define HAP_BRAKE_REG(chip)		(chip->base + 0x5C)
> +#define HAP_BRAKE_PAT_MASK		0x3
> +
> +#define HAP_WF_REPEAT_REG(chip)		(chip->base + 0x5E)
> +#define WF_REPEAT_MASK			GENMASK(6, 4)
> +#define WF_REPEAT_SHIFT			4
> +#define WF_REPEAT_MIN			1
> +#define WF_REPEAT_MAX			128
> +#define WF_S_REPEAT_MASK		GENMASK(1, 0)
> +#define WF_S_REPEAT_MIN			1
> +#define WF_S_REPEAT_MAX			8
> +
> +#define HAP_WF_S1_REG(chip)		(chip->base + 0x60)
> +#define HAP_WF_SIGN_BIT			BIT(7)
> +#define HAP_WF_OVD_BIT			BIT(6)
> +#define HAP_WF_SAMP_MAX			GENMASK(5, 1)
> +#define HAP_WF_SAMPLE_LEN		8
> +
> +#define HAP_PLAY_REG(chip)		(chip->base + 0x70)
> +#define HAP_PLAY_BIT			BIT(7)
> +#define HAP_PAUSE_BIT			BIT(0)
> +
> +#define HAP_SEC_ACCESS_REG(chip)	(chip->base + 0xD0)
> +#define HAP_SEC_ACCESS_UNLOCK		0xA5
> +
> +#define HAP_TEST2_REG(chip)		(chip->base + 0xE3)
> +
> +// Values
> +#define HAP_VMAX_MIN_MV			116
> +#define HAP_VMAX_MAX_MV			3596
> +#define HAP_VMAX_MAX_MV_STRONG		3596
> +
> +#define HAP_WAVE_PLAY_RATE_MIN_US	0
> +#define HAP_WAVE_PLAY_RATE_MAX_US	20475
> +#define HAP_WAVE_PLAY_TIME_MAX_MS	15000
> +
> +#define AUTO_RES_ERR_POLL_TIME_NS	(20 * NSEC_PER_MSEC)
> +#define HAPTICS_BACK_EMF_DELAY_US	20000
> +
> +#define HAP_BRAKE_PAT_LEN		4
> +#define HAP_WAVE_SAMP_LEN		8
> +#define NUM_WF_SET			4
> +#define HAP_WAVE_SAMP_SET_LEN		(HAP_WAVE_SAMP_LEN * NUM_WF_SET)
> +#define HAP_RATE_CFG_STEP_US		5
> +
> +#define SC_MAX_COUNT			5
> +#define SC_COUNT_RST_DELAY_US		1000000
> +
> +enum hap_play_control {
> +	HAP_STOP,
> +	HAP_PAUSE,
> +	HAP_PLAY,
> +};
> +
> +/**
> + * struct qpnp_haptics - struct for qpnp haptics data.
> + *
> + * @pdev: The platform device responsible for the haptics.
> + * @regmap: Register map for the hardware block.
> + * @input_dev: The input device used to receive events.
> + * @work: Work struct to play effects.
> + * @base: Base address of the regmap.
> + * @active: Atomic value used to track if haptics are currently playing.
> + * @play_irq: Fired to load the next wave pattern.
> + * @sc_irq: Short circuit irq.
> + * @last_sc_time: Time since the short circuit IRQ last fired.
> + * @sc_count: Number of times the short circuit IRQ has fired in this interval.
> + * @actuator_type: The type of actuator in use.
> + * @wave_shape: The shape of the waves to use (sine or square).
> + * @play_mode: The play mode to use (direct, buffer, pwm, audio).
> + * @magnitude: The strength we should be playing at.
> + * @vmax: Max voltage to use when playing.
> + * @current_limit: The current limit for this hardware (400mA or 800mA).
> + * @play_wave_rate: The wave rate to use for this hardware.
> + * @wave_samp: The array of wave samples to write for buffer mode.
> + * @brake_pat: The pattern to apply when braking.
> + * @play_lock: Lock to be held when updating the hardware state.
> + */
> +struct qpnp_haptics {
> +	struct platform_device *pdev;

You only ever use this to get to &pdev->dev, so why not store that
instead?

> +	struct regmap *regmap;
> +	struct input_dev *haptics_input_dev;
> +	struct work_struct work;
> +	u32 base;
> +
> +	atomic_t active;
> +
> +	int play_irq;
> +	int sc_irq;
> +	ktime_t last_sc_time;
> +	u8 sc_count;
> +
> +	u8 actuator_type;
> +	u8 wave_shape;
> +	u8 play_mode;
> +	int magnitude;
> +	u32 vmax;
> +	u32 current_limit;
> +	u32 play_wave_rate;
> +
> +	u32 wave_samp[HAP_WAVE_SAMP_SET_LEN];
> +	u8 brake_pat[HAP_BRAKE_PAT_LEN];
> +
> +	struct mutex play_lock;
> +};
> +
> +static inline bool is_secure_addr(u16 addr)
> +{
> +	return (addr & 0xFF) > 0xD0;
> +}
> +
> +static int qpnp_haptics_read(struct qpnp_haptics *haptics,
> +	u16 addr, u8 *val, int len)
> +{
> +	int ret;
> +
> +	ret = regmap_bulk_read(haptics->regmap, addr, val, len);
> +	if (ret < 0)
> +		pr_err("Error reading address: 0x%x, ret %d\n", addr, ret);

dev_err()

> +
> +	return ret;
> +}
> +
> +static int qpnp_haptics_write(struct qpnp_haptics *haptics,
> +	u16 addr, u8 *val, int len)
> +{
> +	int ret, i;
> +
> +	if (is_secure_addr(addr)) {
> +		for (i = 0; i < len; i++) {
> +			pr_info("%s: unlocking for addr: 0x%x, val: 0x%x", __func__,
> +				addr, val[i]);

This is more of a "debug print", so dev_dbg() would be better.

> +			ret = regmap_write(haptics->regmap,
> +				HAP_SEC_ACCESS_REG(haptics), HAP_SEC_ACCESS_UNLOCK);
> +			if (ret < 0) {
> +				pr_err("Error writing unlock code, ret %d\n",
> +					ret);

dev_err()

> +				return ret;
> +			}
> +
> +			ret = regmap_write(haptics->regmap, addr + i, val[i]);
> +			if (ret < 0) {
> +				pr_err("Error writing address 0x%x, ret %d\n",
> +					addr + i, ret);
> +				return ret;
> +			}
> +		}
> +	} else {
> +		if (len > 1)
> +			ret = regmap_bulk_write(haptics->regmap, addr, val, len);
> +		else
> +			ret = regmap_write(haptics->regmap, addr, *val);
> +	}
> +
> +	if (ret < 0)
> +		pr_err("%s: Error writing address: 0x%x, ret %d\n", __func__, addr, ret);
> +
> +	return ret;
> +}
> +
> +static int qpnp_haptics_write_masked(struct qpnp_haptics *haptics,
> +	u16 addr, u8 mask, u8 val)
> +{
> +	int ret;
> +
> +	if (is_secure_addr(addr)) {
> +		ret = regmap_write(haptics->regmap,
> +			HAP_SEC_ACCESS_REG(haptics), HAP_SEC_ACCESS_UNLOCK);
> +		if (ret < 0) {
> +			pr_err("Error writing unlock code - ret %d\n", ret);
> +			return ret;
> +		}
> +	}
> +
> +	ret = regmap_update_bits(haptics->regmap, addr, mask, val);
> +	if (ret < 0)
> +		pr_err("Error writing address: 0x%x - ret %d\n", addr, ret);
> +
> +	return ret;
> +}
> +
> +static bool is_haptics_idle(struct qpnp_haptics *haptics)
> +{
> +	int ret;
> +	u8 val;
> +
> +	if (haptics->play_mode == HAP_PLAY_DIRECT ||
> +			haptics->play_mode == HAP_PLAY_PWM)
> +		return true;
> +
> +	ret = qpnp_haptics_read(haptics, HAP_STATUS_1_REG(haptics), &val, 1);
> +	if (ret < 0 || (val & HAP_BUSY_BIT))
> +		return false;
> +
> +	return true;
> +}
> +
> +static int qpnp_haptics_module_enable(struct qpnp_haptics *haptics, bool enable)
> +{
> +	u8 val;
> +	int rc;
> +
> +	dev_dbg(&haptics->pdev->dev, "Setting module enable: %d", enable);
> +
> +	val = enable ? HAP_EN_BIT : 0;
> +	rc = qpnp_haptics_write(haptics, HAP_EN_CTL_REG(haptics), &val, 1);

rc will either be < 0 or 0, so you can just do:
	return qpnp_haptics_write();

> +	if (rc < 0)
> +		return rc;
> +
> +	return 0;
> +}
> +
> +static int qpnp_haptics_write_vmax(struct qpnp_haptics *haptics)
> +{
> +	u8 val = 0;
> +	int ret;
> +	u32 vmax_mv = haptics->vmax;
> +
> +	vmax_mv = clamp_t(u32, vmax_mv, HAP_VMAX_MIN_MV, HAP_VMAX_MAX_MV);
> +
> +	dev_dbg(&haptics->pdev->dev, "Setting vmax to: %d", vmax_mv);
> +
> +	val = DIV_ROUND_CLOSEST(vmax_mv, HAP_VMAX_MIN_MV);
> +	val <<= HAP_VMAX_SHIFT;
> +	// overdrive is only supported on pm660

Good to know...so what should I do with that information?

> +	val &= ~HAP_VMAX_OVD_BIT;

The only way I can see this bit being set is somehow vmax_mv turnes out
to be more than 5 bits, but then you have a problem.

Consider using FIELD_PREP to stuff the result in the bits and skip this.

> +
> +	ret = qpnp_haptics_write_masked(haptics, HAP_VMAX_CFG_REG(haptics),
> +			HAP_VMAX_MASK | HAP_VMAX_OVD_BIT, val);
> +	return ret;
> +}
> +
> +static int qpnp_haptics_write_current_limit(struct qpnp_haptics *haptics)
> +{
> +	int ret;
> +
> +	haptics->current_limit = clamp_t(u32, haptics->current_limit,
> +					 HAP_ILIM_400_MA, HAP_ILIM_800_MA);
> +
> +	dev_dbg(&haptics->pdev->dev, "Setting current_limit to: 0x%x", haptics->current_limit);
> +
> +	ret = qpnp_haptics_write_masked(haptics, HAP_ILIM_CFG_REG(haptics),
> +			HAP_ILIM_SEL_MASK, haptics->current_limit);
> +	return ret;
> +}
> +
> +static int qpnp_haptics_write_play_mode(struct qpnp_haptics *haptics)
> +{
> +	u8 val = 0;
> +	int ret;
> +
> +	if (!is_haptics_idle(haptics))
> +		return -EBUSY;
> +
> +	dev_dbg(&haptics->pdev->dev, "Setting play_mode to: 0x%x", haptics->play_mode);
> +
> +	val = haptics->play_mode << HAP_WF_SOURCE_SHIFT;

How about:
	val = FIELD_PREP(HAP_WF_SOURCE_MASK, haptics->play_mode);

> +	ret = qpnp_haptics_write_masked(haptics, HAP_SEL_REG(haptics),
> +			HAP_WF_SOURCE_MASK, val);

return qpnp_haptics_write_masked()

> +
> +	return ret;
> +}
> +
> +static int qpnp_haptics_write_play_rate(struct qpnp_haptics *haptics, u16 play_rate)
> +{
> +	int rc;
> +	u8 val[2];
> +
> +	dev_dbg(&haptics->pdev->dev, "Setting play_rate to: %d", play_rate);
> +
> +	val[0] = play_rate & HAP_RATE_CFG1_MASK;
> +	val[1] = (play_rate >> HAP_RATE_CFG2_SHIFT) & HAP_RATE_CFG2_MASK;

Make val a u16 and FIELD_PREP() the bits in there, will be easier to
reason about.

> +	rc = qpnp_haptics_write(haptics, HAP_RATE_CFG1_REG(haptics), val, 2);
> +	if (rc < 0)
> +		return rc;
> +
> +	return 0;
> +}
> +
> +/*
> + * qpnp_haptics_set_auto_res() - Auto resonance
> + * allows the haptics to automatically adjust the
> + * speed of the oscillation in order to maintain
> + * the resonant frequency.
> + */
> +static int qpnp_haptics_set_auto_res(struct qpnp_haptics *haptics, bool enable)
> +{
> +	int rc = 0;
> +	u8 val;
> +
> +	// LRAs are the only type to support auto res
> +	if (haptics->actuator_type != HAP_TYPE_LRA)
> +		return 0;
> +
> +	val = enable ? AUTO_RES_EN_BIT : 0;
> +
> +	rc = qpnp_haptics_write_masked(haptics, HAP_TEST2_REG(haptics),
> +			AUTO_RES_EN_BIT, val);

return qpnp_haptics_write_masked();

> +	if (rc < 0)
> +		return rc;
> +
> +	dev_dbg(&haptics->pdev->dev, "Auto resonance enabled: %d", enable);
> +	return rc;
> +}
> +
> +static int qpnp_haptics_write_brake(struct qpnp_haptics *haptics)
> +{
> +	int ret, i;
> +	u32 temp;
> +	u8 val;
> +
> +	dev_dbg(&haptics->pdev->dev, "Configuring brake pattern");
> +
> +	ret = qpnp_haptics_write_masked(haptics, HAP_EN_CTL2_REG(haptics),
> +			BRAKE_EN_BIT, 1);
> +	if (ret < 0)
> +		return ret;
> +
> +	for (i = HAP_BRAKE_PAT_LEN - 1, val = 0; i >= 0; i--) {
> +		u8 p = haptics->brake_pat[i] & HAP_BRAKE_PAT_MASK;
> +
> +		temp = i << 1;
> +		val |= p << temp;

		val |= p << (i * 2);

Would make this trivial to understand. The temp variable and the extra
bitshifting is confusing, and the compiler will help you with the
optimization.

> +	}
> +
> +	ret = qpnp_haptics_write(haptics, HAP_BRAKE_REG(haptics), &val, 1);

return qpnp_haptics_write();

> +	if (ret < 0)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +static int qpnp_haptics_write_buffer_config(struct qpnp_haptics *haptics)
> +{
> +	u8 buf[HAP_WAVE_SAMP_LEN];
> +	int rc, i;
> +
> +	dev_dbg(&haptics->pdev->dev, "Writing buffer config");
> +
> +	for (i = 0; i < HAP_WAVE_SAMP_LEN; i++)
> +		buf[i] = haptics->wave_samp[i];
> +
> +	rc = qpnp_haptics_write(haptics, HAP_WF_S1_REG(haptics), buf,
> +			HAP_WAVE_SAMP_LEN);

return qpnp_haptics_write();

> +
> +	return rc;
> +}
> +
> +/**
> + * qpnp_haptics_write_wave_repeat() - write wave repeat values.

This kernel-doc is incomplete. Either define it fully or drop one of the
'*'

> + */
> +static int qpnp_haptics_write_wave_repeat(struct qpnp_haptics *haptics)
> +{
> +	int ret;
> +	u8 val = 0, mask = 0;

No need to zero-initialize these.

> +
> +	// The number of times to repeat each wave

/* Comments */

> +	mask = WF_REPEAT_MASK;
> +	val = ilog2(1) << WF_REPEAT_SHIFT; // Currently hard coded to default of 1

So how about val = 0; ?

If you want to leave some of this code around as a todo, perhaps write
it as:

	mask = WF_REPEAT_MASK | WF_S_REPEAT_MASK;
	val = FIELD_PREP(WF_REPEAT_MASK, 0) |
	      FIELD_PREP(WF_S_REPEAT_MASK, 0);

	return qpnp_haptics_write_masked();

> +
> +	// the number of times to repeat each wave sample (group of waves)
> +	mask |= WF_S_REPEAT_MASK;
> +	val |= ilog2(1);
> +
> +	ret = qpnp_haptics_write_masked(haptics, HAP_WF_REPEAT_REG(haptics),
> +			mask, val);
> +	return ret;
> +}
> +
> +static int qpnp_haptics_write_play_control(struct qpnp_haptics *haptics,
> +						enum hap_play_control ctrl)
> +{
> +	u8 val;
> +	int rc;
> +
> +	switch (ctrl) {
> +	case HAP_STOP:
> +		val = 0;
> +		break;
> +	case HAP_PAUSE:
> +		val = HAP_PAUSE_BIT;
> +		break;
> +	case HAP_PLAY:
> +		val = HAP_PLAY_BIT;
> +		break;
> +	default:
> +		return 0;
> +	}
> +
> +	rc = qpnp_haptics_write(haptics, HAP_PLAY_REG(haptics), &val, 1);
> +	if (rc < 0) {
> +		pr_err("Error writing to PLAY_REG, rc=%d\n", rc);

You already printed in all possible code paths through
qpnp_haptics_write()


Move the dev_dbg below up above the write and you can simply:

	return qpnp_haptics_write();

> +		return rc;
> +	}
> +
> +	dev_dbg(&haptics->pdev->dev, "haptics play ctrl: %d\n", ctrl);
> +	return rc;
> +}
> +
> +/*
> + * This IRQ is fired to tell us to load the next wave sample set.
> + * As we only currently support a single sample set, it's unused.
> + */
> +static irqreturn_t qpnp_haptics_play_irq_handler(int irq, void *data)
> +{
> +	struct qpnp_haptics *haptics = data;
> +
> +	dev_dbg(&haptics->pdev->dev, "play_irq triggered");
> +
> +	return IRQ_HANDLED;
> +}
> +
> +/*
> + * Fires every ~50ms whilst the haptics are active.
> + * If the SC_FLAG_BIT is set then that means there isn't a short circuit
> + * and we just need to clear the IRQ to indicate that the device should
> + * keep vibrating.
> + *
> + * Otherwise, it means a short circuit situation has occurred.
> + */
> +static irqreturn_t qpnp_haptics_sc_irq_handler(int irq, void *data)
> +{
> +	struct qpnp_haptics *haptics = data;
> +	int ret;
> +	u8 val;
> +	s64 sc_delta_time_us;
> +	ktime_t temp;
> +
> +	ret = qpnp_haptics_read(haptics, HAP_STATUS_1_REG(haptics), &val, 1);
> +	if (ret < 0)
> +		return IRQ_HANDLED;
> +
> +	if (!(val & SC_FLAG_BIT)) {
> +		haptics->sc_count = 0;
> +		return IRQ_HANDLED;
> +	}
> +
> +	temp = ktime_get();
> +	sc_delta_time_us = ktime_us_delta(temp, haptics->last_sc_time);
> +	haptics->last_sc_time = temp;
> +
> +	if (sc_delta_time_us > SC_COUNT_RST_DELAY_US)
> +		haptics->sc_count = 0;
> +	else
> +		haptics->sc_count++;
> +
> +	// Clear the interrupt flag
> +	val = SC_CLR_BIT;
> +	ret = qpnp_haptics_write(haptics, HAP_SC_CLR_REG(haptics), &val, 1);
> +	if (ret < 0) {
> +		pr_err("Error writing to SC_CLR_REG, rc=%d\n", ret);

You already printed in qpnp_haptics_write()

> +		return IRQ_HANDLED;
> +	}
> +
> +	if (haptics->sc_count > SC_MAX_COUNT) {
> +		pr_crit("Short circuit persists, disabling haptics\n");

dev_crit()?

> +		ret = qpnp_haptics_module_enable(haptics, false);
> +		if (ret < 0) {
> +			pr_err("Error disabling module, rc=%d\n", ret);

dev_err(), and qpnp_haptics_module_enable() already printed an error.

That said, these error messages are more useful, so perhaps it's worth
deciding to move it out to the callers?

> +			return IRQ_HANDLED;

You can just fall down do the return below...

> +		}
> +	}
> +
> +	return IRQ_HANDLED;
> +}
> +
> +
> +/**
> + * qpnp_haptics_init() - Initialise haptics hardware for use
> + * @haptics: haptics device

Missing "Returns:"

> + */
> +static int qpnp_haptics_init(struct qpnp_haptics *haptics)
> +{
> +	int ret;
> +	u8 val, mask;
> +	u16 auto_res_mode;
> +	u16 play_rate = 0;

First use is an assignment, no need to zero-initialize.

> +
> +	ret = qpnp_haptics_write_masked(haptics, HAP_CFG1_REG(haptics),
> +		HAP_ACT_TYPE_MASK, haptics->actuator_type);
> +	if (ret < 0)
> +		return ret;
> +
> +	// Configure auto resonance

/*
 * Multi-line comment.
 */

> +	// see qpnp_haptics_lra_auto_res_config downstream
> +	// This is greatly simplified.
> +	auto_res_mode = HAP_AUTO_RES_ZXD_EOP << LRA_AUTO_RES_MODE_SHIFT;
> +
> +	//Default LRA calibration period
> +	val = ilog2(32 / HAP_RES_CAL_PERIOD_MIN);
> +
> +	val |= (auto_res_mode << LRA_AUTO_RES_MODE_SHIFT);

You shift HAP_AUTO_RES_ZXD_EOP twice here.

> +	// The 1 here is for OPT2 "High Z configuration", there are 3
> +	// options and no documentation indicating the difference
> +	val |= (1 << LRA_HIGH_Z_SHIFT);

Consider writing this as:

	val = FIELD_PREP(LRA_RES_CAL_MASK, ilog2(32 / HAP_RES_CAL_PERIOD_MIN) |
	      FIELD_PREP(LRA_AUTO_RES_MODE_MASK, HAP_AUTO_RES_ZXD_EOP) |
	      FIELD_PREP(LRA_HIGH_Z_MASK, 1);

> +	mask = LRA_AUTO_RES_MODE_MASK | LRA_HIGH_Z_MASK | LRA_RES_CAL_MASK;
> +
> +	ret = qpnp_haptics_write_masked(haptics, HAP_LRA_AUTO_RES_REG(haptics),
> +			mask, val);
> +
> +	dev_dbg(&haptics->pdev->dev, "%s: auto_res_mode: %d\n", __func__,
> +		auto_res_mode);
> +
> +	/* Configure the PLAY MODE register */
> +	ret = qpnp_haptics_write_play_mode(haptics);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = qpnp_haptics_write_vmax(haptics);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* Configure the ILIM register */
> +	ret = qpnp_haptics_write_current_limit(haptics);
> +	if (ret < 0)
> +		return ret;
> +
> +	// Configure the debounce for short-circuit detection.
> +	val = HAP_SC_DEB_CYCLES_MAX;

This val is unused.

> +	ret = qpnp_haptics_write_masked(haptics, HAP_SC_DEB_REG(haptics),
> +			HAP_SC_DEB_MASK, HAP_SC_DEB_CYCLES_MAX);
> +	if (ret < 0)
> +		return ret;
> +
> +	// write the wave shape
> +	ret = qpnp_haptics_write_masked(haptics, HAP_CFG2_REG(haptics),
> +			HAP_LRA_RES_TYPE_MASK, haptics->wave_shape);
> +	if (ret < 0)
> +		return ret;
> +
> +	play_rate = haptics->play_wave_rate / HAP_RATE_CFG_STEP_US;
> +
> +	/*
> +	 * Configure RATE_CFG1 and RATE_CFG2 registers.
> +	 * Note: For ERM these registers act as play rate and
> +	 * for LRA these represent resonance period
> +	 */
> +	ret = qpnp_haptics_write_play_rate(haptics, play_rate);

You don't check this ret.

> +
> +	ret = qpnp_haptics_write_brake(haptics);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (haptics->play_mode == HAP_PLAY_BUFFER) {
> +		ret = qpnp_haptics_write_wave_repeat(haptics);
> +		if (ret < 0)
> +			return ret;
> +
> +		ret = qpnp_haptics_write_buffer_config(haptics);
> +	}
> +
> +	if (haptics->play_irq >= 0) {

Afaict, you won't leave probe successfully with this < 0.

> +		dev_dbg(&haptics->pdev->dev, "%s: Requesting play IRQ, irq: %d", __func__,
> +			haptics->play_irq);
> +		ret = devm_request_threaded_irq(&haptics->pdev->dev, haptics->play_irq,
> +			NULL, qpnp_haptics_play_irq_handler, IRQF_ONESHOT,
> +			"haptics_play_irq", haptics);
> +
> +		if (ret < 0) {
> +			pr_err("Unable to request play IRQ ret=%d\n", ret);
> +			return ret;
> +		}
> +
> +		/* use play_irq only for buffer mode */
> +		if (haptics->play_mode != HAP_PLAY_BUFFER)
> +			disable_irq(haptics->play_irq);
> +	}
> +
> +	if (haptics->sc_irq >= 0) {

Same here.

> +		dev_dbg(&haptics->pdev->dev, "%s: Requesting play IRQ, irq: %d", __func__,
> +			haptics->play_irq);
> +		ret = devm_request_threaded_irq(&haptics->pdev->dev, haptics->sc_irq,
> +			NULL, qpnp_haptics_sc_irq_handler, IRQF_ONESHOT,
> +			"haptics_sc_irq", haptics);
> +
> +		if (ret < 0) {
> +			pr_err("Unable to request sc IRQ ret=%d\n", ret);
> +			return ret;
> +		}
> +	}
> +
> +	return ret;

ret is 0 when you get here.

> +}
> +
> +/**
> + * qpnp_haptics_set - handler to start/stop vibration
> + * @haptics: pointer to haptics struct
> + * @enable: state to set

  * Returns:

> + */
> +static int qpnp_haptics_set(struct qpnp_haptics *haptics, bool enable)

Consider splitting this into haptics_enable() and haptics_disable(), as
this really just is two separate functions squashed into one.

> +{
> +	int ret;
> +
> +	mutex_lock(&haptics->play_lock);
> +
> +	if (enable) {
> +		if (haptics->sc_count > SC_MAX_COUNT) {
> +			pr_err("Can't play while in short circuit");
> +			ret = -1;
> +			goto out;
> +		}
> +		ret = qpnp_haptics_set_auto_res(haptics, false);
> +		if (ret < 0) {
> +			pr_err("Error disabling auto_res, ret=%d\n", ret);
> +			goto out;
> +		}
> +
> +		ret = qpnp_haptics_module_enable(haptics, true);
> +		if (ret < 0) {
> +			pr_err("Error enabling module, ret=%d\n", ret);
> +			goto out;
> +		}
> +
> +		ret = qpnp_haptics_write_play_control(haptics, HAP_PLAY);
> +		if (ret < 0) {
> +			pr_err("Error enabling play, ret=%d\n", ret);
> +			goto out;
> +		}
> +
> +		ret = qpnp_haptics_set_auto_res(haptics, true);
> +		if (ret < 0) {
> +			pr_err("Error enabling auto_res, ret=%d\n", ret);
> +			goto out;
> +		}
> +	} else {
> +		ret = qpnp_haptics_write_play_control(haptics, HAP_STOP);
> +		if (ret < 0) {
> +			pr_err("Error disabling play, ret=%d\n", ret);
> +			goto out;
> +		}
> +
> +		ret = qpnp_haptics_module_enable(haptics, false);
> +		if (ret < 0) {
> +			pr_err("Error disabling module, ret=%d\n", ret);
> +			goto out;
> +		}
> +	}
> +
> +out:
> +	mutex_unlock(&haptics->play_lock);
> +	return ret;
> +}
> +
> +/*
> + * Threaded function to update the haptics state.
> + */
> +static void qpnp_haptics_work(struct work_struct *work)
> +{
> +	struct qpnp_haptics *haptics = container_of(work, struct qpnp_haptics, work);
> +
> +	int ret;
> +	bool enable;
> +
> +	enable = atomic_read(&haptics->active);
> +	dev_dbg(&haptics->pdev->dev, "%s: state: %d\n", __func__, enable);
> +
> +	ret = qpnp_haptics_set(haptics, enable);
> +	if (ret < 0)
> +		pr_err("Error setting haptics, ret=%d", ret);
> +}
> +
> +/**
> + * qpnp_haptics_close - callback for input device close
> + * @dev: input device pointer
> + *
> + * Turns off the vibrator.
> + */
> +static void qpnp_haptics_close(struct input_dev *dev)
> +{
> +	struct qpnp_haptics *haptics = input_get_drvdata(dev);
> +
> +	cancel_work_sync(&haptics->work);
> +	if (atomic_read(&haptics->active)) {
> +		atomic_set(&haptics->active, 0);
> +		schedule_work(&haptics->work);
> +	}
> +}
> +
> +/**
> + * qpnp_haptics_play_effect - play haptics effects
> + * @dev: input device pointer
> + * @data: data of effect
> + * @effect: effect to play
> + */
> +static int qpnp_haptics_play_effect(struct input_dev *dev, void *data,
> +					struct ff_effect *effect)
> +{
> +	struct qpnp_haptics *haptics = input_get_drvdata(dev);
> +
> +	dev_dbg(&haptics->pdev->dev, "%s: Rumbling with strong: %d and weak: %d", __func__,
> +		effect->u.rumble.strong_magnitude, effect->u.rumble.weak_magnitude);
> +
> +	haptics->magnitude = effect->u.rumble.strong_magnitude >> 8;
> +	if (!haptics->magnitude)
> +		haptics->magnitude = effect->u.rumble.weak_magnitude >> 10;
> +
> +	if (!haptics->magnitude) {
> +		atomic_set(&haptics->active, 0);
> +		goto end;
> +	}
> +
> +	atomic_set(&haptics->active, 1);
> +
> +	haptics->vmax = ((HAP_VMAX_MAX_MV - HAP_VMAX_MIN_MV) * haptics->magnitude) / 100 +
> +					HAP_VMAX_MIN_MV;
> +
> +	dev_dbg(&haptics->pdev->dev, "%s: magnitude: %d, vmax: %d", __func__,
> +		haptics->magnitude, haptics->vmax);
> +
> +	qpnp_haptics_write_vmax(haptics);
> +
> +end:
> +	schedule_work(&haptics->work);
> +
> +	return 0;
> +}
> +
> +static int qpnp_haptics_probe(struct platform_device *pdev)
> +{
> +	struct qpnp_haptics *haptics;
> +	struct device_node *node;
> +	struct input_dev *input_dev;
> +	int ret;
> +	u32 val;
> +	int i;
> +
> +	haptics = devm_kzalloc(&pdev->dev, sizeof(*haptics), GFP_KERNEL);
> +	if (!haptics)
> +		return -ENOMEM;
> +
> +	haptics->regmap = dev_get_regmap(pdev->dev.parent, NULL);
> +	if (!haptics->regmap)
> +		return -ENODEV;
> +
> +	node = pdev->dev.of_node;
> +
> +	haptics->pdev = pdev;
> +
> +	ret = of_property_read_u32(node, "reg", &haptics->base);
> +	if (ret < 0) {
> +		pr_err("Couldn't find reg in node = %s ret = %d\n",

dev_err()

> +			node->full_name, ret);
> +		return ret;
> +	}
> +
> +	if (haptics->base <= 0) {
> +		pr_err("Invalid base address: 0x%x\n", haptics->base);

dev_err() but how could it be <= 0 if the read returned ok?

> +		return -EINVAL;
> +	}
> +
> +	haptics->play_irq = platform_get_irq_byname(pdev, "play");
> +	if (haptics->play_irq < 0) {
> +		dev_err(&pdev->dev, "Unable to get play irq\n");
> +		ret = haptics->play_irq;
> +		goto register_fail;
> +	}
> +
> +	haptics->sc_irq = platform_get_irq_byname(pdev, "short");
> +	if (haptics->sc_irq < 0) {
> +		dev_err(&pdev->dev, "Unable to get sc irq\n");
> +		ret = haptics->sc_irq;
> +		goto register_fail;
> +	}
> +
> +	// We only support LRAs for now
> +	haptics->actuator_type = HAP_TYPE_LRA;
> +	ret = of_property_read_u32(node, "qcom,actuator-type", &val);
> +	if (!ret) {
> +		if (val != HAP_TYPE_LRA) {
> +			dev_err(&pdev->dev, "qcom,actuator-type (%d) isn't supported\n", val);
> +			ret = -EINVAL;
> +			goto register_fail;
> +		}
> +		haptics->actuator_type = val;
> +	}
> +
> +	// Only buffer mode is currently supported
> +	haptics->play_mode = HAP_PLAY_BUFFER;
> +	ret = of_property_read_u32(node, "qcom,play-mode", &val);
> +	if (!ret) {
> +		if (val != HAP_PLAY_BUFFER) {
> +			dev_err(&pdev->dev, "qcom,play-mode (%d) isn't supported\n", val);
> +			ret = -EINVAL;
> +			goto register_fail;
> +		}
> +		haptics->play_mode = val;
> +	}
> +
> +	ret = of_property_read_u32(node,
> +			"qcom,wave-play-rate-us", &val);

Unwrap this line.

> +	if (!ret) {
> +		haptics->play_wave_rate = val;
> +	} else if (ret != -EINVAL) {
> +		pr_err("Unable to read play rate ret=%d\n", ret);
> +		goto register_fail;
> +	}
> +
> +	haptics->wave_shape = HAP_WAVE_SINE;
> +	ret = of_property_read_u32(node, "qcom,wave-shape", &val);
> +	if (!ret) {
> +		if (val != HAP_WAVE_SINE && val != HAP_WAVE_SQUARE) {
> +			dev_err(&pdev->dev, "qcom,wave-shape is invalid: %d\n", val);
> +			ret = -EINVAL;
> +			goto register_fail;
> +		}
> +		haptics->wave_shape = val;
> +	}
> +
> +	ret = of_property_read_u8_array(node, "qcom,brake-pattern", haptics->brake_pat, 4);
> +	// Default if not set
> +	if (ret == -EINVAL) {
> +		haptics->brake_pat[0] = 0x3;
> +		haptics->brake_pat[1] = 0x3;
> +		haptics->brake_pat[2] = 0x2;
> +		haptics->brake_pat[3] = 0x1;

You could rely on the fact that these operations doesn't touch the
buffer if the read is going to fail and do:

	haptics->brake_pat[0] = 0x3;
	...
	ret = of_property_read_u8_array();
	if (ret < 0 && ret != -EINVAL)
		fail;

As that will either have the default values (-EINVAL) or the newly
populated values (0).

> +	} else if (ret) {
> +		dev_err(&pdev->dev, "qcom,brake-pattern is invalid, ret = %d\n", ret);

So a qcom,brake-pattern was defined in DT, but it's format is invalid
for some reason. I think it would be better to help the developer find
this by failing the probe here. Otherwise they will have to rely on
someone detecting that line in the kernel log, or some tester noticing
that the behavior is slightly off.

> +	}
> +
> +	haptics->current_limit = HAP_ILIM_400_MA;
> +
> +	for (i = 0; i < HAP_WAVE_SAMP_LEN; i++)
> +		haptics->wave_samp[i] = HAP_WF_SAMP_MAX;
> +
> +	haptics->play_wave_rate =

Perhaps move this up to the place where you assigned play_wave_rate?

> +		clamp_t(u32, haptics->play_wave_rate,
> +		HAP_WAVE_PLAY_RATE_MIN_US, HAP_WAVE_PLAY_RATE_MAX_US);
> +
> +	ret = qpnp_haptics_init(haptics);
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "Error initialising haptics, ret=%d\n",
> +			ret);
> +		goto register_fail;
> +	}
> +
> +	platform_set_drvdata(pdev, haptics);
> +
> +	input_dev = devm_input_allocate_device(&pdev->dev);
> +	if (!input_dev)
> +		return -ENOMEM;
> +
> +	INIT_WORK(&haptics->work, qpnp_haptics_work);
> +	haptics->haptics_input_dev = input_dev;
> +
> +	input_dev->name = "qpnp_haptics";
> +	input_dev->id.version = 3;
> +	input_dev->close = qpnp_haptics_close;
> +	input_set_drvdata(input_dev, haptics);
> +	// Figure out how to make this FF_PERIODIC
> +	input_set_capability(haptics->haptics_input_dev, EV_FF, FF_RUMBLE);
> +
> +	ret = input_ff_create_memless(input_dev, NULL,
> +					qpnp_haptics_play_effect);
> +	if (ret) {
> +		dev_err(&pdev->dev,
> +			"couldn't register vibrator as FF device\n");
> +		goto register_fail;
> +	}
> +
> +	ret = input_register_device(input_dev);
> +	if (ret) {
> +		dev_err(&pdev->dev, "couldn't register input device\n");
> +		goto register_fail;
> +	}
> +
> +	return 0;
> +
> +register_fail:
> +	cancel_work_sync(&haptics->work);
> +	mutex_destroy(&haptics->play_lock);
> +
> +	return ret;
> +}
> +
> +static int __maybe_unused qpnp_haptics_suspend(struct device *dev)
> +{
> +	struct qpnp_haptics *haptics = dev_get_drvdata(dev);
> +
> +	cancel_work_sync(&haptics->work);
> +	qpnp_haptics_set(haptics, false);
> +
> +	return 0;
> +}
> +
> +static SIMPLE_DEV_PM_OPS(qpnp_haptics_pm_ops, qpnp_haptics_suspend, NULL);
> +
> +static int qpnp_haptics_remove(struct platform_device *pdev)
> +{
> +	struct qpnp_haptics *haptics = dev_get_drvdata(&pdev->dev);
> +

Don't you need a input_unregister_device() here?

> +	cancel_work_sync(&haptics->work);
> +	mutex_destroy(&haptics->play_lock);
> +	dev_set_drvdata(&pdev->dev, NULL);

I don't see a reason for you to clear the drvdata here.

> +
> +	return 0;
> +}
> +
> +static void qpnp_haptics_shutdown(struct platform_device *pdev)
> +{
> +	struct qpnp_haptics *haptics = dev_get_drvdata(&pdev->dev);
> +
> +	cancel_work_sync(&haptics->work);
> +
> +	qpnp_haptics_set(haptics, false);
> +}
> +
> +static const struct of_device_id qpnp_haptics_match_table[] = {
> +	{ .compatible = "qcom,qpnp-haptics" },

"qcom,spmi-haptics"

> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, qpnp_haptics_match_table);
> +
> +static struct platform_driver qpnp_haptics_driver = {
> +	.probe		= qpnp_haptics_probe,
> +	.remove		= qpnp_haptics_remove,
> +	.shutdown	= qpnp_haptics_shutdown,
> +	.driver		= {
> +		.name	= "qpnp-haptics",
> +		.pm	= &qpnp_haptics_pm_ops,
> +		.of_match_table = qpnp_haptics_match_table,
> +	},
> +};
> +module_platform_driver(qpnp_haptics_driver);
> +
> +MODULE_ALIAS("platform:qpnp_haptics");

Nothing is going to trigger autoloading of a platform driver with the
alias "qpnp_haptics". Please skip this.

Regards,
Bjorn

> +MODULE_DESCRIPTION("QPNP haptics driver using ff-memless framework");
> +MODULE_LICENSE("GPL v2");
> +MODULE_AUTHOR("Caleb Connolly <caleb@connolly.tech>");
> --
> 2.31.1
> 
> 

Re: [PATCH 1/4] dt-bindings: input: add Qualcomm QPNP haptics driver

[Bjorn Andersson]

On Sat 12 Jun 15:54 CDT 2021, Caleb Connolly wrote:

> Add bindings for pmi8998 qpnp haptics driver.
> 
> Signed-off-by: Caleb Connolly <caleb@connolly.tech>
> ---
>  .../bindings/input/qcom,qpnp-haptics.yaml     | 123 ++++++++++++++++++
>  include/dt-bindings/input/qcom,qpnp-haptics.h |  31 +++++
>  2 files changed, 154 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/input/qcom,qpnp-haptics.yaml
>  create mode 100644 include/dt-bindings/input/qcom,qpnp-haptics.h
> 
> diff --git a/Documentation/devicetree/bindings/input/qcom,qpnp-haptics.yaml b/Documentation/devicetree/bindings/input/qcom,qpnp-haptics.yaml
> new file mode 100644
> index 000000000000..81dd8d6321f0
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/input/qcom,qpnp-haptics.yaml
> @@ -0,0 +1,123 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +# Copyright 2020 Unisoc Inc.
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/input/qcom,qpnp-haptics.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Qualcomm Technologies Inc PMI8998 QPNP haptics
> +
> +maintainers:
> +  - Caleb Connolly <caleb@connolly.tech>
> +
> +description: |
> +  QPNP (Qualcomm Technologies, Inc. Plug N Play) Haptics is a peripheral on some
> +  QTI PMICs. It supports linear resonant actuators commonly found in mobile devices.

Please avoid the Plug'n Play naming, just go with Qualcomm SPMI Haptics

> +
> +properties:
> +  compatible:
> +    enum:
> +      - qcom,qpnp-haptics

qcom,spmi-haptics

But could we also have some pmic-specific compatibles in the binding?
Even though we might not need them in the driver today.

> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    minItems: 2

I don't think you need minItems, when you list the "items"

> +    items:
> +      - description: short circuit interrupt
> +      - description: play interrupt
> +
> +  interrupt-names:
> +    minItems: 2
> +    items:
> +      - const: short
> +      - const: play
> +
> +  qcom,actuator-type:
> +    description: |
> +      The type of actuator attached to the hardware.
> +      Allowed values are,
> +        0 - HAP_TYPE_LRA
> +        1 - HAP_TYPE_ERM
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    enum: [ 0, 1 ]
> +    default: 0
> +
> +  qcom,wave-shape:
> +    description: |
> +      Selects the wave shape to use.
> +      Allowed values are,
> +        0 - HAP_WAVE_SINE
> +        1 - HAP_WAVE_SQUARE
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    enum: [ 0, 1 ]
> +    default: 0
> +
> +  qcom,play-mode:
> +    description: |
> +      Selects the play mode to use.
> +      Allowed values are,
> +        0 - HAP_PLAY_DIRECT
> +        1 - HAP_PLAY_BUFFER
> +        2 - HAP_PLAY_AUDIO
> +        3 - HAP_PLAY_PWM
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    enum: [ 0, 1, 2, 3 ]
> +    default: 2
> +
> +  qcom,wave-play-rate-us:
> +    description: |
> +      Wave sample durection in microseconds, 1/f where f
> +      is the resonant frequency of the actuator.
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    minimum: 0
> +    maximum: 20475
> +
> +  qcom,brake-pattern:
> +    minItems: 4

It's also maxItems: 4, right?

> +    description: |
> +      The brake pattern are the strengths of the pattern
> +      used to brake the haptics. Allowed values are,
> +        0 - 0V
> +        1 - Vmax/4
> +        2 - Vmax/2
> +        3 - Vmax
> +    $ref: /schemas/types.yaml#/definitions/uint32-array
> +    default: [0x3, 0x3, 0x2, 0x1]
> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +  - qcom,wave-play-rate-us
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/spmi/spmi.h>
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +    #include <dt-bindings/input/qcom,qpnp-haptics.h>

Would be nice with a blank line here.

> +    pmi8998_lsid1: pmic@3 {
> +      compatible = "qcom,pmi8998", "qcom,spmi-pmic";
> +      reg = <0x3 SPMI_USID>;
> +      #address-cells = <1>;
> +      #size-cells = <0>;
> +
> +      qpnp_haptics: qcom,haptics@c000 {

No "qcom," in the node name please.

> +        compatible = "qcom,qpnp-haptics";
> +        reg = <0xc000 0x100>;

With #size-cells = <0> this means that you have 2 regs, but binding says
that you may only have one.

> +
> +        interrupts = <0x3 0xc0 0x0 IRQ_TYPE_EDGE_BOTH>,
> +              <0x3 0xc0 0x1 IRQ_TYPE_EDGE_BOTH>;

I don't think the '<' on those two lines are lining up.

Regards,
Bjorn

> +        interrupt-names = "short", "play";
> +
> +        qcom,actuator-type = <HAP_TYPE_LRA>;
> +        qcom,wave-shape = <HAP_WAVE_SINE>;
> +        qcom,play-mode = <HAP_PLAY_BUFFER>;
> +        qcom,brake-pattern = <0x3 0x3 0x2 0x1>;
> +
> +        qcom,wave-play-rate-us = <4255>;
> +      };
> +    };
> diff --git a/include/dt-bindings/input/qcom,qpnp-haptics.h b/include/dt-bindings/input/qcom,qpnp-haptics.h
> new file mode 100644
> index 000000000000..bfbec041484c
> --- /dev/null
> +++ b/include/dt-bindings/input/qcom,qpnp-haptics.h
> @@ -0,0 +1,31 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * This header provides constants for pmi8998 qpnp haptics options.
> + */
> +
> +#ifndef _DT_BINDINGS_QCOM_PMIC_QPNP_HAPTICS_
> +#define _DT_BINDINGS_QCOM_PMIC_QPNP_HAPTICS_
> +
> +// Actuator types
> +#define HAP_TYPE_LRA		0
> +#define HAP_TYPE_ERM		1
> +
> +// LRA Wave type
> +#define HAP_WAVE_SINE		0
> +#define HAP_WAVE_SQUARE		1
> +
> +// Play modes
> +#define HAP_PLAY_DIRECT		0
> +#define HAP_PLAY_BUFFER		1
> +#define HAP_PLAY_AUDIO		2
> +#define HAP_PLAY_PWM		3
> +
> +#define HAP_PLAY_MAX		HAP_PLAY_PWM
> +
> +#define HAP_AUTO_RES_NONE	0
> +#define HAP_AUTO_RES_ZXD	1
> +#define HAP_AUTO_RES_QWD	2
> +#define HAP_AUTO_RES_MAX_QWD	3
> +#define HAP_AUTO_RES_ZXD_EOP	4
> +
> +#endif /* _DT_BINDINGS_QCOM_PMIC_QPNP_HAPTICS_ */
> --
> 2.31.1
> 
> 

Re: [PATCH 2/4] input: add Qualcomm QPNP haptics driver

[Caleb Connolly]

Hi Bjorn,

Thanks a lot for your feedback.

On 13/06/2021 5:09 am, Bjorn Andersson wrote:
> On Sat 12 Jun 15:54 CDT 2021, Caleb Connolly wrote:
>
>> Add support for the haptics found in pmi8998 and related PMICs.
>> Based on the ff-memless interface. Currently this driver provides
>> a partial implementation of hardware features.
>>
>> This driver only supports LRAs (Linear Resonant Actuators) in the "buffer"
>> mode with a single wave pattern.
>>
>> Signed-off-by: Caleb Connolly <caleb@connolly.tech>
>
> Please use a cover letter when posting multiple patches.
I'm not sure why the cover letter didn't make it to your inbox, here's a
link:
https://lore.kernel.org/linux-arm-msm/20210612205405.1233588-1-caleb@connolly.tech/
>
>> ---
>>  drivers/input/misc/Kconfig        |   11 +
>>  drivers/input/misc/Makefile       |    1 +
>>  drivers/input/misc/qpnp-haptics.c | 1022 +++++++++++++++++++++++++++++
>>  3 files changed, 1034 insertions(+)
>>  create mode 100644 drivers/input/misc/qpnp-haptics.c
>>
>> diff --git a/drivers/input/misc/Kconfig b/drivers/input/misc/Kconfig
>> index 498cde376981..b5ba03e6cf58 100644
>> --- a/drivers/input/misc/Kconfig
>> +++ b/drivers/input/misc/Kconfig
>> @@ -186,6 +186,17 @@ config INPUT_PMIC8XXX_PWRKEY
>>  	  To compile this driver as a module, choose M here: the
>>  	  module will be called pmic8xxx-pwrkey.
>>
>> +config INPUT_QPNP_HAPTICS
>
> No-one knows what "QPNP" really is, so please name this
> "INPUT_QCOM_SPMI_HAPTICS"
>
>> +	tristate "Qualcomm QPNP HAPTICS"
>> +	depends on ARCH_QCOM
>
> "depends on SPMI" makes sense here.
>
>> +	select INPUT_FF_MEMLESS
>> +	help
>> +	  This option enables support for the haptics found in pmi8998 and
>> +	  related PMICs. Based on the ff-memless interface.
>> +
>> +	  To compile this driver as module, choose M here: the
>> +	  module will be called qpnp-haptics.
>> +
>>  config INPUT_SPARCSPKR
>>  	tristate "SPARC Speaker support"
>>  	depends on PCI && SPARC64
>> diff --git a/drivers/input/misc/Makefile b/drivers/input/misc/Makefile
>> index f593beed7e05..c43290163db0 100644
>> --- a/drivers/input/misc/Makefile
>> +++ b/drivers/input/misc/Makefile
>> @@ -65,6 +65,7 @@ obj-$(CONFIG_INPUT_PMIC8XXX_PWRKEY)	+= pmic8xxx-pwrkey.o
>>  obj-$(CONFIG_INPUT_POWERMATE)		+= powermate.o
>>  obj-$(CONFIG_INPUT_PWM_BEEPER)		+= pwm-beeper.o
>>  obj-$(CONFIG_INPUT_PWM_VIBRA)		+= pwm-vibra.o
>> +obj-$(CONFIG_INPUT_QPNP_HAPTICS)	+= qpnp-haptics.o
>>  obj-$(CONFIG_INPUT_RAVE_SP_PWRBUTTON)	+= rave-sp-pwrbutton.o
>>  obj-$(CONFIG_INPUT_RB532_BUTTON)	+= rb532_button.o
>>  obj-$(CONFIG_INPUT_REGULATOR_HAPTIC)	+= regulator-haptic.o
>> diff --git a/drivers/input/misc/qpnp-haptics.c b/drivers/input/misc/qpnp-haptics.c
>
> Again, qcom-spmi-haptics.c
>
>> new file mode 100644
>> index 000000000000..daa7a18ffc7d
>> --- /dev/null
>> +++ b/drivers/input/misc/qpnp-haptics.c
>> @@ -0,0 +1,1022 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Copyright (c) 2021, Caleb Connolly <caleb@connolly.tech>
>> + * Qualcomm Plug and Play haptics driver for pmi8998 and related PMICs.
>> + * Based on ./pm8xxx-vibrator.c
>
> Enough "based on" to warrant carrying some copyrights?
Hmm, I only stole the input device parts, so perhaps not.
>
>> + */
>> +
>> +#include <dt-bindings/input/qcom,qpnp-haptics.h>
>> +
>> +#include <linux/atomic.h>
>> +#include <linux/bits.h>
>> +#include <linux/errno.h>
>> +#include <linux/input.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/kernel.h>
>> +#include <linux/log2.h>
>> +#include <linux/minmax.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/of_device.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/regmap.h>
>> +#include <linux/slab.h>
>> +#include <linux/time.h>
>> +#include <linux/types.h>
>> +
>> +/*
>> + * Register and bit definitions
>> + */
>
> This comment doesn't add much value.
>
>> +
>> +#define HAP_STATUS_1_REG(chip)		(chip->base + 0x0A)
>
> This style is somewhat unusual, please define the offset here and do the
> math in qpnp_haptics_read() et al.
>
>> +#define HAP_BUSY_BIT			BIT(1)
>> +#define SC_FLAG_BIT			BIT(3)
>> +#define AUTO_RES_ERROR_BIT		BIT(4)
>> +
>> +#define HAP_LRA_AUTO_RES_LO_REG(chip)	(chip->base + 0x0B)
>> +#define HAP_LRA_AUTO_RES_HI_REG(chip)	(chip->base + 0x0C)
>> +
>> +#define HAP_EN_CTL_REG(chip)		(chip->base + 0x46)
>> +#define HAP_EN_BIT			BIT(7)
>> +
>> +#define HAP_EN_CTL2_REG(chip)		(chip->base + 0x48)
>> +#define BRAKE_EN_BIT			BIT(0)
>> +
>> +#define HAP_AUTO_RES_CTRL_REG(chip)	(chip->base + 0x4B)
>> +#define AUTO_RES_EN_BIT			BIT(7)
>> +#define AUTO_RES_ERR_RECOVERY_BIT	BIT(3)
>> +#define AUTO_RES_EN_FLAG_BIT	BIT(0)
>> +
>> +#define HAP_CFG1_REG(chip)		(chip->base + 0x4C)
>> +#define HAP_ACT_TYPE_MASK		BIT(0)
>> +
>> +#define HAP_CFG2_REG(chip)		(chip->base + 0x4D)
>> +#define HAP_LRA_RES_TYPE_MASK		BIT(0)
>> +
>> +#define HAP_SEL_REG(chip)		(chip->base + 0x4E)
>> +#define HAP_WF_SOURCE_MASK		GENMASK(5, 4)
>> +#define HAP_WF_SOURCE_SHIFT		4
>> +
>> +#define HAP_LRA_AUTO_RES_REG(chip)	(chip->base + 0x4F)
>> +#define LRA_AUTO_RES_MODE_MASK		GENMASK(6, 4)
>> +#define LRA_AUTO_RES_MODE_SHIFT		4
>> +#define LRA_HIGH_Z_MASK			GENMASK(3, 2)
>> +#define LRA_HIGH_Z_SHIFT		2
>> +#define LRA_RES_CAL_MASK		GENMASK(1, 0)
>> +#define HAP_RES_CAL_PERIOD_MIN		4
>> +#define HAP_RES_CAL_PERIOD_MAX		32
>> +
>> +#define HAP_VMAX_CFG_REG(chip)		(chip->base + 0x51)
>> +#define HAP_VMAX_OVD_BIT		BIT(6)
>> +#define HAP_VMAX_MASK			GENMASK(5, 1)
>> +#define HAP_VMAX_SHIFT			1
>> +
>> +#define HAP_ILIM_CFG_REG(chip)		(chip->base + 0x52)
>> +#define HAP_ILIM_SEL_MASK		BIT(0)
>> +#define HAP_ILIM_400_MA			0
>> +#define HAP_ILIM_800_MA			1
>> +
>> +#define HAP_SC_DEB_REG(chip)		(chip->base + 0x53)
>> +#define HAP_SC_DEB_MASK			GENMASK(2, 0)
>> +#define HAP_SC_DEB_CYCLES_MIN		0
>> +#define HAP_DEF_SC_DEB_CYCLES		8
>> +#define HAP_SC_DEB_CYCLES_MAX		32
>> +
>> +#define HAP_RATE_CFG1_REG(chip)		(chip->base + 0x54)
>> +#define HAP_RATE_CFG1_MASK		GENMASK(7, 0)
>> +#define HAP_RATE_CFG2_SHIFT		8 // As CFG2 is the most significant byte
>> +
>> +#define HAP_RATE_CFG2_REG(chip)		(chip->base + 0x55)
>> +#define HAP_RATE_CFG2_MASK		GENMASK(3, 0)
>> +
>> +#define HAP_SC_CLR_REG(chip)		(chip->base + 0x59)
>> +#define SC_CLR_BIT			BIT(0)
>> +
>> +#define HAP_BRAKE_REG(chip)		(chip->base + 0x5C)
>> +#define HAP_BRAKE_PAT_MASK		0x3
>> +
>> +#define HAP_WF_REPEAT_REG(chip)		(chip->base + 0x5E)
>> +#define WF_REPEAT_MASK			GENMASK(6, 4)
>> +#define WF_REPEAT_SHIFT			4
>> +#define WF_REPEAT_MIN			1
>> +#define WF_REPEAT_MAX			128
>> +#define WF_S_REPEAT_MASK		GENMASK(1, 0)
>> +#define WF_S_REPEAT_MIN			1
>> +#define WF_S_REPEAT_MAX			8
>> +
>> +#define HAP_WF_S1_REG(chip)		(chip->base + 0x60)
>> +#define HAP_WF_SIGN_BIT			BIT(7)
>> +#define HAP_WF_OVD_BIT			BIT(6)
>> +#define HAP_WF_SAMP_MAX			GENMASK(5, 1)
>> +#define HAP_WF_SAMPLE_LEN		8
>> +
>> +#define HAP_PLAY_REG(chip)		(chip->base + 0x70)
>> +#define HAP_PLAY_BIT			BIT(7)
>> +#define HAP_PAUSE_BIT			BIT(0)
>> +
>> +#define HAP_SEC_ACCESS_REG(chip)	(chip->base + 0xD0)
>> +#define HAP_SEC_ACCESS_UNLOCK		0xA5
>> +
>> +#define HAP_TEST2_REG(chip)		(chip->base + 0xE3)
>> +
>> +// Values
>> +#define HAP_VMAX_MIN_MV			116
>> +#define HAP_VMAX_MAX_MV			3596
>> +#define HAP_VMAX_MAX_MV_STRONG		3596
>> +
>> +#define HAP_WAVE_PLAY_RATE_MIN_US	0
>> +#define HAP_WAVE_PLAY_RATE_MAX_US	20475
>> +#define HAP_WAVE_PLAY_TIME_MAX_MS	15000
>> +
>> +#define AUTO_RES_ERR_POLL_TIME_NS	(20 * NSEC_PER_MSEC)
>> +#define HAPTICS_BACK_EMF_DELAY_US	20000
>> +
>> +#define HAP_BRAKE_PAT_LEN		4
>> +#define HAP_WAVE_SAMP_LEN		8
>> +#define NUM_WF_SET			4
>> +#define HAP_WAVE_SAMP_SET_LEN		(HAP_WAVE_SAMP_LEN * NUM_WF_SET)
>> +#define HAP_RATE_CFG_STEP_US		5
>> +
>> +#define SC_MAX_COUNT			5
>> +#define SC_COUNT_RST_DELAY_US		1000000
>> +
>> +enum hap_play_control {
>> +	HAP_STOP,
>> +	HAP_PAUSE,
>> +	HAP_PLAY,
>> +};
>> +
>> +/**
>> + * struct qpnp_haptics - struct for qpnp haptics data.
>> + *
>> + * @pdev: The platform device responsible for the haptics.
>> + * @regmap: Register map for the hardware block.
>> + * @input_dev: The input device used to receive events.
>> + * @work: Work struct to play effects.
>> + * @base: Base address of the regmap.
>> + * @active: Atomic value used to track if haptics are currently playing.
>> + * @play_irq: Fired to load the next wave pattern.
>> + * @sc_irq: Short circuit irq.
>> + * @last_sc_time: Time since the short circuit IRQ last fired.
>> + * @sc_count: Number of times the short circuit IRQ has fired in this interval.
>> + * @actuator_type: The type of actuator in use.
>> + * @wave_shape: The shape of the waves to use (sine or square).
>> + * @play_mode: The play mode to use (direct, buffer, pwm, audio).
>> + * @magnitude: The strength we should be playing at.
>> + * @vmax: Max voltage to use when playing.
>> + * @current_limit: The current limit for this hardware (400mA or 800mA).
>> + * @play_wave_rate: The wave rate to use for this hardware.
>> + * @wave_samp: The array of wave samples to write for buffer mode.
>> + * @brake_pat: The pattern to apply when braking.
>> + * @play_lock: Lock to be held when updating the hardware state.
>> + */
>> +struct qpnp_haptics {
>> +	struct platform_device *pdev;
>
> You only ever use this to get to &pdev->dev, so why not store that
> instead?
>
>> +	struct regmap *regmap;
>> +	struct input_dev *haptics_input_dev;
>> +	struct work_struct work;
>> +	u32 base;
>> +
>> +	atomic_t active;
>> +
>> +	int play_irq;
>> +	int sc_irq;
>> +	ktime_t last_sc_time;
>> +	u8 sc_count;
>> +
>> +	u8 actuator_type;
>> +	u8 wave_shape;
>> +	u8 play_mode;
>> +	int magnitude;
>> +	u32 vmax;
>> +	u32 current_limit;
>> +	u32 play_wave_rate;
>> +
>> +	u32 wave_samp[HAP_WAVE_SAMP_SET_LEN];
>> +	u8 brake_pat[HAP_BRAKE_PAT_LEN];
>> +
>> +	struct mutex play_lock;
>> +};
>> +
>> +static inline bool is_secure_addr(u16 addr)
>> +{
>> +	return (addr & 0xFF) > 0xD0;
>> +}
>> +
>> +static int qpnp_haptics_read(struct qpnp_haptics *haptics,
>> +	u16 addr, u8 *val, int len)
>> +{
>> +	int ret;
>> +
>> +	ret = regmap_bulk_read(haptics->regmap, addr, val, len);
>> +	if (ret < 0)
>> +		pr_err("Error reading address: 0x%x, ret %d\n", addr, ret);
>
> dev_err()
>
>> +
>> +	return ret;
>> +}
>> +
>> +static int qpnp_haptics_write(struct qpnp_haptics *haptics,
>> +	u16 addr, u8 *val, int len)
>> +{
>> +	int ret, i;
>> +
>> +	if (is_secure_addr(addr)) {
>> +		for (i = 0; i < len; i++) {
>> +			pr_info("%s: unlocking for addr: 0x%x, val: 0x%x", __func__,
>> +				addr, val[i]);
>
> This is more of a "debug print", so dev_dbg() would be better.
>
>> +			ret = regmap_write(haptics->regmap,
>> +				HAP_SEC_ACCESS_REG(haptics), HAP_SEC_ACCESS_UNLOCK);
>> +			if (ret < 0) {
>> +				pr_err("Error writing unlock code, ret %d\n",
>> +					ret);
>
> dev_err()
>
>> +				return ret;
>> +			}
>> +
>> +			ret = regmap_write(haptics->regmap, addr + i, val[i]);
>> +			if (ret < 0) {
>> +				pr_err("Error writing address 0x%x, ret %d\n",
>> +					addr + i, ret);
>> +				return ret;
>> +			}
>> +		}
>> +	} else {
>> +		if (len > 1)
>> +			ret = regmap_bulk_write(haptics->regmap, addr, val, len);
>> +		else
>> +			ret = regmap_write(haptics->regmap, addr, *val);
>> +	}
>> +
>> +	if (ret < 0)
>> +		pr_err("%s: Error writing address: 0x%x, ret %d\n", __func__, addr, ret);
>> +
>> +	return ret;
>> +}
>> +
>> +static int qpnp_haptics_write_masked(struct qpnp_haptics *haptics,
>> +	u16 addr, u8 mask, u8 val)
>> +{
>> +	int ret;
>> +
>> +	if (is_secure_addr(addr)) {
>> +		ret = regmap_write(haptics->regmap,
>> +			HAP_SEC_ACCESS_REG(haptics), HAP_SEC_ACCESS_UNLOCK);
>> +		if (ret < 0) {
>> +			pr_err("Error writing unlock code - ret %d\n", ret);
>> +			return ret;
>> +		}
>> +	}
>> +
>> +	ret = regmap_update_bits(haptics->regmap, addr, mask, val);
>> +	if (ret < 0)
>> +		pr_err("Error writing address: 0x%x - ret %d\n", addr, ret);
>> +
>> +	return ret;
>> +}
>> +
>> +static bool is_haptics_idle(struct qpnp_haptics *haptics)
>> +{
>> +	int ret;
>> +	u8 val;
>> +
>> +	if (haptics->play_mode == HAP_PLAY_DIRECT ||
>> +			haptics->play_mode == HAP_PLAY_PWM)
>> +		return true;
>> +
>> +	ret = qpnp_haptics_read(haptics, HAP_STATUS_1_REG(haptics), &val, 1);
>> +	if (ret < 0 || (val & HAP_BUSY_BIT))
>> +		return false;
>> +
>> +	return true;
>> +}
>> +
>> +static int qpnp_haptics_module_enable(struct qpnp_haptics *haptics, bool enable)
>> +{
>> +	u8 val;
>> +	int rc;
>> +
>> +	dev_dbg(&haptics->pdev->dev, "Setting module enable: %d", enable);
>> +
>> +	val = enable ? HAP_EN_BIT : 0;
>> +	rc = qpnp_haptics_write(haptics, HAP_EN_CTL_REG(haptics), &val, 1);
>
> rc will either be < 0 or 0, so you can just do:
> 	return qpnp_haptics_write();
>
>> +	if (rc < 0)
>> +		return rc;
>> +
>> +	return 0;
>> +}
>> +
>> +static int qpnp_haptics_write_vmax(struct qpnp_haptics *haptics)
>> +{
>> +	u8 val = 0;
>> +	int ret;
>> +	u32 vmax_mv = haptics->vmax;
>> +
>> +	vmax_mv = clamp_t(u32, vmax_mv, HAP_VMAX_MIN_MV, HAP_VMAX_MAX_MV);
>> +
>> +	dev_dbg(&haptics->pdev->dev, "Setting vmax to: %d", vmax_mv);
>> +
>> +	val = DIV_ROUND_CLOSEST(vmax_mv, HAP_VMAX_MIN_MV);
>> +	val <<= HAP_VMAX_SHIFT;
>> +	// overdrive is only supported on pm660
>
> Good to know...so what should I do with that information?
>
>> +	val &= ~HAP_VMAX_OVD_BIT;
>
> The only way I can see this bit being set is somehow vmax_mv turnes out
> to be more than 5 bits, but then you have a problem.
>
> Consider using FIELD_PREP to stuff the result in the bits and skip this.
>
>> +
>> +	ret = qpnp_haptics_write_masked(haptics, HAP_VMAX_CFG_REG(haptics),
>> +			HAP_VMAX_MASK | HAP_VMAX_OVD_BIT, val);
>> +	return ret;
>> +}
>> +
>> +static int qpnp_haptics_write_current_limit(struct qpnp_haptics *haptics)
>> +{
>> +	int ret;
>> +
>> +	haptics->current_limit = clamp_t(u32, haptics->current_limit,
>> +					 HAP_ILIM_400_MA, HAP_ILIM_800_MA);
>> +
>> +	dev_dbg(&haptics->pdev->dev, "Setting current_limit to: 0x%x", haptics->current_limit);
>> +
>> +	ret = qpnp_haptics_write_masked(haptics, HAP_ILIM_CFG_REG(haptics),
>> +			HAP_ILIM_SEL_MASK, haptics->current_limit);
>> +	return ret;
>> +}
>> +
>> +static int qpnp_haptics_write_play_mode(struct qpnp_haptics *haptics)
>> +{
>> +	u8 val = 0;
>> +	int ret;
>> +
>> +	if (!is_haptics_idle(haptics))
>> +		return -EBUSY;
>> +
>> +	dev_dbg(&haptics->pdev->dev, "Setting play_mode to: 0x%x", haptics->play_mode);
>> +
>> +	val = haptics->play_mode << HAP_WF_SOURCE_SHIFT;
>
> How about:
> 	val = FIELD_PREP(HAP_WF_SOURCE_MASK, haptics->play_mode);
>
>> +	ret = qpnp_haptics_write_masked(haptics, HAP_SEL_REG(haptics),
>> +			HAP_WF_SOURCE_MASK, val);
>
> return qpnp_haptics_write_masked()
>
>> +
>> +	return ret;
>> +}
>> +
>> +static int qpnp_haptics_write_play_rate(struct qpnp_haptics *haptics, u16 play_rate)
>> +{
>> +	int rc;
>> +	u8 val[2];
>> +
>> +	dev_dbg(&haptics->pdev->dev, "Setting play_rate to: %d", play_rate);
>> +
>> +	val[0] = play_rate & HAP_RATE_CFG1_MASK;
>> +	val[1] = (play_rate >> HAP_RATE_CFG2_SHIFT) & HAP_RATE_CFG2_MASK;
>
> Make val a u16 and FIELD_PREP() the bits in there, will be easier to
> reason about.
>
>> +	rc = qpnp_haptics_write(haptics, HAP_RATE_CFG1_REG(haptics), val, 2);
>> +	if (rc < 0)
>> +		return rc;
>> +
>> +	return 0;
>> +}
>> +
>> +/*
>> + * qpnp_haptics_set_auto_res() - Auto resonance
>> + * allows the haptics to automatically adjust the
>> + * speed of the oscillation in order to maintain
>> + * the resonant frequency.
>> + */
>> +static int qpnp_haptics_set_auto_res(struct qpnp_haptics *haptics, bool enable)
>> +{
>> +	int rc = 0;
>> +	u8 val;
>> +
>> +	// LRAs are the only type to support auto res
>> +	if (haptics->actuator_type != HAP_TYPE_LRA)
>> +		return 0;
>> +
>> +	val = enable ? AUTO_RES_EN_BIT : 0;
>> +
>> +	rc = qpnp_haptics_write_masked(haptics, HAP_TEST2_REG(haptics),
>> +			AUTO_RES_EN_BIT, val);
>
> return qpnp_haptics_write_masked();
>
>> +	if (rc < 0)
>> +		return rc;
>> +
>> +	dev_dbg(&haptics->pdev->dev, "Auto resonance enabled: %d", enable);
>> +	return rc;
>> +}
>> +
>> +static int qpnp_haptics_write_brake(struct qpnp_haptics *haptics)
>> +{
>> +	int ret, i;
>> +	u32 temp;
>> +	u8 val;
>> +
>> +	dev_dbg(&haptics->pdev->dev, "Configuring brake pattern");
>> +
>> +	ret = qpnp_haptics_write_masked(haptics, HAP_EN_CTL2_REG(haptics),
>> +			BRAKE_EN_BIT, 1);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	for (i = HAP_BRAKE_PAT_LEN - 1, val = 0; i >= 0; i--) {
>> +		u8 p = haptics->brake_pat[i] & HAP_BRAKE_PAT_MASK;
>> +
>> +		temp = i << 1;
>> +		val |= p << temp;
>
> 		val |= p << (i * 2);
>
> Would make this trivial to understand. The temp variable and the extra
> bitshifting is confusing, and the compiler will help you with the
> optimization.
>
>> +	}
>> +
>> +	ret = qpnp_haptics_write(haptics, HAP_BRAKE_REG(haptics), &val, 1);
>
> return qpnp_haptics_write();
>
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	return 0;
>> +}
>> +
>> +static int qpnp_haptics_write_buffer_config(struct qpnp_haptics *haptics)
>> +{
>> +	u8 buf[HAP_WAVE_SAMP_LEN];
>> +	int rc, i;
>> +
>> +	dev_dbg(&haptics->pdev->dev, "Writing buffer config");
>> +
>> +	for (i = 0; i < HAP_WAVE_SAMP_LEN; i++)
>> +		buf[i] = haptics->wave_samp[i];
>> +
>> +	rc = qpnp_haptics_write(haptics, HAP_WF_S1_REG(haptics), buf,
>> +			HAP_WAVE_SAMP_LEN);
>
> return qpnp_haptics_write();
>
>> +
>> +	return rc;
>> +}
>> +
>> +/**
>> + * qpnp_haptics_write_wave_repeat() - write wave repeat values.
>
> This kernel-doc is incomplete. Either define it fully or drop one of the
> '*'
>
>> + */
>> +static int qpnp_haptics_write_wave_repeat(struct qpnp_haptics *haptics)
>> +{
>> +	int ret;
>> +	u8 val = 0, mask = 0;
>
> No need to zero-initialize these.
>
>> +
>> +	// The number of times to repeat each wave
>
> /* Comments */
>
>> +	mask = WF_REPEAT_MASK;
>> +	val = ilog2(1) << WF_REPEAT_SHIFT; // Currently hard coded to default of 1
>
> So how about val = 0; ?
>
> If you want to leave some of this code around as a todo, perhaps write
> it as:
>
> 	mask = WF_REPEAT_MASK | WF_S_REPEAT_MASK;
> 	val = FIELD_PREP(WF_REPEAT_MASK, 0) |
> 	      FIELD_PREP(WF_S_REPEAT_MASK, 0);
>
> 	return qpnp_haptics_write_masked();
>
>> +
>> +	// the number of times to repeat each wave sample (group of waves)
>> +	mask |= WF_S_REPEAT_MASK;
>> +	val |= ilog2(1);
>> +
>> +	ret = qpnp_haptics_write_masked(haptics, HAP_WF_REPEAT_REG(haptics),
>> +			mask, val);
>> +	return ret;
>> +}
>> +
>> +static int qpnp_haptics_write_play_control(struct qpnp_haptics *haptics,
>> +						enum hap_play_control ctrl)
>> +{
>> +	u8 val;
>> +	int rc;
>> +
>> +	switch (ctrl) {
>> +	case HAP_STOP:
>> +		val = 0;
>> +		break;
>> +	case HAP_PAUSE:
>> +		val = HAP_PAUSE_BIT;
>> +		break;
>> +	case HAP_PLAY:
>> +		val = HAP_PLAY_BIT;
>> +		break;
>> +	default:
>> +		return 0;
>> +	}
>> +
>> +	rc = qpnp_haptics_write(haptics, HAP_PLAY_REG(haptics), &val, 1);
>> +	if (rc < 0) {
>> +		pr_err("Error writing to PLAY_REG, rc=%d\n", rc);
>
> You already printed in all possible code paths through
> qpnp_haptics_write()
>
>
> Move the dev_dbg below up above the write and you can simply:
>
> 	return qpnp_haptics_write();
>
>> +		return rc;
>> +	}
>> +
>> +	dev_dbg(&haptics->pdev->dev, "haptics play ctrl: %d\n", ctrl);
>> +	return rc;
>> +}
>> +
>> +/*
>> + * This IRQ is fired to tell us to load the next wave sample set.
>> + * As we only currently support a single sample set, it's unused.
>> + */
>> +static irqreturn_t qpnp_haptics_play_irq_handler(int irq, void *data)
>> +{
>> +	struct qpnp_haptics *haptics = data;
>> +
>> +	dev_dbg(&haptics->pdev->dev, "play_irq triggered");
>> +
>> +	return IRQ_HANDLED;
>> +}
>> +
>> +/*
>> + * Fires every ~50ms whilst the haptics are active.
>> + * If the SC_FLAG_BIT is set then that means there isn't a short circuit
>> + * and we just need to clear the IRQ to indicate that the device should
>> + * keep vibrating.
>> + *
>> + * Otherwise, it means a short circuit situation has occurred.
>> + */
>> +static irqreturn_t qpnp_haptics_sc_irq_handler(int irq, void *data)
>> +{
>> +	struct qpnp_haptics *haptics = data;
>> +	int ret;
>> +	u8 val;
>> +	s64 sc_delta_time_us;
>> +	ktime_t temp;
>> +
>> +	ret = qpnp_haptics_read(haptics, HAP_STATUS_1_REG(haptics), &val, 1);
>> +	if (ret < 0)
>> +		return IRQ_HANDLED;
>> +
>> +	if (!(val & SC_FLAG_BIT)) {
>> +		haptics->sc_count = 0;
>> +		return IRQ_HANDLED;
>> +	}
>> +
>> +	temp = ktime_get();
>> +	sc_delta_time_us = ktime_us_delta(temp, haptics->last_sc_time);
>> +	haptics->last_sc_time = temp;
>> +
>> +	if (sc_delta_time_us > SC_COUNT_RST_DELAY_US)
>> +		haptics->sc_count = 0;
>> +	else
>> +		haptics->sc_count++;
>> +
>> +	// Clear the interrupt flag
>> +	val = SC_CLR_BIT;
>> +	ret = qpnp_haptics_write(haptics, HAP_SC_CLR_REG(haptics), &val, 1);
>> +	if (ret < 0) {
>> +		pr_err("Error writing to SC_CLR_REG, rc=%d\n", ret);
>
> You already printed in qpnp_haptics_write()
>
>> +		return IRQ_HANDLED;
>> +	}
>> +
>> +	if (haptics->sc_count > SC_MAX_COUNT) {
>> +		pr_crit("Short circuit persists, disabling haptics\n");
>
> dev_crit()?
>
>> +		ret = qpnp_haptics_module_enable(haptics, false);
>> +		if (ret < 0) {
>> +			pr_err("Error disabling module, rc=%d\n", ret);
>
> dev_err(), and qpnp_haptics_module_enable() already printed an error.
>
> That said, these error messages are more useful, so perhaps it's worth
> deciding to move it out to the callers?
>
>> +			return IRQ_HANDLED;
>
> You can just fall down do the return below...
>
>> +		}
>> +	}
>> +
>> +	return IRQ_HANDLED;
>> +}
>> +
>> +
>> +/**
>> + * qpnp_haptics_init() - Initialise haptics hardware for use
>> + * @haptics: haptics device
>
> Missing "Returns:"
>
>> + */
>> +static int qpnp_haptics_init(struct qpnp_haptics *haptics)
>> +{
>> +	int ret;
>> +	u8 val, mask;
>> +	u16 auto_res_mode;
>> +	u16 play_rate = 0;
>
> First use is an assignment, no need to zero-initialize.
>
>> +
>> +	ret = qpnp_haptics_write_masked(haptics, HAP_CFG1_REG(haptics),
>> +		HAP_ACT_TYPE_MASK, haptics->actuator_type);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	// Configure auto resonance
>
> /*
>  * Multi-line comment.
>  */
>
>> +	// see qpnp_haptics_lra_auto_res_config downstream
>> +	// This is greatly simplified.
>> +	auto_res_mode = HAP_AUTO_RES_ZXD_EOP << LRA_AUTO_RES_MODE_SHIFT;
>> +
>> +	//Default LRA calibration period
>> +	val = ilog2(32 / HAP_RES_CAL_PERIOD_MIN);
>> +
>> +	val |= (auto_res_mode << LRA_AUTO_RES_MODE_SHIFT);
>
> You shift HAP_AUTO_RES_ZXD_EOP twice here.
>
>> +	// The 1 here is for OPT2 "High Z configuration", there are 3
>> +	// options and no documentation indicating the difference
>> +	val |= (1 << LRA_HIGH_Z_SHIFT);
>
> Consider writing this as:
>
> 	val = FIELD_PREP(LRA_RES_CAL_MASK, ilog2(32 / HAP_RES_CAL_PERIOD_MIN) |
> 	      FIELD_PREP(LRA_AUTO_RES_MODE_MASK, HAP_AUTO_RES_ZXD_EOP) |
> 	      FIELD_PREP(LRA_HIGH_Z_MASK, 1);
>
>> +	mask = LRA_AUTO_RES_MODE_MASK | LRA_HIGH_Z_MASK | LRA_RES_CAL_MASK;
>> +
>> +	ret = qpnp_haptics_write_masked(haptics, HAP_LRA_AUTO_RES_REG(haptics),
>> +			mask, val);
>> +
>> +	dev_dbg(&haptics->pdev->dev, "%s: auto_res_mode: %d\n", __func__,
>> +		auto_res_mode);
>> +
>> +	/* Configure the PLAY MODE register */
>> +	ret = qpnp_haptics_write_play_mode(haptics);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	ret = qpnp_haptics_write_vmax(haptics);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	/* Configure the ILIM register */
>> +	ret = qpnp_haptics_write_current_limit(haptics);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	// Configure the debounce for short-circuit detection.
>> +	val = HAP_SC_DEB_CYCLES_MAX;
>
> This val is unused.
>
>> +	ret = qpnp_haptics_write_masked(haptics, HAP_SC_DEB_REG(haptics),
>> +			HAP_SC_DEB_MASK, HAP_SC_DEB_CYCLES_MAX);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	// write the wave shape
>> +	ret = qpnp_haptics_write_masked(haptics, HAP_CFG2_REG(haptics),
>> +			HAP_LRA_RES_TYPE_MASK, haptics->wave_shape);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	play_rate = haptics->play_wave_rate / HAP_RATE_CFG_STEP_US;
>> +
>> +	/*
>> +	 * Configure RATE_CFG1 and RATE_CFG2 registers.
>> +	 * Note: For ERM these registers act as play rate and
>> +	 * for LRA these represent resonance period
>> +	 */
>> +	ret = qpnp_haptics_write_play_rate(haptics, play_rate);
>
> You don't check this ret.
>
>> +
>> +	ret = qpnp_haptics_write_brake(haptics);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	if (haptics->play_mode == HAP_PLAY_BUFFER) {
>> +		ret = qpnp_haptics_write_wave_repeat(haptics);
>> +		if (ret < 0)
>> +			return ret;
>> +
>> +		ret = qpnp_haptics_write_buffer_config(haptics);
>> +	}
>> +
>> +	if (haptics->play_irq >= 0) {
>
> Afaict, you won't leave probe successfully with this < 0.
>
>> +		dev_dbg(&haptics->pdev->dev, "%s: Requesting play IRQ, irq: %d", __func__,
>> +			haptics->play_irq);
>> +		ret = devm_request_threaded_irq(&haptics->pdev->dev, haptics->play_irq,
>> +			NULL, qpnp_haptics_play_irq_handler, IRQF_ONESHOT,
>> +			"haptics_play_irq", haptics);
>> +
>> +		if (ret < 0) {
>> +			pr_err("Unable to request play IRQ ret=%d\n", ret);
>> +			return ret;
>> +		}
>> +
>> +		/* use play_irq only for buffer mode */
>> +		if (haptics->play_mode != HAP_PLAY_BUFFER)
>> +			disable_irq(haptics->play_irq);
>> +	}
>> +
>> +	if (haptics->sc_irq >= 0) {
>
> Same here.
>
>> +		dev_dbg(&haptics->pdev->dev, "%s: Requesting play IRQ, irq: %d", __func__,
>> +			haptics->play_irq);
>> +		ret = devm_request_threaded_irq(&haptics->pdev->dev, haptics->sc_irq,
>> +			NULL, qpnp_haptics_sc_irq_handler, IRQF_ONESHOT,
>> +			"haptics_sc_irq", haptics);
>> +
>> +		if (ret < 0) {
>> +			pr_err("Unable to request sc IRQ ret=%d\n", ret);
>> +			return ret;
>> +		}
>> +	}
>> +
>> +	return ret;
>
> ret is 0 when you get here.
>
>> +}
>> +
>> +/**
>> + * qpnp_haptics_set - handler to start/stop vibration
>> + * @haptics: pointer to haptics struct
>> + * @enable: state to set
>
>   * Returns:
>
>> + */
>> +static int qpnp_haptics_set(struct qpnp_haptics *haptics, bool enable)
>
> Consider splitting this into haptics_enable() and haptics_disable(), as
> this really just is two separate functions squashed into one.
>
>> +{
>> +	int ret;
>> +
>> +	mutex_lock(&haptics->play_lock);
>> +
>> +	if (enable) {
>> +		if (haptics->sc_count > SC_MAX_COUNT) {
>> +			pr_err("Can't play while in short circuit");
>> +			ret = -1;
>> +			goto out;
>> +		}
>> +		ret = qpnp_haptics_set_auto_res(haptics, false);
>> +		if (ret < 0) {
>> +			pr_err("Error disabling auto_res, ret=%d\n", ret);
>> +			goto out;
>> +		}
>> +
>> +		ret = qpnp_haptics_module_enable(haptics, true);
>> +		if (ret < 0) {
>> +			pr_err("Error enabling module, ret=%d\n", ret);
>> +			goto out;
>> +		}
>> +
>> +		ret = qpnp_haptics_write_play_control(haptics, HAP_PLAY);
>> +		if (ret < 0) {
>> +			pr_err("Error enabling play, ret=%d\n", ret);
>> +			goto out;
>> +		}
>> +
>> +		ret = qpnp_haptics_set_auto_res(haptics, true);
>> +		if (ret < 0) {
>> +			pr_err("Error enabling auto_res, ret=%d\n", ret);
>> +			goto out;
>> +		}
>> +	} else {
>> +		ret = qpnp_haptics_write_play_control(haptics, HAP_STOP);
>> +		if (ret < 0) {
>> +			pr_err("Error disabling play, ret=%d\n", ret);
>> +			goto out;
>> +		}
>> +
>> +		ret = qpnp_haptics_module_enable(haptics, false);
>> +		if (ret < 0) {
>> +			pr_err("Error disabling module, ret=%d\n", ret);
>> +			goto out;
>> +		}
>> +	}
>> +
>> +out:
>> +	mutex_unlock(&haptics->play_lock);
>> +	return ret;
>> +}
>> +
>> +/*
>> + * Threaded function to update the haptics state.
>> + */
>> +static void qpnp_haptics_work(struct work_struct *work)
>> +{
>> +	struct qpnp_haptics *haptics = container_of(work, struct qpnp_haptics, work);
>> +
>> +	int ret;
>> +	bool enable;
>> +
>> +	enable = atomic_read(&haptics->active);
>> +	dev_dbg(&haptics->pdev->dev, "%s: state: %d\n", __func__, enable);
>> +
>> +	ret = qpnp_haptics_set(haptics, enable);
>> +	if (ret < 0)
>> +		pr_err("Error setting haptics, ret=%d", ret);
>> +}
>> +
>> +/**
>> + * qpnp_haptics_close - callback for input device close
>> + * @dev: input device pointer
>> + *
>> + * Turns off the vibrator.
>> + */
>> +static void qpnp_haptics_close(struct input_dev *dev)
>> +{
>> +	struct qpnp_haptics *haptics = input_get_drvdata(dev);
>> +
>> +	cancel_work_sync(&haptics->work);
>> +	if (atomic_read(&haptics->active)) {
>> +		atomic_set(&haptics->active, 0);
>> +		schedule_work(&haptics->work);
>> +	}
>> +}
>> +
>> +/**
>> + * qpnp_haptics_play_effect - play haptics effects
>> + * @dev: input device pointer
>> + * @data: data of effect
>> + * @effect: effect to play
>> + */
>> +static int qpnp_haptics_play_effect(struct input_dev *dev, void *data,
>> +					struct ff_effect *effect)
>> +{
>> +	struct qpnp_haptics *haptics = input_get_drvdata(dev);
>> +
>> +	dev_dbg(&haptics->pdev->dev, "%s: Rumbling with strong: %d and weak: %d", __func__,
>> +		effect->u.rumble.strong_magnitude, effect->u.rumble.weak_magnitude);
>> +
>> +	haptics->magnitude = effect->u.rumble.strong_magnitude >> 8;
>> +	if (!haptics->magnitude)
>> +		haptics->magnitude = effect->u.rumble.weak_magnitude >> 10;
>> +
>> +	if (!haptics->magnitude) {
>> +		atomic_set(&haptics->active, 0);
>> +		goto end;
>> +	}
>> +
>> +	atomic_set(&haptics->active, 1);
>> +
>> +	haptics->vmax = ((HAP_VMAX_MAX_MV - HAP_VMAX_MIN_MV) * haptics->magnitude) / 100 +
>> +					HAP_VMAX_MIN_MV;
>> +
>> +	dev_dbg(&haptics->pdev->dev, "%s: magnitude: %d, vmax: %d", __func__,
>> +		haptics->magnitude, haptics->vmax);
>> +
>> +	qpnp_haptics_write_vmax(haptics);
>> +
>> +end:
>> +	schedule_work(&haptics->work);
>> +
>> +	return 0;
>> +}
>> +
>> +static int qpnp_haptics_probe(struct platform_device *pdev)
>> +{
>> +	struct qpnp_haptics *haptics;
>> +	struct device_node *node;
>> +	struct input_dev *input_dev;
>> +	int ret;
>> +	u32 val;
>> +	int i;
>> +
>> +	haptics = devm_kzalloc(&pdev->dev, sizeof(*haptics), GFP_KERNEL);
>> +	if (!haptics)
>> +		return -ENOMEM;
>> +
>> +	haptics->regmap = dev_get_regmap(pdev->dev.parent, NULL);
>> +	if (!haptics->regmap)
>> +		return -ENODEV;
>> +
>> +	node = pdev->dev.of_node;
>> +
>> +	haptics->pdev = pdev;
>> +
>> +	ret = of_property_read_u32(node, "reg", &haptics->base);
>> +	if (ret < 0) {
>> +		pr_err("Couldn't find reg in node = %s ret = %d\n",
>
> dev_err()
>
>> +			node->full_name, ret);
>> +		return ret;
>> +	}
>> +
>> +	if (haptics->base <= 0) {
>> +		pr_err("Invalid base address: 0x%x\n", haptics->base);
>
> dev_err() but how could it be <= 0 if the read returned ok?
>
>> +		return -EINVAL;
>> +	}
>> +
>> +	haptics->play_irq = platform_get_irq_byname(pdev, "play");
>> +	if (haptics->play_irq < 0) {
>> +		dev_err(&pdev->dev, "Unable to get play irq\n");
>> +		ret = haptics->play_irq;
>> +		goto register_fail;
>> +	}
>> +
>> +	haptics->sc_irq = platform_get_irq_byname(pdev, "short");
>> +	if (haptics->sc_irq < 0) {
>> +		dev_err(&pdev->dev, "Unable to get sc irq\n");
>> +		ret = haptics->sc_irq;
>> +		goto register_fail;
>> +	}
>> +
>> +	// We only support LRAs for now
>> +	haptics->actuator_type = HAP_TYPE_LRA;
>> +	ret = of_property_read_u32(node, "qcom,actuator-type", &val);
>> +	if (!ret) {
>> +		if (val != HAP_TYPE_LRA) {
>> +			dev_err(&pdev->dev, "qcom,actuator-type (%d) isn't supported\n", val);
>> +			ret = -EINVAL;
>> +			goto register_fail;
>> +		}
>> +		haptics->actuator_type = val;
>> +	}
>> +
>> +	// Only buffer mode is currently supported
>> +	haptics->play_mode = HAP_PLAY_BUFFER;
>> +	ret = of_property_read_u32(node, "qcom,play-mode", &val);
>> +	if (!ret) {
>> +		if (val != HAP_PLAY_BUFFER) {
>> +			dev_err(&pdev->dev, "qcom,play-mode (%d) isn't supported\n", val);
>> +			ret = -EINVAL;
>> +			goto register_fail;
>> +		}
>> +		haptics->play_mode = val;
>> +	}
>> +
>> +	ret = of_property_read_u32(node,
>> +			"qcom,wave-play-rate-us", &val);
>
> Unwrap this line.
>
>> +	if (!ret) {
>> +		haptics->play_wave_rate = val;
>> +	} else if (ret != -EINVAL) {
>> +		pr_err("Unable to read play rate ret=%d\n", ret);
>> +		goto register_fail;
>> +	}
>> +
>> +	haptics->wave_shape = HAP_WAVE_SINE;
>> +	ret = of_property_read_u32(node, "qcom,wave-shape", &val);
>> +	if (!ret) {
>> +		if (val != HAP_WAVE_SINE && val != HAP_WAVE_SQUARE) {
>> +			dev_err(&pdev->dev, "qcom,wave-shape is invalid: %d\n", val);
>> +			ret = -EINVAL;
>> +			goto register_fail;
>> +		}
>> +		haptics->wave_shape = val;
>> +	}
>> +
>> +	ret = of_property_read_u8_array(node, "qcom,brake-pattern", haptics->brake_pat, 4);
>> +	// Default if not set
>> +	if (ret == -EINVAL) {
>> +		haptics->brake_pat[0] = 0x3;
>> +		haptics->brake_pat[1] = 0x3;
>> +		haptics->brake_pat[2] = 0x2;
>> +		haptics->brake_pat[3] = 0x1;
>
> You could rely on the fact that these operations doesn't touch the
> buffer if the read is going to fail and do:
>
> 	haptics->brake_pat[0] = 0x3;
> 	...
> 	ret = of_property_read_u8_array();
> 	if (ret < 0 && ret != -EINVAL)
> 		fail;
>
> As that will either have the default values (-EINVAL) or the newly
> populated values (0).
>
>> +	} else if (ret) {
>> +		dev_err(&pdev->dev, "qcom,brake-pattern is invalid, ret = %d\n", ret);
>
> So a qcom,brake-pattern was defined in DT, but it's format is invalid
> for some reason. I think it would be better to help the developer find
> this by failing the probe here. Otherwise they will have to rely on
> someone detecting that line in the kernel log, or some tester noticing
> that the behavior is slightly off.
>
>> +	}
>> +
>> +	haptics->current_limit = HAP_ILIM_400_MA;
>> +
>> +	for (i = 0; i < HAP_WAVE_SAMP_LEN; i++)
>> +		haptics->wave_samp[i] = HAP_WF_SAMP_MAX;
>> +
>> +	haptics->play_wave_rate =
>
> Perhaps move this up to the place where you assigned play_wave_rate?
>
>> +		clamp_t(u32, haptics->play_wave_rate,
>> +		HAP_WAVE_PLAY_RATE_MIN_US, HAP_WAVE_PLAY_RATE_MAX_US);
>> +
>> +	ret = qpnp_haptics_init(haptics);
>> +	if (ret < 0) {
>> +		dev_err(&pdev->dev, "Error initialising haptics, ret=%d\n",
>> +			ret);
>> +		goto register_fail;
>> +	}
>> +
>> +	platform_set_drvdata(pdev, haptics);
>> +
>> +	input_dev = devm_input_allocate_device(&pdev->dev);
>> +	if (!input_dev)
>> +		return -ENOMEM;
>> +
>> +	INIT_WORK(&haptics->work, qpnp_haptics_work);
>> +	haptics->haptics_input_dev = input_dev;
>> +
>> +	input_dev->name = "qpnp_haptics";
>> +	input_dev->id.version = 3;
>> +	input_dev->close = qpnp_haptics_close;
>> +	input_set_drvdata(input_dev, haptics);
>> +	// Figure out how to make this FF_PERIODIC
>> +	input_set_capability(haptics->haptics_input_dev, EV_FF, FF_RUMBLE);
>> +
>> +	ret = input_ff_create_memless(input_dev, NULL,
>> +					qpnp_haptics_play_effect);
>> +	if (ret) {
>> +		dev_err(&pdev->dev,
>> +			"couldn't register vibrator as FF device\n");
>> +		goto register_fail;
>> +	}
>> +
>> +	ret = input_register_device(input_dev);
>> +	if (ret) {
>> +		dev_err(&pdev->dev, "couldn't register input device\n");
>> +		goto register_fail;
>> +	}
>> +
>> +	return 0;
>> +
>> +register_fail:
>> +	cancel_work_sync(&haptics->work);
>> +	mutex_destroy(&haptics->play_lock);
>> +
>> +	return ret;
>> +}
>> +
>> +static int __maybe_unused qpnp_haptics_suspend(struct device *dev)
>> +{
>> +	struct qpnp_haptics *haptics = dev_get_drvdata(dev);
>> +
>> +	cancel_work_sync(&haptics->work);
>> +	qpnp_haptics_set(haptics, false);
>> +
>> +	return 0;
>> +}
>> +
>> +static SIMPLE_DEV_PM_OPS(qpnp_haptics_pm_ops, qpnp_haptics_suspend, NULL);
>> +
>> +static int qpnp_haptics_remove(struct platform_device *pdev)
>> +{
>> +	struct qpnp_haptics *haptics = dev_get_drvdata(&pdev->dev);
>> +
>
> Don't you need a input_unregister_device() here?
>
>> +	cancel_work_sync(&haptics->work);
>> +	mutex_destroy(&haptics->play_lock);
>> +	dev_set_drvdata(&pdev->dev, NULL);
>
> I don't see a reason for you to clear the drvdata here.
>
>> +
>> +	return 0;
>> +}
>> +
>> +static void qpnp_haptics_shutdown(struct platform_device *pdev)
>> +{
>> +	struct qpnp_haptics *haptics = dev_get_drvdata(&pdev->dev);
>> +
>> +	cancel_work_sync(&haptics->work);
>> +
>> +	qpnp_haptics_set(haptics, false);
>> +}
>> +
>> +static const struct of_device_id qpnp_haptics_match_table[] = {
>> +	{ .compatible = "qcom,qpnp-haptics" },
>
> "qcom,spmi-haptics"
>
>> +	{ }
>> +};
>> +MODULE_DEVICE_TABLE(of, qpnp_haptics_match_table);
>> +
>> +static struct platform_driver qpnp_haptics_driver = {
>> +	.probe		= qpnp_haptics_probe,
>> +	.remove		= qpnp_haptics_remove,
>> +	.shutdown	= qpnp_haptics_shutdown,
>> +	.driver		= {
>> +		.name	= "qpnp-haptics",
>> +		.pm	= &qpnp_haptics_pm_ops,
>> +		.of_match_table = qpnp_haptics_match_table,
>> +	},
>> +};
>> +module_platform_driver(qpnp_haptics_driver);
>> +
>> +MODULE_ALIAS("platform:qpnp_haptics");
>
> Nothing is going to trigger autoloading of a platform driver with the
> alias "qpnp_haptics". Please skip this.
>
> Regards,
> Bjorn
>
>> +MODULE_DESCRIPTION("QPNP haptics driver using ff-memless framework");
>> +MODULE_LICENSE("GPL v2");
>> +MODULE_AUTHOR("Caleb Connolly <caleb@connolly.tech>");
>> --
>> 2.31.1
>>
>>

--
Kind Regards,
Caleb

Re: [PATCH 2/4] input: add Qualcomm QPNP haptics driver

[Bjorn Andersson]

On Sun 13 Jun 11:24 CDT 2021, Caleb Connolly wrote:

> Hi Bjorn,
> 
> Thanks a lot for your feedback.
> 
> On 13/06/2021 5:09 am, Bjorn Andersson wrote:
> > On Sat 12 Jun 15:54 CDT 2021, Caleb Connolly wrote:
> >
> >> Add support for the haptics found in pmi8998 and related PMICs.
> >> Based on the ff-memless interface. Currently this driver provides
> >> a partial implementation of hardware features.
> >>
> >> This driver only supports LRAs (Linear Resonant Actuators) in the "buffer"
> >> mode with a single wave pattern.
> >>
> >> Signed-off-by: Caleb Connolly <caleb@connolly.tech>
> >
> > Please use a cover letter when posting multiple patches.
> I'm not sure why the cover letter didn't make it to your inbox, here's a
> link:
> https://lore.kernel.org/linux-arm-msm/20210612205405.1233588-1-caleb@connolly.tech/

It arrived later, for some reason. Sorry for the fuzz.

> >
> >> ---
> >>  drivers/input/misc/Kconfig        |   11 +
> >>  drivers/input/misc/Makefile       |    1 +
> >>  drivers/input/misc/qpnp-haptics.c | 1022 +++++++++++++++++++++++++++++
> >>  3 files changed, 1034 insertions(+)
> >>  create mode 100644 drivers/input/misc/qpnp-haptics.c
> >>
> >> diff --git a/drivers/input/misc/Kconfig b/drivers/input/misc/Kconfig
> >> index 498cde376981..b5ba03e6cf58 100644
> >> --- a/drivers/input/misc/Kconfig
> >> +++ b/drivers/input/misc/Kconfig
> >> @@ -186,6 +186,17 @@ config INPUT_PMIC8XXX_PWRKEY
> >>  	  To compile this driver as a module, choose M here: the
> >>  	  module will be called pmic8xxx-pwrkey.
> >>
> >> +config INPUT_QPNP_HAPTICS
> >
> > No-one knows what "QPNP" really is, so please name this
> > "INPUT_QCOM_SPMI_HAPTICS"
> >
> >> +	tristate "Qualcomm QPNP HAPTICS"
> >> +	depends on ARCH_QCOM
> >
> > "depends on SPMI" makes sense here.
> >
> >> +	select INPUT_FF_MEMLESS
> >> +	help
> >> +	  This option enables support for the haptics found in pmi8998 and
> >> +	  related PMICs. Based on the ff-memless interface.
> >> +
> >> +	  To compile this driver as module, choose M here: the
> >> +	  module will be called qpnp-haptics.
> >> +
> >>  config INPUT_SPARCSPKR
> >>  	tristate "SPARC Speaker support"
> >>  	depends on PCI && SPARC64
> >> diff --git a/drivers/input/misc/Makefile b/drivers/input/misc/Makefile
> >> index f593beed7e05..c43290163db0 100644
> >> --- a/drivers/input/misc/Makefile
> >> +++ b/drivers/input/misc/Makefile
> >> @@ -65,6 +65,7 @@ obj-$(CONFIG_INPUT_PMIC8XXX_PWRKEY)	+= pmic8xxx-pwrkey.o
> >>  obj-$(CONFIG_INPUT_POWERMATE)		+= powermate.o
> >>  obj-$(CONFIG_INPUT_PWM_BEEPER)		+= pwm-beeper.o
> >>  obj-$(CONFIG_INPUT_PWM_VIBRA)		+= pwm-vibra.o
> >> +obj-$(CONFIG_INPUT_QPNP_HAPTICS)	+= qpnp-haptics.o
> >>  obj-$(CONFIG_INPUT_RAVE_SP_PWRBUTTON)	+= rave-sp-pwrbutton.o
> >>  obj-$(CONFIG_INPUT_RB532_BUTTON)	+= rb532_button.o
> >>  obj-$(CONFIG_INPUT_REGULATOR_HAPTIC)	+= regulator-haptic.o
> >> diff --git a/drivers/input/misc/qpnp-haptics.c b/drivers/input/misc/qpnp-haptics.c
> >
> > Again, qcom-spmi-haptics.c
> >
> >> new file mode 100644
> >> index 000000000000..daa7a18ffc7d
> >> --- /dev/null
> >> +++ b/drivers/input/misc/qpnp-haptics.c
> >> @@ -0,0 +1,1022 @@
> >> +// SPDX-License-Identifier: GPL-2.0-only
> >> +/*
> >> + * Copyright (c) 2021, Caleb Connolly <caleb@connolly.tech>
> >> + * Qualcomm Plug and Play haptics driver for pmi8998 and related PMICs.
> >> + * Based on ./pm8xxx-vibrator.c
> >
> > Enough "based on" to warrant carrying some copyrights?
> Hmm, I only stole the input device parts, so perhaps not.

Then I think you can drop this comment as well.

Regards,
Bjorn

Re: [PATCH 1/4] dt-bindings: input: add Qualcomm QPNP haptics driver

[Rob Herring]

On Sat, 12 Jun 2021 20:54:31 +0000, Caleb Connolly wrote:
> Add bindings for pmi8998 qpnp haptics driver.
> 
> Signed-off-by: Caleb Connolly <caleb@connolly.tech>
> ---
>  .../bindings/input/qcom,qpnp-haptics.yaml     | 123 ++++++++++++++++++
>  include/dt-bindings/input/qcom,qpnp-haptics.h |  31 +++++
>  2 files changed, 154 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/input/qcom,qpnp-haptics.yaml
>  create mode 100644 include/dt-bindings/input/qcom,qpnp-haptics.h
> 

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:
Documentation/devicetree/bindings/input/qcom,qpnp-haptics.example.dt.yaml:0:0: /example-0/pmic@3: failed to match any schema with compatible: ['qcom,pmi8998', 'qcom,spmi-pmic']
Documentation/devicetree/bindings/input/qcom,qpnp-haptics.example.dt.yaml:0:0: /example-0/pmic@3: failed to match any schema with compatible: ['qcom,pmi8998', 'qcom,spmi-pmic']
\ndoc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/patch/1491323

This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.

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.