On Fri, Aug 07, 2015 at 01:44:12PM +0200, Sjoerd Simons wrote: > Changes in v2: > - Remove platform: module alias as it was unused Ugh, I see Paul Bolle replied to one of your earlier patches :/ Please just ignore him, he doesn't understand that we don't require machine definitions to be in tree. A platform: module alias is useful for any platform device. > +static inline struct rk_spdif_dev *to_info(struct snd_soc_dai *dai) > +{ > + return snd_soc_dai_get_drvdata(dai); > +} I'm not sure this is adding much to the two users but whatever. > +static int rk_spdif_snd_txctrl(struct rk_spdif_dev *spdif, int on) > +{ > + int ret; > + > + if (on) { > + ret = regmap_update_bits(spdif->regmap, SPDIF_DMACR, > + SPDIF_DMACR_TDE_ENABLE, > + SPDIF_DMACR_TDE_ENABLE); > + > + if (ret != 0) > + goto bail; > + > + ret = regmap_update_bits(spdif->regmap, SPDIF_XFER, > + SPDIF_XFER_TXS_START, > + SPDIF_XFER_TXS_START); This function has no common code, just two branches in an if/else that share nothing and is called from a single location. Why not just inline this into the trigger function? > + /* Set clock and calculate divider */ > + clk_set_rate(spdif->mclk, mclk); We should check the return value of clk_set_rate(). > + spdif->hclk = devm_clk_get(&pdev->dev, "spdif_hclk"); > + if (IS_ERR(spdif->hclk)) { > + dev_err(&pdev->dev, "Can't retrieve rk_spdif bus clock\n"); > + return PTR_ERR(spdif->hclk); > + } > + ret = clk_prepare_enable(spdif->hclk); > + if (ret) { > + dev_err(spdif->dev, "hclock enable failed %d\n", ret); > + return ret; > + } I notice that we don't manage the hclk over suspend/resume - should we perhaps be using regmap_init_mmio_clk() together with runtime PM to manage it? > + pm_runtime_enable(&pdev->dev); > + if (!pm_runtime_enabled(&pdev->dev)) { > + ret = rk_spdif_runtime_resume(&pdev->dev); > + if (ret) > + goto err_pm_disable; > + } The normal pattern with this is to start the device powered up then idle it - this does mean we need to bounce the power on but fitting in with the common pattern is good and it's less conditional code.
Attachment:
signature.asc
Description: Digital signature