[PATCH v2 0/2] regulator: Add support for TPS6287x

[PATCH v2 0/2] regulator: Add support for TPS6287x

[Mårten Lindahl]

This series adds basic support for TI's TPS62870/TPS62871/TPS62872/
TPS62873 high-frequency single-channel step-down converters with an
I2C interface.

The devices can operate in power save mode for maximum efficiency, or
forced-PWM mode for best transient performance and lowest output
voltage ripple. All chip variants have four output voltage ranges and
the active range is set via the I2C interface.

There are differences in the electrical characteristics and packaging
between the variants, but the register interfaces are identical.

Signed-off-by: Mårten Lindahl <marten.lindahl@axis.com>
---
Mårten Lindahl (2):
      regulator: Add bindings for TPS6287x
      regulator: Add support for TI TPS6287x regulators

 .../devicetree/bindings/regulator/ti,tps62870.yaml |  62 +++++
 drivers/regulator/Kconfig                          |  11 +
 drivers/regulator/Makefile                         |   1 +
 drivers/regulator/tps6287x-regulator.c             | 289 +++++++++++++++++++++
 4 files changed, 363 insertions(+)
---
base-commit: 457391b0380335d5e9a5babdec90ac53928b23b4
change-id: 20230502-tps6287x-driver-af1474b3111e

Best regards,
-- 
Mårten Lindahl <marten.lindahl@axis.com>

[PATCH v2 1/2] regulator: Add bindings for TPS6287x

[Mårten Lindahl]

Add bindings for the TPS62870/TPS62871/TPS62872/TPS62873 voltage
regulators.

Signed-off-by: Mårten Lindahl <marten.lindahl@axis.com>
---
 .../devicetree/bindings/regulator/ti,tps62870.yaml | 62 ++++++++++++++++++++++
 1 file changed, 62 insertions(+)

diff --git a/Documentation/devicetree/bindings/regulator/ti,tps62870.yaml b/Documentation/devicetree/bindings/regulator/ti,tps62870.yaml
new file mode 100644
index 000000000000..32f259f16314
--- /dev/null
+++ b/Documentation/devicetree/bindings/regulator/ti,tps62870.yaml
@@ -0,0 +1,62 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/regulator/ti,tps62870.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: TI TPS62870/TPS62871/TPS62872/TPS62873 voltage regulator
+
+maintainers:
+  - Mårten Lindahl <marten.lindahl@axis.com>
+
+properties:
+  compatible:
+    enum:
+      - ti,tps62870
+      - ti,tps62871
+      - ti,tps62872
+      - ti,tps62873
+
+  reg:
+    maxItems: 1
+
+  regulators:
+    type: object
+
+    properties:
+      "vout":
+        type: object
+        $ref: regulator.yaml#
+        unevaluatedProperties: false
+
+    additionalProperties: false
+
+required:
+  - compatible
+  - reg
+  - regulators
+
+additionalProperties: false
+
+examples:
+  - |
+    i2c {
+      #address-cells = <1>;
+      #size-cells = <0>;
+
+      regulator@41 {
+        compatible = "ti,tps62873";
+        reg = <0x41>;
+
+        regulators {
+          vout {
+            regulator-name = "+0.75V";
+            regulator-min-microvolt = <400000>;
+            regulator-max-microvolt = <1675000>;
+            regulator-initial-mode = <2>;
+          };
+        };
+      };
+    };
+
+...

-- 
2.30.2

Re: [PATCH v2 1/2] regulator: Add bindings for TPS6287x

[Krzysztof Kozlowski]

On 04/05/2023 10:30, Mårten Lindahl wrote:
> Add bindings for the TPS62870/TPS62871/TPS62872/TPS62873 voltage
> regulators.
> 

Use subject prefixes matching the subsystem (which you can get for
example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
your patch is touching).

Just a hint - I in general ignore all the emails without dt-bindings prefix.

> Signed-off-by: Mårten Lindahl <marten.lindahl@axis.com>
> ---
>  .../devicetree/bindings/regulator/ti,tps62870.yaml | 62 ++++++++++++++++++++++
>  1 file changed, 62 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/regulator/ti,tps62870.yaml b/Documentation/devicetree/bindings/regulator/ti,tps62870.yaml
> new file mode 100644
> index 000000000000..32f259f16314
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/regulator/ti,tps62870.yaml
> @@ -0,0 +1,62 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/regulator/ti,tps62870.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: TI TPS62870/TPS62871/TPS62872/TPS62873 voltage regulator
> +
> +maintainers:
> +  - Mårten Lindahl <marten.lindahl@axis.com>
> +
> +properties:
> +  compatible:
> +    enum:
> +      - ti,tps62870
> +      - ti,tps62871
> +      - ti,tps62872
> +      - ti,tps62873
> +
> +  reg:
> +    maxItems: 1
> +
> +  regulators:
> +    type: object
> +
> +    properties:
> +      "vout":

Drop quotes.

Why do you need entire "regulators" node for one regulator? Why do you
need child at first place. Drop it entirely.


> +        type: object
> +        $ref: regulator.yaml#
> +        unevaluatedProperties: false

You missed that piece of explanation:

"The set of possible operating modes depends on the capabilities of
every hardware so each device binding documentation explains which
values the regulator supports."



Best regards,
Krzysztof

Re: [PATCH v2 1/2] regulator: Add bindings for TPS6287x

[Mårten Lindahl]

Hi Krzysztof!

On 5/4/23 11:34, Krzysztof Kozlowski wrote:
> On 04/05/2023 10:30, Mårten Lindahl wrote:
>> Add bindings for the TPS62870/TPS62871/TPS62872/TPS62873 voltage
>> regulators.
>>
> Use subject prefixes matching the subsystem (which you can get for
> example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
> your patch is touching).
>
> Just a hint - I in general ignore all the emails without dt-bindings prefix.
Ok, I'll prefix it "dt-bindings: regulator:"
>
>> Signed-off-by: Mårten Lindahl <marten.lindahl@axis.com>
>> ---
>>   .../devicetree/bindings/regulator/ti,tps62870.yaml | 62 ++++++++++++++++++++++
>>   1 file changed, 62 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/regulator/ti,tps62870.yaml b/Documentation/devicetree/bindings/regulator/ti,tps62870.yaml
>> new file mode 100644
>> index 000000000000..32f259f16314
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/regulator/ti,tps62870.yaml
>> @@ -0,0 +1,62 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/regulator/ti,tps62870.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: TI TPS62870/TPS62871/TPS62872/TPS62873 voltage regulator
>> +
>> +maintainers:
>> +  - Mårten Lindahl <marten.lindahl@axis.com>
>> +
>> +properties:
>> +  compatible:
>> +    enum:
>> +      - ti,tps62870
>> +      - ti,tps62871
>> +      - ti,tps62872
>> +      - ti,tps62873
>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +  regulators:
>> +    type: object
>> +
>> +    properties:
>> +      "vout":
> Drop quotes.
>
> Why do you need entire "regulators" node for one regulator? Why do you
> need child at first place. Drop it entirely.
I will remove the regulators node. I think the vout node is needed to 
get the of_get_regulator_init_data.
>
>
>> +        type: object
>> +        $ref: regulator.yaml#
>> +        unevaluatedProperties: false
> You missed that piece of explanation:
>
> "The set of possible operating modes depends on the capabilities of
> every hardware so each device binding documentation explains which
> values the regulator supports."

Ok, I will add a description for the valid regulator-initial-mode values.

Thanks!

Kind regards

Mårten

>
>
>
> Best regards,
> Krzysztof
>

Re: [PATCH v2 1/2] regulator: Add bindings for TPS6287x

[Krzysztof Kozlowski]

On 04/05/2023 17:08, Mårten Lindahl wrote:
> Hi Krzysztof!
> 
> On 5/4/23 11:34, Krzysztof Kozlowski wrote:
>> On 04/05/2023 10:30, Mårten Lindahl wrote:
>>> Add bindings for the TPS62870/TPS62871/TPS62872/TPS62873 voltage
>>> regulators.
>>>
>> Use subject prefixes matching the subsystem (which you can get for
>> example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
>> your patch is touching).
>>
>> Just a hint - I in general ignore all the emails without dt-bindings prefix.
> Ok, I'll prefix it "dt-bindings: regulator:"

You got command to run, so run it.  This semi-automated response is made
longer for the purpose to help you, not to be quickly scrolled and
ignored. When you run it you will see the order is opposite, regulator
followed by dt-bindings.

You can apply such habit for other subsystems where maintainers also
expect certain prefixes.


>>
>>> Signed-off-by: Mårten Lindahl <marten.lindahl@axis.com>
>>> ---
>>>   .../devicetree/bindings/regulator/ti,tps62870.yaml | 62 ++++++++++++++++++++++
>>>   1 file changed, 62 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/regulator/ti,tps62870.yaml b/Documentation/devicetree/bindings/regulator/ti,tps62870.yaml
>>> new file mode 100644
>>> index 000000000000..32f259f16314
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/regulator/ti,tps62870.yaml
>>> @@ -0,0 +1,62 @@
>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>> +%YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/regulator/ti,tps62870.yaml#
>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>> +
>>> +title: TI TPS62870/TPS62871/TPS62872/TPS62873 voltage regulator
>>> +
>>> +maintainers:
>>> +  - Mårten Lindahl <marten.lindahl@axis.com>
>>> +
>>> +properties:
>>> +  compatible:
>>> +    enum:
>>> +      - ti,tps62870
>>> +      - ti,tps62871
>>> +      - ti,tps62872
>>> +      - ti,tps62873
>>> +
>>> +  reg:
>>> +    maxItems: 1
>>> +
>>> +  regulators:
>>> +    type: object
>>> +
>>> +    properties:
>>> +      "vout":
>> Drop quotes.
>>
>> Why do you need entire "regulators" node for one regulator? Why do you
>> need child at first place. Drop it entirely.
> I will remove the regulators node. I think the vout node is needed to 
> get the of_get_regulator_init_data.

Hmmm, how other simple regulators deal with it? Like all the fixed ones
and few other one-regulator-devices?


Best regards,
Krzysztof

Re: [PATCH v2 1/2] regulator: Add bindings for TPS6287x

[Mårten Lindahl]

On 5/4/23 17:11, Krzysztof Kozlowski wrote:
> On 04/05/2023 17:08, Mårten Lindahl wrote:
>> Hi Krzysztof!
>>
>> On 5/4/23 11:34, Krzysztof Kozlowski wrote:
>>> On 04/05/2023 10:30, Mårten Lindahl wrote:
>>>> Add bindings for the TPS62870/TPS62871/TPS62872/TPS62873 voltage
>>>> regulators.
>>>>
>>> Use subject prefixes matching the subsystem (which you can get for
>>> example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
>>> your patch is touching).
>>>
>>> Just a hint - I in general ignore all the emails without dt-bindings prefix.
>> Ok, I'll prefix it "dt-bindings: regulator:"
> You got command to run, so run it.  This semi-automated response is made
> longer for the purpose to help you, not to be quickly scrolled and
> ignored. When you run it you will see the order is opposite, regulator
> followed by dt-bindings.
>
> You can apply such habit for other subsystems where maintainers also
> expect certain prefixes.

Hi! Sorry, I did run the command and followed the latest commit I could 
see (1ba7dfb905b3) as the result is mixed with prefixes.

But I will of course do as you request: "regulator: dt-bindings: ".

>
>
>>>> Signed-off-by: Mårten Lindahl <marten.lindahl@axis.com>
>>>> ---
>>>>    .../devicetree/bindings/regulator/ti,tps62870.yaml | 62 ++++++++++++++++++++++
>>>>    1 file changed, 62 insertions(+)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/regulator/ti,tps62870.yaml b/Documentation/devicetree/bindings/regulator/ti,tps62870.yaml
>>>> new file mode 100644
>>>> index 000000000000..32f259f16314
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/regulator/ti,tps62870.yaml
>>>> @@ -0,0 +1,62 @@
>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>>> +%YAML 1.2
>>>> +---
>>>> +$id: http://devicetree.org/schemas/regulator/ti,tps62870.yaml#
>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>> +
>>>> +title: TI TPS62870/TPS62871/TPS62872/TPS62873 voltage regulator
>>>> +
>>>> +maintainers:
>>>> +  - Mårten Lindahl <marten.lindahl@axis.com>
>>>> +
>>>> +properties:
>>>> +  compatible:
>>>> +    enum:
>>>> +      - ti,tps62870
>>>> +      - ti,tps62871
>>>> +      - ti,tps62872
>>>> +      - ti,tps62873
>>>> +
>>>> +  reg:
>>>> +    maxItems: 1
>>>> +
>>>> +  regulators:
>>>> +    type: object
>>>> +
>>>> +    properties:
>>>> +      "vout":
>>> Drop quotes.
>>>
>>> Why do you need entire "regulators" node for one regulator? Why do you
>>> need child at first place. Drop it entirely.
>> I will remove the regulators node. I think the vout node is needed to
>> get the of_get_regulator_init_data.
> Hmmm, how other simple regulators deal with it? Like all the fixed ones
> and few other one-regulator-devices?

Ok, I added of_get_regulator_init_data to the driver probe and then it 
works fine. I'll drop the child node.

Thanks!

Kind regards

Mårten

>
>
> Best regards,
> Krzysztof
>

[PATCH v2 2/2] regulator: Add support for TI TPS6287x regulators

[Mårten Lindahl]

Add support for Texas Instruments TPS6287x, single-channel
synchronous step-down converters with four output voltage ranges.

Signed-off-by: Mårten Lindahl <marten.lindahl@axis.com>
---
 drivers/regulator/Kconfig              |  11 ++
 drivers/regulator/Makefile             |   1 +
 drivers/regulator/tps6287x-regulator.c | 289 +++++++++++++++++++++++++++++++++
 3 files changed, 301 insertions(+)

diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
index aae28d0a489c..d7923ac53f53 100644
--- a/drivers/regulator/Kconfig
+++ b/drivers/regulator/Kconfig
@@ -1374,6 +1374,17 @@ config REGULATOR_TPS6286X
 	  high-frequency synchronous step-down converters with an I2C
 	  interface.
 
+config REGULATOR_TPS6287X
+	tristate "TI TPS6287x Power Regulator"
+	depends on I2C && OF
+	select REGMAP_I2C
+	help
+	  This driver supports TPS6287x voltage regulator chips. These are
+	  pin-to-pin high-frequency synchronous step-down dc-dc converters
+	  with an I2C interface.
+
+	  If built as a module it will be called tps6287x-regulator.
+
 config REGULATOR_TPS65023
 	tristate "TI TPS65023 Power regulators"
 	depends on I2C
diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
index ee383d8fc835..857995db3fa4 100644
--- a/drivers/regulator/Makefile
+++ b/drivers/regulator/Makefile
@@ -161,6 +161,7 @@ obj-$(CONFIG_REGULATOR_TI_ABB) += ti-abb-regulator.o
 obj-$(CONFIG_REGULATOR_TPS6105X) += tps6105x-regulator.o
 obj-$(CONFIG_REGULATOR_TPS62360) += tps62360-regulator.o
 obj-$(CONFIG_REGULATOR_TPS6286X) += tps6286x-regulator.o
+obj-$(CONFIG_REGULATOR_TPS6287X) += tps6287x-regulator.o
 obj-$(CONFIG_REGULATOR_TPS65023) += tps65023-regulator.o
 obj-$(CONFIG_REGULATOR_TPS6507X) += tps6507x-regulator.o
 obj-$(CONFIG_REGULATOR_TPS65086) += tps65086-regulator.o
diff --git a/drivers/regulator/tps6287x-regulator.c b/drivers/regulator/tps6287x-regulator.c
new file mode 100644
index 000000000000..644549b6856e
--- /dev/null
+++ b/drivers/regulator/tps6287x-regulator.c
@@ -0,0 +1,289 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (C) 2023 Axis Communications AB
+ *
+ * Driver for Texas Instruments TPS6287x PMIC.
+ * Datasheet: https://www.ti.com/lit/ds/symlink/tps62873.pdf
+ */
+
+#include <linux/err.h>
+#include <linux/i2c.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/regmap.h>
+#include <linux/regulator/of_regulator.h>
+#include <linux/regulator/machine.h>
+#include <linux/regulator/driver.h>
+#include <linux/bitfield.h>
+
+#define TPS6287X_VSET		0x00
+#define TPS6287X_CTRL1		0x01
+#define TPS6287X_CTRL1_VRAMP	GENMASK(1, 0)
+#define TPS6287X_CTRL1_FPWMEN	BIT(4)
+#define TPS6287X_CTRL1_SWEN	BIT(5)
+#define TPS6287X_CTRL2		0x02
+#define TPS6287X_CTRL2_VRANGE	GENMASK(3, 2)
+#define TPS6287X_CTRL3		0x03
+#define TPS6287X_STATUS		0x04
+
+#define TPS6287X_VRANGE_DFLT	2
+#define TPS6287X_MIN_UV_DFLT	400000
+#define TPS6287X_MAX_UV_DFLT	1675000
+#define TPS6287X_STEP_UV_DFLT	5000
+
+struct tps6287x_chip {
+	struct regulator_dev *rdev;
+	struct regmap *regmap;
+	unsigned int uv_step;
+};
+
+static const struct regmap_config tps6287x_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+	.max_register = TPS6287X_STATUS,
+};
+
+/* min_uv, max_uv, uv_step */
+static const unsigned int tps6287x_voltage_table[][3] = {
+	{ 400000, 718750, 1250 },
+	{ 400000, 1037500, 2500 },
+	{ 400000, 1675000, 5000 },
+	{ 800000, 3350000, 10000 },
+};
+
+static const unsigned int tps6287x_ramp_table[] = {
+	10000, 5000, 1250, 500
+};
+
+static int tps6287x_get_voltage(struct regulator_dev *rdev)
+{
+	struct device *dev = rdev_get_dev(rdev);
+	struct tps6287x_chip *chip =
+	    i2c_get_clientdata(to_i2c_client(dev->parent));
+	unsigned int val;
+	int ret;
+
+	ret = regmap_read(rdev->regmap, TPS6287X_VSET, &val);
+	if (ret != 0)
+		return -ENOTRECOVERABLE;
+
+	return (val * chip->uv_step) + rdev->constraints->min_uV;
+}
+
+static int tps6287x_set_voltage(struct regulator_dev *rdev, int min_uv,
+				int max_uv, unsigned int *selector)
+{
+	struct device *dev = rdev_get_dev(rdev);
+	struct tps6287x_chip *chip =
+	    i2c_get_clientdata(to_i2c_client(dev->parent));
+	int val = min_uv;
+
+	*selector = 0;
+
+	if (val < rdev->constraints->min_uV)
+		val = rdev->constraints->min_uV;
+	if (val > rdev->constraints->max_uV)
+		val = rdev->constraints->max_uV;
+
+	val -= rdev->constraints->min_uV;
+	val = DIV_ROUND_CLOSEST(val, chip->uv_step);
+
+	return regmap_write(rdev->regmap, TPS6287X_VSET, val);
+}
+
+static int tps6287x_set_mode(struct regulator_dev *rdev, unsigned int mode)
+{
+	unsigned int val;
+
+	switch (mode) {
+	case REGULATOR_MODE_NORMAL:
+		val = 0;
+		break;
+	case REGULATOR_MODE_FAST:
+		val = TPS6287X_CTRL1_FPWMEN;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return regmap_update_bits(rdev->regmap, TPS6287X_CTRL1,
+				  TPS6287X_CTRL1_FPWMEN, val);
+}
+
+static unsigned int tps6287x_get_mode(struct regulator_dev *rdev)
+{
+	unsigned int val;
+	int ret;
+
+	ret = regmap_read(rdev->regmap, TPS6287X_CTRL1, &val);
+	if (ret < 0)
+		return 0;
+
+	return (val & TPS6287X_CTRL1_FPWMEN) ? REGULATOR_MODE_FAST :
+	    REGULATOR_MODE_NORMAL;
+}
+
+static unsigned int tps6287x_of_map_mode(unsigned int mode)
+{
+	switch (mode) {
+	case REGULATOR_MODE_NORMAL:
+	case REGULATOR_MODE_FAST:
+		return mode;
+	default:
+		return REGULATOR_MODE_INVALID;
+	}
+}
+
+static const struct regulator_ops tps6287x_regulator_ops = {
+	.enable = regulator_enable_regmap,
+	.disable = regulator_disable_regmap,
+	.set_mode = tps6287x_set_mode,
+	.get_mode = tps6287x_get_mode,
+	.is_enabled = regulator_is_enabled_regmap,
+	.get_voltage = tps6287x_get_voltage,
+	.set_voltage = tps6287x_set_voltage,
+	.set_ramp_delay = regulator_set_ramp_delay_regmap,
+};
+
+static struct regulator_desc tps6287x_reg = {
+	.name = "tps6287x",
+	.of_match = of_match_ptr("vout"),
+	.owner = THIS_MODULE,
+	.ops = &tps6287x_regulator_ops,
+	.of_map_mode = tps6287x_of_map_mode,
+	.regulators_node = of_match_ptr("regulators"),
+	.type = REGULATOR_VOLTAGE,
+	.enable_reg = TPS6287X_CTRL1,
+	.enable_mask = TPS6287X_CTRL1_SWEN,
+	.ramp_reg = TPS6287X_CTRL1,
+	.ramp_mask = TPS6287X_CTRL1_VRAMP,
+	.ramp_delay_table = tps6287x_ramp_table,
+	.n_ramp_values = ARRAY_SIZE(tps6287x_ramp_table),
+};
+
+static int tps6287x_setup_vrange(struct tps6287x_chip *chip)
+{
+	struct regulator_dev *rdev = chip->rdev;
+	unsigned int val, r;
+	bool found = false;
+	int ret;
+
+	/*
+	 * Match DT voltage range to one of the predefined ranges,
+	 * and configure the regulator with the selected range.
+	 */
+	for (r = 0; r < ARRAY_SIZE(tps6287x_voltage_table); r++) {
+		if (tps6287x_voltage_table[r][0] == rdev->constraints->min_uV &&
+		    tps6287x_voltage_table[r][1] == rdev->constraints->max_uV) {
+			found = true;
+			break;
+		}
+	}
+
+	if (!found) {
+		dev_warn(rdev_get_dev(rdev)->parent, "Missing or invalid voltage range. Using default.\n");
+		rdev->constraints->min_uV = TPS6287X_MIN_UV_DFLT;
+		rdev->constraints->max_uV = TPS6287X_MAX_UV_DFLT;
+		return 0;
+	}
+
+	if (r == TPS6287X_VRANGE_DFLT)
+		return 0;
+
+	ret = regmap_update_bits(chip->regmap, TPS6287X_CTRL2,
+				 TPS6287X_CTRL2_VRANGE,
+				 FIELD_PREP(TPS6287X_CTRL2_VRANGE, r));
+	if (ret < 0)
+		return ret;
+
+	chip->uv_step = tps6287x_voltage_table[r][2];
+
+	/*
+	 * When voltage range is changed the setpoint register must be cleared
+	 * and rewritten to make the regulator start using the new range.
+	 */
+	ret = regmap_read(rdev->regmap, TPS6287X_VSET, &val);
+	if (ret != 0)
+		return ret;
+
+	ret = regmap_write(rdev->regmap, TPS6287X_VSET, 0);
+	if (ret != 0)
+		return ret;
+
+	return regmap_write(rdev->regmap, TPS6287X_VSET, val);
+}
+
+static int tps6287x_i2c_probe(struct i2c_client *i2c)
+{
+	struct device *dev = &i2c->dev;
+	struct regulator_config config = {};
+	struct tps6287x_chip *chip;
+	int ret;
+
+	chip = devm_kzalloc(dev, sizeof(*chip), GFP_KERNEL);
+	if (!chip)
+		return -ENOMEM;
+
+	chip->uv_step = TPS6287X_STEP_UV_DFLT;
+
+	chip->regmap = devm_regmap_init_i2c(i2c, &tps6287x_regmap_config);
+	if (IS_ERR(chip->regmap)) {
+		dev_err(dev, "Failed to init i2c\n");
+		return PTR_ERR(chip->regmap);
+	}
+
+	i2c_set_clientdata(i2c, chip);
+
+	config.dev = dev;
+	config.of_node = dev->of_node;
+	config.regmap = chip->regmap;
+
+	chip->rdev = devm_regulator_register(dev, &tps6287x_reg, &config);
+	if (IS_ERR(chip->rdev)) {
+		dev_err(dev, "Failed to register regulator\n");
+		return PTR_ERR(chip->rdev);
+	}
+
+	ret = tps6287x_setup_vrange(chip);
+	if (ret)
+		return ret;
+
+	dev_dbg(dev, "Probed regulator\n");
+
+	return 0;
+}
+
+static const struct of_device_id tps6287x_dt_ids[] = {
+	{ .compatible = "ti,tps62870", },
+	{ .compatible = "ti,tps62871", },
+	{ .compatible = "ti,tps62872", },
+	{ .compatible = "ti,tps62873", },
+	{ }
+};
+
+MODULE_DEVICE_TABLE(of, tps6287x_dt_ids);
+
+static const struct i2c_device_id tps6287x_i2c_id[] = {
+	{ "tps62870", 0 },
+	{ "tps62871", 0 },
+	{ "tps62872", 0 },
+	{ "tps62873", 0 },
+	{},
+};
+
+MODULE_DEVICE_TABLE(i2c, tps6287x_i2c_id);
+
+static struct i2c_driver tps6287x_regulator_driver = {
+	.driver = {
+		.name = "tps6287x",
+		.of_match_table = tps6287x_dt_ids,
+	},
+	.probe_new = tps6287x_i2c_probe,
+	.id_table = tps6287x_i2c_id,
+};
+
+module_i2c_driver(tps6287x_regulator_driver);
+
+MODULE_AUTHOR("Mårten Lindahl <marten.lindahl@axis.com>");
+MODULE_DESCRIPTION("Regulator driver for TI TPS6287X PMIC");
+MODULE_LICENSE("GPL");

-- 
2.30.2

Re: [PATCH v2 2/2] regulator: Add support for TI TPS6287x regulators

[Mark Brown]

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

On Thu, May 04, 2023 at 10:30:27AM +0200, Mårten Lindahl wrote:

> +static int tps6287x_get_voltage(struct regulator_dev *rdev)
> +{
> +	struct device *dev = rdev_get_dev(rdev);
> +	struct tps6287x_chip *chip =
> +	    i2c_get_clientdata(to_i2c_client(dev->parent));
> +	unsigned int val;
> +	int ret;
> +
> +	ret = regmap_read(rdev->regmap, TPS6287X_VSET, &val);
> +	if (ret != 0)
> +		return -ENOTRECOVERABLE;
> +
> +	return (val * chip->uv_step) + rdev->constraints->min_uV;
> +}

Don't open code the voltage conversion, just use selectors - in which
case you can simply describe the bitfield that the device has and use
the generic regmap helpers.

The driver should also never be referring to constraints to figure out
what the register values mean, this is just not going to work - boards
will typically be able to use far fewer voltages than the regulator
supports.

Also try to avoid squashing error codes, just pass the result back.

> +static int tps6287x_set_voltage(struct regulator_dev *rdev, int min_uv,
> +				int max_uv, unsigned int *selector)

Similarly here, describe the bitfield and use the generic helpers.

> +static int tps6287x_setup_vrange(struct tps6287x_chip *chip)
> +{
> +	struct regulator_dev *rdev = chip->rdev;
> +	unsigned int val, r;
> +	bool found = false;
> +	int ret;
> +
> +	/*
> +	 * Match DT voltage range to one of the predefined ranges,
> +	 * and configure the regulator with the selected range.
> +	 */
> +	for (r = 0; r < ARRAY_SIZE(tps6287x_voltage_table); r++) {
> +		if (tps6287x_voltage_table[r][0] == rdev->constraints->min_uV &&
> +		    tps6287x_voltage_table[r][1] == rdev->constraints->max_uV) {
> +			found = true;
> +			break;
> +		}
> +	}

No, as I said above the driver should just know what the device
supports based on the device ID.  In general if a regulator driver is
looking at the constraints that indicates that it's doing something
wrong, the purpose of constraints is to grant permission for the
features of the regulator to be used on the board.

> +static const struct of_device_id tps6287x_dt_ids[] = {
> +	{ .compatible = "ti,tps62870", },
> +	{ .compatible = "ti,tps62871", },
> +	{ .compatible = "ti,tps62872", },
> +	{ .compatible = "ti,tps62873", },
> +	{ }
> +};

Use the .data field here...

> +static const struct i2c_device_id tps6287x_i2c_id[] = {
> +	{ "tps62870", 0 },
> +	{ "tps62871", 0 },
> +	{ "tps62872", 0 },
> +	{ "tps62873", 0 },
> +	{},
> +};

...and here to enumerate which of the variants is being used and hence
which voltage range is required.

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

Re: [PATCH v2 2/2] regulator: Add support for TI TPS6287x regulators

[Mårten Lindahl]

Hi Mark!

On 5/4/23 14:04, Mark Brown wrote:
> On Thu, May 04, 2023 at 10:30:27AM +0200, Mårten Lindahl wrote:
>
>> +static int tps6287x_get_voltage(struct regulator_dev *rdev)
>> +{
>> +	struct device *dev = rdev_get_dev(rdev);
>> +	struct tps6287x_chip *chip =
>> +	    i2c_get_clientdata(to_i2c_client(dev->parent));
>> +	unsigned int val;
>> +	int ret;
>> +
>> +	ret = regmap_read(rdev->regmap, TPS6287X_VSET, &val);
>> +	if (ret != 0)
>> +		return -ENOTRECOVERABLE;
>> +
>> +	return (val * chip->uv_step) + rdev->constraints->min_uV;
>> +}
> Don't open code the voltage conversion, just use selectors - in which
> case you can simply describe the bitfield that the device has and use
> the generic regmap helpers.
>
> The driver should also never be referring to constraints to figure out
> what the register values mean, this is just not going to work - boards
> will typically be able to use far fewer voltages than the regulator
> supports.
>
> Also try to avoid squashing error codes, just pass the result back.
>
>> +static int tps6287x_set_voltage(struct regulator_dev *rdev, int min_uv,
>> +				int max_uv, unsigned int *selector)
> Similarly here, describe the bitfield and use the generic helpers.
I understand. I'll change it. Explanation below why I did it like this.
>
>> +static int tps6287x_setup_vrange(struct tps6287x_chip *chip)
>> +{
>> +	struct regulator_dev *rdev = chip->rdev;
>> +	unsigned int val, r;
>> +	bool found = false;
>> +	int ret;
>> +
>> +	/*
>> +	 * Match DT voltage range to one of the predefined ranges,
>> +	 * and configure the regulator with the selected range.
>> +	 */
>> +	for (r = 0; r < ARRAY_SIZE(tps6287x_voltage_table); r++) {
>> +		if (tps6287x_voltage_table[r][0] == rdev->constraints->min_uV &&
>> +		    tps6287x_voltage_table[r][1] == rdev->constraints->max_uV) {
>> +			found = true;
>> +			break;
>> +		}
>> +	}
> No, as I said above the driver should just know what the device
> supports based on the device ID.  In general if a regulator driver is
> looking at the constraints that indicates that it's doing something
> wrong, the purpose of constraints is to grant permission for the
> features of the regulator to be used on the board.

The reason for doing like this is that all 4 device IDs support all 4 
voltage ranges:

   0.4-V to 0.71875-V in 1.25-mV steps
   0.4-V to 1.0375-V in 2.5-mV steps
   0.4-V to 1.675-V in 5-mV steps
   0.8-V to 3.35-V in 10-mV steps

Of which the third is default for all devices. The range is solely 
selected by a register
field (no hardware pin connection), so I can't associate a specific 
device ID with a
specific voltage range, which is why I let the DT properties min/max 
decide the range.

But I see now it should be done in another way. I can think of 2 ways to 
implement it:

1. Introduce a DT property for this driver, like "ti,vrange-selector" 
and select one of
     the 4 voltage ranges by desc->of_parse_cb. This way allows a new 
voltage only to be set
     if it is within the selected range.

2. Dynamically set the range when a new voltage is set. This way any 
voltage from
     0.4V to 3.35V could be set if the DT node has:
         regulator-min-microvolt = <400000>;
         regulator-max-microvolt = <3350000>;

I hope I was clear enough with my reasoning. Maybe there are better ways 
to do it?

Kind regards

Mårten

>
>> +static const struct of_device_id tps6287x_dt_ids[] = {
>> +	{ .compatible = "ti,tps62870", },
>> +	{ .compatible = "ti,tps62871", },
>> +	{ .compatible = "ti,tps62872", },
>> +	{ .compatible = "ti,tps62873", },
>> +	{ }
>> +};
> Use the .data field here...
>
>> +static const struct i2c_device_id tps6287x_i2c_id[] = {
>> +	{ "tps62870", 0 },
>> +	{ "tps62871", 0 },
>> +	{ "tps62872", 0 },
>> +	{ "tps62873", 0 },
>> +	{},
>> +};
> ...and here to enumerate which of the variants is being used and hence
> which voltage range is required.

Re: [PATCH v2 2/2] regulator: Add support for TI TPS6287x regulators

[Mark Brown]

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

On Fri, May 05, 2023 at 08:46:06AM +0200, Mårten Lindahl wrote:

> 2. Dynamically set the range when a new voltage is set. This way any
>    voltage from
>    0.4V to 3.35V could be set if the DT node has:
>    regulator-min-microvolt = <400000>;
>    regulator-max-microvolt = <3350000>;

You can probably use one of the linear range mappings with a custom
set/get which munges the range selection and voltage selection into a
single value.

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

Re: [PATCH v2 2/2] regulator: Add support for TI TPS6287x regulators

[Mårten Lindahl]

On 5/5/23 13:52, Mark Brown wrote:
> On Fri, May 05, 2023 at 08:46:06AM +0200, Mårten Lindahl wrote:
>
>> 2. Dynamically set the range when a new voltage is set. This way any
>>     voltage from
>>     0.4V to 3.35V could be set if the DT node has:
>>     regulator-min-microvolt = <400000>;
>>     regulator-max-microvolt = <3350000>;
> You can probably use one of the linear range mappings with a custom
> set/get which munges the range selection and voltage selection into a
> single value.

Hi Mark!

I found the *sel_pickable_regmap helpers which seems to do what I need 
for this.

Thanks!

Kind regards

Mårten