Re: [PATCH 1/3] ASoC: dt-bindings: mediatek: mt8192: Add i2s-share properties

[Jiaxin Yu <jiaxin.yu@mediatek.com>]

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 <
> > > > > nfraprado@collabora.com>
> > > > > ---
> > > > > 
> > > > >    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.

Best Regards,
Jiaxin Yu

> > Nicolas, take note! :-) :-)
> > 
> > Thanks,
> > Angelo
> > 
> > To unsubscribe, send mail to 
> > kernel-unsubscribe@lists.collabora.co.uk.