Hi,
On Wed, Nov 27, 2013 at 03:49:52AM +0000, thloh@xxxxxxxxxx wrote:
> From: Tien Hock Loh <thloh@xxxxxxxxxx>
>
> Add driver support for Altera GPIO soft IP, including interrupts and I/O.
> Tested on Altera CV SoC board using dipsw and LED using LED framework.
>
> Signed-off-by: Tien Hock Loh <thloh@xxxxxxxxxx>
> ---
> .../devicetree/bindings/gpio/gpio-altera.txt | 35 ++
> drivers/gpio/Kconfig | 7 +
> drivers/gpio/Makefile | 1 +
> drivers/gpio/gpio-altera.c | 395 +++++++++++++++++++++
> 4 files changed, 438 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/gpio/gpio-altera.txt
> create mode 100644 drivers/gpio/gpio-altera.c
>
> diff --git a/Documentation/devicetree/bindings/gpio/gpio-altera.txt b/Documentation/devicetree/bindings/gpio/gpio-altera.txt
> new file mode 100644
> index 0000000..6c57d84
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/gpio/gpio-altera.txt
> @@ -0,0 +1,35 @@
> +Altera GPIO controller bindings
> +
> +Required properties:
> +- compatible:
> + - "altr,pio-1.0"
> +- reg: Physical base address and length of the controller's registers.
> +- #gpio-cells : Should be 1
> + - first cell is the gpio offset number
> +- gpio-controller : Marks the device node as a GPIO controller.
> +- #interrupt-cells : Should be 1.
> +- interrupts: Specify the interrupt.
> +- interrupt-controller: Mark the device node as an interrupt controller
> + The first cell is the local interrupt offset number to the GPIO controller.
Huh? This line doesn't make much sense immediately after the description
of interrupt-controller. Presumably this is associated iwth
#interrupt-cells?
> +
> +Altera GPIO specific properties:
> +- altr,gpio-bank-width: Width of the GPIO bank. This defines how many pins the
> + GPIO device has. Ranges between 1-32.
The code appears to handle this as an optional property, such that this
proeprty is only required if the GPIO controller does not have 32 GPIOs.
It would be worth notign that this is optional, and describing when this
is expected in the binding.
> +- altr,interrupt_trigger: Specifies the interrupt trigger type the GPIO
> + hardware is synthesized. This field is required if the Altera GPIO controller
> + used has IRQ enabled as the interrupt type is not software controlled,
> + but hardware synthesized.
s/_/-/
What type is this, and what values are expected?
The description implies that this is an optional property, but the code
seems to require it.
> +
> +Example:
> +
> +gpio_altr: gpio_altr {
> + compatible = "altr,pio-1.0";
> + reg = <0xff200000 0x10>;
> + interrupts = <0 45 4>;
> + altr,gpio-bank-width = <32>;
> + altr,interrupt_trigger = <0>;
The description doesn't tell me what this porperty in the example
means...
> + #gpio-cells = <1>;
> + gpio-controller;
> + #interrupt-cells = <1>;
> + interrupt-controller;
> +};
[...]
> +int altera_gpio_probe(struct platform_device *pdev)
> +{
> + struct device_node *node = pdev->dev.of_node;
> + int 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->irq = 0;
> +
> + spin_lock_init(&altera_gc->gpio_lock);
> +
> + id = pdev->id;
> +
> + if (of_property_read_u32(node, "altr,gpio-bank-width", ®))
> + /*By default assume full GPIO controller*/
> + altera_gc->mmchip.gc.ngpio = 32;
> + else
> + altera_gc->mmchip.gc.ngpio = reg;
As mentioned above, the binding doesn't describe this as optional, yet
it's treated as such.
> +
> + if (altera_gc->mmchip.gc.ngpio > 32) {
> + pr_warn("%s: ngpio is greater than 32, defaulting to 32\n",
> + node->full_name);
> + 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;
It seems odd to jump directly to a return 0. Why not just return 0 here
(or initialise ret to 0 and combine the two return cases)?
Thanks,
Mark.
--
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
