Hi, Angelo:
On Wed, 2024-02-07 at 15:53 +0100, AngeloGioacchino Del Regno wrote:
> The register bits definitions for RGB666 formats are wrong in
> multiple
> ways: first, in the DSI_PS_SEL bits region, the Packed 18-bits RGB666
> format is selected with bit 1, while the Loosely Packed one is bit 2,
> and second - the definition name "LOOSELY_PS_18BIT_RGB666" is wrong
> because the loosely packed format is 24 bits instead!
>
> Either way, functions mtk_dsi_ps_control_vact() and
> mtk_dsi_ps_control()
> do not even agree on the DSI_PS_SEL bit to set in DSI_PSCTRL: one
> sets
> loosely packed (24) on RGB666, the other sets packed (18), and the
> other
> way around for RGB666_PACKED.
>
> Fixing this entire stack of issues is done in one go:
> - Use the correct bit for the Loosely Packed RGB666 definition
> - Rename LOOSELY_PS_18BIT_RGB666 to LOOSELY_PS_24BIT_RGB666
> - Change ps_bpp_mode in mtk_dsi_ps_control_vact() to set:
> - Loosely Packed, 24-bits for MIPI_DSI_FMT_RGB666
> - Packed, 18-bits for MIPI_DSI_FMT_RGB666_PACKED
Reviewed-by: CK Hu <ck.hu@mediatek.com>
>
> Fixes: 2e54c14e310f ("drm/mediatek: Add DSI sub driver")
> Reviewed-by: Alexandre Mergnat <amergnat@baylibre.com>
> Signed-off-by: AngeloGioacchino Del Regno <
> angelogioacchino.delregno@collabora.com>
> ---
> drivers/gpu/drm/mediatek/mtk_dsi.c | 10 +++++-----
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c
> b/drivers/gpu/drm/mediatek/mtk_dsi.c
> index c66e18006070..8af0afbe9e3d 100644
> --- a/drivers/gpu/drm/mediatek/mtk_dsi.c
> +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
> @@ -71,8 +71,8 @@
> #define DSI_PS_WC GENMASK(13, 0)
> #define DSI_PS_SEL GENMASK(17, 16)
> #define PACKED_PS_16BIT_RGB565 (0 << 16)
> -#define LOOSELY_PS_18BIT_RGB666 (1 << 16)
> -#define PACKED_PS_18BIT_RGB666 (2 << 16)
> +#define PACKED_PS_18BIT_RGB666 (1 << 16)
> +#define LOOSELY_PS_24BIT_RGB666 (2 << 16)
> #define PACKED_PS_24BIT_RGB888 (3 << 16)
>
> #define DSI_VSA_NL 0x20
> @@ -370,10 +370,10 @@ static void mtk_dsi_ps_control_vact(struct
> mtk_dsi *dsi)
> ps_bpp_mode |= PACKED_PS_24BIT_RGB888;
> break;
> case MIPI_DSI_FMT_RGB666:
> - ps_bpp_mode |= PACKED_PS_18BIT_RGB666;
> + ps_bpp_mode |= LOOSELY_PS_24BIT_RGB666;
> break;
> case MIPI_DSI_FMT_RGB666_PACKED:
> - ps_bpp_mode |= LOOSELY_PS_18BIT_RGB666;
> + ps_bpp_mode |= PACKED_PS_18BIT_RGB666;
> break;
> case MIPI_DSI_FMT_RGB565:
> ps_bpp_mode |= PACKED_PS_16BIT_RGB565;
> @@ -427,7 +427,7 @@ static void mtk_dsi_ps_control(struct mtk_dsi
> *dsi)
> dsi_tmp_buf_bpp = 3;
> break;
> case MIPI_DSI_FMT_RGB666:
> - tmp_reg = LOOSELY_PS_18BIT_RGB666;
> + tmp_reg = LOOSELY_PS_24BIT_RGB666;
> dsi_tmp_buf_bpp = 3;
> break;
> case MIPI_DSI_FMT_RGB666_PACKED: