Re: [PATCH v7 06/10] i2c: Introduce OF component probe function

[Doug Anderson <dianders@chromium.org>]

Hi,

On Sun, Sep 15, 2024 at 4:32 AM Chen-Yu Tsai <wenst@chromium.org> wrote:
>
> > > + * Assumes that across the entire device tree the only instances of nodes
> > > + * prefixed with "type" are the ones that need handling for second source
> > > + * components. In other words, if "type" is "touchscreen", then all device
> > > + * nodes named "touchscreen*" are the ones that need probing. There cannot
> >
> > "touchscreen*" implies that it can have an arbitrary suffix. Can it?
>
> That is the idea. The use case is for components that have conflicting
> addresses and need special probing. Such device nodes obviously can't
> have the same node name. This is planned but not implemented in this
> series.

Maybe "touchscreen@*" instead of "touchscreen*" if I'm understanding correctly.


> > > + * be another "touchscreen" node that is already enabled.
> > > + *
> > > + * Assumes that for each "type" of component, only one actually exists. In
> > > + * other words, only one matching and existing device will be enabled.
> > > + *
> > > + * Context: Process context only. Does non-atomic I2C transfers.
> > > + *          Should only be used from a driver probe function, as the function
> > > + *          can return -EPROBE_DEFER if the I2C adapter or other resources
> > > + *          are unavailable.
> > > + * Return: 0 on success or no-op, error code otherwise.
> > > + *         A no-op can happen when it seems like the device tree already
> > > + *         has components of the type to be probed already enabled. This
> > > + *         can happen when the device tree had not been updated to mark
> > > + *         the status of the to-be-probed components as "fail". Or this
> > > + *         function was already run with the same parameters and succeeded
> > > + *         in enabling a component. The latter could happen if the user
> >
> > s/latter/later
>
> Are you sure?

No. latter looked weird to me and I searched quickly and thought I was
right. With a more full search looks like you're right.


> > > +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;
> >
> > Drop extra check of "!cfg". In general kernel conventions don't check
> > for NULL pointers passed by caller unless it's an expected case. You
> > don't check for a NULL "dev" and you shouldn't need to check for a
> > NULL "cfg". They are both simply required parameters.
>
> "dev" is only passed to dev_printk(), and that can handle "dev" being
> NULL. Same can't be said for "cfg".
>
> I don't know what the preference is though. Crashing is probably not the
> nicest thing, even if it only happens to developers.

Honestly as a developer I'd prefer the crash. It points out the exact
line where I had an invalid NULL. Returning an error code means I've
got to compile/boot several more times to track down where the error
code is coming from.

I'm fairly certain that the kernel convention is to only check things
for NULL if it's part of the API to accept NULL or if the value can be
NULL due to untrusted data. If the only way it can be NULL is due to
buggy code elsewhere in the kernel then you should omit the error
checks.

> > > +               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;
> > > +               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;
> >
> > I won't insist, but a part of me wonders whether we should just
> > combine "get_resources" and "enable" and then combine "cleanup" and
> > "free_resources_late". They are always paired one after another and
> > I'm having a hard time seeing why they need to be separate. It's not
> > like you'll ever get the resources and then enable/disable multiple
> > times.
>
> Maybe. The structure was carried over from the original non-callback
> version. I think it's easier to reason about if they are kept separate,
> especially since the outgoing path is slightly different when no working
> component is found and one of the callbacks ends up not getting called.

Actually, both of the outgoing callbacks are always called. It's only
the 3rd callback (the "early" one) that's called sometimes.

I won't insist on combining them, but I still feel like combining them
would be better. I'd be interested in other opinions, though.


-Doug