[PATCH v4 0/5] media: i2c: IMX355 for the Pixel 3a

[PATCH v4 0/5] media: i2c: IMX355 for the Pixel 3a

[Richard Acayan]

This adds support for the IMX355 in devicetree and adds support for the
Pixel 3a front camera.

Changes since v3 (https://lore.kernel.org/r/20250905215516.289998-6-mailingradian@gmail.com):
- separate camera mclk pins and move to different patch (4/5, 5/5)
- remove polarity from rear camera pin (5/5)
- remove output-low from front camera pins (5/5)
- mention effects of dcf6fb89e6f7 ("media: qcom: camss: remove a check for unavailable CAMSS endpoint") (3/5)
- specify single clock-name without items nesting (1/5)
- rebase on 49c6ac166cf7 ("media: i2c: imx355: Replace client->dev
  usage") and eaa7d46d9654 ("media: i2c: imx335: Use V4L2 sensor clock
  helper") (2/5)
- do not use of_match_ptr for OF match table (2/5)
- remove redundant GPIO validity checks (2/5)
- describe endpoint data-lanes (1/5)

Changes since v2 (https://lore.kernel.org/r/20250714210227.714841-6-mailingradian@gmail.com):
- use devm_v4l2_sensor_clk_get (2/4)
- require supplies and clock-names (1/4)
- move unevaluatedProperties down (1/4)
- disable clocks as last power-off action (2/4)
- use 0 in gpio pin power-supply (4/4)

Changes since v1 (https://lore.kernel.org/r/20250630225944.320755-7-mailingradian@gmail.com):
- too much to have a complete list (1-4/4)
- squash camera orientation patch (4/4, previously 5/5)
- squash driver changes (2/4, previously 3/5)
- remove labelled endpoint node in sdm670.dtsi (3/4, 4/4)
- change init sequence to match other similar drivers (2/4)
- retrieve clock frequency from devicetree-defined clock (4/4)
- remove clock-frequency from dt-bindings (1/4)
- remove redundant descriptions of child nodes (1/4)
- switch initial drive of the reset GPIO to low (2/4)
- set mclk frequency to 19.2 MHz (4/4)
- add vdda-pll supply for camss (4/4)
- use common power on and off functions (2/4)
- use devm_clk_get_optional (2/4)
- remove extra layer when describing mclk pin (4/4)
- rename regulators (1/4, 2/4, 4/4)

Richard Acayan (5):
  dt-bindings: media: i2c: Add Sony IMX355
  media: i2c: imx355: Support devicetree and power management
  arm64: dts: qcom: sdm670: remove camss endpoint nodes
  arm64: dts: qcom: sdm670: add camera mclk pins
  arm64: dts: qcom: sdm670-google-sargo: add imx355 front camera

 .../bindings/media/i2c/sony,imx355.yaml       | 119 ++++++++++++++++++
 .../boot/dts/qcom/sdm670-google-sargo.dts     | 107 ++++++++++++++++
 arch/arm64/boot/dts/qcom/sdm670.dtsi          |  33 +++--
 drivers/media/i2c/imx355.c                    | 118 +++++++++++++++--
 4 files changed, 357 insertions(+), 20 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/media/i2c/sony,imx355.yaml

-- 
2.52.0

[PATCH v4 1/5] dt-bindings: media: i2c: Add Sony IMX355

[Richard Acayan]

The IMX355 camera sensor is a camera sensor that can be found as the
front camera in some smartphones, such as the Pixel 3, Pixel 3 XL, Pixel
3a, and Pixel 3a XL. It already has a driver, but needs support for
device tree. Document the IMX355 to support defining it in device tree.

Signed-off-by: Richard Acayan <mailingradian@gmail.com>
---
 .../bindings/media/i2c/sony,imx355.yaml       | 119 ++++++++++++++++++
 1 file changed, 119 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/i2c/sony,imx355.yaml

diff --git a/Documentation/devicetree/bindings/media/i2c/sony,imx355.yaml b/Documentation/devicetree/bindings/media/i2c/sony,imx355.yaml
new file mode 100644
index 000000000000..9aa2c7b7ea71
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/i2c/sony,imx355.yaml
@@ -0,0 +1,119 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/media/i2c/sony,imx355.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Sony IMX355 Sensor
+
+maintainers:
+  - Richard Acayan <mailingradian@gmail.com>
+
+description:
+  The IMX355 sensor is a 3280x2464 image sensor, commonly found as the front
+  camera in smartphones.
+
+allOf:
+  - $ref: /schemas/media/video-interface-devices.yaml#
+
+properties:
+  compatible:
+    const: sony,imx355
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    maxItems: 1
+
+  clock-names:
+    const: mclk
+
+  avdd-supply:
+    description: Analog power supply.
+
+  dvdd-supply:
+    description: Digital power supply.
+
+  dovdd-supply:
+    description: Interface power supply.
+
+  reset-gpios:
+    maxItems: 1
+
+  port:
+    $ref: /schemas/graph.yaml#/$defs/port-base
+    additionalProperties: false
+
+    properties:
+      endpoint:
+        $ref: /schemas/media/video-interfaces.yaml
+        unevaluatedProperties: false
+
+        data-lanes:
+          items:
+            - const: 0
+            - const: 1
+            - const: 2
+            - const: 3
+
+        required:
+          - link-frequencies
+          - data-lanes
+
+    required:
+      - endpoint
+
+required:
+  - compatible
+  - reg
+  - clocks
+  - clock-names
+  - avdd-supply
+  - dvdd-supply
+  - dovdd-supply
+  - port
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/clock/qcom,camcc-sdm845.h>
+    #include <dt-bindings/gpio/gpio.h>
+
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        camera@1a {
+            compatible = "sony,imx355";
+            reg = <0x1a>;
+
+            clocks = <&camcc CAM_CC_MCLK2_CLK>;
+            clock-names = "mclk";
+
+            assigned-clocks = <&camcc CAM_CC_MCLK2_CLK>;
+            assigned-clock-rates = <24000000>;
+
+            reset-gpios = <&tlmm 9 GPIO_ACTIVE_HIGH>;
+
+            avdd-supply = <&cam_front_ldo>;
+            dvdd-supply = <&cam_front_ldo>;
+            dovdd-supply = <&cam_vio_ldo>;
+
+            pinctrl-names = "default";
+            pinctrl-0 = <&cam_front_default>;
+
+            rotation = <270>;
+            orientation = <0>;
+
+            port {
+                cam_front_endpoint: endpoint {
+                    data-lanes = <0 1 2 3>;
+                    link-frequencies = /bits/ 64 <360000000>;
+                    remote-endpoint = <&camss_endpoint1>;
+                };
+            };
+        };
+    };
-- 
2.52.0

[PATCH v4 2/5] media: i2c: imx355: Support devicetree and power management

[Richard Acayan]

A device tree compatible makes it possible for this driver to be used on
Open Firmware devices. Initialization of power-managed resources such as
the reset GPIO and voltage regulators can be specified in the device
tree and handled by the driver. Add support for this so the Pixel 3a can
use the driver.

Signed-off-by: Richard Acayan <mailingradian@gmail.com>
---
 drivers/media/i2c/imx355.c | 118 ++++++++++++++++++++++++++++++++++---
 1 file changed, 110 insertions(+), 8 deletions(-)

diff --git a/drivers/media/i2c/imx355.c b/drivers/media/i2c/imx355.c
index 776107efe386..c225bb8959bd 100644
--- a/drivers/media/i2c/imx355.c
+++ b/drivers/media/i2c/imx355.c
@@ -3,9 +3,13 @@
 
 #include <linux/acpi.h>
 #include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/gpio/consumer.h>
 #include <linux/i2c.h>
 #include <linux/module.h>
+#include <linux/of.h>
 #include <linux/pm_runtime.h>
+#include <linux/regulator/consumer.h>
 #include <linux/unaligned.h>
 
 #include <media/v4l2-ctrls.h>
@@ -125,6 +129,15 @@ struct imx355 {
 	 * Protect access to sensor v4l2 controls.
 	 */
 	struct mutex mutex;
+
+	struct gpio_desc *reset_gpio;
+	struct regulator_bulk_data supplies[3];
+};
+
+static const char * const imx355_supply_names[] = {
+	"avdd",
+	"dvdd",
+	"dovdd",
 };
 
 static const struct imx355_reg imx355_global_regs[] = {
@@ -1515,6 +1528,54 @@ static const struct v4l2_subdev_internal_ops imx355_internal_ops = {
 	.open = imx355_open,
 };
 
+static int imx355_power_off(struct device *dev)
+{
+	struct i2c_client *client = container_of(dev, struct i2c_client, dev);
+	struct v4l2_subdev *sd = i2c_get_clientdata(client);
+	struct imx355 *imx355 = to_imx355(sd);
+
+	gpiod_set_value_cansleep(imx355->reset_gpio, 0);
+
+	regulator_bulk_disable(ARRAY_SIZE(imx355->supplies), imx355->supplies);
+	clk_disable_unprepare(imx355->clk);
+
+	return 0;
+}
+
+static int imx355_power_on(struct device *dev)
+{
+	struct i2c_client *client = container_of(dev, struct i2c_client, dev);
+	struct v4l2_subdev *sd = i2c_get_clientdata(client);
+	struct imx355 *imx355 = to_imx355(sd);
+	int ret;
+
+	ret = clk_prepare_enable(imx355->clk);
+	if (ret) {
+		dev_err(dev, "failed to enable clocks: %d\n", ret);
+		return ret;
+	}
+
+	ret = regulator_bulk_enable(ARRAY_SIZE(imx355->supplies),
+				    imx355->supplies);
+	if (ret) {
+		dev_err(dev, "failed to enable regulators: %d\n", ret);
+		goto error_disable_clocks;
+	}
+
+	usleep_range(5000, 5100);
+	gpiod_set_value_cansleep(imx355->reset_gpio, 1);
+	usleep_range(8000, 8100);
+
+	return 0;
+
+error_disable_clocks:
+	clk_disable_unprepare(imx355->clk);
+	return ret;
+}
+
+static DEFINE_RUNTIME_DEV_PM_OPS(imx355_pm_ops, imx355_power_off,
+				 imx355_power_on, NULL);
+
 /* Initialize control handlers */
 static int imx355_init_controls(struct imx355 *imx355)
 {
@@ -1668,6 +1729,7 @@ static int imx355_probe(struct i2c_client *client)
 {
 	struct imx355 *imx355;
 	unsigned long freq;
+	size_t i;
 	int ret;
 
 	imx355 = devm_kzalloc(&client->dev, sizeof(*imx355), GFP_KERNEL);
@@ -1678,7 +1740,7 @@ static int imx355_probe(struct i2c_client *client)
 
 	mutex_init(&imx355->mutex);
 
-	imx355->clk = devm_v4l2_sensor_clk_get(imx355->dev, NULL);
+	imx355->clk = devm_v4l2_sensor_clk_get(imx355->dev, "mclk");
 	if (IS_ERR(imx355->clk))
 		return dev_err_probe(imx355->dev, PTR_ERR(imx355->clk),
 				     "failed to get clock\n");
@@ -1689,16 +1751,28 @@ static int imx355_probe(struct i2c_client *client)
 				     "external clock %lu is not supported\n",
 				     freq);
 
-	/* Initialize subdev */
-	v4l2_i2c_subdev_init(&imx355->sd, client, &imx355_subdev_ops);
+	for (i = 0; i < ARRAY_SIZE(imx355_supply_names); i++)
+		imx355->supplies[i].supply = imx355_supply_names[i];
 
-	/* Check module identity */
-	ret = imx355_identify_module(imx355);
+	ret = devm_regulator_bulk_get(imx355->dev,
+				      ARRAY_SIZE(imx355->supplies),
+				      imx355->supplies);
 	if (ret) {
-		dev_err(imx355->dev, "failed to find sensor: %d", ret);
+		dev_err_probe(imx355->dev, ret, "could not get regulators");
 		goto error_probe;
 	}
 
+	imx355->reset_gpio = devm_gpiod_get_optional(imx355->dev, "reset",
+						     GPIOD_OUT_LOW);
+	if (IS_ERR(imx355->reset_gpio)) {
+		ret = dev_err_probe(imx355->dev, PTR_ERR(imx355->reset_gpio),
+				    "failed to get gpios");
+		goto error_probe;
+	}
+
+	/* Initialize subdev */
+	v4l2_i2c_subdev_init(&imx355->sd, client, &imx355_subdev_ops);
+
 	imx355->hwcfg = imx355_get_hwcfg(imx355->dev);
 	if (!imx355->hwcfg) {
 		dev_err(imx355->dev, "failed to get hwcfg");
@@ -1706,13 +1780,26 @@ static int imx355_probe(struct i2c_client *client)
 		goto error_probe;
 	}
 
+	ret = imx355_power_on(imx355->dev);
+	if (ret) {
+		dev_err(imx355->dev, "failed to power on sensor: %d", ret);
+		goto error_probe;
+	}
+
+	/* Check module identity */
+	ret = imx355_identify_module(imx355);
+	if (ret) {
+		dev_err(imx355->dev, "failed to find sensor: %d", ret);
+		goto error_power_off;
+	}
+
 	/* Set default mode to max resolution */
 	imx355->cur_mode = &supported_modes[0];
 
 	ret = imx355_init_controls(imx355);
 	if (ret) {
 		dev_err(imx355->dev, "failed to init controls: %d", ret);
-		goto error_probe;
+		goto error_power_off;
 	}
 
 	/* Initialize subdev */
@@ -1752,6 +1839,9 @@ static int imx355_probe(struct i2c_client *client)
 error_handler_free:
 	v4l2_ctrl_handler_free(imx355->sd.ctrl_handler);
 
+error_power_off:
+	imx355_power_off(imx355->dev);
+
 error_probe:
 	mutex_destroy(&imx355->mutex);
 
@@ -1768,7 +1858,11 @@ static void imx355_remove(struct i2c_client *client)
 	v4l2_ctrl_handler_free(sd->ctrl_handler);
 
 	pm_runtime_disable(imx355->dev);
-	pm_runtime_set_suspended(imx355->dev);
+
+	if (!pm_runtime_status_suspended(imx355->dev)) {
+		imx355_power_off(imx355->dev);
+		pm_runtime_set_suspended(imx355->dev);
+	}
 
 	mutex_destroy(&imx355->mutex);
 }
@@ -1779,10 +1873,18 @@ static const struct acpi_device_id imx355_acpi_ids[] __maybe_unused = {
 };
 MODULE_DEVICE_TABLE(acpi, imx355_acpi_ids);
 
+static const struct of_device_id imx355_match_table[] __maybe_unused = {
+	{ .compatible = "sony,imx355", },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, imx355_match_table);
+
 static struct i2c_driver imx355_i2c_driver = {
 	.driver = {
 		.name = "imx355",
 		.acpi_match_table = ACPI_PTR(imx355_acpi_ids),
+		.of_match_table = imx355_match_table,
+		.pm = &imx355_pm_ops,
 	},
 	.probe = imx355_probe,
 	.remove = imx355_remove,
-- 
2.52.0

[PATCH v4 3/5] arm64: dts: qcom: sdm670: remove camss endpoint nodes

[Richard Acayan]

There is no need to add these by default for all of SDM670. Originally,
they were added so there could be a label for each port. This is
unnecessary if the endpoints are all added in a fixup to the camss node.

This is required since dcf6fb89e6f7 ("media: qcom: camss: remove a check
for unavailable CAMSS endpoint") was applied, forcing all endpoint nodes
to be probed, even if they are marked as disabled. According to the body
of this commit, there is "no valid or sane usecase".

Suggested-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
Link: https://lore.kernel.org/r/488281f6-5e5d-4864-8220-63e2a0b2d7f2@linaro.org
Signed-off-by: Richard Acayan <mailingradian@gmail.com>
---
 arch/arm64/boot/dts/qcom/sdm670.dtsi | 12 ------------
 1 file changed, 12 deletions(-)

diff --git a/arch/arm64/boot/dts/qcom/sdm670.dtsi b/arch/arm64/boot/dts/qcom/sdm670.dtsi
index c33f3de779f6..c275089237e4 100644
--- a/arch/arm64/boot/dts/qcom/sdm670.dtsi
+++ b/arch/arm64/boot/dts/qcom/sdm670.dtsi
@@ -1768,26 +1768,14 @@ ports {
 
 				port@0 {
 					reg = <0>;
-
-					camss_endpoint0: endpoint {
-						status = "disabled";
-					};
 				};
 
 				port@1 {
 					reg = <1>;
-
-					camss_endpoint1: endpoint {
-						status = "disabled";
-					};
 				};
 
 				port@2 {
 					reg = <2>;
-
-					camss_endpoint2: endpoint {
-						status = "disabled";
-					};
 				};
 			};
 		};
-- 
2.52.0

[PATCH v4 4/5] arm64: dts: qcom: sdm670: add camera mclk pins

[Richard Acayan]

The camera subsystem is added for the SoC common devicetree, but the
mclk pins should also be common across the SoC. Add the mclk pins for
the cameras.

Suggested-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
Link: https://lore.kernel.org/r/5135823c-f2e4-4873-9e3a-9d190cac0113@oss.qualcomm.com
Signed-off-by: Richard Acayan <mailingradian@gmail.com>
---
 arch/arm64/boot/dts/qcom/sdm670.dtsi | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sdm670.dtsi b/arch/arm64/boot/dts/qcom/sdm670.dtsi
index c275089237e4..69e84cd8ed27 100644
--- a/arch/arm64/boot/dts/qcom/sdm670.dtsi
+++ b/arch/arm64/boot/dts/qcom/sdm670.dtsi
@@ -1190,6 +1190,27 @@ tlmm: pinctrl@3400000 {
 			gpio-ranges = <&tlmm 0 0 151>;
 			wakeup-parent = <&pdc>;
 
+			cam_mclk0_default: cam-mclk0-default-state {
+				pins = "gpio13";
+				function = "cam_mclk";
+				drive-strength = <2>;
+				bias-disable;
+			};
+
+			cam_mclk1_default: cam-mclk1-default-state {
+				pins = "gpio14";
+				function = "cam_mclk";
+				drive-strength = <2>;
+				bias-disable;
+			};
+
+			cam_mclk2_default: cam-mclk2-default-state {
+				pins = "gpio15";
+				function = "cam_mclk";
+				drive-strength = <2>;
+				bias-disable;
+			};
+
 			cci0_default: cci0-default-state {
 				pins = "gpio17", "gpio18";
 				function = "cci_i2c";
-- 
2.52.0

[PATCH v4 5/5] arm64: dts: qcom: sdm670-google-sargo: add imx355 front camera

[Richard Acayan]

The Sony IMX355 is the front camera on the Pixel 3a, mounted in portrait
mode. It is connected to CSIPHY1 and CCI I2C1, and uses MCLK2. Add
support for it.

Co-developed-by: Robert Mader <robert.mader@collabora.com>
Signed-off-by: Robert Mader <robert.mader@collabora.com>
Signed-off-by: Richard Acayan <mailingradian@gmail.com>
---
 .../boot/dts/qcom/sdm670-google-sargo.dts     | 107 ++++++++++++++++++
 1 file changed, 107 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sdm670-google-sargo.dts b/arch/arm64/boot/dts/qcom/sdm670-google-sargo.dts
index d01422844fbf..ede0ad7ded23 100644
--- a/arch/arm64/boot/dts/qcom/sdm670-google-sargo.dts
+++ b/arch/arm64/boot/dts/qcom/sdm670-google-sargo.dts
@@ -172,6 +172,34 @@ vreg_s2b_1p05: vreg-s2b-regulator {
 		regulator-min-microvolt = <1050000>;
 		regulator-max-microvolt = <1050000>;
 	};
+
+	cam_front_ldo: cam-front-ldo-regulator {
+		compatible = "regulator-fixed";
+		regulator-name = "cam_front_ldo";
+		regulator-min-microvolt = <1352000>;
+		regulator-max-microvolt = <1352000>;
+		regulator-enable-ramp-delay = <135>;
+
+		gpios = <&pm660l_gpios 4 GPIO_ACTIVE_HIGH>;
+		enable-active-high;
+
+		pinctrl-0 = <&cam_front_ldo_pin>;
+		pinctrl-names = "default";
+	};
+
+	cam_vio_ldo: cam-vio-ldo-regulator {
+		compatible = "regulator-fixed";
+		regulator-name = "cam_vio_ldo";
+		regulator-min-microvolt = <1800000>;
+		regulator-max-microvolt = <1800000>;
+		regulator-enable-ramp-delay = <233>;
+
+		gpios = <&pm660_gpios 13 GPIO_ACTIVE_HIGH>;
+		enable-active-high;
+
+		pinctrl-0 = <&cam_vio_pin>;
+		pinctrl-names = "default";
+	};
 };
 
 &apps_rsc {
@@ -392,6 +420,64 @@ vreg_bob: bob {
 	};
 };
 
+&camss {
+	vdda-phy-supply = <&vreg_l1a_1p225>;
+	vdda-pll-supply = <&vreg_s6a_0p87>;
+
+	status = "okay";
+
+	ports {
+		port@1 {
+			camss_endpoint1: endpoint {
+				clock-lanes = <7>;
+				data-lanes = <0 1 2 3>;
+				remote-endpoint = <&cam_front_endpoint>;
+			};
+		};
+	};
+};
+
+&cci {
+	pinctrl-0 = <&cci1_default>;
+	pinctrl-1 = <&cci1_sleep>;
+	pinctrl-names = "default", "sleep";
+
+	status = "okay";
+};
+
+&cci_i2c1 {
+	camera@1a {
+		compatible = "sony,imx355";
+		reg = <0x1a>;
+
+		clocks = <&camcc CAM_CC_MCLK2_CLK>;
+		clock-names = "mclk";
+
+		assigned-clocks = <&camcc CAM_CC_MCLK2_CLK>;
+		assigned-clock-rates = <19200000>;
+
+		reset-gpios = <&tlmm 9 GPIO_ACTIVE_HIGH>;
+
+		avdd-supply = <&cam_front_ldo>;
+		dvdd-supply = <&cam_front_ldo>;
+		dovdd-supply = <&cam_vio_ldo>;
+
+		pinctrl-0 = <&cam_front_default &cam_mclk2_default>;
+		pinctrl-names = "default";
+
+		rotation = <270>;
+		orientation = <0>;
+
+		port {
+			cam_front_endpoint: endpoint {
+				data-lanes = <0 1 2 3>;
+				link-frequencies = /bits/ 64 <360000000>;
+				remote-endpoint = <&camss_endpoint1>;
+			};
+		};
+	};
+};
+
 &gcc {
 	protected-clocks = <GCC_QSPI_CORE_CLK>,
 			   <GCC_QSPI_CORE_CLK_SRC>,
@@ -491,6 +577,14 @@ &pm660_charger {
 	status = "okay";
 };
 
+&pm660_gpios {
+	cam_vio_pin: cam-vio-state {
+		pins = "gpio13";
+		function = "normal";
+		power-source = <0>;
+	};
+};
+
 &pm660_rradc {
 	status = "okay";
 };
@@ -509,6 +603,12 @@ led-0 {
 };
 
 &pm660l_gpios {
+	cam_front_ldo_pin: cam-front-state {
+		pins = "gpio4";
+		function = "normal";
+		power-source = <0>;
+	};
+
 	vol_up_pin: vol-up-state {
 		pins = "gpio7";
 		function = "normal";
@@ -548,6 +648,13 @@ &sdhc_1 {
 &tlmm {
 	gpio-reserved-ranges = <0 4>, <81 4>;
 
+	cam_front_default: cam-front-default-state {
+		pins = "gpio9";
+		function = "gpio";
+		drive-strength = <2>;
+		bias-disable;
+	};
+
 	panel_default: panel-default-state {
 		te-pins {
 			pins = "gpio10";
-- 
2.52.0

Re: [PATCH v4 1/5] dt-bindings: media: i2c: Add Sony IMX355

[Krzysztof Kozlowski]

On 11/12/2025 02:48, Richard Acayan wrote:
> The IMX355 camera sensor is a camera sensor that can be found as the
> front camera in some smartphones, such as the Pixel 3, Pixel 3 XL, Pixel
> 3a, and Pixel 3a XL. It already has a driver, but needs support for
> device tree. Document the IMX355 to support defining it in device tree.
> 
> Signed-off-by: Richard Acayan <mailingradian@gmail.com>
> ---


Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@oss.qualcomm.com>

Best regards,
Krzysztof

Re: [PATCH v4 1/5] dt-bindings: media: i2c: Add Sony IMX355

[Rob Herring (Arm)]

On Wed, 10 Dec 2025 20:48:42 -0500, Richard Acayan wrote:
> The IMX355 camera sensor is a camera sensor that can be found as the
> front camera in some smartphones, such as the Pixel 3, Pixel 3 XL, Pixel
> 3a, and Pixel 3a XL. It already has a driver, but needs support for
> device tree. Document the IMX355 to support defining it in device tree.
> 
> Signed-off-by: Richard Acayan <mailingradian@gmail.com>
> ---
>  .../bindings/media/i2c/sony,imx355.yaml       | 119 ++++++++++++++++++
>  1 file changed, 119 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/media/i2c/sony,imx355.yaml
> 

My bot found errors running 'make dt_binding_check' on your patch:

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/media/i2c/sony,imx355.yaml: properties:port:properties:endpoint: 'anyOf' conditional failed, one must be fixed:
	'data-lanes' is not one of ['$ref', 'additionalItems', 'additionalProperties', 'allOf', 'anyOf', 'const', 'contains', 'default', 'dependencies', 'dependentRequired', 'dependentSchemas', 'deprecated', 'description', 'else', 'enum', 'exclusiveMaximum', 'exclusiveMinimum', 'items', 'if', 'minItems', 'minimum', 'maxItems', 'maximum', 'multipleOf', 'not', 'oneOf', 'pattern', 'patternProperties', 'properties', 'required', 'then', 'typeSize', 'unevaluatedProperties', 'uniqueItems']
	'type' was expected
	from schema $id: http://devicetree.org/meta-schemas/keywords.yaml

doc reference errors (make refcheckdocs):

See https://patchwork.kernel.org/project/devicetree/patch/20251211014846.16602-2-mailingradian@gmail.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

Re: [PATCH v4 1/5] dt-bindings: media: i2c: Add Sony IMX355

[Krzysztof Kozlowski]

On 11/12/2025 02:48, Richard Acayan wrote:
> +
> +    properties:
> +      endpoint:
> +        $ref: /schemas/media/video-interfaces.yaml
> +        unevaluatedProperties: false
> +

Ah, here, this obviously was not ever tested. Heh, please do not use the
community as a testing service. Missing props...

> +        data-lanes:
> +          items:
> +            - const: 0
> +            - const: 1
> +            - const: 2
> +            - const: 3


Best regards,
Krzysztof

Re: [PATCH v4 1/5] dt-bindings: media: i2c: Add Sony IMX355

[Krzysztof Kozlowski]

On 11/12/2025 02:48, Richard Acayan wrote:
co> +
> +  port:
> +    $ref: /schemas/graph.yaml#/$defs/port-base
> +    additionalProperties: false
> +
> +    properties:
> +      endpoint:
> +        $ref: /schemas/media/video-interfaces.yaml
> +        unevaluatedProperties: false
> +
> +        data-lanes:
> +          items:
> +            - const: 0
> +            - const: 1
> +            - const: 2
> +            - const: 3

Obviously untested code but another thought: why do you need data-lanes
if they are fixed? They are implied by the compatible.


> +
> +        required:
> +          - link-frequencies
> +          - data-lanes
> +


Best regards,
Krzysztof

Re: [PATCH v4 5/5] arm64: dts: qcom: sdm670-google-sargo: add imx355 front camera

[Dmitry Baryshkov]

On Wed, Dec 10, 2025 at 08:48:46PM -0500, Richard Acayan wrote:
> The Sony IMX355 is the front camera on the Pixel 3a, mounted in portrait
> mode. It is connected to CSIPHY1 and CCI I2C1, and uses MCLK2. Add
> support for it.
> 
> Co-developed-by: Robert Mader <robert.mader@collabora.com>
> Signed-off-by: Robert Mader <robert.mader@collabora.com>
> Signed-off-by: Richard Acayan <mailingradian@gmail.com>
> ---
>  .../boot/dts/qcom/sdm670-google-sargo.dts     | 107 ++++++++++++++++++
>  1 file changed, 107 insertions(+)
> 
> @@ -392,6 +420,64 @@ vreg_bob: bob {
>  	};
>  };
>  
> +&camss {
> +	vdda-phy-supply = <&vreg_l1a_1p225>;
> +	vdda-pll-supply = <&vreg_s6a_0p87>;
> +
> +	status = "okay";
> +
> +	ports {
> +		port@1 {
> +			camss_endpoint1: endpoint {
> +				clock-lanes = <7>;
> +				data-lanes = <0 1 2 3>;
> +				remote-endpoint = <&cam_front_endpoint>;
> +			};
> +		};
> +	};

This would be much better:

  &camss_endpoint1: {
      clock-lanes, data-lanes, remote-endpoint here
  };


> +};

-- 
With best wishes
Dmitry

Re: [PATCH v4 1/5] dt-bindings: media: i2c: Add Sony IMX355

[Vladimir Zapolskiy]

Hi Richard.

On 12/11/25 03:48, Richard Acayan wrote:
> The IMX355 camera sensor is a camera sensor that can be found as the
> front camera in some smartphones, such as the Pixel 3, Pixel 3 XL, Pixel
> 3a, and Pixel 3a XL. It already has a driver, but needs support for
> device tree. Document the IMX355 to support defining it in device tree.
> 
> Signed-off-by: Richard Acayan <mailingradian@gmail.com>
> ---
>   .../bindings/media/i2c/sony,imx355.yaml       | 119 ++++++++++++++++++
>   1 file changed, 119 insertions(+)
>   create mode 100644 Documentation/devicetree/bindings/media/i2c/sony,imx355.yaml
> 
> diff --git a/Documentation/devicetree/bindings/media/i2c/sony,imx355.yaml b/Documentation/devicetree/bindings/media/i2c/sony,imx355.yaml
> new file mode 100644
> index 000000000000..9aa2c7b7ea71
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/i2c/sony,imx355.yaml
> @@ -0,0 +1,119 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/media/i2c/sony,imx355.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Sony IMX355 Sensor
> +
> +maintainers:
> +  - Richard Acayan <mailingradian@gmail.com>
> +
> +description:
> +  The IMX355 sensor is a 3280x2464 image sensor, commonly found as the front
> +  camera in smartphones.
> +
> +allOf:
> +  - $ref: /schemas/media/video-interface-devices.yaml#
> +
> +properties:
> +  compatible:
> +    const: sony,imx355
> +
> +  reg:
> +    maxItems: 1
> +
> +  clocks:
> +    maxItems: 1
> +
> +  clock-names:
> +    const: mclk
> +
> +  avdd-supply:
> +    description: Analog power supply.
> +
> +  dvdd-supply:
> +    description: Digital power supply.
> +
> +  dovdd-supply:
> +    description: Interface power supply.
> +
> +  reset-gpios:
> +    maxItems: 1
> +
> +  port:
> +    $ref: /schemas/graph.yaml#/$defs/port-base
> +    additionalProperties: false
> +
> +    properties:
> +      endpoint:
> +        $ref: /schemas/media/video-interfaces.yaml
> +        unevaluatedProperties: false
> +
> +        data-lanes:
> +          items:
> +            - const: 0
> +            - const: 1
> +            - const: 2
> +            - const: 3

Please enumerate data lanes starting from 1.

> +
> +        required:
> +          - link-frequencies
> +          - data-lanes

If the sensor does not support any other lanes configuration,
the property can be optional.

> +
> +    required:
> +      - endpoint
> +
> +required:
> +  - compatible
> +  - reg
> +  - clocks
> +  - clock-names
> +  - avdd-supply
> +  - dvdd-supply
> +  - dovdd-supply
> +  - port
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/clock/qcom,camcc-sdm845.h>
> +    #include <dt-bindings/gpio/gpio.h>
> +
> +    i2c {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        camera@1a {
> +            compatible = "sony,imx355";
> +            reg = <0x1a>;
> +
> +            clocks = <&camcc CAM_CC_MCLK2_CLK>;
> +            clock-names = "mclk";
> +
> +            assigned-clocks = <&camcc CAM_CC_MCLK2_CLK>;
> +            assigned-clock-rates = <24000000>;
> +
> +            reset-gpios = <&tlmm 9 GPIO_ACTIVE_HIGH>;
> +
> +            avdd-supply = <&cam_front_ldo>;
> +            dvdd-supply = <&cam_front_ldo>;
> +            dovdd-supply = <&cam_vio_ldo>;
> +
> +            pinctrl-names = "default";
> +            pinctrl-0 = <&cam_front_default>;
> +
> +            rotation = <270>;
> +            orientation = <0>;
> +
> +            port {
> +                cam_front_endpoint: endpoint {
> +                    data-lanes = <0 1 2 3>;
> +                    link-frequencies = /bits/ 64 <360000000>;
> +                    remote-endpoint = <&camss_endpoint1>;
> +                };
> +            };
> +        };
> +    };

-- 
Best wishes,
Vladimir

Re: [PATCH v4 2/5] media: i2c: imx355: Support devicetree and power management

[David Heidelberg]

On 11/12/2025 02:48, Richard Acayan wrote:
> A device tree compatible makes it possible for this driver to be used on
> Open Firmware devices. Initialization of power-managed resources such as
> the reset GPIO and voltage regulators can be specified in the device
> tree and handled by the driver. Add support for this so the Pixel 3a can
> use the driver.
> 
> Signed-off-by: Richard Acayan <mailingradian@gmail.com>
> ---
>   drivers/media/i2c/imx355.c | 118 ++++++++++++++++++++++++++++++++++---
>   1 file changed, 110 insertions(+), 8 deletions(-)
> 

[...]

> -	/* Check module identity */
> -	ret = imx355_identify_module(imx355);
> +	ret = devm_regulator_bulk_get(imx355->dev,
> +				      ARRAY_SIZE(imx355->supplies),
> +				      imx355->supplies);
>   	if (ret) {
> -		dev_err(imx355->dev, "failed to find sensor: %d", ret);
> +		dev_err_probe(imx355->dev, ret, "could not get regulators");
>   		goto error_probe;
>   	}

In general patch looks good, but you should use 
devm_regulator_bulk_get_const here (see some conversions on ML done).

David

[...]
-- 
David Heidelberg

Re: [PATCH v4 3/5] arm64: dts: qcom: sdm670: remove camss endpoint nodes

[Vladimir Zapolskiy]

On 12/11/25 03:48, Richard Acayan wrote:
> There is no need to add these by default for all of SDM670. Originally,
> they were added so there could be a label for each port. This is
> unnecessary if the endpoints are all added in a fixup to the camss node.
> 
> This is required since dcf6fb89e6f7 ("media: qcom: camss: remove a check
> for unavailable CAMSS endpoint") was applied, forcing all endpoint nodes
> to be probed, even if they are marked as disabled. According to the body
> of this commit, there is "no valid or sane usecase".

Right, endpoints are not devices.

> 
> Suggested-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> Link: https://lore.kernel.org/r/488281f6-5e5d-4864-8220-63e2a0b2d7f2@linaro.org
> Signed-off-by: Richard Acayan <mailingradian@gmail.com>

Reviewed-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>

-- 
Best wishes,
Vladimir

Re: [PATCH v4 4/5] arm64: dts: qcom: sdm670: add camera mclk pins

[Vladimir Zapolskiy]

On 12/11/25 03:48, Richard Acayan wrote:
> The camera subsystem is added for the SoC common devicetree, but the
> mclk pins should also be common across the SoC. Add the mclk pins for
> the cameras.
> 
> Suggested-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
> Link: https://lore.kernel.org/r/5135823c-f2e4-4873-9e3a-9d190cac0113@oss.qualcomm.com
> Signed-off-by: Richard Acayan <mailingradian@gmail.com>

Reviewed-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>

-- 
Best wishes,
Vladimir

Re: [PATCH v4 5/5] arm64: dts: qcom: sdm670-google-sargo: add imx355 front camera

[Vladimir Zapolskiy]

On 12/11/25 03:48, Richard Acayan wrote:
> The Sony IMX355 is the front camera on the Pixel 3a, mounted in portrait
> mode. It is connected to CSIPHY1 and CCI I2C1, and uses MCLK2. Add
> support for it.
> 
> Co-developed-by: Robert Mader <robert.mader@collabora.com>
> Signed-off-by: Robert Mader <robert.mader@collabora.com>
> Signed-off-by: Richard Acayan <mailingradian@gmail.com>

Reviewed-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>

-- 
Best wishes,
Vladimir

Re: [PATCH v4 2/5] media: i2c: imx355: Support devicetree and power management

[Vladimir Zapolskiy]

On 12/11/25 03:48, Richard Acayan wrote:
> A device tree compatible makes it possible for this driver to be used on
> Open Firmware devices. Initialization of power-managed resources such as
> the reset GPIO and voltage regulators can be specified in the device
> tree and handled by the driver. Add support for this so the Pixel 3a can
> use the driver.
> 
> Signed-off-by: Richard Acayan <mailingradian@gmail.com>
> ---
>   drivers/media/i2c/imx355.c | 118 ++++++++++++++++++++++++++++++++++---
>   1 file changed, 110 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/media/i2c/imx355.c b/drivers/media/i2c/imx355.c
> index 776107efe386..c225bb8959bd 100644
> --- a/drivers/media/i2c/imx355.c
> +++ b/drivers/media/i2c/imx355.c
> @@ -3,9 +3,13 @@
>   
>   #include <linux/acpi.h>
>   #include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/gpio/consumer.h>
>   #include <linux/i2c.h>
>   #include <linux/module.h>
> +#include <linux/of.h>
>   #include <linux/pm_runtime.h>
> +#include <linux/regulator/consumer.h>
>   #include <linux/unaligned.h>
>   
>   #include <media/v4l2-ctrls.h>
> @@ -125,6 +129,15 @@ struct imx355 {
>   	 * Protect access to sensor v4l2 controls.
>   	 */
>   	struct mutex mutex;
> +
> +	struct gpio_desc *reset_gpio;
> +	struct regulator_bulk_data supplies[3];
> +};
> +
> +static const char * const imx355_supply_names[] = {
> +	"avdd",
> +	"dvdd",
> +	"dovdd",
>   };
>   
>   static const struct imx355_reg imx355_global_regs[] = {
> @@ -1515,6 +1528,54 @@ static const struct v4l2_subdev_internal_ops imx355_internal_ops = {
>   	.open = imx355_open,
>   };
>   
> +static int imx355_power_off(struct device *dev)
> +{
> +	struct i2c_client *client = container_of(dev, struct i2c_client, dev);
> +	struct v4l2_subdev *sd = i2c_get_clientdata(client);
> +	struct imx355 *imx355 = to_imx355(sd);
> +
> +	gpiod_set_value_cansleep(imx355->reset_gpio, 0);

It's expected that the reset/shutdown GPIO is active low.

> +
> +	regulator_bulk_disable(ARRAY_SIZE(imx355->supplies), imx355->supplies);
> +	clk_disable_unprepare(imx355->clk);
> +
> +	return 0;
> +}
> +
> +static int imx355_power_on(struct device *dev)
> +{
> +	struct i2c_client *client = container_of(dev, struct i2c_client, dev);
> +	struct v4l2_subdev *sd = i2c_get_clientdata(client);
> +	struct imx355 *imx355 = to_imx355(sd);
> +	int ret;
> +
> +	ret = clk_prepare_enable(imx355->clk);
> +	if (ret) {
> +		dev_err(dev, "failed to enable clocks: %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = regulator_bulk_enable(ARRAY_SIZE(imx355->supplies),
> +				    imx355->supplies);
> +	if (ret) {
> +		dev_err(dev, "failed to enable regulators: %d\n", ret);
> +		goto error_disable_clocks;
> +	}
> +
> +	usleep_range(5000, 5100);
> +	gpiod_set_value_cansleep(imx355->reset_gpio, 1);
> +	usleep_range(8000, 8100);
> +
> +	return 0;
> +
> +error_disable_clocks:
> +	clk_disable_unprepare(imx355->clk);
> +	return ret;
> +}
> +
> +static DEFINE_RUNTIME_DEV_PM_OPS(imx355_pm_ops, imx355_power_off,
> +				 imx355_power_on, NULL);
> +
>   /* Initialize control handlers */
>   static int imx355_init_controls(struct imx355 *imx355)
>   {
> @@ -1668,6 +1729,7 @@ static int imx355_probe(struct i2c_client *client)
>   {
>   	struct imx355 *imx355;
>   	unsigned long freq;
> +	size_t i;
>   	int ret;
>   
>   	imx355 = devm_kzalloc(&client->dev, sizeof(*imx355), GFP_KERNEL);
> @@ -1678,7 +1740,7 @@ static int imx355_probe(struct i2c_client *client)
>   
>   	mutex_init(&imx355->mutex);
>   
> -	imx355->clk = devm_v4l2_sensor_clk_get(imx355->dev, NULL);
> +	imx355->clk = devm_v4l2_sensor_clk_get(imx355->dev, "mclk");

Please keep an unset clock name, NULL is fine here, and it removes
a risk of the regression.

>   	if (IS_ERR(imx355->clk))
>   		return dev_err_probe(imx355->dev, PTR_ERR(imx355->clk),
>   				     "failed to get clock\n");
> @@ -1689,16 +1751,28 @@ static int imx355_probe(struct i2c_client *client)
>   				     "external clock %lu is not supported\n",
>   				     freq);
>   
> -	/* Initialize subdev */
> -	v4l2_i2c_subdev_init(&imx355->sd, client, &imx355_subdev_ops);
> +	for (i = 0; i < ARRAY_SIZE(imx355_supply_names); i++)
> +		imx355->supplies[i].supply = imx355_supply_names[i];
>   
> -	/* Check module identity */
> -	ret = imx355_identify_module(imx355);
> +	ret = devm_regulator_bulk_get(imx355->dev,
> +				      ARRAY_SIZE(imx355->supplies),
> +				      imx355->supplies);
>   	if (ret) {
> -		dev_err(imx355->dev, "failed to find sensor: %d", ret);
> +		dev_err_probe(imx355->dev, ret, "could not get regulators");
>   		goto error_probe;
>   	}
>   
> +	imx355->reset_gpio = devm_gpiod_get_optional(imx355->dev, "reset",
> +						     GPIOD_OUT_LOW);

While switching the logic to active low GPIO, it should be GPIOD_OUT_HIGH here.

> +	if (IS_ERR(imx355->reset_gpio)) {
> +		ret = dev_err_probe(imx355->dev, PTR_ERR(imx355->reset_gpio),
> +				    "failed to get gpios");
> +		goto error_probe;
> +	}
> +
> +	/* Initialize subdev */
> +	v4l2_i2c_subdev_init(&imx355->sd, client, &imx355_subdev_ops);
> +
>   	imx355->hwcfg = imx355_get_hwcfg(imx355->dev);
>   	if (!imx355->hwcfg) {
>   		dev_err(imx355->dev, "failed to get hwcfg");
> @@ -1706,13 +1780,26 @@ static int imx355_probe(struct i2c_client *client)
>   		goto error_probe;
>   	}
>   
> +	ret = imx355_power_on(imx355->dev);
> +	if (ret) {
> +		dev_err(imx355->dev, "failed to power on sensor: %d", ret);
> +		goto error_probe;
> +	}
> +
> +	/* Check module identity */
> +	ret = imx355_identify_module(imx355);
> +	if (ret) {
> +		dev_err(imx355->dev, "failed to find sensor: %d", ret);
> +		goto error_power_off;
> +	}
> +
>   	/* Set default mode to max resolution */
>   	imx355->cur_mode = &supported_modes[0];
>   
>   	ret = imx355_init_controls(imx355);
>   	if (ret) {
>   		dev_err(imx355->dev, "failed to init controls: %d", ret);
> -		goto error_probe;
> +		goto error_power_off;
>   	}
>   
>   	/* Initialize subdev */
> @@ -1752,6 +1839,9 @@ static int imx355_probe(struct i2c_client *client)
>   error_handler_free:
>   	v4l2_ctrl_handler_free(imx355->sd.ctrl_handler);
>   
> +error_power_off:
> +	imx355_power_off(imx355->dev);
> +
>   error_probe:
>   	mutex_destroy(&imx355->mutex);
>   
> @@ -1768,7 +1858,11 @@ static void imx355_remove(struct i2c_client *client)
>   	v4l2_ctrl_handler_free(sd->ctrl_handler);
>   
>   	pm_runtime_disable(imx355->dev);
> -	pm_runtime_set_suspended(imx355->dev);
> +
> +	if (!pm_runtime_status_suspended(imx355->dev)) {
> +		imx355_power_off(imx355->dev);
> +		pm_runtime_set_suspended(imx355->dev);
> +	}
>   
>   	mutex_destroy(&imx355->mutex);
>   }
> @@ -1779,10 +1873,18 @@ static const struct acpi_device_id imx355_acpi_ids[] __maybe_unused = {
>   };
>   MODULE_DEVICE_TABLE(acpi, imx355_acpi_ids);
>   
> +static const struct of_device_id imx355_match_table[] __maybe_unused = {
> +	{ .compatible = "sony,imx355", },
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, imx355_match_table);
> +
>   static struct i2c_driver imx355_i2c_driver = {
>   	.driver = {
>   		.name = "imx355",
>   		.acpi_match_table = ACPI_PTR(imx355_acpi_ids),
> +		.of_match_table = imx355_match_table,
> +		.pm = &imx355_pm_ops,
>   	},
>   	.probe = imx355_probe,
>   	.remove = imx355_remove,

-- 
Best wishes,
Vladimir

Re: [PATCH v4 1/5] dt-bindings: media: i2c: Add Sony IMX355

[Vladimir Zapolskiy]

On 12/11/25 03:48, Richard Acayan wrote:
> The IMX355 camera sensor is a camera sensor that can be found as the
> front camera in some smartphones, such as the Pixel 3, Pixel 3 XL, Pixel
> 3a, and Pixel 3a XL. It already has a driver, but needs support for
> device tree. Document the IMX355 to support defining it in device tree.
> 
> Signed-off-by: Richard Acayan <mailingradian@gmail.com>
> ---
>   .../bindings/media/i2c/sony,imx355.yaml       | 119 ++++++++++++++++++
>   1 file changed, 119 insertions(+)
>   create mode 100644 Documentation/devicetree/bindings/media/i2c/sony,imx355.yaml
> 
> diff --git a/Documentation/devicetree/bindings/media/i2c/sony,imx355.yaml b/Documentation/devicetree/bindings/media/i2c/sony,imx355.yaml
> new file mode 100644
> index 000000000000..9aa2c7b7ea71
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/i2c/sony,imx355.yaml
> @@ -0,0 +1,119 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/media/i2c/sony,imx355.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Sony IMX355 Sensor
> +
> +maintainers:
> +  - Richard Acayan <mailingradian@gmail.com>
> +
> +description:
> +  The IMX355 sensor is a 3280x2464 image sensor, commonly found as the front
> +  camera in smartphones.
> +
> +allOf:
> +  - $ref: /schemas/media/video-interface-devices.yaml#
> +
> +properties:
> +  compatible:
> +    const: sony,imx355
> +
> +  reg:
> +    maxItems: 1
> +
> +  clocks:
> +    maxItems: 1
> +
> +  clock-names:
> +    const: mclk

clock-names property can be removed, there is just one supply clock.

> +
> +  avdd-supply:
> +    description: Analog power supply.
> +
> +  dvdd-supply:
> +    description: Digital power supply.
> +
> +  dovdd-supply:
> +    description: Interface power supply.
> +
> +  reset-gpios:
> +    maxItems: 1

Please explicitly document that the reset GPIO is active low, and make
the correspondent dts changes.

> +
> +  port:
> +    $ref: /schemas/graph.yaml#/$defs/port-base
> +    additionalProperties: false
> +
> +    properties:
> +      endpoint:
> +        $ref: /schemas/media/video-interfaces.yaml
> +        unevaluatedProperties: false
> +
> +        data-lanes:
> +          items:
> +            - const: 0
> +            - const: 1
> +            - const: 2
> +            - const: 3
> +
> +        required:
> +          - link-frequencies
> +          - data-lanes
> +
> +    required:
> +      - endpoint
> +
> +required:
> +  - compatible
> +  - reg
> +  - clocks
> +  - clock-names
> +  - avdd-supply
> +  - dvdd-supply
> +  - dovdd-supply
> +  - port
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/clock/qcom,camcc-sdm845.h>
> +    #include <dt-bindings/gpio/gpio.h>
> +
> +    i2c {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        camera@1a {
> +            compatible = "sony,imx355";
> +            reg = <0x1a>;
> +
> +            clocks = <&camcc CAM_CC_MCLK2_CLK>;
> +            clock-names = "mclk";
> +
> +            assigned-clocks = <&camcc CAM_CC_MCLK2_CLK>;
> +            assigned-clock-rates = <24000000>;
> +
> +            reset-gpios = <&tlmm 9 GPIO_ACTIVE_HIGH>;
> +
> +            avdd-supply = <&cam_front_ldo>;
> +            dvdd-supply = <&cam_front_ldo>;
> +            dovdd-supply = <&cam_vio_ldo>;
> +
> +            pinctrl-names = "default";
> +            pinctrl-0 = <&cam_front_default>;
> +
> +            rotation = <270>;
> +            orientation = <0>;
> +
> +            port {
> +                cam_front_endpoint: endpoint {
> +                    data-lanes = <0 1 2 3>;
> +                    link-frequencies = /bits/ 64 <360000000>;
> +                    remote-endpoint = <&camss_endpoint1>;
> +                };
> +            };
> +        };
> +    };

-- 
Best wishes,
Vladimir

Re: [PATCH v4 1/5] dt-bindings: media: i2c: Add Sony IMX355

[Richard Acayan]

On Thu, Dec 11, 2025 at 06:02:51AM +0100, Krzysztof Kozlowski wrote:
> On 11/12/2025 02:48, Richard Acayan wrote:
> > +
> > +    properties:
> > +      endpoint:
> > +        $ref: /schemas/media/video-interfaces.yaml
> > +        unevaluatedProperties: false
> > +
> 
> Ah, here, this obviously was not ever tested. Heh, please do not use the
> community as a testing service. Missing props...

Just a rookie mistake, I forgot to test on my smartphone which has the
dtschema validation.

Re: [PATCH v4 5/5] arm64: dts: qcom: sdm670-google-sargo: add imx355 front camera

[Richard Acayan]

On Thu, Dec 11, 2025 at 07:16:30AM +0200, Dmitry Baryshkov wrote:
> On Wed, Dec 10, 2025 at 08:48:46PM -0500, Richard Acayan wrote:
> > The Sony IMX355 is the front camera on the Pixel 3a, mounted in portrait
> > mode. It is connected to CSIPHY1 and CCI I2C1, and uses MCLK2. Add
> > support for it.
> > 
> > Co-developed-by: Robert Mader <robert.mader@collabora.com>
> > Signed-off-by: Robert Mader <robert.mader@collabora.com>
> > Signed-off-by: Richard Acayan <mailingradian@gmail.com>
> > ---
> >  .../boot/dts/qcom/sdm670-google-sargo.dts     | 107 ++++++++++++++++++
> >  1 file changed, 107 insertions(+)
> > 
> > @@ -392,6 +420,64 @@ vreg_bob: bob {
> >  	};
> >  };
> >  
> > +&camss {
> > +	vdda-phy-supply = <&vreg_l1a_1p225>;
> > +	vdda-pll-supply = <&vreg_s6a_0p87>;
> > +
> > +	status = "okay";
> > +
> > +	ports {
> > +		port@1 {
> > +			camss_endpoint1: endpoint {
> > +				clock-lanes = <7>;
> > +				data-lanes = <0 1 2 3>;
> > +				remote-endpoint = <&cam_front_endpoint>;
> > +			};
> > +		};
> > +	};
> 
> This would be much better:
> 
>   &camss_endpoint1: {
>       clock-lanes, data-lanes, remote-endpoint here
>   };

I'm not sure what you mean, there might be some typo.

If this is about using the commonly-defined endpoints, Vladimir broke it
in commit dcf6fb89e6f7 ("media: qcom: camss: remove a check for
unavailable CAMSS endpoint"). If I do this again and go full circle, I'm
afraid this could break a second time before even making it to
linux-next.

Re: [PATCH v4 3/5] arm64: dts: qcom: sdm670: remove camss endpoint nodes

[Bryan O'Donoghue]

On 11/12/2025 01:48, Richard Acayan wrote:
> There is no need to add these by default for all of SDM670. Originally,
> they were added so there could be a label for each port. This is
> unnecessary if the endpoints are all added in a fixup to the camss node.
> 
> This is required since dcf6fb89e6f7 ("media: qcom: camss: remove a check
> for unavailable CAMSS endpoint") was applied, forcing all endpoint nodes
> to be probed, even if they are marked as disabled. According to the body
> of this commit, there is "no valid or sane usecase".
> 
> Suggested-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> Link: https://lore.kernel.org/r/488281f6-5e5d-4864-8220-63e2a0b2d7f2@linaro.org
> Signed-off-by: Richard Acayan <mailingradian@gmail.com>
> ---
>   arch/arm64/boot/dts/qcom/sdm670.dtsi | 12 ------------
>   1 file changed, 12 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sdm670.dtsi b/arch/arm64/boot/dts/qcom/sdm670.dtsi
> index c33f3de779f6..c275089237e4 100644
> --- a/arch/arm64/boot/dts/qcom/sdm670.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sdm670.dtsi
> @@ -1768,26 +1768,14 @@ ports {
> 
>   				port@0 {
>   					reg = <0>;
> -
> -					camss_endpoint0: endpoint {
> -						status = "disabled";
> -					};
>   				};
> 
>   				port@1 {
>   					reg = <1>;
> -
> -					camss_endpoint1: endpoint {
> -						status = "disabled";
> -					};
>   				};
> 
>   				port@2 {
>   					reg = <2>;
> -
> -					camss_endpoint2: endpoint {
> -						status = "disabled";
> -					};
>   				};
>   			};
>   		};
> --
> 2.52.0
> 
> 

Reviewed-by: Bryan O'Donoghue <bod@kernel.org>

---
bod

Re: [PATCH v4 4/5] arm64: dts: qcom: sdm670: add camera mclk pins

[Bryan O'Donoghue]

On 11/12/2025 01:48, Richard Acayan wrote:
> The camera subsystem is added for the SoC common devicetree, but the
> mclk pins should also be common across the SoC. Add the mclk pins for
> the cameras.
> 
> Suggested-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
> Link: https://lore.kernel.org/r/5135823c-f2e4-4873-9e3a-9d190cac0113@oss.qualcomm.com
> Signed-off-by: Richard Acayan <mailingradian@gmail.com>
> ---
>   arch/arm64/boot/dts/qcom/sdm670.dtsi | 21 +++++++++++++++++++++
>   1 file changed, 21 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sdm670.dtsi b/arch/arm64/boot/dts/qcom/sdm670.dtsi
> index c275089237e4..69e84cd8ed27 100644
> --- a/arch/arm64/boot/dts/qcom/sdm670.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sdm670.dtsi
> @@ -1190,6 +1190,27 @@ tlmm: pinctrl@3400000 {
>   			gpio-ranges = <&tlmm 0 0 151>;
>   			wakeup-parent = <&pdc>;
> 
> +			cam_mclk0_default: cam-mclk0-default-state {
> +				pins = "gpio13";
> +				function = "cam_mclk";
> +				drive-strength = <2>;
> +				bias-disable;
> +			};
> +
> +			cam_mclk1_default: cam-mclk1-default-state {
> +				pins = "gpio14";
> +				function = "cam_mclk";
> +				drive-strength = <2>;
> +				bias-disable;
> +			};
> +
> +			cam_mclk2_default: cam-mclk2-default-state {
> +				pins = "gpio15";
> +				function = "cam_mclk";
> +				drive-strength = <2>;
> +				bias-disable;
> +			};
> +
>   			cci0_default: cci0-default-state {
>   				pins = "gpio17", "gpio18";
>   				function = "cci_i2c";
> --
> 2.52.0
> 
> 

Reviewed-by: Bryan O'Donoghue <bod@kernel.org>

---

Re: [PATCH v4 2/5] media: i2c: imx355: Support devicetree and power management

[Krzysztof Kozlowski]

On 11/12/2025 02:48, Richard Acayan wrote:
> +				    imx355->supplies);
> +	if (ret) {
> +		dev_err(dev, "failed to enable regulators: %d\n", ret);
> +		goto error_disable_clocks;
> +	}
> +
> +	usleep_range(5000, 5100);
> +	gpiod_set_value_cansleep(imx355->reset_gpio, 1);

So you just keep device in reset state forever :/

Please see other imx sensor drivers how to fix that.



Best regards,
Krzysztof

Re: [PATCH v4 2/5] media: i2c: imx355: Support devicetree and power management

[Krzysztof Kozlowski]

On 12/12/2025 02:43, Krzysztof Kozlowski wrote:
> On 11/12/2025 02:48, Richard Acayan wrote:
>> +				    imx355->supplies);
>> +	if (ret) {
>> +		dev_err(dev, "failed to enable regulators: %d\n", ret);
>> +		goto error_disable_clocks;
>> +	}
>> +
>> +	usleep_range(5000, 5100);
>> +	gpiod_set_value_cansleep(imx355->reset_gpio, 1);
> 
> So you just keep device in reset state forever :/
> 
> Please see other imx sensor drivers how to fix that.
> 
> 

I already told you this at v1 and you ignored the problem and never
responded.

NAK

Best regards,
Krzysztof

Re: [PATCH v4 5/5] arm64: dts: qcom: sdm670-google-sargo: add imx355 front camera

[Bryan O'Donoghue]

On 12/12/2025 00:41, Richard Acayan wrote:
>> This would be much better:
>>
>>    &camss_endpoint1: {
>>        clock-lanes, data-lanes, remote-endpoint here
>>    };
> I'm not sure what you mean, there might be some typo.

At least for me the endpoint name could be improved see:

Take arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts

Also since you likely have more than one sensor I'd suggest endpoint@0

---
bod

Re: [PATCH v4 2/5] media: i2c: imx355: Support devicetree and power management

[Bryan O'Donoghue]

On 11/12/2025 01:48, Richard Acayan wrote:
> A device tree compatible makes it possible for this driver to be used on
> Open Firmware devices. Initialization of power-managed resources such as
> the reset GPIO and voltage regulators can be specified in the device
> tree and handled by the driver. Add support for this so the Pixel 3a can
> use the driver.
> 
> Signed-off-by: Richard Acayan <mailingradian@gmail.com>
> ---
>   drivers/media/i2c/imx355.c | 118 ++++++++++++++++++++++++++++++++++---
>   1 file changed, 110 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/media/i2c/imx355.c b/drivers/media/i2c/imx355.c
> index 776107efe386..c225bb8959bd 100644
> --- a/drivers/media/i2c/imx355.c
> +++ b/drivers/media/i2c/imx355.c
> @@ -3,9 +3,13 @@
> 
>   #include <linux/acpi.h>
>   #include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/gpio/consumer.h>
>   #include <linux/i2c.h>
>   #include <linux/module.h>
> +#include <linux/of.h>
>   #include <linux/pm_runtime.h>
> +#include <linux/regulator/consumer.h>
>   #include <linux/unaligned.h>
> 
>   #include <media/v4l2-ctrls.h>
> @@ -125,6 +129,15 @@ struct imx355 {
>   	 * Protect access to sensor v4l2 controls.
>   	 */
>   	struct mutex mutex;
> +
> +	struct gpio_desc *reset_gpio;
> +	struct regulator_bulk_data supplies[3];
> +};
> +
> +static const char * const imx355_supply_names[] = {
> +	"avdd",
> +	"dvdd",
> +	"dovdd",
>   };
> 
>   static const struct imx355_reg imx355_global_regs[] = {
> @@ -1515,6 +1528,54 @@ static const struct v4l2_subdev_internal_ops imx355_internal_ops = {
>   	.open = imx355_open,
>   };
> 
> +static int imx355_power_off(struct device *dev)
> +{
> +	struct i2c_client *client = container_of(dev, struct i2c_client, dev);
> +	struct v4l2_subdev *sd = i2c_get_clientdata(client);
> +	struct imx355 *imx355 = to_imx355(sd);
> +
> +	gpiod_set_value_cansleep(imx355->reset_gpio, 0);
> +
> +	regulator_bulk_disable(ARRAY_SIZE(imx355->supplies), imx355->supplies);
> +	clk_disable_unprepare(imx355->clk);
> +
> +	return 0;
> +}
> +
> +static int imx355_power_on(struct device *dev)
> +{
> +	struct i2c_client *client = container_of(dev, struct i2c_client, dev);
> +	struct v4l2_subdev *sd = i2c_get_clientdata(client);
> +	struct imx355 *imx355 = to_imx355(sd);
> +	int ret;
> +
> +	ret = clk_prepare_enable(imx355->clk);
> +	if (ret) {
> +		dev_err(dev, "failed to enable clocks: %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = regulator_bulk_enable(ARRAY_SIZE(imx355->supplies),
> +				    imx355->supplies);
> +	if (ret) {
> +		dev_err(dev, "failed to enable regulators: %d\n", ret);
> +		goto error_disable_clocks;
> +	}
> +
> +	usleep_range(5000, 5100);
> +	gpiod_set_value_cansleep(imx355->reset_gpio, 1);
> +	usleep_range(8000, 8100);
> +

I'd suggest taking the chip through an explicit reset sequence - 
including appropriate delays here.

The code as-is assumes the reset line is logic 0, which it may not be.

Better to whack reset, give some reasonable amount of time for reset to 
complete, unwhack - delay and then off you go.

That way you've put the chip into a known state and probably you don't 
have the documentation but, if you had the documentation you'd see the 
chip mandates such a start-up sequence anyway.

---
bod

Re: [PATCH v4 5/5] arm64: dts: qcom: sdm670-google-sargo: add imx355 front camera

[Richard Acayan]

On Fri, Dec 12, 2025 at 01:45:52AM +0000, Bryan O'Donoghue wrote:
> On 12/12/2025 00:41, Richard Acayan wrote:
> >> This would be much better:
> >>
> >>    &camss_endpoint1: {
> >>        clock-lanes, data-lanes, remote-endpoint here
> >>    };
> > I'm not sure what you mean, there might be some typo.
> 
> At least for me the endpoint name could be improved see:
> 
> Take arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts
> 
> Also since you likely have more than one sensor I'd suggest endpoint@0

Uh... the front and rear camera here are on different CSI ports.

Something like:

	&camss {
		ports {
			port@0 {
				endpoint {
					... // rear camera endpoint goes here
				};
			};

			port@1 {
				endpoint {
					... // front camera endpoint goes here
				};
			};
		};
	};

Would they both be endpoint@0? Or would the front camera be endpoint@1?

Re: [PATCH v4 5/5] arm64: dts: qcom: sdm670-google-sargo: add imx355 front camera

[Bryan O'Donoghue]

On 12/12/2025 02:19, Richard Acayan wrote:
> On Fri, Dec 12, 2025 at 01:45:52AM +0000, Bryan O'Donoghue wrote:
>> On 12/12/2025 00:41, Richard Acayan wrote:
>>>> This would be much better:
>>>>
>>>>     &camss_endpoint1: {
>>>>         clock-lanes, data-lanes, remote-endpoint here
>>>>     };
>>> I'm not sure what you mean, there might be some typo.
>>
>> At least for me the endpoint name could be improved see:
>>
>> Take arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts
>>
>> Also since you likely have more than one sensor I'd suggest endpoint@0
> 
> Uh... the front and rear camera here are on different CSI ports.
> 
> Something like:
> 
> 	&camss {
> 		ports {
> 			port@0 {
> 				endpoint {
> 					... // rear camera endpoint goes here
> 				};
> 			};
> 
> 			port@1 {
> 				endpoint {
> 					... // front camera endpoint goes here
> 				};
> 			};
> 		};
> 	};
> 
> Would they both be endpoint@0? Or would the front camera be endpoint@1?

Yeah no fair point you're right, that @0 is useless.

Ignore.

---
bod

Re: [PATCH v4 5/5] arm64: dts: qcom: sdm670-google-sargo: add imx355 front camera

[Dmitry Baryshkov]

On Thu, Dec 11, 2025 at 07:41:37PM -0500, Richard Acayan wrote:
> On Thu, Dec 11, 2025 at 07:16:30AM +0200, Dmitry Baryshkov wrote:
> > On Wed, Dec 10, 2025 at 08:48:46PM -0500, Richard Acayan wrote:
> > > The Sony IMX355 is the front camera on the Pixel 3a, mounted in portrait
> > > mode. It is connected to CSIPHY1 and CCI I2C1, and uses MCLK2. Add
> > > support for it.
> > > 
> > > Co-developed-by: Robert Mader <robert.mader@collabora.com>
> > > Signed-off-by: Robert Mader <robert.mader@collabora.com>
> > > Signed-off-by: Richard Acayan <mailingradian@gmail.com>
> > > ---
> > >  .../boot/dts/qcom/sdm670-google-sargo.dts     | 107 ++++++++++++++++++
> > >  1 file changed, 107 insertions(+)
> > > 
> > > @@ -392,6 +420,64 @@ vreg_bob: bob {
> > >  	};
> > >  };
> > >  
> > > +&camss {
> > > +	vdda-phy-supply = <&vreg_l1a_1p225>;
> > > +	vdda-pll-supply = <&vreg_s6a_0p87>;
> > > +
> > > +	status = "okay";
> > > +
> > > +	ports {
> > > +		port@1 {
> > > +			camss_endpoint1: endpoint {
> > > +				clock-lanes = <7>;
> > > +				data-lanes = <0 1 2 3>;
> > > +				remote-endpoint = <&cam_front_endpoint>;
> > > +			};
> > > +		};
> > > +	};
> > 
> > This would be much better:
> > 
> >   &camss_endpoint1: {
> >       clock-lanes, data-lanes, remote-endpoint here
> >   };
> 
> I'm not sure what you mean, there might be some typo.

My point is that you are duplicating `ports { port@1 {.... }; };` from
the base DTSI here.  We usually try to avoid this kind of path
duplication. If you can't have an empty endpoint in the base DTSI, I
suggest adding necessary labels to port@N nodes and then extending those
nodes in the board DTSI.

> If this is about using the commonly-defined endpoints, Vladimir broke it
> in commit dcf6fb89e6f7 ("media: qcom: camss: remove a check for
> unavailable CAMSS endpoint"). If I do this again and go full circle, I'm
> afraid this could break a second time before even making it to
> linux-next.

-- 
With best wishes
Dmitry

Re: [PATCH v4 5/5] arm64: dts: qcom: sdm670-google-sargo: add imx355 front camera

[David Heidelberg]

On 11/12/2025 02:48, Richard Acayan wrote:
> The Sony IMX355 is the front camera on the Pixel 3a, mounted in portrait
> mode. It is connected to CSIPHY1 and CCI I2C1, and uses MCLK2. Add
> support for it.
> 
> Co-developed-by: Robert Mader <robert.mader@collabora.com>
> Signed-off-by: Robert Mader <robert.mader@collabora.com>
> Signed-off-by: Richard Acayan <mailingradian@gmail.com>
> ---
>   .../boot/dts/qcom/sdm670-google-sargo.dts     | 107 ++++++++++++++++++
>   1 file changed, 107 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sdm670-google-sargo.dts b/arch/arm64/boot/dts/qcom/sdm670-google-sargo.dts
> index d01422844fbf..ede0ad7ded23 100644
> --- a/arch/arm64/boot/dts/qcom/sdm670-google-sargo.dts
> +++ b/arch/arm64/boot/dts/qcom/sdm670-google-sargo.dts
> @@ -172,6 +172,34 @@ vreg_s2b_1p05: vreg-s2b-regulator {
>   		regulator-min-microvolt = <1050000>;
>   		regulator-max-microvolt = <1050000>;
>   	};
> +
> +	cam_front_ldo: cam-front-ldo-regulator {
> +		compatible = "regulator-fixed";
> +		regulator-name = "cam_front_ldo";
> +		regulator-min-microvolt = <1352000>;
> +		regulator-max-microvolt = <1352000>;
> +		regulator-enable-ramp-delay = <135>;
> +
> +		gpios = <&pm660l_gpios 4 GPIO_ACTIVE_HIGH>;
> +		enable-active-high;
> +
> +		pinctrl-0 = <&cam_front_ldo_pin>;
> +		pinctrl-names = "default";
> +	};
> +
> +	cam_vio_ldo: cam-vio-ldo-regulator {
> +		compatible = "regulator-fixed";
> +		regulator-name = "cam_vio_ldo";
> +		regulator-min-microvolt = <1800000>;
> +		regulator-max-microvolt = <1800000>;
> +		regulator-enable-ramp-delay = <233>;
> +
> +		gpios = <&pm660_gpios 13 GPIO_ACTIVE_HIGH>;
> +		enable-active-high;
> +
> +		pinctrl-0 = <&cam_vio_pin>;
> +		pinctrl-names = "default";
> +	};
>   };
>   
>   &apps_rsc {
> @@ -392,6 +420,64 @@ vreg_bob: bob {
>   	};
>   };
>   
> +&camss {
> +	vdda-phy-supply = <&vreg_l1a_1p225>;
> +	vdda-pll-supply = <&vreg_s6a_0p87>;
The vdda-pll-supply is already named wrongly, as 0p87 != 1.352

vreg_s6a_0p87: smps6 {
                        regulator-min-microvolt = <1224000>;
                        regulator-max-microvolt = <1352000>;



vdda-phy-supply is on sdm845 (0.75 - 0.8 V) and sdm670 different.
sdm845: 0.8 V (should be 0.75 - 0.8 V) // OnePlus 6
sc7280: 0.72 V - 1.05 V // Fairphone 5
sdm670: 1.2 V

This may be correct, thou, but still feels like pretty huge diff.

Maybe someone from QCOM can confirm?

David

[...]

Re: [PATCH v4 4/5] arm64: dts: qcom: sdm670: add camera mclk pins

[David Heidelberg]

On 11/12/2025 02:48, Richard Acayan wrote:
> The camera subsystem is added for the SoC common devicetree, but the
> mclk pins should also be common across the SoC. Add the mclk pins for
> the cameras.
> 
> Suggested-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
> Link: https://lore.kernel.org/r/5135823c-f2e4-4873-9e3a-9d190cac0113@oss.qualcomm.com
> Signed-off-by: Richard Acayan <mailingradian@gmail.com>
> ---
>   arch/arm64/boot/dts/qcom/sdm670.dtsi | 21 +++++++++++++++++++++
>   1 file changed, 21 insertions(+)
> 

Reviewed-by: David Heidelberg <david@ixit.cz>

-- 
David Heidelberg

Re: [PATCH v4 1/5] dt-bindings: media: i2c: Add Sony IMX355

[Richard Acayan]

On Thu, Dec 11, 2025 at 01:02:49PM +0200, Vladimir Zapolskiy wrote:
> On 12/11/25 03:48, Richard Acayan wrote:
<snip>
> > +  reset-gpios:
> > +    maxItems: 1
> 
> Please explicitly document that the reset GPIO is active low, and make
> the correspondent dts changes.

On my local copy, the DTS already specifies active low, and I haven't
changed it since sending. I'll just change the dt-bindings.

Re: [PATCH v4 1/5] dt-bindings: media: i2c: Add Sony IMX355

[Richard Acayan]

On Sat, Dec 13, 2025 at 04:50:07PM -0500, Richard Acayan wrote:
> On Thu, Dec 11, 2025 at 01:02:49PM +0200, Vladimir Zapolskiy wrote:
> > On 12/11/25 03:48, Richard Acayan wrote:
> <snip>
> > > +  reset-gpios:
> > > +    maxItems: 1
> > 
> > Please explicitly document that the reset GPIO is active low, and make
> > the correspondent dts changes.
> 
> On my local copy, the DTS already specifies active low, and I haven't
> changed it since sending. I'll just change the dt-bindings.

Oh nevermind. It is still active high.

Re: [PATCH v4 1/5] dt-bindings: media: i2c: Add Sony IMX355

[Richard Acayan]

On Thu, Dec 11, 2025 at 06:05:28AM +0100, Krzysztof Kozlowski wrote:
> On 11/12/2025 02:48, Richard Acayan wrote:
> co> +
> > +  port:
> > +    $ref: /schemas/graph.yaml#/$defs/port-base
> > +    additionalProperties: false
> > +
> > +    properties:
> > +      endpoint:
> > +        $ref: /schemas/media/video-interfaces.yaml
> > +        unevaluatedProperties: false
> > +
> > +        data-lanes:
> > +          items:
> > +            - const: 0
> > +            - const: 1
> > +            - const: 2
> > +            - const: 3
> 
> Obviously untested code but another thought: why do you need data-lanes
> if they are fixed? They are implied by the compatible.

Oh, I just added it when mirroring the other endpoint. It can be removed
from dt-bindings and DTS.

Re: [PATCH v4 5/5] arm64: dts: qcom: sdm670-google-sargo: add imx355 front camera

[Konrad Dybcio]

On 12/12/25 8:22 PM, Dmitry Baryshkov wrote:
> On Thu, Dec 11, 2025 at 07:41:37PM -0500, Richard Acayan wrote:
>> On Thu, Dec 11, 2025 at 07:16:30AM +0200, Dmitry Baryshkov wrote:
>>> On Wed, Dec 10, 2025 at 08:48:46PM -0500, Richard Acayan wrote:
>>>> The Sony IMX355 is the front camera on the Pixel 3a, mounted in portrait
>>>> mode. It is connected to CSIPHY1 and CCI I2C1, and uses MCLK2. Add
>>>> support for it.
>>>>
>>>> Co-developed-by: Robert Mader <robert.mader@collabora.com>
>>>> Signed-off-by: Robert Mader <robert.mader@collabora.com>
>>>> Signed-off-by: Richard Acayan <mailingradian@gmail.com>
>>>> ---
>>>>  .../boot/dts/qcom/sdm670-google-sargo.dts     | 107 ++++++++++++++++++
>>>>  1 file changed, 107 insertions(+)
>>>>
>>>> @@ -392,6 +420,64 @@ vreg_bob: bob {
>>>>  	};
>>>>  };
>>>>  
>>>> +&camss {
>>>> +	vdda-phy-supply = <&vreg_l1a_1p225>;
>>>> +	vdda-pll-supply = <&vreg_s6a_0p87>;
>>>> +
>>>> +	status = "okay";
>>>> +
>>>> +	ports {
>>>> +		port@1 {
>>>> +			camss_endpoint1: endpoint {
>>>> +				clock-lanes = <7>;
>>>> +				data-lanes = <0 1 2 3>;
>>>> +				remote-endpoint = <&cam_front_endpoint>;
>>>> +			};
>>>> +		};
>>>> +	};
>>>
>>> This would be much better:
>>>
>>>   &camss_endpoint1: {
>>>       clock-lanes, data-lanes, remote-endpoint here
>>>   };
>>
>> I'm not sure what you mean, there might be some typo.
> 
> My point is that you are duplicating `ports { port@1 {.... }; };` from
> the base DTSI here.  We usually try to avoid this kind of path
> duplication. If you can't have an empty endpoint in the base DTSI, I
> suggest adding necessary labels to port@N nodes and then extending those
> nodes in the board DTSI.
> 
>> If this is about using the commonly-defined endpoints, Vladimir broke it
>> in commit dcf6fb89e6f7 ("media: qcom: camss: remove a check for
>> unavailable CAMSS endpoint"). If I do this again and go full circle, I'm
>> afraid this could break a second time before even making it to
>> linux-next.

Quite frankly I don't think that commit was valid, given it's conceivable
that an endpoint could be unconnected..

Konrad

Re: [PATCH v4 5/5] arm64: dts: qcom: sdm670-google-sargo: add imx355 front camera

[Vladimir Zapolskiy]

On 12/16/25 15:56, Konrad Dybcio wrote:
> On 12/12/25 8:22 PM, Dmitry Baryshkov wrote:
>> On Thu, Dec 11, 2025 at 07:41:37PM -0500, Richard Acayan wrote:
>>> On Thu, Dec 11, 2025 at 07:16:30AM +0200, Dmitry Baryshkov wrote:
>>>> On Wed, Dec 10, 2025 at 08:48:46PM -0500, Richard Acayan wrote:
>>>>> The Sony IMX355 is the front camera on the Pixel 3a, mounted in portrait
>>>>> mode. It is connected to CSIPHY1 and CCI I2C1, and uses MCLK2. Add
>>>>> support for it.
>>>>>
>>>>> Co-developed-by: Robert Mader <robert.mader@collabora.com>
>>>>> Signed-off-by: Robert Mader <robert.mader@collabora.com>
>>>>> Signed-off-by: Richard Acayan <mailingradian@gmail.com>
>>>>> ---
>>>>>   .../boot/dts/qcom/sdm670-google-sargo.dts     | 107 ++++++++++++++++++
>>>>>   1 file changed, 107 insertions(+)
>>>>>
>>>>> @@ -392,6 +420,64 @@ vreg_bob: bob {
>>>>>   	};
>>>>>   };
>>>>>   
>>>>> +&camss {
>>>>> +	vdda-phy-supply = <&vreg_l1a_1p225>;
>>>>> +	vdda-pll-supply = <&vreg_s6a_0p87>;
>>>>> +
>>>>> +	status = "okay";
>>>>> +
>>>>> +	ports {
>>>>> +		port@1 {
>>>>> +			camss_endpoint1: endpoint {
>>>>> +				clock-lanes = <7>;
>>>>> +				data-lanes = <0 1 2 3>;
>>>>> +				remote-endpoint = <&cam_front_endpoint>;
>>>>> +			};
>>>>> +		};
>>>>> +	};
>>>>
>>>> This would be much better:
>>>>
>>>>    &camss_endpoint1: {
>>>>        clock-lanes, data-lanes, remote-endpoint here
>>>>    };
>>>
>>> I'm not sure what you mean, there might be some typo.
>>
>> My point is that you are duplicating `ports { port@1 {.... }; };` from
>> the base DTSI here.  We usually try to avoid this kind of path
>> duplication. If you can't have an empty endpoint in the base DTSI, I
>> suggest adding necessary labels to port@N nodes and then extending those
>> nodes in the board DTSI.
>>
>>> If this is about using the commonly-defined endpoints, Vladimir broke it
>>> in commit dcf6fb89e6f7 ("media: qcom: camss: remove a check for
>>> unavailable CAMSS endpoint"). If I do this again and go full circle, I'm
>>> afraid this could break a second time before even making it to
>>> linux-next.
> 
> Quite frankly I don't think that commit was valid, given it's conceivable
> that an endpoint could be unconnected..
> 

Endpoint is not a device, status property is the property of devices and
not a property of anything else as the Devicetree Specification v0.4 and
earlier ones define. Dangling endpoints are fine, there is no need to
add another property to determine, if an endpoint is connected or not.

There should be no status properties inside endpoint device tree nodes.

One day I've reviewed v8 of "arm64: dts: qcom: sdm670: add camss and cci",
which didn't contain these properties, and asked to remove the labels, but
v9-v10 appeared with these properties introduced and with my RB, I didn't
notice in time that my RB should be revoked in v9.

-- 
Best wishes,
Vladimir

Re: [PATCH v4 5/5] arm64: dts: qcom: sdm670-google-sargo: add imx355 front camera

[Konrad Dybcio]

On 12/16/25 3:31 PM, Vladimir Zapolskiy wrote:
> On 12/16/25 15:56, Konrad Dybcio wrote:
>> On 12/12/25 8:22 PM, Dmitry Baryshkov wrote:
>>> On Thu, Dec 11, 2025 at 07:41:37PM -0500, Richard Acayan wrote:
>>>> On Thu, Dec 11, 2025 at 07:16:30AM +0200, Dmitry Baryshkov wrote:
>>>>> On Wed, Dec 10, 2025 at 08:48:46PM -0500, Richard Acayan wrote:
>>>>>> The Sony IMX355 is the front camera on the Pixel 3a, mounted in portrait
>>>>>> mode. It is connected to CSIPHY1 and CCI I2C1, and uses MCLK2. Add
>>>>>> support for it.
>>>>>>
>>>>>> Co-developed-by: Robert Mader <robert.mader@collabora.com>
>>>>>> Signed-off-by: Robert Mader <robert.mader@collabora.com>
>>>>>> Signed-off-by: Richard Acayan <mailingradian@gmail.com>
>>>>>> ---
>>>>>>   .../boot/dts/qcom/sdm670-google-sargo.dts     | 107 ++++++++++++++++++
>>>>>>   1 file changed, 107 insertions(+)
>>>>>>
>>>>>> @@ -392,6 +420,64 @@ vreg_bob: bob {
>>>>>>       };
>>>>>>   };
>>>>>>   +&camss {
>>>>>> +    vdda-phy-supply = <&vreg_l1a_1p225>;
>>>>>> +    vdda-pll-supply = <&vreg_s6a_0p87>;
>>>>>> +
>>>>>> +    status = "okay";
>>>>>> +
>>>>>> +    ports {
>>>>>> +        port@1 {
>>>>>> +            camss_endpoint1: endpoint {
>>>>>> +                clock-lanes = <7>;
>>>>>> +                data-lanes = <0 1 2 3>;
>>>>>> +                remote-endpoint = <&cam_front_endpoint>;
>>>>>> +            };
>>>>>> +        };
>>>>>> +    };
>>>>>
>>>>> This would be much better:
>>>>>
>>>>>    &camss_endpoint1: {
>>>>>        clock-lanes, data-lanes, remote-endpoint here
>>>>>    };
>>>>
>>>> I'm not sure what you mean, there might be some typo.
>>>
>>> My point is that you are duplicating `ports { port@1 {.... }; };` from
>>> the base DTSI here.  We usually try to avoid this kind of path
>>> duplication. If you can't have an empty endpoint in the base DTSI, I
>>> suggest adding necessary labels to port@N nodes and then extending those
>>> nodes in the board DTSI.
>>>
>>>> If this is about using the commonly-defined endpoints, Vladimir broke it
>>>> in commit dcf6fb89e6f7 ("media: qcom: camss: remove a check for
>>>> unavailable CAMSS endpoint"). If I do this again and go full circle, I'm
>>>> afraid this could break a second time before even making it to
>>>> linux-next.
>>
>> Quite frankly I don't think that commit was valid, given it's conceivable
>> that an endpoint could be unconnected..
>>
> 
> Endpoint is not a device, status property is the property of devices and
> not a property of anything else as the Devicetree Specification v0.4 and
> earlier ones define. Dangling endpoints are fine, there is no need to
> add another property to determine, if an endpoint is connected or not.
> 
> There should be no status properties inside endpoint device tree nodes.

The spec doesn't actually define what a "device" is. Funnily enough, it refers
to "endpoint" as a device:

2.2.2 Generic Names Recommendation
The name of a node should be somewhat generic, reflecting the function of the
_device_ and not its precise programming model. If appropriate, the name should
be one of the following choices:

[...]

* endpoint


Plus an OF node is opaque in its purpose.. The top node, a firmware node, a
node representing a physical IP block and a config.ini-style blurb are all
"device nodes"


But coming back to the real world, the ports/endpoints represent the physical
connections to CAMSS and it makes sense to have them defined in one place,
especially since there's a predictable number of them that should not be left
up to each board to define.. That quite obviously implies that not all boards
are going to utilize all interfaces and the commit of yours that was mentioned
above seems to only be valid on the basis of semantics, which I above mentioned
are not *really* a valid point..

Konrad

Re: [PATCH v4 5/5] arm64: dts: qcom: sdm670-google-sargo: add imx355 front camera

[Vladimir Zapolskiy]

On 12/16/25 16:41, Konrad Dybcio wrote:
> On 12/16/25 3:31 PM, Vladimir Zapolskiy wrote:
>> On 12/16/25 15:56, Konrad Dybcio wrote:
>>> On 12/12/25 8:22 PM, Dmitry Baryshkov wrote:
>>>> On Thu, Dec 11, 2025 at 07:41:37PM -0500, Richard Acayan wrote:
>>>>> On Thu, Dec 11, 2025 at 07:16:30AM +0200, Dmitry Baryshkov wrote:
>>>>>> On Wed, Dec 10, 2025 at 08:48:46PM -0500, Richard Acayan wrote:
>>>>>>> The Sony IMX355 is the front camera on the Pixel 3a, mounted in portrait
>>>>>>> mode. It is connected to CSIPHY1 and CCI I2C1, and uses MCLK2. Add
>>>>>>> support for it.
>>>>>>>
>>>>>>> Co-developed-by: Robert Mader <robert.mader@collabora.com>
>>>>>>> Signed-off-by: Robert Mader <robert.mader@collabora.com>
>>>>>>> Signed-off-by: Richard Acayan <mailingradian@gmail.com>
>>>>>>> ---
>>>>>>>    .../boot/dts/qcom/sdm670-google-sargo.dts     | 107 ++++++++++++++++++
>>>>>>>    1 file changed, 107 insertions(+)
>>>>>>>
>>>>>>> @@ -392,6 +420,64 @@ vreg_bob: bob {
>>>>>>>        };
>>>>>>>    };
>>>>>>>    +&camss {
>>>>>>> +    vdda-phy-supply = <&vreg_l1a_1p225>;
>>>>>>> +    vdda-pll-supply = <&vreg_s6a_0p87>;
>>>>>>> +
>>>>>>> +    status = "okay";
>>>>>>> +
>>>>>>> +    ports {
>>>>>>> +        port@1 {
>>>>>>> +            camss_endpoint1: endpoint {
>>>>>>> +                clock-lanes = <7>;
>>>>>>> +                data-lanes = <0 1 2 3>;
>>>>>>> +                remote-endpoint = <&cam_front_endpoint>;
>>>>>>> +            };
>>>>>>> +        };
>>>>>>> +    };
>>>>>>
>>>>>> This would be much better:
>>>>>>
>>>>>>     &camss_endpoint1: {
>>>>>>         clock-lanes, data-lanes, remote-endpoint here
>>>>>>     };
>>>>>
>>>>> I'm not sure what you mean, there might be some typo.
>>>>
>>>> My point is that you are duplicating `ports { port@1 {.... }; };` from
>>>> the base DTSI here.  We usually try to avoid this kind of path
>>>> duplication. If you can't have an empty endpoint in the base DTSI, I
>>>> suggest adding necessary labels to port@N nodes and then extending those
>>>> nodes in the board DTSI.
>>>>
>>>>> If this is about using the commonly-defined endpoints, Vladimir broke it
>>>>> in commit dcf6fb89e6f7 ("media: qcom: camss: remove a check for
>>>>> unavailable CAMSS endpoint"). If I do this again and go full circle, I'm
>>>>> afraid this could break a second time before even making it to
>>>>> linux-next.
>>>
>>> Quite frankly I don't think that commit was valid, given it's conceivable
>>> that an endpoint could be unconnected..
>>>
>>
>> Endpoint is not a device, status property is the property of devices and
>> not a property of anything else as the Devicetree Specification v0.4 and
>> earlier ones define. Dangling endpoints are fine, there is no need to
>> add another property to determine, if an endpoint is connected or not.
>>
>> There should be no status properties inside endpoint device tree nodes.
> 
> The spec doesn't actually define what a "device" is. Funnily enough, it refers
> to "endpoint" as a device:
> 
> 2.2.2 Generic Names Recommendation
> The name of a node should be somewhat generic, reflecting the function of the
> _device_ and not its precise programming model. If appropriate, the name should
> be one of the following choices:
> 
> [...]
> 
> * endpoint
> 
> 
> Plus an OF node is opaque in its purpose.. The top node, a firmware node, a
> node representing a physical IP block and a config.ini-style blurb are all
> "device nodes"

It sounds like somebody of DT maintainers should clarify the matter and update
the spec to be less ambiguous, if it happens that "device" term is undefined.

> 
> But coming back to the real world, the ports/endpoints represent the physical
> connections to CAMSS and it makes sense to have them defined in one place,
> especially since there's a predictable number of them that should not be left
> up to each board to define.. That quite obviously implies that not all boards
> are going to utilize all interfaces and the commit of yours that was mentioned
> above seems to only be valid on the basis of semantics, which I above mentioned
> are not *really* a valid point..

For whatever reason CAMSS on SDM670 is very special, because IIRC there is no
other platform with preset status poperties of endpoints. This exclusive SDM670
specifics shall be analysed and eliminated, since it hasn't been done during
patch review time, it's time to do it right now then.

-- 
Best wishes,
Vladimir

Re: [PATCH v4 5/5] arm64: dts: qcom: sdm670-google-sargo: add imx355 front camera

[Richard Acayan]

On Tue, Dec 16, 2025 at 05:23:53PM +0200, Vladimir Zapolskiy wrote:
> On 12/16/25 16:41, Konrad Dybcio wrote:
> > On 12/16/25 3:31 PM, Vladimir Zapolskiy wrote:
> > > On 12/16/25 15:56, Konrad Dybcio wrote:
> > > > On 12/12/25 8:22 PM, Dmitry Baryshkov wrote:
> > > > > On Thu, Dec 11, 2025 at 07:41:37PM -0500, Richard Acayan wrote:
> > > > > > On Thu, Dec 11, 2025 at 07:16:30AM +0200, Dmitry Baryshkov wrote:
> > > > > > > On Wed, Dec 10, 2025 at 08:48:46PM -0500, Richard Acayan wrote:
> > > > > > > > The Sony IMX355 is the front camera on the Pixel 3a, mounted in portrait
> > > > > > > > mode. It is connected to CSIPHY1 and CCI I2C1, and uses MCLK2. Add
> > > > > > > > support for it.
> > > > > > > > 
> > > > > > > > Co-developed-by: Robert Mader <robert.mader@collabora.com>
> > > > > > > > Signed-off-by: Robert Mader <robert.mader@collabora.com>
> > > > > > > > Signed-off-by: Richard Acayan <mailingradian@gmail.com>
> > > > > > > > ---
> > > > > > > >    .../boot/dts/qcom/sdm670-google-sargo.dts     | 107 ++++++++++++++++++
> > > > > > > >    1 file changed, 107 insertions(+)
> > > > > > > > 
> > > > > > > > @@ -392,6 +420,64 @@ vreg_bob: bob {
> > > > > > > >        };
> > > > > > > >    };
> > > > > > > >    +&camss {
> > > > > > > > +    vdda-phy-supply = <&vreg_l1a_1p225>;
> > > > > > > > +    vdda-pll-supply = <&vreg_s6a_0p87>;
> > > > > > > > +
> > > > > > > > +    status = "okay";
> > > > > > > > +
> > > > > > > > +    ports {
> > > > > > > > +        port@1 {
> > > > > > > > +            camss_endpoint1: endpoint {
> > > > > > > > +                clock-lanes = <7>;
> > > > > > > > +                data-lanes = <0 1 2 3>;
> > > > > > > > +                remote-endpoint = <&cam_front_endpoint>;
> > > > > > > > +            };
> > > > > > > > +        };
> > > > > > > > +    };
> > > > > > > 
> > > > > > > This would be much better:
> > > > > > > 
> > > > > > >     &camss_endpoint1: {
> > > > > > >         clock-lanes, data-lanes, remote-endpoint here
> > > > > > >     };
> > > > > > 
> > > > > > I'm not sure what you mean, there might be some typo.
> > > > > 
> > > > > My point is that you are duplicating `ports { port@1 {.... }; };` from
> > > > > the base DTSI here.  We usually try to avoid this kind of path
> > > > > duplication. If you can't have an empty endpoint in the base DTSI, I
> > > > > suggest adding necessary labels to port@N nodes and then extending those
> > > > > nodes in the board DTSI.
> > > > > 
> > > > > > If this is about using the commonly-defined endpoints, Vladimir broke it
> > > > > > in commit dcf6fb89e6f7 ("media: qcom: camss: remove a check for
> > > > > > unavailable CAMSS endpoint"). If I do this again and go full circle, I'm
> > > > > > afraid this could break a second time before even making it to
> > > > > > linux-next.
> > > > 
> > > > Quite frankly I don't think that commit was valid, given it's conceivable
> > > > that an endpoint could be unconnected..
> > > > 
> > > 
> > > Endpoint is not a device, status property is the property of devices and
> > > not a property of anything else as the Devicetree Specification v0.4 and
> > > earlier ones define. Dangling endpoints are fine, there is no need to
> > > add another property to determine, if an endpoint is connected or not.
> > > 
> > > There should be no status properties inside endpoint device tree nodes.
> > 
> > The spec doesn't actually define what a "device" is. Funnily enough, it refers
> > to "endpoint" as a device:
> > 
> > 2.2.2 Generic Names Recommendation
> > The name of a node should be somewhat generic, reflecting the function of the
> > _device_ and not its precise programming model. If appropriate, the name should
> > be one of the following choices:
> > 
> > [...]
> > 
> > * endpoint
> > 
> > 
> > Plus an OF node is opaque in its purpose.. The top node, a firmware node, a
> > node representing a physical IP block and a config.ini-style blurb are all
> > "device nodes"
> 
> It sounds like somebody of DT maintainers should clarify the matter and update
> the spec to be less ambiguous, if it happens that "device" term is undefined.
> 
> > 
> > But coming back to the real world, the ports/endpoints represent the physical
> > connections to CAMSS and it makes sense to have them defined in one place,
> > especially since there's a predictable number of them that should not be left
> > up to each board to define.. That quite obviously implies that not all boards
> > are going to utilize all interfaces and the commit of yours that was mentioned
> > above seems to only be valid on the basis of semantics, which I above mentioned
> > are not *really* a valid point..
> 
> For whatever reason CAMSS on SDM670 is very special, because IIRC there is no
> other platform with preset status poperties of endpoints. This exclusive SDM670
> specifics shall be analysed and eliminated, since it hasn't been done during
> patch review time, it's time to do it right now then.

An SoC-common endpoint node is how panels are hooked up to DSI. This
seems to be the case for SDM670, SDM845, SM8[123456]50, etc.
It's not special or unpopular for DSI, but CAMSS seems to be the
subsystem where an endpoint node pre-defined by the SoC is only in
SDM670.

At least from the context retained in this reply and my memory, the most
compelling argument I've seen from you so far about removing the
pre-defined endpoints is (in multiple steps):

	1. Suggesting common endpoint nodes instead of common port nodes
	https://lore.kernel.org/all/e9dc1a6f-156b-40aa-9209-2010464d54ed@linaro.org/

	2. Breaking common endpoint nodes in CAMSS
	https://lore.kernel.org/all/20250903002255.346026-4-vladimir.zapolskiy@linaro.org/

This is why I try to remove the common endpoint nodes now.

Re: [PATCH v4 5/5] arm64: dts: qcom: sdm670-google-sargo: add imx355 front camera

[Vladimir Zapolskiy]

On 12/17/25 03:41, Richard Acayan wrote:
> On Tue, Dec 16, 2025 at 05:23:53PM +0200, Vladimir Zapolskiy wrote:
>> On 12/16/25 16:41, Konrad Dybcio wrote:
>>> On 12/16/25 3:31 PM, Vladimir Zapolskiy wrote:
>>>> On 12/16/25 15:56, Konrad Dybcio wrote:
>>>>> On 12/12/25 8:22 PM, Dmitry Baryshkov wrote:
>>>>>> On Thu, Dec 11, 2025 at 07:41:37PM -0500, Richard Acayan wrote:
>>>>>>> On Thu, Dec 11, 2025 at 07:16:30AM +0200, Dmitry Baryshkov wrote:
>>>>>>>> On Wed, Dec 10, 2025 at 08:48:46PM -0500, Richard Acayan wrote:
>>>>>>>>> The Sony IMX355 is the front camera on the Pixel 3a, mounted in portrait
>>>>>>>>> mode. It is connected to CSIPHY1 and CCI I2C1, and uses MCLK2. Add
>>>>>>>>> support for it.
>>>>>>>>>
>>>>>>>>> Co-developed-by: Robert Mader <robert.mader@collabora.com>
>>>>>>>>> Signed-off-by: Robert Mader <robert.mader@collabora.com>
>>>>>>>>> Signed-off-by: Richard Acayan <mailingradian@gmail.com>
>>>>>>>>> ---
>>>>>>>>>     .../boot/dts/qcom/sdm670-google-sargo.dts     | 107 ++++++++++++++++++
>>>>>>>>>     1 file changed, 107 insertions(+)
>>>>>>>>>
>>>>>>>>> @@ -392,6 +420,64 @@ vreg_bob: bob {
>>>>>>>>>         };
>>>>>>>>>     };
>>>>>>>>>     +&camss {
>>>>>>>>> +    vdda-phy-supply = <&vreg_l1a_1p225>;
>>>>>>>>> +    vdda-pll-supply = <&vreg_s6a_0p87>;
>>>>>>>>> +
>>>>>>>>> +    status = "okay";
>>>>>>>>> +
>>>>>>>>> +    ports {
>>>>>>>>> +        port@1 {
>>>>>>>>> +            camss_endpoint1: endpoint {
>>>>>>>>> +                clock-lanes = <7>;
>>>>>>>>> +                data-lanes = <0 1 2 3>;
>>>>>>>>> +                remote-endpoint = <&cam_front_endpoint>;
>>>>>>>>> +            };
>>>>>>>>> +        };
>>>>>>>>> +    };
>>>>>>>>
>>>>>>>> This would be much better:
>>>>>>>>
>>>>>>>>      &camss_endpoint1: {
>>>>>>>>          clock-lanes, data-lanes, remote-endpoint here
>>>>>>>>      };
>>>>>>>
>>>>>>> I'm not sure what you mean, there might be some typo.
>>>>>>
>>>>>> My point is that you are duplicating `ports { port@1 {.... }; };` from
>>>>>> the base DTSI here.  We usually try to avoid this kind of path
>>>>>> duplication. If you can't have an empty endpoint in the base DTSI, I
>>>>>> suggest adding necessary labels to port@N nodes and then extending those
>>>>>> nodes in the board DTSI.
>>>>>>
>>>>>>> If this is about using the commonly-defined endpoints, Vladimir broke it
>>>>>>> in commit dcf6fb89e6f7 ("media: qcom: camss: remove a check for
>>>>>>> unavailable CAMSS endpoint"). If I do this again and go full circle, I'm
>>>>>>> afraid this could break a second time before even making it to
>>>>>>> linux-next.
>>>>>
>>>>> Quite frankly I don't think that commit was valid, given it's conceivable
>>>>> that an endpoint could be unconnected..
>>>>>
>>>>
>>>> Endpoint is not a device, status property is the property of devices and
>>>> not a property of anything else as the Devicetree Specification v0.4 and
>>>> earlier ones define. Dangling endpoints are fine, there is no need to
>>>> add another property to determine, if an endpoint is connected or not.
>>>>
>>>> There should be no status properties inside endpoint device tree nodes.
>>>
>>> The spec doesn't actually define what a "device" is. Funnily enough, it refers
>>> to "endpoint" as a device:
>>>
>>> 2.2.2 Generic Names Recommendation
>>> The name of a node should be somewhat generic, reflecting the function of the
>>> _device_ and not its precise programming model. If appropriate, the name should
>>> be one of the following choices:
>>>
>>> [...]
>>>
>>> * endpoint
>>>
>>>
>>> Plus an OF node is opaque in its purpose.. The top node, a firmware node, a
>>> node representing a physical IP block and a config.ini-style blurb are all
>>> "device nodes"
>>
>> It sounds like somebody of DT maintainers should clarify the matter and update
>> the spec to be less ambiguous, if it happens that "device" term is undefined.
>>
>>>
>>> But coming back to the real world, the ports/endpoints represent the physical
>>> connections to CAMSS and it makes sense to have them defined in one place,
>>> especially since there's a predictable number of them that should not be left
>>> up to each board to define.. That quite obviously implies that not all boards
>>> are going to utilize all interfaces and the commit of yours that was mentioned
>>> above seems to only be valid on the basis of semantics, which I above mentioned
>>> are not *really* a valid point..
>>
>> For whatever reason CAMSS on SDM670 is very special, because IIRC there is no
>> other platform with preset status poperties of endpoints. This exclusive SDM670
>> specifics shall be analysed and eliminated, since it hasn't been done during
>> patch review time, it's time to do it right now then.
> 
> An SoC-common endpoint node is how panels are hooked up to DSI. This
> seems to be the case for SDM670, SDM845, SM8[123456]50, etc.
> It's not special or unpopular for DSI, but CAMSS seems to be the
> subsystem where an endpoint node pre-defined by the SoC is only in
> SDM670.

Only SDM670 CAMSS endpoints contain 'status' property, not DSI, not
CAMSS on any other platform. This makes SDM670 CAMSS special, and as
I've said above this very odd exclusiveness should be eliminated.

> 
> At least from the context retained in this reply and my memory, the most
> compelling argument I've seen from you so far about removing the
> pre-defined endpoints is (in multiple steps):
> 
> 	1. Suggesting common endpoint nodes instead of common port nodes
> 	https://lore.kernel.org/all/e9dc1a6f-156b-40aa-9209-2010464d54ed@linaro.org/

No-no, I didn't do such a suggestion, please reread the discussion,
I asked to remove labels to ports, and no more, endpoints into that
email thread were introduced by you, Richard.

By the way the follow-up discussion repeats one in one Dmitry's
suggestion right in the current email thread:

https://lore.kernel.org/linux-arm-msm/wwpqaecvz42jopgaboasbh353ieelctpvgo3yj6y5tnxoem5oz@j5sbx3yxntot/

> 	2. Breaking common endpoint nodes in CAMSS
> 	https://lore.kernel.org/all/20250903002255.346026-4-vladimir.zapolskiy@linaro.org/

The change breaks endpoint nodes in assumption that endpoints are
devices, but they are not fortunately.

> This is why I try to remove the common endpoint nodes now.

Please convert SDM670 CAMSS port device tree subnodes to be one-in-one
similar to any other selected CAMSS port device tree subnodes, all
the problems will be gone. It's a pity that the original SDM670 CAMSS
change introduced an invalid and unexptected property of endpoints.

Please take a look at sm8250.dtsi/qrb5165-rb5-vision-mezzanine.dtso,
and repeat this layout for SDM670 CAMSS, the exclusiveness shall
be removed.

-- 
Best wishes,
Vladimir

Re: [PATCH v4 5/5] arm64: dts: qcom: sdm670-google-sargo: add imx355 front camera

[Konrad Dybcio]

On 12/12/25 11:41 PM, David Heidelberg wrote:
> On 11/12/2025 02:48, Richard Acayan wrote:
>> The Sony IMX355 is the front camera on the Pixel 3a, mounted in portrait
>> mode. It is connected to CSIPHY1 and CCI I2C1, and uses MCLK2. Add
>> support for it.
>>
>> Co-developed-by: Robert Mader <robert.mader@collabora.com>
>> Signed-off-by: Robert Mader <robert.mader@collabora.com>
>> Signed-off-by: Richard Acayan <mailingradian@gmail.com>
>> ---
>>   .../boot/dts/qcom/sdm670-google-sargo.dts     | 107 ++++++++++++++++++
>>   1 file changed, 107 insertions(+)
>>
>> diff --git a/arch/arm64/boot/dts/qcom/sdm670-google-sargo.dts b/arch/arm64/boot/dts/qcom/sdm670-google-sargo.dts
>> index d01422844fbf..ede0ad7ded23 100644
>> --- a/arch/arm64/boot/dts/qcom/sdm670-google-sargo.dts
>> +++ b/arch/arm64/boot/dts/qcom/sdm670-google-sargo.dts
>> @@ -172,6 +172,34 @@ vreg_s2b_1p05: vreg-s2b-regulator {
>>           regulator-min-microvolt = <1050000>;
>>           regulator-max-microvolt = <1050000>;
>>       };
>> +
>> +    cam_front_ldo: cam-front-ldo-regulator {
>> +        compatible = "regulator-fixed";
>> +        regulator-name = "cam_front_ldo";
>> +        regulator-min-microvolt = <1352000>;
>> +        regulator-max-microvolt = <1352000>;
>> +        regulator-enable-ramp-delay = <135>;
>> +
>> +        gpios = <&pm660l_gpios 4 GPIO_ACTIVE_HIGH>;
>> +        enable-active-high;
>> +
>> +        pinctrl-0 = <&cam_front_ldo_pin>;
>> +        pinctrl-names = "default";
>> +    };
>> +
>> +    cam_vio_ldo: cam-vio-ldo-regulator {
>> +        compatible = "regulator-fixed";
>> +        regulator-name = "cam_vio_ldo";
>> +        regulator-min-microvolt = <1800000>;
>> +        regulator-max-microvolt = <1800000>;
>> +        regulator-enable-ramp-delay = <233>;
>> +
>> +        gpios = <&pm660_gpios 13 GPIO_ACTIVE_HIGH>;
>> +        enable-active-high;
>> +
>> +        pinctrl-0 = <&cam_vio_pin>;
>> +        pinctrl-names = "default";
>> +    };
>>   };
>>     &apps_rsc {
>> @@ -392,6 +420,64 @@ vreg_bob: bob {
>>       };
>>   };
>>   +&camss {
>> +    vdda-phy-supply = <&vreg_l1a_1p225>;
>> +    vdda-pll-supply = <&vreg_s6a_0p87>;
> The vdda-pll-supply is already named wrongly, as 0p87 != 1.352
> 
> vreg_s6a_0p87: smps6 {
>                        regulator-min-microvolt = <1224000>;
>                        regulator-max-microvolt = <1352000>;
> 
> 
> 
> vdda-phy-supply is on sdm845 (0.75 - 0.8 V) and sdm670 different.
> sdm845: 0.8 V (should be 0.75 - 0.8 V) // OnePlus 6
> sc7280: 0.72 V - 1.05 V // Fairphone 5
> sdm670: 1.2 V
> 
> This may be correct, thou, but still feels like pretty huge diff.
> 
> Maybe someone from QCOM can confirm?

I can't find SDM670 data, but there's both 1.2 V and 0.88 V inputs
to the CSIPHYs on SDM845.

On downstream, i see SDM670 uses L1A @ 1.2 V and enables the REFGEN.
Maybe i can find something more concrete in a couple days.

Konrad

Re: [PATCH v4 4/5] arm64: dts: qcom: sdm670: add camera mclk pins

[Konrad Dybcio]

On 12/11/25 2:48 AM, Richard Acayan wrote:
> The camera subsystem is added for the SoC common devicetree, but the
> mclk pins should also be common across the SoC. Add the mclk pins for
> the cameras.
> 
> Suggested-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
> Link: https://lore.kernel.org/r/5135823c-f2e4-4873-9e3a-9d190cac0113@oss.qualcomm.com
> Signed-off-by: Richard Acayan <mailingradian@gmail.com>
> ---
>  arch/arm64/boot/dts/qcom/sdm670.dtsi | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sdm670.dtsi b/arch/arm64/boot/dts/qcom/sdm670.dtsi
> index c275089237e4..69e84cd8ed27 100644
> --- a/arch/arm64/boot/dts/qcom/sdm670.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sdm670.dtsi
> @@ -1190,6 +1190,27 @@ tlmm: pinctrl@3400000 {
>  			gpio-ranges = <&tlmm 0 0 151>;
>  			wakeup-parent = <&pdc>;
>  
> +			cam_mclk0_default: cam-mclk0-default-state {
> +				pins = "gpio13";
> +				function = "cam_mclk";
> +				drive-strength = <2>;
> +				bias-disable;
> +			};
> +
> +			cam_mclk1_default: cam-mclk1-default-state {
> +				pins = "gpio14";
> +				function = "cam_mclk";
> +				drive-strength = <2>;
> +				bias-disable;
> +			};
> +
> +			cam_mclk2_default: cam-mclk2-default-state {
> +				pins = "gpio15";
> +				function = "cam_mclk";
> +				drive-strength = <2>;
> +				bias-disable;
> +			};

There's also mclk3 on gpio16

Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>

Konrad

Re: [PATCH v4 2/5] media: i2c: imx355: Support devicetree and power management

[Richard Acayan]

On Fri, Dec 12, 2025 at 02:45:35AM +0100, Krzysztof Kozlowski wrote:
> On 12/12/2025 02:43, Krzysztof Kozlowski wrote:
> > On 11/12/2025 02:48, Richard Acayan wrote:
> >> +				    imx355->supplies);
> >> +	if (ret) {
> >> +		dev_err(dev, "failed to enable regulators: %d\n", ret);
> >> +		goto error_disable_clocks;
> >> +	}
> >> +
> >> +	usleep_range(5000, 5100);
> >> +	gpiod_set_value_cansleep(imx355->reset_gpio, 1);
> > 
> > So you just keep device in reset state forever :/
> > 
> > Please see other imx sensor drivers how to fix that.

Sorry that the reset sequence is too confusing. The reset sequence will
be changed to GPIO_ACTIVE_LOW.

> I already told you this at v1 and you ignored the problem and never
> responded.
> 
> NAK

Anyway, I'll move you to CC for the series and carry this in the next
revision.

Re: [PATCH v4 5/5] arm64: dts: qcom: sdm670-google-sargo: add imx355 front camera

[Richard Acayan]

On Wed, Dec 17, 2025 at 05:11:11AM +0200, Vladimir Zapolskiy wrote:
> On 12/17/25 03:41, Richard Acayan wrote:
> > On Tue, Dec 16, 2025 at 05:23:53PM +0200, Vladimir Zapolskiy wrote:
> > > For whatever reason CAMSS on SDM670 is very special, because IIRC there is no
> > > other platform with preset status poperties of endpoints. This exclusive SDM670
> > > specifics shall be analysed and eliminated, since it hasn't been done during
> > > patch review time, it's time to do it right now then.
> > 
> > An SoC-common endpoint node is how panels are hooked up to DSI. This
> > seems to be the case for SDM670, SDM845, SM8[123456]50, etc.
> > It's not special or unpopular for DSI, but CAMSS seems to be the
> > subsystem where an endpoint node pre-defined by the SoC is only in
> > SDM670.
> 
> Only SDM670 CAMSS endpoints contain 'status' property, not DSI, not
> CAMSS on any other platform. This makes SDM670 CAMSS special, and as
> I've said above this very odd exclusiveness should be eliminated.

Oh, if everyone is actually fine with empty endpoints for CAMSS
(`camss_endpoint0: endpoint {};` in SoC dtsi), it might be easier to
patch the driver to accept this:

https://lore.kernel.org/r/20251230022759.9449-1-mailingradian@gmail.com