diff mbox series

[v4,1/2] regulator: rt6160: Add DT binding document for Richtek RT6160

Message ID 1622008068-13474-1-git-send-email-u0084500@gmail.com
State Superseded
Headers show
Series [v4,1/2] regulator: rt6160: Add DT binding document for Richtek RT6160 | expand

Commit Message

cy_huang May 26, 2021, 5:47 a.m. UTC 
From: ChiYuan Huang <cy_huang@richtek.com>

Add DT binding document for Richtek RT6160 voltage regulator.

Signed-off-by: ChiYuan Huang <cy_huang@richtek.com>
Reviewed-by: Rob Herring <robh@kernel.org>
---
since v4
- Add Reviewed-by.

since v3
- Move all regulator related properties to the upper node.

since v2
- Move buckboost node from patternProperties to Properties.
---
 .../regulator/richtek,rt6160-regulator.yaml        | 61 ++++++++++++++++++++++
 1 file changed, 61 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/regulator/richtek,rt6160-regulator.yaml

Comments

Mark Brown May 26, 2021, 10:51 a.m. UTC | #1 
On Wed, May 26, 2021 at 01:47:48PM +0800, cy_huang wrote:

This looks mostly good, a few small issues below:

> +static int rt6160_set_suspend_voltage(struct regulator_dev *rdev, int uV)
> +{
> +	struct rt6160_priv *priv = rdev_get_drvdata(rdev);
> +	struct regmap *regmap = rdev_get_regmap(rdev);
> +	unsigned int reg = RT6160_REG_VSELH;
> +	int vsel;
> +
> +	vsel = regulator_map_voltage_linear(rdev, uV, uV);
> +	if (vsel < 0)
> +		return vsel;
> +
> +	if (priv->vsel_active_low)
> +		reg = RT6160_REG_VSELL;
> +
> +	return regmap_update_bits(regmap, reg, RT6160_VSEL_MASK, vsel);
> +}

This seems to just be updating the normal voltage configuration
regulator, the suspend mode operations are there for devices that
have a hardware suspend mode that's entered as part of the very
low level system suspend process.

> +static int rt6160_set_ramp_delay(struct regulator_dev *rdev, int ramp_delay)
> +{
> +	struct regmap *regmap = rdev_get_regmap(rdev);
> +	unsigned int ramp_value = RT6160_RAMPRATE_1VMS;
> +
> +	switch (ramp_delay) {
> +	case 1 ... 1000:
> +		ramp_value = RT6160_RAMPRATE_1VMS;
> +		break;

This looks like it could be converted to regulator_set_ramp_delay_regmap()

> +static unsigned int rt6160_of_map_mode(unsigned int mode)
> +{
> +	if (mode == RT6160_MODE_FPWM)
> +		return REGULATOR_MODE_FAST;
> +	else if (mode == RT6160_MODE_AUTO)
> +		return REGULATOR_MODE_NORMAL;
> +

This would be more idiomatically written as a switch statement.

> +	enable_gpio = devm_gpiod_get_optional(&i2c->dev, "enable", GPIOD_OUT_HIGH);
> +	if (IS_ERR(enable_gpio)) {
> +		dev_err(&i2c->dev, "Failed to get 'enable' gpio\n");
> +		return PTR_ERR(enable_gpio);
> +	}

There's no other references to enable_gpio?

> +	regmap = devm_regmap_init_i2c(i2c, &rt6160_regmap_config);
> +	if (IS_ERR(regmap)) {
> +		dev_err(&i2c->dev, "Failed to init regmap\n");
> +		return PTR_ERR(regmap);
> +	}

It's better to print the error code to help anyone who runs into
issues figure out what's wrong.
cy_huang May 26, 2021, 3:04 p.m. UTC | #2 
HI:

Mark Brown <broonie@kernel.org> 於 2021年5月26日 週三 下午6:50寫道：
>
> On Wed, May 26, 2021 at 01:47:48PM +0800, cy_huang wrote:
>
> This looks mostly good, a few small issues below:
>
> > +static int rt6160_set_suspend_voltage(struct regulator_dev *rdev, int uV)
> > +{
> > +     struct rt6160_priv *priv = rdev_get_drvdata(rdev);
> > +     struct regmap *regmap = rdev_get_regmap(rdev);
> > +     unsigned int reg = RT6160_REG_VSELH;
> > +     int vsel;
> > +
> > +     vsel = regulator_map_voltage_linear(rdev, uV, uV);
> > +     if (vsel < 0)
> > +             return vsel;
> > +
> > +     if (priv->vsel_active_low)
> > +             reg = RT6160_REG_VSELL;
> > +
> > +     return regmap_update_bits(regmap, reg, RT6160_VSEL_MASK, vsel);
> > +}
>
> This seems to just be updating the normal voltage configuration
> regulator, the suspend mode operations are there for devices that
> have a hardware suspend mode that's entered as part of the very
> low level system suspend process.
>
There's a independent 'vsel' pin. It depend on user's HW design.
And that's why there's a 'richtek,vsel_active_low' property.
Its normal application is to use vsel high active level, and it means
the opposite level can be used for the suspend voltage

And there're also two voltage registers for vsel level high and low.
> > +static int rt6160_set_ramp_delay(struct regulator_dev *rdev, int ramp_delay)
> > +{
> > +     struct regmap *regmap = rdev_get_regmap(rdev);
> > +     unsigned int ramp_value = RT6160_RAMPRATE_1VMS;
> > +
> > +     switch (ramp_delay) {
> > +     case 1 ... 1000:
> > +             ramp_value = RT6160_RAMPRATE_1VMS;
> > +             break;
>
> This looks like it could be converted to regulator_set_ramp_delay_regmap()
>
I didn't notice there's the regulator_set_ramp_delay_regmap API that
can be used in kernel 5.13.
Ack in next.
> > +static unsigned int rt6160_of_map_mode(unsigned int mode)
> > +{
> > +     if (mode == RT6160_MODE_FPWM)
> > +             return REGULATOR_MODE_FAST;
> > +     else if (mode == RT6160_MODE_AUTO)
> > +             return REGULATOR_MODE_NORMAL;
> > +
>
> This would be more idiomatically written as a switch statement.
>
Ack in next. Change the if-else to switch case. Thx.
> > +     enable_gpio = devm_gpiod_get_optional(&i2c->dev, "enable", GPIOD_OUT_HIGH);
> > +     if (IS_ERR(enable_gpio)) {
> > +             dev_err(&i2c->dev, "Failed to get 'enable' gpio\n");
> > +             return PTR_ERR(enable_gpio);
> > +     }
>
> There's no other references to enable_gpio?
>
The IC is designed for low IQ.
So from the driver probe, I only need to keep 'enable' pin high.
Or if user specify the 'enable' gpio, it will block i2c communication,
register also be reset,
and add more delay time on enable/disable.
That's why there's no other references to 'enable' gpio.
> > +     regmap = devm_regmap_init_i2c(i2c, &rt6160_regmap_config);
> > +     if (IS_ERR(regmap)) {
> > +             dev_err(&i2c->dev, "Failed to init regmap\n");
> > +             return PTR_ERR(regmap);
> > +     }
>
> It's better to print the error code to help anyone who runs into
> issues figure out what's wrong.
Sure, change it to dev_err(&i2c->dev, "Failed to init regmap (%d)\n",
PTR_ERR(regmap));
Ack in next, thx.
cy_huang May 27, 2021, 3:14 a.m. UTC | #3 
HI, Mark:

ChiYuan Huang <u0084500@gmail.com> 於 2021年5月26日 週三 下午11:04寫道：
>

> HI:

>

> Mark Brown <broonie@kernel.org> 於 2021年5月26日 週三 下午6:50寫道：

> >

> > On Wed, May 26, 2021 at 01:47:48PM +0800, cy_huang wrote:

> >

> > This looks mostly good, a few small issues below:

> >

> > > +static int rt6160_set_suspend_voltage(struct regulator_dev *rdev, int uV)

> > > +{

> > > +     struct rt6160_priv *priv = rdev_get_drvdata(rdev);

> > > +     struct regmap *regmap = rdev_get_regmap(rdev);

> > > +     unsigned int reg = RT6160_REG_VSELH;

> > > +     int vsel;

> > > +

> > > +     vsel = regulator_map_voltage_linear(rdev, uV, uV);

> > > +     if (vsel < 0)

> > > +             return vsel;

> > > +

> > > +     if (priv->vsel_active_low)

> > > +             reg = RT6160_REG_VSELL;

> > > +

> > > +     return regmap_update_bits(regmap, reg, RT6160_VSEL_MASK, vsel);

> > > +}

> >

> > This seems to just be updating the normal voltage configuration

> > regulator, the suspend mode operations are there for devices that

> > have a hardware suspend mode that's entered as part of the very

> > low level system suspend process.

> >

> There's a independent 'vsel' pin. It depend on user's HW design.

> And that's why there's a 'richtek,vsel_active_low' property.

> Its normal application is to use vsel high active level, and it means

> the opposite level can be used for the suspend voltage

>

> And there're also two voltage registers for vsel level high and low.

> > > +static int rt6160_set_ramp_delay(struct regulator_dev *rdev, int ramp_delay)

> > > +{

> > > +     struct regmap *regmap = rdev_get_regmap(rdev);

> > > +     unsigned int ramp_value = RT6160_RAMPRATE_1VMS;

> > > +

> > > +     switch (ramp_delay) {

> > > +     case 1 ... 1000:

> > > +             ramp_value = RT6160_RAMPRATE_1VMS;

> > > +             break;

> >

> > This looks like it could be converted to regulator_set_ramp_delay_regmap()

> >

> I didn't notice there's the regulator_set_ramp_delay_regmap API that

> can be used in kernel 5.13.u

> Ack in next.


I review the regulator_set_ramp_delay_regmap API.
If seems I need to fill in the ramp_delay_table by the descend order.
But this chip ramp delay table is designed the ascending value reg bit
field [0 1 2 3] by
the ascending order [1000 2500 5000 10000] uV/uS
Even if I tried to filler in descending order, I also need a inverted operation.

And I found the regulator_set_ramp_delay_regmap API has some logic error.
From the include/linux/regulator/driver.h, the set_ramp_delay function says to
set the less or equal one ramp delay value.
But your logic will get the larger or equal one from the descending
ramp delay table.

Could you help to check about this?
> > > +static unsigned int rt6160_of_map_mode(unsigned int mode)

> > > +{

> > > +     if (mode == RT6160_MODE_FPWM)

> > > +             return REGULATOR_MODE_FAST;

> > > +     else if (mode == RT6160_MODE_AUTO)

> > > +             return REGULATOR_MODE_NORMAL;

> > > +

> >

> > This would be more idiomatically written as a switch statement.

> >

> Ack in next. Change the if-else to switch case. Thx.

> > > +     enable_gpio = devm_gpiod_get_optional(&i2c->dev, "enable", GPIOD_OUT_HIGH);

> > > +     if (IS_ERR(enable_gpio)) {

> > > +             dev_err(&i2c->dev, "Failed to get 'enable' gpio\n");

> > > +             return PTR_ERR(enable_gpio);

> > > +     }

> >

> > There's no other references to enable_gpio?

> >

> The IC is designed for low IQ.

> So from the driver probe, I only need to keep 'enable' pin high.

> Or if user specify the 'enable' gpio, it will block i2c communication,

> register also be reset,

> and add more delay time on enable/disable.

> That's why there's no other references to 'enable' gpio.

> > > +     regmap = devm_regmap_init_i2c(i2c, &rt6160_regmap_config);

> > > +     if (IS_ERR(regmap)) {

> > > +             dev_err(&i2c->dev, "Failed to init regmap\n");

> > > +             return PTR_ERR(regmap);

> > > +     }

> >

> > It's better to print the error code to help anyone who runs into

> > issues figure out what's wrong.

> Sure, change it to dev_err(&i2c->dev, "Failed to init regmap (%d)\n",

> PTR_ERR(regmap));

> Ack in next, thx.
Mark Brown June 1, 2021, 3:52 p.m. UTC | #4 
On Thu, May 27, 2021 at 11:14:17AM +0800, ChiYuan Huang wrote:

> I review the regulator_set_ramp_delay_regmap API.

> If seems I need to fill in the ramp_delay_table by the descend order.

> But this chip ramp delay table is designed the ascending value reg bit

> field [0 1 2 3] by

> the ascending order [1000 2500 5000 10000] uV/uS

> Even if I tried to filler in descending order, I also need a inverted operation.


I see... that really should be supportable, and I'd have expected
find_closest_bigger() to DTRT here, it's not obvious it's expecting
ordering.

> And I found the regulator_set_ramp_delay_regmap API has some logic error.

> From the include/linux/regulator/driver.h, the set_ramp_delay function says to

> set the less or equal one ramp delay value.

> But your logic will get the larger or equal one from the descending

> ramp delay table.


The code is correct here, the documentation should be fixed - with a
delay like this we should be erring on the side of delaying too long to
be safe.
cy_huang June 2, 2021, 1:48 a.m. UTC | #5 
HI, Mark:

Mark Brown <broonie@kernel.org> 於 2021年6月1日 週二 下午11:52寫道：
>

> On Thu, May 27, 2021 at 11:14:17AM +0800, ChiYuan Huang wrote:

>

> > I review the regulator_set_ramp_delay_regmap API.

> > If seems I need to fill in the ramp_delay_table by the descend order.

> > But this chip ramp delay table is designed the ascending value reg bit

> > field [0 1 2 3] by

> > the ascending order [1000 2500 5000 10000] uV/uS

> > Even if I tried to filler in descending order, I also need a inverted operation.

>

> I see... that really should be supportable, and I'd have expected

> find_closest_bigger() to DTRT here, it's not obvious it's expecting

> ordering.

>

> > And I found the regulator_set_ramp_delay_regmap API has some logic error.

> > From the include/linux/regulator/driver.h, the set_ramp_delay function says to

> > set the less or equal one ramp delay value.

> > But your logic will get the larger or equal one from the descending

> > ramp delay table.

>

> The code is correct here, the documentation should be fixed - with a

> delay like this we should be erring on the side of delaying too long to

> be safe.


If so, I will upload v7 patch series to meet this selection logic.
Thanks.
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/regulator/richtek,rt6160-regulator.yaml b/Documentation/devicetree/bindings/regulator/richtek,rt6160-regulator.yaml
new file mode 100644
index 00000000..0534b0d
--- /dev/null
+++ b/Documentation/devicetree/bindings/regulator/richtek,rt6160-regulator.yaml
@@ -0,0 +1,61 @@ 
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/regulator/richtek,rt6160-regulator.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Richtek RT6160 BuckBoost converter
+
+maintainers:
+  - ChiYuan Huang <cy_huang@richtek.com>
+
+description: |
+  The RT6160 is a high-efficiency buck-boost converter that can provide
+  up to 3A output current from 2025mV to 5200mV. And it support the wide
+  input voltage range from 2200mV to 5500mV.
+
+  Datasheet is available at
+  https://www.richtek.com/assets/product_file/RT6160A/DS6160A-00.pdf
+
+allOf:
+  - $ref: regulator.yaml#
+
+properties:
+  compatible:
+    enum:
+      - richtek,rt6160
+
+  reg:
+    maxItems: 1
+
+  enable-gpios:
+    description: A connection of the 'enable' gpio line.
+    maxItems: 1
+
+  richtek,vsel-active-low:
+    description: |
+      Used to indicate the 'vsel' pin active level. if not specified, use
+      high active level as the default.
+    type: boolean
+
+required:
+  - compatible
+  - reg
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    i2c {
+      #address-cells = <1>;
+      #size-cells = <0>;
+
+      rt6160@75 {
+        compatible = "richtek,rt6160";
+        reg = <0x75>;
+        enable-gpios = <&gpio26 2 0>;
+        regulator-name = "rt6160-buckboost";
+        regulator-min-microvolt = <2025000>;
+        regulator-max-microvolt = <5200000>;
+      };
+    };