"Rajashekhara, Sudhakar" <[email protected]> writes:
> 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.
Please follow Dave's suggestion. It will lead to much more readable
code when mulitple chips are present.
Kevin
>>
>>
>> > > +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
> [email protected]
> http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
_______________________________________________
Davinci-linux-open-source mailing list
[email protected]
http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
