[PATCH v5 2/7] dt-bindings: media: max9286: Define 'maxim,gpio-poc'
Define a new vendor property in the maxim,max9286 binding schema.
The new property allows to declare that the remote camera
power-over-coax is controlled by one of the MAX9286 gpio lines.
As it is currently not possible to establish a regulator as consumer
of the MAX9286 gpio controller for this purpose, the property allows to
declare that the camera power is controlled by the MAX9286 directly.
The property accepts a gpio-index (0 or 1) and one line polarity
flag as defined by dt-bindings/gpio/gpio.h.
Reviewed-by: Rob Herring
Reviewed-by: Laurent Pinchart
Signed-off-by: Jacopo Mondi
---
.../bindings/media/i2c/maxim,max9286.yaml | 67 ++-
1 file changed, 66 insertions(+), 1 deletion(-)
diff --git a/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
b/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
index 0e7162998b77..bf93fa73ce41 100644
--- a/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
+++ b/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
@@ -70,6 +70,28 @@ properties:
a remote serializer whose high-threshold noise immunity is not enabled
is 10 micro volts
+ maxim,gpio-poc:
+$ref: '/schemas/types.yaml#/definitions/uint32-array'
+minItems: 2
+maxItems: 2
+description: |
+ Index of the MAX9286 gpio output line (0 or 1) that controls Power over
+ Coax to the cameras and its associated polarity flag.
+
+ The property accepts an array of two unsigned integers, the first being
+ the gpio line index (0 or 1) and the second being the gpio line polarity
+ flag (GPIO_ACTIVE_HIGH or GPIO_ACTIVE_LOW) as defined in
+ .
+
+ When the remote cameras power is controlled by one of the MAX9286 gpio
+ lines, this property has to be used to specify which line among the two
+ available ones controls the remote camera power enablement.
+
+ When this property is used it is not possible to register a gpio
+ controller as the gpio lines are controlled directly by the MAX9286 and
+ not available for consumers, nor the 'poc-supply' property should be
+ specified.
+
ports:
$ref: /schemas/graph.yaml#/properties/ports
@@ -182,7 +204,16 @@ required:
- reg
- ports
- i2c-mux
- - gpio-controller
+
+# If 'maxim,gpio-poc' is present, then 'poc-supply' and 'gpio-controller'
+# are not allowed.
+if:
+ required:
+- maxim,gpio-poc
+then:
+ properties:
+poc-supply: false
+gpio-controller: false
additionalProperties: false
@@ -327,4 +358,38 @@ examples:
};
};
};
+
+/*
+* Example of a deserializer that controls the camera Power over Coax
+* through one of its gpio lines.
+*/
+gmsl-deserializer@6c {
+compatible = "maxim,max9286";
+reg = <0x6c>;
+enable-gpios = < 14 GPIO_ACTIVE_HIGH>;
+
+/*
+* The remote camera power is controlled by MAX9286 GPIO line #0.
+* No 'poc-supply' nor 'gpio-controller' are specified.
+*/
+maxim,gpio-poc = <0 GPIO_ACTIVE_LOW>;
+
+/*
+* Do not describe connections as they're the same as in the
previous
+* example.
+*/
+ports {
+#address-cells = <1>;
+#size-cells = <0>;
+
+port@4 {
+reg = <4>;
+};
+};
+
+i2c-mux {
+#address-cells = <1>;
+#size-cells = <0>;
+};
+};
};
--
2.31.1
[PATCH v5 6/7] arm64: dts: renesas: eagle: Add GMSL .dtsi
From: Kieran Bingham
Describe the FAKRA connector available on Eagle board that allows
connecting GMSL camera modules such as IMI RDACM20 and RDACM21.
Signed-off-by: Kieran Bingham
Signed-off-by: Jacopo Mondi
---
arch/arm64/boot/dts/renesas/eagle-gmsl.dtsi | 178
1 file changed, 178 insertions(+)
create mode 100644 arch/arm64/boot/dts/renesas/eagle-gmsl.dtsi
diff --git a/arch/arm64/boot/dts/renesas/eagle-gmsl.dtsi
b/arch/arm64/boot/dts/renesas/eagle-gmsl.dtsi
new file mode 100644
index ..d2e48dc3e820
--- /dev/null
+++ b/arch/arm64/boot/dts/renesas/eagle-gmsl.dtsi
@@ -0,0 +1,178 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Device Tree Source (overlay) for the Eagle V3M GMSL connectors
+ *
+ * Copyright (C) 2017 Ideas on Board
+ * Copyright (C) 2021 Jacopo Mondi
+ *
+ * This overlay allows you to define GMSL cameras connected to the FAKRA
+ * connectors on the Eagle-V3M (or compatible) board.
+ *
+ * The following cameras are currently supported: RDACM20 and RDACM21.
+ *
+ * The board .dts files that include this select which cameras are in use
+ * by specifying the camera model with:
+ *
+ * #define GMSL_CAMERA_RDACM20
+ * or
+ * #define GMSL_CAMERA_RDACM21
+ *
+ * And which cameras are connected to the board by defining:
+ * #define GMSL_CAMERA_0
+ * #define GMSL_CAMERA_1
+ * #define GMSL_CAMERA_2
+ * #define GMSL_CAMERA_3
+ */
+
+#include
+
+/* Validate the board file settings. */
+#if !defined(GMSL_CAMERA_RDACM20) && !defined(GMSL_CAMERA_RDACM21)
+#error "Camera model should be defined by the board file"
+#endif
+
+#if defined(GMSL_CAMERA_RDACM20) && defined(GMSL_CAMERA_RDACM21)
+#error "A single camera model should be selected"
+#endif
+
+#if !defined(GMSL_CAMERA_0) && !defined(GMSL_CAMERA_1) && \
+!defined(GMSL_CAMERA_2) && !defined(GMSL_CAMERA_3)
+#error "At least one camera should be selected"
+#endif
+
+#if defined(GMSL_CAMERA_RDACM20)
+#define GMSL_CAMERA_MODEL "imi,rdacm20"
+#elif defined(GMSL_CAMERA_RDACM21)
+#define GMSL_CAMERA_MODEL "imi,rdacm21"
+#endif
+
+ {
+ status = "okay";
+};
+
+ {
+ status = "okay";
+};
+
+ {
+ status = "okay";
+};
+
+ {
+ status = "okay";
+};
+
+ {
+ status = "okay";
+
+#if defined(GMSL_CAMERA_RDACM21)
+ maxim,reverse-channel-microvolt = <10>;
+#endif
+
+ ports {
+#ifdef GMSL_CAMERA_0
+ port@0 {
+ max9286_in0: endpoint {
+ remote-endpoint = <_con0>;
+ };
+ };
+#endif
+
+#ifdef GMSL_CAMERA_1
+ port@1 {
+ max9286_in1: endpoint{
+ remote-endpoint = <_con1>;
+ };
+
+ };
+#endif
+
+#ifdef GMSL_CAMERA_2
+ port@2 {
+ max9286_in2: endpoint {
+ remote-endpoint = <_con2>;
+ };
+
+ };
+#endif
+
+#ifdef GMSL_CAMERA_3
+ port@3 {
+ max9286_in3: endpoint {
+ remote-endpoint = <_con3>;
+ };
+
+ };
+#endif
+ };
+
+ i2c-mux {
+#ifdef GMSL_CAMERA_0
+ i2c@0 {
+ status = "okay";
+
+ camera@51 {
+ compatible = GMSL_CAMERA_MODEL;
+ reg = <0x51>, <0x61>;
+
+ port {
+ fakra_con0: endpoint {
+ remote-endpoint =
<_in0>;
+ };
+ };
+ };
+ };
+#endif
+
+#ifdef GMSL_CAMERA_1
+ i2c@1 {
+ status = "okay";
+
+ camera@52 {
+ compatible = GMSL_CAMERA_MODEL;
+ reg = <0x52>, <0x62>;
+
+ port {
+ fakra_con1: endpoint {
+ remote-endpoint =
<_in1>;
+ };
+ };
+ };
+ };
+#endif
+
+#ifdef GMSL_CAMERA_2
+ i2c@2 {
+ status = "okay";
+
+ camera@53 {
+ compatible = GMSL_CAMERA_MODEL;
+ reg = <0x53>, <0x63>;
+
+ port {
+ fakra_con2: endpoint {
+
[PATCH v5 7/7] DNI: arm64: dts: renesas: eagle: Include eagle-gmsl
From: Kieran Bingham
Include the eagle-gmsl.dtsi to enable GMSL camera support on the
Eagle-V3M platform.
Signed-off-by: Kieran Bingham
Signed-off-by: Jacopo Mondi
---
arch/arm64/boot/dts/renesas/r8a77970-eagle.dts | 8
1 file changed, 8 insertions(+)
diff --git a/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts
b/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts
index ab35202857f5..a87d4b7f17c7 100644
--- a/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts
+++ b/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts
@@ -378,3 +378,11 @@ {
status = "okay";
};
+
+/* FAKRA Overlay */
+#define GMSL_CAMERA_RDACM21
+#define GMSL_CAMERA_0
+#define GMSL_CAMERA_1
+#define GMSL_CAMERA_2
+#define GMSL_CAMERA_3
+#include "eagle-gmsl.dtsi"
--
2.31.1
[PATCH v5 5/7] arm64: dts: renesas: eagle: Enable MAX9286
From: Kieran Bingham
Enable the MAX9286 GMSL deserializer on the Eagle-V3M board.
Connected cameras should be defined in a device-tree overlay or included
after these definitions.
Reviewed-by: Laurent Pinchart
Signed-off-by: Kieran Bingham
Signed-off-by: Jacopo Mondi
---
.../arm64/boot/dts/renesas/r8a77970-eagle.dts | 106 ++
1 file changed, 106 insertions(+)
diff --git a/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts
b/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts
index 874a7fc2730b..ab35202857f5 100644
--- a/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts
+++ b/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts
@@ -6,6 +6,8 @@
* Copyright (C) 2017 Cogent Embedded, Inc.
*/
+#include
+
/dts-v1/;
#include "r8a77970.dtsi"
@@ -188,6 +190,11 @@ i2c0_pins: i2c0 {
function = "i2c0";
};
+ i2c3_pins: i2c3 {
+ groups = "i2c3_a";
+ function = "i2c3";
+ };
+
qspi0_pins: qspi0 {
groups = "qspi0_ctrl", "qspi0_data4";
function = "qspi0";
@@ -266,6 +273,105 @@ {
status = "okay";
};
+ {
+ status = "okay";
+
+ ports {
+ port@0 {
+ csi40_in: endpoint {
+ clock-lanes = <0>;
+ data-lanes = <1 2 3 4>;
+ remote-endpoint = <_out0>;
+ };
+ };
+ };
+};
+
+ {
+ pinctrl-0 = <_pins>;
+ pinctrl-names = "default";
+
+ status = "okay";
+ clock-frequency = <40>;
+
+ gmsl: gmsl-deserializer@48 {
+ compatible = "maxim,max9286";
+ reg = <0x48>;
+
+ maxim,gpio-poc = <0 GPIO_ACTIVE_LOW>;
+
+ /* eagle-pca9654-max9286-pwdn */
+ enable-gpios = <_expander 0 GPIO_ACTIVE_HIGH>;
+
+ ports {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ port@0 {
+ reg = <0>;
+ };
+
+ port@1 {
+ reg = <1>;
+ };
+
+ port@2 {
+ reg = <2>;
+ };
+
+ port@3 {
+ reg = <3>;
+ };
+
+ port@4 {
+ reg = <4>;
+ max9286_out0: endpoint {
+ clock-lanes = <0>;
+ data-lanes = <1 2 3 4>;
+ remote-endpoint = <_in>;
+ };
+ };
+ };
+
+ i2c-mux {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ i2c@0 {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ reg = <0>;
+
+ status = "disabled";
+ };
+
+ i2c@1 {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ reg = <1>;
+
+ status = "disabled";
+ };
+
+ i2c@2 {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ reg = <2>;
+
+ status = "disabled";
+ };
+
+ i2c@3 {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ reg = <3>;
+
+ status = "disabled";
+ };
+ };
+ };
+};
+
{
pinctrl-0 = <_pins>;
pinctrl-names = "default";
--
2.31.1
[PATCH v5 4/7] arm64: dts: renesas: r8a77970: Add csi40 port@0
Declare port@0 in the csi40 device node and leave it un-connected.
Each board .dts file will connect the port as it requires.
Reviewed-by: Laurent Pinchart
Signed-off-by: Jacopo Mondi
---
arch/arm64/boot/dts/renesas/r8a77970.dtsi | 4
1 file changed, 4 insertions(+)
diff --git a/arch/arm64/boot/dts/renesas/r8a77970.dtsi
b/arch/arm64/boot/dts/renesas/r8a77970.dtsi
index 5a5d5649332a..e8f6352c3665 100644
--- a/arch/arm64/boot/dts/renesas/r8a77970.dtsi
+++ b/arch/arm64/boot/dts/renesas/r8a77970.dtsi
@@ -1106,6 +1106,10 @@ ports {
#address-cells = <1>;
#size-cells = <0>;
+ port@0 {
+ reg = <0>;
+ };
+
port@1 {
#address-cells = <1>;
#size-cells = <0>;
--
2.31.1
[PATCH v5 3/7] media: i2c: max9286: Use "maxim,gpio-poc" property
The 'maxim,gpio-poc' property is used when the remote camera
power-over-coax is controlled by one of the MAX9286 gpio lines,
to instruct the driver about which line to use and what the line
polarity is.
Add to the max9286 driver support for parsing the newly introduced
property and use it if available in place of the usual supply, as it is
not possible to establish one as consumer of the max9286 gpio
controller.
If the new property is present, no gpio controller is registered and
'poc-supply' is ignored.
In order to maximize code re-use, break out the max9286 gpio handling
function so that they can be used by the gpio controller through the
gpio-consumer API, or directly by the driver code.
Wrap the power up and power down routines to their own function to
be able to use either the gpio line directly or the supply. This will
make it easier to control the remote camera power at run time.
Signed-off-by: Jacopo Mondi
---
drivers/media/i2c/max9286.c | 125 +++-
1 file changed, 94 insertions(+), 31 deletions(-)
diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
index 6fd4d59fcc72..99160aa68a5f 100644
--- a/drivers/media/i2c/max9286.c
+++ b/drivers/media/i2c/max9286.c
@@ -15,6 +15,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -165,6 +166,9 @@ struct max9286_priv {
u32 reverse_channel_mv;
+ u32 gpio_poc;
+ u32 gpio_poc_flags;
+
struct v4l2_ctrl_handler ctrls;
struct v4l2_ctrl *pixelrate;
@@ -1022,20 +1026,27 @@ static int max9286_setup(struct max9286_priv *priv)
return 0;
}
-static void max9286_gpio_set(struct gpio_chip *chip,
-unsigned int offset, int value)
+static int max9286_gpio_set(struct max9286_priv *priv, unsigned int offset,
+ int value)
{
- struct max9286_priv *priv = gpiochip_get_data(chip);
-
if (value)
priv->gpio_state |= BIT(offset);
else
priv->gpio_state &= ~BIT(offset);
- max9286_write(priv, 0x0f, MAX9286_0X0F_RESERVED | priv->gpio_state);
+ return max9286_write(priv, 0x0f,
+MAX9286_0X0F_RESERVED | priv->gpio_state);
+}
+
+static void max9286_gpiochip_set(struct gpio_chip *chip,
+unsigned int offset, int value)
+{
+ struct max9286_priv *priv = gpiochip_get_data(chip);
+
+ max9286_gpio_set(priv, offset, value);
}
-static int max9286_gpio_get(struct gpio_chip *chip, unsigned int offset)
+static int max9286_gpiochip_get(struct gpio_chip *chip, unsigned int offset)
{
struct max9286_priv *priv = gpiochip_get_data(chip);
@@ -1055,16 +1066,81 @@ static int max9286_register_gpio(struct max9286_priv
*priv)
gpio->of_node = dev->of_node;
gpio->ngpio = 2;
gpio->base = -1;
- gpio->set = max9286_gpio_set;
- gpio->get = max9286_gpio_get;
+ gpio->set = max9286_gpiochip_set;
+ gpio->get = max9286_gpiochip_get;
gpio->can_sleep = true;
+ ret = devm_gpiochip_add_data(dev, gpio, priv);
+ if (ret)
+ dev_err(dev, "Unable to create gpio_chip\n");
+
+ return ret;
+}
+
+static int max9286_parse_gpios(struct max9286_priv *priv)
+{
+ struct device *dev = >client->dev;
+ u32 gpio_poc[2];
+ int ret;
+
/* GPIO values default to high */
priv->gpio_state = BIT(0) | BIT(1);
- ret = devm_gpiochip_add_data(dev, gpio, priv);
+ /*
+* Parse the "gpio-poc" vendor property. If the camera power is
+* controlled by one of the MAX9286 gpio lines, do not register
+* the gpio controller and ignore 'poc-supply'.
+*/
+ ret = of_property_read_u32_array(dev->of_node,
+"maxim,gpio-poc", gpio_poc, 2);
+ if (!ret) {
+ priv->gpio_poc = gpio_poc[0];
+ priv->gpio_poc_flags = gpio_poc[1];
+ if (priv->gpio_poc > 1 ||
+ (priv->gpio_poc_flags != GPIO_ACTIVE_HIGH &&
+priv->gpio_poc_flags != GPIO_ACTIVE_LOW)) {
+ dev_err(dev, "Invalid 'gpio-poc': (%u %u)\n",
+ priv->gpio_poc, priv->gpio_poc_flags);
+ return -EINVAL;
+ }
+
+ return 0;
+ }
+
+ ret = max9286_register_gpio(priv);
if (ret)
- dev_err(dev, "Unable to create gpio_chip\n");
+ return ret;
+
+ priv->regulator = devm_regulator_get(dev, "poc");
+ if (IS_ERR(priv->regulator)) {
+ if (PTR_ERR(priv->regulator) != -EPROBE_DEFER)
+ dev_err(dev, "Unable to get PoC regulator (%ld)\n",
+
[PATCH v5 1/7] dt-bindings: media: max9286: Re-indent example
The dt-bindings examples are usually indented with 4 spaces.
The maxim,max9286 schema has the example indented with only
2 spaces, re-indent it.
Cosmetic change only.
Reviewed-by: Laurent Pinchart
Acked-by: Rob Herring
Signed-off-by: Jacopo Mondi
---
.../bindings/media/i2c/maxim,max9286.yaml | 214 +-
1 file changed, 107 insertions(+), 107 deletions(-)
diff --git a/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
b/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
index ee16102fdfe7..0e7162998b77 100644
--- a/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
+++ b/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
@@ -191,140 +191,140 @@ examples:
#include
i2c@e66d8000 {
- #address-cells = <1>;
- #size-cells = <0>;
+#address-cells = <1>;
+#size-cells = <0>;
- reg = <0 0xe66d8000>;
+reg = <0 0xe66d8000>;
- gmsl-deserializer@2c {
-compatible = "maxim,max9286";
-reg = <0x2c>;
-poc-supply = <_poc_12v>;
-enable-gpios = < 13 GPIO_ACTIVE_HIGH>;
+gmsl-deserializer@2c {
+compatible = "maxim,max9286";
+reg = <0x2c>;
+poc-supply = <_poc_12v>;
+enable-gpios = < 13 GPIO_ACTIVE_HIGH>;
-gpio-controller;
-#gpio-cells = <2>;
+gpio-controller;
+#gpio-cells = <2>;
-maxim,reverse-channel-microvolt = <17>;
+maxim,reverse-channel-microvolt = <17>;
-ports {
- #address-cells = <1>;
- #size-cells = <0>;
+ports {
+#address-cells = <1>;
+#size-cells = <0>;
- port@0 {
-reg = <0>;
+port@0 {
+reg = <0>;
-max9286_in0: endpoint {
- remote-endpoint = <_out0>;
-};
- };
-
- port@1 {
-reg = <1>;
-
-max9286_in1: endpoint {
- remote-endpoint = <_out1>;
-};
- };
-
- port@2 {
-reg = <2>;
-
-max9286_in2: endpoint {
- remote-endpoint = <_out2>;
-};
- };
+max9286_in0: endpoint {
+remote-endpoint = <_out0>;
+};
+};
- port@3 {
-reg = <3>;
+port@1 {
+reg = <1>;
-max9286_in3: endpoint {
- remote-endpoint = <_out3>;
-};
- };
+max9286_in1: endpoint {
+remote-endpoint = <_out1>;
+};
+};
- port@4 {
-reg = <4>;
+port@2 {
+reg = <2>;
-max9286_out: endpoint {
- data-lanes = <1 2 3 4>;
- remote-endpoint = <_in>;
-};
- };
-};
+max9286_in2: endpoint {
+remote-endpoint = <_out2>;
+};
+};
-i2c-mux {
- #address-cells = <1>;
- #size-cells = <0>;
+port@3 {
+reg = <3>;
- i2c@0 {
-#address-cells = <1>;
-#size-cells = <0>;
-reg = <0>;
+max9286_in3: endpoint {
+remote-endpoint = <_out3>;
+};
+};
-camera@51 {
- compatible = "imi,rdacm20";
- reg = <0x51>, <0x61>;
+port@4 {
+reg = <4>;
- port {
-rdacm20_out0: endpoint {
- remote-endpoint = <_in0>;
+max9286_out: endpoint {
+data-lanes = <1 2 3 4>;
+remote-endpoint = <_in>;
+};
};
- };
-
};
- };
-
- i2c@1 {
-#address-cells = <1>;
-#size-cells = <0>;
-reg = <1>;
-camera@52 {
- compatible = "imi,rdacm20";
- reg = <0x52>, <0x62>;
+i2c-mux {
+#address-cells = <1>;
+#size-cells = <0>;
- port {
-rdacm20_out1: endpoint {
- remote-endpoint = <_in1>;
-};
- };
-};
- };
+i2c@
[PATCH v5 0/7] arm64: dts: renesas: Enable GMSL on R8A77970 V3M Eagle
Hello, small changes compared to v4: - bindings: - Took in Rob's suggestions and used a more compact if: required: - maxim,gpio-poc then: properties: poc-supply: false gpio-controller: false it's a shame we can't use 'properties' in the if: clause :) but this already looks better than the preceding version - driver: - Backtrack on the set_gpio() function and share implementation with the gpiochip function Tested on Eagle-V3M Thanks j Jacopo Mondi (4): dt-bindings: media: max9286: Re-indent example dt-bindings: media: max9286: Define 'maxim,gpio-poc' media: i2c: max9286: Use "maxim,gpio-poc" property arm64: dts: renesas: r8a77970: Add csi40 port@0 Kieran Bingham (3): arm64: dts: renesas: eagle: Enable MAX9286 arm64: dts: renesas: eagle: Add GMSL .dtsi DNI: arm64: dts: renesas: eagle: Include eagle-gmsl .../bindings/media/i2c/maxim,max9286.yaml | 275 +++--- arch/arm64/boot/dts/renesas/eagle-gmsl.dtsi | 178 .../arm64/boot/dts/renesas/r8a77970-eagle.dts | 114 arch/arm64/boot/dts/renesas/r8a77970.dtsi | 4 + drivers/media/i2c/max9286.c | 125 ++-- 5 files changed, 560 insertions(+), 136 deletions(-) create mode 100644 arch/arm64/boot/dts/renesas/eagle-gmsl.dtsi -- 2.31.1
Re: [PATCH v4 3/7] media: i2c: max9286: Use "maxim,gpio-poc" property
Hi Laurent,
On Thu, Apr 15, 2021 at 10:19:30PM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Thu, Apr 15, 2021 at 02:25:58PM +0200, Jacopo Mondi wrote:
> > The 'maxim,gpio-poc' property is used when the remote camera
> > power-over-coax is controlled by one of the MAX9286 gpio lines,
> > to instruct the driver about which line to use and what the line
> > polarity is.
> >
> > Add to the max9286 driver support for parsing the newly introduced
> > property and use it if available in place of the usual supply, as it is
> > not possible to establish one as consumer of the max9286 gpio
> > controller.
> >
> > If the new property is present, no gpio controller is registered and
> > 'poc-supply' is ignored.
> >
> > In order to maximize code re-use, break out the max9286 gpio handling
> > function so that they can be used by the gpio controller through the
> > gpio-consumer API, or directly by the driver code.
> >
> > Wrap the power up and power down routines to their own function to
> > be able to use either the gpio line directly or the supply. This will
> > make it easier to control the remote camera power at run time.
> >
> > Signed-off-by: Jacopo Mondi
> > ---
> > drivers/media/i2c/max9286.c | 119
> > 1 file changed, 93 insertions(+), 26 deletions(-)
> >
> > diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> > index 6fd4d59fcc72..a8b37783d2ef 100644
> > --- a/drivers/media/i2c/max9286.c
> > +++ b/drivers/media/i2c/max9286.c
> > @@ -15,6 +15,7 @@
> > #include
> > #include
> > #include
> > +#include
> > #include
> > #include
> > #include
> > @@ -165,6 +166,9 @@ struct max9286_priv {
> >
> > u32 reverse_channel_mv;
> >
> > + u32 gpio_poc;
> > + u32 gpio_poc_flags;
> > +
> > struct v4l2_ctrl_handler ctrls;
> > struct v4l2_ctrl *pixelrate;
> >
> > @@ -1022,8 +1026,19 @@ static int max9286_setup(struct max9286_priv *priv)
> > return 0;
> > }
> >
> > -static void max9286_gpio_set(struct gpio_chip *chip,
> > -unsigned int offset, int value)
> > +static int max9286_gpio_set(struct max9286_priv *priv, unsigned int offset,
> > + int value)
> > +{
> > + int ret = max9286_read(priv, 0x0f);
> > + if (ret < 0)
> > + return ret;
> > +
> > + ret = value ? (ret & 0x3) | BIT(offset) : (ret & 0x3) & ~BIT(offset);
> > + return max9286_write(priv, 0x0f, MAX9286_0X0F_RESERVED | ret);
> > +}
>
> Was there anything wrong with v3, sharing the implementation between the
Yes, my understanding that your comment about not mixing the two
cases as they're mutually exclusive :)
I found that it actually made sense, as my reasoning was to support
both at the same time, but it was actually not possible. I then
thought it was a good idea to keep them separate
> gpio-controller and non gpio-controller cases ? In particular, caching
> the state locally seems better, max9286_read() goes over I2C and is thus
> slow.
At the expense of an additional i2c read... The gpio is only toggled
at power-up/down, so I considered this acceptable...
Both solutions are ok with me, I can restore the usage of gpio->state
or keep what I have here...
The only argument in favour of using gpio->state is that it paves the
way for the usage of 'gpio-poc' and for registering a gpio-controller
on the other gpio line at the same time, in future.. I think I'll
switch back to that solution then...
Thanks
j
>
> > +
> > +static void max9286_gpiochip_set(struct gpio_chip *chip,
> > +unsigned int offset, int value)
> > {
> > struct max9286_priv *priv = gpiochip_get_data(chip);
> >
> > @@ -1035,7 +1050,7 @@ static void max9286_gpio_set(struct gpio_chip *chip,
> > max9286_write(priv, 0x0f, MAX9286_0X0F_RESERVED | priv->gpio_state);
> > }
> >
> > -static int max9286_gpio_get(struct gpio_chip *chip, unsigned int offset)
> > +static int max9286_gpiochip_get(struct gpio_chip *chip, unsigned int
> > offset)
> > {
> > struct max9286_priv *priv = gpiochip_get_data(chip);
> >
> > @@ -1055,8 +1070,8 @@ static int max9286_register_gpio(struct max9286_priv
> > *priv)
> > gpio->of_node = dev->of_node;
> > gpio->ngpio = 2;
> > gpio->base = -1;
> > - gpio->set = max9286_gpio_set;
> > - gpio->get = max9286_gpio_get;
> > + gpio->
Re: [PATCH v3 2/5] media: i2c: max9286: Use "maxim,gpio-poc" property
Hi Laurent,
On Thu, Apr 15, 2021 at 10:14:05PM +0300, Laurent Pinchart wrote:
> > > > + /* GPIO values default to high */
> > > > + priv->gpio_state = BIT(0) | BIT(1);
> > >
> > > Why is that ?
> > >
> > As the set/get functions of gpiochip use the gpio_state and I wanted
> > to use the same functions for the internal gpio handling I used
> > gpio_state in gpio_set(). My thinking was that in this way altering
> > the gpio line would be visibile to gpio consumers... which we don't
> > have as I won't register the gpio-controller :)
>
> My question was why they default to high here, when they default to low
> when there's a gpio-controller property.
>
Oh, got it now... the two output lines are high by default :)
Why do you say "they default to low when there's a gpio-controller
property" ? When does that requirement come from ?
Thanks
j
> > > > + priv->regulator = NULL;
> > >
> > > As priv is initialized to 0, you can skip this.
> >
> > Yes, I liked it explicit as it is used as flag, but it is not
> > required...
> >
> > > > +
> > > > + return 0;
> > > > + }
> > > > +
> > > > + ret = max9286_register_gpio(priv);
> > > > + if (ret)
> > > > + return ret;
> > > > +
> > > > + priv->regulator = devm_regulator_get(dev, "poc");
> > > > + if (IS_ERR(priv->regulator)) {
> > > > + if (PTR_ERR(priv->regulator) != -EPROBE_DEFER)
> > > > + dev_err(dev, "Unable to get PoC regulator
> > > > (%ld)\n",
> > > > + PTR_ERR(priv->regulator));
> > > > + return PTR_ERR(priv->regulator);
> > > > + }
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > > > +static int max9286_poc_enable(struct max9286_priv *priv, bool enable)
> > > > +{
> > > > + int ret;
> > > > +
> > > > + /* If "poc-gpio" is used, toggle the line and do not use
> > > > regulator. */
> > > > + if (!priv->regulator)
> > > > + return max9286_gpio_set(priv, priv->gpio_poc,
> > > > + enable ^ priv->gpio_poc_flags);
> > > > +
> > > > + /* Otherwise PoC is controlled using a regulator. */
> > > > + if (enable) {
> > > > + ret = regulator_enable(priv->regulator);
> > > > + if (ret < 0) {
> > > > + dev_err(>client->dev, "Unable to turn PoC
> > > > on\n");
> > >
> > > As error message when max9286_gpio_set() fails (at least in the enable
> > > case) would be good too. Bonus points if there's a single dev_err()
> > > call.
> >
> > I'll see how it looks like
> >
> > > > + return ret;
> > > > + }
> > > > +
> > > > + return 0;
> > > > + }
> > > > +
> > > > + return regulator_disable(priv->regulator);
> > > > +}
> > > > +
> > > > static int max9286_init(struct device *dev)
> > > > {
> > > > struct max9286_priv *priv;
> > > > @@ -1078,17 +1158,14 @@ static int max9286_init(struct device *dev)
> > > > client = to_i2c_client(dev);
> > > > priv = i2c_get_clientdata(client);
> > > >
> > > > - /* Enable the bus power. */
> > > > - ret = regulator_enable(priv->regulator);
> > > > - if (ret < 0) {
> > > > - dev_err(>dev, "Unable to turn PoC on\n");
> > > > + ret = max9286_poc_enable(priv, true);
> > > > + if (ret)
> > > > return ret;
> > > > - }
> > > >
> > > > ret = max9286_setup(priv);
> > > > if (ret) {
> > > > dev_err(dev, "Unable to setup max9286\n");
> > > > - goto err_regulator;
> > > > + goto err_poc_disable;
> > > > }
> > > >
> > > > /*
> > > > @@ -1098,7 +1175,7 @@ static int max9286_init(struct device *dev)
> > > > ret = max9286_v4l2_register(priv);
> > > > if (ret) {
> > > > dev_err(dev, "Failed to register with V4L2\n");
> > > > - goto err_regulator;
> > > > + goto err_poc_disable;
> > > > }
> > > >
> > > > ret = max9286_i2c_mux_init(priv);
> > > > @@ -1114,8 +1191,8 @@ static int max9286_init(struct device *dev)
> > > >
> > > > err_v4l2_register:
> > > > max9286_v4l2_unregister(priv);
> > > > -err_regulator:
> > > > - regulator_disable(priv->regulator);
> > > > +err_poc_disable:
> > > > + max9286_poc_enable(priv, false);
> > > >
> > > > return ret;
> > > > }
> > > > @@ -1286,20 +1363,10 @@ static int max9286_probe(struct i2c_client
> > > > *client)
> > > > */
> > > > max9286_configure_i2c(priv, false);
> > > >
> > > > - ret = max9286_register_gpio(priv);
> > > > + ret = max9286_parse_gpios(priv);
> > > > if (ret)
> > > > goto err_powerdown;
> > > >
> > > > - priv->regulator = devm_regulator_get(>dev, "poc");
> > > > - if (IS_ERR(priv->regulator))
Re: [PATCH v4 16/17] media: v4l2-subdev: De-deprecate init() subdev op
Hi,
with feedback from media maintainers I can resend the series which is
now fully reviewed.
Thanks
j
On Mon, Apr 12, 2021 at 11:34:50AM +0200, Jacopo Mondi wrote:
> The init() subdev core operation is deemed to be deprecated for new
> subdevice drivers. However it could prove useful for complex
> architectures to defer operation that require access to the
> communication bus if said bus is not available (or fully configured)
> at the time when the subdevice probe() function is run.
>
> As an example, the GMSL architecture requires the GMSL configuration
> link to be configured on the host side after the remote subdevice
> has completed its probe function. After the configuration on the host
> side has been performed, the subdevice registers can be accessed through
> the communication bus.
>
> In particular:
>
> HOSTREMOTE
>
> probe()
> |
> -> |
> probe() {
>bus config()
> }
> |<|
> v4l2 async bound {
> bus config()
> call subdev init()
> |>|
>init() {
>access register on the bus()
> }
> |<---
> }
>
> In the GMSL use case the bus configuration requires the enablement of the
> noise immunity threshold on the remote side which ensures reliability
> of communications in electrically noisy environments. After the subdevice
> has enabled the threshold at the end of its probe() sequence the host
> side shall compensate it with an higher signal amplitude. Once this
> sequence has completed the bus can be accessed with noise protection
> enabled and all the operations that require a considerable number of
> transactions on the bus (such as the image sensor configuration
> sequence) are run in the subdevice init() operation implementation.
>
> Signed-off-by: Jacopo Mondi
> ---
> include/media/v4l2-subdev.h | 15 ---
> 1 file changed, 12 insertions(+), 3 deletions(-)
>
> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> index d0e9a5bdb08b..3068d9940669 100644
> --- a/include/media/v4l2-subdev.h
> +++ b/include/media/v4l2-subdev.h
> @@ -148,9 +148,18 @@ struct v4l2_subdev_io_pin_config {
> * each pin being configured. This function could be called at times
> * other than just subdevice initialization.
> *
> - * @init: initialize the sensor registers to some sort of reasonable default
> - * values. Do not use for new drivers and should be removed in existing
> - * drivers.
> + * @init: initialize the subdevice registers to some sort of reasonable
> default
> + * values. Do not use for new drivers (and should be removed in existing
> + * ones) for regular architectures where the image sensor is connected to
> + * the host receiver. For more complex architectures where the subdevice
> + * initialization should be deferred to the completion of the probe
> + * sequence of some intermediate component, or the communication bus
> + * requires configurations on the host side that depend on the completion
> + * of the probe sequence of the remote subdevices, the usage of this
> + * operation could be considered to allow the devices along the pipeline to
> + * probe and register in the media graph and to defer any operation that
> + * require actual access to the communication bus to their init() function
> + * implementation.
> *
> * @load_fw: load firmware.
> *
> --
> 2.31.1
>
[PATCH v4 7/7] DNI: arm64: dts: renesas: eagle: Include eagle-gmsl
From: Kieran Bingham
Include the eagle-gmsl.dtsi to enable GMSL camera support on the
Eagle-V3M platform.
Signed-off-by: Kieran Bingham
Signed-off-by: Jacopo Mondi
---
arch/arm64/boot/dts/renesas/r8a77970-eagle.dts | 8
1 file changed, 8 insertions(+)
diff --git a/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts
b/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts
index ab35202857f5..a87d4b7f17c7 100644
--- a/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts
+++ b/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts
@@ -378,3 +378,11 @@ {
status = "okay";
};
+
+/* FAKRA Overlay */
+#define GMSL_CAMERA_RDACM21
+#define GMSL_CAMERA_0
+#define GMSL_CAMERA_1
+#define GMSL_CAMERA_2
+#define GMSL_CAMERA_3
+#include "eagle-gmsl.dtsi"
--
2.31.1
[PATCH v4 6/7] arm64: dts: renesas: eagle: Add GMSL .dtsi
From: Kieran Bingham
Describe the FAKRA connector available on Eagle board that allows
connecting GMSL camera modules such as IMI RDACM20 and RDACM21.
Signed-off-by: Kieran Bingham
Signed-off-by: Jacopo Mondi
---
arch/arm64/boot/dts/renesas/eagle-gmsl.dtsi | 178
1 file changed, 178 insertions(+)
create mode 100644 arch/arm64/boot/dts/renesas/eagle-gmsl.dtsi
diff --git a/arch/arm64/boot/dts/renesas/eagle-gmsl.dtsi
b/arch/arm64/boot/dts/renesas/eagle-gmsl.dtsi
new file mode 100644
index ..d2e48dc3e820
--- /dev/null
+++ b/arch/arm64/boot/dts/renesas/eagle-gmsl.dtsi
@@ -0,0 +1,178 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Device Tree Source (overlay) for the Eagle V3M GMSL connectors
+ *
+ * Copyright (C) 2017 Ideas on Board
+ * Copyright (C) 2021 Jacopo Mondi
+ *
+ * This overlay allows you to define GMSL cameras connected to the FAKRA
+ * connectors on the Eagle-V3M (or compatible) board.
+ *
+ * The following cameras are currently supported: RDACM20 and RDACM21.
+ *
+ * The board .dts files that include this select which cameras are in use
+ * by specifying the camera model with:
+ *
+ * #define GMSL_CAMERA_RDACM20
+ * or
+ * #define GMSL_CAMERA_RDACM21
+ *
+ * And which cameras are connected to the board by defining:
+ * #define GMSL_CAMERA_0
+ * #define GMSL_CAMERA_1
+ * #define GMSL_CAMERA_2
+ * #define GMSL_CAMERA_3
+ */
+
+#include
+
+/* Validate the board file settings. */
+#if !defined(GMSL_CAMERA_RDACM20) && !defined(GMSL_CAMERA_RDACM21)
+#error "Camera model should be defined by the board file"
+#endif
+
+#if defined(GMSL_CAMERA_RDACM20) && defined(GMSL_CAMERA_RDACM21)
+#error "A single camera model should be selected"
+#endif
+
+#if !defined(GMSL_CAMERA_0) && !defined(GMSL_CAMERA_1) && \
+!defined(GMSL_CAMERA_2) && !defined(GMSL_CAMERA_3)
+#error "At least one camera should be selected"
+#endif
+
+#if defined(GMSL_CAMERA_RDACM20)
+#define GMSL_CAMERA_MODEL "imi,rdacm20"
+#elif defined(GMSL_CAMERA_RDACM21)
+#define GMSL_CAMERA_MODEL "imi,rdacm21"
+#endif
+
+ {
+ status = "okay";
+};
+
+ {
+ status = "okay";
+};
+
+ {
+ status = "okay";
+};
+
+ {
+ status = "okay";
+};
+
+ {
+ status = "okay";
+
+#if defined(GMSL_CAMERA_RDACM21)
+ maxim,reverse-channel-microvolt = <10>;
+#endif
+
+ ports {
+#ifdef GMSL_CAMERA_0
+ port@0 {
+ max9286_in0: endpoint {
+ remote-endpoint = <_con0>;
+ };
+ };
+#endif
+
+#ifdef GMSL_CAMERA_1
+ port@1 {
+ max9286_in1: endpoint{
+ remote-endpoint = <_con1>;
+ };
+
+ };
+#endif
+
+#ifdef GMSL_CAMERA_2
+ port@2 {
+ max9286_in2: endpoint {
+ remote-endpoint = <_con2>;
+ };
+
+ };
+#endif
+
+#ifdef GMSL_CAMERA_3
+ port@3 {
+ max9286_in3: endpoint {
+ remote-endpoint = <_con3>;
+ };
+
+ };
+#endif
+ };
+
+ i2c-mux {
+#ifdef GMSL_CAMERA_0
+ i2c@0 {
+ status = "okay";
+
+ camera@51 {
+ compatible = GMSL_CAMERA_MODEL;
+ reg = <0x51>, <0x61>;
+
+ port {
+ fakra_con0: endpoint {
+ remote-endpoint =
<_in0>;
+ };
+ };
+ };
+ };
+#endif
+
+#ifdef GMSL_CAMERA_1
+ i2c@1 {
+ status = "okay";
+
+ camera@52 {
+ compatible = GMSL_CAMERA_MODEL;
+ reg = <0x52>, <0x62>;
+
+ port {
+ fakra_con1: endpoint {
+ remote-endpoint =
<_in1>;
+ };
+ };
+ };
+ };
+#endif
+
+#ifdef GMSL_CAMERA_2
+ i2c@2 {
+ status = "okay";
+
+ camera@53 {
+ compatible = GMSL_CAMERA_MODEL;
+ reg = <0x53>, <0x63>;
+
+ port {
+ fakra_con2: endpoint {
+
[PATCH v4 4/7] arm64: dts: renesas: r8a77970: Add csi40 port@0
Declare port@0 in the csi40 device node and leave it un-connected.
Each board .dts file will connect the port as it requires.
Signed-off-by: Jacopo Mondi
---
arch/arm64/boot/dts/renesas/r8a77970.dtsi | 4
1 file changed, 4 insertions(+)
diff --git a/arch/arm64/boot/dts/renesas/r8a77970.dtsi
b/arch/arm64/boot/dts/renesas/r8a77970.dtsi
index 5a5d5649332a..e8f6352c3665 100644
--- a/arch/arm64/boot/dts/renesas/r8a77970.dtsi
+++ b/arch/arm64/boot/dts/renesas/r8a77970.dtsi
@@ -1106,6 +1106,10 @@ ports {
#address-cells = <1>;
#size-cells = <0>;
+ port@0 {
+ reg = <0>;
+ };
+
port@1 {
#address-cells = <1>;
#size-cells = <0>;
--
2.31.1
[PATCH v4 5/7] arm64: dts: renesas: eagle: Enable MAX9286
From: Kieran Bingham
Enable the MAX9286 GMSL deserializer on the Eagle-V3M board.
Connected cameras should be defined in a device-tree overlay or included
after these definitions.
Reviewed-by: Laurent Pinchart
Signed-off-by: Kieran Bingham
Signed-off-by: Jacopo Mondi
---
.../arm64/boot/dts/renesas/r8a77970-eagle.dts | 106 ++
1 file changed, 106 insertions(+)
diff --git a/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts
b/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts
index 874a7fc2730b..ab35202857f5 100644
--- a/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts
+++ b/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts
@@ -6,6 +6,8 @@
* Copyright (C) 2017 Cogent Embedded, Inc.
*/
+#include
+
/dts-v1/;
#include "r8a77970.dtsi"
@@ -188,6 +190,11 @@ i2c0_pins: i2c0 {
function = "i2c0";
};
+ i2c3_pins: i2c3 {
+ groups = "i2c3_a";
+ function = "i2c3";
+ };
+
qspi0_pins: qspi0 {
groups = "qspi0_ctrl", "qspi0_data4";
function = "qspi0";
@@ -266,6 +273,105 @@ {
status = "okay";
};
+ {
+ status = "okay";
+
+ ports {
+ port@0 {
+ csi40_in: endpoint {
+ clock-lanes = <0>;
+ data-lanes = <1 2 3 4>;
+ remote-endpoint = <_out0>;
+ };
+ };
+ };
+};
+
+ {
+ pinctrl-0 = <_pins>;
+ pinctrl-names = "default";
+
+ status = "okay";
+ clock-frequency = <40>;
+
+ gmsl: gmsl-deserializer@48 {
+ compatible = "maxim,max9286";
+ reg = <0x48>;
+
+ maxim,gpio-poc = <0 GPIO_ACTIVE_LOW>;
+
+ /* eagle-pca9654-max9286-pwdn */
+ enable-gpios = <_expander 0 GPIO_ACTIVE_HIGH>;
+
+ ports {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ port@0 {
+ reg = <0>;
+ };
+
+ port@1 {
+ reg = <1>;
+ };
+
+ port@2 {
+ reg = <2>;
+ };
+
+ port@3 {
+ reg = <3>;
+ };
+
+ port@4 {
+ reg = <4>;
+ max9286_out0: endpoint {
+ clock-lanes = <0>;
+ data-lanes = <1 2 3 4>;
+ remote-endpoint = <_in>;
+ };
+ };
+ };
+
+ i2c-mux {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ i2c@0 {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ reg = <0>;
+
+ status = "disabled";
+ };
+
+ i2c@1 {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ reg = <1>;
+
+ status = "disabled";
+ };
+
+ i2c@2 {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ reg = <2>;
+
+ status = "disabled";
+ };
+
+ i2c@3 {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ reg = <3>;
+
+ status = "disabled";
+ };
+ };
+ };
+};
+
{
pinctrl-0 = <_pins>;
pinctrl-names = "default";
--
2.31.1
[PATCH v4 2/7] dt-bindings: media: max9286: Define 'maxim,gpio-poc'
Define a new vendor property in the maxim,max9286 binding schema.
The new property allows to declare that the remote camera
power-over-coax is controlled by one of the MAX9286 gpio lines.
As it is currently not possible to establish a regulator as consumer
of the MAX9286 gpio controller for this purpose, the property allows to
declare that the camera power is controlled by the MAX9286 directly.
The property accepts a gpio-index (0 or 1) and one line polarity
flag as defined by dt-bindings/gpio/gpio.h.
Reviewed-by: Laurent Pinchart
Signed-off-by: Jacopo Mondi
---
.../bindings/media/i2c/maxim,max9286.yaml | 71 ++-
1 file changed, 70 insertions(+), 1 deletion(-)
diff --git a/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
b/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
index 0e7162998b77..e2422241b7d0 100644
--- a/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
+++ b/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
@@ -70,6 +70,28 @@ properties:
a remote serializer whose high-threshold noise immunity is not enabled
is 10 micro volts
+ maxim,gpio-poc:
+$ref: '/schemas/types.yaml#/definitions/uint32-array'
+minItems: 2
+maxItems: 2
+description: |
+ Index of the MAX9286 gpio output line (0 or 1) that controls Power over
+ Coax to the cameras and its associated polarity flag.
+
+ The property accepts an array of two unsigned integers, the first being
+ the gpio line index (0 or 1) and the second being the gpio line polarity
+ flag (GPIO_ACTIVE_HIGH or GPIO_ACTIVE_LOW) as defined in
+ .
+
+ When the remote cameras power is controlled by one of the MAX9286 gpio
+ lines, this property has to be used to specify which line among the two
+ available ones controls the remote camera power enablement.
+
+ When this property is used it is not possible to register a gpio
+ controller as the gpio lines are controlled directly by the MAX9286 and
+ not available for consumers, nor the 'poc-supply' property should be
+ specified.
+
ports:
$ref: /schemas/graph.yaml#/properties/ports
@@ -182,7 +204,20 @@ required:
- reg
- ports
- i2c-mux
- - gpio-controller
+
+# If 'maxim,gpio-poc' is present, then 'poc-supply' and 'gpio-controller'
+# are not allowed.
+if:
+ required:
+- maxim,gpio-poc
+then:
+ allOf:
+- not:
+required:
+ - poc-supply
+- not:
+required:
+ - gpio-controller
additionalProperties: false
@@ -327,4 +362,38 @@ examples:
};
};
};
+
+/*
+* Example of a deserializer that controls the camera Power over Coax
+* through one of its gpio lines.
+*/
+gmsl-deserializer@6c {
+compatible = "maxim,max9286";
+reg = <0x6c>;
+enable-gpios = < 14 GPIO_ACTIVE_HIGH>;
+
+/*
+* The remote camera power is controlled by MAX9286 GPIO line #0.
+* No 'poc-supply' nor 'gpio-controller' are specified.
+*/
+maxim,gpio-poc = <0 GPIO_ACTIVE_LOW>;
+
+/*
+* Do not describe connections as they're the same as in the
previous
+* example.
+*/
+ports {
+#address-cells = <1>;
+#size-cells = <0>;
+
+port@4 {
+reg = <4>;
+};
+};
+
+i2c-mux {
+#address-cells = <1>;
+#size-cells = <0>;
+};
+};
};
--
2.31.1
[PATCH v4 3/7] media: i2c: max9286: Use "maxim,gpio-poc" property
The 'maxim,gpio-poc' property is used when the remote camera
power-over-coax is controlled by one of the MAX9286 gpio lines,
to instruct the driver about which line to use and what the line
polarity is.
Add to the max9286 driver support for parsing the newly introduced
property and use it if available in place of the usual supply, as it is
not possible to establish one as consumer of the max9286 gpio
controller.
If the new property is present, no gpio controller is registered and
'poc-supply' is ignored.
In order to maximize code re-use, break out the max9286 gpio handling
function so that they can be used by the gpio controller through the
gpio-consumer API, or directly by the driver code.
Wrap the power up and power down routines to their own function to
be able to use either the gpio line directly or the supply. This will
make it easier to control the remote camera power at run time.
Signed-off-by: Jacopo Mondi
---
drivers/media/i2c/max9286.c | 119
1 file changed, 93 insertions(+), 26 deletions(-)
diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
index 6fd4d59fcc72..a8b37783d2ef 100644
--- a/drivers/media/i2c/max9286.c
+++ b/drivers/media/i2c/max9286.c
@@ -15,6 +15,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -165,6 +166,9 @@ struct max9286_priv {
u32 reverse_channel_mv;
+ u32 gpio_poc;
+ u32 gpio_poc_flags;
+
struct v4l2_ctrl_handler ctrls;
struct v4l2_ctrl *pixelrate;
@@ -1022,8 +1026,19 @@ static int max9286_setup(struct max9286_priv *priv)
return 0;
}
-static void max9286_gpio_set(struct gpio_chip *chip,
-unsigned int offset, int value)
+static int max9286_gpio_set(struct max9286_priv *priv, unsigned int offset,
+ int value)
+{
+ int ret = max9286_read(priv, 0x0f);
+ if (ret < 0)
+ return ret;
+
+ ret = value ? (ret & 0x3) | BIT(offset) : (ret & 0x3) & ~BIT(offset);
+ return max9286_write(priv, 0x0f, MAX9286_0X0F_RESERVED | ret);
+}
+
+static void max9286_gpiochip_set(struct gpio_chip *chip,
+unsigned int offset, int value)
{
struct max9286_priv *priv = gpiochip_get_data(chip);
@@ -1035,7 +1050,7 @@ static void max9286_gpio_set(struct gpio_chip *chip,
max9286_write(priv, 0x0f, MAX9286_0X0F_RESERVED | priv->gpio_state);
}
-static int max9286_gpio_get(struct gpio_chip *chip, unsigned int offset)
+static int max9286_gpiochip_get(struct gpio_chip *chip, unsigned int offset)
{
struct max9286_priv *priv = gpiochip_get_data(chip);
@@ -1055,8 +1070,8 @@ static int max9286_register_gpio(struct max9286_priv
*priv)
gpio->of_node = dev->of_node;
gpio->ngpio = 2;
gpio->base = -1;
- gpio->set = max9286_gpio_set;
- gpio->get = max9286_gpio_get;
+ gpio->set = max9286_gpiochip_set;
+ gpio->get = max9286_gpiochip_get;
gpio->can_sleep = true;
/* GPIO values default to high */
@@ -1069,6 +1084,71 @@ static int max9286_register_gpio(struct max9286_priv
*priv)
return ret;
}
+static int max9286_parse_gpios(struct max9286_priv *priv)
+{
+ struct device *dev = >client->dev;
+ u32 gpio_poc[2];
+ int ret;
+
+ /*
+* Parse the "gpio-poc" vendor property. If the camera power is
+* controlled by one of the MAX9286 gpio lines, do not register
+* the gpio controller and ignore 'poc-supply'.
+*/
+ ret = of_property_read_u32_array(dev->of_node,
+"maxim,gpio-poc", gpio_poc, 2);
+ if (!ret) {
+ priv->gpio_poc = gpio_poc[0];
+ priv->gpio_poc_flags = gpio_poc[1];
+ if (priv->gpio_poc > 1 ||
+ (priv->gpio_poc_flags != GPIO_ACTIVE_HIGH &&
+priv->gpio_poc_flags != GPIO_ACTIVE_LOW)) {
+ dev_err(dev, "Invalid 'gpio-poc': (%u %u)\n",
+ priv->gpio_poc, priv->gpio_poc_flags);
+ return -EINVAL;
+ }
+
+ return 0;
+ }
+
+ ret = max9286_register_gpio(priv);
+ if (ret)
+ return ret;
+
+ priv->regulator = devm_regulator_get(dev, "poc");
+ if (IS_ERR(priv->regulator)) {
+ if (PTR_ERR(priv->regulator) != -EPROBE_DEFER)
+ dev_err(dev, "Unable to get PoC regulator (%ld)\n",
+ PTR_ERR(priv->regulator));
+ return PTR_ERR(priv->regulator);
+ }
+
+ return 0;
+}
+
+static int max9286_poc_enable(struct max9286_priv *priv, bool enable)
+{
+ int ret;
+
+ /* If "poc-gpio" is us
[PATCH v4 1/7] dt-bindings: media: max9286: Re-indent example
The dt-bindings examples are usually indented with 4 spaces.
The maxim,max9286 schema has the example indented with only
2 spaces, re-indent it.
Cosmetic change only.
Signed-off-by: Jacopo Mondi
---
.../bindings/media/i2c/maxim,max9286.yaml | 214 +-
1 file changed, 107 insertions(+), 107 deletions(-)
diff --git a/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
b/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
index ee16102fdfe7..0e7162998b77 100644
--- a/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
+++ b/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
@@ -191,140 +191,140 @@ examples:
#include
i2c@e66d8000 {
- #address-cells = <1>;
- #size-cells = <0>;
+#address-cells = <1>;
+#size-cells = <0>;
- reg = <0 0xe66d8000>;
+reg = <0 0xe66d8000>;
- gmsl-deserializer@2c {
-compatible = "maxim,max9286";
-reg = <0x2c>;
-poc-supply = <_poc_12v>;
-enable-gpios = < 13 GPIO_ACTIVE_HIGH>;
+gmsl-deserializer@2c {
+compatible = "maxim,max9286";
+reg = <0x2c>;
+poc-supply = <_poc_12v>;
+enable-gpios = < 13 GPIO_ACTIVE_HIGH>;
-gpio-controller;
-#gpio-cells = <2>;
+gpio-controller;
+#gpio-cells = <2>;
-maxim,reverse-channel-microvolt = <17>;
+maxim,reverse-channel-microvolt = <17>;
-ports {
- #address-cells = <1>;
- #size-cells = <0>;
+ports {
+#address-cells = <1>;
+#size-cells = <0>;
- port@0 {
-reg = <0>;
+port@0 {
+reg = <0>;
-max9286_in0: endpoint {
- remote-endpoint = <_out0>;
-};
- };
-
- port@1 {
-reg = <1>;
-
-max9286_in1: endpoint {
- remote-endpoint = <_out1>;
-};
- };
-
- port@2 {
-reg = <2>;
-
-max9286_in2: endpoint {
- remote-endpoint = <_out2>;
-};
- };
+max9286_in0: endpoint {
+remote-endpoint = <_out0>;
+};
+};
- port@3 {
-reg = <3>;
+port@1 {
+reg = <1>;
-max9286_in3: endpoint {
- remote-endpoint = <_out3>;
-};
- };
+max9286_in1: endpoint {
+remote-endpoint = <_out1>;
+};
+};
- port@4 {
-reg = <4>;
+port@2 {
+reg = <2>;
-max9286_out: endpoint {
- data-lanes = <1 2 3 4>;
- remote-endpoint = <_in>;
-};
- };
-};
+max9286_in2: endpoint {
+remote-endpoint = <_out2>;
+};
+};
-i2c-mux {
- #address-cells = <1>;
- #size-cells = <0>;
+port@3 {
+reg = <3>;
- i2c@0 {
-#address-cells = <1>;
-#size-cells = <0>;
-reg = <0>;
+max9286_in3: endpoint {
+remote-endpoint = <_out3>;
+};
+};
-camera@51 {
- compatible = "imi,rdacm20";
- reg = <0x51>, <0x61>;
+port@4 {
+reg = <4>;
- port {
-rdacm20_out0: endpoint {
- remote-endpoint = <_in0>;
+max9286_out: endpoint {
+data-lanes = <1 2 3 4>;
+remote-endpoint = <_in>;
+};
};
- };
-
};
- };
-
- i2c@1 {
-#address-cells = <1>;
-#size-cells = <0>;
-reg = <1>;
-camera@52 {
- compatible = "imi,rdacm20";
- reg = <0x52>, <0x62>;
+i2c-mux {
+#address-cells = <1>;
+#size-cells = <0>;
- port {
-rdacm20_out1: endpoint {
- remote-endpoint = <_in1>;
-};
- };
-};
- };
+i2c@0 {
+#address-cells = <1>;
+#size-
[PATCH v4 0/7] arm64: dts: renesas: Enable GMSL on R8A77970 V3M Eagle
Following the recent v3, this new version: - Two new patches (minor fixes) - Address Laurent's comments on gpio-poc bindings and implementation. Naming might still be discussed - Address Laurent's comments on DTS patches - Last patch not for inclusion Thanks j Jacopo Mondi (4): dt-bindings: media: max9286: Re-indent example dt-bindings: media: max9286: Define 'maxim,gpio-poc' media: i2c: max9286: Use "maxim,gpio-poc" property arm64: dts: renesas: r8a77970: Add csi40 port@0 Kieran Bingham (3): arm64: dts: renesas: eagle: Enable MAX9286 arm64: dts: renesas: eagle: Add GMSL .dtsi DNI: arm64: dts: renesas: eagle: Include eagle-gmsl .../bindings/media/i2c/maxim,max9286.yaml | 279 +++--- arch/arm64/boot/dts/renesas/eagle-gmsl.dtsi | 178 +++ .../arm64/boot/dts/renesas/r8a77970-eagle.dts | 114 +++ arch/arm64/boot/dts/renesas/r8a77970.dtsi | 4 + drivers/media/i2c/max9286.c | 119 ++-- 5 files changed, 563 insertions(+), 131 deletions(-) create mode 100644 arch/arm64/boot/dts/renesas/eagle-gmsl.dtsi -- 2.31.1
Re: [PATCH v3 4/5] arm64: dts: renesas: eagle: Add GMSL .dtsi
Hi Laurent,
On Thu, Apr 15, 2021 at 02:53:17AM +0300, Laurent Pinchart wrote:
> Hi Jacopo and Kieran,
>
> Thank you for the patch.
>
> On Wed, Apr 14, 2021 at 03:51:27PM +0200, Jacopo Mondi wrote:
> > From: Kieran Bingham
> >
> > Describe the FAKRA connector available on Eagle board that allows
> > connecting GMSL camera modules such as IMI RDACM20 and RDACM21.
> >
> > Signed-off-by: Kieran Bingham
> > Signed-off-by: Jacopo Mondi
> > ---
> > arch/arm64/boot/dts/renesas/eagle-gmsl.dtsi | 186
> > 1 file changed, 186 insertions(+)
> > create mode 100644 arch/arm64/boot/dts/renesas/eagle-gmsl.dtsi
> >
> > diff --git a/arch/arm64/boot/dts/renesas/eagle-gmsl.dtsi
> > b/arch/arm64/boot/dts/renesas/eagle-gmsl.dtsi
> > new file mode 100644
> > index ..1836bca1e8b2
> > --- /dev/null
> > +++ b/arch/arm64/boot/dts/renesas/eagle-gmsl.dtsi
> > @@ -0,0 +1,186 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Device Tree Source (overlay) for the Eagle V3M GMSL connectors
> > + *
> > + * Copyright (C) 2017 Ideas on Board
> > + * Copyright (C) 2021 Jacopo Mondi
> > + *
> > + * This overlay allows you to define GMSL cameras connected to the FAKRA
> > + * connectors on the Eagle-V3M (or compatible) board.
> > + *
> > + * The following cameras are currently supported:
> > + *"imi,rdacm20"
> > + *"imi,rdacm21"
> > + */
> > +
> > +#include
> > +
> > +/*
> > + * Select which cameras are in use:
> > + * #define EAGLE_CAMERA0_RDACM20
> > + * #define EAGLE_CAMERA0_RDACM21
> > + *
> > + * The two camera modules are configured with different image formats
> > + * and cannot be mixed.
> > + */
> > +#define EAGLE_CAMERA0_RDACM21
> > +#define EAGLE_CAMERA1_RDACM21
> > +#define EAGLE_CAMERA2_RDACM21
> > +#define EAGLE_CAMERA3_RDACM21
>
> To avoid possible errors, I'd use one macro to set the camera model, and
> another macro to select the ports. The second could be a bitmask if
> desired.
>
> These macros should be defined in the file that includes this file, not
> here.
Don't worry, I have quite some comments from you on the dts patches
from the previous versio of the series still to address... As said in
the cover letter I was mostly interested in a validation of the new
property and the driver support.
Thanks
j
>
> > +
> > +/* Set the compatible string based on the camera model. */
> > +#if defined(EAGLE_CAMERA0_RDACM21) || defined(EAGLE_CAMERA1_RDACM21) || \
> > +defined(EAGLE_CAMERA2_RDACM21) || defined(EAGLE_CAMERA3_RDACM21)
> > +#define EAGLE_CAMERA_MODEL "imi,rdacm21"
> > +#define EAGLE_USE_RDACM21
> > +#elif defined(EAGLE_CAMERA0_RDACM20) || defined(EAGLE_CAMERA1_RDACM20) || \
> > + defined(EAGLE_CAMERA2_RDACM20) || defined(EAGLE_CAMERA3_RDACM20)
> > +#define EAGLE_CAMERA_MODEL "imi,rdacm20"
> > +#define EAGLE_USE_RDACM20
> > +#endif
> > +
> > +/* Define which cameras are available. */
> > +#if defined(EAGLE_CAMERA0_RDACM21) || defined(EAGLE_CAMERA0_RDACM20)
> > +#define EAGLE_USE_CAMERA_0
> > +#endif
> > +
> > +#if defined(EAGLE_CAMERA1_RDACM21) || defined(EAGLE_CAMERA1_RDACM20)
> > +#define EAGLE_USE_CAMERA_1
> > +#endif
> > +
> > +#if defined(EAGLE_CAMERA2_RDACM21) || defined(EAGLE_CAMERA2_RDACM20)
> > +#define EAGLE_USE_CAMERA_2
> > +#endif
> > +
> > +#if defined(EAGLE_CAMERA3_RDACM21) || defined(EAGLE_CAMERA3_RDACM20)
> > +#define EAGLE_USE_CAMERA_3
> > +#endif
> > +
> > +/* Define the endpoint links. */
> > +#ifdef EAGLE_USE_CAMERA_0
> > +_in0 {
> > + remote-endpoint = <_con0>;
> > +};
> > +#endif
> > +
> > +#ifdef EAGLE_USE_CAMERA_1
> > +_in1 {
> > + remote-endpoint = <_con1>;
> > +};
> > +#endif
> > +
> > +#ifdef EAGLE_USE_CAMERA_2
> > +_in2 {
> > + remote-endpoint = <_con2>;
> > +};
> > +#endif
> > +
> > +#ifdef EAGLE_USE_CAMERA_3
> > +_in3 {
> > + remote-endpoint = <_con3>;
> > +};
> > +#endif
> > +
> > +/* Populate the GMSL i2c-mux bus with camera nodes. */
> > +#if defined(EAGLE_USE_RDACM21) || defined(EAGLE_USE_RDACM20)
> > +
> > +#ifdef EAGLE_USE_CAMERA_0
> > + {
> > + status = "okay";
> > +};
> > +#endif
> > +
> > +#ifdef EAGLE_USE_CAMERA_1
> >
Re: [PATCH v3 2/5] media: i2c: max9286: Use "maxim,gpio-poc" property
Hi Laurent,
On Thu, Apr 15, 2021 at 03:00:56AM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Wed, Apr 14, 2021 at 03:51:25PM +0200, Jacopo Mondi wrote:
> > The 'maxim,gpio-poc' property is used when the remote camera
> > power-over-coax is controlled by one of the MAX9286 gpio lines,
> > to instruct the driver about which line to use and what the line
> > polarity is.
> >
> > Add to the max9286 driver support for parsing the newly introduce
> s/introduce/introduced/
>
> > property and use it if available in place of the usual supply, as it is
> > not possible to establish one as consumer of the max9286 gpio
> > controller.
> >
> > If the new property is present, no gpio controller is registered and
> > 'poc-supply' is ignored.
> >
> > In order to maximize code re-use, break out the max9286 gpio handling
> > function so that they can be used by the gpio controller through the
> > gpio-consumer API, or directly by the driver code.
> >
> > Wrap the power up and power down routines to their own function to
> > be able to use either the gpio line directly or the supply. This will
> > make it easier to control the remote camera power at run time.
>
> I would have split the patch in two, with a first patch that refactors
> the code, and a second one that extends it, but that's no big deal.
>
> > Signed-off-by: Jacopo Mondi
> > ---
> > drivers/media/i2c/max9286.c | 125 +++-
> > 1 file changed, 96 insertions(+), 29 deletions(-)
> >
> > diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> > index 6fd4d59fcc72..0c125f7b3d9b 100644
> > --- a/drivers/media/i2c/max9286.c
> > +++ b/drivers/media/i2c/max9286.c
> > @@ -15,6 +15,7 @@
> > #include
> > #include
> > #include
> > +#include
> > #include
> > #include
> > #include
> > @@ -165,6 +166,9 @@ struct max9286_priv {
> >
> > u32 reverse_channel_mv;
> >
> > + u32 gpio_poc;
> > + u32 gpio_poc_flags;
> > +
> > struct v4l2_ctrl_handler ctrls;
> > struct v4l2_ctrl *pixelrate;
> >
> > @@ -1022,20 +1026,27 @@ static int max9286_setup(struct max9286_priv *priv)
> > return 0;
> > }
> >
> > -static void max9286_gpio_set(struct gpio_chip *chip,
> > -unsigned int offset, int value)
> > +static int max9286_gpio_set(struct max9286_priv *priv, unsigned int offset,
> > + int value)
> > {
> > - struct max9286_priv *priv = gpiochip_get_data(chip);
> > -
> > if (value)
> > priv->gpio_state |= BIT(offset);
> > else
> > priv->gpio_state &= ~BIT(offset);
> >
> > - max9286_write(priv, 0x0f, MAX9286_0X0F_RESERVED | priv->gpio_state);
> > + return max9286_write(priv, 0x0f,
> > +MAX9286_0X0F_RESERVED | priv->gpio_state);
> > +}
> > +
> > +static void max9286_gpiochip_set(struct gpio_chip *chip,
> > +unsigned int offset, int value)
> > +{
> > + struct max9286_priv *priv = gpiochip_get_data(chip);
> > +
> > + max9286_gpio_set(priv, offset, value);
> > }
> >
> > -static int max9286_gpio_get(struct gpio_chip *chip, unsigned int offset)
> > +static int max9286_gpiochip_get(struct gpio_chip *chip, unsigned int
> > offset)
> > {
> > struct max9286_priv *priv = gpiochip_get_data(chip);
> >
> > @@ -1055,8 +1066,8 @@ static int max9286_register_gpio(struct max9286_priv
> > *priv)
> > gpio->of_node = dev->of_node;
> > gpio->ngpio = 2;
> > gpio->base = -1;
> > - gpio->set = max9286_gpio_set;
> > - gpio->get = max9286_gpio_get;
> > + gpio->set = max9286_gpiochip_set;
> > + gpio->get = max9286_gpiochip_get;
> > gpio->can_sleep = true;
> >
> > /* GPIO values default to high */
> > @@ -1069,6 +1080,75 @@ static int max9286_register_gpio(struct max9286_priv
> > *priv)
> > return ret;
> > }
> >
> > +static int max9286_parse_gpios(struct max9286_priv *priv)
> > +{
> > + struct device *dev = >client->dev;
> > + u32 gpio_poc[2];
> > + int ret;
> > +
> > + /*
> > +* Parse the "gpio-poc" vendor property. If the camera power is
> > +* controlled by one of the MAX9286 gpio lines, do not register
> > +* the gpio controller and ignore 'poc-supply'.
> >
Re: [PATCH v3 1/5] dt-bindings: media: max9286: Define 'maxim,gpio-poc'
Hi Laurent,
On Thu, Apr 15, 2021 at 02:47:12AM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Wed, Apr 14, 2021 at 03:51:24PM +0200, Jacopo Mondi wrote:
> > Define a new vendor property in the maxim,max9286 binding schema.
> >
> > The new property allows to declare that the remote camera
> > power-over-coax is controlled by one of the MAX9286 gpio lines.
> >
> > As it is currently not possible to establish a regulator as consumer
> > of the MAX9286 gpio controller for this purpose, the property allows to
> > declare that the camera power is controlled by the MAX9286 directly.
> >
> > The property accepts a gpio-index (0 or 1) and one line polarity
> > flag as defined by dt-bindings/gpio/gpio.h.
> >
> > Signed-off-by: Jacopo Mondi
> > ---
> > .../bindings/media/i2c/maxim,max9286.yaml | 53 ++-
> > 1 file changed, 52 insertions(+), 1 deletion(-)
> >
> > diff --git a/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
> > b/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
> > index ee16102fdfe7..480a491f3744 100644
> > --- a/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
> > +++ b/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
> > @@ -70,6 +70,24 @@ properties:
> >a remote serializer whose high-threshold noise immunity is not
> > enabled
> >is 10 micro volts
> >
> > + maxim,gpio-poc:
>
> I would have written poc-gpio to match the order of the GPIO bindings
> syntax.
>
That's what I had :) but then the property gets matched against the
gpio schema and I get complains because it expects a phandle as first
argument... Maybe there's a way I've missed to prevent the property to
be matched with *-gpio ?
> > +$ref: '/schemas/types.yaml#/definitions/uint32-array'
> > +minItems: 2
> > +maxItems: 2
> > +description: |
> > + Identifier of gpio line that controls Power over Coax to the cameras
> > and
>
> I'd write "Index of the MAX9286 GPIO output that ..." to make it clear
> that this is not a generic GPIO.
>
Ack
> > + the associated polarity flag (GPIO_ACTIVE_HIGH and GPIO_ACTIVE_LOW)
> > + as defined in .
> > +
> > + When the remote cameras power is controlled by one of the MAX9286
> > gpio
> > + lines, this property has to be used to specify which line among the
> > two
> > + available ones controls the remote camera power enablement.
> > +
> > + When this property is used it is not possible to register a gpio
> > + controller as the gpio lines are controlled directly by the MAX9286
> > and
> > + not available for consumers, nor the 'poc-supply' property should be
> > + specified.
>
> Only one of the two lines would be controlled directly. Shouldn't we
> still register a GPIO controller for the other line ?
I considered that and thought it was a bit of an overkill (and I also
had a bit of troubles identifying how to register only gpio #1, as it
would be identified as gpio #0 if the actual #0 is not registered)
>
> Could you also mention somewhere that the first item in the array should
> be 0 or 1 ? It may be hard to express in a YAML schema, so I'm fine just
> documenting it in the description.
>
Sure, I tried identifying how to express that with yaml and failed :)
> I've been wondering whether this would be a common enough issue that it
> could justify support in the GPIO core to handle consumer-provider
> loops, but even if that happens at some point in the future, I think the
> proposal here is good enough and we won't need to switch.
>
Please note that with the suggestion offline from rob I will add to
the next version:
# If 'maxim,gpio-poc' is present, then 'poc-supply' and 'gpio-controller'
# are not allowed.
if:
required:
- maxim,gpio-poc
then:
allOf:
- not:
required:
- poc-supply
- not:
required:
- gpio-controller
> > +
> >ports:
> > $ref: /schemas/graph.yaml#/properties/ports
> >
> > @@ -182,7 +200,6 @@ required:
> >- reg
> >- ports
> >- i2c-mux
> > - - gpio-controller
> >
> > additionalProperties: false
> >
> > @@ -327,4 +344,38 @@ examples:
> >};
> > };
> >};
> > +
> > + /*
> > + * Example of a deserializer that controls the camera Power over Coax
> > + * through one of its gpio lines.
> > + */
> > + gmsl-deserializer@6c {
> > +com
[PATCH v3 3/5] arm64: dts: renesas: eagle: Enable MAX9286
From: Kieran Bingham
Enable the MAX9286 GMSL deserializer on the Eagle-V3M board.
Connected cameras should be defined in a device-tree overlay or included
after these definitions.
Signed-off-by: Kieran Bingham
Signed-off-by: Jacopo Mondi
---
.../arm64/boot/dts/renesas/r8a77970-eagle.dts | 119 ++
1 file changed, 119 insertions(+)
diff --git a/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts
b/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts
index 874a7fc2730b..d2b6368d1e72 100644
--- a/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts
+++ b/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts
@@ -6,6 +6,8 @@
* Copyright (C) 2017 Cogent Embedded, Inc.
*/
+#include
+
/dts-v1/;
#include "r8a77970.dtsi"
@@ -188,6 +190,11 @@ i2c0_pins: i2c0 {
function = "i2c0";
};
+ i2c3_pins: i2c3 {
+ groups = "i2c3_a";
+ function = "i2c3";
+ };
+
qspi0_pins: qspi0 {
groups = "qspi0_ctrl", "qspi0_data4";
function = "qspi0";
@@ -266,6 +273,118 @@ {
status = "okay";
};
+ {
+ status = "okay";
+
+ ports {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ port@0 {
+ reg = <0>;
+
+ csi40_in: endpoint {
+ clock-lanes = <0>;
+ data-lanes = <1 2 3 4>;
+ remote-endpoint = <_out0>;
+ };
+ };
+ };
+};
+
+ {
+ pinctrl-0 = <_pins>;
+ pinctrl-names = "default";
+
+ status = "okay";
+ clock-frequency = <40>;
+
+ gmsl: gmsl-deserializer@48 {
+ compatible = "maxim,max9286";
+ reg = <0x48>;
+
+ maxim,gpio-poc = <0 GPIO_ACTIVE_LOW>;
+
+ /* eagle-pca9654-max9286-pwdn */
+ enable-gpios = <_expander 0 GPIO_ACTIVE_HIGH>;
+
+ ports {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ port@0 {
+ reg = <0>;
+ max9286_in0: endpoint {
+ };
+ };
+
+ port@1 {
+ reg = <1>;
+ max9286_in1: endpoint {
+ };
+ };
+
+ port@2 {
+ reg = <2>;
+ max9286_in2: endpoint {
+ };
+ };
+
+ port@3 {
+ reg = <3>;
+ max9286_in3: endpoint {
+ };
+ };
+
+ port@4 {
+ reg = <4>;
+ max9286_out0: endpoint {
+ clock-lanes = <0>;
+ data-lanes = <1 2 3 4>;
+ remote-endpoint = <_in>;
+ };
+ };
+ };
+
+ i2c-mux {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ i2c@0 {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ reg = <0>;
+
+ status = "disabled";
+ };
+
+ i2c@1 {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ reg = <1>;
+
+ status = "disabled";
+ };
+
+ i2c@2 {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ reg = <2>;
+
+ status = "disabled";
+ };
+
+ i2c@3 {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ reg = <3>;
+
+ status = "disabled";
+ };
+ };
+ };
+};
+
{
pinctrl-0 = <_pins>;
pinctrl-names = "default";
--
2.31.1
[PATCH v3 5/5] arm64: dts: renesas: eagle: Include eagle-gmsl
From: Kieran Bingham
Include the eagle-gmsl.dtsi to enable GMSL camera support on the
Eagle-V3M platform.
Signed-off-by: Kieran Bingham
Signed-off-by: Jacopo Mondi
---
arch/arm64/boot/dts/renesas/r8a77970-eagle.dts | 3 +++
1 file changed, 3 insertions(+)
diff --git a/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts
b/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts
index d2b6368d1e72..9b8dfb5132fb 100644
--- a/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts
+++ b/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts
@@ -391,3 +391,6 @@ {
status = "okay";
};
+
+/* FAKRA Overlay */
+#include "eagle-gmsl.dtsi"
--
2.31.1
[PATCH v3 4/5] arm64: dts: renesas: eagle: Add GMSL .dtsi
From: Kieran Bingham
Describe the FAKRA connector available on Eagle board that allows
connecting GMSL camera modules such as IMI RDACM20 and RDACM21.
Signed-off-by: Kieran Bingham
Signed-off-by: Jacopo Mondi
---
arch/arm64/boot/dts/renesas/eagle-gmsl.dtsi | 186
1 file changed, 186 insertions(+)
create mode 100644 arch/arm64/boot/dts/renesas/eagle-gmsl.dtsi
diff --git a/arch/arm64/boot/dts/renesas/eagle-gmsl.dtsi
b/arch/arm64/boot/dts/renesas/eagle-gmsl.dtsi
new file mode 100644
index ..1836bca1e8b2
--- /dev/null
+++ b/arch/arm64/boot/dts/renesas/eagle-gmsl.dtsi
@@ -0,0 +1,186 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Device Tree Source (overlay) for the Eagle V3M GMSL connectors
+ *
+ * Copyright (C) 2017 Ideas on Board
+ * Copyright (C) 2021 Jacopo Mondi
+ *
+ * This overlay allows you to define GMSL cameras connected to the FAKRA
+ * connectors on the Eagle-V3M (or compatible) board.
+ *
+ * The following cameras are currently supported:
+ *"imi,rdacm20"
+ *"imi,rdacm21"
+ */
+
+#include
+
+/*
+ * Select which cameras are in use:
+ * #define EAGLE_CAMERA0_RDACM20
+ * #define EAGLE_CAMERA0_RDACM21
+ *
+ * The two camera modules are configured with different image formats
+ * and cannot be mixed.
+ */
+#define EAGLE_CAMERA0_RDACM21
+#define EAGLE_CAMERA1_RDACM21
+#define EAGLE_CAMERA2_RDACM21
+#define EAGLE_CAMERA3_RDACM21
+
+/* Set the compatible string based on the camera model. */
+#if defined(EAGLE_CAMERA0_RDACM21) || defined(EAGLE_CAMERA1_RDACM21) || \
+defined(EAGLE_CAMERA2_RDACM21) || defined(EAGLE_CAMERA3_RDACM21)
+#define EAGLE_CAMERA_MODEL "imi,rdacm21"
+#define EAGLE_USE_RDACM21
+#elif defined(EAGLE_CAMERA0_RDACM20) || defined(EAGLE_CAMERA1_RDACM20) || \
+ defined(EAGLE_CAMERA2_RDACM20) || defined(EAGLE_CAMERA3_RDACM20)
+#define EAGLE_CAMERA_MODEL "imi,rdacm20"
+#define EAGLE_USE_RDACM20
+#endif
+
+/* Define which cameras are available. */
+#if defined(EAGLE_CAMERA0_RDACM21) || defined(EAGLE_CAMERA0_RDACM20)
+#define EAGLE_USE_CAMERA_0
+#endif
+
+#if defined(EAGLE_CAMERA1_RDACM21) || defined(EAGLE_CAMERA1_RDACM20)
+#define EAGLE_USE_CAMERA_1
+#endif
+
+#if defined(EAGLE_CAMERA2_RDACM21) || defined(EAGLE_CAMERA2_RDACM20)
+#define EAGLE_USE_CAMERA_2
+#endif
+
+#if defined(EAGLE_CAMERA3_RDACM21) || defined(EAGLE_CAMERA3_RDACM20)
+#define EAGLE_USE_CAMERA_3
+#endif
+
+/* Define the endpoint links. */
+#ifdef EAGLE_USE_CAMERA_0
+_in0 {
+ remote-endpoint = <_con0>;
+};
+#endif
+
+#ifdef EAGLE_USE_CAMERA_1
+_in1 {
+ remote-endpoint = <_con1>;
+};
+#endif
+
+#ifdef EAGLE_USE_CAMERA_2
+_in2 {
+ remote-endpoint = <_con2>;
+};
+#endif
+
+#ifdef EAGLE_USE_CAMERA_3
+_in3 {
+ remote-endpoint = <_con3>;
+};
+#endif
+
+/* Populate the GMSL i2c-mux bus with camera nodes. */
+#if defined(EAGLE_USE_RDACM21) || defined(EAGLE_USE_RDACM20)
+
+#ifdef EAGLE_USE_CAMERA_0
+ {
+ status = "okay";
+};
+#endif
+
+#ifdef EAGLE_USE_CAMERA_1
+ {
+ status = "okay";
+};
+#endif
+
+#ifdef EAGLE_USE_CAMERA_2
+ {
+ status = "okay";
+};
+#endif
+
+#ifdef EAGLE_USE_CAMERA_3
+ {
+ status = "okay";
+};
+#endif
+
+ {
+
+ status = "okay";
+ maxim,reverse-channel-microvolt = <10>;
+
+ i2c-mux {
+#ifdef EAGLE_USE_CAMERA_0
+ i2c@0 {
+ status = "okay";
+
+ camera@51 {
+ compatible = EAGLE_CAMERA_MODEL;
+ reg = <0x51>, <0x61>;
+
+ port {
+ fakra_con0: endpoint {
+ remote-endpoint =
<_in0>;
+ };
+ };
+ };
+ };
+#endif
+
+#ifdef EAGLE_USE_CAMERA_1
+ i2c@1 {
+ status = "okay";
+
+ camera@52 {
+ compatible = EAGLE_CAMERA_MODEL;
+ reg = <0x52>, <0x62>;
+
+ port {
+ fakra_con1: endpoint {
+ remote-endpoint =
<_in1>;
+ };
+ };
+ };
+ };
+#endif
+
+#ifdef EAGLE_USE_CAMERA_2
+ i2c@2 {
+ status = "okay";
+
+ camera@53 {
+ compatible = EAGLE_CAMERA_MODEL;
+ reg = <0x53>, <0x63>;
+
+ port {
+ fakra_con2: endpoint {
+
[PATCH v3 2/5] media: i2c: max9286: Use "maxim,gpio-poc" property
The 'maxim,gpio-poc' property is used when the remote camera
power-over-coax is controlled by one of the MAX9286 gpio lines,
to instruct the driver about which line to use and what the line
polarity is.
Add to the max9286 driver support for parsing the newly introduce
property and use it if available in place of the usual supply, as it is
not possible to establish one as consumer of the max9286 gpio
controller.
If the new property is present, no gpio controller is registered and
'poc-supply' is ignored.
In order to maximize code re-use, break out the max9286 gpio handling
function so that they can be used by the gpio controller through the
gpio-consumer API, or directly by the driver code.
Wrap the power up and power down routines to their own function to
be able to use either the gpio line directly or the supply. This will
make it easier to control the remote camera power at run time.
Signed-off-by: Jacopo Mondi
---
drivers/media/i2c/max9286.c | 125 +++-
1 file changed, 96 insertions(+), 29 deletions(-)
diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
index 6fd4d59fcc72..0c125f7b3d9b 100644
--- a/drivers/media/i2c/max9286.c
+++ b/drivers/media/i2c/max9286.c
@@ -15,6 +15,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -165,6 +166,9 @@ struct max9286_priv {
u32 reverse_channel_mv;
+ u32 gpio_poc;
+ u32 gpio_poc_flags;
+
struct v4l2_ctrl_handler ctrls;
struct v4l2_ctrl *pixelrate;
@@ -1022,20 +1026,27 @@ static int max9286_setup(struct max9286_priv *priv)
return 0;
}
-static void max9286_gpio_set(struct gpio_chip *chip,
-unsigned int offset, int value)
+static int max9286_gpio_set(struct max9286_priv *priv, unsigned int offset,
+ int value)
{
- struct max9286_priv *priv = gpiochip_get_data(chip);
-
if (value)
priv->gpio_state |= BIT(offset);
else
priv->gpio_state &= ~BIT(offset);
- max9286_write(priv, 0x0f, MAX9286_0X0F_RESERVED | priv->gpio_state);
+ return max9286_write(priv, 0x0f,
+MAX9286_0X0F_RESERVED | priv->gpio_state);
+}
+
+static void max9286_gpiochip_set(struct gpio_chip *chip,
+unsigned int offset, int value)
+{
+ struct max9286_priv *priv = gpiochip_get_data(chip);
+
+ max9286_gpio_set(priv, offset, value);
}
-static int max9286_gpio_get(struct gpio_chip *chip, unsigned int offset)
+static int max9286_gpiochip_get(struct gpio_chip *chip, unsigned int offset)
{
struct max9286_priv *priv = gpiochip_get_data(chip);
@@ -1055,8 +1066,8 @@ static int max9286_register_gpio(struct max9286_priv
*priv)
gpio->of_node = dev->of_node;
gpio->ngpio = 2;
gpio->base = -1;
- gpio->set = max9286_gpio_set;
- gpio->get = max9286_gpio_get;
+ gpio->set = max9286_gpiochip_set;
+ gpio->get = max9286_gpiochip_get;
gpio->can_sleep = true;
/* GPIO values default to high */
@@ -1069,6 +1080,75 @@ static int max9286_register_gpio(struct max9286_priv
*priv)
return ret;
}
+static int max9286_parse_gpios(struct max9286_priv *priv)
+{
+ struct device *dev = >client->dev;
+ u32 gpio_poc[2];
+ int ret;
+
+ /*
+* Parse the "gpio-poc" vendor property. If the camera power is
+* controlled by one of the MAX9286 gpio lines, do not register
+* the gpio controller and ignore 'poc-supply'.
+*/
+ ret = of_property_read_u32_array(dev->of_node,
+"maxim,gpio-poc", gpio_poc, 2);
+ if (!ret) {
+ priv->gpio_poc = gpio_poc[0];
+ priv->gpio_poc_flags = gpio_poc[1];
+ if ((priv->gpio_poc != 0 && priv->gpio_poc != 1) ||
+ (priv->gpio_poc_flags != GPIO_ACTIVE_HIGH &&
+priv->gpio_poc_flags != GPIO_ACTIVE_LOW)) {
+ dev_err(dev, "Invalid 'gpio-poc': (%u %u)\n",
+ priv->gpio_poc, priv->gpio_poc_flags);
+ return -EINVAL;
+ }
+
+ /* GPIO values default to high */
+ priv->gpio_state = BIT(0) | BIT(1);
+ priv->regulator = NULL;
+
+ return 0;
+ }
+
+ ret = max9286_register_gpio(priv);
+ if (ret)
+ return ret;
+
+ priv->regulator = devm_regulator_get(dev, "poc");
+ if (IS_ERR(priv->regulator)) {
+ if (PTR_ERR(priv->regulator) != -EPROBE_DEFER)
+ dev_err(dev, "Unable to get PoC regulator (%ld)\n",
+ PTR_ERR(priv->regulator));
+
[PATCH v3 1/5] dt-bindings: media: max9286: Define 'maxim,gpio-poc'
Define a new vendor property in the maxim,max9286 binding schema.
The new property allows to declare that the remote camera
power-over-coax is controlled by one of the MAX9286 gpio lines.
As it is currently not possible to establish a regulator as consumer
of the MAX9286 gpio controller for this purpose, the property allows to
declare that the camera power is controlled by the MAX9286 directly.
The property accepts a gpio-index (0 or 1) and one line polarity
flag as defined by dt-bindings/gpio/gpio.h.
Signed-off-by: Jacopo Mondi
---
.../bindings/media/i2c/maxim,max9286.yaml | 53 ++-
1 file changed, 52 insertions(+), 1 deletion(-)
diff --git a/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
b/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
index ee16102fdfe7..480a491f3744 100644
--- a/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
+++ b/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
@@ -70,6 +70,24 @@ properties:
a remote serializer whose high-threshold noise immunity is not enabled
is 10 micro volts
+ maxim,gpio-poc:
+$ref: '/schemas/types.yaml#/definitions/uint32-array'
+minItems: 2
+maxItems: 2
+description: |
+ Identifier of gpio line that controls Power over Coax to the cameras and
+ the associated polarity flag (GPIO_ACTIVE_HIGH and GPIO_ACTIVE_LOW)
+ as defined in .
+
+ When the remote cameras power is controlled by one of the MAX9286 gpio
+ lines, this property has to be used to specify which line among the two
+ available ones controls the remote camera power enablement.
+
+ When this property is used it is not possible to register a gpio
+ controller as the gpio lines are controlled directly by the MAX9286 and
+ not available for consumers, nor the 'poc-supply' property should be
+ specified.
+
ports:
$ref: /schemas/graph.yaml#/properties/ports
@@ -182,7 +200,6 @@ required:
- reg
- ports
- i2c-mux
- - gpio-controller
additionalProperties: false
@@ -327,4 +344,38 @@ examples:
};
};
};
+
+ /*
+ * Example of a deserializer that controls the camera Power over Coax
+ * through one of its gpio lines.
+ */
+ gmsl-deserializer@6c {
+compatible = "maxim,max9286";
+reg = <0x6c>;
+enable-gpios = < 14 GPIO_ACTIVE_HIGH>;
+
+/*
+ * The remote camera power is controlled by MAX9286 GPIO line #0.
+ * No 'poc-supply' nor 'gpio-controller' are specified.
+ */
+maxim,gpio-poc = <0 GPIO_ACTIVE_LOW>;
+
+/*
+ * Do not describe connections as they're the same as in the previous
+ * example.
+ */
+ports {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ port@4 {
+reg = <4>;
+ };
+};
+
+i2c-mux {
+ #address-cells = <1>;
+ #size-cells = <0>;
+};
+ };
};
--
2.31.1
[PATCH v3 0/5] arm64: dts: renesas: Enable GMSL on R8A77970 V3M Eagle
Hello, this series follows https://patchwork.kernel.org/project/linux-renesas-soc/list/?series=448427 but implements a different way of handling the regulator-gpio_controller circular dependency which is hunting us. As suggested during review of v2 by Laurent, instead of using a gpio-hog to force the MAX9286 gpio line that controls the remote cameras power, this series introduces a new vendor property that allows to declare that the camera power is controlled by the MAX9286 chip, instead than relying on a canonical supply, which is impossible to establish as consumer of the gpio controller registered by the driver. The series introduces the new property and add support for parsing it in the driver. The DTS changes that follow are identical to v2, and comments there have not been addressed waiting for a validation of patches 1 and 2. Thanks j Jacopo Mondi (2): dt-bindings: media: max9286: Define 'maxim,gpio-poc' media: i2c: max9286: Use "maxim,gpio-poc" property Kieran Bingham (3): arm64: dts: renesas: eagle: Enable MAX9286 arm64: dts: renesas: eagle: Add GMSL .dtsi arm64: dts: renesas: eagle: Include eagle-gmsl .../bindings/media/i2c/maxim,max9286.yaml | 53 - arch/arm64/boot/dts/renesas/eagle-gmsl.dtsi | 186 ++ .../arm64/boot/dts/renesas/r8a77970-eagle.dts | 122 drivers/media/i2c/max9286.c | 125 +--- 4 files changed, 456 insertions(+), 30 deletions(-) create mode 100644 arch/arm64/boot/dts/renesas/eagle-gmsl.dtsi -- 2.31.1
Re: [PATCH v2 30/30] media: atmel: atmel-isc: add microchip-xisc driver
Hi Eugene
On Mon, Apr 12, 2021 at 02:15:32PM +, eugen.hris...@microchip.com wrote:
> On 4/12/21 4:41 PM, Jacopo Mondi wrote:
> > Hi Eugene,
> >
> > On Mon, Apr 12, 2021 at 12:37:41PM +, eugen.hris...@microchip.com wrote:
> >>> +static int xisc_parse_dt(struct device *dev, struct isc_device *isc)
> >>> +{
> >>> + struct device_node *np = dev->of_node;
> >>> + struct device_node *epn = NULL;
> >>> + struct isc_subdev_entity *subdev_entity;
> >>> + unsigned int flags;
> >>> + int ret;
> >>> + bool mipi_mode;
> >>> +
> >>> + INIT_LIST_HEAD(>subdev_entities);
> >>> +
> >>> + mipi_mode = of_property_read_bool(np, "microchip,mipi-mode");
> >>> +
> >>> + while (1) {
> >>> + struct v4l2_fwnode_endpoint v4l2_epn = { .bus_type = 0 };
> >>> +
> >>> + epn = of_graph_get_next_endpoint(np, epn);
> >>> + if (!epn)
> >>> + return 0;
> >>> +
> >>> + ret = v4l2_fwnode_endpoint_parse(of_fwnode_handle(epn),
> >>> +_epn);
> >>> + if (ret) {
> >>> + ret = -EINVAL;
> >>> + dev_err(dev, "Could not parse the endpoint\n");
> >>> + break;
> >>> + }
> >>> +
> >>> + subdev_entity = devm_kzalloc(dev, sizeof(*subdev_entity),
> >>> +GFP_KERNEL);
> >>> + if (!subdev_entity) {
> >>> + ret = -ENOMEM;
> >>> + break;
> >>> + }
> >>> + subdev_entity->epn = epn;
> >>> +
> >>> + flags = v4l2_epn.bus.parallel.flags;
> >>> +
> >>> + if (flags & V4L2_MBUS_HSYNC_ACTIVE_LOW)
> >>> + subdev_entity->pfe_cfg0 = ISC_PFE_CFG0_HPOL_LOW;
> >>> +
> >>> + if (flags & V4L2_MBUS_VSYNC_ACTIVE_LOW)
> >>> + subdev_entity->pfe_cfg0 |= ISC_PFE_CFG0_VPOL_LOW;
> >>> +
> >>> + if (flags & V4L2_MBUS_PCLK_SAMPLE_FALLING)
> >>> + subdev_entity->pfe_cfg0 |= ISC_PFE_CFG0_PPOL_LOW;
> >>> +
> >>> + if (v4l2_epn.bus_type == V4L2_MBUS_BT656)
> >>> + subdev_entity->pfe_cfg0 |= ISC_PFE_CFG0_CCIR_CRC |
> >>> + ISC_PFE_CFG0_CCIR656;
> >>
> >> Hi Jacopo,
> >>
> >> If I use the bus-type property for the 'port' , do I actually have to
> >> change something here ?
> >
> > You can set bus_type to the desired type, if it doesn't match the
> > 'bus-type' property you will have an immediate error and a more strict
> > check on the properties.
> >
> > You would likely:
> >
> > v4l2_epn.bus_type = V4L2_MBUS_PARALLEL;
> > ret = v4l2_fwnode_endpoint_parse()
> > if (!ret) {
> > /* It's parallel */
> > } else {
> > v4l2_epn.bus_type = V4L2_MBUS_BT656;
> > ret = v4l2_fwnode_endpoint_parse()
> > if (ret) {
> > /* Unsupported bus type: error out. */
> > }
> >
> > /* It's BT656 */
> > }
>
> if the v4l2_fwnode_endpoint_parse will already fill in the
> v4l2_epn.bus_type based on what is found in the 'bus-type' , why do I
> need to do this assumption-fail-assumption-fail scenario ?
Because you would know exactly what bus type you're parsing, as
v4l2_fwnode_endpoint_parse() would make sure the type you intend to
parse matches what the endpoint reports
if (vep->bus_type != V4L2_MBUS_UNKNOWN) {
if (mbus_type != V4L2_MBUS_UNKNOWN &&
vep->bus_type != mbus_type) {
pr_debug("expecting bus type %s\n",
v4l2_fwnode_mbus_type_to_string(vep->bus_type));
return -ENXIO;
}
I guess this is mostly useful if you want to specify default
values the v4l2_fwnode_endpoint. If you go with autoguessing, a
parallel endpoint without polarities flag specified can be identified
as BT656 if I'm not mistaken.
> Can't I simply check the value of v4l2_epn.bus_type , as I am doing it
> al
Re: [PATCH v2 30/30] media: atmel: atmel-isc: add microchip-xisc driver
Hi Eugene,
On Mon, Apr 12, 2021 at 12:37:41PM +, eugen.hris...@microchip.com wrote:
> > +static int xisc_parse_dt(struct device *dev, struct isc_device *isc)
> > +{
> > + struct device_node *np = dev->of_node;
> > + struct device_node *epn = NULL;
> > + struct isc_subdev_entity *subdev_entity;
> > + unsigned int flags;
> > + int ret;
> > + bool mipi_mode;
> > +
> > + INIT_LIST_HEAD(>subdev_entities);
> > +
> > + mipi_mode = of_property_read_bool(np, "microchip,mipi-mode");
> > +
> > + while (1) {
> > + struct v4l2_fwnode_endpoint v4l2_epn = { .bus_type = 0 };
> > +
> > + epn = of_graph_get_next_endpoint(np, epn);
> > + if (!epn)
> > + return 0;
> > +
> > + ret = v4l2_fwnode_endpoint_parse(of_fwnode_handle(epn),
> > +_epn);
> > + if (ret) {
> > + ret = -EINVAL;
> > + dev_err(dev, "Could not parse the endpoint\n");
> > + break;
> > + }
> > +
> > + subdev_entity = devm_kzalloc(dev, sizeof(*subdev_entity),
> > +GFP_KERNEL);
> > + if (!subdev_entity) {
> > + ret = -ENOMEM;
> > + break;
> > + }
> > + subdev_entity->epn = epn;
> > +
> > + flags = v4l2_epn.bus.parallel.flags;
> > +
> > + if (flags & V4L2_MBUS_HSYNC_ACTIVE_LOW)
> > + subdev_entity->pfe_cfg0 = ISC_PFE_CFG0_HPOL_LOW;
> > +
> > + if (flags & V4L2_MBUS_VSYNC_ACTIVE_LOW)
> > + subdev_entity->pfe_cfg0 |= ISC_PFE_CFG0_VPOL_LOW;
> > +
> > + if (flags & V4L2_MBUS_PCLK_SAMPLE_FALLING)
> > + subdev_entity->pfe_cfg0 |= ISC_PFE_CFG0_PPOL_LOW;
> > +
> > + if (v4l2_epn.bus_type == V4L2_MBUS_BT656)
> > + subdev_entity->pfe_cfg0 |= ISC_PFE_CFG0_CCIR_CRC |
> > + ISC_PFE_CFG0_CCIR656;
>
> Hi Jacopo,
>
> If I use the bus-type property for the 'port' , do I actually have to
> change something here ?
You can set bus_type to the desired type, if it doesn't match the
'bus-type' property you will have an immediate error and a more strict
check on the properties.
You would likely:
v4l2_epn.bus_type = V4L2_MBUS_PARALLEL;
ret = v4l2_fwnode_endpoint_parse()
if (!ret) {
/* It's parallel */
} else {
v4l2_epn.bus_type = V4L2_MBUS_BT656;
ret = v4l2_fwnode_endpoint_parse()
if (ret) {
/* Unsupported bus type: error out. */
}
/* It's BT656 */
}
Not the greatest API, but it's more robust.
> the v4l2_epn.bus_type won't be set automatically ? by the endpoint
> parser I mean.
Yes, that's what auto-discovery is, the endpoint parser tries to
deduce the bus type from the properties that are there specified. It
works, but leaves quite some ambiguity between ie PARALLEL and BT656
as some polarities might not be necessarily specified even for
PARALLEL bus types.
As I've said, there's still plenty of code that relies on
auto-discovery so I don't think this is blocking, also because making
bus-type mandatory on existing DTS is quite painful. Since this is a
new DTS you can consider this solution if you want to.
Thanks
j
>
> Thanks,
> Eugen
>
> > +
> > + if (mipi_mode)
> > + subdev_entity->pfe_cfg0 |= ISC_PFE_CFG0_MIPI;
> > +
> > + list_add_tail(_entity->list, >subdev_entities);
> > + }
> > + of_node_put(epn);
> > +
> > + return ret;
> > +}
> > +
> > +static int microchip_xisc_probe(struct platform_device *pdev)
> > +{
> > + struct device *dev = >dev;
> > + struct isc_device *isc;
> > + struct resource *res;
> > + void __iomem *io_base;
> > + struct isc_subdev_entity *subdev_entity;
> > + int irq;
> > + int ret;
> > + u32 ver;
> > +
> > + isc = devm_kzalloc(dev, sizeof(*isc), GFP_KERNEL);
> > + if (!isc)
> > + return -ENOMEM;
> > +
> > + platform_set_drvdata(pdev, isc);
> > + isc->dev = dev;
> > +
> > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > + io_base = devm_ioremap_resource(dev, res);
> > + if (IS_ERR(io_base))
> > + return PTR_ERR(io_base);
> > +
> > + isc->regmap = devm_regmap_init_mmio(dev, io_base, _regmap_config);
> > + if (IS_ERR(isc->regmap)) {
> > + ret = PTR_ERR(isc->regmap);
> > + dev_err(dev, "failed to init register map: %d\n", ret);
> > + return ret;
> > + }
> > +
> > + irq = platform_get_irq(pdev, 0);
> > + if (irq < 0)
> > + return irq;
> > +
> > + ret = devm_request_irq(dev, irq, isc_interrupt, 0,
> > + "microchip-sama7g5-xisc", isc);
> > + if (ret < 0) {
> > + dev_err(dev, "can't register ISR for IRQ %u (ret=%i)\n",
> > + irq,
Re: [PATCH v2 28/30] dt-bindings: media: atmel: add microchip-xisc binding
Hi Eugene, On Mon, Apr 12, 2021 at 10:12:22AM +, eugen.hris...@microchip.com wrote: > On 4/12/21 12:57 PM, Jacopo Mondi wrote: > > EXTERNAL EMAIL: Do not click links or open attachments unless you know the > > content is safe > > > > Hi Eugene, > > > > On Mon, Apr 05, 2021 at 06:51:03PM +0300, Eugen Hristev wrote: > >> Add bindings for the microchip xisc, a driver based on atmel-isc. > >> It shares common code with atmel-isc, but the xisc is the next generation > >> ISC which is present on sama7g5 product. > >> It has an enhanced pipeline, additional modules, formats, and it supports > >> not only parallel sensors, but also serial sensors, by connecting to a > >> demux > >> endpoint present on sama7g5. > >> One of the key points for creating a new binding is the clocking scheme, as > >> atmel-isc requires 3 mandatory clocks, the microchip-xisc requires a single > >> input clock. > >> > >> Signed-off-by: Eugen Hristev > >> --- > >> > >> Hello Rob, all, > >> > >> I did not convert this yet to yaml because I would like first your feedback > >> if the binding is good. > >> If it's fine I will convert both this new binding and the old atmel-isc > >> to yaml. > >> > >> Thanks for your feedback, > >> Eugen > >> > >> .../bindings/media/microchip-xisc.txt | 64 +++ > >> 1 file changed, 64 insertions(+) > >> create mode 100644 > >> Documentation/devicetree/bindings/media/microchip-xisc.txt > >> > >> diff --git a/Documentation/devicetree/bindings/media/microchip-xisc.txt > >> b/Documentation/devicetree/bindings/media/microchip-xisc.txt > >> new file mode 100644 > >> index ..080a357ed84d > >> --- /dev/null > >> +++ b/Documentation/devicetree/bindings/media/microchip-xisc.txt > >> @@ -0,0 +1,64 @@ > >> +Microchip eXtended Image Sensor Controller (XISC) > >> +-- > >> + > >> +Required properties for XISC: > >> +- compatible > >> + Must be "microchip,sama7g5-xisc". > >> +- reg > >> + Physical base address and length of the registers set for the device. > >> +- interrupts > >> + Should contain IRQ line for the XISC. > >> +- clocks > >> + List of clock specifiers, corresponding to entries in > >> + the clock-names property; > >> + Please refer to clock-bindings.txt. > >> +- clock-names > >> + Required elements: "hclock". > >> + This is the clock that clocks the sensor controller, and is usually > >> + fed from the clock tree. It is used for the internal controller > >> logic. > >> +- #clock-cells > >> + Should be 0. > >> +- clock-output-names > >> + Should be "isc-mck". > >> +- pinctrl-names, pinctrl-0 > >> + Please refer to pinctrl-bindings.txt. > >> + > >> +Optional properties for XISC: > >> +- microchip,mipi-mode; > >> + As the XISC is usually connected to a demux/bridge, the XISC receives > >> + the same type of input, however, it should be aware of the type of > >> + signals received. The mipi-mode enables different internal handling > >> + of the data and clock lines. > > > > What does 'mipi-mode' do to a component that has an parallel receiver ? > > Actually, this indeed has a parallel receiver, but it's only inside the > SoC. The other end of the parallel connection is a demuxer/bridge. This > demuxer will take the input from either a real parallel sensor or a CSI2 > stream. > Even if the pixels are then converted into a parallel stream, it looks > like the pixel data has a bit of different constrains in term of hold > and setup time, and other electrical characteristics inside the SoC. > The XISC hardware designer decided to leave a bit in the user interface > called 'mipi-mode' , and by setting this, the capture interface of the > XISC is better adapted to a demuxed stream from a CSI2, rather than > adapted to a stream coming from a parallel sensor directly. > > I am not sure I explained it right, but this is what I understand, when > I asked the hardware design about it. > > So we have to manually set this bit if we have the demuxer deserializing > the CSI2 pixels or they are connected to a parallel sensor. > The XISC has no way of telling which is the correct setup, and from the > demuxer perspective,
Re: [PATCH v2 28/30] dt-bindings: media: atmel: add microchip-xisc binding
Hi Eugene,
On Mon, Apr 05, 2021 at 06:51:03PM +0300, Eugen Hristev wrote:
> Add bindings for the microchip xisc, a driver based on atmel-isc.
> It shares common code with atmel-isc, but the xisc is the next generation
> ISC which is present on sama7g5 product.
> It has an enhanced pipeline, additional modules, formats, and it supports
> not only parallel sensors, but also serial sensors, by connecting to a demux
> endpoint present on sama7g5.
> One of the key points for creating a new binding is the clocking scheme, as
> atmel-isc requires 3 mandatory clocks, the microchip-xisc requires a single
> input clock.
>
> Signed-off-by: Eugen Hristev
> ---
>
> Hello Rob, all,
>
> I did not convert this yet to yaml because I would like first your feedback
> if the binding is good.
> If it's fine I will convert both this new binding and the old atmel-isc
> to yaml.
>
> Thanks for your feedback,
> Eugen
>
> .../bindings/media/microchip-xisc.txt | 64 +++
> 1 file changed, 64 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/media/microchip-xisc.txt
>
> diff --git a/Documentation/devicetree/bindings/media/microchip-xisc.txt
> b/Documentation/devicetree/bindings/media/microchip-xisc.txt
> new file mode 100644
> index ..080a357ed84d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/microchip-xisc.txt
> @@ -0,0 +1,64 @@
> +Microchip eXtended Image Sensor Controller (XISC)
> +--
> +
> +Required properties for XISC:
> +- compatible
> + Must be "microchip,sama7g5-xisc".
> +- reg
> + Physical base address and length of the registers set for the device.
> +- interrupts
> + Should contain IRQ line for the XISC.
> +- clocks
> + List of clock specifiers, corresponding to entries in
> + the clock-names property;
> + Please refer to clock-bindings.txt.
> +- clock-names
> + Required elements: "hclock".
> + This is the clock that clocks the sensor controller, and is usually
> + fed from the clock tree. It is used for the internal controller logic.
> +- #clock-cells
> + Should be 0.
> +- clock-output-names
> + Should be "isc-mck".
> +- pinctrl-names, pinctrl-0
> + Please refer to pinctrl-bindings.txt.
> +
> +Optional properties for XISC:
> +- microchip,mipi-mode;
> + As the XISC is usually connected to a demux/bridge, the XISC receives
> + the same type of input, however, it should be aware of the type of
> + signals received. The mipi-mode enables different internal handling
> + of the data and clock lines.
What does 'mipi-mode' do to a component that has an parallel receiver ?
> +
> +XISC supports a single port node with internal parallel bus.
> +It should contain one 'port' child node with child 'endpoint' node.
> +Please refer to the bindings defined in
> +Documentation/devicetree/bindings/media/video-interfaces.txt.
> +
> +This endpoint has to be connected to a bridge that acts as a demux from
> either
> +a serial interface or acts as a simple direct bridge to a parallel sensor.
> +
> +Example:
> +xisc: xisc@e1408000 {
> + compatible = "microchip,sama7g5-isc";
> + reg = <0xe1408000 0x2000>;
> + interrupts = ;
> + #address-cells = <1>;
> + #size-cells = <0>;
> + clocks = < PMC_TYPE_PERIPHERAL 56>;
> + clock-names = "hclock";
> + #clock-cells = <0>;
> + clock-output-names = "isc-mck";
> + microchip,mipi-mode;
> +
> + port@1 {
> + reg = <1>;
> + xisc_in: endpoint {
> + bus-width = <12>;
> + hsync-active = <1>;
> + vsync-active = <1>;
> + remote-endpoint = <_out>;
nit: indentation
Have you consided using bus-type property ? As that's a new binding I
would consider making it mandatory, and to modify the DT parsinga
routine accordingly to remove auto-guessing, which according to my
understanding is almost 'deprecated' ?
> + };
> + };
> +};
> +
> --
> 2.25.1
>
Re: [PATCH v2 04/30] media: atmel: atmel-isc: specialize max width and max height
Ups,
On Mon, Apr 12, 2021 at 11:43:12AM +0200, Jacopo Mondi wrote:
> Hi Eugene,
>
> On Mon, Apr 05, 2021 at 06:50:39PM +0300, Eugen Hristev wrote:
> > Move the max width and max height constants to the product specific driver
> > and have them in the device struct.
> >
> > Signed-off-by: Eugen Hristev
> > ---
> > drivers/media/platform/atmel/atmel-isc-base.c | 28 +--
> > drivers/media/platform/atmel/atmel-isc.h | 9 --
> > .../media/platform/atmel/atmel-sama5d2-isc.c | 7 +++--
> > 3 files changed, 25 insertions(+), 19 deletions(-)
> >
> > diff --git a/drivers/media/platform/atmel/atmel-isc-base.c
> > b/drivers/media/platform/atmel/atmel-isc-base.c
> > index 45fc8dbb7943..350076dd029a 100644
> > --- a/drivers/media/platform/atmel/atmel-isc-base.c
> > +++ b/drivers/media/platform/atmel/atmel-isc-base.c
> > @@ -1204,8 +1204,8 @@ static void isc_try_fse(struct isc_device *isc,
> > * just use the maximum ISC can receive.
> > */
> > if (ret) {
> > - pad_cfg->try_crop.width = ISC_MAX_SUPPORT_WIDTH;
> > - pad_cfg->try_crop.height = ISC_MAX_SUPPORT_HEIGHT;
> > + pad_cfg->try_crop.width = isc->max_width;
> > + pad_cfg->try_crop.height = isc->max_height;
> > } else {
> > pad_cfg->try_crop.width = fse.max_width;
> > pad_cfg->try_crop.height = fse.max_height;
> > @@ -1282,10 +1282,10 @@ static int isc_try_fmt(struct isc_device *isc,
> > struct v4l2_format *f,
> > isc->try_config.sd_format = sd_fmt;
> >
> > /* Limit to Atmel ISC hardware capabilities */
> > - if (pixfmt->width > ISC_MAX_SUPPORT_WIDTH)
> > - pixfmt->width = ISC_MAX_SUPPORT_WIDTH;
> > - if (pixfmt->height > ISC_MAX_SUPPORT_HEIGHT)
> > - pixfmt->height = ISC_MAX_SUPPORT_HEIGHT;
> > + if (pixfmt->width > isc->max_width)
> > + pixfmt->width = isc->max_width;
> > + if (pixfmt->height > isc->max_height)
> > + pixfmt->height = isc->max_height;
> >
> > /*
> > * The mbus format is the one the subdev outputs.
> > @@ -1327,10 +1327,10 @@ static int isc_try_fmt(struct isc_device *isc,
> > struct v4l2_format *f,
> > v4l2_fill_pix_format(pixfmt, );
> >
> > /* Limit to Atmel ISC hardware capabilities */
> > - if (pixfmt->width > ISC_MAX_SUPPORT_WIDTH)
> > - pixfmt->width = ISC_MAX_SUPPORT_WIDTH;
> > - if (pixfmt->height > ISC_MAX_SUPPORT_HEIGHT)
> > - pixfmt->height = ISC_MAX_SUPPORT_HEIGHT;
> > + if (pixfmt->width > isc->max_width)
> > + pixfmt->width = isc->max_width;
> > + if (pixfmt->height > isc->max_height)
> > + pixfmt->height = isc->max_height;
>
> What happens if the sensor sends a frame larger that the ISC max
> supported sizes ?
>
I meant to ask this question on the previous patch :/
> >
> > pixfmt->field = V4L2_FIELD_NONE;
> > pixfmt->bytesperline = (pixfmt->width * isc->try_config.bpp) >> 3;
> > @@ -1368,10 +1368,10 @@ static int isc_set_fmt(struct isc_device *isc,
> > struct v4l2_format *f)
> > return ret;
> >
> > /* Limit to Atmel ISC hardware capabilities */
> > - if (pixfmt->width > ISC_MAX_SUPPORT_WIDTH)
> > - pixfmt->width = ISC_MAX_SUPPORT_WIDTH;
> > - if (pixfmt->height > ISC_MAX_SUPPORT_HEIGHT)
> > - pixfmt->height = ISC_MAX_SUPPORT_HEIGHT;
> > + if (f->fmt.pix.width > isc->max_width)
> > + f->fmt.pix.width = isc->max_width;
> > + if (f->fmt.pix.height > isc->max_height)
> > + f->fmt.pix.height = isc->max_height;
> >
> > isc->fmt = *f;
> >
> > diff --git a/drivers/media/platform/atmel/atmel-isc.h
> > b/drivers/media/platform/atmel/atmel-isc.h
> > index 8d81d9967ad2..6becc6c3aaf0 100644
> > --- a/drivers/media/platform/atmel/atmel-isc.h
> > +++ b/drivers/media/platform/atmel/atmel-isc.h
> > @@ -10,9 +10,6 @@
> > */
> > #ifndef _ATMEL_ISC_H_
> >
> > -#define ISC_MAX_SUPPORT_WIDTH 2592
> > -#define ISC_MAX_SUPPORT_HEIGHT 1944
> > -
> > #define ISC_CLK_MAX_DIV255
> >
> > enum isc_clk_id {
> > @@ -191,6 +188,9 @@ struct isc_ctrls {
> > * @gamma_table: pointer to the table with gamma values, has
> > * gamma_max sets of GAMMA_ENTRIES entries each
&
Re: [PATCH v2 05/30] media: atmel: atmel-isc: specialize dma cfg
Hi Eugene
On Mon, Apr 05, 2021 at 06:50:40PM +0300, Eugen Hristev wrote:
> The dma configuration (DCFG) is specific to the product.
> Move this configuration in the product specific driver, and add the
> field inside the driver struct.
Do you plan to match on different compatible values ?
As in that case you could retrieve platform-specific data with
of_device_get_match_data() maybe ? Just pointing it out if it's anyway
useful to you.
Thanks
j
>
> Signed-off-by: Eugen Hristev
> ---
> drivers/media/platform/atmel/atmel-isc-base.c| 3 +--
> drivers/media/platform/atmel/atmel-isc.h | 2 ++
> drivers/media/platform/atmel/atmel-sama5d2-isc.c | 3 +++
> 3 files changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/media/platform/atmel/atmel-isc-base.c
> b/drivers/media/platform/atmel/atmel-isc-base.c
> index 350076dd029a..ff40ee2e2759 100644
> --- a/drivers/media/platform/atmel/atmel-isc-base.c
> +++ b/drivers/media/platform/atmel/atmel-isc-base.c
> @@ -716,8 +716,7 @@ static int isc_configure(struct isc_device *isc)
> rlp_mode = isc->config.rlp_cfg_mode;
> pipeline = isc->config.bits_pipeline;
>
> - dcfg = isc->config.dcfg_imode |
> -ISC_DCFG_YMBSIZE_BEATS8 | ISC_DCFG_CMBSIZE_BEATS8;
> + dcfg = isc->config.dcfg_imode | isc->dcfg;
>
> pfe_cfg0 |= subdev->pfe_cfg0 | ISC_PFE_CFG0_MODE_PROGRESSIVE;
> mask = ISC_PFE_CFG0_BPS_MASK | ISC_PFE_CFG0_HPOL_LOW |
> diff --git a/drivers/media/platform/atmel/atmel-isc.h
> b/drivers/media/platform/atmel/atmel-isc.h
> index 6becc6c3aaf0..d14ae096fbf6 100644
> --- a/drivers/media/platform/atmel/atmel-isc.h
> +++ b/drivers/media/platform/atmel/atmel-isc.h
> @@ -150,6 +150,7 @@ struct isc_ctrls {
> * @hclock: Hclock clock input (refer datasheet)
> * @ispck: iscpck clock (refer datasheet)
> * @isc_clks:ISC clocks
> + * @dcfg:DMA master configuration, architecture dependent
> *
> * @dev: Registered device driver
> * @v4l2_dev:v4l2 registered device
> @@ -197,6 +198,7 @@ struct isc_device {
> struct clk *hclock;
> struct clk *ispck;
> struct isc_clk isc_clks[2];
> + u32 dcfg;
>
> struct device *dev;
> struct v4l2_device v4l2_dev;
> diff --git a/drivers/media/platform/atmel/atmel-sama5d2-isc.c
> b/drivers/media/platform/atmel/atmel-sama5d2-isc.c
> index f8d1c8ba99b3..6d9942dcd7c1 100644
> --- a/drivers/media/platform/atmel/atmel-sama5d2-isc.c
> +++ b/drivers/media/platform/atmel/atmel-sama5d2-isc.c
> @@ -198,6 +198,9 @@ static int atmel_isc_probe(struct platform_device *pdev)
> isc->max_width = ISC_SAMA5D2_MAX_SUPPORT_WIDTH;
> isc->max_height = ISC_SAMA5D2_MAX_SUPPORT_HEIGHT;
>
> + /* sama5d2-isc - 8 bits per beat */
> + isc->dcfg = ISC_DCFG_YMBSIZE_BEATS8 | ISC_DCFG_CMBSIZE_BEATS8;
> +
> ret = isc_pipeline_init(isc);
> if (ret)
> return ret;
> --
> 2.25.1
>
Re: [PATCH v2 04/30] media: atmel: atmel-isc: specialize max width and max height
Hi Eugene,
On Mon, Apr 05, 2021 at 06:50:39PM +0300, Eugen Hristev wrote:
> Move the max width and max height constants to the product specific driver
> and have them in the device struct.
>
> Signed-off-by: Eugen Hristev
> ---
> drivers/media/platform/atmel/atmel-isc-base.c | 28 +--
> drivers/media/platform/atmel/atmel-isc.h | 9 --
> .../media/platform/atmel/atmel-sama5d2-isc.c | 7 +++--
> 3 files changed, 25 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/media/platform/atmel/atmel-isc-base.c
> b/drivers/media/platform/atmel/atmel-isc-base.c
> index 45fc8dbb7943..350076dd029a 100644
> --- a/drivers/media/platform/atmel/atmel-isc-base.c
> +++ b/drivers/media/platform/atmel/atmel-isc-base.c
> @@ -1204,8 +1204,8 @@ static void isc_try_fse(struct isc_device *isc,
>* just use the maximum ISC can receive.
>*/
> if (ret) {
> - pad_cfg->try_crop.width = ISC_MAX_SUPPORT_WIDTH;
> - pad_cfg->try_crop.height = ISC_MAX_SUPPORT_HEIGHT;
> + pad_cfg->try_crop.width = isc->max_width;
> + pad_cfg->try_crop.height = isc->max_height;
> } else {
> pad_cfg->try_crop.width = fse.max_width;
> pad_cfg->try_crop.height = fse.max_height;
> @@ -1282,10 +1282,10 @@ static int isc_try_fmt(struct isc_device *isc, struct
> v4l2_format *f,
> isc->try_config.sd_format = sd_fmt;
>
> /* Limit to Atmel ISC hardware capabilities */
> - if (pixfmt->width > ISC_MAX_SUPPORT_WIDTH)
> - pixfmt->width = ISC_MAX_SUPPORT_WIDTH;
> - if (pixfmt->height > ISC_MAX_SUPPORT_HEIGHT)
> - pixfmt->height = ISC_MAX_SUPPORT_HEIGHT;
> + if (pixfmt->width > isc->max_width)
> + pixfmt->width = isc->max_width;
> + if (pixfmt->height > isc->max_height)
> + pixfmt->height = isc->max_height;
>
> /*
>* The mbus format is the one the subdev outputs.
> @@ -1327,10 +1327,10 @@ static int isc_try_fmt(struct isc_device *isc, struct
> v4l2_format *f,
> v4l2_fill_pix_format(pixfmt, );
>
> /* Limit to Atmel ISC hardware capabilities */
> - if (pixfmt->width > ISC_MAX_SUPPORT_WIDTH)
> - pixfmt->width = ISC_MAX_SUPPORT_WIDTH;
> - if (pixfmt->height > ISC_MAX_SUPPORT_HEIGHT)
> - pixfmt->height = ISC_MAX_SUPPORT_HEIGHT;
> + if (pixfmt->width > isc->max_width)
> + pixfmt->width = isc->max_width;
> + if (pixfmt->height > isc->max_height)
> + pixfmt->height = isc->max_height;
What happens if the sensor sends a frame larger that the ISC max
supported sizes ?
>
> pixfmt->field = V4L2_FIELD_NONE;
> pixfmt->bytesperline = (pixfmt->width * isc->try_config.bpp) >> 3;
> @@ -1368,10 +1368,10 @@ static int isc_set_fmt(struct isc_device *isc, struct
> v4l2_format *f)
> return ret;
>
> /* Limit to Atmel ISC hardware capabilities */
> - if (pixfmt->width > ISC_MAX_SUPPORT_WIDTH)
> - pixfmt->width = ISC_MAX_SUPPORT_WIDTH;
> - if (pixfmt->height > ISC_MAX_SUPPORT_HEIGHT)
> - pixfmt->height = ISC_MAX_SUPPORT_HEIGHT;
> + if (f->fmt.pix.width > isc->max_width)
> + f->fmt.pix.width = isc->max_width;
> + if (f->fmt.pix.height > isc->max_height)
> + f->fmt.pix.height = isc->max_height;
>
> isc->fmt = *f;
>
> diff --git a/drivers/media/platform/atmel/atmel-isc.h
> b/drivers/media/platform/atmel/atmel-isc.h
> index 8d81d9967ad2..6becc6c3aaf0 100644
> --- a/drivers/media/platform/atmel/atmel-isc.h
> +++ b/drivers/media/platform/atmel/atmel-isc.h
> @@ -10,9 +10,6 @@
> */
> #ifndef _ATMEL_ISC_H_
>
> -#define ISC_MAX_SUPPORT_WIDTH 2592
> -#define ISC_MAX_SUPPORT_HEIGHT 1944
> -
> #define ISC_CLK_MAX_DIV 255
>
> enum isc_clk_id {
> @@ -191,6 +188,9 @@ struct isc_ctrls {
> * @gamma_table: pointer to the table with gamma values, has
> * gamma_max sets of GAMMA_ENTRIES entries each
> * @gamma_max: maximum number of sets of inside the gamma_table
> + *
> + * @max_width: maximum frame width, dependent on the internal
> RAM
> + * @max_height: maximum frame height, dependent on the internal
> RAM
> */
> struct isc_device {
> struct regmap *regmap;
> @@ -254,6 +254,9 @@ struct isc_device {
> /* pointer to the defined gamma table */
> const u32 (*gamma_table)[GAMMA_ENTRIES];
> u32 gamma_max;
> +
> + u32 max_width;
> + u32 max_height;
> };
>
> extern struct isc_format formats_list[];
> diff --git a/drivers/media/platform/atmel/atmel-sama5d2-isc.c
> b/drivers/media/platform/atmel/atmel-sama5d2-isc.c
> index f45d8b96bfb8..f8d1c8ba99b3 100644
> --- a/drivers/media/platform/atmel/atmel-sama5d2-isc.c
> +++ b/drivers/media/platform/atmel/atmel-sama5d2-isc.c
> @@ -49,8 +49,8 @@
> #include "atmel-isc-regs.h"
> #include "atmel-isc.h"
[PATCH v4 14/17] media: i2c: rdacm20: Check return values
The camera module initialization routine does not check the return value of a few functions. Fix that. Reviewed-by: Kieran Bingham Reviewed-by: Laurent Pinchart Signed-off-by: Jacopo Mondi --- drivers/media/i2c/rdacm20.c | 13 + 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/drivers/media/i2c/rdacm20.c b/drivers/media/i2c/rdacm20.c index f1819bfa165d..e6fed86a3281 100644 --- a/drivers/media/i2c/rdacm20.c +++ b/drivers/media/i2c/rdacm20.c @@ -466,11 +466,16 @@ static int rdacm20_initialize(struct rdacm20_device *dev) * Ensure that we have a good link configuration before attempting to * identify the device. */ - max9271_configure_i2c(>serializer, MAX9271_I2CSLVSH_469NS_234NS | - MAX9271_I2CSLVTO_1024US | - MAX9271_I2CMSTBT_105KBPS); + ret = max9271_configure_i2c(>serializer, + MAX9271_I2CSLVSH_469NS_234NS | + MAX9271_I2CSLVTO_1024US | + MAX9271_I2CMSTBT_105KBPS); + if (ret) + return ret; - max9271_configure_gmsl_link(>serializer); + ret = max9271_configure_gmsl_link(>serializer); + if (ret) + return ret; ret = max9271_verify_id(>serializer); if (ret < 0) -- 2.31.1
[PATCH v4 15/17] media: i2c: rdacm20: Re-work ov10635 reset
The OV10635 image sensor embedded in the camera module is currently reset after the MAX9271 initialization with two long delays that were most probably not correctly characterized. Re-work the image sensor reset procedure by holding the chip in reset during the MAX9271 configuration, removing the long sleep delays and only wait after the chip exits from reset for 350-500 microseconds interval, which is larger than the minimum (2048 * (1 / XVCLK)) timeout characterized in the chip manual. Reviewed-by: Kieran Bingham Reviewed-by: Laurent Pinchart Signed-off-by: Jacopo Mondi --- drivers/media/i2c/rdacm20.c | 29 + 1 file changed, 17 insertions(+), 12 deletions(-) diff --git a/drivers/media/i2c/rdacm20.c b/drivers/media/i2c/rdacm20.c index e6fed86a3281..cb725c2778c0 100644 --- a/drivers/media/i2c/rdacm20.c +++ b/drivers/media/i2c/rdacm20.c @@ -473,6 +473,19 @@ static int rdacm20_initialize(struct rdacm20_device *dev) if (ret) return ret; + /* +* Hold OV10635 in reset during max9271 configuration. The reset signal +* has to be asserted for at least 200 microseconds. +*/ + ret = max9271_enable_gpios(>serializer, MAX9271_GPIO1OUT); + if (ret) + return ret; + + ret = max9271_clear_gpios(>serializer, MAX9271_GPIO1OUT); + if (ret) + return ret; + usleep_range(200, 500); + ret = max9271_configure_gmsl_link(>serializer); if (ret) return ret; @@ -487,22 +500,14 @@ static int rdacm20_initialize(struct rdacm20_device *dev) dev->serializer.client->addr = dev->addrs[0]; /* -* Reset the sensor by cycling the OV10635 reset signal connected to the -* MAX9271 GPIO1 and verify communication with the OV10635. +* Release ov10635 from reset and initialize it. The image sensor +* requires at least 2048 XVCLK cycles (85 micro-seconds at 24MHz) +* before being available. Stay safe and wait up to 500 micro-seconds. */ - ret = max9271_enable_gpios(>serializer, MAX9271_GPIO1OUT); - if (ret) - return ret; - - ret = max9271_clear_gpios(>serializer, MAX9271_GPIO1OUT); - if (ret) - return ret; - usleep_range(1, 15000); - ret = max9271_set_gpios(>serializer, MAX9271_GPIO1OUT); if (ret) return ret; - usleep_range(1, 15000); + usleep_range(100, 500); again: ret = ov10635_read16(dev, OV10635_PID); -- 2.31.1
[PATCH v4 17/17] media: gmsl: Reimplement initialization sequence
The current probe() procedure of the RDACM20 and RDACM21 GMSL cameras is
performed with the embedded MAX9271 serializer's noise immunity
threshold disabled. Once the camera has been initialized by probing the
embedded chips, the threshold is enabled and then compensated on the
deserializer's side by increasing the reverse channel signal amplitude
once all cameras have bound.
The probe routine is thus run without noise immunity activated which
in noisy environment conditions makes the probe sequence less reliable as
the chips configuration requires a relatively high amount of i2c
transactions.
Break chip initialization in two:
- At probe time only configure the serializer's reverse channel with
noise immunity activated, to reduce the number of transactions
performed without noise immunity protection enabled
- Move the chips initialization to the .init() core subdev operation
called by the deserializer after all camera have probed and
have increased their noise immunity threshold
The initialization routine looks like the following:
MAX9286 RDACM20/21
probe()
|
-> |
probe() {
enable_threshold()
}
|<|
v4l2 async bound {
compensate_amplitude()
call subdev init()
|>|
init() {
access camera registers()
}
|<---
}
Reviewed-by: Laurent Pinchart
Reviewed-by: Kieran Bingham
Signed-off-by: Jacopo Mondi
---
drivers/media/i2c/max9286.c | 19 +---
drivers/media/i2c/rdacm20.c | 59 ++---
drivers/media/i2c/rdacm21.c | 59 ++---
3 files changed, 87 insertions(+), 50 deletions(-)
diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
index e1c7173f2d00..aafe55f30a0a 100644
--- a/drivers/media/i2c/max9286.c
+++ b/drivers/media/i2c/max9286.c
@@ -563,17 +563,28 @@ static int max9286_notify_bound(struct
v4l2_async_notifier *notifier,
if (priv->bound_sources != priv->source_mask)
return 0;
+ /*
+* Initialize all the remote camera. Increase the channel amplitude
+* to compensate for the remote noise immunity threshold.
+*/
+ max9286_reverse_channel_setup(priv, MAX9286_REV_AMP_HIGH);
+ for_each_source(priv, source) {
+ ret = v4l2_subdev_call(source->sd, core, init, 0);
+ if (ret) {
+ dev_err(>client->dev,
+ "Failed to initialize camera device %u\n",
+ index);
+ return ret;
+ }
+ }
+
/*
* All enabled sources have probed and enabled their reverse control
* channels:
-*
-* - Increase the reverse channel amplitude to compensate for the
-* remote ends high threshold
* - Verify all configuration links are properly detected
* - Disable auto-ack as communication on the control channel are now
* stable.
*/
- max9286_reverse_channel_setup(priv, MAX9286_REV_AMP_HIGH);
max9286_check_config_link(priv, priv->source_mask);
max9286_configure_i2c(priv, false);
diff --git a/drivers/media/i2c/rdacm20.c b/drivers/media/i2c/rdacm20.c
index cb725c2778c0..ce3e05ca6389 100644
--- a/drivers/media/i2c/rdacm20.c
+++ b/drivers/media/i2c/rdacm20.c
@@ -435,33 +435,12 @@ static int rdacm20_get_fmt(struct v4l2_subdev *sd,
return 0;
}
-static const struct v4l2_subdev_video_ops rdacm20_video_ops = {
- .s_stream = rdacm20_s_stream,
-};
-
-static const struct v4l2_subdev_pad_ops rdacm20_subdev_pad_ops = {
- .enum_mbus_code = rdacm20_enum_mbus_code,
- .get_fmt= rdacm20_get_fmt,
- .set_fmt= rdacm20_get_fmt,
-};
-
-static const struct v4l2_subdev_ops rdacm20_subdev_ops = {
- .video = _video_ops,
- .pad= _subdev_pad_ops,
-};
-
-static int rdacm20_initialize(struct rdacm20_device *dev)
+static int rdacm20_init(struct v4l2_subdev *sd, unsigned int val)
{
+ struct rdacm20_device *dev = sd_to_rdacm20(sd);
unsigned int retry = 3;
int ret;
- max9271_wake_up(>serializer);
-
- /* Serial link disabled during config as it needs a valid pixel clock.
*/
- ret = max9271_set_serial_link(>serializer, false);
- if (ret)
- return ret;
-
/*
* Ensure that we have a good link configuration before attempting to
* identify the device.
@@ -549,6 +528,40 @@ static int
[PATCH v4 16/17] media: v4l2-subdev: De-deprecate init() subdev op
The init() subdev core operation is deemed to be deprecated for new
subdevice drivers. However it could prove useful for complex
architectures to defer operation that require access to the
communication bus if said bus is not available (or fully configured)
at the time when the subdevice probe() function is run.
As an example, the GMSL architecture requires the GMSL configuration
link to be configured on the host side after the remote subdevice
has completed its probe function. After the configuration on the host
side has been performed, the subdevice registers can be accessed through
the communication bus.
In particular:
HOSTREMOTE
probe()
|
-> |
probe() {
bus config()
}
|<|
v4l2 async bound {
bus config()
call subdev init()
|>|
init() {
access register on the bus()
}
|<---
}
In the GMSL use case the bus configuration requires the enablement of the
noise immunity threshold on the remote side which ensures reliability
of communications in electrically noisy environments. After the subdevice
has enabled the threshold at the end of its probe() sequence the host
side shall compensate it with an higher signal amplitude. Once this
sequence has completed the bus can be accessed with noise protection
enabled and all the operations that require a considerable number of
transactions on the bus (such as the image sensor configuration
sequence) are run in the subdevice init() operation implementation.
Signed-off-by: Jacopo Mondi
---
include/media/v4l2-subdev.h | 15 ---
1 file changed, 12 insertions(+), 3 deletions(-)
diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
index d0e9a5bdb08b..3068d9940669 100644
--- a/include/media/v4l2-subdev.h
+++ b/include/media/v4l2-subdev.h
@@ -148,9 +148,18 @@ struct v4l2_subdev_io_pin_config {
* each pin being configured. This function could be called at times
* other than just subdevice initialization.
*
- * @init: initialize the sensor registers to some sort of reasonable default
- * values. Do not use for new drivers and should be removed in existing
- * drivers.
+ * @init: initialize the subdevice registers to some sort of reasonable default
+ * values. Do not use for new drivers (and should be removed in existing
+ * ones) for regular architectures where the image sensor is connected to
+ * the host receiver. For more complex architectures where the subdevice
+ * initialization should be deferred to the completion of the probe
+ * sequence of some intermediate component, or the communication bus
+ * requires configurations on the host side that depend on the completion
+ * of the probe sequence of the remote subdevices, the usage of this
+ * operation could be considered to allow the devices along the pipeline to
+ * probe and register in the media graph and to defer any operation that
+ * require actual access to the communication bus to their init() function
+ * implementation.
*
* @load_fw: load firmware.
*
--
2.31.1
[PATCH v4 10/17] media: i2c: rdacm21: Power up OV10640 before OV490
The current RDACM21 initialization routine powers up the OV10640 image
sensor after the OV490 ISP. The ISP is programmed with a firmare loaded
from an embedded serial flash that (most probably) tries to interact and
program also the image sensor connected to the ISP.
As described in commit ("media: i2c: rdacm21: Fix OV10640 powerup") the
image sensor powerdown signal is kept high by an internal pull up
resistor and occasionally fails to startup correctly if the powerdown
line is not asserted explicitely. Failures in the OV10640 startup causes
the OV490 firmware to fail to boot correctly resulting in the camera
module initialization to fail consequentially.
Fix this by powering up the OV10640 image sensor before testing the
OV490 firmware boot completion, by splitting the ov10640_initialize()
function in an ov10640_power_up() one and an ov10640_check_id() one.
Also make sure the OV10640 identification procedure gives enough time to
the image sensor to resume after the programming phase performed by the
OV490 firmware by repeating the ID read procedure.
This commit fixes a sporadic start-up error triggered by a failure to
detect the OV490 firmware boot completion:
rdacm21 8-0054: Timeout waiting for firmware boot
Fixes: a59f853b3b4b ("media: i2c: Add driver for RDACM21 camera module")
Signed-off-by: Jacopo Mondi
---
drivers/media/i2c/rdacm21.c | 46 ++---
1 file changed, 32 insertions(+), 14 deletions(-)
diff --git a/drivers/media/i2c/rdacm21.c b/drivers/media/i2c/rdacm21.c
index 7c0a4a84340a..43c41cb800a4 100644
--- a/drivers/media/i2c/rdacm21.c
+++ b/drivers/media/i2c/rdacm21.c
@@ -69,6 +69,7 @@
#define OV490_ISP_VSIZE_LOW0x80820062
#define OV490_ISP_VSIZE_HIGH 0x80820063
+#define OV10640_PID_TIMEOUT20
#define OV10640_ID_HIGH0xa6
#define OV10640_CHIP_ID0x300a
#define OV10640_PIXEL_RATE 5500
@@ -329,10 +330,8 @@ static const struct v4l2_subdev_ops rdacm21_subdev_ops = {
.pad= _subdev_pad_ops,
};
-static int ov10640_initialize(struct rdacm21_device *dev)
+static void ov10640_power_up(struct rdacm21_device *dev)
{
- u8 val;
-
/* Enable GPIO0#0 (reset) and GPIO1#0 (pwdn) as output lines. */
ov490_write_reg(dev, OV490_GPIO_SEL0, OV490_GPIO0);
ov490_write_reg(dev, OV490_GPIO_SEL1, OV490_SPWDN0);
@@ -347,18 +346,35 @@ static int ov10640_initialize(struct rdacm21_device *dev)
usleep_range(1500, 3000);
ov490_write_reg(dev, OV490_GPIO_OUTPUT_VALUE0, OV490_GPIO0);
usleep_range(3000, 5000);
+}
- /* Read OV10640 ID to test communications. */
- ov490_write_reg(dev, OV490_SCCB_SLAVE0_DIR, OV490_SCCB_SLAVE_READ);
- ov490_write_reg(dev, OV490_SCCB_SLAVE0_ADDR_HIGH, OV10640_CHIP_ID >> 8);
- ov490_write_reg(dev, OV490_SCCB_SLAVE0_ADDR_LOW, OV10640_CHIP_ID &
0xff);
-
- /* Trigger SCCB slave transaction and give it some time to complete. */
- ov490_write_reg(dev, OV490_HOST_CMD, OV490_HOST_CMD_TRIGGER);
- usleep_range(1000, 1500);
+static int ov10640_check_id(struct rdacm21_device *dev)
+{
+ unsigned int i;
+ u8 val;
- ov490_read_reg(dev, OV490_SCCB_SLAVE0_DIR, );
- if (val != OV10640_ID_HIGH) {
+ /* Read OV10640 ID to test communications. */
+ for (i = 0; i < OV10640_PID_TIMEOUT; ++i) {
+ ov490_write_reg(dev, OV490_SCCB_SLAVE0_DIR,
+ OV490_SCCB_SLAVE_READ);
+ ov490_write_reg(dev, OV490_SCCB_SLAVE0_ADDR_HIGH,
+ OV10640_CHIP_ID >> 8);
+ ov490_write_reg(dev, OV490_SCCB_SLAVE0_ADDR_LOW,
+ OV10640_CHIP_ID & 0xff);
+
+ /*
+* Trigger SCCB slave transaction and give it some time
+* to complete.
+*/
+ ov490_write_reg(dev, OV490_HOST_CMD, OV490_HOST_CMD_TRIGGER);
+ usleep_range(1000, 1500);
+
+ ov490_read_reg(dev, OV490_SCCB_SLAVE0_DIR, );
+ if (val == OV10640_ID_HIGH)
+ break;
+ usleep_range(1000, 1500);
+ }
+ if (i == OV10640_PID_TIMEOUT) {
dev_err(dev->dev, "OV10640 ID mismatch: (0x%02x)\n", val);
return -ENODEV;
}
@@ -374,6 +390,8 @@ static int ov490_initialize(struct rdacm21_device *dev)
unsigned int i;
int ret;
+ ov10640_power_up(dev);
+
/*
* Read OV490 Id to test communications. Give it up to 40msec to
* exit from reset.
@@ -411,7 +429,7 @@ static int ov490_initialize(struct rdacm21_device *dev)
return -ENODEV;
}
- ret = ov10640_initialize(dev);
+ ret = ov10640_check_id(dev);
if (ret)
return ret;
--
2.31.1
[PATCH v4 11/17] media: i2c: rdacm20: Enable noise immunity
Enable the noise immunity threshold at the end of the rdacm20 initialization routine. The rdacm20 camera module has been so far tested with a startup delay that allowed the embedded MCU to program the serializer. If the initialization routine is run before the MCU programs the serializer and the image sensor and their addresses gets changed by the rdacm20 driver it is required to manually enable the noise immunity threshold to make the communication on the control channel more reliable. Reviewed-by: Kieran Bingham Signed-off-by: Jacopo Mondi --- drivers/media/i2c/rdacm20.c | 14 +- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/drivers/media/i2c/rdacm20.c b/drivers/media/i2c/rdacm20.c index c1a717153303..5e0314a2b1ca 100644 --- a/drivers/media/i2c/rdacm20.c +++ b/drivers/media/i2c/rdacm20.c @@ -539,7 +539,19 @@ static int rdacm20_initialize(struct rdacm20_device *dev) dev_info(dev->dev, "Identified MAX9271 + OV10635 device\n"); - return 0; + /* +* Set reverse channel high threshold to increase noise immunity. +* +* This should be compensated by increasing the reverse channel +* amplitude on the remote deserializer side. +* +* TODO Inspect the embedded MCU programming sequence to make sure +* there are no conflicts with the configuration applied here. +* +* TODO Clarify the embedded MCU startup delay to avoid write +* collisions on the I2C bus. +*/ + return max9271_set_high_threshold(>serializer, true); } static int rdacm20_probe(struct i2c_client *client) -- 2.31.1
[PATCH v4 13/17] media: i2c: rdacm20: Report camera module name
When the device is identified the driver currently reports the names of the chips embedded in the camera module. Report the name of the camera module itself instead. Cosmetic change only. Reviewed-by: Kieran Bingham Reviewed-by: Laurent Pinchart Signed-off-by: Jacopo Mondi --- drivers/media/i2c/rdacm20.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/media/i2c/rdacm20.c b/drivers/media/i2c/rdacm20.c index 029af8fd7485..f1819bfa165d 100644 --- a/drivers/media/i2c/rdacm20.c +++ b/drivers/media/i2c/rdacm20.c @@ -537,7 +537,7 @@ static int rdacm20_initialize(struct rdacm20_device *dev) if (ret) return ret; - dev_info(dev->dev, "Identified MAX9271 + OV10635 device\n"); + dev_info(dev->dev, "Identified RDACM20 camera module\n"); /* * Set reverse channel high threshold to increase noise immunity. -- 2.31.1
[PATCH v4 12/17] media: i2c: rdacm20: Embed 'serializer' field
There's no reason to allocate dynamically the 'serializer' field in
the driver structure.
Embed the field and adjust all its users in the driver.
Reviewed-by: Kieran Bingham
Reviewed-by: Laurent Pinchart
Signed-off-by: Jacopo Mondi
---
drivers/media/i2c/rdacm20.c | 36 +++-
1 file changed, 15 insertions(+), 21 deletions(-)
diff --git a/drivers/media/i2c/rdacm20.c b/drivers/media/i2c/rdacm20.c
index 5e0314a2b1ca..029af8fd7485 100644
--- a/drivers/media/i2c/rdacm20.c
+++ b/drivers/media/i2c/rdacm20.c
@@ -312,7 +312,7 @@ static const struct ov10635_reg {
struct rdacm20_device {
struct device *dev;
- struct max9271_device *serializer;
+ struct max9271_device serializer;
struct i2c_client *sensor;
struct v4l2_subdev sd;
struct media_padpad;
@@ -399,7 +399,7 @@ static int rdacm20_s_stream(struct v4l2_subdev *sd, int
enable)
{
struct rdacm20_device *dev = sd_to_rdacm20(sd);
- return max9271_set_serial_link(dev->serializer, enable);
+ return max9271_set_serial_link(>serializer, enable);
}
static int rdacm20_enum_mbus_code(struct v4l2_subdev *sd,
@@ -455,10 +455,10 @@ static int rdacm20_initialize(struct rdacm20_device *dev)
unsigned int retry = 3;
int ret;
- max9271_wake_up(dev->serializer);
+ max9271_wake_up(>serializer);
/* Serial link disabled during config as it needs a valid pixel clock.
*/
- ret = max9271_set_serial_link(dev->serializer, false);
+ ret = max9271_set_serial_link(>serializer, false);
if (ret)
return ret;
@@ -466,35 +466,35 @@ static int rdacm20_initialize(struct rdacm20_device *dev)
* Ensure that we have a good link configuration before attempting to
* identify the device.
*/
- max9271_configure_i2c(dev->serializer, MAX9271_I2CSLVSH_469NS_234NS |
- MAX9271_I2CSLVTO_1024US |
- MAX9271_I2CMSTBT_105KBPS);
+ max9271_configure_i2c(>serializer, MAX9271_I2CSLVSH_469NS_234NS |
+ MAX9271_I2CSLVTO_1024US |
+ MAX9271_I2CMSTBT_105KBPS);
- max9271_configure_gmsl_link(dev->serializer);
+ max9271_configure_gmsl_link(>serializer);
- ret = max9271_verify_id(dev->serializer);
+ ret = max9271_verify_id(>serializer);
if (ret < 0)
return ret;
- ret = max9271_set_address(dev->serializer, dev->addrs[0]);
+ ret = max9271_set_address(>serializer, dev->addrs[0]);
if (ret < 0)
return ret;
- dev->serializer->client->addr = dev->addrs[0];
+ dev->serializer.client->addr = dev->addrs[0];
/*
* Reset the sensor by cycling the OV10635 reset signal connected to the
* MAX9271 GPIO1 and verify communication with the OV10635.
*/
- ret = max9271_enable_gpios(dev->serializer, MAX9271_GPIO1OUT);
+ ret = max9271_enable_gpios(>serializer, MAX9271_GPIO1OUT);
if (ret)
return ret;
- ret = max9271_clear_gpios(dev->serializer, MAX9271_GPIO1OUT);
+ ret = max9271_clear_gpios(>serializer, MAX9271_GPIO1OUT);
if (ret)
return ret;
usleep_range(1, 15000);
- ret = max9271_set_gpios(dev->serializer, MAX9271_GPIO1OUT);
+ ret = max9271_set_gpios(>serializer, MAX9271_GPIO1OUT);
if (ret)
return ret;
usleep_range(1, 15000);
@@ -564,13 +564,7 @@ static int rdacm20_probe(struct i2c_client *client)
if (!dev)
return -ENOMEM;
dev->dev = >dev;
-
- dev->serializer = devm_kzalloc(>dev, sizeof(*dev->serializer),
- GFP_KERNEL);
- if (!dev->serializer)
- return -ENOMEM;
-
- dev->serializer->client = client;
+ dev->serializer.client = client;
ret = of_property_read_u32_array(client->dev.of_node, "reg",
dev->addrs, 2);
--
2.31.1
[PATCH v4 05/17] media: i2c: max9286: Rework comments in .bound()
Rephrase a comment in .bound() callback to make it clear we register a subdev notifier and remove a redundant comment about disabling i2c auto-ack. No functional changes intended. Reviewed-by: Kieran Bingham Reviewed-by: Laurent Pinchart Signed-off-by: Jacopo Mondi --- drivers/media/i2c/max9286.c | 11 +++ 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c index 9124d5fa6ea3..e1c7173f2d00 100644 --- a/drivers/media/i2c/max9286.c +++ b/drivers/media/i2c/max9286.c @@ -556,9 +556,9 @@ static int max9286_notify_bound(struct v4l2_async_notifier *notifier, subdev->name, src_pad, index); /* -* We can only register v4l2_async_notifiers, which do not provide a -* means to register a complete callback. bound_sources allows us to -* identify when all remote serializers have completed their probe. +* As we register a subdev notifiers we won't get a .complete() callback +* here, so we have to use bound_sources to identify when all remote +* serializers have probed. */ if (priv->bound_sources != priv->source_mask) return 0; @@ -575,11 +575,6 @@ static int max9286_notify_bound(struct v4l2_async_notifier *notifier, */ max9286_reverse_channel_setup(priv, MAX9286_REV_AMP_HIGH); max9286_check_config_link(priv, priv->source_mask); - - /* -* Re-configure I2C with local acknowledge disabled after cameras have -* probed. -*/ max9286_configure_i2c(priv, false); return max9286_set_pixelrate(priv); -- 2.31.1
[PATCH v4 06/17] media: i2c: max9271: Check max9271_write() return
Check the return value of the max9271_write() function in the
max9271 library driver.
While at it, modify an existing condition to be made identical
to other checks.
Reviewed-by: Kieran Bingham
Reviewed-by: Laurent Pinchart
Signed-off-by: Jacopo Mondi
---
drivers/media/i2c/max9271.c | 30 +++---
1 file changed, 23 insertions(+), 7 deletions(-)
diff --git a/drivers/media/i2c/max9271.c b/drivers/media/i2c/max9271.c
index c495582dcff6..2c7dc7fb9846 100644
--- a/drivers/media/i2c/max9271.c
+++ b/drivers/media/i2c/max9271.c
@@ -106,7 +106,10 @@ int max9271_set_serial_link(struct max9271_device *dev,
bool enable)
* Short delays here appear to show bit-errors in the writes following.
* Therefore a conservative delay seems best here.
*/
- max9271_write(dev, 0x04, val);
+ ret = max9271_write(dev, 0x04, val);
+ if (ret < 0)
+ return ret;
+
usleep_range(5000, 8000);
return 0;
@@ -118,7 +121,7 @@ int max9271_configure_i2c(struct max9271_device *dev, u8
i2c_config)
int ret;
ret = max9271_write(dev, 0x0d, i2c_config);
- if (ret)
+ if (ret < 0)
return ret;
/* The delay required after an I2C bus configuration change is not
@@ -143,7 +146,10 @@ int max9271_set_high_threshold(struct max9271_device *dev,
bool enable)
* Enable or disable reverse channel high threshold to increase
* immunity to power supply noise.
*/
- max9271_write(dev, 0x08, enable ? ret | BIT(0) : ret & ~BIT(0));
+ ret = max9271_write(dev, 0x08, enable ? ret | BIT(0) : ret & ~BIT(0));
+ if (ret < 0)
+ return ret;
+
usleep_range(2000, 2500);
return 0;
@@ -152,6 +158,8 @@ EXPORT_SYMBOL_GPL(max9271_set_high_threshold);
int max9271_configure_gmsl_link(struct max9271_device *dev)
{
+ int ret;
+
/*
* Configure the GMSL link:
*
@@ -162,16 +170,24 @@ int max9271_configure_gmsl_link(struct max9271_device
*dev)
*
* TODO: Make the GMSL link configuration parametric.
*/
- max9271_write(dev, 0x07, MAX9271_DBL | MAX9271_HVEN |
- MAX9271_EDC_1BIT_PARITY);
+ ret = max9271_write(dev, 0x07, MAX9271_DBL | MAX9271_HVEN |
+ MAX9271_EDC_1BIT_PARITY);
+ if (ret < 0)
+ return ret;
+
usleep_range(5000, 8000);
/*
* Adjust spread spectrum to +4% and auto-detect pixel clock
* and serial link rate.
*/
- max9271_write(dev, 0x02, MAX9271_SPREAD_SPECT_4 | MAX9271_R02_RES |
- MAX9271_PCLK_AUTODETECT | MAX9271_SERIAL_AUTODETECT);
+ ret = max9271_write(dev, 0x02,
+ MAX9271_SPREAD_SPECT_4 | MAX9271_R02_RES |
+ MAX9271_PCLK_AUTODETECT |
+ MAX9271_SERIAL_AUTODETECT);
+ if (ret < 0)
+ return ret;
+
usleep_range(5000, 8000);
return 0;
--
2.31.1
[PATCH v4 07/17] media: i2c: max9271: Introduce wake_up() function
The MAX9271 chip manual prescribes a delay of 5 milliseconds
after the chip exits from low power state.
Add a new function to the max9271 library driver that wakes up the chip
with a dummy i2c transaction and implements the correct delay of 5
milliseconds after the chip exits from low power state.
Use the newly introduced function in the rdacm20 and rdacm21 camera
drivers. The former was not respecting the required delay while the
latter was waiting for a too-short timeout.
Reviewed-by: Kieran Bingham
Reviewed-by: Laurent Pinchart
Signed-off-by: Jacopo Mondi
---
drivers/media/i2c/max9271.c | 12
drivers/media/i2c/max9271.h | 9 +
drivers/media/i2c/rdacm20.c | 4 +---
drivers/media/i2c/rdacm21.c | 5 +
4 files changed, 23 insertions(+), 7 deletions(-)
diff --git a/drivers/media/i2c/max9271.c b/drivers/media/i2c/max9271.c
index 2c7dc7fb9846..ff86c8c4ea61 100644
--- a/drivers/media/i2c/max9271.c
+++ b/drivers/media/i2c/max9271.c
@@ -80,6 +80,18 @@ static int max9271_pclk_detect(struct max9271_device *dev)
return -EIO;
}
+void max9271_wake_up(struct max9271_device *dev)
+{
+ /*
+* Use the chip default address as this function has to be called
+* before any other one.
+*/
+ dev->client->addr = MAX9271_DEFAULT_ADDR;
+ i2c_smbus_read_byte(dev->client);
+ usleep_range(5000, 8000);
+}
+EXPORT_SYMBOL_GPL(max9271_wake_up);
+
int max9271_set_serial_link(struct max9271_device *dev, bool enable)
{
int ret;
diff --git a/drivers/media/i2c/max9271.h b/drivers/media/i2c/max9271.h
index d78fb21441e9..dc5e4e70ba6f 100644
--- a/drivers/media/i2c/max9271.h
+++ b/drivers/media/i2c/max9271.h
@@ -85,6 +85,15 @@ struct max9271_device {
struct i2c_client *client;
};
+/**
+ * max9271_wake_up() - Wake up the serializer by issuing an i2c transaction
+ * @dev: The max9271 device
+ *
+ * This function shall be called before any other interaction with the
+ * serializer.
+ */
+void max9271_wake_up(struct max9271_device *dev);
+
/**
* max9271_set_serial_link() - Enable/disable serial link
* @dev: The max9271 device
diff --git a/drivers/media/i2c/rdacm20.c b/drivers/media/i2c/rdacm20.c
index 90eb73f0e6e9..c1a717153303 100644
--- a/drivers/media/i2c/rdacm20.c
+++ b/drivers/media/i2c/rdacm20.c
@@ -455,9 +455,7 @@ static int rdacm20_initialize(struct rdacm20_device *dev)
unsigned int retry = 3;
int ret;
- /* Verify communication with the MAX9271: ping to wakeup. */
- dev->serializer->client->addr = MAX9271_DEFAULT_ADDR;
- i2c_smbus_read_byte(dev->serializer->client);
+ max9271_wake_up(dev->serializer);
/* Serial link disabled during config as it needs a valid pixel clock.
*/
ret = max9271_set_serial_link(dev->serializer, false);
diff --git a/drivers/media/i2c/rdacm21.c b/drivers/media/i2c/rdacm21.c
index 179d107f494c..553e3f03752b 100644
--- a/drivers/media/i2c/rdacm21.c
+++ b/drivers/media/i2c/rdacm21.c
@@ -450,10 +450,7 @@ static int rdacm21_initialize(struct rdacm21_device *dev)
{
int ret;
- /* Verify communication with the MAX9271: ping to wakeup. */
- dev->serializer.client->addr = MAX9271_DEFAULT_ADDR;
- i2c_smbus_read_byte(dev->serializer.client);
- usleep_range(3000, 5000);
+ max9271_wake_up(>serializer);
/* Enable reverse channel and disable the serial link. */
ret = max9271_set_serial_link(>serializer, false);
--
2.31.1
[PATCH v4 08/17] media: i2c: rdacm21: Add dealy after OV490 reset
Add a delay after the OV490 chip is put in reset state. The reset signal shall be held for at least 250 useconds. Signed-off-by: Jacopo Mondi --- drivers/media/i2c/rdacm21.c | 6 +- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/drivers/media/i2c/rdacm21.c b/drivers/media/i2c/rdacm21.c index 553e3f03752b..6be8ce130e78 100644 --- a/drivers/media/i2c/rdacm21.c +++ b/drivers/media/i2c/rdacm21.c @@ -469,7 +469,10 @@ static int rdacm21_initialize(struct rdacm21_device *dev) if (ret) return ret; - /* Enable GPIO1 and hold OV490 in reset during max9271 configuration. */ + /* +* Enable GPIO1 and hold OV490 in reset during max9271 configuration. +* The reset signal has to be asserted for at least 250 useconds. +*/ ret = max9271_enable_gpios(>serializer, MAX9271_GPIO1OUT); if (ret) return ret; @@ -477,6 +480,7 @@ static int rdacm21_initialize(struct rdacm21_device *dev) ret = max9271_clear_gpios(>serializer, MAX9271_GPIO1OUT); if (ret) return ret; + usleep_range(250, 500); ret = max9271_configure_gmsl_link(>serializer); if (ret) -- 2.31.1
[PATCH v4 09/17] media: i2c: rdacm21: Fix OV10640 powerup
The OV10640 image sensor powerdown signal is controlled by the first
line of the OV490 GPIO pad #1, but the pad #0 identifier
OV490_GPIO_OUTPUT_VALUE0 was erroneously used. As a result the image
sensor powerdown signal was never asserted but was left floating and
kept high by an internal pull-up resistor, causing sporadic failures
during the image sensor startup phase.
Fix this by using the correct GPIO pad identifier and wait the mandatory
1.5 millisecond delay after the powerup lane is asserted. The reset
delay is not characterized in the chip manual if not as "255 XVCLK +
initialization". Wait for at least 3 milliseconds to guarantee the SCCB
bus is available.
While at it also fix the reset sequence, as the reset line was released
before the powerdown one, and the line was not cycled.
This commit fixes a sporadic start-up error triggered by a failure to
read the OV10640 chip ID:
rdacm21 8-0054: OV10640 ID mismatch: (0x01)
Fixes: a59f853b3b4b ("media: i2c: Add driver for RDACM21 camera module")
Reviewed-by: Kieran Bingham
Reviewed-by: Laurent Pinchart
Signed-off-by: Jacopo Mondi
---
drivers/media/i2c/rdacm21.c | 10 --
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/drivers/media/i2c/rdacm21.c b/drivers/media/i2c/rdacm21.c
index 6be8ce130e78..7c0a4a84340a 100644
--- a/drivers/media/i2c/rdacm21.c
+++ b/drivers/media/i2c/rdacm21.c
@@ -333,13 +333,19 @@ static int ov10640_initialize(struct rdacm21_device *dev)
{
u8 val;
- /* Power-up OV10640 by setting RESETB and PWDNB pins high. */
+ /* Enable GPIO0#0 (reset) and GPIO1#0 (pwdn) as output lines. */
ov490_write_reg(dev, OV490_GPIO_SEL0, OV490_GPIO0);
ov490_write_reg(dev, OV490_GPIO_SEL1, OV490_SPWDN0);
ov490_write_reg(dev, OV490_GPIO_DIRECTION0, OV490_GPIO0);
ov490_write_reg(dev, OV490_GPIO_DIRECTION1, OV490_SPWDN0);
+
+ /* Power up OV10640 and then reset it. */
+ ov490_write_reg(dev, OV490_GPIO_OUTPUT_VALUE1, OV490_SPWDN0);
+ usleep_range(1500, 3000);
+
+ ov490_write_reg(dev, OV490_GPIO_OUTPUT_VALUE0, 0x00);
+ usleep_range(1500, 3000);
ov490_write_reg(dev, OV490_GPIO_OUTPUT_VALUE0, OV490_GPIO0);
- ov490_write_reg(dev, OV490_GPIO_OUTPUT_VALUE0, OV490_SPWDN0);
usleep_range(3000, 5000);
/* Read OV10640 ID to test communications. */
--
2.31.1
[PATCH v4 04/17] media: i2c: max9286: Define high channel amplitude
Provide a macro to define the reverse channel amplitude to be used to compensate the remote serializer noise immunity. While at it, update a comment. Reviewed-by: Kieran Bingham Reviewed-by: Laurent Pinchart Signed-off-by: Jacopo Mondi --- drivers/media/i2c/max9286.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c index ed1cdefe7c30..9124d5fa6ea3 100644 --- a/drivers/media/i2c/max9286.c +++ b/drivers/media/i2c/max9286.c @@ -113,6 +113,7 @@ #define MAX9286_REV_TRF(n) ((n) << 4) #define MAX9286_REV_AMP(n) n) - 30) / 10) << 1) /* in mV */ #define MAX9286_REV_AMP_X BIT(0) +#define MAX9286_REV_AMP_HIGH 170 /* Register 0x3f */ #define MAX9286_EN_REV_CFG BIT(6) #define MAX9286_REV_FLEN(n)((n) - 20) @@ -567,12 +568,12 @@ static int max9286_notify_bound(struct v4l2_async_notifier *notifier, * channels: * * - Increase the reverse channel amplitude to compensate for the -* remote ends high threshold, if not done already +* remote ends high threshold * - Verify all configuration links are properly detected * - Disable auto-ack as communication on the control channel are now * stable. */ - max9286_reverse_channel_setup(priv, 170); + max9286_reverse_channel_setup(priv, MAX9286_REV_AMP_HIGH); max9286_check_config_link(priv, priv->source_mask); /* -- 2.31.1
[PATCH v4 03/17] media: i2c: max9286: Cache channel amplitude
Cache the current channel amplitude in a driver variable
to skip updating it if the newly requested value is the same
as the currently configured one.
Reviewed-by: Kieran Bingham
Reviewed-by: Laurent Pinchart
Signed-off-by: Jacopo Mondi
---
drivers/media/i2c/max9286.c | 13 ++---
1 file changed, 10 insertions(+), 3 deletions(-)
diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
index 82ec05e96cb7..ed1cdefe7c30 100644
--- a/drivers/media/i2c/max9286.c
+++ b/drivers/media/i2c/max9286.c
@@ -165,6 +165,7 @@ struct max9286_priv {
/* The initial reverse control channel amplitude. */
u32 init_rev_chan_mv;
+ u32 rev_chan_mv;
struct v4l2_ctrl_handler ctrls;
struct v4l2_ctrl *pixelrate;
@@ -341,8 +342,15 @@ static void max9286_configure_i2c(struct max9286_priv
*priv, bool localack)
static void max9286_reverse_channel_setup(struct max9286_priv *priv,
unsigned int chan_amplitude)
{
+ u8 chan_config;
+
+ if (priv->rev_chan_mv == chan_amplitude)
+ return;
+
+ priv->rev_chan_mv = chan_amplitude;
+
/* Reverse channel transmission time: default to 1. */
- u8 chan_config = MAX9286_REV_TRF(1);
+ chan_config = MAX9286_REV_TRF(1);
/*
* Reverse channel setup.
@@ -564,8 +572,7 @@ static int max9286_notify_bound(struct v4l2_async_notifier
*notifier,
* - Disable auto-ack as communication on the control channel are now
* stable.
*/
- if (priv->init_rev_chan_mv < 170)
- max9286_reverse_channel_setup(priv, 170);
+ max9286_reverse_channel_setup(priv, 170);
max9286_check_config_link(priv, priv->source_mask);
/*
--
2.31.1
[PATCH v4 02/17] media: i2c: max9286: Rename reverse_channel_mv
Rename the reverse_channel_mv variable to init_rev_chan_mv as
the next patch will cache the reverse channel amplitude in
a new driver variable.
Reviewed-by: Kieran Bingham
Reviewed-by: Laurent Pinchart
Signed-off-by: Jacopo Mondi
---
drivers/media/i2c/max9286.c | 11 ++-
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
index 1d9951215868..82ec05e96cb7 100644
--- a/drivers/media/i2c/max9286.c
+++ b/drivers/media/i2c/max9286.c
@@ -163,7 +163,8 @@ struct max9286_priv {
unsigned int mux_channel;
bool mux_open;
- u32 reverse_channel_mv;
+ /* The initial reverse control channel amplitude. */
+ u32 init_rev_chan_mv;
struct v4l2_ctrl_handler ctrls;
struct v4l2_ctrl *pixelrate;
@@ -563,7 +564,7 @@ static int max9286_notify_bound(struct v4l2_async_notifier
*notifier,
* - Disable auto-ack as communication on the control channel are now
* stable.
*/
- if (priv->reverse_channel_mv < 170)
+ if (priv->init_rev_chan_mv < 170)
max9286_reverse_channel_setup(priv, 170);
max9286_check_config_link(priv, priv->source_mask);
@@ -971,7 +972,7 @@ static int max9286_setup(struct max9286_priv *priv)
* only. This should be disabled after the mux is initialised.
*/
max9286_configure_i2c(priv, true);
- max9286_reverse_channel_setup(priv, priv->reverse_channel_mv);
+ max9286_reverse_channel_setup(priv, priv->init_rev_chan_mv);
/*
* Enable GMSL links, mask unused ones and autodetect link
@@ -1236,9 +1237,9 @@ static int max9286_parse_dt(struct max9286_priv *priv)
if (of_property_read_u32(dev->of_node,
"maxim,reverse-channel-microvolt",
_channel_microvolt))
- priv->reverse_channel_mv = 170;
+ priv->init_rev_chan_mv = 170;
else
- priv->reverse_channel_mv = reverse_channel_microvolt / 1000U;
+ priv->init_rev_chan_mv = reverse_channel_microvolt / 1000U;
priv->route_mask = priv->source_mask;
--
2.31.1
[PATCH v4 01/17] media: i2c: max9286: Adjust parameters indent
The parameters to max9286_i2c_mux_configure() fits on the previous line. Adjust it. Cosmetic change only. Reviewed-by: Kieran Bingham Reviewed-by: Laurent Pinchart Signed-off-by: Jacopo Mondi --- drivers/media/i2c/max9286.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c index 6fd4d59fcc72..1d9951215868 100644 --- a/drivers/media/i2c/max9286.c +++ b/drivers/media/i2c/max9286.c @@ -287,9 +287,8 @@ static int max9286_i2c_mux_select(struct i2c_mux_core *muxc, u32 chan) priv->mux_channel = chan; - max9286_i2c_mux_configure(priv, - MAX9286_FWDCCEN(chan) | - MAX9286_REVCCEN(chan)); + max9286_i2c_mux_configure(priv, MAX9286_FWDCCEN(chan) | + MAX9286_REVCCEN(chan)); return 0; } -- 2.31.1
[PATCH v4 00/17] media: gmsl: Reliability improvements
Hello, series following: v1: https://patchwork.linuxtv.org/project/linux-media/list/?series=4650 v2: https://patchwork.linuxtv.org/project/linux-media/list/?series=4861 v3: https://patchwork.linuxtv.org/project/linux-media/list/?series=4904 This series is a minor rework that builds on top of the comments received on v3. I've re-order patches for better consumption: [01/17] -> [05/17]: max9286 style fixes [06/17] -> [07/17]: max9271 minor fixes [08/17] -> [10/17]: rdamc21 fixes: these patches are relevant for RDACM21 stability [11/17] -> [15/17]: rdacm20 fixes [16/17] -> [17/17]: GMSL initialization series rework, also relevant for syste, stability The only part where consensus still has to be reached is the last two patches. Unfortunately, Sakari's suggestion of moving the remotes initialization to s_stream() time did not work, and this version is the only one I've found that gurantees a reliable initialization sequence. I've cc-ed Hans and Sakari to continue the discussion. Run quite some tests with Eagle and RDACM21: 1378 boot cycles with 100% boot success (thanks Kieran for the board access). Thanks j Jacopo Mondi (17): media: i2c: max9286: Adjust parameters indent media: i2c: max9286: Rename reverse_channel_mv media: i2c: max9286: Cache channel amplitude media: i2c: max9286: Define high channel amplitude media: i2c: max9286: Rework comments in .bound() media: i2c: max9271: Check max9271_write() return media: i2c: max9271: Introduce wake_up() function media: i2c: rdacm21: Add dealy after OV490 reset media: i2c: rdacm21: Fix OV10640 powerup media: i2c: rdacm21: Power up OV10640 before OV490 media: i2c: rdacm20: Enable noise immunity media: i2c: rdacm20: Embed 'serializer' field media: i2c: rdacm20: Report camera module name media: i2c: rdacm20: Check return values media: i2c: rdacm20: Re-work ov10635 reset media: v4l2-subdev: De-deprecate init() subdev op media: gmsl: Reimplement initialization sequence drivers/media/i2c/max9271.c | 42 +-- drivers/media/i2c/max9271.h | 9 +++ drivers/media/i2c/max9286.c | 56 +-- drivers/media/i2c/rdacm20.c | 135 +--- drivers/media/i2c/rdacm21.c | 124 + include/media/v4l2-subdev.h | 15 +++- 6 files changed, 253 insertions(+), 128 deletions(-) -- 2.31.1
Re: [PATCH 2/3] media: rdacm21: describe better a truncation
Hi Mauro On Thu, Apr 08, 2021 at 09:40:03AM +0200, Mauro Carvalho Chehab wrote: > As warned by sparse: > > drivers/media/i2c/rdacm21.c:348:62: warning: cast truncates bits from > constant value (300a becomes a) > > the intention of the code is to get just the lowest 8 bits. > So, instead of using a typecast, use a bit and logic. > > Signed-off-by: Mauro Carvalho Chehab Please see https://patchwork.linuxtv.org/project/linux-media/patch/20210319164148.199192-11-jacopo+rene...@jmondi.org/ Whatever gets in first it's fine, so you can add my Acked-by: Jacopo Mondi to this one too Thanks j > --- > drivers/media/i2c/rdacm21.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/media/i2c/rdacm21.c b/drivers/media/i2c/rdacm21.c > index dcc21515e5a4..179d107f494c 100644 > --- a/drivers/media/i2c/rdacm21.c > +++ b/drivers/media/i2c/rdacm21.c > @@ -345,7 +345,7 @@ static int ov10640_initialize(struct rdacm21_device *dev) > /* Read OV10640 ID to test communications. */ > ov490_write_reg(dev, OV490_SCCB_SLAVE0_DIR, OV490_SCCB_SLAVE_READ); > ov490_write_reg(dev, OV490_SCCB_SLAVE0_ADDR_HIGH, OV10640_CHIP_ID >> 8); > - ov490_write_reg(dev, OV490_SCCB_SLAVE0_ADDR_LOW, (u8)OV10640_CHIP_ID); > + ov490_write_reg(dev, OV490_SCCB_SLAVE0_ADDR_LOW, OV10640_CHIP_ID & > 0xff); > > /* Trigger SCCB slave transaction and give it some time to complete. */ > ov490_write_reg(dev, OV490_HOST_CMD, OV490_HOST_CMD_TRIGGER); > -- > 2.30.2 >
Re: [PATCH 24/32] MAINTAINERS: update imi,rdacm2x-gmsl.yaml reference
Hi Mauro,
On Thu, Apr 01, 2021 at 02:17:44PM +0200, Mauro Carvalho Chehab wrote:
> The file name: Documentation/devicetree/bindings/media/i2c/rdacm2x-gmsl.yaml
> should be, instead:
> Documentation/devicetree/bindings/media/i2c/imi,rdacm2x-gmsl.yaml.
>
> Update its cross-reference accordingly.
>
> Fixes: 34009bffc1c6 ("media: i2c: Add RDACM20 driver")
> Fixes: e9f817689789 ("media: dt-bindings: media: i2c: Add bindings for IMI
> RDACM2x")
> Signed-off-by: Mauro Carvalho Chehab
Thanks
Acked-by: Jacopo Mondi
> ---
> MAINTAINERS | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 1644b6e9697c..b405ee71f730 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -15258,7 +15258,7 @@ M:Laurent Pinchart
>
> M: Niklas Söderlund
> L: linux-me...@vger.kernel.org
> S: Maintained
> -F: Documentation/devicetree/bindings/media/i2c/rdacm2x-gmsl.yaml
> +F: Documentation/devicetree/bindings/media/i2c/imi,rdacm2x-gmsl.yaml
> F: drivers/media/i2c/max9271.c
> F: drivers/media/i2c/max9271.h
> F: drivers/media/i2c/rdacm21.c
> --
> 2.30.2
>
Re: [PATCH v3 05/19] media: v4l2-subdev: De-deprecate init() subdev op
Hi Sakari,
On Mon, Mar 22, 2021 at 01:51:44PM +0100, Jacopo Mondi wrote:
> Hi Sakari,
>
> On Sun, Mar 21, 2021 at 10:52:56PM +0200, Sakari Ailus wrote:
> > Hi Laurent and Jacopo,
> >
> > Thanks for cc'ing me.
> >
> > On Sat, Mar 20, 2021 at 05:42:12PM +0200, Laurent Pinchart wrote:
> > > Hi Jacopo,
> > >
> > > Thank you for the patch.
> > >
> > > CC'ing Sakari on v3 to get feedback.
> > >
> > > On Fri, Mar 19, 2021 at 05:41:34PM +0100, Jacopo Mondi wrote:
> > > > The init() subdev core operation is deemed to be deprecated for new
> > > > subdevice drivers. However it could prove useful for complex
> > > > architectures to defer operation that require access to the
> > > > communication bus if said bus is not available (or fully configured)
> > > > at the time when the subdevice probe() function is run.
> > > >
> > > > As an example, the GMSL architecture requires the GMSL configuration
> > > > link to be configured on the host side after the remote subdevice
> > > > has completed its probe function. After the configuration on the host
> > > > side has been performed, the subdevice registers can be accessed through
> > > > the communication bus.
> >
> > What does the remote device's probe do that needs to be done before bus
> > config on the host side?
>
> A few lines here below:
>
> In the GMSL use case the bus configuration requires the enablement of the
> noise immunity threshold on the remote side which ensures reliability
> of communications in electrically noisy environments. After the subdevice
> has enabled the threshold at the end of its probe() sequence the host
> side shall compensate it with an higher signal amplitude. Once this
> sequence has completed the bus can be accessed with noise protection
> enabled and all the operations that require a considerable number of
> transactions on the bus (such as the image sensor configuration
> sequence) are run in the subdevice init() operation implementation.
>
> >
> > Alternatively, could the remote init() work be done at the time streaming
> > is started?
>
> That would require programing the sensor, the embedded ISP at s_stream
> time which would take some time.
I'm afraid but from my testing also the chip identification is more
reliable if run in init(). As identifying chips is something that has
to happen at probe/initialization I fear it is not possible to move it
to s_stream time.
>
> I'll take this suggestion into account though and run some more tests.
>
> Thanks
> j
>
> >
> > > >
> > > > In particular:
> > > >
> > > > HOSTREMOTE
> > > >
> > > > probe()
> > > >|
> > > >-> |
> > > > probe() {
> > > > bus config()
> > > > }
> > > >|<|
> > > > v4l2 async bound {
> > > > bus config()
> > > > call subdev init()
> > > >|>|
> > > > init() {
> > > > access register on the bus()
> > > > }
> > > >|<---
> > > > }
> > > >
> > > > In the GMSL use case the bus configuration requires the enablement of
> > > > the
> > > > noise immunity threshold on the remote side which ensures reliability
> > > > of communications in electrically noisy environments. After the
> > > > subdevice
> > > > has enabled the threshold at the end of its probe() sequence the host
> > > > side shall compensate it with an higher signal amplitude. Once this
> > > > sequence has completed the bus can be accessed with noise protection
> > > > enabled and all the operations that require a considerable number of
> > > > transactions on the bus (such as the image sensor configuration
> > > > sequence) are run in the subdevice init() operation implementation.
> > > >
> > > > Signed-off-by: Jacopo Mondi
> > > > ---
> > > > include/media/v4l2-subdev.h | 15 ---
> > > > 1 file changed, 12 insertions(+), 3 deletions(-)
> > > >
> > > > d
Re: [PATCH v3 11/19] media: i2c: rdacm21: Add dealy after OV490 reset
Hi Laurent, Kieran, On Sat, Mar 20, 2021 at 05:52:18PM +0200, Laurent Pinchart wrote: > Hi Jacopo, > > Thank you for the patch. > > On Fri, Mar 19, 2021 at 04:49:44PM +, Kieran Bingham wrote: > > Hi Jacopo, > > > > s/dealy/delay/ in $SUBJECT > > > > On 19/03/2021 16:41, Jacopo Mondi wrote: > > > Add a delay after the OV490 chip is put in reset state. The reset > > > signal shall be held for at least 250 useconds. > > > > > > Signed-off-by: Jacopo Mondi > > > --- > > > drivers/media/i2c/rdacm21.c | 6 +- > > > 1 file changed, 5 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/media/i2c/rdacm21.c b/drivers/media/i2c/rdacm21.c > > > index babd14b21252..875bec9f7904 100644 > > > --- a/drivers/media/i2c/rdacm21.c > > > +++ b/drivers/media/i2c/rdacm21.c > > > @@ -448,7 +448,10 @@ static int rdacm21_init(struct v4l2_subdev *sd, > > > unsigned int val) > > > if (ret) > > > return ret; > > > > > > - /* Enable GPIO1 and hold OV490 in reset during max9271 configuration. */ > > > + /* > > > + * Enable GPIO1 and hold OV490 in reset during max9271 configuration. > > > + * The reset signal has to be asserted for at least 250 useconds. > > > + */ > > > ret = max9271_enable_gpios(>serializer, MAX9271_GPIO1OUT); > > > if (ret) > > > return ret; > > > @@ -456,6 +459,7 @@ static int rdacm21_init(struct v4l2_subdev *sd, > > > unsigned int val) > > > ret = max9271_clear_gpios(>serializer, MAX9271_GPIO1OUT); > > > if (ret) > > > return ret; > > > + usleep_range(250, 500); > > > > Aha, for a second there I thought the comment meant it had to be > > asserted for 250 uS before clearing it again. But it's 250 uS before > > using the OV490. No, it's 250usecond before releasing it. After the OV490 is released from reset there's a 3-5 milliseconds sleep below. > > > > Perhaps possible to update the comment a little, but nothing that matters. > > The commit message and comment should match the code, especially given They do if I'm not mistaken. > that I'm not sure here which of the two is actually incorrect. I suspect > the sleep is actually in the wrong location. > > > Reviewed-by: Kieran Bingham > > > > > > > ret = max9271_configure_gmsl_link(>serializer); > > > if (ret) > > > > > -- > Regards, > > Laurent Pinchart
Re: [PATCH v3 19/19] media: i2c: rdacm20: Re-work ov10635 reset
Hi Laurent,
On Sat, Mar 20, 2021 at 06:24:09PM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Fri, Mar 19, 2021 at 05:41:48PM +0100, Jacopo Mondi wrote:
> > The OV10635 image sensor embedded in the camera module is currently
> > reset after the MAX9271 initialization with two long delays that were
> > most probably not correctly characterized.
> >
> > Re-work the image sensor reset procedure by holding the chip in reset
> > during the MAX9271 configuration, removing the long sleep delays and
> > only wait after the chip exits from reset for 350-500 microseconds
> > interval, which is larger than the minimum (2048 * (1 / XVCLK)) timeout
> > characterized in the chip manual.
> >
> > Reviewed-by: Kieran Bingham
> > Reviewed-by: Laurent Pinchart
> > Signed-off-by: Jacopo Mondi
> > ---
> > drivers/media/i2c/rdacm20.c | 29 +
> > 1 file changed, 17 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/media/i2c/rdacm20.c b/drivers/media/i2c/rdacm20.c
> > index 7ed3866b5335..7ba2d0249da8 100644
> > --- a/drivers/media/i2c/rdacm20.c
> > +++ b/drivers/media/i2c/rdacm20.c
> > @@ -454,6 +454,19 @@ static int rdacm20_init(struct v4l2_subdev *sd,
> > unsigned int val)
> > if (ret)
> > return ret;
> >
> > + /*
> > +* Hold OV10635 in reset during max9271 configuration. The reset signal
> > +* has to be asserted for at least 200 microseconds.
> > +*/
> > + ret = max9271_enable_gpios(>serializer, MAX9271_GPIO1OUT);
> > + if (ret)
> > + return ret;
> > +
> > + ret = max9271_clear_gpios(>serializer, MAX9271_GPIO1OUT);
>
> enable and clear is very cnonfusing. How about mimicking the GPIO API,
> with direction_input(), direction_output() and set_value() functions ?
> It would also be nice if the polarity could be handled in a nicer way.
> There's no GPIO request API here, but maybe a max9271_gpio_set_flags() ?
>
Probably, that's an unrelated rework that could go on top unrelated to
this patch
> > + if (ret)
> > + return ret;
> > + usleep_range(200, 500);
> > +
> > ret = max9271_configure_gmsl_link(>serializer);
> > if (ret)
> > return ret;
> > @@ -468,22 +481,14 @@ static int rdacm20_init(struct v4l2_subdev *sd,
> > unsigned int val)
> > dev->serializer.client->addr = dev->addrs[0];
> >
> > /*
> > -* Reset the sensor by cycling the OV10635 reset signal connected to the
> > -* MAX9271 GPIO1 and verify communication with the OV10635.
> > +* Release ov10635 from reset and initialize it. The image sensor
> > +* requires at least 2048 XVCLK cycles (85 micro-seconds at 24MHz)
> > +* before being available. Stay safe and wait up to 500 micro-seconds.
> > */
> > - ret = max9271_enable_gpios(>serializer, MAX9271_GPIO1OUT);
> > - if (ret)
> > - return ret;
> > -
> > - ret = max9271_clear_gpios(>serializer, MAX9271_GPIO1OUT);
> > - if (ret)
> > - return ret;
> > - usleep_range(1, 15000);
> > -
> > ret = max9271_set_gpios(>serializer, MAX9271_GPIO1OUT);
> > if (ret)
> > return ret;
> > - usleep_range(1, 15000);
> > + usleep_range(100, 500);
> >
> > for (i = 0; i < OV10635_PID_TIMEOUT; ++i) {
> > ret = ov10635_read16(dev, OV10635_PID);
>
> --
> Regards,
>
> Laurent Pinchart
Re: [PATCH v3 13/19] media: i2c: rdacm21: Power up OV10640 before OV490
Hi Laurent,
On Sat, Mar 20, 2021 at 06:14:43PM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Fri, Mar 19, 2021 at 05:41:42PM +0100, Jacopo Mondi wrote:
> > The current RDACM21 initialization routine powers up the
> > OV10640 image sensor after the OV490 ISP. The ISP is programmed with
> > a firmware loaded from an embedded EEPROM that (most probably) tries
>
> It's actually a serial flash, not an EEPROM.
>
> > to interact and program also the image sensor connected to the ISP.
> >
> > As described in commit ccb26c5742f5 ("media: i2c: rdacm21: Fix OV10640
>
> That commit ID won't be valid anymore once the patches get merged,
> unless they are pulled instead of cherry-picked, which is currently not
> done in the Linux media subsystem. I'd drop it an only mention the
> commit's subject.
>
> > powerup") the image sensor powerdown signal is kept high by an internal
> > pull up resistor and occasionally fails to startup correctly if the
> > powerdown line is not asserted explicitly. Failures in the OV10640
> > startup causes the OV490 firmware to fail to boot correctly resulting in
> > the camera module initialization to fail consequentially.
> >
> > Fix this by powering up the OV10640 image sensor before testing the
> > OV490 firmware boot completion, by splitting the ov10640_initialize()
> > function in an ov10640_power_up() one and an ov10640_check_id() one.
> >
> > Also make sure the OV10640 identification procedure gives enough time to
> > the image sensor to resume after the programming phase performed by the
> > OV490 firmware by repeating the ID read procedure,
>
> s/,/./
>
I'll rework
> >
> > This commit fixes a sporadic start-up error triggered by a failure to
> > detect the OV490 firmware boot completion:
> > rdacm21 8-0054: Timeout waiting for firmware boot
> >
> > Fixes: a59f853b3b4b ("media: i2c: Add driver for RDACM21 camera module")
> > Signed-off-by: Jacopo Mondi
> > ---
> > drivers/media/i2c/rdacm21.c | 46 ++---
> > 1 file changed, 32 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/media/i2c/rdacm21.c b/drivers/media/i2c/rdacm21.c
> > index 3f38c465b348..3763eb690d74 100644
> > --- a/drivers/media/i2c/rdacm21.c
> > +++ b/drivers/media/i2c/rdacm21.c
> > @@ -69,6 +69,7 @@
> > #define OV490_ISP_VSIZE_LOW0x80820062
> > #define OV490_ISP_VSIZE_HIGH 0x80820063
> >
> > +#define OV10640_PID_TIMEOUT20
> > #define OV10640_ID_HIGH0xa6
> > #define OV10640_CHIP_ID0x300a
> > #define OV10640_PIXEL_RATE 5500
> > @@ -314,10 +315,8 @@ static int rdacm21_get_fmt(struct v4l2_subdev *sd,
> > return 0;
> > }
> >
> > -static int ov10640_initialize(struct rdacm21_device *dev)
> > +static void ov10640_power_up(struct rdacm21_device *dev)
> > {
> > - u8 val;
> > -
> > /* Enable GPIO0#0 (reset) and GPIO1#0 (pwdn) as output lines. */
> > ov490_write_reg(dev, OV490_GPIO_SEL0, OV490_GPIO0);
> > ov490_write_reg(dev, OV490_GPIO_SEL1, OV490_SPWDN0);
> > @@ -332,18 +331,35 @@ static int ov10640_initialize(struct rdacm21_device
> > *dev)
> > usleep_range(1500, 3000);
> > ov490_write_reg(dev, OV490_GPIO_OUTPUT_VALUE0, OV490_GPIO0);
> > usleep_range(3000, 5000);
> > +}
> >
> > - /* Read OV10640 ID to test communications. */
> > - ov490_write_reg(dev, OV490_SCCB_SLAVE0_DIR, OV490_SCCB_SLAVE_READ);
> > - ov490_write_reg(dev, OV490_SCCB_SLAVE0_ADDR_HIGH, OV10640_CHIP_ID >> 8);
> > - ov490_write_reg(dev, OV490_SCCB_SLAVE0_ADDR_LOW, OV10640_CHIP_ID &
> > 0xff);
> > -
> > - /* Trigger SCCB slave transaction and give it some time to complete. */
> > - ov490_write_reg(dev, OV490_HOST_CMD, OV490_HOST_CMD_TRIGGER);
> > - usleep_range(1000, 1500);
> > +static int ov10640_check_id(struct rdacm21_device *dev)
> > +{
> > + unsigned int i;
> > + u8 val;
> >
> > - ov490_read_reg(dev, OV490_SCCB_SLAVE0_DIR, );
> > - if (val != OV10640_ID_HIGH) {
> > + /* Read OV10640 ID to test communications. */
> > + for (i = 0; i < OV10640_PID_TIMEOUT; ++i) {
>
> OV10640_PID_TIMEOUT is used in this function only, I would have made it
> local.
>
I don't think a local variable is better. The timeout is a read-only
constant which is better defined as a macro than a variable.
> > + ov490_write_reg(dev, OV490_SCCB_SLAVE0_DIR,
> > +
Re: [PATCH v3 05/19] media: v4l2-subdev: De-deprecate init() subdev op
Hi Sakari,
On Sun, Mar 21, 2021 at 10:52:56PM +0200, Sakari Ailus wrote:
> Hi Laurent and Jacopo,
>
> Thanks for cc'ing me.
>
> On Sat, Mar 20, 2021 at 05:42:12PM +0200, Laurent Pinchart wrote:
> > Hi Jacopo,
> >
> > Thank you for the patch.
> >
> > CC'ing Sakari on v3 to get feedback.
> >
> > On Fri, Mar 19, 2021 at 05:41:34PM +0100, Jacopo Mondi wrote:
> > > The init() subdev core operation is deemed to be deprecated for new
> > > subdevice drivers. However it could prove useful for complex
> > > architectures to defer operation that require access to the
> > > communication bus if said bus is not available (or fully configured)
> > > at the time when the subdevice probe() function is run.
> > >
> > > As an example, the GMSL architecture requires the GMSL configuration
> > > link to be configured on the host side after the remote subdevice
> > > has completed its probe function. After the configuration on the host
> > > side has been performed, the subdevice registers can be accessed through
> > > the communication bus.
>
> What does the remote device's probe do that needs to be done before bus
> config on the host side?
A few lines here below:
In the GMSL use case the bus configuration requires the enablement of the
noise immunity threshold on the remote side which ensures reliability
of communications in electrically noisy environments. After the subdevice
has enabled the threshold at the end of its probe() sequence the host
side shall compensate it with an higher signal amplitude. Once this
sequence has completed the bus can be accessed with noise protection
enabled and all the operations that require a considerable number of
transactions on the bus (such as the image sensor configuration
sequence) are run in the subdevice init() operation implementation.
>
> Alternatively, could the remote init() work be done at the time streaming
> is started?
That would require programing the sensor, the embedded ISP at s_stream
time which would take some time.
I'll take this suggestion into account though and run some more tests.
Thanks
j
>
> > >
> > > In particular:
> > >
> > > HOSTREMOTE
> > >
> > > probe()
> > > |
> > > -> |
> > > probe() {
> > >bus config()
> > > }
> > > |<|
> > > v4l2 async bound {
> > > bus config()
> > > call subdev init()
> > > |>|
> > >init() {
> > >access register on the bus()
> > > }
> > > |<---
> > > }
> > >
> > > In the GMSL use case the bus configuration requires the enablement of the
> > > noise immunity threshold on the remote side which ensures reliability
> > > of communications in electrically noisy environments. After the subdevice
> > > has enabled the threshold at the end of its probe() sequence the host
> > > side shall compensate it with an higher signal amplitude. Once this
> > > sequence has completed the bus can be accessed with noise protection
> > > enabled and all the operations that require a considerable number of
> > > transactions on the bus (such as the image sensor configuration
> > > sequence) are run in the subdevice init() operation implementation.
> > >
> > > Signed-off-by: Jacopo Mondi
> > > ---
> > > include/media/v4l2-subdev.h | 15 ---
> > > 1 file changed, 12 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> > > index d0e9a5bdb08b..3068d9940669 100644
> > > --- a/include/media/v4l2-subdev.h
> > > +++ b/include/media/v4l2-subdev.h
> > > @@ -148,9 +148,18 @@ struct v4l2_subdev_io_pin_config {
> > > * each pin being configured. This function could be called at
> > > times
> > > * other than just subdevice initialization.
> > > *
> > > - * @init: initialize the sensor registers to some sort of reasonable
> > > default
> > > - * values. Do not use for new drivers and should be removed in
> > > existing
> > > - * drivers.
> > > + * @init: initialize the subdevice registers to some sort of reasonable
> > > default
> > > + * values. Do not
[PATCH v3 17/19] media: i2c: rdacm20: Report camera module name
When the device is identified the driver currently reports the names of the chips embedded in the camera module. Report the name of the camera module itself instead. Cosmetic change only. Reviewed-by: Kieran Bingham Reviewed-by: Laurent Pinchart Signed-off-by: Jacopo Mondi --- drivers/media/i2c/rdacm20.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/media/i2c/rdacm20.c b/drivers/media/i2c/rdacm20.c index 760705dd2918..d3a2520c4113 100644 --- a/drivers/media/i2c/rdacm20.c +++ b/drivers/media/i2c/rdacm20.c @@ -513,7 +513,7 @@ static int rdacm20_init(struct v4l2_subdev *sd, unsigned int val) if (ret) return ret; - dev_info(dev->dev, "Identified MAX9271 + OV10635 device\n"); + dev_info(dev->dev, "Identified RDACM20 camera module\n"); /* * Set reverse channel high threshold to increase noise immunity. -- 2.30.0
[PATCH v3 19/19] media: i2c: rdacm20: Re-work ov10635 reset
The OV10635 image sensor embedded in the camera module is currently
reset after the MAX9271 initialization with two long delays that were
most probably not correctly characterized.
Re-work the image sensor reset procedure by holding the chip in reset
during the MAX9271 configuration, removing the long sleep delays and
only wait after the chip exits from reset for 350-500 microseconds
interval, which is larger than the minimum (2048 * (1 / XVCLK)) timeout
characterized in the chip manual.
Reviewed-by: Kieran Bingham
Reviewed-by: Laurent Pinchart
Signed-off-by: Jacopo Mondi
---
drivers/media/i2c/rdacm20.c | 29 +
1 file changed, 17 insertions(+), 12 deletions(-)
diff --git a/drivers/media/i2c/rdacm20.c b/drivers/media/i2c/rdacm20.c
index 7ed3866b5335..7ba2d0249da8 100644
--- a/drivers/media/i2c/rdacm20.c
+++ b/drivers/media/i2c/rdacm20.c
@@ -454,6 +454,19 @@ static int rdacm20_init(struct v4l2_subdev *sd, unsigned
int val)
if (ret)
return ret;
+ /*
+* Hold OV10635 in reset during max9271 configuration. The reset signal
+* has to be asserted for at least 200 microseconds.
+*/
+ ret = max9271_enable_gpios(>serializer, MAX9271_GPIO1OUT);
+ if (ret)
+ return ret;
+
+ ret = max9271_clear_gpios(>serializer, MAX9271_GPIO1OUT);
+ if (ret)
+ return ret;
+ usleep_range(200, 500);
+
ret = max9271_configure_gmsl_link(>serializer);
if (ret)
return ret;
@@ -468,22 +481,14 @@ static int rdacm20_init(struct v4l2_subdev *sd, unsigned
int val)
dev->serializer.client->addr = dev->addrs[0];
/*
-* Reset the sensor by cycling the OV10635 reset signal connected to the
-* MAX9271 GPIO1 and verify communication with the OV10635.
+* Release ov10635 from reset and initialize it. The image sensor
+* requires at least 2048 XVCLK cycles (85 micro-seconds at 24MHz)
+* before being available. Stay safe and wait up to 500 micro-seconds.
*/
- ret = max9271_enable_gpios(>serializer, MAX9271_GPIO1OUT);
- if (ret)
- return ret;
-
- ret = max9271_clear_gpios(>serializer, MAX9271_GPIO1OUT);
- if (ret)
- return ret;
- usleep_range(1, 15000);
-
ret = max9271_set_gpios(>serializer, MAX9271_GPIO1OUT);
if (ret)
return ret;
- usleep_range(1, 15000);
+ usleep_range(100, 500);
for (i = 0; i < OV10635_PID_TIMEOUT; ++i) {
ret = ov10635_read16(dev, OV10635_PID);
--
2.30.0
[PATCH v3 18/19] media: i2c: rdacm20: Check return values
The camera module initialization routine does not check the return value of a few functions. Fix that. Reviewed-by: Kieran Bingham Reviewed-by: Laurent Pinchart Signed-off-by: Jacopo Mondi --- drivers/media/i2c/rdacm20.c | 13 + 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/drivers/media/i2c/rdacm20.c b/drivers/media/i2c/rdacm20.c index d3a2520c4113..7ed3866b5335 100644 --- a/drivers/media/i2c/rdacm20.c +++ b/drivers/media/i2c/rdacm20.c @@ -447,11 +447,16 @@ static int rdacm20_init(struct v4l2_subdev *sd, unsigned int val) * Ensure that we have a good link configuration before attempting to * identify the device. */ - max9271_configure_i2c(>serializer, MAX9271_I2CSLVSH_469NS_234NS | - MAX9271_I2CSLVTO_1024US | - MAX9271_I2CMSTBT_105KBPS); + ret = max9271_configure_i2c(>serializer, + MAX9271_I2CSLVSH_469NS_234NS | + MAX9271_I2CSLVTO_1024US | + MAX9271_I2CMSTBT_105KBPS); + if (ret) + return ret; - max9271_configure_gmsl_link(>serializer); + ret = max9271_configure_gmsl_link(>serializer); + if (ret) + return ret; ret = max9271_verify_id(>serializer); if (ret < 0) -- 2.30.0
[PATCH v3 16/19] media: i2c: rdacm20: Replace goto with a loop
During the camera module initialization the image sensor PID is read to
verify it can correctly be identified. The current implementation is
rather confused and uses a loop implemented with a label and a goto.
Replace it with a more compact for() loop.
No functional changes intended.
Reviewed-by: Kieran Bingham
Reviewed-by: Laurent Pinchart
Signed-off-by: Jacopo Mondi
---
drivers/media/i2c/rdacm20.c | 29 +
1 file changed, 13 insertions(+), 16 deletions(-)
diff --git a/drivers/media/i2c/rdacm20.c b/drivers/media/i2c/rdacm20.c
index 7bdcfafa6c10..760705dd2918 100644
--- a/drivers/media/i2c/rdacm20.c
+++ b/drivers/media/i2c/rdacm20.c
@@ -59,6 +59,8 @@
*/
#define OV10635_PIXEL_RATE (4400)
+#define OV10635_PID_TIMEOUT3
+
static const struct ov10635_reg {
u16 reg;
u8 val;
@@ -438,7 +440,7 @@ static int rdacm20_get_fmt(struct v4l2_subdev *sd,
static int rdacm20_init(struct v4l2_subdev *sd, unsigned int val)
{
struct rdacm20_device *dev = sd_to_rdacm20(sd);
- unsigned int retry = 3;
+ unsigned int i;
int ret;
/*
@@ -478,23 +480,18 @@ static int rdacm20_init(struct v4l2_subdev *sd, unsigned
int val)
return ret;
usleep_range(1, 15000);
-again:
- ret = ov10635_read16(dev, OV10635_PID);
- if (ret < 0) {
- if (retry--)
- goto again;
+ for (i = 0; i < OV10635_PID_TIMEOUT; ++i) {
+ ret = ov10635_read16(dev, OV10635_PID);
+ if (ret == OV10635_VERSION)
+ break;
+ else if (ret >= 0)
+ /* Sometimes we get a successful read but a wrong ID. */
+ dev_dbg(dev->dev, "OV10635 ID mismatch (%d)\n", ret);
- dev_err(dev->dev, "OV10635 ID read failed (%d)\n",
- ret);
- return -ENXIO;
+ usleep_range(1000, 2000);
}
-
- if (ret != OV10635_VERSION) {
- if (retry--)
- goto again;
-
- dev_err(dev->dev, "OV10635 ID mismatch (0x%04x)\n",
- ret);
+ if (i == OV10635_PID_TIMEOUT) {
+ dev_err(dev->dev, "OV10635 ID read failed (%d)\n", ret);
return -ENXIO;
}
--
2.30.0
[PATCH v3 15/19] media: i2c: rdacm20: Embed 'serializer' field
There's no reason to allocate dynamically the 'serializer' field in
the driver structure.
Embed the field and adjust all its users in the driver.
Reviewed-by: Kieran Bingham
Reviewed-by: Laurent Pinchart
Signed-off-by: Jacopo Mondi
---
drivers/media/i2c/rdacm20.c | 40 -
1 file changed, 17 insertions(+), 23 deletions(-)
diff --git a/drivers/media/i2c/rdacm20.c b/drivers/media/i2c/rdacm20.c
index 554fd3b3ec3d..7bdcfafa6c10 100644
--- a/drivers/media/i2c/rdacm20.c
+++ b/drivers/media/i2c/rdacm20.c
@@ -312,7 +312,7 @@ static const struct ov10635_reg {
struct rdacm20_device {
struct device *dev;
- struct max9271_device *serializer;
+ struct max9271_device serializer;
struct i2c_client *sensor;
struct v4l2_subdev sd;
struct media_padpad;
@@ -399,7 +399,7 @@ static int rdacm20_s_stream(struct v4l2_subdev *sd, int
enable)
{
struct rdacm20_device *dev = sd_to_rdacm20(sd);
- return max9271_set_serial_link(dev->serializer, enable);
+ return max9271_set_serial_link(>serializer, enable);
}
static int rdacm20_enum_mbus_code(struct v4l2_subdev *sd,
@@ -445,35 +445,35 @@ static int rdacm20_init(struct v4l2_subdev *sd, unsigned
int val)
* Ensure that we have a good link configuration before attempting to
* identify the device.
*/
- max9271_configure_i2c(dev->serializer, MAX9271_I2CSLVSH_469NS_234NS |
- MAX9271_I2CSLVTO_1024US |
- MAX9271_I2CMSTBT_105KBPS);
+ max9271_configure_i2c(>serializer, MAX9271_I2CSLVSH_469NS_234NS |
+ MAX9271_I2CSLVTO_1024US |
+ MAX9271_I2CMSTBT_105KBPS);
- max9271_configure_gmsl_link(dev->serializer);
+ max9271_configure_gmsl_link(>serializer);
- ret = max9271_verify_id(dev->serializer);
+ ret = max9271_verify_id(>serializer);
if (ret < 0)
return ret;
- ret = max9271_set_address(dev->serializer, dev->addrs[0]);
+ ret = max9271_set_address(>serializer, dev->addrs[0]);
if (ret < 0)
return ret;
- dev->serializer->client->addr = dev->addrs[0];
+ dev->serializer.client->addr = dev->addrs[0];
/*
* Reset the sensor by cycling the OV10635 reset signal connected to the
* MAX9271 GPIO1 and verify communication with the OV10635.
*/
- ret = max9271_enable_gpios(dev->serializer, MAX9271_GPIO1OUT);
+ ret = max9271_enable_gpios(>serializer, MAX9271_GPIO1OUT);
if (ret)
return ret;
- ret = max9271_clear_gpios(dev->serializer, MAX9271_GPIO1OUT);
+ ret = max9271_clear_gpios(>serializer, MAX9271_GPIO1OUT);
if (ret)
return ret;
usleep_range(1, 15000);
- ret = max9271_set_gpios(dev->serializer, MAX9271_GPIO1OUT);
+ ret = max9271_set_gpios(>serializer, MAX9271_GPIO1OUT);
if (ret)
return ret;
usleep_range(1, 15000);
@@ -558,11 +558,11 @@ static int rdacm20_initialize(struct rdacm20_device *dev)
int ret;
/* Verify communication with the MAX9271: ping to wakeup. */
- dev->serializer->client->addr = MAX9271_DEFAULT_ADDR;
- max9271_wake_up(dev->serializer);
+ dev->serializer.client->addr = MAX9271_DEFAULT_ADDR;
+ max9271_wake_up(>serializer);
/* Serial link disabled during config as it needs a valid pixel clock.
*/
- ret = max9271_set_serial_link(dev->serializer, false);
+ ret = max9271_set_serial_link(>serializer, false);
if (ret)
return ret;
@@ -572,7 +572,7 @@ static int rdacm20_initialize(struct rdacm20_device *dev)
* This should be compensated by increasing the reverse channel
* amplitude on the remote deserializer side.
*/
- return max9271_set_high_threshold(dev->serializer, true);
+ return max9271_set_high_threshold(>serializer, true);
}
static int rdacm20_probe(struct i2c_client *client)
@@ -585,13 +585,7 @@ static int rdacm20_probe(struct i2c_client *client)
if (!dev)
return -ENOMEM;
dev->dev = >dev;
-
- dev->serializer = devm_kzalloc(>dev, sizeof(*dev->serializer),
- GFP_KERNEL);
- if (!dev->serializer)
- return -ENOMEM;
-
- dev->serializer->client = client;
+ dev->serializer.client = client;
ret = of_property_read_u32_array(client->dev.of_node, "reg",
dev->addrs, 2);
--
2.30.0
[PATCH v3 14/19] media: i2c: rdacm20: Enable noise immunity
Enable the noise immunity threshold at the end of the rdacm20
initialization routine.
The rdacm20 camera module has been so far tested with a startup
delay that allowed the embedded MCU to program the serializer. If
the initialization routine is run before the MCU programs the
serializer and the image sensor and their addresses gets changed
by the rdacm20 driver it is required to manually enable the noise
immunity threshold to make the communication on the control channel
more reliable.
Reviewed-by: Kieran Bingham
Signed-off-by: Jacopo Mondi
---
drivers/media/i2c/rdacm20.c | 14 +-
1 file changed, 13 insertions(+), 1 deletion(-)
diff --git a/drivers/media/i2c/rdacm20.c b/drivers/media/i2c/rdacm20.c
index 2881e752efbe..554fd3b3ec3d 100644
--- a/drivers/media/i2c/rdacm20.c
+++ b/drivers/media/i2c/rdacm20.c
@@ -518,7 +518,19 @@ static int rdacm20_init(struct v4l2_subdev *sd, unsigned
int val)
dev_info(dev->dev, "Identified MAX9271 + OV10635 device\n");
- return 0;
+ /*
+* Set reverse channel high threshold to increase noise immunity.
+*
+* This should be compensated by increasing the reverse channel
+* amplitude on the remote deserializer side.
+*
+* TODO Inspect the embedded MCU programming sequence to make sure
+* there are no conflicts with the configuration applied here.
+*
+* TODO Clarify the embedded MCU startup delay to avoid write
+* collisions on the I2C bus.
+*/
+ return max9271_set_high_threshold(>serializer, true);
}
static const struct v4l2_subdev_core_ops rdacm20_core_ops = {
--
2.30.0
[PATCH v3 13/19] media: i2c: rdacm21: Power up OV10640 before OV490
The current RDACM21 initialization routine powers up the
OV10640 image sensor after the OV490 ISP. The ISP is programmed with
a firmware loaded from an embedded EEPROM that (most probably) tries
to interact and program also the image sensor connected to the ISP.
As described in commit ccb26c5742f5 ("media: i2c: rdacm21: Fix OV10640
powerup") the image sensor powerdown signal is kept high by an internal
pull up resistor and occasionally fails to startup correctly if the
powerdown line is not asserted explicitly. Failures in the OV10640
startup causes the OV490 firmware to fail to boot correctly resulting in
the camera module initialization to fail consequentially.
Fix this by powering up the OV10640 image sensor before testing the
OV490 firmware boot completion, by splitting the ov10640_initialize()
function in an ov10640_power_up() one and an ov10640_check_id() one.
Also make sure the OV10640 identification procedure gives enough time to
the image sensor to resume after the programming phase performed by the
OV490 firmware by repeating the ID read procedure,
This commit fixes a sporadic start-up error triggered by a failure to
detect the OV490 firmware boot completion:
rdacm21 8-0054: Timeout waiting for firmware boot
Fixes: a59f853b3b4b ("media: i2c: Add driver for RDACM21 camera module")
Signed-off-by: Jacopo Mondi
---
drivers/media/i2c/rdacm21.c | 46 ++---
1 file changed, 32 insertions(+), 14 deletions(-)
diff --git a/drivers/media/i2c/rdacm21.c b/drivers/media/i2c/rdacm21.c
index 3f38c465b348..3763eb690d74 100644
--- a/drivers/media/i2c/rdacm21.c
+++ b/drivers/media/i2c/rdacm21.c
@@ -69,6 +69,7 @@
#define OV490_ISP_VSIZE_LOW0x80820062
#define OV490_ISP_VSIZE_HIGH 0x80820063
+#define OV10640_PID_TIMEOUT20
#define OV10640_ID_HIGH0xa6
#define OV10640_CHIP_ID0x300a
#define OV10640_PIXEL_RATE 5500
@@ -314,10 +315,8 @@ static int rdacm21_get_fmt(struct v4l2_subdev *sd,
return 0;
}
-static int ov10640_initialize(struct rdacm21_device *dev)
+static void ov10640_power_up(struct rdacm21_device *dev)
{
- u8 val;
-
/* Enable GPIO0#0 (reset) and GPIO1#0 (pwdn) as output lines. */
ov490_write_reg(dev, OV490_GPIO_SEL0, OV490_GPIO0);
ov490_write_reg(dev, OV490_GPIO_SEL1, OV490_SPWDN0);
@@ -332,18 +331,35 @@ static int ov10640_initialize(struct rdacm21_device *dev)
usleep_range(1500, 3000);
ov490_write_reg(dev, OV490_GPIO_OUTPUT_VALUE0, OV490_GPIO0);
usleep_range(3000, 5000);
+}
- /* Read OV10640 ID to test communications. */
- ov490_write_reg(dev, OV490_SCCB_SLAVE0_DIR, OV490_SCCB_SLAVE_READ);
- ov490_write_reg(dev, OV490_SCCB_SLAVE0_ADDR_HIGH, OV10640_CHIP_ID >> 8);
- ov490_write_reg(dev, OV490_SCCB_SLAVE0_ADDR_LOW, OV10640_CHIP_ID &
0xff);
-
- /* Trigger SCCB slave transaction and give it some time to complete. */
- ov490_write_reg(dev, OV490_HOST_CMD, OV490_HOST_CMD_TRIGGER);
- usleep_range(1000, 1500);
+static int ov10640_check_id(struct rdacm21_device *dev)
+{
+ unsigned int i;
+ u8 val;
- ov490_read_reg(dev, OV490_SCCB_SLAVE0_DIR, );
- if (val != OV10640_ID_HIGH) {
+ /* Read OV10640 ID to test communications. */
+ for (i = 0; i < OV10640_PID_TIMEOUT; ++i) {
+ ov490_write_reg(dev, OV490_SCCB_SLAVE0_DIR,
+ OV490_SCCB_SLAVE_READ);
+ ov490_write_reg(dev, OV490_SCCB_SLAVE0_ADDR_HIGH,
+ OV10640_CHIP_ID >> 8);
+ ov490_write_reg(dev, OV490_SCCB_SLAVE0_ADDR_LOW,
+ OV10640_CHIP_ID & 0xff);
+
+ /*
+* Trigger SCCB slave transaction and give it some time
+* to complete.
+*/
+ ov490_write_reg(dev, OV490_HOST_CMD, OV490_HOST_CMD_TRIGGER);
+ usleep_range(1000, 1500);
+
+ ov490_read_reg(dev, OV490_SCCB_SLAVE0_DIR, );
+ if (val == OV10640_ID_HIGH)
+ break;
+ usleep_range(1000, 1500);
+ }
+ if (i == OV10640_PID_TIMEOUT) {
dev_err(dev->dev, "OV10640 ID mismatch: (0x%02x)\n", val);
return -ENODEV;
}
@@ -359,6 +375,8 @@ static int ov490_initialize(struct rdacm21_device *dev)
unsigned int i;
int ret;
+ ov10640_power_up(dev);
+
/*
* Read OV490 Id to test communications. Give it up to 40msec to
* exit from reset.
@@ -396,7 +414,7 @@ static int ov490_initialize(struct rdacm21_device *dev)
return -ENODEV;
}
- ret = ov10640_initialize(dev);
+ ret = ov10640_check_id(dev);
if (ret)
return ret;
--
2.30.0
[PATCH v3 08/19] media: i2c: max9271: Check max9271_write() return
Check the return value of the max9271_write() function in the
max9271 library driver.
While at it, modify an existing condition to be made identical
to other checks.
Reviewed-by: Kieran Bingham
Reviewed-by: Laurent Pinchart
Signed-off-by: Jacopo Mondi
---
drivers/media/i2c/max9271.c | 30 +++---
1 file changed, 23 insertions(+), 7 deletions(-)
diff --git a/drivers/media/i2c/max9271.c b/drivers/media/i2c/max9271.c
index c495582dcff6..2c7dc7fb9846 100644
--- a/drivers/media/i2c/max9271.c
+++ b/drivers/media/i2c/max9271.c
@@ -106,7 +106,10 @@ int max9271_set_serial_link(struct max9271_device *dev,
bool enable)
* Short delays here appear to show bit-errors in the writes following.
* Therefore a conservative delay seems best here.
*/
- max9271_write(dev, 0x04, val);
+ ret = max9271_write(dev, 0x04, val);
+ if (ret < 0)
+ return ret;
+
usleep_range(5000, 8000);
return 0;
@@ -118,7 +121,7 @@ int max9271_configure_i2c(struct max9271_device *dev, u8
i2c_config)
int ret;
ret = max9271_write(dev, 0x0d, i2c_config);
- if (ret)
+ if (ret < 0)
return ret;
/* The delay required after an I2C bus configuration change is not
@@ -143,7 +146,10 @@ int max9271_set_high_threshold(struct max9271_device *dev,
bool enable)
* Enable or disable reverse channel high threshold to increase
* immunity to power supply noise.
*/
- max9271_write(dev, 0x08, enable ? ret | BIT(0) : ret & ~BIT(0));
+ ret = max9271_write(dev, 0x08, enable ? ret | BIT(0) : ret & ~BIT(0));
+ if (ret < 0)
+ return ret;
+
usleep_range(2000, 2500);
return 0;
@@ -152,6 +158,8 @@ EXPORT_SYMBOL_GPL(max9271_set_high_threshold);
int max9271_configure_gmsl_link(struct max9271_device *dev)
{
+ int ret;
+
/*
* Configure the GMSL link:
*
@@ -162,16 +170,24 @@ int max9271_configure_gmsl_link(struct max9271_device
*dev)
*
* TODO: Make the GMSL link configuration parametric.
*/
- max9271_write(dev, 0x07, MAX9271_DBL | MAX9271_HVEN |
- MAX9271_EDC_1BIT_PARITY);
+ ret = max9271_write(dev, 0x07, MAX9271_DBL | MAX9271_HVEN |
+ MAX9271_EDC_1BIT_PARITY);
+ if (ret < 0)
+ return ret;
+
usleep_range(5000, 8000);
/*
* Adjust spread spectrum to +4% and auto-detect pixel clock
* and serial link rate.
*/
- max9271_write(dev, 0x02, MAX9271_SPREAD_SPECT_4 | MAX9271_R02_RES |
- MAX9271_PCLK_AUTODETECT | MAX9271_SERIAL_AUTODETECT);
+ ret = max9271_write(dev, 0x02,
+ MAX9271_SPREAD_SPECT_4 | MAX9271_R02_RES |
+ MAX9271_PCLK_AUTODETECT |
+ MAX9271_SERIAL_AUTODETECT);
+ if (ret < 0)
+ return ret;
+
usleep_range(5000, 8000);
return 0;
--
2.30.0
[PATCH v3 10/19] media: i2c: rdamc21: Fix warning on u8 cast
Sparse reports a warning on a cast to u8 of a 16 bits constant.
drivers/media/i2c/rdacm21.c:348:62: warning: cast truncates bits
from constant value (300a becomes a)
Even if the behaviour is intended, silence the sparse warning replacing
the cast with a bitwise & operation.
Fixes: a59f853b3b4b ("media: i2c: Add driver for RDACM21 camera module")
Reported-by: Hans Verkuil
Reviewed-by: Kieran Bingham
Reviewed-by: Laurent Pinchart
Signed-off-by: Jacopo Mondi
---
drivers/media/i2c/rdacm21.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/media/i2c/rdacm21.c b/drivers/media/i2c/rdacm21.c
index 1706a59bfd58..babd14b21252 100644
--- a/drivers/media/i2c/rdacm21.c
+++ b/drivers/media/i2c/rdacm21.c
@@ -330,7 +330,7 @@ static int ov10640_initialize(struct rdacm21_device *dev)
/* Read OV10640 ID to test communications. */
ov490_write_reg(dev, OV490_SCCB_SLAVE0_DIR, OV490_SCCB_SLAVE_READ);
ov490_write_reg(dev, OV490_SCCB_SLAVE0_ADDR_HIGH, OV10640_CHIP_ID >> 8);
- ov490_write_reg(dev, OV490_SCCB_SLAVE0_ADDR_LOW, (u8)OV10640_CHIP_ID);
+ ov490_write_reg(dev, OV490_SCCB_SLAVE0_ADDR_LOW, OV10640_CHIP_ID &
0xff);
/* Trigger SCCB slave transaction and give it some time to complete. */
ov490_write_reg(dev, OV490_HOST_CMD, OV490_HOST_CMD_TRIGGER);
--
2.30.0
[PATCH v3 11/19] media: i2c: rdacm21: Add dealy after OV490 reset
Add a delay after the OV490 chip is put in reset state. The reset signal shall be held for at least 250 useconds. Signed-off-by: Jacopo Mondi --- drivers/media/i2c/rdacm21.c | 6 +- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/drivers/media/i2c/rdacm21.c b/drivers/media/i2c/rdacm21.c index babd14b21252..875bec9f7904 100644 --- a/drivers/media/i2c/rdacm21.c +++ b/drivers/media/i2c/rdacm21.c @@ -448,7 +448,10 @@ static int rdacm21_init(struct v4l2_subdev *sd, unsigned int val) if (ret) return ret; - /* Enable GPIO1 and hold OV490 in reset during max9271 configuration. */ + /* +* Enable GPIO1 and hold OV490 in reset during max9271 configuration. +* The reset signal has to be asserted for at least 250 useconds. +*/ ret = max9271_enable_gpios(>serializer, MAX9271_GPIO1OUT); if (ret) return ret; @@ -456,6 +459,7 @@ static int rdacm21_init(struct v4l2_subdev *sd, unsigned int val) ret = max9271_clear_gpios(>serializer, MAX9271_GPIO1OUT); if (ret) return ret; + usleep_range(250, 500); ret = max9271_configure_gmsl_link(>serializer); if (ret) -- 2.30.0
[PATCH v3 12/19] media: i2c: rdacm21: Fix OV10640 powerup
The OV10640 image sensor powerdown signal is controlled by the first
line of the OV490 GPIO pad #1, but the pad #0 identifier
OV490_GPIO_OUTPUT_VALUE0 was erroneously used. As a result the image
sensor powerdown signal was never asserted but was left floating and
kept high by an internal pull-up resistor, causing sporadic failures
during the image sensor startup phase.
Fix this by using the correct GPIO pad identifier and wait the mandatory
1.5 millisecond delay after the powerup lane is asserted. The reset
delay is not characterized in the chip manual if not as "255 XVCLK +
initialization". Wait for at least 3 milliseconds to guarantee the SCCB
bus is available.
While at it also fix the reset sequence, as the reset line was released
before the powerdown one, and the line was not cycled.
This commit fixes a sporadic start-up error triggered by a failure to
read the OV10640 chip ID:
rdacm21 8-0054: OV10640 ID mismatch: (0x01)
Fixes: a59f853b3b4b ("media: i2c: Add driver for RDACM21 camera module")
Reviewed-by: Kieran Bingham
Reviewed-by: Laurent Pinchart
Signed-off-by: Jacopo Mondi
---
drivers/media/i2c/rdacm21.c | 10 --
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/drivers/media/i2c/rdacm21.c b/drivers/media/i2c/rdacm21.c
index 875bec9f7904..3f38c465b348 100644
--- a/drivers/media/i2c/rdacm21.c
+++ b/drivers/media/i2c/rdacm21.c
@@ -318,13 +318,19 @@ static int ov10640_initialize(struct rdacm21_device *dev)
{
u8 val;
- /* Power-up OV10640 by setting RESETB and PWDNB pins high. */
+ /* Enable GPIO0#0 (reset) and GPIO1#0 (pwdn) as output lines. */
ov490_write_reg(dev, OV490_GPIO_SEL0, OV490_GPIO0);
ov490_write_reg(dev, OV490_GPIO_SEL1, OV490_SPWDN0);
ov490_write_reg(dev, OV490_GPIO_DIRECTION0, OV490_GPIO0);
ov490_write_reg(dev, OV490_GPIO_DIRECTION1, OV490_SPWDN0);
+
+ /* Power up OV10640 and then reset it. */
+ ov490_write_reg(dev, OV490_GPIO_OUTPUT_VALUE1, OV490_SPWDN0);
+ usleep_range(1500, 3000);
+
+ ov490_write_reg(dev, OV490_GPIO_OUTPUT_VALUE0, 0x00);
+ usleep_range(1500, 3000);
ov490_write_reg(dev, OV490_GPIO_OUTPUT_VALUE0, OV490_GPIO0);
- ov490_write_reg(dev, OV490_GPIO_OUTPUT_VALUE0, OV490_SPWDN0);
usleep_range(3000, 5000);
/* Read OV10640 ID to test communications. */
--
2.30.0
[PATCH v3 09/19] media: i2c: max9271: Introduce wake_up() function
The MAX9271 chip manual prescribes a delay of 5 milliseconds
after the chip exits from low power state.
Add a new function to the max9271 library driver that wakes up the chip
with a dummy i2c transaction and implements the correct delay of 5
milliseconds after the chip exits from low power state.
Use the newly introduced function in the rdacm20 and rdacm21 camera
drivers. The former was not respecting the required delay while the
latter was waiting for a too-short timeout.
Do not handle the initial i2c address configuration in the library
driver function as the camera module drivers control address
reprogramming.
Reviewed-by: Kieran Bingham
Reviewed-by: Laurent Pinchart
Signed-off-by: Jacopo Mondi
---
drivers/media/i2c/max9271.c | 7 +++
drivers/media/i2c/max9271.h | 9 +
drivers/media/i2c/rdacm20.c | 2 +-
drivers/media/i2c/rdacm21.c | 3 +--
4 files changed, 18 insertions(+), 3 deletions(-)
diff --git a/drivers/media/i2c/max9271.c b/drivers/media/i2c/max9271.c
index 2c7dc7fb9846..f7bfe7266763 100644
--- a/drivers/media/i2c/max9271.c
+++ b/drivers/media/i2c/max9271.c
@@ -80,6 +80,13 @@ static int max9271_pclk_detect(struct max9271_device *dev)
return -EIO;
}
+void max9271_wake_up(struct max9271_device *dev)
+{
+ i2c_smbus_read_byte(dev->client);
+ usleep_range(5000, 8000);
+}
+EXPORT_SYMBOL_GPL(max9271_wake_up);
+
int max9271_set_serial_link(struct max9271_device *dev, bool enable)
{
int ret;
diff --git a/drivers/media/i2c/max9271.h b/drivers/media/i2c/max9271.h
index d78fb21441e9..dc5e4e70ba6f 100644
--- a/drivers/media/i2c/max9271.h
+++ b/drivers/media/i2c/max9271.h
@@ -85,6 +85,15 @@ struct max9271_device {
struct i2c_client *client;
};
+/**
+ * max9271_wake_up() - Wake up the serializer by issuing an i2c transaction
+ * @dev: The max9271 device
+ *
+ * This function shall be called before any other interaction with the
+ * serializer.
+ */
+void max9271_wake_up(struct max9271_device *dev);
+
/**
* max9271_set_serial_link() - Enable/disable serial link
* @dev: The max9271 device
diff --git a/drivers/media/i2c/rdacm20.c b/drivers/media/i2c/rdacm20.c
index 3cab6af7eba6..2881e752efbe 100644
--- a/drivers/media/i2c/rdacm20.c
+++ b/drivers/media/i2c/rdacm20.c
@@ -547,7 +547,7 @@ static int rdacm20_initialize(struct rdacm20_device *dev)
/* Verify communication with the MAX9271: ping to wakeup. */
dev->serializer->client->addr = MAX9271_DEFAULT_ADDR;
- i2c_smbus_read_byte(dev->serializer->client);
+ max9271_wake_up(dev->serializer);
/* Serial link disabled during config as it needs a valid pixel clock.
*/
ret = max9271_set_serial_link(dev->serializer, false);
diff --git a/drivers/media/i2c/rdacm21.c b/drivers/media/i2c/rdacm21.c
index 318a3b40d9e6..1706a59bfd58 100644
--- a/drivers/media/i2c/rdacm21.c
+++ b/drivers/media/i2c/rdacm21.c
@@ -511,8 +511,7 @@ static int rdacm21_initialize(struct rdacm21_device *dev)
/* Verify communication with the MAX9271: ping to wakeup. */
dev->serializer.client->addr = MAX9271_DEFAULT_ADDR;
- i2c_smbus_read_byte(dev->serializer.client);
- usleep_range(3000, 5000);
+ max9271_wake_up(>serializer);
/* Enable reverse channel and disable the serial link. */
ret = max9271_set_serial_link(>serializer, false);
--
2.30.0
[PATCH v3 07/19] media: i2c: max9286: Rework comments in .bound()
Rephrase a comment in .bound() callback to make it clear we register a subdev notifier and remove a redundant comment about disabling i2c auto-ack. No functional changes intended. Reviewed-by: Kieran Bingham Reviewed-by: Laurent Pinchart Signed-off-by: Jacopo Mondi --- drivers/media/i2c/max9286.c | 16 ++-- 1 file changed, 6 insertions(+), 10 deletions(-) diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c index b6347639901e..16b2cb9b44a2 100644 --- a/drivers/media/i2c/max9286.c +++ b/drivers/media/i2c/max9286.c @@ -556,9 +556,9 @@ static int max9286_notify_bound(struct v4l2_async_notifier *notifier, subdev->name, src_pad, index); /* -* We can only register v4l2_async_notifiers, which do not provide a -* means to register a complete callback. bound_sources allows us to -* identify when all remote serializers have completed their probe. +* As we register a subdev notifiers we won't get a .complete() callback +* here, so we have to use bound_sources to identify when all remote +* serializers have probed. */ if (priv->bound_sources != priv->source_mask) return 0; @@ -581,16 +581,12 @@ static int max9286_notify_bound(struct v4l2_async_notifier *notifier, /* * All enabled sources have probed and enabled their reverse control * channels: +* - The reverse channel amplitude stays high * - Verify all configuration links are properly detected -* - Disable auto-ack as communication on the control channel are now -* stable. +* - Disable auto-ack as communications on the control channel are now +* stable */ max9286_check_config_link(priv, priv->source_mask); - - /* -* Re-configure I2C with local acknowledge disabled after cameras have -* probed. -*/ max9286_configure_i2c(priv, false); return max9286_set_pixelrate(priv); -- 2.30.0
[PATCH v3 06/19] media: gmsl: Reimplement initialization sequence
The current probe() procedure of the RDACM20 and RDACM20 GMSL cameras is
performed with the embedded MAX9271 serializer's noise immunity
threshold disabled. Once the camera has been initialized by probing the
embedded chips, the threshold is enabled and then compensated on the
deserializer's side by increasing the reverse channel signal amplitude
once all cameras have bound.
The probe routine is thus run without noise immunity activated which
in noisy environment conditions makes the probe sequence less reliable as
the chips configuration requires a relatively high amount of i2c
transactions.
Break chip initialization in two:
- At probe time only configure the serializer's reverse channel with
noise immunity activated, to reduce the number of transactions
performed without noise immunity protection enabled
- Move the chips initialization to the .init() core subdev operation
called by the deserializer after all camera have probed and
have increased their noise immunity threshold
The initialization routine looks like the following:
MAX9286 RDACM20/21
probe()
|
-> |
probe() {
enable_threshold()
}
|<|
v4l2 async bound {
compensate_amplitude()
call subdev init()
|>|
init() {
access camera registers()
}
|<---
}
Reviewed-by: Laurent Pinchart
Reviewed-by: Kieran Bingham
Signed-off-by: Jacopo Mondi
---
drivers/media/i2c/max9286.c | 19 +++---
drivers/media/i2c/rdacm20.c | 69 +++--
drivers/media/i2c/rdacm21.c | 65 --
3 files changed, 98 insertions(+), 55 deletions(-)
diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
index 9124d5fa6ea3..b6347639901e 100644
--- a/drivers/media/i2c/max9286.c
+++ b/drivers/media/i2c/max9286.c
@@ -563,17 +563,28 @@ static int max9286_notify_bound(struct
v4l2_async_notifier *notifier,
if (priv->bound_sources != priv->source_mask)
return 0;
+ /*
+* Initialize all the remote camera. Increase the channel amplitude
+* to compensate for the remote noise immunity threshold.
+*/
+ max9286_reverse_channel_setup(priv, MAX9286_REV_AMP_HIGH);
+ for_each_source(priv, source) {
+ ret = v4l2_subdev_call(source->sd, core, init, 0);
+ if (ret) {
+ dev_err(>client->dev,
+ "Failed to initialize camera device %u\n",
+ index);
+ return ret;
+ }
+ }
+
/*
* All enabled sources have probed and enabled their reverse control
* channels:
-*
-* - Increase the reverse channel amplitude to compensate for the
-* remote ends high threshold
* - Verify all configuration links are properly detected
* - Disable auto-ack as communication on the control channel are now
* stable.
*/
- max9286_reverse_channel_setup(priv, MAX9286_REV_AMP_HIGH);
max9286_check_config_link(priv, priv->source_mask);
/*
diff --git a/drivers/media/i2c/rdacm20.c b/drivers/media/i2c/rdacm20.c
index 90eb73f0e6e9..3cab6af7eba6 100644
--- a/drivers/media/i2c/rdacm20.c
+++ b/drivers/media/i2c/rdacm20.c
@@ -435,35 +435,12 @@ static int rdacm20_get_fmt(struct v4l2_subdev *sd,
return 0;
}
-static const struct v4l2_subdev_video_ops rdacm20_video_ops = {
- .s_stream = rdacm20_s_stream,
-};
-
-static const struct v4l2_subdev_pad_ops rdacm20_subdev_pad_ops = {
- .enum_mbus_code = rdacm20_enum_mbus_code,
- .get_fmt= rdacm20_get_fmt,
- .set_fmt= rdacm20_get_fmt,
-};
-
-static const struct v4l2_subdev_ops rdacm20_subdev_ops = {
- .video = _video_ops,
- .pad= _subdev_pad_ops,
-};
-
-static int rdacm20_initialize(struct rdacm20_device *dev)
+static int rdacm20_init(struct v4l2_subdev *sd, unsigned int val)
{
+ struct rdacm20_device *dev = sd_to_rdacm20(sd);
unsigned int retry = 3;
int ret;
- /* Verify communication with the MAX9271: ping to wakeup. */
- dev->serializer->client->addr = MAX9271_DEFAULT_ADDR;
- i2c_smbus_read_byte(dev->serializer->client);
-
- /* Serial link disabled during config as it needs a valid pixel clock.
*/
- ret = max9271_set_serial_link(dev->serializer, false);
- if (ret)
- return ret;
-
/*
* Ensur
[PATCH v3 05/19] media: v4l2-subdev: De-deprecate init() subdev op
The init() subdev core operation is deemed to be deprecated for new
subdevice drivers. However it could prove useful for complex
architectures to defer operation that require access to the
communication bus if said bus is not available (or fully configured)
at the time when the subdevice probe() function is run.
As an example, the GMSL architecture requires the GMSL configuration
link to be configured on the host side after the remote subdevice
has completed its probe function. After the configuration on the host
side has been performed, the subdevice registers can be accessed through
the communication bus.
In particular:
HOSTREMOTE
probe()
|
-> |
probe() {
bus config()
}
|<|
v4l2 async bound {
bus config()
call subdev init()
|>|
init() {
access register on the bus()
}
|<---
}
In the GMSL use case the bus configuration requires the enablement of the
noise immunity threshold on the remote side which ensures reliability
of communications in electrically noisy environments. After the subdevice
has enabled the threshold at the end of its probe() sequence the host
side shall compensate it with an higher signal amplitude. Once this
sequence has completed the bus can be accessed with noise protection
enabled and all the operations that require a considerable number of
transactions on the bus (such as the image sensor configuration
sequence) are run in the subdevice init() operation implementation.
Signed-off-by: Jacopo Mondi
---
include/media/v4l2-subdev.h | 15 ---
1 file changed, 12 insertions(+), 3 deletions(-)
diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
index d0e9a5bdb08b..3068d9940669 100644
--- a/include/media/v4l2-subdev.h
+++ b/include/media/v4l2-subdev.h
@@ -148,9 +148,18 @@ struct v4l2_subdev_io_pin_config {
* each pin being configured. This function could be called at times
* other than just subdevice initialization.
*
- * @init: initialize the sensor registers to some sort of reasonable default
- * values. Do not use for new drivers and should be removed in existing
- * drivers.
+ * @init: initialize the subdevice registers to some sort of reasonable default
+ * values. Do not use for new drivers (and should be removed in existing
+ * ones) for regular architectures where the image sensor is connected to
+ * the host receiver. For more complex architectures where the subdevice
+ * initialization should be deferred to the completion of the probe
+ * sequence of some intermediate component, or the communication bus
+ * requires configurations on the host side that depend on the completion
+ * of the probe sequence of the remote subdevices, the usage of this
+ * operation could be considered to allow the devices along the pipeline to
+ * probe and register in the media graph and to defer any operation that
+ * require actual access to the communication bus to their init() function
+ * implementation.
*
* @load_fw: load firmware.
*
--
2.30.0
[PATCH v3 04/19] media: i2c: max9286: Define high channel amplitude
Provide a macro to define the reverse channel amplitude to be used to compensate the remote serializer noise immunity. While at it, update a comment. Reviewed-by: Kieran Bingham Reviewed-by: Laurent Pinchart Signed-off-by: Jacopo Mondi --- drivers/media/i2c/max9286.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c index ed1cdefe7c30..9124d5fa6ea3 100644 --- a/drivers/media/i2c/max9286.c +++ b/drivers/media/i2c/max9286.c @@ -113,6 +113,7 @@ #define MAX9286_REV_TRF(n) ((n) << 4) #define MAX9286_REV_AMP(n) n) - 30) / 10) << 1) /* in mV */ #define MAX9286_REV_AMP_X BIT(0) +#define MAX9286_REV_AMP_HIGH 170 /* Register 0x3f */ #define MAX9286_EN_REV_CFG BIT(6) #define MAX9286_REV_FLEN(n)((n) - 20) @@ -567,12 +568,12 @@ static int max9286_notify_bound(struct v4l2_async_notifier *notifier, * channels: * * - Increase the reverse channel amplitude to compensate for the -* remote ends high threshold, if not done already +* remote ends high threshold * - Verify all configuration links are properly detected * - Disable auto-ack as communication on the control channel are now * stable. */ - max9286_reverse_channel_setup(priv, 170); + max9286_reverse_channel_setup(priv, MAX9286_REV_AMP_HIGH); max9286_check_config_link(priv, priv->source_mask); /* -- 2.30.0
[PATCH v3 01/19] media: i2c: max9286: Adjust parameters indent
The parameters to max9286_i2c_mux_configure() fits on the previous line. Adjust it. Cosmetic change only. Reviewed-by: Kieran Bingham Reviewed-by: Laurent Pinchart Signed-off-by: Jacopo Mondi --- drivers/media/i2c/max9286.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c index 6fd4d59fcc72..1d9951215868 100644 --- a/drivers/media/i2c/max9286.c +++ b/drivers/media/i2c/max9286.c @@ -287,9 +287,8 @@ static int max9286_i2c_mux_select(struct i2c_mux_core *muxc, u32 chan) priv->mux_channel = chan; - max9286_i2c_mux_configure(priv, - MAX9286_FWDCCEN(chan) | - MAX9286_REVCCEN(chan)); + max9286_i2c_mux_configure(priv, MAX9286_FWDCCEN(chan) | + MAX9286_REVCCEN(chan)); return 0; } -- 2.30.0
[PATCH v3 02/19] media: i2c: max9286: Rename reverse_channel_mv
Rename the reverse_channel_mv variable to init_rev_chan_mv as
the next patch will cache the reverse channel amplitude in
a new driver variable.
Reviewed-by: Kieran Bingham
Reviewed-by: Laurent Pinchart
Signed-off-by: Jacopo Mondi
---
drivers/media/i2c/max9286.c | 11 ++-
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
index 1d9951215868..82ec05e96cb7 100644
--- a/drivers/media/i2c/max9286.c
+++ b/drivers/media/i2c/max9286.c
@@ -163,7 +163,8 @@ struct max9286_priv {
unsigned int mux_channel;
bool mux_open;
- u32 reverse_channel_mv;
+ /* The initial reverse control channel amplitude. */
+ u32 init_rev_chan_mv;
struct v4l2_ctrl_handler ctrls;
struct v4l2_ctrl *pixelrate;
@@ -563,7 +564,7 @@ static int max9286_notify_bound(struct v4l2_async_notifier
*notifier,
* - Disable auto-ack as communication on the control channel are now
* stable.
*/
- if (priv->reverse_channel_mv < 170)
+ if (priv->init_rev_chan_mv < 170)
max9286_reverse_channel_setup(priv, 170);
max9286_check_config_link(priv, priv->source_mask);
@@ -971,7 +972,7 @@ static int max9286_setup(struct max9286_priv *priv)
* only. This should be disabled after the mux is initialised.
*/
max9286_configure_i2c(priv, true);
- max9286_reverse_channel_setup(priv, priv->reverse_channel_mv);
+ max9286_reverse_channel_setup(priv, priv->init_rev_chan_mv);
/*
* Enable GMSL links, mask unused ones and autodetect link
@@ -1236,9 +1237,9 @@ static int max9286_parse_dt(struct max9286_priv *priv)
if (of_property_read_u32(dev->of_node,
"maxim,reverse-channel-microvolt",
_channel_microvolt))
- priv->reverse_channel_mv = 170;
+ priv->init_rev_chan_mv = 170;
else
- priv->reverse_channel_mv = reverse_channel_microvolt / 1000U;
+ priv->init_rev_chan_mv = reverse_channel_microvolt / 1000U;
priv->route_mask = priv->source_mask;
--
2.30.0
[PATCH v3 03/19] media: i2c: max9286: Cache channel amplitude
Cache the current channel amplitude in a driver variable
to skip updating it if the newly requested value is the same
as the currently configured one.
Reviewed-by: Kieran Bingham
Reviewed-by: Laurent Pinchart
Signed-off-by: Jacopo Mondi
---
drivers/media/i2c/max9286.c | 13 ++---
1 file changed, 10 insertions(+), 3 deletions(-)
diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
index 82ec05e96cb7..ed1cdefe7c30 100644
--- a/drivers/media/i2c/max9286.c
+++ b/drivers/media/i2c/max9286.c
@@ -165,6 +165,7 @@ struct max9286_priv {
/* The initial reverse control channel amplitude. */
u32 init_rev_chan_mv;
+ u32 rev_chan_mv;
struct v4l2_ctrl_handler ctrls;
struct v4l2_ctrl *pixelrate;
@@ -341,8 +342,15 @@ static void max9286_configure_i2c(struct max9286_priv
*priv, bool localack)
static void max9286_reverse_channel_setup(struct max9286_priv *priv,
unsigned int chan_amplitude)
{
+ u8 chan_config;
+
+ if (priv->rev_chan_mv == chan_amplitude)
+ return;
+
+ priv->rev_chan_mv = chan_amplitude;
+
/* Reverse channel transmission time: default to 1. */
- u8 chan_config = MAX9286_REV_TRF(1);
+ chan_config = MAX9286_REV_TRF(1);
/*
* Reverse channel setup.
@@ -564,8 +572,7 @@ static int max9286_notify_bound(struct v4l2_async_notifier
*notifier,
* - Disable auto-ack as communication on the control channel are now
* stable.
*/
- if (priv->init_rev_chan_mv < 170)
- max9286_reverse_channel_setup(priv, 170);
+ max9286_reverse_channel_setup(priv, 170);
max9286_check_config_link(priv, priv->source_mask);
/*
--
2.30.0
[PATCH v3 00/19] media: gmsl: Reliability improvement
Hello, series following: https://patchwork.linuxtv.org/project/linux-media/list/?series=4650 https://patchwork.linuxtv.org/project/linux-media/list/?series=4861 Compared to the following iteration it seems RDACM21 is finally fixed \o/ I'll sum-up the issue here from the last email exchange: --- Basically, as patch "media: i2c: rdacm21: Fix OV10640 powerdown" of this series describes, the OV10640 power-up was broken before you spotted the usage of the wrong gpio pad and it was working because of an internal pull-up on the SPWDN line, which was erroneously left floating. Once that was fixed, the OV10640 was always identified correctly, leaving us with this puzzling "ov490 boot timeout error" that manifested with more or less the same frequency of the ov10640 identification issue. In the current implementation we power up the OV490 and wait for its firmware to boot -before- powering up the ov10640 sensor. Most probably (or looking at the results I get noaw, most certainly) the OV490 firmware checks for the sensor to be available and probably tries to program it. So we're back to the issue we originally had when the sensor was powered because of the pull up resistor, failing to boot in case the sensor didn't startup correctly which happened in the 20% of the cases. If I do power up the OV10640 -before- the OV490 all the firmware boot errors are now gone. I need to tune a bit the timeouts as after the OV490 boot the OV10640 requires some time before being accessible. Once I nail down the right timeouts I'll send v3. So far I got 0 errors on 50 boot attempts, finally \o/ --- The rest of the series is almost identical a few grammar fixes apart. Thanks j Jacopo Mondi (19): media: i2c: max9286: Adjust parameters indent media: i2c: max9286: Rename reverse_channel_mv media: i2c: max9286: Cache channel amplitude media: i2c: max9286: Define high channel amplitude media: v4l2-subdev: De-deprecate init() subdev op media: gmsl: Reimplement initialization sequence media: i2c: max9286: Rework comments in .bound() media: i2c: max9271: Check max9271_write() return media: i2c: max9271: Introduce wake_up() function media: i2c: rdamc21: Fix warning on u8 cast media: i2c: rdacm21: Add dealy after OV490 reset media: i2c: rdacm21: Fix OV10640 powerup media: i2c: rdacm21: Power up OV10640 before OV490 media: i2c: rdacm20: Enable noise immunity media: i2c: rdacm20: Embed 'serializer' field media: i2c: rdacm20: Replace goto with a loop media: i2c: rdacm20: Report camera module name media: i2c: rdacm20: Check return values media: i2c: rdacm20: Re-work ov10635 reset drivers/media/i2c/max9271.c | 37 ++-- drivers/media/i2c/max9271.h | 9 ++ drivers/media/i2c/max9286.c | 61 - drivers/media/i2c/rdacm20.c | 174 +--- drivers/media/i2c/rdacm21.c | 117 +++- include/media/v4l2-subdev.h | 15 +++- 6 files changed, 268 insertions(+), 145 deletions(-) -- 2.30.0
Re: [PATCH v2 12/18] media: i2c: rdacm21: Give more time to OV490 to boot
Hi Laurent,
On Fri, Mar 19, 2021 at 02:29:30AM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>
> On Wed, Mar 17, 2021 at 11:04:45AM +0100, Jacopo Mondi wrote:
> > On Mon, Mar 15, 2021 at 05:22:37PM +, Kieran Bingham wrote:
> > > On 15/03/2021 13:15, Jacopo Mondi wrote:
> > > > It has been observed through repeated testing (250 boots) that in the
> > > > 10% of the cases the RDACM21 initialization sequence errors out due a
> > > > timeout waiting for the OV490 firmware to complete its boot phase.
> > > >
> > > > Albeit being the current timeout relatively large (300-600
> > > > milliseconds),
> > > > doubling it reduces the sporadic error rate down to 1 over an 80 boot
> > > > sequences test run.
> > > >
> > > > The firmware boot delay is unfortunately not characterized in the camera
> > > > module manual.
> > >
> > > I wonder if we could characterize this alone by pulling this down until
> > > we see failures increase, with all the other fixes in place...
> > >
> > > I don't think that's required, but it might be something to check later
> > > if we don't get rid of that 1/80 failure.
> >
> > This is actually driving me crazy :/
> >
> > I had another test run with a surprising 10% failures.
> > All the failures were due to the ov490 firmware boot I'm trying to
> > mitigate here.
> >
> > I went up to give it -6 seconds- and I still get failures in the same
> > percentage. Another run of 20 boots gave 30% failures with the delay I
> > have here in this patch. Just to make sure I was not going crazy I
> > reduced the delay to 1msec and I get an 80% failure rate.
> >
> > Still, I've seen the 1 on 80 failures (I swear! I have logs! :)
> >
> > I've checked what the BSP does, and if after some 300 attempts the
> > ov490 doesn't boot, they simply go an reset it.
> > https://github.com/renesas-rcar/linux-bsp/commit/0cf6e36f5bf49e1c2aab87139ec5b588623c56f8#diff-d770cad7d6f04923d9e89dfe7da369bb3006776d6e4fb8ef79353d5fab3cd25aR827
> > (sorry, I don't seem to be able to point you to the ov490.c#827 with
> > an URL)
>
> It resets both the sensor and the OV490. It could be interested to try
> the latter selectively to see what happens.
>
They do not make any difference :)
But..
> I also suspect that the OV490 has debugging features (possibly including
> a RAM log buffer that we could read over I2C), but we're probably
> getting out of scope here.
>
> > I assume we don't want anything like this in an upstream driver, but
> > I'm really running out of any plausible explanation :(
>
> As discussed, let's try the reset workaround, to see if it helps.
>
> I wonder if opening the camera and probing signals would be a useful
> option :-)
... I really think I've got something working (for real this time :)
Basically, as patch "media: i2c: rdacm21: Fix OV10640 powerdown" of
this series describes, the OV10640 power-up was broken before you
spotted the usage of the wrong gpio pad and it was working because of
an internal pull-up on the SPWDN line, which was erroneously left
floating. Once that was fixed, the OV10640 was always identified
correctly, leaving us with this puzzling "ov490 boot timeout error"
that manifested with more or less the same frequency of the ov10640
identification issue.
In the current implementation we power up the OV490 and wait for its
firmware to boot -before- powering up the ov10640 sensor. Most
probably (or looking at the results I get noaw, most certainly) the
OV490 firmware checks for the sensor to be available and probably
tries to program it. So we're back to the issue we originally had when
the sensor was powered because of the pull up resistor, failing to
boot in case the sensor didn't startup correctly which happened in the
20% of the cases.
If I do power up the OV10640 -before- the OV490 all the firmware boot
errors are now gone. I need to tune a bit the timeouts as after the
OV490 boot the OV10640 requires some time before being accessible.
Once I nail down the right timeouts I'll send v3. So far I got 0
errors on 50 boot attempts, finally \o/
Thanks for keep pushing, I would have swear this was an issue with the
HW design and was very close to give up like a month ago!
V3 out soon!
Thanks
j
>
> > > > Fixes: a59f853b3b4b ("media: i2c: Add driver for RDACM21 camera module")
> > > > Signed-off-by: Jacopo Mondi
> > >
> > > Reviewed-by: Kieran Bingham
> > >
> > > > ---
> > > > drivers/media/i2c/rdacm21.c | 2 +-
> > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
Re: [PATCH v2 1/4] dt-bindings: media: max9286: Describe gpio-hog
Hi Laurent, On Tue, Mar 16, 2021 at 12:15:16AM +0200, Laurent Pinchart wrote: > Hi Jacopo, > > Thank you for the patch. > > On Mon, Mar 15, 2021 at 05:30:25PM +0100, Jacopo Mondi wrote: > > The MAX9286 GMSL deserializer features gpio controller capabilities, > > as it provides 2 GPIO lines. > > > > As establishing a regulator that uses one of the GPIO lines and > > enabling/disabling it at run-time in the max9286 won't work due to > > a circular dependency on the gpio-controller/regulator creation, allow > > the usage of a gpio-hog for that purpose. > > > > The usage of the gpio-hog is required in designs where the MAX9286 > > GPIO lines control the remote cameras power. > > > > Signed-off-by: Jacopo Mondi > > That's really a workaround until we can find a good solution, do we have > to officially support it in the DT bindings ? > That's an interesting question. The 'good' solution implies resolving the circular dependency on the regulator/gpio-controller creation and I feel like it might take a while to find a proper solution. In the meantime, all designs like Eagle that control the camera power through a MAX9286 gpio have to rely on this. I'll go with the majority here: either we add this and upstream the gmsl .dtsi for eagle, or we keep out-of-tree patches :/ > > --- > > .../bindings/media/i2c/maxim,max9286.yaml| 16 > > 1 file changed, 16 insertions(+) > > > > diff --git a/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml > > b/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml > > index ee16102fdfe7..9038300e373c 100644 > > --- a/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml > > +++ b/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml > > @@ -177,6 +177,22 @@ properties: > > > > additionalProperties: false > > > > +patternProperties: > > + "^(hog-[0-9]+|.+-hog(-[0-9]+)?)$": > > +type: object > > +properties: > > + gpio-hog: true > > + gpios: true > > + output-low: true > > + line-name: true > > + > > +required: > > + - gpio-hog > > + - gpios > > + - output-low > > + > > +additionalProperties: false > > + > > required: > >- compatible > >- reg > > -- > Regards, > > Laurent Pinchart
Re: [PATCH v2 12/18] media: i2c: rdacm21: Give more time to OV490 to boot
Hi Kieran, Laurent,
On Mon, Mar 15, 2021 at 05:22:37PM +, Kieran Bingham wrote:
> On 15/03/2021 13:15, Jacopo Mondi wrote:
> > It has been observed through repeated testing (250 boots) that in the
> > 10% of the cases the RDACM21 initialization sequence errors out due a
> > timeout waiting for the OV490 firmware to complete its boot phase.
> >
> > Albeit being the current timeout relatively large (300-600 milliseconds),
> > doubling it reduces the sporadic error rate down to 1 over an 80 boot
> > sequences test run.
> >
> > The firmware boot delay is unfortunately not characterized in the camera
> > module manual.
> >
>
> I wonder if we could characterize this alone by pulling this down until
> we see failures increase, with all the other fixes in place...
>
> I don't think that's required, but it might be something to check later
> if we don't get rid of that 1/80 failure.
This is actually driving me crazy :/
I had another test run with a surprising 10% failures.
All the failures were due to the ov490 firmware boot I'm trying to
mitigate here.
I went up to give it -6 seconds- and I still get failures in the same
percentage. Another run of 20 boots gave 30% failures with the delay I
have here in this patch. Just to make sure I was not going crazy I
reduced the delay to 1msec and I get an 80% failure rate.
Still, I've seen the 1 on 80 failures (I swear! I have logs! :)
I've checked what the BSP does, and if after some 300 attempts the
ov490 doesn't boot, they simply go an reset it.
https://github.com/renesas-rcar/linux-bsp/commit/0cf6e36f5bf49e1c2aab87139ec5b588623c56f8#diff-d770cad7d6f04923d9e89dfe7da369bb3006776d6e4fb8ef79353d5fab3cd25aR827
(sorry, I don't seem to be able to point you to the ov490.c#827 with
an URL)
I assume we don't want anything like this in an upstream driver, but
I'm really running out of any plausible explanation :(
>
>
>
> > Fixes: a59f853b3b4b ("media: i2c: Add driver for RDACM21 camera module")
> > Signed-off-by: Jacopo Mondi
>
> Reviewed-by: Kieran Bingham
>
> > ---
> > drivers/media/i2c/rdacm21.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/media/i2c/rdacm21.c b/drivers/media/i2c/rdacm21.c
> > index 50a9b0d8255d..07cf077d8efd 100644
> > --- a/drivers/media/i2c/rdacm21.c
> > +++ b/drivers/media/i2c/rdacm21.c
> > @@ -53,7 +53,7 @@
> > #define OV490_PID 0x8080300a
> > #define OV490_VER 0x8080300b
> > #define OV490_PID_TIMEOUT 20
> > -#define OV490_OUTPUT_EN_TIMEOUT300
> > +#define OV490_OUTPUT_EN_TIMEOUT600
> >
> > #define OV490_GPIO0BIT(0)
> > #define OV490_SPWDN0 BIT(0)
> >
>
Re: [PATCH v2 02/18] media: i2c: rdacm20: Enable noise immunity
Hi Laurent, On Mon, Mar 15, 2021 at 11:37:26PM +0200, Laurent Pinchart wrote: > Hi Jacopo, > > Thank you for the patch. > > On Mon, Mar 15, 2021 at 02:14:56PM +0100, Jacopo Mondi wrote: > > Enable the noise immunity threshold at the end of the rdacm20 > > initialization routine. > > > > The rdacm20 camera module has been so far tested with a startup > > delay that allowed the embedded MCU to program the serializer. If > > the initialization routine is run before the MCU programs the > > serializer and the image sensor and their addresses gets changed > > by the rdacm20 driver it is required to manually enable the noise > > immunity threshold to make the communication on the control channel > > more reliable. > > I'm still worried by the race with the MCU. Any update on dumping the > MCU configuration to check what it initializes ? > Not yet, you're right ... I mainly focused on testing with rdacm21, what if I strip the rdacm20 changes out from this series ? I will have to keep the init() operation introduction to maintain compatibility with max9286 changes, and in case of no regressions, we can keep the 8 seconds delay in the .dtsi. However it will break upstream support on Eagle for rdacm20 as we don't have a regulator where to insert the startup delay there, and a downstream patch that waits for 8 seconds in the deserializer driver should be used instead... > > Reviewed-by: Kieran Bingham > > Signed-off-by: Jacopo Mondi > > --- > > drivers/media/i2c/rdacm20.c | 8 +++- > > 1 file changed, 7 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/media/i2c/rdacm20.c b/drivers/media/i2c/rdacm20.c > > index 90eb73f0e6e9..f7fd5ae955d0 100644 > > --- a/drivers/media/i2c/rdacm20.c > > +++ b/drivers/media/i2c/rdacm20.c > > @@ -541,7 +541,13 @@ static int rdacm20_initialize(struct rdacm20_device > > *dev) > > > > dev_info(dev->dev, "Identified MAX9271 + OV10635 device\n"); > > > > - return 0; > > + /* > > +* Set reverse channel high threshold to increase noise immunity. > > +* > > +* This should be compensated by increasing the reverse channel > > +* amplitude on the remote deserializer side. > > +*/ > > + return max9271_set_high_threshold(>serializer, true); > > } > > > > static int rdacm20_probe(struct i2c_client *client) > > -- > Regards, > > Laurent Pinchart
[PATCH v2 4/4] arm64: dts: renesas: eagle: Include eagle-gmsl
From: Kieran Bingham
Include the eagle-gmsl.dtsi to enable GMSL camera support on the
Eagle-V3M platform.
Signed-off-by: Kieran Bingham
Signed-off-by: Jacopo Mondi
---
arch/arm64/boot/dts/renesas/r8a77970-eagle.dts | 3 +++
1 file changed, 3 insertions(+)
diff --git a/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts
b/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts
index d2f63238..555070aae03d 100644
--- a/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts
+++ b/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts
@@ -404,3 +404,6 @@ {
status = "okay";
};
+
+/* FAKRA Overlay */
+#include "eagle-gmsl.dtsi"
--
2.30.0
[PATCH v2 3/4] arm64: dts: renesas: eagle: Add GMSL .dtsi
From: Kieran Bingham
Describe the FAKRA connector available on Eagle board that allows
connecting GMSL camera modules such as IMI RDACM20 and RDACM21.
Signed-off-by: Kieran Bingham
Signed-off-by: Jacopo Mondi
---
arch/arm64/boot/dts/renesas/eagle-gmsl.dtsi | 186
1 file changed, 186 insertions(+)
create mode 100644 arch/arm64/boot/dts/renesas/eagle-gmsl.dtsi
diff --git a/arch/arm64/boot/dts/renesas/eagle-gmsl.dtsi
b/arch/arm64/boot/dts/renesas/eagle-gmsl.dtsi
new file mode 100644
index ..ec3e7493aa71
--- /dev/null
+++ b/arch/arm64/boot/dts/renesas/eagle-gmsl.dtsi
@@ -0,0 +1,186 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Device Tree Source (overlay) for the Eagle V3M GMSL connectors
+ *
+ * Copyright (C) 2017 Ideas on Board
+ * Copyright (C) 2021 Jacopo Mondi
+ *
+ * This overlay allows you to define GMSL cameras connected to the FAKRA
+ * connectors on the Eagle-V3M (or compatible) board.
+ *
+ * The following cameras are currently supported:
+ *"imi,rdacm20"
+ *"imi,rdacm21"
+ */
+
+#include
+
+/*
+ * Select which cameras are in use:
+ * #define EAGLE_CAMERA0_RDACM20
+ * #define EAGLE_CAMERA0_RDACM21
+ *
+ * The two camera modules are configured with different image formats
+ * and cannot be mixed.
+ */
+#define EAGLE_CAMERA0_RDACM20
+#define EAGLE_CAMERA1_RDACM20
+#define EAGLE_CAMERA2_RDACM20
+#define EAGLE_CAMERA3_RDACM20
+
+/* Set the compatible string based on the camera model. */
+#if defined(EAGLE_CAMERA0_RDACM21) || defined(EAGLE_CAMERA1_RDACM21) || \
+defined(EAGLE_CAMERA2_RDACM21) || defined(EAGLE_CAMERA3_RDACM21)
+#define EAGLE_CAMERA_MODEL "imi,rdacm21"
+#define EAGLE_USE_RDACM21
+#elif defined(EAGLE_CAMERA0_RDACM20) || defined(EAGLE_CAMERA1_RDACM20) || \
+defined(EAGLE_CAMERA2_RDACM20) || defined(EAGLE_CAMERA3_RDACM20)
+#define EAGLE_CAMERA_MODEL "imi,rdacm20"
+#define EAGLE_USE_RDACM20
+#endif
+
+/* Define which cameras are available. */
+#if defined(EAGLE_CAMERA0_RDACM21) || defined(EAGLE_CAMERA0_RDACM20)
+#define EAGLE_USE_CAMERA_0
+#endif
+
+#if defined(EAGLE_CAMERA1_RDACM21) || defined(EAGLE_CAMERA1_RDACM20)
+#define EAGLE_USE_CAMERA_1
+#endif
+
+#if defined(EAGLE_CAMERA2_RDACM21) || defined(EAGLE_CAMERA2_RDACM20)
+#define EAGLE_USE_CAMERA_2
+#endif
+
+#if defined(EAGLE_CAMERA3_RDACM21) || defined(EAGLE_CAMERA3_RDACM20)
+#define EAGLE_USE_CAMERA_3
+#endif
+
+/* Define the endpoint links. */
+#ifdef EAGLE_USE_CAMERA_0
+_in0 {
+ remote-endpoint = <_con0>;
+};
+#endif
+
+#ifdef EAGLE_USE_CAMERA_1
+_in1 {
+ remote-endpoint = <_con1>;
+};
+#endif
+
+#ifdef EAGLE_USE_CAMERA_2
+_in2 {
+ remote-endpoint = <_con2>;
+};
+#endif
+
+#ifdef EAGLE_USE_CAMERA_3
+_in3 {
+ remote-endpoint = <_con3>;
+};
+#endif
+
+/* Populate the GMSL i2c-mux bus with camera nodes. */
+#if defined(EAGLE_USE_RDACM21) || defined(EAGLE_USE_RDACM20)
+
+#ifdef EAGLE_USE_CAMERA_0
+ {
+ status = "okay";
+};
+#endif
+
+#ifdef EAGLE_USE_CAMERA_1
+ {
+ status = "okay";
+};
+#endif
+
+#ifdef EAGLE_USE_CAMERA_2
+ {
+ status = "okay";
+};
+#endif
+
+#ifdef EAGLE_USE_CAMERA_3
+ {
+ status = "okay";
+};
+#endif
+
+ {
+
+ status = "okay";
+ maxim,reverse-channel-microvolt = <10>;
+
+ i2c-mux {
+#ifdef EAGLE_USE_CAMERA_0
+ i2c@0 {
+ status = "okay";
+
+ camera@51 {
+ compatible = EAGLE_CAMERA_MODEL;
+ reg = <0x51>, <0x61>;
+
+ port {
+ fakra_con0: endpoint {
+ remote-endpoint =
<_in0>;
+ };
+ };
+ };
+ };
+#endif
+
+#ifdef EAGLE_USE_CAMERA_1
+ i2c@1 {
+ status = "okay";
+
+ camera@52 {
+ compatible = EAGLE_CAMERA_MODEL;
+ reg = <0x52>, <0x62>;
+
+ port {
+ fakra_con1: endpoint {
+ remote-endpoint =
<_in1>;
+ };
+ };
+ };
+ };
+#endif
+
+#ifdef EAGLE_USE_CAMERA_2
+ i2c@2 {
+ status = "okay";
+
+ camera@53 {
+ compatible = EAGLE_CAMERA_MODEL;
+ reg = <0x53>, <0x63>;
+
+ port {
+ fakra_con2: endpoint {
+ remote-en
[PATCH v2 2/4] arm64: dts: renesas: eagle: Enable MAX9286
From: Kieran Bingham
Enable the MAX9286 GMSL deserializer on the Eagle-V3M board.
Connected cameras should be defined in a device-tree overlay or included
after these definitions.
Signed-off-by: Kieran Bingham
Signed-off-by: Jacopo Mondi
---
.../arm64/boot/dts/renesas/r8a77970-eagle.dts | 132 ++
1 file changed, 132 insertions(+)
diff --git a/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts
b/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts
index 874a7fc2730b..d2f63238 100644
--- a/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts
+++ b/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts
@@ -6,6 +6,8 @@
* Copyright (C) 2017 Cogent Embedded, Inc.
*/
+#include
+
/dts-v1/;
#include "r8a77970.dtsi"
@@ -188,6 +190,11 @@ i2c0_pins: i2c0 {
function = "i2c0";
};
+ i2c3_pins: i2c3 {
+ groups = "i2c3_a";
+ function = "i2c3";
+ };
+
qspi0_pins: qspi0 {
groups = "qspi0_ctrl", "qspi0_data4";
function = "qspi0";
@@ -266,6 +273,131 @@ {
status = "okay";
};
+ {
+ status = "okay";
+
+ ports {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ port@0 {
+ reg = <0>;
+
+ csi40_in: endpoint {
+ clock-lanes = <0>;
+ data-lanes = <1 2 3 4>;
+ remote-endpoint = <_out0>;
+ };
+ };
+ };
+};
+
+ {
+ pinctrl-0 = <_pins>;
+ pinctrl-names = "default";
+
+ status = "okay";
+ clock-frequency = <40>;
+
+ gmsl: gmsl-deserializer@48 {
+ gpio-controller;
+ #gpio-cells = <2>;
+
+ compatible = "maxim,max9286";
+ reg = <0x48>;
+
+ /* eagle-pca9654-max9286-pwdn */
+ enable-gpios = <_expander 0 GPIO_ACTIVE_HIGH>;
+
+ /*
+* Workaround: Hog the CAMVDD line high as we can't establish a
+* regulator-fixed on the gpio_chip exposed by due to
+* circular-dependency issues.
+*/
+ camvdd-en-hog {
+ gpio-hog;
+ gpios = <0 0>;
+ output-low;
+ line-name = "CAMVDD_EN";
+ };
+
+ ports {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ port@0 {
+ reg = <0>;
+ max9286_in0: endpoint {
+ };
+ };
+
+ port@1 {
+ reg = <1>;
+ max9286_in1: endpoint {
+ };
+ };
+
+ port@2 {
+ reg = <2>;
+ max9286_in2: endpoint {
+ };
+ };
+
+ port@3 {
+ reg = <3>;
+ max9286_in3: endpoint {
+ };
+ };
+
+ port@4 {
+ reg = <4>;
+ max9286_out0: endpoint {
+ clock-lanes = <0>;
+ data-lanes = <1 2 3 4>;
+ remote-endpoint = <_in>;
+ };
+ };
+ };
+
+ i2c-mux {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ i2c@0 {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ reg = <0>;
+
+ status = "disabled";
+ };
+
+ i2c@1 {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ reg = <1>;
+
+ status = "disabled";
+ };
+
+ i2c@2 {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ reg = <2>;
+
+ status = "disabled";
+
[PATCH v2 1/4] dt-bindings: media: max9286: Describe gpio-hog
The MAX9286 GMSL deserializer features gpio controller capabilities, as it provides 2 GPIO lines. As establishing a regulator that uses one of the GPIO lines and enabling/disabling it at run-time in the max9286 won't work due to a circular dependency on the gpio-controller/regulator creation, allow the usage of a gpio-hog for that purpose. The usage of the gpio-hog is required in designs where the MAX9286 GPIO lines control the remote cameras power. Signed-off-by: Jacopo Mondi --- .../bindings/media/i2c/maxim,max9286.yaml| 16 1 file changed, 16 insertions(+) diff --git a/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml b/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml index ee16102fdfe7..9038300e373c 100644 --- a/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml +++ b/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml @@ -177,6 +177,22 @@ properties: additionalProperties: false +patternProperties: + "^(hog-[0-9]+|.+-hog(-[0-9]+)?)$": +type: object +properties: + gpio-hog: true + gpios: true + output-low: true + line-name: true + +required: + - gpio-hog + - gpios + - output-low + +additionalProperties: false + required: - compatible - reg -- 2.30.0
[PATCH v2 0/4] arm64: dts: renesas: Enable GMSL on R8A77970 V3M Eagle
Hello, this series adds a .dtsi fragment that allow to describe and enable GMSL cameras on the V3M Eagle board. The .dtsi supports connecting the RDACM20 and RDACM21 cameras to the FAKRA connectors installed on the board. Tested on V3M Eagle with RDACM20 and RDACM21 v1->v2: - Use a pattern property to describe the gpio-hog Thanks j Jacopo Mondi (1): dt-bindings: media: max9286: Describe gpio-hog Kieran Bingham (3): arm64: dts: renesas: eagle: Enable MAX9286 arm64: dts: renesas: eagle: Add GMSL .dtsi arm64: dts: renesas: eagle: Include eagle-gmsl .../bindings/media/i2c/maxim,max9286.yaml | 16 ++ arch/arm64/boot/dts/renesas/eagle-gmsl.dtsi | 186 ++ .../arm64/boot/dts/renesas/r8a77970-eagle.dts | 135 + 3 files changed, 337 insertions(+) create mode 100644 arch/arm64/boot/dts/renesas/eagle-gmsl.dtsi -- 2.30.0
[PATCH v2 16/18] media: v4l2-subdev: De-deprecate init() subdev op
The init() subdev core operation is deemed to be deprecated for new
subdevice drivers. However it could prove useful for complex
architectures to defer operation that require access to the
communication bus if said bus is not available (or fully configured)
at the time when the subdevice probe() function is run.
As an example, the GMSL architecture requires the GMSL configuration
link to be configured on the host side after the remote subdevice
has completed its probe function. After the configuration on the host
side has been performed, the subdevice registers can be accessed through
the communication bus.
In particular:
HOSTREMOTE
probe()
|
-> |
probe() {
bus config()
}
|<|
v4l2 async bound {
bus config()
call subdev init()
|>|
init() {
access register on the bus()
}
|<---
}
In the GMSL use case the bus configuration requires the enablement of the
noise immunity threshold on the remote side which ensures reliability
of communications in electrically noisy environments. After the subdevice
has enabled the threshold at the end of its probe() sequence the host
side shall compensate it with an higher signal amplitude. Once this
sequence has completed the bus can be accessed with noise protection
enabled and all the operations that require a considerable number of
transactions on the bus (such as the image sensor configuration
sequence) are run in the subdevice init() operation implementation.
Signed-off-by: Jacopo Mondi
---
include/media/v4l2-subdev.h | 15 ---
1 file changed, 12 insertions(+), 3 deletions(-)
diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
index d0e9a5bdb08b..3068d9940669 100644
--- a/include/media/v4l2-subdev.h
+++ b/include/media/v4l2-subdev.h
@@ -148,9 +148,18 @@ struct v4l2_subdev_io_pin_config {
* each pin being configured. This function could be called at times
* other than just subdevice initialization.
*
- * @init: initialize the sensor registers to some sort of reasonable default
- * values. Do not use for new drivers and should be removed in existing
- * drivers.
+ * @init: initialize the subdevice registers to some sort of reasonable default
+ * values. Do not use for new drivers (and should be removed in existing
+ * ones) for regular architectures where the image sensor is connected to
+ * the host receiver. For more complex architectures where the subdevice
+ * initialization should be deferred to the completion of the probe
+ * sequence of some intermediate component, or the communication bus
+ * requires configurations on the host side that depend on the completion
+ * of the probe sequence of the remote subdevices, the usage of this
+ * operation could be considered to allow the devices along the pipeline to
+ * probe and register in the media graph and to defer any operation that
+ * require actual access to the communication bus to their init() function
+ * implementation.
*
* @load_fw: load firmware.
*
--
2.30.0
[PATCH v2 17/18] media: gmsl: Reimplement initialization sequence
The current probe() procedure of the RDACM20 and RDACM20 GMSL cameras is
performed with the embedded MAX9271 serializer's noise immunity
threshold disabled. Once the camera has been initialized by probing the
embedded chips, the threshold is enabled and then compensated on the
deserializer's side by increasing the reverse channel signal amplitude
once all cameras have bound.
The probe routine is thus run without noise immunity activated which
in noisy environment conditions makes the probe sequence less reliable as
the chips configuration requires a relatively high amount of i2c
transactions.
Break chip initialization in two:
- At probe time only configure the serializer's reverse channel with
noise immunity activated, to reduce the number of transactions
performed without noise immunity protection enabled
- Move the chips initialization to the .init() core subdev operation
called by the deserializer after all camera have probed and
have increased their noise immunity threshold
The initialization routine looks like the following:
MAX9286 RDACM20/21
probe()
|
-> |
probe() {
enable_threshold()
}
|<|
v4l2 async bound {
compensate_amplitude()
call subdev init()
|>|
init() {
access camera registers()
}
|<---
}
Reviewed-by: Laurent Pinchart
Reviewed-by: Kieran Bingham
Signed-off-by: Jacopo Mondi
---
drivers/media/i2c/max9286.c | 19 ---
drivers/media/i2c/rdacm20.c | 63 ++---
drivers/media/i2c/rdacm21.c | 63 ++---
3 files changed, 91 insertions(+), 54 deletions(-)
diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
index 9124d5fa6ea3..b6347639901e 100644
--- a/drivers/media/i2c/max9286.c
+++ b/drivers/media/i2c/max9286.c
@@ -563,17 +563,28 @@ static int max9286_notify_bound(struct
v4l2_async_notifier *notifier,
if (priv->bound_sources != priv->source_mask)
return 0;
+ /*
+* Initialize all the remote camera. Increase the channel amplitude
+* to compensate for the remote noise immunity threshold.
+*/
+ max9286_reverse_channel_setup(priv, MAX9286_REV_AMP_HIGH);
+ for_each_source(priv, source) {
+ ret = v4l2_subdev_call(source->sd, core, init, 0);
+ if (ret) {
+ dev_err(>client->dev,
+ "Failed to initialize camera device %u\n",
+ index);
+ return ret;
+ }
+ }
+
/*
* All enabled sources have probed and enabled their reverse control
* channels:
-*
-* - Increase the reverse channel amplitude to compensate for the
-* remote ends high threshold
* - Verify all configuration links are properly detected
* - Disable auto-ack as communication on the control channel are now
* stable.
*/
- max9286_reverse_channel_setup(priv, MAX9286_REV_AMP_HIGH);
max9286_check_config_link(priv, priv->source_mask);
/*
diff --git a/drivers/media/i2c/rdacm20.c b/drivers/media/i2c/rdacm20.c
index 2265ef7c65d4..57bf4d362cad 100644
--- a/drivers/media/i2c/rdacm20.c
+++ b/drivers/media/i2c/rdacm20.c
@@ -437,35 +437,12 @@ static int rdacm20_get_fmt(struct v4l2_subdev *sd,
return 0;
}
-static const struct v4l2_subdev_video_ops rdacm20_video_ops = {
- .s_stream = rdacm20_s_stream,
-};
-
-static const struct v4l2_subdev_pad_ops rdacm20_subdev_pad_ops = {
- .enum_mbus_code = rdacm20_enum_mbus_code,
- .get_fmt= rdacm20_get_fmt,
- .set_fmt= rdacm20_get_fmt,
-};
-
-static const struct v4l2_subdev_ops rdacm20_subdev_ops = {
- .video = _video_ops,
- .pad= _subdev_pad_ops,
-};
-
-static int rdacm20_initialize(struct rdacm20_device *dev)
+static int rdacm20_init(struct v4l2_subdev *sd, unsigned int val)
{
+ struct rdacm20_device *dev = sd_to_rdacm20(sd);
unsigned int i;
int ret;
- /* Verify communication with the MAX9271: ping to wakeup. */
- dev->serializer.client->addr = MAX9271_DEFAULT_ADDR;
- max9271_wake_up(>serializer);
-
- /* Serial link disabled during config as it needs a valid pixel clock.
*/
- ret = max9271_set_serial_link(>serializer, false);
- if (ret)
- return ret;
-
/*
* Ensure that we have a good link con
[PATCH v2 18/18] media: i2c: max9286: Rework comments in .bound()
Re-phrase a comment in .bound() callback to make it clear we register a subdev notifier and remove a redundant comment about disabling i2c auto-ack. No functional changes intended. Signed-off-by: Jacopo Mondi --- drivers/media/i2c/max9286.c | 16 ++-- 1 file changed, 6 insertions(+), 10 deletions(-) diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c index b6347639901e..16b2cb9b44a2 100644 --- a/drivers/media/i2c/max9286.c +++ b/drivers/media/i2c/max9286.c @@ -556,9 +556,9 @@ static int max9286_notify_bound(struct v4l2_async_notifier *notifier, subdev->name, src_pad, index); /* -* We can only register v4l2_async_notifiers, which do not provide a -* means to register a complete callback. bound_sources allows us to -* identify when all remote serializers have completed their probe. +* As we register a subdev notifiers we won't get a .complete() callback +* here, so we have to use bound_sources to identify when all remote +* serializers have probed. */ if (priv->bound_sources != priv->source_mask) return 0; @@ -581,16 +581,12 @@ static int max9286_notify_bound(struct v4l2_async_notifier *notifier, /* * All enabled sources have probed and enabled their reverse control * channels: +* - The reverse channel amplitude stays high * - Verify all configuration links are properly detected -* - Disable auto-ack as communication on the control channel are now -* stable. +* - Disable auto-ack as communications on the control channel are now +* stable */ max9286_check_config_link(priv, priv->source_mask); - - /* -* Re-configure I2C with local acknowledge disabled after cameras have -* probed. -*/ max9286_configure_i2c(priv, false); return max9286_set_pixelrate(priv); -- 2.30.0
[PATCH v2 12/18] media: i2c: rdacm21: Give more time to OV490 to boot
It has been observed through repeated testing (250 boots) that in the
10% of the cases the RDACM21 initialization sequence errors out due a
timeout waiting for the OV490 firmware to complete its boot phase.
Albeit being the current timeout relatively large (300-600 milliseconds),
doubling it reduces the sporadic error rate down to 1 over an 80 boot
sequences test run.
The firmware boot delay is unfortunately not characterized in the camera
module manual.
Fixes: a59f853b3b4b ("media: i2c: Add driver for RDACM21 camera module")
Signed-off-by: Jacopo Mondi
---
drivers/media/i2c/rdacm21.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/media/i2c/rdacm21.c b/drivers/media/i2c/rdacm21.c
index 50a9b0d8255d..07cf077d8efd 100644
--- a/drivers/media/i2c/rdacm21.c
+++ b/drivers/media/i2c/rdacm21.c
@@ -53,7 +53,7 @@
#define OV490_PID 0x8080300a
#define OV490_VER 0x8080300b
#define OV490_PID_TIMEOUT 20
-#define OV490_OUTPUT_EN_TIMEOUT300
+#define OV490_OUTPUT_EN_TIMEOUT600
#define OV490_GPIO0BIT(0)
#define OV490_SPWDN0 BIT(0)
--
2.30.0
[PATCH v2 14/18] media: i2c: max9286: Cache channel amplitude
Cache the current channel amplitude in a driver variable
to skip updating it if the newly requested value is the same
as the currently configured one.
Reviewed-by: Kieran Bingham
Reviewed-by: Laurent Pinchart
Signed-off-by: Jacopo Mondi
---
drivers/media/i2c/max9286.c | 13 ++---
1 file changed, 10 insertions(+), 3 deletions(-)
diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
index 82ec05e96cb7..ed1cdefe7c30 100644
--- a/drivers/media/i2c/max9286.c
+++ b/drivers/media/i2c/max9286.c
@@ -165,6 +165,7 @@ struct max9286_priv {
/* The initial reverse control channel amplitude. */
u32 init_rev_chan_mv;
+ u32 rev_chan_mv;
struct v4l2_ctrl_handler ctrls;
struct v4l2_ctrl *pixelrate;
@@ -341,8 +342,15 @@ static void max9286_configure_i2c(struct max9286_priv
*priv, bool localack)
static void max9286_reverse_channel_setup(struct max9286_priv *priv,
unsigned int chan_amplitude)
{
+ u8 chan_config;
+
+ if (priv->rev_chan_mv == chan_amplitude)
+ return;
+
+ priv->rev_chan_mv = chan_amplitude;
+
/* Reverse channel transmission time: default to 1. */
- u8 chan_config = MAX9286_REV_TRF(1);
+ chan_config = MAX9286_REV_TRF(1);
/*
* Reverse channel setup.
@@ -564,8 +572,7 @@ static int max9286_notify_bound(struct v4l2_async_notifier
*notifier,
* - Disable auto-ack as communication on the control channel are now
* stable.
*/
- if (priv->init_rev_chan_mv < 170)
- max9286_reverse_channel_setup(priv, 170);
+ max9286_reverse_channel_setup(priv, 170);
max9286_check_config_link(priv, priv->source_mask);
/*
--
2.30.0
[PATCH v2 15/18] media: i2c: max9286: Define high channel amplitude
Provide a macro to define the reverse channel amplitude to be used to compensate the remote serializer noise immunity. While at it, update a comment. Reviewed-by: Kieran Bingham Reviewed-by: Laurent Pinchart Signed-off-by: Jacopo Mondi --- drivers/media/i2c/max9286.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c index ed1cdefe7c30..9124d5fa6ea3 100644 --- a/drivers/media/i2c/max9286.c +++ b/drivers/media/i2c/max9286.c @@ -113,6 +113,7 @@ #define MAX9286_REV_TRF(n) ((n) << 4) #define MAX9286_REV_AMP(n) n) - 30) / 10) << 1) /* in mV */ #define MAX9286_REV_AMP_X BIT(0) +#define MAX9286_REV_AMP_HIGH 170 /* Register 0x3f */ #define MAX9286_EN_REV_CFG BIT(6) #define MAX9286_REV_FLEN(n)((n) - 20) @@ -567,12 +568,12 @@ static int max9286_notify_bound(struct v4l2_async_notifier *notifier, * channels: * * - Increase the reverse channel amplitude to compensate for the -* remote ends high threshold, if not done already +* remote ends high threshold * - Verify all configuration links are properly detected * - Disable auto-ack as communication on the control channel are now * stable. */ - max9286_reverse_channel_setup(priv, 170); + max9286_reverse_channel_setup(priv, MAX9286_REV_AMP_HIGH); max9286_check_config_link(priv, priv->source_mask); /* -- 2.30.0
[PATCH v2 13/18] media: i2c: max9286: Rename reverse_channel_mv
Rename the reverse_channel_mv variable to init_rev_chan_mv as
the next patch will cache the reverse channel amplitude in
a new driver variable.
Reviewed-by: Kieran Bingham
Reviewed-by: Laurent Pinchart
Signed-off-by: Jacopo Mondi
---
drivers/media/i2c/max9286.c | 11 ++-
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
index 1d9951215868..82ec05e96cb7 100644
--- a/drivers/media/i2c/max9286.c
+++ b/drivers/media/i2c/max9286.c
@@ -163,7 +163,8 @@ struct max9286_priv {
unsigned int mux_channel;
bool mux_open;
- u32 reverse_channel_mv;
+ /* The initial reverse control channel amplitude. */
+ u32 init_rev_chan_mv;
struct v4l2_ctrl_handler ctrls;
struct v4l2_ctrl *pixelrate;
@@ -563,7 +564,7 @@ static int max9286_notify_bound(struct v4l2_async_notifier
*notifier,
* - Disable auto-ack as communication on the control channel are now
* stable.
*/
- if (priv->reverse_channel_mv < 170)
+ if (priv->init_rev_chan_mv < 170)
max9286_reverse_channel_setup(priv, 170);
max9286_check_config_link(priv, priv->source_mask);
@@ -971,7 +972,7 @@ static int max9286_setup(struct max9286_priv *priv)
* only. This should be disabled after the mux is initialised.
*/
max9286_configure_i2c(priv, true);
- max9286_reverse_channel_setup(priv, priv->reverse_channel_mv);
+ max9286_reverse_channel_setup(priv, priv->init_rev_chan_mv);
/*
* Enable GMSL links, mask unused ones and autodetect link
@@ -1236,9 +1237,9 @@ static int max9286_parse_dt(struct max9286_priv *priv)
if (of_property_read_u32(dev->of_node,
"maxim,reverse-channel-microvolt",
_channel_microvolt))
- priv->reverse_channel_mv = 170;
+ priv->init_rev_chan_mv = 170;
else
- priv->reverse_channel_mv = reverse_channel_microvolt / 1000U;
+ priv->init_rev_chan_mv = reverse_channel_microvolt / 1000U;
priv->route_mask = priv->source_mask;
--
2.30.0
[PATCH v2 10/18] media: i2c: max9286: Adjust parameters indent
The parameters to max9286_i2c_mux_configure() fits on the previous line. Adjust it. Cosmetic change only. Reviewed-by: Kieran Bingham Reviewed-by: Laurent Pinchart Signed-off-by: Jacopo Mondi --- drivers/media/i2c/max9286.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c index 6fd4d59fcc72..1d9951215868 100644 --- a/drivers/media/i2c/max9286.c +++ b/drivers/media/i2c/max9286.c @@ -287,9 +287,8 @@ static int max9286_i2c_mux_select(struct i2c_mux_core *muxc, u32 chan) priv->mux_channel = chan; - max9286_i2c_mux_configure(priv, - MAX9286_FWDCCEN(chan) | - MAX9286_REVCCEN(chan)); + max9286_i2c_mux_configure(priv, MAX9286_FWDCCEN(chan) | + MAX9286_REVCCEN(chan)); return 0; } -- 2.30.0
[PATCH v2 09/18] media: i2c: max9271: Introduce wake_up() function
The MAX9271 chip manual prescribes a delay of 5 milliseconds
after the chip exists from low power state.
Add a new function to the max9271 library driver that wakes up the chip
with a dummy i2c transaction and implements the correct delay of 5
milliseconds after the chip exits from low power state.
Use the newly introduced function in the rdacm20 and rdacm21 camera
drivers. The former was not respecting the required delay while the
latter was waiting for a too-short timeout.
Do not handle the initial i2c address configuration in the library
driver function as the camera module drivers control address
reprogramming.
Signed-off-by: Jacopo Mondi
---
drivers/media/i2c/max9271.c | 7 +++
drivers/media/i2c/max9271.h | 9 +
drivers/media/i2c/rdacm20.c | 2 +-
drivers/media/i2c/rdacm21.c | 3 +--
4 files changed, 18 insertions(+), 3 deletions(-)
diff --git a/drivers/media/i2c/max9271.c b/drivers/media/i2c/max9271.c
index 2c7dc7fb9846..f7bfe7266763 100644
--- a/drivers/media/i2c/max9271.c
+++ b/drivers/media/i2c/max9271.c
@@ -80,6 +80,13 @@ static int max9271_pclk_detect(struct max9271_device *dev)
return -EIO;
}
+void max9271_wake_up(struct max9271_device *dev)
+{
+ i2c_smbus_read_byte(dev->client);
+ usleep_range(5000, 8000);
+}
+EXPORT_SYMBOL_GPL(max9271_wake_up);
+
int max9271_set_serial_link(struct max9271_device *dev, bool enable)
{
int ret;
diff --git a/drivers/media/i2c/max9271.h b/drivers/media/i2c/max9271.h
index d78fb21441e9..dc5e4e70ba6f 100644
--- a/drivers/media/i2c/max9271.h
+++ b/drivers/media/i2c/max9271.h
@@ -85,6 +85,15 @@ struct max9271_device {
struct i2c_client *client;
};
+/**
+ * max9271_wake_up() - Wake up the serializer by issuing an i2c transaction
+ * @dev: The max9271 device
+ *
+ * This function shall be called before any other interaction with the
+ * serializer.
+ */
+void max9271_wake_up(struct max9271_device *dev);
+
/**
* max9271_set_serial_link() - Enable/disable serial link
* @dev: The max9271 device
diff --git a/drivers/media/i2c/rdacm20.c b/drivers/media/i2c/rdacm20.c
index b9aaa0f7db42..2265ef7c65d4 100644
--- a/drivers/media/i2c/rdacm20.c
+++ b/drivers/media/i2c/rdacm20.c
@@ -459,7 +459,7 @@ static int rdacm20_initialize(struct rdacm20_device *dev)
/* Verify communication with the MAX9271: ping to wakeup. */
dev->serializer.client->addr = MAX9271_DEFAULT_ADDR;
- i2c_smbus_read_byte(dev->serializer.client);
+ max9271_wake_up(>serializer);
/* Serial link disabled during config as it needs a valid pixel clock.
*/
ret = max9271_set_serial_link(>serializer, false);
diff --git a/drivers/media/i2c/rdacm21.c b/drivers/media/i2c/rdacm21.c
index 179d107f494c..7bce55adfd7c 100644
--- a/drivers/media/i2c/rdacm21.c
+++ b/drivers/media/i2c/rdacm21.c
@@ -452,8 +452,7 @@ static int rdacm21_initialize(struct rdacm21_device *dev)
/* Verify communication with the MAX9271: ping to wakeup. */
dev->serializer.client->addr = MAX9271_DEFAULT_ADDR;
- i2c_smbus_read_byte(dev->serializer.client);
- usleep_range(3000, 5000);
+ max9271_wake_up(>serializer);
/* Enable reverse channel and disable the serial link. */
ret = max9271_set_serial_link(>serializer, false);
--
2.30.0
[PATCH v2 08/18] media: i2c: rdacm20: Re-work ov10635 reset
The OV10635 image sensor embedded in the camera module is currently
reset after the MAX9271 initialization with two long delays that were
most probably not correctly characterized.
Re-work the image sensor reset procedure by holding the chip in reset
during the MAX9271 configuration, removing the long sleep delays and
only wait after the chip exits from reset for 350-500 microseconds
interval, which is larger than the minimum (2048 * (1 / XVCLK)) timeout
characterized in the chip manual.
Reviewed-by: Kieran Bingham
Reviewed-by: Laurent Pinchart
Signed-off-by: Jacopo Mondi
---
drivers/media/i2c/rdacm20.c | 29 +
1 file changed, 17 insertions(+), 12 deletions(-)
diff --git a/drivers/media/i2c/rdacm20.c b/drivers/media/i2c/rdacm20.c
index 312450db958d..b9aaa0f7db42 100644
--- a/drivers/media/i2c/rdacm20.c
+++ b/drivers/media/i2c/rdacm20.c
@@ -477,6 +477,19 @@ static int rdacm20_initialize(struct rdacm20_device *dev)
if (ret)
return ret;
+ /*
+* Hold OV10635 in reset during max9271 configuration. The reset signal
+* has to be asserted for at least 200 microseconds.
+*/
+ ret = max9271_enable_gpios(>serializer, MAX9271_GPIO1OUT);
+ if (ret)
+ return ret;
+
+ ret = max9271_clear_gpios(>serializer, MAX9271_GPIO1OUT);
+ if (ret)
+ return ret;
+ usleep_range(200, 500);
+
ret = max9271_configure_gmsl_link(>serializer);
if (ret)
return ret;
@@ -491,22 +504,14 @@ static int rdacm20_initialize(struct rdacm20_device *dev)
dev->serializer.client->addr = dev->addrs[0];
/*
-* Reset the sensor by cycling the OV10635 reset signal connected to the
-* MAX9271 GPIO1 and verify communication with the OV10635.
+* Release ov10635 from reset and initialize it. The image sensor
+* requires at least 2048 XVCLK cycles (85 micro-seconds at 24MHz)
+* before being available. Stay safe and wait up to 500 micro-seconds.
*/
- ret = max9271_enable_gpios(>serializer, MAX9271_GPIO1OUT);
- if (ret)
- return ret;
-
- ret = max9271_clear_gpios(>serializer, MAX9271_GPIO1OUT);
- if (ret)
- return ret;
- usleep_range(1, 15000);
-
ret = max9271_set_gpios(>serializer, MAX9271_GPIO1OUT);
if (ret)
return ret;
- usleep_range(1, 15000);
+ usleep_range(100, 500);
for (i = 0; i < OV10635_PID_TIMEOUT; ++i) {
ret = ov10635_read16(dev, OV10635_PID);
--
2.30.0
[PATCH v2 11/18] media: i2c: rdacm21: Fix OV10640 powerdown
The OV10640 image sensor powerdown signal is controlled by the first
line of the OV490 GPIO pad #1, but the pad #0 identifier
OV490_GPIO_OUTPUT_VALUE0 was erroneously used. As a result the image
sensor powerdown signal was never asserted but was kept high by an
internal pull-up resistor, causing sporadic failures during the
image sensor startup phase.
Fix this by using the correct GPIO pad identifier.
While at it also fix the GPIO signal handling sequence, as the reset
line was released before the powerdown one, and introduce the correct
delays in between the two operations.
Wait the mandatory 1 millisecond delay after the powerup lane is
asserted. The reset delay is not characterized in the chip manual if
not as "255 XVCLK + initialization". Wait for at least 3 milliseconds
to guarantee the SCCB bus is available.
This commit fixes a sporadic start-up error triggered by a failure to
read the OV10640 chip ID:
rdacm21 8-0054: OV10640 ID mismatch: (0x01)
Fixes: a59f853b3b4b ("media: i2c: Add driver for RDACM21 camera module")
Signed-off-by: Jacopo Mondi
---
drivers/media/i2c/rdacm21.c | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/media/i2c/rdacm21.c b/drivers/media/i2c/rdacm21.c
index 7bce55adfd7c..50a9b0d8255d 100644
--- a/drivers/media/i2c/rdacm21.c
+++ b/drivers/media/i2c/rdacm21.c
@@ -333,13 +333,15 @@ static int ov10640_initialize(struct rdacm21_device *dev)
{
u8 val;
- /* Power-up OV10640 by setting RESETB and PWDNB pins high. */
+ /* Power-up OV10640 by setting PWDNB and RESETB pins high. */
ov490_write_reg(dev, OV490_GPIO_SEL0, OV490_GPIO0);
ov490_write_reg(dev, OV490_GPIO_SEL1, OV490_SPWDN0);
ov490_write_reg(dev, OV490_GPIO_DIRECTION0, OV490_GPIO0);
ov490_write_reg(dev, OV490_GPIO_DIRECTION1, OV490_SPWDN0);
+
+ ov490_write_reg(dev, OV490_GPIO_OUTPUT_VALUE1, OV490_SPWDN0);
+ usleep_range(1500, 3000);
ov490_write_reg(dev, OV490_GPIO_OUTPUT_VALUE0, OV490_GPIO0);
- ov490_write_reg(dev, OV490_GPIO_OUTPUT_VALUE0, OV490_SPWDN0);
usleep_range(3000, 5000);
/* Read OV10640 ID to test communications. */
--
2.30.0
