| Message ID | 20170110143201.53539-4-mika.westerberg@linux.intel.com |
|---|---|
| State | New |
| Headers | show |
On Tue, Jan 10, 2017 at 3:31 PM, Mika Westerberg <mika.westerberg@linux.intel.com> wrote: > When a GPIO driver is backed by a pinctrl driver the GPIO driver often > needs to call the pinctrl driver to configure certain things, like > whether the pin is used as input or output. In addition to this there > are other pin configurations applicable to GPIOs such as debounce > timeout. > > To support this we introduce a new function pinctrl_gpio_set_config() > that can be used by gpiolib based driver to pass configuration requests > to the backing pinctrl driver. > > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com> OK so this is needed. But let's first pause and discuss this, because I have some stuff on my mind here. First this kernel-internal ABI from <linux/gpio/driver.h>: struct gpio_chip { (...) int (*set_debounce)(struct gpio_chip *chip, unsigned offset, unsigned debounce); int (*set_single_ended)(struct gpio_chip *chip, unsigned offset, enum single_ended_mode mode); (...) It's not going to scale. We need to replace this with something like int (*set_config)(struct gpio_chip *chip, unsigned offset, unsigned long config); Where "config" takes the packed format described in <linux/pinctrl/pinconf-generic.h> and nothing else, anything else is just inviting disaster. We can also later add: int (*get_config)(struct gpio_chip *chip, unsigned offset, unsigned long *config); We can then set and get arbitrary configs on GPIO lines, and the drivers can simply implement a switch() for the configs they handle else return -ENOTSUPP. But right now only set_config() would be enough. Maybe stuff needs to be split out of that header to be shared between GPIO and pinctrl but hopefully you could just include it. Then we change all in-kernel users of these two APIs over to set_config(). THEN we can think about cross-calling to pin control using the API from this patch. It should be a simple matter of just passing along the same config argument since we're using generic pin config. It's not like it's impossible to merge this patch first, but I want to get some order here. Are you convenient with doing the above patch as part of this series, or shall I do it first so you can rebase on it? (Will take some time if I do it...) We need this because GPIO is going to need more and more config to be done by pinctrl on its behalf, and it will have to go all the way to userspace in many cases, so we need this infrastructure in place. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jan 11, 2017 at 02:06:56PM +0100, Linus Walleij wrote: > On Tue, Jan 10, 2017 at 3:31 PM, Mika Westerberg > <mika.westerberg@linux.intel.com> wrote: > > > When a GPIO driver is backed by a pinctrl driver the GPIO driver often > > needs to call the pinctrl driver to configure certain things, like > > whether the pin is used as input or output. In addition to this there > > are other pin configurations applicable to GPIOs such as debounce > > timeout. > > > > To support this we introduce a new function pinctrl_gpio_set_config() > > that can be used by gpiolib based driver to pass configuration requests > > to the backing pinctrl driver. > > > > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com> > > OK so this is needed. The alternative would be to just handle this internally in pinctrl-intel.c but I thought it is better to make the functionality available for other drivers as well. > But let's first pause and discuss this, because I have some stuff on my > mind here. > > First this kernel-internal ABI from <linux/gpio/driver.h>: > > struct gpio_chip { > (...) > int (*set_debounce)(struct gpio_chip *chip, > unsigned offset, > unsigned debounce); > int (*set_single_ended)(struct gpio_chip *chip, > unsigned offset, > enum single_ended_mode mode); > (...) > > It's not going to scale. We need to replace this with something like > > int (*set_config)(struct gpio_chip *chip, unsigned offset, unsigned > long config); > > Where "config" takes the packed format described in > <linux/pinctrl/pinconf-generic.h> > and nothing else, anything else is just inviting disaster. > > We can also later add: > > int (*get_config)(struct gpio_chip *chip, unsigned offset, unsigned > long *config); > > We can then set and get arbitrary configs on GPIO lines, and the > drivers can simply implement a switch() for the configs they handle > else return -ENOTSUPP. > > But right now only set_config() would be enough. > > Maybe stuff needs to be split out of that header to be shared between > GPIO and pinctrl but hopefully you could just include it. > > Then we change all in-kernel users of these two APIs over to set_config(). > > THEN we can think about cross-calling to pin control using the API > from this patch. It should be a simple matter of just passing along the > same config argument since we're using generic pin config. > > It's not like it's impossible to merge this patch first, but I want to get some > order here. > > Are you convenient with doing the above patch as part of this series, or > shall I do it first so you can rebase on it? (Will take some time if I > do it...) Sure, I can take a look at it. > We need this because GPIO is going to need more and more config > to be done by pinctrl on its behalf, and it will have to go all the > way to userspace in many cases, so we need this infrastructure in > place. OK. -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jan 11, 2017 at 03:33:04PM +0200, Mika Westerberg wrote: > > But let's first pause and discuss this, because I have some stuff on my > > mind here. > > > > First this kernel-internal ABI from <linux/gpio/driver.h>: > > > > struct gpio_chip { > > (...) > > int (*set_debounce)(struct gpio_chip *chip, > > unsigned offset, > > unsigned debounce); > > int (*set_single_ended)(struct gpio_chip *chip, > > unsigned offset, > > enum single_ended_mode mode); > > (...) > > > > It's not going to scale. We need to replace this with something like > > > > int (*set_config)(struct gpio_chip *chip, unsigned offset, unsigned > > long config); > > > > Where "config" takes the packed format described in > > <linux/pinctrl/pinconf-generic.h> > > and nothing else, anything else is just inviting disaster. > > > > We can also later add: > > > > int (*get_config)(struct gpio_chip *chip, unsigned offset, unsigned > > long *config); > > > > We can then set and get arbitrary configs on GPIO lines, and the > > drivers can simply implement a switch() for the configs they handle > > else return -ENOTSUPP. > > > > But right now only set_config() would be enough. > > > > Maybe stuff needs to be split out of that header to be shared between > > GPIO and pinctrl but hopefully you could just include it. > > > > Then we change all in-kernel users of these two APIs over to set_config(). > > > > THEN we can think about cross-calling to pin control using the API > > from this patch. It should be a simple matter of just passing along the > > same config argument since we're using generic pin config. > > > > It's not like it's impossible to merge this patch first, but I want to get some > > order here. > > > > Are you convenient with doing the above patch as part of this series, or > > shall I do it first so you can rebase on it? (Will take some time if I > > do it...) > > Sure, I can take a look at it. Hmm, looking at users of .set_debounce() I can see that the debounce time can be quite large. For example some signals which are connected to physical push-buttons may need > 64ms debounce time. However, the current pinconfig value is defined to be unsigned long which on 32-bit architecture is 32-bits. From that the higher 16-bits are used as config leaving the value to be 16-bits. This gives maximum debounce time of 65535us. I don't think it can cover all the uses of .set_debounce(). This could also be problematic when specifying values for pull resistors. One solution is to convert the packed value to be u64 instead, leaving up to 48-bits for the value. Alternatively we could provide a scale field with the packed format. What do you think? -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jan 12, 2017 at 10:22 AM, Mika Westerberg <mika.westerberg@linux.intel.com> wrote: > Hmm, looking at users of .set_debounce() I can see that the debounce > time can be quite large. For example some signals which are connected to > physical push-buttons may need > 64ms debounce time. > > However, the current pinconfig value is defined to be unsigned long > which on 32-bit architecture is 32-bits. From that the higher 16-bits > are used as config leaving the value to be 16-bits. This gives maximum > debounce time of 65535us. I don't think it can cover all the uses of > .set_debounce(). This could also be problematic when specifying values > for pull resistors. > > One solution is to convert the packed value to be u64 instead, leaving > up to 48-bits for the value. Alternatively we could provide a scale > field with the packed format. Hm yeah as long as all in-kernel users survive I don't see why we couldn't just make it 64bit. Is it a big deal? A scale field (multiplier) can also work, I don't know which is most elegant. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Jan 13, 2017 at 04:36:42PM +0100, Linus Walleij wrote: > On Thu, Jan 12, 2017 at 10:22 AM, Mika Westerberg > <mika.westerberg@linux.intel.com> wrote: > > > Hmm, looking at users of .set_debounce() I can see that the debounce > > time can be quite large. For example some signals which are connected to > > physical push-buttons may need > 64ms debounce time. > > > > However, the current pinconfig value is defined to be unsigned long > > which on 32-bit architecture is 32-bits. From that the higher 16-bits > > are used as config leaving the value to be 16-bits. This gives maximum > > debounce time of 65535us. I don't think it can cover all the uses of > > .set_debounce(). This could also be problematic when specifying values > > for pull resistors. > > > > One solution is to convert the packed value to be u64 instead, leaving > > up to 48-bits for the value. Alternatively we could provide a scale > > field with the packed format. > > Hm yeah as long as all in-kernel users survive I don't see why we > couldn't just make it 64bit. Is it a big deal? As long as everyone is using those macros and inline functions from pinconf-generic.h, I think the conversion should be pretty straightforward. -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Jan 13, 2017 at 5:33 PM, Mika Westerberg <mika.westerberg@linux.intel.com> wrote: > On Fri, Jan 13, 2017 at 04:36:42PM +0100, Linus Walleij wrote: >> On Thu, Jan 12, 2017 at 10:22 AM, Mika Westerberg >> <mika.westerberg@linux.intel.com> wrote: >> >> > Hmm, looking at users of .set_debounce() I can see that the debounce >> > time can be quite large. For example some signals which are connected to >> > physical push-buttons may need > 64ms debounce time. >> > >> > However, the current pinconfig value is defined to be unsigned long >> > which on 32-bit architecture is 32-bits. From that the higher 16-bits >> > are used as config leaving the value to be 16-bits. This gives maximum >> > debounce time of 65535us. I don't think it can cover all the uses of >> > .set_debounce(). This could also be problematic when specifying values >> > for pull resistors. >> > >> > One solution is to convert the packed value to be u64 instead, leaving >> > up to 48-bits for the value. Alternatively we could provide a scale >> > field with the packed format. >> >> Hm yeah as long as all in-kernel users survive I don't see why we >> couldn't just make it 64bit. Is it a big deal? > > As long as everyone is using those macros and inline functions from > pinconf-generic.h, I think the conversion should be pretty > straightforward. I think I just make it a strict requirement that if people want to use the pinctrl back-end for GPIO they simply have to support generic pin control. It's not like they have something else already, and converting a driver is not any unreasonable amount of work. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c index fb38e208f32d..114b7a7bbaea 100644 --- a/drivers/pinctrl/core.c +++ b/drivers/pinctrl/core.c @@ -688,6 +688,34 @@ int pinctrl_gpio_direction_output(unsigned gpio) } EXPORT_SYMBOL_GPL(pinctrl_gpio_direction_output); +/** + * pinctrl_gpio_set_config() - Apply configs to given GPIO pin + * @gpio: the GPIO pin number from the GPIO subsystem number space + * + * This function should *ONLY* be used from gpiolib-based GPIO drivers, if + * they need to call the underlying pin controller to change GPIO config + * (for example set debounce time). + */ +int pinctrl_gpio_set_config(unsigned gpio, unsigned long *configs, + size_t nconfigs) +{ + struct pinctrl_gpio_range *range; + struct pinctrl_dev *pctldev; + int ret, pin; + + ret = pinctrl_get_device_gpio_range(gpio, &pctldev, &range); + if (ret) + return ret; + + mutex_lock(&pctldev->mutex); + pin = gpio_to_pin(range, gpio); + ret = pinconf_gpio_set_config(pctldev, pin, configs, nconfigs); + mutex_unlock(&pctldev->mutex); + + return ret; +} +EXPORT_SYMBOL_GPL(pinctrl_gpio_set_config); + static struct pinctrl_state *find_state(struct pinctrl *p, const char *name) { diff --git a/drivers/pinctrl/pinconf.c b/drivers/pinctrl/pinconf.c index 799048f3c8d4..537c219a4f60 100644 --- a/drivers/pinctrl/pinconf.c +++ b/drivers/pinctrl/pinconf.c @@ -200,6 +200,18 @@ int pinconf_apply_setting(struct pinctrl_setting const *setting) return 0; } +int pinconf_gpio_set_config(struct pinctrl_dev *pctldev, unsigned pin, + unsigned long *configs, size_t nconfigs) +{ + const struct pinconf_ops *ops; + + ops = pctldev->desc->confops; + if (!ops) + return -EINVAL; + + return ops->pin_config_set(pctldev, pin, configs, nconfigs); +} + #ifdef CONFIG_DEBUG_FS static void pinconf_show_config(struct seq_file *s, struct pinctrl_dev *pctldev, diff --git a/drivers/pinctrl/pinconf.h b/drivers/pinctrl/pinconf.h index 55c75780b3b2..6e94e101357f 100644 --- a/drivers/pinctrl/pinconf.h +++ b/drivers/pinctrl/pinconf.h @@ -20,6 +20,9 @@ int pinconf_map_to_setting(struct pinctrl_map const *map, void pinconf_free_setting(struct pinctrl_setting const *setting); int pinconf_apply_setting(struct pinctrl_setting const *setting); +int pinconf_gpio_set_config(struct pinctrl_dev *pctldev, unsigned pin, + unsigned long *configs, size_t nconfigs); + /* * You will only be interested in these if you're using PINCONF * so don't supply any stubs for these. @@ -56,6 +59,13 @@ static inline int pinconf_apply_setting(struct pinctrl_setting const *setting) return 0; } +static inline int pinconf_gpio_set_config(struct pinctrl_dev *pctldev, + unsigned pin, unsigned long *configs, + size_t nconfigs) +{ + return -ENXIO; +} + #endif #if defined(CONFIG_PINCONF) && defined(CONFIG_DEBUG_FS) diff --git a/include/linux/pinctrl/consumer.h b/include/linux/pinctrl/consumer.h index d7e5d608faa7..bf4dce0c04ea 100644 --- a/include/linux/pinctrl/consumer.h +++ b/include/linux/pinctrl/consumer.h @@ -29,6 +29,8 @@ extern int pinctrl_request_gpio(unsigned gpio); extern void pinctrl_free_gpio(unsigned gpio); extern int pinctrl_gpio_direction_input(unsigned gpio); extern int pinctrl_gpio_direction_output(unsigned gpio); +extern int pinctrl_gpio_set_config(unsigned gpio, unsigned long *configs, + size_t nconfigs); extern struct pinctrl * __must_check pinctrl_get(struct device *dev); extern void pinctrl_put(struct pinctrl *p); @@ -80,6 +82,12 @@ static inline int pinctrl_gpio_direction_output(unsigned gpio) return 0; } +static inline int pinctrl_gpio_set_config(unsigned gpio, unsigned long *configs, + size_t nconfigs) +{ + return 0; +} + static inline struct pinctrl * __must_check pinctrl_get(struct device *dev) { return NULL;
When a GPIO driver is backed by a pinctrl driver the GPIO driver often needs to call the pinctrl driver to configure certain things, like whether the pin is used as input or output. In addition to this there are other pin configurations applicable to GPIOs such as debounce timeout. To support this we introduce a new function pinctrl_gpio_set_config() that can be used by gpiolib based driver to pass configuration requests to the backing pinctrl driver. Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com> --- drivers/pinctrl/core.c | 28 ++++++++++++++++++++++++++++ drivers/pinctrl/pinconf.c | 12 ++++++++++++ drivers/pinctrl/pinconf.h | 10 ++++++++++ include/linux/pinctrl/consumer.h | 8 ++++++++ 4 files changed, 58 insertions(+)