On Mon, Feb 22, 2021 at 3:08 AM Laurent Pinchart laurent.pinchart@ideasonboard.com wrote:
Hi Nicolas,
Thank you for the patch.
On Thu, Feb 11, 2021 at 11:33:55AM +0800, Nicolas Boichat wrote:
Many of the DSI flags have names opposite to their actual effects, e.g. MIPI_DSI_MODE_EOT_PACKET means that EoT packets will actually be disabled. Fix this by including _NO_ in the flag names, e.g. MIPI_DSI_MODE_NO_EOT_PACKET.
Signed-off-by: Nicolas Boichat drinkcat@chromium.org
This looks good to me, it increases readability.
Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com
Please however see the end of the mail for a comment.
I considered adding _DISABLE_ instead, but that'd make the flag names a big too long.
Generated with: flag=MIPI_DSI_MODE_VIDEO_HFP; git grep $flag | cut -f1 -d':' | \ xargs -I{} sed -i -e "s/$flag/MIPI_DSI_MODE_VIDEO_NO_HFP/" {} flag=MIPI_DSI_MODE_VIDEO_HBP; git grep $flag | cut -f1 -d':' | \ xargs -I{} sed -i -e "s/$flag/MIPI_DSI_MODE_VIDEO_NO_HBP/" {} flag=MIPI_DSI_MODE_VIDEO_HSA; git grep $flag | cut -f1 -d':' | \ xargs -I{} sed -i -e "s/$flag/MIPI_DSI_MODE_VIDEO_NO_HSA/" {} flag=MIPI_DSI_MODE_EOT_PACKET; git grep $flag | cut -f1 -d':' | \ xargs -I{} sed -i -e "s/$flag/MIPI_DSI_MODE_NO_EOT_PACKET/" {} (then minor format changes)
Ever tried coccinelle ? :-)
Fun project for next time ,-)
drivers/gpu/drm/bridge/adv7511/adv7533.c | 2 +- drivers/gpu/drm/bridge/analogix/anx7625.c | 2 +- drivers/gpu/drm/bridge/cdns-dsi.c | 4 ++-- drivers/gpu/drm/bridge/tc358768.c | 2 +- drivers/gpu/drm/exynos/exynos_drm_dsi.c | 8 ++++---- drivers/gpu/drm/mcde/mcde_dsi.c | 2 +- drivers/gpu/drm/mediatek/mtk_dsi.c | 2 +- drivers/gpu/drm/msm/dsi/dsi_host.c | 8 ++++---- drivers/gpu/drm/panel/panel-asus-z00t-tm5p5-n35596.c | 2 +- drivers/gpu/drm/panel/panel-dsi-cm.c | 2 +- drivers/gpu/drm/panel/panel-elida-kd35t133.c | 2 +- drivers/gpu/drm/panel/panel-khadas-ts050.c | 2 +- drivers/gpu/drm/panel/panel-leadtek-ltk050h3146w.c | 2 +- drivers/gpu/drm/panel/panel-leadtek-ltk500hd1829.c | 2 +- drivers/gpu/drm/panel/panel-novatek-nt35510.c | 2 +- drivers/gpu/drm/panel/panel-osd-osd101t2587-53ts.c | 2 +- drivers/gpu/drm/panel/panel-samsung-s6d16d0.c | 2 +- drivers/gpu/drm/panel/panel-samsung-s6e63j0x03.c | 2 +- drivers/gpu/drm/panel/panel-samsung-s6e63m0-dsi.c | 2 +- drivers/gpu/drm/panel/panel-samsung-s6e8aa0.c | 4 ++-- drivers/gpu/drm/panel/panel-sharp-ls043t1le01.c | 2 +- drivers/gpu/drm/panel/panel-simple.c | 2 +- drivers/gpu/drm/panel/panel-sony-acx424akp.c | 2 +- drivers/gpu/drm/panel/panel-xinpeng-xpp055c272.c | 2 +- include/drm/drm_mipi_dsi.h | 8 ++++---- 25 files changed, 36 insertions(+), 36 deletions(-)
[] diff --git a/include/drm/drm_mipi_dsi.h b/include/drm/drm_mipi_dsi.h index 360e6377e84b..ba91cf22af51 100644 --- a/include/drm/drm_mipi_dsi.h +++ b/include/drm/drm_mipi_dsi.h @@ -119,15 +119,15 @@ struct mipi_dsi_host *of_find_mipi_dsi_host_by_node(struct device_node *node); /* enable hsync-end packets in vsync-pulse and v-porch area */ #define MIPI_DSI_MODE_VIDEO_HSE BIT(4)
We're mixing bits that enable a feature and bits that disable a feature. Are these bits defined in the DSI spec, or internal to DRM ? In the latter case, would it make sense to standardize on one "polarity" ? That would be a more intrusive change in drivers though.
Yes, that'd require auditing every single code path and reverse the logic as needed. I'm not volunteering for that ,-P (hopefully the current change is still an improvement).
Hopefully real DSI experts can comment (Andrzej?), I think the default are sensible settings?
/* disable hfront-porch area */ -#define MIPI_DSI_MODE_VIDEO_HFP BIT(5) +#define MIPI_DSI_MODE_VIDEO_NO_HFP BIT(5) /* disable hback-porch area */ -#define MIPI_DSI_MODE_VIDEO_HBP BIT(6) +#define MIPI_DSI_MODE_VIDEO_NO_HBP BIT(6) /* disable hsync-active area */ -#define MIPI_DSI_MODE_VIDEO_HSA BIT(7) +#define MIPI_DSI_MODE_VIDEO_NO_HSA BIT(7) /* flush display FIFO on vsync pulse */ #define MIPI_DSI_MODE_VSYNC_FLUSH BIT(8) /* disable EoT packets in HS mode */ -#define MIPI_DSI_MODE_EOT_PACKET BIT(9) +#define MIPI_DSI_MODE_NO_EOT_PACKET BIT(9) /* device supports non-continuous clock behavior (DSI spec 5.6.1) */ #define MIPI_DSI_CLOCK_NON_CONTINUOUS BIT(10) /* transmit data in low power */
-- Regards,
Laurent Pinchart