Hi Neil,
On Fri, Jan 7, 2022 at 3:57 PM Neil Armstrong narmstrong@baylibre.com wrote:
This adds supports for the ENCL encoder connected to a MIPI-DSI transceiver on the Amlogic AXG SoCs.
Should this be "AXG and newer SoCs" or is this really AXG specific?
[...]
+#define GAMMA_VCOM_POL 7 /* RW */ +#define GAMMA_RVS_OUT 6 /* RW */ +#define ADR_RDY 5 /* Read Only */ +#define WR_RDY 4 /* Read Only */ +#define RD_RDY 3 /* Read Only */ +#define GAMMA_TR 2 /* RW */ +#define GAMMA_SET 1 /* RW */ +#define GAMMA_EN 0 /* RW */
+#define H_RD 12 +#define H_AUTO_INC 11 +#define H_SEL_R 10 +#define H_SEL_G 9 +#define H_SEL_B 8
I think all values above can be wrapped in the BIT() macro, then you don't need that below.
+#define HADR_MSB 7 /* 7:0 */ +#define HADR 0 /* 7:0 */
Here GENMASK(7, 0) can be used for HADR
Also I think prefixing all macros above with their register name (L_GAMMA_CNTL_PORT_ or L_GAMMA_ADDR_PORT_) will make the code easier to read.
[...]
writel_relaxed(0x8000, priv->io_base + _REG(ENCL_VIDEO_MODE));
The public S905 datasheet calls 0x8000 ENCL_PX_LN_CNT_SHADOW_EN
writel_relaxed(0x0418, priv->io_base + _REG(ENCL_VIDEO_MODE_ADV));
According to the public S905 datasheet this is: - BIT(3): ENCL_VIDEO_MODE_ADV_VFIFO_EN - BIT(4): ENCL_VIDEO_MODE_ADV_GAIN_HDTV - BIT(10): ENCL_SEL_GAMMA_RGB_IN
writel_relaxed(0x1000, priv->io_base + _REG(ENCL_VIDEO_FILT_CTRL));
I don't know the exact name but the 32-bit vendor kernel sources have a comment [0] saying that 0x1000 is "bypass filter" But maybe we can simply call it ENCL_VIDEO_FILT_CTRL_BYPASS_FILTER
[...]
writel_relaxed(3, priv->io_base + _REG(ENCL_VIDEO_RGBIN_CTRL));
The public S905 datasheet says: - BIT(0): USE RGB data from VIU, furthermore a comment in the 3.10 kernel sources make this more clear: bit[0] 1:RGB, 0:YUV - BIT(1): CFG_VIDEO_RGBIN_ZBLK
/* default black pattern */writel_relaxed(0, priv->io_base + _REG(ENCL_TST_MDSEL));writel_relaxed(0, priv->io_base + _REG(ENCL_TST_Y));writel_relaxed(0, priv->io_base + _REG(ENCL_TST_CB));writel_relaxed(0, priv->io_base + _REG(ENCL_TST_CR));writel_relaxed(1, priv->io_base + _REG(ENCL_TST_EN));writel_bits_relaxed(BIT(3), 0, priv->io_base + _REG(ENCL_VIDEO_MODE_ADV));
same as above: ENCL_VIDEO_MODE_ADV_VFIFO_EN
writel_relaxed(1, priv->io_base + _REG(ENCL_VIDEO_EN));writel_relaxed(0, priv->io_base + _REG(L_RGB_BASE_ADDR));writel_relaxed(0x400, priv->io_base + _REG(L_RGB_COEFF_ADDR));
note to self: L_RGB_COEFF_ADDR seems to contain some "magic" value, there's no further info in the 3.10 kernel sources or datasheet
writel_relaxed(0x400, priv->io_base + _REG(L_DITH_CNTL_ADDR));
According to the public S905 datasheet BIT(10) is DITH10_EN (10-bits Dithering to 8 Bits Enable). I am not sure if this would belong to the selected video mode/bit depth. I'll let other reviewers decide if this is relevant or not because I don't know.
[...]
writel_relaxed(0, priv->io_base + _REG(L_INV_CNT_ADDR));writel_relaxed(BIT(4) | BIT(5),priv->io_base + _REG(L_TCON_MISC_SEL_ADDR));
the public S905 datasheet states: - BIT(4): STV1_SEL (STV1 is frame Signal) - BIT(5): STV2_SEL (STV2 is frame Signal) This doesn't seem helpful to me though, but maybe you can still create preprocessor macros for this (for consistency)?
[...]
switch (priv->venc.current_mode) {case MESON_VENC_MODE_MIPI_DSI:writel_relaxed(0x200,priv->io_base + _REG(VENC_INTCTRL));
the public S905 datasheet documents this as: - BIT(9): ENCP_LNRST_INT_EN (Progressive encoder filed change interrupt enable) Please add a preprocessor macro to make it consistent with VENC_INTCTRL_ENCI_LNRST_INT_EN which already exists and is used below.
Best regards, Martin