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

[Chih-Chiang Chang]

On 2015/3/4 äå 08:55, Mark Brown wrote:
> 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.
We cannot find suitable macro in file "include\sound\soc.h", so we want to add below two macro for our chip.
SOC_DOUBLE_L_R_VALUE
SOC_DOUBLE_L_R_TLV
> 
>>>> +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.
These large number of register setting is used to initial our codec, and some of other codec have the same behavior. We will remove few unnecessary register default setting and add some remark for registers.
> 
>>>> +    /* 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.
We will remove it.
> 
>> ===========================================================================================
>> 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.
Our MIS report they have disabled to append message in mail. Hope you do not see it in this mail.
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/