| Message ID | 20220908131548.48120-1-jeff@labundy.com |
|---|---|
| Headers | show |
| Series | Additional fixes for Azoteq IQS7222A/B/C | expand |
On Thu, Sep 08, 2022 at 08:15:38AM -0500, Jeff LaBundy wrote: > Each call to device/fwnode_get_named_child_node() must be matched > with a call to fwnode_handle_put() once the corresponding node is > no longer in use. This ensures a reference count remains balanced > in the case of dynamic device tree support. > > Currently, the driver never calls fwnode_handle_put(). This patch > adds the missing calls. Hmm, dev_fwnode() however does not do that, which means that iqs7222_parse_props() has different refounting, depending on what is being fetched. I think we need to start there. Also, maybe we could avoid sprinkling gotos if we moved property reading code into helpers? Thanks.
On Thu, Sep 08, 2022 at 08:15:39AM -0500, Jeff LaBundy wrote: > Nonzero return values of several calls to fwnode_property_read_u32() > are silenty ignored, leaving no way to know that the properties were > not applied in the event of an error. > > To solve this problem, follow the design pattern used throughout the > rest of the driver by first checking if the property is present, and > then evaluating the return value of fwnode_property_read_u32(). > > Fixes: e505edaedcb9 ("Input: add support for Azoteq IQS7222A/B/C") > Signed-off-by: Jeff LaBundy <jeff@labundy.com> > --- > drivers/input/misc/iqs7222.c | 65 ++++++++++++++++++++++++++++++------ > 1 file changed, 55 insertions(+), 10 deletions(-) > > diff --git a/drivers/input/misc/iqs7222.c b/drivers/input/misc/iqs7222.c > index 04c1050d845c..fdade24caa8a 100644 > --- a/drivers/input/misc/iqs7222.c > +++ b/drivers/input/misc/iqs7222.c > @@ -1819,8 +1819,17 @@ static int iqs7222_parse_chan(struct iqs7222_private *iqs7222, int chan_index) > 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)) { > + if (fwnode_property_present(chan_node, "azoteq,ref-weight")) { > + error = fwnode_property_read_u32(chan_node, > + "azoteq,ref-weight", > + &val); fwnode_property_read_u32() returns EINVAL if property is missing, so maybe have: error = fwnode_property_read_u32(chan_node, "azoteq,ref-weight", &val); if (!error) { ... } else { if (error != -EINVAL) { dev_err(&client->dev, "Failed to read %s reference weight: %d\n", fwnode_get_name(chan_node), error); goto put_chan_node; } } to avoid double calls into property handling code?
On Thu, Sep 08, 2022 at 08:15:42AM -0500, Jeff LaBundy wrote: > According to the datasheets, writing only 0xFF is sufficient to > elicit a communication window. Remove the superfluous 0x00 from > the force communication command. > > Fixes: e505edaedcb9 ("Input: add support for Azoteq IQS7222A/B/C") > Signed-off-by: Jeff LaBundy <jeff@labundy.com> Applied, thank you.
Hi Dmitry, Thank you for taking a look. On Thu, Sep 08, 2022 at 02:17:21PM -0700, Dmitry Torokhov wrote: > On Thu, Sep 08, 2022 at 08:15:38AM -0500, Jeff LaBundy wrote: > > Each call to device/fwnode_get_named_child_node() must be matched > > with a call to fwnode_handle_put() once the corresponding node is > > no longer in use. This ensures a reference count remains balanced > > in the case of dynamic device tree support. > > > > Currently, the driver never calls fwnode_handle_put(). This patch > > adds the missing calls. > > Hmm, dev_fwnode() however does not do that, which means that > iqs7222_parse_props() has different refounting, depending on what is > being fetched. I think we need to start there. Right, but none of the callers that prompt iqs7222_parse_props() to use dev_fwnode() follow with fwnode_handle_put(). > > Also, maybe we could avoid sprinkling gotos if we moved property reading > code into helpers? I like this idea; I will give it a try. > > Thanks. > > -- > Dmitry Kind regards, Jeff LaBundy
Hi Dmitry, On Thu, Sep 08, 2022 at 02:21:13PM -0700, Dmitry Torokhov wrote: > On Thu, Sep 08, 2022 at 08:15:39AM -0500, Jeff LaBundy wrote: > > Nonzero return values of several calls to fwnode_property_read_u32() > > are silenty ignored, leaving no way to know that the properties were > > not applied in the event of an error. > > > > To solve this problem, follow the design pattern used throughout the > > rest of the driver by first checking if the property is present, and > > then evaluating the return value of fwnode_property_read_u32(). > > > > Fixes: e505edaedcb9 ("Input: add support for Azoteq IQS7222A/B/C") > > Signed-off-by: Jeff LaBundy <jeff@labundy.com> > > --- > > drivers/input/misc/iqs7222.c | 65 ++++++++++++++++++++++++++++++------ > > 1 file changed, 55 insertions(+), 10 deletions(-) > > > > diff --git a/drivers/input/misc/iqs7222.c b/drivers/input/misc/iqs7222.c > > index 04c1050d845c..fdade24caa8a 100644 > > --- a/drivers/input/misc/iqs7222.c > > +++ b/drivers/input/misc/iqs7222.c > > @@ -1819,8 +1819,17 @@ static int iqs7222_parse_chan(struct iqs7222_private *iqs7222, int chan_index) > > 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)) { > > + if (fwnode_property_present(chan_node, "azoteq,ref-weight")) { > > + error = fwnode_property_read_u32(chan_node, > > + "azoteq,ref-weight", > > + &val); > > fwnode_property_read_u32() returns EINVAL if property is missing, so > maybe have: > > error = fwnode_property_read_u32(chan_node, > "azoteq,ref-weight", &val); > if (!error) { > ... > } else { > if (error != -EINVAL) { > dev_err(&client->dev, > "Failed to read %s reference weight: %d\n", > fwnode_get_name(chan_node), error); > goto put_chan_node; > } > } > > to avoid double calls into property handling code? That's a better idea; I can fold this into the helper functions proposed for the previous patch too. > > -- > Dmitry Kind regards, Jeff LaBundy
On Thu, Sep 08, 2022 at 09:04:06PM -0500, Jeff LaBundy wrote: > Hi Dmitry, > > Thank you for taking a look. > > On Thu, Sep 08, 2022 at 02:17:21PM -0700, Dmitry Torokhov wrote: > > On Thu, Sep 08, 2022 at 08:15:38AM -0500, Jeff LaBundy wrote: > > > Each call to device/fwnode_get_named_child_node() must be matched > > > with a call to fwnode_handle_put() once the corresponding node is > > > no longer in use. This ensures a reference count remains balanced > > > in the case of dynamic device tree support. > > > > > > Currently, the driver never calls fwnode_handle_put(). This patch > > > adds the missing calls. > > > > Hmm, dev_fwnode() however does not do that, which means that > > iqs7222_parse_props() has different refounting, depending on what is > > being fetched. I think we need to start there. > > Right, but none of the callers that prompt iqs7222_parse_props() to > use dev_fwnode() follow with fwnode_handle_put(). I think this is a problem that code has to be aware of that and behave differently. I'd recommend bumping up refcount in dev_fwnode() path so that all callers would behave uniformly. Thanks.
On Thu, Sep 08, 2022 at 09:08:09PM -0500, Jeff LaBundy wrote: > Hi Dmitry, > > On Thu, Sep 08, 2022 at 02:21:13PM -0700, Dmitry Torokhov wrote: > > On Thu, Sep 08, 2022 at 08:15:39AM -0500, Jeff LaBundy wrote: > > > Nonzero return values of several calls to fwnode_property_read_u32() > > > are silenty ignored, leaving no way to know that the properties were > > > not applied in the event of an error. > > > > > > To solve this problem, follow the design pattern used throughout the > > > rest of the driver by first checking if the property is present, and > > > then evaluating the return value of fwnode_property_read_u32(). > > > > > > Fixes: e505edaedcb9 ("Input: add support for Azoteq IQS7222A/B/C") > > > Signed-off-by: Jeff LaBundy <jeff@labundy.com> > > > --- > > > drivers/input/misc/iqs7222.c | 65 ++++++++++++++++++++++++++++++------ > > > 1 file changed, 55 insertions(+), 10 deletions(-) > > > > > > diff --git a/drivers/input/misc/iqs7222.c b/drivers/input/misc/iqs7222.c > > > index 04c1050d845c..fdade24caa8a 100644 > > > --- a/drivers/input/misc/iqs7222.c > > > +++ b/drivers/input/misc/iqs7222.c > > > @@ -1819,8 +1819,17 @@ static int iqs7222_parse_chan(struct iqs7222_private *iqs7222, int chan_index) > > > 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)) { > > > + if (fwnode_property_present(chan_node, "azoteq,ref-weight")) { > > > + error = fwnode_property_read_u32(chan_node, > > > + "azoteq,ref-weight", > > > + &val); > > > > fwnode_property_read_u32() returns EINVAL if property is missing, so > > maybe have: > > > > error = fwnode_property_read_u32(chan_node, > > "azoteq,ref-weight", &val); > > if (!error) { > > ... > > } else { > > if (error != -EINVAL) { > > dev_err(&client->dev, > > "Failed to read %s reference weight: %d\n", > > fwnode_get_name(chan_node), error); > > goto put_chan_node; > > } > > } > > > > to avoid double calls into property handling code? > > That's a better idea; I can fold this into the helper functions proposed > for the previous patch too. We might be talking about different helpers. I had in mind: static int __iqs7222_parse_cycle(...) { ... } static int iqs7222_parse_cycle(...) { ... int retval = 0; error = iqs7222_parse_props(iqs7222, &cycle_node, cycle_index, IQS7222_REG_GRP_CYCLE, IQS7222_REG_KEY_NONE); if (error) return error; if (cycle_node) { retval = __iqs7222_parse_cycle(...); fwnode_put(cycle_node); } return retval; } so that we drop the node from one place. Thanks.
Hi Dmitry, On Thu, Sep 08, 2022 at 09:37:57PM -0700, Dmitry Torokhov wrote: > On Thu, Sep 08, 2022 at 09:04:06PM -0500, Jeff LaBundy wrote: > > Hi Dmitry, > > > > Thank you for taking a look. > > > > On Thu, Sep 08, 2022 at 02:17:21PM -0700, Dmitry Torokhov wrote: > > > On Thu, Sep 08, 2022 at 08:15:38AM -0500, Jeff LaBundy wrote: > > > > Each call to device/fwnode_get_named_child_node() must be matched > > > > with a call to fwnode_handle_put() once the corresponding node is > > > > no longer in use. This ensures a reference count remains balanced > > > > in the case of dynamic device tree support. > > > > > > > > Currently, the driver never calls fwnode_handle_put(). This patch > > > > adds the missing calls. > > > > > > Hmm, dev_fwnode() however does not do that, which means that > > > iqs7222_parse_props() has different refounting, depending on what is > > > being fetched. I think we need to start there. > > > > Right, but none of the callers that prompt iqs7222_parse_props() to > > use dev_fwnode() follow with fwnode_handle_put(). > > I think this is a problem that code has to be aware of that and behave > differently. I'd recommend bumping up refcount in dev_fwnode() path so > that all callers would behave uniformly. Agreed, right now the problem is that not all callers have a node to put. So, I think the solution is to more thoughtfully encapsulate all of this such that the caller is always responsible for passing a node, iqs7222_parse_props() bumps the refcount of whatever it is fetching, and the caller is always responsible for dropping the node. This way, callers of iqs7222_parse_props() need not be burdened with what it chooses to do under the hood. > > Thanks. > > -- > Dmitry Kind regards, Jeff LaBundy
Hi Dmitry, On Thu, Sep 08, 2022 at 09:42:47PM -0700, Dmitry Torokhov wrote: > On Thu, Sep 08, 2022 at 09:08:09PM -0500, Jeff LaBundy wrote: > > Hi Dmitry, > > > > On Thu, Sep 08, 2022 at 02:21:13PM -0700, Dmitry Torokhov wrote: > > > On Thu, Sep 08, 2022 at 08:15:39AM -0500, Jeff LaBundy wrote: > > > > Nonzero return values of several calls to fwnode_property_read_u32() > > > > are silenty ignored, leaving no way to know that the properties were > > > > not applied in the event of an error. > > > > > > > > To solve this problem, follow the design pattern used throughout the > > > > rest of the driver by first checking if the property is present, and > > > > then evaluating the return value of fwnode_property_read_u32(). > > > > > > > > Fixes: e505edaedcb9 ("Input: add support for Azoteq IQS7222A/B/C") > > > > Signed-off-by: Jeff LaBundy <jeff@labundy.com> > > > > --- > > > > drivers/input/misc/iqs7222.c | 65 ++++++++++++++++++++++++++++++------ > > > > 1 file changed, 55 insertions(+), 10 deletions(-) > > > > > > > > diff --git a/drivers/input/misc/iqs7222.c b/drivers/input/misc/iqs7222.c > > > > index 04c1050d845c..fdade24caa8a 100644 > > > > --- a/drivers/input/misc/iqs7222.c > > > > +++ b/drivers/input/misc/iqs7222.c > > > > @@ -1819,8 +1819,17 @@ static int iqs7222_parse_chan(struct iqs7222_private *iqs7222, int chan_index) > > > > 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)) { > > > > + if (fwnode_property_present(chan_node, "azoteq,ref-weight")) { > > > > + error = fwnode_property_read_u32(chan_node, > > > > + "azoteq,ref-weight", > > > > + &val); > > > > > > fwnode_property_read_u32() returns EINVAL if property is missing, so > > > maybe have: > > > > > > error = fwnode_property_read_u32(chan_node, > > > "azoteq,ref-weight", &val); > > > if (!error) { > > > ... > > > } else { > > > if (error != -EINVAL) { > > > dev_err(&client->dev, > > > "Failed to read %s reference weight: %d\n", > > > fwnode_get_name(chan_node), error); > > > goto put_chan_node; > > > } > > > } > > > > > > to avoid double calls into property handling code? > > > > That's a better idea; I can fold this into the helper functions proposed > > for the previous patch too. > > We might be talking about different helpers. I had in mind: > > static int __iqs7222_parse_cycle(...) > { > ... > } > > static int iqs7222_parse_cycle(...) > { > ... > int retval = 0; > > error = iqs7222_parse_props(iqs7222, &cycle_node, cycle_index, > IQS7222_REG_GRP_CYCLE, > IQS7222_REG_KEY_NONE); > if (error) > return error; > > if (cycle_node) { > retval = __iqs7222_parse_cycle(...); > fwnode_put(cycle_node); > } > > > return retval; > } > > so that we drop the node from one place. Right, originally I had imagined a wrapper around fwnode_property_read_u32() that calls fwnode_handle_put() in the error path. However, your proposal is even better. Thanks for the fruitful discussion; I'll clean all of this up for v2. > > Thanks. > > -- > Dmitry Kind regards, Jeff LaBundy
Hi Dmitry, On Thu, Sep 08, 2022 at 02:24:10PM -0700, Dmitry Torokhov wrote: > On Thu, Sep 08, 2022 at 08:15:42AM -0500, Jeff LaBundy wrote: > > According to the datasheets, writing only 0xFF is sufficient to > > elicit a communication window. Remove the superfluous 0x00 from > > the force communication command. > > > > Fixes: e505edaedcb9 ("Input: add support for Azoteq IQS7222A/B/C") > > Signed-off-by: Jeff LaBundy <jeff@labundy.com> > > Applied, thank you. I didn't happen to see this one hit your tree, so I can simply include it in v2 coming soon. In case I have misunderstood, please let me know. > > -- > Dmitry Kind regards, Jeff LaBundy
On Thu, Sep 08, 2022 at 08:15:43AM -0500, Jeff LaBundy wrote: > Add a check to prevent sending undefined events, which ultimately > map to SYN_REPORT. > > Fixes: e505edaedcb9 ("Input: add support for Azoteq IQS7222A/B/C") > Signed-off-by: Jeff LaBundy <jeff@labundy.com> Applied, thank you.
On Thu, Sep 08, 2022 at 08:15:44AM -0500, Jeff LaBundy wrote: > Some devices expose an ultra-low-power (ULP) mode entry mask for > each channel. If the mask is set, the device cannot enter ULP so > long as the corresponding channel remains in an active state. > > The vendor has advised setting the mask for any disabled channel. > To accommodate this suggestion, initially set all masks and then > clear them only if specified in the device tree. > > Fixes: e505edaedcb9 ("Input: add support for Azoteq IQS7222A/B/C") > Signed-off-by: Jeff LaBundy <jeff@labundy.com> Applied, thank you.
On Tue, Sep 13, 2022 at 04:24:54PM -0500, Jeff LaBundy wrote: > Hi Dmitry, > > On Thu, Sep 08, 2022 at 02:24:10PM -0700, Dmitry Torokhov wrote: > > On Thu, Sep 08, 2022 at 08:15:42AM -0500, Jeff LaBundy wrote: > > > According to the datasheets, writing only 0xFF is sufficient to > > > elicit a communication window. Remove the superfluous 0x00 from > > > the force communication command. > > > > > > Fixes: e505edaedcb9 ("Input: add support for Azoteq IQS7222A/B/C") > > > Signed-off-by: Jeff LaBundy <jeff@labundy.com> > > > > Applied, thank you. > > I didn't happen to see this one hit your tree, so I can simply include > it in v2 coming soon. In case I have misunderstood, please let me know. Should be there now. Thanks.