Message-ID: <CAGXv+5Eo3r6WPDnHwEZPFz7PBd5v48n32iXXWy=Vr-v1H2zEoQ@mail.gmail.com>
Date: Mon, 16 Sep 2024 16:31:09 +0200
From: Chen-Yu Tsai <wenst@...omium.org>
To: Doug Anderson <dianders@...omium.org>
Cc: Rob Herring <robh@...nel.org>, Saravana Kannan <saravanak@...gle.com>, 
	Matthias Brugger <matthias.bgg@...il.com>, 
	AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>, Wolfram Sang <wsa@...nel.org>, 
	Benson Leung <bleung@...omium.org>, Tzung-Bi Shih <tzungbi@...nel.org>, 
	Mark Brown <broonie@...nel.org>, Liam Girdwood <lgirdwood@...il.com>, 
	chrome-platform@...ts.linux.dev, devicetree@...r.kernel.org, 
	linux-arm-kernel@...ts.infradead.org, linux-mediatek@...ts.infradead.org, 
	linux-kernel@...r.kernel.org, Johan Hovold <johan@...nel.org>, 
	Jiri Kosina <jikos@...nel.org>, Andy Shevchenko <andriy.shevchenko@...ux.intel.com>, 
	linux-i2c@...r.kernel.org
Subject: Re: [PATCH v7 06/10] i2c: Introduce OF component probe function

On Mon, Sep 16, 2024 at 4:16 PM Doug Anderson <dianders@...omium.org> wrote:
>
> Hi,
>
> On Sun, Sep 15, 2024 at 4:32 AM Chen-Yu Tsai <wenst@...omium.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.

Then it would be "touchscreen*@*".

> > > > + * 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.

Make sense.

> > > > +               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.

So IIRC do "get + enable" and "cleanup + release_late", and leave
"release_early" alone?

ChenYu

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

Powered by blists - more mailing lists