The first register bank should be the 'main' register bank, in this case
the CSID wrapper register is responsible for muxing PHY/TPG inputs directly
to CSID or to other blocks such as the Sensor Front End.

commit f4792eeaa971 ("dt-bindings: media: qcom,x1e80100-camss: Fix isp unit address")
assigned the address to the first register bank "csid0" whereas what we
should have done is retained the unit address and moved csid_wrapper to be
the first listed bank.

Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
---
 .../devicetree/bindings/media/qcom,x1e80100-camss.yaml       | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/Documentation/devicetree/bindings/media/qcom,x1e80100-camss.yaml b/Documentation/devicetree/bindings/media/qcom,x1e80100-camss.yaml
index b075341caafc1612e4faa3b7c1d0766e16646f7b..2438e08b894f4a3dc577cee4ab85184a3d7232b0 100644
--- a/Documentation/devicetree/bindings/media/qcom,x1e80100-camss.yaml
+++ b/Documentation/devicetree/bindings/media/qcom,x1e80100-camss.yaml
@@ -21,12 +21,12 @@ properties:
 
   reg-names:
     items:
+      - const: csid_wrapper
       - const: csid0
       - const: csid1
       - const: csid2
       - const: csid_lite0
       - const: csid_lite1
-      - const: csid_wrapper
       - const: csiphy0
       - const: csiphy1
       - const: csiphy2
@@ -190,15 +190,15 @@ examples:
         #address-cells = <2>;
         #size-cells = <2>;
 
-        camss: isp@acb7000 {
+        camss: isp@acb6000 {
             compatible = "qcom,x1e80100-camss";
 
-            reg = <0 0x0acb7000 0 0x2000>,
+            reg = <0 0x0acb6000 0 0x1000>,
+                  <0 0x0acb7000 0 0x2000>,
                   <0 0x0acb9000 0 0x2000>,
                   <0 0x0acbb000 0 0x2000>,
                   <0 0x0acc6000 0 0x1000>,
                   <0 0x0acca000 0 0x1000>,
-                  <0 0x0acb6000 0 0x1000>,
                   <0 0x0ace4000 0 0x1000>,
                   <0 0x0ace6000 0 0x1000>,
                   <0 0x0ace8000 0 0x1000>,
@@ -211,12 +211,12 @@ examples:
                   <0 0x0acc7000 0 0x2000>,
                   <0 0x0accb000 0 0x2000>;
 
-            reg-names = "csid0",
+            reg-names = "csid_wrapper",
+                        "csid0",
                         "csid1",
                         "csid2",
                         "csid_lite0",
                         "csid_lite1",
-                        "csid_wrapper",
                         "csiphy0",
                         "csiphy1",
                         "csiphy2",

-- 
2.49.0

On 11/07/2025 14:57, Bryan O'Donoghue wrote:
> The first register bank should be the 'main' register bank, in this case
> the CSID wrapper register is responsible for muxing PHY/TPG inputs directly
> to CSID or to other blocks such as the Sensor Front End.
> 
> commit f4792eeaa971 ("dt-bindings: media: qcom,x1e80100-camss: Fix isp unit address")

I have next from few days ago and I don't have this commit.

> assigned the address to the first register bank "csid0" whereas what we
> should have done is retained the unit address and moved csid_wrapper to be
> the first listed bank.

This is confusing. Did that commit change entries in the binding?


> 
> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> ---
>  .../devicetree/bindings/media/qcom,x1e80100-camss.yaml       | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/media/qcom,x1e80100-camss.yaml b/Documentation/devicetree/bindings/media/qcom,x1e80100-camss.yaml
> index b075341caafc1612e4faa3b7c1d0766e16646f7b..2438e08b894f4a3dc577cee4ab85184a3d7232b0 100644
> --- a/Documentation/devicetree/bindings/media/qcom,x1e80100-camss.yaml
> +++ b/Documentation/devicetree/bindings/media/qcom,x1e80100-camss.yaml
> @@ -21,12 +21,12 @@ properties:
>  
>    reg-names:
>      items:
> +      - const: csid_wrapper

Anyway, this is ABI break, so needs some sort of explanation in the
commit msg. We don't break ABI for cleanup reasons, unless it wasn't
released yet etc.

Best regards,
Krzysztof

On 13/07/2025 09:15, Krzysztof Kozlowski wrote:
> On 11/07/2025 14:57, Bryan O'Donoghue wrote:
>> The first register bank should be the 'main' register bank, in this case
>> the CSID wrapper register is responsible for muxing PHY/TPG inputs directly
>> to CSID or to other blocks such as the Sensor Front End.
>>
>> commit f4792eeaa971 ("dt-bindings: media: qcom,x1e80100-camss: Fix isp unit address")
> 
> I have next from few days ago and I don't have this commit.

https://gitlab.freedesktop.org/linux-media/media-committers/-/commit/1da245b6b73436be0d9936bb472f8a55900193cb

>> assigned the address to the first register bank "csid0" whereas what we
>> should have done is retained the unit address and moved csid_wrapper to be
>> the first listed bank.
> 
> This is confusing. Did that commit change entries in the binding?
Fixed the unit address.

What we _should_ have done is put csid_wrapper as the first entry.


> 
>>
>> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
>> ---
>>   .../devicetree/bindings/media/qcom,x1e80100-camss.yaml       | 12 ++++++------
>>   1 file changed, 6 insertions(+), 6 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/media/qcom,x1e80100-camss.yaml b/Documentation/devicetree/bindings/media/qcom,x1e80100-camss.yaml
>> index b075341caafc1612e4faa3b7c1d0766e16646f7b..2438e08b894f4a3dc577cee4ab85184a3d7232b0 100644
>> --- a/Documentation/devicetree/bindings/media/qcom,x1e80100-camss.yaml
>> +++ b/Documentation/devicetree/bindings/media/qcom,x1e80100-camss.yaml
>> @@ -21,12 +21,12 @@ properties:
>>   
>>     reg-names:
>>       items:
>> +      - const: csid_wrapper
> 
> Anyway, this is ABI break, so needs some sort of explanation in the
> commit msg. We don't break ABI for cleanup reasons, unless it wasn't
> released yet etc.
So I since we haven't added the node to a dts yet which to my 
understanding means no ABI break.

---
bod

On 13/07/2025 11:12, Bryan O'Donoghue wrote:
> On 13/07/2025 09:15, Krzysztof Kozlowski wrote:
>> On 11/07/2025 14:57, Bryan O'Donoghue wrote:
>>> The first register bank should be the 'main' register bank, in this case
>>> the CSID wrapper register is responsible for muxing PHY/TPG inputs directly
>>> to CSID or to other blocks such as the Sensor Front End.
>>>
>>> commit f4792eeaa971 ("dt-bindings: media: qcom,x1e80100-camss: Fix isp unit address")
>>
>> I have next from few days ago and I don't have this commit.
> 
> https://gitlab.freedesktop.org/linux-media/media-committers/-/commit/1da245b6b73436be0d9936bb472f8a55900193cb
> 
>>> assigned the address to the first register bank "csid0" whereas what we
>>> should have done is retained the unit address and moved csid_wrapper to be
>>> the first listed bank.
>>
>> This is confusing. Did that commit change entries in the binding?
> Fixed the unit address.
> 
> What we _should_ have done is put csid_wrapper as the first entry.

That's different problem then. The commit fixed only DTC warning and it
was perfectly fine from that point of view. I would not refer it,
because it just makes impression that commit was not correct or even
complete.

> 
> 
>>
>>>
>>> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
>>> ---
>>>   .../devicetree/bindings/media/qcom,x1e80100-camss.yaml       | 12 ++++++------
>>>   1 file changed, 6 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/media/qcom,x1e80100-camss.yaml b/Documentation/devicetree/bindings/media/qcom,x1e80100-camss.yaml
>>> index b075341caafc1612e4faa3b7c1d0766e16646f7b..2438e08b894f4a3dc577cee4ab85184a3d7232b0 100644
>>> --- a/Documentation/devicetree/bindings/media/qcom,x1e80100-camss.yaml
>>> +++ b/Documentation/devicetree/bindings/media/qcom,x1e80100-camss.yaml
>>> @@ -21,12 +21,12 @@ properties:
>>>   
>>>     reg-names:
>>>       items:
>>> +      - const: csid_wrapper
>>
>> Anyway, this is ABI break, so needs some sort of explanation in the
>> commit msg. We don't break ABI for cleanup reasons, unless it wasn't
>> released yet etc.
> So I since we haven't added the node to a dts yet which to my 
> understanding means no ABI break.

In-kernel DTS is one of the users, but not the only one. The kernel
drivers implement the ABI and for them your DTS does not matter. You
might not have DTS at all and still break the ABI. As mentioned in
second patch - the ABI, expressed by dt docs, once released in final
kernel version becomes the actual explicit ABI.

This is the one which you should not break.

Kernel drivers can sometimes imply ABI (e.g. undocumented one) and
that's another story.

Best regards,
Krzysztof

On Sun, Jul 13, 2025 at 11:34:18AM +0200, Krzysztof Kozlowski wrote:
> On 13/07/2025 11:12, Bryan O'Donoghue wrote:
> > On 13/07/2025 09:15, Krzysztof Kozlowski wrote:
> >> On 11/07/2025 14:57, Bryan O'Donoghue wrote:
> >>> The first register bank should be the 'main' register bank, in this case
> >>> the CSID wrapper register is responsible for muxing PHY/TPG inputs directly
> >>> to CSID or to other blocks such as the Sensor Front End.
> >>>
> >>> commit f4792eeaa971 ("dt-bindings: media: qcom,x1e80100-camss: Fix isp unit address")
> >>
> >> I have next from few days ago and I don't have this commit.
> > 
> > https://gitlab.freedesktop.org/linux-media/media-committers/-/commit/1da245b6b73436be0d9936bb472f8a55900193cb
> > 
> >>> assigned the address to the first register bank "csid0" whereas what we
> >>> should have done is retained the unit address and moved csid_wrapper to be
> >>> the first listed bank.
> >>
> >> This is confusing. Did that commit change entries in the binding?
> > Fixed the unit address.
> > 
> > What we _should_ have done is put csid_wrapper as the first entry.
> 
> That's different problem then. The commit fixed only DTC warning and it
> was perfectly fine from that point of view. I would not refer it,
> because it just makes impression that commit was not correct or even
> complete.

BTW, you have here also checkpatch warnings.

Best regards,
Krzysztof