| Message ID | YyP77bKCoxMEMyjD@nixie71 (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Additional fixes for Azoteq IQS7222A/B/C | expand |
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
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)
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(-)