Message-ID: <CAGXv+5HgkCZ=vdHGgvCW1U-nid=cQrVaxC+V+H2Gknf2pnTbYA@mail.gmail.com>
Date: Sun, 15 Sep 2024 12:44:13 +0200
From: Chen-Yu Tsai <wenst@...omium.org>
To: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
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, Douglas Anderson <dianders@...omium.org>, 
	Johan Hovold <johan@...nel.org>, Jiri Kosina <jikos@...nel.org>, linux-i2c@...r.kernel.org
Subject: Re: [PATCH v7 06/10] i2c: Introduce OF component probe function

On Fri, Sep 13, 2024 at 12:25 PM Andy Shevchenko
<andriy.shevchenko@...ux.intel.com> wrote:
>
> On Wed, Sep 11, 2024 at 03:27:44PM +0800, Chen-Yu Tsai 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
> > given class of devices on the same I2C bus, it will go through all of
> > them, doing a simple I2C read transfer and see which one of them responds.
> > It will then enable the device that responds.
> >
> > This requires some minor modifications in the existing device tree. The
> > status for all the device nodes for the component options must be set
> > to "failed-needs-probe". This makes it clear that some mechanism is
> > needed to enable one of them, and also prevents the prober and device
> > drivers running at the same time.
>
> ...
>
> > +static int i2c_of_probe_enable_node(struct device *dev, struct device_node *node)
> > +{
> > +     int ret;
>
> > +     dev_info(dev, "Enabling %pOF\n", node);
>
> Is it important to be on INFO level?

Not really.

> > +     struct of_changeset *ocs __free(kfree) = kzalloc(sizeof(*ocs), GFP_KERNEL);
> > +     if (!ocs)
> > +             return -ENOMEM;
> > +
> > +     of_changeset_init(ocs);
> > +     ret = of_changeset_update_prop_string(ocs, node, "status", "okay");
> > +     if (ret)
> > +             return ret;
> > +
> > +     ret = of_changeset_apply(ocs);
> > +     if (ret) {
> > +             /* ocs needs to be explicitly cleaned up before being freed. */
> > +             of_changeset_destroy(ocs);
> > +     } else {
> > +             /*
> > +              * ocs is intentionally kept around as it needs to
> > +              * exist as long as the change is applied.
> > +              */
> > +             void *ptr __always_unused = no_free_ptr(ocs);
> > +     }
> > +
> > +     return ret;
> > +}
>
> ...
>
> > +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;
> > +
> > +     ops = cfg->ops ?: &i2c_of_probe_dummy_ops;
> > +     type = cfg->type;
> > +
> > +     i2c_node = i2c_of_probe_get_i2c_node(dev, type);
>
>
>         struct device_node *i2c_node __free(of_node_put) =
>                 i2c_...;

cleanup.h says to not mix the two styles (scoped vs goto). I was trying
to follow that, though I realize now that with the scoped loops it
probably doesn't help.

I'll revert back to having __free().

> > +     if (IS_ERR(i2c_node))
> > +             return PTR_ERR(i2c_node);
> > +
> > +     for_each_child_of_node_with_prefix(i2c_node, node, type) {
> > +             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;
>
> Shouldn't you drop reference count for "node"? (See also below)

This for-each loop the "scoped". It just doesn't have the prefix anymore.
I believe you asked if the prefix could be dropped and then Rob agreed.

> > +             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;
> > +
> > +     ret = 0;
> > +     for_each_child_of_node_with_prefix(i2c_node, node, type) {
> > +             union i2c_smbus_data data;
> > +             u32 addr;
> > +
> > +             if (of_property_read_u32(node, "reg", &addr))
> > +                     continue;
> > +             if (i2c_smbus_xfer(i2c, addr, 0, I2C_SMBUS_READ, 0, I2C_SMBUS_BYTE, &data) < 0)
> > +                     continue;
> > +
> > +             /* Found a device that is responding */
> > +             if (ops->free_resources_early)
> > +                     ops->free_resources_early(ctx);
> > +             ret = i2c_of_probe_enable_node(dev, node);
>
> Hmm... Is "node" reference count left bumped up for a reason?

Same as above.

> > +             break;
> > +     }
> > +
> > +     if (ops->cleanup)
> > +             ops->cleanup(dev, ctx);
> > +out_release_resources:
> > +     if (ops->free_resources_late)
> > +             ops->free_resources_late(ctx);
> > +out_put_i2c_adapter:
> > +     i2c_put_adapter(i2c);
> > +out_put_i2c_node:
> > +     of_node_put(i2c_node);
> > +
> > +     return ret;
> > +}
>
> ...
>
> > +/*
> > + * i2c-of-prober.h - definitions for the Linux I2C OF component prober
>
> Please avoid putting filenames inside files. In the possible future event of
> file renaming this may become a burden and sometimes even forgotten.

Ack.

> > + * Copyright (C) 2024 Google LLC
> > + */
> > +
> > +#ifndef _LINUX_I2C_OF_PROBER_H
> > +#define _LINUX_I2C_OF_PROBER_H
>
> > +#if IS_ENABLED(CONFIG_OF_DYNAMIC)
>
> Do you really need to hide data types with this? Wouldn't be enough to hide
> APIs only?

Ack. Will move the data types outside.


Thanks
ChenYu


> > +struct device;
> > +struct device_node;
> > +
> > +/**
> > + * 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);
> > +
> > +     /**
> > +      * @enable: Enable resources so that the components respond to probes.
> > +      *
> > +      * Resources should be reverted to their initial state before returning if this fails.
> > +      */
> > +     int (*enable)(struct device *dev, void *data);
> > +
> > +     /**
> > +      * @cleanup: Opposite of @enable to balance refcounts after probing.
> > +      *
> > +      * Can not operate on resources already freed in @free_resources_early.
> > +      */
> > +     int (*cleanup)(struct device *dev, void *data);
> > +
> > +     /**
> > +      * @free_resources_late: Release all resources, including those that would have
> > +      *                       been released by @free_resources_early.
> > +      */
> > +     void (*free_resources_late)(void *data);
> > +};
> > +
> > +/**
> > + * struct i2c_of_probe_cfg - I2C OF component prober configuration
> > + * @ops: Callbacks for the prober to use.
> > + * @type: A string to match the device node name prefix to probe for.
> > + */
> > +struct i2c_of_probe_cfg {
> > +     const struct i2c_of_probe_ops *ops;
> > +     const char *type;
> > +};
> > +
> > +int i2c_of_probe_component(struct device *dev, const struct i2c_of_probe_cfg *cfg, void *ctx);
> > +
> > +#endif /* IS_ENABLED(CONFIG_OF_DYNAMIC) */
> > +
> > +#endif /* _LINUX_I2C_OF_PROBER_H */
>
> --
> With Best Regards,
> Andy Shevchenko
>
>

Powered by blists - more mailing lists