RE: [PATCH 1/7] dt-bindings: ufs: Document Renesas R-Car UFS host controller

[Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>]

Hi Krzysztof,

Thank you for your review!

> From: Krzysztof Kozlowski, Sent: Tuesday, April 12, 2022 9:23 PM
> 
> On 12/04/2022 09:36, Yoshihiro Shimoda wrote:
> > Document Renesas R-Car UFS host controller for R-Car S4-8 (r8a779f0).
> >
> > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> > ---
> >  .../devicetree/bindings/ufs/renesas,ufs.yaml  | 63 +++++++++++++++++++
> >  1 file changed, 63 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/ufs/renesas,ufs.yaml
> >
> 
> Thank you for your patch. There is something to discuss/improve.
> 
> > +allOf:
> > +  - $ref: ufs-common.yaml
> > +
> > +properties:
> > +  compatible:
> > +    items:
> 
> No items, so just "const: renesas,...."

I got it. I'll fix it.

> > +      - const: renesas,r8a779f0-ufs
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  clocks:
> > +    maxItems: 2
> > +
> > +  clock-names:
> > +    maxItems: 2
> 
> No need for maxItems. Please test your bindings with dt_binding_check,
> because you should see a warning about it, AFAIR.

I'll fix it. I should have checked dt_binding_check before submitting a patch...
# I thought I had already checked it, but I didn't check it...

> > +    items:
> > +      - const: fck
> > +      - const: ref_clk
> > +
> > +  power-domains:
> > +    maxItems: 1
> > +
> > +  resets:
> > +    maxItems: 1
> > +
> 
> No phys. Are you sure you don't need one?

The phy doesn't have any memory map area. The host controller can access
it via vender specific registers. But, the vender specific registers are used for
initialization of both the controller and phy. So, I don't need phys on the ufs node.

> > +required:
> > +  - compatible
> > +  - reg
> > +  - clocks
> > +  - clock-names
> > +  - power-domains> +  - resets
> > +
> > +unevaluatedProperties: false
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/clock/r8a779f0-cpg-mssr.h>
> > +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> > +    #include <dt-bindings/power/r8a779f0-sysc.h>
> > +
> > +    ufs: scsi@e686000 {
> 
> Node name "ufs".

I got it. I'll fix it.

Best regards,
Yoshihiro Shimoda

> Best regards,
> Krzysztof