Re: [PATCH RESEND v6 16/17] power: supply: bd71828-power: Support ROHM BD72720

[Matti Vaittinen <mazziesaccount@gmail.com>]

On 12/01/2026 02:51, Sebastian Reichel wrote:
> Hi,
> 
> On Mon, Dec 15, 2025 at 03:21:19PM +0200, Matti Vaittinen wrote:
>> From: Matti Vaittinen <mazziesaccount@gmail.com>
>>
>> The ROHM BD72720 is a power management IC with a charger and coulomb
>> counter block which is closely related to the charger / coulomb counter
>> found from the BD71815, BD71828, BD71879 which are all supported by the
>> bd71828-power driver. Due to the similarities it makes sense to support
>> also the BD72720 with the same driver.
>>
>> Add basic support for the charger logic on ROHM BD72720.
>>
>> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
>>
>> ---
>> Revision history:
>>   v2 => :
>>   - No changes
>>
>>   RFCv1 => v2:
>>   - Support using 9-bit register addresses (offset of 0x100) with the
>>     BD72720
>>   - Simplify probe and IC data as we don't need two regmaps
>>   - Drop two BD72720 specific functions as we no longer need different
>>     regmap for it.
>>
>> Note: This patch depends on the series: "power: supply: add charger for
>> BD71828" by Andreas:
>> https://lore.kernel.org/all/20250918-bd71828-charger-v5-0-851164839c28@kemnade.info/
> 
> That should be in v6.19?

Ah, right. As Andreas also stated, yes. This is no longer relevant. I 
simply forgot to clean-up the note from this patch.

>> @@ -958,21 +1043,27 @@ static int bd71828_power_probe(struct platform_device *pdev)
>>   	struct power_supply_config ac_cfg = {};
>>   	struct power_supply_config bat_cfg = {};
>>   	int ret;
>> -	struct regmap *regmap;
>> -
>> -	regmap = dev_get_regmap(pdev->dev.parent, NULL);
>> -	if (!regmap) {
>> -		dev_err(&pdev->dev, "No parent regmap\n");
>> -		return -EINVAL;
>> -	}
>>   
>>   	pwr = devm_kzalloc(&pdev->dev, sizeof(*pwr), GFP_KERNEL);
>>   	if (!pwr)
>>   		return -ENOMEM;
>>   
>> -	pwr->regmap = regmap;
>> -	pwr->dev = &pdev->dev;
>> +	/*
>> +	 * The BD72720 MFD device registers two regmaps. Power-supply driver
>> +	 * uses the "wrap-map", which provides access to both of the I2C slave
>> +	 * addresses used by the BD72720
>> +	 */
>>   	pwr->chip_type = platform_get_device_id(pdev)->driver_data;
>> +	if (pwr->chip_type != ROHM_CHIP_TYPE_BD72720)
>> +		pwr->regmap = dev_get_regmap(pdev->dev.parent, NULL);
>> +	else
>> +		pwr->regmap = dev_get_regmap(pdev->dev.parent, "wrap-map");
>> +	if (!pwr->regmap) {
>> +		dev_err(&pdev->dev, "No parent regmap\n");
>> +		return -EINVAL;
>> +	}
> 
> return dev_err_probe(&pdev->dev, -EINVAL, "No parent regmap\n");
> 
> Otherwise LGTM.

Thanks Sebastian! I appreciate the review!

This driver uses dev_err_probe() only in cases where the error to be 
returned is not hard-coded. The design dates back to when I was first 
introduced to the dev_err_probe() - and using pattern like:

ret = -EINVAL;
if (ret == -EPROBE_DEFER)
  ...

(which results from calling the dev_err_probe with hard-coded error) 
felt very repulsive to me.

I have since quit resisting the 'use dev_err_probe() for all probe error 
paths' -policy (since Resistance is futile - and because there are other 
benefits besides the deferred probe handling) - but I suppose we should 
clean-up also the other similar cases in this driver (I see at least one 
other occurrence in the chip_type-check below). Is it Ok to merge this 
as is, and do a follow-up patch to clean-up all the occurrences? If yes, 
then this might go "as-is" via MFD, together with the other stuff, right?

I can also re-spin this with the print fixed and:
  - add new patch to fix the other occurrence.
  - meld the fix for existing print in this patch.

Just please let me know your preference.



>>   	switch (pwr->chip_type) {
>>   	case ROHM_CHIP_TYPE_BD71828:
>> @@ -985,6 +1076,12 @@ static int bd71828_power_probe(struct platform_device *pdev)
>>   		pwr->get_temp = bd71815_get_temp;
>>   		pwr->regs = &pwr_regs_bd71815;
>>   		break;
>> +	case ROHM_CHIP_TYPE_BD72720:
>> +		pwr->bat_inserted = bd71828_bat_inserted;
>> +		pwr->regs = &pwr_regs_bd72720;
>> +		pwr->get_temp = bd71828_get_temp;
>> +		dev_dbg(pwr->dev, "Found ROHM BD72720\n");
>> +		break;
>>   	default:
>>   		dev_err(pwr->dev, "Unknown PMIC\n");
>>   		return -EINVAL;

The other occurrence --^

>> @@ -1030,6 +1127,7 @@ static int bd71828_power_probe(struct platform_device *pdev)
>>   static const struct platform_device_id bd71828_charger_id[] = {
>>   	{ "bd71815-power", ROHM_CHIP_TYPE_BD71815 },
>>   	{ "bd71828-power", ROHM_CHIP_TYPE_BD71828 },
>> +	{ "bd72720-power", ROHM_CHIP_TYPE_BD72720 },
>>   	{ },
>>   };
>>   MODULE_DEVICE_TABLE(platform, bd71828_charger_id);
>> -- 
>> 2.52.0

Yours,
	-- Matti


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

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