Hi,
[let's hope my mails make it through SMTP correctly this time]
On Mon, Feb 18, 2008 at 02:30:55AM +0100, [EMAIL PROTECTED] wrote:
> From: Stefan Schmidt <[EMAIL PROTECTED]>
> Precedence: list
> On Sun, 2008-02-17 at 19:19, Daniel Ribeiro wrote:
> >
> > +#ifdef CONFIG_I2C_BOARDINFO
> > +static struct i2c_board_info __initdata e680_i2c_board_info[] = {
> > + {
> > + I2C_BOARD_INFO("lm4857", 0x7c),
> > + .type = "lm4857",
> > + },
> > + /* TODO when driver support is ready:
> > + * - E680 FM radio
> > + * - ... etc
> > + */
>
> A todo list inside the code is IMHO not that useful.
This one is not my idea originally, I saw it in several other drivers ;)
> > + * TODO:
> > + * - verify driver dependencies / kbuild
> > + * - (re-)activate Neo Mode? (neo1973_get_scenario etc.)
> > + * - have a look at neo1973_wm8753.c to integrate more functionality
> > + * - adapt neo1973_wm8753.c to use our isolated driver instead of
> > + * commingling disparate functionality
> > + * - add very specific probing to make sure we actually have
> > + * an LM4857 at this address
> > + * - mutex needed? (e.g. snd_i2c_lock as used by ALSA-created i2c drivers)
> > + * - remove debugging printks
> > + * - remove all FIXMEs
> > + * - use per-instance data object instead of global data
>
> Todo list in code again. Perhaps just a question of personal taste.
_This_ one IMHO should very much remain just where it is,
since it is a very code-centric item list.
And since the driver still is a bit rough (see below) it's quite useful,
especially since in OSS you have lots of people working on a common
code base and they should know what other people thought about it.
> > +#if CONFIG_PXA_EZX_E680
>
> We need to be carefull with these. Goal is still to have a
> multimachine kernel with runtime detection.
True. However I wanted to get some useful work done ASAP and get it out fast
as I don't have too much time, as evidenced by this lateish reply
(further hindered by SMTP anti-spam foobar).
Improving this should be relatively easy OTOH, thus I'll work on it
during future hacking events.
> Another point would be to run run code through Lindent (Linux ßources
> scripts/) and your patch through checkpatch.pl to see what kernel
> CodingStyle expects from you.
Been there, done that (checkpatch.pl, not lindent).
I'm afraid that the amplifier driver work itself is a rather minor thing only,
since always getting audio routing right no matter whichever situation
the user may be in at a certain moment likely is much more tricky. However
the Neo1973 should hopefully have done many important steps already.
Note that I'm currently more inclined to do some integration work
for compcache into Angstrom or so, since performance of recent rootfs
images on my E680 is absolutely glacial, which largely seems to be
due to horrid memory pressure (48MB RAM ""only"", which is still rather small
compared to Neo's 128MB/256MB(?)).
Thanks for your review!
Andreas Mohr
