lkml.org 
[lkml]   [2014]   [Jun]   [30]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v2] ASoC: tas2552: Support TI TAS2552 Amplifier
Hi

On 06/30/2014 12:21 PM, Mark Rutland wrote:
> Hi,
>
> On Mon, Jun 30, 2014 at 06:10:59PM +0100, 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@ti.com>
>> ---
>>
>> v2 - Address RFC comments- Added regmap, and snd_soc calls
>> removed debug code, address checkpatch errors -https://patchwork.kernel.org/patch/4378281/
>>
>> .../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 | 462 ++++++++++++++++++++
>> sound/soc/codecs/tas2552.h | 75 ++++
>> 6 files changed, 591 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..58e931b
>> --- /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 - "string" - One of:
>> + "ti,tas2552" - TAS2552
> Drop the "string". The compatible list is a standard property and its
> type (string list) is well-known.
>
> You could change "One of:" to "should contain:", which will look less
> weird with a single entry.
>
>> +- reg - <int> - I2C slave address
> Similarly to "string" we can get rid of <int> here. i2c addresses are
> well-known to be described in a single u32 cell.
>
>> +
>> +Optional properties:
>> +
>> +- power-gpio - gpio pin to enable/disable the device
> The code below seems to look for "enable-gpio". Searching for
> "power-gpio" only hits in the line above and the example below. I assume
> the code is in error?

No this was PEBKAC I did not update the documentation when I wrote the
code.

>
>> +
>> +Example:
>> +
>> +tas2552: tas2552@41 {
>> + compatible = "ti,tas2552";
>> + reg = <0x41>;
>> + power-gpio = <&gpio4 2 GPIO_ACTIVE_HIGH>;
>> +};
> [...]
>
>> + if (pdata) {
>> + data->power_gpio = pdata->power_gpio;
>> + } else if (np) {
>> + data->power_gpio = of_get_named_gpio(np, "enable-gpio", 0);
> Usually I see this logic the other way around, looking for DT data first
> then falling back to platform data if no DT information was present.
>
> Cheers,
> Mark.

Thanks for the review. I will take these changes into v3 after a good code review soak time.

Dan

--
------------------
Dan Murphy



\
 
 \ /
  Last update: 2014-06-30 20:01    [W:0.054 / U:2.464 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site