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

[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 alsa-devel" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html