Skip to content

Comments

fw/drivers/pmic: replace npm1300 platform ifdefs with board-level config#829

Open
leopoldch wants to merge 2 commits intocoredevices:mainfrom
leopoldch:refactor/nPM1300
Open

fw/drivers/pmic: replace npm1300 platform ifdefs with board-level config#829
leopoldch wants to merge 2 commits intocoredevices:mainfrom
leopoldch:refactor/nPM1300

Conversation

@leopoldch
Copy link

Issue #233

Why

The npm1300 driver used hardcoded #if PLATFORM_XXX. Several TODO and FIXME comments in the code already acknowledged this needed to be cleaned up. Adding a new board meant editing the driver itself, which is fragile and hard to review.

What

All platform-specific PMIC configuration is moved into new fields in Npm1300Config (voltages, enable flags, modes, SW control, erratum workaround). Each board file (board_asterix.c, board_obelix.c, board_getafix.c) now declares its own complete power rail config, and the driver in npm1300.c applies it generically without any #if PLATFORM_*.

Note

The patch compiles cleanly but has not been tested on hardware.
Also note that some magic values remain in this part of the codebase

Copy link
Member

@gmarull gmarull left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

follow commit guidelines

Comment on lines 211 to 215
.chg_current_ma = 128,
.dischg_limit_ma = 200,
.term_current_pct = 10,
.thermistor_beta = 3380,
.vterm_setting = NPM1300_VTERM_4V20,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please, expose human readable settings at board level, no magic constants that are impl details

…ble per board

Signed-off-by: Léopold Chappuis <leopold@lchappuis.fr>
…le via the Npm1300Config struct

Signed-off-by: Léopold Chappuis <leopold@lchappuis.fr>
@leopoldch
Copy link
Author

@gmarull I rewrote my commit messages to include Signed-off-by: and added comments to avoid magic values. Let me know if you’d like me to store these in separate variables 🙂

Comment on lines +214 to +223
.vterm_setting = NPM1300_VTERM_4V20,

// Buck1 (1.8V)
.buck1_enable = true,
.buck1_voltage_sel = 8, // 1.8V

// Buck2 (3.0V)
.buck2_enable = true,
.buck2_voltage_sel = 20, // 3.0V
.buck_sw_ctrl_sel = 3, // both of them, load
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

feedback not addressed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants