mbox series

[v2,0/2] ASoC: nau8325: Modify driver code and dtschema.

Message ID 20240222083825.190854-1-wtli@nuvoton.com
Headers show
Series ASoC: nau8325: Modify driver code and dtschema. | expand

Message

Seven Lee Feb. 22, 2024, 8:38 a.m. UTC 
Remove the "Clock detection" switch control, and modify dtschema
for the explanation of "nuvoton,dac-vref".

Change:
V1 -> V2
 - Revise the driver description part for the C++ comment.
 - In the nau8325_clkdet_put function, modify the max variable to hard code.
 - Removed "Clock Detection" switch control.
 - modify the "ALC Enable" switch name.
 - Revise the dtschema for "nuvoton,dac-vref".

Seven Lee (2):
  ASoC: dt-bindings: Added schema for "nuvoton,nau8325"
  ASoC: nau8325: new driver

 .../bindings/sound/nuvoton,nau8325.yaml       |  82 ++
 sound/soc/codecs/nau8325.c                    | 856 ++++++++++++++++++
 sound/soc/codecs/nau8325.h                    | 391 ++++++++
 3 files changed, 1329 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/sound/nuvoton,nau8325.yaml
 create mode 100755 sound/soc/codecs/nau8325.c
 create mode 100644 sound/soc/codecs/nau8325.h

Comments

Mark Brown Feb. 22, 2024, 3:47 p.m. UTC | #1 
On Thu, Feb 22, 2024 at 04:38:25PM +0800, Seven Lee wrote:

A few very minor things below but basically this looks good.

> +static int nau8325_clksrc_choose(struct nau8325 *nau8325,
> +				 const struct nau8325_srate_attr **srate_table,
> +				 int *n1_sel, int *mult_sel, int *n2_sel)
> +{

> +proc_done:
> +	dev_err(nau8325->dev, "nau8325->fs=%d,range=0x%x, %s, (n1,mu,n2,dmu):(%d,%d,%d), MCLK_SRC=%uHz (%d)",
> +		nau8325->fs, (*srate_table)->range,
> +		(*srate_table)->max ? "MAX" : "MIN",
> +		*n1_sel == CLK_PROC_BYPASS ?
> +		CLK_PROC_BYPASS : mclk_n1_div[*n1_sel].param,
> +		*mult_sel == CLK_PROC_BYPASS ?
> +		CLK_PROC_BYPASS : 1 << mclk_n3_mult[*mult_sel].param,
> +		1 << mclk_n2_div[*n2_sel].param,
> +		(*srate_table)->mclk_src[ratio],
> +		(*srate_table)->mclk_src[ratio] / nau8325->fs);

This should be a dev_dbg(), dev_err() will be very noisy in normal
operation.

> +static int nau8325_set_fmt(struct snd_soc_dai *dai, unsigned int fmt)
> +{
> +	struct snd_soc_component *component = dai->component;
> +	struct nau8325 *nau8325 = snd_soc_component_get_drvdata(component);
> +	unsigned int ctrl1_val = 0;
> +
> +	switch (fmt & SND_SOC_DAIFMT_MASTER_MASK) {
> +	case SND_SOC_DAIFMT_CBS_CFS:
> +		break;

Please use the modern _CBC_CFC defines.

> +	ret = regmap_read(nau8325->regmap, NAU8325_R02_DEVICE_ID, &value);
> +	if (ret) {
> +		dev_err(dev, "Failed to read device id (%d)", ret);
> +		goto err;
> +	}

Probably a good idea to check that the device ID is what we expected
too.
Seven Lee March 1, 2024, 6:35 a.m. UTC | #2 
Mark Brown 於 2/22/2024 11:47 PM 寫道:
> On Thu, Feb 22, 2024 at 04:38:25PM +0800, Seven Lee wrote:
>
> A few very minor things below but basically this looks good.
>
>> +static int nau8325_clksrc_choose(struct nau8325 *nau8325,
>> +                             const struct nau8325_srate_attr **srate_table,
>> +                             int *n1_sel, int *mult_sel, int *n2_sel)
>> +{
>> +proc_done:
>> +    dev_err(nau8325->dev, "nau8325->fs=%d,range=0x%x, %s, (n1,mu,n2,dmu):(%d,%d,%d), MCLK_SRC=%uHz (%d)",
>> +            nau8325->fs, (*srate_table)->range,
>> +            (*srate_table)->max ? "MAX" : "MIN",
>> +            *n1_sel == CLK_PROC_BYPASS ?
>> +            CLK_PROC_BYPASS : mclk_n1_div[*n1_sel].param,
>> +            *mult_sel == CLK_PROC_BYPASS ?
>> +            CLK_PROC_BYPASS : 1 << mclk_n3_mult[*mult_sel].param,
>> +            1 << mclk_n2_div[*n2_sel].param,
>> +            (*srate_table)->mclk_src[ratio],
>> +            (*srate_table)->mclk_src[ratio] / nau8325->fs);
> This should be a dev_dbg(), dev_err() will be very noisy in normal
> operation.

okay, I will make the correction to dev_dbg().

>
>> +static int nau8325_set_fmt(struct snd_soc_dai *dai, unsigned int fmt)
>> +{
>> +    struct snd_soc_component *component = dai->component;
>> +    struct nau8325 *nau8325 = snd_soc_component_get_drvdata(component);
>> +    unsigned int ctrl1_val = 0;
>> +
>> +    switch (fmt & SND_SOC_DAIFMT_MASTER_MASK) {
>> +    case SND_SOC_DAIFMT_CBS_CFS:
>> +            break;
> Please use the modern _CBC_CFC defines.

okay, I will revise.

>
>> +    ret = regmap_read(nau8325->regmap, NAU8325_R02_DEVICE_ID, &value);
>> +    if (ret) {
>> +            dev_err(dev, "Failed to read device id (%d)", ret);
>> +            goto err;
>> +    }
> Probably a good idea to check that the device ID is what we expected
> too.

okay, thanks.