Re: [PATCH 1/3 v2] GPIO: gpio-dwapb: Enable platform driver binding to MFD driver

["Shevchenko, Andriy" <andriy.shevchenko@xxxxxxxxx>]

On Fri, 2014-09-05 at 07:53 -0700, Weike Chen wrote:
> The Synopsys DesignWare APB GPIO driver only supports open firmware devices.
> But, like Intel Quark X1000 SOC, which has a single PCI function exporting
> a GPIO and an I2C controller, it is a Multifunction device. This patch is
> to enable the current Synopsys DesignWare APB GPIO driver to support the
> Multifunction device which exports the designware GPIO controller.

Few comments below.

> 
> Reviewed-by: Hock Leong Kweh <hock.leong.kweh@xxxxxxxxx>
> Reviewed-by: Shevchenko, Andriy <andriy.shevchenko@xxxxxxxxx>
> Signed-off-by: Weike Chen <alvin.chen@xxxxxxxxx>
> ---
>  drivers/gpio/Kconfig                     |    1 -
>  drivers/gpio/gpio-dwapb.c                |  233 +++++++++++++++++++++++-------
>  include/linux/platform_data/gpio-dwapb.h |   32 ++++
>  3 files changed, 211 insertions(+), 55 deletions(-)
>  create mode 100644 include/linux/platform_data/gpio-dwapb.h
> 
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index 9de1515..8250a44 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -136,7 +136,6 @@ config GPIO_DWAPB
>  	tristate "Synopsys DesignWare APB GPIO driver"
>  	select GPIO_GENERIC
>  	select GENERIC_IRQ_CHIP
> -	depends on OF_GPIO
>  	help
>  	  Say Y or M here to build support for the Synopsys DesignWare APB
>  	  GPIO block.
> diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c
> index d6618a6..f2264a2 100644
> --- a/drivers/gpio/gpio-dwapb.c
> +++ b/drivers/gpio/gpio-dwapb.c
> @@ -21,6 +21,7 @@
>  #include <linux/of_irq.h>
>  #include <linux/platform_device.h>
>  #include <linux/spinlock.h>
> +#include <linux/platform_data/gpio-dwapb.h>
>  
>  #define GPIO_SWPORTA_DR		0x00
>  #define GPIO_SWPORTA_DDR	0x04
> @@ -84,11 +85,10 @@ static void dwapb_toggle_trigger(struct dwapb_gpio *gpio, unsigned int offs)
>  	writel(v, gpio->regs + GPIO_INT_POLARITY);
>  }
>  
> -static void dwapb_irq_handler(u32 irq, struct irq_desc *desc)
> +static u32 _dwapb_irq_handler(struct dwapb_gpio *gpio)

What about dwapb_do_irq() ?


>  {
> -	struct dwapb_gpio *gpio = irq_get_handler_data(irq);
> -	struct irq_chip *chip = irq_desc_get_chip(desc);
>  	u32 irq_status = readl_relaxed(gpio->regs + GPIO_INTSTATUS);
> +	u32 ret = irq_status;
>  
>  	while (irq_status) {
>  		int hwirq = fls(irq_status) - 1;
> @@ -102,6 +102,16 @@ static void dwapb_irq_handler(u32 irq, struct irq_desc *desc)
>  			dwapb_toggle_trigger(gpio, hwirq);
>  	}
>  
> +	return ret;
> +}
> +
> +static void dwapb_irq_handler(u32 irq, struct irq_desc *desc)
> +{
> +	struct dwapb_gpio *gpio = irq_get_handler_data(irq);
> +	struct irq_chip *chip = irq_desc_get_chip(desc);
> +
> +	_dwapb_irq_handler(gpio);
> +
>  	if (chip->irq_eoi)
>  		chip->irq_eoi(irq_desc_get_irq_data(desc));
>  }
> @@ -207,22 +217,26 @@ static int dwapb_irq_set_type(struct irq_data *d, u32 type)
>  	return 0;
>  }
>  
> +static irqreturn_t dwapb_irq_handler_mfd(int irq, void *dev_id)
> +{
> +	u32 worked;
> +	struct dwapb_gpio *gpio = (struct dwapb_gpio *)dev_id;

No need to cast explicitly from void *.

> +
> +	worked = _dwapb_irq_handler(gpio);
> +
> +	return worked ? IRQ_HANDLED : IRQ_NONE;
> +}
> +
>  static void dwapb_configure_irqs(struct dwapb_gpio *gpio,
> -				 struct dwapb_gpio_port *port)
> +				 struct dwapb_gpio_port *port,
> +				 struct dwapb_port_property *pp)
>  {
>  	struct gpio_chip *gc = &port->bgc.gc;
> -	struct device_node *node =  gc->of_node;
> -	struct irq_chip_generic	*irq_gc;
> +	struct device_node *node = pp->node;
> +	struct irq_chip_generic	*irq_gc = NULL;
>  	unsigned int hwirq, ngpio = gc->ngpio;
>  	struct irq_chip_type *ct;
> -	int err, irq, i;
> -
> -	irq = irq_of_parse_and_map(node, 0);
> -	if (!irq) {
> -		dev_warn(gpio->dev, "no irq for bank %s\n",
> -			port->bgc.gc.of_node->full_name);
> -		return;
> -	}
> +	int err, i;
>  
>  	gpio->domain = irq_domain_add_linear(node, ngpio,
>  					     &irq_generic_chip_ops, gpio);
> @@ -269,8 +283,24 @@ static void dwapb_configure_irqs(struct dwapb_gpio *gpio,
>  	irq_gc->chip_types[1].type = IRQ_TYPE_EDGE_BOTH;
>  	irq_gc->chip_types[1].handler = handle_edge_irq;
>  
> -	irq_set_chained_handler(irq, dwapb_irq_handler);
> -	irq_set_handler_data(irq, gpio);
> +	if (!pp->irq_shared) {
> +		irq_set_chained_handler(pp->irq, dwapb_irq_handler);
> +		irq_set_handler_data(pp->irq, gpio);
> +	} else {
> +		/*
> +		 * Request a shared IRQ since where MFD would have devices
> +		 * using the same irq pin
> +		 */
> +		err = devm_request_irq(gpio->dev, pp->irq,
> +				       dwapb_irq_handler_mfd,
> +				       IRQF_SHARED, "gpio-dwapb-mfd", gpio);
> +		if (err) {
> +			dev_err(gpio->dev, "error requesting IRQ\n");
> +			irq_domain_remove(gpio->domain);
> +			gpio->domain = NULL;
> +			return;
> +		}
> +	}
>  
>  	for (hwirq = 0 ; hwirq < ngpio ; hwirq++)
>  		irq_create_mapping(gpio->domain, hwirq);
> @@ -296,57 +326,42 @@ static void dwapb_irq_teardown(struct dwapb_gpio *gpio)
>  }
>  
>  static int dwapb_gpio_add_port(struct dwapb_gpio *gpio,
> -			       struct device_node *port_np,
> +			       struct dwapb_port_property *pp,
>  			       unsigned int offs)
>  {
>  	struct dwapb_gpio_port *port;
> -	u32 port_idx, ngpio;
>  	void __iomem *dat, *set, *dirout;
>  	int err;
>  
> -	if (of_property_read_u32(port_np, "reg", &port_idx) ||
> -		port_idx >= DWAPB_MAX_PORTS) {
> -		dev_err(gpio->dev, "missing/invalid port index for %s\n",
> -			port_np->full_name);
> -		return -EINVAL;
> -	}
> -
>  	port = &gpio->ports[offs];
>  	port->gpio = gpio;
>  
> -	if (of_property_read_u32(port_np, "snps,nr-gpios", &ngpio)) {
> -		dev_info(gpio->dev, "failed to get number of gpios for %s\n",
> -			 port_np->full_name);
> -		ngpio = 32;
> -	}
> -
> -	dat = gpio->regs + GPIO_EXT_PORTA + (port_idx * GPIO_EXT_PORT_SIZE);
> -	set = gpio->regs + GPIO_SWPORTA_DR + (port_idx * GPIO_SWPORT_DR_SIZE);
> +	dat = gpio->regs + GPIO_EXT_PORTA + (pp->idx * GPIO_EXT_PORT_SIZE);
> +	set = gpio->regs + GPIO_SWPORTA_DR + (pp->idx * GPIO_SWPORT_DR_SIZE);
>  	dirout = gpio->regs + GPIO_SWPORTA_DDR +
> -		(port_idx * GPIO_SWPORT_DDR_SIZE);
> +		(pp->idx * GPIO_SWPORT_DDR_SIZE);
>  
>  	err = bgpio_init(&port->bgc, gpio->dev, 4, dat, set, NULL, dirout,
>  			 NULL, false);
>  	if (err) {
>  		dev_err(gpio->dev, "failed to init gpio chip for %s\n",
> -			port_np->full_name);
> +			pp->name);
>  		return err;
>  	}
>  
> -	port->bgc.gc.ngpio = ngpio;
> -	port->bgc.gc.of_node = port_np;
> +#ifdef CONFIG_OF_GPIO

Do we really need this #ifdef ?
of_node will be NULL anyway, or I missed something?

> +	port->bgc.gc.of_node = pp->node;
> +#endif
> +	port->bgc.gc.ngpio = pp->ngpio;
> +	port->bgc.gc.base = pp->gpio_base;
>  
> -	/*
> -	 * Only port A can provide interrupts in all configurations of the IP.
> -	 */
> -	if (port_idx == 0 &&
> -	    of_property_read_bool(port_np, "interrupt-controller"))
> -		dwapb_configure_irqs(gpio, port);
> +	if (pp->irq)

irq == 0 is a valid hwirq (hardware irq) number. Yes, there is unlikely
we have it somewhere, but still it's possible. And yes, IRQ framework
doesn't work with virq == 0 (*virtual* irq), but accepts hwirq == 0. I
recommend to use int type for irq line number, and recognize negative
value (usually -1) as no irq needed / found.

> +		dwapb_configure_irqs(gpio, port, pp);
>  
>  	err = gpiochip_add(&port->bgc.gc);
>  	if (err)
>  		dev_err(gpio->dev, "failed to register gpiochip for %s\n",
> -			port_np->full_name);
> +			pp->name);
>  	else
>  		port->is_registered = true;
>  
> @@ -362,25 +377,130 @@ static void dwapb_gpio_unregister(struct dwapb_gpio *gpio)
>  			gpiochip_remove(&gpio->ports[m].bgc.gc);
>  }
>  
> +#ifdef CONFIG_OF_GPIO
> +
> +static struct dwapb_platform_data *
> +dwapb_gpio_get_pdata_of(struct device *dev)
> +{
> +	struct device_node *node, *port_np;
> +	struct dwapb_platform_data *pdata;
> +	struct dwapb_port_property *pp;
> +	int nports;
> +	int i;
> +
> +	node = dev->of_node;
> +	if (!node)
> +		return ERR_PTR(-ENODEV);
> +
> +	nports = of_get_child_count(node);
> +	if (nports == 0)
> +		return ERR_PTR(-ENODEV);
> +
> +	pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
> +	if (!pdata)
> +		return ERR_PTR(-ENOMEM);
> +
> +	pdata->properties = devm_kcalloc(dev, nports, sizeof(*pp), GFP_KERNEL);
> +	if (!pdata->properties)
> +		return ERR_PTR(-ENOMEM);
> +
> +	pdata->nports = nports;
> +
> +	i = 0;
> +	for_each_child_of_node(node, port_np) {
> +		pp = &pdata->properties[i++];
> +		pp->node = port_np;
> +
> +		if (of_property_read_u32(port_np, "reg", &pp->idx) ||
> +		    pp->idx >= DWAPB_MAX_PORTS) {
> +			dev_err(dev, "missing/invalid port index for %s\n",
> +				port_np->full_name);
> +			return ERR_PTR(-EINVAL);
> +		}
> +
> +		if (of_property_read_u32(port_np, "snps,nr-gpios",
> +					 &pp->ngpio)) {
> +			dev_info(dev, "failed to get number of gpios for %s\n",
> +				 port_np->full_name);
> +			pp->ngpio = 32;
> +		}
> +
> +		/*
> +		 * Only port A can provide interrupts in all configurations of
> +		 * the IP.
> +		 */
> +		if (pp->idx == 0 &&
> +		    of_property_read_bool(port_np, "interrupt-controller")) {
> +			pp->irq = irq_of_parse_and_map(port_np, 0);
> +			if (!pp->irq) {
> +				dev_warn(dev, "no irq for bank %s\n",
> +					 port_np->full_name);
> +			}
> +		} else {
> +			pp->irq	= 0;
> +		}
> +
> +		pp->irq_shared	= false;
> +		pp->gpio_base	= -1;
> +		pp->name	= port_np->full_name;
> +	}
> +
> +	return pdata;
> +}
> +
> +static inline void dwapb_free_pdata_of(struct device *dev,
> +				       struct dwapb_platform_data *pdata)
> +{
> +	if (!pdata)
> +		return;
> +	if (pdata->properties)
> +		devm_kfree(dev, pdata->properties);
> +	devm_kfree(dev, pdata);
> +}
> +
> +#else
> +
> +static inline struct dwapb_platform_data *
> +dwapb_gpio_get_pdata_of(struct device *dev)
> +{
> +	return ERR_PTR(-ENODEV);
> +}
> +
> +static inline void dwapb_free_pdata_of(struct device *dev,
> +				       struct dwapb_platform_data *pdata)
> +{
> +}
> +
> +#endif
> +
>  static int dwapb_gpio_probe(struct platform_device *pdev)
>  {
> +	int i;
> +	bool is_of;

is_pdata_alloc (it might be not only OF case in future).



>  	struct resource *res;
>  	struct dwapb_gpio *gpio;
> -	struct device_node *np;
>  	int err;
> -	unsigned int offs = 0;
> +	struct device *dev = &pdev->dev;
> +	struct dwapb_platform_data *pdata = dev_get_platdata(dev);
> +
> +	if (!pdata) {

(*)

> +		pdata = dwapb_gpio_get_pdata_of(dev);
> +		if (IS_ERR(pdata))
> +			return PTR_ERR(pdata);

> +		is_of = true;
> +	} else {
> +		is_of = false;

Instead of above three lines, how about
bool is_pdata_alloc = !pdata;

And (*) if (is_pdata_alloc) ...


> +	}
> +	if (!pdata->nports)
> +		return -ENODEV;
>  
>  	gpio = devm_kzalloc(&pdev->dev, sizeof(*gpio), GFP_KERNEL);
>  	if (!gpio)
>  		return -ENOMEM;
>  	gpio->dev = &pdev->dev;
> +	gpio->nr_ports = pdata->nports;
>  
> -	gpio->nr_ports = of_get_child_count(pdev->dev.of_node);
> -	if (!gpio->nr_ports) {
> -		err = -EINVAL;
> -		goto out_err;
> -	}
> -	gpio->ports = devm_kzalloc(&pdev->dev, gpio->nr_ports *
> +	gpio->ports = devm_kcalloc(&pdev->dev, gpio->nr_ports,
>  				   sizeof(*gpio->ports), GFP_KERNEL);
>  	if (!gpio->ports) {
>  		err = -ENOMEM;
> @@ -394,13 +514,18 @@ static int dwapb_gpio_probe(struct platform_device *pdev)
>  		goto out_err;
>  	}
>  
> -	for_each_child_of_node(pdev->dev.of_node, np) {
> -		err = dwapb_gpio_add_port(gpio, np, offs++);
> +	for (i = 0; i < gpio->nr_ports; i++) {
> +		err = dwapb_gpio_add_port(gpio, &pdata->properties[i], i);
>  		if (err)
>  			goto out_unregister;
>  	}
>  	platform_set_drvdata(pdev, gpio);
>  
> +	if (is_of) {
> +		dwapb_free_pdata_of(dev, pdata);
> +		pdata = NULL;

Besides that pdata assignment is probably redundant, you may use plain
kmalloc/kfree and avoid unnecessary devm_* calls.

> +	}
> +
>  	return 0;
>  
>  out_unregister:
> diff --git a/include/linux/platform_data/gpio-dwapb.h b/include/linux/platform_data/gpio-dwapb.h
> new file mode 100644
> index 0000000..28702c8
> --- /dev/null
> +++ b/include/linux/platform_data/gpio-dwapb.h
> @@ -0,0 +1,32 @@
> +/*
> + * Copyright(c) 2014 Intel Corporation.
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope 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.
> + */
> +
> +#ifndef GPIO_DW_APB_H
> +#define GPIO_DW_APB_H
> +
> +struct dwapb_port_property {
> +	struct device_node *node;
> +	const char	*name;
> +	unsigned int	idx;
> +	unsigned int	ngpio;
> +	unsigned int	gpio_base;
> +	unsigned int	irq;
> +	bool		irq_shared;
> +};
> +
> +struct dwapb_platform_data {
> +	struct dwapb_port_property *properties;
> +	unsigned int nports;
> +};
> +
> +#endif


-- 
Andy Shevchenko <andriy.shevchenko@xxxxxxxxx>
Intel Finland Oy
---------------------------------------------------------------------
Intel Finland Oy
Registered Address: PL 281, 00181 Helsinki 
Business Identity Code: 0357606 - 4 
Domiciled in Helsinki 

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.
��.n��������+%������w��{.n�����{��
b���ܨ}���Ơz�j:+v�����w����ޙ��&�)ߡ�a����z�ޗ���ݢj��w�f