| Message ID | 1654581161-12349-1-git-send-email-u0084500@gmail.com |
|---|---|
| Headers | show |
| Series | Add Richtek RT5120 PMIC support | expand |
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 > > + > > + 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? > > > + > > + properties: > > + compatible: > > + enum: > > + - richtek,rt5120-pwrkey > > + > > + required: > > + - compatible > > + > > + additionalProperties: false > > + > > +required: > > + - compatible > > + - reg > > + - interrupts > > + - '#interrupt-cells' > > + - interrupt-controller > > + - regulators > > + - powerkey > > You wrote powerkey is optional... so the node should not be required, right? Yes, required. Please refer to the above explanation. > > > + > > +additionalProperties: false > > + > > +examples: > > + - | > > + #include <dt-bindings/interrupt-controller/irq.h> > > + > > + i2c { > > + #address-cells = <1>; > > + #size-cells = <0>; > > + > > + pmic@62 { > > + compatible = "richtek,rt5120"; > > + reg = <0x62>; > > + interrupts-extended = <&gpio_intc 32 IRQ_TYPE_LEVEL_LOW>; > > + interrupt-controller; > > + #interrupt-cells = <1>; > > + wakeup-source; > > + > > + regulators { > > + buck1 { > > + regulator-name = "rt5120-buck1"; > > + regulator-min-microvolt = <600000>; > > + regulator-max-microvolt = <1393750>; > > + regulator-allowed-modes = <0 1>; > > + regulator-boot-on; > > + }; > > + buck2 { > > + regulator-name = "rt5120-buck2"; > > + regulator-min-microvolt = <1100000>; > > + regulator-max-microvolt = <1100000>; > > + regulator-allowed-modes = <0 1>; > > + regulator-always-on; > > + }; > > + buck3 { > > + regulator-name = "rt5120-buck3"; > > + regulator-min-microvolt = <1800000>; > > + regulator-max-microvolt = <1800000>; > > + regulator-allowed-modes = <0 1>; > > + regulator-always-on; > > + }; > > + buck4 { > > + regulator-name = "rt5120-buck4"; > > + regulator-min-microvolt = <3300000>; > > + regulator-max-microvolt = <3300000>; > > + regulator-allowed-modes = <0 1>; > > + regulator-always-on; > > + }; > > + ldo { > > + regulator-name = "rt5120-ldo"; > > + regulator-min-microvolt = <1800000>; > > + regulator-max-microvolt = <1800000>; > > + regulator-always-on; > > + }; > > + exten { > > + regulator-name = "rt5120-exten"; > > + regulator-min-microvolt = <3000000>; > > + regulator-max-microvolt = <3000000>; > > + regulator-always-on; > > + }; > > + }; > > + powerkey { > > + status = "okay"; > > Messed up indentation. No need for status in examples. Fix in next. > > > + compatible = "richtek,rt5120-pwrkey"; > > + }; > > + }; > > + }; > > > Best regards, > Krzysztof
Mark Brown <broonie@kernel.org> 於 2022年6月8日 週三 上午3:00寫道： > > On Tue, Jun 07, 2022 at 01:52:40PM +0800, cy_huang wrote: > > This looks mostly good, a few things though: > > > +static void rt5120_fillin_regulator_desc(struct regulator_desc *desc, int rid) > > +{ > > + static const char * const name[] = { "buck1", "buck2", "buck3", "buck4", > > + "ldo", "exten" }; > > + static const char * const sname[] = { "vin1", "vin2", "vin3", "vin4", > > + "vinldo", NULL }; > > It would be easier and clearer to just make this a static table like > other drivers do, there's no need to generate anything dynamically as > far as I can see. My excuse. let me explain it. buck1 voltage range from 600mV to 1393.75mV. buck2~4/ldo/exten is the fixed regulator. buck3 and buck4 is fixed by the IC efuse default. buck2 and ldo is fixed by the external resistor chosen. exten is designed to connected to the external power. That's why I cannot directly declared it as the static regulator_desc. > > > +static int rt5120_of_parse_cb(struct rt5120_priv *priv, int rid, > > + struct of_regulator_match *match) > > +{ > > + struct regulator_desc *desc = priv->rdesc + rid; > > + struct regulator_init_data *init_data = match->init_data; > > + > > + if (!init_data || rid == RT5120_REGULATOR_BUCK1) > > + return 0; > > + > > + if (init_data->constraints.min_uV != init_data->constraints.max_uV) { > > + dev_err(priv->dev, "Variable voltage for fixed regulator\n"); > > + return -EINVAL; > > + } > > + > > + desc->fixed_uV = init_data->constraints.min_uV; > > + init_data->constraints.apply_uV = 0; > > Drivers should never override constraints passed in by machine drivers, > if there's validation needed let the core do it. The same probably > applies to providing a voltage range for a fixed regulator though that's > not modifying everything so not such a problem. Please check the above explanation about each power rails. > > > +static int rt5120_parse_regulator_dt_data(struct rt5120_priv *priv) > > +{ > > + struct device *dev = priv->dev->parent; > > + struct device_node *reg_node; > > + int i, ret; > > + > > + for (i = 0; i < RT5120_MAX_REGULATOR; i++) { > > + rt5120_fillin_regulator_desc(priv->rdesc + i, i); > > + > > + rt5120_regu_match[i].desc = priv->rdesc + i; > > + } > > Like I said above just make the list of regulators static data and loop > through registering them. Ditto > > > + > > + reg_node = of_get_child_by_name(dev->of_node, "regulators"); > > + if (!reg_node) { > > + dev_err(priv->dev, "Couldn't find 'regulators' node\n"); > > + return -ENODEV; > > + } > > + > > + ret = of_regulator_match(priv->dev, reg_node, rt5120_regu_match, > > + ARRAY_SIZE(rt5120_regu_match)); > > + > > + of_node_put(reg_node); > > + > > + if (ret < 0) { > > + dev_err(priv->dev, > > + "Error parsing regulator init data (%d)\n", ret); > > + return ret; > > + } > > + > > + for (i = 0; i < RT5120_MAX_REGULATOR; i++) { > > + ret = rt5120_of_parse_cb(priv, i, rt5120_regu_match + i); > > + if (ret) { > > + dev_err(priv->dev, "Failed in [%d] of_passe_cb\n", i); > > + return ret; > > + } > > + } > > This is all open coding stuff that's in the core - just provde an > of_parse_cb() operation and let the core take care of calling it. Ditto > > > +static int rt5120_device_property_init(struct rt5120_priv *priv) > > +{ > > + struct device *dev = priv->dev->parent; > > + bool prot_enable; > > + unsigned int prot_enable_val = 0; > > + > > + /* Assign UV/OV HW protection behavior */ > > + prot_enable = device_property_read_bool(dev, > > + "richtek,enable-undervolt-hiccup"); > > + if (prot_enable) > > + prot_enable_val |= RT5120_UVHICCUP_MASK; > > Use the DT APIs to parse DT - since ACPI has a very strong idea of how > power management works which is fundamentally incompatible with with the > DT model we should be writing code in a way that minimises the risk that > we'll end up trying to parse DT properties out of ACPI systems and > creating confusion as DT and ACPI software tries to run on the same > system. Ok, I'll replace it by DT API 'of_property_read_bool'.
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. Best regards, Krzysztof
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
On 08/06/2022 09:25, ChiYuan Huang wrote: > 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. I repeated my point twice - your description is not relevant to the bindings: "This description is not helpful, does not describe the hardware. Please describe hardware, not Devicetree usage." "System implementation - MFD, power off handling - is here (almost) irrelevant." "Don't mention in description of hardware anything about disabling it or not." Please describe briefly the hardware behind this property, not status or other Devicetree usage. Best regards, Krzysztof
On Wed, Jun 08, 2022 at 11:15:56AM +0800, ChiYuan Huang wrote: > Mark Brown <broonie@kernel.org> 於 2022年6月8日 週三 上午3:00寫道： > > On Tue, Jun 07, 2022 at 01:52:40PM +0800, cy_huang wrote: > > > + static const char * const name[] = { "buck1", "buck2", "buck3", "buck4", > > > + "ldo", "exten" }; > > > + static const char * const sname[] = { "vin1", "vin2", "vin3", "vin4", > > > + "vinldo", NULL }; > > It would be easier and clearer to just make this a static table like > > other drivers do, there's no need to generate anything dynamically as > > far as I can see. > My excuse. let me explain it. > buck1 voltage range from 600mV to 1393.75mV. > buck2~4/ldo/exten is the fixed regulator. > buck3 and buck4 is fixed by the IC efuse default. > buck2 and ldo is fixed by the external resistor chosen. > exten is designed to connected to the external power. > That's why I cannot directly declared it as the static regulator_desc. So buck 2-4 need some dynamic handling then but the rest can be static - that would be a lot clearer. You could also have a template for the ones with some dynamic values and just override the few fields that need it. > > > + if (init_data->constraints.min_uV != init_data->constraints.max_uV) { > > > + dev_err(priv->dev, "Variable voltage for fixed regulator\n"); > > > + return -EINVAL; > > > + } > > > + > > > + desc->fixed_uV = init_data->constraints.min_uV; > > > + init_data->constraints.apply_uV = 0; > > Drivers should never override constraints passed in by machine drivers, > > if there's validation needed let the core do it. The same probably > > applies to providing a voltage range for a fixed regulator though that's > > not modifying everything so not such a problem. > Please check the above explanation about each power rails. I'm not sure what you're referencing here? > > > + for (i = 0; i < RT5120_MAX_REGULATOR; i++) { > > > + ret = rt5120_of_parse_cb(priv, i, rt5120_regu_match + i); > > > + if (ret) { > > > + dev_err(priv->dev, "Failed in [%d] of_passe_cb\n", i); > > > + return ret; > > > + } > > > + } > > > > This is all open coding stuff that's in the core - just provde an > > of_parse_cb() operation and let the core take care of calling it. > Ditto Or here.
Mark Brown <broonie@kernel.org> 於 2022年6月8日 週三 下午6:12寫道： > > On Wed, Jun 08, 2022 at 11:15:56AM +0800, ChiYuan Huang wrote: > > Mark Brown <broonie@kernel.org> 於 2022年6月8日 週三 上午3:00寫道： > > > On Tue, Jun 07, 2022 at 01:52:40PM +0800, cy_huang wrote: > > > > > + static const char * const name[] = { "buck1", "buck2", "buck3", "buck4", > > > > + "ldo", "exten" }; > > > > + static const char * const sname[] = { "vin1", "vin2", "vin3", "vin4", > > > > + "vinldo", NULL }; > > > > It would be easier and clearer to just make this a static table like > > > other drivers do, there's no need to generate anything dynamically as > > > far as I can see. > > > My excuse. let me explain it. > > buck1 voltage range from 600mV to 1393.75mV. > > buck2~4/ldo/exten is the fixed regulator. > > buck3 and buck4 is fixed by the IC efuse default. > > buck2 and ldo is fixed by the external resistor chosen. > > exten is designed to connected to the external power. > > > That's why I cannot directly declared it as the static regulator_desc. > > So buck 2-4 need some dynamic handling then but the rest can be static - > that would be a lot clearer. You could also have a template for the > ones with some dynamic values and just override the few fields that need > it. > Not just buck2/3, buck2/3/4/ldo/exten all need the dynamic handling. > > > > + if (init_data->constraints.min_uV != init_data->constraints.max_uV) { > > > > + dev_err(priv->dev, "Variable voltage for fixed regulator\n"); > > > > + return -EINVAL; > > > > + } > > > > + > > > > + desc->fixed_uV = init_data->constraints.min_uV; > > > > + init_data->constraints.apply_uV = 0; > > > > Drivers should never override constraints passed in by machine drivers, > > > if there's validation needed let the core do it. The same probably > > > applies to providing a voltage range for a fixed regulator though that's > > > not modifying everything so not such a problem. > > > Please check the above explanation about each power rails. > > I'm not sure what you're referencing here? > Sorry. Let me explain it. You mean 'of_parse_cb' must not override constraint. But if the regulator is fixed and dynamic, after 'of_get_regulation_constraint', apply_uV will be true. The is referring to 'fixed.c' > > > > + for (i = 0; i < RT5120_MAX_REGULATOR; i++) { > > > > + ret = rt5120_of_parse_cb(priv, i, rt5120_regu_match + i); > > > > + if (ret) { > > > > + dev_err(priv->dev, "Failed in [%d] of_passe_cb\n", i); > > > > + return ret; > > > > + } > > > > + } > > > > > > This is all open coding stuff that's in the core - just provde an > > > of_parse_cb() operation and let the core take care of calling it. > > > Ditto > > Or here. If I put 'of_parce_cb' to make core handling it, the input parameter 'init_data' is declared as const. I cannot override the 'apply_uV'. Right?
On Thu, Jun 09, 2022 at 02:35:07PM +0800, ChiYuan Huang wrote: > Mark Brown <broonie@kernel.org> 於 2022年6月8日 週三 下午6:12寫道： > > On Wed, Jun 08, 2022 at 11:15:56AM +0800, ChiYuan Huang wrote: > > > My excuse. let me explain it. > > > buck1 voltage range from 600mV to 1393.75mV. > > > buck2~4/ldo/exten is the fixed regulator. > > > buck3 and buck4 is fixed by the IC efuse default. > > > buck2 and ldo is fixed by the external resistor chosen. > > > exten is designed to connected to the external power. > > > That's why I cannot directly declared it as the static regulator_desc. > > So buck 2-4 need some dynamic handling then but the rest can be static - > > that would be a lot clearer. You could also have a template for the > > ones with some dynamic values and just override the few fields that need > > it. > Not just buck2/3, buck2/3/4/ldo/exten all need the dynamic handling. Why do the others need it? > > > > Drivers should never override constraints passed in by machine drivers, > > > > if there's validation needed let the core do it. The same probably > > > > applies to providing a voltage range for a fixed regulator though that's > > > > not modifying everything so not such a problem. > > > Please check the above explanation about each power rails. > > I'm not sure what you're referencing here? > Sorry. Let me explain it. > You mean 'of_parse_cb' must not override constraint. > But if the regulator is fixed and dynamic, after > 'of_get_regulation_constraint', apply_uV will be true. > The is referring to 'fixed.c' fixed.c is a special case due to legacy issues and being generic, for normal fixed voltage regulators in a device where we know what they're fixed to they can just have their voltage hard coded in the driver. If there's issues with the machine providing invalid or nonsensical constraints the driver should just let the core deal with them. > > > > This is all open coding stuff that's in the core - just provde an > > > > of_parse_cb() operation and let the core take care of calling it. > > > Ditto > > Or here. > If I put 'of_parce_cb' to make core handling it, the input parameter > 'init_data' is declared as const. > I cannot override the 'apply_uV'. > Right? Yes, that's by design.
From: ChiYuan Huang <cy_huang@richtek.com> This patch series is to add Richtek RT5120 PMIC support. In RT5120, it integrates four channels of buck converter, one channel of LDO, and one external enable channel to control the external power source. ChiYuan Huang (4): dt-binding: mfd: Add Richtek RT5120 PMIC support mfd: rt5120: Add Richtek PMIC support regulator: rt5120: Add PMIC regulator support input: misc: rt5120: Add power key support .../devicetree/bindings/mfd/richtek,rt5120.yaml | 180 +++++++++ drivers/input/misc/Kconfig | 9 + drivers/input/misc/Makefile | 1 + drivers/input/misc/rt5120-pwrkey.c | 115 ++++++ drivers/mfd/Kconfig | 12 + drivers/mfd/Makefile | 1 + drivers/mfd/rt5120.c | 125 ++++++ drivers/regulator/Kconfig | 10 + drivers/regulator/Makefile | 1 + drivers/regulator/rt5120-regulator.c | 417 +++++++++++++++++++++ 10 files changed, 871 insertions(+) create mode 100644 Documentation/devicetree/bindings/mfd/richtek,rt5120.yaml create mode 100644 drivers/input/misc/rt5120-pwrkey.c create mode 100644 drivers/mfd/rt5120.c create mode 100644 drivers/regulator/rt5120-regulator.c