| Message ID | 2820412.JxDUeWI2ec@debian64 (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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
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
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>
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");