On Tue, May 23, 2017 at 11:43 PM, Nikita Yushchenko <nikita.yoush@xxxxxxxxxxxxxxxxxx> wrote: > > > 24.05.2017 02:25, Andrey Smirnov wrote: >> On Mon, May 22, 2017 at 11:52 PM, Nikita Yushchenko >> <nikita.yoush@xxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxx http://lists.infradead.org/mailman/listinfo/barebox
