| Message ID | 20240904090016.2841572-10-wenst@chromium.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | platform/chrome: Introduce DT hardware prober | expand |
On Wed, Sep 04, 2024 at 05:00:11PM +0800, Chen-Yu Tsai wrote: > This adds regulator management to the I2C OF component prober. > Components that the prober intends to probe likely require their > regulator supplies be enabled, and GPIOs be toggled to enable them or > bring them out of reset before they will respond to probe attempts. > GPIOs will be handled in the next patch. > > Without specific knowledge of each component's resource names or > power sequencing requirements, the prober can only enable the > regulator supplies all at once, and toggle the GPIOs all at once. > Luckily, reset pins tend to be active low, while enable pins tend to > be active high, so setting the raw status of all GPIO pins to high > should work. The wait time before and after resources are enabled > are collected from existing drivers and device trees. > > The prober collects resources from all possible components and enables > them together, instead of enabling resources and probing each component > one by one. The latter approach does not provide any boot time benefits > over simply enabling each component and letting each driver probe > sequentially. > > The prober will also deduplicate the resources, since on a component > swap out or co-layout design, the resources are always the same. > While duplicate regulator supplies won't cause much issue, shared > GPIOs don't work reliably, especially with other drivers. For the > same reason, the prober will release the GPIOs before the successfully > probed component is actually enabled. ... > +static int i2c_of_probe_get_regulators(struct device *dev, struct device_node *node, > + struct i2c_of_probe_data *data) > +{ > + struct regulator_bulk_data *tmp, *new_regulators; > + int ret; > + > + ret = of_regulator_bulk_get_all(dev, node, &tmp); > + if (ret < 0) { > + return ret; > + } else if (ret == 0) { > + /* > + * It's entirely possible for a device node to not have > + * regulator supplies. While it doesn't make sense from > + * a hardware perspective, the supplies could be always > + * on or otherwise not modeled in the device tree, but > + * the device would still work. > + */ > + return ret; > + } if (ret < 0) return ret; /* * It's entirely possible for a device node to not have regulator * supplies. While it doesn't make sense from a hardware perspective, * the supplies could be always on or otherwise not modeled in * the device tree, but the device would still work. */ if (ret == 0) return ret; > + if (!data->regulators) { > + data->regulators = tmp; > + data->regulators_num = ret; > + return ret; > + }; > + > + new_regulators = krealloc_array(data->regulators, (data->regulators_num + ret), Redundant parentheses. > + sizeof(*tmp), GFP_KERNEL); > + if (!new_regulators) { > + regulator_bulk_free(ret, tmp); > + return -ENOMEM; > + } > + > + data->regulators = new_regulators; > + memcpy(&data->regulators[data->regulators_num], tmp, sizeof(*tmp) * ret); Shouldn't be the size calculated based on the size of the destination? > + data->regulators_num += ret; > + > + return ret; > +} ... As I said earlier my main concern is that timeout heuristic which seems fragile. But I have no ideas to propose, leave this to others to comment on / think about.
Hi, On Wed, Sep 4, 2024 at 2:01 AM Chen-Yu Tsai <wenst@chromium.org> wrote: > > This adds regulator management to the I2C OF component prober. > Components that the prober intends to probe likely require their > regulator supplies be enabled, and GPIOs be toggled to enable them or > bring them out of reset before they will respond to probe attempts. > GPIOs will be handled in the next patch. > > Without specific knowledge of each component's resource names or > power sequencing requirements, the prober can only enable the > regulator supplies all at once, and toggle the GPIOs all at once. > Luckily, reset pins tend to be active low, while enable pins tend to > be active high, so setting the raw status of all GPIO pins to high > should work. The wait time before and after resources are enabled > are collected from existing drivers and device trees. > > The prober collects resources from all possible components and enables > them together, instead of enabling resources and probing each component > one by one. The latter approach does not provide any boot time benefits > over simply enabling each component and letting each driver probe > sequentially. > > The prober will also deduplicate the resources, since on a component > swap out or co-layout design, the resources are always the same. > While duplicate regulator supplies won't cause much issue, shared > GPIOs don't work reliably, especially with other drivers. For the > same reason, the prober will release the GPIOs before the successfully > probed component is actually enabled. > > Signed-off-by: Chen-Yu Tsai <wenst@chromium.org> > --- > Changes since v5: > - Split of_regulator_bulk_get_all() return value check and explain > "ret == 0" case > - Switched to of_get_next_child_with_prefix_scoped() where applicable > - Used krealloc_array() instead of directly calculating size > - copy whole regulator array in one memcpy() call > - Drop "0" from struct zeroing initializer > - Split out regulator helper from i2c_of_probe_enable_res() to keep > code cleaner when combined with the next patch > - Added options for customizing power sequencing delay > - Rename i2c_of_probe_get_regulator() to i2c_of_probe_get_regulators() > - Add i2c_of_probe_free_regulator() helper > > Changes since v4: > - Split out GPIO handling to separate patch > - Rewrote using of_regulator_bulk_get_all() > - Replaced "regulators" with "regulator supplies" in debug messages > > Changes since v3: > - New patch > > This change is kept as a separate patch for now since the changes are > quite numerous. > --- > drivers/i2c/i2c-core-of-prober.c | 154 +++++++++++++++++++++++++++++-- > include/linux/i2c.h | 10 +- > 2 files changed, 155 insertions(+), 9 deletions(-) I never jumped back into looking at this since you started sending new versions last month (sorry), but I finally did... At a high level, I have to say I'm not really a fan of the "reach into all of the devices, jam their regulators on, force their GPIOs high, and hope for the best" approach. It just feels like it's going to break at the first bit of slightly different hardware and cause power sequence violations left and right. If nothing else, regulators often need delays between when one regulator is enabled and the next. There may also be complex relationships between regulators and GPIOs, GPIOs, GPIOs that need to be low, or even GPIO "toggle sequences" (power on rail 1, wait 1 ms, assert reset, wait 10 ms, deassert reset, power on rail 2). IMO the only way to make this reliable is to have this stuff be much less automatic and much more driven by the board. I think that, in general, before the board prober checks i2c address then the prober should be in charge of setting up pinctrl and turning on regulators / GPIOs. Given that the same regulator(s) and GPIO(s) may be specified by different children, the prober will just have to pick one child to find those resources. It should have enough board-specific knowledge to make this choice. Then the prober should turn them on via a board-specific power-on sequence that's known to work for all the children. Then it should start probing. I think there can still be plenty of common helper functions that the board-specific prober can leverage, but overall I'd expect the actual power-on and power-off code to be board-specific. If many boards have in common that we need to turn on exactly one regulator + one GPIO, or just one regulator, or whatever then having a helper function that handles these cases is fine. ...but it should be one of many choices that a board proper could use and not the only one. -Doug
On Thu, Sep 5, 2024 at 6:57 AM Doug Anderson <dianders@chromium.org> wrote: > > Hi, > > On Wed, Sep 4, 2024 at 2:01 AM Chen-Yu Tsai <wenst@chromium.org> wrote: > > > > This adds regulator management to the I2C OF component prober. > > Components that the prober intends to probe likely require their > > regulator supplies be enabled, and GPIOs be toggled to enable them or > > bring them out of reset before they will respond to probe attempts. > > GPIOs will be handled in the next patch. > > > > Without specific knowledge of each component's resource names or > > power sequencing requirements, the prober can only enable the > > regulator supplies all at once, and toggle the GPIOs all at once. > > Luckily, reset pins tend to be active low, while enable pins tend to > > be active high, so setting the raw status of all GPIO pins to high > > should work. The wait time before and after resources are enabled > > are collected from existing drivers and device trees. > > > > The prober collects resources from all possible components and enables > > them together, instead of enabling resources and probing each component > > one by one. The latter approach does not provide any boot time benefits > > over simply enabling each component and letting each driver probe > > sequentially. > > > > The prober will also deduplicate the resources, since on a component > > swap out or co-layout design, the resources are always the same. > > While duplicate regulator supplies won't cause much issue, shared > > GPIOs don't work reliably, especially with other drivers. For the > > same reason, the prober will release the GPIOs before the successfully > > probed component is actually enabled. > > > > Signed-off-by: Chen-Yu Tsai <wenst@chromium.org> > > --- > > Changes since v5: > > - Split of_regulator_bulk_get_all() return value check and explain > > "ret == 0" case > > - Switched to of_get_next_child_with_prefix_scoped() where applicable > > - Used krealloc_array() instead of directly calculating size > > - copy whole regulator array in one memcpy() call > > - Drop "0" from struct zeroing initializer > > - Split out regulator helper from i2c_of_probe_enable_res() to keep > > code cleaner when combined with the next patch > > - Added options for customizing power sequencing delay > > - Rename i2c_of_probe_get_regulator() to i2c_of_probe_get_regulators() > > - Add i2c_of_probe_free_regulator() helper > > > > Changes since v4: > > - Split out GPIO handling to separate patch > > - Rewrote using of_regulator_bulk_get_all() > > - Replaced "regulators" with "regulator supplies" in debug messages > > > > Changes since v3: > > - New patch > > > > This change is kept as a separate patch for now since the changes are > > quite numerous. > > --- > > drivers/i2c/i2c-core-of-prober.c | 154 +++++++++++++++++++++++++++++-- > > include/linux/i2c.h | 10 +- > > 2 files changed, 155 insertions(+), 9 deletions(-) > > I never jumped back into looking at this since you started sending new > versions last month (sorry), but I finally did... > > At a high level, I have to say I'm not really a fan of the "reach into > all of the devices, jam their regulators on, force their GPIOs high, > and hope for the best" approach. It just feels like it's going to > break at the first bit of slightly different hardware and cause power > sequence violations left and right. If nothing else, regulators often > need delays between when one regulator is enabled and the next. There > may also be complex relationships between regulators and GPIOs, GPIOs, > GPIOs that need to be low, or even GPIO "toggle sequences" (power on > rail 1, wait 1 ms, assert reset, wait 10 ms, deassert reset, power on > rail 2). > > IMO the only way to make this reliable is to have this stuff be much > less automatic and much more driven by the board. > > I think that, in general, before the board prober checks i2c address > then the prober should be in charge of setting up pinctrl and turning > on regulators / GPIOs. Given that the same regulator(s) and GPIO(s) > may be specified by different children, the prober will just have to > pick one child to find those resources. It should have enough > board-specific knowledge to make this choice. Then the prober should > turn them on via a board-specific power-on sequence that's known to > work for all the children. Then it should start probing. > > I think there can still be plenty of common helper functions that the > board-specific prober can leverage, but overall I'd expect the actual > power-on and power-off code to be board-specific. > > If many boards have in common that we need to turn on exactly one > regulator + one GPIO, or just one regulator, or whatever then having a > helper function that handles these cases is fine. ...but it should be > one of many choices that a board proper could use and not the only > one. IIUC we could have the "options" data structure have much more board specific information: - name of node to fetch resources (regulator supplies and GPIOs) from - names of the resources for the node given from the previous item - delay time after each resource is toggled - polarity in the case of GPIOs - prober callback to do power sequencing The "resource collection" step would use the first two items to retrieve the regulator supplies and GPIOS instead of the bulk APIs used right now. The power sequencing callback would use the resources combined with the given delays to enable the supplies and toggle the GPIOs. For now I would probably only implement a generic one regulator supply plus one GPIO helper. That is the common case for touchscreens and trackpads connected over a ribbon cable. Does that sound like what you have in mind? This next item would be a later enhancement (which isn't implemented in this series anyway): - optional prober callback that does actual probing In our case it would only be used for cases where an HID-over-I2C component shares the same address as a non-HID one, and some extra work is needed to determine which type it is. I still need to think about the structure of this. Thanks ChenYu
Hi, On Thu, Sep 5, 2024 at 8:10 AM Chen-Yu Tsai <wenst@chromium.org> wrote: > > On Thu, Sep 5, 2024 at 6:57 AM Doug Anderson <dianders@chromium.org> wrote: > > > > Hi, > > > > On Wed, Sep 4, 2024 at 2:01 AM Chen-Yu Tsai <wenst@chromium.org> wrote: > > > > > > This adds regulator management to the I2C OF component prober. > > > Components that the prober intends to probe likely require their > > > regulator supplies be enabled, and GPIOs be toggled to enable them or > > > bring them out of reset before they will respond to probe attempts. > > > GPIOs will be handled in the next patch. > > > > > > Without specific knowledge of each component's resource names or > > > power sequencing requirements, the prober can only enable the > > > regulator supplies all at once, and toggle the GPIOs all at once. > > > Luckily, reset pins tend to be active low, while enable pins tend to > > > be active high, so setting the raw status of all GPIO pins to high > > > should work. The wait time before and after resources are enabled > > > are collected from existing drivers and device trees. > > > > > > The prober collects resources from all possible components and enables > > > them together, instead of enabling resources and probing each component > > > one by one. The latter approach does not provide any boot time benefits > > > over simply enabling each component and letting each driver probe > > > sequentially. > > > > > > The prober will also deduplicate the resources, since on a component > > > swap out or co-layout design, the resources are always the same. > > > While duplicate regulator supplies won't cause much issue, shared > > > GPIOs don't work reliably, especially with other drivers. For the > > > same reason, the prober will release the GPIOs before the successfully > > > probed component is actually enabled. > > > > > > Signed-off-by: Chen-Yu Tsai <wenst@chromium.org> > > > --- > > > Changes since v5: > > > - Split of_regulator_bulk_get_all() return value check and explain > > > "ret == 0" case > > > - Switched to of_get_next_child_with_prefix_scoped() where applicable > > > - Used krealloc_array() instead of directly calculating size > > > - copy whole regulator array in one memcpy() call > > > - Drop "0" from struct zeroing initializer > > > - Split out regulator helper from i2c_of_probe_enable_res() to keep > > > code cleaner when combined with the next patch > > > - Added options for customizing power sequencing delay > > > - Rename i2c_of_probe_get_regulator() to i2c_of_probe_get_regulators() > > > - Add i2c_of_probe_free_regulator() helper > > > > > > Changes since v4: > > > - Split out GPIO handling to separate patch > > > - Rewrote using of_regulator_bulk_get_all() > > > - Replaced "regulators" with "regulator supplies" in debug messages > > > > > > Changes since v3: > > > - New patch > > > > > > This change is kept as a separate patch for now since the changes are > > > quite numerous. > > > --- > > > drivers/i2c/i2c-core-of-prober.c | 154 +++++++++++++++++++++++++++++-- > > > include/linux/i2c.h | 10 +- > > > 2 files changed, 155 insertions(+), 9 deletions(-) > > > > I never jumped back into looking at this since you started sending new > > versions last month (sorry), but I finally did... > > > > At a high level, I have to say I'm not really a fan of the "reach into > > all of the devices, jam their regulators on, force their GPIOs high, > > and hope for the best" approach. It just feels like it's going to > > break at the first bit of slightly different hardware and cause power > > sequence violations left and right. If nothing else, regulators often > > need delays between when one regulator is enabled and the next. There > > may also be complex relationships between regulators and GPIOs, GPIOs, > > GPIOs that need to be low, or even GPIO "toggle sequences" (power on > > rail 1, wait 1 ms, assert reset, wait 10 ms, deassert reset, power on > > rail 2). > > > > IMO the only way to make this reliable is to have this stuff be much > > less automatic and much more driven by the board. > > > > I think that, in general, before the board prober checks i2c address > > then the prober should be in charge of setting up pinctrl and turning > > on regulators / GPIOs. Given that the same regulator(s) and GPIO(s) > > may be specified by different children, the prober will just have to > > pick one child to find those resources. It should have enough > > board-specific knowledge to make this choice. Then the prober should > > turn them on via a board-specific power-on sequence that's known to > > work for all the children. Then it should start probing. > > > > I think there can still be plenty of common helper functions that the > > board-specific prober can leverage, but overall I'd expect the actual > > power-on and power-off code to be board-specific. > > > > If many boards have in common that we need to turn on exactly one > > regulator + one GPIO, or just one regulator, or whatever then having a > > helper function that handles these cases is fine. ...but it should be > > one of many choices that a board proper could use and not the only > > one. > > IIUC we could have the "options" data structure have much more board > specific information: > > - name of node to fetch resources (regulator supplies and GPIOs) from > - names of the resources for the node given from the previous item > - delay time after each resource is toggled > - polarity in the case of GPIOs > - prober callback to do power sequencing > > The "resource collection" step would use the first two items to retrieve > the regulator supplies and GPIOS instead of the bulk APIs used right now. > > The power sequencing callback would use the resources combined with the > given delays to enable the supplies and toggle the GPIOs. > > For now I would probably only implement a generic one regulator supply > plus one GPIO helper. That is the common case for touchscreens and > trackpads connected over a ribbon cable. > > Does that sound like what you have in mind? I guess I'd have to see how the code looks to know for sure, but if I understand it sounds a little awkward. Specifically, the "options" sound like they might become complicated enough that you're inventing your own little programming language (with delays, abilities to drive pins low and high, abilities to turn on/off clocks, and abilities to turn off/on regulators) and then probers need to code up their programs in this language. You also need to handle undoing things properly if there is a failure in the middle. Like your "program" would look like this (obviously you'd have to play with enums more, but you get the idea): { { OPCODE_TURN_REGULATOR_ON, "vdd" }, { OPCODE_DELAY, 10 }, { OPCODE_GPIO_ASSERT, "reset" }, { OPCODE_DELAY, 5 }, { OPCODE_GPIO_DEASSERT "reset" }, { OPCODE_DELAY, 100 }, { OPCODE_TURN_REGULATOR_ON, "vddIO" }, } Why not just expect the board probers to write C code to turn things on before looking for i2c devices, then provide helpers to the C code? So there wouldn't be some generic "resource collection" API, but you'd provide a helper to make it easy to grab regulators from one of the nodes by name. If you think bulk enabling regulators is common then you could make a helper that grabs all of the regulators from a node in a way that is consistent with the bulk APIs, but I wouldn't expect every driver to use that since devices I've seen expect regulators to be enabled in a very specific order even if they don't need a delay between them. I wouldn't expect a "collect all GPIOs" API because it seems really weird to me that we'd ever want to jam multiple GPIOs in a state without knowing exactly which GPIO was what and asserting them in the right sequence. > This next item would be a later enhancement (which isn't implemented in > this series anyway): > > - optional prober callback that does actual probing > > In our case it would only be used for cases where an HID-over-I2C > component shares the same address as a non-HID one, and some extra > work is needed to determine which type it is. I still need to think > about the structure of this. IMO _that_ would be a great option to the i2c prober. It feels like you could have an optional register read that needs to match to have the i2c prober succeed. Most people would leave it blank (just the i2c device existing is enough) but probably a single register read would be enough to confirm you got the right device. Most i2c devices have some sort of "version" / "vendor" / "id" type register somewhere. -Doug
On Thu, Sep 05, 2024 at 11:14:58AM -0700, Doug Anderson wrote: > On Thu, Sep 5, 2024 at 8:10 AM Chen-Yu Tsai <wenst@chromium.org> wrote: > > This next item would be a later enhancement (which isn't implemented in > > this series anyway): > > - optional prober callback that does actual probing > > In our case it would only be used for cases where an HID-over-I2C > > component shares the same address as a non-HID one, and some extra > > work is needed to determine which type it is. I still need to think > > about the structure of this. > IMO _that_ would be a great option to the i2c prober. It feels like > you could have an optional register read that needs to match to have > the i2c prober succeed. Most people would leave it blank (just the i2c > device existing is enough) but probably a single register read would > be enough to confirm you got the right device. Most i2c devices have > some sort of "version" / "vendor" / "id" type register somewhere. I'm not so sure about the "most" there - it depends quite a bit on the class of device. This also imply that the prober would have a regmap config as well...
On Thu, Sep 05, 2024 at 11:14:58AM -0700, Doug Anderson wrote: > On Thu, Sep 5, 2024 at 8:10 AM Chen-Yu Tsai <wenst@chromium.org> wrote: ... > I guess I'd have to see how the code looks to know for sure, but if I > understand it sounds a little awkward. Specifically, the "options" > sound like they might become complicated enough that you're inventing > your own little programming language (with delays, abilities to drive > pins low and high, abilities to turn on/off clocks, and abilities to > turn off/on regulators) and then probers need to code up their > programs in this language. You beat me up to it. I have the same thought. However, what is described is exactly what a regular PMIC has. They already have their own language for exactly this purposes. (At least I see that in a few Intel SoC-based platforms.) So, after all it may be not a bad idea But would be good to have it standardized (if it's even possible :-).
On Fri, Sep 6, 2024 at 2:15 AM Doug Anderson <dianders@chromium.org> wrote: > > Hi, > > On Thu, Sep 5, 2024 at 8:10 AM Chen-Yu Tsai <wenst@chromium.org> wrote: > > > > On Thu, Sep 5, 2024 at 6:57 AM Doug Anderson <dianders@chromium.org> wrote: > > > > > > Hi, > > > > > > On Wed, Sep 4, 2024 at 2:01 AM Chen-Yu Tsai <wenst@chromium.org> wrote: > > > > > > > > This adds regulator management to the I2C OF component prober. > > > > Components that the prober intends to probe likely require their > > > > regulator supplies be enabled, and GPIOs be toggled to enable them or > > > > bring them out of reset before they will respond to probe attempts. > > > > GPIOs will be handled in the next patch. > > > > > > > > Without specific knowledge of each component's resource names or > > > > power sequencing requirements, the prober can only enable the > > > > regulator supplies all at once, and toggle the GPIOs all at once. > > > > Luckily, reset pins tend to be active low, while enable pins tend to > > > > be active high, so setting the raw status of all GPIO pins to high > > > > should work. The wait time before and after resources are enabled > > > > are collected from existing drivers and device trees. > > > > > > > > The prober collects resources from all possible components and enables > > > > them together, instead of enabling resources and probing each component > > > > one by one. The latter approach does not provide any boot time benefits > > > > over simply enabling each component and letting each driver probe > > > > sequentially. > > > > > > > > The prober will also deduplicate the resources, since on a component > > > > swap out or co-layout design, the resources are always the same. > > > > While duplicate regulator supplies won't cause much issue, shared > > > > GPIOs don't work reliably, especially with other drivers. For the > > > > same reason, the prober will release the GPIOs before the successfully > > > > probed component is actually enabled. > > > > > > > > Signed-off-by: Chen-Yu Tsai <wenst@chromium.org> > > > > --- > > > > Changes since v5: > > > > - Split of_regulator_bulk_get_all() return value check and explain > > > > "ret == 0" case > > > > - Switched to of_get_next_child_with_prefix_scoped() where applicable > > > > - Used krealloc_array() instead of directly calculating size > > > > - copy whole regulator array in one memcpy() call > > > > - Drop "0" from struct zeroing initializer > > > > - Split out regulator helper from i2c_of_probe_enable_res() to keep > > > > code cleaner when combined with the next patch > > > > - Added options for customizing power sequencing delay > > > > - Rename i2c_of_probe_get_regulator() to i2c_of_probe_get_regulators() > > > > - Add i2c_of_probe_free_regulator() helper > > > > > > > > Changes since v4: > > > > - Split out GPIO handling to separate patch > > > > - Rewrote using of_regulator_bulk_get_all() > > > > - Replaced "regulators" with "regulator supplies" in debug messages > > > > > > > > Changes since v3: > > > > - New patch > > > > > > > > This change is kept as a separate patch for now since the changes are > > > > quite numerous. > > > > --- > > > > drivers/i2c/i2c-core-of-prober.c | 154 +++++++++++++++++++++++++++++-- > > > > include/linux/i2c.h | 10 +- > > > > 2 files changed, 155 insertions(+), 9 deletions(-) > > > > > > I never jumped back into looking at this since you started sending new > > > versions last month (sorry), but I finally did... > > > > > > At a high level, I have to say I'm not really a fan of the "reach into > > > all of the devices, jam their regulators on, force their GPIOs high, > > > and hope for the best" approach. It just feels like it's going to > > > break at the first bit of slightly different hardware and cause power > > > sequence violations left and right. If nothing else, regulators often > > > need delays between when one regulator is enabled and the next. There > > > may also be complex relationships between regulators and GPIOs, GPIOs, > > > GPIOs that need to be low, or even GPIO "toggle sequences" (power on > > > rail 1, wait 1 ms, assert reset, wait 10 ms, deassert reset, power on > > > rail 2). > > > > > > IMO the only way to make this reliable is to have this stuff be much > > > less automatic and much more driven by the board. > > > > > > I think that, in general, before the board prober checks i2c address > > > then the prober should be in charge of setting up pinctrl and turning > > > on regulators / GPIOs. Given that the same regulator(s) and GPIO(s) > > > may be specified by different children, the prober will just have to > > > pick one child to find those resources. It should have enough > > > board-specific knowledge to make this choice. Then the prober should > > > turn them on via a board-specific power-on sequence that's known to > > > work for all the children. Then it should start probing. > > > > > > I think there can still be plenty of common helper functions that the > > > board-specific prober can leverage, but overall I'd expect the actual > > > power-on and power-off code to be board-specific. > > > > > > If many boards have in common that we need to turn on exactly one > > > regulator + one GPIO, or just one regulator, or whatever then having a > > > helper function that handles these cases is fine. ...but it should be > > > one of many choices that a board proper could use and not the only > > > one. > > > > IIUC we could have the "options" data structure have much more board > > specific information: > > > > - name of node to fetch resources (regulator supplies and GPIOs) from > > - names of the resources for the node given from the previous item > > - delay time after each resource is toggled > > - polarity in the case of GPIOs > > - prober callback to do power sequencing > > > > The "resource collection" step would use the first two items to retrieve > > the regulator supplies and GPIOS instead of the bulk APIs used right now. > > > > The power sequencing callback would use the resources combined with the > > given delays to enable the supplies and toggle the GPIOs. > > > > For now I would probably only implement a generic one regulator supply > > plus one GPIO helper. That is the common case for touchscreens and > > trackpads connected over a ribbon cable. > > > > Does that sound like what you have in mind? > > I guess I'd have to see how the code looks to know for sure, but if I > understand it sounds a little awkward. Specifically, the "options" > sound like they might become complicated enough that you're inventing > your own little programming language (with delays, abilities to drive > pins low and high, abilities to turn on/off clocks, and abilities to > turn off/on regulators) and then probers need to code up their > programs in this language. You also need to handle undoing things > properly if there is a failure in the middle. Like your "program" > would look like this (obviously you'd have to play with enums more, > but you get the idea): > > { > { OPCODE_TURN_REGULATOR_ON, "vdd" }, > { OPCODE_DELAY, 10 }, > { OPCODE_GPIO_ASSERT, "reset" }, > { OPCODE_DELAY, 5 }, > { OPCODE_GPIO_DEASSERT "reset" }, > { OPCODE_DELAY, 100 }, > { OPCODE_TURN_REGULATOR_ON, "vddIO" }, > } > > Why not just expect the board probers to write C code to turn things > on before looking for i2c devices, then provide helpers to the C code? > > So there wouldn't be some generic "resource collection" API, but you'd > provide a helper to make it easy to grab regulators from one of the > nodes by name. If you think bulk enabling regulators is common then > you could make a helper that grabs all of the regulators from a node > in a way that is consistent with the bulk APIs, but I wouldn't expect > every driver to use that since devices I've seen expect regulators to > be enabled in a very specific order even if they don't need a delay > between them. > > I wouldn't expect a "collect all GPIOs" API because it seems really > weird to me that we'd ever want to jam multiple GPIOs in a state > without knowing exactly which GPIO was what and asserting them in the > right sequence. So I'm slightly confused, as it sounds like at this point the i2c prober would be litter more than just a framework, and the heavy lifting is to be all done by callbacks provided by the board-specific driver? So the framework becomes something like: 1. find i2c bus node 2. call provided callback with i2c bus node to gather resources; let callback handle specifics 3. call provided callback to enable resources 4. for each i2c component, call provided callback to probe If the probe succeeded: 5. call provided callback for early release of resources (GPIOs) 6. set "status" to "okay" 7. call provided callback for late release of resources (regulators) Otherwise at the end of the loop 8. release resources The current code can be reworked into helpers for steps 2, 3, 5, 7 for the single regulator single GPIO case. > > This next item would be a later enhancement (which isn't implemented in > > this series anyway): > > > > - optional prober callback that does actual probing > > > > In our case it would only be used for cases where an HID-over-I2C > > component shares the same address as a non-HID one, and some extra > > work is needed to determine which type it is. I still need to think > > about the structure of this. > > IMO _that_ would be a great option to the i2c prober. It feels like > you could have an optional register read that needs to match to have > the i2c prober succeed. Most people would leave it blank (just the i2c > device existing is enough) but probably a single register read would > be enough to confirm you got the right device. Most i2c devices have > some sort of "version" / "vendor" / "id" type register somewhere. At least for the stuff that we have (touchscreens and trackpads) such registers typically don't exist, unless it's an HID-over-I2C device, in which case there's the standard HID descriptor at some address. But, yeah, reading the HID descriptor was the use case I had in mind. At least for one Chromebooks it's a bit more tricky because that one HID-over-I2C component shares the same address as a non-HID one. We currently have different SKU IDs and thus different device trees for them, but we could make the prober work with this. It just has be able to tell if the component it's currently probing needs the special prober and is it responding correctly. This bit I need to think about. ChenYu
Hi, On Thu, Sep 5, 2024 at 8:45 PM Chen-Yu Tsai <wenst@chromium.org> wrote: > > > > IIUC we could have the "options" data structure have much more board > > > specific information: > > > > > > - name of node to fetch resources (regulator supplies and GPIOs) from > > > - names of the resources for the node given from the previous item > > > - delay time after each resource is toggled > > > - polarity in the case of GPIOs > > > - prober callback to do power sequencing > > > > > > The "resource collection" step would use the first two items to retrieve > > > the regulator supplies and GPIOS instead of the bulk APIs used right now. > > > > > > The power sequencing callback would use the resources combined with the > > > given delays to enable the supplies and toggle the GPIOs. > > > > > > For now I would probably only implement a generic one regulator supply > > > plus one GPIO helper. That is the common case for touchscreens and > > > trackpads connected over a ribbon cable. > > > > > > Does that sound like what you have in mind? > > > > I guess I'd have to see how the code looks to know for sure, but if I > > understand it sounds a little awkward. Specifically, the "options" > > sound like they might become complicated enough that you're inventing > > your own little programming language (with delays, abilities to drive > > pins low and high, abilities to turn on/off clocks, and abilities to > > turn off/on regulators) and then probers need to code up their > > programs in this language. You also need to handle undoing things > > properly if there is a failure in the middle. Like your "program" > > would look like this (obviously you'd have to play with enums more, > > but you get the idea): > > > > { > > { OPCODE_TURN_REGULATOR_ON, "vdd" }, > > { OPCODE_DELAY, 10 }, > > { OPCODE_GPIO_ASSERT, "reset" }, > > { OPCODE_DELAY, 5 }, > > { OPCODE_GPIO_DEASSERT "reset" }, > > { OPCODE_DELAY, 100 }, > > { OPCODE_TURN_REGULATOR_ON, "vddIO" }, > > } > > > > Why not just expect the board probers to write C code to turn things > > on before looking for i2c devices, then provide helpers to the C code? > > > > So there wouldn't be some generic "resource collection" API, but you'd > > provide a helper to make it easy to grab regulators from one of the > > nodes by name. If you think bulk enabling regulators is common then > > you could make a helper that grabs all of the regulators from a node > > in a way that is consistent with the bulk APIs, but I wouldn't expect > > every driver to use that since devices I've seen expect regulators to > > be enabled in a very specific order even if they don't need a delay > > between them. > > > > I wouldn't expect a "collect all GPIOs" API because it seems really > > weird to me that we'd ever want to jam multiple GPIOs in a state > > without knowing exactly which GPIO was what and asserting them in the > > right sequence. > > So I'm slightly confused, as it sounds like at this point the i2c prober > would be litter more than just a framework, and the heavy lifting is to > be all done by callbacks provided by the board-specific driver? > > So the framework becomes something like: > > 1. find i2c bus node > 2. call provided callback with i2c bus node to gather resources; > let callback handle specifics > 3. call provided callback to enable resources > 4. for each i2c component, call provided callback to probe I don't think I'd do it as callbacks but just have the HW prober call the functions directly. AKA, instead of doing: i2c_of_probe_component(dev, "touchscreen", ts_opts, ts_callbacks); Do: grab_touchscreen_resources(...); power_on_touchscreens(...); i2c_of_probe_component(...); power_off_touchscreen(...); release_touchscreen_resources(...); Obviously I'm spitballing here, though. Without writing the code it's hard for me to know that my proposal would be better, but my gut tells me that trying to write something overly generic with lots of options / callbacks would be more confusing. > If the probe succeeded: > > 5. call provided callback for early release of resources (GPIOs) > 6. set "status" to "okay" > 7. call provided callback for late release of resources (regulators) > > Otherwise at the end of the loop > > 8. release resources > > The current code can be reworked into helpers for steps 2, 3, 5, 7 for > the single regulator single GPIO case. > > > > This next item would be a later enhancement (which isn't implemented in > > > this series anyway): > > > > > > - optional prober callback that does actual probing > > > > > > In our case it would only be used for cases where an HID-over-I2C > > > component shares the same address as a non-HID one, and some extra > > > work is needed to determine which type it is. I still need to think > > > about the structure of this. > > > > IMO _that_ would be a great option to the i2c prober. It feels like > > you could have an optional register read that needs to match to have > > the i2c prober succeed. Most people would leave it blank (just the i2c > > device existing is enough) but probably a single register read would > > be enough to confirm you got the right device. Most i2c devices have > > some sort of "version" / "vendor" / "id" type register somewhere. > > At least for the stuff that we have (touchscreens and trackpads) such > registers typically don't exist, unless it's an HID-over-I2C device, > in which case there's the standard HID descriptor at some address. > But, yeah, reading the HID descriptor was the use case I had in mind. > > At least for one Chromebooks it's a bit more tricky because that one > HID-over-I2C component shares the same address as a non-HID one. We > currently have different SKU IDs and thus different device trees for > them, but we could make the prober work with this. It just has be able > to tell if the component it's currently probing needs the special > prober and is it responding correctly. This bit I need to think about. I guess Mark Brown also thought that there wouldn't be some magic register, but my gut still tells me that most i2c devices have some way to confirm that they are what you expect even if it's not an official "vendor" or "version" register. Some type of predictable register at a predictable location that you could use, at least if you knew all of the options that someone might stuff. For instance, in elan trackpads you can see elan_i2c_get_product_id(). That just reads a location (ETP_I2C_UNIQUEID_CMD = 0x0101) that could theoretically be used to figure out (maybe in conjunction with other registers) that it's an elan trackpad instead of an i2c-hid one. You'd have to (of course) confirm that an i2c-hid device wouldn't somehow return back data from this read that made it look like an elan trackpad, but it feels like there ought to be some way to figure it out with a few i2c register reads. ...that being said, I guess my original assertion that you might be able to figure out with a simple register read was naive and you'd actually need a function (maybe as a callback) to figure this out. -Doug
On Wed, Sep 11, 2024 at 8:30 AM Doug Anderson <dianders@chromium.org> wrote: > > Hi, > > On Thu, Sep 5, 2024 at 8:45 PM Chen-Yu Tsai <wenst@chromium.org> wrote: > > > > > > IIUC we could have the "options" data structure have much more board > > > > specific information: > > > > > > > > - name of node to fetch resources (regulator supplies and GPIOs) from > > > > - names of the resources for the node given from the previous item > > > > - delay time after each resource is toggled > > > > - polarity in the case of GPIOs > > > > - prober callback to do power sequencing > > > > > > > > The "resource collection" step would use the first two items to retrieve > > > > the regulator supplies and GPIOS instead of the bulk APIs used right now. > > > > > > > > The power sequencing callback would use the resources combined with the > > > > given delays to enable the supplies and toggle the GPIOs. > > > > > > > > For now I would probably only implement a generic one regulator supply > > > > plus one GPIO helper. That is the common case for touchscreens and > > > > trackpads connected over a ribbon cable. > > > > > > > > Does that sound like what you have in mind? > > > > > > I guess I'd have to see how the code looks to know for sure, but if I > > > understand it sounds a little awkward. Specifically, the "options" > > > sound like they might become complicated enough that you're inventing > > > your own little programming language (with delays, abilities to drive > > > pins low and high, abilities to turn on/off clocks, and abilities to > > > turn off/on regulators) and then probers need to code up their > > > programs in this language. You also need to handle undoing things > > > properly if there is a failure in the middle. Like your "program" > > > would look like this (obviously you'd have to play with enums more, > > > but you get the idea): > > > > > > { > > > { OPCODE_TURN_REGULATOR_ON, "vdd" }, > > > { OPCODE_DELAY, 10 }, > > > { OPCODE_GPIO_ASSERT, "reset" }, > > > { OPCODE_DELAY, 5 }, > > > { OPCODE_GPIO_DEASSERT "reset" }, > > > { OPCODE_DELAY, 100 }, > > > { OPCODE_TURN_REGULATOR_ON, "vddIO" }, > > > } > > > > > > Why not just expect the board probers to write C code to turn things > > > on before looking for i2c devices, then provide helpers to the C code? > > > > > > So there wouldn't be some generic "resource collection" API, but you'd > > > provide a helper to make it easy to grab regulators from one of the > > > nodes by name. If you think bulk enabling regulators is common then > > > you could make a helper that grabs all of the regulators from a node > > > in a way that is consistent with the bulk APIs, but I wouldn't expect > > > every driver to use that since devices I've seen expect regulators to > > > be enabled in a very specific order even if they don't need a delay > > > between them. > > > > > > I wouldn't expect a "collect all GPIOs" API because it seems really > > > weird to me that we'd ever want to jam multiple GPIOs in a state > > > without knowing exactly which GPIO was what and asserting them in the > > > right sequence. > > > > So I'm slightly confused, as it sounds like at this point the i2c prober > > would be litter more than just a framework, and the heavy lifting is to > > be all done by callbacks provided by the board-specific driver? > > > > So the framework becomes something like: > > > > 1. find i2c bus node > > 2. call provided callback with i2c bus node to gather resources; > > let callback handle specifics > > 3. call provided callback to enable resources > > 4. for each i2c component, call provided callback to probe > > I don't think I'd do it as callbacks but just have the HW prober call > the functions directly. AKA, instead of doing: > > i2c_of_probe_component(dev, "touchscreen", ts_opts, ts_callbacks); > > Do: > > grab_touchscreen_resources(...); > power_on_touchscreens(...); > i2c_of_probe_component(...); > power_off_touchscreen(...); > release_touchscreen_resources(...); > > Obviously I'm spitballing here, though. Without writing the code it's > hard for me to know that my proposal would be better, but my gut tells > me that trying to write something overly generic with lots of options > / callbacks would be more confusing. The helpers would be exported along with the framework, so there's nothing preventing others from using the helpers directly. The framework provides boilerplate so that scenarios fitting it won't have to duplicate the code. Obviously I'm basing it on my scenario here, but I think it works out for the simpler cases. I'll send out what I have reworked, and we can continue the discussion either on the mailing list or in person at ELCE and Plumbers. > > If the probe succeeded: > > > > 5. call provided callback for early release of resources (GPIOs) > > 6. set "status" to "okay" > > 7. call provided callback for late release of resources (regulators) > > > > Otherwise at the end of the loop > > > > 8. release resources > > > > The current code can be reworked into helpers for steps 2, 3, 5, 7 for > > the single regulator single GPIO case. > > > > > > This next item would be a later enhancement (which isn't implemented in > > > > this series anyway): > > > > > > > > - optional prober callback that does actual probing > > > > > > > > In our case it would only be used for cases where an HID-over-I2C > > > > component shares the same address as a non-HID one, and some extra > > > > work is needed to determine which type it is. I still need to think > > > > about the structure of this. > > > > > > IMO _that_ would be a great option to the i2c prober. It feels like > > > you could have an optional register read that needs to match to have > > > the i2c prober succeed. Most people would leave it blank (just the i2c > > > device existing is enough) but probably a single register read would > > > be enough to confirm you got the right device. Most i2c devices have > > > some sort of "version" / "vendor" / "id" type register somewhere. > > > > At least for the stuff that we have (touchscreens and trackpads) such > > registers typically don't exist, unless it's an HID-over-I2C device, > > in which case there's the standard HID descriptor at some address. > > But, yeah, reading the HID descriptor was the use case I had in mind. > > > > At least for one Chromebooks it's a bit more tricky because that one > > HID-over-I2C component shares the same address as a non-HID one. We > > currently have different SKU IDs and thus different device trees for > > them, but we could make the prober work with this. It just has be able > > to tell if the component it's currently probing needs the special > > prober and is it responding correctly. This bit I need to think about. > > I guess Mark Brown also thought that there wouldn't be some magic > register, but my gut still tells me that most i2c devices have some > way to confirm that they are what you expect even if it's not an > official "vendor" or "version" register. Some type of predictable > register at a predictable location that you could use, at least if you > knew all of the options that someone might stuff. > > For instance, in elan trackpads you can see elan_i2c_get_product_id(). > That just reads a location (ETP_I2C_UNIQUEID_CMD = 0x0101) that could > theoretically be used to figure out (maybe in conjunction with other > registers) that it's an elan trackpad instead of an i2c-hid one. You'd > have to (of course) confirm that an i2c-hid device wouldn't somehow > return back data from this read that made it look like an elan > trackpad, but it feels like there ought to be some way to figure it > out with a few i2c register reads. > > ...that being said, I guess my original assertion that you might be > able to figure out with a simple register read was naive and you'd > actually need a function (maybe as a callback) to figure this out. Yeah, that's my plan for a follow-up series, likely after the SKU ID based prober work is done. The actual probe function would likely only target the known cases of I2C address conflicts we have on ChromeOS devices.
On Tue, Sep 10, 2024 at 05:30:07PM -0700, Doug Anderson wrote: > On Thu, Sep 5, 2024 at 8:45 PM Chen-Yu Tsai <wenst@chromium.org> wrote: ... > > At least for the stuff that we have (touchscreens and trackpads) such > > registers typically don't exist, unless it's an HID-over-I2C device, > > in which case there's the standard HID descriptor at some address. > > But, yeah, reading the HID descriptor was the use case I had in mind. > > > > At least for one Chromebooks it's a bit more tricky because that one > > HID-over-I2C component shares the same address as a non-HID one. We > > currently have different SKU IDs and thus different device trees for > > them, but we could make the prober work with this. It just has be able > > to tell if the component it's currently probing needs the special > > prober and is it responding correctly. This bit I need to think about. > > I guess Mark Brown also thought that there wouldn't be some magic > register, but my gut still tells me that most i2c devices have some > way to confirm that they are what you expect even if it's not an > official "vendor" or "version" register. Some type of predictable > register at a predictable location that you could use, at least if you > knew all of the options that someone might stuff. "most" is way too optimistic to say, I believe that not even close to majority of I²C target devices they are not reliably discoverable. That's the downside of non-discoverable busses like I²C. Maybe I³C has a mechanism for that, but I am not an expert, just wondering. > For instance, in elan trackpads you can see elan_i2c_get_product_id(). > That just reads a location (ETP_I2C_UNIQUEID_CMD = 0x0101) that could > theoretically be used to figure out (maybe in conjunction with other > registers) that it's an elan trackpad instead of an i2c-hid one. You'd > have to (of course) confirm that an i2c-hid device wouldn't somehow > return back data from this read that made it look like an elan > trackpad, but it feels like there ought to be some way to figure it > out with a few i2c register reads. > > ...that being said, I guess my original assertion that you might be > able to figure out with a simple register read was naive and you'd > actually need a function (maybe as a callback) to figure this out.
On Wed, Sep 11, 2024 at 05:38:23PM +0300, Andy Shevchenko wrote: > On Tue, Sep 10, 2024 at 05:30:07PM -0700, Doug Anderson wrote: > > On Thu, Sep 5, 2024 at 8:45 PM Chen-Yu Tsai <wenst@chromium.org> wrote: > > > At least for one Chromebooks it's a bit more tricky because that one > > > HID-over-I2C component shares the same address as a non-HID one. We > > > currently have different SKU IDs and thus different device trees for > > > them, but we could make the prober work with this. It just has be able > > > to tell if the component it's currently probing needs the special > > > prober and is it responding correctly. This bit I need to think about. > > I guess Mark Brown also thought that there wouldn't be some magic > > register, but my gut still tells me that most i2c devices have some > > way to confirm that they are what you expect even if it's not an > > official "vendor" or "version" register. Some type of predictable > > register at a predictable location that you could use, at least if you > > knew all of the options that someone might stuff. > "most" is way too optimistic to say, I believe that not even close to majority > of I²C target devices they are not reliably discoverable. > That's the downside of non-discoverable busses like I²C. Maybe I³C has > a mechanism for that, but I am not an expert, just wondering. There's no standard mechanism for I2C, however it is relatively common for devices to have some kind of ID registers. This is especially true if you're confining yourself to a particular class of device, bigger and more modern devices are more likely to have this - if you want to pick your audio CODEC or touchscreen controller it's a lot more likely that'll work than something like a simple DAC or ADC for example. You also have the different I2C addresses that vendors pick to help out. It's not an actual specified discovery mechanism, but practically speaking you probably can generalise some of the ID register probing. Though equally it's good practice for drivers to check this anyway so it's not clear how much benefit there is over just trying to run probe().
diff --git a/drivers/i2c/i2c-core-of-prober.c b/drivers/i2c/i2c-core-of-prober.c index 64d7631f4885..56b06ad7aa64 100644 --- a/drivers/i2c/i2c-core-of-prober.c +++ b/drivers/i2c/i2c-core-of-prober.c @@ -6,12 +6,14 @@ */ #include <linux/cleanup.h> +#include <linux/delay.h> #include <linux/device.h> #include <linux/dev_printk.h> #include <linux/err.h> #include <linux/i2c.h> #include <linux/module.h> #include <linux/of.h> +#include <linux/regulator/consumer.h> #include <linux/slab.h> /* @@ -27,11 +29,119 @@ * address responds. * * TODO: - * - Support handling common regulators. * - Support handling common GPIOs. * - Support I2C muxes */ +struct i2c_of_probe_data { + const struct i2c_of_probe_opts *opts; + struct regulator_bulk_data *regulators; + unsigned int regulators_num; +}; + +/* Returns number of regulator supplies found for node, or error. */ +static int i2c_of_probe_get_regulators(struct device *dev, struct device_node *node, + struct i2c_of_probe_data *data) +{ + struct regulator_bulk_data *tmp, *new_regulators; + int ret; + + ret = of_regulator_bulk_get_all(dev, node, &tmp); + if (ret < 0) { + return ret; + } else if (ret == 0) { + /* + * It's entirely possible for a device node to not have + * regulator supplies. While it doesn't make sense from + * a hardware perspective, the supplies could be always + * on or otherwise not modeled in the device tree, but + * the device would still work. + */ + return ret; + } + + if (!data->regulators) { + data->regulators = tmp; + data->regulators_num = ret; + return ret; + }; + + new_regulators = krealloc_array(data->regulators, (data->regulators_num + ret), + sizeof(*tmp), GFP_KERNEL); + if (!new_regulators) { + regulator_bulk_free(ret, tmp); + return -ENOMEM; + } + + data->regulators = new_regulators; + memcpy(&data->regulators[data->regulators_num], tmp, sizeof(*tmp) * ret); + data->regulators_num += ret; + + return ret; +} + +static void i2c_of_probe_free_regulators(struct i2c_of_probe_data *data) +{ + regulator_bulk_free(data->regulators_num, data->regulators); + data->regulators_num = 0; + data->regulators = NULL; +} + +static void i2c_of_probe_free_res(struct i2c_of_probe_data *data) +{ + i2c_of_probe_free_regulators(data); +} + +static int i2c_of_probe_get_res(struct device *dev, struct device_node *node, + struct i2c_of_probe_data *data) +{ + struct property *prop; + int ret; + + ret = i2c_of_probe_get_regulators(dev, node, data); + if (ret < 0) { + dev_err_probe(dev, ret, "Failed to get regulator supplies from %pOF\n", node); + goto err_cleanup; + } + + return 0; + +err_cleanup: + i2c_of_probe_free_res(data); + return ret; +} + +static int i2c_of_probe_enable_regulators(struct device *dev, struct i2c_of_probe_data *data) +{ + int ret; + + dev_dbg(dev, "Enabling regulator supplies\n"); + + ret = regulator_bulk_enable(data->regulators_num, data->regulators); + if (ret) + return ret; + + msleep(data->opts->post_power_on_delay_ms); + + return 0; +} + +static void i2c_of_probe_disable_regulators(struct i2c_of_probe_data *data) +{ + regulator_bulk_disable(data->regulators_num, data->regulators); +} + +static int i2c_of_probe_enable_res(struct device *dev, struct i2c_of_probe_data *data) +{ + int ret; + + ret = i2c_of_probe_enable_regulators(dev, data); + if (ret) + return ret; + + return 0; +} + static struct device_node *i2c_of_probe_get_i2c_node(struct device *dev, const char *type) { struct device_node *node __free(device_node) = of_find_node_by_name(NULL, type); @@ -78,10 +188,17 @@ static int i2c_of_probe_enable_node(struct device *dev, struct device_node *node return ret; } +static const struct i2c_of_probe_opts i2c_of_probe_opts_default = { + /* largest post-power-on pre-reset-deassert delay seen among drivers */ + .post_power_on_delay_ms = 500, +}; + /** * 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 + * @opts: &struct i2c_of_probe_data containing tweakable options for the prober. + * Defaults are used if this is %NULL. * * Probe for possible I2C components of the same "type" on the same I2C bus * that have their status marked as "fail". @@ -108,8 +225,9 @@ static int i2c_of_probe_enable_node(struct device *dev, struct device_node *node * had multiple types of components to probe, and one of them down * the list caused a deferred probe. This is expected behavior. */ -int i2c_of_probe_component(struct device *dev, const char *type) +int i2c_of_probe_component(struct device *dev, const char *type, const struct i2c_of_probe_opts *opts) { + struct i2c_of_probe_data probe_data = { .opts = opts ?: &i2c_of_probe_opts_default }; struct i2c_adapter *i2c; int ret; @@ -117,9 +235,7 @@ int i2c_of_probe_component(struct device *dev, const char *type) if (IS_ERR(i2c_node)) return PTR_ERR(i2c_node); - for_each_child_of_node_scoped(i2c_node, node) { - if (!of_node_name_prefix(node, type)) - continue; + for_each_child_of_node_with_prefix_scoped(i2c_node, node, type) { if (!of_device_is_available(node)) continue; @@ -134,13 +250,33 @@ int i2c_of_probe_component(struct device *dev, const char *type) if (!i2c) return dev_err_probe(dev, -EPROBE_DEFER, "Couldn't get I2C adapter\n"); + /* Grab resources */ + for_each_child_of_node_with_prefix_scoped(i2c_node, node, type) { + u32 addr; + + if (of_property_read_u32(node, "reg", &addr)) + continue; + + dev_dbg(dev, "Requesting resources for %pOF\n", node); + ret = i2c_of_probe_get_res(dev, node, &probe_data); + if (ret) + return ret; + } + + dev_dbg(dev, "Resources: # of regulator supplies = %d\n", probe_data.regulators_num); + + /* Enable resources */ + ret = i2c_of_probe_enable_res(dev, &probe_data); + if (ret) { + i2c_of_probe_free_res(&probe_data); + return dev_err_probe(dev, ret, "Failed to enable resources\n"); + } + ret = 0; - for_each_child_of_node_scoped(i2c_node, node) { + for_each_child_of_node_with_prefix_scoped(i2c_node, node, type) { 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) @@ -151,6 +287,8 @@ int i2c_of_probe_component(struct device *dev, const char *type) break; } + i2c_of_probe_disable_regulators(&probe_data); + i2c_of_probe_free_res(&probe_data); i2c_put_adapter(i2c); return ret; diff --git a/include/linux/i2c.h b/include/linux/i2c.h index c6c16731243d..dbcdb8edbf6f 100644 --- a/include/linux/i2c.h +++ b/include/linux/i2c.h @@ -1034,7 +1034,15 @@ 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); +/** + * i2c_of_probe_opts - I2C OF component prober customization options + * @post_power_on_delay_us: Delay in ms after regulators are powered on. Passed to msleep(). + */ +struct i2c_of_probe_opts { + unsigned int post_power_on_delay_ms; +}; + +int i2c_of_probe_component(struct device *dev, const char *type, const struct i2c_of_probe_opts *opts); #endif #else
This adds regulator management to the I2C OF component prober. Components that the prober intends to probe likely require their regulator supplies be enabled, and GPIOs be toggled to enable them or bring them out of reset before they will respond to probe attempts. GPIOs will be handled in the next patch. Without specific knowledge of each component's resource names or power sequencing requirements, the prober can only enable the regulator supplies all at once, and toggle the GPIOs all at once. Luckily, reset pins tend to be active low, while enable pins tend to be active high, so setting the raw status of all GPIO pins to high should work. The wait time before and after resources are enabled are collected from existing drivers and device trees. The prober collects resources from all possible components and enables them together, instead of enabling resources and probing each component one by one. The latter approach does not provide any boot time benefits over simply enabling each component and letting each driver probe sequentially. The prober will also deduplicate the resources, since on a component swap out or co-layout design, the resources are always the same. While duplicate regulator supplies won't cause much issue, shared GPIOs don't work reliably, especially with other drivers. For the same reason, the prober will release the GPIOs before the successfully probed component is actually enabled. Signed-off-by: Chen-Yu Tsai <wenst@chromium.org> --- Changes since v5: - Split of_regulator_bulk_get_all() return value check and explain "ret == 0" case - Switched to of_get_next_child_with_prefix_scoped() where applicable - Used krealloc_array() instead of directly calculating size - copy whole regulator array in one memcpy() call - Drop "0" from struct zeroing initializer - Split out regulator helper from i2c_of_probe_enable_res() to keep code cleaner when combined with the next patch - Added options for customizing power sequencing delay - Rename i2c_of_probe_get_regulator() to i2c_of_probe_get_regulators() - Add i2c_of_probe_free_regulator() helper Changes since v4: - Split out GPIO handling to separate patch - Rewrote using of_regulator_bulk_get_all() - Replaced "regulators" with "regulator supplies" in debug messages Changes since v3: - New patch This change is kept as a separate patch for now since the changes are quite numerous. --- drivers/i2c/i2c-core-of-prober.c | 154 +++++++++++++++++++++++++++++-- include/linux/i2c.h | 10 +- 2 files changed, 155 insertions(+), 9 deletions(-)