Re: [alsa-devel] [PATCH v2] ASoC: mxs-saif: add mclk enable/disable ops

On Thu, Dec 22, 2016 at 04:49:26PM +0000, Mans Rullgard wrote:

int mxs_saif_put_mclk(unsigned int saif_id) {

• struct mxs_saif *saif = mxs_saif[saif_id];
• u32 stat;

• struct clk *clk;

• if (!saif)

• return -EINVAL;
  

• clk = clk_get(NULL, "mxs_saif_mclk");
• if (IS_ERR(clk))

• return PTR_ERR(clk);
  

So, this *is* an in place refactoring but it's only half done in that we don't have any followup patches that move the clk_get() to device probe where it should be.

+static void mxs_saif_mclk_disable(struct clk_hw *hw) +{

• struct mxs_saif *saif = to_mxs_saif(hw);
• if (!saif->ongoing)

• __raw_writel(BM_SAIF_CTRL_RUN,
  

• 	     saif->base + SAIF_CTRL + MXS_CLR_ADDR);
  

• saif->mclk_in_use = 0;

+}

We silently ignore disables if the clock is in use? That seems error prone. I'd expect us to at least warn in that case.

+static unsigned long mxs_saif_mclk_recalc_rate(struct clk_hw *hw,

• 			       unsigned long parent_rate)
  

+{

• return clk_divider_ops.recalc_rate(hw, parent_rate);

+}

Can't we just assign these ops directly? Having to write wrapper functions like this looks like we're doing something wrong here.