[RFC 0/8] ARM: imx27 pinctrl

[RFC 0/8] ARM: imx27 pinctrl

[Markus Pargmann]

Hi,

this series implements a imx27 pinctrl driver. imx27/21 have a register format
that is not compatible with the rest of imx series processors. I added some
functions to support it within the core imx pinctrl driver.

The gpio data registers are between iomux control registers. In this series,
the gpio devices are initialized as subdevices of the iomux device, sharing the
mapped registers.

Regards,

Markus


Markus Pargmann (8):
      gpio: mxc: Use devm_ functions
      gpio: mxc: Support for initialization as submodule
      pinctrl: imx: Move private struct to header file
      pinctrl: imx: Support imx21 register format
      pinctrl: imx27: imx27 pincontrol driver
      ARM: imx27: enable pinctrl
      ARM: dts: imx27 pin functions
      ARM: dts: imx27 pinctrl

 .../bindings/pinctrl/fsl,imx27-pinctrl.txt         |  53 ++
 arch/arm/boot/dts/imx27-pinfunc.h                  | 520 +++++++++++++++++++
 arch/arm/boot/dts/imx27.dtsi                       | 217 +++++---
 arch/arm/mach-imx/Kconfig                          |   2 +
 drivers/gpio/gpio-mxc.c                            |  51 +-
 drivers/pinctrl/Kconfig                            |   8 +
 drivers/pinctrl/Makefile                           |   1 +
 drivers/pinctrl/pinctrl-imx.c                      | 272 +++++++++-
 drivers/pinctrl/pinctrl-imx.h                      |  12 +
 drivers/pinctrl/pinctrl-imx27.c                    | 553 +++++++++++++++++++++
 include/linux/gpio-mxc.h                           |  17 +
 11 files changed, 1597 insertions(+), 109 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/pinctrl/fsl,imx27-pinctrl.txt
 create mode 100644 arch/arm/boot/dts/imx27-pinfunc.h
 create mode 100644 drivers/pinctrl/pinctrl-imx27.c
 create mode 100644 include/linux/gpio-mxc.h

[RFC 4/8] pinctrl: imx: Support imx21 register format

[Sascha Hauer]

Hi Markus,

On Sat, Jul 27, 2013 at 05:26:44PM +0200, Markus Pargmann wrote:
> The imx pinctrl core driver does not support the imx21 register format
> yet. This patch adds support for the strange iomux control registers of
> imx21 and imx27. Next to a slightly different DT parse function.
> 
> Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
> ---
>  drivers/pinctrl/pinctrl-imx.c | 264 +++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 250 insertions(+), 14 deletions(-)

Is it really worth sharing the i.MX1/21/27 support with the imx driver?
It seems most of the code is rewritten. I just tried and removed all
lines from pinctrl-imx.c you don't use. There's not much left to share.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[RFC 4/8] pinctrl: imx: Support imx21 register format

[Markus Pargmann]

Hi Sascha,

On Sun, Jul 28, 2013 at 10:34:36AM +0200, Sascha Hauer wrote:
> Hi Markus,
> 
> On Sat, Jul 27, 2013 at 05:26:44PM +0200, Markus Pargmann wrote:
> > The imx pinctrl core driver does not support the imx21 register format
> > yet. This patch adds support for the strange iomux control registers of
> > imx21 and imx27. Next to a slightly different DT parse function.
> > 
> > Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
> > ---
> >  drivers/pinctrl/pinctrl-imx.c | 264 +++++++++++++++++++++++++++++++++++++++---
> >  1 file changed, 250 insertions(+), 14 deletions(-)
> 
> Is it really worth sharing the i.MX1/21/27 support with the imx driver?
> It seems most of the code is rewritten. I just tried and removed all
> lines from pinctrl-imx.c you don't use. There's not much left to share.

Yes it's really not that much to share. I will move it to a seperate
file for imx1/21/27.

Thanks,

Markus

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[RFC 1/8] gpio: mxc: Use devm_ functions

[Shawn Guo]

On Sat, Jul 27, 2013 at 05:26:41PM +0200, Markus Pargmann wrote:
> Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
> ---
>  drivers/gpio/gpio-mxc.c | 30 +++++++++++-------------------
>  1 file changed, 11 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-mxc.c b/drivers/gpio/gpio-mxc.c
> index 7176743..cee040f 100644
> --- a/drivers/gpio/gpio-mxc.c
> +++ b/drivers/gpio/gpio-mxc.c
> @@ -405,33 +405,31 @@ static int mxc_gpio_probe(struct platform_device *pdev)
>  
>  	mxc_gpio_get_hw(pdev);
>  
> -	port = kzalloc(sizeof(struct mxc_gpio_port), GFP_KERNEL);
> +	port = devm_kzalloc(&pdev->dev, sizeof(struct mxc_gpio_port),
> +			GFP_KERNEL);
>  	if (!port)
>  		return -ENOMEM;
>  
>  	iores = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>  	if (!iores) {
> -		err = -ENODEV;
> -		goto out_kfree;
> +		return -ENODEV;
>  	}
>  
> -	if (!request_mem_region(iores->start, resource_size(iores),
> -				pdev->name)) {
> -		err = -EBUSY;
> -		goto out_kfree;
> +	if (!devm_request_mem_region(&pdev->dev, iores->start,
> +				resource_size(iores), pdev->name)) {
> +		return -EBUSY;
>  	}
>  
> -	port->base = ioremap(iores->start, resource_size(iores));
> +	port->base = devm_ioremap(&pdev->dev, iores->start,
> +			resource_size(iores));

It can be even cleaner by using devm_ioremap_resource().

>  	if (!port->base) {
> -		err = -ENOMEM;
> -		goto out_release_mem;
> +		return -ENOMEM;
>  	}

We should probably drop the braces then.

Shawn

>  
>  	port->irq_high = platform_get_irq(pdev, 1);
>  	port->irq = platform_get_irq(pdev, 0);
>  	if (port->irq < 0) {
> -		err = -EINVAL;
> -		goto out_iounmap;
> +		return -EINVAL;
>  	}
>  
>  	/* disable the interrupt and clear the status */
> @@ -462,7 +460,7 @@ static int mxc_gpio_probe(struct platform_device *pdev)
>  			 port->base + GPIO_DR, NULL,
>  			 port->base + GPIO_GDIR, NULL, 0);
>  	if (err)
> -		goto out_iounmap;
> +		return err;
>  
>  	port->bgc.gc.to_irq = mxc_gpio_to_irq;
>  	port->bgc.gc.base = (pdev->id < 0) ? of_alias_get_id(np, "gpio") * 32 :
> @@ -498,12 +496,6 @@ out_gpiochip_remove:
>  	WARN_ON(gpiochip_remove(&port->bgc.gc) < 0);
>  out_bgpio_remove:
>  	bgpio_remove(&port->bgc);
> -out_iounmap:
> -	iounmap(port->base);
> -out_release_mem:
> -	release_mem_region(iores->start, resource_size(iores));
> -out_kfree:
> -	kfree(port);
>  	dev_info(&pdev->dev, "%s failed with errno %d\n", __func__, err);
>  	return err;
>  }
> -- 
> 1.8.3.2
> 

[RFC 2/8] gpio: mxc: Support for initialization as submodule

[Shawn Guo]

On Sat, Jul 27, 2013 at 05:26:42PM +0200, Markus Pargmann wrote:
> On imx27 and imx21, there is no clear seperation between iomux control
> registers and GPIO data registers. For easier pingroup definitions, the
> gpio drivers will be initialized as subnodes of the iomux controller. It
> is necessary to share the registers between iomux and gpio.
> 
> This patch adds support to pass a register memory region via platform
> data.
> 
> Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
> ---
>  drivers/gpio/gpio-mxc.c  | 35 +++++++++++++++++++----------------
>  include/linux/gpio-mxc.h | 17 +++++++++++++++++
>  2 files changed, 36 insertions(+), 16 deletions(-)
>  create mode 100644 include/linux/gpio-mxc.h

Since the header is created only for holding platform_data, we should
probably put it into include/linux/platform_data/.
> 
> diff --git a/drivers/gpio/gpio-mxc.c b/drivers/gpio/gpio-mxc.c
> index cee040f..0c4e32c 100644
> --- a/drivers/gpio/gpio-mxc.c
> +++ b/drivers/gpio/gpio-mxc.c
> @@ -26,6 +26,7 @@
>  #include <linux/irqdomain.h>
>  #include <linux/irqchip/chained_irq.h>
>  #include <linux/gpio.h>
> +#include <linux/gpio-mxc.h>
>  #include <linux/platform_device.h>
>  #include <linux/slab.h>
>  #include <linux/basic_mmio_gpio.h>
> @@ -400,6 +401,7 @@ static int mxc_gpio_probe(struct platform_device *pdev)
>  	struct device_node *np = pdev->dev.of_node;
>  	struct mxc_gpio_port *port;
>  	struct resource *iores;
> +	struct mxc_gpio_platform_data *pdata = pdev->dev.platform_data;
>  	int irq_base;
>  	int err;
>  
> @@ -410,27 +412,28 @@ static int mxc_gpio_probe(struct platform_device *pdev)
>  	if (!port)
>  		return -ENOMEM;
>  
> -	iores = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> -	if (!iores) {
> -		return -ENODEV;
> -	}
> -
> -	if (!devm_request_mem_region(&pdev->dev, iores->start,
> -				resource_size(iores), pdev->name)) {
> -		return -EBUSY;
> -	}
> -
> -	port->base = devm_ioremap(&pdev->dev, iores->start,
> -			resource_size(iores));
> -	if (!port->base) {
> -		return -ENOMEM;
> +	if (pdata) {
> +		port->base = pdata->base;
> +		iores = NULL;

Why is this needed?

> +	} else {
> +		iores = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +		if (!iores)
> +			return -ENODEV;
> +
> +		if (!devm_request_mem_region(&pdev->dev, iores->start,
> +					resource_size(iores), pdev->name))
> +			return -EBUSY;
> +
> +		port->base = devm_ioremap(&pdev->dev, iores->start,
> +				resource_size(iores));
> +		if (!port->base)
> +			return -ENOMEM;
>  	}
>  
>  	port->irq_high = platform_get_irq(pdev, 1);
>  	port->irq = platform_get_irq(pdev, 0);
> -	if (port->irq < 0) {
> +	if (port->irq < 0)
>  		return -EINVAL;
> -	}

The change belongs to patch #1.

Shawn

>  
>  	/* disable the interrupt and clear the status */
>  	writel(0, port->base + GPIO_IMR);
> diff --git a/include/linux/gpio-mxc.h b/include/linux/gpio-mxc.h
> new file mode 100644
> index 0000000..6be51bc
> --- /dev/null
> +++ b/include/linux/gpio-mxc.h
> @@ -0,0 +1,17 @@
> +#ifndef _LINUX_GPIO_MXC_H
> +#define _LINUX_GPIO_MXC_H
> +
> +#include <linux/types.h>
> +
> +/*
> + * MXC GPIO driver platform data. If this platform data is passed to the
> + * driver, it will use the memory base defined in the struct. This is used for
> + * iomuxc drivers on imx21 and imx27, where the GPIO data register is embedded
> + * between the iomuxc registers.
> + */
> +
> +struct mxc_gpio_platform_data {
> +	void __iomem *base;
> +};
> +
> +#endif /* _LINUX_GPIO_MXC_H */
> -- 
> 1.8.3.2
> 

[RFC 1/8] gpio: mxc: Use devm_ functions

[Fabio Estevam]

On Tue, Jul 30, 2013 at 12:14 AM, Shawn Guo <shawn.guo@linaro.org> wrote:

> It can be even cleaner by using devm_ioremap_resource().

I have already sent a patch that converts to devm_ioremap_resource()
and it is already in Linus Walleij's tree.

[RFC 5/8] pinctrl: imx27: imx27 pincontrol driver

[Shawn Guo]

On Sat, Jul 27, 2013 at 05:26:45PM +0200, Markus Pargmann wrote:
> +static struct imx_pinctrl_soc_info imx27_pinctrl_info = {
> + .pins = imx27_pinctrl_pads,
> + .npins = ARRAY_SIZE(imx27_pinctrl_pads),
> +};
> +
> +static struct of_device_id imx27_pinctrl_of_match[] = {
> + { .compatible = "fsl,imx27-iomuxc", },
> + { /* sentinel */ }
> +};
> +
> +static struct of_device_id imx21_gpio_of_match[] = {
> + { .compatible = "fsl,imx21-gpio", },

Since we also have "fsl,imx27-gpio" in the compatible string, we can
use it here.

> + { /* sentinel */ }
> +};
> +
> +struct imx27_pinctrl_private {
> + int num_gpio_childs;

Maybe I missed something.  But I see this member is used nowhere.

> + struct platform_device **gpio_dev;
> + struct mxc_gpio_platform_data *gpio_pdata;
> +};

I do not see too much point to have this structure at all.

Shawn

> +
> +static int imx27_pinctrl_probe(struct platform_device *pdev)
> +{
> + struct device_node *child;
> + struct imx_pinctrl *ipctl;
> + struct imx27_pinctrl_private *pctl;
> + int ret;
> + u32 base_addr;
> + int num_gpios = 0;
> + int i = 0;
> +
> + ret = of_property_read_u32(pdev->dev.of_node, "reg", &base_addr);
> + if (ret)
> + return ret;
> +
> + ret = imx_pinctrl_probe(pdev, &imx27_pinctrl_info);
> + if (ret)
> + return ret;
> +
> + pctl = devm_kzalloc(&pdev->dev, sizeof(*pctl), GFP_KERNEL);
> + if (!pctl)
> + return -ENOMEM;
> +
> + ipctl = platform_get_drvdata(pdev);
> +
> + for_each_available_child_of_node(pdev->dev.of_node, child) {
> + u32 memoffset;
> + if (!of_match_node(imx21_gpio_of_match, child))
> + continue;
> + ret = of_property_read_u32(child, "reg", &memoffset);
> + if (ret)
> + continue;
> + ++num_gpios;
> + }
> +
> + pctl->num_gpio_childs = num_gpios;
> + pctl->gpio_dev = devm_kzalloc(&pdev->dev,
> + sizeof(*pctl->gpio_dev) * num_gpios, GFP_KERNEL);
> + pctl->gpio_pdata = devm_kzalloc(&pdev->dev,
> + sizeof(*pctl->gpio_pdata) * num_gpios, GFP_KERNEL);
> +
> + if (!pctl->gpio_pdata || !pctl->gpio_dev)
> + return -ENOMEM;
> +
> +
> + for_each_available_child_of_node(pdev->dev.of_node, child) {
> + u32 memoffset;
> +
> + if (!of_match_node(imx21_gpio_of_match, child))
> + continue;
> + ret = of_property_read_u32(child, "reg", &memoffset);
> + if (ret)
> + continue;
> +
> + memoffset -= base_addr;
> + pctl->gpio_pdata[i].base = ipctl->base + memoffset;
> +
> + pctl->gpio_dev[i] = of_device_alloc(child, NULL, &pdev->dev);
> + pctl->gpio_dev[i]->dev.platform_data = &pctl->gpio_pdata[i];
> + pctl->gpio_dev[i]->dev.bus = &platform_bus_type;
> +
> + ret = of_device_add(pctl->gpio_dev[i]);
> + if (ret) {
> + dev_err(&pdev->dev,
> + "Failed to add child gpio device\n");
> + goto gpio_err;
> + }
> + ++i;
> + }
> + return 0;
> +gpio_err:
> + while (i--)
> + put_device(&pctl->gpio_dev[i]->dev);
> + return ret;
> +}
> +
> +static struct platform_driver imx27_pinctrl_driver = {
> + .driver = {
> + .name = "imx27-pinctrl",
> + .owner = THIS_MODULE,
> + .of_match_table = of_match_ptr(imx27_pinctrl_of_match),
> + },
> + .probe = imx27_pinctrl_probe,
> + .remove = imx_pinctrl_remove,
> +};
> +
> +static int __init imx27_pinctrl_init(void)
> +{
> + return platform_driver_register(&imx27_pinctrl_driver);
> +}
> +arch_initcall(imx27_pinctrl_init);
> +
> +static void __exit imx27_pinctrl_exit(void)
> +{
> + platform_driver_unregister(&imx27_pinctrl_driver);
> +}
> +module_exit(imx27_pinctrl_exit);
> +MODULE_AUTHOR("Markus Pargmann <mpa@pengutronix.de>");
> +MODULE_DESCRIPTION("Freescale IMX27 pinctrl driver");
> +MODULE_LICENSE("GPL v2");
> --
> 1.8.3.2
>

[RFC 2/8] gpio: mxc: Support for initialization as submodule

[Markus Pargmann]

On Tue, Jul 30, 2013 at 11:43:57AM +0800, Shawn Guo wrote:
> On Sat, Jul 27, 2013 at 05:26:42PM +0200, Markus Pargmann wrote:
> > On imx27 and imx21, there is no clear seperation between iomux control
> > registers and GPIO data registers. For easier pingroup definitions, the
> > gpio drivers will be initialized as subnodes of the iomux controller. It
> > is necessary to share the registers between iomux and gpio.
> > 
> > This patch adds support to pass a register memory region via platform
> > data.
> > 
> > Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
> > ---
> >  drivers/gpio/gpio-mxc.c  | 35 +++++++++++++++++++----------------
> >  include/linux/gpio-mxc.h | 17 +++++++++++++++++
> >  2 files changed, 36 insertions(+), 16 deletions(-)
> >  create mode 100644 include/linux/gpio-mxc.h
> 
> Since the header is created only for holding platform_data, we should
> probably put it into include/linux/platform_data/.

Yes, I moved the header to include/linux/platform_data/gpio-mxc.h.

> > 
> > diff --git a/drivers/gpio/gpio-mxc.c b/drivers/gpio/gpio-mxc.c
> > index cee040f..0c4e32c 100644
> > --- a/drivers/gpio/gpio-mxc.c
> > +++ b/drivers/gpio/gpio-mxc.c
> > @@ -26,6 +26,7 @@
> >  #include <linux/irqdomain.h>
> >  #include <linux/irqchip/chained_irq.h>
> >  #include <linux/gpio.h>
> > +#include <linux/gpio-mxc.h>
> >  #include <linux/platform_device.h>
> >  #include <linux/slab.h>
> >  #include <linux/basic_mmio_gpio.h>
> > @@ -400,6 +401,7 @@ static int mxc_gpio_probe(struct platform_device *pdev)
> >  	struct device_node *np = pdev->dev.of_node;
> >  	struct mxc_gpio_port *port;
> >  	struct resource *iores;
> > +	struct mxc_gpio_platform_data *pdata = pdev->dev.platform_data;
> >  	int irq_base;
> >  	int err;
> >  
> > @@ -410,27 +412,28 @@ static int mxc_gpio_probe(struct platform_device *pdev)
> >  	if (!port)
> >  		return -ENOMEM;
> >  
> > -	iores = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > -	if (!iores) {
> > -		return -ENODEV;
> > -	}
> > -
> > -	if (!devm_request_mem_region(&pdev->dev, iores->start,
> > -				resource_size(iores), pdev->name)) {
> > -		return -EBUSY;
> > -	}
> > -
> > -	port->base = devm_ioremap(&pdev->dev, iores->start,
> > -			resource_size(iores));
> > -	if (!port->base) {
> > -		return -ENOMEM;
> > +	if (pdata) {
> > +		port->base = pdata->base;
> > +		iores = NULL;
> 
> Why is this needed?

The memory regions used for iomux control overlap the regions used by
the gpio driver. So we need to share the memory with the pinctrl driver.

> 
> > +	} else {
> > +		iores = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +		if (!iores)
> > +			return -ENODEV;
> > +
> > +		if (!devm_request_mem_region(&pdev->dev, iores->start,
> > +					resource_size(iores), pdev->name))
> > +			return -EBUSY;
> > +
> > +		port->base = devm_ioremap(&pdev->dev, iores->start,
> > +				resource_size(iores));
> > +		if (!port->base)
> > +			return -ENOMEM;
> >  	}
> >  
> >  	port->irq_high = platform_get_irq(pdev, 1);
> >  	port->irq = platform_get_irq(pdev, 0);
> > -	if (port->irq < 0) {
> > +	if (port->irq < 0)
> >  		return -EINVAL;
> > -	}
> 
> The change belongs to patch #1.

Removed from this patch.

Thanks,

Markus

> 
> Shawn
> 
> >  
> >  	/* disable the interrupt and clear the status */
> >  	writel(0, port->base + GPIO_IMR);
> > diff --git a/include/linux/gpio-mxc.h b/include/linux/gpio-mxc.h
> > new file mode 100644
> > index 0000000..6be51bc
> > --- /dev/null
> > +++ b/include/linux/gpio-mxc.h
> > @@ -0,0 +1,17 @@
> > +#ifndef _LINUX_GPIO_MXC_H
> > +#define _LINUX_GPIO_MXC_H
> > +
> > +#include <linux/types.h>
> > +
> > +/*
> > + * MXC GPIO driver platform data. If this platform data is passed to the
> > + * driver, it will use the memory base defined in the struct. This is used for
> > + * iomuxc drivers on imx21 and imx27, where the GPIO data register is embedded
> > + * between the iomuxc registers.
> > + */
> > +
> > +struct mxc_gpio_platform_data {
> > +	void __iomem *base;
> > +};
> > +
> > +#endif /* _LINUX_GPIO_MXC_H */
> > -- 
> > 1.8.3.2
> > 
> 
> 

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[RFC 1/8] gpio: mxc: Use devm_ functions

[Markus Pargmann]

On Tue, Jul 30, 2013 at 12:45:33AM -0300, Fabio Estevam wrote:
> On Tue, Jul 30, 2013 at 12:14 AM, Shawn Guo <shawn.guo@linaro.org> wrote:
> 
> > It can be even cleaner by using devm_ioremap_resource().
> 
> I have already sent a patch that converts to devm_ioremap_resource()
> and it is already in Linus Walleij's tree.
> 
Thanks, I will remove this patch and use yours.

Regards,

Markus

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[RFC 5/8] pinctrl: imx27: imx27 pincontrol driver

[Markus Pargmann]

On Tue, Jul 30, 2013 at 02:09:57PM +0800, Shawn Guo wrote:
> On Sat, Jul 27, 2013 at 05:26:45PM +0200, Markus Pargmann wrote:
> > +static struct imx_pinctrl_soc_info imx27_pinctrl_info = {
> > + .pins = imx27_pinctrl_pads,
> > + .npins = ARRAY_SIZE(imx27_pinctrl_pads),
> > +};
> > +
> > +static struct of_device_id imx27_pinctrl_of_match[] = {
> > + { .compatible = "fsl,imx27-iomuxc", },
> > + { /* sentinel */ }
> > +};
> > +
> > +static struct of_device_id imx21_gpio_of_match[] = {
> > + { .compatible = "fsl,imx21-gpio", },
> 
> Since we also have "fsl,imx27-gpio" in the compatible string, we can
> use it here.

Yes, but I moved the child gpio detection into imx1 core driver now,
where I use "fsl,imx1-gpio" and "fsl,imx21-gpio".

> 
> > + { /* sentinel */ }
> > +};
> > +
> > +struct imx27_pinctrl_private {
> > + int num_gpio_childs;
> 
> Maybe I missed something.  But I see this member is used nowhere.
> 
> > + struct platform_device **gpio_dev;
> > + struct mxc_gpio_platform_data *gpio_pdata;
> > +};
> 
> I do not see too much point to have this structure at all.

I forgot to add put_device calls in the driver remove function. So the
structure is used now.

Thanks,

Markus

> 
> Shawn
> 
> > +
> > +static int imx27_pinctrl_probe(struct platform_device *pdev)
> > +{
> > + struct device_node *child;
> > + struct imx_pinctrl *ipctl;
> > + struct imx27_pinctrl_private *pctl;
> > + int ret;
> > + u32 base_addr;
> > + int num_gpios = 0;
> > + int i = 0;
> > +
> > + ret = of_property_read_u32(pdev->dev.of_node, "reg", &base_addr);
> > + if (ret)
> > + return ret;
> > +
> > + ret = imx_pinctrl_probe(pdev, &imx27_pinctrl_info);
> > + if (ret)
> > + return ret;
> > +
> > + pctl = devm_kzalloc(&pdev->dev, sizeof(*pctl), GFP_KERNEL);
> > + if (!pctl)
> > + return -ENOMEM;
> > +
> > + ipctl = platform_get_drvdata(pdev);
> > +
> > + for_each_available_child_of_node(pdev->dev.of_node, child) {
> > + u32 memoffset;
> > + if (!of_match_node(imx21_gpio_of_match, child))
> > + continue;
> > + ret = of_property_read_u32(child, "reg", &memoffset);
> > + if (ret)
> > + continue;
> > + ++num_gpios;
> > + }
> > +
> > + pctl->num_gpio_childs = num_gpios;
> > + pctl->gpio_dev = devm_kzalloc(&pdev->dev,
> > + sizeof(*pctl->gpio_dev) * num_gpios, GFP_KERNEL);
> > + pctl->gpio_pdata = devm_kzalloc(&pdev->dev,
> > + sizeof(*pctl->gpio_pdata) * num_gpios, GFP_KERNEL);
> > +
> > + if (!pctl->gpio_pdata || !pctl->gpio_dev)
> > + return -ENOMEM;
> > +
> > +
> > + for_each_available_child_of_node(pdev->dev.of_node, child) {
> > + u32 memoffset;
> > +
> > + if (!of_match_node(imx21_gpio_of_match, child))
> > + continue;
> > + ret = of_property_read_u32(child, "reg", &memoffset);
> > + if (ret)
> > + continue;
> > +
> > + memoffset -= base_addr;
> > + pctl->gpio_pdata[i].base = ipctl->base + memoffset;
> > +
> > + pctl->gpio_dev[i] = of_device_alloc(child, NULL, &pdev->dev);
> > + pctl->gpio_dev[i]->dev.platform_data = &pctl->gpio_pdata[i];
> > + pctl->gpio_dev[i]->dev.bus = &platform_bus_type;
> > +
> > + ret = of_device_add(pctl->gpio_dev[i]);
> > + if (ret) {
> > + dev_err(&pdev->dev,
> > + "Failed to add child gpio device\n");
> > + goto gpio_err;
> > + }
> > + ++i;
> > + }
> > + return 0;
> > +gpio_err:
> > + while (i--)
> > + put_device(&pctl->gpio_dev[i]->dev);
> > + return ret;
> > +}
> > +
> > +static struct platform_driver imx27_pinctrl_driver = {
> > + .driver = {
> > + .name = "imx27-pinctrl",
> > + .owner = THIS_MODULE,
> > + .of_match_table = of_match_ptr(imx27_pinctrl_of_match),
> > + },
> > + .probe = imx27_pinctrl_probe,
> > + .remove = imx_pinctrl_remove,
> > +};
> > +
> > +static int __init imx27_pinctrl_init(void)
> > +{
> > + return platform_driver_register(&imx27_pinctrl_driver);
> > +}
> > +arch_initcall(imx27_pinctrl_init);
> > +
> > +static void __exit imx27_pinctrl_exit(void)
> > +{
> > + platform_driver_unregister(&imx27_pinctrl_driver);
> > +}
> > +module_exit(imx27_pinctrl_exit);
> > +MODULE_AUTHOR("Markus Pargmann <mpa@pengutronix.de>");
> > +MODULE_DESCRIPTION("Freescale IMX27 pinctrl driver");
> > +MODULE_LICENSE("GPL v2");
> > --
> > 1.8.3.2
> >
> 
> 

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[RFC 2/8] gpio: mxc: Support for initialization as submodule

[Shawn Guo]

On Tue, Jul 30, 2013 at 10:39:29AM +0200, Markus Pargmann wrote:
> > > +	if (pdata) {
> > > +		port->base = pdata->base;
> > > +		iores = NULL;
> > 
> > Why is this needed?
> 
> The memory regions used for iomux control overlap the regions used by
> the gpio driver. So we need to share the memory with the pinctrl driver.

I meant for iores = NULL;

Shawn

[RFC 4/8] pinctrl: imx: Support imx21 register format

[Linus Walleij]

On Sun, Jul 28, 2013 at 12:22 PM, Markus Pargmann <mpa@pengutronix.de> wrote:

>> Is it really worth sharing the i.MX1/21/27 support with the imx driver?
>> It seems most of the code is rewritten. I just tried and removed all
>> lines from pinctrl-imx.c you don't use. There's not much left to share.
>
> Yes it's really not that much to share. I will move it to a seperate
> file for imx1/21/27.

OK I'll wait for a v2 version of this patch set before reviewing.

Yours,
Linus Walleij

[RFC 2/8] gpio: mxc: Support for initialization as submodule

[Markus Pargmann]

On Tue, Jul 30, 2013 at 05:37:43PM +0800, Shawn Guo wrote:
> On Tue, Jul 30, 2013 at 10:39:29AM +0200, Markus Pargmann wrote:
> > > > +	if (pdata) {
> > > > +		port->base = pdata->base;
> > > > +		iores = NULL;
> > > 
> > > Why is this needed?
> > 
> > The memory regions used for iomux control overlap the regions used by
> > the gpio driver. So we need to share the memory with the pinctrl driver.
> 
> I meant for iores = NULL;

Not necessary, I removed it.

Thanks,

Markus

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |