[alsa-devel] [RFC 1/4] ASoC SST: Add msic codec driver
broonie at opensource.wolfsonmicro.com
Tue Dec 28 16:54:39 CET 2010
On Tue, Dec 28, 2010 at 09:06:28PM +0530, Koul, Vinod wrote:
> On Tue, Dec 28, 2010 at 08:18:27PM +0530, Mark Brown wrote:
> > > sound/soc/codecs/msic.h | 102 +++++++
> > > 2 files changed, 823 insertions(+), 0 deletions(-)
> > Makefile and Kconfig too please.
> We added all make and kconfig changes in patch4, if you want this to be per
> driver basic, next time I will do that way
Certainly for the CODEC driver. For the platform stuff that's fine.
> Along with read and write the platform provides update bits api. So internally I
> call this when I want to update a bit (instead of soc_update which does and read
> and then write)
It'd still be easier to read the code if you used the standard APIs.
> On this I have a proposal if system provides a modify api can we add one more
> function and use that in update bits. Like:
> if (codec->modif)
> rest of current update logic...
I'm not sure it's worth it; I'd be more worried about the drivers
implementing this badly than anything else. It's also not going to play
well with the register cache stuff.
> > > + /*adding pcm 2 here for a while*/
> > > + msic_modify(codec, MSIC_PCM2C2, BIT(0), BIT(0));
> > > + break;
> > Hrm?
> This is PCM enable which I initially added in codec map and caused problems,
> hence the comment. Does this look apt in this place?
At least explain what the code is doing and why it's temporary. The
comment isn't terribly clear.
> > Most of these look like they're just plain mute controls which would
> > more normally be represented as regular sound controls not related to
> > power, leaving these controls as PGAs (which is what they mostly appear
> > to be). That would make the sequencing better, I expect, and would give
> > stereo controls to the application layer which is more normal.
> So what you are proposing is to add these playback switches as PGAs?
> These are actually the output driver enable switches which should be turned on
> only when stream is active, hence added these as DAPM_SWITCH.
If they're power controls they shouldn't be user visible switches at all
and should be PGA widgets.
> > > + /*fix the initial volume at 0dB*/
> > > + msic_write(codec, MSIC_HSLVOLCTRL, 0x08);
> > > + msic_write(codec, MSIC_HSRVOLCTRL, 0x08);
> > > + msic_write(codec, MSIC_IHFLVOLCTRL, 0x08);
> > > + msic_write(codec, MSIC_IHFRVOLCTRL, 0x08);
> > Leave these at the hardware defaults; 0dB may not be appropriate for
> > other systems (it seems rather loud for a headphone output...).
> The h/w default is +9dB hence setting it to 0.
Fail; I'm assuming this is extremely loud - if it is then please add a
comment explaining what the problem with the hardware defaults is and
why you're working around it. It may be worth considering going to the
other extreme and muting by default.
> > > + /*dac mode and lo w/a*/
> > > + msic_write(codec, MSIC_SSR2, 0x10);
> > > + msic_write(codec, MSIC_SSR3, 0x40);
> > "lo w/a"?
> Lineout w/a, unless we program these they don't work as intended :(
"w/a" isn't very clear.
More information about the Alsa-devel