| Message ID | 20231128084236.157152-3-wenst@chromium.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | platform/chrome: Introduce DT hardware prober | expand |
On Tue, Nov 28, 2023 at 04:42:31PM +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. ... > +/** > + * i2c_of_probe_component() - probe for devices of "type" on the same i2c bus > + * @dev: &struct device of the caller, only used for dev_* printk messages > + * @type: a string to match the device node name prefix to probe for > + * > + * Probe for possible I2C components of the same "type" on the same I2C bus > + * that have their status marked as "fail". Definitely you haven't run kernel-doc validation. > + */ ... > + return dev_err_probe(dev, -ENODEV, "Could not find %s device node\n", type); I haven't noticed clear statement in the description that this API is only for the ->probe() stages. ... > + if (i2c_smbus_xfer(i2c, addr, 0, I2C_SMBUS_READ, 0, I2C_SMBUS_BYTE, &data) < 0) > + continue; This will require the device to be powered on. Are you sure it will be always the case?
On Wed, Nov 29, 2023 at 12:22 AM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Tue, Nov 28, 2023 at 04:42:31PM +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. > > ... > > > +/** > > + * i2c_of_probe_component() - probe for devices of "type" on the same i2c bus > > + * @dev: &struct device of the caller, only used for dev_* printk messages > > + * @type: a string to match the device node name prefix to probe for > > + * > > + * Probe for possible I2C components of the same "type" on the same I2C bus > > + * that have their status marked as "fail". > > Definitely you haven't run kernel-doc validation. Right. Will add missing parts. > > + */ > > ... > > > + return dev_err_probe(dev, -ENODEV, "Could not find %s device node\n", type); > > I haven't noticed clear statement in the description that this API is only for > the ->probe() stages. Will add that to the Context part of the kernel-doc. > ... > > > + if (i2c_smbus_xfer(i2c, addr, 0, I2C_SMBUS_READ, 0, I2C_SMBUS_BYTE, &data) < 0) > > + continue; > > This will require the device to be powered on. Are you sure it will be always > the case? This is left as TODO. The devices I have tie the component power supplies to an always on power rail. I guess I could get a trace of the function calls to see if things are running as they should. Not sure if that is enough? ChenYu
Hi, On Tue, Nov 28, 2023 at 12:45 AM Chen-Yu Tsai <wenst@chromium.org> wrote: > > @@ -217,4 +217,114 @@ static int of_i2c_notify(struct notifier_block *nb, unsigned long action, > struct notifier_block i2c_of_notifier = { > .notifier_call = of_i2c_notify, > }; > + > +/* > + * Some devices, such as Google Hana Chromebooks, are produced by multiple > + * vendors each using their preferred components. Such components are all > + * in the device tree. Instead of having all of them enabled and having each > + * driver separately try and probe its device while fighting over shared > + * resources, they can be marked as "fail-needs-probe" and have a prober > + * figure out which one is actually used beforehand. > + * > + * This prober assumes such drop-in parts are on the same I2C bus, have > + * non-conflicting addresses, and can be directly probed by seeing which > + * address responds. > + * > + * TODO: > + * - Support handling common regulators and GPIOs. IMO you should prototype how you're going to handle regulators and GPIOs before finalizing the design. I was going to write that you should just document that it was up to the caller to power things up before calling this function, but then I realized that the caller would have to duplicate much of this function in order to do so. In the very least they'd have to find the nodes of the relevant devices so that they could grab regulators and/or GPIOs. In order to avoid this duplication, would the design need to change? Perhaps this would be as simple as adding a callback function here that's called with all of the nodes before probing? If that's right, it would be nice to have that callback from the beginning so we don't need two variants of the function... > + * - Support I2C muxes > + */ > + > +/** > + * i2c_of_probe_component() - probe for devices of "type" on the same i2c bus > + * @dev: &struct device of the caller, only used for dev_* printk messages > + * @type: a string to match the device node name prefix to probe for > + * > + * Probe for possible I2C components of the same "type" on the same I2C bus > + * that have their status marked as "fail". Should document these current limitations with the code: * Assumes that across the entire device tree the only instances of nodes named "type" are ones we're trying to handle second sourcing for. In other words if we're searching for "touchscreen" then all nodes named "touchscreen" are ones that need to be probed. * Assumes that there is exactly one group of each "type". In other words, if we're searching for "touchscreen" then exactly one touchscreen will be enabled across the whole tree. Obviously those could be relaxed with more code, but that's the current limitation and it makes the code easier to understand with that context. > + */ > +int i2c_of_probe_component(struct device *dev, const char *type) > +{ > + struct device_node *node, *i2c_node; > + struct i2c_adapter *i2c; > + struct of_changeset *ocs = NULL; > + int ret; > + > + node = of_find_node_by_name(NULL, type); > + if (!node) > + return dev_err_probe(dev, -ENODEV, "Could not find %s device node\n", type); > + > + i2c_node = of_get_next_parent(node); > + if (!of_node_name_eq(i2c_node, "i2c")) { > + of_node_put(i2c_node); > + return dev_err_probe(dev, -EINVAL, "%s device isn't on I2C bus\n", type); > + } Personally I'd skip checking for the "i2c" node name. Just rely on of_get_i2c_adapter_by_node() returning an error. Oh, I guess you have this because you need to tell the difference between -EINVAL and -EPROBE_DEFER? It feels like instead you could use the firmware node to lookup a device more generically. If a device isn't associated with the node at all then you return -EPROBE_DEFER. Otherwise if it's associated but not an i2c device then you return -EINVAL. I guess maybe it doesn't make a huge difference, but it somehow feels less fragile than relying on the node name being "i2c". Maybe I just haven't had enough DT Kool-Aid. One thing this made me wonder is if of_get_i2c_adapter_by_node() is race free anyway. Can't that return you a device that hasn't finished probing yet? I see: - i2c_register_adapter() -- device_register() --- device_add() ---- bus_add_device() ---- bus_probe_device() As soon as bus_add_device() is called then it will be in "klist_devices" and I believe i2c_find_device_by_fwnode() will be able to find it. However, it hasn't necessarily been probed yet. I think that means calling i2c_smbus_xfer() on it might not work yet... One last thing is that you should check to make sure that the i2c adapter is not marked "disabled". ...otherwise I think you'd end up constantly trying again and again... > + for_each_child_of_node(i2c_node, node) { > + if (!of_node_name_prefix(node, type)) > + continue; > + if (of_device_is_available(node)) { > + /* > + * Device tree has component already enabled. Either the > + * device tree isn't supported or we already probed once. I guess the "already probed once" is somewhat expected if more than one type of second source component is defined and we end up deferring the second one? We don't undo the resolution of the first one and probably don't keep track of the fact that it succeeded? Probably should be added to the function comments that this is an expected/normal case? > + */ > + of_node_put(node); > + of_node_put(i2c_node); > + return 0; > + } > + } > + > + i2c = of_get_i2c_adapter_by_node(i2c_node); > + if (!i2c) { > + of_node_put(i2c_node); > + return dev_err_probe(dev, -EPROBE_DEFER, "Couldn't get I2C adapter\n"); > + } > + > + ret = 0; Why init ret to 0? > + for_each_child_of_node(i2c_node, node) { > + 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) I'd be tempted to say that the caller should be able to pass in a function pointer here so they could use an alternative method to probe instead of i2c_smbus_xfer(), though you'd want to make it easy to default to i2c_smbus_xfer(). I could imagine someone might need a different way to probe. For instance if you had two touchscreens both at the same "reg" but that had different "hid-descr-addr" then this could be important. > + continue; > + Put the "break" right here. You've found the device and that was the point of the loop. Once you do that then the error handling makes a little more sense. It was weird that the error handling was jumped through from inside the loop... > + dev_info(dev, "Enabling %pOF\n", node); > + > + ocs = kzalloc(sizeof(*ocs), GFP_KERNEL); > + if (!ocs) { > + ret = -ENOMEM; > + goto err_put_node; I think this error path (and some others) miss "i2c_put_adapter()" and "of_node_put(i2c_node)"
On Sat, Dec 2, 2023 at 8:58 AM Doug Anderson <dianders@chromium.org> wrote: > > Hi, > > On Tue, Nov 28, 2023 at 12:45 AM Chen-Yu Tsai <wenst@chromium.org> wrote: > > > > @@ -217,4 +217,114 @@ static int of_i2c_notify(struct notifier_block *nb, unsigned long action, > > struct notifier_block i2c_of_notifier = { > > .notifier_call = of_i2c_notify, > > }; > > + > > +/* > > + * Some devices, such as Google Hana Chromebooks, are produced by multiple > > + * vendors each using their preferred components. Such components are all > > + * in the device tree. Instead of having all of them enabled and having each > > + * driver separately try and probe its device while fighting over shared > > + * resources, they can be marked as "fail-needs-probe" and have a prober > > + * figure out which one is actually used beforehand. > > + * > > + * This prober assumes such drop-in parts are on the same I2C bus, have > > + * non-conflicting addresses, and can be directly probed by seeing which > > + * address responds. > > + * > > + * TODO: > > + * - Support handling common regulators and GPIOs. > > IMO you should prototype how you're going to handle regulators and > GPIOs before finalizing the design. I was going to write that you > should just document that it was up to the caller to power things up > before calling this function, but then I realized that the caller > would have to duplicate much of this function in order to do so. In > the very least they'd have to find the nodes of the relevant devices > so that they could grab regulators and/or GPIOs. In order to avoid > this duplication, would the design need to change? Perhaps this would > be as simple as adding a callback function here that's called with all > of the nodes before probing? If that's right, it would be nice to have > that callback from the beginning so we don't need two variants of the > function... So I think I can prototype designs with one GPIO and multiple regulators, assuming each node has the same number of both? At least they should if they're on the same connector. More than one GPIO probably means there are some ordering and timing constraints, and won't be as generic. > > + * - Support I2C muxes > > + */ > > + > > +/** > > + * i2c_of_probe_component() - probe for devices of "type" on the same i2c bus > > + * @dev: &struct device of the caller, only used for dev_* printk messages > > + * @type: a string to match the device node name prefix to probe for > > + * > > + * Probe for possible I2C components of the same "type" on the same I2C bus > > + * that have their status marked as "fail". > > Should document these current limitations with the code: > > * Assumes that across the entire device tree the only instances of > nodes named "type" are ones we're trying to handle second sourcing > for. In other words if we're searching for "touchscreen" then all > nodes named "touchscreen" are ones that need to be probed. > > * Assumes that there is exactly one group of each "type". In other > words, if we're searching for "touchscreen" then exactly one > touchscreen will be enabled across the whole tree. > > Obviously those could be relaxed with more code, but that's the > current limitation and it makes the code easier to understand with > that context. Done. > > + */ > > +int i2c_of_probe_component(struct device *dev, const char *type) > > +{ > > + struct device_node *node, *i2c_node; > > + struct i2c_adapter *i2c; > > + struct of_changeset *ocs = NULL; > > + int ret; > > + > > + node = of_find_node_by_name(NULL, type); > > + if (!node) > > + return dev_err_probe(dev, -ENODEV, "Could not find %s device node\n", type); > > + > > + i2c_node = of_get_next_parent(node); > > + if (!of_node_name_eq(i2c_node, "i2c")) { > > + of_node_put(i2c_node); > > + return dev_err_probe(dev, -EINVAL, "%s device isn't on I2C bus\n", type); > > + } > > Personally I'd skip checking for the "i2c" node name. Just rely on > of_get_i2c_adapter_by_node() returning an error. > > Oh, I guess you have this because you need to tell the difference > between -EINVAL and -EPROBE_DEFER? It feels like instead you could use > the firmware node to lookup a device more generically. If a device > isn't associated with the node at all then you return -EPROBE_DEFER. > Otherwise if it's associated but not an i2c device then you return > -EINVAL. I guess maybe it doesn't make a huge difference, but it > somehow feels less fragile than relying on the node name being "i2c". > Maybe I just haven't had enough DT Kool-Aid. The current way it's written is to do the device tree structure checks first before doing any driver model access. Also it needs to tell (as you mentioned later) if the i2c adapter is "disabled" or just not probed yet. > One thing this made me wonder is if of_get_i2c_adapter_by_node() is > race free anyway. Can't that return you a device that hasn't finished > probing yet? I see: > > - i2c_register_adapter() > -- device_register() > --- device_add() > ---- bus_add_device() > ---- bus_probe_device() > > As soon as bus_add_device() is called then it will be in > "klist_devices" and I believe i2c_find_device_by_fwnode() will be able > to find it. However, it hasn't necessarily been probed yet. I think > that means calling i2c_smbus_xfer() on it might not work yet... It does look like there's a small window between the device_register() and when the i2c adapter is ready, i.e. can't bail out on an error. I guess it needs either a flag or some reordering of the code. It looks like Wolfram will be at OSS JP. I'll try and have a chat about the whole thing. > One last thing is that you should check to make sure that the i2c > adapter is not marked "disabled". ...otherwise I think you'd end up > constantly trying again and again... Yeah. I added a check for that as well. > > + for_each_child_of_node(i2c_node, node) { > > + if (!of_node_name_prefix(node, type)) > > + continue; > > + if (of_device_is_available(node)) { > > + /* > > + * Device tree has component already enabled. Either the > > + * device tree isn't supported or we already probed once. > > I guess the "already probed once" is somewhat expected if more than > one type of second source component is defined and we end up deferring > the second one? We don't undo the resolution of the first one and > probably don't keep track of the fact that it succeeded? That's right. > Probably should be added to the function comments that this is an > expected/normal case? Added to the Return: section of the kernel-doc. > > + */ > > + of_node_put(node); > > + of_node_put(i2c_node); > > + return 0; > > + } > > + } > > + > > + i2c = of_get_i2c_adapter_by_node(i2c_node); > > + if (!i2c) { > > + of_node_put(i2c_node); > > + return dev_err_probe(dev, -EPROBE_DEFER, "Couldn't get I2C adapter\n"); > > + } > > + > > + ret = 0; > > Why init ret to 0? It was requested by Andy. I believe the reason was having the initial value used for bailout closer was better. > > + for_each_child_of_node(i2c_node, node) { > > + 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) > > I'd be tempted to say that the caller should be able to pass in a > function pointer here so they could use an alternative method to probe > instead of i2c_smbus_xfer(), though you'd want to make it easy to > default to i2c_smbus_xfer(). I could imagine someone might need a > different way to probe. For instance if you had two touchscreens both > at the same "reg" but that had different "hid-descr-addr" then this > could be important. I'd say the only specific probable type is hid-i2c. And that could be generic enough that we could incorporate it here if we wanted. However I think we want to keep the initial version a bit simpler. > > + continue; > > + > > Put the "break" right here. You've found the device and that was the > point of the loop. In its place we'd have an if (node) { <enable node> } block. I guess it makes it easier to read still? > Once you do that then the error handling makes a little more sense. It > was weird that the error handling was jumped through from inside the > loop... > > > > + dev_info(dev, "Enabling %pOF\n", node); > > + > > + ocs = kzalloc(sizeof(*ocs), GFP_KERNEL); > > + if (!ocs) { > > + ret = -ENOMEM; > > + goto err_put_node; > > I think this error path (and some others) miss "i2c_put_adapter()" and > "of_node_put(i2c_node)" Right. I'll check. Thanks ChenYu
Hi, On Mon, Dec 4, 2023 at 1:53 AM Chen-Yu Tsai <wenst@chromium.org> wrote: > > > IMO you should prototype how you're going to handle regulators and > > GPIOs before finalizing the design. I was going to write that you > > should just document that it was up to the caller to power things up > > before calling this function, but then I realized that the caller > > would have to duplicate much of this function in order to do so. In > > the very least they'd have to find the nodes of the relevant devices > > so that they could grab regulators and/or GPIOs. In order to avoid > > this duplication, would the design need to change? Perhaps this would > > be as simple as adding a callback function here that's called with all > > of the nodes before probing? If that's right, it would be nice to have > > that callback from the beginning so we don't need two variants of the > > function... > > So I think I can prototype designs with one GPIO and multiple regulators, > assuming each node has the same number of both? At least they should if > they're on the same connector. > > More than one GPIO probably means there are some ordering and timing > constraints, and won't be as generic. I was hoping to see a prototype of how this could work in the non-generic case where the board needed a custom function to power things up. It seems like we'd still want to be able to use your code for probing. > > > + for_each_child_of_node(i2c_node, node) { > > > + 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) > > > > I'd be tempted to say that the caller should be able to pass in a > > function pointer here so they could use an alternative method to probe > > instead of i2c_smbus_xfer(), though you'd want to make it easy to > > default to i2c_smbus_xfer(). I could imagine someone might need a > > different way to probe. For instance if you had two touchscreens both > > at the same "reg" but that had different "hid-descr-addr" then this > > could be important. > > I'd say the only specific probable type is hid-i2c. And that could be > generic enough that we could incorporate it here if we wanted. However > I think we want to keep the initial version a bit simpler. I don't mind if the initial version is simpler, but I'd love to understand how this will grow. It doesn't feel terrible to take in a function pointer that will probe the device and then provide a function that callers could pass in that simply did the simple i2c_smbus_xfer(). > > > + continue; > > > + > > > > Put the "break" right here. You've found the device and that was the > > point of the loop. > > In its place we'd have an if (node) { <enable node> } block. I guess it > makes it easier to read still? ...or perhaps an "if (!node) goto exit" block and then you don't need indentation? Essentially the loop becomes the implementation: "node = find_the_one_that_exists(...)". -Doug
On Fri, Dec 01, 2023 at 04:57:46PM -0800, Doug Anderson wrote: > Hi, > > On Tue, Nov 28, 2023 at 12:45 AM Chen-Yu Tsai <wenst@chromium.org> wrote: > > > > @@ -217,4 +217,114 @@ static int of_i2c_notify(struct notifier_block *nb, unsigned long action, > > struct notifier_block i2c_of_notifier = { > > .notifier_call = of_i2c_notify, > > }; > > + > > +/* > > + * Some devices, such as Google Hana Chromebooks, are produced by multiple > > + * vendors each using their preferred components. Such components are all > > + * in the device tree. Instead of having all of them enabled and having each > > + * driver separately try and probe its device while fighting over shared > > + * resources, they can be marked as "fail-needs-probe" and have a prober > > + * figure out which one is actually used beforehand. > > + * > > + * This prober assumes such drop-in parts are on the same I2C bus, have > > + * non-conflicting addresses, and can be directly probed by seeing which > > + * address responds. > > + * > > + * TODO: > > + * - Support handling common regulators and GPIOs. > > IMO you should prototype how you're going to handle regulators and > GPIOs before finalizing the design. I was going to write that you > should just document that it was up to the caller to power things up > before calling this function, but then I realized that the caller > would have to duplicate much of this function in order to do so. In > the very least they'd have to find the nodes of the relevant devices > so that they could grab regulators and/or GPIOs. In order to avoid > this duplication, would the design need to change? Perhaps this would > be as simple as adding a callback function here that's called with all > of the nodes before probing? If that's right, it would be nice to have > that callback from the beginning so we don't need two variants of the > function... > > > + * - Support I2C muxes > > + */ > > + > > +/** > > + * i2c_of_probe_component() - probe for devices of "type" on the same i2c bus > > + * @dev: &struct device of the caller, only used for dev_* printk messages > > + * @type: a string to match the device node name prefix to probe for > > + * > > + * Probe for possible I2C components of the same "type" on the same I2C bus > > + * that have their status marked as "fail". > > Should document these current limitations with the code: > > * Assumes that across the entire device tree the only instances of > nodes named "type" are ones we're trying to handle second sourcing > for. In other words if we're searching for "touchscreen" then all > nodes named "touchscreen" are ones that need to be probed. named "type" and marked as needs probe. > > * Assumes that there is exactly one group of each "type". In other > words, if we're searching for "touchscreen" then exactly one > touchscreen will be enabled across the whole tree. Does that need to be a limitation? If you just keep going thru all devices, wouldn't that just work? Rob
diff --git a/drivers/i2c/i2c-core-of.c b/drivers/i2c/i2c-core-of.c index a6c407d36800..3a0b4986c585 100644 --- a/drivers/i2c/i2c-core-of.c +++ b/drivers/i2c/i2c-core-of.c @@ -217,4 +217,114 @@ static int of_i2c_notify(struct notifier_block *nb, unsigned long action, struct notifier_block i2c_of_notifier = { .notifier_call = of_i2c_notify, }; + +/* + * Some devices, such as Google Hana Chromebooks, are produced by multiple + * vendors each using their preferred components. Such components are all + * in the device tree. Instead of having all of them enabled and having each + * driver separately try and probe its device while fighting over shared + * resources, they can be marked as "fail-needs-probe" and have a prober + * figure out which one is actually used beforehand. + * + * This prober assumes such drop-in parts are on the same I2C bus, have + * non-conflicting addresses, and can be directly probed by seeing which + * address responds. + * + * TODO: + * - Support handling common regulators and GPIOs. + * - Support I2C muxes + */ + +/** + * i2c_of_probe_component() - probe for devices of "type" on the same i2c bus + * @dev: &struct device of the caller, only used for dev_* printk messages + * @type: a string to match the device node name prefix to probe for + * + * Probe for possible I2C components of the same "type" on the same I2C bus + * that have their status marked as "fail". + */ +int i2c_of_probe_component(struct device *dev, const char *type) +{ + struct device_node *node, *i2c_node; + struct i2c_adapter *i2c; + struct of_changeset *ocs = NULL; + int ret; + + node = of_find_node_by_name(NULL, type); + if (!node) + return dev_err_probe(dev, -ENODEV, "Could not find %s device node\n", type); + + i2c_node = of_get_next_parent(node); + if (!of_node_name_eq(i2c_node, "i2c")) { + of_node_put(i2c_node); + return dev_err_probe(dev, -EINVAL, "%s device isn't on I2C bus\n", type); + } + + for_each_child_of_node(i2c_node, node) { + if (!of_node_name_prefix(node, type)) + continue; + if (of_device_is_available(node)) { + /* + * Device tree has component already enabled. Either the + * device tree isn't supported or we already probed once. + */ + of_node_put(node); + of_node_put(i2c_node); + return 0; + } + } + + i2c = of_get_i2c_adapter_by_node(i2c_node); + if (!i2c) { + of_node_put(i2c_node); + return dev_err_probe(dev, -EPROBE_DEFER, "Couldn't get I2C adapter\n"); + } + + ret = 0; + for_each_child_of_node(i2c_node, node) { + 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; + + dev_info(dev, "Enabling %pOF\n", node); + + ocs = kzalloc(sizeof(*ocs), GFP_KERNEL); + if (!ocs) { + ret = -ENOMEM; + goto err_put_node; + } + + /* Found a device that is responding */ + of_changeset_init(ocs); + ret = of_changeset_update_prop_string(ocs, node, "status", "okay"); + if (ret) + goto err_free_ocs; + ret = of_changeset_apply(ocs); + if (ret) + goto err_destroy_ocs; + + of_node_put(node); + break; + } + + i2c_put_adapter(i2c); + of_node_put(i2c_node); + + return 0; + +err_destroy_ocs: + of_changeset_destroy(ocs); +err_free_ocs: + kfree(ocs); +err_put_node: + of_node_put(node); + return ret; +} +EXPORT_SYMBOL_GPL(i2c_of_probe_component); #endif /* CONFIG_OF_DYNAMIC */ diff --git a/include/linux/i2c.h b/include/linux/i2c.h index 0dae9db27538..75fbbd5a4b15 100644 --- a/include/linux/i2c.h +++ b/include/linux/i2c.h @@ -997,6 +997,10 @@ const struct of_device_id int of_i2c_get_board_info(struct device *dev, struct device_node *node, struct i2c_board_info *info); +#if IS_ENABLED(CONFIG_OF_DYNAMIC) +int i2c_of_probe_component(struct device *dev, const char *type); +#endif + #else static inline struct i2c_client *of_find_i2c_device_by_node(struct device_node *node)
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. Signed-off-by: Chen-Yu Tsai <wenst@chromium.org> --- Changes since v2: - New patch split out from "of: Introduce hardware prober driver" - Addressed Rob's comments - Move i2c prober to i2c subsystem - Use of_node_is_available() to check if node is enabled. - Use OF changeset API to update status property - Addressed Andy's comments - Probe function now accepts "struct device *dev" instead to reduce line length and dereferences - Move "ret = 0" to just before for_each_child_of_node(i2c_node, node) --- drivers/i2c/i2c-core-of.c | 110 ++++++++++++++++++++++++++++++++++++++ include/linux/i2c.h | 4 ++ 2 files changed, 114 insertions(+)