[1/1] ASoC: codecs: max98090: Allow dsp_a mode

  TDM mode for max98090 is dsp_a compatible. So allow it.

Signed-off-by: Maxim Kochetkov <fido_max@inbox.ru>
---
 sound/soc/codecs/max98090.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
  

On Wed, Jun 21, 2023 at 02:53:27PM +0300, Maxim Kochetkov wrote:

> TDM mode for max98090 is dsp_a compatible. So allow it.

>  		case SND_SOC_DAIFMT_DSP_A:
> -			/* Not supported mode */
> +			break;

This is configuring DSP A identically to left justified mode, it looks
like the format configuration needs at least some interlock with the TDM
configuration.
  

On 21.06.2023 15:26, Mark Brown wrote:
> On Wed, Jun 21, 2023 at 02:53:27PM +0300, Maxim Kochetkov wrote:
> 
>> TDM mode for max98090 is dsp_a compatible. So allow it.
> 
>>   		case SND_SOC_DAIFMT_DSP_A:
>> -			/* Not supported mode */
>> +			break;
> 
> This is configuring DSP A identically to left justified mode, it looks
> like the format configuration needs at least some interlock with the TDM
> configuration.

According to datasheet MAX98090 supports only DSP_A (L data MSB after 
FRM LRC) TDM mode. Allowing this mode will let us proper configure CPU 
audio node via DT. Actual TDM mode activation is performed in 
max98090_set_tdm_slot() via M98090_REG_TDM_FORMAT/M98090_REG_TDM_CONTROL 
registers.
  

On Wed, Jun 21, 2023 at 04:02:34PM +0300, Maxim Kochetkov wrote:
> On 21.06.2023 15:26, Mark Brown wrote:

> > This is configuring DSP A identically to left justified mode, it looks
> > like the format configuration needs at least some interlock with the TDM
> > configuration.

> According to datasheet MAX98090 supports only DSP_A (L data MSB after FRM
> LRC) TDM mode. Allowing this mode will let us proper configure CPU audio
> node via DT. Actual TDM mode activation is performed in
> max98090_set_tdm_slot() via M98090_REG_TDM_FORMAT/M98090_REG_TDM_CONTROL
> registers.

I'm saying there should be some interlock between these two settings, if
nothing else setting DSP A mode should force TDM mode with automatically
configured slot sizes.
  

On 21.06.2023 16:18, Mark Brown wrote:
> On Wed, Jun 21, 2023 at 04:02:34PM +0300, Maxim Kochetkov wrote:
>> On 21.06.2023 15:26, Mark Brown wrote:
> 
>>> This is configuring DSP A identically to left justified mode, it looks
>>> like the format configuration needs at least some interlock with the TDM
>>> configuration.
> 
>> According to datasheet MAX98090 supports only DSP_A (L data MSB after FRM
>> LRC) TDM mode. Allowing this mode will let us proper configure CPU audio
>> node via DT. Actual TDM mode activation is performed in
>> max98090_set_tdm_slot() via M98090_REG_TDM_FORMAT/M98090_REG_TDM_CONTROL
>> registers.
> 
> I'm saying there should be some interlock between these two settings, if
> nothing else setting DSP A mode should force TDM mode with automatically
> configured slot sizes.

At this time there is no any interlock for TDM mode in MAX98090 driver. 
We can specify dai-tdm-slot-* properties in DT and .set_tdm_slot() will 
be called to setup TDM mode. And SND_SOC_DAIFMT cannot affect it. I 
checked other codecs drivers: most of them performs TDM setup this way. 
So why do we need such interlock right now?
  

On Wed, Jun 21, 2023 at 04:55:18PM +0300, Maxim Kochetkov wrote:
> On 21.06.2023 16:18, Mark Brown wrote:

> > I'm saying there should be some interlock between these two settings, if
> > nothing else setting DSP A mode should force TDM mode with automatically
> > configured slot sizes.

> At this time there is no any interlock for TDM mode in MAX98090 driver. We

Yes, that's the problem I am identifying.  The driver allows TDM mode to
be configured independently of the DAI format but the two are related.

> can specify dai-tdm-slot-* properties in DT and .set_tdm_slot() will be
> called to setup TDM mode. And SND_SOC_DAIFMT cannot affect it. I checked
> other codecs drivers: most of them performs TDM setup this way. So why do we
> need such interlock right now?

A lot of devices support TDM modes with other DAI formats, or allow the
mode that is required for TDM to be configured even without doing TDM
setup.  Some always configure TDM like I'm suggesting, with the explicit
TDM configuration just being an override.  Some are just buggy and
nobody noticed.  The issue is that the driver will claim to have
configured DSP A mode but actually done something else unless the user
also configures TDM.
  

On 21.06.2023 17:01, Mark Brown wrote:
> On Wed, Jun 21, 2023 at 04:55:18PM +0300, Maxim Kochetkov wrote:
>> On 21.06.2023 16:18, Mark Brown wrote:
> 
>>> I'm saying there should be some interlock between these two settings, if
>>> nothing else setting DSP A mode should force TDM mode with automatically
>>> configured slot sizes.
> 
>> At this time there is no any interlock for TDM mode in MAX98090 driver. We
> 
> Yes, that's the problem I am identifying.  The driver allows TDM mode to
> be configured independently of the DAI format but the two are related.

But DSP_A mode is just bit/frame format. It is just compatible with TDM.

> 
>> can specify dai-tdm-slot-* properties in DT and .set_tdm_slot() will be
>> called to setup TDM mode. And SND_SOC_DAIFMT cannot affect it. I checked
>> other codecs drivers: most of them performs TDM setup this way. So why do we
>> need such interlock right now?
> 
> A lot of devices support TDM modes with other DAI formats, or allow the
> mode that is required for TDM to be configured even without doing TDM
> setup.  Some always configure TDM like I'm suggesting, with the explicit
> TDM configuration just being an override.  Some are just buggy and
> nobody noticed.  The issue is that the driver will claim to have
> configured DSP A mode but actually done something else unless the user
> also configures TDM.

Yep. But we have to specify TDM parameters (slot masks, slot width, etc) 
any way. Because there is no default TDM configuration like I2S and so. 
And pure DSP_A/B mode just have no sense.

Anyway. What do you suggest? Should I perform some refactoring for the 
driver? Should I move M98090_REG_TDM_FORMAT/M98090_REG_TDM_CONTROL 
registers setup to the max98090_dai_set_fmt()?
  

On Wed, Jun 21, 2023 at 05:28:41PM +0300, Maxim Kochetkov wrote:

> Yep. But we have to specify TDM parameters (slot masks, slot width, etc) any
> way. Because there is no default TDM configuration like I2S and so. And pure
> DSP_A/B mode just have no sense.

No, they make perfect sense and are widely supported - the sample size
is chosen based on the hwparams instead of being forced by configuring
TDM mode.

> Anyway. What do you suggest? Should I perform some refactoring for the
> driver? Should I move M98090_REG_TDM_FORMAT/M98090_REG_TDM_CONTROL registers
> setup to the max98090_dai_set_fmt()?

To repeat:

> > > I'm saying there should be some interlock between these two settings, if
> > > nothing else setting DSP A mode should force TDM mode with automatically
> > > configured slot sizes.
  

On 21.06.2023 17:35, Mark Brown wrote:
> To repeat:
> 
>>>> I'm saying there should be some interlock between these two settings, if
>>>> nothing else setting DSP A mode should force TDM mode with automatically
>>>> configured slot sizes.

Ok. How about that:
---
  sound/soc/codecs/max98090.c | 52 ++++++++++++++++++++-----------------
  sound/soc/codecs/max98090.h |  2 ++
  2 files changed, 30 insertions(+), 24 deletions(-)

diff --git a/sound/soc/codecs/max98090.c b/sound/soc/codecs/max98090.c
index 142083b13ac3..c30305269514 100644
--- a/sound/soc/codecs/max98090.c
+++ b/sound/soc/codecs/max98090.c
@@ -1582,7 +1582,7 @@ static int max98090_dai_set_fmt(struct snd_soc_dai *codec_dai,
  	struct snd_soc_component *component = codec_dai->component;
  	struct max98090_priv *max98090 = snd_soc_component_get_drvdata(component);
  	struct max98090_cdata *cdata;
-	u8 regval;
+	u8 regval, tdm_regval;

  	max98090->dai_fmt = fmt;
  	cdata = &max98090->dai[0];
@@ -1591,6 +1591,7 @@ static int max98090_dai_set_fmt(struct snd_soc_dai *codec_dai,
  		cdata->fmt = fmt;

  		regval = 0;
+		tdm_regval = 0;
  		switch (fmt & SND_SOC_DAIFMT_CLOCK_PROVIDER_MASK) {
  		case SND_SOC_DAIFMT_CBC_CFC:
  			/* Set to consumer mode PLL - MAS mode off */
@@ -1636,7 +1637,8 @@ static int max98090_dai_set_fmt(struct snd_soc_dai *codec_dai,
  			regval |= M98090_RJ_MASK;
  			break;
  		case SND_SOC_DAIFMT_DSP_A:
-			/* Not supported mode */
+			tdm_regval |= M98090_TDM_MASK;
+			break;
  		default:
  			dev_err(component->dev, "DAI format unsupported");
  			return -EINVAL;
@@ -1665,11 +1667,20 @@ static int max98090_dai_set_fmt(struct snd_soc_dai *codec_dai,
  		 * seen for the case of TDM mode. The remaining cases have
  		 * normal logic.
  		 */
-		if (max98090->tdm_slots > 1)
+		if (tdm_regval)
  			regval ^= M98090_BCI_MASK;

  		snd_soc_component_write(component,
  			M98090_REG_INTERFACE_FORMAT, regval);
+
+		regval = 0;
+		if (tdm_regval)
+			regval = max98090->tdm_lslot << M98090_TDM_SLOTL_SHIFT |
+				 max98090->tdm_rslot << M98090_TDM_SLOTR_SHIFT |
+				 0 << M98090_TDM_SLOTDLY_SHIFT;
+
+		snd_soc_component_write(component, M98090_REG_TDM_FORMAT, regval);
+		snd_soc_component_write(component, M98090_REG_TDM_CONTROL, tdm_regval);
  	}

  	return 0;
@@ -1680,33 +1691,23 @@ static int max98090_set_tdm_slot(struct snd_soc_dai *codec_dai,
  {
  	struct snd_soc_component *component = codec_dai->component;
  	struct max98090_priv *max98090 = snd_soc_component_get_drvdata(component);
-	struct max98090_cdata *cdata;
-	cdata = &max98090->dai[0];

  	if (slots < 0 || slots > 4)
  		return -EINVAL;

-	max98090->tdm_slots = slots;
-	max98090->tdm_width = slot_width;
+	if (slot_width != 16)
+		return -EINVAL;

-	if (max98090->tdm_slots > 1) {
-		/* SLOTL SLOTR SLOTDLY */
-		snd_soc_component_write(component, M98090_REG_TDM_FORMAT,
-			0 << M98090_TDM_SLOTL_SHIFT |
-			1 << M98090_TDM_SLOTR_SHIFT |
-			0 << M98090_TDM_SLOTDLY_SHIFT);
-
-		/* FSW TDM */
-		snd_soc_component_update_bits(component, M98090_REG_TDM_CONTROL,
-			M98090_TDM_MASK,
-			M98090_TDM_MASK);
-	}
+	if (rx_mask != tx_mask)
+		return -EINVAL;

-	/*
-	 * Normally advisable to set TDM first, but this permits either order
-	 */
-	cdata->fmt = 0;
-	max98090_dai_set_fmt(codec_dai, max98090->dai_fmt);
+	if (!rx_mask)
+		return -EINVAL;
+
+	max98090->tdm_slots = slots;
+	max98090->tdm_width = slot_width;
+	max98090->tdm_lslot = ffs(rx_mask) - 1;
+	max98090->tdm_rslot = fls(rx_mask) - 1;

  	return 0;
  }
@@ -2411,6 +2412,9 @@ static int max98090_probe(struct snd_soc_component *component)
  	max98090->pa1en = 0;
  	max98090->pa2en = 0;

+	max98090->tdm_lslot = 0;
+	max98090->tdm_rslot = 1;
+
  	ret = snd_soc_component_read(component, M98090_REG_REVISION_ID);
  	if (ret < 0) {
  		dev_err(component->dev, "Failed to read device revision: %d\n",
diff --git a/sound/soc/codecs/max98090.h b/sound/soc/codecs/max98090.h
index a197114b0dad..2d2971acd150 100644
--- a/sound/soc/codecs/max98090.h
+++ b/sound/soc/codecs/max98090.h
@@ -1534,6 +1534,8 @@ struct max98090_priv {
  	unsigned int dai_fmt;
  	int tdm_slots;
  	int tdm_width;
+	int tdm_lslot;
+	int tdm_rslot;
  	u8 lin_state;
  	unsigned int pa1en;
  	unsigned int pa2en;
  

On Wed, Jun 21, 2023 at 10:57:18PM +0300, Maxim Kochetkov wrote:
> On 21.06.2023 17:35, Mark Brown wrote:

> > > > > I'm saying there should be some interlock between these two settings, if
> > > > > nothing else setting DSP A mode should force TDM mode with automatically
> > > > > configured slot sizes.

> Ok. How about that:
> ---
>  sound/soc/codecs/max98090.c | 52 ++++++++++++++++++++-----------------
>  sound/soc/codecs/max98090.h |  2 ++
>  2 files changed, 30 insertions(+), 24 deletions(-)

That looks plausible, yes.

I do note that the driver ignores tdm_width (and the entire TDM
configuration) when configuring BCLK, I guess it only works in clock
consumer mode for TDM?  If that's the case there should really be some
validation, and there should probably be a check for slot width being 16
since that looks like the only thing supported.  Those were already
broken though.
  

On 21.06.2023 23:46, Mark Brown wrote:
>> Ok. How about that:
>> ---
>>   sound/soc/codecs/max98090.c | 52 ++++++++++++++++++++-----------------
>>   sound/soc/codecs/max98090.h |  2 ++
>>   2 files changed, 30 insertions(+), 24 deletions(-)
> 
> That looks plausible, yes.
> 
> I do note that the driver ignores tdm_width (and the entire TDM
> configuration) when configuring BCLK, I guess it only works in clock
> consumer mode for TDM?  If that's the case there should really be some
> validation, and there should probably be a check for slot width being 16
> since that looks like the only thing supported.  Those were already
> broken though.

Yep, tdm_width is not used in the driver and I can drop it. We already 
have slot_width validation in set_tdm_slot(), so only 16 is allowed. I 
didn't check/use clock provider mode. But it looks fine for me. 
set_fmt() just sets frame width here: 32 - by default, 48/64 - 3/4 slots 
if configured. We also check slots count in set_tdm_slots(). It will 
work for I2S/TDM.