| Message ID | 20240822092006.3134096-6-wenst@chromium.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | platform/chrome: Introduce DT hardware prober | expand |
On Thu, Aug 22, 2024 at 05:19:58PM +0800, Chen-Yu Tsai wrote: > The I2C device tree component prober needs to get and toggle GPIO lines > for the components it intends to probe. These components may not use the > same name for their GPIO lines, so the prober must go through the device > tree, check each property to see it is a GPIO property, and get the GPIO > line. > > Instead of duplicating the GPIO suffixes, or exporting them to the > prober to do pattern matching, simply add and export a new function that > does the pattern matching and returns the length of the GPIO name. The > caller can then use that to copy out the name if it needs to. ... > +/** > + * gpio_property_name_length - Returns the GPIO name length from a property name > + * @str: string to check It's property name, so, I would name this 'propname'. > + * This function checks if the given name matches the GPIO property patterns, and > + * returns the length of the name of the GPIO. The pattern is "*-<GPIO suffix>" > + * or just "<GPIO suffix>". > + * > + * Returns: > + * The length of the string before '-' if it matches "*-<GPIO suffix>", or What about "x-y-gpios"? It's unclear what will be the behaviour. > + * 0 if no name part, just the suffix, or > + * -EINVAL if the string doesn't match the pattern. > + */ > +int gpio_property_name_length(const char *str) gpio_get_... ? > +{ > + size_t len; > + > + len = strlen(str); If it has a thousands characters...? > + /* string need to be at minimum len(gpio) */ > + if (len < 4) > + return -EINVAL; Do we really need it here? See below as well. > + /* Check for no-name case: "gpio" / "gpios" */ > + for (const char *const *p = gpio_suffixes; *p; p++) > + if (!strcmp(str, *p)) > + return 0; > + for (size_t i = len - 4; i > 0; i--) { > + /* find right-most '-' and check if remainder matches suffix */ > + if (str[i] != '-') > + continue; > + > + for (const char *const *p = gpio_suffixes; *p; p++) > + if (!strcmp(str + i + 1, *p)) > + return i; > + > + return -EINVAL; > + } This can be combined with the above for (const char *const *p = gpio_suffixes; *p; p++) { /* * Find right-most '-' and check if remainder matches suffix. * If no separator found, check for no-name cases. */ dash = strrchr(propname, '-'); if (!strcmp(dash ? dash + 1 : propname, *p)) return i; } > + return -EINVAL; > +}
On Thu, Aug 22, 2024 at 10:37 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Thu, Aug 22, 2024 at 05:19:58PM +0800, Chen-Yu Tsai wrote: > > The I2C device tree component prober needs to get and toggle GPIO lines > > for the components it intends to probe. These components may not use the > > same name for their GPIO lines, so the prober must go through the device > > tree, check each property to see it is a GPIO property, and get the GPIO > > line. > > > > Instead of duplicating the GPIO suffixes, or exporting them to the > > prober to do pattern matching, simply add and export a new function that > > does the pattern matching and returns the length of the GPIO name. The > > caller can then use that to copy out the name if it needs to. > > ... > > > +/** > > + * gpio_property_name_length - Returns the GPIO name length from a property name > > + * @str: string to check > > It's property name, so, I would name this 'propname'. Ack. > > + * This function checks if the given name matches the GPIO property patterns, and > > + * returns the length of the name of the GPIO. The pattern is "*-<GPIO suffix>" > > + * or just "<GPIO suffix>". > > + * > > + * Returns: > > + * The length of the string before '-' if it matches "*-<GPIO suffix>", or > > What about "x-y-gpios"? It's unclear what will be the behaviour. I thought it was implied that the '-' mentioned here is the one before the suffix. I made it more explicit. > > + * 0 if no name part, just the suffix, or > > + * -EINVAL if the string doesn't match the pattern. > > + */ > > +int gpio_property_name_length(const char *str) > > gpio_get_... ? Ack. > > +{ > > + size_t len; > > + > > + len = strlen(str); > > If it has a thousands characters...? Shouldn't matter much? I suppose using strrchr() as you suggested requires one less pass. > > + /* string need to be at minimum len(gpio) */ > > + if (len < 4) > > + return -EINVAL; > > Do we really need it here? See below as well. > > > + /* Check for no-name case: "gpio" / "gpios" */ > > + for (const char *const *p = gpio_suffixes; *p; p++) > > + if (!strcmp(str, *p)) > > + return 0; > > > + for (size_t i = len - 4; i > 0; i--) { > > + /* find right-most '-' and check if remainder matches suffix */ > > + if (str[i] != '-') > > + continue; > > + > > + for (const char *const *p = gpio_suffixes; *p; p++) > > + if (!strcmp(str + i + 1, *p)) > > + return i; > > + > > + return -EINVAL; > > + } > > This can be combined with the above > > for (const char *const *p = gpio_suffixes; *p; p++) { > /* > * Find right-most '-' and check if remainder matches suffix. > * If no separator found, check for no-name cases. > */ > dash = strrchr(propname, '-'); I believe this line could be moved out of the for-loop. Otherwise it looks much more concise compared to my version. I'll omit the comment though, as it is just rehashing the kerneldoc description, and now that the function is so short, it shouldn't be hard to read. I'll add you as "Suggested-by". Thanks ChenYu > if (!strcmp(dash ? dash + 1 : propname, *p)) > return i; > } > > > + return -EINVAL; > > +} > > -- > With Best Regards, > Andy Shevchenko > >
On Fri, Aug 23, 2024 at 03:50:55PM +0800, Chen-Yu Tsai wrote: > On Thu, Aug 22, 2024 at 10:37 PM Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: > > On Thu, Aug 22, 2024 at 05:19:58PM +0800, Chen-Yu Tsai wrote: ... > > > + len = strlen(str); > > > > If it has a thousands characters...? > > Shouldn't matter much? I suppose using strrchr() as you suggested > requires one less pass. Yes, this is the point. ... > > This can be combined with the above > > > > for (const char *const *p = gpio_suffixes; *p; p++) { > > /* > > * Find right-most '-' and check if remainder matches suffix. > > * If no separator found, check for no-name cases. > > */ > > dash = strrchr(propname, '-'); > > I believe this line could be moved out of the for-loop. Otherwise it > looks much more concise compared to my version. I'll omit the comment > though, as it is just rehashing the kerneldoc description, and now > that the function is so short, it shouldn't be hard to read. Agree. And I put comment inside the loop, while it should be outside. But then, as you said, the function becomes so little that kernel-doc above does the job, hence no comment in the code needed. > I'll add you as "Suggested-by". Fine with me. > > if (!strcmp(dash ? dash + 1 : propname, *p)) > > return i; > > }
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c index 3903d0a75304..e4228ef6f131 100644 --- a/drivers/gpio/gpiolib.c +++ b/drivers/gpio/gpiolib.c @@ -4295,6 +4295,50 @@ struct gpio_desc *fwnode_gpiod_get_index(struct fwnode_handle *fwnode, } EXPORT_SYMBOL_GPL(fwnode_gpiod_get_index); +/** + * gpio_property_name_length - Returns the GPIO name length from a property name + * @str: string to check + * + * This function checks if the given name matches the GPIO property patterns, and + * returns the length of the name of the GPIO. The pattern is "*-<GPIO suffix>" + * or just "<GPIO suffix>". + * + * Returns: + * The length of the string before '-' if it matches "*-<GPIO suffix>", or + * 0 if no name part, just the suffix, or + * -EINVAL if the string doesn't match the pattern. + */ +int gpio_property_name_length(const char *str) +{ + size_t len; + + len = strlen(str); + + /* string need to be at minimum len(gpio) */ + if (len < 4) + return -EINVAL; + + /* Check for no-name case: "gpio" / "gpios" */ + for (const char *const *p = gpio_suffixes; *p; p++) + if (!strcmp(str, *p)) + return 0; + + for (size_t i = len - 4; i > 0; i--) { + /* find right-most '-' and check if remainder matches suffix */ + if (str[i] != '-') + continue; + + for (const char *const *p = gpio_suffixes; *p; p++) + if (!strcmp(str + i + 1, *p)) + return i; + + return -EINVAL; + } + + return -EINVAL; +} +EXPORT_SYMBOL_GPL(gpio_property_name_length); + /** * gpiod_count - return the number of GPIOs associated with a device / function * or -ENOENT if no GPIO has been assigned to the requested function diff --git a/include/linux/gpio/consumer.h b/include/linux/gpio/consumer.h index db2dfbae8edb..ce3a5f86a037 100644 --- a/include/linux/gpio/consumer.h +++ b/include/linux/gpio/consumer.h @@ -56,6 +56,8 @@ enum gpiod_flags { #ifdef CONFIG_GPIOLIB +int gpio_property_name_length(const char *name); + /* Return the number of GPIOs associated with a device / function */ int gpiod_count(struct device *dev, const char *con_id);
The I2C device tree component prober needs to get and toggle GPIO lines for the components it intends to probe. These components may not use the same name for their GPIO lines, so the prober must go through the device tree, check each property to see it is a GPIO property, and get the GPIO line. Instead of duplicating the GPIO suffixes, or exporting them to the prober to do pattern matching, simply add and export a new function that does the pattern matching and returns the length of the GPIO name. The caller can then use that to copy out the name if it needs to. Signed-off-by: Chen-Yu Tsai <wenst@chromium.org> --- Changes since v4: - new patch Depends on commit 4b91188dced8 ("gpiolib: Replace gpio_suffix_count with NULL-terminated array"). --- drivers/gpio/gpiolib.c | 44 +++++++++++++++++++++++++++++++++++ include/linux/gpio/consumer.h | 2 ++ 2 files changed, 46 insertions(+)