From mboxrd@z Thu Jan 1 00:00:00 1970 Return-path: Received: from mail-pf0-x244.google.com ([2607:f8b0:400e:c00::244]) by bombadil.infradead.org with esmtps (Exim 4.87 #1 (Red Hat Linux)) id 1dFiHv-0006Gp-4I for barebox@lists.infradead.org; Tue, 30 May 2017 14:38:24 +0000 Received: by mail-pf0-x244.google.com with SMTP id w69so18290623pfk.1 for ; Tue, 30 May 2017 07:38:02 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: <06bc04e1-6a7c-c676-23f9-5566f93e5d56@cogentembedded.com> References: <20170522152420.14443-1-andrew.smirnov@gmail.com> <20170522152420.14443-3-andrew.smirnov@gmail.com> <46762e56-b2a0-4afd-6967-99b36af1e6fb@cogentembedded.com> <06bc04e1-6a7c-c676-23f9-5566f93e5d56@cogentembedded.com> From: Andrey Smirnov Date: Tue, 30 May 2017 07:38:01 -0700 Message-ID: List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "barebox" Errors-To: barebox-bounces+u.kleine-koenig=pengutronix.de@lists.infradead.org Subject: Re: [PATCH 3/4] gpiolib: Add support for GPIO "hog" nodes To: Nikita Yushchenko Cc: "barebox@lists.infradead.org" , Chris Healy , Sascha Hauer On Tue, May 23, 2017 at 11:43 PM, Nikita Yushchenko wrote: > > > 24.05.2017 02:25, Andrey Smirnov wrote: >> On Mon, May 22, 2017 at 11:52 PM, Nikita Yushchenko >> wrote: >>>> + ret = of_property_read_u32(chip_np, "#gpio-cells", &gpio_cells); >>>> + if (ret) >>>> + return ret; >>>> + >>>> + if (WARN_ON(gpio_cells != 2)) >>>> + return -ENOTSUPP; >>>> + >>>> + ret = of_property_read_u32_index(np, "gpios", idx * gpio_cells, >>>> + &gpio_num); >>>> + if (ret) >>>> + return ret; >>>> + >>>> + ret = of_property_read_u32_index(np, "gpios", idx * gpio_cells + 1, >>>> + &gpio_flags); >>>> + if (ret) >>>> + return ret; >>> >>> Doesn't this hardcode interpretation of device tree words in gpio >>> specification - while this is intended to be gpio-provider specific and >>> that's why #gpio-cells exist? >>> >> >> It does and yes that's my understanding of the purpose of #gpio-cells >> as well. The reason I did in such a primitive way was because >> Barebox's GPIO subsystem doesn't have any translation plumbing to be >> able to handle anything more than a simple one dimensional offset. >> Given the fact that of_get_named_gpio_flags() make similar assumption >> I thought that there are no real consumers of that functionality and >> left proper implementation as a future improvement that can be made >> once the need arises. > > Maybe then at least make this [wrong] thing done in single place? I.e. > extract relevant code from of_get_named_gpio_flags() into separate > routine and call it from two places? (And add a comment there, that it > is a stub assuming dump representation) > The code of the two doesn't have much, if anything, in common. Of_get_named_gpio_flags is expecting a phandle to the gpio node be a part of the field it parses, whereas gpio specifier in hog nodes omits that. I don't think I can have any meaningful code sharing here. >>>> +static int of_gpiochip_scan_gpios(struct gpio_chip *chip) >>> >>> Not best choice of name for routine that scans hogs? >>> >>> (although I understand that it comes from linux counterpart) >>> >> >> Eh, I don't have any strong opinion on this one, I am more than happy >> to rename it if you think there are better alternatives. > > of_gpiochip_scan_hogs() ? > Sure, I'll do that in v2. Thanks, Andrey Smirnov _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox