Re: [PATCH 2/4] pinctrl: cygnus: add initial pinctrl support

From: Linus Walleij
Date: Fri Jan 09 2015 - 06:03:47 EST

Next message: Michal Hocko: "[PATCH -v3 0/5] OOM vs PM freezer fixes"
Previous message: Jiri Slaby: "[PATCH 3.12 20/78] i2c: omap: fix NACK and Arbitration Lost irq handling"
Next in thread: Ray Jui: "Re: [PATCH 2/4] pinctrl: cygnus: add initial pinctrl support"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Fri, Nov 28, 2014 at 12:46 AM, Ray Jui <rjui@xxxxxxxxxxxx> wrote:

> 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@xxxxxxxxxxxx>
> Reviewed-by: Scott Branden <sbranden@xxxxxxxxxxxx>
> Signed-off-by: Fengguang Wu <fengguang.wu@xxxxxxxxx>
> ---
> 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.

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

> +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! :)

> +/*
> + * 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?

> +/*
> + * 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?

> +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...

> +/*
> + * 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.

> +#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.

> +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.

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

As mentioned, just "groups".

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

As mentioned, just "function".

> + 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Next message: Michal Hocko: "[PATCH -v3 0/5] OOM vs PM freezer fixes"
Previous message: Jiri Slaby: "[PATCH 3.12 20/78] i2c: omap: fix NACK and Arbitration Lost irq handling"
Next in thread: Ray Jui: "Re: [PATCH 2/4] pinctrl: cygnus: add initial pinctrl support"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]