[PATCH v2 0/7] Additional fixes for Azoteq IQS7222A/B/C

[PATCH v2 0/7] Additional fixes for Azoteq IQS7222A/B/C

[Jeff LaBundy]

This series comprises a second round of fixes that result from
continued testing and updated guidance from the vendor.

Jeff LaBundy (7):
  Input: iqs7222 - drop unused device node references
  dt-bindings: input: iqs7222: Reduce 'linux,code' to optional
  Input: iqs7222 - report malformed properties
  dt-bindings: input: iqs7222: Correct minimum slider size
  Input: iqs7222 - protect against undefined slider size
  dt-bindings: input: iqs7222: Add support for IQS7222A v1.13+
  Input: iqs7222 - add support for IQS7222A v1.13+

 .../bindings/input/azoteq,iqs7222.yaml        |  25 +-
 drivers/input/misc/iqs7222.c                  | 489 +++++++++++-------
 2 files changed, 313 insertions(+), 201 deletions(-)

-- 
2.34.1

[PATCH v2 1/7] Input: iqs7222 - 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(). Solve this
problem by moving the node handling from iqs7222_parse_props() to
the new iqs7222_parse_reg_grp(), leaving the former to do nothing
but parse properties. The latter then manages the reference count
in a single location and consistent fashion.

This change drastically simplifies iqs7222_parse_all(), which can
then call iqs7222_parse_reg_grp() on every register group without
having to treat each register group differently.

For nested event nodes, common parsing code has been factored out
to the new iqs7222_parse_event() so as to allow the event node to
be dropped from as few locations as possible.

As part of this refactor, the 'linux,code' property has been made
optional. This enables applications that define an event with the
sole purpose of enabling a GPIO.

Fixes: e505edaedcb9 ("Input: add support for Azoteq IQS7222A/B/C")
Signed-off-by: Jeff LaBundy <jeff@labundy.com>
---
Changes in v2:
 - Created new iqs7222_parse_reg_grp() and iqs7222_parse_event() helpers
   to prevent multiple goto statements and calls to fwnode_handle_put()
 - Updated commit message

 drivers/input/misc/iqs7222.c | 326 ++++++++++++++++-------------------
 1 file changed, 149 insertions(+), 177 deletions(-)

diff --git a/drivers/input/misc/iqs7222.c b/drivers/input/misc/iqs7222.c
index ddb863bf63ee..d39b3fdfb849 100644
--- a/drivers/input/misc/iqs7222.c
+++ b/drivers/input/misc/iqs7222.c
@@ -105,14 +105,14 @@ enum iqs7222_reg_grp_id {
 	IQS7222_NUM_REG_GRPS
 };
 
-static const char * const iqs7222_reg_grp_names[] = {
+static const char * const iqs7222_reg_grp_names[IQS7222_NUM_REG_GRPS] = {
 	[IQS7222_REG_GRP_CYCLE] = "cycle",
 	[IQS7222_REG_GRP_CHAN] = "channel",
 	[IQS7222_REG_GRP_SLDR] = "slider",
 	[IQS7222_REG_GRP_GPIO] = "gpio",
 };
 
-static const unsigned int iqs7222_max_cols[] = {
+static const unsigned int iqs7222_max_cols[IQS7222_NUM_REG_GRPS] = {
 	[IQS7222_REG_GRP_STAT] = IQS7222_MAX_COLS_STAT,
 	[IQS7222_REG_GRP_CYCLE] = IQS7222_MAX_COLS_CYCLE,
 	[IQS7222_REG_GRP_GLBL] = IQS7222_MAX_COLS_GLBL,
@@ -1567,56 +1567,17 @@ static int iqs7222_gpio_select(struct iqs7222_private *iqs7222,
 }
 
 static int iqs7222_parse_props(struct iqs7222_private *iqs7222,
-			       struct fwnode_handle **child_node,
-			       int child_index,
+			       struct fwnode_handle *reg_grp_node,
+			       int reg_grp_index,
 			       enum iqs7222_reg_grp_id reg_grp,
 			       enum iqs7222_reg_key_id reg_key)
 {
-	u16 *setup = iqs7222_setup(iqs7222, reg_grp, child_index);
+	u16 *setup = iqs7222_setup(iqs7222, reg_grp, reg_grp_index);
 	struct i2c_client *client = iqs7222->client;
-	struct fwnode_handle *reg_grp_node;
-	char reg_grp_name[16];
 	int i;
 
-	switch (reg_grp) {
-	case IQS7222_REG_GRP_CYCLE:
-	case IQS7222_REG_GRP_CHAN:
-	case IQS7222_REG_GRP_SLDR:
-	case IQS7222_REG_GRP_GPIO:
-	case IQS7222_REG_GRP_BTN:
-		/*
-		 * These groups derive a child node and return it to the caller
-		 * for additional group-specific processing. In some cases, the
-		 * child node may have already been derived.
-		 */
-		reg_grp_node = *child_node;
-		if (reg_grp_node)
-			break;
-
-		snprintf(reg_grp_name, sizeof(reg_grp_name), "%s-%d",
-			 iqs7222_reg_grp_names[reg_grp], child_index);
-
-		reg_grp_node = device_get_named_child_node(&client->dev,
-							   reg_grp_name);
-		if (!reg_grp_node)
-			return 0;
-
-		*child_node = reg_grp_node;
-		break;
-
-	case IQS7222_REG_GRP_GLBL:
-	case IQS7222_REG_GRP_FILT:
-	case IQS7222_REG_GRP_SYS:
-		/*
-		 * These groups are not organized beneath a child node, nor are
-		 * they subject to any additional processing by the caller.
-		 */
-		reg_grp_node = dev_fwnode(&client->dev);
-		break;
-
-	default:
-		return -EINVAL;
-	}
+	if (!setup)
+		return 0;
 
 	for (i = 0; i < ARRAY_SIZE(iqs7222_props); i++) {
 		const char *name = iqs7222_props[i].name;
@@ -1686,11 +1647,66 @@ static int iqs7222_parse_props(struct iqs7222_private *iqs7222,
 	return 0;
 }
 
-static int iqs7222_parse_cycle(struct iqs7222_private *iqs7222, int cycle_index)
+static int iqs7222_parse_event(struct iqs7222_private *iqs7222,
+			       struct fwnode_handle *event_node,
+			       int reg_grp_index,
+			       enum iqs7222_reg_grp_id reg_grp,
+			       enum iqs7222_reg_key_id reg_key,
+			       u16 event_enable, u16 event_link,
+			       unsigned int *event_type,
+			       unsigned int *event_code)
+{
+	struct i2c_client *client = iqs7222->client;
+	int error;
+
+	error = iqs7222_parse_props(iqs7222, event_node, reg_grp_index,
+				    reg_grp, reg_key);
+	if (error)
+		return error;
+
+	error = iqs7222_gpio_select(iqs7222, event_node, event_enable,
+				    event_link);
+	if (error)
+		return error;
+
+	error = fwnode_property_read_u32(event_node, "linux,code", event_code);
+	if (error == -EINVAL) {
+		return 0;
+	} else if (error) {
+		dev_err(&client->dev, "Failed to read %s code: %d\n",
+			fwnode_get_name(event_node), error);
+		return error;
+	}
+
+	if (!event_type) {
+		input_set_capability(iqs7222->keypad, EV_KEY, *event_code);
+		return 0;
+	}
+
+	error = fwnode_property_read_u32(event_node, "linux,input-type",
+					 event_type);
+	if (error == -EINVAL) {
+		*event_type = EV_KEY;
+	} else if (error) {
+		dev_err(&client->dev, "Failed to read %s input type: %d\n",
+			fwnode_get_name(event_node), error);
+		return error;
+	} else if (*event_type != EV_KEY && *event_type != EV_SW) {
+		dev_err(&client->dev, "Invalid %s input type: %d\n",
+			fwnode_get_name(event_node), *event_type);
+		return -EINVAL;
+	}
+
+	input_set_capability(iqs7222->keypad, *event_type, *event_code);
+
+	return 0;
+}
+
+static int iqs7222_parse_cycle(struct iqs7222_private *iqs7222,
+			       struct fwnode_handle *cycle_node, int cycle_index)
 {
 	u16 *cycle_setup = iqs7222->cycle_setup[cycle_index];
 	struct i2c_client *client = iqs7222->client;
-	struct fwnode_handle *cycle_node = NULL;
 	unsigned int pins[9];
 	int error, count, i;
 
@@ -1699,15 +1715,12 @@ static int iqs7222_parse_cycle(struct iqs7222_private *iqs7222, int cycle_index)
 	 * channels to cycles is fixed. Properties defined for a cycle impact
 	 * both channels tied to the cycle.
 	 */
-	error = iqs7222_parse_props(iqs7222, &cycle_node, cycle_index,
+	error = iqs7222_parse_props(iqs7222, cycle_node, cycle_index,
 				    IQS7222_REG_GRP_CYCLE,
 				    IQS7222_REG_KEY_NONE);
 	if (error)
 		return error;
 
-	if (!cycle_node)
-		return 0;
-
 	/*
 	 * Unlike channels which are restricted to a select range of CRx pins
 	 * based on channel number, any cycle can claim any of the device's 9
@@ -1750,11 +1763,11 @@ static int iqs7222_parse_cycle(struct iqs7222_private *iqs7222, int cycle_index)
 	return 0;
 }
 
-static int iqs7222_parse_chan(struct iqs7222_private *iqs7222, int chan_index)
+static int iqs7222_parse_chan(struct iqs7222_private *iqs7222,
+			      struct fwnode_handle *chan_node, int chan_index)
 {
 	const struct iqs7222_dev_desc *dev_desc = iqs7222->dev_desc;
 	struct i2c_client *client = iqs7222->client;
-	struct fwnode_handle *chan_node = NULL;
 	int num_chan = dev_desc->reg_grps[IQS7222_REG_GRP_CHAN].num_row;
 	int ext_chan = rounddown(num_chan, 10);
 	int error, i;
@@ -1762,15 +1775,12 @@ static int iqs7222_parse_chan(struct iqs7222_private *iqs7222, int chan_index)
 	u16 *sys_setup = iqs7222->sys_setup;
 	unsigned int val;
 
-	error = iqs7222_parse_props(iqs7222, &chan_node, chan_index,
+	error = iqs7222_parse_props(iqs7222, chan_node, chan_index,
 				    IQS7222_REG_GRP_CHAN,
 				    IQS7222_REG_KEY_NONE);
 	if (error)
 		return error;
 
-	if (!chan_node)
-		return 0;
-
 	if (dev_desc->allow_offset &&
 	    fwnode_property_present(chan_node, "azoteq,ulp-allow"))
 		sys_setup[dev_desc->allow_offset] &= ~BIT(chan_index);
@@ -1892,18 +1902,6 @@ static int iqs7222_parse_chan(struct iqs7222_private *iqs7222, int chan_index)
 		if (!event_node)
 			continue;
 
-		error = iqs7222_parse_props(iqs7222, &event_node, chan_index,
-					    IQS7222_REG_GRP_BTN,
-					    iqs7222_kp_events[i].reg_key);
-		if (error)
-			return error;
-
-		error = iqs7222_gpio_select(iqs7222, event_node,
-					    BIT(chan_index),
-					    dev_desc->touch_link - (i ? 0 : 2));
-		if (error)
-			return error;
-
 		if (!fwnode_property_read_u32(event_node,
 					      "azoteq,timeout-press-ms",
 					      &val)) {
@@ -1919,7 +1917,8 @@ static int iqs7222_parse_chan(struct iqs7222_private *iqs7222, int chan_index)
 			if (val > U8_MAX * 500) {
 				dev_err(&client->dev,
 					"Invalid %s press timeout: %u\n",
-					fwnode_get_name(chan_node), val);
+					fwnode_get_name(event_node), val);
+				fwnode_handle_put(event_node);
 				return -EINVAL;
 			}
 
@@ -1927,49 +1926,16 @@ static int iqs7222_parse_chan(struct iqs7222_private *iqs7222, int chan_index)
 			*setup |= (val / 500 << i * 8);
 		}
 
-		error = fwnode_property_read_u32(event_node, "linux,code",
-						 &val);
-		if (error) {
-			dev_err(&client->dev, "Failed to read %s code: %d\n",
-				fwnode_get_name(chan_node), error);
+		error = iqs7222_parse_event(iqs7222, event_node, chan_index,
+					    IQS7222_REG_GRP_BTN,
+					    iqs7222_kp_events[i].reg_key,
+					    BIT(chan_index),
+					    dev_desc->touch_link - (i ? 0 : 2),
+					    &iqs7222->kp_type[chan_index][i],
+					    &iqs7222->kp_code[chan_index][i]);
+		fwnode_handle_put(event_node);
+		if (error)
 			return error;
-		}
-
-		iqs7222->kp_code[chan_index][i] = val;
-		iqs7222->kp_type[chan_index][i] = EV_KEY;
-
-		if (fwnode_property_present(event_node, "linux,input-type")) {
-			error = fwnode_property_read_u32(event_node,
-							 "linux,input-type",
-							 &val);
-			if (error) {
-				dev_err(&client->dev,
-					"Failed to read %s input type: %d\n",
-					fwnode_get_name(chan_node), error);
-				return error;
-			}
-
-			if (val != EV_KEY && val != EV_SW) {
-				dev_err(&client->dev,
-					"Invalid %s input type: %u\n",
-					fwnode_get_name(chan_node), val);
-				return -EINVAL;
-			}
-
-			iqs7222->kp_type[chan_index][i] = val;
-		}
-
-		/*
-		 * Reference channels can opt out of event reporting by using
-		 * KEY_RESERVED in place of a true key or switch code.
-		 */
-		if (iqs7222->kp_type[chan_index][i] == EV_KEY &&
-		    iqs7222->kp_code[chan_index][i] == KEY_RESERVED)
-			continue;
-
-		input_set_capability(iqs7222->keypad,
-				     iqs7222->kp_type[chan_index][i],
-				     iqs7222->kp_code[chan_index][i]);
 
 		if (!dev_desc->event_offset)
 			continue;
@@ -1981,16 +1947,16 @@ static int iqs7222_parse_chan(struct iqs7222_private *iqs7222, int chan_index)
 	 * The following call handles a special pair of properties that apply
 	 * to a channel node, but reside within the button (event) group.
 	 */
-	return iqs7222_parse_props(iqs7222, &chan_node, chan_index,
+	return iqs7222_parse_props(iqs7222, chan_node, chan_index,
 				   IQS7222_REG_GRP_BTN,
 				   IQS7222_REG_KEY_DEBOUNCE);
 }
 
-static int iqs7222_parse_sldr(struct iqs7222_private *iqs7222, int sldr_index)
+static int iqs7222_parse_sldr(struct iqs7222_private *iqs7222,
+			      struct fwnode_handle *sldr_node, int sldr_index)
 {
 	const struct iqs7222_dev_desc *dev_desc = iqs7222->dev_desc;
 	struct i2c_client *client = iqs7222->client;
-	struct fwnode_handle *sldr_node = NULL;
 	int num_chan = dev_desc->reg_grps[IQS7222_REG_GRP_CHAN].num_row;
 	int ext_chan = rounddown(num_chan, 10);
 	int count, error, reg_offset, i;
@@ -1998,15 +1964,12 @@ static int iqs7222_parse_sldr(struct iqs7222_private *iqs7222, int sldr_index)
 	u16 *sldr_setup = iqs7222->sldr_setup[sldr_index];
 	unsigned int chan_sel[4], val;
 
-	error = iqs7222_parse_props(iqs7222, &sldr_node, sldr_index,
+	error = iqs7222_parse_props(iqs7222, sldr_node, sldr_index,
 				    IQS7222_REG_GRP_SLDR,
 				    IQS7222_REG_KEY_NONE);
 	if (error)
 		return error;
 
-	if (!sldr_node)
-		return 0;
-
 	/*
 	 * Each slider can be spread across 3 to 4 channels. It is possible to
 	 * select only 2 channels, but doing so prevents the slider from using
@@ -2130,46 +2093,37 @@ static int iqs7222_parse_sldr(struct iqs7222_private *iqs7222, int sldr_index)
 	for (i = 0; i < ARRAY_SIZE(iqs7222_sl_events); i++) {
 		const char *event_name = iqs7222_sl_events[i].name;
 		struct fwnode_handle *event_node;
+		enum iqs7222_reg_key_id reg_key;
 
 		event_node = fwnode_get_named_child_node(sldr_node, event_name);
 		if (!event_node)
 			continue;
 
-		error = iqs7222_parse_props(iqs7222, &event_node, sldr_index,
-					    IQS7222_REG_GRP_SLDR,
-					    reg_offset ?
-					    IQS7222_REG_KEY_RESERVED :
-					    iqs7222_sl_events[i].reg_key);
-		if (error)
-			return error;
+		if (reg_offset)
+			reg_key = IQS7222_REG_KEY_RESERVED;
+		else
+			reg_key = iqs7222_sl_events[i].reg_key;
 
 		/*
 		 * The press/release event does not expose a direct GPIO link,
 		 * but one can be emulated by tying each of the participating
 		 * channels to the same GPIO.
 		 */
-		error = iqs7222_gpio_select(iqs7222, event_node,
+		error = iqs7222_parse_event(iqs7222, event_node, sldr_index,
+					    IQS7222_REG_GRP_SLDR, reg_key,
 					    i ? iqs7222_sl_events[i].enable
 					      : sldr_setup[3 + reg_offset],
 					    i ? 1568 + sldr_index * 30
-					      : sldr_setup[4 + reg_offset]);
+					      : sldr_setup[4 + reg_offset],
+					    NULL,
+					    &iqs7222->sl_code[sldr_index][i]);
+		fwnode_handle_put(event_node);
 		if (error)
 			return error;
 
 		if (!reg_offset)
 			sldr_setup[9] |= iqs7222_sl_events[i].enable;
 
-		error = fwnode_property_read_u32(event_node, "linux,code",
-						 &val);
-		if (error) {
-			dev_err(&client->dev, "Failed to read %s code: %d\n",
-				fwnode_get_name(sldr_node), error);
-			return error;
-		}
-
-		iqs7222->sl_code[sldr_index][i] = val;
-		input_set_capability(iqs7222->keypad, EV_KEY, val);
-
 		if (!dev_desc->event_offset)
 			continue;
 
@@ -2190,19 +2144,64 @@ static int iqs7222_parse_sldr(struct iqs7222_private *iqs7222, int sldr_index)
 	 * The following call handles a special pair of properties that shift
 	 * to make room for a wheel enable control in the case of IQS7222C.
 	 */
-	return iqs7222_parse_props(iqs7222, &sldr_node, sldr_index,
+	return iqs7222_parse_props(iqs7222, sldr_node, sldr_index,
 				   IQS7222_REG_GRP_SLDR,
 				   dev_desc->wheel_enable ?
 				   IQS7222_REG_KEY_WHEEL :
 				   IQS7222_REG_KEY_NO_WHEEL);
 }
 
+static int (*iqs7222_parse_extra[IQS7222_NUM_REG_GRPS])
+				(struct iqs7222_private *iqs7222,
+				 struct fwnode_handle *reg_grp_node,
+				 int reg_grp_index) = {
+	[IQS7222_REG_GRP_CYCLE] = iqs7222_parse_cycle,
+	[IQS7222_REG_GRP_CHAN] = iqs7222_parse_chan,
+	[IQS7222_REG_GRP_SLDR] = iqs7222_parse_sldr,
+};
+
+static int iqs7222_parse_reg_grp(struct iqs7222_private *iqs7222,
+				 enum iqs7222_reg_grp_id reg_grp,
+				 int reg_grp_index)
+{
+	struct i2c_client *client = iqs7222->client;
+	struct fwnode_handle *reg_grp_node;
+	int error;
+
+	if (iqs7222_reg_grp_names[reg_grp]) {
+		char reg_grp_name[16];
+
+		snprintf(reg_grp_name, sizeof(reg_grp_name), "%s-%d",
+			 iqs7222_reg_grp_names[reg_grp], reg_grp_index);
+
+		reg_grp_node = device_get_named_child_node(&client->dev,
+							   reg_grp_name);
+	} else {
+		reg_grp_node = fwnode_handle_get(dev_fwnode(&client->dev));
+	}
+
+	if (!reg_grp_node)
+		return 0;
+
+	if (iqs7222_parse_extra[reg_grp])
+		error = iqs7222_parse_extra[reg_grp](iqs7222, reg_grp_node,
+						     reg_grp_index);
+	else
+		error = iqs7222_parse_props(iqs7222, reg_grp_node,
+					    reg_grp_index,
+					    reg_grp, IQS7222_REG_KEY_NONE);
+
+	fwnode_handle_put(reg_grp_node);
+
+	return error;
+}
+
 static int iqs7222_parse_all(struct iqs7222_private *iqs7222)
 {
 	const struct iqs7222_dev_desc *dev_desc = iqs7222->dev_desc;
 	const struct iqs7222_reg_grp_desc *reg_grps = dev_desc->reg_grps;
 	u16 *sys_setup = iqs7222->sys_setup;
-	int error, i;
+	int error, i, j;
 
 	if (dev_desc->allow_offset)
 		sys_setup[dev_desc->allow_offset] = U16_MAX;
@@ -2210,32 +2209,13 @@ static int iqs7222_parse_all(struct iqs7222_private *iqs7222)
 	if (dev_desc->event_offset)
 		sys_setup[dev_desc->event_offset] = IQS7222_EVENT_MASK_ATI;
 
-	for (i = 0; i < reg_grps[IQS7222_REG_GRP_CYCLE].num_row; i++) {
-		error = iqs7222_parse_cycle(iqs7222, i);
-		if (error)
-			return error;
-	}
-
-	error = iqs7222_parse_props(iqs7222, NULL, 0, IQS7222_REG_GRP_GLBL,
-				    IQS7222_REG_KEY_NONE);
-	if (error)
-		return error;
-
 	for (i = 0; i < reg_grps[IQS7222_REG_GRP_GPIO].num_row; i++) {
-		struct fwnode_handle *gpio_node = NULL;
 		u16 *gpio_setup = iqs7222->gpio_setup[i];
-		int j;
 
 		gpio_setup[0] &= ~IQS7222_GPIO_SETUP_0_GPIO_EN;
 		gpio_setup[1] = 0;
 		gpio_setup[2] = 0;
 
-		error = iqs7222_parse_props(iqs7222, &gpio_node, i,
-					    IQS7222_REG_GRP_GPIO,
-					    IQS7222_REG_KEY_NONE);
-		if (error)
-			return error;
-
 		if (reg_grps[IQS7222_REG_GRP_GPIO].num_row == 1)
 			continue;
 
@@ -2258,29 +2238,21 @@ static int iqs7222_parse_all(struct iqs7222_private *iqs7222)
 		chan_setup[5] = 0;
 	}
 
-	for (i = 0; i < reg_grps[IQS7222_REG_GRP_CHAN].num_row; i++) {
-		error = iqs7222_parse_chan(iqs7222, i);
-		if (error)
-			return error;
-	}
-
-	error = iqs7222_parse_props(iqs7222, NULL, 0, IQS7222_REG_GRP_FILT,
-				    IQS7222_REG_KEY_NONE);
-	if (error)
-		return error;
-
 	for (i = 0; i < reg_grps[IQS7222_REG_GRP_SLDR].num_row; i++) {
 		u16 *sldr_setup = iqs7222->sldr_setup[i];
 
 		sldr_setup[0] &= ~IQS7222_SLDR_SETUP_0_CHAN_CNT_MASK;
+	}
 
-		error = iqs7222_parse_sldr(iqs7222, i);
-		if (error)
-			return error;
+	for (i = 0; i < IQS7222_NUM_REG_GRPS; i++) {
+		for (j = 0; j < reg_grps[i].num_row; j++) {
+			error = iqs7222_parse_reg_grp(iqs7222, i, j);
+			if (error)
+				return error;
+		}
 	}
 
-	return iqs7222_parse_props(iqs7222, NULL, 0, IQS7222_REG_GRP_SYS,
-				   IQS7222_REG_KEY_NONE);
+	return 0;
 }
 
 static int iqs7222_report(struct iqs7222_private *iqs7222)
-- 
2.34.1

Re: [PATCH v2 1/7] Input: iqs7222 - drop unused device node references

[Mattijs Korpershoek]

On Thu, Sep 15, 2022 at 23:30, Jeff LaBundy <jeff@labundy.com> 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(). Solve this
> problem by moving the node handling from iqs7222_parse_props() to
> the new iqs7222_parse_reg_grp(), leaving the former to do nothing
> but parse properties. The latter then manages the reference count
> in a single location and consistent fashion.
>
> This change drastically simplifies iqs7222_parse_all(), which can
> then call iqs7222_parse_reg_grp() on every register group without
> having to treat each register group differently.
>
> For nested event nodes, common parsing code has been factored out
> to the new iqs7222_parse_event() so as to allow the event node to
> be dropped from as few locations as possible.
>
> As part of this refactor, the 'linux,code' property has been made
> optional. This enables applications that define an event with the
> sole purpose of enabling a GPIO.
>
> Fixes: e505edaedcb9 ("Input: add support for Azoteq IQS7222A/B/C")
> Signed-off-by: Jeff LaBundy <jeff@labundy.com>
> ---
> Changes in v2:
>  - Created new iqs7222_parse_reg_grp() and iqs7222_parse_event() helpers
>    to prevent multiple goto statements and calls to fwnode_handle_put()
>  - Updated commit message
>
>  drivers/input/misc/iqs7222.c | 326 ++++++++++++++++-------------------
>  1 file changed, 149 insertions(+), 177 deletions(-)

Hi Jeff,

This is a pretty big change. It was difficult to review so I hope I did
not miss anything.

Reviewed-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>

>
> diff --git a/drivers/input/misc/iqs7222.c b/drivers/input/misc/iqs7222.c
> index ddb863bf63ee..d39b3fdfb849 100644
> --- a/drivers/input/misc/iqs7222.c
> +++ b/drivers/input/misc/iqs7222.c
> @@ -105,14 +105,14 @@ enum iqs7222_reg_grp_id {
>  	IQS7222_NUM_REG_GRPS
>  };
>  
> -static const char * const iqs7222_reg_grp_names[] = {
> +static const char * const iqs7222_reg_grp_names[IQS7222_NUM_REG_GRPS] = {
>  	[IQS7222_REG_GRP_CYCLE] = "cycle",
>  	[IQS7222_REG_GRP_CHAN] = "channel",
>  	[IQS7222_REG_GRP_SLDR] = "slider",
>  	[IQS7222_REG_GRP_GPIO] = "gpio",
>  };
>  
> -static const unsigned int iqs7222_max_cols[] = {
> +static const unsigned int iqs7222_max_cols[IQS7222_NUM_REG_GRPS] = {
>  	[IQS7222_REG_GRP_STAT] = IQS7222_MAX_COLS_STAT,
>  	[IQS7222_REG_GRP_CYCLE] = IQS7222_MAX_COLS_CYCLE,
>  	[IQS7222_REG_GRP_GLBL] = IQS7222_MAX_COLS_GLBL,
> @@ -1567,56 +1567,17 @@ static int iqs7222_gpio_select(struct iqs7222_private *iqs7222,
>  }
>  
>  static int iqs7222_parse_props(struct iqs7222_private *iqs7222,
> -			       struct fwnode_handle **child_node,
> -			       int child_index,
> +			       struct fwnode_handle *reg_grp_node,
> +			       int reg_grp_index,
>  			       enum iqs7222_reg_grp_id reg_grp,
>  			       enum iqs7222_reg_key_id reg_key)
>  {
> -	u16 *setup = iqs7222_setup(iqs7222, reg_grp, child_index);
> +	u16 *setup = iqs7222_setup(iqs7222, reg_grp, reg_grp_index);
>  	struct i2c_client *client = iqs7222->client;
> -	struct fwnode_handle *reg_grp_node;
> -	char reg_grp_name[16];
>  	int i;
>  
> -	switch (reg_grp) {
> -	case IQS7222_REG_GRP_CYCLE:
> -	case IQS7222_REG_GRP_CHAN:
> -	case IQS7222_REG_GRP_SLDR:
> -	case IQS7222_REG_GRP_GPIO:
> -	case IQS7222_REG_GRP_BTN:
> -		/*
> -		 * These groups derive a child node and return it to the caller
> -		 * for additional group-specific processing. In some cases, the
> -		 * child node may have already been derived.
> -		 */
> -		reg_grp_node = *child_node;
> -		if (reg_grp_node)
> -			break;
> -
> -		snprintf(reg_grp_name, sizeof(reg_grp_name), "%s-%d",
> -			 iqs7222_reg_grp_names[reg_grp], child_index);
> -
> -		reg_grp_node = device_get_named_child_node(&client->dev,
> -							   reg_grp_name);
> -		if (!reg_grp_node)
> -			return 0;
> -
> -		*child_node = reg_grp_node;
> -		break;
> -
> -	case IQS7222_REG_GRP_GLBL:
> -	case IQS7222_REG_GRP_FILT:
> -	case IQS7222_REG_GRP_SYS:
> -		/*
> -		 * These groups are not organized beneath a child node, nor are
> -		 * they subject to any additional processing by the caller.
> -		 */
> -		reg_grp_node = dev_fwnode(&client->dev);
> -		break;
> -
> -	default:
> -		return -EINVAL;
> -	}
> +	if (!setup)
> +		return 0;
>  
>  	for (i = 0; i < ARRAY_SIZE(iqs7222_props); i++) {
>  		const char *name = iqs7222_props[i].name;
> @@ -1686,11 +1647,66 @@ static int iqs7222_parse_props(struct iqs7222_private *iqs7222,
>  	return 0;
>  }
>  
> -static int iqs7222_parse_cycle(struct iqs7222_private *iqs7222, int cycle_index)
> +static int iqs7222_parse_event(struct iqs7222_private *iqs7222,
> +			       struct fwnode_handle *event_node,
> +			       int reg_grp_index,
> +			       enum iqs7222_reg_grp_id reg_grp,
> +			       enum iqs7222_reg_key_id reg_key,
> +			       u16 event_enable, u16 event_link,
> +			       unsigned int *event_type,
> +			       unsigned int *event_code)
> +{
> +	struct i2c_client *client = iqs7222->client;
> +	int error;
> +
> +	error = iqs7222_parse_props(iqs7222, event_node, reg_grp_index,
> +				    reg_grp, reg_key);
> +	if (error)
> +		return error;
> +
> +	error = iqs7222_gpio_select(iqs7222, event_node, event_enable,
> +				    event_link);
> +	if (error)
> +		return error;
> +
> +	error = fwnode_property_read_u32(event_node, "linux,code", event_code);
> +	if (error == -EINVAL) {
> +		return 0;
> +	} else if (error) {
> +		dev_err(&client->dev, "Failed to read %s code: %d\n",
> +			fwnode_get_name(event_node), error);
> +		return error;
> +	}
> +
> +	if (!event_type) {
> +		input_set_capability(iqs7222->keypad, EV_KEY, *event_code);
> +		return 0;
> +	}
> +
> +	error = fwnode_property_read_u32(event_node, "linux,input-type",
> +					 event_type);
> +	if (error == -EINVAL) {
> +		*event_type = EV_KEY;
> +	} else if (error) {
> +		dev_err(&client->dev, "Failed to read %s input type: %d\n",
> +			fwnode_get_name(event_node), error);
> +		return error;
> +	} else if (*event_type != EV_KEY && *event_type != EV_SW) {
> +		dev_err(&client->dev, "Invalid %s input type: %d\n",
> +			fwnode_get_name(event_node), *event_type);
> +		return -EINVAL;
> +	}
> +
> +	input_set_capability(iqs7222->keypad, *event_type, *event_code);
> +
> +	return 0;
> +}
> +
> +static int iqs7222_parse_cycle(struct iqs7222_private *iqs7222,
> +			       struct fwnode_handle *cycle_node, int cycle_index)
>  {
>  	u16 *cycle_setup = iqs7222->cycle_setup[cycle_index];
>  	struct i2c_client *client = iqs7222->client;
> -	struct fwnode_handle *cycle_node = NULL;
>  	unsigned int pins[9];
>  	int error, count, i;
>  
> @@ -1699,15 +1715,12 @@ static int iqs7222_parse_cycle(struct iqs7222_private *iqs7222, int cycle_index)
>  	 * channels to cycles is fixed. Properties defined for a cycle impact
>  	 * both channels tied to the cycle.
>  	 */
> -	error = iqs7222_parse_props(iqs7222, &cycle_node, cycle_index,
> +	error = iqs7222_parse_props(iqs7222, cycle_node, cycle_index,
>  				    IQS7222_REG_GRP_CYCLE,
>  				    IQS7222_REG_KEY_NONE);
>  	if (error)
>  		return error;
>  
> -	if (!cycle_node)
> -		return 0;
> -
>  	/*
>  	 * Unlike channels which are restricted to a select range of CRx pins
>  	 * based on channel number, any cycle can claim any of the device's 9
> @@ -1750,11 +1763,11 @@ static int iqs7222_parse_cycle(struct iqs7222_private *iqs7222, int cycle_index)
>  	return 0;
>  }
>  
> -static int iqs7222_parse_chan(struct iqs7222_private *iqs7222, int chan_index)
> +static int iqs7222_parse_chan(struct iqs7222_private *iqs7222,
> +			      struct fwnode_handle *chan_node, int chan_index)
>  {
>  	const struct iqs7222_dev_desc *dev_desc = iqs7222->dev_desc;
>  	struct i2c_client *client = iqs7222->client;
> -	struct fwnode_handle *chan_node = NULL;
>  	int num_chan = dev_desc->reg_grps[IQS7222_REG_GRP_CHAN].num_row;
>  	int ext_chan = rounddown(num_chan, 10);
>  	int error, i;
> @@ -1762,15 +1775,12 @@ static int iqs7222_parse_chan(struct iqs7222_private *iqs7222, int chan_index)
>  	u16 *sys_setup = iqs7222->sys_setup;
>  	unsigned int val;
>  
> -	error = iqs7222_parse_props(iqs7222, &chan_node, chan_index,
> +	error = iqs7222_parse_props(iqs7222, chan_node, chan_index,
>  				    IQS7222_REG_GRP_CHAN,
>  				    IQS7222_REG_KEY_NONE);
>  	if (error)
>  		return error;
>  
> -	if (!chan_node)
> -		return 0;
> -
>  	if (dev_desc->allow_offset &&
>  	    fwnode_property_present(chan_node, "azoteq,ulp-allow"))
>  		sys_setup[dev_desc->allow_offset] &= ~BIT(chan_index);
> @@ -1892,18 +1902,6 @@ static int iqs7222_parse_chan(struct iqs7222_private *iqs7222, int chan_index)
>  		if (!event_node)
>  			continue;
>  
> -		error = iqs7222_parse_props(iqs7222, &event_node, chan_index,
> -					    IQS7222_REG_GRP_BTN,
> -					    iqs7222_kp_events[i].reg_key);
> -		if (error)
> -			return error;
> -
> -		error = iqs7222_gpio_select(iqs7222, event_node,
> -					    BIT(chan_index),
> -					    dev_desc->touch_link - (i ? 0 : 2));
> -		if (error)
> -			return error;
> -
>  		if (!fwnode_property_read_u32(event_node,
>  					      "azoteq,timeout-press-ms",
>  					      &val)) {
> @@ -1919,7 +1917,8 @@ static int iqs7222_parse_chan(struct iqs7222_private *iqs7222, int chan_index)
>  			if (val > U8_MAX * 500) {
>  				dev_err(&client->dev,
>  					"Invalid %s press timeout: %u\n",
> -					fwnode_get_name(chan_node), val);
> +					fwnode_get_name(event_node), val);
> +				fwnode_handle_put(event_node);
>  				return -EINVAL;
>  			}
>  
> @@ -1927,49 +1926,16 @@ static int iqs7222_parse_chan(struct iqs7222_private *iqs7222, int chan_index)
>  			*setup |= (val / 500 << i * 8);
>  		}
>  
> -		error = fwnode_property_read_u32(event_node, "linux,code",
> -						 &val);
> -		if (error) {
> -			dev_err(&client->dev, "Failed to read %s code: %d\n",
> -				fwnode_get_name(chan_node), error);
> +		error = iqs7222_parse_event(iqs7222, event_node, chan_index,
> +					    IQS7222_REG_GRP_BTN,
> +					    iqs7222_kp_events[i].reg_key,
> +					    BIT(chan_index),
> +					    dev_desc->touch_link - (i ? 0 : 2),
> +					    &iqs7222->kp_type[chan_index][i],
> +					    &iqs7222->kp_code[chan_index][i]);
> +		fwnode_handle_put(event_node);
> +		if (error)
>  			return error;
> -		}
> -
> -		iqs7222->kp_code[chan_index][i] = val;
> -		iqs7222->kp_type[chan_index][i] = EV_KEY;
> -
> -		if (fwnode_property_present(event_node, "linux,input-type")) {
> -			error = fwnode_property_read_u32(event_node,
> -							 "linux,input-type",
> -							 &val);
> -			if (error) {
> -				dev_err(&client->dev,
> -					"Failed to read %s input type: %d\n",
> -					fwnode_get_name(chan_node), error);
> -				return error;
> -			}
> -
> -			if (val != EV_KEY && val != EV_SW) {
> -				dev_err(&client->dev,
> -					"Invalid %s input type: %u\n",
> -					fwnode_get_name(chan_node), val);
> -				return -EINVAL;
> -			}
> -
> -			iqs7222->kp_type[chan_index][i] = val;
> -		}
> -
> -		/*
> -		 * Reference channels can opt out of event reporting by using
> -		 * KEY_RESERVED in place of a true key or switch code.
> -		 */
> -		if (iqs7222->kp_type[chan_index][i] == EV_KEY &&
> -		    iqs7222->kp_code[chan_index][i] == KEY_RESERVED)
> -			continue;
> -
> -		input_set_capability(iqs7222->keypad,
> -				     iqs7222->kp_type[chan_index][i],
> -				     iqs7222->kp_code[chan_index][i]);
>  
>  		if (!dev_desc->event_offset)
>  			continue;
> @@ -1981,16 +1947,16 @@ static int iqs7222_parse_chan(struct iqs7222_private *iqs7222, int chan_index)
>  	 * The following call handles a special pair of properties that apply
>  	 * to a channel node, but reside within the button (event) group.
>  	 */
> -	return iqs7222_parse_props(iqs7222, &chan_node, chan_index,
> +	return iqs7222_parse_props(iqs7222, chan_node, chan_index,
>  				   IQS7222_REG_GRP_BTN,
>  				   IQS7222_REG_KEY_DEBOUNCE);
>  }
>  
> -static int iqs7222_parse_sldr(struct iqs7222_private *iqs7222, int sldr_index)
> +static int iqs7222_parse_sldr(struct iqs7222_private *iqs7222,
> +			      struct fwnode_handle *sldr_node, int sldr_index)
>  {
>  	const struct iqs7222_dev_desc *dev_desc = iqs7222->dev_desc;
>  	struct i2c_client *client = iqs7222->client;
> -	struct fwnode_handle *sldr_node = NULL;
>  	int num_chan = dev_desc->reg_grps[IQS7222_REG_GRP_CHAN].num_row;
>  	int ext_chan = rounddown(num_chan, 10);
>  	int count, error, reg_offset, i;
> @@ -1998,15 +1964,12 @@ static int iqs7222_parse_sldr(struct iqs7222_private *iqs7222, int sldr_index)
>  	u16 *sldr_setup = iqs7222->sldr_setup[sldr_index];
>  	unsigned int chan_sel[4], val;
>  
> -	error = iqs7222_parse_props(iqs7222, &sldr_node, sldr_index,
> +	error = iqs7222_parse_props(iqs7222, sldr_node, sldr_index,
>  				    IQS7222_REG_GRP_SLDR,
>  				    IQS7222_REG_KEY_NONE);
>  	if (error)
>  		return error;
>  
> -	if (!sldr_node)
> -		return 0;
> -
>  	/*
>  	 * Each slider can be spread across 3 to 4 channels. It is possible to
>  	 * select only 2 channels, but doing so prevents the slider from using
> @@ -2130,46 +2093,37 @@ static int iqs7222_parse_sldr(struct iqs7222_private *iqs7222, int sldr_index)
>  	for (i = 0; i < ARRAY_SIZE(iqs7222_sl_events); i++) {
>  		const char *event_name = iqs7222_sl_events[i].name;
>  		struct fwnode_handle *event_node;
> +		enum iqs7222_reg_key_id reg_key;
>  
>  		event_node = fwnode_get_named_child_node(sldr_node, event_name);
>  		if (!event_node)
>  			continue;
>  
> -		error = iqs7222_parse_props(iqs7222, &event_node, sldr_index,
> -					    IQS7222_REG_GRP_SLDR,
> -					    reg_offset ?
> -					    IQS7222_REG_KEY_RESERVED :
> -					    iqs7222_sl_events[i].reg_key);
> -		if (error)
> -			return error;
> +		if (reg_offset)
> +			reg_key = IQS7222_REG_KEY_RESERVED;
> +		else
> +			reg_key = iqs7222_sl_events[i].reg_key;
>  
>  		/*
>  		 * The press/release event does not expose a direct GPIO link,
>  		 * but one can be emulated by tying each of the participating
>  		 * channels to the same GPIO.
>  		 */
> -		error = iqs7222_gpio_select(iqs7222, event_node,
> +		error = iqs7222_parse_event(iqs7222, event_node, sldr_index,
> +					    IQS7222_REG_GRP_SLDR, reg_key,
>  					    i ? iqs7222_sl_events[i].enable
>  					      : sldr_setup[3 + reg_offset],
>  					    i ? 1568 + sldr_index * 30
> -					      : sldr_setup[4 + reg_offset]);
> +					      : sldr_setup[4 + reg_offset],
> +					    NULL,
> +					    &iqs7222->sl_code[sldr_index][i]);
> +		fwnode_handle_put(event_node);
>  		if (error)
>  			return error;
>  
>  		if (!reg_offset)
>  			sldr_setup[9] |= iqs7222_sl_events[i].enable;
>  
> -		error = fwnode_property_read_u32(event_node, "linux,code",
> -						 &val);
> -		if (error) {
> -			dev_err(&client->dev, "Failed to read %s code: %d\n",
> -				fwnode_get_name(sldr_node), error);
> -			return error;
> -		}
> -
> -		iqs7222->sl_code[sldr_index][i] = val;
> -		input_set_capability(iqs7222->keypad, EV_KEY, val);
> -
>  		if (!dev_desc->event_offset)
>  			continue;
>  
> @@ -2190,19 +2144,64 @@ static int iqs7222_parse_sldr(struct iqs7222_private *iqs7222, int sldr_index)
>  	 * The following call handles a special pair of properties that shift
>  	 * to make room for a wheel enable control in the case of IQS7222C.
>  	 */
> -	return iqs7222_parse_props(iqs7222, &sldr_node, sldr_index,
> +	return iqs7222_parse_props(iqs7222, sldr_node, sldr_index,
>  				   IQS7222_REG_GRP_SLDR,
>  				   dev_desc->wheel_enable ?
>  				   IQS7222_REG_KEY_WHEEL :
>  				   IQS7222_REG_KEY_NO_WHEEL);
>  }
>  
> +static int (*iqs7222_parse_extra[IQS7222_NUM_REG_GRPS])
> +				(struct iqs7222_private *iqs7222,
> +				 struct fwnode_handle *reg_grp_node,
> +				 int reg_grp_index) = {
> +	[IQS7222_REG_GRP_CYCLE] = iqs7222_parse_cycle,
> +	[IQS7222_REG_GRP_CHAN] = iqs7222_parse_chan,
> +	[IQS7222_REG_GRP_SLDR] = iqs7222_parse_sldr,
> +};
> +
> +static int iqs7222_parse_reg_grp(struct iqs7222_private *iqs7222,
> +				 enum iqs7222_reg_grp_id reg_grp,
> +				 int reg_grp_index)
> +{
> +	struct i2c_client *client = iqs7222->client;
> +	struct fwnode_handle *reg_grp_node;
> +	int error;
> +
> +	if (iqs7222_reg_grp_names[reg_grp]) {
> +		char reg_grp_name[16];
> +
> +		snprintf(reg_grp_name, sizeof(reg_grp_name), "%s-%d",
> +			 iqs7222_reg_grp_names[reg_grp], reg_grp_index);
> +
> +		reg_grp_node = device_get_named_child_node(&client->dev,
> +							   reg_grp_name);
> +	} else {
> +		reg_grp_node = fwnode_handle_get(dev_fwnode(&client->dev));
> +	}
> +
> +	if (!reg_grp_node)
> +		return 0;
> +
> +	if (iqs7222_parse_extra[reg_grp])
> +		error = iqs7222_parse_extra[reg_grp](iqs7222, reg_grp_node,
> +						     reg_grp_index);
> +	else
> +		error = iqs7222_parse_props(iqs7222, reg_grp_node,
> +					    reg_grp_index,
> +					    reg_grp, IQS7222_REG_KEY_NONE);
> +
> +	fwnode_handle_put(reg_grp_node);
> +
> +	return error;
> +}
> +
>  static int iqs7222_parse_all(struct iqs7222_private *iqs7222)
>  {
>  	const struct iqs7222_dev_desc *dev_desc = iqs7222->dev_desc;
>  	const struct iqs7222_reg_grp_desc *reg_grps = dev_desc->reg_grps;
>  	u16 *sys_setup = iqs7222->sys_setup;
> -	int error, i;
> +	int error, i, j;
>  
>  	if (dev_desc->allow_offset)
>  		sys_setup[dev_desc->allow_offset] = U16_MAX;
> @@ -2210,32 +2209,13 @@ static int iqs7222_parse_all(struct iqs7222_private *iqs7222)
>  	if (dev_desc->event_offset)
>  		sys_setup[dev_desc->event_offset] = IQS7222_EVENT_MASK_ATI;
>  
> -	for (i = 0; i < reg_grps[IQS7222_REG_GRP_CYCLE].num_row; i++) {
> -		error = iqs7222_parse_cycle(iqs7222, i);
> -		if (error)
> -			return error;
> -	}
> -
> -	error = iqs7222_parse_props(iqs7222, NULL, 0, IQS7222_REG_GRP_GLBL,
> -				    IQS7222_REG_KEY_NONE);
> -	if (error)
> -		return error;
> -
>  	for (i = 0; i < reg_grps[IQS7222_REG_GRP_GPIO].num_row; i++) {
> -		struct fwnode_handle *gpio_node = NULL;
>  		u16 *gpio_setup = iqs7222->gpio_setup[i];
> -		int j;
>  
>  		gpio_setup[0] &= ~IQS7222_GPIO_SETUP_0_GPIO_EN;
>  		gpio_setup[1] = 0;
>  		gpio_setup[2] = 0;
>  
> -		error = iqs7222_parse_props(iqs7222, &gpio_node, i,
> -					    IQS7222_REG_GRP_GPIO,
> -					    IQS7222_REG_KEY_NONE);
> -		if (error)
> -			return error;
> -
>  		if (reg_grps[IQS7222_REG_GRP_GPIO].num_row == 1)
>  			continue;
>  
> @@ -2258,29 +2238,21 @@ static int iqs7222_parse_all(struct iqs7222_private *iqs7222)
>  		chan_setup[5] = 0;
>  	}
>  
> -	for (i = 0; i < reg_grps[IQS7222_REG_GRP_CHAN].num_row; i++) {
> -		error = iqs7222_parse_chan(iqs7222, i);
> -		if (error)
> -			return error;
> -	}
> -
> -	error = iqs7222_parse_props(iqs7222, NULL, 0, IQS7222_REG_GRP_FILT,
> -				    IQS7222_REG_KEY_NONE);
> -	if (error)
> -		return error;
> -
>  	for (i = 0; i < reg_grps[IQS7222_REG_GRP_SLDR].num_row; i++) {
>  		u16 *sldr_setup = iqs7222->sldr_setup[i];
>  
>  		sldr_setup[0] &= ~IQS7222_SLDR_SETUP_0_CHAN_CNT_MASK;
> +	}
>  
> -		error = iqs7222_parse_sldr(iqs7222, i);
> -		if (error)
> -			return error;
> +	for (i = 0; i < IQS7222_NUM_REG_GRPS; i++) {
> +		for (j = 0; j < reg_grps[i].num_row; j++) {
> +			error = iqs7222_parse_reg_grp(iqs7222, i, j);
> +			if (error)
> +				return error;
> +		}
>  	}
>  
> -	return iqs7222_parse_props(iqs7222, NULL, 0, IQS7222_REG_GRP_SYS,
> -				   IQS7222_REG_KEY_NONE);
> +	return 0;
>  }
>  
>  static int iqs7222_report(struct iqs7222_private *iqs7222)
> -- 
> 2.34.1

[PATCH v2 2/7] dt-bindings: input: iqs7222: Reduce 'linux,code' to optional

[Jeff LaBundy]

Following a recent refactor of the driver to properly drop unused
device nodes, the 'linux,code' property is now optional. This can
be useful for applications that define GPIO-mapped events that do
not correspond to any keycode.

Fixes: 44dc42d254bf ("dt-bindings: input: Add bindings for Azoteq IQS7222A/B/C")
Signed-off-by: Jeff LaBundy <jeff@labundy.com>
---
Changes in v2:
 - Updated commit message

 Documentation/devicetree/bindings/input/azoteq,iqs7222.yaml | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/Documentation/devicetree/bindings/input/azoteq,iqs7222.yaml b/Documentation/devicetree/bindings/input/azoteq,iqs7222.yaml
index 02e605fac408..b4eb650dbcb8 100644
--- a/Documentation/devicetree/bindings/input/azoteq,iqs7222.yaml
+++ b/Documentation/devicetree/bindings/input/azoteq,iqs7222.yaml
@@ -473,9 +473,6 @@ patternProperties:
               Specifies whether the event is to be interpreted as a key (1)
               or a switch (5).
 
-        required:
-          - linux,code
-
         additionalProperties: false
 
     dependencies:
@@ -620,9 +617,6 @@ patternProperties:
               GPIO, they must all be of the same type (proximity, touch or
               slider gesture).
 
-        required:
-          - linux,code
-
         additionalProperties: false
 
     required:
-- 
2.34.1

Re: [PATCH v2 2/7] dt-bindings: input: iqs7222: Reduce 'linux,code' to optional

[Rob Herring]

On Thu, 15 Sep 2022 23:31:07 -0500, Jeff LaBundy wrote:
> Following a recent refactor of the driver to properly drop unused
> device nodes, the 'linux,code' property is now optional. This can
> be useful for applications that define GPIO-mapped events that do
> not correspond to any keycode.
> 
> Fixes: 44dc42d254bf ("dt-bindings: input: Add bindings for Azoteq IQS7222A/B/C")
> Signed-off-by: Jeff LaBundy <jeff@labundy.com>
> ---
> Changes in v2:
>  - Updated commit message
> 
>  Documentation/devicetree/bindings/input/azoteq,iqs7222.yaml | 6 ------
>  1 file changed, 6 deletions(-)
> 

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

[PATCH v2 3/7] Input: iqs7222 - report malformed properties

[Jeff LaBundy]

Nonzero return values of several calls to fwnode_property_read_u32()
are silently ignored, leaving no way to know the properties were not
applied in the event of an error.

Solve this problem by evaluating fwnode_property_read_u32()'s return
value, and reporting an error for any nonzero return value not equal
to -EINVAL which indicates the property was absent altogether.

Fixes: e505edaedcb9 ("Input: add support for Azoteq IQS7222A/B/C")
Signed-off-by: Jeff LaBundy <jeff@labundy.com>
---
Changes in v2:
 - Used -EINVAL returned by fwnode_property_read_u32() to indicate an absent
   optional property as opposed to calling fwnode_property_present()
 - Updated commit message

 drivers/input/misc/iqs7222.c | 43 +++++++++++++++++++++++++++++-------
 1 file changed, 35 insertions(+), 8 deletions(-)

diff --git a/drivers/input/misc/iqs7222.c b/drivers/input/misc/iqs7222.c
index d39b3fdfb849..36c3b24e99a3 100644
--- a/drivers/input/misc/iqs7222.c
+++ b/drivers/input/misc/iqs7222.c
@@ -1820,8 +1820,9 @@ static int iqs7222_parse_chan(struct iqs7222_private *iqs7222,
 		chan_setup[0] |= IQS7222_CHAN_SETUP_0_REF_MODE_FOLLOW;
 		chan_setup[4] = val * 42 + 1048;
 
-		if (!fwnode_property_read_u32(chan_node, "azoteq,ref-weight",
-					      &val)) {
+		error = fwnode_property_read_u32(chan_node, "azoteq,ref-weight",
+						 &val);
+		if (!error) {
 			if (val > U16_MAX) {
 				dev_err(&client->dev,
 					"Invalid %s reference weight: %u\n",
@@ -1830,6 +1831,11 @@ static int iqs7222_parse_chan(struct iqs7222_private *iqs7222,
 			}
 
 			chan_setup[5] = val;
+		} else if (error != -EINVAL) {
+			dev_err(&client->dev,
+				"Failed to read %s reference weight: %d\n",
+				fwnode_get_name(chan_node), error);
+			return error;
 		}
 
 		/*
@@ -1902,9 +1908,10 @@ static int iqs7222_parse_chan(struct iqs7222_private *iqs7222,
 		if (!event_node)
 			continue;
 
-		if (!fwnode_property_read_u32(event_node,
-					      "azoteq,timeout-press-ms",
-					      &val)) {
+		error = fwnode_property_read_u32(event_node,
+						 "azoteq,timeout-press-ms",
+						 &val);
+		if (!error) {
 			/*
 			 * The IQS7222B employs a global pair of press timeout
 			 * registers as opposed to channel-specific registers.
@@ -1924,6 +1931,11 @@ static int iqs7222_parse_chan(struct iqs7222_private *iqs7222,
 
 			*setup &= ~(U8_MAX << i * 8);
 			*setup |= (val / 500 << i * 8);
+		} else if (error != -EINVAL) {
+			dev_err(&client->dev,
+				"Failed to read %s press timeout: %d\n",
+				fwnode_get_name(event_node), error);
+			return error;
 		}
 
 		error = iqs7222_parse_event(iqs7222, event_node, chan_index,
@@ -2028,7 +2040,8 @@ static int iqs7222_parse_sldr(struct iqs7222_private *iqs7222,
 	if (fwnode_property_present(sldr_node, "azoteq,use-prox"))
 		sldr_setup[4 + reg_offset] -= 2;
 
-	if (!fwnode_property_read_u32(sldr_node, "azoteq,slider-size", &val)) {
+	error = fwnode_property_read_u32(sldr_node, "azoteq,slider-size", &val);
+	if (!error) {
 		if (!val || val > dev_desc->sldr_res) {
 			dev_err(&client->dev, "Invalid %s size: %u\n",
 				fwnode_get_name(sldr_node), val);
@@ -2042,9 +2055,14 @@ static int iqs7222_parse_sldr(struct iqs7222_private *iqs7222,
 			sldr_setup[2] |= (val / 16 <<
 					  IQS7222_SLDR_SETUP_2_RES_SHIFT);
 		}
+	} else if (error != -EINVAL) {
+		dev_err(&client->dev, "Failed to read %s size: %d\n",
+			fwnode_get_name(sldr_node), error);
+		return error;
 	}
 
-	if (!fwnode_property_read_u32(sldr_node, "azoteq,top-speed", &val)) {
+	error = fwnode_property_read_u32(sldr_node, "azoteq,top-speed", &val);
+	if (!error) {
 		if (val > (reg_offset ? U16_MAX : U8_MAX * 4)) {
 			dev_err(&client->dev, "Invalid %s top speed: %u\n",
 				fwnode_get_name(sldr_node), val);
@@ -2057,9 +2075,14 @@ static int iqs7222_parse_sldr(struct iqs7222_private *iqs7222,
 			sldr_setup[2] &= ~IQS7222_SLDR_SETUP_2_TOP_SPEED_MASK;
 			sldr_setup[2] |= (val / 4);
 		}
+	} else if (error != -EINVAL) {
+		dev_err(&client->dev, "Failed to read %s top speed: %d\n",
+			fwnode_get_name(sldr_node), error);
+		return error;
 	}
 
-	if (!fwnode_property_read_u32(sldr_node, "linux,axis", &val)) {
+	error = fwnode_property_read_u32(sldr_node, "linux,axis", &val);
+	if (!error) {
 		u16 sldr_max = sldr_setup[3] - 1;
 
 		if (!reg_offset) {
@@ -2073,6 +2096,10 @@ static int iqs7222_parse_sldr(struct iqs7222_private *iqs7222,
 
 		input_set_abs_params(iqs7222->keypad, val, 0, sldr_max, 0, 0);
 		iqs7222->sl_axis[sldr_index] = val;
+	} else if (error != -EINVAL) {
+		dev_err(&client->dev, "Failed to read %s axis: %d\n",
+			fwnode_get_name(sldr_node), error);
+		return error;
 	}
 
 	if (dev_desc->wheel_enable) {
-- 
2.34.1

Re: [PATCH v2 3/7] Input: iqs7222 - report malformed properties

[Mattijs Korpershoek]

On Thu, Sep 15, 2022 at 23:31, Jeff LaBundy <jeff@labundy.com> wrote:

> Nonzero return values of several calls to fwnode_property_read_u32()
> are silently ignored, leaving no way to know the properties were not
> applied in the event of an error.
>
> Solve this problem by evaluating fwnode_property_read_u32()'s return
> value, and reporting an error for any nonzero return value not equal
> to -EINVAL which indicates the property was absent altogether.
>
> Fixes: e505edaedcb9 ("Input: add support for Azoteq IQS7222A/B/C")
> Signed-off-by: Jeff LaBundy <jeff@labundy.com>
> ---
> Changes in v2:
>  - Used -EINVAL returned by fwnode_property_read_u32() to indicate an absent
>    optional property as opposed to calling fwnode_property_present()
>  - Updated commit message
>
>  drivers/input/misc/iqs7222.c | 43 +++++++++++++++++++++++++++++-------
>  1 file changed, 35 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/input/misc/iqs7222.c b/drivers/input/misc/iqs7222.c
> index d39b3fdfb849..36c3b24e99a3 100644
> --- a/drivers/input/misc/iqs7222.c
> +++ b/drivers/input/misc/iqs7222.c
> @@ -1820,8 +1820,9 @@ static int iqs7222_parse_chan(struct iqs7222_private *iqs7222,
>  		chan_setup[0] |= IQS7222_CHAN_SETUP_0_REF_MODE_FOLLOW;
>  		chan_setup[4] = val * 42 + 1048;
>  
> -		if (!fwnode_property_read_u32(chan_node, "azoteq,ref-weight",
> -					      &val)) {
> +		error = fwnode_property_read_u32(chan_node, "azoteq,ref-weight",
> +						 &val);
> +		if (!error) {
>  			if (val > U16_MAX) {
>  				dev_err(&client->dev,
>  					"Invalid %s reference weight: %u\n",
> @@ -1830,6 +1831,11 @@ static int iqs7222_parse_chan(struct iqs7222_private *iqs7222,
>  			}
>  
>  			chan_setup[5] = val;
> +		} else if (error != -EINVAL) {
> +			dev_err(&client->dev,
> +				"Failed to read %s reference weight: %d\n",
> +				fwnode_get_name(chan_node), error);
> +			return error;
>  		}
>  
>  		/*
> @@ -1902,9 +1908,10 @@ static int iqs7222_parse_chan(struct iqs7222_private *iqs7222,
>  		if (!event_node)
>  			continue;
>  
> -		if (!fwnode_property_read_u32(event_node,
> -					      "azoteq,timeout-press-ms",
> -					      &val)) {
> +		error = fwnode_property_read_u32(event_node,
> +						 "azoteq,timeout-press-ms",
> +						 &val);
> +		if (!error) {
>  			/*
>  			 * The IQS7222B employs a global pair of press timeout
>  			 * registers as opposed to channel-specific registers.
> @@ -1924,6 +1931,11 @@ static int iqs7222_parse_chan(struct iqs7222_private *iqs7222,
>  
>  			*setup &= ~(U8_MAX << i * 8);
>  			*setup |= (val / 500 << i * 8);
> +		} else if (error != -EINVAL) {
> +			dev_err(&client->dev,
> +				"Failed to read %s press timeout: %d\n",
> +				fwnode_get_name(event_node), error);

Shouldn't we call fwnode_handle_put(event_node); here?
It's what we do in the error path just above (line 2029)

With that added, feel free to include:

Reviewed-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>

> +			return error;
>  		}
>  
>  		error = iqs7222_parse_event(iqs7222, event_node, chan_index,
> @@ -2028,7 +2040,8 @@ static int iqs7222_parse_sldr(struct iqs7222_private *iqs7222,
>  	if (fwnode_property_present(sldr_node, "azoteq,use-prox"))
>  		sldr_setup[4 + reg_offset] -= 2;
>  
> -	if (!fwnode_property_read_u32(sldr_node, "azoteq,slider-size", &val)) {
> +	error = fwnode_property_read_u32(sldr_node, "azoteq,slider-size", &val);
> +	if (!error) {
>  		if (!val || val > dev_desc->sldr_res) {
>  			dev_err(&client->dev, "Invalid %s size: %u\n",
>  				fwnode_get_name(sldr_node), val);
> @@ -2042,9 +2055,14 @@ static int iqs7222_parse_sldr(struct iqs7222_private *iqs7222,
>  			sldr_setup[2] |= (val / 16 <<
>  					  IQS7222_SLDR_SETUP_2_RES_SHIFT);
>  		}
> +	} else if (error != -EINVAL) {
> +		dev_err(&client->dev, "Failed to read %s size: %d\n",
> +			fwnode_get_name(sldr_node), error);
> +		return error;
>  	}
>  
> -	if (!fwnode_property_read_u32(sldr_node, "azoteq,top-speed", &val)) {
> +	error = fwnode_property_read_u32(sldr_node, "azoteq,top-speed", &val);
> +	if (!error) {
>  		if (val > (reg_offset ? U16_MAX : U8_MAX * 4)) {
>  			dev_err(&client->dev, "Invalid %s top speed: %u\n",
>  				fwnode_get_name(sldr_node), val);
> @@ -2057,9 +2075,14 @@ static int iqs7222_parse_sldr(struct iqs7222_private *iqs7222,
>  			sldr_setup[2] &= ~IQS7222_SLDR_SETUP_2_TOP_SPEED_MASK;
>  			sldr_setup[2] |= (val / 4);
>  		}
> +	} else if (error != -EINVAL) {
> +		dev_err(&client->dev, "Failed to read %s top speed: %d\n",
> +			fwnode_get_name(sldr_node), error);
> +		return error;
>  	}
>  
> -	if (!fwnode_property_read_u32(sldr_node, "linux,axis", &val)) {
> +	error = fwnode_property_read_u32(sldr_node, "linux,axis", &val);
> +	if (!error) {
>  		u16 sldr_max = sldr_setup[3] - 1;
>  
>  		if (!reg_offset) {
> @@ -2073,6 +2096,10 @@ static int iqs7222_parse_sldr(struct iqs7222_private *iqs7222,
>  
>  		input_set_abs_params(iqs7222->keypad, val, 0, sldr_max, 0, 0);
>  		iqs7222->sl_axis[sldr_index] = val;
> +	} else if (error != -EINVAL) {
> +		dev_err(&client->dev, "Failed to read %s axis: %d\n",
> +			fwnode_get_name(sldr_node), error);
> +		return error;
>  	}
>  
>  	if (dev_desc->wheel_enable) {
> -- 
> 2.34.1

Re: [PATCH v2 3/7] Input: iqs7222 - report malformed properties

[Jeff LaBundy]

Hi Mattijs,

On Fri, Oct 07, 2022 at 03:12:17PM +0200, Mattijs Korpershoek wrote:
> On Thu, Sep 15, 2022 at 23:31, Jeff LaBundy <jeff@labundy.com> wrote:
> 
> > Nonzero return values of several calls to fwnode_property_read_u32()
> > are silently ignored, leaving no way to know the properties were not
> > applied in the event of an error.
> >
> > Solve this problem by evaluating fwnode_property_read_u32()'s return
> > value, and reporting an error for any nonzero return value not equal
> > to -EINVAL which indicates the property was absent altogether.
> >
> > Fixes: e505edaedcb9 ("Input: add support for Azoteq IQS7222A/B/C")
> > Signed-off-by: Jeff LaBundy <jeff@labundy.com>
> > ---
> > Changes in v2:
> >  - Used -EINVAL returned by fwnode_property_read_u32() to indicate an absent
> >    optional property as opposed to calling fwnode_property_present()
> >  - Updated commit message
> >
> >  drivers/input/misc/iqs7222.c | 43 +++++++++++++++++++++++++++++-------
> >  1 file changed, 35 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/input/misc/iqs7222.c b/drivers/input/misc/iqs7222.c
> > index d39b3fdfb849..36c3b24e99a3 100644
> > --- a/drivers/input/misc/iqs7222.c
> > +++ b/drivers/input/misc/iqs7222.c
> > @@ -1820,8 +1820,9 @@ static int iqs7222_parse_chan(struct iqs7222_private *iqs7222,
> >  		chan_setup[0] |= IQS7222_CHAN_SETUP_0_REF_MODE_FOLLOW;
> >  		chan_setup[4] = val * 42 + 1048;
> >  
> > -		if (!fwnode_property_read_u32(chan_node, "azoteq,ref-weight",
> > -					      &val)) {
> > +		error = fwnode_property_read_u32(chan_node, "azoteq,ref-weight",
> > +						 &val);
> > +		if (!error) {
> >  			if (val > U16_MAX) {
> >  				dev_err(&client->dev,
> >  					"Invalid %s reference weight: %u\n",
> > @@ -1830,6 +1831,11 @@ static int iqs7222_parse_chan(struct iqs7222_private *iqs7222,
> >  			}
> >  
> >  			chan_setup[5] = val;
> > +		} else if (error != -EINVAL) {
> > +			dev_err(&client->dev,
> > +				"Failed to read %s reference weight: %d\n",
> > +				fwnode_get_name(chan_node), error);
> > +			return error;
> >  		}
> >  
> >  		/*
> > @@ -1902,9 +1908,10 @@ static int iqs7222_parse_chan(struct iqs7222_private *iqs7222,
> >  		if (!event_node)
> >  			continue;
> >  
> > -		if (!fwnode_property_read_u32(event_node,
> > -					      "azoteq,timeout-press-ms",
> > -					      &val)) {
> > +		error = fwnode_property_read_u32(event_node,
> > +						 "azoteq,timeout-press-ms",
> > +						 &val);
> > +		if (!error) {
> >  			/*
> >  			 * The IQS7222B employs a global pair of press timeout
> >  			 * registers as opposed to channel-specific registers.
> > @@ -1924,6 +1931,11 @@ static int iqs7222_parse_chan(struct iqs7222_private *iqs7222,
> >  
> >  			*setup &= ~(U8_MAX << i * 8);
> >  			*setup |= (val / 500 << i * 8);
> > +		} else if (error != -EINVAL) {
> > +			dev_err(&client->dev,
> > +				"Failed to read %s press timeout: %d\n",
> > +				fwnode_get_name(event_node), error);
> 
> Shouldn't we call fwnode_handle_put(event_node); here?
> It's what we do in the error path just above (line 2029)

Thank you for your review and for finding this; it's a great catch. I
will send out a v3 after the merge window with this fixed.

> 
> With that added, feel free to include:
> 
> Reviewed-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>
> 
> > +			return error;
> >  		}
> >  
> >  		error = iqs7222_parse_event(iqs7222, event_node, chan_index,
> > @@ -2028,7 +2040,8 @@ static int iqs7222_parse_sldr(struct iqs7222_private *iqs7222,
> >  	if (fwnode_property_present(sldr_node, "azoteq,use-prox"))
> >  		sldr_setup[4 + reg_offset] -= 2;
> >  
> > -	if (!fwnode_property_read_u32(sldr_node, "azoteq,slider-size", &val)) {
> > +	error = fwnode_property_read_u32(sldr_node, "azoteq,slider-size", &val);
> > +	if (!error) {
> >  		if (!val || val > dev_desc->sldr_res) {
> >  			dev_err(&client->dev, "Invalid %s size: %u\n",
> >  				fwnode_get_name(sldr_node), val);
> > @@ -2042,9 +2055,14 @@ static int iqs7222_parse_sldr(struct iqs7222_private *iqs7222,
> >  			sldr_setup[2] |= (val / 16 <<
> >  					  IQS7222_SLDR_SETUP_2_RES_SHIFT);
> >  		}
> > +	} else if (error != -EINVAL) {
> > +		dev_err(&client->dev, "Failed to read %s size: %d\n",
> > +			fwnode_get_name(sldr_node), error);
> > +		return error;
> >  	}
> >  
> > -	if (!fwnode_property_read_u32(sldr_node, "azoteq,top-speed", &val)) {
> > +	error = fwnode_property_read_u32(sldr_node, "azoteq,top-speed", &val);
> > +	if (!error) {
> >  		if (val > (reg_offset ? U16_MAX : U8_MAX * 4)) {
> >  			dev_err(&client->dev, "Invalid %s top speed: %u\n",
> >  				fwnode_get_name(sldr_node), val);
> > @@ -2057,9 +2075,14 @@ static int iqs7222_parse_sldr(struct iqs7222_private *iqs7222,
> >  			sldr_setup[2] &= ~IQS7222_SLDR_SETUP_2_TOP_SPEED_MASK;
> >  			sldr_setup[2] |= (val / 4);
> >  		}
> > +	} else if (error != -EINVAL) {
> > +		dev_err(&client->dev, "Failed to read %s top speed: %d\n",
> > +			fwnode_get_name(sldr_node), error);
> > +		return error;
> >  	}
> >  
> > -	if (!fwnode_property_read_u32(sldr_node, "linux,axis", &val)) {
> > +	error = fwnode_property_read_u32(sldr_node, "linux,axis", &val);
> > +	if (!error) {
> >  		u16 sldr_max = sldr_setup[3] - 1;
> >  
> >  		if (!reg_offset) {
> > @@ -2073,6 +2096,10 @@ static int iqs7222_parse_sldr(struct iqs7222_private *iqs7222,
> >  
> >  		input_set_abs_params(iqs7222->keypad, val, 0, sldr_max, 0, 0);
> >  		iqs7222->sl_axis[sldr_index] = val;
> > +	} else if (error != -EINVAL) {
> > +		dev_err(&client->dev, "Failed to read %s axis: %d\n",
> > +			fwnode_get_name(sldr_node), error);
> > +		return error;
> >  	}
> >  
> >  	if (dev_desc->wheel_enable) {
> > -- 
> > 2.34.1

Kind regards,
Jeff LaBundy

[PATCH v2 4/7] dt-bindings: input: iqs7222: Correct minimum slider size

[Jeff LaBundy]

The minimum slider size enforced by the driver is 1 or 16 for the
IQS7222C or IQS7222A, respectively.

Fixes: 44dc42d254bf ("dt-bindings: input: Add bindings for Azoteq IQS7222A/B/C")
Signed-off-by: Jeff LaBundy <jeff@labundy.com>
Acked-by: Rob Herring <robh@kernel.org>
---
Changes in v2:
 - Added Acked-by

 Documentation/devicetree/bindings/input/azoteq,iqs7222.yaml | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/input/azoteq,iqs7222.yaml b/Documentation/devicetree/bindings/input/azoteq,iqs7222.yaml
index b4eb650dbcb8..913fd2da9862 100644
--- a/Documentation/devicetree/bindings/input/azoteq,iqs7222.yaml
+++ b/Documentation/devicetree/bindings/input/azoteq,iqs7222.yaml
@@ -498,7 +498,7 @@ patternProperties:
 
       azoteq,slider-size:
         $ref: /schemas/types.yaml#/definitions/uint32
-        minimum: 0
+        minimum: 1
         maximum: 65535
         description:
           Specifies the slider's one-dimensional resolution, equal to the
@@ -687,6 +687,7 @@ allOf:
           properties:
             azoteq,slider-size:
               multipleOf: 16
+              minimum: 16
               maximum: 4080
 
             azoteq,top-speed:
-- 
2.34.1

[PATCH v2 5/7] Input: iqs7222 - protect against undefined slider size

[Jeff LaBundy]

Select variants of silicon do not define a default slider size, in
which case the size must be specified in the device tree. If it is
not, the axis's maximum value is reported as 65535 due to unsigned
integer overflow.

To solve this problem, move the existing zero-check outside of the
conditional block that checks whether the property is present.

Fixes: e505edaedcb9 ("Input: add support for Azoteq IQS7222A/B/C")
Signed-off-by: Jeff LaBundy <jeff@labundy.com>
---
Changes in v2:
 - Rebased to account for changes earlier in series

 drivers/input/misc/iqs7222.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/input/misc/iqs7222.c b/drivers/input/misc/iqs7222.c
index 36c3b24e99a3..0a592b90f471 100644
--- a/drivers/input/misc/iqs7222.c
+++ b/drivers/input/misc/iqs7222.c
@@ -2042,7 +2042,7 @@ static int iqs7222_parse_sldr(struct iqs7222_private *iqs7222,
 
 	error = fwnode_property_read_u32(sldr_node, "azoteq,slider-size", &val);
 	if (!error) {
-		if (!val || val > dev_desc->sldr_res) {
+		if (val > dev_desc->sldr_res) {
 			dev_err(&client->dev, "Invalid %s size: %u\n",
 				fwnode_get_name(sldr_node), val);
 			return -EINVAL;
@@ -2061,6 +2061,13 @@ static int iqs7222_parse_sldr(struct iqs7222_private *iqs7222,
 		return error;
 	}
 
+	if (!(reg_offset ? sldr_setup[3]
+			 : sldr_setup[2] & IQS7222_SLDR_SETUP_2_RES_MASK)) {
+		dev_err(&client->dev, "Undefined %s size\n",
+			fwnode_get_name(sldr_node));
+		return -EINVAL;
+	}
+
 	error = fwnode_property_read_u32(sldr_node, "azoteq,top-speed", &val);
 	if (!error) {
 		if (val > (reg_offset ? U16_MAX : U8_MAX * 4)) {
-- 
2.34.1

[PATCH v2 6/7] dt-bindings: input: iqs7222: Add support for IQS7222A v1.13+

[Jeff LaBundy]

IQS7222A revisions 1.13 and later widen the gesture multiplier from
x4 ms to x16 ms; update the binding accordingly.

As part of this change, refresh the corresponding properties in the
example as well.

Fixes: 44dc42d254bf ("dt-bindings: input: Add bindings for Azoteq IQS7222A/B/C")
Signed-off-by: Jeff LaBundy <jeff@labundy.com>
Reviewed-by: Rob Herring <robh@kernel.org>
---
Changes in v2:
 - Added Reviewed-by

 .../bindings/input/azoteq,iqs7222.yaml           | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/Documentation/devicetree/bindings/input/azoteq,iqs7222.yaml b/Documentation/devicetree/bindings/input/azoteq,iqs7222.yaml
index 913fd2da9862..9ddba7f2e7aa 100644
--- a/Documentation/devicetree/bindings/input/azoteq,iqs7222.yaml
+++ b/Documentation/devicetree/bindings/input/azoteq,iqs7222.yaml
@@ -572,9 +572,9 @@ patternProperties:
           linux,code: true
 
           azoteq,gesture-max-ms:
-            multipleOf: 4
+            multipleOf: 16
             minimum: 0
-            maximum: 1020
+            maximum: 4080
             description:
               Specifies the length of time (in ms) within which a tap, swipe
               or flick gesture must be completed in order to be acknowledged
@@ -582,9 +582,9 @@ patternProperties:
               gesture applies to all remaining swipe or flick gestures.
 
           azoteq,gesture-min-ms:
-            multipleOf: 4
+            multipleOf: 16
             minimum: 0
-            maximum: 124
+            maximum: 496
             description:
               Specifies the length of time (in ms) for which a tap gesture must
               be held in order to be acknowledged by the device.
@@ -930,14 +930,14 @@ examples:
 
                             event-tap {
                                     linux,code = <KEY_PLAYPAUSE>;
-                                    azoteq,gesture-max-ms = <600>;
-                                    azoteq,gesture-min-ms = <24>;
+                                    azoteq,gesture-max-ms = <400>;
+                                    azoteq,gesture-min-ms = <32>;
                             };
 
                             event-flick-pos {
                                     linux,code = <KEY_NEXTSONG>;
-                                    azoteq,gesture-max-ms = <600>;
-                                    azoteq,gesture-dist = <816>;
+                                    azoteq,gesture-max-ms = <800>;
+                                    azoteq,gesture-dist = <800>;
                             };
 
                             event-flick-neg {
-- 
2.34.1

[PATCH v2 7/7] Input: iqs7222 - add support for IQS7222A v1.13+

[Jeff LaBundy]

IQS7222A revisions 1.13 and later widen the gesture multiplier from
x4 ms to x16 ms. Add a means to scale the gesture timings specified
in the device tree based on the revision of the device.

Fixes: e505edaedcb9 ("Input: add support for Azoteq IQS7222A/B/C")
Signed-off-by: Jeff LaBundy <jeff@labundy.com>
---
Changes in v2:
 - Rebased to account for changes earlier in series

 drivers/input/misc/iqs7222.c | 111 +++++++++++++++++++++++++++++++++++
 1 file changed, 111 insertions(+)

diff --git a/drivers/input/misc/iqs7222.c b/drivers/input/misc/iqs7222.c
index 0a592b90f471..3340de51fb2d 100644
--- a/drivers/input/misc/iqs7222.c
+++ b/drivers/input/misc/iqs7222.c
@@ -86,7 +86,9 @@ enum iqs7222_reg_key_id {
 	IQS7222_REG_KEY_TOUCH,
 	IQS7222_REG_KEY_DEBOUNCE,
 	IQS7222_REG_KEY_TAP,
+	IQS7222_REG_KEY_TAP_LEGACY,
 	IQS7222_REG_KEY_AXIAL,
+	IQS7222_REG_KEY_AXIAL_LEGACY,
 	IQS7222_REG_KEY_WHEEL,
 	IQS7222_REG_KEY_NO_WHEEL,
 	IQS7222_REG_KEY_RESERVED
@@ -202,10 +204,68 @@ struct iqs7222_dev_desc {
 	int allow_offset;
 	int event_offset;
 	int comms_offset;
+	bool legacy_gesture;
 	struct iqs7222_reg_grp_desc reg_grps[IQS7222_NUM_REG_GRPS];
 };
 
 static const struct iqs7222_dev_desc iqs7222_devs[] = {
+	{
+		.prod_num = IQS7222_PROD_NUM_A,
+		.fw_major = 1,
+		.fw_minor = 13,
+		.sldr_res = U8_MAX * 16,
+		.touch_link = 1768,
+		.allow_offset = 9,
+		.event_offset = 10,
+		.comms_offset = 12,
+		.reg_grps = {
+			[IQS7222_REG_GRP_STAT] = {
+				.base = IQS7222_SYS_STATUS,
+				.num_row = 1,
+				.num_col = 8,
+			},
+			[IQS7222_REG_GRP_CYCLE] = {
+				.base = 0x8000,
+				.num_row = 7,
+				.num_col = 3,
+			},
+			[IQS7222_REG_GRP_GLBL] = {
+				.base = 0x8700,
+				.num_row = 1,
+				.num_col = 3,
+			},
+			[IQS7222_REG_GRP_BTN] = {
+				.base = 0x9000,
+				.num_row = 12,
+				.num_col = 3,
+			},
+			[IQS7222_REG_GRP_CHAN] = {
+				.base = 0xA000,
+				.num_row = 12,
+				.num_col = 6,
+			},
+			[IQS7222_REG_GRP_FILT] = {
+				.base = 0xAC00,
+				.num_row = 1,
+				.num_col = 2,
+			},
+			[IQS7222_REG_GRP_SLDR] = {
+				.base = 0xB000,
+				.num_row = 2,
+				.num_col = 11,
+			},
+			[IQS7222_REG_GRP_GPIO] = {
+				.base = 0xC000,
+				.num_row = 1,
+				.num_col = 3,
+			},
+			[IQS7222_REG_GRP_SYS] = {
+				.base = IQS7222_SYS_SETUP,
+				.num_row = 1,
+				.num_col = 13,
+			},
+		},
+	},
 	{
 		.prod_num = IQS7222_PROD_NUM_A,
 		.fw_major = 1,
@@ -215,6 +275,7 @@ static const struct iqs7222_dev_desc iqs7222_devs[] = {
 		.allow_offset = 9,
 		.event_offset = 10,
 		.comms_offset = 12,
+		.legacy_gesture = true,
 		.reg_grps = {
 			[IQS7222_REG_GRP_STAT] = {
 				.base = IQS7222_SYS_STATUS,
@@ -874,6 +935,16 @@ static const struct iqs7222_prop_desc iqs7222_props[] = {
 		.reg_offset = 9,
 		.reg_shift = 8,
 		.reg_width = 8,
+		.val_pitch = 16,
+		.label = "maximum gesture time",
+	},
+	{
+		.name = "azoteq,gesture-max-ms",
+		.reg_grp = IQS7222_REG_GRP_SLDR,
+		.reg_key = IQS7222_REG_KEY_TAP_LEGACY,
+		.reg_offset = 9,
+		.reg_shift = 8,
+		.reg_width = 8,
 		.val_pitch = 4,
 		.label = "maximum gesture time",
 	},
@@ -884,6 +955,16 @@ static const struct iqs7222_prop_desc iqs7222_props[] = {
 		.reg_offset = 9,
 		.reg_shift = 3,
 		.reg_width = 5,
+		.val_pitch = 16,
+		.label = "minimum gesture time",
+	},
+	{
+		.name = "azoteq,gesture-min-ms",
+		.reg_grp = IQS7222_REG_GRP_SLDR,
+		.reg_key = IQS7222_REG_KEY_TAP_LEGACY,
+		.reg_offset = 9,
+		.reg_shift = 3,
+		.reg_width = 5,
 		.val_pitch = 4,
 		.label = "minimum gesture time",
 	},
@@ -897,6 +978,16 @@ static const struct iqs7222_prop_desc iqs7222_props[] = {
 		.val_pitch = 16,
 		.label = "gesture distance",
 	},
+	{
+		.name = "azoteq,gesture-dist",
+		.reg_grp = IQS7222_REG_GRP_SLDR,
+		.reg_key = IQS7222_REG_KEY_AXIAL_LEGACY,
+		.reg_offset = 10,
+		.reg_shift = 8,
+		.reg_width = 8,
+		.val_pitch = 16,
+		.label = "gesture distance",
+	},
 	{
 		.name = "azoteq,gesture-max-ms",
 		.reg_grp = IQS7222_REG_GRP_SLDR,
@@ -904,6 +995,16 @@ static const struct iqs7222_prop_desc iqs7222_props[] = {
 		.reg_offset = 10,
 		.reg_shift = 0,
 		.reg_width = 8,
+		.val_pitch = 16,
+		.label = "maximum gesture time",
+	},
+	{
+		.name = "azoteq,gesture-max-ms",
+		.reg_grp = IQS7222_REG_GRP_SLDR,
+		.reg_key = IQS7222_REG_KEY_AXIAL_LEGACY,
+		.reg_offset = 10,
+		.reg_shift = 0,
+		.reg_width = 8,
 		.val_pitch = 4,
 		.label = "maximum gesture time",
 	},
@@ -2133,8 +2234,18 @@ static int iqs7222_parse_sldr(struct iqs7222_private *iqs7222,
 		if (!event_node)
 			continue;
 
+		/*
+		 * Depending on the device, gestures are either offered using
+		 * one of two timing resolutions, or are not supported at all.
+		 */
 		if (reg_offset)
 			reg_key = IQS7222_REG_KEY_RESERVED;
+		else if (dev_desc->legacy_gesture &&
+			 iqs7222_sl_events[i].reg_key == IQS7222_REG_KEY_TAP)
+			reg_key = IQS7222_REG_KEY_TAP_LEGACY;
+		else if (dev_desc->legacy_gesture &&
+			 iqs7222_sl_events[i].reg_key == IQS7222_REG_KEY_AXIAL)
+			reg_key = IQS7222_REG_KEY_AXIAL_LEGACY;
 		else
 			reg_key = iqs7222_sl_events[i].reg_key;
 
-- 
2.34.1

Re: [PATCH v2 7/7] Input: iqs7222 - add support for IQS7222A v1.13+

[Mattijs Korpershoek]

On Thu, Sep 15, 2022 at 23:32, Jeff LaBundy <jeff@labundy.com> wrote:

> IQS7222A revisions 1.13 and later widen the gesture multiplier from
> x4 ms to x16 ms. Add a means to scale the gesture timings specified
> in the device tree based on the revision of the device.
>
> Fixes: e505edaedcb9 ("Input: add support for Azoteq IQS7222A/B/C")
> Signed-off-by: Jeff LaBundy <jeff@labundy.com>
> ---
> Changes in v2:
>  - Rebased to account for changes earlier in series
>
>  drivers/input/misc/iqs7222.c | 111 +++++++++++++++++++++++++++++++++++
>  1 file changed, 111 insertions(+)

Reviewed-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>

>
> diff --git a/drivers/input/misc/iqs7222.c b/drivers/input/misc/iqs7222.c
> index 0a592b90f471..3340de51fb2d 100644
> --- a/drivers/input/misc/iqs7222.c
> +++ b/drivers/input/misc/iqs7222.c
> @@ -86,7 +86,9 @@ enum iqs7222_reg_key_id {
>  	IQS7222_REG_KEY_TOUCH,
>  	IQS7222_REG_KEY_DEBOUNCE,
>  	IQS7222_REG_KEY_TAP,
> +	IQS7222_REG_KEY_TAP_LEGACY,
>  	IQS7222_REG_KEY_AXIAL,
> +	IQS7222_REG_KEY_AXIAL_LEGACY,
>  	IQS7222_REG_KEY_WHEEL,
>  	IQS7222_REG_KEY_NO_WHEEL,
>  	IQS7222_REG_KEY_RESERVED
> @@ -202,10 +204,68 @@ struct iqs7222_dev_desc {
>  	int allow_offset;
>  	int event_offset;
>  	int comms_offset;
> +	bool legacy_gesture;
>  	struct iqs7222_reg_grp_desc reg_grps[IQS7222_NUM_REG_GRPS];
>  };
>  
>  static const struct iqs7222_dev_desc iqs7222_devs[] = {
> +	{
> +		.prod_num = IQS7222_PROD_NUM_A,
> +		.fw_major = 1,
> +		.fw_minor = 13,
> +		.sldr_res = U8_MAX * 16,
> +		.touch_link = 1768,
> +		.allow_offset = 9,
> +		.event_offset = 10,
> +		.comms_offset = 12,
> +		.reg_grps = {
> +			[IQS7222_REG_GRP_STAT] = {
> +				.base = IQS7222_SYS_STATUS,
> +				.num_row = 1,
> +				.num_col = 8,
> +			},
> +			[IQS7222_REG_GRP_CYCLE] = {
> +				.base = 0x8000,
> +				.num_row = 7,
> +				.num_col = 3,
> +			},
> +			[IQS7222_REG_GRP_GLBL] = {
> +				.base = 0x8700,
> +				.num_row = 1,
> +				.num_col = 3,
> +			},
> +			[IQS7222_REG_GRP_BTN] = {
> +				.base = 0x9000,
> +				.num_row = 12,
> +				.num_col = 3,
> +			},
> +			[IQS7222_REG_GRP_CHAN] = {
> +				.base = 0xA000,
> +				.num_row = 12,
> +				.num_col = 6,
> +			},
> +			[IQS7222_REG_GRP_FILT] = {
> +				.base = 0xAC00,
> +				.num_row = 1,
> +				.num_col = 2,
> +			},
> +			[IQS7222_REG_GRP_SLDR] = {
> +				.base = 0xB000,
> +				.num_row = 2,
> +				.num_col = 11,
> +			},
> +			[IQS7222_REG_GRP_GPIO] = {
> +				.base = 0xC000,
> +				.num_row = 1,
> +				.num_col = 3,
> +			},
> +			[IQS7222_REG_GRP_SYS] = {
> +				.base = IQS7222_SYS_SETUP,
> +				.num_row = 1,
> +				.num_col = 13,
> +			},
> +		},
> +	},
>  	{
>  		.prod_num = IQS7222_PROD_NUM_A,
>  		.fw_major = 1,
> @@ -215,6 +275,7 @@ static const struct iqs7222_dev_desc iqs7222_devs[] = {
>  		.allow_offset = 9,
>  		.event_offset = 10,
>  		.comms_offset = 12,
> +		.legacy_gesture = true,
>  		.reg_grps = {
>  			[IQS7222_REG_GRP_STAT] = {
>  				.base = IQS7222_SYS_STATUS,
> @@ -874,6 +935,16 @@ static const struct iqs7222_prop_desc iqs7222_props[] = {
>  		.reg_offset = 9,
>  		.reg_shift = 8,
>  		.reg_width = 8,
> +		.val_pitch = 16,
> +		.label = "maximum gesture time",
> +	},
> +	{
> +		.name = "azoteq,gesture-max-ms",
> +		.reg_grp = IQS7222_REG_GRP_SLDR,
> +		.reg_key = IQS7222_REG_KEY_TAP_LEGACY,
> +		.reg_offset = 9,
> +		.reg_shift = 8,
> +		.reg_width = 8,
>  		.val_pitch = 4,
>  		.label = "maximum gesture time",
>  	},
> @@ -884,6 +955,16 @@ static const struct iqs7222_prop_desc iqs7222_props[] = {
>  		.reg_offset = 9,
>  		.reg_shift = 3,
>  		.reg_width = 5,
> +		.val_pitch = 16,
> +		.label = "minimum gesture time",
> +	},
> +	{
> +		.name = "azoteq,gesture-min-ms",
> +		.reg_grp = IQS7222_REG_GRP_SLDR,
> +		.reg_key = IQS7222_REG_KEY_TAP_LEGACY,
> +		.reg_offset = 9,
> +		.reg_shift = 3,
> +		.reg_width = 5,
>  		.val_pitch = 4,
>  		.label = "minimum gesture time",
>  	},
> @@ -897,6 +978,16 @@ static const struct iqs7222_prop_desc iqs7222_props[] = {
>  		.val_pitch = 16,
>  		.label = "gesture distance",
>  	},
> +	{
> +		.name = "azoteq,gesture-dist",
> +		.reg_grp = IQS7222_REG_GRP_SLDR,
> +		.reg_key = IQS7222_REG_KEY_AXIAL_LEGACY,
> +		.reg_offset = 10,
> +		.reg_shift = 8,
> +		.reg_width = 8,
> +		.val_pitch = 16,
> +		.label = "gesture distance",
> +	},
>  	{
>  		.name = "azoteq,gesture-max-ms",
>  		.reg_grp = IQS7222_REG_GRP_SLDR,
> @@ -904,6 +995,16 @@ static const struct iqs7222_prop_desc iqs7222_props[] = {
>  		.reg_offset = 10,
>  		.reg_shift = 0,
>  		.reg_width = 8,
> +		.val_pitch = 16,
> +		.label = "maximum gesture time",
> +	},
> +	{
> +		.name = "azoteq,gesture-max-ms",
> +		.reg_grp = IQS7222_REG_GRP_SLDR,
> +		.reg_key = IQS7222_REG_KEY_AXIAL_LEGACY,
> +		.reg_offset = 10,
> +		.reg_shift = 0,
> +		.reg_width = 8,
>  		.val_pitch = 4,
>  		.label = "maximum gesture time",
>  	},
> @@ -2133,8 +2234,18 @@ static int iqs7222_parse_sldr(struct iqs7222_private *iqs7222,
>  		if (!event_node)
>  			continue;
>  
> +		/*
> +		 * Depending on the device, gestures are either offered using
> +		 * one of two timing resolutions, or are not supported at all.
> +		 */
>  		if (reg_offset)
>  			reg_key = IQS7222_REG_KEY_RESERVED;
> +		else if (dev_desc->legacy_gesture &&
> +			 iqs7222_sl_events[i].reg_key == IQS7222_REG_KEY_TAP)
> +			reg_key = IQS7222_REG_KEY_TAP_LEGACY;
> +		else if (dev_desc->legacy_gesture &&
> +			 iqs7222_sl_events[i].reg_key == IQS7222_REG_KEY_AXIAL)
> +			reg_key = IQS7222_REG_KEY_AXIAL_LEGACY;
>  		else
>  			reg_key = iqs7222_sl_events[i].reg_key;
>  
> -- 
> 2.34.1