[PATCH v2 0/2] Miscellaneous fixes for Azoteq IQS626A

[PATCH v2 0/2] Miscellaneous fixes for Azoteq IQS626A

[Jeff LaBundy]

This series addresses a fwnode_handle reference count leak. Tightly
coupled with this logic is that behind the discussion in [1], where
it was pointed out that two properties in question have different
types across bindings for the same vendor.

Addressing the latter in fact simplifies the former; therefore both
changes are introduced in patch [1/2]. As there are no known users
of this driver, it is safe to change the binding in this way.

Patch [2/2] makes the corresponding documentation change and carries
a fixes tag so that it accompanies patch [1/2] into stable releases.

[1] https://patchwork.kernel.org/patch/13116327/

Jeff LaBundy (2):
  Input: iqs626a - drop unused device node references
  dt-bindings: input: iqs626a: Redefine trackpad property types

 .../devicetree/bindings/input/iqs626a.yaml    |  94 ++++++++---
 drivers/input/misc/iqs626a.c                  | 156 ++++++++----------
 2 files changed, 140 insertions(+), 110 deletions(-)

-- 
2.34.1

[PATCH v2 1/2] Input: iqs626a - drop unused device node references

[Jeff LaBundy]

Each call to device/fwnode_get_named_child_node() must be matched
with a call to fwnode_handle_put() once the corresponding node is
no longer in use. This ensures a reference count remains balanced
in the case of dynamic device tree support.

Currently, the driver never calls fwnode_handle_put(); this patch
adds the missing calls. Because fwnode_handle_put() does not take
a const *fwnode_handle, the const qualifier is removed across all
corresponding *fwnode_handle instances.

As part of this change, trackpad channel touch thresholds and ATI
base values are now specified under single trackpad channel child
nodes. This enhancement moves both properties to scalar values as
opposed to arrays, making their types consistent across bindings.

Fixes: f1d2809de97a ("Input: Add support for Azoteq IQS626A")
Signed-off-by: Jeff LaBundy <jeff@labundy.com>
---
Changes in v2:
 - Added support for trackpad channel child nodes and removed logic that
   parsed former trackpad touch threshold and ATI base arrays

 drivers/input/misc/iqs626a.c | 156 ++++++++++++++++-------------------
 1 file changed, 73 insertions(+), 83 deletions(-)

diff --git a/drivers/input/misc/iqs626a.c b/drivers/input/misc/iqs626a.c
index 4727e6b95e41..90f997a905b5 100644
--- a/drivers/input/misc/iqs626a.c
+++ b/drivers/input/misc/iqs626a.c
@@ -458,18 +458,15 @@ struct iqs626_private {
 
 static noinline_for_stack int
 iqs626_parse_events(struct iqs626_private *iqs626,
-		    const struct fwnode_handle *ch_node,
-		    enum iqs626_ch_id ch_id)
+		    struct fwnode_handle *ch_node, enum iqs626_ch_id ch_id)
 {
 	struct iqs626_sys_reg *sys_reg = &iqs626->sys_reg;
 	struct i2c_client *client = iqs626->client;
-	const struct fwnode_handle *ev_node;
+	struct fwnode_handle *ev_node;
 	const char *ev_name;
 	u8 *thresh, *hyst;
-	unsigned int thresh_tp[IQS626_NUM_CH_TP_3];
 	unsigned int val;
-	int num_ch = iqs626_channels[ch_id].num_ch;
-	int error, i, j;
+	int i;
 
 	switch (ch_id) {
 	case IQS626_CH_ULP_0:
@@ -509,7 +506,7 @@ iqs626_parse_events(struct iqs626_private *iqs626,
 			 * Trackpad touch events are simply described under the
 			 * trackpad child node.
 			 */
-			ev_node = ch_node;
+			ev_node = fwnode_handle_get(ch_node);
 		} else {
 			ev_name = iqs626_events[i].name;
 			ev_node = fwnode_get_named_child_node(ch_node, ev_name);
@@ -533,6 +530,7 @@ iqs626_parse_events(struct iqs626_private *iqs626,
 					dev_err(&client->dev,
 						"Invalid input type: %u\n",
 						val);
+					fwnode_handle_put(ev_node);
 					return -EINVAL;
 				}
 
@@ -547,6 +545,7 @@ iqs626_parse_events(struct iqs626_private *iqs626,
 				dev_err(&client->dev,
 					"Invalid %s channel hysteresis: %u\n",
 					fwnode_get_name(ch_node), val);
+				fwnode_handle_put(ev_node);
 				return -EINVAL;
 			}
 
@@ -567,6 +566,7 @@ iqs626_parse_events(struct iqs626_private *iqs626,
 				dev_err(&client->dev,
 					"Invalid %s channel threshold: %u\n",
 					fwnode_get_name(ch_node), val);
+				fwnode_handle_put(ev_node);
 				return -EINVAL;
 			}
 
@@ -574,32 +574,9 @@ iqs626_parse_events(struct iqs626_private *iqs626,
 				*thresh = val;
 			else
 				*(thresh + iqs626_events[i].th_offs) = val;
-
-			continue;
-		}
-
-		if (!fwnode_property_present(ev_node, "azoteq,thresh"))
-			continue;
-
-		error = fwnode_property_read_u32_array(ev_node, "azoteq,thresh",
-						       thresh_tp, num_ch);
-		if (error) {
-			dev_err(&client->dev,
-				"Failed to read %s channel thresholds: %d\n",
-				fwnode_get_name(ch_node), error);
-			return error;
 		}
 
-		for (j = 0; j < num_ch; j++) {
-			if (thresh_tp[j] > IQS626_CHx_THRESH_MAX) {
-				dev_err(&client->dev,
-					"Invalid %s channel threshold: %u\n",
-					fwnode_get_name(ch_node), thresh_tp[j]);
-				return -EINVAL;
-			}
-
-			sys_reg->tp_grp_reg.ch_reg_tp[j].thresh = thresh_tp[j];
-		}
+		fwnode_handle_put(ev_node);
 	}
 
 	return 0;
@@ -607,16 +584,13 @@ iqs626_parse_events(struct iqs626_private *iqs626,
 
 static noinline_for_stack int
 iqs626_parse_ati_target(struct iqs626_private *iqs626,
-			const struct fwnode_handle *ch_node,
-			enum iqs626_ch_id ch_id)
+			struct fwnode_handle *ch_node, enum iqs626_ch_id ch_id)
 {
 	struct iqs626_sys_reg *sys_reg = &iqs626->sys_reg;
 	struct i2c_client *client = iqs626->client;
-	unsigned int ati_base[IQS626_NUM_CH_TP_3];
 	unsigned int val;
 	u8 *ati_target;
-	int num_ch = iqs626_channels[ch_id].num_ch;
-	int error, i;
+	int i;
 
 	switch (ch_id) {
 	case IQS626_CH_ULP_0:
@@ -683,40 +657,13 @@ iqs626_parse_ati_target(struct iqs626_private *iqs626,
 
 		*ati_target &= ~IQS626_CHx_ATI_BASE_MASK;
 		*ati_target |= val;
-
-		return 0;
-	}
-
-	if (!fwnode_property_present(ch_node, "azoteq,ati-base"))
-		return 0;
-
-	error = fwnode_property_read_u32_array(ch_node, "azoteq,ati-base",
-					       ati_base, num_ch);
-	if (error) {
-		dev_err(&client->dev,
-			"Failed to read %s channel ATI bases: %d\n",
-			fwnode_get_name(ch_node), error);
-		return error;
-	}
-
-	for (i = 0; i < num_ch; i++) {
-		if (ati_base[i] < IQS626_TPx_ATI_BASE_MIN ||
-		    ati_base[i] > IQS626_TPx_ATI_BASE_MAX) {
-			dev_err(&client->dev,
-				"Invalid %s channel ATI base: %u\n",
-				fwnode_get_name(ch_node), ati_base[i]);
-			return -EINVAL;
-		}
-
-		ati_base[i] -= IQS626_TPx_ATI_BASE_MIN;
-		sys_reg->tp_grp_reg.ch_reg_tp[i].ati_base = ati_base[i];
 	}
 
 	return 0;
 }
 
 static int iqs626_parse_pins(struct iqs626_private *iqs626,
-			     const struct fwnode_handle *ch_node,
+			     struct fwnode_handle *ch_node,
 			     const char *propname, u8 *enable)
 {
 	struct i2c_client *client = iqs626->client;
@@ -764,13 +711,14 @@ static int iqs626_parse_pins(struct iqs626_private *iqs626,
 }
 
 static int iqs626_parse_trackpad(struct iqs626_private *iqs626,
-				 const struct fwnode_handle *ch_node)
+				 struct fwnode_handle *ch_node,
+				 enum iqs626_ch_id ch_id)
 {
 	struct iqs626_sys_reg *sys_reg = &iqs626->sys_reg;
 	struct i2c_client *client = iqs626->client;
 	u8 *hyst = &sys_reg->tp_grp_reg.hyst;
+	int error, count, i;
 	unsigned int val;
-	int error, count;
 
 	if (!fwnode_property_read_u32(ch_node, "azoteq,lta-update", &val)) {
 		if (val > IQS626_MISC_A_TPx_LTA_UPDATE_MAX) {
@@ -823,6 +771,48 @@ static int iqs626_parse_trackpad(struct iqs626_private *iqs626,
 		*hyst |= (val << IQS626_FILT_STR_LP_TPx_SHIFT);
 	}
 
+	for (i = 0; i < iqs626_channels[ch_id].num_ch; i++) {
+		u8 *ati_base = &sys_reg->tp_grp_reg.ch_reg_tp[i].ati_base;
+		u8 *thresh = &sys_reg->tp_grp_reg.ch_reg_tp[i].thresh;
+		struct fwnode_handle *tc_node;
+		char tc_name[10];
+
+		snprintf(tc_name, sizeof(tc_name), "channel-%d", i);
+
+		tc_node = fwnode_get_named_child_node(ch_node, tc_name);
+		if (!tc_node)
+			continue;
+
+		if (!fwnode_property_read_u32(tc_node, "azoteq,ati-base",
+					      &val)) {
+			if (val < IQS626_TPx_ATI_BASE_MIN ||
+			    val > IQS626_TPx_ATI_BASE_MAX) {
+				dev_err(&client->dev,
+					"Invalid %s %s ATI base: %u\n",
+					fwnode_get_name(ch_node), tc_name, val);
+				fwnode_handle_put(tc_node);
+				return -EINVAL;
+			}
+
+			*ati_base = val - IQS626_TPx_ATI_BASE_MIN;
+		}
+
+		if (!fwnode_property_read_u32(tc_node, "azoteq,thresh",
+					      &val)) {
+			if (val > IQS626_CHx_THRESH_MAX) {
+				dev_err(&client->dev,
+					"Invalid %s %s threshold: %u\n",
+					fwnode_get_name(ch_node), tc_name, val);
+				fwnode_handle_put(tc_node);
+				return -EINVAL;
+			}
+
+			*thresh = val;
+		}
+
+		fwnode_handle_put(tc_node);
+	}
+
 	if (!fwnode_property_present(ch_node, "linux,keycodes"))
 		return 0;
 
@@ -889,8 +879,7 @@ static int iqs626_parse_trackpad(struct iqs626_private *iqs626,
 
 static noinline_for_stack int
 iqs626_parse_channel(struct iqs626_private *iqs626,
-		     const struct fwnode_handle *ch_node,
-		     enum iqs626_ch_id ch_id)
+		     struct fwnode_handle *ch_node, enum iqs626_ch_id ch_id)
 {
 	struct iqs626_sys_reg *sys_reg = &iqs626->sys_reg;
 	struct i2c_client *client = iqs626->client;
@@ -924,6 +913,20 @@ iqs626_parse_channel(struct iqs626_private *iqs626,
 		return -EINVAL;
 	}
 
+	error = iqs626_parse_ati_target(iqs626, ch_node, ch_id);
+	if (error)
+		return error;
+
+	error = iqs626_parse_events(iqs626, ch_node, ch_id);
+	if (error)
+		return error;
+
+	if (!fwnode_property_present(ch_node, "azoteq,ati-exclude"))
+		sys_reg->redo_ati |= iqs626_channels[ch_id].active;
+
+	if (!fwnode_property_present(ch_node, "azoteq,reseed-disable"))
+		sys_reg->reseed |= iqs626_channels[ch_id].active;
+
 	*engine |= IQS626_CHx_ENG_0_MEAS_CAP_SIZE;
 	if (fwnode_property_present(ch_node, "azoteq,meas-cap-decrease"))
 		*engine &= ~IQS626_CHx_ENG_0_MEAS_CAP_SIZE;
@@ -1057,7 +1060,7 @@ iqs626_parse_channel(struct iqs626_private *iqs626,
 		*(engine + 1) |= IQS626_CHx_ENG_1_ATI_BAND_TIGHTEN;
 
 	if (ch_id == IQS626_CH_TP_2 || ch_id == IQS626_CH_TP_3)
-		return iqs626_parse_trackpad(iqs626, ch_node);
+		return iqs626_parse_trackpad(iqs626, ch_node, ch_id);
 
 	if (ch_id == IQS626_CH_ULP_0) {
 		sys_reg->ch_reg_ulp.hyst &= ~IQS626_ULP_PROJ_ENABLE;
@@ -1378,23 +1381,10 @@ static int iqs626_parse_prop(struct iqs626_private *iqs626)
 			continue;
 
 		error = iqs626_parse_channel(iqs626, ch_node, i);
+		fwnode_handle_put(ch_node);
 		if (error)
 			return error;
 
-		error = iqs626_parse_ati_target(iqs626, ch_node, i);
-		if (error)
-			return error;
-
-		error = iqs626_parse_events(iqs626, ch_node, i);
-		if (error)
-			return error;
-
-		if (!fwnode_property_present(ch_node, "azoteq,ati-exclude"))
-			sys_reg->redo_ati |= iqs626_channels[i].active;
-
-		if (!fwnode_property_present(ch_node, "azoteq,reseed-disable"))
-			sys_reg->reseed |= iqs626_channels[i].active;
-
 		sys_reg->active |= iqs626_channels[i].active;
 	}
 
-- 
2.34.1

Re: [PATCH v2 1/2] Input: iqs626a - drop unused device node references

[Dmitry Torokhov]

On Fri, Jan 27, 2023 at 04:29:41PM -0600, Jeff LaBundy wrote:
> Each call to device/fwnode_get_named_child_node() must be matched
> with a call to fwnode_handle_put() once the corresponding node is
> no longer in use. This ensures a reference count remains balanced
> in the case of dynamic device tree support.
> 
> Currently, the driver never calls fwnode_handle_put(); this patch
> adds the missing calls. Because fwnode_handle_put() does not take
> a const *fwnode_handle, the const qualifier is removed across all
> corresponding *fwnode_handle instances.
> 
> As part of this change, trackpad channel touch thresholds and ATI
> base values are now specified under single trackpad channel child
> nodes. This enhancement moves both properties to scalar values as
> opposed to arrays, making their types consistent across bindings.
> 
> Fixes: f1d2809de97a ("Input: Add support for Azoteq IQS626A")
> Signed-off-by: Jeff LaBundy <jeff@labundy.com>

Applied, thank you.

-- 
Dmitry

[PATCH v2 2/2] dt-bindings: input: iqs626a: Redefine trackpad property types

[Jeff LaBundy]

Following a recent refactor of the driver to properly drop unused
device nodes, the driver expects trackpad channel touch thresholds
and ATI base values to be specified under single trackpad channel
child nodes.

This enhancement moves both properties to scalar values as opposed
to arrays, making their types consistent across bindings.

Fixes: a8f1f0dc865c ("dt-bindings: input: Add bindings for Azoteq IQS626A")
Signed-off-by: Jeff LaBundy <jeff@labundy.com>
---
Changes in v2:
 - Added to series

 .../devicetree/bindings/input/iqs626a.yaml    | 94 +++++++++++++------
 1 file changed, 67 insertions(+), 27 deletions(-)

diff --git a/Documentation/devicetree/bindings/input/iqs626a.yaml b/Documentation/devicetree/bindings/input/iqs626a.yaml
index 7a27502095f3..e424d67b0542 100644
--- a/Documentation/devicetree/bindings/input/iqs626a.yaml
+++ b/Documentation/devicetree/bindings/input/iqs626a.yaml
@@ -564,16 +564,6 @@ patternProperties:
           2: Partial
           3: Full
 
-      azoteq,ati-base:
-        $ref: /schemas/types.yaml#/definitions/uint32-array
-        minItems: 6
-        maxItems: 9
-        items:
-          minimum: 45
-          maximum: 300
-        default: [45, 45, 45, 45, 45, 45, 45, 45, 45]
-        description: Specifies each individual trackpad channel's ATI base.
-
       azoteq,ati-target:
         $ref: /schemas/types.yaml#/definitions/uint32
         multipleOf: 32
@@ -620,17 +610,6 @@ patternProperties:
         description:
           Tightens the ATI band from 1/8 to 1/16 of the desired target.
 
-      azoteq,thresh:
-        $ref: /schemas/types.yaml#/definitions/uint32-array
-        minItems: 6
-        maxItems: 9
-        items:
-          minimum: 0
-          maximum: 255
-        default: [0, 0, 0, 0, 0, 0, 0, 0, 0]
-        description:
-          Specifies each individual trackpad channel's touch threshold.
-
       azoteq,hyst:
         $ref: /schemas/types.yaml#/definitions/uint32
         minimum: 0
@@ -720,6 +699,28 @@ patternProperties:
           Specifies the number of points across which an axial gesture must
           travel in order to be interpreted as a flick or swipe.
 
+    patternProperties:
+      "^channel-[0-8]$":
+        type: object
+        description: Represents a single trackpad channel.
+
+        properties:
+          azoteq,thresh:
+            $ref: /schemas/types.yaml#/definitions/uint32
+            minimum: 0
+            maximum: 255
+            default: 0
+            description: Specifies the threshold for the channel.
+
+          azoteq,ati-base:
+            $ref: /schemas/types.yaml#/definitions/uint32
+            minimum: 45
+            maximum: 300
+            default: 45
+            description: Specifies the channel's ATI base.
+
+        additionalProperties: false
+
     dependencies:
       azoteq,gesture-swipe: ["linux,keycodes"]
       azoteq,timeout-tap-ms: ["linux,keycodes"]
@@ -780,14 +781,8 @@ examples:
                             azoteq,filt-str-lp-cnt = <1>;
 
                             azoteq,hyst = <4>;
-                            azoteq,thresh = <35>, <40>, <40>,
-                                            <38>, <33>, <38>,
-                                            <35>, <35>, <35>;
 
                             azoteq,ati-mode = <3>;
-                            azoteq,ati-base = <195>, <195>, <195>,
-                                              <195>, <195>, <195>,
-                                              <195>, <195>, <195>;
                             azoteq,ati-target = <512>;
 
                             azoteq,proj-bias = <1>;
@@ -804,6 +799,51 @@ examples:
                             azoteq,timeout-swipe-ms = <800>;
                             azoteq,timeout-tap-ms = <400>;
                             azoteq,thresh-swipe = <40>;
+
+                            channel-0 {
+                                    azoteq,thresh = <35>;
+                                    azoteq,ati-base = <195>;
+                            };
+
+                            channel-1 {
+                                    azoteq,thresh = <40>;
+                                    azoteq,ati-base = <195>;
+                            };
+
+                            channel-2 {
+                                    azoteq,thresh = <40>;
+                                    azoteq,ati-base = <195>;
+                            };
+
+                            channel-3 {
+                                    azoteq,thresh = <38>;
+                                    azoteq,ati-base = <195>;
+                            };
+
+                            channel-4 {
+                                    azoteq,thresh = <33>;
+                                    azoteq,ati-base = <195>;
+                            };
+
+                            channel-5 {
+                                    azoteq,thresh = <38>;
+                                    azoteq,ati-base = <195>;
+                            };
+
+                            channel-6 {
+                                    azoteq,thresh = <35>;
+                                    azoteq,ati-base = <195>;
+                            };
+
+                            channel-7 {
+                                    azoteq,thresh = <35>;
+                                    azoteq,ati-base = <195>;
+                            };
+
+                            channel-8 {
+                                    azoteq,thresh = <35>;
+                                    azoteq,ati-base = <195>;
+                            };
                     };
 
                     /*
-- 
2.34.1

Re: [PATCH v2 2/2] dt-bindings: input: iqs626a: Redefine trackpad property types

[Rob Herring]

On Fri, 27 Jan 2023 16:30:09 -0600, Jeff LaBundy wrote:
> Following a recent refactor of the driver to properly drop unused
> device nodes, the driver expects trackpad channel touch thresholds
> and ATI base values to be specified under single trackpad channel
> child nodes.
> 
> This enhancement moves both properties to scalar values as opposed
> to arrays, making their types consistent across bindings.
> 
> Fixes: a8f1f0dc865c ("dt-bindings: input: Add bindings for Azoteq IQS626A")
> Signed-off-by: Jeff LaBundy <jeff@labundy.com>
> ---
> Changes in v2:
>  - Added to series
> 
>  .../devicetree/bindings/input/iqs626a.yaml    | 94 +++++++++++++------
>  1 file changed, 67 insertions(+), 27 deletions(-)
> 

Reviewed-by: Rob Herring <robh@kernel.org>

Re: [PATCH v2 2/2] dt-bindings: input: iqs626a: Redefine trackpad property types

[Dmitry Torokhov]

On Fri, Jan 27, 2023 at 04:30:09PM -0600, Jeff LaBundy wrote:
> Following a recent refactor of the driver to properly drop unused
> device nodes, the driver expects trackpad channel touch thresholds
> and ATI base values to be specified under single trackpad channel
> child nodes.
> 
> This enhancement moves both properties to scalar values as opposed
> to arrays, making their types consistent across bindings.
> 
> Fixes: a8f1f0dc865c ("dt-bindings: input: Add bindings for Azoteq IQS626A")
> Signed-off-by: Jeff LaBundy <jeff@labundy.com>

Applied, thank you.

-- 
Dmitry