Re: [PATCH v1 3/6] mfd: support ROHM BD96801 PMIC core

From: Matti Vaittinen
Date: Fri May 03 2024 - 04:44:01 EST


On 5/3/24 10:41, Lee Jones wrote:
On Tue, 30 Apr 2024, Matti Vaittinen wrote:

The ROHM BD96801 PMIC is highly customizable automotive grade PMIC
which integrates regulator and watchdog funtionalities.

Provide INTB IRQ and register accesses for regulator/watchdog drivers.

Signed-off-by: Matti Vaittinen <mazziesaccount@xxxxxxxxx>

---
Changelog: RFCv2 => v1:
- drop ERRB interrupts (for now)
- bd96801: Unlock registers in core driver

Changelog: RFCv1 => RFCv2
- Work-around the IRQ domain name conflict
- Add watchdog IRQ
- Various styling fixes based on review by Lee
---
drivers/mfd/Kconfig | 13 ++
drivers/mfd/Makefile | 1 +
drivers/mfd/rohm-bd96801.c | 278 +++++++++++++++++++++++++++++++
include/linux/mfd/rohm-bd96801.h | 215 ++++++++++++++++++++++++
include/linux/mfd/rohm-generic.h | 1 +
5 files changed, 508 insertions(+)
create mode 100644 drivers/mfd/rohm-bd96801.c
create mode 100644 include/linux/mfd/rohm-bd96801.h

Still some nits, sorry.

No need to be sorry :) And, thanks for the review!

I probably wouldn't have been so picky if I hadn't have found the unused enum.

Yeah, that's what you say ... XD
It's Ok. Besides, I like your style of providing the better alternatives along with your comments. (ref the print comments which I agreed with and dropped from this reply).

diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index 4b023ee229cf..9e874453d94d 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -2089,6 +2089,19 @@ config MFD_ROHM_BD957XMUF
BD9573MUF Power Management ICs. BD9576 and BD9573 are primarily
designed to be used to power R-Car series processors.
+config MFD_ROHM_BD96801
+ tristate "ROHM BD96801 Power Management IC"
+ depends on I2C=y
+ depends on OF
+ select REGMAP_I2C
+ select REGMAP_IRQ
+ select MFD_CORE
+ help
+ Select this option to get support for the ROHM BD96801 Power
+ Management IC. The ROHM BD96801 is a highly scalable Power Management
+ IC for industrial and automotive use. The BD96801 can be used as a
+ master PMIC in a chained PMIC solution with suitable companion PMICs.
+
config MFD_STM32_LPTIMER
tristate "Support for STM32 Low-Power Timer"
depends on (ARCH_STM32 && OF) || COMPILE_TEST
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index c66f07edcd0e..e792892d4a8b 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -264,6 +264,7 @@ obj-$(CONFIG_RAVE_SP_CORE) += rave-sp.o
obj-$(CONFIG_MFD_ROHM_BD71828) += rohm-bd71828.o
obj-$(CONFIG_MFD_ROHM_BD718XX) += rohm-bd718x7.o
obj-$(CONFIG_MFD_ROHM_BD957XMUF) += rohm-bd9576.o
+obj-$(CONFIG_MFD_ROHM_BD96801) += rohm-bd96801.o
obj-$(CONFIG_MFD_STMFX) += stmfx.o
obj-$(CONFIG_MFD_KHADAS_MCU) += khadas-mcu.o
obj-$(CONFIG_MFD_ACER_A500_EC) += acer-ec-a500.o
diff --git a/drivers/mfd/rohm-bd96801.c b/drivers/mfd/rohm-bd96801.c
new file mode 100644
index 000000000000..1e9c49c857c1
--- /dev/null
+++ b/drivers/mfd/rohm-bd96801.c

..

+
+enum {
+ WDG_CELL = 0,
+ REGULATOR_CELL,
+};

Dead code?

Yep. A leftover from the version with the ERRB stuff. Thanks for pointing it out!

..

+
+static int __init bd96801_i2c_init(void)
+{
+ return i2c_add_driver(&bd96801_i2c_driver);
+}
+
+/* Initialise early so consumer devices can complete system boot? */

Why the question mark? What are you unsure about?

Why doesn't -EPROBE_DEFER work for this?

I am unsure of what kind of dependencies the real setups using these PMICs have at boot time. Hence the (?). I've written drivers for a few PMICs, and I've sometimes just done the usual:

module_i2c_driver();

This, however, tends to get converted to boilerplate + subsys_initcall() by customers who actually start using the drivers - so I believe there is a problem in getting the consumers powered if the PMIC(s) are initialized too late. Anyways, it's probably nicer to try making it so it works better on different setups out of the box :)

I'd appreciate if someone wanted to shed some more light on this though!

Yours,
-- Matti

--
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~