Message-ID: <20140702150259.GC5541@saruman.home>
Date:	Wed, 2 Jul 2014 10:03:00 -0500
From:	Felipe Balbi <balbi@...com>
To:	Dan Murphy <dmurphy@...com>
CC:	<balbi@...com>, <linux-sound@...r.kernel.org>,
	<linux-kernel@...r.kernel.org>, <alsa-devel@...a-project.org>,
	<broonie@...nel.org>, <devicetree@...r.kernel.org>
Subject: Re: [PATCH v3] ASoC: tas2552: Support TI TAS2552 Amplifier

Hi,

On Wed, Jul 02, 2014 at 09:53:10AM -0500, Dan Murphy wrote:
> Felipe
> Thanks for the review
> 
> On 07/02/2014 09:10 AM, Felipe Balbi wrote:
> > Hi,
> >
> > On Wed, Jul 02, 2014 at 08:30:52AM -0500, Dan Murphy wrote:
> >> Support the TI TAS2552 Class D amplifier.
> >>
> >> The TAS2552 is a high efficiency Class-D audio
> >> power amplifier with advanced battery current
> >> management and an integrated Class-G boost
> >> The device constantly measures the
> >> current and voltage across the load and provides a
> >> digital stream of this information.
> >>
> >> Signed-off-by: Dan Murphy <dmurphy@...com>
> >> ---
> >>
> >> v3 - Updated bindings doc per comments, rearranged probe pdata vs 
> >> np check - https://patchwork.kernel.org/patch/4453481/
> >>
> >>  .../devicetree/bindings/sound/tas2552.txt          |   22 +
> >>  include/sound/tas2552-plat.h                       |   25 ++
> >>  sound/soc/codecs/Kconfig                           |    5 +
> >>  sound/soc/codecs/Makefile                          |    2 +
> >>  sound/soc/codecs/tas2552.c                         |  463 ++++++++++++++++++++
> >>  sound/soc/codecs/tas2552.h                         |   75 ++++
> >>  6 files changed, 592 insertions(+)
> >>  create mode 100644 Documentation/devicetree/bindings/sound/tas2552.txt
> >>  create mode 100644 include/sound/tas2552-plat.h
> >>  create mode 100644 sound/soc/codecs/tas2552.c
> >>  create mode 100644 sound/soc/codecs/tas2552.h
> >>
> >> diff --git a/Documentation/devicetree/bindings/sound/tas2552.txt b/Documentation/devicetree/bindings/sound/tas2552.txt
> >> new file mode 100644
> >> index 0000000..ada8fd4
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/sound/tas2552.txt
> >> @@ -0,0 +1,22 @@
> >> +Texas Instruments - tas2552 Codec module
> >> +
> >> +The tas2552 serial control bus communicates through I2C protocols
> >> +
> >> +Required properties:
> >> +
> >> +- compatible - One of:
> >> +    "ti,tas2552" - TAS2552
> >> +
> >> +- reg -  I2C slave address
> >> +
> >> +Optional properties:
> >> +
> >> +- power-gpio - gpio pin to enable/disable the device
> >> +
> >> +Example:
> >> +
> >> +tas2552: tas2552@41 {
> >> +	compatible = "ti,tas2552";
> >> +	reg = <0x41>;
> >> +	enable-gpio = <&gpio4 2 GPIO_ACTIVE_HIGH>;
> > you probably want to add:
> >
> > 	pvdd-supply = <&pvdd>;
> > 	vbat-supply = <&vbat>;
> > 	avdd-supply = <&avdd>;
> > 	iovdd-supply = <&iovdd>;
> >
> > that way you can make sure to switch your regulators on from the driver.
> > Since they must be all on, you can just grab them all with
> > regulator_bulk_get() and enable them all with regulator_bulk_enable().
> 
> I could add this but I don't have a use case for this so I did not add
> the code.

actually, you do. Right now you have a device which is powered by
several different sources (fixed or not).

> The supplies I used were always-on so adding the regulators was not
> testable in this patchset.

it _is_ testable. regulator_get()/regulator_enable() still works on
fixed regulators.

> >> @@ -325,3 +326,4 @@ obj-$(CONFIG_SND_SOC_WM_HUBS)	+= snd-soc-wm-hubs.o
> >>  # Amp
> >>  obj-$(CONFIG_SND_SOC_MAX9877)	+= snd-soc-max9877.o
> >>  obj-$(CONFIG_SND_SOC_TPA6130A2)	+= snd-soc-tpa6130a2.o
> >> +obj-$(CONFIG_SND_SOC_TAS2552)	+= snd-soc-tas2552.o
> >> diff --git a/sound/soc/codecs/tas2552.c b/sound/soc/codecs/tas2552.c
> >> new file mode 100644
> >> index 0000000..79b8212
> >> --- /dev/null
> >> +++ b/sound/soc/codecs/tas2552.c
> >> @@ -0,0 +1,463 @@
> >> +/*
> >> + * ALSA SoC Texas Instruments TAS2552 Mono Audio Amplifier
> >> + *
> >> + * Copyright (C) 2014 Texas Instruments Inc.
> >> + *
> >> + * Author: Dan Murphy <dmurphy@...com>
> >> + *
> >> + * This program is free software; you can redistribute it and/or
> >> + * modify it under the terms of the GNU General Public License
> >> + * version 2 as published by the Free Software Foundation.
> >> + *
> >> + * This program is distributed in the hope that it will be useful, but
> >> + * WITHOUT ANY WARRANTY; without even the implied warranty of
> >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> >> + * General Public License for more details.
> >> + */
> >> +
> >> +#include <linux/module.h>
> >> +#include <linux/errno.h>
> >> +#include <linux/device.h>
> >> +#include <linux/i2c.h>
> >> +#include <linux/gpio.h>
> >> +#include <linux/of_gpio.h>
> >> +#include <linux/regmap.h>
> >> +#include <linux/slab.h>
> >> +
> >> +#include <sound/pcm.h>
> >> +#include <sound/pcm_params.h>
> >> +#include <sound/soc.h>
> >> +#include <sound/soc-dapm.h>
> >> +#include <sound/tlv.h>
> >> +#include <sound/tas2552-plat.h>
> >> +
> >> +#include "tas2552.h"
> >> +
> >> +static struct reg_default tas2552_reg_defs[] = {
> >> +	{TAS2552_CFG_1, 0x16},
> >> +	{TAS2552_CFG_3, 0x5E},
> >> +	{TAS2552_DOUT, 0x00},
> >> +	{TAS2552_OUTPUT_DATA, 0xC8},
> >> +	{TAS2552_PDM_CFG, 0x02},
> >> +	{TAS2552_PGA_GAIN, 0x10},
> >> +	{TAS2552_BOOST_PT_CTRL, 0x0F},
> >> +	{TAS2552_LIMIT_LVL_CTRL, 0x0C},
> >> +	{TAS2552_LIMIT_RATE_HYS, 0x20},
> >> +	{TAS2552_CFG_2, 0xEA},
> >> +	{TAS2552_SER_CTRL_1, 0x00},
> >> +	{TAS2552_SER_CTRL_2, 0x00},
> >> +	{TAS2552_PLL_CTRL_1, 0x10},
> >> +	{TAS2552_PLL_CTRL_2, 0x00},
> >> +	{TAS2552_PLL_CTRL_3, 0x00},
> >> +	{TAS2552_BTIP, 0x8f},
> >> +	{TAS2552_BTS_CTRL, 0x80},
> >> +	{TAS2552_LIMIT_RELEASE, 0x05},
> >> +	{TAS2552_LIMIT_INT_COUNT, 0x00},
> >> +	{TAS2552_EDGE_RATE_CTRL, 0x40},
> >> +	{TAS2552_VBAT_DATA, 0x00},
> >> +};
> >> +
> >> +struct tas2552_data {
> >> +	struct mutex mutex;
> >> +	struct snd_soc_codec *codec;
> >> +	struct regmap *regmap;
> >> +	struct i2c_client *tas2552_client;
> >> +	unsigned char regs[TAS2552_VBAT_DATA];
> >> +	int power_gpio;
> >> +	u8 power_state:1;
> >> +};
> >> +
> >> +static int tas2552_power(struct tas2552_data *data, u8 power)
> >> +{
> >> +	int	ret = 0;
> >> +
> >> +	BUG_ON(data->tas2552_client == NULL);
> > don't hang the entire machine because of a bug on the amplifier driver,
> > WARN() should be enough, followed by the return of an error code.
> >
> > In fact, is this really necessary ? It would be a simple bug on the
> > driver to fix.
> 
> Yeah I can remove this.  I was following an older example.
> 
> >
> >> +
> >> +	mutex_lock(&data->mutex);
> >> +	if (power == data->power_state)
> > Same here. Is this really necessary ? It's simple to guarantee this case
> > won't happen in code.
> 
> Yes this LOC is necessary.  It is checking the current state of the tas2552.

heh, the point is that all your calls to this function can be balanced
easily, so this check becomes pointless, as it will never be true.

> >> +		goto exit;
> >> +
> >> +	if (power) {
> >> +		if (data->power_gpio >= 0)
> >> +			gpio_set_value(data->power_gpio, 1);
> >> +
> >> +		data->power_state = 1;
> >> +	} else {
> >> +		if (data->power_gpio >= 0)
> >> +			gpio_set_value(data->power_gpio, 0);
> >> +
> >> +		data->power_state = 0;
> >> +	}
> >> +
> >> +exit:
> >> +	mutex_unlock(&data->mutex);
> >> +	return ret;
> >> +}
> >> +
> >> +static void tas2552_sw_shutdown(struct tas2552_data *tas_data, int sw_shutdown)
> >> +{
> >> +	u8 cfg1_reg = 0x0;
> >> +
> >> +	if (sw_shutdown)
> >> +		cfg1_reg |= (sw_shutdown << 1);
> > this line is dangerous. You're using a 32-bit variable to write a single
> > bit on cfg1 register. What if user passes 0xff on sw_shutdown ?
> >
> > I think a better approach would be to:
> >
> > a) first of all, move this sw_shutdown function to
> > runtime_suspend/runtime_resume.
> 
> Yeah that is not the intent of this API.  This API is called when the ALSA layer
> opens/closes the device.  It is not governed by pm calls.

and PM calls are exactly for that. You start with a powered off device,
then when user wants to use it, you power it up. This is exactly what's
you're doing here.

> > b) to the check as below:
> >
> > 	if (shutdown)
> > 		cfg1_reg |= TAS2552_SWS;
> > 	else
> > 		cfg1_reg &= ~TAS2552_SWS;
> >
> > then, of course #define TAS2552_SWS (1 << 1) (or BIT(1), even)
> 
> But I will make this change.
> 
> >
> >> +	else
> >> +		cfg1_reg &= ~TAS2552_SWS_MASK;
> >> +
> >> +	snd_soc_update_bits(tas_data->codec, TAS2552_CFG_1,
> >> +						 TAS2552_SWS_MASK, cfg1_reg);
> >> +}
> >> +
> >> +static void tas2552_init(struct snd_soc_codec *codec)
> >> +{
> >> +	snd_soc_write(codec, TAS2552_CFG_1, 0x16);
> >> +	snd_soc_write(codec, TAS2552_CFG_3, 0x5E);
> >> +	snd_soc_write(codec, TAS2552_DOUT, 0x00);
> >> +	snd_soc_write(codec, TAS2552_OUTPUT_DATA, 0xC8);
> >> +	snd_soc_write(codec, TAS2552_PDM_CFG, 0x02);
> >> +	snd_soc_write(codec, TAS2552_PGA_GAIN, 0x10);
> >> +	snd_soc_write(codec, TAS2552_BOOST_PT_CTRL, 0x0F);
> >> +	snd_soc_write(codec, TAS2552_LIMIT_LVL_CTRL, 0x0C);
> >> +	snd_soc_write(codec, TAS2552_LIMIT_RATE_HYS, 0x20);
> >> +	snd_soc_write(codec, TAS2552_CFG_2, 0xEA);
> > what do all these magic constants mean ? Also, lower case hex numbers
> > are usually preferred.
> 
> I will add comments to what the numbers mean and change to lower case

I would rather see proper bit macros and the driver using them.

> > No battery tracking ?  Any plans to add that at a later date ? It's
> > probably not needed to have functional audio, but might have some use
> > cases where you want it.
> 
> The battery tracking was not the scope of the driver.  We just need to
> get the basic driver in place with audio functional and add the
> battery tracking later.

it's a single bit

> I also did not have a device that had the battery tracking enabled so
> I could not develop that level of code anyway.

device will track your constant VBAT and, ideally, since voltage would
never drop on VBAT, it shouldn't have to adjust gain.

> > /* goes re-read datasheet */
> >
> > Actually, I strongly believe you want to enable battery tracking (LIM_EN
> > on cfg2).
> >
> >> +}
> >> +
> >> +static int tas2552_hw_params(struct snd_pcm_substream *substream,
> >> +			     struct snd_pcm_hw_params *params,
> >> +			     struct snd_soc_dai *dai)
> >> +{
> >> +	u8 wclk_reg;
> >> +	struct snd_soc_codec *codec = dai->codec;
> >> +
> >> +	/* Setting DAC clock dividers based on substream sample rate. */
> >> +	switch (params_rate(params)) {
> >> +	case 8000:
> >> +		wclk_reg = TAS2552_8KHZ;
> >> +		break;
> >> +	case 11025:
> >> +		wclk_reg = TAS2552_11_12KHZ;
> >> +		break;
> >> +	case 16000:
> >> +		wclk_reg = TAS2552_16KHZ;
> >> +		break;
> >> +	case 32000:
> >> +		wclk_reg = TAS2552_32KHZ;
> >> +		break;
> >> +	case 22050:
> >> +	case 24000:
> >> +		wclk_reg = TAS2552_22_24KHZ;
> >> +		break;
> >> +	case 44100:
> >> +	case 48000:
> >> +		wclk_reg = TAS2552_44_48KHZ;
> >> +		break;
> >> +	case 96000:
> >> +		wclk_reg = TAS2552_88_96KHZ;
> >> +		break;
> >> +	default:
> > might be worth adding a dev_vdbg() here.
> 
> I could, but trying to not add a lot of logging in the code.

dev_vdbg() is only built when CONFIG_VERBOSE_DEBUG is set. Otherwise
it's a no-op and optimized away.

> >> +		return -EINVAL;
> >> +	}
> >> +
> >> +	snd_soc_update_bits(codec, TAS2552_CFG_3, TAS2552_WCLK_MASK, wclk_reg);
> >> +
> >> +	return 0;
> >> +}
> >> +
> >> +static int tas2552_set_dai_fmt(struct snd_soc_dai *dai, unsigned int fmt)
> >> +{
> >> +	u8 serial_format;
> >> +	struct snd_soc_codec *codec = dai->codec;
> >> +
> >> +	switch (fmt & SND_SOC_DAIFMT_MASTER_MASK) {
> >> +	case SND_SOC_DAIFMT_CBS_CFS:
> >> +		serial_format = 0x00;
> >> +		break;
> >> +	case SND_SOC_DAIFMT_CBS_CFM:
> >> +		serial_format = TAS2552_WORD_CLK_MASK;
> >> +		break;
> >> +	case SND_SOC_DAIFMT_CBM_CFS:
> >> +		serial_format = TAS2552_BIT_CLK_MASK;
> >> +		break;
> >> +	case SND_SOC_DAIFMT_CBM_CFM:
> >> +		serial_format = (TAS2552_BIT_CLK_MASK | TAS2552_WORD_CLK_MASK);
> >> +		break;
> >> +	default:
> >> +		return -EINVAL;
> >> +	}
> >> +
> >> +	snd_soc_update_bits(codec, TAS2552_SER_CTRL_1,
> >> +			    (TAS2552_BIT_CLK_MASK | TAS2552_WORD_CLK_MASK),
> >> +			    serial_format);
> >> +
> >> +	switch (fmt & SND_SOC_DAIFMT_FORMAT_MASK) {
> >> +	case SND_SOC_DAIFMT_I2S:
> >> +		serial_format = 0x0;
> >> +		break;
> >> +	case SND_SOC_DAIFMT_DSP_A:
> >> +		serial_format = TAS2552_DAIFMT_DSP;
> >> +		break;
> >> +	case SND_SOC_DAIFMT_RIGHT_J:
> >> +		serial_format = TAS2552_DAIFMT_RIGHT_J;
> >> +		break;
> >> +	case SND_SOC_DAIFMT_LEFT_J:
> >> +		serial_format = TAS2552_DAIFMT_LEFT_J;
> >> +		break;
> >> +
> >> +	default:
> >> +		return -EINVAL;
> >> +	}
> >> +
> >> +	snd_soc_update_bits(codec, TAS2552_SER_CTRL_1, TAS2552_DATA_FORMAT_MASK,
> >> +						serial_format);
> >> +
> >> +	return 0;
> >> +}
> >> +
> >> +static int tas2552_set_dai_sysclk(struct snd_soc_dai *dai, int clk_id,
> >> +				  unsigned int freq, int dir)
> >> +{
> >> +	struct snd_soc_codec *codec = dai->codec;
> >> +	struct tas2552_data *data = dev_get_drvdata(dai->dev);
> >> +
> >> +	/* Fill in the PLL control registers for J & D
> >> +	 * PLL_CLK = (.5 * freq * J.D) / 2^p
> >> +	 * Need to fill in J and D here based on incoming freq
> >> +	 */
> >> +
> >> +	tas2552_sw_shutdown(data, 1);
> > if you move sw_shutdown to runtime_suspend/resume, you could implement
> > this as follows:
> >
> > 	ret = pm_runtime_get_sync(data->dev);
> > 	if (ret)
> > 		return ret;
> 
> See above comment about these APIs not being related to power management

shutdown is not related to PM ? interesting...

-- 
balbi

Download attachment "signature.asc" of type "application/pgp-signature" (820 bytes)

Powered by blists - more mailing lists