Hans, That was quick!
Please see my response inline. >>> >>>Yes, that's OK. What timeframe for the subdev conversion did you have in >>>mind? Personally I would like to see v4l2-int-device removed for kernel >>>2.6.31. >>> [MK] Sorry to ask a question back. What is the timeframe when 2.6.31 is available ? I am not very familiar with the kernel release process. I need to coordinate this with Vaibhav who is responsible from OMAP side which is using the same tvp514x driver. I will ask him to be in this discussion. >>>> Other responses are inline. >>>> >>>> >>>I have one comment that also refers to the new DM646x driver that >>>was >>>> >>>send to >>>> >>>the list for review, which is why I CC-ed this to Chaithrika as >>>well: >>>> >>> >>>> >>>The DM335/DM6446 adds the following files: >>>> >>> >>>> >>> create mode 100644 drivers/media/video/davinci/ccdc_davinci.c >>>> >>> create mode 100644 drivers/media/video/davinci/ccdc_davinci.h >>>> >>> create mode 100644 drivers/media/video/davinci/vpfe_capture.c >>>> >>> create mode 100644 drivers/media/video/davinci/ccdc_dm355.c >>>> >>> create mode 100644 drivers/media/video/davinci/ccdc_dm355.h >>>> >>> create mode 100644 include/media/davinci/vpfe_capture.h >>>> >>> create mode 100644 include/media/davinci/vpfe_types.h >>>> >>> create mode 100644 include/media/davinci/ccdc_common.h >>>> >>> create mode 100644 include/media/davinci/ccdc_hw_device.h >>>> >>>> [MK] VPFE capture is a bridge driver that is used on DM355, DM6446 and >>>> DM365. Since all these DM SoCs have common VPFE (Video Processing front >>>> end) that is used for capturing video, I have named the capture driver >>>as >>>> vpfe_capture.c. vpfe_types.h has common vpfe types used across all >>>DMxxx >>>> family. >>> >>>But the dm646x also has a vpfe, but that's different than these, right? >>>So [MK] dm646x has vpif (video peripheral/port interface). So my suggestion is to prefix the bridge driver file by the IP name. For example, DM6467 has vpif IP for capture and display. So I would go for vpif_capture.c for bridge driver and vpif_display.c for display driver. However for HW modules responsible for configuring video port parameters (vpif for DM6467 and ccdc for DM355/6446), I would suggest adding dmxxx prefix along with IP name since the hw may differ from one dmxxx family to another (in terms of features). In future, if TI come up with another DMxxx SoC similar to DM6467 that too uses vpif, then we don't have to change the names again to re-use the bridge driver. So I propose the following for DM6467 (I expect Chaithrika to comment on this). vpif_capture.[ch] -> DM6467 Capture bridge driver vpif_display.[ch] -> DM6467 Display bridge driver dm6467_vpif.[ch] -> DM6467 capture & display hw module For future DM6467 variants, we may able to use the same bridge driver as was done in DM355/DM6446. I am not sure if the DM6467 capture driver requires change to re-use across another DM6467 variant. I will let Chaithrika comment on this. >>>that would have headers like dm646x_capture.h etc. >>> >>>Given this I am inclined to rather go for dmxxx_foo.c/h. Not ideal, but >>>that's because you messed up your numbering scheme :-) :-) >>> >>>What do you think? >>> [MK] See my explanation above. Let us also hear from Chaithrika on this. >>>> I agree with you on renaming following files as suggested. Also >>>> I will add a header description for the terms used (example ccdc, vpfe >>>> etc.) >>>> >>>> ccdc_davinci.c ->dm6446_ccdc.c >>>> ccdc_davinci.h ->dm6446_ccdc.h + dm6446_ccdc_regs.h >>>> vpfe_capture.c -> no change as explained above >>>> ccdc_dm355.c -> dm355_ccdc.c >>>> ccdc_dm355.h -> dm355_ccdc.h + dm355_ccdc_regs.h >>>> vpfe_capture.h -> no change >>>> vpfe_types.h -> no change >>>> ccdc_common.h -> ccdc_types.h (all common types used across DMxxx CCDC) >>>> ccdc_hw_device.h -> header file for hw APIs to configure the CCDC. >>>Since >>>> same bridge driver is used across multiple DMxxx all functions used for >>>> configuring the CCDC are abstracted into these header file and >>>> implemented by the respective ccdc_dmxxx.c. >>> >>>Shouldn't it be dm3x5? Or am I now too pedantic... Anyway, this is >>>already a >>>lot better. >>> >>>> >>>Unless you think it is really important I am not going to review >>>these >>>> >>>drivers. I suggest that you watch my review of the dm646x and fix >>>any >>>> >>>issues that will also apply to this driver before it comes up for >>>> >>> review on the linux-media list. >>>> >>>> [MK] As I have mentioned, my driver is using a different framework >>>> (v4l2-int-device) than DM646x and I will try to address any comments >>>that >>>> are applicable in that context. I will leave it to you if you want >>>review >>>> the driver now or later when I submit the same to v4l2 mailing list. >>> >>>While I had some comments regarding the v4l2_device/v4l2_subdev framework, >>>most comments were about other things that might well apply to this >>>driver >>>as well. >>> >>>I do not have the time to look at this driver as well, so I'll postpone >>>my >>>review until you submit it to the linux-media list. >>> >>>Regards, >>> >>> Hans >>> >>>-- >>>Hans Verkuil - video4linux developer - sponsored by TANDBERG Murali Karicheri Software Design Engineer Texas Instruments Inc. Germantown, MD 20874 Phone : 301-515-3736 email: [email protected] _______________________________________________ Davinci-linux-open-source mailing list [email protected] http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
