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
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
