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

[Maxime Ripard <maxime@cerno.tech>]

[-- Attachment #1: Type: text/plain, Size: 3223 bytes --]

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@cerno.tech> 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@cerno.tech> 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.

> 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.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]