Received-SPF: pass (google.com: domain of linux-kernel+bounces-167406-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Message-ID: <6c461609-33e2-422e-a3a4-7d222ae3f7c4@gmail.com>
Date: Fri, 3 May 2024 11:43:24 +0300
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v1 3/6] mfd: support ROHM BD96801 PMIC core
Content-Language: en-US, en-GB
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
References: <cover.1714478142.git.mazziesaccount@gmail.com>
 <91eafc06728ebb6158d86b58ce987fc8f802a453.1714478142.git.mazziesaccount@gmail.com>
 <20240503074106.GG1227636@google.com>
From: Matti Vaittinen <mazziesaccount@gmail.com>
In-Reply-To: <20240503074106.GG1227636@google.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit

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! ~~