Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Wed, 12 Feb 2020 13:53:45 +0100
From:   Maxime Ripard <maxime@cerno.tech>
To:     Andrey Lebedev <andrey.lebedev@gmail.com>
Cc:     wens@csie.org, airlied@linux.ie, daniel@ffwll.ch,
        dri-devel@lists.freedesktop.org,
        linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 1/1] Support LVDS output on Allwinner A20
Message-ID: <20200212125345.j6e3txfjqekuxh2s@gilmour.lan>
References: <20200210195633.GA21832@kedthinkpad>
 <20200211072004.46tbqixn5ftilxae@gilmour.lan>
 <20200211204828.GA4361@kedthinkpad>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha256;
        protocol="application/pgp-signature"; boundary="pu5japc265ilzlxp"
Content-Disposition: inline
In-Reply-To: <20200211204828.GA4361@kedthinkpad>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


--pu5japc265ilzlxp
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

Hi Andrey,

On Tue, Feb 11, 2020 at 10:48:28PM +0200, Andrey Lebedev wrote:
> Maxime, thanks for your comments. I'll update the patch, but meanwhile,
> I have some remarks/questions, see below.
>
> On Tue, Feb 11, 2020 at 08:20:04AM +0100, Maxime Ripard wrote:
> > > +	regmap_update_bits(tcon->regs, SUN4I_TCON0_LVDS_ANA1_REG,
> > > +			   SUN4I_TCON0_LVDS_ANA1_UPDATE,
> > > +			   SUN4I_TCON0_LVDS_ANA1_UPDATE);
> >
> > You refer to U-Boot in your commit log, but the sequence is not quite
> > the same, why did you change it?
>
> I actually had two reference implementations at my hand. One was u-boot
> and another - an old (abandoned) branch of Priit Laes [1] (I took the
> split-up of u-boot SUNXI_LCDC_LVDS_ANA0 constant from there).
>
> This is an attempt to simplify the sequence, since I noticed that the
> same bit was being set to the same register twice [2] and removing that
> duplication didn't produce any observable regression. Priit
> implementation didn't have that bit set in the end of the sequence
> either, so I omitted it. That said, I agree that it could've been a bit
> naive on my side, and I can get it back to match u-boot version, if you
> feel that might be important.
>
> For the reference the U-Boot code is here: [3]
>
> [1] https://github.com/plaes/linux/commit/cc8c8bab2f2f2752ba6b11632dcd0f41bac249bc#diff-014a76a5007005a7a240825a972b8c7fR127
> [2] setbits_le32(&lcdc->lvds_ana0, SUNXI_LCDC_LVDS_ANA0_UPDATE);
> [3] https://github.com/ARM-software/u-boot/blob/master/drivers/video/sunxi/lcdc.c#L60

The U-Boot code has been here for a while and we know it's robust by
now, so I'd prefer to be conservative and use it here.

> > > +#define SUN4I_TCON0_LVDS_ANA1_REG		0x224
> > > +#define SUN4I_TCON0_LVDS_ANA1_INIT			(0x1f << 26 | 0x1f << 10)
> > > +#define SUN4I_TCON0_LVDS_ANA1_UPDATE			(0x1f << 16 | 0x1f << 00)
> >
> > Having proper defines for those fields would be great too.
>
> If by "proper" you mean "split them up to individual bits", I would
> agree, but I can't find any actual hardware reference documentation that
> would mention the meaning of these registers.

Of course we don't.. :)

It's fine to leave them as is then

> In both places (u-boot and Priit) these constants are defined the same way.
>
> I took the liberty to rename ANA1_INIT1 to ANA1_INIT and ANA1_INIT2 to
> ANA1_UPDATE to match Priit naming rather than u-boot, as I felt it was
> more descriptive. I have no strong opinion here though.
>
> > Side question, this will need some DT changes too, right?
>
> Hm, I agree. I think it would be reasonable to include LVDS0/1 pins

That, but most importantly, the reset and clocks for the LVDS
block. Also from looking at it, I'm not entirely sure that the TCON1
has a LVDS output, do you have a board when you have been able to test
it?

> and sample (but disabled) lvds panel,

That's good for the sake of the example, but it shouldn't be in the
same patch, it won't be merged.

> connected to tcon to arch/arm/boot/dts/sun7i-a20.dtsi. Does that
> make sense to you? Would you expect dts changes in the same patch or
> separate?
>
> P.S. This is my first patch to the linux kernel, please forgive me my
> inexperience.

You're doing fine so far :)
Maxime

--pu5japc265ilzlxp
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iHUEABYIAB0WIQRcEzekXsqa64kGDp7j7w1vZxhRxQUCXkP1WQAKCRDj7w1vZxhR
xZNVAQDk7kYMe1KzSTh9h8gVCxcVbKhxx1W/v2MVlCfHsy8/BQEAtsTcaVX0N/k6
q5+dn9ebIf4B0VB50BvQrFkvCbmxZA4=
=VUQU
-----END PGP SIGNATURE-----

--pu5japc265ilzlxp--