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?
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_****?
depends on OF_GPIO?
Will it really be used on non-DT systems?
> +#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)
- 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?
> + 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.
Yours,
Linus Walleij
--
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
