diff mbox

streamline TLV320AIC23 drivers

Message ID b4127646-565b-ab16-0a8f-1fd6c263389e@ADLINKtech.com (mailing list archive)
State New, archived
Headers show

Commit Message

Jens Rottmann April 21, 2017, 7:22 p.m. UTC 
The iMX-TLV320AIC23 driver isn't from Freescale, but from a company named
Eukrea Electromatique, originally for their own boards. From the code I get
the impression it is a bit older, its DT options use a differing naming
scheme. Patch it up a bit:

- Remove Eukrea naming, i.MX is from Freescale, TLV320AIC23 is from TI,
  driver was written by Eukrea, but it's DT capable, so it's not exclusive:
  - Kconfig option title
  - 'model' option
  - driver 'compatible' string
- Other options just have changed over time, this driver remaining (one of)
  the last with the old semantics:
  - 'audio-codec' option (also moved from ssi node)
  - 'mux-int/ext-port' options
- All options stay backwards compatible, the DT binding documents new and
  old names.

CONFIG variable and files have not been renamed, though, so no need to
change old defconfigs.

Signed-off-by: Jens Rottmann <Jens.Rottmann@ADLINKtech.com>
---

Comments

Rob Herring (Arm) April 28, 2017, 5:11 p.m. UTC | #1 
On Fri, Apr 21, 2017 at 09:22:02PM +0200, Jens Rottmann wrote:
> The iMX-TLV320AIC23 driver isn't from Freescale, but from a company named
> Eukrea Electromatique, originally for their own boards. From the code I get
> the impression it is a bit older, its DT options use a differing naming
> scheme. Patch it up a bit:

Needs a subject following the format of the subsystem.

> 
> - Remove Eukrea naming, i.MX is from Freescale, TLV320AIC23 is from TI,
>   driver was written by Eukrea, but it's DT capable, so it's not exclusive:
>   - Kconfig option title
>   - 'model' option
>   - driver 'compatible' string
> - Other options just have changed over time, this driver remaining (one of)
>   the last with the old semantics:
>   - 'audio-codec' option (also moved from ssi node)
>   - 'mux-int/ext-port' options
> - All options stay backwards compatible, the DT binding documents new and
>   old names.
> 
> CONFIG variable and files have not been renamed, though, so no need to
> change old defconfigs.
> 
> Signed-off-by: Jens Rottmann <Jens.Rottmann@ADLINKtech.com>
> ---
> 
> --- a/Documentation/devicetree/bindings/sound/eukrea-tlv320.txt
> +++ b/Documentation/devicetree/bindings/sound/eukrea-tlv320.txt

Perhaps change the filename. The compatible string is a good choice.

> @@ -1,16 +1,23 @@
> -Audio complex for Eukrea boards with tlv320aic23 codec.
> +Audio complex for Freescale i.MX boards with TI TLV320AIC23 I2S codecs,
> +like those from Eukrea Electromatique.
>  
>  Required properties:
>  
> -  - compatible		: "eukrea,asoc-tlv320"
> +  - compatible		: "fsl,imx-audio-tlv320aic23" or
> +			  "eukrea,asoc-tlv320" (deprecated)
>  
> -  - eukrea,model	: The user-visible name of this sound complex.
> +  - model		: The user-visible name of this sound complex.
> +  - eukrea,model	: Dito, deprecated.
>  
>    - ssi-controller	: The phandle of the SSI controller.
>  
> -  - fsl,mux-int-port	: The internal port of the i.MX audio muxer (AUDMUX).
> +  - mux-int-port	: The internal port of the i.MX audio muxer (AUDMUX).
> +  - fsl,mux-int-port	: Dito, deprecated.
>  
> -  - fsl,mux-ext-port	: The external port of the i.MX audio muxer.
> +  - mux-ext-port	: The external port of the i.MX audio muxer.
> +  - fsl,mux-ext-port	: Dito, deprecated.

Is this used elsewhere? This is FSL specific, so you should keep the 
prefix.

> +
> +  - audio-codec		: The phandle of the audio codec.
>  
>  Note: The AUDMUX port numbering should start at 1, which is consistent with
>  hardware manual.
> @@ -18,9 +25,10 @@ hardware manual.
>  Example:
>  
>  	sound {
> -		compatible = "eukrea,asoc-tlv320";
> -		eukrea,model = "imx51-eukrea-tlv320aic23";
> +		compatible = "fsl,imx-audio-tlv320aic23";
> +		model = "imx51-eukrea-tlv320aic23";
>  		ssi-controller = <&ssi2>;
> -		fsl,mux-int-port = <2>;
> -		fsl,mux-ext-port = <3>;
> +		mux-int-port = <2>;
> +		mux-ext-port = <3>;
> +		audio-codec = <&codec>;
>  	};
Jens Rottmann May 5, 2017, 10:39 p.m. UTC | #2 
Hi Rob,
sorry for the delay.

> On Fri, Apr 21, 2017 at 09:22:02PM +0200, Jens Rottmann wrote:
>> - Remove Eukrea naming, i.MX is from Freescale, TLV320AIC23 is from TI,
>>   driver was written by Eukrea, but it's DT capable, so it's not exclusive:
>>   - Kconfig option title
>>   - 'model' option
>>   - driver 'compatible' string
>> [...]
>> CONFIG variable and files have not been renamed, though, so no need to
>> change old defconfigs.

On 04/28/2017 19:11, Rob Herring answered:
> Perhaps change the filename. [...]

I wanted to avoid the churn, but ok. In that case:

Do you want me to change all indentifiers inside driver, too?
eukrea_tlv320_probe() --> imx_tlv320aic23_probe()

Do you want me to also rename the CONFIG var?
CONFIG_SND_SOC_EUKREA_TLV320 --> CONFIG_SND_SOC_IMX_TLV320
I'd adapt in-tree defconfigs, but would unavoidably break
out-of-tree defconfigs...

>> -  - fsl,mux-int-port	: The internal port of the i.MX audio muxer (AUDMUX).
>> +  - mux-int-port	: The internal port of the i.MX audio muxer (AUDMUX).
>> +  - fsl,mux-int-port	: Dito, deprecated.
>> -  - fsl,mux-ext-port	: The external port of the i.MX audio muxer.
>> +  - mux-ext-port	: The external port of the i.MX audio muxer.
>> +  - fsl,mux-ext-port	: Dito, deprecated.

> Is this used elsewhere? This is FSL specific, so you should keep the
> prefix.

.../sound/soc/fsl$ egrep 'property.*mux-(int|ext)-port' *
eukrea-tlv320.c: ret = of_property_read_u32(np, "fsl,mux-int-port", &int_port);
eukrea-tlv320.c: ret = of_property_read_u32(np, "fsl,mux-ext-port", &ext_port);
fsl-asoc-card.c: ret = of_property_read_u32(np, "mux-int-port", &int_port);
fsl-asoc-card.c: ret = of_property_read_u32(np, "mux-ext-port", &ext_port);
imx-es8328.c:    ret = of_property_read_u32(np, "mux-int-port", &int_port);
imx-es8328.c:    ret = of_property_read_u32(np, "mux-ext-port", &ext_port);
imx-sgtl5000.c:  ret = of_property_read_u32(np, "mux-int-port", &int_port);
imx-sgtl5000.c:  ret = of_property_read_u32(np, "mux-ext-port", &ext_port);
imx-wm8962.c:    ret = of_property_read_u32(np, "mux-int-port", &int_port);
imx-wm8962.c:    ret = of_property_read_u32(np, "mux-ext-port", &ext_port);

All drivers use mux-int|ext-port without prefix, tlv320 is the only
odd driver out, that's why I did this streamlining patch in the 1st place.

I would rather not have to change all other drivers to match the tlv320 one.

Thanks,
Jens
Jens Rottmann June 2, 2017, 9:22 p.m. UTC | #3 
Hi Rob,
repeatedly got no response. Assuming all my mails got blocked, so moved to yet another account. Hope I'll get through this time.
Regards, Jens

-------- Original Message --------
Subject: Re: [PATCH] streamline TLV320AIC23 drivers
Date: Sat, 6 May 2017 00:39:20 +0200
From: Jens Rottmann <JRottm@yahoo.com>
To: Rob Herring
CC: Mark Rutland, Jaroslav Kysela, Takashi Iwai, alsa-devel@alsa-project.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, Liam Girdwood, Mark Brown

Hi Rob,
sorry for the delay.

> On Fri, Apr 21, 2017 at 09:22:02PM +0200, Jens Rottmann wrote:
>> - Remove Eukrea naming, i.MX is from Freescale, TLV320AIC23 is from TI,
>>   driver was written by Eukrea, but it's DT capable, so it's not exclusive:
>>   - Kconfig option title
>>   - 'model' option
>>   - driver 'compatible' string
>> [...]
>> CONFIG variable and files have not been renamed, though, so no need to
>> change old defconfigs.

On 04/28/2017 19:11, Rob Herring answered:
> Perhaps change the filename. [...]

I wanted to avoid the churn, but ok. In that case:

Do you want me to change all indentifiers inside driver, too?
eukrea_tlv320_probe() --> imx_tlv320aic23_probe()

Do you want me to also rename the CONFIG var?
CONFIG_SND_SOC_EUKREA_TLV320 --> CONFIG_SND_SOC_IMX_TLV320
I'd adapt in-tree defconfigs, but would unavoidably break
out-of-tree defconfigs...

>> -  - fsl,mux-int-port	: The internal port of the i.MX audio muxer (AUDMUX).
>> +  - mux-int-port	: The internal port of the i.MX audio muxer (AUDMUX).
>> +  - fsl,mux-int-port	: Dito, deprecated.
>> -  - fsl,mux-ext-port	: The external port of the i.MX audio muxer.
>> +  - mux-ext-port	: The external port of the i.MX audio muxer.
>> +  - fsl,mux-ext-port	: Dito, deprecated.

> Is this used elsewhere? This is FSL specific, so you should keep the
> prefix.

.../sound/soc/fsl$ egrep 'property.*mux-(int|ext)-port' *
eukrea-tlv320.c: ret = of_property_read_u32(np, "fsl,mux-int-port", &int_port);
eukrea-tlv320.c: ret = of_property_read_u32(np, "fsl,mux-ext-port", &ext_port);
fsl-asoc-card.c: ret = of_property_read_u32(np, "mux-int-port", &int_port);
fsl-asoc-card.c: ret = of_property_read_u32(np, "mux-ext-port", &ext_port);
imx-es8328.c:    ret = of_property_read_u32(np, "mux-int-port", &int_port);
imx-es8328.c:    ret = of_property_read_u32(np, "mux-ext-port", &ext_port);
imx-sgtl5000.c:  ret = of_property_read_u32(np, "mux-int-port", &int_port);
imx-sgtl5000.c:  ret = of_property_read_u32(np, "mux-ext-port", &ext_port);
imx-wm8962.c:    ret = of_property_read_u32(np, "mux-int-port", &int_port);
imx-wm8962.c:    ret = of_property_read_u32(np, "mux-ext-port", &ext_port);

All drivers use mux-int|ext-port without prefix, tlv320 is the only
odd driver out, that's why I did this streamlining patch in the 1st place.

I would rather not have to change all other drivers to match the tlv320 one.

Thanks,
Jens
Jens Rottmann Aug. 1, 2017, 9:07 p.m. UTC | #4 
Hi Rob,
repeatedly got no response. Hope I'll get through this time.
Regards, Jens

-------- Original Message --------
Subject: Re: [PATCH] streamline TLV320AIC23 drivers
Date: Sat, 6 May 2017 00:39:20 +0200
From: Jens Rottmann <JRottm@yahoo.com>
To: Rob Herring
CC: Mark Rutland, Jaroslav Kysela, Takashi Iwai, alsa-devel@alsa-project.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, Liam Girdwood, Mark Brown

Hi Rob,
sorry for the delay.

> On Fri, Apr 21, 2017 at 09:22:02PM +0200, Jens Rottmann wrote:
>> - Remove Eukrea naming, i.MX is from Freescale, TLV320AIC23 is from TI,
>>   driver was written by Eukrea, but it's DT capable, so it's not exclusive:
>>   - Kconfig option title
>>   - 'model' option
>>   - driver 'compatible' string
>> [...]
>> CONFIG variable and files have not been renamed, though, so no need to
>> change old defconfigs.

On 04/28/2017 19:11, Rob Herring answered:
> Perhaps change the filename. [...]

I wanted to avoid the churn, but ok. In that case:

Do you want me to change all indentifiers inside driver, too?
eukrea_tlv320_probe() --> imx_tlv320aic23_probe()

Do you want me to also rename the CONFIG var?
CONFIG_SND_SOC_EUKREA_TLV320 --> CONFIG_SND_SOC_IMX_TLV320
I'd adapt in-tree defconfigs, but would unavoidably break
out-of-tree defconfigs...

>> -  - fsl,mux-int-port	: The internal port of the i.MX audio muxer (AUDMUX).
>> +  - mux-int-port	: The internal port of the i.MX audio muxer (AUDMUX).
>> +  - fsl,mux-int-port	: Dito, deprecated.
>> -  - fsl,mux-ext-port	: The external port of the i.MX audio muxer.
>> +  - mux-ext-port	: The external port of the i.MX audio muxer.
>> +  - fsl,mux-ext-port	: Dito, deprecated.

> Is this used elsewhere? This is FSL specific, so you should keep the
> prefix.

.../sound/soc/fsl$ egrep 'property.*mux-(int|ext)-port' *
eukrea-tlv320.c: ret = of_property_read_u32(np, "fsl,mux-int-port", &int_port);
eukrea-tlv320.c: ret = of_property_read_u32(np, "fsl,mux-ext-port", &ext_port);
fsl-asoc-card.c: ret = of_property_read_u32(np, "mux-int-port", &int_port);
fsl-asoc-card.c: ret = of_property_read_u32(np, "mux-ext-port", &ext_port);
imx-es8328.c:    ret = of_property_read_u32(np, "mux-int-port", &int_port);
imx-es8328.c:    ret = of_property_read_u32(np, "mux-ext-port", &ext_port);
imx-sgtl5000.c:  ret = of_property_read_u32(np, "mux-int-port", &int_port);
imx-sgtl5000.c:  ret = of_property_read_u32(np, "mux-ext-port", &ext_port);
imx-wm8962.c:    ret = of_property_read_u32(np, "mux-int-port", &int_port);
imx-wm8962.c:    ret = of_property_read_u32(np, "mux-ext-port", &ext_port);

All drivers use mux-int|ext-port without prefix, tlv320 is the only
odd driver out, that's why I did this streamlining patch in the 1st place.

I would rather not have to change all other drivers to match the tlv320 one.

Thanks,
Jens
diff mbox

Patch

--- a/Documentation/devicetree/bindings/sound/eukrea-tlv320.txt
+++ b/Documentation/devicetree/bindings/sound/eukrea-tlv320.txt
@@ -1,16 +1,23 @@ 
-Audio complex for Eukrea boards with tlv320aic23 codec.
+Audio complex for Freescale i.MX boards with TI TLV320AIC23 I2S codecs,
+like those from Eukrea Electromatique.
 
 Required properties:
 
-  - compatible		: "eukrea,asoc-tlv320"
+  - compatible		: "fsl,imx-audio-tlv320aic23" or
+			  "eukrea,asoc-tlv320" (deprecated)
 
-  - eukrea,model	: The user-visible name of this sound complex.
+  - model		: The user-visible name of this sound complex.
+  - eukrea,model	: Dito, deprecated.
 
   - ssi-controller	: The phandle of the SSI controller.
 
-  - fsl,mux-int-port	: The internal port of the i.MX audio muxer (AUDMUX).
+  - mux-int-port	: The internal port of the i.MX audio muxer (AUDMUX).
+  - fsl,mux-int-port	: Dito, deprecated.
 
-  - fsl,mux-ext-port	: The external port of the i.MX audio muxer.
+  - mux-ext-port	: The external port of the i.MX audio muxer.
+  - fsl,mux-ext-port	: Dito, deprecated.
+
+  - audio-codec		: The phandle of the audio codec.
 
 Note: The AUDMUX port numbering should start at 1, which is consistent with
 hardware manual.
@@ -18,9 +25,10 @@  hardware manual.
 Example:
 
 	sound {
-		compatible = "eukrea,asoc-tlv320";
-		eukrea,model = "imx51-eukrea-tlv320aic23";
+		compatible = "fsl,imx-audio-tlv320aic23";
+		model = "imx51-eukrea-tlv320aic23";
 		ssi-controller = <&ssi2>;
-		fsl,mux-int-port = <2>;
-		fsl,mux-ext-port = <3>;
+		mux-int-port = <2>;
+		mux-ext-port = <3>;
+		audio-codec = <&codec>;
 	};
--- a/sound/soc/fsl/Kconfig
+++ b/sound/soc/fsl/Kconfig
@@ -220,7 +220,7 @@  config SND_SOC_PHYCORE_AC97
 	  and phyCARD boards in AC97 mode
 
 config SND_SOC_EUKREA_TLV320
-	tristate "Eukrea TLV320"
+	tristate "SoC Audio support for i.MX boards with TLV320AIC23"
 	depends on ARCH_MXC && I2C
 	select SND_SOC_TLV320AIC23_I2C
 	select SND_SOC_IMX_AUDMUX
--- a/sound/soc/fsl/eukrea-tlv320.c
+++ b/sound/soc/fsl/eukrea-tlv320.c
@@ -92,11 +92,13 @@  static int eukrea_tlv320_probe(struct platform_device *pdev)
 
 	eukrea_tlv320.dev = &pdev->dev;
 	if (np) {
-		ret = snd_soc_of_parse_card_name(&eukrea_tlv320,
-						 "eukrea,model");
+		ret = snd_soc_of_parse_card_name(&eukrea_tlv320, "model");
+		if (ret) /* backwards compatible */
+			ret = snd_soc_of_parse_card_name(&eukrea_tlv320,
+			                                 "eukrea,model");
 		if (ret) {
 			dev_err(&pdev->dev,
-				"eukrea,model node missing or invalid.\n");
+			        "model node missing or invalid.\n");
 			goto err;
 		}
 
@@ -109,22 +111,28 @@  static int eukrea_tlv320_probe(struct platform_device *pdev)
 			goto err;
 		}
 
-		codec_np = of_parse_phandle(ssi_np, "codec-handle", 0);
+		codec_np = of_parse_phandle(pdev->dev.of_node, "audio-codec", 0);
+		if (!codec_np) /* backwards compatible */
+			codec_np = of_parse_phandle(ssi_np, "codec-handle", 0);
 		if (codec_np)
 			eukrea_tlv320_dai.codec_of_node = codec_np;
 		else
-			dev_err(&pdev->dev, "codec-handle node missing or invalid.\n");
+			dev_err(&pdev->dev, "audio-codec node missing or invalid.\n");
 
-		ret = of_property_read_u32(np, "fsl,mux-int-port", &int_port);
+		ret = of_property_read_u32(np, "mux-int-port", &int_port);
+		if (ret) /* backwards compatible */
+			ret = of_property_read_u32(np, "fsl,mux-int-port", &int_port);
 		if (ret) {
 			dev_err(&pdev->dev,
-				"fsl,mux-int-port node missing or invalid.\n");
+			        "mux-int-port node missing or invalid.\n");
 			return ret;
 		}
-		ret = of_property_read_u32(np, "fsl,mux-ext-port", &ext_port);
+		ret = of_property_read_u32(np, "mux-ext-port", &ext_port);
+		if (ret) /* backwards compatible */
+			ret = of_property_read_u32(np, "fsl,mux-ext-port", &ext_port);
 		if (ret) {
 			dev_err(&pdev->dev,
-				"fsl,mux-ext-port node missing or invalid.\n");
+			        "mux-ext-port node missing or invalid.\n");
 			return ret;
 		}
 
@@ -213,7 +221,8 @@  static int eukrea_tlv320_remove(struct platform_device *pdev)
 }
 
 static const struct of_device_id imx_tlv320_dt_ids[] = {
-	{ .compatible = "eukrea,asoc-tlv320"},
+	{ .compatible = "eukrea,asoc-tlv320"}, /* backwards compatible */
+	{ .compatible = "fsl,imx-audio-tlv320aic23"},
 	{ /* sentinel */ }
 };
 MODULE_DEVICE_TABLE(of, imx_tlv320_dt_ids);