Re: [PATCH] ASoC: nau8810: Add driver for Nuvoton codec chip NAU88C10

[John Hsu <KCHSU0@nuvoton.com>]

On 7/1/2016 6:04 PM, Mark Brown wrote:
> On Fri, Jul 01, 2016 at 11:34:31AM +0800, John Hsu wrote:
>   
>> On 6/28/2016 1:15 AM, Mark Brown wrote:
>>     
>
>   
>>> This looks like a regmap with 7 bit registers and 9 bit value.  Why
>>> aren't we just using the standard regmap support for this?
>>>       
>
>   
>> Yes, that is the i2c format of this codec. The format is not common,
>> and the register map only supports write but not supports read.
>> The driver only can read information from cache, but it can't read
>> the read-only register. Thus, we need to have our own read and write
>> function for codec.
>>     
>
> No, you don't - this is entirely normal for 7x9 regmaps, I've never seen
> such a device that supported readback.  Look at devices like wm8731 for
> examples.
>
>   

Sometimes, we need to know the codec information and need read it
from hardware, not cache. I'm afraid that it can't be done in this
case.

>>>> +	SOC_SINGLE("Digital Loopback Switch", NAU8810_REG_COMP,
>>>> +		NAU8810_ADDAP_SFT, 1, 0),
>>>>         
>
>   
>>> This looks like it should be a DAPM control.
>>>       
>
>   
>> The function is only for debug normally. The playback and capture
>> shouldn't enable the function. Thus, we only put it in the user
>> control.
>>     
>
> If it's for routing it should go into DAPM, someone might find a use for
> it and it'll stop confusion.
>
>   

I know the reason and move it to DAPM.

>>>> +	nau8810->div_id = div_id;
>>>> +	if (div_id != NAU8810_MCLK_DIV_MCLK)
>>>> +		/* Defer the master clock prescaler configuration to DAI
>>>> +		 * hardware parameter if master clock from MCLK because
>>>> +		 * it needs runtime fs information to get the proper div.
>>>> +		 */
>>>> +		ret = nau8810_config_clkdiv(nau8810, div, 0);
>>>> +
>>>> +	return ret;
>>>> +}
>>>>         
>
>   
>>> You shouldn't be implementing new set_clkdiv() operations, there's no
>>> point in having each machine driver figure out the internal clocking of
>>> the device.  Just specify the clocks coming into the device and have
>>> the driver figure out what to do with them.
>>>       
>
>   
>> We want to calculate the proper divide for MCLK as clock source.
>> The design needs sampling rate information for the calculation.
>> In the application sequence, there is no rate information in this
>> stage and it should defer until codec hardware parameter.
>>     
>
> That should be fine, you can do this in your hw_params() can't you?
>   

Yes, it can be done in hw_params().