Re: [RFC PATCH 1/3] dt-bindings: media: qcom: Add CAMSS Offline Processing Engine (OPE)

[Loic Poulain]

Hi Krzysztof,

On Mon, Mar 23, 2026 at 2:04 PM Krzysztof Kozlowski <krzk@xxxxxxxxxx> wrote:
>
> On 23/03/2026 13:58, Loic Poulain wrote:
> > Add Devicetree binding documentation for the Qualcomm Camera Subsystem
> > Offline Processing Engine (OPE) found on platforms such as Agatti.
> > The OPE is a memory-to-memory image processing block which operates
> > on frames read from and written back to system memory.
> >
> > Signed-off-by: Loic Poulain <loic.poulain@xxxxxxxxxxxxxxxx>
>
> I don't see explanation in cover letter why this is RFC, so I assume
> this is not ready, thus not a full review but just few nits to spare you
> resubmits later when this becomes reviewable.
>
> > ---
> >  .../bindings/media/qcom,camss-ope.yaml        | 86 +++++++++++++++++++
> >  1 file changed, 86 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/media/qcom,camss-ope.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/media/qcom,camss-ope.yaml b/Documentation/devicetree/bindings/media/qcom,camss-ope.yaml
> > new file mode 100644
> > index 000000000000..509b4e89a88a
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/media/qcom,camss-ope.yaml
>
> Filename must match compatible.

Some bindings (for example clock/qcom,mmcc.yaml) do not strictly
follow this rule and instead use a more generic filename that groups
multiple device-specific compatibles. I mention this because my
intention with a generic filename was to allow the binding to cover
additional compatibles in the future.

As I understand it, in the current state I should either:
- rename the file so that it matches the specific compatible, e.g.
qcom,qcm2290-camss-ope.yaml, or
- keep the generic filename (qcom,camss-ope.yaml) and add a top-level
const: qcom,camss-ope compatible to justify the generic naming.

Any preferred/valid direction?

>
> > @@ -0,0 +1,86 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > +%YAML 1.2
>
> ...
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - reg-names
> > +  - clocks
> > +  - clock-names
> > +  - interrupts
> > +  - interconnects
> > +  - interconnect-names
> > +  - iommus
> > +  - power-domains
> > +  - power-domain-names
> > +
> > +additionalProperties: true
>
> There are no bindings like that. You cannot have here true.

ok.

>
> Also, lack of example is a no-go.

Ouch, yes. Would it make sense to have dt_binding_check catch this
kind of issue?

>
> BTW, also remember about proper versioning of your patchset. b4 would do
> that for you, but since you did not use it, you must handle it.

ack.