Messages in this thread | | | Date | Wed, 22 Dec 2021 14:10:12 +0800 | Subject | Re: [PATCH 2/4] media: aspeed: Use FIELD_GET to improve readability | From | Jammy Huang <> |
| |
Hi Joel,
OK, I will update in next patch as you advised. Thanks for your review.
On 2021/12/22 上午 08:59, Joel Stanley wrote: > On Fri, 17 Dec 2021 at 09:53, Jammy Huang <jammy_huang@aspeedtech.com> wrote: >> Use the bitfield macro FIELD_GET, and GENMASK to do the shift and mask in >> one go for reg values. >> >> Signed-off-by: Jammy Huang <jammy_huang@aspeedtech.com> >> --- >> drivers/media/platform/aspeed-video.c | 35 ++++++++++++--------------- >> 1 file changed, 16 insertions(+), 19 deletions(-) >> >> diff --git a/drivers/media/platform/aspeed-video.c b/drivers/media/platform/aspeed-video.c >> index d5f77b205175..581a4261f9b7 100644 >> --- a/drivers/media/platform/aspeed-video.c >> +++ b/drivers/media/platform/aspeed-video.c >> @@ -156,26 +156,22 @@ >> #define VE_SRC_LR_EDGE_DET_NO_H BIT(13) >> #define VE_SRC_LR_EDGE_DET_NO_DISP BIT(14) >> #define VE_SRC_LR_EDGE_DET_NO_CLK BIT(15) >> -#define VE_SRC_LR_EDGE_DET_RT_SHF 16 >> -#define VE_SRC_LR_EDGE_DET_RT GENMASK(27, VE_SRC_LR_EDGE_DET_RT_SHF) >> +#define VE_SRC_LR_EDGE_DET_RT GENMASK(27, 16) >> #define VE_SRC_LR_EDGE_DET_INTERLACE BIT(31) >> >> #define VE_SRC_TB_EDGE_DET 0x094 >> #define VE_SRC_TB_EDGE_DET_TOP GENMASK(12, 0) >> -#define VE_SRC_TB_EDGE_DET_BOT_SHF 16 >> -#define VE_SRC_TB_EDGE_DET_BOT GENMASK(28, VE_SRC_TB_EDGE_DET_BOT_SHF) >> +#define VE_SRC_TB_EDGE_DET_BOT GENMASK(28, 16) >> >> #define VE_MODE_DETECT_STATUS 0x098 >> #define VE_MODE_DETECT_H_PERIOD GENMASK(11, 0) >> -#define VE_MODE_DETECT_V_LINES_SHF 16 >> -#define VE_MODE_DETECT_V_LINES GENMASK(27, VE_MODE_DETECT_V_LINES_SHF) >> +#define VE_MODE_DETECT_V_LINES GENMASK(27, 16) >> #define VE_MODE_DETECT_STATUS_VSYNC BIT(28) >> #define VE_MODE_DETECT_STATUS_HSYNC BIT(29) >> >> #define VE_SYNC_STATUS 0x09c >> #define VE_SYNC_STATUS_HSYNC GENMASK(11, 0) >> -#define VE_SYNC_STATUS_VSYNC_SHF 16 >> -#define VE_SYNC_STATUS_VSYNC GENMASK(27, VE_SYNC_STATUS_VSYNC_SHF) >> +#define VE_SYNC_STATUS_VSYNC GENMASK(27, 16) >> >> #define VE_H_TOTAL_PIXELS 0x0A0 >> >> @@ -988,23 +984,24 @@ static void aspeed_video_get_resolution(struct aspeed_video *video) >> sync = aspeed_video_read(video, VE_SYNC_STATUS); >> htotal = aspeed_video_read(video, VE_H_TOTAL_PIXELS); >> >> - video->frame_bottom = (src_tb_edge & VE_SRC_TB_EDGE_DET_BOT) >> >> - VE_SRC_TB_EDGE_DET_BOT_SHF; >> - video->frame_top = src_tb_edge & VE_SRC_TB_EDGE_DET_TOP; >> + video->frame_bottom = FIELD_GET(VE_SRC_TB_EDGE_DET_BOT, >> + src_tb_edge); >> + video->frame_top = FIELD_GET(VE_SRC_TB_EDGE_DET_TOP, >> + src_tb_edge); >> det->vfrontporch = video->frame_top; >> - det->vbackporch = ((mds & VE_MODE_DETECT_V_LINES) >> >> - VE_MODE_DETECT_V_LINES_SHF) - video->frame_bottom; >> - det->vsync = (sync & VE_SYNC_STATUS_VSYNC) >> >> - VE_SYNC_STATUS_VSYNC_SHF; >> + det->vbackporch = FIELD_GET(VE_MODE_DETECT_V_LINES, mds) - >> + video->frame_bottom; >> + det->vsync = FIELD_GET(VE_SYNC_STATUS_VSYNC, sync); >> if (video->frame_top > video->frame_bottom) >> continue; >> >> - video->frame_right = (src_lr_edge & VE_SRC_LR_EDGE_DET_RT) >> >> - VE_SRC_LR_EDGE_DET_RT_SHF; >> - video->frame_left = src_lr_edge & VE_SRC_LR_EDGE_DET_LEFT; >> + video->frame_right = FIELD_GET(VE_SRC_LR_EDGE_DET_RT, >> + src_lr_edge); >> + video->frame_left = FIELD_GET(VE_SRC_LR_EDGE_DET_LEFT, >> + src_lr_edge); > I suggest putting these on one line to further improve readability: > > video->frame_right = FIELD_GET(VE_SRC_LR_EDGE_DET_RT, > src_lr_edge); > video->frame_left = FIELD_GET(VE_SRC_LR_EDGE_DET_LEFT, > src_lr_edge); > > The same for the other lines you've changed above. > > And then add: > > Reviewed-by: Joel Stanley <joel@jms.id.au> > >> det->hfrontporch = video->frame_left; >> det->hbackporch = htotal - video->frame_right; >> - det->hsync = sync & VE_SYNC_STATUS_HSYNC; >> + det->hsync = FIELD_GET(VE_SYNC_STATUS_HSYNC, sync); >> if (video->frame_left > video->frame_right) >> continue; >> >> -- >> 2.25.1 >> -- Best Regards Jammy
| |