On 2022-10-04 17:41:07, Dmitry Baryshkov wrote: > On Sat, 1 Oct 2022 at 23:23, Marijn Suijten > <marijn.suijten@xxxxxxxxxxxxxx> wrote: > [..] > > Pre-empting the reviews: I was contemplating whether to use FIELD_PREP > > here, given that it's not yet used anywhere else in this file. For that > > I'd remove the existing _SHIFT definitions and replace them with: > > > > #define DSC_PPS_RC_RANGE_MINQP_MASK GENMASK(15, 11) > > #define DSC_PPS_RC_RANGE_MAXQP_MASK GENMASK(10, 6) > > #define DSC_PPS_RC_RANGE_BPG_OFFSET_MASK GENMASK(5, 0) > > > > And turn this section of code into: > > > > cpu_to_be16(FIELD_PREP(DSC_PPS_RC_RANGE_MINQP_MASK, > > dsc_cfg->rc_range_params[i].range_min_qp) | > > FIELD_PREP(DSC_PPS_RC_RANGE_MAXQP_MASK, > > dsc_cfg->rc_range_params[i].range_max_qp) | > > FIELD_PREP(DSC_PPS_RC_RANGE_BPG_OFFSET_MASK, > > dsc_cfg->rc_range_params[i].range_bpg_offset)); > > > > Is that okay/recommended? > > This is definitely easier to review. However if you do not want to use > FIELD_PREP, it would be better to split this into a series of `data |= > something` assignments terminated with the rc_range_parameters[i] > assignment. Anything is fine by me, I have no strong opinion on this and rather leave it up to the maintainers. However, FIELD_PREP gives us concise `#define`s through a single `GENMASK()` carrying both the shift and mask/field-width. At the same time these genmask definitions map more clearly to the layout comment right above: /* * For DSC sink programming the RC Range parameter fields * are as follows: Min_qp[15:11], max_qp[10:6], offset[5:0] */ If switching to `data |=` however, I've been recommended to not use FIELD_PREP but fulyl write out `data |= (value & MASK) << SHIFT` instead. Perhaps a more important question is how to apply this consistently throughout the function? - Marijn
