[PATCH v5 2/8] pinctrl: imx27: imx27 pincontrol driver

[Matt Sealey]

On Thu, Oct 24, 2013 at 10:38 AM, Markus Pargmann <mpa at pengutronix.de> wrote:
> imx27 pincontrol driver using the imx1 core driver. The DT bindings are
> similar to other imx pincontrol drivers.
>
> Signed-off-by: Markus Pargmann <mpa at pengutronix.de>
> Acked-by: Sascha Hauer <s.hauer at pengutronix.de>
> Acked-by: Shawn Guo <shawn.guo at linaro.org>
> ---
>  .../bindings/pinctrl/fsl,imx27-pinctrl.txt         |  50 +++
>  drivers/pinctrl/Kconfig                            |   8 +
>  drivers/pinctrl/Makefile                           |   1 +
>  drivers/pinctrl/pinctrl-imx27.c                    | 477 +++++++++++++++++++++
>  4 files changed, 536 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pinctrl/fsl,imx27-pinctrl.txt
>  create mode 100644 drivers/pinctrl/pinctrl-imx27.c
>
> diff --git a/Documentation/devicetree/bindings/pinctrl/fsl,imx27-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/fsl,imx27-pinctrl.txt
> new file mode 100644
> index 0000000..6619968
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pinctrl/fsl,imx27-pinctrl.txt
> @@ -0,0 +1,50 @@
> +* Freescale IMX27 IOMUX Controller
> +
> +Required properties:
> +- compatible: "fsl,imx27-iomuxc"
> +
> +The iomuxc driver node should define subnodes containing of pinctrl configuration subnodes.
> +
> +Required properties for pin configuration node:
> +- fsl,pins: three integers array, represents a group of pins mux and config
> +  setting. The format is fsl,pins = <PIN MUX_ID CONFIG>. PIN and MUX_ID are
> +  defined as macros in arch/arm/boot/dts/imx27-pinfunc.h. CONFIG can be 0 or

Can we get away from this concept that a binding is predicated on the
fact that they're preprocessed? You're getting it half right.. as
below.

Binding descriptions must NOT rely on preprocessing helpers, for the
following reasons

* Device trees could be generated by methods other than preprocessed source
* Device trees could be generated by Linux builds and passed to other
operating systems (therefore they need to know the definition in
reality).

Please provide and describe the RAW data format of the property and
then somewhere afterwards mention that as a convenience there are
preprocessor macros which make life easier. You can also mention the
include name, but the path (arch/arm/boot/dts) may not be fixed. This
may not even be Linux. They may be split elsewhere in a separate tree
that has no OS and therefore a different layout. Bindings should be OS
agnostic - certainly source code location agnostic - where possible.

> +  PIN is a integer between 0 and 0xbf

Correct.

>                                                                imx27 has 6 ports with 32 configurable
> +  configurable pins each. PIN is PORT * 32 + PORT_PIN, PORT_PIN is the pin
> +  number on the specific port (between 0 and 31).

Correct.

> +  MUX_ID is
> +  function + (direction << 2) + (gpio_oconf << 4) + (gpio_iconfa << 8) + (gpio_iconfb << 10)
> +  function:      0 - Primary function
> +                 1 - Alternate function
> +                 2 - GPIO
> +  direction:     0 - Input
> +                 1 - Output
> +  gpio_oconf:    0 - A_IN
> +                 1 - B_IN
> +                 2 - C_IN
> +                 3 - Data Register
> +  gpio_iconfa/b: 0 - GPIO_IN
> +                 1 - Interrupt Status Register
> +                 2 - 0
> +                 3 - 1

Correct, but I don't like mixing register values for multiple
registers in a single field. I guess you'd waste a ton of space any
other way, so acceptable.

If they're the bits for a single register (could be, but I don't have
access to an i.MX27 manual right now) then just describe it as the
content of the register as defined in a location in the manual, you
won't need to re-describe what is already in the manual.

> +Example:
> +
> +iomuxc: iomuxc at 10015000 {
> +       compatible = "fsl,imx27-iomuxc";
> +       reg = <0x10015000 0x600>;
> +
> +       uart {
> +               pinctrl_uart1: uart-1 {
> +                       fsl,pins = <
> +                               MX27_PAD_UART1_TXD__UART1_TXD 0x0

Incorrect.

Your example should show the raw output. Try compiling this and then
performing the following:

scripts/dtc/dtc -I dtb -O dts imx27-my-platform.dtb

The place where pinctrl_uart1 is in that output is your example. Lots
of awful, unreadable numbers.

However you may provide a second example explaining the convenience of
using the preprocessor macros exactly as above, perhaps with the
location of the preprocessor macros include.

> +                               MX27_PAD_UART1_RXD__UART1_RXD 0x0

One question; are these definitions the one in the manual (for the
names etc, in the huge enums later, especially?) or stripped from
existing source code?

Bindings - and their preprocessor helper macros - should follow the
latest manual if they can. There are many odd differences in i.MX6
pinctrl, but for instance if the manual describes it as SD_DAT3 with
function SD_DATA3 then it should be SD_DAT3__SD_DATA3 rather than
SD_DAT3__SD_DAT3 (even if the SD Association spec says DAT3, since you
are describing an i.MX27 mux, not the SD physical layer :). That way
the preprocessor definitions can be searched for in the manual with
some mangling, and searched for in the headers by pasting out the
function name or the original pin name. If they differ, then someone
has to resolve the differences with their brain which is more work for
the reader.

Thanks,
Matt Sealey <neko at bakuhatsu.net>