Dave,
> Looks pretty good to me ... arguably, ready to merge right
> now (but I didn't test it), and apply other fixes later.
>
> The basic interfaces look right. I have a few comments
> on the tables themselves, and their management, which
> might better be addressed before merge, to help those
> later updates be smaller.
>
>
> On Tuesday 16 December 2008, Sudhakar Rajashekhara wrote:
> > +
> > +struct pin_config {
>
> Plese rename to "mux_config", so that this infrastructure
> can be used with e.g. the INTMUX and EVTMUX registers on
> the DM355 chips. And update comments accordingly -- just
> stop assuming it's only *pins* that get muxed.
>
> That way follow-on patches can add support for the other
> kinds of registers, without making too many other changes.
>
I'll change the structure name to mux_config.
>
> > + char *name;
> > + unsigned char busy;
>
> "busy" isn't used anywhere; remove it.
Agree.
>
>
> > + unsigned char debug;
>
> This would be more clear as "bool", updating the tables
> to say true/false. (Long lists of numbers == error-prone.)
>
> Plus, if you moved it to the end of the structure with the
> three other byte values, you'd save a word per structure
> by removing needless internal struct padding.
>
>
> > + const char *mux_reg_name;
Agree.
>
> I'll disagree with Felipe about always including the
> two names. That's two pointers per struct, plus the
> strings they hold ... a chunk of memory that's not
> useful except when debugging. But, that waste can
> be removed by a follow-on patch; it's nice to keep
> the initial feature support clean.
I'll leave the initial patch as it is and will submit a patch
at a later point in time to distinguish between debug and
non-debug case.
>
>
> > + const unsigned int mux_reg;
> > + const unsigned char mask_offset;
> > + const unsigned char mask;
> > + const unsigned char mode;
> > +};
>
> And you're still not flagging these struct tables as "const"
> so they'd live in a read-only mapping. Marking some fields
> as immutable is a start, but not a systematic fix.
>
>
> IMO the best way to address that is to just get rid of the
> init-or-module logic; if modules need it, non-module code
> will also need it at runtime. Starting with:
>
> > +struct pin_config __initdata_or_module davinci_dm355_pins[] = {
>
> becoming
>
> > +static const struct mux_config davinci_dm355_mux[] = {
>
> which doesn't needlessly export symbols (so they can be
> detected as unused at build-time and removed), makes them
> const (so they go into read-only sections) and uses more
> generic names so that INTMUX and EVTMUX support can just
> reuse this nice infrastructure without puzzling anyone
> about why "pin" mux code is involved. :)
Agree.
Thanks, Sudhakar
>
> - Dave
> _______________________________________________
Davinci-linux-open-source mailing list
Davinci-linux-open-source@linux.davincidsp.com
http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
