Re: [PATCH v2] sound/soc/adi/axi-spdif.c: Support programmable master clock

[Mike Looijmans]

ïOn 12/10/2014 10:34 AM, Lars-Peter Clausen wrote:
> On 12/05/2014 01:37 PM, Mike Looijmans wrote:
> > If the master clock supports programmable rates, program it to generate
> > the desired frequency. Only apply constraints when the clock is fixed.
> > This allows proper clock generation for both 44100 and 48000 Hz based
> > sampling rates if the platform supports it.
> >
> > The clock frequency must be set before enabling it. Enabling the clock
> > was done in "startup", but that occurs before "hw_params" where the rate
> > is known. Enabling a programmable clock without first setting a valid
> > frequency may harm the system. Move the clock start to the hw_params
> > routine, and keep track of whether the clock has been started, because
> > shutdown may be called without having called hw_params first.
> > Starting the clock and enabling the SPDIF output AFTER programming the
> > dividers is a more logical order anyway.
> >
> > To detect if the source clock is fixed, the driver calls clk_round_rate
> > for two frequencies. If the results are equal, or if the call returns
> > an error, the driver assumes the clock is fixed.
> >
> > Signed-off-by: Mike Looijmans <mike.looijmans@xxxxxxxx>
>
> Hi,
>
> Sorry for the delay.
>
> [...]
> > +    /* Try to set the master clock */
> > +    clk_set_rate(spdif->clk_ref, rate * 128);
> > +
> >       clkdiv = DIV_ROUND_CLOSEST(clk_get_rate(spdif->clk_ref),
> >               rate * 64 * 2) - 1;
> >       clkdiv <<= AXI_SPDIF_CTRL_CLKDIV_OFFSET;
> > @@ -103,6 +108,14 @@ static int axi_spdif_hw_params(struct snd_pcm_substream
> > *substream,
> >       regmap_update_bits(spdif->regmap, AXI_SPDIF_REG_CTRL,
> >           AXI_SPDIF_CTRL_CLKDIV_MASK, clkdiv);
> >
> > +    ret = clk_prepare_enable(spdif->clk_ref);
>
> I'm still not convinced this is the right place. I do see your point. But it
> just feels wrong to enable the clock in hw_params. It's a bit of a dilemma.
> the startup callback is to early, hw_params is the wrong place and we can't
> put it in the trigger callback as the trigger callback can not sleep.

The clock interface suggests that we should do the clk_prepare in hw_params 
and the clk_enable in trigger then, since clk_prepare may sleep but clk_enable 
cannot. Which would complicate the clock state housekeeping because I'd have 
to keep track of prepare and enable states separately.

(Imagine the housekeeping on a board that could have two codecs running on one 
DAI using the clock generated by a third codec... I never even bothered to 
submit that...)

> But in any way hwparmas can be called multiple times, so you need to handle
> the case where the clock is already enabled.

A simple "if (spdif->clk_ref_running)" check would fix that. I wasn't aware of 
that though, I thought there'd be a shutdown before another hw_params.

> > +    if (ret)
> > +        return ret;
> > +    spdif->clk_ref_running = true;
> > +
> > +    regmap_update_bits(spdif->regmap, AXI_SPDIF_REG_CTRL,
> > +        AXI_SPDIF_CTRL_TXEN, AXI_SPDIF_CTRL_TXEN);
>
> This should probably be moved to the trigger callback though.


I'll create a v3.




Met vriendelijke groet / kind regards,

Mike Looijmans
System Expert


TOPIC Embedded Systems
Eindhovenseweg 32-C, NL-5683 KH Best
Postbus 440, NL-5680 AK Best
Telefoon: (+31) (0) 499 33 69 79
Telefax:  (+31) (0) 499 33 69 70
E-mail: mike.looijmans@xxxxxxxx
Website: www.topic.nl

Please consider the environment before printing this e-mail

Topic zoekt gedreven (embedded) software specialisten!
http://topic.nl/vacatures/topic-zoekt-software-engineers/

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/