gpio: Renesas RZ GPIO driver
PATCH 1/1 gpio: Renesas RZ GPIO driver
From: Magnus Damm <[email protected]>
This patch adds a GPIO driver for the RZ series of SoCs from Renesas. The driver can be used as platform device with dynamic or static GPIO assignment or via DT using dynamic GPIOs.
The hardware allows control of GPIOs in blocks of up to 16 pins, and once device may span multiple blocks. Interrupts are not included in this hardware block, if interrupts are needed then the PFC needs to be configured to a IRQ pin function which is handled by the GIC hardware.
Tested with yet-to-be-posted platform device and DT devices on r7s72100 and Genmai using LEDs, DIP switches and I2C bitbang.
---
drivers/gpio/Kconfig | 6 drivers/gpio/Makefile | 1 drivers/gpio/gpio-rz.c | 241 +++++++++++++++++++++++++++++++++ include/linux/platform_data/gpio-rz.h | 13 + 4 files changed, 261 insertions(+)
drivers/gpio/Kconfig +6 -0
@@ -230,6 +230,12 @@ config GPIO_RCAR help Say yes here to support GPIO on Renesas R-Car SoCs. +config GPIO_RZ + tristate "Renesas RZ GPIO" + depends on ARM + help + Say yes here to support GPIO on Renesas RZ SoCs. + config GPIO_SAMSUNG bool depends on PLAT_SAMSUNG
drivers/gpio/Makefile +1 -0
@@ -62,6 +62,7 @@ obj-$(CONFIG_GPIO_PXA) += gpio-pxa.o obj-$(CONFIG_GPIO_RC5T583) += gpio-rc5t583.o obj-$(CONFIG_GPIO_RDC321X) += gpio-rdc321x.o obj-$(CONFIG_GPIO_RCAR) += gpio-rcar.o +obj-$(CONFIG_GPIO_RZ) += gpio-rz.o obj-$(CONFIG_GPIO_SAMSUNG) += gpio-samsung.o obj-$(CONFIG_ARCH_SA1100) += gpio-sa1100.o obj-$(CONFIG_GPIO_SCH) += gpio-sch.o
drivers/gpio/gpio-rz.c new file +241 -0
241 lines not on this page · raw message
include/linux/platform_data/gpio-rz.h new file +13 -0
@@ -0,0 +1,13 @@ +#ifndef __GPIO_RZ_H__ +#define __GPIO_RZ_H__ + +struct gpio_rz_config { +int gpio_base; +const char *pctl_name; +}; + +#define RZ_GPIOS_PER_PORT 16 + +#define RZ_PORT_PIN (bank, pin) (((bank) * RZ_GPIOS_PER_PORT) + (pin)) + +#endif /* __GPIO_RZ_H__ */
Quoted 5 lines · Magnus Damm
From: Magnus Damm <[email protected]>
This patch adds a GPIO driver for the RZ series of SoCs from Renesas. The driver can be used as platform device with dynamic or static GPIO assignment or via DT using dynamic GPIOs.
So given that this is for a new system which should only ever be booted using device tree, why are we bothering with supporting platform data passing at all?
Is it so that arch/sh is more soft on this for example...? Can some arch maintainer like SH/Paul ACK this approach?
Read: SH is not moving to device tree...?
(...)
Quoted 2 lines
Tested with yet-to-be-posted platform device and DT devices on r7s72100 and Genmai using LEDs, DIP switches and I2C bitbang.
Do you think the maintainers will merge the platform device approach?
include/linux/platform_data/gpio-rz.h 2013-11-06 14:18:46.000000000 +0900 line 5
Quoted 8 lines
include/linux/platform_data/gpio-rz.h 2013-11-06 14:18:46.000000000 +0900 new file +5
-0@@ -0,0 +1,13 @@ +#ifndef __GPIO_RZ_H__ +#define __GPIO_RZ_H__ + +struct gpio_rz_config { + int gpio_base;
Passing these static base offsets around is not good for the kernel and we're trying to get rid of it :-(
include/linux/platform_data/gpio-rz.h 2013-11-06 14:18:46.000000000 +0900 line 6
Quoted 1 line
include/linux/platform_data/gpio-rz.h 2013-11-06 14:18:46.000000000 +0900 new file +1
-0+ const char *pctl_name;
Ho hum... This needs some kerneldoc describing that this is used to map the GPIO range to the right pin controller.
include/linux/platform_data/gpio-rz.h 2013-11-06 14:18:46.000000000 +0900 line 9
Quoted 3 lines
include/linux/platform_data/gpio-rz.h 2013-11-06 14:18:46.000000000 +0900 new file +3
-0+}; + +#define RZ_GPIOS_PER_PORT 16
This is only used in the driver so move it into the driver.
include/linux/platform_data/gpio-rz.h 2013-11-06 14:18:46.000000000 +0900 line 10
Quoted 1 line
include/linux/platform_data/gpio-rz.h 2013-11-06 14:18:46.000000000 +0900 new file +1
-0+#define RZ_PORT_PIN(bank, pin) (((bank) * RZ_GPIOS_PER_PORT) + (pin))
This is not used anywhere so delete it.
If it is to be kept I'd like "pin" replaced with "line" to avoid confusion with the pin control business.
Yours,
Linus Walleij
Hi Linus,
Quoted 11 lines · Linus Walleij
Quoted 5 lines · Magnus Damm
From: Magnus Damm <[email protected]>
This patch adds a GPIO driver for the RZ series of SoCs from Renesas. The driver can be used as platform device with dynamic or static GPIO assignment or via DT using dynamic GPIOs.
So given that this is for a new system which should only ever be booted using device tree, why are we bothering with supporting platform data passing at all?
Mainly to support the same interfaces as our other GPIO drivers. But I can easily remove the platform data init method if that is the preferred way, no problem.
Quoted 4 lines
Is it so that
arch/shis more soft on this for example...? Can some arch maintainer like SH/Paul ACK this approach?Read: SH is not moving to device tree...?
From what I can tell this GPIO block is not used with SH, so I don't think SH is related, but regarding DT on SH, do you know when it was decided that other architectures also were supposed to move DT?
Quoted 6 lines
(...)
Quoted 2 lines
Tested with yet-to-be-posted platform device and DT devices on r7s72100 and Genmai using LEDs, DIP switches and I2C bitbang.
Do you think the maintainers will merge the platform device approach?
I would not assume so. But the goal with these patches is not upstream, instead they basically serve as a stop-gap solution between now and when I get OK that the DT bits in this GPIO driver looks fine. If they are going to be merged or not is a different question IMO.
Quoted 11 lines
include/linux/platform_data/gpio-rz.h 2013-11-06 14:18:46.000000000 +0900 line 5
Quoted 8 lines
include/linux/platform_data/gpio-rz.h 2013-11-06 14:18:46.000000000 +0900 new file +5
-0@@ -0,0 +1,13 @@ +#ifndef __GPIO_RZ_H__ +#define __GPIO_RZ_H__ + +struct gpio_rz_config { + int gpio_base;Passing these static base offsets around is not good for the kernel and we're trying to get rid of it :-(
Sure.
Quoted 4 lines
include/linux/platform_data/gpio-rz.h 2013-11-06 14:18:46.000000000 +0900 line 6
Quoted 1 line
include/linux/platform_data/gpio-rz.h 2013-11-06 14:18:46.000000000 +0900 new file +1
-0+ const char *pctl_name;Ho hum... This needs some kerneldoc describing that this is used to map the GPIO range to the right pin controller.
Ok, but it will just go away when the platform data init method is removed
Quoted 3 lines
include/linux/platform_data/gpio-rz.h 2013-11-06 14:18:46.000000000 +0900 line 7
Quoted 1 line
include/linux/platform_data/gpio-rz.h 2013-11-06 14:18:46.000000000 +0900 new file +1
-0+#define RZ_GPIOS_PER_PORT 16This is only used in the driver so move it into the driver.
It is also used by the macro below. =)
Quoted 6 lines
include/linux/platform_data/gpio-rz.h 2013-11-06 14:18:46.000000000 +0900 line 8
Quoted 1 line
include/linux/platform_data/gpio-rz.h 2013-11-06 14:18:46.000000000 +0900 new file +1
-0+#define RZ_PORT_PIN(bank, pin) (((bank) * RZ_GPIOS_PER_PORT) + (pin))This is not used anywhere so delete it.
If it is to be kept I'd like "pin" replaced with "line" to avoid confusion with the pin control business.
The idea with that macro is to allow board code to select which pin from which bank, but I realize it may clash with pinctrl terminology. Also, since the only reason for this header file is to provide a platform data init interface for the driver all these things will go away if we go DT-only.
I'll ditch the platform data interface and post a V2.
Thanks for your feedback!
Cheers,
/ magnus
Quoted 11 lines · Magnus Damm
Quoted 6 lines · Linus Walleij
On Thu, Nov 7, 2013 at 12:47 AM, Magnus Damm <[email protected]> wrote:
Is it so that
arch/shis more soft on this for example...? Can some arch maintainer like SH/Paul ACK this approach?Read: SH is not moving to device tree...?
From what I can tell this GPIO block is not used with SH, so I don't think SH is related, but regarding DT on SH, do you know when it was decided that other architectures also were supposed to move DT?
I don't think these is any such decision, I'm just asking. I know that we only want to see DT on new archs and old hairy board code should be ridded using DT ... So I just want to know what the situation is like wrt Super-H.
Quoted 10 lines
Quoted 5 lines
Quoted 2 lines
Tested with yet-to-be-posted platform device and DT devices on r7s72100 and Genmai using LEDs, DIP switches and I2C bitbang.
Do you think the maintainers will merge the platform device approach?
I would not assume so. But the goal with these patches is not upstream, instead they basically serve as a stop-gap solution between now and when I get OK that the DT bits in this GPIO driver looks fine. If they are going to be merged or not is a different question IMO.
Um so as an upstream maintainer I don't really know what to do at this point :-)
But a DT-only driver is uncontroversial so I'd merge that.
Quoted 1 line
I'll ditch the platform data interface and post a V2.
Okay that sounds good...
Thanks,
Linus Walleij
Quoted 9 lines · Linus Walleij
Quoted 8 lines · Magnus Damm
Quoted 6 lines · Linus Walleij
On Thu, Nov 7, 2013 at 12:47 AM, Magnus Damm <[email protected]> wrote:
Is it so that
arch/shis more soft on this for example...? Can some arch maintainer like SH/Paul ACK this approach?Read: SH is not moving to device tree...?
Quoted 8 lines
Quoted 3 lines
From what I can tell this GPIO block is not used with SH, so I don't think SH is related, but regarding DT on SH, do you know when it was decided that other architectures also were supposed to move DT?
I don't think these is any such decision, I'm just asking. I know that we only want to see DT on new archs and old hairy board code should be ridded using DT ... So I just want to know what the situation is like wrt Super-H.
I think Paul as the arch/sh maintainer has made it very clear in the past that he is not interested in converting the architecture.
IMHO that makes sense because the current code is working well and there won't be new SoCs that need to get added to it. While there is a set of drivers that are shared with arm/shmobile, that set is known and we can handle each driver individually. In some cases we actually end up having two completely different drivers for the same peripheral (e.g. some interrupt controllers), something we normally try very hard to avoid but that seems appropriate here. Let's just not make it a common theme for other drivers that are not in this situation.
One thing that makes arch/sh and traditionally mach-shmobile different from everything else is a very sophisticated way for describing platforms in an abstract way using C data structures. It's not necessarily bad, but it's inconsistent with other drivers we have on ARM, and it's to a large degree incompatible to how we use DT to describe the same hardware.
Arnd
Hi Magnus,
Thank you for the patch.
Please read below for a couple of comments in addition to Linus' review.
Quoted 69 lines · On Thursday 07 November 2013 08:47:37 Magnus Damm wrote:
From: Magnus Damm <[email protected]>
This patch adds a GPIO driver for the RZ series of SoCs from Renesas. The driver can be used as platform device with dynamic or static GPIO assignment or via DT using dynamic GPIOs.
The hardware allows control of GPIOs in blocks of up to 16 pins, and once device may span multiple blocks. Interrupts are not included in this hardware block, if interrupts are needed then the PFC needs to be configured to a IRQ pin function which is handled by the GIC hardware.
Tested with yet-to-be-posted platform device and DT devices on r7s72100 and Genmai using LEDs, DIP switches and I2C bitbang.
Signed-off-by Magnus Damm <[email protected]>---
drivers/gpio/Kconfig | 6 drivers/gpio/Makefile | 1 drivers/gpio/gpio-rz.c | 241 ++++++++++++++++++++++++++++++ include/linux/platform_data/gpio-rz.h | 13 + 4 files changed, 261 insertions(+)drivers/gpio/Kconfig +6
-0@@ -230,6 +230,12 @@ config GPIO_RCAR help Say yes here to support GPIO on Renesas R-Car SoCs. +config GPIO_RZ + tristate "Renesas RZ GPIO" + depends on ARM + help + Say yes here to support GPIO on Renesas RZ SoCs. + config GPIO_SAMSUNG bool depends on PLAT_SAMSUNGdrivers/gpio/Makefile +1
-0@@ -62,6 +62,7 @@ obj-$(CONFIG_GPIO_PXA) += gpio-pxa.o obj-$(CONFIG_GPIO_RC5T583) += gpio-rc5t583.o obj-$(CONFIG_GPIO_RDC321X) += gpio-rdc321x.o obj-$(CONFIG_GPIO_RCAR) += gpio-rcar.o +obj-$(CONFIG_GPIO_RZ) += gpio-rz.o obj-$(CONFIG_GPIO_SAMSUNG) += gpio-samsung.o obj-$(CONFIG_ARCH_SA1100) += gpio-sa1100.o obj-$(CONFIG_GPIO_SCH) += gpio-sch.odrivers/gpio/gpio-rz.c new file +17
-0@@ -0,0 +1,241 @@ +/* +* RZ GPIO Support - Ports +* +* Copyright (C) 2013 Magnus Damm +* +* This program is free software; you can redistribute it and/or modify +* it under the terms of the GNU General Public License as published by +* the Free Software Foundation; either version 2 of the License +* +* This program is distributed in the hope that it will be useful, +* but WITHOUT ANY WARRANTY; without even the implied warranty of +* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +* GNU General Public License for more details. +* +* You should have received a copy of the GNU General Public License +* along with this program; if not, write to the Free Software +* Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307
Quoted 2 lines
USA
+ */
You can ditch the last two paragraphs.
Quoted 4 lines
drivers/gpio/gpio-rz.c new file +4
-0+ +#include <linux/init.h> +#include <linux/platform_device.h> +#include <linux/spinlock.h>
drivers/gpio/gpio-rz.c line 32
Quoted 10 lines
drivers/gpio/gpio-rz.c new file +10
-0+#include <linux/interrupt.h> +#include <linux/ioport.h> +#include <linux/io.h> +#include <linux/bitops.h> +#include <linux/err.h> +#include <linux/gpio.h> +#include <linux/slab.h> +#include <linux/module.h> +#include <linux/pinctrl/consumer.h> +#include <linux/platform_data/gpio-rz.h>
Could you please sort the headers alphabetically ?
drivers/gpio/gpio-rz.c line 44
Quoted 13 lines
drivers/gpio/gpio-rz.c new file +9
-0+ +enum {REG_PSR ,REG_PPR ,REG_PMSR ,REG_NR }; + +struct rz_gpio_priv { +void __iomem *io[REG_NR ]; +struct gpio_chip gpio_chip; +}; + +static inline unsigned long rz_gpio_read_ppr(struct rz_gpio_priv *p, intoffs)
drivers/gpio/gpio-rz.c new file +3
-0+{ +unsigned long msk =BIT (offs %RZ_GPIOS_PER_PORT ); +int offset = (offs /RZ_GPIOS_PER_PORT ) *4 ;
offs and offset are unsigned, you can make them unsigned int.
drivers/gpio/gpio-rz.c line 46
Quoted 1 line
drivers/gpio/gpio-rz.c new file +1
-0+return ioread32 (p->io[REG_PPR ] + offset) & msk;
I believe you should return !!(...) here, or in the caller, to make sure the gpio_get_value() operation returns either 0 or 1. I would do it here and return a u32 instead of unsigned long.
drivers/gpio/gpio-rz.c line 53
Quoted 7 lines
drivers/gpio/gpio-rz.c new file +7
-0+} + +static inline void rz_gpio_write (struct rz_gpio_priv *p,int reg,int offs, +bool value) +{ +unsigned long msk =BIT (offs %RZ_GPIOS_PER_PORT ); +int offset = (offs /RZ_GPIOS_PER_PORT ) *4 ;
offs and offset are unsigned here too.
drivers/gpio/gpio-rz.c line 56
Quoted 3 lines
drivers/gpio/gpio-rz.c new file +3
-0+ +/* upper 16 bits contain mask and lower 16 actual value */ + iowrite32(value ? (msk | (msk << 16)) : (msk << 16),
I would have written it has
(value ? msk : 0) | (msk << 16)
but I suppose gcc is smart enough to optimize this.
Quoted 54 lines
drivers/gpio/gpio-rz.c new file +28
-0+ p->io[reg] + offset); +} + +static inline struct rz_gpio_priv *gpio_to_priv (struct gpio_chip *chip) +{ + return container_of(chip, struct rz_gpio_priv, gpio_chip); +} + +static int rz_gpio_direction_input (struct gpio_chip *chip,unsigned offset) +{ +/* Set bit in PM register via PMSR to disable output */ +rz_gpio_write (gpio_to_priv (chip),REG_PMSR , offset, true); +return 0 ; +} + +static int rz_gpio_get (struct gpio_chip *chip,unsigned offset) +{ +/* Get bit from PPR register to determine pin state */ +return (int )(rz_gpio_read_ppr (gpio_to_priv (chip), offset)); +} + +static void rz_gpio_set (struct gpio_chip *chip,unsigned offset,int value) +{ +/* Set bit in P register via PSR to control output */ +rz_gpio_write (gpio_to_priv (chip),REG_PSR , offset, !!value); +} + +static int rz_gpio_direction_output(struct gpio_chip *chip, unsignedoffset,
drivers/gpio/gpio-rz.c new file +25
-0+ int value) +{ +/* Write GPIO value to output before selecting output mode of pin */ +rz_gpio_set (chip, offset, value); + +/* Clear bit in PM register via PMSR to enable output */ +rz_gpio_write (gpio_to_priv (chip),REG_PMSR , offset, false); +return 0 ; +} + +static int rz_gpio_request (struct gpio_chip *chip,unsigned offset) +{ +return pinctrl_request_gpio (chip->base + offset); +} + +static void rz_gpio_free (struct gpio_chip *chip,unsigned offset) +{ +pinctrl_free_gpio (chip->base + offset); + +/* Set the GPIO as an input to ensure that the next GPIO request won't +* drive the GPIO pin as an output. +*/ +rz_gpio_direction_input (chip, offset); +} +
drivers/gpio/gpio-rz.c line 119
Quoted 10 lines
drivers/gpio/gpio-rz.c new file +10
-0+static int rz_gpio_probe (struct platform_device *pdev) +{ +struct gpio_rz_config *pdata =dev_get_platdata (&pdev->dev); +struct rz_gpio_priv *p; +struct resource *io[3 ]; +struct gpio_chip *gpio_chip; +struct device_node *np = pdev->dev.of_node; +struct of_phandle_args args; +int number_of_pins, gpio_base; +int k, nr;
unsigned ?
By the way, what's wrong with i as a loop index ? :-)
Quoted 17 lines
drivers/gpio/gpio-rz.c new file +17
-0+int ret; + + p =devm_kzalloc (&pdev->dev,sizeof (*p),GFP_KERNEL ); +if (!p) { +dev_err (&pdev->dev,"failed to allocate driver data\n" ); +return -ENOMEM ; + } + +for (k =0 ; k <REG_NR ; k++) + io[k] =platform_get_resource (pdev,IORESOURCE_MEM , k); + +/* In case of 3 resources PSR, PPR and PMSR order is expected */ +if (io[REG_PSR ] && io[REG_PPR ] && io[REG_PMSR ]) { + nr =REG_NR ; + }else { +/* A single resource is also acceptable (PPR only) */ +if (io[0 ] && !io[1 ] && !io[2 ]) {
drivers/gpio/gpio-rz.c line 146
Quoted 10 lines
drivers/gpio/gpio-rz.c new file +10
-0+ nr =1 ; + }else { +dev_err (&pdev->dev,"missing IOMEM\n" ); +return -EINVAL ; + } + } + +for (k =0 ; k < nr; k++) { + p->io[k] =devm_ioremap_nocache (&pdev->dev, io[k]->start, +resource_size (io[k]));
You can use devm_ioremap_resource. The function prints an error on failure so you can remove the dev_err() call below. Make sure to check the return value with IS_ERR() and return PTR_ERR() insted of the fixed -ENXIO error.
include/linux/platform_data/gpio-rz.h
Quoted 97 lines
drivers/gpio/gpio-rz.c new file +95
-0+if (!p->io[k]) { +dev_err (&pdev->dev,"failed to remap low I/O memory\n" ); +return -ENXIO ; + } + } + +/* If only 1 resource is available it must be PPR */ +if (nr ==1 ) { + io[REG_PPR ] = io[0 ]; + p->io[REG_PPR ] = p->io[0 ]; + io[0 ] =NULL ; + p->io[0 ] =NULL ; + } + +/* Support one device spanning several ports */ + number_of_pins =RZ_GPIOS_PER_PORT ; +if (io[REG_PSR ]) + number_of_pins *=resource_size (io[REG_PSR ]) /4 ; + + gpio_base = pdata ? pdata->gpio_base :-1 ; + +if (IS_ENABLED (CONFIG_OF ) && np) { + ret =of_parse_phandle_with_fixed_args (np,"gpio-ranges" ,3 ,0 , + &args); + number_of_pins = ret ==0 ? args.args[2 ] :RZ_GPIOS_PER_PORT ; + } + + gpio_chip = &p->gpio_chip; + gpio_chip->direction_input = rz_gpio_direction_input; + gpio_chip->get = rz_gpio_get; + gpio_chip->direction_output = rz_gpio_direction_output; + gpio_chip->set = rz_gpio_set; + gpio_chip->request = rz_gpio_request; + gpio_chip->free = rz_gpio_free; + gpio_chip->label =dev_name (&pdev->dev); + gpio_chip->dev = &pdev->dev; + gpio_chip->owner =THIS_MODULE ; + gpio_chip->base = gpio_base; + gpio_chip->ngpio = number_of_pins; + + ret =gpiochip_add (gpio_chip); +if (ret) { +dev_err (&pdev->dev,"failed to add GPIO controller\n" ); +return ret; + } + +dev_info (&pdev->dev,"driving %d GPIOs\n" , number_of_pins); + +if (pdata && pdata->pctl_name) { + ret =gpiochip_add_pin_range (gpio_chip, pdata->pctl_name,0 , + gpio_chip->base, gpio_chip->ngpio); +if (ret <0 ) +dev_warn (&pdev->dev,"failed to add pin range\n" ); + } +return 0 ; +} + +static int rz_gpio_remove (struct platform_device *pdev) +{ +struct rz_gpio_priv *p =platform_get_drvdata (pdev); + +return gpiochip_remove (&p->gpio_chip); +} + +static const struct of_device_id rz_gpio_dt_ids[] = { + { .compatible ="renesas,gpio-rz" , }, + {}, +}; +MODULE_DEVICE_TABLE (of, rz_gpio_dt_ids); + +static struct platform_driver rz_gpio_device_driver = { + .probe = rz_gpio_probe, + .remove = rz_gpio_remove, + .driver = { + .name ="gpio_rz" , + .of_match_table = rz_gpio_dt_ids, + .owner =THIS_MODULE , + } +}; + +static int __init rz_gpio_init (void ) +{ +return platform_driver_register (&rz_gpio_device_driver); +} +postcore_initcall (rz_gpio_init); + +static void __exit rz_gpio_exit(void) +{ +platform_driver_unregister (&rz_gpio_device_driver); +} +module_exit (rz_gpio_exit); + +MODULE_AUTHOR ("Magnus Damm" ); +MODULE_DESCRIPTION ("Renesas RZ Port GPIO Driver" ); +MODULE_LICENSE ("GPL v2" );include/linux/platform_data/gpio-rz.h new file +0
-0
14:18:46.000000000
Quoted 14 lines
+0900 @@ -0,0 +1,13 @@
include/linux/platform_data/gpio-rz.h new file +13
-0+#ifndef __GPIO_RZ_H__ +#define __GPIO_RZ_H__ + +struct gpio_rz_config { +int gpio_base; +const char *pctl_name; +}; + +#define RZ_GPIOS_PER_PORT 16 + +#define RZ_PORT_PIN (bank, pin) (((bank) * RZ_GPIOS_PER_PORT) + (pin)) + +#endif /* __GPIO_RZ_H__ */
Signature · 3 lines
Regards, Laurent Pinchart
Hi Laurent,
Quoted 72 lines · Laurent Pinchart
Hi Magnus,
Thank you for the patch.
Please read below for a couple of comments in addition to Linus' review.
Quoted 65 lines · On Thursday 07 November 2013 08:47:37 Magnus Damm wrote:
From: Magnus Damm <[email protected]>
This patch adds a GPIO driver for the RZ series of SoCs from Renesas. The driver can be used as platform device with dynamic or static GPIO assignment or via DT using dynamic GPIOs.
The hardware allows control of GPIOs in blocks of up to 16 pins, and once device may span multiple blocks. Interrupts are not included in this hardware block, if interrupts are needed then the PFC needs to be configured to a IRQ pin function which is handled by the GIC hardware.
Tested with yet-to-be-posted platform device and DT devices on r7s72100 and Genmai using LEDs, DIP switches and I2C bitbang.
Signed-off-by Magnus Damm <[email protected]>---
drivers/gpio/Kconfig | 6 drivers/gpio/Makefile | 1 drivers/gpio/gpio-rz.c | 241 ++++++++++++++++++++++++++++++ include/linux/platform_data/gpio-rz.h | 13 + 4 files changed, 261 insertions(+)drivers/gpio/Kconfig 2013-11-06 12:07:13.000000000 +0900 renamed from drivers/gpio/Kconfig +6
-0@@ -230,6 +230,12 @@ config GPIO_RCAR help Say yes here to support GPIO on Renesas R-Car SoCs. +config GPIO_RZ + tristate "Renesas RZ GPIO" + depends on ARM + help + Say yes here to support GPIO on Renesas RZ SoCs. + config GPIO_SAMSUNG bool depends on PLAT_SAMSUNGdrivers/gpio/Makefile 2013-11-06 12:07:13.000000000 +0900 renamed from drivers/gpio/Makefile +1
-0@@ -62,6 +62,7 @@ obj-$(CONFIG_GPIO_PXA) += gpio-pxa.o obj-$(CONFIG_GPIO_RC5T583) += gpio-rc5t583.o obj-$(CONFIG_GPIO_RDC321X) += gpio-rdc321x.o obj-$(CONFIG_GPIO_RCAR) += gpio-rcar.o +obj-$(CONFIG_GPIO_RZ) += gpio-rz.o obj-$(CONFIG_GPIO_SAMSUNG) += gpio-samsung.o obj-$(CONFIG_ARCH_SA1100) += gpio-sa1100.o obj-$(CONFIG_GPIO_SCH) += gpio-sch.odrivers/gpio/gpio-rz.c 2013-11-06 14:20:02.000000000 +0900 new file +13
-0@@ -0,0 +1,241 @@ +/* + * RZ GPIO Support - Ports + * + * Copyright (C) 2013 Magnus Damm + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details.
Quoted 8 lines
Quoted 6 lines
drivers/gpio/gpio-rz.c 2013-11-06 14:20:02.000000000 +0900 new file +4
-0+ * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307USA
+ */You can ditch the last two paragraphs.
Ok!
Quoted 8 lines
Quoted 8 lines
drivers/gpio/gpio-rz.c 2013-11-06 14:20:02.000000000 +0900 new file +8
-0+ +#include <linux/init.h> +#include <linux/platform_device.h> +#include <linux/spinlock.h> +#include <linux/interrupt.h> +#include <linux/ioport.h> +#include <linux/io.h> +#include <linux/bitops.h>
Quoted 8 lines
drivers/gpio/gpio-rz.c 2013-11-06 14:20:02.000000000 +0900
Quoted 6 lines
drivers/gpio/gpio-rz.c 2013-11-06 14:20:02.000000000 +0900 new file +6
-0+#include <linux/err.h> +#include <linux/gpio.h> +#include <linux/slab.h> +#include <linux/module.h> +#include <linux/pinctrl/consumer.h> +#include <linux/platform_data/gpio-rz.h>Could you please sort the headers alphabetically ?
Sure, good idea.
Quoted 7 lines
Quoted 7 lines
drivers/gpio/gpio-rz.c 2013-11-06 14:20:02.000000000 +0900 new file +7
-0+ +enum { REG_PSR, REG_PPR, REG_PMSR, REG_NR }; + +struct rz_gpio_priv { + void __iomem *io[REG_NR]; + struct gpio_chip gpio_chip; +};
Quoted 8 lines
drivers/gpio/gpio-rz.c 2013-11-06 14:20:02.000000000 +0900
Quoted 6 lines
drivers/gpio/gpio-rz.c 2013-11-06 14:20:02.000000000 +0900 new file +2
-0+ +static inline unsigned long rz_gpio_read_ppr(struct rz_gpio_priv *p, intoffs)
drivers/gpio/gpio-rz.c 2013-11-06 14:20:02.000000000 +0900 new file +3
-0+{ + unsigned long msk = BIT(offs % RZ_GPIOS_PER_PORT); + int offset = (offs / RZ_GPIOS_PER_PORT) * 4;offs and offset are unsigned, you can make them unsigned int.
Ok!
Quoted 5 lines
Quoted 1 line
+ return
ioread32(p->io[REG_PPR] + offset)& msk;I believe you should return !!(...) here, or in the caller, to make sure the
gpio_get_value()operation returns either 0 or 1. I would do it here and return a u32 instead of unsigned long.
I disagree with the !! because it is just pure overhead, please see the __gpio_get_value() comment, it says returning zero or nonzero. So I left this portion as-is.
Quoted 9 lines
drivers/gpio/gpio-rz.c 2013-11-06 14:20:02.000000000 +0900
Quoted 7 lines
drivers/gpio/gpio-rz.c 2013-11-06 14:20:02.000000000 +0900 new file +7
-0+} + +static inline void rz_gpio_write(struct rz_gpio_priv *p, int reg, int offs, + bool value) +{ + unsigned long msk = BIT(offs % RZ_GPIOS_PER_PORT); + int offset = (offs / RZ_GPIOS_PER_PORT) * 4;offs and offset are unsigned here too.
Ok!
Quoted 9 lines
drivers/gpio/gpio-rz.c 2013-11-06 14:20:02.000000000 +0900
Quoted 3 lines
drivers/gpio/gpio-rz.c 2013-11-06 14:20:02.000000000 +0900 new file +3
-0+ + /* upper 16 bits contain mask and lower 16 actual value */ + iowrite32(value ? (msk | (msk << 16)) : (msk << 16),I would have written it has
(value ? msk : 0) | (msk << 16)
but I suppose gcc is smart enough to optimize this.
I left this as-is.
Quoted 58 lines
Quoted 58 lines
drivers/gpio/gpio-rz.c 2013-11-06 14:20:02.000000000 +0900 new file +28
-0+ p->io[reg] + offset); +} + +static inline struct rz_gpio_priv *gpio_to_priv(struct gpio_chip *chip) +{ + return container_of(chip, struct rz_gpio_priv, gpio_chip); +} + +static int rz_gpio_direction_input(struct gpio_chip *chip, unsigned offset) +{ + /* Set bit in PM register via PMSR to disable output */ + rz_gpio_write(gpio_to_priv(chip), REG_PMSR, offset, true); + return 0; +} + +static int rz_gpio_get(struct gpio_chip *chip, unsigned offset) +{ + /* Get bit from PPR register to determine pin state */ + return (int)(rz_gpio_read_ppr(gpio_to_priv(chip), offset)); +} + +static void rz_gpio_set(struct gpio_chip *chip, unsigned offset, int value) +{ + /* Set bit in P register via PSR to control output */ + rz_gpio_write(gpio_to_priv(chip), REG_PSR, offset, !!value); +} + +static int rz_gpio_direction_output(struct gpio_chip *chip, unsignedoffset,
drivers/gpio/gpio-rz.c 2013-11-06 14:20:02.000000000 +0900 new file +29
-0+ int value) +{ + /* Write GPIO value to output before selecting output mode of pin */ + rz_gpio_set(chip, offset, value); + + /* Clear bit in PM register via PMSR to enable output */ + rz_gpio_write(gpio_to_priv(chip), REG_PMSR, offset, false); + return 0; +} + +static int rz_gpio_request(struct gpio_chip *chip, unsigned offset) +{ + return pinctrl_request_gpio(chip->base + offset); +} + +static void rz_gpio_free(struct gpio_chip *chip, unsigned offset) +{ + pinctrl_free_gpio(chip->base + offset); + + /* Set the GPIO as an input to ensure that the next GPIO request won't + * drive the GPIO pin as an output. + */ + rz_gpio_direction_input(chip, offset); +} + +static int rz_gpio_probe(struct platform_device *pdev) +{ + struct gpio_rz_config *pdata = dev_get_platdata(&pdev->dev); + struct rz_gpio_priv *p;
Quoted 8 lines
drivers/gpio/gpio-rz.c 2013-11-06 14:20:02.000000000 +0900
Quoted 6 lines
drivers/gpio/gpio-rz.c 2013-11-06 14:20:02.000000000 +0900 new file +6
-0+ struct resource *io[3]; + struct gpio_chip *gpio_chip; + struct device_node *np = pdev->dev.of_node; + struct of_phandle_args args; + int number_of_pins, gpio_base; + int k, nr;unsigned ?
Ok!
Quoted 1 line
By the way, what's wrong with i as a loop index ? :-)
Nothing, but I left it as-is anyway! =)
Quoted 23 lines
Quoted 23 lines
drivers/gpio/gpio-rz.c 2013-11-06 14:20:02.000000000 +0900 new file +23
-0+ int ret; + + p = devm_kzalloc(&pdev->dev, sizeof(*p), GFP_KERNEL); + if (!p) { + dev_err(&pdev->dev, "failed to allocate driver data\n"); + return -ENOMEM; + } + + for (k = 0; k < REG_NR; k++) + io[k] = platform_get_resource(pdev, IORESOURCE_MEM, k); + + /* In case of 3 resources PSR, PPR and PMSR order is expected */ + if (io[REG_PSR] && io[REG_PPR] && io[REG_PMSR]) { + nr = REG_NR; + } else { + /* A single resource is also acceptable (PPR only) */ + if (io[0] && !io[1] && !io[2]) { + nr = 1; + } else { + dev_err(&pdev->dev, "missing IOMEM\n"); + return -EINVAL; + } + }
Quoted 8 lines
drivers/gpio/gpio-rz.c 2013-11-06 14:20:02.000000000 +0900
Quoted 4 lines
drivers/gpio/gpio-rz.c 2013-11-06 14:20:02.000000000 +0900 new file +4
-0+ + for (k = 0; k < nr; k++) { + p->io[k] = devm_ioremap_nocache(&pdev->dev, io[k]->start, + resource_size(io[k]));You can use
devm_ioremap_resource. The function prints an error on failure so you can remove thedev_err()call below. Make sure to check the return value withIS_ERR()and returnPTR_ERR()insted of the fixed-ENXIOerror.
Good idea, this indeed makes the code nicer.
I've included these changes in V2 that I posted a little while ago.
Thanks for your review.
Cheers,
/ magnus
Hi Magnus,
Quoted 31 lines · On Thursday 14 November 2013 08:49:16 Magnus Damm wrote:
Quoted 30 lines · Laurent Pinchart
Hi Magnus,
Thank you for the patch.
Please read below for a couple of comments in addition to Linus' review.
Quoted 23 lines · On Thursday 07 November 2013 08:47:37 Magnus Damm wrote:
From: Magnus Damm <[email protected]>
This patch adds a GPIO driver for the RZ series of SoCs from Renesas. The driver can be used as platform device with dynamic or static GPIO assignment or via DT using dynamic GPIOs.
The hardware allows control of GPIOs in blocks of up to 16 pins, and once device may span multiple blocks. Interrupts are not included in this hardware block, if interrupts are needed then the PFC needs to be configured to a IRQ pin function which is handled by the GIC hardware.
Tested with yet-to-be-posted platform device and DT devices on r7s72100 and Genmai using LEDs, DIP switches and I2C bitbang.
Signed-off-by Magnus Damm <[email protected]>---
drivers/gpio/Kconfig | 6 drivers/gpio/Makefile | 1 drivers/gpio/gpio-rz.c | 241 +++++++++++++++++++++++++++ include/linux/platform_data/gpio-rz.h | 13 + 4 files changed, 261 insertions(+)
[snip]
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
drivers/gpio/gpio-rz.c 2013-11-06 14:20:02.000000000 +0900 new file +0
-0@@ -0,0 +1,241 @@
[snip]
Quoted 12 lines
Quoted 7 lines
drivers/gpio/gpio-rz.c 2013-11-06 14:20:02.000000000 +0900
Quoted 5 lines
+static inline unsigned long
rz_gpio_read_ppr(struct rz_gpio_priv *p, int offs)drivers/gpio/gpio-rz.c 2013-11-06 14:20:02.000000000 +0900 new file +3
-0+{ + unsigned long msk = BIT(offs % RZ_GPIOS_PER_PORT); + int offset = (offs / RZ_GPIOS_PER_PORT) * 4;offs and offset are unsigned, you can make them unsigned int.
Ok!
Quoted 2 lines
Quoted 1 line
+ return
ioread32(p->io[REG_PPR] + offset)& msk;
Quoted 7 lines
Quoted 3 lines
I believe you should return !!(...) here, or in the caller, to make sure the
gpio_get_value()operation returns either 0 or 1. I would do it here and return a u32 instead of unsigned long.I disagree with the !! because it is just pure overhead, please see the
__gpio_get_value()comment, it says returning zero or nonzero. So I left this portion as-is.
OK, good point. Linus, what's the best practice rule for GPIO drivers ? Should they just return any non-zero value, or is any specific value preferred ?
Quoted 1 line
Quoted 1 line
Quoted 1 line
+}
[snip]
Quoted 13 lines
Quoted 4 lines
Quoted 4 lines
drivers/gpio/gpio-rz.c 2013-11-06 14:20:02.000000000 +0900 new file +4
-0+static int rz_gpio_probe(struct platform_device *pdev) +{ + struct gpio_rz_config *pdata = dev_get_platdata(&pdev->dev); + struct rz_gpio_priv *p;Quoted 8 lines
drivers/gpio/gpio-rz.c 2013-11-06 14:20:02.000000000 +0900
Quoted 6 lines
drivers/gpio/gpio-rz.c 2013-11-06 14:20:02.000000000 +0900 new file +6
-0+ struct resource *io[3]; + struct gpio_chip *gpio_chip; + struct device_node *np = pdev->dev.of_node; + struct of_phandle_args args; + int number_of_pins, gpio_base; + int k, nr;unsigned ?
Quoted 5 lines
Ok!
Quoted 1 line
By the way, what's wrong with i as a loop index ? :-)
Nothing, but I left it as-is anyway! =)
Good to know it's not wrong. But it's still an int in v2 ;-)
Quoted 1 line
Quoted 1 line
Quoted 1 line
+ int ret;
Signature · 3 lines
Regards, Laurent Pinchart
Hi Laurent,
Quoted 59 lines · Laurent Pinchart
Hi Magnus,
Quoted 31 lines · On Thursday 14 November 2013 08:49:16 Magnus Damm wrote:
Quoted 30 lines · Laurent Pinchart
Hi Magnus,
Thank you for the patch.
Please read below for a couple of comments in addition to Linus' review.
Quoted 23 lines · On Thursday 07 November 2013 08:47:37 Magnus Damm wrote:
From: Magnus Damm <[email protected]>
This patch adds a GPIO driver for the RZ series of SoCs from Renesas. The driver can be used as platform device with dynamic or static GPIO assignment or via DT using dynamic GPIOs.
The hardware allows control of GPIOs in blocks of up to 16 pins, and once device may span multiple blocks. Interrupts are not included in this hardware block, if interrupts are needed then the PFC needs to be configured to a IRQ pin function which is handled by the GIC hardware.
Tested with yet-to-be-posted platform device and DT devices on r7s72100 and Genmai using LEDs, DIP switches and I2C bitbang.
Signed-off-by Magnus Damm <[email protected]>---
drivers/gpio/Kconfig | 6 drivers/gpio/Makefile | 1 drivers/gpio/gpio-rz.c | 241 +++++++++++++++++++++++++++ include/linux/platform_data/gpio-rz.h | 13 + 4 files changed, 261 insertions(+)[snip]
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
drivers/gpio/gpio-rz.c 2013-11-06 14:20:02.000000000 +0900 new file +0
-0@@ -0,0 +1,241 @@[snip]
Quoted 16 lines
Quoted 7 lines
drivers/gpio/gpio-rz.c 2013-11-06 14:20:02.000000000 +0900
Quoted 5 lines
+static inline unsigned long
rz_gpio_read_ppr(struct rz_gpio_priv *p, int offs)drivers/gpio/gpio-rz.c 2013-11-06 14:20:02.000000000 +0900 new file +3
-0+{ + unsigned long msk = BIT(offs % RZ_GPIOS_PER_PORT); + int offset = (offs / RZ_GPIOS_PER_PORT) * 4;offs and offset are unsigned, you can make them unsigned int.
Ok!
Quoted 5 lines
Quoted 1 line
+ return
ioread32(p->io[REG_PPR] + offset)& msk;I believe you should return !!(...) here, or in the caller, to make sure the
gpio_get_value()operation returns either 0 or 1. I would do it here and return a u32 instead of unsigned long.
Quoted 6 lines
Quoted 3 lines
I disagree with the !! because it is just pure overhead, please see the
__gpio_get_value()comment, it says returning zero or nonzero. So I left this portion as-is.OK, good point. Linus, what's the best practice rule for GPIO drivers ? Should they just return any non-zero value, or is any specific value preferred ?
I too would like to hear Linus opinion. From my side, the other two GPIO drivers that I've written follow this rule. If someone for some unknown reason want to change things around then perhaps the interface should also be updated to instead return a bool?
Regardless, since other drivers follow this style (see for instance davinci_gpio_get()) perhaps this kind of interface adjustment simply could be handled as a separate discussion with potential incremental changes?
Quoted 13 lines
Quoted 13 lines
Quoted 12 lines
drivers/gpio/gpio-rz.c 2013-11-06 14:20:02.000000000 +0900
Quoted 10 lines
drivers/gpio/gpio-rz.c 2013-11-06 14:20:02.000000000 +0900 new file +10
-0+static int rz_gpio_probe(struct platform_device *pdev) +{ + struct gpio_rz_config *pdata = dev_get_platdata(&pdev->dev); + struct rz_gpio_priv *p; + struct resource *io[3]; + struct gpio_chip *gpio_chip; + struct device_node *np = pdev->dev.of_node; + struct of_phandle_args args; + int number_of_pins, gpio_base; + int k, nr;unsigned ?
Quoted 7 lines
Quoted 5 lines
Ok!
Quoted 1 line
By the way, what's wrong with i as a loop index ? :-)
Nothing, but I left it as-is anyway! =)
Good to know it's not wrong. But it's still an int in v2 ;-)
Ok-ok, gotcha, will use unsigned there too.
As for "i" vs "k", my tired old eyes prefer to see "k" over "i", "I", "1", "|"...
Thanks,
/ magnus
Quoted 59 lines · Laurent Pinchart
Hi Magnus,
Thank you for the patch.
Please read below for a couple of comments in addition to Linus' review.
Quoted 52 lines · On Thursday 07 November 2013 08:47:37 Magnus Damm wrote:
From: Magnus Damm <[email protected]>
This patch adds a GPIO driver for the RZ series of SoCs from Renesas. The driver can be used as platform device with dynamic or static GPIO assignment or via DT using dynamic GPIOs.
The hardware allows control of GPIOs in blocks of up to 16 pins, and once device may span multiple blocks. Interrupts are not included in this hardware block, if interrupts are needed then the PFC needs to be configured to a IRQ pin function which is handled by the GIC hardware.
Tested with yet-to-be-posted platform device and DT devices on r7s72100 and Genmai using LEDs, DIP switches and I2C bitbang.
Signed-off-by Magnus Damm <[email protected]>---
drivers/gpio/Kconfig | 6 drivers/gpio/Makefile | 1 drivers/gpio/gpio-rz.c | 241 ++++++++++++++++++++++++++++++ include/linux/platform_data/gpio-rz.h | 13 + 4 files changed, 261 insertions(+)drivers/gpio/Kconfig +6
-0@@ -230,6 +230,12 @@ config GPIO_RCAR help Say yes here to support GPIO on Renesas R-Car SoCs. +config GPIO_RZ + tristate "Renesas RZ GPIO" + depends on ARM + help + Say yes here to support GPIO on Renesas RZ SoCs. + config GPIO_SAMSUNG bool depends on PLAT_SAMSUNGdrivers/gpio/Makefile +1
-0@@ -62,6 +62,7 @@ obj-$(CONFIG_GPIO_PXA) += gpio-pxa.o obj-$(CONFIG_GPIO_RC5T583) += gpio-rc5t583.o obj-$(CONFIG_GPIO_RDC321X) += gpio-rdc321x.o obj-$(CONFIG_GPIO_RCAR) += gpio-rcar.o +obj-$(CONFIG_GPIO_RZ) += gpio-rz.o obj-$(CONFIG_GPIO_SAMSUNG) += gpio-samsung.o obj-$(CONFIG_ARCH_SA1100) += gpio-sa1100.o obj-$(CONFIG_GPIO_SCH) += gpio-sch.odrivers/gpio/gpio-rz.c new file +0
-0@@ -0,0 +1,241 @@
Quoted 8 lines
drivers/gpio/gpio-rz.c line 8
Quoted 8 lines
drivers/gpio/gpio-rz.c new file +8
-0+/* +* RZ GPIO Support - Ports +* +* Copyright (C) 2013 Magnus Damm +* +* This program is free software; you can redistribute it and/or modify +* it under the terms of the GNU General Public License as published by +* the Free Software Foundation; either version 2 of the License
Is there a trailing portion of this paragraph that is missing?
Quoted 14 lines
Quoted 11 lines
drivers/gpio/gpio-rz.c new file +9
-0+* +* This program is distributed in the hope that it will be useful, +* but WITHOUT ANY WARRANTY; without even the implied warranty of +* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +* GNU General Public License for more details. +* +* You should have received a copy of the GNU General Public License +* along with this program; if not, write to the Free Software +* Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
+ */You can ditch the last two paragraphs.
Quoted 16 lines
Quoted 4 lines
drivers/gpio/gpio-rz.c new file +4
-0+ +#include <linux/init.h> +#include <linux/platform_device.h> +#include <linux/spinlock.h> drivers/gpio/gpio-rz.c line 32
Quoted 10 lines
drivers/gpio/gpio-rz.c new file +10
-0+#include <linux/interrupt.h> +#include <linux/ioport.h> +#include <linux/io.h> +#include <linux/bitops.h> +#include <linux/err.h> +#include <linux/gpio.h> +#include <linux/slab.h> +#include <linux/module.h> +#include <linux/pinctrl/consumer.h> +#include <linux/platform_data/gpio-rz.h> Could you please sort the headers alphabetically ?
As its slightly relevant: I recently discovered that vim has a sort command.
:-13,. sort
[snip]
Quoted 2 lines · Magnus Damm
The hardware allows control of GPIOs in blocks of up to 16 pins, and once device may span multiple blocks. Interrupts are not
... one device ...
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds