| Message ID | 1471604025-21575-2-git-send-email-jonas.gorski@gmail.com |
|---|---|
| State | New |
| Headers | show |
On Fri, Aug 19, 2016 at 12:53 PM, Jonas Gorski <jonas.gorski@gmail.com> wrote: > Setup directory and add a helper for bcm63xx pinctrl support. > > Signed-off-by: Jonas Gorski <jonas.gorski@gmail.com> > 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 linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On 22 August 2016 at 14:46, Linus Walleij <linus.walleij@linaro.org> wrote: > On Fri, Aug 19, 2016 at 12:53 PM, Jonas Gorski <jonas.gorski@gmail.com> wrote: > >> Setup directory and add a helper for bcm63xx pinctrl support. >> >> Signed-off-by: Jonas Gorski <jonas.gorski@gmail.com> > >> 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 linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Aug 22, 2016 at 3:44 PM, Jonas Gorski <jonas.gorski@gmail.com> wrote: > On 22 August 2016 at 14:46, Linus Walleij <linus.walleij@linaro.org> wrote: >> 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. OK thanks. >> 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. This is usually used to narrow the compile coverage to an arch so that the autobuilders do not explode on the driver when they try to enable and compile it for every arch in the world using randconfig. >> 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. Tentatively OK. If you are sure it will actually happen, generally I don't like "design for the future" I prefer "design for here and now". >> - 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. If you were using syscon to access the register instead of remapping then this problem would go away, because then the pin control and the GPIO driver can simply just write into the same registers to change the direction, because regmap provides marshalling (serialization) and locking of register access so you don't even need a spinlock or anything to share: you're already sharing the regmap abstraction. This is part of the beauty of doing this with syscon+regmap and makes me even more convinced that it is the right solution for these drivers. If you're worried about the pin controller changing the direction of the GPIOs behind the back of the GPIO driver: that is what the .get_direction() callback in the gpio_chip is for, i.e. it is not a problem. > 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 ;-) So avoid that entire dilemma by removing the entire cross-dependency. Use syscon+regmap and just write into the direction registers from the pin controller driver. >>> +#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. And I'm not convinced that having several gpiochips spawn from one device is a good idea. >> 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. Ranges can be registered from boardfiles. In fact that was how it was devised from the beginning, OF ranges were added later. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/MAINTAINERS b/MAINTAINERS index 20bb1d0..33c3026 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -2604,6 +2604,7 @@ F: arch/mips/boot/dts/brcm/bcm*.dts* F: drivers/irqchip/irq-bcm63* F: drivers/irqchip/irq-bcm7* F: drivers/irqchip/irq-brcmstb* +F: drivers/pinctrl/bcm63xx/* F: include/linux/bcm963xx_nvram.h F: include/linux/bcm963xx_tag.h diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig index b3fe1d3..13a2b29 100644 --- a/drivers/pinctrl/Kconfig +++ b/drivers/pinctrl/Kconfig @@ -255,6 +255,7 @@ config PINCTRL_ZYNQ This selects the pinctrl driver for Xilinx Zynq. source "drivers/pinctrl/bcm/Kconfig" +source "drivers/pinctrl/bcm63xx/Kconfig" source "drivers/pinctrl/berlin/Kconfig" source "drivers/pinctrl/freescale/Kconfig" source "drivers/pinctrl/intel/Kconfig" diff --git a/drivers/pinctrl/Makefile b/drivers/pinctrl/Makefile index 8ebd7b8..366e574 100644 --- a/drivers/pinctrl/Makefile +++ b/drivers/pinctrl/Makefile @@ -38,6 +38,7 @@ obj-$(CONFIG_PINCTRL_ST) += pinctrl-st.o obj-$(CONFIG_PINCTRL_ZYNQ) += pinctrl-zynq.o obj-y += bcm/ +obj-y += bcm63xx/ obj-$(CONFIG_PINCTRL_BERLIN) += berlin/ obj-y += freescale/ obj-$(CONFIG_X86) += intel/ diff --git a/drivers/pinctrl/bcm63xx/Kconfig b/drivers/pinctrl/bcm63xx/Kconfig new file mode 100644 index 0000000..dd58f95 --- /dev/null +++ b/drivers/pinctrl/bcm63xx/Kconfig @@ -0,0 +1,3 @@ +config PINCTRL_BCM63XX + bool + select GPIO_GENERIC diff --git a/drivers/pinctrl/bcm63xx/Makefile b/drivers/pinctrl/bcm63xx/Makefile new file mode 100644 index 0000000..d16e74b --- /dev/null +++ b/drivers/pinctrl/bcm63xx/Makefile @@ -0,0 +1 @@ +obj-$(CONFIG_PINCTRL_BCM63XX) += pinctrl-bcm63xx.o diff --git a/drivers/pinctrl/bcm63xx/pinctrl-bcm63xx.c b/drivers/pinctrl/bcm63xx/pinctrl-bcm63xx.c new file mode 100644 index 0000000..e118171 --- /dev/null +++ b/drivers/pinctrl/bcm63xx/pinctrl-bcm63xx.c @@ -0,0 +1,141 @@ +/* + * This file is subject to the terms and conditions of the GNU General Public + * License. See the file "COPYING" in the main directory of this archive + * for more details. + * + * Copyright (C) 2016 Jonas Gorski <jonas.gorski@gmail.com> + */ + +#include <linux/bitops.h> +#include <linux/device.h> +#include <linux/gpio/driver.h> + +#include "pinctrl-bcm63xx.h" +#include "../core.h" + +#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 + +static int bcm63xx_setup_gpio(struct device *dev, struct gpio_chip *gc, + void __iomem *dirout, void __iomem *data, + size_t sz, int ngpio) + +{ + int banks, chips, i, ret = -EINVAL; + + chips = DIV_ROUND_UP(ngpio, PINS_PER_BANK); + banks = sz / BANK_SIZE; + + for (i = 0; i < chips; i++) { + int offset, pins; + int reg_offset; + char *label; + + label = devm_kasprintf(dev, GFP_KERNEL, "bcm63xx-gpio.%i", i); + if (!label) + return -ENOMEM; + + offset = i * PINS_PER_BANK; + pins = min_t(int, ngpio - offset, PINS_PER_BANK); + + /* the registers are treated like a huge big endian register */ + reg_offset = (banks - i - 1) * BANK_SIZE; + + ret = bgpio_init(&gc[i], dev, BANK_SIZE, data + reg_offset, + NULL, NULL, dirout + reg_offset, NULL, + BGPIOF_BIG_ENDIAN_BYTE_ORDER); + if (ret) + return ret; + + 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); + } + + return 0; +} + +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); + } +} + +struct pinctrl_dev *bcm63xx_pinctrl_register(struct platform_device *pdev, + struct pinctrl_desc *desc, + void *priv, struct gpio_chip *gc, + int ngpio) +{ + struct pinctrl_dev *pctldev; + struct resource *res; + void __iomem *dirout, *data; + size_t sz; + int ret; + + 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; +} diff --git a/drivers/pinctrl/bcm63xx/pinctrl-bcm63xx.h b/drivers/pinctrl/bcm63xx/pinctrl-bcm63xx.h new file mode 100644 index 0000000..9bfe1d0 --- /dev/null +++ b/drivers/pinctrl/bcm63xx/pinctrl-bcm63xx.h @@ -0,0 +1,14 @@ +#ifndef __PINCTRL_BCM63XX +#define __PINCTRL_BCM63XX + +#include <linux/kernel.h> +#include <linux/gpio.h> +#include <linux/pinctrl/pinctrl.h> +#include <linux/platform_device.h> + +struct pinctrl_dev *bcm63xx_pinctrl_register(struct platform_device *pdev, + struct pinctrl_desc *desc, + void *priv, struct gpio_chip *gc, + int ngpio); + +#endif
Setup directory and add a helper for bcm63xx pinctrl support. Signed-off-by: Jonas Gorski <jonas.gorski@gmail.com> --- MAINTAINERS | 1 + drivers/pinctrl/Kconfig | 1 + drivers/pinctrl/Makefile | 1 + drivers/pinctrl/bcm63xx/Kconfig | 3 + drivers/pinctrl/bcm63xx/Makefile | 1 + drivers/pinctrl/bcm63xx/pinctrl-bcm63xx.c | 141 ++++++++++++++++++++++++++++++ drivers/pinctrl/bcm63xx/pinctrl-bcm63xx.h | 14 +++ 7 files changed, 162 insertions(+) create mode 100644 drivers/pinctrl/bcm63xx/Kconfig create mode 100644 drivers/pinctrl/bcm63xx/Makefile create mode 100644 drivers/pinctrl/bcm63xx/pinctrl-bcm63xx.c create mode 100644 drivers/pinctrl/bcm63xx/pinctrl-bcm63xx.h