Felipe
As always great feedback thanks!!!
On 07/02/2014 10:43 AM, Felipe Balbi wrote:
> 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@xxxxxx>
>>>>>> ---
>>>>>>
>>>>>> 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.
Will add this then.
>
>>>> 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@xxxxxx>
>>>>>> + *
>>>>>> + * 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.
SGTM I will see what I can do.
>
>>>>> 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.
>
I could do that.
But that would be testing the HW itself and not really this code.
I would expect to just verify that the bit was flipped and stayed that
way through operation.
--
------------------
Dan Murphy
--
To unsubscribe from this list: send the line "unsubscribe linux-sound" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
[Index of Archives]
[Pulseaudio]
[Linux Audio Users]
[ALSA Devel]
[Fedora Desktop]
[Fedora SELinux]
[Big List of Linux Books]
[Yosemite News]
[KDE Users]
