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

[Chen-Yu Tsai]

On Sat, Sep 14, 2024 at 1:44 AM Doug Anderson <dianders@xxxxxxxxxxxx> wrote:
>
> Hi,
>
> On Wed, Sep 11, 2024 at 12:28 AM Chen-Yu Tsai <wenst@xxxxxxxxxxxx> wrote:
> >
> > Some devices are designed and manufactured with some components having
> > multiple drop-in replacement options. These components are often
> > connected to the mainboard via ribbon cables, having the same signals
> > and pin assignments across all options. These may include the display
> > panel and touchscreen on laptops and tablets, and the trackpad on
> > laptops. Sometimes which component option is used in a particular device
> > can be detected by some firmware provided identifier, other times that
> > information is not available, and the kernel has to try to probe each
> > device.
> >
> > This change attempts to make the "probe each device" case cleaner. The
> > current approach is to have all options added and enabled in the device
> > tree. The kernel would then bind each device and run each driver's probe
> > function. This works, but has been broken before due to the introduction
> > of asynchronous probing, causing multiple instances requesting "shared"
> > resources, such as pinmuxes, GPIO pins, interrupt lines, at the same
> > time, with only one instance succeeding. Work arounds for these include
> > moving the pinmux to the parent I2C controller, using GPIO hogs or
> > pinmux settings to keep the GPIO pins in some fixed configuration, and
> > requesting the interrupt line very late. Such configurations can be seen
> > on the MT8183 Krane Chromebook tablets, and the Qualcomm sc8280xp-based
> > Lenovo Thinkpad 13S.
> >
> > Instead of this delicate dance between drivers and device tree quirks,
> > this change introduces a simple I2C component probe. function For a
>
> s/probe. function/probe function./

Ack.

> > +static int i2c_of_probe_enable_node(struct device *dev, struct device_node *node)
> > +{
> > +       int ret;
> > +
> > +       dev_info(dev, "Enabling %pOF\n", node);
> > +
> > +       struct of_changeset *ocs __free(kfree) = kzalloc(sizeof(*ocs), GFP_KERNEL);
> > +       if (!ocs)
> > +               return -ENOMEM;
>
> I guess the kernel lets you mix code and declarations now? I'm still
> used to all declarations being together but maybe I'm old school... I
> would have put the "dev_info" below the allocation...

AFAIK this is an exception. Excerpt from include/linux/cleanup.h:

    When the unwind order matters it requires that variables be defined
    mid-function scope rather than at the top of the file.

and

    Given that the "__free(...) = NULL" pattern for variables defined at
    the top of the function poses this potential interdependency problem
    the recommendation is to always define and assign variables in one
    statement and not group variable definitions at the top of the
    function when __free() is used.


>
> > +/**
> > + * i2c_of_probe_component() - probe for devices of "type" on the same i2c bus
> > + * @dev: Pointer to the &struct device of the caller, only used for dev_printk() messages.
> > + * @cfg: Pointer to the &struct i2c_of_probe_cfg containing callbacks and other options
> > + *       for the prober.
> > + * @ctx: Context data for callbacks.
> > + *
> > + * Probe for possible I2C components of the same "type" (&i2c_of_probe_cfg->type)
> > + * on the same I2C bus that have their status marked as "fail".
>
> I may have missed it, but originally this was ones marked
> "fail-needs-probe", right? Now it tries all types of fail?
>
>
> > + * 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.

> ...or can it just have a unit address?

IIUC in DT jargon the "node name" does not include the address. The name
including the address is the "full name".

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

> > + *         had multiple types of components to probe, and one of them down
> > + *         the list caused a deferred probe. This is expected behavior.
> > + */
> > +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.

> There are a few other places in the patch series where it feels like
> there are extra arg checks that aren't really needed...
>
>
> > +       ops = cfg->ops ?: &i2c_of_probe_dummy_ops;
> > +       type = cfg->type;
> > +
> > +       i2c_node = i2c_of_probe_get_i2c_node(dev, type);
> > +       if (IS_ERR(i2c_node))
> > +               return PTR_ERR(i2c_node);
> > +
> > +       for_each_child_of_node_with_prefix(i2c_node, node, type) {
>
> I wouldn't object to a comment before this for loop:
>
> /* If any devices of this type are already enabled then the function
> is a noop */
>
> ...or it could be a helper function.

That is what the commeet within the loop is trying to say. I'll move
it before the loop and incorporate your words. The loop can then be
rewritten to return early.

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

> > +/**
> > + * struct i2c_of_probe_ops - I2C OF component prober callbacks
> > + *
> > + * A set of callbacks to be used by i2c_of_probe_component().
> > + *
> > + * All callbacks are optional. Callbacks are called only once per run, and are
> > + * used in the order they are defined in this structure.
> > + *
> > + * All callbacks that have return values shall return %0 on success,
> > + * or a negative error number on failure.
> > + *
> > + * The @dev parameter passed to the callbacks is the same as @dev passed to
> > + * i2c_of_probe_component(). It should only be used for dev_printk() calls
> > + * and nothing else, especially not managed device resource (devres) APIs.
> > + */
> > +struct i2c_of_probe_ops {
> > +       /** @get_resources: Retrieve resources for components. */
> > +       int (*get_resources)(struct device *dev, struct device_node *bus_node, void *data);
> > +
> > +       /** @free_resources_early: Release exclusive resources prior to enabling component. */
> > +       void (*free_resources_early)(void *data);
>
> It would be good if the doc here mentioned what happened if no
> components were found and thus nothing was enabled. Is the function
> still called? It looks like "no" and "cleanup" is in charge of
> cleaning in this case. Feels like the docs need to be more explicit.

Ack.


Thanks
ChenYu