* [RFC PATCH 0/2] fixing the gpio ownership @ 2018-01-15 16:22 Ludovic Desroches 2018-01-15 16:22 ` [RFC PATCH 1/2] pinctrl: add consumer variant for gpio request Ludovic Desroches 2018-01-15 16:22 ` [RFC PATCH 2/2] gpio: provide a consumer when requesting a gpio Ludovic Desroches 0 siblings, 2 replies; 11+ messages in thread From: Ludovic Desroches @ 2018-01-15 16:22 UTC (permalink / raw) To: linux-gpio, linux-arm-kernel Cc: linus.walleij, linux-kernel, nicolas.free, Ludovic Desroches Hi, A few weeks ago, I have sent an RFC about adding bias support for GPIOs [1]. It was motivated by the fact that I wanted to enable the pinmuxing strict mode for my pin controller which can muxed a pin as a peripheral or as a GPIO. Enabling the strict mode prevents several devices to be probed because requesting a GPIO fails. The pin request function complains about the ownership of the GPIO which is different from the mux ownership. I have to remove my pinctrl node to avoid this conflict but I need it to configure my pins and to set a pull-up bias for my GPIOs. My first idea was to add new flags in addition to GPIO_ACTIVE_HIGH and others. Obviously, it was not the way to go since many new flags may be added: strength, debounce, etc. Then I proposed a very "quick and dirty" patch to give the picture of what I have in mind but I had no feedback. It was probably too dirty. The idea was to add a cell to the gpios property with a phandle on a pinctrl node which contains only the pinconf, no pinmux. The configuration is applied later when requesting the GPIO. The main issue is that enabling the strict mode will break old DTBs. I was going to submit patches for this but, after using the sysfs which still show me a bad ownership, I decided that it should be fixed. So I did these patches. Unfortunately, there are several ways to lead to gpiod_request(). It does the trick only for the gpiod_get family. The issue is still present with legacy gpio_request and fwnode_get_named_gpiod. It seems that more and more drivers are converted to use GPIO descriptors so there is some hope. The advantage of this solution is to not break old DTBs. As I am not aware of all usage of the gpiolib, I tried to implement it in the safest way. Regards Ludovic [1] https://www.spinics.net/lists/arm-kernel/msg623149.html Ludovic Desroches (2): pinctrl: add consumer variant for gpio request gpio: provide a consumer when requesting a gpio drivers/gpio/gpiolib.c | 40 +++++++++++++++++++++++++++++++++------- drivers/pinctrl/core.c | 13 ++++++++++--- drivers/pinctrl/pinmux.c | 16 ++++++++++++++-- drivers/pinctrl/pinmux.h | 10 ++++++++++ include/linux/gpio/driver.h | 5 +++++ include/linux/pinctrl/consumer.h | 6 ++++++ 6 files changed, 78 insertions(+), 12 deletions(-) -- 2.12.2 ^ permalink raw reply [flat|nested] 11+ messages in thread
* [RFC PATCH 1/2] pinctrl: add consumer variant for gpio request 2018-01-15 16:22 [RFC PATCH 0/2] fixing the gpio ownership Ludovic Desroches @ 2018-01-15 16:22 ` Ludovic Desroches 2018-01-15 20:19 ` Andy Shevchenko 2018-01-18 9:46 ` Linus Walleij 2018-01-15 16:22 ` [RFC PATCH 2/2] gpio: provide a consumer when requesting a gpio Ludovic Desroches 1 sibling, 2 replies; 11+ messages in thread From: Ludovic Desroches @ 2018-01-15 16:22 UTC (permalink / raw) To: linux-gpio, linux-arm-kernel Cc: linus.walleij, linux-kernel, nicolas.free, Ludovic Desroches Add a consumer variant to GPIO request relative functions. The goal is to fix the bad ownership, which is arbitrary set to "range->name:gpio", of a GPIO. There is a lack of configuration features for GPIO. For instance, we can't set the bias. Some pin controllers manage both device's pins and GPIOs. GPIOs can benefit from pin configuration. Usually, a pinctrl node is used to mux the pin as a GPIO and to set up its configuration. The pinmuxing strict mode involves that a pin which is muxed can't be requested as a GPIO if the owner is not the same. Unfortunately, the ownership of a GPIO is set arbitrarily to "range->name:gpio". So there is a mismatch about the ownership which prevents a device from being the owner of the pinmuxing and requesting the same pin as a GPIO. Adding some consumer variants for GPIO request stuff will allow to pass the name of the device which requests the GPIO to not return an error if it's also the owner of the pinmuxing. Signed-off-by: Ludovic Desroches <ludovic.desroches@microchip.com> --- drivers/pinctrl/core.c | 13 ++++++++++--- drivers/pinctrl/pinmux.c | 16 ++++++++++++++-- drivers/pinctrl/pinmux.h | 10 ++++++++++ include/linux/pinctrl/consumer.h | 6 ++++++ 4 files changed, 40 insertions(+), 5 deletions(-) diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c index 2c0dbfcff3e6..51c75a6057b2 100644 --- a/drivers/pinctrl/core.c +++ b/drivers/pinctrl/core.c @@ -733,14 +733,15 @@ int pinctrl_get_group_selector(struct pinctrl_dev *pctldev, } /** - * pinctrl_gpio_request() - request a single pin to be used as GPIO + * pinctrl_gpio_request_consumer() - request a single pin to be used as GPIO * @gpio: the GPIO pin number from the GPIO subsystem number space + * @consumer: the name of the device requesting the GPIO * * This function should *ONLY* be used from gpiolib-based GPIO drivers, * as part of their gpio_request() semantics, platforms and individual drivers * shall *NOT* request GPIO pins to be muxed in. */ -int pinctrl_gpio_request(unsigned gpio) +int pinctrl_gpio_request_consumer(unsigned gpio, const char *consumer) { struct pinctrl_dev *pctldev; struct pinctrl_gpio_range *range; @@ -759,12 +760,18 @@ int pinctrl_gpio_request(unsigned gpio) /* Convert to the pin controllers number space */ pin = gpio_to_pin(range, gpio); - ret = pinmux_request_gpio(pctldev, range, pin, gpio); + ret = pinmux_request_gpio_consumer(pctldev, range, pin, gpio, consumer); mutex_unlock(&pctldev->mutex); return ret; } +EXPORT_SYMBOL_GPL(pinctrl_gpio_request_consumer); + +int pinctrl_gpio_request(unsigned gpio) +{ + return pinctrl_gpio_request_consumer(gpio, NULL); +} EXPORT_SYMBOL_GPL(pinctrl_gpio_request); /** diff --git a/drivers/pinctrl/pinmux.c b/drivers/pinctrl/pinmux.c index 55502fc4479c..8d422eb0ed38 100644 --- a/drivers/pinctrl/pinmux.c +++ b/drivers/pinctrl/pinmux.c @@ -232,14 +232,19 @@ static const char *pin_free(struct pinctrl_dev *pctldev, int pin, * @pctldev: pin controller device affected * @pin: the pin to mux in for GPIO * @range: the applicable GPIO range + * @consumer: the name of the device requesting the GPIO */ -int pinmux_request_gpio(struct pinctrl_dev *pctldev, +int pinmux_request_gpio_consumer(struct pinctrl_dev *pctldev, struct pinctrl_gpio_range *range, - unsigned pin, unsigned gpio) + unsigned pin, unsigned gpio, + const char *consumer) { const char *owner; int ret; + if (consumer) + return pin_request(pctldev, pin, consumer, range); + /* Conjure some name stating what chip and pin this is taken by */ owner = kasprintf(GFP_KERNEL, "%s:%d", range->name, gpio); if (!owner) @@ -252,6 +257,13 @@ int pinmux_request_gpio(struct pinctrl_dev *pctldev, return ret; } +int pinmux_request_gpio(struct pinctrl_dev *pctldev, + struct pinctrl_gpio_range *range, + unsigned pin, unsigned gpio) +{ + return pinmux_request_gpio_consumer(pctldev, range, pin, gpio, NULL); +} + /** * pinmux_free_gpio() - release a pin from GPIO muxing * @pctldev: the pin controller device for the pin diff --git a/drivers/pinctrl/pinmux.h b/drivers/pinctrl/pinmux.h index a331fcdbedd9..837599922a42 100644 --- a/drivers/pinctrl/pinmux.h +++ b/drivers/pinctrl/pinmux.h @@ -19,6 +19,9 @@ int pinmux_validate_map(const struct pinctrl_map *map, int i); int pinmux_request_gpio(struct pinctrl_dev *pctldev, struct pinctrl_gpio_range *range, unsigned pin, unsigned gpio); +int pinmux_request_gpio_consumer(struct pinctrl_dev *pctldev, + struct pinctrl_gpio_range *range, + unsigned pin, unsigned gpio, const char *consumer); void pinmux_free_gpio(struct pinctrl_dev *pctldev, unsigned pin, struct pinctrl_gpio_range *range); int pinmux_gpio_direction(struct pinctrl_dev *pctldev, @@ -50,6 +53,13 @@ static inline int pinmux_request_gpio(struct pinctrl_dev *pctldev, return 0; } +static inline int pinmux_request_gpio_consumer(struct pinctrl_dev *pctldev, + struct pinctrl_gpio_range *range, + unsigned pin, unsigned gpio, const char *consumer) +{ + return 0; +} + static inline void pinmux_free_gpio(struct pinctrl_dev *pctldev, unsigned pin, struct pinctrl_gpio_range *range) diff --git a/include/linux/pinctrl/consumer.h b/include/linux/pinctrl/consumer.h index 0412cc9833e9..8c521a14db43 100644 --- a/include/linux/pinctrl/consumer.h +++ b/include/linux/pinctrl/consumer.h @@ -26,6 +26,7 @@ struct device; /* External interface to pin control */ extern int pinctrl_gpio_request(unsigned gpio); +extern int pinctrl_gpio_request_consumer(unsigned gpio, const char *consumer); extern void pinctrl_gpio_free(unsigned gpio); extern int pinctrl_gpio_direction_input(unsigned gpio); extern int pinctrl_gpio_direction_output(unsigned gpio); @@ -67,6 +68,11 @@ static inline int pinctrl_gpio_request(unsigned gpio) return 0; } +static inline int pinctrl_gpio_request_consumer(unsigned gpio, const char *consumer); +{ + return 0; +} + static inline void pinctrl_gpio_free(unsigned gpio) { } -- 2.12.2 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [RFC PATCH 1/2] pinctrl: add consumer variant for gpio request 2018-01-15 16:22 ` [RFC PATCH 1/2] pinctrl: add consumer variant for gpio request Ludovic Desroches @ 2018-01-15 20:19 ` Andy Shevchenko 2018-01-16 9:01 ` Ludovic Desroches 2018-01-18 9:46 ` Linus Walleij 1 sibling, 1 reply; 11+ messages in thread From: Andy Shevchenko @ 2018-01-15 20:19 UTC (permalink / raw) To: Ludovic Desroches Cc: open list:GPIO SUBSYSTEM, linux-arm Mailing List, Linus Walleij, Linux Kernel Mailing List, nicolas.free On Mon, Jan 15, 2018 at 6:22 PM, Ludovic Desroches <ludovic.desroches@microchip.com> wrote: Did I miss cover letter for this? > Add a consumer variant to GPIO request relative functions. The goal > is to fix the bad ownership, which is arbitrary set to > "range->name:gpio", of a GPIO. Hmm... It's supposed to be name of the owner of the pin range (pin control device name IIUC). > There is a lack of configuration features for GPIO. For instance, > we can't set the bias. Some pin controllers manage both device's > pins and GPIOs. GPIOs can benefit from pin configuration. Usually, > a pinctrl node is used to mux the pin as a GPIO and to set up its > configuration. Don't we have means to do that? At least that what I see in aspeed_gpio_set_config(). Or I missed a point here? > The pinmuxing strict mode involves that a pin which is muxed can't > be requested as a GPIO if the owner is not the same. Any elaborated example? > Unfortunately, > the ownership of a GPIO is set arbitrarily to "range->name:gpio". > So there is a mismatch about the ownership which prevents a device > from being the owner of the pinmuxing and requesting the same pin as > a GPIO. > Adding some consumer variants for GPIO request stuff will allow to > pass the name of the device which requests the GPIO to not return an > error if it's also the owner of the pinmuxing. I think we need something more generic in core than producing more API functions. But I would like to get problem first. > + if (consumer) > + return pin_request(pctldev, pin, consumer, range); > + Hmm... My understanding that GPIO is just a (special) function out of pin muxing. So, doing musing is essential to get proper function out of it. Does your hardware considers this differently? If so, I would really want to see some datasheets. -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC PATCH 1/2] pinctrl: add consumer variant for gpio request 2018-01-15 20:19 ` Andy Shevchenko @ 2018-01-16 9:01 ` Ludovic Desroches 2018-01-16 14:33 ` Andy Shevchenko 0 siblings, 1 reply; 11+ messages in thread From: Ludovic Desroches @ 2018-01-16 9:01 UTC (permalink / raw) To: Andy Shevchenko Cc: Ludovic Desroches, open list:GPIO SUBSYSTEM, linux-arm Mailing List, Linus Walleij, Linux Kernel Mailing List, nicolas.free Hi, On Mon, Jan 15, 2018 at 10:19:39PM +0200, Andy Shevchenko wrote: > On Mon, Jan 15, 2018 at 6:22 PM, Ludovic Desroches > <ludovic.desroches@microchip.com> wrote: > > Did I miss cover letter for this? > It seems: https://lkml.org/lkml/2018/1/15/841 > > Add a consumer variant to GPIO request relative functions. The goal > > is to fix the bad ownership, which is arbitrary set to > > "range->name:gpio", of a GPIO. > > Hmm... It's supposed to be name of the owner of the pin range (pin > control device name IIUC). > Yes, the owner is the pinctrl device. > > There is a lack of configuration features for GPIO. For instance, > > we can't set the bias. Some pin controllers manage both device's > > pins and GPIOs. GPIOs can benefit from pin configuration. Usually, > > a pinctrl node is used to mux the pin as a GPIO and to set up its > > configuration. > > Don't we have means to do that? > I don't think so. I am not the only one to have this issue for a long time. There is an interesting thread here: https://www.spinics.net/lists/linux-gpio/msg16769.html If things have changed and I missed it, please tell me. I have seen some improvements but it still don't fit my needs. > At least that what I see in aspeed_gpio_set_config(). > Yes this is part of the improvements I have seen. The set_config operation is used at several places in the gpiolib: - gpio_set_drive_single_ended - gpiod_set_debounce - gpiod_set_transitory It doesn't cover all my needs. In the cover letter, I mentionned that I based my job on this adding parameters I need. Someone told me it may be difficult to pul all the parameters in one cell. For instance, the drive strength is a numeric value using several bits. I am not sure duplicating the parameters we have for pinconf is the appropriate solution. Then I prepare a second set of patches to add a cell with a phandle on the pin configuration. I was going to send it when I realize that fixing the ownership issue is probably a better solution. It may involve more code change but less duplication. > Or I missed a point here? > > > The pinmuxing strict mode involves that a pin which is muxed can't > > be requested as a GPIO if the owner is not the same. > > Any elaborated example? > More details in the thread I mentionned earlier. I want to enable the strict mode to prevent weird behavior using the sysfs (or the char device). Moreover, the strict mode fits the behavior of my pin controller. In my DTS, at the moment, if a device needs GPIOs, then I add a pinctrl node where I put the pinmuxing of the pins as GPIOs and I set the configuration (for instance, bias pull-up, debounce). If the strict mode is enabled, when the driver will request the GPIOs, it will fail even if it's the owner of the pinmuxing. > > Unfortunately, > > the ownership of a GPIO is set arbitrarily to "range->name:gpio". > > So there is a mismatch about the ownership which prevents a device > > from being the owner of the pinmuxing and requesting the same pin as > > a GPIO. > > > Adding some consumer variants for GPIO request stuff will allow to > > pass the name of the device which requests the GPIO to not return an > > error if it's also the owner of the pinmuxing. > > I think we need something more generic in core than producing more API > functions. > I didn't want to introduce too many changes to keep it safe and I didn't want to break current API by adding a parameter. > But I would like to get problem first. > > > + if (consumer) > > + return pin_request(pctldev, pin, consumer, range); > > + > > Hmm... My understanding that GPIO is just a (special) function out of > pin muxing. So, doing musing is essential to get proper function out > of it. > > Does your hardware considers this differently? If so, I would really > want to see some datasheets. No you're right about the behavior of my hardware. Regards Ludovic ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC PATCH 1/2] pinctrl: add consumer variant for gpio request 2018-01-16 9:01 ` Ludovic Desroches @ 2018-01-16 14:33 ` Andy Shevchenko 2018-01-17 14:54 ` Ludovic Desroches 0 siblings, 1 reply; 11+ messages in thread From: Andy Shevchenko @ 2018-01-16 14:33 UTC (permalink / raw) To: Andy Shevchenko, open list:GPIO SUBSYSTEM, linux-arm Mailing List, Linus Walleij, Linux Kernel Mailing List, nicolas.free Cc: Ludovic Desroches On Tue, Jan 16, 2018 at 11:01 AM, Ludovic Desroches <ludovic.desroches@microchip.com> wrote: > On Mon, Jan 15, 2018 at 10:19:39PM +0200, Andy Shevchenko wrote: >> On Mon, Jan 15, 2018 at 6:22 PM, Ludovic Desroches >> <ludovic.desroches@microchip.com> wrote: >> >> Did I miss cover letter for this? > It seems: https://lkml.org/lkml/2018/1/15/841 >> > Add a consumer variant to GPIO request relative functions. The goal >> > is to fix the bad ownership, which is arbitrary set to >> > "range->name:gpio", of a GPIO. >> >> Hmm... It's supposed to be name of the owner of the pin range (pin >> control device name IIUC). > Yes, the owner is the pinctrl device. > There is an interesting thread here: > https://www.spinics.net/lists/linux-gpio/msg16769.html Okay, I have dove into all links provided. Below a set of my thoughts with regard to topic. > If things have changed and I missed it, please tell me. I have seen some > improvements but it still don't fit my needs. First of all, the main architectural issue with current pin control design is so called "states". It's quite artificial entity which is not represented by hardware per se. Instead we better to provide an API to pin configuration and pin muxing: I would like to switch to *this* pin function with *this* pin configuration. Second, the pin control and GPIOs are orthogonal as your hardware confirms. The propagating pin configuration or muxing via GPIO API looks to me a wrong direction. To the point of the specific issue you are trying to solve. I would rather agree with Maxime: "Something like "if the configuration is a gpio, and my pinctrl driver is also a gpio controller, and that gpiod_request is being called on that pin, hand over the reference" Just in case of architectural review imagine a below case. We have two UART devices which share same pins, and at the same time their pins can be switched to GPIO function. So, use cases and questions: - allow potential driver to switch between UARTs at run time - allow UART driver to switch mode between no flow control (2 wire mode) and HW flow (4 wire mode), though not specifically at run time - allow GPIO function for some pins at run time to support OOB wake up, for example, when UART is a console More caveats: - switching and reconfiguring pins at run time is a bad idea for the start (only some exceptions can be applied, see above) because of electrical properties of the devices and potential damage to the hardware - switching to GPIO is allowed at run time for the purpose of OOB wake source - after switching to GPIO and freeing it the pin configuration (and perhaps muxing) would return back to previous (before GPIO) settings (this would be helpful to case described above: OOB wake up) -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC PATCH 1/2] pinctrl: add consumer variant for gpio request 2018-01-16 14:33 ` Andy Shevchenko @ 2018-01-17 14:54 ` Ludovic Desroches 2018-01-17 16:07 ` Andy Shevchenko 0 siblings, 1 reply; 11+ messages in thread From: Ludovic Desroches @ 2018-01-17 14:54 UTC (permalink / raw) To: Andy Shevchenko Cc: open list:GPIO SUBSYSTEM, linux-arm Mailing List, Linus Walleij, Linux Kernel Mailing List, nicolas.ferre, Ludovic Desroches On Tue, Jan 16, 2018 at 04:33:29PM +0200, Andy Shevchenko wrote: > On Tue, Jan 16, 2018 at 11:01 AM, Ludovic Desroches > <ludovic.desroches@microchip.com> wrote: > > On Mon, Jan 15, 2018 at 10:19:39PM +0200, Andy Shevchenko wrote: > >> On Mon, Jan 15, 2018 at 6:22 PM, Ludovic Desroches > >> <ludovic.desroches@microchip.com> wrote: > >> > >> Did I miss cover letter for this? > > It seems: https://lkml.org/lkml/2018/1/15/841 > > >> > Add a consumer variant to GPIO request relative functions. The goal > >> > is to fix the bad ownership, which is arbitrary set to > >> > "range->name:gpio", of a GPIO. > >> > >> Hmm... It's supposed to be name of the owner of the pin range (pin > >> control device name IIUC). > > > Yes, the owner is the pinctrl device. > > > There is an interesting thread here: > > https://www.spinics.net/lists/linux-gpio/msg16769.html > > Okay, I have dove into all links provided. Below a set of my thoughts > with regard to topic. > > > If things have changed and I missed it, please tell me. I have seen some > > improvements but it still don't fit my needs. > > First of all, the main architectural issue with current pin control > design is so called "states". It's quite artificial entity which is > not represented by hardware per se. > > Instead we better to provide an API to pin configuration and pin > muxing: I would like to switch to *this* pin function with *this* pin > configuration. > gpio_request_enable() allows to switch to the GPIO pin function. To clarify the situation, from my point of view there are two issues: - Several pin controllers didn't enabled the strict mode when they were introduced. Nowadays, enabling it will break several DTs. Maybe a DT property to enable/disable strict mode could do the trick: we remove pinctrl nodes (so pinmux + pinconf) for pins which will be requested by gpiod_request() and we set the strict property to true. - But... The GPIO pin configuration is missing. Some configuration features have been added, we can continue to add new ones but I am not sure it's the best solution. At the moment, we rely on a single cell to manage the configuration. It may not be enough and each time a new pin configuration will appear, it will have to be added both to the gpiolib and pinconf. > Second, the pin control and GPIOs are orthogonal as your hardware > confirms. The propagating pin configuration or muxing via GPIO API > looks to me a wrong direction. > I agree but it seems we have started to follow this path. > To the point of the specific issue you are trying to solve. I would > rather agree with Maxime: > > "Something like "if the configuration is a gpio, and my pinctrl driver > is also a gpio controller, and that gpiod_request is being called on > that pin, hand over the reference" > Sorry, what do you see behind "hand over the reference"? > Just in case of architectural review imagine a below case. We have two > UART devices which share same pins, and at the same time their pins > can be switched to GPIO function. So, use cases and questions: > - allow potential driver to switch between UARTs at run time > - allow UART driver to switch mode between no flow control (2 wire > mode) and HW flow (4 wire mode), though not specifically at run time > - allow GPIO function for some pins at run time to support OOB wake > up, for example, when UART is a console > I have the feeling that 1 and 2 are achievable with pinctrl states. 3 may be also acheviable excepting if the pin is part of a pinctrl state. > More caveats: > - switching and reconfiguring pins at run time is a bad idea for the > start (only some exceptions can be applied, see above) because of > electrical properties of the devices and potential damage to the > hardware > - switching to GPIO is allowed at run time for the purpose of OOB wake source > - after switching to GPIO and freeing it the pin configuration (and > perhaps muxing) would return back to previous (before GPIO) settings > (this would be helpful to case described above: OOB wake up) > I share another case which is the one motivating me to do these patches. I am writing a driver for a new device which uses several lines. The lines used depends on the firmware loaded so there is no reason to reserve all the lines and so the pins corresponding to these lines. The pins will be requested as GPIOs because the pin controller in this specific case is bypassed. I mean, if the device uses a line, it will take the ownership even if it is muxed to a function. I want to prevent this. Before using the line, I'll request the pin as a GPIO. If an error occurs (this is why I need to enable the strict mode), it means the pin is already muxed and we are going to break the device which uses it. > -- > With Best Regards, > Andy Shevchenko > -- > 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC PATCH 1/2] pinctrl: add consumer variant for gpio request 2018-01-17 14:54 ` Ludovic Desroches @ 2018-01-17 16:07 ` Andy Shevchenko 2018-01-18 7:56 ` Ludovic Desroches 0 siblings, 1 reply; 11+ messages in thread From: Andy Shevchenko @ 2018-01-17 16:07 UTC (permalink / raw) To: Andy Shevchenko, open list:GPIO SUBSYSTEM, linux-arm Mailing List, Linus Walleij, Linux Kernel Mailing List, Nicolas Ferre Cc: Ludovic Desroches On Wed, Jan 17, 2018 at 4:54 PM, Ludovic Desroches <ludovic.desroches@microchip.com> wrote: > On Tue, Jan 16, 2018 at 04:33:29PM +0200, Andy Shevchenko wrote: >> First of all, the main architectural issue with current pin control >> design is so called "states". It's quite artificial entity which is >> not represented by hardware per se. >> >> Instead we better to provide an API to pin configuration and pin >> muxing: I would like to switch to *this* pin function with *this* pin >> configuration. > gpio_request_enable() allows to switch to the GPIO pin function. ...as a particular case of "set this pin to this function". > To clarify the situation, from my point of view there are two issues: > > - Several pin controllers didn't enabled the strict mode when they were > introduced. Nowadays, enabling it will break several DTs. Maybe a DT > property to enable/disable strict mode could do the trick: we remove > pinctrl nodes (so pinmux + pinconf) for pins which will be requested > by gpiod_request() and we set the strict property to true. OK. > - But... The GPIO pin configuration is missing. Here you mixed up the things. Either we are talking about GPIO configuration (direction, enabling/disabling I/O buffers), or we are talking about pin configuration (bias, drive strength, slew rate, debounce, etc.). > Some configuration features > have been added, we can continue to add new ones but I am not sure > it's the best solution. See below. > At the moment, we rely on a single cell to > manage the configuration. It may not be enough and each time a new pin > configuration will appear, it will have to be added both to the gpiolib > and pinconf. >> Second, the pin control and GPIOs are orthogonal as your hardware >> confirms. The propagating pin configuration or muxing via GPIO API >> looks to me a wrong direction. >> > > I agree but it seems we have started to follow this path. Which is still wrong and nothing prevents us to just stop here and consider better way. The issue is how we historically represent pins inside kernel and how hardware evolves at the same time. Looking from now retrospectively I can state the following: - each GPIO controller *might* have (few) capabilities of pin configuration - pin control may not necessary have GPIO function of the pin Design is now GPIO oriented while it should be pin oriented. So, instead of littering GPIO API, would we consider to redesign a bit from the above point of view? (Read ahead: to be backward compatible and be more friendly for simple GPIO IPs it would make sense to leave some of the basic pin properties to be controlled from GPIO API, otherwise forget completely about GPIO driver, and think of pin control driver for new complex IPs) [pin]: might have attached set of functions, set of [electrical] properties. It might be re-configured run time in terms of function and configuration. It might have none, one, or more owners (this is already an OS abstraction) Thus, the core function is pin request, while GPIO request is just a particular case. Owner of the pin is defined independently on what function or configuration is chosen. Therefore, we will have something like this (permissions): - none (no one can do anything, except acquiring an ownership == requesting pin) - exclusive (only one user allowed to cover all properties of the pin) - shared (several owners can do modify all or selected properties) - shared for all (anyone can do anything, perhaps we don't need this) >> To the point of the specific issue you are trying to solve. I would >> rather agree with Maxime: >> >> "Something like "if the configuration is a gpio, and my pinctrl driver >> is also a gpio controller, and that gpiod_request is being called on >> that pin, hand over the reference" > Sorry, what do you see behind "hand over the reference"? This is similar to what I called before as "shared" ownership. To be precise, transformation "exclusive" to "shared". >> Just in case of architectural review imagine a below case. We have two >> UART devices which share same pins, and at the same time their pins >> can be switched to GPIO function. So, use cases and questions: >> - allow potential driver to switch between UARTs at run time >> - allow UART driver to switch mode between no flow control (2 wire >> mode) and HW flow (4 wire mode), though not specifically at run time >> - allow GPIO function for some pins at run time to support OOB wake >> up, for example, when UART is a console >> > > I have the feeling that 1 and 2 are achievable with pinctrl states. 3 may be > also acheviable excepting if the pin is part of a pinctrl state. Please, no artificial states here. It brings more issues than solves. Deterministic is: - pin electrical properties - current (active) function - current (active) owner(s) Whatever currently means "states" they are defined per owner basis. >> More caveats: >> - switching and reconfiguring pins at run time is a bad idea for the >> start (only some exceptions can be applied, see above) because of >> electrical properties of the devices and potential damage to the >> hardware >> - switching to GPIO is allowed at run time for the purpose of OOB wake source >> - after switching to GPIO and freeing it the pin configuration (and >> perhaps muxing) would return back to previous (before GPIO) settings >> (this would be helpful to case described above: OOB wake up) >> > > I share another case which is the one motivating me to do these patches. > > I am writing a driver for a new device which uses several lines. The lines used > depends on the firmware loaded so there is no reason to reserve all the > lines and so the pins corresponding to these lines. Reserve how? In DT? > The pins will be requested as GPIOs because the pin controller in this specific > case is bypassed. I mean, if the device uses a line, it will take the ownership > even if it is muxed to a function. I want to prevent this. Yes, valid point. > Before using the line, I'll request the pin as a GPIO. If an error occurs (this > is why I need to enable the strict mode), it means the pin is already muxed and > we are going to break the device which uses it. Correct, or any other function. What we need is pin_request_function() API and pin_set_config(). GPIO is kinda legacy (in terns of configuring and controlling pins). So, consider my idea above about ownership. It would give you less pain how to proceed with pins. In DT or ACPI we may supply a property to tell OS how ownership has to be handled: strict (or "exclusive", one owner for pin properties), "shared", or "none" (not requested, first come, first served) Yes, pin function might need a special type of owners to do power management, for example, but in above scheme it would be just a subtype of "shared" (okay, "strict" in that way also would "shared" but only for PM core). -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC PATCH 1/2] pinctrl: add consumer variant for gpio request 2018-01-17 16:07 ` Andy Shevchenko @ 2018-01-18 7:56 ` Ludovic Desroches 0 siblings, 0 replies; 11+ messages in thread From: Ludovic Desroches @ 2018-01-18 7:56 UTC (permalink / raw) To: Andy Shevchenko Cc: open list:GPIO SUBSYSTEM, linux-arm Mailing List, Linus Walleij, Linux Kernel Mailing List, Nicolas Ferre, Ludovic Desroches On Wed, Jan 17, 2018 at 06:07:02PM +0200, Andy Shevchenko wrote: > On Wed, Jan 17, 2018 at 4:54 PM, Ludovic Desroches > <ludovic.desroches@microchip.com> wrote: > > On Tue, Jan 16, 2018 at 04:33:29PM +0200, Andy Shevchenko wrote: > > >> First of all, the main architectural issue with current pin control > >> design is so called "states". It's quite artificial entity which is > >> not represented by hardware per se. > >> > >> Instead we better to provide an API to pin configuration and pin > >> muxing: I would like to switch to *this* pin function with *this* pin > >> configuration. > > > gpio_request_enable() allows to switch to the GPIO pin function. > > ...as a particular case of "set this pin to this function". > > > To clarify the situation, from my point of view there are two issues: > > > > - Several pin controllers didn't enabled the strict mode when they were > > introduced. Nowadays, enabling it will break several DTs. Maybe a DT > > property to enable/disable strict mode could do the trick: we remove > > pinctrl nodes (so pinmux + pinconf) for pins which will be requested > > by gpiod_request() and we set the strict property to true. > > OK. > > > - But... The GPIO pin configuration is missing. > > Here you mixed up the things. Either we are talking about GPIO > configuration (direction, enabling/disabling I/O buffers), or we are > talking about pin configuration (bias, drive strength, slew rate, > debounce, etc.). I was talking about the pin configuration of the GPIO so about bias, drive strength, etc. > > > Some configuration features > > have been added, we can continue to add new ones but I am not sure > > it's the best solution. > > See below. > > > At the moment, we rely on a single cell to > > manage the configuration. It may not be enough and each time a new pin > > configuration will appear, it will have to be added both to the gpiolib > > and pinconf. > > >> Second, the pin control and GPIOs are orthogonal as your hardware > >> confirms. The propagating pin configuration or muxing via GPIO API > >> looks to me a wrong direction. > >> > > > > I agree but it seems we have started to follow this path. > > Which is still wrong and nothing prevents us to just stop here and > consider better way. > > The issue is how we historically represent pins inside kernel and how > hardware evolves at the same time. > > Looking from now retrospectively I can state the following: > - each GPIO controller *might* have (few) capabilities of pin configuration > - pin control may not necessary have GPIO function of the pin > > Design is now GPIO oriented while it should be pin oriented. > Agree > So, instead of littering GPIO API, would we consider to redesign a bit > from the above point of view? > > (Read ahead: to be backward compatible and be more friendly for simple > GPIO IPs it would make sense to leave some of the basic pin properties > to be controlled from GPIO API, otherwise forget completely about GPIO > driver, and think of pin control driver for new complex IPs) > > [pin]: might have attached set of functions, set of [electrical] properties. > It might be re-configured run time in terms of function and configuration. > It might have none, one, or more owners (this is already an OS abstraction) > > Thus, the core function is pin request, while GPIO request is just a > particular case. > Owner of the pin is defined independently on what function or > configuration is chosen. > > Therefore, we will have something like this (permissions): > - none (no one can do anything, except acquiring an ownership == requesting pin) > - exclusive (only one user allowed to cover all properties of the pin) > - shared (several owners can do modify all or selected properties) > - shared for all (anyone can do anything, perhaps we don't need this) > It seems nice. > >> To the point of the specific issue you are trying to solve. I would > >> rather agree with Maxime: > >> > >> "Something like "if the configuration is a gpio, and my pinctrl driver > >> is also a gpio controller, and that gpiod_request is being called on > >> that pin, hand over the reference" > > > Sorry, what do you see behind "hand over the reference"? > > This is similar to what I called before as "shared" ownership. To be > precise, transformation "exclusive" to "shared". > > >> Just in case of architectural review imagine a below case. We have two > >> UART devices which share same pins, and at the same time their pins > >> can be switched to GPIO function. So, use cases and questions: > >> - allow potential driver to switch between UARTs at run time > >> - allow UART driver to switch mode between no flow control (2 wire > >> mode) and HW flow (4 wire mode), though not specifically at run time > >> - allow GPIO function for some pins at run time to support OOB wake > >> up, for example, when UART is a console > >> > > > > I have the feeling that 1 and 2 are achievable with pinctrl states. 3 may be > > also acheviable excepting if the pin is part of a pinctrl state. > > Please, no artificial states here. It brings more issues than solves. > Deterministic is: > - pin electrical properties > - current (active) function > - current (active) owner(s) > > Whatever currently means "states" they are defined per owner basis. > > >> More caveats: > >> - switching and reconfiguring pins at run time is a bad idea for the > >> start (only some exceptions can be applied, see above) because of > >> electrical properties of the devices and potential damage to the > >> hardware > >> - switching to GPIO is allowed at run time for the purpose of OOB wake source > >> - after switching to GPIO and freeing it the pin configuration (and > >> perhaps muxing) would return back to previous (before GPIO) settings > >> (this would be helpful to case described above: OOB wake up) > >> > > > > I share another case which is the one motivating me to do these patches. > > > > I am writing a driver for a new device which uses several lines. The lines used > > depends on the firmware loaded so there is no reason to reserve all the > > lines and so the pins corresponding to these lines. > > Reserve how? In DT? > Yes with the usual pinctrl node describing all the pins which can be used by the device. Since the device pins is only SoC dependant, I would like to get the pins list in the driver (depending on the compatible string) or using line_name-gpio in the DT. I am not sure about which solution is the best one. Then, once the firmware is loaded, I can pick only the pins I need. > > The pins will be requested as GPIOs because the pin controller in this specific > > case is bypassed. I mean, if the device uses a line, it will take the ownership > > even if it is muxed to a function. I want to prevent this. > > Yes, valid point. > > > Before using the line, I'll request the pin as a GPIO. If an error occurs (this > > is why I need to enable the strict mode), it means the pin is already muxed and > > we are going to break the device which uses it. > > Correct, or any other function. > > What we need is pin_request_function() API and pin_set_config(). GPIO > is kinda legacy (in terns of configuring and controlling pins). > > So, consider my idea above about ownership. It would give you less > pain how to proceed with pins. In DT or ACPI we may supply a property > to tell OS how ownership has to be handled: > strict (or "exclusive", one owner for pin properties), "shared", or > "none" (not requested, first come, first served) > I have to clear up my mind regarding your interesting proposals. > Yes, pin function might need a special type of owners to do power > management, for example, but in above scheme it would be just a > subtype of "shared" (okay, "strict" in that way also would "shared" > but only for PM core). > > -- > With Best Regards, > Andy Shevchenko > -- > 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC PATCH 1/2] pinctrl: add consumer variant for gpio request 2018-01-15 16:22 ` [RFC PATCH 1/2] pinctrl: add consumer variant for gpio request Ludovic Desroches 2018-01-15 20:19 ` Andy Shevchenko @ 2018-01-18 9:46 ` Linus Walleij 1 sibling, 0 replies; 11+ messages in thread From: Linus Walleij @ 2018-01-18 9:46 UTC (permalink / raw) To: Ludovic Desroches; +Cc: linux-gpio, Linux ARM, linux-kernel, nicolas.free On Mon, Jan 15, 2018 at 5:22 PM, Ludovic Desroches <ludovic.desroches@microchip.com> wrote: > Add a consumer variant to GPIO request relative functions. The goal > is to fix the bad ownership, which is arbitrary set to > "range->name:gpio", of a GPIO. For this patch on its own (apart from the context): I what you want to achieve is to pass a consumer name from gpiolib to pinmux portions of pinctrl, then augment the existing pinctrl_gpio_request() to pass an optional consumer name, and change all existing in-kernel users to just pass NULL and then use the range name as fallback if the consumer name is NULL. > There is a lack of configuration features for GPIO. For instance, > we can't set the bias. Some pin controllers manage both device's > pins and GPIOs. GPIOs can benefit from pin configuration. Usually, > a pinctrl node is used to mux the pin as a GPIO and to set up its > configuration. Pin config takes care of bias, pull up/down, drive strength etc etc. GPIO hammers lines 0->1, 1->2, and reads them as 0 or 1. It can group lines into arrays etc but that is what it does. The two systems are cross-connected using the GPIO ranges. There are cross-calls for GPIO to ask pin controllers for favors: extern int pinctrl_gpio_request(unsigned gpio); extern void pinctrl_gpio_free(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 config); Note: when the GPIO hardware is very simple apart from some extra register or two providing open drain and/or debounce, the gpio driver can simply implement .set_config() itself so no pin control back-end is needed. We could require a separate pin config driver but why complicate things for the sake of abstraction. Nah. The last API (pinctrl_gpio_set_config()) should only be called to set up electrical properties under special circumstances. Those are as follows: 1) When the in-kernel client needs to configure electrical properties, gpiolib exposes interfaces for this, such as: int gpiod_set_debounce(struct gpio_desc *desc, unsigned debounce); int gpiod_set_transitory(struct gpio_desc *desc, bool transitory); Likewise devm_gpiod_get() can pass flags such as GPIOD_OUT_LOW_OPEN_DRAIN to state that "I want this line, and I want it to be initialized logical low (deasserted), and it must be open drain" If the corresponding device tree phandle flag or board file description or whatever is not also flagging the line as open drain, gpiolib will protest with a warning print and enforce open drain. But it is clear that it *should* have been configured correctly in the device tree. This API is not all inclusive: notice that we just support open drain, not open source. (No upfront design: we just deal with what drivers need, not what they may theoretically need.) This is partly for historical reasons, but it also makes sense that things like I2C that can only work electrically with open drain, has a way to specify that these lines must really be configured as open drain for this thing to work. This is necessary when the logic of the code must tell gpiolib how to electrically use the pin, i.e. it is not a static configuration that comes from device tree or ACPI or a board file. This is why open drain/source and debounce can be set up from the in-kernel API. 2) Userspace consumers. To be clear: these are not laptops, desktops, servers, set-top boxes, mobile phones etc. Anything comprising a mass-produced system should NOT use userspace GPIO. That is just WRONG. Real, widely deployed systems should have proper kernel drivers for their hardware and deploy their systems with pin config and GPIO use cases set up in device tree or ACPI or whatever. NOT in userspace scripts. Userpace consumers are automatic control: oil burners, electric bikes, door openers, alarms etc using some GPIO lines as, yeah, essentially as GPIO lines. "General purpose", as opposed to "dedicated purpose". These users include the maker community that do some fiddely-fiddling with GPIOs from userspace, and that is fine. Future of this ABI/API: I do not yet know how much pin config we should allow to come in from this end *ONLY*. What does userspace consumers really need? Also there is no reciprocation between userspace ABI and in-kernel API. If we for example decide to let bias and drive strength be set up from userspace, that does not necessarily mean that we must let all in-kernel drivers do that too. We can allow .set_config() to be called from the userspace ABI and down to .set_config() in the pin control driver ONLY without allowing the same path for in-kernel users. Yours, Linus Walleij ^ permalink raw reply [flat|nested] 11+ messages in thread
* [RFC PATCH 2/2] gpio: provide a consumer when requesting a gpio 2018-01-15 16:22 [RFC PATCH 0/2] fixing the gpio ownership Ludovic Desroches 2018-01-15 16:22 ` [RFC PATCH 1/2] pinctrl: add consumer variant for gpio request Ludovic Desroches @ 2018-01-15 16:22 ` Ludovic Desroches 1 sibling, 0 replies; 11+ messages in thread From: Ludovic Desroches @ 2018-01-15 16:22 UTC (permalink / raw) To: linux-gpio, linux-arm-kernel Cc: linus.walleij, linux-kernel, nicolas.free, Ludovic Desroches It can be useful for the pinmuxing layer to know which device is requesting a GPIO. Add a consumer variant for gpiod_request to reach this goal. GPIO chips managed by pin controllers should provide the new request_consumer operation. They can rely on gpiochip_generic_request_consumer instead of gpiochip_generic_request. Signed-off-by: Ludovic Desroches <ludovic.desroches@microchip.com> --- drivers/gpio/gpiolib.c | 40 +++++++++++++++++++++++++++++++++------- include/linux/gpio/driver.h | 5 +++++ 2 files changed, 38 insertions(+), 7 deletions(-) diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c index 39a0144d5be5..e6645101ec74 100644 --- a/drivers/gpio/gpiolib.c +++ b/drivers/gpio/gpiolib.c @@ -1970,6 +1970,20 @@ int gpiochip_generic_request(struct gpio_chip *chip, unsigned offset) EXPORT_SYMBOL_GPL(gpiochip_generic_request); /** + * gpiochip_generic_request_consumer() - request the gpio function for a pin + * @chip: the gpiochip owning the GPIO + * @offset: the offset of the GPIO to request for GPIO function + * @consumer: name of the device requesting the GPIO + */ +int gpiochip_generic_request_consumer(struct gpio_chip *chip, unsigned offset, + const char *consumer) +{ + return pinctrl_gpio_request_consumer(chip->gpiodev->base + offset, + consumer); +} +EXPORT_SYMBOL_GPL(gpiochip_generic_request_consumer); + +/** * gpiochip_generic_free() - free the gpio function from a pin * @chip: the gpiochip to request the gpio function for * @offset: the offset of the GPIO to free from GPIO function @@ -2119,7 +2133,8 @@ EXPORT_SYMBOL_GPL(gpiochip_remove_pin_ranges); * on each other, and help provide better diagnostics in debugfs. * They're called even less than the "set direction" calls. */ -static int gpiod_request_commit(struct gpio_desc *desc, const char *label) +static int gpiod_request_commit(struct gpio_desc *desc, const char *label, + const char *consumer) { struct gpio_chip *chip = desc->gdev->chip; int status; @@ -2139,10 +2154,14 @@ static int gpiod_request_commit(struct gpio_desc *desc, const char *label) goto done; } - if (chip->request) { + if (chip->request || chip->request_consumer) { /* chip->request may sleep */ spin_unlock_irqrestore(&gpio_lock, flags); - status = chip->request(chip, gpio_chip_hwgpio(desc)); + if (chip->request_consumer) + status = chip->request_consumer(chip, + gpio_chip_hwgpio(desc), consumer); + else + status = chip->request(chip, gpio_chip_hwgpio(desc)); spin_lock_irqsave(&gpio_lock, flags); if (status < 0) { @@ -2200,7 +2219,8 @@ static int validate_desc(const struct gpio_desc *desc, const char *func) return; \ } while (0) -int gpiod_request(struct gpio_desc *desc, const char *label) +int gpiod_request_consumer(struct gpio_desc *desc, const char *label, + const char *consumer) { int status = -EPROBE_DEFER; struct gpio_device *gdev; @@ -2209,7 +2229,7 @@ int gpiod_request(struct gpio_desc *desc, const char *label) gdev = desc->gdev; if (try_module_get(gdev->owner)) { - status = gpiod_request_commit(desc, label); + status = gpiod_request_commit(desc, label, consumer); if (status < 0) module_put(gdev->owner); else @@ -2222,6 +2242,11 @@ int gpiod_request(struct gpio_desc *desc, const char *label) return status; } +int gpiod_request(struct gpio_desc *desc, const char *label) +{ + return gpiod_request_consumer(desc, label, NULL); +} + static bool gpiod_free_commit(struct gpio_desc *desc) { bool ret = false; @@ -2320,7 +2345,7 @@ struct gpio_desc *gpiochip_request_own_desc(struct gpio_chip *chip, u16 hwnum, return desc; } - err = gpiod_request_commit(desc, label); + err = gpiod_request_commit(desc, label, NULL); if (err < 0) return ERR_PTR(err); @@ -3672,7 +3697,8 @@ struct gpio_desc *__must_check gpiod_get_index(struct device *dev, } /* If a connection label was passed use that, else use the device name as label */ - status = gpiod_request(desc, con_id ? con_id : dev_name(dev)); + status = gpiod_request_consumer(desc, con_id ? con_id : dev_name(dev), + dev_name(dev)); if (status < 0) return ERR_PTR(status); diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h index 1ba9a331ec51..6bc5c57f0cbd 100644 --- a/include/linux/gpio/driver.h +++ b/include/linux/gpio/driver.h @@ -228,6 +228,9 @@ struct gpio_chip { int (*request)(struct gpio_chip *chip, unsigned offset); + int (*request_consumer)(struct gpio_chip *chip, + unsigned offset, + const char *consumer); void (*free)(struct gpio_chip *chip, unsigned offset); int (*get_direction)(struct gpio_chip *chip, @@ -500,6 +503,8 @@ static inline int gpiochip_irqchip_add_nested(struct gpio_chip *gpiochip, #endif /* CONFIG_GPIOLIB_IRQCHIP */ int gpiochip_generic_request(struct gpio_chip *chip, unsigned offset); +int gpiochip_generic_request_consumer(struct gpio_chip *chip, unsigned offset, + const char *consumer); void gpiochip_generic_free(struct gpio_chip *chip, unsigned offset); int gpiochip_generic_config(struct gpio_chip *chip, unsigned offset, unsigned long config); -- 2.12.2 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* [RESEND RFC PATCH 0/2] fixing the gpio ownership @ 2018-01-15 16:24 Ludovic Desroches 2018-01-15 16:24 ` [RFC PATCH 1/2] pinctrl: add consumer variant for gpio request Ludovic Desroches 0 siblings, 1 reply; 11+ messages in thread From: Ludovic Desroches @ 2018-01-15 16:24 UTC (permalink / raw) To: linux-gpio, linux-arm-kernel Cc: linus.walleij, linux-kernel, nicolas.ferre, Ludovic Desroches RESEND: fix typo in email address. Hi, A few weeks ago, I have sent an RFC about adding bias support for GPIOs [1]. It was motivated by the fact that I wanted to enable the pinmuxing strict mode for my pin controller which can muxed a pin as a peripheral or as a GPIO. Enabling the strict mode prevents several devices to be probed because requesting a GPIO fails. The pin request function complains about the ownership of the GPIO which is different from the mux ownership. I have to remove my pinctrl node to avoid this conflict but I need it to configure my pins and to set a pull-up bias for my GPIOs. My first idea was to add new flags in addition to GPIO_ACTIVE_HIGH and others. Obviously, it was not the way to go since many new flags may be added: strength, debounce, etc. Then I proposed a very "quick and dirty" patch to give the picture of what I have in mind but I had no feedback. It was probably too dirty. The idea was to add a cell to the gpios property with a phandle on a pinctrl node which contains only the pinconf, no pinmux. The configuration is applied later when requesting the GPIO. The main issue is that enabling the strict mode will break old DTBs. I was going to submit patches for this but, after using the sysfs which still show me a bad ownership, I decided that it should be fixed. So I did these patches. Unfortunately, there are several ways to lead to gpiod_request(). It does the trick only for the gpiod_get family. The issue is still present with legacy gpio_request and fwnode_get_named_gpiod. It seems that more and more drivers are converted to use GPIO descriptors so there is some hope. The advantage of this solution is to not break old DTBs. As I am not aware of all usage of the gpiolib, I tried to implement it in the safest way. Regards Ludovic [1] https://www.spinics.net/lists/arm-kernel/msg623149.html Ludovic Desroches (2): pinctrl: add consumer variant for gpio request gpio: provide a consumer when requesting a gpio drivers/gpio/gpiolib.c | 40 +++++++++++++++++++++++++++++++++------- drivers/pinctrl/core.c | 13 ++++++++++--- drivers/pinctrl/pinmux.c | 16 ++++++++++++++-- drivers/pinctrl/pinmux.h | 10 ++++++++++ include/linux/gpio/driver.h | 5 +++++ include/linux/pinctrl/consumer.h | 6 ++++++ 6 files changed, 78 insertions(+), 12 deletions(-) -- 2.12.2 ^ permalink raw reply [flat|nested] 11+ messages in thread
* [RFC PATCH 1/2] pinctrl: add consumer variant for gpio request 2018-01-15 16:24 [RESEND RFC PATCH 0/2] fixing the gpio ownership Ludovic Desroches @ 2018-01-15 16:24 ` Ludovic Desroches 0 siblings, 0 replies; 11+ messages in thread From: Ludovic Desroches @ 2018-01-15 16:24 UTC (permalink / raw) To: linux-gpio, linux-arm-kernel Cc: linus.walleij, linux-kernel, nicolas.ferre, Ludovic Desroches Add a consumer variant to GPIO request relative functions. The goal is to fix the bad ownership, which is arbitrary set to "range->name:gpio", of a GPIO. There is a lack of configuration features for GPIO. For instance, we can't set the bias. Some pin controllers manage both device's pins and GPIOs. GPIOs can benefit from pin configuration. Usually, a pinctrl node is used to mux the pin as a GPIO and to set up its configuration. The pinmuxing strict mode involves that a pin which is muxed can't be requested as a GPIO if the owner is not the same. Unfortunately, the ownership of a GPIO is set arbitrarily to "range->name:gpio". So there is a mismatch about the ownership which prevents a device from being the owner of the pinmuxing and requesting the same pin as a GPIO. Adding some consumer variants for GPIO request stuff will allow to pass the name of the device which requests the GPIO to not return an error if it's also the owner of the pinmuxing. Signed-off-by: Ludovic Desroches <ludovic.desroches@microchip.com> --- drivers/pinctrl/core.c | 13 ++++++++++--- drivers/pinctrl/pinmux.c | 16 ++++++++++++++-- drivers/pinctrl/pinmux.h | 10 ++++++++++ include/linux/pinctrl/consumer.h | 6 ++++++ 4 files changed, 40 insertions(+), 5 deletions(-) diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c index 2c0dbfcff3e6..51c75a6057b2 100644 --- a/drivers/pinctrl/core.c +++ b/drivers/pinctrl/core.c @@ -733,14 +733,15 @@ int pinctrl_get_group_selector(struct pinctrl_dev *pctldev, } /** - * pinctrl_gpio_request() - request a single pin to be used as GPIO + * pinctrl_gpio_request_consumer() - request a single pin to be used as GPIO * @gpio: the GPIO pin number from the GPIO subsystem number space + * @consumer: the name of the device requesting the GPIO * * This function should *ONLY* be used from gpiolib-based GPIO drivers, * as part of their gpio_request() semantics, platforms and individual drivers * shall *NOT* request GPIO pins to be muxed in. */ -int pinctrl_gpio_request(unsigned gpio) +int pinctrl_gpio_request_consumer(unsigned gpio, const char *consumer) { struct pinctrl_dev *pctldev; struct pinctrl_gpio_range *range; @@ -759,12 +760,18 @@ int pinctrl_gpio_request(unsigned gpio) /* Convert to the pin controllers number space */ pin = gpio_to_pin(range, gpio); - ret = pinmux_request_gpio(pctldev, range, pin, gpio); + ret = pinmux_request_gpio_consumer(pctldev, range, pin, gpio, consumer); mutex_unlock(&pctldev->mutex); return ret; } +EXPORT_SYMBOL_GPL(pinctrl_gpio_request_consumer); + +int pinctrl_gpio_request(unsigned gpio) +{ + return pinctrl_gpio_request_consumer(gpio, NULL); +} EXPORT_SYMBOL_GPL(pinctrl_gpio_request); /** diff --git a/drivers/pinctrl/pinmux.c b/drivers/pinctrl/pinmux.c index 55502fc4479c..8d422eb0ed38 100644 --- a/drivers/pinctrl/pinmux.c +++ b/drivers/pinctrl/pinmux.c @@ -232,14 +232,19 @@ static const char *pin_free(struct pinctrl_dev *pctldev, int pin, * @pctldev: pin controller device affected * @pin: the pin to mux in for GPIO * @range: the applicable GPIO range + * @consumer: the name of the device requesting the GPIO */ -int pinmux_request_gpio(struct pinctrl_dev *pctldev, +int pinmux_request_gpio_consumer(struct pinctrl_dev *pctldev, struct pinctrl_gpio_range *range, - unsigned pin, unsigned gpio) + unsigned pin, unsigned gpio, + const char *consumer) { const char *owner; int ret; + if (consumer) + return pin_request(pctldev, pin, consumer, range); + /* Conjure some name stating what chip and pin this is taken by */ owner = kasprintf(GFP_KERNEL, "%s:%d", range->name, gpio); if (!owner) @@ -252,6 +257,13 @@ int pinmux_request_gpio(struct pinctrl_dev *pctldev, return ret; } +int pinmux_request_gpio(struct pinctrl_dev *pctldev, + struct pinctrl_gpio_range *range, + unsigned pin, unsigned gpio) +{ + return pinmux_request_gpio_consumer(pctldev, range, pin, gpio, NULL); +} + /** * pinmux_free_gpio() - release a pin from GPIO muxing * @pctldev: the pin controller device for the pin diff --git a/drivers/pinctrl/pinmux.h b/drivers/pinctrl/pinmux.h index a331fcdbedd9..837599922a42 100644 --- a/drivers/pinctrl/pinmux.h +++ b/drivers/pinctrl/pinmux.h @@ -19,6 +19,9 @@ int pinmux_validate_map(const struct pinctrl_map *map, int i); int pinmux_request_gpio(struct pinctrl_dev *pctldev, struct pinctrl_gpio_range *range, unsigned pin, unsigned gpio); +int pinmux_request_gpio_consumer(struct pinctrl_dev *pctldev, + struct pinctrl_gpio_range *range, + unsigned pin, unsigned gpio, const char *consumer); void pinmux_free_gpio(struct pinctrl_dev *pctldev, unsigned pin, struct pinctrl_gpio_range *range); int pinmux_gpio_direction(struct pinctrl_dev *pctldev, @@ -50,6 +53,13 @@ static inline int pinmux_request_gpio(struct pinctrl_dev *pctldev, return 0; } +static inline int pinmux_request_gpio_consumer(struct pinctrl_dev *pctldev, + struct pinctrl_gpio_range *range, + unsigned pin, unsigned gpio, const char *consumer) +{ + return 0; +} + static inline void pinmux_free_gpio(struct pinctrl_dev *pctldev, unsigned pin, struct pinctrl_gpio_range *range) diff --git a/include/linux/pinctrl/consumer.h b/include/linux/pinctrl/consumer.h index 0412cc9833e9..8c521a14db43 100644 --- a/include/linux/pinctrl/consumer.h +++ b/include/linux/pinctrl/consumer.h @@ -26,6 +26,7 @@ struct device; /* External interface to pin control */ extern int pinctrl_gpio_request(unsigned gpio); +extern int pinctrl_gpio_request_consumer(unsigned gpio, const char *consumer); extern void pinctrl_gpio_free(unsigned gpio); extern int pinctrl_gpio_direction_input(unsigned gpio); extern int pinctrl_gpio_direction_output(unsigned gpio); @@ -67,6 +68,11 @@ static inline int pinctrl_gpio_request(unsigned gpio) return 0; } +static inline int pinctrl_gpio_request_consumer(unsigned gpio, const char *consumer); +{ + return 0; +} + static inline void pinctrl_gpio_free(unsigned gpio) { } -- 2.12.2 ^ permalink raw reply related [flat|nested] 11+ messages in thread
end of thread, other threads:[~2018-01-18 9:46 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-01-15 16:22 [RFC PATCH 0/2] fixing the gpio ownership Ludovic Desroches 2018-01-15 16:22 ` [RFC PATCH 1/2] pinctrl: add consumer variant for gpio request Ludovic Desroches 2018-01-15 20:19 ` Andy Shevchenko 2018-01-16 9:01 ` Ludovic Desroches 2018-01-16 14:33 ` Andy Shevchenko 2018-01-17 14:54 ` Ludovic Desroches 2018-01-17 16:07 ` Andy Shevchenko 2018-01-18 7:56 ` Ludovic Desroches 2018-01-18 9:46 ` Linus Walleij 2018-01-15 16:22 ` [RFC PATCH 2/2] gpio: provide a consumer when requesting a gpio Ludovic Desroches 2018-01-15 16:24 [RESEND RFC PATCH 0/2] fixing the gpio ownership Ludovic Desroches 2018-01-15 16:24 ` [RFC PATCH 1/2] pinctrl: add consumer variant for gpio request Ludovic Desroches
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).