From: Karel Balej <[email protected]>
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.
---
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 | 92 ++++++++
drivers/input/misc/Kconfig | 7 +
drivers/input/misc/Makefile | 1 +
drivers/mfd/88pm886.c | 210 ++++++++++++++++++
drivers/mfd/Kconfig | 12 +
drivers/mfd/Makefile | 1 +
drivers/regulator/88pm886-regulator.c | 195 ++++++++++++++++
drivers/regulator/Kconfig | 6 +
drivers/regulator/Makefile | 1 +
include/linux/mfd/88pm886.h | 46 ++++
12 files changed, 656 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
From: Karel Balej <[email protected]>
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.
Only implement basic support to allow for the use of regulators and
onkey omitting the currently unused register definitions and I2C
subclients which should thus be added with the subdevice drivers which
need them.
Signed-off-by: Karel Balej <[email protected]>
---
Notes:
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 | 210 ++++++++++++++++++++++++++++++++++++
drivers/mfd/Kconfig | 12 +++
drivers/mfd/Makefile | 1 +
include/linux/mfd/88pm886.h | 46 ++++++++
4 files changed, 269 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..c17220e1b7e2
--- /dev/null
+++ b/drivers/mfd/88pm886.c
@@ -0,0 +1,210 @@
+// 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_REGMAP_CONF_REG_BITS 8
+#define PM886_REGMAP_CONF_VAL_BITS 8
+#define PM886_REGMAP_CONF_MAX_REG 0xfe
+
+enum pm886_irq_number {
+ PM886_IRQ_ONKEY,
+
+ PM886_MAX_IRQ
+};
+
+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[] = {
+ {
+ .name = "88pm886-onkey",
+ .num_resources = ARRAY_SIZE(pm886_onkey_resources),
+ .resources = pm886_onkey_resources,
+ },
+ {
+ .name = "88pm886-regulator",
+ .id = PM886_REGULATOR_ID_LDO2,
+ },
+ {
+ .name = "88pm886-regulator",
+ .id = PM886_REGULATOR_ID_LDO15,
+ },
+ {
+ .name = "88pm886-regulator",
+ .id = PM886_REGULATOR_ID_BUCK2,
+ },
+};
+
+static const struct regmap_config pm886_i2c_regmap = {
+ .reg_bits = PM886_REGMAP_CONF_REG_BITS,
+ .val_bits = PM886_REGMAP_CONF_VAL_BITS,
+ .max_register = PM886_REGMAP_CONF_MAX_REG,
+};
+
+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->regmaps[PM886_REGMAP_BASE];
+ 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_initialize_subregmaps(struct pm886_chip *chip)
+{
+ struct device *dev = &chip->client->dev;
+ struct i2c_client *page;
+ struct regmap *regmap;
+ int err;
+
+ /* regulators page */
+ page = devm_i2c_new_dummy_device(dev, chip->client->adapter,
+ chip->client->addr + PM886_PAGE_OFFSET_REGULATORS);
+ if (IS_ERR(page)) {
+ err = PTR_ERR(page);
+ dev_err(dev, "Failed to initialize regulators client: %d\n", err);
+ return err;
+ }
+ regmap = devm_regmap_init_i2c(page, &pm886_i2c_regmap);
+ if (IS_ERR(regmap)) {
+ err = PTR_ERR(regmap);
+ dev_err(dev, "Failed to initialize regulators regmap: %d\n", err);
+ return err;
+ }
+ chip->regmaps[PM886_REGMAP_REGULATORS] = regmap;
+
+ return 0;
+}
+
+static int pm886_setup_irq(struct pm886_chip *chip,
+ struct regmap_irq_chip_data **irq_data)
+{
+ struct regmap *regmap = chip->regmaps[PM886_REGMAP_BASE];
+ 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->whoami = (uintptr_t)device_get_match_data(dev);
+ i2c_set_clientdata(client, chip);
+
+ regmap = devm_regmap_init_i2c(client, &pm886_i2c_regmap);
+ if (IS_ERR(regmap)) {
+ err = PTR_ERR(regmap);
+ return dev_err_probe(dev, err, "Failed to initialize regmap\n");
+ }
+ chip->regmaps[PM886_REGMAP_BASE] = 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->whoami != chip_id)
+ return dev_err_probe(dev, -EINVAL, "Device reported wrong chip ID: %u\n",
+ chip_id);
+
+ err = pm886_initialize_subregmaps(chip);
+ if (err)
+ return err;
+
+ 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;
+}
+
+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 e7a6e45b9fac..9ef921c59f30 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..c7527bab0fba
--- /dev/null
+++ b/include/linux/mfd/88pm886.h
@@ -0,0 +1,46 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+#ifndef __MFD_88PM886_H
+#define __MFD_88PM886_H
+
+#include <linux/mfd/core.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)
+
+#define PM886_PAGE_OFFSET_REGULATORS 1
+
+enum pm886_regulator_id {
+ PM886_REGULATOR_ID_LDO2,
+ PM886_REGULATOR_ID_LDO15,
+ PM886_REGULATOR_ID_BUCK2,
+
+ PM886_REGULATOR_ID_SENTINEL
+};
+
+enum pm886_regmap_index {
+ PM886_REGMAP_BASE,
+ PM886_REGMAP_REGULATORS,
+
+ PM886_REGMAP_NR
+};
+
+struct pm886_chip {
+ struct i2c_client *client;
+ unsigned int whoami;
+ struct regmap *regmaps[PM886_REGMAP_NR];
+};
+#endif /* __MFD_88PM886_H */
--
2.44.0
From: Karel Balej <[email protected]>
Support the LDO and buck regulators of the Marvell 88PM886 PMIC.
Signed-off-by: Karel Balej <[email protected]>
---
Notes:
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 | 195 ++++++++++++++++++++++++++
drivers/regulator/Kconfig | 6 +
drivers/regulator/Makefile | 1 +
3 files changed, 202 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..73f6ce413dc3
--- /dev/null
+++ b/drivers/regulator/88pm886-regulator.c
@@ -0,0 +1,195 @@
+// SPDX-License-Identifier: GPL-2.0-only
+#include <linux/kernel.h>
+#include <linux/linear_range.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/regmap.h>
+#include <linux/regulator/driver.h>
+#include <linux/regulator/of_regulator.h>
+
+#include <linux/mfd/88pm886.h>
+
+#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
+
+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[] = {
+ [PM886_REGULATOR_ID_LDO2] = {
+ .desc = {
+ .name = "LDO2",
+ .id = PM886_REGULATOR_ID_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,
+ },
+ [PM886_REGULATOR_ID_LDO15] = {
+ .desc = {
+ .name = "LDO15",
+ .id = PM886_REGULATOR_ID_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,
+ },
+ [PM886_REGULATOR_ID_BUCK2] = {
+ .desc = {
+ .name = "buck2",
+ .id = PM886_REGULATOR_ID_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,
+ },
+};
+
+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;
+
+ if (pdev->id < 0 || pdev->id >= PM886_REGULATOR_ID_SENTINEL)
+ return dev_err_probe(dev, -EINVAL, "Invalid regulator ID: %d\n",
+ pdev->id);
+
+ rcfg.dev = dev->parent;
+ regulator = &pm886_regulators[pdev->id];
+ rdesc = ®ulator->desc;
+ rcfg.driver_data = regulator;
+ rcfg.regmap = chip->regmaps[PM886_REGMAP_REGULATORS];
+ rdev = devm_regulator_register(dev, rdesc, &rcfg);
+ if (IS_ERR(rdev))
+ return dev_err_probe(dev, PTR_ERR(rdev), "Failed to register %s",
+ rdesc->name);
+
+ return 0;
+}
+
+static struct platform_driver pm886_regulator_driver = {
+ .driver = {
+ .name = "88pm886-regulator",
+ },
+ .probe = pm886_regulator_probe,
+};
+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 550145f82726..e8f504d4b9f6 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
From: Karel Balej <[email protected]>
Marvell 88PM886 is a PMIC with several subdevices such as onkey,
regulators or battery and charger. It comes in at least two revisions,
A0 and A1 -- only A1 is described here at the moment.
Signed-off-by: Karel Balej <[email protected]>
---
Notes:
RFC v3:
- Add wakeup-source property.
- Address Rob's feedback:
- Move regulators into the MFD file.
- Remove interrupt-controller and #interrupt-cells properties.
RFC v2:
- Address Rob's feedback:
- Drop mention of 88PM880.
- Make sure the file passes bindings check (add the necessary header
and fix `interrupt-cells`).
- Other small changes.
- Add regulators. Changes with respect to the regulator RFC series:
- Address Krzysztof's comments:
- Drop unused compatible.
- Fix sub-node pattern.
.../bindings/mfd/marvell,88pm886-a1.yaml | 76 +++++++++++++++++++
1 file changed, 76 insertions(+)
create mode 100644 Documentation/devicetree/bindings/mfd/marvell,88pm886-a1.yaml
diff --git a/Documentation/devicetree/bindings/mfd/marvell,88pm886-a1.yaml b/Documentation/devicetree/bindings/mfd/marvell,88pm886-a1.yaml
new file mode 100644
index 000000000000..61ffbf669e90
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/marvell,88pm886-a1.yaml
@@ -0,0 +1,76 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/mfd/marvell,88pm886-a1.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Marvell 88PM886 PMIC core
+
+maintainers:
+ - Karel Balej <[email protected]>
+
+description:
+ Marvell 88PM886 is a PMIC providing several functions such as onkey,
+ regulators or battery and charger.
+
+properties:
+ compatible:
+ const: marvell,88pm886-a1
+
+ reg:
+ maxItems: 1
+
+ interrupts:
+ maxItems: 1
+
+ wakeup-source: true
+
+ regulators:
+ type: object
+ additionalProperties: false
+ patternProperties:
+ "^(ldo(1[0-6]|[1-9])|buck[1-5])$":
+ type: object
+ $ref: /schemas/regulator/regulator.yaml#
+ description: LDO or buck regulator.
+ unevaluatedProperties: false
+
+required:
+ - compatible
+ - reg
+ - interrupts
+
+additionalProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/interrupt-controller/irq.h>
+ i2c {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ pmic@30 {
+ compatible = "marvell,88pm886-a1";
+ reg = <0x30>;
+ interrupts = <0 4 IRQ_TYPE_LEVEL_HIGH>;
+ interrupt-parent = <&gic>;
+ wakeup-source;
+
+ regulators {
+ ldo2: ldo2 {
+ regulator-min-microvolt = <3100000>;
+ regulator-max-microvolt = <3300000>;
+ };
+
+ ldo15: ldo15 {
+ regulator-min-microvolt = <3300000>;
+ regulator-max-microvolt = <3300000>;
+ };
+
+ buck2: buck2 {
+ regulator-min-microvolt = <1800000>;
+ regulator-max-microvolt = <1800000>;
+ };
+ };
+ };
+ };
+...
--
2.44.0
From: Karel Balej <[email protected]>
Marvell 88PM886 PMIC provides onkey among other things. Add client
driver to handle it. The driver currently only provides a basic support
omitting additional functions found in the vendor version, such as long
onkey and GPIO integration.
Signed-off-by: Karel Balej <[email protected]>
---
Notes:
RFC v3:
- Drop wakeup-source.
RFC v2:
- Address Dmitry's feedback:
- Sort includes alphabetically.
- Drop onkey->irq.
- ret -> err in irq_handler and no initialization.
- Break long lines and other formatting.
- Do not clobber platform_get_irq error.
- Do not set device parent manually.
- Use input_set_capability.
- Use the wakeup-source DT property.
- Drop of_match_table.
- Use more temporaries.
- Use dev_err_probe.
- Modify Kconfig description.
drivers/input/misc/88pm886-onkey.c | 92 ++++++++++++++++++++++++++++++
drivers/input/misc/Kconfig | 7 +++
drivers/input/misc/Makefile | 1 +
3 files changed, 100 insertions(+)
create mode 100644 drivers/input/misc/88pm886-onkey.c
diff --git a/drivers/input/misc/88pm886-onkey.c b/drivers/input/misc/88pm886-onkey.c
new file mode 100644
index 000000000000..2e5df21cd11b
--- /dev/null
+++ b/drivers/input/misc/88pm886-onkey.c
@@ -0,0 +1,92 @@
+// SPDX-License-Identifier: GPL-2.0-only
+#include <linux/input.h>
+#include <linux/interrupt.h>
+#include <linux/irq.h>
+#include <linux/kernel.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+
+#include <linux/mfd/88pm886.h>
+
+struct pm886_onkey {
+ struct input_dev *idev;
+ struct pm886_chip *chip;
+};
+
+static irqreturn_t pm886_onkey_irq_handler(int irq, void *data)
+{
+ struct pm886_onkey *onkey = data;
+ struct regmap *regmap = onkey->chip->regmaps[PM886_REGMAP_BASE];
+ struct input_dev *idev = onkey->idev;
+ struct device *parent = idev->dev.parent;
+ unsigned int val;
+ int err;
+
+ err = regmap_read(regmap, PM886_REG_STATUS1, &val);
+ if (err) {
+ dev_err(parent, "Failed to read status: %d\n", err);
+ return IRQ_NONE;
+ }
+ val &= PM886_ONKEY_STS1;
+
+ input_report_key(idev, KEY_POWER, val);
+ input_sync(idev);
+
+ return IRQ_HANDLED;
+}
+
+static int pm886_onkey_probe(struct platform_device *pdev)
+{
+ struct pm886_chip *chip = dev_get_drvdata(pdev->dev.parent);
+ struct device *dev = &pdev->dev;
+ struct pm886_onkey *onkey;
+ struct input_dev *idev;
+ int irq, err;
+
+ onkey = devm_kzalloc(dev, sizeof(*onkey), GFP_KERNEL);
+ if (!onkey)
+ return -ENOMEM;
+
+ onkey->chip = chip;
+
+ irq = platform_get_irq(pdev, 0);
+ if (irq < 0)
+ return dev_err_probe(dev, irq, "Failed to get IRQ\n");
+
+ idev = devm_input_allocate_device(dev);
+ if (!idev) {
+ dev_err(dev, "Failed to allocate input device\n");
+ return -ENOMEM;
+ }
+ onkey->idev = idev;
+
+ idev->name = "88pm886-onkey";
+ idev->phys = "88pm886-onkey/input0";
+ idev->id.bustype = BUS_I2C;
+
+ input_set_capability(idev, EV_KEY, KEY_POWER);
+
+ err = devm_request_threaded_irq(dev, irq, NULL, pm886_onkey_irq_handler,
+ IRQF_ONESHOT | IRQF_NO_SUSPEND, "onkey",
+ onkey);
+ if (err)
+ return dev_err_probe(dev, err, "Failed to request IRQ\n");
+
+ err = input_register_device(idev);
+ if (err)
+ return dev_err_probe(dev, err, "Failed to register input device\n");
+
+ return 0;
+}
+
+static struct platform_driver pm886_onkey_driver = {
+ .driver = {
+ .name = "88pm886-onkey",
+ },
+ .probe = pm886_onkey_probe,
+};
+module_platform_driver(pm886_onkey_driver);
+
+MODULE_DESCRIPTION("Marvell 88PM886 onkey driver");
+MODULE_AUTHOR("Karel Balej <[email protected]>");
+MODULE_LICENSE("GPL");
diff --git a/drivers/input/misc/Kconfig b/drivers/input/misc/Kconfig
index 6ba984d7f0b1..16a079d9f0f2 100644
--- a/drivers/input/misc/Kconfig
+++ b/drivers/input/misc/Kconfig
@@ -33,6 +33,13 @@ config INPUT_88PM80X_ONKEY
To compile this driver as a module, choose M here: the module
will be called 88pm80x_onkey.
+config INPUT_88PM886_ONKEY
+ tristate "Marvell 88PM886 onkey support"
+ depends on MFD_88PM886_PMIC
+ help
+ Support the onkey of Marvell 88PM886 PMIC as an input device
+ reporting power button status.
+
config INPUT_AB8500_PONKEY
tristate "AB8500 Pon (PowerOn) Key"
depends on AB8500_CORE
diff --git a/drivers/input/misc/Makefile b/drivers/input/misc/Makefile
index 04296a4abe8e..054a6dc1ac27 100644
--- a/drivers/input/misc/Makefile
+++ b/drivers/input/misc/Makefile
@@ -7,6 +7,7 @@
obj-$(CONFIG_INPUT_88PM860X_ONKEY) += 88pm860x_onkey.o
obj-$(CONFIG_INPUT_88PM80X_ONKEY) += 88pm80x_onkey.o
+obj-$(CONFIG_INPUT_88PM886_ONKEY) += 88pm886-onkey.o
obj-$(CONFIG_INPUT_AB8500_PONKEY) += ab8500-ponkey.o
obj-$(CONFIG_INPUT_AD714X) += ad714x.o
obj-$(CONFIG_INPUT_AD714X_I2C) += ad714x-i2c.o
--
2.44.0
From: Karel Balej <[email protected]>
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 960512bec428..944f88c92df6 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -12949,6 +12949,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
Hi Karel,
On Sun, Mar 03, 2024 at 11:04:25AM +0100, Karel Balej wrote:
> From: Karel Balej <[email protected]>
>
> Marvell 88PM886 PMIC provides onkey among other things. Add client
> driver to handle it. The driver currently only provides a basic support
> omitting additional functions found in the vendor version, such as long
> onkey and GPIO integration.
>
> Signed-off-by: Karel Balej <[email protected]>
> ---
>
> Notes:
> RFC v3:
> - Drop wakeup-source.
> RFC v2:
> - Address Dmitry's feedback:
> - Sort includes alphabetically.
> - Drop onkey->irq.
> - ret -> err in irq_handler and no initialization.
> - Break long lines and other formatting.
> - Do not clobber platform_get_irq error.
> - Do not set device parent manually.
> - Use input_set_capability.
> - Use the wakeup-source DT property.
> - Drop of_match_table.
I only said that you should not be using of_match_ptr(), but you still
need to have of_match_table set and have MODULE_DEVICE_TABLE() for the
proper module loading support.
With that fixed:
Acked-by: Dmitry Torokhov <[email protected]>
Thanks.
--
Dmitry
On 03/03/2024 11:04, Karel Balej wrote:
> From: Karel Balej <[email protected]>
>
> +examples:
> + - |
> + #include <dt-bindings/interrupt-controller/irq.h>
> + i2c {
> + #address-cells = <1>;
> + #size-cells = <0>;
> + pmic@30 {
> + compatible = "marvell,88pm886-a1";
> + reg = <0x30>;
> + interrupts = <0 4 IRQ_TYPE_LEVEL_HIGH>;
> + interrupt-parent = <&gic>;
> + wakeup-source;
> +
> + regulators {
> + ldo2: ldo2 {
> + regulator-min-microvolt = <3100000>;
> + regulator-max-microvolt = <3300000>;
> + };
Messed indentation here and in following lines..
Reviewed-by: Krzysztof Kozlowski <[email protected]>
Best regards,
Krzysztof
Dmitry,
Dmitry Torokhov, 2024-03-03T12:39:46-08:00:
> On Sun, Mar 03, 2024 at 11:04:25AM +0100, Karel Balej wrote:
> > From: Karel Balej <[email protected]>
> >
> > Marvell 88PM886 PMIC provides onkey among other things. Add client
> > driver to handle it. The driver currently only provides a basic support
> > omitting additional functions found in the vendor version, such as long
> > onkey and GPIO integration.
> >
> > Signed-off-by: Karel Balej <[email protected]>
> > ---
> >
> > Notes:
> > RFC v3:
> > - Drop wakeup-source.
> > RFC v2:
> > - Address Dmitry's feedback:
> > - Sort includes alphabetically.
> > - Drop onkey->irq.
> > - ret -> err in irq_handler and no initialization.
> > - Break long lines and other formatting.
> > - Do not clobber platform_get_irq error.
> > - Do not set device parent manually.
> > - Use input_set_capability.
> > - Use the wakeup-source DT property.
> > - Drop of_match_table.
>
> I only said that you should not be using of_match_ptr(), but you still
> need to have of_match_table set and have MODULE_DEVICE_TABLE() for the
> proper module loading support.
I removed of_match_table because I no longer need compatible for this --
there are no device tree properties and the driver is being instantiated
by the MFD driver.
Is the MODULE_DEVICE_TABLE() entry needed for the driver to probe when
compiled as module? If that is the case, given what I write above, am I
correct that MODULE_DEVICE_TABLE(platform,...) would be the right thing
to use here?
Thank you, kind regards,
K. B.
On Mon, Mar 04, 2024 at 09:28:45PM +0100, Karel Balej wrote:
> Dmitry,
>
> Dmitry Torokhov, 2024-03-03T12:39:46-08:00:
> > On Sun, Mar 03, 2024 at 11:04:25AM +0100, Karel Balej wrote:
> > > From: Karel Balej <[email protected]>
> > >
> > > Marvell 88PM886 PMIC provides onkey among other things. Add client
> > > driver to handle it. The driver currently only provides a basic support
> > > omitting additional functions found in the vendor version, such as long
> > > onkey and GPIO integration.
> > >
> > > Signed-off-by: Karel Balej <[email protected]>
> > > ---
> > >
> > > Notes:
> > > RFC v3:
> > > - Drop wakeup-source.
> > > RFC v2:
> > > - Address Dmitry's feedback:
> > > - Sort includes alphabetically.
> > > - Drop onkey->irq.
> > > - ret -> err in irq_handler and no initialization.
> > > - Break long lines and other formatting.
> > > - Do not clobber platform_get_irq error.
> > > - Do not set device parent manually.
> > > - Use input_set_capability.
> > > - Use the wakeup-source DT property.
> > > - Drop of_match_table.
> >
> > I only said that you should not be using of_match_ptr(), but you still
> > need to have of_match_table set and have MODULE_DEVICE_TABLE() for the
> > proper module loading support.
>
> I removed of_match_table because I no longer need compatible for this --
> there are no device tree properties and the driver is being instantiated
> by the MFD driver.
>
> Is the MODULE_DEVICE_TABLE() entry needed for the driver to probe when
> compiled as module? If that is the case, given what I write above, am I
> correct that MODULE_DEVICE_TABLE(platform,...) would be the right thing
> to use here?
Yes, if uevent generated for the device is "platform:<name>" then
MODULE_DEVICE_TABLE(platform,...) will suffice. I am not sure how MFD
sets it up (OF modalias or platform), but you should be able to check
the format looking at the "uevent" attribute for your device in sysfs
(/sys/devices/bus/platform/...).
Thanks.
--
Dmitry
On Sun, 03 Mar 2024, Karel Balej wrote:
> From: Karel Balej <[email protected]>
>
> 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.
>
> Only implement basic support to allow for the use of regulators and
> onkey omitting the currently unused register definitions and I2C
> subclients which should thus be added with the subdevice drivers which
> need them.
>
> Signed-off-by: Karel Balej <[email protected]>
> ---
>
> Notes:
> 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
I still see 'whoami'.
> - 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 | 210 ++++++++++++++++++++++++++++++++++++
> drivers/mfd/Kconfig | 12 +++
> drivers/mfd/Makefile | 1 +
> include/linux/mfd/88pm886.h | 46 ++++++++
> 4 files changed, 269 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..c17220e1b7e2
> --- /dev/null
> +++ b/drivers/mfd/88pm886.c
> @@ -0,0 +1,210 @@
> +// SPDX-License-Identifier: GPL-2.0-only
Header? Description? Author? Copyright?
> +#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_REGMAP_CONF_REG_BITS 8
> +#define PM886_REGMAP_CONF_VAL_BITS 8
These 2 are not commonly defined, just the one below please.
> +#define PM886_REGMAP_CONF_MAX_REG 0xfe
> +enum pm886_irq_number {
> + PM886_IRQ_ONKEY,
> +
> + PM886_MAX_IRQ
> +};
Seems superfluous for 1 IRQ and an unused MAX.
Looks like I've mentioned this before.
The IRQ number probably shouldn't be arbitrary either.
> +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[] = {
> + {
> + .name = "88pm886-onkey",
> + .num_resources = ARRAY_SIZE(pm886_onkey_resources),
> + .resources = pm886_onkey_resources,
> + },
> + {
> + .name = "88pm886-regulator",
> + .id = PM886_REGULATOR_ID_LDO2,
Why doesn't PLATFORM_DEVID_AUTO work for this device?
> + },
> + {
> + .name = "88pm886-regulator",
> + .id = PM886_REGULATOR_ID_LDO15,
> + },
> + {
> + .name = "88pm886-regulator",
> + .id = PM886_REGULATOR_ID_BUCK2,
> + },
> +};
> +
> +static const struct regmap_config pm886_i2c_regmap = {
> + .reg_bits = PM886_REGMAP_CONF_REG_BITS,
> + .val_bits = PM886_REGMAP_CONF_VAL_BITS,
> + .max_register = PM886_REGMAP_CONF_MAX_REG,
> +};
> +
/> +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->regmaps[PM886_REGMAP_BASE];
> + 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_initialize_subregmaps(struct pm886_chip *chip)
> +{
> + struct device *dev = &chip->client->dev;
> + struct i2c_client *page;
> + struct regmap *regmap;
> + int err;
> +
> + /* regulators page */
> + page = devm_i2c_new_dummy_device(dev, chip->client->adapter,
> + chip->client->addr + PM886_PAGE_OFFSET_REGULATORS);
> + if (IS_ERR(page)) {
> + err = PTR_ERR(page);
> + dev_err(dev, "Failed to initialize regulators client: %d\n", err);
> + return err;
> + }
> + regmap = devm_regmap_init_i2c(page, &pm886_i2c_regmap);
> + if (IS_ERR(regmap)) {
> + err = PTR_ERR(regmap);
> + dev_err(dev, "Failed to initialize regulators regmap: %d\n", err);
> + return err;
> + }
> + chip->regmaps[PM886_REGMAP_REGULATORS] = regmap;
Except for the regulator driver, where else is the regulators regmap used?
> +
> + return 0;
> +}
> +
> +static int pm886_setup_irq(struct pm886_chip *chip,
> + struct regmap_irq_chip_data **irq_data)
> +{
> + struct regmap *regmap = chip->regmaps[PM886_REGMAP_BASE];
> + 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->whoami = (uintptr_t)device_get_match_data(dev);
> + i2c_set_clientdata(client, chip);
> +
> + regmap = devm_regmap_init_i2c(client, &pm886_i2c_regmap);
> + if (IS_ERR(regmap)) {
> + err = PTR_ERR(regmap);
> + return dev_err_probe(dev, err, "Failed to initialize regmap\n");
> + }
> + chip->regmaps[PM886_REGMAP_BASE] = regmap;
> +
> + err = regmap_read(regmap, PM886_REG_ID, &chip_id);
> + if (err)
> + return dev_err_probe(dev, err, "Failed to read chip ID\n");
'\n'
> + if (chip->whoami != chip_id)
> + return dev_err_probe(dev, -EINVAL, "Device reported wrong chip ID: %u\n",
"Unsupported chip"
> + chip_id);
How many chars does this take up if on one line?
Nit: If you have to break, break after the "-EINVAL" param.
> +
> + err = pm886_initialize_subregmaps(chip);
> + if (err)
> + return err;
> +
> + 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;
> +}
> +
> +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 e7a6e45b9fac..9ef921c59f30 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..c7527bab0fba
> --- /dev/null
> +++ b/include/linux/mfd/88pm886.h
> @@ -0,0 +1,46 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +#ifndef __MFD_88PM886_H
> +#define __MFD_88PM886_H
> +
> +#include <linux/mfd/core.h>
What's this for?
> +#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)
> +
> +#define PM886_PAGE_OFFSET_REGULATORS 1
> +
> +enum pm886_regulator_id {
> + PM886_REGULATOR_ID_LDO2,
> + PM886_REGULATOR_ID_LDO15,
> + PM886_REGULATOR_ID_BUCK2,
> +
> + PM886_REGULATOR_ID_SENTINEL
> +};
> +
> +enum pm886_regmap_index {
> + PM886_REGMAP_BASE,
> + PM886_REGMAP_REGULATORS,
> +
> + PM886_REGMAP_NR
> +};
> +
> +struct pm886_chip {
> + struct i2c_client *client;
> + unsigned int whoami;
chip_id
> + struct regmap *regmaps[PM886_REGMAP_NR];
> +};
> +#endif /* __MFD_88PM886_H */
> --
> 2.44.0
>
--
Lee Jones [李琼斯]
Lee Jones, 2024-03-05T11:44:18+00:00:
> > +static struct mfd_cell pm886_devs[] = {
> > + {
> > + .name = "88pm886-onkey",
> > + .num_resources = ARRAY_SIZE(pm886_onkey_resources),
> > + .resources = pm886_onkey_resources,
> > + },
> > + {
> > + .name = "88pm886-regulator",
> > + .id = PM886_REGULATOR_ID_LDO2,
>
> Why doesn't PLATFORM_DEVID_AUTO work for this device?
Because I am using the IDs in the regulator driver to determine which
regulator data to use/which regulator to register.
> > +static int pm886_initialize_subregmaps(struct pm886_chip *chip)
> > +{
> > + struct device *dev = &chip->client->dev;
> > + struct i2c_client *page;
> > + struct regmap *regmap;
> > + int err;
> > +
> > + /* regulators page */
> > + page = devm_i2c_new_dummy_device(dev, chip->client->adapter,
> > + chip->client->addr + PM886_PAGE_OFFSET_REGULATORS);
> > + if (IS_ERR(page)) {
> > + err = PTR_ERR(page);
> > + dev_err(dev, "Failed to initialize regulators client: %d\n", err);
> > + return err;
> > + }
> > + regmap = devm_regmap_init_i2c(page, &pm886_i2c_regmap);
> > + if (IS_ERR(regmap)) {
> > + err = PTR_ERR(regmap);
> > + dev_err(dev, "Failed to initialize regulators regmap: %d\n", err);
> > + return err;
> > + }
> > + chip->regmaps[PM886_REGMAP_REGULATORS] = regmap;
>
> Except for the regulator driver, where else is the regulators regmap used?
Nowhere, at least as of now. So you are saying that I should initialize
the regmap in the regulator driver?
Thank you,
K. B.
> > > +static int pm886_initialize_subregmaps(struct pm886_chip *chip)
> > > +{
> > > + struct device *dev = &chip->client->dev;
> > > + struct i2c_client *page;
> > > + struct regmap *regmap;
> > > + int err;
> > > +
> > > + /* regulators page */
> > > + page = devm_i2c_new_dummy_device(dev, chip->client->adapter,
> > > + chip->client->addr + PM886_PAGE_OFFSET_REGULATORS);
> > > + if (IS_ERR(page)) {
> > > + err = PTR_ERR(page);
> > > + dev_err(dev, "Failed to initialize regulators client: %d\n", err);
> > > + return err;
> > > + }
> > > + regmap = devm_regmap_init_i2c(page, &pm886_i2c_regmap);
> > > + if (IS_ERR(regmap)) {
> > > + err = PTR_ERR(regmap);
> > > + dev_err(dev, "Failed to initialize regulators regmap: %d\n", err);
> > > + return err;
> > > + }
> > > + chip->regmaps[PM886_REGMAP_REGULATORS] = regmap;
> >
> > Except for the regulator driver, where else is the regulators regmap used?
>
> Nowhere, at least as of now. So you are saying that I should initialize
> the regmap in the regulator driver?
I am.
--
Lee Jones [李琼斯]
Dmitry Torokhov, 2024-03-04T17:10:59-08:00:
> On Mon, Mar 04, 2024 at 09:28:45PM +0100, Karel Balej wrote:
> > Dmitry,
> >
> > Dmitry Torokhov, 2024-03-03T12:39:46-08:00:
> > > On Sun, Mar 03, 2024 at 11:04:25AM +0100, Karel Balej wrote:
> > > > From: Karel Balej <[email protected]>
> > > >
> > > > Marvell 88PM886 PMIC provides onkey among other things. Add client
> > > > driver to handle it. The driver currently only provides a basic support
> > > > omitting additional functions found in the vendor version, such as long
> > > > onkey and GPIO integration.
> > > >
> > > > Signed-off-by: Karel Balej <[email protected]>
> > > > ---
> > > >
> > > > Notes:
> > > > RFC v3:
> > > > - Drop wakeup-source.
> > > > RFC v2:
> > > > - Address Dmitry's feedback:
> > > > - Sort includes alphabetically.
> > > > - Drop onkey->irq.
> > > > - ret -> err in irq_handler and no initialization.
> > > > - Break long lines and other formatting.
> > > > - Do not clobber platform_get_irq error.
> > > > - Do not set device parent manually.
> > > > - Use input_set_capability.
> > > > - Use the wakeup-source DT property.
> > > > - Drop of_match_table.
> > >
> > > I only said that you should not be using of_match_ptr(), but you still
> > > need to have of_match_table set and have MODULE_DEVICE_TABLE() for the
> > > proper module loading support.
> >
> > I removed of_match_table because I no longer need compatible for this --
> > there are no device tree properties and the driver is being instantiated
> > by the MFD driver.
> >
> > Is the MODULE_DEVICE_TABLE() entry needed for the driver to probe when
> > compiled as module? If that is the case, given what I write above, am I
> > correct that MODULE_DEVICE_TABLE(platform,...) would be the right thing
> > to use here?
>
> Yes, if uevent generated for the device is "platform:<name>" then
> MODULE_DEVICE_TABLE(platform,...) will suffice. I am not sure how MFD
> sets it up (OF modalias or platform), but you should be able to check
> the format looking at the "uevent" attribute for your device in sysfs
> (/sys/devices/bus/platform/...).
The uevent is indeed platform.
But since there is only one device, perhaps having a device table is
superfluous and using `MODULE_ALIAS("platform:88pm886-onkey")` is more
fitting?
Although I don't understand why this is even necessary when the driver
name is such and the module is registered using
`module_platform_driver`...
Thank you, best regards,
K. B.
On 10/03/2024 12:35, Karel Balej wrote:
> Dmitry Torokhov, 2024-03-04T17:10:59-08:00:
>> On Mon, Mar 04, 2024 at 09:28:45PM +0100, Karel Balej wrote:
>>> Dmitry,
>>>
>>> Dmitry Torokhov, 2024-03-03T12:39:46-08:00:
>>>> On Sun, Mar 03, 2024 at 11:04:25AM +0100, Karel Balej wrote:
>>>>> From: Karel Balej <[email protected]>
>>>>>
>>>>> Marvell 88PM886 PMIC provides onkey among other things. Add client
>>>>> driver to handle it. The driver currently only provides a basic support
>>>>> omitting additional functions found in the vendor version, such as long
>>>>> onkey and GPIO integration.
>>>>>
>>>>> Signed-off-by: Karel Balej <[email protected]>
>>>>> ---
>>>>>
>>>>> Notes:
>>>>> RFC v3:
>>>>> - Drop wakeup-source.
>>>>> RFC v2:
>>>>> - Address Dmitry's feedback:
>>>>> - Sort includes alphabetically.
>>>>> - Drop onkey->irq.
>>>>> - ret -> err in irq_handler and no initialization.
>>>>> - Break long lines and other formatting.
>>>>> - Do not clobber platform_get_irq error.
>>>>> - Do not set device parent manually.
>>>>> - Use input_set_capability.
>>>>> - Use the wakeup-source DT property.
>>>>> - Drop of_match_table.
>>>>
>>>> I only said that you should not be using of_match_ptr(), but you still
>>>> need to have of_match_table set and have MODULE_DEVICE_TABLE() for the
>>>> proper module loading support.
>>>
>>> I removed of_match_table because I no longer need compatible for this --
>>> there are no device tree properties and the driver is being instantiated
>>> by the MFD driver.
>>>
>>> Is the MODULE_DEVICE_TABLE() entry needed for the driver to probe when
>>> compiled as module? If that is the case, given what I write above, am I
>>> correct that MODULE_DEVICE_TABLE(platform,...) would be the right thing
>>> to use here?
>>
>> Yes, if uevent generated for the device is "platform:<name>" then
>> MODULE_DEVICE_TABLE(platform,...) will suffice. I am not sure how MFD
>> sets it up (OF modalias or platform), but you should be able to check
>> the format looking at the "uevent" attribute for your device in sysfs
>> (/sys/devices/bus/platform/...).
>
> The uevent is indeed platform.
>
> But since there is only one device, perhaps having a device table is
> superfluous and using `MODULE_ALIAS("platform:88pm886-onkey")` is more
> fitting?
Adding aliases for standard IDs and standard cases is almost never
correct. If you need module alias, it means your ID table is wrong (or
missing, which is usually wrong).
>
> Although I don't understand why this is even necessary when the driver
> name is such and the module is registered using
> `module_platform_driver`...
ID table and MODULE_DEVICE_TABLE() are necessary for modprobe to work.
Just run `modinfo`.
Best regards,
Krzysztof
On Sun, Mar 10, 2024 at 09:35:36PM +0100, Krzysztof Kozlowski wrote:
> On 10/03/2024 12:35, Karel Balej wrote:
> > Dmitry Torokhov, 2024-03-04T17:10:59-08:00:
> >> On Mon, Mar 04, 2024 at 09:28:45PM +0100, Karel Balej wrote:
> >>> Dmitry,
> >>>
> >>> Dmitry Torokhov, 2024-03-03T12:39:46-08:00:
> >>>> On Sun, Mar 03, 2024 at 11:04:25AM +0100, Karel Balej wrote:
> >>>>> From: Karel Balej <[email protected]>
> >>>>>
> >>>>> Marvell 88PM886 PMIC provides onkey among other things. Add client
> >>>>> driver to handle it. The driver currently only provides a basic support
> >>>>> omitting additional functions found in the vendor version, such as long
> >>>>> onkey and GPIO integration.
> >>>>>
> >>>>> Signed-off-by: Karel Balej <[email protected]>
> >>>>> ---
> >>>>>
> >>>>> Notes:
> >>>>> RFC v3:
> >>>>> - Drop wakeup-source.
> >>>>> RFC v2:
> >>>>> - Address Dmitry's feedback:
> >>>>> - Sort includes alphabetically.
> >>>>> - Drop onkey->irq.
> >>>>> - ret -> err in irq_handler and no initialization.
> >>>>> - Break long lines and other formatting.
> >>>>> - Do not clobber platform_get_irq error.
> >>>>> - Do not set device parent manually.
> >>>>> - Use input_set_capability.
> >>>>> - Use the wakeup-source DT property.
> >>>>> - Drop of_match_table.
> >>>>
> >>>> I only said that you should not be using of_match_ptr(), but you still
> >>>> need to have of_match_table set and have MODULE_DEVICE_TABLE() for the
> >>>> proper module loading support.
> >>>
> >>> I removed of_match_table because I no longer need compatible for this --
> >>> there are no device tree properties and the driver is being instantiated
> >>> by the MFD driver.
> >>>
> >>> Is the MODULE_DEVICE_TABLE() entry needed for the driver to probe when
> >>> compiled as module? If that is the case, given what I write above, am I
> >>> correct that MODULE_DEVICE_TABLE(platform,...) would be the right thing
> >>> to use here?
> >>
> >> Yes, if uevent generated for the device is "platform:<name>" then
> >> MODULE_DEVICE_TABLE(platform,...) will suffice. I am not sure how MFD
> >> sets it up (OF modalias or platform), but you should be able to check
> >> the format looking at the "uevent" attribute for your device in sysfs
> >> (/sys/devices/bus/platform/...).
> >
> > The uevent is indeed platform.
> >
> > But since there is only one device, perhaps having a device table is
> > superfluous and using `MODULE_ALIAS("platform:88pm886-onkey")` is more
> > fitting?
>
> Adding aliases for standard IDs and standard cases is almost never
> correct. If you need module alias, it means your ID table is wrong (or
> missing, which is usually wrong).
>
> >
> > Although I don't understand why this is even necessary when the driver
> > name is such and the module is registered using
> > `module_platform_driver`...
>
> ID table and MODULE_DEVICE_TABLE() are necessary for modprobe to work.
> Just run `modinfo`.
MODULE_DEVICE_TABLE() and MODULE_ALIAS() reduce to the same thing, but I
agree that we should not try to be too clever and simply use the ID
table.
Thanks.
--
Dmitry
Krzysztof Kozlowski, 2024-03-10T21:35:36+01:00:
> On 10/03/2024 12:35, Karel Balej wrote:
> > Dmitry Torokhov, 2024-03-04T17:10:59-08:00:
> >> On Mon, Mar 04, 2024 at 09:28:45PM +0100, Karel Balej wrote:
> >>> Dmitry,
> >>>
> >>> Dmitry Torokhov, 2024-03-03T12:39:46-08:00:
> >>>> On Sun, Mar 03, 2024 at 11:04:25AM +0100, Karel Balej wrote:
> >>>>> From: Karel Balej <[email protected]>
> >>>>>
> >>>>> Marvell 88PM886 PMIC provides onkey among other things. Add client
> >>>>> driver to handle it. The driver currently only provides a basic support
> >>>>> omitting additional functions found in the vendor version, such as long
> >>>>> onkey and GPIO integration.
> >>>>>
> >>>>> Signed-off-by: Karel Balej <[email protected]>
> >>>>> ---
> >>>>>
> >>>>> Notes:
> >>>>> RFC v3:
> >>>>> - Drop wakeup-source.
> >>>>> RFC v2:
> >>>>> - Address Dmitry's feedback:
> >>>>> - Sort includes alphabetically.
> >>>>> - Drop onkey->irq.
> >>>>> - ret -> err in irq_handler and no initialization.
> >>>>> - Break long lines and other formatting.
> >>>>> - Do not clobber platform_get_irq error.
> >>>>> - Do not set device parent manually.
> >>>>> - Use input_set_capability.
> >>>>> - Use the wakeup-source DT property.
> >>>>> - Drop of_match_table.
> >>>>
> >>>> I only said that you should not be using of_match_ptr(), but you still
> >>>> need to have of_match_table set and have MODULE_DEVICE_TABLE() for the
> >>>> proper module loading support.
> >>>
> >>> I removed of_match_table because I no longer need compatible for this --
> >>> there are no device tree properties and the driver is being instantiated
> >>> by the MFD driver.
> >>>
> >>> Is the MODULE_DEVICE_TABLE() entry needed for the driver to probe when
> >>> compiled as module? If that is the case, given what I write above, am I
> >>> correct that MODULE_DEVICE_TABLE(platform,...) would be the right thing
> >>> to use here?
> >>
> >> Yes, if uevent generated for the device is "platform:<name>" then
> >> MODULE_DEVICE_TABLE(platform,...) will suffice. I am not sure how MFD
> >> sets it up (OF modalias or platform), but you should be able to check
> >> the format looking at the "uevent" attribute for your device in sysfs
> >> (/sys/devices/bus/platform/...).
> >
> > The uevent is indeed platform.
> >
> > But since there is only one device, perhaps having a device table is
> > superfluous and using `MODULE_ALIAS("platform:88pm886-onkey")` is more
> > fitting?
>
> Adding aliases for standard IDs and standard cases is almost never
> correct. If you need module alias, it means your ID table is wrong (or
> missing, which is usually wrong).
>
> >
> > Although I don't understand why this is even necessary when the driver
> > name is such and the module is registered using
> > `module_platform_driver`...
>
> ID table and MODULE_DEVICE_TABLE() are necessary for modprobe to work.
I think I understand the practical reasons. My point was that I would
expect the alias to be added automatically even in the case that the
device table is absent based solely on the driver name and the
registration method (*module*_*platform*_driver). Why is that not the
case? Obviously the driver name matching the mfd_cell name is sufficient
for the driver to probe when it is built in so the name does seem to
serve as some identification for the device just as a device table entry
would.
Furthermore, drivers/input/serio/ioc3kbd.c does not seem to have an ID
table either, nor a MODULE_ALIAS -- is that a mistake? If not, what
mechanism causes the driver to probe when compiled as a module? It seems
to me to effectively be the same setup as with my driver and that does
not load automatically (because of the missing alias).
> Just run `modinfo`.
Thank you very much,
K. B.
On 11/03/2024 11:26, Karel Balej wrote:
> Krzysztof Kozlowski, 2024-03-10T21:35:36+01:00:
>> On 10/03/2024 12:35, Karel Balej wrote:
>>> Dmitry Torokhov, 2024-03-04T17:10:59-08:00:
>>>> On Mon, Mar 04, 2024 at 09:28:45PM +0100, Karel Balej wrote:
>>>>> Dmitry,
>>>>>
>>>>> Dmitry Torokhov, 2024-03-03T12:39:46-08:00:
>>>>>> On Sun, Mar 03, 2024 at 11:04:25AM +0100, Karel Balej wrote:
>>>>>>> From: Karel Balej <[email protected]>
>>>>>>>
>>>>>>> Marvell 88PM886 PMIC provides onkey among other things. Add client
>>>>>>> driver to handle it. The driver currently only provides a basic support
>>>>>>> omitting additional functions found in the vendor version, such as long
>>>>>>> onkey and GPIO integration.
>>>>>>>
>>>>>>> Signed-off-by: Karel Balej <[email protected]>
>>>>>>> ---
>>>>>>>
>>>>>>> Notes:
>>>>>>> RFC v3:
>>>>>>> - Drop wakeup-source.
>>>>>>> RFC v2:
>>>>>>> - Address Dmitry's feedback:
>>>>>>> - Sort includes alphabetically.
>>>>>>> - Drop onkey->irq.
>>>>>>> - ret -> err in irq_handler and no initialization.
>>>>>>> - Break long lines and other formatting.
>>>>>>> - Do not clobber platform_get_irq error.
>>>>>>> - Do not set device parent manually.
>>>>>>> - Use input_set_capability.
>>>>>>> - Use the wakeup-source DT property.
>>>>>>> - Drop of_match_table.
>>>>>>
>>>>>> I only said that you should not be using of_match_ptr(), but you still
>>>>>> need to have of_match_table set and have MODULE_DEVICE_TABLE() for the
>>>>>> proper module loading support.
>>>>>
>>>>> I removed of_match_table because I no longer need compatible for this --
>>>>> there are no device tree properties and the driver is being instantiated
>>>>> by the MFD driver.
>>>>>
>>>>> Is the MODULE_DEVICE_TABLE() entry needed for the driver to probe when
>>>>> compiled as module? If that is the case, given what I write above, am I
>>>>> correct that MODULE_DEVICE_TABLE(platform,...) would be the right thing
>>>>> to use here?
>>>>
>>>> Yes, if uevent generated for the device is "platform:<name>" then
>>>> MODULE_DEVICE_TABLE(platform,...) will suffice. I am not sure how MFD
>>>> sets it up (OF modalias or platform), but you should be able to check
>>>> the format looking at the "uevent" attribute for your device in sysfs
>>>> (/sys/devices/bus/platform/...).
>>>
>>> The uevent is indeed platform.
>>>
>>> But since there is only one device, perhaps having a device table is
>>> superfluous and using `MODULE_ALIAS("platform:88pm886-onkey")` is more
>>> fitting?
>>
>> Adding aliases for standard IDs and standard cases is almost never
>> correct. If you need module alias, it means your ID table is wrong (or
>> missing, which is usually wrong).
>>
>>>
>>> Although I don't understand why this is even necessary when the driver
>>> name is such and the module is registered using
>>> `module_platform_driver`...
>>
>> ID table and MODULE_DEVICE_TABLE() are necessary for modprobe to work.
>
> I think I understand the practical reasons. My point was that I would
> expect the alias to be added automatically even in the case that the
> device table is absent based solely on the driver name and the
> registration method (*module*_*platform*_driver). Why is that not the
> case? Obviously the driver name matching the mfd_cell name is sufficient
You mean add it automatically by macro-magic based on presence of
id_table and/or of_match_table?
That's a good question. I cannot find answer why not, except that maybe
no one ever wrote it...
> for the driver to probe when it is built in so the name does seem to
> serve as some identification for the device just as a device table entry
> would.
>
> Furthermore, drivers/input/serio/ioc3kbd.c does not seem to have an ID
> table either, nor a MODULE_ALIAS -- is that a mistake? If not, what
> mechanism causes the driver to probe when compiled as a module? It seems
You are now mixing two different things: probing of driver (so bind) and
module auto-loading. Probing is done also by driver name. Auto-loading,
not sure, maybe by name as well? However it is also likely that
auto-loading is broken. Several drivers had such issues in the past.
Best regards,
Krzysztof
Krzysztof Kozlowski, 2024-03-11T11:41:53+01:00:
> On 11/03/2024 11:26, Karel Balej wrote:
> > Krzysztof Kozlowski, 2024-03-10T21:35:36+01:00:
> >> On 10/03/2024 12:35, Karel Balej wrote:
> >>> Dmitry Torokhov, 2024-03-04T17:10:59-08:00:
> >>>> On Mon, Mar 04, 2024 at 09:28:45PM +0100, Karel Balej wrote:
> >>>>> Dmitry,
> >>>>>
> >>>>> Dmitry Torokhov, 2024-03-03T12:39:46-08:00:
> >>>>>> On Sun, Mar 03, 2024 at 11:04:25AM +0100, Karel Balej wrote:
> >>>>>>> From: Karel Balej <[email protected]>
> >>>>>>>
> >>>>>>> Marvell 88PM886 PMIC provides onkey among other things. Add client
> >>>>>>> driver to handle it. The driver currently only provides a basic support
> >>>>>>> omitting additional functions found in the vendor version, such as long
> >>>>>>> onkey and GPIO integration.
> >>>>>>>
> >>>>>>> Signed-off-by: Karel Balej <[email protected]>
> >>>>>>> ---
> >>>>>>>
> >>>>>>> Notes:
> >>>>>>> RFC v3:
> >>>>>>> - Drop wakeup-source.
> >>>>>>> RFC v2:
> >>>>>>> - Address Dmitry's feedback:
> >>>>>>> - Sort includes alphabetically.
> >>>>>>> - Drop onkey->irq.
> >>>>>>> - ret -> err in irq_handler and no initialization.
> >>>>>>> - Break long lines and other formatting.
> >>>>>>> - Do not clobber platform_get_irq error.
> >>>>>>> - Do not set device parent manually.
> >>>>>>> - Use input_set_capability.
> >>>>>>> - Use the wakeup-source DT property.
> >>>>>>> - Drop of_match_table.
> >>>>>>
> >>>>>> I only said that you should not be using of_match_ptr(), but you still
> >>>>>> need to have of_match_table set and have MODULE_DEVICE_TABLE() for the
> >>>>>> proper module loading support.
> >>>>>
> >>>>> I removed of_match_table because I no longer need compatible for this --
> >>>>> there are no device tree properties and the driver is being instantiated
> >>>>> by the MFD driver.
> >>>>>
> >>>>> Is the MODULE_DEVICE_TABLE() entry needed for the driver to probe when
> >>>>> compiled as module? If that is the case, given what I write above, am I
> >>>>> correct that MODULE_DEVICE_TABLE(platform,...) would be the right thing
> >>>>> to use here?
> >>>>
> >>>> Yes, if uevent generated for the device is "platform:<name>" then
> >>>> MODULE_DEVICE_TABLE(platform,...) will suffice. I am not sure how MFD
> >>>> sets it up (OF modalias or platform), but you should be able to check
> >>>> the format looking at the "uevent" attribute for your device in sysfs
> >>>> (/sys/devices/bus/platform/...).
> >>>
> >>> The uevent is indeed platform.
> >>>
> >>> But since there is only one device, perhaps having a device table is
> >>> superfluous and using `MODULE_ALIAS("platform:88pm886-onkey")` is more
> >>> fitting?
> >>
> >> Adding aliases for standard IDs and standard cases is almost never
> >> correct. If you need module alias, it means your ID table is wrong (or
> >> missing, which is usually wrong).
> >>
> >>>
> >>> Although I don't understand why this is even necessary when the driver
> >>> name is such and the module is registered using
> >>> `module_platform_driver`...
> >>
> >> ID table and MODULE_DEVICE_TABLE() are necessary for modprobe to work.
> >
> > I think I understand the practical reasons. My point was that I would
> > expect the alias to be added automatically even in the case that the
> > device table is absent based solely on the driver name and the
> > registration method (*module*_*platform*_driver). Why is that not the
> > case? Obviously the driver name matching the mfd_cell name is sufficient
>
> You mean add it automatically by macro-magic based on presence of
> id_table and/or of_match_table?
Yes, that plus: if id_table is present, use that for module aliases,
otherwise use driver name. In fact, I checked the platform core and it
seems to proceed in exactly this way when determining whether there is a
match. And while autoloading and probing are two different things, I
assume that we always want to consider a module for autoloading when we
know that it will probe because we have a compatible device.
> That's a good question. I cannot find answer why not, except that maybe
> no one ever wrote it...
>
>
> > for the driver to probe when it is built in so the name does seem to
> > serve as some identification for the device just as a device table entry
> > would.
> >
> > Furthermore, drivers/input/serio/ioc3kbd.c does not seem to have an ID
> > table either, nor a MODULE_ALIAS -- is that a mistake? If not, what
> > mechanism causes the driver to probe when compiled as a module? It seems
>
> You are now mixing two different things: probing of driver (so bind) and
> module auto-loading.
Yes, sorry, I meant autoloading.
> Probing is done also by driver name. Auto-loading,
> not sure, maybe by name as well?
Well probably not, otherwise it would work here too, no? Unless there
are some fundamental differences in this between PCI and platform
drivers. But the input driver is platform too and is required through
the MFD cell, so I think it should be the same scenario.
> However it is also likely that
> auto-loading is broken. Several drivers had such issues in the past.
OK, I see, thank you. I think this was the main source of my confusion
because I looked at other drivers for reference when trying to
understand which properties (name/device table) are necessary for what
action (probing/autoloading).
> Best regards,
> Krzysztof
Thank you again, kind regards,
K. B.
On Mon, Mar 11, 2024 at 11:26:16AM +0100, Karel Balej wrote:
> Krzysztof Kozlowski, 2024-03-10T21:35:36+01:00:
> > On 10/03/2024 12:35, Karel Balej wrote:
> > > Dmitry Torokhov, 2024-03-04T17:10:59-08:00:
> > >> On Mon, Mar 04, 2024 at 09:28:45PM +0100, Karel Balej wrote:
> > >>> Dmitry,
> > >>>
> > >>> Dmitry Torokhov, 2024-03-03T12:39:46-08:00:
> > >>>> On Sun, Mar 03, 2024 at 11:04:25AM +0100, Karel Balej wrote:
> > >>>>> From: Karel Balej <[email protected]>
> > >>>>>
> > >>>>> Marvell 88PM886 PMIC provides onkey among other things. Add client
> > >>>>> driver to handle it. The driver currently only provides a basic support
> > >>>>> omitting additional functions found in the vendor version, such as long
> > >>>>> onkey and GPIO integration.
> > >>>>>
> > >>>>> Signed-off-by: Karel Balej <[email protected]>
> > >>>>> ---
> > >>>>>
> > >>>>> Notes:
> > >>>>> RFC v3:
> > >>>>> - Drop wakeup-source.
> > >>>>> RFC v2:
> > >>>>> - Address Dmitry's feedback:
> > >>>>> - Sort includes alphabetically.
> > >>>>> - Drop onkey->irq.
> > >>>>> - ret -> err in irq_handler and no initialization.
> > >>>>> - Break long lines and other formatting.
> > >>>>> - Do not clobber platform_get_irq error.
> > >>>>> - Do not set device parent manually.
> > >>>>> - Use input_set_capability.
> > >>>>> - Use the wakeup-source DT property.
> > >>>>> - Drop of_match_table.
> > >>>>
> > >>>> I only said that you should not be using of_match_ptr(), but you still
> > >>>> need to have of_match_table set and have MODULE_DEVICE_TABLE() for the
> > >>>> proper module loading support.
> > >>>
> > >>> I removed of_match_table because I no longer need compatible for this --
> > >>> there are no device tree properties and the driver is being instantiated
> > >>> by the MFD driver.
> > >>>
> > >>> Is the MODULE_DEVICE_TABLE() entry needed for the driver to probe when
> > >>> compiled as module? If that is the case, given what I write above, am I
> > >>> correct that MODULE_DEVICE_TABLE(platform,...) would be the right thing
> > >>> to use here?
> > >>
> > >> Yes, if uevent generated for the device is "platform:<name>" then
> > >> MODULE_DEVICE_TABLE(platform,...) will suffice. I am not sure how MFD
> > >> sets it up (OF modalias or platform), but you should be able to check
> > >> the format looking at the "uevent" attribute for your device in sysfs
> > >> (/sys/devices/bus/platform/...).
> > >
> > > The uevent is indeed platform.
> > >
> > > But since there is only one device, perhaps having a device table is
> > > superfluous and using `MODULE_ALIAS("platform:88pm886-onkey")` is more
> > > fitting?
> >
> > Adding aliases for standard IDs and standard cases is almost never
> > correct. If you need module alias, it means your ID table is wrong (or
> > missing, which is usually wrong).
> >
> > >
> > > Although I don't understand why this is even necessary when the driver
> > > name is such and the module is registered using
> > > `module_platform_driver`...
> >
> > ID table and MODULE_DEVICE_TABLE() are necessary for modprobe to work.
>
> I think I understand the practical reasons. My point was that I would
> expect the alias to be added automatically even in the case that the
> device table is absent based solely on the driver name and the
> registration method (*module*_*platform*_driver). Why is that not the
> case? Obviously the driver name matching the mfd_cell name is sufficient
> for the driver to probe when it is built in so the name does seem to
> serve as some identification for the device just as a device table entry
> would.
>
> Furthermore, drivers/input/serio/ioc3kbd.c does not seem to have an ID
> table either, nor a MODULE_ALIAS -- is that a mistake? If not, what
> mechanism causes the driver to probe when compiled as a module? It seems
> to me to effectively be the same setup as with my driver and that does
> not load automatically (because of the missing alias).
Yes, ioc3kbd is broken as far as module auto-loading goes. It probably
did not get noticed before because the driver is likely to be built-in
on the target architecture.
I'll take patches.
Thanks.
--
Dmitry