Re: [PATCH v2 7/9] gpio-tz1090: add TZ1090 gpio driver

[Linus Walleij]

On Fri, May 24, 2013 at 6:21 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.

(...)

This is looking much better.

However I have some more improvement comments, due to new
knowledge. I am sorry about the moving target but it's not my fault...

Basically you should adopt the new style of referencing
flags using the preprocessor.

The examples can be found in the linux-next tree.

> +++ b/Documentation/devicetree/bindings/gpio/gpio-tz1090.txt
(...)
> +  - #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]
> +        [gpio flags]>
> +
> +    Values for gpio specifier:
> +    - GPIO number: a value in the range 0 to 29.
> +    - GPIO flags: bit field of flags:
> +        1: active low

Reference flags from
#include <dt-bindings/gpio/gpio.h>

Then you can use the named constants:
GPIO_ACTIVE_HIGH, GPIO_ACTIVE_LOW

(...)
> +  - #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]
> +        [irq flags]>
> +
> +    Values for irq specifier:
> +    - GPIO number: a value in the range 0 to 29
> +    - IRQ flags: value to describe edge and level triggering
> +        1: trigger on rising edge
> +        2: trigger on falling edge
> +        3: trigger on both rising and falling edges
> +        4: trigger when high
> +        8: trigger when low

This goes here as well.
#include <dt-bindings/interrupt-controller/irq.h>

Then you can use:
IRQ_TYPE_NONE. IRQ_TYPE_EDGE_RISING (etc)

> +       gpios: gpio-controller@02005800 {
> +               #address-cells = <1>;
> +               #size-cells = <0>;
> +               compatible = "img,tz1090-gpio";
> +               reg = <0x02005800 0x90>;
> +
> +               /* bank 0 with an interrupt */
> +               gpios0: bank@0 {
> +                       #gpio-cells = <2>;
> +                       #interrupt-cells = <2>;
> +                       reg = <0>;
> +                       interrupts = <13 4 /* level */>;

And it will look like:
interrupts = <13 IRQ_TYPE_LEVEL_HIGH>;

Which is way easier to understand and you no longer
need to insert comments to explain things.

(...)
> +++ b/drivers/gpio/gpio-tz1090.c

(...)
> +/* REG_GPIO_IRQ_PLRT */
> +#define GPIO_POLARITY_LOW      0
> +#define GPIO_POLARITY_HIGH     1
> +
> +/* REG_GPIO_IRQ_TYPE */
> +#define GPIO_LEVEL_TRIGGERED   0
> +#define GPIO_EDGE_TRIGGERED    1

Why does the comment start with REG_* but not the actual
definition?

(...)
> +/* 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)
> +{
> +       u32 value;
> +
> +       value = tz1090_gpio_read(bank, reg_offs);
> +       value &= ~BIT(offset);
> +       value |= !!val << offset;

I can't parse that last line, it is equivalent to writing:

if (val)
    value |= BIT(offset);

Which I think is easier to understand.


> +/* set polarity to trigger on next edge, whether rising or falling */
> +static void tz1090_gpio_irq_next_edge(struct tz1090_gpio_bank *bank,
> +                                     unsigned int offset)
> +{
> +       unsigned int value_p, value_i;
> +       int lstat;
> +
> +       __global_lock2(lstat);
> +       /* irq_polarity[offset] = !input[offset] */

This comments probably need to be a bit more verbose, like explain
to readers what is happening here.

> +       value_i = ~tz1090_gpio_read(bank, REG_GPIO_DIN);
> +       value_p = tz1090_gpio_read(bank, REG_GPIO_IRQ_PLRT);
> +       value_p &= ~BIT(offset);
> +       value_p |= value_i & BIT(offset);
> +       tz1090_gpio_write(bank, REG_GPIO_IRQ_PLRT, value_p);
> +       __global_unlock2(lstat);
> +}

This looks like it'll work though, nice!

(...)
> +static struct of_device_id tz1090_gpio_of_match[] = {
> +       { .compatible = "img,tz1090-gpio" },
> +       { },
> +};

(...)
> +postcore_initcall(tz1090_gpio_init);

Is it really necessary to have this so early?

Apart from these remarks it is looking very good.

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/