| Message ID | 1559927056-12064-1-git-send-email-hancock@sedsystems.ca |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [v3] gpio: xilinx: convert from OF GPIO to standard devm APIs | expand |
On 07. 06. 19 19:04, Robert Hancock wrote: > This driver was using the OF GPIO helper API, but barely used any of its > features and it cost more code than it saved. Also, the OF GPIO code is > now deprecated. Convert it to use a more standard setup and use devm > APIs for initialization to avoid the need for a remove function. > > Our rationale for this change is that we are using the Xilinx GPIO with > resources injected using the MFD core rather than on the device tree > itself. Using platform rather than OF-specific resources allows this to > work for free. > > Signed-off-by: Robert Hancock <hancock@sedsystems.ca> > --- > > Changed from v2: Cleaned up probe error message code > > drivers/gpio/Kconfig | 1 - > drivers/gpio/gpio-xilinx.c | 90 +++++++++++++++++++--------------------------- > 2 files changed, 36 insertions(+), 55 deletions(-) > > diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig > index acd40eb..66f1f13 100644 > --- a/drivers/gpio/Kconfig > +++ b/drivers/gpio/Kconfig > @@ -602,7 +602,6 @@ config GPIO_XGENE_SB > > config GPIO_XILINX > tristate "Xilinx GPIO support" > - depends on OF_GPIO > help > Say yes here to support the Xilinx FPGA GPIO device > > diff --git a/drivers/gpio/gpio-xilinx.c b/drivers/gpio/gpio-xilinx.c > index 32944eb..a9748b5 100644 > --- a/drivers/gpio/gpio-xilinx.c > +++ b/drivers/gpio/gpio-xilinx.c > @@ -11,7 +11,6 @@ > #include <linux/module.h> > #include <linux/of_device.h> > #include <linux/of_platform.h> > -#include <linux/of_gpio.h> > #include <linux/io.h> > #include <linux/gpio/driver.h> > #include <linux/slab.h> > @@ -33,14 +32,16 @@ > > /** > * struct xgpio_instance - Stores information about GPIO device > - * @mmchip: OF GPIO chip for memory mapped banks > + * @gc: GPIO chip > + * @regs: register block > * @gpio_width: GPIO width for every channel > * @gpio_state: GPIO state shadow register > * @gpio_dir: GPIO direction shadow register > * @gpio_lock: Lock used for synchronization > */ > struct xgpio_instance { > - struct of_mm_gpio_chip mmchip; > + struct gpio_chip gc; > + void __iomem *regs; > unsigned int gpio_width[2]; > u32 gpio_state[2]; > u32 gpio_dir[2]; > @@ -84,11 +85,10 @@ static inline int xgpio_offset(struct xgpio_instance *chip, int gpio) > */ > static int xgpio_get(struct gpio_chip *gc, unsigned int gpio) > { > - struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc); > struct xgpio_instance *chip = gpiochip_get_data(gc); > u32 val; > > - val = xgpio_readreg(mm_gc->regs + XGPIO_DATA_OFFSET + > + val = xgpio_readreg(chip->regs + XGPIO_DATA_OFFSET + > xgpio_regoffset(chip, gpio)); > > return !!(val & BIT(xgpio_offset(chip, gpio))); > @@ -106,7 +106,6 @@ static int xgpio_get(struct gpio_chip *gc, unsigned int gpio) > static void xgpio_set(struct gpio_chip *gc, unsigned int gpio, int val) > { > unsigned long flags; > - struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc); > struct xgpio_instance *chip = gpiochip_get_data(gc); > int index = xgpio_index(chip, gpio); > int offset = xgpio_offset(chip, gpio); > @@ -119,7 +118,7 @@ static void xgpio_set(struct gpio_chip *gc, unsigned int gpio, int val) > else > chip->gpio_state[index] &= ~BIT(offset); > > - xgpio_writereg(mm_gc->regs + XGPIO_DATA_OFFSET + > + xgpio_writereg(chip->regs + XGPIO_DATA_OFFSET + > xgpio_regoffset(chip, gpio), chip->gpio_state[index]); > > spin_unlock_irqrestore(&chip->gpio_lock[index], flags); > @@ -138,7 +137,6 @@ static void xgpio_set_multiple(struct gpio_chip *gc, unsigned long *mask, > unsigned long *bits) > { > unsigned long flags; > - struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc); > struct xgpio_instance *chip = gpiochip_get_data(gc); > int index = xgpio_index(chip, 0); > int offset, i; > @@ -150,7 +148,7 @@ static void xgpio_set_multiple(struct gpio_chip *gc, unsigned long *mask, > if (*mask == 0) > break; > if (index != xgpio_index(chip, i)) { > - xgpio_writereg(mm_gc->regs + XGPIO_DATA_OFFSET + > + xgpio_writereg(chip->regs + XGPIO_DATA_OFFSET + > xgpio_regoffset(chip, i), > chip->gpio_state[index]); > spin_unlock_irqrestore(&chip->gpio_lock[index], flags); > @@ -166,7 +164,7 @@ static void xgpio_set_multiple(struct gpio_chip *gc, unsigned long *mask, > } > } > > - xgpio_writereg(mm_gc->regs + XGPIO_DATA_OFFSET + > + xgpio_writereg(chip->regs + XGPIO_DATA_OFFSET + > xgpio_regoffset(chip, i), chip->gpio_state[index]); > > spin_unlock_irqrestore(&chip->gpio_lock[index], flags); > @@ -184,7 +182,6 @@ static void xgpio_set_multiple(struct gpio_chip *gc, unsigned long *mask, > static int xgpio_dir_in(struct gpio_chip *gc, unsigned int gpio) > { > unsigned long flags; > - struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc); > struct xgpio_instance *chip = gpiochip_get_data(gc); > int index = xgpio_index(chip, gpio); > int offset = xgpio_offset(chip, gpio); > @@ -193,7 +190,7 @@ static int xgpio_dir_in(struct gpio_chip *gc, unsigned int gpio) > > /* Set the GPIO bit in shadow register and set direction as input */ > chip->gpio_dir[index] |= BIT(offset); > - xgpio_writereg(mm_gc->regs + XGPIO_TRI_OFFSET + > + xgpio_writereg(chip->regs + XGPIO_TRI_OFFSET + > xgpio_regoffset(chip, gpio), chip->gpio_dir[index]); > > spin_unlock_irqrestore(&chip->gpio_lock[index], flags); > @@ -216,7 +213,6 @@ static int xgpio_dir_in(struct gpio_chip *gc, unsigned int gpio) > static int xgpio_dir_out(struct gpio_chip *gc, unsigned int gpio, int val) > { > unsigned long flags; > - struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc); > struct xgpio_instance *chip = gpiochip_get_data(gc); > int index = xgpio_index(chip, gpio); > int offset = xgpio_offset(chip, gpio); > @@ -228,12 +224,12 @@ static int xgpio_dir_out(struct gpio_chip *gc, unsigned int gpio, int val) > chip->gpio_state[index] |= BIT(offset); > else > chip->gpio_state[index] &= ~BIT(offset); > - xgpio_writereg(mm_gc->regs + XGPIO_DATA_OFFSET + > + xgpio_writereg(chip->regs + XGPIO_DATA_OFFSET + > xgpio_regoffset(chip, gpio), chip->gpio_state[index]); > > /* Clear the GPIO bit in shadow register and set direction as output */ > chip->gpio_dir[index] &= ~BIT(offset); > - xgpio_writereg(mm_gc->regs + XGPIO_TRI_OFFSET + > + xgpio_writereg(chip->regs + XGPIO_TRI_OFFSET + > xgpio_regoffset(chip, gpio), chip->gpio_dir[index]); > > spin_unlock_irqrestore(&chip->gpio_lock[index], flags); > @@ -243,43 +239,23 @@ static int xgpio_dir_out(struct gpio_chip *gc, unsigned int gpio, int val) > > /** > * xgpio_save_regs - Set initial values of GPIO pins > - * @mm_gc: Pointer to memory mapped GPIO chip structure > + * @chip: Pointer to GPIO instance > */ > -static void xgpio_save_regs(struct of_mm_gpio_chip *mm_gc) > +static void xgpio_save_regs(struct xgpio_instance *chip) > { > - struct xgpio_instance *chip = > - container_of(mm_gc, struct xgpio_instance, mmchip); > - > - xgpio_writereg(mm_gc->regs + XGPIO_DATA_OFFSET, chip->gpio_state[0]); > - xgpio_writereg(mm_gc->regs + XGPIO_TRI_OFFSET, chip->gpio_dir[0]); > + xgpio_writereg(chip->regs + XGPIO_DATA_OFFSET, chip->gpio_state[0]); > + xgpio_writereg(chip->regs + XGPIO_TRI_OFFSET, chip->gpio_dir[0]); > > if (!chip->gpio_width[1]) > return; > > - xgpio_writereg(mm_gc->regs + XGPIO_DATA_OFFSET + XGPIO_CHANNEL_OFFSET, > + xgpio_writereg(chip->regs + XGPIO_DATA_OFFSET + XGPIO_CHANNEL_OFFSET, > chip->gpio_state[1]); > - xgpio_writereg(mm_gc->regs + XGPIO_TRI_OFFSET + XGPIO_CHANNEL_OFFSET, > + xgpio_writereg(chip->regs + XGPIO_TRI_OFFSET + XGPIO_CHANNEL_OFFSET, > chip->gpio_dir[1]); > } > > /** > - * xgpio_remove - Remove method for the GPIO device. > - * @pdev: pointer to the platform device > - * > - * This function remove gpiochips and frees all the allocated resources. > - * > - * Return: 0 always > - */ > -static int xgpio_remove(struct platform_device *pdev) > -{ > - struct xgpio_instance *chip = platform_get_drvdata(pdev); > - > - of_mm_gpiochip_remove(&chip->mmchip); > - > - return 0; > -} > - > -/** > * xgpio_of_probe - Probe method for the GPIO device. > * @pdev: pointer to the platform device > * > @@ -340,21 +316,28 @@ static int xgpio_probe(struct platform_device *pdev) > spin_lock_init(&chip->gpio_lock[1]); > } > > - chip->mmchip.gc.ngpio = chip->gpio_width[0] + chip->gpio_width[1]; > - chip->mmchip.gc.parent = &pdev->dev; > - chip->mmchip.gc.direction_input = xgpio_dir_in; > - chip->mmchip.gc.direction_output = xgpio_dir_out; > - chip->mmchip.gc.get = xgpio_get; > - chip->mmchip.gc.set = xgpio_set; > - chip->mmchip.gc.set_multiple = xgpio_set_multiple; > + chip->gc.base = -1; > + chip->gc.ngpio = chip->gpio_width[0] + chip->gpio_width[1]; > + chip->gc.parent = &pdev->dev; > + chip->gc.direction_input = xgpio_dir_in; > + chip->gc.direction_output = xgpio_dir_out; > + chip->gc.get = xgpio_get; > + chip->gc.set = xgpio_set; > + chip->gc.set_multiple = xgpio_set_multiple; > + > + chip->gc.label = dev_name(&pdev->dev); > + > + chip->regs = devm_platform_ioremap_resource(pdev, 0); > + if (IS_ERR(chip->regs)) { > + dev_err(&pdev->dev, "failed to ioremap memory resource\n"); > + return PTR_ERR(chip->regs); > + } > > - chip->mmchip.save_regs = xgpio_save_regs; > + xgpio_save_regs(chip); > > - /* Call the OF gpio helper to setup and register the GPIO device */ > - status = of_mm_gpiochip_add_data(np, &chip->mmchip, chip); > + status = devm_gpiochip_add_data(&pdev->dev, &chip->gc, chip); > if (status) { > - pr_err("%pOF: error in probe function with status %d\n", > - np, status); > + dev_err(&pdev->dev, "failed to add GPIO chip\n"); > return status; > } > > @@ -370,7 +353,6 @@ static int xgpio_probe(struct platform_device *pdev) > > static struct platform_driver xgpio_plat_driver = { > .probe = xgpio_probe, > - .remove = xgpio_remove, > .driver = { > .name = "gpio-xilinx", > .of_match_table = xgpio_of_match, > Looks good. Acked-by: Michal Simek <michal.simek@xilinx.com> Thanks, Michal
On Fri, Jun 7, 2019 at 7:04 PM Robert Hancock <hancock@sedsystems.ca> wrote: > This driver was using the OF GPIO helper API, but barely used any of its > features and it cost more code than it saved. Also, the OF GPIO code is > now deprecated. Convert it to use a more standard setup and use devm > APIs for initialization to avoid the need for a remove function. > > Our rationale for this change is that we are using the Xilinx GPIO with > resources injected using the MFD core rather than on the device tree > itself. Using platform rather than OF-specific resources allows this to > work for free. > > Signed-off-by: Robert Hancock <hancock@sedsystems.ca> Patch applied with Michal's ACK. Yours, Linus Walleij
diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig index acd40eb..66f1f13 100644 --- a/drivers/gpio/Kconfig +++ b/drivers/gpio/Kconfig @@ -602,7 +602,6 @@ config GPIO_XGENE_SB config GPIO_XILINX tristate "Xilinx GPIO support" - depends on OF_GPIO help Say yes here to support the Xilinx FPGA GPIO device diff --git a/drivers/gpio/gpio-xilinx.c b/drivers/gpio/gpio-xilinx.c index 32944eb..a9748b5 100644 --- a/drivers/gpio/gpio-xilinx.c +++ b/drivers/gpio/gpio-xilinx.c @@ -11,7 +11,6 @@ #include <linux/module.h> #include <linux/of_device.h> #include <linux/of_platform.h> -#include <linux/of_gpio.h> #include <linux/io.h> #include <linux/gpio/driver.h> #include <linux/slab.h> @@ -33,14 +32,16 @@ /** * struct xgpio_instance - Stores information about GPIO device - * @mmchip: OF GPIO chip for memory mapped banks + * @gc: GPIO chip + * @regs: register block * @gpio_width: GPIO width for every channel * @gpio_state: GPIO state shadow register * @gpio_dir: GPIO direction shadow register * @gpio_lock: Lock used for synchronization */ struct xgpio_instance { - struct of_mm_gpio_chip mmchip; + struct gpio_chip gc; + void __iomem *regs; unsigned int gpio_width[2]; u32 gpio_state[2]; u32 gpio_dir[2]; @@ -84,11 +85,10 @@ static inline int xgpio_offset(struct xgpio_instance *chip, int gpio) */ static int xgpio_get(struct gpio_chip *gc, unsigned int gpio) { - struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc); struct xgpio_instance *chip = gpiochip_get_data(gc); u32 val; - val = xgpio_readreg(mm_gc->regs + XGPIO_DATA_OFFSET + + val = xgpio_readreg(chip->regs + XGPIO_DATA_OFFSET + xgpio_regoffset(chip, gpio)); return !!(val & BIT(xgpio_offset(chip, gpio))); @@ -106,7 +106,6 @@ static int xgpio_get(struct gpio_chip *gc, unsigned int gpio) static void xgpio_set(struct gpio_chip *gc, unsigned int gpio, int val) { unsigned long flags; - struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc); struct xgpio_instance *chip = gpiochip_get_data(gc); int index = xgpio_index(chip, gpio); int offset = xgpio_offset(chip, gpio); @@ -119,7 +118,7 @@ static void xgpio_set(struct gpio_chip *gc, unsigned int gpio, int val) else chip->gpio_state[index] &= ~BIT(offset); - xgpio_writereg(mm_gc->regs + XGPIO_DATA_OFFSET + + xgpio_writereg(chip->regs + XGPIO_DATA_OFFSET + xgpio_regoffset(chip, gpio), chip->gpio_state[index]); spin_unlock_irqrestore(&chip->gpio_lock[index], flags); @@ -138,7 +137,6 @@ static void xgpio_set_multiple(struct gpio_chip *gc, unsigned long *mask, unsigned long *bits) { unsigned long flags; - struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc); struct xgpio_instance *chip = gpiochip_get_data(gc); int index = xgpio_index(chip, 0); int offset, i; @@ -150,7 +148,7 @@ static void xgpio_set_multiple(struct gpio_chip *gc, unsigned long *mask, if (*mask == 0) break; if (index != xgpio_index(chip, i)) { - xgpio_writereg(mm_gc->regs + XGPIO_DATA_OFFSET + + xgpio_writereg(chip->regs + XGPIO_DATA_OFFSET + xgpio_regoffset(chip, i), chip->gpio_state[index]); spin_unlock_irqrestore(&chip->gpio_lock[index], flags); @@ -166,7 +164,7 @@ static void xgpio_set_multiple(struct gpio_chip *gc, unsigned long *mask, } } - xgpio_writereg(mm_gc->regs + XGPIO_DATA_OFFSET + + xgpio_writereg(chip->regs + XGPIO_DATA_OFFSET + xgpio_regoffset(chip, i), chip->gpio_state[index]); spin_unlock_irqrestore(&chip->gpio_lock[index], flags); @@ -184,7 +182,6 @@ static void xgpio_set_multiple(struct gpio_chip *gc, unsigned long *mask, static int xgpio_dir_in(struct gpio_chip *gc, unsigned int gpio) { unsigned long flags; - struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc); struct xgpio_instance *chip = gpiochip_get_data(gc); int index = xgpio_index(chip, gpio); int offset = xgpio_offset(chip, gpio); @@ -193,7 +190,7 @@ static int xgpio_dir_in(struct gpio_chip *gc, unsigned int gpio) /* Set the GPIO bit in shadow register and set direction as input */ chip->gpio_dir[index] |= BIT(offset); - xgpio_writereg(mm_gc->regs + XGPIO_TRI_OFFSET + + xgpio_writereg(chip->regs + XGPIO_TRI_OFFSET + xgpio_regoffset(chip, gpio), chip->gpio_dir[index]); spin_unlock_irqrestore(&chip->gpio_lock[index], flags); @@ -216,7 +213,6 @@ static int xgpio_dir_in(struct gpio_chip *gc, unsigned int gpio) static int xgpio_dir_out(struct gpio_chip *gc, unsigned int gpio, int val) { unsigned long flags; - struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc); struct xgpio_instance *chip = gpiochip_get_data(gc); int index = xgpio_index(chip, gpio); int offset = xgpio_offset(chip, gpio); @@ -228,12 +224,12 @@ static int xgpio_dir_out(struct gpio_chip *gc, unsigned int gpio, int val) chip->gpio_state[index] |= BIT(offset); else chip->gpio_state[index] &= ~BIT(offset); - xgpio_writereg(mm_gc->regs + XGPIO_DATA_OFFSET + + xgpio_writereg(chip->regs + XGPIO_DATA_OFFSET + xgpio_regoffset(chip, gpio), chip->gpio_state[index]); /* Clear the GPIO bit in shadow register and set direction as output */ chip->gpio_dir[index] &= ~BIT(offset); - xgpio_writereg(mm_gc->regs + XGPIO_TRI_OFFSET + + xgpio_writereg(chip->regs + XGPIO_TRI_OFFSET + xgpio_regoffset(chip, gpio), chip->gpio_dir[index]); spin_unlock_irqrestore(&chip->gpio_lock[index], flags); @@ -243,43 +239,23 @@ static int xgpio_dir_out(struct gpio_chip *gc, unsigned int gpio, int val) /** * xgpio_save_regs - Set initial values of GPIO pins - * @mm_gc: Pointer to memory mapped GPIO chip structure + * @chip: Pointer to GPIO instance */ -static void xgpio_save_regs(struct of_mm_gpio_chip *mm_gc) +static void xgpio_save_regs(struct xgpio_instance *chip) { - struct xgpio_instance *chip = - container_of(mm_gc, struct xgpio_instance, mmchip); - - xgpio_writereg(mm_gc->regs + XGPIO_DATA_OFFSET, chip->gpio_state[0]); - xgpio_writereg(mm_gc->regs + XGPIO_TRI_OFFSET, chip->gpio_dir[0]); + xgpio_writereg(chip->regs + XGPIO_DATA_OFFSET, chip->gpio_state[0]); + xgpio_writereg(chip->regs + XGPIO_TRI_OFFSET, chip->gpio_dir[0]); if (!chip->gpio_width[1]) return; - xgpio_writereg(mm_gc->regs + XGPIO_DATA_OFFSET + XGPIO_CHANNEL_OFFSET, + xgpio_writereg(chip->regs + XGPIO_DATA_OFFSET + XGPIO_CHANNEL_OFFSET, chip->gpio_state[1]); - xgpio_writereg(mm_gc->regs + XGPIO_TRI_OFFSET + XGPIO_CHANNEL_OFFSET, + xgpio_writereg(chip->regs + XGPIO_TRI_OFFSET + XGPIO_CHANNEL_OFFSET, chip->gpio_dir[1]); } /** - * xgpio_remove - Remove method for the GPIO device. - * @pdev: pointer to the platform device - * - * This function remove gpiochips and frees all the allocated resources. - * - * Return: 0 always - */ -static int xgpio_remove(struct platform_device *pdev) -{ - struct xgpio_instance *chip = platform_get_drvdata(pdev); - - of_mm_gpiochip_remove(&chip->mmchip); - - return 0; -} - -/** * xgpio_of_probe - Probe method for the GPIO device. * @pdev: pointer to the platform device * @@ -340,21 +316,28 @@ static int xgpio_probe(struct platform_device *pdev) spin_lock_init(&chip->gpio_lock[1]); } - chip->mmchip.gc.ngpio = chip->gpio_width[0] + chip->gpio_width[1]; - chip->mmchip.gc.parent = &pdev->dev; - chip->mmchip.gc.direction_input = xgpio_dir_in; - chip->mmchip.gc.direction_output = xgpio_dir_out; - chip->mmchip.gc.get = xgpio_get; - chip->mmchip.gc.set = xgpio_set; - chip->mmchip.gc.set_multiple = xgpio_set_multiple; + chip->gc.base = -1; + chip->gc.ngpio = chip->gpio_width[0] + chip->gpio_width[1]; + chip->gc.parent = &pdev->dev; + chip->gc.direction_input = xgpio_dir_in; + chip->gc.direction_output = xgpio_dir_out; + chip->gc.get = xgpio_get; + chip->gc.set = xgpio_set; + chip->gc.set_multiple = xgpio_set_multiple; + + chip->gc.label = dev_name(&pdev->dev); + + chip->regs = devm_platform_ioremap_resource(pdev, 0); + if (IS_ERR(chip->regs)) { + dev_err(&pdev->dev, "failed to ioremap memory resource\n"); + return PTR_ERR(chip->regs); + } - chip->mmchip.save_regs = xgpio_save_regs; + xgpio_save_regs(chip); - /* Call the OF gpio helper to setup and register the GPIO device */ - status = of_mm_gpiochip_add_data(np, &chip->mmchip, chip); + status = devm_gpiochip_add_data(&pdev->dev, &chip->gc, chip); if (status) { - pr_err("%pOF: error in probe function with status %d\n", - np, status); + dev_err(&pdev->dev, "failed to add GPIO chip\n"); return status; } @@ -370,7 +353,6 @@ static int xgpio_probe(struct platform_device *pdev) static struct platform_driver xgpio_plat_driver = { .probe = xgpio_probe, - .remove = xgpio_remove, .driver = { .name = "gpio-xilinx", .of_match_table = xgpio_of_match,
This driver was using the OF GPIO helper API, but barely used any of its features and it cost more code than it saved. Also, the OF GPIO code is now deprecated. Convert it to use a more standard setup and use devm APIs for initialization to avoid the need for a remove function. Our rationale for this change is that we are using the Xilinx GPIO with resources injected using the MFD core rather than on the device tree itself. Using platform rather than OF-specific resources allows this to work for free. Signed-off-by: Robert Hancock <hancock@sedsystems.ca> --- Changed from v2: Cleaned up probe error message code drivers/gpio/Kconfig | 1 - drivers/gpio/gpio-xilinx.c | 90 +++++++++++++++++++--------------------------- 2 files changed, 36 insertions(+), 55 deletions(-)