[PATCH v2] ASoC: madera: Read device tree configuration

[PATCH v2] ASoC: madera: Read device tree configuration

[Charles Keepax]

Read the configuration of the Madera ASoC driver from device tree.

Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
---

Changes since v1:
 - Returned to using a helping in the madera driver itself,
   discussions around adding a generic helper indicated that all the
   error reporting would still need to be contained in the driver which
   constitutes the vast majority of the code in the helper anyway.
 - Updated the code to use the new device_property_count_u32 helper.

Thanks,
Charles

 sound/soc/codecs/madera.c | 97 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 97 insertions(+)

diff --git a/sound/soc/codecs/madera.c b/sound/soc/codecs/madera.c
index 1b1be19a2f991..5f1e32a5a8556 100644
--- a/sound/soc/codecs/madera.c
+++ b/sound/soc/codecs/madera.c
@@ -300,6 +300,100 @@ int madera_free_overheat(struct madera_priv *priv)
 }
 EXPORT_SYMBOL_GPL(madera_free_overheat);
 
+static int madera_get_variable_u32_array(struct device *dev,
+					 const char *propname,
+					 u32 *dest, int n_max,
+					 int multiple)
+{
+	int n, ret;
+
+	n = device_property_count_u32(dev, propname);
+	if (n < 0) {
+		if (n == -EINVAL)
+			return 0;	/* missing, ignore */
+
+		dev_warn(dev, "%s malformed (%d)\n", propname, n);
+
+		return n;
+	} else if ((n % multiple) != 0) {
+		dev_warn(dev, "%s not a multiple of %d entries\n",
+			 propname, multiple);
+
+		return -EINVAL;
+	}
+
+	if (n > n_max)
+		n = n_max;
+
+	ret = device_property_read_u32_array(dev, propname, dest, n);
+	if (ret < 0)
+		return ret;
+
+	return n;
+}
+
+static void madera_prop_get_inmode(struct madera_priv *priv)
+{
+	struct madera *madera = priv->madera;
+	struct madera_codec_pdata *pdata = &madera->pdata.codec;
+	u32 tmp[MADERA_MAX_INPUT * MADERA_MAX_MUXED_CHANNELS];
+	int n, i, in_idx, ch_idx;
+
+	BUILD_BUG_ON(ARRAY_SIZE(pdata->inmode) != MADERA_MAX_INPUT);
+	BUILD_BUG_ON(ARRAY_SIZE(pdata->inmode[0]) != MADERA_MAX_MUXED_CHANNELS);
+
+	n = madera_get_variable_u32_array(madera->dev, "cirrus,inmode",
+					  tmp, ARRAY_SIZE(tmp),
+					  MADERA_MAX_MUXED_CHANNELS);
+	if (n < 0)
+		return;
+
+	in_idx = 0;
+	ch_idx = 0;
+	for (i = 0; i < n; ++i) {
+		pdata->inmode[in_idx][ch_idx] = tmp[i];
+
+		if (++ch_idx == MADERA_MAX_MUXED_CHANNELS) {
+			ch_idx = 0;
+			++in_idx;
+		}
+	}
+}
+
+static void madera_prop_get_pdata(struct madera_priv *priv)
+{
+	struct madera *madera = priv->madera;
+	struct madera_codec_pdata *pdata = &madera->pdata.codec;
+	u32 out_mono[ARRAY_SIZE(pdata->out_mono)];
+	int i, n;
+
+	madera_prop_get_inmode(priv);
+
+	n = madera_get_variable_u32_array(madera->dev, "cirrus,out-mono",
+					  out_mono, ARRAY_SIZE(out_mono), 1);
+	if (n > 0)
+		for (i = 0; i < n; ++i)
+			pdata->out_mono[i] = !!out_mono[i];
+
+	madera_get_variable_u32_array(madera->dev,
+				      "cirrus,max-channels-clocked",
+				      pdata->max_channels_clocked,
+				      ARRAY_SIZE(pdata->max_channels_clocked),
+				      1);
+
+	madera_get_variable_u32_array(madera->dev, "cirrus,pdm-fmt",
+				      pdata->pdm_fmt,
+				      ARRAY_SIZE(pdata->pdm_fmt), 1);
+
+	madera_get_variable_u32_array(madera->dev, "cirrus,pdm-mute",
+				      pdata->pdm_mute,
+				      ARRAY_SIZE(pdata->pdm_mute), 1);
+
+	madera_get_variable_u32_array(madera->dev, "cirrus,dmic-ref",
+				      pdata->dmic_ref,
+				      ARRAY_SIZE(pdata->dmic_ref), 1);
+}
+
 int madera_core_init(struct madera_priv *priv)
 {
 	int i;
@@ -308,6 +402,9 @@ int madera_core_init(struct madera_priv *priv)
 	BUILD_BUG_ON(!madera_mixer_texts[MADERA_NUM_MIXER_INPUTS - 1]);
 	BUILD_BUG_ON(!madera_mixer_values[MADERA_NUM_MIXER_INPUTS - 1]);
 
+	if (!dev_get_platdata(priv->madera->dev))
+		madera_prop_get_pdata(priv);
+
 	mutex_init(&priv->rate_lock);
 
 	for (i = 0; i < MADERA_MAX_HP_OUTPUT; i++)
-- 
2.11.0

Re: [PATCH v2] ASoC: madera: Read device tree configuration

[Cezary Rojewski]

On 2019-07-22 15:52, Charles Keepax wrote:
> +static void madera_prop_get_inmode(struct madera_priv *priv)
> +{
> +	struct madera *madera = priv->madera;
> +	struct madera_codec_pdata *pdata = &madera->pdata.codec;
> +	u32 tmp[MADERA_MAX_INPUT * MADERA_MAX_MUXED_CHANNELS];
> +	int n, i, in_idx, ch_idx;
> +
> +	BUILD_BUG_ON(ARRAY_SIZE(pdata->inmode) != MADERA_MAX_INPUT);
> +	BUILD_BUG_ON(ARRAY_SIZE(pdata->inmode[0]) != MADERA_MAX_MUXED_CHANNELS);
> +
> +	n = madera_get_variable_u32_array(madera->dev, "cirrus,inmode",
> +					  tmp, ARRAY_SIZE(tmp),
> +					  MADERA_MAX_MUXED_CHANNELS);
> +	if (n < 0)
> +		return;
> +
> +	in_idx = 0;
> +	ch_idx = 0;
> +	for (i = 0; i < n; ++i) {
> +		pdata->inmode[in_idx][ch_idx] = tmp[i];
> +
> +		if (++ch_idx == MADERA_MAX_MUXED_CHANNELS) {
> +			ch_idx = 0;
> +			++in_idx;
> +		}
> +	}
> +}
> +
> +static void madera_prop_get_pdata(struct madera_priv *priv)
> +{
> +	struct madera *madera = priv->madera;
> +	struct madera_codec_pdata *pdata = &madera->pdata.codec;
> +	u32 out_mono[ARRAY_SIZE(pdata->out_mono)];
> +	int i, n;
> +
> +	madera_prop_get_inmode(priv);
> +
> +	n = madera_get_variable_u32_array(madera->dev, "cirrus,out-mono",
> +					  out_mono, ARRAY_SIZE(out_mono), 1);
> +	if (n > 0)
> +		for (i = 0; i < n; ++i)
> +			pdata->out_mono[i] = !!out_mono[i];
> +
> +	madera_get_variable_u32_array(madera->dev,
> +				      "cirrus,max-channels-clocked",
> +				      pdata->max_channels_clocked,
> +				      ARRAY_SIZE(pdata->max_channels_clocked),
> +				      1);
> +
> +	madera_get_variable_u32_array(madera->dev, "cirrus,pdm-fmt",
> +				      pdata->pdm_fmt,
> +				      ARRAY_SIZE(pdata->pdm_fmt), 1);
> +
> +	madera_get_variable_u32_array(madera->dev, "cirrus,pdm-mute",
> +				      pdata->pdm_mute,
> +				      ARRAY_SIZE(pdata->pdm_mute), 1);
> +
> +	madera_get_variable_u32_array(madera->dev, "cirrus,dmic-ref",
> +				      pdata->dmic_ref,
> +				      ARRAY_SIZE(pdata->dmic_ref), 1);

Hmm, madera_get_variable_u32_array calls are not permissive within 
madera_prop_get_inmode yet here they are. Is this intentional?

> +}
> +
>   int madera_core_init(struct madera_priv *priv)
>   {
>   	int i;
> @@ -308,6 +402,9 @@ int madera_core_init(struct madera_priv *priv)
>   	BUILD_BUG_ON(!madera_mixer_texts[MADERA_NUM_MIXER_INPUTS - 1]);
>   	BUILD_BUG_ON(!madera_mixer_values[MADERA_NUM_MIXER_INPUTS - 1]);
>   
> +	if (!dev_get_platdata(priv->madera->dev))
> +		madera_prop_get_pdata(priv);
> +
>   	mutex_init(&priv->rate_lock);
>   
>   	for (i = 0; i < MADERA_MAX_HP_OUTPUT; i++)
> 

Re: [PATCH v2] ASoC: madera: Read device tree configuration

[Charles Keepax]

On Tue, Jul 23, 2019 at 12:07:32AM +0200, Cezary Rojewski wrote:
> On 2019-07-22 15:52, Charles Keepax wrote:
> >+static void madera_prop_get_inmode(struct madera_priv *priv)
> >+{
> >+	struct madera *madera = priv->madera;
> >+	struct madera_codec_pdata *pdata = &madera->pdata.codec;
> >+	u32 tmp[MADERA_MAX_INPUT * MADERA_MAX_MUXED_CHANNELS];
> >+	int n, i, in_idx, ch_idx;
> >+
> >+	BUILD_BUG_ON(ARRAY_SIZE(pdata->inmode) != MADERA_MAX_INPUT);
> >+	BUILD_BUG_ON(ARRAY_SIZE(pdata->inmode[0]) != MADERA_MAX_MUXED_CHANNELS);
> >+
> >+	n = madera_get_variable_u32_array(madera->dev, "cirrus,inmode",
> >+					  tmp, ARRAY_SIZE(tmp),
> >+					  MADERA_MAX_MUXED_CHANNELS);
> >+	if (n < 0)
> >+		return;
> >+
> >+	in_idx = 0;
> >+	ch_idx = 0;
> >+	for (i = 0; i < n; ++i) {
> >+		pdata->inmode[in_idx][ch_idx] = tmp[i];
> >+
> >+		if (++ch_idx == MADERA_MAX_MUXED_CHANNELS) {
> >+			ch_idx = 0;
> >+			++in_idx;
> >+		}
> >+	}
> >+}
> >+
> >+static void madera_prop_get_pdata(struct madera_priv *priv)
> >+{
> >+	struct madera *madera = priv->madera;
> >+	struct madera_codec_pdata *pdata = &madera->pdata.codec;
> >+	u32 out_mono[ARRAY_SIZE(pdata->out_mono)];
> >+	int i, n;
> >+
> >+	madera_prop_get_inmode(priv);
> 
> Hmm, madera_get_variable_u32_array calls are not permissive within
> madera_prop_get_inmode yet here they are. Is this intentional?
> 

Apologies but could you clarify what you mean by "not
permissive"?

I can't see anything that would prevent the function from
being called (indeed it builds and works), and the binding
documentation does specify that this field can be of variable
size.

Thanks,
Charles

Applied "ASoC: madera: Read device tree configuration" to the asoc tree

[Mark Brown]

The patch

   ASoC: madera: Read device tree configuration

has been applied to the asoc tree at

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-5.4

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.  

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

>From 748fd07e2b9ca4132e3d2aae25395aedc4d1aee8 Mon Sep 17 00:00:00 2001
From: Charles Keepax <ckeepax@opensource.cirrus.com>
Date: Mon, 22 Jul 2019 14:52:09 +0100
Subject: [PATCH] ASoC: madera: Read device tree configuration

Read the configuration of the Madera ASoC driver from device tree.

Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
Link: https://lore.kernel.org/r/20190722135209.30302-1-ckeepax@opensource.cirrus.com
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 sound/soc/codecs/madera.c | 97 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 97 insertions(+)

diff --git a/sound/soc/codecs/madera.c b/sound/soc/codecs/madera.c
index 1b1be19a2f99..5f1e32a5a855 100644
--- a/sound/soc/codecs/madera.c
+++ b/sound/soc/codecs/madera.c
@@ -300,6 +300,100 @@ int madera_free_overheat(struct madera_priv *priv)
 }
 EXPORT_SYMBOL_GPL(madera_free_overheat);
 
+static int madera_get_variable_u32_array(struct device *dev,
+					 const char *propname,
+					 u32 *dest, int n_max,
+					 int multiple)
+{
+	int n, ret;
+
+	n = device_property_count_u32(dev, propname);
+	if (n < 0) {
+		if (n == -EINVAL)
+			return 0;	/* missing, ignore */
+
+		dev_warn(dev, "%s malformed (%d)\n", propname, n);
+
+		return n;
+	} else if ((n % multiple) != 0) {
+		dev_warn(dev, "%s not a multiple of %d entries\n",
+			 propname, multiple);
+
+		return -EINVAL;
+	}
+
+	if (n > n_max)
+		n = n_max;
+
+	ret = device_property_read_u32_array(dev, propname, dest, n);
+	if (ret < 0)
+		return ret;
+
+	return n;
+}
+
+static void madera_prop_get_inmode(struct madera_priv *priv)
+{
+	struct madera *madera = priv->madera;
+	struct madera_codec_pdata *pdata = &madera->pdata.codec;
+	u32 tmp[MADERA_MAX_INPUT * MADERA_MAX_MUXED_CHANNELS];
+	int n, i, in_idx, ch_idx;
+
+	BUILD_BUG_ON(ARRAY_SIZE(pdata->inmode) != MADERA_MAX_INPUT);
+	BUILD_BUG_ON(ARRAY_SIZE(pdata->inmode[0]) != MADERA_MAX_MUXED_CHANNELS);
+
+	n = madera_get_variable_u32_array(madera->dev, "cirrus,inmode",
+					  tmp, ARRAY_SIZE(tmp),
+					  MADERA_MAX_MUXED_CHANNELS);
+	if (n < 0)
+		return;
+
+	in_idx = 0;
+	ch_idx = 0;
+	for (i = 0; i < n; ++i) {
+		pdata->inmode[in_idx][ch_idx] = tmp[i];
+
+		if (++ch_idx == MADERA_MAX_MUXED_CHANNELS) {
+			ch_idx = 0;
+			++in_idx;
+		}
+	}
+}
+
+static void madera_prop_get_pdata(struct madera_priv *priv)
+{
+	struct madera *madera = priv->madera;
+	struct madera_codec_pdata *pdata = &madera->pdata.codec;
+	u32 out_mono[ARRAY_SIZE(pdata->out_mono)];
+	int i, n;
+
+	madera_prop_get_inmode(priv);
+
+	n = madera_get_variable_u32_array(madera->dev, "cirrus,out-mono",
+					  out_mono, ARRAY_SIZE(out_mono), 1);
+	if (n > 0)
+		for (i = 0; i < n; ++i)
+			pdata->out_mono[i] = !!out_mono[i];
+
+	madera_get_variable_u32_array(madera->dev,
+				      "cirrus,max-channels-clocked",
+				      pdata->max_channels_clocked,
+				      ARRAY_SIZE(pdata->max_channels_clocked),
+				      1);
+
+	madera_get_variable_u32_array(madera->dev, "cirrus,pdm-fmt",
+				      pdata->pdm_fmt,
+				      ARRAY_SIZE(pdata->pdm_fmt), 1);
+
+	madera_get_variable_u32_array(madera->dev, "cirrus,pdm-mute",
+				      pdata->pdm_mute,
+				      ARRAY_SIZE(pdata->pdm_mute), 1);
+
+	madera_get_variable_u32_array(madera->dev, "cirrus,dmic-ref",
+				      pdata->dmic_ref,
+				      ARRAY_SIZE(pdata->dmic_ref), 1);
+}
+
 int madera_core_init(struct madera_priv *priv)
 {
 	int i;
@@ -308,6 +402,9 @@ int madera_core_init(struct madera_priv *priv)
 	BUILD_BUG_ON(!madera_mixer_texts[MADERA_NUM_MIXER_INPUTS - 1]);
 	BUILD_BUG_ON(!madera_mixer_values[MADERA_NUM_MIXER_INPUTS - 1]);
 
+	if (!dev_get_platdata(priv->madera->dev))
+		madera_prop_get_pdata(priv);
+
 	mutex_init(&priv->rate_lock);
 
 	for (i = 0; i < MADERA_MAX_HP_OUTPUT; i++)
-- 
2.20.1

Re: [PATCH v2] ASoC: madera: Read device tree configuration

[Cezary Rojewski]

On 2019-07-23 10:17, Charles Keepax wrote:
> On Tue, Jul 23, 2019 at 12:07:32AM +0200, Cezary Rojewski wrote:
>> On 2019-07-22 15:52, Charles Keepax wrote:
>>> +static void madera_prop_get_inmode(struct madera_priv *priv)
>>> +{
>>> +	struct madera *madera = priv->madera;
>>> +	struct madera_codec_pdata *pdata = &madera->pdata.codec;
>>> +	u32 tmp[MADERA_MAX_INPUT * MADERA_MAX_MUXED_CHANNELS];
>>> +	int n, i, in_idx, ch_idx;
>>> +
>>> +	BUILD_BUG_ON(ARRAY_SIZE(pdata->inmode) != MADERA_MAX_INPUT);
>>> +	BUILD_BUG_ON(ARRAY_SIZE(pdata->inmode[0]) != MADERA_MAX_MUXED_CHANNELS);
>>> +
>>> +	n = madera_get_variable_u32_array(madera->dev, "cirrus,inmode",
>>> +					  tmp, ARRAY_SIZE(tmp),
>>> +					  MADERA_MAX_MUXED_CHANNELS);
>>> +	if (n < 0)
>>> +		return;
>>> +
>>> +	in_idx = 0;
>>> +	ch_idx = 0;
>>> +	for (i = 0; i < n; ++i) {
>>> +		pdata->inmode[in_idx][ch_idx] = tmp[i];
>>> +
>>> +		if (++ch_idx == MADERA_MAX_MUXED_CHANNELS) {
>>> +			ch_idx = 0;
>>> +			++in_idx;
>>> +		}
>>> +	}
>>> +}
>>> +
>>> +static void madera_prop_get_pdata(struct madera_priv *priv)
>>> +{
>>> +	struct madera *madera = priv->madera;
>>> +	struct madera_codec_pdata *pdata = &madera->pdata.codec;
>>> +	u32 out_mono[ARRAY_SIZE(pdata->out_mono)];
>>> +	int i, n;
>>> +
>>> +	madera_prop_get_inmode(priv);
>>
>> Hmm, madera_get_variable_u32_array calls are not permissive within
>> madera_prop_get_inmode yet here they are. Is this intentional?
>>
> 
> Apologies but could you clarify what you mean by "not
> permissive"?
> 
> I can't see anything that would prevent the function from
> being called (indeed it builds and works), and the binding
> documentation does specify that this field can be of variable
> size.
> 
> Thanks,
> Charles

No worries. By "permissive" I described the usage of 
_get_variable_u32_array within madera_prop_get_pdata. In 
madera_prop_get_inmode you do care about the return value. In 
madera_prop_get_pdata however, you ignore all of them. From 
_get_variable_u32_array declaration, it seems function may fail.
Sometimes it's desired to be permissive, simply asking if that's 
intentional.

Czarek

Re: [PATCH v2] ASoC: madera: Read device tree configuration

[Charles Keepax]

On Tue, Jul 23, 2019 at 04:21:41PM +0200, Cezary Rojewski wrote:
> On 2019-07-23 10:17, Charles Keepax wrote:
> >On Tue, Jul 23, 2019 at 12:07:32AM +0200, Cezary Rojewski wrote:
> >>On 2019-07-22 15:52, Charles Keepax wrote:
> >>>+static void madera_prop_get_inmode(struct madera_priv *priv)
> >>>+
> >>>+	n = madera_get_variable_u32_array(madera->dev, "cirrus,inmode",
> >>>+					  tmp, ARRAY_SIZE(tmp),
> >>>+					  MADERA_MAX_MUXED_CHANNELS);
> >>>+	if (n < 0)
> >>>+		return;
> >>
> >>Hmm, madera_get_variable_u32_array calls are not permissive within
> >>madera_prop_get_inmode yet here they are. Is this intentional?
> >>
> >
> >Apologies but could you clarify what you mean by "not
> >permissive"?
> >
> >I can't see anything that would prevent the function from
> >being called (indeed it builds and works), and the binding
> >documentation does specify that this field can be of variable
> >size.
> >
> >Thanks,
> >Charles
> 
> No worries. By "permissive" I described the usage of
> _get_variable_u32_array within madera_prop_get_pdata. In
> madera_prop_get_inmode you do care about the return value. In
> madera_prop_get_pdata however, you ignore all of them. From
> _get_variable_u32_array declaration, it seems function may fail.
> Sometimes it's desired to be permissive, simply asking if that's
> intentional.
> 

Ah.. yes I follow. Yes this is intentional, all the DT fields in
question are optional so the driver should proceed if even if they
are corrupt or missing.

madera_prop_get_inmode checks the return value because additional
code is required to insert the values into the pdata structure, so
that code should be skipped in the case cirrus,inmode is not
present/fails. Most of the calls in madera_prop_get_pdata are
self-contained not requiring further handling other than reading
the data directly into the pdata structure. Uou can see the same
on the read of cirrus,out-mono the additional processing is
skipped in the case of an error.

Thanks,
Charles

Re: [PATCH v2] ASoC: madera: Read device tree configuration

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 354 bytes --]

On Tue, Jul 23, 2019 at 04:01:25PM +0100, Charles Keepax wrote:

> Ah.. yes I follow. Yes this is intentional, all the DT fields in
> question are optional so the driver should proceed if even if they
> are corrupt or missing.

If they're present but unparsable you should probably say something
about that really.  That's clearly different to optional.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH v2] ASoC: madera: Read device tree configuration

[Charles Keepax]

On Tue, Jul 23, 2019 at 04:03:02PM +0100, Mark Brown wrote:
> On Tue, Jul 23, 2019 at 04:01:25PM +0100, Charles Keepax wrote:
> 
> > Ah.. yes I follow. Yes this is intentional, all the DT fields in
> > question are optional so the driver should proceed if even if they
> > are corrupt or missing.
> 
> If they're present but unparsable you should probably say something
> about that really.  That's clearly different to optional.

The helper logs an error message itself since essentially the
only difference in the messages is the name of the property,
there is just no need to process the return value since we
would take the same code path regardless of failure or success.

Thanks,
Charles