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: snd_soc_update_bits() { if (codec->modif) codec->modify(....) 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.