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?
> + 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_...;
> + 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)
> + 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?
> + 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.
> + * 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?
> +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
