[PATCH v7 1/8] davinci: vpif: remove machine specific inclusion from driver

Nori, Sekhar nsekhar at ti.com
Thu Dec 22 08:10:01 EST 2011


Hi Manju,

On Wed, Dec 21, 2011 at 19:13:34, Hadli, Manjunath wrote:
> remove machine specific inclusion from the driver which
> comes in the way of platform code consolidation.

I think it would be more readable to use the term "header file"
here and in the headline. Just "machine specific inclusion"
begs the question - "inclusion of what?"

> currently was seen that these header inclusions were
> not necessary.

Sorry about nit-picking, but it is not good to talk in
past tense in commit text. Past tense is natural for you
to use since you write the text after making the changes,
but for the reviewer it is not natural since he is seeing
the commit text and the patch both at once. Also, usage
of "currently" in above line is not necessary. It is assumed
that commit text talks about current state of affairs.

I would have made these changes myself after Mauro's ack,
but..

> 
> Signed-off-by: Manjunath Hadli <manjunath.hadli at ti.com>
> Cc: Mauro Carvalho Chehab <mchehab at infradead.org>
> Cc: LMML <linux-media at vger.kernel.org>
> ---
>  drivers/media/video/davinci/vpif.h         |    2 --
>  drivers/media/video/davinci/vpif_display.c |    2 --
>  include/media/davinci/vpif_types.h         |    2 ++
>  sound/soc/codecs/cq93vc.c                  |    2 --

.. you clubbed this unrelated sound/soc/ change in this patch.
First, the change is not related to VPIF in any way so it
has no business being in this patch. Second, there is no way
the sound/soc folks will have a look at this patch, so basically
the change will end up bypassing the right maintainers if other
reviewers fail to catch it.

Please separate the change into another patch. You can just
post the two patches alone copying the right maintainers
in each instead of posting the entire series again.

Thanks,
Sekhar




More information about the linux-arm-kernel mailing list