| Message ID | 20180115162233.6205-2-ludovic.desroches@microchip.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
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
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)
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
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).
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
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
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) { }
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(-)