[PATCH v2 0/7] Fix issues on pf8x00 driver

[PATCH v2 0/7] Fix issues on pf8x00 driver

[Adrien Grassein]

Hello,

this patch set aims is to fix some issues contains in the pf8x00 driver.
Some issues are documentation ones, some others are in code.

These issues where found while developing another version of the same
driver. I prefer to share with you the patch for it.

I do these patches on the master branch of the linux-next repository.

1: regulator_name property is useless in documentation.

2: there is a missing documentation when we build the driver as a
module.

3: nxp,phase-shift documentation was not optimal.

4: nxp,ilim-ma is useless since another property does (nearly) the same.

5: Same than 4, in driver code.

6: It's better to use a linear_range to describe the voltage of bucks.

7: Fix several mistakes in the handle of nxp,phase-shift property.

Changes introduced in v2:
Some patches were mixed (and did not compile separatly);
Add path 7 for the handle of nxp,phase-shift property.


Adrien Grassein (7):
  regulator: dt-bindings: remove useless properties
  regulator: pf8x00: add a doc for the module
  regulator: dt-bindings: pf8x00: fix nxp,phase-shift doc
  regulator: dt-bindings: pf8x00: remove nxp,ilim-ma property
  regulator: pf8x00: remove nxp,ilim-ma property
  regulator: pf8x00: use linear range for buck 1-6
  regulator: pf8x00: fix nxp,phase-shift

 .../regulator/nxp,pf8x00-regulator.yaml       |  46 +----
 drivers/regulator/Kconfig                     |   4 +
 drivers/regulator/pf8x00-regulator.c          | 174 ++++++++----------
 3 files changed, 81 insertions(+), 143 deletions(-)

-- 
2.20.1

[PATCH v2 1/7] regulator: dt-bindings: remove useless properties

[Adrien Grassein]

regulator-name is a generic property of the regulator.
Don't repeat it here.

Signed-off-by: Adrien Grassein <adrien.grassein@gmail.com>
---
 .../regulator/nxp,pf8x00-regulator.yaml         | 17 -----------------
 1 file changed, 17 deletions(-)

diff --git a/Documentation/devicetree/bindings/regulator/nxp,pf8x00-regulator.yaml b/Documentation/devicetree/bindings/regulator/nxp,pf8x00-regulator.yaml
index a6c259ce9785..913532d0532e 100644
--- a/Documentation/devicetree/bindings/regulator/nxp,pf8x00-regulator.yaml
+++ b/Documentation/devicetree/bindings/regulator/nxp,pf8x00-regulator.yaml
@@ -36,12 +36,6 @@ properties:
         description:
           Properties for single LDO regulator.
 
-        properties:
-          regulator-name:
-            pattern: "^ldo[1-4]$"
-            description:
-              should be "ldo1", ..., "ldo4"
-
         unevaluatedProperties: false
 
       "^buck[1-7]$":
@@ -51,11 +45,6 @@ properties:
           Properties for single BUCK regulator.
 
         properties:
-          regulator-name:
-            pattern: "^buck[1-7]$"
-            description:
-              should be "buck1", ..., "buck7"
-
           nxp,ilim-ma:
             $ref: "/schemas/types.yaml#/definitions/uint32"
             minimum: 2100
@@ -94,12 +83,6 @@ properties:
         description:
           Properties for single VSNVS regulator.
 
-        properties:
-          regulator-name:
-            pattern: "^vsnvs$"
-            description:
-              should be "vsnvs"
-
         unevaluatedProperties: false
 
     additionalProperties: false
-- 
2.20.1

[PATCH v2 2/7] regulator: pf8x00: add a doc for the module

[Adrien Grassein]

pf8x00 module build was not documented.

Signed-off-by: Adrien Grassein <adrien.grassein@gmail.com>
---
 drivers/regulator/Kconfig | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
index 53fa84f4d1e1..9c6508e99fdd 100644
--- a/drivers/regulator/Kconfig
+++ b/drivers/regulator/Kconfig
@@ -828,6 +828,10 @@ config REGULATOR_PF8X00
 	  Say y here to support the regulators found on the NXP
 	  PF8100/PF8121A/PF8200 PMIC.
 
+	  Say M here if you want to support for the regulators found
+	  on the NXP PF8100/PF8121A/PF8200 PMIC. The module will be named
+	  "pf8x00-regulator".
+
 config REGULATOR_PFUZE100
 	tristate "Freescale PFUZE100/200/3000/3001 regulator driver"
 	depends on I2C && OF
-- 
2.20.1

[PATCH v2 3/7] regulator: dt-bindings: pf8x00: fix nxp,phase-shift doc

[Adrien Grassein]

nxp,phase-shift is an enum so use enum format to describe it.
Minimum and maximum values are also wrong.

Signed-off-by: Adrien Grassein <adrien.grassein@gmail.com>
---
 .../bindings/regulator/nxp,pf8x00-regulator.yaml | 16 ++++------------
 1 file changed, 4 insertions(+), 12 deletions(-)

diff --git a/Documentation/devicetree/bindings/regulator/nxp,pf8x00-regulator.yaml b/Documentation/devicetree/bindings/regulator/nxp,pf8x00-regulator.yaml
index 913532d0532e..1da724c6e2ba 100644
--- a/Documentation/devicetree/bindings/regulator/nxp,pf8x00-regulator.yaml
+++ b/Documentation/devicetree/bindings/regulator/nxp,pf8x00-regulator.yaml
@@ -60,21 +60,13 @@ properties:
 
           nxp,phase-shift:
             $ref: "/schemas/types.yaml#/definitions/uint32"
-            minimum: 45
-            maximum: 0
+            minimum: 0
+            maximum: 315
+            default: 0
+            enum: [ 0, 45, 90, 135, 180, 225, 270, 315 ]
             description:
               BUCK regulators phase shift control in degrees.
 
-              Listed phase shift control values in degrees are,
-              45
-              90
-              135
-              180
-              225
-              270
-              315
-              0 (default)
-
         unevaluatedProperties: false
 
       "^vsnvs$":
-- 
2.20.1

[PATCH v2 4/7] regulator: dt-bindings: pf8x00: remove nxp,ilim-ma property

[Adrien Grassein]

This property seems useless because we can use the
regulator-max-microamp generic property to do the same
and using generic code.

The only things it changes is the default value. The original
code was using "2100" (mA) as default one, but I think we should
keep the value in the OTP memory as the default one. This value
is automatically loaded in the register by the chip.

Signed-off-by: Adrien Grassein <adrien.grassein@gmail.com>
---
 .../bindings/regulator/nxp,pf8x00-regulator.yaml    | 13 -------------
 1 file changed, 13 deletions(-)

diff --git a/Documentation/devicetree/bindings/regulator/nxp,pf8x00-regulator.yaml b/Documentation/devicetree/bindings/regulator/nxp,pf8x00-regulator.yaml
index 1da724c6e2ba..b6ea3b8eb811 100644
--- a/Documentation/devicetree/bindings/regulator/nxp,pf8x00-regulator.yaml
+++ b/Documentation/devicetree/bindings/regulator/nxp,pf8x00-regulator.yaml
@@ -45,19 +45,6 @@ properties:
           Properties for single BUCK regulator.
 
         properties:
-          nxp,ilim-ma:
-            $ref: "/schemas/types.yaml#/definitions/uint32"
-            minimum: 2100
-            maximum: 4500
-            description:
-              BUCK regulators current limit in mA.
-
-              Listed current limits in mA are,
-              2100 (default)
-              2600
-              3000
-              4500
-
           nxp,phase-shift:
             $ref: "/schemas/types.yaml#/definitions/uint32"
             minimum: 0
-- 
2.20.1

[PATCH v2 5/7] regulator: pf8x00: remove nxp,ilim-ma property

[Adrien Grassein]

This property seems useless because we can use the
regulator-max-microamp generic property to do the same
and using generic code.

The only things it changes is the default value. The original
code was using "2100" (mA) as default one, but I think we should
keep the value in the OTP memory as the default one. This value
is automatically loaded in the register by the chip.

Signed-off-by: Adrien Grassein <adrien.grassein@gmail.com>
---
 drivers/regulator/pf8x00-regulator.c | 95 ++++++++++++++--------------
 1 file changed, 47 insertions(+), 48 deletions(-)

diff --git a/drivers/regulator/pf8x00-regulator.c b/drivers/regulator/pf8x00-regulator.c
index 308c27fa6ea8..ebb0ef0745f1 100644
--- a/drivers/regulator/pf8x00-regulator.c
+++ b/drivers/regulator/pf8x00-regulator.c
@@ -105,13 +105,6 @@ enum pf8x00_ldo_states {
 };
 #define PF8X00_LDO_BASE(i)		(6 * (i - PF8X00_LDO1) + PF8X00_LDO1_CONFIG1)
 
-enum swxilim_bits {
-	SWXILIM_2100_MA,
-	SWXILIM_2600_MA,
-	SWXILIM_3000_MA,
-	SWXILIM_4500_MA,
-};
-#define PF8X00_SWXILIM_SHIFT		3
 #define PF8X00_SWXILIM_MASK		GENMASK(4, 3)
 #define PF8X00_SWXPHASE_MASK		GENMASK(2, 0)
 #define PF8X00_SWXPHASE_DEFAULT		0
@@ -128,7 +121,6 @@ enum pf8x00_devid {
 
 struct pf8x00_regulator {
 	struct regulator_desc desc;
-	u8 ilim;
 	u8 phase_shift;
 };
 
@@ -150,6 +142,11 @@ static const int pf8x00_ldo_voltages[] = {
 	3100000, 3150000, 3200000, 3300000, 3350000, 1650000, 1700000, 5000000,
 };
 
+/* Output: 2.1A to 4.5A */
+static const unsigned int pf8x00_sw_current_table[] = {
+	2100000, 2600000, 3000000, 4500000,
+};
+
 #define SWV(i)		(6250 * i + 400000)
 #define SWV_LINE(i)	SWV(i*8+0), SWV(i*8+1), SWV(i*8+2), SWV(i*8+3), \
 			SWV(i*8+4), SWV(i*8+5), SWV(i*8+6), SWV(i*8+7)
@@ -199,32 +196,6 @@ static struct pf8x00_regulator *desc_to_regulator(const struct regulator_desc *d
 	return container_of(desc, struct pf8x00_regulator, desc);
 }
 
-static void swxilim_select(const struct regulator_desc *desc, int ilim)
-{
-	struct pf8x00_regulator *data = desc_to_regulator(desc);
-	u8 ilim_sel;
-
-	switch (ilim) {
-	case 2100:
-		ilim_sel = SWXILIM_2100_MA;
-		break;
-	case 2600:
-		ilim_sel = SWXILIM_2600_MA;
-		break;
-	case 3000:
-		ilim_sel = SWXILIM_3000_MA;
-		break;
-	case 4500:
-		ilim_sel = SWXILIM_4500_MA;
-		break;
-	default:
-		ilim_sel = SWXILIM_2100_MA;
-		break;
-	}
-
-	data->ilim = ilim_sel;
-}
-
 static int pf8x00_of_parse_cb(struct device_node *np,
 			      const struct regulator_desc *desc,
 			      struct regulator_config *config)
@@ -235,13 +206,6 @@ static int pf8x00_of_parse_cb(struct device_node *np,
 	int val;
 	int ret;
 
-	ret = of_property_read_u32(np, "nxp,ilim-ma", &val);
-	if (ret)
-		dev_dbg(chip->dev, "unspecified ilim for BUCK%d, use 2100 mA\n",
-			desc->id - PF8X00_LDO4);
-
-	swxilim_select(desc, val);
-
 	ret = of_property_read_u32(np, "nxp,phase-shift", &val);
 	if (ret) {
 		dev_dbg(chip->dev,
@@ -272,7 +236,18 @@ static const struct regulator_ops pf8x00_ldo_ops = {
 	.get_voltage_sel = regulator_get_voltage_sel_regmap,
 };
 
-static const struct regulator_ops pf8x00_buck_ops = {
+static const struct regulator_ops pf8x00_buck1_6_ops = {
+	.enable = regulator_enable_regmap,
+	.disable = regulator_disable_regmap,
+	.is_enabled = regulator_is_enabled_regmap,
+	.list_voltage = regulator_list_voltage_table,
+	.set_voltage_sel = regulator_set_voltage_sel_regmap,
+	.get_voltage_sel = regulator_get_voltage_sel_regmap,
+	.get_current_limit = regulator_get_current_limit_regmap,
+	.set_current_limit = regulator_set_current_limit_regmap,
+};
+
+static const struct regulator_ops pf8x00_buck7_ops = {
 	.enable = regulator_enable_regmap,
 	.disable = regulator_disable_regmap,
 	.is_enabled = regulator_is_enabled_regmap,
@@ -320,13 +295,41 @@ static const struct regulator_ops pf8x00_vsnvs_ops = {
 			.regulators_node = "regulators",	\
 			.of_parse_cb = pf8x00_of_parse_cb,	\
 			.n_voltages = ARRAY_SIZE(voltages),	\
-			.ops = &pf8x00_buck_ops,		\
+			.ops = &pf8x00_buck1_6_ops,		\
 			.type = REGULATOR_VOLTAGE,		\
 			.id = PF8X00_BUCK ## _id,		\
 			.owner = THIS_MODULE,			\
 			.volt_table = voltages,			\
 			.vsel_reg = (base) + SW_RUN_VOLT,	\
 			.vsel_mask = 0xff,			\
+			.curr_table = pf8x00_sw_current_table, \
+			.n_current_limits = \
+				ARRAY_SIZE(pf8x00_sw_current_table), \
+			.csel_reg = (base) + SW_CONFIG2,	\
+			.csel_mask = PF8X00_SWXILIM_MASK,	\
+			.enable_reg = (base) + SW_MODE1,	\
+			.enable_val = 0x3,			\
+			.disable_val = 0x0,			\
+			.enable_mask = 0x3,			\
+			.enable_time = 500,			\
+		},						\
+	}
+
+#define PF8X00BUCK7(_name, base, voltages)			\
+	[PF8X00_BUCK7] = {				\
+		.desc = {					\
+			.name = _name,				\
+			.of_match = _name,			\
+			.regulators_node = "regulators",	\
+			.of_parse_cb = pf8x00_of_parse_cb,	\
+			.n_voltages = ARRAY_SIZE(voltages),	\
+			.ops = &pf8x00_buck7_ops,		\
+			.type = REGULATOR_VOLTAGE,		\
+			.id = PF8X00_BUCK7,		\
+			.owner = THIS_MODULE,			\
+			.volt_table = voltages,			\
+			.vsel_reg = (base) + SW_RUN_VOLT,	\
+			.vsel_mask = 0xff,			\
 			.enable_reg = (base) + SW_MODE1,	\
 			.enable_val = 0x3,			\
 			.disable_val = 0x0,			\
@@ -363,7 +366,7 @@ static struct pf8x00_regulator pf8x00_regulators_data[PF8X00_MAX_REGULATORS] = {
 	PF8X00BUCK(4, "buck4", PF8X00_SW_BASE(PF8X00_BUCK4), pf8x00_sw1_to_6_voltages),
 	PF8X00BUCK(5, "buck5", PF8X00_SW_BASE(PF8X00_BUCK5), pf8x00_sw1_to_6_voltages),
 	PF8X00BUCK(6, "buck6", PF8X00_SW_BASE(PF8X00_BUCK6), pf8x00_sw1_to_6_voltages),
-	PF8X00BUCK(7, "buck7", PF8X00_SW_BASE(PF8X00_BUCK7), pf8x00_sw7_voltages),
+	PF8X00BUCK7("buck7", PF8X00_SW_BASE(PF8X00_BUCK7), pf8x00_sw7_voltages),
 	PF8X00VSNVS("vsnvs", PF8X00_VSNVS_CONFIG1, pf8x00_vsnvs_voltages),
 };
 
@@ -458,10 +461,6 @@ static int pf8x00_i2c_probe(struct i2c_client *client)
 			regmap_update_bits(chip->regmap, reg,
 					   PF8X00_SWXPHASE_MASK,
 					   data->phase_shift);
-
-			regmap_update_bits(chip->regmap, reg,
-					   PF8X00_SWXILIM_MASK,
-					   data->ilim << PF8X00_SWXILIM_SHIFT);
 		}
 	}
 
-- 
2.20.1

[PATCH v2 6/7] regulator: pf8x00: use linear range for buck 1-6

[Adrien Grassein]

Use a linear range to describe the voltages of the
bucks 1-6 instead of listing it one by one (via a macro)

Signed-off-by: Adrien Grassein <adrien.grassein@gmail.com>
---
 drivers/regulator/pf8x00-regulator.c | 40 +++++++---------------------
 1 file changed, 9 insertions(+), 31 deletions(-)

diff --git a/drivers/regulator/pf8x00-regulator.c b/drivers/regulator/pf8x00-regulator.c
index ebb0ef0745f1..5ad940b3db0a 100644
--- a/drivers/regulator/pf8x00-regulator.c
+++ b/drivers/regulator/pf8x00-regulator.c
@@ -147,35 +147,11 @@ static const unsigned int pf8x00_sw_current_table[] = {
 	2100000, 2600000, 3000000, 4500000,
 };
 
-#define SWV(i)		(6250 * i + 400000)
-#define SWV_LINE(i)	SWV(i*8+0), SWV(i*8+1), SWV(i*8+2), SWV(i*8+3), \
-			SWV(i*8+4), SWV(i*8+5), SWV(i*8+6), SWV(i*8+7)
-
 /* Output: 0.4V to 1.8V */
-static const int pf8x00_sw1_to_6_voltages[] = {
-	SWV_LINE(0),
-	SWV_LINE(1),
-	SWV_LINE(2),
-	SWV_LINE(3),
-	SWV_LINE(4),
-	SWV_LINE(5),
-	SWV_LINE(6),
-	SWV_LINE(7),
-	SWV_LINE(8),
-	SWV_LINE(9),
-	SWV_LINE(10),
-	SWV_LINE(11),
-	SWV_LINE(12),
-	SWV_LINE(13),
-	SWV_LINE(14),
-	SWV_LINE(15),
-	SWV_LINE(16),
-	SWV_LINE(17),
-	SWV_LINE(18),
-	SWV_LINE(19),
-	SWV_LINE(20),
-	SWV_LINE(21),
-	1500000, 1800000,
+#define PF8XOO_SW1_6_VOLTAGE_NUM 0xB2
+static const struct linear_range pf8x00_sw1_to_6_voltages[] = {
+	REGULATOR_LINEAR_RANGE(400000, 0x00, 0xB0, 6250),
+	REGULATOR_LINEAR_RANGE(1800000, 0xB1, 0xB1, 0),
 };
 
 /* Output: 1.0V to 4.1V */
@@ -240,7 +216,7 @@ static const struct regulator_ops pf8x00_buck1_6_ops = {
 	.enable = regulator_enable_regmap,
 	.disable = regulator_disable_regmap,
 	.is_enabled = regulator_is_enabled_regmap,
-	.list_voltage = regulator_list_voltage_table,
+	.list_voltage = regulator_list_voltage_linear_range,
 	.set_voltage_sel = regulator_set_voltage_sel_regmap,
 	.get_voltage_sel = regulator_get_voltage_sel_regmap,
 	.get_current_limit = regulator_get_current_limit_regmap,
@@ -294,12 +270,14 @@ static const struct regulator_ops pf8x00_vsnvs_ops = {
 			.of_match = _name,			\
 			.regulators_node = "regulators",	\
 			.of_parse_cb = pf8x00_of_parse_cb,	\
-			.n_voltages = ARRAY_SIZE(voltages),	\
+			.n_voltages = PF8XOO_SW1_6_VOLTAGE_NUM,	\
 			.ops = &pf8x00_buck1_6_ops,		\
 			.type = REGULATOR_VOLTAGE,		\
 			.id = PF8X00_BUCK ## _id,		\
 			.owner = THIS_MODULE,			\
-			.volt_table = voltages,			\
+			.linear_ranges = pf8x00_sw1_to_6_voltages, \
+			.n_linear_ranges = \
+				ARRAY_SIZE(pf8x00_sw1_to_6_voltages), \
 			.vsel_reg = (base) + SW_RUN_VOLT,	\
 			.vsel_mask = 0xff,			\
 			.curr_table = pf8x00_sw_current_table, \
-- 
2.20.1

[PATCH v2 7/7] regulator: pf8x00: fix nxp,phase-shift

[Adrien Grassein]

Fix the ternary condition which is a bad coding style
in the kernel

I also remove the defering configuration of the nxp,phase-shift.
The configuration is now done at parsing time. It save some memory
and it's better for comprehension.

I also use the OTP default configuration when the paramater is wrong
or not specified.
I think that it's better to use the default configuration from the chip
than an arbitrary value.

Signed-off-by: Adrien Grassein <adrien.grassein@gmail.com>
---
 drivers/regulator/pf8x00-regulator.c | 45 +++++++++++++---------------
 1 file changed, 20 insertions(+), 25 deletions(-)

diff --git a/drivers/regulator/pf8x00-regulator.c b/drivers/regulator/pf8x00-regulator.c
index 5ad940b3db0a..b8b3ac393ee8 100644
--- a/drivers/regulator/pf8x00-regulator.c
+++ b/drivers/regulator/pf8x00-regulator.c
@@ -107,7 +107,6 @@ enum pf8x00_ldo_states {
 
 #define PF8X00_SWXILIM_MASK		GENMASK(4, 3)
 #define PF8X00_SWXPHASE_MASK		GENMASK(2, 0)
-#define PF8X00_SWXPHASE_DEFAULT		0
 #define PF8X00_SWXPHASE_SHIFT		7
 
 enum pf8x00_devid {
@@ -121,7 +120,6 @@ enum pf8x00_devid {
 
 struct pf8x00_regulator {
 	struct regulator_desc desc;
-	u8 phase_shift;
 };
 
 struct pf8x00_chip {
@@ -167,17 +165,13 @@ static const int pf8x00_vsnvs_voltages[] = {
 	0, 1800000, 3000000, 3300000,
 };
 
-static struct pf8x00_regulator *desc_to_regulator(const struct regulator_desc *desc)
-{
-	return container_of(desc, struct pf8x00_regulator, desc);
-}
-
 static int pf8x00_of_parse_cb(struct device_node *np,
 			      const struct regulator_desc *desc,
 			      struct regulator_config *config)
 {
-	struct pf8x00_regulator *data = desc_to_regulator(desc);
 	struct pf8x00_chip *chip = config->driver_data;
+	unsigned char id = desc->id - PF8X00_LDO4;
+	unsigned char reg = PF8X00_SW_BASE(id) + SW_CONFIG2;
 	int phase;
 	int val;
 	int ret;
@@ -185,21 +179,30 @@ static int pf8x00_of_parse_cb(struct device_node *np,
 	ret = of_property_read_u32(np, "nxp,phase-shift", &val);
 	if (ret) {
 		dev_dbg(chip->dev,
-			"unspecified phase-shift for BUCK%d, use 0 degrees\n",
-			desc->id - PF8X00_LDO4);
-		val = PF8X00_SWXPHASE_DEFAULT;
+			"unspecified phase-shift for BUCK%d, using OTP configuration\n",
+			id);
+		goto end;
 	}
 
-	phase = val / 45;
-	if ((phase * 45) != val) {
+	if (val < 0 || val > 315 || val % 45 != 0) {
 		dev_warn(config->dev,
-			 "invalid phase_shift %d for BUCK%d, use 0 degrees\n",
-			 (phase * 45), desc->id - PF8X00_LDO4);
-		phase = PF8X00_SWXPHASE_SHIFT;
+			 "invalid phase_shift %d for BUCK%d, using OTP configuration\n",
+			 val, id);
+		goto end;
 	}
 
-	data->phase_shift = (phase >= 1) ? phase - 1 : PF8X00_SWXPHASE_SHIFT;
+	phase = val / 45;
+
+	if (phase >= 1)
+		phase -= 1;
+	else
+		phase = PF8X00_SWXPHASE_SHIFT;
+
+	regmap_update_bits(chip->regmap, reg,
+			PF8X00_SWXPHASE_MASK,
+			phase);
 
+end:
 	return 0;
 }
 
@@ -432,14 +435,6 @@ static int pf8x00_i2c_probe(struct i2c_client *client)
 				"failed to register %s regulator\n", data->desc.name);
 			return PTR_ERR(rdev);
 		}
-
-		if ((id >= PF8X00_BUCK1) && (id <= PF8X00_BUCK7)) {
-			u8 reg = PF8X00_SW_BASE(id) + SW_CONFIG2;
-
-			regmap_update_bits(chip->regmap, reg,
-					   PF8X00_SWXPHASE_MASK,
-					   data->phase_shift);
-		}
 	}
 
 	return 0;
-- 
2.20.1

Re: [PATCH v2 1/7] regulator: dt-bindings: remove useless properties

[Jagan Teki]

On Wed, Dec 16, 2020 at 5:26 AM Adrien Grassein
<adrien.grassein@gmail.com> wrote:
>
> regulator-name is a generic property of the regulator.
> Don't repeat it here.
>
> Signed-off-by: Adrien Grassein <adrien.grassein@gmail.com>
> ---
>  .../regulator/nxp,pf8x00-regulator.yaml         | 17 -----------------
>  1 file changed, 17 deletions(-)

Please add pf8x00 in commit head, otherwise

Reviewed-by: Jagan Teki <jagan@amarulasolutions.com>

Re: [PATCH v2 2/7] regulator: pf8x00: add a doc for the module

[Jagan Teki]

On Wed, Dec 16, 2020 at 5:27 AM Adrien Grassein
<adrien.grassein@gmail.com> wrote:
>
> pf8x00 module build was not documented.
>
> Signed-off-by: Adrien Grassein <adrien.grassein@gmail.com>
> ---
>  drivers/regulator/Kconfig | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
> index 53fa84f4d1e1..9c6508e99fdd 100644
> --- a/drivers/regulator/Kconfig
> +++ b/drivers/regulator/Kconfig
> @@ -828,6 +828,10 @@ config REGULATOR_PF8X00
>           Say y here to support the regulators found on the NXP
>           PF8100/PF8121A/PF8200 PMIC.
>
> +         Say M here if you want to support for the regulators found
> +         on the NXP PF8100/PF8121A/PF8200 PMIC. The module will be named
> +         "pf8x00-regulator".

Reviewed-by: Jagan Teki <jagan@amarulasolutions.com>

Re: [PATCH v2 3/7] regulator: dt-bindings: pf8x00: fix nxp,phase-shift doc

[Jagan Teki]

On Wed, Dec 16, 2020 at 5:27 AM Adrien Grassein
<adrien.grassein@gmail.com> wrote:
>
> nxp,phase-shift is an enum so use enum format to describe it.
> Minimum and maximum values are also wrong.
>
> Signed-off-by: Adrien Grassein <adrien.grassein@gmail.com>
> ---
>  .../bindings/regulator/nxp,pf8x00-regulator.yaml | 16 ++++------------
>  1 file changed, 4 insertions(+), 12 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/regulator/nxp,pf8x00-regulator.yaml b/Documentation/devicetree/bindings/regulator/nxp,pf8x00-regulator.yaml
> index 913532d0532e..1da724c6e2ba 100644
> --- a/Documentation/devicetree/bindings/regulator/nxp,pf8x00-regulator.yaml
> +++ b/Documentation/devicetree/bindings/regulator/nxp,pf8x00-regulator.yaml
> @@ -60,21 +60,13 @@ properties:
>
>            nxp,phase-shift:
>              $ref: "/schemas/types.yaml#/definitions/uint32"
> -            minimum: 45
> -            maximum: 0
> +            minimum: 0
> +            maximum: 315
> +            default: 0
> +            enum: [ 0, 45, 90, 135, 180, 225, 270, 315 ]

On Wed, Dec 16, 2020 at 5:27 AM Adrien Grassein
<adrien.grassein@gmail.com> wrote:
>
> pf8x00 module build was not documented.
>
> Signed-off-by: Adrien Grassein <adrien.grassein@gmail.com>
> ---
>  drivers/regulator/Kconfig | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
> index 53fa84f4d1e1..9c6508e99fdd 100644
> --- a/drivers/regulator/Kconfig
> +++ b/drivers/regulator/Kconfig
> @@ -828,6 +828,10 @@ config REGULATOR_PF8X00
>           Say y here to support the regulators found on the NXP
>           PF8100/PF8121A/PF8200 PMIC.
>
> +         Say M here if you want to support for the regulators found
> +         on the NXP PF8100/PF8121A/PF8200 PMIC. The module will be named
> +         "pf8x00-regulator".

Reviewed-by: Jagan Teki <jagan@amarulasolutions.com>

Re: [PATCH v2 7/7] regulator: pf8x00: fix nxp,phase-shift

[Jagan Teki]

On Wed, Dec 16, 2020 at 5:27 AM Adrien Grassein
<adrien.grassein@gmail.com> wrote:
>
> Fix the ternary condition which is a bad coding style
> in the kernel
>
> I also remove the defering configuration of the nxp,phase-shift.
> The configuration is now done at parsing time. It save some memory
> and it's better for comprehension.
>
> I also use the OTP default configuration when the paramater is wrong
> or not specified.
> I think that it's better to use the default configuration from the chip
> than an arbitrary value.
>
> Signed-off-by: Adrien Grassein <adrien.grassein@gmail.com>
> ---
>  drivers/regulator/pf8x00-regulator.c | 45 +++++++++++++---------------
>  1 file changed, 20 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/regulator/pf8x00-regulator.c b/drivers/regulator/pf8x00-regulator.c
> index 5ad940b3db0a..b8b3ac393ee8 100644
> --- a/drivers/regulator/pf8x00-regulator.c
> +++ b/drivers/regulator/pf8x00-regulator.c
> @@ -107,7 +107,6 @@ enum pf8x00_ldo_states {
>
>  #define PF8X00_SWXILIM_MASK            GENMASK(4, 3)
>  #define PF8X00_SWXPHASE_MASK           GENMASK(2, 0)
> -#define PF8X00_SWXPHASE_DEFAULT                0

Keep this as it is and assign it 1 as the below code assigns 1.

>  #define PF8X00_SWXPHASE_SHIFT          7
>
>  enum pf8x00_devid {
> @@ -121,7 +120,6 @@ enum pf8x00_devid {
>
>  struct pf8x00_regulator {
>         struct regulator_desc desc;
> -       u8 phase_shift;
>  };
>
>  struct pf8x00_chip {
> @@ -167,17 +165,13 @@ static const int pf8x00_vsnvs_voltages[] = {
>         0, 1800000, 3000000, 3300000,
>  };
>
> -static struct pf8x00_regulator *desc_to_regulator(const struct regulator_desc *desc)
> -{
> -       return container_of(desc, struct pf8x00_regulator, desc);
> -}
> -
>  static int pf8x00_of_parse_cb(struct device_node *np,
>                               const struct regulator_desc *desc,
>                               struct regulator_config *config)
>  {
> -       struct pf8x00_regulator *data = desc_to_regulator(desc);
>         struct pf8x00_chip *chip = config->driver_data;
> +       unsigned char id = desc->id - PF8X00_LDO4;
> +       unsigned char reg = PF8X00_SW_BASE(id) + SW_CONFIG2;
>         int phase;
>         int val;
>         int ret;
> @@ -185,21 +179,30 @@ static int pf8x00_of_parse_cb(struct device_node *np,
>         ret = of_property_read_u32(np, "nxp,phase-shift", &val);
>         if (ret) {
>                 dev_dbg(chip->dev,
> -                       "unspecified phase-shift for BUCK%d, use 0 degrees\n",
> -                       desc->id - PF8X00_LDO4);
> -               val = PF8X00_SWXPHASE_DEFAULT;
> +                       "unspecified phase-shift for BUCK%d, using OTP configuration\n",
> +                       id);
> +               goto end;
>         }
>
> -       phase = val / 45;
> -       if ((phase * 45) != val) {
> +       if (val < 0 || val > 315 || val % 45 != 0) {
>                 dev_warn(config->dev,
> -                        "invalid phase_shift %d for BUCK%d, use 0 degrees\n",
> -                        (phase * 45), desc->id - PF8X00_LDO4);
> -               phase = PF8X00_SWXPHASE_SHIFT;
> +                        "invalid phase_shift %d for BUCK%d, using OTP configuration\n",
> +                        val, id);
> +               goto end;
>         }
>
> -       data->phase_shift = (phase >= 1) ? phase - 1 : PF8X00_SWXPHASE_SHIFT;
> +       phase = val / 45;
> +
> +       if (phase >= 1)
> +               phase -= 1;
> +       else
> +               phase = PF8X00_SWXPHASE_SHIFT;
> +
> +       regmap_update_bits(chip->regmap, reg,
> +                       PF8X00_SWXPHASE_MASK,
> +                       phase);

Add all these arguments in the same line.

Re: [PATCH v2 7/7] regulator: pf8x00: fix nxp,phase-shift

[Adrien Grassein]

Hello,

Thanks for your review.

Le jeu. 17 déc. 2020 à 18:42, Jagan Teki <jagan@amarulasolutions.com> a écrit :
>
> On Wed, Dec 16, 2020 at 5:27 AM Adrien Grassein
> <adrien.grassein@gmail.com> wrote:
> >
> > Fix the ternary condition which is a bad coding style
> > in the kernel
> >
> > I also remove the defering configuration of the nxp,phase-shift.
> > The configuration is now done at parsing time. It save some memory
> > and it's better for comprehension.
> >
> > I also use the OTP default configuration when the paramater is wrong
> > or not specified.
> > I think that it's better to use the default configuration from the chip
> > than an arbitrary value.
> >
> > Signed-off-by: Adrien Grassein <adrien.grassein@gmail.com>
> > ---
> >  drivers/regulator/pf8x00-regulator.c | 45 +++++++++++++---------------
> >  1 file changed, 20 insertions(+), 25 deletions(-)
> >
> > diff --git a/drivers/regulator/pf8x00-regulator.c b/drivers/regulator/pf8x00-regulator.c
> > index 5ad940b3db0a..b8b3ac393ee8 100644
> > --- a/drivers/regulator/pf8x00-regulator.c
> > +++ b/drivers/regulator/pf8x00-regulator.c
> > @@ -107,7 +107,6 @@ enum pf8x00_ldo_states {
> >
> >  #define PF8X00_SWXILIM_MASK            GENMASK(4, 3)
> >  #define PF8X00_SWXPHASE_MASK           GENMASK(2, 0)
> > -#define PF8X00_SWXPHASE_DEFAULT                0
>
> Keep this as it is and assign it 1 as the below code assigns 1.

I don't understand, PF8X00_SWXPHASE_DEFAULT is not used anymore in the code.
Why should I keep it ?
>
> >  #define PF8X00_SWXPHASE_SHIFT          7
> >
> >  enum pf8x00_devid {
> > @@ -121,7 +120,6 @@ enum pf8x00_devid {
> >
> >  struct pf8x00_regulator {
> >         struct regulator_desc desc;
> > -       u8 phase_shift;
> >  };
> >
> >  struct pf8x00_chip {
> > @@ -167,17 +165,13 @@ static const int pf8x00_vsnvs_voltages[] = {
> >         0, 1800000, 3000000, 3300000,
> >  };
> >
> > -static struct pf8x00_regulator *desc_to_regulator(const struct regulator_desc *desc)
> > -{
> > -       return container_of(desc, struct pf8x00_regulator, desc);
> > -}
> > -
> >  static int pf8x00_of_parse_cb(struct device_node *np,
> >                               const struct regulator_desc *desc,
> >                               struct regulator_config *config)
> >  {
> > -       struct pf8x00_regulator *data = desc_to_regulator(desc);
> >         struct pf8x00_chip *chip = config->driver_data;
> > +       unsigned char id = desc->id - PF8X00_LDO4;
> > +       unsigned char reg = PF8X00_SW_BASE(id) + SW_CONFIG2;
> >         int phase;
> >         int val;
> >         int ret;
> > @@ -185,21 +179,30 @@ static int pf8x00_of_parse_cb(struct device_node *np,
> >         ret = of_property_read_u32(np, "nxp,phase-shift", &val);
> >         if (ret) {
> >                 dev_dbg(chip->dev,
> > -                       "unspecified phase-shift for BUCK%d, use 0 degrees\n",
> > -                       desc->id - PF8X00_LDO4);
> > -               val = PF8X00_SWXPHASE_DEFAULT;
> > +                       "unspecified phase-shift for BUCK%d, using OTP configuration\n",
> > +                       id);
> > +               goto end;
> >         }
> >
> > -       phase = val / 45;
> > -       if ((phase * 45) != val) {
> > +       if (val < 0 || val > 315 || val % 45 != 0) {
> >                 dev_warn(config->dev,
> > -                        "invalid phase_shift %d for BUCK%d, use 0 degrees\n",
> > -                        (phase * 45), desc->id - PF8X00_LDO4);
> > -               phase = PF8X00_SWXPHASE_SHIFT;
> > +                        "invalid phase_shift %d for BUCK%d, using OTP configuration\n",
> > +                        val, id);
> > +               goto end;
> >         }
> >
> > -       data->phase_shift = (phase >= 1) ? phase - 1 : PF8X00_SWXPHASE_SHIFT;
> > +       phase = val / 45;
> > +
> > +       if (phase >= 1)
> > +               phase -= 1;
> > +       else
> > +               phase = PF8X00_SWXPHASE_SHIFT;
> > +
> > +       regmap_update_bits(chip->regmap, reg,
> > +                       PF8X00_SWXPHASE_MASK,
> > +                       phase);
>
> Add all these arguments in the same line.
OK.


Thanks again,

Adrien

Re: [PATCH v2 1/7] regulator: dt-bindings: remove useless properties

[Rob Herring]

On Wed, Dec 16, 2020 at 12:56:33AM +0100, Adrien Grassein wrote:
> regulator-name is a generic property of the regulator.
> Don't repeat it here.

Yes, but the values aren't generic.

> 
> Signed-off-by: Adrien Grassein <adrien.grassein@gmail.com>
> ---
>  .../regulator/nxp,pf8x00-regulator.yaml         | 17 -----------------
>  1 file changed, 17 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/regulator/nxp,pf8x00-regulator.yaml b/Documentation/devicetree/bindings/regulator/nxp,pf8x00-regulator.yaml
> index a6c259ce9785..913532d0532e 100644
> --- a/Documentation/devicetree/bindings/regulator/nxp,pf8x00-regulator.yaml
> +++ b/Documentation/devicetree/bindings/regulator/nxp,pf8x00-regulator.yaml
> @@ -36,12 +36,6 @@ properties:
>          description:
>            Properties for single LDO regulator.
>  
> -        properties:
> -          regulator-name:
> -            pattern: "^ldo[1-4]$"
> -            description:
> -              should be "ldo1", ..., "ldo4"
> -
>          unevaluatedProperties: false
>  
>        "^buck[1-7]$":
> @@ -51,11 +45,6 @@ properties:
>            Properties for single BUCK regulator.
>  
>          properties:
> -          regulator-name:
> -            pattern: "^buck[1-7]$"
> -            description:
> -              should be "buck1", ..., "buck7"
> -
>            nxp,ilim-ma:
>              $ref: "/schemas/types.yaml#/definitions/uint32"
>              minimum: 2100
> @@ -94,12 +83,6 @@ properties:
>          description:
>            Properties for single VSNVS regulator.
>  
> -        properties:
> -          regulator-name:
> -            pattern: "^vsnvs$"
> -            description:
> -              should be "vsnvs"
> -
>          unevaluatedProperties: false
>  
>      additionalProperties: false
> -- 
> 2.20.1
> 

Re: [PATCH v2 3/7] regulator: dt-bindings: pf8x00: fix nxp,phase-shift doc

[Rob Herring]

On Wed, Dec 16, 2020 at 12:56:35AM +0100, Adrien Grassein wrote:
> nxp,phase-shift is an enum so use enum format to describe it.
> Minimum and maximum values are also wrong.
> 
> Signed-off-by: Adrien Grassein <adrien.grassein@gmail.com>
> ---
>  .../bindings/regulator/nxp,pf8x00-regulator.yaml | 16 ++++------------
>  1 file changed, 4 insertions(+), 12 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/regulator/nxp,pf8x00-regulator.yaml b/Documentation/devicetree/bindings/regulator/nxp,pf8x00-regulator.yaml
> index 913532d0532e..1da724c6e2ba 100644
> --- a/Documentation/devicetree/bindings/regulator/nxp,pf8x00-regulator.yaml
> +++ b/Documentation/devicetree/bindings/regulator/nxp,pf8x00-regulator.yaml
> @@ -60,21 +60,13 @@ properties:
>  
>            nxp,phase-shift:
>              $ref: "/schemas/types.yaml#/definitions/uint32"
> -            minimum: 45
> -            maximum: 0
> +            minimum: 0
> +            maximum: 315

You can drop this. It's redundant with the enum.

> +            default: 0
> +            enum: [ 0, 45, 90, 135, 180, 225, 270, 315 ]
>              description:
>                BUCK regulators phase shift control in degrees.
>  
> -              Listed phase shift control values in degrees are,
> -              45
> -              90
> -              135
> -              180
> -              225
> -              270
> -              315
> -              0 (default)
> -
>          unevaluatedProperties: false
>  
>        "^vsnvs$":
> -- 
> 2.20.1
> 

Re: [PATCH v2 4/7] regulator: dt-bindings: pf8x00: remove nxp,ilim-ma property

[Rob Herring]

On Wed, 16 Dec 2020 00:56:36 +0100, Adrien Grassein wrote:
> This property seems useless because we can use the
> regulator-max-microamp generic property to do the same
> and using generic code.
> 
> The only things it changes is the default value. The original
> code was using "2100" (mA) as default one, but I think we should
> keep the value in the OTP memory as the default one. This value
> is automatically loaded in the register by the chip.
> 
> Signed-off-by: Adrien Grassein <adrien.grassein@gmail.com>
> ---
>  .../bindings/regulator/nxp,pf8x00-regulator.yaml    | 13 -------------
>  1 file changed, 13 deletions(-)
> 

Reviewed-by: Rob Herring <robh@kernel.org>