Re: [RFC PATCH] adding support for setting bus parameters in sub device

["Jean-Philippe François" <jp.francois@cynove.com>]

Karicheri, Muralidharan a écrit :
>>> data9-data15 for MT9T031
>>> data11-data15 for MT9P031
>> But then you could set the host bus width accordingly for example :
>> MT9T031 MSB connected do data 9 : HOST Buswidth = 10
>> MT9T031 MSB connected to data 15: Host Buswdth = 16
> 
> How does the driver know which MSB of the sensor data line is connected to the host bus? This is to be configured in our hardware register. Without this, I need to hardcode it which doesn't seem right.
> 
I know you don't want to hardcode this, it is just not clear to me why 3 
parameters are needed. Let's take two different hardware setup : MT9T031 
with MSB connected on video pin 9 and 11

struct v4l2_bus_settings mt9t031_msb9 = {
	.subdev_width=10
	.host_width= ???
	.host_msb = 9
};

struct v4l2_bus_settings mt9t031_msb11 = {
	.subdev_width=10
	.host_width= ???
	.host_msb = 11
};

I can't see what is the use of those three parameters, and you have a 
cohabitation of width and bit number which is error prone.

Can you explain what is the meaning of host_width, or provide a 
realistic example where this 3 settings are useful ?



> Murali Karicheri
> Software Design Engineer
> Texas Instruments Inc.
> Germantown, MD 20874
> Phone : 301-515-3736
> email: m-karicheri2@ti.com
> 
>> -----Original Message-----
>> From: Jean-Philippe François [mailto:jp.francois@cynove.com]
>> Sent: Monday, June 29, 2009 12:23 PM
>> To: Karicheri, Muralidharan
>> Cc: Hans Verkuil; davinci-linux-open-source@linux.davincidsp.com; linux-
>> media@vger.kernel.org
>> Subject: Re: [RFC PATCH] adding support for setting bus parameters in sub
>> device
>>
>> Karicheri, Muralidharan a écrit :
>>> Hans,
>>>
>>> Had hit the send by mistake. Please ignore my previous reply...
>>>
>>> <snip>
>>>>> It seems very unlikely to me that someone would ever choose to e.g.
>> zero
>>>>> one or more MSB pins instead of the LSB pins in a case like this.
>>>>>
>>> No case in my experience...
>>>
>>> <snip>
>>>>> Or do you have examples where that actually happens?
>>>>>
>>> DM365 can work with 10 bit or 12 bit sensors. DM365 CCDC that receives
>> the
>>> image data needs to know which data lines have valid data. This is done
>> by
>>> specifying the MSB position. There is ccdc register to configure this
>> information. Ideally, you could connect the MSB of sensor to following
>> lines on host bus:-
>>> data9-data15 for MT9T031
>>> data11-data15 for MT9P031
>> But then you could set the host bus width accordingly for example :
>> MT9T031 MSB connected do data 9 : HOST Buswidth = 10
>> MT9T031 MSB connected to data 15: Host Buswdth = 16
>>
>> Since the host/sensor info are in the same structure, we can have
>> several subdev with different host buswidth.
>>
>> Jean-Philippe François
>>
>>>
>>> So it makes sense to specify this choice in the structure. I have not
>> added this earlier since we wanted to use the structure only for sub device
>> configuration. It has changed since then.
>>> I am also not sure if s_bus() is required since this will get set in the
>> platform data which could then be passed to the sub device using the new
>> api while loading it. When would host driver call s_bus()?
>>>>> If this never happens, then there is also no need for such a bitfield.
>>>>>
>>>>> I think I want to actually see someone using this before we add a field
>>>>> like that.
>>>>>
>>>>> Regards,
>>>>>
>>>>>       Hans
>>>>>
>>>>>
>>>>>> Murali Karicheri
>>>>>> Software Design Engineer
>>>>>> Texas Instruments Inc.
>>>>>> Germantown, MD 20874
>>>>>> email: m-karicheri2@ti.com
>>>>>>
>>>>>>> -----Original Message-----
>>>>>>> From: Hans Verkuil [mailto:hverkuil@xs4all.nl]
>>>>>>> Sent: Monday, June 29, 2009 5:26 AM
>>>>>>> To: Karicheri, Muralidharan
>>>>>>> Cc: linux-media@vger.kernel.org; davinci-linux-open-
>>>>>>> source@linux.davincidsp.com
>>>>>>> Subject: Re: [RFC PATCH] adding support for setting bus parameters in
>>>> sub
>>>>>>> device
>>>>>>>
>>>>>>> Hi Murali,
>>>>>>>
>>>>>>>> From: Muralidharan Karicheri <a0868495@gt516km11.gt.design.ti.com>
>>>>>>>>
>>>>>>>> This patch adds support for setting bus parameters such as bus type
>>>>>>>> (Raw Bayer or Raw YUV image data bus), bus width (example 10 bit raw
>>>>>>>> image data bus, 10 bit BT.656 etc.), and polarities (vsync, hsync,
>>>>>>>> field
>>>>>>>> etc) in sub device. This allows bridge driver to configure the sub
>>>>>>>> device
>>>>>>>> bus for a specific set of bus parameters through s_bus() function
>> call.
>>>>>>>> This also can be used to define platform specific bus parameters for
>>>>>>>> host and sub-devices.
>>>>>>>>
>>>>>>>> Reviewed by: Hans Verkuil <hverkuil@xs4all.nl>
>>>>>>>> Signed-off-by: Murali Karicheri <m-karicheri2@ti.com>
>>>>>>>> ---
>>>>>>>> Applies to v4l-dvb repository
>>>>>>>>
>>>>>>>>  include/media/v4l2-subdev.h |   40
>>>>>>>> ++++++++++++++++++++++++++++++++++++++++
>>>>>>>>  1 files changed, 40 insertions(+), 0 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-
>> subdev.h
>>>>>>>> index 1785608..2f5ec98 100644
>>>>>>>> --- a/include/media/v4l2-subdev.h
>>>>>>>> +++ b/include/media/v4l2-subdev.h
>>>>>>>> @@ -37,6 +37,43 @@ struct v4l2_decode_vbi_line {
>>>>>>>>  	u32 type;		/* VBI service type (V4L2_SLICED_*). 0 if no
>>>>>>> service found */
>>>>>>>>  };
>>>>>>>>
>>>>>>>> +/*
>>>>>>>> + * Some sub-devices are connected to the host/bridge device through
>> a
>>>>>>> bus
>>>>>>>> that
>>>>>>>> + * carries the clock, vsync, hsync and data. Some interfaces such
>> as
>>>>>>>> BT.656
>>>>>>>> + * carries the sync embedded in the data where as others have
>>>> separate
>>>>>>>> line
>>>>>>>> + * carrying the sync signals. The structure below is used to define
>>>>>>>> bus
>>>>>>>> + * configuration parameters for host as well as sub-device
>>>>>>>> + */
>>>>>>>> +enum v4l2_subdev_bus_type {
>>>>>>>> +	/* Raw YUV image data bus */
>>>>>>>> +	V4L2_SUBDEV_BUS_RAW_YUV,
>>>>>>>> +	/* Raw Bayer image data bus */
>>>>>>>> +	V4L2_SUBDEV_BUS_RAW_BAYER
>>>>>>>> +};
>>>>>>>> +
>>>>>>>> +struct v4l2_bus_settings {
>>>>>>>> +	/* yuv or bayer image data bus */
>>>>>>>> +	enum v4l2_subdev_bus_type type;
>>>>>>>> +	/* subdev bus width */
>>>>>>>> +	u8 subdev_width;
>>>>>>>> +	/* host bus width */
>>>>>>>> +	u8 host_width;
>>>>>>>> +	/* embedded sync, set this when sync is embedded in the data
>>>> stream
>>>>>>> */
>>>>>>>> +	unsigned embedded_sync:1;
>>>>>>>> +	/* master or slave */
>>>>>>>> +	unsigned host_is_master:1;
>>>>>>>> +	/* 0 - active low, 1 - active high */
>>>>>>>> +	unsigned pol_vsync:1;
>>>>>>>> +	/* 0 - active low, 1 - active high */
>>>>>>>> +	unsigned pol_hsync:1;
>>>>>>>> +	/* 0 - low to high , 1 - high to low */
>>>>>>>> +	unsigned pol_field:1;
>>>>>>>> +	/* 0 - sample at falling edge , 1 - sample at rising edge */
>>>>>>>> +	unsigned pol_pclock:1;
>>>>>>>> +	/* 0 - active low , 1 - active high */
>>>>>>>> +	unsigned pol_data:1;
>>>>>>>> +};
>>>>>>> I've been thinking about this for a while and I think this struct
>> should
>>>>>>> be extended with the host bus parameters as well:
>>>>>>>
>>>>>>> struct v4l2_bus_settings {
>>>>>>> 	/* yuv or bayer image data bus */
>>>>>>> 	enum v4l2_bus_type type;
>>>>>>> 	/* embedded sync, set this when sync is embedded in the data
>> stream
>>>>>>> */
>>>>>>> 	unsigned embedded_sync:1;
>>>>>>> 	/* master or slave */
>>>>>>> 	unsigned host_is_master:1;
>>>>>>>
>>>>>>> 	/* bus width */
>>>>>>> 	unsigned sd_width:8;
>>>>>>> 	/* 0 - active low, 1 - active high */
>>>>>>> 	unsigned sd_pol_vsync:1;
>>>>>>> 	/* 0 - active low, 1 - active high */
>>>>>>> 	unsigned sd_pol_hsync:1;
>>>>>>> 	/* 0 - low to high, 1 - high to low */
>>>>>>> 	unsigned sd_pol_field:1;
>>>>>>> 	/* 0 - sample at falling edge, 1 - sample at rising edge */
>>>>>>> 	unsigned sd_edge_pclock:1;
>>>>>>> 	/* 0 - active low, 1 - active high */
>>>>>>> 	unsigned sd_pol_data:1;
>>>>>>>
>>>>>>> 	/* host bus width */
>>>>>>> 	unsigned host_width:8;
>>>>>>> 	/* 0 - active low, 1 - active high */
>>>>>>> 	unsigned host_pol_vsync:1;
>>>>>>> 	/* 0 - active low, 1 - active high */
>>>>>>> 	unsigned host_pol_hsync:1;
>>>>>>> 	/* 0 - low to high, 1 - high to low */
>>>>>>> 	unsigned host_pol_field:1;
>>>>>>> 	/* 0 - sample at falling edge, 1 - sample at rising edge */
>>>>>>> 	unsigned host_edge_pclock:1;
>>>>>>> 	/* 0 - active low, 1 - active high */
>>>>>>> 	unsigned host_pol_data:1;
>>>>>>> };
>>>>>>>
>>>>>>> It makes sense since you need to setup both ends of the bus, and
>> having
>>>>>>> both ends defined in the same struct keeps everything together. I
>> have
>>>>>>> thought about having separate host and subdev structs, but part of
>> the
>>>>>>> bus
>>>>>>> description is always common (bus type, master/slave,
>> embedded/separate
>>>>>>> syncs), while another part can be different for each end of the bus.
>>>>>>>
>>>>>>> It's all bitfields, so it is a very compact representation.
>>>>>>>
>>>>>>> In addition, I think we need to require that at the start of the
>> s_bus
>>>>>>> implementation in the host or subdev there should be a standard
>> comment
>>>>>>> block describing the possible combinations supported by the hardware:
>>>>>>>
>>>>>>> /* Subdevice foo supports the following bus settings:
>>>>>>>
>>>>>>>   types: RAW_BAYER (widths: 8/10/12, syncs: embedded/separate)
>>>>>>>          RAW_YUV (widths: 8/16, syncs: embedded)
>>>>>>>   bus master: slave
>>>>>>>   vsync polarity: 0/1
>>>>>>>   hsync polarity: 0/1
>>>>>>>   field polarity: not applicable
>>>>>>>   sampling edge pixelclock: 0/1
>>>>>>>   data polarity: 1
>>>>>>> */
>>>>>>>
>>>>>>> This should make it easy for implementers to pick a valid set of bus
>>>>>>> parameters.
>>>>>>>
>>>>>>> Regards,
>>>>>>>
>>>>>>>       Hans
>>>>>>>
>>>>>>>> +
>>>>>>>>  /* Sub-devices are devices that are connected somehow to the main
>>>>>>>> bridge
>>>>>>>>     device. These devices are usually audio/video
>>>>>>> muxers/encoders/decoders
>>>>>>>> or
>>>>>>>>     sensors and webcam controllers.
>>>>>>>> @@ -199,6 +236,8 @@ struct v4l2_subdev_audio_ops {
>>>>>>>>
>>>>>>>>     s_routing: see s_routing in audio_ops, except this version is
>> for
>>>>>>>> video
>>>>>>>>  	devices.
>>>>>>>> +
>>>>>>>> +   s_bus: set bus parameters in sub device to configure the bus
>>>>>>>>   */
>>>>>>>>  struct v4l2_subdev_video_ops {
>>>>>>>>  	int (*s_routing)(struct v4l2_subdev *sd, u32 input, u32 output,
>>>> u32
>>>>>>>> config);
>>>>>>>> @@ -219,6 +258,7 @@ struct v4l2_subdev_video_ops {
>>>>>>>>  	int (*s_parm)(struct v4l2_subdev *sd, struct v4l2_streamparm
>>>> *param);
>>>>>>>>  	int (*enum_framesizes)(struct v4l2_subdev *sd, struct
>>>>>>> v4l2_frmsizeenum
>>>>>>>> *fsize);
>>>>>>>>  	int (*enum_frameintervals)(struct v4l2_subdev *sd, struct
>>>>>>>> v4l2_frmivalenum *fival);
>>>>>>>> +	int (*s_bus)(struct v4l2_subdev *sd, const struct
>>>> v4l2_bus_settings
>>>>>>>> *bus);
>>>>>>>>  };
>>>>>>>>
>>>>>>>>  struct v4l2_subdev_ops {
>>>>>>>> --
>>>>>>>> 1.6.0.4
>>>>>>>>
>>>>>>>> --
>>>>>>>> To unsubscribe from this list: send the line "unsubscribe linux-
>> media"
>>>>>>>> in
>>>>>>>> the body of a message to majordomo@vger.kernel.org
>>>>>>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>>>>>>
>>>>>>> --
>>>>>>> Hans Verkuil - video4linux developer - sponsored by TANDBERG
>>>>>>>
>>>>>>
>>>>> --
>>>>> Hans Verkuil - video4linux developer - sponsored by TANDBERG
>>>>>
>>> _______________________________________________
>>> Davinci-linux-open-source mailing list
>>> Davinci-linux-open-source@linux.davincidsp.com
>>> http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
>>>
> 
>