On 1/9/2015 3:03 AM, Linus Walleij wrote:

On Fri, Nov 28, 2014 at 12:46 AM, Ray Jui [off-list ref] wrote:

quoted

This adds the initial driver support for the Broadcom Cygnus pinctrl
controller. The Cygnus pinctrl controller supports group based
alternate function configuration

Signed-off-by: Ray Jui <rjui@broadcom.com>
Reviewed-by: Scott Branden <sbranden@broadcom.com>
Signed-off-by: Fengguang Wu <redacted>
---
 drivers/pinctrl/Kconfig              |    7 +
 drivers/pinctrl/Makefile             |    1 +
 drivers/pinctrl/pinctrl-bcm-cygnus.c |  753 ++++++++++++++++++++++++++++++++++

With the proliferation of Broadcom drivers, please first send a
patch moving pinctrl-bcm281xx.c and pinctrl-bcm2835.c to
drivers/pinctrl/broadcom or something, so we can collect them
there.

Okay. This change will be included as the first patch in the next patch set.

I don't know if the hardware has any similarity though, so invite
the authors of the previous drivers to review this code.

They are completely different. The only similarity between Cygnus and
bcm281xx pinctrl is that they use the same concept of alternation
functions (1, 2, 3, 4) for mux configuration.

quoted

+config PINCTRL_BCM_CYGNUS
+       bool "Broadcom Cygnus pinctrl driver"
+       depends on (ARCH_BCM_CYGNUS || COMPILE_TEST)
+       select PINMUX
+       select PINCONF
+       select GENERIC_PINCONF

Nice that you use GENERIC_PINCONF! :)

quoted

+/*
+ * Cygnus pinctrl core
+ *
+ * @pctl: pointer to pinctrl_dev
+ * @dev: pointer to the device
+ * @base: I/O register base for Cygnus pinctrl configuration
+ *
+ */
+struct cygnus_pinctrl {
+       struct pinctrl_dev *pctl;
+       struct device *dev;
+       void __iomem *base;
+
+       const struct pinctrl_pin_desc *pins;
+       unsigned num_pins;

Why is this not simply just a part of struct pinctrl_desc?
Why does it have to be multiplied here?

Okay. Let me look into this.

quoted

+/*
+ * List of groups of pins
+ */
+static const unsigned gpio0_pins[] = { 12 };
+static const unsigned gpio1_pins[] = { 13 };
+static const unsigned gpio2_pins[] = { 14 };
+static const unsigned gpio3_pins[] = { 15 };
+static const unsigned gpio4_pins[] = { 16 };
+static const unsigned gpio5_pins[] = { 17 };
+static const unsigned gpio6_pins[] = { 18 };
+static const unsigned gpio7_pins[] = { 19 };
+static const unsigned gpio8_pins[] = { 20 };
+static const unsigned gpio9_pins[] = { 21 };
+static const unsigned gpio10_pins[] = { 22 };
+static const unsigned gpio11_pins[] = { 23 };
+static const unsigned gpio12_pins[] = { 24 };
+static const unsigned gpio13_pins[] = { 25 };
+static const unsigned gpio14_pins[] = { 26 };
+static const unsigned gpio15_pins[] = { 27 };
+static const unsigned gpio16_pins[] = { 28 };
+static const unsigned gpio17_pins[] = { 29 };
+static const unsigned gpio18_pins[] = { 30 };
+static const unsigned gpio19_pins[] = { 31 };
+static const unsigned gpio20_pins[] = { 32 };
+static const unsigned gpio21_pins[] = { 33 };
+static const unsigned gpio22_pins[] = { 34 };
+static const unsigned gpio23_pins[] = { 35 };

Have you considered implementing .gpio_request_enable()
and .gpio_disable_free() to get around having to have one
group for each GPIO line?

Okay the Cygnus pin controller is really a mess. GPIO 0 ~ GPIO23 are
really 23 distinct groups, each with one pin. Then the rest of GPIOs go
under other groups. In general, when we set a group to alternate
function 4, all pins become GPIO.

quoted

+static const unsigned pwm0_pins[] = { 38 };
+static const unsigned pwm1_pins[] = { 39 };
+static const unsigned pwm2_pins[] = { 40 };
+static const unsigned pwm3_pins[] = { 41 };
+static const unsigned sdio0_pins[] = { 94, 95, 96, 97, 98, 99 };
+static const unsigned smart_card0_pins[] = { 42, 43, 44, 46, 47 };
+static const unsigned smart_card1_pins[] = { 48, 49, 50, 52, 53 };
+static const unsigned spi0_pins[] = { 54, 55, 56, 57 };
+static const unsigned spi1_pins[] = { 58, 59, 60, 61 };
+static const unsigned spi2_pins[] = { 62, 63, 64, 65 };
+static const unsigned spi3_pins[] = { 66, 67, 68, 69 };
+static const unsigned d1w_pins[] = { 10, 11 };
+static const unsigned lcd_pins[] = { 126, 127, 128, 129, 130, 131, 132,        133,
+       134, 135, 136, 137, 138, 139, 140, 141, 142, 143, 144, 145, 146, 147,
+       148, 149, 150, 151, 152, 153, 154, 155 };
+static const unsigned uart0_pins[] = { 70, 71, 72, 73 };
+static const unsigned uart1_dte_pins[] = { 75, 76, 77, 78 };
+static const unsigned uart1_pins[] = { 74, 79, 80, 81 };
+static const unsigned uart3_pins[] = { 82, 83 };
+static const unsigned qspi_pins[] = { 104, 105, 106, 107 };
+static const unsigned nand_pins[] = { 110, 111, 112, 113, 114, 115, 116, 117,
+       118, 119, 120, 121, 122, 123, 124, 125 };
+static const unsigned sdio0_cd_pins[] = { 103 };
+static const unsigned sdio0_mmc_pins[] = { 100, 101, 102 };
+static const unsigned can0_spi4_pins[] = { 86, 87 };
+static const unsigned can1_spi4_pins[] = { 88, 89 };
+static const unsigned sdio1_cd_pins[] = { 93 };
+static const unsigned sdio1_led_pins[] = { 84, 85 };
+static const unsigned sdio1_mmc_pins[] = { 90, 91, 92 };
+static const unsigned camera_led_pins[] = { 156, 157, 158, 159, 160 };
+static const unsigned camera_rgmii_pins[] = { 169, 170, 171, 169, 170, 171,
+       172, 173 };
+static const unsigned camera_sram_rgmii_pins[] = { 161, 162, 163, 164, 165,
+       166, 167, 168 };
+static const unsigned qspi_gpio_pins[] = { 108, 109 };
+static const unsigned smart_card0_fcb_pins[] = { 45 };
+static const unsigned smart_card1_fcb_pins[] = { 51 };
+static const unsigned gpio0_3p3_pins[] = { 176 };
+static const unsigned gpio1_3p3_pins[] = { 177 };
+static const unsigned gpio2_3p3_pins[] = { 178 };

Looks good...

Note these pins are definitions in the driver that help to describe the
pad layout. We can't really configure any individual pins in Cygnus.

quoted

+/*
+ * List of groups names. Need to match the order in cygnus_pin_groups
+ */
+static const char * const cygnus_pin_group_names[] = {
+       "gpio0",
+       "gpio1",
+       "gpio2",
+       "gpio3",
+       "gpio4",
+       "gpio5",
+       "gpio6",
+       "gpio7",
+       "gpio8",
+       "gpio9",
+       "gpio10",
+       "gpio11",
+       "gpio12",
+       "gpio13",
+       "gpio14",
+       "gpio15",
+       "gpio16",
+       "gpio17",
+       "gpio18",
+       "gpio19",
+       "gpio20",
+       "gpio21",
+       "gpio22",
+       "gpio23",
+       "pwm0",
+       "pwm1",
+       "pwm2",
+       "pwm3",
+       "sdio0",
+       "smart_card0",
+       "smart_card1",
+       "spi0",
+       "spi1",
+       "spi2",
+       "spi3",
+       "d1w",
+       "lcd",
+       "uart0",
+       "uart1_dte",
+       "uart1",
+       "uart3",
+       "qspi",
+       "nand",
+       "sdio0_cd",
+       "sdio0_mmc",
+       "can0_spi4",
+       "can1_spi4",
+       "sdio1_cd",
+       "sdio1_led",
+       "sdio1_mmc",
+       "camera_led",
+       "camera_rgmii",
+       "camera_sram_rgmii",
+       "qspi_gpio",
+       "smart_card0_fcb",
+       "smart_card1_fcb",
+       "gpio0_3p3",
+       "gpio1_3p3",
+       "gpio2_3p3",
+};

This looks very much like function names as noted in the binding.
I would say, suffix every group with _grp or something so it's not
as confusing. Remember, spi0 is a function of the SoC,
pins {1,2} is just a group of pins that it may appear on.

Yes, suffix every group with _grp helps a lot to clarify the confusion.
Will fix this.

quoted

+#define CYGNUS_PIN_FUNCTION(fcn_name, mux_val)                 \
+{                                                              \
+       .name = #fcn_name,                                      \
+       .group_names = cygnus_pin_group_names,                  \
+       .num_groups = ARRAY_SIZE(cygnus_pin_group_names),       \
+       .mux = mux_val,                                         \
+}
+
+/*
+ * Cygnus has 4 alternate functions. All groups can be configured to any of
+ * the 4 alternate functions
+ */
+static const struct cygnus_pin_function cygnus_pin_functions[] = {
+       CYGNUS_PIN_FUNCTION(alt1, 0),
+       CYGNUS_PIN_FUNCTION(alt2, 1),
+       CYGNUS_PIN_FUNCTION(alt3, 2),
+       CYGNUS_PIN_FUNCTION(alt4, 3),
+};

These are not functions. These are per-pin mux ways.

Re-read the documentation of what a function is: it is not something
abstract like "alternative something" but something very direct like
uart0 or spi0.

Yes, agree. Will fix.

quoted

+static int cygnus_dt_node_to_map(struct pinctrl_dev *pctrl_dev,
+               struct device_node *np, struct pinctrl_map **map,
+               unsigned *num_maps)
+{

After S?ren Brinkmanns patches youy should be able to use core
functions for this and avoid this code altogether.

Will that help to take care our case, based on the way we will use
"function" and "group"?

quoted

+       num_groups = of_property_count_strings(np, "brcm,groups");

As mentioned, just "groups".

I guess I will use "group"?

quoted

+       ret = of_property_read_string(np, "brcm,function", &function_name);

As mentioned, just "function".

Yes.

quoted

+       ret = pinctrl_utils_reserve_map(pctrl_dev, map, &reserved_maps,
+                       num_maps, num_groups);

Good use of utilities!

Apart from this things look nice.

The main comment to address is the definition of functions.

Yours,
Linus Walleij

Thanks a lot for the review!!!