Messages in this thread | | | Date | Thu, 28 Aug 2008 13:55:33 +0800 | From | "Bryan Wu" <> | Subject | Re: [alsa-devel] [PATCH 2/4] ASOC codec: add support for SSM2602 audio codec in ALSA SoC framework |
| |
On Wed, Aug 27, 2008 at 6:54 PM, Mark Brown <broonie@sirena.org.uk> wrote: > On Wed, Aug 27, 2008 at 05:39:26PM +0800, Bryan Wu wrote: > > This looks basically good, thanks - I've picked up a few things below > but they are mostly either minor or reflect the fact that the patch > looks like it's been developed against current release kernels but > there's been some churn recently in the ASoC APIs which need updates. > >> #define I2C_DRIVERID_CS5345 96 /* cs5345 audio processor */ >> +#define I2C_DRIVERID_SSM2602 97 /* BF52xC built in audio codec */ > > It should be possible to just remove this - it shouldn't be needed. >
OK, I'll take care of this i2c stuff here.
>> --- a/sound/soc/codecs/Kconfig >> +++ b/sound/soc/codecs/Kconfig > > Current kernels have a Kconfig option SND_SOC_ALL_CODECS which should > have your codec added - this allows codec drivers to be built without > boards for test purposes. > Is this option in alsa git tree or in the mainline? I fail to find it in upstream mainline. And do you mean I don't need to add SND_SOC_SSM2602 at all?
>> +#define SSM2602_DEBUG 0 >> + >> +#ifdef SSM2602_DEBUG >> +#define dbg(format, arg...) \ >> + printk(KERN_DEBUG AUDIO_NAME ": " format "\n" , ## arg) >> +#else >> +#define dbg(format, arg...) do {} while (0) >> +#endif >> +#define err(format, arg...) \ >> + printk(KERN_ERR AUDIO_NAME ": " format "\n" , ## arg) >> +#define info(format, arg...) \ >> + printk(KERN_INFO AUDIO_NAME ": " format "\n" , ## arg) >> +#define warn(format, arg...) \ >> + printk(KERN_WARNING AUDIO_NAME ": " format "\n" , ## arg) > > Please convert these to use the standard pr_ macros (or ideally the dev_ > ones where possible) for debug prints. >
Right, I killed this local definition and replaced them to pr_xxxx.
>> + >> +#define ssm2602_reset(c) ssm2602_write(c, SSM2602_RESET, 0) >> +/*Appending several "None"s just for OSS mixer use*/ >> +static const char *ssm2602_input_select[] = {"Line", "Mic", "None", "None", "None", >> + "None", "None", "None"}; >> +static const char *ssm2602_deemph[] = {"None", "32Khz", "44.1Khz", "48Khz"}; > > Please keep the lines under 80 characters where reasonable. A little > more while space (blank lines and at the start and end of comments) > would be nice too. >
Yes, I will fix this issue by running checkpatch.pl. And for other API conflicts and I2C interface upgrading stuffs, I will leave them to Cliff.
Thanks -Bryan
| |