| Message ID | 1460394019-3393-2-git-send-email-eric@nelint.com |
|---|---|
| State | Superseded |
| Delegated to: | Tom Rini |
| Headers | show |
On 04/11/2016 11:00 AM, Eric Nelson wrote: > Many drivers use a common form of offset + flags for device > tree nodes. e.g.: > <&gpio1 2 GPIO_ACTIVE_LOW> > > This patch adds a common implementation of this type of parsing > and calls it when a gpio driver doesn't supply its' own xlate > routine. > > This will allow removal of the driver-specific versions in a > handful of drivers and simplify the addition of new drivers. The series, Acked-by: Stephen Warren <swarren@nvidia.com> Although one nit below. > diff --git a/drivers/gpio/gpio-uclass.c b/drivers/gpio/gpio-uclass.c > +int gpio_xlate_offs_flags(struct udevice *dev, > + struct gpio_desc *desc, > + struct fdtdec_phandle_args *args) > +{ > + int ret = -EINVAL; > + if (args->args_count > 1) { > + desc->offset = args->args[0]; > + if (args->args[1] & GPIO_ACTIVE_LOW) > + desc->flags = GPIOD_ACTIVE_LOW; > + ret = 0; > + } > + return ret; > +} Why not return the error first, and avoid the need for an extra indentation level for the rest of the function, and make it quite a bit more readable in my opinion. The default could also be made to cover more situations (e.g. bindings that define a single cell for the GPIO but not cell at all for any flags): if (args->args_count < 1) return -EINVAL; desc->offset = args->args[0]; if (args->args_count < 2) return 0; if (args->args[1] & GPIO_ACTIVE_LOW) desc->flags = GPIOD_ACTIVE_LOW; return 0;
Hi Stephen, On 04/11/2016 10:16 AM, Stephen Warren wrote: > On 04/11/2016 11:00 AM, Eric Nelson wrote: >> Many drivers use a common form of offset + flags for device >> tree nodes. e.g.: >> <&gpio1 2 GPIO_ACTIVE_LOW> >> >> This patch adds a common implementation of this type of parsing >> and calls it when a gpio driver doesn't supply its' own xlate >> routine. >> >> This will allow removal of the driver-specific versions in a >> handful of drivers and simplify the addition of new drivers. > > The series, > Acked-by: Stephen Warren <swarren@nvidia.com> > > Although one nit below. > >> diff --git a/drivers/gpio/gpio-uclass.c b/drivers/gpio/gpio-uclass.c > >> +int gpio_xlate_offs_flags(struct udevice *dev, >> + struct gpio_desc *desc, >> + struct fdtdec_phandle_args *args) >> +{ >> + int ret = -EINVAL; >> + if (args->args_count > 1) { >> + desc->offset = args->args[0]; >> + if (args->args[1] & GPIO_ACTIVE_LOW) >> + desc->flags = GPIOD_ACTIVE_LOW; >> + ret = 0; >> + } >> + return ret; >> +} > > Why not return the error first, and avoid the need for an extra > indentation level for the rest of the function, and make it quite a bit > more readable in my opinion. The default could also be made to cover > more situations (e.g. bindings that define a single cell for the GPIO > but not cell at all for any flags): > > if (args->args_count < 1) > return -EINVAL; > > desc->offset = args->args[0]; > > if (args->args_count < 2) > return 0; > > if (args->args[1] & GPIO_ACTIVE_LOW) > desc->flags = GPIOD_ACTIVE_LOW; > > return 0; I like it. V3 of that one patch forthcoming.
diff --git a/drivers/gpio/gpio-uclass.c b/drivers/gpio/gpio-uclass.c index b58d4e6..6c24e65 100644 --- a/drivers/gpio/gpio-uclass.c +++ b/drivers/gpio/gpio-uclass.c @@ -6,6 +6,7 @@ #include <common.h> #include <dm.h> +#include <dt-bindings/gpio/gpio.h> #include <errno.h> #include <fdtdec.h> #include <malloc.h> @@ -113,19 +114,29 @@ int gpio_lookup_name(const char *name, struct udevice **devp, return 0; } +int gpio_xlate_offs_flags(struct udevice *dev, + struct gpio_desc *desc, + struct fdtdec_phandle_args *args) +{ + int ret = -EINVAL; + if (args->args_count > 1) { + desc->offset = args->args[0]; + if (args->args[1] & GPIO_ACTIVE_LOW) + desc->flags = GPIOD_ACTIVE_LOW; + ret = 0; + } + return ret; +} + static int gpio_find_and_xlate(struct gpio_desc *desc, struct fdtdec_phandle_args *args) { struct dm_gpio_ops *ops = gpio_get_ops(desc->dev); - /* Use the first argument as the offset by default */ - if (args->args_count > 0) - desc->offset = args->args[0]; + if (ops->xlate) + return ops->xlate(desc->dev, desc, args); else - desc->offset = -1; - desc->flags = 0; - - return ops->xlate ? ops->xlate(desc->dev, desc, args) : 0; + return gpio_xlate_offs_flags(desc->dev, desc, args); } int dm_gpio_request(struct gpio_desc *desc, const char *label) @@ -605,6 +616,7 @@ static int _gpio_request_by_name_nodev(const void *blob, int node, desc->dev = NULL; desc->offset = 0; + desc->flags = 0; ret = fdtdec_parse_phandle_with_args(blob, node, list_name, "#gpio-cells", 0, index, &args); if (ret) { diff --git a/include/asm-generic/gpio.h b/include/asm-generic/gpio.h index 68b5f0b..2500c10 100644 --- a/include/asm-generic/gpio.h +++ b/include/asm-generic/gpio.h @@ -207,6 +207,16 @@ int gpio_requestf(unsigned gpio, const char *fmt, ...) struct fdtdec_phandle_args; /** + * gpio_xlate_offs_flags() - implementation for common use of dm_gpio_ops.xlate + * + * This routine sets the offset field to args[0] and the flags field to + * GPIOD_ACTIVE_LOW if the GPIO_ACTIVE_LOW flag is present in args[1]. + * + */ +int gpio_xlate_offs_flags(struct udevice *dev, struct gpio_desc *desc, + struct fdtdec_phandle_args *args); + +/** * struct struct dm_gpio_ops - Driver model GPIO operations * * Refer to functions above for description. These function largely copy @@ -258,12 +268,11 @@ struct dm_gpio_ops { * * @desc->dev to @dev * @desc->flags to 0 - * @desc->offset to the value of the first argument in args, if any, - * otherwise -1 (which is invalid) + * @desc->offset to 0 * - * This method is optional so if the above defaults suit it can be - * omitted. Typical behaviour is to set up the GPIOD_ACTIVE_LOW flag - * in desc->flags. + * This method is optional and defaults to gpio_xlate_offs_flags, + * which will parse offset and the GPIO_ACTIVE_LOW flag in the first + * two arguments. * * Note that @dev is passed in as a parameter to follow driver model * uclass conventions, even though it is already available as
Many drivers use a common form of offset + flags for device tree nodes. e.g.: <&gpio1 2 GPIO_ACTIVE_LOW> This patch adds a common implementation of this type of parsing and calls it when a gpio driver doesn't supply its' own xlate routine. This will allow removal of the driver-specific versions in a handful of drivers and simplify the addition of new drivers. Signed-off-by: Eric Nelson <eric@nelint.com> --- drivers/gpio/gpio-uclass.c | 26 +++++++++++++++++++------- include/asm-generic/gpio.h | 19 ++++++++++++++----- 2 files changed, 33 insertions(+), 12 deletions(-)