Re: [PATCH 03/15] dt-bindings: clock: qcom,dispcc-sm6125: Require GCC PLL0 DIV clock

[Marijn Suijten]

On 2023-06-26 18:15:13, Krzysztof Kozlowski wrote:
> On 26/06/2023 16:26, Marijn Suijten wrote:
> > On 2023-06-26 11:43:39, Konrad Dybcio wrote:
> >> On 25.06.2023 21:48, Marijn Suijten wrote:
> >>> On 2023-06-24 03:45:02, Konrad Dybcio wrote:
> >>>> On 24.06.2023 02:41, Marijn Suijten wrote:
> >>>>> The "gcc_disp_gpll0_div_clk_src" clock is consumed by the driver, will
> >>>>> be passed from DT, and should be required by the bindings.
> >>>>>
> >>>>> Fixes: 8397c9c0c26b ("dt-bindings: clock: add QCOM SM6125 display clock bindings")
> >>>>> Signed-off-by: Marijn Suijten <marijn.suijten@xxxxxxxxxxxxxx>
> >>>>> ---
> >>>> Ideally, you'd stick it at the bottom of the list, as the items: order
> >>>> is part of the ABI
> >>>
> >>> This isn't an ABI break, as this driver nor its bindings require/declare
> >>> a fixed order: they declare a relation between clocks and clock-names.
> >> Bindings describe the ABI, drivers implement compliant code flow.
> > 
> > That is how bindings are supposed to be...  However typically the driver
> > is written/ported first and then the bindings are simply created to
> 
> Your development process does not matter for the bindings. Whatever you
> decide to do "typically" is your choice, although of course I understand
> why you do it like that. You can argument the same that "I never create
> bindings in my process, so the driver defines the ABI".

This is not my process, nor my choice.

> > reflect this, and sometimes (as is the case with this patch)
> > incorrectly.
> > 
> > That, together with a lack of DTS and known-working device with it
> > (which is why I'm submitting driver+bindings+dts in one series now!)
> > makes us shoot ourselves in the foot by locking everyone into an ABI
> > that makes no sense.
> 
> No one is locked into the ABI. SoC maintainer decides on this. However
> unjustified ABI breaking or not caring about it at all is not the way to
> go. It is not the correct process.

It definitely sounds like it.

> >>> This orders the GCC clock just like other dispccs.  And the previous
> >>> patch dropped the unused cfg_ahb_clk from the bindings, so all bets are
> >>> off anyway.
> >> Thinking about it again, the binding has not been consumed by any upstream
> >> DT to date, so it should (tm) be fine to let it slide..
> > 
> > Exactly, I hope/doubt anyone was already using these incomplete
> > bindings.  And again: the ABI here is the name->phandle mapping, the
> > order Does Not Matter™.
> 
> No, it's not. Your one driver does not define the ABI. There are many
> different drivers, many different operating systems and other software
> components.

You missed the point entirely.

The point is that someone contributed a dt-bindings patch that does not
represent the hardware (hence not the driver for that hardware either).
It was taken without objection.  So now what?  We are stuck with a
broken ABI that does not allow us to describe the hardware?

If there are many different drivers and other OSes, why are we solely
responsible for describing broken bindings?  Why were there no
objections elsewhere that this does not allow us to describe the
hardware in question?

Note that the person signing off on and sending that initial dt-bindings
patch for qcom,dispcc-sm6125.yaml is me, by the way.

- Marijn