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.