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

From: ChiYuan Huang
Date: Wed Jun 08 2022 - 03:58:08 EST


Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx> 於 2022年6月8日 週三 下午3:02寫道:
>
> On 08/06/2022 04:52, ChiYuan Huang wrote:
> > Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx> 於 2022年6月7日 週二 下午7:52寫道:
> >>
> >> On 07/06/2022 07:52, cy_huang wrote:
> >>> From: ChiYuan Huang <cy_huang@xxxxxxxxxxx>
> >>>
> >>> Add Richtek RT5120 PMIC devicetree document.
> >>>
> >>> Signed-off-by: ChiYuan Huang <cy_huang@xxxxxxxxxxx>
> >>> ---
> >>> .../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@xxxxxxxxxxx>
> >>> +
> >>> +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