Re: [PATCH 09/15] media: soc_camera pad-aware driver initialisation

2015-06-14 Thread Guennadi Liakhovetski
Hi William,

On Wed, 3 Jun 2015, William Towle wrote:

 Add detection of source pad number for drivers aware of the media
 controller API, so that the combination of soc_camera and rcar_vin
 can create device nodes to support modern drivers such as adv7604.c
 (for HDMI on Lager) and the converted adv7180.c (for composite)
 underneath.
 
 Building rcar_vin gains a dependency on CONFIG_MEDIA_CONTROLLER, in
 line with requirements for building the drivers associated with it.
 
 Signed-off-by: William Towle william.to...@codethink.co.uk
 Signed-off-by: Rob Taylor rob.tay...@codethink.co.uk
 ---
  drivers/media/platform/soc_camera/Kconfig  |1 +
  drivers/media/platform/soc_camera/rcar_vin.c   |1 +
  drivers/media/platform/soc_camera/soc_camera.c |   46 
 
  include/media/soc_camera.h |1 +
  4 files changed, 49 insertions(+)
 
 diff --git a/drivers/media/platform/soc_camera/Kconfig 
 b/drivers/media/platform/soc_camera/Kconfig
 index f2776cd..5c45c83 100644
 --- a/drivers/media/platform/soc_camera/Kconfig
 +++ b/drivers/media/platform/soc_camera/Kconfig
 @@ -38,6 +38,7 @@ config VIDEO_RCAR_VIN
   depends on VIDEO_DEV  SOC_CAMERA
   depends on ARCH_SHMOBILE || COMPILE_TEST
   depends on HAS_DMA
 + depends on MEDIA_CONTROLLER
   select VIDEOBUF2_DMA_CONTIG
   select SOC_CAMERA_SCALE_CROP
   ---help---
 diff --git a/drivers/media/platform/soc_camera/rcar_vin.c 
 b/drivers/media/platform/soc_camera/rcar_vin.c
 index 16352a8..00c1034 100644
 --- a/drivers/media/platform/soc_camera/rcar_vin.c
 +++ b/drivers/media/platform/soc_camera/rcar_vin.c
 @@ -1359,6 +1359,7 @@ static int rcar_vin_get_formats(struct 
 soc_camera_device *icd, unsigned int idx,
   struct device *dev = icd-parent;
   int shift;
  
 + fmt.pad = icd-src_pad_idx;
   ret = v4l2_subdev_call(sd, pad, get_fmt, NULL, fmt);
   if (ret  0)
   return ret;
 diff --git a/drivers/media/platform/soc_camera/soc_camera.c 
 b/drivers/media/platform/soc_camera/soc_camera.c
 index d708df4..c4952c8 100644
 --- a/drivers/media/platform/soc_camera/soc_camera.c
 +++ b/drivers/media/platform/soc_camera/soc_camera.c
 @@ -1293,6 +1293,9 @@ static int soc_camera_probe_finish(struct 
 soc_camera_device *icd)
   .which = V4L2_SUBDEV_FORMAT_ACTIVE,
   };
   struct v4l2_mbus_framefmt *mf = fmt.format;
 +#if defined(CONFIG_MEDIA_CONTROLLER)
 + struct media_pad pad;
 +#endif
   int ret;
  
   sd-grp_id = soc_camera_grp_id(icd);
 @@ -1310,8 +1313,40 @@ static int soc_camera_probe_finish(struct 
 soc_camera_device *icd)
   return ret;
   }
  
 + icd-src_pad_idx = -1;
 +#if defined(CONFIG_MEDIA_CONTROLLER)
   /* At this point client .probe() should have run already */

Please, leave this comment outside of #ifdef.

 + ret = media_entity_init(icd-vdev-entity, 1, pad, 0);
 + if (ret  0) {
 + goto eusrfmt;
 + } else {

You don't need this else here. Just put pad_idx declaration under 
#ifdef above.

 + int pad_idx;
 +
 + for (pad_idx = 0; pad_idx  sd-entity.num_pads; pad_idx++)
 + if (sd-entity.pads[pad_idx].flags
 + == MEDIA_PAD_FL_SOURCE)
 + break;
 + if (pad_idx = sd-entity.num_pads)
 + goto eusrfmt;
 +
 + ret = media_entity_create_link(icd-vdev-entity, 0,
 + sd-entity, pad_idx,
 + MEDIA_LNK_FL_IMMUTABLE |
 + MEDIA_LNK_FL_ENABLED);
 + if (ret  0)
 + goto eusrfmt;
 +
 + icd-src_pad_idx = pad_idx;
 + ret = soc_camera_init_user_formats(icd);

I don't think you need soc_camera_init_user_formats() twice - under ifdef 
and outside of it. Just add icd-src_pad_idx = -1; in the error case 
below:

 + if (ret  0) {
 + icd-src_pad_idx = -1;
 + goto eusrfmt;
 + }
 + }
 +#else
   ret = soc_camera_init_user_formats(icd);
 +#endif
 +
   if (ret  0)
   goto eusrfmt;
  
 @@ -1322,6 +1357,9 @@ static int soc_camera_probe_finish(struct 
 soc_camera_device *icd)
   goto evidstart;
  
   /* Try to improve our guess of a reasonable window format */
 +#if defined(CONFIG_MEDIA_CONTROLLER)
 + fmt.pad = icd-src_pad_idx;
 +#endif
   if (!v4l2_subdev_call(sd, pad, get_fmt, NULL, fmt)) {
   icd-user_width = mf-width;
   icd-user_height= mf-height;
 @@ -1335,6 +1373,9 @@ static int soc_camera_probe_finish(struct 
 soc_camera_device *icd)
  evidstart:
   soc_camera_free_user_formats(icd);
  eusrfmt:
 +#if defined(CONFIG_MEDIA_CONTROLLER)

+   icd-src_pad_idx = -1;

 + 

Re: [PATCH 09/15] media: soc_camera pad-aware driver initialisation

2015-06-12 Thread Hans Verkuil
On 06/03/2015 03:59 PM, William Towle wrote:
 Add detection of source pad number for drivers aware of the media
 controller API, so that the combination of soc_camera and rcar_vin
 can create device nodes to support modern drivers such as adv7604.c
 (for HDMI on Lager) and the converted adv7180.c (for composite)
 underneath.
 
 Building rcar_vin gains a dependency on CONFIG_MEDIA_CONTROLLER, in
 line with requirements for building the drivers associated with it.
 
 Signed-off-by: William Towle william.to...@codethink.co.uk
 Signed-off-by: Rob Taylor rob.tay...@codethink.co.uk
 ---
  drivers/media/platform/soc_camera/Kconfig  |1 +
  drivers/media/platform/soc_camera/rcar_vin.c   |1 +
  drivers/media/platform/soc_camera/soc_camera.c |   46 
 
  include/media/soc_camera.h |1 +
  4 files changed, 49 insertions(+)
 
 diff --git a/drivers/media/platform/soc_camera/Kconfig 
 b/drivers/media/platform/soc_camera/Kconfig
 index f2776cd..5c45c83 100644
 --- a/drivers/media/platform/soc_camera/Kconfig
 +++ b/drivers/media/platform/soc_camera/Kconfig
 @@ -38,6 +38,7 @@ config VIDEO_RCAR_VIN
   depends on VIDEO_DEV  SOC_CAMERA
   depends on ARCH_SHMOBILE || COMPILE_TEST
   depends on HAS_DMA
 + depends on MEDIA_CONTROLLER
   select VIDEOBUF2_DMA_CONTIG
   select SOC_CAMERA_SCALE_CROP
   ---help---
 diff --git a/drivers/media/platform/soc_camera/rcar_vin.c 
 b/drivers/media/platform/soc_camera/rcar_vin.c
 index 16352a8..00c1034 100644
 --- a/drivers/media/platform/soc_camera/rcar_vin.c
 +++ b/drivers/media/platform/soc_camera/rcar_vin.c
 @@ -1359,6 +1359,7 @@ static int rcar_vin_get_formats(struct 
 soc_camera_device *icd, unsigned int idx,
   struct device *dev = icd-parent;
   int shift;
  
 + fmt.pad = icd-src_pad_idx;
   ret = v4l2_subdev_call(sd, pad, get_fmt, NULL, fmt);
   if (ret  0)
   return ret;
 diff --git a/drivers/media/platform/soc_camera/soc_camera.c 
 b/drivers/media/platform/soc_camera/soc_camera.c
 index d708df4..c4952c8 100644
 --- a/drivers/media/platform/soc_camera/soc_camera.c
 +++ b/drivers/media/platform/soc_camera/soc_camera.c
 @@ -1293,6 +1293,9 @@ static int soc_camera_probe_finish(struct 
 soc_camera_device *icd)
   .which = V4L2_SUBDEV_FORMAT_ACTIVE,
   };
   struct v4l2_mbus_framefmt *mf = fmt.format;
 +#if defined(CONFIG_MEDIA_CONTROLLER)
 + struct media_pad pad;
 +#endif
   int ret;
  
   sd-grp_id = soc_camera_grp_id(icd);
 @@ -1310,8 +1313,40 @@ static int soc_camera_probe_finish(struct 
 soc_camera_device *icd)
   return ret;
   }
  
 + icd-src_pad_idx = -1;

If CONFIG_MEDIA_CONTROLLER is not defined, then I would expect that src_pad_idx 
should
be 0, not -1.

 +#if defined(CONFIG_MEDIA_CONTROLLER)
   /* At this point client .probe() should have run already */
 + ret = media_entity_init(icd-vdev-entity, 1, pad, 0);
 + if (ret  0) {
 + goto eusrfmt;
 + } else {
 + int pad_idx;
 +
 + for (pad_idx = 0; pad_idx  sd-entity.num_pads; pad_idx++)
 + if (sd-entity.pads[pad_idx].flags
 + == MEDIA_PAD_FL_SOURCE)
 + break;
 + if (pad_idx = sd-entity.num_pads)
 + goto eusrfmt;
 +
 + ret = media_entity_create_link(icd-vdev-entity, 0,
 + sd-entity, pad_idx,
 + MEDIA_LNK_FL_IMMUTABLE |
 + MEDIA_LNK_FL_ENABLED);

Why would you want to create a link and set up the vdev entity?

All you need to do here is to find the sd source pad and store it in
sd-src_pad_idx.

You are not registering a media device here, so there is no need to
do anything more. I'd drop all that code.

 + if (ret  0)
 + goto eusrfmt;
 +
 + icd-src_pad_idx = pad_idx;
 + ret = soc_camera_init_user_formats(icd);
 + if (ret  0) {
 + icd-src_pad_idx = -1;
 + goto eusrfmt;
 + }
 + }
 +#else
   ret = soc_camera_init_user_formats(icd);
 +#endif
 +
   if (ret  0)
   goto eusrfmt;
  
 @@ -1322,6 +1357,9 @@ static int soc_camera_probe_finish(struct 
 soc_camera_device *icd)
   goto evidstart;
  
   /* Try to improve our guess of a reasonable window format */
 +#if defined(CONFIG_MEDIA_CONTROLLER)
 + fmt.pad = icd-src_pad_idx;
 +#endif

If src_pad_idx is 0 if CONFIG_MEDIA_CONTROLLER is undefined, then you don't 
need the
#if defined() here.

Regards,

Hans

   if (!v4l2_subdev_call(sd, pad, get_fmt, NULL, fmt)) {
   icd-user_width = mf-width;
   icd-user_height= mf-height;
 @@ -1335,6 +1373,9 @@ static int