Re: [PATCH v3 13/24] platform: add video-multiplexer subdevice driver

[Hans Verkuil]

On 02/05/2017 04:48 PM, Laurent Pinchart wrote:
> Hi Steve,
> 
> On Tuesday 24 Jan 2017 18:07:55 Steve Longerbeam wrote:
>> On 01/24/2017 04:02 AM, Philipp Zabel wrote:
>>> On Fri, 2017-01-20 at 15:03 +0100, Hans Verkuil wrote:
>>>>> +
>>>>> +int vidsw_g_mbus_config(struct v4l2_subdev *sd, struct v4l2_mbus_config
>>>>> *cfg)
>>>>> +{
>>>>> +	struct vidsw *vidsw = v4l2_subdev_to_vidsw(sd);
>>>>> +	struct media_pad *pad;
>>>>> +	int ret;
>>>>> +
>>>>> +	if (vidsw->active == -1) {
>>>>> +		dev_err(sd->dev, "no configuration for inactive mux\n");
>>>>> +		return -EINVAL;
>>>>> +	}
>>>>> +
>>>>> +	/*
>>>>> +	 * Retrieve media bus configuration from the entity connected to the
>>>>> +	 * active input
>>>>> +	 */
>>>>> +	pad = media_entity_remote_pad(&vidsw->pads[vidsw->active]);
>>>>> +	if (pad) {
>>>>> +		sd = media_entity_to_v4l2_subdev(pad->entity);
>>>>> +		ret = v4l2_subdev_call(sd, video, g_mbus_config, cfg);
>>>>> +		if (ret == -ENOIOCTLCMD)
>>>>> +			pad = NULL;
>>>>> +		else if (ret < 0) {
>>>>> +			dev_err(sd->dev, "failed to get source 
> configuration\n");
>>>>> +			return ret;
>>>>> +		}
>>>>> +	}
>>>>> +	if (!pad) {
>>>>> +		/* Mirror the input side on the output side */
>>>>> +		cfg->type = vidsw->endpoint[vidsw->active].bus_type;
>>>>> +		if (cfg->type == V4L2_MBUS_PARALLEL ||
>>>>> +		    cfg->type == V4L2_MBUS_BT656)
>>>>> +			cfg->flags = vidsw->endpoint[vidsw-
>> active].bus.parallel.flags;
>>>>> +	}
>>>>> +
>>>>> +	return 0;
>>>>> +}
>>>>
>>>> I am not certain this op is needed at all. In the current kernel this op
>>>> is only used by soc_camera, pxa_camera and omap3isp (somewhat dubious).
>>>> Normally this information should come from the device tree and there
>>>> should be no need for this op.
>>>>
>>>> My (tentative) long-term plan was to get rid of this op.
>>>>
>>>> If you don't need it, then I recommend it is removed.
>>
>> Hi Hans, the imx-media driver was only calling g_mbus_config to the camera
>> sensor, and it was doing that to determine the sensor's bus type. This info
>> was already available from parsing a v4l2_of_endpoint from the sensor node.
>> So it was simple to remove the g_mbus_config calls, and instead rely on the
>> parsed sensor v4l2_of_endpoint.
> 
> That's not a good point. The imx-media driver must not parse the sensor DT 
> node as it is not aware of what bindings the sensor is compatible with. 
> Information must instead be queried from the sensor subdev at runtime, through 
> the g_mbus_config() operation.
> 
> Of course, if you can get the information from the imx-media DT node, that's 
> certainly an option. It's only information provided by the sensor driver that 
> you have no choice but query using a subdev operation.

Shouldn't this come from the imx-media DT node? BTW, why is omap3isp using this?

The reason I am suspicious about this op is that it came from soc-camera and
predates the DT. The contents of v4l2_mbus_config seems very much like a HW
description to me, i.e. something that belongs in the DT.

Regards,

	Hans