* [PATCH v1 1/3] dt-bindings: ingenic,aic: Remove unnecessary clocks from schema
2022-10-28 10:34 [PATCH v1 0/3] ASoC: jz4740-i2s: Remove .set_sysclk() & friends Aidan MacDonald
@ 2022-10-28 10:34 ` Aidan MacDonald
2022-10-30 11:56 ` Paul Cercueil
` (2 more replies)
2022-10-28 10:34 ` [PATCH v1 2/3] mips: dts: ingenic: Remove unnecessary AIC clocks Aidan MacDonald
` (2 subsequent siblings)
3 siblings, 3 replies; 9+ messages in thread
From: Aidan MacDonald @ 2022-10-28 10:34 UTC (permalink / raw)
To: paul, lgirdwood, broonie, robh+dt, krzysztof.kozlowski+dt,
tsbogend, perex, tiwai
Cc: alsa-devel, linux-mips, devicetree, linux-kernel
The AIC needs only the first two clocks: "aic" is a gate that's used
for gating the I2S controller when it's suspended, and "i2s" is the
system clock, from which the bit and frame clocks are derived. Both
clocks are therefore reasonably part of the AIC and should be passed
to the OS.
But the "ext" and "pll half" clocks are a little more questionable.
It appears these bindings were introduced when the schema was first
converted to YAML, but weren't present in the original .txt binding.
They are intended to be the possible parent clocks of "i2s".
The JZ4770 actually has three parents for its "i2s" clock, named
"ext", "pll0", and "pll1" in the Linux driver. The JZ4780 has two
parents but it doesn't have a "pll half" clock, instead it has an
"i2s_pll" clock which behaves much differently to the actual
"pll half" clock found on the JZ4740 & JZ4760. And there are other
Ingenic SoCs that share the JZ4780's clock layout, eg, the X1000.
Therefore, the bindings aren't really adequate for the JZ4770 and
a bit misleading for the JZ4780. Either we should fix the bindings,
or remove them entirely.
This patch opts to remove the bindings. There is a good case to be
made that "ext" and "pll half" don't belong here because they aren't
directly used by the AIC. They are only used to set the parent of
the "i2s" clock; they have no other effect on the AIC.
A good way to think of it is in terms of how the AIC constrains
clocks. The AIC can only generate the bit & frame clocks from the
system clock in certain ratios. Setting the sample rate effectively
constrains the frame clock, which, because of the clock dividers
controlled by the AIC, translates to constraints on the "i2s" clock.
Nothing in the AIC imposes a direct constraint on the parents of
the "i2s" clock, and the AIC does not need to enable or disable
the parents directly, so in principle the AIC doesn't need to be
aware of the parent clocks at all.
The choice of parent clock is still important, but the AIC doesn't
have enough information to apply such constraints itself. The sound
card does have that information because it knows how the AIC is
connected to other components. We need to use other DT mechanisms
to communicate those constraints at the sound card level, instead
of passing the clocks through to the AIC, and inventing ad-hoc ways
to plumb the constraints around behind the scenes.
Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@gmail.com>
---
.../devicetree/bindings/sound/ingenic,aic.yaml | 10 ++--------
1 file changed, 2 insertions(+), 8 deletions(-)
diff --git a/Documentation/devicetree/bindings/sound/ingenic,aic.yaml b/Documentation/devicetree/bindings/sound/ingenic,aic.yaml
index d607325f2f15..c4f9b3c2bde5 100644
--- a/Documentation/devicetree/bindings/sound/ingenic,aic.yaml
+++ b/Documentation/devicetree/bindings/sound/ingenic,aic.yaml
@@ -37,15 +37,11 @@ properties:
items:
- description: AIC clock
- description: I2S clock
- - description: EXT clock
- - description: PLL/2 clock
clock-names:
items:
- const: aic
- const: i2s
- - const: ext
- - const: pll half
dmas:
items:
@@ -82,10 +78,8 @@ examples:
interrupts = <18>;
clocks = <&cgu JZ4740_CLK_AIC>,
- <&cgu JZ4740_CLK_I2S>,
- <&cgu JZ4740_CLK_EXT>,
- <&cgu JZ4740_CLK_PLL_HALF>;
- clock-names = "aic", "i2s", "ext", "pll half";
+ <&cgu JZ4740_CLK_I2S>;
+ clock-names = "aic", "i2s";
dmas = <&dmac 25 0xffffffff>, <&dmac 24 0xffffffff>;
dma-names = "rx", "tx";
--
2.38.1
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH v1 1/3] dt-bindings: ingenic,aic: Remove unnecessary clocks from schema
2022-10-28 10:34 ` [PATCH v1 1/3] dt-bindings: ingenic,aic: Remove unnecessary clocks from schema Aidan MacDonald
@ 2022-10-30 11:56 ` Paul Cercueil
2022-10-31 12:15 ` Mark Brown
2022-10-31 18:46 ` Rob Herring
2 siblings, 0 replies; 9+ messages in thread
From: Paul Cercueil @ 2022-10-30 11:56 UTC (permalink / raw)
To: Aidan MacDonald
Cc: lgirdwood, broonie, robh+dt, krzysztof.kozlowski+dt, tsbogend,
perex, tiwai, alsa-devel, linux-mips, devicetree, linux-kernel
Hi,
Le ven. 28 oct. 2022 à 11:34:16 +0100, Aidan MacDonald
<aidanmacdonald.0x0@gmail.com> a écrit :
> The AIC needs only the first two clocks: "aic" is a gate that's used
> for gating the I2S controller when it's suspended, and "i2s" is the
> system clock, from which the bit and frame clocks are derived. Both
> clocks are therefore reasonably part of the AIC and should be passed
> to the OS.
>
> But the "ext" and "pll half" clocks are a little more questionable.
> It appears these bindings were introduced when the schema was first
> converted to YAML, but weren't present in the original .txt binding.
> They are intended to be the possible parent clocks of "i2s".
>
> The JZ4770 actually has three parents for its "i2s" clock, named
> "ext", "pll0", and "pll1" in the Linux driver. The JZ4780 has two
> parents but it doesn't have a "pll half" clock, instead it has an
> "i2s_pll" clock which behaves much differently to the actual
> "pll half" clock found on the JZ4740 & JZ4760. And there are other
> Ingenic SoCs that share the JZ4780's clock layout, eg, the X1000.
>
> Therefore, the bindings aren't really adequate for the JZ4770 and
> a bit misleading for the JZ4780. Either we should fix the bindings,
> or remove them entirely.
>
> This patch opts to remove the bindings. There is a good case to be
> made that "ext" and "pll half" don't belong here because they aren't
> directly used by the AIC. They are only used to set the parent of
> the "i2s" clock; they have no other effect on the AIC.
>
> A good way to think of it is in terms of how the AIC constrains
> clocks. The AIC can only generate the bit & frame clocks from the
> system clock in certain ratios. Setting the sample rate effectively
> constrains the frame clock, which, because of the clock dividers
> controlled by the AIC, translates to constraints on the "i2s" clock.
> Nothing in the AIC imposes a direct constraint on the parents of
> the "i2s" clock, and the AIC does not need to enable or disable
> the parents directly, so in principle the AIC doesn't need to be
> aware of the parent clocks at all.
>
> The choice of parent clock is still important, but the AIC doesn't
> have enough information to apply such constraints itself. The sound
> card does have that information because it knows how the AIC is
> connected to other components. We need to use other DT mechanisms
> to communicate those constraints at the sound card level, instead
> of passing the clocks through to the AIC, and inventing ad-hoc ways
> to plumb the constraints around behind the scenes.
>
> Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@gmail.com>
Yes, it makes sense also because from a DT point of view, these clocks
were redundant information. It's enough to know the i2s clock to also
know its parents.
Acked-by: Paul Cercueil <paul@crapouillou.net>
Cheers,
-Paul
> ---
> .../devicetree/bindings/sound/ingenic,aic.yaml | 10
> ++--------
> 1 file changed, 2 insertions(+), 8 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/sound/ingenic,aic.yaml
> b/Documentation/devicetree/bindings/sound/ingenic,aic.yaml
> index d607325f2f15..c4f9b3c2bde5 100644
> --- a/Documentation/devicetree/bindings/sound/ingenic,aic.yaml
> +++ b/Documentation/devicetree/bindings/sound/ingenic,aic.yaml
> @@ -37,15 +37,11 @@ properties:
> items:
> - description: AIC clock
> - description: I2S clock
> - - description: EXT clock
> - - description: PLL/2 clock
>
> clock-names:
> items:
> - const: aic
> - const: i2s
> - - const: ext
> - - const: pll half
>
> dmas:
> items:
> @@ -82,10 +78,8 @@ examples:
> interrupts = <18>;
>
> clocks = <&cgu JZ4740_CLK_AIC>,
> - <&cgu JZ4740_CLK_I2S>,
> - <&cgu JZ4740_CLK_EXT>,
> - <&cgu JZ4740_CLK_PLL_HALF>;
> - clock-names = "aic", "i2s", "ext", "pll half";
> + <&cgu JZ4740_CLK_I2S>;
> + clock-names = "aic", "i2s";
>
> dmas = <&dmac 25 0xffffffff>, <&dmac 24 0xffffffff>;
> dma-names = "rx", "tx";
> --
> 2.38.1
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v1 1/3] dt-bindings: ingenic,aic: Remove unnecessary clocks from schema
2022-10-28 10:34 ` [PATCH v1 1/3] dt-bindings: ingenic,aic: Remove unnecessary clocks from schema Aidan MacDonald
2022-10-30 11:56 ` Paul Cercueil
@ 2022-10-31 12:15 ` Mark Brown
2022-10-31 18:46 ` Rob Herring
2 siblings, 0 replies; 9+ messages in thread
From: Mark Brown @ 2022-10-31 12:15 UTC (permalink / raw)
To: Aidan MacDonald
Cc: paul, lgirdwood, robh+dt, krzysztof.kozlowski+dt, tsbogend, perex,
tiwai, alsa-devel, linux-mips, devicetree, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 691 bytes --]
On Fri, Oct 28, 2022 at 11:34:16AM +0100, Aidan MacDonald wrote:
> The AIC needs only the first two clocks: "aic" is a gate that's used
> for gating the I2S controller when it's suspended, and "i2s" is the
> system clock, from which the bit and frame clocks are derived. Both
> clocks are therefore reasonably part of the AIC and should be passed
> to the OS.
Please submit patches using subject lines reflecting the style for the
subsystem, this makes it easier for people to identify relevant patches.
Look at what existing commits in the area you're changing are doing and
make sure your subject lines visually resemble what they're doing.
There's no need to resubmit to fix this alone.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v1 1/3] dt-bindings: ingenic,aic: Remove unnecessary clocks from schema
2022-10-28 10:34 ` [PATCH v1 1/3] dt-bindings: ingenic,aic: Remove unnecessary clocks from schema Aidan MacDonald
2022-10-30 11:56 ` Paul Cercueil
2022-10-31 12:15 ` Mark Brown
@ 2022-10-31 18:46 ` Rob Herring
2 siblings, 0 replies; 9+ messages in thread
From: Rob Herring @ 2022-10-31 18:46 UTC (permalink / raw)
To: Aidan MacDonald
Cc: krzysztof.kozlowski+dt, alsa-devel, perex, paul, robh+dt, broonie,
tsbogend, linux-mips, lgirdwood, linux-kernel, tiwai, devicetree
On Fri, 28 Oct 2022 11:34:16 +0100, Aidan MacDonald wrote:
> The AIC needs only the first two clocks: "aic" is a gate that's used
> for gating the I2S controller when it's suspended, and "i2s" is the
> system clock, from which the bit and frame clocks are derived. Both
> clocks are therefore reasonably part of the AIC and should be passed
> to the OS.
>
> But the "ext" and "pll half" clocks are a little more questionable.
> It appears these bindings were introduced when the schema was first
> converted to YAML, but weren't present in the original .txt binding.
> They are intended to be the possible parent clocks of "i2s".
>
> The JZ4770 actually has three parents for its "i2s" clock, named
> "ext", "pll0", and "pll1" in the Linux driver. The JZ4780 has two
> parents but it doesn't have a "pll half" clock, instead it has an
> "i2s_pll" clock which behaves much differently to the actual
> "pll half" clock found on the JZ4740 & JZ4760. And there are other
> Ingenic SoCs that share the JZ4780's clock layout, eg, the X1000.
>
> Therefore, the bindings aren't really adequate for the JZ4770 and
> a bit misleading for the JZ4780. Either we should fix the bindings,
> or remove them entirely.
>
> This patch opts to remove the bindings. There is a good case to be
> made that "ext" and "pll half" don't belong here because they aren't
> directly used by the AIC. They are only used to set the parent of
> the "i2s" clock; they have no other effect on the AIC.
>
> A good way to think of it is in terms of how the AIC constrains
> clocks. The AIC can only generate the bit & frame clocks from the
> system clock in certain ratios. Setting the sample rate effectively
> constrains the frame clock, which, because of the clock dividers
> controlled by the AIC, translates to constraints on the "i2s" clock.
> Nothing in the AIC imposes a direct constraint on the parents of
> the "i2s" clock, and the AIC does not need to enable or disable
> the parents directly, so in principle the AIC doesn't need to be
> aware of the parent clocks at all.
>
> The choice of parent clock is still important, but the AIC doesn't
> have enough information to apply such constraints itself. The sound
> card does have that information because it knows how the AIC is
> connected to other components. We need to use other DT mechanisms
> to communicate those constraints at the sound card level, instead
> of passing the clocks through to the AIC, and inventing ad-hoc ways
> to plumb the constraints around behind the scenes.
>
> Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@gmail.com>
> ---
> .../devicetree/bindings/sound/ingenic,aic.yaml | 10 ++--------
> 1 file changed, 2 insertions(+), 8 deletions(-)
>
Acked-by: Rob Herring <robh@kernel.org>
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v1 2/3] mips: dts: ingenic: Remove unnecessary AIC clocks
2022-10-28 10:34 [PATCH v1 0/3] ASoC: jz4740-i2s: Remove .set_sysclk() & friends Aidan MacDonald
2022-10-28 10:34 ` [PATCH v1 1/3] dt-bindings: ingenic,aic: Remove unnecessary clocks from schema Aidan MacDonald
@ 2022-10-28 10:34 ` Aidan MacDonald
2022-10-28 10:34 ` [PATCH v1 3/3] ASoC: jz4740-i2s: Remove .set_sysclk() Aidan MacDonald
2022-10-31 18:59 ` (subset) [PATCH v1 0/3] ASoC: jz4740-i2s: Remove .set_sysclk() & friends Mark Brown
3 siblings, 0 replies; 9+ messages in thread
From: Aidan MacDonald @ 2022-10-28 10:34 UTC (permalink / raw)
To: paul, lgirdwood, broonie, robh+dt, krzysztof.kozlowski+dt,
tsbogend, perex, tiwai
Cc: alsa-devel, linux-mips, devicetree, linux-kernel
These clocks arguably don't belong in the DT because there isn't
much the AIC can do with them in principle. It's safe to remove
them because the jz4740-i2s Linux driver has never depended on
them in the DT, despite superficial appearances to the contrary.
Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@gmail.com>
---
arch/mips/boot/dts/ingenic/jz4725b.dtsi | 7 ++-----
arch/mips/boot/dts/ingenic/jz4740.dtsi | 7 ++-----
arch/mips/boot/dts/ingenic/jz4770.dtsi | 5 ++---
3 files changed, 6 insertions(+), 13 deletions(-)
diff --git a/arch/mips/boot/dts/ingenic/jz4725b.dtsi b/arch/mips/boot/dts/ingenic/jz4725b.dtsi
index e9e48022f631..acbbe8c4664c 100644
--- a/arch/mips/boot/dts/ingenic/jz4725b.dtsi
+++ b/arch/mips/boot/dts/ingenic/jz4725b.dtsi
@@ -198,11 +198,8 @@ aic: audio-controller@10020000 {
#sound-dai-cells = <0>;
- clocks = <&cgu JZ4725B_CLK_AIC>,
- <&cgu JZ4725B_CLK_I2S>,
- <&cgu JZ4725B_CLK_EXT>,
- <&cgu JZ4725B_CLK_PLL_HALF>;
- clock-names = "aic", "i2s", "ext", "pll half";
+ clocks = <&cgu JZ4725B_CLK_AIC>, <&cgu JZ4725B_CLK_I2S>;
+ clock-names = "aic", "i2s";
interrupt-parent = <&intc>;
interrupts = <10>;
diff --git a/arch/mips/boot/dts/ingenic/jz4740.dtsi b/arch/mips/boot/dts/ingenic/jz4740.dtsi
index 7f76cba03a08..bdd6f4d82ec9 100644
--- a/arch/mips/boot/dts/ingenic/jz4740.dtsi
+++ b/arch/mips/boot/dts/ingenic/jz4740.dtsi
@@ -192,11 +192,8 @@ aic: audio-controller@10020000 {
interrupt-parent = <&intc>;
interrupts = <18>;
- clocks = <&cgu JZ4740_CLK_AIC>,
- <&cgu JZ4740_CLK_I2S>,
- <&cgu JZ4740_CLK_EXT>,
- <&cgu JZ4740_CLK_PLL_HALF>;
- clock-names = "aic", "i2s", "ext", "pll half";
+ clocks = <&cgu JZ4740_CLK_AIC>, <&cgu JZ4740_CLK_I2S>;
+ clock-names = "aic", "i2s";
dmas = <&dmac 25 0xffffffff>, <&dmac 24 0xffffffff>;
dma-names = "rx", "tx";
diff --git a/arch/mips/boot/dts/ingenic/jz4770.dtsi b/arch/mips/boot/dts/ingenic/jz4770.dtsi
index bda0a3a86ed5..9c0099919db7 100644
--- a/arch/mips/boot/dts/ingenic/jz4770.dtsi
+++ b/arch/mips/boot/dts/ingenic/jz4770.dtsi
@@ -238,9 +238,8 @@ aic: audio-controller@10020000 {
#sound-dai-cells = <0>;
- clocks = <&cgu JZ4770_CLK_AIC>, <&cgu JZ4770_CLK_I2S>,
- <&cgu JZ4770_CLK_EXT>, <&cgu JZ4770_CLK_PLL0>;
- clock-names = "aic", "i2s", "ext", "pll half";
+ clocks = <&cgu JZ4770_CLK_AIC>, <&cgu JZ4770_CLK_I2S>;
+ clock-names = "aic", "i2s";
interrupt-parent = <&intc>;
interrupts = <34>;
--
2.38.1
^ permalink raw reply related [flat|nested] 9+ messages in thread* [PATCH v1 3/3] ASoC: jz4740-i2s: Remove .set_sysclk()
2022-10-28 10:34 [PATCH v1 0/3] ASoC: jz4740-i2s: Remove .set_sysclk() & friends Aidan MacDonald
2022-10-28 10:34 ` [PATCH v1 1/3] dt-bindings: ingenic,aic: Remove unnecessary clocks from schema Aidan MacDonald
2022-10-28 10:34 ` [PATCH v1 2/3] mips: dts: ingenic: Remove unnecessary AIC clocks Aidan MacDonald
@ 2022-10-28 10:34 ` Aidan MacDonald
2022-10-30 11:58 ` Paul Cercueil
2022-10-31 18:59 ` (subset) [PATCH v1 0/3] ASoC: jz4740-i2s: Remove .set_sysclk() & friends Mark Brown
3 siblings, 1 reply; 9+ messages in thread
From: Aidan MacDonald @ 2022-10-28 10:34 UTC (permalink / raw)
To: paul, lgirdwood, broonie, robh+dt, krzysztof.kozlowski+dt,
tsbogend, perex, tiwai
Cc: alsa-devel, linux-mips, devicetree, linux-kernel
.set_sysclk() is effectively unused here. No machine drivers use
jz4740-i2s; and JZ4740_I2S_CLKSRC_EXT is the only selectable clock
source with simple-card, but that is also the default source and
has a fixed frequency, so configuring it would be redundant.
simple-card ignores -ENOTSUPP error codes when setting the sysclock,
so any device trees that do set the sysclock for some reason should
still work.
It's still possible to configure the clock parent manually in the
device tree and control frequency using other simple-card options,
so at the end of the day there's no real loss in functionality.
Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@gmail.com>
---
Meant to be applied on top of jz4740-i2s cleanups series already
in linux-next.
Link: https://lore.kernel.org/alsa-devel/20221023143328.160866-1-aidanmacdonald.0x0@gmail.com/
sound/soc/jz4740/jz4740-i2s.c | 32 --------------------------------
sound/soc/jz4740/jz4740-i2s.h | 10 ----------
2 files changed, 42 deletions(-)
delete mode 100644 sound/soc/jz4740/jz4740-i2s.h
diff --git a/sound/soc/jz4740/jz4740-i2s.c b/sound/soc/jz4740/jz4740-i2s.c
index b620d4462d90..6d9cfe0a5041 100644
--- a/sound/soc/jz4740/jz4740-i2s.c
+++ b/sound/soc/jz4740/jz4740-i2s.c
@@ -23,8 +23,6 @@
#include <sound/initval.h>
#include <sound/dmaengine_pcm.h>
-#include "jz4740-i2s.h"
-
#define JZ_REG_AIC_CONF 0x00
#define JZ_REG_AIC_CTRL 0x04
#define JZ_REG_AIC_I2S_FMT 0x10
@@ -273,35 +271,6 @@ static int jz4740_i2s_hw_params(struct snd_pcm_substream *substream,
return 0;
}
-static int jz4740_i2s_set_sysclk(struct snd_soc_dai *dai, int clk_id,
- unsigned int freq, int dir)
-{
- struct jz4740_i2s *i2s = snd_soc_dai_get_drvdata(dai);
- struct clk *parent;
- int ret = 0;
-
- switch (clk_id) {
- case JZ4740_I2S_CLKSRC_EXT:
- parent = clk_get(NULL, "ext");
- if (IS_ERR(parent))
- return PTR_ERR(parent);
- clk_set_parent(i2s->clk_i2s, parent);
- break;
- case JZ4740_I2S_CLKSRC_PLL:
- parent = clk_get(NULL, "pll half");
- if (IS_ERR(parent))
- return PTR_ERR(parent);
- clk_set_parent(i2s->clk_i2s, parent);
- ret = clk_set_rate(i2s->clk_i2s, freq);
- break;
- default:
- return -EINVAL;
- }
- clk_put(parent);
-
- return ret;
-}
-
static int jz4740_i2s_dai_probe(struct snd_soc_dai *dai)
{
struct jz4740_i2s *i2s = snd_soc_dai_get_drvdata(dai);
@@ -318,7 +287,6 @@ static const struct snd_soc_dai_ops jz4740_i2s_dai_ops = {
.trigger = jz4740_i2s_trigger,
.hw_params = jz4740_i2s_hw_params,
.set_fmt = jz4740_i2s_set_fmt,
- .set_sysclk = jz4740_i2s_set_sysclk,
};
#define JZ4740_I2S_FMTS (SNDRV_PCM_FMTBIT_S8 | \
diff --git a/sound/soc/jz4740/jz4740-i2s.h b/sound/soc/jz4740/jz4740-i2s.h
deleted file mode 100644
index 4da14eac1145..000000000000
--- a/sound/soc/jz4740/jz4740-i2s.h
+++ /dev/null
@@ -1,10 +0,0 @@
-/* SPDX-License-Identifier: GPL-2.0-only */
-
-#ifndef _JZ4740_I2S_H
-#define _JZ4740_I2S_H
-
-/* I2S clock source */
-#define JZ4740_I2S_CLKSRC_EXT 0
-#define JZ4740_I2S_CLKSRC_PLL 1
-
-#endif
--
2.38.1
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH v1 3/3] ASoC: jz4740-i2s: Remove .set_sysclk()
2022-10-28 10:34 ` [PATCH v1 3/3] ASoC: jz4740-i2s: Remove .set_sysclk() Aidan MacDonald
@ 2022-10-30 11:58 ` Paul Cercueil
0 siblings, 0 replies; 9+ messages in thread
From: Paul Cercueil @ 2022-10-30 11:58 UTC (permalink / raw)
To: Aidan MacDonald
Cc: lgirdwood, broonie, robh+dt, krzysztof.kozlowski+dt, tsbogend,
perex, tiwai, alsa-devel, linux-mips, devicetree, linux-kernel
Hi Aidan,
Le ven. 28 oct. 2022 à 11:34:18 +0100, Aidan MacDonald
<aidanmacdonald.0x0@gmail.com> a écrit :
> .set_sysclk() is effectively unused here. No machine drivers use
> jz4740-i2s; and JZ4740_I2S_CLKSRC_EXT is the only selectable clock
> source with simple-card, but that is also the default source and
> has a fixed frequency, so configuring it would be redundant.
>
> simple-card ignores -ENOTSUPP error codes when setting the sysclock,
> so any device trees that do set the sysclock for some reason should
> still work.
>
> It's still possible to configure the clock parent manually in the
> device tree and control frequency using other simple-card options,
> so at the end of the day there's no real loss in functionality.
>
> Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@gmail.com>
Reviewed-by: Paul Cercueil <paul@crapouillou.net>
Cheers,
-Paul
> ---
> Meant to be applied on top of jz4740-i2s cleanups series already
> in linux-next.
> Link:
> https://lore.kernel.org/alsa-devel/20221023143328.160866-1-aidanmacdonald.0x0@gmail.com/
>
> sound/soc/jz4740/jz4740-i2s.c | 32 --------------------------------
> sound/soc/jz4740/jz4740-i2s.h | 10 ----------
> 2 files changed, 42 deletions(-)
> delete mode 100644 sound/soc/jz4740/jz4740-i2s.h
>
> diff --git a/sound/soc/jz4740/jz4740-i2s.c
> b/sound/soc/jz4740/jz4740-i2s.c
> index b620d4462d90..6d9cfe0a5041 100644
> --- a/sound/soc/jz4740/jz4740-i2s.c
> +++ b/sound/soc/jz4740/jz4740-i2s.c
> @@ -23,8 +23,6 @@
> #include <sound/initval.h>
> #include <sound/dmaengine_pcm.h>
>
> -#include "jz4740-i2s.h"
> -
> #define JZ_REG_AIC_CONF 0x00
> #define JZ_REG_AIC_CTRL 0x04
> #define JZ_REG_AIC_I2S_FMT 0x10
> @@ -273,35 +271,6 @@ static int jz4740_i2s_hw_params(struct
> snd_pcm_substream *substream,
> return 0;
> }
>
> -static int jz4740_i2s_set_sysclk(struct snd_soc_dai *dai, int clk_id,
> - unsigned int freq, int dir)
> -{
> - struct jz4740_i2s *i2s = snd_soc_dai_get_drvdata(dai);
> - struct clk *parent;
> - int ret = 0;
> -
> - switch (clk_id) {
> - case JZ4740_I2S_CLKSRC_EXT:
> - parent = clk_get(NULL, "ext");
> - if (IS_ERR(parent))
> - return PTR_ERR(parent);
> - clk_set_parent(i2s->clk_i2s, parent);
> - break;
> - case JZ4740_I2S_CLKSRC_PLL:
> - parent = clk_get(NULL, "pll half");
> - if (IS_ERR(parent))
> - return PTR_ERR(parent);
> - clk_set_parent(i2s->clk_i2s, parent);
> - ret = clk_set_rate(i2s->clk_i2s, freq);
> - break;
> - default:
> - return -EINVAL;
> - }
> - clk_put(parent);
> -
> - return ret;
> -}
> -
> static int jz4740_i2s_dai_probe(struct snd_soc_dai *dai)
> {
> struct jz4740_i2s *i2s = snd_soc_dai_get_drvdata(dai);
> @@ -318,7 +287,6 @@ static const struct snd_soc_dai_ops
> jz4740_i2s_dai_ops = {
> .trigger = jz4740_i2s_trigger,
> .hw_params = jz4740_i2s_hw_params,
> .set_fmt = jz4740_i2s_set_fmt,
> - .set_sysclk = jz4740_i2s_set_sysclk,
> };
>
> #define JZ4740_I2S_FMTS (SNDRV_PCM_FMTBIT_S8 | \
> diff --git a/sound/soc/jz4740/jz4740-i2s.h
> b/sound/soc/jz4740/jz4740-i2s.h
> deleted file mode 100644
> index 4da14eac1145..000000000000
> --- a/sound/soc/jz4740/jz4740-i2s.h
> +++ /dev/null
> @@ -1,10 +0,0 @@
> -/* SPDX-License-Identifier: GPL-2.0-only */
> -
> -#ifndef _JZ4740_I2S_H
> -#define _JZ4740_I2S_H
> -
> -/* I2S clock source */
> -#define JZ4740_I2S_CLKSRC_EXT 0
> -#define JZ4740_I2S_CLKSRC_PLL 1
> -
> -#endif
> --
> 2.38.1
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: (subset) [PATCH v1 0/3] ASoC: jz4740-i2s: Remove .set_sysclk() & friends
2022-10-28 10:34 [PATCH v1 0/3] ASoC: jz4740-i2s: Remove .set_sysclk() & friends Aidan MacDonald
` (2 preceding siblings ...)
2022-10-28 10:34 ` [PATCH v1 3/3] ASoC: jz4740-i2s: Remove .set_sysclk() Aidan MacDonald
@ 2022-10-31 18:59 ` Mark Brown
3 siblings, 0 replies; 9+ messages in thread
From: Mark Brown @ 2022-10-31 18:59 UTC (permalink / raw)
To: tsbogend, paul, lgirdwood, krzysztof.kozlowski+dt, perex, tiwai,
Aidan MacDonald, robh+dt
Cc: alsa-devel, linux-mips, devicetree, linux-kernel
On Fri, 28 Oct 2022 11:34:15 +0100, Aidan MacDonald wrote:
> A quick series to get rid of .set_sysclk() from jz4740-i2s.
> It wasn't used in practice so this shouldn't be troublesome for anyone,
> and fortunately there aren't any backward compatibility concerns.
>
> The actual rationale for removing it, as opposed to fixing the
> issues of the current DT bindings and implementation, is provided
> in the dt-bindings patch.
>
> [...]
Applied to
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next
Thanks!
[1/3] dt-bindings: ingenic,aic: Remove unnecessary clocks from schema
commit: fc839054615427aa15de7677082b23b3033faf07
[3/3] ASoC: jz4740-i2s: Remove .set_sysclk()
commit: 1c0036e03edd5d97fc0af94dd3ab7e8c58b8191d
All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.
You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.
If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.
Please add any relevant lists and maintainers to the CCs when replying
to this mail.
Thanks,
Mark
^ permalink raw reply [flat|nested] 9+ messages in thread