Re: [PATCH 2/2] pinctrl: add a generic control interface

[Shawn Guo <shawn.guo@freescale.com>]

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.

-- 
Regards,
Shawn