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

[John Hsu <KCHSU0@nuvoton.com>]

Hi,

On 8/15/2016 10:06 PM, Mark Brown wrote:
> On Mon, Aug 15, 2016 at 05:02:23PM +0800, John Hsu wrote:
>
> This looks very good overall, a few fairly small things but nothing too
> major.
>
>   
>> +	val = (u16 *)ucontrol->value.bytes.data;
>> +	reg = NAU8810_REG_EQ1;
>> +	for (i = 0; i < params->max / sizeof(u16); i++) {
>> +		regmap_read(nau8810->regmap, reg + i, &reg_val);
>> +		reg_val = cpu_to_be16(reg_val);
>> +		memcpy(val + i, &reg_val, sizeof(reg_val));
>> +	}
>>     
>
> This looks like it's trying to do regmap_raw_read()?  Raw I/O bypasses
> all the endianness conversions.
>
>   

We also try regmap_raw_read() but it fails because the value width
is 9 bits. Therefor, we make the functions by ourselves.

>> +static int nau8810_set_clkdiv(struct snd_soc_dai *dai, int div_id, int div)
>> +{
>> +	struct snd_soc_codec *codec = dai->codec;
>> +	struct nau8810 *nau8810 = snd_soc_codec_get_drvdata(codec);
>> +	struct regmap *regmap = nau8810->regmap;
>> +	struct nau8810_pll *pll = &nau8810->pll;
>> +
>> +	switch (nau8810->div_id) {
>> +	case NAU8810_MCLK_DIV_PLL:
>> +		/* master clock from PLL and enable PLL */
>>     
>
> I'd really not expect new drivers to need to have a set_clkdiv()
> operation.  Most things should just be worked out by the driver, that
> means less duplicated code in machine drivers and that things like
> simple-card have more chance of working for a device.  This one I'm not
> really sure what the divider is so it's hard to make specific
> recommendations.
>
>   

I agree to remove the function. The clock divider has done by PLL
function if the clock source through PLL.

>> +
>> +	case 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.
>> +		 */
>> +		break;
>>     
>
> This is obviously not good since it means that we just ignore anything
> that's set - if the caller is trying to set a divider we shouldn't just
> be silently discarding what they set.  It looks like this can just be
> removed since the driver is able to calcuate 
>
>   

Yes, it's able to calculate and remove it.

>> +	case NAU8810_BCLK_DIV:
>> +		regmap_update_bits(regmap, NAU8810_REG_CLOCK,
>> +			NAU8810_BCLKSEL_MASK, (div << NAU8810_BCLKSEL_SFT));
>> +		break;
>>     
>
> If this is really needed by anything the set_bclk_ratio() call is more
> appropriate.
>
>   

I can study it. The function is used seldom and maybe I won't
provide the function.

>> +static int nau8810_probe(struct snd_soc_codec *codec)
>> +{
>> +	return 0;
>> +}
>>     
>
> Remove empty functions, if they can reasonably be empty then the
> framework will handle them being missing.
>
>   

OK, I'll do it.

>> +static struct snd_soc_codec_driver soc_codec_dev_nau8810 = {
>>     
>
>   
>> +	.controls = nau8810_snd_controls,
>> +	.num_controls = ARRAY_SIZE(nau8810_snd_controls),
>> +	.dapm_widgets = nau8810_dapm_widgets,
>> +	.num_dapm_widgets = ARRAY_SIZE(nau8810_dapm_widgets),
>> +	.dapm_routes = nau8810_dapm_routes,
>> +	.num_dapm_routes = ARRAY_SIZE(nau8810_dapm_routes),
>>     
>
> Move this data into the component driver please.
>   

I don't understand about it clearly. Is the snd_soc_codec_driver
not component driver? Could you tell me more details? Thank you.