[PATCH 1/2] gpio: Add CNS3XXX mmio gpio support

Russell King - ARM Linux linux at arm.linux.org.uk
Fri Aug 12 05:19:08 EDT 2011


On Thu, Aug 11, 2011 at 03:48:51AM +0800, Tommy Lin wrote:
> diff --git a/arch/arm/mach-cns3xxx/include/mach/gpio.h b/arch/arm/mach-cns3xxx/include/mach/gpio.h
> new file mode 100644
> index 0000000..7b68acf
> --- /dev/null
> +++ b/arch/arm/mach-cns3xxx/include/mach/gpio.h
> @@ -0,0 +1,64 @@
> +#ifndef	_CNS3XXX_GPIO_H_
> +#define	_CNS3XXX_GPIO_H_
> +
> +#include <mach/cns3xxx.h>
> +
> +#define ARCH_NR_GPIOS				MAX_GPIO_NO

Ok.

> +
> +#include <asm-generic/gpio.h>

Please see linux-next's arch/arm/include/asm/gpio.h

> +
> +#define GPIO_OUTPUT_OFFSET			0x00
> +#define GPIO_INPUT_OFFSET			0x04
> +#define GPIO_DIR_OFFSET				0x08
> +#define GPIO_BIT_SET_OFFSET			0x10
> +#define GPIO_BIT_CLEAR_OFFSET			0x14
> +#define GPIO_INTR_ENABLE_OFFSET			0x20
> +#define GPIO_INTR_RAW_STATUS_OFFSET		0x24
> +#define GPIO_INTR_MASKED_STATUS_OFFSET		0x28
> +#define GPIO_INTR_MASK_OFFSET			0x2C
> +#define GPIO_INTR_CLEAR_OFFSET			0x30
> +#define GPIO_INTR_TRIGGER_METHOD_OFFSET		0x34
> +#define GPIO_INTR_TRIGGER_BOTH_EDGES_OFFSET	0x38
> +#define GPIO_INTR_TRIGGER_POL_OFFSET		0x3C
> +#define GPIO_BOUNCE_ENABLE_OFFSET		0x40
> +#define GPIO_BOUNCE_PRESCALE_OFFSET		0x44

Do you really need to put these here?  Can they go in a private header file
maybe in arch/arm/mach-xxx/ ?

> +
> +
> +#define gpio_get_value			__gpio_get_value
> +#define gpio_set_value			__gpio_set_value
> +#define gpio_cansleep			__gpio_cansleep
> +#define gpio_to_irq			cns3xxx_gpio_to_irq

Again, please see linux-next.

> +struct chog_gpio_chip {
> +	char				*name;

const?

> +	int				base;
> +	u16				ngpio;
> +	int				irq;
> +	struct irq_chip_generic		*gc;
> +	void __iomem			*reg_base;
> +	void __iomem			*reg_gpio_dis;
> +};
...
> +struct cns3xxx_regs {
> +	char		*name;

const?

> +	void __iomem	*addr;
> +	u32		offset;
> +};
...
> +inline int cns3xxx_gpio_to_irq(unsigned gpio)
> +{
> +	return NR_IRQS_CNS3XXX + gpio;
> +}
> +EXPORT_SYMBOL(gpio_to_irq);

No.  Three issues:

1. You are exporting a function you're not defining here.
2. The function is marked inline yet nothing in this file uses it, so
   the inline provides no useful purpose.
3. Please use the standard gpiolib gpio_to_irq() and hook the translation
   into the gpiolib .to_irq method.

...
> +static void __iomem *gpio_map(struct platform_device *pdev,
> +		const char *name, int *err)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct resource *r;
> +	resource_size_t start;
> +	resource_size_t sz;
> +	void __iomem *ret;
> +
> +	*err = 0;
> +
> +	r = platform_get_resource_byname(pdev, IORESOURCE_MEM, name);
> +	if (!r)
> +		return NULL;
> +
> +	sz = resource_size(r);
> +	start = r->start;
> +
> +	if (!devm_request_mem_region(dev, start, sz, r->name)) {
> +		*err = -EBUSY;
> +		return NULL;
> +	}
> +
> +	ret = devm_ioremap(dev, start, sz);
> +	if (!ret) {
> +		*err = -ENOMEM;
> +		return NULL;
> +	}
> +
> +	return ret;
> +}
> +
> +static int __devinit gpio_probe(struct platform_device *pdev)
> +{
> +	int i, nr_gpios = 0, err = 0;
> +	struct device *dev = &pdev->dev;
> +	struct resource *res;
> +	void __iomem *misc_reg;
> +	struct irq_chip_type *ct;
> +
> +	/* TODO Once the clk framework (clk_get() + clk_enable()) is
> +	 * implemented. Use clok framework APIs instead of these APIs.
> +	 */
> +	cns3xxx_pwr_clk_en(0x1 << PM_CLK_GATE_REG_OFFSET_GPIO);
> +	cns3xxx_pwr_soft_rst(0x1 << PM_CLK_GATE_REG_OFFSET_GPIO);
> +
> +
> +	misc_reg = gpio_map(pdev, "MISC", &err);
> +	if (!misc_reg) {
> +		dev_dbg(dev, "%s gpio_map \"MISC\" failure! err=%d\n",
> +				__func__, err);
> +		return err;
> +	}
> +
> +	/* Scan and match GPIO resources */
> +	for (i = 0; i < nr_banks; i++) {
> +		/* Fetech GPIO interrupt control register base address */
> +		gc_info[i].reg_base = gpio_map(pdev, gc_info[i].name, &err);
> +		if (!gc_info[i].reg_base) {
> +			dev_dbg(dev, "%s gpio_map %s failure! err=%d\n",
> +					__func__, gc_info[i].name, err);
> +			goto err1;
> +		}
> +
> +		/* Fetech GPIO interrupt ID number */
> +		res = platform_get_resource_byname(pdev, IORESOURCE_IRQ,
> +				gc_info[i].name);
> +		if (!res) {
> +			dev_dbg(dev, "%s platform_get_resource_byname (%s)"
> +					"failed!\n", gc_info[i].name, __func__);
> +			err = -ENODEV;
> +			goto err2;
> +		}
> +		gc_info[i].irq = res->start;
> +		gc_info[i].reg_gpio_dis	= misc_reg + 0x14 + i * 4;
> +
> +		gc_info[i].gc = irq_alloc_generic_chip("GPIO", 1,
> +				NR_IRQS_CNS3XXX + gc_info[i].base,
> +				gc_info[i].reg_base, handle_level_irq);
> +		if (!gc_info[i].gc) {
> +			dev_dbg(dev, "%s irq_alloc_generic_chip failed!\n",
> +					__func__);
> +			err = -ENOMEM;
> +			goto err2;
> +		}
> +		gc_info[i].gc->private = &gc_info[i];
> +
> +		ct = gc_info[i].gc->chip_types;
> +		ct->regs.mask = get_offset(GPIO_INTR_ENABLE_OFFSET);
> +		ct->regs.ack = get_offset(GPIO_INTR_CLEAR_OFFSET);
> +		ct->regs.type = get_offset(GPIO_INTR_TRIGGER_METHOD_OFFSET);
> +		ct->regs.polarity = get_offset(GPIO_INTR_TRIGGER_POL_OFFSET);
> +		ct->type = IRQ_TYPE_LEVEL_HIGH | IRQ_TYPE_LEVEL_LOW;
> +		ct->chip.irq_ack = irq_gc_ack_set_bit;
> +		ct->chip.irq_mask = irq_gc_mask_clr_bit;
> +		ct->chip.irq_unmask = irq_gc_mask_set_bit;
> +		ct->chip.irq_set_type = cns3xxx_gpio_set_irq_type;
> +
> +		irq_setup_generic_chip(gc_info[i].gc, IRQ_MSK(gc_info[i].ngpio),
> +			IRQ_GC_INIT_MASK_CACHE | IRQ_GC_INIT_NESTED_LOCK,
> +			IRQ_NOREQUEST, IRQ_LEVEL | IRQ_NOPROBE);
> +
> +		irq_set_chained_handler(gc_info[i].irq, gpio_irq_handler);
> +		err = irq_set_handler_data(gc_info[i].irq, &gc_info[i]);
> +		if (err) {
> +			dev_dbg(dev, "%s irq_set_handler_data fail!\n",
> +					__func__);
> +			goto err2;
> +		}
> +
> +		nr_gpios += gc_info[i].ngpio;
> +		if (nr_gpios >= MAX_GPIO_NO)
> +			break;
> +	}
> +
> +	return 0;
> +
> +err2:
> +	if (gc_info[1].reg_base)
> +		devm_iounmap(dev, gc_info[0].reg_base);

This looks buggy.  You're unmapping the wrong reg_base.

> +
> +err1:
> +	if (gc_info[0].reg_base)
> +		devm_iounmap(dev, gc_info[0].reg_base);
> +
> +	devm_iounmap(dev, misc_reg);

This is also rather inconsistent.  While you iounmap, you're not releasing
the region which you claimed in gpio_map().

It's also rather unnecessary.  One of the reasons for devm_* is that
drivers shouldn't need to care about cleaning up the managed resources.
The driver model will take care of that when a probe fails, or upon
remove.  It avoids people making mistakes like the above.

However, I don't see where you're cleaning up the generic irqchip
which was allocated (which is an unmanaged resource).




More information about the linux-arm-kernel mailing list