In preparation for collapsing the component driver structure into
a monolith, move private device structure defines to a new common
header file.
Signed-off-by: Shuah Khan <[email protected]>
---
drivers/media/platform/vimc/vimc-capture.c | 21 +----
drivers/media/platform/vimc/vimc-core.c | 18 +---
drivers/media/platform/vimc/vimc-debayer.c | 16 +---
drivers/media/platform/vimc/vimc-scaler.c | 15 +--
drivers/media/platform/vimc/vimc-sensor.c | 13 +--
drivers/media/platform/vimc/vimc.h | 102 +++++++++++++++++++++
6 files changed, 107 insertions(+), 78 deletions(-)
create mode 100644 drivers/media/platform/vimc/vimc.h
diff --git a/drivers/media/platform/vimc/vimc-capture.c b/drivers/media/platform/vimc/vimc-capture.c
index 664855708fdf..c52fc5d97c2d 100644
--- a/drivers/media/platform/vimc/vimc-capture.c
+++ b/drivers/media/platform/vimc/vimc-capture.c
@@ -13,6 +13,7 @@
#include <media/videobuf2-core.h>
#include <media/videobuf2-vmalloc.h>
+#include "vimc.h"
#include "vimc-common.h"
#include "vimc-streamer.h"
@@ -44,26 +45,6 @@ static const u32 vimc_cap_supported_pixfmt[] = {
V4L2_PIX_FMT_SRGGB12,
};
-struct vimc_cap_device {
- struct vimc_ent_device ved;
- struct video_device vdev;
- struct device *dev;
- struct v4l2_pix_format format;
- struct vb2_queue queue;
- struct list_head buf_list;
- /*
- * NOTE: in a real driver, a spin lock must be used to access the
- * queue because the frames are generated from a hardware interruption
- * and the isr is not allowed to sleep.
- * Even if it is not necessary a spinlock in the vimc driver, we
- * use it here as a code reference
- */
- spinlock_t qlock;
- struct mutex lock;
- u32 sequence;
- struct vimc_stream stream;
-};
-
static const struct v4l2_pix_format fmt_default = {
.width = 640,
.height = 480,
diff --git a/drivers/media/platform/vimc/vimc-core.c b/drivers/media/platform/vimc/vimc-core.c
index 571c55aa0e16..c9b351472272 100644
--- a/drivers/media/platform/vimc/vimc-core.c
+++ b/drivers/media/platform/vimc/vimc-core.c
@@ -12,6 +12,7 @@
#include <media/media-device.h>
#include <media/v4l2-device.h>
+#include "vimc.h"
#include "vimc-common.h"
#define VIMC_MDEV_MODEL_NAME "VIMC MDEV"
@@ -24,23 +25,6 @@
.flags = link_flags, \
}
-struct vimc_device {
- /* The platform device */
- struct platform_device pdev;
-
- /* The pipeline configuration */
- const struct vimc_pipeline_config *pipe_cfg;
-
- /* The Associated media_device parent */
- struct media_device mdev;
-
- /* Internal v4l2 parent device*/
- struct v4l2_device v4l2_dev;
-
- /* Subdevices */
- struct platform_device **subdevs;
-};
-
/* Structure which describes individual configuration for each entity */
struct vimc_ent_config {
const char *name;
diff --git a/drivers/media/platform/vimc/vimc-debayer.c b/drivers/media/platform/vimc/vimc-debayer.c
index 00598fbf3cba..750752bb173c 100644
--- a/drivers/media/platform/vimc/vimc-debayer.c
+++ b/drivers/media/platform/vimc/vimc-debayer.c
@@ -13,6 +13,7 @@
#include <linux/v4l2-mediabus.h>
#include <media/v4l2-subdev.h>
+#include "vimc.h"
#include "vimc-common.h"
#define VIMC_DEB_DRV_NAME "vimc-debayer"
@@ -44,21 +45,6 @@ struct vimc_deb_pix_map {
enum vimc_deb_rgb_colors order[2][2];
};
-struct vimc_deb_device {
- struct vimc_ent_device ved;
- struct v4l2_subdev sd;
- struct device *dev;
- /* The active format */
- struct v4l2_mbus_framefmt sink_fmt;
- u32 src_code;
- void (*set_rgb_src)(struct vimc_deb_device *vdeb, unsigned int lin,
- unsigned int col, unsigned int rgb[3]);
- /* Values calculated when the stream starts */
- u8 *src_frame;
- const struct vimc_deb_pix_map *sink_pix_map;
- unsigned int sink_bpp;
-};
-
static const struct v4l2_mbus_framefmt sink_fmt_default = {
.width = 640,
.height = 480,
diff --git a/drivers/media/platform/vimc/vimc-scaler.c b/drivers/media/platform/vimc/vimc-scaler.c
index c7123a45c55b..fe99b9102ada 100644
--- a/drivers/media/platform/vimc/vimc-scaler.c
+++ b/drivers/media/platform/vimc/vimc-scaler.c
@@ -13,6 +13,7 @@
#include <linux/v4l2-mediabus.h>
#include <media/v4l2-subdev.h>
+#include "vimc.h"
#include "vimc-common.h"
#define VIMC_SCA_DRV_NAME "vimc-scaler"
@@ -31,20 +32,6 @@ static const u32 vimc_sca_supported_pixfmt[] = {
V4L2_PIX_FMT_ARGB32,
};
-struct vimc_sca_device {
- struct vimc_ent_device ved;
- struct v4l2_subdev sd;
- struct device *dev;
- /* NOTE: the source fmt is the same as the sink
- * with the width and hight multiplied by mult
- */
- struct v4l2_mbus_framefmt sink_fmt;
- /* Values calculated when the stream starts */
- u8 *src_frame;
- unsigned int src_line_size;
- unsigned int bpp;
-};
-
static const struct v4l2_mbus_framefmt sink_fmt_default = {
.width = 640,
.height = 480,
diff --git a/drivers/media/platform/vimc/vimc-sensor.c b/drivers/media/platform/vimc/vimc-sensor.c
index 51359472eef2..6c57b1e262f9 100644
--- a/drivers/media/platform/vimc/vimc-sensor.c
+++ b/drivers/media/platform/vimc/vimc-sensor.c
@@ -16,22 +16,11 @@
#include <media/v4l2-subdev.h>
#include <media/tpg/v4l2-tpg.h>
+#include "vimc.h"
#include "vimc-common.h"
#define VIMC_SEN_DRV_NAME "vimc-sensor"
-struct vimc_sen_device {
- struct vimc_ent_device ved;
- struct v4l2_subdev sd;
- struct device *dev;
- struct tpg_data tpg;
- struct task_struct *kthread_sen;
- u8 *frame;
- /* The active format */
- struct v4l2_mbus_framefmt mbus_format;
- struct v4l2_ctrl_handler hdl;
-};
-
static const struct v4l2_mbus_framefmt fmt_default = {
.width = 640,
.height = 480,
diff --git a/drivers/media/platform/vimc/vimc.h b/drivers/media/platform/vimc/vimc.h
new file mode 100644
index 000000000000..a5adebdda941
--- /dev/null
+++ b/drivers/media/platform/vimc/vimc.h
@@ -0,0 +1,102 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/*
+ *
+ * Copyright (C) 2019 Shuah Khan <[email protected]>
+ *
+ */
+
+/*
+ * This file defines vimc driver device and sub-device structures.
+ */
+
+#ifndef _VIMC_H_
+#define _VIMC_H_
+
+#include <linux/platform_device.h>
+#include <media/media-device.h>
+#include <media/v4l2-device.h>
+#include <media/videobuf2-core.h>
+#include <media/tpg/v4l2-tpg.h>
+#include <media/v4l2-ctrls.h>
+
+#include "vimc-common.h"
+
+struct vimc_cap_device {
+ struct vimc_ent_device ved;
+ struct video_device vdev;
+ struct device *dev;
+ struct v4l2_pix_format format;
+ struct vb2_queue queue;
+ struct list_head buf_list;
+ /*
+ * NOTE: in a real driver, a spin lock must be used to access the
+ * queue because the frames are generated from a hardware interruption
+ * and the isr is not allowed to sleep.
+ * Even if it is not necessary a spinlock in the vimc driver, we
+ * use it here as a code reference
+ */
+ spinlock_t qlock;
+ struct mutex lock;
+ u32 sequence;
+ struct vimc_stream stream;
+};
+
+struct vimc_sca_device {
+ struct vimc_ent_device ved;
+ struct v4l2_subdev sd;
+ struct device *dev;
+ /* NOTE: the source fmt is the same as the sink
+ * with the width and hight multiplied by mult
+ */
+ struct v4l2_mbus_framefmt sink_fmt;
+ /* Values calculated when the stream starts */
+ u8 *src_frame;
+ unsigned int src_line_size;
+ unsigned int bpp;
+};
+
+struct vimc_deb_device {
+ struct vimc_ent_device ved;
+ struct v4l2_subdev sd;
+ struct device *dev;
+ /* The active format */
+ struct v4l2_mbus_framefmt sink_fmt;
+ u32 src_code;
+ void (*set_rgb_src)(struct vimc_deb_device *vdeb, unsigned int lin,
+ unsigned int col, unsigned int rgb[3]);
+ /* Values calculated when the stream starts */
+ u8 *src_frame;
+ const struct vimc_deb_pix_map *sink_pix_map;
+ unsigned int sink_bpp;
+};
+
+struct vimc_sen_device {
+ struct vimc_ent_device ved;
+ struct v4l2_subdev sd;
+ struct device *dev;
+ struct tpg_data tpg;
+ struct task_struct *kthread_sen;
+ u8 *frame;
+ /* The active format */
+ struct v4l2_mbus_framefmt mbus_format;
+ struct v4l2_ctrl_handler hdl;
+};
+
+struct vimc_device {
+ /* The platform device */
+ struct platform_device pdev;
+
+ /* The pipeline configuration */
+ const struct vimc_pipeline_config *pipe_cfg;
+
+ /* The Associated media_device parent */
+ struct media_device mdev;
+
+ /* Internal v4l2 parent device*/
+ struct v4l2_device v4l2_dev;
+
+ /* Subdevices */
+ struct platform_device **subdevs;
+};
+
+#endif
--
2.17.1
Hi Shuah,
Thanks for the patch.
On 8/9/19 6:45 PM, Shuah Khan wrote:
> In preparation for collapsing the component driver structure into
> a monolith, move private device structure defines to a new common
> header file.
>
> Signed-off-by: Shuah Khan <[email protected]>
> ---
> drivers/media/platform/vimc/vimc-capture.c | 21 +----
> drivers/media/platform/vimc/vimc-core.c | 18 +---
> drivers/media/platform/vimc/vimc-debayer.c | 16 +---
> drivers/media/platform/vimc/vimc-scaler.c | 15 +--
> drivers/media/platform/vimc/vimc-sensor.c | 13 +--
> drivers/media/platform/vimc/vimc.h | 102 +++++++++++++++++++++
> 6 files changed, 107 insertions(+), 78 deletions(-)
> create mode 100644 drivers/media/platform/vimc/vimc.h
>
> diff --git a/drivers/media/platform/vimc/vimc-capture.c b/drivers/media/platform/vimc/vimc-capture.c
> index 664855708fdf..c52fc5d97c2d 100644
> --- a/drivers/media/platform/vimc/vimc-capture.c
> +++ b/drivers/media/platform/vimc/vimc-capture.c
> @@ -13,6 +13,7 @@
> #include <media/videobuf2-core.h>
> #include <media/videobuf2-vmalloc.h>
>
> +#include "vimc.h"
> #include "vimc-common.h"
> #include "vimc-streamer.h"
>
> @@ -44,26 +45,6 @@ static const u32 vimc_cap_supported_pixfmt[] = {
> V4L2_PIX_FMT_SRGGB12,
> };
>
> -struct vimc_cap_device {
> - struct vimc_ent_device ved;
> - struct video_device vdev;
> - struct device *dev;
> - struct v4l2_pix_format format;
> - struct vb2_queue queue;
> - struct list_head buf_list;
> - /*
> - * NOTE: in a real driver, a spin lock must be used to access the
> - * queue because the frames are generated from a hardware interruption
> - * and the isr is not allowed to sleep.
> - * Even if it is not necessary a spinlock in the vimc driver, we
> - * use it here as a code reference
> - */
> - spinlock_t qlock;
> - struct mutex lock;
> - u32 sequence;
> - struct vimc_stream stream;
> -};
> -
> static const struct v4l2_pix_format fmt_default = {
> .width = 640,
> .height = 480,
> diff --git a/drivers/media/platform/vimc/vimc-core.c b/drivers/media/platform/vimc/vimc-core.c
> index 571c55aa0e16..c9b351472272 100644
> --- a/drivers/media/platform/vimc/vimc-core.c
> +++ b/drivers/media/platform/vimc/vimc-core.c
> @@ -12,6 +12,7 @@
> #include <media/media-device.h>
> #include <media/v4l2-device.h>
>
> +#include "vimc.h"
> #include "vimc-common.h"
>
> #define VIMC_MDEV_MODEL_NAME "VIMC MDEV"
> @@ -24,23 +25,6 @@
> .flags = link_flags, \
> }
>
> -struct vimc_device {
> - /* The platform device */
> - struct platform_device pdev;
> -
> - /* The pipeline configuration */
> - const struct vimc_pipeline_config *pipe_cfg;
> -
> - /* The Associated media_device parent */
> - struct media_device mdev;
> -
> - /* Internal v4l2 parent device*/
> - struct v4l2_device v4l2_dev;
> -
> - /* Subdevices */
> - struct platform_device **subdevs;
> -};
> -
> /* Structure which describes individual configuration for each entity */
> struct vimc_ent_config {
> const char *name;
> diff --git a/drivers/media/platform/vimc/vimc-debayer.c b/drivers/media/platform/vimc/vimc-debayer.c
> index 00598fbf3cba..750752bb173c 100644
> --- a/drivers/media/platform/vimc/vimc-debayer.c
> +++ b/drivers/media/platform/vimc/vimc-debayer.c
> @@ -13,6 +13,7 @@
> #include <linux/v4l2-mediabus.h>
> #include <media/v4l2-subdev.h>
>
> +#include "vimc.h"
> #include "vimc-common.h"
>
> #define VIMC_DEB_DRV_NAME "vimc-debayer"
> @@ -44,21 +45,6 @@ struct vimc_deb_pix_map {
> enum vimc_deb_rgb_colors order[2][2];
> };
>
> -struct vimc_deb_device {
> - struct vimc_ent_device ved;
> - struct v4l2_subdev sd;
> - struct device *dev;
> - /* The active format */
> - struct v4l2_mbus_framefmt sink_fmt;
> - u32 src_code;
> - void (*set_rgb_src)(struct vimc_deb_device *vdeb, unsigned int lin,
> - unsigned int col, unsigned int rgb[3]);
> - /* Values calculated when the stream starts */
> - u8 *src_frame;
> - const struct vimc_deb_pix_map *sink_pix_map;
> - unsigned int sink_bpp;
> -};
> -
> static const struct v4l2_mbus_framefmt sink_fmt_default = {
> .width = 640,
> .height = 480,
> diff --git a/drivers/media/platform/vimc/vimc-scaler.c b/drivers/media/platform/vimc/vimc-scaler.c
> index c7123a45c55b..fe99b9102ada 100644
> --- a/drivers/media/platform/vimc/vimc-scaler.c
> +++ b/drivers/media/platform/vimc/vimc-scaler.c
> @@ -13,6 +13,7 @@
> #include <linux/v4l2-mediabus.h>
> #include <media/v4l2-subdev.h>
>
> +#include "vimc.h"
> #include "vimc-common.h"
>
> #define VIMC_SCA_DRV_NAME "vimc-scaler"
> @@ -31,20 +32,6 @@ static const u32 vimc_sca_supported_pixfmt[] = {
> V4L2_PIX_FMT_ARGB32,
> };
>
> -struct vimc_sca_device {
> - struct vimc_ent_device ved;
> - struct v4l2_subdev sd;
> - struct device *dev;
> - /* NOTE: the source fmt is the same as the sink
> - * with the width and hight multiplied by mult
> - */
> - struct v4l2_mbus_framefmt sink_fmt;
> - /* Values calculated when the stream starts */
> - u8 *src_frame;
> - unsigned int src_line_size;
> - unsigned int bpp;
> -};
> -
> static const struct v4l2_mbus_framefmt sink_fmt_default = {
> .width = 640,
> .height = 480,
> diff --git a/drivers/media/platform/vimc/vimc-sensor.c b/drivers/media/platform/vimc/vimc-sensor.c
> index 51359472eef2..6c57b1e262f9 100644
> --- a/drivers/media/platform/vimc/vimc-sensor.c
> +++ b/drivers/media/platform/vimc/vimc-sensor.c
> @@ -16,22 +16,11 @@
> #include <media/v4l2-subdev.h>
> #include <media/tpg/v4l2-tpg.h>
>
> +#include "vimc.h"
> #include "vimc-common.h"
>
> #define VIMC_SEN_DRV_NAME "vimc-sensor"
>
> -struct vimc_sen_device {
> - struct vimc_ent_device ved;
> - struct v4l2_subdev sd;
> - struct device *dev;
> - struct tpg_data tpg;
> - struct task_struct *kthread_sen;
> - u8 *frame;
> - /* The active format */
> - struct v4l2_mbus_framefmt mbus_format;
> - struct v4l2_ctrl_handler hdl;
> -};
> -
> static const struct v4l2_mbus_framefmt fmt_default = {
> .width = 640,
> .height = 480,
> diff --git a/drivers/media/platform/vimc/vimc.h b/drivers/media/platform/vimc/vimc.h
> new file mode 100644
> index 000000000000..a5adebdda941
> --- /dev/null
> +++ b/drivers/media/platform/vimc/vimc.h
I was wondering if instead of creating a vimc.h, if we could move
to vimc-common.h,just because it is not clear to me the difference
between the two now, what do you think?
Regards,
Helen
> @@ -0,0 +1,102 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + *
> + * Copyright (C) 2019 Shuah Khan <[email protected]>
> + *
> + */
> +
> +/*
> + * This file defines vimc driver device and sub-device structures.
> + */
> +
> +#ifndef _VIMC_H_
> +#define _VIMC_H_
> +
> +#include <linux/platform_device.h>
> +#include <media/media-device.h>
> +#include <media/v4l2-device.h>
> +#include <media/videobuf2-core.h>
> +#include <media/tpg/v4l2-tpg.h>
> +#include <media/v4l2-ctrls.h>
> +
> +#include "vimc-common.h"
> +
> +struct vimc_cap_device {
> + struct vimc_ent_device ved;
> + struct video_device vdev;
> + struct device *dev;
> + struct v4l2_pix_format format;
> + struct vb2_queue queue;
> + struct list_head buf_list;
> + /*
> + * NOTE: in a real driver, a spin lock must be used to access the
> + * queue because the frames are generated from a hardware interruption
> + * and the isr is not allowed to sleep.
> + * Even if it is not necessary a spinlock in the vimc driver, we
> + * use it here as a code reference
> + */
> + spinlock_t qlock;
> + struct mutex lock;
> + u32 sequence;
> + struct vimc_stream stream;
> +};
> +
> +struct vimc_sca_device {
> + struct vimc_ent_device ved;
> + struct v4l2_subdev sd;
> + struct device *dev;
> + /* NOTE: the source fmt is the same as the sink
> + * with the width and hight multiplied by mult
> + */
> + struct v4l2_mbus_framefmt sink_fmt;
> + /* Values calculated when the stream starts */
> + u8 *src_frame;
> + unsigned int src_line_size;
> + unsigned int bpp;
> +};
> +
> +struct vimc_deb_device {
> + struct vimc_ent_device ved;
> + struct v4l2_subdev sd;
> + struct device *dev;
> + /* The active format */
> + struct v4l2_mbus_framefmt sink_fmt;
> + u32 src_code;
> + void (*set_rgb_src)(struct vimc_deb_device *vdeb, unsigned int lin,
> + unsigned int col, unsigned int rgb[3]);
> + /* Values calculated when the stream starts */
> + u8 *src_frame;
> + const struct vimc_deb_pix_map *sink_pix_map;
> + unsigned int sink_bpp;
> +};
> +
> +struct vimc_sen_device {
> + struct vimc_ent_device ved;
> + struct v4l2_subdev sd;
> + struct device *dev;
> + struct tpg_data tpg;
> + struct task_struct *kthread_sen;
> + u8 *frame;
> + /* The active format */
> + struct v4l2_mbus_framefmt mbus_format;
> + struct v4l2_ctrl_handler hdl;
> +};
> +
> +struct vimc_device {
> + /* The platform device */
> + struct platform_device pdev;
> +
> + /* The pipeline configuration */
> + const struct vimc_pipeline_config *pipe_cfg;
> +
> + /* The Associated media_device parent */
> + struct media_device mdev;
> +
> + /* Internal v4l2 parent device*/
> + struct v4l2_device v4l2_dev;
> +
> + /* Subdevices */
> + struct platform_device **subdevs;
> +};
> +
> +#endif
>
Hi Shuah,
Thank you for the patch.
On Fri, Aug 09, 2019 at 03:45:41PM -0600, Shuah Khan wrote:
> In preparation for collapsing the component driver structure into
> a monolith, move private device structure defines to a new common
> header file.
Apart from the vimc_device structure, this doesn't seem to be needed.
I'd rather keep each structure private to the .c file that handles it,
and only share vimc_device globally.
> Signed-off-by: Shuah Khan <[email protected]>
> ---
> drivers/media/platform/vimc/vimc-capture.c | 21 +----
> drivers/media/platform/vimc/vimc-core.c | 18 +---
> drivers/media/platform/vimc/vimc-debayer.c | 16 +---
> drivers/media/platform/vimc/vimc-scaler.c | 15 +--
> drivers/media/platform/vimc/vimc-sensor.c | 13 +--
> drivers/media/platform/vimc/vimc.h | 102 +++++++++++++++++++++
> 6 files changed, 107 insertions(+), 78 deletions(-)
> create mode 100644 drivers/media/platform/vimc/vimc.h
>
> diff --git a/drivers/media/platform/vimc/vimc-capture.c b/drivers/media/platform/vimc/vimc-capture.c
> index 664855708fdf..c52fc5d97c2d 100644
> --- a/drivers/media/platform/vimc/vimc-capture.c
> +++ b/drivers/media/platform/vimc/vimc-capture.c
> @@ -13,6 +13,7 @@
> #include <media/videobuf2-core.h>
> #include <media/videobuf2-vmalloc.h>
>
> +#include "vimc.h"
> #include "vimc-common.h"
> #include "vimc-streamer.h"
>
> @@ -44,26 +45,6 @@ static const u32 vimc_cap_supported_pixfmt[] = {
> V4L2_PIX_FMT_SRGGB12,
> };
>
> -struct vimc_cap_device {
> - struct vimc_ent_device ved;
> - struct video_device vdev;
> - struct device *dev;
> - struct v4l2_pix_format format;
> - struct vb2_queue queue;
> - struct list_head buf_list;
> - /*
> - * NOTE: in a real driver, a spin lock must be used to access the
> - * queue because the frames are generated from a hardware interruption
> - * and the isr is not allowed to sleep.
> - * Even if it is not necessary a spinlock in the vimc driver, we
> - * use it here as a code reference
> - */
> - spinlock_t qlock;
> - struct mutex lock;
> - u32 sequence;
> - struct vimc_stream stream;
> -};
> -
> static const struct v4l2_pix_format fmt_default = {
> .width = 640,
> .height = 480,
> diff --git a/drivers/media/platform/vimc/vimc-core.c b/drivers/media/platform/vimc/vimc-core.c
> index 571c55aa0e16..c9b351472272 100644
> --- a/drivers/media/platform/vimc/vimc-core.c
> +++ b/drivers/media/platform/vimc/vimc-core.c
> @@ -12,6 +12,7 @@
> #include <media/media-device.h>
> #include <media/v4l2-device.h>
>
> +#include "vimc.h"
> #include "vimc-common.h"
>
> #define VIMC_MDEV_MODEL_NAME "VIMC MDEV"
> @@ -24,23 +25,6 @@
> .flags = link_flags, \
> }
>
> -struct vimc_device {
> - /* The platform device */
> - struct platform_device pdev;
> -
> - /* The pipeline configuration */
> - const struct vimc_pipeline_config *pipe_cfg;
> -
> - /* The Associated media_device parent */
> - struct media_device mdev;
> -
> - /* Internal v4l2 parent device*/
> - struct v4l2_device v4l2_dev;
> -
> - /* Subdevices */
> - struct platform_device **subdevs;
> -};
> -
> /* Structure which describes individual configuration for each entity */
> struct vimc_ent_config {
> const char *name;
> diff --git a/drivers/media/platform/vimc/vimc-debayer.c b/drivers/media/platform/vimc/vimc-debayer.c
> index 00598fbf3cba..750752bb173c 100644
> --- a/drivers/media/platform/vimc/vimc-debayer.c
> +++ b/drivers/media/platform/vimc/vimc-debayer.c
> @@ -13,6 +13,7 @@
> #include <linux/v4l2-mediabus.h>
> #include <media/v4l2-subdev.h>
>
> +#include "vimc.h"
> #include "vimc-common.h"
>
> #define VIMC_DEB_DRV_NAME "vimc-debayer"
> @@ -44,21 +45,6 @@ struct vimc_deb_pix_map {
> enum vimc_deb_rgb_colors order[2][2];
> };
>
> -struct vimc_deb_device {
> - struct vimc_ent_device ved;
> - struct v4l2_subdev sd;
> - struct device *dev;
> - /* The active format */
> - struct v4l2_mbus_framefmt sink_fmt;
> - u32 src_code;
> - void (*set_rgb_src)(struct vimc_deb_device *vdeb, unsigned int lin,
> - unsigned int col, unsigned int rgb[3]);
> - /* Values calculated when the stream starts */
> - u8 *src_frame;
> - const struct vimc_deb_pix_map *sink_pix_map;
> - unsigned int sink_bpp;
> -};
> -
> static const struct v4l2_mbus_framefmt sink_fmt_default = {
> .width = 640,
> .height = 480,
> diff --git a/drivers/media/platform/vimc/vimc-scaler.c b/drivers/media/platform/vimc/vimc-scaler.c
> index c7123a45c55b..fe99b9102ada 100644
> --- a/drivers/media/platform/vimc/vimc-scaler.c
> +++ b/drivers/media/platform/vimc/vimc-scaler.c
> @@ -13,6 +13,7 @@
> #include <linux/v4l2-mediabus.h>
> #include <media/v4l2-subdev.h>
>
> +#include "vimc.h"
> #include "vimc-common.h"
>
> #define VIMC_SCA_DRV_NAME "vimc-scaler"
> @@ -31,20 +32,6 @@ static const u32 vimc_sca_supported_pixfmt[] = {
> V4L2_PIX_FMT_ARGB32,
> };
>
> -struct vimc_sca_device {
> - struct vimc_ent_device ved;
> - struct v4l2_subdev sd;
> - struct device *dev;
> - /* NOTE: the source fmt is the same as the sink
> - * with the width and hight multiplied by mult
> - */
> - struct v4l2_mbus_framefmt sink_fmt;
> - /* Values calculated when the stream starts */
> - u8 *src_frame;
> - unsigned int src_line_size;
> - unsigned int bpp;
> -};
> -
> static const struct v4l2_mbus_framefmt sink_fmt_default = {
> .width = 640,
> .height = 480,
> diff --git a/drivers/media/platform/vimc/vimc-sensor.c b/drivers/media/platform/vimc/vimc-sensor.c
> index 51359472eef2..6c57b1e262f9 100644
> --- a/drivers/media/platform/vimc/vimc-sensor.c
> +++ b/drivers/media/platform/vimc/vimc-sensor.c
> @@ -16,22 +16,11 @@
> #include <media/v4l2-subdev.h>
> #include <media/tpg/v4l2-tpg.h>
>
> +#include "vimc.h"
> #include "vimc-common.h"
>
> #define VIMC_SEN_DRV_NAME "vimc-sensor"
>
> -struct vimc_sen_device {
> - struct vimc_ent_device ved;
> - struct v4l2_subdev sd;
> - struct device *dev;
> - struct tpg_data tpg;
> - struct task_struct *kthread_sen;
> - u8 *frame;
> - /* The active format */
> - struct v4l2_mbus_framefmt mbus_format;
> - struct v4l2_ctrl_handler hdl;
> -};
> -
> static const struct v4l2_mbus_framefmt fmt_default = {
> .width = 640,
> .height = 480,
> diff --git a/drivers/media/platform/vimc/vimc.h b/drivers/media/platform/vimc/vimc.h
> new file mode 100644
> index 000000000000..a5adebdda941
> --- /dev/null
> +++ b/drivers/media/platform/vimc/vimc.h
> @@ -0,0 +1,102 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + *
> + * Copyright (C) 2019 Shuah Khan <[email protected]>
> + *
> + */
> +
> +/*
> + * This file defines vimc driver device and sub-device structures.
> + */
> +
> +#ifndef _VIMC_H_
> +#define _VIMC_H_
> +
> +#include <linux/platform_device.h>
> +#include <media/media-device.h>
> +#include <media/v4l2-device.h>
> +#include <media/videobuf2-core.h>
> +#include <media/tpg/v4l2-tpg.h>
> +#include <media/v4l2-ctrls.h>
> +
> +#include "vimc-common.h"
> +
> +struct vimc_cap_device {
> + struct vimc_ent_device ved;
> + struct video_device vdev;
> + struct device *dev;
> + struct v4l2_pix_format format;
> + struct vb2_queue queue;
> + struct list_head buf_list;
> + /*
> + * NOTE: in a real driver, a spin lock must be used to access the
> + * queue because the frames are generated from a hardware interruption
> + * and the isr is not allowed to sleep.
> + * Even if it is not necessary a spinlock in the vimc driver, we
> + * use it here as a code reference
> + */
> + spinlock_t qlock;
> + struct mutex lock;
> + u32 sequence;
> + struct vimc_stream stream;
> +};
> +
> +struct vimc_sca_device {
> + struct vimc_ent_device ved;
> + struct v4l2_subdev sd;
> + struct device *dev;
> + /* NOTE: the source fmt is the same as the sink
> + * with the width and hight multiplied by mult
> + */
> + struct v4l2_mbus_framefmt sink_fmt;
> + /* Values calculated when the stream starts */
> + u8 *src_frame;
> + unsigned int src_line_size;
> + unsigned int bpp;
> +};
> +
> +struct vimc_deb_device {
> + struct vimc_ent_device ved;
> + struct v4l2_subdev sd;
> + struct device *dev;
> + /* The active format */
> + struct v4l2_mbus_framefmt sink_fmt;
> + u32 src_code;
> + void (*set_rgb_src)(struct vimc_deb_device *vdeb, unsigned int lin,
> + unsigned int col, unsigned int rgb[3]);
> + /* Values calculated when the stream starts */
> + u8 *src_frame;
> + const struct vimc_deb_pix_map *sink_pix_map;
> + unsigned int sink_bpp;
> +};
> +
> +struct vimc_sen_device {
> + struct vimc_ent_device ved;
> + struct v4l2_subdev sd;
> + struct device *dev;
> + struct tpg_data tpg;
> + struct task_struct *kthread_sen;
> + u8 *frame;
> + /* The active format */
> + struct v4l2_mbus_framefmt mbus_format;
> + struct v4l2_ctrl_handler hdl;
> +};
> +
> +struct vimc_device {
> + /* The platform device */
> + struct platform_device pdev;
> +
> + /* The pipeline configuration */
> + const struct vimc_pipeline_config *pipe_cfg;
> +
> + /* The Associated media_device parent */
> + struct media_device mdev;
> +
> + /* Internal v4l2 parent device*/
> + struct v4l2_device v4l2_dev;
> +
> + /* Subdevices */
> + struct platform_device **subdevs;
> +};
> +
> +#endif
--
Regards,
Laurent Pinchart
Hi Helen,
On 8/9/19 9:15 PM, Helen Koike wrote:
> Hi Shuah,
>
> Thanks for the patch.
>
> On 8/9/19 6:45 PM, Shuah Khan wrote:
>> In preparation for collapsing the component driver structure into
>> a monolith, move private device structure defines to a new common
>> header file.
>>
>> Signed-off-by: Shuah Khan <[email protected]>
>> ---
>> drivers/media/platform/vimc/vimc-capture.c | 21 +----
>> drivers/media/platform/vimc/vimc-core.c | 18 +---
>> drivers/media/platform/vimc/vimc-debayer.c | 16 +---
>> drivers/media/platform/vimc/vimc-scaler.c | 15 +--
>> drivers/media/platform/vimc/vimc-sensor.c | 13 +--
>> drivers/media/platform/vimc/vimc.h | 102 +++++++++++++++++++++
>> 6 files changed, 107 insertions(+), 78 deletions(-)
>> create mode 100644 drivers/media/platform/vimc/vimc.h
>>
>> diff --git a/drivers/media/platform/vimc/vimc-capture.c b/drivers/media/platform/vimc/vimc-capture.c
>> index 664855708fdf..c52fc5d97c2d 100644
>> --- a/drivers/media/platform/vimc/vimc-capture.c
>> +++ b/drivers/media/platform/vimc/vimc-capture.c
>> @@ -13,6 +13,7 @@
>> #include <media/videobuf2-core.h>
>> #include <media/videobuf2-vmalloc.h>
>>
>> +#include "vimc.h"
>> #include "vimc-common.h"
>> #include "vimc-streamer.h"
>>
>> @@ -44,26 +45,6 @@ static const u32 vimc_cap_supported_pixfmt[] = {
>> V4L2_PIX_FMT_SRGGB12,
>> };
>>
>> -struct vimc_cap_device {
>> - struct vimc_ent_device ved;
>> - struct video_device vdev;
>> - struct device *dev;
>> - struct v4l2_pix_format format;
>> - struct vb2_queue queue;
>> - struct list_head buf_list;
>> - /*
>> - * NOTE: in a real driver, a spin lock must be used to access the
>> - * queue because the frames are generated from a hardware interruption
>> - * and the isr is not allowed to sleep.
>> - * Even if it is not necessary a spinlock in the vimc driver, we
>> - * use it here as a code reference
>> - */
>> - spinlock_t qlock;
>> - struct mutex lock;
>> - u32 sequence;
>> - struct vimc_stream stream;
>> -};
>> -
>> static const struct v4l2_pix_format fmt_default = {
>> .width = 640,
>> .height = 480,
>> diff --git a/drivers/media/platform/vimc/vimc-core.c b/drivers/media/platform/vimc/vimc-core.c
>> index 571c55aa0e16..c9b351472272 100644
>> --- a/drivers/media/platform/vimc/vimc-core.c
>> +++ b/drivers/media/platform/vimc/vimc-core.c
>> @@ -12,6 +12,7 @@
>> #include <media/media-device.h>
>> #include <media/v4l2-device.h>
>>
>> +#include "vimc.h"
>> #include "vimc-common.h"
>>
>> #define VIMC_MDEV_MODEL_NAME "VIMC MDEV"
>> @@ -24,23 +25,6 @@
>> .flags = link_flags, \
>> }
>>
>> -struct vimc_device {
>> - /* The platform device */
>> - struct platform_device pdev;
>> -
>> - /* The pipeline configuration */
>> - const struct vimc_pipeline_config *pipe_cfg;
>> -
>> - /* The Associated media_device parent */
>> - struct media_device mdev;
>> -
>> - /* Internal v4l2 parent device*/
>> - struct v4l2_device v4l2_dev;
>> -
>> - /* Subdevices */
>> - struct platform_device **subdevs;
>> -};
>> -
>> /* Structure which describes individual configuration for each entity */
>> struct vimc_ent_config {
>> const char *name;
>> diff --git a/drivers/media/platform/vimc/vimc-debayer.c b/drivers/media/platform/vimc/vimc-debayer.c
>> index 00598fbf3cba..750752bb173c 100644
>> --- a/drivers/media/platform/vimc/vimc-debayer.c
>> +++ b/drivers/media/platform/vimc/vimc-debayer.c
>> @@ -13,6 +13,7 @@
>> #include <linux/v4l2-mediabus.h>
>> #include <media/v4l2-subdev.h>
>>
>> +#include "vimc.h"
>> #include "vimc-common.h"
>>
>> #define VIMC_DEB_DRV_NAME "vimc-debayer"
>> @@ -44,21 +45,6 @@ struct vimc_deb_pix_map {
>> enum vimc_deb_rgb_colors order[2][2];
>> };
>>
>> -struct vimc_deb_device {
>> - struct vimc_ent_device ved;
>> - struct v4l2_subdev sd;
>> - struct device *dev;
>> - /* The active format */
>> - struct v4l2_mbus_framefmt sink_fmt;
>> - u32 src_code;
>> - void (*set_rgb_src)(struct vimc_deb_device *vdeb, unsigned int lin,
>> - unsigned int col, unsigned int rgb[3]);
>> - /* Values calculated when the stream starts */
>> - u8 *src_frame;
>> - const struct vimc_deb_pix_map *sink_pix_map;
>> - unsigned int sink_bpp;
>> -};
>> -
>> static const struct v4l2_mbus_framefmt sink_fmt_default = {
>> .width = 640,
>> .height = 480,
>> diff --git a/drivers/media/platform/vimc/vimc-scaler.c b/drivers/media/platform/vimc/vimc-scaler.c
>> index c7123a45c55b..fe99b9102ada 100644
>> --- a/drivers/media/platform/vimc/vimc-scaler.c
>> +++ b/drivers/media/platform/vimc/vimc-scaler.c
>> @@ -13,6 +13,7 @@
>> #include <linux/v4l2-mediabus.h>
>> #include <media/v4l2-subdev.h>
>>
>> +#include "vimc.h"
>> #include "vimc-common.h"
>>
>> #define VIMC_SCA_DRV_NAME "vimc-scaler"
>> @@ -31,20 +32,6 @@ static const u32 vimc_sca_supported_pixfmt[] = {
>> V4L2_PIX_FMT_ARGB32,
>> };
>>
>> -struct vimc_sca_device {
>> - struct vimc_ent_device ved;
>> - struct v4l2_subdev sd;
>> - struct device *dev;
>> - /* NOTE: the source fmt is the same as the sink
>> - * with the width and hight multiplied by mult
>> - */
>> - struct v4l2_mbus_framefmt sink_fmt;
>> - /* Values calculated when the stream starts */
>> - u8 *src_frame;
>> - unsigned int src_line_size;
>> - unsigned int bpp;
>> -};
>> -
>> static const struct v4l2_mbus_framefmt sink_fmt_default = {
>> .width = 640,
>> .height = 480,
>> diff --git a/drivers/media/platform/vimc/vimc-sensor.c b/drivers/media/platform/vimc/vimc-sensor.c
>> index 51359472eef2..6c57b1e262f9 100644
>> --- a/drivers/media/platform/vimc/vimc-sensor.c
>> +++ b/drivers/media/platform/vimc/vimc-sensor.c
>> @@ -16,22 +16,11 @@
>> #include <media/v4l2-subdev.h>
>> #include <media/tpg/v4l2-tpg.h>
>>
>> +#include "vimc.h"
>> #include "vimc-common.h"
>>
>> #define VIMC_SEN_DRV_NAME "vimc-sensor"
>>
>> -struct vimc_sen_device {
>> - struct vimc_ent_device ved;
>> - struct v4l2_subdev sd;
>> - struct device *dev;
>> - struct tpg_data tpg;
>> - struct task_struct *kthread_sen;
>> - u8 *frame;
>> - /* The active format */
>> - struct v4l2_mbus_framefmt mbus_format;
>> - struct v4l2_ctrl_handler hdl;
>> -};
>> -
>> static const struct v4l2_mbus_framefmt fmt_default = {
>> .width = 640,
>> .height = 480,
>> diff --git a/drivers/media/platform/vimc/vimc.h b/drivers/media/platform/vimc/vimc.h
>> new file mode 100644
>> index 000000000000..a5adebdda941
>> --- /dev/null
>> +++ b/drivers/media/platform/vimc/vimc.h
>
> I was wondering if instead of creating a vimc.h, if we could move
> to vimc-common.h,just because it is not clear to me the difference
> between the two now, what do you think?
>
My thinking is that vimc-common.h is common for all the subdevs and
putting vimc-core defines and structures it shares it with the subdev
files can be in a separate file.
It is more of design choice to keep structures and defined organized.
Originally I was thinking all the subdev device structires need to be
global, and my patch set I sent out as such doesn't need that. I just
overlooked that when I sent the patches out.
This reduces the number of things that need to be common, I don't really
have any strong reasons for either choice of adding common defines to
vimc-common.h vs vimc.h - maybe with a slight tilt towards liking vimc.h
thanks,
-- Shuah
Hi Laurent,
On 8/10/19 8:14 AM, Laurent Pinchart wrote:
> Hi Shuah,
>
> Thank you for the patch.
>
> On Fri, Aug 09, 2019 at 03:45:41PM -0600, Shuah Khan wrote:
>> In preparation for collapsing the component driver structure into
>> a monolith, move private device structure defines to a new common
>> header file.
>
> Apart from the vimc_device structure, this doesn't seem to be needed.
> I'd rather keep each structure private to the .c file that handles it,
> and only share vimc_device globally.
>
Right. I initially thought that I needed these global. Once I completed
the patches without needing these as global, I overlooked updating the
patches.
I will take care of that. Any thoughts on vimc.h vs. adding vimc_device
struct to existing vimc-common.h
As I explained to Helen in response to her comment about:
"My thinking is that vimc-common.h is common for all the subdevs and
putting vimc-core defines and structures it shares it with the subdev
files can be in a separate file.
It is more of design choice to keep structures and defined organized.
Originally I was thinking all the subdev device structires need to be
global, and my patch set I sent out as such doesn't need that. I just
overlooked that when I sent the patches out.
This reduces the number of things that need to be common, I don't really
have any strong reasons for either choice of adding common defines to
vimc-common.h vs vimc.h - maybe with a slight tilt towards vimc.h"
Thanks all for a quick review and testing. I will work on v2 with your
comments. I want to make sure topology either looks the same as what
is in media master. I think it is, but I want to double check.
thanks,
-- Shuah
Hi Shua,
On Mon, Aug 12, 2019 at 08:19:27AM -0600, Shuah Khan wrote:
> On 8/10/19 8:14 AM, Laurent Pinchart wrote:
> > On Fri, Aug 09, 2019 at 03:45:41PM -0600, Shuah Khan wrote:
> >> In preparation for collapsing the component driver structure into
> >> a monolith, move private device structure defines to a new common
> >> header file.
> >
> > Apart from the vimc_device structure, this doesn't seem to be needed.
> > I'd rather keep each structure private to the .c file that handles it,
> > and only share vimc_device globally.
>
> Right. I initially thought that I needed these global. Once I completed
> the patches without needing these as global, I overlooked updating the
> patches.
>
> I will take care of that. Any thoughts on vimc.h vs. adding vimc_device
> struct to existing vimc-common.h
>
> As I explained to Helen in response to her comment about:
>
> "My thinking is that vimc-common.h is common for all the subdevs and
> putting vimc-core defines and structures it shares it with the subdev
> files can be in a separate file.
>
> It is more of design choice to keep structures and defined organized.
> Originally I was thinking all the subdev device structires need to be
> global, and my patch set I sent out as such doesn't need that. I just
> overlooked that when I sent the patches out.
>
> This reduces the number of things that need to be common, I don't really
> have any strong reasons for either choice of adding common defines to
> vimc-common.h vs vimc.h - maybe with a slight tilt towards vimc.h"
The vimc_device structure fits nicely in vimc-common.h in my opinion, as
it's used by every component. I don't care much either way.
> Thanks all for a quick review and testing. I will work on v2 with your
> comments. I want to make sure topology either looks the same as what
> is in media master. I think it is, but I want to double check.
--
Regards,
Laurent Pinchart
On 8/12/19 8:24 AM, Laurent Pinchart wrote:
> Hi Shua,
>
> On Mon, Aug 12, 2019 at 08:19:27AM -0600, Shuah Khan wrote:
>> On 8/10/19 8:14 AM, Laurent Pinchart wrote:
>>> On Fri, Aug 09, 2019 at 03:45:41PM -0600, Shuah Khan wrote:
>>>> In preparation for collapsing the component driver structure into
>>>> a monolith, move private device structure defines to a new common
>>>> header file.
>>>
>>> Apart from the vimc_device structure, this doesn't seem to be needed.
>>> I'd rather keep each structure private to the .c file that handles it,
>>> and only share vimc_device globally.
>>
>> Right. I initially thought that I needed these global. Once I completed
>> the patches without needing these as global, I overlooked updating the
>> patches.
>>
>> I will take care of that. Any thoughts on vimc.h vs. adding vimc_device
>> struct to existing vimc-common.h
>>
>> As I explained to Helen in response to her comment about:
>>
>> "My thinking is that vimc-common.h is common for all the subdevs and
>> putting vimc-core defines and structures it shares it with the subdev
>> files can be in a separate file.
>>
>> It is more of design choice to keep structures and defined organized.
>> Originally I was thinking all the subdev device structires need to be
>> global, and my patch set I sent out as such doesn't need that. I just
>> overlooked that when I sent the patches out.
>>
>> This reduces the number of things that need to be common, I don't really
>> have any strong reasons for either choice of adding common defines to
>> vimc-common.h vs vimc.h - maybe with a slight tilt towards vimc.h"
>
> The vimc_device structure fits nicely in vimc-common.h in my opinion, as
> it's used by every component. I don't care much either way.
>
Sounds good to me.
thanks,
-- Shuah