Re: [PATCH v3 3/8] gpio: gpio-rz: GPIO driver for Renesas RZ series

2017-01-19 Thread Geert Uytterhoeven
Hi Linus,

On Thu, Jan 19, 2017 at 10:27 AM, Linus Walleij
 wrote:
> On Wed, Jan 18, 2017 at 3:06 PM, Geert Uytterhoeven
>  wrote:
>> On Wed, Jan 18, 2017 at 2:58 PM, Linus Walleij  
>> wrote:
 +   gpio_chip->request = rz_gpio_request;
 +   gpio_chip->free = rz_gpio_free;
 +   gpio_chip->label = dev_name(>dev);
 +   gpio_chip->parent = >dev;
 +   gpio_chip->owner = THIS_MODULE;
 +   gpio_chip->base = -1;
 +   gpio_chip->ngpio = ret == 0 ? args.args[2] : RZ_GPIOS_PER_PORT;
>>>
>>> bgpio_init() will have already set this up to 16 (RZ_GPIOS_PER_PORT)
>>> as we pass width 2 bytes.
>>
>> Note that some banks have less than 16 GPIOs, cfr. the last value of the
>> gpio-ranges tuple being less than 16.
>
> Aha OK then it is fine to override this default value calculate from
> the register size.
>
> But for that case we should use the standard DT property
> ngpios described in
> Documentation/devicetree/bindings/gpio/gpio.txt
> It is for exactly this purpose.

IC.

Note that gpio-rcar uses the same method, switching to "ngpios" would
break backwards compatibility.

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org

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


Re: [PATCH v3 3/8] gpio: gpio-rz: GPIO driver for Renesas RZ series

2017-01-19 Thread Linus Walleij
On Thu, Jan 19, 2017 at 10:36 AM, Geert Uytterhoeven
 wrote:
> On Thu, Jan 19, 2017 at 10:27 AM, Linus Walleij
>  wrote:
>> On Wed, Jan 18, 2017 at 3:06 PM, Geert Uytterhoeven
>>  wrote:
>>> On Wed, Jan 18, 2017 at 2:58 PM, Linus Walleij  
>>> wrote:
> +   gpio_chip->request = rz_gpio_request;
> +   gpio_chip->free = rz_gpio_free;
> +   gpio_chip->label = dev_name(>dev);
> +   gpio_chip->parent = >dev;
> +   gpio_chip->owner = THIS_MODULE;
> +   gpio_chip->base = -1;
> +   gpio_chip->ngpio = ret == 0 ? args.args[2] : RZ_GPIOS_PER_PORT;

 bgpio_init() will have already set this up to 16 (RZ_GPIOS_PER_PORT)
 as we pass width 2 bytes.
>>>
>>> Note that some banks have less than 16 GPIOs, cfr. the last value of the
>>> gpio-ranges tuple being less than 16.
>>
>> Aha OK then it is fine to override this default value calculate from
>> the register size.
>>
>> But for that case we should use the standard DT property
>> ngpios described in
>> Documentation/devicetree/bindings/gpio/gpio.txt
>> It is for exactly this purpose.
>
> IC.
>
> Note that gpio-rcar uses the same method, switching to "ngpios" would
> break backwards compatibility.

Can we support both?

Yours,
Linus Walleij


Re: [PATCH v3 3/8] gpio: gpio-rz: GPIO driver for Renesas RZ series

2017-01-19 Thread Linus Walleij
On Wed, Jan 18, 2017 at 3:06 PM, Geert Uytterhoeven
 wrote:
> Hi Linus,
>
> On Wed, Jan 18, 2017 at 2:58 PM, Linus Walleij  
> wrote:
>>> +   gpio_chip->request = rz_gpio_request;
>>> +   gpio_chip->free = rz_gpio_free;
>>> +   gpio_chip->label = dev_name(>dev);
>>> +   gpio_chip->parent = >dev;
>>> +   gpio_chip->owner = THIS_MODULE;
>>> +   gpio_chip->base = -1;
>>> +   gpio_chip->ngpio = ret == 0 ? args.args[2] : RZ_GPIOS_PER_PORT;
>>
>> bgpio_init() will have already set this up to 16 (RZ_GPIOS_PER_PORT)
>> as we pass width 2 bytes.
>
> Note that some banks have less than 16 GPIOs, cfr. the last value of the
> gpio-ranges tuple being less than 16.

Aha OK then it is fine to override this default value calculate from
the register size.

But for that case we should use the standard DT property
ngpios described in
Documentation/devicetree/bindings/gpio/gpio.txt
It is for exactly this purpose.

Yours,
Linus Walleij


Re: [PATCH v3 3/8] gpio: gpio-rz: GPIO driver for Renesas RZ series

2017-01-18 Thread jacopo mondi

Hi Linus,
   thanks for review

On 18/01/2017 14:58, Linus Walleij wrote:

On Mon, Jan 16, 2017 at 1:12 PM, Jacopo Mondi  wrote:


From: Magnus Damm 

This commit combines Magnus' original driver and minor fixes to
forward-port it to a more recent kernel version (v4.10).

Compared to the original driver the set of registers used to set/get
direction is changed to extend compatibility with other RZ-Series
processors.

Signed-off-by: Magnus Damm 
Signed-off-by: Jacopo Mondi 


Sorry for not noting all you can do on first iteration... :/


Wow, it seems like there's lot of code already in place we can exploit here!

IF I'm going to send out v4 I'll move it to use gpio_generic.
If as it seems, we're going to try submit a new combined PFC+GPIO driver 
for RZ/A processor, I'll see if I can do the same there.


Thanks
   j





+config GPIO_RZ
+   tristate "Renesas RZ GPIO"
+   depends on ARCH_RENESAS


select GPIO_GENERIC

Trust me. It's gonna be real nice.


+static inline struct rz_gpio_priv *gpio_to_priv(struct gpio_chip *chip)
+{
+   return gpiochip_get_data(chip);
+}
+
+static inline u16 rz_gpio_read(struct  gpio_chip *chip, int reg)
+{
+   return ioread16(gpio_to_priv(chip)->io[reg]);
+}
+
+static inline void rz_gpio_write(struct gpio_chip *chip, int reg, u16 val)
+{
+   iowrite16(val, gpio_to_priv(chip)->io[reg]);
+}
+
+static int rz_gpio_get(struct gpio_chip *chip, unsigned gpio)
+{
+   u16 tmp = rz_gpio_read(chip, REG_PPR);
+
+   return tmp & BIT(gpio);
+}
+
+static void rz_gpio_set(struct gpio_chip *chip, unsigned gpio, int value)
+{
+   u16 tmp;
+
+   if (gpio_to_priv(chip)->nreg == PORT0_NUM_REGS)
+   return;
+
+   tmp = rz_gpio_read(chip, REG_P);
+
+   if (value)
+   rz_gpio_write(chip, REG_P, tmp | BIT(gpio));
+   else
+   rz_gpio_write(chip, REG_P, tmp & ~BIT(gpio));
+}
+
+static int rz_gpio_direction_input(struct gpio_chip *chip, unsigned gpio)
+{
+   /* Set bit in PM register (input buffer enabled by PFC for the pin) */
+   rz_gpio_write(chip, REG_PM, rz_gpio_read(chip, REG_PM) | BIT(gpio));
+
+   return 0;
+}
+
+static int rz_gpio_direction_output(struct gpio_chip *chip, unsigned gpio,
+  int value)
+{
+
+   if (gpio_to_priv(chip)->nreg == PORT0_NUM_REGS)
+   return -EINVAL;
+
+   /* Write GPIO value before selecting output mode of pin */
+   rz_gpio_set(chip, gpio, value);
+
+   /* Clear bit in PM register to enable output */
+   rz_gpio_write(chip, REG_PM, rz_gpio_read(chip, REG_PM) & BIT(gpio));
+
+   return 0;
+}
+
+static int rz_gpio_get_direction(struct gpio_chip *chip, unsigned gpio)
+{
+   if (gpio_to_priv(chip)->nreg == PORT0_NUM_REGS)
+   return 1;
+
+   return rz_gpio_read(chip, REG_PM) & BIT(gpio);
+}



Delete ALL the above functions.


+static int rz_gpio_request(struct gpio_chip *chip, unsigned gpio)
+{
+   return gpiochip_generic_request(chip, gpio);
+}
+
+static void rz_gpio_free(struct gpio_chip *chip, unsigned gpio)
+{
+   gpiochip_generic_free(chip, gpio);
+
+   /* 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, gpio);


Change this line to:
chip->direction_input(chip, gpio);


+static int rz_gpio_probe(struct platform_device *pdev)
+{
+   struct rz_gpio_priv *p;
+   struct resource *io[REG_NR - 1];
+   struct gpio_chip *gpio_chip;
+   struct device_node *np = pdev->dev.of_node;
+   struct of_phandle_args args;
+   int ret, k;
+
+   p = devm_kzalloc(>dev, sizeof(*p), GFP_KERNEL);
+   if (!p) {
+   dev_err(>dev, "failed to allocate driver data\n");
+   return -ENOMEM;
+   }
+
+   /* As registers for each port instance are scattered in the same
+* address space, we have to map them singularly */
+   for (k = 0; k < REG_NR; k++) {
+   io[k] = platform_get_resource(pdev, IORESOURCE_MEM, k);
+
+   /* Port0 and JP0 are inuput only: has REG_PPR only */
+   if (!io[k])
+   break;
+
+   p->io[k] = devm_ioremap_resource(>dev, io[k]);
+   if (IS_ERR(p->io[k]))
+   return PTR_ERR(p->io[k]);
+
+   p->nreg++;
+   }
+
+   /* move REG_PPR in correct position for Port0 and JP0 */
+   if (p->nreg == PORT0_NUM_REGS) {
+   p->io[REG_PPR] = p->io[REG_P];
+   p->io[REG_P] = p->io[REG_PM] = NULL;
+   }
+
+   ret = of_parse_phandle_with_fixed_args(np, "gpio-ranges", 3, 0, );
+
+   gpio_chip = >gpio_chip;


Replace from here:


+   gpio_chip->get = rz_gpio_get;
+   gpio_chip->set = rz_gpio_set;
+   gpio_chip->direction_input = rz_gpio_direction_input;
+  

Re: [PATCH v3 3/8] gpio: gpio-rz: GPIO driver for Renesas RZ series

2017-01-18 Thread Geert Uytterhoeven
Hi Linus,

On Wed, Jan 18, 2017 at 2:58 PM, Linus Walleij  wrote:
>> +   gpio_chip->request = rz_gpio_request;
>> +   gpio_chip->free = rz_gpio_free;
>> +   gpio_chip->label = dev_name(>dev);
>> +   gpio_chip->parent = >dev;
>> +   gpio_chip->owner = THIS_MODULE;
>> +   gpio_chip->base = -1;
>> +   gpio_chip->ngpio = ret == 0 ? args.args[2] : RZ_GPIOS_PER_PORT;
>
> bgpio_init() will have already set this up to 16 (RZ_GPIOS_PER_PORT)
> as we pass width 2 bytes.

Note that some banks have less than 16 GPIOs, cfr. the last value of the
gpio-ranges tuple being less than 16.

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org

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


Re: [PATCH v3 3/8] gpio: gpio-rz: GPIO driver for Renesas RZ series

2017-01-18 Thread Linus Walleij
On Mon, Jan 16, 2017 at 1:12 PM, Jacopo Mondi  wrote:

> From: Magnus Damm 
>
> This commit combines Magnus' original driver and minor fixes to
> forward-port it to a more recent kernel version (v4.10).
>
> Compared to the original driver the set of registers used to set/get
> direction is changed to extend compatibility with other RZ-Series
> processors.
>
> Signed-off-by: Magnus Damm 
> Signed-off-by: Jacopo Mondi 

Sorry for not noting all you can do on first iteration... :/

> +config GPIO_RZ
> +   tristate "Renesas RZ GPIO"
> +   depends on ARCH_RENESAS

select GPIO_GENERIC

Trust me. It's gonna be real nice.

> +static inline struct rz_gpio_priv *gpio_to_priv(struct gpio_chip *chip)
> +{
> +   return gpiochip_get_data(chip);
> +}
> +
> +static inline u16 rz_gpio_read(struct  gpio_chip *chip, int reg)
> +{
> +   return ioread16(gpio_to_priv(chip)->io[reg]);
> +}
> +
> +static inline void rz_gpio_write(struct gpio_chip *chip, int reg, u16 val)
> +{
> +   iowrite16(val, gpio_to_priv(chip)->io[reg]);
> +}
> +
> +static int rz_gpio_get(struct gpio_chip *chip, unsigned gpio)
> +{
> +   u16 tmp = rz_gpio_read(chip, REG_PPR);
> +
> +   return tmp & BIT(gpio);
> +}
> +
> +static void rz_gpio_set(struct gpio_chip *chip, unsigned gpio, int value)
> +{
> +   u16 tmp;
> +
> +   if (gpio_to_priv(chip)->nreg == PORT0_NUM_REGS)
> +   return;
> +
> +   tmp = rz_gpio_read(chip, REG_P);
> +
> +   if (value)
> +   rz_gpio_write(chip, REG_P, tmp | BIT(gpio));
> +   else
> +   rz_gpio_write(chip, REG_P, tmp & ~BIT(gpio));
> +}
> +
> +static int rz_gpio_direction_input(struct gpio_chip *chip, unsigned gpio)
> +{
> +   /* Set bit in PM register (input buffer enabled by PFC for the pin) */
> +   rz_gpio_write(chip, REG_PM, rz_gpio_read(chip, REG_PM) | BIT(gpio));
> +
> +   return 0;
> +}
> +
> +static int rz_gpio_direction_output(struct gpio_chip *chip, unsigned gpio,
> +  int value)
> +{
> +
> +   if (gpio_to_priv(chip)->nreg == PORT0_NUM_REGS)
> +   return -EINVAL;
> +
> +   /* Write GPIO value before selecting output mode of pin */
> +   rz_gpio_set(chip, gpio, value);
> +
> +   /* Clear bit in PM register to enable output */
> +   rz_gpio_write(chip, REG_PM, rz_gpio_read(chip, REG_PM) & BIT(gpio));
> +
> +   return 0;
> +}
> +
> +static int rz_gpio_get_direction(struct gpio_chip *chip, unsigned gpio)
> +{
> +   if (gpio_to_priv(chip)->nreg == PORT0_NUM_REGS)
> +   return 1;
> +
> +   return rz_gpio_read(chip, REG_PM) & BIT(gpio);
> +}


Delete ALL the above functions.

> +static int rz_gpio_request(struct gpio_chip *chip, unsigned gpio)
> +{
> +   return gpiochip_generic_request(chip, gpio);
> +}
> +
> +static void rz_gpio_free(struct gpio_chip *chip, unsigned gpio)
> +{
> +   gpiochip_generic_free(chip, gpio);
> +
> +   /* 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, gpio);

Change this line to:
chip->direction_input(chip, gpio);

> +static int rz_gpio_probe(struct platform_device *pdev)
> +{
> +   struct rz_gpio_priv *p;
> +   struct resource *io[REG_NR - 1];
> +   struct gpio_chip *gpio_chip;
> +   struct device_node *np = pdev->dev.of_node;
> +   struct of_phandle_args args;
> +   int ret, k;
> +
> +   p = devm_kzalloc(>dev, sizeof(*p), GFP_KERNEL);
> +   if (!p) {
> +   dev_err(>dev, "failed to allocate driver data\n");
> +   return -ENOMEM;
> +   }
> +
> +   /* As registers for each port instance are scattered in the same
> +* address space, we have to map them singularly */
> +   for (k = 0; k < REG_NR; k++) {
> +   io[k] = platform_get_resource(pdev, IORESOURCE_MEM, k);
> +
> +   /* Port0 and JP0 are inuput only: has REG_PPR only */
> +   if (!io[k])
> +   break;
> +
> +   p->io[k] = devm_ioremap_resource(>dev, io[k]);
> +   if (IS_ERR(p->io[k]))
> +   return PTR_ERR(p->io[k]);
> +
> +   p->nreg++;
> +   }
> +
> +   /* move REG_PPR in correct position for Port0 and JP0 */
> +   if (p->nreg == PORT0_NUM_REGS) {
> +   p->io[REG_PPR] = p->io[REG_P];
> +   p->io[REG_P] = p->io[REG_PM] = NULL;
> +   }
> +
> +   ret = of_parse_phandle_with_fixed_args(np, "gpio-ranges", 3, 0, 
> );
> +
> +   gpio_chip = >gpio_chip;

Replace from here:

> +   gpio_chip->get = rz_gpio_get;
> +   gpio_chip->set = rz_gpio_set;
> +   gpio_chip->direction_input = rz_gpio_direction_input;
> +   gpio_chip->direction_output = rz_gpio_direction_output;
> +   gpio_chip->get_direction = 

[PATCH v3 3/8] gpio: gpio-rz: GPIO driver for Renesas RZ series

2017-01-16 Thread Jacopo Mondi
From: Magnus Damm 

This commit combines Magnus' original driver and minor fixes to
forward-port it to a more recent kernel version (v4.10).

Compared to the original driver the set of registers used to set/get
direction is changed to extend compatibility with other RZ-Series
processors.

Signed-off-by: Magnus Damm 
Signed-off-by: Jacopo Mondi 
---
 drivers/gpio/Kconfig   |   6 ++
 drivers/gpio/Makefile  |   1 +
 drivers/gpio/gpio-rz.c | 211 +
 3 files changed, 218 insertions(+)
 create mode 100644 drivers/gpio/gpio-rz.c

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index d5d3654..e9ad7b4 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -369,6 +369,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 ARCH_RENESAS
+   help
+ Say yes here to support GPIO on Renesas RZ SoCs.
+
 config GPIO_SPEAR_SPICS
bool "ST SPEAr13xx SPI Chip Select as GPIO support"
depends on PLAT_SPEAR
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index a7676b8..f0b2713 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -96,6 +96,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_ARCH_SA1100)  += gpio-sa1100.o
 obj-$(CONFIG_GPIO_SCH) += gpio-sch.o
 obj-$(CONFIG_GPIO_SCH311X) += gpio-sch311x.o
diff --git a/drivers/gpio/gpio-rz.c b/drivers/gpio/gpio-rz.c
new file mode 100644
index 000..ad67975
--- /dev/null
+++ b/drivers/gpio/gpio-rz.c
@@ -0,0 +1,211 @@
+/*
+ * 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; version 2 of the
+ * License.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#define RZ_GPIOS_PER_PORT 16
+#define PORT0_NUM_REGS 1
+
+enum { REG_P, REG_PPR, REG_PM, REG_NR };
+
+struct rz_gpio_priv {
+   void __iomem *io[REG_NR];
+   struct gpio_chip gpio_chip;
+   int nreg;
+};
+
+static inline struct rz_gpio_priv *gpio_to_priv(struct gpio_chip *chip)
+{
+   return gpiochip_get_data(chip);
+}
+
+static inline u16 rz_gpio_read(struct  gpio_chip *chip, int reg)
+{
+   return ioread16(gpio_to_priv(chip)->io[reg]);
+}
+
+static inline void rz_gpio_write(struct gpio_chip *chip, int reg, u16 val)
+{
+   iowrite16(val, gpio_to_priv(chip)->io[reg]);
+}
+
+static int rz_gpio_get(struct gpio_chip *chip, unsigned gpio)
+{
+   u16 tmp = rz_gpio_read(chip, REG_PPR);
+
+   return tmp & BIT(gpio);
+}
+
+static void rz_gpio_set(struct gpio_chip *chip, unsigned gpio, int value)
+{
+   u16 tmp;
+
+   if (gpio_to_priv(chip)->nreg == PORT0_NUM_REGS)
+   return;
+
+   tmp = rz_gpio_read(chip, REG_P);
+
+   if (value)
+   rz_gpio_write(chip, REG_P, tmp | BIT(gpio));
+   else
+   rz_gpio_write(chip, REG_P, tmp & ~BIT(gpio));
+}
+
+static int rz_gpio_direction_input(struct gpio_chip *chip, unsigned gpio)
+{
+   /* Set bit in PM register (input buffer enabled by PFC for the pin) */
+   rz_gpio_write(chip, REG_PM, rz_gpio_read(chip, REG_PM) | BIT(gpio));
+
+   return 0;
+}
+
+static int rz_gpio_direction_output(struct gpio_chip *chip, unsigned gpio,
+  int value)
+{
+
+   if (gpio_to_priv(chip)->nreg == PORT0_NUM_REGS)
+   return -EINVAL;
+
+   /* Write GPIO value before selecting output mode of pin */
+   rz_gpio_set(chip, gpio, value);
+
+   /* Clear bit in PM register to enable output */
+   rz_gpio_write(chip, REG_PM, rz_gpio_read(chip, REG_PM) & BIT(gpio));
+
+   return 0;
+}
+
+static int rz_gpio_get_direction(struct gpio_chip *chip, unsigned gpio)
+{
+   if (gpio_to_priv(chip)->nreg == PORT0_NUM_REGS)
+   return 1;
+
+   return rz_gpio_read(chip, REG_PM) & BIT(gpio);
+}
+
+static int rz_gpio_request(struct gpio_chip *chip, unsigned gpio)
+{
+   return gpiochip_generic_request(chip, gpio);
+}
+
+static void rz_gpio_free(struct gpio_chip *chip, unsigned gpio)
+{
+   gpiochip_generic_free(chip, gpio);
+
+   /* 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, gpio);
+}
+
+static int rz_gpio_probe(struct platform_device *pdev)
+{
+   struct rz_gpio_priv *p;
+   struct resource *io[REG_NR - 1];
+   struct