RE: [PATCH v2 2/5] dt-bindings: mfd: adi,max77541.yaml Add MAX77541 bindings

[Sahin, Okan]

Hi Krzysztof,

Thank you for your feedback and efforts. I apologize for some missing points of v2 patch. I tried to do my best. I will be more careful to fix all feedback before sending new patch so I want to ask a few things before sending v3 patch

On Tue, 27 Dec 2022 10:54 AM
Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx> wrote:

> On 26/12/2022 23:38, Okan Sahin wrote:
> > The bindings for MAX77541 MFD driver. It also
> 
> 
> 1. You did not follow entirely my advice here. Read again what I asked you to fix
> in commit msg.
> 
> 2. No improvements in subject - ignored comment.
> 
> 
> > includes MAX77540 driver whose regmap is covered by MAX77541.
> >
> > Signed-off-by: Okan Sahin <okan.sahin@xxxxxxxxxx>
> > ---
> >  .../devicetree/bindings/mfd/adi,max77541.yaml | 102 ++++++++++++++++++
> >  MAINTAINERS                                   |   1 +
> >  2 files changed, 103 insertions(+)
> >  create mode 100644
> > Documentation/devicetree/bindings/mfd/adi,max77541.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/mfd/adi,max77541.yaml
> > b/Documentation/devicetree/bindings/mfd/adi,max77541.yaml
> > new file mode 100644
> > index 000000000000..50f93cb0bb66
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/mfd/adi,max77541.yaml
> > @@ -0,0 +1,102 @@
> > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause %YAML 1.2
> > +---
> > +$id:
> > +https://urldefense.com/v3/__http://devicetree.org/schemas/mfd/adi,max
> >
> +77541.yaml*__;Iw!!A3Ni8CS0y2Y!5i9A0O1bNDHqRib46J285KCGbVLbqULqRRa
> 153Q
> > +XIaclV3BEUrMbK0XAuXk3meed6XG3y7HigRu81P9dqjnUsb9S3g2t$
> > +$schema:
> > +https://urldefense.com/v3/__http://devicetree.org/meta-schemas/core.y
> >
> +aml*__;Iw!!A3Ni8CS0y2Y!5i9A0O1bNDHqRib46J285KCGbVLbqULqRRa153QXIa
> clV3
> > +BEUrMbK0XAuXk3meed6XG3y7HigRu81P9dqjnUsRlxWY6o$
> > +
> > +title: MAX77540/MAX77541 PMIC from ADI.
> 
> Drop tralling full stop.
> 
> > +
> > +maintainers:
> > +  - Okan Sahin <okan.sahin@xxxxxxxxxx>
> > +
> > +description: |
> > +  MAX77540 is a Power Management IC with 2 buck regulators.
> > +
> > +  MAX77541 is a Power Management IC with 2 buck regulators and 1 ADC.
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - adi,max77540
> > +      - adi,max77541
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  interrupts:
> > +    maxItems: 1
> > +
> > +  regulators:
> > +    $ref: ../regulator/adi,max77541-regulator.yaml#
> 
> Wrong path - it should be full path with /schemas/ just like it was last time.
> However such file does not exist, so again as I said - you did not test this patch.
> It's non-bisectable patchset. Order it properly. don't ignore the comments.
> 
> > +
> > +  adc:
> > +    type: object
> > +    additionalProperties: false
> > +    properties:
> > +      compatible:
> > +        const: adi,max77541-adc
> 
> This is a friendly reminder during the review process.
> 
> It seems my previous comments were not fully addressed. Maybe my feedback
> got lost between the quotes, maybe you just forgot to apply it.
> Please go back to the previous discussion and either implement all requested
> changes or keep discussing them.
> 
> Thank you.
Honestly, I don't quite understand what you're suggesting regarding the adc part. I thought I should add the adc as an object since it is in the mfd device. Do I need to remove this part?
> 
> > +
> > +    required:
> > +      - compatible
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - interrupts
> > +
> > +allOf:
> > +  - if:
> > +      properties:
> > +        compatible:
> > +          contains:
> > +            const: adi,max77540
> > +    then:
> > +      properties:
> > +        regulator:
> > +          properties:
> > +            compatible:
> > +              const: adi,max77540-regulator
> > +    else:
> > +      properties:
> > +        regulator:
> > +          properties:
> > +            compatible:
> > +              const: adi,max77541-regulator
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/interrupt-controller/irq.h>
> > +
> > +    i2c {
> > +        #address-cells = <1>;
> > +        #size-cells = <0>;
> > +
> > +        pmic@69 {
> > +            compatible = "adi,max77541";
> > +            reg = <0x69>;
> > +            interrupt-parent = <&gpio>;
> > +            interrupts = <16 IRQ_TYPE_EDGE_FALLING>;
> > +
> > +            regulators {
> > +                BUCK1 {
> > +                    regulator-min-microvolt = <500000>;
> > +                    regulator-max-microvolt = <5200000>;
> > +                    regulator-boot-on;
> > +                    regulator-always-on;
> > +                };
> > +                BUCK2 {
> > +                    regulator-min-microvolt = <500000>;
> > +                    regulator-max-microvolt = <5200000>;
> > +                    regulator-boot-on;
> > +                    regulator-always-on;
> > +                };
> > +            };
> > +
> > +            adc {
> > +                compatible = "adi,max77541-adc";
> > +            };
> > +        };
> > +    };
> > +
> 
> Stray new line. Drop it.
> 
> Best regards,
> Krzysztof
Best regards,
Okan