Re: [PATCH] ASoC: Add support for NAU8824 codec to ASoC

[Mark Brown]

On Wed, Mar 04, 2015 at 08:35:52PM +0800, Chih-Chiang Chang wrote:
> On 2015/2/24 äå 10:13, Mark Brown wrote:

> > I would have expected the headphone volume control to be a stereo
> > (double) control - same for speakers.

> The nau8824 related registers which control left/right volume are located
> in different addresses and different shift bits. Since there is no available
> preprocessor macro to meet our requirements, the driver consists of left/right
> volume control separately.

Add relevant control types if you need them, it's important to have
proper stereo controls available to userspace.

> >> +struct nau8824_init_reg {
> >> +    u8 reg;
> >> +    u16 val;
> >> +};

> > This looks like you're reimplementing regmap's register sequence
> > stuff...  It's also a *very* large sequence you have, are you sure it's
> > all required?  It seems like this may be doing a bunch of machine
> > specific configuration but since it's all magic numbers it's hard to
> > tell.

> Initial settings are arranged in order

This doesn't answer or address my concern.

> >> +    /* Dynamic Headset detection enabled */
> >> +    snd_soc_update_bits(codec, 0x01, 0x400, 0x400);
> >> +    snd_soc_update_bits(codec, 0x02, 0x0008, 0x0008);
> >> +    snd_soc_update_bits(codec, 0x0f, 0x0300, 0x0100);
> >> +    snd_soc_write(codec, 0x09, 0xE000);
> >> +    snd_soc_write(codec, NAU8824_IRQ_SETTING, 0x1006);
> >> +    snd_soc_write(codec, 0x13, 0x1615);
> >> +    snd_soc_write(codec, 0x15, 0x0414);
> >> +    snd_soc_update_bits(codec, 0x16, 0xFF00, 0x5900);
> >> +    snd_soc_update_bits(codec, 0x66, 0x0070, 0x0060);

> > Too many magic numbers here I think and this looks like it should be
> > configured using platform data and/or the machine driver (what if the
> > headphone detection/IRQ aren't wired up?).  I'd also expect to see
> > reporting via the standard interfaces for jack reporting.

> The above initial settings are for jack detection. As for other jack
> detection flow, it will be implemented in machine driver but not be included in
> this application.

Please either remove this for now or implement it properly.

> ===========================================================================================
> The privileged confidential information contained in this email is intended for use only by the addressees as indicated by the original sender of this email. If you are not the addressee indicated in this email or are not responsible for delivery of the email to such a person, please kindly reply to the sender indicating this fact and delete all copies of it from your computer and network server immediately. Your cooperation is highly appreciated. It is advised that any unauthorized use of confidential information of Nuvoton is strictly prohibited; and any information in this email irrelevant to the official business of Nuvoton shall be deemed as neither given nor endorsed by Nuvoton.

Don't include noise like this in upstream communication, if your company
won't fix this then please use an external mail account for upstream
communication.

Attachment: signature.asc
Description: Digital signature