Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1550191 > unrolled thread
| Started by | Steve Longerbeam <slongerbeam@gmail.com> |
|---|---|
| First post | 2017-01-03 22:10 +0100 |
| Last post | 2017-01-06 19:40 +0100 |
| Articles | 14 — 3 participants |
Back to article view | Back to linux.kernel
[PATCH v2 00/19] i.MX Media Driver Steve Longerbeam <slongerbeam@gmail.com> - 2017-01-03 22:10 +0100
[PATCH v2 07/19] ARM: dts: imx6-sabreauto: add reset-gpios property for max7310_b Steve Longerbeam <slongerbeam@gmail.com> - 2017-01-03 22:10 +0100
[PATCH v2 19/19] ARM: imx_v6_v7_defconfig: Enable staging video4linux drivers Steve Longerbeam <slongerbeam@gmail.com> - 2017-01-03 22:10 +0100
[PATCH v2 06/19] ARM: dts: imx6-sabreauto: create i2cmux for i2c3 Steve Longerbeam <slongerbeam@gmail.com> - 2017-01-03 22:10 +0100
[PATCH v2 05/19] ARM: dts: imx6-sabresd: add OV5642 and OV5640 camera sensors Steve Longerbeam <slongerbeam@gmail.com> - 2017-01-03 22:10 +0100
Re: [PATCH v2 05/19] ARM: dts: imx6-sabresd: add OV5642 and OV5640 camera sensors Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com> - 2017-01-04 13:40 +0100
Re: [PATCH v2 05/19] ARM: dts: imx6-sabresd: add OV5642 and OV5640 camera sensors Steve Longerbeam <slongerbeam@gmail.com> - 2017-01-05 23:40 +0100
Re: [PATCH v2 05/19] ARM: dts: imx6-sabresd: add OV5642 and OV5640 camera sensors Fabio Estevam <festevam@gmail.com> - 2017-01-04 16:30 +0100
Re: [PATCH v2 05/19] ARM: dts: imx6-sabresd: add OV5642 and OV5640 camera sensors Steve Longerbeam <slongerbeam@gmail.com> - 2017-01-07 01:30 +0100
Re: [PATCH v2 00/19] i.MX Media Driver Fabio Estevam <festevam@gmail.com> - 2017-01-03 22:20 +0100
Re: [PATCH v2 10/19] media: Add i.MX media core driver Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com> - 2017-01-04 14:50 +0100
Re: [PATCH v2 10/19] media: Add i.MX media core driver Steve Longerbeam <slongerbeam@gmail.com> - 2017-01-06 02:30 +0100
Re: [PATCH v2 13/19] media: imx: Add IC subdev drivers Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com> - 2017-01-04 16:00 +0100
Re: [PATCH v2 13/19] media: imx: Add IC subdev drivers Steve Longerbeam <slongerbeam@gmail.com> - 2017-01-06 19:40 +0100
| From | Steve Longerbeam <slongerbeam@gmail.com> |
|---|---|
| Date | 2017-01-03 22:10 +0100 |
| Subject | [PATCH v2 00/19] i.MX Media Driver |
| Message-ID | <sVElz-3en-7@gated-at.bofh.it> |
In version 2 (no functional changes):
- removed patch "gpio: pca953x: Add optional reset gpio control", it
has been submitted separately.
- fixed some whitespace errors.
- added a few missing Signed-off-by's.
Philipp Zabel (2):
ARM: dts: imx6qdl: Add mipi_ipu1/2 multiplexers, mipi_csi, and their
connections
media: imx: Add video switch subdev driver
Steve Longerbeam (17):
ARM: dts: imx6qdl: Add compatible, clocks, irqs to MIPI CSI-2 node
ARM: dts: imx6qdl: add media device
ARM: dts: imx6-sabrelite: add OV5642 and OV5640 camera sensors
ARM: dts: imx6-sabresd: add OV5642 and OV5640 camera sensors
ARM: dts: imx6-sabreauto: create i2cmux for i2c3
ARM: dts: imx6-sabreauto: add reset-gpios property for max7310_b
ARM: dts: imx6-sabreauto: add pinctrl for gpt input capture
ARM: dts: imx6-sabreauto: add the ADV7180 video decoder
media: Add i.MX media core driver
media: imx: Add CSI subdev driver
media: imx: Add SMFC subdev driver
media: imx: Add IC subdev drivers
media: imx: Add Camera Interface subdev driver
media: imx: Add MIPI CSI-2 Receiver subdev driver
media: imx: Add MIPI CSI-2 OV5640 sensor subdev driver
media: imx: Add Parallel OV5642 sensor subdev driver
ARM: imx_v6_v7_defconfig: Enable staging video4linux drivers
Documentation/devicetree/bindings/media/imx.txt | 205 +
Documentation/media/v4l-drivers/imx.rst | 430 ++
arch/arm/boot/dts/imx6dl-sabrelite.dts | 5 +
arch/arm/boot/dts/imx6dl-sabresd.dts | 5 +
arch/arm/boot/dts/imx6dl.dtsi | 183 +
arch/arm/boot/dts/imx6q-sabrelite.dts | 6 +
arch/arm/boot/dts/imx6q-sabresd.dts | 5 +
arch/arm/boot/dts/imx6q.dtsi | 123 +
arch/arm/boot/dts/imx6qdl-sabreauto.dtsi | 142 +-
arch/arm/boot/dts/imx6qdl-sabrelite.dtsi | 122 +-
arch/arm/boot/dts/imx6qdl-sabresd.dtsi | 114 +-
arch/arm/boot/dts/imx6qdl.dtsi | 25 +-
arch/arm/configs/imx_v6_v7_defconfig | 10 +-
drivers/staging/media/Kconfig | 2 +
drivers/staging/media/Makefile | 1 +
drivers/staging/media/imx/Kconfig | 36 +
drivers/staging/media/imx/Makefile | 16 +
drivers/staging/media/imx/TODO | 18 +
drivers/staging/media/imx/imx-camif.c | 1010 +++++
drivers/staging/media/imx/imx-csi.c | 638 +++
drivers/staging/media/imx/imx-ic-common.c | 113 +
drivers/staging/media/imx/imx-ic-pp.c | 636 +++
drivers/staging/media/imx/imx-ic-prpenc.c | 1037 +++++
drivers/staging/media/imx/imx-ic-prpvf.c | 1180 ++++++
drivers/staging/media/imx/imx-ic.h | 36 +
drivers/staging/media/imx/imx-media-common.c | 985 +++++
drivers/staging/media/imx/imx-media-dev.c | 479 +++
drivers/staging/media/imx/imx-media-fim.c | 509 +++
drivers/staging/media/imx/imx-media-internal-sd.c | 457 +++
drivers/staging/media/imx/imx-media-of.c | 291 ++
drivers/staging/media/imx/imx-media-of.h | 25 +
drivers/staging/media/imx/imx-media.h | 299 ++
drivers/staging/media/imx/imx-mipi-csi2.c | 509 +++
drivers/staging/media/imx/imx-smfc.c | 739 ++++
drivers/staging/media/imx/imx-video-switch.c | 351 ++
drivers/staging/media/imx/ov5640-mipi.c | 2349 +++++++++++
drivers/staging/media/imx/ov5642.c | 4364 +++++++++++++++++++++
include/media/imx.h | 15 +
include/uapi/Kbuild | 1 +
include/uapi/linux/v4l2-controls.h | 4 +
include/uapi/media/Kbuild | 2 +
include/uapi/media/imx.h | 30 +
42 files changed, 17479 insertions(+), 28 deletions(-)
create mode 100644 Documentation/devicetree/bindings/media/imx.txt
create mode 100644 Documentation/media/v4l-drivers/imx.rst
create mode 100644 drivers/staging/media/imx/Kconfig
create mode 100644 drivers/staging/media/imx/Makefile
create mode 100644 drivers/staging/media/imx/TODO
create mode 100644 drivers/staging/media/imx/imx-camif.c
create mode 100644 drivers/staging/media/imx/imx-csi.c
create mode 100644 drivers/staging/media/imx/imx-ic-common.c
create mode 100644 drivers/staging/media/imx/imx-ic-pp.c
create mode 100644 drivers/staging/media/imx/imx-ic-prpenc.c
create mode 100644 drivers/staging/media/imx/imx-ic-prpvf.c
create mode 100644 drivers/staging/media/imx/imx-ic.h
create mode 100644 drivers/staging/media/imx/imx-media-common.c
create mode 100644 drivers/staging/media/imx/imx-media-dev.c
create mode 100644 drivers/staging/media/imx/imx-media-fim.c
create mode 100644 drivers/staging/media/imx/imx-media-internal-sd.c
create mode 100644 drivers/staging/media/imx/imx-media-of.c
create mode 100644 drivers/staging/media/imx/imx-media-of.h
create mode 100644 drivers/staging/media/imx/imx-media.h
create mode 100644 drivers/staging/media/imx/imx-mipi-csi2.c
create mode 100644 drivers/staging/media/imx/imx-smfc.c
create mode 100644 drivers/staging/media/imx/imx-video-switch.c
create mode 100644 drivers/staging/media/imx/ov5640-mipi.c
create mode 100644 drivers/staging/media/imx/ov5642.c
create mode 100644 include/media/imx.h
create mode 100644 include/uapi/media/Kbuild
create mode 100644 include/uapi/media/imx.h
--
2.7.4
[toc] | [next] | [standalone]
| From | Steve Longerbeam <slongerbeam@gmail.com> |
|---|---|
| Date | 2017-01-03 22:10 +0100 |
| Subject | [PATCH v2 07/19] ARM: dts: imx6-sabreauto: add reset-gpios property for max7310_b |
| Message-ID | <sVEvg-3xp-51@gated-at.bofh.it> |
| In reply to | #1550191 |
The reset pin to the port expander chip (MAX7310) is controlled by a gpio,
so define a reset-gpios property to control it. There are three MAX7310's
on the SabreAuto CPU card (max7310_[abc]), but all use the same pin for
their reset. Since all can't acquire the same pin, assign it to max7310_b,
that chip is needed by more functions (usb and adv7180).
Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
---
arch/arm/boot/dts/imx6qdl-sabreauto.dtsi | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/arch/arm/boot/dts/imx6qdl-sabreauto.dtsi b/arch/arm/boot/dts/imx6qdl-sabreauto.dtsi
index 4a6d038..516bac6 100644
--- a/arch/arm/boot/dts/imx6qdl-sabreauto.dtsi
+++ b/arch/arm/boot/dts/imx6qdl-sabreauto.dtsi
@@ -136,6 +136,9 @@
reg = <0x32>;
gpio-controller;
#gpio-cells = <2>;
+ pinctrl-names = "default";
+ pinctrl-0 = <&pinctrl_max7310>;
+ reset-gpios = <&gpio1 15 GPIO_ACTIVE_LOW>;
};
max7310_c: gpio@34 {
@@ -442,6 +445,12 @@
>;
};
+ pinctrl_max7310: max7310grp {
+ fsl,pins = <
+ MX6QDL_PAD_SD2_DAT0__GPIO1_IO15 0x80000000
+ >;
+ };
+
pinctrl_pwm3: pwm1grp {
fsl,pins = <
MX6QDL_PAD_SD4_DAT1__PWM3_OUT 0x1b0b1
--
2.7.4
[toc] | [prev] | [next] | [standalone]
| From | Steve Longerbeam <slongerbeam@gmail.com> |
|---|---|
| Date | 2017-01-03 22:10 +0100 |
| Subject | [PATCH v2 19/19] ARM: imx_v6_v7_defconfig: Enable staging video4linux drivers |
| Message-ID | <sVEvg-3xp-41@gated-at.bofh.it> |
| In reply to | #1550191 |
Enable imx v4l2 staging drivers. For video capture on the SabreAuto, the ADV7180 video decoder also requires the i2c-mux-gpio and the max7310 port expander. The Sabrelite requires PWM clocks for the OV5640. Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com> --- arch/arm/configs/imx_v6_v7_defconfig | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/arch/arm/configs/imx_v6_v7_defconfig b/arch/arm/configs/imx_v6_v7_defconfig index cbe7faf..5da4d8e 100644 --- a/arch/arm/configs/imx_v6_v7_defconfig +++ b/arch/arm/configs/imx_v6_v7_defconfig @@ -51,6 +51,7 @@ CONFIG_PREEMPT_VOLUNTARY=y CONFIG_AEABI=y CONFIG_HIGHMEM=y CONFIG_CMA=y +CONFIG_FORCE_MAX_ZONEORDER=14 CONFIG_CMDLINE="noinitrd console=ttymxc0,115200" CONFIG_KEXEC=y CONFIG_CPU_FREQ=y @@ -181,6 +182,7 @@ CONFIG_SERIAL_FSL_LPUART=y CONFIG_SERIAL_FSL_LPUART_CONSOLE=y # CONFIG_I2C_COMPAT is not set CONFIG_I2C_CHARDEV=y +CONFIG_I2C_MUX=y CONFIG_I2C_MUX_GPIO=y # CONFIG_I2C_HELPER_AUTO is not set CONFIG_I2C_ALGOPCF=m @@ -194,11 +196,11 @@ CONFIG_GPIO_SYSFS=y CONFIG_GPIO_MC9S08DZ60=y CONFIG_GPIO_PCA953X=y CONFIG_GPIO_STMPE=y -CONFIG_POWER_SUPPLY=y CONFIG_POWER_RESET=y CONFIG_POWER_RESET_IMX=y CONFIG_POWER_RESET_SYSCON=y CONFIG_POWER_RESET_SYSCON_POWEROFF=y +CONFIG_POWER_SUPPLY=y CONFIG_SENSORS_GPIO_FAN=y CONFIG_SENSORS_IIO_HWMON=y CONFIG_THERMAL=y @@ -221,6 +223,8 @@ CONFIG_REGULATOR_PFUZE100=y CONFIG_MEDIA_SUPPORT=y CONFIG_MEDIA_CAMERA_SUPPORT=y CONFIG_MEDIA_RC_SUPPORT=y +CONFIG_MEDIA_CONTROLLER=y +CONFIG_VIDEO_V4L2_SUBDEV_API=y CONFIG_RC_DEVICES=y CONFIG_IR_GPIO_CIR=y CONFIG_MEDIA_USB_SUPPORT=y @@ -229,6 +233,8 @@ CONFIG_V4L_PLATFORM_DRIVERS=y CONFIG_SOC_CAMERA=y CONFIG_V4L_MEM2MEM_DRIVERS=y CONFIG_VIDEO_CODA=y +# CONFIG_MEDIA_SUBDRV_AUTOSELECT is not set +CONFIG_VIDEO_ADV7180=m CONFIG_SOC_CAMERA_OV2640=y CONFIG_IMX_IPUV3_CORE=y CONFIG_DRM=y @@ -338,6 +344,8 @@ CONFIG_FSL_EDMA=y CONFIG_IMX_SDMA=y CONFIG_MXS_DMA=y CONFIG_STAGING=y +CONFIG_STAGING_MEDIA=y +CONFIG_COMMON_CLK_PWM=y CONFIG_IIO=y CONFIG_VF610_ADC=y CONFIG_MPL3115=y -- 2.7.4
[toc] | [prev] | [next] | [standalone]
| From | Steve Longerbeam <slongerbeam@gmail.com> |
|---|---|
| Date | 2017-01-03 22:10 +0100 |
| Subject | [PATCH v2 06/19] ARM: dts: imx6-sabreauto: create i2cmux for i2c3 |
| Message-ID | <sVEvh-3xp-75@gated-at.bofh.it> |
| In reply to | #1550191 |
The sabreauto uses a steering pin to select between the SDA signal on
i2c3 bus, and a data-in pin for an SPI NOR chip. Use i2cmux to control
this steering pin. Idle state of the i2cmux selects SPI NOR. This is not
a classic way to use i2cmux, since one side of the mux selects something
other than an i2c bus, but it works and is probably the cleanest
solution. Note that if one thread is attempting to access SPI NOR while
another thread is accessing i2c3, the SPI NOR access will fail since the
i2cmux has selected the SDA pin rather than SPI NOR data-in. This couldn't
be avoided in any case, the board is not designed to allow concurrent
i2c3 and SPI NOR functions (and the default device-tree does not enable
SPI NOR anyway).
Devices hanging off i2c3 should now be defined under i2cmux, so
that the steering pin can be properly controlled to access those
devices. The port expanders (MAX7310) are thus moved into i2cmux.
Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
---
arch/arm/boot/dts/imx6qdl-sabreauto.dtsi | 65 +++++++++++++++++++++-----------
1 file changed, 44 insertions(+), 21 deletions(-)
diff --git a/arch/arm/boot/dts/imx6qdl-sabreauto.dtsi b/arch/arm/boot/dts/imx6qdl-sabreauto.dtsi
index 52390ba..4a6d038 100644
--- a/arch/arm/boot/dts/imx6qdl-sabreauto.dtsi
+++ b/arch/arm/boot/dts/imx6qdl-sabreauto.dtsi
@@ -108,6 +108,44 @@
default-brightness-level = <7>;
status = "okay";
};
+
+ i2cmux {
+ compatible = "i2c-mux-gpio";
+ #address-cells = <1>;
+ #size-cells = <0>;
+ pinctrl-names = "default";
+ pinctrl-0 = <&pinctrl_i2c3mux>;
+ mux-gpios = <&gpio5 4 0>;
+ i2c-parent = <&i2c3>;
+ idle-state = <0>;
+
+ i2c@1 {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ reg = <1>;
+
+ max7310_a: gpio@30 {
+ compatible = "maxim,max7310";
+ reg = <0x30>;
+ gpio-controller;
+ #gpio-cells = <2>;
+ };
+
+ max7310_b: gpio@32 {
+ compatible = "maxim,max7310";
+ reg = <0x32>;
+ gpio-controller;
+ #gpio-cells = <2>;
+ };
+
+ max7310_c: gpio@34 {
+ compatible = "maxim,max7310";
+ reg = <0x34>;
+ gpio-controller;
+ #gpio-cells = <2>;
+ };
+ };
+ };
};
&clks {
@@ -291,27 +329,6 @@
pinctrl-names = "default";
pinctrl-0 = <&pinctrl_i2c3>;
status = "okay";
-
- max7310_a: gpio@30 {
- compatible = "maxim,max7310";
- reg = <0x30>;
- gpio-controller;
- #gpio-cells = <2>;
- };
-
- max7310_b: gpio@32 {
- compatible = "maxim,max7310";
- reg = <0x32>;
- gpio-controller;
- #gpio-cells = <2>;
- };
-
- max7310_c: gpio@34 {
- compatible = "maxim,max7310";
- reg = <0x34>;
- gpio-controller;
- #gpio-cells = <2>;
- };
};
&iomuxc {
@@ -419,6 +436,12 @@
>;
};
+ pinctrl_i2c3mux: i2c3muxgrp {
+ fsl,pins = <
+ MX6QDL_PAD_EIM_A24__GPIO5_IO04 0x80000000
+ >;
+ };
+
pinctrl_pwm3: pwm1grp {
fsl,pins = <
MX6QDL_PAD_SD4_DAT1__PWM3_OUT 0x1b0b1
--
2.7.4
[toc] | [prev] | [next] | [standalone]
| From | Steve Longerbeam <slongerbeam@gmail.com> |
|---|---|
| Date | 2017-01-03 22:10 +0100 |
| Subject | [PATCH v2 05/19] ARM: dts: imx6-sabresd: add OV5642 and OV5640 camera sensors |
| Message-ID | <sVEvh-3xp-79@gated-at.bofh.it> |
| In reply to | #1550191 |
Enables the OV5642 parallel-bus sensor, and the OV5640 MIPI CSI-2 sensor.
The OV5642 connects to the parallel-bus mux input port on ipu1_csi0_mux.
The OV5640 connects to the input port on the MIPI CSI-2 receiver on
mipi_csi. It is set to transmit over MIPI virtual channel 1.
Until the OV5652 sensor module compatible with the SabreSD becomes
available for testing, the ov5642 node is currently disabled.
Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
---
arch/arm/boot/dts/imx6dl-sabresd.dts | 5 ++
arch/arm/boot/dts/imx6q-sabresd.dts | 5 ++
arch/arm/boot/dts/imx6qdl-sabresd.dtsi | 114 ++++++++++++++++++++++++++++++++-
3 files changed, 123 insertions(+), 1 deletion(-)
diff --git a/arch/arm/boot/dts/imx6dl-sabresd.dts b/arch/arm/boot/dts/imx6dl-sabresd.dts
index 1e45f2f..6cf7a50 100644
--- a/arch/arm/boot/dts/imx6dl-sabresd.dts
+++ b/arch/arm/boot/dts/imx6dl-sabresd.dts
@@ -15,3 +15,8 @@
model = "Freescale i.MX6 DualLite SABRE Smart Device Board";
compatible = "fsl,imx6dl-sabresd", "fsl,imx6dl";
};
+
+&ipu1_csi1_from_ipu1_csi1_mux {
+ data-lanes = <0 1>;
+ clock-lanes = <2>;
+};
diff --git a/arch/arm/boot/dts/imx6q-sabresd.dts b/arch/arm/boot/dts/imx6q-sabresd.dts
index 9cbdfe7..8c1d7ad 100644
--- a/arch/arm/boot/dts/imx6q-sabresd.dts
+++ b/arch/arm/boot/dts/imx6q-sabresd.dts
@@ -23,3 +23,8 @@
&sata {
status = "okay";
};
+
+&ipu1_csi1_from_mipi_vc1 {
+ data-lanes = <0 1>;
+ clock-lanes = <2>;
+};
diff --git a/arch/arm/boot/dts/imx6qdl-sabresd.dtsi b/arch/arm/boot/dts/imx6qdl-sabresd.dtsi
index 55ef535..39b4228 100644
--- a/arch/arm/boot/dts/imx6qdl-sabresd.dtsi
+++ b/arch/arm/boot/dts/imx6qdl-sabresd.dtsi
@@ -10,6 +10,7 @@
* http://www.gnu.org/copyleft/gpl.html
*/
+#include <dt-bindings/clock/imx6qdl-clock.h>
#include <dt-bindings/gpio/gpio.h>
#include <dt-bindings/input/input.h>
@@ -146,6 +147,33 @@
};
};
+&ipu1_csi0_from_ipu1_csi0_mux {
+ bus-width = <8>;
+ data-shift = <12>; /* Lines 19:12 used */
+ hsync-active = <1>;
+ vsync-active = <1>;
+};
+
+&ipu1_csi0_mux_from_parallel_sensor {
+ remote-endpoint = <&ov5642_to_ipu1_csi0_mux>;
+};
+
+&ipu1_csi0 {
+ pinctrl-names = "default";
+ pinctrl-0 = <&pinctrl_ipu1_csi0>;
+};
+
+&mipi_csi {
+ status = "okay";
+};
+
+/* Incoming port from sensor */
+&mipi_csi_from_mipi_sensor {
+ remote-endpoint = <&ov5640_to_mipi_csi>;
+ data-lanes = <0 1>;
+ clock-lanes = <2>;
+};
+
&audmux {
pinctrl-names = "default";
pinctrl-0 = <&pinctrl_audmux>;
@@ -214,7 +242,33 @@
0x8014 /* 4:FN_DMICCDAT */
0x0000 /* 5:Default */
>;
- };
+ };
+
+ camera: ov5642@3c {
+ compatible = "ovti,ov5642";
+ pinctrl-names = "default";
+ pinctrl-0 = <&pinctrl_ov5642>;
+ clocks = <&clks IMX6QDL_CLK_CKO>;
+ clock-names = "xclk";
+ reg = <0x3c>;
+ xclk = <24000000>;
+ DOVDD-supply = <&vgen4_reg>; /* 1.8v */
+ AVDD-supply = <&vgen5_reg>; /* 2.8v, rev C board is VGEN3
+ rev B board is VGEN5 */
+ DVDD-supply = <&vgen2_reg>; /* 1.5v*/
+ pwdn-gpios = <&gpio1 16 GPIO_ACTIVE_HIGH>; /* SD1_DAT0 */
+ reset-gpios = <&gpio1 17 GPIO_ACTIVE_LOW>; /* SD1_DAT1 */
+ status = "disabled";
+
+ port {
+ ov5642_to_ipu1_csi0_mux: endpoint {
+ remote-endpoint = <&ipu1_csi0_mux_from_parallel_sensor>;
+ bus-width = <8>;
+ hsync-active = <1>;
+ vsync-active = <1>;
+ };
+ };
+ };
};
&i2c2 {
@@ -322,6 +376,34 @@
};
};
};
+
+ mipi_camera: ov5640@3c {
+ compatible = "ovti,ov5640_mipi";
+ pinctrl-names = "default";
+ pinctrl-0 = <&pinctrl_ov5640>;
+ reg = <0x3c>;
+ clocks = <&clks IMX6QDL_CLK_CKO>;
+ clock-names = "xclk";
+ xclk = <24000000>;
+ DOVDD-supply = <&vgen4_reg>; /* 1.8v */
+ AVDD-supply = <&vgen5_reg>; /* 2.8v, rev C board is VGEN3
+ rev B board is VGEN5 */
+ DVDD-supply = <&vgen2_reg>; /* 1.5v*/
+ pwdn-gpios = <&gpio1 19 GPIO_ACTIVE_HIGH>; /* SD1_DAT2 */
+ reset-gpios = <&gpio1 20 GPIO_ACTIVE_LOW>; /* SD1_CLK */
+
+ port {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ ov5640_to_mipi_csi: endpoint@1 {
+ reg = <1>;
+ remote-endpoint = <&mipi_csi_from_mipi_sensor>;
+ data-lanes = <0 1>;
+ clock-lanes = <2>;
+ };
+ };
+ };
};
&i2c3 {
@@ -426,6 +508,36 @@
>;
};
+ pinctrl_ov5640: ov5640grp {
+ fsl,pins = <
+ MX6QDL_PAD_SD1_DAT2__GPIO1_IO19 0x80000000
+ MX6QDL_PAD_SD1_CLK__GPIO1_IO20 0x80000000
+ >;
+ };
+
+ pinctrl_ov5642: ov5642grp {
+ fsl,pins = <
+ MX6QDL_PAD_SD1_DAT0__GPIO1_IO16 0x80000000
+ MX6QDL_PAD_SD1_DAT1__GPIO1_IO17 0x80000000
+ >;
+ };
+
+ pinctrl_ipu1_csi0: ipu1grp-csi0 {
+ fsl,pins = <
+ MX6QDL_PAD_CSI0_DAT12__IPU1_CSI0_DATA12 0x80000000
+ MX6QDL_PAD_CSI0_DAT13__IPU1_CSI0_DATA13 0x80000000
+ MX6QDL_PAD_CSI0_DAT14__IPU1_CSI0_DATA14 0x80000000
+ MX6QDL_PAD_CSI0_DAT15__IPU1_CSI0_DATA15 0x80000000
+ MX6QDL_PAD_CSI0_DAT16__IPU1_CSI0_DATA16 0x80000000
+ MX6QDL_PAD_CSI0_DAT17__IPU1_CSI0_DATA17 0x80000000
+ MX6QDL_PAD_CSI0_DAT18__IPU1_CSI0_DATA18 0x80000000
+ MX6QDL_PAD_CSI0_DAT19__IPU1_CSI0_DATA19 0x80000000
+ MX6QDL_PAD_CSI0_PIXCLK__IPU1_CSI0_PIXCLK 0x80000000
+ MX6QDL_PAD_CSI0_MCLK__IPU1_CSI0_HSYNC 0x80000000
+ MX6QDL_PAD_CSI0_VSYNC__IPU1_CSI0_VSYNC 0x80000000
+ >;
+ };
+
pinctrl_pcie: pciegrp {
fsl,pins = <
MX6QDL_PAD_GPIO_17__GPIO7_IO12 0x1b0b0
--
2.7.4
[toc] | [prev] | [next] | [standalone]
| From | Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com> |
|---|---|
| Date | 2017-01-04 13:40 +0100 |
| Subject | Re: [PATCH v2 05/19] ARM: dts: imx6-sabresd: add OV5642 and OV5640 camera sensors |
| Message-ID | <sVT1f-4FT-17@gated-at.bofh.it> |
| In reply to | #1550200 |
On 01/03/2017 10:57 PM, Steve Longerbeam wrote:
> Enables the OV5642 parallel-bus sensor, and the OV5640 MIPI CSI-2 sensor.
>
> The OV5642 connects to the parallel-bus mux input port on ipu1_csi0_mux.
>
> The OV5640 connects to the input port on the MIPI CSI-2 receiver on
> mipi_csi. It is set to transmit over MIPI virtual channel 1.
>
> Until the OV5652 sensor module compatible with the SabreSD becomes
> available for testing, the ov5642 node is currently disabled.
>
> Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
> ---
[snip]
> +
> + camera: ov5642@3c {
ov5642: camera@3c
> + compatible = "ovti,ov5642";
> + pinctrl-names = "default";
> + pinctrl-0 = <&pinctrl_ov5642>;
> + clocks = <&clks IMX6QDL_CLK_CKO>;
> + clock-names = "xclk";
> + reg = <0x3c>;
> + xclk = <24000000>;
> + DOVDD-supply = <&vgen4_reg>; /* 1.8v */
> + AVDD-supply = <&vgen5_reg>; /* 2.8v, rev C board is VGEN3
> + rev B board is VGEN5 */
> + DVDD-supply = <&vgen2_reg>; /* 1.5v*/
> + pwdn-gpios = <&gpio1 16 GPIO_ACTIVE_HIGH>; /* SD1_DAT0 */
> + reset-gpios = <&gpio1 17 GPIO_ACTIVE_LOW>; /* SD1_DAT1 */
Comments about SD1_* pad names are redundant.
> + status = "disabled";
Why is it disabled here?
> +
> + port {
> + ov5642_to_ipu1_csi0_mux: endpoint {
> + remote-endpoint = <&ipu1_csi0_mux_from_parallel_sensor>;
> + bus-width = <8>;
> + hsync-active = <1>;
> + vsync-active = <1>;
> + };
> + };
> + };
> };
>
> &i2c2 {
> @@ -322,6 +376,34 @@
> };
> };
> };
> +
> + mipi_camera: ov5640@3c {
ov5640: camera@3c
> + compatible = "ovti,ov5640_mipi";
> + pinctrl-names = "default";
> + pinctrl-0 = <&pinctrl_ov5640>;
> + reg = <0x3c>;
> + clocks = <&clks IMX6QDL_CLK_CKO>;
> + clock-names = "xclk";
> + xclk = <24000000>;
> + DOVDD-supply = <&vgen4_reg>; /* 1.8v */
> + AVDD-supply = <&vgen5_reg>; /* 2.8v, rev C board is VGEN3
> + rev B board is VGEN5 */
> + DVDD-supply = <&vgen2_reg>; /* 1.5v*/
> + pwdn-gpios = <&gpio1 19 GPIO_ACTIVE_HIGH>; /* SD1_DAT2 */
> + reset-gpios = <&gpio1 20 GPIO_ACTIVE_LOW>; /* SD1_CLK */
Comments about SD1_* pad names are redundant.
> +
> + port {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + ov5640_to_mipi_csi: endpoint@1 {
> + reg = <1>;
> + remote-endpoint = <&mipi_csi_from_mipi_sensor>;
> + data-lanes = <0 1>;
> + clock-lanes = <2>;
> + };
> + };
> + };
> };
>
> &i2c3 {
> @@ -426,6 +508,36 @@
> >;
> };
>
> + pinctrl_ov5640: ov5640grp {
> + fsl,pins = <
> + MX6QDL_PAD_SD1_DAT2__GPIO1_IO19 0x80000000
> + MX6QDL_PAD_SD1_CLK__GPIO1_IO20 0x80000000
> + >;
> + };
> +
> + pinctrl_ov5642: ov5642grp {
> + fsl,pins = <
> + MX6QDL_PAD_SD1_DAT0__GPIO1_IO16 0x80000000
> + MX6QDL_PAD_SD1_DAT1__GPIO1_IO17 0x80000000
> + >;
> + };
> +
> + pinctrl_ipu1_csi0: ipu1grp-csi0 {
Please rename the node name to ipu1csi0grp.
Please add new pin control groups preserving the alphanimerical order.
> + fsl,pins = <
> + MX6QDL_PAD_CSI0_DAT12__IPU1_CSI0_DATA12 0x80000000
> + MX6QDL_PAD_CSI0_DAT13__IPU1_CSI0_DATA13 0x80000000
> + MX6QDL_PAD_CSI0_DAT14__IPU1_CSI0_DATA14 0x80000000
> + MX6QDL_PAD_CSI0_DAT15__IPU1_CSI0_DATA15 0x80000000
> + MX6QDL_PAD_CSI0_DAT16__IPU1_CSI0_DATA16 0x80000000
> + MX6QDL_PAD_CSI0_DAT17__IPU1_CSI0_DATA17 0x80000000
> + MX6QDL_PAD_CSI0_DAT18__IPU1_CSI0_DATA18 0x80000000
> + MX6QDL_PAD_CSI0_DAT19__IPU1_CSI0_DATA19 0x80000000
> + MX6QDL_PAD_CSI0_PIXCLK__IPU1_CSI0_PIXCLK 0x80000000
> + MX6QDL_PAD_CSI0_MCLK__IPU1_CSI0_HSYNC 0x80000000
> + MX6QDL_PAD_CSI0_VSYNC__IPU1_CSI0_VSYNC 0x80000000
> + >;
> + };
> +
> pinctrl_pcie: pciegrp {
> fsl,pins = <
> MX6QDL_PAD_GPIO_17__GPIO7_IO12 0x1b0b0
>
--
With best wishes,
Vladimir
[toc] | [prev] | [next] | [standalone]
| From | Steve Longerbeam <slongerbeam@gmail.com> |
|---|---|
| Date | 2017-01-05 23:40 +0100 |
| Subject | Re: [PATCH v2 05/19] ARM: dts: imx6-sabresd: add OV5642 and OV5640 camera sensors |
| Message-ID | <sWoRr-Mr-11@gated-at.bofh.it> |
| In reply to | #1550727 |
On 01/04/2017 04:33 AM, Vladimir Zapolskiy wrote:
>
>> +
>> + camera: ov5642@3c {
> ov5642: camera@3c
done.
>> + pwdn-gpios = <&gpio1 16 GPIO_ACTIVE_HIGH>; /* SD1_DAT0 */
>> + reset-gpios = <&gpio1 17 GPIO_ACTIVE_LOW>; /* SD1_DAT1 */
> Comments about SD1_* pad names are redundant.
sure, removed.
>> + status = "disabled";
> Why is it disabled here?
It's explained in the header. I don't yet have the OV5642 module for
the sabresd for testing, so it is disabled for now.
>> +
>> + mipi_camera: ov5640@3c {
> ov5640: camera@3c
done.
>
>> + pwdn-gpios = <&gpio1 19 GPIO_ACTIVE_HIGH>; /* SD1_DAT2 */
>> + reset-gpios = <&gpio1 20 GPIO_ACTIVE_LOW>; /* SD1_CLK */
> Comments about SD1_* pad names are redundant.
removed.
>> +
>> + pinctrl_ipu1_csi0: ipu1grp-csi0 {
> Please rename the node name to ipu1csi0grp.
>
> Please add new pin control groups preserving the alphanimerical order.
done and done.
Steve
[toc] | [prev] | [next] | [standalone]
| From | Fabio Estevam <festevam@gmail.com> |
|---|---|
| Date | 2017-01-04 16:30 +0100 |
| Subject | Re: [PATCH v2 05/19] ARM: dts: imx6-sabresd: add OV5642 and OV5640 camera sensors |
| Message-ID | <sVVFL-6tl-15@gated-at.bofh.it> |
| In reply to | #1550200 |
On Tue, Jan 3, 2017 at 6:57 PM, Steve Longerbeam <slongerbeam@gmail.com> wrote:
> + camera: ov5642@3c {
> + compatible = "ovti,ov5642";
> + pinctrl-names = "default";
> + pinctrl-0 = <&pinctrl_ov5642>;
> + clocks = <&clks IMX6QDL_CLK_CKO>;
> + clock-names = "xclk";
> + reg = <0x3c>;
> + xclk = <24000000>;
> + DOVDD-supply = <&vgen4_reg>; /* 1.8v */
> + AVDD-supply = <&vgen5_reg>; /* 2.8v, rev C board is VGEN3
> + rev B board is VGEN5 */
Please use vgen3 so that by default we have the valid AVDD-supply for
revC boards which is more recent and more the users have access to.
> + mipi_camera: ov5640@3c {
> + compatible = "ovti,ov5640_mipi";
> + pinctrl-names = "default";
> + pinctrl-0 = <&pinctrl_ov5640>;
> + reg = <0x3c>;
> + clocks = <&clks IMX6QDL_CLK_CKO>;
> + clock-names = "xclk";
> + xclk = <24000000>;
> + DOVDD-supply = <&vgen4_reg>; /* 1.8v */
> + AVDD-supply = <&vgen5_reg>; /* 2.8v, rev C board is VGEN3
> + rev B board is VGEN5 */
Same here.
> + pinctrl_ov5640: ov5640grp {
> + fsl,pins = <
> + MX6QDL_PAD_SD1_DAT2__GPIO1_IO19 0x80000000
> + MX6QDL_PAD_SD1_CLK__GPIO1_IO20 0x80000000
Please avoid all the 0x80000000 IOMUX settings and replace them by
their real values.
[toc] | [prev] | [next] | [standalone]
| From | Steve Longerbeam <slongerbeam@gmail.com> |
|---|---|
| Date | 2017-01-07 01:30 +0100 |
| Subject | Re: [PATCH v2 05/19] ARM: dts: imx6-sabresd: add OV5642 and OV5640 camera sensors |
| Message-ID | <sWN3r-1cB-1@gated-at.bofh.it> |
| In reply to | #1550918 |
On 01/04/2017 07:26 AM, Fabio Estevam wrote:
> On Tue, Jan 3, 2017 at 6:57 PM, Steve Longerbeam <slongerbeam@gmail.com> wrote:
>
>> + camera: ov5642@3c {
>> + compatible = "ovti,ov5642";
>> + pinctrl-names = "default";
>> + pinctrl-0 = <&pinctrl_ov5642>;
>> + clocks = <&clks IMX6QDL_CLK_CKO>;
>> + clock-names = "xclk";
>> + reg = <0x3c>;
>> + xclk = <24000000>;
>> + DOVDD-supply = <&vgen4_reg>; /* 1.8v */
>> + AVDD-supply = <&vgen5_reg>; /* 2.8v, rev C board is VGEN3
>> + rev B board is VGEN5 */
> Please use vgen3 so that by default we have the valid AVDD-supply for
> revC boards which is more recent and more the users have access to.
done.
>
>> + mipi_camera: ov5640@3c {
>> + compatible = "ovti,ov5640_mipi";
>> + pinctrl-names = "default";
>> + pinctrl-0 = <&pinctrl_ov5640>;
>> + reg = <0x3c>;
>> + clocks = <&clks IMX6QDL_CLK_CKO>;
>> + clock-names = "xclk";
>> + xclk = <24000000>;
>> + DOVDD-supply = <&vgen4_reg>; /* 1.8v */
>> + AVDD-supply = <&vgen5_reg>; /* 2.8v, rev C board is VGEN3
>> + rev B board is VGEN5 */
> Same here.
done.
>
>> + pinctrl_ov5640: ov5640grp {
>> + fsl,pins = <
>> + MX6QDL_PAD_SD1_DAT2__GPIO1_IO19 0x80000000
>> + MX6QDL_PAD_SD1_CLK__GPIO1_IO20 0x80000000
> Please avoid all the 0x80000000 IOMUX settings and replace them by
> their real values.
yeah, finally got around to this, done!
Steve
[toc] | [prev] | [next] | [standalone]
| From | Fabio Estevam <festevam@gmail.com> |
|---|---|
| Date | 2017-01-03 22:20 +0100 |
| Message-ID | <sVEEW-3Cx-17@gated-at.bofh.it> |
| In reply to | #1550191 |
On Tue, Jan 3, 2017 at 6:57 PM, Steve Longerbeam <slongerbeam@gmail.com> wrote: > In version 2 (no functional changes): > > - removed patch "gpio: pca953x: Add optional reset gpio control", it > has been submitted separately. > - fixed some whitespace errors. > - added a few missing Signed-off-by's. Tested the series on a mx6qsabresd, so: Tested-by: Fabio Estevam <fabio.estevam@nxp.com>
[toc] | [prev] | [next] | [standalone]
| From | Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com> |
|---|---|
| Date | 2017-01-04 14:50 +0100 |
| Subject | Re: [PATCH v2 10/19] media: Add i.MX media core driver |
| Message-ID | <sVU71-5lr-63@gated-at.bofh.it> |
| In reply to | #1550191 |
Hi Steve,
On 01/03/2017 10:57 PM, Steve Longerbeam wrote:
> Add the core media driver for i.MX SOC.
>
> Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
> ---
> Documentation/devicetree/bindings/media/imx.txt | 205 +++++
v2 was sent before getting Rob's review comments, but still they
should be addressed in v3.
Also I would suggest to separate device tree binding documentation
change and place it as the first patch in the series, this should
make the following DTS changes valid.
> Documentation/media/v4l-drivers/imx.rst | 430 ++++++++++
> drivers/staging/media/Kconfig | 2 +
> drivers/staging/media/Makefile | 1 +
> drivers/staging/media/imx/Kconfig | 8 +
> drivers/staging/media/imx/Makefile | 6 +
> drivers/staging/media/imx/TODO | 18 +
> drivers/staging/media/imx/imx-media-common.c | 985 ++++++++++++++++++++++
> drivers/staging/media/imx/imx-media-dev.c | 479 +++++++++++
> drivers/staging/media/imx/imx-media-fim.c | 509 +++++++++++
> drivers/staging/media/imx/imx-media-internal-sd.c | 457 ++++++++++
> drivers/staging/media/imx/imx-media-of.c | 291 +++++++
> drivers/staging/media/imx/imx-media-of.h | 25 +
> drivers/staging/media/imx/imx-media.h | 299 +++++++
> include/media/imx.h | 15 +
> include/uapi/Kbuild | 1 +
> include/uapi/linux/v4l2-controls.h | 4 +
> include/uapi/media/Kbuild | 2 +
> include/uapi/media/imx.h | 30 +
Probably Greg should ack the UAPI changes, you may consider
to split them into a separate patch.
> 19 files changed, 3767 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/media/imx.txt
> create mode 100644 Documentation/media/v4l-drivers/imx.rst
> create mode 100644 drivers/staging/media/imx/Kconfig
> create mode 100644 drivers/staging/media/imx/Makefile
> create mode 100644 drivers/staging/media/imx/TODO
> create mode 100644 drivers/staging/media/imx/imx-media-common.c
> create mode 100644 drivers/staging/media/imx/imx-media-dev.c
> create mode 100644 drivers/staging/media/imx/imx-media-fim.c
> create mode 100644 drivers/staging/media/imx/imx-media-internal-sd.c
> create mode 100644 drivers/staging/media/imx/imx-media-of.c
> create mode 100644 drivers/staging/media/imx/imx-media-of.h
> create mode 100644 drivers/staging/media/imx/imx-media.h
> create mode 100644 include/media/imx.h
> create mode 100644 include/uapi/media/Kbuild
> create mode 100644 include/uapi/media/imx.h
>
[snip]
> +
> +struct imx_media_subdev *
> +imx_media_find_subdev_by_sd(struct imx_media_dev *imxmd,
> + struct v4l2_subdev *sd)
> +{
> + struct imx_media_subdev *imxsd;
> + int i, ret = -ENODEV;
> +
> + for (i = 0; i < imxmd->num_subdevs; i++) {
> + imxsd = &imxmd->subdev[i];
> + if (sd == imxsd->sd) {
This can be simplifed:
...
if (sd == imxsd->sd)
return imxsd;
}
return ERR_PTR(-ENODEV);
> + ret = 0;
> + break;
> + }
> + }
> +
> + return ret ? ERR_PTR(ret) : imxsd;
> +}
> +EXPORT_SYMBOL_GPL(imx_media_find_subdev_by_sd);
> +
> +struct imx_media_subdev *
> +imx_media_find_subdev_by_id(struct imx_media_dev *imxmd, u32 grp_id)
> +{
> + struct imx_media_subdev *imxsd;
> + int i, ret = -ENODEV;
> +
> + for (i = 0; i < imxmd->num_subdevs; i++) {
> + imxsd = &imxmd->subdev[i];
> + if (imxsd->sd && imxsd->sd->grp_id == grp_id) {
> + ret = 0;
> + break;
This can be simplifed:
...
if (imxsd->sd && imxsd->sd->grp_id == grp_i)
return imxsd;
}
return ERR_PTR(-ENODEV);
> + }
> + }
> +
> + return ret ? ERR_PTR(ret) : imxsd;
> +}
> +EXPORT_SYMBOL_GPL(imx_media_find_subdev_by_id);
> +
[snip]
> diff --git a/drivers/staging/media/imx/imx-media-dev.c b/drivers/staging/media/imx/imx-media-dev.c
> new file mode 100644
> index 0000000..8d22730
> --- /dev/null
> +++ b/drivers/staging/media/imx/imx-media-dev.c
> @@ -0,0 +1,479 @@
> +/*
> + * V4L2 Media Controller Driver for Freescale i.MX5/6 SOC
> + *
> + * Copyright (c) 2016 Mentor Graphics Inc.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + */
> +#include <linux/module.h>
> +#include <linux/delay.h>
> +#include <linux/fs.h>
> +#include <linux/timer.h>
> +#include <linux/sched.h>
> +#include <linux/slab.h>
> +#include <linux/spinlock.h>
> +#include <linux/platform_device.h>
> +#include <linux/pinctrl/consumer.h>
> +#include <linux/of_platform.h>
> +#include <media/v4l2-ioctl.h>
> +#include <media/v4l2-ctrls.h>
> +#include <media/v4l2-event.h>
> +#include <media/v4l2-mc.h>
Please sort out the list of headers alphabetically.
> +#include <video/imx-ipu-v3.h>
> +#include <media/imx.h>
> +#include "imx-media.h"
> +#include "imx-media-of.h"
> +
> +#define DEVICE_NAME "imx-media"
I suppose you don't need this macro.
[snip]
> + */
> +static int imx_media_create_links(struct imx_media_dev *imxmd)
> +{
> + struct imx_media_subdev *local_sd;
> + struct imx_media_subdev *remote_sd;
> + struct v4l2_subdev *source, *sink;
> + struct imx_media_link *link;
> + struct imx_media_pad *pad;
> + u16 source_pad, sink_pad;
> + int num_pads, i, j, k;
> + int ret = 0;
> +
> + for (i = 0; i < imxmd->num_subdevs; i++) {
> + local_sd = &imxmd->subdev[i];
> + num_pads = local_sd->num_sink_pads + local_sd->num_src_pads;
> +
> + for (j = 0; j < num_pads; j++) {
> + pad = &local_sd->pad[j];
> +
> + for (k = 0; k < pad->num_links; k++) {
> + link = &pad->link[k];
> +
> + remote_sd = imx_media_find_async_subdev(
> + imxmd, link->remote_sd_node,
> + link->remote_devname);
> + if (!remote_sd) {
> + v4l2_warn(&imxmd->v4l2_dev,
> + "%s: no remote for %s:%d\n",
> + __func__, local_sd->sd->name,
> + link->local_pad);
> + continue;
> + }
> +
> + /* only create the source->sink links */
> + if (pad->pad.flags & MEDIA_PAD_FL_SINK)
> + continue;
> +
> + source = local_sd->sd;
> + sink = remote_sd->sd;
> + source_pad = link->local_pad;
> + sink_pad = link->remote_pad;
> +
> + v4l2_info(&imxmd->v4l2_dev,
> + "%s: %s:%d -> %s:%d\n", __func__,
> + source->name, source_pad,
> + sink->name, sink_pad);
> +
> + ret = media_create_pad_link(&source->entity,
> + source_pad,
> + &sink->entity,
> + sink_pad,
> + 0);
> + if (ret) {
> + v4l2_err(&imxmd->v4l2_dev,
> + "create_pad_link failed: %d\n",
> + ret);
> + goto out;
Indentation depth is quite terrific.
> + }
> + }
> + }
> + }
> +
> +out:
> + return ret;
>
[snip]
> +
> +static const struct of_device_id imx_media_dt_ids[] = {
> + { .compatible = "fsl,imx-media" },
> + { /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, imx_media_dt_ids);
> +
> +static struct platform_driver imx_media_pdrv = {
> + .probe = imx_media_probe,
> + .remove = imx_media_remove,
> + .driver = {
> + .name = DEVICE_NAME,
> + .owner = THIS_MODULE,
Setting of .owner is not needed nowadays IIRC.
> + .of_match_table = imx_media_dt_ids,
> + },
> +};
> +
> +module_platform_driver(imx_media_pdrv);
> +
> +MODULE_DESCRIPTION("i.MX5/6 v4l2 media controller driver");
> +MODULE_AUTHOR("Steve Longerbeam <steve_longerbeam@mentor.com>");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/staging/media/imx/imx-media-fim.c b/drivers/staging/media/imx/imx-media-fim.c
> new file mode 100644
> index 0000000..52bfa8d
> --- /dev/null
> +++ b/drivers/staging/media/imx/imx-media-fim.c
> @@ -0,0 +1,509 @@
> +/*
> + * Frame Interval Monitor.
> + *
> + * Copyright (c) 2016 Mentor Graphics Inc.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + */
> +#include <linux/module.h>
> +#include <linux/delay.h>
> +#include <linux/slab.h>
> +#include <linux/platform_device.h>
> +#ifdef CONFIG_IMX_GPT_ICAP
> +#include <linux/mxc_icap.h>
> +#endif
This looks clumsy. Include it unconditionally, if needed
do #ifdef's inside the header file.
> +#include <media/v4l2-subdev.h>
> +#include <media/v4l2-of.h>
> +#include <media/v4l2-ctrls.h>
Please sort out the list alphabetically.
> +#include <media/imx.h>
> +#include "imx-media.h"
> +
[snip]
> +
> +static int of_parse_fim(struct imx_media_fim *fim, struct device_node *np)
> +{
> + struct device_node *fim_np;
> + u32 val, tol[2], icap[2];
> + int ret;
> +
> + fim_np = of_get_child_by_name(np, "fim");
> + if (!fim_np) {
> + /* set to the default defaults */
> + fim->of_defaults[FIM_CL_ENABLE] = FIM_CL_ENABLE_DEF;
> + fim->of_defaults[FIM_CL_NUM] = FIM_CL_NUM_DEF;
> + fim->of_defaults[FIM_CL_NUM_SKIP] = FIM_CL_NUM_SKIP_DEF;
> + fim->of_defaults[FIM_CL_TOLERANCE_MIN] =
> + FIM_CL_TOLERANCE_MIN_DEF;
> + fim->of_defaults[FIM_CL_TOLERANCE_MAX] =
> + FIM_CL_TOLERANCE_MAX_DEF;
> + fim->icap_channel = -1;
> + return 0;
> + }
> +
> + ret = of_property_read_u32(fim_np, "enable", &val);
> + if (ret)
> + val = FIM_CL_ENABLE_DEF;
> + fim->of_defaults[FIM_CL_ENABLE] = val;
> +
> + ret = of_property_read_u32(fim_np, "num-avg", &val);
> + if (ret)
> + val = FIM_CL_NUM_DEF;
> + fim->of_defaults[FIM_CL_NUM] = val;
> +
> + ret = of_property_read_u32(fim_np, "num-skip", &val);
> + if (ret)
> + val = FIM_CL_NUM_SKIP_DEF;
> + fim->of_defaults[FIM_CL_NUM_SKIP] = val;
> +
> + ret = of_property_read_u32_array(fim_np, "tolerance-range", tol, 2);
> + if (ret) {
> + tol[0] = FIM_CL_TOLERANCE_MIN_DEF;
> + tol[1] = FIM_CL_TOLERANCE_MAX_DEF;
> + }
> + fim->of_defaults[FIM_CL_TOLERANCE_MIN] = tol[0];
> + fim->of_defaults[FIM_CL_TOLERANCE_MAX] = tol[1];
> +
> + fim->icap_channel = -1;
> + if (IS_ENABLED(CONFIG_IMX_GPT_ICAP)) {
> + ret = of_property_read_u32_array(fim_np,
> + "input-capture-channel",
> + icap, 2);
> + if (!ret) {
> + fim->icap_channel = icap[0];
> + fim->icap_flags = icap[1];
> + }
Should you return error otherwise?
> + }
> +
> + of_node_put(fim_np);
> + return 0;
> +}
[snip]
> diff --git a/drivers/staging/media/imx/imx-media-of.c b/drivers/staging/media/imx/imx-media-of.c
> new file mode 100644
> index 0000000..018d05a
> --- /dev/null
> +++ b/drivers/staging/media/imx/imx-media-of.c
> @@ -0,0 +1,291 @@
> +/*
> + * Media driver for Freescale i.MX5/6 SOC
> + *
> + * Open Firmware parsing.
> + *
> + * Copyright (c) 2016 Mentor Graphics Inc.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + */
> +#include <linux/of_platform.h>
> +#include <media/v4l2-device.h>
> +#include <media/videobuf2-dma-contig.h>
> +#include <media/v4l2-subdev.h>
> +#include <media/v4l2-of.h>
> +#include <media/v4l2-ctrls.h>
Please sort out the list alphabetically.
> +#include <video/imx-ipu-v3.h>
> +#include "imx-media.h"
> +
> +static int of_add_pad_link(struct imx_media_dev *imxmd,
> + struct imx_media_pad *pad,
> + struct device_node *local_sd_node,
> + struct device_node *remote_sd_node,
> + int local_pad, int remote_pad)
> +{
> + dev_dbg(imxmd->dev, "%s: adding %s:%d -> %s:%d\n", __func__,
> + local_sd_node->name, local_pad,
> + remote_sd_node->name, remote_pad);
> +
> + return imx_media_add_pad_link(imxmd, pad, remote_sd_node, NULL,
> + local_pad, remote_pad);
> +}
> +
> +/* parse inputs property from a sensor node */
> +static void of_parse_sensor_inputs(struct imx_media_dev *imxmd,
> + struct imx_media_subdev *sensor,
> + struct device_node *sensor_np)
> +{
> + struct imx_media_sensor_input *sinput = &sensor->input;
> + int ret, i;
> +
> + for (i = 0; i < IMX_MEDIA_MAX_SENSOR_INPUTS; i++) {
> + const char *input_name;
> + u32 val;
> +
> + ret = of_property_read_u32_index(sensor_np, "inputs", i, &val);
> + if (ret)
> + break;
> +
> + sinput->value[i] = val;
> +
> + ret = of_property_read_string_index(sensor_np, "input-names",
> + i, &input_name);
> + /*
> + * if input-names not provided, they will be set using
> + * the subdev name once the sensor is known during
> + * async bind
> + */
> + if (!ret)
> + strncpy(sinput->name[i], input_name,
> + sizeof(sinput->name[i]));
> + }
> +
> + sinput->num = i;
> +
> + /* if no inputs provided just assume a single input */
> + if (sinput->num == 0)
> + sinput->num = 1;
> +}
> +
> +static void of_parse_sensor(struct imx_media_dev *imxmd,
> + struct imx_media_subdev *sensor,
> + struct device_node *sensor_np)
> +{
> + struct device_node *endpoint;
> +
> + of_parse_sensor_inputs(imxmd, sensor, sensor_np);
> +
> + endpoint = of_graph_get_next_endpoint(sensor_np, NULL);
> + if (endpoint) {
> + v4l2_of_parse_endpoint(endpoint, &sensor->sensor_ep);
> + of_node_put(endpoint);
> + }
> +}
> +
> +static int of_get_port_count(const struct device_node *np)
> +{
> + struct device_node *child;
> + int num = 0;
> +
> + /* if this node is itself a port, return 1 */
> + if (of_node_cmp(np->name, "port") == 0)
> + return 1;
> +
> + for_each_child_of_node(np, child) {
> + if (of_node_cmp(child->name, "port") == 0)
> + num++;
> + }
Unneeded bracers.
> + return num;
> +}
> +
> +/*
> + * find the remote device node and remote port id (remote pad #)
> + * given local endpoint node
> + */
> +static void of_get_remote_pad(struct device_node *epnode,
> + struct device_node **remote_node,
> + int *remote_pad)
> +{
> + struct device_node *rp, *rpp;
> + struct device_node *remote;
> +
> + rp = of_graph_get_remote_port(epnode);
> + rpp = of_graph_get_remote_port_parent(epnode);
> +
> + if (of_device_is_compatible(rpp, "fsl,imx6q-ipu")) {
> + /* the remote is one of the CSI ports */
> + remote = rp;
> + *remote_pad = 0;
> + of_node_put(rpp);
> + } else {
> + remote = rpp;
> + of_property_read_u32(rp, "reg", remote_pad);
> + of_node_put(rp);
> + }
> +
> + if (!remote || !of_device_is_available(remote)) {
> + of_node_put(remote);
> + *remote_node = NULL;
> + } else {
> + *remote_node = remote;
> + }
> +}
> +
> +static struct imx_media_subdev *
> +of_parse_subdev(struct imx_media_dev *imxmd, struct device_node *sd_np,
> + bool is_csi_port)
> +{
> + struct imx_media_subdev *imxsd;
> + int i, num_pads, ret;
> +
> + if (!of_device_is_available(sd_np)) {
> + dev_dbg(imxmd->dev, "%s: %s not enabled\n", __func__,
> + sd_np->name);
> + return NULL;
> + }
> +
> + /* register this subdev with async notifier */
> + imxsd = imx_media_add_async_subdev(imxmd, sd_np, NULL);
> + if (!imxsd)
> + return NULL;
> + if (IS_ERR(imxsd))
> + return imxsd;
if (IS_ERR_OR_NULL(imxsd))
return imxsd;
> +
> + if (is_csi_port) {
> + /*
> + * the ipu-csi has one sink port and one source port.
> + * The source port is not represented in the device tree,
> + * but is described by the internal pads and links later.
> + */
> + num_pads = 2;
> + imxsd->num_sink_pads = 1;
> + } else if (of_device_is_compatible(sd_np, "fsl,imx-mipi-csi2")) {
> + num_pads = of_get_port_count(sd_np);
> + /* the mipi csi2 receiver has only one sink port */
> + imxsd->num_sink_pads = 1;
> + } else if (of_device_is_compatible(sd_np, "imx-video-mux")) {
> + num_pads = of_get_port_count(sd_np);
> + /* for the video mux, all but the last port are sinks */
> + imxsd->num_sink_pads = num_pads - 1;
> + } else {
> + /* must be a sensor */
> + num_pads = 1;
> + imxsd->num_sink_pads = 0;
> + }
> +
> + if (imxsd->num_sink_pads >= num_pads)
> + return ERR_PTR(-EINVAL);
> +
> + imxsd->num_src_pads = num_pads - imxsd->num_sink_pads;
> +
> + dev_dbg(imxmd->dev, "%s: %s has %d pads (%d sink, %d src)\n",
> + __func__, sd_np->name, num_pads,
> + imxsd->num_sink_pads, imxsd->num_src_pads);
> +
> + if (imxsd->num_sink_pads == 0) {
> + /* this might be a sensor */
> + of_parse_sensor(imxmd, imxsd, sd_np);
> + }
Unneeded bracers.
> +
> + for (i = 0; i < num_pads; i++) {
> + struct device_node *epnode = NULL, *port, *remote_np;
> + struct imx_media_subdev *remote_imxsd;
> + struct imx_media_pad *pad;
> + int remote_pad;
Too deep indentation, may be move the cycle body into a separate function?
> +
> + /* init this pad */
> + pad = &imxsd->pad[i];
> + pad->pad.flags = (i < imxsd->num_sink_pads) ?
> + MEDIA_PAD_FL_SINK : MEDIA_PAD_FL_SOURCE;
> +
> + if (is_csi_port)
> + port = (i < imxsd->num_sink_pads) ? sd_np : NULL;
> + else
> + port = of_graph_get_port_by_id(sd_np, i);
> + if (!port)
> + continue;
> +
> + while ((epnode = of_get_next_child(port, epnode))) {
Please reuse for_each_child_of_node() here.
> + of_get_remote_pad(epnode, &remote_np, &remote_pad);
> + if (!remote_np) {
> + of_node_put(epnode);
Please remove of_node_put() here, of_get_next_child() does it.
> + continue;
> + }
> +
> + ret = of_add_pad_link(imxmd, pad, sd_np, remote_np,
> + i, remote_pad);
> + if (ret) {
> + imxsd = ERR_PTR(ret);
> + break;
> + }
> +
> + if (i < imxsd->num_sink_pads) {
> + /* follow sink endpoints upstream */
> + remote_imxsd = of_parse_subdev(imxmd,
> + remote_np,
> + false);
> + if (IS_ERR(remote_imxsd)) {
> + imxsd = remote_imxsd;
> + break;
> + }
> + }
> +
> + of_node_put(remote_np);
> + of_node_put(epnode);
> + }
> +
> + if (port != sd_np)
> + of_node_put(port);
> + if (IS_ERR(imxsd)) {
> + of_node_put(remote_np);
> + of_node_put(epnode);
> + break;
> + }
> + }
> +
> + return imxsd;
> +}
> +
> +int imx_media_of_parse(struct imx_media_dev *imxmd,
> + struct imx_media_subdev *(*csi)[4],
> + struct device_node *np)
> +{
> + struct device_node *csi_np;
> + struct imx_media_subdev *lcsi;
Please swap two lines above to get the reverse christmas tree ordering.
> + u32 ipu_id, csi_id;
> + int i, ret;
> +
> + for (i = 0; ; i++) {
> + csi_np = of_parse_phandle(np, "ports", i);
> + if (!csi_np)
> + break;
> +
> + lcsi = of_parse_subdev(imxmd, csi_np, true);
> + if (IS_ERR(lcsi)) {
> + ret = PTR_ERR(lcsi);
> + goto err_put;
> + }
> +
> + of_property_read_u32(csi_np, "reg", &csi_id);
Not sure if it is safe enough to ignore return value and potentially
left csi_id uninitialized.
> + ipu_id = of_alias_get_id(csi_np->parent, "ipu");
> +
> + if (ipu_id > 1 || csi_id > 1) {
> + dev_err(imxmd->dev, "%s: invalid ipu/csi id (%u/%u)\n",
> + __func__, ipu_id, csi_id);
> + ret = -EINVAL;
> + goto err_put;
> + }
> +
> + of_node_put(csi_np);
You can put the node right after of_alias_get_id() call, then in case
of error return right from the if block and remove the goto label.
> +
> + (*csi)[ipu_id * 2 + csi_id] = lcsi;
> + }
> +
> + return 0;
> +err_put:
> + of_node_put(csi_np);
> + return ret;
> +}
> diff --git a/drivers/staging/media/imx/imx-media-of.h b/drivers/staging/media/imx/imx-media-of.h
> new file mode 100644
> index 0000000..0c61b05
> --- /dev/null
> +++ b/drivers/staging/media/imx/imx-media-of.h
> @@ -0,0 +1,25 @@
> +/*
> + * V4L2 Media Controller Driver for Freescale i.MX5/6 SOC
> + *
> + * Open Firmware parsing.
> + *
> + * Copyright (c) 2016 Mentor Graphics Inc.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + */
> +#ifndef _IMX_MEDIA_OF_H
> +#define _IMX_MEDIA_OF_H
> +
I do believe you should include some headers or add declarations
of "struct imx_media_dev", "struct imx_media_subdev", "struct device_node".
> +struct imx_media_subdev *
> +imx_media_of_find_subdev(struct imx_media_dev *imxmd,
> + struct device_node *np,
> + const char *name);
> +
> +int imx_media_of_parse(struct imx_media_dev *dev,
> + struct imx_media_subdev *(*csi)[4],
> + struct device_node *np);
> +
> +#endif
> diff --git a/drivers/staging/media/imx/imx-media.h b/drivers/staging/media/imx/imx-media.h
> new file mode 100644
> index 0000000..6a018a9
> --- /dev/null
> +++ b/drivers/staging/media/imx/imx-media.h
> @@ -0,0 +1,299 @@
> +/*
> + * V4L2 Media Controller Driver for Freescale i.MX5/6 SOC
> + *
> + * Copyright (c) 2016 Mentor Graphics Inc.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + */
> +#ifndef _IMX_MEDIA_H
> +#define _IMX_MEDIA_H
Please insert here an empty line to improve readability.
> +#include <media/v4l2-device.h>
> +#include <media/v4l2-subdev.h>
> +#include <media/v4l2-ctrls.h>
> +#include <media/v4l2-of.h>
Please sort out the list alphabetically.
> +#include <media/videobuf2-dma-contig.h>
> +#include <video/imx-ipu-v3.h>
> +
> +/*
> + * This is somewhat arbitrary, but we need at least:
> + * - 2 camera interface subdevs
> + * - 3 IC subdevs
> + * - 2 CSI subdevs
> + * - 1 mipi-csi2 receiver subdev
> + * - 2 video-mux subdevs
> + * - 3 camera sensor subdevs (2 parallel, 1 mipi-csi2)
> + *
> + * And double the above numbers for quad i.mx!
> + */
[snip]
--
With best wishes,
Vladimir
[toc] | [prev] | [next] | [standalone]
| From | Steve Longerbeam <slongerbeam@gmail.com> |
|---|---|
| Date | 2017-01-06 02:30 +0100 |
| Subject | Re: [PATCH v2 10/19] media: Add i.MX media core driver |
| Message-ID | <sWrvX-2Jv-5@gated-at.bofh.it> |
| In reply to | #1550822 |
On 01/04/2017 05:33 AM, Vladimir Zapolskiy wrote:
> Hi Steve,
>
> On 01/03/2017 10:57 PM, Steve Longerbeam wrote:
>> Add the core media driver for i.MX SOC.
>>
>> Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
>> ---
>> Documentation/devicetree/bindings/media/imx.txt | 205 +++++
> v2 was sent before getting Rob's review comments, but still they
> should be addressed in v3.
yes, those changes will be part of v3 as well.
>
> Also I would suggest to separate device tree binding documentation
> change and place it as the first patch in the series, this should
> make the following DTS changes valid.
done.
>
>> Documentation/media/v4l-drivers/imx.rst | 430 ++++++++++
>> drivers/staging/media/Kconfig | 2 +
>> drivers/staging/media/Makefile | 1 +
>> drivers/staging/media/imx/Kconfig | 8 +
>> drivers/staging/media/imx/Makefile | 6 +
>> drivers/staging/media/imx/TODO | 18 +
>> drivers/staging/media/imx/imx-media-common.c | 985 ++++++++++++++++++++++
>> drivers/staging/media/imx/imx-media-dev.c | 479 +++++++++++
>> drivers/staging/media/imx/imx-media-fim.c | 509 +++++++++++
>> drivers/staging/media/imx/imx-media-internal-sd.c | 457 ++++++++++
>> drivers/staging/media/imx/imx-media-of.c | 291 +++++++
>> drivers/staging/media/imx/imx-media-of.h | 25 +
>> drivers/staging/media/imx/imx-media.h | 299 +++++++
>> include/media/imx.h | 15 +
>> include/uapi/Kbuild | 1 +
>> include/uapi/linux/v4l2-controls.h | 4 +
>> include/uapi/media/Kbuild | 2 +
>> include/uapi/media/imx.h | 30 +
> Probably Greg should ack the UAPI changes, you may consider
> to split them into a separate patch.
I split out the one-line addition to include/uapi/Kbuild with an empty
include/uapi/media/Kbuild into a new patch "UAPI: Add media UAPI Kbuild
file".
Also added a patch "media: Add userspace header file for i.MX".
>
>
>> +
>> +struct imx_media_subdev *
>> +imx_media_find_subdev_by_sd(struct imx_media_dev *imxmd,
>> + struct v4l2_subdev *sd)
>> +{
>> + struct imx_media_subdev *imxsd;
>> + int i, ret = -ENODEV;
>> +
>> + for (i = 0; i < imxmd->num_subdevs; i++) {
>> + imxsd = &imxmd->subdev[i];
>> + if (sd == imxsd->sd) {
> This can be simplifed:
>
> ...
>
> if (sd == imxsd->sd)
> return imxsd;
> }
>
> return ERR_PTR(-ENODEV);
yep, done.
>
>> +struct imx_media_subdev *
>> +imx_media_find_subdev_by_id(struct imx_media_dev *imxmd, u32 grp_id)
>> +{
>> + struct imx_media_subdev *imxsd;
>> + int i, ret = -ENODEV;
>> +
>> + for (i = 0; i < imxmd->num_subdevs; i++) {
>> + imxsd = &imxmd->subdev[i];
>> + if (imxsd->sd && imxsd->sd->grp_id == grp_id) {
>> + ret = 0;
>> + break;
> This can be simplifed:
>
> ...
>
> if (imxsd->sd && imxsd->sd->grp_id == grp_i)
> return imxsd;
> }
>
> return ERR_PTR(-ENODEV);
done.
>
>
>> diff --git a/drivers/staging/media/imx/imx-media-dev.c b/drivers/staging/media/imx/imx-media-dev.c
>> new file mode 100644
>> index 0000000..8d22730
>> --- /dev/null
>> +++ b/drivers/staging/media/imx/imx-media-dev.c
>> @@ -0,0 +1,479 @@
>> +/*
>> + * V4L2 Media Controller Driver for Freescale i.MX5/6 SOC
>> + *
>> + * Copyright (c) 2016 Mentor Graphics Inc.
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation; either version 2 of the License, or
>> + * (at your option) any later version.
>> + */
>> +#include <linux/module.h>
>> +#include <linux/delay.h>
>> +#include <linux/fs.h>
>> +#include <linux/timer.h>
>> +#include <linux/sched.h>
>> +#include <linux/slab.h>
>> +#include <linux/spinlock.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/pinctrl/consumer.h>
>> +#include <linux/of_platform.h>
>> +#include <media/v4l2-ioctl.h>
>> +#include <media/v4l2-ctrls.h>
>> +#include <media/v4l2-event.h>
>> +#include <media/v4l2-mc.h>
> Please sort out the list of headers alphabetically.
done.
>> +#include <video/imx-ipu-v3.h>
>> +#include <media/imx.h>
>> +#include "imx-media.h"
>> +#include "imx-media-of.h"
>> +
>> +#define DEVICE_NAME "imx-media"
> I suppose you don't need this macro.
sure why not, removed.
>
> [snip]
>
>> + */
>> +static int imx_media_create_links(struct imx_media_dev *imxmd)
>> +{
>> + struct imx_media_subdev *local_sd;
>> + struct imx_media_subdev *remote_sd;
>> + struct v4l2_subdev *source, *sink;
>> + struct imx_media_link *link;
>> + struct imx_media_pad *pad;
>> + u16 source_pad, sink_pad;
>> + int num_pads, i, j, k;
>> + int ret = 0;
>> +
>> + for (i = 0; i < imxmd->num_subdevs; i++) {
>> + local_sd = &imxmd->subdev[i];
>> + num_pads = local_sd->num_sink_pads + local_sd->num_src_pads;
>> +
>> + for (j = 0; j < num_pads; j++) {
>> + pad = &local_sd->pad[j];
>> +
>> + for (k = 0; k < pad->num_links; k++) {
>> + link = &pad->link[k];
>> +
>> <snip>
> Indentation depth is quite terrific.
yes, it needs to iterate by subdev, pads in the subdev, then links
in the pad. But I moved the code under the innermost loop into a
function imx_media_create_link(), which creates a single media
link. So it's not so much an eye-sore now.
>
>
>> +static struct platform_driver imx_media_pdrv = {
>> + .probe = imx_media_probe,
>> + .remove = imx_media_remove,
>> + .driver = {
>> + .name = DEVICE_NAME,
>> + .owner = THIS_MODULE,
> Setting of .owner is not needed nowadays IIRC.
a quick look at struct device_driver definition didn't mention that
.owner member is deprecated or not needed, so for now I'll leave
it in place. We can revisit that later.
>
>> + .of_match_table = imx_media_dt_ids,
>> + },
>> +};
>> +
>> +module_platform_driver(imx_media_pdrv);
>> +
>> +MODULE_DESCRIPTION("i.MX5/6 v4l2 media controller driver");
>> +MODULE_AUTHOR("Steve Longerbeam <steve_longerbeam@mentor.com>");
>> +MODULE_LICENSE("GPL");
>> diff --git a/drivers/staging/media/imx/imx-media-fim.c b/drivers/staging/media/imx/imx-media-fim.c
>> new file mode 100644
>> index 0000000..52bfa8d
>> --- /dev/null
>> +++ b/drivers/staging/media/imx/imx-media-fim.c
>> @@ -0,0 +1,509 @@
>> +/*
>> + * Frame Interval Monitor.
>> + *
>> + * Copyright (c) 2016 Mentor Graphics Inc.
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation; either version 2 of the License, or
>> + * (at your option) any later version.
>> + */
>> +#include <linux/module.h>
>> +#include <linux/delay.h>
>> +#include <linux/slab.h>
>> +#include <linux/platform_device.h>
>> +#ifdef CONFIG_IMX_GPT_ICAP
>> +#include <linux/mxc_icap.h>
>> +#endif
> This looks clumsy. Include it unconditionally, if needed
> do #ifdef's inside the header file.
The i.MX input capture patch will come later. These are just
placeholders until then, the config name could even change.
For now I just removed the above conditional include.
>
>> +#include <media/v4l2-subdev.h>
>> +#include <media/v4l2-of.h>
>> +#include <media/v4l2-ctrls.h>
> Please sort out the list alphabetically.
done.
>> + if (IS_ENABLED(CONFIG_IMX_GPT_ICAP)) {
>> + ret = of_property_read_u32_array(fim_np,
>> + "input-capture-channel",
>> + icap, 2);
>> + if (!ret) {
>> + fim->icap_channel = icap[0];
>> + fim->icap_flags = icap[1];
>> + }
> Should you return error otherwise?
nope, it's an optional property.
>
>> diff --git a/drivers/staging/media/imx/imx-media-of.c b/drivers/staging/media/imx/imx-media-of.c
>> new file mode 100644
>> index 0000000..018d05a
>> --- /dev/null
>> +++ b/drivers/staging/media/imx/imx-media-of.c
>> @@ -0,0 +1,291 @@
>> +/*
>> + * Media driver for Freescale i.MX5/6 SOC
>> + *
>> + * Open Firmware parsing.
>> + *
>> + * Copyright (c) 2016 Mentor Graphics Inc.
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation; either version 2 of the License, or
>> + * (at your option) any later version.
>> + */
>> +#include <linux/of_platform.h>
>> +#include <media/v4l2-device.h>
>> +#include <media/videobuf2-dma-contig.h>
>> +#include <media/v4l2-subdev.h>
>> +#include <media/v4l2-of.h>
>> +#include <media/v4l2-ctrls.h>
> Please sort out the list alphabetically.
done.
>>
>> +static int of_get_port_count(const struct device_node *np)
>> +{
>> + struct device_node *child;
>> + int num = 0;
>> +
>> + /* if this node is itself a port, return 1 */
>> + if (of_node_cmp(np->name, "port") == 0)
>> + return 1;
>> +
>> + for_each_child_of_node(np, child) {
>> + if (of_node_cmp(child->name, "port") == 0)
>> + num++;
>> + }
> Unneeded bracers.
fixed.
>
>> +static struct imx_media_subdev *
>> +of_parse_subdev(struct imx_media_dev *imxmd, struct device_node *sd_np,
>> + bool is_csi_port)
>> +{
>> + struct imx_media_subdev *imxsd;
>> + int i, num_pads, ret;
>> +
>> + if (!of_device_is_available(sd_np)) {
>> + dev_dbg(imxmd->dev, "%s: %s not enabled\n", __func__,
>> + sd_np->name);
>> + return NULL;
>> + }
>> +
>> + /* register this subdev with async notifier */
>> + imxsd = imx_media_add_async_subdev(imxmd, sd_np, NULL);
>> + if (!imxsd)
>> + return NULL;
>> + if (IS_ERR(imxsd))
>> + return imxsd;
> if (IS_ERR_OR_NULL(imxsd))
> return imxsd;
yep, done.
>> +
>> + if (imxsd->num_sink_pads == 0) {
>> + /* this might be a sensor */
>> + of_parse_sensor(imxmd, imxsd, sd_np);
>> + }
> Unneeded bracers.
ok removed.
>
>> +
>> + for (i = 0; i < num_pads; i++) {
>> + struct device_node *epnode = NULL, *port, *remote_np;
>> + struct imx_media_subdev *remote_imxsd;
>> + struct imx_media_pad *pad;
>> + int remote_pad;
> Too deep indentation, may be move the cycle body into a separate function?
in this case I prefer not to, of_parse_subdev() is called recursively, and I
think it's bad form to hide that fact by moving the recursive call into a
separate function. The indentation is not that deep, only two loops deep.
>
>> +
>> + /* init this pad */
>> + pad = &imxsd->pad[i];
>> + pad->pad.flags = (i < imxsd->num_sink_pads) ?
>> + MEDIA_PAD_FL_SINK : MEDIA_PAD_FL_SOURCE;
>> +
>> + if (is_csi_port)
>> + port = (i < imxsd->num_sink_pads) ? sd_np : NULL;
>> + else
>> + port = of_graph_get_port_by_id(sd_np, i);
>> + if (!port)
>> + continue;
>> +
>> + while ((epnode = of_get_next_child(port, epnode))) {
> Please reuse for_each_child_of_node() here.
done.
>
>> + of_get_remote_pad(epnode, &remote_np, &remote_pad);
>> + if (!remote_np) {
>> + of_node_put(epnode);
> Please remove of_node_put() here, of_get_next_child() does it.
oops, good catch, fixed.
>
>> + continue;
>> + }
>> +
>> + ret = of_add_pad_link(imxmd, pad, sd_np, remote_np,
>> + i, remote_pad);
>> + if (ret) {
>> + imxsd = ERR_PTR(ret);
>> + break;
>> + }
>> +
>> + if (i < imxsd->num_sink_pads) {
>> + /* follow sink endpoints upstream */
>> + remote_imxsd = of_parse_subdev(imxmd,
>> + remote_np,
>> + false);
>> + if (IS_ERR(remote_imxsd)) {
>> + imxsd = remote_imxsd;
>> + break;
>> + }
>> + }
>> +
>> + of_node_put(remote_np);
>> + of_node_put(epnode);
also removed this put of epnode.
>>
>> +
>> +int imx_media_of_parse(struct imx_media_dev *imxmd,
>> + struct imx_media_subdev *(*csi)[4],
>> + struct device_node *np)
>> +{
>> + struct device_node *csi_np;
>> + struct imx_media_subdev *lcsi;
> Please swap two lines above to get the reverse christmas tree ordering.
done.
>
>> + u32 ipu_id, csi_id;
>> + int i, ret;
>> +
>> + for (i = 0; ; i++) {
>> + csi_np = of_parse_phandle(np, "ports", i);
>> + if (!csi_np)
>> + break;
>> +
>> + lcsi = of_parse_subdev(imxmd, csi_np, true);
>> + if (IS_ERR(lcsi)) {
>> + ret = PTR_ERR(lcsi);
>> + goto err_put;
>> + }
>> +
>> + of_property_read_u32(csi_np, "reg", &csi_id);
> Not sure if it is safe enough to ignore return value and potentially
> left csi_id uninitialized.
The CSI nodes are port nodes, and the reg property is required,
so I added a check and error return here.
>
>> + ipu_id = of_alias_get_id(csi_np->parent, "ipu");
>> +
>> + if (ipu_id > 1 || csi_id > 1) {
>> + dev_err(imxmd->dev, "%s: invalid ipu/csi id (%u/%u)\n",
>> + __func__, ipu_id, csi_id);
>> + ret = -EINVAL;
>> + goto err_put;
>> + }
>> +
>> + of_node_put(csi_np);
> You can put the node right after of_alias_get_id() call, then in case
> of error return right from the if block and remove the goto label.
done.
>
>> +
>> + (*csi)[ipu_id * 2 + csi_id] = lcsi;
>> + }
>> +
>> + return 0;
>> +err_put:
>> + of_node_put(csi_np);
>> + return ret;
>> +}
>> diff --git a/drivers/staging/media/imx/imx-media-of.h b/drivers/staging/media/imx/imx-media-of.h
>> new file mode 100644
>> index 0000000..0c61b05
>> --- /dev/null
>> +++ b/drivers/staging/media/imx/imx-media-of.h
>> @@ -0,0 +1,25 @@
>> +/*
>> + * V4L2 Media Controller Driver for Freescale i.MX5/6 SOC
>> + *
>> + * Open Firmware parsing.
>> + *
>> + * Copyright (c) 2016 Mentor Graphics Inc.
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation; either version 2 of the License, or
>> + * (at your option) any later version.
>> + */
>> +#ifndef _IMX_MEDIA_OF_H
>> +#define _IMX_MEDIA_OF_H
>> +
> I do believe you should include some headers or add declarations
> of "struct imx_media_dev", "struct imx_media_subdev", "struct device_node".
actually imx-media-of.h isn't really needed, I just moved those prototypes
into imx-media.h and removed imx-media-of.h.
>> diff --git a/drivers/staging/media/imx/imx-media.h b/drivers/staging/media/imx/imx-media.h
>> new file mode 100644
>> index 0000000..6a018a9
>> --- /dev/null
>> +++ b/drivers/staging/media/imx/imx-media.h
>> @@ -0,0 +1,299 @@
>> +/*
>> + * V4L2 Media Controller Driver for Freescale i.MX5/6 SOC
>> + *
>> + * Copyright (c) 2016 Mentor Graphics Inc.
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation; either version 2 of the License, or
>> + * (at your option) any later version.
>> + */
>> +#ifndef _IMX_MEDIA_H
>> +#define _IMX_MEDIA_H
> Please insert here an empty line to improve readability.
done.
>
>> +#include <media/v4l2-device.h>
>> +#include <media/v4l2-subdev.h>
>> +#include <media/v4l2-ctrls.h>
>> +#include <media/v4l2-of.h>
> Please sort out the list alphabetically.
done.
Steve
[toc] | [prev] | [next] | [standalone]
| From | Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com> |
|---|---|
| Date | 2017-01-04 16:00 +0100 |
| Subject | Re: [PATCH v2 13/19] media: imx: Add IC subdev drivers |
| Message-ID | <sVVcK-62m-25@gated-at.bofh.it> |
| In reply to | #1550191 |
On 01/03/2017 10:57 PM, Steve Longerbeam wrote:
> This is a set of three media entity subdevice drivers for the i.MX
> Image Converter. The i.MX IC module contains three independent
> "tasks":
>
> - Pre-processing Encode task: video frames are routed directly from
> the CSI and can be scaled, color-space converted, and rotated.
> Scaled output is limited to 1024x1024 resolution. Output frames
> are routed to the camera interface entities (camif).
>
> - Pre-processing Viewfinder task: this task can perform the same
> conversions as the pre-process encode task, but in addition can
> be used for hardware motion compensated deinterlacing. Frames can
> come either directly from the CSI or from the SMFC entities (memory
> buffers via IDMAC channels). Scaled output is limited to 1024x1024
> resolution. Output frames can be routed to various sinks including
> the post-processing task entities.
>
> - Post-processing task: same conversions as pre-process encode. However
> this entity sends frames to the i.MX IPU image converter which supports
> image tiling, which allows scaled output up to 4096x4096 resolution.
> Output frames can be routed to the camera interfaces.
>
> Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
> ---
[snip]
> +static int imx_ic_probe(struct platform_device *pdev)
> +{
> + struct imx_media_internal_sd_platformdata *pdata;
> + struct imx_ic_priv *priv;
> + int ret;
> +
> + priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
> + if (!priv)
> + return -ENOMEM;
> +
> + platform_set_drvdata(pdev, &priv->sd);
> + priv->dev = &pdev->dev;
> +
> + /* get our ipu_id, grp_id and IC task id */
> + pdata = priv->dev->platform_data;
> + priv->ipu_id = pdata->ipu_id;
> + switch (pdata->grp_id) {
> + case IMX_MEDIA_GRP_ID_IC_PRPENC:
> + priv->task_id = IC_TASK_ENCODER;
> + break;
> + case IMX_MEDIA_GRP_ID_IC_PRPVF:
> + priv->task_id = IC_TASK_VIEWFINDER;
> + break;
> + case IMX_MEDIA_GRP_ID_IC_PP0...IMX_MEDIA_GRP_ID_IC_PP3:
> + priv->task_id = IC_TASK_POST_PROCESSOR;
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + v4l2_subdev_init(&priv->sd, ic_ops[priv->task_id]->subdev_ops);
> + v4l2_set_subdevdata(&priv->sd, priv);
> + priv->sd.internal_ops = ic_ops[priv->task_id]->internal_ops;
> + priv->sd.entity.ops = ic_ops[priv->task_id]->entity_ops;
> + priv->sd.entity.function = MEDIA_ENT_F_PROC_VIDEO_SCALER;
> + priv->sd.dev = &pdev->dev;
> + priv->sd.owner = THIS_MODULE;
> + priv->sd.flags = V4L2_SUBDEV_FL_HAS_DEVNODE | V4L2_SUBDEV_FL_HAS_EVENTS;
> + priv->sd.grp_id = pdata->grp_id;
> + strncpy(priv->sd.name, pdata->sd_name, sizeof(priv->sd.name));
> +
> + ret = ic_ops[priv->task_id]->init(priv);
> + if (ret)
> + return ret;
> +
> + ret = v4l2_async_register_subdev(&priv->sd);
> + if (ret)
> + goto remove;
> +
> + return 0;
> +remove:
> + ic_ops[priv->task_id]->remove(priv);
> + return ret;
if (ret)
ic_ops[priv->task_id]->remove(priv);
return ret;
as an alternative.
[snip]
> +static const struct platform_device_id imx_ic_ids[] = {
> + { .name = "imx-ipuv3-ic" },
> + { },
> +};
> +MODULE_DEVICE_TABLE(platform, imx_ic_ids);
> +
> +static struct platform_driver imx_ic_driver = {
> + .probe = imx_ic_probe,
> + .remove = imx_ic_remove,
> + .id_table = imx_ic_ids,
> + .driver = {
> + .name = "imx-ipuv3-ic",
> + .owner = THIS_MODULE,
Please drop .owner assignment.
> + },
> +};
> +module_platform_driver(imx_ic_driver);
> +
> +MODULE_DESCRIPTION("i.MX IC subdev driver");
> +MODULE_AUTHOR("Steve Longerbeam <steve_longerbeam@mentor.com>");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:imx-ipuv3-ic");
> diff --git a/drivers/staging/media/imx/imx-ic-pp.c b/drivers/staging/media/imx/imx-ic-pp.c
> new file mode 100644
> index 0000000..5ef0581
> --- /dev/null
> +++ b/drivers/staging/media/imx/imx-ic-pp.c
> @@ -0,0 +1,636 @@
> +/*
> + * V4L2 IC Post-Processor Subdev for Freescale i.MX5/6 SOC
> + *
> + * Copyright (c) 2014-2016 Mentor Graphics Inc.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + */
> +#include <linux/module.h>
> +#include <linux/delay.h>
> +#include <linux/fs.h>
> +#include <linux/timer.h>
> +#include <linux/sched.h>
> +#include <linux/slab.h>
> +#include <linux/interrupt.h>
> +#include <linux/platform_device.h>
> +#include <linux/pinctrl/consumer.h>
> +#include <media/v4l2-device.h>
> +#include <media/v4l2-ioctl.h>
> +#include <media/videobuf2-dma-contig.h>
> +#include <media/v4l2-subdev.h>
> +#include <media/v4l2-of.h>
> +#include <media/v4l2-ctrls.h>
Please sort the list of headers alphabetically.
> +#include <media/imx.h>
> +#include <video/imx-ipu-image-convert.h>
> +#include "imx-media.h"
> +#include "imx-ic.h"
> +
[snip]
> +
> +static int pp_start(struct pp_priv *priv)
> +{
> + struct imx_ic_priv *ic_priv = priv->ic_priv;
> + struct ipu_image image_in, image_out;
> + const struct imx_media_pixfmt *incc;
> + struct v4l2_mbus_framefmt *infmt;
> + int i, in_size, ret;
> +
> + /* ask the sink for the buffer ring */
> + ret = v4l2_subdev_call(priv->sink_sd, core, ioctl,
> + IMX_MEDIA_REQ_DMA_BUF_SINK_RING,
> + &priv->out_ring);
> + if (ret)
> + return ret;
> +
> + imx_media_mbus_fmt_to_ipu_image(&image_in,
> + &priv->format_mbus[priv->input_pad]);
> + imx_media_mbus_fmt_to_ipu_image(&image_out,
> + &priv->format_mbus[priv->output_pad]);
> +
> + priv->ipu = priv->md->ipu[ic_priv->ipu_id];
> + priv->ic_ctx = ipu_image_convert_prepare(priv->ipu,
> + IC_TASK_POST_PROCESSOR,
> + &image_in, &image_out,
> + priv->rot_mode,
> + pp_convert_complete, priv);
> + if (IS_ERR(priv->ic_ctx))
> + return PTR_ERR(priv->ic_ctx);
> +
> + infmt = &priv->format_mbus[priv->input_pad];
> + incc = priv->cc[priv->input_pad];
> + in_size = (infmt->width * incc->bpp * infmt->height) >> 3;
> +
> + if (priv->in_ring) {
> + v4l2_warn(&ic_priv->sd, "%s: dma-buf ring was not freed\n",
> + __func__);
> + imx_media_free_dma_buf_ring(priv->in_ring);
> + }
> +
> + priv->in_ring = imx_media_alloc_dma_buf_ring(priv->md,
> + &priv->src_sd->entity,
> + &ic_priv->sd.entity,
> + in_size,
> + IMX_MEDIA_MIN_RING_BUFS,
> + true);
> + if (IS_ERR(priv->in_ring)) {
> + v4l2_err(&ic_priv->sd,
> + "failed to alloc dma-buf ring\n");
> + ret = PTR_ERR(priv->in_ring);
> + priv->in_ring = NULL;
> + goto out_unprep;
> + }
> +
> + for (i = 0; i < IMX_MEDIA_MIN_RING_BUFS; i++)
> + imx_media_dma_buf_queue(priv->in_ring, i);
> +
> + priv->out_run = kzalloc(IMX_MEDIA_MAX_RING_BUFS *
> + sizeof(*priv->out_run), GFP_KERNEL);
> + if (!priv->out_run) {
> + v4l2_err(&ic_priv->sd, "failed to alloc src ring runs\n");
In OOM situation the core will report it, probably you can drop the message.
> + ret = -ENOMEM;
> + goto out_free_ring;
> + }
> +
> + priv->stop = false;
> +
> + return 0;
> +
> +out_free_ring:
> + imx_media_free_dma_buf_ring(priv->in_ring);
> + priv->in_ring = NULL;
> +out_unprep:
> + ipu_image_convert_unprepare(priv->ic_ctx);
> + return ret;
> +}
> +
[snip]
> diff --git a/drivers/staging/media/imx/imx-ic-prpenc.c b/drivers/staging/media/imx/imx-ic-prpenc.c
> new file mode 100644
> index 0000000..e17216b
> --- /dev/null
> +++ b/drivers/staging/media/imx/imx-ic-prpenc.c
> @@ -0,0 +1,1037 @@
> +/*
> + * V4L2 Capture IC Encoder Subdev for Freescale i.MX5/6 SOC
> + *
> + * This subdevice handles capture of video frames from the CSI, which
> + * are routed directly to the Image Converter preprocess encode task,
> + * for resizing, colorspace conversion, and rotation.
> + *
> + * Copyright (c) 2012-2016 Mentor Graphics Inc.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + */
> +#include <linux/module.h>
> +#include <linux/delay.h>
> +#include <linux/fs.h>
> +#include <linux/timer.h>
> +#include <linux/sched.h>
> +#include <linux/slab.h>
> +#include <linux/interrupt.h>
> +#include <linux/spinlock.h>
> +#include <linux/platform_device.h>
> +#include <linux/pinctrl/consumer.h>
> +#include <media/v4l2-device.h>
> +#include <media/v4l2-ioctl.h>
> +#include <media/videobuf2-dma-contig.h>
> +#include <media/v4l2-subdev.h>
> +#include <media/v4l2-of.h>
> +#include <media/v4l2-ctrls.h>
Please sort the list of headers alphabetically.
> +#include <media/imx.h>
> +#include "imx-media.h"
> +#include "imx-ic.h"
> +
[snip]
> +static irqreturn_t prpenc_eof_interrupt(int irq, void *dev_id)
> +{
> + struct prpenc_priv *priv = dev_id;
> + struct imx_media_dma_buf *done, *next;
> + struct ipuv3_channel *channel;
> + unsigned long flags;
> +
> + spin_lock_irqsave(&priv->irqlock, flags);
Here spin_lock(&priv->irqlock) should be sufficient.
> +
> + if (priv->last_eof) {
> + complete(&priv->last_eof_comp);
> + priv->last_eof = false;
> + goto unlock;
> + }
> +
> + /* inform CSI of this EOF so it can monitor frame intervals */
> + v4l2_subdev_call(priv->src_sd, core, interrupt_service_routine,
> + 0, NULL);
> +
> + channel = (ipu_rot_mode_is_irt(priv->rot_mode)) ?
> + priv->enc_rot_out_ch : priv->enc_ch;
> +
> + done = imx_media_dma_buf_get_active(priv->out_ring);
> + /* give the completed buffer to the sink */
> + if (!WARN_ON(!done))
> + imx_media_dma_buf_done(done, IMX_MEDIA_BUF_STATUS_DONE);
> +
> + /* priv->next buffer is now the active one */
> + imx_media_dma_buf_set_active(priv->next);
> +
> + /* bump the EOF timeout timer */
> + mod_timer(&priv->eof_timeout_timer,
> + jiffies + msecs_to_jiffies(IMX_MEDIA_EOF_TIMEOUT));
> +
> + if (ipu_idmac_buffer_is_ready(channel, priv->ipu_buf_num))
> + ipu_idmac_clear_buffer(channel, priv->ipu_buf_num);
> +
> + /* get next queued buffer */
> + next = imx_media_dma_buf_get_next_queued(priv->out_ring);
> +
> + ipu_cpmem_set_buffer(channel, priv->ipu_buf_num, next->phys);
> + ipu_idmac_select_buffer(channel, priv->ipu_buf_num);
> +
> + /* toggle IPU double-buffer index */
> + priv->ipu_buf_num ^= 1;
> + priv->next = next;
> +
> +unlock:
> + spin_unlock_irqrestore(&priv->irqlock, flags);
> + return IRQ_HANDLED;
> +}
[snip]
> +static int prpenc_registered(struct v4l2_subdev *sd)
> +{
> + struct prpenc_priv *priv = sd_to_priv(sd);
> + struct imx_media_subdev *imxsd;
> + struct imx_media_pad *pad;
> + int i, ret;
> +
> + /* get media device */
> + priv->md = dev_get_drvdata(sd->v4l2_dev->dev);
> +
> + imxsd = imx_media_find_subdev_by_sd(priv->md, sd);
> + if (IS_ERR(imxsd))
> + return PTR_ERR(imxsd);
> +
> + if (imxsd->num_sink_pads != 1 || imxsd->num_src_pads != 1)
> + return -EINVAL;
> +
> + for (i = 0; i < PRPENC_NUM_PADS; i++) {
> + pad = &imxsd->pad[i];
> + priv->pad[i] = pad->pad;
> + if (priv->pad[i].flags & MEDIA_PAD_FL_SINK)
> + priv->input_pad = i;
> + else
> + priv->output_pad = i;
> +
> + /* set a default mbus format */
> + ret = imx_media_init_mbus_fmt(&priv->format_mbus[i],
> + 640, 480, 0, V4L2_FIELD_NONE,
> + &priv->cc[i]);
> + if (ret)
> + return ret;
> + }
> +
> + ret = prpenc_init_controls(priv);
> + if (ret)
> + return ret;
> +
> + ret = media_entity_pads_init(&sd->entity, PRPENC_NUM_PADS, priv->pad);
> + if (ret)
> + goto free_ctrls;
> +
> + return 0;
> +free_ctrls:
> + v4l2_ctrl_handler_free(&priv->ctrl_hdlr);
> + return ret;
if (ret)
v4l2_ctrl_handler_free(&priv->ctrl_hdlr);
return ret;
version is shorter.
> +}
[snip]
> diff --git a/drivers/staging/media/imx/imx-ic-prpvf.c b/drivers/staging/media/imx/imx-ic-prpvf.c
> new file mode 100644
> index 0000000..53ce006
> --- /dev/null
> +++ b/drivers/staging/media/imx/imx-ic-prpvf.c
> @@ -0,0 +1,1180 @@
> +/*
> + * V4L2 IC Deinterlacer Subdev for Freescale i.MX5/6 SOC
> + *
> + * Copyright (c) 2014-2016 Mentor Graphics Inc.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + */
> +#include <linux/module.h>
> +#include <linux/delay.h>
> +#include <linux/fs.h>
> +#include <linux/timer.h>
> +#include <linux/sched.h>
> +#include <linux/slab.h>
> +#include <linux/interrupt.h>
> +#include <linux/platform_device.h>
> +#include <linux/pinctrl/consumer.h>
> +#include <media/v4l2-device.h>
> +#include <media/v4l2-ioctl.h>
> +#include <media/videobuf2-dma-contig.h>
> +#include <media/v4l2-subdev.h>
> +#include <media/v4l2-of.h>
> +#include <media/v4l2-ctrls.h>
Please sort the list of headers alphabetically.
> +#include <media/imx.h>
> +#include "imx-media.h"
> +#include "imx-ic.h"
> +
> +/*
[snip]
> +/* prpvf_out_ch EOF interrupt (progressive frame ready) */
> +static irqreturn_t prpvf_out_eof_interrupt(int irq, void *dev_id)
> +{
> + struct prpvf_priv *priv = dev_id;
> + struct imx_media_dma_buf *done;
> + unsigned long flags;
> +
> + spin_lock_irqsave(&priv->irqlock, flags);
Here spin_lock(&priv->irqlock) should be sufficient.
> +
> + if (priv->last_eof) {
> + complete(&priv->last_eof_comp);
> + priv->last_eof = false;
> + goto unlock;
> + }
> +
> + if (priv->csi_direct) {
> + /* inform CSI of this EOF so it can monitor frame intervals */
> + /* FIXME: frames are coming in twice as fast in direct path! */
> + v4l2_subdev_call(priv->src_sd, core, interrupt_service_routine,
> + 0, NULL);
> + }
> +
> + done = imx_media_dma_buf_get_active(priv->out_ring);
> + /* give the completed buffer to the sink */
> + if (!WARN_ON(!done))
> + imx_media_dma_buf_done(done, IMX_MEDIA_BUF_STATUS_DONE);
> +
> + if (!priv->csi_direct) {
> + /* we're done with the input buffer, queue it back */
> + imx_media_dma_buf_queue(priv->in_ring,
> + priv->curr_in_buf->index);
> +
> + /* current input buffer is now last */
> + priv->last_in_buf = priv->curr_in_buf;
> + } else {
> + /*
> + * priv->next buffer is now the active one due
> + * to IPU double-buffering
> + */
> + imx_media_dma_buf_set_active(priv->next_out_buf);
> + }
> +
> + /* bump the EOF timeout timer */
> + mod_timer(&priv->eof_timeout_timer,
> + jiffies + msecs_to_jiffies(IMX_MEDIA_EOF_TIMEOUT));
> +
> + if (priv->csi_direct) {
> + prepare_prpvf_out_buffer(priv);
> + /* toggle IPU double-buffer index */
> + priv->ipu_buf_num ^= 1;
> + }
> +
> +unlock:
> + spin_unlock_irqrestore(&priv->irqlock, flags);
> + return IRQ_HANDLED;
> +}
> +
[snip]
> diff --git a/drivers/staging/media/imx/imx-ic.h b/drivers/staging/media/imx/imx-ic.h
> new file mode 100644
> index 0000000..9aed5f5
> --- /dev/null
> +++ b/drivers/staging/media/imx/imx-ic.h
> @@ -0,0 +1,36 @@
> +/*
> + * V4L2 Image Converter Subdev for Freescale i.MX5/6 SOC
> + *
> + * Copyright (c) 2016 Mentor Graphics Inc.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + */
> +#ifndef _IMX_IC_H
> +#define _IMX_IC_H
> +
Please add header files or declarations of all used structs.
> +struct imx_ic_priv {
> + struct device *dev;
> + struct v4l2_subdev sd;
> + int ipu_id;
> + int task_id;
> + void *task_priv;
> +};
> +
> +struct imx_ic_ops {
> + struct v4l2_subdev_ops *subdev_ops;
> + struct v4l2_subdev_internal_ops *internal_ops;
> + struct media_entity_operations *entity_ops;
> +
> + int (*init)(struct imx_ic_priv *ic_priv);
> + void (*remove)(struct imx_ic_priv *ic_priv);
> +};
> +
> +extern struct imx_ic_ops imx_ic_prpenc_ops;
> +extern struct imx_ic_ops imx_ic_prpvf_ops;
> +extern struct imx_ic_ops imx_ic_pp_ops;
> +
> +#endif
> +
>
--
With best wishes,
Vladimir
[toc] | [prev] | [next] | [standalone]
| From | Steve Longerbeam <slongerbeam@gmail.com> |
|---|---|
| Date | 2017-01-06 19:40 +0100 |
| Subject | Re: [PATCH v2 13/19] media: imx: Add IC subdev drivers |
| Message-ID | <sWHAK-5Op-7@gated-at.bofh.it> |
| In reply to | #1550895 |
On 01/04/2017 06:48 AM, Vladimir Zapolskiy wrote:
> On 01/03/2017 10:57 PM, Steve Longerbeam wrote:
>
>> +
>> + ret = ic_ops[priv->task_id]->init(priv);
>> + if (ret)
>> + return ret;
>> +
>> + ret = v4l2_async_register_subdev(&priv->sd);
>> + if (ret)
>> + goto remove;
>> +
>> + return 0;
>> +remove:
>> + ic_ops[priv->task_id]->remove(priv);
>> + return ret;
> if (ret)
> ic_ops[priv->task_id]->remove(priv);
>
> return ret;
>
> as an alternative.
done.
>
>
> +
> +static struct platform_driver imx_ic_driver = {
> + .probe = imx_ic_probe,
> + .remove = imx_ic_remove,
> + .id_table = imx_ic_ids,
> + .driver = {
> + .name = "imx-ipuv3-ic",
> + .owner = THIS_MODULE,
> Please drop .owner assignment.
done.
>> diff --git a/drivers/staging/media/imx/imx-ic-pp.c b/drivers/staging/media/imx/imx-ic-pp.c
>> new file mode 100644
>> index 0000000..5ef0581
>> --- /dev/null
>> +++ b/drivers/staging/media/imx/imx-ic-pp.c
>> @@ -0,0 +1,636 @@
>> +/*
>> + * V4L2 IC Post-Processor Subdev for Freescale i.MX5/6 SOC
>> + *
>> + * Copyright (c) 2014-2016 Mentor Graphics Inc.
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation; either version 2 of the License, or
>> + * (at your option) any later version.
>> + */
>> +#include <linux/module.h>
>> +#include <linux/delay.h>
>> +#include <linux/fs.h>
>> +#include <linux/timer.h>
>> +#include <linux/sched.h>
>> +#include <linux/slab.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/pinctrl/consumer.h>
>> +#include <media/v4l2-device.h>
>> +#include <media/v4l2-ioctl.h>
>> +#include <media/videobuf2-dma-contig.h>
>> +#include <media/v4l2-subdev.h>
>> +#include <media/v4l2-of.h>
>> +#include <media/v4l2-ctrls.h>
> Please sort the list of headers alphabetically.
done.
>
>>
>> +
>> + priv->out_run = kzalloc(IMX_MEDIA_MAX_RING_BUFS *
>> + sizeof(*priv->out_run), GFP_KERNEL);
>> + if (!priv->out_run) {
>> + v4l2_err(&ic_priv->sd, "failed to alloc src ring runs\n");
> In OOM situation the core will report it, probably you can drop the message.
with a backtrace? I can't remember, if not I'd like to keep this.
>> diff --git a/drivers/staging/media/imx/imx-ic-prpenc.c b/drivers/staging/media/imx/imx-ic-prpenc.c
>> new file mode 100644
>> index 0000000..e17216b
>> --- /dev/null
>> +++ b/drivers/staging/media/imx/imx-ic-prpenc.c
>> @@ -0,0 +1,1037 @@
>> +/*
>> + * V4L2 Capture IC Encoder Subdev for Freescale i.MX5/6 SOC
>> + *
>> + * This subdevice handles capture of video frames from the CSI, which
>> + * are routed directly to the Image Converter preprocess encode task,
>> + * for resizing, colorspace conversion, and rotation.
>> + *
>> + * Copyright (c) 2012-2016 Mentor Graphics Inc.
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation; either version 2 of the License, or
>> + * (at your option) any later version.
>> + */
>> +#include <linux/module.h>
>> +#include <linux/delay.h>
>> +#include <linux/fs.h>
>> +#include <linux/timer.h>
>> +#include <linux/sched.h>
>> +#include <linux/slab.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/spinlock.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/pinctrl/consumer.h>
>> +#include <media/v4l2-device.h>
>> +#include <media/v4l2-ioctl.h>
>> +#include <media/videobuf2-dma-contig.h>
>> +#include <media/v4l2-subdev.h>
>> +#include <media/v4l2-of.h>
>> +#include <media/v4l2-ctrls.h>
> Please sort the list of headers alphabetically.
done.
>
>> +static irqreturn_t prpenc_eof_interrupt(int irq, void *dev_id)
>> +{
>> + struct prpenc_priv *priv = dev_id;
>> + struct imx_media_dma_buf *done, *next;
>> + struct ipuv3_channel *channel;
>> + unsigned long flags;
>> +
>> + spin_lock_irqsave(&priv->irqlock, flags);
> Here spin_lock(&priv->irqlock) should be sufficient.
done.
>> +
>> + ret = media_entity_pads_init(&sd->entity, PRPENC_NUM_PADS, priv->pad);
>> + if (ret)
>> + goto free_ctrls;
>> +
>> + return 0;
>> +free_ctrls:
>> + v4l2_ctrl_handler_free(&priv->ctrl_hdlr);
>> + return ret;
> if (ret)
> v4l2_ctrl_handler_free(&priv->ctrl_hdlr);
>
> return ret;
>
> version is shorter.
done.
>
> diff --git a/drivers/staging/media/imx/imx-ic-prpvf.c
> b/drivers/staging/media/imx/imx-ic-prpvf.c
>> new file mode 100644
>> index 0000000..53ce006
>> --- /dev/null
>> +++ b/drivers/staging/media/imx/imx-ic-prpvf.c
>> @@ -0,0 +1,1180 @@
>> +/*
>> + * V4L2 IC Deinterlacer Subdev for Freescale i.MX5/6 SOC
>> + *
>> + * Copyright (c) 2014-2016 Mentor Graphics Inc.
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation; either version 2 of the License, or
>> + * (at your option) any later version.
>> + */
>> +#include <linux/module.h>
>> +#include <linux/delay.h>
>> +#include <linux/fs.h>
>> +#include <linux/timer.h>
>> +#include <linux/sched.h>
>> +#include <linux/slab.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/pinctrl/consumer.h>
>> +#include <media/v4l2-device.h>
>> +#include <media/v4l2-ioctl.h>
>> +#include <media/videobuf2-dma-contig.h>
>> +#include <media/v4l2-subdev.h>
>> +#include <media/v4l2-of.h>
>> +#include <media/v4l2-ctrls.h>
> Please sort the list of headers alphabetically.
done.
>
>> +/* prpvf_out_ch EOF interrupt (progressive frame ready) */
>> +static irqreturn_t prpvf_out_eof_interrupt(int irq, void *dev_id)
>> +{
>> + struct prpvf_priv *priv = dev_id;
>> + struct imx_media_dma_buf *done;
>> + unsigned long flags;
>> +
>> + spin_lock_irqsave(&priv->irqlock, flags);
>
> Here spin_lock(&priv->irqlock) should be sufficient.
done.
>
>> diff --git a/drivers/staging/media/imx/imx-ic.h b/drivers/staging/media/imx/imx-ic.h
>> new file mode 100644
>> index 0000000..9aed5f5
>> --- /dev/null
>> +++ b/drivers/staging/media/imx/imx-ic.h
>> @@ -0,0 +1,36 @@
>> +/*
>> + * V4L2 Image Converter Subdev for Freescale i.MX5/6 SOC
>> + *
>> + * Copyright (c) 2016 Mentor Graphics Inc.
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation; either version 2 of the License, or
>> + * (at your option) any later version.
>> + */
>> +#ifndef _IMX_IC_H
>> +#define _IMX_IC_H
>> +
> Please add header files or declarations of all used structs.
done (it only needs v4l2-subdev.h).
Steve
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web