Re: [PATCH] ASoC: CS4271 codec support

[Alexander <subaparts@yandex.ru>]

Dear Dimitris,

On Mon, 2011-01-17 at 10:30 +0000, Dimitris Papastamos wrote:
> On Mon, 2011-01-17 at 01:35 +0300, Alexander wrote:
> > +/* CS4271 default register settings, except auto-mute is off */
> > +static const u8 cs4271_dflt_reg[CS4271_NR_REGS] = {
> > +	0, 0, 0, CS4271_DACVOL_ATAPI_AL_BR, 0, 0, 0,
> > +	CS4271_MODE2_CPEN | CS4271_MODE2_PDN,
> > +};
> 
> I'd be better to leave these as defaults, and perform any initialization
> in the the cs4271_probe() function.

It's space optimized without affecting readability. I can rename it to
cs4271_init_reg[].

> 
> > +	/* Configure DAC */
> > +	val = cs4271_clk_tab[i].speed_mode;
> > +
> > +	if (cs4271->master)
> > +		val |= cs4271_clk_tab[i].mclk_master | CS4271_MODE1_MASTER;
> > +	else
> > +		val |= cs4271_clk_tab[i].mclk_slave;
> 
> This should ideally live in cs4271_set_dai_fmt().

It's space optimized for particular codec hardware without affecting
readability.

> 
> > +	switch (cs4271->mode) {
> > +	case SND_SOC_DAIFMT_LEFT_J:
> > +		val |= CS4271_MODE1_DAC_DIF_LJ;
> > +		break;
> > +	case SND_SOC_DAIFMT_I2S:
> > +		val |= CS4271_MODE1_DAC_DIF_I2S;
> > +		break;
> > +	default:
> > +		dev_err(codec->dev, "Invalid DAI format\n");
> > +		return -EINVAL;
> > +	}
> 
> Same here.

Same here.

> 
> > +/* CS4271 controls */
> > +static DECLARE_TLV_DB_SCALE(cs4271_dac_tlv, -12700, 100, 0);
> 
> Are you use this doesn't mute the DAC?  If so the the last parameter
> should be 1.

No I do not use this, some people asked for this last time I've tried to
submit this code. And no, this doesn't mute the DAC.

> 
> > +#ifdef CONFIG_PM
> > +static int cs4271_soc_suspend(struct snd_soc_codec *codec, pm_message_t mesg)
> > +{
> > +	/* Set power-down bit */
> > +	snd_soc_update_bits(codec, CS4271_MODE2, CS4271_MODE2_PDN,
> > +		CS4271_MODE2_PDN);
> > +
> > +	return 0;
> > +}
> > +#else
> > +#define cs4271_soc_suspend	NULL
> > +#endif /* CONFIG_PM */
> > +
> > +/* This function used also in codec probe function and not only for PM */
> > +static int cs4271_soc_resume(struct snd_soc_codec *codec)
> > +{
> > +	int ret;
> > +
> > +	/* Restore codec state */
> > +	ret = cs4271_write_cache(codec);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	/* then disable the power-down bit */
> > +	snd_soc_update_bits(codec, CS4271_MODE2, CS4271_MODE2_PDN, 0);
> > +
> > +	return 0;
> > +}
> 
> Consider setting the set_bias_level callback and performing the required
> work in there.  You can then trigger a change in the bias levels by
> calling your cs4271_set_bias_level() function from within your suspend()
> and resume() functions.
> 
> > +	/*
> > +	 * In case of I2C, chip address specified in board data.
> > +	 * So cache IO operations use 8 bit codec register address.
> > +	 * In case of SPI, chip address and register address
> > +	 * passed together as 16 bit value.
> > +	 * Anyway, register address is masked with 0xFF inside
> > +	 * soc-cache code.
> > +	 */
> 
> Have you tested your driver using both SPI and I2C?

Yes.

> 
> > +	ret = (cs4271->bus_type == SND_SOC_SPI) ? 16 : 8;
> 
> Please avoid using the ternary operator like this.

What's wrong with it?

> 
> > +	ret = snd_soc_codec_set_cache_io(codec, ret, 8, cs4271->bus_type);
> > +	if (ret) {
> > +		dev_err(codec->dev, "Failed to set cache I/O: %d\n", ret);
> > +		return ret;
> > +	}
> > +
> > +	ret = cs4271_soc_resume(codec);
> > +	if (ret < 0)
> > +		return ret;
> 
> This should also changed to a call to
> cs4271_set_bias_level(codec, SND_SOC_BIAS_STANDBY);
> 

There is no need for DAPM on hardware I consider. I can even remove PM
stuff. Someone may implement it later, if CS4271 would appear on
something except Cirrus dev. boards.

> Thanks,
> Dimitris
> 
> 

Thanks,
	Alexander.