* [PATCH v12 1/6] dt-bindings: media: qcom,x1e80100-camss: Add optional PHY handle definitions
2026-07-08 0:06 [PATCH v12 0/6] Add dt-bindings and PHY updates for CAMSS on x1e80100 silicon Bryan O'Donoghue
@ 2026-07-08 0:06 ` Bryan O'Donoghue
2026-07-08 0:16 ` sashiko-bot
` (3 more replies)
2026-07-08 0:06 ` [PATCH v12 2/6] dt-bindings: media: qcom,x1e80100-camss: Describe iommu entries Bryan O'Donoghue
` (4 subsequent siblings)
5 siblings, 4 replies; 20+ messages in thread
From: Bryan O'Donoghue @ 2026-07-08 0:06 UTC (permalink / raw)
To: Bjorn Andersson, Michael Turquette, Stephen Boyd, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Robert Foss, Todor Tomov,
Mauro Carvalho Chehab, Konrad Dybcio, Vladimir Zapolskiy,
Bryan O'Donoghue, Loic Poulain
Cc: linux-arm-msm, linux-clk, devicetree, linux-kernel, linux-media,
Bryan O'Donoghue, Krzysztof Kozlowski
Add optional PHY handle definitions. This will allow for supporting both
legacy PHY definitions as well as supporting the optional new handle based
approach.
Drop the legacy high-level 0p8 and 1p2 supplies as required, each PHY has
its own individual rails. The old binding is still valid but with
individual nodes we define the rails in the CSIPHY sub-nodes.
Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
---
.../bindings/media/qcom,x1e80100-camss.yaml | 33 ++++++++++++++++++++--
1 file changed, 31 insertions(+), 2 deletions(-)
diff --git a/Documentation/devicetree/bindings/media/qcom,x1e80100-camss.yaml b/Documentation/devicetree/bindings/media/qcom,x1e80100-camss.yaml
index 2d1662ef522b7..c17b9757b2c86 100644
--- a/Documentation/devicetree/bindings/media/qcom,x1e80100-camss.yaml
+++ b/Documentation/devicetree/bindings/media/qcom,x1e80100-camss.yaml
@@ -39,6 +39,14 @@ properties:
- const: vfe_lite0
- const: vfe_lite1
+ '#address-cells':
+ const: 2
+
+ '#size-cells':
+ const: 2
+
+ ranges: true
+
clocks:
maxItems: 29
@@ -126,6 +134,16 @@ properties:
description:
1.2V supply to a PHY.
+ phys:
+ maxItems: 4
+
+ phy-names:
+ items:
+ - const: csiphy0
+ - const: csiphy1
+ - const: csiphy2
+ - const: csiphy4
+
ports:
$ref: /schemas/graph.yaml#/properties/ports
@@ -158,6 +176,14 @@ properties:
required:
- data-lanes
+patternProperties:
+ "^phy@[0-9a-f]+$":
+ $ref: /schemas/phy/qcom,x1e80100-csi2-phy.yaml
+ unevaluatedProperties: false
+
+ "^opp-table(-.*)?$":
+ type: object
+
required:
- compatible
- reg
@@ -171,8 +197,6 @@ required:
- iommus
- power-domains
- power-domain-names
- - vdd-csiphy-0p8-supply
- - vdd-csiphy-1p2-supply
- ports
additionalProperties: false
@@ -184,6 +208,7 @@ examples:
#include <dt-bindings/clock/qcom,x1e80100-camcc.h>
#include <dt-bindings/interconnect/qcom,icc.h>
#include <dt-bindings/interconnect/qcom,x1e80100-rpmh.h>
+ #include <dt-bindings/phy/phy.h>
#include <dt-bindings/power/qcom-rpmpd.h>
soc {
@@ -229,6 +254,10 @@ examples:
"vfe_lite0",
"vfe_lite1";
+ #address-cells = <2>;
+ #size-cells = <2>;
+ ranges;
+
clocks = <&camcc CAM_CC_CAMNOC_AXI_NRT_CLK>,
<&camcc CAM_CC_CAMNOC_AXI_RT_CLK>,
<&camcc CAM_CC_CORE_AHB_CLK>,
--
2.54.0
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH v12 1/6] dt-bindings: media: qcom,x1e80100-camss: Add optional PHY handle definitions
2026-07-08 0:06 ` [PATCH v12 1/6] dt-bindings: media: qcom,x1e80100-camss: Add optional PHY handle definitions Bryan O'Donoghue
@ 2026-07-08 0:16 ` sashiko-bot
2026-07-08 1:34 ` Rob Herring (Arm)
` (2 subsequent siblings)
3 siblings, 0 replies; 20+ messages in thread
From: sashiko-bot @ 2026-07-08 0:16 UTC (permalink / raw)
To: Bryan O'Donoghue; +Cc: conor+dt, robh, devicetree
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] The binding adds a `$ref` to `qcom,x1e80100-csi2-phy.yaml`, which does not exist.
- [High] Overlapping MMIO region claims between parent and child nodes.
- [Medium] Removing legacy power supplies from the required list without an alternative constraint allows incomplete DT definitions.
- [Medium] The patch silently adds `opp-table` to `patternProperties` without documentation or proper schema constraints.
--
--- Patch [1]: [PATCH v12 1/6] dt-bindings: media: qcom,x1e80100-camss: Add optional PHY handle definitions ---
commit a10cbc21177fae0f236d507fc2a6068204516723
Author: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
dt-bindings: media: qcom,x1e80100-camss: Add optional PHY handle definitions
Add optional PHY handle definitions. This will allow for supporting both
legacy PHY definitions as well as supporting the optional new handle based
approach. Drops the legacy high-level 0p8 and 1p2 supplies as required.
> diff --git a/Documentation/devicetree/bindings/media/qcom,x1e80100-camss.yaml b/Documentation/devicetree/bindings/media/qcom,x1e80100-camss.yaml
> index 2d1662ef522b7..c17b9757b2c86 100644
> --- a/Documentation/devicetree/bindings/media/qcom,x1e80100-camss.yaml
> +++ b/Documentation/devicetree/bindings/media/qcom,x1e80100-camss.yaml
[ ... ]
> @@ -158,6 +176,14 @@ properties:
> required:
> - data-lanes
>
> +patternProperties:
> + "^phy@[0-9a-f]+$":
> + $ref: /schemas/phy/qcom,x1e80100-csi2-phy.yaml
[Severity: High]
Does this referenced schema exist? It appears that running make
dt_binding_check will crash with a jsonschema.exceptions.RefResolutionError
because qcom,x1e80100-csi2-phy.yaml is missing from the tree.
[Severity: High]
If the parent CAMSS node still explicitly claims the csiphy0 through csiphy4
memory regions in its reg-names array, won't adding child nodes here that
map these same MMIO regions cause overlapping request_mem_region calls in
the driver, leading to EBUSY probe failures?
> + unevaluatedProperties: false
> +
> + "^opp-table(-.*)?$":
> + type: object
[Severity: Medium]
Is it intended to add opp-table without a $ref to opp-v2-base.yaml? Using
only type: object bypasses strict schema validation for OPP tables, and this
behavioral change doesn't appear to be mentioned in the commit message.
> required:
> - compatible
> - reg
[ ... ]
> @@ -171,8 +197,6 @@ required:
> - iommus
> - power-domains
> - power-domain-names
> - - vdd-csiphy-0p8-supply
> - - vdd-csiphy-1p2-supply
> - ports
>
> additionalProperties: false
[Severity: Medium]
By unconditionally dropping these global supplies without enforcing an
alternative (such as using anyOf to require either the legacy supplies or
the new phys nodes), does this allow incomplete device tree descriptions
that omit PHY configurations entirely to silently pass validation?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260708-b4-linux-next-25-03-13-dtsi-x1e80100-camss-v12-0-f8588da41f16@linaro.org?part=1
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v12 1/6] dt-bindings: media: qcom,x1e80100-camss: Add optional PHY handle definitions
2026-07-08 0:06 ` [PATCH v12 1/6] dt-bindings: media: qcom,x1e80100-camss: Add optional PHY handle definitions Bryan O'Donoghue
2026-07-08 0:16 ` sashiko-bot
@ 2026-07-08 1:34 ` Rob Herring (Arm)
2026-07-08 7:49 ` Vladimir Zapolskiy
2026-07-08 22:09 ` Frank Li
3 siblings, 0 replies; 20+ messages in thread
From: Rob Herring (Arm) @ 2026-07-08 1:34 UTC (permalink / raw)
To: Bryan O'Donoghue
Cc: Krzysztof Kozlowski, Michael Turquette, Loic Poulain, linux-media,
Bryan O'Donoghue, Bjorn Andersson, Todor Tomov,
Vladimir Zapolskiy, Stephen Boyd, Krzysztof Kozlowski,
Konrad Dybcio, devicetree, Mauro Carvalho Chehab, Robert Foss,
Conor Dooley, linux-clk, linux-arm-msm, linux-kernel
On Wed, 08 Jul 2026 01:06:55 +0100, Bryan O'Donoghue wrote:
> Add optional PHY handle definitions. This will allow for supporting both
> legacy PHY definitions as well as supporting the optional new handle based
> approach.
>
> Drop the legacy high-level 0p8 and 1p2 supplies as required, each PHY has
> its own individual rails. The old binding is still valid but with
> individual nodes we define the rails in the CSIPHY sub-nodes.
>
> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> ---
> .../bindings/media/qcom,x1e80100-camss.yaml | 33 ++++++++++++++++++++--
> 1 file changed, 31 insertions(+), 2 deletions(-)
>
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/qcom,x1e80100-camss.yaml: Unresolvable reference: /schemas/phy/qcom,x1e80100-csi2-phy.yaml
doc reference errors (make refcheckdocs):
See https://patchwork.kernel.org/project/devicetree/patch/20260708-b4-linux-next-25-03-13-dtsi-x1e80100-camss-v12-1-f8588da41f16@linaro.org
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.
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v12 1/6] dt-bindings: media: qcom,x1e80100-camss: Add optional PHY handle definitions
2026-07-08 0:06 ` [PATCH v12 1/6] dt-bindings: media: qcom,x1e80100-camss: Add optional PHY handle definitions Bryan O'Donoghue
2026-07-08 0:16 ` sashiko-bot
2026-07-08 1:34 ` Rob Herring (Arm)
@ 2026-07-08 7:49 ` Vladimir Zapolskiy
2026-07-08 22:09 ` Frank Li
3 siblings, 0 replies; 20+ messages in thread
From: Vladimir Zapolskiy @ 2026-07-08 7:49 UTC (permalink / raw)
To: Bryan O'Donoghue, Bjorn Andersson, Michael Turquette,
Stephen Boyd, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Robert Foss, Todor Tomov, Mauro Carvalho Chehab, Konrad Dybcio,
Bryan O'Donoghue, Loic Poulain
Cc: linux-arm-msm, linux-clk, devicetree, linux-kernel, linux-media,
Krzysztof Kozlowski
On 7/8/26 03:06, Bryan O'Donoghue wrote:
> Add optional PHY handle definitions. This will allow for supporting both
> legacy PHY definitions as well as supporting the optional new handle based
> approach.
>
> Drop the legacy high-level 0p8 and 1p2 supplies as required, each PHY has
> its own individual rails. The old binding is still valid but with
> individual nodes we define the rails in the CSIPHY sub-nodes.
>
> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> ---
> .../bindings/media/qcom,x1e80100-camss.yaml | 33 ++++++++++++++++++++--
> 1 file changed, 31 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/media/qcom,x1e80100-camss.yaml b/Documentation/devicetree/bindings/media/qcom,x1e80100-camss.yaml
> index 2d1662ef522b7..c17b9757b2c86 100644
> --- a/Documentation/devicetree/bindings/media/qcom,x1e80100-camss.yaml
> +++ b/Documentation/devicetree/bindings/media/qcom,x1e80100-camss.yaml
> @@ -39,6 +39,14 @@ properties:
> - const: vfe_lite0
> - const: vfe_lite1
>
> + '#address-cells':
> + const: 2
> +
> + '#size-cells':
> + const: 2
> +
> + ranges: true
> +
> clocks:
> maxItems: 29
>
> @@ -126,6 +134,16 @@ properties:
> description:
> 1.2V supply to a PHY.
>
> + phys:
> + maxItems: 4
> +
> + phy-names:
> + items:
> + - const: csiphy0
> + - const: csiphy1
> + - const: csiphy2
> + - const: csiphy4
> +
> ports:
> $ref: /schemas/graph.yaml#/properties/ports
>
> @@ -158,6 +176,14 @@ properties:
> required:
> - data-lanes
>
> +patternProperties:
> + "^phy@[0-9a-f]+$":
This is too wide regexp mask for 4 children CSIPHYs.
> + $ref: /schemas/phy/qcom,x1e80100-csi2-phy.yaml
> + unevaluatedProperties: false
> +
> + "^opp-table(-.*)?$":
> + type: object
> +
> required:
> - compatible
> - reg
> @@ -171,8 +197,6 @@ required:
> - iommus
> - power-domains
> - power-domain-names
> - - vdd-csiphy-0p8-supply
> - - vdd-csiphy-1p2-supply
> - ports
>
> additionalProperties: false
> @@ -184,6 +208,7 @@ examples:
> #include <dt-bindings/clock/qcom,x1e80100-camcc.h>
> #include <dt-bindings/interconnect/qcom,icc.h>
> #include <dt-bindings/interconnect/qcom,x1e80100-rpmh.h>
> + #include <dt-bindings/phy/phy.h>
The inclusion of this header is not needed.
> #include <dt-bindings/power/qcom-rpmpd.h>
>
> soc {
> @@ -229,6 +254,10 @@ examples:
> "vfe_lite0",
> "vfe_lite1";
>
> + #address-cells = <2>;
> + #size-cells = <2>;
> + ranges;
> +
> clocks = <&camcc CAM_CC_CAMNOC_AXI_NRT_CLK>,
> <&camcc CAM_CC_CAMNOC_AXI_RT_CLK>,
> <&camcc CAM_CC_CORE_AHB_CLK>,
>
--
Best wishes,
Vladimir
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v12 1/6] dt-bindings: media: qcom,x1e80100-camss: Add optional PHY handle definitions
2026-07-08 0:06 ` [PATCH v12 1/6] dt-bindings: media: qcom,x1e80100-camss: Add optional PHY handle definitions Bryan O'Donoghue
` (2 preceding siblings ...)
2026-07-08 7:49 ` Vladimir Zapolskiy
@ 2026-07-08 22:09 ` Frank Li
2026-07-08 22:15 ` Bryan O'Donoghue
3 siblings, 1 reply; 20+ messages in thread
From: Frank Li @ 2026-07-08 22:09 UTC (permalink / raw)
To: Bryan O'Donoghue
Cc: Bjorn Andersson, Michael Turquette, Stephen Boyd, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Robert Foss, Todor Tomov,
Mauro Carvalho Chehab, Konrad Dybcio, Vladimir Zapolskiy,
Bryan O'Donoghue, Loic Poulain, linux-arm-msm, linux-clk,
devicetree, linux-kernel, linux-media, Krzysztof Kozlowski
On Wed, Jul 08, 2026 at 01:06:55AM +0100, Bryan O'Donoghue wrote:
> Add optional PHY handle definitions. This will allow for supporting both
> legacy PHY definitions as well as supporting the optional new handle based
> approach.
>
> Drop the legacy high-level 0p8 and 1p2 supplies as required, each PHY has
> its own individual rails. The old binding is still valid but with
> individual nodes we define the rails in the CSIPHY sub-nodes.
>
> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> ---
> .../bindings/media/qcom,x1e80100-camss.yaml | 33 ++++++++++++++++++++--
> 1 file changed, 31 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/media/qcom,x1e80100-camss.yaml b/Documentation/devicetree/bindings/media/qcom,x1e80100-camss.yaml
> index 2d1662ef522b7..c17b9757b2c86 100644
> --- a/Documentation/devicetree/bindings/media/qcom,x1e80100-camss.yaml
> +++ b/Documentation/devicetree/bindings/media/qcom,x1e80100-camss.yaml
> @@ -39,6 +39,14 @@ properties:
> - const: vfe_lite0
> - const: vfe_lite1
>
> + '#address-cells':
> + const: 2
> +
> + '#size-cells':
> + const: 2
> +
> + ranges: true
> +
> clocks:
> maxItems: 29
>
> @@ -126,6 +134,16 @@ properties:
> description:
> 1.2V supply to a PHY.
>
> + phys:
> + maxItems: 4
> +
> + phy-names:
> + items:
> + - const: csiphy0
> + - const: csiphy1
> + - const: csiphy2
> + - const: csiphy4
> +
needn't "phy" for phy-names, just csi0|csi1 ...
> ports:
> $ref: /schemas/graph.yaml#/properties/ports
>
> @@ -158,6 +176,14 @@ properties:
> required:
> - data-lanes
>
> +patternProperties:
> + "^phy@[0-9a-f]+$":
Not sure why phy need be children node? does this phy is used by other
controller?
Frank
> + $ref: /schemas/phy/qcom,x1e80100-csi2-phy.yaml
> + unevaluatedProperties: false
> +
> + "^opp-table(-.*)?$":
> + type: object
> +
> required:
> - compatible
> - reg
> @@ -171,8 +197,6 @@ required:
> - iommus
> - power-domains
> - power-domain-names
> - - vdd-csiphy-0p8-supply
> - - vdd-csiphy-1p2-supply
> - ports
>
> additionalProperties: false
> @@ -184,6 +208,7 @@ examples:
> #include <dt-bindings/clock/qcom,x1e80100-camcc.h>
> #include <dt-bindings/interconnect/qcom,icc.h>
> #include <dt-bindings/interconnect/qcom,x1e80100-rpmh.h>
> + #include <dt-bindings/phy/phy.h>
> #include <dt-bindings/power/qcom-rpmpd.h>
>
> soc {
> @@ -229,6 +254,10 @@ examples:
> "vfe_lite0",
> "vfe_lite1";
>
> + #address-cells = <2>;
> + #size-cells = <2>;
> + ranges;
> +
> clocks = <&camcc CAM_CC_CAMNOC_AXI_NRT_CLK>,
> <&camcc CAM_CC_CAMNOC_AXI_RT_CLK>,
> <&camcc CAM_CC_CORE_AHB_CLK>,
>
> --
> 2.54.0
>
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v12 1/6] dt-bindings: media: qcom,x1e80100-camss: Add optional PHY handle definitions
2026-07-08 22:09 ` Frank Li
@ 2026-07-08 22:15 ` Bryan O'Donoghue
0 siblings, 0 replies; 20+ messages in thread
From: Bryan O'Donoghue @ 2026-07-08 22:15 UTC (permalink / raw)
To: Frank Li, Bryan O'Donoghue
Cc: Bjorn Andersson, Michael Turquette, Stephen Boyd, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Robert Foss, Todor Tomov,
Mauro Carvalho Chehab, Konrad Dybcio, Vladimir Zapolskiy,
Loic Poulain, linux-arm-msm, linux-clk, devicetree, linux-kernel,
linux-media, Krzysztof Kozlowski
On 08/07/2026 23:09, Frank Li wrote:
>> + phy-names:
>> + items:
>> + - const: csiphy0
>> + - const: csiphy1
>> + - const: csiphy2
>> + - const: csiphy4
>> +
> needn't "phy" for phy-names, just csi0|csi1 ...
yeah nice and now that I think about it, this is a more consistent
naming schema with other architectures.
thx.
---
bod
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v12 2/6] dt-bindings: media: qcom,x1e80100-camss: Describe iommu entries
2026-07-08 0:06 [PATCH v12 0/6] Add dt-bindings and PHY updates for CAMSS on x1e80100 silicon Bryan O'Donoghue
2026-07-08 0:06 ` [PATCH v12 1/6] dt-bindings: media: qcom,x1e80100-camss: Add optional PHY handle definitions Bryan O'Donoghue
@ 2026-07-08 0:06 ` Bryan O'Donoghue
2026-07-08 12:39 ` Loic Poulain
2026-07-08 15:15 ` Vikram Sharma
2026-07-08 0:06 ` [PATCH v12 3/6] media: qcom: camss: Add support to populate sub-devices Bryan O'Donoghue
` (3 subsequent siblings)
5 siblings, 2 replies; 20+ messages in thread
From: Bryan O'Donoghue @ 2026-07-08 0:06 UTC (permalink / raw)
To: Bjorn Andersson, Michael Turquette, Stephen Boyd, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Robert Foss, Todor Tomov,
Mauro Carvalho Chehab, Konrad Dybcio, Vladimir Zapolskiy,
Bryan O'Donoghue, Loic Poulain
Cc: linux-arm-msm, linux-clk, devicetree, linux-kernel, linux-media,
Bryan O'Donoghue, Krzysztof Kozlowski, Krzysztof Kozlowski
The original iommus list included entries for ICP and BPS/IPE S1
contexts. Only the five S1 HLOS stream IDs are required by the CAMSS
ISP hardware: IFE/IFE_LITE read and write, SFE read and write, and
CDM IFE. The remaining entries serve other hardware blocks which will
be described in their own nodes as support is added.
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@oss.qualcomm.com>
Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
---
.../bindings/media/qcom,x1e80100-camss.yaml | 26 ++++++++++++++++------
1 file changed, 19 insertions(+), 7 deletions(-)
diff --git a/Documentation/devicetree/bindings/media/qcom,x1e80100-camss.yaml b/Documentation/devicetree/bindings/media/qcom,x1e80100-camss.yaml
index c17b9757b2c86..c4454355f07ea 100644
--- a/Documentation/devicetree/bindings/media/qcom,x1e80100-camss.yaml
+++ b/Documentation/devicetree/bindings/media/qcom,x1e80100-camss.yaml
@@ -112,7 +112,22 @@ properties:
- const: sf_icp_mnoc
iommus:
- maxItems: 8
+ oneOf:
+ - items:
+ - description: S1 HLOS IFE and IFE_LITE non-protected read
+ - description: S1 HLOS IFE and IFE_LITE non-protected write
+ - description: S1 HLOS SFE non-protected read
+ - description: S1 HLOS SFE non-protected write
+ - description: S1 HLOS CDM IFE non-protected
+ - description: Legacy slot 0 - do not use
+ - description: Legacy slot 1 - do not use
+ - description: Legacy slot 2 - do not use
+ - items:
+ - description: S1 HLOS IFE and IFE_LITE non-protected read
+ - description: S1 HLOS IFE and IFE_LITE non-protected write
+ - description: S1 HLOS SFE non-protected read
+ - description: S1 HLOS SFE non-protected write
+ - description: S1 HLOS CDM IFE non-protected
power-domains:
items:
@@ -361,13 +376,10 @@ examples:
"sf_icp_mnoc";
iommus = <&apps_smmu 0x800 0x60>,
+ <&apps_smmu 0x820 0x60>,
+ <&apps_smmu 0x840 0x60>,
<&apps_smmu 0x860 0x60>,
- <&apps_smmu 0x1800 0x60>,
- <&apps_smmu 0x1860 0x60>,
- <&apps_smmu 0x18e0 0x00>,
- <&apps_smmu 0x1980 0x20>,
- <&apps_smmu 0x1900 0x00>,
- <&apps_smmu 0x19a0 0x20>;
+ <&apps_smmu 0x18a0 0x0>;
power-domains = <&camcc CAM_CC_IFE_0_GDSC>,
<&camcc CAM_CC_IFE_1_GDSC>,
--
2.54.0
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH v12 2/6] dt-bindings: media: qcom,x1e80100-camss: Describe iommu entries
2026-07-08 0:06 ` [PATCH v12 2/6] dt-bindings: media: qcom,x1e80100-camss: Describe iommu entries Bryan O'Donoghue
@ 2026-07-08 12:39 ` Loic Poulain
2026-07-08 15:15 ` Vikram Sharma
1 sibling, 0 replies; 20+ messages in thread
From: Loic Poulain @ 2026-07-08 12:39 UTC (permalink / raw)
To: Bryan O'Donoghue
Cc: Bjorn Andersson, Michael Turquette, Stephen Boyd, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Robert Foss, Todor Tomov,
Mauro Carvalho Chehab, Konrad Dybcio, Vladimir Zapolskiy,
Bryan O'Donoghue, linux-arm-msm, linux-clk, devicetree,
linux-kernel, linux-media, Krzysztof Kozlowski,
Krzysztof Kozlowski
On Wed, Jul 8, 2026 at 2:07 AM Bryan O'Donoghue
<bryan.odonoghue@linaro.org> wrote:
>
> The original iommus list included entries for ICP and BPS/IPE S1
> contexts. Only the five S1 HLOS stream IDs are required by the CAMSS
> ISP hardware: IFE/IFE_LITE read and write, SFE read and write, and
> CDM IFE. The remaining entries serve other hardware blocks which will
> be described in their own nodes as support is added.
>
> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@oss.qualcomm.com>
> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
Reviewed-by: Loic Poulain <loic.poulain@oss.qualcomm.com>
> ---
> .../bindings/media/qcom,x1e80100-camss.yaml | 26 ++++++++++++++++------
> 1 file changed, 19 insertions(+), 7 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/media/qcom,x1e80100-camss.yaml b/Documentation/devicetree/bindings/media/qcom,x1e80100-camss.yaml
> index c17b9757b2c86..c4454355f07ea 100644
> --- a/Documentation/devicetree/bindings/media/qcom,x1e80100-camss.yaml
> +++ b/Documentation/devicetree/bindings/media/qcom,x1e80100-camss.yaml
> @@ -112,7 +112,22 @@ properties:
> - const: sf_icp_mnoc
>
> iommus:
> - maxItems: 8
> + oneOf:
> + - items:
> + - description: S1 HLOS IFE and IFE_LITE non-protected read
> + - description: S1 HLOS IFE and IFE_LITE non-protected write
> + - description: S1 HLOS SFE non-protected read
> + - description: S1 HLOS SFE non-protected write
> + - description: S1 HLOS CDM IFE non-protected
> + - description: Legacy slot 0 - do not use
> + - description: Legacy slot 1 - do not use
> + - description: Legacy slot 2 - do not use
> + - items:
> + - description: S1 HLOS IFE and IFE_LITE non-protected read
> + - description: S1 HLOS IFE and IFE_LITE non-protected write
> + - description: S1 HLOS SFE non-protected read
> + - description: S1 HLOS SFE non-protected write
> + - description: S1 HLOS CDM IFE non-protected
>
> power-domains:
> items:
> @@ -361,13 +376,10 @@ examples:
> "sf_icp_mnoc";
>
> iommus = <&apps_smmu 0x800 0x60>,
> + <&apps_smmu 0x820 0x60>,
> + <&apps_smmu 0x840 0x60>,
> <&apps_smmu 0x860 0x60>,
> - <&apps_smmu 0x1800 0x60>,
> - <&apps_smmu 0x1860 0x60>,
> - <&apps_smmu 0x18e0 0x00>,
> - <&apps_smmu 0x1980 0x20>,
> - <&apps_smmu 0x1900 0x00>,
> - <&apps_smmu 0x19a0 0x20>;
> + <&apps_smmu 0x18a0 0x0>;
>
> power-domains = <&camcc CAM_CC_IFE_0_GDSC>,
> <&camcc CAM_CC_IFE_1_GDSC>,
>
> --
> 2.54.0
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v12 2/6] dt-bindings: media: qcom,x1e80100-camss: Describe iommu entries
2026-07-08 0:06 ` [PATCH v12 2/6] dt-bindings: media: qcom,x1e80100-camss: Describe iommu entries Bryan O'Donoghue
2026-07-08 12:39 ` Loic Poulain
@ 2026-07-08 15:15 ` Vikram Sharma
2026-07-08 21:32 ` Bryan O'Donoghue
1 sibling, 1 reply; 20+ messages in thread
From: Vikram Sharma @ 2026-07-08 15:15 UTC (permalink / raw)
To: Bryan O'Donoghue, Bjorn Andersson, Michael Turquette,
Stephen Boyd, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Robert Foss, Todor Tomov, Mauro Carvalho Chehab, Konrad Dybcio,
Vladimir Zapolskiy, Bryan O'Donoghue, Loic Poulain
Cc: linux-arm-msm, linux-clk, devicetree, linux-kernel, linux-media,
Krzysztof Kozlowski, Krzysztof Kozlowski
On 7/8/2026 5:36 AM, Bryan O'Donoghue wrote:
> iommus = <&apps_smmu 0x800 0x60>,
> + <&apps_smmu 0x820 0x60>,
> + <&apps_smmu 0x840 0x60>,
> <&apps_smmu 0x860 0x60>,
I think only "iommus = <&apps_smmu 0x800 0x60>" should be enough as mask which 0x60 is implicitly covering sids like 0x820, 0x840 and 0x860.
Regards,
Vikram
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v12 2/6] dt-bindings: media: qcom,x1e80100-camss: Describe iommu entries
2026-07-08 15:15 ` Vikram Sharma
@ 2026-07-08 21:32 ` Bryan O'Donoghue
0 siblings, 0 replies; 20+ messages in thread
From: Bryan O'Donoghue @ 2026-07-08 21:32 UTC (permalink / raw)
To: Vikram Sharma, Bryan O'Donoghue, Bjorn Andersson,
Michael Turquette, Stephen Boyd, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Robert Foss, Todor Tomov, Mauro Carvalho Chehab,
Konrad Dybcio, Vladimir Zapolskiy, Loic Poulain
Cc: linux-arm-msm, linux-clk, devicetree, linux-kernel, linux-media,
Krzysztof Kozlowski, Krzysztof Kozlowski
On 08/07/2026 16:15, Vikram Sharma wrote:
>
> On 7/8/2026 5:36 AM, Bryan O'Donoghue wrote:
>> iommus = <&apps_smmu 0x800 0x60>,
>> + <&apps_smmu 0x820 0x60>,
>> + <&apps_smmu 0x840 0x60>,
>> <&apps_smmu 0x860 0x60>,
>
> I think only "iommus = <&apps_smmu 0x800 0x60>" should be enough as mask which 0x60 is implicitly covering sids like 0x820, 0x840 and 0x860.
>
> Regards,
> Vikram
>
These all come from the Hamoa iommu defintions in qcom docs.
Can you double check for yourself and let me know.
---
bod
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v12 3/6] media: qcom: camss: Add support to populate sub-devices
2026-07-08 0:06 [PATCH v12 0/6] Add dt-bindings and PHY updates for CAMSS on x1e80100 silicon Bryan O'Donoghue
2026-07-08 0:06 ` [PATCH v12 1/6] dt-bindings: media: qcom,x1e80100-camss: Add optional PHY handle definitions Bryan O'Donoghue
2026-07-08 0:06 ` [PATCH v12 2/6] dt-bindings: media: qcom,x1e80100-camss: Describe iommu entries Bryan O'Donoghue
@ 2026-07-08 0:06 ` Bryan O'Donoghue
2026-07-08 0:18 ` sashiko-bot
2026-07-08 0:06 ` [PATCH v12 4/6] media: qcom: camss: Add legacy_phy flag to SoC definition structures Bryan O'Donoghue
` (2 subsequent siblings)
5 siblings, 1 reply; 20+ messages in thread
From: Bryan O'Donoghue @ 2026-07-08 0:06 UTC (permalink / raw)
To: Bjorn Andersson, Michael Turquette, Stephen Boyd, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Robert Foss, Todor Tomov,
Mauro Carvalho Chehab, Konrad Dybcio, Vladimir Zapolskiy,
Bryan O'Donoghue, Loic Poulain
Cc: linux-arm-msm, linux-clk, devicetree, linux-kernel, linux-media,
Bryan O'Donoghue, Krzysztof Kozlowski
Use devm_of_platform_populate() to populate subs in the tree.
Reviewed-by: Loic Poulain <loic.poulain@oss.qualcomm.com>
Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
---
drivers/media/platform/qcom/camss/camss.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c
index 2123f6388e3d7..95e655a8b6aa0 100644
--- a/drivers/media/platform/qcom/camss/camss.c
+++ b/drivers/media/platform/qcom/camss/camss.c
@@ -16,6 +16,7 @@
#include <linux/of.h>
#include <linux/of_device.h>
#include <linux/of_graph.h>
+#include <linux/of_platform.h>
#include <linux/pm_runtime.h>
#include <linux/pm_domain.h>
#include <linux/slab.h>
@@ -5362,6 +5363,8 @@ static int camss_probe(struct platform_device *pdev)
if (!camss)
return -ENOMEM;
+ devm_of_platform_populate(dev);
+
camss->res = of_device_get_match_data(dev);
atomic_set(&camss->ref_count, 0);
--
2.54.0
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH v12 3/6] media: qcom: camss: Add support to populate sub-devices
2026-07-08 0:06 ` [PATCH v12 3/6] media: qcom: camss: Add support to populate sub-devices Bryan O'Donoghue
@ 2026-07-08 0:18 ` sashiko-bot
0 siblings, 0 replies; 20+ messages in thread
From: sashiko-bot @ 2026-07-08 0:18 UTC (permalink / raw)
To: Bryan O'Donoghue; +Cc: conor+dt, devicetree, robh
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
New issues:
- [Medium] The return value of `devm_of_platform_populate()` is ignored, leading to silent initialization failures and potential permanent resource leaks.
Pre-existing issues:
- [High] Missing NULL check for the result of `of_device_get_match_data()` leads to a potential NULL pointer dereference.
--
--- Patch [3]: [PATCH v12 3/6] media: qcom: camss: Add support to populate sub-devices ---
commit 187ecc190c79a7e3479de2344bee652ac9f2b71f
Author: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
media: qcom: camss: Add support to populate sub-devices
This commit updates the Qualcomm Camera Subsystem (CAMSS) probe function
to use devm_of_platform_populate() for populating sub-devices in the tree.
> diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c
> index 2123f6388e3d7..95e655a8b6aa0 100644
> --- a/drivers/media/platform/qcom/camss/camss.c
> +++ b/drivers/media/platform/qcom/camss/camss.c
[ ... ]
> @@ -5362,6 +5363,8 @@ static int camss_probe(struct platform_device *pdev)
> if (!camss)
> return -ENOMEM;
>
> + devm_of_platform_populate(dev);
[Severity: Medium]
Should the return value of devm_of_platform_populate() be checked here?
When this function fails, it returns a negative error code and the devres
framework frees the associated devres action rather than attaching it to the
parent device.
If the population fails midway, could this leave successfully created child
devices dangling, resulting in a permanent resource leak upon device unbind?
> +
> camss->res = of_device_get_match_data(dev);
[Severity: High]
This is a pre-existing issue, but is it possible for
of_device_get_match_data() to return NULL here?
If the driver is bound via sysfs to a platform device that lacks an OF node
(for instance, one manually created and bound by root),
of_device_get_match_data() will return NULL.
Would this lead to a NULL pointer dereference shortly after this line when
accessing camss->res->csiphy_num during the csiphy allocation?
>
> atomic_set(&camss->ref_count, 0);
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260708-b4-linux-next-25-03-13-dtsi-x1e80100-camss-v12-0-f8588da41f16@linaro.org?part=3
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v12 4/6] media: qcom: camss: Add legacy_phy flag to SoC definition structures
2026-07-08 0:06 [PATCH v12 0/6] Add dt-bindings and PHY updates for CAMSS on x1e80100 silicon Bryan O'Donoghue
` (2 preceding siblings ...)
2026-07-08 0:06 ` [PATCH v12 3/6] media: qcom: camss: Add support to populate sub-devices Bryan O'Donoghue
@ 2026-07-08 0:06 ` Bryan O'Donoghue
2026-07-08 0:14 ` sashiko-bot
2026-07-08 0:06 ` [PATCH v12 5/6] media: qcom: camss: Add support for PHY API devices Bryan O'Donoghue
2026-07-08 0:07 ` [PATCH v12 6/6] media: qcom: camss: Drop legacy PHY descriptions from x1e Bryan O'Donoghue
5 siblings, 1 reply; 20+ messages in thread
From: Bryan O'Donoghue @ 2026-07-08 0:06 UTC (permalink / raw)
To: Bjorn Andersson, Michael Turquette, Stephen Boyd, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Robert Foss, Todor Tomov,
Mauro Carvalho Chehab, Konrad Dybcio, Vladimir Zapolskiy,
Bryan O'Donoghue, Loic Poulain
Cc: linux-arm-msm, linux-clk, devicetree, linux-kernel, linux-media,
Bryan O'Donoghue, Krzysztof Kozlowski
Flag which SoCs have legacy - builtin PHY code. This will be useful in
subsequent patches to inform PHY bringup logic if legacy bindings are
available.
Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
---
drivers/media/platform/qcom/camss/camss.c | 17 +++++++++++++++++
drivers/media/platform/qcom/camss/camss.h | 1 +
2 files changed, 18 insertions(+)
diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c
index 95e655a8b6aa0..e814a96953b1b 100644
--- a/drivers/media/platform/qcom/camss/camss.c
+++ b/drivers/media/platform/qcom/camss/camss.c
@@ -5512,6 +5512,7 @@ static void camss_remove(struct platform_device *pdev)
static const struct camss_resources msm8916_resources = {
.version = CAMSS_8x16,
+ .legacy_phy = true,
.csiphy_res = csiphy_res_8x16,
.csid_res = csid_res_8x16,
.ispif_res = &ispif_res_8x16,
@@ -5523,6 +5524,7 @@ static const struct camss_resources msm8916_resources = {
static const struct camss_resources msm8939_resources = {
.version = CAMSS_8x39,
+ .legacy_phy = true,
.csiphy_res = csiphy_res_8x39,
.csid_res = csid_res_8x39,
.ispif_res = &ispif_res_8x39,
@@ -5534,6 +5536,7 @@ static const struct camss_resources msm8939_resources = {
static const struct camss_resources msm8953_resources = {
.version = CAMSS_8x53,
+ .legacy_phy = true,
.icc_res = icc_res_8x53,
.icc_path_num = ARRAY_SIZE(icc_res_8x53),
.csiphy_res = csiphy_res_8x96,
@@ -5547,6 +5550,7 @@ static const struct camss_resources msm8953_resources = {
static const struct camss_resources msm8996_resources = {
.version = CAMSS_8x96,
+ .legacy_phy = true,
.csiphy_res = csiphy_res_8x96,
.csid_res = csid_res_8x96,
.ispif_res = &ispif_res_8x96,
@@ -5558,6 +5562,7 @@ static const struct camss_resources msm8996_resources = {
static const struct camss_resources qcm2290_resources = {
.version = CAMSS_2290,
+ .legacy_phy = true,
.csiphy_res = csiphy_res_2290,
.csid_res = csid_res_2290,
.vfe_res = vfe_res_2290,
@@ -5571,6 +5576,7 @@ static const struct camss_resources qcm2290_resources = {
static const struct camss_resources qcs8300_resources = {
.version = CAMSS_8300,
.pd_name = "top",
+ .legacy_phy = true,
.csiphy_res = csiphy_res_8300,
.tpg_res = tpg_res_8775p,
.csid_res = csid_res_8775p,
@@ -5587,6 +5593,7 @@ static const struct camss_resources qcs8300_resources = {
static const struct camss_resources sa8775p_resources = {
.version = CAMSS_8775P,
.pd_name = "top",
+ .legacy_phy = true,
.csiphy_res = csiphy_res_8775p,
.tpg_res = tpg_res_8775p,
.csid_res = csid_res_8775p,
@@ -5602,6 +5609,7 @@ static const struct camss_resources sa8775p_resources = {
static const struct camss_resources sdm660_resources = {
.version = CAMSS_660,
+ .legacy_phy = true,
.csiphy_res = csiphy_res_660,
.csid_res = csid_res_660,
.ispif_res = &ispif_res_660,
@@ -5613,6 +5621,7 @@ static const struct camss_resources sdm660_resources = {
static const struct camss_resources sdm670_resources = {
.version = CAMSS_845,
+ .legacy_phy = true,
.csiphy_res = csiphy_res_670,
.csid_res = csid_res_670,
.vfe_res = vfe_res_670,
@@ -5624,6 +5633,7 @@ static const struct camss_resources sdm670_resources = {
static const struct camss_resources sdm845_resources = {
.version = CAMSS_845,
.pd_name = "top",
+ .legacy_phy = true,
.csiphy_res = csiphy_res_845,
.csid_res = csid_res_845,
.vfe_res = vfe_res_845,
@@ -5635,6 +5645,7 @@ static const struct camss_resources sdm845_resources = {
static const struct camss_resources sm6150_resources = {
.version = CAMSS_6150,
.pd_name = "top",
+ .legacy_phy = true,
.csiphy_res = csiphy_res_sm6150,
.csid_res = csid_res_sm6150,
.vfe_res = vfe_res_sm6150,
@@ -5661,6 +5672,7 @@ static const struct camss_resources sm6350_resources = {
static const struct camss_resources sm8250_resources = {
.version = CAMSS_8250,
.pd_name = "top",
+ .legacy_phy = true,
.csiphy_res = csiphy_res_8250,
.csid_res = csid_res_8250,
.vfe_res = vfe_res_8250,
@@ -5674,6 +5686,7 @@ static const struct camss_resources sm8250_resources = {
static const struct camss_resources sc8280xp_resources = {
.version = CAMSS_8280XP,
.pd_name = "top",
+ .legacy_phy = true,
.csiphy_res = csiphy_res_sc8280xp,
.csid_res = csid_res_sc8280xp,
.ispif_res = NULL,
@@ -5688,6 +5701,7 @@ static const struct camss_resources sc8280xp_resources = {
static const struct camss_resources sc7280_resources = {
.version = CAMSS_7280,
.pd_name = "top",
+ .legacy_phy = true,
.csiphy_res = csiphy_res_7280,
.csid_res = csid_res_7280,
.vfe_res = vfe_res_7280,
@@ -5701,6 +5715,7 @@ static const struct camss_resources sc7280_resources = {
static const struct camss_resources sm8550_resources = {
.version = CAMSS_8550,
.pd_name = "top",
+ .legacy_phy = true,
.csiphy_res = csiphy_res_8550,
.csid_res = csid_res_8550,
.vfe_res = vfe_res_8550,
@@ -5715,6 +5730,7 @@ static const struct camss_resources sm8550_resources = {
static const struct camss_resources sm8650_resources = {
.version = CAMSS_8650,
.pd_name = "top",
+ .legacy_phy = true,
.csiphy_res = csiphy_res_sm8650,
.csid_res = csid_res_sm8650,
.csid_wrapper_res = &csid_wrapper_res_sm8550,
@@ -5729,6 +5745,7 @@ static const struct camss_resources sm8650_resources = {
static const struct camss_resources x1e80100_resources = {
.version = CAMSS_X1E80100,
.pd_name = "top",
+ .legacy_phy = true,
.csiphy_res = csiphy_res_x1e80100,
.tpg_res = tpg_res_x1e80100,
.csid_res = csid_res_x1e80100,
diff --git a/drivers/media/platform/qcom/camss/camss.h b/drivers/media/platform/qcom/camss/camss.h
index 93d691c8ac63b..698694d3064ea 100644
--- a/drivers/media/platform/qcom/camss/camss.h
+++ b/drivers/media/platform/qcom/camss/camss.h
@@ -107,6 +107,7 @@ enum icc_count {
struct camss_resources {
enum camss_version version;
const char *pd_name;
+ const bool legacy_phy;
const struct camss_subdev_resources *csiphy_res;
const struct camss_subdev_resources *tpg_res;
const struct camss_subdev_resources *csid_res;
--
2.54.0
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH v12 4/6] media: qcom: camss: Add legacy_phy flag to SoC definition structures
2026-07-08 0:06 ` [PATCH v12 4/6] media: qcom: camss: Add legacy_phy flag to SoC definition structures Bryan O'Donoghue
@ 2026-07-08 0:14 ` sashiko-bot
2026-07-08 0:16 ` Bryan O'Donoghue
0 siblings, 1 reply; 20+ messages in thread
From: sashiko-bot @ 2026-07-08 0:14 UTC (permalink / raw)
To: Bryan O'Donoghue; +Cc: conor+dt, devicetree, robh
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Missing `.legacy_phy = true` flag initialization for the `sm6350_resources` SoC definition.
--
--- Patch [4]: [PATCH v12 4/6] media: qcom: camss: Add legacy_phy flag to SoC definition structures ---
commit 909874316ccb4262ad4b5121de9b3eda2618f46a
Author: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
media: qcom: camss: Add legacy_phy flag to SoC definition structures
This commit adds a legacy_phy flag to various SoC definition structures
to indicate which SoCs have builtin PHY code. This flag is intended to
inform PHY bringup logic in subsequent patches if legacy bindings are
available.
> diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c
> index 95e655a8b6aa0..e814a96953b1b 100644
> --- a/drivers/media/platform/qcom/camss/camss.c
> +++ b/drivers/media/platform/qcom/camss/camss.c
[ ... ]
> @@ -5661,6 +5672,7 @@ static const struct camss_resources sm6350_resources = {
[Severity: Medium]
Did we miss setting .legacy_phy = true for sm6350_resources?
Looking at the other SoC definitions in camss.c, they all receive the
.legacy_phy = true flag, but it appears sm6350_resources was skipped:
static const struct camss_resources sm6350_resources = {
.version = CAMSS_6350,
.pd_name = "top",
.csiphy_res = csiphy_res_sm6350,
.csid_res = csid_res_sm6350,
...
};
Will this cause SM6350 to incorrectly fall into the non-legacy path when
the PHY bringup logic is updated to rely on this flag?
> static const struct camss_resources sm8250_resources = {
> .version = CAMSS_8250,
> .pd_name = "top",
> + .legacy_phy = true,
> .csiphy_res = csiphy_res_8250,
> .csid_res = csid_res_8250,
> .vfe_res = vfe_res_8250,
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260708-b4-linux-next-25-03-13-dtsi-x1e80100-camss-v12-0-f8588da41f16@linaro.org?part=4
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v12 5/6] media: qcom: camss: Add support for PHY API devices
2026-07-08 0:06 [PATCH v12 0/6] Add dt-bindings and PHY updates for CAMSS on x1e80100 silicon Bryan O'Donoghue
` (3 preceding siblings ...)
2026-07-08 0:06 ` [PATCH v12 4/6] media: qcom: camss: Add legacy_phy flag to SoC definition structures Bryan O'Donoghue
@ 2026-07-08 0:06 ` Bryan O'Donoghue
2026-07-08 0:19 ` sashiko-bot
2026-07-08 0:07 ` [PATCH v12 6/6] media: qcom: camss: Drop legacy PHY descriptions from x1e Bryan O'Donoghue
5 siblings, 1 reply; 20+ messages in thread
From: Bryan O'Donoghue @ 2026-07-08 0:06 UTC (permalink / raw)
To: Bjorn Andersson, Michael Turquette, Stephen Boyd, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Robert Foss, Todor Tomov,
Mauro Carvalho Chehab, Konrad Dybcio, Vladimir Zapolskiy,
Bryan O'Donoghue, Loic Poulain
Cc: linux-arm-msm, linux-clk, devicetree, linux-kernel, linux-media,
Bryan O'Donoghue, Krzysztof Kozlowski
Add the ability to use a PHY pointer which interacts with the standard PHY
API.
In the first instance the code will try to use the new PHY interface. If no
PHYs are present in the DT then the legacy method will be attempted.
Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
---
drivers/media/platform/qcom/camss/Kconfig | 1 +
drivers/media/platform/qcom/camss/camss-csiphy.c | 179 +++++++++++++++++++++--
drivers/media/platform/qcom/camss/camss-csiphy.h | 7 +
drivers/media/platform/qcom/camss/camss.c | 88 +++++++++--
4 files changed, 245 insertions(+), 30 deletions(-)
diff --git a/drivers/media/platform/qcom/camss/Kconfig b/drivers/media/platform/qcom/camss/Kconfig
index 4eda48cb1adf0..1edc5e5a1829e 100644
--- a/drivers/media/platform/qcom/camss/Kconfig
+++ b/drivers/media/platform/qcom/camss/Kconfig
@@ -7,3 +7,4 @@ config VIDEO_QCOM_CAMSS
select VIDEO_V4L2_SUBDEV_API
select VIDEOBUF2_DMA_SG
select V4L2_FWNODE
+ select PHY_QCOM_MIPI_CSI2
diff --git a/drivers/media/platform/qcom/camss/camss-csiphy.c b/drivers/media/platform/qcom/camss/camss-csiphy.c
index 539ac4888b608..b740edd0c67e6 100644
--- a/drivers/media/platform/qcom/camss/camss-csiphy.c
+++ b/drivers/media/platform/qcom/camss/camss-csiphy.c
@@ -7,12 +7,14 @@
* Copyright (c) 2011-2015, The Linux Foundation. All rights reserved.
* Copyright (C) 2016-2018 Linaro Ltd.
*/
+#include <dt-bindings/phy/phy.h>
#include <linux/clk.h>
#include <linux/delay.h>
#include <linux/interrupt.h>
#include <linux/io.h>
#include <linux/kernel.h>
#include <linux/of.h>
+#include <linux/phy/phy.h>
#include <linux/platform_device.h>
#include <linux/pm_runtime.h>
#include <media/media-entity.h>
@@ -131,10 +133,10 @@ static u8 csiphy_get_bpp(const struct csiphy_format_info *formats,
}
/*
- * csiphy_set_clock_rates - Calculate and set clock rates on CSIPHY module
+ * csiphy_set_clock_rates_legacy - Calculate and set clock rates on CSIPHY module
* @csiphy: CSIPHY device
*/
-static int csiphy_set_clock_rates(struct csiphy_device *csiphy)
+static int csiphy_set_clock_rates_legacy(struct csiphy_device *csiphy)
{
struct device *dev = csiphy->camss->dev;
s64 link_freq;
@@ -200,7 +202,7 @@ static int csiphy_set_clock_rates(struct csiphy_device *csiphy)
*
* Return 0 on success or a negative error code otherwise
*/
-static int csiphy_set_power(struct v4l2_subdev *sd, int on)
+static int csiphy_set_power_legacy(struct v4l2_subdev *sd, int on)
{
struct csiphy_device *csiphy = v4l2_get_subdevdata(sd);
struct device *dev = csiphy->camss->dev;
@@ -219,7 +221,7 @@ static int csiphy_set_power(struct v4l2_subdev *sd, int on)
return ret;
}
- ret = csiphy_set_clock_rates(csiphy);
+ ret = csiphy_set_clock_rates_legacy(csiphy);
if (ret < 0) {
regulator_bulk_disable(csiphy->num_supplies,
csiphy->supplies);
@@ -254,7 +256,7 @@ static int csiphy_set_power(struct v4l2_subdev *sd, int on)
}
/*
- * csiphy_stream_on - Enable streaming on CSIPHY module
+ * csiphy_stream_on_legacy - Enable streaming on CSIPHY module
* @csiphy: CSIPHY device
*
* Helper function to enable streaming on CSIPHY module.
@@ -262,7 +264,7 @@ static int csiphy_set_power(struct v4l2_subdev *sd, int on)
*
* Return 0 on success or a negative error code otherwise
*/
-static int csiphy_stream_on(struct csiphy_device *csiphy)
+static int csiphy_stream_on_legacy(struct csiphy_device *csiphy)
{
struct csiphy_config *cfg = &csiphy->cfg;
s64 link_freq;
@@ -306,11 +308,89 @@ static int csiphy_stream_on(struct csiphy_device *csiphy)
*
* Helper function to disable streaming on CSIPHY module
*/
-static void csiphy_stream_off(struct csiphy_device *csiphy)
+static void csiphy_stream_off_legacy(struct csiphy_device *csiphy)
{
csiphy->res->hw_ops->lanes_disable(csiphy, &csiphy->cfg);
}
+/*
+ * csiphy_stream_on - Enable streaming on CSIPHY module
+ * @csiphy: CSIPHY device
+ *
+ * Helper function to enable streaming on CSIPHY module.
+ * Main configuration of CSIPHY module is also done here.
+ *
+ * Return 0 on success or a negative error code otherwise
+ */
+static int csiphy_stream_on(struct csiphy_device *csiphy)
+{
+ u8 bpp = csiphy_get_bpp(csiphy->res->formats->formats, csiphy->res->formats->nformats,
+ csiphy->fmt[MSM_CSIPHY_PAD_SINK].code);
+ struct csiphy_lanes_cfg *lncfg = &csiphy->cfg.csi2->lane_cfg;
+ struct phy_configure_opts_mipi_dphy *dphy_cfg;
+ union phy_configure_opts dphy_opts = { 0 };
+ struct device *dev = csiphy->camss->dev;
+ u8 num_lanes = lncfg->num_data;
+ s64 link_freq;
+ int i;
+ int ret;
+
+ dphy_cfg = &dphy_opts.mipi_dphy;
+
+ link_freq = camss_get_link_freq(&csiphy->subdev.entity, bpp, num_lanes);
+
+ if (link_freq < 0) {
+ dev_err(dev,
+ "Cannot get CSI2 transmitter's link frequency\n");
+ return -EINVAL;
+ }
+
+ phy_mipi_dphy_get_default_config_for_hsclk(link_freq, num_lanes, dphy_cfg);
+
+ phy_set_mode(csiphy->phy, PHY_MODE_MIPI_DPHY);
+
+ ret = phy_configure(csiphy->phy, &dphy_opts);
+ if (ret) {
+ dev_err(dev, "failed to configure MIPI D-PHY\n");
+ goto error;
+ }
+
+ return phy_power_on(csiphy->phy);
+
+error:
+ return ret;
+}
+
+/*
+ * csiphy_stream_off - Disable streaming on CSIPHY module
+ * @csiphy: CSIPHY device
+ *
+ * Helper function to disable streaming on CSIPHY module
+ */
+static void csiphy_stream_off(struct csiphy_device *csiphy)
+{
+ phy_power_off(csiphy->phy);
+}
+
+/*
+ * csiphy_set_stream - Enable/disable streaming on CSIPHY module
+ * @sd: CSIPHY V4L2 subdevice
+ * @enable: Requested streaming state
+ *
+ * Return 0 on success or a negative error code otherwise
+ */
+static int csiphy_set_stream_legacy(struct v4l2_subdev *sd, int enable)
+{
+ struct csiphy_device *csiphy = v4l2_get_subdevdata(sd);
+ int ret = 0;
+
+ if (enable)
+ ret = csiphy_stream_on_legacy(csiphy);
+ else
+ csiphy_stream_off_legacy(csiphy);
+
+ return ret;
+}
/*
* csiphy_set_stream - Enable/disable streaming on CSIPHY module
@@ -572,16 +652,16 @@ csiphy_match_clock_name(const char *clock_name, const char *format, ...)
}
/*
- * msm_csiphy_subdev_init - Initialize CSIPHY device structure and resources
+ * msm_csiphy_subdev_init_legacy - Initialize CSIPHY device structure and resources
* @csiphy: CSIPHY device
* @res: CSIPHY module resources table
* @id: CSIPHY module id
*
* Return 0 on success or a negative error code otherwise
*/
-int msm_csiphy_subdev_init(struct camss *camss,
- struct csiphy_device *csiphy,
- const struct camss_subdev_resources *res, u8 id)
+int msm_csiphy_subdev_init_legacy(struct camss *camss,
+ struct csiphy_device *csiphy,
+ const struct camss_subdev_resources *res, u8 id)
{
struct device *dev = camss->dev;
struct platform_device *pdev = to_platform_device(dev);
@@ -709,6 +789,60 @@ int msm_csiphy_subdev_init(struct camss *camss,
return ret;
}
+/*
+ * msm_csiphy_subdev_init - Initialize CSIPHY device structure and resources
+ * @csiphy: CSIPHY device
+ * @res: CSIPHY module resources table
+ * @id: CSIPHY module id
+ *
+ * Return 0 on success or a negative error code otherwise
+ */
+int msm_csiphy_subdev_init(struct camss *camss,
+ struct csiphy_device *csiphy,
+ const struct camss_subdev_resources *res, u8 id)
+{
+ struct device *dev = camss->dev;
+ struct of_phandle_args args;
+ int idx;
+ int ret;
+
+ snprintf(csiphy->name, ARRAY_SIZE(csiphy->name), "csiphy%d", id);
+
+ idx = of_property_match_string(dev->of_node, "phy-names", csiphy->name);
+ if (idx < 0) {
+ dev_err(dev, "%s not found\n", csiphy->name);
+ return idx;
+ }
+
+ ret = of_parse_phandle_with_args(dev->of_node, "phys", "#phy-cells", idx, &args);
+ if (ret < 0) {
+ dev_err(dev, "unable to parse phys args %s\n", csiphy->name);
+ return ret;
+ }
+
+ if (!of_device_is_available(args.np))
+ goto put_np;
+
+ csiphy->phy = devm_phy_get(dev, csiphy->name);
+ if (IS_ERR(csiphy->phy)) {
+ ret = PTR_ERR(csiphy->phy);
+ goto put_np;
+ }
+
+ csiphy->camss = camss;
+ csiphy->id = id;
+ csiphy->res = &res->csiphy;
+
+ ret = phy_init(csiphy->phy);
+ if (ret)
+ dev_err(dev, "phy %s init fail %d\n", csiphy->name, ret);
+
+put_np:
+ of_node_put(args.np);
+
+ return ret;
+}
+
/*
* csiphy_link_setup - Setup CSIPHY connections
* @entity: Pointer to media entity structure
@@ -743,8 +877,12 @@ static int csiphy_link_setup(struct media_entity *entity,
return 0;
}
-static const struct v4l2_subdev_core_ops csiphy_core_ops = {
- .s_power = csiphy_set_power,
+static const struct v4l2_subdev_core_ops csiphy_core_ops_legacy = {
+ .s_power = csiphy_set_power_legacy,
+};
+
+static const struct v4l2_subdev_video_ops csiphy_video_ops_legacy = {
+ .s_stream = csiphy_set_stream_legacy,
};
static const struct v4l2_subdev_video_ops csiphy_video_ops = {
@@ -758,8 +896,13 @@ static const struct v4l2_subdev_pad_ops csiphy_pad_ops = {
.set_fmt = csiphy_set_format,
};
+static const struct v4l2_subdev_ops csiphy_v4l2_ops_legacy = {
+ .core = &csiphy_core_ops_legacy,
+ .video = &csiphy_video_ops_legacy,
+ .pad = &csiphy_pad_ops,
+};
+
static const struct v4l2_subdev_ops csiphy_v4l2_ops = {
- .core = &csiphy_core_ops,
.video = &csiphy_video_ops,
.pad = &csiphy_pad_ops,
};
@@ -788,7 +931,11 @@ int msm_csiphy_register_entity(struct csiphy_device *csiphy,
struct device *dev = csiphy->camss->dev;
int ret;
- v4l2_subdev_init(sd, &csiphy_v4l2_ops);
+ if (IS_ERR(csiphy->phy))
+ v4l2_subdev_init(sd, &csiphy_v4l2_ops_legacy);
+ else
+ v4l2_subdev_init(sd, &csiphy_v4l2_ops);
+
sd->internal_ops = &csiphy_v4l2_internal_ops;
sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
snprintf(sd->name, ARRAY_SIZE(sd->name), "%s%d",
@@ -828,6 +975,8 @@ int msm_csiphy_register_entity(struct csiphy_device *csiphy,
*/
void msm_csiphy_unregister_entity(struct csiphy_device *csiphy)
{
+ if (!IS_ERR(csiphy->phy))
+ phy_exit(csiphy->phy);
v4l2_device_unregister_subdev(&csiphy->subdev);
media_entity_cleanup(&csiphy->subdev.entity);
}
diff --git a/drivers/media/platform/qcom/camss/camss-csiphy.h b/drivers/media/platform/qcom/camss/camss-csiphy.h
index 9d9657b82f748..1879826034aac 100644
--- a/drivers/media/platform/qcom/camss/camss-csiphy.h
+++ b/drivers/media/platform/qcom/camss/camss-csiphy.h
@@ -12,6 +12,7 @@
#include <linux/clk.h>
#include <linux/interrupt.h>
+#include <linux/phy/phy.h>
#include <media/media-entity.h>
#include <media/v4l2-device.h>
#include <media/v4l2-mediabus.h>
@@ -97,6 +98,7 @@ struct csiphy_device_regs {
struct csiphy_device {
struct camss *camss;
+ struct phy *phy;
u8 id;
struct v4l2_subdev subdev;
struct media_pad pads[MSM_CSIPHY_PADS_NUM];
@@ -104,6 +106,7 @@ struct csiphy_device {
void __iomem *base_clk_mux;
u32 irq;
char irq_name[30];
+ char name[16];
struct camss_clock *clock;
bool *rate_set;
int nclocks;
@@ -118,6 +121,10 @@ struct csiphy_device {
struct camss_subdev_resources;
+int msm_csiphy_subdev_init_legacy(struct camss *camss,
+ struct csiphy_device *csiphy,
+ const struct camss_subdev_resources *res, u8 id);
+
int msm_csiphy_subdev_init(struct camss *camss,
struct csiphy_device *csiphy,
const struct camss_subdev_resources *res, u8 id);
diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c
index e814a96953b1b..146fea438486f 100644
--- a/drivers/media/platform/qcom/camss/camss.c
+++ b/drivers/media/platform/qcom/camss/camss.c
@@ -4794,14 +4794,51 @@ static int camss_parse_endpoint_node(struct device *dev,
static int camss_parse_ports(struct camss *camss)
{
struct device *dev = camss->dev;
+ const struct camss_resources *res = camss->res;
struct fwnode_handle *fwnode = dev_fwnode(dev), *ep;
int ret;
fwnode_graph_for_each_endpoint(fwnode, ep) {
struct camss_async_subdev *csd;
+ struct fwnode_handle *remote;
+
+ if (!fwnode_device_is_available(ep))
+ continue;
+
+ if (res->legacy_phy) {
+ csd = v4l2_async_nf_add_fwnode_remote(&camss->notifier, ep,
+ typeof(*csd));
+ } else {
+ struct fwnode_handle *phy_out, *phy_node, *phy_in, *sensor_ep;
+
+ phy_out = fwnode_graph_get_remote_endpoint(ep);
+ if (!phy_out)
+ continue;
+
+ phy_node = fwnode_graph_get_port_parent(phy_out);
+ fwnode_handle_put(phy_out);
+ if (!phy_node)
+ continue;
+
+ phy_in = fwnode_graph_get_endpoint_by_id(phy_node, 0, 0, 0);
+ fwnode_handle_put(phy_node);
+ if (!phy_in)
+ continue;
+
+ sensor_ep = fwnode_graph_get_remote_endpoint(phy_in);
+ fwnode_handle_put(phy_in);
+ if (!sensor_ep)
+ continue;
+
+ csd = v4l2_async_nf_add_fwnode(&camss->notifier, sensor_ep,
+ struct camss_async_subdev);
+ fwnode_handle_put(sensor_ep);
+ if (IS_ERR(csd)) {
+ ret = PTR_ERR(csd);
+ goto err_cleanup;
+ }
+ }
- csd = v4l2_async_nf_add_fwnode_remote(&camss->notifier, ep,
- typeof(*csd));
if (IS_ERR(csd)) {
ret = PTR_ERR(csd);
goto err_cleanup;
@@ -4833,15 +4870,26 @@ static int camss_init_subdevices(struct camss *camss)
unsigned int i;
int ret;
- for (i = 0; i < camss->res->csiphy_num; i++) {
- ret = msm_csiphy_subdev_init(camss, &camss->csiphy[i],
- &res->csiphy_res[i],
- res->csiphy_res[i].csiphy.id);
- if (ret < 0) {
- dev_err(camss->dev,
- "Failed to init csiphy%d sub-device: %d\n",
- i, ret);
- return ret;
+ if (!res->legacy_phy) {
+ for (i = 0; i < camss->res->csiphy_num; i++) {
+ ret = msm_csiphy_subdev_init(camss, &camss->csiphy[i],
+ &res->csiphy_res[i],
+ res->csiphy_res[i].csiphy.id);
+ if (ret < 0)
+ return ret;
+ }
+ } else {
+ for (i = 0; i < camss->res->csiphy_num; i++) {
+ ret = msm_csiphy_subdev_init_legacy(camss, &camss->csiphy[i],
+ &res->csiphy_res[i],
+ res->csiphy_res[i].csiphy.id);
+ if (ret < 0) {
+ dev_err(camss->dev,
+ "Failed to init csiphy%d sub-device: %d\n",
+ i, ret);
+ return ret;
+ }
+ camss->csiphy[i].phy = ERR_PTR(-ENODEV);
}
}
@@ -4931,6 +4979,9 @@ static int camss_link_entities(struct camss *camss)
for (i = 0; i < camss->res->csiphy_num; i++) {
for (j = 0; j < camss->res->csid_num; j++) {
+ if (!camss->csiphy[i].phy)
+ continue;
+
ret = media_create_pad_link(&camss->csiphy[i].subdev.entity,
MSM_CSIPHY_PAD_SRC,
&camss->csid[j].subdev.entity,
@@ -5057,6 +5108,9 @@ static int camss_register_entities(struct camss *camss)
int ret;
for (i = 0; i < camss->res->csiphy_num; i++) {
+ if (!camss->csiphy[i].phy)
+ continue;
+
ret = msm_csiphy_register_entity(&camss->csiphy[i],
&camss->v4l2_dev);
if (ret < 0) {
@@ -5132,8 +5186,10 @@ static int camss_register_entities(struct camss *camss)
i = camss->res->csiphy_num;
err_reg_csiphy:
- for (i--; i >= 0; i--)
- msm_csiphy_unregister_entity(&camss->csiphy[i]);
+ for (i--; i >= 0; i--) {
+ if (camss->csiphy[i].phy)
+ msm_csiphy_unregister_entity(&camss->csiphy[i]);
+ }
return ret;
}
@@ -5148,8 +5204,10 @@ static void camss_unregister_entities(struct camss *camss)
{
unsigned int i;
- for (i = 0; i < camss->res->csiphy_num; i++)
- msm_csiphy_unregister_entity(&camss->csiphy[i]);
+ for (i = 0; i < camss->res->csiphy_num; i++) {
+ if (camss->csiphy[i].phy)
+ msm_csiphy_unregister_entity(&camss->csiphy[i]);
+ }
if (camss->tpg) {
for (i = 0; i < camss->res->tpg_num; i++)
--
2.54.0
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH v12 5/6] media: qcom: camss: Add support for PHY API devices
2026-07-08 0:06 ` [PATCH v12 5/6] media: qcom: camss: Add support for PHY API devices Bryan O'Donoghue
@ 2026-07-08 0:19 ` sashiko-bot
0 siblings, 0 replies; 20+ messages in thread
From: sashiko-bot @ 2026-07-08 0:19 UTC (permalink / raw)
To: Bryan O'Donoghue; +Cc: devicetree, robh, conor+dt
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Missing `phy_exit()` cleanup on multiple probe error paths, leaking PHY initialization state.
- [High] Missing PHY node availability check in `camss_parse_ports()` causes a WARN_ON crash during link setup if a PHY is disabled in DT.
- [Medium] The commit unconditionally selects a non-existent Kconfig symbol `PHY_QCOM_MIPI_CSI2`, causing unmet dependencies and build failures.
- [Medium] Ignored return value from `phy_mipi_dphy_get_default_config_for_hsclk()` may lead to invalid PHY configuration.
--
--- Patch [5]: [PATCH v12 5/6] media: qcom: camss: Add support for PHY API devices ---
commit 6ca141cbb4ec0862b23ea5f00a1fb741065d4818
Author: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
media: qcom: camss: Add support for PHY API devices
Add the ability to use a PHY pointer which interacts with the standard PHY
API.
In the first instance the code will try to use the new PHY interface. If no
PHYs are present in the DT then the legacy method will be attempted.
> diff --git a/drivers/media/platform/qcom/camss/Kconfig b/drivers/media/platform/qcom/camss/Kconfig
> index 4eda48cb1adf0..1edc5e5a1829e 100644
> --- a/drivers/media/platform/qcom/camss/Kconfig
> +++ b/drivers/media/platform/qcom/camss/Kconfig
> @@ -7,3 +7,4 @@ config VIDEO_QCOM_CAMSS
> select VIDEO_V4L2_SUBDEV_API
> select VIDEOBUF2_DMA_SG
> select V4L2_FWNODE
> + select PHY_QCOM_MIPI_CSI2
[Severity: Medium]
Is PHY_QCOM_MIPI_CSI2 a valid configuration symbol in the kernel tree?
Selecting a non-existent symbol can lead to unmet dependency warnings or
build failures since the intended driver won't be enabled.
> diff --git a/drivers/media/platform/qcom/camss/camss-csiphy.c b/drivers/media/platform/qcom/camss/camss-csiphy.c
> index 539ac4888b608..b740edd0c67e6 100644
> --- a/drivers/media/platform/qcom/camss/camss-csiphy.c
> +++ b/drivers/media/platform/qcom/camss/camss-csiphy.c
[ ... ]
> @@ -306,11 +308,89 @@ static int csiphy_stream_on(struct csiphy_device *csiphy)
[ ... ]
> + phy_mipi_dphy_get_default_config_for_hsclk(link_freq, num_lanes, dphy_cfg);
> +
> + phy_set_mode(csiphy->phy, PHY_MODE_MIPI_DPHY);
> +
> + ret = phy_configure(csiphy->phy, &dphy_opts);
[Severity: Medium]
Should the return value of phy_mipi_dphy_get_default_config_for_hsclk() be
checked here?
If link_freq is zero or calculation fails, it returns a negative error code
and leaves dphy_opts unpopulated. Passing an uninitialized struct to
phy_configure() might cause invalid configurations or division-by-zero errors.
[ ... ]
> @@ -709,6 +789,60 @@ int msm_csiphy_subdev_init(struct camss *camss,
[ ... ]
> + ret = phy_init(csiphy->phy);
> + if (ret)
> + dev_err(dev, "phy %s init fail %d\n", csiphy->name, ret);
> +
> +put_np:
> + of_node_put(args.np);
> +
> + return ret;
> +}
[Severity: High]
Since phy_init() is called here without a devres wrapper, could this leak the
PHY initialization state if probe fails later?
For example, if camss_init_subdevices() fails after this point, the error
path jumps to err_genpd_cleanup and bypasses the camss_unregister_entities()
call needed to invoke phy_exit().
> diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c
> index e814a96953b1b..146fea438486f 100644
> --- a/drivers/media/platform/qcom/camss/camss.c
> +++ b/drivers/media/platform/qcom/camss/camss.c
> @@ -4794,14 +4794,51 @@ static int camss_parse_endpoint_node(struct device *dev,
> static int camss_parse_ports(struct camss *camss)
> {
> struct device *dev = camss->dev;
> + const struct camss_resources *res = camss->res;
> struct fwnode_handle *fwnode = dev_fwnode(dev), *ep;
> int ret;
>
> fwnode_graph_for_each_endpoint(fwnode, ep) {
> struct camss_async_subdev *csd;
> + struct fwnode_handle *remote;
> +
> + if (!fwnode_device_is_available(ep))
> + continue;
> +
> + if (res->legacy_phy) {
> + csd = v4l2_async_nf_add_fwnode_remote(&camss->notifier, ep,
> + typeof(*csd));
> + } else {
> + struct fwnode_handle *phy_out, *phy_node, *phy_in, *sensor_ep;
> +
> + phy_out = fwnode_graph_get_remote_endpoint(ep);
> + if (!phy_out)
> + continue;
> +
> + phy_node = fwnode_graph_get_port_parent(phy_out);
> + fwnode_handle_put(phy_out);
> + if (!phy_node)
> + continue;
> +
> + phy_in = fwnode_graph_get_endpoint_by_id(phy_node, 0, 0, 0);
[Severity: High]
Is it possible to encounter a crash during link setup if a PHY is disabled
in the device tree?
This code retrieves phy_node but does not check if it is available using
fwnode_device_is_available() before adding the remote sensor endpoint to
the notifier.
Later, msm_csiphy_subdev_init() correctly skips the disabled PHY, leaving
the csiphy entity uninitialized with 0 pads. When the sensor binds,
camss_subdev_notifier_complete() attempts to link the sensor to this
empty entity, which triggers a WARN_ON crash inside media_create_pad_link()
due to sink_pad >= sink->num_pads.
[ ... ]
> @@ -5132,8 +5186,10 @@ static int camss_register_entities(struct camss *camss)
>
> i = camss->res->csiphy_num;
> err_reg_csiphy:
> - for (i--; i >= 0; i--)
> - msm_csiphy_unregister_entity(&camss->csiphy[i]);
> + for (i--; i >= 0; i--) {
> + if (camss->csiphy[i].phy)
> + msm_csiphy_unregister_entity(&camss->csiphy[i]);
> + }
>
> return ret;
> }
[Severity: High]
If msm_csiphy_register_entity() fails at index i, does this rollback loop
miss cleaning up the PHY at index i?
The loop decrements i before the check, so it only cleans up indices
strictly less than the failing index, permanently leaking the initialized
PHY for the failing index itself.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260708-b4-linux-next-25-03-13-dtsi-x1e80100-camss-v12-0-f8588da41f16@linaro.org?part=5
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v12 6/6] media: qcom: camss: Drop legacy PHY descriptions from x1e
2026-07-08 0:06 [PATCH v12 0/6] Add dt-bindings and PHY updates for CAMSS on x1e80100 silicon Bryan O'Donoghue
` (4 preceding siblings ...)
2026-07-08 0:06 ` [PATCH v12 5/6] media: qcom: camss: Add support for PHY API devices Bryan O'Donoghue
@ 2026-07-08 0:07 ` Bryan O'Donoghue
2026-07-08 0:26 ` sashiko-bot
5 siblings, 1 reply; 20+ messages in thread
From: Bryan O'Donoghue @ 2026-07-08 0:07 UTC (permalink / raw)
To: Bjorn Andersson, Michael Turquette, Stephen Boyd, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Robert Foss, Todor Tomov,
Mauro Carvalho Chehab, Konrad Dybcio, Vladimir Zapolskiy,
Bryan O'Donoghue, Loic Poulain
Cc: linux-arm-msm, linux-clk, devicetree, linux-kernel, linux-media,
Bryan O'Donoghue, Krzysztof Kozlowski
x1e is the first CAMSS SoC to use the new PHY interface. Drop the redundant
legacy CSIPHY descriptions.
Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
---
drivers/media/platform/qcom/camss/camss.c | 37 -------------------------------
1 file changed, 37 deletions(-)
diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c
index 146fea438486f..f360b2c99bf89 100644
--- a/drivers/media/platform/qcom/camss/camss.c
+++ b/drivers/media/platform/qcom/camss/camss.c
@@ -4191,15 +4191,6 @@ static const struct resources_icc icc_res_sa8775p[] = {
static const struct camss_subdev_resources csiphy_res_x1e80100[] = {
/* CSIPHY0 */
{
- .regulators = {
- { .supply = "vdd-csiphy-0p8", .init_load_uA = 105000 },
- { .supply = "vdd-csiphy-1p2", .init_load_uA = 58900 }
- },
- .clock = { "csiphy0", "csiphy0_timer" },
- .clock_rate = { { 300000000, 400000000, 480000000 },
- { 266666667, 400000000 } },
- .reg = { "csiphy0" },
- .interrupt = { "csiphy0" },
.csiphy = {
.id = 0,
.hw_ops = &csiphy_ops_3ph_1_0,
@@ -4208,15 +4199,6 @@ static const struct camss_subdev_resources csiphy_res_x1e80100[] = {
},
/* CSIPHY1 */
{
- .regulators = {
- { .supply = "vdd-csiphy-0p8", .init_load_uA = 105000 },
- { .supply = "vdd-csiphy-1p2", .init_load_uA = 58900 }
- },
- .clock = { "csiphy1", "csiphy1_timer" },
- .clock_rate = { { 300000000, 400000000, 480000000 },
- { 266666667, 400000000 } },
- .reg = { "csiphy1" },
- .interrupt = { "csiphy1" },
.csiphy = {
.id = 1,
.hw_ops = &csiphy_ops_3ph_1_0,
@@ -4225,15 +4207,6 @@ static const struct camss_subdev_resources csiphy_res_x1e80100[] = {
},
/* CSIPHY2 */
{
- .regulators = {
- { .supply = "vdd-csiphy-0p8", .init_load_uA = 105000 },
- { .supply = "vdd-csiphy-1p2", .init_load_uA = 58900 }
- },
- .clock = { "csiphy2", "csiphy2_timer" },
- .clock_rate = { { 300000000, 400000000, 480000000 },
- { 266666667, 400000000 } },
- .reg = { "csiphy2" },
- .interrupt = { "csiphy2" },
.csiphy = {
.id = 2,
.hw_ops = &csiphy_ops_3ph_1_0,
@@ -4242,15 +4215,6 @@ static const struct camss_subdev_resources csiphy_res_x1e80100[] = {
},
/* CSIPHY4 */
{
- .regulators = {
- { .supply = "vdd-csiphy-0p8", .init_load_uA = 105000 },
- { .supply = "vdd-csiphy-1p2", .init_load_uA = 58900 }
- },
- .clock = { "csiphy4", "csiphy4_timer" },
- .clock_rate = { { 300000000, 400000000, 480000000 },
- { 266666667, 400000000 } },
- .reg = { "csiphy4" },
- .interrupt = { "csiphy4" },
.csiphy = {
.id = 4,
.hw_ops = &csiphy_ops_3ph_1_0,
@@ -5803,7 +5767,6 @@ static const struct camss_resources sm8650_resources = {
static const struct camss_resources x1e80100_resources = {
.version = CAMSS_X1E80100,
.pd_name = "top",
- .legacy_phy = true,
.csiphy_res = csiphy_res_x1e80100,
.tpg_res = tpg_res_x1e80100,
.csid_res = csid_res_x1e80100,
--
2.54.0
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH v12 6/6] media: qcom: camss: Drop legacy PHY descriptions from x1e
2026-07-08 0:07 ` [PATCH v12 6/6] media: qcom: camss: Drop legacy PHY descriptions from x1e Bryan O'Donoghue
@ 2026-07-08 0:26 ` sashiko-bot
0 siblings, 0 replies; 20+ messages in thread
From: sashiko-bot @ 2026-07-08 0:26 UTC (permalink / raw)
To: Bryan O'Donoghue; +Cc: conor+dt, robh, devicetree
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
New issues:
- [High] Missing `phy_exit()` cleanup on probe error paths results in PHY resource leaks for non-legacy PHYs.
Pre-existing issues:
- [High] Potential out-of-bounds access in `camss_subdev_notifier_bound()` due to non-contiguous hardware IDs.
--
--- Patch [6]: [PATCH v12 6/6] media: qcom: camss: Drop legacy PHY descriptions from x1e ---
commit 77049628030f3bcd1557363f75216c90c62c80af
Author: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
media: qcom: camss: Drop legacy PHY descriptions from x1e
x1e is the first CAMSS SoC to use the new PHY interface. This patch drops
the redundant legacy CSIPHY descriptions to enable the new interface.
> diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c
> index 146fea438486f..f360b2c99bf89 100644
> --- a/drivers/media/platform/qcom/camss/camss.c
> +++ b/drivers/media/platform/qcom/camss/camss.c
[ ... ]
> @@ -5803,7 +5767,6 @@ static const struct camss_resources sm8650_resources = {
> static const struct camss_resources x1e80100_resources = {
> .version = CAMSS_X1E80100,
> .pd_name = "top",
> - .legacy_phy = true,
[Severity: High]
Does dropping the legacy PHY interface introduce PHY resource leaks on probe
error paths?
If msm_csiphy_subdev_init() fails during camss_init_subdevices(), previously
initialized PHYs do not appear to be rolled back with phy_exit():
drivers/media/platform/qcom/camss/camss.c:camss_init_subdevices() {
...
ret = msm_csiphy_subdev_init(camss, &camss->csiphy[i],
&res->csiphy_res[i],
res->csiphy_res[i].csiphy.id);
if (ret < 0)
return ret;
...
}
Additionally, if camss_probe() fails after camss_init_subdevices() (for
instance, in dma_set_mask_and_coherent()), the error path jumps to
err_genpd_cleanup, bypassing camss_unregister_entities() and permanently
leaking all initialized PHYs:
drivers/media/platform/qcom/camss/camss.c:camss_probe() {
...
ret = dma_set_mask_and_coherent(dev, 0xffffffff);
if (ret)
goto err_genpd_cleanup;
...
}
Similarly, does camss_register_entities() fail to call phy_exit() for the
failed csiphy[i] and all subsequent PHYs, since the cleanup loop starts at
i - 1?
drivers/media/platform/qcom/camss/camss.c:camss_register_entities() {
...
err_reg_csiphy:
for (i--; i >= 0; i--) {
if (camss->csiphy[i].phy)
msm_csiphy_unregister_entity(&camss->csiphy[i]);
}
...
}
[Severity: High]
This is a pre-existing issue, but since x1e80100 defines non-contiguous PHY
IDs (0, 1, 2, and 4), does this cause an out-of-bounds access?
The csiphy_num is 4 for x1e80100, meaning the camss->csiphy array only has
4 elements. The endpoint port ID is parsed from the device tree and stored
in csd->interface.csiphy_id:
drivers/media/platform/qcom/camss/camss.c:camss_parse_endpoint_node() {
...
csd->interface.csiphy_id = vep.base.port;
...
}
If the port number aligns with the physical hardware ID (e.g., port@4 for
CSIPHY4), will this use id = 4 to index the 4-element array in
camss_subdev_notifier_bound(), causing an out-of-bounds access?
drivers/media/platform/qcom/camss/camss.c:camss_subdev_notifier_bound() {
...
u8 id = csd->interface.csiphy_id;
struct csiphy_device *csiphy = &camss->csiphy[id];
csiphy->cfg.csi2 = &csd->interface.csi2;
subdev->host_priv = csiphy;
...
}
> .csiphy_res = csiphy_res_x1e80100,
> .tpg_res = tpg_res_x1e80100,
> .csid_res = csid_res_x1e80100,
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260708-b4-linux-next-25-03-13-dtsi-x1e80100-camss-v12-0-f8588da41f16@linaro.org?part=6
^ permalink raw reply [flat|nested] 20+ messages in thread