The GPO2 on BD71815 is "hard to use on PCB" due to positioning in the middle of the GND pins. It was not needed in a target project so it was decided to mark the pin as GND in data-sheet. The reference driver exposes this pin as GPO and allows driving it - which may cause issues if pin is connected to GND as suggested by data-sheet.

This should be fixed by disabling the GPO2 by default and by allowing control only if it is explicitly requested via DT property.

The BD96801 watchdog driver does not configure the hw_heartbeat values if the watchdog is running when driver is probed. This can cause problems with small timeouts.

As a fix: If watchdog is running at probe time, we should read the configured time-out values and set the hw_heartbeat accordingly.

The BD71815 interrupts should be handled as active low. Example handles them as edge triggered. Handling level active interrupts as edge triggered will cause a race with new IRQ being triggered by device between reading and acking the status bits. This will yield the new IRQ to be not acknowleged and HW to keep the IRQ line pulled low - which means that new edges will never be produced because device keeps IRQ asserted but processor does not detect this.

This is a classical mistake which causes annoying occasional hard-to-reproduce issues where IRQs seem to be no longer generated.

I don't see DVSSEL being used anywhere in the code? DVS could (perhaps) be supported.

Bunch of extra register accesses could be avoided by using the regmap main IRQ functionality.

The BD96801 watchdog driver was originally written to match a very early specification version. The timeout values aren't correct for the current version of the IC. The FAST_NG value from the latest spec (as writing of this) is ~1mS while the driver uses value close to 3.37 mS. This error accumulates to all computed timeout values.

Regmap cache could be utilized for most of the registers.

(Note: Comment is misleading. The code says that no registers are cached)

The LDO5 supports fast toggling between two voltage values using GPIO. SW could support this feature.

The RUN_LEVEL definitions from include/linux/mfd/rohm-generic.h are used for bitmask by regulator drivers. The definitions are an enum W/O set values in the header => first few values (1,2) work fine but value 3 will already be invalid as it also sets both 1&2 and probably never matches 3 (if only one bit is compared).

On BD71837/47/50 this is probably not causing any problems as they only use RUN/IDLE/SUSPEND levels - and regulators which support suspend do also support RUN/IDLE. On BD71828 this may cause problems depending on device-tree.

A fix to this problem was submitted to the Linux community:
https://lore.kernel.org/lkml/[email protected]/T/#u

As was pointed by Guenter Roeck, the "always-running"-property is used both for a wrong purpose and incorrectly.

Without documentation, it looks like the always-running (from
linux,wdt-gpio.yaml) may be abused. Its defined meaning is
"the watchdog is always running and can not be stopped". Its
use here seems to be "start watchdog when loading the module and
prevent it from being stopped".

and

• if (w->always_running)

• bd96801_wdt_start(&w->wdt);
  

I think this needs to set WDOG_HW_RUNNING or the watchdog will trigger
a reboot if the watchdog device is not opened and the watchdog wasn't
already running when the module was loaded.

That makes me wonder what happens if the property is set and the
watchdog daemon isn't started in the bd9576_wdt driver.

For BD9576 this bug is already in upstream

The ROHM BD96801 and BD96802 have lock register. After the start-up a magic value must be written to the lock register before any other register is writable. This is done in the regulator driver.

It is possible the MFD core or the other sub-devices need to access the PMIC registers before this. Those accesses will fail. The "right thing to do (tm)" is to unlock registers already in the MFD driver core before invoking the IRQ chip code or kicking other sub-devices.

The BD96801 experimental driver allocates per regulator ERRB IRQ names from the stack. The regulator_irq_helper() did not originally duplicate memory for IRQ names but passed a pointer to the provided name directly to request_threaded_irq(). This means the IRQ name got trashed when calling function exited.

A fix to the regulator_irq_helper() is sent:
https://lore.kernel.org/lkml/[email protected]/

this will fix the bad memory access for experimental BD96801 regulator driver. Please, apply the regulator_irq_helper() -patch linked above if you use the experimental BD96801 driver with a kernel where this fix is not yet included. (Fix is in no kernel releases as writing of this. The Linux v6.9 development cycle is ongoing)

On watchdog timeout the BD96801 can be either configured to shut-off power outputs or to only generate INTB IRQ. The mode to be used can be selected based on device-tree properties. These properties do not have binding documentation.

Guys, any plans for upstreaming BD71815AGW PMIC driver?

The timeout computation for setting the watchdog heartbeat timer based on device-tree values is done using micro seconds in 32bit signed integer. This will easily overflow when timeouts are in second scale, causing wrong timeout values to be set.

LDO4 enable/disable can be controlled via GPIO. SW could support this.