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

[PATCH 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.

This depends on https://lore.kernel.org/r/20250630224158.249726-2-mailingradian@gmail.com
because the GPIOs would go right next to the charging, if sorted
alphabetically.

Richard Acayan (4):
  dt-bindings: media: i2c: Add Sony IMX355
  media: i2c: imx355: Support device tree probing and resource
    management
  media: i2c: imx355: Add power management for managed resources
  arm64: dts: qcom: sdm670-google-sargo: add imx355 front camera

Robert Mader (1):
  arm64: dts: qcom: sdm670-google-sargo: Add front camera
    rotation/orientation

 .../bindings/media/i2c/sony,imx355.yaml       | 122 ++++++++++++++++++
 .../boot/dts/qcom/sdm670-google-sargo.dts     | 115 +++++++++++++++++
 drivers/media/i2c/imx355.c                    | 109 ++++++++++++++++
 3 files changed, 346 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/i2c/sony,imx355.yaml

-- 
2.50.0

[PATCH 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       | 122 ++++++++++++++++++
 1 file changed, 122 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..7f5f365ee5df
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/i2c/sony,imx355.yaml
@@ -0,0 +1,122 @@
+# 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
+
+  assigned-clocks: true
+  assigned-clock-rates: true
+
+  clocks:
+    maxItems: 1
+
+  clock-names:
+    items:
+      - const: mclk
+
+  clock-frequency:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description: External clock frequency.
+
+  vana-supply:
+    description: Analog power supply.
+
+  vdig-supply:
+    description: Digital power supply.
+
+  vio-supply:
+    description: Interface power supply.
+
+  reset-gpios:
+    maxItems: 1
+
+  port:
+    $ref: /schemas/graph.yaml#/$defs/port-base
+    description:
+      CSI output port.
+
+    properties:
+      endpoint:
+        $ref: /schemas/media/video-interfaces.yaml
+        description:
+          CSI endpoint for the sensor output.
+
+        unevaluatedProperties: false
+
+        required:
+          - link-frequencies
+
+    unevaluatedProperties: false
+
+    required:
+      - endpoint
+
+unevaluatedProperties: false
+
+required:
+  - compatible
+  - reg
+  - clock-frequency
+
+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";
+
+            clock-frequency = <19200000>;
+
+            assigned-clocks = <&camcc CAM_CC_MCLK2_CLK>;
+            assigned-clock-rates = <24000000>;
+
+            reset-gpios = <&tlmm 9 GPIO_ACTIVE_HIGH>;
+
+            vana-supply = <&cam_front_ldo>;
+            vdig-supply = <&cam_front_ldo>;
+            vio-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.50.0

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

[Krzysztof Kozlowski]

On 01/07/2025 00:59, Richard Acayan wrote:
> +
> +  reg:
> +    maxItems: 1
> +
> +  assigned-clocks: true

Drop

> +  assigned-clock-rates: true

Drop, that's some old binding pattern

> +
> +  clocks:
> +    maxItems: 1
> +
> +  clock-names:
> +    items:
> +      - const: mclk
> +
> +  clock-frequency:

Drop, it is a legacy and not needed. Clock gives you the frequency.


> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description: External clock frequency.
> +
> +  vana-supply:
> +    description: Analog power supply.
> +
> +  vdig-supply:
> +    description: Digital power supply.
> +
> +  vio-supply:
> +    description: Interface power supply.
> +
> +  reset-gpios:
> +    maxItems: 1
> +
> +  port:
> +    $ref: /schemas/graph.yaml#/$defs/port-base
> +    description:
> +      CSI output port.
> +
> +    properties:
> +      endpoint:
> +        $ref: /schemas/media/video-interfaces.yaml
> +        description:
> +          CSI endpoint for the sensor output.

Drop description, redundant.

> +
> +        unevaluatedProperties: false

This goes after ref

What binding did you take as reference?

> +
> +        required:
> +          - link-frequencies
> +
> +    unevaluatedProperties: false

This goes up, look at imx415 or 335 (which has very similar number to yours)

> +
> +    required:
> +      - endpoint
> +
> +unevaluatedProperties: false

Wrongly placed, look at other bindings

> +
> +required:
> +  - compatible
> +  - reg
> +  - clock-frequency

No, drop, cannot be required.

But clocks, port and supplies are required. Please look at other recent
binding.
port

> +
> +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";
> +
> +            clock-frequency = <19200000>;
> +
> +            assigned-clocks = <&camcc CAM_CC_MCLK2_CLK>;
> +            assigned-clock-rates = <24000000>;
> +
> +            reset-gpios = <&tlmm 9 GPIO_ACTIVE_HIGH>;

Really? Really high? Let me check your driver...


Best regards,
Krzysztof

[PATCH 2/5] media: i2c: imx355: Support device tree probing and resource management

[Richard Acayan]

A device tree compatible makes it possible for this driver to be used on
Open Firmware devices. Initialization of 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 | 63 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 63 insertions(+)

diff --git a/drivers/media/i2c/imx355.c b/drivers/media/i2c/imx355.c
index b2dce67c0b6b..42bd08e8ac50 100644
--- a/drivers/media/i2c/imx355.c
+++ b/drivers/media/i2c/imx355.c
@@ -3,9 +3,14 @@
 
 #include <linux/unaligned.h>
 #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 <media/v4l2-ctrls.h>
 #include <media/v4l2-device.h>
 #include <media/v4l2-event.h>
@@ -121,6 +126,16 @@ struct imx355 {
 	 * Protect access to sensor v4l2 controls.
 	 */
 	struct mutex mutex;
+
+	struct clk *mclk;
+	struct gpio_desc *reset_gpio;
+	struct regulator_bulk_data supplies[3];
+};
+
+static const char * const imx355_supply_names[] = {
+	"vana",
+	"vdig",
+	"vio",
 };
 
 static const struct imx355_reg imx355_global_regs[] = {
@@ -1683,6 +1698,7 @@ static struct imx355_hwcfg *imx355_get_hwcfg(struct device *dev)
 static int imx355_probe(struct i2c_client *client)
 {
 	struct imx355 *imx355;
+	size_t i;
 	int ret;
 
 	imx355 = devm_kzalloc(&client->dev, sizeof(*imx355), GFP_KERNEL);
@@ -1694,6 +1710,42 @@ static int imx355_probe(struct i2c_client *client)
 	/* 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];
+
+	ret = devm_regulator_bulk_get(&client->dev,
+				      ARRAY_SIZE(imx355->supplies),
+				      imx355->supplies);
+	if (ret) {
+		dev_err_probe(&client->dev, ret, "could not get regulators");
+		return ret;
+	}
+
+	ret = regulator_bulk_enable(ARRAY_SIZE(imx355->supplies),
+				    imx355->supplies);
+	if (ret) {
+		dev_err(&client->dev, "failed to enable regulators: %d\n", ret);
+		return ret;
+	}
+
+	imx355->reset_gpio = devm_gpiod_get_optional(&client->dev, "reset",
+						     GPIOD_OUT_HIGH);
+	if (IS_ERR(imx355->reset_gpio)) {
+		ret = PTR_ERR(imx355->reset_gpio);
+		dev_err_probe(&client->dev, ret, "failed to get gpios");
+		goto error_vreg_disable;
+	}
+
+	imx355->mclk = devm_clk_get(&client->dev, "mclk");
+	if (IS_ERR(imx355->mclk)) {
+		ret = PTR_ERR(imx355->mclk);
+		dev_err_probe(&client->dev, ret, "failed to get mclk");
+		goto error_vreg_disable;
+	}
+
+	clk_prepare_enable(imx355->mclk);
+	usleep_range(12000, 13000);
+
 	/* Check module identity */
 	ret = imx355_identify_module(imx355);
 	if (ret) {
@@ -1756,6 +1808,10 @@ static int imx355_probe(struct i2c_client *client)
 
 error_probe:
 	mutex_destroy(&imx355->mutex);
+	clk_disable_unprepare(imx355->mclk);
+
+error_vreg_disable:
+	regulator_bulk_disable(ARRAY_SIZE(imx355->supplies), imx355->supplies);
 
 	return ret;
 }
@@ -1781,10 +1837,17 @@ 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 = of_match_ptr(imx355_match_table),
 	},
 	.probe = imx355_probe,
 	.remove = imx355_remove,
-- 
2.50.0

Re: [PATCH 2/5] media: i2c: imx355: Support device tree probing and resource management

[Bryan O'Donoghue]

On 30/06/2025 23:59, Richard Acayan wrote:
> A device tree compatible makes it possible for this driver to be used on
> Open Firmware devices. Initialization of 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 | 63 ++++++++++++++++++++++++++++++++++++++
>   1 file changed, 63 insertions(+)
> 
> diff --git a/drivers/media/i2c/imx355.c b/drivers/media/i2c/imx355.c
> index b2dce67c0b6b..42bd08e8ac50 100644
> --- a/drivers/media/i2c/imx355.c
> +++ b/drivers/media/i2c/imx355.c
> @@ -3,9 +3,14 @@
> 
>   #include <linux/unaligned.h>
>   #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 <media/v4l2-ctrls.h>
>   #include <media/v4l2-device.h>
>   #include <media/v4l2-event.h>
> @@ -121,6 +126,16 @@ struct imx355 {
>   	 * Protect access to sensor v4l2 controls.
>   	 */
>   	struct mutex mutex;
> +
> +	struct clk *mclk;
> +	struct gpio_desc *reset_gpio;
> +	struct regulator_bulk_data supplies[3];
> +};
> +
> +static const char * const imx355_supply_names[] = {
> +	"vana",
> +	"vdig",
> +	"vio",
>   };
> 
>   static const struct imx355_reg imx355_global_regs[] = {
> @@ -1683,6 +1698,7 @@ static struct imx355_hwcfg *imx355_get_hwcfg(struct device *dev)
>   static int imx355_probe(struct i2c_client *client)
>   {
>   	struct imx355 *imx355;
> +	size_t i;
>   	int ret;
> 
>   	imx355 = devm_kzalloc(&client->dev, sizeof(*imx355), GFP_KERNEL);
> @@ -1694,6 +1710,42 @@ static int imx355_probe(struct i2c_client *client)
>   	/* 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];
> +
> +	ret = devm_regulator_bulk_get(&client->dev,
> +				      ARRAY_SIZE(imx355->supplies),
> +				      imx355->supplies);
> +	if (ret) {
> +		dev_err_probe(&client->dev, ret, "could not get regulators");
> +		return ret;
> +	}
> +
> +	ret = regulator_bulk_enable(ARRAY_SIZE(imx355->supplies),
> +				    imx355->supplies);
> +	if (ret) {
> +		dev_err(&client->dev, "failed to enable regulators: %d\n", ret);
> +		return ret;
> +	}

You should stick to dev_err_probe().

There's no reason to enable the rails this early either, should be in a 
dedicated power_on() function called in the right place with its own 
cleanup logic for error - to save us from replicating cleanup with jump 
labels.

> +
> +	imx355->reset_gpio = devm_gpiod_get_optional(&client->dev, "reset",
> +						     GPIOD_OUT_HIGH);
> +	if (IS_ERR(imx355->reset_gpio)) {
> +		ret = PTR_ERR(imx355->reset_gpio);
> +		dev_err_probe(&client->dev, ret, "failed to get gpios");
> +		goto error_vreg_disable;
> +	}
> +
> +	imx355->mclk = devm_clk_get(&client->dev, "mclk");
> +	if (IS_ERR(imx355->mclk)) {
> +		ret = PTR_ERR(imx355->mclk);
> +		dev_err_probe(&client->dev, ret, "failed to get mclk");
> +		goto error_vreg_disable;
> +	}
> +
> +	clk_prepare_enable(imx355->mclk);
> +	usleep_range(12000, 13000);
This should go into a dedicated power_on() routine which is reusable, so 
that we have one place to get the power-on sequence right.

Something like drivers/media/i2c/ov02e10.c::ov02e10_power_on()

> +
>   	/* Check module identity */
>   	ret = imx355_identify_module(imx355);

If you move identify_module to come after get_hwcfg then all of the 
above code can be buried inside of get_hwcfg which IMO would be a nice 
pattern to follow.

driver/media/i2c/ov02[c|e].c

---bod

Re: [PATCH 2/5] media: i2c: imx355: Support device tree probing and resource management

[Krzysztof Kozlowski]

On 01/07/2025 00:59, Richard Acayan wrote:

>  static const struct imx355_reg imx355_global_regs[] = {
> @@ -1683,6 +1698,7 @@ static struct imx355_hwcfg *imx355_get_hwcfg(struct device *dev)
>  static int imx355_probe(struct i2c_client *client)
>  {
>  	struct imx355 *imx355;
> +	size_t i;
>  	int ret;
>  
>  	imx355 = devm_kzalloc(&client->dev, sizeof(*imx355), GFP_KERNEL);
> @@ -1694,6 +1710,42 @@ static int imx355_probe(struct i2c_client *client)
>  	/* 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];
> +
> +	ret = devm_regulator_bulk_get(&client->dev,
> +				      ARRAY_SIZE(imx355->supplies),
> +				      imx355->supplies);
> +	if (ret) {
> +		dev_err_probe(&client->dev, ret, "could not get regulators");

Syntax is return dev_err_probe

> +		return ret;
> +	}
> +
> +	ret = regulator_bulk_enable(ARRAY_SIZE(imx355->supplies),
> +				    imx355->supplies);
> +	if (ret) {
> +		dev_err(&client->dev, "failed to enable regulators: %d\n", ret);
> +		return ret;
> +	}
> +
> +	imx355->reset_gpio = devm_gpiod_get_optional(&client->dev, "reset",
> +						     GPIOD_OUT_HIGH);

So you keep the device disabled all the time... this makes no sense. How
can it work if it is always disabled?

> +	if (IS_ERR(imx355->reset_gpio)) {
> +		ret = PTR_ERR(imx355->reset_gpio);
> +		dev_err_probe(&client->dev, ret, "failed to get gpios");
> +		goto error_vreg_disable;
> +	}
> +
> +	imx355->mclk = devm_clk_get(&client->dev, "mclk");
> +	if (IS_ERR(imx355->mclk)) {
> +		ret = PTR_ERR(imx355->mclk);
> +		dev_err_probe(&client->dev, ret, "failed to get mclk");

No, syntax is ret = dev_err_probe.

> +		goto error_vreg_disable;
> +	}
> +
> +	clk_prepare_enable(imx355->mclk);
> +	usleep_range(12000, 13000);
> +
>  	/* Check module identity */
>  	ret = imx355_identify_module(imx355);
>  	if (ret) {
> @@ -1756,6 +1808,10 @@ static int imx355_probe(struct i2c_client *client)
>  
>  error_probe:
>  	mutex_destroy(&imx355->mutex);
> +	clk_disable_unprepare(imx355->mclk);



And where do you clean in remove()? No, just use devm_clk_get_enabled. I
think you are trying to upstream some old code. No, don't. You repeat
several poor patterns, like these err_probe handling or above clock bug
(actual bug). Just look how newest drivers are doing, not the
downstream. Downstream is the greatest antipattern. Absolute antipattern.


Best regards,
Krzysztof

[PATCH 3/5] media: i2c: imx355: Add power management for managed resources

[Richard Acayan]

When enabled, the resources consume power, even if the camera sensor
itself is unused. Add power management functions to release the
resources when the device is suspended.

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

diff --git a/drivers/media/i2c/imx355.c b/drivers/media/i2c/imx355.c
index 42bd08e8ac50..112b1f22afbd 100644
--- a/drivers/media/i2c/imx355.c
+++ b/drivers/media/i2c/imx355.c
@@ -1531,6 +1531,51 @@ static const struct v4l2_subdev_internal_ops imx355_internal_ops = {
 	.open = imx355_open,
 };
 
+static int imx355_suspend(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;
+
+	clk_disable_unprepare(imx355->mclk);
+
+	gpiod_set_value_cansleep(imx355->reset_gpio, 0);
+
+	ret = regulator_bulk_disable(ARRAY_SIZE(imx355->supplies),
+				    imx355->supplies);
+	if (ret) {
+		dev_err(dev, "failed to disable regulators: %d\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int imx355_resume(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 = regulator_bulk_enable(ARRAY_SIZE(imx355->supplies),
+				    imx355->supplies);
+	if (ret) {
+		dev_err(dev, "failed to enable regulators: %d\n", ret);
+		return ret;
+	}
+
+	gpiod_set_value_cansleep(imx355->reset_gpio, 1);
+
+	clk_prepare_enable(imx355->mclk);
+	usleep_range(12000, 13000);
+
+	return 0;
+}
+
+DEFINE_RUNTIME_DEV_PM_OPS(imx355_pm_ops, imx355_suspend, imx355_resume, NULL);
+
 /* Initialize control handlers */
 static int imx355_init_controls(struct imx355 *imx355)
 {
@@ -1848,6 +1893,7 @@ static struct i2c_driver imx355_i2c_driver = {
 		.name = "imx355",
 		.acpi_match_table = ACPI_PTR(imx355_acpi_ids),
 		.of_match_table = of_match_ptr(imx355_match_table),
+		.pm = &imx355_pm_ops,
 	},
 	.probe = imx355_probe,
 	.remove = imx355_remove,
-- 
2.50.0

Re: [PATCH 3/5] media: i2c: imx355: Add power management for managed resources

[Bryan O'Donoghue]

On 30/06/2025 23:59, Richard Acayan wrote:
> +static int imx355_resume(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 = regulator_bulk_enable(ARRAY_SIZE(imx355->supplies),
> +				    imx355->supplies);
> +	if (ret) {
> +		dev_err(dev, "failed to enable regulators: %d\n", ret);
> +		return ret;
> +	}
> +
> +	gpiod_set_value_cansleep(imx355->reset_gpio, 1);
> +
> +	clk_prepare_enable(imx355->mclk);
> +	usleep_range(12000, 13000);
> +
> +	return 0;
> +}

I'd say this sequence is out of spec w/r/t your sensor.

Almost certainly should be

- clock
- rails
- reset lines

The reset sequence should be

- Assert reset
- Wait for some amount of time. Either you get this from the spec
   or you borrow a value from a similar driver...
   Suggest again what we've done for ov02c10 and ov02e10
- De-assert reset

Then use these power_on/power_off sequences in your probe discovery, 
since they will already have error jump labels to do your tidy up.

---
bod

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

[Richard Acayan]

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

Signed-off-by: Richard Acayan <mailingradian@gmail.com>
---
 .../boot/dts/qcom/sdm670-google-sargo.dts     | 112 ++++++++++++++++++
 1 file changed, 112 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..0af6a440ecbc 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-names = "default";
+		pinctrl-0 = <&cam_front_ldo_pin>;
+	};
+
+	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-names = "default";
+		pinctrl-0 = <&cam_vio_pin>;
+	};
 };
 
 &apps_rsc {
@@ -392,6 +420,58 @@ vreg_bob: bob {
 	};
 };
 
+&camss {
+	vdda-phy-supply = <&vreg_l1a_1p225>;
+	status = "okay";
+};
+
+&camss_endpoint1 {
+	clock-lanes = <7>;
+	data-lanes = <0 1 2 3>;
+	remote-endpoint = <&cam_front_endpoint>;
+	status = "okay";
+};
+
+&cci {
+	pinctrl-names = "default", "sleep";
+	pinctrl-0 = <&cci1_default &cam_mclk_default>;
+	pinctrl-1 = <&cci1_sleep>;
+
+	status = "okay";
+};
+
+&cci_i2c1 {
+	camera@1a {
+		compatible = "sony,imx355";
+		reg = <0x1a>;
+
+		clocks = <&camcc CAM_CC_MCLK2_CLK>;
+		clock-names = "mclk";
+
+		clock-frequency = <19200000>;
+
+		assigned-clocks = <&camcc CAM_CC_MCLK2_CLK>;
+		assigned-clock-rates = <24000000>;
+
+		reset-gpios = <&tlmm 9 GPIO_ACTIVE_HIGH>;
+
+		vana-supply = <&cam_front_ldo>;
+		vdig-supply = <&cam_front_ldo>;
+		vio-supply = <&cam_vio_ldo>;
+
+		pinctrl-names = "default";
+		pinctrl-0 = <&cam_front_default>;
+
+		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 +571,15 @@ &pm660_charger {
 	status = "okay";
 };
 
+&pm660_gpios {
+	cam_vio_pin: cam-vio-state {
+		pins = "gpio13";
+		function = "normal";
+		power-source = <PM8941_GPIO_VPH>;
+		output-low;
+	};
+};
+
 &pm660_rradc {
 	status = "okay";
 };
@@ -509,6 +598,13 @@ led-0 {
 };
 
 &pm660l_gpios {
+	cam_front_ldo_pin: cam-front-state {
+		pins = "gpio4";
+		function = "normal";
+		power-source = <PM8941_GPIO_VPH>;
+		output-low;
+	};
+
 	vol_up_pin: vol-up-state {
 		pins = "gpio7";
 		function = "normal";
@@ -548,6 +644,22 @@ &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;
+	};
+
+	cam_mclk_default: cam-default-state {
+		mclk2-pins {
+			pins = "gpio15";
+			function = "cam_mclk";
+			drive-strength = <2>;
+			bias-disable;
+		};
+	};
+
 	panel_default: panel-default-state {
 		te-pins {
 			pins = "gpio10";
-- 
2.50.0

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

[Konrad Dybcio]

On 01-Jul-25 00:59, Richard Acayan wrote:
> The Sony IMX355 is the front camera on the Pixel 3a. It is connected to
> CSIPHY1 and CCI I2C1, and uses MCLK2. Add support for it.
> 
> Signed-off-by: Richard Acayan <mailingradian@gmail.com>
> ---
>  .../boot/dts/qcom/sdm670-google-sargo.dts     | 112 ++++++++++++++++++
>  1 file changed, 112 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..0af6a440ecbc 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-names = "default";
> +		pinctrl-0 = <&cam_front_ldo_pin>;

property-n
property-names

please

[...]

> +&camss {
> +	vdda-phy-supply = <&vreg_l1a_1p225>;
> +	status = "okay";

Please consistently add a newline before 'status'

[...]

> +&cci_i2c1 {
> +	camera@1a {
> +		compatible = "sony,imx355";
> +		reg = <0x1a>;
> +
> +		clocks = <&camcc CAM_CC_MCLK2_CLK>;
> +		clock-names = "mclk";
> +
> +		clock-frequency = <19200000>;
> +
> +		assigned-clocks = <&camcc CAM_CC_MCLK2_CLK>;
> +		assigned-clock-rates = <24000000>;

This and the above rate are in a bit of a disagreement..

[...]

> +	cam_mclk_default: cam-default-state {
> +		mclk2-pins {

You can drop this extra level of {} and put the properties
directly under cam-default-state

Konrad

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

[Bryan O'Donoghue]

On 30/06/2025 23:59, Richard Acayan wrote:
> The Sony IMX355 is the front camera on the Pixel 3a. It is connected to
> CSIPHY1 and CCI I2C1, and uses MCLK2. Add support for it.
> 
> Signed-off-by: Richard Acayan <mailingradian@gmail.com>
> ---
>   .../boot/dts/qcom/sdm670-google-sargo.dts     | 112 ++++++++++++++++++
>   1 file changed, 112 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..0af6a440ecbc 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-names = "default";
> +		pinctrl-0 = <&cam_front_ldo_pin>;
> +	};
> +
> +	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-names = "default";
> +		pinctrl-0 = <&cam_vio_pin>;
> +	};
>   };
> 
>   &apps_rsc {
> @@ -392,6 +420,58 @@ vreg_bob: bob {
>   	};
>   };
> 
> +&camss {
> +	vdda-phy-supply = <&vreg_l1a_1p225>;

You've got your 1p2 but looks like you are missing your 0p8 supply

> +	status = "okay";
> +};
> +
> +&camss_endpoint1 {
> +	clock-lanes = <7>;
> +	data-lanes = <0 1 2 3>;
> +	remote-endpoint = <&cam_front_endpoint>;
> +	status = "okay";
> +};

This looks not like how the other dts are upstream. Does this work and 
pass the dt checker ?

Right now upstream wants something like this

&camss {
         vdda-phy-supply = <&vreg_l5a_0p88>;
         vdda-pll-supply = <&vreg_l9a_1p2>;
         status = "okay";

         ports {
                 /* The port index denotes CSIPHY id i.e. csiphy2 */
                 port@2 {
                         csiphy2_ep: endpoint {
                                 clock-lanes = <7>;
                                 data-lanes = <0 1 2 3>;
                                 remote-endpoint = <&imx577_ep>;
                         };
                 };
         };
};

Can the upstream driver actually consume the dt as you specified above ?

> +
> +&cci {
> +	pinctrl-names = "default", "sleep";
> +	pinctrl-0 = <&cci1_default &cam_mclk_default>;
> +	pinctrl-1 = <&cci1_sleep>;
> +
> +	status = "okay";
> +};
> +
> +&cci_i2c1 {
> +	camera@1a {
> +		compatible = "sony,imx355";
> +		reg = <0x1a>;
> +
> +		clocks = <&camcc CAM_CC_MCLK2_CLK>;
> +		clock-names = "mclk";
> +
> +		clock-frequency = <19200000>;
> +
> +		assigned-clocks = <&camcc CAM_CC_MCLK2_CLK>;
> +		assigned-clock-rates = <24000000>;
> +
> +		reset-gpios = <&tlmm 9 GPIO_ACTIVE_HIGH>;
> +
> +		vana-supply = <&cam_front_ldo>;
> +		vdig-supply = <&cam_front_ldo>;
> +		vio-supply = <&cam_vio_ldo>;

These are the downstream names, taking imx512/477 as a reference point

                 dovdd-supply = <&vreg_l7f_1p8>;
                 avdd-supply = <&vdc_5v>;
                 dvdd-supply = <&vdc_5v>;

I'd guess the data sheet probably has better names like that.

---
bod

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

[Richard Acayan]

On Tue, Jul 01, 2025 at 01:23:44PM +0100, Bryan O'Donoghue wrote:
> On 30/06/2025 23:59, Richard Acayan wrote:
> > The Sony IMX355 is the front camera on the Pixel 3a. It is connected to
> > CSIPHY1 and CCI I2C1, and uses MCLK2. Add support for it.
> > 
> > Signed-off-by: Richard Acayan <mailingradian@gmail.com>
> > ---
> >   .../boot/dts/qcom/sdm670-google-sargo.dts     | 112 ++++++++++++++++++
> >   1 file changed, 112 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..0af6a440ecbc 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-names = "default";
> > +		pinctrl-0 = <&cam_front_ldo_pin>;
> > +	};
> > +
> > +	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-names = "default";
> > +		pinctrl-0 = <&cam_vio_pin>;
> > +	};
> >   };
> > 
> >   &apps_rsc {
> > @@ -392,6 +420,58 @@ vreg_bob: bob {
> >   	};
> >   };
> > 
> > +&camss {
> > +	vdda-phy-supply = <&vreg_l1a_1p225>;
> 
> You've got your 1p2 but looks like you are missing your 0p8 supply

This is probably vreg_s6a_0p87 which supplies vreg_l1a_1p225.

> > +	status = "okay";
> > +};
> > +
> > +&camss_endpoint1 {
> > +	clock-lanes = <7>;
> > +	data-lanes = <0 1 2 3>;
> > +	remote-endpoint = <&cam_front_endpoint>;
> > +	status = "okay";
> > +};
> 
> This looks not like how the other dts are upstream. Does this work and pass
> the dt checker ?
> 
> Right now upstream wants something like this
> 
> &camss {
>         vdda-phy-supply = <&vreg_l5a_0p88>;
>         vdda-pll-supply = <&vreg_l9a_1p2>;
>         status = "okay";
> 
>         ports {
>                 /* The port index denotes CSIPHY id i.e. csiphy2 */
>                 port@2 {
>                         csiphy2_ep: endpoint {
>                                 clock-lanes = <7>;
>                                 data-lanes = <0 1 2 3>;
>                                 remote-endpoint = <&imx577_ep>;
>                         };
>                 };
>         };
> };

I misunderstood a review comment from an earlier series. We can do the
same thing here instead of pushing a different style.

> Can the upstream driver actually consume the dt as you specified above ?

If you're curious, it does understand and let you go as far as using the
camera.

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

[Bryan O'Donoghue]

On 02/07/2025 00:15, Richard Acayan wrote:
> On Tue, Jul 01, 2025 at 01:23:44PM +0100, Bryan O'Donoghue wrote:
>> On 30/06/2025 23:59, Richard Acayan wrote:
>>> The Sony IMX355 is the front camera on the Pixel 3a. It is connected to
>>> CSIPHY1 and CCI I2C1, and uses MCLK2. Add support for it.
>>>
>>> Signed-off-by: Richard Acayan <mailingradian@gmail.com>
>>> ---
>>>    .../boot/dts/qcom/sdm670-google-sargo.dts     | 112 ++++++++++++++++++
>>>    1 file changed, 112 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..0af6a440ecbc 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-names = "default";
>>> +		pinctrl-0 = <&cam_front_ldo_pin>;
>>> +	};
>>> +
>>> +	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-names = "default";
>>> +		pinctrl-0 = <&cam_vio_pin>;
>>> +	};
>>>    };
>>>
>>>    &apps_rsc {
>>> @@ -392,6 +420,58 @@ vreg_bob: bob {
>>>    	};
>>>    };
>>>
>>> +&camss {
>>> +	vdda-phy-supply = <&vreg_l1a_1p225>;
>>
>> You've got your 1p2 but looks like you are missing your 0p8 supply
> 
> This is probably vreg_s6a_0p87 which supplies vreg_l1a_1p225.

Yep adds up to me.

> 
>>> +	status = "okay";
>>> +};
>>> +
>>> +&camss_endpoint1 {
>>> +	clock-lanes = <7>;
>>> +	data-lanes = <0 1 2 3>;
>>> +	remote-endpoint = <&cam_front_endpoint>;
>>> +	status = "okay";
>>> +};
>>
>> This looks not like how the other dts are upstream. Does this work and pass
>> the dt checker ?
>>
>> Right now upstream wants something like this
>>
>> &camss {
>>          vdda-phy-supply = <&vreg_l5a_0p88>;
>>          vdda-pll-supply = <&vreg_l9a_1p2>;
>>          status = "okay";
>>
>>          ports {
>>                  /* The port index denotes CSIPHY id i.e. csiphy2 */
>>                  port@2 {
>>                          csiphy2_ep: endpoint {
>>                                  clock-lanes = <7>;
>>                                  data-lanes = <0 1 2 3>;
>>                                  remote-endpoint = <&imx577_ep>;
>>                          };
>>                  };
>>          };
>> };
> 
> I misunderstood a review comment from an earlier series. We can do the
> same thing here instead of pushing a different style.

Yes. Unless/until there's a change in linux-next you can safely ignore 
all opinions/statements including mine on how things will be in the 
future...

>> Can the upstream driver actually consume the dt as you specified above ?
> 
> If you're curious, it does understand and let you go as far as using the
> camera.

Nice.

Good work.

---
bod

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

[Krzysztof Kozlowski]

On 01/07/2025 00:59, Richard Acayan wrote:
> +&cci_i2c1 {
> +	camera@1a {
> +		compatible = "sony,imx355";
> +		reg = <0x1a>;
> +
> +		clocks = <&camcc CAM_CC_MCLK2_CLK>;
> +		clock-names = "mclk";
> +
> +		clock-frequency = <19200000>;

No, drop. This is not a suitable property for DT I2C devices.

Best regards,
Krzysztof

[PATCH 5/5] arm64: dts: qcom: sdm670-google-sargo: Add front camera rotation/orientation

[Richard Acayan]

From: Robert Mader <robert.mader@collabora.com>

So it gets properly reported to clients like libcamera.

Signed-off-by: Robert Mader <robert.mader@collabora.com>
[richard: rebase onto patch series]
Signed-off-by: Richard Acayan <mailingradian@gmail.com>
---
 arch/arm64/boot/dts/qcom/sdm670-google-sargo.dts | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sdm670-google-sargo.dts b/arch/arm64/boot/dts/qcom/sdm670-google-sargo.dts
index 0af6a440ecbc..fcd2f1b77679 100644
--- a/arch/arm64/boot/dts/qcom/sdm670-google-sargo.dts
+++ b/arch/arm64/boot/dts/qcom/sdm670-google-sargo.dts
@@ -462,6 +462,9 @@ camera@1a {
 		pinctrl-names = "default";
 		pinctrl-0 = <&cam_front_default>;
 
+		rotation = <270>;
+		orientation = <0>;
+
 		port {
 			cam_front_endpoint: endpoint {
 				data-lanes = <0 1 2 3>;
-- 
2.50.0

Re: [PATCH 5/5] arm64: dts: qcom: sdm670-google-sargo: Add front camera rotation/orientation

[Konrad Dybcio]

On 01-Jul-25 00:59, Richard Acayan wrote:
> From: Robert Mader <robert.mader@collabora.com>
> 
> So it gets properly reported to clients like libcamera.
> 
> Signed-off-by: Robert Mader <robert.mader@collabora.com>
> [richard: rebase onto patch series]
> Signed-off-by: Richard Acayan <mailingradian@gmail.com>
> ---

It would make sense to squash this patch into the previous one,
you can mention Robert's contribution in the commit message

Konrad

Re: [PATCH 5/5] arm64: dts: qcom: sdm670-google-sargo: Add front camera rotation/orientation

[Krzysztof Kozlowski]

On 01/07/2025 00:59, Richard Acayan wrote:
> From: Robert Mader <robert.mader@collabora.com>
> 
> So it gets properly reported to clients like libcamera.
> 
> Signed-off-by: Robert Mader <robert.mader@collabora.com>
> [richard: rebase onto patch series]
> Signed-off-by: Richard Acayan <mailingradian@gmail.com>
> ---
>  arch/arm64/boot/dts/qcom/sdm670-google-sargo.dts | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sdm670-google-sargo.dts b/arch/arm64/boot/dts/qcom/sdm670-google-sargo.dts
> index 0af6a440ecbc..fcd2f1b77679 100644
> --- a/arch/arm64/boot/dts/qcom/sdm670-google-sargo.dts
> +++ b/arch/arm64/boot/dts/qcom/sdm670-google-sargo.dts
> @@ -462,6 +462,9 @@ camera@1a {
>  		pinctrl-names = "default";
>  		pinctrl-0 = <&cam_front_default>;
>  
> +		rotation = <270>;
> +		orientation = <0>;
> +

So your previous commit added buggy code which you fix now. No, add
correct code in the first place - not a bug which later you fix. This
must be squashed.

Best regards,
Krzysztof