by Guenter Roeck

[permalink] [raw]

Subject: Re: [PATCH v1 5/6] watchdog: ROHM BD96801 PMIC WDG driver

On 4/30/24 05:01, Matti Vaittinen wrote:
> Introduce driver for WDG block on ROHM BD96801 scalable PMIC.
>
> This driver only supports watchdog with I2C feeding and delayed
> response detection. Whether the watchdog toggles PRSTB pin or
> just causes an interrupt can be configured via device-tree.
>
> The BD96801 PMIC HW supports also window watchdog (too early
> feeding detection) and Q&A mode. These are not supported by
> this driver.
>
> Signed-off-by: Matti Vaittinen <[email protected]>
>
> ---
> Revision history:
> RFCv2 => v1:
> - Fix watchdog time-outs to match DS4
> - Fix target timeout overflow
> - Improve dbg prints
>
> RFCv1 => RFCv2:
> - remove always running
> - add IRQ handling
> - call emergency_restart()
> - drop MODULE_ALIAS and add MODULE_DEVICE_TABLE
> ---
> drivers/watchdog/Kconfig | 13 ++
> drivers/watchdog/Makefile | 1 +
> drivers/watchdog/bd96801_wdt.c | 400 +++++++++++++++++++++++++++++++++
> 3 files changed, 414 insertions(+)
> create mode 100644 drivers/watchdog/bd96801_wdt.c
>
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index 6bee137cfbe0..d97e735e1faa 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -181,6 +181,19 @@ config BD957XMUF_WATCHDOG
> watchdog. Alternatively say M to compile the driver as a module,
> which will be called bd9576_wdt.
>
> +config BD96801_WATCHDOG
> + tristate "ROHM BD96801 PMIC Watchdog"
> + depends on MFD_ROHM_BD96801
> + select WATCHDOG_CORE
> + help
> + Support for the watchdog in the ROHM BD96801 PMIC. Watchdog can be
> + configured to only generate IRQ or to trigger system reset via reset
> + pin.
> +
> + Say Y here to include support for the ROHM BD96801 watchdog.
> + Alternatively say M to compile the driver as a module,
> + which will be called bd96801_wdt.
> +
> config CROS_EC_WATCHDOG
> tristate "ChromeOS EC-based watchdog"
> select WATCHDOG_CORE
> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
> index 3710c218f05e..31bc94436c81 100644
> --- a/drivers/watchdog/Makefile
> +++ b/drivers/watchdog/Makefile
> @@ -217,6 +217,7 @@ obj-$(CONFIG_XEN_WDT) += xen_wdt.o
>
> # Architecture Independent
> obj-$(CONFIG_BD957XMUF_WATCHDOG) += bd9576_wdt.o
> +obj-$(CONFIG_BD96801_WATCHDOG) += bd96801_wdt.o
> obj-$(CONFIG_CROS_EC_WATCHDOG) += cros_ec_wdt.o
> obj-$(CONFIG_DA9052_WATCHDOG) += da9052_wdt.o
> obj-$(CONFIG_DA9055_WATCHDOG) += da9055_wdt.o
> diff --git a/drivers/watchdog/bd96801_wdt.c b/drivers/watchdog/bd96801_wdt.c
> new file mode 100644
> index 000000000000..6069f1d75356
> --- /dev/null
> +++ b/drivers/watchdog/bd96801_wdt.c
> @@ -0,0 +1,400 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (C) 2024 ROHM Semiconductors
> + *
> + * ROHM BD96801 watchdog driver
> + */
> +
> +#include <linux/interrupt.h>
> +#include <linux/kernel.h>
> +#include <linux/mfd/rohm-bd96801.h>
> +#include <linux/mfd/rohm-generic.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +#include <linux/watchdog.h>
> +
> +static bool nowayout;
> +module_param(nowayout, bool, 0);
> +MODULE_PARM_DESC(nowayout,
> + "Watchdog cannot be stopped once started (default=\"false\")");
> +
> +#define BD96801_WD_TMO_SHORT_MASK 0x70
> +#define BD96801_WD_RATIO_MASK 0x3
> +#define BD96801_WD_TYPE_MASK 0x4
> +#define BD96801_WD_TYPE_SLOW 0x4
> +#define BD96801_WD_TYPE_WIN 0x0
> +
> +#define BD96801_WD_EN_MASK 0x3
> +#define BD96801_WD_IF_EN 0x1
> +#define BD96801_WD_QA_EN 0x2
> +#define BD96801_WD_DISABLE 0x0
> +
> +#define BD96801_WD_ASSERT_MASK 0x8
> +#define BD96801_WD_ASSERT_RST 0x8
> +#define BD96801_WD_ASSERT_IRQ 0x0
> +
> +#define BD96801_WD_FEED_MASK 0x1
> +#define BD96801_WD_FEED 0x1
> +
> +/* units in uS */
> +#define FASTNG_MIN 11
> +
> +#define BD96801_WDT_DEFAULT_MARGIN_MS 1843
> +/* Unit is seconds */
> +#define DEFAULT_TIMEOUT 30
> +
> +/*
> + * BD96801 WDG supports window mode so the TMO consists of SHORT and LONG
> + * timeout values. SHORT time is meaningfull only in window mode where feeding

meaningful

> + * period shorter than SHORT would be an error. LONG time is used to detect if
> + * feeding is not occurring within given time limit (SoC SW hangs). The LONG
> + * timeout time is a multiple of (2, 4, 8 0r 16 times) the SHORT timeout.

0r -> or

> + */
> +
> +struct wdtbd96801 {
> + struct device *dev;
> + struct regmap *regmap;
> + struct watchdog_device wdt;
> +};
> +
> +static int bd96801_wdt_ping(struct watchdog_device *wdt)
> +{
> + struct wdtbd96801 *w = watchdog_get_drvdata(wdt);
> +
> + return regmap_update_bits(w->regmap, BD96801_REG_WD_FEED,
> + BD96801_WD_FEED_MASK, BD96801_WD_FEED);
> +}
> +
> +static int bd96801_wdt_start(struct watchdog_device *wdt)
> +{
> + struct wdtbd96801 *w = watchdog_get_drvdata(wdt);
> +
> + return regmap_update_bits(w->regmap, BD96801_REG_WD_CONF,
> + BD96801_WD_EN_MASK, BD96801_WD_IF_EN);
> +}
> +
> +static int bd96801_wdt_stop(struct watchdog_device *wdt)
> +{
> + struct wdtbd96801 *w = watchdog_get_drvdata(wdt);
> +
> + return regmap_update_bits(w->regmap, BD96801_REG_WD_CONF,
> + BD96801_WD_EN_MASK, BD96801_WD_DISABLE);
> +}
> +
> +static const struct watchdog_info bd96801_wdt_info = {
> + .options = WDIOF_MAGICCLOSE | WDIOF_KEEPALIVEPING |
> + WDIOF_SETTIMEOUT,
> + .identity = "BD96801 Watchdog",
> +};
> +
> +static const struct watchdog_ops bd96801_wdt_ops = {
> + .start = bd96801_wdt_start,
> + .stop = bd96801_wdt_stop,
> + .ping = bd96801_wdt_ping,
> +};
> +
> +static int find_closest_fast(unsigned int target, int *sel, unsigned int *val)
> +{
> + unsigned int window = FASTNG_MIN;
> + int i;
> +
> + for (i = 0; i < 8 && window < target; i++)
> + window <<= 1;
> +
> + *val = window;
> + *sel = i;
> +
> + if (i == 8)
> + return -EINVAL;
> +

It might make sense to have the error check before assigning return values,
similar to the other find functions.

> + return 0;
> +}
> +
> +static int find_closest_slow_by_fast(unsigned int fast_val, unsigned int *target, int *slowsel)
> +{
> + static const int multipliers[] = {2, 4, 8, 16};
> + int sel;
> +
> + for (sel = 0; sel < ARRAY_SIZE(multipliers) &&
> + multipliers[sel] * fast_val < *target; sel++)
> + ;
> +
> + if (sel == ARRAY_SIZE(multipliers))
> + return -EINVAL;
> +
> + *slowsel = sel;
> + *target = multipliers[sel] * fast_val;
> +
> + return 0;
> +}
> +
> +static int find_closest_slow(unsigned int *target, int *slow_sel, int *fast_sel)
> +{
> + static const int multipliers[] = {2, 4, 8, 16};
> + unsigned int window = FASTNG_MIN;
> + unsigned int val = 0;
> + int i, j;
> +
> + for (i = 0; i < 8; i++) {
> + for (j = 0; j < ARRAY_SIZE(multipliers); j++) {
> + unsigned int slow;
> +
> + slow = window * multipliers[j];
> + if (slow >= *target && (!val || slow < val)) {
> + val = slow;
> + *fast_sel = i;
> + *slow_sel = j;
> + }
> + }
> + window <<= 1;
> + }
> + if (!val)
> + return -EINVAL;
> +
> + *target = val;
> +
> + return 0;
> +}
> +
> +static int bd96801_set_wdt_mode(struct wdtbd96801 *w, unsigned int hw_margin,
> + unsigned int hw_margin_min)
> +{
> + int fastng, slowng, type, ret, reg, mask;
> + struct device *dev = w->dev;
> +
> + /*
> + * Convert to 100uS to guarantee reasonable timeouts fit in
> + * 32bit maintaining also a decent accuracy.
> + */
> + hw_margin *= 10;
> + hw_margin_min *= 10;
> +
> + if (hw_margin_min) {
> + unsigned int min;
> +
> + type = BD96801_WD_TYPE_WIN;
> + dev_dbg(dev, "Setting type WINDOW 0x%x\n", type);
> + ret = find_closest_fast(hw_margin_min, &fastng, &min);
> + if (ret) {
> + dev_err(dev, "bad WDT window for fast timeout\n");
> + return ret;
> + }
> +
> + ret = find_closest_slow_by_fast(min, &hw_margin, &slowng);
> + if (ret) {
> + dev_err(dev, "bad WDT window\n");
> + return ret;
> + }
> + w->wdt.min_hw_heartbeat_ms = min / 10;
> + } else {
> + type = BD96801_WD_TYPE_SLOW;
> + dev_dbg(dev, "Setting type SLOW 0x%x\n", type);
> + ret = find_closest_slow(&hw_margin, &slowng, &fastng);
> + if (ret) {
> + dev_err(dev, "bad WDT window\n");

What is the value of those error messages ? To me they only leave big
question marks. One would see "bad WDT window" or "bad WDT window for
fast timeout" and then what ? If the cause is bad values for hw_margin
and/or hw_margin_min, the message should include the offending values
and not leave the user in the dark.

> + return ret;
> + }
> + }
> +
> + w->wdt.max_hw_heartbeat_ms = hw_margin / 10;
> +
> + fastng <<= ffs(BD96801_WD_TMO_SHORT_MASK) - 1;

Any reason fore not using standard functionality such as FIELD_PREP here ?

> +
> + reg = slowng | fastng;
> + mask = BD96801_WD_RATIO_MASK | BD96801_WD_TMO_SHORT_MASK;
> + ret = regmap_update_bits(w->regmap, BD96801_REG_WD_TMO,
> + mask, reg);
> + if (ret)
> + return ret;
> +
> + ret = regmap_update_bits(w->regmap, BD96801_REG_WD_CONF,
> + BD96801_WD_TYPE_MASK, type);
> +
> + return ret;
> +}
> +
> +static int bd96801_set_heartbeat_from_hw(struct wdtbd96801 *w,
> + unsigned int conf_reg)
> +{
> + int ret;
> + unsigned int val, sel, fast;
> +
> + /*
> + * The BD96801 supports a somewhat peculiar QA-mode, which we do not
> + * support in this driver. If the QA-mode is enabled then we just
> + * warn and bail-out.
> + */
> + if ((conf_reg & BD96801_WD_EN_MASK) != BD96801_WD_IF_EN) {
> + dev_warn(w->dev, "watchdog set to Q&A mode - exiting\n");

This should be dev_err(). It is not just a warning.

> + return -EINVAL;
> + }
> +
> + ret = regmap_read(w->regmap, BD96801_REG_WD_TMO, &val);
> + if (ret)
> + return ret;
> +
> + sel = val & BD96801_WD_TMO_SHORT_MASK;
> + sel >>= ffs(BD96801_WD_TMO_SHORT_MASK) - 1;

Same question as above: Why not use FIELD_GET() ?

> + fast = FASTNG_MIN << sel;
> +
> + sel = (val & BD96801_WD_RATIO_MASK) + 1;
> + w->wdt.max_hw_heartbeat_ms = (fast << sel) / USEC_PER_MSEC;
> +
> + if ((conf_reg & BD96801_WD_TYPE_MASK) == BD96801_WD_TYPE_WIN)
> + w->wdt.min_hw_heartbeat_ms = fast / USEC_PER_MSEC;
> +
> + return 0;
> +}
> +
> +static int init_wdg_hw(struct wdtbd96801 *w)
> +{
> + u32 hw_margin[2];
> + int count, ret;
> + u32 hw_margin_max = BD96801_WDT_DEFAULT_MARGIN_MS, hw_margin_min = 0;
> +
> + count = device_property_count_u32(w->dev->parent, "rohm,hw-timeout-ms");
> + if (count < 0 && count != -EINVAL)
> + return count;
> +
> + if (count > 0) {
> + if (count > ARRAY_SIZE(hw_margin))
> + return -EINVAL;
> +
> + ret = device_property_read_u32_array(w->dev->parent,
> + "rohm,hw-timeout-ms",
> + &hw_margin[0], count);
> + if (ret < 0)
> + return ret;
> +
> + if (count == 1)
> + hw_margin_max = hw_margin[0];
> +
> + if (count == 2) {
> + if (hw_margin[1] > hw_margin[0]) {
> + hw_margin_max = hw_margin[1];
> + hw_margin_min = hw_margin[0];
> + } else {
> + hw_margin_max = hw_margin[0];
> + hw_margin_min = hw_margin[1];
> + }
> + }
> + }
> +
> + ret = bd96801_set_wdt_mode(w, hw_margin_max, hw_margin_min);
> + if (ret)
> + return ret;
> +
> + ret = device_property_match_string(w->dev->parent, "rohm,wdg-action",
> + "prstb");
> + if (ret >= 0) {
> + ret = regmap_update_bits(w->regmap, BD96801_REG_WD_CONF,
> + BD96801_WD_ASSERT_MASK,
> + BD96801_WD_ASSERT_RST);
> + return ret;
> + }
> +
> + ret = device_property_match_string(w->dev->parent, "rohm,wdg-action",
> + "intb-only");
> + if (ret >= 0) {
> + ret = regmap_update_bits(w->regmap, BD96801_REG_WD_CONF,
> + BD96801_WD_ASSERT_MASK,
> + BD96801_WD_ASSERT_IRQ);
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +extern void emergency_restart(void);

No. include linux/reboot.h instead.

> +static irqreturn_t bd96801_irq_hnd(int irq, void *data)
> +{
> + emergency_restart();
> +
> + return IRQ_NONE;
> +}
> +
> +static int bd96801_wdt_probe(struct platform_device *pdev)
> +{
> + struct wdtbd96801 *w;
> + int ret, irq;
> + unsigned int val;
> +
> + w = devm_kzalloc(&pdev->dev, sizeof(*w), GFP_KERNEL);
> + if (!w)
> + return -ENOMEM;
> +
> + w->regmap = dev_get_regmap(pdev->dev.parent, NULL);
> + w->dev = &pdev->dev;
> +
> + w->wdt.info = &bd96801_wdt_info;
> + w->wdt.ops = &bd96801_wdt_ops;
> + w->wdt.parent = pdev->dev.parent;
> + w->wdt.timeout = DEFAULT_TIMEOUT;
> + watchdog_set_drvdata(&w->wdt, w);
> +
> + ret = regmap_read(w->regmap, BD96801_REG_WD_CONF, &val);
> + if (ret)
> + return dev_err_probe(&pdev->dev, ret,
> + "Failed to get the watchdog state\n");
> +
> + /*
> + * If the WDG is already enabled we assume it is configured by boot.
> + * In this case we just update the hw-timeout based on values set to
> + * the timeout / mode registers and leave the hardware configs
> + * untouched.
> + */
> + if ((val & BD96801_WD_EN_MASK) != BD96801_WD_DISABLE) {
> + dev_dbg(&pdev->dev, "watchdog was running during probe\n");
> + ret = bd96801_set_heartbeat_from_hw(w, val);
> + if (ret)
> + return ret;
> +
> + set_bit(WDOG_HW_RUNNING, &w->wdt.status);
> + } else {
> + /* If WDG is not running so we will initializate it */
> + ret = init_wdg_hw(w);
> + if (ret)
> + return ret;
> + }
> +
> + dev_dbg(w->dev, "heartbeat set to %u - %u\n",
> + w->wdt.min_hw_heartbeat_ms, w->wdt.max_hw_heartbeat_ms);
> +
> + watchdog_init_timeout(&w->wdt, 0, pdev->dev.parent);
> + watchdog_set_nowayout(&w->wdt, nowayout);
> + watchdog_stop_on_reboot(&w->wdt);
> +
> + irq = platform_get_irq_byname(pdev, "bd96801-wdg");
> + if (irq > 0) {
> + ret = devm_request_threaded_irq(&pdev->dev, irq, NULL,
> + bd96801_irq_hnd,
> + IRQF_ONESHOT, "bd96801-wdg",
> + NULL);
> + if (ret)
> + return dev_err_probe(&pdev->dev, ret,
> + "Failed to register IRQ\n");
> + }
> +
> + return devm_watchdog_register_device(&pdev->dev, &w->wdt);
> +}
> +
> +static const struct platform_device_id bd96801_wdt_id[] = {
> + { "bd96801-wdt", },
> + { }
> +};
> +MODULE_DEVICE_TABLE(platform, bd96801_wdt_id);
> +
> +static struct platform_driver bd96801_wdt = {
> + .driver = {
> + .name = "bd96801-wdt"
> + },
> + .probe = bd96801_wdt_probe,
> + .id_table = bd96801_wdt_id,
> +};
> +module_platform_driver(bd96801_wdt);
> +
> +MODULE_AUTHOR("Matti Vaittinen <[email protected]>");
> +MODULE_DESCRIPTION("BD96801 watchdog driver");
> +MODULE_LICENSE("GPL");

2024-05-01 10:16:40

by Matti Vaittinen

[permalink] [raw]

Subject: Re: [PATCH v1 5/6] watchdog: ROHM BD96801 PMIC WDG driver

Hi Guenter,

Thanks again for the review :) I agree with all of your comments, thanks!

On 4/30/24 19:24, Guenter Roeck wrote:
> On 4/30/24 05:01, Matti Vaittinen wrote:
>> Introduce driver for WDG block on ROHM BD96801 scalable PMIC.
>>
>> This driver only supports watchdog with I2C feeding and delayed
>> response detection. Whether the watchdog toggles PRSTB pin or
>> just causes an interrupt can be configured via device-tree.
>>
>> The BD96801 PMIC HW supports also window watchdog (too early
>> feeding detection) and Q&A mode. These are not supported by
>> this driver.
>>
>> Signed-off-by: Matti Vaittinen <[email protected]>
>>
>> ---
>> Revision history:
>> RFCv2 => v1:
>> - Fix watchdog time-outs to match DS4
>> - Fix target timeout overflow
>> - Improve dbg prints
>>
>> RFCv1 => RFCv2:
>> - remove always running
>> - add IRQ handling
>> - call emergency_restart()
>> - drop MODULE_ALIAS and add MODULE_DEVICE_TABLE
>> ---

..

>> +
>> +static int bd96801_set_wdt_mode(struct wdtbd96801 *w, unsigned int
>> hw_margin,
>> +                   unsigned int hw_margin_min)
>> +{
>> +    int fastng, slowng, type, ret, reg, mask;
>> +    struct device *dev = w->dev;
>> +
>> +    /*
>> +     * Convert to 100uS to guarantee reasonable timeouts fit in
>> +     * 32bit maintaining also a decent accuracy.
>> +     */
>> +    hw_margin *= 10;
>> +    hw_margin_min *= 10;
>> +
>> +    if (hw_margin_min) {
>> +        unsigned int min;
>> +
>> +        type = BD96801_WD_TYPE_WIN;
>> +        dev_dbg(dev, "Setting type WINDOW 0x%x\n", type);
>> +        ret = find_closest_fast(hw_margin_min, &fastng, &min);
>> +        if (ret) {
>> +            dev_err(dev, "bad WDT window for fast timeout\n");
>> +            return ret;
>> +        }
>> +
>> +        ret = find_closest_slow_by_fast(min, &hw_margin, &slowng);
>> +        if (ret) {
>> +            dev_err(dev, "bad WDT window\n");
>> +            return ret;
>> +        }
>> +        w->wdt.min_hw_heartbeat_ms = min / 10;
>> +    } else {
>> +        type = BD96801_WD_TYPE_SLOW;
>> +        dev_dbg(dev, "Setting type SLOW 0x%x\n", type);
>> +        ret = find_closest_slow(&hw_margin, &slowng, &fastng);
>> +        if (ret) {
>> +            dev_err(dev, "bad WDT window\n");
>
> What is the value of those error messages ? To me they only leave big
> question marks. One would see "bad WDT window" or "bad WDT window for
> fast timeout" and then what ? If the cause is bad values for hw_margin
> and/or hw_margin_min, the message should include the offending values
> and not leave the user in the dark.

Well, at least one can grep the error from module which printed it :)
But yes, you have a very valid point. I will see how to improve, thanks!

>
>> +            return ret;
>> +        }
>> +    }
>> +
>> +    w->wdt.max_hw_heartbeat_ms = hw_margin / 10;
>> +
>> +    fastng <<= ffs(BD96801_WD_TMO_SHORT_MASK) - 1;
>
> Any reason fore not using standard functionality such as FIELD_PREP here ?

A reason, yes. A good reason, no :) Reason is that I wrote this before I
learned about the FIELD_PREP() and FIELD_GET(). And actually, I have
always found those two more confusing than the open-coded shift, '|' and
'&'. Still, I believe the FIELD_GET() and FIELD_PREP() make it obvious
for a reader that the bit magic is just a standard register field stuff.

So, no good reason. I'll fix this for the next version.

..

>> +extern void emergency_restart(void);
>
> No. include linux/reboot.h instead.

Hmm.. I am sure I had checked the headers for prototypes before doing
something like this... I think I must've missed something. I'll fix this
for the next version, or add a good explanation if there indeed was some
problem finding the prototype.

Yours,
-- Matti

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

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

2024-05-02 16:34:47

On 5/3/24 10:41, Lee Jones wrote:
> On Tue, 30 Apr 2024, Matti Vaittinen wrote:
>
>> The ROHM BD96801 PMIC is highly customizable automotive grade PMIC
>> which integrates regulator and watchdog funtionalities.
>>
>> Provide INTB IRQ and register accesses for regulator/watchdog drivers.
>>
>> Signed-off-by: Matti Vaittinen <[email protected]>
>>
>> ---
>> Changelog: RFCv2 => v1:
>> - drop ERRB interrupts (for now)
>> - bd96801: Unlock registers in core driver
>>
>> Changelog: RFCv1 => RFCv2
>> - Work-around the IRQ domain name conflict
>> - Add watchdog IRQ
>> - Various styling fixes based on review by Lee
>> ---
>> drivers/mfd/Kconfig | 13 ++
>> drivers/mfd/Makefile | 1 +
>> drivers/mfd/rohm-bd96801.c | 278 +++++++++++++++++++++++++++++++
>> include/linux/mfd/rohm-bd96801.h | 215 ++++++++++++++++++++++++
>> include/linux/mfd/rohm-generic.h | 1 +
>> 5 files changed, 508 insertions(+)
>> create mode 100644 drivers/mfd/rohm-bd96801.c
>> create mode 100644 include/linux/mfd/rohm-bd96801.h
>
> Still some nits, sorry.

No need to be sorry :) And, thanks for the review!

> I probably wouldn't have been so picky if I hadn't have found the unused enum.

Yeah, that's what you say ... XD
It's Ok. Besides, I like your style of providing the better alternatives
along with your comments. (ref the print comments which I agreed with
and dropped from this reply).

>> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
>> index 4b023ee229cf..9e874453d94d 100644
>> --- a/drivers/mfd/Kconfig
>> +++ b/drivers/mfd/Kconfig
>> @@ -2089,6 +2089,19 @@ config MFD_ROHM_BD957XMUF
>> BD9573MUF Power Management ICs. BD9576 and BD9573 are primarily
>> designed to be used to power R-Car series processors.
>>
>> +config MFD_ROHM_BD96801
>> + tristate "ROHM BD96801 Power Management IC"
>> + depends on I2C=y
>> + depends on OF
>> + select REGMAP_I2C
>> + select REGMAP_IRQ
>> + select MFD_CORE
>> + help
>> + Select this option to get support for the ROHM BD96801 Power
>> + Management IC. The ROHM BD96801 is a highly scalable Power Management
>> + IC for industrial and automotive use. The BD96801 can be used as a
>> + master PMIC in a chained PMIC solution with suitable companion PMICs.
>> +
>> config MFD_STM32_LPTIMER
>> tristate "Support for STM32 Low-Power Timer"
>> depends on (ARCH_STM32 && OF) || COMPILE_TEST
>> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
>> index c66f07edcd0e..e792892d4a8b 100644
>> --- a/drivers/mfd/Makefile
>> +++ b/drivers/mfd/Makefile
>> @@ -264,6 +264,7 @@ obj-$(CONFIG_RAVE_SP_CORE) += rave-sp.o
>> obj-$(CONFIG_MFD_ROHM_BD71828) += rohm-bd71828.o
>> obj-$(CONFIG_MFD_ROHM_BD718XX) += rohm-bd718x7.o
>> obj-$(CONFIG_MFD_ROHM_BD957XMUF) += rohm-bd9576.o
>> +obj-$(CONFIG_MFD_ROHM_BD96801) += rohm-bd96801.o
>> obj-$(CONFIG_MFD_STMFX) += stmfx.o
>> obj-$(CONFIG_MFD_KHADAS_MCU) += khadas-mcu.o
>> obj-$(CONFIG_MFD_ACER_A500_EC) += acer-ec-a500.o
>> diff --git a/drivers/mfd/rohm-bd96801.c b/drivers/mfd/rohm-bd96801.c
>> new file mode 100644
>> index 000000000000..1e9c49c857c1
>> --- /dev/null
>> +++ b/drivers/mfd/rohm-bd96801.c

..

>> +
>> +enum {
>> + WDG_CELL = 0,
>> + REGULATOR_CELL,
>> +};
>
> Dead code?

Yep. A leftover from the version with the ERRB stuff. Thanks for
pointing it out!

..

>> +
>> +static int __init bd96801_i2c_init(void)
>> +{
>> + return i2c_add_driver(&bd96801_i2c_driver);
>> +}
>> +
>> +/* Initialise early so consumer devices can complete system boot? */
>
> Why the question mark? What are you unsure about?
>
> Why doesn't -EPROBE_DEFER work for this?

I am unsure of what kind of dependencies the real setups using these
PMICs have at boot time. Hence the (?). I've written drivers for a few
PMICs, and I've sometimes just done the usual:

module_i2c_driver();

This, however, tends to get converted to boilerplate + subsys_initcall()
by customers who actually start using the drivers - so I believe there
is a problem in getting the consumers powered if the PMIC(s) are
initialized too late. Anyways, it's probably nicer to try making it so
it works better on different setups out of the box :)

I'd appreciate if someone wanted to shed some more light on this though!

Yours,
-- Matti

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

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