On Tue, Sep 07, 2010 at 03:23:20PM +0200, Arnaud Patard wrote:
Mark Brown broonie@opensource.wolfsonmicro.com writes:
It looks like it'd be simpler to do something like:
{ "HPL", NULL, "HP" }, { "HPR", NULL, "HP" },
then have the two ADC channels feed into the left and right channels of the headphone while having the single switches feed into the HP widget (which would be a NOPM dummy widget for routing purposes).
hmm... I'm not sure to understand correctly. The two bits ALC5623_PWR_ADD2_?_HP_MIXER are for the HP mixers. There are 2 mixers but I'm doing like there's only 1 mixer due to the single bit inputs. Can you please give more details about your suggestion ?
Add on to the above things like this:
{ "HPL", "ADC Switch", "ADCL" }, { "HPR", "ADC Switch", "ADCR" },
{ "HP", "AUX Switch", "Aux" },
so you get separate left/right control where you have stereo control while still getting the mono control on the virtual mono HP widget.
+#define ALC5623_ADD1_POWER_EN_5622 \
- (ALC5623_PWR_ADD1_MAIN_I2S_EN | ALC5623_PWR_ADD1_MIC1_BIAS_EN \
- | ALC5623_PWR_ADD1_SHORT_CURR_DET_EN \
- | ALC5623_PWR_ADD1_HP_OUT_AMP)
Most of the stuff in here looks like it ought to be managed by DAPM?
hmm... I thought it was easier/better done like this. Is it a hard requirement ?
Given that you're already defining widgets for things like the I2S (the AIF widgets) it seems silly not to. For the micbias I'd say it's a hard requirement since driving the micbias when not needed is potenially harmful.
Since we now support out of line resume for ASoC devices (so we don't hold up the rest of the system) it should be possible to just do this stuff in the set_bias_level() function rather than faffing around with delayed work like this. This is less racy.
I was even wondering about removing that stuff but as it was there in original driver, I choose to keep it. WDYT ?
If you look at the current WM8753 driver you'll see that it's been removed from there.
- if (alc5623->add_ctrl) {
snd_soc_write(codec, ALC5623_ADD_CTRL_REG,alc5623->add_ctrl);- }
Hrm?
this register contains some board specific values and I didn't see how it could be handled except by using platform_data.
What are they?
+#ifndef _INCLUDE_LINUX_ALC5623_H +#define _INCLUDE_LINUX_ALC5623_H +struct alc5623_platform_data {
- unsigned int add_ctrl;
- unsigned int jack_det_ctrl;
+};
Some documentation explaining what these are would probably be useful.
The names are really near to the names in the specs. As you need to look at the specs to have their values, I thought it would be enough.
So saying something like "Values to be written to the add_ctrl and jack_det_ctrl registers" would cover it. Remember, if a software engineer is picking up a preexisting driver they may never even look at the datasheet.