Re: [PATCH] ASoC: Add support for Cirrus Logic CS42L52 Lowpower Stereo Codec

Re: [PATCH] ASoC: Add support for Cirrus Logic CS42L52 Lowpower Stereo Codec

[Mark Brown]

On Thu, Feb 25, 2010 at 06:07:35PM -0600, Brian wrote:

This is a quick first pass review only.  It'd be best if you could
include a Signed-off-by line in submissions so that they can be merged
if they're OK, even if you expect problems you may be surprised!

Overall this looks fairly good - there's a lot of comments below but the
vast majority of them are stylistic issues or needing to update to
current APIs rather than major structural problems with the code.  The
main thing I don't really follow is the register access code, there's
quite a bit of stuff going on in there.

> + * published by the Free Software Foundation.
> + * Revision history
> + * Nov 2007  Initial version.
> + * Oct 2008  Updated to 2.6.26
> + * Feb 2010  Updated to latest asoc git tree

Please remove the changelog, once things are merged git takes care of
that.

> + */
> +#define DEBUG

Not by default in mainline :)

> +#ifdef DEBUG
> +#define SOCDBG(fmt, arg...)	printk(KERN_ERR "%s: %s() " fmt, SOC_CS42L52_NAME, __FUNCTION__, ##arg)
> +#else
> +#define SOCDBG(fmt, arg...)
> +#endif
> +#define SOCINF(fmt, args...)	printk(KERN_INFO "%s: " fmt, SOC_CS42L52_NAME,  ##args)
> +#define SOCERR(fmt, args...)	printk(KERN_ERR "%s: " fmt, SOC_CS42L52_NAME,  ##args)

Please convert the driver to use the standard dev_ and pr_ macros for
this (where possible the dev_ ones are better).

> +static int soc_cs42l52_set_bias_level(struct snd_soc_codec *codec,
> +				enum snd_soc_bias_level level);
> +static int soc_cs42l52_pcm_hw_params(struct snd_pcm_substream *substream,
> +                        	struct snd_pcm_hw_params *params,
> +				struct snd_soc_dai *dai);
> +static int soc_cs42l52_set_sysclk(struct snd_soc_dai *codec_dai,
> +                        int clk_id, u_int freq, int dir);
> +static int soc_cs42l52_digital_mute(struct snd_soc_dai *dai, int mute);
> +static int soc_cs42l52_set_fmt(struct snd_soc_dai *codec_dai,
> +                        u_int fmt);
> +
> +static unsigned int soc_cs42l52_read(struct snd_soc_codec *codec,
> +		                u_int reg);

Please use 'unsigned int' rather than 'u_int' for consistency with the
rest of ASoC.  It's a bit surprising that you need the forward
declarations at all but they do no harm.

> +/**
> + * snd_soc_get_volsw - single mixer get callback
> + * @kcontrol: mixer control
> + * @uinfo: control element information
> + *
> + * Callback to get the value of a single mixer control.
> + *
> + * Returns 0 for success.
> + */
> +int snd_soc_cs42l5x_get_volsw(struct snd_kcontrol *kcontrol,
> +    struct snd_ctl_elem_value *ucontrol)
> +{
> +    struct snd_soc_codec *codec = snd_kcontrol_chip(kcontrol);
> +    int reg = kcontrol->private_value & 0xff;

The indentation looks wrong - should be tabs for indenting the code
blocks.  scripts/checkpatch.pl is a pretty good check for this sort of
stuff, I'll not mention other issues.

> +    int shift = (kcontrol->private_value >> 8) & 0x0f;
> +    int rshift = (kcontrol->private_value >> 12) & 0x0f;
> +    int max = (kcontrol->private_value >> 16) & 0xff;
> +    int mask = (1 << fls(max)) - 1;
> +    int min = (kcontrol->private_value >> 24) & 0xff;
> +
> +    ucontrol->value.integer.value[0] =
> +	((snd_soc_read(codec, reg) >> shift) - min) & mask;
> +    if (shift != rshift)
> +	ucontrol->value.integer.value[1] =
> +	    ((snd_soc_read(codec, reg) >> rshift) - min) & mask;

This and most of the other custom control stuff looks an awful lot like
it should be done by adding a SND_SOC_DOUBLE_SIGNED() or something along
the lines of the existing _S8() but I didn't read entirely closely.  I
was speaking to someone on IRC only the other day who needed some of
those types for a different device.

These should also be updated to use struct soc_mixer_control for the
private value rather than shifting everything into the private value, it
allows wider ranges and is less error prone.

> +static const u8 soc_cs42l52_reg_default[] = {
> +	0x00, 0xE0, 0x01, 0x07, 0x05, /*4*/
> +	0xa0, 0x00, 0x00, 0x81, /*8*/

Very nitpicky but you're using a mix of upper and lower case for the
letters in the hex digits.

> +static inline int soc_cs42l52_read_reg_cache(struct snd_soc_codec *codec,
> +		u_int reg)
> +{
> +	u8 *cache = codec->reg_cache;
> +
> +	return reg > SOC_CS42L52_REG_NUM ? -EINVAL : cache[reg];
> +}

You should be able to replace all your register I/O code with use of
soc-cache.c with 8 bit data, 8 bit register.  Except...

> +	if(info->flags & SOC_CS42L52_ALL_IN_ONE)
> +	{
> +		for(i = 0; i < codec->num_dai; i++)
> +		{
> +			if(info->machine_handler) 
> +				info->machine_handler(i);

...I'm not sure what's going on here but it could really use an
explanation?  I'm not sure what ALL_IN_ONE is supposed to do, or what
the machine_handler() function is for.

> +		if(info->flags & SOC_CS42L52_CHIP_SWICTH)
> +		{
> +			num = info->flags & SOC_CS42L52_CHIP_MASK;
> +			if(info->machine_handler)
> +                                info->machine_handler(num);
> +		}

This too.  There's a lot of unusual code using this machine handler
function.  I suspect this can all use ASoC standard stuff.

> +static inline int soc_cs42l52_get_revison(struct snd_soc_codec *codec)
> +{
> +	u8 data;
> +        u8 addr;
> +	int num, ret = 0;

> +	struct soc_codec_cs42l52 *info = (struct soc_codec_cs42l52*)codec->private_data;

No need to cast away from void (quite a few examples of this in the
code).

> +	return ret < 0 ? ret : data;

I'm not a big fan of the ternery operator...

> +static unsigned int soc_cs42l52_read(struct snd_soc_codec *codec,
> +		u_int reg)
> +{
> +	u8 data;
> +	u8 addr;
> +        int i, ret = 0;
> +        struct soc_codec_cs42l52 *info = (struct soc_codec_cs42l52*)codec->private_data;
> +#ifndef CONFIG_CS42L52_DEBUG
> +	if(reg == CODEC_CS42L52_SPK_STATUS)
> +	{
> +#endif

This really shouldn't be ifdefed.  If you need to handle registers that
report status from the device with the soc-cache stuff use the
volatile_register() callback in the CODEC, any registers that need a
CODEC read will call through to the hardware rather than using the
cache.

> +static const char *cs42l52_adc_mux[] = {"AIN1", "AIN2", "AIN3", "AIN4", "PGA"};
> +static const char *cs42l52_mic_mux[] = {"MIC1", "MIC2"};
> +static const char *cs42l52_stereo_mux[] = {"Mono", "Stereo"};
> +static const char *cs42l52_off[] = {"On", "Off"};
> +static const char *cs42l52_hpmux[] = {"Off", "On"};
> +
> +static const struct soc_enum soc_cs42l52_enum[] = {
> +SOC_ENUM_DOUBLE(CODEC_CS42L52_ANALOG_HPF_CTL, 4, 6, 2, cs42l52_hpf_freeze), /*0*/
> +SOC_ENUM_SINGLE(CODEC_CS42L52_ADC_HPF_FREQ, 0, 4, cs42l52_hpf_corner_freq),

Don't use an array of enums, define individual variables for them.  Some
drivers use the arrays for historical reasons - they're quite painful to
work with if you ever want to change anything in the middle and we've
found a few off by one errors in the referencing into the arrays before
:/.

> +static const struct snd_kcontrol_new soc_cs42l52_controls[] = {
> +
> +SOC_ENUM("Mic VA Capture Switch", soc_cs42l52_enum[18]), /*0*/

enums shouldn't be called Switch - Switch has a specific meaning to ALSA
(see Documentation/sound/alsa/ControlNames.txt, there's quite a few
other controls could do with having the components of the name reordered
and various other things following that).  Normally just omit the ' Switch'
from the name.

> +SOC_DOUBLE("Analog SR Capture Switch", CODEC_CS42L52_ANALOG_HPF_CTL, 1, 3, 1, 1),
> +SOC_DOUBLE("Analog ZC Capture Switch", CODEC_CS42L52_ANALOG_HPF_CTL, 0, 2, 1, 1),

This'd normally be 'Capture ZC Switch' if it's for zero cross.

> +SOC_ENUM("HPF corner freq Capture Switch", soc_cs42l52_enum[1]), /*5*/
> +
> +SOC_SINGLE("Ganged Ctl Capture Switch", CODEC_CS42L52_ADC_MISC_CTL, 7, 1, 1), /* should be enabled init */

What does the comment mean here?

> +SOC_SINGLE("HP Analog Gain Playback Volume", CODEC_CS42L52_PB_CTL1, 5, 7, 0),

Just "Headphone Volume"?

> +SOC_SINGLE("Playback B=A Volume Playback Switch", CODEC_CS42L52_PB_CTL1, 4, 1, 0), /*10*/ /*should be enabled init*/ 

If this is the mute for the headphone volume it should have the same
name but with Switch instead of Volume so applications like alsamixer
can display the two together.

> +static int soc_cs42l52_add_controls(struct snd_soc_codec *codec)
> +{
> +	int i,ret = 0;
> +
> +	for(i = 0; i < ARRAY_SIZE(soc_cs42l52_controls); i++)
> +	{
> +		ret = snd_ctl_add(codec->card,
> +			snd_soc_cnew(&soc_cs42l52_controls[i], codec, NULL));
> +		if(ret < 0)
> +		{
> +			SOCDBG("add cs42l52 controls failed\n");
> +			break;

Replace this with snd_soc_add_controls().

> +	/* Sum switches */
> +	SND_SOC_DAPM_PGA("AIN1A Switch", CODEC_CS42L52_ADC_PGA_A, 0, 0, NULL, 0),

Without having checked the datasheet I *suspect* most of these switches
should be modelled as inputs to a mixer (possibly one with no power
control of its own)?  The "Switch" in the name certainly looks very out
of place in the name of a DAPM control.

> +	/* Output path */
> +	SND_SOC_DAPM_MUX("Passthrough Left Playback Switch", SND_SOC_NOPM, 0, 0, &cs42l52_hpa_mux),

Normally "Bypass" instead of "Passthrough".

> +	/* Mic Bias */
> +        {"Mic Bias Capture Switch", "On", "PGA MICA"},
> +        {"Mic Bias Capture Switch", "On", "PGA MICB"},
> +	{"Mic-Bias", NULL, "Mic Bias Capture Switch"},
> +	{"Mic-Bias", NULL, "Mic Bias Capture Switch"},
> +        {"ADC Mux Left Capture Switch",  "PGA", "Mic-Bias"},
> +        {"ADC Mux Right Capture Switch", "PGA", "Mic-Bias"},
> +        {"Passthrough Left Playback Switch",  "On", "Mic-Bias"},
> +        {"Passthrough Right Playback Switch", "On", "Mic-Bias"},

This looks odd - normally the microphone bias would be connected to the
microphone input externally and would not be hooked up as an input
wihtin the CODEC?

> +	/* terminator */
> +        {NULL, NULL, NULL},

Not needed any more (I'm vaugely surprised this doesn't generate an
error).

> +#define SOC_CS42L52_RATES ( SNDRV_PCM_RATE_8000  | SNDRV_PCM_RATE_11025 | \
> +                            SNDRV_PCM_RATE_16000 | SNDRV_PCM_RATE_22050 | \
> +                            SNDRV_PCM_RATE_32000 | SNDRV_PCM_RATE_44100 | \
> +                            SNDRV_PCM_RATE_48000 | SNDRV_PCM_RATE_88200 | \
> +                            SNDRV_PCM_RATE_96000 ) /*refer to cs42l52 datasheet page35*/

This is equivalent to SNDRV_PCM_RATE_8000_96000

> +/* #define CONFIG_MANUAL_CLK */

This might use some comments (or should possibly be deleted)?

> +	soc_cs42l52_set_bias_level(soc_codec, SND_SOC_BIAS_OFF);
> +	soc_codec->bias_level = SND_SOC_BIAS_STANDBY;
> +	schedule_delayed_work(&soc_codec->delayed_work, msecs_to_jiffies(1000));
> +	soc_cs42l52_required_setup(soc_codec);

A comment about what the delayed work is up to wouldn't hurt?

> +		/*set hp default volume*/
> +		soc_cs42l52_write(soc_codec, CODEC_CS42L52_HPA_VOL, DEFAULT_HP_VOL);
> +		soc_cs42l52_write(soc_codec, CODEC_CS42L52_HPB_VOL, DEFAULT_HP_VOL);

For pretty much all the configuration you're doing here just let
userspace set things up, there's no telling what's an appropriate
setting on any given system and presumably the chip designers made a
reasonable effort to ensure that the default state of the chip is at
least not harmful.

> +#ifdef CONFIG_MANUAL_CLK
> +		soc_cs42l52_write(soc_codec, CODEC_CS42L52_CLK_CTL, 
> +				(soc_cs42l52_read(soc_codec, CODEC_CS42L52_CLK_CTL) 
> +				 & ~CLK_CTL_AUTODECT_ENABLE));

Pass this configuration in as platform data, this will allow multiple
boards to build from the same kernel.

> +	ret = snd_soc_init_card(soc_dev);
> +	if(ret)
> +	{
> +		SOCERR("add snd card failed\n");
> +		goto card_err;
> +	}

Please develop against

  git://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound-2.6.git for-2.6.35

(or whatever the latest kernel version branch is at any given time.)

You should also be calling snd_soc_register_codec() and
snd_soc_register_dai() from this function.

> +#if defined (CONFIG_I2C) || defined (CONFIG_I2C_MODULE)

No need for the ifdefs if you only support I2C, the ifdefs are used by
drivers that have multiple control interfaces (typically I2C plus SPI).

> +	}
> +
> +
> +	return ret;
> +}
> +
> +static int cs42l52_i2c_remove(struct i2c_client *client)
> +{
> +	struct snd_soc_codec *soc_codec;
> + 	if(client)
> +	{
> +		soc_codec = i2c_get_clientdata(client);
> +		if(soc_codec)
> + 	        	kfree(soc_codec->reg_cache);
> +	}
> +        return 0;
> +}

This should be unregistering from ASoC as well, or refusing to remove if
not implemented.

> +	case SND_SOC_DAIFMT_DSP_B:
> +		SOCINF("unsupported format\n");
> +		ret = -EINVAL;
> +		goto done;
> +	default:
> +		SOCINF("invaild format\n");
> +		ret = -EINVAL;
> +		goto done;

Could just squash these together.

> +	case SND_SOC_DAIFMT_IB_IF:
> +		SOCDBG("codec dai fmt inversed sclk\n");
> +		iface |= IFACE_CTL1_INV_SCLK;
> +		break;
> +	case SND_SOC_DAIFMT_IB_NF:
> +		iface |= IFACE_CTL1_INV_SCLK;
> +		break;

It looks like the hardware only supports the inversion of one of the
clocks so only two of the clock polarity combinations should be
supported?

> +	info->format = iface;

I'd really expect to see interaction with the hardware here?

> +	/* 96k */
> +	{12288000, 96000, CLK_CTL_S_DS_MODE, CLK_CTL_NOT_32K, CLK_CTL_NOT_27M, CLK_CTL_RATIO_128, 0},
> +	{18432000, 96000, CLK_CTL_S_DS_MODE, CLK_CTL_NOT_32K, CLK_CTL_NOT_27M, CLK_CTL_RATIO_128, 0},/*29*/
> +	{12000000, 96000, CLK_CTL_S_DS_MODE, CLK_CTL_NOT_32K, CLK_CTL_NOT_27M, CLK_CTL_RATIO_125, 0},
> +	{24000000, 96000, CLK_CTL_S_DS_MODE, CLK_CTL_NOT_32K, CLK_CTL_NOT_27M, CLK_CTL_RATIO_125, 1},

If you're feeling adventurous you should be able to use this data to set
up constraints for the DAI in the startup() callback so only sample
rates supported from the system clock can be used by userspace.

> +static int soc_cs42l52_pcm_hw_params(struct snd_pcm_substream *substream,
> +				     struct snd_pcm_hw_params *params,
> +				     struct snd_soc_dai *dai)
> +{
> +	int ret = 0;
> +	struct snd_soc_pcm_runtime *rtd = substream->private_data;
> +	struct snd_soc_device *soc_dev = rtd->socdev;
> +	struct snd_soc_codec *soc_codec = soc_dev->card->codec;
> +	struct soc_codec_cs42l52 *info = (struct soc_codec_cs42l52*)soc_codec->private_data;
> +
> +	u32 clk = 0;
> +	int index = soc_cs42l52_get_clk(info->sysclk, params_rate(params));

> +static int soc_cs42l52_suspend(struct platform_device *pdev, pm_message_t state)
> +{
> +	struct snd_soc_device *soc_dev = (struct snd_soc_device*)platform_get_drvdata(pdev);
> +	struct snd_soc_codec *soc_codec = soc_dev->card->codec;
> +	
> +	soc_cs42l52_write(soc_codec, CODEC_CS42L52_PWCTL1, PWCTL1_PDN_CODEC);
> +	soc_cs42l52_set_bias_level(soc_codec, SND_SOC_BIAS_OFF);

I'd expect that set_bias_level() would do what the explict write does?

> +#if defined (CONFIG_I2C) || defined (CONFIG_I2C_MODULE)
> +	if(soc_codec_data->i2c_addr)
> +	{
> +		ret = i2c_add_driver(&cs42l52_i2c_drv);
> +		
> +		if(ret)
> +		{
> +			SOCERR("register i2c driver failed\n");
> +			goto out;
> +			
> +		}
> +		SOCINF("Cirrus Logic ALSA SoC codec cs42l52 driver verison 1.2 2/2010\n");
> +		return ret;	
> +	}

This should be changed to use standard device model registration - see

   http://git.kernel.org/?p=linux/kernel/git/broonie/sound-2.6.git;a=commit;h=5998102b9095fdb7c67755812038612afea315c5

for an example of doing the conversion.

> +struct soc_codec_cs42l52_data{
> +	u_short i2c_addr;
> +};

With the device model stuff this won't be needed any more.

> +/*
> + *CS42L52 internal registers
> + */
> +#define CODEC_CS42L52_CHIP	0x01
> +#define CHIP_ID   		0xE0
> +#define CHIP_ID_MASK		0xF8

Many of the register names and other definitions for register fields and
values need to be namespace to avoid collisions.

Re: [PATCH] ASoC: Add support for Cirrus Logic CS42L52 LowpowerStereo Codec

[Austin, Brian]

On Feb 25, 2010, at 6:53 PM, "Mark Brown" <broonie@opensource.wolfsonmicro.com 
 > wrote:

> On Thu, Feb 25, 2010 at 06:07:35PM -0600, Brian wrote:
>
> This is a quick first pass review only.  It'd be best if you could
> include a Signed-off-by line in submissions so that they can be merged
> if they're OK, even if you expect problems you may be surprised!
>
> Overall this looks fairly good - there's a lot of comments below but  
> the
> vast majority of them are stylistic issues or needing to update to
> current APIs rather than major structural problems with the code.  The
> main thing I don't really follow is the register access code, there's
> quite a bit of stuff going on in there.
>
>> + * published by the Free Software Foundation.
>> + * Revision history
>> + * Nov 2007  Initial version.
>> + * Oct 2008  Updated to 2.6.26
>> + * Feb 2010  Updated to latest asoc git tree
>
> Please remove the changelog, once things are merged git takes care of
> that.
>
>> + */
>> +#define DEBUG
>
> Not by default in mainline :)
>
>> +#ifdef DEBUG
>> +#define SOCDBG(fmt, arg...)    printk(KERN_ERR "%s: %s() " fmt,  
>> SOC_CS42L52_NAME, __FUNCTION__, ##arg)
>> +#else
>> +#define SOCDBG(fmt, arg...)
>> +#endif
>> +#define SOCINF(fmt, args...)    printk(KERN_INFO "%s: " fmt,  
>> SOC_CS42L52_NAME,  ##args)
>> +#define SOCERR(fmt, args...)    printk(KERN_ERR "%s: " fmt,  
>> SOC_CS42L52_NAME,  ##args)
>
> Please convert the driver to use the standard dev_ and pr_ macros for
> this (where possible the dev_ ones are better).
>
>> +static int soc_cs42l52_set_bias_level(struct snd_soc_codec *codec,
>> +                enum snd_soc_bias_level level);
>> +static int soc_cs42l52_pcm_hw_params(struct snd_pcm_substream  
>> *substream,
>> +                            struct snd_pcm_hw_params *params,
>> +                struct snd_soc_dai *dai);
>> +static int soc_cs42l52_set_sysclk(struct snd_soc_dai *codec_dai,
>> +                        int clk_id, u_int freq, int dir);
>> +static int soc_cs42l52_digital_mute(struct snd_soc_dai *dai, int  
>> mute);
>> +static int soc_cs42l52_set_fmt(struct snd_soc_dai *codec_dai,
>> +                        u_int fmt);
>> +
>> +static unsigned int soc_cs42l52_read(struct snd_soc_codec *codec,
>> +                        u_int reg);
>
> Please use 'unsigned int' rather than 'u_int' for consistency with the
> rest of ASoC.  It's a bit surprising that you need the forward
> declarations at all but they do no harm.
>
>> +/**
>> + * snd_soc_get_volsw - single mixer get callback
>> + * @kcontrol: mixer control
>> + * @uinfo: control element information
>> + *
>> + * Callback to get the value of a single mixer control.
>> + *
>> + * Returns 0 for success.
>> + */
>> +int snd_soc_cs42l5x_get_volsw(struct snd_kcontrol *kcontrol,
>> +    struct snd_ctl_elem_value *ucontrol)
>> +{
>> +    struct snd_soc_codec *codec = snd_kcontrol_chip(kcontrol);
>> +    int reg = kcontrol->private_value & 0xff;
>
> The indentation looks wrong - should be tabs for indenting the code
> blocks.  scripts/checkpatch.pl is a pretty good check for this sort of
> stuff, I'll not mention other issues.
>
>> +    int shift = (kcontrol->private_value >> 8) & 0x0f;
>> +    int rshift = (kcontrol->private_value >> 12) & 0x0f;
>> +    int max = (kcontrol->private_value >> 16) & 0xff;
>> +    int mask = (1 << fls(max)) - 1;
>> +    int min = (kcontrol->private_value >> 24) & 0xff;
>> +
>> +    ucontrol->value.integer.value[0] =
>> +    ((snd_soc_read(codec, reg) >> shift) - min) & mask;
>> +    if (shift != rshift)
>> +    ucontrol->value.integer.value[1] =
>> +        ((snd_soc_read(codec, reg) >> rshift) - min) & mask;
>
> This and most of the other custom control stuff looks an awful lot  
> like
> it should be done by adding a SND_SOC_DOUBLE_SIGNED() or something  
> along
> the lines of the existing _S8() but I didn't read entirely closely.  I
> was speaking to someone on IRC only the other day who needed some of
> those types for a different device.
>
> These should also be updated to use struct soc_mixer_control for the
> private value rather than shifting everything into the private  
> value, it
> allows wider ranges and is less error prone.
>
>> +static const u8 soc_cs42l52_reg_default[] = {
>> +    0x00, 0xE0, 0x01, 0x07, 0x05, /*4*/
>> +    0xa0, 0x00, 0x00, 0x81, /*8*/
>
> Very nitpicky but you're using a mix of upper and lower case for the
> letters in the hex digits.
>
>> +static inline int soc_cs42l52_read_reg_cache(struct snd_soc_codec  
>> *codec,
>> +        u_int reg)
>> +{
>> +    u8 *cache = codec->reg_cache;
>> +
>> +    return reg > SOC_CS42L52_REG_NUM ? -EINVAL : cache[reg];
>> +}
>
> You should be able to replace all your register I/O code with use of
> soc-cache.c with 8 bit data, 8 bit register.  Except...
>
>> +    if(info->flags & SOC_CS42L52_ALL_IN_ONE)
>> +    {
>> +        for(i = 0; i < codec->num_dai; i++)
>> +        {
>> +            if(info->machine_handler)
>> +                info->machine_handler(i);
>
> ...I'm not sure what's going on here but it could really use an
> explanation?  I'm not sure what ALL_IN_ONE is supposed to do, or what
> the machine_handler() function is for.
>
>> +        if(info->flags & SOC_CS42L52_CHIP_SWICTH)
>> +        {
>> +            num = info->flags & SOC_CS42L52_CHIP_MASK;
>> +            if(info->machine_handler)
>> +                                info->machine_handler(num);
>> +        }
>
> This too.  There's a lot of unusual code using this machine handler
> function.  I suspect this can all use ASoC standard stuff.
>
>> +static inline int soc_cs42l52_get_revison(struct snd_soc_codec  
>> *codec)
>> +{
>> +    u8 data;
>> +        u8 addr;
>> +    int num, ret = 0;
>
>> +    struct soc_codec_cs42l52 *info = (struct soc_codec_cs42l52*) 
>> codec->private_data;
>
> No need to cast away from void (quite a few examples of this in the
> code).
>
>> +    return ret < 0 ? ret : data;
>
> I'm not a big fan of the ternery operator...
>
>> +static unsigned int soc_cs42l52_read(struct snd_soc_codec *codec,
>> +        u_int reg)
>> +{
>> +    u8 data;
>> +    u8 addr;
>> +        int i, ret = 0;
>> +        struct soc_codec_cs42l52 *info = (struct  
>> soc_codec_cs42l52*)codec->private_data;
>> +#ifndef CONFIG_CS42L52_DEBUG
>> +    if(reg == CODEC_CS42L52_SPK_STATUS)
>> +    {
>> +#endif
>
> This really shouldn't be ifdefed.  If you need to handle registers  
> that
> report status from the device with the soc-cache stuff use the
> volatile_register() callback in the CODEC, any registers that need a
> CODEC read will call through to the hardware rather than using the
> cache.
>
>> +static const char *cs42l52_adc_mux[] = {"AIN1", "AIN2", "AIN3",  
>> "AIN4", "PGA"};
>> +static const char *cs42l52_mic_mux[] = {"MIC1", "MIC2"};
>> +static const char *cs42l52_stereo_mux[] = {"Mono", "Stereo"};
>> +static const char *cs42l52_off[] = {"On", "Off"};
>> +static const char *cs42l52_hpmux[] = {"Off", "On"};
>> +
>> +static const struct soc_enum soc_cs42l52_enum[] = {
>> +SOC_ENUM_DOUBLE(CODEC_CS42L52_ANALOG_HPF_CTL, 4, 6, 2,  
>> cs42l52_hpf_freeze), /*0*/
>> +SOC_ENUM_SINGLE(CODEC_CS42L52_ADC_HPF_FREQ, 0, 4,  
>> cs42l52_hpf_corner_freq),
>
> Don't use an array of enums, define individual variables for them.   
> Some
> drivers use the arrays for historical reasons - they're quite  
> painful to
> work with if you ever want to change anything in the middle and we've
> found a few off by one errors in the referencing into the arrays  
> before
> :/.
>
>> +static const struct snd_kcontrol_new soc_cs42l52_controls[] = {
>> +
>> +SOC_ENUM("Mic VA Capture Switch", soc_cs42l52_enum[18]), /*0*/
>
> enums shouldn't be called Switch - Switch has a specific meaning to  
> ALSA
> (see Documentation/sound/alsa/ControlNames.txt, there's quite a few
> other controls could do with having the components of the name  
> reordered
> and various other things following that).  Normally just omit the '  
> Switch'
> from the name.
>
>> +SOC_DOUBLE("Analog SR Capture Switch",  
>> CODEC_CS42L52_ANALOG_HPF_CTL, 1, 3, 1, 1),
>> +SOC_DOUBLE("Analog ZC Capture Switch",  
>> CODEC_CS42L52_ANALOG_HPF_CTL, 0, 2, 1, 1),
>
> This'd normally be 'Capture ZC Switch' if it's for zero cross.
>
>> +SOC_ENUM("HPF corner freq Capture Switch", soc_cs42l52_enum[1]), / 
>> *5*/
>> +
>> +SOC_SINGLE("Ganged Ctl Capture Switch",  
>> CODEC_CS42L52_ADC_MISC_CTL, 7, 1, 1), /* should be enabled init */
>
> What does the comment mean here?
>
>> +SOC_SINGLE("HP Analog Gain Playback Volume",  
>> CODEC_CS42L52_PB_CTL1, 5, 7, 0),
>
> Just "Headphone Volume"?
>
>> +SOC_SINGLE("Playback B=A Volume Playback Switch",  
>> CODEC_CS42L52_PB_CTL1, 4, 1, 0), /*10*/ /*should be enabled init*/
>
> If this is the mute for the headphone volume it should have the same
> name but with Switch instead of Volume so applications like alsamixer
> can display the two together.
>
>> +static int soc_cs42l52_add_controls(struct snd_soc_codec *codec)
>> +{
>> +    int i,ret = 0;
>> +
>> +    for(i = 0; i < ARRAY_SIZE(soc_cs42l52_controls); i++)
>> +    {
>> +        ret = snd_ctl_add(codec->card,
>> +            snd_soc_cnew(&soc_cs42l52_controls[i], codec, NULL));
>> +        if(ret < 0)
>> +        {
>> +            SOCDBG("add cs42l52 controls failed\n");
>> +            break;
>
> Replace this with snd_soc_add_controls().
>
>> +    /* Sum switches */
>> +    SND_SOC_DAPM_PGA("AIN1A Switch", CODEC_CS42L52_ADC_PGA_A, 0,  
>> 0, NULL, 0),
>
> Without having checked the datasheet I *suspect* most of these  
> switches
> should be modelled as inputs to a mixer (possibly one with no power
> control of its own)?  The "Switch" in the name certainly looks very  
> out
> of place in the name of a DAPM control.
>
>> +    /* Output path */
>> +    SND_SOC_DAPM_MUX("Passthrough Left Playback Switch",  
>> SND_SOC_NOPM, 0, 0, &cs42l52_hpa_mux),
>
> Normally "Bypass" instead of "Passthrough".
>
>> +    /* Mic Bias */
>> +        {"Mic Bias Capture Switch", "On", "PGA MICA"},
>> +        {"Mic Bias Capture Switch", "On", "PGA MICB"},
>> +    {"Mic-Bias", NULL, "Mic Bias Capture Switch"},
>> +    {"Mic-Bias", NULL, "Mic Bias Capture Switch"},
>> +        {"ADC Mux Left Capture Switch",  "PGA", "Mic-Bias"},
>> +        {"ADC Mux Right Capture Switch", "PGA", "Mic-Bias"},
>> +        {"Passthrough Left Playback Switch",  "On", "Mic-Bias"},
>> +        {"Passthrough Right Playback Switch", "On", "Mic-Bias"},
>
> This looks odd - normally the microphone bias would be connected to  
> the
> microphone input externally and would not be hooked up as an input
> wihtin the CODEC?
>
>> +    /* terminator */
>> +        {NULL, NULL, NULL},
>
> Not needed any more (I'm vaugely surprised this doesn't generate an
> error).
>
>> +#define SOC_CS42L52_RATES ( SNDRV_PCM_RATE_8000  |  
>> SNDRV_PCM_RATE_11025 | \
>> +                            SNDRV_PCM_RATE_16000 |  
>> SNDRV_PCM_RATE_22050 | \
>> +                            SNDRV_PCM_RATE_32000 |  
>> SNDRV_PCM_RATE_44100 | \
>> +                            SNDRV_PCM_RATE_48000 |  
>> SNDRV_PCM_RATE_88200 | \
>> +                            SNDRV_PCM_RATE_96000 ) /*refer to  
>> cs42l52 datasheet page35*/
>
> This is equivalent to SNDRV_PCM_RATE_8000_96000
>
>> +/* #define CONFIG_MANUAL_CLK */
>
> This might use some comments (or should possibly be deleted)?
>
>> +    soc_cs42l52_set_bias_level(soc_codec, SND_SOC_BIAS_OFF);
>> +    soc_codec->bias_level = SND_SOC_BIAS_STANDBY;
>> +    schedule_delayed_work(&soc_codec->delayed_work,  
>> msecs_to_jiffies(1000));
>> +    soc_cs42l52_required_setup(soc_codec);
>
> A comment about what the delayed work is up to wouldn't hurt?
>
>> +        /*set hp default volume*/
>> +        soc_cs42l52_write(soc_codec, CODEC_CS42L52_HPA_VOL,  
>> DEFAULT_HP_VOL);
>> +        soc_cs42l52_write(soc_codec, CODEC_CS42L52_HPB_VOL,  
>> DEFAULT_HP_VOL);
>
> For pretty much all the configuration you're doing here just let
> userspace set things up, there's no telling what's an appropriate
> setting on any given system and presumably the chip designers made a
> reasonable effort to ensure that the default state of the chip is at
> least not harmful.
>
>> +#ifdef CONFIG_MANUAL_CLK
>> +        soc_cs42l52_write(soc_codec, CODEC_CS42L52_CLK_CTL,
>> +                (soc_cs42l52_read(soc_codec, CODEC_CS42L52_CLK_CTL)
>> +                 & ~CLK_CTL_AUTODECT_ENABLE));
>
> Pass this configuration in as platform data, this will allow multiple
> boards to build from the same kernel.
>
>> +    ret = snd_soc_init_card(soc_dev);
>> +    if(ret)
>> +    {
>> +        SOCERR("add snd card failed\n");
>> +        goto card_err;
>> +    }
>
> Please develop against
>
>  git://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound-2.6.git  
> for-2.6.35
>
> (or whatever the latest kernel version branch is at any given time.)
>
> You should also be calling snd_soc_register_codec() and
> snd_soc_register_dai() from this function.
>
>> +#if defined (CONFIG_I2C) || defined (CONFIG_I2C_MODULE)
>
> No need for the ifdefs if you only support I2C, the ifdefs are used by
> drivers that have multiple control interfaces (typically I2C plus  
> SPI).
>
>> +    }
>> +
>> +
>> +    return ret;
>> +}
>> +
>> +static int cs42l52_i2c_remove(struct i2c_client *client)
>> +{
>> +    struct snd_soc_codec *soc_codec;
>> +    if(client)
>> +    {
>> +        soc_codec = i2c_get_clientdata(client);
>> +        if(soc_codec)
>> +                kfree(soc_codec->reg_cache);
>> +    }
>> +        return 0;
>> +}
>
> This should be unregistering from ASoC as well, or refusing to  
> remove if
> not implemented.
>
>> +    case SND_SOC_DAIFMT_DSP_B:
>> +        SOCINF("unsupported format\n");
>> +        ret = -EINVAL;
>> +        goto done;
>> +    default:
>> +        SOCINF("invaild format\n");
>> +        ret = -EINVAL;
>> +        goto done;
>
> Could just squash these together.
>
>> +    case SND_SOC_DAIFMT_IB_IF:
>> +        SOCDBG("codec dai fmt inversed sclk\n");
>> +        iface |= IFACE_CTL1_INV_SCLK;
>> +        break;
>> +    case SND_SOC_DAIFMT_IB_NF:
>> +        iface |= IFACE_CTL1_INV_SCLK;
>> +        break;
>
> It looks like the hardware only supports the inversion of one of the
> clocks so only two of the clock polarity combinations should be
> supported?
>
>> +    info->format = iface;
>
> I'd really expect to see interaction with the hardware here?
>
>> +    /* 96k */
>> +    {12288000, 96000, CLK_CTL_S_DS_MODE, CLK_CTL_NOT_32K,  
>> CLK_CTL_NOT_27M, CLK_CTL_RATIO_128, 0},
>> +    {18432000, 96000, CLK_CTL_S_DS_MODE, CLK_CTL_NOT_32K,  
>> CLK_CTL_NOT_27M, CLK_CTL_RATIO_128, 0},/*29*/
>> +    {12000000, 96000, CLK_CTL_S_DS_MODE, CLK_CTL_NOT_32K,  
>> CLK_CTL_NOT_27M, CLK_CTL_RATIO_125, 0},
>> +    {24000000, 96000, CLK_CTL_S_DS_MODE, CLK_CTL_NOT_32K,  
>> CLK_CTL_NOT_27M, CLK_CTL_RATIO_125, 1},
>
> If you're feeling adventurous you should be able to use this data to  
> set
> up constraints for the DAI in the startup() callback so only sample
> rates supported from the system clock can be used by userspace.
>
>> +static int soc_cs42l52_pcm_hw_params(struct snd_pcm_substream  
>> *substream,
>> +                     struct snd_pcm_hw_params *params,
>> +                     struct snd_soc_dai *dai)
>> +{
>> +    int ret = 0;
>> +    struct snd_soc_pcm_runtime *rtd = substream->private_data;
>> +    struct snd_soc_device *soc_dev = rtd->socdev;
>> +    struct snd_soc_codec *soc_codec = soc_dev->card->codec;
>> +    struct soc_codec_cs42l52 *info = (struct soc_codec_cs42l52*) 
>> soc_codec->private_data;
>> +
>> +    u32 clk = 0;
>> +    int index = soc_cs42l52_get_clk(info->sysclk, params_rate 
>> (params));
>
>> +static int soc_cs42l52_suspend(struct platform_device *pdev,  
>> pm_message_t state)
>> +{
>> +    struct snd_soc_device *soc_dev = (struct snd_soc_device*) 
>> platform_get_drvdata(pdev);
>> +    struct snd_soc_codec *soc_codec = soc_dev->card->codec;
>> +
>> +    soc_cs42l52_write(soc_codec, CODEC_CS42L52_PWCTL1,  
>> PWCTL1_PDN_CODEC);
>> +    soc_cs42l52_set_bias_level(soc_codec, SND_SOC_BIAS_OFF);
>
> I'd expect that set_bias_level() would do what the explict write does?
>
>> +#if defined (CONFIG_I2C) || defined (CONFIG_I2C_MODULE)
>> +    if(soc_codec_data->i2c_addr)
>> +    {
>> +        ret = i2c_add_driver(&cs42l52_i2c_drv);
>> +
>> +        if(ret)
>> +        {
>> +            SOCERR("register i2c driver failed\n");
>> +            goto out;
>> +
>> +        }
>> +        SOCINF("Cirrus Logic ALSA SoC codec cs42l52 driver verison 1.2 2/2010 
>> \n");
>> +        return ret;
>> +    }
>
> This should be changed to use standard device model registration - see
>
>   http://git.kernel.org/?p=linux/kernel/git/broonie/sound-2.6.git;a=commit;h=5998102b9095fdb7c67755812038612afea315c5
>
> for an example of doing the conversion.
>
>> +struct soc_codec_cs42l52_data{
>> +    u_short i2c_addr;
>> +};
>
> With the device model stuff this won't be needed any more.
>
>> +/*
>> + *CS42L52 internal registers
>> + */
>> +#define CODEC_CS42L52_CHIP    0x01
>> +#define CHIP_ID           0xE0
>> +#define CHIP_ID_MASK        0xF8
>
> Many of the register names and other definitions for register fields  
> and
> values need to be namespace to avoid collisions.
>

Thanks alot for the feedback, I'll start fixing it up tomorrow.
Should I send the smdk2443 driver, or wait till this is fixed up?

Thanks again
Brian

Re: [PATCH] ASoC: Add support for Cirrus Logic CS42L52 LowpowerStereo Codec

[Mark Brown]

On Thu, Feb 25, 2010 at 07:59:29PM -0600, Austin, Brian wrote:

> Thanks alot for the feedback, I'll start fixing it up tomorrow.
> Should I send the smdk2443 driver, or wait till this is fixed up?

May as well send it, there should be relatively little coupling to the
CODEC driver.  Is this a plugin module for the SMDK or does it need
flying wires or mods to get it hooked up?

Re: [PATCH] ASoC: Add support for Cirrus Logic CS42L52 LowpowerStereo Codec

[Brian Austin]

On Fri, 2010-02-26 at 10:12 +0000, Mark Brown wrote:
> On Thu, Feb 25, 2010 at 07:59:29PM -0600, Austin, Brian wrote:
> 
> > Thanks alot for the feedback, I'll start fixing it up tomorrow.
> > Should I send the smdk2443 driver, or wait till this is fixed up?
> 
> May as well send it, there should be relatively little coupling to the
> CODEC driver.  Is this a plugin module for the SMDK or does it need
> flying wires or mods to get it hooked up?
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
> 

It requires wiring onto the I2C Connector and Dropping lines on the I2S
as well.

The device is wired correctly, just haven't gotten it to work with the
newer kernels.

Re: [PATCH] ASoC: Add support for Cirrus Logic CS42L52 LowpowerStereo Codec

[Mark Brown]

On Fri, Feb 26, 2010 at 11:18:00AM -0600, Brian Austin wrote:

> The device is wired correctly, just haven't gotten it to work with the
> newer kernels.

Sure, I was more wondering if this is something for mainline or not - if
it's a flying wire style setup then that doesn't generally get merged.