Hi Hans, On Sunday 24 May 2015 10:08:15 Hans Verkuil wrote: > On 05/23/2015 08:32 PM, Laurent Pinchart wrote: > > On Thursday 21 May 2015 07:55:10 Hans Verkuil wrote: > >> On 05/20/2015 06:39 PM, William Towle wrote: > >>> Add detection of source pad number for drivers aware of the media > >>> controller API, so that soc_camera/rcar_vin can create device nodes > >>> to support a driver such as adv7604.c (for HDMI on Lager) underneath. > >>> > >>> Signed-off-by: William Towle <william.towle@xxxxxxxxxxxxxxx> > >>> Reviewed-by: Rob Taylor <rob.taylor@xxxxxxxxxxxxxxx> > >>> --- > >>> > >>> drivers/media/platform/soc_camera/rcar_vin.c | 4 ++++ > >>> drivers/media/platform/soc_camera/soc_camera.c | 27 ++++++++++++++++- > >>> include/media/soc_camera.h | 1 + > >>> 3 files changed, 31 insertions(+), 1 deletion(-) > >>> > >>> diff --git a/drivers/media/platform/soc_camera/rcar_vin.c > >>> b/drivers/media/platform/soc_camera/rcar_vin.c index 0f67646..b4e9b43 > >>> 100644 > >>> --- a/drivers/media/platform/soc_camera/rcar_vin.c > >>> +++ b/drivers/media/platform/soc_camera/rcar_vin.c > >>> @@ -1364,8 +1364,12 @@ static int rcar_vin_get_formats(struct > >>> soc_camera_device *icd, unsigned int idx, > >>> struct v4l2_mbus_framefmt *mf = &fmt.format; > >>> struct v4l2_rect rect; > >>> struct device *dev = icd->parent; > >>> + struct media_pad *remote_pad; > >>> int shift; > >>> > >>> + remote_pad = media_entity_remote_pad( > >>> + &icd->vdev->entity.pads[0]); > >>> + fmt.pad = remote_pad->index; > >> > >> This won't work if CONFIG_MEDIA_CONTROLLER isn't defined. All these media > >> calls would all have to be under #ifdef CONFIG_MEDIA_CONTROLLER. > >> > >> Unfortunately, if it is not defined, then you still have no way of > >> finding the source pad. > >> > >> Laurent, do you think if it would make sense to add a new subdev core op > >> that will return the default source pad (I'm saying 'default' in case > >> there are more) of a subdev? That way it can be used in non-MC drivers. > >> We never needed the source pad before, but now we do, and this op only > >> needs to be implemented if the default source pad != 0. > > > > I'm not too fond of that. Is there something wrong with the method > > implemented in this patch ? Is the dependency on CONFIG_MEDIA_CONTROLLER > > an issue ? > > 1) it's a heck of a lot of code just to get a simple source pad that the > subdev knows anyway, I don't think the subdev knows. If a subdev has multiple source pads there's no concept of a default source. It all depends on how the subdevs are connected, and media_entity_remote_pad() is the right way to find out. > 2) soc-camera doesn't use the media controller today, so this would add a > dependency on the mc just for this, I agree that we shouldn't pull the whole MC userspace API in just for this, but the kernel side of the API should be available as pad-level operations depend on MC. We could split the CONFIG_MEDIA_CONTROLLER option in two. > 3) it doesn't actually make a media device, it is just fakes enough to make > the subdev think it there is an MC. > > It doesn't actually have to be a new op, it could be a new field in > v4l2_subdev as well. For those bridge drivers that do not use the MC but do > need to know the source pad this is important information. > > It might even simplify the device tree if the default source pad is implied > unless stated otherwise (but that might be a step too far). > > I wonder if a default input pad might also be useful to expose. I suspect > this might become important if we want to add MC support to all existing > v4l2 drivers. The concept of a default sink pad makes more sense, but I'm not sure I like it too much either. I'd have to think about it. -- Regards, Laurent Pinchart -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
