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)