| Message ID | 20160730142716.29377-3-codekipper@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 2f6963cb52bee440105f732b3411f18e38ed6e52 |
| Headers | show |
Hi Marcus, On Sun, Jul 31, 2016 at 12:27 AM, <codekipper@gmail.com> wrote: > From: Marcus Cooper <codekipper@gmail.com> > > The A31 SoC uses the same SPDIF block as found in earlier SoCs, but its > reset is controlled via a separate reset controller. > > The DMA also complains when the maxburst is set to 4 so it's been adjusted > to 8 which suites both the older and newer SoCs. > > Signed-off-by: Marcus Cooper <codekipper@gmail.com> > --- > sound/soc/sunxi/sun4i-spdif.c | 17 ++++++++++++++++- > 1 file changed, 16 insertions(+), 1 deletion(-) > > diff --git a/sound/soc/sunxi/sun4i-spdif.c b/sound/soc/sunxi/sun4i-spdif.c > index 0b04fb0..88fbb3a 100644 > --- a/sound/soc/sunxi/sun4i-spdif.c > +++ b/sound/soc/sunxi/sun4i-spdif.c > @@ -482,11 +485,23 @@ static int sun4i_spdif_probe(struct platform_device *pdev) > } > > host->dma_params_tx.addr = res->start + SUN4I_SPDIF_TXFIFO; > - host->dma_params_tx.maxburst = 4; > + host->dma_params_tx.maxburst = 8; > host->dma_params_tx.addr_width = DMA_SLAVE_BUSWIDTH_2_BYTES; > > platform_set_drvdata(pdev, host); > > + if (of_device_is_compatible(pdev->dev.of_node, > + "allwinner,sun6i-a31-spdif")) { Given how much Allwinner likes to shuffle stuff around with each SoC generation, would it make sense to add a flag for this in some compatible specific config structure instead of checking against the compatible? Thanks,
30.07.2016, 22:45, "codekipper@gmail.com" <codekipper@gmail.com>: > From: Marcus Cooper <codekipper@gmail.com> > > The A31 SoC uses the same SPDIF block as found in earlier SoCs, but its > reset is controlled via a separate reset controller. > > The DMA also complains when the maxburst is set to 4 so it's been adjusted > to 8 which suites both the older and newer SoCs. > > Signed-off-by: Marcus Cooper <codekipper@gmail.com> > --- > sound/soc/sunxi/sun4i-spdif.c | 17 ++++++++++++++++- > 1 file changed, 16 insertions(+), 1 deletion(-) > > diff --git a/sound/soc/sunxi/sun4i-spdif.c b/sound/soc/sunxi/sun4i-spdif.c > index 0b04fb0..88fbb3a 100644 > --- a/sound/soc/sunxi/sun4i-spdif.c > +++ b/sound/soc/sunxi/sun4i-spdif.c > @@ -29,6 +29,7 @@ > #include <linux/module.h> > #include <linux/platform_device.h> > #include <linux/pm_runtime.h> > +#include <linux/reset.h> > #include <sound/dmaengine_pcm.h> > #include <sound/pcm_params.h> > #include <sound/soc.h> > @@ -162,6 +163,7 @@ struct sun4i_spdif_dev { > struct platform_device *pdev; > struct clk *spdif_clk; > struct clk *apb_clk; > + struct reset_control *rst; > struct snd_soc_dai_driver cpu_dai_drv; > struct regmap *regmap; > struct snd_dmaengine_dai_dma_data dma_params_tx; > @@ -411,6 +413,7 @@ static const struct snd_soc_dapm_route dit_routes[] = { > > static const struct of_device_id sun4i_spdif_of_match[] = { > { .compatible = "allwinner,sun4i-a10-spdif", }, > + { .compatible = "allwinner,sun6i-a31-spdif", }, > { /* sentinel */ } > }; > MODULE_DEVICE_TABLE(of, sun4i_spdif_of_match); > @@ -482,11 +485,23 @@ static int sun4i_spdif_probe(struct platform_device *pdev) > } > > host->dma_params_tx.addr = res->start + SUN4I_SPDIF_TXFIFO; > - host->dma_params_tx.maxburst = 4; > + host->dma_params_tx.maxburst = 8; > host->dma_params_tx.addr_width = DMA_SLAVE_BUSWIDTH_2_BYTES; > > platform_set_drvdata(pdev, host); > > + if (of_device_is_compatible(pdev->dev.of_node, > + "allwinner,sun6i-a31-spdif")) { > + host->rst = devm_reset_control_get_optional(&pdev->dev, NULL); > + if (IS_ERR(host->rst) && PTR_ERR(host->rst) == -EPROBE_DEFER) { > + ret = -EPROBE_DEFER; > + dev_err(&pdev->dev, "Failed to get reset: %d\n", ret); > + goto err_disable_apb_clk; > + } > + if (!IS_ERR(host->rst)) > + reset_control_deassert(host->rst); > + } > + I think you do not need the compatible. You can just detect whether the reset is present. > ret = devm_snd_soc_register_component(&pdev->dev, > &sun4i_spdif_component, &sun4i_spdif_dai, 1); > if (ret) > -- > 2.9.2 > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Sat, Jul 30, 2016 at 04:27:16PM +0200, codekipper@gmail.com wrote: > From: Marcus Cooper <codekipper@gmail.com> > > The A31 SoC uses the same SPDIF block as found in earlier SoCs, but its > reset is controlled via a separate reset controller. > > The DMA also complains when the maxburst is set to 4 so it's been adjusted > to 8 which suites both the older and newer SoCs. > > Signed-off-by: Marcus Cooper <codekipper@gmail.com> Acked-by: Maxime Ripard <maxime.ripard@free-electrons.com> Thanks, Maxime
On Sat, Jul 30, 2016 at 10:52:45PM +0800, Icenowy Zheng wrote: > > + if (of_device_is_compatible(pdev->dev.of_node, > > + "allwinner,sun6i-a31-spdif")) { > > + host->rst = devm_reset_control_get_optional(&pdev->dev, NULL); > > + if (IS_ERR(host->rst) && PTR_ERR(host->rst) == -EPROBE_DEFER) { > > + ret = -EPROBE_DEFER; > > + dev_err(&pdev->dev, "Failed to get reset: %d\n", ret); > > + goto err_disable_apb_clk; > > + } > > + if (!IS_ERR(host->rst)) > > + reset_control_deassert(host->rst); > > + } > > + > I think you do not need the compatible. > You can just detect whether the reset is present. That would weaken the error check. If we're running on the A31 and are missing our reset property, it would go unnoticed. Maxime
On Sun, Jul 31, 2016 at 12:40:48AM +1000, Julian Calaby wrote: > Hi Marcus, > > On Sun, Jul 31, 2016 at 12:27 AM, <codekipper@gmail.com> wrote: > > From: Marcus Cooper <codekipper@gmail.com> > > > > The A31 SoC uses the same SPDIF block as found in earlier SoCs, but its > > reset is controlled via a separate reset controller. > > > > The DMA also complains when the maxburst is set to 4 so it's been adjusted > > to 8 which suites both the older and newer SoCs. > > > > Signed-off-by: Marcus Cooper <codekipper@gmail.com> > > --- > > sound/soc/sunxi/sun4i-spdif.c | 17 ++++++++++++++++- > > 1 file changed, 16 insertions(+), 1 deletion(-) > > > > diff --git a/sound/soc/sunxi/sun4i-spdif.c b/sound/soc/sunxi/sun4i-spdif.c > > index 0b04fb0..88fbb3a 100644 > > --- a/sound/soc/sunxi/sun4i-spdif.c > > +++ b/sound/soc/sunxi/sun4i-spdif.c > > @@ -482,11 +485,23 @@ static int sun4i_spdif_probe(struct platform_device *pdev) > > } > > > > host->dma_params_tx.addr = res->start + SUN4I_SPDIF_TXFIFO; > > - host->dma_params_tx.maxburst = 4; > > + host->dma_params_tx.maxburst = 8; > > host->dma_params_tx.addr_width = DMA_SLAVE_BUSWIDTH_2_BYTES; > > > > platform_set_drvdata(pdev, host); > > > > + if (of_device_is_compatible(pdev->dev.of_node, > > + "allwinner,sun6i-a31-spdif")) { > > Given how much Allwinner likes to shuffle stuff around with each SoC > generation, would it make sense to add a flag for this in some > compatible specific config structure instead of checking against the > compatible? It really depends on the size of the quirks you have to maintain. If it's just a single one like here, I'd say it's less of a hassle (and actually easier and conciser to implement). Maxime
30.07.2016, 23:20, "maxime.ripard@free-electrons.com" <maxime.ripard@free-electrons.com>: > On Sat, Jul 30, 2016 at 10:52:45PM +0800, Icenowy Zheng wrote: >> > + if (of_device_is_compatible(pdev->dev.of_node, >> > + "allwinner,sun6i-a31-spdif")) { >> > + host->rst = devm_reset_control_get_optional(&pdev->dev, NULL); >> > + if (IS_ERR(host->rst) && PTR_ERR(host->rst) == -EPROBE_DEFER) { >> > + ret = -EPROBE_DEFER; >> > + dev_err(&pdev->dev, "Failed to get reset: %d\n", ret); >> > + goto err_disable_apb_clk; >> > + } >> > + if (!IS_ERR(host->rst)) >> > + reset_control_deassert(host->rst); >> > + } >> > + >> I think you do not need the compatible. >> You can just detect whether the reset is present. > > That would weaken the error check. If we're running on the A31 and are > missing our reset property, it would go unnoticed. The reset property is in the SoC's dtsi file, so it won't be easily missing... > > Maxime > > -- > Maxime Ripard, Free Electrons > Embedded Linux and Kernel engineering > http://free-electrons.com
Hi, On Sat, Jul 30, 2016 at 11:20 PM, maxime.ripard@free-electrons.com <maxime.ripard@free-electrons.com> wrote: > On Sat, Jul 30, 2016 at 10:52:45PM +0800, Icenowy Zheng wrote: >> > + if (of_device_is_compatible(pdev->dev.of_node, >> > + "allwinner,sun6i-a31-spdif")) { >> > + host->rst = devm_reset_control_get_optional(&pdev->dev, NULL); >> > + if (IS_ERR(host->rst) && PTR_ERR(host->rst) == -EPROBE_DEFER) { >> > + ret = -EPROBE_DEFER; >> > + dev_err(&pdev->dev, "Failed to get reset: %d\n", ret); >> > + goto err_disable_apb_clk; >> > + } >> > + if (!IS_ERR(host->rst)) >> > + reset_control_deassert(host->rst); >> > + } >> > + >> I think you do not need the compatible. >> You can just detect whether the reset is present. > > That would weaken the error check. If we're running on the A31 and are > missing our reset property, it would go unnoticed. We've been doing it this way with the mmc controller and the usb hosts though. IIRC you once said in the older SoCs, the reset control is tied to the clock gate in the hardware. The _optional variant is also funny, though I understand it is a design of the reset controller framework. Regards ChenYu
01.08.2016, 21:40, "Chen-Yu Tsai" <wens@csie.org>: > Hi, > > On Sat, Jul 30, 2016 at 11:20 PM, maxime.ripard@free-electrons.com > <maxime.ripard@free-electrons.com> wrote: >> On Sat, Jul 30, 2016 at 10:52:45PM +0800, Icenowy Zheng wrote: >>> > + if (of_device_is_compatible(pdev->dev.of_node, >>> > + "allwinner,sun6i-a31-spdif")) { >>> > + host->rst = devm_reset_control_get_optional(&pdev->dev, NULL); >>> > + if (IS_ERR(host->rst) && PTR_ERR(host->rst) == -EPROBE_DEFER) { >>> > + ret = -EPROBE_DEFER; >>> > + dev_err(&pdev->dev, "Failed to get reset: %d\n", ret); >>> > + goto err_disable_apb_clk; >>> > + } >>> > + if (!IS_ERR(host->rst)) >>> > + reset_control_deassert(host->rst); >>> > + } >>> > + >>> I think you do not need the compatible. >>> You can just detect whether the reset is present. >> >> That would weaken the error check. If we're running on the A31 and are >> missing our reset property, it would go unnoticed. > > We've been doing it this way with the mmc controller and the usb hosts though. I did the same thing on NAND controller. > IIRC you once said in the older SoCs, the reset control is tied to the clock > gate in the hardware. > > The _optional variant is also funny, though I understand it is a design > of the reset controller framework. > > Regards > ChenYu
Hi, On Mon, Aug 01, 2016 at 09:39:34PM +0800, Chen-Yu Tsai wrote: > Hi, > > On Sat, Jul 30, 2016 at 11:20 PM, maxime.ripard@free-electrons.com > <maxime.ripard@free-electrons.com> wrote: > > On Sat, Jul 30, 2016 at 10:52:45PM +0800, Icenowy Zheng wrote: > >> > + if (of_device_is_compatible(pdev->dev.of_node, > >> > + "allwinner,sun6i-a31-spdif")) { > >> > + host->rst = devm_reset_control_get_optional(&pdev->dev, NULL); > >> > + if (IS_ERR(host->rst) && PTR_ERR(host->rst) == -EPROBE_DEFER) { > >> > + ret = -EPROBE_DEFER; > >> > + dev_err(&pdev->dev, "Failed to get reset: %d\n", ret); > >> > + goto err_disable_apb_clk; > >> > + } > >> > + if (!IS_ERR(host->rst)) > >> > + reset_control_deassert(host->rst); > >> > + } > >> > + > >> I think you do not need the compatible. > >> You can just detect whether the reset is present. > > > > That would weaken the error check. If we're running on the A31 and are > > missing our reset property, it would go unnoticed. > > We've been doing it this way with the mmc controller and the usb hosts though. > IIRC you once said in the older SoCs, the reset control is tied to the clock > gate in the hardware. > > The _optional variant is also funny, though I understand it is a design > of the reset controller framework. Yes, I know. But that doesn't prevent that design from being better. Maxime
diff --git a/sound/soc/sunxi/sun4i-spdif.c b/sound/soc/sunxi/sun4i-spdif.c index 0b04fb0..88fbb3a 100644 --- a/sound/soc/sunxi/sun4i-spdif.c +++ b/sound/soc/sunxi/sun4i-spdif.c @@ -29,6 +29,7 @@ #include <linux/module.h> #include <linux/platform_device.h> #include <linux/pm_runtime.h> +#include <linux/reset.h> #include <sound/dmaengine_pcm.h> #include <sound/pcm_params.h> #include <sound/soc.h> @@ -162,6 +163,7 @@ struct sun4i_spdif_dev { struct platform_device *pdev; struct clk *spdif_clk; struct clk *apb_clk; + struct reset_control *rst; struct snd_soc_dai_driver cpu_dai_drv; struct regmap *regmap; struct snd_dmaengine_dai_dma_data dma_params_tx; @@ -411,6 +413,7 @@ static const struct snd_soc_dapm_route dit_routes[] = { static const struct of_device_id sun4i_spdif_of_match[] = { { .compatible = "allwinner,sun4i-a10-spdif", }, + { .compatible = "allwinner,sun6i-a31-spdif", }, { /* sentinel */ } }; MODULE_DEVICE_TABLE(of, sun4i_spdif_of_match); @@ -482,11 +485,23 @@ static int sun4i_spdif_probe(struct platform_device *pdev) } host->dma_params_tx.addr = res->start + SUN4I_SPDIF_TXFIFO; - host->dma_params_tx.maxburst = 4; + host->dma_params_tx.maxburst = 8; host->dma_params_tx.addr_width = DMA_SLAVE_BUSWIDTH_2_BYTES; platform_set_drvdata(pdev, host); + if (of_device_is_compatible(pdev->dev.of_node, + "allwinner,sun6i-a31-spdif")) { + host->rst = devm_reset_control_get_optional(&pdev->dev, NULL); + if (IS_ERR(host->rst) && PTR_ERR(host->rst) == -EPROBE_DEFER) { + ret = -EPROBE_DEFER; + dev_err(&pdev->dev, "Failed to get reset: %d\n", ret); + goto err_disable_apb_clk; + } + if (!IS_ERR(host->rst)) + reset_control_deassert(host->rst); + } + ret = devm_snd_soc_register_component(&pdev->dev, &sun4i_spdif_component, &sun4i_spdif_dai, 1); if (ret)