[PATCH 1/5] ASoC: mc13783: Use module_platform_driver_probe()

[PATCH 1/5] ASoC: mc13783: Use module_platform_driver_probe()

[Alexander Shiyan]

mc13783-codec is probed only by MC13XXX MFD core driver so
use module_platform_driver_probe().

Signed-off-by: Alexander Shiyan <shc_work@mail.ru>
---
 sound/soc/codecs/mc13783.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/sound/soc/codecs/mc13783.c b/sound/soc/codecs/mc13783.c
index bae6016..8ab9668 100644
--- a/sound/soc/codecs/mc13783.c
+++ b/sound/soc/codecs/mc13783.c
@@ -750,7 +750,7 @@ static struct snd_soc_codec_driver soc_codec_dev_mc13783 = {
 	.num_dapm_routes = ARRAY_SIZE(mc13783_routes),
 };
 
-static int mc13783_codec_probe(struct platform_device *pdev)
+static int __init mc13783_codec_probe(struct platform_device *pdev)
 {
 	struct mc13xxx *mc13xxx;
 	struct mc13783_priv *priv;
@@ -804,11 +804,9 @@ static struct platform_driver mc13783_codec_driver = {
 		   .name = "mc13783-codec",
 		   .owner = THIS_MODULE,
 		   },
-	.probe = mc13783_codec_probe,
 	.remove = mc13783_codec_remove,
 };
-
-module_platform_driver(mc13783_codec_driver);
+module_platform_driver_probe(mc13783_codec_driver, mc13783_codec_probe);
 
 MODULE_DESCRIPTION("ASoC MC13783 driver");
 MODULE_AUTHOR("Sascha Hauer, Pengutronix <s.hauer@pengutronix.de>");
-- 
1.8.3.2

[PATCH 2/5] ASoC: mc13783: Use core error messages if registration fails

[Alexander Shiyan]

Signed-off-by: Alexander Shiyan <shc_work@mail.ru>
---
 sound/soc/codecs/mc13783.c | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/sound/soc/codecs/mc13783.c b/sound/soc/codecs/mc13783.c
index 8ab9668..c2def5d 100644
--- a/sound/soc/codecs/mc13783.c
+++ b/sound/soc/codecs/mc13783.c
@@ -781,14 +781,6 @@ static int __init mc13783_codec_probe(struct platform_device *pdev)
 		ret = snd_soc_register_codec(&pdev->dev, &soc_codec_dev_mc13783,
 			mc13783_dai_async, ARRAY_SIZE(mc13783_dai_async));
 
-	if (ret)
-		goto err_register_codec;
-
-	return 0;
-
-err_register_codec:
-	dev_err(&pdev->dev, "register codec failed with %d\n", ret);
-
 	return ret;
 }
 
-- 
1.8.3.2

[PATCH 3/5] ASoC: mc13783: Drop fixed ADC & DAC ports usage

[Alexander Shiyan]

There are no users of this driver without pdata, so stop using
constant assignment of ADC and DAC ports.

Signed-off-by: Alexander Shiyan <shc_work@mail.ru>
---
 sound/soc/codecs/mc13783.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/sound/soc/codecs/mc13783.c b/sound/soc/codecs/mc13783.c
index c2def5d..997f708 100644
--- a/sound/soc/codecs/mc13783.c
+++ b/sound/soc/codecs/mc13783.c
@@ -770,8 +770,7 @@ static int __init mc13783_codec_probe(struct platform_device *pdev)
 		priv->adc_ssi_port = pdata->adc_ssi_port;
 		priv->dac_ssi_port = pdata->dac_ssi_port;
 	} else {
-		priv->adc_ssi_port = MC13783_SSI1_PORT;
-		priv->dac_ssi_port = MC13783_SSI2_PORT;
+		return -ENOSYS;
 	}
 
 	if (priv->adc_ssi_port == priv->dac_ssi_port)
-- 
1.8.3.2

[PATCH 4/5] ASoC: mc13783: trivial: Cleanup module

[Alexander Shiyan]

This is a trivial cleanup: remove useless variable mc13xxx and
extra spaces. No functional changes.

Signed-off-by: Alexander Shiyan <shc_work@mail.ru>
---
 sound/soc/codecs/mc13783.c | 17 +++++++----------
 1 file changed, 7 insertions(+), 10 deletions(-)

diff --git a/sound/soc/codecs/mc13783.c b/sound/soc/codecs/mc13783.c
index 997f708..582c2bb 100644
--- a/sound/soc/codecs/mc13783.c
+++ b/sound/soc/codecs/mc13783.c
@@ -752,20 +752,14 @@ static struct snd_soc_codec_driver soc_codec_dev_mc13783 = {
 
 static int __init mc13783_codec_probe(struct platform_device *pdev)
 {
-	struct mc13xxx *mc13xxx;
 	struct mc13783_priv *priv;
 	struct mc13xxx_codec_platform_data *pdata = pdev->dev.platform_data;
 	int ret;
 
-	mc13xxx = dev_get_drvdata(pdev->dev.parent);
-
-
 	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
-	if (priv == NULL)
+	if (!priv)
 		return -ENOMEM;
 
-	dev_set_drvdata(&pdev->dev, priv);
-	priv->mc13xxx = mc13xxx;
 	if (pdata) {
 		priv->adc_ssi_port = pdata->adc_ssi_port;
 		priv->dac_ssi_port = pdata->dac_ssi_port;
@@ -773,6 +767,9 @@ static int __init mc13783_codec_probe(struct platform_device *pdev)
 		return -ENOSYS;
 	}
 
+	dev_set_drvdata(&pdev->dev, priv);
+	priv->mc13xxx = dev_get_drvdata(pdev->dev.parent);
+
 	if (priv->adc_ssi_port == priv->dac_ssi_port)
 		ret = snd_soc_register_codec(&pdev->dev, &soc_codec_dev_mc13783,
 			mc13783_dai_sync, ARRAY_SIZE(mc13783_dai_sync));
@@ -792,9 +789,9 @@ static int mc13783_codec_remove(struct platform_device *pdev)
 
 static struct platform_driver mc13783_codec_driver = {
 	.driver = {
-		   .name = "mc13783-codec",
-		   .owner = THIS_MODULE,
-		   },
+		.name	= "mc13783-codec",
+		.owner	= THIS_MODULE,
+	},
 	.remove = mc13783_codec_remove,
 };
 module_platform_driver_probe(mc13783_codec_driver, mc13783_codec_probe);
-- 
1.8.3.2

[PATCH 5/5] ASoC: mc13783: Add devicetree support

[Alexander Shiyan]

This patch adds devicetree support for mc13783-codec.
Since we have not compatible string for this codec, just override
of_node for allow using phandle of node in the DT.

Signed-off-by: Alexander Shiyan <shc_work@mail.ru>
---
 Documentation/devicetree/bindings/mfd/mc13xxx.txt |  4 +++-
 drivers/mfd/mc13xxx-core.c                        | 12 +++++++-----
 sound/soc/codecs/mc13783.c                        | 20 ++++++++++++++++++++
 3 files changed, 30 insertions(+), 6 deletions(-)

diff --git a/Documentation/devicetree/bindings/mfd/mc13xxx.txt b/Documentation/devicetree/bindings/mfd/mc13xxx.txt
index abd9e3c..f3047c9 100644
--- a/Documentation/devicetree/bindings/mfd/mc13xxx.txt
+++ b/Documentation/devicetree/bindings/mfd/mc13xxx.txt
@@ -5,11 +5,13 @@ Required properties:
 
 Optional properties:
 - fsl,mc13xxx-uses-adc : Indicate the ADC is being used
-- fsl,mc13xxx-uses-codec : Indicate the Audio Codec is being used
 - fsl,mc13xxx-uses-rtc : Indicate the RTC is being used
 - fsl,mc13xxx-uses-touch : Indicate the touchscreen controller is being used
 
 Sub-nodes:
+- codec: Contain the Audio Codec node.
+  - adc-port: Contain PMIC SSI port used for ADC.
+  - dac-port: Contain PMIC SSI port used for DAC.
 - regulators : Contain the regulator nodes. The regulators are bound using
   their names as listed below with their registers and bits for enabling.
 
diff --git a/drivers/mfd/mc13xxx-core.c b/drivers/mfd/mc13xxx-core.c
index 06e64b6..7d144aa 100644
--- a/drivers/mfd/mc13xxx-core.c
+++ b/drivers/mfd/mc13xxx-core.c
@@ -618,7 +618,7 @@ static int mc13xxx_probe_flags_dt(struct mc13xxx *mc13xxx)
 	if (of_get_property(np, "fsl,mc13xxx-uses-adc", NULL))
 		mc13xxx->flags |= MC13XXX_USE_ADC;
 
-	if (of_get_property(np, "fsl,mc13xxx-uses-codec", NULL))
+	if (of_find_node_by_name(np, "codec"))
 		mc13xxx->flags |= MC13XXX_USE_CODEC;
 
 	if (of_get_property(np, "fsl,mc13xxx-uses-rtc", NULL))
@@ -673,10 +673,6 @@ int mc13xxx_common_init(struct device *dev)
 	if (mc13xxx->flags & MC13XXX_USE_ADC)
 		mc13xxx_add_subdevice(mc13xxx, "%s-adc");
 
-	if (mc13xxx->flags & MC13XXX_USE_CODEC)
-		mc13xxx_add_subdevice_pdata(mc13xxx, "%s-codec",
-					pdata->codec, sizeof(*pdata->codec));
-
 	if (mc13xxx->flags & MC13XXX_USE_RTC)
 		mc13xxx_add_subdevice(mc13xxx, "%s-rtc");
 
@@ -691,10 +687,16 @@ int mc13xxx_common_init(struct device *dev)
 				pdata->leds, sizeof(*pdata->leds));
 		mc13xxx_add_subdevice_pdata(mc13xxx, "%s-pwrbutton",
 				pdata->buttons, sizeof(*pdata->buttons));
+		if (mc13xxx->flags & MC13XXX_USE_CODEC)
+			mc13xxx_add_subdevice_pdata(mc13xxx, "%s-codec",
+						    pdata->codec,
+						    sizeof(*pdata->codec));
 	} else {
 		mc13xxx_add_subdevice(mc13xxx, "%s-regulator");
 		mc13xxx_add_subdevice(mc13xxx, "%s-led");
 		mc13xxx_add_subdevice(mc13xxx, "%s-pwrbutton");
+		if (mc13xxx->flags & MC13XXX_USE_CODEC)
+			mc13xxx_add_subdevice(mc13xxx, "%s-codec");
 	}
 
 	return 0;
diff --git a/sound/soc/codecs/mc13783.c b/sound/soc/codecs/mc13783.c
index 582c2bb..cd0181f 100644
--- a/sound/soc/codecs/mc13783.c
+++ b/sound/soc/codecs/mc13783.c
@@ -23,6 +23,7 @@
 #include <linux/module.h>
 #include <linux/device.h>
 #include <linux/mfd/mc13xxx.h>
+#include <linux/of.h>
 #include <linux/slab.h>
 #include <sound/core.h>
 #include <sound/control.h>
@@ -760,13 +761,32 @@ static int __init mc13783_codec_probe(struct platform_device *pdev)
 	if (!priv)
 		return -ENOMEM;
 
+	pdev->dev.of_node = of_find_node_by_name(pdev->dev.parent->of_node,
+						 "codec");
+
 	if (pdata) {
 		priv->adc_ssi_port = pdata->adc_ssi_port;
 		priv->dac_ssi_port = pdata->dac_ssi_port;
+	} else if (pdev->dev.of_node) {
+		ret = of_property_read_u32(pdev->dev.of_node, "adc-port",
+					   &priv->adc_ssi_port);
+		if (ret)
+			return ret;
+		ret = of_property_read_u32(pdev->dev.of_node, "dac-port",
+					   &priv->dac_ssi_port);
+		if (ret)
+			return ret;
 	} else {
 		return -ENOSYS;
 	}
 
+	if (priv->adc_ssi_port != MC13783_SSI1_PORT &&
+	    priv->adc_ssi_port != MC13783_SSI2_PORT)
+		return -EINVAL;
+	if (priv->dac_ssi_port != MC13783_SSI1_PORT &&
+	    priv->dac_ssi_port != MC13783_SSI2_PORT)
+		return -EINVAL;
+
 	dev_set_drvdata(&pdev->dev, priv);
 	priv->mc13xxx = dev_get_drvdata(pdev->dev.parent);
 
-- 
1.8.3.2

Re: [PATCH 5/5] ASoC: mc13783: Add devicetree support

[Mark Brown]

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

On Sun, Jan 05, 2014 at 11:38:35AM +0400, Alexander Shiyan wrote:
> This patch adds devicetree support for mc13783-codec.
> Since we have not compatible string for this codec, just override
> of_node for allow using phandle of node in the DT.

All DT patches should be sent to the DT maintainers for review.

>  Documentation/devicetree/bindings/mfd/mc13xxx.txt |  4 +++-
>  drivers/mfd/mc13xxx-core.c                        | 12 +++++++-----
> 
Since this has MFD updates you also need to send it to the MFD
maintainers.

>  Optional properties:
>  - fsl,mc13xxx-uses-adc : Indicate the ADC is being used
> -- fsl,mc13xxx-uses-codec : Indicate the Audio Codec is being used

This is an incompatible binding change; deprecating is fine but it
should still be supported.  Is there not a default set of port
assignments that can be used?

> +	pdev->dev.of_node = of_find_node_by_name(pdev->dev.parent->of_node,
> +						 "codec");
> +

You shouldn't be messing with the of_node in the struct device, this is
managed by the driver model and setting one that wasn't added in sync
with everything else can introduce breakage.

> +	if (priv->adc_ssi_port != MC13783_SSI1_PORT &&
> +	    priv->adc_ssi_port != MC13783_SSI2_PORT)
> +		return -EINVAL;
> +	if (priv->dac_ssi_port != MC13783_SSI1_PORT &&
> +	    priv->dac_ssi_port != MC13783_SSI2_PORT)
> +		return -EINVAL;

What about DAC or ADC only systems?

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

Re: [PATCH 5/5] ASoC: mc13783: Add devicetree support

[Alexander Shiyan]

Hello.

Понедельник,  6 января 2014, 13:28 UTC от Mark Brown <broonie@kernel.org>:
> On Sun, Jan 05, 2014 at 11:38:35AM +0400, Alexander Shiyan wrote:
> > This patch adds devicetree support for mc13783-codec.
> > Since we have not compatible string for this codec, just override
> > of_node for allow using phandle of node in the DT.
> 
> All DT patches should be sent to the DT maintainers for review.
> 
> >  Documentation/devicetree/bindings/mfd/mc13xxx.txt |  4 +++-
> >  drivers/mfd/mc13xxx-core.c                        | 12 +++++++-----
> > 
> Since this has MFD updates you also need to send it to the MFD
> maintainers.
> 
> >  Optional properties:
> >  - fsl,mc13xxx-uses-adc : Indicate the ADC is being used
> > -- fsl,mc13xxx-uses-codec : Indicate the Audio Codec is being used
> 
> This is an incompatible binding change; deprecating is fine but it
> should still be supported.  Is there not a default set of port
> assignments that can be used?

"fsl,mc13xxx-uses-codec" is not used anywhere and even if it was used,
it would not usable, because we can not get to the codec phandle.
So I think this change is safe.

> 
> > +	pdev->dev.of_node = of_find_node_by_name(pdev->dev.parent->of_node,
> > +						 "codec");
> > +
> 
> You shouldn't be messing with the of_node in the struct device, this is
> managed by the driver model and setting one that wasn't added in sync
> with everything else can introduce breakage.

Hmm, probably you are right, but I cannot imagine other way to set of_node...
Have you other ideas?

> > +	if (priv->adc_ssi_port != MC13783_SSI1_PORT &&
> > +	    priv->adc_ssi_port != MC13783_SSI2_PORT)
> > +		return -EINVAL;
> > +	if (priv->dac_ssi_port != MC13783_SSI1_PORT &&
> > +	    priv->dac_ssi_port != MC13783_SSI2_PORT)
> > +		return -EINVAL;
> 
> What about DAC or ADC only systems?

In this case we should rewrite this codec driver completely from scratch.
...

Thanks.

---
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH 5/5] ASoC: mc13783: Add devicetree support

[Mark Brown]

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

On Mon, Jan 06, 2014 at 05:46:21PM +0400, Alexander Shiyan wrote:
> Понедельник,  6 января 2014, 13:28 UTC от Mark Brown <broonie@kernel.org>:

> > This is an incompatible binding change; deprecating is fine but it
> > should still be supported.  Is there not a default set of port
> > assignments that can be used?

> "fsl,mc13xxx-uses-codec" is not used anywhere and even if it was used,
> it would not usable, because we can not get to the codec phandle.
> So I think this change is safe.

Why would it prevent anything getting the CODEC phandle and what do you
mean by that anyway?  A lack of in tree users shouldn't mean anything,
we need to be compatible with DTs shipped with boards.

> > > +	pdev->dev.of_node = of_find_node_by_name(pdev->dev.parent->of_node,
> > > +						 "codec");

> > You shouldn't be messing with the of_node in the struct device, this is
> > managed by the driver model and setting one that wasn't added in sync
> > with everything else can introduce breakage.

> Hmm, probably you are right, but I cannot imagine other way to set of_node...
> Have you other ideas?

The whole problem is that you're trying to set of_node.  Don't do that.

> > > +	if (priv->adc_ssi_port != MC13783_SSI1_PORT &&
> > > +	    priv->adc_ssi_port != MC13783_SSI2_PORT)
> > > +		return -EINVAL;
> > > +	if (priv->dac_ssi_port != MC13783_SSI1_PORT &&
> > > +	    priv->dac_ssi_port != MC13783_SSI2_PORT)
> > > +		return -EINVAL;

> > What about DAC or ADC only systems?

> In this case we should rewrite this codec driver completely from scratch.

What makes you say that?

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

Re: [PATCH 1/5] ASoC: mc13783: Use module_platform_driver_probe()

[Mark Brown]

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

On Sun, Jan 05, 2014 at 11:38:31AM +0400, Alexander Shiyan wrote:
> mc13783-codec is probed only by MC13XXX MFD core driver so
> use module_platform_driver_probe().

Applied, thanks.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

Re: [PATCH 2/5] ASoC: mc13783: Use core error messages if registration fails

[Mark Brown]

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

On Sun, Jan 05, 2014 at 11:38:32AM +0400, Alexander Shiyan wrote:
> Signed-off-by: Alexander Shiyan <shc_work@mail.ru>

Applied, thanks.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

Re: [PATCH 3/5] ASoC: mc13783: Drop fixed ADC & DAC ports usage

[Mark Brown]

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

On Sun, Jan 05, 2014 at 11:38:33AM +0400, Alexander Shiyan wrote:
> There are no users of this driver without pdata, so stop using
> constant assignment of ADC and DAC ports.

Applied, thanks.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

Re: [PATCH 4/5] ASoC: mc13783: trivial: Cleanup module

[Mark Brown]

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

On Sun, Jan 05, 2014 at 11:38:34AM +0400, Alexander Shiyan wrote:
> This is a trivial cleanup: remove useless variable mc13xxx and
> extra spaces. No functional changes.

Applied, thanks.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

Re: [PATCH 5/5] ASoC: mc13783: Add devicetree support

[Alexander Shiyan]

Hello.

On Mon, 6 Jan 2014 15:12:27 +0000
Mark Brown <broonie@kernel.org> wrote:
> > > This is an incompatible binding change; deprecating is fine but it
> > > should still be supported.  Is there not a default set of port
> > > assignments that can be used?
> 
> > "fsl,mc13xxx-uses-codec" is not used anywhere and even if it was used,
> > it would not usable, because we can not get to the codec phandle.
> > So I think this change is safe.
> 
> Why would it prevent anything getting the CODEC phandle and what do you
> mean by that anyway?  A lack of in tree users shouldn't mean anything,
> we need to be compatible with DTs shipped with boards.
> 
> > > > +	pdev->dev.of_node = of_find_node_by_name(pdev->dev.parent->of_node,
> > > > +						 "codec");
> 
> > > You shouldn't be messing with the of_node in the struct device, this is
> > > managed by the driver model and setting one that wasn't added in sync
> > > with everything else can introduce breakage.
> 
> > Hmm, probably you are right, but I cannot imagine other way to set of_node...
> > Have you other ideas?
> 
> The whole problem is that you're trying to set of_node.  Don't do that.

I try to explain my vision of this:

With DT and simple audio codec we should have description like this:
	sound {
		compatible = "simple-audio-card";
		...
		simple-audio-card,cpu {
			sound-dai = <&ssi1>;
			...
		};
		simple-audio-card,codec {
			sound-dai = <&mc13783codec>;
			...
		};
	};
So, we need codec phandle.
The codec is probed by MC13XXX MFD core, and codec pdev not contain
compatible string, but we should associate our codec with DT.
if ASoC core wants to use codec it will try to find it by of_node.
Maybe my opinion is wrong, fixme.
Thanks.

-- 
Alexander Shiyan <shc_work@mail.ru>

Re: [PATCH 5/5] ASoC: mc13783: Add devicetree support

[Mark Brown]

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

On Tue, Jan 07, 2014 at 11:00:53AM +0400, Alexander Shiyan wrote:

> So, we need codec phandle.
> The codec is probed by MC13XXX MFD core, and codec pdev not contain
> compatible string, but we should associate our codec with DT.
> if ASoC core wants to use codec it will try to find it by of_node.
> Maybe my opinion is wrong, fixme.

The fact that we happen to use platform devices inside Linux to split
the MFD between subsystems isn't something that should leak out into the
device tree representation, this is a Linux implementation detail.  The
card should be referencing the root node for the device in its binding.

You're also confusing the idea of using an of_node for something with
putting it into the struct device; you should never need to modify
what's in the struct device to use the device tree data.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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