| Message ID | 1445233398-27129-8-git-send-email-pramodku@broadcom.com |
|---|---|
| State | New |
| 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 -- 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
SGkgTGludXMsDQoNCj4gLS0tLS1PcmlnaW5hbCBNZXNzYWdlLS0tLS0NCj4gRnJvbTogTGludXMg V2FsbGVpaiBbbWFpbHRvOmxpbnVzLndhbGxlaWpAbGluYXJvLm9yZ10NCj4gU2VudDogMjcgT2N0 b2JlciAyMDE1IDE1OjIyDQo+IFRvOiBQcmFtb2QgS3VtYXINCj4gQ2M6IFJvYiBIZXJyaW5nOyBQ YXdlbCBNb2xsOyBNYXJrIFJ1dGxhbmQ7IElhbiBDYW1wYmVsbDsgS3VtYXIgR2FsYTsgUmF5IEp1 aTsNCj4gU2NvdHQgQnJhbmRlbjsgUnVzc2VsbCBLaW5nOyBsaW51eC1ncGlvQHZnZXIua2VybmVs Lm9yZzsgYmNtLWtlcm5lbC1mZWVkYmFjay0NCj4gbGlzdDsgSmFzb24gVXk7IE1hc2FoaXJvIFlh bWFkYTsgVGhvbWFzIEdsZWl4bmVyOyBMYXVyZW50IFBpbmNoYXJ0Ow0KPiBkZXZpY2V0cmVlQHZn ZXIua2VybmVsLm9yZzsgbGludXgtYXJtLWtlcm5lbEBsaXN0cy5pbmZyYWRlYWQub3JnOyBsaW51 eC0NCj4ga2VybmVsQHZnZXIua2VybmVsLm9yZzsgSm9uYXMgR29yc2tpDQo+IFN1YmplY3Q6IFJl OiBbUEFUQ0ggMDcvMTFdIHBpbmN0cmw6IHVzZSBuZ3Bpb3MgcHJvcGV0eSBmcm9tIERUDQo+IA0K PiBPbiBNb24sIE9jdCAxOSwgMjAxNSBhdCA3OjQzIEFNLCBQcmFtb2QgS3VtYXIgPHByYW1vZGt1 QGJyb2FkY29tLmNvbT4NCj4gd3JvdGU6DQo+IA0KPiA+IFNpbmNlIGlkZW50aWNhbCBoYXJkd2Fy ZSBpcyB1c2VkIGluIHNldmVyYWwgaW5zdGFuY2VzIGFuZCBhbGwgcGlucyBhcmUNCj4gPiBub3Qg cm91dGVkIHRvIHBpbmN0cmwgaGVuY2UgZ2V0dGluZyB0b3RhbCBudW1iZXIgb2YgZ3Bpb3MgZnJv bSBEVCBtYWtlDQo+ID4gbW9yZSBzZW5zZSBoZW5jZSBzdG9wIHVzaW5nIHRvdGFsIG51bWJlciBv ZiBncGlvcyBwaW5zIGZyb20gZHJpdmVycw0KPiA+IGFuZCBleHRyYWN0IGl0IGZyb20gRFQuDQo+ ID4NCj4gPiBTaWduZWQtb2ZmLWJ5OiBQcmFtb2QgS3VtYXIgPHByYW1vZGt1QGJyb2FkY29tLmNv bT4NCj4gPiBSZXZpZXdlZC1ieTogUmF5IEp1aSA8cmp1aUBicm9hZGNvbS5jb20+DQo+ID4gUmV2 aWV3ZWQtYnk6IFNjb3R0IEJyYW5kZW4gPHNicmFuZGVuQGJyb2FkY29tLmNvbT4NCj4gDQo+IFRo aXMgcGF0Y2ggaXMgd3JvbmcuDQo+IA0KPiBLZWVwIHRoaXMgcGVyLWNvbXBhdGlibGUgY29kZSwg YW5kIG9ubHkgb3ZlcnJpZCB0aGUgbmdwaW9zIGlmIGFuZCBvbmx5IGlmOg0KPiANCj4gLSBUaGUg bmdwaW9zIGlzIHNldCBpbiB0aGUgRFQgbm9kZQ0KPiAtIFRoZSBuZ3Bpb3MgaW4gdGhlIERUIG5v ZGUgaXMgKnNtYWxsZXIqIHRoYW4gdGhlIGhhcmR3YXJlDQo+ICAgZGVmaW5lZCBudW1iZXIgb2Yg R1BJT3MuDQo+IA0KPiBuZ3Bpb3MgaXMgZm9yIHJlc3RyaWN0aW5nIHRoZSBudW1iZXIgb2YgYXZh aWxhYmxlIGxpbmVzIGR1ZSB0byByb3V0aW5nIGV0Yywgbm90IHRvDQo+IGRlZmluZSB3aGF0IHRo ZSBoYXJkd2FyZSBoYXMsIGJlY2F1c2UgdGhlIGhhcmR3YXJlIG1vc3QgY2VydGFpbmx5IGhhdmUg YWxsIHRoZQ0KPiBsaW5lcywgaXQncyBqdXN0IHRoYXQgeW91J3JlIG5vdCB1c2luZyBhbGwgb2Yg dGhlbS4NCj4gDQo+IFlvdXJzLA0KPiBMaW51cyBXYWxsZWlqDQoNCkkgZGlzY3Vzc2VkIHdpdGgg QVNJQyB0ZWFtIHJlZ2FyZGluZyB0aGlzIGlQcm9jIEdQSU8gYmxvY2suIFRoZXkgdXNlIGEgbGli cmFyeSB0byBjcmVhdGUgdGhlIEdQSU9zIGJsb2NrIHdoZXJlICJ0b3RhbCBudW1iZXIgb2YgR1BJ TyBwaW5zKCBsZXQgc2F5IE4pIGluIEdQSU8gYmxvY2siIGlzIHVzZWQgYXMgYW4gcGFyYW1ldGVy LiANCkxpYnJhcnkgdXNlcyBhIGNvbnN0cnVjdCBmb3IgKmEqIEdQSU8gcGluLiBUaGlzIGdldHMg aW5zdGFudGlhdGVkIE4gdGltZXMgdG8gY3JlYXRlIGEgY29tcGxldGUgR1BJTyBibG9jayB3aXRo IE4gcGlucy4NCg0KQWxsIGlQcm9jIGJhc2VkIFNvQ3MgdXNlcyB0aGlzIGxpYnJhcnkuIFNvIEkn bSBub3Qgc3VyZSB3aGV0aGVyIGF0dGFjaGluZyAidG90YWwgbnVtYmVyIG9mIEdQSU9zIHBpbnMi IHRvIGNvbXBhdGlibGUtc3RyaW5nIG1ha2Ugc2Vuc2UgaW4gdGhpcyBjYXNlLiANCkkgcGVyc29u YWxseSBmZWVsIHRoYXQgcGFzc2luZyB0aGlzIG51bWJlciBmcm9tIHRoZSBEVCBtYWtlcyBtb3Jl IHNlbnNlIGhlcmUuIEFueSBpUHJvYyBiYXNlZCBmdXR1cmUgYXMgd2VsbCBhcyBjdXJyZW50IFNv Q3Mgd291bGQgYmUgYWJsZSB0byB1c2UgdGhpcyBkcml2ZXIgd2l0aG91dCBhbnkgY2hhbmdlLg0K DQpQbGVhc2UgYWR2aXNlIHVzIGluIHRoaXMgY2FzZS4NCg0KUmVnYXJkcywNClByYW1vZA0K -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 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 > -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 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 -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 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 -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 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 -- 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
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;