Message-ID: <20140702154323.GD5541@saruman.home>
Date:	Wed, 2 Jul 2014 10:43:23 -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 10:30:25AM -0500, Dan Murphy wrote:
> On 07/02/2014 10:03 AM, Felipe Balbi wrote:
> > 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).
> 
> Does the regulator_enable *gaurantee* that the supplies will be turned on in
> the following order?
> 
> 1. VBAT,
> 2. IOVDD,
> 3. AVDD.

you can always regulator_enable() each one in the order you want.

> >> 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.
> 
> I did not say i used fixed regulators.  I indicated that the regulators
> were always-on.  So regulator_enable/disable would have no affect.  right?
> 
> I mean you cannot turn off vbat right?

It depends. If you're sharing VBAT with the board's power source, then
yeah, you can't disable it. You would still see
regulator_enable()/regulator_disable() being called, which serves as
unit testing.

And VBAT, *would* be modeled as a fixed regulator, so all the code paths
are tested, it's just that there's no gpio (or whatever) to be toggled
for VBAT.

> >>>> @@ -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.
> 
> I will remove it.
> 
> >>>> +		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.
> 
> I will look into it.
> 
> But I am not convinced I need these calls yet.

what your shutdown function is basically a private runtime_pm
implementation. Look at your usage of it:

	shutdown(0);
	do_something_magical();
	shutdown(1);

The translation to:

	pm_runtime_get_sync();
	do_something_magical();
	pm_runtime_put_sync();

is really straight forward. You can even move the GPIO enable to
runtime_resume() and this shutdown(1) to runtime_idle so that it would
look like:

static int tas2552_runtime_resume(struct device *dev)
{
	struct tas2552_data *tas = dev_get_drvdata(dev);
	u8 reg;

	gpio_set_value(tas->enable_gpio, 1);
	reg |= TAS2552_SWS;
	snd_soc_update_bits(tas_data->codec, TAS2552_CFG_1,
			TAS2552_SWS_MASK, reg);

	return 0;
}

static int tas2552_runtime_suspend(struct device *dev)
{
	struct tas2552_data *tas = dev_get_drvdata(dev);

	gpio_set_value(tas->enable_gpio, 0);

	return 0;
}

static int tas2552_runtime_idle(struct device *dev)
{
	struct tas2552_data *tas = dev_get_drvdata(dev);
	u8 reg;

	reg &= ~TAS2552_SWS;
	snd_soc_update_bits(tas_data->codec, TAS2552_CFG_1,
			TAS2552_SWS_MASK, reg);
}

how to properly use them in this driver and make sure the device is only
"suspended" on close() is left as an exercise.

> >>> 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.
> 
> Yeah I started doing that in my initial driver but the bit macros were getting
> a little obsessive.
> 
> >
> >>> 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 will flip the bit for the default.
> 
> I looked back in my emails and it was the IVsense code I could not develop not
> the battery checking.

if you really want to make sure the device *does* track the battery,
just hook that pin to a lab power supply and slowly dial down the output
voltage, then look at the gain to see if it is tracking.

-- 
balbi

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

Powered by blists - more mailing lists