Re: [PATCH 1/3] media: vimc: move private defines to a common header

[Shuah Khan <skhan@xxxxxxxxxxxxxxxxxxx>]

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 <skhan@xxxxxxxxxxxxxxxxxxx>
> > ---
> >   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