diff mbox series

Revert "Revert "dt-bindings: i2c: qcom-cci: Document sc8280xp compatible""

Message ID 20240411085218.450237-1-vladimir.zapolskiy@linaro.org
State New
Headers show
Series Revert "Revert "dt-bindings: i2c: qcom-cci: Document sc8280xp compatible"" | expand

Commit Message

Vladimir Zapolskiy April 11, 2024, 8:52 a.m. UTC
This reverts commit 3e383dce513f426b7d79c0e6f8afe5d22a581f58.

The commit ae2a1f0f2cb5 ("dt-bindings: i2c: qcom-cci: Document sc8280xp compatible")
was correct apparently, it is required to describe the sc8280xp-cci
controller properly, as well it eliminates dtbs_check warnings.

Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>
---
 Documentation/devicetree/bindings/i2c/qcom,i2c-cci.yaml | 2 ++
 1 file changed, 2 insertions(+)

Comments

Krzysztof Kozlowski April 11, 2024, 10:18 a.m. UTC | #1
On 11/04/2024 12:16, Krzysztof Kozlowski wrote:
> On 11/04/2024 12:12, Bryan O'Donoghue wrote:
>> On 11/04/2024 10:36, Krzysztof Kozlowski wrote:
>>> On 11/04/2024 10:52, Vladimir Zapolskiy wrote:
>>>> This reverts commit 3e383dce513f426b7d79c0e6f8afe5d22a581f58.
>>>>
>>>> The commit ae2a1f0f2cb5 ("dt-bindings: i2c: qcom-cci: Document sc8280xp compatible")
>>>> was correct apparently, it is required to describe the sc8280xp-cci
>>>> controller properly, as well it eliminates dtbs_check warnings.
>>>>
>>>> Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>
>>>
>>> I am lost. Not on your patch, because it looks reasonable, but on entire
>>> history.
>>>
>>> Can anyone explain me why original commit was reverted?
>>>
>>> https://lore.kernel.org/all/767bc246-a0a0-4dad-badc-81ed50573832@linaro.org/
>>>
>>> Best regards,
>>> Krzysztof
>>>
>>
>> https://patchwork.ozlabs.org/project/devicetree-bindings/cover/20231006120159.3413789-1-bryan.odonoghue@linaro.org/#3195094
>>
>> We can you sm8250-cci instead, so dropped the additional compat.
> 
> I am sorry, but that links point to cover letter and actually the same
> thread as I linked. What does it prove?
> 

And just to remind because you bring some discussions from driver: we
talk here *only* about bindings patch. Not driver.

Best regards,
Krzysztof
Bryan O'Donoghue April 11, 2024, 10:24 a.m. UTC | #2
On 11/04/2024 11:18, Krzysztof Kozlowski wrote:
> On 11/04/2024 12:16, Krzysztof Kozlowski wrote:
>> On 11/04/2024 12:12, Bryan O'Donoghue wrote:
>>> On 11/04/2024 10:36, Krzysztof Kozlowski wrote:
>>>> On 11/04/2024 10:52, Vladimir Zapolskiy wrote:
>>>>> This reverts commit 3e383dce513f426b7d79c0e6f8afe5d22a581f58.
>>>>>
>>>>> The commit ae2a1f0f2cb5 ("dt-bindings: i2c: qcom-cci: Document sc8280xp compatible")
>>>>> was correct apparently, it is required to describe the sc8280xp-cci
>>>>> controller properly, as well it eliminates dtbs_check warnings.
>>>>>
>>>>> Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>
>>>>
>>>> I am lost. Not on your patch, because it looks reasonable, but on entire
>>>> history.
>>>>
>>>> Can anyone explain me why original commit was reverted?
>>>>
>>>> https://lore.kernel.org/all/767bc246-a0a0-4dad-badc-81ed50573832@linaro.org/
>>>>
>>>> Best regards,
>>>> Krzysztof
>>>>
>>>
>>> https://patchwork.ozlabs.org/project/devicetree-bindings/cover/20231006120159.3413789-1-bryan.odonoghue@linaro.org/#3195094
>>>
>>> We can you sm8250-cci instead, so dropped the additional compat.
>>
>> I am sorry, but that links point to cover letter and actually the same
>> thread as I linked. What does it prove?
>>
> 
> And just to remind because you bring some discussions from driver: we
> talk here *only* about bindings patch. Not driver.

https://patchwork.ozlabs.org/project/devicetree-bindings/cover/20231006120159.3413789-1-bryan.odonoghue@linaro.org/#3195327

Konrad pointed out we don't need a new compat because the sm8250 compat 
string could be reused.

So, I don't believe this revert should be reverted and I do believe Vlad 
needs his own compat string because his clock list isn't supported.

---
bod
Krzysztof Kozlowski April 11, 2024, 12:02 p.m. UTC | #3
On 11/04/2024 13:57, Bryan O'Donoghue wrote:
> On 11/04/2024 11:28, Krzysztof Kozlowski wrote:
>> On 11/04/2024 12:24, Bryan O'Donoghue wrote:
>>> On 11/04/2024 11:18, Krzysztof Kozlowski wrote:
>>>> On 11/04/2024 12:16, Krzysztof Kozlowski wrote:
>>>>> On 11/04/2024 12:12, Bryan O'Donoghue wrote:
>>>>>> On 11/04/2024 10:36, Krzysztof Kozlowski wrote:
>>>>>>> On 11/04/2024 10:52, Vladimir Zapolskiy wrote:
>>>>>>>> This reverts commit 3e383dce513f426b7d79c0e6f8afe5d22a581f58.
>>>>>>>>
>>>>>>>> The commit ae2a1f0f2cb5 ("dt-bindings: i2c: qcom-cci: Document sc8280xp compatible")
>>>>>>>> was correct apparently, it is required to describe the sc8280xp-cci
>>>>>>>> controller properly, as well it eliminates dtbs_check warnings.
>>>>>>>>
>>>>>>>> Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>
>>>>>>>
>>>>>>> I am lost. Not on your patch, because it looks reasonable, but on entire
>>>>>>> history.
>>>>>>>
>>>>>>> Can anyone explain me why original commit was reverted?
>>>>>>>
>>>>>>> https://lore.kernel.org/all/767bc246-a0a0-4dad-badc-81ed50573832@linaro.org/
>>>>>>>
>>>>>>> Best regards,
>>>>>>> Krzysztof
>>>>>>>
>>>>>>
>>>>>> https://patchwork.ozlabs.org/project/devicetree-bindings/cover/20231006120159.3413789-1-bryan.odonoghue@linaro.org/#3195094
>>>>>>
>>>>>> We can you sm8250-cci instead, so dropped the additional compat.
>>>>>
>>>>> I am sorry, but that links point to cover letter and actually the same
>>>>> thread as I linked. What does it prove?
>>>>>
>>>>
>>>> And just to remind because you bring some discussions from driver: we
>>>> talk here *only* about bindings patch. Not driver.
>>>
>>> https://patchwork.ozlabs.org/project/devicetree-bindings/cover/20231006120159.3413789-1-bryan.odonoghue@linaro.org/#3195327
>>>
>>> Konrad pointed out we don't need a new compat because the sm8250 compat
>>> string could be reused.
>>
>> Where did he point that? I see only comment about driver, not bindings.
>> Please point me to his comment (and again, not patchwork which gives you
>> entire discussion and no one knows to which comment you refer, but lore
>> link which leads to specific one email where Konrad said it).
> 
> Konrad made a comment about the compat string in the driver, I looked at 
> the yaml and realised I could reuse the compat string.

For the driver. The YAML was correct, wasn't it? It got reviewed and it
was about entirely different SoC, not sm8250. You cannot use sm8250 as
sc8280xp in the binding. These SoCs do not share anything.

> 
> Then asked for a reversion of the add.
> 
> I still think this is the right thing to do, no ?

So if IIUC, the misunderstanding was about the driver/binding.

Then no, binding should have been dropped.

Best regards,
Krzysztof
Bryan O'Donoghue April 11, 2024, 12:21 p.m. UTC | #4
On 11/04/2024 13:03, Krzysztof Kozlowski wrote:
> On 11/04/2024 10:52, Vladimir Zapolskiy wrote:
>> This reverts commit 3e383dce513f426b7d79c0e6f8afe5d22a581f58.
>>
>> The commit ae2a1f0f2cb5 ("dt-bindings: i2c: qcom-cci: Document sc8280xp compatible")
>> was correct apparently, it is required to describe the sc8280xp-cci
>> controller properly, as well it eliminates dtbs_check warnings.
>>
>> Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>
> 
> Subject:
> dt-bindings: i2c: Revert ....
> 
> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> 
> Best regards,
> Krzysztof

Reversion is not the right fix.

I'll send a proper fix asap.

cci0: cci@ac4a000 {
         compatible = "qcom,sc8280xp-cci", "qcom,msm8996-cci";

          clock-names = "camnoc_axi",
                        "slow_ahb_src",
                        "cpas_ahb",
                        "cci";

   - if:
       properties:
         compatible:
           contains:
             enum:
               - qcom,sc7280-cci
               - qcom,sc8280xp-cci-no-bueno
               - qcom,sm8250-cci
               - qcom,sm8450-cci
     then:
       properties:
         clocks:
           minItems: 5
           maxItems: 5
         clock-names:
           items:
             - const: camnoc_axi
             - const: slow_ahb_src
             - const: cpas_ahb
             - const: cci
             - const: cci_src

---
bod
Krzysztof Kozlowski April 11, 2024, 12:36 p.m. UTC | #5
On 11/04/2024 14:21, Bryan O'Donoghue wrote:
> On 11/04/2024 13:03, Krzysztof Kozlowski wrote:
>> On 11/04/2024 10:52, Vladimir Zapolskiy wrote:
>>> This reverts commit 3e383dce513f426b7d79c0e6f8afe5d22a581f58.
>>>
>>> The commit ae2a1f0f2cb5 ("dt-bindings: i2c: qcom-cci: Document sc8280xp compatible")
>>> was correct apparently, it is required to describe the sc8280xp-cci
>>> controller properly, as well it eliminates dtbs_check warnings.
>>>
>>> Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>
>>
>> Subject:
>> dt-bindings: i2c: Revert ....
>>
>> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>>
>> Best regards,
>> Krzysztof
> 
> Reversion is not the right fix.
> 
> I'll send a proper fix asap.
> 
> cci0: cci@ac4a000 {
>          compatible = "qcom,sc8280xp-cci", "qcom,msm8996-cci";
> 
>           clock-names = "camnoc_axi",
>                         "slow_ahb_src",
>                         "cpas_ahb",

So this patch was not tested?

Un-reviewed.... I complain a lot that people from various companies send
untested patches, but it seems Linaro does the same. That does not make
me happy :/

Best regards,
Krzysztof
Vladimir Zapolskiy April 11, 2024, 1:20 p.m. UTC | #6
Hi Bryan,

On 4/11/24 15:21, Bryan O'Donoghue wrote:
> On 11/04/2024 13:03, Krzysztof Kozlowski wrote:
>> On 11/04/2024 10:52, Vladimir Zapolskiy wrote:
>>> This reverts commit 3e383dce513f426b7d79c0e6f8afe5d22a581f58.
>>>
>>> The commit ae2a1f0f2cb5 ("dt-bindings: i2c: qcom-cci: Document sc8280xp compatible")
>>> was correct apparently, it is required to describe the sc8280xp-cci
>>> controller properly, as well it eliminates dtbs_check warnings.
>>>
>>> Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>
>>
>> Subject:
>> dt-bindings: i2c: Revert ....
>>
>> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>>
>> Best regards,
>> Krzysztof
> 
> Reversion is not the right fix.
> 
> I'll send a proper fix asap.

Thank you for review of this change related to "sc8280xp-cci" compatible
and the expressed desire to sort the mess out, I will be waiting for your
new change impatiently, since afterwards it will let me send my update
to the "sm8650-cci" related change.

--
Best wishes,
Vladimir
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/i2c/qcom,i2c-cci.yaml b/Documentation/devicetree/bindings/i2c/qcom,i2c-cci.yaml
index f0eabff86310..43af1c23ebe4 100644
--- a/Documentation/devicetree/bindings/i2c/qcom,i2c-cci.yaml
+++ b/Documentation/devicetree/bindings/i2c/qcom,i2c-cci.yaml
@@ -26,6 +26,7 @@  properties:
       - items:
           - enum:
               - qcom,sc7280-cci
+              - qcom,sc8280xp-cci
               - qcom,sdm845-cci
               - qcom,sm6350-cci
               - qcom,sm8250-cci
@@ -161,6 +162,7 @@  allOf:
           contains:
             enum:
               - qcom,sc7280-cci
+              - qcom,sc8280xp-cci
               - qcom,sm8250-cci
               - qcom,sm8450-cci
     then: