On Sun, 03 Mar 2024, Karel Balej wrote: > From: Karel Balej <balejk@xxxxxxxxx> > > 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 <balejk@xxxxxxxxx> > --- > > 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 <balejk@xxxxxxxxx>"); > +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 [李琼斯]