[alsa-devel] [PATCH 3/3] ASoC: rt5677: Support DSP function

[Mark Brown]

On Mon, Jul 07, 2014 at 03:37:01PM +0800, Oder Chiou wrote:

> +int rt5677_spi_write(u8 *txbuf, size_t len)
> +{
> +	static DEFINE_MUTEX(lock);
> +	int status;
> +
> +	mutex_lock(&lock);
> +
> +	status = spi_write(g_spi, txbuf, len);
> +
> +	mutex_unlock(&lock);

Since this lock is local to the function it doesn't do anything, the SPI
framework has its own locking which makes it reentrant.

> +int rt5677_spi_burst_write(u32 addr, const struct firmware *fw)
> +{
> +	u8 spi_cmd = 0x05;
> +	u8 *write_buf;
> +	unsigned int i, end, offset = 0;
> +
> +	write_buf = kmalloc(8 * 30 + 6, GFP_KERNEL);

Magic numbers!  You also need to be aware that memory allocated by
kmalloc() may not be contigous and while things using the SPI core DMA
handling ought to be able to cope not everything will be.

> +static int rt5677_spi_probe(struct spi_device *spi)
> +{
> +	g_spi = spi;
> +	return 0;
> +}

> +static int rt5677_spi_remove(struct spi_device *spi)
> +{
> +	return 0;
> +}

Delete empty functions.

> +static int __init rt5677_spi_init(void)
> +{
> +	return spi_register_driver(&rt5677_spi_driver);
> +}
> +module_init(rt5677_spi_init);

module_spi_driver()

> +static int rt5677_dsp_put(struct snd_kcontrol *kcontrol,
> +		struct snd_ctl_elem_value *ucontrol)
> +{
> +	struct snd_soc_codec *codec = snd_kcontrol_chip(kcontrol);
> +	struct rt5677_priv *rt5677 = snd_soc_codec_get_drvdata(codec);
> +
> +	if (ucontrol->value.integer.value[0] != 0 && rt5677->dsp_en == false) {
> +		rt5677->dsp_en = true;
> +
> +		regmap_update_bits(rt5677->regmap, RT5677_PWR_ANLG1,
> +			RT5677_LDO1_SEL_MASK, 0x0);
> +		regmap_update_bits(rt5677->regmap, RT5677_PWR_ANLG2,
> +			RT5677_PWR_LDO1, RT5677_PWR_LDO1);
> +		regmap_write(rt5677->regmap, RT5677_GLB_CLK2, 0x0080);
> +		regmap_write(rt5677->regmap, RT5677_PWR_DSP2, 0x07ff);
> +		regmap_write(rt5677->regmap, RT5677_PWR_DSP1, 0x07ff);

This doesn't seem joined up with the rest of the CODEC power management
which suggests there's going to be problems - for example LDO1_SEL_MASK
is also being managed by the existing set_bias_level().  What other
devices do is manage the power for the DSP using a DAPM widget, enabling
it when there's audio flowing through it.

> +	mutex_init(&rt5677->dsp_cmd_lock);
> +
> +	request_firmware_nowait(THIS_MODULE, FW_ACTION_HOTPLUG,
> +		RT5677_FIRMWARE1, codec->dev, GFP_KERNEL, codec,
> +		rt5677_fw1_loaded);
> +
> +	request_firmware_nowait(THIS_MODULE, FW_ACTION_HOTPLUG,
> +		RT5677_FIRMWARE2, codec->dev, GFP_KERNEL, codec,
> +		rt5677_fw2_loaded);

It would be nicer to be able to only request firmware when needed rather
than loading it once a boot time, that way we use a little less memory
and we'll always have the latest firmware rather than what happened to
be there when the system is booting.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://mailman.alsa-project.org/pipermail/alsa-devel/attachments/20140816/c4ff31ea/attachment.sig>