Em Mon, 19 Sep 2016 21:33:13 +0300 Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx> escreveu: > Hi Mauro, > > On Monday 19 Sep 2016 15:26:15 Mauro Carvalho Chehab wrote: > > Em Mon, 19 Sep 2016 20:59:56 +0300 Laurent Pinchart escreveu: > > > On Monday 19 Sep 2016 14:55:43 Mauro Carvalho Chehab wrote: > > >> Em Wed, 14 Sep 2016 02:16:59 +0300 Laurent Pinchart escreveu: > > >>> Cropping on the WPF sink pad restricts the left and top coordinates to > > >>> 0-255. The same result can be obtained by cropping on the RPF without > > >>> any such restriction, this feature isn't useful. Disable it. > > >>> > > >>> Signed-off-by: Laurent Pinchart > > >>> <laurent.pinchart+renesas@xxxxxxxxxxxxxxxx> > > >>> --- > > >>> > > >>> drivers/media/platform/vsp1/vsp1_rwpf.c | 37 ++++++++++++------------ > > >>> drivers/media/platform/vsp1/vsp1_wpf.c | 18 +++++++--------- > > >>> 2 files changed, 26 insertions(+), 29 deletions(-) > > >>> > > >>> diff --git a/drivers/media/platform/vsp1/vsp1_rwpf.c > > >>> b/drivers/media/platform/vsp1/vsp1_rwpf.c index > > >>> 8cb87e96b78b..a3ace8df7f4d 100644 > > >>> --- a/drivers/media/platform/vsp1/vsp1_rwpf.c > > >>> +++ b/drivers/media/platform/vsp1/vsp1_rwpf.c > > [snip] > > > >>> @@ -129,8 +132,10 @@ static int vsp1_rwpf_get_selection(struct > > >>> v4l2_subdev *subdev, > > >>> struct v4l2_mbus_framefmt *format; > > >>> int ret = 0; > > >>> > > >>> - /* Cropping is implemented on the sink pad. */ > > >>> - if (sel->pad != RWPF_PAD_SINK) > > >>> + /* Cropping is only supported on the RPF and is implemented on > > >>> the sink > > >>> + * pad. > > >>> + */ > > >> > > >> Please read CodingStyle and run checkpatch before sending stuff > > >> upstream. > > >> > > >> This violates the CodingStyle: it should be, instead: > > >> /* > > >> * foo > > >> * bar > > >> */ > > > > > > But it's consistent with the coding style of this driver. I'm OK fixing > > > it, but it should be done globally in that case. > > > > There are inconsistencies inside the driver too on multi-line > > comments even without fixing the ones introduced on this series, > > as, on several places, multi-line comments are correct: > > > > drivers/media/platform/vsp1/vsp1_bru.c:/* > > drivers/media/platform/vsp1/vsp1_bru.c- * The BRU can't perform format > > conversion, all sink and source formats must be > > drivers/media/platform/vsp1/vsp1_bru.c- * identical. We pick the format on > > the first sink pad (pad 0) and propagate it > > drivers/media/platform/vsp1/vsp1_bru.c- * to all other pads. > > drivers/media/platform/vsp1/vsp1_bru.c- */ > > > > drivers/media/platform/vsp1/vsp1_dl.c:/* > > drivers/media/platform/vsp1/vsp1_dl.c- * Initialize a display list body > > object and allocate DMA memory for the body > > drivers/media/platform/vsp1/vsp1_dl.c- * data. The display list body object > > is expected to have been initialized to > > drivers/media/platform/vsp1/vsp1_dl.c- * 0 when allocated. > > drivers/media/platform/vsp1/vsp1_dl.c- */ > > > > ... > > > > I'll address the ones only the CodingStyle violation introduced by this > > series. I'll leave for the vsp1 maintainers/developers. > > OK, I'll address that. > > > Btw, there are several kernel-doc tags that use just: > > /* > > ... > > */ > > > > instead of: > > > > /** > > ... > > */ > > > > I suggest you to add the files/headers with kernel-doc markups on > > a Documentation/media/v4l-drivers/vsp1.rst file, to be created. > > > > This way, you can validate that such documentation is correct, > > and produce an auto-generated documentation for this driver. > > I've thought about it, but I don't think those comments should become part of > the kernel documentation. They're really about driver internals, and meant for > the driver developers. In particular only a subset of the driver is documented > that way, when I've considered that the code or structures were complex enough > to need proper documentation. A generated doc would then be quite incomplete > and not very useful, the comments are meant to be read while working on the > code. The v4l-drivers book is meant to have driver internals documentation, and not the subsystem kAPI or uAPI. I don't see any problems if you want to document just the more complex functions/structs over the v4l-drivers/ book. Yet, as you already took the time to write documentation for those functions, providing that the kernel-doc markups are ok, a v4l-drivers/vsp1.rst file for it could be as simple as (untested): VSP1 driver ^^^^^^^^^^^ .. kernel-doc:: drivers/media/platform/vsp1/vsp1_dl.c .. kernel-doc:: drivers/media/platform/vsp1/vsp1_drm.c .. kernel-doc:: drivers/media/platform/vsp1/vsp1_drm.h .. kernel-doc:: drivers/media/platform/vsp1/vsp1_entity.c .. kernel-doc:: drivers/media/platform/vsp1/vsp1_entity.h .. kernel-doc:: drivers/media/platform/vsp1/vsp1_pipe.c .. kernel-doc:: drivers/media/platform/vsp1/vsp1_video.c PS.: Eventually, you may need an extra attribute for the files with EXPORT_SYMBOL*, in order to associate a *.c file with the corresponding *.h file.
