* [PATCH v5 0/2] Add support for MAX77675 device
@ 2025-10-29 2:32 Joan-Na-adi
2025-10-29 2:32 ` [PATCH v5 1/2] regulator: dt-bindings: Add MAX77675 regulator binding Joan-Na-adi
2025-10-29 2:32 ` [PATCH v5 2/2] regulator: max77675: Add MAX77675 regulator driver Joan-Na-adi
0 siblings, 2 replies; 8+ messages in thread
From: Joan-Na-adi @ 2025-10-29 2:32 UTC (permalink / raw)
To: Liam Girdwood
Cc: Mark Brown, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
linux-kernel, devicetree, Joan Na
From: Joan Na <joan.na@analog.com>
MAX77675 regulator driver and device tree bindings
---
Changes in v5:
- Fix subject prefix order to match subsystem ('regulator: dt-bindings: ...')
- Drop unnecessary '|' from description fields
- Drop repeated constraint descriptions from YAML
- Rename 'maxim,en-debounce-time-us' to 'maxim,debounce-delay-us' for clearer naming
- Used common 'reset-time-sec' property from input.yaml
- Remove duplicate description for 'maxim,fps-slot' and wrap lines to 80 chars
- Drop the node reference immediately after registering the regulator
Changes in v4:
- Remove the 'maxim,max77675-regulator.h' file as it is no longer used for bindings
- Eliminate unnecessary '|' characters where they are not needed
- Add and modify code to drop references that are no longer used
- Remove dead code
- Add detailed descriptions for each mode of 'maxim,en-mode'
- Rename 'maxim,latency-mode' to 'maxim,voltage-change-latency-us' for clearer meaning
- Update max77675_parse_latency_mode function to max77675_parse_voltage_change_latency accordingly
- Fix errors detected by running make dt_binding_check
- Fix incorrect indentation in the YAML file
Changes in v3:
- Removed unused variable 'value'
- Removed duplicate .list_voltage initializer
- Wrapped of_match_table with of_match_ptr() to fix build failure when CONFIG_OF is not set
- Updated driver code to match new DT binding schema
- Changed regmap_config from REGCACHE_NONE to REGCACHE_MAPLE for improved performance
- Added volatile_reg() to mark status registers as non-cacheable
- Missing explanation of `maxim,fps-slot` default value
- Updated DT binding enums to use string values (e.g., "low", "high") instead of integers
- Converted several binary properties to boolean typei
- Renamed time-based properties to use standard unit suffixes (e.g., "-sec", "-us")
- Added default values for properties
- Removed unused macros
- Renamed macros for clarity
Changes in v2:
- Fixed build error due to missing 'max77675_of_match' declaration
- Removed duplicate '.list_voltage' initialization
- Corrected value usage in regmap_update_bits call
- Added CONFIG_OF guards and used of_match_ptr()
---
Joan Na (2):
regulator: dt-bindings: Add MAX77675 regulator binding
regulator: max77675: Add MAX77675 regulator driver
.../bindings/regulator/maxim,max77675.yaml | 186 ++++
drivers/regulator/Kconfig | 9 +
drivers/regulator/Makefile | 1 +
drivers/regulator/max77675-regulator.c | 861 ++++++++++++++++++
drivers/regulator/max77675-regulator.h | 260 ++++++
5 files changed, 1317 insertions(+)
create mode 100644 Documentation/devicetree/bindings/regulator/maxim,max77675.yaml
create mode 100644 drivers/regulator/max77675-regulator.c
create mode 100644 drivers/regulator/max77675-regulator.h
base-commit: 6548d364a3e850326831799d7e3ea2d7bb97ba08
--
2.34.1
^ permalink raw reply [flat|nested] 8+ messages in thread* [PATCH v5 1/2] regulator: dt-bindings: Add MAX77675 regulator binding 2025-10-29 2:32 [PATCH v5 0/2] Add support for MAX77675 device Joan-Na-adi @ 2025-10-29 2:32 ` Joan-Na-adi 2025-10-29 5:52 ` Krzysztof Kozlowski 2025-10-29 2:32 ` [PATCH v5 2/2] regulator: max77675: Add MAX77675 regulator driver Joan-Na-adi 1 sibling, 1 reply; 8+ messages in thread From: Joan-Na-adi @ 2025-10-29 2:32 UTC (permalink / raw) To: Liam Girdwood Cc: Mark Brown, Rob Herring, Krzysztof Kozlowski, Conor Dooley, linux-kernel, devicetree, Joan Na From: Joan Na <joan.na@analog.com> Add device tree binding YAML schema for the Maxim MAX77675 PMIC regulator. This defines the node properties and supported regulator names for use in device tree sources. Signed-off-by: Joan Na <joan.na@analog.com> --- .../bindings/regulator/maxim,max77675.yaml | 186 ++++++++++++++++++ 1 file changed, 186 insertions(+) create mode 100644 Documentation/devicetree/bindings/regulator/maxim,max77675.yaml diff --git a/Documentation/devicetree/bindings/regulator/maxim,max77675.yaml b/Documentation/devicetree/bindings/regulator/maxim,max77675.yaml new file mode 100644 index 000000000000..5d4a643ea5b1 --- /dev/null +++ b/Documentation/devicetree/bindings/regulator/maxim,max77675.yaml @@ -0,0 +1,186 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/regulator/maxim,max77675.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Maxim MAX77675 PMIC Regulator + +maintainers: + - Joan Na <joan.na@analog.com> + +description: + The MAX77675 is a Power Management IC providing four switching buck + regulators (SBB0–SBB3) accessible via I2C. It supports configuration + of output voltages and enable controls for each regulator. + +allOf: + - $ref: /schemas/input/input.yaml + +properties: + compatible: + const: maxim,max77675 + + reg: + maxItems: 1 + + reset-time-sec: + description: Manual reset time in seconds + enum: [4, 8, 12, 16] + default: 4 + + maxim,en-mode: + description: | + Enable mode configuration. + The debounce time set by 'maxim,debounce-time-us' applies to + both push-button and slide-switch modes. + "push-button" - A long press triggers power-on or power-down + "slide-switch" - Low level powers on, high level powers down + "logic" - Low level powers on, high level powers down (no debounce time) + $ref: /schemas/types.yaml#/definitions/string + enum: [push-button, slide-switch, logic] + default: slide-switch + + maxim,voltage-change-latency-us: + description: + Specifies the delay (in microseconds) between an output voltage change request + and the start of the SBB voltage ramp. + enum: [10, 100] + default: 100 + + maxim,drv-sbb-strength: + description: | + SIMO Buck-Boost Drive Strength Trim. + Controls the drive strength of the SIMO regulator's power MOSFETs. + This setting affects the switching speed, which impacts power efficiency and EMI. + "max" – Maximum drive strength (~0.6 ns transition time) + "high" – High drive strength (~1.2 ns transition time) + "low" – Low drive strength (~1.8 ns transition time) + "min" – Minimum drive strength (~8 ns transition time) + $ref: /schemas/types.yaml#/definitions/string + enum: [max, high, low, min] + default: max + + maxim,dvs-slew-rate-mv-per-us: + description: + Dynamic rising slew rate for output voltage transitions, in mV/μs. + This setting is only used when 'maxim,fixed-slew-rate' is not present. + enum: [5, 10] + default: 5 + + maxim,debounce-time-us: + description: Debounce time for the enable pin, in microseconds + enum: [100, 30000] + default: 100 + + maxim,en-pullup-disable: + type: boolean + description: Disable internal pull-up for EN pin + default: false + + maxim,bias-low-power-request: + type: boolean + description: Request low-power bias mode + default: false + + maxim,simo-int-ldo-always-on: + type: boolean + description: Set internal LDO to always supply 1.8V + default: false + + regulators: + type: object + description: Regulator child nodes + patternProperties: + "^sbb[0-3]$": + type: object + $ref: regulator.yaml# + properties: + maxim,fps-slot: + description: | + FPS (Flexible Power Sequencer) slot selection. + The Flexible Power Sequencer allows resources to power up under + hardware or software control. Additionally, each resource can + power up independently or among a group of other regulators with + adjustable power-up and power-down slots. + "slot0" - Assign to FPS Slot 0 + "slot1" - Assign to FPS Slot 1 + "slot2" - Assign to FPS Slot 2 + "slot3" - Assign to FPS Slot 3 + "default" - Use the default FPS slot value stored in OTP and read from the register + $ref: /schemas/types.yaml#/definitions/string + enum: [slot0, slot1, slot2, slot3, default] + default: default + + maxim,fixed-slew-rate: + type: boolean + description: + When this property is present, the device uses a constant 2 mV/μs + slew rate and ignores any dynamic slew rate configuration. + When absent, the device uses the dynamic slew rate specified + by 'maxim,dvs-slew-rate-mv-per-us' + default: true + + unevaluatedProperties: false + +required: + - compatible + - reg + - regulators + +additionalProperties: false + +examples: + - | + i2c { + #address-cells = <1>; + #size-cells = <0>; + + max77675: pmic@44 { + compatible = "maxim,max77675"; + reg = <0x44>; + + reset-time-sec = <4>; + + maxim,en-mode = "slide-switch"; + maxim,voltage-change-latency-us = <100>; + maxim,drv-sbb-strength = "max"; + maxim,dvs-slew-rate-mv-per-us = <5>; + maxim,debounce-time-us = <100>; + + regulators { + sbb0: sbb0 { + regulator-name = "sbb0"; + regulator-min-microvolt = <500000>; + regulator-max-microvolt = <5500000>; + maxim,fps-slot = "default"; + maxim,fixed-slew-rate; + }; + + sbb1: sbb1 { + regulator-name = "sbb1"; + regulator-min-microvolt = <500000>; + regulator-max-microvolt = <5500000>; + maxim,fps-slot = "default"; + maxim,fixed-slew-rate; + }; + + sbb2: sbb2 { + regulator-name = "sbb2"; + regulator-min-microvolt = <500000>; + regulator-max-microvolt = <5500000>; + maxim,fps-slot = "default"; + maxim,fixed-slew-rate; + }; + + sbb3: sbb3 { + regulator-name = "sbb3"; + regulator-min-microvolt = <500000>; + regulator-max-microvolt = <5500000>; + maxim,fps-slot = "default"; + maxim,fixed-slew-rate; + }; + }; + }; + }; + -- 2.34.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v5 1/2] regulator: dt-bindings: Add MAX77675 regulator binding 2025-10-29 2:32 ` [PATCH v5 1/2] regulator: dt-bindings: Add MAX77675 regulator binding Joan-Na-adi @ 2025-10-29 5:52 ` Krzysztof Kozlowski 2025-10-31 11:24 ` Joan Na 0 siblings, 1 reply; 8+ messages in thread From: Krzysztof Kozlowski @ 2025-10-29 5:52 UTC (permalink / raw) To: Joan-Na-adi Cc: Liam Girdwood, Mark Brown, Rob Herring, Krzysztof Kozlowski, Conor Dooley, linux-kernel, devicetree, Joan Na On Wed, Oct 29, 2025 at 11:32:52AM +0900, Joan-Na-adi wrote: > From: Joan Na <joan.na@analog.com> > > Add device tree binding YAML schema for the Maxim MAX77675 PMIC regulator. > This defines the node properties and supported regulator names for use > in device tree sources. My comments form v3 were still not applied. I pointed out this at v4. You still did not reply to comments. Did not implement them, either. <form letter> This is a friendly reminder during the review process. It seems my or other reviewer's previous comments were not fully addressed. Maybe the feedback got lost between the quotes, maybe you just forgot to apply it. Please go back to the previous discussion and either implement all requested changes or keep discussing them. Thank you. </form letter> Best regards, Krzysztof ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v5 1/2] regulator: dt-bindings: Add MAX77675 regulator binding 2025-10-29 5:52 ` Krzysztof Kozlowski @ 2025-10-31 11:24 ` Joan Na 0 siblings, 0 replies; 8+ messages in thread From: Joan Na @ 2025-10-31 11:24 UTC (permalink / raw) To: Krzysztof Kozlowski Cc: Liam Girdwood, Mark Brown, Rob Herring, Krzysztof Kozlowski, Conor Dooley, linux-kernel, devicetree, Joan Na On Wed, Oct 29, 2025 at 06:52:48AM +0100, Krzysztof Kozlowski wrote: > On Wed, Oct 29, 2025 at 11:32:52AM +0900, Joan-Na-adi wrote: > > From: Joan Na <joan.na@analog.com> > > > > Add device tree binding YAML schema for the Maxim MAX77675 PMIC regulator. > > This defines the node properties and supported regulator names for use > > in device tree sources. > > My comments form v3 were still not applied. I pointed out this at v4. > > You still did not reply to comments. Did not implement them, either. > > <form letter> > This is a friendly reminder during the review process. > > It seems my or other reviewer's previous comments were not fully > addressed. Maybe the feedback got lost between the quotes, maybe you > just forgot to apply it. Please go back to the previous discussion and > either implement all requested changes or keep discussing them. > > Thank you. > </form letter> > > Best regards, > Krzysztof > Thank you for pointing this out. I apologize for missing your comments from v3 and v4. I’ll make sure to review them carefully and address each point in the next update. I appreciate your patience. -- Best regards, Joan Na ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v5 2/2] regulator: max77675: Add MAX77675 regulator driver 2025-10-29 2:32 [PATCH v5 0/2] Add support for MAX77675 device Joan-Na-adi 2025-10-29 2:32 ` [PATCH v5 1/2] regulator: dt-bindings: Add MAX77675 regulator binding Joan-Na-adi @ 2025-10-29 2:32 ` Joan-Na-adi 2025-10-29 9:55 ` Nuno Sá 1 sibling, 1 reply; 8+ messages in thread From: Joan-Na-adi @ 2025-10-29 2:32 UTC (permalink / raw) To: Liam Girdwood Cc: Mark Brown, Rob Herring, Krzysztof Kozlowski, Conor Dooley, linux-kernel, devicetree, Joan Na From: Joan Na <joan.na@analog.com> Add support for the Maxim Integrated MAX77675 PMIC regulator. The MAX77675 is a compact, highly efficient SIMO (Single Inductor Multiple Output) power management IC that provides four programmable buck-boost switching regulators with only one inductor. It supports up to 700mA total output current and operates from a single-cell Li-ion battery. An integrated power-up sequencer and I2C interface allow flexible startup configuration and runtime control. Signed-off-by: Joan Na <joan.na@analog.com> --- drivers/regulator/Kconfig | 9 + drivers/regulator/Makefile | 1 + drivers/regulator/max77675-regulator.c | 861 +++++++++++++++++++++++++ drivers/regulator/max77675-regulator.h | 260 ++++++++ 4 files changed, 1131 insertions(+) create mode 100644 drivers/regulator/max77675-regulator.c create mode 100644 drivers/regulator/max77675-regulator.h diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig index d84f3d054c59..93131446e402 100644 --- a/drivers/regulator/Kconfig +++ b/drivers/regulator/Kconfig @@ -649,6 +649,15 @@ config REGULATOR_MAX77650 Semiconductor. This device has a SIMO with three independent power rails and an LDO. +config REGULATOR_MAX77675 + tristate "Maxim MAX77675 regulator driver" + depends on I2C + select REGMAP_I2C + help + This driver controls the Maxim MAX77675 power regulator via I2C. + It supports four programmable buck-boost outputs. + Say Y here to enable the regulator driver + config REGULATOR_MAX77857 tristate "ADI MAX77857/MAX77831 regulator support" depends on I2C diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile index b3101376029d..cdd99669cd24 100644 --- a/drivers/regulator/Makefile +++ b/drivers/regulator/Makefile @@ -78,6 +78,7 @@ obj-$(CONFIG_REGULATOR_MAX77503) += max77503-regulator.o obj-$(CONFIG_REGULATOR_MAX77541) += max77541-regulator.o obj-$(CONFIG_REGULATOR_MAX77620) += max77620-regulator.o obj-$(CONFIG_REGULATOR_MAX77650) += max77650-regulator.o +obj-$(CONFIG_REGULATOR_MAX77675) += max77675-regulator.o obj-$(CONFIG_REGULATOR_MAX8649) += max8649.o obj-$(CONFIG_REGULATOR_MAX8660) += max8660.o obj-$(CONFIG_REGULATOR_MAX8893) += max8893.o diff --git a/drivers/regulator/max77675-regulator.c b/drivers/regulator/max77675-regulator.c new file mode 100644 index 000000000000..c1281f07fe43 --- /dev/null +++ b/drivers/regulator/max77675-regulator.c @@ -0,0 +1,861 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +/* + * Copyright (c) 2025 Analog Devices, Inc. + * ADI regulator driver for MAX77675. + */ + +#include <linux/module.h> +#include <linux/slab.h> +#include <linux/of.h> +#include <linux/i2c.h> +#include <linux/regmap.h> +#include <linux/platform_device.h> +#include <linux/regulator/driver.h> +#include <linux/regulator/consumer.h> +#include <linux/regulator/of_regulator.h> +#include <linux/bitfield.h> + +#include "max77675-regulator.h" + +struct max77675_regulator_pdata { + u8 fps_slot; + bool fixed_slew_rate; +}; + +struct max77675_config { + u8 en_mode; + u8 voltage_change_latency; + u8 drv_sbb_strength; + u8 dvs_slew_rate; + u8 debounce_time; + u8 manual_reset_time; + bool en_pullup_disable; + bool bias_low_power_request; + bool simo_int_ldo_always_on; +}; + +struct max77675_regulator { + struct device *dev; + struct regmap *regmap; + struct max77675_config config; + struct max77675_regulator_pdata pdata[MAX77675_ID_NUM_MAX]; +}; + +/** + * Set latency mode. + * + * @param maxreg Pointer to max77675 device structure. + * @param enable true to enable latency mode, false to disable. + */ +static int max77675_set_latency_mode(struct max77675_regulator *maxreg, bool enable) +{ + return regmap_update_bits(maxreg->regmap, MAX77675_REG_CNFG_SBB_TOP_B, + MAX77675_LAT_MODE_BIT, + FIELD_PREP(MAX77675_LAT_MODE_BIT, enable)); +} + +/** + * Set DVS slew rate mode. + * + * @param maxreg Pointer to max77675 device structure. + * @param enable true to use DVS-controlled slew rate, false for fixed 2mV/us. + */ +static int max77675_set_dvs_slew_rate(struct max77675_regulator *maxreg, bool enable) +{ + return regmap_update_bits(maxreg->regmap, MAX77675_REG_CNFG_SBB_TOP_B, + MAX77675_DVS_SLEW_BIT, + FIELD_PREP(MAX77675_DVS_SLEW_BIT, enable)); +} + +/** + * Set drive strength. + * + * @param maxreg Pointer to max77675 device structure. + * @param strength 2-bit drive strength value (0-3). + * + * @return 0 on success, negative error code on failure. + */ +static int max77675_set_drv_sbb_strength(struct max77675_regulator *maxreg, u8 strength) +{ + return regmap_update_bits(maxreg->regmap, MAX77675_REG_CNFG_SBB_TOP_A, + MAX77675_DRV_SBB_MASK, + FIELD_PREP(MAX77675_DRV_SBB_MASK, strength)); +} + +/** + * Set manual reset time (MRT) for EN pin. + * + * @param maxreg Pointer to max77675 device structure. + * @param mrt 2-bit value (0x0: 4s, 0x1: 8s, 0x2: 12s, 0x3: 16s) + */ +static int max77675_set_manual_reset_time(struct max77675_regulator *maxreg, u8 mrt) +{ + return regmap_update_bits(maxreg->regmap, MAX77675_REG_CNFG_GLBL_A, + MAX77675_MRT_MASK, + FIELD_PREP(MAX77675_MRT_MASK, mrt)); +} + +/** + * Enable or disable internal pull-up resistor on EN pin. + * + * @param maxreg Pointer to max77675 device structure. + * @param disable true to disable pull-up, false to enable + */ +static int max77675_set_en_pullup_disable(struct max77675_regulator *maxreg, bool disable) +{ + return regmap_update_bits(maxreg->regmap, MAX77675_REG_CNFG_GLBL_A, + MAX77675_PU_DIS_BIT, + FIELD_PREP(MAX77675_PU_DIS_BIT, disable)); +} + +/** + * Request main bias to enter low-power mode. + * + * @param maxreg Pointer to max77675 device structure. + * @param enable true to request low-power mode, false for normal + */ +static int max77675_set_bias_low_power_request(struct max77675_regulator *maxreg, bool enable) +{ + return regmap_update_bits(maxreg->regmap, MAX77675_REG_CNFG_GLBL_A, + MAX77675_BIAS_LPM_BIT, + FIELD_PREP(MAX77675_BIAS_LPM_BIT, enable)); +} + +/** + * Force SIMO internal LDO to always supply 1.8V. + * + * @param maxreg Pointer to max77675 device structure. + * @param enable true to always supply 1.8V, false for normal operation + */ +static int max77675_set_simo_int_ldo_always_on(struct max77675_regulator *maxreg, bool enable) +{ + return regmap_update_bits(maxreg->regmap, MAX77675_REG_CNFG_GLBL_A, + MAX77675_SIMO_CH_DIS_BIT, + FIELD_PREP(MAX77675_SIMO_CH_DIS_BIT, enable)); +} + +/** + * Set EN pin mode. + * + * @param maxreg Pointer to max77675 device structure. + * @param mode 2-bit value: 0x0 (push-button), 0x1 (slide-switch), 0x2 (logic) + */ +static int max77675_set_en_mode(struct max77675_regulator *maxreg, u8 mode) +{ + return regmap_update_bits(maxreg->regmap, MAX77675_REG_CNFG_GLBL_A, + MAX77675_EN_MODE_MASK, + FIELD_PREP(MAX77675_EN_MODE_MASK, mode)); +} + +/** + * Set debounce time for EN pin. + * + * @param maxreg Pointer to max77675 device structure. + * @param debounce_30ms true for 30ms, false for 100us + */ +static int max77675_set_debounce_time(struct max77675_regulator *maxreg, bool debounce_30ms) +{ + return regmap_update_bits(maxreg->regmap, MAX77675_REG_CNFG_GLBL_A, + MAX77675_DBEN_EN_BIT, + FIELD_PREP(MAX77675_DBEN_EN_BIT, debounce_30ms)); +} + +static int max77675_regulator_get_fps_src(struct max77675_regulator *maxreg, int id) +{ + unsigned int reg_addr; + unsigned int val; + int ret; + + switch (id) { + case MAX77675_ID_SBB0: + reg_addr = MAX77675_REG_CNFG_SBB0_B; + break; + case MAX77675_ID_SBB1: + reg_addr = MAX77675_REG_CNFG_SBB1_B; + break; + case MAX77675_ID_SBB2: + reg_addr = MAX77675_REG_CNFG_SBB2_B; + break; + case MAX77675_ID_SBB3: + reg_addr = MAX77675_REG_CNFG_SBB3_B; + break; + default: + dev_err(maxreg->dev, "Invalid regulator id: %d\n", id); + return -EINVAL; + } + + ret = regmap_read(maxreg->regmap, reg_addr, &val); + if (ret < 0) { + dev_err(maxreg->dev, "Failed to read FPS source (reg 0x%02x): %d\n", + reg_addr, ret); + return ret; + } + + return val & MAX77675_EN_SBB_MASK; +} + +static int max77675_regulator_set_fps_src(struct max77675_regulator *maxreg, int id, u8 fps_src) +{ + unsigned int reg_addr; + int ret; + + switch (id) { + case MAX77675_ID_SBB0: + reg_addr = MAX77675_REG_CNFG_SBB0_B; + break; + case MAX77675_ID_SBB1: + reg_addr = MAX77675_REG_CNFG_SBB1_B; + break; + case MAX77675_ID_SBB2: + reg_addr = MAX77675_REG_CNFG_SBB2_B; + break; + case MAX77675_ID_SBB3: + reg_addr = MAX77675_REG_CNFG_SBB3_B; + break; + default: + dev_err(maxreg->dev, "Invalid regulator id: %d\n", id); + return -EINVAL; + } + + ret = regmap_update_bits(maxreg->regmap, reg_addr, + MAX77675_EN_SBB_MASK, fps_src); + if (ret < 0) { + dev_err(maxreg->dev, "Failed to set FPS source (reg 0x%02x): %d\n", + reg_addr, ret); + return ret; + } + + return 0; +} + +/** + * max77675_set_sbb_slew_rate_fixed - Set the slew rate for a specific SBB regulator channel + * + * @maxreg: Pointer to the max77675 regulator structure + * @id: Regulator channel ID (ID_SBB0 ~ ID_SBB3) + * @fixed: Slew rate value (true = 2mV/us, false = use DVS_SLEW) + * + * This function configures the slew rate control source for the specified SBB channel by + * updating the corresponding bits in the CNFG_SBB_TOP_B register. + * + * Return: 0 on success, negative error code on failure (e.g., invalid channel ID). + */ +static int max77675_set_sbb_slew_rate_fixed(struct max77675_regulator *maxreg, int id, bool fixed) +{ + u8 mask, value; + u8 slew_src_ctrl_bit = fixed ? 0 : 1; + + switch (id) { + case MAX77675_ID_SBB0: + mask = MAX77675_SR_SBB0_BIT; + value = FIELD_PREP(MAX77675_SR_SBB0_BIT, slew_src_ctrl_bit); + break; + + case MAX77675_ID_SBB1: + mask = MAX77675_SR_SBB1_BIT; + value = FIELD_PREP(MAX77675_SR_SBB1_BIT, slew_src_ctrl_bit); + break; + + case MAX77675_ID_SBB2: + mask = MAX77675_SR_SBB2_BIT; + value = FIELD_PREP(MAX77675_SR_SBB2_BIT, slew_src_ctrl_bit); + break; + + case MAX77675_ID_SBB3: + mask = MAX77675_SR_SBB3_BIT; + value = FIELD_PREP(MAX77675_SR_SBB3_BIT, slew_src_ctrl_bit); + break; + + default: + return -EINVAL; + } + + return regmap_update_bits(maxreg->regmap, MAX77675_REG_CNFG_SBB_TOP_B, mask, value); +} + +static int max77675_init_regulator(struct max77675_regulator *maxreg, int id) +{ + struct max77675_regulator_pdata *rpdata = &maxreg->pdata[id]; + int ret; + + if (rpdata->fps_slot == MAX77675_FPS_DEF) { + ret = max77675_regulator_get_fps_src(maxreg, id); + if (ret < 0) { + dev_err(maxreg->dev, "Failed to read FPS source for ID %d\n", id); + return ret; + } + rpdata->fps_slot = ret; + } else { + ret = max77675_regulator_set_fps_src(maxreg, id, rpdata->fps_slot); + if (ret) + dev_warn(maxreg->dev, "Failed to set FPS source for ID %d\n", id); + } + + ret = max77675_set_sbb_slew_rate_fixed(maxreg, id, rpdata->fixed_slew_rate); + if (ret) + dev_warn(maxreg->dev, "Failed to set slew rate for ID %d\n", id); + + return 0; +} + +static int max77675_of_parse_cb(struct device_node *np, + const struct regulator_desc *desc, + struct regulator_config *config) +{ + struct max77675_regulator *maxreg = config->driver_data; + struct max77675_regulator_pdata *rpdata = &maxreg->pdata[desc->id]; + u32 pval; + int ret; + + /* Parse FPS slot from DT */ + ret = of_property_read_u32(np, "maxim,fps-slot", &pval); + rpdata->fps_slot = (!ret) ? (u8)pval : MAX77675_FPS_DEF; + + /* Parse slew rate control source */ + rpdata->fixed_slew_rate = of_property_read_bool(np, "maxim,fixed-slew-rate"); + + /* Apply parsed configuration */ + return max77675_init_regulator(maxreg, desc->id); +} + +static int max77675_get_error_flags(struct regulator_dev *rdev, unsigned int *flags) +{ + struct max77675_regulator *maxreg = rdev_get_drvdata(rdev); + unsigned int int_flags; + int id = rdev_get_id(rdev); + int ret; + + ret = regmap_read(maxreg->regmap, MAX77675_REG_INT_GLBL, &int_flags); + if (ret) { + dev_err(maxreg->dev, "Failed to read INT_GLBL: %d\n", ret); + return ret; + } + + *flags = 0; + + switch (id) { + case MAX77675_ID_SBB0: + if (int_flags & MAX77675_INT_SBB0_F_BIT) + *flags |= REGULATOR_ERROR_FAIL; + break; + case MAX77675_ID_SBB1: + if (int_flags & MAX77675_INT_SBB1_F_BIT) + *flags |= REGULATOR_ERROR_FAIL; + break; + case MAX77675_ID_SBB2: + if (int_flags & MAX77675_INT_SBB2_F_BIT) + *flags |= REGULATOR_ERROR_FAIL; + break; + case MAX77675_ID_SBB3: + if (int_flags & MAX77675_INT_SBB3_F_BIT) + *flags |= REGULATOR_ERROR_FAIL; + break; + default: + dev_warn(maxreg->dev, "Unsupported regulator ID: %d\n", id); + break; + } + + if (int_flags & MAX77675_INT_TJAL2_R_BIT) { + /* TJAL2 interrupt: Over-temperature condition (above 120 degree) */ + *flags |= REGULATOR_ERROR_OVER_TEMP; + } + + return 0; +} + +static const struct regulator_ops max77675_regulator_ops = { + .list_voltage = regulator_list_voltage_linear, + .enable = regulator_enable_regmap, + .disable = regulator_disable_regmap, + .is_enabled = regulator_is_enabled_regmap, + .map_voltage = regulator_map_voltage_linear, + .set_voltage_sel = regulator_set_voltage_sel_regmap, + .get_voltage_sel = regulator_get_voltage_sel_regmap, + .set_active_discharge = regulator_set_active_discharge_regmap, + .get_error_flags = max77675_get_error_flags, +}; + +static struct regulator_desc max77675_regulators[MAX77675_ID_NUM_MAX] = { + { + .name = "sbb0", + .of_match = of_match_ptr("sbb0"), + .regulators_node = of_match_ptr("regulators"), + .of_parse_cb = max77675_of_parse_cb, + .id = MAX77675_ID_SBB0, + .ops = &max77675_regulator_ops, + .type = REGULATOR_VOLTAGE, + .owner = THIS_MODULE, + .n_voltages = MAX77675_NUM_LEVELS_25MV, + .min_uV = MAX77675_MIN_UV, + .uV_step = MAX77675_STEP_25MV, + .vsel_reg = MAX77675_REG_CNFG_SBB0_A, + .vsel_mask = MAX77675_TV_SBB0_MASK, + .enable_reg = MAX77675_REG_CNFG_SBB0_B, + .enable_mask = MAX77675_EN_SBB0_MASK, + .enable_val = MAX77675_ENABLE_ON, + .disable_val = MAX77675_ENABLE_OFF, + .active_discharge_off = MAX77675_REGULATOR_AD_OFF, + .active_discharge_on = MAX77675_REGULATOR_AD_ON, + .active_discharge_mask = MAX77675_ADE_SBB0_BIT, + .active_discharge_reg = MAX77675_REG_CNFG_SBB0_B, + }, + { + .name = "sbb1", + .of_match = of_match_ptr("sbb1"), + .regulators_node = of_match_ptr("regulators"), + .of_parse_cb = max77675_of_parse_cb, + .id = MAX77675_ID_SBB1, + .ops = &max77675_regulator_ops, + .type = REGULATOR_VOLTAGE, + .owner = THIS_MODULE, + .n_voltages = MAX77675_NUM_LEVELS_25MV, + .min_uV = MAX77675_MIN_UV, + .uV_step = MAX77675_STEP_25MV, + .vsel_reg = MAX77675_REG_CNFG_SBB1_A, + .vsel_mask = MAX77675_TV_SBB1_MASK, + .enable_reg = MAX77675_REG_CNFG_SBB1_B, + .enable_mask = MAX77675_EN_SBB1_MASK, + .enable_val = MAX77675_ENABLE_ON, + .disable_val = MAX77675_ENABLE_OFF, + .active_discharge_off = MAX77675_REGULATOR_AD_OFF, + .active_discharge_on = MAX77675_REGULATOR_AD_ON, + .active_discharge_mask = MAX77675_ADE_SBB1_BIT, + .active_discharge_reg = MAX77675_REG_CNFG_SBB1_B, + }, + { + .name = "sbb2", + .of_match = of_match_ptr("sbb2"), + .regulators_node = of_match_ptr("regulators"), + .of_parse_cb = max77675_of_parse_cb, + .id = MAX77675_ID_SBB2, + .ops = &max77675_regulator_ops, + .type = REGULATOR_VOLTAGE, + .owner = THIS_MODULE, + .n_voltages = MAX77675_NUM_LEVELS_25MV, + .min_uV = MAX77675_MIN_UV, + .uV_step = MAX77675_STEP_25MV, + .vsel_reg = MAX77675_REG_CNFG_SBB2_A, + .vsel_mask = MAX77675_TV_SBB2_MASK, + .enable_reg = MAX77675_REG_CNFG_SBB2_B, + .enable_mask = MAX77675_EN_SBB2_MASK, + .enable_val = MAX77675_ENABLE_ON, + .disable_val = MAX77675_ENABLE_OFF, + .active_discharge_off = MAX77675_REGULATOR_AD_OFF, + .active_discharge_on = MAX77675_REGULATOR_AD_ON, + .active_discharge_mask = MAX77675_ADE_SBB2_BIT, + .active_discharge_reg = MAX77675_REG_CNFG_SBB2_B, + }, + { + .name = "sbb3", + .of_match = of_match_ptr("sbb3"), + .regulators_node = of_match_ptr("regulators"), + .of_parse_cb = max77675_of_parse_cb, + .id = MAX77675_ID_SBB3, + .ops = &max77675_regulator_ops, + .type = REGULATOR_VOLTAGE, + .owner = THIS_MODULE, + .n_voltages = MAX77675_NUM_LEVELS_25MV, + .min_uV = MAX77675_MIN_UV, + .uV_step = MAX77675_STEP_25MV, + .vsel_reg = MAX77675_REG_CNFG_SBB3_A, + .vsel_mask = MAX77675_TV_SBB3_MASK, + .enable_reg = MAX77675_REG_CNFG_SBB3_B, + .enable_mask = MAX77675_EN_SBB3_MASK, + .enable_val = MAX77675_ENABLE_ON, + .disable_val = MAX77675_ENABLE_OFF, + .active_discharge_off = MAX77675_REGULATOR_AD_OFF, + .active_discharge_on = MAX77675_REGULATOR_AD_ON, + .active_discharge_mask = MAX77675_ADE_SBB3_BIT, + .active_discharge_reg = MAX77675_REG_CNFG_SBB3_B, + }, +}; + +static bool max77675_volatile_reg(struct device *dev, unsigned int reg) +{ + switch (reg) { + case MAX77675_REG_CNFG_GLBL_B: + /* This register can be updated by an internal state machine */ + case MAX77675_REG_INT_GLBL: + case MAX77675_REG_STAT_GLBL: + case MAX77675_REG_ERCF_GLBL: + return true; + default: + return false; + } +} + +static const struct regmap_config max77675_regmap_config = { + .reg_bits = 8, + .val_bits = 8, + .max_register = MAX77675_MAX_REGISTER, + .cache_type = REGCACHE_MAPLE, + .volatile_reg = max77675_volatile_reg, +}; + +static int max77675_apply_config(struct max77675_regulator *maxreg) +{ + const struct max77675_config *config = &maxreg->config; + int ret; + + ret = max77675_set_en_mode(maxreg, config->en_mode); + if (ret) { + dev_err(maxreg->dev, "Failed to set EN mode: %d\n", ret); + return ret; + } + + ret = max77675_set_latency_mode(maxreg, config->voltage_change_latency); + if (ret) { + dev_err(maxreg->dev, "Failed to set latency mode: %d\n", ret); + return ret; + } + + ret = max77675_set_drv_sbb_strength(maxreg, config->drv_sbb_strength); + if (ret) { + dev_err(maxreg->dev, "Failed to set drive strength: %d\n", ret); + return ret; + } + + ret = max77675_set_dvs_slew_rate(maxreg, config->dvs_slew_rate); + if (ret) { + dev_err(maxreg->dev, "Failed to set DVS slew rate: %d\n", ret); + return ret; + } + + ret = max77675_set_debounce_time(maxreg, config->debounce_time); + if (ret) { + dev_err(maxreg->dev, "Failed to set EN debounce time: %d\n", ret); + return ret; + } + + ret = max77675_set_manual_reset_time(maxreg, config->manual_reset_time); + if (ret) { + dev_err(maxreg->dev, "Failed to set manual reset time: %d\n", ret); + return ret; + } + + ret = max77675_set_en_pullup_disable(maxreg, config->en_pullup_disable); + if (ret) { + dev_err(maxreg->dev, "Failed to set EN pull-up disable: %d\n", ret); + return ret; + } + + ret = max77675_set_bias_low_power_request(maxreg, config->bias_low_power_request); + if (ret) { + dev_err(maxreg->dev, "Failed to set bias low-power request: %d\n", ret); + return ret; + } + + ret = max77675_set_simo_int_ldo_always_on(maxreg, config->simo_int_ldo_always_on); + if (ret) { + dev_err(maxreg->dev, "Failed to set SIMO internal LDO always-on: %d\n", ret); + return ret; + } + + return 0; +} + +static u8 max77675_parse_voltage_change_latency(struct device_node *np) +{ + u32 val; + + if (!of_property_read_u32(np, "maxim,voltage-change-latency-us", &val)) { + switch (val) { + case 10: + return MAX77675_LOW_LATENCY_MODE; + case 100: + return MAX77675_HIGH_LATENCY_MODE; + default: + break; + } + } + + /* default: high latency */ + return MAX77675_HIGH_LATENCY_MODE; +} + +static u8 max77675_parse_en_mode(struct device_node *np) +{ + const char *str; + + if (!of_property_read_string(np, "maxim,en-mode", &str)) { + if (!strcasecmp(str, "push-button")) + return MAX77675_EN_PUSH_BUTTON; + else if (!strcasecmp(str, "slide-switch")) + return MAX77675_EN_SLIDE_SWITCH; + else if (!strcasecmp(str, "logic")) + return MAX77675_EN_LOGIC; + } + + /* default : slide-switch */ + return MAX77675_EN_SLIDE_SWITCH; +} + +static u8 max77675_parse_manual_reset_time(struct device_node *np) +{ + u32 val; + + if (!of_property_read_u32(np, "reset-time-sec", &val)) { + switch (val) { + case 4: + return MAX77675_MRT_4S; + case 8: + return MAX77675_MRT_8S; + case 12: + return MAX77675_MRT_12S; + case 16: + return MAX77675_MRT_16S; + default: + break; + } + } + + /* default : 4 seconds */ + return MAX77675_MRT_4S; +} + +static u8 max77675_parse_dvs_slew_rate(struct device_node *np) +{ + u32 val; + + if (!of_property_read_u32(np, "maxim,dvs-slew-rate-mv-per-us", &val)) { + switch (val) { + case 5: + return MAX77675_DVS_SLEW_5MV_PER_US; + case 10: + return MAX77675_DVS_SLEW_10MV_PER_US; + default: + break; + } + } + + /* default: 5 mV/us */ + return MAX77675_DVS_SLEW_5MV_PER_US; +} + +static u8 max77675_parse_drv_sbb_strength(struct device_node *np) +{ + const char *str; + + if (!of_property_read_string(np, "maxim,drv-sbb-strength", &str)) { + if (!strcasecmp(str, "max")) + return MAX77675_DRV_SBB_STRENGTH_MAX; + else if (!strcasecmp(str, "high")) + return MAX77675_DRV_SBB_STRENGTH_HIGH; + else if (!strcasecmp(str, "low")) + return MAX77675_DRV_SBB_STRENGTH_LOW; + else if (!strcasecmp(str, "min")) + return MAX77675_DRV_SBB_STRENGTH_MIN; + } + + /* default : maximum */ + return MAX77675_DRV_SBB_STRENGTH_MAX; +} + +static u8 max77675_parse_debounce_time_us(struct device_node *np) +{ + u32 val; + + if (!of_property_read_u32(np, "maxim,debounce-time-us", &val)) { + switch (val) { + case 100: + return MAX77675_DBEN_100US; + case 30000: + return MAX77675_DBEN_30000US; + default: + break; + } + } + + /* default: 100 us */ + return MAX77675_DBEN_100US; +} + +static int max77675_parse_config(struct max77675_regulator *maxreg) +{ + struct device_node *np = maxreg->dev->of_node; + struct max77675_config *config = &maxreg->config; + int ret; + + /* EN pin mode: push-button, slide-switch, or logic */ + config->en_mode = max77675_parse_en_mode(np); + + /* latency mode */ + config->voltage_change_latency = max77675_parse_voltage_change_latency(np); + + /* drive strength */ + config->drv_sbb_strength = max77675_parse_drv_sbb_strength(np); + + /* drv slew rate */ + config->dvs_slew_rate = max77675_parse_dvs_slew_rate(np); + + /* Debounce time for EN pin */ + config->debounce_time = max77675_parse_debounce_time_us(np); + + /* Manual reset time for EN pin */ + config->manual_reset_time = max77675_parse_manual_reset_time(np); + + /* Disable internal pull-up resistor on EN pin */ + config->en_pullup_disable = of_property_read_bool(np, "maxim,en-pullup-disable"); + + /* Request low-power mode for main bias */ + config->bias_low_power_request = of_property_read_bool(np, "maxim,bias-low-power-request"); + + /* Force internal LDO to always supply 1.8V */ + config->simo_int_ldo_always_on = of_property_read_bool(np, "maxim,simo-int-ldo-always-on"); + + ret = max77675_apply_config(maxreg); + + return ret; +} + +static int max77675_init_event(struct max77675_regulator *maxreg) +{ + unsigned int ercflag, int_glbl; + int ret; + + ret = regmap_read(maxreg->regmap, MAX77675_REG_ERCF_GLBL, &ercflag); + if (ret) { + dev_err(maxreg->dev, "Failed to read CID register: %d\n", ret); + return ret; + } + + ret = regmap_read(maxreg->regmap, MAX77675_REG_INT_GLBL, &int_glbl); + if (ret) { + dev_err(maxreg->dev, "Failed to read INT_GLBL register: %d\n", ret); + return ret; + } + + if (ercflag & MAX77675_SFT_CRST_F_BIT) + dev_info(maxreg->dev, "Software Cold Reset Flag is set\n"); + + if (ercflag & MAX77675_SFT_OFF_F_BIT) + dev_info(maxreg->dev, "Software Off Flag is set\n"); + + if (ercflag & MAX77675_MRST_BIT) + dev_info(maxreg->dev, "Manual Reset Timer Flag is set\n"); + + if (ercflag & MAX77675_UVLO_BIT) + dev_info(maxreg->dev, "Undervoltage Lockout Flag is set\n"); + + if (ercflag & MAX77675_OVLO_BIT) + dev_info(maxreg->dev, "Overvoltage Lockout Flag is set\n"); + + if (ercflag & MAX77675_TOVLD_BIT) + dev_info(maxreg->dev, "Thermal Overload Flag is set\n"); + + if (int_glbl & MAX77675_INT_SBB3_F_BIT) + dev_info(maxreg->dev, "SBB3 Channel Fault Interrupt occurred\n"); + + if (int_glbl & MAX77675_INT_SBB2_F_BIT) + dev_info(maxreg->dev, "SBB2 Channel Fault Interrupt occurred\n"); + + if (int_glbl & MAX77675_INT_SBB1_F_BIT) + dev_info(maxreg->dev, "SBB1 Channel Fault Interrupt occurred\n"); + + if (int_glbl & MAX77675_INT_SBB0_F_BIT) + dev_info(maxreg->dev, "SBB0 Channel Fault Interrupt occurred\n"); + + if (int_glbl & MAX77675_INT_TJAL2_R_BIT) + dev_info(maxreg->dev, "Thermal Alarm 2 Rising Interrupt occurred\n"); + + if (int_glbl & MAX77675_INT_TJAL1_R_BIT) + dev_info(maxreg->dev, "Thermal Alarm 1 Rising Interrupt occurred\n"); + + if (int_glbl & MAX77675_INT_EN_R_BIT) + dev_info(maxreg->dev, "nEN Rising Edge Interrupt occurred\n"); + + if (int_glbl & MAX77675_INT_EN_F_BIT) + dev_info(maxreg->dev, "nEN Falling Edge Interrupt occurred\n"); + + return 0; +} + +static int max77675_regulator_probe(struct i2c_client *client) +{ + struct max77675_regulator *maxreg; + struct regulator_config config = {}; + struct device_node *regulators_np; + int i, ret; + + maxreg = devm_kzalloc(&client->dev, sizeof(*maxreg), GFP_KERNEL); + if (!maxreg) + return -ENOMEM; + + maxreg->dev = &client->dev; + + maxreg->regmap = devm_regmap_init_i2c(client, &max77675_regmap_config); + if (IS_ERR(maxreg->regmap)) + return dev_err_probe(maxreg->dev, + PTR_ERR(maxreg->regmap), + "Failed to init regmap\n"); + + ret = max77675_init_event(maxreg); + if (ret) + return dev_err_probe(maxreg->dev, ret, "Failed to init event\n"); + + ret = max77675_parse_config(maxreg); + if (ret) + return dev_err_probe(maxreg->dev, ret, "Failed to apply config\n"); + + config.dev = &client->dev; + config.regmap = maxreg->regmap; + config.driver_data = maxreg; + + regulators_np = of_get_child_by_name(client->dev.of_node, "regulators"); + if (!regulators_np) { + dev_err(maxreg->dev, "No 'regulators' subnode found in DT\n"); + return -EINVAL; + } + + for (i = 0; i < MAX77675_ID_NUM_MAX; i++) { + const struct regulator_desc *desc = &max77675_regulators[i]; + struct regulator_dev *rdev; + + config.of_node = of_get_child_by_name(regulators_np, desc->name); + if (!config.of_node) { + dev_warn(maxreg->dev, "No DT node for regulator %s\n", desc->name); + continue; + } + + rdev = devm_regulator_register(&client->dev, desc, &config); + of_node_put(config.of_node); + if (IS_ERR(rdev)) { + of_node_put(regulators_np); + return dev_err_probe(maxreg->dev, PTR_ERR(rdev), + "Failed to register regulator %d (%s): %d\n", + i, desc->name, ret); + } + } + + of_node_put(regulators_np); + i2c_set_clientdata(client, maxreg); + + return 0; +} + +static const struct i2c_device_id max77675_i2c_id[] = { + { "max77675", 0 }, + { } +}; +MODULE_DEVICE_TABLE(i2c, max77675_i2c_id); + +static const struct of_device_id __maybe_unused max77675_of_match[] = { + { .compatible = "maxim,max77675", }, + { } +}; +MODULE_DEVICE_TABLE(of, max77675_of_match); + +static struct i2c_driver max77675_regulator_driver = { + .driver = { + .name = "max77675", + .of_match_table = of_match_ptr(max77675_of_match), + }, + .probe = max77675_regulator_probe, + .id_table = max77675_i2c_id, +}; + +module_i2c_driver(max77675_regulator_driver); + +MODULE_DESCRIPTION("MAX77675 Regulator Driver"); +MODULE_AUTHOR("Joan Na <joan.na@analog.com>"); +MODULE_LICENSE("GPL"); diff --git a/drivers/regulator/max77675-regulator.h b/drivers/regulator/max77675-regulator.h new file mode 100644 index 000000000000..0aaa30a630ca --- /dev/null +++ b/drivers/regulator/max77675-regulator.h @@ -0,0 +1,260 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/* + * MAX77675 Register Definitions + * Reference: MAX77675 Datasheet + */ + +#ifndef __MAX77675_REG_H__ +#define __MAX77675_REG_H__ + +#include <linux/bitops.h> + +/* Register Addresses */ +#define MAX77675_REG_CNFG_GLBL_A 0x00 +#define MAX77675_REG_CNFG_GLBL_B 0x01 +#define MAX77675_REG_INT_GLBL 0x02 +#define MAX77675_REG_INTM_GLBL 0x03 +#define MAX77675_REG_STAT_GLBL 0x04 +#define MAX77675_REG_ERCF_GLBL 0x05 +#define MAX77675_REG_CID 0x06 +#define MAX77675_REG_CNFG_SBB_TOP_A 0x07 +#define MAX77675_REG_CNFG_SBB0_A 0x08 +#define MAX77675_REG_CNFG_SBB0_B 0x09 +#define MAX77675_REG_CNFG_SBB1_A 0x0A +#define MAX77675_REG_CNFG_SBB1_B 0x0B +#define MAX77675_REG_CNFG_SBB2_A 0x0C +#define MAX77675_REG_CNFG_SBB2_B 0x0D +#define MAX77675_REG_CNFG_SBB3_A 0x0E +#define MAX77675_REG_CNFG_SBB3_B 0x0F +#define MAX77675_REG_CNFG_SBB_TOP_B 0x10 + +/* CNFG_GLBL_A (0x00) bit masks and shifts */ +#define MAX77675_MRT_MASK GENMASK(7, 6) /* Manual Reset Time (bits 7:6) */ +#define MAX77675_MRT_SHIFT 6 +#define MAX77675_PU_DIS_BIT BIT(5) /* Pullup Disable (bit 5) */ +#define MAX77675_PU_DIS_SHIFT 5 +#define MAX77675_BIAS_LPM_BIT BIT(4) /* Bias Low Power Mode (bit 4) */ +#define MAX77675_BIAS_LPM_SHIFT 4 +#define MAX77675_SIMO_CH_DIS_BIT BIT(3) /* SIMO Internal Channel Disable (bit 3) */ +#define MAX77675_SIMO_CH_DIS_SHIFT 3 +#define MAX77675_EN_MODE_MASK GENMASK(2, 1) /* nEN Mode (bits 2:1) */ +#define MAX77675_EN_MODE_SHIFT 1 +#define MAX77675_DBEN_EN_BIT BIT(0) /* Debounce Enable (bit 0) */ +#define MAX77675_DBEN_EN_SHIFT 0 + +/* CNFG_GLBL_B (0x01) */ +#define MAX77675_SFT_CTRL_MASK GENMASK(2, 0) /* Soft Start Control */ +#define MAX77675_SFT_CTRL_SHIFT 0 + +/* INT_GLBL (0x02) bit bits and shifts */ +#define MAX77675_INT_SBB3_F_BIT BIT(7) +#define MAX77675_INT_SBB3_F_SHIFT 7 +#define MAX77675_INT_SBB2_F_BIT BIT(6) +#define MAX77675_INT_SBB2_F_SHIFT 6 +#define MAX77675_INT_SBB1_F_BIT BIT(5) +#define MAX77675_INT_SBB1_F_SHIFT 5 +#define MAX77675_INT_SBB0_F_BIT BIT(4) +#define MAX77675_INT_SBB0_F_SHIFT 4 +#define MAX77675_INT_TJAL2_R_BIT BIT(3) +#define MAX77675_INT_TJAL2_R_SHIFT 3 +#define MAX77675_INT_TJAL1_R_BIT BIT(2) +#define MAX77675_INT_TJAL1_R_SHIFT 2 +#define MAX77675_INT_EN_R_BIT BIT(1) +#define MAX77675_INT_EN_R_SHIFT 1 +#define MAX77675_INT_EN_F_BIT BIT(0) +#define MAX77675_INT_EN_F_SHIFT 0 + +/* INTM_GLBL (0x03) bits and shifts */ +#define MAX77675_INTM_SBB3_F_BIT BIT(7) +#define MAX77675_INTM_SBB3_F_SHIFT 7 +#define MAX77675_INTM_SBB2_F_BIT BIT(6) +#define MAX77675_INTM_SBB2_F_SHIFT 6 +#define MAX77675_INTM_SBB1_F_BIT BIT(5) +#define MAX77675_INTM_SBB1_F_SHIFT 5 +#define MAX77675_INTM_SBB0_F_BIT BIT(4) +#define MAX77675_INTM_SBB0_F_SHIFT 4 +#define MAX77675_INTM_TJAL2_R_BIT BIT(3) +#define MAX77675_INTM_TJAL2_R_SHIFT 3 +#define MAX77675_INTM_TJAL1_R_BIT BIT(2) +#define MAX77675_INTM_TJAL1_R_SHIFT 2 +#define MAX77675_INTM_EN_R_BIT BIT(1) +#define MAX77675_INTM_EN_R_SHIFT 1 +#define MAX77675_INTM_EN_F_BIT BIT(0) +#define MAX77675_INTM_EN_F_SHIFT 0 + +/* STAT_GLBL (0x04) bits and shifts */ +#define MAX77675_STAT_SBB3_S_BIT BIT(7) +#define MAX77675_STAT_SBB3_S_SHIFT 7 +#define MAX77675_STAT_SBB2_S_BIT BIT(6) +#define MAX77675_STAT_SBB2_S_SHIFT 6 +#define MAX77675_STAT_SBB1_S_BIT BIT(5) +#define MAX77675_STAT_SBB1_S_SHIFT 5 +#define MAX77675_STAT_SBB0_S_BIT BIT(4) +#define MAX77675_STAT_SBB0_S_SHIFT 4 +#define MAX77675_STAT_TJAL2_S_BIT BIT(2) +#define MAX77675_STAT_TJAL2_S_SHIFT 2 +#define MAX77675_STAT_TJAL1_S_BIT BIT(1) +#define MAX77675_STAT_TJAL1_S_SHIFT 1 +#define MAX77675_STAT_STAT_EN_BIT BIT(0) +#define MAX77675_STAT_STAT_EN_SHIFT 0 + +#define MAX77675_STAT_STAT_EN_BIT BIT(0) +#define MAX77675_STAT_STAT_EN_SHIFT 0 + +/* ERCFLAG (0x05) bits and shifts */ +#define MAX77675_SFT_CRST_F_BIT BIT(5) /* Software Cold Reset Flag */ +#define MAX77675_SFT_CRST_F_SHIFT 5 +#define MAX77675_SFT_OFF_F_BIT BIT(4) /* Software Off Flag */ +#define MAX77675_SFT_OFF_F_SHIFT 4 +#define MAX77675_MRST_BIT BIT(3) /* Manual Reset Timer Flag */ +#define MAX77675_MRST_SHIFT 3 +#define MAX77675_UVLO_BIT BIT(2) /* Undervoltage Lockout Flag */ +#define MAX77675_UVLO_SHIFT 2 +#define MAX77675_OVLO_BIT BIT(1) /* Overvoltage Lockout Flag */ +#define MAX77675_OVLO_SHIFT 1 +#define MAX77675_TOVLD_BIT BIT(0) /* Thermal Overload Flag */ +#define MAX77675_TOVLD_SHIFT 0 + +/* CID (0x06) bits and shifts */ +#define MAX77675_CID_MASK GENMASK(4, 0) /* Chip Identification Code mask */ +#define MAX77675_CID_SHIFT 0 /* Starts at bit 0 */ + +/* CNFG_SBB_TOP_A (0x07) bits and shifts */ +#define MAX77675_STEP_SZ_SBB3_BIT BIT(5) +#define MAX77675_STEP_SZ_SBB3_SHIFT 5 +#define MAX77675_STEP_SZ_SBB2_BIT BIT(4) +#define MAX77675_STEP_SZ_SBB2_SHIFT 4 +#define MAX77675_STEP_SZ_SBB1_BIT BIT(3) +#define MAX77675_STEP_SZ_SBB1_SHIFT 3 +#define MAX77675_STEP_SZ_SBB0_BIT BIT(2) +#define MAX77675_STEP_SZ_SBB0_SHIFT 2 +#define MAX77675_DRV_SBB_MASK GENMASK(1, 0) +#define MAX77675_DRV_SBB_SHIFT 0 + +/* CNFG_SBB0_A (0x08) bits and shifts */ +#define MAX77675_TV_SBB0_MASK GENMASK(7, 0) +#define MAX77675_TV_SBB0_SHIFT 0 + +/* CNFG_SBB0_B (0x09) bits and shifts */ +#define MAX77675_ADE_SBB0_BIT BIT(3) +#define MAX77675_ADE_SBB0_SHIFT 3 +#define MAX77675_EN_SBB0_MASK GENMASK(2, 0) +#define MAX77675_EN_SBB0_SHIFT 0 + +/* CNFG_SBB1_A (0x0A) bits and shifts */ +#define MAX77675_TV_SBB1_MASK GENMASK(7, 0) +#define MAX77675_TV_SBB1_SHIFT 0 + +/* CNFG_SBB1_B (0x0B) bits and shifts */ +#define MAX77675_ADE_SBB1_BIT BIT(3) +#define MAX77675_ADE_SBB1_SHIFT 3 +#define MAX77675_EN_SBB1_MASK GENMASK(2, 0) +#define MAX77675_EN_SBB1_SHIFT 0 + +/* CNFG_SBB2_A (0x0C) bits and shifts */ +#define MAX77675_TV_SBB2_MASK GENMASK(7, 0) +#define MAX77675_TV_SBB2_SHIFT 0 + +/* CNFG_SBB2_B (0x0D) bits and shifts */ +#define MAX77675_ADE_SBB2_BIT BIT(3) +#define MAX77675_ADE_SBB2_SHIFT 3 +#define MAX77675_EN_SBB2_MASK GENMASK(2, 0) +#define MAX77675_EN_SBB2_SHIFT 0 + +/* CNFG_SBB3_A (0x0E) bits and shifts */ +#define MAX77675_TV_SBB3_MASK GENMASK(7, 0) +#define MAX77675_TV_SBB3_SHIFT 0 + +/* CNFG_SBB3_B (0x0F) bits and shifts */ +#define MAX77675_ADE_SBB3_BIT BIT(3) +#define MAX77675_ADE_SBB3_SHIFT 3 +#define MAX77675_EN_SBB3_MASK GENMASK(2, 0) +#define MAX77675_EN_SBB3_SHIFT 0 + +#define MAX77675_EN_SBB_MASK GENMASK(2, 0) + +/* CNFG_SBB_TOP_B (0x10) bits and shifts */ +#define MAX77675_DVS_SLEW_BIT BIT(5) +#define MAX77675_DVS_SLEW_SHIFT 5 +#define MAX77675_LAT_MODE_BIT BIT(4) +#define MAX77675_LAT_MODE_SHIFT 4 +#define MAX77675_SR_SBB3_BIT BIT(3) +#define MAX77675_SR_SBB3_SHIFT 3 +#define MAX77675_SR_SBB2_BIT BIT(2) +#define MAX77675_SR_SBB2_SHIFT 2 +#define MAX77675_SR_SBB1_BIT BIT(1) +#define MAX77675_SR_SBB1_SHIFT 1 +#define MAX77675_SR_SBB0_BIT BIT(0) +#define MAX77675_SR_SBB0_SHIFT 0 + +#define MAX77675_MAX_REGISTER 0x10 + +/* Common minimum voltage (in microvolts) */ +#define MAX77675_MIN_UV 500000 // 500 mV + +/* Voltage step configuration for 25mV mode */ +#define MAX77675_STEP_25MV 25000 // Step size: 25 mV +#define MAX77675_MAX_UV_25MV 5500000 // Max voltage: 5.5 V +#define MAX77675_NUM_LEVELS_25MV 201 // levels = (5500mV - 500mV) / 25mV + 1 + +/* Voltage step configuration for 12.5mV mode */ +#define MAX77675_STEP_12_5MV 12500 // Step size: 12.5 mV +#define MAX77675_MAX_UV_12_5MV 3687500 // Max voltage: 3.6875 V +#define MAX77675_NUM_LEVELS_12_5MV 255 // levels = (3687.5mV - 500mV) / 12.5mV + 1 + +#define MAX77675_ENABLE_OFF 0x04 +#define MAX77675_ENABLE_ON 0x06 + +#define MAX77675_REGULATOR_AD_OFF 0x00 +#define MAX77675_REGULATOR_AD_ON BIT(3) + +/* FPS source */ +#define MAX77675_FPS_SLOT_0 0x0 +#define MAX77675_FPS_SLOT_1 0x1 +#define MAX77675_FPS_SLOT_2 0x2 +#define MAX77675_FPS_SLOT_3 0x3 +#define MAX77675_FPS_DEF 0x4 + +/* nEN Manual Reset Time Configuration (MRT) */ +#define MAX77675_MRT_4S 0x0 +#define MAX77675_MRT_8S 0x1 +#define MAX77675_MRT_12S 0x2 +#define MAX77675_MRT_16S 0x3 + +/* nEN Mode Configuration */ +#define MAX77675_EN_PUSH_BUTTON 0x0 +#define MAX77675_EN_SLIDE_SWITCH 0x1 +#define MAX77675_EN_LOGIC 0x2 + +/* Debounce Timer Enable (DBEN_nEN) */ +#define MAX77675_DBEN_100US 0x0 +#define MAX77675_DBEN_30000US 0x1 + +/* Rising slew rate control for SBB0 when ramping up */ +#define MAX77675_SR_2MV_PER_US 0x0 // 2 mV/us +#define MAX77675_SR_USE_DVS 0x1 // Use DVS slew rate setting (maxim,dvs-slew-rate) + +/* Dynamic Voltage Scaling (DVS) Slew Rate */ +#define MAX77675_DVS_SLEW_5MV_PER_US 0x0 // 5 mV/us +#define MAX77675_DVS_SLEW_10MV_PER_US 0x1 // 10 mV/us + +/* Latency Mode */ +#define MAX77675_HIGH_LATENCY_MODE 0x0 // High latency, low quiescent current (~100us) +#define MAX77675_LOW_LATENCY_MODE 0x1 // Low latency, high quiescent current (~10us) + +/* SIMO Buck-Boost Drive Strength (All Channels) */ +#define MAX77675_DRV_SBB_STRENGTH_MAX 0x0 // Maximum drive strength (~0.6 ns transition time) +#define MAX77675_DRV_SBB_STRENGTH_HIGH 0x1 // High drive strength (~1.2 ns transition time) +#define MAX77675_DRV_SBB_STRENGTH_LOW 0x2 // Low drive strength (~1.8 ns transition time) +#define MAX77675_DRV_SBB_STRENGTH_MIN 0x3 // Minimum drive strength (~8 ns transition time) + +/* Regulator ID enumeration */ +enum max77675_regulator_id { + MAX77675_ID_SBB0 = 0, + MAX77675_ID_SBB1, + MAX77675_ID_SBB2, + MAX77675_ID_SBB3, + MAX77675_ID_NUM_MAX, +}; + +#endif /* __MAX77675_REG_H__ */ -- 2.34.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v5 2/2] regulator: max77675: Add MAX77675 regulator driver 2025-10-29 2:32 ` [PATCH v5 2/2] regulator: max77675: Add MAX77675 regulator driver Joan-Na-adi @ 2025-10-29 9:55 ` Nuno Sá 2025-11-03 2:45 ` Joan Na 0 siblings, 1 reply; 8+ messages in thread From: Nuno Sá @ 2025-10-29 9:55 UTC (permalink / raw) To: Joan-Na-adi, Liam Girdwood Cc: Mark Brown, Rob Herring, Krzysztof Kozlowski, Conor Dooley, linux-kernel, devicetree, Joan Na On Wed, 2025-10-29 at 11:32 +0900, Joan-Na-adi wrote: > From: Joan Na <joan.na@analog.com> > > Add support for the Maxim Integrated MAX77675 PMIC regulator. > > The MAX77675 is a compact, highly efficient SIMO (Single Inductor Multiple Output) > power management IC that provides four programmable buck-boost switching regulators > with only one inductor. It supports up to 700mA total output current and operates > from a single-cell Li-ion battery. > > An integrated power-up sequencer and I2C interface allow flexible startup > configuration and runtime control. > > Signed-off-by: Joan Na <joan.na@analog.com> > --- Hi Joan, Some comments from me... > drivers/regulator/Kconfig | 9 + > drivers/regulator/Makefile | 1 + > drivers/regulator/max77675-regulator.c | 861 +++++++++++++++++++++++++ > drivers/regulator/max77675-regulator.h | 260 ++++++++ > 4 files changed, 1131 insertions(+) > create mode 100644 drivers/regulator/max77675-regulator.c > create mode 100644 drivers/regulator/max77675-regulator.h > > diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig > index d84f3d054c59..93131446e402 100644 > --- a/drivers/regulator/Kconfig > +++ b/drivers/regulator/Kconfig > @@ -649,6 +649,15 @@ config REGULATOR_MAX77650 > Semiconductor. This device has a SIMO with three independent > power rails and an LDO. > > +config REGULATOR_MAX77675 > + tristate "Maxim MAX77675 regulator driver" > + depends on I2C Looking at your code, I would expected OF to be a dependency as well. > + select REGMAP_I2C > + help > + This driver controls the Maxim MAX77675 power regulator via I2C. > + It supports four programmable buck-boost outputs. > + Say Y here to enable the regulator driver > + > config REGULATOR_MAX77857 > tristate "ADI MAX77857/MAX77831 regulator support" > depends on I2C > diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile > index b3101376029d..cdd99669cd24 100644 > --- a/drivers/regulator/Makefile > +++ b/drivers/regulator/Makefile > @@ -78,6 +78,7 @@ obj-$(CONFIG_REGULATOR_MAX77503) += max77503-regulator.o > obj-$(CONFIG_REGULATOR_MAX77541) += max77541-regulator.o > obj-$(CONFIG_REGULATOR_MAX77620) += max77620-regulator.o > obj-$(CONFIG_REGULATOR_MAX77650) += max77650-regulator.o > +obj-$(CONFIG_REGULATOR_MAX77675) += max77675-regulator.o > obj-$(CONFIG_REGULATOR_MAX8649) += max8649.o > obj-$(CONFIG_REGULATOR_MAX8660) += max8660.o > obj-$(CONFIG_REGULATOR_MAX8893) += max8893.o > diff --git a/drivers/regulator/max77675-regulator.c b/drivers/regulator/max77675-regulator.c > new file mode 100644 > index 000000000000..c1281f07fe43 > --- /dev/null > +++ b/drivers/regulator/max77675-regulator.c > @@ -0,0 +1,861 @@ > +// SPDX-License-Identifier: GPL-2.0-or-later > +/* > + * Copyright (c) 2025 Analog Devices, Inc. > + * ADI regulator driver for MAX77675. > + */ > + > +#include <linux/module.h> > +#include <linux/slab.h> > +#include <linux/of.h> > +#include <linux/i2c.h> > +#include <linux/regmap.h> > +#include <linux/platform_device.h> > +#include <linux/regulator/driver.h> > +#include <linux/regulator/consumer.h> > +#include <linux/regulator/of_regulator.h> > +#include <linux/bitfield.h> You're clearly missing (at least) mod_devicetable.h. I know that at least clang allows you to get iwyu, > + > +#include "max77675-regulator.h" Why do we need the header file? Just include everything in the source code unless you expect to share something with another module (which I dunno)? > + > +struct max77675_regulator_pdata { > + u8 fps_slot; > + bool fixed_slew_rate; > +}; > + I would get rid of the _pdata suffix. Implies some legacy way of doing things (but kind of a nitpick) > +struct max77675_config { > + u8 en_mode; > + u8 voltage_change_latency; > + u8 drv_sbb_strength; > + u8 dvs_slew_rate; > + u8 debounce_time; > + u8 manual_reset_time; > + bool en_pullup_disable; > + bool bias_low_power_request; > + bool simo_int_ldo_always_on; > +}; > + > +struct max77675_regulator { > + struct device *dev; > + struct regmap *regmap; > + struct max77675_config config; > + struct max77675_regulator_pdata pdata[MAX77675_ID_NUM_MAX]; > +}; > + > +/** > + * Set latency mode. > + * > + * @param maxreg Pointer to max77675 device structure. > + * @param enable true to enable latency mode, false to disable. > + */ > +static int max77675_set_latency_mode(struct max77675_regulator *maxreg, bool enable) > +{ > + return regmap_update_bits(maxreg->regmap, MAX77675_REG_CNFG_SBB_TOP_B, > + MAX77675_LAT_MODE_BIT, > + FIELD_PREP(MAX77675_LAT_MODE_BIT, enable)); > +} > + I would drop these one liner wrappers. Personally, I don't see a big benefit on it. > +/** > + * Set DVS slew rate mode. > + * > + * @param maxreg Pointer to max77675 device structure. > + * @param enable true to use DVS-controlled slew rate, false for fixed 2mV/us. > + */ > +static int max77675_set_dvs_slew_rate(struct max77675_regulator *maxreg, bool enable) > +{ > + return regmap_update_bits(maxreg->regmap, MAX77675_REG_CNFG_SBB_TOP_B, > + MAX77675_DVS_SLEW_BIT, > + FIELD_PREP(MAX77675_DVS_SLEW_BIT, enable)); > +} > + Ditto for all other places. ... > > + > +/** > + * Set debounce time for EN pin. > + * > + * @param maxreg Pointer to max77675 device structure. > + * @param debounce_30ms true for 30ms, false for 100us > + */ > +static int max77675_set_debounce_time(struct max77675_regulator *maxreg, bool debounce_30ms) > +{ > + return regmap_update_bits(maxreg->regmap, MAX77675_REG_CNFG_GLBL_A, > + MAX77675_DBEN_EN_BIT, > + FIELD_PREP(MAX77675_DBEN_EN_BIT, debounce_30ms)); > +} > + > +static int max77675_regulator_get_fps_src(struct max77675_regulator *maxreg, int id) > +{ > + unsigned int reg_addr; > + unsigned int val; > + int ret; > + > + switch (id) { > + case MAX77675_ID_SBB0: > + reg_addr = MAX77675_REG_CNFG_SBB0_B; > + break; > + case MAX77675_ID_SBB1: > + reg_addr = MAX77675_REG_CNFG_SBB1_B; > + break; > + case MAX77675_ID_SBB2: > + reg_addr = MAX77675_REG_CNFG_SBB2_B; > + break; > + case MAX77675_ID_SBB3: > + reg_addr = MAX77675_REG_CNFG_SBB3_B; > + break; > + default: > + dev_err(maxreg->dev, "Invalid regulator id: %d\n", id); > + return -EINVAL; > + } > + > + ret = regmap_read(maxreg->regmap, reg_addr, &val); > + if (ret < 0) { > + dev_err(maxreg->dev, "Failed to read FPS source (reg 0x%02x): %d\n", > + reg_addr, ret); > + return ret; > + } > + > + return val & MAX77675_EN_SBB_MASK; Ok, this works since the mask is 0x7. However, FIELD_GET() would make it more readable and easy to review. I mean, I would not need to go and see the mask value. > +} > + > +static int max77675_regulator_set_fps_src(struct max77675_regulator *maxreg, int id, u8 fps_src) > +{ > + unsigned int reg_addr; > + int ret; > + > + switch (id) { > + case MAX77675_ID_SBB0: > + reg_addr = MAX77675_REG_CNFG_SBB0_B; > + break; > + case MAX77675_ID_SBB1: > + reg_addr = MAX77675_REG_CNFG_SBB1_B; > + break; > + case MAX77675_ID_SBB2: > + reg_addr = MAX77675_REG_CNFG_SBB2_B; > + break; > + case MAX77675_ID_SBB3: > + reg_addr = MAX77675_REG_CNFG_SBB3_B; > + break; > + default: > + dev_err(maxreg->dev, "Invalid regulator id: %d\n", id); > + return -EINVAL; > + } > + > + ret = regmap_update_bits(maxreg->regmap, reg_addr, > + MAX77675_EN_SBB_MASK, fps_src); > + if (ret < 0) { > + dev_err(maxreg->dev, "Failed to set FPS source (reg 0x%02x): %d\n", > + reg_addr, ret); > + return ret; > + } I would drop the log and just do return regmap_update_bits(). Up to you... > + > + return 0; > +} > + > +/** > + * max77675_set_sbb_slew_rate_fixed - Set the slew rate for a specific SBB regulator channel > + * > + * @maxreg: Pointer to the max77675 regulator structure > + * @id: Regulator channel ID (ID_SBB0 ~ ID_SBB3) > + * @fixed: Slew rate value (true = 2mV/us, false = use DVS_SLEW) > + * > + * This function configures the slew rate control source for the specified SBB channel by > + * updating the corresponding bits in the CNFG_SBB_TOP_B register. > + * > + * Return: 0 on success, negative error code on failure (e.g., invalid channel ID). > + */ > +static int max77675_set_sbb_slew_rate_fixed(struct max77675_regulator *maxreg, int id, bool > fixed) > +{ > + u8 mask, value; > + u8 slew_src_ctrl_bit = fixed ? 0 : 1; > + > + switch (id) { > + case MAX77675_ID_SBB0: > + mask = MAX77675_SR_SBB0_BIT; > + value = FIELD_PREP(MAX77675_SR_SBB0_BIT, slew_src_ctrl_bit); > + break; > + > + case MAX77675_ID_SBB1: > + mask = MAX77675_SR_SBB1_BIT; > + value = FIELD_PREP(MAX77675_SR_SBB1_BIT, slew_src_ctrl_bit); > + break; > + > + case MAX77675_ID_SBB2: > + mask = MAX77675_SR_SBB2_BIT; > + value = FIELD_PREP(MAX77675_SR_SBB2_BIT, slew_src_ctrl_bit); > + break; > + > + case MAX77675_ID_SBB3: > + mask = MAX77675_SR_SBB3_BIT; > + value = FIELD_PREP(MAX77675_SR_SBB3_BIT, slew_src_ctrl_bit); > + break; > + > + default: > + return -EINVAL; > + } > + > + return regmap_update_bits(maxreg->regmap, MAX77675_REG_CNFG_SBB_TOP_B, mask, value); > +} > + > +static int max77675_init_regulator(struct max77675_regulator *maxreg, int id) > +{ > + struct max77675_regulator_pdata *rpdata = &maxreg->pdata[id]; > + int ret; > + > + if (rpdata->fps_slot == MAX77675_FPS_DEF) { > + ret = max77675_regulator_get_fps_src(maxreg, id); > + if (ret < 0) { > + dev_err(maxreg->dev, "Failed to read FPS source for ID %d\n", id); > + return ret; > + } > + rpdata->fps_slot = ret; > + } else { > + ret = max77675_regulator_set_fps_src(maxreg, id, rpdata->fps_slot); > + if (ret) > + dev_warn(maxreg->dev, "Failed to set FPS source for ID %d\n", id); > + } > + > + ret = max77675_set_sbb_slew_rate_fixed(maxreg, id, rpdata->fixed_slew_rate); > + if (ret) > + dev_warn(maxreg->dev, "Failed to set slew rate for ID %d\n", id); Do we really want to treat this as a warning (as FPS)? If so, I would expect a proper comment explaining why we can afford it. > + > + return 0; > +} > + > +static int max77675_of_parse_cb(struct device_node *np, > + const struct regulator_desc *desc, > + struct regulator_config *config) > +{ > + struct max77675_regulator *maxreg = config->driver_data; > + struct max77675_regulator_pdata *rpdata = &maxreg->pdata[desc->id]; > + u32 pval; > + int ret; > + > + /* Parse FPS slot from DT */ > + ret = of_property_read_u32(np, "maxim,fps-slot", &pval); > + rpdata->fps_slot = (!ret) ? (u8)pval : MAX77675_FPS_DEF; > + So, can we get any arbitrary value for pval? I see we you have an enum in the bindings so make sure we properly validate it. Same for all other properties. The bindings also have this as a string and here you have a u32? Not going to work. You need of_property_read_string() + match_string(). Also, "maxim,"? For some time now it's "adi,". > + /* Parse slew rate control source */ > + rpdata->fixed_slew_rate = of_property_read_bool(np, "maxim,fixed-slew-rate"); > + > + /* Apply parsed configuration */ > + return max77675_init_regulator(maxreg, desc->id); > +} > + > +static int max77675_get_error_flags(struct regulator_dev *rdev, unsigned int *flags) > +{ > + struct max77675_regulator *maxreg = rdev_get_drvdata(rdev); > + unsigned int int_flags; > + int id = rdev_get_id(rdev); > + int ret; > + > + ret = regmap_read(maxreg->regmap, MAX77675_REG_INT_GLBL, &int_flags); > + if (ret) { > + dev_err(maxreg->dev, "Failed to read INT_GLBL: %d\n", ret); > + return ret; > + } > + > + *flags = 0; > + > + switch (id) { > + case MAX77675_ID_SBB0: > + if (int_flags & MAX77675_INT_SBB0_F_BIT) > + *flags |= REGULATOR_ERROR_FAIL; > + break; > + case MAX77675_ID_SBB1: > + if (int_flags & MAX77675_INT_SBB1_F_BIT) > + *flags |= REGULATOR_ERROR_FAIL; > + break; > + case MAX77675_ID_SBB2: > + if (int_flags & MAX77675_INT_SBB2_F_BIT) > + *flags |= REGULATOR_ERROR_FAIL; > + break; > + case MAX77675_ID_SBB3: > + if (int_flags & MAX77675_INT_SBB3_F_BIT) > + *flags |= REGULATOR_ERROR_FAIL; > + break; > + default: > + dev_warn(maxreg->dev, "Unsupported regulator ID: %d\n", id); > + break; Should not be a warning. Also wonder if it can happen at all? > + } > + > + if (int_flags & MAX77675_INT_TJAL2_R_BIT) { > + /* TJAL2 interrupt: Over-temperature condition (above 120 degree) */ > + *flags |= REGULATOR_ERROR_OVER_TEMP; > + } > + > + return 0; > +} > + > +static const struct regulator_ops max77675_regulator_ops = { > + .list_voltage = regulator_list_voltage_linear, > + .enable = regulator_enable_regmap, > + .disable = regulator_disable_regmap, > + .is_enabled = regulator_is_enabled_regmap, > + .map_voltage = regulator_map_voltage_linear, > + .set_voltage_sel = regulator_set_voltage_sel_regmap, > + .get_voltage_sel = regulator_get_voltage_sel_regmap, > + .set_active_discharge = regulator_set_active_discharge_regmap, > + .get_error_flags = max77675_get_error_flags, > +}; > + > +static struct regulator_desc max77675_regulators[MAX77675_ID_NUM_MAX] = { > + { > + .name = "sbb0", > + .of_match = of_match_ptr("sbb0"), > + .regulators_node = of_match_ptr("regulators"), I wonder if we need of_match_ptr() given that the whole thing depends on OF. ... > + > +static int max77675_apply_config(struct max77675_regulator *maxreg) > +{ > + const struct max77675_config *config = &maxreg->config; > + int ret; > + > + ret = max77675_set_en_mode(maxreg, config->en_mode); > + if (ret) { > + dev_err(maxreg->dev, "Failed to set EN mode: %d\n", ret); > + return ret; > + } > + > + ret = max77675_set_latency_mode(maxreg, config->voltage_change_latency); > + if (ret) { > + dev_err(maxreg->dev, "Failed to set latency mode: %d\n", ret); > + return ret; > + } > + > + ret = max77675_set_drv_sbb_strength(maxreg, config->drv_sbb_strength); > + if (ret) { > + dev_err(maxreg->dev, "Failed to set drive strength: %d\n", ret); > + return ret; > + } > + > + ret = max77675_set_dvs_slew_rate(maxreg, config->dvs_slew_rate); > + if (ret) { > + dev_err(maxreg->dev, "Failed to set DVS slew rate: %d\n", ret); > + return ret; > + } > + > + ret = max77675_set_debounce_time(maxreg, config->debounce_time); > + if (ret) { > + dev_err(maxreg->dev, "Failed to set EN debounce time: %d\n", ret); > + return ret; > + } > + > + ret = max77675_set_manual_reset_time(maxreg, config->manual_reset_time); > + if (ret) { > + dev_err(maxreg->dev, "Failed to set manual reset time: %d\n", ret); > + return ret; > + } > + > + ret = max77675_set_en_pullup_disable(maxreg, config->en_pullup_disable); > + if (ret) { > + dev_err(maxreg->dev, "Failed to set EN pull-up disable: %d\n", ret); > + return ret; > + } > + > + ret = max77675_set_bias_low_power_request(maxreg, config->bias_low_power_request); > + if (ret) { > + dev_err(maxreg->dev, "Failed to set bias low-power request: %d\n", ret); > + return ret; > + } > + > + ret = max77675_set_simo_int_ldo_always_on(maxreg, config->simo_int_ldo_always_on); > + if (ret) { > + dev_err(maxreg->dev, "Failed to set SIMO internal LDO always-on: %d\n", ret); > + return ret; > + } This seems to called during probe. All the above can be return dev_err_probe()... > + > + return 0; > +} > + > +static u8 max77675_parse_voltage_change_latency(struct device_node *np) > +{ > + u32 val; > + > + if (!of_property_read_u32(np, "maxim,voltage-change-latency-us", &val)) { > + switch (val) { > + case 10: > + return MAX77675_LOW_LATENCY_MODE; > + case 100: > + return MAX77675_HIGH_LATENCY_MODE; > + default: > + break; The above is wrong. You're ignoring invalid values being given and overwrite them with defaults. The pattern is: val = MAX77675_HIGH_LATENCY_MODE; if (!of_property_read_u32(np, "maxim,voltage-change-latency-us", &val)) { ... default: return dev_err_probe(dev, -EINVAL, ...); } You can also do: ret = of_property_read_u32(...); /* Not 100% sure if -EINVAL is the error code for a missing property */ if (ret && ret != -EINVAL) return ret; if (!ret) { ... } > + } > + } > + > + /* default: high latency */ > + return MAX77675_HIGH_LATENCY_MODE; > +} > + > +static u8 max77675_parse_en_mode(struct device_node *np) > +{ > + const char *str; > + > + if (!of_property_read_string(np, "maxim,en-mode", &str)) { > + if (!strcasecmp(str, "push-button")) > + return MAX77675_EN_PUSH_BUTTON; > + else if (!strcasecmp(str, "slide-switch")) > + return MAX77675_EN_SLIDE_SWITCH; > + else if (!strcasecmp(str, "logic")) > + return MAX77675_EN_LOGIC; redundant else if(). Moreover, this looks like it could use match_string() > + } > + > + /* default : slide-switch */ > + return MAX77675_EN_SLIDE_SWITCH; > +} > + > +static u8 max77675_parse_manual_reset_time(struct device_node *np) > +{ > + u32 val; > + > + if (!of_property_read_u32(np, "reset-time-sec", &val)) { > + switch (val) { > + case 4: > + return MAX77675_MRT_4S; > + case 8: > + return MAX77675_MRT_8S; > + case 12: > + return MAX77675_MRT_12S; > + case 16: > + return MAX77675_MRT_16S; > + default: > + break; Ditto. ... > + > +static int max77675_parse_config(struct max77675_regulator *maxreg) > +{ > + struct device_node *np = maxreg->dev->of_node; > + struct max77675_config *config = &maxreg->config; > + int ret; > + > + /* EN pin mode: push-button, slide-switch, or logic */ > + config->en_mode = max77675_parse_en_mode(np); > + > + /* latency mode */ > + config->voltage_change_latency = max77675_parse_voltage_change_latency(np); > + > + /* drive strength */ > + config->drv_sbb_strength = max77675_parse_drv_sbb_strength(np); > + > + /* drv slew rate */ > + config->dvs_slew_rate = max77675_parse_dvs_slew_rate(np); > + > + /* Debounce time for EN pin */ > + config->debounce_time = max77675_parse_debounce_time_us(np); > + > + /* Manual reset time for EN pin */ > + config->manual_reset_time = max77675_parse_manual_reset_time(np); Seems to me that all of the above will need some error handling. > + > + /* Disable internal pull-up resistor on EN pin */ > + config->en_pullup_disable = of_property_read_bool(np, "maxim,en-pullup-disable"); > + > + /* Request low-power mode for main bias */ > + config->bias_low_power_request = of_property_read_bool(np, "maxim,bias-low-power- > request"); > + > + /* Force internal LDO to always supply 1.8V */ > + config->simo_int_ldo_always_on = of_property_read_bool(np, "maxim,simo-int-ldo-always- > on"); > + > + ret = max77675_apply_config(maxreg); > + > + return ret; > +} > + > +static int max77675_init_event(struct max77675_regulator *maxreg) > +{ > + unsigned int ercflag, int_glbl; > + int ret; > + > + ret = regmap_read(maxreg->regmap, MAX77675_REG_ERCF_GLBL, &ercflag); > + if (ret) { > + dev_err(maxreg->dev, "Failed to read CID register: %d\n", ret); > + return ret; > + } > + > + ret = regmap_read(maxreg->regmap, MAX77675_REG_INT_GLBL, &int_glbl); > + if (ret) { > + dev_err(maxreg->dev, "Failed to read INT_GLBL register: %d\n", ret); > + return ret; > + } > + > + if (ercflag & MAX77675_SFT_CRST_F_BIT) > + dev_info(maxreg->dev, "Software Cold Reset Flag is set\n"); > + > + if (ercflag & MAX77675_SFT_OFF_F_BIT) > + dev_info(maxreg->dev, "Software Off Flag is set\n"); > + > + if (ercflag & MAX77675_MRST_BIT) > + dev_info(maxreg->dev, "Manual Reset Timer Flag is set\n"); > + > + if (ercflag & MAX77675_UVLO_BIT) > + dev_info(maxreg->dev, "Undervoltage Lockout Flag is set\n"); > + > + if (ercflag & MAX77675_OVLO_BIT) > + dev_info(maxreg->dev, "Overvoltage Lockout Flag is set\n"); > + > + if (ercflag & MAX77675_TOVLD_BIT) > + dev_info(maxreg->dev, "Thermal Overload Flag is set\n"); > + > + if (int_glbl & MAX77675_INT_SBB3_F_BIT) > + dev_info(maxreg->dev, "SBB3 Channel Fault Interrupt occurred\n"); > + > + if (int_glbl & MAX77675_INT_SBB2_F_BIT) > + dev_info(maxreg->dev, "SBB2 Channel Fault Interrupt occurred\n"); > + > + if (int_glbl & MAX77675_INT_SBB1_F_BIT) > + dev_info(maxreg->dev, "SBB1 Channel Fault Interrupt occurred\n"); > + > + if (int_glbl & MAX77675_INT_SBB0_F_BIT) > + dev_info(maxreg->dev, "SBB0 Channel Fault Interrupt occurred\n"); > + > + if (int_glbl & MAX77675_INT_TJAL2_R_BIT) > + dev_info(maxreg->dev, "Thermal Alarm 2 Rising Interrupt occurred\n"); > + > + if (int_glbl & MAX77675_INT_TJAL1_R_BIT) > + dev_info(maxreg->dev, "Thermal Alarm 1 Rising Interrupt occurred\n"); > + > + if (int_glbl & MAX77675_INT_EN_R_BIT) > + dev_info(maxreg->dev, "nEN Rising Edge Interrupt occurred\n"); > + > + if (int_glbl & MAX77675_INT_EN_F_BIT) > + dev_info(maxreg->dev, "nEN Falling Edge Interrupt occurred\n"); All of the above looks like dev_dbg() to me. > + > + return 0; > +} > + > +static int max77675_regulator_probe(struct i2c_client *client) > +{ > + struct max77675_regulator *maxreg; > + struct regulator_config config = {}; > + struct device_node *regulators_np; > + int i, ret; > + > + maxreg = devm_kzalloc(&client->dev, sizeof(*maxreg), GFP_KERNEL); > + if (!maxreg) > + return -ENOMEM; > + > + maxreg->dev = &client->dev; > + > + maxreg->regmap = devm_regmap_init_i2c(client, &max77675_regmap_config); > + if (IS_ERR(maxreg->regmap)) > + return dev_err_probe(maxreg->dev, > + PTR_ERR(maxreg->regmap), > + "Failed to init regmap\n"); > + > + ret = max77675_init_event(maxreg); > + if (ret) > + return dev_err_probe(maxreg->dev, ret, "Failed to init event\n"); > + > + ret = max77675_parse_config(maxreg); > + if (ret) > + return dev_err_probe(maxreg->dev, ret, "Failed to apply config\n"); > + > + config.dev = &client->dev; > + config.regmap = maxreg->regmap; > + config.driver_data = maxreg; > + > + regulators_np = of_get_child_by_name(client->dev.of_node, "regulators"); The above can actually be: struct device_node *regulators_np __free(device_node) = of_get_child_by_name(...) and then no need to care about of_node_put(). You need cleanup.h > + if (!regulators_np) { > + dev_err(maxreg->dev, "No 'regulators' subnode found in DT\n"); > + return -EINVAL; > + } > + > + for (i = 0; i < MAX77675_ID_NUM_MAX; i++) { > + const struct regulator_desc *desc = &max77675_regulators[i]; > + struct regulator_dev *rdev; > + > + config.of_node = of_get_child_by_name(regulators_np, desc->name); > + if (!config.of_node) { > + dev_warn(maxreg->dev, "No DT node for regulator %s\n", desc->name); > + continue; > + } > + > + rdev = devm_regulator_register(&client->dev, desc, &config); > + of_node_put(config.of_node); > + if (IS_ERR(rdev)) { > + of_node_put(regulators_np); > + return dev_err_probe(maxreg->dev, PTR_ERR(rdev), > + "Failed to register regulator %d (%s): %d\n", > + i, desc->name, ret); > + } > + } > + > + of_node_put(regulators_np); > + i2c_set_clientdata(client, maxreg); I do not see i2c_get_clientdata() anywhere. I suspect you can drop the above. > + > + return 0; > +} > + > +static const struct i2c_device_id max77675_i2c_id[] = { > + { "max77675", 0 }, > + { } > +}; > +MODULE_DEVICE_TABLE(i2c, max77675_i2c_id); > + > +static const struct of_device_id __maybe_unused max77675_of_match[] = { > + { .compatible = "maxim,max77675", }, > + { } > +}; > +MODULE_DEVICE_TABLE(of, max77675_of_match); > + > +static struct i2c_driver max77675_regulator_driver = { > + .driver = { > + .name = "max77675", > + .of_match_table = of_match_ptr(max77675_of_match), I guess we can drop of_match_ptr() if we agree that we depend on OF > + }, > + .probe = max77675_regulator_probe, > + .id_table = max77675_i2c_id, > +}; > + > +module_i2c_driver(max77675_regulator_driver); > + > +MODULE_DESCRIPTION("MAX77675 Regulator Driver"); > +MODULE_AUTHOR("Joan Na <joan.na@analog.com>"); > +MODULE_LICENSE("GPL"); > diff --git a/drivers/regulator/max77675-regulator.h b/drivers/regulator/max77675-regulator.h > new file mode 100644 > index 000000000000..0aaa30a630ca > --- /dev/null > +++ b/drivers/regulator/max77675-regulator.h As said, drop the header. - Nuno Sá ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v5 2/2] regulator: max77675: Add MAX77675 regulator driver 2025-10-29 9:55 ` Nuno Sá @ 2025-11-03 2:45 ` Joan Na 2025-11-03 11:04 ` Nuno Sá 0 siblings, 1 reply; 8+ messages in thread From: Joan Na @ 2025-11-03 2:45 UTC (permalink / raw) To: Nuno Sá Cc: Liam Girdwood, Mark Brown, Rob Herring, Krzysztof Kozlowski, Conor Dooley, linux-kernel, devicetree, Joan Na On Wed, Oct 29, 2025 at 09:55:53AM +0000, Nuno Sá wrote: > On Wed, 2025-10-29 at 11:32 +0900, Joan-Na-adi wrote: > > From: Joan Na <joan.na@analog.com> > > > > Add support for the Maxim Integrated MAX77675 PMIC regulator. > > > > The MAX77675 is a compact, highly efficient SIMO (Single Inductor Multiple Output) > > power management IC that provides four programmable buck-boost switching regulators > > with only one inductor. It supports up to 700mA total output current and operates > > from a single-cell Li-ion battery. > > > > An integrated power-up sequencer and I2C interface allow flexible startup > > configuration and runtime control. > > > > Signed-off-by: Joan Na <joan.na@analog.com> > > --- > > Hi Joan, > > Some comments from me... > Hello Nuno, Thank you for taking the time to review. Please refer to my response below. > > drivers/regulator/Kconfig | 9 + > > drivers/regulator/Makefile | 1 + > > drivers/regulator/max77675-regulator.c | 861 +++++++++++++++++++++++++ > > drivers/regulator/max77675-regulator.h | 260 ++++++++ > > 4 files changed, 1131 insertions(+) > > create mode 100644 drivers/regulator/max77675-regulator.c > > create mode 100644 drivers/regulator/max77675-regulator.h > > > > diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig > > index d84f3d054c59..93131446e402 100644 > > --- a/drivers/regulator/Kconfig > > +++ b/drivers/regulator/Kconfig > > @@ -649,6 +649,15 @@ config REGULATOR_MAX77650 > > Semiconductor. This device has a SIMO with three independent > > power rails and an LDO. > > > > +config REGULATOR_MAX77675 > > + tristate "Maxim MAX77675 regulator driver" > > + depends on I2C > > Looking at your code, I would expected OF to be a dependency as well. > I’ll add OF as a dependency > > + select REGMAP_I2C > > + help > > + This driver controls the Maxim MAX77675 power regulator via I2C. > > + It supports four programmable buck-boost outputs. > > + Say Y here to enable the regulator driver > > + > > config REGULATOR_MAX77857 > > tristate "ADI MAX77857/MAX77831 regulator support" > > depends on I2C > > diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile > > index b3101376029d..cdd99669cd24 100644 > > --- a/drivers/regulator/Makefile > > +++ b/drivers/regulator/Makefile > > @@ -78,6 +78,7 @@ obj-$(CONFIG_REGULATOR_MAX77503) += max77503-regulator.o > > obj-$(CONFIG_REGULATOR_MAX77541) += max77541-regulator.o > > obj-$(CONFIG_REGULATOR_MAX77620) += max77620-regulator.o > > obj-$(CONFIG_REGULATOR_MAX77650) += max77650-regulator.o > > +obj-$(CONFIG_REGULATOR_MAX77675) += max77675-regulator.o > > obj-$(CONFIG_REGULATOR_MAX8649) += max8649.o > > obj-$(CONFIG_REGULATOR_MAX8660) += max8660.o > > obj-$(CONFIG_REGULATOR_MAX8893) += max8893.o > > diff --git a/drivers/regulator/max77675-regulator.c b/drivers/regulator/max77675-regulator.c > > new file mode 100644 > > index 000000000000..c1281f07fe43 > > --- /dev/null > > +++ b/drivers/regulator/max77675-regulator.c > > @@ -0,0 +1,861 @@ > > +// SPDX-License-Identifier: GPL-2.0-or-later > > +/* > > + * Copyright (c) 2025 Analog Devices, Inc. > > + * ADI regulator driver for MAX77675. > > + */ > > + > > +#include <linux/module.h> > > +#include <linux/slab.h> > > +#include <linux/of.h> > > +#include <linux/i2c.h> > > +#include <linux/regmap.h> > > +#include <linux/platform_device.h> > > +#include <linux/regulator/driver.h> > > +#include <linux/regulator/consumer.h> > > +#include <linux/regulator/of_regulator.h> > > +#include <linux/bitfield.h> > > You're clearly missing (at least) mod_devicetable.h. I know that at least clang allows you to get > iwyu, > I will add it. > > + > > +#include "max77675-regulator.h" > > Why do we need the header file? Just include everything in the source code unless you expect to > share something with another module (which I dunno)? > I will remove the header file as suggested > > + > > +struct max77675_regulator_pdata { > > + u8 fps_slot; > > + bool fixed_slew_rate; > > +}; > > + > > I would get rid of the _pdata suffix. Implies some legacy way of doing things (but kind of a > nitpick) > I will rename it to max77675_regulator_sbb_setting. > > +struct max77675_config { > > + u8 en_mode; > > + u8 voltage_change_latency; > > + u8 drv_sbb_strength; > > + u8 dvs_slew_rate; > > + u8 debounce_time; > > + u8 manual_reset_time; > > + bool en_pullup_disable; > > + bool bias_low_power_request; > > + bool simo_int_ldo_always_on; > > +}; > > + > > +struct max77675_regulator { > > + struct device *dev; > > + struct regmap *regmap; > > + struct max77675_config config; > > + struct max77675_regulator_pdata pdata[MAX77675_ID_NUM_MAX]; > > +}; > > + > > +/** > > + * Set latency mode. > > + * > > + * @param maxreg Pointer to max77675 device structure. > > + * @param enable true to enable latency mode, false to disable. > > + */ > > +static int max77675_set_latency_mode(struct max77675_regulator *maxreg, bool enable) > > +{ > > + return regmap_update_bits(maxreg->regmap, MAX77675_REG_CNFG_SBB_TOP_B, > > + MAX77675_LAT_MODE_BIT, > > + FIELD_PREP(MAX77675_LAT_MODE_BIT, enable)); > > +} > > + > > I would drop these one liner wrappers. Personally, I don't see a big benefit on it. > I agree with your point and will remove these one-liner wrappers as suggested > > +/** > > + * Set DVS slew rate mode. > > + * > > + * @param maxreg Pointer to max77675 device structure. > > + * @param enable true to use DVS-controlled slew rate, false for fixed 2mV/us. > > + */ > > +static int max77675_set_dvs_slew_rate(struct max77675_regulator *maxreg, bool enable) > > +{ > > + return regmap_update_bits(maxreg->regmap, MAX77675_REG_CNFG_SBB_TOP_B, > > + MAX77675_DVS_SLEW_BIT, > > + FIELD_PREP(MAX77675_DVS_SLEW_BIT, enable)); > > +} > > + > > Ditto for all other places. > > ... > > > > > + > > +/** > > + * Set debounce time for EN pin. > > + * > > + * @param maxreg Pointer to max77675 device structure. > > + * @param debounce_30ms true for 30ms, false for 100us > > + */ > > +static int max77675_set_debounce_time(struct max77675_regulator *maxreg, bool debounce_30ms) > > +{ > > + return regmap_update_bits(maxreg->regmap, MAX77675_REG_CNFG_GLBL_A, > > + MAX77675_DBEN_EN_BIT, > > + FIELD_PREP(MAX77675_DBEN_EN_BIT, debounce_30ms)); > > +} > > + > > +static int max77675_regulator_get_fps_src(struct max77675_regulator *maxreg, int id) > > +{ > > + unsigned int reg_addr; > > + unsigned int val; > > + int ret; > > + > > + switch (id) { > > + case MAX77675_ID_SBB0: > > + reg_addr = MAX77675_REG_CNFG_SBB0_B; > > + break; > > + case MAX77675_ID_SBB1: > > + reg_addr = MAX77675_REG_CNFG_SBB1_B; > > + break; > > + case MAX77675_ID_SBB2: > > + reg_addr = MAX77675_REG_CNFG_SBB2_B; > > + break; > > + case MAX77675_ID_SBB3: > > + reg_addr = MAX77675_REG_CNFG_SBB3_B; > > + break; > > + default: > > + dev_err(maxreg->dev, "Invalid regulator id: %d\n", id); > > + return -EINVAL; > > + } > > + > > + ret = regmap_read(maxreg->regmap, reg_addr, &val); > > + if (ret < 0) { > > + dev_err(maxreg->dev, "Failed to read FPS source (reg 0x%02x): %d\n", > > + reg_addr, ret); > > + return ret; > > + } > > + > > + return val & MAX77675_EN_SBB_MASK; > > Ok, this works since the mask is 0x7. However, FIELD_GET() would make it more > readable and easy to review. I mean, I would not need to go and see the mask value. > I will change it to "return FIELD_GET(MAX77675_EN_SBB_MASK, val)" > > +} > > + > > +static int max77675_regulator_set_fps_src(struct max77675_regulator *maxreg, int id, u8 fps_src) > > +{ > > + unsigned int reg_addr; > > + int ret; > > + > > + switch (id) { > > + case MAX77675_ID_SBB0: > > + reg_addr = MAX77675_REG_CNFG_SBB0_B; > > + break; > > + case MAX77675_ID_SBB1: > > + reg_addr = MAX77675_REG_CNFG_SBB1_B; > > + break; > > + case MAX77675_ID_SBB2: > > + reg_addr = MAX77675_REG_CNFG_SBB2_B; > > + break; > > + case MAX77675_ID_SBB3: > > + reg_addr = MAX77675_REG_CNFG_SBB3_B; > > + break; > > + default: > > + dev_err(maxreg->dev, "Invalid regulator id: %d\n", id); > > + return -EINVAL; > > + } > > + > > + ret = regmap_update_bits(maxreg->regmap, reg_addr, > > + MAX77675_EN_SBB_MASK, fps_src); > > + if (ret < 0) { > > + dev_err(maxreg->dev, "Failed to set FPS source (reg 0x%02x): %d\n", > > + reg_addr, ret); > > + return ret; > > + } > > I would drop the log and just do return regmap_update_bits(). Up to you... > I will change it > > + > > + return 0; > > +} > > + > > +/** > > + * max77675_set_sbb_slew_rate_fixed - Set the slew rate for a specific SBB regulator channel > > + * > > + * @maxreg: Pointer to the max77675 regulator structure > > + * @id: Regulator channel ID (ID_SBB0 ~ ID_SBB3) > > + * @fixed: Slew rate value (true = 2mV/us, false = use DVS_SLEW) > > + * > > + * This function configures the slew rate control source for the specified SBB channel by > > + * updating the corresponding bits in the CNFG_SBB_TOP_B register. > > + * > > + * Return: 0 on success, negative error code on failure (e.g., invalid channel ID). > > + */ > > +static int max77675_set_sbb_slew_rate_fixed(struct max77675_regulator *maxreg, int id, bool > > fixed) > > +{ > > + u8 mask, value; > > + u8 slew_src_ctrl_bit = fixed ? 0 : 1; > > + > > + switch (id) { > > + case MAX77675_ID_SBB0: > > + mask = MAX77675_SR_SBB0_BIT; > > + value = FIELD_PREP(MAX77675_SR_SBB0_BIT, slew_src_ctrl_bit); > > + break; > > + > > + case MAX77675_ID_SBB1: > > + mask = MAX77675_SR_SBB1_BIT; > > + value = FIELD_PREP(MAX77675_SR_SBB1_BIT, slew_src_ctrl_bit); > > + break; > > + > > + case MAX77675_ID_SBB2: > > + mask = MAX77675_SR_SBB2_BIT; > > + value = FIELD_PREP(MAX77675_SR_SBB2_BIT, slew_src_ctrl_bit); > > + break; > > + > > + case MAX77675_ID_SBB3: > > + mask = MAX77675_SR_SBB3_BIT; > > + value = FIELD_PREP(MAX77675_SR_SBB3_BIT, slew_src_ctrl_bit); > > + break; > > + > > + default: > > + return -EINVAL; > > + } > > + > > + return regmap_update_bits(maxreg->regmap, MAX77675_REG_CNFG_SBB_TOP_B, mask, value); > > +} > > + > > +static int max77675_init_regulator(struct max77675_regulator *maxreg, int id) > > +{ > > + struct max77675_regulator_pdata *rpdata = &maxreg->pdata[id]; > > + int ret; > > + > > + if (rpdata->fps_slot == MAX77675_FPS_DEF) { > > + ret = max77675_regulator_get_fps_src(maxreg, id); > > + if (ret < 0) { > > + dev_err(maxreg->dev, "Failed to read FPS source for ID %d\n", id); > > + return ret; > > + } > > + rpdata->fps_slot = ret; > > + } else { > > + ret = max77675_regulator_set_fps_src(maxreg, id, rpdata->fps_slot); > > + if (ret) > > + dev_warn(maxreg->dev, "Failed to set FPS source for ID %d\n", id); > > + } > > + > > + ret = max77675_set_sbb_slew_rate_fixed(maxreg, id, rpdata->fixed_slew_rate); > > + if (ret) > > + dev_warn(maxreg->dev, "Failed to set slew rate for ID %d\n", id); > > Do we really want to treat this as a warning (as FPS)? If so, I would expect a proper > comment explaining why we can afford it. > As this could impact proper operation, I’ll make the function return an error. > > + > > + return 0; > > +} > > + > > +static int max77675_of_parse_cb(struct device_node *np, > > + const struct regulator_desc *desc, > > + struct regulator_config *config) > > +{ > > + struct max77675_regulator *maxreg = config->driver_data; > > + struct max77675_regulator_pdata *rpdata = &maxreg->pdata[desc->id]; > > + u32 pval; > > + int ret; > > + > > + /* Parse FPS slot from DT */ > > + ret = of_property_read_u32(np, "maxim,fps-slot", &pval); > > + rpdata->fps_slot = (!ret) ? (u8)pval : MAX77675_FPS_DEF; > > + > > So, can we get any arbitrary value for pval? I see we you have an enum in > the bindings so make sure we properly validate it. Same for all other > properties. The bindings also have this as a string and here you have a u32? > Not going to work. You need of_property_read_string() + match_string(). Thanks, I will change it > > > Also, "maxim,"? For some time now it's "adi,". > I will change it > > + /* Parse slew rate control source */ > > + rpdata->fixed_slew_rate = of_property_read_bool(np, "maxim,fixed-slew-rate"); > > + > > + /* Apply parsed configuration */ > > + return max77675_init_regulator(maxreg, desc->id); > > +} > > + > > +static int max77675_get_error_flags(struct regulator_dev *rdev, unsigned int *flags) > > +{ > > + struct max77675_regulator *maxreg = rdev_get_drvdata(rdev); > > + unsigned int int_flags; > > + int id = rdev_get_id(rdev); > > + int ret; > > + > > + ret = regmap_read(maxreg->regmap, MAX77675_REG_INT_GLBL, &int_flags); > > + if (ret) { > > + dev_err(maxreg->dev, "Failed to read INT_GLBL: %d\n", ret); > > + return ret; > > + } > > + > > + *flags = 0; > > + > > + switch (id) { > > + case MAX77675_ID_SBB0: > > + if (int_flags & MAX77675_INT_SBB0_F_BIT) > > + *flags |= REGULATOR_ERROR_FAIL; > > + break; > > + case MAX77675_ID_SBB1: > > + if (int_flags & MAX77675_INT_SBB1_F_BIT) > > + *flags |= REGULATOR_ERROR_FAIL; > > + break; > > + case MAX77675_ID_SBB2: > > + if (int_flags & MAX77675_INT_SBB2_F_BIT) > > + *flags |= REGULATOR_ERROR_FAIL; > > + break; > > + case MAX77675_ID_SBB3: > > + if (int_flags & MAX77675_INT_SBB3_F_BIT) > > + *flags |= REGULATOR_ERROR_FAIL; > > + break; > > + default: > > + dev_warn(maxreg->dev, "Unsupported regulator ID: %d\n", id); > > + break; > > Should not be a warning. Also wonder if it can happen at all? > Agreed, this case shouldn’t happen. I’ll drop the warning. > > + } > > + > > + if (int_flags & MAX77675_INT_TJAL2_R_BIT) { > > + /* TJAL2 interrupt: Over-temperature condition (above 120 degree) */ > > + *flags |= REGULATOR_ERROR_OVER_TEMP; > > + } > > + > > + return 0; > > +} > > + > > +static const struct regulator_ops max77675_regulator_ops = { > > + .list_voltage = regulator_list_voltage_linear, > > + .enable = regulator_enable_regmap, > > + .disable = regulator_disable_regmap, > > + .is_enabled = regulator_is_enabled_regmap, > > + .map_voltage = regulator_map_voltage_linear, > > + .set_voltage_sel = regulator_set_voltage_sel_regmap, > > + .get_voltage_sel = regulator_get_voltage_sel_regmap, > > + .set_active_discharge = regulator_set_active_discharge_regmap, > > + .get_error_flags = max77675_get_error_flags, > > +}; > > + > > +static struct regulator_desc max77675_regulators[MAX77675_ID_NUM_MAX] = { > > + { > > + .name = "sbb0", > > + .of_match = of_match_ptr("sbb0"), > > + .regulators_node = of_match_ptr("regulators"), > > I wonder if we need of_match_ptr() given that the whole thing depends on OF. > > ... > I agree with you. If there are no objections, I will remove it. > > + > > +static int max77675_apply_config(struct max77675_regulator *maxreg) > > +{ > > + const struct max77675_config *config = &maxreg->config; > > + int ret; > > + > > + ret = max77675_set_en_mode(maxreg, config->en_mode); > > + if (ret) { > > + dev_err(maxreg->dev, "Failed to set EN mode: %d\n", ret); > > + return ret; > > + } > > + > > + ret = max77675_set_latency_mode(maxreg, config->voltage_change_latency); > > + if (ret) { > > + dev_err(maxreg->dev, "Failed to set latency mode: %d\n", ret); > > + return ret; > > + } > > + > > + ret = max77675_set_drv_sbb_strength(maxreg, config->drv_sbb_strength); > > + if (ret) { > > + dev_err(maxreg->dev, "Failed to set drive strength: %d\n", ret); > > + return ret; > > + } > > + > > + ret = max77675_set_dvs_slew_rate(maxreg, config->dvs_slew_rate); > > + if (ret) { > > + dev_err(maxreg->dev, "Failed to set DVS slew rate: %d\n", ret); > > + return ret; > > + } > > + > > + ret = max77675_set_debounce_time(maxreg, config->debounce_time); > > + if (ret) { > > + dev_err(maxreg->dev, "Failed to set EN debounce time: %d\n", ret); > > + return ret; > > + } > > + > > + ret = max77675_set_manual_reset_time(maxreg, config->manual_reset_time); > > + if (ret) { > > + dev_err(maxreg->dev, "Failed to set manual reset time: %d\n", ret); > > + return ret; > > + } > > + > > + ret = max77675_set_en_pullup_disable(maxreg, config->en_pullup_disable); > > + if (ret) { > > + dev_err(maxreg->dev, "Failed to set EN pull-up disable: %d\n", ret); > > + return ret; > > + } > > + > > + ret = max77675_set_bias_low_power_request(maxreg, config->bias_low_power_request); > > + if (ret) { > > + dev_err(maxreg->dev, "Failed to set bias low-power request: %d\n", ret); > > + return ret; > > + } > > + > > + ret = max77675_set_simo_int_ldo_always_on(maxreg, config->simo_int_ldo_always_on); > > + if (ret) { > > + dev_err(maxreg->dev, "Failed to set SIMO internal LDO always-on: %d\n", ret); > > + return ret; > > + } > > This seems to called during probe. All the above can be return dev_err_probe()... > I’m thinking of handling it directly where the return value is checked. What are your thoughts? ret = max77675_apply_config(maxreg); if (ret) return dev_err_probe(maxreg->dev, ret, "Failed to apply config\n"); > > + > > + return 0; > > +} > > + > > +static u8 max77675_parse_voltage_change_latency(struct device_node *np) > > +{ > > + u32 val; > > + > > + if (!of_property_read_u32(np, "maxim,voltage-change-latency-us", &val)) { > > + switch (val) { > > + case 10: > > + return MAX77675_LOW_LATENCY_MODE; > > + case 100: > > + return MAX77675_HIGH_LATENCY_MODE; > > + default: > > + break; > > The above is wrong. You're ignoring invalid values being given and overwrite them > with defaults. The pattern is: > > val = MAX77675_HIGH_LATENCY_MODE; > if (!of_property_read_u32(np, "maxim,voltage-change-latency-us", &val)) { > ... > default: > return dev_err_probe(dev, -EINVAL, ...); > } > > You can also do: > > ret = of_property_read_u32(...); > /* Not 100% sure if -EINVAL is the error code for a missing property */ > if (ret && ret != -EINVAL) > return ret; > if (!ret) { > ... > } > I will update the code to set a default value first and handle invalid values by returning an error. > > + } > > + } > > + > > + /* default: high latency */ > > + return MAX77675_HIGH_LATENCY_MODE; > > +} > > + > > +static u8 max77675_parse_en_mode(struct device_node *np) > > +{ > > + const char *str; > > + > > + if (!of_property_read_string(np, "maxim,en-mode", &str)) { > > + if (!strcasecmp(str, "push-button")) > > + return MAX77675_EN_PUSH_BUTTON; > > + else if (!strcasecmp(str, "slide-switch")) > > + return MAX77675_EN_SLIDE_SWITCH; > > + else if (!strcasecmp(str, "logic")) > > + return MAX77675_EN_LOGIC; > > redundant else if(). Moreover, this looks like it could use match_string() > I will update it to use match_string. > > + } > > + > > + /* default : slide-switch */ > > + return MAX77675_EN_SLIDE_SWITCH; > > +} > > + > > +static u8 max77675_parse_manual_reset_time(struct device_node *np) > > +{ > > + u32 val; > > + > > + if (!of_property_read_u32(np, "reset-time-sec", &val)) { > > + switch (val) { > > + case 4: > > + return MAX77675_MRT_4S; > > + case 8: > > + return MAX77675_MRT_8S; > > + case 12: > > + return MAX77675_MRT_12S; > > + case 16: > > + return MAX77675_MRT_16S; > > + default: > > + break; > > Ditto. > > ... > > > + > > +static int max77675_parse_config(struct max77675_regulator *maxreg) > > +{ > > + struct device_node *np = maxreg->dev->of_node; > > + struct max77675_config *config = &maxreg->config; > > + int ret; > > + > > + /* EN pin mode: push-button, slide-switch, or logic */ > > + config->en_mode = max77675_parse_en_mode(np); > > + > > + /* latency mode */ > > + config->voltage_change_latency = max77675_parse_voltage_change_latency(np); > > + > > + /* drive strength */ > > + config->drv_sbb_strength = max77675_parse_drv_sbb_strength(np); > > + > > + /* drv slew rate */ > > + config->dvs_slew_rate = max77675_parse_dvs_slew_rate(np); > > + > > + /* Debounce time for EN pin */ > > + config->debounce_time = max77675_parse_debounce_time_us(np); > > + > > + /* Manual reset time for EN pin */ > > + config->manual_reset_time = max77675_parse_manual_reset_time(np); > > Seems to me that all of the above will need some error handling. > I will update the code to handle errors. > > + > > + /* Disable internal pull-up resistor on EN pin */ > > + config->en_pullup_disable = of_property_read_bool(np, "maxim,en-pullup-disable"); > > + > > + /* Request low-power mode for main bias */ > > + config->bias_low_power_request = of_property_read_bool(np, "maxim,bias-low-power- > > request"); > > + > > + /* Force internal LDO to always supply 1.8V */ > > + config->simo_int_ldo_always_on = of_property_read_bool(np, "maxim,simo-int-ldo-always- > > on"); > > + > > + ret = max77675_apply_config(maxreg); > > + > > + return ret; > > +} > > + > > +static int max77675_init_event(struct max77675_regulator *maxreg) > > +{ > > + unsigned int ercflag, int_glbl; > > + int ret; > > + > > + ret = regmap_read(maxreg->regmap, MAX77675_REG_ERCF_GLBL, &ercflag); > > + if (ret) { > > + dev_err(maxreg->dev, "Failed to read CID register: %d\n", ret); > > + return ret; > > + } > > + > > + ret = regmap_read(maxreg->regmap, MAX77675_REG_INT_GLBL, &int_glbl); > > + if (ret) { > > + dev_err(maxreg->dev, "Failed to read INT_GLBL register: %d\n", ret); > > + return ret; > > + } > > + > > + if (ercflag & MAX77675_SFT_CRST_F_BIT) > > + dev_info(maxreg->dev, "Software Cold Reset Flag is set\n"); > > + > > + if (ercflag & MAX77675_SFT_OFF_F_BIT) > > + dev_info(maxreg->dev, "Software Off Flag is set\n"); > > + > > + if (ercflag & MAX77675_MRST_BIT) > > + dev_info(maxreg->dev, "Manual Reset Timer Flag is set\n"); > > + > > + if (ercflag & MAX77675_UVLO_BIT) > > + dev_info(maxreg->dev, "Undervoltage Lockout Flag is set\n"); > > + > > + if (ercflag & MAX77675_OVLO_BIT) > > + dev_info(maxreg->dev, "Overvoltage Lockout Flag is set\n"); > > + > > + if (ercflag & MAX77675_TOVLD_BIT) > > + dev_info(maxreg->dev, "Thermal Overload Flag is set\n"); > > + > > + if (int_glbl & MAX77675_INT_SBB3_F_BIT) > > + dev_info(maxreg->dev, "SBB3 Channel Fault Interrupt occurred\n"); > > + > > + if (int_glbl & MAX77675_INT_SBB2_F_BIT) > > + dev_info(maxreg->dev, "SBB2 Channel Fault Interrupt occurred\n"); > > + > > + if (int_glbl & MAX77675_INT_SBB1_F_BIT) > > + dev_info(maxreg->dev, "SBB1 Channel Fault Interrupt occurred\n"); > > + > > + if (int_glbl & MAX77675_INT_SBB0_F_BIT) > > + dev_info(maxreg->dev, "SBB0 Channel Fault Interrupt occurred\n"); > > + > > + if (int_glbl & MAX77675_INT_TJAL2_R_BIT) > > + dev_info(maxreg->dev, "Thermal Alarm 2 Rising Interrupt occurred\n"); > > + > > + if (int_glbl & MAX77675_INT_TJAL1_R_BIT) > > + dev_info(maxreg->dev, "Thermal Alarm 1 Rising Interrupt occurred\n"); > > + > > + if (int_glbl & MAX77675_INT_EN_R_BIT) > > + dev_info(maxreg->dev, "nEN Rising Edge Interrupt occurred\n"); > > + > > + if (int_glbl & MAX77675_INT_EN_F_BIT) > > + dev_info(maxreg->dev, "nEN Falling Edge Interrupt occurred\n"); > > All of the above looks like dev_dbg() to me. > I will change it > > + > > + return 0; > > +} > > + > > +static int max77675_regulator_probe(struct i2c_client *client) > > +{ > > + struct max77675_regulator *maxreg; > > + struct regulator_config config = {}; > > + struct device_node *regulators_np; > > + int i, ret; > > + > > + maxreg = devm_kzalloc(&client->dev, sizeof(*maxreg), GFP_KERNEL); > > + if (!maxreg) > > + return -ENOMEM; > > + > > + maxreg->dev = &client->dev; > > + > > + maxreg->regmap = devm_regmap_init_i2c(client, &max77675_regmap_config); > > + if (IS_ERR(maxreg->regmap)) > > + return dev_err_probe(maxreg->dev, > > + PTR_ERR(maxreg->regmap), > > + "Failed to init regmap\n"); > > + > > + ret = max77675_init_event(maxreg); > > + if (ret) > > + return dev_err_probe(maxreg->dev, ret, "Failed to init event\n"); > > + > > + ret = max77675_parse_config(maxreg); > > + if (ret) > > + return dev_err_probe(maxreg->dev, ret, "Failed to apply config\n"); > > + > > + config.dev = &client->dev; > > + config.regmap = maxreg->regmap; > > + config.driver_data = maxreg; > > + > > + regulators_np = of_get_child_by_name(client->dev.of_node, "regulators"); > > The above can actually be: > > struct device_node *regulators_np __free(device_node) = of_get_child_by_name(...) > > and then no need to care about of_node_put(). You need cleanup.h > Thanks fot letting me know, I will change it > > + if (!regulators_np) { > > + dev_err(maxreg->dev, "No 'regulators' subnode found in DT\n"); > > + return -EINVAL; > > + } > > + > > + for (i = 0; i < MAX77675_ID_NUM_MAX; i++) { > > + const struct regulator_desc *desc = &max77675_regulators[i]; > > + struct regulator_dev *rdev; > > + > > + config.of_node = of_get_child_by_name(regulators_np, desc->name); > > + if (!config.of_node) { > > + dev_warn(maxreg->dev, "No DT node for regulator %s\n", desc->name); > > + continue; > > + } > > + > > + rdev = devm_regulator_register(&client->dev, desc, &config); > > + of_node_put(config.of_node); > > + if (IS_ERR(rdev)) { > > + of_node_put(regulators_np); > > + return dev_err_probe(maxreg->dev, PTR_ERR(rdev), > > + "Failed to register regulator %d (%s): %d\n", > > + i, desc->name, ret); > > + } > > + } > > + > > + of_node_put(regulators_np); > > + i2c_set_clientdata(client, maxreg); > > I do not see i2c_get_clientdata() anywhere. I suspect you can drop the above. > I will remove it > > + > > + return 0; > > +} > > + > > +static const struct i2c_device_id max77675_i2c_id[] = { > > + { "max77675", 0 }, > > + { } > > +}; > > +MODULE_DEVICE_TABLE(i2c, max77675_i2c_id); > > + > > +static const struct of_device_id __maybe_unused max77675_of_match[] = { > > + { .compatible = "maxim,max77675", }, > > + { } > > +}; > > +MODULE_DEVICE_TABLE(of, max77675_of_match); > > + > > +static struct i2c_driver max77675_regulator_driver = { > > + .driver = { > > + .name = "max77675", > > + .of_match_table = of_match_ptr(max77675_of_match), > > I guess we can drop of_match_ptr() if we agree that we depend on OF > I agree with you. If there are no objections, I will remove it. > > + }, > > + .probe = max77675_regulator_probe, > > + .id_table = max77675_i2c_id, > > +}; > > + > > +module_i2c_driver(max77675_regulator_driver); > > + > > +MODULE_DESCRIPTION("MAX77675 Regulator Driver"); > > +MODULE_AUTHOR("Joan Na <joan.na@analog.com>"); > > +MODULE_LICENSE("GPL"); > > diff --git a/drivers/regulator/max77675-regulator.h b/drivers/regulator/max77675-regulator.h > > new file mode 100644 > > index 000000000000..0aaa30a630ca > > --- /dev/null > > +++ b/drivers/regulator/max77675-regulator.h > > As said, drop the header. > I will drop the header. > - Nuno Sá -- Best regards, Joan Na ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v5 2/2] regulator: max77675: Add MAX77675 regulator driver 2025-11-03 2:45 ` Joan Na @ 2025-11-03 11:04 ` Nuno Sá 0 siblings, 0 replies; 8+ messages in thread From: Nuno Sá @ 2025-11-03 11:04 UTC (permalink / raw) To: Joan Na Cc: Liam Girdwood, Mark Brown, Rob Herring, Krzysztof Kozlowski, Conor Dooley, linux-kernel, devicetree, Joan Na On Mon, 2025-11-03 at 11:45 +0900, Joan Na wrote: > On Wed, Oct 29, 2025 at 09:55:53AM +0000, Nuno Sá wrote: > > On Wed, 2025-10-29 at 11:32 +0900, Joan-Na-adi wrote: > > > From: Joan Na <joan.na@analog.com> > > > > > > Add support for the Maxim Integrated MAX77675 PMIC regulator. > > > > > > The MAX77675 is a compact, highly efficient SIMO (Single Inductor Multiple Output) > > > power management IC that provides four programmable buck-boost switching regulators > > > with only one inductor. It supports up to 700mA total output current and operates > > > from a single-cell Li-ion battery. > > > > > > An integrated power-up sequencer and I2C interface allow flexible startup > > > configuration and runtime control. > > > > > > Signed-off-by: Joan Na <joan.na@analog.com> > > > --- > > > > Hi Joan, > > > > Some comments from me... > > > > Hello Nuno, > > Thank you for taking the time to review. > Please refer to my response below. > ... > > > > > > + > > > +static int max77675_apply_config(struct max77675_regulator *maxreg) > > > +{ > > > + const struct max77675_config *config = &maxreg->config; > > > + int ret; > > > + > > > + ret = max77675_set_en_mode(maxreg, config->en_mode); > > > + if (ret) { > > > + dev_err(maxreg->dev, "Failed to set EN mode: %d\n", ret); > > > + return ret; > > > + } > > > + > > > + ret = max77675_set_latency_mode(maxreg, config->voltage_change_latency); > > > + if (ret) { > > > + dev_err(maxreg->dev, "Failed to set latency mode: %d\n", ret); > > > + return ret; > > > + } > > > + > > > + ret = max77675_set_drv_sbb_strength(maxreg, config->drv_sbb_strength); > > > + if (ret) { > > > + dev_err(maxreg->dev, "Failed to set drive strength: %d\n", ret); > > > + return ret; > > > + } > > > + > > > + ret = max77675_set_dvs_slew_rate(maxreg, config->dvs_slew_rate); > > > + if (ret) { > > > + dev_err(maxreg->dev, "Failed to set DVS slew rate: %d\n", ret); > > > + return ret; > > > + } > > > + > > > + ret = max77675_set_debounce_time(maxreg, config->debounce_time); > > > + if (ret) { > > > + dev_err(maxreg->dev, "Failed to set EN debounce time: %d\n", ret); > > > + return ret; > > > + } > > > + > > > + ret = max77675_set_manual_reset_time(maxreg, config->manual_reset_time); > > > + if (ret) { > > > + dev_err(maxreg->dev, "Failed to set manual reset time: %d\n", ret); > > > + return ret; > > > + } > > > + > > > + ret = max77675_set_en_pullup_disable(maxreg, config->en_pullup_disable); > > > + if (ret) { > > > + dev_err(maxreg->dev, "Failed to set EN pull-up disable: %d\n", ret); > > > + return ret; > > > + } > > > + > > > + ret = max77675_set_bias_low_power_request(maxreg, config->bias_low_power_request); > > > + if (ret) { > > > + dev_err(maxreg->dev, "Failed to set bias low-power request: %d\n", ret); > > > + return ret; > > > + } > > > + > > > + ret = max77675_set_simo_int_ldo_always_on(maxreg, config->simo_int_ldo_always_on); > > > + if (ret) { > > > + dev_err(maxreg->dev, "Failed to set SIMO internal LDO always-on: %d\n", ret); > > > + return ret; > > > + } > > > > This seems to called during probe. All the above can be return dev_err_probe()... > > > > I’m thinking of handling it directly where the return value is checked. What are your thoughts? > > ret = max77675_apply_config(maxreg); > if (ret) > return dev_err_probe(maxreg->dev, ret, "Failed to apply config\n"); Up to you. You'll have less details if you encounter some issue though... - Nuno Sá > ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2025-11-03 11:04 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2025-10-29 2:32 [PATCH v5 0/2] Add support for MAX77675 device Joan-Na-adi 2025-10-29 2:32 ` [PATCH v5 1/2] regulator: dt-bindings: Add MAX77675 regulator binding Joan-Na-adi 2025-10-29 5:52 ` Krzysztof Kozlowski 2025-10-31 11:24 ` Joan Na 2025-10-29 2:32 ` [PATCH v5 2/2] regulator: max77675: Add MAX77675 regulator driver Joan-Na-adi 2025-10-29 9:55 ` Nuno Sá 2025-11-03 2:45 ` Joan Na 2025-11-03 11:04 ` Nuno Sá
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).