On Wed, Sep 11, 2019 at 10:42:39AM -0400, Shengjiu Wang wrote:

This looks good, a few minor comments below but nothing major -
it's mostly nits with the DT binding.

> --- /dev/null
> +++ b/sound/soc/fsl/fsl_mqs.c
> @@ -0,0 +1,336 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * ALSA SoC IMX MQS driver
> + *
> + * Copyright (C) 2014-2019 Freescale Semiconductor, Inc.
> + *

Please make the entire comment block a C++ comment so things look
neater.

> +     /* On i.MX6sx the MQS control register is in GPR domain
> +      * But in i.MX8QM/i.MX8QXP the control register is moved
> +      * to its own domain.
> +      */
> +     if (of_device_is_compatible(np, "fsl,imx8qm-mqs"))
> +             mqs_priv->use_gpr = false;
> +     else
> +             mqs_priv->use_gpr = true;

The GPR was listed as a required property in the binding document
but it is only needed here so the binding document should say
"required if compatible is...".

> +     } else {
> +             res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +             regs = devm_ioremap_resource(&pdev->dev, res);
> +             if (IS_ERR(regs))

You can use devm_platform_ioremap_resource() here.

> +             mqs_priv->ipg = devm_clk_get(&pdev->dev, "core");
> +             if (IS_ERR(mqs_priv->ipg)) {
> +                     dev_err(&pdev->dev, "failed to get the clock: %ld\n",
> +                             PTR_ERR(mqs_priv->ipg));
> +                     goto out;
> +             }

The core clock wasn't listed in the bindings document.

Attachment: signature.asc
Description: PGP signature

Reply via email to