[PATCH v14 1/9] drm/display/dsc: Add flatness and initial scale value calculations

[Marijn Suijten]

On 2023-05-24 18:05:51, Jessica Zhang wrote:
<snip>
> >> +/**
> >> + * drm_dsc_initial_scale_value() - Calculate the initial scale value for the given DSC config
> >> + * @dsc: Pointer to DRM DSC config struct
> >> + *
> >> + * Return: Calculated initial scale value
> > 
> > Perhaps just drop Calculated from Return:?
> > 
> >> + */
> >> +u8 drm_dsc_initial_scale_value(const struct drm_dsc_config *dsc)
> >> +{
> >> +	return 8 * dsc->rc_model_size / (dsc->rc_model_size - dsc->initial_offset);
> >> +}
> >> +EXPORT_SYMBOL(drm_dsc_initial_scale_value);
> >> +
> >> +/**
> >> + * drm_dsc_flatness_det_thresh() - Calculate the flatness_det_thresh for the given DSC config
> > 
> > You've written out the word ("flatness det thresh" and "initial scale
> > value") entirely elsewhere, why the underscores in the doc comment here?
> > 
> > Instead we should have the full meaning here (and in the Return: below),
> > please correct me if I'm wrong but in VESA DSC v1.2a spec 6.8.5.1
> > Encoder Flatness Decision I think this variable means "flatness
> > determination threshold"?  If so, use that in the doc comment :)
> > 
> > (and drop the leading "the", so just "Calculate flatness determination
> > threshold for the given DSC config")
> > 
> >> + * @dsc: Pointer to DRM DSC config struct
> >> + *
> >> + * Return: Calculated flatness det thresh value
> > 
> > Nit: perhaps we can just drop "calculated" here?
> 
> 
> Hi Marijn,
> 
> Sure, I will make these changes if a v15 is necessary.
> 
> In the future, can we try to group comments on wording/grammar/patch 
> formatting with comments on the code itself?

Can you clarify what you mean?  v14 here is the first series including
this doc comment so there was no way for me to have reviewed this
earlier.  Code contents were already successfully reviewed many
revisions ago.

> I really appreciate your feedback and help in improving the 
> documentation around this feature, however I don't find it very 
> productive to have revisions where the only changes are on (in my 
> opinion) small wording details.

It is also down to you to have some patience and collect more review
from other maintainers and batch up changes, instead of spinning another
revision quickly after a review comment.

But this request can also be turned around: review and scan your own
series for simple inconsistencies before sending it to the lists, that
will surely make the time spent by reviewers much more "productive" as
well.
(Note that this goes hand in hand with the request to slow down
 consecutive revisions!)

And finally, as already said before: you can always decide to ignore my
review nits.  I am not a maintainer and don't have final say on whatever
is blocking for a patch to get merged.
But, when another revision is needed, the things I pointed out can at
least be incorporated, which is why they were shared in the first place.

Thanks for understanding.

- Marijn