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 alsa-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
