Date: Mon, 24 Feb 2014 23:47:09 +0800
From: Nicolin Chen <Guangyu.Chen@freescale.com>
To: Mark Brown <broonie@kernel.org>
CC: <brian.austin@cirrus.com>, <Paul.Handrigan@cirrus.com>,
        <robh+dt@kernel.org>, <pawel.moll@arm.com>, <mark.rutland@arm.com>,
        <ijc+devicetree@hellion.org.uk>, <galak@codeaurora.org>,
        <rob@landley.net>, <lgirdwood@gmail.com>, <grant.likely@linaro.org>,
        <devicetree@vger.kernel.org>, <linux-doc@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>, <alsa-devel@alsa-project.org>
Subject: Re: [PATCH] ASoC: cs42888: Add codec driver support
Message-ID: <20140224154708.GA6132@MrMyself>
References: <1393224929-7555-1-git-send-email-Guangyu.Chen@freescale.com>
 <20140224113011.GE25940@sirena.org.uk>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <20140224113011.GE25940@sirena.org.uk>
User-Agent: Mutt/1.5.21 (2010-09-15)
Sender: linux-kernel-owner@vger.kernel.org

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?


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/