Re: [PATCH v6 3/5] regulator: add regulators driver for Marvell 88PM886 PMIC

2024-05-05 Thread Mark Brown
On Sun, May 05, 2024 at 08:52:06PM +0200, Karel Balej wrote:

> Should I then drop this op and the max_uA values from all the
> regulators?

Probably, yes.


signature.asc
Description: PGP signature


Re: [PATCH v6 3/5] regulator: add regulators driver for Marvell 88PM886 PMIC

2024-05-05 Thread Karel Balej
Mark Brown, 2024-05-06T00:15:01+09:00:
> On Sat, May 04, 2024 at 09:37:06PM +0200, Karel Balej wrote:
>
> > +static const struct regulator_ops pm886_ldo_ops = {
> > +   .list_voltage = regulator_list_voltage_table,
> > +   .map_voltage = regulator_map_voltage_iterate,
> > +   .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,
> > +   .get_current_limit = pm886_regulator_get_ilim,
>
> Do these regulators actually enforce this limit or is this just a spec
> limit beyond which regulation may fail?  If it's just a spec limit I'd
> not expect this operation to be provided, it's more for a hard limit
> where the regulator will detect and act on issues.  I don't see an error
> interrupt or anything and this would be an unusual feature for a LDO.

I'm afraid I don't have the answer -- my only reference is the vendor
version of the driver and I don't see anything there based on which I
would be able to tell.

But based on what you write, my guess would be that it's just a spec limit.

Should I then drop this op and the max_uA values from all the
regulators?

Thank you,
K. B.



Re: [PATCH v6 3/5] regulator: add regulators driver for Marvell 88PM886 PMIC

2024-05-05 Thread Mark Brown
On Sat, May 04, 2024 at 09:37:06PM +0200, Karel Balej wrote:

> +static const struct regulator_ops pm886_ldo_ops = {
> + .list_voltage = regulator_list_voltage_table,
> + .map_voltage = regulator_map_voltage_iterate,
> + .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,
> + .get_current_limit = pm886_regulator_get_ilim,

Do these regulators actually enforce this limit or is this just a spec
limit beyond which regulation may fail?  If it's just a spec limit I'd
not expect this operation to be provided, it's more for a hard limit
where the regulator will detect and act on issues.  I don't see an error
interrupt or anything and this would be an unusual feature for a LDO.


signature.asc
Description: PGP signature


[PATCH v6 3/5] regulator: add regulators driver for Marvell 88PM886 PMIC

2024-05-04 Thread Karel Balej
Support the LDO and buck regulators of the Marvell 88PM886 PMIC.

Signed-off-by: Karel Balej 
---

Notes:
v6:
- Remove all definitions (now present in the header).
v5:
- Add remaining regulators.
- Clean up includes.
- Address Mark's feedback:
  - Use dedicated regmap config.
RFC v4:
- Initialize regulators regmap in the regulators driver.
- Register all regulators at once.
- Drop regulator IDs.
- Add missing '\n' to dev_err_probe message.
- Fix includes.
- Add ID table.
RFC v3:
- Do not have a variable for each regulator -- define them all in the
  pm886_regulators array.
- Use new regulators regmap index name.
- Use dev_err_probe.
RFC v2:
- Drop of_compatible and related code.
- Drop unused include.
- Remove some abstraction: use only one regmap for all regulators and
  only mention 88PM886 in Kconfig description.
- Reword commit message.

 drivers/regulator/88pm886-regulator.c | 476 ++
 drivers/regulator/Kconfig |   6 +
 drivers/regulator/Makefile|   1 +
 3 files changed, 483 insertions(+)
 create mode 100644 drivers/regulator/88pm886-regulator.c

diff --git a/drivers/regulator/88pm886-regulator.c 
b/drivers/regulator/88pm886-regulator.c
new file mode 100644
index ..7d6e113c93b3
--- /dev/null
+++ b/drivers/regulator/88pm886-regulator.c
@@ -0,0 +1,476 @@
+// SPDX-License-Identifier: GPL-2.0-only
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include 
+
+static const struct regmap_config pm886_regulator_regmap_config = {
+   .reg_bits = 8,
+   .val_bits = 8,
+   .max_register = PM886_REG_BUCK5_VOUT,
+};
+
+struct pm886_regulator {
+   struct regulator_desc desc;
+   int max_uA;
+};
+
+static int pm886_regulator_get_ilim(struct regulator_dev *rdev)
+{
+   struct pm886_regulator *data = rdev_get_drvdata(rdev);
+
+   if (!data) {
+   dev_err(>dev, "Failed to get regulator data\n");
+   return -EINVAL;
+   }
+   return data->max_uA;
+}
+
+static const struct regulator_ops pm886_ldo_ops = {
+   .list_voltage = regulator_list_voltage_table,
+   .map_voltage = regulator_map_voltage_iterate,
+   .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,
+   .get_current_limit = pm886_regulator_get_ilim,
+};
+
+static const struct regulator_ops pm886_buck_ops = {
+   .list_voltage = regulator_list_voltage_linear_range,
+   .map_voltage = regulator_map_voltage_linear_range,
+   .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,
+   .get_current_limit = pm886_regulator_get_ilim,
+};
+
+static const unsigned int pm886_ldo_volt_table1[] = {
+   170, 180, 190, 250, 280, 290, 310, 330,
+};
+
+static const unsigned int pm886_ldo_volt_table2[] = {
+   120, 125, 170, 180, 185, 190, 250, 260,
+   270, 275, 280, 285, 290, 300, 310, 330,
+};
+
+static const unsigned int pm886_ldo_volt_table3[] = {
+   170, 180, 190, 200, 210, 250, 270, 280,
+};
+
+static const struct linear_range pm886_buck_volt_ranges1[] = {
+   REGULATOR_LINEAR_RANGE(60, 0, 79, 12500),
+   REGULATOR_LINEAR_RANGE(160, 80, 84, 5),
+};
+
+static const struct linear_range pm886_buck_volt_ranges2[] = {
+   REGULATOR_LINEAR_RANGE(60, 0, 79, 12500),
+   REGULATOR_LINEAR_RANGE(160, 80, 114, 5),
+};
+
+static struct pm886_regulator pm886_regulators[] = {
+   {
+   .desc = {
+   .name = "LDO1",
+   .regulators_node = "regulators",
+   .of_match = "ldo1",
+   .ops = _ldo_ops,
+   .type = REGULATOR_VOLTAGE,
+   .enable_reg = PM886_REG_LDO_EN1,
+   .enable_mask = BIT(0),
+   .volt_table = pm886_ldo_volt_table1,
+   .n_voltages = ARRAY_SIZE(pm886_ldo_volt_table1),
+   .vsel_reg = PM886_REG_LDO1_VOUT,
+   .vsel_mask = PM886_LDO_VSEL_MASK,
+   },
+   .max_uA = 10,
+   },
+   {
+   .desc = {
+   .name = "LDO2",
+   .regulators_node = "regulators",
+   .of_match = "ldo2",
+   .ops = _ldo_ops,
+   .type = REGULATOR_VOLTAGE,
+   .enable_reg =