[PATCH 0/5] MediaTek Helio X10 MT6795 - MT6331 PMIC Keys

[PATCH 0/5] MediaTek Helio X10 MT6795 - MT6331 PMIC Keys

[AngeloGioacchino Del Regno]

In an effort to give some love to the apparently forgotten MT6795 SoC,
I am upstreaming more components that are necessary to support platforms
powered by this one apart from a simple boot to serial console.

This series performs some cleanups in mtk-pmic-keys and adds support for
the MT6331 PMIC's keys.

Adding support to each driver in each subsystem is done in different
patch series as to avoid spamming uninteresting patches to maintainers.

This series depends on another two series series [1], [2] named
"MediaTek Helio X10 MT6795 - MT6331/6332 PMIC Wrapper" and
"MediaTek Helio X10 MT6795 - MT6331/6332 PMIC MFD integration"

Tested on a MT6795 Sony Xperia M5 (codename "Holly") smartphone.


[1]: https://lore.kernel.org/lkml/20220520124039.228314-1-angelogioacchino.delregno@collabora.com/T/#t
[2]: https://lore.kernel.org/lkml/20220520124617.228808-1-angelogioacchino.delregno@collabora.com/T/#t

AngeloGioacchino Del Regno (5):
  Input: mtk-pmic-keys - Add kerneldoc to driver structures
  Input: mtk-pmic-keys - Use regmap_{set,clear}_bits where possible
  Input: mtk-pmic-keys - Transfer per-key bit in mtk_pmic_keys_regs
  Input: mtk-pmic-keys - Move long press debounce mask to mtk_pmic_regs
  Input: mtk-pmic-keys - Add support for MT6331 PMIC keys

 drivers/input/keyboard/mtk-pmic-keys.c | 128 +++++++++++++++++--------
 1 file changed, 87 insertions(+), 41 deletions(-)

-- 
2.35.1

[PATCH 1/5] Input: mtk-pmic-keys - Add kerneldoc to driver structures

[AngeloGioacchino Del Regno]

To enhance human readability, add kerneldoc to all driver structs.

Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 drivers/input/keyboard/mtk-pmic-keys.c | 30 +++++++++++++++++++++++++-
 1 file changed, 29 insertions(+), 1 deletion(-)

diff --git a/drivers/input/keyboard/mtk-pmic-keys.c b/drivers/input/keyboard/mtk-pmic-keys.c
index c31ab4368388..8e4fa7cd16e6 100644
--- a/drivers/input/keyboard/mtk-pmic-keys.c
+++ b/drivers/input/keyboard/mtk-pmic-keys.c
@@ -34,6 +34,13 @@
 #define MTK_PMIC_HOMEKEY_INDEX	1
 #define MTK_PMIC_MAX_KEY_COUNT	2
 
+/**
+ * struct mtk_pmic_keys_regs - PMIC keys per-key registers
+ * @deb_reg:             Debounced key status register
+ * @deb_mask:            Bitmask of this key in status register
+ * @intsel_reg:          Interrupt selector register
+ * @intsel_mask:         Bitmask of this key in interrupt selector
+ */
 struct mtk_pmic_keys_regs {
 	u32 deb_reg;
 	u32 deb_mask;
@@ -50,6 +57,11 @@ struct mtk_pmic_keys_regs {
 	.intsel_mask		= _intsel_mask,		\
 }
 
+/**
+ * struct mtk_pmic_regs - PMIC Keys registers
+ * @keys_regs:           Specific key registers
+ * @pmic_rst_reg:        PMIC Keys reset register
+ */
 struct mtk_pmic_regs {
 	const struct mtk_pmic_keys_regs keys_regs[MTK_PMIC_MAX_KEY_COUNT];
 	u32 pmic_rst_reg;
@@ -85,15 +97,31 @@ static const struct mtk_pmic_regs mt6358_regs = {
 	.pmic_rst_reg = MT6358_TOP_RST_MISC,
 };
 
+/**
+ * struct mtk_pmic_keys_info - PMIC Keys per-key params
+ * @keys:                Pointer to main driver structure
+ * @regs:                Register offsets/masks for this key
+ * @keycode:             Key code for this key
+ * @irq:                 Keypress or press/release interrupt
+ * @irq_r:               Key release interrupt (optional)
+ * @wakeup:              Indicates whether to use this key as a wakeup source
+ */
 struct mtk_pmic_keys_info {
 	struct mtk_pmic_keys *keys;
 	const struct mtk_pmic_keys_regs *regs;
 	unsigned int keycode;
 	int irq;
-	int irq_r; /* optional: release irq if different */
+	int irq_r;
 	bool wakeup:1;
 };
 
+/**
+ * struct mtk_pmic_keys - Main driver structure
+ * @input_dev:           Input device pointer
+ * @dev:                 Device pointer
+ * @regmap:              Regmap handle
+ * @keys:                Per-key parameters
+ */
 struct mtk_pmic_keys {
 	struct input_dev *input_dev;
 	struct device *dev;
-- 
2.35.1

[PATCH 2/5] Input: mtk-pmic-keys - Use regmap_{set,clear}_bits where possible

[AngeloGioacchino Del Regno]

Instead of always using regmap_update_bits(), let's go for the shorter
regmap_set_bits() and regmap_clear_bits() where possible.

No functional change.

Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 drivers/input/keyboard/mtk-pmic-keys.c | 24 ++++++------------------
 1 file changed, 6 insertions(+), 18 deletions(-)

diff --git a/drivers/input/keyboard/mtk-pmic-keys.c b/drivers/input/keyboard/mtk-pmic-keys.c
index 8e4fa7cd16e6..83d0b90cc8cb 100644
--- a/drivers/input/keyboard/mtk-pmic-keys.c
+++ b/drivers/input/keyboard/mtk-pmic-keys.c
@@ -157,28 +157,16 @@ static void mtk_pmic_keys_lp_reset_setup(struct mtk_pmic_keys *keys,
 
 	switch (long_press_mode) {
 	case LP_ONEKEY:
-		regmap_update_bits(keys->regmap, pmic_rst_reg,
-				   MTK_PMIC_PWRKEY_RST,
-				   MTK_PMIC_PWRKEY_RST);
-		regmap_update_bits(keys->regmap, pmic_rst_reg,
-				   MTK_PMIC_HOMEKEY_RST,
-				   0);
+		regmap_set_bits(keys->regmap, pmic_rst_reg, MTK_PMIC_PWRKEY_RST);
+		regmap_clear_bits(keys->regmap, pmic_rst_reg, MTK_PMIC_HOMEKEY_RST);
 		break;
 	case LP_TWOKEY:
-		regmap_update_bits(keys->regmap, pmic_rst_reg,
-				   MTK_PMIC_PWRKEY_RST,
-				   MTK_PMIC_PWRKEY_RST);
-		regmap_update_bits(keys->regmap, pmic_rst_reg,
-				   MTK_PMIC_HOMEKEY_RST,
-				   MTK_PMIC_HOMEKEY_RST);
+		regmap_set_bits(keys->regmap, pmic_rst_reg, MTK_PMIC_PWRKEY_RST);
+		regmap_set_bits(keys->regmap, pmic_rst_reg, MTK_PMIC_HOMEKEY_RST);
 		break;
 	case LP_DISABLE:
-		regmap_update_bits(keys->regmap, pmic_rst_reg,
-				   MTK_PMIC_PWRKEY_RST,
-				   0);
-		regmap_update_bits(keys->regmap, pmic_rst_reg,
-				   MTK_PMIC_HOMEKEY_RST,
-				   0);
+		regmap_clear_bits(keys->regmap, pmic_rst_reg, MTK_PMIC_PWRKEY_RST);
+		regmap_clear_bits(keys->regmap, pmic_rst_reg, MTK_PMIC_HOMEKEY_RST);
 		break;
 	default:
 		break;
-- 
2.35.1

[PATCH 3/5] Input: mtk-pmic-keys - Transfer per-key bit in mtk_pmic_keys_regs

[AngeloGioacchino Del Regno]

Place the key bit in struct mtk_pmic_keys_regs to enhance this
driver's flexibility, in preparation for adding support for more
PMICs.

While at it, also remove the *_MASK and *_SHIFT definitions, as
these can be simply expressed as BIT(x), and "slightly rename"
the MTK_PMIC_{HOME,PWR}KEY_RST macro to better reflect the real
name for these bits.

This commit brings no functional changes.

Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 drivers/input/keyboard/mtk-pmic-keys.c | 46 ++++++++++++++------------
 1 file changed, 25 insertions(+), 21 deletions(-)

diff --git a/drivers/input/keyboard/mtk-pmic-keys.c b/drivers/input/keyboard/mtk-pmic-keys.c
index 83d0b90cc8cb..d8285612265f 100644
--- a/drivers/input/keyboard/mtk-pmic-keys.c
+++ b/drivers/input/keyboard/mtk-pmic-keys.c
@@ -18,17 +18,11 @@
 #include <linux/platform_device.h>
 #include <linux/regmap.h>
 
-#define MTK_PMIC_PWRKEY_RST_EN_MASK	0x1
-#define MTK_PMIC_PWRKEY_RST_EN_SHIFT	6
-#define MTK_PMIC_HOMEKEY_RST_EN_MASK	0x1
-#define MTK_PMIC_HOMEKEY_RST_EN_SHIFT	5
 #define MTK_PMIC_RST_DU_MASK		0x3
 #define MTK_PMIC_RST_DU_SHIFT		8
 
-#define MTK_PMIC_PWRKEY_RST		\
-	(MTK_PMIC_PWRKEY_RST_EN_MASK << MTK_PMIC_PWRKEY_RST_EN_SHIFT)
-#define MTK_PMIC_HOMEKEY_RST		\
-	(MTK_PMIC_HOMEKEY_RST_EN_MASK << MTK_PMIC_HOMEKEY_RST_EN_SHIFT)
+#define MTK_PMIC_MT6397_HOMEKEY_RST_EN	BIT(5)
+#define MTK_PMIC_MT6397_PWRKEY_RST_EN	BIT(6)
 
 #define MTK_PMIC_PWRKEY_INDEX	0
 #define MTK_PMIC_HOMEKEY_INDEX	1
@@ -40,21 +34,24 @@
  * @deb_mask:            Bitmask of this key in status register
  * @intsel_reg:          Interrupt selector register
  * @intsel_mask:         Bitmask of this key in interrupt selector
+ * @rst_en_mask:         Bitmask of this key in PMIC keys reset register
  */
 struct mtk_pmic_keys_regs {
 	u32 deb_reg;
 	u32 deb_mask;
 	u32 intsel_reg;
 	u32 intsel_mask;
+	u32 rst_en_mask;
 };
 
 #define MTK_PMIC_KEYS_REGS(_deb_reg, _deb_mask,		\
-	_intsel_reg, _intsel_mask)			\
+	_intsel_reg, _intsel_mask, _rst_mask)		\
 {							\
 	.deb_reg		= _deb_reg,		\
 	.deb_mask		= _deb_mask,		\
 	.intsel_reg		= _intsel_reg,		\
 	.intsel_mask		= _intsel_mask,		\
+	.rst_en_mask		= _rst_mask,		\
 }
 
 /**
@@ -70,30 +67,32 @@ struct mtk_pmic_regs {
 static const struct mtk_pmic_regs mt6397_regs = {
 	.keys_regs[MTK_PMIC_PWRKEY_INDEX] =
 		MTK_PMIC_KEYS_REGS(MT6397_CHRSTATUS,
-		0x8, MT6397_INT_RSV, 0x10),
+		0x8, MT6397_INT_RSV, 0x10, MTK_PMIC_MT6397_PWRKEY_RST_EN),
 	.keys_regs[MTK_PMIC_HOMEKEY_INDEX] =
 		MTK_PMIC_KEYS_REGS(MT6397_OCSTATUS2,
-		0x10, MT6397_INT_RSV, 0x8),
+		0x10, MT6397_INT_RSV, 0x8, MTK_PMIC_MT6397_HOMEKEY_RST_EN),
 	.pmic_rst_reg = MT6397_TOP_RST_MISC,
 };
 
 static const struct mtk_pmic_regs mt6323_regs = {
 	.keys_regs[MTK_PMIC_PWRKEY_INDEX] =
 		MTK_PMIC_KEYS_REGS(MT6323_CHRSTATUS,
-		0x2, MT6323_INT_MISC_CON, 0x10),
+		0x2, MT6323_INT_MISC_CON, 0x10, MTK_PMIC_MT6397_PWRKEY_RST_EN),
 	.keys_regs[MTK_PMIC_HOMEKEY_INDEX] =
 		MTK_PMIC_KEYS_REGS(MT6323_CHRSTATUS,
-		0x4, MT6323_INT_MISC_CON, 0x8),
+		0x4, MT6323_INT_MISC_CON, 0x8, MTK_PMIC_MT6397_HOMEKEY_RST_EN),
 	.pmic_rst_reg = MT6323_TOP_RST_MISC,
 };
 
 static const struct mtk_pmic_regs mt6358_regs = {
 	.keys_regs[MTK_PMIC_PWRKEY_INDEX] =
 		MTK_PMIC_KEYS_REGS(MT6358_TOPSTATUS,
-				   0x2, MT6358_PSC_TOP_INT_CON0, 0x5),
+				   0x2, MT6358_PSC_TOP_INT_CON0, 0x5,
+				   MTK_PMIC_MT6397_PWRKEY_RST_EN),
 	.keys_regs[MTK_PMIC_HOMEKEY_INDEX] =
 		MTK_PMIC_KEYS_REGS(MT6358_TOPSTATUS,
-				   0x8, MT6358_PSC_TOP_INT_CON0, 0xa),
+				   0x8, MT6358_PSC_TOP_INT_CON0, 0xa,
+				   MTK_PMIC_MT6397_HOMEKEY_RST_EN),
 	.pmic_rst_reg = MT6358_TOP_RST_MISC,
 };
 
@@ -140,6 +139,11 @@ static void mtk_pmic_keys_lp_reset_setup(struct mtk_pmic_keys *keys,
 {
 	int ret;
 	u32 long_press_mode, long_press_debounce;
+	const struct mtk_pmic_keys_regs *kregs_pwr;
+	const struct mtk_pmic_keys_regs *kregs_home;
+
+	kregs_pwr = keys->keys[MTK_PMIC_PWRKEY_INDEX].regs;
+	kregs_home = keys->keys[MTK_PMIC_HOMEKEY_INDEX].regs;
 
 	ret = of_property_read_u32(keys->dev->of_node,
 		"power-off-time-sec", &long_press_debounce);
@@ -157,16 +161,16 @@ static void mtk_pmic_keys_lp_reset_setup(struct mtk_pmic_keys *keys,
 
 	switch (long_press_mode) {
 	case LP_ONEKEY:
-		regmap_set_bits(keys->regmap, pmic_rst_reg, MTK_PMIC_PWRKEY_RST);
-		regmap_clear_bits(keys->regmap, pmic_rst_reg, MTK_PMIC_HOMEKEY_RST);
+		regmap_set_bits(keys->regmap, pmic_rst_reg, kregs_pwr->rst_en_mask);
+		regmap_clear_bits(keys->regmap, pmic_rst_reg, kregs_home->rst_en_mask);
 		break;
 	case LP_TWOKEY:
-		regmap_set_bits(keys->regmap, pmic_rst_reg, MTK_PMIC_PWRKEY_RST);
-		regmap_set_bits(keys->regmap, pmic_rst_reg, MTK_PMIC_HOMEKEY_RST);
+		regmap_set_bits(keys->regmap, pmic_rst_reg, kregs_pwr->rst_en_mask);
+		regmap_set_bits(keys->regmap, pmic_rst_reg, kregs_home->rst_en_mask);
 		break;
 	case LP_DISABLE:
-		regmap_clear_bits(keys->regmap, pmic_rst_reg, MTK_PMIC_PWRKEY_RST);
-		regmap_clear_bits(keys->regmap, pmic_rst_reg, MTK_PMIC_HOMEKEY_RST);
+		regmap_clear_bits(keys->regmap, pmic_rst_reg, kregs_pwr->rst_en_mask);
+		regmap_clear_bits(keys->regmap, pmic_rst_reg, kregs_home->rst_en_mask);
 		break;
 	default:
 		break;
-- 
2.35.1

[PATCH 4/5] Input: mtk-pmic-keys - Move long press debounce mask to mtk_pmic_regs

[AngeloGioacchino Del Regno]

As the second and last step of preparation to add support for more
PMICs in this driver, move the long press debounce mask to struct
mtk_pmic_regs and use that in mtk_pmic_keys_lp_reset_setup() instead
of directly using the definition.

While at it, remove the MTK_PMIC_RST_DU_{MASK,SHIFT} definitions, as
these can be expressed with the GENMASK macro and a new name was
chosen for that, as to uniform the definition names with the others
found in this driver.

Lastly, it was necessary to change the function signature of
mtk_pmic_keys_lp_reset_setup() to now pass a pointer to the main
mtk_pmic_regs structure, since that's what contains the reset
debounce mask now and, for readability purposes, for this function,
all of the references to keys->regmap were changed to use a local
'rmap' pointer, or the calls to regmap_{set,clear}_bits would be
~94 columns long.

This commit brings no functional changes.

Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 drivers/input/keyboard/mtk-pmic-keys.c | 33 +++++++++++++++-----------
 1 file changed, 19 insertions(+), 14 deletions(-)

diff --git a/drivers/input/keyboard/mtk-pmic-keys.c b/drivers/input/keyboard/mtk-pmic-keys.c
index d8285612265f..acd5aefac5f9 100644
--- a/drivers/input/keyboard/mtk-pmic-keys.c
+++ b/drivers/input/keyboard/mtk-pmic-keys.c
@@ -18,11 +18,9 @@
 #include <linux/platform_device.h>
 #include <linux/regmap.h>
 
-#define MTK_PMIC_RST_DU_MASK		0x3
-#define MTK_PMIC_RST_DU_SHIFT		8
-
 #define MTK_PMIC_MT6397_HOMEKEY_RST_EN	BIT(5)
 #define MTK_PMIC_MT6397_PWRKEY_RST_EN	BIT(6)
+#define MTK_PMIC_MT6397_RST_DU_MASK	GENMASK(9, 8)
 
 #define MTK_PMIC_PWRKEY_INDEX	0
 #define MTK_PMIC_HOMEKEY_INDEX	1
@@ -58,10 +56,12 @@ struct mtk_pmic_keys_regs {
  * struct mtk_pmic_regs - PMIC Keys registers
  * @keys_regs:           Specific key registers
  * @pmic_rst_reg:        PMIC Keys reset register
+ * @rst_lprst_mask:      Long-press reset timeout bitmask
  */
 struct mtk_pmic_regs {
 	const struct mtk_pmic_keys_regs keys_regs[MTK_PMIC_MAX_KEY_COUNT];
 	u32 pmic_rst_reg;
+	u32 rst_lprst_mask;
 };
 
 static const struct mtk_pmic_regs mt6397_regs = {
@@ -72,6 +72,7 @@ static const struct mtk_pmic_regs mt6397_regs = {
 		MTK_PMIC_KEYS_REGS(MT6397_OCSTATUS2,
 		0x10, MT6397_INT_RSV, 0x8, MTK_PMIC_MT6397_HOMEKEY_RST_EN),
 	.pmic_rst_reg = MT6397_TOP_RST_MISC,
+	.rst_lprst_mask = MTK_PMIC_MT6397_RST_DU_MASK,
 };
 
 static const struct mtk_pmic_regs mt6323_regs = {
@@ -82,6 +83,7 @@ static const struct mtk_pmic_regs mt6323_regs = {
 		MTK_PMIC_KEYS_REGS(MT6323_CHRSTATUS,
 		0x4, MT6323_INT_MISC_CON, 0x8, MTK_PMIC_MT6397_HOMEKEY_RST_EN),
 	.pmic_rst_reg = MT6323_TOP_RST_MISC,
+	.rst_lprst_mask = MTK_PMIC_MT6397_RST_DU_MASK,
 };
 
 static const struct mtk_pmic_regs mt6358_regs = {
@@ -94,6 +96,7 @@ static const struct mtk_pmic_regs mt6358_regs = {
 				   0x8, MT6358_PSC_TOP_INT_CON0, 0xa,
 				   MTK_PMIC_MT6397_HOMEKEY_RST_EN),
 	.pmic_rst_reg = MT6358_TOP_RST_MISC,
+	.rst_lprst_mask = MTK_PMIC_MT6397_RST_DU_MASK,
 };
 
 /**
@@ -135,24 +138,26 @@ enum mtk_pmic_keys_lp_mode {
 };
 
 static void mtk_pmic_keys_lp_reset_setup(struct mtk_pmic_keys *keys,
-		u32 pmic_rst_reg)
+					 const struct mtk_pmic_regs *regs)
 {
 	int ret;
+	struct regmap *rmap;
 	u32 long_press_mode, long_press_debounce;
 	const struct mtk_pmic_keys_regs *kregs_pwr;
 	const struct mtk_pmic_keys_regs *kregs_home;
 
 	kregs_pwr = keys->keys[MTK_PMIC_PWRKEY_INDEX].regs;
 	kregs_home = keys->keys[MTK_PMIC_HOMEKEY_INDEX].regs;
+	rmap = keys->regmap;
 
 	ret = of_property_read_u32(keys->dev->of_node,
 		"power-off-time-sec", &long_press_debounce);
 	if (ret)
 		long_press_debounce = 0;
 
-	regmap_update_bits(keys->regmap, pmic_rst_reg,
-			   MTK_PMIC_RST_DU_MASK << MTK_PMIC_RST_DU_SHIFT,
-			   long_press_debounce << MTK_PMIC_RST_DU_SHIFT);
+	regmap_update_bits(rmap, regs->pmic_rst_reg,
+			   regs->rst_lprst_mask,
+			   long_press_debounce << ffs(regs->rst_lprst_mask));
 
 	ret = of_property_read_u32(keys->dev->of_node,
 		"mediatek,long-press-mode", &long_press_mode);
@@ -161,16 +166,16 @@ static void mtk_pmic_keys_lp_reset_setup(struct mtk_pmic_keys *keys,
 
 	switch (long_press_mode) {
 	case LP_ONEKEY:
-		regmap_set_bits(keys->regmap, pmic_rst_reg, kregs_pwr->rst_en_mask);
-		regmap_clear_bits(keys->regmap, pmic_rst_reg, kregs_home->rst_en_mask);
+		regmap_set_bits(rmap, regs->pmic_rst_reg, kregs_pwr->rst_en_mask);
+		regmap_clear_bits(rmap, regs->pmic_rst_reg, kregs_home->rst_en_mask);
 		break;
 	case LP_TWOKEY:
-		regmap_set_bits(keys->regmap, pmic_rst_reg, kregs_pwr->rst_en_mask);
-		regmap_set_bits(keys->regmap, pmic_rst_reg, kregs_home->rst_en_mask);
+		regmap_set_bits(rmap, regs->pmic_rst_reg, kregs_pwr->rst_en_mask);
+		regmap_set_bits(rmap, regs->pmic_rst_reg, kregs_home->rst_en_mask);
 		break;
 	case LP_DISABLE:
-		regmap_clear_bits(keys->regmap, pmic_rst_reg, kregs_pwr->rst_en_mask);
-		regmap_clear_bits(keys->regmap, pmic_rst_reg, kregs_home->rst_en_mask);
+		regmap_clear_bits(rmap, regs->pmic_rst_reg, kregs_pwr->rst_en_mask);
+		regmap_clear_bits(rmap, regs->pmic_rst_reg, kregs_home->rst_en_mask);
 		break;
 	default:
 		break;
@@ -378,7 +383,7 @@ static int mtk_pmic_keys_probe(struct platform_device *pdev)
 		return error;
 	}
 
-	mtk_pmic_keys_lp_reset_setup(keys, mtk_pmic_regs->pmic_rst_reg);
+	mtk_pmic_keys_lp_reset_setup(keys, mtk_pmic_regs);
 
 	platform_set_drvdata(pdev, keys);
 
-- 
2.35.1

[PATCH 5/5] Input: mtk-pmic-keys - Add support for MT6331 PMIC keys

[AngeloGioacchino Del Regno]

Add support for PMIC Keys of the MT6331 PMIC.

Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 drivers/input/keyboard/mtk-pmic-keys.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/drivers/input/keyboard/mtk-pmic-keys.c b/drivers/input/keyboard/mtk-pmic-keys.c
index acd5aefac5f9..4a03fdfe8282 100644
--- a/drivers/input/keyboard/mtk-pmic-keys.c
+++ b/drivers/input/keyboard/mtk-pmic-keys.c
@@ -9,6 +9,7 @@
 #include <linux/interrupt.h>
 #include <linux/kernel.h>
 #include <linux/mfd/mt6323/registers.h>
+#include <linux/mfd/mt6331/registers.h>
 #include <linux/mfd/mt6358/registers.h>
 #include <linux/mfd/mt6397/core.h>
 #include <linux/mfd/mt6397/registers.h>
@@ -22,6 +23,10 @@
 #define MTK_PMIC_MT6397_PWRKEY_RST_EN	BIT(6)
 #define MTK_PMIC_MT6397_RST_DU_MASK	GENMASK(9, 8)
 
+#define MTK_PMIC_MT6331_HOMEKEY_RST_EN	BIT(8)
+#define MTK_PMIC_MT6331_PWRKEY_RST_EN	BIT(9)
+#define MTK_PMIC_MT6331_RST_DU_MASK	GENMASK(13, 12)
+
 #define MTK_PMIC_PWRKEY_INDEX	0
 #define MTK_PMIC_HOMEKEY_INDEX	1
 #define MTK_PMIC_MAX_KEY_COUNT	2
@@ -86,6 +91,19 @@ static const struct mtk_pmic_regs mt6323_regs = {
 	.rst_lprst_mask = MTK_PMIC_MT6397_RST_DU_MASK,
 };
 
+static const struct mtk_pmic_regs mt6331_regs = {
+	.keys_regs[MTK_PMIC_PWRKEY_INDEX] =
+		MTK_PMIC_KEYS_REGS(MT6331_TOPSTATUS, 0x2,
+				   MT6331_INT_MISC_CON, 0x4,
+				   MTK_PMIC_MT6331_PWRKEY_RST_EN),
+	.keys_regs[MTK_PMIC_HOMEKEY_INDEX] =
+		MTK_PMIC_KEYS_REGS(MT6331_TOPSTATUS, 0x4,
+				   MT6331_INT_MISC_CON, 0x2,
+				   MTK_PMIC_MT6331_HOMEKEY_RST_EN),
+	.pmic_rst_reg = MT6331_TOP_RST_MISC,
+	.rst_lprst_mask = MTK_PMIC_MT6331_RST_DU_MASK,
+};
+
 static const struct mtk_pmic_regs mt6358_regs = {
 	.keys_regs[MTK_PMIC_PWRKEY_INDEX] =
 		MTK_PMIC_KEYS_REGS(MT6358_TOPSTATUS,
@@ -284,6 +302,9 @@ static const struct of_device_id of_mtk_pmic_keys_match_tbl[] = {
 	}, {
 		.compatible = "mediatek,mt6323-keys",
 		.data = &mt6323_regs,
+	}, {
+		.compatible = "mediatek,mt6331-keys",
+		.data = &mt6331_regs,
 	}, {
 		.compatible = "mediatek,mt6358-keys",
 		.data = &mt6358_regs,
-- 
2.35.1

Re: [PATCH 1/5] Input: mtk-pmic-keys - Add kerneldoc to driver structures

[Mattijs Korpershoek]

On ven., mai 20, 2022 at 14:51, AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> wrote:

> To enhance human readability, add kerneldoc to all driver structs.
>
> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

Reviewed-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>
> ---
>  drivers/input/keyboard/mtk-pmic-keys.c | 30 +++++++++++++++++++++++++-
>  1 file changed, 29 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/input/keyboard/mtk-pmic-keys.c b/drivers/input/keyboard/mtk-pmic-keys.c
> index c31ab4368388..8e4fa7cd16e6 100644
> --- a/drivers/input/keyboard/mtk-pmic-keys.c
> +++ b/drivers/input/keyboard/mtk-pmic-keys.c
> @@ -34,6 +34,13 @@
>  #define MTK_PMIC_HOMEKEY_INDEX	1
>  #define MTK_PMIC_MAX_KEY_COUNT	2
>  
> +/**
> + * struct mtk_pmic_keys_regs - PMIC keys per-key registers
> + * @deb_reg:             Debounced key status register
> + * @deb_mask:            Bitmask of this key in status register
> + * @intsel_reg:          Interrupt selector register
> + * @intsel_mask:         Bitmask of this key in interrupt selector
> + */
>  struct mtk_pmic_keys_regs {
>  	u32 deb_reg;
>  	u32 deb_mask;
> @@ -50,6 +57,11 @@ struct mtk_pmic_keys_regs {
>  	.intsel_mask		= _intsel_mask,		\
>  }
>  
> +/**
> + * struct mtk_pmic_regs - PMIC Keys registers
> + * @keys_regs:           Specific key registers
> + * @pmic_rst_reg:        PMIC Keys reset register
> + */
>  struct mtk_pmic_regs {
>  	const struct mtk_pmic_keys_regs keys_regs[MTK_PMIC_MAX_KEY_COUNT];
>  	u32 pmic_rst_reg;
> @@ -85,15 +97,31 @@ static const struct mtk_pmic_regs mt6358_regs = {
>  	.pmic_rst_reg = MT6358_TOP_RST_MISC,
>  };
>  
> +/**
> + * struct mtk_pmic_keys_info - PMIC Keys per-key params
> + * @keys:                Pointer to main driver structure
> + * @regs:                Register offsets/masks for this key
> + * @keycode:             Key code for this key
> + * @irq:                 Keypress or press/release interrupt
> + * @irq_r:               Key release interrupt (optional)
> + * @wakeup:              Indicates whether to use this key as a wakeup source
> + */
>  struct mtk_pmic_keys_info {
>  	struct mtk_pmic_keys *keys;
>  	const struct mtk_pmic_keys_regs *regs;
>  	unsigned int keycode;
>  	int irq;
> -	int irq_r; /* optional: release irq if different */
> +	int irq_r;
>  	bool wakeup:1;
>  };
>  
> +/**
> + * struct mtk_pmic_keys - Main driver structure
> + * @input_dev:           Input device pointer
> + * @dev:                 Device pointer
> + * @regmap:              Regmap handle
> + * @keys:                Per-key parameters
> + */
>  struct mtk_pmic_keys {
>  	struct input_dev *input_dev;
>  	struct device *dev;
> -- 
> 2.35.1

Re: [PATCH 2/5] Input: mtk-pmic-keys - Use regmap_{set,clear}_bits where possible

[Mattijs Korpershoek]

On ven., mai 20, 2022 at 14:51, AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> wrote:

> Instead of always using regmap_update_bits(), let's go for the shorter
> regmap_set_bits() and regmap_clear_bits() where possible.
>
> No functional change.
>
> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

Reviewed-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>
> ---
>  drivers/input/keyboard/mtk-pmic-keys.c | 24 ++++++------------------
>  1 file changed, 6 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/input/keyboard/mtk-pmic-keys.c b/drivers/input/keyboard/mtk-pmic-keys.c
> index 8e4fa7cd16e6..83d0b90cc8cb 100644
> --- a/drivers/input/keyboard/mtk-pmic-keys.c
> +++ b/drivers/input/keyboard/mtk-pmic-keys.c
> @@ -157,28 +157,16 @@ static void mtk_pmic_keys_lp_reset_setup(struct mtk_pmic_keys *keys,
>  
>  	switch (long_press_mode) {
>  	case LP_ONEKEY:
> -		regmap_update_bits(keys->regmap, pmic_rst_reg,
> -				   MTK_PMIC_PWRKEY_RST,
> -				   MTK_PMIC_PWRKEY_RST);
> -		regmap_update_bits(keys->regmap, pmic_rst_reg,
> -				   MTK_PMIC_HOMEKEY_RST,
> -				   0);
> +		regmap_set_bits(keys->regmap, pmic_rst_reg, MTK_PMIC_PWRKEY_RST);
> +		regmap_clear_bits(keys->regmap, pmic_rst_reg, MTK_PMIC_HOMEKEY_RST);
>  		break;
>  	case LP_TWOKEY:
> -		regmap_update_bits(keys->regmap, pmic_rst_reg,
> -				   MTK_PMIC_PWRKEY_RST,
> -				   MTK_PMIC_PWRKEY_RST);
> -		regmap_update_bits(keys->regmap, pmic_rst_reg,
> -				   MTK_PMIC_HOMEKEY_RST,
> -				   MTK_PMIC_HOMEKEY_RST);
> +		regmap_set_bits(keys->regmap, pmic_rst_reg, MTK_PMIC_PWRKEY_RST);
> +		regmap_set_bits(keys->regmap, pmic_rst_reg, MTK_PMIC_HOMEKEY_RST);
>  		break;
>  	case LP_DISABLE:
> -		regmap_update_bits(keys->regmap, pmic_rst_reg,
> -				   MTK_PMIC_PWRKEY_RST,
> -				   0);
> -		regmap_update_bits(keys->regmap, pmic_rst_reg,
> -				   MTK_PMIC_HOMEKEY_RST,
> -				   0);
> +		regmap_clear_bits(keys->regmap, pmic_rst_reg, MTK_PMIC_PWRKEY_RST);
> +		regmap_clear_bits(keys->regmap, pmic_rst_reg, MTK_PMIC_HOMEKEY_RST);
>  		break;
>  	default:
>  		break;
> -- 
> 2.35.1

Re: [PATCH 3/5] Input: mtk-pmic-keys - Transfer per-key bit in mtk_pmic_keys_regs

[Mattijs Korpershoek]

On ven., mai 20, 2022 at 14:51, AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> wrote:

> Place the key bit in struct mtk_pmic_keys_regs to enhance this
> driver's flexibility, in preparation for adding support for more
> PMICs.
>
> While at it, also remove the *_MASK and *_SHIFT definitions, as
> these can be simply expressed as BIT(x), and "slightly rename"
> the MTK_PMIC_{HOME,PWR}KEY_RST macro to better reflect the real
> name for these bits.
>
> This commit brings no functional changes.
>
> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

Reviewed-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>
> ---
>  drivers/input/keyboard/mtk-pmic-keys.c | 46 ++++++++++++++------------
>  1 file changed, 25 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/input/keyboard/mtk-pmic-keys.c b/drivers/input/keyboard/mtk-pmic-keys.c
> index 83d0b90cc8cb..d8285612265f 100644
> --- a/drivers/input/keyboard/mtk-pmic-keys.c
> +++ b/drivers/input/keyboard/mtk-pmic-keys.c
> @@ -18,17 +18,11 @@
>  #include <linux/platform_device.h>
>  #include <linux/regmap.h>
>  
> -#define MTK_PMIC_PWRKEY_RST_EN_MASK	0x1
> -#define MTK_PMIC_PWRKEY_RST_EN_SHIFT	6
> -#define MTK_PMIC_HOMEKEY_RST_EN_MASK	0x1
> -#define MTK_PMIC_HOMEKEY_RST_EN_SHIFT	5
>  #define MTK_PMIC_RST_DU_MASK		0x3
>  #define MTK_PMIC_RST_DU_SHIFT		8
>  
> -#define MTK_PMIC_PWRKEY_RST		\
> -	(MTK_PMIC_PWRKEY_RST_EN_MASK << MTK_PMIC_PWRKEY_RST_EN_SHIFT)
> -#define MTK_PMIC_HOMEKEY_RST		\
> -	(MTK_PMIC_HOMEKEY_RST_EN_MASK << MTK_PMIC_HOMEKEY_RST_EN_SHIFT)
> +#define MTK_PMIC_MT6397_HOMEKEY_RST_EN	BIT(5)
> +#define MTK_PMIC_MT6397_PWRKEY_RST_EN	BIT(6)
>  
>  #define MTK_PMIC_PWRKEY_INDEX	0
>  #define MTK_PMIC_HOMEKEY_INDEX	1
> @@ -40,21 +34,24 @@
>   * @deb_mask:            Bitmask of this key in status register
>   * @intsel_reg:          Interrupt selector register
>   * @intsel_mask:         Bitmask of this key in interrupt selector
> + * @rst_en_mask:         Bitmask of this key in PMIC keys reset register
>   */
>  struct mtk_pmic_keys_regs {
>  	u32 deb_reg;
>  	u32 deb_mask;
>  	u32 intsel_reg;
>  	u32 intsel_mask;
> +	u32 rst_en_mask;
>  };
>  
>  #define MTK_PMIC_KEYS_REGS(_deb_reg, _deb_mask,		\
> -	_intsel_reg, _intsel_mask)			\
> +	_intsel_reg, _intsel_mask, _rst_mask)		\
>  {							\
>  	.deb_reg		= _deb_reg,		\
>  	.deb_mask		= _deb_mask,		\
>  	.intsel_reg		= _intsel_reg,		\
>  	.intsel_mask		= _intsel_mask,		\
> +	.rst_en_mask		= _rst_mask,		\
>  }
>  
>  /**
> @@ -70,30 +67,32 @@ struct mtk_pmic_regs {
>  static const struct mtk_pmic_regs mt6397_regs = {
>  	.keys_regs[MTK_PMIC_PWRKEY_INDEX] =
>  		MTK_PMIC_KEYS_REGS(MT6397_CHRSTATUS,
> -		0x8, MT6397_INT_RSV, 0x10),
> +		0x8, MT6397_INT_RSV, 0x10, MTK_PMIC_MT6397_PWRKEY_RST_EN),
>  	.keys_regs[MTK_PMIC_HOMEKEY_INDEX] =
>  		MTK_PMIC_KEYS_REGS(MT6397_OCSTATUS2,
> -		0x10, MT6397_INT_RSV, 0x8),
> +		0x10, MT6397_INT_RSV, 0x8, MTK_PMIC_MT6397_HOMEKEY_RST_EN),
>  	.pmic_rst_reg = MT6397_TOP_RST_MISC,
>  };
>  
>  static const struct mtk_pmic_regs mt6323_regs = {
>  	.keys_regs[MTK_PMIC_PWRKEY_INDEX] =
>  		MTK_PMIC_KEYS_REGS(MT6323_CHRSTATUS,
> -		0x2, MT6323_INT_MISC_CON, 0x10),
> +		0x2, MT6323_INT_MISC_CON, 0x10, MTK_PMIC_MT6397_PWRKEY_RST_EN),
>  	.keys_regs[MTK_PMIC_HOMEKEY_INDEX] =
>  		MTK_PMIC_KEYS_REGS(MT6323_CHRSTATUS,
> -		0x4, MT6323_INT_MISC_CON, 0x8),
> +		0x4, MT6323_INT_MISC_CON, 0x8, MTK_PMIC_MT6397_HOMEKEY_RST_EN),
>  	.pmic_rst_reg = MT6323_TOP_RST_MISC,
>  };
>  
>  static const struct mtk_pmic_regs mt6358_regs = {
>  	.keys_regs[MTK_PMIC_PWRKEY_INDEX] =
>  		MTK_PMIC_KEYS_REGS(MT6358_TOPSTATUS,
> -				   0x2, MT6358_PSC_TOP_INT_CON0, 0x5),
> +				   0x2, MT6358_PSC_TOP_INT_CON0, 0x5,
> +				   MTK_PMIC_MT6397_PWRKEY_RST_EN),
>  	.keys_regs[MTK_PMIC_HOMEKEY_INDEX] =
>  		MTK_PMIC_KEYS_REGS(MT6358_TOPSTATUS,
> -				   0x8, MT6358_PSC_TOP_INT_CON0, 0xa),
> +				   0x8, MT6358_PSC_TOP_INT_CON0, 0xa,
> +				   MTK_PMIC_MT6397_HOMEKEY_RST_EN),
>  	.pmic_rst_reg = MT6358_TOP_RST_MISC,
>  };
>  
> @@ -140,6 +139,11 @@ static void mtk_pmic_keys_lp_reset_setup(struct mtk_pmic_keys *keys,
>  {
>  	int ret;
>  	u32 long_press_mode, long_press_debounce;
> +	const struct mtk_pmic_keys_regs *kregs_pwr;
> +	const struct mtk_pmic_keys_regs *kregs_home;
> +
> +	kregs_pwr = keys->keys[MTK_PMIC_PWRKEY_INDEX].regs;
> +	kregs_home = keys->keys[MTK_PMIC_HOMEKEY_INDEX].regs;
>  
>  	ret = of_property_read_u32(keys->dev->of_node,
>  		"power-off-time-sec", &long_press_debounce);
> @@ -157,16 +161,16 @@ static void mtk_pmic_keys_lp_reset_setup(struct mtk_pmic_keys *keys,
>  
>  	switch (long_press_mode) {
>  	case LP_ONEKEY:
> -		regmap_set_bits(keys->regmap, pmic_rst_reg, MTK_PMIC_PWRKEY_RST);
> -		regmap_clear_bits(keys->regmap, pmic_rst_reg, MTK_PMIC_HOMEKEY_RST);
> +		regmap_set_bits(keys->regmap, pmic_rst_reg, kregs_pwr->rst_en_mask);
> +		regmap_clear_bits(keys->regmap, pmic_rst_reg, kregs_home->rst_en_mask);
>  		break;
>  	case LP_TWOKEY:
> -		regmap_set_bits(keys->regmap, pmic_rst_reg, MTK_PMIC_PWRKEY_RST);
> -		regmap_set_bits(keys->regmap, pmic_rst_reg, MTK_PMIC_HOMEKEY_RST);
> +		regmap_set_bits(keys->regmap, pmic_rst_reg, kregs_pwr->rst_en_mask);
> +		regmap_set_bits(keys->regmap, pmic_rst_reg, kregs_home->rst_en_mask);
>  		break;
>  	case LP_DISABLE:
> -		regmap_clear_bits(keys->regmap, pmic_rst_reg, MTK_PMIC_PWRKEY_RST);
> -		regmap_clear_bits(keys->regmap, pmic_rst_reg, MTK_PMIC_HOMEKEY_RST);
> +		regmap_clear_bits(keys->regmap, pmic_rst_reg, kregs_pwr->rst_en_mask);
> +		regmap_clear_bits(keys->regmap, pmic_rst_reg, kregs_home->rst_en_mask);
>  		break;
>  	default:
>  		break;
> -- 
> 2.35.1

Re: [PATCH 4/5] Input: mtk-pmic-keys - Move long press debounce mask to mtk_pmic_regs

[Mattijs Korpershoek]

On ven., mai 20, 2022 at 14:51, AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> wrote:

> As the second and last step of preparation to add support for more
> PMICs in this driver, move the long press debounce mask to struct
> mtk_pmic_regs and use that in mtk_pmic_keys_lp_reset_setup() instead
> of directly using the definition.
>
> While at it, remove the MTK_PMIC_RST_DU_{MASK,SHIFT} definitions, as
> these can be expressed with the GENMASK macro and a new name was
> chosen for that, as to uniform the definition names with the others
> found in this driver.
>
> Lastly, it was necessary to change the function signature of
> mtk_pmic_keys_lp_reset_setup() to now pass a pointer to the main
> mtk_pmic_regs structure, since that's what contains the reset
> debounce mask now and, for readability purposes, for this function,
> all of the references to keys->regmap were changed to use a local
> 'rmap' pointer, or the calls to regmap_{set,clear}_bits would be
> ~94 columns long.
>
> This commit brings no functional changes.
>
> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

Reviewed-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>
> ---
>  drivers/input/keyboard/mtk-pmic-keys.c | 33 +++++++++++++++-----------
>  1 file changed, 19 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/input/keyboard/mtk-pmic-keys.c b/drivers/input/keyboard/mtk-pmic-keys.c
> index d8285612265f..acd5aefac5f9 100644
> --- a/drivers/input/keyboard/mtk-pmic-keys.c
> +++ b/drivers/input/keyboard/mtk-pmic-keys.c
> @@ -18,11 +18,9 @@
>  #include <linux/platform_device.h>
>  #include <linux/regmap.h>
>  
> -#define MTK_PMIC_RST_DU_MASK		0x3
> -#define MTK_PMIC_RST_DU_SHIFT		8
> -
>  #define MTK_PMIC_MT6397_HOMEKEY_RST_EN	BIT(5)
>  #define MTK_PMIC_MT6397_PWRKEY_RST_EN	BIT(6)
> +#define MTK_PMIC_MT6397_RST_DU_MASK	GENMASK(9, 8)
>  
>  #define MTK_PMIC_PWRKEY_INDEX	0
>  #define MTK_PMIC_HOMEKEY_INDEX	1
> @@ -58,10 +56,12 @@ struct mtk_pmic_keys_regs {
>   * struct mtk_pmic_regs - PMIC Keys registers
>   * @keys_regs:           Specific key registers
>   * @pmic_rst_reg:        PMIC Keys reset register
> + * @rst_lprst_mask:      Long-press reset timeout bitmask
>   */
>  struct mtk_pmic_regs {
>  	const struct mtk_pmic_keys_regs keys_regs[MTK_PMIC_MAX_KEY_COUNT];
>  	u32 pmic_rst_reg;
> +	u32 rst_lprst_mask;
>  };
>  
>  static const struct mtk_pmic_regs mt6397_regs = {
> @@ -72,6 +72,7 @@ static const struct mtk_pmic_regs mt6397_regs = {
>  		MTK_PMIC_KEYS_REGS(MT6397_OCSTATUS2,
>  		0x10, MT6397_INT_RSV, 0x8, MTK_PMIC_MT6397_HOMEKEY_RST_EN),
>  	.pmic_rst_reg = MT6397_TOP_RST_MISC,
> +	.rst_lprst_mask = MTK_PMIC_MT6397_RST_DU_MASK,
>  };
>  
>  static const struct mtk_pmic_regs mt6323_regs = {
> @@ -82,6 +83,7 @@ static const struct mtk_pmic_regs mt6323_regs = {
>  		MTK_PMIC_KEYS_REGS(MT6323_CHRSTATUS,
>  		0x4, MT6323_INT_MISC_CON, 0x8, MTK_PMIC_MT6397_HOMEKEY_RST_EN),
>  	.pmic_rst_reg = MT6323_TOP_RST_MISC,
> +	.rst_lprst_mask = MTK_PMIC_MT6397_RST_DU_MASK,
>  };
>  
>  static const struct mtk_pmic_regs mt6358_regs = {
> @@ -94,6 +96,7 @@ static const struct mtk_pmic_regs mt6358_regs = {
>  				   0x8, MT6358_PSC_TOP_INT_CON0, 0xa,
>  				   MTK_PMIC_MT6397_HOMEKEY_RST_EN),
>  	.pmic_rst_reg = MT6358_TOP_RST_MISC,
> +	.rst_lprst_mask = MTK_PMIC_MT6397_RST_DU_MASK,
>  };
>  
>  /**
> @@ -135,24 +138,26 @@ enum mtk_pmic_keys_lp_mode {
>  };
>  
>  static void mtk_pmic_keys_lp_reset_setup(struct mtk_pmic_keys *keys,
> -		u32 pmic_rst_reg)
> +					 const struct mtk_pmic_regs *regs)
>  {
>  	int ret;
> +	struct regmap *rmap;
>  	u32 long_press_mode, long_press_debounce;
>  	const struct mtk_pmic_keys_regs *kregs_pwr;
>  	const struct mtk_pmic_keys_regs *kregs_home;
>  
>  	kregs_pwr = keys->keys[MTK_PMIC_PWRKEY_INDEX].regs;
>  	kregs_home = keys->keys[MTK_PMIC_HOMEKEY_INDEX].regs;
> +	rmap = keys->regmap;
>  
>  	ret = of_property_read_u32(keys->dev->of_node,
>  		"power-off-time-sec", &long_press_debounce);
>  	if (ret)
>  		long_press_debounce = 0;
>  
> -	regmap_update_bits(keys->regmap, pmic_rst_reg,
> -			   MTK_PMIC_RST_DU_MASK << MTK_PMIC_RST_DU_SHIFT,
> -			   long_press_debounce << MTK_PMIC_RST_DU_SHIFT);
> +	regmap_update_bits(rmap, regs->pmic_rst_reg,
> +			   regs->rst_lprst_mask,
> +			   long_press_debounce << ffs(regs->rst_lprst_mask));
>  
>  	ret = of_property_read_u32(keys->dev->of_node,
>  		"mediatek,long-press-mode", &long_press_mode);
> @@ -161,16 +166,16 @@ static void mtk_pmic_keys_lp_reset_setup(struct mtk_pmic_keys *keys,
>  
>  	switch (long_press_mode) {
>  	case LP_ONEKEY:
> -		regmap_set_bits(keys->regmap, pmic_rst_reg, kregs_pwr->rst_en_mask);
> -		regmap_clear_bits(keys->regmap, pmic_rst_reg, kregs_home->rst_en_mask);
> +		regmap_set_bits(rmap, regs->pmic_rst_reg, kregs_pwr->rst_en_mask);
> +		regmap_clear_bits(rmap, regs->pmic_rst_reg, kregs_home->rst_en_mask);
>  		break;
>  	case LP_TWOKEY:
> -		regmap_set_bits(keys->regmap, pmic_rst_reg, kregs_pwr->rst_en_mask);
> -		regmap_set_bits(keys->regmap, pmic_rst_reg, kregs_home->rst_en_mask);
> +		regmap_set_bits(rmap, regs->pmic_rst_reg, kregs_pwr->rst_en_mask);
> +		regmap_set_bits(rmap, regs->pmic_rst_reg, kregs_home->rst_en_mask);
>  		break;
>  	case LP_DISABLE:
> -		regmap_clear_bits(keys->regmap, pmic_rst_reg, kregs_pwr->rst_en_mask);
> -		regmap_clear_bits(keys->regmap, pmic_rst_reg, kregs_home->rst_en_mask);
> +		regmap_clear_bits(rmap, regs->pmic_rst_reg, kregs_pwr->rst_en_mask);
> +		regmap_clear_bits(rmap, regs->pmic_rst_reg, kregs_home->rst_en_mask);
>  		break;
>  	default:
>  		break;
> @@ -378,7 +383,7 @@ static int mtk_pmic_keys_probe(struct platform_device *pdev)
>  		return error;
>  	}
>  
> -	mtk_pmic_keys_lp_reset_setup(keys, mtk_pmic_regs->pmic_rst_reg);
> +	mtk_pmic_keys_lp_reset_setup(keys, mtk_pmic_regs);
>  
>  	platform_set_drvdata(pdev, keys);
>  
> -- 
> 2.35.1

Re: [PATCH 5/5] Input: mtk-pmic-keys - Add support for MT6331 PMIC keys

[Mattijs Korpershoek]

On ven., mai 20, 2022 at 14:51, AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> wrote:

> Add support for PMIC Keys of the MT6331 PMIC.
>
> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

Reviewed-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>
> ---
>  drivers/input/keyboard/mtk-pmic-keys.c | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
>
> diff --git a/drivers/input/keyboard/mtk-pmic-keys.c b/drivers/input/keyboard/mtk-pmic-keys.c
> index acd5aefac5f9..4a03fdfe8282 100644
> --- a/drivers/input/keyboard/mtk-pmic-keys.c
> +++ b/drivers/input/keyboard/mtk-pmic-keys.c
> @@ -9,6 +9,7 @@
>  #include <linux/interrupt.h>
>  #include <linux/kernel.h>
>  #include <linux/mfd/mt6323/registers.h>
> +#include <linux/mfd/mt6331/registers.h>
>  #include <linux/mfd/mt6358/registers.h>
>  #include <linux/mfd/mt6397/core.h>
>  #include <linux/mfd/mt6397/registers.h>
> @@ -22,6 +23,10 @@
>  #define MTK_PMIC_MT6397_PWRKEY_RST_EN	BIT(6)
>  #define MTK_PMIC_MT6397_RST_DU_MASK	GENMASK(9, 8)
>  
> +#define MTK_PMIC_MT6331_HOMEKEY_RST_EN	BIT(8)
> +#define MTK_PMIC_MT6331_PWRKEY_RST_EN	BIT(9)
> +#define MTK_PMIC_MT6331_RST_DU_MASK	GENMASK(13, 12)
> +
>  #define MTK_PMIC_PWRKEY_INDEX	0
>  #define MTK_PMIC_HOMEKEY_INDEX	1
>  #define MTK_PMIC_MAX_KEY_COUNT	2
> @@ -86,6 +91,19 @@ static const struct mtk_pmic_regs mt6323_regs = {
>  	.rst_lprst_mask = MTK_PMIC_MT6397_RST_DU_MASK,
>  };
>  
> +static const struct mtk_pmic_regs mt6331_regs = {
> +	.keys_regs[MTK_PMIC_PWRKEY_INDEX] =
> +		MTK_PMIC_KEYS_REGS(MT6331_TOPSTATUS, 0x2,
> +				   MT6331_INT_MISC_CON, 0x4,
> +				   MTK_PMIC_MT6331_PWRKEY_RST_EN),
> +	.keys_regs[MTK_PMIC_HOMEKEY_INDEX] =
> +		MTK_PMIC_KEYS_REGS(MT6331_TOPSTATUS, 0x4,
> +				   MT6331_INT_MISC_CON, 0x2,
> +				   MTK_PMIC_MT6331_HOMEKEY_RST_EN),
> +	.pmic_rst_reg = MT6331_TOP_RST_MISC,
> +	.rst_lprst_mask = MTK_PMIC_MT6331_RST_DU_MASK,
> +};
> +
>  static const struct mtk_pmic_regs mt6358_regs = {
>  	.keys_regs[MTK_PMIC_PWRKEY_INDEX] =
>  		MTK_PMIC_KEYS_REGS(MT6358_TOPSTATUS,
> @@ -284,6 +302,9 @@ static const struct of_device_id of_mtk_pmic_keys_match_tbl[] = {
>  	}, {
>  		.compatible = "mediatek,mt6323-keys",
>  		.data = &mt6323_regs,
> +	}, {
> +		.compatible = "mediatek,mt6331-keys",
> +		.data = &mt6331_regs,
>  	}, {
>  		.compatible = "mediatek,mt6358-keys",
>  		.data = &mt6358_regs,
> -- 
> 2.35.1

Re: [PATCH 1/5] Input: mtk-pmic-keys - Add kerneldoc to driver structures

[Dmitry Torokhov]

Hi AngeloGioacchino,

On Fri, May 20, 2022 at 02:51:28PM +0200, AngeloGioacchino Del Regno wrote:
> To enhance human readability, add kerneldoc to all driver structs.

I am doubtful that this is useful. The reason is that I believe
kerneldoc format is only useful for documenting cross-subsystem APIs.
Kerneldoc for driver-private data and functions simply pollutes API
docs.

> 
> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> ---
>  drivers/input/keyboard/mtk-pmic-keys.c | 30 +++++++++++++++++++++++++-
>  1 file changed, 29 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/input/keyboard/mtk-pmic-keys.c b/drivers/input/keyboard/mtk-pmic-keys.c
> index c31ab4368388..8e4fa7cd16e6 100644
> --- a/drivers/input/keyboard/mtk-pmic-keys.c
> +++ b/drivers/input/keyboard/mtk-pmic-keys.c
> @@ -34,6 +34,13 @@
>  #define MTK_PMIC_HOMEKEY_INDEX	1
>  #define MTK_PMIC_MAX_KEY_COUNT	2
>  
> +/**
> + * struct mtk_pmic_keys_regs - PMIC keys per-key registers
> + * @deb_reg:             Debounced key status register
> + * @deb_mask:            Bitmask of this key in status register
> + * @intsel_reg:          Interrupt selector register
> + * @intsel_mask:         Bitmask of this key in interrupt selector
> + */
>  struct mtk_pmic_keys_regs {
>  	u32 deb_reg;
>  	u32 deb_mask;
> @@ -50,6 +57,11 @@ struct mtk_pmic_keys_regs {
>  	.intsel_mask		= _intsel_mask,		\
>  }
>  
> +/**
> + * struct mtk_pmic_regs - PMIC Keys registers
> + * @keys_regs:           Specific key registers

This new description of the structure and of the keys_regs does not add
any information for me.

> + * @pmic_rst_reg:        PMIC Keys reset register
> + */
>  struct mtk_pmic_regs {
>  	const struct mtk_pmic_keys_regs keys_regs[MTK_PMIC_MAX_KEY_COUNT];
>  	u32 pmic_rst_reg;
> @@ -85,15 +97,31 @@ static const struct mtk_pmic_regs mt6358_regs = {
>  	.pmic_rst_reg = MT6358_TOP_RST_MISC,
>  };
>  
> +/**
> + * struct mtk_pmic_keys_info - PMIC Keys per-key params
> + * @keys:                Pointer to main driver structure

That is obvious from the field definition.

> + * @regs:                Register offsets/masks for this key

Ditto.

> + * @keycode:             Key code for this key

Yep.

> + * @irq:                 Keypress or press/release interrupt
> + * @irq_r:               Key release interrupt (optional)
> + * @wakeup:              Indicates whether to use this key as a wakeup source
> + */
>  struct mtk_pmic_keys_info {
>  	struct mtk_pmic_keys *keys;
>  	const struct mtk_pmic_keys_regs *regs;
>  	unsigned int keycode;
>  	int irq;
> -	int irq_r; /* optional: release irq if different */
> +	int irq_r;
>  	bool wakeup:1;
>  };
>  
> +/**
> + * struct mtk_pmic_keys - Main driver structure
> + * @input_dev:           Input device pointer

I do not find this helpful.

> + * @dev:                 Device pointer

And neither this.

> + * @regmap:              Regmap handle

Nor this.

> + * @keys:                Per-key parameters
> + */
>  struct mtk_pmic_keys {
>  	struct input_dev *input_dev;
>  	struct device *dev;
> -- 
> 2.35.1
> 

In the end we ended up with something that now has a chance of
introducing warning when someone changes code, for very little benefit,
if any at all.

For driver-private data and functions we should rely on expressive
variable and function names and only use comments for something that
might be unclear or requires additional qualification.

Thanks.

-- 
Dmitry

Re: [PATCH 2/5] Input: mtk-pmic-keys - Use regmap_{set,clear}_bits where possible

[Dmitry Torokhov]

On Fri, May 20, 2022 at 02:51:29PM +0200, AngeloGioacchino Del Regno wrote:
> Instead of always using regmap_update_bits(), let's go for the shorter
> regmap_set_bits() and regmap_clear_bits() where possible.
> 
> No functional change.
> 
> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> ---
>  drivers/input/keyboard/mtk-pmic-keys.c | 24 ++++++------------------
>  1 file changed, 6 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/input/keyboard/mtk-pmic-keys.c b/drivers/input/keyboard/mtk-pmic-keys.c
> index 8e4fa7cd16e6..83d0b90cc8cb 100644
> --- a/drivers/input/keyboard/mtk-pmic-keys.c
> +++ b/drivers/input/keyboard/mtk-pmic-keys.c
> @@ -157,28 +157,16 @@ static void mtk_pmic_keys_lp_reset_setup(struct mtk_pmic_keys *keys,
>  
>  	switch (long_press_mode) {
>  	case LP_ONEKEY:
> -		regmap_update_bits(keys->regmap, pmic_rst_reg,
> -				   MTK_PMIC_PWRKEY_RST,
> -				   MTK_PMIC_PWRKEY_RST);
> -		regmap_update_bits(keys->regmap, pmic_rst_reg,
> -				   MTK_PMIC_HOMEKEY_RST,
> -				   0);
> +		regmap_set_bits(keys->regmap, pmic_rst_reg, MTK_PMIC_PWRKEY_RST);
> +		regmap_clear_bits(keys->regmap, pmic_rst_reg, MTK_PMIC_HOMEKEY_RST);

Why not combine this into a single update instead? I.e. assuming

#define MTK_PMIC_KEY_RST_MASK GENMASK(6, 5)

		regmap_update_bits(keys->regmap, pmic_rst_reg,
				   MTK_PMIC_KEY_RST_MASK,
				   MTK_PMIC_PWRKEY_RST);

>  		break;
>  	case LP_TWOKEY:
> -		regmap_update_bits(keys->regmap, pmic_rst_reg,
> -				   MTK_PMIC_PWRKEY_RST,
> -				   MTK_PMIC_PWRKEY_RST);
> -		regmap_update_bits(keys->regmap, pmic_rst_reg,
> -				   MTK_PMIC_HOMEKEY_RST,
> -				   MTK_PMIC_HOMEKEY_RST);
> +		regmap_set_bits(keys->regmap, pmic_rst_reg, MTK_PMIC_PWRKEY_RST);
> +		regmap_set_bits(keys->regmap, pmic_rst_reg, MTK_PMIC_HOMEKEY_RST);

		regmap_update_bits(keys->regmap, pmic_rst_reg,
				   MTK_PMIC_KEY_RST_MASK,
				   MTK_PMIC_PWRKEY_RST | MTK_PMIC_HOMEKEY_RST);

>  		break;
>  	case LP_DISABLE:
> -		regmap_update_bits(keys->regmap, pmic_rst_reg,
> -				   MTK_PMIC_PWRKEY_RST,
> -				   0);
> -		regmap_update_bits(keys->regmap, pmic_rst_reg,
> -				   MTK_PMIC_HOMEKEY_RST,
> -				   0);
> +		regmap_clear_bits(keys->regmap, pmic_rst_reg, MTK_PMIC_PWRKEY_RST);
> +		regmap_clear_bits(keys->regmap, pmic_rst_reg, MTK_PMIC_HOMEKEY_RST);

		regmap_update_bits(keys->regmap, pmic_rst_reg,
				   MTK_PMIC_KEY_RST_MASKi, 0);

>  		break;
>  	default:
>  		break;
> -- 
> 2.35.1
> 

Thanks.

-- 
Dmitry

Re: [PATCH 1/5] Input: mtk-pmic-keys - Add kerneldoc to driver structures

[AngeloGioacchino Del Regno]

Il 23/05/22 06:33, Dmitry Torokhov ha scritto:
> Hi AngeloGioacchino,
> 
> On Fri, May 20, 2022 at 02:51:28PM +0200, AngeloGioacchino Del Regno wrote:
>> To enhance human readability, add kerneldoc to all driver structs.
> 
> I am doubtful that this is useful. The reason is that I believe
> kerneldoc format is only useful for documenting cross-subsystem APIs.
> Kerneldoc for driver-private data and functions simply pollutes API
> docs.
> 
>>
>> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
>> ---
>>   drivers/input/keyboard/mtk-pmic-keys.c | 30 +++++++++++++++++++++++++-
>>   1 file changed, 29 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/input/keyboard/mtk-pmic-keys.c b/drivers/input/keyboard/mtk-pmic-keys.c
>> index c31ab4368388..8e4fa7cd16e6 100644
>> --- a/drivers/input/keyboard/mtk-pmic-keys.c
>> +++ b/drivers/input/keyboard/mtk-pmic-keys.c
>> @@ -34,6 +34,13 @@
>>   #define MTK_PMIC_HOMEKEY_INDEX	1
>>   #define MTK_PMIC_MAX_KEY_COUNT	2
>>   
>> +/**
>> + * struct mtk_pmic_keys_regs - PMIC keys per-key registers
>> + * @deb_reg:             Debounced key status register
>> + * @deb_mask:            Bitmask of this key in status register
>> + * @intsel_reg:          Interrupt selector register
>> + * @intsel_mask:         Bitmask of this key in interrupt selector
>> + */
>>   struct mtk_pmic_keys_regs {
>>   	u32 deb_reg;
>>   	u32 deb_mask;
>> @@ -50,6 +57,11 @@ struct mtk_pmic_keys_regs {
>>   	.intsel_mask		= _intsel_mask,		\
>>   }
>>   
>> +/**
>> + * struct mtk_pmic_regs - PMIC Keys registers
>> + * @keys_regs:           Specific key registers
> 
> This new description of the structure and of the keys_regs does not add
> any information for me.
> 
>> + * @pmic_rst_reg:        PMIC Keys reset register
>> + */
>>   struct mtk_pmic_regs {
>>   	const struct mtk_pmic_keys_regs keys_regs[MTK_PMIC_MAX_KEY_COUNT];
>>   	u32 pmic_rst_reg;
>> @@ -85,15 +97,31 @@ static const struct mtk_pmic_regs mt6358_regs = {
>>   	.pmic_rst_reg = MT6358_TOP_RST_MISC,
>>   };
>>   
>> +/**
>> + * struct mtk_pmic_keys_info - PMIC Keys per-key params
>> + * @keys:                Pointer to main driver structure
> 
> That is obvious from the field definition.
> 
>> + * @regs:                Register offsets/masks for this key
> 
> Ditto.
> 
>> + * @keycode:             Key code for this key
> 
> Yep.
> 
>> + * @irq:                 Keypress or press/release interrupt
>> + * @irq_r:               Key release interrupt (optional)
>> + * @wakeup:              Indicates whether to use this key as a wakeup source
>> + */
>>   struct mtk_pmic_keys_info {
>>   	struct mtk_pmic_keys *keys;
>>   	const struct mtk_pmic_keys_regs *regs;
>>   	unsigned int keycode;
>>   	int irq;
>> -	int irq_r; /* optional: release irq if different */
>> +	int irq_r;
>>   	bool wakeup:1;
>>   };
>>   
>> +/**
>> + * struct mtk_pmic_keys - Main driver structure
>> + * @input_dev:           Input device pointer
> 
> I do not find this helpful.
> 
>> + * @dev:                 Device pointer
> 
> And neither this.
> 
>> + * @regmap:              Regmap handle
> 
> Nor this.
> 
>> + * @keys:                Per-key parameters
>> + */
>>   struct mtk_pmic_keys {
>>   	struct input_dev *input_dev;
>>   	struct device *dev;
>> -- 
>> 2.35.1
>>
> 
> In the end we ended up with something that now has a chance of
> introducing warning when someone changes code, for very little benefit,
> if any at all.
> 
> For driver-private data and functions we should rely on expressive
> variable and function names and only use comments for something that
> might be unclear or requires additional qualification.
> 

Hello Dmitry,

it's been very helpful for me to see kerneldoc documentation in the various
drivers across the kernel - helped me understanding what was going on in an
easier, more immediate way, especially when looking at drivers having some
kind of "complicated" flow.
About introducing warnings when someone changes code, I believe that this
may also be helpful (for a developer) in some *corner* cases, but I agree
that this is unnecessarily tedious in some others... in the end, it's all
about personal opinions...

Of course, some of the documentation being obvious is unavoidable when it
comes to kerneldoc as you either document 'em all, or nothing.

In any case, if you really dislike having this kind of documentation, I can
drop these commits and eventually add in-line comments to some variables to
make them perfectly understandable, or I can avoid documenting at all (even
though I am strongly for documenting things clearly).

Regards,
Angelo

> Thanks.
> 

Re: [PATCH 2/5] Input: mtk-pmic-keys - Use regmap_{set,clear}_bits where possible

[AngeloGioacchino Del Regno]

Il 23/05/22 06:51, Dmitry Torokhov ha scritto:
> On Fri, May 20, 2022 at 02:51:29PM +0200, AngeloGioacchino Del Regno wrote:
>> Instead of always using regmap_update_bits(), let's go for the shorter
>> regmap_set_bits() and regmap_clear_bits() where possible.
>>
>> No functional change.
>>
>> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
>> ---
>>   drivers/input/keyboard/mtk-pmic-keys.c | 24 ++++++------------------
>>   1 file changed, 6 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/input/keyboard/mtk-pmic-keys.c b/drivers/input/keyboard/mtk-pmic-keys.c
>> index 8e4fa7cd16e6..83d0b90cc8cb 100644
>> --- a/drivers/input/keyboard/mtk-pmic-keys.c
>> +++ b/drivers/input/keyboard/mtk-pmic-keys.c
>> @@ -157,28 +157,16 @@ static void mtk_pmic_keys_lp_reset_setup(struct mtk_pmic_keys *keys,
>>   
>>   	switch (long_press_mode) {
>>   	case LP_ONEKEY:
>> -		regmap_update_bits(keys->regmap, pmic_rst_reg,
>> -				   MTK_PMIC_PWRKEY_RST,
>> -				   MTK_PMIC_PWRKEY_RST);
>> -		regmap_update_bits(keys->regmap, pmic_rst_reg,
>> -				   MTK_PMIC_HOMEKEY_RST,
>> -				   0);
>> +		regmap_set_bits(keys->regmap, pmic_rst_reg, MTK_PMIC_PWRKEY_RST);
>> +		regmap_clear_bits(keys->regmap, pmic_rst_reg, MTK_PMIC_HOMEKEY_RST);
> 
> Why not combine this into a single update instead? I.e. assuming
> 

All downstream kernels (at least, I checked 4 different kernel versions for 4
different SoCs) are doing these updates one-at-a-time, never combining them.

Even though I agree with you about one single update being simply more logical,
I am afraid that (on some SoCs) the IP will not like that so - since I don't have
any *clear* documentation saying that this is possible, or that this is not, I
would leave it like that.


> #define MTK_PMIC_KEY_RST_MASK GENMASK(6, 5)
> 
> 		regmap_update_bits(keys->regmap, pmic_rst_reg,
> 				   MTK_PMIC_KEY_RST_MASK,
> 				   MTK_PMIC_PWRKEY_RST);
> 
>>   		break;
>>   	case LP_TWOKEY:
>> -		regmap_update_bits(keys->regmap, pmic_rst_reg,
>> -				   MTK_PMIC_PWRKEY_RST,
>> -				   MTK_PMIC_PWRKEY_RST);
>> -		regmap_update_bits(keys->regmap, pmic_rst_reg,
>> -				   MTK_PMIC_HOMEKEY_RST,
>> -				   MTK_PMIC_HOMEKEY_RST);
>> +		regmap_set_bits(keys->regmap, pmic_rst_reg, MTK_PMIC_PWRKEY_RST);
>> +		regmap_set_bits(keys->regmap, pmic_rst_reg, MTK_PMIC_HOMEKEY_RST);
> 
> 		regmap_update_bits(keys->regmap, pmic_rst_reg,
> 				   MTK_PMIC_KEY_RST_MASK,
> 				   MTK_PMIC_PWRKEY_RST | MTK_PMIC_HOMEKEY_RST);
> 
>>   		break;
>>   	case LP_DISABLE:
>> -		regmap_update_bits(keys->regmap, pmic_rst_reg,
>> -				   MTK_PMIC_PWRKEY_RST,
>> -				   0);
>> -		regmap_update_bits(keys->regmap, pmic_rst_reg,
>> -				   MTK_PMIC_HOMEKEY_RST,
>> -				   0);
>> +		regmap_clear_bits(keys->regmap, pmic_rst_reg, MTK_PMIC_PWRKEY_RST);
>> +		regmap_clear_bits(keys->regmap, pmic_rst_reg, MTK_PMIC_HOMEKEY_RST);
> 
> 		regmap_update_bits(keys->regmap, pmic_rst_reg,
> 				   MTK_PMIC_KEY_RST_MASKi, 0);
> 
>>   		break;
>>   	default:
>>   		break;
>> -- 
>> 2.35.1
>>
> 
> Thanks.
> 

Re: [PATCH 1/5] Input: mtk-pmic-keys - Add kerneldoc to driver structures

[Dmitry Torokhov]

On Mon, May 23, 2022 at 10:54:03AM +0200, AngeloGioacchino Del Regno wrote:
> Il 23/05/22 06:33, Dmitry Torokhov ha scritto:
> > Hi AngeloGioacchino,
> > 
> > On Fri, May 20, 2022 at 02:51:28PM +0200, AngeloGioacchino Del Regno wrote:
> > > To enhance human readability, add kerneldoc to all driver structs.
> > 
> > I am doubtful that this is useful. The reason is that I believe
> > kerneldoc format is only useful for documenting cross-subsystem APIs.
> > Kerneldoc for driver-private data and functions simply pollutes API
> > docs.
> > 
> > > 
> > > Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> > > ---
> > >   drivers/input/keyboard/mtk-pmic-keys.c | 30 +++++++++++++++++++++++++-
> > >   1 file changed, 29 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/input/keyboard/mtk-pmic-keys.c b/drivers/input/keyboard/mtk-pmic-keys.c
> > > index c31ab4368388..8e4fa7cd16e6 100644
> > > --- a/drivers/input/keyboard/mtk-pmic-keys.c
> > > +++ b/drivers/input/keyboard/mtk-pmic-keys.c
> > > @@ -34,6 +34,13 @@
> > >   #define MTK_PMIC_HOMEKEY_INDEX	1
> > >   #define MTK_PMIC_MAX_KEY_COUNT	2
> > > +/**
> > > + * struct mtk_pmic_keys_regs - PMIC keys per-key registers
> > > + * @deb_reg:             Debounced key status register
> > > + * @deb_mask:            Bitmask of this key in status register
> > > + * @intsel_reg:          Interrupt selector register
> > > + * @intsel_mask:         Bitmask of this key in interrupt selector
> > > + */
> > >   struct mtk_pmic_keys_regs {
> > >   	u32 deb_reg;
> > >   	u32 deb_mask;
> > > @@ -50,6 +57,11 @@ struct mtk_pmic_keys_regs {
> > >   	.intsel_mask		= _intsel_mask,		\
> > >   }
> > > +/**
> > > + * struct mtk_pmic_regs - PMIC Keys registers
> > > + * @keys_regs:           Specific key registers
> > 
> > This new description of the structure and of the keys_regs does not add
> > any information for me.
> > 
> > > + * @pmic_rst_reg:        PMIC Keys reset register
> > > + */
> > >   struct mtk_pmic_regs {
> > >   	const struct mtk_pmic_keys_regs keys_regs[MTK_PMIC_MAX_KEY_COUNT];
> > >   	u32 pmic_rst_reg;
> > > @@ -85,15 +97,31 @@ static const struct mtk_pmic_regs mt6358_regs = {
> > >   	.pmic_rst_reg = MT6358_TOP_RST_MISC,
> > >   };
> > > +/**
> > > + * struct mtk_pmic_keys_info - PMIC Keys per-key params
> > > + * @keys:                Pointer to main driver structure
> > 
> > That is obvious from the field definition.
> > 
> > > + * @regs:                Register offsets/masks for this key
> > 
> > Ditto.
> > 
> > > + * @keycode:             Key code for this key
> > 
> > Yep.
> > 
> > > + * @irq:                 Keypress or press/release interrupt
> > > + * @irq_r:               Key release interrupt (optional)
> > > + * @wakeup:              Indicates whether to use this key as a wakeup source
> > > + */
> > >   struct mtk_pmic_keys_info {
> > >   	struct mtk_pmic_keys *keys;
> > >   	const struct mtk_pmic_keys_regs *regs;
> > >   	unsigned int keycode;
> > >   	int irq;
> > > -	int irq_r; /* optional: release irq if different */
> > > +	int irq_r;
> > >   	bool wakeup:1;
> > >   };
> > > +/**
> > > + * struct mtk_pmic_keys - Main driver structure
> > > + * @input_dev:           Input device pointer
> > 
> > I do not find this helpful.
> > 
> > > + * @dev:                 Device pointer
> > 
> > And neither this.
> > 
> > > + * @regmap:              Regmap handle
> > 
> > Nor this.
> > 
> > > + * @keys:                Per-key parameters
> > > + */
> > >   struct mtk_pmic_keys {
> > >   	struct input_dev *input_dev;
> > >   	struct device *dev;
> > > -- 
> > > 2.35.1
> > > 
> > 
> > In the end we ended up with something that now has a chance of
> > introducing warning when someone changes code, for very little benefit,
> > if any at all.
> > 
> > For driver-private data and functions we should rely on expressive
> > variable and function names and only use comments for something that
> > might be unclear or requires additional qualification.
> > 
> 
> Hello Dmitry,
> 
> it's been very helpful for me to see kerneldoc documentation in the various
> drivers across the kernel - helped me understanding what was going on in an
> easier, more immediate way, especially when looking at drivers having some
> kind of "complicated" flow.
> About introducing warnings when someone changes code, I believe that this
> may also be helpful (for a developer) in some *corner* cases, but I agree
> that this is unnecessarily tedious in some others... in the end, it's all
> about personal opinions...
> 
> Of course, some of the documentation being obvious is unavoidable when it
> comes to kerneldoc as you either document 'em all, or nothing.
> 
> In any case, if you really dislike having this kind of documentation, I can
> drop these commits and eventually add in-line comments to some variables to
> make them perfectly understandable, or I can avoid documenting at all (even
> though I am strongly for documenting things clearly).

I might be mistaken, but I think what you appreciated is not presence of
comments particularly in kerneldoc format (you did not generate htmldocs
or something to study the drivers internal API disconnected from the
code, did you?) but rather the fact that some drivers have been
well-commented.

To be clear, I am all for adding meaningful comments to the drivers
structures and code, but kerneldoc is too rigid and adds too much noise.
As I mentioned "this is a pointer to an input device" description might
be OK when looking at HTML docs on the web or elsewhere, but really does
not add anything when you see "struct input_dev *input_dev;" a few lines
below.  So my preference would be to add free-form comments to places
where intent is not clear. For example the original comment to "irq_r"
was pretty good IMO - it called out some irregularity and explained what
it is. We could definitely add comment to "irq" as well to tell that we
expect it to fire on both press and release.

Thanks.

-- 
Dmitry

Re: [PATCH 2/5] Input: mtk-pmic-keys - Use regmap_{set,clear}_bits where possible

[Dmitry Torokhov]

On Mon, May 23, 2022 at 10:58:47AM +0200, AngeloGioacchino Del Regno wrote:
> Il 23/05/22 06:51, Dmitry Torokhov ha scritto:
> > On Fri, May 20, 2022 at 02:51:29PM +0200, AngeloGioacchino Del Regno wrote:
> > > Instead of always using regmap_update_bits(), let's go for the shorter
> > > regmap_set_bits() and regmap_clear_bits() where possible.
> > > 
> > > No functional change.
> > > 
> > > Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> > > ---
> > >   drivers/input/keyboard/mtk-pmic-keys.c | 24 ++++++------------------
> > >   1 file changed, 6 insertions(+), 18 deletions(-)
> > > 
> > > diff --git a/drivers/input/keyboard/mtk-pmic-keys.c b/drivers/input/keyboard/mtk-pmic-keys.c
> > > index 8e4fa7cd16e6..83d0b90cc8cb 100644
> > > --- a/drivers/input/keyboard/mtk-pmic-keys.c
> > > +++ b/drivers/input/keyboard/mtk-pmic-keys.c
> > > @@ -157,28 +157,16 @@ static void mtk_pmic_keys_lp_reset_setup(struct mtk_pmic_keys *keys,
> > >   	switch (long_press_mode) {
> > >   	case LP_ONEKEY:
> > > -		regmap_update_bits(keys->regmap, pmic_rst_reg,
> > > -				   MTK_PMIC_PWRKEY_RST,
> > > -				   MTK_PMIC_PWRKEY_RST);
> > > -		regmap_update_bits(keys->regmap, pmic_rst_reg,
> > > -				   MTK_PMIC_HOMEKEY_RST,
> > > -				   0);
> > > +		regmap_set_bits(keys->regmap, pmic_rst_reg, MTK_PMIC_PWRKEY_RST);
> > > +		regmap_clear_bits(keys->regmap, pmic_rst_reg, MTK_PMIC_HOMEKEY_RST);
> > 
> > Why not combine this into a single update instead? I.e. assuming
> > 
> 
> All downstream kernels (at least, I checked 4 different kernel versions for 4
> different SoCs) are doing these updates one-at-a-time, never combining them.

It is not like drivers in these downstream kernels were developed
separately and each of them discovered this as a requirement. It was
written once by someone and then either copied as is, or maybe
additional key handling was added later.

> 
> Even though I agree with you about one single update being simply more logical,
> I am afraid that (on some SoCs) the IP will not like that so - since I don't have
> any *clear* documentation saying that this is possible, or that this is not, I
> would leave it like that.

If we go with that we will never be able to touch any of the hardware
interfaces, as I do not recall when spec would explicitly document every
register and call out that individual bits can be changed together in
one write.

Thanks.

-- 
Dmitry