From: ChiYuan Huang <u0084500@gmail.com>
To: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Cc: Rob Herring <robh+dt@kernel.org>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
Lee Jones <lee.jones@linaro.org>, Mark Brown <broonie@kernel.org>,
dmitry.torokhov@gmail.com, Liam Girdwood <lgirdwood@gmail.com>,
cy_huang <cy_huang@richtek.com>,
"open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS"
<devicetree@vger.kernel.org>, lkml <linux-kernel@vger.kernel.org>,
linux-input@vger.kernel.org
Subject: Re: [PATCH 1/4] dt-binding: mfd: Add Richtek RT5120 PMIC support
Date: Wed, 8 Jun 2022 15:25:20 +0800 [thread overview]
Message-ID: <CADiBU38Gc2McH0ryRFPUAkmJzL1rxQnLHqvo5-+U_TPVjgHxzg@mail.gmail.com> (raw)
In-Reply-To: <678ce480-cbbc-ffa8-10bf-d93021a0df0a@linaro.org>
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
next prev parent reply other threads:[~2022-06-08 7:58 UTC|newest]
Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-06-07 5:52 [PATCH 0/4] Add Richtek RT5120 PMIC support cy_huang
2022-06-07 5:52 ` [PATCH 1/4] dt-binding: mfd: " cy_huang
2022-06-07 11:52 ` Krzysztof Kozlowski
2022-06-08 2:52 ` ChiYuan Huang
2022-06-08 7:01 ` Krzysztof Kozlowski
2022-06-08 7:25 ` ChiYuan Huang [this message]
2022-06-08 7:44 ` Krzysztof Kozlowski
2022-06-07 19:02 ` Mark Brown
2022-06-08 3:04 ` ChiYuan Huang
2022-06-07 5:52 ` [PATCH 2/4] mfd: rt5120: Add Richtek " cy_huang
2022-06-07 5:52 ` [PATCH 3/4] regulator: rt5120: Add PMIC regulator support cy_huang
2022-06-07 19:00 ` Mark Brown
2022-06-08 3:15 ` ChiYuan Huang
2022-06-08 10:12 ` Mark Brown
2022-06-09 6:35 ` ChiYuan Huang
2022-06-10 11:03 ` Mark Brown
2022-06-13 1:49 ` ChiYuan Huang
2022-06-13 11:10 ` Mark Brown
2022-06-07 5:52 ` [PATCH 4/4] input: misc: rt5120: Add power key support cy_huang
2022-06-08 7:03 ` Krzysztof Kozlowski
2022-06-08 7:05 ` Krzysztof Kozlowski
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CADiBU38Gc2McH0ryRFPUAkmJzL1rxQnLHqvo5-+U_TPVjgHxzg@mail.gmail.com \
--to=u0084500@gmail.com \
--cc=broonie@kernel.org \
--cc=cy_huang@richtek.com \
--cc=devicetree@vger.kernel.org \
--cc=dmitry.torokhov@gmail.com \
--cc=krzysztof.kozlowski+dt@linaro.org \
--cc=krzysztof.kozlowski@linaro.org \
--cc=lee.jones@linaro.org \
--cc=lgirdwood@gmail.com \
--cc=linux-input@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=robh+dt@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).