Re: [PATCH] dt-bindings: clk: vc5: Make SD/OE pin configuration properties not required

[Luca Ceresoli <luca.ceresoli@bootlin.com>]

Hello Sean, Geert,

On Tue, 10 Sep 2024 17:13:55 -0500
Rob Herring <robh+dt@kernel.org> wrote:

> On Wed, Mar 22, 2023 at 3:39 AM Luca Ceresoli <luca.ceresoli@bootlin.com> wrote:
> >
> > Hello Stephen,
> >
> > On Mon, 20 Mar 2023 14:27:56 -0700
> > Stephen Boyd <sboyd@kernel.org> wrote:
> >  
> > > Quoting Sean Anderson (2023-01-24 08:23:45)  
> > > > On 1/24/23 03:28, Geert Uytterhoeven wrote:  
> > > > > Hi Luca,
> > > > >
> > > > > On Tue, Jan 24, 2023 at 9:12 AM Luca Ceresoli <luca.ceresoli@bootlin.com> wrote:  
> > > > >> On Thu, 19 Jan 2023 14:27:43 -0500
> > > > >> Sean Anderson <sean.anderson@seco.com> wrote:  
> > > > >> > On 1/11/23 10:55, Geert Uytterhoeven wrote:  
> > > > >  
> > > > >> I'm wondering whether Geert has a practical example of a situation
> > > > >> where it is better to have these properties optional.  
> > > > >
> > > > > My issue was that these properties were introduced long after the
> > > > > initial bindings, hence pre-existing DTS does not have them.
> > > > > Yes, we can add them, but then we have to read out the OTP-programmed
> > > > > settings first. If that's the way to go, I can look into that, though...  
> > > >
> > > > FWIW I think there's no need to update existing bindings which don't
> > > > have this property. The required aspect is mainly a reminder for new
> > > > device trees.
> > > >  
> > >
> > > Is there any resolution on this thread? I'm dropping this patch from my
> > > queue.  
> >
> > IIRC Geert kind of accepted the idea that these properties should stay
> > required. Which is a bit annoying but it's the safest option, so unless
> > there are new complaints with solid use cases for making them optionalm,
> > I think it's OK to drop the patch.  
> 
> The warnings related to this are now at the top of the list (by number
> of occurrences):
> 
>      50 clock-generator@6a: 'idt,shutdown' is a required property
>      50 clock-generator@6a: 'idt,output-enable-active' is a required property
> 
> IMO, if these properties haven't been needed for years, then they
> obviously aren't really required.

I think Rob's point adds to Geert's observation that there are other
"idt,*" properties in the output nodes that may also be important to
have correctly set, and are optional.

So, Sean, I understand when you state it's safer to have these set.
However this is valid for lots of other optional properties in any
binding. Optional properties _can_ be set if that's important, just
it's not mandatory to set them in all cases.

As a matter of fact, we have been having for a long time some in-tree
device trees which don't set these properties, which I believe implies
it's OK for those cases to not set them, and to let them be set for the
device trees where it is important.

Finally, there is a maintenance/legacy issue: if we wanted to keep these
properties optional, who would chase all the boards defined in existing
device trees to discover the correct values?

Bottom line, my Reviewed-by tag is still valid.

What is your opinion given these last few discussion point Sean?

Luca

-- 
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com