linux-kernel - Re: [PATCH 1/3] pinctrl: pinctrl-imx: add imx pinctrl core driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20120418033801.GA18021@shlinux2.ap.freescale.net>
Date:	Wed, 18 Apr 2012 11:38:02 +0800
From:	Dong Aisheng <aisheng.dong@...escale.com>
To:	Sascha Hauer <s.hauer@...gutronix.de>
CC:	Dong Aisheng-B29396 <B29396@...escale.com>,
	Shawn Guo <shawn.guo@...aro.org>,
	Dong Aisheng <dongas86@...il.com>,
	"linus.walleij@...ricsson.com" <linus.walleij@...ricsson.com>,
	"devicetree-discuss@...ts.ozlabs.org" 
	<devicetree-discuss@...ts.ozlabs.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"rob.herring@...xeda.com" <rob.herring@...xeda.com>,
	"kernel@...gutronix.de" <kernel@...gutronix.de>,
	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH 1/3] pinctrl: pinctrl-imx: add imx pinctrl core driver

On Tue, Apr 17, 2012 at 08:30:06PM +0800, Sascha Hauer wrote:
> On Tue, Apr 17, 2012 at 05:57:23PM +0800, Dong Aisheng wrote:
> > Sascha & Shawn,
> > 
> > Just let you know:
> > After changes the pinctrl table may look like:
> > (i also removed fsl,* config properties since it was introduced for better
> > readability before, but after dtc macro support is available, it could also
> > be removed. So i removed now. Then the using is much similar as iomux-v3).
> > 
> > iomuxc@...e0000 {
> >         compatible = "fsl,imx6q-iomuxc";
> >         reg = <0x020e0000 0x4000>;
> > 
> >         /* shared pinctrl settings */
> >         uart4 {
> >                 pinctrl_uart4_1: uart4grp-1 {
> >                         fsl,pins = <58 61>;
> >                         fsl,configs = <0x12407>
> 
> Why do we have a hex value for the config now? This should be
> independent on how we describe the mux settings.
> 
Yes, that's independent on mux.
Originally for config, it is like:
fsl,pull = <2>;
fsl,pue = <1>;
fsl,pke = <1>;
fsl,speed = <2>;
fsl,drive-strength = <6>;
fsl,slew-rate = <1>;
fsl,hysteresis = <1>;

But i think, as mux, after dtc supports macro, we do not need them any more.
A macro seems enough to tell people what the config data means.
Like:
/define/ MX6Q_UART_PAD_CTRL 0x12407

Since i have already changed mux to raw data, i also want to change config to
raw data at the same time and this is also easy to use.

> Also I think a single fsl,configs entry for a pin group is not
> enough, what for example with I2C where the data line needs a pullup
> whereas the clock line does not?
> 
For this case, the hex value for fsl,configs has not difference as the original one.

The original purpose is put the same config pins in one group and the config value
functions on all the pins in that group.
But if we have a pin needs different config, we need separate it in another group.
For example,
i2c1 {
	pinctrl_i2c_scl: i2c1clk-1 {
		fsl,pins = <...>;
		fsl,configs = <PULLUP..>;
	};
	pinctrl_i2c_sda: i2c1sda-1 {
		fsl,pins = <...>;
		fsl,configs = <NO_PULLUP...>;
	};
};

I have ever also considered another approach for imx which can avoid this issue
by dropping the fsl,configs property which is like:
i2c1 {
	pinctrl_i2c1: i2c1 {
		fsl,pins = <MX6Q_PAD_EIM_D21__I2C1_SCL PULLUP..
			    MX6Q_PAD_EIM_D28__I2C1_SDA PULLDOWN..>
	};
};
Each pin can have its specific pad config setting.
Without macro support, it is like:
i2c1 {
	pinctrl_i2c1: i2c1 {
		/* the value is write randomly, just for showing example */
		fsl,pins = <1001 0x13502 /* with pull up */
			    1056 0x3502>; /* no pull up */
	};
};
The benefit is that it can get better flexibility and the drawback is that
for pins have no config, we still need write a dummy value
to tell the core this pin have no config.
For iomux v3, it's
#define NO_PAD_CTRL                     (1 << 16)
It works for mx5, but for mx6, it needs to be :
#define NO_PAD_CTRL                     (1 << 17) /* bigger than 16 */
(It seems it's not good enough since in the future it may need change again)

Then dts file will look like:
pinctrl_i2c1: i2c1 {
	fsl,pins = <1001 0x20000	/* no config */
		    1056 0x20000>;	/* no config */
};
pinctrl_xxx: xxx1 {
	fsl,pins = <1001 0x20000	/* no config */
		    1056 0x20000	/* no config */
		    1066 0x20000	/* no config */
		    1076 0x20000	/* no config */
		    1086 0x20000	/* no config */
		    1096 0x20000	/* no config */
		    1006 0x20000	/* no config */
		    1016 0x20000	/* no config */
		    1026 0x20000	/* no config */
};
First the 0x20000 cause confusing, second the 0x20000 data is a little redundancy
and many pins may have the same config.

One way may add a property to tell core whether pins have config, but that seems
does not work since it's possible some pins have config while others not.

This is unlike the current way, we can do not define fsl,configs property to tell
driver no config need for these pins.
And based on the assumption that most pins have the same pin config setting.
I chosed the original approach.
The is also the same way as Tegra does.

What do you think of those two approaches? What's your suggestion?

Regards
Dong Aisheng

--
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/