Re: [PATCH 6/8] gpio-tz1090: add TZ1090 gpio driver

[Linus Walleij]

On Tue, Apr 23, 2013 at 4:33 PM, James Hogan <james.hogan@xxxxxxxxxx> wrote:

> Add a GPIO driver for the main GPIOs found in the TZ1090 (Comet) SoC.
> This doesn't include low-power GPIOs as they're controlled separately
> via the Powerdown Controller (PDC) registers.
>
> The driver is instantiated by device tree and supports interrupts for
> all GPIOs.
>
> Signed-off-by: James Hogan <james.hogan@xxxxxxxxxx>
> Cc: Grant Likely <grant.likely@xxxxxxxxxxxx>
> Cc: Rob Herring <rob.herring@xxxxxxxxxxx>
> Cc: Rob Landley <rob@xxxxxxxxxxx>
> Cc: Linus Walleij <linus.walleij@xxxxxxxxxx>
> Cc: linux-doc@xxxxxxxxxxxxxxx

(...)
> +  - #gpio-cells: Should be 2. The syntax of the gpio specifier used by client
> +    nodes should have the following values.
> +       <[phandle of the gpio controller node]
> +        [gpio number within the gpio bank]
> +        [standard Linux gpio flags]>

So when someone using this device tree for Symbian or Windows
Mobile start to work, what does "standard Linux gpio flags" tell them?

> +    Values for gpio specifier:
> +    - GPIO number: a value in the range 0 to 29.
> +    - GPIO flags: standard Linux GPIO flags as found in of_gpio.h

Dito. Linux-specifics are not generally allowed in device trees,
and if they are anyway used they shall be prefixed with "linux,"

> +  Bank subnode optional properties:
> +  - gpio-ranges: Mapping to pin controller pins

Here you seem to use DT GPIO ranges, yet the pinctrl driver registers
some GPIO range, care to explain how that fits together?

> +  - #interrupt-cells: Should be 2. The syntax of the interrupt specifier used by
> +    client nodes should have the following values.
> +       <[phandle of the interurupt controller]
> +        [gpio number within the gpio bank]
> +        [standard Linux irq flags]>
> +
> +    Values for irq specifier:
> +    - GPIO number: a value in the range 0 to 29
> +    - IRQ flags: standard Linux IRQ flags for edge and level triggering

Same comments.

(...)

+#include <asm/global_lock.h>

What on earth is that. I can only fear it. I don't like the
looks of that thing.

(...)
> +/* Convenience register accessors */
> +static void tz1090_gpio_write(struct tz1090_gpio_bank *bank,
> +                             unsigned int reg_offs, u32 data)
> +{
> +       iowrite32(data, bank->reg + reg_offs);
> +}
> +
> +static u32 tz1090_gpio_read(struct tz1090_gpio_bank *bank,
> +                           unsigned int reg_offs)
> +{
> +       return ioread32(bank->reg + reg_offs);
> +}

The pinctrl driver included the keyword "inline" for these so
this should be consistent and do that too.

(...)
> +static void tz1090_gpio_clear_bit(struct tz1090_gpio_bank *bank,
> +                                 unsigned int reg_offs,
> +                                 unsigned int offset)
> +{
> +       int lstat;
> +
> +       __global_lock2(lstat);
> +       _tz1090_gpio_clear_bit(bank, reg_offs, offset);
> +       __global_unlock2(lstat);
> +}

This global lock scares me.

+static inline void _tz1090_gpio_clear_bit(struct tz1090_gpio_bank *bank,
+                                         unsigned int reg_offs,
+                                         unsigned int offset)
+{
+       u32 value;
+
+       value = tz1090_gpio_read(bank, reg_offs);
+       value &= ~(0x1 << offset);

I usually do this:

#include <linux/bitops.h>

value &= ~BIT(offset);

+       tz1090_gpio_write(bank, reg_offs, value);
+}

> +/* caller must hold LOCK2 */
> +static inline void _tz1090_gpio_set_bit(struct tz1090_gpio_bank *bank,
> +                                       unsigned int reg_offs,
> +                                       unsigned int offset)
> +{
> +       u32 value;
> +
> +       value = tz1090_gpio_read(bank, reg_offs);
> +       value |= 0x1 << offset;

I usually do this:

#include <linux/bitops.h>

value |= BIT(offset);

> +/* caller must hold LOCK2 */
> +static inline void _tz1090_gpio_mod_bit(struct tz1090_gpio_bank *bank,
> +                                       unsigned int reg_offs,
> +                                       unsigned int offset,
> +                                       int val)

If val is used as it is, make it a bool.

> +{
> +       u32 value;
> +
> +       value = tz1090_gpio_read(bank, reg_offs);
> +       value &= ~(0x1 << offset);
> +       value |= !!val << offset;

You're claming val to [0,1] obviously it's a bool.

> +       tz1090_gpio_write(bank, reg_offs, value);
> +}

(...)
> +static int tz1090_gpio_request(struct gpio_chip *chip, unsigned offset)
> +{
> +       struct tz1090_gpio_bank *bank = to_bank(chip);
> +       int ret;
> +
> +       ret = pinctrl_request_gpio(chip->base + offset);
> +       if (ret)
> +               return ret;
> +
> +       tz1090_gpio_set_bit(bank, REG_GPIO_DIR, offset);
> +       tz1090_gpio_set_bit(bank, REG_GPIO_BIT_EN, offset);
> +
> +       return 0;
> +}

This is nice, it just glues smoothly into pinctrl here.

> +static void tz1090_gpio_free(struct gpio_chip *chip, unsigned offset)
> +{
> +       struct tz1090_gpio_bank *bank = to_bank(chip);
> +
> +       pinctrl_free_gpio(chip->base + offset);
> +
> +       tz1090_gpio_clear_bit(bank, REG_GPIO_BIT_EN, offset);
> +}

And here.

(...)
> +static int gpio_set_irq_type(struct irq_data *data, unsigned int flow_type)
> +{
> +       struct tz1090_gpio_bank *bank = irqd_to_gpio_bank(data);
> +       unsigned int type;
> +       unsigned int polarity;
> +
> +       switch (flow_type) {
> +       case IRQ_TYPE_EDGE_BOTH:
> +               type = GPIO_EDGE_TRIGGERED;
> +               polarity = GPIO_POLARITY_LOW;
> +               break;
> +       case IRQ_TYPE_EDGE_RISING:
> +               type = GPIO_EDGE_TRIGGERED;
> +               polarity = GPIO_POLARITY_HIGH;
> +               break;
> +       case IRQ_TYPE_EDGE_FALLING:
> +               type = GPIO_EDGE_TRIGGERED;
> +               polarity = GPIO_POLARITY_LOW;
> +               break;
> +       case IRQ_TYPE_LEVEL_HIGH:
> +               type = GPIO_LEVEL_TRIGGERED;
> +               polarity = GPIO_POLARITY_HIGH;
> +               break;
> +       case IRQ_TYPE_LEVEL_LOW:
> +               type = GPIO_LEVEL_TRIGGERED;
> +               polarity = GPIO_POLARITY_LOW;
> +               break;
> +       default:
> +               return -EINVAL;
> +       }
> +
> +       tz1090_gpio_irq_type(bank, data->hwirq, type);
> +       if (type == GPIO_LEVEL_TRIGGERED)
> +               __irq_set_handler_locked(data->irq, handle_level_irq);
> +       else
> +               __irq_set_handler_locked(data->irq, handle_edge_irq);
> +
> +       if (flow_type == IRQ_TYPE_EDGE_BOTH)
> +               tz1090_gpio_irq_next_edge(bank, data->hwirq);
> +       else
> +               tz1090_gpio_irq_polarity(bank, data->hwirq, polarity);
> +
> +       return 0;
> +}

This is also very nice and handling the toggling edge in
a working way.

Overall looking very nice, just needs som polishing, and I'm way
scared about that global lock.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/