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/cc8c8bab2f2f2752ba6b11632dcd0f41bac249... [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....
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