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
>
