| Message ID | 20240822092006.3134096-9-wenst@chromium.org (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | platform/chrome: Introduce DT hardware prober | expand |
On Thu, Aug 22, 2024 at 05:20:01PM +0800, Chen-Yu Tsai wrote: > This adds GPIO 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. > regulator support was added in the previous 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. ... > + struct fwnode_handle *fwnode = of_fwnode_handle(node); > + struct gpio_descs *gpiods; > + struct gpio_desc *gpiod; > + char con[32]; /* 32 is max size of property name */ Use 'propname' to be aligned with GPIO library usages. > + char *con_id = NULL; > + size_t new_size; > + int len; ... > + if (len >= sizeof(con) - 1) { This can be transformed to check the returned value from strscpy(). > + pr_err("%pOF: length of GPIO name \"%s\" exceeds current limit\n", > + node, prop->name); > + return -EINVAL; > + } > + > + if (len > 0) { > + strscpy(con, prop->name, len + 1); The correct (robust) call is with destination size. Which means here that you may use 2-argument strscpy(). > + con_id = con; > + } ... > + if (!data->gpiods) > + return 0; If it comes a new code (something else besides GPIOs and regulators) this will be a (small) impediment. Better to have a helper for each case and do ret = ..._gpiods(); if (ret) ... Same for regulators and anything else in the future, if any. > + /* > + * reset GPIOs normally have opposite polarity compared to "reset" > + * enable GPIOs. Instead of parsing the flags again, simply "enable" > + * set the raw value to high. This is quite a fragile assumption. Yes, it would work in 98% cases, but will break if it's not true somewhere else. > + */ ... > + /* largest post-reset-deassert delay seen in tree for Elan I2C HID */ > + msleep(300); Same Q, how do you monitor _all_ the drivers? ... > +disable_gpios: > + for (gpio_i--; gpio_i >= 0; gpio_i--) > + gpiod_set_raw_value_cansleep(data->gpiods->desc[gpio_i], 0); Can't you call the _array() variant here? ... > - dev_dbg(dev, "Resources: # of regulator supplies = %d\n", probe_data.regulators_num); > + dev_dbg(dev, "Resources: # of GPIOs = %d, # of regulator supplies = %d\n", > + probe_data.gpiods ? probe_data.gpiods->ndescs : 0, > + probe_data.regulators_num); I would issue one message per class of the devices (GPIOs, regulators, ...)
On Thu, Aug 22, 2024 at 10:20 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Thu, Aug 22, 2024 at 05:20:01PM +0800, Chen-Yu Tsai wrote: > > This adds GPIO 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. > > regulator support was added in the previous 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. > > ... > > > + struct fwnode_handle *fwnode = of_fwnode_handle(node); > > + struct gpio_descs *gpiods; > > + struct gpio_desc *gpiod; > > + char con[32]; /* 32 is max size of property name */ > > Use 'propname' to be aligned with GPIO library usages. Ack. > > + char *con_id = NULL; > > + size_t new_size; > > + int len; > > ... > > > + if (len >= sizeof(con) - 1) { > > This can be transformed to check the returned value from strscpy(). Ack. > > + pr_err("%pOF: length of GPIO name \"%s\" exceeds current limit\n", > > + node, prop->name); > > + return -EINVAL; > > + } > > + > > + if (len > 0) { > > + strscpy(con, prop->name, len + 1); > > The correct (robust) call is with destination size. Which means here that you > may use 2-argument strscpy(). Ack. > > + con_id = con; > > + } > > ... > > > + if (!data->gpiods) > > + return 0; > > If it comes a new code (something else besides GPIOs and regulators) this will be a (small) impediment. Better to have a helper for each case and do > > ret = ..._gpiods(); > if (ret) > ... > > Same for regulators and anything else in the future, if any. I'm not sure I follow. Do you mean wrap each individual type in a wrapper and call those here, like the following? i2c_of_probe_enable_res(...) { ret = i2c_of_probe_enable_regulators(...) if (ret) return ret; ret = i2c_of_probe_enable_gpios(...) if (ret) goto error_disable_regulators; ... } > > + /* > > + * reset GPIOs normally have opposite polarity compared to > > "reset" > > > + * enable GPIOs. Instead of parsing the flags again, simply > > "enable" > > > + * set the raw value to high. > > This is quite a fragile assumption. Yes, it would work in 98% cases, but will > break if it's not true somewhere else. Well, this seems to be the de facto standard. Or it would have to remember what each GPIO descriptor's name is, and try to classify those into either "enable" or "reset", and set their respective logical values to 1 or 0. And then you run into a peripheral with a broken binding that has its "reset" GPIO inverted, i.e. it's driver behavior needs to follow the "enable" GPIO style. The class of devices this prober targets are consumer electronics (laptops, tablets, phones) that at least have gone through some component selection where the options won't have conflicting requirements. And if the polarities of the possible components don't line up, then this probe structure can't really do anything. One would need something that power sequences each component separately and probes it. I would really like to avoid that if possible, as it makes the boot time (to peripheral available) dependent on which component you have and how far down the list it is. We have Chromebooks that have 4 touchscreen components introduced over the years. In that case something more like Doug's original proposal would work better: something that forces mutual exclusivity among a class of devices. > > + */ > > ... > > > + /* largest post-reset-deassert delay seen in tree for Elan I2C HID */ > > + msleep(300); > > Same Q, how do you monitor _all_ the drivers? Discussion in the previous patch. > ... > > > +disable_gpios: > > + for (gpio_i--; gpio_i >= 0; gpio_i--) > > + gpiod_set_raw_value_cansleep(data->gpiods->desc[gpio_i], 0); > > Can't you call the _array() variant here? I thought that without |struct gpio_array| the _array() variant wouldn't help, i.e. it would still be a loop internally. Looks like I was wrong. > ... > > > - dev_dbg(dev, "Resources: # of regulator supplies = %d\n", probe_data.regulators_num); > > + dev_dbg(dev, "Resources: # of GPIOs = %d, # of regulator supplies = %d\n", > > + probe_data.gpiods ? probe_data.gpiods->ndescs : 0, > > + probe_data.regulators_num); > > I would issue one message per class of the devices (GPIOs, regulators, ...) Ack. Thank you for the review. ChenYu
On Fri, Aug 23, 2024 at 06:32:16PM +0800, Chen-Yu Tsai wrote: > On Thu, Aug 22, 2024 at 10:20 PM Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: > > On Thu, Aug 22, 2024 at 05:20:01PM +0800, Chen-Yu Tsai wrote: ... > > > + if (!data->gpiods) > > > + return 0; > > > > If it comes a new code (something else besides GPIOs and regulators) this > > will be a (small) impediment. Better to have a helper for each case and do > > > > ret = ..._gpiods(); > > if (ret) > > ... > > > > Same for regulators and anything else in the future, if any. > > I'm not sure I follow. Do you mean wrap each individual type in a wrapper > and call those here, like the following? > > i2c_of_probe_enable_res(...) > { > ret = i2c_of_probe_enable_regulators(...) > if (ret) > return ret; > > ret = i2c_of_probe_enable_gpios(...) > if (ret) > goto error_disable_regulators; > > ... > } Yes. ... > > > + /* > > > + * reset GPIOs normally have opposite polarity compared to > > > > "reset" > > > > > + * enable GPIOs. Instead of parsing the flags again, simply > > > > "enable" > > > > > + * set the raw value to high. > > > > This is quite a fragile assumption. Yes, it would work in 98% cases, but will > > break if it's not true somewhere else. > > Well, this seems to be the de facto standard. Or it would have to remember > what each GPIO descriptor's name is, and try to classify those into either > "enable" or "reset", and set their respective logical values to 1 or 0. > And then you run into a peripheral with a broken binding that has its > "reset" GPIO inverted, i.e. it's driver behavior needs to follow the > "enable" GPIO style. The class of devices this prober targets are > consumer electronics (laptops, tablets, phones) that at least have gone > through some component selection where the options won't have conflicting > requirements. I'm talking from real life example(s) :-) Recently I looked at the OV7251 sensor driver that expects "enable" GPIO while all users supply "reset"-as-"enable" with the exact trouble I described. Yet it's pure software / ABI issue in that case, but who knows what PCB engineers may come up with. > And if the polarities of the possible components don't line up, then this > probe structure can't really do anything. One would need something that > power sequences each component separately and probes it. I would really > like to avoid that if possible, as it makes the boot time (to peripheral > available) dependent on which component you have and how far down the > list it is. We have Chromebooks that have 4 touchscreen components > introduced over the years. In that case something more like Doug's > original proposal would work better: something that forces mutual > exclusivity among a class of devices. Maybe. I just pointed out the potential problem. > > > + */
On Fri, Aug 23, 2024 at 10:01 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Fri, Aug 23, 2024 at 06:32:16PM +0800, Chen-Yu Tsai wrote: > > On Thu, Aug 22, 2024 at 10:20 PM Andy Shevchenko > > <andriy.shevchenko@linux.intel.com> wrote: > > > On Thu, Aug 22, 2024 at 05:20:01PM +0800, Chen-Yu Tsai wrote: > > ... > > > > > + if (!data->gpiods) > > > > + return 0; > > > > > > If it comes a new code (something else besides GPIOs and regulators) this > > > will be a (small) impediment. Better to have a helper for each case and do > > > > > > ret = ..._gpiods(); > > > if (ret) > > > ... > > > > > > Same for regulators and anything else in the future, if any. > > > > I'm not sure I follow. Do you mean wrap each individual type in a wrapper > > and call those here, like the following? > > > > i2c_of_probe_enable_res(...) > > { > > ret = i2c_of_probe_enable_regulators(...) > > if (ret) > > return ret; > > > > ret = i2c_of_probe_enable_gpios(...) > > if (ret) > > goto error_disable_regulators; > > > > ... > > } > > Yes. > > ... > > > > > + /* > > > > + * reset GPIOs normally have opposite polarity compared to > > > > > > "reset" > > > > > > > + * enable GPIOs. Instead of parsing the flags again, simply > > > > > > "enable" > > > > > > > + * set the raw value to high. > > > > > > This is quite a fragile assumption. Yes, it would work in 98% cases, but will > > > break if it's not true somewhere else. > > > > Well, this seems to be the de facto standard. Or it would have to remember > > what each GPIO descriptor's name is, and try to classify those into either > > "enable" or "reset", and set their respective logical values to 1 or 0. > > And then you run into a peripheral with a broken binding that has its > > "reset" GPIO inverted, i.e. it's driver behavior needs to follow the > > "enable" GPIO style. The class of devices this prober targets are > > consumer electronics (laptops, tablets, phones) that at least have gone > > through some component selection where the options won't have conflicting > > requirements. > > I'm talking from real life example(s) :-) > > Recently I looked at the OV7251 sensor driver that expects "enable" GPIO while > all users supply "reset"-as-"enable" with the exact trouble I described. > Yet it's pure software / ABI issue in that case, but who knows what PCB > engineers may come up with. For the OV7251 case specifically, the current approach works fine, as the polarity is the same: high electric level for a working chip. But now that you mention it, camera sensors are exactly the case I had in mind, though not the same chip. Some of the OmniVision and GalaxyCore sensors have a "shutdown" pin that is active high, i.e. high electric level shuts down the chip. So I guess the alternative would be to remember each GPIO's name, and do the appropriate thing based on the name, and also set the logical value, not the raw value. If it says "shutdown" or "reset", set the logical value to 0; if it says "enable", set it to 1. And hopefully we don't run into a binding which has "shutdown" but is actually "enable" ... I don't have this case and I kind of want to leave it as a TODO though. I feel like the scope of the series is expanding ever so slightly. ChenYu > > And if the polarities of the possible components don't line up, then this > > probe structure can't really do anything. One would need something that > > power sequences each component separately and probes it. I would really > > like to avoid that if possible, as it makes the boot time (to peripheral > > available) dependent on which component you have and how far down the > > list it is. We have Chromebooks that have 4 touchscreen components > > introduced over the years. In that case something more like Doug's > > original proposal would work better: something that forces mutual > > exclusivity among a class of devices. > > Maybe. I just pointed out the potential problem. > > > > > + */ > > -- > With Best Regards, > Andy Shevchenko > >
diff --git a/drivers/i2c/i2c-core-of-prober.c b/drivers/i2c/i2c-core-of-prober.c index 32184cfd10f6..046e6605053c 100644 --- a/drivers/i2c/i2c-core-of-prober.c +++ b/drivers/i2c/i2c-core-of-prober.c @@ -10,6 +10,7 @@ #include <linux/device.h> #include <linux/dev_printk.h> #include <linux/err.h> +#include <linux/gpio/consumer.h> #include <linux/i2c.h> #include <linux/module.h> #include <linux/of.h> @@ -29,11 +30,11 @@ * address responds. * * TODO: - * - Support handling common GPIOs. * - Support I2C muxes */ struct i2c_of_probe_data { + struct gpio_descs *gpiods; struct regulator_bulk_data *regulators; unsigned int regulators_num; }; @@ -71,8 +72,88 @@ static int i2c_of_probe_get_regulator(struct device *dev, struct device_node *no return ret; } +/* + * Returns 1 if property is GPIO and GPIO successfully requested, + * 0 if not a GPIO property, or error if request for GPIO failed. + */ +static int i2c_of_probe_get_gpiod(struct device_node *node, struct property *prop, + struct i2c_of_probe_data *data) +{ + struct fwnode_handle *fwnode = of_fwnode_handle(node); + struct gpio_descs *gpiods; + struct gpio_desc *gpiod; + char con[32]; /* 32 is max size of property name */ + char *con_id = NULL; + size_t new_size; + int len; + + len = gpio_property_name_length(prop->name); + if (len < 0) + return 0; + + if (len >= sizeof(con) - 1) { + pr_err("%pOF: length of GPIO name \"%s\" exceeds current limit\n", + node, prop->name); + return -EINVAL; + } + + if (len > 0) { + strscpy(con, prop->name, len + 1); + con_id = con; + } + + /* + * GPIO descriptors are not reference counted. GPIOD_FLAGS_BIT_NONEXCLUSIVE + * can't differentiate between GPIOs shared between devices to be probed and + * other devices (which is incorrect). If the initial request fails with + * -EBUSY, retry with GPIOD_FLAGS_BIT_NONEXCLUSIVE and see if it matches + * any existing ones. + */ + gpiod = fwnode_gpiod_get_index(fwnode, con_id, 0, GPIOD_ASIS, "i2c-of-prober"); + if (IS_ERR(gpiod)) { + int i; + + if (PTR_ERR(gpiod) != -EBUSY || !data->gpiods) + return PTR_ERR(gpiod); + + gpiod = fwnode_gpiod_get_index(fwnode, con_id, 0, + GPIOD_ASIS | GPIOD_FLAGS_BIT_NONEXCLUSIVE, + "i2c-of-prober"); + for (i = 0; i < data->gpiods->ndescs; i++) + if (gpiod == data->gpiods->desc[i]) + return 1; + + return -EBUSY; + } + + new_size = struct_size(gpiods, desc, data->gpiods ? data->gpiods->ndescs + 1 : 1); + gpiods = krealloc(data->gpiods, new_size, GFP_KERNEL); + if (!gpiods) { + gpiod_put(gpiod); + return -ENOMEM; + } + + data->gpiods = gpiods; + data->gpiods->desc[data->gpiods->ndescs++] = gpiod; + + return 1; +} + +/* + * This is split into two functions because in the normal flow the GPIOs + * have to be released before the actual driver probes so that the latter + * can acquire them. + */ +static void i2c_of_probe_free_gpios(struct i2c_of_probe_data *data) +{ + if (data->gpiods) + gpiod_put_array(data->gpiods); + data->gpiods = NULL; +} + static void i2c_of_probe_free_res(struct i2c_of_probe_data *data) { + i2c_of_probe_free_gpios(data); regulator_bulk_free(data->regulators_num, data->regulators); } @@ -88,6 +169,18 @@ static int i2c_of_probe_get_res(struct device *dev, struct device_node *node, goto err_cleanup; } + for_each_property_of_node(node, prop) { + dev_dbg(dev, "Trying property %pOF/%s\n", node, prop->name); + + /* GPIOs */ + ret = i2c_of_probe_get_gpiod(node, prop, data); + if (ret < 0) { + dev_err_probe(dev, ret, "Failed to get GPIO from %pOF/%s\n", + node, prop->name); + goto err_cleanup; + } + } + return 0; err_cleanup: @@ -98,6 +191,7 @@ static int i2c_of_probe_get_res(struct device *dev, struct device_node *node, static int i2c_of_probe_enable_res(struct device *dev, struct i2c_of_probe_data *data) { int ret = 0; + int gpio_i; dev_dbg(dev, "Enabling regulator supplies\n"); @@ -108,7 +202,32 @@ static int i2c_of_probe_enable_res(struct device *dev, struct i2c_of_probe_data /* largest post-power-on pre-reset-deassert delay seen among drivers */ msleep(500); + if (!data->gpiods) + return 0; + + for (gpio_i = 0; gpio_i < data->gpiods->ndescs; gpio_i++) { + /* + * reset GPIOs normally have opposite polarity compared to + * enable GPIOs. Instead of parsing the flags again, simply + * set the raw value to high. + */ + dev_dbg(dev, "Setting GPIO %d\n", gpio_i); + ret = gpiod_direction_output_raw(data->gpiods->desc[gpio_i], 1); + if (ret) + goto disable_gpios; + } + + /* largest post-reset-deassert delay seen in tree for Elan I2C HID */ + msleep(300); + return 0; + +disable_gpios: + for (gpio_i--; gpio_i >= 0; gpio_i--) + gpiod_set_raw_value_cansleep(data->gpiods->desc[gpio_i], 0); + regulator_bulk_disable(data->regulators_num, data->regulators); + + return ret; } static void i2c_of_probe_disable_regulators(struct i2c_of_probe_data *data) @@ -234,7 +353,9 @@ int i2c_of_probe_component(struct device *dev, const char *type) return ret; } - dev_dbg(dev, "Resources: # of regulator supplies = %d\n", probe_data.regulators_num); + dev_dbg(dev, "Resources: # of GPIOs = %d, # of regulator supplies = %d\n", + probe_data.gpiods ? probe_data.gpiods->ndescs : 0, + probe_data.regulators_num); /* Enable resources */ ret = i2c_of_probe_enable_res(dev, &probe_data); @@ -256,6 +377,7 @@ int i2c_of_probe_component(struct device *dev, const char *type) continue; /* Found a device that is responding */ + i2c_of_probe_free_gpios(&probe_data); ret = i2c_of_probe_enable_node(dev, node); break; }
This adds GPIO 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. regulator support was added in the previous 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 v4: - Split out from previous patch - Moved GPIO property name check to common function in gpiolib.c in new patch - Moved i2c_of_probe_free_gpios() into for_each_child_of_node_scoped() - Rewrote in gpiod_*_array-esque fashion --- drivers/i2c/i2c-core-of-prober.c | 126 ++++++++++++++++++++++++++++++- 1 file changed, 124 insertions(+), 2 deletions(-)