[PATCH] ASoC: da7219: Use clk_round_rate to handle enabled bclk/wclk case

[PATCH] ASoC: da7219: Use clk_round_rate to handle enabled bclk/wclk case

[Adam Thomson]

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(-)

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;
-- 
1.9.1

Re: [PATCH] ASoC: da7219: Use clk_round_rate to handle enabled bclk/wclk case

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 694 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

RE: [PATCH] ASoC: da7219: Use clk_round_rate to handle enabled bclk/wclk case

[Adam Thomson]

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.

Re: [PATCH] ASoC: da7219: Use clk_round_rate to handle enabled bclk/wclk case

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 816 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]