[alsa-devel] Request for comments for SND_SOC_IMX_WM8731

[Fabio Estevam]

On Fri, Oct 24, 2014 at 2:42 AM, Jonathan Bennett <jbscience87 at gmail.com> wrote:
> This patch is a continuation of my work on supporting the Utilite devices on
> the vanilla kernel. I've modified the code slightly to conform to
> established practice. Namely instead of using src-port and ext-port, we use
> mux-int-port and mux-ext-port.
>
> This driver has support for putting the codec in either master mode or slave
> mode. So far I've only had success with using slave mode. For trying to
> submit for kernel inclusion, should I strip out the code for codec master

Yes, just add codec slave support then.

> mode? As a kernel dev noob, any other comments are welcome.

Some suggestions:
- Run ./scripts/checkpatch on your patch and fix all the reported
errors/warnings
- Add the maintainers on Cc (Mark Brown, Nicolin Chen)
- In the subject, use something like: ASoC: fsl: Add support for
imx-wm8731 and a proper commit message
- Send the patch via git send-email

More comments below:

> +config SND_SOC_IMX_WM8731
> + tristate "SoC Audio support for i.MX boards with wm8731
> + depends on OF && I2C
> + select SND_SOC_WM8731
> + select SND_SOC_IMX_PCM_DMA
> + select SND_SOC_IMX_AUDMUX
> + select SND_SOC_FSL_SSI
> + select SND_SOC_FSL_UTILS

You can remove this UTILS option.

> +static int wm8731_mst_mode_init(struct imx_wm8731_data *data)
> +{
> + long rate;
> + struct clk *new_parent;
> + struct clk *ssi_clk;
> + struct i2c_client *codec_dev = data->codec_dev;
> +
> + new_parent = devm_clk_get(&codec_dev->dev, "cko2");
> + if (IS_ERR(new_parent)) {
> + pr_err("Could not get \"cko2\" clock \n");
> + return PTR_ERR(new_parent);
> + }
> +
> + ssi_clk = devm_clk_get(&codec_dev->dev, "cko");
> + if (IS_ERR(ssi_clk)) {
> + pr_err("Could not get \"cko\" clock \n");
> + return PTR_ERR(ssi_clk);

I am not sure that adding the clock handling here is the correct approach.

The dts you use looks like:

   codec: wm8731 at 1a {
      compatible = "wlf,wm8731";
      reg = <0x1a>;
      clocks = <&clks 173>, <&clks 158>, <&clks 201>, <&clks 200>;
      clock-names = "pll4", "imx-ssi.1", "cko", "cko2";
      AVDD-supply = <&reg_3p3v>;
      HPVDD-supply = <&reg_3p3v>;
      DCVDD-supply = <&reg_3p3v>;
      DBVDD-supply = <&reg_3p3v>;

,which does not match the current bindings for the wm8731. The wm8731
driver could be extended to handle the clocks if needed, instead of
handling them via machine driver.

> +static int imx_hifi_hw_params_slv_mode(struct snd_pcm_substream *substream,
> +       struct snd_pcm_hw_params *params)
> +{
> + struct snd_soc_pcm_runtime *rtd = substream->private_data;
> + struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
> + struct snd_soc_dai *codec_dai = rtd->codec_dai;
> + struct snd_soc_card *card = codec_dai->card;
> + struct imx_wm8731_data *data = snd_soc_card_get_drvdata(card);
> +
> + u32 dai_format;
> + snd_pcm_format_t sample_format;
> + unsigned int channels;
> + unsigned int tx_mask, rx_mask;
> + unsigned int sampling_rate;
> + unsigned int div_2, div_psr, div_pm;
> + int ret;
> +
> + sampling_rate = params_rate(params);
> + sample_format = params_format(params);
> +
> + channels = params_channels(params);
> + printk("%s:%s  sampling rate = %u  channels = %u \n", __FUNCTION__,

pr_debug here?

> + div_2 = 0;
> + div_psr = 0;
> + switch (sampling_rate) {
> + case 8000:
> + // 1x1x12

No // style comments. Comments should use teh /* bla bla bla */ style.

> +static int imx_wm8731_probe(struct platform_device *pdev)
> +{
> + struct device_node *ssi_np, *codec_np;
> + struct platform_device *ssi_pdev;
> + struct imx_priv *priv = &card_priv;
> + struct i2c_client *codec_dev;
> + struct imx_wm8731_data *data;
> + unsigned int src_port, ext_port;
> + unsigned int ssi_mode;
> + const char *ssi_mode_str;
> +
> + int ret;
> +
> + priv->pdev = pdev;
> +
> + ssi_np = of_parse_phandle(pdev->dev.of_node, "ssi-controller", 0);
> + codec_np = of_parse_phandle(pdev->dev.of_node, "audio-codec", 0);
> + if (!ssi_np || !codec_np) {
> + dev_err(&pdev->dev, "phandle missing or invalid\n");
> + ret = -EINVAL;
> + goto fail;

No need for jumping to 'fail', just return the error directly. Same on
the other locations.

> + }
> +
> + ssi_pdev = of_find_device_by_node(ssi_np);
> + if (!ssi_pdev) {
> + dev_err(&pdev->dev, "failed to find SSI platform device\n");
> + ret = -EINVAL;
> + goto fail;
> + }
> +
> + codec_dev = of_find_i2c_device_by_node(codec_np);
> + if (!codec_dev || !codec_dev->dev.driver) {
> + dev_err(&pdev->dev, "failed to find codec platform device\n");
> + ret = -EINVAL;
> + goto fail;
> + }
> +
> + data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
> + if (!data) {
> + ret = -ENOMEM;
> + goto fail;
> + }
> +
> + card_priv.data = data;
> +
> + data->codec_dev = codec_dev;
> +
> + data->dai.name = "HiFi";
> + data->dai.stream_name = "HiFi";
> + data->dai.codec_dai_name = "wm8731-hifi";
> + data->dai.codec_of_node = codec_np;
> + data->dai.cpu_dai_name = dev_name(&ssi_pdev->dev);
> + data->dai.platform_of_node = ssi_np;
> + data->dai.ops = &imx_hifi_ops;
> + data->dai.init = &imx_wm8731_init;
> +
> + ret = of_property_read_u32(pdev->dev.of_node, "mux-int-port", &src_port);
> + if (ret) {
> + dev_err(&pdev->dev, "failed to get \"mux-int-port\" value\n");
> + ret = -EINVAL;
> + goto fail;
> + }
> +
> + ret = of_property_read_u32(pdev->dev.of_node, "mux-ext-port", &ext_port);
> + if (ret) {
> + dev_err(&pdev->dev, "failed to get \"mux-ext-port\" value\n");
> + ret = -EINVAL;
> + goto fail;
> + }
> +
> + ret = of_property_read_string(ssi_np, "fsl,mode", &ssi_mode_str);
> + if (ret) {
> + dev_err(&pdev->dev, "failed to get \"fsl,mode\" value\n");
> + ret = -EINVAL;
> + goto fail;
> + }
> +
> + ssi_mode = strcmp(ssi_mode_str, "i2s-master");
> +
> + if (ssi_mode) {
> + /* Master Mode */
> + imx_audmux_config_mst_mode(src_port, ext_port);
> + wm8731_mst_mode_init(data);
> + data->clock_enable = wm8731_mst_mode_clock_enable;
> + imx_hifi_ops.hw_params = imx_hifi_hw_params_mst_mode;
> + imx_hifi_ops.startup = imx_hifi_startup_mst_mode;
> + } else {
> + /* Slave Mode */
> + imx_audmux_config_slv_mode(src_port, ext_port);
> + wm8731_slv_mode_init(data);
> + data->clock_enable = wm8731_slv_mode_clock_enable;
> + imx_hifi_ops.hw_params = imx_hifi_hw_params_slv_mode;
> + imx_hifi_ops.startup = imx_hifi_startup_slv_mode;
> + }
> +
> + data->card.dev = &pdev->dev;
> + ret = snd_soc_of_parse_card_name(&data->card, "model");
> + if (ret)
> + goto fail;
> +
> + ret = snd_soc_of_parse_audio_routing(&data->card, "audio-routing");

> + if (ssi_np)
> + of_node_put(ssi_np);

of_node_put can handle NULL, so no need for the if test.

> +
> + if (codec_np)
> + of_node_put(codec_np);

Same here.