[RESEND RFC PATCH 0/2] fixing the gpio ownership

[RESEND RFC PATCH 0/2] fixing the gpio ownership

[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

[RFC PATCH 1/2] pinctrl: add consumer variant for gpio request

[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

[RFC PATCH 2/2] gpio: provide a consumer when requesting a gpio

[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

Re: [RESEND RFC PATCH 0/2] fixing the gpio ownership

[Linus Walleij]

Hi Ludovic, thanks for your patches!

On Mon, Jan 15, 2018 at 5:24 PM, Ludovic Desroches
<ludovic.desroches@microchip.com> wrote:

> A few weeks ago, I have sent an RFC about adding bias support for GPIOs [1].

I was confused I think, because the issue of ownership and adding
bias support were conflated.

I think I discussed properly the ideas I have for pin control properties
vs the GPIOlib API/ABI in my response to patch 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.

So that is a different thing from bias support.

> 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.

Okay I think the right solution is to fix the ownership issue, and set
up bias using pin control/config but use the line through gpiolib for now.

> The main issue is that enabling the strict mode will
> break old DTBs.

Yeah we need to work around that.

> 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.

Yep :)

> 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.

fwnode_get_named_gpiod() must really be fixed too. You probably
want to have things like LEDs and GPIO keys working even if
your pin controller is strict.

I don't care so much about the old functions, I guess you just have
to make sure that the drivers for *your* pin controller all use descriptors
so that you can enable strict mode on *your* pin controller, right?

Restrict your task to this, I'd say.

> It seems
> that more and more drivers are converted to use GPIO descriptors so there is
> some hope.

Yeah I'm doing this when I have time. There is plenty of work...
Help appreciated.

Yours,
Linus Walleij

Re: [RFC PATCH 2/2] gpio: provide a consumer when requesting a gpio

[Linus Walleij]

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

Re: [RESEND RFC PATCH 0/2] fixing the gpio ownership

[Ludovic Desroches]

On Thu, Jan 18, 2018 at 11:16:44AM +0100, Linus Walleij wrote:
> Hi Ludovic, thanks for your patches!
> 
> On Mon, Jan 15, 2018 at 5:24 PM, Ludovic Desroches
> <ludovic.desroches@microchip.com> wrote:
> 
> > A few weeks ago, I have sent an RFC about adding bias support for GPIOs [1].
> 
> I was confused I think, because the issue of ownership and adding
> bias support were conflated.
> 

No problem, at the beginning, I only wanted to enable the strict. Doing
this involves that I have to remove pinctrl nodes for the pins which are
going to be request through the gpiolib to avoid conflicts. These pins
were configured with bias-pull-up. That's why I try to add the bias
support.

> I think I discussed properly the ideas I have for pin control properties
> vs the GPIOlib API/ABI in my response to patch 1.
> 

Thanks for the detailed answer about what you have in mind.

> > 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.
> 
> So that is a different thing from bias support.
> 

Well, yes and not! As a consequence of enabling strict mode, I have to
find another way to configure the pins.

> > 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.
> 
> Okay I think the right solution is to fix the ownership issue, and set
> up bias using pin control/config but use the line through gpiolib for now.
> 
> > The main issue is that enabling the strict mode will
> > break old DTBs.
> 
> Yeah we need to work around that.
> 
> > 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.
> 
> Yep :)
> 
> > 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.
> 
> fwnode_get_named_gpiod() must really be fixed too. You probably
> want to have things like LEDs and GPIO keys working even if
> your pin controller is strict.
> 

Yes, I have noticed this issue.

> I don't care so much about the old functions, I guess you just have
> to make sure that the drivers for *your* pin controller all use descriptors
> so that you can enable strict mode on *your* pin controller, right?
> 

Right, I have spotted some drivers to fix.

> Restrict your task to this, I'd say.
> 
> > It seems
> > that more and more drivers are converted to use GPIO descriptors so there is
> > some hope.
> 
> Yeah I'm doing this when I have time. There is plenty of work...
> Help appreciated.
> 

I will try to handle the ones related to the platforms I am using.

Regards

Ludovic

Re: [RFC PATCH 2/2] gpio: provide a consumer when requesting a gpio

[Ludovic Desroches]

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

Re: [RESEND RFC PATCH 0/2] fixing the gpio ownership

[Linus Walleij]

On Thu, Jan 18, 2018 at 4:12 PM, Ludovic Desroches
<ludovic.desroches@microchip.com> wrote:
> On Thu, Jan 18, 2018 at 11:16:44AM +0100, Linus Walleij wrote:

>> > It seems
>> > that more and more drivers are converted to use GPIO descriptors so there is
>> > some hope.
>>
>> Yeah I'm doing this when I have time. There is plenty of work...
>> Help appreciated.
>
> I will try to handle the ones related to the platforms I am using.

I'm looking into SPI and regulators for the next kernel cycle, so
those will hopefully get fixed.

Yours,
Linus Walleij

Re: [RFC PATCH 2/2] gpio: provide a consumer when requesting a gpio

[Ludovic Desroches]

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

Re: [RFC PATCH 2/2] gpio: provide a consumer when requesting a gpio

[Andy Shevchenko]

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.

-- 
With Best Regards,
Andy Shevchenko

Re: [RFC PATCH 2/2] gpio: provide a consumer when requesting a gpio

[Ludovic Desroches]

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

Re: [RFC PATCH 2/2] gpio: provide a consumer when requesting a gpio

[Andy Shevchenko]

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.

-- 
With Best Regards,
Andy Shevchenko

Re: [RFC PATCH 2/2] gpio: provide a consumer when requesting a gpio

[Ludovic Desroches]

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