Felipe,
> -----Original Message-----
> From: Felipe Balbi [mailto:[email protected]]
> Sent: Wednesday, December 10, 2008 10:52 PM
> To: Rajashekhara, Sudhakar
> Cc: [email protected]
> Subject: Re: [PATCH v2] ARM: DaVinci: generalize dm644x pinmux
>
> On Wed, Dec 10, 2008 at 03:35:28PM +0530, Sudhakar Rajashekhara wrote:
> > Generalizes dm644x pinmux.
> >
> > The existing pinmux layer works only when the PINMUX register has single
> bit
> > field to enable the secondary function. DM646x can support secondary as
> well
> > as tertiary pin functions. This new pinmux layer is similar to the one
> being
> > used by OMAP architecture.
> >
> > Checkpatch script reports a false positive error at line no.408 of this
> patch.
>
> but it also reports a real positives:
I am using checkpatch version 0.24 and I am not seeing the below real errors.
Regards, Sudhakar
>
> ERROR: Macros with complex values should be enclosed in parenthesis
> #442: FILE: arch/arm/mach-davinci/mux_cfg.c:29:
> +#define MUX_REG(reg, mode_offset, mode_mask, mux_mode) \
> + .mux_reg_name = "PINMUX"#reg, \
> + .mux_reg = PINMUX##reg, \
> + .mask_offset = mode_offset, \
> + .mask = mode_mask, \
> + .mode = mux_mode,
>
> ERROR: Macros with complex values should be enclosed in parenthesis
> #449: FILE: arch/arm/mach-davinci/mux_cfg.c:36:
> +#define MUX_REG(reg, mode_offset, mode_mask, mux_mode) \
> + .mux_reg = PINMUX##reg, \
> + .mask_offset = mode_offset, \
> + .mask = mode_mask, \
> + .mode = mux_mode,
>
> ERROR: Macros with multiple statements should be enclosed in a do - while
> loop
> #456: FILE: arch/arm/mach-davinci/mux_cfg.c:43:
> +#define MUX_CFG(desc, mux_reg, mode_offset, mode_mask, mux_mode, dbg)
> \
> +{ \
> + .name = desc, \
> + .debug = dbg, \
> + MUX_REG(mux_reg, mode_offset, mode_mask, mux_mode) \
> +},
>
> but most likely you want something like:
>
> +#define MUX_REG(reg, mode_offset, mode_mask, mux_mode) \
> + { \
> + .mux_reg_name = "PINMUX"#reg, \
> + .mux_reg = PINMUX##reg, \
> + .mask_offset = mode_offset, \
> + .mask = mode_mask, \
> + .mode = mux_mode, \
> + },
>
> > diff --git a/arch/arm/mach-davinci/Kconfig b/arch/arm/mach-
> davinci/Kconfig
> > index f82e6f4..20e55ea 100644
> > --- a/arch/arm/mach-davinci/Kconfig
> > +++ b/arch/arm/mach-davinci/Kconfig
> > @@ -50,6 +50,31 @@ config MACH_DAVINCI_SFFSDR
> > Say Y here to select the Lyrtech Small Form Factor
> > Software Defined Radio (SFFSDR) board.
> >
> > +config DAVINCI_MUX
> > + bool "DAVINCI multiplexing support"
> > + depends on ARCH_DAVINCI
> > + default y
> > + help
> > + Pin multiplexing support for DAVINCI boards. If your bootloader
> > + sets the multiplexing correctly, say N. Otherwise, or if unsure,
> > + say Y.
> > +
> > +config DAVINCI_MUX_DEBUG
> > + bool "Multiplexing debug output"
> > + depends on DAVINCI_MUX
> > + help
> > + Makes the multiplexing functions print out a lot of debug
> info.
> > + This is useful if you want to find out the correct values of
> the
> > + multiplexing registers.
> > +
> > +config DAVINCI_MUX_WARNINGS
> > + bool "Warn about pins the bootloader didn't set up"
>
> I'd say we shouldn't rely on bootloader to setup mux config, so this
> would be quite useless.
>
> > + depends on DAVINCI_MUX
> > + help
> > + Choose Y here to warn whenever driver initialization logic
> needs
> > + to change the pin multiplexing setup. When there are no
> warnings
> > + printed, it's safe to deselect DAVINCI_MUX for your product.
> > +
> > config DAVINCI_RESET_CLOCKS
> > bool "Reset unused clocks during boot"
> > depends on ARCH_DAVINCI
> > diff --git a/arch/arm/mach-davinci/Makefile b/arch/arm/mach-
> davinci/Makefile
> > index 2a339db..d2b4e24 100644
> > --- a/arch/arm/mach-davinci/Makefile
> > +++ b/arch/arm/mach-davinci/Makefile
> > @@ -5,7 +5,7 @@
> >
> > # Common objects
> > obj-y := time.o irq.o clock.o serial.o io.o id.o
> > psc.o \
> > - gpio.o mux.o devices.o usb.o dma.o common.o iram.o
> > + gpio.o mux.o mux_cfg.o devices.o usb.o dma.o common.o
> iram.o
>
> mux should only build if CONFIG_DAVINCI_MUX is true.
>
> > # Board specific
> > obj-$(CONFIG_MACH_DAVINCI_EVM) += board-dm644x-evm.o
> > diff --git a/arch/arm/mach-davinci/include/mach/mux.h b/arch/arm/mach-
> davinci/include/mach/mux.h
> > index 9e7cf69..317f7c7 100644
> > --- a/arch/arm/mach-davinci/include/mach/mux.h
> > +++ b/arch/arm/mach-davinci/include/mach/mux.h
> > @@ -1,60 +1,93 @@
> > /*
> > - * DaVinci pin multiplexing defines
> > + * Table of the DAVINCI register configurations for the PINMUX
> combinations
> > *
> > * Author: Vladimir Barinov, MontaVista Software, Inc.
> <[email protected]>
> > *
> > + * Based on linux/include/asm-arm/arch-omap/mux.h:
> > + * Copyright (C) 2003 - 2005 Nokia Corporation
> > + * Written by Tony Lindgren <[email protected]>
> > + *
> > * 2007 (c) MontaVista Software, Inc. This file is licensed under
> > * the terms of the GNU General Public License version 2. This program
> > * is licensed "as is" without any warranty of any kind, whether
> express
> > * or implied.
> > */
> > +
>
> why ??
>
> > #ifndef __ASM_ARCH_MUX_H
> > #define __ASM_ARCH_MUX_H
> >
> > -/* System control register offsets */
> > -#define PINMUX0 0x00
> > -#define PINMUX1 0x04
> > -
> > -/* System control register bits */
> > -#define DAVINCI_MUX_AEAW0 0
> > -#define DAVINCI_MUX_AEAW1 1
> > -#define DAVINCI_MUX_AEAW2 2
> > -#define DAVINCI_MUX_AEAW3 3
> > -#define DAVINCI_MUX_AEAW4 4
> > -#define DAVINCI_MUX_AECS4 10
> > -#define DAVINCI_MUX_AECS5 11
> > -#define DAVINCI_MUX_VLYNQWD0 12
> > -#define DAVINCI_MUX_VLYNQWD1 13
> > -#define DAVINCI_MUX_VLSCREN 14
> > -#define DAVINCI_MUX_VLYNQEN 15
> > -#define DAVINCI_MUX_HDIREN 16
> > -#define DAVINCI_MUX_ATAEN 17
> > -#define DAVINCI_MUX_RGB666 22
> > -#define DAVINCI_MUX_RGB888 23
> > -#define DAVINCI_MUX_LOEEN 24
> > -#define DAVINCI_MUX_LFLDEN 25
> > -#define DAVINCI_MUX_CWEN 26
> > -#define DAVINCI_MUX_CFLDEN 27
> > -#define DAVINCI_MUX_HPIEN 29
> > -#define DAVINCI_MUX_1394EN 30
> > -#define DAVINCI_MUX_EMACEN 31
> > -
> > -#define DAVINCI_MUX_LEVEL2 32
> > -#define DAVINCI_MUX_UART0 (DAVINCI_MUX_LEVEL2 + 0)
> > -#define DAVINCI_MUX_UART1 (DAVINCI_MUX_LEVEL2 + 1)
> > -#define DAVINCI_MUX_UART2 (DAVINCI_MUX_LEVEL2 + 2)
> > -#define DAVINCI_MUX_U2FLO (DAVINCI_MUX_LEVEL2 + 3)
> > -#define DAVINCI_MUX_PWM0 (DAVINCI_MUX_LEVEL2 + 4)
> > -#define DAVINCI_MUX_PWM1 (DAVINCI_MUX_LEVEL2 + 5)
> > -#define DAVINCI_MUX_PWM2 (DAVINCI_MUX_LEVEL2 + 6)
> > -#define DAVINCI_MUX_I2C (DAVINCI_MUX_LEVEL2 + 7)
> > -#define DAVINCI_MUX_SPI (DAVINCI_MUX_LEVEL2 + 8)
> > -#define DAVINCI_MUX_MSTK (DAVINCI_MUX_LEVEL2 + 9)
> > -#define DAVINCI_MUX_ASP (DAVINCI_MUX_LEVEL2 + 10)
> > -#define DAVINCI_MUX_CLK0 (DAVINCI_MUX_LEVEL2 + 16)
> > -#define DAVINCI_MUX_CLK1 (DAVINCI_MUX_LEVEL2 + 17)
> > -#define DAVINCI_MUX_TIMIN (DAVINCI_MUX_LEVEL2 + 18)
> > -
> > -extern void davinci_mux_peripheral(unsigned int mux, unsigned int
> enable);
> > -
> > -#endif /* __ASM_ARCH_MUX_H */
> > +struct pin_config {
> > + char *name;
> > + unsigned char busy;
> > + unsigned char debug;
> > + const char *mux_reg_name;
> > + const unsigned int mux_reg;
> > + const unsigned char mask_offset;
> > + const unsigned char mask;
> > + const unsigned char mode;
> > +};
> > +
> > +enum davinci_dm644x_index {
> > + /* ATA and HDDIR functions */
> > + DM644X_HDIREN,
> > + DM644X_ATAEN,
> > +
> > + /* Memory Stick */
> > + DM644X_MSTK,
> > +
> > + /* I2C */
> > + DM644X_I2C,
> > +
> > + /* ASP function */
> > + DM644X_MCBSP,
> > +
> > + /* PWM0 */
> > + DM644X_PWM0,
> > +
> > + /* PWM1 */
> > + DM644X_PWM1,
> > +
> > + /* PWM2 */
> > + DM644X_PWM2,
> > +
> > + /* VLINQ function */
> > + DM644X_VLINQEN,
> > + DM644X_VLINQWD,
> > +
> > + /* EMAC and MDIO function */
> > + DM644X_EMACEN,
> > +
> > + /* GPIO3V[0:16] pins */
> > + DM644X_GPIO3V,
> > +
> > + /* GPIO pins */
> > + DM644X_GPIO0,
> > + DM644X_GPIO3,
> > + DM644X_GPIO43_44,
> > + DM644X_GPIO46_47,
> > +
> > + /* VPBE */
> > + DM644X_RGB666,
> > +
> > + /* LCD */
> > + DM644X_LOEEN,
> > + DM644X_LFLDEN,
> > +};
> > +
> > +#ifdef CONFIG_DAVINCI_MUX
> > +/* setup pin muxing */
> > +extern void davinci_mux_init(void);
> > +extern int davinci_mux_register(struct pin_config *pins, unsigned long
> size);
> > +extern int davinci_cfg_reg(unsigned long reg_cfg);
> > +#else
> > +/* boot loader does it all (no warnings from
> CONFIG_DAVINCI_MUX_WARNINGS) */
> > +static inline void davinci_mux_init(void)
> > +{
> > + do {} while (0);
>
> remove this line, it's as useful as
> if (0)
> printk(KERN_INFO "This won't execute\n");
>
> > +}
> > +static inline int davinci_cfg_reg(unsigned long reg_cfg) { return 0; }
> > +#endif
> > +
> > +extern void (*davinci_pinmux_setup)(unsigned int id);
> > +
> > +#endif
> > diff --git a/arch/arm/mach-davinci/io.c b/arch/arm/mach-davinci/io.c
> > index 299515f..c493889 100644
> > --- a/arch/arm/mach-davinci/io.c
> > +++ b/arch/arm/mach-davinci/io.c
> > @@ -18,6 +18,7 @@
> >
> > #include <asm/mach/map.h>
> > #include <mach/clock.h>
> > +#include <mach/mux.h>
> >
> > extern void davinci_check_revision(void);
> >
> > @@ -53,5 +54,6 @@ void __init davinci_map_common_io(void)
> >
> > void __init davinci_init_common_hw(void)
> > {
> > + davinci_mux_init();
> > davinci_clk_init();
> > }
> > diff --git a/arch/arm/mach-davinci/mux.c b/arch/arm/mach-davinci/mux.c
> > index be33108..e41f6f5 100644
> > --- a/arch/arm/mach-davinci/mux.c
> > +++ b/arch/arm/mach-davinci/mux.c
> > @@ -1,8 +1,12 @@
> > /*
> > - * DaVinci pin multiplexing configurations
> > + * Utility to set the DAVINCI MUX register from a table in mux.h
>
> the table should probably be chip-specific.
>
> > *
> > * Author: Vladimir Barinov, MontaVista Software, Inc.
> <[email protected]>
> > *
> > + * Based on linux/arch/arm/plat-omap/mux.c:
> > + * Copyright (C) 2003 - 2005 Nokia Corporation
> > + * Written by Tony Lindgren <[email protected]>
>
> where did you get this mail ??
> It's [email protected]
>
> > + *
> > * 2007 (c) MontaVista Software, Inc. This file is licensed under
> > * the terms of the GNU General Public License version 2. This program
> > * is licensed "as is" without any warranty of any kind, whether
> express
> > @@ -13,27 +17,84 @@
> > #include <linux/spinlock.h>
> >
> > #include <mach/hardware.h>
> > -
> > #include <mach/mux.h>
> >
> > -static DEFINE_SPINLOCK(mux_lock);
> > +#ifdef CONFIG_DAVINCI_MUX
>
> don't ifdef here.
>
> > +
> > +static struct pin_config *pin_table;
> > +static unsigned long pin_table_sz;
> > +
> > +int __init davinci_mux_register(struct pin_config *pins, unsigned long
> size)
> > +{
> > + pin_table = pins;
> > + pin_table_sz = size;
> > +
> > + return 0;
> > +}
> >
> > -void davinci_mux_peripheral(unsigned int mux, unsigned int enable)
> > +/*
> > + * Sets the DAVINCI MUX register based on the table
> > + */
> > +int __init_or_module davinci_cfg_reg(const unsigned long index)
>
> this cannot be called by modules, or at least shouldn't. So __init
> should be safe.
>
> > {
> > - u32 pinmux, muxreg = PINMUX0;
> > + static DEFINE_SPINLOCK(mux_spin_lock);
> > +
> > + unsigned long flags;
> > + struct pin_config *cfg;
> > + unsigned int reg_orig = 0, reg = 0;
> > + unsigned int mask, warn = 0;
> > +
> > + if (!pin_table)
> > + BUG();
> > +
> > + if (index >= pin_table_sz) {
> > + printk(KERN_ERR "Invalid pin mux index: %lu (%lu)\n",
> > + index, pin_table_sz);
> > + dump_stack();
> > + return -ENODEV;
> > + }
> > +
> > + cfg = (struct pin_config *)&pin_table[index];
> > +
> > + /* Check the mux register in question */
> > + if (cfg->mux_reg) {
> > + unsigned tmp1, tmp2;
> > +
> > + spin_lock_irqsave(&mux_spin_lock, flags);
> > + reg_orig = davinci_readl(cfg->mux_reg);
> > +
> > + mask = (cfg->mask << cfg->mask_offset);
> > + tmp1 = reg_orig & mask;
> > + reg = reg_orig & ~mask;
> > +
> > + tmp2 = (cfg->mode << cfg->mask_offset);
> > + reg |= tmp2;
> > +
> > + if (tmp1 != tmp2)
> > + warn = 1;
> > +
> > + davinci_writel(reg, cfg->mux_reg);
> > + spin_unlock_irqrestore(&mux_spin_lock, flags);
> > + }
> > +
> > + if (warn) {
> > +#ifdef CONFIG_DAVINCI_MUX_WARNINGS
> > + printk(KERN_WARNING "MUX: initialized %s\n", cfg->name);
> > +#endif
> > + }
> >
> > - if (mux >= DAVINCI_MUX_LEVEL2) {
> > - muxreg = PINMUX1;
> > - mux -= DAVINCI_MUX_LEVEL2;
> > +#ifdef CONFIG_DAVINCI_MUX_DEBUG
> > + if (cfg->debug || warn) {
> > + printk(KERN_WARNING "MUX: Setting register %s\n", cfg->name);
> > + printk(KERN_WARNING " %s (0x%08x) = 0x%08x -> 0x%08x\n",
> > + cfg->mux_reg_name, cfg->mux_reg, reg_orig, reg);
> > }
> > +#endif
> >
> > - spin_lock(&mux_lock);
> > - pinmux = davinci_readl(DAVINCI_SYSTEM_MODULE_BASE + muxreg);
> > - if (enable)
> > - pinmux |= (1 << mux);
> > - else
> > - pinmux &= ~(1 << mux);
> > - davinci_writel(pinmux, DAVINCI_SYSTEM_MODULE_BASE + muxreg);
> > - spin_unlock(&mux_lock);
> > + return 0;
> > }
> > -EXPORT_SYMBOL(davinci_mux_peripheral);
> > +EXPORT_SYMBOL(davinci_cfg_reg);
> > +#else
> > +#define davinci_mux_init() do {} while (0)
> > +#define davinci_cfg_reg(x) do {} while (0)
>
> this should be done in the header file and should be static inline,
> instead of defines.
>
> > +#endif /* CONFIG_DAVINCI_MUX */
> > diff --git a/arch/arm/mach-davinci/mux_cfg.c b/arch/arm/mach-
> davinci/mux_cfg.c
> > new file mode 100644
> > index 0000000..fc00e6b
> > --- /dev/null
> > +++ b/arch/arm/mach-davinci/mux_cfg.c
> > @@ -0,0 +1,99 @@
> > +/*
> > + * DAVINCI pin multiplexing configurations
> > + *
> > + * Author: Vladimir Barinov, MontaVista Software, Inc.
> <[email protected]>
> > + *
> > + * Based on linux/arch/arm/mach-omap1/mux.c:
> > + * Copyright (C) 2003 - 2005 Nokia Corporation
> > + * Written by Tony Lindgren <[email protected]>
> > + *
> > + * 2007 (c) MontaVista Software, Inc. This file is licensed under
> > + * the terms of the GNU General Public License version 2. This program
> > + * is licensed "as is" without any warranty of any kind, whether
> express
> > + * or implied.
> > + */
> > +
> > +#include <linux/module.h>
> > +#include <linux/init.h>
> > +
> > +#include <mach/hardware.h>
> > +#include <mach/cpu.h>
> > +#include <mach/mux.h>
> > +#include <mach/dm644x.h>
> > +
> > +/* System module registers */
> > +#define PINMUX0 (DAVINCI_SYSTEM_MODULE_BASE + 0x00)
> > +#define PINMUX1 (DAVINCI_SYSTEM_MODULE_BASE + 0x04)
> > +
> > +#ifdef CONFIG_DAVINCI_MUX_DEBUG
> > +#define MUX_REG(reg, mode_offset, mode_mask, mux_mode) \
> > + .mux_reg_name = "PINMUX"#reg, \
>
> you should always have the name. Remove this conditional redefinition.
>
> > + .mux_reg = PINMUX##reg, \
> > + .mask_offset = mode_offset, \
> > + .mask = mode_mask, \
> > + .mode = mux_mode,
> > +#else
> > +#define MUX_REG(reg, mode_offset, mode_mask, mux_mode) \
> > + .mux_reg = PINMUX##reg, \
> > + .mask_offset = mode_offset, \
> > + .mask = mode_mask, \
> > + .mode = mux_mode,
> > +#endif /* CONFIG_DAVINCI_MUX_DEBUG */
> > +
> > +#define MUX_CFG(desc, mux_reg, mode_offset, mode_mask, mux_mode, dbg)
> \
> > +{ \
> > + .name = desc, \
> > + .debug = dbg, \
> > + MUX_REG(mux_reg, mode_offset, mode_mask, mux_mode) \
> > +},
> > +
> > +#ifdef CONFIG_DAVINCI_MUX
> > +
> > +struct pin_config __initdata_or_module davinci_dm644x_pins[] = {
> > +/*
> > + * description mux mode mode mux dbg
> ^ trailing whitespace
> > + * reg offset mask mode
> > + */
>
> this comment should be above the struct.
>
> > +[DM644X_HDIREN] = MUX_CFG("HDIREN", 0, 16, 1,
> 1, 1)
> > +[DM644X_ATAEN] = MUX_CFG("ATAEN", 0, 17, 1, 1,
> 1)
> > +
> > +[DM644X_MSTK] = MUX_CFG("MSTK", 1, 9, 1, 0,
> 0)
> > +
> > +[DM644X_I2C] = MUX_CFG("I2C", 1, 7, 1, 1,
> 0)
> > +
> > +[DM644X_MCBSP] = MUX_CFG("MCBSP", 1, 10, 1, 1,
> 0)
> > +
> > +[DM644X_PWM0] = MUX_CFG("PWM0", 1, 4, 1, 1,
> 0)
> > +
> > +[DM644X_PWM1] = MUX_CFG("PWM1", 1, 5, 1, 1,
> 0)
> > +
> > +[DM644X_PWM2] = MUX_CFG("PWM2", 1, 6, 1, 1,
> 0)
> > +
> > +[DM644X_VLINQEN] = MUX_CFG("VLINQEN", 0, 15, 1,
> 1, 0)
> > +[DM644X_VLINQWD] = MUX_CFG("VLINQWD", 0, 12, 3,
> 3, 0)
> > +
> > +[DM644X_EMACEN] = MUX_CFG("EMACEN", 0, 31, 1,
> 1, 1)
> > +
> > +[DM644X_GPIO3V] = MUX_CFG("GPIO3V", 0, 31, 1,
> 0, 1)
> > +
> > +[DM644X_GPIO0] = MUX_CFG("GPIO0", 0, 24, 1, 0,
> 1)
> > +[DM644X_GPIO3] = MUX_CFG("GPIO3", 0, 25, 1, 0,
> 0)
> > +[DM644X_GPIO43_44] = MUX_CFG("GPIO43_44", 1, 7, 1, 0,
> 0)
> > +[DM644X_GPIO46_47] = MUX_CFG("GPIO46_47", 0, 22, 1, 0,
> 1)
> > +
> > +[DM644X_RGB666] = MUX_CFG("RGB666", 0, 22, 1,
> 1, 1)
> > +
> > +[DM644X_LOEEN] = MUX_CFG("LOEEN", 0, 24, 1, 1,
> 1)
> > +[DM644X_LFLDEN] = MUX_CFG("LFLDEN", 0, 25, 1,
> 1, 0)
> > +};
> > +
> > +void __init davinci_mux_init(void)
> > +{
> > + if (cpu_is_davinci_dm644x())
> > + davinci_mux_register(davinci_dm644x_pins,
> > + ARRAY_SIZE(davinci_dm644x_pins));
> > + else
> > + printk(KERN_ERR "PSC: no mux hook for this CPU\n");
>
> you should pass the mux table as a parameter to this function.
>
> --
> balbi
_______________________________________________
Davinci-linux-open-source mailing list
[email protected]
http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
