On Wed, Sep 4, 2024 at 9:47 PM Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx> wrote: > > On Wed, Sep 04, 2024 at 05:00:10PM +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. > > ... > > > +int i2c_of_probe_component(struct device *dev, const char *type) > > +{ > > + struct i2c_adapter *i2c; > > + int ret; > > + > > + struct device_node *i2c_node __free(device_node) = i2c_of_probe_get_i2c_node(dev, type); > > + if (IS_ERR(i2c_node)) > > + return PTR_ERR(i2c_node); > > > + for_each_child_of_node_scoped(i2c_node, node) { > > Hmm, but can it be for_each_child_of_node_with_prefix_scoped() now? Looks like at least one of the fixups got squashed into the wrong patch. I fixed both of them to use for_each_child_of_node_with_prefix(). That isn't compatible with Rob's comment though, and I'm not sure we want a for_each_available_child_of_node_with_prefix() helper just for one instance? > > + if (!of_node_name_prefix(node, type)) > > + continue; > > + 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. > > + */ > > + return 0; > > + } > > + > > + i2c = of_get_i2c_adapter_by_node(i2c_node); > > + if (!i2c) > > + return dev_err_probe(dev, -EPROBE_DEFER, "Couldn't get I2C adapter\n"); > > + > > + ret = 0; > > + for_each_child_of_node_scoped(i2c_node, node) { > > Ditto. > > > + union i2c_smbus_data data; > > + u32 addr; > > + > > + if (!of_node_name_prefix(node, type)) > > + continue; > > + 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 */ > > + ret = i2c_of_probe_enable_node(dev, node); > > + break; > > + } > > + > > + i2c_put_adapter(i2c); > > + > > + return ret; > > +} > > ... > > > +EXPORT_SYMBOL_GPL(i2c_of_probe_component); > > Wonder if we may already use namespaced export from day 1. I think that would be a good idea, especially given Doug's request for exposing more helpers. Thanks ChenYu