I'll incorporate the comments from Dave and Felipe and resubmit the patch to the list.
Thanks, Sudhakar ________________________________________ From: David Brownell [[email protected]] Sent: Thursday, December 11, 2008 12:44 AM To: [email protected]; Rajashekhara, Sudhakar Cc: [email protected] Subject: Re: [PATCH v2] ARM: DaVinci: generalize dm644x pinmux I'll disagree with a few of Felipe's points here, and offer a few other comments. On Wednesday 10 December 2008, Felipe Balbi wrote: > 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. As I said before: I think this is the right general idea. Though for generality, I'd like to see at least partial DM355 support here too ... that's a good way to help ensure that DM644x-specific assumptions are minimized! Minimally, just add the other PINMUX registers and provide a table with a few DM355 pinmux options (to be completed later). Maybe the same for DM646x too. > 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, \ > + }, Or better yet: +#define MUX_REG(reg, mode_offset, mode_mask, mux_mode) \ + [SOC##_##reg] = { ... then #define SOC to DM644X before the table of pinmux configs for those chips, redefine to DM355 for its set, and so on. > > +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. Not relying on the boot loader is a good general policy; but the thing with policies like that is that there are sometimes good reasons to use different ones. I have no problems leaving this warning mechanism in place, and would certainly prefer to have the option of leaving all the pinmux code out of a kernel. > > * 2007 (c) MontaVista Software, Inc. Really? Not TI? And no TI copyright at all?? > > #ifndef __ASM_ARCH_MUX_H > > #define __ASM_ARCH_MUX_H It's not <asm/arch/mux.h> it's <mach/mux.h> so I'd suggest using a different #include guard. :) > > --- 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. And it is! But it's in mux_cfg.c ... which might eventually get split into chip-specific files, though I don't see a need to do that yet. It's just a bunch of data, most of which gets compiled out because non-development kernels don't support very many chips (usually just one). > > +/* > > + * 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. Disagree for two reasons. One is just the observation that developers on platforms which started with such code living in __init sections have needed to change that so that it could be called later. As I noted earlier: this kind of policy needs exceptions. (Like being able to briefly use a pin as a GPIO, before re-associating it with some controller.) The other is that DaVinci (at least DM6446) has at least one fairly common reason to want to tweak the pinmux settings at runtime. IDE and NOR flash are mutually exclusive ... systems that need to use both of them are going to need to remux things. > > + 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]; No cast needed ... and the paranoia in me wants to see the pinmux tables always be "const" data, to minimize ways that memory-trashing bugs can cause major goofage. Also check to make sure that what cfg points to is valid. Now that you've moved away from relying on the enum order being the same as the table order (yay! another type of programmer oversight becomes innocuous!), there's the possibility for a table entry not to have been initialized (except to all-zeroes). > > +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. I think it's simpler to do it this way. It *could* be done with several CPU-specific files calling to davinci_mux_register(), but there's no need to do that ... if this file holds tables for dm644x, dm355, dm646x and so on, it's clear enough and will not cause maintainance headaches for some time. - Dave_______________________________________________ Davinci-linux-open-source mailing list [email protected] http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
