| Message ID | CAFP0Ok_3yTv9QWAykvBk3yLgbThycVPdx0taU9F1Wo_WTnVjyA@mail.gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/4] media: staging: phy-rockchip-dphy-rx0: add rk3288 support to DPHY driver | expand |
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. 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@gmail.com> > Signed-off-by: Karthik Poduval <karthik.poduval@gmail.com> 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 > 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"; >
On Wed, Apr 1, 2020 at 5:00 PM Helen Koike <helen.koike@collabora.com> 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 ? > 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@gmail.com> > > Signed-off-by: Karthik Poduval <karthik.poduval@gmail.com> > > 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 ? > > > 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"; > > >
On 4/1/20 11:29 PM, karthik poduval wrote: > On Wed, Apr 1, 2020 at 5:00 PM Helen Koike <helen.koike@collabora.com> 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@gmail.com> >>> Signed-off-by: Karthik Poduval <karthik.poduval@gmail.com> >> >> 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"; >>> >> >
On Thu, Apr 2, 2020 at 7:46 AM Helen Koike <helen.koike@collabora.com> wrote: > > > > On 4/1/20 11:29 PM, karthik poduval wrote: > > On Wed, Apr 1, 2020 at 5:00 PM Helen Koike <helen.koike@collabora.com> 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 ..." > Thanks will do. > >> 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@gmail.com> > >>> Signed-off-by: Karthik Poduval <karthik.poduval@gmail.com> > >> > >> 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 Agree so let me abandon this commit then, the documentation devicetree bindings example should help with anyone who is trying to bring up an image sensor. For reference sake I am thinking of putting up my ov5647 and tinkerbaord changes to a custom yocto layer which would make it that configuration (tinkerbaord + ov5647) specific. > > >> > >>> 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"; > >>> > >> > >
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";
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@gmail.com> Signed-off-by: Karthik Poduval <karthik.poduval@gmail.com> --- arch/arm/boot/dts/rk3288-tinker.dtsi | 37 ++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+)