Re: [PATCH 1/4] dt-binding: mfd: Add Richtek RT5120 PMIC support

[ChiYuan Huang <u0084500@gmail.com>]

Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> 於 2022年6月8日 週三 下午3:02寫道：
>
> On 08/06/2022 04:52, ChiYuan Huang wrote:
> > Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> 於 2022年6月7日 週二 下午7:52寫道：
> >>
> >> On 07/06/2022 07:52, cy_huang wrote:
> >>> From: ChiYuan Huang <cy_huang@richtek.com>
> >>>
> >>> Add Richtek RT5120 PMIC devicetree document.
> >>>
> >>> Signed-off-by: ChiYuan Huang <cy_huang@richtek.com>
> >>> ---
> >>>  .../devicetree/bindings/mfd/richtek,rt5120.yaml    | 180 +++++++++++++++++++++
> >>>  1 file changed, 180 insertions(+)
> >>>  create mode 100644 Documentation/devicetree/bindings/mfd/richtek,rt5120.yaml
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/mfd/richtek,rt5120.yaml b/Documentation/devicetree/bindings/mfd/richtek,rt5120.yaml
> >>> new file mode 100644
> >>> index 00000000..376bf73
> >>> --- /dev/null
> >>> +++ b/Documentation/devicetree/bindings/mfd/richtek,rt5120.yaml
> >>> @@ -0,0 +1,180 @@
> >>> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> >>> +%YAML 1.2
> >>> +---
> >>> +$id: http://devicetree.org/schemas/mfd/richtek,rt5120.yaml#
> >>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >>> +
> >>> +title: Richtek RT5120 PMIC
> >>> +
> >>> +maintainers:
> >>> +  - ChiYuan Huang <cy_huang@richtek.com>
> >>> +
> >>> +description: |
> >>> +  The RT5120 provides four high-efficiency buck converters and one LDO voltage
> >>> +  regulator. The device is targeted at providingthe processor voltage, memory,
> >>> +  I/O, and peripheral rails in home entertainment devices. The I2C interface is
> >>> +  used for dynamic voltage scaling of the processor voltage, power rails on/off
> >>> +  sequence control, operation mode selection.
> >>> +
> >>> +properties:
> >>> +  compatible:
> >>> +    enum:
> >>> +      - richtek,rt5120
> >>> +
> >>> +  reg:
> >>> +    maxItems: 1
> >>> +
> >>> +  interrupts:
> >>> +    maxItems: 1
>
> Your powerkey driver takes two interrupts. You should describe them in
> the powerkey.
>
> >>> +
> >>> +  interrupt-controller: true
> >>> +
> >>> +  "#interrupt-cells":
> >>> +    const: 1
> >>> +
> >>> +  wakeup-source: true
> >>> +
> >>> +  richtek,enable-undervolt-hiccup:
> >>> +    type: boolean
> >>> +    description: |
> >>> +      If used, under voltage protection trigger hiccup behavior, else latchup as
> >>> +      default
> >>> +
> >>> +  richtek,enable-overvolt-hiccup:
> >>> +    type: boolean
> >>> +    description:
> >>> +      Like as 'enable-uv-hiccup', it configures over voltage protection to
> >>> +      hiccup, else latchup as default
> >>> +
> >>> +  vin1-supply:
> >>> +    description: phandle for buck1 input power source
> >>> +
> >>> +  vin2-supply:
> >>> +    description: phandle for buck2 input power source
> >>> +
> >>> +  vin3-supply:
> >>> +    description: phandle for buck3 input power source
> >>> +
> >>> +  vin4-supply:
> >>> +    description: phandle for buck4 input power source
> >>> +
> >>> +  vinldo-supply:
> >>> +    description: phandle for ldo input power source
> >>> +
> >>> +  regulators:
> >>> +    type: object
> >>> +
> >>> +    patternProperties:
> >>> +      "^buck[1-4]$":
> >>> +        type: object
> >>> +        $ref: /schemas/regulator/regulator.yaml#
> >>> +
> >>> +        properties:
> >>> +          regulator-allowed-modes:
> >>> +            description: |
> >>> +              Used to specify the allowed buck converter operating mode
> >>> +              mode mapping:
> >>> +                0: auto mode
> >>> +                1: force pwm mode
> >>> +            items:
> >>> +              enum: [0, 1]
> >>> +
> >>> +        unevaluatedProperties: false
> >>
> >> Better to put it after '$ref' for readability.
> > OK, Fix in next
> >>
> >>> +
> >>> +      "^(ldo|exten)$":
> >>> +        type: object
> >>> +        $ref: /schemas/regulator/regulator.yaml#
> >>
> >> You need here unevaluatedProperties:false as well (for the ldo/exten
> >> properties)
> > Fix in next.
> >>
> >>> +
> >>> +    additionalProperties: false
> >>> +
> >>> +  powerkey:
> >>> +    type: object
> >>> +    description:
> >>> +      The power key driver may be optional. If not used, change node status to
> >>> +      'disabled'
> >>
> >> This description is not helpful, does not describe the hardware. Please
> >> describe hardware, not Devicetree usage.
> > That's because it's a PMIC. Power key is also connected to it.
> > For power key press, all power rails will start to power up.
> > But in the application, there may be other PMIC that's also connected
> > to power key.
> > That's why this power key driver may need to be optional.
> > One system only need one driver to report the power key status.
> >
> > Currently in some linux OS, it uses the auto module loading mechanism.
> > All kernel module files may be all the same, but it uses the
> > devicetree to decide how many devices
> > need to be declared. Since RT5120 power key device may be optional,
> > following by mfd_add_device, if of_node is
> > found, and status is "disabled", the sub device would be skipped.
> >
> > Actually, I'm also confused about it. There may be three ways to implement it
> > 1. not to build this kernel module -> seems to violate my above application
> > 2. Use one boolean property to decide power key cell need to be used or not??
> > 3. like as now, use the node status to decide it.
> >
> > Is there the better way to do it?
>
> The status does not determine whether device in the bindings is optional
> or not. Rather it's presence. In the term of bindings the "optional"
> means that something might not be there physically. E.g. clock line
> connected or not. System implementation - MFD, power off handling - is
> here (almost) irrelevant.
>
> In your case, the power key feature seems to be there always, so the
> "powerkey" node should be required and not disabled. Don't mention in
> description of hardware anything about disabling it or not.
>
> In your application, I would say it is interesting design that someone
> connects one power up line to two different PMICs in a conflicting way.
> This sounds like total mistake from hardware point of view.
>
> Anyway it is not the job for this patch to solve such conflicts.
>
Thanks,  I think your point is 'optional' keyword.
If there's only redundant description line, I may decide to remove it.
The property name already show its usage.
> Best regards,
> Krzysztof