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