Re: [PATCH 1/8] ASoC: TPA6130A2 amplifier driver

2009-10-09 Thread Peter Ujfalusi 
On Thursday 08 October 2009 16:53:42 ext Mark Brown wrote:
 On Thu, Oct 08, 2009 at 04:38:24PM +0300, Peter Ujfalusi wrote:
  On Thursday 08 October 2009 15:52:13 ext Mark Brown wrote:
   On Thu, Oct 08, 2009 at 02:58:50PM +0300, Eduardo Valentin wrote:
+struct tpa6130a2_platform_data {
+   int (*set_power)(int state);
+};
  
   Why is this a callback and not just a GPIO number?  That'd seem simpler
   for users.
 
  Well, same amount of code, but in different place if the power is enabled
 
 Until someone adds a second board, at which point they need to copy the
 code to acquire and release the GPIO.
 
  through a GPIO. But if the power is controlled via different means
  (nothing comes to mind, but there are exotic systems out there), in this
  way it is simple to handle
 
 I suspect we can deal with that adequately when it crops up, for example
 by having the driver set up a default callback function if a GPIO is
 specified instead of a callback.

Good point.
I'll replace the .set_power with power_gpio.

 
+   pdata = (struct tpa6130a2_platform_data
*)client-dev.platform_data; +  /* Set default register values */
+   data-regs[TPA6130A2_REG_CONTROL] = TPA6130A2_SWS |
+   TPA6130A2_HP_EN_R |
+   TPA6130A2_HP_EN_L;
  
   This looks like you could implement stereo paths with individual power
   control?
 
  Can we put something in between DAPM_OUTPUT and DAPM_HP to handle the
  stereo path correctly?
 
 Yes.

Great. I can add one SND_SOC_DAPM_PGA_E per channel, which enables/disables the 
channel.
Adding a widget with actual alsa control seams to be problematic, since those 
are working with the codec's registers, so adding such a widget would require 
to 
implement a new set of .info .get  and .set functions for the TPA alone.

 
  We could have two paths from codec to the TPA6130A2 Headphone, which is
  needed to actually control the power of the amplifier.
 
 Or make TPA6130A2 Headphone be a SND_SOC_DAPM_SUPPLY() connected to
 the individual channels for the headphone outputs, which should do the
 right thing.

I see, using the event/event_flags with the DAPM_SUPPLY should do it.
So is it OK if I modify the tpa6130a2 DAPM path to be like this:
PGA_E(TPA6130A2 Left)  - SUPPLY(TPA6130A2 power) - HP(TPA6130A2 
Headphone)

PGA_E(TPA6130A2 Right) - SUPPLY(TPA6130A2 power) - HP(TPA6130A2 
Headphone)

Or should I add individual HP for the two channels:
HP(TPA6130A2 Headphone Left)
HP(TPA6130A2 Headphone Right)

Than in machine driver just connect (for example rx51):
{TPA6130A2 Left, NULL, LLOUT},
{TPA6130A2 Right, NULL, RLOUT},


 
  At the end we are not really gaining much, but one more level of switch
  on the path.
  We could have simple mute alsa controls in tpa6130a2_controls for the
  amplifier itself...
 
 It'd mean that mono outputs would only need to enable one of the
 channels.  Depending on the hardware feeding the amp this may behave
 better - there may be some noise or leakage on the idle channel which
 it'd be better to avoid amplifying - and it should certainly use a
 little less power.

OK, Does my proposal above feasible?

 
+   err = tpa6130a2_i2c_read(TPA6130A2_REG_VERSION) 
+TPA6130A2_VERSION_MASK;
+   if ((err != 1)  (err != 2)) {
+   dev_err(dev, Unexpected headphone amplifier chip 
version 
+  of 0x%02x, was expecting 0x01 or 0x02\n, err);
+   err = -ENODEV;
  
   This seems a bit excessive - is it really likely that the register map
   would be changed incompatibly in a future version?
 
  Hmm, we have only seen these versions of the chip, and we know that the
  driver works with these. Also we don't have any information on different
  versions, but I would think that the register map is not changing (the
  reset values for some registers are different)
 
 It's fairly common to see some incompatible changes in early silicon
 revisions but once things get properly launched it's fairly unusual.
 I'd be more inclined to print a warning here rather than fail - chances
 are that the driver will work fine with any new revisions that are
 produced.

Right, going to be warning (with text change).

 

Thanks,
Péter
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [alsa-devel] [PATCH 1/8] ASoC: TPA6130A2 amplifier driver

2009-10-09 Thread Mark Brown 
On Fri, Oct 09, 2009 at 09:53:21AM +0300, Peter Ujfalusi wrote:
 On Thursday 08 October 2009 16:53:42 ext Mark Brown wrote:

 Adding a widget with actual alsa control seams to be problematic, since those 
 are working with the codec's registers, so adding such a widget would require 
 to 
 implement a new set of .info .get  and .set functions for the TPA alone.

Yes, chips like this are the major reason for needing to support having
more than one CODEC device in the core.

 Or should I add individual HP for the two channels:
 HP(TPA6130A2 Headphone Left)
 HP(TPA6130A2 Headphone Right)

 Than in machine driver just connect (for example rx51):
 {TPA6130A2 Left, NULL, LLOUT},
 {TPA6130A2 Right, NULL, RLOUT},

That's what I was thinking of, yes.
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/8] ASoC: TPA6130A2 amplifier driver

2009-10-08 Thread Eduardo Valentin 
From: Peter Ujfalusi peter.ujfal...@nokia.com

Driver for Texas Instruments TPA6130A2 headphone stereo
amplifier.

Signed-off-by: Peter Ujfalusi peter.ujfal...@nokia.com
Signed-off-by: Eduardo Valentin eduardo.valen...@nokia.com
---
 include/sound/tpa6130a2-plat.h |   30 +++
 sound/soc/codecs/Kconfig   |4 +
 sound/soc/codecs/Makefile  |2 +
 sound/soc/codecs/tpa6130a2.c   |  380 
 sound/soc/codecs/tpa6130a2.h   |   62 +++
 5 files changed, 478 insertions(+), 0 deletions(-)
 create mode 100644 include/sound/tpa6130a2-plat.h
 create mode 100644 sound/soc/codecs/tpa6130a2.c
 create mode 100644 sound/soc/codecs/tpa6130a2.h

diff --git a/include/sound/tpa6130a2-plat.h b/include/sound/tpa6130a2-plat.h
new file mode 100644
index 000..d315728
--- /dev/null
+++ b/include/sound/tpa6130a2-plat.h
@@ -0,0 +1,30 @@
+/*
+ * TPA6130A2 driver platform header
+ *
+ * Copyright (C) Nokia Corporation
+ *
+ * Written by Peter Ujfalusi peter.ujfal...@nokia.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.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA
+ * 02110-1301 USA
+ */
+
+#ifndef TPA6130A2_PLAT_H
+#define TPA6130A2_PLAT_H
+
+struct tpa6130a2_platform_data {
+   int (*set_power)(int state);
+};
+
+#endif
diff --git a/sound/soc/codecs/Kconfig b/sound/soc/codecs/Kconfig
index 0edca93..2437fd3 100644
--- a/sound/soc/codecs/Kconfig
+++ b/sound/soc/codecs/Kconfig
@@ -28,6 +28,7 @@ config SND_SOC_ALL_CODECS
select SND_SOC_TLV320AIC23 if I2C
select SND_SOC_TLV320AIC26 if SPI_MASTER
select SND_SOC_TLV320AIC3X if I2C
+   select SND_SOC_TPA6130A2 if I2C
select SND_SOC_TWL4030 if TWL4030_CORE
select SND_SOC_UDA134X
select SND_SOC_UDA1380 if I2C
@@ -220,3 +221,6 @@ config SND_SOC_WM9713
 # Amp
 config SND_SOC_MAX9877
tristate
+
+config SND_SOC_TPA6130A2
+   tristate
diff --git a/sound/soc/codecs/Makefile b/sound/soc/codecs/Makefile
index fb4af28..498c024 100644
--- a/sound/soc/codecs/Makefile
+++ b/sound/soc/codecs/Makefile
@@ -47,6 +47,7 @@ snd-soc-wm-hubs-objs := wm_hubs.o
 
 # Amp
 snd-soc-max9877-objs := max9877.o
+snd-soc-tpa6130a2-objs := tpa6130a2.o
 
 obj-$(CONFIG_SND_SOC_AC97_CODEC)   += snd-soc-ac97.o
 obj-$(CONFIG_SND_SOC_AD1836)   += snd-soc-ad1836.o
@@ -97,3 +98,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
diff --git a/sound/soc/codecs/tpa6130a2.c b/sound/soc/codecs/tpa6130a2.c
new file mode 100644
index 000..d246aad
--- /dev/null
+++ b/sound/soc/codecs/tpa6130a2.c
@@ -0,0 +1,380 @@
+/*
+ * ALSA SoC Texas Instruments TPA6130A2 headset stereo amplifier driver
+ *
+ * Copyright (C) Nokia Corporation
+ *
+ * Author: Peter Ujfalusi peter.ujfal...@nokia.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.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA
+ * 02110-1301 USA
+ */
+
+#include linux/module.h
+#include linux/errno.h
+#include linux/string.h
+#include linux/device.h
+#include linux/i2c.h
+#include linux/sysfs.h
+#include sound/tpa6130a2-plat.h
+#include sound/soc.h
+#include sound/soc-dapm.h
+#include sound/tlv.h
+
+#include tpa6130a2.h
+
+struct i2c_client *tpa6130a2_client;
+
+/* This struct is used to save the context */
+struct tpa6130a2_data {
+   /* mutex protect access to tpa6130a2_data structure */
+   struct mutex mutex;
+   unsigned char regs[TPA6130A2_CACHEREGNUM];
+   unsigned char power_state;
+   int (*set_power)(int state);
+};
+
+static int tpa6130a2_i2c_read(int reg)
+{
+   struct tpa6130a2_data *data;
+   int val;
+
+   BUG_ON(tpa6130a2_client == NULL);
+
+   data = i2c_get_clientdata(tpa6130a2_client);
+
+   /* If powered off, return the cached value */
+   if (data-power_state) {
+   val

Re: [PATCH 1/8] ASoC: TPA6130A2 amplifier driver

2009-10-08 Thread Eero Nurkkala 
On Thu, 2009-10-08 at 13:58 +0200, Valentin Eduardo (Nokia-D/Helsinki)
wrote:
 +/*
 + * TPA6130 volume. From -59.5 to 4 dB with increasing step size when going
 + * down in gain. Justify scale so that it is quite correct from -20 dB and
 + * up. This setting shows -30 dB at minimum, -12.95 dB at 49 % (actual
 + * is -10.3 dB) and 4.65 dB at maximum (actual is 4 dB).
 + */

The comment is misleading from all it says. For me it seems that the
scale is quite correct from -59.5 to 4 dB or so. Also the minimum is
-59.5 dB, not -30.

 +static const unsigned int tpa6130_tlv[] = {
 +   TLV_DB_RANGE_HEAD(10),
 +   0, 1, TLV_DB_SCALE_ITEM(-5950, 600, 0),
 +   2, 3, TLV_DB_SCALE_ITEM(-5000, 250, 0),
 +   4, 5, TLV_DB_SCALE_ITEM(-4550, 160, 0),
 +   6, 7, TLV_DB_SCALE_ITEM(-4140, 190, 0),
 +   8, 9, TLV_DB_SCALE_ITEM(-3650, 120, 0),
 +   10, 11, TLV_DB_SCALE_ITEM(-3330, 160, 0),
 +   12, 13, TLV_DB_SCALE_ITEM(-3040, 180, 0),
 +   14, 20, TLV_DB_SCALE_ITEM(-2710, 110, 0),
 +   21, 37, TLV_DB_SCALE_ITEM(-1960, 74, 0),
 +   38, 63, TLV_DB_SCALE_ITEM(-720, 45, 0),
 +};
 +

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/8] ASoC: TPA6130A2 amplifier driver

2009-10-08 Thread Mark Brown 
On Thu, Oct 08, 2009 at 02:58:50PM +0300, Eduardo Valentin wrote:

 +struct tpa6130a2_platform_data {
 + int (*set_power)(int state);
 +};

Why is this a callback and not just a GPIO number?  That'd seem simpler
for users.

 +int tpa6130a2_add_controls(struct snd_soc_codec *codec)
 +{
 + snd_soc_dapm_new_controls(codec, tpa6130a2_dapm_widgets,
 + ARRAY_SIZE(tpa6130a2_dapm_widgets));
 +
 + return snd_soc_add_controls(codec, tpa6130a2_controls,
 + ARRAY_SIZE(tpa6130a2_controls));
 +
 +}
 +EXPORT_SYMBOL(tpa6130a2_add_controls);

All the ASoC APIs are EXPORT_SYMBOL_GPL().

 + pdata = (struct tpa6130a2_platform_data *)client-dev.platform_data;
 + /* Set default register values */
 + data-regs[TPA6130A2_REG_CONTROL] = TPA6130A2_SWS |
 + TPA6130A2_HP_EN_R |
 + TPA6130A2_HP_EN_L;

This looks like you could implement stereo paths with individual power
control?

 + data-regs[TPA6130A2_REG_VOL_MUTE] = TPA6130A2_VOLUME(20);

The standard thing is to use hardware defaults and leave decisions like
this up to user space.

 + data-set_power = pdata-set_power;
 + if (!pdata-set_power) {
 + data-power_state = 1;
 + tpa6130a2_initialize();
 + }
 + tpa6130a2_power(1);
 +
 + /* Read version */
 + err = tpa6130a2_i2c_read(TPA6130A2_REG_VERSION) 
 +  TPA6130A2_VERSION_MASK;
 + if ((err != 1)  (err != 2)) {
 + dev_err(dev, Unexpected headphone amplifier chip version 
 +of 0x%02x, was expecting 0x01 or 0x02\n, err);
 + err = -ENODEV;

This seems a bit excessive - is it really likely that the register map
would be changed incompatibly in a future version?
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/8] ASoC: TPA6130A2 amplifier driver

2009-10-08 Thread Peter Ujfalusi 
On Thursday 08 October 2009 15:30:29 Nurkkala Eero.An (EXT-Offcode/Oulu) wrote:
 On Thu, 2009-10-08 at 13:58 +0200, Valentin Eduardo (Nokia-D/Helsinki)
 
 wrote:
  +/*
  + * TPA6130 volume. From -59.5 to 4 dB with increasing step size when
  going + * down in gain. Justify scale so that it is quite correct from
  -20 dB and + * up. This setting shows -30 dB at minimum, -12.95 dB at 49
  % (actual + * is -10.3 dB) and 4.65 dB at maximum (actual is 4 dB).
  + */
 
 The comment is misleading from all it says. For me it seems that the
 scale is quite correct from -59.5 to 4 dB or so. Also the minimum is
 -59.5 dB, not -30.

Yes, the scale is really close to the reality, the comment need to be fixed. 

-- 
Péter
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/8] ASoC: TPA6130A2 amplifier driver

2009-10-08 Thread Peter Ujfalusi 
On Thursday 08 October 2009 15:52:13 ext Mark Brown wrote:
 On Thu, Oct 08, 2009 at 02:58:50PM +0300, Eduardo Valentin wrote:
  +struct tpa6130a2_platform_data {
  +   int (*set_power)(int state);
  +};
 
 Why is this a callback and not just a GPIO number?  That'd seem simpler
 for users.

Well, same amount of code, but in different place if the power is enabled 
through a GPIO. But if the power is controlled via different means (nothing 
comes to mind, but there are exotic systems out there), in this way it is 
simple 
to handle

 
  +int tpa6130a2_add_controls(struct snd_soc_codec *codec)
  +{
  +   snd_soc_dapm_new_controls(codec, tpa6130a2_dapm_widgets,
  +   ARRAY_SIZE(tpa6130a2_dapm_widgets));
  +
  +   return snd_soc_add_controls(codec, tpa6130a2_controls,
  +   ARRAY_SIZE(tpa6130a2_controls));
  +
  +}
  +EXPORT_SYMBOL(tpa6130a2_add_controls);
 
 All the ASoC APIs are EXPORT_SYMBOL_GPL().

Right.

 
  +   pdata = (struct tpa6130a2_platform_data *)client-dev.platform_data;
  +   /* Set default register values */
  +   data-regs[TPA6130A2_REG_CONTROL] = TPA6130A2_SWS |
  +   TPA6130A2_HP_EN_R |
  +   TPA6130A2_HP_EN_L;
 
 This looks like you could implement stereo paths with individual power
 control?

Can we put something in between DAPM_OUTPUT and DAPM_HP to handle the stereo 
path correctly?
We could have two paths from codec to the TPA6130A2 Headphone, which is 
needed 
to actually control the power of the amplifier.
At the end we are not really gaining much, but one more level of switch on the 
path.
We could have simple mute alsa controls in tpa6130a2_controls for the amplifier 
itself...

 
  +   data-regs[TPA6130A2_REG_VOL_MUTE] = TPA6130A2_VOLUME(20);
 
 The standard thing is to use hardware defaults and leave decisions like
 this up to user space.

OK, The reset value is 0 (-59.5 dB)

 
  +   data-set_power = pdata-set_power;
  +   if (!pdata-set_power) {
  +   data-power_state = 1;
  +   tpa6130a2_initialize();
  +   }
  +   tpa6130a2_power(1);
  +
  +   /* Read version */
  +   err = tpa6130a2_i2c_read(TPA6130A2_REG_VERSION) 
  +TPA6130A2_VERSION_MASK;
  +   if ((err != 1)  (err != 2)) {
  +   dev_err(dev, Unexpected headphone amplifier chip version 
  +  of 0x%02x, was expecting 0x01 or 0x02\n, err);
  +   err = -ENODEV;
 
 This seems a bit excessive - is it really likely that the register map
 would be changed incompatibly in a future version?

Hmm, we have only seen these versions of the chip, and we know that the driver 
works with these. Also we don't have any information on different versions, but 
I would think that the register map is not changing (the reset values for some 
registers are different)

-- 
Péter
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/8] ASoC: TPA6130A2 amplifier driver

2009-10-08 Thread Mark Brown 
On Thu, Oct 08, 2009 at 04:38:24PM +0300, Peter Ujfalusi wrote:
 On Thursday 08 October 2009 15:52:13 ext Mark Brown wrote:
  On Thu, Oct 08, 2009 at 02:58:50PM +0300, Eduardo Valentin wrote:
   +struct tpa6130a2_platform_data {
   + int (*set_power)(int state);
   +};

  Why is this a callback and not just a GPIO number?  That'd seem simpler
  for users.

 Well, same amount of code, but in different place if the power is enabled 

Until someone adds a second board, at which point they need to copy the
code to acquire and release the GPIO.

 through a GPIO. But if the power is controlled via different means (nothing 
 comes to mind, but there are exotic systems out there), in this way it is 
 simple 
 to handle

I suspect we can deal with that adequately when it crops up, for example
by having the driver set up a default callback function if a GPIO is
specified instead of a callback.

   + pdata = (struct tpa6130a2_platform_data *)client-dev.platform_data;
   + /* Set default register values */
   + data-regs[TPA6130A2_REG_CONTROL] = TPA6130A2_SWS |
   + TPA6130A2_HP_EN_R |
   + TPA6130A2_HP_EN_L;

  This looks like you could implement stereo paths with individual power
  control?

 Can we put something in between DAPM_OUTPUT and DAPM_HP to handle the stereo 
 path correctly?

Yes.

 We could have two paths from codec to the TPA6130A2 Headphone, which is 
 needed 
 to actually control the power of the amplifier.

Or make TPA6130A2 Headphone be a SND_SOC_DAPM_SUPPLY() connected to
the individual channels for the headphone outputs, which should do the
right thing.

 At the end we are not really gaining much, but one more level of switch on 
 the 
 path.
 We could have simple mute alsa controls in tpa6130a2_controls for the 
 amplifier 
 itself...

It'd mean that mono outputs would only need to enable one of the
channels.  Depending on the hardware feeding the amp this may behave
better - there may be some noise or leakage on the idle channel which
it'd be better to avoid amplifying - and it should certainly use a
little less power.

   + err = tpa6130a2_i2c_read(TPA6130A2_REG_VERSION) 
   +  TPA6130A2_VERSION_MASK;
   + if ((err != 1)  (err != 2)) {
   + dev_err(dev, Unexpected headphone amplifier chip version 
   +of 0x%02x, was expecting 0x01 or 0x02\n, err);
   + err = -ENODEV;

  This seems a bit excessive - is it really likely that the register map
  would be changed incompatibly in a future version?

 Hmm, we have only seen these versions of the chip, and we know that the 
 driver 
 works with these. Also we don't have any information on different versions, 
 but 
 I would think that the register map is not changing (the reset values for 
 some 
 registers are different)

It's fairly common to see some incompatible changes in early silicon
revisions but once things get properly launched it's fairly unusual.
I'd be more inclined to print a warning here rather than fail - chances
are that the driver will work fine with any new revisions that are
produced.
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html