Hi, On 22 August 2016 at 14:46, Linus Walleij <linus.walleij@xxxxxxxxxx> wrote: > On Fri, Aug 19, 2016 at 12:53 PM, Jonas Gorski <jonas.gorski@xxxxxxxxx> wrote: > >> Setup directory and add a helper for bcm63xx pinctrl support. >> >> Signed-off-by: Jonas Gorski <jonas.gorski@xxxxxxxxx> > >> drivers/pinctrl/bcm63xx/Kconfig | 3 + >> drivers/pinctrl/bcm63xx/Makefile | 1 + > > Why not just put it in the existing pinctrl/bcm directory? Because at the time I started writing these drivers it was still exclusive for ARCH_BCM, will move them there. > The drivers in there share nothing but being Broadcom > anyways. And when you're at it, do take a look at the > other Broadcom drivers to check if they would > happen to share something with your hardware, I find > it dazzling that hardware engineers repeatedly reinvents > pin controllers but what can I do. > >> +config PINCTRL_BCM63XX >> + bool >> + select GPIO_GENERIC > > depends on ARCH_****? This isn't a user selectable symbol so I don't see the need for that. > > depends on OF_GPIO? > > Will it really be used on non-DT systems? I plan to use it on both mips/bcm63xx (no dts support) and bmips (dts support), so yes. > >> +#define BANK_SIZE sizeof(u32) >> +#define PINS_PER_BANK (BANK_SIZE * BITS_PER_BYTE) >> + >> +#ifdef CONFIG_OF >> +static int bcm63xx_gpio_of_xlate(struct gpio_chip *gc, >> + const struct of_phandle_args *gpiospec, >> + u32 *flags) >> +{ >> + struct gpio_chip *base = gpiochip_get_data(gc); >> + int pin = gpiospec->args[0]; >> + >> + if (gc != &base[pin / PINS_PER_BANK]) >> + return -EINVAL; >> + >> + pin = pin % PINS_PER_BANK; >> + >> + if (pin >= gc->ngpio) >> + return -EINVAL; >> + >> + if (flags) >> + *flags = gpiospec->args[1]; >> + >> + return pin; >> +} >> +#endif > > - Do you really have to support the !OF_GPIO case? (depend in Kconfig, > get rid of #ifdef) See above. > > - The only reason for not using of_gpio_simple_xlate() seems to be that > you have several GPIO banks. So why not model every bank as a > separate device? Or did you consider this already? I did consider it, but it makes the !OF case more complicated, and the current of_gpio base code requires changing for it. That's because some of the pinctrl chips need to set the gpio-direction for muxes, and for that need to have a reference to the gpio-controller(s). Since any muxes directly tied to the controller node will get applied as soon as the controller is registered, it needs to aquire the gpio-controller references first. On the gpio-controller side, to flag these a requiring pinctrl those would then have a gpio-range property, which will cause the probe being deferred until the reference to the controller can be resolved. Which waits for the gpio-controller to be registered, so it can resolve its references to it. A true catch 22 ;-) This could probably resolved by deferring resolving node-to-pincontrol devices to gpio request time. Not sure if this then would conflict which gpio-hogs on such a gpio-controller node? I haven't really thought how much work it would be for the !OF case, but would probably mean I need to acquire references based on their pdev names. >> + gc[i].request = gpiochip_generic_request; >> + gc[i].free = gpiochip_generic_free; >> + >> +#ifdef CONFIG_OF >> + gc[i].of_gpio_n_cells = 2; >> + gc[i].of_xlate = bcm63xx_gpio_of_xlate; >> +#endif >> + >> + gc[i].label = label; >> + gc[i].ngpio = pins; >> + >> + devm_gpiochip_add_data(dev, &gc[i], gc); > > Because this also gets pretty hairy... since you don't have one device > per gpiochip. > >> +static void bcm63xx_setup_pinranges(struct gpio_chip *gc, const char *name, >> + int ngpio) >> +{ >> + int i, chips = DIV_ROUND_UP(ngpio, PINS_PER_BANK); >> + >> + for (i = 0; i < chips; i++) { >> + int offset, pins; >> + >> + offset = i * PINS_PER_BANK; >> + pins = min_t(int, ngpio - offset, PINS_PER_BANK); >> + >> + gpiochip_add_pin_range(&gc[i], name, 0, offset, pins); >> + } >> +} > > And this, same thing. Could be so much easier if it was just the same > driver instantiated twice for two banks. > >> + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dirout"); >> + dirout = devm_ioremap_resource(&pdev->dev, res); >> + if (IS_ERR(dirout)) >> + return ERR_CAST(dirout); >> + >> + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dat"); >> + data = devm_ioremap_resource(&pdev->dev, res); >> + if (IS_ERR(data)) >> + return ERR_CAST(data); >> + >> + sz = resource_size(res); >> + >> + ret = bcm63xx_setup_gpio(&pdev->dev, gc, dirout, data, sz, ngpio); >> + if (ret) >> + return ERR_PTR(ret); >> + >> + pctldev = devm_pinctrl_register(&pdev->dev, desc, priv); >> + if (IS_ERR(pctldev)) >> + return pctldev; >> + >> + bcm63xx_setup_pinranges(gc, pinctrl_dev_get_devname(pctldev), ngpio); >> + >> + dev_info(&pdev->dev, "registered at mmio %p\n", dirout); >> + >> + return pctldev; > > A current trend in simple MMIO devices is to simply add themselves as a > compatible string in drivers/gpio/gpio-mmio.c. Example: > > https://git.kernel.org/cgit/linux/kernel/git/linusw/linux-gpio.git/commit/drivers/gpio/gpio-mmio.c?h=devel&id=05cc995f4d44c2b14a1f15f3271cc9715cb81099 > > This could be a viable approach if you find a way to flag that such a GPIO > has a pin control backend. The most obvious would be having a gpio-ranges property, but this leads to issues mentioned above. And only works for OF. Regards Jonas -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html