Messages in this thread | | | Subject | Re: [PATCH 1/3] media: vimc: move private defines to a common header | From | Shuah Khan <> | Date | Mon, 12 Aug 2019 08:03:53 -0600 |
| |
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
| |