| Message ID | 20180115162407.6314-3-ludovic.desroches@microchip.com |
|---|---|
| State | New |
| Headers | show |
| Series | fixing the gpio ownership | expand |
On Mon, Jan 15, 2018 at 5:24 PM, Ludovic Desroches <ludovic.desroches@microchip.com> wrote: > 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> I think we need to think over what is a good way to share ownership of a pin. Russell pointed me to a similar problem incidentally and I briefly looked into it: there are cases when several devices may need to hold the same pin. Can't we just look up the associated gpio_chip from the GPIO range, and in case the pin is connected between the pin controller and the GPIO chip, then we allow the gpiochip to also take a reference? I.e. in that case you just allow gpio_owner to proceed and take the pin just like with a non-strict controller. 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 Thu, Jan 18, 2018 at 11:30:00AM +0100, Linus Walleij wrote: > On Mon, Jan 15, 2018 at 5:24 PM, Ludovic Desroches > <ludovic.desroches@microchip.com> wrote: > > > 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> > > I think we need to think over what is a good way to share ownership > of a pin. > > Russell pointed me to a similar problem incidentally and I briefly looked > into it: there are cases when several devices may need to hold the > same pin. > > Can't we just look up the associated gpio_chip from the GPIO range, > and in case the pin is connected between the pin controller and > the GPIO chip, then we allow the gpiochip to also take a > reference? > It's the probably the way to go, it was Maxime's proposal and Andy seems to agree this solution. > I.e. in that case you just allow gpio_owner to proceed and take the > pin just like with a non-strict controller. > > Yours, > Linus Walleij Regards Ludovic -- 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 18, 2018 at 04:22:28PM +0100, Ludovic Desroches wrote: > On Thu, Jan 18, 2018 at 11:30:00AM +0100, Linus Walleij wrote: > > On Mon, Jan 15, 2018 at 5:24 PM, Ludovic Desroches > > <ludovic.desroches@microchip.com> wrote: > > > > > 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> > > > > I think we need to think over what is a good way to share ownership > > of a pin. > > > > Russell pointed me to a similar problem incidentally and I briefly looked > > into it: there are cases when several devices may need to hold the > > same pin. > > > > Can't we just look up the associated gpio_chip from the GPIO range, > > and in case the pin is connected between the pin controller and > > the GPIO chip, then we allow the gpiochip to also take a > > reference? > > > > It's the probably the way to go, it was Maxime's proposal and Andy seems > to agree this solution. > If pin_request() is called with gpio_range not NULL, it means that the requests comes from a GPIO chip and the pin controller handles this pin. In this case, I would say the pin is connected between the pin controller and the GPIO chip. Is my assumption right? I am not sure it will fit all the cases: - case 1: device A requests the pin (pinctrl-default state) and mux it as a GPIO. Later,it requests the pin as a GPIO (gpiolib). This 'weird' situation happens because some strict pin controllers were not declared as strict and/or pinconf is needed. - case 2: device A requests the pin (pinctrl-default state). Device B requests the pin as a GPIO (gpiolib). In case 1, pin_request must not return an error. In case 2, pin_request must return an error even if the pin is connected between the pin controller and the GPIO chip. > > I.e. in that case you just allow gpio_owner to proceed and take the > > pin just like with a non-strict controller. Regards Ludovic -- 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 24, 2018 at 3:07 PM, Ludovic Desroches <ludovic.desroches@microchip.com> wrote: > On Thu, Jan 18, 2018 at 04:22:28PM +0100, Ludovic Desroches wrote: >> On Thu, Jan 18, 2018 at 11:30:00AM +0100, Linus Walleij wrote: >> > On Mon, Jan 15, 2018 at 5:24 PM, Ludovic Desroches >> > <ludovic.desroches@microchip.com> wrote: >> > I think we need to think over what is a good way to share ownership >> > of a pin. >> > >> > Russell pointed me to a similar problem incidentally and I briefly looked >> > into it: there are cases when several devices may need to hold the >> > same pin. >> > >> > Can't we just look up the associated gpio_chip from the GPIO range, >> > and in case the pin is connected between the pin controller and >> > the GPIO chip, then we allow the gpiochip to also take a >> > reference? How do you find my proposal about introducing ownership level (not requested yet; exclusive; shared)? >> It's the probably the way to go, it was Maxime's proposal and Andy seems >> to agree this solution. Confirm with caveat that this is a fix for subset of cases. > If pin_request() is called with gpio_range not NULL, it means that the > requests comes from a GPIO chip and the pin controller handles this pin. > In this case, I would say the pin is connected between the pin > controller and the GPIO chip. Is my assumption right? > > I am not sure it will fit all the cases: I think it doesn't cover cases when you have UART + UART + GPIO (I posted early a use case example). But at least it doesn't move things in a wrong direction. > - case 1: device A requests the pin (pinctrl-default state) and mux it > as a GPIO. Later,it requests the pin as a GPIO (gpiolib). This 'weird' > situation happens because some strict pin controllers were not declared > as strict and/or pinconf is needed. > > - case 2: device A requests the pin (pinctrl-default state). Device B > requests the pin as a GPIO (gpiolib). > > In case 1, pin_request must not return an error. In case 2, pin_request > must return an error even if the pin is connected between the pin > controller and the GPIO chip. For these cases looks OK to me. >> > I.e. in that case you just allow gpio_owner to proceed and take the >> > pin just like with a non-strict controller.
On Wed, Jan 24, 2018 at 05:42:15PM +0200, Andy Shevchenko wrote: > On Wed, Jan 24, 2018 at 3:07 PM, Ludovic Desroches > <ludovic.desroches@microchip.com> wrote: > > On Thu, Jan 18, 2018 at 04:22:28PM +0100, Ludovic Desroches wrote: > >> On Thu, Jan 18, 2018 at 11:30:00AM +0100, Linus Walleij wrote: > >> > On Mon, Jan 15, 2018 at 5:24 PM, Ludovic Desroches > >> > <ludovic.desroches@microchip.com> wrote: > > >> > I think we need to think over what is a good way to share ownership > >> > of a pin. > >> > > >> > Russell pointed me to a similar problem incidentally and I briefly looked > >> > into it: there are cases when several devices may need to hold the > >> > same pin. > >> > > >> > Can't we just look up the associated gpio_chip from the GPIO range, > >> > and in case the pin is connected between the pin controller and > >> > the GPIO chip, then we allow the gpiochip to also take a > >> > reference? > > How do you find my proposal about introducing ownership level (not > requested yet; exclusive; shared)? > Yes but I don't see how I can fix my issue with these levels. In my case, I need an exclusive ownership at device level not at pin level. In reality, it is at pin level but I am in this situation because my pin controler was introduced as non strict and also because I need to set the configuration of the pin which is going to be used as a GPIO. If the ownership is exclusive, pinmuxing coming from pinctrl-default will be accepted but the GPIO request will fail even if it comes from the same device. If the ownership is shared then, pinmuxing coming from pinctrl-default will be accepted but a GPIO request from another device will be accepted too. Both situations are incorrect in my case. Let me know if I have not well understood your proposal. My concern is to get out of this situation without breaking current DTs. Regards Ludovic > >> It's the probably the way to go, it was Maxime's proposal and Andy seems > >> to agree this solution. > > Confirm with caveat that this is a fix for subset of cases. > > > If pin_request() is called with gpio_range not NULL, it means that the > > requests comes from a GPIO chip and the pin controller handles this pin. > > In this case, I would say the pin is connected between the pin > > controller and the GPIO chip. Is my assumption right? > > > > I am not sure it will fit all the cases: > > I think it doesn't cover cases when you have UART + UART + GPIO (I > posted early a use case example). > > But at least it doesn't move things in a wrong direction. > > > - case 1: device A requests the pin (pinctrl-default state) and mux it > > as a GPIO. Later,it requests the pin as a GPIO (gpiolib). This 'weird' > > situation happens because some strict pin controllers were not declared > > as strict and/or pinconf is needed. > > > > - case 2: device A requests the pin (pinctrl-default state). Device B > > requests the pin as a GPIO (gpiolib). > > > > In case 1, pin_request must not return an error. In case 2, pin_request > > must return an error even if the pin is connected between the pin > > controller and the GPIO chip. > > For these cases looks OK to me. > > >> > I.e. in that case you just allow gpio_owner to proceed and take the > >> > pin just like with a non-strict controller. > > -- > 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 -- 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 26, 2018 at 9:32 AM, Ludovic Desroches <ludovic.desroches@microchip.com> wrote: > On Wed, Jan 24, 2018 at 05:42:15PM +0200, Andy Shevchenko wrote: >> On Wed, Jan 24, 2018 at 3:07 PM, Ludovic Desroches >> <ludovic.desroches@microchip.com> wrote: >> > On Thu, Jan 18, 2018 at 04:22:28PM +0100, Ludovic Desroches wrote: >> >> On Thu, Jan 18, 2018 at 11:30:00AM +0100, Linus Walleij wrote: >> >> > Can't we just look up the associated gpio_chip from the GPIO range, >> >> > and in case the pin is connected between the pin controller and >> >> > the GPIO chip, then we allow the gpiochip to also take a >> >> > reference? >> >> How do you find my proposal about introducing ownership level (not >> requested yet; exclusive; shared)? > Yes but I don't see how I can fix my issue with these levels. In my > case, I need an exclusive ownership at device level not at pin level. In > reality, it is at pin level but I am in this situation because my pin > controler was introduced as non strict and also because I need to set > the configuration of the pin which is going to be used as a GPIO. > > If the ownership is exclusive, pinmuxing coming from pinctrl-default > will be accepted but the GPIO request will fail even if it comes from the > same device. The problem here is to declare a right consumer of the resource. My understanding that consumer at the end is device or device(s): none: resource is free to acquire exclusive: certain device has access to the resource (pin) shared: several devices may access to the resource In both cases couple of caveats: - power management has a special access level to the resource on behalf of the owner(s) - it can have some flags, like 'locked', which means no more owners can be changed / added, but still possible to free resource by all owners to go to state 'none' > If the ownership is shared then, pinmuxing coming from pinctrl-default > will be accepted but a GPIO request from another device will be accepted > too. > > Both situations are incorrect in my case. Yes, since the ownership design is based on subsystem rather consumer device. > Let me know if I have not well understood your proposal. My concern is > to get out of this situation without breaking current DTs. See above, hope it clarifies a bit.
On Fri, Jan 26, 2018 at 07:13:32PM +0200, Andy Shevchenko wrote: > On Fri, Jan 26, 2018 at 9:32 AM, Ludovic Desroches > <ludovic.desroches@microchip.com> wrote: > > On Wed, Jan 24, 2018 at 05:42:15PM +0200, Andy Shevchenko wrote: > >> On Wed, Jan 24, 2018 at 3:07 PM, Ludovic Desroches > >> <ludovic.desroches@microchip.com> wrote: > >> > On Thu, Jan 18, 2018 at 04:22:28PM +0100, Ludovic Desroches wrote: > >> >> On Thu, Jan 18, 2018 at 11:30:00AM +0100, Linus Walleij wrote: > > >> >> > Can't we just look up the associated gpio_chip from the GPIO range, > >> >> > and in case the pin is connected between the pin controller and > >> >> > the GPIO chip, then we allow the gpiochip to also take a > >> >> > reference? > >> > >> How do you find my proposal about introducing ownership level (not > >> requested yet; exclusive; shared)? > > > Yes but I don't see how I can fix my issue with these levels. In my > > case, I need an exclusive ownership at device level not at pin level. In > > reality, it is at pin level but I am in this situation because my pin > > controler was introduced as non strict and also because I need to set > > the configuration of the pin which is going to be used as a GPIO. > > > > If the ownership is exclusive, pinmuxing coming from pinctrl-default > > will be accepted but the GPIO request will fail even if it comes from the > > same device. > > The problem here is to declare a right consumer of the resource. > > My understanding that consumer at the end is device or device(s): > > none: resource is free to acquire > exclusive: certain device has access to the resource (pin) > shared: several devices may access to the resource > > In both cases couple of caveats: > - power management has a special access level to the resource on > behalf of the owner(s) > - it can have some flags, like 'locked', which means no more owners > can be changed / added, but still possible to free resource by all > owners to go to state 'none' > > > If the ownership is shared then, pinmuxing coming from pinctrl-default > > will be accepted but a GPIO request from another device will be accepted > > too. > > > > Both situations are incorrect in my case. > > Yes, since the ownership design is based on subsystem rather consumer device. > > > Let me know if I have not well understood your proposal. My concern is > > to get out of this situation without breaking current DTs. > > See above, hope it clarifies a bit. Yes I get it but I still don't see how I can use your approach to solve my issue. We have a situation for several pin controllers. If I can't know who is requesting the GPIO, I have no idea about how to solve this issue. Bypassing the strict mode, as suggested, if the pin controller is also a gpio controller may lead, IMO, to wrong behaviors. Do I have to try to find a way to fix this situation? Maybe, it will be easier to progress on the muxing and configuration topic and to introduce a DT property to enable the strict mode or wathever modes you want once everything is ready and DTs fixed. I'd prefer to fix the current situation then to improve muxing and configuration stuff because it will take time. Regards Ludovic -- 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/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);
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(-)