On Wed, Aug 31, 2016 at 3:17 PM, Danny Milosavljevic
<dannym@xxxxxxxxxxxxxxx> wrote:
> Hi Chen-Yu,
>
>> > +static const char * const sun4i_codec_difflinein_capture_source[] = {
>> > + "Non-Differential",
>> > + "Differential",
>>
>> How about "Stereo"? And possibly "Mono Differential"? or just "Mono".
>
>> A differential input can be used single-ended by grounding one side.
>
> Yes, but that's interpretation of what it's going to be used for.
> The hardware either subtracts or not, nothing more.
> That said, I'll change it to "Stereo" and "Mono Differential".
>
>> > + SOC_SINGLE_TLV("Line Capture Volume", \
>> > + SUN4I_CODEC_ADC_ACTL, \
>> > + SUN4I_CODEC_ADC_ACTL_LNPREG, \
>> > + 7, \
>> > + 0, \
>> > + sun4i_codec_linein_preamp_gain_scale)
>>
>> Nope. This is a pre-gain or boost. It affects both playback and
>> capture. "Line Boost Volume" would be better.
>
> According to Documentation/sound/alsa/ControlNames.txt that is not a valid name.
> Also alsa-lib does special things depending on the control name so we are not
> free to choose here. If it were me freely choosing the names then half the
> control names in this module would change.
I saw some other drivers use "Boost Volume". Also, alsa-lib looks
for suffices, such as "Volume", "Playback Volume", "Switch", etc..
"Boost" is not part of any special name treatment. Though if you
want to follow ControlNames.txt closely, I would suggest "X Pre-Amp",
which would really be the source.
>
>> Same for the Mic pre-amp gain controls.
>
> Likewise. I can see that it's confusing... but what should we do?
>
>> I have a few patches that introduce SOC_DAPM_DOUBLE, so you can share a
>> control between left/right channels. IMHO it makes the userspace mixer
>> less confusing.
>
> I agree. It would be nice to use this in the future when it's merged.
> Will you post it?
I will, probably as part of the A31 codec series. If you really like it
I can post it first, and we both can base our patches on it.
As you mentioned ControlNames.txt, I need to revisit the A31 control
names.
>
>> > + /* MUX */
>> > + SND_SOC_DAPM_MUX("Left Capture Select", SND_SOC_NOPM, 0, 0,
>> > + &sun4i_codec_capture_source_controls),
>> > + SND_SOC_DAPM_MUX("Right Capture Select", SND_SOC_NOPM, 0, 0,
>> > + &sun4i_codec_capture_source_controls),
>> > + SND_SOC_DAPM_MUX("Differential Line Capture Switch", SND_SOC_NOPM,
>> > + 0,
>> > + 0,
>> > + &sun4i_codec_difflinein_capture_source_controls),
>>
>> The proper function suffix is "Route", not "Select".
>
> Indeed. Also for "Differential Line Capture Switch" except for the enum, I suppose.
IIRC alsa-lib checks if it's an enum first, so it would appear as the right
type of control anyway. I'm not sure though.
>
>> > + /* Inputs */
>> > SND_SOC_DAPM_INPUT("Mic1"),
>> > + SND_SOC_DAPM_INPUT("Mic2"),
>>
>> How about SND_SOC_DAPM_MIC?
>
> What does it do differently? Seems to use different callback and all.
> Is it worth changing an extensively tested patch because of it?
Seems that you can tie an extra event handler to it, and it is treated
as an endpoint on a fully routed card. Neither is the case here, so
it really makes no difference. Lets keep it as INPUT for now.
Ref: On a fully routed card, INPUT/OUTPUT widgets are not endpoints.
You need to route Mic/Headphone/Speaker/Line widgets to/from them
for the whole path to work.
I might need to rethink my approach as well.
>
>>And what about microphone bias?
>
> The User Manual mentions microphone bias exactly once - in the summary.
>
> Searching for just "bias", there's AC_ADDA_BIAS_CTRL "for DAC/ADC
> performance tuning" and AC_DAC_CAL BIASCALI.
> Is it one of those? How does it work?
As I mentioned in my other reply, you did everything right in this
patch. Sorry for the noise.
>> > + SND_SOC_DAPM_INPUT("Line Right"),
>> > + SND_SOC_DAPM_INPUT("Line Left"),
>> > + SND_SOC_DAPM_INPUT("FM Right"),
>> > + SND_SOC_DAPM_INPUT("FM Left"),
>>
>> Why the left/right channels?
>
> Because they exist in hardware.
> Also Mic1 and Mic2 are listed as well, so for symmetry.
>
>> You aren't doing anything special for
>> them. You could just have one Line and one FM, and have routes to
>> left/right mixers.
>
>> > static struct snd_soc_codec_driver sun7i_codec_codec = {
>> > .component_driver = {
>> > - .controls = sun7i_codec_controls,
>> > - .num_controls = ARRAY_SIZE(sun7i_codec_controls),
>> > - .dapm_widgets = sun4i_codec_codec_dapm_widgets,
>> > - .num_dapm_widgets = ARRAY_SIZE(sun4i_codec_codec_dapm_widgets),
>> > - .dapm_routes = sun4i_codec_codec_dapm_routes,
>> > - .num_dapm_routes = ARRAY_SIZE(sun4i_codec_codec_dapm_routes),
>> > + .controls = sun7i_codec_controls,
>> > + .num_controls = ARRAY_SIZE(sun7i_codec_controls),
>> > + .dapm_widgets = sun4i_codec_codec_dapm_widgets,
>> > + .num_dapm_widgets = ARRAY_SIZE(sun4i_codec_codec_dapm_widgets),
>> > + .dapm_routes = sun4i_codec_codec_dapm_routes,
>> > + .num_dapm_routes = ARRAY_SIZE(sun4i_codec_codec_dapm_routes),
>>
>> You should put these changes in the first patch.
>
> Indeed.
>
> Cheers,
> Danny
Regards
ChenYu
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
