| Message ID | 1445233398-27129-8-git-send-email-pramodku@broadcom.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Oct 19, 2015 at 7:43 AM, Pramod Kumar <pramodku@broadcom.com> wrote: > Since identical hardware is used in several instances and all pins > are not routed to pinctrl hence getting total number of gpios from > DT make more sense hence stop using total number of gpios pins from > drivers and extract it from DT. > > Signed-off-by: Pramod Kumar <pramodku@broadcom.com> > Reviewed-by: Ray Jui <rjui@broadcom.com> > Reviewed-by: Scott Branden <sbranden@broadcom.com> This patch is wrong. Keep this per-compatible code, and only overrid the ngpios if and only if: - The ngpios is set in the DT node - The ngpios in the DT node is *smaller* than the hardware defined number of GPIOs. ngpios is for restricting the number of available lines due to routing etc, not to define what the hardware has, because the hardware most certainly have all the lines, it's just that you're not using all of them. Yours, Linus Walleij
Hi Linus, > -----Original Message----- > From: Linus Walleij [mailto:linus.walleij@linaro.org] > Sent: 27 October 2015 15:22 > To: Pramod Kumar > Cc: Rob Herring; Pawel Moll; Mark Rutland; Ian Campbell; Kumar Gala; Ray Jui; > Scott Branden; Russell King; linux-gpio@vger.kernel.org; bcm-kernel-feedback- > list; Jason Uy; Masahiro Yamada; Thomas Gleixner; Laurent Pinchart; > devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux- > kernel@vger.kernel.org; Jonas Gorski > Subject: Re: [PATCH 07/11] pinctrl: use ngpios propety from DT > > On Mon, Oct 19, 2015 at 7:43 AM, Pramod Kumar <pramodku@broadcom.com> > wrote: > > > Since identical hardware is used in several instances and all pins are > > not routed to pinctrl hence getting total number of gpios from DT make > > more sense hence stop using total number of gpios pins from drivers > > and extract it from DT. > > > > Signed-off-by: Pramod Kumar <pramodku@broadcom.com> > > Reviewed-by: Ray Jui <rjui@broadcom.com> > > Reviewed-by: Scott Branden <sbranden@broadcom.com> > > This patch is wrong. > > Keep this per-compatible code, and only overrid the ngpios if and only if: > > - The ngpios is set in the DT node > - The ngpios in the DT node is *smaller* than the hardware > defined number of GPIOs. > > ngpios is for restricting the number of available lines due to routing etc, not to > define what the hardware has, because the hardware most certainly have all the > lines, it's just that you're not using all of them. > > Yours, > Linus Walleij I discussed with ASIC team regarding this iProc GPIO block. They use a library to create the GPIOs block where "total number of GPIO pins( let say N) in GPIO block" is used as an parameter. Library uses a construct for *a* GPIO pin. This gets instantiated N times to create a complete GPIO block with N pins. All iProc based SoCs uses this library. So I'm not sure whether attaching "total number of GPIOs pins" to compatible-string make sense in this case. I personally feel that passing this number from the DT makes more sense here. Any iProc based future as well as current SoCs would be able to use this driver without any change. Please advise us in this case. Regards, Pramod
On 10/28/2015 4:52 AM, Pramod Kumar wrote: > Hi Linus, > >> -----Original Message----- >> From: Linus Walleij [mailto:linus.walleij@linaro.org] >> Sent: 27 October 2015 15:22 >> To: Pramod Kumar >> Cc: Rob Herring; Pawel Moll; Mark Rutland; Ian Campbell; Kumar Gala; Ray Jui; >> Scott Branden; Russell King; linux-gpio@vger.kernel.org; bcm-kernel-feedback- >> list; Jason Uy; Masahiro Yamada; Thomas Gleixner; Laurent Pinchart; >> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux- >> kernel@vger.kernel.org; Jonas Gorski >> Subject: Re: [PATCH 07/11] pinctrl: use ngpios propety from DT >> >> On Mon, Oct 19, 2015 at 7:43 AM, Pramod Kumar <pramodku@broadcom.com> >> wrote: >> >>> Since identical hardware is used in several instances and all pins are >>> not routed to pinctrl hence getting total number of gpios from DT make >>> more sense hence stop using total number of gpios pins from drivers >>> and extract it from DT. >>> >>> Signed-off-by: Pramod Kumar <pramodku@broadcom.com> >>> Reviewed-by: Ray Jui <rjui@broadcom.com> >>> Reviewed-by: Scott Branden <sbranden@broadcom.com> >> >> This patch is wrong. >> >> Keep this per-compatible code, and only overrid the ngpios if and only if: >> >> - The ngpios is set in the DT node >> - The ngpios in the DT node is *smaller* than the hardware >> defined number of GPIOs. >> >> ngpios is for restricting the number of available lines due to routing etc, not to >> define what the hardware has, because the hardware most certainly have all the >> lines, it's just that you're not using all of them. >> >> Yours, >> Linus Walleij > > I discussed with ASIC team regarding this iProc GPIO block. They use a library to create the GPIOs block where "total number of GPIO pins( let say N) in GPIO block" is used as an parameter. > Library uses a construct for *a* GPIO pin. This gets instantiated N times to create a complete GPIO block with N pins. Just to confirm, N can be *any number*, and when it exceeds 32, additional registers will be created by the library, correct? I think that's what I saw with Cygnus, where 3 instances of this GPIO controller was used, with two of them less supporting less than 32 GPIOs and one of them (ASIU) supporting 146 GPIOs, in which case, 5 register banks are used with 0x200 segment each. > > All iProc based SoCs uses this library. So I'm not sure whether attaching "total number of GPIOs pins" to compatible-string make sense in this case. The closest I can think of is to tie a very large number of N to the "brcm,iproc-gpio" compatible string for all new iProc SoCs, but even that, one can argue how large is *large* > I personally feel that passing this number from the DT makes more sense here. Any iProc based future as well as current SoCs would be able to use this driver without any change. > > Please advise us in this case. > > Regards, > Pramod >
On Wed, Oct 28, 2015 at 12:52 PM, Pramod Kumar <pramodku@broadcom.com> wrote: > I discussed with ASIC team regarding this iProc GPIO block. They use a library to create the GPIOs block where "total number of GPIO pins( let say N) in GPIO block" is used as an parameter. > Library uses a construct for *a* GPIO pin. This gets instantiated N times to create a complete GPIO block with N pins. > > All iProc based SoCs uses this library. So I'm not sure whether attaching "total number of GPIOs pins" to compatible-string make sense in this case. > I personally feel that passing this number from the DT makes more sense here. Any iProc based future as well as current SoCs would be able to use this driver without any change. > > Please advise us in this case. Hm! You make a good case. But this contradicts the traditional use of ngpios. But on the other hand: git grep ngpio Documentation/devicetree/bindings/gpio/ Gives at hand that the use of ngpio[s] is a complete mess. :( I will think about patching the standard bindings to fix this mess and include your case. Give me some time. Yours, Linus Walleij
On 29.10.2015 15:36, Linus Walleij wrote: > On Wed, Oct 28, 2015 at 12:52 PM, Pramod Kumar <pramodku@broadcom.com> wrote: > >> I discussed with ASIC team regarding this iProc GPIO block. They use a library to create the GPIOs block where "total number of GPIO pins( let say N) in GPIO block" is used as an parameter. >> Library uses a construct for *a* GPIO pin. This gets instantiated N times to create a complete GPIO block with N pins. >> >> All iProc based SoCs uses this library. So I'm not sure whether attaching "total number of GPIOs pins" to compatible-string make sense in this case. >> I personally feel that passing this number from the DT makes more sense here. Any iProc based future as well as current SoCs would be able to use this driver without any change. >> >> Please advise us in this case. > > Hm! You make a good case. > > But this contradicts the traditional use of ngpios. > > But on the other hand: > git grep ngpio Documentation/devicetree/bindings/gpio/ > > Gives at hand that the use of ngpio[s] is a complete mess. > > :( > > I will think about patching the standard bindings to fix this mess > and include your case. Give me some time. Using ngpios to restrict the amount of actually available GPIOs from the possible amount of GPIOs seems a rather limited use, as rerouted gpios are seldom at the end of the GPIO space. So maybe it makes more sense to use ngpio as the number of possible gpios and then have an additional "reserved-gpios" bitmask / list of unavailable gpios? Ideally those would be just consumed by a pinctrl instance, but I guess that these are sometimes controlled through pinstrapping, so there might be no driver to attach to them. Regards Jonas
On Thu, Oct 29, 2015 at 3:47 PM, Jonas Gorski <jogo@openwrt.org> wrote: > Using ngpios to restrict the amount of actually available GPIOs from > the possible amount of GPIOs seems a rather limited use, as rerouted > gpios are seldom at the end of the GPIO space. I need an example. > So maybe it makes more sense to use ngpio as the number of > possible gpios and then have an additional "reserved-gpios" > bitmask / list of unavailable gpios? This idea is good, but let's not make upfront design until we have a piece of hardware that requires exactly this. I sent a patch to the gpio.txt binding including a diplomatic statement on this... > Ideally those would be just consumed by a pinctrl instance, but I > guess that these are sometimes controlled through pinstrapping, > so there might be no driver to attach to them. Uhmm.... Not quite following but all right... With gpio ranges the GPIO to pins are mapped, and they can switch functions at runtime. Yours, Linus Walleij
diff --git a/drivers/pinctrl/bcm/pinctrl-cygnus-gpio.c b/drivers/pinctrl/bcm/pinctrl-cygnus-gpio.c index 12a48f4..498a58a 100644 --- a/drivers/pinctrl/bcm/pinctrl-cygnus-gpio.c +++ b/drivers/pinctrl/bcm/pinctrl-cygnus-gpio.c @@ -642,35 +642,11 @@ static void cygnus_gpio_unregister_pinconf(struct cygnus_gpio *chip) pinctrl_unregister(chip->pctl); } -struct cygnus_gpio_data { - unsigned num_gpios; -}; - -static const struct cygnus_gpio_data cygnus_cmm_gpio_data = { - .num_gpios = 24, -}; - -static const struct cygnus_gpio_data cygnus_asiu_gpio_data = { - .num_gpios = 146, -}; - -static const struct cygnus_gpio_data cygnus_crmu_gpio_data = { - .num_gpios = 6, -}; - static const struct of_device_id cygnus_gpio_of_match[] = { - { - .compatible = "brcm,cygnus-ccm-gpio", - .data = &cygnus_cmm_gpio_data, - }, - { - .compatible = "brcm,cygnus-asiu-gpio", - .data = &cygnus_asiu_gpio_data, - }, - { - .compatible = "brcm,cygnus-crmu-gpio", - .data = &cygnus_crmu_gpio_data, - } + { .compatible = "brcm,cygnus-ccm-gpio" }, + { .compatible = "brcm,cygnus-asiu-gpio" }, + { .compatible = "brcm,cygnus-crmu-gpio" }, + { } }; static int cygnus_gpio_probe(struct platform_device *pdev) @@ -681,14 +657,6 @@ static int cygnus_gpio_probe(struct platform_device *pdev) struct gpio_chip *gc; u32 ngpios; int irq, ret; - const struct of_device_id *match; - const struct cygnus_gpio_data *gpio_data; - - match = of_match_device(cygnus_gpio_of_match, dev); - if (!match) - return -ENODEV; - gpio_data = match->data; - ngpios = gpio_data->num_gpios; chip = devm_kzalloc(dev, sizeof(*chip), GFP_KERNEL); if (!chip) @@ -713,6 +681,11 @@ static int cygnus_gpio_probe(struct platform_device *pdev) } } + if (of_property_read_u32(dev->of_node, "ngpios", &ngpios)) { + dev_err(&pdev->dev, "missing ngpios DT property\n"); + return -ENODEV; + } + spin_lock_init(&chip->lock); gc = &chip->gc;