[PATCH 0/5] ROHM bd71828 power-supply fixes

[PATCH 0/5] ROHM bd71828 power-supply fixes

[Matti Vaittinen]

[-- Attachment #1: Type: text/plain, Size: 1072 bytes --]

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
-- 
2.55.0


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Matti Vaittinen]

[-- Attachment #1: Type: text/plain, Size: 1062 bytes --]

From: Matti Vaittinen <mazziesaccount@gmail.com>

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 <mazziesaccount@gmail.com>
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(-)

diff --git a/drivers/power/supply/bd71828-power.c b/drivers/power/supply/bd71828-power.c
index b671563ead79..e1ae5fe52e1d 100644
--- a/drivers/power/supply/bd71828-power.c
+++ b/drivers/power/supply/bd71828-power.c
@@ -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;
 }
 
-- 
2.55.0


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Matti Vaittinen]

[-- Attachment #1: Type: text/plain, Size: 1365 bytes --]

From: Matti Vaittinen <mazziesaccount@gmail.com>

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 <mazziesaccount@gmail.com>
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(-)

diff --git a/drivers/power/supply/bd71828-power.c b/drivers/power/supply/bd71828-power.c
index e1ae5fe52e1d..9927a7e8934a 100644
--- a/drivers/power/supply/bd71828-power.c
+++ b/drivers/power/supply/bd71828-power.c
@@ -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)
-- 
2.55.0


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Andy Shevchenko]

On Mon, Aug 10, 2026 at 01:09:07PM +0300, Matti Vaittinen wrote:

> 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.

...

> 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++) {


While at it

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

> +		int dir = 1;


...and drop it from the top declaration.

-- 
With Best Regards,
Andy Shevchenko

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

[Matti Vaittinen]

Thanks for the review(s) Andy.

On 10/08/2026 20:18, Andy Shevchenko wrote:
> On Mon, Aug 10, 2026 at 01:09:07PM +0300, Matti Vaittinen wrote:
> 
>> 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.
> 
> ...
> 
>> 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++) {
> 
> 
> While at it
> 
> 	for (unsigned int i = 0; i < ARRAY_SIZE(regs); i++) {
> 
>> +		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

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

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

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

[Andy Shevchenko]

On Tue, Aug 11, 2026 at 11:46:59AM +0300, Matti Vaittinen wrote:
> On 10/08/2026 20:18, Andy Shevchenko wrote:
> > On Mon, Aug 10, 2026 at 01:09:07PM +0300, Matti Vaittinen wrote:

...

> > > 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++) {
> > 
> > 
> > While at it
> > 
> > 	for (unsigned int i = 0; i < ARRAY_SIZE(regs); i++) {
> > 
> > > +		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.

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/alCoTEDUY8AO9OzD@ashevche-desk.local/

-- 
With Best Regards,
Andy Shevchenko

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

[Andreas Kemnade]

On Mon, 10 Aug 2026 13:09:07 +0300
Matti Vaittinen <matti.vaittinen@linux.dev> wrote:

> From: Matti Vaittinen <mazziesaccount@gmail.com>
> 
> 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 <mazziesaccount@gmail.com>
> 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 <andreas@kemnade.info> # Kobo Clara 2e rev B

> ---
>  drivers/power/supply/bd71828-power.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/power/supply/bd71828-power.c b/drivers/power/supply/bd71828-power.c
> index e1ae5fe52e1d..9927a7e8934a 100644
> --- a/drivers/power/supply/bd71828-power.c
> +++ b/drivers/power/supply/bd71828-power.c
> @@ -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)

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

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

[Andy Shevchenko]

On Wed, Aug 12, 2026 at 10:10:50AM +0200, Andreas Kemnade wrote:
> On Mon, 10 Aug 2026 13:09:07 +0300
> Matti Vaittinen <matti.vaittinen@linux.dev> wrote:

...

> 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?

-- 
With Best Regards,
Andy Shevchenko

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

[Andreas Kemnade]

On Wed, 12 Aug 2026 11:17:36 +0300
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> On Wed, Aug 12, 2026 at 10:10:50AM +0200, Andreas Kemnade wrote:
> > On Mon, 10 Aug 2026 13:09:07 +0300
> > Matti Vaittinen <matti.vaittinen@linux.dev> wrote:  
> 
> ...
> 
> > 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

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

[Andy Shevchenko]

On Wed, Aug 12, 2026 at 10:25:36AM +0200, Andreas Kemnade wrote:
> On Wed, 12 Aug 2026 11:17:36 +0300
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> > On Wed, Aug 12, 2026 at 10:10:50AM +0200, Andreas Kemnade wrote:
> > > On Mon, 10 Aug 2026 13:09:07 +0300
> > > Matti Vaittinen <matti.vaittinen@linux.dev> wrote:  

...

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

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

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

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

-- 
With Best Regards,
Andy Shevchenko

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

[Matti Vaittinen]

On 12/08/2026 12:30, Andy Shevchenko wrote:
> On Wed, Aug 12, 2026 at 10:25:36AM +0200, Andreas Kemnade wrote:
>> On Wed, 12 Aug 2026 11:17:36 +0300
>> Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
>>> On Wed, Aug 12, 2026 at 10:10:50AM +0200, Andreas Kemnade wrote:
>>>> On Mon, 10 Aug 2026 13:09:07 +0300
>>>> Matti Vaittinen <matti.vaittinen@linux.dev> wrote:
> 
> ...
> 
>>>> 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
> 
> 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

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

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

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

[Andy Shevchenko]

On Wed, Aug 12, 2026 at 12:37:49PM +0300, Matti Vaittinen wrote:
> On 12/08/2026 12:30, Andy Shevchenko wrote:
> > On Wed, Aug 12, 2026 at 10:25:36AM +0200, Andreas Kemnade wrote:
> > > On Wed, 12 Aug 2026 11:17:36 +0300
> > > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> > > > On Wed, Aug 12, 2026 at 10:10:50AM +0200, Andreas Kemnade wrote:
> > > > > On Mon, 10 Aug 2026 13:09:07 +0300
> > > > > Matti Vaittinen <matti.vaittinen@linux.dev> wrote:

...

> > > > > 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
> > 
> > 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);

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;

-- 
With Best Regards,
Andy Shevchenko

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

[Matti Vaittinen]

On 12/08/2026 11:25, Andreas Kemnade wrote:
> On Wed, 12 Aug 2026 11:17:36 +0300
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> 
>> On Wed, Aug 12, 2026 at 10:10:50AM +0200, Andreas Kemnade wrote:
>>> On Mon, 10 Aug 2026 13:09:07 +0300
>>> Matti Vaittinen <matti.vaittinen@linux.dev> wrote:
>>
>> ...
>>
>>> 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.
> 

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

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

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

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

[Andreas Kemnade]

On Wed, 12 Aug 2026 12:40:47 +0300
Matti Vaittinen <mazziesaccount@gmail.com> wrote:

> On 12/08/2026 11:25, Andreas Kemnade wrote:
> > On Wed, 12 Aug 2026 11:17:36 +0300
> > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> >   
> >> On Wed, Aug 12, 2026 at 10:10:50AM +0200, Andreas Kemnade wrote:  
> >>> On Mon, 10 Aug 2026 13:09:07 +0300
> >>> Matti Vaittinen <matti.vaittinen@linux.dev> wrote:  
> >>
> >> ...
> >>  
> >>> 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.
> >   
> 
> 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.

> 
> 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. 

> 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

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

[Matti Vaittinen]

[-- Attachment #1: Type: text/plain, Size: 917 bytes --]

From: Matti Vaittinen <mazziesaccount@gmail.com>

The POWER_SUPPLY_PROP_HEALTH is added into property-array twice.

Drop the duplicate property.

Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
Fixes: 5bff79dad20a ("power: supply: Add bd718(15/28/78) charger driver")
---
 drivers/power/supply/bd71828-power.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/power/supply/bd71828-power.c b/drivers/power/supply/bd71828-power.c
index 9927a7e8934a..a564ad7210be 100644
--- a/drivers/power/supply/bd71828-power.c
+++ b/drivers/power/supply/bd71828-power.c
@@ -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,
-- 
2.55.0


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Andy Shevchenko]

On Mon, Aug 10, 2026 at 01:09:25PM +0300, Matti Vaittinen wrote:

> 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)?

-- 
With Best Regards,
Andy Shevchenko

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

[Matti Vaittinen]

[-- Attachment #1: Type: text/plain, Size: 1525 bytes --]

From: Matti Vaittinen <mazziesaccount@gmail.com>

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 <mazziesaccount@gmail.com>
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(-)

diff --git a/drivers/power/supply/bd71828-power.c b/drivers/power/supply/bd71828-power.c
index a564ad7210be..589e12225cb2 100644
--- a/drivers/power/supply/bd71828-power.c
+++ b/drivers/power/supply/bd71828-power.c
@@ -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);
-- 
2.55.0


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Matti Vaittinen]

[-- Attachment #1: Type: text/plain, Size: 2851 bytes --]

From: Matti Vaittinen <mazziesaccount@gmail.com>

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 <mazziesaccount@gmail.com>
Fixes: 0902f8366491 ("power: supply: Support ROHM bd99954 charger")
---
 drivers/power/supply/bd99954-charger.h | 23 +----------------------
 1 file changed, 1 insertion(+), 22 deletions(-)

diff --git a/drivers/power/supply/bd99954-charger.h b/drivers/power/supply/bd99954-charger.h
index f58897925383..d053ac901274 100644
--- a/drivers/power/supply/bd99954-charger.h
+++ b/drivers/power/supply/bd99954-charger.h
@@ -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),
-- 
2.55.0


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Andy Shevchenko]

On Mon, Aug 10, 2026 at 01:10:03PM +0300, Matti Vaittinen wrote:

> 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.

> 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.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 0/5] ROHM bd71828 power-supply fixes

[Kalle Niemi]

On 8/10/26 13:08, Matti Vaittinen wrote:
> 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.

Hello,

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

Tested-by: Kalle Niemi <kaleposti@gmail.com>