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:
quoted
On Thu, Jan 09, 2014 at 06:57:58PM +0800, Nicolin Chen wrote:
quoted
+/**
+ * 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.
quoted
quoted
+ 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
From: Mark Brown <broonie@kernel.org> Date: 2014-01-10 12:05:16
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:
quoted
On Thu, Jan 09, 2014 at 06:44:53PM +0000, Mark Brown wrote:
quoted
quoted
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?
quoted
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.
quoted
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:
quoted
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...
quoted
On Fri, Jan 10, 2014 at 10:32:52AM +0800, Nicolin Chen wrote:
quoted
On Thu, Jan 09, 2014 at 06:44:53PM +0000, Mark Brown wrote:
quoted
quoted
quoted
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?
quoted
quoted
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.
quoted
quoted
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
From: Mark Brown <broonie@kernel.org> Date: 2014-01-10 13:27:15
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:
quoted
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.
quoted
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:
quoted
On Fri, Jan 10, 2014 at 12:04:39PM +0000, Mark Brown wrote:
quoted
quoted
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.
quoted
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.
quoted
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
From: Mark Brown <broonie@kernel.org> Date: 2014-01-10 16:53:04
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:
quoted
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.
quoted
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