On 4/1/20 11:29 PM, karthik poduval wrote: > On Wed, Apr 1, 2020 at 5:00 PM Helen Koike <helen.koike@xxxxxxxxxxxxx> wrote: >> >> Hi Karthik, >> >> It is nice to see this driver being used and tested elsewhere. >> >> How did you tested the series? It would be nice to add it in the commit message. >> > Firstly thank you for your work on the rkisp1 and libcamera and > efforts of bringing it to mainline kernel. I tested the patch series > using yocto build with meta-rockchip and using media kernel tree TOT. > I also used libcamera to configure the pipeline (since it was more > convenient as compared to media-ctl commands). Sure I will add them to > the commit message in my next commit. Would this commit be the best > place to include testing instructions or include into all 4 commits > that I posted ? Nice, I'm glad it worked fine. I think you could just quickly mention in the commits something like: "Tested streaming on a tinkerboard with ov5647 with libcamera command: cam ..." >> On 3/31/20 4:57 AM, karthik poduval wrote: >>> ov5647 is one of the two camera modules as described in >>> https://tinkerboarding.co.uk/wiki/index.php/CSI-camera >>> >>> changes ported over from https://github.com/TinkerBoard/debian_kernel.git >>> >>> Reported-by: Karthik Poduval <karthik.poduval@xxxxxxxxx> >>> Signed-off-by: Karthik Poduval <karthik.poduval@xxxxxxxxx> >> >> please, see my comments from previous commit. >> >>> --- >>> arch/arm/boot/dts/rk3288-tinker.dtsi | 37 ++++++++++++++++++++++++++++ >> >> I wondering if changing thinkerboard dts make sense. Is the camera really hardwired >> on the tinker board, or is it an add-on? >> >> Regards, >> Helen > No the camera isn't hardwired but connects over a flex cable to an > adapter board, however based on my search I found that only 2 camera > adapter boards have been made for tinkerboard, IMX219 and OV5647. The > only camera adapter board I have with me is ov5647. Normally offboard > peripherals also need device tree entries. The tinkerbaord debain 4.4 > kernel includes imx219 device entry in the device tree and provides > ov5647 device tree entry as an overlay. What is the recommendation > here ? Should it be an overlay or not added at all ? So usually this change don't go to the board's dtsi, otherwise you are mandating other users to use the same sensor as you did. I'm not sure how would be the alternative though, since overlays don't usually go upstream, and I'm not sure if there is a way to declare a "expected" node. Regards, Helen >> >>> 1 file changed, 37 insertions(+) >>> >>> diff --git a/arch/arm/boot/dts/rk3288-tinker.dtsi >>> b/arch/arm/boot/dts/rk3288-tinker.dtsi >>> index 312582c1bd37..720dd80ea3aa 100644 >>> --- a/arch/arm/boot/dts/rk3288-tinker.dtsi >>> +++ b/arch/arm/boot/dts/rk3288-tinker.dtsi >>> @@ -107,6 +107,13 @@ >>> startup-delay-us = <100000>; >>> vin-supply = <&vcc_io>; >>> }; >>> + >>> + ext_cam_clk: external-camera-clock { >>> + compatible = "fixed-clock"; >>> + clock-frequency = <25000000>; >>> + clock-output-names = "CLK_CAMERA_25MHZ"; >>> + #clock-cells = <0>; >>> + }; >>> }; >>> >>> &cpu0 { >>> @@ -345,12 +352,42 @@ >>> >>> &i2c2 { >>> status = "okay"; >>> + camera0: ov5647@36 { >>> + compatible = "ovti,ov5647"; >>> + reg = <0x36>; >>> + clocks = <&ext_cam_clk>; >>> + status = "okay"; >>> + enable-gpios = <&gpio2 0 GPIO_ACTIVE_LOW>; >>> + port { >>> + ov5647_out: endpoint { >>> + remote-endpoint = <&isp_mipi_in>; >>> + data-lanes = <1 2>; >>> + }; >>> + }; >>> + }; >>> }; >>> >>> &i2c5 { >>> status = "okay"; >>> }; >>> >>> +&isp { >>> + status = "okay"; >>> + phys = <&mipi_phy_rx0>; >>> + phy-names = "dphy"; >>> + >>> + port { >>> + isp_mipi_in: endpoint { >>> + remote-endpoint = <&ov5647_out>; >>> + data-lanes = <1 2>; >>> + }; >>> + }; >>> +}; >>> + >>> +&mipi_phy_rx0 { >>> + status = "okay"; >>> +}; >>> + >>> &i2s { >>> #sound-dai-cells = <0>; >>> status = "okay"; >>> >> >
