[alsa-devel] [PATCH] ALSA SOC driver for s3c24xx with uda1341

[chri]

On Mon, Nov 10, 2008 at 11:09 PM, Ben Dooks <ben-linux at fluff.org> wrote:

>
> example into a doc file please, or point at an implementation of
> this.
>

ack

>> +#define setdat(adap, val)    (adap->setdat(adap, val))
>> +#define setclk(adap, val)    (adap->setclk(adap, val))
>> +#define setmode(adap, val)   (adap->setmode(adap, val))
>
> inline functions are better than #define.
>

ack

>> +#include "uda1341_platform_data.h"
>
> please find as better name for that file.
>

I always have problems as far as platform data is concerned because it
should be separated from the private data structures of the driver; so
it's rather common to just have a small ad-hoc file for them. In
socketcan a decision was made to put all the platform data in a
subdirectory so the name can be short and one knows where to look. Do
you have any suggestion where is the best place to put this file and
for a consistent naming convention?

>> +}
>
> do you really want to be busy-waiting the cpu for that long? how
> about msleep?
>

ack, I'll use msleep

>> +     void (*setmode) (struct uda1341_platform_data *, int);
> no spaces              ^
>

ack. It's strange that checkpatch.pl didn't catch this

>
> find somewhere for these to live that can be included without
> resorting to knowing the directory layout.
>

ack, I'll try include/sound

>> +}
>
> please use the generic gpio layer, the s3c24xx functions are going
> to be deprecated and removed as soon as possible, probably after
> 2.6.28.
>

ack.

>> +     pclk = clk_get(NULL, "pclk");
>
> check for errors. really you should be passing a device for pclk.
>

ack

Thanks,


-- 
Christian Pellegrin, see http://www.evolware.org/chri/
"Real Programmers don't play tennis, or any other sport which requires
you to change clothes. Mountain climbing is OK, and Real Programmers
wear their climbing boots to work in case a mountain should suddenly
spring up in the middle of the computer room."