<html><body><p> <pre> Hi, Angelo: On Wed, 2024-02-07 at 15:53 +0100, AngeloGioacchino Del Regno wrote: > Instead of open coding bitshifting for various register fields, > use the bitfield macro FIELD_PREP(): this allows to enhance the > human readability, decrease likeliness of mistakes (and register > field overflowing) and also to simplify the code. > The latter is especially seen in mtk_dsi_rxtx_control(), where > it was possible to change a switch to a short for loop and to > also remove the need to check for maximum DSI lanes == 4 thanks > to the FIELD_PREP macro masking the value. > > While at it, also add the missing DA_HS_SYNC bitmask, used in > mtk_dsi_phy_timconfig(). > > Reviewed-by: Alexandre Mergnat <amergnat@baylibre.com> > Signed-off-by: AngeloGioacchino Del Regno < > angelogioacchino.delregno@collabora.com> > --- > drivers/gpu/drm/mediatek/mtk_dsi.c | 97 ++++++++++++++++---------- > ---- > 1 file changed, 52 insertions(+), 45 deletions(-) > > diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c > b/drivers/gpu/drm/mediatek/mtk_dsi.c > index 7d38e9500700..a330bb94c44a 100644 > --- a/drivers/gpu/drm/mediatek/mtk_dsi.c > +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c > @@ -3,6 +3,7 @@ > * Copyright (c) 2015 MediaTek Inc. > */ > > +#include <linux/bitfield.h> > #include <linux/clk.h> > #include <linux/component.h> > #include <linux/iopoll.h> > @@ -70,16 +71,19 @@ > #define DSI_PSCTRL0x1c > #define DSI_PS_WCGENMASK(13, 0) > #define DSI_PS_SELGENMASK(17, 16) > -#define PACKED_PS_16BIT_RGB565(0 << 16) > -#define PACKED_PS_18BIT_RGB666(1 << 16) > -#define LOOSELY_PS_24BIT_RGB666(2 << 16) > -#define PACKED_PS_24BIT_RGB888(3 << 16) > +#define PACKED_PS_16BIT_RGB5650 > +#define PACKED_PS_18BIT_RGB6661 > +#define LOOSELY_PS_24BIT_RGB6662 > +#define PACKED_PS_24BIT_RGB8883 > > #define DSI_VSA_NL0x20 > #define DSI_VBP_NL0x24 > #define DSI_VFP_NL0x28 > #define DSI_VACT_NL0x2C > +#define VACT_NLGENMASK(14, 0) > #define DSI_SIZE_CON0x38 > +#define DSI_HEIGHTGENMASK(30, 16) > +#define DSI_WIDTHGENMASK(14, 0) > #define DSI_HSA_WC0x50 > #define DSI_HBP_WC0x54 > #define DSI_HFP_WC0x58 > @@ -122,6 +126,7 @@ > > #define DSI_PHY_TIMECON20x118 > #define CONT_DETGENMASK(7, 0) > +#define DA_HS_SYNCGENMASK(15, 8) > #define CLK_ZEROGENMASK(23, 16) > #define CLK_TRAILGENMASK(31, 24) > > @@ -253,14 +258,23 @@ static void mtk_dsi_phy_timconfig(struct > mtk_dsi *dsi) > timing->clk_hs_zero = timing->clk_hs_trail * 4; > timing->clk_hs_exit = 2 * timing->clk_hs_trail; > > -timcon0 = timing->lpx | timing->da_hs_prepare << 8 | > - timing->da_hs_zero << 16 | timing->da_hs_trail << 24; > -timcon1 = timing->ta_go | timing->ta_sure << 8 | > - timing->ta_get << 16 | timing->da_hs_exit << 24; > -timcon2 = 1 << 8 | timing->clk_hs_zero << 16 | > - timing->clk_hs_trail << 24; > -timcon3 = timing->clk_hs_prepare | timing->clk_hs_post << 8 | > - timing->clk_hs_exit << 16; > +timcon0 = FIELD_PREP(LPX, timing->lpx) | > + FIELD_PREP(HS_PREP, timing->da_hs_prepare) | > + FIELD_PREP(HS_ZERO, timing->da_hs_zero) | > + FIELD_PREP(HS_TRAIL, timing->da_hs_trail); > + > +timcon1 = FIELD_PREP(TA_GO, timing->ta_go) | > + FIELD_PREP(TA_SURE, timing->ta_sure) | > + FIELD_PREP(TA_GET, timing->ta_get) | > + FIELD_PREP(DA_HS_EXIT, timing->da_hs_exit); > + > +timcon2 = FIELD_PREP(DA_HS_SYNC, 1) | > + FIELD_PREP(CLK_ZERO, timing->clk_hs_zero) | > + FIELD_PREP(CLK_TRAIL, timing->clk_hs_trail); > + > +timcon3 = FIELD_PREP(CLK_HS_PREP, timing->clk_hs_prepare) | > + FIELD_PREP(CLK_HS_POST, timing->clk_hs_post) | > + FIELD_PREP(CLK_HS_EXIT, timing->clk_hs_exit); > > writel(timcon0, dsi->regs + DSI_PHY_TIMECON0); > writel(timcon1, dsi->regs + DSI_PHY_TIMECON1); > @@ -353,69 +367,61 @@ static void mtk_dsi_set_vm_cmd(struct mtk_dsi > *dsi) > > static void mtk_dsi_rxtx_control(struct mtk_dsi *dsi) > { > -u32 tmp_reg; > +u32 regval, tmp_reg = 0; > +u8 i; > > -switch (dsi->lanes) { > -case 1: > -tmp_reg = 1 << 2; > -break; > -case 2: > -tmp_reg = 3 << 2; > -break; > -case 3: > -tmp_reg = 7 << 2; > -break; > -case 4: > -tmp_reg = 0xf << 2; > -break; > -default: > -tmp_reg = 0xf << 2; > -break; > -} > +/* Number of DSI lanes (max 4 lanes), each bit enables one DSI > lane. */ > +for (i = 0; i < dsi->lanes; i++) > +tmp_reg |= BIT(i); > + > +regval = FIELD_PREP(LANE_NUM, tmp_reg); > > if (dsi->mode_flags & MIPI_DSI_CLOCK_NON_CONTINUOUS) > -tmp_reg |= HSTX_CKLP_EN; > +regval |= HSTX_CKLP_EN; > > if (dsi->mode_flags & MIPI_DSI_MODE_NO_EOT_PACKET) > -tmp_reg |= DIS_EOT; > +regval |= DIS_EOT; > > -writel(tmp_reg, dsi->regs + DSI_TXRX_CTRL); > +writel(regval, dsi->regs + DSI_TXRX_CTRL); > } > > static void mtk_dsi_ps_control(struct mtk_dsi *dsi, bool > config_vact) > { > -struct videomode *vm = &dsi->vm; > -u32 dsi_buf_bpp, ps_wc; > -u32 ps_bpp_mode; > +u32 dsi_buf_bpp, ps_val, ps_wc, vact_nl; > > if (dsi->format == MIPI_DSI_FMT_RGB565) > dsi_buf_bpp = 2; > else > dsi_buf_bpp = 3; > > -ps_wc = vm->hactive * dsi_buf_bpp; > -ps_bpp_mode = ps_wc; > +/* Word count */ > +ps_wc = FIELD_PREP(DSI_PS_WC, dsi->vm.hactive * dsi_buf_bpp); > +ps_val = ps_wc; > > +/* Pixel Stream type */ > switch (dsi->format) { > +default: > +fallthrough; This is not related to bitfield macro. You remove default in previous patch but add it back in this patch. I think this should be moved to previous patch. After this change, Reviewed-by: CK Hu <ck.hu@mediatek.com> > case MIPI_DSI_FMT_RGB888: > -ps_bpp_mode |= PACKED_PS_24BIT_RGB888; > +ps_val |= FIELD_PREP(DSI_PS_SEL, > PACKED_PS_24BIT_RGB888); > break; > case MIPI_DSI_FMT_RGB666: > -ps_bpp_mode |= LOOSELY_PS_24BIT_RGB666; > +ps_val |= FIELD_PREP(DSI_PS_SEL, > LOOSELY_PS_24BIT_RGB666); > break; > case MIPI_DSI_FMT_RGB666_PACKED: > -ps_bpp_mode |= PACKED_PS_18BIT_RGB666; > +ps_val |= FIELD_PREP(DSI_PS_SEL, > PACKED_PS_18BIT_RGB666); > break; > case MIPI_DSI_FMT_RGB565: > -ps_bpp_mode |= PACKED_PS_16BIT_RGB565; > +ps_val |= FIELD_PREP(DSI_PS_SEL, > PACKED_PS_16BIT_RGB565); > break; > } > > if (config_vact) { > -writel(vm->vactive, dsi->regs + DSI_VACT_NL); > +vact_nl = FIELD_PREP(VACT_NL, dsi->vm.vactive); > +writel(vact_nl, dsi->regs + DSI_VACT_NL); > writel(ps_wc, dsi->regs + DSI_HSTX_CKL_WC); > } > -writel(ps_bpp_mode, dsi->regs + DSI_PSCTRL); > +writel(ps_val, dsi->regs + DSI_PSCTRL); > } > > static void mtk_dsi_config_vdo_timing(struct mtk_dsi *dsi) > @@ -442,7 +448,8 @@ static void mtk_dsi_config_vdo_timing(struct > mtk_dsi *dsi) > writel(vm->vactive, dsi->regs + DSI_VACT_NL); > > if (dsi->driver_data->has_size_ctl) > -writel(vm->vactive << 16 | vm->hactive, > +writel(FIELD_PREP(DSI_HEIGHT, vm->vactive) | > + FIELD_PREP(DSI_WIDTH, vm->hactive), > dsi->regs + DSI_SIZE_CON); > > horizontal_sync_active_byte = (vm->hsync_len * dsi_tmp_buf_bpp > - 10); </pre> </p></body></html><pre>************* MEDIATEK Confidentiality Notice ******************** The information contained in this e-mail message (including any attachments) may be confidential, proprietary, privileged, or otherwise exempt from disclosure under applicable laws. It is intended to be conveyed only to the designated recipient(s). Any use, dissemination, distribution, printing, retaining or copying of this e-mail (including its attachments) by unintended recipient(s) is strictly prohibited and may be unlawful. If you are not an intended recipient of this e-mail, or believe that you have received this e-mail in error, please notify the sender immediately (by replying to this e-mail), delete any and all copies of this e-mail (including any attachments) from your system, and do not disclose the content of this e-mail to any other person. Thank you! </pre>