| Message ID | 20230901155732.252436-5-alain.volmat@foss.st.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add support for DCMIPP camera interface of STMicroelectronics STM32 SoC series | expand |
Hi Alain, Thank you for the patch. On Fri, Sep 01, 2023 at 05:57:23PM +0200, Alain Volmat wrote: > From: Hugues Fruchet <hugues.fruchet@foss.st.com> > > Add dcmipp support to STM32MP135. > > Signed-off-by: Hugues Fruchet <hugues.fruchet@foss.st.com> > Signed-off-by: Alain Volmat <alain.volmat@foss.st.com> > --- > arch/arm/boot/dts/st/stm32mp135.dtsi | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/arch/arm/boot/dts/st/stm32mp135.dtsi b/arch/arm/boot/dts/st/stm32mp135.dtsi > index abf2acd37b4e..beee9ec7ed0d 100644 > --- a/arch/arm/boot/dts/st/stm32mp135.dtsi > +++ b/arch/arm/boot/dts/st/stm32mp135.dtsi > @@ -8,5 +8,13 @@ > > / { > soc { > + dcmipp: dcmipp@5a000000 { > + compatible = "st,stm32mp13-dcmipp"; > + reg = <0x5a000000 0x400>; > + interrupts = <GIC_SPI 79 IRQ_TYPE_LEVEL_HIGH>; > + resets = <&rcc DCMIPP_R>; > + clocks = <&rcc DCMIPP_K>; > + status = "disabled"; This needs a port, as it's marked as required in the bindings. You can leave the endpoint out. With this fixed, Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > + }; > }; > };
Hi Laurent, On Tue, Sep 05, 2023 at 12:02:58PM +0300, Laurent Pinchart wrote: > Hi Alain, > > Thank you for the patch. > > On Fri, Sep 01, 2023 at 05:57:23PM +0200, Alain Volmat wrote: > > From: Hugues Fruchet <hugues.fruchet@foss.st.com> > > > > Add dcmipp support to STM32MP135. > > > > Signed-off-by: Hugues Fruchet <hugues.fruchet@foss.st.com> > > Signed-off-by: Alain Volmat <alain.volmat@foss.st.com> > > --- > > arch/arm/boot/dts/st/stm32mp135.dtsi | 8 ++++++++ > > 1 file changed, 8 insertions(+) > > > > diff --git a/arch/arm/boot/dts/st/stm32mp135.dtsi b/arch/arm/boot/dts/st/stm32mp135.dtsi > > index abf2acd37b4e..beee9ec7ed0d 100644 > > --- a/arch/arm/boot/dts/st/stm32mp135.dtsi > > +++ b/arch/arm/boot/dts/st/stm32mp135.dtsi > > @@ -8,5 +8,13 @@ > > > > / { > > soc { > > + dcmipp: dcmipp@5a000000 { > > + compatible = "st,stm32mp13-dcmipp"; > > + reg = <0x5a000000 0x400>; > > + interrupts = <GIC_SPI 79 IRQ_TYPE_LEVEL_HIGH>; > > + resets = <&rcc DCMIPP_R>; > > + clocks = <&rcc DCMIPP_K>; > > + status = "disabled"; > > This needs a port, as it's marked as required in the bindings. You can > leave the endpoint out. I first agreed with your comment but, having done the check (make CHECK_DTBS=y ...) this doesn't seem to be required because the dcmipp node is kept disabled within our dtsi. (it is later on only enabled in dts file which as well have the port property). Indeed, to check this I changed it to okay and DTC_CHK complained about missing port property. Hence, I'd think that port doesn't have to be added in this dtsi file. Would you agree with that ? Regards, Alain > > With this fixed, > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > + }; > > }; > > }; > > -- > Regards, > > Laurent Pinchart
On Fri, Sep 22, 2023 at 06:02:27PM +0200, Alain Volmat wrote: > On Tue, Sep 05, 2023 at 12:02:58PM +0300, Laurent Pinchart wrote: > > On Fri, Sep 01, 2023 at 05:57:23PM +0200, Alain Volmat wrote: > > > From: Hugues Fruchet <hugues.fruchet@foss.st.com> > > > > > > Add dcmipp support to STM32MP135. > > > > > > Signed-off-by: Hugues Fruchet <hugues.fruchet@foss.st.com> > > > Signed-off-by: Alain Volmat <alain.volmat@foss.st.com> > > > --- > > > arch/arm/boot/dts/st/stm32mp135.dtsi | 8 ++++++++ > > > 1 file changed, 8 insertions(+) > > > > > > diff --git a/arch/arm/boot/dts/st/stm32mp135.dtsi b/arch/arm/boot/dts/st/stm32mp135.dtsi > > > index abf2acd37b4e..beee9ec7ed0d 100644 > > > --- a/arch/arm/boot/dts/st/stm32mp135.dtsi > > > +++ b/arch/arm/boot/dts/st/stm32mp135.dtsi > > > @@ -8,5 +8,13 @@ > > > > > > / { > > > soc { > > > + dcmipp: dcmipp@5a000000 { > > > + compatible = "st,stm32mp13-dcmipp"; > > > + reg = <0x5a000000 0x400>; > > > + interrupts = <GIC_SPI 79 IRQ_TYPE_LEVEL_HIGH>; > > > + resets = <&rcc DCMIPP_R>; > > > + clocks = <&rcc DCMIPP_K>; > > > + status = "disabled"; > > > > This needs a port, as it's marked as required in the bindings. You can > > leave the endpoint out. > > I first agreed with your comment but, having done the check (make > CHECK_DTBS=y ...) this doesn't seem to be required because the dcmipp > node is kept disabled within our dtsi. Interesting. > (it is later on only enabled in dts file which as well have the port > property). > Indeed, to check this I changed it to okay and DTC_CHK complained about > missing port property. > > Hence, I'd think that port doesn't have to be added in this dtsi file. > Would you agree with that ? I still think the port belongs here, as it's an intrinsic property of the dcmipp, not a property of the board. Does it cause any issue to add a port in the .dtsi ? > > With this fixed, > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > > + }; > > > }; > > > };
Hi Laurent, On Fri, Sep 22, 2023 at 07:08:18PM +0300, Laurent Pinchart wrote: > On Fri, Sep 22, 2023 at 06:02:27PM +0200, Alain Volmat wrote: > > On Tue, Sep 05, 2023 at 12:02:58PM +0300, Laurent Pinchart wrote: > > > On Fri, Sep 01, 2023 at 05:57:23PM +0200, Alain Volmat wrote: > > > > From: Hugues Fruchet <hugues.fruchet@foss.st.com> > > > > > > > > Add dcmipp support to STM32MP135. > > > > > > > > Signed-off-by: Hugues Fruchet <hugues.fruchet@foss.st.com> > > > > Signed-off-by: Alain Volmat <alain.volmat@foss.st.com> > > > > --- > > > > arch/arm/boot/dts/st/stm32mp135.dtsi | 8 ++++++++ > > > > 1 file changed, 8 insertions(+) > > > > > > > > diff --git a/arch/arm/boot/dts/st/stm32mp135.dtsi b/arch/arm/boot/dts/st/stm32mp135.dtsi > > > > index abf2acd37b4e..beee9ec7ed0d 100644 > > > > --- a/arch/arm/boot/dts/st/stm32mp135.dtsi > > > > +++ b/arch/arm/boot/dts/st/stm32mp135.dtsi > > > > @@ -8,5 +8,13 @@ > > > > > > > > / { > > > > soc { > > > > + dcmipp: dcmipp@5a000000 { > > > > + compatible = "st,stm32mp13-dcmipp"; > > > > + reg = <0x5a000000 0x400>; > > > > + interrupts = <GIC_SPI 79 IRQ_TYPE_LEVEL_HIGH>; > > > > + resets = <&rcc DCMIPP_R>; > > > > + clocks = <&rcc DCMIPP_K>; > > > > + status = "disabled"; > > > > > > This needs a port, as it's marked as required in the bindings. You can > > > leave the endpoint out. > > > > I first agreed with your comment but, having done the check (make > > CHECK_DTBS=y ...) this doesn't seem to be required because the dcmipp > > node is kept disabled within our dtsi. > > Interesting. > > > (it is later on only enabled in dts file which as well have the port > > property). > > Indeed, to check this I changed it to okay and DTC_CHK complained about > > missing port property. > > > > Hence, I'd think that port doesn't have to be added in this dtsi file. > > Would you agree with that ? > > I still think the port belongs here, as it's an intrinsic property of > the dcmipp, not a property of the board. Does it cause any issue to add > a port in the .dtsi ? I agree that the port refers more to the SoC (hence dtsi) rather than the board (hence dts), however I am wondering if this is really something usually done. I had a look at other dtsi with node related to similar kind of devices and it seems to me that there is no such case of a dtsi with a port having nothing in it. Did I missed something ? > > > > With this fixed, > > > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > > > > + }; > > > > }; > > > > }; > > -- > Regards, > > Laurent Pinchart
On Mon, Sep 25, 2023 at 01:35:42PM +0200, Alain Volmat wrote: > On Fri, Sep 22, 2023 at 07:08:18PM +0300, Laurent Pinchart wrote: > > On Fri, Sep 22, 2023 at 06:02:27PM +0200, Alain Volmat wrote: > > > On Tue, Sep 05, 2023 at 12:02:58PM +0300, Laurent Pinchart wrote: > > > > On Fri, Sep 01, 2023 at 05:57:23PM +0200, Alain Volmat wrote: > > > > > From: Hugues Fruchet <hugues.fruchet@foss.st.com> > > > > > > > > > > Add dcmipp support to STM32MP135. > > > > > > > > > > Signed-off-by: Hugues Fruchet <hugues.fruchet@foss.st.com> > > > > > Signed-off-by: Alain Volmat <alain.volmat@foss.st.com> > > > > > --- > > > > > arch/arm/boot/dts/st/stm32mp135.dtsi | 8 ++++++++ > > > > > 1 file changed, 8 insertions(+) > > > > > > > > > > diff --git a/arch/arm/boot/dts/st/stm32mp135.dtsi b/arch/arm/boot/dts/st/stm32mp135.dtsi > > > > > index abf2acd37b4e..beee9ec7ed0d 100644 > > > > > --- a/arch/arm/boot/dts/st/stm32mp135.dtsi > > > > > +++ b/arch/arm/boot/dts/st/stm32mp135.dtsi > > > > > @@ -8,5 +8,13 @@ > > > > > > > > > > / { > > > > > soc { > > > > > + dcmipp: dcmipp@5a000000 { > > > > > + compatible = "st,stm32mp13-dcmipp"; > > > > > + reg = <0x5a000000 0x400>; > > > > > + interrupts = <GIC_SPI 79 IRQ_TYPE_LEVEL_HIGH>; > > > > > + resets = <&rcc DCMIPP_R>; > > > > > + clocks = <&rcc DCMIPP_K>; > > > > > + status = "disabled"; > > > > > > > > This needs a port, as it's marked as required in the bindings. You can > > > > leave the endpoint out. > > > > > > I first agreed with your comment but, having done the check (make > > > CHECK_DTBS=y ...) this doesn't seem to be required because the dcmipp > > > node is kept disabled within our dtsi. > > > > Interesting. > > > > > (it is later on only enabled in dts file which as well have the port > > > property). > > > Indeed, to check this I changed it to okay and DTC_CHK complained about > > > missing port property. > > > > > > Hence, I'd think that port doesn't have to be added in this dtsi file. > > > Would you agree with that ? > > > > I still think the port belongs here, as it's an intrinsic property of > > the dcmipp, not a property of the board. Does it cause any issue to add > > a port in the .dtsi ? > > I agree that the port refers more to the SoC (hence dtsi) rather than > the board (hence dts), however I am wondering if this is really > something usually done. I had a look at other dtsi with node related > to similar kind of devices and it seems to me that there is no such case > of a dtsi with a port having nothing in it. Did I missed something ? Look at the csi@32e4000 and csi@32e5000 nodes in arch/arm64/boot/dts/freescale/imx8mp.dtsi for instance. There are quite a few other examples. > > > > With this fixed, > > > > > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > > > > > > + }; > > > > > }; > > > > > };
Hi Laurent, On Mon, Sep 25, 2023 at 02:43:32PM +0300, Laurent Pinchart wrote: > On Mon, Sep 25, 2023 at 01:35:42PM +0200, Alain Volmat wrote: > > On Fri, Sep 22, 2023 at 07:08:18PM +0300, Laurent Pinchart wrote: > > > On Fri, Sep 22, 2023 at 06:02:27PM +0200, Alain Volmat wrote: > > > > On Tue, Sep 05, 2023 at 12:02:58PM +0300, Laurent Pinchart wrote: > > > > > On Fri, Sep 01, 2023 at 05:57:23PM +0200, Alain Volmat wrote: > > > > > > From: Hugues Fruchet <hugues.fruchet@foss.st.com> > > > > > > > > > > > > Add dcmipp support to STM32MP135. > > > > > > > > > > > > Signed-off-by: Hugues Fruchet <hugues.fruchet@foss.st.com> > > > > > > Signed-off-by: Alain Volmat <alain.volmat@foss.st.com> > > > > > > --- > > > > > > arch/arm/boot/dts/st/stm32mp135.dtsi | 8 ++++++++ > > > > > > 1 file changed, 8 insertions(+) > > > > > > > > > > > > diff --git a/arch/arm/boot/dts/st/stm32mp135.dtsi b/arch/arm/boot/dts/st/stm32mp135.dtsi > > > > > > index abf2acd37b4e..beee9ec7ed0d 100644 > > > > > > --- a/arch/arm/boot/dts/st/stm32mp135.dtsi > > > > > > +++ b/arch/arm/boot/dts/st/stm32mp135.dtsi > > > > > > @@ -8,5 +8,13 @@ > > > > > > > > > > > > / { > > > > > > soc { > > > > > > + dcmipp: dcmipp@5a000000 { > > > > > > + compatible = "st,stm32mp13-dcmipp"; > > > > > > + reg = <0x5a000000 0x400>; > > > > > > + interrupts = <GIC_SPI 79 IRQ_TYPE_LEVEL_HIGH>; > > > > > > + resets = <&rcc DCMIPP_R>; > > > > > > + clocks = <&rcc DCMIPP_K>; > > > > > > + status = "disabled"; > > > > > > > > > > This needs a port, as it's marked as required in the bindings. You can > > > > > leave the endpoint out. > > > > > > > > I first agreed with your comment but, having done the check (make > > > > CHECK_DTBS=y ...) this doesn't seem to be required because the dcmipp > > > > node is kept disabled within our dtsi. > > > > > > Interesting. > > > > > > > (it is later on only enabled in dts file which as well have the port > > > > property). > > > > Indeed, to check this I changed it to okay and DTC_CHK complained about > > > > missing port property. > > > > > > > > Hence, I'd think that port doesn't have to be added in this dtsi file. > > > > Would you agree with that ? > > > > > > I still think the port belongs here, as it's an intrinsic property of > > > the dcmipp, not a property of the board. Does it cause any issue to add > > > a port in the .dtsi ? > > > > I agree that the port refers more to the SoC (hence dtsi) rather than > > the board (hence dts), however I am wondering if this is really > > something usually done. I had a look at other dtsi with node related > > to similar kind of devices and it seems to me that there is no such case > > of a dtsi with a port having nothing in it. Did I missed something ? > > Look at the csi@32e4000 and csi@32e5000 nodes in > arch/arm64/boot/dts/freescale/imx8mp.dtsi for instance. There are quite > a few other examples. Ok, thanks for pointer. Understood, I add an empty port child within the node. I've also covered the points of your review of the v3 and post now the v4. > > > > > > With this fixed, > > > > > > > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > > > > > > > > + }; > > > > > > }; > > > > > > }; > > -- > Regards, > > Laurent Pinchart
diff --git a/arch/arm/boot/dts/st/stm32mp135.dtsi b/arch/arm/boot/dts/st/stm32mp135.dtsi index abf2acd37b4e..beee9ec7ed0d 100644 --- a/arch/arm/boot/dts/st/stm32mp135.dtsi +++ b/arch/arm/boot/dts/st/stm32mp135.dtsi @@ -8,5 +8,13 @@ / { soc { + dcmipp: dcmipp@5a000000 { + compatible = "st,stm32mp13-dcmipp"; + reg = <0x5a000000 0x400>; + interrupts = <GIC_SPI 79 IRQ_TYPE_LEVEL_HIGH>; + resets = <&rcc DCMIPP_R>; + clocks = <&rcc DCMIPP_K>; + status = "disabled"; + }; }; };