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

Hi,

I'll just skip over the "right, will fix that" issues and just address the 
unclear ones.

Am Dienstag, 4. Juni 2013, 09:08:09 schrieb Linus Walleij:

On Mon, Jun 3, 2013 at 12:59 AM, Heiko St?bner [off-list ref] wrote:

quoted

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.

quoted

+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...

Citing the original gpio driver:

	/*
	 * Values written to this register independently
	 * control Pullup/Pulldown or not for the
	 * corresponding data bit in GPIO.
	 * 0: pull up/down enable, PAD type will decide
	 * to be up or down, not related with this value
	 * 1: pull up/down disable
	 */

So if it's a pull up or down is decided based on the mux of the pin. Calling 
everything a "pull down" (or up) when it isn't seemed somehow wrong to me.

The rk3188 on the other hand supports both pull up and down separately.

Or should this be selected as PULL_UP | PULL_DOWN in the config?

quoted

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

Using #defines, nice!

everything for bonus-points ;-)

And actually it's definitly easier on me as well, as I don't have to remember 
what each integer value means.

generic pinconf sounds interesting ... will give it a try.

The only problem is the pull stuff mentioned above that is either pull up or 
down without the driver having knowledge about it. And generic_pinconf only 
knows about them separately right now.

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

(...)

quoted

+++ 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....)

when using pinconfig_generic, its dump_pin function should be more talkative 
right?

quoted

+
+               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.

yeah, but sometimes it's also good to try to break them ... your solution is 
much nicer to read (and shorter).

All I could think of right now...

Thanks for the review
Heiko