Hello Jan, Thanks for your feedback. On Tue, 08 Jan 2019 15:00:22 +0100, Jan Kundrát wrote: > > diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c > > index 7f1260c78270..6518dc8c7c4c 100644 > > --- a/drivers/gpio/gpiolib-of.c > > +++ b/drivers/gpio/gpiolib-of.c > > @@ -323,6 +323,11 @@ struct gpio_desc *of_find_gpio(struct > > device *dev, const char *con_id, > > if (of_flags & OF_GPIO_TRANSITORY) > > *flags |= GPIO_TRANSITORY; > > > > + if (of_flags & OF_GPIO_PULL_UP) > > + *flags |= GPIO_PULL_UP; > > + else if (of_flags & OF_GPIO_PULL_DOWN) > > + *flags |= GPIO_PULL_DOWN; > > + > > return desc; > > } > > > > Hi Thomas, > my recommendation is to add an explicit "error handling" code here to warn > when the DT specifies both pull-up and pull-down bits. It's outside of any > hot path, and it will help identify mistakes instead of silently prefering > a random bit choice. Sure. > > +/* Bit 4 express pull up */ > > +#define GPIO_PULL_UP 16 > > + > > +/* Bit 5 express pull down */ > > +#define GPIO_PULL_DOWN 32 > > + > > > + GPIO_PULL_UP = (1 << 4), > > + GPIO_PULL_DOWN = (1 << 5), > > > + OF_GPIO_PULL_UP = 0x10, > > + OF_GPIO_PULL_DOWN = 0x20, > > I understand that it's already there, but I wonder if this duplication can > be removed. Am I missing something, perhaps a reason why > include/linux/of_gpio.h and include/dt-bindings/gpio/gpio.h are separate > files? I also wondered why there was such duplication when writing the patches. I've assumed it was done so that include/dt-bindings/gpio/gpio.h is "clean" enough to be included in DT, while include/linux/of_gpio.h some more elaborate definitions. But indeed <linux/of_gpio.h> could include <dt-bindings/gpio/gpio.h>. Perhaps there's a good reason for this duplication? Let's see the feedback of GPIO maintainers about this. Best regards, Thomas -- Thomas Petazzoni, CTO, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
