Date:	Tue, 4 Jun 2013 14:05:12 +0200
From:	Heiko Stübner <heiko@...ech.de>
To:	Linus Walleij <linus.walleij@...aro.org>
Cc:	Stephen Warren <swarren@...dotorg.org>,
	Laurent Pinchart <laurent.pinchart+renesas@...asonboard.com>,
	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	John Stultz <john.stultz@...aro.org>,
	Thomas Gleixner <tglx@...utronix.de>,
	Mike Turquette <mturquette@...aro.org>,
	Seungwon Jeon <tgih.jun@...sung.com>,
	Jaehoon Chung <jh80.chung@...sung.com>,
	Chris Ball <cjb@...top.org>,
	"linux-mmc@...r.kernel.org" <linux-mmc@...r.kernel.org>,
	Grant Likely <grant.likely@...aro.org>,
	Rob Herring <rob.herring@...xeda.com>,
	"devicetree-discuss@...ts.ozlabs.org" 
	<devicetree-discuss@...ts.ozlabs.org>,
	Russell King <linux@....linux.org.uk>,
	Arnd Bergmann <arnd@...db.de>, Olof Johansson <olof@...om.net>
Subject: Re: [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 <heiko@...ech.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@...ech.de>
> 
> Overall this is looking very good, mainly minor comments.

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


> > +uart2: serial@...64000 {
> > +       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.

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

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


> > +
> > +               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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists