From mboxrd@z Thu Jan 1 00:00:00 1970 Return-path: Received: from smtp4-g21.free.fr ([212.27.42.4]:34361 "EHLO smtp4-g21.free.fr" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751203AbZF3Pk4 (ORCPT ); Tue, 30 Jun 2009 11:40:56 -0400 Message-ID: <4A4A3201.1030802@cynove.com> Date: Tue, 30 Jun 2009 17:40:49 +0200 From: =?ISO-8859-1?Q?Jean-Philippe_Fran=E7ois?= MIME-Version: 1.0 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 References: <36355.62.70.2.252.1246287669.squirrel@webmail.xs4all.nl> <4A48EA7A.5050603@cynove.com> In-Reply-To: Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 8bit Sender: linux-media-owner@vger.kernel.org List-ID: 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... >>> >>> >>>>> 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... >>> >>> >>>>> 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 >>>>>>>> >>>>>>>> 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 >>>>>>>> Signed-off-by: Murali Karicheri >>>>>>>> --- >>>>>>>> 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 >>> > >