On Fri, 2020-08-14 at 17:01 +0100, Mark Brown wrote: > On Fri, Aug 14, 2020 at 06:45:53PM +0800, Jiaxin Yu wrote: > > This looks mostly good, a couple of very small things: > > > + ret = regulator_enable(priv->avdd_reg); > > + if (ret) { > > + dev_err(&pdev->dev, "%s(), failed to enable regulator!\n", > > + __func__); > > + return ret; > > + } > > You need a remove() function to undo this enable. > Ok, I will add a remove() function to do regulator_disable() and snd_soc_unregister_component(). > > + dev_info(&pdev->dev, "%s(), dev name %s\n", > > + __func__, dev_name(&pdev->dev)); > > This isn't really adding anything, just remove it - it's not reading > info from the hardware or anything. Yes, it was unnecessary, removed it in PATCH v4.