Re: [PATCH 3/9] dm355 ccdc module for vpfe capture driver
Hi,
sorry not to have answered sooner.
On Tuesday 09 June 2009 00:05:10 Karicheri, Muralidharan wrote:
+
+/* register access routines */
+static inline u32 regr(u32 offset)
+{
+ if (offset = ccdc_addr_size)
This should be .
+ return __raw_readl(ccdc_base_addr + offset);
+ else {
+ dev_err(dev, offset exceeds ccdc register address space\n);
+ return -1;
+ }
+}
+
+static inline u32 regw(u32 val, u32 offset)
+{
+ if (offset = ccdc_addr_size) {
This should be .
[MK] I removed above two checks since we are getting the required IO block
mapped and this check is not needed. if some reason there is an offset
outside the valid range, it is a bug and will be caught through oops,
right?.
Not necessarily. I'm not sure how ioremap works exactly, but accessing data
outside the remapped region might end up being a valid access. Anyway this
shouldn't happen unless a bug creeps in the driver. If you want to validate
the size it should be done in ccdc_set_ccdc_base instead, but that's not
really required.
+ __raw_writel(val, ccdc_base_addr + offset);
+ return val;
+ } else {
+ dev_err(dev, offset exceeds ccdc register address space\n);
+ return -1;
+ }
+}
Can't you check that ccdc_addr_size is big enough in ccdc_set_ccdc_base ?
The read/write access functions could then be made much simpler.
[MK] See above comment
+ }
+
+ switch (ctrl-id) {
+ case CCDC_CID_R_GAIN:
+ gain-r_ye = ctrl-value CCDC_GAIN_MASK;
+ break;
+ case CCDC_CID_GR_GAIN:
+ gain-gr_cy = ctrl-value CCDC_GAIN_MASK;
+ break;
+ case CCDC_CID_GB_GAIN:
+ gain-gb_g = ctrl-value CCDC_GAIN_MASK;
+ break;
+
+ case CCDC_CID_B_GAIN:
+ gain-b_mg = ctrl-value CCDC_GAIN_MASK;
+ break;
case CCDC_CID_OFFSET: ?
+ default:
+ ccdc_hw_params_raw.ccdc_offset = ctrl-value
CCDC_OFFSET_MASK;
[MK] default is for offset. We are validating the ids above for any of the
checked values in the switch statement
If you're sure that the control ID is one of the above cases, use
CCDC_CID_OFFSET instead of default, the code will be easier to read.
+
+static void ccdc_reset(void)
+{
+ int i;
+ /* disable CCDC */
+ dev_dbg(dev, \nstarting ccdc_reset...);
+ ccdc_enable(0);
+ /* set all registers to default value */
+ for (i = 0; i = 0x15c; i += 4)
+ regw(0, i);
Ouch ! Not all registers have a 0 default value. Beside, blindly resetting
all registers sounds hackish. According to the documentation, the proper
way to reset the VPFE/VPSS is through the power sleep controller.
[MK] This function actually write default values in the registers since the
implementation assume this. So I have renamed it as ccdc_restore_defaults().
We don't want to reset the ccdc here, only want to write default values.
Some registers have a non-0 default value according to the datasheet.
+/*
+ * ccdc_setwin
+ *
+ * This function will configure the window size to
+ * be capture in CCDC reg
+ */
+static void ccdc_setwin(struct ccdc_imgwin *image_win,
+ enum ccdc_frmfmt frm_fmt, int ppc)
What's does ppc stand for ?
[MK] per pixel count. I documented this.
+{
+ int horz_start, horz_nr_pixels;
Does horz_nr_pixels really store the number of pixels ? It seems to me it
counts the number of bytes. hstart and hsize would then be more
appropriate names.
[MK]. It is storing number of pixels and the documentation say so. So names
used are fine as per hw spec.
Then I'm puzzled by
horz_start = image_win-left (ppc - 1);
image_win-left is a number of pixels. Shifting it left by 1 in the YUV case
will give a number of bytes.
+ int vert_start, vert_nr_lines;
If the above comment is correct, this could become vstart and vsize to be
consistent.
[MK] No change. See above
+ return 0;
+}
+static enum ccdc_buftype ccdc_get_buftype(void)
+{
+ if (ccdc_if_type == VPFE_RAW_BAYER)
+ return ccdc_hw_params_raw.buf_type;
+ return ccdc_hw_params_ycbcr.buf_type;
+}
+
+static int ccdc_enum_pix(enum vpfe_hw_pix_format *hw_pix, int i)
+{
+ int ret = -EINVAL;
+ if (ccdc_if_type == VPFE_RAW_BAYER) {
+ if (i CCDC_MAX_RAW_BAYER_FORMATS) {
+ *hw_pix = ccdc_raw_bayer_hw_formats[i];
How does this compare to memcpy in term of code size and run time ?
[MK] not sure what you are asking here.
I was wondering which of
*hw_pix = ccdc_raw_bayer_hw_formats[i];
and
memcpy(hw_pix, ccdc_raw_bayer_hw_formats[i], sizeof(*hw_pix));
lead to a smaller code size and run time. It's a minor issue, it can always be
changed later if memcpy is found to be smaller and faster.
+ return 0;
+}
+#define CCDC_WIN_PAL {0, 0, 720,
RE: [PATCH 3/9] dm355 ccdc module for vpfe capture driver
Laurent,
Laurent,
Thanks for reviewing this. See my response below for few comments. Rest of them
I have incorporated into my next patch.
Murali Karicheri
Software Design Engineer
Texas Instruments Inc.
Germantown, MD 20874
Phone : 301-515-3736
email: m-kariche...@ti.com
+
+/* register access routines */
+static inline u32 regr(u32 offset)
+{
+if (offset = ccdc_addr_size)
This should be .
+return __raw_readl(ccdc_base_addr + offset);
+else {
+dev_err(dev, offset exceeds ccdc register address space\n);
+return -1;
+}
+}
+
+static inline u32 regw(u32 val, u32 offset)
+{
+if (offset = ccdc_addr_size) {
This should be .
[MK] I removed above two checks since we are getting the required IO block
mapped and this check is not needed. if some reason there is an offset outside
the valid range, it is a bug and will be caught through oops, right?.
+__raw_writel(val, ccdc_base_addr + offset);
+return val;
+} else {
+dev_err(dev, offset exceeds ccdc register address space\n);
+return -1;
+}
+}
Can't you check that ccdc_addr_size is big enough in ccdc_set_ccdc_base ?
The
read/write access functions could then be made much simpler.
[MK] See above comment
+
+
+/* Query control. Only applicable for Bayer capture */
+static int ccdc_queryctrl(struct v4l2_queryctrl *qctrl)
+{
+int i, id;
+struct v4l2_queryctrl *control = NULL;
+
+dev_dbg(dev, ccdc_queryctrl: start\n);
+if (NULL == qctrl) {
Can this happen ?
[MK] No. removed
+dev_err(dev, ccdc_queryctrl : invalid user ptr\n);
+return -EINVAL;
+}
+
+if (VPFE_RAW_BAYER != ccdc_if_type) {
+dev_err(dev,
+ ccdc_queryctrl : Not doing Raw Bayer Capture\n);
+return -EINVAL;
+}
+
+id = qctrl-id;
+memset(qctrl, 0, sizeof(struct v4l2_queryctrl));
+for (i = 0; i CCDC_MAX_CONTROLS; i++) {
+control = ccdc_control_info[i];
+if (control-id == id)
+break;
+}
+if (i == CCDC_MAX_CONTROLS) {
+if (NULL == ctrl) {
+dev_err(dev, ccdc_setcontrol: invalid user ptr\n);
+return -EINVAL;
+}
+
+if (ccdc_if_type != VPFE_RAW_BAYER) {
+dev_err(dev,
+ ccdc_setcontrol: Not doing Raw Bayer Capture\n);
Should user-triggered errors use dev_err or dev_dbg ? I'm not sure we want
to
fill the kernel log with KERN_ERR message just because an application
doesn't
get its control ids right.
[MK] changed to dev_dbg
+}
+
+switch (ctrl-id) {
+case CCDC_CID_R_GAIN:
+gain-r_ye = ctrl-value CCDC_GAIN_MASK;
+break;
+case CCDC_CID_GR_GAIN:
+gain-gr_cy = ctrl-value CCDC_GAIN_MASK;
+break;
+case CCDC_CID_GB_GAIN:
+gain-gb_g = ctrl-value CCDC_GAIN_MASK;
+break;
+
+case CCDC_CID_B_GAIN:
+gain-b_mg = ctrl-value CCDC_GAIN_MASK;
+break;
case CCDC_CID_OFFSET: ?
+default:
+ccdc_hw_params_raw.ccdc_offset = ctrl-value
CCDC_OFFSET_MASK;
[MK] default is for offset. We are validating the ids above for any of the
checked values in the switch statement
+
+static void ccdc_reset(void)
+{
+int i;
+/* disable CCDC */
+dev_dbg(dev, \nstarting ccdc_reset...);
+ccdc_enable(0);
+/* set all registers to default value */
+for (i = 0; i = 0x15c; i += 4)
+regw(0, i);
Ouch ! Not all registers have a 0 default value. Beside, blindly resetting
all
registers sounds hackish. According to the documentation, the proper way to
reset the VPFE/VPSS is through the power sleep controller.
[MK] This function actually write default values in the registers since the
implementation assume this. So I have renamed it as ccdc_restore_defaults(). We
don't want to reset the ccdc here, only want to write default values.
+/* no culling support */
+regw(0x, CULH);
+regw(0xff, CULV);
+/* Set default Gain and Offset */
+ccdc_hw_params_raw.gain.r_ye = 256;
+ccdc_hw_params_raw.gain.gb_g = 256;
+ccdc_hw_params_raw.gain.gr_cy = 256;
+ccdc_hw_params_raw.gain.b_mg = 256;
+ccdc_hw_params_raw.ccdc_offset = 0;
+ccdc_config_gain_offset();
+/* up to 12 bit sensor */
+regw(0x0FFF, OUTCLIP);
+/* CCDC input Mux select directly from sensor */
+regw_bl(0x00, CCDCMUX);
+dev_dbg(dev, \nEnd of ccdc_reset...);
+}
+
+static int ccdc_open(struct device *device)
+{
+dev = device;
+ccdc_reset();
+return 0;
+}
+
+static int ccdc_close(struct device *device)
+{
+/* do nothing for now */
+return 0;
+}
+/*
+ * ccdc_setwin
+ *
+ * This function will configure the window size to
+ * be capture in CCDC reg
+ */
+static void ccdc_setwin(struct ccdc_imgwin
Re: [PATCH 3/9] dm355 ccdc module for vpfe capture driver
Hi, On Tuesday 02 June 2009 02:12:41 Kevin Hilman wrote: Karicheri, Muralidharan m-kariche...@ti.com writes: Thanks for reviewing this. I have not gone through all of your comments, but would like to respond to the following one first. I will respond to the rest as I do the rework. I've had a quick look at the DM355 and DM6446 datasheets. The CCDC and VPSS registers share the same memory block. Can't you use a single resource ? One nice (and better in my opinion) solution would be to declare a structure with all the VPSS/CCDC registers as they are implemented in hardware and access the structure fields with __raw_read/write*. You would then store a single pointer only. Check arch/powerpc/include/asm/immap_cpm2.h for an example. I think, a better solution will be to move the vpss system module part to the board specific file dm355.c or dm6446.c Just to clarify, the files you mention are SoC specific files, not board-specific files... and export functions to configure them as needed by the different drivers. My first reaction to this is... no. I'm reluctant to have a bunch of driver specific hooks in the core davinci SoC specific code. I'd much rather see this stuff kept along with the driver in drivers/media/* and abstracted as necessary there. I agree with Kevin on this. arch/* is mostly meant for platform-specific infrastructure code. Device drivers should go in drivers/*. The VPSS/VPFE/CCDC/... abstraction should live in drivers/media/video/*. SoC- specific code that can be shared between multiple drivers (I remember we discussed IRQ routing for instance) can go in arch/*. The vpss has evolved quite a lot from DM6446 to DM355 to DM365. Registers such as INTSEL and INTSTAT and others are applicable to other modules as well, not just the ccdc module. The VPBE and resizer drivers will need to configure them too. Also the vpss system module features available in DM365 is much more than that in DM355. The VPBE and resizer modules will live in drivers/media/video/* as well (correct me if I'm wrong), so most abstraction can go there. INTSEL and INTSTAT could be handled in arch/*, but they're located in the ISP registers block, so I suppose they're only applicable to video-related modules. Based on this, it sounds to me that this driver needs to be broken up a little bit more and some of the shared pieces need to be abstracted out so they can be shared among the modules. Interrupts line to ARM are programmable in DM365 vpss and multiple vpss irq lines can be muxed to the ARM side. So I would imaging functions enable/disable irq line to arm, clearing irq bits, enabling various clocks etc can be done in a specific soc specific file (dm355.c, dm365.c or dm6446.c) and can be exported for use in specific drivers. I just want to get your feedback so that I can make this change. With this change, there is no need to use structures for holding register offsets as you have suggested. I agree with that. Interrupt handling, clock routing and other SoC-specific infrastructure code that isn't module-dependent should live in arch/*. Best regards, Laurent Pinchart -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 3/9] dm355 ccdc module for vpfe capture driver
My first reaction to this is... no. I'm reluctant to have a bunch of driver specific hooks in the core davinci SoC specific code. I'd much rather see this stuff kept along with the driver in drivers/media/* and abstracted as necessary there. I agree with Kevin on this. arch/* is mostly meant for platform-specific infrastructure code. Device drivers should go in drivers/*. The VPSS/VPFE/CCDC/... abstraction should live in drivers/media/video/*. SoC- specific code that can be shared between multiple drivers (I remember we discussed IRQ routing for instance) can go in arch/*. [MK] yes. As per your definition vpss module registers are shared across all video drivers. So it appears it has to go in arch/*. But I am now more inclined to write a platform driver for vpss that live inside media/video/davinci/ and shall export a bunch of library functions shared across drivers. I will make this change and will send it as part of v2 version of the patch series... -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 3/9] dm355 ccdc module for vpfe capture driver
Laurent, Thanks for reviewing this. I have not gone through all of your comments, but would like to respond to the following one first. I will respond to the rest as I do the rework. I've had a quick look at the DM355 and DM6446 datasheets. The CCDC and VPSS registers share the same memory block. Can't you use a single resource ? One nice (and better in my opinion) solution would be to declare a structure with all the VPSS/CCDC registers as they are implemented in hardware and access the structure fields with __raw_read/write*. You would then store a single pointer only. Check arch/powerpc/include/asm/immap_cpm2.h for an example. I think, a better solution will be to move the vpss system module part to the board specific file dm355.c or dm6446.c and export functions to configure them as needed by the different drivers. The vpss has evolved quite a lot from DM6446 to DM355 to DM365. Registers such as INTSEL and INTSTAT and others are applicable to other modules as well, not just the ccdc module. The VPBE and resizer drivers will need to configure them too. Also the vpss system module features available in DM365 is much more than that in DM355. Interrupts line to ARM are programmable in DM365 vpss and multiple vpss irq lines can be muxed to the ARM side. So I would imaging functions enable/disable irq line to arm, clearing irq bits, enabling various clocks etc can be done in a specific soc specific file (dm355.c, dm365.c or dm6446.c) and can be exported for use in specific drivers. I just want to get your feedback so that I can make this change. With this change, there is no need to use structures for holding register offsets as you have suggested. -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/9] dm355 ccdc module for vpfe capture driver
Karicheri, Muralidharan m-kariche...@ti.com writes: Laurent, Thanks for reviewing this. I have not gone through all of your comments, but would like to respond to the following one first. I will respond to the rest as I do the rework. I've had a quick look at the DM355 and DM6446 datasheets. The CCDC and VPSS registers share the same memory block. Can't you use a single resource ? One nice (and better in my opinion) solution would be to declare a structure with all the VPSS/CCDC registers as they are implemented in hardware and access the structure fields with __raw_read/write*. You would then store a single pointer only. Check arch/powerpc/include/asm/immap_cpm2.h for an example. I think, a better solution will be to move the vpss system module part to the board specific file dm355.c or dm6446.c Just to clarify, the files you mention are SoC specific files, not board-specific files... and export functions to configure them as needed by the different drivers. My first reaction to this is... no. I'm reluctant to have a bunch of driver specific hooks in the core davinci SoC specific code. I'd much rather see this stuff kept along with the driver in drivers/media/* and abstracted as necessary there. The vpss has evolved quite a lot from DM6446 to DM355 to DM365. Registers such as INTSEL and INTSTAT and others are applicable to other modules as well, not just the ccdc module. The VPBE and resizer drivers will need to configure them too. Also the vpss system module features available in DM365 is much more than that in DM355. Based on this, it sounds to me that this driver needs to be broken up a little bit more and some of the shared pieces need to be abstracted out so they can be shared among the modules. Interrupts line to ARM are programmable in DM365 vpss and multiple vpss irq lines can be muxed to the ARM side. So I would imaging functions enable/disable irq line to arm, clearing irq bits, enabling various clocks etc can be done in a specific soc specific file (dm355.c, dm365.c or dm6446.c) and can be exported for use in specific drivers. I just want to get your feedback so that I can make this change. With this change, there is no need to use structures for holding register offsets as you have suggested. Kevin -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/9] dm355 ccdc module for vpfe capture driver
On Thursday 28 May 2009 15:18:17 Laurent Pinchart wrote: Hi, most comments here apply to the DM6446 CCDC module as well. Generic comments apply throughout the source code. Hans, I'd appreciate if you could review my comments, as some of them might made according to personal preferences more than best practice rules. I did a quick review of your review :-) and it looks fine to me! Regards, Hans -- Hans Verkuil - video4linux developer - sponsored by TANDBERG Telecom -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
