Re: [alsa-devel] [PATCH V2] ASoC: fsl: add imx-wm8962 machine driver
On Thu, Jun 06, 2013 at 11:02:43AM -0300, Fabio Estevam wrote: > You could try something like (pass the real clock in the device tree): > http://permalink.gmane.org/gmane.linux.alsa.devel/107614 > (I will re-post this patch later) That's exactly what I'm suggesting. signature.asc Description: Digital signature ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [alsa-devel] [PATCH V2] ASoC: fsl: add imx-wm8962 machine driver
Hi Nicolin, On Thu, Jun 6, 2013 at 9:49 AM, Nicolin Chen wrote: > But I always got "failed to create fixed clk" error during system booting. > So I think it's pretty different to get a fixed clock with normal since > it's on the root_list of clock tree. > How can I get the clock here, or more specifically, any way to get the > rate of the fixed-rate-clk? You could try something like (pass the real clock in the device tree): http://permalink.gmane.org/gmane.linux.alsa.devel/107614 (I will re-post this patch later) Regards, Fabio Estevam ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH V2] ASoC: fsl: add imx-wm8962 machine driver
On Thu, Jun 06, 2013 at 08:49:53PM +0800, Nicolin Chen wrote:
> On Wed, Jun 05, 2013 at 12:55:44PM +0100, Mark Brown wrote:
> > Since it's easy to define a fixed rate clock (there's a generic driver
> > for that) I'd just require the user to provide a clock API clock and fix
> > the rate using that. This is going to be less error prone and makes the
> > code simpler.
> I tried to use fixed rate clock as below:
> data->codec_clk = devm_clk_get(&codec_dev->dev, NULL);
> if (IS_ERR(data->codec_clk)) {
> of_fixed_clk_setup(codec_np);
> data->codec_clk = clk_get(NULL, codec_np->name);
No, this is silly. Have the board define the clock in the DT.
signature.asc
Description: Digital signature
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH V2] ASoC: fsl: add imx-wm8962 machine driver
On Thu, Jun 06, 2013 at 12:39:20PM +0800, Nicolin Chen wrote: > On Wed, Jun 05, 2013 at 12:55:44PM +0100, Mark Brown wrote: > > > +- hp-det-gpios : The gpio port of Headphone detection. > > > +- mic-det-gpios: The gpio port of Micphone detection. > > I'd say a bit more about these - obviously the CODEC has its own > > accessory detection here, and I rather suspect that in fact the HP > > detection you have there is really jack detection. > hasn't been included in this patch though. So since hardware has pins here, > I think it's better to put in the binding doc as well. But I guess it'd be > better to put it into OPTIONAL area? Yes, they need to be optional and you need to explain what the function of these pins is so people can tell what they are. As I said I really do expect that the pin you have labelled headphone detect is really a jack detect pin. > > The sample rate checking can be done with the symmmetric_rates feature > > in the core. The sample_bits check can't be but it's something that a > > lot of systems probably ought to have so it ought to be factored out > > into the core too rather than open coded in the driver. > [Nic:] fsl_ssi.c uses symmmetric_rates, but last time I traced soc-pcm.c > and found the core only cares about symmmetric_rates during pcm_open(), > which means the startup() in dai/machine driver, while what I'm considering > is something like "arecord -Dhw:0 -r 44100 | aplay -Dhw:0 -r 48000": > The two startup() would run almost at the same time and each of them would > be earlier than hw_param() -- the exact point we can get the sample rate > from application, but before this point no sample-rate actually be set to > make hw_constraint(). Then the two hw_param() would continue their code and > successively call set_fll() twice with different sample rates. > But I agree that it's kinda ugly to put code here. So I think maybe I need > to drop this part of code and to think about another patch for the core? Yes, you certainly need to drop this patch. There is no way of fixing this with the current ALSA APIs, they are fundamentally racy and you will always have the possibility of failures during simultaneous startup for hardware with these limits. > > You're enabling and disabling the CODEC only while there's an audio > > stream active. This means that it's not possible to support analogue > > bypass paths (which the device can do) - you should probably also > > support enabling the FLL via set_bias_level() and use set_bias_level() > > to turn it off (which works in all cases). > [Nic] I've seen and tested set_bias_level() way as samsung/tobermory.c does. > But I find a flaw to the method that if I play two and more wav files like > 'aplay -Dhw:0 audio44Khz.wav audio48Khz.wav audio22Khz.wav', which needs to > set_fll() before each playback and to keep SYSCLK valid by changing its source > to MCLK after the playback, but I only found that hw_param() and hw_free() > are symmetrically called before/after each playback in this case. Two things here. One is that of course there's no reference counting on FLL enables, they just happen, and the other is that as you'd expect the bias level callbacks just won't happen for the second stream as the device is powered up. You only need to set the clocking up for the first stream as the clocking can't be changed when the device is active. signature.asc Description: Digital signature ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH V2] ASoC: fsl: add imx-wm8962 machine driver
On Wed, Jun 05, 2013 at 04:21:41PM +0800, Nicolin Chen wrote:
Looks pretty good, a few comments but they're all fairly minor.
> + WM8962 pins:
> + * HPOUTL
> + * HPOUTR
> + * SPKOUTL
> + * SPKOUTR
> + * MICBIAS
> + * IN3R
> + * DMIC
> + * DMICDAT
No need to list all these, just reference the CODEC (and add them to the
CODEC binding documentation if they're not there which they probably
aren't). This is generally good practice if there's a family of devices
sharing a driver and means that you don't have to worry about missing
all the other input pins as you've done here. :)
> +- hp-det-gpios : The gpio port of Headphone detection.
> +- mic-det-gpios: The gpio port of Micphone detection.
I'd say a bit more about these - obviously the CODEC has its own
accessory detection here, and I rather suspect that in fact the HP
detection you have there is really jack detection.
> +static int check_hw_params(struct snd_pcm_substream *substream,
> + snd_pcm_format_t sample_format, unsigned int sample_rate)
> +{
> + struct imx_priv *priv = &card_priv;
> + struct device *dev = substream->pcm->card->dev;
> +
> + substream->runtime->sample_bits =
> + snd_pcm_format_physical_width(sample_format);
> + substream->runtime->rate = sample_rate;
> + substream->runtime->format = sample_format;
> +
> + if (!priv->first_stream) {
> + priv->first_stream = substream;
> + } else {
> + priv->second_stream = substream;
> +
> + if (priv->first_stream->runtime->rate !=
> + priv->second_stream->runtime->rate) {
> + dev_err(dev, "KEEP THE SAME SAMPLE RATE: %d!\n",
> + priv->first_stream->runtime->rate);
> + return -EINVAL;
> + }
> + if (priv->first_stream->runtime->sample_bits !=
> + priv->second_stream->runtime->sample_bits) {
> + snd_pcm_format_t first_format =
> + priv->first_stream->runtime->format;
> +
> + dev_err(dev, "KEEP THE SAME FORMAT: %s!\n",
> + snd_pcm_format_name(first_format));
> + return -EINVAL;
> + }
> + }
The sample rate checking can be done with the symmmetric_rates feature
in the core. The sample_bits check can't be but it's something that a
lot of systems probably ought to have so it ought to be factored out
into the core too rather than open coded in the driver.
> + /*
> + * SYSCLK of wm8962's not disabled yet. If we wanna close FLL,
No '.
> + ret = snd_soc_dai_set_sysclk(codec_dai, WM8962_SYSCLK_MCLK,
> + data->clk_frequency, SND_SOC_CLOCK_IN);
> + if (ret < 0) {
> + dev_err(dev, "Failed to set SYSCLK: %d\n", ret);
> + return ret;
> + }
> +
> + /* Disable FLL and let codec do pm_runtime_put() */
> + ret = snd_soc_dai_set_pll(codec_dai, WM8962_FLL, 0, 0, 0);
> + if (ret < 0) {
> + dev_err(dev, "Failed to disable FLL: %d\n", ret);
> + return ret;
You're enabling and disabling the CODEC only while there's an audio
stream active. This means that it's not possible to support analogue
bypass paths (which the device can do) - you should probably also
support enabling the FLL via set_bias_level() and use set_bias_level()
to turn it off (which works in all cases).
> + data->codec_clk = clk_get(&codec_dev->dev, NULL);
> + if (IS_ERR(data->codec_clk)) {
devm_clk_get().
> + /* assuming clock enabled by default */
> + data->codec_clk = NULL;
> + ret = of_property_read_u32(codec_np, "clock-frequency",
> + &data->clk_frequency);
> + if (ret) {
> + dev_err(&codec_dev->dev,
> + "clock-frequency missing or invalid\n");
> + goto fail;
> + }
Since it's easy to define a fixed rate clock (there's a generic driver
for that) I'd just require the user to provide a clock API clock and fix
the rate using that. This is going to be less error prone and makes the
code simpler.
> + } else {
> + data->clk_frequency = clk_get_rate(data->codec_clk);
> + clk_prepare_enable(data->codec_clk);
Not needed immediately but if there is actually a clock we have control
of we might want to vary the frequency...
signature.asc
Description: Digital signature
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
