From: Matti Vaittinen <mazziesaccount@gmail.com>
To: Lee Jones <lee@kernel.org>
Cc: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>,
Rob Herring <robh@kernel.org>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
Conor Dooley <conor+dt@kernel.org>,
Liam Girdwood <lgirdwood@gmail.com>,
Mark Brown <broonie@kernel.org>,
Wim Van Sebroeck <wim@linux-watchdog.org>,
Guenter Roeck <linux@roeck-us.net>,
devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-watchdog@vger.kernel.org
Subject: Re: [PATCH v1 3/6] mfd: support ROHM BD96801 PMIC core
Date: Fri, 3 May 2024 11:43:24 +0300 [thread overview]
Message-ID: <6c461609-33e2-422e-a3a4-7d222ae3f7c4@gmail.com> (raw)
In-Reply-To: <20240503074106.GG1227636@google.com>
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@gmail.com>
>>
>> ---
>> 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! ~~
next prev parent reply other threads:[~2024-05-03 8:43 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-30 11:59 [PATCH v1 0/6] Support ROHM BD96801 scalable PMIC Matti Vaittinen
2024-04-30 11:59 ` [PATCH v1 1/6] dt-bindings: ROHM BD96801 PMIC regulators Matti Vaittinen
2024-05-02 16:20 ` Conor Dooley
2024-05-03 4:54 ` Matti Vaittinen
2024-05-03 7:25 ` Matti Vaittinen
2024-04-30 12:00 ` [PATCH v1 2/6] dt-bindings: mfd: bd96801 PMIC core Matti Vaittinen
2024-04-30 12:00 ` [PATCH v1 3/6] mfd: support ROHM BD96801 " Matti Vaittinen
2024-05-03 7:41 ` Lee Jones
2024-05-03 8:43 ` Matti Vaittinen [this message]
2024-04-30 12:01 ` [PATCH v1 4/6] regulator: bd96801: ROHM BD96801 PMIC regulators Matti Vaittinen
2024-04-30 12:01 ` [PATCH v1 5/6] watchdog: ROHM BD96801 PMIC WDG driver Matti Vaittinen
2024-04-30 16:24 ` Guenter Roeck
2024-05-01 10:16 ` Matti Vaittinen
2024-04-30 12:02 ` [PATCH v1 6/6] MAINTAINERS: Add ROHM BD96801 'scalable PMIC' entries Matti Vaittinen
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=6c461609-33e2-422e-a3a4-7d222ae3f7c4@gmail.com \
--to=mazziesaccount@gmail.com \
--cc=broonie@kernel.org \
--cc=conor+dt@kernel.org \
--cc=devicetree@vger.kernel.org \
--cc=krzysztof.kozlowski+dt@linaro.org \
--cc=lee@kernel.org \
--cc=lgirdwood@gmail.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-watchdog@vger.kernel.org \
--cc=linux@roeck-us.net \
--cc=matti.vaittinen@fi.rohmeurope.com \
--cc=robh@kernel.org \
--cc=wim@linux-watchdog.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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 read-only IMAP folder(s) and NNTP newsgroup(s).