lkml.org 
[lkml]   [2019]   [Aug]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH 1/3] media: vimc: move private defines to a common header
From
Date
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

\
 
 \ /
  Last update: 2019-08-12 16:04    [W:1.587 / U:0.100 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site