Message-ID: <CAD=FV=Udc9aP7bSzTWP82zsaztRD2YnVNpSDA54FC0dKQ-Nz2A@mail.gmail.com>
Date: Fri, 13 Sep 2024 16:43:04 -0700
From: Doug Anderson <dianders@...omium.org>
To: Chen-Yu Tsai <wenst@...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

Hi,

On Wed, Sep 11, 2024 at 12:28 AM Chen-Yu Tsai <wenst@...omium.org> 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./


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


> +/**
> + * 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?
...or can it just have a unit address?


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

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

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.


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


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

Powered by blists - more mailing lists