[PATCH 0/2] Add Richtek RT5739 switching voltage regulator support

[PATCH 0/2] Add Richtek RT5739 switching voltage regulator support

[cy_huang]

From: ChiYuan Huang <cy_huang@richtek.com>

This patch set is to add Richtek RT5739 regulator support.

The RT5739 is a step-down switching voltage regulator that delivers a digitally
programmable output from an input voltage supply from 2.5V to 5.5V. The output
voltage is programmed through an I2C interface capable of operating up to
3.4MHz.

ChiYuan Huang (2):
  dt-bindings: regulator: Add bindings for Richtek RT5739
  regulator: Add support for Richtek RT5739 voltage regulator

 .../bindings/regulator/richtek,rt5739.yaml         |  80 ++++++
 drivers/regulator/Kconfig                          |  13 +
 drivers/regulator/Makefile                         |   1 +
 drivers/regulator/rt5739.c                         | 290 +++++++++++++++++++++
 4 files changed, 384 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/regulator/richtek,rt5739.yaml
 create mode 100644 drivers/regulator/rt5739.c

-- 
2.7.4

[PATCH 1/2] dt-bindings: regulator: Add bindings for Richtek RT5739

[cy_huang]

From: ChiYuan Huang <cy_huang@richtek.com>

Add the binding document for Richtek RT5739.

Signed-off-by: ChiYuan Huang <cy_huang@richtek.com>
---
 .../bindings/regulator/richtek,rt5739.yaml         | 80 ++++++++++++++++++++++
 1 file changed, 80 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/regulator/richtek,rt5739.yaml

diff --git a/Documentation/devicetree/bindings/regulator/richtek,rt5739.yaml b/Documentation/devicetree/bindings/regulator/richtek,rt5739.yaml
new file mode 100644
index 00000000..7dc4f78
--- /dev/null
+++ b/Documentation/devicetree/bindings/regulator/richtek,rt5739.yaml
@@ -0,0 +1,80 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/regulator/richtek,rt5739.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Richtek RT5739 2.4MHz 3.5A Step-Down Converter
+
+maintainers:
+  - ChiYuan Huang <cy_huang@richtek.com>
+
+description: |
+  The RT5739 is a step-down switching voltage regulator that delivers a
+  digitally programmable output from an input voltage supply of 2.5V to 5.5V.
+  The output voltage is programmed through an I2C interface capable of
+  operating up to 3.4MHz.
+
+  Using a proprietary architecture with synchronous rectification, the RT5739
+  is capable of delivering 3.5A continuously at over 80% efficiency,
+  maintaining that efficiency at load current as low as 10mA. The regulator
+  operates at a normal fixed frequency of 2.4MHz, which reduces the value of
+  the external components. Additional output capacitance can be added to
+  improve regulation during load transients without affecting stability.
+
+allOf:
+  - $ref: regulator.yaml#
+
+properties:
+  compatible:
+    enum:
+      - richtek,rt5739
+
+  reg:
+    maxItems: 1
+
+  enable-gpios:
+    maxItems: 1
+
+  richtek,vsel-active-high:
+    description: |
+      If property is present, use the 'VSEL1' register group for buck control.
+      Else, use the 'VSEL0' register group. This depends on external hardware
+      'VSEL' pin connecton.
+    type: boolean
+
+  regulator-allowed-modes:
+    description: |
+      buck allowed operating mode
+        0: Auto PFM/PWM mode
+        1: Forced PWM mode
+    maxItems: 2
+    items:
+      enum: [0, 1]
+
+required:
+  - compatible
+  - reg
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+
+    i2c {
+      #address-cells = <1>;
+      #size-cells = <0>;
+
+      regulator@50 {
+        compatible = "richtek,rt5739";
+        reg = <0x50>;
+        enable-gpios = <&gpio26 1 GPIO_ACTIVE_HIGH>;
+        richtek,vsel-active-high;
+        regulator-name = "richtek,rt5739-buck";
+        regulator-min-microvolt = <3000000>;
+        regulator-max-microvolt = <13000000>;
+        regulator-allowed-modes = <0 1>;
+        regulator-boot-on;
+      };
+    };
-- 
2.7.4

[PATCH 2/2] regulator: Add support for Richtek RT5739 voltage regulator

[cy_huang]

From: ChiYuan Huang <cy_huang@richtek.com>

The RT5739 is a step-down switching voltage regulator that supports
output voltage ragne from 300mV to 1300mV with the wide input supply
voltage range from 2.5V to 5.5V.

Signed-off-by: ChiYuan Huang <cy_huang@richtek.com>
---
 drivers/regulator/Kconfig  |  13 ++
 drivers/regulator/Makefile |   1 +
 drivers/regulator/rt5739.c | 290 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 304 insertions(+)
 create mode 100644 drivers/regulator/rt5739.c

diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
index 820c9a0..f7f992c 100644
--- a/drivers/regulator/Kconfig
+++ b/drivers/regulator/Kconfig
@@ -1112,6 +1112,19 @@ config REGULATOR_RT5190A
 	  buck converters, 1 LDO, mute AC OFF depop function, with the general
 	  I2C control interface.
 
+config REGULATOR_RT5739
+	tristate "Rcihtek RT5739 Regulator"
+	depends on I2C
+	select REGMAP_I2C
+	help
+	  This adds support for voltage regulator in Richtek RT5739.
+	  It's a step-down switching voltage regulator. Using a proprietary
+	  architecture with synchronous rectification, it is capable of
+	  delivering 3.5A continuously at over 80% efficiency.
+
+	  This driver can also be built as a module. If so, the module
+	  will be called rt5739.
+
 config REGULATOR_RT5759
 	tristate "Richtek RT5759 Regulator"
 	depends on I2C
diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
index b9f5eb3..30a61fc 100644
--- a/drivers/regulator/Makefile
+++ b/drivers/regulator/Makefile
@@ -133,6 +133,7 @@ obj-$(CONFIG_REGULATOR_RT4831)	+= rt4831-regulator.o
 obj-$(CONFIG_REGULATOR_RT5033)	+= rt5033-regulator.o
 obj-$(CONFIG_REGULATOR_RT5120)	+= rt5120-regulator.o
 obj-$(CONFIG_REGULATOR_RT5190A) += rt5190a-regulator.o
+obj-$(CONFIG_REGULATOR_RT5739)	+= rt5739.o
 obj-$(CONFIG_REGULATOR_RT5759)	+= rt5759-regulator.o
 obj-$(CONFIG_REGULATOR_RT6160)	+= rt6160-regulator.o
 obj-$(CONFIG_REGULATOR_RT6190)	+= rt6190-regulator.o
diff --git a/drivers/regulator/rt5739.c b/drivers/regulator/rt5739.c
new file mode 100644
index 00000000..0a9e102
--- /dev/null
+++ b/drivers/regulator/rt5739.c
@@ -0,0 +1,290 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Device driver for RT5739 regulator
+ *
+ * Copyright (C) 2023 Richtek Technology Corp.
+ *
+ * Author: ChiYuan Huang <cy_huang@richtek.com>
+ */
+
+#include <linux/bits.h>
+#include <linux/gpio/consumer.h>
+#include <linux/i2c.h>
+#include <linux/kernel.h>
+#include <linux/mod_devicetable.h>
+#include <linux/property.h>
+#include <linux/regmap.h>
+#include <linux/regulator/driver.h>
+#include <linux/regulator/of_regulator.h>
+
+#define RT5739_AUTO_MODE	0
+#define RT5739_FPWM_MODE	1
+
+#define RT5739_REG_NSEL0	0x00
+#define RT5739_REG_NSEL1	0x01
+#define RT5739_REG_CNTL1	0x02
+#define RT5739_REG_ID1		0x03
+#define RT5739_REG_CNTL2	0x06
+#define RT5739_REG_CNTL4	0x08
+
+#define RT5739_VSEL_MASK	GENMASK(7, 0)
+#define RT5739_MODEVSEL1_MASK	BIT(1)
+#define RT5739_MODEVSEL0_MASK	BIT(0)
+#define RT5739_VID_MASK		GENMASK(7, 5)
+#define RT5739_ACTD_MASK	BIT(7)
+#define RT5739_ENVSEL1_MASK	BIT(1)
+#define RT5739_ENVSEL0_MASK	BIT(0)
+
+#define RT5739_VOLT_MINUV	300000
+#define RT5739_VOLT_MAXUV	1300000
+#define RT5739_VOLT_STPUV	5000
+#define RT5739_N_VOLTS		201
+#define RT5739_I2CRDY_TIMEUS	1000
+
+static int rt5739_set_mode(struct regulator_dev *rdev, unsigned int mode)
+{
+	const struct regulator_desc *desc = rdev->desc;
+	struct regmap *regmap = rdev_get_regmap(rdev);
+	unsigned int mask, val;
+
+	if (desc->vsel_reg == RT5739_REG_NSEL0)
+		mask = RT5739_MODEVSEL0_MASK;
+	else
+		mask = RT5739_MODEVSEL1_MASK;
+
+	switch (mode) {
+	case REGULATOR_MODE_FAST:
+		val = mask;
+		break;
+	case REGULATOR_MODE_NORMAL:
+		val = 0;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return regmap_update_bits(regmap, RT5739_REG_CNTL1, mask, val);
+}
+
+static unsigned int rt5739_get_mode(struct regulator_dev *rdev)
+{
+	const struct regulator_desc *desc = rdev->desc;
+	struct regmap *regmap = rdev_get_regmap(rdev);
+	unsigned int mask, val;
+	int ret;
+
+	if (desc->vsel_reg == RT5739_REG_NSEL0)
+		mask = RT5739_MODEVSEL0_MASK;
+	else
+		mask = RT5739_MODEVSEL1_MASK;
+
+	ret = regmap_read(regmap, RT5739_REG_CNTL1, &val);
+	if (ret)
+		return REGULATOR_MODE_INVALID;
+
+	if (val & mask)
+		return REGULATOR_MODE_FAST;
+
+	return REGULATOR_MODE_NORMAL;
+}
+
+static int rt5739_set_suspend_voltage(struct regulator_dev *rdev, int uV)
+{
+	const struct regulator_desc *desc = rdev->desc;
+	struct regmap *regmap = rdev_get_regmap(rdev);
+	unsigned int reg, vsel;
+
+	if (uV < RT5739_VOLT_MINUV || uV > RT5739_VOLT_MAXUV)
+		return -EINVAL;
+
+	if (desc->vsel_reg == RT5739_REG_NSEL0)
+		reg = RT5739_REG_NSEL1;
+	else
+		reg = RT5739_REG_NSEL0;
+
+	vsel = (uV - RT5739_VOLT_MINUV) / RT5739_VOLT_STPUV;
+	return regmap_write(regmap, reg, vsel);
+}
+
+static int rt5739_set_suspend_enable(struct regulator_dev *rdev)
+{
+	const struct regulator_desc *desc = rdev->desc;
+	struct regmap *regmap = rdev_get_regmap(rdev);
+	unsigned int mask;
+
+	if (desc->vsel_reg == RT5739_REG_NSEL0)
+		mask = RT5739_ENVSEL1_MASK;
+	else
+		mask = RT5739_ENVSEL0_MASK;
+
+	return regmap_update_bits(regmap, desc->enable_reg, mask, mask);
+}
+
+static int rt5739_set_suspend_disable(struct regulator_dev *rdev)
+{
+	const struct regulator_desc *desc = rdev->desc;
+	struct regmap *regmap = rdev_get_regmap(rdev);
+	unsigned int mask;
+
+	if (desc->vsel_reg == RT5739_REG_NSEL0)
+		mask = RT5739_ENVSEL1_MASK;
+	else
+		mask = RT5739_ENVSEL0_MASK;
+
+	return regmap_update_bits(regmap, desc->enable_reg, mask, 0);
+}
+
+static int rt5739_set_suspend_mode(struct regulator_dev *rdev,
+				   unsigned int mode)
+{
+	const struct regulator_desc *desc = rdev->desc;
+	struct regmap *regmap = rdev_get_regmap(rdev);
+	unsigned int mask, val;
+
+	if (desc->vsel_reg == RT5739_REG_NSEL0)
+		mask = RT5739_MODEVSEL1_MASK;
+	else
+		mask = RT5739_MODEVSEL0_MASK;
+
+	switch (mode) {
+	case REGULATOR_MODE_FAST:
+		val = mask;
+		break;
+	case REGULATOR_MODE_NORMAL:
+		val = 0;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return regmap_update_bits(regmap, RT5739_REG_CNTL1, mask, val);
+}
+
+static const struct regulator_ops rt5739_regulator_ops = {
+	.list_voltage = regulator_list_voltage_linear,
+	.get_voltage_sel = regulator_get_voltage_sel_regmap,
+	.set_voltage_sel = regulator_set_voltage_sel_regmap,
+	.enable	= regulator_enable_regmap,
+	.disable = regulator_disable_regmap,
+	.is_enabled = regulator_is_enabled_regmap,
+	.set_active_discharge = regulator_set_active_discharge_regmap,
+	.set_mode = rt5739_set_mode,
+	.get_mode = rt5739_get_mode,
+	.set_suspend_voltage = rt5739_set_suspend_voltage,
+	.set_suspend_enable = rt5739_set_suspend_enable,
+	.set_suspend_disable = rt5739_set_suspend_disable,
+	.set_suspend_mode = rt5739_set_suspend_mode,
+};
+
+static unsigned int rt5739_of_map_mode(unsigned int mode)
+{
+	switch (mode) {
+	case RT5739_AUTO_MODE:
+		return REGULATOR_MODE_NORMAL;
+	case RT5739_FPWM_MODE:
+		return REGULATOR_MODE_FAST;
+	default:
+		return REGULATOR_MODE_INVALID;
+	}
+}
+
+static void rt5739_init_regulator_desc(struct regulator_desc *desc,
+				       bool vsel_active_high)
+{
+	/* Fixed */
+	desc->name = "rt5739-regulator";
+	desc->owner = THIS_MODULE;
+	desc->ops = &rt5739_regulator_ops;
+	desc->n_voltages = RT5739_N_VOLTS;
+	desc->min_uV = RT5739_VOLT_MINUV;
+	desc->uV_step = RT5739_VOLT_STPUV;
+	desc->vsel_mask = RT5739_VSEL_MASK;
+	desc->enable_reg = RT5739_REG_CNTL2;
+	desc->active_discharge_reg = RT5739_REG_CNTL1;
+	desc->active_discharge_mask = RT5739_ACTD_MASK;
+	desc->active_discharge_on = RT5739_ACTD_MASK;
+	desc->of_map_mode = rt5739_of_map_mode;
+
+	/* Assigned by vsel level */
+	if (vsel_active_high) {
+		desc->vsel_reg = RT5739_REG_NSEL1;
+		desc->enable_mask = RT5739_ENVSEL1_MASK;
+	} else {
+		desc->vsel_reg = RT5739_REG_NSEL0;
+		desc->enable_mask = RT5739_ENVSEL0_MASK;
+	}
+}
+
+static const struct regmap_config rt5739_regmap_config = {
+	.name = "rt5739",
+	.reg_bits = 8,
+	.val_bits = 8,
+	.max_register = RT5739_REG_CNTL4,
+};
+
+static int rt5739_probe(struct i2c_client *i2c)
+{
+	struct device *dev = &i2c->dev;
+	struct regulator_desc *desc;
+	struct regmap *regmap;
+	struct gpio_desc *enable_gpio;
+	struct regulator_config cfg = {};
+	struct regulator_dev *rdev;
+	bool vsel_acth;
+	unsigned int vid;
+	int ret;
+
+	desc = devm_kzalloc(dev, sizeof(*desc), GFP_KERNEL);
+	if (!desc)
+		return -ENOMEM;
+
+	enable_gpio = devm_gpiod_get_optional(dev, "enable", GPIOD_OUT_HIGH);
+	if (IS_ERR(enable_gpio))
+		return dev_err_probe(dev, PTR_ERR(enable_gpio), "Failed to get 'enable' gpio\n");
+	else if (enable_gpio)
+		usleep_range(RT5739_I2CRDY_TIMEUS, RT5739_I2CRDY_TIMEUS + 1000);
+
+	regmap = devm_regmap_init_i2c(i2c, &rt5739_regmap_config);
+	if (IS_ERR(regmap))
+		return dev_err_probe(dev, PTR_ERR(regmap), "Failed to init regmap\n");
+
+	ret = regmap_read(regmap, RT5739_REG_ID1, &vid);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to read VID\n");
+
+	/* RT5739: (VID & MASK) must be 0 */
+	if (vid & RT5739_VID_MASK)
+		return dev_err_probe(dev, -ENODEV, "Incorrect VID (0x%02x)\n", vid);
+
+	vsel_acth = device_property_read_bool(dev, "richtek,vsel-active-high");
+
+	rt5739_init_regulator_desc(desc, vsel_acth);
+
+	cfg.dev = dev;
+	cfg.of_node = dev_of_node(dev);
+	cfg.init_data = of_get_regulator_init_data(dev, dev_of_node(dev), desc);
+	rdev = devm_regulator_register(dev, desc, &cfg);
+	if (IS_ERR(rdev))
+		return dev_err_probe(dev, PTR_ERR(rdev), "Failed to register regulator\n");
+
+	return 0;
+}
+
+static const struct of_device_id rt5739_device_table[] = {
+	{ .compatible = "richtek,rt5739" },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, rt5739_device_table);
+
+static struct i2c_driver rt5739_driver = {
+	.driver = {
+		.name = "rt5739",
+		.of_match_table = rt5739_device_table,
+	},
+	.probe_new = rt5739_probe,
+};
+module_i2c_driver(rt5739_driver);
+
+MODULE_AUTHOR("ChiYuan Huang <cy_huang@richtek.com>");
+MODULE_DESCRIPTION("Richtek RT5739 regulator driver");
+MODULE_LICENSE("GPL");
-- 
2.7.4

Re: [PATCH 1/2] dt-bindings: regulator: Add bindings for Richtek RT5739

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 799 bytes --]

On Wed, Feb 15, 2023 at 10:00:56AM +0800, cy_huang@richtek.com wrote:

> +  richtek,vsel-active-high:
> +    description: |
> +      If property is present, use the 'VSEL1' register group for buck control.
> +      Else, use the 'VSEL0' register group. This depends on external hardware
> +      'VSEL' pin connecton.
> +    type: boolean

I would expect this to be GPIO controlled rather than fixed in
the design, at least for some systems?  The name suggests it's
supposed to control the polarity of a GPIO too.  Sadly we don't
really have good infrastructure for fixed GPIOs AFAIK...  ideally
this would be a GPIO and then if we need to hold it high then
there would be some binding we could connect to the GPIO that
says that.  We don't have that though so for now a fixed property
might be OK.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/2] dt-bindings: regulator: Add bindings for Richtek RT5739

[ChiYuan Huang]

Hi,

Mark Brown <broonie@kernel.org> 於 2023年2月15日 週三 下午9:03寫道：
>
> On Wed, Feb 15, 2023 at 10:00:56AM +0800, cy_huang@richtek.com wrote:
>
> > +  richtek,vsel-active-high:
> > +    description: |
> > +      If property is present, use the 'VSEL1' register group for buck control.
> > +      Else, use the 'VSEL0' register group. This depends on external hardware
> > +      'VSEL' pin connecton.
> > +    type: boolean
>
> I would expect this to be GPIO controlled rather than fixed in
> the design, at least for some systems?  The name suggests it's
> supposed to control the polarity of a GPIO too.  Sadly we don't
> really have good infrastructure for fixed GPIOs AFAIK...  ideally
> this would be a GPIO and then if we need to hold it high then
> there would be some binding we could connect to the GPIO that
> says that.  We don't have that though so for now a fixed property
> might be OK.

It's most like two phase buck controllers internally. 'Vsel' to
control which buck output.
Each phase has its registers (voltage/enable/mode).
The common usage of 'vsel' pin is to connect the pin by SoC suspend/resume pin.
This can be used to implement system low power design (like as system
awake 0.6V, sleep 0.3V or sleep buck off).
I haven't seen the usage to connect it directly by GPIO.

Re: [PATCH 1/2] dt-bindings: regulator: Add bindings for Richtek RT5739

[Krzysztof Kozlowski]

On 15/02/2023 03:00, cy_huang@richtek.com wrote:
> From: ChiYuan Huang <cy_huang@richtek.com>
> 
> Add the binding document for Richtek RT5739.

Subject: drop second/last, redundant "bindings for". The "dt-bindings"
prefix is already stating that these are bindings.

> 
> Signed-off-by: ChiYuan Huang <cy_huang@richtek.com>
> ---
>  .../bindings/regulator/richtek,rt5739.yaml         | 80 ++++++++++++++++++++++
>  1 file changed, 80 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/regulator/richtek,rt5739.yaml
> 
> diff --git a/Documentation/devicetree/bindings/regulator/richtek,rt5739.yaml b/Documentation/devicetree/bindings/regulator/richtek,rt5739.yaml
> new file mode 100644
> index 00000000..7dc4f78
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/regulator/richtek,rt5739.yaml
> @@ -0,0 +1,80 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/regulator/richtek,rt5739.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Richtek RT5739 2.4MHz 3.5A Step-Down Converter
> +
> +maintainers:
> +  - ChiYuan Huang <cy_huang@richtek.com>
> +
> +description: |
> +  The RT5739 is a step-down switching voltage regulator that delivers a
> +  digitally programmable output from an input voltage supply of 2.5V to 5.5V.
> +  The output voltage is programmed through an I2C interface capable of
> +  operating up to 3.4MHz.
> +
> +  Using a proprietary architecture with synchronous rectification, the RT5739
> +  is capable of delivering 3.5A continuously at over 80% efficiency,
> +  maintaining that efficiency at load current as low as 10mA. The regulator
> +  operates at a normal fixed frequency of 2.4MHz, which reduces the value of
> +  the external components. 

Can we drop the marketing from kernel? Last part of sentence is not
related to this submission at all. The internal frequency also looks
unrelated to the topic...

> Additional output capacitance can be added to
> +  improve regulation during load transients without affecting stability.
> +
> +allOf:
> +  - $ref: regulator.yaml#
> +
> +properties:
> +  compatible:
> +    enum:
> +      - richtek,rt5739
> +
> +  reg:
> +    maxItems: 1
> +
> +  enable-gpios:
> +    maxItems: 1
> +
> +  richtek,vsel-active-high:
> +    description: |
> +      If property is present, use the 'VSEL1' register group for buck control.
> +      Else, use the 'VSEL0' register group. This depends on external hardware
> +      'VSEL' pin connecton.
> +    type: boolean
> +
> +  regulator-allowed-modes:
> +    description: |
> +      buck allowed operating mode
> +        0: Auto PFM/PWM mode
> +        1: Forced PWM mode
> +    maxItems: 2
> +    items:
> +      enum: [0, 1]

So you always require two items? Thus I wonder what's the point of
having it in DT? To skip the property entirely if none of the modes are
allowed?


Best regards,
Krzysztof

Re: [PATCH 1/2] dt-bindings: regulator: Add bindings for Richtek RT5739

[ChiYuan Huang]

On Thu, Feb 16, 2023 at 10:12:15AM +0100, Krzysztof Kozlowski wrote:
> On 15/02/2023 03:00, cy_huang@richtek.com wrote:
> > From: ChiYuan Huang <cy_huang@richtek.com>
> > 
> > Add the binding document for Richtek RT5739.
> 
> Subject: drop second/last, redundant "bindings for". The "dt-bindings"
> prefix is already stating that these are bindings.
>
Then, refine it to "dt-bindings: regulator: Add Richtek RT5739 document" 
> > 
> > Signed-off-by: ChiYuan Huang <cy_huang@richtek.com>
> > ---
> >  .../bindings/regulator/richtek,rt5739.yaml         | 80 ++++++++++++++++++++++
> >  1 file changed, 80 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/regulator/richtek,rt5739.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/regulator/richtek,rt5739.yaml b/Documentation/devicetree/bindings/regulator/richtek,rt5739.yaml
> > new file mode 100644
> > index 00000000..7dc4f78
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/regulator/richtek,rt5739.yaml
> > @@ -0,0 +1,80 @@
> > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/regulator/richtek,rt5739.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Richtek RT5739 2.4MHz 3.5A Step-Down Converter
> > +
> > +maintainers:
> > +  - ChiYuan Huang <cy_huang@richtek.com>
> > +
> > +description: |
> > +  The RT5739 is a step-down switching voltage regulator that delivers a
> > +  digitally programmable output from an input voltage supply of 2.5V to 5.5V.
> > +  The output voltage is programmed through an I2C interface capable of
> > +  operating up to 3.4MHz.
> > +
> > +  Using a proprietary architecture with synchronous rectification, the RT5739
> > +  is capable of delivering 3.5A continuously at over 80% efficiency,
> > +  maintaining that efficiency at load current as low as 10mA. The regulator
> > +  operates at a normal fixed frequency of 2.4MHz, which reduces the value of
> > +  the external components. 
> 
> Can we drop the marketing from kernel? Last part of sentence is not
> related to this submission at all. The internal frequency also looks
> unrelated to the topic...
>
Okay, too much marketing text. I'll shorten it and simply describe the function or
voltage range only. 
> > Additional output capacitance can be added to
> > +  improve regulation during load transients without affecting stability.
> > +
> > +allOf:
> > +  - $ref: regulator.yaml#
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - richtek,rt5739
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  enable-gpios:
> > +    maxItems: 1
> > +
> > +  richtek,vsel-active-high:
> > +    description: |
> > +      If property is present, use the 'VSEL1' register group for buck control.
> > +      Else, use the 'VSEL0' register group. This depends on external hardware
> > +      'VSEL' pin connecton.
> > +    type: boolean
> > +
> > +  regulator-allowed-modes:
> > +    description: |
> > +      buck allowed operating mode
> > +        0: Auto PFM/PWM mode
> > +        1: Forced PWM mode
> > +    maxItems: 2
> > +    items:
> > +      enum: [0, 1]
> 
> So you always require two items? Thus I wonder what's the point of
> having it in DT? To skip the property entirely if none of the modes are
> allowed?
>
Not always need two. So does it mean no need to describe the 'maxItems' and 'Items'.
Keep the description of 'regulator-allowed-modes' to tell the user what the valid value is.
> 
> Best regards,
> Krzysztof
> 

Re: [PATCH 1/2] dt-bindings: regulator: Add bindings for Richtek RT5739

[Krzysztof Kozlowski]

On 16/02/2023 10:57, ChiYuan Huang wrote:
> On Thu, Feb 16, 2023 at 10:12:15AM +0100, Krzysztof Kozlowski wrote:
>> On 15/02/2023 03:00, cy_huang@richtek.com wrote:
>>> From: ChiYuan Huang <cy_huang@richtek.com>
>>>
>>> Add the binding document for Richtek RT5739.
>>
>> Subject: drop second/last, redundant "bindings for". The "dt-bindings"
>> prefix is already stating that these are bindings.
>>
> Then, refine it to "dt-bindings: regulator: Add Richtek RT5739 document" 

I propose also to drop "document" - it is also redundant. Can bindings
be something else than document?


>>>
>>> Signed-off-by: ChiYuan Huang <cy_huang@richtek.com>
>>> ---
>>>  .../bindings/regulator/richtek,rt5739.yaml         | 80 ++++++++++++++++++++++
>>>  1 file changed, 80 insertions(+)
>>>  create mode 100644 Documentation/devicetree/bindings/regulator/richtek,rt5739.yaml
>>>
>>> diff --git a/Documentation/devicetree/bindings/regulator/richtek,rt5739.yaml b/Documentation/devicetree/bindings/regulator/richtek,rt5739.yaml
>>> new file mode 100644
>>> index 00000000..7dc4f78
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/regulator/richtek,rt5739.yaml
>>> @@ -0,0 +1,80 @@
>>> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
>>> +%YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/regulator/richtek,rt5739.yaml#
>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>> +
>>> +title: Richtek RT5739 2.4MHz 3.5A Step-Down Converter
>>> +
>>> +maintainers:
>>> +  - ChiYuan Huang <cy_huang@richtek.com>
>>> +
>>> +description: |
>>> +  The RT5739 is a step-down switching voltage regulator that delivers a
>>> +  digitally programmable output from an input voltage supply of 2.5V to 5.5V.
>>> +  The output voltage is programmed through an I2C interface capable of
>>> +  operating up to 3.4MHz.
>>> +
>>> +  Using a proprietary architecture with synchronous rectification, the RT5739
>>> +  is capable of delivering 3.5A continuously at over 80% efficiency,
>>> +  maintaining that efficiency at load current as low as 10mA. The regulator
>>> +  operates at a normal fixed frequency of 2.4MHz, which reduces the value of
>>> +  the external components. 
>>
>> Can we drop the marketing from kernel? Last part of sentence is not
>> related to this submission at all. The internal frequency also looks
>> unrelated to the topic...
>>
> Okay, too much marketing text. I'll shorten it and simply describe the function or
> voltage range only. 
>>> Additional output capacitance can be added to
>>> +  improve regulation during load transients without affecting stability.
>>> +
>>> +allOf:
>>> +  - $ref: regulator.yaml#
>>> +
>>> +properties:
>>> +  compatible:
>>> +    enum:
>>> +      - richtek,rt5739
>>> +
>>> +  reg:
>>> +    maxItems: 1
>>> +
>>> +  enable-gpios:
>>> +    maxItems: 1
>>> +
>>> +  richtek,vsel-active-high:
>>> +    description: |
>>> +      If property is present, use the 'VSEL1' register group for buck control.
>>> +      Else, use the 'VSEL0' register group. This depends on external hardware
>>> +      'VSEL' pin connecton.
>>> +    type: boolean
>>> +
>>> +  regulator-allowed-modes:
>>> +    description: |
>>> +      buck allowed operating mode
>>> +        0: Auto PFM/PWM mode
>>> +        1: Forced PWM mode
>>> +    maxItems: 2
>>> +    items:
>>> +      enum: [0, 1]
>>
>> So you always require two items? Thus I wonder what's the point of
>> having it in DT? To skip the property entirely if none of the modes are
>> allowed?
>>
> Not always need two. So does it mean no need to describe the 'maxItems' and 'Items'.

Your minItems is 2, so you always need two. If you accept one, the add
minItems: 1.



Best regards,
Krzysztof

Re: [PATCH 1/2] dt-bindings: regulator: Add bindings for Richtek RT5739

[ChiYuan Huang]

On Thu, Feb 16, 2023 at 11:03:39AM +0100, Krzysztof Kozlowski wrote:
> On 16/02/2023 10:57, ChiYuan Huang wrote:
> > On Thu, Feb 16, 2023 at 10:12:15AM +0100, Krzysztof Kozlowski wrote:
> >> On 15/02/2023 03:00, cy_huang@richtek.com wrote:
> >>> From: ChiYuan Huang <cy_huang@richtek.com>
> >>>
> >>> Add the binding document for Richtek RT5739.
> >>
> >> Subject: drop second/last, redundant "bindings for". The "dt-bindings"
> >> prefix is already stating that these are bindings.
> >>
> > Then, refine it to "dt-bindings: regulator: Add Richtek RT5739 document" 
> 
> I propose also to drop "document" - it is also redundant. Can bindings
> be something else than document?
>
Yes, you'r right. 
> 
> >>>
> >>> Signed-off-by: ChiYuan Huang <cy_huang@richtek.com>
> >>> ---
> >>>  .../bindings/regulator/richtek,rt5739.yaml         | 80 ++++++++++++++++++++++
> >>>  1 file changed, 80 insertions(+)
> >>>  create mode 100644 Documentation/devicetree/bindings/regulator/richtek,rt5739.yaml
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/regulator/richtek,rt5739.yaml b/Documentation/devicetree/bindings/regulator/richtek,rt5739.yaml
> >>> new file mode 100644
> >>> index 00000000..7dc4f78
> >>> --- /dev/null
> >>> +++ b/Documentation/devicetree/bindings/regulator/richtek,rt5739.yaml
> >>> @@ -0,0 +1,80 @@
> >>> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> >>> +%YAML 1.2
> >>> +---
> >>> +$id: http://devicetree.org/schemas/regulator/richtek,rt5739.yaml#
> >>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >>> +
> >>> +title: Richtek RT5739 2.4MHz 3.5A Step-Down Converter
> >>> +
> >>> +maintainers:
> >>> +  - ChiYuan Huang <cy_huang@richtek.com>
> >>> +
> >>> +description: |
> >>> +  The RT5739 is a step-down switching voltage regulator that delivers a
> >>> +  digitally programmable output from an input voltage supply of 2.5V to 5.5V.
> >>> +  The output voltage is programmed through an I2C interface capable of
> >>> +  operating up to 3.4MHz.
> >>> +
> >>> +  Using a proprietary architecture with synchronous rectification, the RT5739
> >>> +  is capable of delivering 3.5A continuously at over 80% efficiency,
> >>> +  maintaining that efficiency at load current as low as 10mA. The regulator
> >>> +  operates at a normal fixed frequency of 2.4MHz, which reduces the value of
> >>> +  the external components. 
> >>
> >> Can we drop the marketing from kernel? Last part of sentence is not
> >> related to this submission at all. The internal frequency also looks
> >> unrelated to the topic...
> >>
> > Okay, too much marketing text. I'll shorten it and simply describe the function or
> > voltage range only. 
> >>> Additional output capacitance can be added to
> >>> +  improve regulation during load transients without affecting stability.
> >>> +
> >>> +allOf:
> >>> +  - $ref: regulator.yaml#
> >>> +
> >>> +properties:
> >>> +  compatible:
> >>> +    enum:
> >>> +      - richtek,rt5739
> >>> +
> >>> +  reg:
> >>> +    maxItems: 1
> >>> +
> >>> +  enable-gpios:
> >>> +    maxItems: 1
> >>> +
> >>> +  richtek,vsel-active-high:
> >>> +    description: |
> >>> +      If property is present, use the 'VSEL1' register group for buck control.
> >>> +      Else, use the 'VSEL0' register group. This depends on external hardware
> >>> +      'VSEL' pin connecton.
> >>> +    type: boolean
> >>> +
> >>> +  regulator-allowed-modes:
> >>> +    description: |
> >>> +      buck allowed operating mode
> >>> +        0: Auto PFM/PWM mode
> >>> +        1: Forced PWM mode
> >>> +    maxItems: 2
> >>> +    items:
> >>> +      enum: [0, 1]
> >>
> >> So you always require two items? Thus I wonder what's the point of
> >> having it in DT? To skip the property entirely if none of the modes are
> >> allowed?
> >>
> > Not always need two. So does it mean no need to describe the 'maxItems' and 'Items'.
> 
> Your minItems is 2, so you always need two. If you accept one, the add
> minItems: 1.
> 
https://elixir.bootlin.com/linux/v6.2-rc8/source/drivers/regulator/of_regulator.c#L198
It seems no need to limit the maxItems. Regulator core will call 'of_map_mode' to check the value.
Even the value is repeat.

And for minItems, 'regulator.yaml' already said it's uint32-array. Must be lager than zero, right?

So how about just keep 'items' and remove the 'maxItems'?
> 
> 
> Best regards,
> Krzysztof
> 

Re: [PATCH 1/2] dt-bindings: regulator: Add bindings for Richtek RT5739

[Krzysztof Kozlowski]

On 16/02/2023 11:35, ChiYuan Huang wrote:

> https://elixir.bootlin.com/linux/v6.2-rc8/source/drivers/regulator/of_regulator.c#L198
> It seems no need to limit the maxItems. Regulator core will call 'of_map_mode' to check the value.
> Even the value is repeat.
> 
> And for minItems, 'regulator.yaml' already said it's uint32-array. Must be lager than zero, right?
> 
> So how about just keep 'items' and remove the 'maxItems'?

Yeah, go ahead, with that enum, it should be correct..

Best regards,
Krzysztof