Re: [PATCH v4] ASoC: imx-wm8958: add imx-wm8958 machine driver

[Nicolin Chen <nicoleotsuka@gmail.com>]

On Tue, Mar 08, 2016 at 02:27:56PM +0800, Zidan Wang wrote:
> This is an initial imx-wm8958 device-tree-only machine driver.
> The sound card driver support three dai link, HIFI, VOICE and BT.
> We can configure the cpu dai from device tree, if present it
> will create corresponding sound card subdevice. So at least
> one cpu dai phandle should be set from device tree.
> 
> Signed-off-by: Zidan Wang <zidan.wang@freescale.com>
> ---
> v3->v4: remove the dummy cpu dai, support set cpu dai1/2/3 from device tree.
> 	When cpu-dai1/2/3 present it will create croresponding sound card
> 	subdevice. At least one cpu dai should be configured.

> +++ b/Documentation/devicetree/bindings/sound/imx-audio-wm8958.txt
> +Optional properties:
> +
> + - cpu-dai1		   : The phandle of CPU DAI1 controller. At least one
> +			     cpu dai is required.

Should indicate that it's for the aif1 of WM8958.

> +
> + - cpu-dai2                : The phandle of CPU DAI2 controller. At least one
> +			     cpu dai is required.
> +
> + - cpu-dai3                : The phandle of CPU DAI3 controller. At least one
> +			     cpu dai is required.

Ditto

> diff --git a/arch/arm/configs/imx_v6_v7_defconfig b/arch/arm/configs/imx_v6_v7_defconfig
> index 978c5de..7ac5856 100644
> --- a/arch/arm/configs/imx_v6_v7_defconfig
> +++ b/arch/arm/configs/imx_v6_v7_defconfig
> @@ -251,6 +251,7 @@ CONFIG_SND_SOC_FSL_ASRC=y
>  CONFIG_SND_IMX_SOC=y
>  CONFIG_SND_SOC_PHYCORE_AC97=y
>  CONFIG_SND_SOC_EUKREA_TLV320=y
> +CONFIG_SND_SOC_IMX_WM8958=y
>  CONFIG_SND_SOC_IMX_WM8962=y
>  CONFIG_SND_SOC_IMX_SGTL5000=y
>  CONFIG_SND_SOC_IMX_SPDIF=y

Shouldn't include this here.

> diff --git a/sound/soc/fsl/imx-wm8958.c b/sound/soc/fsl/imx-wm8958.c

> +#define HIFI_DAI (0)
> +#define VOICE_DAI (1)
> +#define BT_DAI (2)

We don't necessarily limit each AIF's role *unless* each AIF can only
act as the one listed above -- In general, AIF1_DAI, AIF2_DAI would be
more flexible for users IMO.

If you really want each role of the AIF to be clear, I suggest you to
add an extra property (eg. dai-link-name) in the DT bindings because
it does reflect the hardware connections according to the schematics.

> +struct imx_wm8958_data {
> +	struct snd_soc_dai_link dai_link[DAI_LINK_NUM];
> +	int num_links;
> +	struct snd_soc_card card;
> +	struct clk *mclk[WM8958_MCLK_MAX];

Since the Codec driver handles the mclk now, we don't need to maintain
it in the private data here any more.

> +	u32 mclk_freq[WM8958_MCLK_MAX];

unsigned long

> +
> +static struct snd_soc_ops imx_hifi_ops = {
> +	.hw_params = imx_wm8958_hw_params,
> +	.hw_free   = imx_wm8958_hw_free,
> +};
> +
> +static struct snd_soc_ops imx_voice_ops = {
> +	.hw_params = imx_wm8958_hw_params,
> +	.hw_free   = imx_wm8958_hw_free,
> +};

What's the difference between these two ops?

> +static int imx_wm8958_probe(struct platform_device *pdev)
> +{
> +	struct device_node *cpu_np[DAI_LINK_NUM], *codec_np;
> +	struct device_node *np = pdev->dev.of_node;
> +	struct platform_device *cpu_pdev[DAI_LINK_NUM];
> +	struct i2c_client *codec_dev;
> +	struct imx_wm8958_data *data;
> +	char tmp[8];
> +	int ret, i;
> +
> +	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < DAI_LINK_NUM; i++) {
> +		sprintf(tmp, "cpu-dai%d", i + 1);
> +		cpu_np[i] = of_parse_phandle(np, tmp, 0);
> +		if (cpu_np[i]) {
> +			cpu_pdev[i] = of_find_device_by_node(cpu_np[i]);
> +			if (!cpu_pdev[i]) {
> +				dev_err(&pdev->dev,
> +					"failed to get cpu dai%d platform device\n", i);
> +				ret = -EINVAL;
> +				goto fail;
> +			}
> +
> +			data->dai_link[data->num_links] = imx_wm8958_dai_link[i];
> +			data->dai_link[data->num_links].cpu_dai_name = dev_name(&cpu_pdev[i]->dev);
> +			data->dai_link[data->num_links++].platform_of_node = cpu_np[i];

Some lines here crossed 80-characters. If I were you, I would use:
		if (!cpu_np[i])
			continue;

And we don't necessarily maintain num_links in the private data since
it's not being used in any other function and we may still fetch it
from data->card.num_links even if we somehow need it later.