Re: [PATCH V6 1/1] drivers/gpio: Altera soft IP GPIO driver

[Gerhard Sittig <gsi@denx.de>]

[ this reply is related to the Linux driver source code,
  I kept device tree and doc concerns in the other reply ]

you introduce a Linux driver, yet I don't see any Linux
development ML in the Cc: list (only the GPIO maintainer and
doc/DT related lists) -- am I missing something?

On Wed, Jan 22, 2014 at 10:54 +0800, thloh@altera.com wrote:
> 
> --- /dev/null
> +++ b/drivers/gpio/gpio-altera.c
> [ ... ]
> +
> +static int altera_gpio_irq_set_type(struct irq_data *d,
> +				unsigned int type)
> +{
> +	struct altera_gpio_chip *altera_gc = irq_data_get_irq_chip_data(d);
> +
> +	if (type == IRQ_TYPE_NONE)
> +		return 0;
> +
> +	if (type == IRQ_TYPE_LEVEL_HIGH &&
> +		altera_gc->interrupt_trigger == IRQ_TYPE_LEVEL_HIGH) {
> +		return 0;
> +	} else {
> +		if (type == IRQ_TYPE_EDGE_RISING &&
> +			altera_gc->interrupt_trigger == IRQ_TYPE_EDGE_RISING)
> +			return 0;
> +		else if (type == IRQ_TYPE_EDGE_FALLING &&
> +			altera_gc->interrupt_trigger == IRQ_TYPE_EDGE_FALLING)
> +			return 0;
> +		else if (type == IRQ_TYPE_EDGE_BOTH &&
> +			altera_gc->interrupt_trigger == IRQ_TYPE_EDGE_BOTH)
> +			return 0;
> +	}
> +
> +	return -EINVAL;
> +}

This style of indentation makes readers miss the fact that the
condition is multi-line and the body is just a single line.  From
a quick look one might assume broken code due to missing braces.
Please adjust the alignment.

> +static void altera_gpio_irq_handler(unsigned int irq, struct irq_desc *desc)
> +{
> +	struct altera_gpio_chip *altera_gc = irq_desc_get_handler_data(desc);
> +	struct irq_chip *chip = irq_desc_get_chip(desc);
> +	struct of_mm_gpio_chip *mm_gc = &altera_gc->mmchip;
> +	unsigned long status;
> +
> +	int i;
> +
> +	chained_irq_enter(chip, desc);
> +	/* Handling for level trigger and edge trigger is different */
> +	if (altera_gc->interrupt_trigger == IRQ_TYPE_LEVEL_HIGH) {
> +		status = readl_relaxed(mm_gc->regs + ALTERA_GPIO_DATA);
> +		status &= readl_relaxed(mm_gc->regs + ALTERA_GPIO_IRQ_MASK);
> +
> +		for (i = 0; i < mm_gc->gc.ngpio; i++) {
> +			if (BIT(i) & status) {

this looks like "Yoda programming" :)  checking the constant
value against a variable, making readers stop and wonder

> +				generic_handle_irq(irq_find_mapping(
> +					altera_gc->domain, i));
> +			}
> +		}
> +	} else {
> +		while ((status =
> +			(readl_relaxed(mm_gc->regs + ALTERA_GPIO_EDGE_CAP) &
> +			readl_relaxed(mm_gc->regs + ALTERA_GPIO_IRQ_MASK)))) {
> +			writel_relaxed(status,
> +				mm_gc->regs + ALTERA_GPIO_EDGE_CAP);
> +			for (i = 0; i < mm_gc->gc.ngpio; i++) {
> +				if (BIT(i) & status) {
> +					generic_handle_irq(irq_find_mapping(
> +						altera_gc->domain, i));
> +				}
> +			}
> +		}
> +	}
> +
> +	chained_irq_exit(chip, desc);
> +}

> +int altera_gpio_probe(struct platform_device *pdev)
> +{
> +	struct device_node *node = pdev->dev.of_node;
> +	int i, id, reg, ret;
> +	struct altera_gpio_chip *altera_gc = devm_kzalloc(&pdev->dev,
> +				sizeof(*altera_gc), GFP_KERNEL);
> +	if (altera_gc == NULL) {
> +		pr_err("%s: out of memory\n", node->full_name);
> +		return -ENOMEM;
> +	}
> +	altera_gc->domain = 0;
> +
> +	spin_lock_init(&altera_gc->gpio_lock);
> +
> +	id = pdev->id;
> +
> +	if (of_property_read_u32(node, "altr,gpio-bank-width", &reg))
> +		/*By default assume full GPIO controller*/
> +		altera_gc->mmchip.gc.ngpio = 32;
> +	else
> +		altera_gc->mmchip.gc.ngpio = reg;
> +
> +	if (altera_gc->mmchip.gc.ngpio > 32) {
> +		pr_warn("%s: ngpio is greater than 32, defaulting to 32\n",
> +			node->full_name);

use dev_warn() so users can better identify the instance?
hand-rolling this with pr_warn() and node->full_name is
unexpected

> +		altera_gc->mmchip.gc.ngpio = 32;
> +	}
> +
> +	altera_gc->mmchip.gc.direction_input	= altera_gpio_direction_input;
> +	altera_gc->mmchip.gc.direction_output	= altera_gpio_direction_output;
> +	altera_gc->mmchip.gc.get		= altera_gpio_get;
> +	altera_gc->mmchip.gc.set		= altera_gpio_set;
> +	altera_gc->mmchip.gc.to_irq		= altera_gpio_to_irq;
> +	altera_gc->mmchip.gc.owner		= THIS_MODULE;
> +
> +	ret = of_mm_gpiochip_add(node, &altera_gc->mmchip);
> +	if (ret) {
> +		pr_err("%s: Failed adding memory mapped gpiochip\n",
> +			node->full_name);
> +		return ret;
> +	}
> +
> +	platform_set_drvdata(pdev, altera_gc);
> +
> +	altera_gc->mapped_irq = irq_of_parse_and_map(node, 0);
> +
> +	if (!altera_gc->mapped_irq)
> +		goto skip_irq;

interrupt controller support is optional in the code, too -- good

> +
> +	altera_gc->domain = irq_domain_add_linear(node,
> +		altera_gc->mmchip.gc.ngpio, &altera_gpio_irq_ops, altera_gc);
> +
> +	for (i = 0; i < altera_gc->mmchip.gc.ngpio; i++)
> +		irq_create_mapping(altera_gc->domain, i);
> +
> +	if (!altera_gc->domain) {
> +		ret = -ENODEV;
> +		goto dispose_irq;
> +	}

do you reference the domain already between allocation and the
check for successful allocation?

> +
> +	if (of_property_read_u32(node, "altr,interrupt_trigger", &reg)) {
> +		ret = -EINVAL;
> +		pr_err("%s: interrupt_trigger value not set in device tree\n",
> +			node->full_name);
> +		goto teardown;
> +	}
> +	altera_gc->interrupt_trigger = reg;
> +
> +	irq_set_handler_data(altera_gc->mapped_irq, altera_gc);
> +	irq_set_chained_handler(altera_gc->mapped_irq, altera_gpio_irq_handler);
> +
> +	return 0;
> +
> +teardown:
> +	irq_domain_remove(altera_gc->domain);
> +dispose_irq:
> +	irq_dispose_mapping(altera_gc->mapped_irq);
> +	WARN_ON(gpiochip_remove(&altera_gc->mmchip.gc) < 0);
> +
> +	pr_err("%s: registration failed with status %d\n",
> +		node->full_name, ret);
> +
> +	return ret;
> +skip_irq:
> +	return 0;
> +}


virtually yours
Gerhard Sittig
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr. 5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office@denx.de