On 07/01/2022 23:33, Martin Blumenstingl wrote:
> 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?
Yup should be, thanks for noting
>
> [...]
>> +#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.
yep
>
>> +#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
Thanks for searching !
>
>> + 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
Yep
>
> [...]
>> + 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.
it would probably for pre-GXL when the pipeline was 8bit, would probably need to add
a comment if someone wants to us DPI/LVDS on pre-GXL.
>
> [...]
>> + 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)?
yep
>
> [...]
>> + 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.
Yep
Thanks for the review :-)
Neil
>
>
> Best regards,
> Martin
>
