Re: [PATCH 07/10] pinctrl: add pinctrl driver for Rockchip SoCs

[Linus Walleij <linus.walleij@linaro.org>]

On Mon, Jun 3, 2013 at 12:59 AM, Heiko Stübner <heiko@sntech.de> wrote:

> This driver adds support the Cortex-A9 based SoCs from Rockchip,
> so at least the RK2928, RK3066 (a and b) and RK3188.
> Earlier Rockchip SoCs seem to use similar mechanics for gpio
> handling so should be supportable with relative small changes.
> Pull handling on the rk3188 is currently a stub, due to it being
> a bit different to the earlier SoCs.
>
> Pinmuxing as well as gpio (and interrupt-) handling tested on
> a rk3066a based machine.
>
> Signed-off-by: Heiko Stuebner <heiko@sntech.de>

Overall this is looking very good, mainly minor comments.

> +++ b/Documentation/devicetree/bindings/pinctrl/rockchip-pinctrl.txt

Please name this beginning with the vendor and similar to
the compatible string: rockchip,pinctrl or something.
(Check the neighbors.)

(...)
> +Required properties for gpio sub nodes:
> +  - compatible: "rockchip,gpio-bank"
> +  - reg: register of the gpio bank (different than the iomux registerset)
> +  - interrupts: base interrupt of the gpio bank in the interrupt controller
> +  - clocks: clock that drives this bank
> +  - gpio-controller: identifies the node as a gpio controller and pin bank.
> +  - #gpio-cells: number of cells in GPIO specifier. Since the generic GPIO
> +    binding is used, the amount of cells must be specified as 2. See generic
> +    GPIO binding documentation for description of particular cells.
> +  - interrupt-controller: identifies the controller node as interrupt-parent.
> +  - #interrupt-cells: the value of this property should be 2.
> +     - First Cell: represents the external gpio interrupt number local to the
> +       external gpio interrupt space of the controller.
> +     - Second Cell: flags to identify the type of the interrupt
> +       - 1 = rising edge triggered
> +       - 2 = falling edge triggered
> +       - 3 = rising and falling edge triggered
> +       - 4 = high level triggered
> +       - 8 = low level triggered

Can't you just reference
Documentation/devicetree/bindings/interrupt-controller/interrupts.txt?

> +Required properties for pin configuration node:
> +  - rockchip,pins: 4 integers array, represents a group of pins mux and config
> +    setting. The format is rockchip,pins = <PIN_BANK PIN_BANK_NUM MUX CONFIG>.
> +    The MUX 0 means gpio and MUX 1 to 3 mean the specific device function
> +
> +Bits used for CONFIG:
> +PULL_AUTO      (1 << 0): indicate this pin needs a pull setting for SoCs
> +                         that determine the pull up or down themselfs

Hm, never saw that before...

(...)
> +       uart2 {
> +               uart2_xfer: uart2-xfer {
> +                       rockchip,pins = <RK_GPIO1 8 1 RK_PINCTRL_PULL_AUTO>,
> +                                       <RK_GPIO1 9 1 RK_PINCTRL_PULL_AUTO>;
> +               };

This looks like you're using #include to define these constants,
probably you should include that in the example or mention it?

> +uart2: serial@20064000 {
> +       compatible = "snps,dw-apb-uart";
> +       reg = <0x20064000 0x400>;
> +       interrupts = <GIC_SPI 36 IRQ_TYPE_LEVEL_HIGH>;

Using #defines, nice!

(...)
+++ b/drivers/pinctrl/Kconfig
@@ -158,6 +158,12 @@ config PINCTRL_DB8540
        bool "DB8540 pin controller driver"
        depends on PINCTRL_NOMADIK && ARCH_U8500

+config PINCTRL_ROCKCHIP
+       bool
+       select PINMUX
+       select PINCONF
+       select GENERIC_IRQ_CHIP

Why is this super-simple pin config thing not using
GENERIC_PINCONF?

I *know* it is simpler to implement your own thing, but think of the
poor maintainers that have to wade through 50 identical implementations.
Do this, it pays off.

BTW: it leads to wanting to use generic pinconf DT bindings as experienced
by Laurent and others. We need to fix that too...

(...)
> +++ b/drivers/pinctrl/pinctrl-rockchip.c
> +static void rockchip_pin_dbg_show(struct pinctrl_dev *pctldev,
> +                                       struct seq_file *s, unsigned offset)
> +{
> +       seq_printf(s, "%s", dev_name(pctldev->dev));
> +}

Nothing else you want to say about the pins here?
(No big deal for sure, but....)

> +static int rockchip_get_pull(struct rockchip_pin_bank *bank, int pin_num)
> +{
> +       struct rockchip_pinctrl *info = bank->drvdata;
> +       void __iomem *reg;
> +       int bit;

Is that really an int? I'd guess more like u8...

> +       u32 data;
> +
> +       /* rk3066b does support any pulls */
> +       if (!info->ctrl->pull_offset)
> +               return 0;
> +
> +       reg = info->reg_base + info->ctrl->pull_offset;
> +
> +       if (info->ctrl->pull_auto) {
> +               reg += bank->bank_num * 8;

I'd define some constant like

#define RK_BANK_STRIDE 8
reg += bank->bank_num * RK_BANK_STRIDE;
(Since 8 bytes of stride is no natural law.)

And then use that here and elsewhere.

> +               reg += (pin_num / 16) * 4;
> +               bit = pin_num % 16;

This is clear however.

> +
> +               data = readl_relaxed(reg);
> +               data >>= bit;
> +               data &= 1;
> +
> +               return !data;


That's a bit hard to read, I'd just:

#include <linux/bitops.h>
return !(readl_relaxed(reg) & BIT(bit));

And skip the "data" variable. The ! operator will
clamp this to a bool (0/1).

But we all have our habits.

> +static int rockchip_set_pull(struct rockchip_pin_bank *bank,
> +                                       int pin_num, int pull)

Similar comments for this function.

> +static int rockchip_pmx_gpio_set_direction(struct pinctrl_dev *pctldev,
> +                                             struct pinctrl_gpio_range *range,
> +                                             unsigned offset, bool input)
(...)
> +       /* set bit to 1 for output, 0 for input */
> +       if (!input)
> +               data |= (1 << pin);
> +       else
> +               data &= ~(1 << pin);

Here again I would use <linux/bitops.h> and:

data |= BIT(pin);

etc, but it's a matter of taste so if you prefer this, do keep it.

(...)
> +static int rockchip_irq_set_type(struct irq_data *d, unsigned int type)
> +{
> +       struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
> +       struct rockchip_pin_bank *bank = gc->private;
> +       u32 bit = (1 << d->hwirq);

Name that something like "mask" to match other naming in the
kernel. "bit" sort of implies a number between 0 and 31 and that
is not the case.

All I could think of right now...

Yours,
Linus Walleij