linux-kernel - Re: [PATCH] ASoC: cs42888: Add codec driver support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Mon, 24 Feb 2014 23:47:09 +0800
From:	Nicolin Chen <Guangyu.Chen@...escale.com>
To:	Mark Brown <broonie@...nel.org>
CC:	<brian.austin@...rus.com>, <Paul.Handrigan@...rus.com>,
	<robh+dt@...nel.org>, <pawel.moll@....com>, <mark.rutland@....com>,
	<ijc+devicetree@...lion.org.uk>, <galak@...eaurora.org>,
	<rob@...dley.net>, <lgirdwood@...il.com>,
	<grant.likely@...aro.org>, <devicetree@...r.kernel.org>,
	<linux-doc@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
	<alsa-devel@...a-project.org>
Subject: Re: [PATCH] ASoC: cs42888: Add codec driver support

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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/