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

Matti Vaittinen
2026-01-12T06:44:50Z

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