Dave,
> 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.
I'll be adding the minimal DM646x and DM355 defines.
>
>
> > 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.
I will be leaving it as it is for now.
>
>
> > > +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??
I'll be adding TI copyright.
>
>
> > > #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. :)
>
Agree.
>
> > > --- 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).
Agree.
Regards, Sudhakar
>
>
> > > +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
Davinci-linux-open-source@linux.davincidsp.com
http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
