[lkml]   [2020]   [Feb]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
SubjectRe: [PATCH 1/1] Support LVDS output on Allwinner A20
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,
> >
> > 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]
> [2] setbits_le32(&lcdc->lvds_ana0, SUNXI_LCDC_LVDS_ANA0_UPDATE);
> [3]

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

> 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 :)
[unhandled content-type:application/pgp-signature]
 \ /
  Last update: 2020-02-12 13:54    [W:0.212 / U:2.104 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site