Re: [alsa-devel] [PATCH v2] ASoC: fsl_esai: Add ESAI CPU DAI driver
Resent this because of losing attached file.
On Fri, Jan 10, 2014 at 10:32:52AM +0800, Nicolin Chen wrote:
On Thu, Jan 09, 2014 at 06:44:53PM +0000, Mark Brown wrote:
On Thu, Jan 09, 2014 at 06:57:58PM +0800, Nicolin Chen wrote:
+/**
- This function configures the ratio between MCLK (HCK) and BCLK (SCK)
- (For DAI Master Mode only)
- Note: Machine driver should calculate the ratio to call this function.
Only effective after calling set_dai_sysclk() to set HCK direction.- */
+static int fsl_esai_set_bclk_ratio(struct snd_soc_dai *dai, unsigned int ratio)
Why does the machine driver have to do this by hand, being able to override is fine but having sensible defaults is easier? Or does it actually do that and the comment just needs updating?
The divider part of ESAI is pretty complicated due to caring about three configure bits - ETO, ETI, HCKD. (I've attached a diagram to this mail.) So setting sysclk() alone is not enough to take care all the configurations. That's why I designed these two interfaces at the first place. And it should be hard to find a default situation.
But there is one approach to omit this calling for machine driver is to do the set_bclk_ratio() at this CPU DAI driver. And this might be a good idea because we can then separate the settings between PLAYBACK and CAPTURE, even though we might then need to check the Master or Slave state to apply the settings accordingly.
I'll try this idea in v3 if you feel it's plausible.
- ret = devm_snd_soc_register_component(&pdev->dev, &fsl_esai_component,
&fsl_esai_dai, 1);- if (ret) {
dev_err(&pdev->dev, "failed to register DAI: %d\n", ret);return ret;- }
- ret = imx_pcm_dma_init(pdev);
- if (ret)
dev_err(&pdev->dev, "failed to init imx pcm dma: %d\n", ret);- /* Reset ESAI unit */
- regmap_write(esai_priv->regmap, REG_ESAI_ECR, ESAI_ECR_ERST);
- /*
* We need to enable ESAI so as to access some of its registers.* Otherwise, we would fail to dump regmap from user space.*/- regmap_write(esai_priv->regmap, REG_ESAI_ECR, ESAI_ECR_ESAIEN);
I would expect to see the hardware initialisation before we start registering with the core otherwise the core might start trying to run prior to the hardware being initialised.
Will switch this initial code in v3 as your suggestion.
Thank you, Nicolin Chen
On Fri, Jan 10, 2014 at 10:35:39AM +0800, Nicolin Chen wrote:
Resent this because of losing attached file.
I don't think your previous mail got sent at all, or at least it must've been caught by spam filters here...
On Fri, Jan 10, 2014 at 10:32:52AM +0800, Nicolin Chen wrote:
On Thu, Jan 09, 2014 at 06:44:53PM +0000, Mark Brown wrote:
Why does the machine driver have to do this by hand, being able to override is fine but having sensible defaults is easier? Or does it actually do that and the comment just needs updating?
The divider part of ESAI is pretty complicated due to caring about three configure bits - ETO, ETI, HCKD. (I've attached a diagram to this mail.) So setting sysclk() alone is not enough to take care all the configurations. That's why I designed these two interfaces at the first place. And it should be hard to find a default situation.
Why is the machine driver going to be able to come up with a sensible configuration then? It's OK to support overriding the configuration where needed, my concern is about providing a default.
But there is one approach to omit this calling for machine driver is to do the set_bclk_ratio() at this CPU DAI driver. And this might be a good idea because we can then separate the settings between PLAYBACK and CAPTURE, even though we might then need to check the Master or Slave state to apply the settings accordingly.
This is about what I'd expect but then surely the next step is for the driver to choose a defualt BCLK ratio - that's how most drivers work, they try to generate the exact rate that is needed to clock the data. Are the bit clock shared between playback and capture?
I just received the 'applied' mail. But still want to confirm this topic to see how to refine the driver in the step ahead.
On Fri, Jan 10, 2014 at 12:04:39PM +0000, Mark Brown wrote:
On Fri, Jan 10, 2014 at 10:35:39AM +0800, Nicolin Chen wrote:
Resent this because of losing attached file.
I don't think your previous mail got sent at all, or at least it must've been caught by spam filters here...
On Fri, Jan 10, 2014 at 10:32:52AM +0800, Nicolin Chen wrote:
On Thu, Jan 09, 2014 at 06:44:53PM +0000, Mark Brown wrote:
Why does the machine driver have to do this by hand, being able to override is fine but having sensible defaults is easier? Or does it actually do that and the comment just needs updating?
The divider part of ESAI is pretty complicated due to caring about three configure bits - ETO, ETI, HCKD. (I've attached a diagram to this mail.) So setting sysclk() alone is not enough to take care all the configurations. That's why I designed these two interfaces at the first place. And it should be hard to find a default situation.
Why is the machine driver going to be able to come up with a sensible configuration then? It's OK to support overriding the configuration where needed, my concern is about providing a default.
But there is one approach to omit this calling for machine driver is to do the set_bclk_ratio() at this CPU DAI driver. And this might be a good idea because we can then separate the settings between PLAYBACK and CAPTURE, even though we might then need to check the Master or Slave state to apply the settings accordingly.
This is about what I'd expect but then surely the next step is for the driver to choose a defualt BCLK ratio - that's how most drivers work, they try to generate the exact rate that is needed to clock the data.
Does that mean I should call set_bclk() once in the startup() when !active to set a default bit clock rate to suit a common sample rate like 44100Hz? I'm a bit confused if so. Because the driver would call set_bclk() any way in the hw_params().
But your suggestion just reminds me of the slave mode in SSI driver as default mode. And I should patch ESAI to slave mode for default as well, shouldn't I?
Are the bit clock shared between playback and capture?
Only shared in synchronous mode and totally individual in asynchronous mode. Each of them can have their own HCK(MCLK) from different sources and derive their own SCK(BCLK).
Thank you, Nicolin
On Fri, Jan 10, 2014 at 09:03:39PM +0800, Nicolin Chen wrote:
On Fri, Jan 10, 2014 at 12:04:39PM +0000, Mark Brown wrote:
This is about what I'd expect but then surely the next step is for the driver to choose a defualt BCLK ratio - that's how most drivers work, they try to generate the exact rate that is needed to clock the data.
Does that mean I should call set_bclk() once in the startup() when !active to set a default bit clock rate to suit a common sample rate like 44100Hz? I'm a bit confused if so. Because the driver would call set_bclk() any way in the hw_params().
Right, any choice here needs to be deferred to hw_params() as you say.
But your suggestion just reminds me of the slave mode in SSI driver as default mode. And I should patch ESAI to slave mode for default as well, shouldn't I?
master/slave selection is kind of orthogonal here - the two bits of information that are normally needed are the MCLK to use (and its rate) and the sample rate/format (which give you the BCLK that is needed). Normally it's then possible to caculate a divider which generates BCLK from MCLK. Overriding is normally only needed if there are additional constraints on BCLK due to something like limitations in one of the devices or sample rates for the opposite direction if the BCLK is shared but LRCLK isn't.
Are the bit clock shared between playback and capture?
Only shared in synchronous mode and totally individual in asynchronous mode. Each of them can have their own HCK(MCLK) from different sources and derive their own SCK(BCLK).
OK, so in asynchronous mode there should be a good chance of a a default choice working since it won't restrict the other direction.
On Fri, Jan 10, 2014 at 01:26:42PM +0000, Mark Brown wrote:
On Fri, Jan 10, 2014 at 09:03:39PM +0800, Nicolin Chen wrote:
On Fri, Jan 10, 2014 at 12:04:39PM +0000, Mark Brown wrote:
This is about what I'd expect but then surely the next step is for the driver to choose a defualt BCLK ratio - that's how most drivers work, they try to generate the exact rate that is needed to clock the data.
Does that mean I should call set_bclk() once in the startup() when !active to set a default bit clock rate to suit a common sample rate like 44100Hz? I'm a bit confused if so. Because the driver would call set_bclk() any way in the hw_params().
Right, any choice here needs to be deferred to hw_params() as you say.
But your suggestion just reminds me of the slave mode in SSI driver as default mode. And I should patch ESAI to slave mode for default as well, shouldn't I?
master/slave selection is kind of orthogonal here - the two bits of information that are normally needed are the MCLK to use (and its rate) and the sample rate/format (which give you the BCLK that is needed). Normally it's then possible to caculate a divider which generates BCLK from MCLK. Overriding is normally only needed if there are additional constraints on BCLK due to something like limitations in one of the devices or sample rates for the opposite direction if the BCLK is shared but LRCLK isn't.
I think I start to understand the point here: If a user only needs to playback the default case - 44.1KHz for example, the driver can just configure all the dividers once at the beginning, not every time, so that we can save further register overriding operation or even complicated clock selection and divisor calculation, which obviously makes the procedure clean and reduces the system loading even if it might be just in a slight level.
Is this the reason, or maybe one of the reasons, to the defaults providing?
Thank you, Nicolin Chen
On Fri, Jan 10, 2014 at 11:48:25PM +0800, Nicolin Chen wrote:
I think I start to understand the point here: If a user only needs to playback the default case - 44.1KHz for example, the driver can just configure all the dividers once at the beginning, not every time, so that we can save further register overriding operation or even complicated clock selection and divisor calculation, which obviously makes the procedure clean and reduces the system loading even if it might be just in a slight level.
Is this the reason, or maybe one of the reasons, to the defaults providing?
The main thing is that if the DAI driver does it then it's less code in the machine drivers using it - what tends to happen otherwise is that quite a few machine drivers end up replicating the same logic. Hardware designers tend to do a lot of cut'n'paste with these things so even if the CODEC is different the clocking is often very similar.
On Fri, Jan 10, 2014 at 04:52:29PM +0000, Mark Brown wrote:
On Fri, Jan 10, 2014 at 11:48:25PM +0800, Nicolin Chen wrote:
I think I start to understand the point here: If a user only needs to playback the default case - 44.1KHz for example, the driver can just configure all the dividers once at the beginning, not every time, so that we can save further register overriding operation or even complicated clock selection and divisor calculation, which obviously makes the procedure clean and reduces the system loading even if it might be just in a slight level.
Is this the reason, or maybe one of the reasons, to the defaults providing?
The main thing is that if the DAI driver does it then it's less code in the machine drivers using it - what tends to happen otherwise is that quite a few machine drivers end up replicating the same logic. Hardware designers tend to do a lot of cut'n'paste with these things so even if the CODEC is different the clocking is often very similar.
Point taken. And it also depends on how common the defaults would be. I think I should try to figure out a comparably generic template for the clock selection and its rate settings here and also check other redundant places.
I learned another lesson today. Thank you indeed. Nicolin
participants (2)
-
Mark Brown -
Nicolin Chen