[U-Boot] [PATCH v4 2/7] gpio: Add Multi-Function-Pin configuration driver for Marvell SoCs

[Prafulla Wadaskar]

> -----Original Message-----
> From: Lei Wen [mailto:adrian.wenl at gmail.com]
> Sent: Tuesday, December 07, 2010 8:53 PM
> To: Prafulla Wadaskar
> Cc: u-boot at lists.denx.de; Eric Miao; Manas Saksena; Lei Wen; Yu Tang;
> Ashish Karkare; Kiran Vedere; Prabhanjan Sarnaik
> Subject: Re: [U-Boot] [PATCH v4 2/7] gpio: Add Multi-Function-Pin
> configuration driver for Marvell SoCs
> 
> Hi Prafulla,
> 
> On Wed, Dec 8, 2010 at 1:06 AM, Prafulla Wadaskar <prafulla at marvell.com>
> wrote:
> > Most of the Marvell SoCs has Multi Function Pin (MFP) configuration
> registers
> > For ex. ARMADA100.
> >
> > These registers are programmed to expose the specific functionality
> > associated with respective SoC Pins
> >
> > This driver provides configuration APIs,
> > using them, configuration need to be done in board specific code
> >
> > for ex- following code configures MFPs 107 and 108 for UART_TX/RX
> functionality
> >
> > int board_early_init_f(void)
> > {
> >        u32 mfp_cfg[] = {
> >                /* Console on UART1 */
> >                MFP107_UART1_RXD,
> >                MFP108_UART1_TXD,
> >                MFP_EOC         /*End of configureation*/
> >        };
> >        /* configure MFP's */
> >        mfp_config(mfp_cfg);
> >        return 0;
> > }
> >
> > Signed-off-by: Prafulla Wadaskar <prafulla at marvell.com>
> > ---
> > Changelog v4:
> > 1. Driver renamed as mvmfp
> > 2. Re-architected mvmfp driver as per review feedback
> >
> >  drivers/gpio/Makefile |    1 +
> >  drivers/gpio/mvmfp.c  |   90
> ++++++++++++++++++++++++++++++++++++++++++++
> >  include/mvmfp.h       |  100
> +++++++++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 191 insertions(+), 0 deletions(-)
> >  create mode 100644 drivers/gpio/mvmfp.c
> >  create mode 100644 include/mvmfp.h
> >
> > diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
> > index 398024c..a5fa2b5 100644
> > --- a/drivers/gpio/Makefile
> > +++ b/drivers/gpio/Makefile
> > @@ -27,6 +27,7 @@ LIB   := $(obj)libgpio.o
> >
> >  COBJS-$(CONFIG_AT91_GPIO)      += at91_gpio.o
> >  COBJS-$(CONFIG_KIRKWOOD_GPIO)  += kw_gpio.o
> > +COBJS-$(CONFIG_MARVELL_MFP)    += mvmfp.o
> >  COBJS-$(CONFIG_MXC_GPIO)       += mxc_gpio.o
> >  COBJS-$(CONFIG_PCA953X)                += pca953x.o
> >  COBJS-$(CONFIG_S5P)            += s5p_gpio.o
> > diff --git a/drivers/gpio/mvmfp.c b/drivers/gpio/mvmfp.c
> > new file mode 100644
> > index 0000000..3472278
> > --- /dev/null
> > +++ b/drivers/gpio/mvmfp.c
> > @@ -0,0 +1,90 @@
> > +/*
> > + * (C) Copyright 2010
> > + * Marvell Semiconductor <www.marvell.com>
> > + * Written-by: Prafulla Wadaskar <prafulla at marvell.com>,
> > + *
> > + * See file CREDITS for list of people who contributed to this
> > + * project.
> > + *
> > + * 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 (at your option) 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, write to the Free Software
> > + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston,
> > + * MA 02110-1301 USA
> > + */
> > +
> > +#include <common.h>
> > +#include <asm/io.h>
> > +#include <mvmfp.h>
> > +#include <asm/arch/mfp.h>
> > +#ifdef CONFIG_ARMADA100
> > +#include <asm/arch/armada100.h>
> > +#define MFPR_BASE      ARMD1_MFPR_BASE
> > +#else
> > +#error Unsupported SoC...
> > +#endif
> 
> Why not directly name a CONFIG_MFPR_BASE, and define its value in the
> config file?

Otherway, We can eliminate #define MFPR_BASE here and define the same in asm/arch/armada100.h instead of ARMD1_MFPR_BASE

> If we do like this ifdef, we may need do add each arch here, seems
> some kind of redundant?

This is required here, see drivers/serial/serial.c

...snip...
> > +               /* Write a mfg register as per configuration */
> > +               if (cfg_val & MFP_AF_FLAG) {
> > +                       /* Abstract and program Afternate-Func Selection
> */
> > +                       val &= ~MFP_AF_MASK;
> Do we need to do this & here? For val is only 0 here...

This can be removed.

> Should not it be more concise like:
> writel(cfg_val, p_mfpr);

NACK, cfg_val have some more stuff like offset, flags, those are not required to write on mfp register.

Regards..
Prafulla . .