Re: [PATCH v3 12/14] dt-bindings: msm/dp: Add bindings for HDCP registers

[Sean Paul <sean@poorly.run>]

On Mon, Oct 04, 2021 at 02:58:41PM -0500, Bjorn Andersson wrote:
> On Fri 01 Oct 10:11 CDT 2021, Sean Paul wrote:
> 
> > From: Sean Paul <seanpaul@chromium.org>
> > 
> > This patch adds the bindings for the MSM DisplayPort HDCP registers
> > which are required to write the HDCP key into the display controller as
> > well as the registers to enable HDCP authentication/key
> > exchange/encryption.
> > 
> > We'll use a new compatible string for this since the fields are optional.
> > 
> 
> I don't think you need a new compatible, in particular since I presume
> we should use the hdcp compatible in all platforms? Or is there a reason
> for not picking that one?
> 
> Instead I suggest that you simply do minItems: 1, maxItems: 3 and detect
> which of the two cases you have in the driver.

Thanks for your review, Bjorn! I had done this in v2 (see [1] & [2]), but it was
suggested that a new compatible would be better. I'll change it back to this
method rebased on top of your changes.

Sean

[1]- https://patchwork.freedesktop.org/patch/454066/?series=94712&rev=1
[2]- https://patchwork.freedesktop.org/patch/454068/?series=94712&rev=1


> 
> PS. I hope to get
> https://lore.kernel.org/linux-arm-msm/20211001174400.981707-1-bjorn.andersson@linaro.org/
> landed before we add these new optional regions...
> 
> Regards,
> Bjorn
> 
> > Cc: Rob Herring <robh@kernel.org>
> > Cc: Stephen Boyd <swboyd@chromium.org>
> > Signed-off-by: Sean Paul <seanpaul@chromium.org>
> > Link: https://patchwork.freedesktop.org/patch/msgid/20210913175747.47456-13-sean@poorly.run #v1
> > Link: https://patchwork.freedesktop.org/patch/msgid/20210915203834.1439-13-sean@poorly.run #v2
> > 
> > Changes in v2:
> > -Drop register range names (Stephen)
> > -Fix yaml errors (Rob)
> > Changes in v3:
> > -Add new compatible string for dp-hdcp
> > -Add descriptions to reg
> > -Add minItems/maxItems to reg
> > -Make reg depend on the new hdcp compatible string
> > ---
> > 
> > Disclaimer: I really don't know if this is the right way to approach
> > this. I tried using examples from other bindings, but feedback would be
> > very much welcome on how I could add the optional register ranges.
> > 
> > 
> >  .../bindings/display/msm/dp-controller.yaml   | 34 ++++++++++++++++---
> >  1 file changed, 30 insertions(+), 4 deletions(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
> > index 64d8d9e5e47a..a176f97b2f4c 100644
> > --- a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
> > +++ b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
> > @@ -17,9 +17,10 @@ properties:
> >    compatible:
> >      enum:
> >        - qcom,sc7180-dp
> > +      - qcom,sc7180-dp-hdcp
> >  
> > -  reg:
> > -    maxItems: 1
> > +  # See compatible-specific constraints below.
> > +  reg: true
> >  
> >    interrupts:
> >      maxItems: 1
> > @@ -89,6 +90,29 @@ required:
> >    - power-domains
> >    - ports
> >  
> > +allOf:
> > +  - if:
> > +      properties:
> > +        compatible:
> > +          contains:
> > +            const: qcom,sc7180-dp-hdcp
> > +    then:
> > +      properties:
> > +        reg:
> > +          minItems: 3
> > +          maxItems: 3
> > +          items:
> > +            - description: Registers for base DP functionality
> > +            - description: (Optional) Registers for HDCP device key injection
> > +            - description: (Optional) Registers for HDCP TrustZone interaction
> > +    else:
> > +      properties:
> > +        reg:
> > +          minItems: 1
> > +          maxItems: 1
> > +          items:
> > +            - description: Registers for base DP functionality
> > +
> >  additionalProperties: false
> >  
> >  examples:
> > @@ -99,8 +123,10 @@ examples:
> >      #include <dt-bindings/power/qcom-rpmpd.h>
> >  
> >      displayport-controller@ae90000 {
> > -        compatible = "qcom,sc7180-dp";
> > -        reg = <0xae90000 0x1400>;
> > +        compatible = "qcom,sc7180-dp-hdcp";
> > +        reg = <0 0x0ae90000 0 0x1400>,
> > +              <0 0x0aed1000 0 0x174>,
> > +              <0 0x0aee1000 0 0x2c>;
> >          interrupt-parent = <&mdss>;
> >          interrupts = <12>;
> >          clocks = <&dispcc DISP_CC_MDSS_AHB_CLK>,
> > -- 
> > Sean Paul, Software Engineer, Google / Chromium OS
> > 

-- 
Sean Paul, Software Engineer, Google / Chromium OS