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

[AngeloGioacchino Del Regno]

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.

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