Re: [PATCH v2 0/4] PCI: rcar, rcar-gen2: More Gen2 compatibility strings

[Bjorn Helgaas <helgaas@kernel.org>]

Hi Geert,

On Fri, Dec 11, 2015 at 04:58:06PM +0100, Geert Uytterhoeven wrote:
> On Fri, Dec 11, 2015 at 3:59 PM, Bjorn Helgaas <helgaas@kernel.org> wrote:
> > On Fri, Dec 11, 2015 at 11:14:34AM +0900, Simon Horman wrote:
> >> On Wed, Dec 09, 2015 at 12:37:43PM -0600, Bjorn Helgaas wrote:
> >> > On Thu, Dec 03, 2015 at 07:51:36AM +0900, Simon Horman wrote:
> >> > > this short series adds generic gen2 and SoC-specific r8a7793 compatibility
> >> > > strings to the rcar PCI and rcar-gen2 PCIE drivers. The intention is to
> >> > > provide a complete set of compatibility strings for known Gen2 SoCs.
> >> > >
> >> > > Key Changes in v2:
> >> > > * Include "rcar-" in generic bindings
> >> > >
> >> > > Simon Horman (4):
> >> > >   PCI: rcar-gen2: add gen2 fallback compatibility string
> >> > >   PCI: rcar-gen2: add device tree support for r8a7793
> >> > >   PCI: rcar: add gen2 fallback compatibility string
> >> > >   PCI: rcar: add device tree support for r8a7793
> >> > >
> >> > >  Documentation/devicetree/bindings/pci/pci-rcar-gen2.txt | 12 ++++++++++--
> >> > >  Documentation/devicetree/bindings/pci/rcar-pci.txt      | 14 +++++++++++---
> >> > >  drivers/pci/host/pci-rcar-gen2.c                        |  1 +
> >> > >  drivers/pci/host/pcie-rcar.c                            |  1 +
> >> > >  4 files changed, 23 insertions(+), 5 deletions(-)
> >> >
> >> > I applied these:
> >> >
> >> > >   PCI: rcar-gen2: add gen2 fallback compatibility string
> >> > >   PCI: rcar: add gen2 fallback compatibility string
> >> >
> >> > to pci/host-rcar for v4.5, thanks!
> >> >
> >> > I haven't applied the R8A7793 binding documentation updates yet, but
> >> > I'll be happy to do so given a short description of why they're
> >> > useful (since they don't update a DT or the driver).  Or they could be
> >> > merged via a DT tree.
> >>
> >> To clarify: you would like a description in the changelog?
> >
> > Yes, please.  The email discussion so far hasn't contained what I'm
> > looking for (if it had, I would have just inserted it and been done
> > with it).
> >
> > Apparently it has to do with the stable DT rules, which I don't know.
> > A concrete example would probably help clear it up.
> 
> The stable DT rules mean that an old DTS should keep on working with
> newer kernels.
> 
> Suppose we have two SoCs, that both contain "foo" modules, which look
> identical. Hence the DTS for both declares the devices to be compatible
> with "vendor,foo".
> 
> Later, we discover a difference between the two "foo" modules in the two
> SoCs (e.g. a feature present in one of them, or worse, a bug), which we
> need to handle in the driver. But how can we distinguish between them?
> We can change the compatible value in the DTS, but that means the user
> has to update the DTS when updating the kernel. For a new feature that
> may be deemed acceptable, for a bug fix that's not acceptable.
> 
> Hence we always use an SoC-specific compatible value, which may or may
> not be accompanied by a family-specific and/or generic compatible value.
> As long as everything can be handled the same, the driver will just match
> against the most generic compatible value used. But if needed later, the
> driver can be updated to match against a more specific compatible value,
> and can have special handling for  a module in a specific SoC.
> 
> So that's why we want to have compatible value in the DT bindings that
> are not necessarily used by the driver.
> 
> In a perfect world, where all hardware is properly documented, or even
> Open Source, we wouldn't need this. There we could just declare a device
> compatible with what it really is, based on the module's internal version ID
> (ideally a git commit ID of its HDL source ;-).
> 
> I hope the above explains it. If you have more questions, feel free to ask!

The above is all pretty standard device identification technique.
That's not what I've been missing.

But I just had an epiphany.  Tell me if I'm talking sense or
gibberish:

  If a patch adds a use of "renesas,pcie-r8a7793" in a .dts, .c, or .h
  file, e.g., in arch/arm/boot/dts/ or in the driver, and somebody
  runs checkpatch on that patch, checkpatch will complain unless
  "renesas,pcie-r8a7793" appears somewhere in
  devicetree/bindings/pci/.

So if I understand correctly, the only point of this patch is to shut
up checkpatch in that situation.  It doesn't seem terribly useful to
me because checkpatch is only relevant to Linux kernel patches, and
even there it's optional and advisory.

The latest changelog says "By documenting this compat string it may be
used in DTSs shipped, for example as part of ROMs", which seems to be
saying a DTS may not be shipped unless checkpatch approves of it.
That's a social and procedural question, not a coding question.

It's fine with me if you want to try to enforce that via checkpatch,
but I'd just like to be clear on the mechanism.

Bjorn