Re: [PATCH v3 3/7] ASoC: sun4i-i2s: Add support for H6 I2S

[ClÃment PÃron]

Hi Maxime,

On Mon, 4 May 2020 at 14:09, Maxime Ripard <maxime@xxxxxxxxxx> wrote:
>
> Hi Clement,
>
> On Thu, Apr 30, 2020 at 04:00:14PM +0200, ClÃment PÃron wrote:
> > On Thu, 30 Apr 2020 at 10:46, Maxime Ripard <maxime@xxxxxxxxxx> wrote:
> > > On Wed, Apr 29, 2020 at 06:33:00PM +0200, ClÃment PÃron wrote:
> > > > On Wed, 29 Apr 2020 at 14:35, Maxime Ripard <maxime@xxxxxxxxxx> wrote:
> > > > >
> > > > > On Tue, Apr 28, 2020 at 10:55:47AM +0200, ClÃment PÃron wrote:
> > > > > > > > +static int sun50i_i2s_set_soc_fmt(const struct sun4i_i2s *i2s,
> > > > > > > > +                              unsigned int fmt)
> > > > > > >
> > > > > > > The alignment is off here
> > > > > > >
> > > > > > > > +{
> > > > > > > > +     u32 mode, val;
> > > > > > > > +     u8 offset;
> > > > > > > > +
> > > > > > > > +     /*
> > > > > > > > +      * DAI clock polarity
> > > > > > > > +      *
> > > > > > > > +      * The setup for LRCK contradicts the datasheet, but under a
> > > > > > > > +      * scope it's clear that the LRCK polarity is reversed
> > > > > > > > +      * compared to the expected polarity on the bus.
> > > > > > > > +      */
> > > > > > >
> > > > > > > Did you check this or has it been copy-pasted?
> > > > > >
> > > > > > copy-pasted, I will check this.
> > > > >
> > > > > It's not going to be easy to do this if you only have a board with HDMI. If you
> > > > > can't test that easily, just remove the comment (or make it explicit that you
> > > > > copy pasted it?), no comment is better than a wrong one.
> > > >
> > > > I have talked with Marcus Cooper it may be able to test this this week-end.
> > > > Also this can explain why we need the "
> > > > simple-audio-card,frame-inversion;" in the device-tree.
> > > >
> > > > If think this fix has been introduced by you, correct? Could you say
> > > > on which SoC did you see this issue?
> > >
> > > This was seen on an H3
> >
> > Just two more questions:
> > - Did you observe this issue on both TDM and I2S mode?
> > - On which DAI node?
>
> This is fairly fuzzy now and I don't remember if I tested it in I2S. Let's
> assume I didn't. And similarly, I'm not sure what the exact controller was, but
> it was one of the regular controllers (so not either connected to the codec or
> the HDMI, one that goes out of the SoC).
>
> We had pretty much the same issue on the A33 in I2S for the codec though:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/sound/soc/sunxi/sun8i-codec.c?id=18c1bf35c1c09bca05cf70bc984a4764e0b0372b
>
> I didn't think of it that way then, but it might very well have been the i2s
> controller suffering the same issue.
>
> > Since recent change in sun4i-i2s.c, we had to introduce the
> > "simple-audio-card,frame-inversion" in LibreElec tree.
>
> Do you have a link to that commit ? I couldn't find anything on libreelec's
> github repo.

These commits:
https://github.com/LibreELEC/LibreELEC.tv/blob/master/projects/Allwinner/devices/A64/patches/linux/04-Add-frame-inversion-to-correct-multi-channel-audio.patch
https://github.com/LibreELEC/LibreELEC.tv/blob/master/projects/Allwinner/devices/H3/patches/linux/17-Add-frame-inversion-to-correct-multi-channel-audio.patch
https://github.com/LibreELEC/LibreELEC.tv/blob/master/projects/Allwinner/devices/H6/patches/linux/16-Add-frame-inversion-to-correct-multi-channel-audio.patch

>
> > H3 boards are quite common in LibreElec community so I think:
> >  - This fix is only needed in TDM mode
> >  - Or this fix is not required for the HDMI DAI node (HDMI DAI is a
> > little bit different compare to other DAI but I think the first guess
> > is more likely)
>
> Given what we know about the A33, I'd be inclined to say the latter. I'd don't
> have the tools to check anymore, but if you have even a cheap logical analyzer,
> i2s being pretty slow you can definitely see it.

Me neither but maybe Marcus will be able to check this.
Thanks for all these informations.

>
> Maxime