[v2] pinctrl: msm: fix gpio-hog related boot issues

Message ID 20180402121005.10080-1-chunkeey@gmail.com
State New
Headers show
Series
  • [v2] pinctrl: msm: fix gpio-hog related boot issues
Related show

Commit Message

Christian Lamparter April 2, 2018, 12:10 p.m.
Sven Eckelmann reported an issue with the current IPQ4019 pinctrl.
Setting up any gpio-hog in the device-tree for his device would
"kill the bootup completely":

| [    0.477838] msm_serial 78af000.serial: could not find pctldev for node /soc/pinctrl@1000000/serial_pinmux, deferring probe
| [    0.499828] spi_qup 78b5000.spi: could not find pctldev for node /soc/pinctrl@1000000/spi_0_pinmux, deferring probe
| [    1.298883] requesting hog GPIO enable USB2 power (chip 1000000.pinctrl, offset 58) failed, -517
| [    1.299609] gpiochip_add_data: GPIOs 0..99 (1000000.pinctrl) failed to register
| [    1.308589] ipq4019-pinctrl 1000000.pinctrl: Failed register gpiochip
| [    1.316586] msm_serial 78af000.serial: could not find pctldev for node /soc/pinctrl@1000000/serial_pinmux, deferring probe
| [    1.322415] spi_qup 78b5000.spi: could not find pctldev for node /soc/pinctrl@1000000/spi_0_pinmux, deferri

This was also verified on a RT-AC58U (IPQ4018) which would
no longer boot, if a gpio-hog was specified. (Tried forcing
the USB LED PIN (GPIO0) to high.).

The problem is that Pinctrl+GPIO registration is currently
peformed in the following order in pinctrl-msm.c:
	1. pinctrl_register()
	2. gpiochip_add()
	3. gpiochip_add_pin_range()

The actual error code -517 == -EPROBE_DEFER is coming from
pinctrl_get_device_gpio_range(), which is called through:
        gpiochip_add
            of_gpiochip_add
                of_gpiochip_scan_gpios
                    gpiod_hog
                        gpiochip_request_own_desc
                            __gpiod_request
                                chip->request
                                    gpiochip_generic_request
                                       pinctrl_gpio_request
                                          pinctrl_get_device_gpio_range

pinctrl_get_device_gpio_range() is unable to find any valid
pin ranges, since nothing has been added to the pinctrldev_list yet.
so the range can't be found, and the operation fails with -EPROBE_DEFER.

This patch fixes the issue by adding the "gpio-ranges" property
to the pinctrl device node of all upstream Qcom SoC, so the
ranges are added by of_gpiochip_add_pin_range(), which is
called by of_gpiochip_add() before the call to
of_gpiochip_scan_gpios() happens.gpiochip_add_pin_range() is longer
needed and removed (to prevent adding the same entry to the
pinctrldev_list twice).

Cc: stable@vger.kernel.org
Reported-by: Sven Eckelmann <sven.eckelmann@openmesh.com>
Signed-off-by: Christian Lamparter <chunkeey@gmail.com>
---
The msm8998 would need something like
	gpio-ranges = <&tlmm 0 0 150>;

v1->v2: restore gpiochip_add_pin_range() for ACPI compatibility
---
 arch/arm/boot/dts/qcom-apq8064.dtsi   |  1 +
 arch/arm/boot/dts/qcom-apq8084.dtsi   |  1 +
 arch/arm/boot/dts/qcom-ipq4019.dtsi   |  1 +
 arch/arm/boot/dts/qcom-ipq8064.dtsi   |  1 +
 arch/arm/boot/dts/qcom-mdm9615.dtsi   |  1 +
 arch/arm/boot/dts/qcom-msm8660.dtsi   |  1 +
 arch/arm/boot/dts/qcom-msm8960.dtsi   |  1 +
 arch/arm/boot/dts/qcom-msm8974.dtsi   |  1 +
 arch/arm64/boot/dts/qcom/ipq8074.dtsi |  3 ++-
 arch/arm64/boot/dts/qcom/msm8916.dtsi |  1 +
 arch/arm64/boot/dts/qcom/msm8992.dtsi |  1 +
 arch/arm64/boot/dts/qcom/msm8994.dtsi |  1 +
 arch/arm64/boot/dts/qcom/msm8996.dtsi |  1 +
 drivers/pinctrl/qcom/pinctrl-msm.c    | 21 ++++++++++++++++-----
 14 files changed, 30 insertions(+), 6 deletions(-)

Comments

Bjorn Andersson April 2, 2018, 3:04 p.m. | #1
On Mon 02 Apr 05:10 PDT 2018, Christian Lamparter wrote:

[..]
> diff --git a/arch/arm64/boot/dts/qcom/msm8996.dtsi b/arch/arm64/boot/dts/qcom/msm8996.dtsi
> index 0a6f7952bbb1..18511e782cbd 100644
> --- a/arch/arm64/boot/dts/qcom/msm8996.dtsi
> +++ b/arch/arm64/boot/dts/qcom/msm8996.dtsi
> @@ -530,6 +530,7 @@
>  			reg = <0x01010000 0x300000>;
>  			interrupts = <GIC_SPI 208 IRQ_TYPE_LEVEL_HIGH>;
>  			gpio-controller;
> +			gpio-ranges = <&msmgpio 0 0 150>;
>  			#gpio-cells = <2>;
>  			interrupt-controller;
>  			#interrupt-cells = <2>;
> diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
> index 495432f3341b..258fa357d946 100644
> --- a/drivers/pinctrl/qcom/pinctrl-msm.c
> +++ b/drivers/pinctrl/qcom/pinctrl-msm.c
> @@ -831,11 +831,22 @@ static int msm_gpio_init(struct msm_pinctrl *pctrl)
>  		return ret;
>  	}
>  
> -	ret = gpiochip_add_pin_range(&pctrl->chip, dev_name(pctrl->dev), 0, 0, chip->ngpio);
> -	if (ret) {
> -		dev_err(pctrl->dev, "Failed to add pin range\n");
> -		gpiochip_remove(&pctrl->chip);
> -		return ret;
> +	if (!is_of_node(pctrl->dev->fwnode)) {

Afaict this still means that if I boot this kernel with yesterday's dtb
(without gpio-ranges) I will not get any gpios. This isn't okay. 

@Linus, I count 24 callers of gpiochip_add_pin_range(). Is this
suggestion reasonable?

Can we make gpiochip_add_pin_range() check if there's already a
gpio-range and return ok in some way?

> +		/*
> +		 * gpiochip_add_pin_range() is meant for platforms that
> +		 * don't support DT. All DT platforms should just add
> +		 * the gpio-ranges property to the pinctrl device node.
> +		 *
> +		 * See Documentation/devicetree/bindings/gpio/gpio.txt .
> +		 */
> +		ret = gpiochip_add_pin_range(&pctrl->chip,
> +			dev_name(pctrl->dev), 0, 0, chip->ngpio);
> +
> +		if (ret) {
> +			dev_err(pctrl->dev, "Failed to add pin range\n");
> +			gpiochip_remove(&pctrl->chip);
> +			return ret;
> +		}

Regards,
Bjorn
--
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
Christian Lamparter April 5, 2018, 4:35 p.m. | #2
On Montag, 2. April 2018 17:04:47 CEST Bjorn Andersson wrote:
> On Mon 02 Apr 05:10 PDT 2018, Christian Lamparter wrote:
> > diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
> > index 495432f3341b..258fa357d946 100644
> > --- a/drivers/pinctrl/qcom/pinctrl-msm.c
> > +++ b/drivers/pinctrl/qcom/pinctrl-msm.c
> > @@ -831,11 +831,22 @@ static int msm_gpio_init(struct msm_pinctrl *pctrl)
> >  		return ret;
> >  	}
> >  
> > -	ret = gpiochip_add_pin_range(&pctrl->chip, dev_name(pctrl->dev), 0, 0, chip->ngpio);
> > -	if (ret) {
> > -		dev_err(pctrl->dev, "Failed to add pin range\n");
> > -		gpiochip_remove(&pctrl->chip);
> > -		return ret;
> > +	if (!is_of_node(pctrl->dev->fwnode)) {
> 
> Afaict this still means that if I boot this kernel with yesterday's dtb
> (without gpio-ranges) I will not get any gpios. This isn't okay. 
Ok, fair point. I drop this chunk.
 
> @Linus, I count 24 callers of gpiochip_add_pin_range(). Is this
> suggestion reasonable?
> 
> Can we make gpiochip_add_pin_range() check if there's already a
> gpio-range and return ok in some way?

Looks like Linus is currently really busy updating the gemini
target (a lot of work went into it) for OpenWrt.
<https://lists.openwrt.org/pipermail/openwrt-devel/2018-April/043752.html>
(Kinda funny, because I do help to maintain the apm821xx and the new 
ipq40xx target over there.)

In any case, I implemented your suggestion and it does look reasonable.
The gpiolib already uses the gpio_offset as an ID of some sorts. For
now I went with a simple ID value check, but this could be extended to
a range/intersection check if necessary. But then again, let's not
overengineer it. Comments are welcome, I'll wait around till sometime
next week before I post v3.

Regards,
Christian
---
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index d66de67ef307..21c0f88e1159 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -2013,6 +2013,19 @@ int gpiochip_generic_config(struct gpio_chip *chip, unsigned offset,
 }
 EXPORT_SYMBOL_GPL(gpiochip_generic_config);
 
+static struct gpio_pin_range *gpiochip_find_by_id(struct gpio_chip *chip,
+						  unsigned int id)
+{
+	struct gpio_pin_range *pin_range;
+	struct gpio_device *gdev = chip->gpiodev;
+
+	list_for_each_entry(pin_range, &gdev->pin_ranges, node) {
+		if (pin_range->range.id == id)
+			return pin_range;
+	}
+	return NULL;
+}
+
 #ifdef CONFIG_PINCTRL
 
 /**
@@ -2030,6 +2043,20 @@ int gpiochip_add_pingroup_range(struct gpio_chip *chip,
 	struct gpio_device *gdev = chip->gpiodev;
 	int ret;
 
+	/*
+	 * look if a GPIO range with the same ID has already been registered.
+	 * For pinctrls that are set up through devicetree, the GPIO Range
+	 * might be already set by the the gpio-ranges property.
+	 * (see Documentation/devicetree/bindings/gpio/gpio.txt)
+	 */
+	pin_range = gpiochip_find_by_id(chip, gpio_offset);
+	if (pin_range) {
+		chip_dbg(chip, "found existing GPIO range %d->%d - skipping\n",
+				gpio_offset,
+				gpio_offset + pin_range->range.npins - 1);
+		return 0;
+	}
+
 	pin_range = kzalloc(sizeof(*pin_range), GFP_KERNEL);
 	if (!pin_range) {
 		chip_err(chip, "failed to allocate pin ranges\n");
@@ -2083,6 +2110,20 @@ int gpiochip_add_pin_range(struct gpio_chip *chip, const char *pinctl_name,
 	struct gpio_device *gdev = chip->gpiodev;
 	int ret;
 
+	/*
+	 * look if a GPIO range with the same ID has already been registered.
+	 * For pinctrls that are set up through devicetree, the GPIO Range
+	 * might be already set by the the gpio-ranges property.
+	 * (see Documentation/devicetree/bindings/gpio/gpio.txt)
+	 */
+	pin_range = gpiochip_find_by_id(chip, gpio_offset);
+	if (pin_range) {
+		chip_dbg(chip, "found existing GPIO range %d->%d - skipping\n",
+			 gpio_offset,
+			 gpio_offset + pin_range->range.npins - 1);
+		return 0;
+	}
+
 	pin_range = kzalloc(sizeof(*pin_range), GFP_KERNEL);
 	if (!pin_range) {
 		chip_err(chip, "failed to allocate pin ranges\n");
---


--
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
Linus Walleij April 26, 2018, 9:12 a.m. | #3
I see I might have missed this mail, sorry for that.

On Thu, Apr 5, 2018 at 6:35 PM, Christian Lamparter <chunkeey@gmail.com> wrote:
> On Montag, 2. April 2018 17:04:47 CEST Bjorn Andersson wrote:
>> On Mon 02 Apr 05:10 PDT 2018, Christian Lamparter wrote:
>> > diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
>> > index 495432f3341b..258fa357d946 100644
>> > --- a/drivers/pinctrl/qcom/pinctrl-msm.c
>> > +++ b/drivers/pinctrl/qcom/pinctrl-msm.c
>> > @@ -831,11 +831,22 @@ static int msm_gpio_init(struct msm_pinctrl *pctrl)
>> >             return ret;
>> >     }
>> >
>> > -   ret = gpiochip_add_pin_range(&pctrl->chip, dev_name(pctrl->dev), 0, 0, chip->ngpio);
>> > -   if (ret) {
>> > -           dev_err(pctrl->dev, "Failed to add pin range\n");
>> > -           gpiochip_remove(&pctrl->chip);
>> > -           return ret;
>> > +   if (!is_of_node(pctrl->dev->fwnode)) {
>>
>> Afaict this still means that if I boot this kernel with yesterday's dtb
>> (without gpio-ranges) I will not get any gpios. This isn't okay.
> Ok, fair point. I drop this chunk.
>
>> @Linus, I count 24 callers of gpiochip_add_pin_range(). Is this
>> suggestion reasonable?
>>
>> Can we make gpiochip_add_pin_range() check if there's already a
>> gpio-range and return ok in some way?

I think I replied in some other mail that I think we need to
be backwards compatible and it's not too hard to do
both. (Correct me if I'm wrong.)

> Looks like Linus is currently really busy updating the gemini
> target (a lot of work went into it) for OpenWrt.
> <https://lists.openwrt.org/pipermail/openwrt-devel/2018-April/043752.html>
> (Kinda funny, because I do help to maintain the apm821xx and the new
> ipq40xx target over there.)

Yeah it was more of a hobby, partly research for a lecture
on maintaining old ARM systems and all dangerous aftermarket
devices that is littering the world.

Since I have noticed that the adoption of OpenWRT/LEDE goes
far beyond routers and it is one of the most prevalent
distributions that is existing in the IoT of darkness on the
planet being the most deployed OS for bitcoin mining and
being picked up for functional safety (such as automotive)
because of its minimalist userspace.

I'm still amazed by the weirdness of our business.

> In any case, I implemented your suggestion and it does look reasonable.
> The gpiolib already uses the gpio_offset as an ID of some sorts. For
> now I went with a simple ID value check, but this could be extended to
> a range/intersection check if necessary. But then again, let's not
> overengineer it. Comments are welcome, I'll wait around till sometime
> next week before I post v3.

I'm going through my mailbox now, sorry for delays in feedback
and review :(

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
Christian Lamparter April 26, 2018, 9:47 p.m. | #4
On Thu, Apr 26, 2018 11:12:21 CEST Linus Walleij wrote:
> On Thu, Apr 5, 2018 at 6:35 PM, Christian Lamparter wrote:
> > On Montag, 2. April 2018 17:04:47 CEST Bjorn Andersson wrote:
> >> @Linus, I count 24 callers of gpiochip_add_pin_range(). Is this
> >> suggestion reasonable?
> >>
> >> Can we make gpiochip_add_pin_range() check if there's already a
> >> gpio-range and return ok in some way?
> 
> I think I replied in some other mail that I think we need to
> be backwards compatible and it's not too hard to do
> both. (Correct me if I'm wrong.)
I think so too, I looked around and found that the nvidia pinctrl was
doing something similar with of_find_property():
<https://elixir.bootlin.com/linux/v4.17-rc2/source/drivers/pinctrl/tegra/pinctrl-tegra.c#L652>
|	has_prop = of_find_property(np, "gpio-ranges", NULL);

However this looks kinda funny, since "has_prob" is declared as a bool
and of_find_property() returns a pointer to a "struct property"....
Tell you what: If nobody beats me to it, I'll sent a patch for this after
the pinctrl-msm's gpio-hog has been dealt with. :)

> > Looks like Linus is currently really busy updating the gemini
> > target (a lot of work went into it) for OpenWrt.
> > <https://lists.openwrt.org/pipermail/openwrt-devel/2018-April/043752.html>
> > (Kinda funny, because I do help to maintain the apm821xx and the new
> > ipq40xx target over there.)
> 
> Yeah it was more of a hobby, partly research for a lecture
> on maintaining old ARM systems and all dangerous aftermarket
> devices that is littering the world.
> 
> Since I have noticed that the adoption of OpenWRT/LEDE goes
> far beyond routers and it is one of the most prevalent
> distributions that is existing in the IoT of darkness on the
> planet being the most deployed OS for bitcoin mining and
> being picked up for functional safety (such as automotive)
> because of its minimalist userspace.
> 
> I'm still amazed by the weirdness of our business.
I think I can add something to this thought as well. Albeit, it's a
more traditional, less zany story.

I started with the apm821xx (a PowerPC 464 derivative SoC) back
in 2015-2016 because of the wide availabilty of cheap (as low as $5 ~ $10)
secondhand enterprise accesspoints like the Cisco Meraki MR24
<https://openwrt.org/toh/meraki/mr24>. 

What happen was that cooperations ditched all the outdated 802.11n
devices and upgraded to the shiny new wave1 802.11ac gear. Now, the
old 802.11n devices still worked perfectly fine but the firmware on
the device requires the user to have a valid subscription to a 
"Meraki Cloud License".

And as you can probably guess: The License is sold as a yearly subscription
starting from $150 for a single device. So, these devices became practically
e-waste since nobody in their right mind would buy a used device and then
fork over the ~$150 fee per annum.

The MR24 craze is mostly over by now. You can still find a few. However
some listings are now selling them with OpenWrt/LEDE for ~$40.

And obviously, this cycle will continue on, but now with the old wave1
802.11ac gear that gets replaced. In fact this business has spawned
companies that are actively working on supporting "old" enterprise gear
via their own OpenWrt/LEDE derivatives.

> > In any case, I implemented your suggestion and it does look reasonable.
> > The gpiolib already uses the gpio_offset as an ID of some sorts. For
> > now I went with a simple ID value check, but this could be extended to
> > a range/intersection check if necessary. But then again, let's not
> > overengineer it. Comments are welcome, I'll wait around till sometime
> > next week before I post v3.
> 
> I'm going through my mailbox now, sorry for delays in feedback
> and review :(
I see you skipped v3 and replied to v4.

Best Regards,
Christian Lamparter


--
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
Linus Walleij May 2, 2018, 12:14 p.m. | #5
On Thu, Apr 26, 2018 at 11:47 PM, Christian Lamparter
<chunkeey@gmail.com> wrote:
> On Thu, Apr 26, 2018 11:12:21 CEST Linus Walleij wrote:

>> I think I replied in some other mail that I think we need to
>> be backwards compatible and it's not too hard to do
>> both. (Correct me if I'm wrong.)
>
> I think so too, I looked around and found that the nvidia pinctrl was
> doing something similar with of_find_property():
> <https://elixir.bootlin.com/linux/v4.17-rc2/source/drivers/pinctrl/tegra/pinctrl-tegra.c#L652>
> |       has_prop = of_find_property(np, "gpio-ranges", NULL);
>
> However this looks kinda funny, since "has_prob" is declared as a bool
> and of_find_property() returns a pointer to a "struct property"....
> Tell you what: If nobody beats me to it, I'll sent a patch for this after
> the pinctrl-msm's gpio-hog has been dealt with. :)

Yeah the nVidia driver is one of the oldest and also at the time
DT was kind of new. I haven't heard from Stephen for a while
but I bet he will pop up, else check with Laxman, he's got
a good grip on nVidia pinctrl+GPIO as well.

> And as you can probably guess: The License is sold as a yearly subscription
> starting from $150 for a single device. So, these devices became practically
> e-waste since nobody in their right mind would buy a used device and then
> fork over the ~$150 fee per annum.
>
> The MR24 craze is mostly over by now. You can still find a few. However
> some listings are now selling them with OpenWrt/LEDE for ~$40.
>
> And obviously, this cycle will continue on, but now with the old wave1
> 802.11ac gear that gets replaced. In fact this business has spawned
> companies that are actively working on supporting "old" enterprise gear
> via their own OpenWrt/LEDE derivatives.

Haha that is just awesome :D
I hope they salvage a lot of them.

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
Christian Lamparter May 3, 2018, 5:43 p.m. | #6
On Mittwoch, 2. Mai 2018 14:14:39 CEST Linus Walleij wrote:
> On Thu, Apr 26, 2018 at 11:47 PM, Christian Lamparter
> <chunkeey@gmail.com> wrote:
> > On Thu, Apr 26, 2018 11:12:21 CEST Linus Walleij wrote:
> 
> >> I think I replied in some other mail that I think we need to
> >> be backwards compatible and it's not too hard to do
> >> both. (Correct me if I'm wrong.)
> >
> > I think so too, I looked around and found that the nvidia pinctrl was
> > doing something similar with of_find_property():
> > <https://elixir.bootlin.com/linux/v4.17-rc2/source/drivers/pinctrl/tegra/pinctrl-tegra.c#L652>
> > |       has_prop = of_find_property(np, "gpio-ranges", NULL);
> >
> > However this looks kinda funny, since "has_prob" is declared as a bool
> > and of_find_property() returns a pointer to a "struct property"....
> > Tell you what: If nobody beats me to it, I'll sent a patch for this after
> > the pinctrl-msm's gpio-hog has been dealt with. :)
> 
> Yeah the nVidia driver is one of the oldest and also at the time
> DT was kind of new. I haven't heard from Stephen for a while
> but I bet he will pop up, else check with Laxman, he's got
> a good grip on nVidia pinctrl+GPIO as well.
All in good time. 

But first @Bjorn and @Andy or @David can you please look and
review v4 "pinctrl: msm: fix gpio-hog related boot issues"
<https://patchwork.kernel.org/patch/10339129/>

Thanks.

> > The MR24 craze is mostly over by now. You can still find a few. However
> > some listings are now selling them with OpenWrt/LEDE for ~$40.
> >
> > And obviously, this cycle will continue on, but now with the old wave1
> > 802.11ac gear that gets replaced. In fact this business has spawned
> > companies that are actively working on supporting "old" enterprise gear
> > via their own OpenWrt/LEDE derivatives.
> 
> Haha that is just awesome :D
> I hope they salvage a lot of them.
Yes, I'm aware of that some of them where put to good use in the
Personal Telco Projec (501(c)(3) non-profit organization in Portland, Oregon):
<https://personaltelco.net/wiki/PersonalTelco>
And they have been somewhat vocal about it too:
<https://groups.google.com/forum/#!msg/ptp-general/RZ-VjKFonVo/cQgnGn2wAgAJ>
"The MR24 is a dual-band 802.11n 3x3 MIMO access point with a single 
ethernet port.  They are "last-gen" devices, and are starting to show up 
on ebay at reasonable prices as Meraki's deep-pocket cloud-management 
enterprise users are beginning to "upgrade" to 802.11ac gear.  Note that 
802.11n in this case means a fully-open-source driver (ath9k).  802.11ac 
drivers involve firmware blobs across the board.  You don't get 
super-wide 5GHz channels, but you get freedom. "

;)

By the way, it gets even weirder. In the past (and to this day) Meraki has
given away their current crop of enterprise APs via their
"Free AP for IT Professionals" <https://meraki.cisco.com/tc/freeap> program.
Of course, the main idea probably was to get them all "hooked/sold" on their
cloud-management firmware. Because of course that "Free AP" is intended to be
only good for the lifespan of included the 3-year license. However, 
"IT Professionals" do have their own mind and that's why there is some
continued interest in making alternative firmwares for these devices.

Best Regards,
Christian


--
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
Laxman Dewangan May 4, 2018, 9:04 a.m. | #7
On Thursday 03 May 2018 11:13 PM, Christian Lamparter wrote:
> On Mittwoch, 2. Mai 2018 14:14:39 CEST Linus Walleij wrote:
>> On Thu, Apr 26, 2018 at 11:47 PM, Christian Lamparter
>> <chunkeey@gmail.com> wrote:
>>> On Thu, Apr 26, 2018 11:12:21 CEST Linus Walleij wrote:
>>>> I think I replied in some other mail that I think we need to
>>>> be backwards compatible and it's not too hard to do
>>>> both. (Correct me if I'm wrong.)
>>> I think so too, I looked around and found that the nvidia pinctrl was
>>> doing something similar with of_find_property():
>>> <https://elixir.bootlin.com/linux/v4.17-rc2/source/drivers/pinctrl/tegra/pinctrl-tegra.c#L652>
>>> |       has_prop = of_find_property(np, "gpio-ranges", NULL);
>>>
>>> However this looks kinda funny, since "has_prob" is declared as a bool
>>> and of_find_property() returns a pointer to a "struct property"....
>>> Tell you what: If nobody beats me to it, I'll sent a patch for this after
>>> the pinctrl-msm's gpio-hog has been dealt with. :)
>> Yeah the nVidia driver is one of the oldest and also at the time
>> DT was kind of new. I haven't heard from Stephen for a while
>> but I bet he will pop up, else check with Laxman, he's got
>> a good grip on nVidia pinctrl+GPIO as well.
> All in good time.
>
> But first @Bjorn and @Andy or @David can you please look and
> review v4 "pinctrl: msm: fix gpio-hog related boot issues"
> <https://patchwork.kernel.org/patch/10339129/>

Agree with the patch as by adding gpio-ranges, we add the gpio range 
part of gpiochip_add(), called by 
of_gpiochip_add()->of_gpiochip_add_pin_range(). and so does not need to 
explicitly call of the gpiochip_add_pin_range().

Please add ack in above patch.

Acked-by: Laxman Dewangan <ldewangan@nvidia.com>

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

Patch

diff --git a/arch/arm/boot/dts/qcom-apq8064.dtsi b/arch/arm/boot/dts/qcom-apq8064.dtsi
index 3ca96e361878..17ad9cbd9f8c 100644
--- a/arch/arm/boot/dts/qcom-apq8064.dtsi
+++ b/arch/arm/boot/dts/qcom-apq8064.dtsi
@@ -327,6 +327,7 @@ 
 			reg = <0x800000 0x4000>;
 
 			gpio-controller;
+			gpio-ranges = <&tlmm_pinmux 0 0 90>;
 			#gpio-cells = <2>;
 			interrupt-controller;
 			#interrupt-cells = <2>;
diff --git a/arch/arm/boot/dts/qcom-apq8084.dtsi b/arch/arm/boot/dts/qcom-apq8084.dtsi
index 0e1e98707e3f..d9481d083802 100644
--- a/arch/arm/boot/dts/qcom-apq8084.dtsi
+++ b/arch/arm/boot/dts/qcom-apq8084.dtsi
@@ -396,6 +396,7 @@ 
 			compatible = "qcom,apq8084-pinctrl";
 			reg = <0xfd510000 0x4000>;
 			gpio-controller;
+			gpio-ranges = <&tlmm 0 0 147>;
 			#gpio-cells = <2>;
 			interrupt-controller;
 			#interrupt-cells = <2>;
diff --git a/arch/arm/boot/dts/qcom-ipq4019.dtsi b/arch/arm/boot/dts/qcom-ipq4019.dtsi
index c1e65aaf3cad..d85b15774c64 100644
--- a/arch/arm/boot/dts/qcom-ipq4019.dtsi
+++ b/arch/arm/boot/dts/qcom-ipq4019.dtsi
@@ -147,6 +147,7 @@ 
 			compatible = "qcom,ipq4019-pinctrl";
 			reg = <0x01000000 0x300000>;
 			gpio-controller;
+			gpio-ranges = <&tlmm 0 0 100>;
 			#gpio-cells = <2>;
 			interrupt-controller;
 			#interrupt-cells = <2>;
diff --git a/arch/arm/boot/dts/qcom-ipq8064.dtsi b/arch/arm/boot/dts/qcom-ipq8064.dtsi
index 1e0a3b446f7a..26eab9a68d90 100644
--- a/arch/arm/boot/dts/qcom-ipq8064.dtsi
+++ b/arch/arm/boot/dts/qcom-ipq8064.dtsi
@@ -108,6 +108,7 @@ 
 			reg = <0x800000 0x4000>;
 
 			gpio-controller;
+			gpio-ranges = <&qcom_pinmux 0 0 69>;
 			#gpio-cells = <2>;
 			interrupt-controller;
 			#interrupt-cells = <2>;
diff --git a/arch/arm/boot/dts/qcom-mdm9615.dtsi b/arch/arm/boot/dts/qcom-mdm9615.dtsi
index c852b69229c9..cfdaca5f259a 100644
--- a/arch/arm/boot/dts/qcom-mdm9615.dtsi
+++ b/arch/arm/boot/dts/qcom-mdm9615.dtsi
@@ -128,6 +128,7 @@ 
 		msmgpio: pinctrl@800000 {
 			compatible = "qcom,mdm9615-pinctrl";
 			gpio-controller;
+			gpio-ranges = <&msmgpio 0 0 88>;
 			#gpio-cells = <2>;
 			interrupts = <GIC_SPI 16 IRQ_TYPE_LEVEL_HIGH>;
 			interrupt-controller;
diff --git a/arch/arm/boot/dts/qcom-msm8660.dtsi b/arch/arm/boot/dts/qcom-msm8660.dtsi
index 33030f9419fe..47cf9c4ca062 100644
--- a/arch/arm/boot/dts/qcom-msm8660.dtsi
+++ b/arch/arm/boot/dts/qcom-msm8660.dtsi
@@ -110,6 +110,7 @@ 
 			reg = <0x800000 0x4000>;
 
 			gpio-controller;
+			gpio-ranges = <&tlmm 0 0 173>;
 			#gpio-cells = <2>;
 			interrupts = <0 16 0x4>;
 			interrupt-controller;
diff --git a/arch/arm/boot/dts/qcom-msm8960.dtsi b/arch/arm/boot/dts/qcom-msm8960.dtsi
index 1733d8f40ab1..f6d8b1af5a8a 100644
--- a/arch/arm/boot/dts/qcom-msm8960.dtsi
+++ b/arch/arm/boot/dts/qcom-msm8960.dtsi
@@ -102,6 +102,7 @@ 
 		msmgpio: pinctrl@800000 {
 			compatible = "qcom,msm8960-pinctrl";
 			gpio-controller;
+			gpio-ranges = <&msmgpio 0 0 152>;
 			#gpio-cells = <2>;
 			interrupts = <0 16 0x4>;
 			interrupt-controller;
diff --git a/arch/arm/boot/dts/qcom-msm8974.dtsi b/arch/arm/boot/dts/qcom-msm8974.dtsi
index d9019a49b292..1250e071a6e2 100644
--- a/arch/arm/boot/dts/qcom-msm8974.dtsi
+++ b/arch/arm/boot/dts/qcom-msm8974.dtsi
@@ -696,6 +696,7 @@ 
 			compatible = "qcom,msm8974-pinctrl";
 			reg = <0xfd510000 0x4000>;
 			gpio-controller;
+			gpio-ranges = <&msmgpio 0 0 146>;
 			#gpio-cells = <2>;
 			interrupt-controller;
 			#interrupt-cells = <2>;
diff --git a/arch/arm64/boot/dts/qcom/ipq8074.dtsi b/arch/arm64/boot/dts/qcom/ipq8074.dtsi
index 2bc5dec5614d..d2c36b467466 100644
--- a/arch/arm64/boot/dts/qcom/ipq8074.dtsi
+++ b/arch/arm64/boot/dts/qcom/ipq8074.dtsi
@@ -24,11 +24,12 @@ 
 		ranges = <0 0 0 0xffffffff>;
 		compatible = "simple-bus";
 
-		pinctrl@1000000 {
+		tlmm: pinctrl@1000000 {
 			compatible = "qcom,ipq8074-pinctrl";
 			reg = <0x1000000 0x300000>;
 			interrupts = <GIC_SPI 208 IRQ_TYPE_LEVEL_HIGH>;
 			gpio-controller;
+			gpio-ranges = <&tlmm 0 0 70>;
 			#gpio-cells = <0x2>;
 			interrupt-controller;
 			#interrupt-cells = <0x2>;
diff --git a/arch/arm64/boot/dts/qcom/msm8916.dtsi b/arch/arm64/boot/dts/qcom/msm8916.dtsi
index e51b04900726..e06cb90c8ec3 100644
--- a/arch/arm64/boot/dts/qcom/msm8916.dtsi
+++ b/arch/arm64/boot/dts/qcom/msm8916.dtsi
@@ -281,6 +281,7 @@ 
 			reg = <0x1000000 0x300000>;
 			interrupts = <GIC_SPI 208 IRQ_TYPE_LEVEL_HIGH>;
 			gpio-controller;
+			gpio-ranges = <&msmgpio 0 0 122>;
 			#gpio-cells = <2>;
 			interrupt-controller;
 			#interrupt-cells = <2>;
diff --git a/arch/arm64/boot/dts/qcom/msm8992.dtsi b/arch/arm64/boot/dts/qcom/msm8992.dtsi
index 171578747ed0..173b6bc60816 100644
--- a/arch/arm64/boot/dts/qcom/msm8992.dtsi
+++ b/arch/arm64/boot/dts/qcom/msm8992.dtsi
@@ -179,6 +179,7 @@ 
 			reg = <0xfd510000 0x4000>;
 			interrupts = <GIC_SPI 208 IRQ_TYPE_LEVEL_HIGH>;
 			gpio-controller;
+			gpio-ranges = <&msmgpio 0 0 146>;
 			#gpio-cells = <2>;
 			interrupt-controller;
 			#interrupt-cells = <2>;
diff --git a/arch/arm64/boot/dts/qcom/msm8994.dtsi b/arch/arm64/boot/dts/qcom/msm8994.dtsi
index f33c41d01c86..68705db4696b 100644
--- a/arch/arm64/boot/dts/qcom/msm8994.dtsi
+++ b/arch/arm64/boot/dts/qcom/msm8994.dtsi
@@ -141,6 +141,7 @@ 
 			reg = <0xfd510000 0x4000>;
 			interrupts = <GIC_SPI 208 IRQ_TYPE_LEVEL_HIGH>;
 			gpio-controller;
+			gpio-ranges = <&msmgpio 0 0 146>;
 			#gpio-cells = <2>;
 			interrupt-controller;
 			#interrupt-cells = <2>;
diff --git a/arch/arm64/boot/dts/qcom/msm8996.dtsi b/arch/arm64/boot/dts/qcom/msm8996.dtsi
index 0a6f7952bbb1..18511e782cbd 100644
--- a/arch/arm64/boot/dts/qcom/msm8996.dtsi
+++ b/arch/arm64/boot/dts/qcom/msm8996.dtsi
@@ -530,6 +530,7 @@ 
 			reg = <0x01010000 0x300000>;
 			interrupts = <GIC_SPI 208 IRQ_TYPE_LEVEL_HIGH>;
 			gpio-controller;
+			gpio-ranges = <&msmgpio 0 0 150>;
 			#gpio-cells = <2>;
 			interrupt-controller;
 			#interrupt-cells = <2>;
diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
index 495432f3341b..258fa357d946 100644
--- a/drivers/pinctrl/qcom/pinctrl-msm.c
+++ b/drivers/pinctrl/qcom/pinctrl-msm.c
@@ -831,11 +831,22 @@  static int msm_gpio_init(struct msm_pinctrl *pctrl)
 		return ret;
 	}
 
-	ret = gpiochip_add_pin_range(&pctrl->chip, dev_name(pctrl->dev), 0, 0, chip->ngpio);
-	if (ret) {
-		dev_err(pctrl->dev, "Failed to add pin range\n");
-		gpiochip_remove(&pctrl->chip);
-		return ret;
+	if (!is_of_node(pctrl->dev->fwnode)) {
+		/*
+		 * gpiochip_add_pin_range() is meant for platforms that
+		 * don't support DT. All DT platforms should just add
+		 * the gpio-ranges property to the pinctrl device node.
+		 *
+		 * See Documentation/devicetree/bindings/gpio/gpio.txt .
+		 */
+		ret = gpiochip_add_pin_range(&pctrl->chip,
+			dev_name(pctrl->dev), 0, 0, chip->ngpio);
+
+		if (ret) {
+			dev_err(pctrl->dev, "Failed to add pin range\n");
+			gpiochip_remove(&pctrl->chip);
+			return ret;
+		}
 	}
 
 	ret = gpiochip_irqchip_add(chip,