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

[Mark Brown <broonie@opensource.wolfsonmicro.com>]

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.