ROHM bd71828 power-supply fixes

PATCH Applied Matti Vaittinen 6 participants 21 messages Aug 10 – Aug 12

Subsystems (3)

Power supply ROHM BD99954 charger IC ROHM BD71828 charger

lore

Share & tools

lore

Matti Vaittinen

author

2026-08-10 10:08 UTC #0 lore raw

cover ROHM bd71828 power-supply fixes

Fix a few issues from PSU drivers for ROHM components.

I hired couple of gnomes to work for me. :) (Ran AI reviews). Unsurprizingly some bugs were spotted. Time to try fix mess (mostly) I have (originally) authored.

Bugs were found by AI review but fixes are made by hand. Hence no AI-tooling tags are added. Please, let me know if one is needed.

---

NOTE: Only _very_ shallow testing is done. Some of the fixes are not tested in the hardware at all. All reviewing and testing is appreciated as usual.

Matti Vaittinen (5):
power: supply: bd71815: Fix temperature reading
power: supply: bd71828: Fix current direction
power: supply: bd71828: Drop duplicate power-supply property
power: supply: bd71828: Do not hide errors
power: supply: bd99954: Drop bad register fields

 drivers/power/supply/bd71828-power.c   | 20 ++++++++++++--------
 drivers/power/supply/bd99954-charger.h | 23 +----------------------
 2 files changed, 13 insertions(+), 30 deletions(-)

base-commit: dc59e4fea9d83f03bad6bddf3fa2e52491777482

Signature · 1 line

2.55.0

Matti Vaittinen

author

2026-08-10 10:09 UTC #1 lore raw

PATCH 1/5 power: supply: bd71815: Fix temperature reading

From: Matti Vaittinen <[email protected]>

The temperature read from the hardware is never returned to caller. Furthermore, the check for temperature validity is wrong. This yields garbage value to be returned to caller, and also detection of bad values read from hardware to fail.

Signed-off-by Matti Vaittinen <[email protected]>

Fixes 5bff79dad20a ("power: supply: Add bd718(15/28/78) charger driver")

---
 drivers/power/supply/bd71828-power.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

drivers/power/supply/bd71828-power.c +2 -2

index b671563ead79..e1ae5fe52e1d 100644
@@ -225,13 +225,13 @@ static int bd71815_get_temp(struct bd71828_power *pwr, int *temp)
 	if (ret)
 		return ret;
 
-	t = 200 - t;
-
 	if (t > 200) {
 		dev_err(pwr->dev, "Failed to read battery temperature\n");
 		return -ENODATA;
 	}
 
+	*temp = 200 - t;
+
 	return 0;
 }

Signature · 1 line

2.55.0

Matti Vaittinen

author

2026-08-10 10:09 UTC #2 lore raw

PATCH 2/5 power: supply: bd71828: Fix current direction

In reply to Matti Vaittinen

From: Matti Vaittinen <[email protected]>

Both the current and the average current registers contain same 'direction' -bit to denote the direction of the current. The code reading these registers incorrectly caches the direction read from the first register, and uses it also for the second.

Fix this by initializing the direction bit for both register reads.

Signed-off-by Matti Vaittinen <[email protected]>

Fixes 5bff79dad20a ("power: supply: Add bd718(15/28/78) charger driver")

---
 drivers/power/supply/bd71828-power.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

drivers/power/supply/bd71828-power.c +3 -2

index e1ae5fe52e1d..9927a7e8934a 100644
@@ -192,12 +192,13 @@ static int bd71828_get_current_ds_adc(struct bd71828_power *pwr, int *curr, int
 {
 	__be16 tmp_curr;
 	char *tmp = (char *)&tmp_curr;
-	int dir = 1;
 	int regs[] = { pwr->regs->ibat, pwr->regs->ibat_avg };
 	int *vals[] = { curr, curr_avg };
 	int ret, i;
 
-	for (dir = 1, i = 0; i < ARRAY_SIZE(regs); i++) {
+	for (i = 0; i < ARRAY_SIZE(regs); i++) {
+		int dir = 1;
+
 		ret = regmap_bulk_read(pwr->regmap, regs[i], &tmp_curr,
 				       sizeof(tmp_curr));
 		if (ret)

Signature · 1 line

2.55.0

Andy Shevchenko

2026-08-10 17:18 UTC #3 lore raw

Re: [PATCH 2/5] power: supply: bd71828: Fix current direction

Quoted 6 lines · Matti Vaittinen

Both the current and the average current registers contain same 'direction' -bit to denote the direction of the current. The code reading these registers incorrectly caches the direction read from the first register, and uses it also for the second.
Fix this by initializing the direction bit for both register reads.

...

drivers/power/supply/bd71828-power.c static int bd71828_get_current_ds_adc(struct bd71828_power *pwr, int *curr, int · line 199

Quoted 12 lines

static int bd71828_get_current_ds_adc(struct bd71828_power *pwr, int *curr, int

{

drivers/power/supply/bd71828-power.c +1 -2

 	__be16 tmp_curr;
 	char *tmp = (char *)&tmp_curr;
-	int dir = 1;
 	int regs[] = { pwr->regs->ibat, pwr->regs->ibat_avg };
 	int *vals[] = { curr, curr_avg };
 	int ret, i;
 
-	for (dir = 1, i = 0; i < ARRAY_SIZE(regs); i++) {
+	for (i = 0; i < ARRAY_SIZE(regs); i++) {

While at it

	for (unsigned int i = 0; i < ARRAY_SIZE(regs); i++) {

drivers/power/supply/bd71828-power.c static int bd71828_get_current_ds_adc(struct bd71828_power *pwr, int *curr, int · line 200

Quoted 1 line

drivers/power/supply/bd71828-power.c +1 -0
+		int dir = 1;

...and drop it from the top declaration.

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Matti Vaittinen

maintainer

2026-08-11 08:47 UTC #4 lore raw

Re: [PATCH 2/5] power: supply: bd71828: Fix current direction

Thanks for the review(s) Andy.

Quoted 27 lines · Andy Shevchenko

Quoted 6 lines · Matti Vaittinen
Both the current and the average current registers contain same 'direction' -bit to denote the direction of the current. The code reading these registers incorrectly caches the direction read from the first register, and uses it also for the second.
Fix this by initializing the direction bit for both register reads.
...
drivers/power/supply/bd71828-power.c static int bd71828_get_current_ds_adc(struct bd71828_power *pwr, int *curr, int · line 199
Quoted 12 lines
static int bd71828_get_current_ds_adc(struct bd71828_power *pwr, int *curr, int
  {
  	__be16 tmp_curr;
  	char *tmp = (char *)&tmp_curr;
drivers/power/supply/bd71828-power.c +1 -2
-	int dir = 1;
  	int regs[] = { pwr->regs->ibat, pwr->regs->ibat_avg };
  	int *vals[] = { curr, curr_avg };
  	int ret, i;
 
-	for (dir = 1, i = 0; i < ARRAY_SIZE(regs); i++) {
+	for (i = 0; i < ARRAY_SIZE(regs); i++) {
While at it

Quoted 6 lines

	for (unsigned int i = 0; i < ARRAY_SIZE(regs); i++) {
drivers/power/supply/bd71828-power.c static int bd71828_get_current_ds_adc(struct bd71828_power *pwr, int *curr, int · line 200
Quoted 1 line
drivers/power/supply/bd71828-power.c +1 -0
+		int dir = 1;
...and drop it from the top declaration.

Nope. I'm not a fan of introducing variables inside loop conditions. It's too easy for me to miss them. And, even if I liked the idea (and I don't), it is not a change related to the actual fix - and making it for this loop only would make the style inconsistent in this file.

Yours,
-- Matti

Signature · 5 lines

Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

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

Andy Shevchenko

2026-08-11 10:04 UTC #5 lore raw

Re: [PATCH 2/5] power: supply: bd71828: Fix current direction

Quoted 2 lines · Matti Vaittinen

Quoted 1 line · Andy Shevchenko
On Mon, Aug 10, 2026 at 01:09:07PM +0300, Matti Vaittinen wrote:

...

Quoted 23 lines

Quoted 16 lines
drivers/power/supply/bd71828-power.c static int bd71828_get_current_ds_adc(struct bd71828_power *pwr, int *curr, int · line 199
Quoted 12 lines
static int bd71828_get_current_ds_adc(struct bd71828_power *pwr, int *curr, int
  {
  	__be16 tmp_curr;
  	char *tmp = (char *)&tmp_curr;
drivers/power/supply/bd71828-power.c +1 -2
-	int dir = 1;
  	int regs[] = { pwr->regs->ibat, pwr->regs->ibat_avg };
  	int *vals[] = { curr, curr_avg };
  	int ret, i;
 
-	for (dir = 1, i = 0; i < ARRAY_SIZE(regs); i++) {
+	for (i = 0; i < ARRAY_SIZE(regs); i++) {
While at it
Quoted 6 lines
	for (unsigned int i = 0; i < ARRAY_SIZE(regs); i++) {
drivers/power/supply/bd71828-power.c static int bd71828_get_current_ds_adc(struct bd71828_power *pwr, int *curr, int · line 200
Quoted 1 line
drivers/power/supply/bd71828-power.c +1 -0
+		int dir = 1;
...and drop it from the top declaration.

Quoted 4 lines

Nope. I'm not a fan of introducing variables inside loop conditions. It's too easy for me to miss them. And, even if I liked the idea (and I don't), it is not a change related to the actual fix - and making it for this loop only would make the style inconsistent in this file.

This is a preferred style [1]. So, I would still do it independently on this being a fix. This is practically a small change that's coupled with what you are doing. A separate patch would be an unneeded churn, indeed.

[1]: (sub-thread) https://lore.kernel.org/lkml/[email protected]/

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Andreas Kemnade

maintainer Tested-by on v1 2/5

2026-08-12 08:10 UTC #6 lore raw

Re: [PATCH 2/5] power: supply: bd71828: Fix current direction

In reply to Matti Vaittinen

Quoted 11 lines · On Mon, 10 Aug 2026 13:09:07 +0300 Matti Vaittinen <[email protected]>…

From: Matti Vaittinen <[email protected]>
Both the current and the average current registers contain same 'direction' -bit to denote the direction of the current. The code reading these registers incorrectly caches the direction read from the first register, and uses it also for the second.
Fix this by initializing the direction bit for both register reads.
Signed-off-by Matti Vaittinen <[email protected]>
Fixes 5bff79dad20a ("power: supply: Add bd718(15/28/78) charger driver")

Weird. I reported that issue after you have submitted an RFC. current_avg just changing direction after disconnecting power supply.

Then I had a fix to enable current_now property.
Somehow we thought that averaging current in the chip ist just weird.

But now it makes sense.

Tested-by Andreas Kemnade <[email protected]> # Kobo Clara 2e rev B

Quoted 14 lines

---
 drivers/power/supply/bd71828-power.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

drivers/power/supply/bd71828-power.c +0 -1

index e1ae5fe52e1d..9927a7e8934a 100644
@@ -192,12 +192,13 @@ static int bd71828_get_current_ds_adc(struct bd71828_power *pwr, int *curr, int
 {
 	__be16 tmp_curr;
 	char *tmp = (char *)&tmp_curr;
-	int dir = 1;
 	int regs[] = { pwr->regs->ibat, pwr->regs->ibat_avg };

drivers/power/supply/bd71828-power.c static int bd71828_get_current_ds_adc(struct bd71828_power *pwr, int *curr, int · line 204

Quoted 10 lines

drivers/power/supply/bd71828-power.c +3 -1

 	int *vals[] = { curr, curr_avg };
 	int ret, i;
 
-	for (dir = 1, i = 0; i < ARRAY_SIZE(regs); i++) {
+	for (i = 0; i < ARRAY_SIZE(regs); i++) {
+		int dir = 1;
+
 		ret = regmap_bulk_read(pwr->regmap, regs[i], &tmp_curr,
 				       sizeof(tmp_curr));
 		if (ret)

That code is complicated. What about a generic 16bit non_2complemented sign
to 2-complemented sign function/macro. Such would have prevented the error
in the first places. So &vals[i] = signbit_to_2complement(be16_to_cpu(tmp_curr), 15) * pwr->curr_factor

Regards,
Andreas

Andy Shevchenko

2026-08-12 08:17 UTC #7 lore raw

Re: [PATCH 2/5] power: supply: bd71828: Fix current direction

Quoted 2 lines · Andreas Kemnade

On Mon, 10 Aug 2026 13:09:07 +0300
Matti Vaittinen <[email protected]> wrote:

...

Quoted 3 lines

That code is complicated. What about a generic 16bit non_2complemented sign
to 2-complemented sign function/macro. Such would have prevented the error
in the first places. So &vals[i] = signbit_to_2complement(be16_to_cpu(tmp_curr), 15) * pwr->curr_factor

Do you mean existing sign_extend32() from bitops.h?

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Andreas Kemnade

maintainer

2026-08-12 08:25 UTC #8 lore raw

Re: [PATCH 2/5] power: supply: bd71828: Fix current direction

Quoted 4 lines · On Wed, 12 Aug 2026 11:17:36 +0300 Andy Shevchenko…

Quoted 2 lines · Andreas Kemnade
On Mon, 10 Aug 2026 13:09:07 +0300
Matti Vaittinen <[email protected]> wrote:

Quoted 8 lines

...
Quoted 3 lines
That code is complicated. What about a generic 16bit non_2complemented sign
to 2-complemented sign function/macro. Such would have prevented the error
in the first places. So &vals[i] = signbit_to_2complement(be16_to_cpu(tmp_curr), 15) * pwr->curr_factor
Do you mean existing sign_extend32() from bitops.h?

I don't think it does what is needed. Here we have

0000 0000 0000 0001 -> 1
Usually we have

1111 1111 1111 1111 -> -1

but here

1000 0000 0000 0001 -> -1

If I do understand things correctly. I not find something useful in bitops.h.

Regards,
Andreas

Andy Shevchenko

2026-08-12 09:30 UTC #9 lore raw

Re: [PATCH 2/5] power: supply: bd71828: Fix current direction

Quoted 5 lines · Andreas Kemnade

Quoted 3 lines · On Wed, 12 Aug 2026 11:17:36 +0300 Andy Shevchenko…
Quoted 2 lines · Andreas Kemnade
On Mon, 10 Aug 2026 13:09:07 +0300
Matti Vaittinen <[email protected]> wrote:

...

Quoted 9 lines

Quoted 6 lines
Quoted 3 lines
That code is complicated. What about a generic 16bit non_2complemented sign
to 2-complemented sign function/macro. Such would have prevented the error
in the first places. So &vals[i] = signbit_to_2complement(be16_to_cpu(tmp_curr), 15) * pwr->curr_factor
Do you mean existing sign_extend32() from bitops.h?
I don't think it does what is needed. Here we have

Quoted 8 lines

0000 0000 0000 0001 -> 1
Usually we have
1111 1111 1111 1111 -> -1
but here
1000 0000 0000 0001 -> -1

I see, wouldn't this be then as simple as

	sign = $REGVAL & BIT(15 /* sign bit */);
	if (sign)
		$REGVAL = sign - $REGVAL;

Quoted 2 lines

If I do understand things correctly. I not find something useful in bitops.h.

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Matti Vaittinen

maintainer

2026-08-12 09:37 UTC #10 lore raw

Re: [PATCH 2/5] power: supply: bd71828: Fix current direction

Quoted 25 lines · Andy Shevchenko

Quoted 5 lines · Andreas Kemnade
Quoted 3 lines · On Wed, 12 Aug 2026 11:17:36 +0300 Andy Shevchenko…
Quoted 2 lines · Andreas Kemnade
On Mon, 10 Aug 2026 13:09:07 +0300
Matti Vaittinen <[email protected]> wrote:
...
Quoted 16 lines
Quoted 6 lines
Quoted 3 lines
That code is complicated. What about a generic 16bit non_2complemented sign
to 2-complemented sign function/macro. Such would have prevented the error
in the first places. So &vals[i] = signbit_to_2complement(be16_to_cpu(tmp_curr), 15) * pwr->curr_factor
Do you mean existing sign_extend32() from bitops.h?
I don't think it does what is needed. Here we have
0000 0000 0000 0001 -> 1
Usually we have
1111 1111 1111 1111 -> -1
but here

Quoted 7 lines

Quoted 1 line
1000 0000 0000 0001 -> -1
I see, wouldn't this be then as simple as
	sign = $REGVAL & BIT(15 /* sign bit */);
	if (sign)
		$REGVAL = sign - $REGVAL;

I find this more confusing than the current idea, which effectively is:

if (sign)
val = val_without_sign * -1;
else
val = val_without_sign (* 1);

(I can't straight away say if "$REGVAL = sign - $REGVAL;" yields same result.)

Yours,
-- Matti

Signature · 5 lines

Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

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

Andy Shevchenko

2026-08-12 09:55 UTC #11 lore raw

Re: [PATCH 2/5] power: supply: bd71828: Fix current direction

Quoted 7 lines · Matti Vaittinen

Quoted 6 lines · Andy Shevchenko
Quoted 5 lines · Andreas Kemnade
Quoted 3 lines · On Wed, 12 Aug 2026 11:17:36 +0300 Andy Shevchenko…
Quoted 2 lines · Andreas Kemnade
On Mon, 10 Aug 2026 13:09:07 +0300
Matti Vaittinen <[email protected]> wrote:

...

Quoted 24 lines

Quoted 16 lines
Quoted 16 lines
Quoted 6 lines
Quoted 3 lines
That code is complicated. What about a generic 16bit non_2complemented sign
to 2-complemented sign function/macro. Such would have prevented the error
in the first places. So &vals[i] = signbit_to_2complement(be16_to_cpu(tmp_curr), 15) * pwr->curr_factor
Do you mean existing sign_extend32() from bitops.h?
I don't think it does what is needed. Here we have
0000 0000 0000 0001 -> 1
Usually we have
1111 1111 1111 1111 -> -1
but here
Quoted 7 lines
Quoted 1 line
1000 0000 0000 0001 -> -1
I see, wouldn't this be then as simple as
	sign = $REGVAL & BIT(15 /* sign bit */);
	if (sign)
		$REGVAL = sign - $REGVAL;

Quoted 6 lines

I find this more confusing than the current idea, which effectively is:
if (sign)
val = val_without_sign * -1;
else
val = val_without_sign (* 1);

If you have other use for val_wo_sign, then you can even drop multiplication.

	val = sign ? -val_wo_sing: val_wo_sign;

More comprehensive view

	sign = $REGVAL & BIT(15 /* sign bit */);
	$REGVAL &= ~sign;
	val = sign ? -$REGVAL : $REGVAL;

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Matti Vaittinen

maintainer

2026-08-12 09:40 UTC #12 lore raw

Re: [PATCH 2/5] power: supply: bd71828: Fix current direction

In reply to Andreas Kemnade

Quoted 21 lines · Andreas Kemnade

Quoted 4 lines · On Wed, 12 Aug 2026 11:17:36 +0300 Andy Shevchenko…
Quoted 2 lines · Andreas Kemnade
On Mon, 10 Aug 2026 13:09:07 +0300
Matti Vaittinen <[email protected]> wrote:
Quoted 8 lines
...
Quoted 3 lines
That code is complicated. What about a generic 16bit non_2complemented sign
to 2-complemented sign function/macro. Such would have prevented the error
in the first places. So &vals[i] = signbit_to_2complement(be16_to_cpu(tmp_curr), 15) * pwr->curr_factor
Do you mean existing sign_extend32() from bitops.h?
I don't think it does what is needed. Here we have
0000 0000 0000 0001 -> 1
Usually we have

Quoted 9 lines

1111 1111 1111 1111 -> -1
but here
1000 0000 0000 0001 -> -1
If I do understand things correctly. I not find something useful in bitops.h.

Thanks for the review and testing Andreas! We share the same (mis?)understanding. ;)

I am not sure this is so widely used it'd warrant a generic macro. I am afraid that if someone encountered similar design, he would not expect a generic macro to exist. My gut feeling is that this is too marginal to be widely useful. I am not against it though, if someone wants to implement it.

What comes to complexity - I agree. The code is messy, but mostly because of "clever hacks" done so values are in arrays indexed by loop.

I, however, would like to see a minimal backportable fix, and potentially, larger clean-up which can then go in as a feature, without any rush.

Yours,
-- Matti

Signature · 5 lines

Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

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

Andreas Kemnade

maintainer

2026-08-12 11:32 UTC #13 lore raw

Re: [PATCH 2/5] power: supply: bd71828: Fix current direction

Quoted 32 lines · On Wed, 12 Aug 2026 12:40:47 +0300 Matti Vaittinen <[email protected]>…

Quoted 21 lines · Andreas Kemnade
Quoted 4 lines · On Wed, 12 Aug 2026 11:17:36 +0300 Andy Shevchenko…
Quoted 2 lines · Andreas Kemnade
On Mon, 10 Aug 2026 13:09:07 +0300
Matti Vaittinen <[email protected]> wrote:
Quoted 8 lines
...
Quoted 3 lines
That code is complicated. What about a generic 16bit non_2complemented sign
to 2-complemented sign function/macro. Such would have prevented the error
in the first places. So &vals[i] = signbit_to_2complement(be16_to_cpu(tmp_curr), 15) * pwr->curr_factor
Do you mean existing sign_extend32() from bitops.h?
I don't think it does what is needed. Here we have
0000 0000 0000 0001 -> 1
Usually we have
Quoted 9 lines
1111 1111 1111 1111 -> -1
but here
1000 0000 0000 0001 -> -1
If I do understand things correctly. I not find something useful in bitops.h.

Quoted 8 lines

Thanks for the review and testing Andreas! We share the same (mis?)understanding. ;)
I am not sure this is so widely used it'd warrant a generic macro. I am afraid that if someone encountered similar design, he would not expect a generic macro to exist. My gut feeling is that this is too marginal to be widely useful. I am not against it though, if someone wants to implement it.

here is the special case that the sign bit is detached from the rest Bit6/14 is unused. The other problem would be the name of the macro. Probably nothing more meaningful than the code.

Quoted 4 lines

What comes to complexity - I agree. The code is messy, but mostly because of "clever hacks" done so values are in arrays indexed by loop.

Looking at it: The relationship between tmp and tmp_curr. IMHO reading into an u8 array and doing get_unaligned_be16() would be the best improvment of readablity.

Quoted 4 lines

I, however, would like to see a minimal backportable fix, and potentially, larger clean-up which can then go in as a feature, without any rush.

Yes, that is a good point.

Regards,
Andreas

Matti Vaittinen

author

2026-08-10 10:09 UTC #14 lore raw

PATCH 3/5 power: supply: bd71828: Drop duplicate power-supply property

In reply to Matti Vaittinen

From: Matti Vaittinen <[email protected]>

The POWER_SUPPLY_PROP_HEALTH is added into property-array twice.

Drop the duplicate property.

Signed-off-by Matti Vaittinen <[email protected]>

Fixes 5bff79dad20a ("power: supply: Add bd718(15/28/78) charger driver")

---
 drivers/power/supply/bd71828-power.c | 1 -
 1 file changed, 1 deletion(-)

drivers/power/supply/bd71828-power.c +0 -1

index 9927a7e8934a..a564ad7210be 100644
@@ -640,7 +640,6 @@ static const enum power_supply_property bd71828_charger_props[] = {
 
 static const enum power_supply_property bd71828_battery_props[] = {
 	POWER_SUPPLY_PROP_STATUS,
-	POWER_SUPPLY_PROP_HEALTH,
 	POWER_SUPPLY_PROP_VOLTAGE_NOW,
 	POWER_SUPPLY_PROP_HEALTH,
 	POWER_SUPPLY_PROP_PRESENT,

Signature · 1 line

2.55.0

Andy Shevchenko

2026-08-10 18:01 UTC #15 lore raw

Re: [PATCH 3/5] power: supply: bd71828: Drop duplicate power-supply property

Quoted 3 lines · Matti Vaittinen

The POWER_SUPPLY_PROP_HEALTH is added into property-array twice.
Drop the duplicate property.

From this commit message is not obvious why is this a fix. Perhaps elaborate that the index is somehow related (or what is the reason behind)?

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Matti Vaittinen

author

2026-08-10 10:09 UTC #16 lore raw

PATCH 4/5 power: supply: bd71828: Do not hide errors

In reply to Matti Vaittinen

From: Matti Vaittinen <[email protected]>

The bd71828 reports success / continues operation even when some regmap operations fail. This can lead incorrect data to be reported.

Return appropriate errors when operations fail.

Signed-off-by Matti Vaittinen <[email protected]>

Fixes 5bff79dad20a ("power: supply: Add bd718(15/28/78) charger driver")

---
 drivers/power/supply/bd71828-power.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

drivers/power/supply/bd71828-power.c +7 -3

index a564ad7210be..589e12225cb2 100644
@@ -456,8 +456,10 @@ static int bd71828_charger_get_property(struct power_supply *psy,
 	switch (psp) {
 	case POWER_SUPPLY_PROP_ONLINE:
 		ret = get_chg_online(pwr, &online);
-		if (!ret)
-			val->intval = online;
+		if (ret)
+			return ret;
+
+		val->intval = online;
 		break;
 	case POWER_SUPPLY_PROP_VOLTAGE_NOW:
 		ret = bd7182x_read16_himask(pwr, pwr->regs->vdcin,

@@ -1218,7 +1220,9 @@ static int bd71828_power_probe(struct platform_device *pdev)
 		return dev_err_probe(&pdev->dev, ret, "sense resistor missing\n");
 
 	dev_set_drvdata(&pdev->dev, pwr);
-	bd71828_init_hardware(pwr);
+	ret = bd71828_init_hardware(pwr);
+	if (ret)
+		return dev_err_probe(&pdev->dev, ret, "hardware init failed\n");
 
 	bat_cfg.drv_data	= pwr;
 	bat_cfg.fwnode		= dev_fwnode(&pdev->dev);

Signature · 1 line

2.55.0

Matti Vaittinen

author

2026-08-10 10:10 UTC #17 lore raw

PATCH 5/5 power: supply: bd99954: Drop bad register fields

In reply to Matti Vaittinen

From: Matti Vaittinen <[email protected]>

The bd99954 driver creates regmap fields for all the registers. All the fields for VBUS_UCD_FCTRL_SET (extended command 0x33) are accidentally created for VCC_UCD_FCTRL_SET (extended command 0x2b), causing all reads/writes for the fields to access wrong register. Luckily the fields for VBUS_UCD_FCTRL_SET are unused.

Taking a look at the data-sheet:
https://fscdn.rohm.com/en/products/databook/datasheet/ic/power/battery_management/bd99954xxx-e.pdf

all the fields in the VBUS_UCD_FCTRL_SET are marked as:
"In normal operation, please don’t set these registers" with not much of additional documentation. As the fields are wrong and because there seems to be no users of the fields - it is better to just drop them. This will also decrease the memory footprint.

Drop incorrect VBUS_UCD_FCTRL_SET fields.

Signed-off-by Matti Vaittinen <[email protected]>

Fixes 0902f8366491 ("power: supply: Support ROHM bd99954 charger")

---
 drivers/power/supply/bd99954-charger.h | 23 +----------------------
 1 file changed, 1 insertion(+), 22 deletions(-)

drivers/power/supply/bd99954-charger.h +1 ~~-22~~

index f58897925383..d053ac901274 100644
@@ -279,17 +279,7 @@ enum bd9995x_fields {
 	F_VBUS_EXTID,
 	F_VBUS_IDRDET,
 	F_VBUS_INDO,
-	F_VBUS_UCDSWEN,
-	F_VBUS_RREF_EN,
-	F_VBUS_DPPU_EN,
-	F_VBUS_DPREF_EN,
-	F_VBUS_DMREF_EN,
-	F_VBUS_DPDET_EN,
-	F_VBUS_DMDET_EN,
-	F_VBUS_DPSINK_EN,
-	F_VBUS_DMSINK_EN,
-	F_VBUS_DP_BUFF_EN,
-	F_VBUS_DM_BUFF_EN,
+
 	F_VBUS_EXTCLKENBL,
 	F_VBUS_PLSTESTEN,
 	F_VBUS_UCDSWEN_TSTENB,

@@ -626,17 +616,6 @@ static const struct reg_field bd9995x_reg_fields[] = {
 	    [F_VBUS_EXTID] = REG_FIELD(VBUS_IDD_STATUS, 5, 5),
 	    [F_VBUS_IDRDET] = REG_FIELD(VBUS_IDD_STATUS, 4, 4),
 	    [F_VBUS_INDO] = REG_FIELD(VBUS_IDD_STATUS, 0, 3),
-	    [F_VBUS_UCDSWEN] = REG_FIELD(VCC_UCD_FCTRL_SET, 10, 10),
-	    [F_VBUS_RREF_EN] = REG_FIELD(VCC_UCD_FCTRL_SET, 9, 9),
-	    [F_VBUS_DPPU_EN] = REG_FIELD(VCC_UCD_FCTRL_SET, 8, 8),
-	    [F_VBUS_DPREF_EN] = REG_FIELD(VCC_UCD_FCTRL_SET, 7, 7),
-	    [F_VBUS_DMREF_EN] = REG_FIELD(VCC_UCD_FCTRL_SET, 6, 6),
-	    [F_VBUS_DPDET_EN] = REG_FIELD(VCC_UCD_FCTRL_SET, 5, 5),
-	    [F_VBUS_DMDET_EN] = REG_FIELD(VCC_UCD_FCTRL_SET, 4, 4),
-	    [F_VBUS_DPSINK_EN] = REG_FIELD(VCC_UCD_FCTRL_SET, 3, 3),
-	    [F_VBUS_DMSINK_EN] = REG_FIELD(VCC_UCD_FCTRL_SET, 2, 2),
-	    [F_VBUS_DP_BUFF_EN] = REG_FIELD(VCC_UCD_FCTRL_SET, 1, 1),
-	    [F_VBUS_DM_BUFF_EN] = REG_FIELD(VCC_UCD_FCTRL_SET, 0, 0),
 
 	    [F_VBUS_EXTCLKENBL] = REG_FIELD(VBUS_UCD_FCTRL_EN, 15, 15),
 	    [F_VBUS_PLSTESTEN] = REG_FIELD(VBUS_UCD_FCTRL_EN, 14, 14),

Signature · 1 line

2.55.0

Andy Shevchenko

2026-08-10 18:03 UTC #18 lore raw

Re: [PATCH 5/5] power: supply: bd99954: Drop bad register fields

Quoted 8 lines · Matti Vaittinen

The bd99954 driver creates regmap fields for all the registers. All the fields for VBUS_UCD_FCTRL_SET (extended command 0x33) are accidentally created for VCC_UCD_FCTRL_SET (extended command 0x2b), causing all reads/writes for the fields to access wrong register. Luckily the fields for VBUS_UCD_FCTRL_SET are unused.
Taking a look at the data-sheet:
https://fscdn.rohm.com/en/products/databook/datasheet/ic/power/battery_management/bd99954xxx-e.pdf

Make it a Link or Datasheet tag.

Quoted 7 lines

all the fields in the VBUS_UCD_FCTRL_SET are marked as:
"In normal operation, please don’t set these registers" with not much of additional documentation. As the fields are wrong and because there seems to be no users of the fields - it is better to just drop them. This will also decrease the memory footprint.
Drop incorrect VBUS_UCD_FCTRL_SET fields.

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Kalle Niemi

Tested-by on the v1 series

2026-08-12 06:59 UTC #19 lore raw

In reply to Matti Vaittinen

Quoted 6 lines · Matti Vaittinen

Fix a few issues from PSU drivers for ROHM components.
I hired couple of gnomes to work for me. :) (Ran AI reviews). Unsurprizingly some bugs were spotted. Time to try fix mess (mostly) I have (originally) authored.

Quoted 8 lines

Bugs were found by AI review but fixes are made by hand. Hence no AI-tooling tags are added. Please, let me know if one is needed.
---
NOTE: Only _very_ shallow testing is done. Some of the fixes are not tested in the hardware at all. All reviewing and testing is appreciated as usual.

Hello,

This series is tested with a setup that has BD71815 and BD71828 installed.

Tested-by Kalle Niemi <[email protected]>

Sebastian Reichel

Applied

2026-08-12 22:23 UTC #20 lore raw

In reply to Matti Vaittinen

Quoted 10 lines · Matti Vaittinen

Fix a few issues from PSU drivers for ROHM components.
I hired couple of gnomes to work for me. :) (Ran AI reviews). Unsurprizingly some bugs were spotted. Time to try fix mess (mostly) I have (originally) authored.
Bugs were found by AI review but fixes are made by hand. Hence no AI-tooling tags are added. Please, let me know if one is needed.
[...]

Applied, thanks!

[1/5] power: supply: bd71815: Fix temperature reading

      commit: c15f0eabec5a051cb8190fa2bff3b211730f4acb
[2/5] power: supply: bd71828: Fix current direction
      commit: d012fab12fed2053e0dca4c8cd4e2b2053afabfb
[3/5] power: supply: bd71828: Drop duplicate power-supply property
      commit: 6605209ceb53eaa25582d1f4726217bd09eff392
[4/5] power: supply: bd71828: Do not hide errors
      commit: a3c841e420e124783a805919ab072551543e477b
[5/5] power: supply: bd99954: Drop bad register fields
      commit: 177299384a224377b4152242d875e752e5958c02

Best regards,

Signature · 1 line

Sebastian Reichel <[email protected]>