Re: [alsa-devel] [PATCH v4 1/2] ASoC: fsl: Add S/PDIF CPU DAI driver
On 08/15/2013 03:24 AM, Shawn Guo wrote: On Thu, Aug 15, 2013 at 10:18:23AM +0800, Nicolin Chen wrote: Hi Stephen, On Wed, Aug 14, 2013 at 09:47:19AM -0600, Stephen Warren wrote: If the clock source name list is different, then it needs a different compatible value, so that each compatible value can specify which clock names are required. Also, the compatible value itself should always include the exact HW that's present (most specific HW version), as well as any other HW it's compatible with. Thank you for the comments. Yes, I did so in v1-v3, but after rethinking about the situation (Actually both the HW version and the clock mux itself are same, just the clock sources connecting to the mux might be different), so I decided to do this by abstracting the driver from those source info and letting DT binding to pass such information. Because I think putting the clock sources into the driver differed by compatible value would make the driver more like SoC-specified, not the ideal way -- SoC-independent, since the clock sources are based on SoC design, not on itself. +1 It's pretty much the differences at SoC integration level not the IP itself, and it just happens to be handled in a register of the IP. OK, if the difference are the sources of the clocks and not the set of clocks, then there's no issue. I can't remember what triggered my comments above, but obviously it wasn't clear that this was the case. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [alsa-devel] [PATCH v4 1/2] ASoC: fsl: Add S/PDIF CPU DAI driver
On Thu, Aug 15, 2013 at 10:18:23AM +0800, Nicolin Chen wrote: Hi Stephen, On Wed, Aug 14, 2013 at 09:47:19AM -0600, Stephen Warren wrote: If the clock source name list is different, then it needs a different compatible value, so that each compatible value can specify which clock names are required. Also, the compatible value itself should always include the exact HW that's present (most specific HW version), as well as any other HW it's compatible with. Thank you for the comments. Yes, I did so in v1-v3, but after rethinking about the situation (Actually both the HW version and the clock mux itself are same, just the clock sources connecting to the mux might be different), so I decided to do this by abstracting the driver from those source info and letting DT binding to pass such information. Because I think putting the clock sources into the driver differed by compatible value would make the driver more like SoC-specified, not the ideal way -- SoC-independent, since the clock sources are based on SoC design, not on itself. +1 It's pretty much the differences at SoC integration level not the IP itself, and it just happens to be handled in a register of the IP. Shawn ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [alsa-devel] [PATCH v4 1/2] ASoC: fsl: Add S/PDIF CPU DAI driver
On Wed, Aug 14, 2013 at 01:30:17PM +0800, Nicolin Chen wrote: Hi Shwan, On Wed, Aug 14, 2013 at 11:27:00AM +0800, Shawn Guo wrote: I do not think we need this general compatible string. Device tree compatible should be specific. So I should just use 'fsl,chip-spdif and list all chip-spdif in compatible list? I added 'fsl,fsl-spdif' just for those not-in-list chips, Vybrid for example. So you think I should add all the possible chips to the list? But what if some chip I don't know right now that also uses this spdif controller? Add them to the list later? We only need to maintain those versions that require different programming model in the list. For example, if S/PDIF on Vybrid is completely compatible with imx6q one and uses the exactly same programming model, we do not need to maintain a compatible string for Vybrid S/PDIF at all. Instead, we only need to have something like below in Vybrid dts file, and S/PDIF driver will just work for it. compatible = fsl,vf600-spdif, fsl,imx6q-spdif; Shawn ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [alsa-devel] [PATCH v4 1/2] ASoC: fsl: Add S/PDIF CPU DAI driver
On Wed, Aug 14, 2013 at 02:39:33PM +0800, Shawn Guo wrote: We only need to maintain those versions that require different programming model in the list. For example, if S/PDIF on Vybrid is completely compatible with imx6q one and uses the exactly same programming model, we do not need to maintain a compatible string for Vybrid S/PDIF at all. Instead, we only need to have something like below in Vybrid dts file, and S/PDIF driver will just work for it. compatible = fsl,vf600-spdif, fsl,imx6q-spdif; Shawn Clear. Thank you for the explain. Then I think I can merely remain fsl,imx6q-spdif here, because all other cases should be completely compatible with this one. They are only different in the clock source names list, which's already being specified in dts file. Please correct me if you think this still isn't proper. Best regards, Nicolin Chen ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [alsa-devel] [PATCH v4 1/2] ASoC: fsl: Add S/PDIF CPU DAI driver
On Wed, Aug 14, 2013 at 02:34:45PM +0800, Nicolin Chen wrote: On Wed, Aug 14, 2013 at 02:39:33PM +0800, Shawn Guo wrote: We only need to maintain those versions that require different programming model in the list. For example, if S/PDIF on Vybrid is completely compatible with imx6q one and uses the exactly same programming model, we do not need to maintain a compatible string for Vybrid S/PDIF at all. Instead, we only need to have something like below in Vybrid dts file, and S/PDIF driver will just work for it. compatible = fsl,vf600-spdif, fsl,imx6q-spdif; Shawn Clear. Thank you for the explain. Then I think I can merely remain fsl,imx6q-spdif here, because all other cases should be completely compatible with this one. They are only different in the clock source names list, which's already being specified in dts file. Please correct me if you think this still isn't proper. And we generally prefer to use the soc that firstly integrates the IP to name the compatible. For IMX series, I think imx35 is the one, so we would name it fsl,imx35-spdif. Shawn ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [alsa-devel] [PATCH v4 1/2] ASoC: fsl: Add S/PDIF CPU DAI driver
On 08/14/2013 02:14 AM, Shawn Guo wrote: On Wed, Aug 14, 2013 at 02:34:45PM +0800, Nicolin Chen wrote: On Wed, Aug 14, 2013 at 02:39:33PM +0800, Shawn Guo wrote: We only need to maintain those versions that require different programming model in the list. For example, if S/PDIF on Vybrid is completely compatible with imx6q one and uses the exactly same programming model, we do not need to maintain a compatible string for Vybrid S/PDIF at all. Instead, we only need to have something like below in Vybrid dts file, and S/PDIF driver will just work for it. compatible = fsl,vf600-spdif, fsl,imx6q-spdif; Shawn Clear. Thank you for the explain. Then I think I can merely remain fsl,imx6q-spdif here, because all other cases should be completely compatible with this one. They are only different in the clock source names list, which's already being specified in dts file. Please correct me if you think this still isn't proper. If the clock source name list is different, then it needs a different compatible value, so that each compatible value can specify which clock names are required. Also, the compatible value itself should always include the exact HW that's present (most specific HW version), as well as any other HW it's compatible with. And we generally prefer to use the soc that firstly integrates the IP to name the compatible. For IMX series, I think imx35 is the one, so we would name it fsl,imx35-spdif. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [alsa-devel] [PATCH v4 1/2] ASoC: fsl: Add S/PDIF CPU DAI driver
Hi Stephen, On Wed, Aug 14, 2013 at 09:47:19AM -0600, Stephen Warren wrote: If the clock source name list is different, then it needs a different compatible value, so that each compatible value can specify which clock names are required. Also, the compatible value itself should always include the exact HW that's present (most specific HW version), as well as any other HW it's compatible with. Thank you for the comments. Yes, I did so in v1-v3, but after rethinking about the situation (Actually both the HW version and the clock mux itself are same, just the clock sources connecting to the mux might be different), so I decided to do this by abstracting the driver from those source info and letting DT binding to pass such information. Because I think putting the clock sources into the driver differed by compatible value would make the driver more like SoC-specified, not the ideal way -- SoC-independent, since the clock sources are based on SoC design, not on itself. Thank you, Nicolin Chen ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [alsa-devel] [PATCH v4 1/2] ASoC: fsl: Add S/PDIF CPU DAI driver
On Mon, Aug 12, 2013 at 9:01 AM, Nicolin Chen b42...@freescale.com wrote:
+Required properties:
+
+ - compatible : Compatible list, contains fsl,chip-spdif. Using general
Can't we just use fsl,fsl-spdif instead?
+ fsl,fsl-spdif will get the default SoC type -- imx6q-spdif.
+
I think this is not the usual approach we do with dt.
+static const struct of_device_id fsl_spdif_dt_ids[] = {
+ { .compatible = fsl,fsl-spdif, },
Isn't only the first entry enough here?
+ { .compatible = fsl,imx6q-spdif, },
+ { .compatible = fsl,imx6sl-spdif, },
+ { .compatible = fsl,imx53-spdif, },
+ {}
+};
+MODULE_DEVICE_TABLE(of, fsl_spdif_dt_ids);
+
+static struct platform_driver fsl_spdif_driver = {
+ .driver = {
+ .name = fsl-spdif-dai,
+ .owner = THIS_MODULE,
+ .of_match_table = fsl_spdif_dt_ids,
+ },
+ .probe = fsl_spdif_probe,
+ .remove = fsl_spdif_remove,
+};
+
+module_platform_driver(fsl_spdif_driver);
+
+MODULE_AUTHOR(Freescale Semiconductor, Inc.);
+MODULE_DESCRIPTION(Freescale S/PDIF CPU DAI Driver);
+MODULE_LICENSE(GPL v2);
+MODULE_ALIAS(platform:fsl_spdif);
This MODULE_ALIAS name does not match the name you provided earlier:
.name = fsl-spdif-dai
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [alsa-devel] [PATCH v4 1/2] ASoC: fsl: Add S/PDIF CPU DAI driver
On Tue, Aug 13, 2013 at 2:58 PM, Fabio Estevam feste...@gmail.com wrote: On Mon, Aug 12, 2013 at 9:01 AM, Nicolin Chen b42...@freescale.com wrote: +Required properties: + + - compatible : Compatible list, contains fsl,chip-spdif. Using general Can't we just use fsl,fsl-spdif instead? Or maybe fsl,imx35-spdif, since mx35 was the first imx SoC with this spdif IP block. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [alsa-devel] [PATCH v4 1/2] ASoC: fsl: Add S/PDIF CPU DAI driver
On Tue, Aug 13, 2013 at 02:58:26PM -0300, Fabio Estevam wrote: On Mon, Aug 12, 2013 at 9:01 AM, Nicolin Chen b42...@freescale.com wrote: +Required properties: + - compatible : Compatible list, contains fsl,chip-spdif. Using general Can't we just use fsl,fsl-spdif instead? It's better to list the specific chips in the DT so that we can quirk if we need to later. signature.asc Description: Digital signature ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [alsa-devel] [PATCH v4 1/2] ASoC: fsl: Add S/PDIF CPU DAI driver
Hi Fabio,
Thank you for the comments.
On Tue, Aug 13, 2013 at 02:58:26PM -0300, Fabio Estevam wrote:
On Mon, Aug 12, 2013 at 9:01 AM, Nicolin Chen b42...@freescale.com wrote:
+Required properties:
+
+ - compatible : Compatible list, contains fsl,chip-spdif. Using
general
Can't we just use fsl,fsl-spdif instead?
+ fsl,fsl-spdif will get the default SoC type -- imx6q-spdif.
+
I think this is not the usual approach we do with dt.
+static const struct of_device_id fsl_spdif_dt_ids[] = {
+ { .compatible = fsl,fsl-spdif, },
Isn't only the first entry enough here?
I just saw Mark's words as well. So I don't need to change this part right?
But I'd like to add imx35 on it.
This MODULE_ALIAS name does not match the name you provided earlier:
.name = fsl-spdif-dai
I'll fix it in v5.
Best regards,
Nicolin Chen
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [alsa-devel] [PATCH v4 1/2] ASoC: fsl: Add S/PDIF CPU DAI driver
On Tue, Aug 13, 2013 at 02:58:26PM -0300, Fabio Estevam wrote: On Mon, Aug 12, 2013 at 9:01 AM, Nicolin Chen b42...@freescale.com wrote: +Required properties: + + - compatible : Compatible list, contains fsl,chip-spdif. Using general Can't we just use fsl,fsl-spdif instead? + fsl,fsl-spdif will get the default SoC type -- imx6q-spdif. I do not think we need this general compatible string. Device tree compatible should be specific. Shawn ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [alsa-devel] [PATCH v4 1/2] ASoC: fsl: Add S/PDIF CPU DAI driver
Hi Shwan, On Wed, Aug 14, 2013 at 11:27:00AM +0800, Shawn Guo wrote: I do not think we need this general compatible string. Device tree compatible should be specific. So I should just use 'fsl,chip-spdif and list all chip-spdif in compatible list? I added 'fsl,fsl-spdif' just for those not-in-list chips, Vybrid for example. So you think I should add all the possible chips to the list? But what if some chip I don't know right now that also uses this spdif controller? Add them to the list later? Thank you. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
