[1/2] ASoC: rockchip: i2s: add 8 channels capture and lrck-mode support
diff mbox

Message ID 1442979683-9441-2-git-send-email-sugar.zhang@rock-chips.com
State New
Headers show

Commit Message

Sugar Zhang Sept. 23, 2015, 3:41 a.m. UTC
support max 8 channels capture, please add property
'rockchip,capture-channels' in dts to enable this,
if not, support 2 channels capture default.

support lrck clk mode configuration, there are 3 modes:

 - txrx: lrck_tx and lrck_rx are different.
 - tx_share: lrck_tx is shared with lrck_rx.
 - rx_share: lrck_rx is shared with lrck_tx.

to enable this, please add property 'rockchip,lrck-mode' in dts,
if not, use 'txrx' lrck mode default.

Signed-off-by: Sugar Zhang <sugar.zhang@rock-chips.com>
---
 sound/soc/rockchip/rockchip_i2s.c | 48 +++++++++++++++++++++++++++++++++++++--
 sound/soc/rockchip/rockchip_i2s.h | 23 +++++++++++++++++++
 2 files changed, 69 insertions(+), 2 deletions(-)

Comments

Mark Brown Sept. 23, 2015, 4:24 p.m. UTC | #1
On Wed, Sep 23, 2015 at 11:41:22AM +0800, Sugar Zhang wrote:

> +	/* configure tx/rx lrck use mode */
> +	if (!of_property_read_u32(node, "rockchip,lrck-mode", &val)) {
> +		if (val >= LRCK_TXRX && val <= LRCK_RX_SHARE)
> +			regmap_update_bits(i2s->regmap, I2S_CKR,
> +					   I2S_CKR_TRCM_MASK,
> +					   I2S_CKR_TRCM(val));
> +	}

This looks like it's for a board configuration thing so I'd not really
expect this to be handled in a device specific property - it's fairly
common to have this situation and we already have the symmetric_rates
flag for the DAI to handle it (and if we do end up adding this property
we'd need the driver to set that flag so that the core can handle things
properly and make sure that userspace doesn't try to set different rates
in different directions).

My initial thought here is that the machine driver should be responsible
for setting this and then the DAI driver should check to see if
symmetric_rates are in use and configure itself appropriately.  Is there
a reason why this won't work here?
Sugar Zhang Sept. 28, 2015, 8:16 a.m. UTC | #2
Hi Mark Brown,

? 9/24/2015 00:24, Mark Brown ??:
> On Wed, Sep 23, 2015 at 11:41:22AM +0800, Sugar Zhang wrote:
>
>> +	/* configure tx/rx lrck use mode */
>> +	if (!of_property_read_u32(node, "rockchip,lrck-mode", &val)) {
>> +		if (val >= LRCK_TXRX && val <= LRCK_RX_SHARE)
>> +			regmap_update_bits(i2s->regmap, I2S_CKR,
>> +					   I2S_CKR_TRCM_MASK,
>> +					   I2S_CKR_TRCM(val));
>> +	}
>
> This looks like it's for a board configuration thing so I'd not really
> expect this to be handled in a device specific property - it's fairly
> common to have this situation and we already have the symmetric_rates
> flag for the DAI to handle it (and if we do end up adding this property
> we'd need the driver to set that flag so that the core can handle things
> properly and make sure that userspace doesn't try to set different rates
> in different directions).
>
> My initial thought here is that the machine driver should be responsible
> for setting this and then the DAI driver should check to see if
> symmetric_rates are in use and configure itself appropriately.  Is there
> a reason why this won't work here?
>

It's for i2s ip configuration, in the most situation, there is no need
to use this property, except one case:

In order to save gpio pins for other function use, we may use single
lrck(tx or rx) pin. of course, it depends on product design. when in i2s
slave mode, we need to configure this to share lrck with tx/rx inside 
i2s logic.

symmetric_rates flag works fine on rockchip platform, but it can't cover 
the above case.

Do you have any suggestion about this or maybe there is no need to 
upstream this special part?

Best Regards
Sugar
Mark Brown Sept. 30, 2015, 6:46 p.m. UTC | #3
On Mon, Sep 28, 2015 at 04:16:12PM +0800, sugar wrote:
> ? 9/24/2015 00:24, Mark Brown ??:

> >My initial thought here is that the machine driver should be responsible
> >for setting this and then the DAI driver should check to see if
> >symmetric_rates are in use and configure itself appropriately.  Is there
> >a reason why this won't work here?

> It's for i2s ip configuration, in the most situation, there is no need
> to use this property, except one case:

> In order to save gpio pins for other function use, we may use single
> lrck(tx or rx) pin. of course, it depends on product design. when in i2s
> slave mode, we need to configure this to share lrck with tx/rx inside i2s
> logic.

> symmetric_rates flag works fine on rockchip platform, but it can't cover the
> above case.

> Do you have any suggestion about this or maybe there is no need to upstream
> this special part?

What makes you say that the symmetric_rates flag can't be used to cover
this case?  What you describe above is hte normal reason for needing to
enforce symmetric_rates.  The driver should be able to check if the flag
has been set just as well as the core is.
Sugar Zhang Oct. 7, 2015, 8:01 a.m. UTC | #4
Sorry for late reply, we were in the National Day Holiday.

? 10/1/2015 02:46, Mark Brown ??:
> On Mon, Sep 28, 2015 at 04:16:12PM +0800, sugar wrote:
>> ? 9/24/2015 00:24, Mark Brown ??:
>
>>> My initial thought here is that the machine driver should be responsible
>>> for setting this and then the DAI driver should check to see if
>>> symmetric_rates are in use and configure itself appropriately.  Is there
>>> a reason why this won't work here?
>
>> It's for i2s ip configuration, in the most situation, there is no need
>> to use this property, except one case:
>
>> In order to save gpio pins for other function use, we may use single
>> lrck(tx or rx) pin. of course, it depends on product design. when in i2s
>> slave mode, we need to configure this to share lrck with tx/rx inside i2s
>> logic.
>
>> symmetric_rates flag works fine on rockchip platform, but it can't cover the
>> above case.
>
>> Do you have any suggestion about this or maybe there is no need to upstream
>> this special part?
>
> What makes you say that the symmetric_rates flag can't be used to cover
> this case?  What you describe above is hte normal reason for needing to
> enforce symmetric_rates.  The driver should be able to check if the flag
> has been set just as well as the core is.
>

Got it, How about the following modify?

if (dai->symmetric_rates)
	regmap_update_bits(i2s->regmap, I2S_CKR,
                            I2S_CKR_TRCM_MASK,
                            I2S_CKR_TRCM(val));
Mark Brown Oct. 7, 2015, 9:27 a.m. UTC | #5
On Wed, Oct 07, 2015 at 04:01:38PM +0800, sugar wrote:
> ? 10/1/2015 02:46, Mark Brown ??:

> >What makes you say that the symmetric_rates flag can't be used to cover
> >this case?  What you describe above is hte normal reason for needing to
> >enforce symmetric_rates.  The driver should be able to check if the flag
> >has been set just as well as the core is.

> Got it, How about the following modify?

> if (dai->symmetric_rates)
> 	regmap_update_bits(i2s->regmap, I2S_CKR,
>                            I2S_CKR_TRCM_MASK,
>                            I2S_CKR_TRCM(val));

Yes, something like that.  You'll need to check both links in the DAI
and the DAI link itself rather than just your own DAI but we should have
a helper function for that - I'll add one, look out for a patch shortly.
Mark Brown Oct. 7, 2015, 9:39 a.m. UTC | #6
On Wed, Oct 07, 2015 at 10:27:14AM +0100, Mark Brown wrote:
> On Wed, Oct 07, 2015 at 04:01:38PM +0800, sugar wrote:

> > if (dai->symmetric_rates)
> > 	regmap_update_bits(i2s->regmap, I2S_CKR,
> >                            I2S_CKR_TRCM_MASK,
> >                            I2S_CKR_TRCM(val));

> Yes, something like that.  You'll need to check both links in the DAI
> and the DAI link itself rather than just your own DAI but we should have
> a helper function for that - I'll add one, look out for a patch shortly.

Actually no, now I look at the code you probably want to just clone the
check from soc_pcm_apply_symmetry() so have that be:

	struct snd_soc_pcm_runtime *rtd = substream->private_data;

	if (rtd->dai_link->symmetrict_rates) {
	}

instead.
Sugar Zhang Oct. 7, 2015, 10:01 a.m. UTC | #7
? 10/7/2015 17:39, Mark Brown ??:
> On Wed, Oct 07, 2015 at 10:27:14AM +0100, Mark Brown wrote:
>> On Wed, Oct 07, 2015 at 04:01:38PM +0800, sugar wrote:
>
>>> if (dai->symmetric_rates)
>>> 	regmap_update_bits(i2s->regmap, I2S_CKR,
>>>                             I2S_CKR_TRCM_MASK,
>>>                             I2S_CKR_TRCM(val));
>
>> Yes, something like that.  You'll need to check both links in the DAI
>> and the DAI link itself rather than just your own DAI but we should have
>> a helper function for that - I'll add one, look out for a patch shortly.
>
> Actually no, now I look at the code you probably want to just clone the
> check from soc_pcm_apply_symmetry() so have that be:
>
> 	struct snd_soc_pcm_runtime *rtd = substream->private_data;
>
> 	if (rtd->dai_link->symmetrict_rates) {
> 	}
>
> instead.
>

OK, this will be done in patchset v2, thanks.
Caleb Crome Oct. 7, 2015, 1:04 p.m. UTC | #8
Hi sugar,
   Can the rockchip support more than 8 channels?  Ideally, we would like to capture 16 channels and playback 16 channels simultaneously.  

Thanks,  Caleb 

Sent from my iPhone

> On Oct 7, 2015, at 1:01 AM, sugar <sugar.zhang@rock-chips.com> wrote:
> 
> Sorry for late reply, we were in the National Day Holiday.
> 
>> ? 10/1/2015 02:46, Mark Brown ??:
>>> On Mon, Sep 28, 2015 at 04:16:12PM +0800, sugar wrote:
>>> ? 9/24/2015 00:24, Mark Brown ??:
>> 
>>>> My initial thought here is that the machine driver should be responsible
>>>> for setting this and then the DAI driver should check to see if
>>>> symmetric_rates are in use and configure itself appropriately.  Is there
>>>> a reason why this won't work here?
>> 
>>> It's for i2s ip configuration, in the most situation, there is no need
>>> to use this property, except one case:
>> 
>>> In order to save gpio pins for other function use, we may use single
>>> lrck(tx or rx) pin. of course, it depends on product design. when in i2s
>>> slave mode, we need to configure this to share lrck with tx/rx inside i2s
>>> logic.
>> 
>>> symmetric_rates flag works fine on rockchip platform, but it can't cover the
>>> above case.
>> 
>>> Do you have any suggestion about this or maybe there is no need to upstream
>>> this special part?
>> 
>> What makes you say that the symmetric_rates flag can't be used to cover
>> this case?  What you describe above is hte normal reason for needing to
>> enforce symmetric_rates.  The driver should be able to check if the flag
>> has been set just as well as the core is.
> 
> Got it, How about the following modify?
> 
> if (dai->symmetric_rates)
>    regmap_update_bits(i2s->regmap, I2S_CKR,
>                           I2S_CKR_TRCM_MASK,
>                           I2S_CKR_TRCM(val));
> 
> 
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
Sugar Zhang Oct. 8, 2015, 1:47 a.m. UTC | #9
Hi Caleb,

we haven't support 16 channels capture and playback yet, would you mind 
to detail the use case? if must, we may support this feature on next 
generation chip.

? 10/7/2015 21:04, Caleb Crome ??:
> Hi sugar,
>     Can the rockchip support more than 8 channels?  Ideally, we would like to capture 16 channels and playback 16 channels simultaneously.
>
> Thanks,  Caleb
>
> Sent from my iPhone
Caleb Crome Oct. 8, 2015, 2:36 a.m. UTC | #10
We make microphone arrays with lots of microphones.  Some as few as 2, many with 7 channels, and a few with 14 or more.  

Can you support 8 channels with 32 bits/sample?  That would work, we can just set the codecs for 16-bit samples, which would be 256 bits/frame.

Hope you had a great golden week holiday!

Thanks,  
    Caleb Crome




Sent from my iPhone

> On Oct 7, 2015, at 6:47 PM, sugar <sugar.zhang@rock-chips.com> wrote:
> 
> Hi Caleb,
> 
> we haven't support 16 channels capture and playback yet, would you mind to detail the use case? if must, we may support this feature on next generation chip.
> 
>> ? 10/7/2015 21:04, Caleb Crome ??:
>> Hi sugar,
>>    Can the rockchip support more than 8 channels?  Ideally, we would like to capture 16 channels and playback 16 channels simultaneously.
>> 
>> Thanks,  Caleb
>> 
>> Sent from my iPhone
> 
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
Sugar Zhang Oct. 8, 2015, 2:44 a.m. UTC | #11
Yes, we can support 8 channels with 32bits/sample, thanks.

? 10/8/2015 10:36, Caleb Crome ??:
> We make microphone arrays with lots of microphones.  Some as few as 2, many with 7 channels, and a few with 14 or more.
>
> Can you support 8 channels with 32 bits/sample?  That would work, we can just set the codecs for 16-bit samples, which would be 256 bits/frame.
>
> Hope you had a great golden week holiday!
>
> Thanks,
>      Caleb Crome
>
>
>
>
> Sent from my iPhone
Caleb Crome Oct. 8, 2015, 3:19 a.m. UTC | #12
That will work!  Do all your cortex-a parts support that?

Thanks, Caleb 
Sent from my iPhone

> On Oct 7, 2015, at 7:44 PM, sugar <sugar.zhang@rock-chips.com> wrote:
> 
> Yes, we can support 8 channels with 32bits/sample, thanks.
> 
>> ? 10/8/2015 10:36, Caleb Crome ??:
>> We make microphone arrays with lots of microphones.  Some as few as 2, many with 7 channels, and a few with 14 or more.
>> 
>> Can you support 8 channels with 32 bits/sample?  That would work, we can just set the codecs for 16-bit samples, which would be 256 bits/frame.
>> 
>> Hope you had a great golden week holiday!
>> 
>> Thanks,
>>     Caleb Crome
>> 
>> 
>> 
>> 
>> Sent from my iPhone
> 
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
Sugar Zhang Oct. 8, 2015, 3:59 a.m. UTC | #13
Only the new design support 8 channels capture now, the others support 2 
channels capture. but all of them support 32bits/sample.

On 10/8/2015 11:19, Caleb Crome wrote:
>   That will work!  Do all your cortex-a parts support that?
>
> Thanks, Caleb
> Sent from my iPhone

Patch
diff mbox

diff --git a/sound/soc/rockchip/rockchip_i2s.c b/sound/soc/rockchip/rockchip_i2s.c
index b936102..a8cb414 100644
--- a/sound/soc/rockchip/rockchip_i2s.c
+++ b/sound/soc/rockchip/rockchip_i2s.c
@@ -245,8 +245,34 @@  static int rockchip_i2s_hw_params(struct snd_pcm_substream *substream,
 		return -EINVAL;
 	}
 
-	regmap_update_bits(i2s->regmap, I2S_TXCR, I2S_TXCR_VDW_MASK, val);
-	regmap_update_bits(i2s->regmap, I2S_RXCR, I2S_RXCR_VDW_MASK, val);
+	switch (params_channels(params)) {
+	case 8:
+		val |= I2S_CHN_8;
+		break;
+	case 6:
+		val |= I2S_CHN_6;
+		break;
+	case 4:
+		val |= I2S_CHN_4;
+		break;
+	case 2:
+		val |= I2S_CHN_2;
+		break;
+	default:
+		dev_err(i2s->dev, "invalid channel: %d\n",
+			params_channels(params));
+		return -EINVAL;
+	}
+
+	if (substream->stream == SNDRV_PCM_STREAM_CAPTURE)
+		regmap_update_bits(i2s->regmap, I2S_RXCR,
+				   I2S_RXCR_VDW_MASK | I2S_RXCR_CSR_MASK,
+				   val);
+	else
+		regmap_update_bits(i2s->regmap, I2S_TXCR,
+				   I2S_TXCR_VDW_MASK | I2S_TXCR_CSR_MASK,
+				   val);
+
 	regmap_update_bits(i2s->regmap, I2S_DMACR, I2S_DMACR_TDL_MASK,
 			   I2S_DMACR_TDL(16));
 	regmap_update_bits(i2s->regmap, I2S_DMACR, I2S_DMACR_RDL_MASK,
@@ -415,10 +441,12 @@  static const struct regmap_config rockchip_i2s_regmap_config = {
 
 static int rockchip_i2s_probe(struct platform_device *pdev)
 {
+	struct device_node *node = pdev->dev.of_node;
 	struct rk_i2s_dev *i2s;
 	struct resource *res;
 	void __iomem *regs;
 	int ret;
+	int val;
 
 	i2s = devm_kzalloc(&pdev->dev, sizeof(*i2s), GFP_KERNEL);
 	if (!i2s) {
@@ -475,6 +503,22 @@  static int rockchip_i2s_probe(struct platform_device *pdev)
 			goto err_pm_disable;
 	}
 
+	/* refine capture channels */
+	if (!of_property_read_u32(node, "rockchip,capture-channels", &val)) {
+		if (val >= 2 && val <= 8)
+			rockchip_i2s_dai.capture.channels_max = val;
+		else
+			rockchip_i2s_dai.capture.channels_max = 2;
+	}
+
+	/* configure tx/rx lrck use mode */
+	if (!of_property_read_u32(node, "rockchip,lrck-mode", &val)) {
+		if (val >= LRCK_TXRX && val <= LRCK_RX_SHARE)
+			regmap_update_bits(i2s->regmap, I2S_CKR,
+					   I2S_CKR_TRCM_MASK,
+					   I2S_CKR_TRCM(val));
+	}
+
 	ret = devm_snd_soc_register_component(&pdev->dev,
 					      &rockchip_i2s_component,
 					      &rockchip_i2s_dai, 1);
diff --git a/sound/soc/rockchip/rockchip_i2s.h b/sound/soc/rockchip/rockchip_i2s.h
index 93f456f..0d285d1 100644
--- a/sound/soc/rockchip/rockchip_i2s.h
+++ b/sound/soc/rockchip/rockchip_i2s.h
@@ -49,6 +49,9 @@ 
  * RXCR
  * receive operation control register
 */
+#define I2S_RXCR_CSR_SHIFT	15
+#define I2S_RXCR_CSR(x)		(x << I2S_RXCR_CSR_SHIFT)
+#define I2S_RXCR_CSR_MASK	(3 << I2S_RXCR_CSR_SHIFT)
 #define I2S_RXCR_HWT		BIT(14)
 #define I2S_RXCR_SJM_SHIFT	12
 #define I2S_RXCR_SJM_R		(0 << I2S_RXCR_SJM_SHIFT)
@@ -75,6 +78,12 @@ 
  * CKR
  * clock generation register
 */
+#define I2S_CKR_TRCM_SHIFT	28
+#define I2S_CKR_TRCM(x)	(x << I2S_CKR_TRCM_SHIFT)
+#define I2S_CKR_TRCM_TXRX	(0 << I2S_CKR_TRCM_SHIFT)
+#define I2S_CKR_TRCM_TXSHARE	(1 << I2S_CKR_TRCM_SHIFT)
+#define I2S_CKR_TRCM_RXSHARE	(2 << I2S_CKR_TRCM_SHIFT)
+#define I2S_CKR_TRCM_MASK	(3 << I2S_CKR_TRCM_SHIFT)
 #define I2S_CKR_MSS_SHIFT	27
 #define I2S_CKR_MSS_MASTER	(0 << I2S_CKR_MSS_SHIFT)
 #define I2S_CKR_MSS_SLAVE	(1 << I2S_CKR_MSS_SHIFT)
@@ -207,6 +216,20 @@  enum {
 	ROCKCHIP_DIV_BCLK,
 };
 
+/* channel select */
+#define I2S_CSR_SHIFT	15
+#define I2S_CHN_2	(0 << I2S_CSR_SHIFT)
+#define I2S_CHN_4	(1 << I2S_CSR_SHIFT)
+#define I2S_CHN_6	(2 << I2S_CSR_SHIFT)
+#define I2S_CHN_8	(3 << I2S_CSR_SHIFT)
+
+/* lrck use mode */
+enum {
+	LRCK_TXRX = 0,
+	LRCK_TX_SHARE,
+	LRCK_RX_SHARE,
+};
+
 /* I2S REGS */
 #define I2S_TXCR	(0x0000)
 #define I2S_RXCR	(0x0004)