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

[Jiaxin Yu]

On Fri, 2022-05-06 at 10:09 -0400, Nícolas F. R. A. Prado wrote:
> 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 <
> > > > > > > nfraprado@xxxxxxxxxxxxx>
> > > > > > > ---
> > > > > > > 
> > > > > > >    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

Hi Nícolas,

Yes, you are right. They completely depend on which set of I2S clock
are used on the platform hardware.

 i2s9-share = "I2S8"
 ==> The I2S8 and the I2S9 combine into one set of standard I2S, and
use I2S8's clock.

 i2s8-share = "I2S9"
 ==> The I2S8 and the I2S9 combine into one set of standard I2S, and
use I2S9's clock.

Best Regards,
Jiaxin Yu