| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <33787eea-f0e0-4041-b9e6-102209af5138@gmail.com>
Date: Mon, 12 Jan 2026 08:44:45 +0200
From: Matti Vaittinen <mazziesaccount@...il.com>
To: Sebastian Reichel <sebastian.reichel@...labora.com>
Cc: Matti Vaittinen <matti.vaittinen@...rohmeurope.com>,
Lee Jones <lee@...nel.org>, Pavel Machek <pavel@...nel.org>,
Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
Conor Dooley <conor+dt@...nel.org>, Liam Girdwood <lgirdwood@...il.com>,
Mark Brown <broonie@...nel.org>, Michael Turquette
<mturquette@...libre.com>, Stephen Boyd <sboyd@...nel.org>,
Linus Walleij <linusw@...nel.org>, Bartosz Golaszewski <brgl@...nel.org>,
Alexandre Belloni <alexandre.belloni@...tlin.com>,
linux-leds@...r.kernel.org, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-pm@...r.kernel.org,
linux-clk@...r.kernel.org, linux-gpio@...r.kernel.org,
linux-rtc@...r.kernel.org, Andreas Kemnade <andreas@...nade.info>
Subject: Re: [PATCH RESEND v6 16/17] power: supply: bd71828-power: Support
ROHM BD72720
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@...il.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@...il.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! ~~
Powered by blists - more mailing lists