Messages in this thread | | | Date | Mon, 23 Jan 2017 19:07:01 +0000 | From | Mark Brown <> | Subject | Re: [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. [unhandled content-type:application/pgp-signature] | |