Messages in this thread | | | Date | Wed, 28 Oct 2020 10:39:12 +0100 | From | Matthias Reichl <> | Subject | Re: [PATCH] ASoC: bcm2835: Add enable/disable clock functions |
| |
On Wed, Oct 28, 2020 at 01:18:33AM -0700, Allen Martin wrote: > Hi, just checking if you had a chance to review this patch. > > On Sat, Oct 10, 2020 at 12:26 PM Allen Martin <armartin@gmail.com> wrote: > > > Add functions to control enable/disable of BCLK output of bcm2835 I2S > > controller so that BCLK output only starts when dma starts. This > > resolves issues of audio pop with DACs such as max98357 on rpi. The > > LRCLK output of bcm2835 only starts when the frame size has been > > configured and there is data in the FIFO. The max98357 dac makes a > > loud popping sound when BCLK is toggling but LRCLK is not.
I'm afraid that changing the clocking in the way you proposed has a high potential of breaking existing setups which need bclk to be present after prepare(). And it complicates the already rather convoluted clock setup even more. So I don't think this patch should be applied.
Since you mentioned max98357: have you properly connected and configured the sd-mode GPIO? This chip has very strict timing requirements and is known to "pop" without sd-mode wired up - see the datasheet and devicetree binding docs.
so long,
Hias
> > > > Signed-off-by: Allen Martin <armartin@gmail.com> > > --- > > sound/soc/bcm/bcm2835-i2s.c | 35 +++++++++++++++++++++++++++++++++++ > > 1 file changed, 35 insertions(+) > > > > diff --git a/sound/soc/bcm/bcm2835-i2s.c b/sound/soc/bcm/bcm2835-i2s.c > > index e6a12e271b07..5c8649864c0d 100644 > > --- a/sound/soc/bcm/bcm2835-i2s.c > > +++ b/sound/soc/bcm/bcm2835-i2s.c > > @@ -122,9 +122,27 @@ struct bcm2835_i2s_dev { > > struct regmap *i2s_regmap; > > struct clk *clk; > > bool clk_prepared; > > + bool clk_enabled; > > int clk_rate; > > }; > > > > +static void bcm2835_i2s_enable_clock(struct bcm2835_i2s_dev *dev) > > +{ > > + if (dev->clk_enabled) > > + return; > > + > > + regmap_update_bits(dev->i2s_regmap, BCM2835_I2S_MODE_A_REG, > > BCM2835_I2S_CLKDIS, 0); > > + dev->clk_enabled = true; > > +} > > + > > +static void bcm2835_i2s_disable_clock(struct bcm2835_i2s_dev *dev) > > +{ > > + if (dev->clk_enabled) > > + regmap_update_bits(dev->i2s_regmap, > > BCM2835_I2S_MODE_A_REG, BCM2835_I2S_CLKDIS, BCM2835_I2S_CLKDIS); > > + > > + dev->clk_enabled = false; > > +} > > + > > static void bcm2835_i2s_start_clock(struct bcm2835_i2s_dev *dev) > > { > > unsigned int master = dev->fmt & SND_SOC_DAIFMT_MASTER_MASK; > > @@ -145,6 +163,7 @@ static void bcm2835_i2s_start_clock(struct > > bcm2835_i2s_dev *dev) > > > > static void bcm2835_i2s_stop_clock(struct bcm2835_i2s_dev *dev) > > { > > + bcm2835_i2s_disable_clock(dev); > > if (dev->clk_prepared) > > clk_disable_unprepare(dev->clk); > > dev->clk_prepared = false; > > @@ -158,6 +177,7 @@ static void bcm2835_i2s_clear_fifos(struct > > bcm2835_i2s_dev *dev, > > uint32_t csreg; > > uint32_t i2s_active_state; > > bool clk_was_prepared; > > + bool clk_was_enabled; > > uint32_t off; > > uint32_t clr; > > > > @@ -176,6 +196,11 @@ static void bcm2835_i2s_clear_fifos(struct > > bcm2835_i2s_dev *dev, > > if (!clk_was_prepared) > > bcm2835_i2s_start_clock(dev); > > > > + /* Enable clock if not enabled */ > > + clk_was_enabled = dev->clk_enabled; > > + if (!clk_was_enabled) > > + bcm2835_i2s_enable_clock(dev); > > + > > /* Stop I2S module */ > > regmap_update_bits(dev->i2s_regmap, BCM2835_I2S_CS_A_REG, off, 0); > > > > @@ -207,6 +232,10 @@ static void bcm2835_i2s_clear_fifos(struct > > bcm2835_i2s_dev *dev, > > if (!timeout) > > dev_err(dev->dev, "I2S SYNC error!\n"); > > > > + /* Disable clock if it was not enabled before */ > > + if (!clk_was_enabled) > > + bcm2835_i2s_disable_clock(dev); > > + > > /* Stop clock if it was not running before */ > > if (!clk_was_prepared) > > bcm2835_i2s_stop_clock(dev); > > @@ -414,6 +443,8 @@ static int bcm2835_i2s_hw_params(struct > > snd_pcm_substream *substream, > > /* Clock should only be set up here if CPU is clock master */ > > if (bit_clock_master && > > (!dev->clk_prepared || dev->clk_rate != bclk_rate)) { > > + if (dev->clk_enabled) > > + bcm2835_i2s_disable_clock(dev); > > if (dev->clk_prepared) > > bcm2835_i2s_stop_clock(dev); > > > > @@ -534,6 +565,8 @@ static int bcm2835_i2s_hw_params(struct > > snd_pcm_substream *substream, > > mode |= BCM2835_I2S_FTXP | BCM2835_I2S_FRXP; > > } > > > > + if (!dev->clk_enabled) > > + mode |= BCM2835_I2S_CLKDIS; > > mode |= BCM2835_I2S_FLEN(frame_length - 1); > > mode |= BCM2835_I2S_FSLEN(framesync_length); > > > > @@ -668,6 +701,7 @@ static int bcm2835_i2s_trigger(struct > > snd_pcm_substream *substream, int cmd, > > case SNDRV_PCM_TRIGGER_RESUME: > > case SNDRV_PCM_TRIGGER_PAUSE_RELEASE: > > bcm2835_i2s_start_clock(dev); > > + bcm2835_i2s_enable_clock(dev); > > > > if (substream->stream == SNDRV_PCM_STREAM_CAPTURE) > > mask = BCM2835_I2S_RXON; > > @@ -839,6 +873,7 @@ static int bcm2835_i2s_probe(struct platform_device > > *pdev) > > > > /* get the clock */ > > dev->clk_prepared = false; > > + dev->clk_enabled = false; > > dev->clk = devm_clk_get(&pdev->dev, NULL); > > if (IS_ERR(dev->clk)) { > > dev_err(&pdev->dev, "could not get clk: %ld\n", > > -- > > 2.20.1 > > > >
| |