| Message ID | 20210412075306.102884-2-tomi.valkeinen@ideasonboard.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] ARM: dts: dra76-evm: remove ov5640 | expand |
Hi Tomi, Thank you for the patch. On Mon, Apr 12, 2021 at 10:53:06AM +0300, Tomi Valkeinen wrote: > AM654 EVM boards are not shipped with OV5640 sensor module, it is a > separate purchase. OV5640 module is also just one of the possible > sensors or capture boards you can connect. > > However, for some reason, OV5640 has been added to the board dts file, > making it cumbersome to use other sensors. > > Remove the OV5640 from the dts file so that it is easy to use other > sensors via DT overlays. > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> > --- > .../arm64/boot/dts/ti/k3-am654-base-board.dts | 27 ------------------- > 1 file changed, 27 deletions(-) > > diff --git a/arch/arm64/boot/dts/ti/k3-am654-base-board.dts b/arch/arm64/boot/dts/ti/k3-am654-base-board.dts > index fe3043943906..76358b4944e1 100644 > --- a/arch/arm64/boot/dts/ti/k3-am654-base-board.dts > +++ b/arch/arm64/boot/dts/ti/k3-am654-base-board.dts > @@ -85,12 +85,6 @@ sw6 { > gpios = <&wkup_gpio0 27 GPIO_ACTIVE_LOW>; > }; > }; > - > - clk_ov5640_fixed: clock { > - compatible = "fixed-clock"; > - #clock-cells = <0>; > - clock-frequency = <24000000>; > - }; > }; > > &wkup_pmx0 { > @@ -288,22 +282,6 @@ &main_i2c1 { > pinctrl-0 = <&main_i2c1_pins_default>; > clock-frequency = <400000>; > > - ov5640: camera@3c { > - compatible = "ovti,ov5640"; > - reg = <0x3c>; > - > - clocks = <&clk_ov5640_fixed>; > - clock-names = "xclk"; > - > - port { > - csi2_cam0: endpoint { > - remote-endpoint = <&csi2_phy0>; > - clock-lanes = <0>; > - data-lanes = <1 2>; > - }; > - }; > - }; > - > }; As for patch 1/2, you could drop the two nodes completely. Same question about overlay availability. > > &main_i2c2 { > @@ -497,11 +475,6 @@ flash@0{ > }; > > &csi2_0 { > - csi2_phy0: endpoint { > - remote-endpoint = <&csi2_cam0>; > - clock-lanes = <0>; > - data-lanes = <1 2>; > - }; > }; > > &mcu_cpsw {
+ Vignesh On 12/04/21 11:00AM, Laurent Pinchart wrote: > Hi Tomi, > > Thank you for the patch. > > On Mon, Apr 12, 2021 at 10:53:06AM +0300, Tomi Valkeinen wrote: > > AM654 EVM boards are not shipped with OV5640 sensor module, it is a > > separate purchase. OV5640 module is also just one of the possible > > sensors or capture boards you can connect. > > > > However, for some reason, OV5640 has been added to the board dts file, > > making it cumbersome to use other sensors. > > > > Remove the OV5640 from the dts file so that it is easy to use other > > sensors via DT overlays. > > > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> > > --- > > .../arm64/boot/dts/ti/k3-am654-base-board.dts | 27 ------------------- > > 1 file changed, 27 deletions(-) > > > > diff --git a/arch/arm64/boot/dts/ti/k3-am654-base-board.dts b/arch/arm64/boot/dts/ti/k3-am654-base-board.dts > > index fe3043943906..76358b4944e1 100644 > > --- a/arch/arm64/boot/dts/ti/k3-am654-base-board.dts > > +++ b/arch/arm64/boot/dts/ti/k3-am654-base-board.dts > > @@ -85,12 +85,6 @@ sw6 { > > gpios = <&wkup_gpio0 27 GPIO_ACTIVE_LOW>; > > }; > > }; > > - > > - clk_ov5640_fixed: clock { > > - compatible = "fixed-clock"; > > - #clock-cells = <0>; > > - clock-frequency = <24000000>; > > - }; > > }; > > > > &wkup_pmx0 { > > @@ -288,22 +282,6 @@ &main_i2c1 { > > pinctrl-0 = <&main_i2c1_pins_default>; > > clock-frequency = <400000>; > > > > - ov5640: camera@3c { > > - compatible = "ovti,ov5640"; > > - reg = <0x3c>; > > - > > - clocks = <&clk_ov5640_fixed>; > > - clock-names = "xclk"; > > - > > - port { > > - csi2_cam0: endpoint { > > - remote-endpoint = <&csi2_phy0>; > > - clock-lanes = <0>; > > - data-lanes = <1 2>; > > - }; > > - }; > > - }; > > - > > }; > > As for patch 1/2, you could drop the two nodes completely. Same question > about overlay availability. The &main_i2c1 node was added much before the OV5640 node in 19a1768fc34a (arm64: dts: ti: k3-am654-base-board: Add I2C nodes, 2018-11-13). I wonder if there is any reason for having it present even if there are no subnodes. One reason that I can think of is that this node defines the pinmux configuration and clock frequency which makes more sense here than in an overlay. > > > > > &main_i2c2 { > > @@ -497,11 +475,6 @@ flash@0{ > > }; > > > > &csi2_0 { > > - csi2_phy0: endpoint { > > - remote-endpoint = <&csi2_cam0>; > > - clock-lanes = <0>; > > - data-lanes = <1 2>; > > - }; > > }; I agree with Laurent that the entire &csi2_0 node can be dropped. Have you tested the CAL driver with this node removed and no overlay to add it back? Can it handle the error gracefully or does it crash and burn? > > > > &mcu_cpsw { > > -- > Regards, > > Laurent Pinchart
On 12/04/2021 11:36, Pratyush Yadav wrote: > + Vignesh > > On 12/04/21 11:00AM, Laurent Pinchart wrote: >> Hi Tomi, >> >> Thank you for the patch. >> >> On Mon, Apr 12, 2021 at 10:53:06AM +0300, Tomi Valkeinen wrote: >>> AM654 EVM boards are not shipped with OV5640 sensor module, it is a >>> separate purchase. OV5640 module is also just one of the possible >>> sensors or capture boards you can connect. >>> >>> However, for some reason, OV5640 has been added to the board dts file, >>> making it cumbersome to use other sensors. >>> >>> Remove the OV5640 from the dts file so that it is easy to use other >>> sensors via DT overlays. >>> >>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> >>> --- >>> .../arm64/boot/dts/ti/k3-am654-base-board.dts | 27 ------------------- >>> 1 file changed, 27 deletions(-) >>> >>> diff --git a/arch/arm64/boot/dts/ti/k3-am654-base-board.dts b/arch/arm64/boot/dts/ti/k3-am654-base-board.dts >>> index fe3043943906..76358b4944e1 100644 >>> --- a/arch/arm64/boot/dts/ti/k3-am654-base-board.dts >>> +++ b/arch/arm64/boot/dts/ti/k3-am654-base-board.dts >>> @@ -85,12 +85,6 @@ sw6 { >>> gpios = <&wkup_gpio0 27 GPIO_ACTIVE_LOW>; >>> }; >>> }; >>> - >>> - clk_ov5640_fixed: clock { >>> - compatible = "fixed-clock"; >>> - #clock-cells = <0>; >>> - clock-frequency = <24000000>; >>> - }; >>> }; >>> >>> &wkup_pmx0 { >>> @@ -288,22 +282,6 @@ &main_i2c1 { >>> pinctrl-0 = <&main_i2c1_pins_default>; >>> clock-frequency = <400000>; >>> >>> - ov5640: camera@3c { >>> - compatible = "ovti,ov5640"; >>> - reg = <0x3c>; >>> - >>> - clocks = <&clk_ov5640_fixed>; >>> - clock-names = "xclk"; >>> - >>> - port { >>> - csi2_cam0: endpoint { >>> - remote-endpoint = <&csi2_phy0>; >>> - clock-lanes = <0>; >>> - data-lanes = <1 2>; >>> - }; >>> - }; >>> - }; >>> - >>> }; >> >> As for patch 1/2, you could drop the two nodes completely. Same question >> about overlay availability. > > The &main_i2c1 node was added much before the OV5640 node in > 19a1768fc34a (arm64: dts: ti: k3-am654-base-board: Add I2C nodes, > 2018-11-13). I wonder if there is any reason for having it present even > if there are no subnodes. One reason that I can think of is that this > node defines the pinmux configuration and clock frequency which makes > more sense here than in an overlay. Right, and we also have an empty main_i2c2 there. I'd rather keep empty main_i2c1 to be in line with main_i2c2, and to have the pinmux in the main dts file. Unless someone can say we can remove both main_i2c1 and main_i2c2. > >> >>> >>> &main_i2c2 { >>> @@ -497,11 +475,6 @@ flash@0{ >>> }; >>> >>> &csi2_0 { >>> - csi2_phy0: endpoint { >>> - remote-endpoint = <&csi2_cam0>; >>> - clock-lanes = <0>; >>> - data-lanes = <1 2>; >>> - }; >>> }; > > I agree with Laurent that the entire &csi2_0 node can be dropped. > > Have you tested the CAL driver with this node removed and no overlay to > add it back? Can it handle the error gracefully or does it crash and > burn? No, I haven't tested that for a while. Tomi
On 12/04/21 2:06 pm, Pratyush Yadav wrote: > + Vignesh > > On 12/04/21 11:00AM, Laurent Pinchart wrote: >> Hi Tomi, >> >> Thank you for the patch. >> >> On Mon, Apr 12, 2021 at 10:53:06AM +0300, Tomi Valkeinen wrote: >>> AM654 EVM boards are not shipped with OV5640 sensor module, it is a >>> separate purchase. OV5640 module is also just one of the possible >>> sensors or capture boards you can connect. >>> >>> However, for some reason, OV5640 has been added to the board dts file, >>> making it cumbersome to use other sensors. >>> >>> Remove the OV5640 from the dts file so that it is easy to use other >>> sensors via DT overlays. >>> >>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> >>> --- >>> .../arm64/boot/dts/ti/k3-am654-base-board.dts | 27 ------------------- >>> 1 file changed, 27 deletions(-) >>> >>> diff --git a/arch/arm64/boot/dts/ti/k3-am654-base-board.dts b/arch/arm64/boot/dts/ti/k3-am654-base-board.dts >>> index fe3043943906..76358b4944e1 100644 >>> --- a/arch/arm64/boot/dts/ti/k3-am654-base-board.dts >>> +++ b/arch/arm64/boot/dts/ti/k3-am654-base-board.dts >>> @@ -85,12 +85,6 @@ sw6 { >>> gpios = <&wkup_gpio0 27 GPIO_ACTIVE_LOW>; >>> }; >>> }; >>> - >>> - clk_ov5640_fixed: clock { >>> - compatible = "fixed-clock"; >>> - #clock-cells = <0>; >>> - clock-frequency = <24000000>; >>> - }; >>> }; >>> >>> &wkup_pmx0 { >>> @@ -288,22 +282,6 @@ &main_i2c1 { >>> pinctrl-0 = <&main_i2c1_pins_default>; >>> clock-frequency = <400000>; >>> >>> - ov5640: camera@3c { >>> - compatible = "ovti,ov5640"; >>> - reg = <0x3c>; >>> - >>> - clocks = <&clk_ov5640_fixed>; >>> - clock-names = "xclk"; >>> - >>> - port { >>> - csi2_cam0: endpoint { >>> - remote-endpoint = <&csi2_phy0>; >>> - clock-lanes = <0>; >>> - data-lanes = <1 2>; >>> - }; >>> - }; >>> - }; >>> - >>> }; >> >> As for patch 1/2, you could drop the two nodes completely. Same question >> about overlay availability. > > The &main_i2c1 node was added much before the OV5640 node in > 19a1768fc34a (arm64: dts: ti: k3-am654-base-board: Add I2C nodes, > 2018-11-13). I wonder if there is any reason for having it present even > if there are no subnodes. One reason that I can think of is that this > node defines the pinmux configuration and clock frequency which makes > more sense here than in an overlay. > No, please don't drop main_i2c1 node. As long as pinmux is setup, its possible to communicate with I2C devices from user space too even when there are no subnodes.
On 10:53-20210412, Tomi Valkeinen wrote: > AM654 EVM boards are not shipped with OV5640 sensor module, it is a > separate purchase. OV5640 module is also just one of the possible > sensors or capture boards you can connect. > > However, for some reason, OV5640 has been added to the board dts file, > making it cumbersome to use other sensors. > > Remove the OV5640 from the dts file so that it is easy to use other > sensors via DT overlays. > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> > --- > .../arm64/boot/dts/ti/k3-am654-base-board.dts | 27 ------------------- > 1 file changed, 27 deletions(-) > > diff --git a/arch/arm64/boot/dts/ti/k3-am654-base-board.dts b/arch/arm64/boot/dts/ti/k3-am654-base-board.dts > index fe3043943906..76358b4944e1 100644 > --- a/arch/arm64/boot/dts/ti/k3-am654-base-board.dts > +++ b/arch/arm64/boot/dts/ti/k3-am654-base-board.dts will be good if you can submit this patch separately. dra7 and am654 are maintained in different trees.
diff --git a/arch/arm64/boot/dts/ti/k3-am654-base-board.dts b/arch/arm64/boot/dts/ti/k3-am654-base-board.dts index fe3043943906..76358b4944e1 100644 --- a/arch/arm64/boot/dts/ti/k3-am654-base-board.dts +++ b/arch/arm64/boot/dts/ti/k3-am654-base-board.dts @@ -85,12 +85,6 @@ sw6 { gpios = <&wkup_gpio0 27 GPIO_ACTIVE_LOW>; }; }; - - clk_ov5640_fixed: clock { - compatible = "fixed-clock"; - #clock-cells = <0>; - clock-frequency = <24000000>; - }; }; &wkup_pmx0 { @@ -288,22 +282,6 @@ &main_i2c1 { pinctrl-0 = <&main_i2c1_pins_default>; clock-frequency = <400000>; - ov5640: camera@3c { - compatible = "ovti,ov5640"; - reg = <0x3c>; - - clocks = <&clk_ov5640_fixed>; - clock-names = "xclk"; - - port { - csi2_cam0: endpoint { - remote-endpoint = <&csi2_phy0>; - clock-lanes = <0>; - data-lanes = <1 2>; - }; - }; - }; - }; &main_i2c2 { @@ -497,11 +475,6 @@ flash@0{ }; &csi2_0 { - csi2_phy0: endpoint { - remote-endpoint = <&csi2_cam0>; - clock-lanes = <0>; - data-lanes = <1 2>; - }; }; &mcu_cpsw {
AM654 EVM boards are not shipped with OV5640 sensor module, it is a separate purchase. OV5640 module is also just one of the possible sensors or capture boards you can connect. However, for some reason, OV5640 has been added to the board dts file, making it cumbersome to use other sensors. Remove the OV5640 from the dts file so that it is easy to use other sensors via DT overlays. Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> --- .../arm64/boot/dts/ti/k3-am654-base-board.dts | 27 ------------------- 1 file changed, 27 deletions(-)