[v3,3/9] dt-bindings: PCI: renesas,pci-rcar-gen2: 'depends-on' is no more optional

Message ID 20221207162435.1001782-4-herve.codina@bootlin.com
State New
Headers
Series Add the Renesas USBF controller support |

Commit Message

Herve Codina Dec. 7, 2022, 4:24 p.m. UTC
  The 'depends-on' property is set in involved DTS.

Move it to a required property.

Signed-off-by: Herve Codina <herve.codina@bootlin.com>
---
 Documentation/devicetree/bindings/pci/renesas,pci-rcar-gen2.yaml | 1 +
 1 file changed, 1 insertion(+)
  

Comments

Krzysztof Kozlowski Dec. 8, 2022, 8:26 a.m. UTC | #1
On 07/12/2022 17:24, Herve Codina wrote:
> The 'depends-on' property is set in involved DTS.
> 
> Move it to a required property.
> 
> Signed-off-by: Herve Codina <herve.codina@bootlin.com>
> ---
>  Documentation/devicetree/bindings/pci/renesas,pci-rcar-gen2.yaml | 1 +

This should be squashed with previous patch. There is no point to add
property and immediately in the next patch make it required. Remember
that bindings are separate from DTS.

Best regards,
Krzysztof
  
Herve Codina Dec. 8, 2022, 9:05 a.m. UTC | #2
Hi Krzysztof,

On Thu, 8 Dec 2022 09:26:41 +0100
Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:

> On 07/12/2022 17:24, Herve Codina wrote:
> > The 'depends-on' property is set in involved DTS.
> > 
> > Move it to a required property.
> > 
> > Signed-off-by: Herve Codina <herve.codina@bootlin.com>
> > ---
> >  Documentation/devicetree/bindings/pci/renesas,pci-rcar-gen2.yaml | 1 +  
> 
> This should be squashed with previous patch. There is no point to add
> property and immediately in the next patch make it required. Remember
> that bindings are separate from DTS.
> 
> Best regards,
> Krzysztof
> 

I though about make dtbs_check in case of git bisect.

But, ok I will squash or perhaps remove completely this commit.
It introduces a DT compatibility break adding a new mandatory
property (raised by Rob on cover letter review).
Is this compatibility break can be acceptable ?

Best regards,
Herve
  
Krzysztof Kozlowski Dec. 8, 2022, 9:46 a.m. UTC | #3
On 08/12/2022 10:05, Herve Codina wrote:
> Hi Krzysztof,
> 
> On Thu, 8 Dec 2022 09:26:41 +0100
> Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:
> 
>> On 07/12/2022 17:24, Herve Codina wrote:
>>> The 'depends-on' property is set in involved DTS.
>>>
>>> Move it to a required property.
>>>
>>> Signed-off-by: Herve Codina <herve.codina@bootlin.com>
>>> ---
>>>  Documentation/devicetree/bindings/pci/renesas,pci-rcar-gen2.yaml | 1 +  
>>
>> This should be squashed with previous patch. There is no point to add
>> property and immediately in the next patch make it required. Remember
>> that bindings are separate from DTS.
>>
>> Best regards,
>> Krzysztof
>>
> 
> I though about make dtbs_check in case of git bisect.

And what would this commit change? In Git you will have
1. dt-bindings: PCI: renesas,pci-rcar-gen2: Add depends-on for RZ/N1 SoC
family
2. dt-bindings: PCI: renesas,pci-rcar-gen2: 'depends-on' is no more optional

so what is the difference for git bisect?

> 
> But, ok I will squash or perhaps remove completely this commit.
> It introduces a DT compatibility break adding a new mandatory
> property (raised by Rob on cover letter review).
> Is this compatibility break can be acceptable ?

Requiring property in bindings as a fix for something which was broken
is ok. But this is independent of Linux drivers, which should not stop
working.

Best regards,
Krzysztof
  
Herve Codina Dec. 8, 2022, 3:51 p.m. UTC | #4
Hi Krzysztof,

On Thu, 8 Dec 2022 10:46:32 +0100
Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:

> On 08/12/2022 10:05, Herve Codina wrote:
> > Hi Krzysztof,
> > 
> > On Thu, 8 Dec 2022 09:26:41 +0100
> > Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:
> >   
> >> On 07/12/2022 17:24, Herve Codina wrote:  
> >>> The 'depends-on' property is set in involved DTS.
> >>>
> >>> Move it to a required property.
> >>>
> >>> Signed-off-by: Herve Codina <herve.codina@bootlin.com>
> >>> ---
> >>>  Documentation/devicetree/bindings/pci/renesas,pci-rcar-gen2.yaml | 1 +    
> >>
> >> This should be squashed with previous patch. There is no point to add
> >> property and immediately in the next patch make it required. Remember
> >> that bindings are separate from DTS.
> >>
> >> Best regards,
> >> Krzysztof
> >>  
> > 
> > I though about make dtbs_check in case of git bisect.  
> 
> And what would this commit change? In Git you will have
> 1. dt-bindings: PCI: renesas,pci-rcar-gen2: Add depends-on for RZ/N1 SoC
> family
> 2. dt-bindings: PCI: renesas,pci-rcar-gen2: 'depends-on' is no more optional
> 
> so what is the difference for git bisect?

Well, today, I have:
1. dt-bindings: Add depends-on
2. dts: Add depends-on
3. dt-bindings: Move depends-on to mandatory

If I squash dt-bindings commits, I am going to have:
  1. dt-bindings: Add mandatory depends-on
  2. dts: Add depends-on
or
  1. dts: Add depends-on
  2. dt-bindings: Add mandatory depends-on

I have not tested but if I used only the first commit in each
case (git bisect):
In the first case, dtbs_check is probably going to signal the
missing 'depends-on' property on dts.
In the second case, dtbs_check is probably going to signal the
not described 'depends-on' property present in dts.

> 
> > 
> > But, ok I will squash or perhaps remove completely this commit.
> > It introduces a DT compatibility break adding a new mandatory
> > property (raised by Rob on cover letter review).
> > Is this compatibility break can be acceptable ?  
> 
> Requiring property in bindings as a fix for something which was broken
> is ok. But this is independent of Linux drivers, which should not stop
> working.

Ok, thanks.

> 
> Best regards,
> Krzysztof
> 

Regards,
Hervé
  
Krzysztof Kozlowski Dec. 9, 2022, 8:06 a.m. UTC | #5
On 08/12/2022 16:51, Herve Codina wrote:
> Hi Krzysztof,
> 
> On Thu, 8 Dec 2022 10:46:32 +0100
> Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:
> 
>> On 08/12/2022 10:05, Herve Codina wrote:
>>> Hi Krzysztof,
>>>
>>> On Thu, 8 Dec 2022 09:26:41 +0100
>>> Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:
>>>   
>>>> On 07/12/2022 17:24, Herve Codina wrote:  
>>>>> The 'depends-on' property is set in involved DTS.
>>>>>
>>>>> Move it to a required property.
>>>>>
>>>>> Signed-off-by: Herve Codina <herve.codina@bootlin.com>
>>>>> ---
>>>>>  Documentation/devicetree/bindings/pci/renesas,pci-rcar-gen2.yaml | 1 +    
>>>>
>>>> This should be squashed with previous patch. There is no point to add
>>>> property and immediately in the next patch make it required. Remember
>>>> that bindings are separate from DTS.
>>>>
>>>> Best regards,
>>>> Krzysztof
>>>>  
>>>
>>> I though about make dtbs_check in case of git bisect.  
>>
>> And what would this commit change? In Git you will have
>> 1. dt-bindings: PCI: renesas,pci-rcar-gen2: Add depends-on for RZ/N1 SoC
>> family
>> 2. dt-bindings: PCI: renesas,pci-rcar-gen2: 'depends-on' is no more optional
>>
>> so what is the difference for git bisect?
> 
> Well, today, I have:
> 1. dt-bindings: Add depends-on
> 2. dts: Add depends-on
> 3. dt-bindings: Move depends-on to mandatory

What does it mean "I have"? Patches on mailing list? But we talk about
Git and I wrote you bindings are DTS are not going the same tree.

> 
> If I squash dt-bindings commits, I am going to have:
>   1. dt-bindings: Add mandatory depends-on
>   2. dts: Add depends-on
> or
>   1. dts: Add depends-on
>   2. dt-bindings: Add mandatory depends-on

And how does it matter? Anyway it goes separate trees.

> 
> I have not tested but if I used only the first commit in each
> case (git bisect):

It's not bisectable anyway, you cannot make it bisectable within one
release.

> In the first case, dtbs_check is probably going to signal the
> missing 'depends-on' property on dts.
> In the second case, dtbs_check is probably going to signal the
> not described 'depends-on' property present in dts.

And why is that even a problem?

Best regards,
Krzysztof
  
Herve Codina Dec. 13, 2022, 1:22 p.m. UTC | #6
Hi Krzysztof,

On Fri, 9 Dec 2022 09:06:55 +0100
Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:

> On 08/12/2022 16:51, Herve Codina wrote:
> > Hi Krzysztof,
> > 
> > On Thu, 8 Dec 2022 10:46:32 +0100
> > Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:
> >   
> >> On 08/12/2022 10:05, Herve Codina wrote:  
> >>> Hi Krzysztof,
> >>>
> >>> On Thu, 8 Dec 2022 09:26:41 +0100
> >>> Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:
> >>>     
> >>>> On 07/12/2022 17:24, Herve Codina wrote:    
> >>>>> The 'depends-on' property is set in involved DTS.
> >>>>>
> >>>>> Move it to a required property.
> >>>>>
> >>>>> Signed-off-by: Herve Codina <herve.codina@bootlin.com>
> >>>>> ---
> >>>>>  Documentation/devicetree/bindings/pci/renesas,pci-rcar-gen2.yaml | 1 +      
> >>>>
> >>>> This should be squashed with previous patch. There is no point to add
> >>>> property and immediately in the next patch make it required. Remember
> >>>> that bindings are separate from DTS.
> >>>>
> >>>> Best regards,
> >>>> Krzysztof
> >>>>    
> >>>
> >>> I though about make dtbs_check in case of git bisect.    
> >>
> >> And what would this commit change? In Git you will have
> >> 1. dt-bindings: PCI: renesas,pci-rcar-gen2: Add depends-on for RZ/N1 SoC
> >> family
> >> 2. dt-bindings: PCI: renesas,pci-rcar-gen2: 'depends-on' is no more optional
> >>
> >> so what is the difference for git bisect?  
> > 
> > Well, today, I have:
> > 1. dt-bindings: Add depends-on
> > 2. dts: Add depends-on
> > 3. dt-bindings: Move depends-on to mandatory  
> 
> What does it mean "I have"? Patches on mailing list? But we talk about
> Git and I wrote you bindings are DTS are not going the same tree.
> 
> > 
> > If I squash dt-bindings commits, I am going to have:
> >   1. dt-bindings: Add mandatory depends-on
> >   2. dts: Add depends-on
> > or
> >   1. dts: Add depends-on
> >   2. dt-bindings: Add mandatory depends-on  
> 
> And how does it matter? Anyway it goes separate trees.

I finally understand what you mean by separate trees.
And indeed, you're right, my patches split does not make
any sense.

According to feedbacks on this v3 series, these 3 patches
will be removed in v4.

Thanks for the review,
Hervé
  

Patch

diff --git a/Documentation/devicetree/bindings/pci/renesas,pci-rcar-gen2.yaml b/Documentation/devicetree/bindings/pci/renesas,pci-rcar-gen2.yaml
index e1221ad68465..4dd0f2f72e62 100644
--- a/Documentation/devicetree/bindings/pci/renesas,pci-rcar-gen2.yaml
+++ b/Documentation/devicetree/bindings/pci/renesas,pci-rcar-gen2.yaml
@@ -138,6 +138,7 @@  allOf:
             the underlying USB hosts start.
       required:
         - clock-names
+        - depends-on
     else:
       properties:
         clocks: