[PATCH 0/2] Add Richtek RT5759 buck converter support

[PATCH 0/2] Add Richtek RT5759 buck converter support

[cy_huang]

From: ChiYuan Huang <cy_huang@richtek.com>

This patch series add Richtek RT5759 buck coverter support.

ChiYuan Huang (2):
  dt-bindings: regulator: Add binding for Richtek RT5759 DCDC converter
  regulator: rt5759: Add support for Richtek RT5759 DCDC converter

 .../regulator/richtek,rt5759-regulator.yaml        |  90 +++++
 drivers/regulator/Kconfig                          |  10 +
 drivers/regulator/Makefile                         |   1 +
 drivers/regulator/rt5759-regulator.c               | 372 +++++++++++++++++++++
 4 files changed, 473 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/regulator/richtek,rt5759-regulator.yaml
 create mode 100644 drivers/regulator/rt5759-regulator.c

-- 
2.7.4

[PATCH 1/2] dt-bindings: regulator: Add binding for Richtek RT5759 DCDC converter

[cy_huang]

From: ChiYuan Huang <cy_huang@richtek.com>

Add bindings for Richtek RT5759 high-performance DCDC converter.

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

diff --git a/Documentation/devicetree/bindings/regulator/richtek,rt5759-regulator.yaml b/Documentation/devicetree/bindings/regulator/richtek,rt5759-regulator.yaml
new file mode 100644
index 00000000..c24b583
--- /dev/null
+++ b/Documentation/devicetree/bindings/regulator/richtek,rt5759-regulator.yaml
@@ -0,0 +1,90 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/regulator/richtek,rt5759-regulator.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Richtek RT5759 High Performance DCDC Concverter
+
+maintainers:
+  - ChiYuan Huang <cy_huang@richtek.com>
+
+description: |
+  The RT5759 is a high-performance, synchronous step-down DC-DC converter that
+  can deliver up to 9A output current from 3V to 6.5V input supply, The output
+  voltage can be programmable with I2C controlled 7-Bit VID.
+
+  Datasheet is available at
+  https://www.richtek.com/assets/product_file/RT5759/DS5759-00.pdf
+
+allOf:
+  - $ref: regulator.yaml#
+
+properties:
+  compatible:
+    enum:
+      - richtek,rt5759
+      - richtek,rt5759a
+
+  reg:
+    maxItems: 1
+
+  regulator-allowed-modes:
+    description: |
+      buck allowed operating mode
+        0: auto mode (PSKIP: pulse skipping)
+        1: force pwm mode
+    items:
+      enum: [0, 1]
+
+  richtek,watchdog-enable:
+    description: enable the external watchdog reset pin listening
+    type: boolean
+
+if:
+  properties:
+    compatible:
+      contains:
+        const: richtek,rt5759
+then:
+  properties:
+    richtek,watchdog-enable: false
+
+required:
+  - compatible
+  - reg
+
+unevaluatedProperties: false
+
+examples:
+  # example 1 for RT5759
+  - |
+    i2c {
+      #address-cells = <1>;
+      #size-cells = <0>;
+
+      rt5759@62 {
+        compatible = "richtek,rt5759";
+        reg = <0x62>;
+        regulator-name = "rt5759-buck";
+        regulator-min-microvolt = <600000>;
+        regulator-max-microvolt = <1500000>;
+        regulator-boot-on;
+      };
+    };
+  # example 2 for RT5759A
+  - |
+    i2c {
+      #address-cells = <1>;
+      #size-cells = <0>;
+
+      rt5759a@62 {
+        compatible = "richtek,rt5759a";
+        reg = <0x62>;
+        regulator-name = "rt5759a-buck";
+        regulator-min-microvolt = <600000>;
+        regulator-max-microvolt = <1725000>;
+        regulator-boot-on;
+        richtek,watchdog-enable;
+      };
+    };
-- 
2.7.4

[PATCH 2/2] regulator: rt5759: Add support for Richtek RT5759 DCDC converter

[cy_huang]

From: ChiYuan Huang <cy_huang@richtek.com>

Add support for Richtek RT5759 high-performance DCDC converter.

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

diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
index 1c35fed2..72e0318 100644
--- a/drivers/regulator/Kconfig
+++ b/drivers/regulator/Kconfig
@@ -1046,6 +1046,16 @@ config REGULATOR_RT5033
 	  RT5033 PMIC. The device supports multiple regulators like
 	  current source, LDO and Buck.
 
+config REGULATOR_RT5759
+	tristate "Richtek RT5759 Regulator"
+	depends on I2C
+	select REGMAP_I2C
+	help
+	  This adds support for voltage regulator in Richtek RT5759.
+	  The RT5759 is a high-performance, synchronous step-down DC-DC
+	  converter that can deliver up to 9A output current from 3V to 6.5V
+	  input supply.
+
 config REGULATOR_RT6160
 	tristate "Richtek RT6160 BuckBoost voltage regulator"
 	depends on I2C
diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
index 2e1b087..239a3a3 100644
--- a/drivers/regulator/Makefile
+++ b/drivers/regulator/Makefile
@@ -126,6 +126,7 @@ obj-$(CONFIG_REGULATOR_ROHM)	+= rohm-regulator.o
 obj-$(CONFIG_REGULATOR_RT4801)	+= rt4801-regulator.o
 obj-$(CONFIG_REGULATOR_RT4831)	+= rt4831-regulator.o
 obj-$(CONFIG_REGULATOR_RT5033)	+= rt5033-regulator.o
+obj-$(CONFIG_REGULATOR_RT5759)	+= rt5759-regulator.o
 obj-$(CONFIG_REGULATOR_RT6160)	+= rt6160-regulator.o
 obj-$(CONFIG_REGULATOR_RT6245)	+= rt6245-regulator.o
 obj-$(CONFIG_REGULATOR_RTMV20)	+= rtmv20-regulator.o
diff --git a/drivers/regulator/rt5759-regulator.c b/drivers/regulator/rt5759-regulator.c
new file mode 100644
index 00000000..eaff77b
--- /dev/null
+++ b/drivers/regulator/rt5759-regulator.c
@@ -0,0 +1,372 @@
+// SPDX-License-Identifier: GPL-2.0+
+
+#include <linux/bits.h>
+#include <linux/i2c.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/regmap.h>
+#include <linux/regulator/driver.h>
+#include <linux/regulator/of_regulator.h>
+
+#define RT5759_REG_VENDORINFO	0x00
+#define RT5759_REG_FREQ		0x01
+#define RT5759_REG_VSEL		0x02
+#define RT5759_REG_DCDCCTRL	0x03
+#define RT5759_REG_STATUS	0x04
+#define RT5759_REG_DCDCSET	0x05
+#define RT5759A_REG_WDTEN	0x42
+
+#define RT5759_TSTEP_MASK	GENMASK(3, 2)
+#define RT5759_VSEL_MASK	GENMASK(6, 0)
+#define RT5759_DISCHARGE_MASK	BIT(3)
+#define RT5759_FPWM_MASK	BIT(2)
+#define RT5759_ENABLE_MASK	BIT(1)
+#define RT5759_OT_MASK		BIT(1)
+#define RT5759_UV_MASK		BIT(0)
+#define RT5957_OCLVL_MASK	GENMASK(7, 6)
+#define RT5759_OCLVL_SHIFT	6
+#define RT5957_OTLVL_MASK	GENMASK(5, 4)
+#define RT5759_OTLVL_SHIFT	4
+#define RT5759A_WDTEN_MASK	BIT(1)
+
+#define RT5759_MANUFACTURER_ID	0x82
+/* vsel range 0x00 ~ 0x5A */
+#define RT5759_NUM_VOLTS	91
+#define RT5759_MIN_UV		600000
+#define RT5759_STEP_UV		10000
+#define RT5759A_STEP_UV		12500
+#define RT5759_MINSS_TIMEUS	1500
+
+#define RT5759_PSKIP_MODE	0
+#define RT5759_FPWM_MODE	1
+
+enum {
+	CHIP_TYPE_RT5759 = 0,
+	CHIP_TYPE_RT5759A,
+	CHIP_TYPE_MAX
+};
+
+struct rt5759_priv {
+	struct device *dev;
+	struct regmap *regmap;
+	struct regulator_desc desc;
+	unsigned long chip_type;
+};
+
+static int rt5759_set_mode(struct regulator_dev *rdev, unsigned int mode)
+{
+	struct regmap *regmap = rdev_get_regmap(rdev);
+	unsigned int mode_val;
+
+	switch (mode) {
+	case REGULATOR_MODE_NORMAL:
+		mode_val = 0;
+		break;
+	case REGULATOR_MODE_FAST:
+		mode_val = RT5759_FPWM_MASK;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return regmap_update_bits(regmap, RT5759_REG_STATUS, RT5759_FPWM_MASK,
+				  mode_val);
+}
+
+static unsigned int rt5759_get_mode(struct regulator_dev *rdev)
+{
+	struct regmap *regmap = rdev_get_regmap(rdev);
+	unsigned int regval;
+	int ret;
+
+	ret = regmap_read(regmap, RT5759_REG_DCDCCTRL, &regval);
+	if (ret)
+		return REGULATOR_MODE_INVALID;
+
+	if (regval & RT5759_FPWM_MASK)
+		return REGULATOR_MODE_FAST;
+
+	return REGULATOR_MODE_NORMAL;
+}
+
+static int rt5759_get_error_flags(struct regulator_dev *rdev,
+				  unsigned int *flags)
+{
+	struct regmap *regmap = rdev_get_regmap(rdev);
+	unsigned int status, events = 0;
+	int ret;
+
+	ret = regmap_read(regmap, RT5759_REG_STATUS, &status);
+	if (ret)
+		return ret;
+
+	if (status & RT5759_OT_MASK)
+		events |= REGULATOR_ERROR_OVER_TEMP;
+
+	if (status & RT5759_UV_MASK)
+		events |= REGULATOR_ERROR_UNDER_VOLTAGE;
+
+	*flags = events;
+	return 0;
+}
+
+static int rt5759_set_ocp(struct regulator_dev *rdev, int lim_uA, int severity,
+			  bool enable)
+{
+	struct regmap *regmap = rdev_get_regmap(rdev);
+	int ocp_lvl[] = { 9800000, 10800000, 11800000 };
+	unsigned int ocp_regval;
+	int i;
+
+	/* Only support over current protection parameter */
+	if (severity != REGULATOR_SEVERITY_PROT)
+		return 0;
+
+	if (enable) {
+		/* Default ocp level is 10.8A */
+		if (lim_uA == 0)
+			lim_uA = 10800000;
+
+		for (i = 0; i < ARRAY_SIZE(ocp_lvl); i++) {
+			if (lim_uA <= ocp_lvl[i])
+				break;
+		}
+
+		if (i == ARRAY_SIZE(ocp_lvl))
+			i = ARRAY_SIZE(ocp_lvl) - 1;
+
+		ocp_regval = i + 1;
+	} else
+		ocp_regval = 0;
+
+	return regmap_update_bits(regmap, RT5759_REG_DCDCSET, RT5957_OCLVL_MASK,
+				  ocp_regval << RT5759_OCLVL_SHIFT);
+}
+
+static int rt5759_set_otp(struct regulator_dev *rdev, int lim, int severity,
+			  bool enable)
+{
+	struct regmap *regmap = rdev_get_regmap(rdev);
+	int otp_lvl[] = { 140, 150, 170 };
+	unsigned int otp_regval;
+	int i;
+
+	/* Only support over temperature protection parameter */
+	if (severity != REGULATOR_SEVERITY_PROT)
+		return 0;
+
+	if (enable) {
+		/* Default otp level is 150'c */
+		if (lim == 0)
+			lim = 150;
+
+		for (i = 0; i < ARRAY_SIZE(otp_lvl); i++) {
+			if (lim <= otp_lvl[i])
+				break;
+		}
+
+		if (i == ARRAY_SIZE(otp_lvl))
+			i = ARRAY_SIZE(otp_lvl) - 1;
+
+		otp_regval = i + 1;
+	} else
+		otp_regval = 0;
+
+	return regmap_update_bits(regmap, RT5759_REG_DCDCSET, RT5957_OTLVL_MASK,
+				  otp_regval << RT5759_OTLVL_SHIFT);
+}
+
+static const struct regulator_ops rt5759_regulator_ops = {
+	.list_voltage = regulator_list_voltage_linear,
+	.set_voltage_sel = regulator_set_voltage_sel_regmap,
+	.get_voltage_sel = regulator_get_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 = rt5759_set_mode,
+	.get_mode = rt5759_get_mode,
+	.set_ramp_delay = regulator_set_ramp_delay_regmap,
+	.get_error_flags = rt5759_get_error_flags,
+	.set_over_current_protection = rt5759_set_ocp,
+	.set_thermal_protection = rt5759_set_otp,
+};
+
+static unsigned int rt5759_of_map_mode(unsigned int mode)
+{
+	switch (mode) {
+	case RT5759_FPWM_MODE:
+		return REGULATOR_MODE_FAST;
+	case RT5759_PSKIP_MODE:
+		return REGULATOR_MODE_NORMAL;
+	default:
+		return REGULATOR_MODE_INVALID;
+	}
+}
+
+static const unsigned int rt5759_ramp_table[] = { 20000, 15000, 10000, 5000 };
+
+static int rt5759_regulator_register(struct rt5759_priv *priv)
+{
+	struct device_node *np = priv->dev->of_node;
+	struct regulator_desc *reg_desc = &priv->desc;
+	struct regulator_config reg_cfg;
+	struct regulator_dev *rdev;
+	int ret;
+
+	reg_desc->name = "rt5759-buck";
+	reg_desc->type = REGULATOR_VOLTAGE;
+	reg_desc->owner = THIS_MODULE;
+	reg_desc->ops = &rt5759_regulator_ops;
+	reg_desc->n_voltages = RT5759_NUM_VOLTS;
+	reg_desc->min_uV = RT5759_MIN_UV;
+	reg_desc->uV_step = RT5759_STEP_UV;
+	reg_desc->vsel_reg = RT5759_REG_VSEL;
+	reg_desc->vsel_mask = RT5759_VSEL_MASK;
+	reg_desc->enable_reg = RT5759_REG_DCDCCTRL;
+	reg_desc->enable_mask = RT5759_ENABLE_MASK;
+	reg_desc->active_discharge_reg = RT5759_REG_DCDCCTRL;
+	reg_desc->active_discharge_mask = RT5759_DISCHARGE_MASK;
+	reg_desc->active_discharge_on = RT5759_DISCHARGE_MASK;
+	reg_desc->ramp_reg = RT5759_REG_FREQ;
+	reg_desc->ramp_mask = RT5759_TSTEP_MASK;
+	reg_desc->ramp_delay_table = rt5759_ramp_table;
+	reg_desc->n_ramp_values = ARRAY_SIZE(rt5759_ramp_table);
+	reg_desc->enable_time = RT5759_MINSS_TIMEUS;
+	reg_desc->of_map_mode = rt5759_of_map_mode;
+
+	/*
+	 * RT5759 step uV = 10000
+	 * RT5759A step uV = 12500
+	 */
+	if (priv->chip_type == CHIP_TYPE_RT5759A)
+		reg_desc->uV_step = RT5759A_STEP_UV;
+
+	reg_cfg.dev = priv->dev;
+	reg_cfg.of_node = np;
+	reg_cfg.init_data = of_get_regulator_init_data(priv->dev, np, reg_desc);
+	reg_cfg.regmap = priv->regmap;
+
+	rdev = devm_regulator_register(priv->dev, reg_desc, &reg_cfg);
+	if (IS_ERR(rdev)) {
+		ret = PTR_ERR(rdev);
+		dev_err(priv->dev, "Failed to register regulator (%d)\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int rt5759_init_device_property(struct rt5759_priv *priv)
+{
+	unsigned int val = 0;
+	bool wdt_enable;
+
+	/*
+	 * Only RT5759A support external watchdog input
+	 */
+	if (priv->chip_type != CHIP_TYPE_RT5759A)
+		return 0;
+
+	wdt_enable = device_property_read_bool(priv->dev,
+					       "richtek,watchdog-enable");
+	if (wdt_enable)
+		val = RT5759A_WDTEN_MASK;
+
+	return regmap_update_bits(priv->regmap, RT5759A_REG_WDTEN,
+				  RT5759A_WDTEN_MASK, val);
+}
+
+static int rt5759_manufacturer_check(struct rt5759_priv *priv)
+{
+	unsigned int vendor;
+	int ret;
+
+	ret = regmap_read(priv->regmap, RT5759_REG_VENDORINFO, &vendor);
+	if (ret)
+		return ret;
+
+	if (vendor != RT5759_MANUFACTURER_ID) {
+		dev_err(priv->dev, "vendor info not correct (%d)\n", vendor);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static bool rt5759_is_accessible_reg(struct device *dev, unsigned int reg)
+{
+	struct rt5759_priv *priv = dev_get_drvdata(dev);
+
+	if (reg <= RT5759_REG_DCDCSET)
+		return true;
+
+	if (priv->chip_type == CHIP_TYPE_RT5759A && reg == RT5759A_REG_WDTEN)
+		return true;
+
+	return false;
+}
+
+static const struct regmap_config rt5759_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+	.max_register = RT5759A_REG_WDTEN,
+	.readable_reg = rt5759_is_accessible_reg,
+	.writeable_reg = rt5759_is_accessible_reg,
+};
+
+static int rt5759_probe(struct i2c_client *i2c)
+{
+	struct rt5759_priv *priv;
+	int ret;
+
+	priv = devm_kzalloc(&i2c->dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->dev = &i2c->dev;
+	priv->chip_type = (unsigned long)of_device_get_match_data(&i2c->dev);
+	i2c_set_clientdata(i2c, priv);
+
+	priv->regmap = devm_regmap_init_i2c(i2c, &rt5759_regmap_config);
+	if (IS_ERR(priv->regmap)) {
+		ret = PTR_ERR(priv->regmap);
+		dev_err(&i2c->dev, "Failed to allocate regmap (%d)\n", ret);
+		return ret;
+	}
+
+	ret = rt5759_manufacturer_check(priv);
+	if (ret) {
+		dev_err(&i2c->dev, "Failed to check device (%d)\n", ret);
+		return ret;
+	}
+
+	ret = rt5759_init_device_property(priv);
+	if (ret) {
+		dev_err(&i2c->dev, "Failed to init device (%d)\n", ret);
+		return ret;
+	}
+
+	return rt5759_regulator_register(priv);
+}
+
+static const struct of_device_id __maybe_unused rt5759_device_table[] = {
+	{ .compatible = "richtek,rt5759", .data = (void *)CHIP_TYPE_RT5759 },
+	{ .compatible = "richtek,rt5759a", .data = (void *)CHIP_TYPE_RT5759A },
+	{}
+};
+MODULE_DEVICE_TABLE(of, rt5759_device_table);
+
+static struct i2c_driver rt5759_driver = {
+	.driver = {
+		.name = "rt5759",
+		.of_match_table = rt5759_device_table,
+	},
+	.probe_new = rt5759_probe,
+};
+module_i2c_driver(rt5759_driver);
+
+MODULE_AUTHOR("ChiYuan Huang <cy_huang@richtek.com>");
+MODULE_DESCRIPTION("Richtek RT5759 Regulator Driver");
+MODULE_LICENSE("GPL v2");
-- 
2.7.4

Re: [PATCH 1/2] dt-bindings: regulator: Add binding for Richtek RT5759 DCDC converter

[Krzysztof Kozlowski]

On 25/03/2022 02:06, cy_huang wrote:
> From: ChiYuan Huang <cy_huang@richtek.com>
> 
> Add bindings for Richtek RT5759 high-performance DCDC converter.
> 
> Signed-off-by: ChiYuan Huang <cy_huang@richtek.com>
> ---
>  .../regulator/richtek,rt5759-regulator.yaml        | 90 ++++++++++++++++++++++
>  1 file changed, 90 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/regulator/richtek,rt5759-regulator.yaml
> 
> diff --git a/Documentation/devicetree/bindings/regulator/richtek,rt5759-regulator.yaml b/Documentation/devicetree/bindings/regulator/richtek,rt5759-regulator.yaml
> new file mode 100644
> index 00000000..c24b583
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/regulator/richtek,rt5759-regulator.yaml
> @@ -0,0 +1,90 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/regulator/richtek,rt5759-regulator.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Richtek RT5759 High Performance DCDC Concverter

typo: Converter

> +
> +maintainers:
> +  - ChiYuan Huang <cy_huang@richtek.com>
> +
> +description: |
> +  The RT5759 is a high-performance, synchronous step-down DC-DC converter that
> +  can deliver up to 9A output current from 3V to 6.5V input supply, The output
> +  voltage can be programmable with I2C controlled 7-Bit VID.
> +
> +  Datasheet is available at
> +  https://www.richtek.com/assets/product_file/RT5759/DS5759-00.pdf
> +
> +allOf:
> +  - $ref: regulator.yaml#
> +
> +properties:
> +  compatible:
> +    enum:
> +      - richtek,rt5759
> +      - richtek,rt5759a
> +
> +  reg:
> +    maxItems: 1
> +
> +  regulator-allowed-modes:
> +    description: |
> +      buck allowed operating mode
> +        0: auto mode (PSKIP: pulse skipping)
> +        1: force pwm mode
> +    items:
> +      enum: [0, 1]
> +
> +  richtek,watchdog-enable:
> +    description: enable the external watchdog reset pin listening
> +    type: boolean
> +
> +if:

This should be inside allOf. Move allOf here,

> +  properties:
> +    compatible:
> +      contains:
> +        const: richtek,rt5759
> +then:
> +  properties:
> +    richtek,watchdog-enable: false
> +
> +required:
> +  - compatible
> +  - reg
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  # example 1 for RT5759
> +  - |
> +    i2c {
> +      #address-cells = <1>;
> +      #size-cells = <0>;
> +
> +      rt5759@62 {

Generic node name, so pmic.

> +        compatible = "richtek,rt5759";
> +        reg = <0x62>;
> +        regulator-name = "rt5759-buck";
> +        regulator-min-microvolt = <600000>;
> +        regulator-max-microvolt = <1500000>;
> +        regulator-boot-on;
> +      };
> +    };
> +  # example 2 for RT5759A
> +  - |
> +    i2c {
> +      #address-cells = <1>;
> +      #size-cells = <0>;
> +
> +      rt5759a@62 {

Ditto

> +        compatible = "richtek,rt5759a";
> +        reg = <0x62>;
> +        regulator-name = "rt5759a-buck";
> +        regulator-min-microvolt = <600000>;
> +        regulator-max-microvolt = <1725000>;
> +        regulator-boot-on;
> +        richtek,watchdog-enable;
> +      };
> +    };


Best regards,
Krzysztof

Re: [PATCH 2/2] regulator: rt5759: Add support for Richtek RT5759 DCDC converter

[Krzysztof Kozlowski]

On 25/03/2022 02:06, cy_huang wrote:
> From: ChiYuan Huang <cy_huang@richtek.com>
> 
> Add support for Richtek RT5759 high-performance DCDC converter.
> 
> Signed-off-by: ChiYuan Huang <cy_huang@richtek.com>
> ---
>  drivers/regulator/Kconfig            |  10 +
>  drivers/regulator/Makefile           |   1 +
>  drivers/regulator/rt5759-regulator.c | 372 +++++++++++++++++++++++++++++++++++
>  3 files changed, 383 insertions(+)
>  create mode 100644 drivers/regulator/rt5759-regulator.c
> 

(...)

> +static int rt5759_init_device_property(struct rt5759_priv *priv)
> +{
> +	unsigned int val = 0;
> +	bool wdt_enable;
> +
> +	/*
> +	 * Only RT5759A support external watchdog input
> +	 */
> +	if (priv->chip_type != CHIP_TYPE_RT5759A)
> +		return 0;
> +
> +	wdt_enable = device_property_read_bool(priv->dev,
> +					       "richtek,watchdog-enable");
> +	if (wdt_enable)

No need for separate wdt_enable variable.

> +		val = RT5759A_WDTEN_MASK;
> +
> +	return regmap_update_bits(priv->regmap, RT5759A_REG_WDTEN,
> +				  RT5759A_WDTEN_MASK, val);
> +}
> +
> +static int rt5759_manufacturer_check(struct rt5759_priv *priv)
> +{
> +	unsigned int vendor;
> +	int ret;
> +
> +	ret = regmap_read(priv->regmap, RT5759_REG_VENDORINFO, &vendor);
> +	if (ret)
> +		return ret;
> +
> +	if (vendor != RT5759_MANUFACTURER_ID) {
> +		dev_err(priv->dev, "vendor info not correct (%d)\n", vendor);
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static bool rt5759_is_accessible_reg(struct device *dev, unsigned int reg)
> +{
> +	struct rt5759_priv *priv = dev_get_drvdata(dev);
> +
> +	if (reg <= RT5759_REG_DCDCSET)
> +		return true;
> +
> +	if (priv->chip_type == CHIP_TYPE_RT5759A && reg == RT5759A_REG_WDTEN)
> +		return true;
> +
> +	return false;
> +}
> +
> +static const struct regmap_config rt5759_regmap_config = {
> +	.reg_bits = 8,
> +	.val_bits = 8,
> +	.max_register = RT5759A_REG_WDTEN,
> +	.readable_reg = rt5759_is_accessible_reg,
> +	.writeable_reg = rt5759_is_accessible_reg,
> +};
> +
> +static int rt5759_probe(struct i2c_client *i2c)
> +{
> +	struct rt5759_priv *priv;
> +	int ret;
> +
> +	priv = devm_kzalloc(&i2c->dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	priv->dev = &i2c->dev;
> +	priv->chip_type = (unsigned long)of_device_get_match_data(&i2c->dev);
> +	i2c_set_clientdata(i2c, priv);
> +
> +	priv->regmap = devm_regmap_init_i2c(i2c, &rt5759_regmap_config);
> +	if (IS_ERR(priv->regmap)) {
> +		ret = PTR_ERR(priv->regmap);
> +		dev_err(&i2c->dev, "Failed to allocate regmap (%d)\n", ret);
> +		return ret;
> +	}
> +
> +	ret = rt5759_manufacturer_check(priv);
> +	if (ret) {
> +		dev_err(&i2c->dev, "Failed to check device (%d)\n", ret);
> +		return ret;
> +	}
> +
> +	ret = rt5759_init_device_property(priv);
> +	if (ret) {
> +		dev_err(&i2c->dev, "Failed to init device (%d)\n", ret);
> +		return ret;
> +	}
> +
> +	return rt5759_regulator_register(priv);
> +}
> +
> +static const struct of_device_id __maybe_unused rt5759_device_table[] = {

I don't think this can be __maybe_unused. It is always referenced via
of_match_table, isn't it?

> +	{ .compatible = "richtek,rt5759", .data = (void *)CHIP_TYPE_RT5759 },
> +	{ .compatible = "richtek,rt5759a", .data = (void *)CHIP_TYPE_RT5759A },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, rt5759_device_table);
> +
> +static struct i2c_driver rt5759_driver = {
> +	.driver = {
> +		.name = "rt5759",
> +		.of_match_table = rt5759_device_table,
> +	},
> +	.probe_new = rt5759_probe,
> +};
> +module_i2c_driver(rt5759_driver);
> +
> +MODULE_AUTHOR("ChiYuan Huang <cy_huang@richtek.com>");
> +MODULE_DESCRIPTION("Richtek RT5759 Regulator Driver");
> +MODULE_LICENSE("GPL v2");


Best regards,
Krzysztof

Re: [PATCH 1/2] dt-bindings: regulator: Add binding for Richtek RT5759 DCDC converter

[ChiYuan Huang]

Krzysztof Kozlowski <krzk@kernel.org> 於 2022年3月25日 週五 下午8:14寫道：
>
> On 25/03/2022 02:06, cy_huang wrote:
> > From: ChiYuan Huang <cy_huang@richtek.com>
> >
> > Add bindings for Richtek RT5759 high-performance DCDC converter.
> >
> > Signed-off-by: ChiYuan Huang <cy_huang@richtek.com>
> > ---
> >  .../regulator/richtek,rt5759-regulator.yaml        | 90 ++++++++++++++++++++++
> >  1 file changed, 90 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/regulator/richtek,rt5759-regulator.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/regulator/richtek,rt5759-regulator.yaml b/Documentation/devicetree/bindings/regulator/richtek,rt5759-regulator.yaml
> > new file mode 100644
> > index 00000000..c24b583
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/regulator/richtek,rt5759-regulator.yaml
> > @@ -0,0 +1,90 @@
> > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/regulator/richtek,rt5759-regulator.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Richtek RT5759 High Performance DCDC Concverter
>
> typo: Converter
>
Ack in next.
> > +
> > +maintainers:
> > +  - ChiYuan Huang <cy_huang@richtek.com>
> > +
> > +description: |
> > +  The RT5759 is a high-performance, synchronous step-down DC-DC converter that
> > +  can deliver up to 9A output current from 3V to 6.5V input supply, The output
> > +  voltage can be programmable with I2C controlled 7-Bit VID.
> > +
> > +  Datasheet is available at
> > +  https://www.richtek.com/assets/product_file/RT5759/DS5759-00.pdf
> > +
> > +allOf:
> > +  - $ref: regulator.yaml#
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - richtek,rt5759
> > +      - richtek,rt5759a
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  regulator-allowed-modes:
> > +    description: |
> > +      buck allowed operating mode
> > +        0: auto mode (PSKIP: pulse skipping)
> > +        1: force pwm mode
> > +    items:
> > +      enum: [0, 1]
> > +
> > +  richtek,watchdog-enable:
> > +    description: enable the external watchdog reset pin listening
> > +    type: boolean
> > +
> > +if:
>
> This should be inside allOf. Move allOf here,
>
Ack in next.
> > +  properties:
> > +    compatible:
> > +      contains:
> > +        const: richtek,rt5759
> > +then:
> > +  properties:
> > +    richtek,watchdog-enable: false
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +
> > +unevaluatedProperties: false
> > +
> > +examples:
> > +  # example 1 for RT5759
> > +  - |
> > +    i2c {
> > +      #address-cells = <1>;
> > +      #size-cells = <0>;
> > +
> > +      rt5759@62 {
>
> Generic node name, so pmic.
>
As my understanding, 'pmic' means there must be multiple channels of
buck or ldo.
But  rt5759 is only one channel buck converter.

> > +        compatible = "richtek,rt5759";
> > +        reg = <0x62>;
> > +        regulator-name = "rt5759-buck";
> > +        regulator-min-microvolt = <600000>;
> > +        regulator-max-microvolt = <1500000>;
> > +        regulator-boot-on;
> > +      };
> > +    };
> > +  # example 2 for RT5759A
> > +  - |
> > +    i2c {
> > +      #address-cells = <1>;
> > +      #size-cells = <0>;
> > +
> > +      rt5759a@62 {
>
> Ditto
>
> > +        compatible = "richtek,rt5759a";
> > +        reg = <0x62>;
> > +        regulator-name = "rt5759a-buck";
> > +        regulator-min-microvolt = <600000>;
> > +        regulator-max-microvolt = <1725000>;
> > +        regulator-boot-on;
> > +        richtek,watchdog-enable;
> > +      };
> > +    };
>
>
> Best regards,
> Krzysztof

Re: [PATCH 2/2] regulator: rt5759: Add support for Richtek RT5759 DCDC converter

[ChiYuan Huang]

Krzysztof Kozlowski <krzk@kernel.org> 於 2022年3月25日 週五 下午8:17寫道：
>
> On 25/03/2022 02:06, cy_huang wrote:
> > From: ChiYuan Huang <cy_huang@richtek.com>
> >
> > Add support for Richtek RT5759 high-performance DCDC converter.
> >
> > Signed-off-by: ChiYuan Huang <cy_huang@richtek.com>
> > ---
> >  drivers/regulator/Kconfig            |  10 +
> >  drivers/regulator/Makefile           |   1 +
> >  drivers/regulator/rt5759-regulator.c | 372 +++++++++++++++++++++++++++++++++++
> >  3 files changed, 383 insertions(+)
> >  create mode 100644 drivers/regulator/rt5759-regulator.c
> >
>
> (...)
>
> > +static int rt5759_init_device_property(struct rt5759_priv *priv)
> > +{
> > +     unsigned int val = 0;
> > +     bool wdt_enable;
> > +
> > +     /*
> > +      * Only RT5759A support external watchdog input
> > +      */
> > +     if (priv->chip_type != CHIP_TYPE_RT5759A)
> > +             return 0;
> > +
> > +     wdt_enable = device_property_read_bool(priv->dev,
> > +                                            "richtek,watchdog-enable");
> > +     if (wdt_enable)
>
> No need for separate wdt_enable variable.
>
Ack in next.
> > +             val = RT5759A_WDTEN_MASK;
> > +
> > +     return regmap_update_bits(priv->regmap, RT5759A_REG_WDTEN,
> > +                               RT5759A_WDTEN_MASK, val);
> > +}
> > +
> > +static int rt5759_manufacturer_check(struct rt5759_priv *priv)
> > +{
> > +     unsigned int vendor;
> > +     int ret;
> > +
> > +     ret = regmap_read(priv->regmap, RT5759_REG_VENDORINFO, &vendor);
> > +     if (ret)
> > +             return ret;
> > +
> > +     if (vendor != RT5759_MANUFACTURER_ID) {
> > +             dev_err(priv->dev, "vendor info not correct (%d)\n", vendor);
> > +             return -EINVAL;
> > +     }
> > +
> > +     return 0;
> > +}
> > +
> > +static bool rt5759_is_accessible_reg(struct device *dev, unsigned int reg)
> > +{
> > +     struct rt5759_priv *priv = dev_get_drvdata(dev);
> > +
> > +     if (reg <= RT5759_REG_DCDCSET)
> > +             return true;
> > +
> > +     if (priv->chip_type == CHIP_TYPE_RT5759A && reg == RT5759A_REG_WDTEN)
> > +             return true;
> > +
> > +     return false;
> > +}
> > +
> > +static const struct regmap_config rt5759_regmap_config = {
> > +     .reg_bits = 8,
> > +     .val_bits = 8,
> > +     .max_register = RT5759A_REG_WDTEN,
> > +     .readable_reg = rt5759_is_accessible_reg,
> > +     .writeable_reg = rt5759_is_accessible_reg,
> > +};
> > +
> > +static int rt5759_probe(struct i2c_client *i2c)
> > +{
> > +     struct rt5759_priv *priv;
> > +     int ret;
> > +
> > +     priv = devm_kzalloc(&i2c->dev, sizeof(*priv), GFP_KERNEL);
> > +     if (!priv)
> > +             return -ENOMEM;
> > +
> > +     priv->dev = &i2c->dev;
> > +     priv->chip_type = (unsigned long)of_device_get_match_data(&i2c->dev);
> > +     i2c_set_clientdata(i2c, priv);
> > +
> > +     priv->regmap = devm_regmap_init_i2c(i2c, &rt5759_regmap_config);
> > +     if (IS_ERR(priv->regmap)) {
> > +             ret = PTR_ERR(priv->regmap);
> > +             dev_err(&i2c->dev, "Failed to allocate regmap (%d)\n", ret);
> > +             return ret;
> > +     }
> > +
> > +     ret = rt5759_manufacturer_check(priv);
> > +     if (ret) {
> > +             dev_err(&i2c->dev, "Failed to check device (%d)\n", ret);
> > +             return ret;
> > +     }
> > +
> > +     ret = rt5759_init_device_property(priv);
> > +     if (ret) {
> > +             dev_err(&i2c->dev, "Failed to init device (%d)\n", ret);
> > +             return ret;
> > +     }
> > +
> > +     return rt5759_regulator_register(priv);
> > +}
> > +
> > +static const struct of_device_id __maybe_unused rt5759_device_table[] = {
>
> I don't think this can be __maybe_unused. It is always referenced via
> of_match_table, isn't it?
>
I think it can declared as '__maybe_unused'.
If 'of_device_id' is unused, then in probe stage,
'of_device_get_match_data' will return NULL.
priv->chip_type will get zero as the return value. And it will be
treated as rt5759, not rt5759a.
The difference between these two are only watchdog function supported or not.

> > +     { .compatible = "richtek,rt5759", .data = (void *)CHIP_TYPE_RT5759 },
> > +     { .compatible = "richtek,rt5759a", .data = (void *)CHIP_TYPE_RT5759A },
> > +     {}
> > +};
> > +MODULE_DEVICE_TABLE(of, rt5759_device_table);
> > +
> > +static struct i2c_driver rt5759_driver = {
> > +     .driver = {
> > +             .name = "rt5759",
> > +             .of_match_table = rt5759_device_table,
> > +     },
> > +     .probe_new = rt5759_probe,
> > +};
> > +module_i2c_driver(rt5759_driver);
> > +
> > +MODULE_AUTHOR("ChiYuan Huang <cy_huang@richtek.com>");
> > +MODULE_DESCRIPTION("Richtek RT5759 Regulator Driver");
> > +MODULE_LICENSE("GPL v2");
>
>
> Best regards,
> Krzysztof

Re: [PATCH 1/2] dt-bindings: regulator: Add binding for Richtek RT5759 DCDC converter

[Krzysztof Kozlowski]

On 25/03/2022 14:44, ChiYuan Huang wrote:

(...)

>>> +unevaluatedProperties: false
>>> +
>>> +examples:
>>> +  # example 1 for RT5759
>>> +  - |
>>> +    i2c {
>>> +      #address-cells = <1>;
>>> +      #size-cells = <0>;
>>> +
>>> +      rt5759@62 {
>>
>> Generic node name, so pmic.
>>
> As my understanding, 'pmic' means there must be multiple channels of
> buck or ldo.
> But  rt5759 is only one channel buck converter.

Then "regulator". rt5759 is not a generic name but specific.

Best regards,
Krzysztof

Re: [PATCH 2/2] regulator: rt5759: Add support for Richtek RT5759 DCDC converter

[Krzysztof Kozlowski]

On 25/03/2022 15:10, ChiYuan Huang wrote:
> Krzysztof Kozlowski <krzk@kernel.org> 於 2022年3月25日 週五 下午8:17寫道：
>>
>> On 25/03/2022 02:06, cy_huang wrote:
>>> From: ChiYuan Huang <cy_huang@richtek.com>
>>>
>>> Add support for Richtek RT5759 high-performance DCDC converter.
>>>
>>> Signed-off-by: ChiYuan Huang <cy_huang@richtek.com>
>>> ---
>>>  drivers/regulator/Kconfig            |  10 +
>>>  drivers/regulator/Makefile           |   1 +
>>>  drivers/regulator/rt5759-regulator.c | 372 +++++++++++++++++++++++++++++++++++
>>>  3 files changed, 383 insertions(+)
>>>  create mode 100644 drivers/regulator/rt5759-regulator.c
>>>
>>
>> (...)
>>
>>> +static int rt5759_init_device_property(struct rt5759_priv *priv)
>>> +{
>>> +     unsigned int val = 0;
>>> +     bool wdt_enable;
>>> +
>>> +     /*
>>> +      * Only RT5759A support external watchdog input
>>> +      */
>>> +     if (priv->chip_type != CHIP_TYPE_RT5759A)
>>> +             return 0;
>>> +
>>> +     wdt_enable = device_property_read_bool(priv->dev,
>>> +                                            "richtek,watchdog-enable");
>>> +     if (wdt_enable)
>>
>> No need for separate wdt_enable variable.
>>
> Ack in next.
>>> +             val = RT5759A_WDTEN_MASK;
>>> +
>>> +     return regmap_update_bits(priv->regmap, RT5759A_REG_WDTEN,
>>> +                               RT5759A_WDTEN_MASK, val);
>>> +}
>>> +
>>> +static int rt5759_manufacturer_check(struct rt5759_priv *priv)
>>> +{
>>> +     unsigned int vendor;
>>> +     int ret;
>>> +
>>> +     ret = regmap_read(priv->regmap, RT5759_REG_VENDORINFO, &vendor);
>>> +     if (ret)
>>> +             return ret;
>>> +
>>> +     if (vendor != RT5759_MANUFACTURER_ID) {
>>> +             dev_err(priv->dev, "vendor info not correct (%d)\n", vendor);
>>> +             return -EINVAL;
>>> +     }
>>> +
>>> +     return 0;
>>> +}
>>> +
>>> +static bool rt5759_is_accessible_reg(struct device *dev, unsigned int reg)
>>> +{
>>> +     struct rt5759_priv *priv = dev_get_drvdata(dev);
>>> +
>>> +     if (reg <= RT5759_REG_DCDCSET)
>>> +             return true;
>>> +
>>> +     if (priv->chip_type == CHIP_TYPE_RT5759A && reg == RT5759A_REG_WDTEN)
>>> +             return true;
>>> +
>>> +     return false;
>>> +}
>>> +
>>> +static const struct regmap_config rt5759_regmap_config = {
>>> +     .reg_bits = 8,
>>> +     .val_bits = 8,
>>> +     .max_register = RT5759A_REG_WDTEN,
>>> +     .readable_reg = rt5759_is_accessible_reg,
>>> +     .writeable_reg = rt5759_is_accessible_reg,
>>> +};
>>> +
>>> +static int rt5759_probe(struct i2c_client *i2c)
>>> +{
>>> +     struct rt5759_priv *priv;
>>> +     int ret;
>>> +
>>> +     priv = devm_kzalloc(&i2c->dev, sizeof(*priv), GFP_KERNEL);
>>> +     if (!priv)
>>> +             return -ENOMEM;
>>> +
>>> +     priv->dev = &i2c->dev;
>>> +     priv->chip_type = (unsigned long)of_device_get_match_data(&i2c->dev);
>>> +     i2c_set_clientdata(i2c, priv);
>>> +
>>> +     priv->regmap = devm_regmap_init_i2c(i2c, &rt5759_regmap_config);
>>> +     if (IS_ERR(priv->regmap)) {
>>> +             ret = PTR_ERR(priv->regmap);
>>> +             dev_err(&i2c->dev, "Failed to allocate regmap (%d)\n", ret);
>>> +             return ret;
>>> +     }
>>> +
>>> +     ret = rt5759_manufacturer_check(priv);
>>> +     if (ret) {
>>> +             dev_err(&i2c->dev, "Failed to check device (%d)\n", ret);
>>> +             return ret;
>>> +     }
>>> +
>>> +     ret = rt5759_init_device_property(priv);
>>> +     if (ret) {
>>> +             dev_err(&i2c->dev, "Failed to init device (%d)\n", ret);
>>> +             return ret;
>>> +     }
>>> +
>>> +     return rt5759_regulator_register(priv);
>>> +}
>>> +
>>> +static const struct of_device_id __maybe_unused rt5759_device_table[] = {
>>
>> I don't think this can be __maybe_unused. It is always referenced via
>> of_match_table, isn't it?
>>
> I think it can declared as '__maybe_unused'.
> If 'of_device_id' is unused, then in probe stage,
> 'of_device_get_match_data' will return NULL.

But your of_device_id cannot be unused. It is always referenced.

> priv->chip_type will get zero as the return value. And it will be
> treated as rt5759, not rt5759a.
> The difference between these two are only watchdog function supported or not.


Best regards,
Krzysztof

Re: [PATCH 1/2] dt-bindings: regulator: Add binding for Richtek RT5759 DCDC converter

[ChiYuan Huang]

Krzysztof Kozlowski <krzk@kernel.org> 於 2022年3月25日 週五 下午10:46寫道：
>
> On 25/03/2022 14:44, ChiYuan Huang wrote:
>
> (...)
>
> >>> +unevaluatedProperties: false
> >>> +
> >>> +examples:
> >>> +  # example 1 for RT5759
> >>> +  - |
> >>> +    i2c {
> >>> +      #address-cells = <1>;
> >>> +      #size-cells = <0>;
> >>> +
> >>> +      rt5759@62 {
> >>
> >> Generic node name, so pmic.
> >>
> > As my understanding, 'pmic' means there must be multiple channels of
> > buck or ldo.
> > But  rt5759 is only one channel buck converter.
>
> Then "regulator". rt5759 is not a generic name but specific.
>
OK, it's more generic node name for this kind of single channel buck.
> Best regards,
> Krzysztof

Re: [PATCH 2/2] regulator: rt5759: Add support for Richtek RT5759 DCDC converter

[ChiYuan Huang]

Krzysztof Kozlowski <krzk@kernel.org> 於 2022年3月25日 週五 下午10:47寫道：
>
> On 25/03/2022 15:10, ChiYuan Huang wrote:
> > Krzysztof Kozlowski <krzk@kernel.org> 於 2022年3月25日 週五 下午8:17寫道：
> >>
> >> On 25/03/2022 02:06, cy_huang wrote:
> >>> From: ChiYuan Huang <cy_huang@richtek.com>
> >>>
> >>> Add support for Richtek RT5759 high-performance DCDC converter.
> >>>
> >>> Signed-off-by: ChiYuan Huang <cy_huang@richtek.com>
> >>> ---
> >>>  drivers/regulator/Kconfig            |  10 +
> >>>  drivers/regulator/Makefile           |   1 +
> >>>  drivers/regulator/rt5759-regulator.c | 372 +++++++++++++++++++++++++++++++++++
> >>>  3 files changed, 383 insertions(+)
> >>>  create mode 100644 drivers/regulator/rt5759-regulator.c
> >>>
> >>
> >> (...)
> >>
> >>> +static int rt5759_init_device_property(struct rt5759_priv *priv)
> >>> +{
> >>> +     unsigned int val = 0;
> >>> +     bool wdt_enable;
> >>> +
> >>> +     /*
> >>> +      * Only RT5759A support external watchdog input
> >>> +      */
> >>> +     if (priv->chip_type != CHIP_TYPE_RT5759A)
> >>> +             return 0;
> >>> +
> >>> +     wdt_enable = device_property_read_bool(priv->dev,
> >>> +                                            "richtek,watchdog-enable");
> >>> +     if (wdt_enable)
> >>
> >> No need for separate wdt_enable variable.
> >>
> > Ack in next.
> >>> +             val = RT5759A_WDTEN_MASK;
> >>> +
> >>> +     return regmap_update_bits(priv->regmap, RT5759A_REG_WDTEN,
> >>> +                               RT5759A_WDTEN_MASK, val);
> >>> +}
> >>> +
> >>> +static int rt5759_manufacturer_check(struct rt5759_priv *priv)
> >>> +{
> >>> +     unsigned int vendor;
> >>> +     int ret;
> >>> +
> >>> +     ret = regmap_read(priv->regmap, RT5759_REG_VENDORINFO, &vendor);
> >>> +     if (ret)
> >>> +             return ret;
> >>> +
> >>> +     if (vendor != RT5759_MANUFACTURER_ID) {
> >>> +             dev_err(priv->dev, "vendor info not correct (%d)\n", vendor);
> >>> +             return -EINVAL;
> >>> +     }
> >>> +
> >>> +     return 0;
> >>> +}
> >>> +
> >>> +static bool rt5759_is_accessible_reg(struct device *dev, unsigned int reg)
> >>> +{
> >>> +     struct rt5759_priv *priv = dev_get_drvdata(dev);
> >>> +
> >>> +     if (reg <= RT5759_REG_DCDCSET)
> >>> +             return true;
> >>> +
> >>> +     if (priv->chip_type == CHIP_TYPE_RT5759A && reg == RT5759A_REG_WDTEN)
> >>> +             return true;
> >>> +
> >>> +     return false;
> >>> +}
> >>> +
> >>> +static const struct regmap_config rt5759_regmap_config = {
> >>> +     .reg_bits = 8,
> >>> +     .val_bits = 8,
> >>> +     .max_register = RT5759A_REG_WDTEN,
> >>> +     .readable_reg = rt5759_is_accessible_reg,
> >>> +     .writeable_reg = rt5759_is_accessible_reg,
> >>> +};
> >>> +
> >>> +static int rt5759_probe(struct i2c_client *i2c)
> >>> +{
> >>> +     struct rt5759_priv *priv;
> >>> +     int ret;
> >>> +
> >>> +     priv = devm_kzalloc(&i2c->dev, sizeof(*priv), GFP_KERNEL);
> >>> +     if (!priv)
> >>> +             return -ENOMEM;
> >>> +
> >>> +     priv->dev = &i2c->dev;
> >>> +     priv->chip_type = (unsigned long)of_device_get_match_data(&i2c->dev);
> >>> +     i2c_set_clientdata(i2c, priv);
> >>> +
> >>> +     priv->regmap = devm_regmap_init_i2c(i2c, &rt5759_regmap_config);
> >>> +     if (IS_ERR(priv->regmap)) {
> >>> +             ret = PTR_ERR(priv->regmap);
> >>> +             dev_err(&i2c->dev, "Failed to allocate regmap (%d)\n", ret);
> >>> +             return ret;
> >>> +     }
> >>> +
> >>> +     ret = rt5759_manufacturer_check(priv);
> >>> +     if (ret) {
> >>> +             dev_err(&i2c->dev, "Failed to check device (%d)\n", ret);
> >>> +             return ret;
> >>> +     }
> >>> +
> >>> +     ret = rt5759_init_device_property(priv);
> >>> +     if (ret) {
> >>> +             dev_err(&i2c->dev, "Failed to init device (%d)\n", ret);
> >>> +             return ret;
> >>> +     }
> >>> +
> >>> +     return rt5759_regulator_register(priv);
> >>> +}
> >>> +
> >>> +static const struct of_device_id __maybe_unused rt5759_device_table[] = {
> >>
> >> I don't think this can be __maybe_unused. It is always referenced via
> >> of_match_table, isn't it?
> >>
> > I think it can declared as '__maybe_unused'.
> > If 'of_device_id' is unused, then in probe stage,
> > 'of_device_get_match_data' will return NULL.
>
> But your of_device_id cannot be unused. It is always referenced.
>
I'm not sure, but your assumption is based on 'CONFIG_OF', right?
Only if 'CONFIG_OF' is not defined, then it'll be really unused.

> > priv->chip_type will get zero as the return value. And it will be
> > treated as rt5759, not rt5759a.
> > The difference between these two are only watchdog function supported or not.
>
>
> Best regards,
> Krzysztof

Re: [PATCH 2/2] regulator: rt5759: Add support for Richtek RT5759 DCDC converter

[Krzysztof Kozlowski]

On 25/03/2022 15:59, ChiYuan Huang wrote:
> Krzysztof Kozlowski <krzk@kernel.org> 於 2022年3月25日 週五 下午10:47寫道：
>>
>>>>> +
>>>>> +static const struct of_device_id __maybe_unused rt5759_device_table[] = {
>>>>
>>>> I don't think this can be __maybe_unused. It is always referenced via
>>>> of_match_table, isn't it?
>>>>
>>> I think it can declared as '__maybe_unused'.
>>> If 'of_device_id' is unused, then in probe stage,
>>> 'of_device_get_match_data' will return NULL.
>>
>> But your of_device_id cannot be unused. It is always referenced.
>>
> I'm not sure, but your assumption is based on 'CONFIG_OF', right?
> Only if 'CONFIG_OF' is not defined, then it'll be really unused.

Is it possible to build this driver without CONFIG_OF? Did you try it?

Best regards,
Krzysztof

Re: [PATCH 2/2] regulator: rt5759: Add support for Richtek RT5759 DCDC converter

[ChiYuan Huang]

Krzysztof Kozlowski <krzk@kernel.org> 於 2022年3月25日 週五 下午11:37寫道：
>
> On 25/03/2022 15:59, ChiYuan Huang wrote:
> > Krzysztof Kozlowski <krzk@kernel.org> 於 2022年3月25日 週五 下午10:47寫道：
> >>
> >>>>> +
> >>>>> +static const struct of_device_id __maybe_unused rt5759_device_table[] = {
> >>>>
> >>>> I don't think this can be __maybe_unused. It is always referenced via
> >>>> of_match_table, isn't it?
> >>>>
> >>> I think it can declared as '__maybe_unused'.
> >>> If 'of_device_id' is unused, then in probe stage,
> >>> 'of_device_get_match_data' will return NULL.
> >>
> >> But your of_device_id cannot be unused. It is always referenced.
> >>
> > I'm not sure, but your assumption is based on 'CONFIG_OF', right?
> > Only if 'CONFIG_OF' is not defined, then it'll be really unused.
>
> Is it possible to build this driver without CONFIG_OF? Did you try it?
>
No, my development board always  use device tree that's defined CONFIG_OF.

But theoretically, you can also check of_device.h, if CONFIG_OF is not
defined, 'of_device_get_match_data' always return NULL.
If so, my usage is still right.
> Best regards,
> Krzysztof

Re: [PATCH 2/2] regulator: rt5759: Add support for Richtek RT5759 DCDC converter

[Krzysztof Kozlowski]

On 25/03/2022 16:50, ChiYuan Huang wrote:
> Krzysztof Kozlowski <krzk@kernel.org> 於 2022年3月25日 週五 下午11:37寫道：
>>
>> On 25/03/2022 15:59, ChiYuan Huang wrote:
>>> Krzysztof Kozlowski <krzk@kernel.org> 於 2022年3月25日 週五 下午10:47寫道：
>>>>
>>>>>>> +
>>>>>>> +static const struct of_device_id __maybe_unused rt5759_device_table[] = {
>>>>>>
>>>>>> I don't think this can be __maybe_unused. It is always referenced via
>>>>>> of_match_table, isn't it?
>>>>>>
>>>>> I think it can declared as '__maybe_unused'.
>>>>> If 'of_device_id' is unused, then in probe stage,
>>>>> 'of_device_get_match_data' will return NULL.
>>>>
>>>> But your of_device_id cannot be unused. It is always referenced.
>>>>
>>> I'm not sure, but your assumption is based on 'CONFIG_OF', right?
>>> Only if 'CONFIG_OF' is not defined, then it'll be really unused.
>>
>> Is it possible to build this driver without CONFIG_OF? Did you try it?
>>
> No, my development board always  use device tree that's defined CONFIG_OF.
> 
> But theoretically, you can also check of_device.h, if CONFIG_OF is not
> defined, 'of_device_get_match_data' always return NULL.
> If so, my usage is still right.

No, it does not look right and your arguments are not even related to
the topic. I don't think we talk about the same thing.

You mention board, some of_device_get_match_data() so you talk about
runtime. maybe_unused is not about runtime. It is about build time.

The code you sent cannot have this structure unused. If you think
otherwise, please provide argument, but not about runtime (again). You
can for example build it without OF and see...

Best regards,
Krzysztof

Re: [PATCH 2/2] regulator: rt5759: Add support for Richtek RT5759 DCDC converter

[Mark Brown]

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

On Fri, Mar 25, 2022 at 04:55:25PM +0100, Krzysztof Kozlowski wrote:

> You mention board, some of_device_get_match_data() so you talk about
> runtime. maybe_unused is not about runtime. It is about build time.

> The code you sent cannot have this structure unused. If you think
> otherwise, please provide argument, but not about runtime (again). You
> can for example build it without OF and see...

If you use of_match_ptr() in the struct device (which is good practice,
didn't check if this driver does it) then that causes the ID table to be
unreferenced as of_match_ptr() compiles to NULL when !OF.

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

Re: [PATCH 2/2] regulator: rt5759: Add support for Richtek RT5759 DCDC converter

[Krzysztof Kozlowski]

On 25/03/2022 17:05, Mark Brown wrote:
> On Fri, Mar 25, 2022 at 04:55:25PM +0100, Krzysztof Kozlowski wrote:
> 
>> You mention board, some of_device_get_match_data() so you talk about
>> runtime. maybe_unused is not about runtime. It is about build time.
> 
>> The code you sent cannot have this structure unused. If you think
>> otherwise, please provide argument, but not about runtime (again). You
>> can for example build it without OF and see...
> 
> If you use of_match_ptr() in the struct device (which is good practice,
> didn't check if this driver does it) then that causes the ID table to be
> unreferenced as of_match_ptr() compiles to NULL when !OF.

Yep, then the case is obvious, but the driver does not use it.

+static struct i2c_driver rt5759_driver = {
+	.driver = {
+		.name = "rt5759",
+		.of_match_table = rt5759_device_table,

Therefore the of_device_id cannot be unused, so __maybe_unused is not
correct. This can be fixed in two different ways, which we did not
discuss yet...

Best regards,
Krzysztof

Re: [PATCH 2/2] regulator: rt5759: Add support for Richtek RT5759 DCDC converter

[ChiYuan Huang]

Krzysztof Kozlowski <krzk@kernel.org> 於 2022年3月26日 週六 上午12:08寫道：
>
> On 25/03/2022 17:05, Mark Brown wrote:
> > On Fri, Mar 25, 2022 at 04:55:25PM +0100, Krzysztof Kozlowski wrote:
> >
> >> You mention board, some of_device_get_match_data() so you talk about
> >> runtime. maybe_unused is not about runtime. It is about build time.
> >
> >> The code you sent cannot have this structure unused. If you think
> >> otherwise, please provide argument, but not about runtime (again). You
> >> can for example build it without OF and see...
> >
> > If you use of_match_ptr() in the struct device (which is good practice,
> > didn't check if this driver does it) then that causes the ID table to be
> > unreferenced as of_match_ptr() compiles to NULL when !OF.
>
> Yep, then the case is obvious, but the driver does not use it.
>
> +static struct i2c_driver rt5759_driver = {
> +       .driver = {
> +               .name = "rt5759",
> +               .of_match_table = rt5759_device_table,
>
> Therefore the of_device_id cannot be unused, so __maybe_unused is not
> correct. This can be fixed in two different ways, which we did not
> discuss yet...
>
As my past experience, '__maybe_unused' must be added to fix W=1 build
warning when OF=n
You can refer to the below link.
https://lore.kernel.org/lkml/1598234713-8532-1-git-send-email-u0084500@gmail.com/

And it's based on 'of_match_ptr' is used.

I tried to remove only __maybe_unused and build with x86 config  that
CONFIG_OF=n.
There's no warning or error message when compiling the rt5759 source code.

If so, I will remove only '__maybe_unused'.
May I ask whether 'of_match_ptr'  need to be added or not?

> Best regards,
> Krzysztof

Re: [PATCH 2/2] regulator: rt5759: Add support for Richtek RT5759 DCDC converter

[Mark Brown]

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

On Sat, Mar 26, 2022 at 08:58:47AM +0800, ChiYuan Huang wrote:

> I tried to remove only __maybe_unused and build with x86 config  that
> CONFIG_OF=n.
> There's no warning or error message when compiling the rt5759 source code.

> If so, I will remove only '__maybe_unused'.
> May I ask whether 'of_match_ptr'  need to be added or not?

If you add of_match_ptr() (which is a little better, though it's
a tiny different either way) then you shouldn't remove
__maybe_unused - the thing here is that the __maybe_unused is
needed because of_match_ptr() is used.

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

Re: [PATCH 2/2] regulator: rt5759: Add support for Richtek RT5759 DCDC converter

[ChiYuan Huang]

Mark Brown <broonie@kernel.org> 於 2022年3月26日 週六 上午9:07寫道：
>
> On Sat, Mar 26, 2022 at 08:58:47AM +0800, ChiYuan Huang wrote:
>
> > I tried to remove only __maybe_unused and build with x86 config  that
> > CONFIG_OF=n.
> > There's no warning or error message when compiling the rt5759 source code.
>
> > If so, I will remove only '__maybe_unused'.
> > May I ask whether 'of_match_ptr'  need to be added or not?
>
> If you add of_match_ptr() (which is a little better, though it's
> a tiny different either way) then you shouldn't remove
> __maybe_unused - the thing here is that the __maybe_unused is
> needed because of_match_ptr() is used.

Sorry, I'm confused.
Originally, Krzysztof's opinion is to tell me why 'of_device_id' is
declared as '__maybe_unused'.
That's why I mentioned that the return value  about of_device_get_match_data'
And now we're talking about to add 'of_match_ptr' in struct driver
of_match_table.

Back to the original topic, two ways can solve this.
1) only remove '__maybe_unused' in of_device_id
2) keep '__maybe_unused' in of_device_id, and add of_match_ptr for
of_match_table.
But option 2 seems conflict with Krzysztof's gueestion.

May I ask which option you suggested?

Re: [PATCH 2/2] regulator: rt5759: Add support for Richtek RT5759 DCDC converter

[Krzysztof Kozlowski]

On 26/03/2022 08:55, ChiYuan Huang wrote:
> Mark Brown <broonie@kernel.org> 於 2022年3月26日 週六 上午9:07寫道：
>>
>> On Sat, Mar 26, 2022 at 08:58:47AM +0800, ChiYuan Huang wrote:
>>
>>> I tried to remove only __maybe_unused and build with x86 config  that
>>> CONFIG_OF=n.
>>> There's no warning or error message when compiling the rt5759 source code.
>>
>>> If so, I will remove only '__maybe_unused'.
>>> May I ask whether 'of_match_ptr'  need to be added or not?
>>
>> If you add of_match_ptr() (which is a little better, though it's
>> a tiny different either way) then you shouldn't remove
>> __maybe_unused - the thing here is that the __maybe_unused is
>> needed because of_match_ptr() is used.
> 
> Sorry, I'm confused.
> Originally, Krzysztof's opinion is to tell me why 'of_device_id' is
> declared as '__maybe_unused'.
> That's why I mentioned that the return value  about of_device_get_match_data'

Your answer is not related to my question. of_device_get_match_data()
has nothing to do with __maybe_unused...

> And now we're talking about to add 'of_match_ptr' in struct driver
> of_match_table.

Because this is one of the solutions.

> 
> Back to the original topic, two ways can solve this.
> 1) only remove '__maybe_unused' in of_device_id
> 2) keep '__maybe_unused' in of_device_id, and add of_match_ptr for
> of_match_table.
> But option 2 seems conflict with Krzysztof's gueestion.
> 
> May I ask which option you suggested?

Option two does not conflict my suggestion. I pointed out that having
ONLY maybe_unused is incorrect. I pointed the mistake. Nothing more... I
said that there are two ways to solve it later, just choose one. I don't
know why do we talk about such basic issue for so long. This should be
one email from my side and one confirmation from you...

Obviously maybe_unused it has to be removed if you do not add
of_match_ptr. But if you intend to add of_match_ptr, then things change...

Just for the record of choosing between options (which I also mentioned
that there are two solutions) - having no of_match_ptr allows to match
with ACPI PRP0001 (AFAIU also when !OF).


Best regards,
Krzysztof

Re: [PATCH 2/2] regulator: rt5759: Add support for Richtek RT5759 DCDC converter

[ChiYuan Huang]

Krzysztof Kozlowski <krzk@kernel.org> 於 2022年3月26日 週六 下午4:13寫道：
>
> On 26/03/2022 08:55, ChiYuan Huang wrote:
> > Mark Brown <broonie@kernel.org> 於 2022年3月26日 週六 上午9:07寫道：
> >>
> >> On Sat, Mar 26, 2022 at 08:58:47AM +0800, ChiYuan Huang wrote:
> >>
> >>> I tried to remove only __maybe_unused and build with x86 config  that
> >>> CONFIG_OF=n.
> >>> There's no warning or error message when compiling the rt5759 source code.
> >>
> >>> If so, I will remove only '__maybe_unused'.
> >>> May I ask whether 'of_match_ptr'  need to be added or not?
> >>
> >> If you add of_match_ptr() (which is a little better, though it's
> >> a tiny different either way) then you shouldn't remove
> >> __maybe_unused - the thing here is that the __maybe_unused is
> >> needed because of_match_ptr() is used.
> >
> > Sorry, I'm confused.
> > Originally, Krzysztof's opinion is to tell me why 'of_device_id' is
> > declared as '__maybe_unused'.
> > That's why I mentioned that the return value  about of_device_get_match_data'
>
> Your answer is not related to my question. of_device_get_match_data()
> has nothing to do with __maybe_unused...
>
> > And now we're talking about to add 'of_match_ptr' in struct driver
> > of_match_table.
>
> Because this is one of the solutions.
>
> >
> > Back to the original topic, two ways can solve this.
> > 1) only remove '__maybe_unused' in of_device_id
> > 2) keep '__maybe_unused' in of_device_id, and add of_match_ptr for
> > of_match_table.
> > But option 2 seems conflict with Krzysztof's gueestion.
> >
> > May I ask which option you suggested?
>
> Option two does not conflict my suggestion. I pointed out that having
> ONLY maybe_unused is incorrect. I pointed the mistake. Nothing more... I
> said that there are two ways to solve it later, just choose one. I don't
> know why do we talk about such basic issue for so long. This should be
> one email from my side and one confirmation from you...
>
> Obviously maybe_unused it has to be removed if you do not add
> of_match_ptr. But if you intend to add of_match_ptr, then things change...
>
> Just for the record of choosing between options (which I also mentioned
> that there are two solutions) - having no of_match_ptr allows to match
> with ACPI PRP0001 (AFAIU also when !OF).
>
Ok, I think the way to add 'of_match_ptr' and keep '__maybe_unused' is
more formal.
I'll choose that one.

Ack in next.
Thanks.
>
> Best regards,
> Krzysztof