[RFC PATCH] drivers/pinctrl: Add pinctrl-ast2400

[RFC PATCH] drivers/pinctrl: Add pinctrl-ast2400

[Andrew Jeffery]

Hi all,

This is a patch partially implementing a pinctrl/pinmux driver for the Aspeed
AST2400 ARM SoC, complementing Joel Stanley's recent series[1] adding the core
support for the chip. With additional integration patches there's enough pinmux
to boot the SoC and configure a number of i2c busses, but not much more.

I'm posting it as an RFC for several reasons:

* This is my first attempt at a pinmux driver (and first significant kernel
  patch) and so I've probably made mistakes and overlooked useful interfaces or
  patterns

* The hardware seems funky and makes things tedious. Configuring a
  mux function often requires writing multiple bits to multiple registers,
  sometimes for each pin in a bus

* I've added a fair bit of commentary to the implementation to explain the
  approach. Likely some of it is considered common knowledge and can be
  stripped out, so thoughts there would be appreciated. Maybe it's too verbose
  and is more confusing than helpful. The datasheet isn't publicly available so
  I went with more-is-better.

* It makes heavy use of macros to "simplify" the pin/function declarations.
  Maybe it's too much of a macro hell. Maybe there's an alternative
  representation that's less dependent on interlinked structures. Maybe more
  should be pushed into devicetree? In any case, I've kernel-doc'ed the
  high-level macros to describe their use.

As mentioned the driver not complete and the patch doesn't integrate it in any
way, the implementation only describes a subset of the functions and pins
available and pinconf isn't yet handled. To be useful in the system I'm testing
against (an OpenPOWER Palmetto BMC) we need more pins configured as GPIO, but I
held off on implementing functionality for all required pins in order to first
get some feedback on the approach.

Cheers,

Andrew

[1] http://www.spinics.net/lists/arm-kernel/msg498895.html

Andrew Jeffery (1):
  drivers/pinctrl: Add pinctrl-ast2400

 drivers/pinctrl/pinctrl-ast2400.c | 1488 +++++++++++++++++++++++++++++++++++++
 1 file changed, 1488 insertions(+)
 create mode 100644 drivers/pinctrl/pinctrl-ast2400.c

-- 
2.7.4

[RFC PATCH] drivers/pinctrl: Add pinctrl-ast2400

[Andrew Jeffery]

Add pinctrl/pinmux support for the Aspeed AST2400 SoC. Configuration of
the SoC is somewhat involved: Enabling functions can involve writes of
multiple bits to multiple registers, and signals provided by a pin are
determined on a priority basis. That is, without care, it's possible to
configure a function in the mux and to not gain expected functionality.

Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
---
 drivers/pinctrl/pinctrl-ast2400.c | 1488 +++++++++++++++++++++++++++++++++++++
 1 file changed, 1488 insertions(+)
 create mode 100644 drivers/pinctrl/pinctrl-ast2400.c

diff --git a/drivers/pinctrl/pinctrl-ast2400.c b/drivers/pinctrl/pinctrl-ast2400.c
new file mode 100644
index 000000000000..0f0c19005046
--- /dev/null
+++ b/drivers/pinctrl/pinctrl-ast2400.c
@@ -0,0 +1,1488 @@
+/*
+ * Copyright (C) 2016 IBM
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation version 2.
+ *
+ * This program is distributed "as is" WITHOUT ANY WARRANTY of any
+ * kind, whether express or implied; without even the implied warranty
+ * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+#include <linux/bitops.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/pinctrl/pinctrl.h>
+#include <linux/pinctrl/pinmux.h>
+#include <linux/pinctrl/pinconf.h>
+#include <linux/pinctrl/pinconf-generic.h>
+#include <linux/string.h>
+#include <linux/types.h>
+
+#include "core.h"
+#include "pinctrl-utils.h"
+
+/* Challenges in the AST2400 Multifunction Pin Design
+ * --------------------------------------------------
+ *
+ * * Reasonable number of pins (216)
+ *
+ * * The SoC function enabled on a pin is determined on a priority basis
+ *
+ * * Using the priority system, pins can provide up to 3 different signals
+ *   types
+ *
+ * * The signal active on a pin is often described by compound logical
+ *   expressions involving multiple operators, registers and bits
+ *
+ *   * The high and low priority functions' bit masks are frequently not the
+ *     same
+ *     (i.e. cannot just flip a bit to change from high to low), or even in the
+ *     same register.
+ *
+ *   * Not all functions are disabled by unsetting bits, some require setting
+ *     bits
+ *
+ *   * Not all signals can be deactivated as some expressions depend on values
+ *     in the hardware strapping register, which is treated as read-only.
+ *
+ * SoC Multi-function Pin Expression Examples
+ * ------------------------------------------
+ *
+ * Here are some sample mux configurations from the datasheet to illustrate the
+ * corner cases, roughly in order of least to most corner. In the table, HP and
+ * LP are high- and low- priority respectively.
+ *
+ * D6 is a pin with a single (i.e. aside from GPIO), high priority signal
+ * that participates in one function:
+ *
+ * Ball | Default | HP Signal | HP Expression               | LP Signal | LP Expression | GPIO Name
+ * -----+---------+-----------+-----------------------------+-----------+---------------+----------
+ *  D6    GPIOA0    MAC1LINK    SCU80[0]=1                                                GPIOA0
+ * -----+---------+-----------+-----------------------------+-----------+---------------+----------
+ *
+ * C5 is a multi-signal pin (both high and low priority signals). Here we touch
+ * different registers for the different functions that enable each signal:
+ *
+ * -----+---------+-----------+-----------------------------+-----------+---------------+----------
+ *  C5    GPIOA4    SCL9        SCU90[22]=1                   TIMER5      SCU80[4]=1      GPIOA4
+ * -----+---------+-----------+-----------------------------+-----------+---------------+----------
+ *
+ * E19 is a single-signal pin with two functions that influence the active
+ * signal. In this case both bits have the same meaning - enable a dedicated
+ * LPC reset pin. However it's not always the case that the bits in the
+ * OR-relationship have the same meaning.
+ *
+ * -----+---------+-----------+-----------------------------+-----------+---------------+----------
+ *  E19   GPIOB4    LPCRST#     SCU80[12]=1 | Strap[14]=1                                 GPIOB4
+ * -----+---------+-----------+-----------------------------+-----------+---------------+----------
+ *
+ * For example, pin B19 has a low-priority signal that's enabled by two
+ * distinct SoC functions: A specific SIOPBI bit in SCUA4, and an ACPI bit
+ * in the STRAP register. The ACPI bit configures signals on pins in addition
+ * to B19. Both of the low priority functions as well as the high priority
+ * function must be disabled for GPIOF1 to be used.
+ *
+ * Ball | Default | HP Signal | HP Expression                           | LP Signal | LP Expression                          | GPIO Name
+ * -----+---------+-----------+-----------------------------------------+-----------+----------------------------------------+----------
+ *  B19   GPIOF1    NDCD4       SCU80[25]=1                               SIOPBI#     SCUA4[12]=1 | Strap[19]=0                GPIOF1
+ * -----+---------+-----------+-----------------------------------------+-----------+----------------------------------------+----------
+ *
+ * For pin E18, the SoC ANDs the expected state of three bits to determine the
+ * pin's active signal:
+ *
+ * * SCU3C[3]: Enable external SOC reset function
+ * * SCU80[15]: Enable SPICS1# or EXTRST# function pin
+ * * SCU90[31]: Select SPI interface CS# output
+ *
+ * -----+---------+-----------+-----------------------------------------+-----------+----------------------------------------+----------
+ *  E18   GPIOB7    EXTRST#     SCU3C[3]=1 & SCU80[15]=1 & SCU90[31]=0    SPICS1#     SCU3C[3]=1 & SCU80[15]=1 & SCU90[31]=1   GPIOB7
+ * -----+---------+-----------+-----------------------------------------+-----------+----------------------------------------+----------
+ *
+ * (Bits SCU3C[3] and SCU80[15] appear to only be used in the expressions for
+ * selecting the signals on pin E18)
+ *
+ * Pin T5 is a multi-signal pin with a more complex configuration:
+ *
+ * Ball | Default | HP Signal | HP Expression                | LP Signal | LP Expression | GPIO Name
+ * -----+---------+-----------+------------------------------+-----------+---------------+----------
+ *  T5    GPIOL1    VPIDE       SCU90[5:4]!=0 & SCU84[17]=1    NDCD1       SCU84[17]=1     GPIOL1
+ * -----+---------+-----------+------------------------------+-----------+---------------+----------
+ *
+ * The high priority signal configuration is best thought of in terms of its
+ * exploded form, with reference to the SCU90[5:4] bits:
+ *
+ * * SCU90[5:4]=00: disable
+ * * SCU90[5:4]=01: 18 bits (R6/G6/B6) video mode.
+ * * SCU90[5:4]=10: 24 bits (R8/G8/B8) video mode.
+ * * SCU90[5:4]=11: 30 bits (R10/G10/B10) video mode.
+ *
+ * -----+---------+-----------+------------------------------+-----------+---------------+----------
+ *  T5    GPIOL1    VPIDE      (SCU90[5:4]=1 & SCU84[17]=1)    NDCD1       SCU84[17]=1     GPIOL1
+ *                             | (SCU90[5:4]=2 & SCU84[17]=1)
+ *                             | (SCU90[5:4]=3 & SCU84[17]=1)
+ * -----+---------+-----------+------------------------------+-----------+---------------+----------
+ *
+ * For reference the SCU84[17] bit enables the "UART1 NDCD1 or Video VPIDE
+ * function pin", where the signal itself is determined by whether SCU94[5:4]
+ * is disabled or in one of the 18, 24 or 30bit video modes.
+ *
+ * Other video-input-related pins require an explicit state in SCU90[5:4]:
+ *
+ * -----+---------+-----------+------------------------------+-----------+---------------+----------
+ *  W1    GPIOL6    VPIB0       SCU90[5:4]=3 & SCU84[22]=1     TXD1        SCU84[22]=1     GPIOL6
+ * -----+---------+-----------+------------------------------+-----------+---------------+----------
+ *
+ * The examples of T5 and W1 are particularly fertile, as they also demonstrate
+ * that despite operating as part of the video input bus each signal needs to
+ * be enabled individually via it's own SCU84 (in the cases of T5 and W1)
+ * register bit. This is a little crazy if the bus doesn't have optional
+ * signals, but is used to decent effect with some of the UARTs where not all
+ * signals are required. However, this isn't done consistently - UART1 is
+ * enabled on a per-pin basis, and by contrast, all signals for UART6 are
+ * enabled by a single bit.
+ *
+ * Ultimately the requirement to control pins like T5 drive the design:
+ *
+ * * Pins provide signals according to functions activated in the mux
+ *   configuration
+ *
+ * * Pins provide two to three signals in a priority order
+ *
+ * * For priorities levels defined on a pin, each priority provides one signal
+ *
+ * * Enabling lower priority signals requires higher priority signals be
+ *   disabled
+ *
+ * * A function represents a set of signals; functions are distinct if their
+ *   sets of signals are not equal
+ *
+ * * Signals participate in one or more functions
+ *
+ * * A function is described by an expression of one or more function
+ *   descriptors, which compare bit values in a register
+ *
+ * * A function expression is the smallest set of function descriptors whose
+ *   comparisons must evaluate 'true' for a function to be enabled.
+ *
+ * * A function is active if evaluating every function descriptor in the
+ *   function expression yields a 'true' result
+ *
+ * * A signal at a given priority on a given pin is active if any of the
+ *   functions in which the signal participates are active, and no higher
+ *   priority signal on the pin is active
+ *
+ * * GPIO is configured per-pin
+ *
+ * And so:
+ *
+ * * To disable a signal, any function(s) activating the signal must be
+ *   disabled
+ *
+ * * Each pin must know the expressions of functions in which it participates,
+ *   for the purpose of enabling GPIO. This is done by deactivating all
+ *   functions that activate higher priority signals on the pin (except in the
+ *   case of GPIO[TUV], where GPIO is the high priority function).
+ *
+ * As a concrete example:
+ *
+ * * T5 provides three signals types: VPIDE, NDCD1 and GPIO
+ *
+ * * The VPIDE signal participates in 3 functions: VPI18, VPI24 and VPI30
+ *
+ * * The NDCD1 signal participates in just its own NDCD1 function
+ *
+ * * VPIDE is high priority, NDCD1 is low priority, and GPIOL1 is the least
+ *   prioritised
+ *
+ * * The prerequisit for activating the NDCD1 signal is that the VPI18, VPI24
+ *   and VPI30 functions all be disabled
+ *
+ * * Similarly, all of VPI18, VPI24, VPI30 and NDCD1 functions must be disabled
+ *   to provide GPIOL6
+ *
+ * Considerations
+ * --------------
+ *
+ * * If pinctrl allows us to allocate a pin we can configure a function without
+ *   concern for the function of already allocated pins, if pin groups are
+ *   created with respect to the SoC functions in which they participate (I felt
+ *   this was worth mentioning as it didn't seem obvious from the bit/pin
+ *   relationships; I've done some network analysis on the problem).
+ *
+ *     * Conversely, failing to allocate all pins in a group indicates some
+ *       bits (as well as pins) required for the group's configuration will
+ *       already be in use, likely in a way that's inconsistent with the
+ *       requirements of the failed group.
+ *
+ * * Implement data structures such that we can lean on the compiler to avoid
+ *   or catch errors
+ *
+ *   * Define symbols for structures so they can be referenced by name
+ *
+ *     * Compiler enforces unique symbol names, detecting copy/paste errors
+ *     * Compiler errors on undefined symbol names, detecting typos or missing
+ *       information
+ *
+ *   * Compiler can calculate array sizes for us
+ *
+ * * Implement macros such that we can lean on the CPP to
+ *
+ *   * Avoid duplicate specification of information where possible
+ *   * Reduce line noise of type declaration and assignment
+ */
+
+ /**
+  * A function descriptor, which describes the register, bits and the
+  * enable/disable values that should be compared or written.
+  *
+  * @reg: The register offset from base in bytes
+  * @mask: The mask to apply to the register. The lowest set bit of the mask is
+  *        used to derive the shift value.
+  * @enable: The value that enables the function. Value should be in the LSBs,
+  *          not at the position of the mask.
+  * @disable: The value that disables the function. Value should be in the
+  *           LSBs, not at the position of the mask.
+  */
+struct func_desc {
+	unsigned reg;
+	u32 mask;
+	u32 enable;
+	u32 disable;
+};
+
+/**
+ * Describes a function expression. The expression is evaluated by ANDing the
+ * evaluation of the descriptors.
+ *
+ * @name: The function name
+ * @ndescs: The number of function descriptors in the expression
+ * @descs: Pointer to an array of function descriptors that comprise the
+ *         function expression
+ */
+struct func_expr {
+	const char *name;
+	int ndescs;
+	const struct func_desc *descs;
+};
+
+/**
+ * A struct capturing the function expressions enabling signals at each
+ * priority for a given pin. A high or low priority signal configuration is
+ * evaluated by ORing the evaluation of the function expressions in the
+ * respective priority's list.
+ *
+ * @high: A NULL-terminated list of function expressions that enable the
+ *        high-priority signal
+ * @low: A NULL-terminated list of function expressions that enable the
+ *       low-priority signal
+ * @other: The name of the "other" function, enabled by disabling the high and
+ *         low priority signals
+ */
+struct pin_prio {
+	const struct func_expr **high;
+	const struct func_expr **low;
+	const char *other;
+};
+
+/* Macro hell */
+
+/**
+ * Short-hand macro describing a mux function enabled by the state of one bit.
+ * The disable value is derived.
+ *
+ * @reg: The function's associated register, offset from base
+ * @idx: The function's bit index in the register
+ * @val: The value (0 or 1) that enables the function
+ */
+#define FUNC_DESC_BIT(reg, idx, val) \
+	{ reg, BIT_MASK(idx), val, ((val + 1) & 1) }
+
+/**
+ * A further short-hand macro describing a mux function enabled with a set bit.
+ *
+ * @reg: The function's associated register, offset from base
+ * @idx: The function's bit index in the register
+ */
+#define FUNC_DESC_SET(reg, idx) FUNC_DESC_BIT(reg, idx, 1)
+
+#define FUNC_DESC_LIST_SYM(func) func_descs_ ## func
+#define FUNC_DESC_LIST_DECL(func, ...) \
+	static const struct func_desc FUNC_DESC_LIST_SYM(func)[] = \
+		{ __VA_ARGS__ }
+
+#define FUNC_EXPR_SYM(func) func_expr_ ## func
+#define FUNC_EXPR_DECL_(func) \
+	static const struct func_expr FUNC_EXPR_SYM(func) = \
+	{ \
+		.name = #func, \
+		.ndescs = ARRAY_SIZE(FUNC_DESC_LIST_SYM(func)), \
+		.descs = &(FUNC_DESC_LIST_SYM(func))[0], \
+	}
+
+/**
+ * Declare a function expression.
+ *
+ * @func: A macro symbol name for the function (is subjected to stringification
+ *        and token pasting)
+ * @...: Function descriptors that define the function expression
+ *
+ * For example, the following declares the ACPI function:
+ *
+ *     FUNC_EXPR_DECL(ACPI, FUNC_DESC_BIT(STRAP, 19, 0));
+ *
+ * And (with a few defines left out for brevity) the following declares the
+ * 8-signal ROM function:
+ *
+ *     FUNC_EXPR_DECL(ROM8,
+ *         VPOOFF0_DESC,
+ *         ROMEN_0_DESC,
+ *         ROMEN_1_DESC,
+ *         ROMEN_2_DESC);
+ */
+#define FUNC_EXPR_DECL(func, ...) \
+	FUNC_DESC_LIST_DECL(func, __VA_ARGS__); \
+	FUNC_EXPR_DECL_(func)
+
+/**
+ * Declare a pointer to a function expression
+ *
+ * @func: The macro symbol name for the function (subjected to token pasting)
+ */
+#define FUNC_EXPR_PTR(func) (&FUNC_EXPR_SYM(func))
+
+#define FUNC_EXPR_LIST_SYM(func) func_exprs_ ## func
+
+/**
+ * Declare a function list for reference in a struct pin_prio.
+ *
+ * @func: A macro symbol name for the function (is subjected to token pasting)
+ * @...: Function expression structure pointers (use FUNC_EXPR_POINTER())
+ */
+#define FUNC_EXPR_LIST_DECL(func, ...) \
+	const struct func_expr *FUNC_EXPR_LIST_SYM(func)[] = { __VA_ARGS__ }
+
+/**
+ * A short-hand macro for declaring an expression list consisting of a single
+ * function.
+ *
+ * @func: A macro symbol name for the function (is subjected to token pasting)
+ */
+#define FUNC_EXPR_LIST_DECL_SINGLE(func) \
+	FUNC_EXPR_LIST_DECL(func, FUNC_EXPR_PTR(func), NULL)
+
+#define FUNC_EXPR_LIST_PTR(func) (&FUNC_EXPR_LIST_SYM(func)[0])
+
+/**
+ * A short-hand macro for declaring a function expression and an expression
+ * list with a single function.
+ *
+ * @func: A macro symbol name for the function (is subjected to token pasting)
+ * @...: Function descriptors that define the function expression
+ */
+#define FUNC_EXPR_DECL_SINGLE(func, ...) \
+	FUNC_DESC_LIST_DECL(func, __VA_ARGS__); \
+	FUNC_EXPR_DECL_(func); \
+	FUNC_EXPR_LIST_DECL(func, FUNC_EXPR_PTR(func), NULL)
+
+#define PIN_SYM(pin) pin_ ## pin
+
+#define MS_PIN_DECL_(pin, other, high, low) \
+	static const struct pin_prio PIN_SYM(pin) = { high, low, #other }
+
+/**
+ * Declare a multi-signal pin
+ *
+ * @pin: The pin number
+ * @other: Macro name for "other" functionality (subjected to stringification)
+ * @high: Macro name for the high signal functions
+ * @low: Macro name for the low signal functions
+ *
+ * For example:
+ *
+ *    FUNC_EXPR_DECL_SINGLE(SD1, FUNC_DESC_SET(SCU90, 0));
+ *    FUNC_EXPR_DECL_SINGLE(I2C10, FUNC_DESC_SET(SCU90, 23));
+ *
+ *    #define C4 16
+ *    MS_PIN_DECL(C4, GPIOC0, SD1, I2C10);
+ *    #define B3 17
+ *    MS_PIN_DECL(B3, GPIOC1, SD1, I2C10);
+ */
+#define MS_PIN_DECL(pin, other, high, low) \
+	MS_PIN_DECL_(pin, other, \
+			FUNC_EXPR_LIST_PTR(high), \
+			FUNC_EXPR_LIST_PTR(low))
+
+#define PIN_GROUP_SYM(func) pins_ ## func
+#define FUNC_GROUP_SYM(func) groups_ ## func
+#define FUNC_GROUP_DECL(func, ...) \
+	static const int PIN_GROUP_SYM(func)[] = { __VA_ARGS__ }; \
+	static const char *const FUNC_GROUP_SYM(func)[] = { #func }
+
+/**
+ * Declare a single signal pin
+ *
+ * @pin: The pin number
+ * @other: Macro name for "other" functionality (subjected to stringification)
+ * @func: Macro name for the function
+ *
+ * For example:
+ *
+ *    FUNC_EXPR_DECL_SINGLE(I2C5, FUNC_DESC_SET(SCU90, 18));
+ *
+ *    #define E3 80
+ *    SS_PIN_DECL(E3, GPIOK0, I2C5);
+ *
+ *    #define D2 81
+ *    SS_PIN_DECL(D2, GPIOK1, I2C5);
+ */
+#define SS_PIN_DECL(pin, other, func) \
+	MS_PIN_DECL_(pin, other, FUNC_EXPR_LIST_PTR(func), NULL)
+
+/**
+ * Single signal, single function pin declaration
+ *
+ * @pin: The pin number
+ * @other: Macro name for "other" functionality (subjected to stringification)
+ * @func: Macro name for the function
+ * @...: Function descriptors that define the function expression
+ *
+ * For example:
+ *
+ *    SSSF_PIN_DECL(A4, GPIOA2, TIMER3, FUNC_DESC_SET(SCU80, 2));
+ */
+#define SSSF_PIN_DECL(pin, other, func, ...) \
+	FUNC_EXPR_DECL(func, __VA_ARGS__); \
+	FUNC_EXPR_LIST_DECL(func, FUNC_EXPR_PTR(func), NULL); \
+	MS_PIN_DECL_(pin, other, FUNC_EXPR_LIST_PTR(func), NULL); \
+	FUNC_GROUP_DECL(func, pin)
+
+#define SCU3C 0x3C
+#define STRAP 0x70
+#define SCU80 0x80
+#define SCU84 0x84
+#define SCU88 0x88
+#define SCU8C 0x8C
+#define SCU90 0x90
+#define SCU94 0x94
+#define SCUA4 0xA4
+
+/* Use undefined macros for symbol naming and references, eg GPIOA0, MAC1LINK,
+ * TIMER3 etc.
+ */
+
+/* Pins are defined in GPIO bank order:
+ *
+ * GPIOA0: 0
+ * ...
+ * GPIOA7: 7
+ * ...
+ * GPIOB0: 8
+ * ...
+ * GPIOZ7: 207
+ * GPIOAA0: 208
+ * ...
+ * GPIOAB3: 219
+ *
+ * Not all pins have their signals defined.
+ */
+
+#define A4 2
+SSSF_PIN_DECL(A4, GPIOA2, TIMER3, FUNC_DESC_SET(SCU80, 2));
+
+FUNC_EXPR_DECL_SINGLE(SD1, FUNC_DESC_SET(SCU90, 0));
+
+FUNC_EXPR_DECL_SINGLE(I2C10, FUNC_DESC_SET(SCU90, 23));
+
+#define C4 16
+MS_PIN_DECL(C4, GPIOC0, SD1, I2C10);
+#define B3 17
+MS_PIN_DECL(B3, GPIOC1, SD1, I2C10);
+
+FUNC_GROUP_DECL(I2C10, C4, B3);
+
+FUNC_EXPR_DECL_SINGLE(I2C11, FUNC_DESC_SET(SCU90, 24));
+
+#define A2 18
+MS_PIN_DECL(A2, GPIOC2, SD1, I2C11);
+#define E5 19
+MS_PIN_DECL(E5, GPIOC3, SD1, I2C11);
+
+FUNC_GROUP_DECL(I2C11, A2, E5);
+
+FUNC_EXPR_DECL_SINGLE(I2C12, FUNC_DESC_SET(SCU90, 25));
+
+#define D4 20
+MS_PIN_DECL(D4, GPIOC4, SD1, I2C12);
+#define C3 21
+MS_PIN_DECL(C3, GPIOC5, SD1, I2C12);
+
+FUNC_GROUP_DECL(I2C12, D4, C3);
+
+FUNC_EXPR_DECL_SINGLE(I2C13, FUNC_DESC_SET(SCU90, 26));
+
+#define B2 22
+MS_PIN_DECL(B2, GPIOC6, SD1, I2C13);
+#define A1 23
+MS_PIN_DECL(A1, GPIOC7, SD1, I2C13);
+
+FUNC_GROUP_DECL(I2C13, B2, A1);
+FUNC_GROUP_DECL(SD1, C4, B3, A2, E5, D4, C3, B2, A1);
+
+#define D18 40
+SSSF_PIN_DECL(D18, GPIOF0, NCTS4, FUNC_DESC_SET(SCU80, 24));
+
+FUNC_EXPR_DECL(ACPI, FUNC_DESC_BIT(STRAP, 19, 0));
+
+#define B19 41
+FUNC_EXPR_DECL(NDCD4, FUNC_DESC_SET(SCU80, 25));
+FUNC_EXPR_LIST_DECL_SINGLE(NDCD4);
+FUNC_EXPR_DECL(SIOPBI, FUNC_DESC_SET(SCUA4, 12));
+FUNC_EXPR_LIST_DECL(SIOPBI, FUNC_EXPR_PTR(SIOPBI), FUNC_EXPR_PTR(ACPI), NULL);
+MS_PIN_DECL(B19, GPIOF1, NDCD4, SIOPBI);
+FUNC_GROUP_DECL(NDCD4, B19);
+FUNC_GROUP_DECL(SIOPBI, B19);
+
+#define D17 43
+FUNC_EXPR_DECL(NRI4, FUNC_DESC_SET(SCU80, 27));
+FUNC_EXPR_LIST_DECL_SINGLE(NRI4);
+FUNC_EXPR_DECL(SIOPBO, FUNC_DESC_SET(SCUA4, 14));
+FUNC_EXPR_LIST_DECL(SIOPBO, FUNC_EXPR_PTR(SIOPBO), FUNC_EXPR_PTR(ACPI), NULL);
+MS_PIN_DECL(D17, GPIOF3, NRI4, SIOPBO);
+FUNC_GROUP_DECL(NRI4, D17);
+FUNC_GROUP_DECL(SIOPBO, D17);
+
+FUNC_GROUP_DECL(ACPI, B19, D17);
+
+#define E16 46
+SSSF_PIN_DECL(E16, GPIOF6, TXD4, FUNC_DESC_SET(SCU80, 30));
+
+#define C17 47
+SSSF_PIN_DECL(C17, GPIOF7, RXD4, FUNC_DESC_SET(SCU80, 31));
+
+#define AA22 54
+SSSF_PIN_DECL(AA22, GPIOG6, FLBUSY, FUNC_DESC_SET(SCU84, 6));
+
+#define U18 55
+SSSF_PIN_DECL(U18, GPIOG7, FLWP, FUNC_DESC_SET(SCU84, 7));
+
+/* The following descriptors describe incomplete list of signals for VPO/ROM,
+ * enough to cover the pins which we need to configure in the OpenPOWER
+ * Palmetto BMC. Some functions require separate functions be disabled, so
+ * we're describing masks for those separate functions and setting the enabled
+ * value to the disabled value. Also, GENMASK() is used to require a range of
+ * set bits, where each signal has its own activation bit rather than all being
+ * activated by a single bit.
+ */
+#define VPOOFF0_DESC { SCU94, GENMASK(1, 0), 0, 0 }
+#define ROMEN_0_DESC \
+	{ SCU88, GENMASK(29, 24), GENMASK(5, 0), 0 }
+#define ROMEN_1_DESC \
+	{ SCU8C, GENMASK(7, 0), GENMASK(7, 0), 0 }
+#define ROMEN_2_DESC { SCU94, BIT_MASK(1), 0, 0 }
+#define ROMEN_3_DESC \
+	{ STRAP, GENMASK(1, 0), 0, 0 }
+
+FUNC_EXPR_DECL(ROM8,
+		VPOOFF0_DESC,
+		ROMEN_0_DESC,
+		ROMEN_1_DESC,
+		ROMEN_2_DESC);
+
+/* ROM16 enabled by SCU90 */
+FUNC_EXPR_DECL(ROM16,
+		VPOOFF0_DESC,
+		ROMEN_0_DESC,
+		ROMEN_1_DESC,
+		ROMEN_2_DESC,
+		FUNC_DESC_SET(SCU90, 6));
+
+/* ROM16 enabled by strapping register */
+FUNC_EXPR_DECL(ROM16S,
+		VPOOFF0_DESC,
+		ROMEN_0_DESC,
+		ROMEN_1_DESC,
+		ROMEN_2_DESC,
+		FUNC_DESC_SET(STRAP, 4),
+		ROMEN_3_DESC);
+
+FUNC_EXPR_LIST_DECL(ROM16,
+		FUNC_EXPR_PTR(ROM16),
+		FUNC_EXPR_PTR(ROM16S),
+		NULL);
+
+FUNC_EXPR_DECL_SINGLE(UART6, FUNC_DESC_SET(SCU90, 7));
+
+#define A8 56
+MS_PIN_DECL(A8, GPIOH0, ROM16, UART6);
+
+#define C7 57
+MS_PIN_DECL(C7, GPIOH1, ROM16, UART6);
+
+#define B7 58
+MS_PIN_DECL(B7, GPIOH2, ROM16, UART6);
+
+#define A7 59
+MS_PIN_DECL(A7, GPIOH3, ROM16, UART6);
+
+#define D7 60
+MS_PIN_DECL(D7, GPIOH4, ROM16, UART6);
+
+#define B6 61
+MS_PIN_DECL(B6, GPIOH5, ROM16, UART6);
+
+#define A6 62
+MS_PIN_DECL(A6, GPIOH6, ROM16, UART6);
+
+#define E7 63
+MS_PIN_DECL(E7, GPIOH7, ROM16, UART6);
+
+FUNC_GROUP_DECL(UART6, A8, C7, B7, A7, D7, B6, A6, E7);
+
+#define T4 76
+SSSF_PIN_DECL(T4, GPIOJ4, VGAHS, FUNC_DESC_SET(SCU84, 12));
+
+#define U2 77
+SSSF_PIN_DECL(U2, GPIOJ5, VGAVS, FUNC_DESC_SET(SCU84, 13));
+
+#define T2 78
+SSSF_PIN_DECL(T2, GPIOJ6, DDCCLK, FUNC_DESC_SET(SCU84, 14));
+
+#define T1 79
+SSSF_PIN_DECL(T1, GPIOJ7, DDCDAT, FUNC_DESC_SET(SCU84, 15));
+
+FUNC_EXPR_DECL_SINGLE(I2C5, FUNC_DESC_SET(SCU90, 18));
+
+#define E3 80
+SS_PIN_DECL(E3, GPIOK0, I2C5);
+
+#define D2 81
+SS_PIN_DECL(D2, GPIOK1, I2C5);
+
+FUNC_GROUP_DECL(I2C5, E3, D2);
+
+FUNC_EXPR_DECL_SINGLE(I2C6, FUNC_DESC_SET(SCU90, 19));
+
+#define C1 82
+SS_PIN_DECL(C1, GPIOK2, I2C6);
+
+#define F4 83
+SS_PIN_DECL(F4, GPIOK3, I2C6);
+
+FUNC_GROUP_DECL(I2C6, C1, F4);
+
+FUNC_EXPR_DECL_SINGLE(I2C7, FUNC_DESC_SET(SCU90, 20));
+
+#define E2 84
+SS_PIN_DECL(E2, GPIOK4, I2C7);
+
+#define D1 85
+SS_PIN_DECL(D1, GPIOK5, I2C7);
+
+FUNC_GROUP_DECL(I2C7, E2, D1);
+
+FUNC_EXPR_DECL_SINGLE(I2C8, FUNC_DESC_SET(SCU90, 21));
+
+#define G5 86
+SS_PIN_DECL(G5, GPIOK6, I2C8);
+
+#define F3 87
+SS_PIN_DECL(F3, GPIOK7, I2C8);
+
+FUNC_GROUP_DECL(I2C8, G5, F3);
+
+#define U1 88
+SSSF_PIN_DECL(U1, GPIOL0, NCTS1, FUNC_DESC_SET(SCU84, 16));
+
+#define VPI18_DESC { SCU84, GENMASK(5, 4), 1, 0 }
+#define VPI24_DESC { SCU84, GENMASK(5, 4), 2, 0 }
+#define VPI30_DESC { SCU84, GENMASK(5, 4), 3, 0 }
+
+#define VPIEN_0_DESC \
+	{ SCU84, GENMASK(21, 17), GENMASK(4, 0), 0 }
+#define VPIEN_1_DESC \
+	{ SCU84, GENMASK(23, 22), GENMASK(1, 0), 0 }
+#define VPIEN_2_DESC \
+	{ SCU88, GENMASK(1, 0), GENMASK(1, 0), 0 }
+#define VPIEN_3_DESC \
+	{ SCU88, GENMASK(5, 2), GENMASK(3, 0), 0 }
+#define VPIEN_4_DESC \
+	{ SCU88, GENMASK(7, 6), GENMASK(1, 0), 0 }
+
+FUNC_EXPR_DECL(VPI18,
+		VPI18_DESC,
+		VPIEN_0_DESC,
+		VPIEN_3_DESC);
+
+FUNC_EXPR_DECL(VPI24,
+		VPI24_DESC,
+		VPIEN_0_DESC,
+		VPIEN_3_DESC,
+		VPIEN_4_DESC);
+
+FUNC_EXPR_DECL(VPI30,
+		VPI30_DESC,
+		VPIEN_0_DESC,
+		VPIEN_1_DESC,
+		VPIEN_2_DESC,
+		VPIEN_3_DESC);
+
+FUNC_EXPR_LIST_DECL(VPI,
+		FUNC_EXPR_PTR(VPI18),
+		FUNC_EXPR_PTR(VPI24),
+		FUNC_EXPR_PTR(VPI30),
+		NULL);
+
+#define T5 89
+FUNC_EXPR_DECL_SINGLE(NDCD1, FUNC_DESC_SET(SCU84, 17));
+MS_PIN_DECL(T5, GPIOL1, VPI, NDCD1);
+FUNC_GROUP_DECL(NDCD1, T5);
+
+#define U3 90
+FUNC_EXPR_DECL_SINGLE(NDSR1, FUNC_DESC_SET(SCU84, 18));
+MS_PIN_DECL(U3, GPIOL2, VPI, NDSR1);
+FUNC_GROUP_DECL(NDSR1, U3);
+
+#define V1 91
+FUNC_EXPR_DECL_SINGLE(NRI1, FUNC_DESC_SET(SCU84, 19));
+MS_PIN_DECL(V1, GPIOL3, VPI, NRI1);
+FUNC_GROUP_DECL(NRI1, V1);
+
+#define U4 92
+FUNC_EXPR_DECL_SINGLE(NDTR1, FUNC_DESC_SET(SCU84, 20));
+MS_PIN_DECL(U4, GPIOL4, VPI, NDTR1);
+FUNC_GROUP_DECL(NDTR1, U4);
+
+#define V2 93
+FUNC_EXPR_DECL_SINGLE(NRTS1, FUNC_DESC_SET(SCU84, 21));
+MS_PIN_DECL(V2, GPIOL5, VPI, NRTS1);
+FUNC_GROUP_DECL(NRTS1, V2);
+
+#define W1 94
+FUNC_EXPR_DECL_SINGLE(TXD1, FUNC_DESC_SET(SCU84, 22));
+MS_PIN_DECL(W1, GPIOL6, VPI, TXD1);
+FUNC_GROUP_DECL(TXD1, W1);
+
+#define U5 95
+FUNC_EXPR_DECL_SINGLE(RXD1, FUNC_DESC_SET(SCU84, 23));
+MS_PIN_DECL(U5, GPIOL7, VPI, RXD1);
+FUNC_GROUP_DECL(RXD1, U5);
+
+#define W4 104
+FUNC_EXPR_DECL_SINGLE(PWM0, FUNC_DESC_SET(SCU88, 0));
+MS_PIN_DECL(W4, GPION0, VPI, PWM0);
+FUNC_GROUP_DECL(PWM0, W4);
+
+#define Y3 105
+FUNC_EXPR_DECL_SINGLE(PWM1, FUNC_DESC_SET(SCU88, 1));
+MS_PIN_DECL(Y3, GPION1, VPI, PWM1);
+FUNC_GROUP_DECL(PWM1, Y3);
+
+#define AA2 106
+FUNC_EXPR_DECL_SINGLE(PWM2, FUNC_DESC_SET(SCU88, 2));
+MS_PIN_DECL(AA2, GPION2, VPI, PWM2);
+FUNC_GROUP_DECL(PWM2, AA2);
+
+#define AB1 107
+FUNC_EXPR_DECL_SINGLE(PWM3, FUNC_DESC_SET(SCU88, 3));
+MS_PIN_DECL(AB1, GPION3, VPI, PWM3);
+FUNC_GROUP_DECL(PWM3, AB1);
+
+#define W5 108
+FUNC_EXPR_DECL_SINGLE(PWM4, FUNC_DESC_SET(SCU88, 4));
+MS_PIN_DECL(W5, GPION4, VPI, PWM4);
+FUNC_GROUP_DECL(PWM4, W5);
+
+#define Y4 109
+FUNC_EXPR_DECL_SINGLE(PWM5, FUNC_DESC_SET(SCU88, 5));
+MS_PIN_DECL(Y4, GPION5, VPI, PWM5);
+FUNC_GROUP_DECL(PWM5, Y4);
+
+#define AA3 110
+FUNC_EXPR_DECL_SINGLE(PWM6, FUNC_DESC_SET(SCU88, 6));
+MS_PIN_DECL(AA3, GPION6, VPI, PWM6);
+FUNC_GROUP_DECL(PWM6, AA3);
+
+#define AB2 111
+FUNC_EXPR_DECL_SINGLE(PWM7, FUNC_DESC_SET(SCU88, 7));
+MS_PIN_DECL(AB2, GPION7, VPI, PWM7);
+FUNC_GROUP_DECL(PWM7, AB2);
+
+FUNC_GROUP_DECL(VPI18, T5, U3, V1, U4, V2, AA22, W5, Y4, AA3, AB2);
+FUNC_GROUP_DECL(VPI24, T5, U3, V1, U4, V2, AA22, W5, Y4, AA3, AB2);
+FUNC_GROUP_DECL(VPI30, T5, U3, V1, U4, V2, W1, U5, W4, Y3, AA22, W5, Y4, AA3,
+		AB2);
+
+#define AA7 126
+SSSF_PIN_DECL(AA7, GPIOP6, BMCINT, FUNC_DESC_SET(SCU88, 22));
+
+#define AB7 127
+SSSF_PIN_DECL(AB7, GPIOP7, FLACK, FUNC_DESC_SET(SCU88, 23));
+
+FUNC_EXPR_DECL(I2C3, FUNC_DESC_SET(SCU90, 16));
+FUNC_EXPR_LIST_DECL_SINGLE(I2C3);
+
+#define D3 128
+SS_PIN_DECL(D3, GPIOQ0, I2C3);
+
+#define C2 129
+SS_PIN_DECL(C2, GPIOQ1, I2C3);
+
+FUNC_GROUP_DECL(I2C3, D3, C2);
+
+FUNC_EXPR_DECL_SINGLE(I2C4, FUNC_DESC_SET(SCU90, 17));
+
+#define B1 130
+SS_PIN_DECL(B1, GPIOQ2, I2C4);
+
+#define F5 131
+SS_PIN_DECL(F5, GPIOQ3, I2C4);
+
+FUNC_GROUP_DECL(I2C4, B1, F5);
+
+#define VPOOFF1_DESC { SCU94, GENMASK(1, 0), 3, 0 }
+#define VPO12_DESC { SCU94, GENMASK(1, 0), 1, 0 }
+#define VPO24_DESC { SCU94, GENMASK(1, 0), 2, 0 }
+
+/* Enables an incomplete list of signals, only enough to cover the pins that we
+ * need to describe at the moment
+ */
+#define VPOEN_0_DESC \
+	{ SCU88, GENMASK(29, 28), GENMASK(1, 0), 0 }
+#define VPOEN_1_DESC \
+	{ SCU8C, GENMASK(3, 0), GENMASK(3, 0), 0 }
+#define VPOEN_2_DESC \
+	{ SCU8C, GENMASK(7, 6), GENMASK(1, 0), 0 }
+
+FUNC_EXPR_LIST_DECL(ROM,
+		FUNC_EXPR_PTR(ROM8),
+		FUNC_EXPR_PTR(ROM16),
+		FUNC_EXPR_PTR(ROM16S),
+		NULL);
+
+#define V20 136
+MS_PIN_DECL_(V20, GPIOR0, FUNC_EXPR_LIST_PTR(ROM), NULL);
+
+FUNC_EXPR_DECL(VPO12,
+		VPO12_DESC,
+		VPOEN_0_DESC,
+		VPOEN_1_DESC,
+		VPOEN_2_DESC);
+
+FUNC_EXPR_DECL(VPO24,
+		VPO24_DESC,
+		VPOEN_0_DESC,
+		VPOEN_1_DESC,
+		VPOEN_2_DESC);
+
+FUNC_EXPR_DECL(VPOOFF1,
+		VPOOFF1_DESC,
+		VPOEN_0_DESC,
+		VPOEN_1_DESC,
+		VPOEN_2_DESC);
+
+FUNC_EXPR_LIST_DECL(VPO,
+		FUNC_EXPR_PTR(VPO12),
+		FUNC_EXPR_PTR(VPO24),
+		FUNC_EXPR_PTR(VPOOFF1),
+		NULL);
+
+#define U21 144
+MS_PIN_DECL(U21, GPIOS0, ROM, VPO);
+
+#define T19 145
+MS_PIN_DECL(T19, GPIOS1, ROM, VPO);
+
+#define V22 146
+MS_PIN_DECL(V22, GPIOS2, ROM, VPO);
+
+#define U20 147
+MS_PIN_DECL(U20, GPIOS3, ROM, VPO);
+
+#define R18 148
+MS_PIN_DECL_(R18, GPIOS4, FUNC_EXPR_LIST_PTR(ROM), NULL);
+
+#define N21 149
+MS_PIN_DECL_(N21, GPIOS5, FUNC_EXPR_LIST_PTR(ROM), NULL);
+
+#define L22 150
+MS_PIN_DECL(L22, GPIOS6, ROM, VPO);
+
+#define K18 151
+MS_PIN_DECL(K18, GPIOS7, ROM, VPO);
+
+FUNC_GROUP_DECL(ROM8, V20, U21, T19, V22, U20, R18, N21, L22, K18);
+FUNC_GROUP_DECL(ROM16, V20, U21, T19, V22, U20, R18, N21, L22, K18,
+		A8, C7, B7, A7, D7, B6, A6, E7);
+FUNC_GROUP_DECL(VPO12, U21, T19, V22, U20);
+FUNC_GROUP_DECL(VPO24, U21, T19, V22, U20, L22, K18);
+
+#define AST_PINCTRL_PIN(name_) \
+	[name_] = { \
+		.number = name_, \
+		.name = #name_, \
+		.drv_data = (void *) &(PIN_SYM(name_)) \
+	}
+
+/* Note we account for GPIOY4-GPIOY7 even though they're not valid, thus 216
+ * pins becomes 220.
+ */
+#define AST2400_NR_GPIOS 220
+
+static struct pinctrl_pin_desc ast2400_pins[AST2400_NR_GPIOS] = {
+	AST_PINCTRL_PIN(A4),
+	AST_PINCTRL_PIN(C4),
+	AST_PINCTRL_PIN(B3),
+	AST_PINCTRL_PIN(A2),
+	AST_PINCTRL_PIN(E5),
+	AST_PINCTRL_PIN(D4),
+	AST_PINCTRL_PIN(C3),
+	AST_PINCTRL_PIN(B2),
+	AST_PINCTRL_PIN(A1),
+	AST_PINCTRL_PIN(D18),
+	AST_PINCTRL_PIN(B19),
+	AST_PINCTRL_PIN(D17),
+	AST_PINCTRL_PIN(E16),
+	AST_PINCTRL_PIN(C17),
+	AST_PINCTRL_PIN(AA22),
+	AST_PINCTRL_PIN(U18),
+	AST_PINCTRL_PIN(A8),
+	AST_PINCTRL_PIN(C7),
+	AST_PINCTRL_PIN(B7),
+	AST_PINCTRL_PIN(A7),
+	AST_PINCTRL_PIN(D7),
+	AST_PINCTRL_PIN(B6),
+	AST_PINCTRL_PIN(A6),
+	AST_PINCTRL_PIN(E7),
+	AST_PINCTRL_PIN(T4),
+	AST_PINCTRL_PIN(U2),
+	AST_PINCTRL_PIN(T2),
+	AST_PINCTRL_PIN(T1),
+	AST_PINCTRL_PIN(E3),
+	AST_PINCTRL_PIN(D2),
+	AST_PINCTRL_PIN(C1),
+	AST_PINCTRL_PIN(F4),
+	AST_PINCTRL_PIN(E2),
+	AST_PINCTRL_PIN(D1),
+	AST_PINCTRL_PIN(G5),
+	AST_PINCTRL_PIN(F3),
+	AST_PINCTRL_PIN(U1),
+	AST_PINCTRL_PIN(T5),
+	AST_PINCTRL_PIN(U3),
+	AST_PINCTRL_PIN(V1),
+	AST_PINCTRL_PIN(U4),
+	AST_PINCTRL_PIN(V2),
+	AST_PINCTRL_PIN(W1),
+	AST_PINCTRL_PIN(U5),
+	AST_PINCTRL_PIN(W4),
+	AST_PINCTRL_PIN(Y3),
+	AST_PINCTRL_PIN(AA2),
+	AST_PINCTRL_PIN(AB1),
+	AST_PINCTRL_PIN(W5),
+	AST_PINCTRL_PIN(Y4),
+	AST_PINCTRL_PIN(AA3),
+	AST_PINCTRL_PIN(AB2),
+	AST_PINCTRL_PIN(AA7),
+	AST_PINCTRL_PIN(AB7),
+	AST_PINCTRL_PIN(D3),
+	AST_PINCTRL_PIN(C2),
+	AST_PINCTRL_PIN(B1),
+	AST_PINCTRL_PIN(F5),
+	AST_PINCTRL_PIN(V20),
+	AST_PINCTRL_PIN(U21),
+	AST_PINCTRL_PIN(T19),
+	AST_PINCTRL_PIN(V22),
+	AST_PINCTRL_PIN(U20),
+	AST_PINCTRL_PIN(R18),
+	AST_PINCTRL_PIN(N21),
+	AST_PINCTRL_PIN(L22),
+	AST_PINCTRL_PIN(K18),
+};
+
+struct ast2400_pin_group {
+	const char *name;
+	const unsigned int *pins;
+	const unsigned npins;
+};
+
+#define AST_PINCTRL_GROUP(name_) { \
+	.name = #name_, \
+	.pins = &(PIN_GROUP_SYM(name_))[0], \
+	.npins = ARRAY_SIZE(PIN_GROUP_SYM(name_)), \
+}
+
+static const struct ast2400_pin_group ast2400_groups[] = {
+	AST_PINCTRL_GROUP(TIMER3),
+	AST_PINCTRL_GROUP(SD1),
+	AST_PINCTRL_GROUP(I2C10),
+	AST_PINCTRL_GROUP(I2C11),
+	AST_PINCTRL_GROUP(I2C12),
+	AST_PINCTRL_GROUP(I2C13),
+	AST_PINCTRL_GROUP(NCTS4),
+	AST_PINCTRL_GROUP(ACPI),
+	AST_PINCTRL_GROUP(NDCD4),
+	AST_PINCTRL_GROUP(SIOPBI),
+	AST_PINCTRL_GROUP(NRI4),
+	AST_PINCTRL_GROUP(SIOPBO),
+	AST_PINCTRL_GROUP(TXD4),
+	AST_PINCTRL_GROUP(RXD4),
+	AST_PINCTRL_GROUP(FLBUSY),
+	AST_PINCTRL_GROUP(FLWP),
+	AST_PINCTRL_GROUP(UART6),
+	AST_PINCTRL_GROUP(VGAHS),
+	AST_PINCTRL_GROUP(VGAVS),
+	AST_PINCTRL_GROUP(DDCCLK),
+	AST_PINCTRL_GROUP(DDCDAT),
+	AST_PINCTRL_GROUP(I2C5),
+	AST_PINCTRL_GROUP(I2C6),
+	AST_PINCTRL_GROUP(I2C7),
+	AST_PINCTRL_GROUP(I2C8),
+	AST_PINCTRL_GROUP(NCTS1),
+	AST_PINCTRL_GROUP(NDCD1),
+	AST_PINCTRL_GROUP(NDSR1),
+	AST_PINCTRL_GROUP(NRI1),
+	AST_PINCTRL_GROUP(NDTR1),
+	AST_PINCTRL_GROUP(NRTS1),
+	AST_PINCTRL_GROUP(TXD1),
+	AST_PINCTRL_GROUP(RXD1),
+	AST_PINCTRL_GROUP(PWM0),
+	AST_PINCTRL_GROUP(PWM1),
+	AST_PINCTRL_GROUP(PWM2),
+	AST_PINCTRL_GROUP(PWM3),
+	AST_PINCTRL_GROUP(PWM4),
+	AST_PINCTRL_GROUP(PWM5),
+	AST_PINCTRL_GROUP(PWM6),
+	AST_PINCTRL_GROUP(PWM7),
+	AST_PINCTRL_GROUP(VPI18),
+	AST_PINCTRL_GROUP(VPI24),
+	AST_PINCTRL_GROUP(VPI30),
+	AST_PINCTRL_GROUP(BMCINT),
+	AST_PINCTRL_GROUP(FLACK),
+	AST_PINCTRL_GROUP(I2C3),
+	AST_PINCTRL_GROUP(I2C4),
+	AST_PINCTRL_GROUP(ROM8),
+	AST_PINCTRL_GROUP(ROM16),
+	AST_PINCTRL_GROUP(VPO12),
+	AST_PINCTRL_GROUP(VPO24),
+};
+
+struct ast2400_pin_function {
+	const char *name;
+	const char *const *groups;
+	unsigned ngroups;
+	const struct func_expr *expr;
+};
+
+#define AST_PINCTRL_FUNC(name_, ...) { \
+	.name = #name_, \
+	.groups = &FUNC_GROUP_SYM(name_)[0], \
+	.ngroups = ARRAY_SIZE(FUNC_GROUP_SYM(name_)), \
+	.expr = FUNC_EXPR_PTR(name_), \
+}
+
+static const struct ast2400_pin_function ast2400_functions[] = {
+	AST_PINCTRL_FUNC(TIMER3),
+	AST_PINCTRL_FUNC(SD1),
+	AST_PINCTRL_FUNC(I2C10),
+	AST_PINCTRL_FUNC(I2C11),
+	AST_PINCTRL_FUNC(I2C12),
+	AST_PINCTRL_FUNC(I2C13),
+	AST_PINCTRL_FUNC(NCTS4),
+	AST_PINCTRL_FUNC(ACPI),
+	AST_PINCTRL_FUNC(NDCD4),
+	AST_PINCTRL_FUNC(SIOPBI),
+	AST_PINCTRL_FUNC(NRI4),
+	AST_PINCTRL_FUNC(SIOPBO),
+	AST_PINCTRL_FUNC(TXD4),
+	AST_PINCTRL_FUNC(RXD4),
+	AST_PINCTRL_FUNC(FLBUSY),
+	AST_PINCTRL_FUNC(FLWP),
+	AST_PINCTRL_FUNC(UART6),
+	AST_PINCTRL_FUNC(VGAHS),
+	AST_PINCTRL_FUNC(VGAVS),
+	AST_PINCTRL_FUNC(DDCCLK),
+	AST_PINCTRL_FUNC(DDCDAT),
+	AST_PINCTRL_FUNC(I2C5),
+	AST_PINCTRL_FUNC(I2C6),
+	AST_PINCTRL_FUNC(I2C7),
+	AST_PINCTRL_FUNC(I2C8),
+	AST_PINCTRL_FUNC(NCTS1),
+	AST_PINCTRL_FUNC(NDCD1),
+	AST_PINCTRL_FUNC(NDSR1),
+	AST_PINCTRL_FUNC(NRI1),
+	AST_PINCTRL_FUNC(NDTR1),
+	AST_PINCTRL_FUNC(NRTS1),
+	AST_PINCTRL_FUNC(TXD1),
+	AST_PINCTRL_FUNC(RXD1),
+	AST_PINCTRL_FUNC(PWM0),
+	AST_PINCTRL_FUNC(PWM1),
+	AST_PINCTRL_FUNC(PWM2),
+	AST_PINCTRL_FUNC(PWM3),
+	AST_PINCTRL_FUNC(PWM4),
+	AST_PINCTRL_FUNC(PWM5),
+	AST_PINCTRL_FUNC(PWM6),
+	AST_PINCTRL_FUNC(PWM7),
+	AST_PINCTRL_FUNC(VPI18),
+	AST_PINCTRL_FUNC(VPI24),
+	AST_PINCTRL_FUNC(VPI30),
+	AST_PINCTRL_FUNC(BMCINT),
+	AST_PINCTRL_FUNC(FLACK),
+	AST_PINCTRL_FUNC(I2C3),
+	AST_PINCTRL_FUNC(I2C4),
+	AST_PINCTRL_FUNC(ROM8),
+	AST_PINCTRL_FUNC(ROM16),
+	AST_PINCTRL_FUNC(VPO12),
+	AST_PINCTRL_FUNC(VPO24),
+};
+
+struct ast2400_pinctrl_data {
+	void __iomem *reg_base;
+
+	const struct pinctrl_pin_desc *pins;
+	const unsigned npins;
+
+	const struct ast2400_pin_group *groups;
+	const unsigned ngroups;
+
+	const struct ast2400_pin_function *functions;
+	const unsigned nfunctions;
+};
+
+static struct ast2400_pinctrl_data ast2400_pinctrl = {
+	.pins = ast2400_pins,
+	.npins = ARRAY_SIZE(ast2400_pins),
+	.groups = ast2400_groups,
+	.ngroups = ARRAY_SIZE(ast2400_groups),
+	.functions = ast2400_functions,
+	.nfunctions = ARRAY_SIZE(ast2400_functions),
+};
+
+static int ast2400_pinctrl_get_groups_count(struct pinctrl_dev *pctldev)
+{
+	struct ast2400_pinctrl_data *pdata = pinctrl_dev_get_drvdata(pctldev);
+
+	return pdata->ngroups;
+}
+
+static const char *ast2400_pinctrl_get_group_name(struct pinctrl_dev *pctldev,
+		unsigned group)
+{
+	struct ast2400_pinctrl_data *pdata = pinctrl_dev_get_drvdata(pctldev);
+
+	return pdata->groups[group].name;
+}
+
+static int ast2400_pinctrl_get_group_pins(struct pinctrl_dev *pctldev,
+		unsigned group, const unsigned **pins, unsigned *npins)
+{
+	struct ast2400_pinctrl_data *pdata = pinctrl_dev_get_drvdata(pctldev);
+
+	*pins = &pdata->groups[group].pins[0];
+	*npins = pdata->groups[group].npins;
+
+	return 0;
+}
+
+static void ast2400_pinctrl_pin_dbg_show(struct pinctrl_dev *pctldev,
+		struct seq_file *s, unsigned offset)
+{
+	seq_printf(s, " %s", dev_name(pctldev->dev));
+}
+
+static struct pinctrl_ops ast2400_pinctrl_ops = {
+	.get_groups_count = ast2400_pinctrl_get_groups_count,
+	.get_group_name = ast2400_pinctrl_get_group_name,
+	.get_group_pins = ast2400_pinctrl_get_group_pins,
+	.pin_dbg_show = ast2400_pinctrl_pin_dbg_show,
+	.dt_node_to_map = pinconf_generic_dt_node_to_map_pin,
+	.dt_free_map = pinctrl_utils_dt_free_map,
+};
+
+static int ast2400_pinmux_get_fn_count(struct pinctrl_dev *pctldev)
+{
+	struct ast2400_pinctrl_data *pdata = pinctrl_dev_get_drvdata(pctldev);
+
+	return pdata->nfunctions;
+}
+
+static const char *ast2400_pinmux_get_fn_name(struct pinctrl_dev *pctldev,
+						unsigned function)
+{
+	struct ast2400_pinctrl_data *pdata = pinctrl_dev_get_drvdata(pctldev);
+
+	return pdata->functions[function].name;
+}
+
+static int ast2400_pinmux_get_fn_groups(struct pinctrl_dev *pctldev,
+		unsigned function, const char * const **groups,
+		unsigned * const num_groups)
+{
+	struct ast2400_pinctrl_data *pdata = pinctrl_dev_get_drvdata(pctldev);
+
+	*groups = pdata->functions[function].groups;
+	*num_groups = pdata->functions[function].ngroups;
+
+	return 0;
+}
+
+static bool func_desc_eval(const struct func_desc *desc, void __iomem *base)
+{
+	u32 raw = ioread32(base + desc->reg);
+
+	return ((raw & desc->mask) >> __ffs(desc->mask)) == desc->enable;
+}
+
+static bool func_expr_eval(const struct func_expr *expr, void __iomem *base)
+{
+	bool ret = true;
+	int i;
+
+	for (i = 0; i < expr->ndescs; i++) {
+		const struct func_desc *desc = &expr->descs[i];
+
+		ret = ret && func_desc_eval(desc, base);
+	}
+
+	return ret;
+}
+
+static bool func_expr_enable(const struct func_expr *expr, void __iomem *base)
+{
+	int i;
+	int ret;
+
+	ret = func_expr_eval(expr, base);
+	if (ret)
+		return ret;
+
+	for (i = 0; i < expr->ndescs; i++) {
+		const struct func_desc *desc = &expr->descs[i];
+		u32 val;
+
+		if (desc->reg == STRAP)
+			continue;
+
+		val = ioread32(base + desc->reg);
+		val &= ~desc->mask;
+		val |= desc->enable << __ffs(desc->mask);
+		iowrite32(val, base + desc->reg);
+	}
+
+	ret = func_expr_eval(expr, base);
+	return ret;
+}
+
+static bool func_expr_disable(const struct func_expr *expr, void __iomem *base)
+{
+	int i;
+	bool ret;
+
+	/* Ensure that we change any unsupported state to disabled */
+	for (i = 0; i < expr->ndescs; i++) {
+		const struct func_desc *desc = &expr->descs[i];
+		u32 val;
+
+		if (desc->reg == STRAP)
+			continue;
+
+		val = ioread32(base + desc->reg);
+		val &= ~desc->mask;
+		val |= (desc->disable << __ffs(desc->mask));
+		iowrite32(val, base + desc->reg);
+	}
+
+	ret = !func_expr_eval(expr, base);
+	return ret;
+}
+
+static inline bool maybe_disable(const struct func_expr **expr,
+		void __iomem *base)
+{
+	bool ret;
+
+	if (!expr)
+		return true;
+
+	while (*expr) {
+		ret |= func_expr_disable(*expr, base);
+		expr++;
+	}
+	return ret;
+}
+
+static bool sig_in_exprs(const struct func_expr **exprs,
+		const struct func_expr *sig)
+{
+	if (!exprs)
+		return false;
+
+	while (*exprs) {
+		if (sig == *exprs)
+			return true;
+		exprs++;
+	}
+	return false;
+}
+
+static int ast2400_pinmux_set_mux(struct pinctrl_dev *pctldev,
+		unsigned function, unsigned group)
+{
+	int i;
+	const struct ast2400_pinctrl_data *pdata =
+		pinctrl_dev_get_drvdata(pctldev);
+	const struct ast2400_pin_group *pgroup = &pdata->groups[group];
+	const struct ast2400_pin_function *pfunc =
+		&pdata->functions[function];
+
+	for (i = 0; i < pgroup->npins; i++) {
+		int pin = pgroup->pins[i];
+		const struct pin_prio *pprio = pdata->pins[pin].drv_data;
+		const struct func_expr *func = pfunc->expr;
+
+		if (!pprio)
+			return -1;
+
+		/* If low priority is requested, ensure high is disabled */
+		if (sig_in_exprs(pprio->low, func)) {
+			if (!maybe_disable(pprio->high, pdata->reg_base)) {
+				pr_err("Failed to disable high priority function on pin %d\n",
+						pin);
+				return -1;
+			}
+		}
+
+		/* Nothing to do if we're already enabled */
+		if (func_expr_eval(func, pdata->reg_base))
+			continue;
+
+		/* Configure the pin */
+		if (!func_expr_enable(func, pdata->reg_base))
+			return -1;
+	}
+
+	return 0;
+}
+
+static int ast2400_gpio_request_enable(struct pinctrl_dev *pctldev,
+		struct pinctrl_gpio_range *range,
+		unsigned offset)
+{
+	unsigned pin = range->base + offset;
+	const struct ast2400_pinctrl_data *pdata =
+		pinctrl_dev_get_drvdata(pctldev);
+	const struct pin_prio *pprio =
+		pdata->pins[pin].drv_data;
+
+	if (!pprio)
+		return 0;
+
+	if (!maybe_disable(pprio->high, pdata->reg_base))
+		return -1;
+
+	if (!maybe_disable(pprio->low, pdata->reg_base))
+		return -1;
+
+	return 0;
+}
+
+static struct pinmux_ops ast2400_pinmux_ops = {
+	.get_functions_count = ast2400_pinmux_get_fn_count,
+	.get_function_name = ast2400_pinmux_get_fn_name,
+	.get_function_groups = ast2400_pinmux_get_fn_groups,
+	.set_mux = ast2400_pinmux_set_mux,
+	.gpio_request_enable = ast2400_gpio_request_enable,
+	.strict = true,
+};
+
+static int ast2400_pin_config_get(struct pinctrl_dev *pctldev,
+		unsigned pin,
+		unsigned long *config)
+{
+	return 0;
+}
+
+static int ast2400_pin_config_set(struct pinctrl_dev *pctldev,
+		unsigned pin,
+		unsigned long *configs,
+		unsigned num_configs)
+{
+	return 0;
+}
+
+static struct pinconf_ops ast2400_pinconf_ops = {
+	.pin_config_get = ast2400_pin_config_get,
+	.pin_config_set = ast2400_pin_config_set,
+};
+
+static struct pinctrl_desc ast2400_pinctrl_desc = {
+	.name = "pinctrl-ast2400",
+	.pins = ast2400_pins,
+	.npins = ARRAY_SIZE(ast2400_pins),
+	.pctlops = &ast2400_pinctrl_ops,
+	.pmxops = &ast2400_pinmux_ops,
+	.confops = &ast2400_pinconf_ops,
+	.owner = THIS_MODULE,
+};
+
+static struct pinctrl_gpio_range ast2400_gpios = {
+	.name = "ast2400-pctrl-gpio-range",
+	.npins = ARRAY_SIZE(ast2400_pins),
+};
+
+static int __init ast2400_pinctrl_probe(struct platform_device *pdev)
+{
+	struct ast2400_pinctrl_data *pdata = &ast2400_pinctrl;
+	struct resource *res;
+	struct pinctrl_dev *pctl;
+	int i;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	pdata->reg_base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(pdata->reg_base))
+		return PTR_ERR(pdata->reg_base);
+
+	/* We allocated space for all pins, make sure we initialise all pin
+	 * numbers. Those pins we haven't defined won't yet have had their
+	 * number initialised, and it's effectively a no-op for those which
+	 * have.
+	 */
+	for (i = 0; i < ARRAY_SIZE(ast2400_pins); i++)
+		ast2400_pins[i].number = i;
+
+	pctl = pinctrl_register(&ast2400_pinctrl_desc, &pdev->dev, pdata);
+
+	if (IS_ERR(pctl)) {
+		dev_err(&pdev->dev, "Failed to register pinctrl\n");
+		return PTR_ERR(pctl);
+	}
+
+	platform_set_drvdata(pdev, pdata);
+
+	/* grange.name = "exynos5440-pctrl-gpio-range"; */
+	pinctrl_add_gpio_range(pctl, &ast2400_gpios);
+
+	return 0;
+}
+
+static const struct of_device_id ast2400_pinctrl_of_match[] = {
+	{ .compatible = "aspeed,ast2400-pinctrl", },
+	{ },
+};
+
+static struct platform_driver ast2400_pinctrl_driver = {
+	.driver = {
+		.name = "pinctrl-ast2400",
+		.of_match_table = ast2400_pinctrl_of_match,
+	},
+};
+
+module_platform_driver_probe(ast2400_pinctrl_driver, ast2400_pinctrl_probe);
+
+MODULE_AUTHOR("Andrew Jeffery <andrew@aj.id.au>");
+MODULE_DESCRIPTION("ASPEED AST2400 pinctrl driver");
+MODULE_LICENSE("GPL v2");
-- 
2.7.4

Re: [RFC PATCH] drivers/pinctrl: Add pinctrl-ast2400

[Linus Walleij]

Hi sorry for taking so long before reviewing. Too busy, what can I say.

On Fri, May 6, 2016 at 8:20 AM, Andrew Jeffery <andrew@aj.id.au> wrote:

> Add pinctrl/pinmux support for the Aspeed AST2400 SoC. Configuration of
> the SoC is somewhat involved: Enabling functions can involve writes of
> multiple bits to multiple registers, and signals provided by a pin are
> determined on a priority basis. That is, without care, it's possible to
> configure a function in the mux and to not gain expected functionality.
>
> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>

> +#include <linux/bitops.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>

You should include your changes to Kconfig and Makefile in the patch
so it is a complete driver. This is an example of why: if I can't see if
your driver is really tristate in Kconfig I don't know whether <linux/module.h>
should be included or not.

If it is a bool don't include this.
If it is a bool, don't use any MODULE_* macros either.

> +/* Challenges in the AST2400 Multifunction Pin Design
> + * --------------------------------------------------
> + *
> + * * Reasonable number of pins (216)
> + *
> + * * The SoC function enabled on a pin is determined on a priority basis
> + *
> + * * Using the priority system, pins can provide up to 3 different signals
> + *   types
> + *
> + * * The signal active on a pin is often described by compound logical
> + *   expressions involving multiple operators, registers and bits
> + *
> + *   * The high and low priority functions' bit masks are frequently not the
> + *     same
> + *     (i.e. cannot just flip a bit to change from high to low), or even in the
> + *     same register.
> + *
> + *   * Not all functions are disabled by unsetting bits, some require setting
> + *     bits
> + *
> + *   * Not all signals can be deactivated as some expressions depend on values
> + *     in the hardware strapping register, which is treated as read-only.

Try to compresse the bullet list to text. This looks like a powerpoint
presentation.

> + * SoC Multi-function Pin Expression Examples
> + * ------------------------------------------
> + *
> + * Here are some sample mux configurations from the datasheet to illustrate the
> + * corner cases, roughly in order of least to most corner. In the table, HP and
> + * LP are high- and low- priority respectively.

I believe that you got the hardware details right. Only someone with
experience with the same hardware could review these gory details.
So I will just trust that you got that part right.

> + * * Pins provide two to three signals in a priority order
> + *
> + * * For priorities levels defined on a pin, each priority provides one signal
> + *
> + * * Enabling lower priority signals requires higher priority signals be
> + *   disabled
(...)
> + * * A signal at a given priority on a given pin is active if any of the
> + *   functions in which the signal participates are active, and no higher
> + *   priority signal on the pin is active

That's a pretty baroque design. I wonder who came up with the idea
that this needed to be a priority-based state machine thing. Did they
have a usecase of different software threads independently competing
about using the hardware or what?

Or is it some hardware aid to help the programmer from shooting
him/herself in the foot?

> +#define A4 2
> +SSSF_PIN_DECL(A4, GPIOA2, TIMER3, FUNC_DESC_SET(SCU80, 2));
> +
> +FUNC_EXPR_DECL_SINGLE(SD1, FUNC_DESC_SET(SCU90, 0));
> +
> +FUNC_EXPR_DECL_SINGLE(I2C10, FUNC_DESC_SET(SCU90, 23));
> +
> +#define C4 16
> +MS_PIN_DECL(C4, GPIOC0, SD1, I2C10);
> +#define B3 17
> +MS_PIN_DECL(B3, GPIOC1, SD1, I2C10);

These things look very terse. But I guess there is no way to alleviate
the terseness that you can think of?

> +/* Note we account for GPIOY4-GPIOY7 even though they're not valid, thus 216
> + * pins becomes 220.
> + */
> +#define AST2400_NR_GPIOS 220

Rename AST2400_NR_PINS, they are not just GPIO's right?

> +static struct pinctrl_ops ast2400_pinctrl_ops = {
> +       .get_groups_count = ast2400_pinctrl_get_groups_count,
> +       .get_group_name = ast2400_pinctrl_get_group_name,
> +       .get_group_pins = ast2400_pinctrl_get_group_pins,
> +       .pin_dbg_show = ast2400_pinctrl_pin_dbg_show,
> +       .dt_node_to_map = pinconf_generic_dt_node_to_map_pin,
> +       .dt_free_map = pinctrl_utils_dt_free_map,

This has been renamed in the upstream kernel so rebase the patch
on v4.6-rc1 when it's out.

> +static bool func_expr_disable(const struct func_expr *expr, void __iomem *base)
> +static inline bool maybe_disable(const struct func_expr **expr,
> +               void __iomem *base)
> +static bool sig_in_exprs(const struct func_expr **exprs,
> +               const struct func_expr *sig)

Those functions are very hard to understand, maybe you could
kerneldoc them? Just a few line about what each does.

> +static int ast2400_gpio_request_enable(struct pinctrl_dev *pctldev,
> +               struct pinctrl_gpio_range *range,
> +               unsigned offset)
> +{
> +       unsigned pin = range->base + offset;
> +       const struct ast2400_pinctrl_data *pdata =
> +               pinctrl_dev_get_drvdata(pctldev);
> +       const struct pin_prio *pprio =
> +               pdata->pins[pin].drv_data;
> +
> +       if (!pprio)
> +               return 0;
> +
> +       if (!maybe_disable(pprio->high, pdata->reg_base))
> +               return -1;
> +
> +       if (!maybe_disable(pprio->low, pdata->reg_base))
> +               return -1;

No returning opaque -1, use -ENODEV or something sensible.

> +static struct pinctrl_gpio_range ast2400_gpios = {
> +       .name = "ast2400-pctrl-gpio-range",
> +       .npins = ARRAY_SIZE(ast2400_pins),
> +};

Nope.

> +       /* grange.name = "exynos5440-pctrl-gpio-range"; */
> +       pinctrl_add_gpio_range(pctl, &ast2400_gpios);

And nope.

A device tree driver should define the GPIO ranges in the
device tree using gpio-ranges = <...>. There is documentation and
examples of how to do this in
Documentation/devicetree/bindings/gpio/gpio.txt

Yours,
Linus Walleij

Re: [RFC PATCH] drivers/pinctrl: Add pinctrl-ast2400

[Andrew Jeffery]

[-- Attachment #1: Type: text/plain, Size: 8676 bytes --]

Hi Linus,

On Mon, 2016-05-23 at 14:38 +0200, Linus Walleij wrote:
> Hi sorry for taking so long before reviewing. Too busy, what can I say.

No worries, I expected as much. Thanks for taking the time!

> 
> On Fri, May 6, 2016 at 8:20 AM, Andrew Jeffery <andrew@aj.id.au> wrote:
> 
> > 
> > Add pinctrl/pinmux support for the Aspeed AST2400 SoC. Configuration of
> > the SoC is somewhat involved: Enabling functions can involve writes of
> > multiple bits to multiple registers, and signals provided by a pin are
> > determined on a priority basis. That is, without care, it's possible to
> > configure a function in the mux and to not gain expected functionality.
> > 
> > Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> > 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> You should include your changes to Kconfig and Makefile in the patch
> so it is a complete driver. This is an example of why: if I can't see if
> your driver is really tristate in Kconfig I don't know whether 
> should be included or not.
> 
> If it is a bool don't include this.
> If it is a bool, don't use any MODULE_* macros either.

Okay. I'll do so when I send follow-up patches.

> 
> > 
> > +/* Challenges in the AST2400 Multifunction Pin Design
> > + * --------------------------------------------------
> > + *
> > + * * Reasonable number of pins (216)
> > + *
> > + * * The SoC function enabled on a pin is determined on a priority basis
> > + *
> > + * * Using the priority system, pins can provide up to 3 different signals
> > + *   types
> > + *
> > + * * The signal active on a pin is often described by compound logical
> > + *   expressions involving multiple operators, registers and bits
> > + *
> > + *   * The high and low priority functions' bit masks are frequently not the
> > + *     same
> > + *     (i.e. cannot just flip a bit to change from high to low), or even in the
> > + *     same register.
> > + *
> > + *   * Not all functions are disabled by unsetting bits, some require setting
> > + *     bits
> > + *
> > + *   * Not all signals can be deactivated as some expressions depend on values
> > + *     in the hardware strapping register, which is treated as read-only.
> Try to compresse the bullet list to text. This looks like a powerpoint
> presentation.

Will do.

> 
> > 
> > + * SoC Multi-function Pin Expression Examples
> > + * ------------------------------------------
> > + *
> > + * Here are some sample mux configurations from the datasheet to illustrate the
> > + * corner cases, roughly in order of least to most corner. In the table, HP and
> > + * LP are high- and low- priority respectively.
> I believe that you got the hardware details right. Only someone with
> experience with the same hardware could review these gory details.
> So I will just trust that you got that part right.
> 
> > 
> > + * * Pins provide two to three signals in a priority order
> > + *
> > + * * For priorities levels defined on a pin, each priority provides one signal
> > + *
> > + * * Enabling lower priority signals requires higher priority signals be
> > + *   disabled
> (...)
> > 
> > + * * A signal at a given priority on a given pin is active if any of the
> > + *   functions in which the signal participates are active, and no higher
> > + *   priority signal on the pin is active
> That's a pretty baroque design. I wonder who came up with the idea
> that this needed to be a priority-based state machine thing. Did they
> have a usecase of different software threads independently competing
> about using the hardware or what?
> 
> Or is it some hardware aid to help the programmer from shooting
> him/herself in the foot?

The datasheet doesn't appear to explain the motivation, so we're in the
dark.

> 
> > 
> > +#define A4 2
> > +SSSF_PIN_DECL(A4, GPIOA2, TIMER3, FUNC_DESC_SET(SCU80, 2));
> > +
> > +FUNC_EXPR_DECL_SINGLE(SD1, FUNC_DESC_SET(SCU90, 0));
> > +
> > +FUNC_EXPR_DECL_SINGLE(I2C10, FUNC_DESC_SET(SCU90, 23));
> > +
> > +#define C4 16
> > +MS_PIN_DECL(C4, GPIOC0, SD1, I2C10);
> > +#define B3 17
> > +MS_PIN_DECL(B3, GPIOC1, SD1, I2C10);
> These things look very terse. But I guess there is no way to alleviate
> the terseness that you can think of?

Less terse in what way? Something other than more descriptive/longer
the macro names?

Since sending the patch I've been concentrating on other things, but
I've had some half-baked thoughts about alternative approaches. My
intent was a roughly iterative approach, and with this first patch
revision I was trying to understand the problem of the hardware's
function priorities and capturing a signal's bitfield relationships.
The motivation for sending the patch as it is (now that I have an
understanding of the problems that need solving) was to find out if
there were existing patterns that could be used to shape the
implementation. Or if not, whether the approach was reasonable.

However, that was at the expense of a deeper understanding of the
approaches to integrating a solution into the pinctrl subsystem. Now
that I have a mental model for representing the mux configuration I
plan to hash out those half-baked ideas to see if they give a more
obvious solution. Hopefully any results will be less terse. 

> 
> > 
> > +/* Note we account for GPIOY4-GPIOY7 even though they're not valid, thus 216
> > + * pins becomes 220.
> > + */
> > +#define AST2400_NR_GPIOS 220
> Rename AST2400_NR_PINS, they are not just GPIO's right?

Will do.

> 
> > 
> > +static struct pinctrl_ops ast2400_pinctrl_ops = {
> > +       .get_groups_count = ast2400_pinctrl_get_groups_count,
> > +       .get_group_name = ast2400_pinctrl_get_group_name,
> > +       .get_group_pins = ast2400_pinctrl_get_group_pins,
> > +       .pin_dbg_show = ast2400_pinctrl_pin_dbg_show,
> > +       .dt_node_to_map = pinconf_generic_dt_node_to_map_pin,
> > +       .dt_free_map = pinctrl_utils_dt_free_map,
> This has been renamed in the upstream kernel so rebase the patch
> on v4.6-rc1 when it's out.

Will do.

> 
> > 
> > +static bool func_expr_disable(const struct func_expr *expr, void __iomem *base)
> > +static inline bool maybe_disable(const struct func_expr **expr,
> > +               void __iomem *base)
> > +static bool sig_in_exprs(const struct func_expr **exprs,
> > +               const struct func_expr *sig)
> Those functions are very hard to understand, maybe you could
> kerneldoc them? Just a few line about what each does.

Will do. No doubt they'll evolve in further experiments, but I'll
document them regardless.

> 
> > 
> > +static int ast2400_gpio_request_enable(struct pinctrl_dev *pctldev,
> > +               struct pinctrl_gpio_range *range,
> > +               unsigned offset)
> > +{
> > +       unsigned pin = range->base + offset;
> > +       const struct ast2400_pinctrl_data *pdata =
> > +               pinctrl_dev_get_drvdata(pctldev);
> > +       const struct pin_prio *pprio =
> > +               pdata->pins[pin].drv_data;
> > +
> > +       if (!pprio)
> > +               return 0;
> > +
> > +       if (!maybe_disable(pprio->high, pdata->reg_base))
> > +               return -1;
> > +
> > +       if (!maybe_disable(pprio->low, pdata->reg_base))
> > +               return -1;
> No returning opaque -1, use -ENODEV or something sensible.

Right, that was a quick-and-inexperienced hack. Will fix.

> 
> > 
> > +static struct pinctrl_gpio_range ast2400_gpios = {
> > +       .name = "ast2400-pctrl-gpio-range",
> > +       .npins = ARRAY_SIZE(ast2400_pins),
> > +};
> Nope.
> 
> > 
> > +       /* grange.name = "exynos5440-pctrl-gpio-range"; */
> > +       pinctrl_add_gpio_range(pctl, &ast2400_gpios);
> And nope.
> 
> A device tree driver should define the GPIO ranges in the
> device tree using gpio-ranges = <...>. There is documentation and
> examples of how to do this in
> Documentation/devicetree/bindings/gpio/gpio.txt

Yes. Understanding various devicetree integration was part of my
follow-up plan, e.g. the registers we're touching are really part of
what should be represented as an mfd/syscon device, and so some
(separate) work needs to be done to integrate that.

Thanks again for taking the time to look at the patch.

Andrew

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [RFC PATCH] drivers/pinctrl: Add pinctrl-ast2400

[Linus Walleij]

On Wed, May 25, 2016 at 7:22 AM, Andrew Jeffery <andrew@aj.id.au> wrote:
> [Me]
>> > +#define A4 2
>> > +SSSF_PIN_DECL(A4, GPIOA2, TIMER3, FUNC_DESC_SET(SCU80, 2));
>> > +
>> > +FUNC_EXPR_DECL_SINGLE(SD1, FUNC_DESC_SET(SCU90, 0));
>> > +
>> > +FUNC_EXPR_DECL_SINGLE(I2C10, FUNC_DESC_SET(SCU90, 23));
>> > +
>> > +#define C4 16
>> > +MS_PIN_DECL(C4, GPIOC0, SD1, I2C10);
>> > +#define B3 17
>> > +MS_PIN_DECL(B3, GPIOC1, SD1, I2C10);
>>
>> These things look very terse. But I guess there is no way to alleviate
>> the terseness that you can think of?
>
> Less terse in what way? Something other than more descriptive/longer
> the macro names?

Well I was just thinking that it's hard to read and understand.

But if it's complicated, it's complicated and that cannot be fixed
by syntax. Algebra books are not and easy read either, hehe :)

Yours,
Linus Walleij