2024-03-31 10:59:16

by Karel Balej

[permalink] [raw]
Subject: [PATCH 0/5] initial support for Marvell 88PM886 PMIC

Hello,

the following implements basic support for Marvell's 88PM886 PMIC which
is found for instance as a component of the samsung,coreprimevelte
smartphone which inspired this and also serves as a testing platform.

The code for the MFD is based primarily on this old series [1] with the
addition of poweroff based on the smartphone's downstream kernel tree
[2]. The onkey and regulators drivers are based on the latter. I am not
in possesion of the datasheet.

[1] https://lore.kernel.org/all/[email protected]/
[2] https://github.com/CoderCharmander/g361f-kernel

Thank you and kind regards,
K. B.
---
v1:
- RFC v4: https://lore.kernel.org/r/[email protected]/
- Rebase to v6.9-rc1.
- Thank you to everybody for their feedback on the RFC!
RFC v4:
- RFC v3: https://lore.kernel.org/all/[email protected]/
RFC v3:
- Address Rob's feedback:
- Drop onkey bindings patch.
- Rename PM88X -> PM886 everywhere.
- RFC v2: https://lore.kernel.org/all/[email protected]/
RFC v2:
- Merge with the regulators series to have multiple devices and thus
justify the use of the MFD framework.
- Rebase on v6.8-rc3.
- Reorder patches.
- MFD RFC v1: https://lore.kernel.org/all/[email protected]/
- regulators RFC v1: https://lore.kernel.org/all/[email protected]/

Karel Balej (5):
dt-bindings: mfd: add entry for Marvell 88PM886 PMIC
mfd: add driver for Marvell 88PM886 PMIC
regulator: add regulators driver for Marvell 88PM886 PMIC
input: add onkey driver for Marvell 88PM886 PMIC
MAINTAINERS: add myself for Marvell 88PM886 PMIC

.../bindings/mfd/marvell,88pm886-a1.yaml | 76 +++
MAINTAINERS | 9 +
drivers/input/misc/88pm886-onkey.c | 98 ++++
drivers/input/misc/Kconfig | 7 +
drivers/input/misc/Makefile | 1 +
drivers/mfd/88pm886.c | 157 ++++++
drivers/mfd/Kconfig | 12 +
drivers/mfd/Makefile | 1 +
drivers/regulator/88pm886-regulator.c | 509 ++++++++++++++++++
drivers/regulator/Kconfig | 6 +
drivers/regulator/Makefile | 1 +
include/linux/mfd/88pm886.h | 30 ++
12 files changed, 907 insertions(+)
create mode 100644 Documentation/devicetree/bindings/mfd/marvell,88pm886-a1.yaml
create mode 100644 drivers/input/misc/88pm886-onkey.c
create mode 100644 drivers/mfd/88pm886.c
create mode 100644 drivers/regulator/88pm886-regulator.c
create mode 100644 include/linux/mfd/88pm886.h

--
2.44.0



2024-03-31 10:59:41

by Karel Balej

[permalink] [raw]
Subject: [PATCH 5/5] MAINTAINERS: add myself for Marvell 88PM886 PMIC

Add an entry to MAINTAINERS for the Marvell 88PM886 PMIC MFD, onkey and
regulator drivers.

Signed-off-by: Karel Balej <[email protected]>
---

Notes:
RFC v3:
- Remove onkey bindings file.
RFC v2:
- Only mention 88PM886 in the commit message.
- Add regulator driver.
- Rename the entry.

MAINTAINERS | 9 +++++++++
1 file changed, 9 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index aa3b947fb080..c6bdf93ea3a7 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -13048,6 +13048,15 @@ F: drivers/net/dsa/mv88e6xxx/
F: include/linux/dsa/mv88e6xxx.h
F: include/linux/platform_data/mv88e6xxx.h

+MARVELL 88PM886 PMIC DRIVER
+M: Karel Balej <[email protected]>
+S: Maintained
+F: Documentation/devicetree/bindings/mfd/marvell,88pm886-a1.yaml
+F: drivers/input/misc/88pm886-onkey.c
+F: drivers/mfd/88pm886.c
+F: drivers/regulators/88pm886-regulator.c
+F: include/linux/mfd/88pm886.h
+
MARVELL ARMADA 3700 PHY DRIVERS
M: Miquel Raynal <[email protected]>
S: Maintained
--
2.44.0


2024-03-31 10:59:55

by Karel Balej

[permalink] [raw]
Subject: [PATCH 2/5] mfd: add driver for Marvell 88PM886 PMIC

Marvell 88PM886 is a PMIC which provides various functions such as
onkey, battery, charger and regulators. It is found for instance in the
samsung,coreprimevelte smartphone with which this was tested. Implement
basic support to allow for the use of regulators and onkey.

Signed-off-by: Karel Balej <[email protected]>
---

Notes:
v1:
- Address Mark's feedback:
- Move regmap config back out of the header and rename it. Also lower
its maximum register based on what's actually used in the downstream
code.
RFC v4:
- Use MFD_CELL_* macros.
- Address Lee's feedback:
- Do not define regmap_config.val_bits and .reg_bits.
- Drop everything regulator related except mfd_cell (regmap
initialization, IDs enum etc.). Drop pm886_initialize_subregmaps.
- Do not store regmap pointers as an array as there is now only one
regmap. Also drop the corresponding enum.
- Move regmap_config to the header as it is needed in the regulators
driver.
- pm886_chip.whoami -> chip_id
- Reword chip ID mismatch error message and print the ID as
hexadecimal.
- Fix includes in include/linux/88pm886.h.
- Drop the pm886_irq_number enum and define the (for the moment) only
IRQ explicitly.
- Have only one MFD cell for all regulators as they are now registered
all at once in the regulators driver.
- Reword commit message.
- Make device table static and remove comma after the sentinel to signal
that nothing should come after it.
RFC v3:
- Drop onkey cell .of_compatible.
- Rename LDO page offset and regmap to REGULATORS.
RFC v2:
- Remove some abstraction.
- Sort includes alphabetically and add linux/of.h.
- Depend on OF, remove of_match_ptr and add MODULE_DEVICE_TABLE.
- Use more temporaries and break long lines.
- Do not initialize ret in probe.
- Use the wakeup-source DT property.
- Rename ret to err.
- Address Lee's comments:
- Drop patched in presets for base regmap and related defines.
- Use full sentences in comments.
- Remove IRQ comment.
- Define regmap_config member values.
- Rename data to sys_off_data.
- Add _PMIC suffix to Kconfig.
- Use dev_err_probe.
- Do not store irq_data.
- s/WHOAMI/CHIP_ID
- Drop LINUX part of include guard name.
- Merge in the regulator series modifications in order to have more
devices and modify the commit message accordingly. Changes with
respect to the original regulator series patches:
- ret -> err
- Add temporary for dev in pm88x_initialize_subregmaps.
- Drop of_compatible for the regulators.
- Do not duplicate LDO regmap for bucks.
- Rewrite commit message.

drivers/mfd/88pm886.c | 157 ++++++++++++++++++++++++++++++++++++
drivers/mfd/Kconfig | 12 +++
drivers/mfd/Makefile | 1 +
include/linux/mfd/88pm886.h | 30 +++++++
4 files changed, 200 insertions(+)
create mode 100644 drivers/mfd/88pm886.c
create mode 100644 include/linux/mfd/88pm886.h

diff --git a/drivers/mfd/88pm886.c b/drivers/mfd/88pm886.c
new file mode 100644
index 000000000000..e06d418a5da9
--- /dev/null
+++ b/drivers/mfd/88pm886.c
@@ -0,0 +1,157 @@
+// SPDX-License-Identifier: GPL-2.0-only
+#include <linux/i2c.h>
+#include <linux/mfd/core.h>
+#include <linux/module.h>
+#include <linux/notifier.h>
+#include <linux/of.h>
+#include <linux/reboot.h>
+#include <linux/regmap.h>
+
+#include <linux/mfd/88pm886.h>
+
+#define PM886_REG_INT_STATUS1 0x05
+
+#define PM886_REG_INT_ENA_1 0x0a
+#define PM886_INT_ENA1_ONKEY BIT(0)
+
+#define PM886_IRQ_ONKEY 0
+
+#define PM886_REGMAP_CONF_MAX_REG 0xef
+
+static const struct regmap_config pm886_regmap_config = {
+ .reg_bits = 8,
+ .val_bits = 8,
+ .max_register = PM886_REGMAP_CONF_MAX_REG,
+};
+
+static struct regmap_irq pm886_regmap_irqs[] = {
+ REGMAP_IRQ_REG(PM886_IRQ_ONKEY, 0, PM886_INT_ENA1_ONKEY),
+};
+
+static struct regmap_irq_chip pm886_regmap_irq_chip = {
+ .name = "88pm886",
+ .irqs = pm886_regmap_irqs,
+ .num_irqs = ARRAY_SIZE(pm886_regmap_irqs),
+ .num_regs = 4,
+ .status_base = PM886_REG_INT_STATUS1,
+ .ack_base = PM886_REG_INT_STATUS1,
+ .unmask_base = PM886_REG_INT_ENA_1,
+};
+
+static struct resource pm886_onkey_resources[] = {
+ DEFINE_RES_IRQ_NAMED(PM886_IRQ_ONKEY, "88pm886-onkey"),
+};
+
+static struct mfd_cell pm886_devs[] = {
+ MFD_CELL_RES("88pm886-onkey", pm886_onkey_resources),
+ MFD_CELL_NAME("88pm886-regulator"),
+};
+
+static int pm886_power_off_handler(struct sys_off_data *sys_off_data)
+{
+ struct pm886_chip *chip = sys_off_data->cb_data;
+ struct regmap *regmap = chip->regmap;
+ struct device *dev = &chip->client->dev;
+ int err;
+
+ err = regmap_update_bits(regmap, PM886_REG_MISC_CONFIG1, PM886_SW_PDOWN,
+ PM886_SW_PDOWN);
+ if (err) {
+ dev_err(dev, "Failed to power off the device: %d\n", err);
+ return NOTIFY_BAD;
+ }
+ return NOTIFY_DONE;
+}
+
+static int pm886_setup_irq(struct pm886_chip *chip,
+ struct regmap_irq_chip_data **irq_data)
+{
+ struct regmap *regmap = chip->regmap;
+ struct device *dev = &chip->client->dev;
+ int err;
+
+ /* Set interrupt clearing mode to clear on write. */
+ err = regmap_update_bits(regmap, PM886_REG_MISC_CONFIG2,
+ PM886_INT_INV | PM886_INT_CLEAR | PM886_INT_MASK_MODE,
+ PM886_INT_WC);
+ if (err) {
+ dev_err(dev, "Failed to set interrupt clearing mode: %d\n", err);
+ return err;
+ }
+
+ err = devm_regmap_add_irq_chip(dev, regmap, chip->client->irq,
+ IRQF_ONESHOT, -1, &pm886_regmap_irq_chip,
+ irq_data);
+ if (err) {
+ dev_err(dev, "Failed to request IRQ: %d\n", err);
+ return err;
+ }
+
+ return 0;
+}
+
+static int pm886_probe(struct i2c_client *client)
+{
+ struct regmap_irq_chip_data *irq_data;
+ struct device *dev = &client->dev;
+ struct pm886_chip *chip;
+ struct regmap *regmap;
+ unsigned int chip_id;
+ int err;
+
+ chip = devm_kzalloc(dev, sizeof(*chip), GFP_KERNEL);
+ if (!chip)
+ return -ENOMEM;
+
+ chip->client = client;
+ chip->chip_id = (uintptr_t)device_get_match_data(dev);
+ i2c_set_clientdata(client, chip);
+
+ regmap = devm_regmap_init_i2c(client, &pm886_regmap_config);
+ if (IS_ERR(regmap))
+ return dev_err_probe(dev, PTR_ERR(regmap), "Failed to initialize regmap\n");
+ chip->regmap = regmap;
+
+ err = regmap_read(regmap, PM886_REG_ID, &chip_id);
+ if (err)
+ return dev_err_probe(dev, err, "Failed to read chip ID\n");
+
+ if (chip->chip_id != chip_id)
+ return dev_err_probe(dev, -EINVAL, "Unsupported chip: 0x%x\n", chip_id);
+
+ err = pm886_setup_irq(chip, &irq_data);
+ if (err)
+ return err;
+
+ err = devm_mfd_add_devices(dev, 0, pm886_devs, ARRAY_SIZE(pm886_devs),
+ NULL, 0, regmap_irq_get_domain(irq_data));
+ if (err)
+ return dev_err_probe(dev, err, "Failed to add devices\n");
+
+ err = devm_register_power_off_handler(dev, pm886_power_off_handler, chip);
+ if (err)
+ return dev_err_probe(dev, err, "Failed to register power off handler\n");
+
+ device_init_wakeup(dev, device_property_read_bool(dev, "wakeup-source"));
+
+ return 0;
+}
+
+static const struct of_device_id pm886_of_match[] = {
+ { .compatible = "marvell,88pm886-a1", .data = (void *)PM886_A1_CHIP_ID },
+ { }
+};
+MODULE_DEVICE_TABLE(of, pm886_of_match);
+
+static struct i2c_driver pm886_i2c_driver = {
+ .driver = {
+ .name = "88pm886",
+ .of_match_table = pm886_of_match,
+ },
+ .probe = pm886_probe,
+};
+module_i2c_driver(pm886_i2c_driver);
+
+MODULE_DESCRIPTION("Marvell 88PM886 PMIC driver");
+MODULE_AUTHOR("Karel Balej <[email protected]>");
+MODULE_LICENSE("GPL");
diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index 4b023ee229cf..d6a762b2bd47 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -794,6 +794,18 @@ config MFD_88PM860X
select individual components like voltage regulators, RTC and
battery-charger under the corresponding menus.

+config MFD_88PM886_PMIC
+ bool "Marvell 88PM886 PMIC"
+ depends on I2C=y
+ depends on OF
+ select REGMAP_I2C
+ select REGMAP_IRQ
+ select MFD_CORE
+ help
+ This enables support for Marvell 88PM886 Power Management IC.
+ This includes the I2C driver and the core APIs _only_, you have to
+ select individual components like onkey under the corresponding menus.
+
config MFD_MAX14577
tristate "Maxim Semiconductor MAX14577/77836 MUIC + Charger Support"
depends on I2C
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index c66f07edcd0e..d016b7fed354 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -7,6 +7,7 @@
obj-$(CONFIG_MFD_88PM860X) += 88pm860x.o
obj-$(CONFIG_MFD_88PM800) += 88pm800.o 88pm80x.o
obj-$(CONFIG_MFD_88PM805) += 88pm805.o 88pm80x.o
+obj-$(CONFIG_MFD_88PM886_PMIC) += 88pm886.o
obj-$(CONFIG_MFD_ACT8945A) += act8945a.o
obj-$(CONFIG_MFD_SM501) += sm501.o
obj-$(CONFIG_ARCH_BCM2835) += bcm2835-pm.o
diff --git a/include/linux/mfd/88pm886.h b/include/linux/mfd/88pm886.h
new file mode 100644
index 000000000000..5ce30a3b85aa
--- /dev/null
+++ b/include/linux/mfd/88pm886.h
@@ -0,0 +1,30 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+#ifndef __MFD_88PM886_H
+#define __MFD_88PM886_H
+
+#include <linux/i2c.h>
+#include <linux/regmap.h>
+
+#define PM886_A1_CHIP_ID 0xa1
+
+#define PM886_REG_ID 0x00
+
+#define PM886_REG_STATUS1 0x01
+#define PM886_ONKEY_STS1 BIT(0)
+
+#define PM886_REG_MISC_CONFIG1 0x14
+#define PM886_SW_PDOWN BIT(5)
+
+#define PM886_REG_MISC_CONFIG2 0x15
+#define PM886_INT_INV BIT(0)
+#define PM886_INT_CLEAR BIT(1)
+#define PM886_INT_RC 0x00
+#define PM886_INT_WC BIT(1)
+#define PM886_INT_MASK_MODE BIT(2)
+
+struct pm886_chip {
+ struct i2c_client *client;
+ unsigned int chip_id;
+ struct regmap *regmap;
+};
+#endif /* __MFD_88PM886_H */
--
2.44.0


2024-03-31 11:07:41

by Karel Balej

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

Support the LDO and buck regulators of the Marvell 88PM886 PMIC.

Signed-off-by: Karel Balej <[email protected]>
---

Notes:
v1:
- 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 | 509 ++++++++++++++++++++++++++
drivers/regulator/Kconfig | 6 +
drivers/regulator/Makefile | 1 +
3 files changed, 516 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 000000000000..05d24bf444cb
--- /dev/null
+++ b/drivers/regulator/88pm886-regulator.c
@@ -0,0 +1,509 @@
+// SPDX-License-Identifier: GPL-2.0-only
+#include <linux/i2c.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/regulator/driver.h>
+
+#include <linux/mfd/88pm886.h>
+
+#define PM886_PAGE_OFFSET_REGULATORS 1
+
+#define PM886_REG_LDO_EN1 0x09
+#define PM886_REG_LDO_EN2 0x0a
+
+#define PM886_REG_BUCK_EN 0x08
+
+#define PM886_REG_LDO1_VOUT 0x20
+#define PM886_REG_LDO2_VOUT 0x26
+#define PM886_REG_LDO3_VOUT 0x2c
+#define PM886_REG_LDO4_VOUT 0x32
+#define PM886_REG_LDO5_VOUT 0x38
+#define PM886_REG_LDO6_VOUT 0x3e
+#define PM886_REG_LDO7_VOUT 0x44
+#define PM886_REG_LDO8_VOUT 0x4a
+#define PM886_REG_LDO9_VOUT 0x50
+#define PM886_REG_LDO10_VOUT 0x56
+#define PM886_REG_LDO11_VOUT 0x5c
+#define PM886_REG_LDO12_VOUT 0x62
+#define PM886_REG_LDO13_VOUT 0x68
+#define PM886_REG_LDO14_VOUT 0x6e
+#define PM886_REG_LDO15_VOUT 0x74
+#define PM886_REG_LDO16_VOUT 0x7a
+
+#define PM886_REG_BUCK1_VOUT 0xa5
+#define PM886_REG_BUCK2_VOUT 0xb3
+#define PM886_REG_BUCK3_VOUT 0xc1
+#define PM886_REG_BUCK4_VOUT 0xcf
+#define PM886_REG_BUCK5_VOUT 0xdd
+
+#define PM886_LDO_VSEL_MASK 0x0f
+#define PM886_BUCK_VSEL_MASK 0x7f
+
+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(&rdev->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[] = {
+ 1700000, 1800000, 1900000, 2500000, 2800000, 2900000, 3100000, 3300000,
+};
+
+static const unsigned int pm886_ldo_volt_table2[] = {
+ 1200000, 1250000, 1700000, 1800000, 1850000, 1900000, 2500000, 2600000,
+ 2700000, 2750000, 2800000, 2850000, 2900000, 3000000, 3100000, 3300000,
+};
+
+static const unsigned int pm886_ldo_volt_table3[] = {
+ 1700000, 1800000, 1900000, 2000000, 2100000, 2500000, 2700000, 2800000,
+};
+
+static const struct linear_range pm886_buck_volt_ranges1[] = {
+ REGULATOR_LINEAR_RANGE(600000, 0, 79, 12500),
+ REGULATOR_LINEAR_RANGE(1600000, 80, 84, 50000),
+};
+
+static const struct linear_range pm886_buck_volt_ranges2[] = {
+ REGULATOR_LINEAR_RANGE(600000, 0, 79, 12500),
+ REGULATOR_LINEAR_RANGE(1600000, 80, 114, 50000),
+};
+
+static struct pm886_regulator pm886_regulators[] = {
+ {
+ .desc = {
+ .name = "LDO1",
+ .regulators_node = "regulators",
+ .of_match = "ldo1",
+ .ops = &pm886_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 = 100000,
+ },
+ {
+ .desc = {
+ .name = "LDO2",
+ .regulators_node = "regulators",
+ .of_match = "ldo2",
+ .ops = &pm886_ldo_ops,
+ .type = REGULATOR_VOLTAGE,
+ .enable_reg = PM886_REG_LDO_EN1,
+ .enable_mask = BIT(1),
+ .volt_table = pm886_ldo_volt_table1,
+ .n_voltages = ARRAY_SIZE(pm886_ldo_volt_table1),
+ .vsel_reg = PM886_REG_LDO2_VOUT,
+ .vsel_mask = PM886_LDO_VSEL_MASK,
+ },
+ .max_uA = 100000,
+ },
+ {
+ .desc = {
+ .name = "LDO3",
+ .regulators_node = "regulators",
+ .of_match = "ldo3",
+ .ops = &pm886_ldo_ops,
+ .type = REGULATOR_VOLTAGE,
+ .enable_reg = PM886_REG_LDO_EN1,
+ .enable_mask = BIT(2),
+ .volt_table = pm886_ldo_volt_table1,
+ .n_voltages = ARRAY_SIZE(pm886_ldo_volt_table1),
+ .vsel_reg = PM886_REG_LDO3_VOUT,
+ .vsel_mask = PM886_LDO_VSEL_MASK,
+ },
+ .max_uA = 100000,
+ },
+ {
+ .desc = {
+ .name = "LDO4",
+ .regulators_node = "regulators",
+ .of_match = "ldo4",
+ .ops = &pm886_ldo_ops,
+ .type = REGULATOR_VOLTAGE,
+ .enable_reg = PM886_REG_LDO_EN1,
+ .enable_mask = BIT(3),
+ .volt_table = pm886_ldo_volt_table2,
+ .n_voltages = ARRAY_SIZE(pm886_ldo_volt_table2),
+ .vsel_reg = PM886_REG_LDO4_VOUT,
+ .vsel_mask = PM886_LDO_VSEL_MASK,
+ },
+ .max_uA = 400000,
+ },
+ {
+ .desc = {
+ .name = "LDO5",
+ .regulators_node = "regulators",
+ .of_match = "ldo5",
+ .ops = &pm886_ldo_ops,
+ .type = REGULATOR_VOLTAGE,
+ .enable_reg = PM886_REG_LDO_EN1,
+ .enable_mask = BIT(4),
+ .volt_table = pm886_ldo_volt_table2,
+ .n_voltages = ARRAY_SIZE(pm886_ldo_volt_table2),
+ .vsel_reg = PM886_REG_LDO5_VOUT,
+ .vsel_mask = PM886_LDO_VSEL_MASK,
+ },
+ .max_uA = 400000,
+ },
+ {
+ .desc = {
+ .name = "LDO6",
+ .regulators_node = "regulators",
+ .of_match = "ldo6",
+ .ops = &pm886_ldo_ops,
+ .type = REGULATOR_VOLTAGE,
+ .enable_reg = PM886_REG_LDO_EN1,
+ .enable_mask = BIT(5),
+ .volt_table = pm886_ldo_volt_table2,
+ .n_voltages = ARRAY_SIZE(pm886_ldo_volt_table2),
+ .vsel_reg = PM886_REG_LDO6_VOUT,
+ .vsel_mask = PM886_LDO_VSEL_MASK,
+ },
+ .max_uA = 400000,
+ },
+ {
+ .desc = {
+ .name = "LDO7",
+ .regulators_node = "regulators",
+ .of_match = "ldo7",
+ .ops = &pm886_ldo_ops,
+ .type = REGULATOR_VOLTAGE,
+ .enable_reg = PM886_REG_LDO_EN1,
+ .enable_mask = BIT(6),
+ .volt_table = pm886_ldo_volt_table2,
+ .n_voltages = ARRAY_SIZE(pm886_ldo_volt_table2),
+ .vsel_reg = PM886_REG_LDO7_VOUT,
+ .vsel_mask = PM886_LDO_VSEL_MASK,
+ },
+ .max_uA = 400000,
+ },
+ {
+ .desc = {
+ .name = "LDO8",
+ .regulators_node = "regulators",
+ .of_match = "ldo8",
+ .ops = &pm886_ldo_ops,
+ .type = REGULATOR_VOLTAGE,
+ .enable_reg = PM886_REG_LDO_EN1,
+ .enable_mask = BIT(7),
+ .volt_table = pm886_ldo_volt_table2,
+ .n_voltages = ARRAY_SIZE(pm886_ldo_volt_table2),
+ .vsel_reg = PM886_REG_LDO8_VOUT,
+ .vsel_mask = PM886_LDO_VSEL_MASK,
+ },
+ .max_uA = 400000,
+ },
+ {
+ .desc = {
+ .name = "LDO9",
+ .regulators_node = "regulators",
+ .of_match = "ldo9",
+ .ops = &pm886_ldo_ops,
+ .type = REGULATOR_VOLTAGE,
+ .enable_reg = PM886_REG_LDO_EN2,
+ .enable_mask = BIT(0),
+ .volt_table = pm886_ldo_volt_table2,
+ .n_voltages = ARRAY_SIZE(pm886_ldo_volt_table2),
+ .vsel_reg = PM886_REG_LDO9_VOUT,
+ .vsel_mask = PM886_LDO_VSEL_MASK,
+ },
+ .max_uA = 400000,
+ },
+ {
+ .desc = {
+ .name = "LDO10",
+ .regulators_node = "regulators",
+ .of_match = "ldo10",
+ .ops = &pm886_ldo_ops,
+ .type = REGULATOR_VOLTAGE,
+ .enable_reg = PM886_REG_LDO_EN2,
+ .enable_mask = BIT(1),
+ .volt_table = pm886_ldo_volt_table2,
+ .n_voltages = ARRAY_SIZE(pm886_ldo_volt_table2),
+ .vsel_reg = PM886_REG_LDO10_VOUT,
+ .vsel_mask = PM886_LDO_VSEL_MASK,
+ },
+ .max_uA = 200000,
+ },
+ {
+ .desc = {
+ .name = "LDO11",
+ .regulators_node = "regulators",
+ .of_match = "ldo11",
+ .ops = &pm886_ldo_ops,
+ .type = REGULATOR_VOLTAGE,
+ .enable_reg = PM886_REG_LDO_EN2,
+ .enable_mask = BIT(2),
+ .volt_table = pm886_ldo_volt_table2,
+ .n_voltages = ARRAY_SIZE(pm886_ldo_volt_table2),
+ .vsel_reg = PM886_REG_LDO11_VOUT,
+ .vsel_mask = PM886_LDO_VSEL_MASK,
+ },
+ .max_uA = 200000,
+ },
+ {
+ .desc = {
+ .name = "LDO12",
+ .regulators_node = "regulators",
+ .of_match = "ldo12",
+ .ops = &pm886_ldo_ops,
+ .type = REGULATOR_VOLTAGE,
+ .enable_reg = PM886_REG_LDO_EN2,
+ .enable_mask = BIT(3),
+ .volt_table = pm886_ldo_volt_table2,
+ .n_voltages = ARRAY_SIZE(pm886_ldo_volt_table2),
+ .vsel_reg = PM886_REG_LDO12_VOUT,
+ .vsel_mask = PM886_LDO_VSEL_MASK,
+ },
+ .max_uA = 200000,
+ },
+ {
+ .desc = {
+ .name = "LDO13",
+ .regulators_node = "regulators",
+ .of_match = "ldo13",
+ .ops = &pm886_ldo_ops,
+ .type = REGULATOR_VOLTAGE,
+ .enable_reg = PM886_REG_LDO_EN2,
+ .enable_mask = BIT(4),
+ .volt_table = pm886_ldo_volt_table2,
+ .n_voltages = ARRAY_SIZE(pm886_ldo_volt_table2),
+ .vsel_reg = PM886_REG_LDO13_VOUT,
+ .vsel_mask = PM886_LDO_VSEL_MASK,
+ },
+ .max_uA = 200000,
+ },
+ {
+ .desc = {
+ .name = "LDO14",
+ .regulators_node = "regulators",
+ .of_match = "ldo14",
+ .ops = &pm886_ldo_ops,
+ .type = REGULATOR_VOLTAGE,
+ .enable_reg = PM886_REG_LDO_EN2,
+ .enable_mask = BIT(5),
+ .volt_table = pm886_ldo_volt_table2,
+ .n_voltages = ARRAY_SIZE(pm886_ldo_volt_table2),
+ .vsel_reg = PM886_REG_LDO14_VOUT,
+ .vsel_mask = PM886_LDO_VSEL_MASK,
+ },
+ .max_uA = 200000,
+ },
+ {
+ .desc = {
+ .name = "LDO15",
+ .regulators_node = "regulators",
+ .of_match = "ldo15",
+ .ops = &pm886_ldo_ops,
+ .type = REGULATOR_VOLTAGE,
+ .enable_reg = PM886_REG_LDO_EN2,
+ .enable_mask = BIT(6),
+ .volt_table = pm886_ldo_volt_table2,
+ .n_voltages = ARRAY_SIZE(pm886_ldo_volt_table2),
+ .vsel_reg = PM886_REG_LDO15_VOUT,
+ .vsel_mask = PM886_LDO_VSEL_MASK,
+ },
+ .max_uA = 200000,
+ },
+ {
+ .desc = {
+ .name = "LDO16",
+ .regulators_node = "regulators",
+ .of_match = "ldo16",
+ .ops = &pm886_ldo_ops,
+ .type = REGULATOR_VOLTAGE,
+ .enable_reg = PM886_REG_LDO_EN2,
+ .enable_mask = BIT(7),
+ .volt_table = pm886_ldo_volt_table3,
+ .n_voltages = ARRAY_SIZE(pm886_ldo_volt_table3),
+ .vsel_reg = PM886_REG_LDO16_VOUT,
+ .vsel_mask = PM886_LDO_VSEL_MASK,
+ },
+ .max_uA = 200000,
+ },
+ {
+ .desc = {
+ .name = "buck1",
+ .regulators_node = "regulators",
+ .of_match = "buck1",
+ .ops = &pm886_buck_ops,
+ .type = REGULATOR_VOLTAGE,
+ .n_voltages = 85,
+ .linear_ranges = pm886_buck_volt_ranges1,
+ .n_linear_ranges = ARRAY_SIZE(pm886_buck_volt_ranges1),
+ .vsel_reg = PM886_REG_BUCK1_VOUT,
+ .vsel_mask = PM886_BUCK_VSEL_MASK,
+ .enable_reg = PM886_REG_BUCK_EN,
+ .enable_mask = BIT(0),
+ },
+ .max_uA = 3000000,
+ },
+ {
+ .desc = {
+ .name = "buck2",
+ .regulators_node = "regulators",
+ .of_match = "buck2",
+ .ops = &pm886_buck_ops,
+ .type = REGULATOR_VOLTAGE,
+ .n_voltages = 115,
+ .linear_ranges = pm886_buck_volt_ranges2,
+ .n_linear_ranges = ARRAY_SIZE(pm886_buck_volt_ranges2),
+ .vsel_reg = PM886_REG_BUCK2_VOUT,
+ .vsel_mask = PM886_BUCK_VSEL_MASK,
+ .enable_reg = PM886_REG_BUCK_EN,
+ .enable_mask = BIT(1),
+ },
+ .max_uA = 1200000,
+ },
+ {
+ .desc = {
+ .name = "buck3",
+ .regulators_node = "regulators",
+ .of_match = "buck3",
+ .ops = &pm886_buck_ops,
+ .type = REGULATOR_VOLTAGE,
+ .n_voltages = 115,
+ .linear_ranges = pm886_buck_volt_ranges2,
+ .n_linear_ranges = ARRAY_SIZE(pm886_buck_volt_ranges2),
+ .vsel_reg = PM886_REG_BUCK3_VOUT,
+ .vsel_mask = PM886_BUCK_VSEL_MASK,
+ .enable_reg = PM886_REG_BUCK_EN,
+ .enable_mask = BIT(2),
+ },
+ .max_uA = 1200000,
+ },
+ {
+ .desc = {
+ .name = "buck4",
+ .regulators_node = "regulators",
+ .of_match = "buck4",
+ .ops = &pm886_buck_ops,
+ .type = REGULATOR_VOLTAGE,
+ .n_voltages = 115,
+ .linear_ranges = pm886_buck_volt_ranges2,
+ .n_linear_ranges = ARRAY_SIZE(pm886_buck_volt_ranges2),
+ .vsel_reg = PM886_REG_BUCK4_VOUT,
+ .vsel_mask = PM886_BUCK_VSEL_MASK,
+ .enable_reg = PM886_REG_BUCK_EN,
+ .enable_mask = BIT(3),
+ },
+ .max_uA = 1200000,
+ },
+ {
+ .desc = {
+ .name = "buck5",
+ .regulators_node = "regulators",
+ .of_match = "buck5",
+ .ops = &pm886_buck_ops,
+ .type = REGULATOR_VOLTAGE,
+ .n_voltages = 115,
+ .linear_ranges = pm886_buck_volt_ranges2,
+ .n_linear_ranges = ARRAY_SIZE(pm886_buck_volt_ranges2),
+ .vsel_reg = PM886_REG_BUCK5_VOUT,
+ .vsel_mask = PM886_BUCK_VSEL_MASK,
+ .enable_reg = PM886_REG_BUCK_EN,
+ .enable_mask = BIT(4),
+ },
+ .max_uA = 1200000,
+ },
+};
+
+static int pm886_regulator_probe(struct platform_device *pdev)
+{
+ struct pm886_chip *chip = dev_get_drvdata(pdev->dev.parent);
+ struct regulator_config rcfg = { };
+ struct pm886_regulator *regulator;
+ struct device *dev = &pdev->dev;
+ struct regulator_desc *rdesc;
+ struct regulator_dev *rdev;
+ struct i2c_client *page;
+ struct regmap *regmap;
+
+ page = devm_i2c_new_dummy_device(dev, chip->client->adapter,
+ chip->client->addr + PM886_PAGE_OFFSET_REGULATORS);
+ if (IS_ERR(page))
+ return dev_err_probe(dev, PTR_ERR(page),
+ "Failed to initialize regulators client\n");
+
+ regmap = devm_regmap_init_i2c(page, &pm886_regulator_regmap_config);
+ if (IS_ERR(regmap))
+ return dev_err_probe(dev, PTR_ERR(regmap),
+ "Failed to initialize regulators regmap\n");
+ rcfg.regmap = regmap;
+
+ rcfg.dev = dev->parent;
+
+ for (int i = 0; i < ARRAY_SIZE(pm886_regulators); i++) {
+ regulator = &pm886_regulators[i];
+ rdesc = &regulator->desc;
+ rcfg.driver_data = regulator;
+ rdev = devm_regulator_register(dev, rdesc, &rcfg);
+ if (IS_ERR(rdev))
+ return dev_err_probe(dev, PTR_ERR(rdev),
+ "Failed to register %s\n", rdesc->name);
+ }
+
+ return 0;
+}
+
+static const struct platform_device_id pm886_regulator_id_table[] = {
+ { "88pm886-regulator", },
+ { }
+};
+MODULE_DEVICE_TABLE(platform, pm886_regulator_id_table);
+
+static struct platform_driver pm886_regulator_driver = {
+ .driver = {
+ .name = "88pm886-regulator",
+ },
+ .probe = pm886_regulator_probe,
+ .id_table = pm886_regulator_id_table,
+};
+module_platform_driver(pm886_regulator_driver);
+
+MODULE_DESCRIPTION("Marvell 88PM886 PMIC regulator driver");
+MODULE_AUTHOR("Karel Balej <[email protected]>");
+MODULE_LICENSE("GPL");
diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
index 7db0a29b5b8d..89845892c533 100644
--- a/drivers/regulator/Kconfig
+++ b/drivers/regulator/Kconfig
@@ -91,6 +91,12 @@ config REGULATOR_88PM8607
help
This driver supports 88PM8607 voltage regulator chips.

+config REGULATOR_88PM886
+ tristate "Marvell 88PM886 voltage regulators"
+ depends on MFD_88PM886_PMIC
+ help
+ This driver implements support for Marvell 88PM886 voltage regulators.
+
config REGULATOR_ACT8865
tristate "Active-semi act8865 voltage regulator"
depends on I2C
diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
index 46fb569e6be8..f30089b74b2e 100644
--- a/drivers/regulator/Makefile
+++ b/drivers/regulator/Makefile
@@ -14,6 +14,7 @@ obj-$(CONFIG_REGULATOR_USERSPACE_CONSUMER) += userspace-consumer.o
obj-$(CONFIG_REGULATOR_88PG86X) += 88pg86x.o
obj-$(CONFIG_REGULATOR_88PM800) += 88pm800-regulator.o
obj-$(CONFIG_REGULATOR_88PM8607) += 88pm8607.o
+obj-$(CONFIG_REGULATOR_88PM886) += 88pm886-regulator.o
obj-$(CONFIG_REGULATOR_CROS_EC) += cros-ec-regulator.o
obj-$(CONFIG_REGULATOR_CPCAP) += cpcap-regulator.o
obj-$(CONFIG_REGULATOR_AAT2870) += aat2870-regulator.o
--
2.44.0


2024-04-11 11:38:14

by Lee Jones

[permalink] [raw]
Subject: Re: [PATCH 2/5] mfd: add driver for Marvell 88PM886 PMIC

On Sun, 31 Mar 2024, Karel Balej wrote:

> Marvell 88PM886 is a PMIC which provides various functions such as
> onkey, battery, charger and regulators. It is found for instance in the
> samsung,coreprimevelte smartphone with which this was tested. Implement
> basic support to allow for the use of regulators and onkey.
>
> Signed-off-by: Karel Balej <[email protected]>
> ---
>
> Notes:
> v1:
> - Address Mark's feedback:
> - Move regmap config back out of the header and rename it. Also lower
> its maximum register based on what's actually used in the downstream
> code.
> RFC v4:
> - Use MFD_CELL_* macros.
> - Address Lee's feedback:
> - Do not define regmap_config.val_bits and .reg_bits.
> - Drop everything regulator related except mfd_cell (regmap
> initialization, IDs enum etc.). Drop pm886_initialize_subregmaps.
> - Do not store regmap pointers as an array as there is now only one
> regmap. Also drop the corresponding enum.
> - Move regmap_config to the header as it is needed in the regulators
> driver.
> - pm886_chip.whoami -> chip_id
> - Reword chip ID mismatch error message and print the ID as
> hexadecimal.
> - Fix includes in include/linux/88pm886.h.
> - Drop the pm886_irq_number enum and define the (for the moment) only
> IRQ explicitly.
> - Have only one MFD cell for all regulators as they are now registered
> all at once in the regulators driver.
> - Reword commit message.
> - Make device table static and remove comma after the sentinel to signal
> that nothing should come after it.
> RFC v3:
> - Drop onkey cell .of_compatible.
> - Rename LDO page offset and regmap to REGULATORS.
> RFC v2:
> - Remove some abstraction.
> - Sort includes alphabetically and add linux/of.h.
> - Depend on OF, remove of_match_ptr and add MODULE_DEVICE_TABLE.
> - Use more temporaries and break long lines.
> - Do not initialize ret in probe.
> - Use the wakeup-source DT property.
> - Rename ret to err.
> - Address Lee's comments:
> - Drop patched in presets for base regmap and related defines.
> - Use full sentences in comments.
> - Remove IRQ comment.
> - Define regmap_config member values.
> - Rename data to sys_off_data.
> - Add _PMIC suffix to Kconfig.
> - Use dev_err_probe.
> - Do not store irq_data.
> - s/WHOAMI/CHIP_ID
> - Drop LINUX part of include guard name.
> - Merge in the regulator series modifications in order to have more
> devices and modify the commit message accordingly. Changes with
> respect to the original regulator series patches:
> - ret -> err
> - Add temporary for dev in pm88x_initialize_subregmaps.
> - Drop of_compatible for the regulators.
> - Do not duplicate LDO regmap for bucks.
> - Rewrite commit message.
>
> drivers/mfd/88pm886.c | 157 ++++++++++++++++++++++++++++++++++++
> drivers/mfd/Kconfig | 12 +++
> drivers/mfd/Makefile | 1 +
> include/linux/mfd/88pm886.h | 30 +++++++
> 4 files changed, 200 insertions(+)
> create mode 100644 drivers/mfd/88pm886.c
> create mode 100644 include/linux/mfd/88pm886.h
>
> diff --git a/drivers/mfd/88pm886.c b/drivers/mfd/88pm886.c
> new file mode 100644
> index 000000000000..e06d418a5da9
> --- /dev/null
> +++ b/drivers/mfd/88pm886.c
> @@ -0,0 +1,157 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +#include <linux/i2c.h>
> +#include <linux/mfd/core.h>
> +#include <linux/module.h>
> +#include <linux/notifier.h>
> +#include <linux/of.h>
> +#include <linux/reboot.h>
> +#include <linux/regmap.h>
> +
> +#include <linux/mfd/88pm886.h>
> +
> +#define PM886_REG_INT_STATUS1 0x05
> +
> +#define PM886_REG_INT_ENA_1 0x0a
> +#define PM886_INT_ENA1_ONKEY BIT(0)
> +
> +#define PM886_IRQ_ONKEY 0
> +
> +#define PM886_REGMAP_CONF_MAX_REG 0xef

Why have you split the defines up between here and the header?

Please place them all in the header.

> +static const struct regmap_config pm886_regmap_config = {
> + .reg_bits = 8,
> + .val_bits = 8,
> + .max_register = PM886_REGMAP_CONF_MAX_REG,
> +};
> +
> +static struct regmap_irq pm886_regmap_irqs[] = {
> + REGMAP_IRQ_REG(PM886_IRQ_ONKEY, 0, PM886_INT_ENA1_ONKEY),
> +};
> +
> +static struct regmap_irq_chip pm886_regmap_irq_chip = {
> + .name = "88pm886",
> + .irqs = pm886_regmap_irqs,
> + .num_irqs = ARRAY_SIZE(pm886_regmap_irqs),
> + .num_regs = 4,
> + .status_base = PM886_REG_INT_STATUS1,
> + .ack_base = PM886_REG_INT_STATUS1,
> + .unmask_base = PM886_REG_INT_ENA_1,
> +};
> +
> +static struct resource pm886_onkey_resources[] = {
> + DEFINE_RES_IRQ_NAMED(PM886_IRQ_ONKEY, "88pm886-onkey"),
> +};
> +
> +static struct mfd_cell pm886_devs[] = {
> + MFD_CELL_RES("88pm886-onkey", pm886_onkey_resources),
> + MFD_CELL_NAME("88pm886-regulator"),
> +};
> +
> +static int pm886_power_off_handler(struct sys_off_data *sys_off_data)
> +{
> + struct pm886_chip *chip = sys_off_data->cb_data;
> + struct regmap *regmap = chip->regmap;
> + struct device *dev = &chip->client->dev;
> + int err;
> +
> + err = regmap_update_bits(regmap, PM886_REG_MISC_CONFIG1, PM886_SW_PDOWN,
> + PM886_SW_PDOWN);

Use 100-chars to avoid the '\n'.

> + if (err) {
> + dev_err(dev, "Failed to power off the device: %d\n", err);
> + return NOTIFY_BAD;
> + }
> + return NOTIFY_DONE;
> +}
> +
> +static int pm886_setup_irq(struct pm886_chip *chip,
> + struct regmap_irq_chip_data **irq_data)
> +{
> + struct regmap *regmap = chip->regmap;
> + struct device *dev = &chip->client->dev;
> + int err;
> +
> + /* Set interrupt clearing mode to clear on write. */
> + err = regmap_update_bits(regmap, PM886_REG_MISC_CONFIG2,
> + PM886_INT_INV | PM886_INT_CLEAR | PM886_INT_MASK_MODE,
> + PM886_INT_WC);
> + if (err) {
> + dev_err(dev, "Failed to set interrupt clearing mode: %d\n", err);
> + return err;
> + }
> +
> + err = devm_regmap_add_irq_chip(dev, regmap, chip->client->irq,
> + IRQF_ONESHOT, -1, &pm886_regmap_irq_chip,
> + irq_data);
> + if (err) {
> + dev_err(dev, "Failed to request IRQ: %d\n", err);
> + return err;
> + }
> +
> + return 0;
> +}
> +
> +static int pm886_probe(struct i2c_client *client)
> +{
> + struct regmap_irq_chip_data *irq_data;
> + struct device *dev = &client->dev;
> + struct pm886_chip *chip;
> + struct regmap *regmap;
> + unsigned int chip_id;
> + int err;
> +
> + chip = devm_kzalloc(dev, sizeof(*chip), GFP_KERNEL);
> + if (!chip)
> + return -ENOMEM;
> +
> + chip->client = client;
> + chip->chip_id = (uintptr_t)device_get_match_data(dev);
> + i2c_set_clientdata(client, chip);
> +
> + regmap = devm_regmap_init_i2c(client, &pm886_regmap_config);
> + if (IS_ERR(regmap))
> + return dev_err_probe(dev, PTR_ERR(regmap), "Failed to initialize regmap\n");
> + chip->regmap = regmap;
> +
> + err = regmap_read(regmap, PM886_REG_ID, &chip_id);
> + if (err)
> + return dev_err_probe(dev, err, "Failed to read chip ID\n");
> +
> + if (chip->chip_id != chip_id)
> + return dev_err_probe(dev, -EINVAL, "Unsupported chip: 0x%x\n", chip_id);
> +
> + err = pm886_setup_irq(chip, &irq_data);
> + if (err)
> + return err;
> +
> + err = devm_mfd_add_devices(dev, 0, pm886_devs, ARRAY_SIZE(pm886_devs),

Why 0?

> + NULL, 0, regmap_irq_get_domain(irq_data));
> + if (err)
> + return dev_err_probe(dev, err, "Failed to add devices\n");
> +
> + err = devm_register_power_off_handler(dev, pm886_power_off_handler, chip);
> + if (err)
> + return dev_err_probe(dev, err, "Failed to register power off handler\n");
> +
> + device_init_wakeup(dev, device_property_read_bool(dev, "wakeup-source"));
> +
> + return 0;
> +}
> +
> +static const struct of_device_id pm886_of_match[] = {
> + { .compatible = "marvell,88pm886-a1", .data = (void *)PM886_A1_CHIP_ID },
> + { }
> +};
> +MODULE_DEVICE_TABLE(of, pm886_of_match);
> +
> +static struct i2c_driver pm886_i2c_driver = {
> + .driver = {
> + .name = "88pm886",
> + .of_match_table = pm886_of_match,
> + },
> + .probe = pm886_probe,
> +};
> +module_i2c_driver(pm886_i2c_driver);
> +
> +MODULE_DESCRIPTION("Marvell 88PM886 PMIC driver");
> +MODULE_AUTHOR("Karel Balej <[email protected]>");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index 4b023ee229cf..d6a762b2bd47 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -794,6 +794,18 @@ config MFD_88PM860X
> select individual components like voltage regulators, RTC and
> battery-charger under the corresponding menus.
>
> +config MFD_88PM886_PMIC
> + bool "Marvell 88PM886 PMIC"
> + depends on I2C=y
> + depends on OF
> + select REGMAP_I2C
> + select REGMAP_IRQ
> + select MFD_CORE
> + help
> + This enables support for Marvell 88PM886 Power Management IC.
> + This includes the I2C driver and the core APIs _only_, you have to
> + select individual components like onkey under the corresponding menus.
> +
> config MFD_MAX14577
> tristate "Maxim Semiconductor MAX14577/77836 MUIC + Charger Support"
> depends on I2C
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index c66f07edcd0e..d016b7fed354 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -7,6 +7,7 @@
> obj-$(CONFIG_MFD_88PM860X) += 88pm860x.o
> obj-$(CONFIG_MFD_88PM800) += 88pm800.o 88pm80x.o
> obj-$(CONFIG_MFD_88PM805) += 88pm805.o 88pm80x.o
> +obj-$(CONFIG_MFD_88PM886_PMIC) += 88pm886.o
> obj-$(CONFIG_MFD_ACT8945A) += act8945a.o
> obj-$(CONFIG_MFD_SM501) += sm501.o
> obj-$(CONFIG_ARCH_BCM2835) += bcm2835-pm.o
> diff --git a/include/linux/mfd/88pm886.h b/include/linux/mfd/88pm886.h
> new file mode 100644
> index 000000000000..5ce30a3b85aa
> --- /dev/null
> +++ b/include/linux/mfd/88pm886.h
> @@ -0,0 +1,30 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +#ifndef __MFD_88PM886_H
> +#define __MFD_88PM886_H
> +
> +#include <linux/i2c.h>
> +#include <linux/regmap.h>
> +
> +#define PM886_A1_CHIP_ID 0xa1
> +
> +#define PM886_REG_ID 0x00
> +
> +#define PM886_REG_STATUS1 0x01
> +#define PM886_ONKEY_STS1 BIT(0)
> +
> +#define PM886_REG_MISC_CONFIG1 0x14
> +#define PM886_SW_PDOWN BIT(5)
> +
> +#define PM886_REG_MISC_CONFIG2 0x15
> +#define PM886_INT_INV BIT(0)
> +#define PM886_INT_CLEAR BIT(1)
> +#define PM886_INT_RC 0x00
> +#define PM886_INT_WC BIT(1)
> +#define PM886_INT_MASK_MODE BIT(2)
> +
> +struct pm886_chip {
> + struct i2c_client *client;
> + unsigned int chip_id;
> + struct regmap *regmap;
> +};
> +#endif /* __MFD_88PM886_H */
> --
> 2.44.0
>

--
Lee Jones [李琼斯]

2024-04-11 15:15:45

by Karel Balej

[permalink] [raw]
Subject: Re: [PATCH 2/5] mfd: add driver for Marvell 88PM886 PMIC

Lee Jones, 2024-04-11T12:37:26+01:00:
[...]
> > diff --git a/drivers/mfd/88pm886.c b/drivers/mfd/88pm886.c
> > new file mode 100644
> > index 000000000000..e06d418a5da9
> > --- /dev/null
> > +++ b/drivers/mfd/88pm886.c
> > @@ -0,0 +1,157 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +#include <linux/i2c.h>
> > +#include <linux/mfd/core.h>
> > +#include <linux/module.h>
> > +#include <linux/notifier.h>
> > +#include <linux/of.h>
> > +#include <linux/reboot.h>
> > +#include <linux/regmap.h>
> > +
> > +#include <linux/mfd/88pm886.h>
> > +
> > +#define PM886_REG_INT_STATUS1 0x05
> > +
> > +#define PM886_REG_INT_ENA_1 0x0a
> > +#define PM886_INT_ENA1_ONKEY BIT(0)
> > +
> > +#define PM886_IRQ_ONKEY 0
> > +
> > +#define PM886_REGMAP_CONF_MAX_REG 0xef
>
> Why have you split the defines up between here and the header?

I tried to keep defines tied to the code which uses them and only put
defines needed in multiple places in the header. With the exception of
closely related things, such as register bits which I am keeping
together with the respective register definitions for clarity. Does that
not make sense?

> Please place them all in the header.

Would you then also have me move all the definitions from the regulators
driver there?

[...]

> > + err = devm_mfd_add_devices(dev, 0, pm886_devs, ARRAY_SIZE(pm886_devs),
>
> Why 0?

PLATFORM_DEVID_AUTO then? Or will PLATFORM_DEVID_NONE suffice since the
cells all have different names now (it would probably cause problems
though if the driver was used multiple times for some reason, wouldn't
it?)?

Thank you,
K. B.

2024-04-11 17:56:26

by Lee Jones

[permalink] [raw]
Subject: Re: [PATCH 2/5] mfd: add driver for Marvell 88PM886 PMIC

On Thu, 11 Apr 2024, Karel Balej wrote:

> Lee Jones, 2024-04-11T12:37:26+01:00:
> [...]
> > > diff --git a/drivers/mfd/88pm886.c b/drivers/mfd/88pm886.c
> > > new file mode 100644
> > > index 000000000000..e06d418a5da9
> > > --- /dev/null
> > > +++ b/drivers/mfd/88pm886.c
> > > @@ -0,0 +1,157 @@
> > > +// SPDX-License-Identifier: GPL-2.0-only
> > > +#include <linux/i2c.h>
> > > +#include <linux/mfd/core.h>
> > > +#include <linux/module.h>
> > > +#include <linux/notifier.h>
> > > +#include <linux/of.h>
> > > +#include <linux/reboot.h>
> > > +#include <linux/regmap.h>
> > > +
> > > +#include <linux/mfd/88pm886.h>
> > > +
> > > +#define PM886_REG_INT_STATUS1 0x05
> > > +
> > > +#define PM886_REG_INT_ENA_1 0x0a
> > > +#define PM886_INT_ENA1_ONKEY BIT(0)
> > > +
> > > +#define PM886_IRQ_ONKEY 0
> > > +
> > > +#define PM886_REGMAP_CONF_MAX_REG 0xef
> >
> > Why have you split the defines up between here and the header?
>
> I tried to keep defines tied to the code which uses them and only put
> defines needed in multiple places in the header. With the exception of
> closely related things, such as register bits which I am keeping
> together with the respective register definitions for clarity. Does that
> not make sense?

It makes sense and it's a nice thought, but I think it's nicer to keep
them all together, rather than have to worry about which ones are and
which ones are not used here or there. Also, there will be holes in the
definitions, etc.

> > Please place them all in the header.
>
> Would you then also have me move all the definitions from the regulators
> driver there?

I think it would be nice to have them all nice and contiguous.

So, yes.

> [...]
>
> > > + err = devm_mfd_add_devices(dev, 0, pm886_devs, ARRAY_SIZE(pm886_devs),
> >
> > Why 0?
>
> PLATFORM_DEVID_AUTO then? Or will PLATFORM_DEVID_NONE suffice since the
> cells all have different names now (it would probably cause problems
> though if the driver was used multiple times for some reason, wouldn't
> it?)?

You tell me. Please try and understand the code you author. :)

--
Lee Jones [李琼斯]