| Message ID | 20241202-starqltechn_integration_upstream-v9-0-a1adc3bae2b8@gmail.com |
|---|---|
| Headers | show |
| Series | Add support for Maxim Integrated MAX77705 PMIC | expand |
On 02/12/2024 10:47, Dzmitry Sankouski wrote: > Add max77705 fuel gauge support. > > Signed-off-by: Dzmitry Sankouski <dsankouski@gmail.com> > --- > Documentation/devicetree/bindings/power/supply/maxim,max17042.yaml | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/Documentation/devicetree/bindings/power/supply/maxim,max17042.yaml b/Documentation/devicetree/bindings/power/supply/maxim,max17042.yaml > index 085e2504d0dc..f929e7e2b82a 100644 > --- a/Documentation/devicetree/bindings/power/supply/maxim,max17042.yaml > +++ b/Documentation/devicetree/bindings/power/supply/maxim,max17042.yaml > @@ -20,6 +20,7 @@ properties: > - maxim,max17050 > - maxim,max17055 > - maxim,max77849-battery > + - maxim,max77705-battery Keep alphabetical order. Best regards, Krzysztof
On 02/12/2024 10:47, Dzmitry Sankouski wrote: > Remove 'reg' property from required list, because in platform > variant it should not be present, because i2c client is passed > from MFD driver. > > Signed-off-by: Dzmitry Sankouski <dsankouski@gmail.com> Sorry, not really. Device needs I2C address. I don't get what is "platform variant" in terms of hardware, maybe you refer to some driver changes. But anyway this needs proper hardware description, e.g. that device does not have its own I2C address. If that's the case, of course. Best regards, Krzysztof
On 02/12/2024 10:47, Dzmitry Sankouski wrote: > Add max77705 fuel gauge support. > > Signed-off-by: Dzmitry Sankouski <dsankouski@gmail.com> > --- > drivers/power/supply/max17042_battery.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/power/supply/max17042_battery.c b/drivers/power/supply/max17042_battery.c > index 6a1bfc4a7b13..8623c89ad274 100644 > --- a/drivers/power/supply/max17042_battery.c > +++ b/drivers/power/supply/max17042_battery.c > @@ -1212,6 +1212,7 @@ static const struct of_device_id max17042_dt_match[] = { > { .compatible = "maxim,max17050" }, > { .compatible = "maxim,max17055" }, > { .compatible = "maxim,max77849-battery" }, > + { .compatible = "maxim,max77705-battery" }, Always keep existing order. Best regards, Krzysztof
On 02/12/2024 10:47, 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> > ... > + > +static int max77705_i2c_probe(struct i2c_client *i2c) > +{ > + struct max77693_dev *max77705; > + struct i2c_client *i2c_fg; > + struct regmap_irq_chip_data *irq_data; > + struct irq_domain *domain; > + int ret; > + unsigned int pmic_rev_value; > + enum max77705_hw_rev pmic_rev; > + > + Only one blank line > + max77705 = devm_kzalloc(&i2c->dev, sizeof(*max77705), GFP_KERNEL); > + if (!max77705) > + return -ENOMEM; > + > + max77705->i2c = i2c; > + 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); > + No blank line. Theer is never blank line between call and its error check. > + 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; > + Drop blank line. > + if (pmic_rev != MAX77705_PASS3) { > + dev_err(max77705->dev, "rev.0x%x is not tested", > + pmic_rev); Unnecessary line wrap. > + 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); > + > + ret = devm_regmap_add_irq_chip(max77705->dev, max77705->regmap, > + max77705->irq, > + IRQF_ONESHOT | IRQF_SHARED, 0, > + &max77705_topsys_irq_chip, > + &irq_data); > + Same issues, all over the code. > + 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); The need for cast comes from some compiler warning? > + > + if (ret < 0) { > + dev_err(max77705->dev, > + "Could not unmask interrupts in INTSRC: %d\n", ret); > + return ret; > + } > + > + domain = regmap_irq_get_domain(irq_data); > + > + i2c_fg = devm_i2c_new_dummy_device(max77705->dev, max77705->i2c->adapter, I2C_ADDR_FG); > + > + if (IS_ERR(i2c_fg)) > + return PTR_ERR(i2c_fg); > + > + max77705->i2c_fg = i2c_fg; > + > + for (int i = 0; i < ARRAY_SIZE(max77705_devs); i++) { > + if (!strcmp(max77705_devs[i].name, FUEL_GAUGE_NAME)) { > + max77705_devs[i].platform_data = &i2c_fg; > + max77705_devs[i].pdata_size = sizeof(i2c_fg); > + } > + } > + > + 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 register child devices: %d\n", ret); > + 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); > + > + 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); > + > + enable_irq(max77705->irq); > + > + return 0; > +} > +DEFINE_SIMPLE_DEV_PM_OPS(max77705_pm_ops, max77705_suspend, max77705_resume); > + > +static const struct of_device_id max77705_i2c_of_match[] = { > + { .compatible = "maxim,max77705" }, > + { }, > +}; > +MODULE_DEVICE_TABLE(of, max77705_i2c_of_match); > + > +static struct i2c_driver max77705_i2c_driver = { > + .driver = { > + .name = "max77705", > + .of_match_table = max77705_i2c_of_match, > + .pm = pm_sleep_ptr(&max77705_pm_ops), > + .suppress_bind_attrs = true, > + }, > + .probe = max77705_i2c_probe, > +}; > +module_i2c_driver(max77705_i2c_driver); > + > +MODULE_DESCRIPTION("Maxim MAX77705 PMIC core driver"); > +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..8665097892cd 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 */ You mix patchsets. Don't grow 77843 wigth 77705. Or is this not max77843? > > enum max77693_types type; > > @@ -32,6 +34,7 @@ struct max77693_dev { > struct regmap *regmap_muic; > struct regmap *regmap_haptic; /* Only MAX77693 */ > struct regmap *regmap_chg; /* Only MAX77843 */ > + 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..be781a0f9802 ... > + > +enum max77705_led_reg { > + MAX77705_RGBLED_REG_BASE = 0x30, > + MAX77705_RGBLED_REG_LEDEN = 0, > + 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; Why do you need it in the header? > + > +#endif /* __LINUX_MFD_MAX77705_PRIV_H */ > Best regards, Krzysztof
On 02/12/2024 10:48, Dzmitry Sankouski wrote: > Add driver for Maxim 77705 switch-mode charger (part of max77705 > MFD driver) providing power supply class information to userspace. > > The driver is configured through DTS (battery and system related > settings). > > Signed-off-by: Dzmitry Sankouski <dsankouski@gmail.com> > ... > + > +static int max77705_charger_probe(struct platform_device *pdev) > +{ > + struct power_supply_config pscfg = {}; > + struct i2c_client *i2c_chg; > + struct max77693_dev *max77705; > + struct max77705_charger_data *chg; > + struct device *dev, *parent; > + struct regmap_irq_chip_data *irq_data; > + int ret; > + > + dev = &pdev->dev; > + parent = dev->parent; > + max77705 = dev_get_drvdata(parent); > + > + chg = devm_kzalloc(dev, sizeof(*chg), GFP_KERNEL); > + if (!chg) > + return -ENOMEM; > + > + platform_set_drvdata(pdev, chg); > + > + i2c_chg = devm_i2c_new_dummy_device(dev, max77705->i2c->adapter, I2C_ADDR_CHG); > + Same problems as in other patchset. This really needs some work to look closer to Linux coding style. > + if (IS_ERR(i2c_chg)) > + return PTR_ERR(i2c_chg); > + > + chg->regmap = devm_regmap_init_i2c(i2c_chg, &max77705_chg_regmap_config); > + > + if (IS_ERR(chg->regmap)) > + return PTR_ERR(chg->regmap); > + > + chg->dev = dev; > + > + ret = regmap_update_bits(chg->regmap, > + MAX77705_CHG_REG_INT_MASK, > + MAX77705_CHGIN_IM, 0); > + > + if (ret) > + return ret; > + > + pscfg.of_node = dev->of_node; > + pscfg.drv_data = chg; > + > + chg->psy_chg = devm_power_supply_register(dev, &max77705_charger_psy_desc, &pscfg); > + if (IS_ERR(chg->psy_chg)) > + return PTR_ERR(chg->psy_chg); > + > + max77705_charger_irq_chip.irq_drv_data = chg; > + ret = devm_regmap_add_irq_chip(chg->dev, chg->regmap, max77705->irq, > + IRQF_ONESHOT | IRQF_SHARED, 0, > + &max77705_charger_irq_chip, > + &irq_data); > + if (ret) { > + dev_err(dev, "failed to add irq chip: %d\n", ret); > + return ret; > + } > + > + chg->wqueue = create_singlethread_workqueue(dev_name(dev)); > + if (IS_ERR(chg->wqueue)) { > + dev_err(dev, "failed to create workqueue\n"); > + return PTR_ERR(chg->wqueue); > + } > + INIT_WORK(&chg->chgin_work, max77705_chgin_isr_work); > + > + max77705_charger_initialize(chg); > + > + return max77705_charger_enable(chg); > +} > + > +static void max77705_charger_remove(struct platform_device *pdev) > +{ > + struct max77705_charger_data *chg = platform_get_drvdata(pdev); > + > + max77705_charger_disable(chg); Use devm for this. You use shared interrupt, so you are not suppose to mix devm and non-devm, even if this is actually safe. > +} > + > +static const struct of_device_id max77705_charger_of_match[] = { > + { .compatible = "maxim,max77705-charger" }, > + { } > +}; > +MODULE_DEVICE_TABLE(of, max77705_charger_of_match); > + > +static struct platform_driver max77705_charger_driver = { > + .driver = { > + .name = "max77705-charger", > + .of_match_table = max77705_charger_of_match, > + }, > + .probe = max77705_charger_probe, > + .remove = max77705_charger_remove, > +}; > +module_platform_driver(max77705_charger_driver); > + > +MODULE_AUTHOR("Dzmitry Sankouski <dsankouski@gmail.com>"); > +MODULE_DESCRIPTION("Maxim MAX77705 charger driver"); > +MODULE_LICENSE("GPL"); > diff --git a/include/linux/power/max77705_charger.h b/include/linux/power/max77705_charger.h > new file mode 100644 > index 000000000000..44ecd6b32cbe > --- /dev/null > +++ b/include/linux/power/max77705_charger.h > @@ -0,0 +1,216 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +/* > + * Maxim MAX77705 definitions. > + * > + * Copyright (C) 2015 Samsung Electronics, Inc. > + * Copyright (C) 2024 Dzmitry Sankouski <dsankouski@gmail.com> > + */ ... > + > +/* MAX77705_CHG_REG_CNFG_10 */ > +#define MAX77705_CHG_WCIN_LIM GENMASK(5, 0) > + > +/* MAX77705_CHG_REG_CNFG_11 */ > +#define MAX77705_VBYPSET_SHIFT 0 > +#define MAX77705_VBYPSET_MASK GENMASK(6, 0) > + > +/* MAX77705_CHG_REG_CNFG_12 */ > +#define MAX77705_CHGINSEL_SHIFT 5 > +#define MAX77705_CHGINSEL_MASK BIT(MAX77705_CHGINSEL_SHIFT) > +#define MAX77705_WCINSEL_SHIFT 6 > +#define MAX77705_WCINSEL_MASK BIT(MAX77705_WCINSEL_SHIFT) > +#define MAX77705_VCHGIN_REG_MASK GENMASK(4, 3) > +#define MAX77705_WCIN_REG_MASK GENMASK(2, 1) > +#define MAX77705_REG_DISKIP_SHIFT 0 > +#define MAX77705_REG_DISKIP_MASK BIT(MAX77705_REG_DISKIP_SHIFT) > +/* REG=4.5V, UVLO=4.7V */ > +#define MAX77705_VCHGIN_4_5 0 > +/* REG=4.5V, UVLO=4.7V */ > +#define MAX77705_WCIN_4_5 0 > +#define MAX77705_DISABLE_SKIP 1 > +#define MAX77705_AUTO_SKIP 0 > + > +/* mA */ > +#define MAX77705_CURRENT_STEP 25 > +#define MAX77705_CURRENT_WCIN_MAX 1600 > +#define MAX77705_CURRENT_CHGIN_MAX 3200 > + > +/* Convert current in mA to corresponding CNFG09 value */ > +inline u8 max77705_convert_ma_to_chgin_ilim_value(unsigned int cur) > +{ > + if (cur < MAX77705_CURRENT_STEP) > + return 0; > + if (cur < MAX77705_CURRENT_CHGIN_MAX) > + return (cur / MAX77705_CURRENT_STEP) - 1; > + else > + return (MAX77705_CURRENT_CHGIN_MAX / MAX77705_CURRENT_STEP) - 1; > +} Drop all inlines from header. You are not suppose to have such driver functions inlined all over the users. Best regards, Krzysztof
пн, 2 дек. 2024 г. в 13:23, Krzysztof Kozlowski <krzk@kernel.org>: > > On 02/12/2024 10:47, 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> > > (...) > > + /* 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); > > > The need for cast comes from some compiler warning? > 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’`.
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 v9: - fuel gauge: use max17042 driver instead of separate max77705 - fix kernel bot error - charger: enable interrupt after power supply registration - add dependency on max17042 patch series - Link to v8: https://lore.kernel.org/r/20241031-starqltechn_integration_upstream-v8-0-2fa666c2330e@gmail.com Changes in v8: - Fix comment style - join line where possible to fit in 100 chars - Link to v7: https://lore.kernel.org/r/20241023-starqltechn_integration_upstream-v7-0-9bfaa3f4a1a0@gmail.com Changes in v7: - Fix review comments - Link to v6: https://lore.kernel.org/r/20241007-starqltechn_integration_upstream-v6-0-0d38b5090c57@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 (9): power: supply: add undervoltage health status property dt-bindings: power: supply: max17042: add max77705 support dt-bindings: power: supply: max17042: remove reg from required dt-bindings: mfd: add maxim,max77705 power: supply: max17042: add max77705 fuel gauge support mfd: Add new driver for MAX77705 PMIC input: max77693: add max77705 haptic support power: supply: max77705: Add charger driver for Maxim 77705 leds: max77705: Add LEDs support Documentation/ABI/testing/sysfs-class-power | 2 +- Documentation/devicetree/bindings/mfd/maxim,max77705.yaml | 155 +++++++++++++++++++++++++++++ Documentation/devicetree/bindings/power/supply/maxim,max17042.yaml | 2 +- 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 | 167 ++++++++++++++++++++++++++++++++ drivers/mfd/Kconfig | 12 +++ drivers/mfd/Makefile | 2 + drivers/mfd/max77705.c | 242 ++++++++++++++++++++++++++++++++++++++++++++++ drivers/power/supply/Kconfig | 6 ++ drivers/power/supply/Makefile | 1 + drivers/power/supply/max17042_battery.c | 3 + drivers/power/supply/max77705_charger.c | 599 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ drivers/power/supply/power_supply_sysfs.c | 1 + include/linux/mfd/max77693-common.h | 5 +- include/linux/mfd/max77705-private.h | 199 ++++++++++++++++++++++++++++++++++++++ include/linux/power/max77705_charger.h | 216 +++++++++++++++++++++++++++++++++++++++++ include/linux/power_supply.h | 1 + 22 files changed, 1638 insertions(+), 6 deletions(-) --- base-commit: f486c8aa16b8172f63bddc70116a0c897a7f3f02 change-id: 20240617-starqltechn_integration_upstream-bc86850b2fe3 prerequisite-change-id: 20241108-b4-max17042-9306fc75afae:v4 prerequisite-patch-id: a78c51c4a1b48756c00cbc3d56b9e019577e4a6b prerequisite-patch-id: 735d52c3137c5e474f3601adf010f9fe2f3f7036 Best regards,