| Message ID | 20240911072751.365361-9-wenst@chromium.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | platform/chrome: Introduce DT hardware prober | expand |
On Wed, Sep 11, 2024 at 03:27:46PM +0800, Chen-Yu Tsai wrote: > Add GPIO support to the simple helpers for 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 supplies were handled in the previous patch. > > The assumption is that the same class of components to be probed are > always connected in the same fashion with the same regulator supply > and GPIO. The names may vary due to binding differences, but the > physical layout does not change. > > This supports at most one GPIO pin. The user must specify the GPIO name, > the polarity, and the amount of time to wait after the GPIO is toggled. > Devices with more than one GPIO pin likely require specific power > sequencing beyond what generic code can easily support. ... > +static int i2c_of_probe_simple_get_gpiod(struct device *dev, struct device_node *node, > + struct i2c_of_probe_simple_ctx *ctx) > +{ > + struct fwnode_handle *fwnode = of_fwnode_handle(node); > + struct gpio_desc *gpiod; > + const char *con_id = NULL; > + > + /* NULL signals no GPIO needed */ > + if (!ctx->opts->gpio_name) > + return 0; > + > + /* An empty string signals an unnamed GPIO */ > + if (strlen(ctx->opts->gpio_name)) You run for entire string in order to check the first byte only? if (ctx->opts->gpio_name[0] == '\0') con_id = NULL; else > + con_id = ctx->opts->gpio_name; Also note, that comment is given in inverted condition to what you actually do in the code. With my suggestion it looks better. > + gpiod = fwnode_gpiod_get_index(fwnode, con_id, 0, GPIOD_ASIS, "i2c-of-prober"); > + if (IS_ERR(gpiod)) > + return PTR_ERR(gpiod); > + > + ctx->gpiod = gpiod; > + > + return 0; > +} ... > +static int i2c_of_probe_simple_set_gpio(struct device *dev, struct i2c_of_probe_simple_ctx *ctx) > +{ > + int ret; > + > + if (!ctx->gpiod) > + return 0; > + dev_dbg(dev, "Setting GPIO\n"); > + ret = gpiod_direction_output_raw(ctx->gpiod, ctx->opts->gpio_high_to_enable ? 1 : 0); Hmm... _raw() in use... Perhaps it's on a territory of Bart and Linus to review and comment on this. > + if (ret) > + return ret; > + > + msleep(ctx->opts->post_reset_deassert_delay_ms); > + > + return 0; > +}
Hi, On Wed, Sep 11, 2024 at 12:29 AM Chen-Yu Tsai <wenst@chromium.org> wrote: > > +static int i2c_of_probe_simple_set_gpio(struct device *dev, struct i2c_of_probe_simple_ctx *ctx) > +{ > + int ret; > + > + if (!ctx->gpiod) > + return 0; > + > + dev_dbg(dev, "Setting GPIO\n"); > + > + ret = gpiod_direction_output_raw(ctx->gpiod, ctx->opts->gpio_high_to_enable ? 1 : 0); > + if (ret) > + return ret; > + > + msleep(ctx->opts->post_reset_deassert_delay_ms); Like in the previous patch, you need an "if (ctx->opts->post_reset_deassert_delay_ms)" before the msleep(). > +static void i2c_of_probe_simple_disable_gpio(struct device *dev, struct i2c_of_probe_simple_ctx *ctx) > +{ > + if (!ctx->gpiod) > + return; > + > + dev_dbg(dev, "Setting GPIO to input\n"); > + > + gpiod_direction_input(ctx->gpiod); This is weird. Why set it to input? > @@ -347,6 +438,7 @@ int i2c_of_probe_simple_cleanup(struct device *dev, void *data) > { > struct i2c_of_probe_simple_ctx *ctx = data; > > + i2c_of_probe_simple_disable_gpio(dev, ctx); Maybe add a comment before the GPIO call that it's a noop if we found something and i2c_of_probe_simple_free_res_early() was already called? Otherwise you need to read into the function to understand why this doesn't crash if the early call was made. To me, that makes the abstraction add confusion instead of simplifying things. > @@ -92,24 +93,33 @@ int i2c_of_probe_component(struct device *dev, const struct i2c_of_probe_cfg *cf > * struct i2c_of_probe_simple_opts - Options for simple I2C component prober callbacks > * @res_node_compatible: Compatible string of device node to retrieve resources from. > * @supply_name: Name of regulator supply. > + * @gpio_name: Name of GPIO. Talk about the fact that gpio_name can be NULL or an empty string and that they mean different things. > * @post_power_on_delay_ms: Delay in ms after regulators are powered on. Passed to msleep(). > + * @post_reset_deassert_delay_ms: Delay in ms after GPIOs are set. Passed to msleep(). > + * @gpio_high_to_enable: %true if GPIO should be set to electrical high to enable component. Now that you've added the GPIOs and more delays, the description of this structure needs to list exactly what the power sequence your simple functions assume. I would also say: given that you're providing a parameter anyway and you're giving the GPIO name, can you please move away from the "raw" values and move to "logical" values?
Hi, On 24-09-11 15:27, Chen-Yu Tsai wrote: > Add GPIO support to the simple helpers for 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 supplies were handled in the previous patch. > > The assumption is that the same class of components to be probed are > always connected in the same fashion with the same regulator supply > and GPIO. The names may vary due to binding differences, but the > physical layout does not change. > > This supports at most one GPIO pin. The user must specify the GPIO name, > the polarity, and the amount of time to wait after the GPIO is toggled. > Devices with more than one GPIO pin likely require specific power > sequencing beyond what generic code can easily support. > > Signed-off-by: Chen-Yu Tsai <wenst@chromium.org> > diff --git a/include/linux/i2c-of-prober.h b/include/linux/i2c-of-prober.h > index 541451fbf58d..c5e241163c94 100644 > --- a/include/linux/i2c-of-prober.h > +++ b/include/linux/i2c-of-prober.h > @@ -83,6 +83,7 @@ int i2c_of_probe_component(struct device *dev, const struct i2c_of_probe_cfg *cf > * > * The following helpers are provided: > * * i2c_of_probe_simple_get_res() > + * * i2c_of_probe_simple_free_res_early() > * * i2c_of_probe_simple_free_res_late() > * * i2c_of_probe_simple_enable() > * * i2c_of_probe_simple_cleanup() > @@ -92,24 +93,33 @@ int i2c_of_probe_component(struct device *dev, const struct i2c_of_probe_cfg *cf > * struct i2c_of_probe_simple_opts - Options for simple I2C component prober callbacks > * @res_node_compatible: Compatible string of device node to retrieve resources from. > * @supply_name: Name of regulator supply. > + * @gpio_name: Name of GPIO. > * @post_power_on_delay_ms: Delay in ms after regulators are powered on. Passed to msleep(). > + * @post_reset_deassert_delay_ms: Delay in ms after GPIOs are set. Passed to msleep(). > + * @gpio_high_to_enable: %true if GPIO should be set to electrical high to enable component. > */ > struct i2c_of_probe_simple_opts { > const char *res_node_compatible; > const char *supply_name; > + const char *gpio_name; > unsigned int post_power_on_delay_ms; > + unsigned int post_reset_deassert_delay_ms; > + bool gpio_high_to_enable; Missing '#include <linux/types.h>', otherwise compiler complains about unknown bool type, when 'i2c-of-prober.h' included without any previous includes.
On Sat, Sep 14, 2024 at 1:49 AM Doug Anderson <dianders@chromium.org> wrote: > > Hi, > > On Wed, Sep 11, 2024 at 12:29 AM Chen-Yu Tsai <wenst@chromium.org> wrote: > > > > +static int i2c_of_probe_simple_set_gpio(struct device *dev, struct i2c_of_probe_simple_ctx *ctx) > > +{ > > + int ret; > > + > > + if (!ctx->gpiod) > > + return 0; > > + > > + dev_dbg(dev, "Setting GPIO\n"); > > + > > + ret = gpiod_direction_output_raw(ctx->gpiod, ctx->opts->gpio_high_to_enable ? 1 : 0); > > + if (ret) > > + return ret; > > + > > + msleep(ctx->opts->post_reset_deassert_delay_ms); > > Like in the previous patch, you need an "if > (ctx->opts->post_reset_deassert_delay_ms)" before the msleep(). Ack. > > +static void i2c_of_probe_simple_disable_gpio(struct device *dev, struct i2c_of_probe_simple_ctx *ctx) > > +{ > > + if (!ctx->gpiod) > > + return; > > + > > + dev_dbg(dev, "Setting GPIO to input\n"); > > + > > + gpiod_direction_input(ctx->gpiod); > > This is weird. Why set it to input? It seemed to me this would be more like the initial state, without knowing the actual initial state. > > @@ -347,6 +438,7 @@ int i2c_of_probe_simple_cleanup(struct device *dev, void *data) > > { > > struct i2c_of_probe_simple_ctx *ctx = data; > > > > + i2c_of_probe_simple_disable_gpio(dev, ctx); > > Maybe add a comment before the GPIO call that it's a noop if we found > something and i2c_of_probe_simple_free_res_early() was already called? > Otherwise you need to read into the function to understand why this > doesn't crash if the early call was made. To me, that makes the > abstraction add confusion instead of simplifying things. Ack. > > @@ -92,24 +93,33 @@ int i2c_of_probe_component(struct device *dev, const struct i2c_of_probe_cfg *cf > > * struct i2c_of_probe_simple_opts - Options for simple I2C component prober callbacks > > * @res_node_compatible: Compatible string of device node to retrieve resources from. > > * @supply_name: Name of regulator supply. > > + * @gpio_name: Name of GPIO. > > Talk about the fact that gpio_name can be NULL or an empty string and > that they mean different things. Ack. > > * @post_power_on_delay_ms: Delay in ms after regulators are powered on. Passed to msleep(). > > + * @post_reset_deassert_delay_ms: Delay in ms after GPIOs are set. Passed to msleep(). > > + * @gpio_high_to_enable: %true if GPIO should be set to electrical high to enable component. > > Now that you've added the GPIOs and more delays, the description of > this structure needs to list exactly what the power sequence your > simple functions assume. Ack. > I would also say: given that you're providing a parameter anyway and > you're giving the GPIO name, can you please move away from the "raw" > values and move to "logical" values? I disagree. When hardware engineers design for swappable components, they likely look at the electrical compatibility of them. In this case, an active-high "enable" pin is electrically compatible with an active-low "reset" pin. If we use the logical value here, then we would need more logic to know when the logical polarity has to be reversed. Note that this doesn't work for components that are electrically incompatible. But in that case a lot more board dependent code would be needed anyway. ChenYu
Hi, On Tue, Sep 17, 2024 at 5:41 AM Chen-Yu Tsai <wenst@chromium.org> wrote: > > > > +static void i2c_of_probe_simple_disable_gpio(struct device *dev, struct i2c_of_probe_simple_ctx *ctx) > > > +{ > > > + if (!ctx->gpiod) > > > + return; > > > + > > > + dev_dbg(dev, "Setting GPIO to input\n"); > > > + > > > + gpiod_direction_input(ctx->gpiod); > > > > This is weird. Why set it to input? > > It seemed to me this would be more like the initial state, without knowing > the actual initial state. In this case, though, you're trying to turn off the resource, not trying to get back to the initial state. IMO deasserting the GPIO is the way to do this. If the output needs to make it an input in this case then it can use some type of open drain mode. > > I would also say: given that you're providing a parameter anyway and > > you're giving the GPIO name, can you please move away from the "raw" > > values and move to "logical" values? > > I disagree. When hardware engineers design for swappable components, they > likely look at the electrical compatibility of them. In this case, an > active-high "enable" pin is electrically compatible with an active-low > "reset" pin. If we use the logical value here, then we would need more > logic to know when the logical polarity has to be reversed. > > Note that this doesn't work for components that are electrically > incompatible. But in that case a lot more board dependent code would be > needed anyway. As we talked about in person, that made sense in the previous version of the patch where you were looking for all GPIOs willy-nilly. Now you'll be specifically asking for a GPIO by name and we should honor the "active high" or "active low" nature of it. -Doug
diff --git a/drivers/i2c/i2c-core-of-prober.c b/drivers/i2c/i2c-core-of-prober.c index 1371ea565556..6794ec749882 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/i2c-of-prober.h> #include <linux/module.h> @@ -30,7 +31,6 @@ * address responds. * * TODO: - * - Support handling common GPIOs. * - Support I2C muxes */ @@ -257,6 +257,64 @@ static void i2c_of_probe_simple_disable_regulator(struct device *dev, struct i2c regulator_disable(ctx->supply); } +static int i2c_of_probe_simple_get_gpiod(struct device *dev, struct device_node *node, + struct i2c_of_probe_simple_ctx *ctx) +{ + struct fwnode_handle *fwnode = of_fwnode_handle(node); + struct gpio_desc *gpiod; + const char *con_id = NULL; + + /* NULL signals no GPIO needed */ + if (!ctx->opts->gpio_name) + return 0; + + /* An empty string signals an unnamed GPIO */ + if (strlen(ctx->opts->gpio_name)) + con_id = ctx->opts->gpio_name; + + gpiod = fwnode_gpiod_get_index(fwnode, con_id, 0, GPIOD_ASIS, "i2c-of-prober"); + if (IS_ERR(gpiod)) + return PTR_ERR(gpiod); + + ctx->gpiod = gpiod; + + return 0; +} + +static void i2c_of_probe_simple_put_gpiod(struct i2c_of_probe_simple_ctx *ctx) +{ + gpiod_put(ctx->gpiod); + ctx->gpiod = NULL; +} + +static int i2c_of_probe_simple_set_gpio(struct device *dev, struct i2c_of_probe_simple_ctx *ctx) +{ + int ret; + + if (!ctx->gpiod) + return 0; + + dev_dbg(dev, "Setting GPIO\n"); + + ret = gpiod_direction_output_raw(ctx->gpiod, ctx->opts->gpio_high_to_enable ? 1 : 0); + if (ret) + return ret; + + msleep(ctx->opts->post_reset_deassert_delay_ms); + + return 0; +} + +static void i2c_of_probe_simple_disable_gpio(struct device *dev, struct i2c_of_probe_simple_ctx *ctx) +{ + if (!ctx->gpiod) + return; + + dev_dbg(dev, "Setting GPIO to input\n"); + + gpiod_direction_input(ctx->gpiod); +} + /** * i2c_of_probe_simple_get_res - Simple helper for I2C OF prober to get resources * @dev: Pointer to the &struct device of the caller, only used for dev_printk() messages @@ -264,6 +322,8 @@ static void i2c_of_probe_simple_disable_regulator(struct device *dev, struct i2c * @data: Pointer to &struct i2c_of_probe_simple_ctx helper context. * * If &i2c_of_probe_simple_opts->supply_name is given, request the named regulator supply. + * If &i2c_of_probe_simple_opts->gpio_name is given, request the named GPIO. Or if it is + * the empty string, request the unnamed GPIO. * * Return: %0 on success or no-op, or a negative error number on failure. */ @@ -292,14 +352,36 @@ int i2c_of_probe_simple_get_res(struct device *dev, struct device_node *bus_node if (ret) goto out_put_node; + ret = i2c_of_probe_simple_get_gpiod(dev, node, ctx); + if (ret) + goto out_put_supply; + return 0; +out_put_supply: + i2c_of_probe_simple_put_supply(ctx); out_put_node: of_node_put(node); return ret; } EXPORT_SYMBOL_NS_GPL(i2c_of_probe_simple_get_res, I2C_OF_PROBER); +/** + * i2c_of_probe_simple_free_res_early - \ + * Simple helper for I2C OF prober to release GPIOs before component is enabled + * @data: Pointer to &struct i2c_of_probe_simple_ctx helper context. + * + * GPIO descriptors are exclusive and have to be released before the + * actual driver probes so that the latter can acquire them. + */ +void i2c_of_probe_simple_free_res_early(void *data) +{ + struct i2c_of_probe_simple_ctx *ctx = data; + + i2c_of_probe_simple_put_gpiod(ctx); +} +EXPORT_SYMBOL_NS_GPL(i2c_of_probe_simple_free_res_early, I2C_OF_PROBER); + /** * i2c_of_probe_simple_free_res_late - Simple helper for I2C OF prober to release all resources. * @data: Pointer to &struct i2c_of_probe_simple_ctx helper context. @@ -308,6 +390,7 @@ void i2c_of_probe_simple_free_res_late(void *data) { struct i2c_of_probe_simple_ctx *ctx = data; + i2c_of_probe_simple_put_gpiod(ctx); i2c_of_probe_simple_put_supply(ctx); } EXPORT_SYMBOL_NS_GPL(i2c_of_probe_simple_free_res_late, I2C_OF_PROBER); @@ -330,7 +413,15 @@ int i2c_of_probe_simple_enable(struct device *dev, void *data) if (ret) return ret; + ret = i2c_of_probe_simple_set_gpio(dev, ctx); + if (ret) + goto err; + return 0; + +err: + i2c_of_probe_simple_disable_regulator(dev, ctx); + return ret; } EXPORT_SYMBOL_NS_GPL(i2c_of_probe_simple_enable, I2C_OF_PROBER); @@ -347,6 +438,7 @@ int i2c_of_probe_simple_cleanup(struct device *dev, void *data) { struct i2c_of_probe_simple_ctx *ctx = data; + i2c_of_probe_simple_disable_gpio(dev, ctx); i2c_of_probe_simple_disable_regulator(dev, ctx); return 0; @@ -355,6 +447,7 @@ EXPORT_SYMBOL_NS_GPL(i2c_of_probe_simple_cleanup, I2C_OF_PROBER); struct i2c_of_probe_ops i2c_of_probe_simple_ops = { .get_resources = i2c_of_probe_simple_get_res, + .free_resources_early = i2c_of_probe_simple_free_res_early, .enable = i2c_of_probe_simple_enable, .cleanup = i2c_of_probe_simple_cleanup, .free_resources_late = i2c_of_probe_simple_free_res_late, diff --git a/include/linux/i2c-of-prober.h b/include/linux/i2c-of-prober.h index 541451fbf58d..c5e241163c94 100644 --- a/include/linux/i2c-of-prober.h +++ b/include/linux/i2c-of-prober.h @@ -83,6 +83,7 @@ int i2c_of_probe_component(struct device *dev, const struct i2c_of_probe_cfg *cf * * The following helpers are provided: * * i2c_of_probe_simple_get_res() + * * i2c_of_probe_simple_free_res_early() * * i2c_of_probe_simple_free_res_late() * * i2c_of_probe_simple_enable() * * i2c_of_probe_simple_cleanup() @@ -92,24 +93,33 @@ int i2c_of_probe_component(struct device *dev, const struct i2c_of_probe_cfg *cf * struct i2c_of_probe_simple_opts - Options for simple I2C component prober callbacks * @res_node_compatible: Compatible string of device node to retrieve resources from. * @supply_name: Name of regulator supply. + * @gpio_name: Name of GPIO. * @post_power_on_delay_ms: Delay in ms after regulators are powered on. Passed to msleep(). + * @post_reset_deassert_delay_ms: Delay in ms after GPIOs are set. Passed to msleep(). + * @gpio_high_to_enable: %true if GPIO should be set to electrical high to enable component. */ struct i2c_of_probe_simple_opts { const char *res_node_compatible; const char *supply_name; + const char *gpio_name; unsigned int post_power_on_delay_ms; + unsigned int post_reset_deassert_delay_ms; + bool gpio_high_to_enable; }; struct regulator; +struct gpio_desc; struct i2c_of_probe_simple_ctx { /* public: provided by user before helpers are used. */ const struct i2c_of_probe_simple_opts *opts; /* private: internal fields for helpers. */ struct regulator *supply; + struct gpio_desc *gpiod; }; int i2c_of_probe_simple_get_res(struct device *dev, struct device_node *bus_node, void *data); +void i2c_of_probe_simple_free_res_early(void *data); void i2c_of_probe_simple_free_res_late(void *data); int i2c_of_probe_simple_enable(struct device *dev, void *data); int i2c_of_probe_simple_cleanup(struct device *dev, void *data);
Add GPIO support to the simple helpers for 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 supplies were handled in the previous patch. The assumption is that the same class of components to be probed are always connected in the same fashion with the same regulator supply and GPIO. The names may vary due to binding differences, but the physical layout does not change. This supports at most one GPIO pin. The user must specify the GPIO name, the polarity, and the amount of time to wait after the GPIO is toggled. Devices with more than one GPIO pin likely require specific power sequencing beyond what generic code can easily support. Signed-off-by: Chen-Yu Tsai <wenst@chromium.org> --- Changes since v6: - Restructured into helpers for the I2C OF component prober - Reduced to only handle one GPIO - Set GPIO to input on (failure) cleanup - Updated commit message Changes since v5: - Renamed "con" to "propname" in i2c_of_probe_get_gpiod() - Copy string first and check return value of strscpy() for overflow in i2c_of_probe_get_gpiod() - Add parenthesis around "enable" and "reset" GPIO names in comments - Split resource count debug message into two separate lines - Split out GPIO helper from i2c_of_probe_enable_res() to keep code cleaner following the previous patch - Adopted options for customizing power sequencing delay following previous patch 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 | 95 +++++++++++++++++++++++++++++++- include/linux/i2c-of-prober.h | 10 ++++ 2 files changed, 104 insertions(+), 1 deletion(-)