Re: [PATCH] ASoC: cs42888: Add codec driver support

[Nicolin Chen <Guangyu.Chen@freescale.com>]

On Mon, Feb 24, 2014 at 08:30:11PM +0900, Mark Brown wrote:
> On Mon, Feb 24, 2014 at 02:55:29PM +0800, Nicolin Chen wrote:
> > This patch adds support for the Cirrus Logic CS42888 Audio CODEC that
> > has four 24-bit A/D and eight 24-bit D/A converters.
> 
> Looks generally good, some fairly small nits below.

I'll revise all of them.

Thank you.
Nicolin

----

> 
> > [ CS42888 supports both I2C and SPI control ports. As initial patch,
> >   this patch only adds the support for I2C. ]
> 
> >  5 files changed, 795 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/sound/cs42888.txt
> >  create mode 100644 sound/soc/codecs/cs42888.c
> >  create mode 100644 sound/soc/codecs/cs42888.h
> 
> Given that we're starting to split out separate bus drivers for the I2C
> and SPI CODECs (look at the recent submissions from Lars-Peter) it'd be
> good to start this off with a separate bus driver for I2C even if the
> SPI one is still to be done - that way the Kconfig stuff for machine
> drivers is all in place and doesn't need updating.
> 
> > +  - clocks : phandle to the clock source for MCLK
> > +
> > +  - clock-names : must contain "mclk".
> 
> These should really be lists though there's only one documented element
> so it's purely a documentation update.
> 
> > +	/* Disable auto-mute */
> > +	regmap_update_bits(cs42888->regmap, CS42888_TXCTL,
> > +			   CS42888_TXCTL_AMUTE | CS42888_TXCTL_DAC_SZC_MASK,
> > +			   CS42888_TXCTL_DAC_SZC_SR);
> 
> Does this interfere with the manual mute controls or is it a separate
> thing?  If it plays nicely with the manual controls it's probably better
> to leave it enabled since it improves performance in some benchmarks
> (that's why hardware tends to have the feature).
> 
> > +	/*
> > +	 * We haven't marked the chip revision as volatile due to
> > +	 * sharing a register with the right input volume; explicitly
> > +	 * bypass the cache to read it.
> > +	 */
> > +	regcache_cache_bypass(cs42888->regmap, true);
> 
> The other option here is to just not provide a default so that the first
> time it's read it goes to hardware.  It doesn't make much difference
> either way though.
> 
> > +static int cs42888_i2c_remove(struct i2c_client *i2c_client)
> > +{
> > +	snd_soc_unregister_codec(&i2c_client->dev);
> > +	return 0;
> > +}
> 
> The driver ought to disable runtime PM, the clock and the regulators here.
> 
> > +	/*
> > +	 * In case the device was put to hard reset during sleep,
> > +	 * we need to wait 500ns here before any I2C communication
> > +	 */
> > +	mdelay(5);
> 
> Do we need 500ns or 5ms?  
> 
> > +	regcache_sync(cs42888->regmap);
> 
> Should really check the return value here.
> 
> > +	if (!IS_ERR(cs42888->clk))
> > +		clk_disable_unprepare(cs42888->clk);
> 
> Does the device work without MCLK?