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@xxxxxxxxxxx>
> > > > ---
> > > > 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
