Re: [PATCH 2/5] OMAP: DSS2: configuring non-zero values for fir_hinc/fir_vinc

[Tomi Valkeinen <tomi.valkeinen@ti.com>]

On Thu, 2011-05-19 at 11:01 +0530, Amber Jain wrote:
> fir_hinc and fir_vinc can only have a non-zero value as per TRM.
> Hence removed the if...else condition and also made the necesary changes

Typo with necessary.

> caused as the result of the condition removal.

Here also a bit more description would help to understand the patch.

If the patch is not totally trivial, I think it's normally good to
provide at least two distinct parts in the description, which are
something like:
- The current state, and what's wrong with it
- What this patch does and why it fixes the thing

So here you should tell something like:
- FIR values can not have zero values as per TRM, and the current code
writes zero there when no scaling is used. However, when zero values are
written to FIR registers scaling is disabled, so it shouldn't cause any
problems, but it's still safer to fix it.
- The patch changes to code to write a calculated FIR value even when no
scaling is used (meaning FIR value of 1024), but the scaling enable bits
are still kept off if scaling is not needed.

Also, if it's not obvious, it's nice to mention if the patch should
change some functionality or not. In this case nothing should change.

And generally try not to refer to the code in a way that requires the
reviewer to read the patch before understanding the description, like
"Hence removed the if...else condition and also made the necesary
changes caused as the result of the condition removal". The sentence
doesn't make sense if you don't read the patch first, which is totally
not the point of the description.

Either say something like "removed the if..else condition used to do
this and that, which cause this and that, requiring changing that and
this", which may get a bit confusing. Usually it's better to speak in
higher level terms about what the patch does (not always, though),
something similar to what I wrote in the paragraph someway up there.

 Tomi