On Fri, Nov 29, 2013 at 4:24 AM, Gerhard Sittig <gsi@xxxxxxx> wrote:
> On Wed, Nov 27, 2013 at 11:49 +0800, thloh@xxxxxxxxxx wrote:
>>
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/gpio/gpio-altera.txt
>> @@ -0,0 +1,35 @@
>> +[ ... ]
>> +
>> +Example:
>> +
>> +gpio_altr: gpio_altr {
>> + compatible = "altr,pio-1.0";
>> + reg = <0xff200000 0x10>;
>
> This length appears to be less than what the code defines (the
> latter has offsets beyond 0x10).
The higher registers (0x10 and 0x14) are only available if the
controller is configured as GPIO output. Thus, there are two
configuration - one that has the length 0x10 (GPIO without output
ports), and one with the length 0x20 (GPIO with output ports). Is
there anything I need to handle for cases like this?
>
>> + interrupts = <0 45 4>;
>> + altr,gpio-bank-width = <32>;
>> + altr,interrupt_trigger = <0>;
>
> The numerical value of zero is not one of the valid options.
> Strictly speaking, "none" is zero -- but if the GPIO chip is an
> IRQ controller, the value should be _some_ trigger type, I guess.
>
Yes, you're right, I'll get it fixed.
> Is it useful to drop numerical specs and use symbolic names, when
> you already refer to dt-bindings header files in your reply?
> And/or reference common GPIO and IRQ related documentation.
>
Noted. I'll update the documentation on using dt-binding macros
instead of values.
>> + #gpio-cells = <1>;
>> + gpio-controller;
>> + #interrupt-cells = <1>;
>> + interrupt-controller;
>> +};
>
>
>> --- /dev/null
>> +++ b/drivers/gpio/gpio-altera.c
>> @@ -0,0 +1,395 @@
>> +[ ... ]
>> +
>> +#define ALTERA_GPIO_DATA 0x0
>> +#define ALTERA_GPIO_DIR 0x4
>> +#define ALTERA_GPIO_IRQ_MASK 0x8
>> +#define ALTERA_GPIO_EDGE_CAP 0xc
>> +#define ALTERA_GPIO_OUTSET 0x10
>> +#define ALTERA_GPIO_OUTCLEAR 0x14
>
> See above. These aren't used in the code, but they suggest that
> the register window is larger than 0x10.
See my explanation above.
>
>> +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;
>> + chip->irq_mask(&desc->irq_data);
>> +
>> + /* 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) {
>> + generic_handle_irq(irq_linear_revmap(
>> + altera_gc->irq, i));
>> + }
>> + }
>
> for_each_set_bit()
>
>> + } 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_linear_revmap(
>> + altera_gc->irq, i));
>> + }
>> + }
>> + }
>> + }
>
> Is the "level low" trigger missing? Or is it not supported (in
> the hardware) and is this worth documenting? OTOH later versions
> of the IP block may support all types of triggers, so the main
> concern is that the driver's setup and handling are consistent,
> which they are from a quick glance.
Yes, the Altera GPIO IP doesn't support level low trigger. Patch V5
has the supported interrupt triggers in the documentation.
>
>> +
>> + chip->irq_eoi(irq_desc_get_irq_data(desc));
>> + chip->irq_unmask(&desc->irq_data);
>> +}
>
>
>> +int altera_gpio_probe(struct platform_device *pdev)
>> +{
>> +[ ... ]
>> +
>> + altera_gc->mapped_irq = irq_of_parse_and_map(node, 0);
>> +
>> + if (!altera_gc->mapped_irq)
>> + goto skip_irq;
>
> Personally I would not eliminate this goto instruction, but would
> put the skip_irq label into the regular probe() path. After all
> it's a shortcut when an option does not apply, it's not an error
> path.
Right. I'll get this fixed.
>
>> +
>> + altera_gc->irq = irq_domain_add_linear(node, altera_gc->mmchip.gc.ngpio,
>> + &altera_gpio_irq_ops, altera_gc);
>> +
>> + if (!altera_gc->irq) {
>> + ret = -ENODEV;
>> + goto dispose_irq;
>> + }
>> +
>> + if (of_property_read_u32(node, "altr,interrupt_trigger", ®)) {
>> + 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;
>
> I'd put skip_irq here, right before the return (as other GPIO
> drivers do). The remaining lines of this routine all can be
> considered "exceptions" and "bail out" paths.
Noted. Thanks.
>
>> +
>> +teardown:
>> + irq_domain_remove(altera_gc->irq);
>> +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@xxxxxxx
Thanks for the comments. I'll get these fixed.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
