On Thu, Oct 20, 2011 at 04:18:49PM +0200, Linus Walleij wrote:
On Thu, Oct 20, 2011 at 3:10 PM, Shawn Guo shawn.guo@freescale.com wrote:
without the common definition from PIN_CONFIG_PULL to PIN_CONFIG_USER, many platforms will need to definite them repeatedly. that is what we hate.
I prefer to completely leave the encoding of this 'u32 param' to pinctrl drivers, so that they can map the bit definition of 'param' to the controller register as much as they can. On imx, we have one register accommodating all pin configuration options for one pin, so we can write 'param' directly into register with exactly same bit definition as register.
I understand this argument, because it makes it more convenient to write drivers and you can cut a few corners.
However I would prefer to keep things more abstract, and the reason is that we strive to have drivers reused across SoCs. For example drivers/tty/serial/amba-pl011.c is used on a multitude of SoCs, U300, Nomadik, Ux500, ARM Versatile, ARM Vexpress, LPC32XX etc etc.
What if this common driver suddenly need to bias a pin? Is it:
#include <linus/pinctrl/pinctrl.h>
ret = pin_config(pctldev, pin, PIN_CONFIG_BIAS_HIGH, 0);
Or (OK silly example but you get the point):
#include <linus/pinctrl/pinctrl.h>
if (machine_is_u300()) ret = pin_config(pctldev, pin, PIN_CONFIG_U300_BIAS_VDD, 0); else if (machine_is_nomadik()) ret = pin_config(pctldev, pin, PIN_CONFIG_NOMADIK_BIAS_UP, 0); else if (machine_is_ux500()) ret = pin_config(pctldev, pin, PIN_CONFIG_UX500_PULLHIGH, 0); else if (machine_is_versatile()) ret = pin_config(pctldev, pin, PIN_CONFIG_VERSATILE_POW, 0); else if (machine_is_vexpress()) ret = pin_config(pctldev, pin, PIN_CONFIG_VEXPRESS_XTRA, 0); else if (machine_is_lpc32xx()) ret = pin_config(pctldev, pin, PIN_CONFIG_LPC_SUPPLY, 0); ...
IMO we have enough strange and custom things in the kernel already and we need to make things more abstract, not the least so that people can understand what the code is doing.
To me, pin_config() and its parameters should be invisible to client drivers. For amba-pl011 example, do you think any of those SoCs will need multiple sets of uart pin configurations to switch for different working modes? If that's case, the individual SoC pinctrl driver should be responsible for mapping particular pin configuration to specific pl011 working mode. Otherwise, the amba-pl011 can simply call pinmux_enable(...) to have both mux and cfg set up.