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
From: Kieran Bingham <[email protected]>
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 <[email protected]>
Signed-off-by: Kieran Bingham <[email protected]>
Signed-off-by: Jacopo Mondi <[email protected]>
---
.../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 <dt-bindings/gpio/gpio.h>
+
/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 @@ &rwdt {
status = "okay";
};
+&csi40 {
+ status = "okay";
+
+ ports {
+ port@0 {
+ csi40_in: endpoint {
+ clock-lanes = <0>;
+ data-lanes = <1 2 3 4>;
+ remote-endpoint = <&max9286_out0>;
+ };
+ };
+ };
+};
+
+&i2c3 {
+ pinctrl-0 = <&i2c3_pins>;
+ pinctrl-names = "default";
+
+ status = "okay";
+ clock-frequency = <400000>;
+
+ gmsl: gmsl-deserializer@48 {
+ compatible = "maxim,max9286";
+ reg = <0x48>;
+
+ maxim,gpio-poc = <0 GPIO_ACTIVE_LOW>;
+
+ /* eagle-pca9654-max9286-pwdn */
+ enable-gpios = <&io_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 = <&csi40_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";
+ };
+ };
+ };
+};
+
&scif0 {
pinctrl-0 = <&scif0_pins>;
pinctrl-names = "default";
--
2.31.1
From: Kieran Bingham <[email protected]>
Include the eagle-gmsl.dtsi to enable GMSL camera support on the
Eagle-V3M platform.
Signed-off-by: Kieran Bingham <[email protected]>
Signed-off-by: Jacopo Mondi <[email protected]>
---
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 @@ &scif0 {
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
From: Kieran Bingham <[email protected]>
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 <[email protected]>
Signed-off-by: Jacopo Mondi <[email protected]>
---
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 000000000000..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 <[email protected]>
+ * Copyright (C) 2021 Jacopo Mondi <[email protected]>
+ *
+ * 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 <dt-bindings/gpio/gpio.h>
+
+/* 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
+
+&vin0 {
+ status = "okay";
+};
+
+&vin1 {
+ status = "okay";
+};
+
+&vin2 {
+ status = "okay";
+};
+
+&vin3 {
+ status = "okay";
+};
+
+&gmsl {
+ status = "okay";
+
+#if defined(GMSL_CAMERA_RDACM21)
+ maxim,reverse-channel-microvolt = <100000>;
+#endif
+
+ ports {
+#ifdef GMSL_CAMERA_0
+ port@0 {
+ max9286_in0: endpoint {
+ remote-endpoint = <&fakra_con0>;
+ };
+ };
+#endif
+
+#ifdef GMSL_CAMERA_1
+ port@1 {
+ max9286_in1: endpoint{
+ remote-endpoint = <&fakra_con1>;
+ };
+
+ };
+#endif
+
+#ifdef GMSL_CAMERA_2
+ port@2 {
+ max9286_in2: endpoint {
+ remote-endpoint = <&fakra_con2>;
+ };
+
+ };
+#endif
+
+#ifdef GMSL_CAMERA_3
+ port@3 {
+ max9286_in3: endpoint {
+ remote-endpoint = <&fakra_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 = <&max9286_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 = <&max9286_in1>;
+ };
+ };
+ };
+ };
+#endif
+
+#ifdef GMSL_CAMERA_2
+ i2c@2 {
+ status = "okay";
+
+ camera@53 {
+ compatible = GMSL_CAMERA_MODEL;
+ reg = <0x53>, <0x63>;
+
+ port {
+ fakra_con2: endpoint {
+ remote-endpoint = <&max9286_in2>;
+ };
+ };
+ };
+ };
+#endif
+
+#ifdef GMSL_CAMERA_3
+ i2c@3 {
+ status = "okay";
+
+ camera@54 {
+ compatible = GMSL_CAMERA_MODEL;
+ reg = <0x54>, <0x64>;
+
+ port {
+ fakra_con3: endpoint {
+ remote-endpoint = <&max9286_in3>;
+ };
+ };
+ };
+ };
+#endif
+ };
+};
--
2.31.1
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 <[email protected]>
Reviewed-by: Laurent Pinchart <[email protected]>
Signed-off-by: Jacopo Mondi <[email protected]>
---
.../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 100000 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
+ <include/dt-bindings/gpio/gpio.h>.
+
+ 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 = <&gpio 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
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 <[email protected]>
Acked-by: Rob Herring <[email protected]>
Signed-off-by: Jacopo Mondi <[email protected]>
---
.../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 <dt-bindings/gpio/gpio.h>
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 = <&camera_poc_12v>;
- enable-gpios = <&gpio 13 GPIO_ACTIVE_HIGH>;
+ gmsl-deserializer@2c {
+ compatible = "maxim,max9286";
+ reg = <0x2c>;
+ poc-supply = <&camera_poc_12v>;
+ enable-gpios = <&gpio 13 GPIO_ACTIVE_HIGH>;
- gpio-controller;
- #gpio-cells = <2>;
+ gpio-controller;
+ #gpio-cells = <2>;
- maxim,reverse-channel-microvolt = <170000>;
+ maxim,reverse-channel-microvolt = <170000>;
- 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 = <&rdacm20_out0>;
- };
- };
-
- port@1 {
- reg = <1>;
-
- max9286_in1: endpoint {
- remote-endpoint = <&rdacm20_out1>;
- };
- };
-
- port@2 {
- reg = <2>;
-
- max9286_in2: endpoint {
- remote-endpoint = <&rdacm20_out2>;
- };
- };
+ max9286_in0: endpoint {
+ remote-endpoint = <&rdacm20_out0>;
+ };
+ };
- port@3 {
- reg = <3>;
+ port@1 {
+ reg = <1>;
- max9286_in3: endpoint {
- remote-endpoint = <&rdacm20_out3>;
- };
- };
+ max9286_in1: endpoint {
+ remote-endpoint = <&rdacm20_out1>;
+ };
+ };
- port@4 {
- reg = <4>;
+ port@2 {
+ reg = <2>;
- max9286_out: endpoint {
- data-lanes = <1 2 3 4>;
- remote-endpoint = <&csi40_in>;
- };
- };
- };
+ max9286_in2: endpoint {
+ remote-endpoint = <&rdacm20_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 = <&rdacm20_out3>;
+ };
+ };
- camera@51 {
- compatible = "imi,rdacm20";
- reg = <0x51>, <0x61>;
+ port@4 {
+ reg = <4>;
- port {
- rdacm20_out0: endpoint {
- remote-endpoint = <&max9286_in0>;
+ max9286_out: endpoint {
+ data-lanes = <1 2 3 4>;
+ remote-endpoint = <&csi40_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 = <&max9286_in1>;
- };
- };
- };
- };
+ i2c@0 {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ reg = <0>;
- i2c@2 {
- #address-cells = <1>;
- #size-cells = <0>;
- reg = <2>;
+ camera@51 {
+ compatible = "imi,rdacm20";
+ reg = <0x51>, <0x61>;
- camera@53 {
- compatible = "imi,rdacm20";
- reg = <0x53>, <0x63>;
+ port {
+ rdacm20_out0: endpoint {
+ remote-endpoint = <&max9286_in0>;
+ };
+ };
- port {
- rdacm20_out2: endpoint {
- remote-endpoint = <&max9286_in2>;
+ };
};
- };
- };
- };
- i2c@3 {
- #address-cells = <1>;
- #size-cells = <0>;
- reg = <3>;
+ i2c@1 {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ reg = <1>;
+
+ camera@52 {
+ compatible = "imi,rdacm20";
+ reg = <0x52>, <0x62>;
+
+ port {
+ rdacm20_out1: endpoint {
+ remote-endpoint = <&max9286_in1>;
+ };
+ };
+ };
+ };
- camera@54 {
- compatible = "imi,rdacm20";
- reg = <0x54>, <0x64>;
+ i2c@2 {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ reg = <2>;
+
+ camera@53 {
+ compatible = "imi,rdacm20";
+ reg = <0x53>, <0x63>;
+
+ port {
+ rdacm20_out2: endpoint {
+ remote-endpoint = <&max9286_in2>;
+ };
+ };
+ };
+ };
- port {
- rdacm20_out3: endpoint {
- remote-endpoint = <&max9286_in3>;
+ i2c@3 {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ reg = <3>;
+
+ camera@54 {
+ compatible = "imi,rdacm20";
+ reg = <0x54>, <0x64>;
+
+ port {
+ rdacm20_out3: endpoint {
+ remote-endpoint = <&max9286_in3>;
+ };
+ };
+ };
};
- };
};
- };
};
- };
};
--
2.31.1
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 <[email protected]>
---
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 <linux/fwnode.h>
#include <linux/gpio/consumer.h>
#include <linux/gpio/driver.h>
+#include <linux/gpio/machine.h>
#include <linux/i2c.h>
#include <linux/i2c-mux.h>
#include <linux/module.h>
@@ -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 = &priv->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",
+ 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 (enable)
+ ret = priv->regulator
+ ? regulator_enable(priv->regulator)
+ : max9286_gpio_set(priv, priv->gpio_poc,
+ enable ^ priv->gpio_poc_flags);
+ else
+ ret = priv->regulator
+ ? regulator_disable(priv->regulator)
+ : max9286_gpio_set(priv, priv->gpio_poc,
+ enable ^ priv->gpio_poc_flags);
+
+ if (ret < 0)
+ dev_err(&priv->client->dev, "Unable to turn PoC %s\n",
+ enable ? "on" : "off");
return ret;
}
@@ -1078,17 +1154,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(&client->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 +1171,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 +1187,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 +1359,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(&client->dev, "poc");
- if (IS_ERR(priv->regulator)) {
- if (PTR_ERR(priv->regulator) != -EPROBE_DEFER)
- dev_err(&client->dev,
- "Unable to get PoC regulator (%ld)\n",
- PTR_ERR(priv->regulator));
- ret = PTR_ERR(priv->regulator);
- goto err_powerdown;
- }
-
ret = max9286_parse_dt(priv);
if (ret)
goto err_powerdown;
@@ -1326,7 +1389,7 @@ static int max9286_remove(struct i2c_client *client)
max9286_v4l2_unregister(priv);
- regulator_disable(priv->regulator);
+ max9286_poc_enable(priv, false);
gpiod_set_value_cansleep(priv->gpiod_pwdn, 0);
--
2.31.1
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 <[email protected]>
Signed-off-by: Jacopo Mondi <[email protected]>
---
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
Hello,
gentle ping.
This change is required to move forward with integration of GMSL
on Eagle.
Thanks
j
On Mon, Apr 19, 2021 at 04:23:41PM +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 <[email protected]>
> ---
> 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 <linux/fwnode.h>
> #include <linux/gpio/consumer.h>
> #include <linux/gpio/driver.h>
> +#include <linux/gpio/machine.h>
> #include <linux/i2c.h>
> #include <linux/i2c-mux.h>
> #include <linux/module.h>
> @@ -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 = &priv->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",
> + 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 (enable)
> + ret = priv->regulator
> + ? regulator_enable(priv->regulator)
> + : max9286_gpio_set(priv, priv->gpio_poc,
> + enable ^ priv->gpio_poc_flags);
> + else
> + ret = priv->regulator
> + ? regulator_disable(priv->regulator)
> + : max9286_gpio_set(priv, priv->gpio_poc,
> + enable ^ priv->gpio_poc_flags);
> +
> + if (ret < 0)
> + dev_err(&priv->client->dev, "Unable to turn PoC %s\n",
> + enable ? "on" : "off");
>
> return ret;
> }
> @@ -1078,17 +1154,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(&client->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 +1171,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 +1187,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 +1359,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(&client->dev, "poc");
> - if (IS_ERR(priv->regulator)) {
> - if (PTR_ERR(priv->regulator) != -EPROBE_DEFER)
> - dev_err(&client->dev,
> - "Unable to get PoC regulator (%ld)\n",
> - PTR_ERR(priv->regulator));
> - ret = PTR_ERR(priv->regulator);
> - goto err_powerdown;
> - }
> -
> ret = max9286_parse_dt(priv);
> if (ret)
> goto err_powerdown;
> @@ -1326,7 +1389,7 @@ static int max9286_remove(struct i2c_client *client)
>
> max9286_v4l2_unregister(priv);
>
> - regulator_disable(priv->regulator);
> + max9286_poc_enable(priv, false);
>
> gpiod_set_value_cansleep(priv->gpiod_pwdn, 0);
>
> --
> 2.31.1
>
Hi Jacopo and Kieran,
Thank you for the patch.
On Mon, Apr 19, 2021 at 04:23:44PM +0200, Jacopo Mondi wrote:
> From: Kieran Bingham <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Jacopo Mondi <[email protected]>
> ---
> 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 000000000000..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 <[email protected]>
> + * Copyright (C) 2021 Jacopo Mondi <[email protected]>
> + *
> + * 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 <dt-bindings/gpio/gpio.h>
> +
> +/* 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
This won't scale when we'll support more than two different cameras, but
we'll switch to overlays then :-)
Reviewed-by: Laurent Pinchart <[email protected]>
> +
> +#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
> +
> +&vin0 {
> + status = "okay";
> +};
> +
> +&vin1 {
> + status = "okay";
> +};
> +
> +&vin2 {
> + status = "okay";
> +};
> +
> +&vin3 {
> + status = "okay";
> +};
> +
> +&gmsl {
> + status = "okay";
> +
> +#if defined(GMSL_CAMERA_RDACM21)
> + maxim,reverse-channel-microvolt = <100000>;
> +#endif
> +
> + ports {
> +#ifdef GMSL_CAMERA_0
> + port@0 {
> + max9286_in0: endpoint {
> + remote-endpoint = <&fakra_con0>;
> + };
> + };
> +#endif
> +
> +#ifdef GMSL_CAMERA_1
> + port@1 {
> + max9286_in1: endpoint{
> + remote-endpoint = <&fakra_con1>;
> + };
> +
> + };
> +#endif
> +
> +#ifdef GMSL_CAMERA_2
> + port@2 {
> + max9286_in2: endpoint {
> + remote-endpoint = <&fakra_con2>;
> + };
> +
> + };
> +#endif
> +
> +#ifdef GMSL_CAMERA_3
> + port@3 {
> + max9286_in3: endpoint {
> + remote-endpoint = <&fakra_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 = <&max9286_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 = <&max9286_in1>;
> + };
> + };
> + };
> + };
> +#endif
> +
> +#ifdef GMSL_CAMERA_2
> + i2c@2 {
> + status = "okay";
> +
> + camera@53 {
> + compatible = GMSL_CAMERA_MODEL;
> + reg = <0x53>, <0x63>;
> +
> + port {
> + fakra_con2: endpoint {
> + remote-endpoint = <&max9286_in2>;
> + };
> + };
> + };
> + };
> +#endif
> +
> +#ifdef GMSL_CAMERA_3
> + i2c@3 {
> + status = "okay";
> +
> + camera@54 {
> + compatible = GMSL_CAMERA_MODEL;
> + reg = <0x54>, <0x64>;
> +
> + port {
> + fakra_con3: endpoint {
> + remote-endpoint = <&max9286_in3>;
> + };
> + };
> + };
> + };
> +#endif
> + };
> +};
--
Regards,
Laurent Pinchart
Hi Jacopo,
On 19/04/2021 15:23, 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.
I think I've seen Laurent's despair at the auxillary device bus already,
but I can't help but feel it might be a way to register the gpio and
regulator fully without having to handle any probe deferrals and allow
the GPIO chip to be used as it's own regulator. (I.e. solve the issues I
was facing last time I looked at it)
But that said however, it's only a hypothesis having not yet fully
investigated the option. It seems a shame to have to expose multiple
ways of powering up the cameras, but I guess ultimately it's how the
hardware is connected.
Have we confirmed that the start up delays are no longer needed for the
RDACM20 cameras? (which we've previously exposed as a regulator power up
delay?)
How would this handle those delays if required?
> Signed-off-by: Jacopo Mondi <[email protected]>
> ---
> 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 <linux/fwnode.h>
> #include <linux/gpio/consumer.h>
> #include <linux/gpio/driver.h>
> +#include <linux/gpio/machine.h>
> #include <linux/i2c.h>
> #include <linux/i2c-mux.h>
> #include <linux/module.h>
> @@ -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 = &priv->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",
> + 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 (enable)
> + ret = priv->regulator
> + ? regulator_enable(priv->regulator)
> + : max9286_gpio_set(priv, priv->gpio_poc,
> + enable ^ priv->gpio_poc_flags);
> + else
> + ret = priv->regulator
> + ? regulator_disable(priv->regulator)
> + : max9286_gpio_set(priv, priv->gpio_poc,
> + enable ^ priv->gpio_poc_flags);
> +
> + if (ret < 0)
> + dev_err(&priv->client->dev, "Unable to turn PoC %s\n",
> + enable ? "on" : "off");
>
> return ret;
> }
> @@ -1078,17 +1154,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(&client->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 +1171,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 +1187,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 +1359,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(&client->dev, "poc");
> - if (IS_ERR(priv->regulator)) {
> - if (PTR_ERR(priv->regulator) != -EPROBE_DEFER)
> - dev_err(&client->dev,
> - "Unable to get PoC regulator (%ld)\n",
> - PTR_ERR(priv->regulator));
> - ret = PTR_ERR(priv->regulator);
> - goto err_powerdown;
> - }
> -
> ret = max9286_parse_dt(priv);
> if (ret)
> goto err_powerdown;
> @@ -1326,7 +1389,7 @@ static int max9286_remove(struct i2c_client *client)
>
> max9286_v4l2_unregister(priv);
>
> - regulator_disable(priv->regulator);
> + max9286_poc_enable(priv, false);
>
> gpiod_set_value_cansleep(priv->gpiod_pwdn, 0);
>
>
--
Regards
--
Kieran
On Thu, Jun 17, 2021 at 03:26:13AM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
I forgot to mention, shouldn't this patch and the previous one be sent
to the linux-media mailing list as well ?
> On Mon, Apr 19, 2021 at 04:23:41PM +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 <[email protected]>
>
> Reviewed-by: Laurent Pinchart <[email protected]>
>
> > ---
> > 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 <linux/fwnode.h>
> > #include <linux/gpio/consumer.h>
> > #include <linux/gpio/driver.h>
> > +#include <linux/gpio/machine.h>
> > #include <linux/i2c.h>
> > #include <linux/i2c-mux.h>
> > #include <linux/module.h>
> > @@ -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 = &priv->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",
> > + 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 (enable)
> > + ret = priv->regulator
> > + ? regulator_enable(priv->regulator)
> > + : max9286_gpio_set(priv, priv->gpio_poc,
> > + enable ^ priv->gpio_poc_flags);
> > + else
> > + ret = priv->regulator
> > + ? regulator_disable(priv->regulator)
> > + : max9286_gpio_set(priv, priv->gpio_poc,
> > + enable ^ priv->gpio_poc_flags);
> > +
> > + if (ret < 0)
> > + dev_err(&priv->client->dev, "Unable to turn PoC %s\n",
> > + enable ? "on" : "off");
> >
> > return ret;
> > }
> > @@ -1078,17 +1154,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(&client->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 +1171,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 +1187,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 +1359,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(&client->dev, "poc");
> > - if (IS_ERR(priv->regulator)) {
> > - if (PTR_ERR(priv->regulator) != -EPROBE_DEFER)
> > - dev_err(&client->dev,
> > - "Unable to get PoC regulator (%ld)\n",
> > - PTR_ERR(priv->regulator));
> > - ret = PTR_ERR(priv->regulator);
> > - goto err_powerdown;
> > - }
> > -
> > ret = max9286_parse_dt(priv);
> > if (ret)
> > goto err_powerdown;
> > @@ -1326,7 +1389,7 @@ static int max9286_remove(struct i2c_client *client)
> >
> > max9286_v4l2_unregister(priv);
> >
> > - regulator_disable(priv->regulator);
> > + max9286_poc_enable(priv, false);
> >
> > gpiod_set_value_cansleep(priv->gpiod_pwdn, 0);
> >
--
Regards,
Laurent Pinchart
Hi Jacopo,
Thank you for the patch.
On Mon, Apr 19, 2021 at 04:23:41PM +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 <[email protected]>
Reviewed-by: Laurent Pinchart <[email protected]>
> ---
> 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 <linux/fwnode.h>
> #include <linux/gpio/consumer.h>
> #include <linux/gpio/driver.h>
> +#include <linux/gpio/machine.h>
> #include <linux/i2c.h>
> #include <linux/i2c-mux.h>
> #include <linux/module.h>
> @@ -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 = &priv->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",
> + 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 (enable)
> + ret = priv->regulator
> + ? regulator_enable(priv->regulator)
> + : max9286_gpio_set(priv, priv->gpio_poc,
> + enable ^ priv->gpio_poc_flags);
> + else
> + ret = priv->regulator
> + ? regulator_disable(priv->regulator)
> + : max9286_gpio_set(priv, priv->gpio_poc,
> + enable ^ priv->gpio_poc_flags);
> +
> + if (ret < 0)
> + dev_err(&priv->client->dev, "Unable to turn PoC %s\n",
> + enable ? "on" : "off");
>
> return ret;
> }
> @@ -1078,17 +1154,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(&client->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 +1171,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 +1187,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 +1359,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(&client->dev, "poc");
> - if (IS_ERR(priv->regulator)) {
> - if (PTR_ERR(priv->regulator) != -EPROBE_DEFER)
> - dev_err(&client->dev,
> - "Unable to get PoC regulator (%ld)\n",
> - PTR_ERR(priv->regulator));
> - ret = PTR_ERR(priv->regulator);
> - goto err_powerdown;
> - }
> -
> ret = max9286_parse_dt(priv);
> if (ret)
> goto err_powerdown;
> @@ -1326,7 +1389,7 @@ static int max9286_remove(struct i2c_client *client)
>
> max9286_v4l2_unregister(priv);
>
> - regulator_disable(priv->regulator);
> + max9286_poc_enable(priv, false);
>
> gpiod_set_value_cansleep(priv->gpiod_pwdn, 0);
>
--
Regards,
Laurent Pinchart
Hi Jacopo,
On Thu, Jun 17, 2021 at 2:30 AM Laurent Pinchart
<[email protected]> wrote:
> On Mon, Apr 19, 2021 at 04:23:44PM +0200, Jacopo Mondi wrote:
> > From: Kieran Bingham <[email protected]>
> >
> > 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 <[email protected]>
> > Signed-off-by: Jacopo Mondi <[email protected]>
>
> This won't scale when we'll support more than two different cameras, but
> we'll switch to overlays then :-)
FTR, overlay support has landed upstream.
Still not sure about .dts vs .dtso...
> Reviewed-by: Laurent Pinchart <[email protected]>
The DT parts are still pending acceptance of "maxim,gpio-poc", right?
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Hi Kieran
On Thu, Jun 17, 2021 at 01:02:36AM +0100, Kieran Bingham wrote:
> Hi Jacopo,
>
> On 19/04/2021 15:23, 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.
>
> I think I've seen Laurent's despair at the auxillary device bus already,
> but I can't help but feel it might be a way to register the gpio and
> regulator fully without having to handle any probe deferrals and allow
> the GPIO chip to be used as it's own regulator. (I.e. solve the issues I
> was facing last time I looked at it)
>
> But that said however, it's only a hypothesis having not yet fully
> investigated the option. It seems a shame to have to expose multiple
> ways of powering up the cameras, but I guess ultimately it's how the
> hardware is connected.
I can't really comment as I don't know what auxillary bus is, but I
guess this simple solution is enough to unblock eagle upstreaming and
doesn't tie our hands if we want to switch to something completely
different in future.
>
> Have we confirmed that the start up delays are no longer needed for the
> RDACM20 cameras? (which we've previously exposed as a regulator power up
> delay?)
Yes, not delay needed as long as
https://patchwork.linuxtv.org/project/linux-media/patch/[email protected]/
is applied
>
> How would this handle those delays if required?
>
>
> > Signed-off-by: Jacopo Mondi <[email protected]>
> > ---
> > 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 <linux/fwnode.h>
> > #include <linux/gpio/consumer.h>
> > #include <linux/gpio/driver.h>
> > +#include <linux/gpio/machine.h>
> > #include <linux/i2c.h>
> > #include <linux/i2c-mux.h>
> > #include <linux/module.h>
> > @@ -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 = &priv->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",
> > + 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 (enable)
> > + ret = priv->regulator
> > + ? regulator_enable(priv->regulator)
> > + : max9286_gpio_set(priv, priv->gpio_poc,
> > + enable ^ priv->gpio_poc_flags);
> > + else
> > + ret = priv->regulator
> > + ? regulator_disable(priv->regulator)
> > + : max9286_gpio_set(priv, priv->gpio_poc,
> > + enable ^ priv->gpio_poc_flags);
> > +
> > + if (ret < 0)
> > + dev_err(&priv->client->dev, "Unable to turn PoC %s\n",
> > + enable ? "on" : "off");
> >
> > return ret;
> > }
> > @@ -1078,17 +1154,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(&client->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 +1171,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 +1187,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 +1359,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(&client->dev, "poc");
> > - if (IS_ERR(priv->regulator)) {
> > - if (PTR_ERR(priv->regulator) != -EPROBE_DEFER)
> > - dev_err(&client->dev,
> > - "Unable to get PoC regulator (%ld)\n",
> > - PTR_ERR(priv->regulator));
> > - ret = PTR_ERR(priv->regulator);
> > - goto err_powerdown;
> > - }
> > -
> > ret = max9286_parse_dt(priv);
> > if (ret)
> > goto err_powerdown;
> > @@ -1326,7 +1389,7 @@ static int max9286_remove(struct i2c_client *client)
> >
> > max9286_v4l2_unregister(priv);
> >
> > - regulator_disable(priv->regulator);
> > + max9286_poc_enable(priv, false);
> >
> > gpiod_set_value_cansleep(priv->gpiod_pwdn, 0);
> >
> >
>
> --
> Regards
> --
> Kieran
Hi Kieran,
On Thu, Jun 17, 2021 at 10:19:06AM +0200, Jacopo Mondi wrote:
> Hi Kieran
>
> On Thu, Jun 17, 2021 at 01:02:36AM +0100, Kieran Bingham wrote:
> > Hi Jacopo,
> >
> > On 19/04/2021 15:23, 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.
> >
> > I think I've seen Laurent's despair at the auxillary device bus already,
> > but I can't help but feel it might be a way to register the gpio and
> > regulator fully without having to handle any probe deferrals and allow
> > the GPIO chip to be used as it's own regulator. (I.e. solve the issues I
> > was facing last time I looked at it)
> >
> > But that said however, it's only a hypothesis having not yet fully
> > investigated the option. It seems a shame to have to expose multiple
> > ways of powering up the cameras, but I guess ultimately it's how the
> > hardware is connected.
>
> I can't really comment as I don't know what auxillary bus is, but I
> guess this simple solution is enough to unblock eagle upstreaming and
> doesn't tie our hands if we want to switch to something completely
> different in future.
>
> >
> > Have we confirmed that the start up delays are no longer needed for the
> > RDACM20 cameras? (which we've previously exposed as a regulator power up
> > delay?)
>
> Yes, not delay needed as long as
> https://patchwork.linuxtv.org/project/linux-media/patch/[email protected]/
> is applied
>
Let me (again) correct myself about the RDACM20.
While the above mentioned patch solves all issues at boot time, it still
doesn't guarantee stability of the capture operations.
I blame write collision or rather the MCU changing our programming
after the boot phase. I get an error when accessing the max9271 at
s_stream() time, while during boot I can read its id consistently.
I'm afraid we somehow need that delay to let the MCU boot complete and
overwrite its settings, in this way I get a 100% success of the
capture operations.
> >
> > How would this handle those delays if required?
> >
> >
> > > Signed-off-by: Jacopo Mondi <[email protected]>
> > > ---
> > > 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 <linux/fwnode.h>
> > > #include <linux/gpio/consumer.h>
> > > #include <linux/gpio/driver.h>
> > > +#include <linux/gpio/machine.h>
> > > #include <linux/i2c.h>
> > > #include <linux/i2c-mux.h>
> > > #include <linux/module.h>
> > > @@ -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 = &priv->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",
> > > + 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 (enable)
> > > + ret = priv->regulator
> > > + ? regulator_enable(priv->regulator)
> > > + : max9286_gpio_set(priv, priv->gpio_poc,
> > > + enable ^ priv->gpio_poc_flags);
> > > + else
> > > + ret = priv->regulator
> > > + ? regulator_disable(priv->regulator)
> > > + : max9286_gpio_set(priv, priv->gpio_poc,
> > > + enable ^ priv->gpio_poc_flags);
> > > +
> > > + if (ret < 0)
> > > + dev_err(&priv->client->dev, "Unable to turn PoC %s\n",
> > > + enable ? "on" : "off");
> > >
> > > return ret;
> > > }
> > > @@ -1078,17 +1154,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(&client->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 +1171,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 +1187,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 +1359,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(&client->dev, "poc");
> > > - if (IS_ERR(priv->regulator)) {
> > > - if (PTR_ERR(priv->regulator) != -EPROBE_DEFER)
> > > - dev_err(&client->dev,
> > > - "Unable to get PoC regulator (%ld)\n",
> > > - PTR_ERR(priv->regulator));
> > > - ret = PTR_ERR(priv->regulator);
> > > - goto err_powerdown;
> > > - }
> > > -
> > > ret = max9286_parse_dt(priv);
> > > if (ret)
> > > goto err_powerdown;
> > > @@ -1326,7 +1389,7 @@ static int max9286_remove(struct i2c_client *client)
> > >
> > > max9286_v4l2_unregister(priv);
> > >
> > > - regulator_disable(priv->regulator);
> > > + max9286_poc_enable(priv, false);
> > >
> > > gpiod_set_value_cansleep(priv->gpiod_pwdn, 0);
> > >
> > >
> >
> > --
> > Regards
> > --
> > Kieran