| Message ID | 20220516115846.58328-1-max.oss.09@gmail.com |
|---|---|
| Headers | show |
| Series | ARM: dts: imx6q-apalis: Misc improvements and newly added carrier | expand |
Hi Max, On Mon, May 16, 2022 at 8:59 AM Max Krummenacher <max.oss.09@gmail.com> wrote: > > From: Oleksandr Suvorov <oleksandr.suvorov@toradex.com> > > The Apalis iMX6 modules allow connecting a mipi-csi video input. > Add support for our OV5640 camera module but have it disabled. > This allows to enable it in an overlay per the current system > configuration. > > Signed-off-by: Oleksandr Suvorov <oleksandr.suvorov@toradex.com> > Signed-off-by: Max Krummenacher <max.krummenacher@toradex.com> > --- > > arch/arm/boot/dts/imx6qdl-apalis.dtsi | 67 ++++++++++++++++++++++++++- > 1 file changed, 66 insertions(+), 1 deletion(-) > > diff --git a/arch/arm/boot/dts/imx6qdl-apalis.dtsi b/arch/arm/boot/dts/imx6qdl-apalis.dtsi > index 506d040ea37a..0d1004eede62 100644 > --- a/arch/arm/boot/dts/imx6qdl-apalis.dtsi > +++ b/arch/arm/boot/dts/imx6qdl-apalis.dtsi > @@ -29,6 +29,12 @@ > status = "disabled"; > }; > > + clk_ov5640_osc: clk_ov5640_osc_int { Node names should have "-", not "_" clk_ov5640_osc: clk-ov5640-osc Also, no need for the _int suffix. Just curious: is ov5640 mipi support functional? I recalled that I had issues in getting Gstreamer pipeline to capture from the ov5640 mipi. There were some errors related to LP-11 during the start of the capture.
Hi Max, On Mon, May 16, 2022 at 8:59 AM Max Krummenacher <max.oss.09@gmail.com> wrote: > + adv_7280: adv7280@21 { > + compatible = "adi,adv7280"; > + reg = <0x21>; > + pinctrl-names = "default"; > + pinctrl-0 = <&pinctrl_ipu1_csi0>; I suggest passing "adv,force-bt656-4" property as this fixes sync problems.
On 16.05.22 13:58, Max Krummenacher wrote: > From: Max Krummenacher <max.krummenacher@toradex.com> > > The STMPE MFD device binding requires the child node to have a fixed > name, i.e. with '_', not '-'. Otherwise the stmpe_adc, stmpe_touchscreen > drivers will not be probed. IMO, the Linux driver should be fixed and the requirement to use a fixed node name be dropped from the binding. The driver itself already probes by compatible, the node name seems only to be used by the MFD driver to detect which functions to enable. It could do the same by checking for compatibles. Otherwise you invite a game of cat and mouse, where in future, this is changed back again reintroducing the regression.. > Fixes: 56086b5e804f ("ARM: dts: imx6qdl-apalis: Avoid underscore in node name") > Signed-off-by: Max Krummenacher <max.krummenacher@toradex.com> Anyway, as a fix, this looks ok for now: Reviewed-by: Ahmad Fatoum <a.fatoum@pengutronix.de> Cheers, Ahmad > --- > > arch/arm/boot/dts/imx6qdl-apalis.dtsi | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/arch/arm/boot/dts/imx6qdl-apalis.dtsi b/arch/arm/boot/dts/imx6qdl-apalis.dtsi > index bd763bae596b..da919d0544a8 100644 > --- a/arch/arm/boot/dts/imx6qdl-apalis.dtsi > +++ b/arch/arm/boot/dts/imx6qdl-apalis.dtsi > @@ -315,7 +315,7 @@ > /* ADC conversion time: 80 clocks */ > st,sample-time = <4>; > > - stmpe_touchscreen: stmpe-touchscreen { > + stmpe_touchscreen: stmpe_touchscreen { > compatible = "st,stmpe-ts"; > /* 8 sample average control */ > st,ave-ctrl = <3>; > @@ -332,7 +332,7 @@ > st,touch-det-delay = <5>; > }; > > - stmpe_adc: stmpe-adc { > + stmpe_adc: stmpe_adc { > compatible = "st,stmpe-adc"; > /* forbid to use ADC channels 3-0 (touch) */ > st,norequest-mask = <0x0F>;
On Mon, May 16, 2022 at 02:49:12PM +0200, Ahmad Fatoum wrote: > On 16.05.22 13:58, Max Krummenacher wrote: > > From: Max Krummenacher <max.krummenacher@toradex.com> > > > > The STMPE MFD device binding requires the child node to have a fixed > > name, i.e. with '_', not '-'. Otherwise the stmpe_adc, stmpe_touchscreen > > drivers will not be probed. > > IMO, the Linux driver should be fixed and the requirement to use a fixed > node name be dropped from the binding. The driver itself already probes > by compatible, the node name seems only to be used by the MFD driver to > detect which functions to enable. It could do the same by checking for > compatibles. Otherwise you invite a game of cat and mouse, where in > future, this is changed back again reintroducing the regression.. How would you handle in general such kind of change? Would you keep the driver probing for both the old name with the `_` and the compatible or you just break the compatibility? Francesco
Hello Francesco, On 16.05.22 16:53, Francesco Dolcini wrote: > On Mon, May 16, 2022 at 02:49:12PM +0200, Ahmad Fatoum wrote: >> On 16.05.22 13:58, Max Krummenacher wrote: >>> From: Max Krummenacher <max.krummenacher@toradex.com> >>> >>> The STMPE MFD device binding requires the child node to have a fixed >>> name, i.e. with '_', not '-'. Otherwise the stmpe_adc, stmpe_touchscreen >>> drivers will not be probed. >> >> IMO, the Linux driver should be fixed and the requirement to use a fixed >> node name be dropped from the binding. The driver itself already probes >> by compatible, the node name seems only to be used by the MFD driver to >> detect which functions to enable. It could do the same by checking for >> compatibles. Otherwise you invite a game of cat and mouse, where in >> future, this is changed back again reintroducing the regression.. > > How would you handle in general such kind of change? Would you keep the > driver probing for both the old name with the `_` and the compatible or > you just break the compatibility? The Binding requires child nodes to have both a specific node name and compatible. So if we remove the node name restriction, we stay compliant to the binding. The MFD driver requires specific node names, while the MFD cell drivers seem to be matched by compatibles. It's thus should be safe to replace the node name readout in the MFD driver with a compatible check. Existing device trees will continue to work. Newer device trees can use dashes. Once the binding is converted to YAML, we could enforce a name to get everyone aligned, but it will be just a binding checker warning, not a breakage on update. Cheers, Ahmad > > Francesco > >
Hi Max, On Mon, May 16, 2022 at 8:59 AM Max Krummenacher <max.oss.09@gmail.com> wrote: > + ov5640_csi_cam: ov5640_mipi@3c { > + compatible = "ovti,ov5640"; Does it make sense to describe the ov5640 camera in the Apalis SoM dtsi? The camera is not populated in the SoM. What if the customer baseboard uses a different camera? The same applies to the adv720 description.
Hi Fabio On Mon, May 16, 2022 at 2:10 PM Fabio Estevam <festevam@gmail.com> wrote: > > Hi Max, > > On Mon, May 16, 2022 at 8:59 AM Max Krummenacher <max.oss.09@gmail.com> wrote: > > > + adv_7280: adv7280@21 { > > + compatible = "adi,adv7280"; > > + reg = <0x21>; > > + pinctrl-names = "default"; > > + pinctrl-0 = <&pinctrl_ipu1_csi0>; > > I suggest passing "adv,force-bt656-4" property as this fixes sync problems. I will add the property in a V2 of the patchset. Thanks for the suggestion. With my setup, i.e. PAL camera as the signal source I see no noticeable change though. Max
Hi Max, On Wed, May 18, 2022 at 11:07 AM Max Krummenacher <max.oss.09@gmail.com> wrote: > I will add the property in a V2 of the patchset. Thanks for the suggestion. > With my setup, i.e. PAL camera as the signal source I see no > noticeable change though. Correct. This property helps to fix the sync with NTSC video.
Hi Fabio On Mon, May 16, 2022 at 2:08 PM Fabio Estevam <festevam@gmail.com> wrote: > > Hi Max, > > On Mon, May 16, 2022 at 8:59 AM Max Krummenacher <max.oss.09@gmail.com> wrote: > > > > From: Oleksandr Suvorov <oleksandr.suvorov@toradex.com> > > > > The Apalis iMX6 modules allow connecting a mipi-csi video input. > > Add support for our OV5640 camera module but have it disabled. > > This allows to enable it in an overlay per the current system > > configuration. > > > > Signed-off-by: Oleksandr Suvorov <oleksandr.suvorov@toradex.com> > > Signed-off-by: Max Krummenacher <max.krummenacher@toradex.com> > > --- > > > > arch/arm/boot/dts/imx6qdl-apalis.dtsi | 67 ++++++++++++++++++++++++++- > > 1 file changed, 66 insertions(+), 1 deletion(-) > > > > diff --git a/arch/arm/boot/dts/imx6qdl-apalis.dtsi b/arch/arm/boot/dts/imx6qdl-apalis.dtsi > > index 506d040ea37a..0d1004eede62 100644 > > --- a/arch/arm/boot/dts/imx6qdl-apalis.dtsi > > +++ b/arch/arm/boot/dts/imx6qdl-apalis.dtsi > > @@ -29,6 +29,12 @@ > > status = "disabled"; > > }; > > > > + clk_ov5640_osc: clk_ov5640_osc_int { > > Node names should have "-", not "_" Ups, missed that one. Will fix it in V2. Thanks. > > clk_ov5640_osc: clk-ov5640-osc > > Also, no need for the _int suffix. > > Just curious: is ov5640 mipi support functional? > > I recalled that I had issues in getting Gstreamer pipeline to capture > from the ov5640 mipi. > > There were some errors related to LP-11 during the start of the capture. In my (limited) testing I saw no issues that the camera would not come up. It takes 2 to 3 seconds until the pipeline is running but I noted no hickups. I did this with setting the resolution to 640x480 and 1920x1080 which probably would not relate to the LP-11 issue you mention. I.e. I set up the v4l2 pipeline and start capture to the screen as follows: ``` media-ctl -l "'ov5640 1-003c':0 -> 'imx6-mipi-csi2':0[1]" media-ctl -l "'imx6-mipi-csi2':2 -> 'ipu1_csi1':0[1]" media-ctl -l "'ipu1_csi1':2 -> 'ipu1_csi1 capture':0[1]" media-ctl -V "'ov5640 1-003c':0 [fmt:UYVY2X8/640x480 field:none]" media-ctl -V "'imx6-mipi-csi2':2 [fmt:UYVY2X8/640x480 field:none]" media-ctl -V "'ipu1_csi1':2 [fmt:AYUV32/640x480 field:none]" gst-launch-1.0 v4l2src device='/dev/video0' ! videoconvert ! waylandsink ``` Max
Hi Max, On Wed, May 18, 2022 at 11:21 AM Max Krummenacher <max.oss.09@gmail.com> wrote: > In my (limited) testing I saw no issues that the camera would not come up. > It takes 2 to 3 seconds until the pipeline is running but I noted no hickups. > I did this with setting the resolution to 640x480 and 1920x1080 which > probably would not relate to the LP-11 issue you mention. > > I.e. I set up the v4l2 pipeline and start capture to the screen as follows: Just curious: which baseboard did you use to test the ov5640 camera? Most likely there was an issue with the customer's baseboard that prevented the camera capture to start. Thanks
Hi Fabio On Tue, May 17, 2022 at 8:28 PM Fabio Estevam <festevam@gmail.com> wrote: > > Hi Max, > > On Mon, May 16, 2022 at 8:59 AM Max Krummenacher <max.oss.09@gmail.com> wrote: > > > + ov5640_csi_cam: ov5640_mipi@3c { > > + compatible = "ovti,ov5640"; > > Does it make sense to describe the ov5640 camera in the Apalis SoM dtsi? > > The camera is not populated in the SoM. What if the customer baseboard > uses a different camera? > > The same applies to the adv720 description. We moved to a pattern where we do describe the 'Toradex' peripherals in the SoM dtsi but keep their status disabled. Then if the peripheral is on the carrier board we only have to enable it in the carrier board dts, for peripherals attached to the carrier boards (e.g. like the cameras) we enable it in device tree overlays. This did reduce code duplication a lot. A customer who attaches a different camera would add its camera node in its device tree or overlay and keep the ov5640 or adv7280 nodes disabled. So I would rather not change anything here. Max
Hi Fabio On Wed, May 18, 2022 at 4:25 PM Fabio Estevam <festevam@gmail.com> wrote: > > Hi Max, > > On Wed, May 18, 2022 at 11:21 AM Max Krummenacher <max.oss.09@gmail.com> wrote: > > > In my (limited) testing I saw no issues that the camera would not come up. > > It takes 2 to 3 seconds until the pipeline is running but I noted no hickups. > > I did this with setting the resolution to 640x480 and 1920x1080 which > > probably would not relate to the LP-11 issue you mention. > > > > I.e. I set up the v4l2 pipeline and start capture to the screen as follows: > > Just curious: which baseboard did you use to test the ov5640 camera? > > Most likely there was an issue with the customer's baseboard that > prevented the camera capture to start. I used an 'Apalis Evaluation Board' with an 'Apalis iMX6 Mezannine V2.0A'. Now I did retry and it works equally well on an 'Ixora V1.1A' Note that in the beginning the camera module required the SoM to provide a 24MHz clock. The V1.1B HW version then got its local oscillator and the clock input no longer requires a clock. That may or may not have been the issue your customer saw. Max
On Mon, May 16, 2022 at 01:58:30PM +0200, Max Krummenacher wrote: > From: Max Krummenacher <max.krummenacher@toradex.com> > > The STMPE MFD device binding requires the child node to have a fixed > name, i.e. with '_', not '-'. Otherwise the stmpe_adc, stmpe_touchscreen > drivers will not be probed. > > Fixes: 56086b5e804f ("ARM: dts: imx6qdl-apalis: Avoid underscore in node name") > Signed-off-by: Max Krummenacher <max.krummenacher@toradex.com> > --- > > arch/arm/boot/dts/imx6qdl-apalis.dtsi | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/arch/arm/boot/dts/imx6qdl-apalis.dtsi b/arch/arm/boot/dts/imx6qdl-apalis.dtsi > index bd763bae596b..da919d0544a8 100644 > --- a/arch/arm/boot/dts/imx6qdl-apalis.dtsi > +++ b/arch/arm/boot/dts/imx6qdl-apalis.dtsi > @@ -315,7 +315,7 @@ > /* ADC conversion time: 80 clocks */ > st,sample-time = <4>; > > - stmpe_touchscreen: stmpe-touchscreen { > + stmpe_touchscreen: stmpe_touchscreen { In any case, the correct name would have been 'touchscreen' and 'adc'. > compatible = "st,stmpe-ts"; > /* 8 sample average control */ > st,ave-ctrl = <3>; > @@ -332,7 +332,7 @@ > st,touch-det-delay = <5>; > }; > > - stmpe_adc: stmpe-adc { > + stmpe_adc: stmpe_adc { > compatible = "st,stmpe-adc"; > /* forbid to use ADC channels 3-0 (touch) */ > st,norequest-mask = <0x0F>; > -- > 2.20.1 >
From: Max Krummenacher <max.krummenacher@toradex.com> Shawn, this patchset reworks a lot of the imx6qdl-apalis. Commit fa51e1dc4b91 ("ARM: dts: imx6qdl-apalis: Fix sgtl5000 detection issue") which is in imx/fixes also touches the file. To keep the rebasing work minimal I based the series on top of imx/dt, but did additionally cherry-pick commit fa51e1dc4b91. Is this the way to go in such circumstances? This is a general update of the Apalis iMX6 device tree files. The Toradex Apalis family is composed of a SoM that can be plugged on various carrier boards, with carrier boards allowing multiple optional accessories (e.g. display, camera, ...). The device tree sources are structured into a SoM dtsi and a carrier dts which then includes the SoM dtsi. The SoM dtsi defines and enables the functionality self contained on the SoM and prepares for functionality provided by the carrier HW or accessories, so that the carrier dts then can enable or amend nodes provided. Accessories are enabled in overlays depending on HW configuration. The series improves the existing Apalis carrier board device trees and adds a new device trees for the Ixora V1.2 carrier board. Improvements: - Specifies GPIO line names for use with libgpiod. - Disables optional accessories. They would be enabled in overlays depending on HW configuration. - Lower power consumption after poweroff. - Move more functionality into the SoM dtsi file to reduce code duplication. - General cleanup to adhere to dtbs bindings and missed alphabetically ordering. - PWM backlight for backlights with inverted logic on its PWM input. Fixes: - STMPE ADC not functional due to wrong node name in dts. Adds: - imx6q-apalis-ixora-v1.2.dtb: used for a Apalis iMX6 mated in an Ixora V1.2 carrier board. - Adds disable support for a OV5640 MIPI-CSI2 Camera and a ADV7280 Video ADC on a parallel video input. Denys Drozdov (1): ARM: dts: imx6q-apalis: Clean-up sd card support Max Krummenacher (10): dt-bindings: arm: fsl: Add carrier for toradex,apalis-imx6q Revert "ARM: dts: imx6qdl-apalis: Avoid underscore in node name" ARM: dts: imx6q-apalis: Add gpio-line-names ARM: dts: imx6q-apalis: Command pmic to standby for poweroff ARM: dts: imx6q-apalis: Move Atmel MXT touch ctrl to SoM dtsi ARM: dts: imx6q-apalis: Disable HDMI ARM: dts: imx6q-apalis: Add support for Toradex Ixora V1.2 carrier boards ARM: dts: imx6q-apalis: backlight pwm: Simplify inverted backlight ARM: dts: imx6q-apalis: backlight pwm: Adapt brightness steps ARM: dts: imx6q-apalis: Cleanup Oleksandr Suvorov (6): ARM: dts: imx6q-apalis: Move parallel rgb interface to SoM dtsi ARM: dts: imx6q-apalis: Move pinmux groups to SoM dtsi ARM: dts: imx6q-apalis: Add LVDS panel support ARM: dts: imx6q-apalis: Disable stmpe touchscreen ARM: dts: imx6q-apalis: Add ov5640 mipi csi camera ARM: dts: imx6q-apalis: Add adv7280 video input .../devicetree/bindings/arm/fsl.yaml | 1 + arch/arm/boot/dts/Makefile | 1 + arch/arm/boot/dts/imx6q-apalis-eval.dts | 117 +--- arch/arm/boot/dts/imx6q-apalis-ixora-v1.1.dts | 263 +------- arch/arm/boot/dts/imx6q-apalis-ixora-v1.2.dts | 290 ++++++++ arch/arm/boot/dts/imx6q-apalis-ixora.dts | 103 +-- arch/arm/boot/dts/imx6qdl-apalis.dtsi | 623 ++++++++++++++---- 7 files changed, 840 insertions(+), 558 deletions(-) create mode 100644 arch/arm/boot/dts/imx6q-apalis-ixora-v1.2.dts