| Message ID | 20241007-starqltechn_integration_upstream-v6-0-0d38b5090c57@gmail.com |
|---|---|
| Headers | show |
| Series | Add support for Maxim Integrated MAX77705 MFD | expand |
On Mon, 07 Oct 2024, Dzmitry Sankouski wrote: > Add the core MFD driver for max77705 PMIC. We define five sub-devices > for which the drivers will be added in subsequent patches. > > Signed-off-by: Dzmitry Sankouski <dsankouski@gmail.com> > > --- > Changes for v5: > - license change to 2.0 > - use same hardware name in Kconfig and module descriptions > Changes for v4: > - rework driver from scratch > - migrate to regmap_add_irq_chip, remove max77705-irq.c, > rename max77705-core.c to max77705.c > - cleanup headers > - remove debugfs code > - migrate to use max77693_dev structure > - remove max77705.h > --- > MAINTAINERS | 2 ++ > drivers/mfd/Kconfig | 12 +++++++ > drivers/mfd/Makefile | 2 ++ > drivers/mfd/max77705.c | 248 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > include/linux/mfd/max77693-common.h | 6 +++- > include/linux/mfd/max77705-private.h | 180 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > 6 files changed, 449 insertions(+), 1 deletion(-) > > diff --git a/MAINTAINERS b/MAINTAINERS > index 9ed8bdaaaca9..4bc9c0da6adb 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -14078,6 +14078,7 @@ F: drivers/*/*max77843.c > F: drivers/*/max14577*.c > F: drivers/*/max77686*.c > F: drivers/*/max77693*.c > +F: drivers/*/max77705*.c > F: drivers/clk/clk-max77686.c > F: drivers/extcon/extcon-max14577.c > F: drivers/extcon/extcon-max77693.c > @@ -14085,6 +14086,7 @@ F: drivers/rtc/rtc-max77686.c > F: include/linux/mfd/max14577*.h > F: include/linux/mfd/max77686*.h > F: include/linux/mfd/max77693*.h > +F: include/linux/mfd/max77705*.h > > MAXIRADIO FM RADIO RECEIVER DRIVER > M: Hans Verkuil <hverkuil@xs4all.nl> > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig > index f9325bcce1b9..090eb3fb3d67 100644 > --- a/drivers/mfd/Kconfig > +++ b/drivers/mfd/Kconfig > @@ -904,6 +904,18 @@ config MFD_MAX77693 > additional drivers must be enabled in order to use the functionality > of the device. > > +config MFD_MAX77705 > + tristate "Maxim MAX77705 PMIC Support" > + depends on I2C > + select MFD_CORE > + help > + Say yes here to add support for Maxim Integrated MAX77705. "Integrated MAX77705 PMIC." > + This is a Power Management IC with Charger, safe LDOs, Flash, Haptic > + and MUIC controls on chip. > + This driver provides common support for accessing the device; > + additional drivers must be enabled in order to use the functionality > + of the device. > + > config MFD_MAX77714 > tristate "Maxim Semiconductor MAX77714 PMIC Support" > depends on I2C > diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile > index 2a9f91e81af8..3dc5742c6aeb 100644 > --- a/drivers/mfd/Makefile > +++ b/drivers/mfd/Makefile > @@ -167,6 +167,7 @@ obj-$(CONFIG_MFD_MAX77620) += max77620.o > obj-$(CONFIG_MFD_MAX77650) += max77650.o > obj-$(CONFIG_MFD_MAX77686) += max77686.o > obj-$(CONFIG_MFD_MAX77693) += max77693.o > +obj-$(CONFIG_MFD_MAX77705) += max77705.o > obj-$(CONFIG_MFD_MAX77714) += max77714.o > obj-$(CONFIG_MFD_MAX77843) += max77843.o > obj-$(CONFIG_MFD_MAX8907) += max8907.o > @@ -232,6 +233,7 @@ obj-$(CONFIG_MFD_RK8XX_I2C) += rk8xx-i2c.o > obj-$(CONFIG_MFD_RK8XX_SPI) += rk8xx-spi.o > obj-$(CONFIG_MFD_RN5T618) += rn5t618.o > obj-$(CONFIG_MFD_SEC_CORE) += sec-core.o sec-irq.o > +obj-$(CONFIG_MFD_S2DOS05) += s2dos05.o > obj-$(CONFIG_MFD_SYSCON) += syscon.o > obj-$(CONFIG_MFD_LM3533) += lm3533-core.o lm3533-ctrlbank.o > obj-$(CONFIG_MFD_VEXPRESS_SYSREG) += vexpress-sysreg.o > diff --git a/drivers/mfd/max77705.c b/drivers/mfd/max77705.c > new file mode 100644 > index 000000000000..553f20a6cdd5 > --- /dev/null > +++ b/drivers/mfd/max77705.c > @@ -0,0 +1,248 @@ > +// SPDX-License-Identifier: GPL-2.0+ > +// > +// max77705.c - mfd core driver for the MAX77705 No filenames in headers please - they have a habit of going out of date. Drop references to "MFD": * Maxim MAX77705 PMIC core driver > +// Copyright (C) 2024 Dzmitry Sankouski <dsankouski@gmail.com> Only the SPDX in C++ comments please. > + > +#include <linux/i2c.h> > +#include <linux/interrupt.h> > +#include <linux/mfd/core.h> > +#include <linux/mfd/max77705-private.h> > +#include <linux/mfd/max77693-common.h> > +#include <linux/module.h> > +#include <linux/of.h> > +#include <linux/regmap.h> Alphabetical please. > + > +#define I2C_ADDR_CHG (0xD2 >> 1) > +#define I2C_ADDR_FG (0x6C >> 1) Why are these being shifted? > + > +static struct mfd_cell max77705_devs[] = { > + { > + .name = "leds-max77705-rgb", Why is 'leds' prefixed here? > + .of_compatible = "maxim,max77705-led", Is it a straight LED or RGB? > + }, > + { > + .name = "max77705-fuel-gauge", > + .of_compatible = "maxim,max77705-fuel-gauge", > + }, > + { > + .name = "max77705-charger", > + .of_compatible = "maxim,max77705-charger", > + }, > + { > + .name = "max77705-haptic", > + .of_compatible = "maxim,max77705-haptic", > + }, > +}; > + > +static const struct regmap_range max77705_readable_ranges[] = { > + regmap_reg_range(MAX77705_PMIC_REG_PMICID1, MAX77705_PMIC_REG_BSTOUT_MASK), > + regmap_reg_range(MAX77705_PMIC_REG_INTSRC, MAX77705_PMIC_REG_RESERVED_29), > + regmap_reg_range(MAX77705_PMIC_REG_BOOSTCONTROL1, MAX77705_PMIC_REG_BOOSTCONTROL1), > + regmap_reg_range(MAX77705_PMIC_REG_MCONFIG, MAX77705_PMIC_REG_MCONFIG2), > + regmap_reg_range(MAX77705_PMIC_REG_FORCE_EN_MASK, MAX77705_PMIC_REG_FORCE_EN_MASK), > + regmap_reg_range(MAX77705_PMIC_REG_BOOSTCONTROL1, MAX77705_PMIC_REG_BOOSTCONTROL1), > + regmap_reg_range(MAX77705_PMIC_REG_BOOSTCONTROL2, MAX77705_PMIC_REG_BOOSTCONTROL2), > + regmap_reg_range(MAX77705_PMIC_REG_SW_RESET, MAX77705_PMIC_REG_USBC_RESET), > +}; > + > +static const struct regmap_range max77705_writable_ranges[] = { > + regmap_reg_range(MAX77705_PMIC_REG_MAINCTRL1, MAX77705_PMIC_REG_BSTOUT_MASK), > + regmap_reg_range(MAX77705_PMIC_REG_INTSRC, MAX77705_PMIC_REG_RESERVED_29), > + regmap_reg_range(MAX77705_PMIC_REG_BOOSTCONTROL1, MAX77705_PMIC_REG_BOOSTCONTROL1), > + regmap_reg_range(MAX77705_PMIC_REG_MCONFIG, MAX77705_PMIC_REG_MCONFIG2), > + regmap_reg_range(MAX77705_PMIC_REG_FORCE_EN_MASK, MAX77705_PMIC_REG_FORCE_EN_MASK), > + regmap_reg_range(MAX77705_PMIC_REG_BOOSTCONTROL1, MAX77705_PMIC_REG_BOOSTCONTROL1), > + regmap_reg_range(MAX77705_PMIC_REG_BOOSTCONTROL2, MAX77705_PMIC_REG_BOOSTCONTROL2), > + regmap_reg_range(MAX77705_PMIC_REG_SW_RESET, MAX77705_PMIC_REG_USBC_RESET), > + > +}; > + > +static const struct regmap_access_table max77705_readable_table = { > + .yes_ranges = max77705_readable_ranges, > + .n_yes_ranges = ARRAY_SIZE(max77705_readable_ranges), > +}; > + > +static const struct regmap_access_table max77705_writable_table = { > + .yes_ranges = max77705_writable_ranges, > + .n_yes_ranges = ARRAY_SIZE(max77705_writable_ranges), > +}; > + > +static const struct regmap_config max77705_regmap_config = { > + .reg_bits = 8, > + .val_bits = 8, > + .rd_table = &max77705_readable_table, > + .wr_table = &max77705_writable_table, > + .max_register = MAX77705_PMIC_REG_USBC_RESET, > +}; > + > +static const struct regmap_config max77705_leds_regmap_config = { > + .reg_base = MAX77705_RGBLED_REG_BASE, > + .reg_bits = 8, > + .val_bits = 8, > + .max_register = MAX77705_LED_REG_END, > +}; > + > +static const struct regmap_config max77705_chg_regmap_config = { > + .reg_base = MAX77705_CHG_REG_BASE, > + .reg_bits = 8, > + .val_bits = 8, > + .max_register = MAX77705_CHG_REG_SAFEOUT_CTRL, > +}; > + > +static const struct regmap_config max77705_fg_regmap_config = { > + .reg_bits = 8, > + .val_bits = 8, > + .max_register = MAX77705_FG_END, > +}; > + > +static const struct regmap_irq max77705_topsys_irqs[] = { > + { .mask = MAX77705_SYSTEM_IRQ_BSTEN_INT, }, > + { .mask = MAX77705_SYSTEM_IRQ_SYSUVLO_INT, }, > + { .mask = MAX77705_SYSTEM_IRQ_SYSOVLO_INT, }, > + { .mask = MAX77705_SYSTEM_IRQ_TSHDN_INT, }, > + { .mask = MAX77705_SYSTEM_IRQ_TM_INT, }, > +}; > + > +static const struct regmap_irq_chip max77705_topsys_irq_chip = { > + .name = "max77705-topsys", > + .status_base = MAX77705_PMIC_REG_SYSTEM_INT, > + .mask_base = MAX77705_PMIC_REG_SYSTEM_INT_MASK, > + .num_regs = 1, > + .irqs = max77705_topsys_irqs, > + .num_irqs = ARRAY_SIZE(max77705_topsys_irqs), > +}; > + > +static int max77705_i2c_probe(struct i2c_client *i2c) > +{ > + struct max77693_dev *max77705; > + struct i2c_client *i2c_chg; > + struct i2c_client *i2c_fg; > + struct regmap_irq_chip_data *irq_data; > + struct irq_domain *domain; > + int ret; > + unsigned int pmic_rev_value; > + u8 pmic_ver, pmic_rev; > + > + > + max77705 = devm_kzalloc(&i2c->dev, sizeof(struct max77693_dev), sizeof(*max77705) > + GFP_KERNEL); Why wrap here? You're using 100-chars above. > + if (!max77705) > + return -ENOMEM; > + > + max77705->dev = &i2c->dev; > + max77705->irq = i2c->irq; > + max77705->type = TYPE_MAX77705; > + i2c_set_clientdata(i2c, max77705); > + > + max77705->regmap = devm_regmap_init_i2c(i2c, &max77705_regmap_config); > + if (IS_ERR(max77705->regmap)) > + return PTR_ERR(max77705->regmap); > + > + if (regmap_read(max77705->regmap, MAX77705_PMIC_REG_PMICREV, &pmic_rev_value) < 0) > + return -ENODEV; > + > + pmic_rev = (pmic_rev_value & MAX77705_REVISION_MASK); > + pmic_ver = ((pmic_rev_value & MAX77705_VERSION_MASK) >> MAX77705_VERSION_SHIFT); Over-bracketing. > + dev_dbg(max77705->dev, "device found: rev.0x%x, ver.0x%x\n", > + pmic_rev, pmic_ver); Is this ever going to be useful after initial development? > + if (pmic_rev != MAX77705_PASS3) { > + dev_err(max77705->dev, "rev.0x%x is not tested", > + pmic_rev); ... especially since you're going to print unexpected values out anyway. > + return -ENODEV; > + } > + > + max77705->regmap_leds = devm_regmap_init_i2c(i2c, &max77705_leds_regmap_config); > + if (IS_ERR(max77705->regmap_leds)) > + return PTR_ERR(max77705->regmap_leds); > + > + i2c_chg = devm_i2c_new_dummy_device(max77705->dev, > + i2c->adapter, I2C_ADDR_CHG); '\n' here. > + max77705->regmap_chg = devm_regmap_init_i2c(i2c_chg, > + &max77705_chg_regmap_config); Why not create this in the CHG driver? > + if (IS_ERR(max77705->regmap_chg)) > + return PTR_ERR(max77705->regmap_chg); > + > + i2c_fg = devm_i2c_new_dummy_device(max77705->dev, i2c->adapter, > + I2C_ADDR_FG); > + max77705->regmap_fg = devm_regmap_init_i2c(i2c_fg, > + &max77705_fg_regmap_config); Why not create this in the FG driver? > + if (IS_ERR(max77705->regmap_fg)) > + return PTR_ERR(max77705->regmap_fg); > + > + ret = devm_regmap_add_irq_chip(max77705->dev, max77705->regmap, > + max77705->irq, > + IRQF_ONESHOT | IRQF_SHARED, 0, > + &max77705_topsys_irq_chip, > + &irq_data); > + if (ret) > + dev_err(max77705->dev, "failed to add irq chip: %d\n", ret); > + > + /* Unmask interrupts from all blocks in interrupt source register */ > + ret = regmap_update_bits(max77705->regmap, > + MAX77705_PMIC_REG_INTSRC_MASK, > + MAX77705_SRC_IRQ_ALL, (unsigned int)~MAX77705_SRC_IRQ_ALL); Why the cast? > + if (ret < 0) > + dev_err(max77705->dev, > + "Could not unmask interrupts in INTSRC: %d\n", ret); If it's an error, you should return. > + domain = regmap_irq_get_domain(irq_data); '\n' here. > + ret = devm_mfd_add_devices(max77705->dev, PLATFORM_DEVID_NONE, > + max77705_devs, ARRAY_SIZE(max77705_devs), > + NULL, 0, domain); > + if (ret) { > + dev_err(max77705->dev, "failed to add MFD devices: %d\n", ret); Failed to register child devices. > + return ret; > + } > + > + device_init_wakeup(max77705->dev, true); > + > + return 0; > +} > + > +static int max77705_suspend(struct device *dev) > +{ > + struct i2c_client *i2c = to_i2c_client(dev); > + struct max77693_dev *max77705 = i2c_get_clientdata(i2c); > + > + disable_irq(max77705->irq); '\n' here. > + if (device_may_wakeup(dev)) > + enable_irq_wake(max77705->irq); > + > + return 0; > +} > + > +static int max77705_resume(struct device *dev) > +{ > + struct i2c_client *i2c = to_i2c_client(dev); > + struct max77693_dev *max77705 = i2c_get_clientdata(i2c); > + > + if (device_may_wakeup(dev)) > + disable_irq_wake(max77705->irq); '\n' here. > + enable_irq(max77705->irq); > + > + return 0; > +} > + Remove this line. > +DEFINE_SIMPLE_DEV_PM_OPS(max77705_pm_ops, max77705_suspend, max77705_resume); > + > +static const struct of_device_id max77705_i2c_dt_ids[] = { max77705_i2c_of_match > + { .compatible = "maxim,max77705" }, > + { }, > +}; > +MODULE_DEVICE_TABLE(of, max77705_i2c_dt_ids); > + > +static struct i2c_driver max77705_i2c_driver = { > + .driver = { > + .name = "max77705", > + .of_match_table = max77705_i2c_dt_ids, > + .pm = pm_sleep_ptr(&max77705_pm_ops), > + .suppress_bind_attrs = true, > + }, > + .probe = max77705_i2c_probe, Please get rid of all of this ugly tabbing. > +}; > +module_i2c_driver(max77705_i2c_driver); > + > +MODULE_DESCRIPTION("Maxim MAX77705 MFD core driver"); Not "MFD" - as above. > +MODULE_AUTHOR("Dzmitry Sankouski <dsankouski@gmail.com>"); > +MODULE_LICENSE("GPL"); > diff --git a/include/linux/mfd/max77693-common.h b/include/linux/mfd/max77693-common.h > index a5bce099f1ed..1b87b1ada21c 100644 > --- a/include/linux/mfd/max77693-common.h > +++ b/include/linux/mfd/max77693-common.h > @@ -1,6 +1,6 @@ > /* SPDX-License-Identifier: GPL-2.0+ */ > /* > - * Common data shared between Maxim 77693 and 77843 drivers > + * Common data shared between Maxim 77693, 77705 and 77843 drivers > * > * Copyright (C) 2015 Samsung Electronics > */ > @@ -11,6 +11,7 @@ > enum max77693_types { > TYPE_MAX77693_UNKNOWN, > TYPE_MAX77693, > + TYPE_MAX77705, > TYPE_MAX77843, > > TYPE_MAX77693_NUM, > @@ -25,6 +26,7 @@ struct max77693_dev { > struct i2c_client *i2c_muic; /* 0x4A , MUIC */ > struct i2c_client *i2c_haptic; /* MAX77693: 0x90 , Haptic */ > struct i2c_client *i2c_chg; /* MAX77843: 0xD2, Charger */ > + struct i2c_client *i2c_fg; /* MAX77843: 0xD2, Charger */ > > enum max77693_types type; > > @@ -32,6 +34,8 @@ struct max77693_dev { > struct regmap *regmap_muic; > struct regmap *regmap_haptic; /* Only MAX77693 */ > struct regmap *regmap_chg; /* Only MAX77843 */ > + struct regmap *regmap_fg; /* Only MAX77705 */ > + struct regmap *regmap_leds; /* Only MAX77705 */ > > struct regmap_irq_chip_data *irq_data_led; > struct regmap_irq_chip_data *irq_data_topsys; > diff --git a/include/linux/mfd/max77705-private.h b/include/linux/mfd/max77705-private.h > new file mode 100644 > index 000000000000..8479d1b6cbe4 > --- /dev/null > +++ b/include/linux/mfd/max77705-private.h > @@ -0,0 +1,180 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +// > +// Maxim MAX77705 definitions. > +// > +// Copyright (C) 2015 Samsung Electronics, Inc. > +// Copyright (C) 2024 Dzmitry Sankouski <dsankouski@gmail.com> No C++ please. > + > +#ifndef __LINUX_MFD_MAX77705_PRIV_H > +#define __LINUX_MFD_MAX77705_PRIV_H > + > +#include <linux/pm.h> > + > +#define MAX77705_SRC_IRQ_CHG BIT(0) > +#define MAX77705_SRC_IRQ_TOP BIT(1) > +#define MAX77705_SRC_IRQ_FG BIT(2) > +#define MAX77705_SRC_IRQ_USBC BIT(3) > +#define MAX77705_SRC_IRQ_ALL (MAX77705_SRC_IRQ_CHG | MAX77705_SRC_IRQ_TOP | \ > + MAX77705_SRC_IRQ_FG | MAX77705_SRC_IRQ_USBC) > + > +// MAX77705_PMIC_REG_PMICREV register No C++ please. > +#define MAX77705_VERSION_SHIFT 3 > +#define MAX77705_REVISION_MASK GENMASK(2, 0) > +#define MAX77705_VERSION_MASK GENMASK(7, MAX77705_VERSION_SHIFT) > +// MAX77705_PMIC_REG_MAINCTRL1 register > +#define MAX77705_MAINCTRL1_BIASEN_SHIFT 7 > +#define MAX77705_MAINCTRL1_BIASEN_MASK BIT(MAX77705_MAINCTRL1_BIASEN_SHIFT) > +// MAX77705_PMIC_REG_MCONFIG2 (haptics) register > +#define MAX77705_CONFIG2_MEN_SHIFT 6 > +#define MAX77705_CONFIG2_MODE_SHIFT 7 > +#define MAX77705_CONFIG2_HTYP_SHIFT 5 > +// MAX77705_PMIC_REG_SYSTEM_INT_MASK register > +#define MAX77705_SYSTEM_IRQ_BSTEN_INT BIT(3) > +#define MAX77705_SYSTEM_IRQ_SYSUVLO_INT BIT(4) > +#define MAX77705_SYSTEM_IRQ_SYSOVLO_INT BIT(5) > +#define MAX77705_SYSTEM_IRQ_TSHDN_INT BIT(6) > +#define MAX77705_SYSTEM_IRQ_TM_INT BIT(7) > + > +enum max77705_hw_rev { > + MAX77705_PASS1 = 1, > + MAX77705_PASS2, > + MAX77705_PASS3, > +}; > + > +enum max77705_reg { > + MAX77705_PMIC_REG_PMICID1 = 0x00, > + MAX77705_PMIC_REG_PMICREV = 0x01, > + MAX77705_PMIC_REG_MAINCTRL1 = 0x02, > + MAX77705_PMIC_REG_BSTOUT_MASK = 0x03, > + MAX77705_PMIC_REG_FORCE_EN_MASK = 0x08, > + MAX77705_PMIC_REG_MCONFIG = 0x10, > + MAX77705_PMIC_REG_MCONFIG2 = 0x11, > + MAX77705_PMIC_REG_INTSRC = 0x22, > + MAX77705_PMIC_REG_INTSRC_MASK = 0x23, > + MAX77705_PMIC_REG_SYSTEM_INT = 0x24, > + MAX77705_PMIC_REG_RESERVED_25 = 0x25, > + MAX77705_PMIC_REG_SYSTEM_INT_MASK = 0x26, > + MAX77705_PMIC_REG_RESERVED_27 = 0x27, > + MAX77705_PMIC_REG_RESERVED_28 = 0x28, > + MAX77705_PMIC_REG_RESERVED_29 = 0x29, > + MAX77705_PMIC_REG_BOOSTCONTROL1 = 0x4C, > + MAX77705_PMIC_REG_BOOSTCONTROL2 = 0x4F, > + MAX77705_PMIC_REG_SW_RESET = 0x50, > + MAX77705_PMIC_REG_USBC_RESET = 0x51, > + > + MAX77705_PMIC_REG_END, > +}; > + > +enum max77705_chg_reg { > + MAX77705_CHG_REG_BASE = 0xB0, > + MAX77705_CHG_REG_INT = 0, Tab these out like above and below. > + MAX77705_CHG_REG_INT_MASK, > + MAX77705_CHG_REG_INT_OK, > + MAX77705_CHG_REG_DETAILS_00, > + MAX77705_CHG_REG_DETAILS_01, > + MAX77705_CHG_REG_DETAILS_02, > + MAX77705_CHG_REG_DTLS_03, > + MAX77705_CHG_REG_CNFG_00, > + MAX77705_CHG_REG_CNFG_01, > + MAX77705_CHG_REG_CNFG_02, > + MAX77705_CHG_REG_CNFG_03, > + MAX77705_CHG_REG_CNFG_04, > + MAX77705_CHG_REG_CNFG_05, > + MAX77705_CHG_REG_CNFG_06, > + MAX77705_CHG_REG_CNFG_07, > + MAX77705_CHG_REG_CNFG_08, > + MAX77705_CHG_REG_CNFG_09, > + MAX77705_CHG_REG_CNFG_10, > + MAX77705_CHG_REG_CNFG_11, > + MAX77705_CHG_REG_CNFG_12, > + MAX77705_CHG_REG_CNFG_13, > + MAX77705_CHG_REG_CNFG_14, > + MAX77705_CHG_REG_SAFEOUT_CTRL, > +}; > + > +enum max77705_fuelgauge_reg { > + STATUS_REG = 0x00, > + VALRT_THRESHOLD_REG = 0x01, > + TALRT_THRESHOLD_REG = 0x02, > + SALRT_THRESHOLD_REG = 0x03, > + REMCAP_REP_REG = 0x05, > + SOCREP_REG = 0x06, > + TEMPERATURE_REG = 0x08, > + VCELL_REG = 0x09, > + TIME_TO_EMPTY_REG = 0x11, > + FULLSOCTHR_REG = 0x13, > + CURRENT_REG = 0x0A, > + AVG_CURRENT_REG = 0x0B, > + SOCMIX_REG = 0x0D, > + SOCAV_REG = 0x0E, > + REMCAP_MIX_REG = 0x0F, > + FULLCAP_REG = 0x10, > + RFAST_REG = 0x15, > + AVR_TEMPERATURE_REG = 0x16, > + CYCLES_REG = 0x17, > + DESIGNCAP_REG = 0x18, > + AVR_VCELL_REG = 0x19, > + TIME_TO_FULL_REG = 0x20, > + CONFIG_REG = 0x1D, > + ICHGTERM_REG = 0x1E, > + REMCAP_AV_REG = 0x1F, > + FULLCAP_NOM_REG = 0x23, > + LEARN_CFG_REG = 0x28, > + FILTER_CFG_REG = 0x29, > + MISCCFG_REG = 0x2B, > + QRTABLE20_REG = 0x32, > + FULLCAP_REP_REG = 0x35, > + RCOMP_REG = 0x38, > + VEMPTY_REG = 0x3A, > + FSTAT_REG = 0x3D, > + DISCHARGE_THRESHOLD_REG = 0x40, > + QRTABLE30_REG = 0x42, > + ISYS_REG = 0x43, > + DQACC_REG = 0x45, > + DPACC_REG = 0x46, > + AVGISYS_REG = 0x4B, > + QH_REG = 0x4D, > + VSYS_REG = 0xB1, > + TALRTTH2_REG = 0xB2, > + VBYP_REG = 0xB3, > + CONFIG2_REG = 0xBB, > + IIN_REG = 0xD0, > + OCV_REG = 0xEE, > + VFOCV_REG = 0xFB, > + VFSOC_REG = 0xFF, > + > + MAX77705_FG_END, > +}; > + > +enum max77705_led_reg { > + MAX77705_RGBLED_REG_BASE = 0x30, > + MAX77705_RGBLED_REG_LEDEN = 0, Here too. > + MAX77705_RGBLED_REG_LED0BRT, > + MAX77705_RGBLED_REG_LED1BRT, > + MAX77705_RGBLED_REG_LED2BRT, > + MAX77705_RGBLED_REG_LED3BRT, > + MAX77705_RGBLED_REG_LEDRMP, > + MAX77705_RGBLED_REG_LEDBLNK, > + MAX77705_LED_REG_END > +}; > + > +enum max77705_charger_battery_state { > + MAX77705_BATTERY_NOBAT, > + MAX77705_BATTERY_PREQUALIFICATION, > + MAX77705_BATTERY_DEAD, > + MAX77705_BATTERY_GOOD, > + MAX77705_BATTERY_LOWVOLTAGE, > + MAX77705_BATTERY_OVERVOLTAGE, > + MAX77705_BATTERY_RESERVED, > +}; > + > +enum max77705_charger_charge_type { > + MAX77705_CHARGER_CONSTANT_CURRENT = 1, > + MAX77705_CHARGER_CONSTANT_VOLTAGE, > + MAX77705_CHARGER_END_OF_CHARGE, > + MAX77705_CHARGER_DONE, > +}; > + > +extern const struct dev_pm_ops max77705_pm_ops; > + > +#endif /* __LINUX_MFD_MAX77705_PRIV_H */ > > -- > 2.39.2 >
On Mon, 07 Oct 2024, Dzmitry Sankouski wrote: > This adds basic support for LEDs for the max77705 PMIC. > > Signed-off-by: Dzmitry Sankouski <dsankouski@gmail.com> > > --- > Changes for v5: > - use same hardware name in Kconfig and module descriptions > - remove copyrighter owner from module authors > > Changes in v4: > - inline BLINK_(ON|OFF) macro > - remove camel case > - drop backwards compatibility(new driver) > - drop module alias > --- > MAINTAINERS | 1 + > drivers/leds/Kconfig | 6 ++++++ > drivers/leds/Makefile | 1 + > drivers/leds/leds-max77705.c | 157 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ Lol! How big is your terminal? :) > 4 files changed, 165 insertions(+) > > diff --git a/MAINTAINERS b/MAINTAINERS > index 4bc9c0da6adb..66a1dd7577c4 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -14079,6 +14079,7 @@ F: drivers/*/max14577*.c > F: drivers/*/max77686*.c > F: drivers/*/max77693*.c > F: drivers/*/max77705*.c > +F: drivers/leds/leds-max77705.c Alphabetical? > F: drivers/clk/clk-max77686.c > F: drivers/extcon/extcon-max14577.c > F: drivers/extcon/extcon-max77693.c > diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig > index b784bb74a837..a8492623caa4 100644 > --- a/drivers/leds/Kconfig > +++ b/drivers/leds/Kconfig > @@ -753,6 +753,12 @@ config LEDS_MAX77650 > help > LEDs driver for MAX77650 family of PMICs from Maxim Integrated. > > +config LEDS_MAX77705 > + tristate "LED support for Maxim MAX77705 PMIC" > + depends on MFD_MAX77705 && LEDS_CLASS && I2C If MFD_MAX77705 depends on I2C, you shouldn't need that here. > + help > + LED driver for MAX77705 MFD chip from Maxim Integrated. No such thing as an "MFD chip". > + > config LEDS_MAX8997 > tristate "LED support for MAX8997 PMIC" > depends on LEDS_CLASS && MFD_MAX8997 > diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile > index 18afbb5a23ee..096bf244527d 100644 > --- a/drivers/leds/Makefile > +++ b/drivers/leds/Makefile > @@ -60,6 +60,7 @@ obj-$(CONFIG_LEDS_LP8860) += leds-lp8860.o > obj-$(CONFIG_LEDS_LT3593) += leds-lt3593.o > obj-$(CONFIG_LEDS_MAX5970) += leds-max5970.o > obj-$(CONFIG_LEDS_MAX77650) += leds-max77650.o > +obj-$(CONFIG_LEDS_MAX77705) += leds-max77705.o > obj-$(CONFIG_LEDS_MAX8997) += leds-max8997.o > obj-$(CONFIG_LEDS_MC13783) += leds-mc13783.o > obj-$(CONFIG_LEDS_MENF21BMC) += leds-menf21bmc.o > diff --git a/drivers/leds/leds-max77705.c b/drivers/leds/leds-max77705.c > new file mode 100644 > index 000000000000..50af81fb7324 > --- /dev/null > +++ b/drivers/leds/leds-max77705.c > @@ -0,0 +1,157 @@ > +// SPDX-License-Identifier: GPL-2.0 > +// > +// Based on leds-max77650 driver: > +// Copyright (C) 2018 BayLibre SAS > +// Author: Bartosz Golaszewski <bgolaszewski@baylibre.com> You don't need to do that. > +// > +// LED driver for MAXIM 77705 MFD. Remove MFD from everywhere. > +// Copyright (C) 2024 Dzmitry Sankouski <dsankouski@gmail.org> Only the SPDX in C++ please. > +#include <linux/i2c.h> > +#include <linux/leds.h> > +#include <linux/mfd/max77705-private.h> > +#include <linux/module.h> > +#include <linux/platform_device.h> > +#include <linux/regmap.h> > + > +#define MAX77705_LED_NUM_LEDS 4 > +#define MAX77705_LED_EN_MASK GENMASK(1, 0) > +#define MAX77705_LED_MAX_BRIGHTNESS 0xff > + > +struct max77705_led { > + struct led_classdev cdev; > + struct regmap *regmap; > + unsigned int en_shift; > + unsigned int reg_brightness; > +}; > + > +static struct max77705_led *max77705_to_led(struct led_classdev *cdev) > +{ > + return container_of(cdev, struct max77705_led, cdev); > +} > + > +static int max77705_rgb_blink(struct led_classdev *cdev, > + unsigned long *delay_on, > + unsigned long *delay_off) > +{ > + struct max77705_led *led = max77705_to_led(cdev); > + int value, on_value, off_value; > + > + on_value = (((*delay_on < 100) ? 0 : > + (*delay_on < 500) ? *delay_on/100 - 1 : > + (*delay_on < 3250) ? (*delay_on - 500) / 250 + 4 : 15) << 4); > + off_value = ((*delay_off < 1) ? 0x00 : > + (*delay_off < 500) ? 0x01 : > + (*delay_off < 5000) ? *delay_off / 500 : > + (*delay_off < 8000) ? (*delay_off - 5000) / 1000 + 10 : > + (*delay_off < 12000) ? (*delay_off - 8000) / 2000 + 13 : 15); These nested ternary are pretty miserable. Please break them out. Also all of these magic numbers probably need defining so the maths becomes easier to follow for humans. > + value = on_value | off_value; > + return regmap_write(led->regmap, MAX77705_RGBLED_REG_LEDBLNK, value); > +} > + > +static int max77705_led_brightness_set(struct led_classdev *cdev, > + enum led_brightness brightness) > +{ > + struct max77705_led *led = max77705_to_led(cdev); > + int ret; Pop the ret on the bottom, if no other reason than for my OCD! > + unsigned long blink_default = 0; > + > + if (brightness == LED_OFF) { > + // Flash OFF No C++ comments please. > + ret = regmap_update_bits(led->regmap, > + MAX77705_RGBLED_REG_LEDEN, > + MAX77705_LED_EN_MASK << led->en_shift, 0); > + max77705_rgb_blink(cdev, &blink_default, &blink_default); > + } else { > + // Set current > + ret = regmap_write(led->regmap, > + led->reg_brightness, brightness); Line wrap at 100-chars. > + if (ret < 0) > + return ret; > + > + ret = regmap_update_bits(led->regmap, > + MAX77705_RGBLED_REG_LEDEN, LED_ON << led->en_shift, > + MAX77705_LED_EN_MASK << led->en_shift); > + } > + > + return ret; > +} > + > +static int max77705_led_probe(struct platform_device *pdev) > +{ > + struct fwnode_handle *child; > + struct max77705_led *leds, *led; > + struct device *dev; > + struct regmap *map; s/map/regmap/ > + int rv, num_leds; What's rv? If it's "return value", then please just use ret. > + u32 reg; > + > + dev = &pdev->dev; Do this during the declaration > + leds = devm_kcalloc(dev, sizeof(*leds), > + MAX77705_LED_NUM_LEDS, GFP_KERNEL); > + if (!leds) > + return -ENOMEM; > + > + map = dev_get_regmap(dev->parent, NULL); > + if (!map) > + return -ENODEV; > + > + num_leds = device_get_child_node_count(dev); > + if (!num_leds || num_leds > MAX77705_LED_NUM_LEDS) num_leds < 0 > + return -ENODEV; > + > + device_for_each_child_node(dev, child) { If you use the _scoped version, you can drop the gotos. > + struct led_init_data init_data = {}; > + > + rv = fwnode_property_read_u32(child, "reg", ®); > + if (rv || reg >= MAX77705_LED_NUM_LEDS) { > + rv = -EINVAL; > + goto err_node_put; > + } > + > + led = &leds[reg]; > + led->regmap = map; > + led->reg_brightness = MAX77705_RGBLED_REG_LED0BRT + reg; > + led->en_shift = 2 * reg; Why 2? > + led->cdev.brightness_set_blocking = max77705_led_brightness_set; > + led->cdev.blink_set = max77705_rgb_blink; > + led->cdev.max_brightness = MAX77705_LED_MAX_BRIGHTNESS; > + > + init_data.fwnode = child; > + init_data.devicename = "max77705"; You don't want to use the label as the devicename? https://github.com/torvalds/linux/blob/master/drivers/leds/led-core.c#L526 > + > + rv = devm_led_classdev_register_ext(dev, &led->cdev, > + &init_data); > + if (rv) > + goto err_node_put; > + > + rv = max77705_led_brightness_set(&led->cdev, LED_OFF); > + if (rv) > + goto err_node_put; > + } > + > + return 0; '\n' here. > +err_node_put: > + fwnode_handle_put(child); > + return rv; > +} > + > +static const struct of_device_id max77705_led_of_match[] = { > + { .compatible = "maxim,max77705-led" }, > + { } > +}; > +MODULE_DEVICE_TABLE(of, max77705_led_of_match); > + > +static struct platform_driver max77705_led_driver = { > + .driver = { > + .name = "max77705-led", > + .of_match_table = max77705_led_of_match, > + }, > + .probe = max77705_led_probe, > +}; > +module_platform_driver(max77705_led_driver); > + > +MODULE_DESCRIPTION("Maxim MAX77705 LED driver"); > +MODULE_AUTHOR("Dzmitry Sankouski <dsankouski@gmail.com>"); > +MODULE_LICENSE("GPL"); > > -- > 2.39.2 >
Hello, On Mon, Oct 07, 2024 at 06:55:53PM +0300, Dzmitry Sankouski wrote: > +static struct platform_driver max77705_charger_driver = { > + .driver = { > + .name = "max77705-charger", > + .of_match_table = max77705_charger_of_match, > + }, > + .probe = max77705_charger_probe, > + .remove_new = max77705_charger_remove, > +}; > +module_platform_driver(max77705_charger_driver); After commit 0edb555a65d1 ("platform: Make platform_driver::remove() return void") .remove() is (again) the right callback to implement for platform drivers. Please just drop "_new". Best regards Uwe
> > + /* Unmask interrupts from all blocks in interrupt source register */ > > + ret = regmap_update_bits(max77705->regmap, > > + MAX77705_PMIC_REG_INTSRC_MASK, > > + MAX77705_SRC_IRQ_ALL, (unsigned int)~MAX77705_SRC_IRQ_ALL); > > Why the cast? > BIT macro creates a 64 bit constant value. When inverted, it overruns 32 bit value, causing compiler to warn on conversion like `warning: conversion from ‘long unsigned int’ to ‘unsigned int’`.
(...) > > drivers/leds/leds-max77705.c | 157 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > > Lol! How big is your terminal? :) > Dunno actually, this was generated by b4 (...) > No C++ comments please. > This conflicts with https://patchwork.kernel.org/comment/25898728/ > > + ret = regmap_update_bits(led->regmap, > > + MAX77705_RGBLED_REG_LEDEN, > > + MAX77705_LED_EN_MASK << led->en_shift, 0); > > + max77705_rgb_blink(cdev, &blink_default, &blink_default); > > + } else { > > + // Set current > > + ret = regmap_write(led->regmap, > > + led->reg_brightness, brightness); > > Line wrap at 100-chars. >
On Tue, 22 Oct 2024, Dzmitry Sankouski wrote: > (...) > > > drivers/leds/leds-max77705.c | 157 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > > > > Lol! How big is your terminal? :) > > > > Dunno actually, this was generated by b4 > > (...) > > > > No C++ comments please. > > > > This conflicts with https://patchwork.kernel.org/comment/25898728/ a) That's just for the header. b) That's a different subsystem and Mark's choice. > > > + ret = regmap_update_bits(led->regmap, > > > + MAX77705_RGBLED_REG_LEDEN, > > > + MAX77705_LED_EN_MASK << led->en_shift, 0); > > > + max77705_rgb_blink(cdev, &blink_default, &blink_default); > > > + } else { > > > + // Set current > > > + ret = regmap_write(led->regmap, > > > + led->reg_brightness, brightness); > > > > Line wrap at 100-chars. > > > > From coding-style.rst: > `The preferred limit on the length of a single line is 80 columns.` > I only exceed 80 chars, when there's no good wrapping. a) This is old guidance b) A lot of your lines already break the 80-char rule.
On Mon, 21 Oct 2024, Dzmitry Sankouski wrote: > > > diff --git a/drivers/mfd/max77705.c b/drivers/mfd/max77705.c > > > new file mode 100644 > > > index 000000000000..553f20a6cdd5 > > > --- /dev/null > > > +++ b/drivers/mfd/max77705.c > > > @@ -0,0 +1,248 @@ > > > +// SPDX-License-Identifier: GPL-2.0+ > > > +// > > > +// max77705.c - mfd core driver for the MAX77705 > > > (...) > > > +// Copyright (C) 2024 Dzmitry Sankouski <dsankouski@gmail.com> > > > > Only the SPDX in C++ comments please. > > > This conflicts with https://patchwork.kernel.org/comment/25898728/ a) Mark is only talking about the file header > > > + > (...) > > > > +++ b/include/linux/mfd/max77705-private.h > > > @@ -0,0 +1,180 @@ > > > +/* SPDX-License-Identifier: GPL-2.0 */ > > > +// > > > +// Maxim MAX77705 definitions. > > > +// > > > +// Copyright (C) 2015 Samsung Electronics, Inc. > > > +// Copyright (C) 2024 Dzmitry Sankouski <dsankouski@gmail.com> > > > > No C++ please. > > This conflicts with https://patchwork.kernel.org/comment/25898728/ a) Mark is only talking about the file header b) Different subsystem, different rules. > > > > > > + > > > +#ifndef __LINUX_MFD_MAX77705_PRIV_H > > > +#define __LINUX_MFD_MAX77705_PRIV_H > > > + > > > +#include <linux/pm.h> > > > + > > > +#define MAX77705_SRC_IRQ_CHG BIT(0) > > > +#define MAX77705_SRC_IRQ_TOP BIT(1) > > > +#define MAX77705_SRC_IRQ_FG BIT(2) > > > +#define MAX77705_SRC_IRQ_USBC BIT(3) > > > +#define MAX77705_SRC_IRQ_ALL (MAX77705_SRC_IRQ_CHG | MAX77705_SRC_IRQ_TOP | \ > > > + MAX77705_SRC_IRQ_FG | MAX77705_SRC_IRQ_USBC) > > > + > > > +// MAX77705_PMIC_REG_PMICREV register > > > > No C++ please. > > This conflicts with https://patchwork.kernel.org/comment/25898728/ a) Mark is only talking about the file header b) Different subsystem, different rules. > > -- > > Best regards, > Dzmitry >
The Maxim MAX77705 is a Companion Power Management and Type-C interface IC which includes charger, fuelgauge, LED, haptic motor driver and Type-C management IC. It's used in Samsung S series smart phones starting from S9 model. Add features: - charger - fuelgauge - haptic - led Signed-off-by: Dzmitry Sankouski <dsankouski@gmail.com> --- Changes in v6: - fix binding review comments - update trailers - Link to v5: https://lore.kernel.org/r/20240617-starqltechn_integration_upstream-v5-0-e0033f141d17@gmail.com Changes in v5: - Split patchset per subsystem - Link to v4: https://lore.kernel.org/r/20240913-starqltechn_integration_upstream-v4-0-2d2efd5c5877@gmail.com Changes in v4: - Rewrite max77705, max77705_charger, max77705_fuel_gauge from scratch - Reorder patches: - squash max77705 subdevice bindings in core file because no resources there - split device tree changes - Use _ as space for filenames in power/supply like the majority - Link to v3: https://lore.kernel.org/r/20240618-starqltechn_integration_upstream-v3-0-e3f6662017ac@gmail.com --- Dzmitry Sankouski (7): power: supply: add undervoltage health status property dt-bindings: mfd: add maxim,max77705 mfd: Add new driver for MAX77705 PMIC input: max77693: add max77705 haptic support power: supply: max77705: Add charger driver for Maxim 77705 power: supply: max77705: Add fuel gauge driver for Maxim 77705 leds: max77705: Add LEDs support Documentation/ABI/testing/sysfs-class-power | 2 +- Documentation/devicetree/bindings/mfd/maxim,max77705.yaml | 174 +++++++++++++++++++++++++++++++ MAINTAINERS | 4 + drivers/input/misc/Kconfig | 4 +- drivers/input/misc/Makefile | 1 + drivers/input/misc/max77693-haptic.c | 15 ++- drivers/leds/Kconfig | 6 ++ drivers/leds/Makefile | 1 + drivers/leds/leds-max77705.c | 157 ++++++++++++++++++++++++++++ drivers/mfd/Kconfig | 12 +++ drivers/mfd/Makefile | 2 + drivers/mfd/max77705.c | 248 +++++++++++++++++++++++++++++++++++++++++++++ drivers/power/supply/Kconfig | 13 +++ drivers/power/supply/Makefile | 2 + drivers/power/supply/max77705_charger.c | 585 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ drivers/power/supply/max77705_fuel_gauge.c | 348 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ drivers/power/supply/power_supply_sysfs.c | 1 + include/linux/mfd/max77693-common.h | 6 +- include/linux/mfd/max77705-private.h | 180 +++++++++++++++++++++++++++++++++ include/linux/power/max77705_charger.h | 215 +++++++++++++++++++++++++++++++++++++++ include/linux/power/max77705_fuelgauge.h | 65 ++++++++++++ include/linux/power_supply.h | 1 + 22 files changed, 2037 insertions(+), 5 deletions(-) --- base-commit: 58ca61c1a866bfdaa5e19fb19a2416764f847d75 change-id: 20240617-starqltechn_integration_upstream-bc86850b2fe3 Best regards,