[PATCH v8 4/8] i2c: Introduce OF component probe function

[Chen-Yu Tsai]

On Wed, Oct 16, 2024 at 1:58 AM Doug Anderson <dianders at chromium.org> wrote:
>
> Hi,
>
> On Tue, Oct 8, 2024 at 12:35 AM Chen-Yu Tsai <wenst at chromium.org> 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 i2c_adapter *i2c;
> > +       int ret;
> > +
> > +       ops = cfg->ops ?: &i2c_of_probe_dummy_ops;
> > +       type = cfg->type;
> > +
> > +       struct device_node *i2c_node __free(device_node) = i2c_of_probe_get_i2c_node(dev, type);
> > +       if (IS_ERR(i2c_node))
> > +               return PTR_ERR(i2c_node);
>
> I'm still getting comfortable with the __free() syntax so sorry if I'm
> wrong, but I _think_ the above is buggy. I believe that the definition
> of the free function for "device_node" is from:
>
> DEFINE_FREE(device_node, struct device_node *, if (_T) of_node_put(_T))
>
> ...which means it'll call of_node_put() to free "i2c_node" when it
> goes out of scope. of_node_put() handles NULL pointers but _not_ ERR
> pointers. So I think that if you get an error back and then return via
> the PTR_ERR(i2c_node) then it'll crash because it will try to free an
> ERR pointer. Did I get that right? Presumably you need to instead do:
>
>   return PTR_ERR(no_free_ptr(i2c_node));
>
> ...or change of_node_put() to be a noop for error pointers?

Good catch! As Andy suggested, it should be updated to handle both.
I'll add a patch for this.

> > +struct i2c_of_probe_ops {
> > +       /**
> > +        * @enable: Retrieve and enable resources so that the components respond to probes.
> > +        *
> > +        * Resources should be reverted to their initial state before returning if this fails.
>
> nit: might be worth explicitly noting that returning -EPROBE_DEFER is
> OK here because this both retrieves resources and enables.

Makes sense. Will do.

> > +        */
> > +       int (*enable)(struct device *dev, struct device_node *bus_node, void *data);
> > +
> > +       /**
> > +        * @cleanup_early: Release exclusive resources prior to enabling component.
>
> nit: change the word "enabling" to "calling probe() on a detected"
> since otherwise it could be confused with the "enable" function above.

Makes sense. Will do.


ChenYu