| Message ID | 20190426125925.04F3F3FB4A@swsrvapps-01.diasemi.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ASoC: da7219: Use clk_round_rate to handle enabled bclk/wclk case | expand |
On Fri, Apr 26, 2019 at 01:59:24PM +0100, Adam Thomson wrote: > + /* > + * Rounding the rate here avoids failure trying to set a new > + * rate on an already enabled wclk. In that instance this will > + * just set the same rate as is currently in use, and so should > + * continue without problem. > + */ > + sr = clk_round_rate(wclk, sr); > ret = clk_set_rate(wclk, sr); > if (ret) { > dev_err(component->dev, Don't we need to validate that the rounded rate is actually viable for the parameters we're trying to set here? If there's missing constraints causing something to try to do something unsupportable then we should return an error rather than silently accept.
On 27 April 2019 18:20, Mark Brown wrote: > On Fri, Apr 26, 2019 at 01:59:24PM +0100, Adam Thomson wrote: > > > + /* > > + * Rounding the rate here avoids failure trying to set a new > > + * rate on an already enabled wclk. In that instance this will > > + * just set the same rate as is currently in use, and so should > > + * continue without problem. > > + */ > > + sr = clk_round_rate(wclk, sr); > > ret = clk_set_rate(wclk, sr); > > if (ret) { > > dev_err(component->dev, > > Don't we need to validate that the rounded rate is actually viable for > the parameters we're trying to set here? If there's missing constraints > causing something to try to do something unsupportable then we should > return an error rather than silently accept. Thanks for directing my gaze to this again. Actually I don't think the SR should be rounded at all. If it doesn't match exactly it should fail so I'll remove the rounding here. Not sure what my brain was doing there. For the BCLK rate, I'll see what checking can be added there to make sure the word length is compatible with the 'rounded' BCLK rate.
On Mon, Apr 29, 2019 at 09:16:08AM +0000, Adam Thomson wrote: > On 27 April 2019 18:20, Mark Brown wrote: > > Don't we need to validate that the rounded rate is actually viable for > > the parameters we're trying to set here? If there's missing constraints > > causing something to try to do something unsupportable then we should > > return an error rather than silently accept. > Thanks for directing my gaze to this again. Actually I don't think the SR should > be rounded at all. If it doesn't match exactly it should fail so I'll remove the > rounding here. Not sure what my brain was doing there. Yeah, rounding is dubious with sample rate. Many applications will be able to tolerate *some* variation as there's tolerances in the crystals if nothing else but intentionally allowing it is a bit different.
diff --git a/sound/soc/codecs/da7219.c b/sound/soc/codecs/da7219.c index 5f5fa34..72268f5 100644 --- a/sound/soc/codecs/da7219.c +++ b/sound/soc/codecs/da7219.c @@ -1593,6 +1593,13 @@ static int da7219_hw_params(struct snd_pcm_substream *substream, sr = params_rate(params); if (da7219->master && wclk) { + /* + * Rounding the rate here avoids failure trying to set a new + * rate on an already enabled wclk. In that instance this will + * just set the same rate as is currently in use, and so should + * continue without problem. + */ + sr = clk_round_rate(wclk, sr); ret = clk_set_rate(wclk, sr); if (ret) { dev_err(component->dev, @@ -1621,6 +1628,14 @@ static int da7219_hw_params(struct snd_pcm_substream *substream, if (bclk) { bclk_rate = frame_size * sr; + /* + * Rounding the rate here avoids failure trying to set a + * new rate on an already enabled bclk. In that + * instance this will just set the same rate as is + * currently in use, and so should continue without + * problem. + */ + bclk_rate = clk_round_rate(bclk, bclk_rate); ret = clk_set_rate(bclk, bclk_rate); if (ret) { dev_err(component->dev, @@ -1927,9 +1942,6 @@ static unsigned long da7219_wclk_recalc_rate(struct clk_hw *hw, struct snd_soc_component *component = da7219->component; u8 fs = snd_soc_component_read32(component, DA7219_SR); - if (!da7219->master) - return 0; - switch (fs & DA7219_SR_MASK) { case DA7219_SR_8000: return 8000; @@ -2016,9 +2028,6 @@ static unsigned long da7219_bclk_recalc_rate(struct clk_hw *hw, u8 bclks_per_wclk = snd_soc_component_read32(component, DA7219_DAI_CLK_MODE); - if (!da7219->master) - return 0; - switch (bclks_per_wclk & DA7219_DAI_BCLKS_PER_WCLK_MASK) { case DA7219_DAI_BCLKS_PER_WCLK_32: return parent_rate * 32;
For some platforms where DA7219 is the DAI clock master, BCLK/WCLK will be set and enabled prior to the codec's hw_params() function being called. It is possible the platform requires a different BCLK/WCLK configuration than would be chosen by hw_params(), for example S16_LE format needed with a 64-bit frame to satisfy certain devices using the clocks. To handle those kinds of scenarios, the use of clk_round_rate() is now employed as part of hw_params(). If BCLK/WCLk are already enabled then this function will just return the currently set rates, so the subsequent calls to clk_set_rate() will succeed and nothing changes with regards to clocking. In addition the specific recalc_rate() implementations needed updating to always give back a real value, as those functions are called as part of the clk init code and a real value is needed for the clk_round_rate() calls to work as expected. Signed-off-by: Adam Thomson <Adam.Thomson.Opensource@diasemi.com> --- sound/soc/codecs/da7219.c | 21 +++++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-)