On Mon, Aug 19, 2013 at 8:59 PM, Markus Mayer <markus.mayer@xxxxxxxxxx> wrote:
> From: Markus Mayer <mmayer@xxxxxxxxxxxx>
>
> This patch adds the GPIO driver for the bcm281xx family of chips.
>
> Signed-off-by: Markus Mayer <markus.mayer@xxxxxxxxxx>
> Reviewed-by: Christian Daudt <csd@xxxxxxxxxxxx>
> Reviewed-by: Tim Kryger <tim.kryger@xxxxxxxxxx>
> Reviewed-by: Matt Porter <matt.porter@xxxxxxxxxx>
This is getting into nice shape quickly, but at this point the driver
will still miss the v3.12 merge window as it seems, so let's
aim for v3.13.
> +/* This function counts IRQ entries in the device tree */
> +static int bcm_kona_count_irq_resources(struct platform_device *pdev)
> +{
> + int i, ret;
> +
> + for (i = 0;; i++) {
> + ret = platform_get_irq(pdev, i);
> + if (ret < 0)
> + break;
> + }
> + return i;
> +}
This looks weird, and it is not operating on a device tree but
on the platform resources created by the device tree core,
so atleast remove that comment. What it does is just count
the number of IRQs for this device, no matter where it came
from, right?
> +static void bcm_kona_gpio_set(struct gpio_chip *chip, unsigned gpio,
> + int value)
> +{
(...)
> + val = readl(reg_base + reg_offset);
> + val |= 1 << bit;
I would really like you to do like this:
#include <linux/bitops.h>
val |= BIT(bit);
(...)
> +static int bcm_kona_gpio_get(struct gpio_chip *chip, unsigned gpio)
> +{
(...)
> + /* return the specified bit status */
> + return (val >> bit) & 1;
Please do like this:
return !!(val & bit);
> +static int bcm_kona_gpio_direction_output(struct gpio_chip *chip,
> + unsigned gpio, int value)
> +{
(...)
> + val = readl(reg_base + reg_offset);
> + val |= 1 << bit;
val |= BIT(bit);
> +static void bcm_kona_gpio_irq_handler(unsigned int irq, struct irq_desc *desc)
> +{
> + void __iomem *reg_base;
> + int bit, bank_id;
> + unsigned long sta;
> + struct bcm_kona_gpio_bank *bank = irq_get_handler_data(irq);
> + struct irq_chip *chip = irq_desc_get_chip(desc);
> +
> + chained_irq_enter(chip, desc);
> +
> + /*
> + * For bank interrupts, we can't use chip_data to store the kona_gpio
> + * pointer, since GIC needs it for its own purposes. Therefore, we get
> + * our pointer from the bank structure.
> + */
> + reg_base = bank->reg_base;
> + bank_id = bank->id;
> + bcm_kona_gpio_unlock_bank(reg_base, bank_id);
> +
> + sta = readl(reg_base + GPIO_INT_STATUS(bank_id)) &
> + (~(readl(reg_base + GPIO_INT_MASK(bank_id))));
> +
> + for_each_set_bit(bit, &sta, 32) {
> + /*
> + * Clear interrupt before handler is called so we don't
> + * miss any interrupt occurred during executing them.
> + */
> + writel(readl(reg_base + GPIO_INT_STATUS(bank_id)) |
> + (1 << bit),
Use BIT(bit)
> + reg_base + GPIO_INT_STATUS(bank_id));
> + /* Invoke interrupt handler */
> + generic_handle_irq(gpio_to_irq(GPIO_PER_BANK * bank_id + bit));
> + }
Usually you may want to re-read thet status for each iteration
of this loop, if IRQs appear as you are processing.
> +static int bcm_kona_gpio_probe(struct platform_device *pdev)
> +{
(...)
> + kona_gpio->irq_base = irq_alloc_descs(-1, 0, chip->ngpio, 0);
> + if (kona_gpio->irq_base < 0) {
> + dev_err(dev, "Couldn't allocate IRQ numbers\n");
> + return -ENXIO;
> + }
As mentioned by Tomasz this looks strange.
Apart from this last thing, the rest is really nitpicking comments
and no blockers.
Yours,
Linus Walleij
--
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
