Re: [PATCH 1/7] [media] tpg: Export the tpg code from vivid as a module

Laurent Pinchart
2015-08-13T20:14:37Z

Hi Helen,

Thank you for the patch. I just have a couple of small comments.

On Thursday 06 August 2015 17:26:08 Helen Fornazier wrote:
> The test pattern generator will be used by other drivers as the virtual
> media controller (vimc)
>
> Signed-off-by: Helen Fornazier <helen.fornazier@gmail.com>
> ---
> drivers/media/platform/Kconfig | 2 +
> drivers/media/platform/Makefile | 1 +
> drivers/media/platform/tpg/Kconfig | 5 +
> drivers/media/platform/tpg/Makefile | 3 +
> drivers/media/platform/tpg/tpg-colors.c | 1181 ++++++++++++
> drivers/media/platform/tpg/tpg-core.c | 2211 ++++++++++++++++++++
> drivers/media/platform/vivid/Kconfig | 1 +
> drivers/media/platform/vivid/Makefile | 2 +-
> drivers/media/platform/vivid/vivid-core.h | 2 +-
> drivers/media/platform/vivid/vivid-tpg-colors.c | 1182 ------------
> drivers/media/platform/vivid/vivid-tpg-colors.h | 68 -
> drivers/media/platform/vivid/vivid-tpg.c | 2191 --------------------
> drivers/media/platform/vivid/vivid-tpg.h | 596 ------
> include/media/tpg-colors.h | 68 +
> include/media/tpg.h | 595 ++++++
> 15 files changed, 4069 insertions(+), 4039 deletions(-)
> create mode 100644 drivers/media/platform/tpg/Kconfig
> create mode 100644 drivers/media/platform/tpg/Makefile
> create mode 100644 drivers/media/platform/tpg/tpg-colors.c
> create mode 100644 drivers/media/platform/tpg/tpg-core.c
> delete mode 100644 drivers/media/platform/vivid/vivid-tpg-colors.c
> delete mode 100644 drivers/media/platform/vivid/vivid-tpg-colors.h
> delete mode 100644 drivers/media/platform/vivid/vivid-tpg.c
> delete mode 100644 drivers/media/platform/vivid/vivid-tpg.h
> create mode 100644 include/media/tpg-colors.h
> create mode 100644 include/media/tpg.h

[snip]

> drivers diff --git a/drivers/media/platform/tpg/Makefile
> b/drivers/media/platform/tpg/Makefile new file mode 100644
> index 0000000..01f2212
> --- /dev/null
> +++ b/drivers/media/platform/tpg/Makefile
> @@ -0,0 +1,3 @@
> +tpg-objs := tpg-core.o tpg-colors.o
> +
> +obj-$(CONFIG_VIDEO_TPG) += tpg.o

I would call the module video-tpg, just tpg is a bit too generic.

[snip]

> diff --git a/include/media/tpg.h b/include/media/tpg.h
> new file mode 100644
> index 0000000..6dc79fb
> --- /dev/null
> +++ b/include/media/tpg.h
> @@ -0,0 +1,595 @@
> +/*
> + * tpg.h - Test Pattern Generator
> + *
> + * Copyright 2014 Cisco Systems, Inc. and/or its affiliates. All rights
> reserved.
> + *
> + * This program is free software; you may redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; version 2 of the License.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
> + * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
> + * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
> + * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS
> + * BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN
> + * ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN
> + * CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
> + * SOFTWARE.
> + */
> +
> +#ifndef _TPG_H_
> +#define _TPG_H_

For the same reason I'd use _MEDIA_TPG_H_ here, and for consistency,
_MEDIA_TPG_COLORS_H_ for tpg-colors.h.

--
Regards,

Laurent Pinchart