[U-Boot] [PATCH 3/6] arm64: mvebu: pinctrl: Add pin control driver for A8K family

Kostya Porotchkin kostap at marvell.com
Thu Nov 24 09:28:21 CET 2016


Hello, Simon,

Thank you very much for your review.
I am preparing a second patch release, which will include the requested fixes.
For the license I have to check with the company legal department first.
I am also going to fix some functions values in DTS files following internal review.

Regards
Konstantin
________________________________________
From: sjg at google.com <sjg at google.com> on behalf of Simon Glass <sjg at chromium.org>
Sent: Thursday, November 24, 2016 04:20
To: Kostya Porotchkin
Cc: U-Boot Mailing List; Stefan Roese; Nadav Haklai; Neta Zur Hershkovits; Omri Itach; Igal Liberman; Haim Boot; Hanna Hawa
Subject: Re: [PATCH 3/6] arm64: mvebu: pinctrl: Add pin control driver for A8K family

Hi,

On 20 November 2016 at 08:38,  <kostap at marvell.com> wrote:
> From: Konstantin Porotchkin <kostap at marvell.com>
>
> Add a port of Marvell pin control driver.
> The A8K SoC family contains several silicone dies interconnected
> in a single package. Every die is normally equuipped with its own
> pin controller unit.
> Since the UCLASS_PINCTRL device only calls the probe method for
> the first detected pin controller, a trick similar to used with
> comphy driver is required.
> In order to bring up all pin controllers available in A8K SoC,
> the arch_early_init_r() function sequentially calls the
> uclass_get_device() function for each UCLASS_PINCTRL device.
>
> Change-Id: Iff143827e8f1558a554d77173562c4b52ce179d7
> Signed-off-by: Konstantin Porotchkin <kostap at marvell.com>
> Cc: Simon Glass <sjg at chromium.org>
> Cc: Stefan Roese <sr at denx.de>
> Cc: Nadav Haklai <nadavh at marvell.com>
> Cc: Neta Zur Hershkovits <neta at marvell.com>
> Cc: Omri Itach <omrii at marvell.com>
> Cc: Igal Liberman <igall at marvell.com>
> Cc: Haim Boot <hayim at marvell.com>
> Cc: Hanna Hawa <hannah at marvell.com>
> ---
>  arch/arm/include/asm/arch-armada8k/soc-info.h      |  45 +++++
>  arch/arm/mach-mvebu/arm64-common.c                 |   1 -
>  .../pinctrl/marvell,mvebu-pinctrl.txt              | 113 ++++++++++++
>  drivers/pinctrl/Kconfig                            |   1 +
>  drivers/pinctrl/Makefile                           |   1 +
>  drivers/pinctrl/mvebu/Kconfig                      |   7 +
>  drivers/pinctrl/mvebu/Makefile                     |  17 ++
>  drivers/pinctrl/mvebu/pinctrl-mvebu.c              | 195 +++++++++++++++++++++
>  drivers/pinctrl/mvebu/pinctrl-mvebu.h              |  44 +++++
>  9 files changed, 423 insertions(+), 1 deletion(-)
>  create mode 100644 arch/arm/include/asm/arch-armada8k/soc-info.h
>  create mode 100644 doc/device-tree-bindings/pinctrl/marvell,mvebu-pinctrl.txt
>  create mode 100644 drivers/pinctrl/mvebu/Kconfig
>  create mode 100644 drivers/pinctrl/mvebu/Makefile
>  create mode 100644 drivers/pinctrl/mvebu/pinctrl-mvebu.c
>  create mode 100644 drivers/pinctrl/mvebu/pinctrl-mvebu.h
>

Generally looks good but I have a load of nits sorry.

> diff --git a/arch/arm/include/asm/arch-armada8k/soc-info.h b/arch/arm/include/asm/arch-armada8k/soc-info.h
> new file mode 100644
> index 0000000..4640deb
> --- /dev/null
> +++ b/arch/arm/include/asm/arch-armada8k/soc-info.h
> @@ -0,0 +1,45 @@
> +/*
> + * ***************************************************************************
> + * Copyright (C) 2015 Marvell International Ltd.
> + * ***************************************************************************
> + * 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, either version 2 of the License, or any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program.  If not, see <http://www.gnu.org/licenses/>.
> + * ***************************************************************************

Can you please use SPDX?

> + */
> +
> +#ifndef _SOC_INFO_H_
> +#define _SOC_INFO_H_
> +
> +/* General MPP definitions */
> +#define MAX_MPP_OPTS           7
> +#define MAX_MPP_ID             15
> +
> +#define MPP_BIT_CNT            4
> +#define MPP_FIELD_MASK         0x7
> +#define MPP_FIELD_BITS         3
> +#define MPP_VAL_MASK           0xF
> +
> +#define MPPS_PER_REG           (32 / MPP_BIT_CNT)
> +#define MAX_MPP_REGS           ((MAX_MPP_ID + MPPS_PER_REG) / MPPS_PER_REG)
> +
> +/* MPP pins and functions correcsponding to UART RX connections
> +   This information is used for detection of recovery boot mode (boot from UART) */

/*
 * MPP pins
 * ...
 * /

> +#define MPP_UART_RX_PINS       { 3, 5 }
> +#define MPP_UART_RX_FUNCTIONS  { 1, 2 }
> +
> +/* Pin Ctrl driver definitions */
> +#define BITS_PER_PIN           4
> +#define PIN_FUNC_MASK          ((1 << BITS_PER_PIN) - 1)
> +#define PIN_REG_SHIFT          3
> +#define PIN_FIELD_MASK         ((1 << PIN_REG_SHIFT) - 1)
> +
> +#endif /* _SOC_INFO_H_ */
> diff --git a/arch/arm/mach-mvebu/arm64-common.c b/arch/arm/mach-mvebu/arm64-common.c
> index 1fc2ff2..78fe7a7 100644
> --- a/arch/arm/mach-mvebu/arm64-common.c
> +++ b/arch/arm/mach-mvebu/arm64-common.c
> @@ -124,7 +124,6 @@ int arch_early_init_r(void)
>                 if (ret)
>                         break;
>         }
> -

Unrelated change?

>         /* Cause the SATA device to do its early init */
>         uclass_first_device(UCLASS_AHCI, &dev);
>
> diff --git a/doc/device-tree-bindings/pinctrl/marvell,mvebu-pinctrl.txt b/doc/device-tree-bindings/pinctrl/marvell,mvebu-pinctrl.txt
> new file mode 100644
> index 0000000..0973db8
> --- /dev/null
> +++ b/doc/device-tree-bindings/pinctrl/marvell,mvebu-pinctrl.txt
> @@ -0,0 +1,113 @@
> +The pinctrl driver enables Marvell Armada 8K SoCs to configure the multi-purpose
> +pins (mpp) to a specific function.
> +A Marvell SoC pin configuration node is a node of a group of pins which can
> +be used for a specific device or function. Each node requires one or more
> +mpp pins or group of pins and a mpp function common to all pins.
> +
> +Required properties for the pinctrl driver:
> +- compatible:  "marvell,mvebu-pinctrl",
> +               "marvell,armada-ap806-pinctrl",
> +               "marvell,a70x0-pinctrl",
> +               "marvell,a80x0-cp0-pinctrl",
> +               "marvell,a80x0-cp1-pinctrl"
> +- bank-name:   A string defining the pinc controller bank name
> +- reg:                 A pair of values defining the pin controller base address
> +               and the address space
> +- pin-count:   Numeric value defining the amount of multi purpose pins
> +               included in this bank
> +- max-func:    Numeric value defining the maximum function value for
> +               pins in this bank
> +- pin-func:    Array of pin function values for every pin in the bank.
> +               When the function value for a specific pin equal 0xFF,
> +               the pin configuration is skipped and a default function
> +               value is used for this pin.
> +
> +The A8K is a hybrid SoC that contains several silicon dies interconnected in
> +a single package. Each such die may have a separate pin controller.
> +
> +Example:
> +/ {
> +       ap806 {
> +               config-space {
> +                       pinctl: pinctl at 6F4000 {
> +                               compatible = "marvell,mvebu-pinctrl",
> +                                            "marvell,armada-ap806-pinctrl";
> +                               bank-name ="apn-806";
> +                               reg = <0x6F4000 0x10>;
> +                               pin-count = <20>;
> +                               max-func = <3>;
> +                               /* MPP Bus:
> +                                       SPI0 [0-3]
> +                                       I2C0 [4-5]
> +                                       UART0 [11,19]
> +                               */
> +                                         /* 0 1 2 3 4 5 6 7 8 9 */
> +                               pin-func = < 3 3 3 3 3 3 0 0 0 0
> +                                            0 3 0 0 0 0 0 0 0 3>;
> +                       };
> +               };
> +       };
> +
> +       cp110-master {
> +               config-space {
> +                       cpm_pinctl: pinctl at 44000 {
> +                               compatible = "marvell,mvebu-pinctrl",
> +                                            "marvell,a70x0-pinctrl",
> +                                            "marvell,a80x0-cp0-pinctrl";
> +                               bank-name ="cp0-110";
> +                               reg = <0x440000 0x20>;
> +                               pin-count = <63>;
> +                               max-func = <0xf>;
> +                               /* MPP Bus:
> +                                  [0-31] = 0xff: Keep default CP0_shared_pins:
> +                                  [11] CLKOUT_MPP_11 (out)
> +                                  [23] LINK_RD_IN_CP2CP (in)
> +                                  [25] CLKOUT_MPP_25 (out)
> +                                  [29] AVS_FB_IN_CP2CP (in)
> +                                  [32,34] SMI
> +                                  [31]    GPIO: push button/Wake
> +                                  [35-36] GPIO
> +                                  [37-38] I2C
> +                                  [40-41] SATA[0/1]_PRESENT_ACTIVEn
> +                                  [42-43] XSMI
> +                                  [44-55] RGMII1
> +                                  [56-62] SD
> +                               */
> +                                       /*   0    1    2    3    4    5    6    7    8    9 */
> +                               pin-func = < 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff
> +                                            0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff
> +                                            0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff
> +                                            0xff 0    7    0    7    0    0    2    2    0
> +                                            0    0    8    8    1    1    1    1    1    1
> +                                            1    1    1    1    1    1    0xE  0xE  0xE  0xE
> +                                            0xE  0xE  0xE>;
> +                       };
> +               };
> +       };
> +
> +       cp110-slave {
> +               config-space {
> +                       cps_pinctl: pinctl at 44000 {
> +                               compatible = "marvell,mvebu-pinctrl",
> +                                            "marvell,a80x0-cp1-pinctrl";
> +                               bank-name ="cp1-110";
> +                               reg = <0x440000 0x20>;
> +                               pin-count = <63>;
> +                               max-func = <0xf>;
> +                               /* MPP Bus:
> +                                  [0-11]  RGMII0
> +                                  [27,31] GE_MDIO/MDC
> +                                  [32-62] = 0xff: Keep default CP1_shared_pins:
> +                               */
> +                                       /*   0    1    2    3    4    5    6    7    8    9 */
> +                               pin-func = < 0x3  0x3  0x3  0x3  0x3  0x3  0x3  0x3  0x3  0x3
> +                                            0x3  0x3  0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff
> +                                            0xff 0xff 0xff 0xff 0xff 0xff 0xff 0x8  0xff 0xff
> +                                            0xff 0x8  0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff
> +                                            0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff
> +                                            0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff
> +                                            0xff 0xff 0xff>;
> +                       };
> +               };
> +       };
> +}
> diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
> index 12be3cf..efcb4c0 100644
> --- a/drivers/pinctrl/Kconfig
> +++ b/drivers/pinctrl/Kconfig
> @@ -181,5 +181,6 @@ source "drivers/pinctrl/meson/Kconfig"
>  source "drivers/pinctrl/nxp/Kconfig"
>  source "drivers/pinctrl/uniphier/Kconfig"
>  source "drivers/pinctrl/exynos/Kconfig"
> +source "drivers/pinctrl/mvebu/Kconfig"
>
>  endmenu
> diff --git a/drivers/pinctrl/Makefile b/drivers/pinctrl/Makefile
> index f28b5c1..512112a 100644
> --- a/drivers/pinctrl/Makefile
> +++ b/drivers/pinctrl/Makefile
> @@ -15,3 +15,4 @@ obj-$(CONFIG_PINCTRL_UNIPHIER)        += uniphier/
>  obj-$(CONFIG_PIC32_PINCTRL)    += pinctrl_pic32.o
>  obj-$(CONFIG_PINCTRL_EXYNOS)   += exynos/
>  obj-$(CONFIG_PINCTRL_MESON)    += meson/
> +obj-$(CONFIG_PINCTRL_MVEBU)    += mvebu/
> diff --git a/drivers/pinctrl/mvebu/Kconfig b/drivers/pinctrl/mvebu/Kconfig
> new file mode 100644
> index 0000000..cf9c299
> --- /dev/null
> +++ b/drivers/pinctrl/mvebu/Kconfig
> @@ -0,0 +1,7 @@
> +config PINCTRL_MVEBU
> +       depends on ARCH_MVEBU
> +       bool
> +       default y
> +       help
> +          Support pin multiplexing and pin configuration control on
> +          Marvell's Armada-8K SoC.
> diff --git a/drivers/pinctrl/mvebu/Makefile b/drivers/pinctrl/mvebu/Makefile
> new file mode 100644
> index 0000000..7db2b97
> --- /dev/null
> +++ b/drivers/pinctrl/mvebu/Makefile
> @@ -0,0 +1,17 @@
> +#* ***************************************************************************
> +#* Copyright (C) 2016 Marvell International Ltd.
> +#* ***************************************************************************
> +#* 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, either version 2 of the License, or any later version.
> +#*
> +#* This program is distributed in the hope that it will be useful,
> +#* but WITHOUT ANY WARRANTY; without even the implied warranty of
> +#* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +#* GNU General Public License for more details.
> +#*
> +#* You should have received a copy of the GNU General Public License
> +#* along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +#* ***************************************************************************
> +
> +obj-$(CONFIG_PINCTRL_MVEBU)    += pinctrl-mvebu.o
> diff --git a/drivers/pinctrl/mvebu/pinctrl-mvebu.c b/drivers/pinctrl/mvebu/pinctrl-mvebu.c
> new file mode 100644
> index 0000000..02fcd2f
> --- /dev/null
> +++ b/drivers/pinctrl/mvebu/pinctrl-mvebu.c
> @@ -0,0 +1,195 @@
> +/*
> + * ***************************************************************************
> + * Copyright (C) 2016 Marvell International Ltd.
> + * ***************************************************************************
> + * 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, either version 2 of the License, or any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program.  If not, see <http://www.gnu.org/licenses/>.
> + * ***************************************************************************
> + */
> +
> +#include <config.h>
> +#include <common.h>
> +#include <dm.h>
> +#include <asm/system.h>
> +#include <asm/io.h>
> +#include <dm/pinctrl.h>
> +#include <dm/root.h>
> +#include <fdtdec.h>
> +#include <errno.h>
> +#include <asm/arch/fdt.h>
> +#include <asm/arch-armada8k/soc-info.h>
> +#include "pinctrl-mvebu.h"

Please check include-file ordering here:

http://www.denx.de/wiki/U-Boot/CodingStyle

It is close.

> +
> +/*
> + * mvebu_pinctrl_set_state: configure pin functions.
> + * dev: the pinctrl device to be configured.
> + * config: the state to be configured.

/*
 * mvebu_pinctrl_set_state() - configure pin functions
 * @dev: the pinctrl device to be configured.
 * @config: the state to be configured.
 * @return ...
 */

Please use that consistently.

> + */
> +int mvebu_pinctrl_set_state(struct udevice *dev, struct udevice *config)
> +{
> +       const void *blob = gd->fdt_blob;
> +       int node = config->of_offset;
> +       struct mvebu_pinctrl_priv *priv;
> +       u32 pin_arr[CONFIG_MAX_PINS_PER_BANK];
> +       u32 function;
> +       int i, pin_count;
> +
> +       priv = dev_get_priv(dev);
> +       if (!priv) {
> +               printf("%s: Failed to get private\n", __func__);

debug()

> +               return -EINVAL;
> +       }
> +
> +       pin_count = fdtdec_get_int_array_count(blob, node, "marvell,pins", pin_arr, CONFIG_MAX_PINS_PER_BANK);

Are you sure this passes checkpatch? That line seems to long.

> +       if (pin_count <= 0) {
> +               error("Failed reading pins array for pinconfig %s (%d)\n", config->name, pin_count);

debug() to avoid bloating the code? (globally)

> +               return -EINVAL;
> +       }
> +
> +       function = fdtdec_get_int(blob, node, "marvell,function", 0xFF);

nit: Lower case hex throughout

> +
> +       for (i = 0; i < pin_count; i++) {
> +               int reg_offset;
> +               int field_offset;
> +               u32 reg, mask;
> +               int pin = pin_arr[i];
> +
> +               if (function > priv->max_func) {
> +                       error("Illegal function %d for pinconfig %s\n", function, config->name);
> +                       return -EINVAL;
> +               }
> +
> +               /* Calculate register address and bit in register */
> +               reg_offset   = priv->reg_direction * 4 * (pin >> (PIN_REG_SHIFT));
> +               field_offset = (BITS_PER_PIN) * (pin & PIN_FIELD_MASK);
> +               mask = ~(PIN_FUNC_MASK << field_offset);
> +
> +               /* Clip value to field resolution */
> +               function &= PIN_FUNC_MASK;
> +
> +               reg = readl(priv->base_reg + reg_offset);
> +               reg = (reg & mask) | (function << field_offset);
> +               writel(reg, priv->base_reg + reg_offset);

You can use clrsetbits_le32()

> +       }
> +
> +       return 0;
> +}
> +
> +/*
> + * mvebu_pinctrl_set_state_all: configure the entire bank pin functions.
> + * dev: the pinctrl device to be configured.
> + * config: the state to be configured.
> + */
> +static int mvebu_pinctrl_set_state_all(struct udevice *dev, struct udevice *config)
> +{
> +       const void *blob = gd->fdt_blob;
> +       int node = config->of_offset;
> +       struct mvebu_pinctrl_priv *priv;
> +       u32 func_arr[CONFIG_MAX_PINS_PER_BANK];
> +       int pin, err;
> +
> +       priv = dev_get_priv(dev);
> +       if (!priv) {
> +               printf("%s: Failed to get private\n", __func__);

This cannot happen if the device is probed. Add an assert() if you are
paranoid, but it has not benefit.

> +               return -EINVAL;
> +       }
> +
> +       err = fdtdec_get_int_array(blob, node, "pin-func", func_arr, priv->pin_cnt);
> +       if (err) {
> +               error("Failed reading pin functions for bank %s\n", priv->bank_name);
> +               return -EINVAL;
> +       }
> +
> +       for (pin = 0; pin < priv->pin_cnt; pin++) {
> +               int reg_offset;
> +               int field_offset;
> +               u32 reg, mask;
> +               u32 func = func_arr[pin];
> +
> +               /* Bypass pins with function 0xFF */
> +               if (func == 0xFF) {
> +                       debug("Warning: pin %d value is not modified (kept as default\n", pin);
> +                       continue;
> +               } else if (func > priv->max_func) {
> +                       error("Illegal function %d for pin %d\n", func, pin);
> +                       return -EINVAL;
> +               }
> +
> +               /* Calculate register address and bit in register */
> +               reg_offset   = priv->reg_direction * 4 * (pin >> (PIN_REG_SHIFT));
> +               field_offset = (BITS_PER_PIN) * (pin & PIN_FIELD_MASK);
> +               mask = ~(PIN_FUNC_MASK << field_offset);
> +
> +               /* Clip value to field resolution */
> +               func &= PIN_FUNC_MASK;
> +
> +               reg = readl(priv->base_reg + reg_offset);
> +               reg = (reg & mask) | (func << field_offset);
> +               writel(reg, priv->base_reg + reg_offset);

clrsetbits_le32()

> +       }
> +
> +       return 0;
> +}
> +
> +int mvebu_pinctl_probe(struct udevice *dev)
> +{
> +       const void *blob = gd->fdt_blob;
> +       int node = dev->of_offset;
> +       struct mvebu_pinctrl_priv *priv;
> +       fdt_addr_t base;
> +
> +       priv = dev_get_priv(dev);
> +       if (!priv) {
> +               printf("%s: Failed to get private\n", __func__);

debug() - please fix globally - drivers should not print messages.

> +               return -EINVAL;
> +       }
> +
> +       base = dev_get_addr(dev);
> +       if (base == FDT_ADDR_T_NONE) {
> +               printf("%s: Failed to get base address\n", __func__);
> +               return -EINVAL;
> +       }
> +
> +       priv->base_reg  = (u8 *)base;

Or use dev_get_addr_ptr() ?

> +       priv->pin_cnt   = fdtdec_get_int(blob, node, "pin-count", CONFIG_MAX_PINS_PER_BANK);
> +       priv->max_func  = fdtdec_get_int(blob, node, "max-func", CONFIG_MAX_FUNC);
> +       priv->bank_name = fdt_getprop(blob, node, "bank-name", NULL);
> +
> +       priv->reg_direction = 1;
> +       if (fdtdec_get_bool(blob, node, "reverse-reg"))
> +               priv->reg_direction = -1;
> +
> +       return mvebu_pinctrl_set_state_all(dev, dev);
> +}
> +
> +
> +static struct pinctrl_ops mvebu_pinctrl_ops = {
> +       .set_state      = mvebu_pinctrl_set_state
> +};
> +
> +static const struct udevice_id mvebu_pinctrl_ids[] = {
> +       { .compatible = "marvell,mvebu-pinctrl" },
> +       { .compatible = "marvell,armada-ap806-pinctrl" },
> +       { .compatible = "marvell,a70x0-pinctrl" },
> +       { .compatible = "marvell,a80x0-cp0-pinctrl" },
> +       { .compatible = "marvell,a80x0-cp1-pinctrl" },
> +       { }
> +};
> +
> +U_BOOT_DRIVER(pinctrl_mvebu) = {
> +       .name           = "mvebu_pinctrl",
> +       .id             = UCLASS_PINCTRL,
> +       .of_match       = mvebu_pinctrl_ids,
> +       .priv_auto_alloc_size = sizeof(struct mvebu_pinctrl_priv),
> +       .ops            = &mvebu_pinctrl_ops,
> +       .probe          = mvebu_pinctl_probe
> +};
> diff --git a/drivers/pinctrl/mvebu/pinctrl-mvebu.h b/drivers/pinctrl/mvebu/pinctrl-mvebu.h
> new file mode 100644
> index 0000000..61c84ce
> --- /dev/null
> +++ b/drivers/pinctrl/mvebu/pinctrl-mvebu.h
> @@ -0,0 +1,44 @@
> +/*
> + * ***************************************************************************
> + * Copyright (C) 2016 Marvell International Ltd.
> + * ***************************************************************************
> + * 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, either version 2 of the License, or any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program.  If not, see <http://www.gnu.org/licenses/>.
> + * ***************************************************************************
> + */
> +
> + #ifndef __PINCTRL_MVEBU_H_
> + #define __PINCTRL_MVEBU_H_
> +
> + #define CONFIG_MAX_PINCTL_BANKS       4
> + #define CONFIG_MAX_PINS_PER_BANK      100
> + #define CONFIG_MAX_FUNC               0xF

Avoid using CONFIG_ prefixes since this looks like a new global CONFIG
option. Just dropping the CONFIG_ will do.

> +
> +DECLARE_GLOBAL_DATA_PTR;

Please put that in the C file that needs it.

> +
> +/*
> + * struct mvebu_pin_bank_data: mvebu-pinctrl bank data

* struct mvebu_pin_bank_data - mvebu-pinctrl bank data


> + * @base_reg: controller base address for this bank
> + * @pin_cnt:  number of ping included in this bank

ping?

> + * @max_func: maximum configurable function value for pins in this bank
> + * @reg_direction:

?

> + * @bank_name: the pins bank name

pin's

> + */
> +struct mvebu_pinctrl_priv {
> +       u8              *base_reg;
> +       u32             pin_cnt;
> +       u32             max_func;

Why are these u32? Can they be uint?

> +       int             reg_direction;
> +       const char      *bank_name;
> +};
> +
> +#endif /* __PINCTRL_MVEBU_H_ */
> --
> 2.7.4
>

Regards,
Simon


More information about the U-Boot mailing list