Hi Neil,
On Fri, Jan 7, 2022 at 3:57 PM Neil Armstrong <narmstrong@xxxxxxxxxxxx> 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
