Linux-Media Archive on lore.kernel.org
 help / color / Atom feed
From: Shuah Khan <skhan@linuxfoundation.org>
To: Helen Koike <helen.koike@collabora.com>,
	mchehab@kernel.org, hverkuil@xs4all.nl,
	laurent.pinchart@ideasonboard.com
Cc: linux-kernel@vger.kernel.org, linux-media@vger.kernel.org,
	"skh >> Shuah Khan" <skhan@linuxfoundation.org>
Subject: Re: [PATCH 1/3] media: vimc: move private defines to a common header
Date: Mon, 12 Aug 2019 08:03:53 -0600
Message-ID: <a0067165-85b5-7093-9660-445eb69b9578@linuxfoundation.org> (raw)
In-Reply-To: <5f9df1e2-41d6-b301-a841-0670f56464e3@collabora.com>

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@linuxfoundation.org>
>> ---
>>   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


  reply index

Thread overview: 27+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-08-09 21:45 [PATCH 0/3] Collapse vimc into single monolithic driver Shuah Khan
2019-08-09 21:45 ` [PATCH 1/3] media: vimc: move private defines to a common header Shuah Khan
2019-08-10  3:15   ` Helen Koike
2019-08-12 14:03     ` Shuah Khan [this message]
2019-08-10 14:14   ` Laurent Pinchart
2019-08-12 14:19     ` Shuah Khan
2019-08-12 14:24       ` Laurent Pinchart
2019-08-12 14:27         ` Shuah Khan
2019-08-09 21:45 ` [PATCH 2/3] media: vimc: Collapse component structure into a single monolithic driver Shuah Khan
2019-08-10  4:12   ` Helen Koike
2019-08-12 14:12     ` Shuah Khan
2019-08-09 21:45 ` [PATCH 3/3] media: vimc: Fix gpf in rmmod path when stream is active Shuah Khan
2019-08-09 23:52 ` [PATCH 0/3] Collapse vimc into single monolithic driver André Almeida
2019-08-10  0:17   ` Shuah Khan
2019-08-10  0:24     ` André Almeida
2019-08-10  0:48       ` Shuah Khan
2019-08-10  3:51       ` Helen Koike
2019-08-12 14:08         ` Shuah Khan
2019-08-12 18:52           ` André Almeida
2019-08-12 19:10             ` Shuah Khan
2019-08-12 22:14               ` Shuah Khan
2019-08-12 23:41                 ` Helen Koike
2019-08-13  0:58                   ` Shuah Khan
2019-08-13  9:56                   ` Laurent Pinchart
2019-08-13 12:25                     ` Helen Koike
2019-08-13 12:36                       ` Laurent Pinchart
2019-08-13 23:22                         ` Shuah Khan

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=a0067165-85b5-7093-9660-445eb69b9578@linuxfoundation.org \
    --to=skhan@linuxfoundation.org \
    --cc=helen.koike@collabora.com \
    --cc=hverkuil@xs4all.nl \
    --cc=laurent.pinchart@ideasonboard.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-media@vger.kernel.org \
    --cc=mchehab@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-Media Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-media/0 linux-media/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-media linux-media/ https://lore.kernel.org/linux-media \
		linux-media@vger.kernel.org
	public-inbox-index linux-media

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-media


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git