The Mediatek AFE PCM controller for MT8192 allows sharing of an I2S bus
between two busses. Add a pattern for these properties in the
dt-binding.
Signed-off-by: Nícolas F. R. A. Prado <[email protected]>
---
Documentation/devicetree/bindings/sound/mt8192-afe-pcm.yaml | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/Documentation/devicetree/bindings/sound/mt8192-afe-pcm.yaml b/Documentation/devicetree/bindings/sound/mt8192-afe-pcm.yaml
index 7a25bc9b8060..5b03c8dbf318 100644
--- a/Documentation/devicetree/bindings/sound/mt8192-afe-pcm.yaml
+++ b/Documentation/devicetree/bindings/sound/mt8192-afe-pcm.yaml
@@ -54,6 +54,11 @@ properties:
- const: aud_infra_clk
- const: aud_infra_26m_clk
+patternProperties:
+ "^i2s[0-35-9]-share$":
+ description: Name of the I2S bus that is shared with this bus
+ pattern: "^I2S[0-35-9]$"
+
required:
- compatible
- interrupts
--
2.36.0
On Fri, Apr 29, 2022 at 04:30:37PM -0400, N?colas F. R. A. Prado wrote:
> The Mediatek AFE PCM controller for MT8192 allows sharing of an I2S bus
> between two busses. Add a pattern for these properties in the
> dt-binding.
>
> Signed-off-by: N?colas F. R. A. Prado <[email protected]>
>
> ---
>
> Documentation/devicetree/bindings/sound/mt8192-afe-pcm.yaml | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/sound/mt8192-afe-pcm.yaml b/Documentation/devicetree/bindings/sound/mt8192-afe-pcm.yaml
> index 7a25bc9b8060..5b03c8dbf318 100644
> --- a/Documentation/devicetree/bindings/sound/mt8192-afe-pcm.yaml
> +++ b/Documentation/devicetree/bindings/sound/mt8192-afe-pcm.yaml
> @@ -54,6 +54,11 @@ properties:
> - const: aud_infra_clk
> - const: aud_infra_26m_clk
>
> +patternProperties:
> + "^i2s[0-35-9]-share$":
> + description: Name of the I2S bus that is shared with this bus
> + pattern: "^I2S[0-35-9]$"
Why not a phandle to the the other bus? That would be the DT way to do
it. But I'm not sure I really understand who is sharing what here.
> +
> required:
> - compatible
> - interrupts
> --
> 2.36.0
>
>
Il 29/04/22 22:30, Nícolas F. R. A. Prado ha scritto:
> The Mediatek AFE PCM controller for MT8192 allows sharing of an I2S bus
> between two busses. Add a pattern for these properties in the
> dt-binding.
>
> Signed-off-by: Nícolas F. R. A. Prado <[email protected]>
> ---
>
> Documentation/devicetree/bindings/sound/mt8192-afe-pcm.yaml | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/sound/mt8192-afe-pcm.yaml b/Documentation/devicetree/bindings/sound/mt8192-afe-pcm.yaml
> index 7a25bc9b8060..5b03c8dbf318 100644
> --- a/Documentation/devicetree/bindings/sound/mt8192-afe-pcm.yaml
> +++ b/Documentation/devicetree/bindings/sound/mt8192-afe-pcm.yaml
> @@ -54,6 +54,11 @@ properties:
> - const: aud_infra_clk
> - const: aud_infra_26m_clk
>
> +patternProperties:
> + "^i2s[0-35-9]-share$":
> + description: Name of the I2S bus that is shared with this bus
> + pattern: "^I2S[0-35-9]$"
> +
> required:
> - compatible
> - interrupts
>
The only other way of doing this would be to complicate this in the driver
so that we can do something like
"i2s-share = <0 2>"; instead of i2s0-share = "I2S2";
...and I don't think that this would be any more straightforward than the
provided way.
There's an improvement that we can do to that pattern description though,
which would be explaining that declaring 'i2s0-share = "I2S2"' means that
I2S2's data pin will be used as DATA-OUT, while i2s0 is DATA-IN.
Another thing that comes to mind here is that this is a MediaTek specific
property and *not* a generic one, which means that both the driver and
this binding should be fixed to get a "mediatek," prefix, so, this property
should - in reality - be "mediatek,i2s[0-35-9]-share" instead.
I think that everyone agrees about that, but let's see what the others say.
Cheers,
Angelo
Il 05/05/22 10:48, Jiaxin Yu ha scritto:
> On Thu, 2022-05-05 at 10:08 +0200, AngeloGioacchino Del Regno wrote:
>> Il 29/04/22 22:30, Nícolas F. R. A. Prado ha scritto:
>>> The Mediatek AFE PCM controller for MT8192 allows sharing of an I2S
>>> bus
>>> between two busses. Add a pattern for these properties in the
>>> dt-binding.
>>>
>>> Signed-off-by: Nícolas F. R. A. Prado <[email protected]>
>>> ---
>>>
>>> Documentation/devicetree/bindings/sound/mt8192-afe-pcm.yaml | 5
>>> +++++
>>> 1 file changed, 5 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/sound/mt8192-afe-
>>> pcm.yaml b/Documentation/devicetree/bindings/sound/mt8192-afe-
>>> pcm.yaml
>>> index 7a25bc9b8060..5b03c8dbf318 100644
>>> --- a/Documentation/devicetree/bindings/sound/mt8192-afe-pcm.yaml
>>> +++ b/Documentation/devicetree/bindings/sound/mt8192-afe-pcm.yaml
>>> @@ -54,6 +54,11 @@ properties:
>>> - const: aud_infra_clk
>>> - const: aud_infra_26m_clk
>>>
>>> +patternProperties:
>>> + "^i2s[0-35-9]-share$":
>>> + description: Name of the I2S bus that is shared with this bus
>>> + pattern: "^I2S[0-35-9]$"
>>> +
>>> required:
>>> - compatible
>>> - interrupts
>>>
>>
>> The only other way of doing this would be to complicate this in the
>> driver
>> so that we can do something like
>>
>> "i2s-share = <0 2>"; instead of i2s0-share = "I2S2";
>>
>> ...and I don't think that this would be any more straightforward than
>> the
>> provided way.
>>
>> There's an improvement that we can do to that pattern description
>> though,
>> which would be explaining that declaring 'i2s0-share = "I2S2"' means
>> that
>> I2S2's data pin will be used as DATA-OUT, while i2s0 is DATA-IN.
>>
>> Another thing that comes to mind here is that this is a MediaTek
>> specific
>> property and *not* a generic one, which means that both the driver
>> and
>> this binding should be fixed to get a "mediatek," prefix, so, this
>> property
>> should - in reality - be "mediatek,i2s[0-35-9]-share" instead.
>>
>> I think that everyone agrees about that, but let's see what the
>> others say.
>>
>> Cheers,
>> Angelo
>
> Hi Angelo,
>
> 'i2s0-share = "I2S2"' means that if we want use I2S0, there need open
> I2S2 to provide clock. Conversely, if we want to use I2S2, we don't
> need to open I2S0. However, MediaTek I2S0 and I2S2 hardware are
> generally designed as input. So usually we use 'i2s0-share = "I2S1"'.
> Even numbers represent input, odd numbers represent output.
>
> Yes, I think adding the "mediatek," prefix is the right way to define a
> non-generic property.
>
Hello Jiaxin,
if I get this correctly, i2s0-share = "I2S2" would be *invalid*... as you
just explained, i2sX, where:
X = even number -> always DATA IN
X = odd number -> always DATA OUT
...this means that the dt-binding needs a pattern to specify that only odd
can be assigned to only even.
Nicolas, take note! :-) :-)
Thanks,
Angelo
On Thu, May 05, 2022 at 10:52:45AM +0200, AngeloGioacchino Del Regno wrote:
> Il 05/05/22 10:48, Jiaxin Yu ha scritto:
> > On Thu, 2022-05-05 at 10:08 +0200, AngeloGioacchino Del Regno wrote:
> > > Il 29/04/22 22:30, N?colas F. R. A. Prado ha scritto:
> > > > The Mediatek AFE PCM controller for MT8192 allows sharing of an I2S
> > > > bus
> > > > between two busses. Add a pattern for these properties in the
> > > > dt-binding.
> > > >
> > > > Signed-off-by: N?colas F. R. A. Prado <[email protected]>
> > > > ---
> > > >
> > > > Documentation/devicetree/bindings/sound/mt8192-afe-pcm.yaml | 5
> > > > +++++
> > > > 1 file changed, 5 insertions(+)
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/sound/mt8192-afe-
> > > > pcm.yaml b/Documentation/devicetree/bindings/sound/mt8192-afe-
> > > > pcm.yaml
> > > > index 7a25bc9b8060..5b03c8dbf318 100644
> > > > --- a/Documentation/devicetree/bindings/sound/mt8192-afe-pcm.yaml
> > > > +++ b/Documentation/devicetree/bindings/sound/mt8192-afe-pcm.yaml
> > > > @@ -54,6 +54,11 @@ properties:
> > > > - const: aud_infra_clk
> > > > - const: aud_infra_26m_clk
> > > > +patternProperties:
> > > > + "^i2s[0-35-9]-share$":
> > > > + description: Name of the I2S bus that is shared with this bus
> > > > + pattern: "^I2S[0-35-9]$"
> > > > +
> > > > required:
> > > > - compatible
> > > > - interrupts
> > > >
> > >
> > > The only other way of doing this would be to complicate this in the
> > > driver
> > > so that we can do something like
> > >
> > > "i2s-share = <0 2>"; instead of i2s0-share = "I2S2";
> > >
> > > ...and I don't think that this would be any more straightforward than
> > > the
> > > provided way.
> > >
> > > There's an improvement that we can do to that pattern description
> > > though,
> > > which would be explaining that declaring 'i2s0-share = "I2S2"' means
> > > that
> > > I2S2's data pin will be used as DATA-OUT, while i2s0 is DATA-IN.
> > >
> > > Another thing that comes to mind here is that this is a MediaTek
> > > specific
> > > property and *not* a generic one, which means that both the driver
> > > and
> > > this binding should be fixed to get a "mediatek," prefix, so, this
> > > property
> > > should - in reality - be "mediatek,i2s[0-35-9]-share" instead.
> > >
> > > I think that everyone agrees about that, but let's see what the
> > > others say.
> > >
> > > Cheers,
> > > Angelo
> >
> > Hi Angelo,
> >
> > 'i2s0-share = "I2S2"' means that if we want use I2S0, there need open
> > I2S2 to provide clock. Conversely, if we want to use I2S2, we don't
> > need to open I2S0. However, MediaTek I2S0 and I2S2 hardware are
> > generally designed as input. So usually we use 'i2s0-share = "I2S1"'.
> > Even numbers represent input, odd numbers represent output.
> >
> > Yes, I think adding the "mediatek," prefix is the right way to define a
> > non-generic property.
> >
Hi Jiaxin,
thank you for the insights.
>
> Hello Jiaxin,
>
> if I get this correctly, i2s0-share = "I2S2" would be *invalid*... as you
> just explained, i2sX, where:
>
> X = even number -> always DATA IN
> X = odd number -> always DATA OUT
>
> ...this means that the dt-binding needs a pattern to specify that only odd
> can be assigned to only even.
So, the situation seems different at least on mt8192-asurada-spherion.
Here, I2S8 is used for the headset microphone and I2S9 for the headset audio.
Even for input and odd for output agree with Jiaxin's description. However, the
input bus seems to be the main one, that is, disabling I2S8:
amixer cset name='UL2_CH1 I2S8_CH1' 0
amixer cset name='UL2_CH2 I2S8_CH2' 0
not only disables the microphone but also the audio on the headset. If I add
i2s9-share = "I2S8";
on the DT, then everything works, I can disable I2S8 without impacting the
headset audio. So the pattern for the property on this platform is the opposite
that Jiaxin mentioned. This tells me that we should keep the binding more
generic (not assume where odds and evens go). I will still apply the other
suggestions mentioned though.
Thanks,
N?colas
>
> Nicolas, take note! :-) :-)
>
> Thanks,
> Angelo
>
> --
> To unsubscribe, send mail to [email protected].
On Fri, May 06, 2022 at 01:45:24PM +0800, Jiaxin Yu wrote:
> On Thu, 2022-05-05 at 12:25 -0400, N?colas F. R. A. Prado wrote:
> > >
>
> > On Thu, May 05, 2022 at 10:52:45AM +0200, AngeloGioacchino Del Regno
> > wrote:
> > > Il 05/05/22 10:48, Jiaxin Yu ha scritto:
> > > > On Thu, 2022-05-05 at 10:08 +0200, AngeloGioacchino Del Regno
> > > > wrote:
> > > > > Il 29/04/22 22:30, N?colas F. R. A. Prado ha scritto:
> > > > > > The Mediatek AFE PCM controller for MT8192 allows sharing of
> > > > > > an I2S
> > > > > > bus
> > > > > > between two busses. Add a pattern for these properties in the
> > > > > > dt-binding.
> > > > > >
> > > > > > Signed-off-by: N?colas F. R. A. Prado <
> > > > > > [email protected]>
> > > > > > ---
> > > > > >
> > > > > > Documentation/devicetree/bindings/sound/mt8192-afe-
> > > > > > pcm.yaml | 5
> > > > > > +++++
> > > > > > 1 file changed, 5 insertions(+)
> > > > > >
> > > > > > diff --git a/Documentation/devicetree/bindings/sound/mt8192-
> > > > > > afe-
> > > > > > pcm.yaml b/Documentation/devicetree/bindings/sound/mt8192-
> > > > > > afe-
> > > > > > pcm.yaml
> > > > > > index 7a25bc9b8060..5b03c8dbf318 100644
> > > > > > --- a/Documentation/devicetree/bindings/sound/mt8192-afe-
> > > > > > pcm.yaml
> > > > > > +++ b/Documentation/devicetree/bindings/sound/mt8192-afe-
> > > > > > pcm.yaml
> > > > > > @@ -54,6 +54,11 @@ properties:
> > > > > > - const: aud_infra_clk
> > > > > > - const: aud_infra_26m_clk
> > > > > > +patternProperties:
> > > > > > + "^i2s[0-35-9]-share$":
> > > > > > + description: Name of the I2S bus that is shared with
> > > > > > this bus
> > > > > > + pattern: "^I2S[0-35-9]$"
> > > > > > +
> > > > > > required:
> > > > > > - compatible
> > > > > > - interrupts
> > > > > >
> > > > >
> > > > > The only other way of doing this would be to complicate this in
> > > > > the
> > > > > driver
> > > > > so that we can do something like
> > > > >
> > > > > "i2s-share = <0 2>"; instead of i2s0-share = "I2S2";
> > > > >
> > > > > ...and I don't think that this would be any more
> > > > > straightforward than
> > > > > the
> > > > > provided way.
> > > > >
> > > > > There's an improvement that we can do to that pattern
> > > > > description
> > > > > though,
> > > > > which would be explaining that declaring 'i2s0-share = "I2S2"'
> > > > > means
> > > > > that
> > > > > I2S2's data pin will be used as DATA-OUT, while i2s0 is DATA-
> > > > > IN.
> > > > >
> > > > > Another thing that comes to mind here is that this is a
> > > > > MediaTek
> > > > > specific
> > > > > property and *not* a generic one, which means that both the
> > > > > driver
> > > > > and
> > > > > this binding should be fixed to get a "mediatek," prefix, so,
> > > > > this
> > > > > property
> > > > > should - in reality - be "mediatek,i2s[0-35-9]-share" instead.
> > > > >
> > > > > I think that everyone agrees about that, but let's see what the
> > > > > others say.
> > > > >
> > > > > Cheers,
> > > > > Angelo
> > > >
> > > > Hi Angelo,
> > > >
> > > > 'i2s0-share = "I2S2"' means that if we want use I2S0, there need
> > > > open
> > > > I2S2 to provide clock. Conversely, if we want to use I2S2, we
> > > > don't
> > > > need to open I2S0. However, MediaTek I2S0 and I2S2 hardware are
> > > > generally designed as input. So usually we use 'i2s0-share =
> > > > "I2S1"'.
> > > > Even numbers represent input, odd numbers represent output.
> > > >
> > > > Yes, I think adding the "mediatek," prefix is the right way to
> > > > define a
> > > > non-generic property.
> > > >
> >
> > Hi Jiaxin,
> >
> > thank you for the insights.
> >
> > >
> > > Hello Jiaxin,
> > >
> > > if I get this correctly, i2s0-share = "I2S2" would be *invalid*...
> > > as you
> > > just explained, i2sX, where:
> > >
> > > X = even number -> always DATA IN
> > > X = odd number -> always DATA OUT
> > >
> > > ...this means that the dt-binding needs a pattern to specify that
> > > only odd
> > > can be assigned to only even.
> >
> > So, the situation seems different at least on mt8192-asurada-
> > spherion.
> > Here, I2S8 is used for the headset microphone and I2S9 for the
> > headset audio.
> > Even for input and odd for output agree with Jiaxin's description.
> > However, the
> > input bus seems to be the main one, that is, disabling I2S8:
> >
> > amixer cset name='UL2_CH1 I2S8_CH1' 0
> > amixer cset name='UL2_CH2 I2S8_CH2' 0
> >
> > not only disables the microphone but also the audio on the headset.
> > If I add
> >
> > i2s9-share = "I2S8";
> >
> > on the DT, then everything works, I can disable I2S8 without
> > impacting the
> > headset audio. So the pattern for the property on this platform is
> > the opposite
> > that Jiaxin mentioned. This tells me that we should keep the binding
> > more
> > generic (not assume where odds and evens go). I will still apply the
> > other
> > suggestions mentioned though.
> >
> > Thanks,
> > N?colas
> >
> Hi N?colas,
>
> From software point, I2S8 and I2S9 belong to different hardware, so if
> you turn off I2S8 with CMD1, of course it will not affect I2S9.
>
> CMD1:
> amixer cset name='UL2_CH1 I2S8_CH1' 0
> amixer cset name='UL2_CH2 I2S8_CH2' 0
>
> Frome hardware point, I2S9 will use(share) I2S8's clock. If we don't
> want the user to perceive this, the driver need to help do something.
> So this property 'i2s9-share = "I2S8";' will be added to inform the
> driver.
Hi Jiaxin,
yes, that's what I figured. What I was saying is that for the binding, your
example was
i2s0-share = "I2S1"
^ even,input ^ odd,output
while on mt8192-asurada-spherion the use case is
i2s9-share = "I2S8";
^ odd,output ^ even,input
So Angelo's idea to require in the dt-binding that the left side is always even
(input) and the right side always odd (based on your example), wouldn't work for
my use case.
Basically it's a question of whether the input always shares the clock from an
output (your example), or if output always shares the clock from an input (my
use case), or if both are valid. I'm taking from this that both are valid, so I
won't add any such restriction in the dt-binding in the following version, but
do let me know if this is not the case.
Thanks,
N?colas