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.
> + char *name;
> + unsigned char busy;
"busy" isn't used anywhere; remove it.
> + 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;
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.
> + 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. :)
- Dave
_______________________________________________
Davinci-linux-open-source mailing list
Davinci-linux-open-source@linux.davincidsp.com
http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
