Re: [PATCH v2 1/2] dt-bindings: memory: mediatek: Add SMI reset and clamp related property

["Friday Yang (杨阳)" <Friday.Yang@mediatek.com>]

On Wed, 2024-11-20 at 08:45 +0100, Krzysztof Kozlowski wrote:
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> 
> 
> On Wed, Nov 20, 2024 at 02:36:38PM +0800, Friday Yang wrote:
> > diff --git a/Documentation/devicetree/bindings/memory-
> > controllers/mediatek,smi-larb.yaml
> > b/Documentation/devicetree/bindings/memory-
> > controllers/mediatek,smi-larb.yaml
> > index 2381660b324c..302c0f93b49d 100644
> > --- a/Documentation/devicetree/bindings/memory-
> > controllers/mediatek,smi-larb.yaml
> > +++ b/Documentation/devicetree/bindings/memory-
> > controllers/mediatek,smi-larb.yaml
> > @@ -69,6 +69,18 @@ properties:
> >      description: the hardware id of this larb. It's only required
> > when this
> >        hardware id is not consecutive from its M4U point of view.
> > 
> > +  resets:
> > +    maxItems: 1
> > +    description: This contains a phandle to the reset controller
> > node and an index
> > +      to a reset signal. SMI larbs need to get the reset
> > controller by the node.
> 
> First sentence is 100% redundant. Arguments depend on the reset-
> cells,
> not this binding.
> 
> > +      SMI could get the reset signal by the index number defined
> > in the header
> > +      include/dt-bindings/reset/mt8188-resets.h.
> 
> What? How this can depend on consumer? Drop entire description, it is
> useless.
> 

Thanks for your comments, I will remove the entire description.

> > +
> > +  reset-names:
> > +    const: larb
> > +    description: The name of reset controller. SMI driver need to
> > obtain the
> > +      reset controller based on this.
> 
> Drop description, useless.

OK, I will remove the entire description.

> 
> > +
> >  required:
> >    - compatible
> >    - reg
> > @@ -125,19 +137,38 @@ allOf:
> >        required:
> >          - mediatek,larb-id
> > 
> > +  - if:  # only for camera and image subsys
> > +      properties:
> > +        mediatek,smi:
> > +            contains:
> 
> Never tested.

OK, I will fix the indentation.

> 
> > +              enum:
> > +                - smi_sub_common_img0_4x1
> > +                - smi_sub_common_img1_4x1
> > +                - smi_sub_common_cam_5x1
> > +                - smi_sub_common_cam_8x1
> 
> Does not work. Test your code before you send it. No clue what you
> want
> to achieve, so not sure how to help.
> 

As I mentioned in SMI driver, only SMI larbs located in
camera and image subsys need to apply clamp and reset operation,
others can be skipped.
So I want to know if this description method meets your expectations
here(smi_sub_common_img0_4x1, smi_sub_common_img1_4x1 ...).

> 
> > +
> > +    then:
> > +      required:
> > +        - resets
> > +        - reset-names
> > +
> >  additionalProperties: false
> > 
> >  examples:
> >    - |+
> > -    #include <dt-bindings/clock/mt8173-clk.h>
> > -    #include <dt-bindings/power/mt8173-power.h>
> > -
> > -    larb1: larb@16010000 {
> > -      compatible = "mediatek,mt8173-smi-larb";
> > -      reg = <0x16010000 0x1000>;
> > -      mediatek,smi = <&smi_common>;
> > -      power-domains = <&scpsys MT8173_POWER_DOMAIN_VDEC>;
> > -      clocks = <&vdecsys CLK_VDEC_CKEN>,
> > -               <&vdecsys CLK_VDEC_LARB_CKEN>;
> > -      clock-names = "apb", "smi";
> > +    #include <dt-bindings/clock/mediatek,mt8188-clk.h>
> > +    #include <dt-bindings/power/mediatek,mt8188-power.h>
> > +    #include <dt-bindings/reset/mt8188-resets.h>
> 
> This is some total mess. Never tested, not correct. Sorry, run it
> internally through some sort of review or internal checklist which
> will
> ask you to test the code before sending.
> 
> Best regards,
> Krzysztof
>