[PATCH 1/2] ASoC: Improve hi6210-i2s DT bindings

[PATCH 1/2] ASoC: Improve hi6210-i2s DT bindings

[John Stultz]

This patch improves the previously submitted hi6210-i2s DT
binding, adding extra details to how the multi-dai index
value maps to the potential interfaces.
(Currently just index 0 -> the S2 interface, as there is
only one supported, but in the future other interfaces may
be enabled.)

Cc: Zhangfei Gao <zhangfei.gao@linaro.org>
Cc: Liam Girdwood <lgirdwood@gmail.com>
Cc: Mark Brown <broonie@kernel.org>
Cc: Jaroslav Kysela <perex@perex.cz>
Cc: Wei Xu <xuwei5@hisilicon.com>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Andy Green <andy@warmcat.com>
Cc: Dave Long <dave.long@linaro.org>
Cc: Guodong Xu <guodong.xu@linaro.org>
Signed-off-by: John Stultz <john.stultz@linaro.org>
---
 Documentation/devicetree/bindings/sound/hisilicon,hi6210-i2s.txt | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/Documentation/devicetree/bindings/sound/hisilicon,hi6210-i2s.txt b/Documentation/devicetree/bindings/sound/hisilicon,hi6210-i2s.txt
index 680bb035..7a29678 100644
--- a/Documentation/devicetree/bindings/sound/hisilicon,hi6210-i2s.txt
+++ b/Documentation/devicetree/bindings/sound/hisilicon,hi6210-i2s.txt
@@ -17,6 +17,10 @@ Required properties:
 - dma-names: should be "tx" and "rx"
 - hisilicon,sysctrl-syscon: phandle to sysctrl syscon
 - #sound-dai-cells: Should be set to 1 (for multi-dai)
+   - The dai cell indexes reference the following interfaces:
+       0: S2 interface
+       (Currently that is the only one available, but more may be
+        supported in the future)
 
 Example for the hi6210 i2s controller:
 
@@ -32,3 +36,7 @@ i2s0: i2s@f7118000{
 	hisilicon,sysctrl-syscon = <&sys_ctrl>;
 	#sound-dai-cells = <1>;
 };
+
+Then when referencing the i2s controller:
+	sound-dai = <&i2s0 0>; /* index 0 => S2 interface */
+
-- 
2.7.4

[PATCH 2/2] ASoC: hisilicon: Minor fixups to hi6210 i2s audio driver

[John Stultz]

Mark had a few minor fixups he requested to the hi6210 i2s
driver, so this patch proves them.

This patch adds a few extra error returns in cases that
shouldn't happen, some style nits adding breaks to final
default cases in switch statements, and tweaks to use
devm_ variant of snd_soc_register_component.

Cc: Zhangfei Gao <zhangfei.gao@linaro.org>
Cc: Liam Girdwood <lgirdwood@gmail.com>
Cc: Mark Brown <broonie@kernel.org>
Cc: Jaroslav Kysela <perex@perex.cz>
Cc: Wei Xu <xuwei5@hisilicon.com>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Andy Green <andy@warmcat.com>
Cc: Dave Long <dave.long@linaro.org>
Cc: Guodong Xu <guodong.xu@linaro.org>
Signed-off-by: John Stultz <john.stultz@linaro.org>
---
 sound/soc/hisilicon/hi6210-i2s.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/sound/soc/hisilicon/hi6210-i2s.c b/sound/soc/hisilicon/hi6210-i2s.c
index 45691b70..bdbf982 100644
--- a/sound/soc/hisilicon/hi6210-i2s.c
+++ b/sound/soc/hisilicon/hi6210-i2s.c
@@ -324,6 +324,7 @@ static int hi6210_i2s_hw_params(struct snd_pcm_substream *substream,
 	default:
 		i2s->bits = 16;
 		dma_data->addr_width = 2;
+		break;
 	}
 	i2s->rate = params_rate(params);
 	i2s->channels = params_channels(params);
@@ -395,6 +396,7 @@ static int hi6210_i2s_hw_params(struct snd_pcm_substream *substream,
 		break;
 	default:
 		WARN_ONCE(1, "Invalid i2s->fmt MASTER_MASK. This shouldn't happen\n");
+		return -EINVAL;
 	}
 
 	switch (i2s->format & SND_SOC_DAIFMT_FORMAT_MASK) {
@@ -409,6 +411,7 @@ static int hi6210_i2s_hw_params(struct snd_pcm_substream *substream,
 		break;
 	default:
 		WARN_ONCE(1, "Invalid i2s->fmt FORMAT_MASK. This shouldn't happen\n");
+		return -EINVAL;
 	}
 
 	val = hi6210_read_reg(i2s, HII2S_I2S_CFG);
@@ -440,6 +443,7 @@ static int hi6210_i2s_hw_params(struct snd_pcm_substream *substream,
 		val = hi6210_read_reg(i2s, HII2S_I2S_CFG);
 		val &= ~HII2S_I2S_CFG__S2_FRAME_MODE;
 		hi6210_write_reg(i2s, HII2S_I2S_CFG, val);
+		break;
 	}
 
 	/* clear loopback, set signed type and word length */
@@ -587,20 +591,14 @@ static int hi6210_i2s_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
-	ret = snd_soc_register_component(&pdev->dev, &hi6210_i2s_i2s_comp,
+	ret = devm_snd_soc_register_component(&pdev->dev, &hi6210_i2s_i2s_comp,
 					 &i2s->dai, 1);
-	if (ret) {
-		dev_err(&pdev->dev, "Failed to register dai\n");
-		return ret;
-	}
-
-	return 0;
+	return ret;
 }
 
 static int hi6210_i2s_remove(struct platform_device *pdev)
 {
 	snd_soc_unregister_component(&pdev->dev);
-	dev_set_drvdata(&pdev->dev, NULL);
 
 	return 0;
 }
-- 
2.7.4

Re: [PATCH 2/2] ASoC: hisilicon: Minor fixups to hi6210 i2s audio driver

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 683 bytes --]

On Mon, Apr 10, 2017 at 12:35:12PM -0700, John Stultz wrote:

> This patch adds a few extra error returns in cases that
> shouldn't happen, some style nits adding breaks to final
> default cases in switch statements, and tweaks to use
> devm_ variant of snd_soc_register_component.

Please don't make multiple unrelated changes in a single patch, it's bad
practice which can slow down the bits of the patch which are OK and
makes it harder to review if the changes match up with the changelog.

>  static int hi6210_i2s_remove(struct platform_device *pdev)
>  {
>         snd_soc_unregister_component(&pdev->dev);

Since you converted to devm_ this is now going to be a double free.

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

Re: [PATCH 2/2] ASoC: hisilicon: Minor fixups to hi6210 i2s audio driver

[John Stultz]

On Tue, Apr 11, 2017 at 11:34 AM, Mark Brown <broonie@kernel.org> wrote:
> On Mon, Apr 10, 2017 at 12:35:12PM -0700, John Stultz wrote:
>
>> This patch adds a few extra error returns in cases that
>> shouldn't happen, some style nits adding breaks to final
>> default cases in switch statements, and tweaks to use
>> devm_ variant of snd_soc_register_component.
>
> Please don't make multiple unrelated changes in a single patch, it's bad
> practice which can slow down the bits of the patch which are OK and
> makes it harder to review if the changes match up with the changelog.

Apologies, I'll split it up and resend.

>>  static int hi6210_i2s_remove(struct platform_device *pdev)
>>  {
>>         snd_soc_unregister_component(&pdev->dev);
>
> Since you converted to devm_ this is now going to be a double free.

Ah. So it looks like can yank the whole remove implementation.

thanks
-john