On Sun, Sep 15, 2024 at 12:44:13PM +0200, Chen-Yu Tsai wrote:
> On Fri, Sep 13, 2024 at 12:25 PM Andy Shevchenko
> <andriy.shevchenko@xxxxxxxxxxxxxxx> wrote:
> > On Wed, Sep 11, 2024 at 03:27:44PM +0800, Chen-Yu Tsai wrote:
...
> > > +int i2c_of_probe_component(struct device *dev, const struct i2c_of_probe_cfg *cfg, void *ctx)
> > > +{
> > > + const struct i2c_of_probe_ops *ops;
> > > + const char *type;
> > > + struct device_node *i2c_node;
> > > + struct i2c_adapter *i2c;
> > > + int ret;
> > > +
> > > + if (!cfg)
> > > + return -EINVAL;
> > > +
> > > + ops = cfg->ops ?: &i2c_of_probe_dummy_ops;
> > > + type = cfg->type;
> > > +
> > > + i2c_node = i2c_of_probe_get_i2c_node(dev, type);
> >
> >
> > struct device_node *i2c_node __free(of_node_put) =
> > i2c_...;
>
> cleanup.h says to not mix the two styles (scoped vs goto). I was trying
> to follow that, though I realize now that with the scoped loops it
> probably doesn't help.
>
> I'll revert back to having __free().
>
> > > + if (IS_ERR(i2c_node))
> > > + return PTR_ERR(i2c_node);
> > > +
> > > + for_each_child_of_node_with_prefix(i2c_node, node, type) {
> > > + if (!of_device_is_available(node))
> > > + continue;
> > > +
> > > + /*
> > > + * Device tree has component already enabled. Either the
> > > + * device tree isn't supported or we already probed once.
> > > + */
> > > + ret = 0;
> >
> > Shouldn't you drop reference count for "node"? (See also below)
>
> This for-each loop the "scoped". It just doesn't have the prefix anymore.
> I believe you asked if the prefix could be dropped and then Rob agreed.
Hmm... I have looked into the implementation and I haven't found the evidence
that this is anyhow scoped. Can you point out what I have missed?
> > > + goto out_put_i2c_node;
> > > + }
> > > +
> > > + i2c = of_get_i2c_adapter_by_node(i2c_node);
> > > + if (!i2c) {
> > > + ret = dev_err_probe(dev, -EPROBE_DEFER, "Couldn't get I2C adapter\n");
> > > + goto out_put_i2c_node;
> > > + }
> > > +
> > > + /* Grab resources */
> > > + ret = 0;
> > > + if (ops->get_resources)
> > > + ret = ops->get_resources(dev, i2c_node, ctx);
> > > + if (ret)
> > > + goto out_put_i2c_adapter;
> > > +
> > > + /* Enable resources */
> > > + if (ops->enable)
> > > + ret = ops->enable(dev, ctx);
> > > + if (ret)
> > > + goto out_release_resources;
> > > +
> > > + ret = 0;
> > > + for_each_child_of_node_with_prefix(i2c_node, node, type) {
> > > + union i2c_smbus_data data;
> > > + u32 addr;
> > > +
> > > + if (of_property_read_u32(node, "reg", &addr))
> > > + continue;
> > > + if (i2c_smbus_xfer(i2c, addr, 0, I2C_SMBUS_READ, 0, I2C_SMBUS_BYTE, &data) < 0)
> > > + continue;
> > > +
> > > + /* Found a device that is responding */
> > > + if (ops->free_resources_early)
> > > + ops->free_resources_early(ctx);
> > > + ret = i2c_of_probe_enable_node(dev, node);
> >
> > Hmm... Is "node" reference count left bumped up for a reason?
>
> Same as above.
Same as above.
> > > + break;
> > > + }
> > > +
> > > + if (ops->cleanup)
> > > + ops->cleanup(dev, ctx);
> > > +out_release_resources:
> > > + if (ops->free_resources_late)
> > > + ops->free_resources_late(ctx);
> > > +out_put_i2c_adapter:
> > > + i2c_put_adapter(i2c);
> > > +out_put_i2c_node:
> > > + of_node_put(i2c_node);
> > > +
> > > + return ret;
> > > +}
--
With Best Regards,
Andy Shevchenko
