From mboxrd@z Thu Jan 1 00:00:00 1970 From: Gregory CLEMENT Subject: Re: [PATCH 3/6] pinctrl: armada-37xx: Add gpio support Date: Wed, 22 Mar 2017 12:54:50 +0100 Message-ID: <874lylfrw5.fsf@free-electrons.com> References: <20161222172501.16121-1-gregory.clement@free-electrons.com> <20161222172501.16121-4-gregory.clement@free-electrons.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: In-Reply-To: (Linus Walleij's message of "Fri, 30 Dec 2016 09:51:51 +0100") List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=m.gmane.org@lists.infradead.org To: Linus Walleij Cc: Thomas Petazzoni , Andrew Lunn , Jason Cooper , Hua Jing , Omri Itach , Nadav Haklai , "linux-gpio@vger.kernel.org" , Victor Gu , Terry Zhou , Marcin Wojtas , Wilson Ding , "linux-arm-kernel@lists.infradead.org" , Sebastian Hesselbarth List-Id: linux-gpio@vger.kernel.org SGkgTGludXMsCiAKIE9uIHZlbi4sIGTDqWMuIDMwIDIwMTYsIExpbnVzIFdhbGxlaWogPGxpbnVz LndhbGxlaWpAbGluYXJvLm9yZz4gd3JvdGU6Cgo+IE9uIFRodSwgRGVjIDIyLCAyMDE2IGF0IDY6 MjQgUE0sIEdyZWdvcnkgQ0xFTUVOVAo+IDxncmVnb3J5LmNsZW1lbnRAZnJlZS1lbGVjdHJvbnMu Y29tPiB3cm90ZToKPgo+PiBHUElPIG1hbmFnZW1lbnQgaXMgcHJldHR5IHNpbXBsZSBhbmQgaXMg cGFydCBvZiB0aGUgc2FtZSBJUCB0aGFuIHRoZSBwaW4KPj4gY29udHJvbGxlciBmb3IgdGhlIEFy bWFkYSAzN3h4IFNvQ3MuICBUaGlzIHBhdGNoIGFkZHMgdGhlIEdQSU8gc3VwcG9ydCB0bwo+PiB0 aGUgcGluY3RybC1hcm1hZGEtMzd4eC5jIGZpbGUsIGl0IGFsc28gYWxsb3dzIHNoYXJpbmcgY29t bW9uIGZ1bmN0aW9ucwo+PiBiZXR3ZWVuIHRoZSBncGlvbGliIGFuZCB0aGUgcGluY3RybCBkcml2 ZXJzLgo+Pgo+PiBTaWduZWQtb2ZmLWJ5OiBHcmVnb3J5IENMRU1FTlQgPGdyZWdvcnkuY2xlbWVu dEBmcmVlLWVsZWN0cm9ucy5jb20+Cj4KPiBTb21lIHJlbWFya3M6Cj4KPj4gK3N0YXRpYyBpbnQg YXJtYWRhXzM3eHhfZ3Bpb19nZXRfZGlyZWN0aW9uKHN0cnVjdCBncGlvX2NoaXAgKmNoaXAsCj4+ ICsgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgIHVuc2lnbmVkIGludCBv ZmZzZXQpCj4+ICt7Cj4+ICsgICAgICAgc3RydWN0IGFybWFkYV8zN3h4X3BpbmN0cmwgKmluZm8g PSBncGlvY2hpcF9nZXRfZGF0YShjaGlwKTsKPj4gKyAgICAgICB1bnNpZ25lZCBpbnQgcmVnID0g T1VUUFVUX0VOOwo+PiArICAgICAgIHVuc2lnbmVkIGludCB2YWwsIG1hc2s7Cj4+ICsKPj4gKyAg ICAgICBpZiAob2Zmc2V0ID49IEdQSU9fUEVSX1JFRykgewo+PiArICAgICAgICAgICAgICAgb2Zm c2V0IC09IEdQSU9fUEVSX1JFRzsKPj4gKyAgICAgICAgICAgICAgIHJlZyArPSBzaXplb2YodTMy KTsKPj4gKyAgICAgICB9Cj4KPiBBZGQgYSBjb21tZW50IHNheWluZyB3ZSBuZXZlciBoYXZlIG1v cmUgdGhhbiB0d28gcmVnaXN0ZXJzPwo+IElmIHRoZXJlIHdvdWxkIGJlIHRocmVlIHJlZ2lzdGVy cyB0aGlzIHdvdWxkIGZhaWwsIHJpZ2h0PwoKSSBhZGRlZCB0aGUgY29tbWVudAoKPgo+PiArICAg ICAgIG1hc2sgPSBCSVQob2Zmc2V0KTsKPj4gKwo+PiArICAgICAgIHJlZ21hcF9yZWFkKGluZm8t PnJlZ21hcCwgcmVnLCAmdmFsKTsKPj4KPj4gKyAgICAgICByZXR1cm4gKHZhbCAmIG1hc2spID09 IDA7Cj4KPiBVc2UgdGhpczoKPgo+IHJldHVybiAhKHZhbCAmIG1hc2spOwoKZG9uZSBidXQgSSBj b3VsZCB0b3UgZXhwbGFpbiB0aGUgYWR2YW50YWdlIG9mIGRvaW5nIGl0PwoKPgo+PiArc3RhdGlj IGludCBhcm1hZGFfMzd4eF9ncGlvX2dldChzdHJ1Y3QgZ3Bpb19jaGlwICpjaGlwLCB1bnNpZ25l ZCBpbnQgb2Zmc2V0KQo+PiArewo+PiArICAgICAgIHN0cnVjdCBhcm1hZGFfMzd4eF9waW5jdHJs ICppbmZvID0gZ3Bpb2NoaXBfZ2V0X2RhdGEoY2hpcCk7Cj4+ICsgICAgICAgdW5zaWduZWQgaW50 IHJlZyA9IElOUFVUX1ZBTDsKPj4gKyAgICAgICB1bnNpZ25lZCBpbnQgdmFsLCBtYXNrOwo+PiAr Cj4+ICsgICAgICAgaWYgKG9mZnNldCA+PSBHUElPX1BFUl9SRUcpIHsKPj4gKyAgICAgICAgICAg ICAgIG9mZnNldCAtPSBHUElPX1BFUl9SRUc7Cj4+ICsgICAgICAgICAgICAgICByZWcgKz0gc2l6 ZW9mKHUzMik7Cj4+ICsgICAgICAgfQo+PiArICAgICAgIG1hc2sgPSBCSVQob2Zmc2V0KTsKPgo+ IFRoaXMgY29kZSBpcyByZXBlYXRpbmcuIEJyZWFrIG91dCBhIHN0YXRpYyAoaW5saW5lPykgaGVs cGVyIHRvIGRvCj4gdGhpcy4KCmRvbmUKCj4KPj4gK3N0YXRpYyBpbnQgYXJtYWRhXzM3eHhfZ3Bp b2xpYl9yZWdpc3RlcihzdHJ1Y3QgcGxhdGZvcm1fZGV2aWNlICpwZGV2LAo+PiArICAgICAgICAg ICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgc3RydWN0IGFybWFkYV8zN3h4X3BpbmN0cmwg KmluZm8pCj4KPiBOaXQ6IGdwaW9jaGlwX3JlZ2lzdGVyIG9yIHNvIGlzIG1vcmUgdG8gdGhlIHBv aW50Lgo+Cj4+ICsgICAgICAgcmV0ID0gZ3Bpb2NoaXBfYWRkX3Bpbl9yYW5nZSgmaW5mby0+Z3Bp b19jaGlwLCBkZXZfbmFtZShkZXYpLCAwLAo+PiArICAgICAgICAgICAgICAgICAgICAgICAgICAg ICAgICAgICAgcGluYmFzZSwgaW5mby0+ZGF0YS0+bnJfcGlucyk7Cj4+ICsgICAgICAgaWYgKHJl dCkKPj4gKyAgICAgICAgICAgICAgIHJldHVybiByZXQ7Cj4KPiBXaHkgZG8geW91IGRvIHRoaXM/ Cj4KPiBXaHkgbm90IGp1c3QgcHV0IHRoZSByYW5nZXMgaW50byB0aGUgZGV2aWNlIHRyZWU/IFdl IGFscmVhZHkgc3VwcG9ydAo+IHRoaXMgaW4gdGhlIGdwaW9saWIgY29yZSBhbmQgaXQgaXMgaGVs cGZ1bC4KPgo+IFNlZSBEb2N1bWVudGF0aW9uL2RldmljZXRyZWUvYmluZGluZ3MvZ3Bpby9ncGlv LnR4dAo+IGFuZCBvdGhlciBEVFMgZmlsZXMgZm9yIGdwaW8tcmFuZ2VzLgoKRm9sbG93aW5nIHlv dXIgcmV2aWV3LCBJIHRyaWVkIHRvIHVzZSBpdCBidXQgaXQgZGlkbid0IHdvcmsgZm9yCm1lLiBX aGVuIHRoZSBzZWNvbmQgcGluIGNvbnRyb2xsZXIgd2FzIHByb2JlZCB0aGVuIHRoZXJlIHdhcyBj b2xsaXNpb24KZm9yIHRoZSBncGlvIG51bWJlci4gSSB0cmllZCBzZXZlcmFsIGNvbWJpbmF0aW9u IHdpdGhvdXQgYW55IGx1Y2suCgpTbyBmb3Igbm93IEkgbGVmdCBpdCBhc2lkZS4KCkkgY2FuIHNo b3cgeW91IHRoZSBlcnJvcnMgbWVzc2FnZSBJIGdldCBhbmQgdGhlIGJpbmRpbmcgSSB1c2VkIGlm IHlvdQphcmUgaW50ZXJlc3RlZC4KCgpUaGFua3MsCgpHcmVnb3J5CgotLSAKR3JlZ29yeSBDbGVt ZW50LCBGcmVlIEVsZWN0cm9ucwpLZXJuZWwsIGRyaXZlcnMsIHJlYWwtdGltZSBhbmQgZW1iZWRk ZWQgTGludXgKZGV2ZWxvcG1lbnQsIGNvbnN1bHRpbmcsIHRyYWluaW5nIGFuZCBzdXBwb3J0Lgpo dHRwOi8vZnJlZS1lbGVjdHJvbnMuY29tCgpfX19fX19fX19fX19fX19fX19fX19fX19fX19fX19f X19fX19fX19fX19fX19fXwpsaW51eC1hcm0ta2VybmVsIG1haWxpbmcgbGlzdApsaW51eC1hcm0t a2VybmVsQGxpc3RzLmluZnJhZGVhZC5vcmcKaHR0cDovL2xpc3RzLmluZnJhZGVhZC5vcmcvbWFp bG1hbi9saXN0aW5mby9saW51eC1hcm0ta2VybmVsCg== From mboxrd@z Thu Jan 1 00:00:00 1970 From: gregory.clement@free-electrons.com (Gregory CLEMENT) Date: Wed, 22 Mar 2017 12:54:50 +0100 Subject: [PATCH 3/6] pinctrl: armada-37xx: Add gpio support In-Reply-To: (Linus Walleij's message of "Fri, 30 Dec 2016 09:51:51 +0100") References: <20161222172501.16121-1-gregory.clement@free-electrons.com> <20161222172501.16121-4-gregory.clement@free-electrons.com> Message-ID: <874lylfrw5.fsf@free-electrons.com> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org Hi Linus, On ven., d?c. 30 2016, Linus Walleij wrote: > On Thu, Dec 22, 2016 at 6:24 PM, Gregory CLEMENT > wrote: > >> GPIO management is pretty simple and is part of the same IP than the pin >> controller for the Armada 37xx SoCs. This patch adds the GPIO support to >> the pinctrl-armada-37xx.c file, it also allows sharing common functions >> between the gpiolib and the pinctrl drivers. >> >> Signed-off-by: Gregory CLEMENT > > Some remarks: > >> +static int armada_37xx_gpio_get_direction(struct gpio_chip *chip, >> + unsigned int offset) >> +{ >> + struct armada_37xx_pinctrl *info = gpiochip_get_data(chip); >> + unsigned int reg = OUTPUT_EN; >> + unsigned int val, mask; >> + >> + if (offset >= GPIO_PER_REG) { >> + offset -= GPIO_PER_REG; >> + reg += sizeof(u32); >> + } > > Add a comment saying we never have more than two registers? > If there would be three registers this would fail, right? I added the comment > >> + mask = BIT(offset); >> + >> + regmap_read(info->regmap, reg, &val); >> >> + return (val & mask) == 0; > > Use this: > > return !(val & mask); done but I could tou explain the advantage of doing it? > >> +static int armada_37xx_gpio_get(struct gpio_chip *chip, unsigned int offset) >> +{ >> + struct armada_37xx_pinctrl *info = gpiochip_get_data(chip); >> + unsigned int reg = INPUT_VAL; >> + unsigned int val, mask; >> + >> + if (offset >= GPIO_PER_REG) { >> + offset -= GPIO_PER_REG; >> + reg += sizeof(u32); >> + } >> + mask = BIT(offset); > > This code is repeating. Break out a static (inline?) helper to do > this. done > >> +static int armada_37xx_gpiolib_register(struct platform_device *pdev, >> + struct armada_37xx_pinctrl *info) > > Nit: gpiochip_register or so is more to the point. > >> + ret = gpiochip_add_pin_range(&info->gpio_chip, dev_name(dev), 0, >> + pinbase, info->data->nr_pins); >> + if (ret) >> + return ret; > > Why do you do this? > > Why not just put the ranges into the device tree? We already support > this in the gpiolib core and it is helpful. > > See Documentation/devicetree/bindings/gpio/gpio.txt > and other DTS files for gpio-ranges. Following your review, I tried to use it but it didn't work for me. When the second pin controller was probed then there was collision for the gpio number. I tried several combination without any luck. So for now I left it aside. I can show you the errors message I get and the binding I used if you are interested. Thanks, Gregory -- Gregory Clement, Free Electrons Kernel, drivers, real-time and embedded Linux development, consulting, training and support. http://free-electrons.com