mbox series

[v1,0/6] Support ROHM BD9576MUF and BD9573MUF PMICs

Message ID cover.1600329307.git.matti.vaittinen@fi.rohmeurope.com
Headers show
Series Support ROHM BD9576MUF and BD9573MUF PMICs | expand

Message

Vaittinen, Matti Sept. 17, 2020, 8:01 a.m. UTC
Initial support for ROHM BD9576MUF and BD9573MUF PMICs.

These PMICs are primarily intended to be used to power the R-Car family
processors. BD9576MUF includes some additional safety features the
BD9573MUF does not have. This initial version of drivers does not
utilize these features and for now the SW behaviour is identical.

Please note that this version of drivers is only tested on BD9576MUF
but according to the data-sheets the relevant parts of registers should
be same so drivers should also work on BD9573MUF.

This patch series includes MFD, watchdog and regulator drivers with
basic functionality such as:

- Enabling and pinging the watchdog
- configuring watchog timeout / window from device-tree
- reading regulator states/voltages
- enabling/disabling VOUT1 (VD50) when control mode B is used.

This patch series does not bring interrupt support. BD9576MUF and BD9573MUF
are designed to keep the IRQ line low for whole duration of error
condition. IRQ can't be 'acked'. So proper IRQ support would require
some IRQ limiter implementation (delayed unmask?) in order to not hog
the CPU.

---


Matti Vaittinen (6):
  dt_bindings: mfd: Add ROHM BD9576MUF and BD9573MUF PMICs
  dt_bindings: regulator: Add ROHM BD9576MUF and BD9573MUF PMICs
  mfd: Support ROHM BD9576MUF and BD9573MUF
  wdt: Support wdt on ROHM BD9576MUF and BD9573MUF
  regulator: Support ROHM BD9576MUF and BD9573MUF
  MAINTAINERS: Add ROHM BD9576MUF and BD9573MUF drivers

 .../bindings/mfd/rohm,bd9576-pmic.yaml        | 129 +++++++
 .../regulator/rohm,bd9576-regulator.yaml      |  33 ++
 MAINTAINERS                                   |   4 +
 drivers/mfd/Kconfig                           |  11 +
 drivers/mfd/Makefile                          |   1 +
 drivers/mfd/rohm-bd9576.c                     | 130 +++++++
 drivers/regulator/Kconfig                     |  10 +
 drivers/regulator/Makefile                    |   1 +
 drivers/regulator/bd9576-regulator.c          | 337 ++++++++++++++++++
 drivers/watchdog/Kconfig                      |  13 +
 drivers/watchdog/Makefile                     |   1 +
 drivers/watchdog/bd9576_wdt.c                 | 295 +++++++++++++++
 include/linux/mfd/rohm-bd957x.h               |  61 ++++
 include/linux/mfd/rohm-generic.h              |   2 +
 14 files changed, 1028 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mfd/rohm,bd9576-pmic.yaml
 create mode 100644 Documentation/devicetree/bindings/regulator/rohm,bd9576-regulator.yaml
 create mode 100644 drivers/mfd/rohm-bd9576.c
 create mode 100644 drivers/regulator/bd9576-regulator.c
 create mode 100644 drivers/watchdog/bd9576_wdt.c
 create mode 100644 include/linux/mfd/rohm-bd957x.h


base-commit: f4d51dffc6c01a9e94650d95ce0104964f8ae822

Comments

Mark Brown Sept. 17, 2020, 6:58 p.m. UTC | #1
On Thu, 17 Sep 2020 11:01:27 +0300, Matti Vaittinen wrote:
> Initial support for ROHM BD9576MUF and BD9573MUF PMICs.
> 
> These PMICs are primarily intended to be used to power the R-Car family
> processors. BD9576MUF includes some additional safety features the
> BD9573MUF does not have. This initial version of drivers does not
> utilize these features and for now the SW behaviour is identical.
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git for-next

Thanks!

[1/2] dt_bindings: regulator: Add ROHM BD9576MUF and BD9573MUF PMICs
      commit: fdb2f9ffc9f533ceef16666818557ea7b6edfe2a
[2/2] regulator: Support ROHM BD9576MUF and BD9573MUF
      commit: b014e9fae7e7de4329a7092ade4256982c5ce974

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark
Guenter Roeck Sept. 18, 2020, 5:45 a.m. UTC | #2
On 9/17/20 1:03 AM, Matti Vaittinen wrote:
> Add Watchdog support for ROHM BD9576MUF and BD9573MUF PMICs which are
> mainly used to power the R-Car series processors. The watchdog is
> pinged using a GPIO and enabled using another GPIO. Additionally
> watchdog time-out can be configured to HW prior starting the watchdog.
> Watchdog timeout can be configured to detect only delayed ping or in
> a window mode where also too fast pings are detected.
> 
> Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
> ---
>  drivers/watchdog/Kconfig      |  13 ++
>  drivers/watchdog/Makefile     |   1 +
>  drivers/watchdog/bd9576_wdt.c | 295 ++++++++++++++++++++++++++++++++++
>  3 files changed, 309 insertions(+)
>  create mode 100644 drivers/watchdog/bd9576_wdt.c
> 
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index ab7aad5a1e69..d042a517a946 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -172,6 +172,19 @@ config BD70528_WATCHDOG
>  	  Alternatively say M to compile the driver as a module,
>  	  which will be called bd70528_wdt.
>  
> +config BD957XMUF_WATCHDOG
> +	tristate "ROHM BD9576MUF and BD9573MUF PMIC Watchdog"
> +	depends on MFD_ROHM_BD957XMUF
> +	select WATCHDOG_CORE
> +	help
> +	  Support for the watchdog in the ROHM BD9576 and BD9573 PMICs.
> +	  These PMIC ICs contain watchdog block which can be configured
> +	  to toggle reset line if SoC fails to ping watchdog via GPIO.
> +
> +	  Say Y here to include support for the ROHM BD9576 or BD9573
> +	  watchdog. Alternatively say M to compile the driver as a module,
> +	  which will be called bd9576_wdt.
> +
>  config DA9052_WATCHDOG
>  	tristate "Dialog DA9052 Watchdog"
>  	depends on PMIC_DA9052 || COMPILE_TEST
> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
> index 97bed1d3d97c..14d75f98e3df 100644
> --- a/drivers/watchdog/Makefile
> +++ b/drivers/watchdog/Makefile
> @@ -208,6 +208,7 @@ obj-$(CONFIG_XEN_WDT) += xen_wdt.o
>  
>  # Architecture Independent
>  obj-$(CONFIG_BD70528_WATCHDOG) += bd70528_wdt.o
> +obj-$(CONFIG_BD957XMUF_WATCHDOG) += bd9576_wdt.o
>  obj-$(CONFIG_DA9052_WATCHDOG) += da9052_wdt.o
>  obj-$(CONFIG_DA9055_WATCHDOG) += da9055_wdt.o
>  obj-$(CONFIG_DA9062_WATCHDOG) += da9062_wdt.o
> diff --git a/drivers/watchdog/bd9576_wdt.c b/drivers/watchdog/bd9576_wdt.c
> new file mode 100644
> index 000000000000..917c8c7ddeb1
> --- /dev/null
> +++ b/drivers/watchdog/bd9576_wdt.c
> @@ -0,0 +1,295 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (C) 2020 ROHM Semiconductors
> + *
> + * ROHM BD9576MUF and BD9573MUF Watchdog driver
> + */
> +
> +#include <linux/err.h>
> +#include <linux/delay.h>

Alphabetic include file order please.

> +#include <linux/mfd/rohm-bd957x.h>
> +#include <linux/module.h>
> +#include <linux/gpio/consumer.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 HW_MARGIN_MIN 2
> +#define HW_MARGIN_MAX 4416
> +#define BD957X_WDT_DEFAULT_MARGIN 4416
> +
> +struct bd9576_wdt_priv {
> +	struct gpio_desc	*gpiod_ping;
> +	struct gpio_desc	*gpiod_en;
> +	struct device		*dev;
> +	struct regmap		*regmap;
> +	bool			always_running;
> +	struct watchdog_device	wdd;
> +};
> +
> +static void bd9576_wdt_disable(struct bd9576_wdt_priv *priv)
> +{
> +	gpiod_set_value_cansleep(priv->gpiod_en, 0);
> +}
> +
> +static int bd9576_wdt_ping(struct watchdog_device *wdd)
> +{
> +	struct bd9576_wdt_priv *priv = watchdog_get_drvdata(wdd);
> +
> +	/* Pulse */
> +	gpiod_set_value_cansleep(priv->gpiod_ping, 1);
> +	gpiod_set_value_cansleep(priv->gpiod_ping, 0);
> +
> +	return 0;
> +}
> +
> +static int bd9576_wdt_start(struct watchdog_device *wdd)
> +{
> +	struct bd9576_wdt_priv *priv = watchdog_get_drvdata(wdd);
> +
> +	gpiod_set_value_cansleep(priv->gpiod_en, 1);
> +
> +	return bd9576_wdt_ping(wdd);
> +}
> +
> +static int bd9576_wdt_stop(struct watchdog_device *wdd)
> +{
> +	struct bd9576_wdt_priv *priv = watchdog_get_drvdata(wdd);
> +
> +	if (!priv->always_running)
> +		bd9576_wdt_disable(priv);
> +	else
> +		set_bit(WDOG_HW_RUNNING, &wdd->status);
> +
> +	return 0;
> +}
> +
> +static const struct watchdog_info bd957x_wdt_ident = {
> +	.options	= WDIOF_MAGICCLOSE | WDIOF_KEEPALIVEPING |
> +			  WDIOF_SETTIMEOUT,
> +	.identity	= "BD957x Watchdog",
> +};
> +
> +static const struct watchdog_ops bd957x_wdt_ops = {
> +	.owner		= THIS_MODULE,
> +	.start		= bd9576_wdt_start,
> +	.stop		= bd9576_wdt_stop,
> +	.ping		= bd9576_wdt_ping,
> +};
> +
> +/* Unit is hundreds of uS */
> +#define FASTNG_MIN 23
> +
> +static int find_closest_fast(int target, int *sel, int *val)
> +{
> +	int i;
> +	int window = FASTNG_MIN;
> +
> +	for (i = 0; i < 8 && window < target; i++)
> +		window <<= 1;
> +
> +	*val = window;
> +	*sel = i;
> +
> +	if (i == 8)
> +		return -EINVAL;
> +
> +	return 0;
> +
> +}
> +
> +static int find_closest_slow_by_fast(int fast_val, int target, int *slowsel)
> +{
> +	int sel;
> +	static const int multipliers[] = {2, 3, 7, 15};
> +
> +	for (sel = 0; sel < ARRAY_SIZE(multipliers) &&
> +	     multipliers[sel] * fast_val < target; sel++)
> +		;
> +
> +	if (sel == ARRAY_SIZE(multipliers))
> +		return -EINVAL;
> +
> +	*slowsel = sel;
> +
> +	return 0;
> +}
> +
> +static int find_closest_slow(int target, int *slow_sel, int *fast_sel)
> +{
> +	static const int multipliers[] = {2, 3, 7, 15};
> +	int i, j;
> +	int val = 0;
> +	int window = FASTNG_MIN;
> +
> +	for (i = 0; i < 8; i++) {
> +		for (j = 0; j < ARRAY_SIZE(multipliers); j++) {
> +			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;
> +
> +	return 0;
> +}
> +
> +#define BD957X_WDG_TYPE_WINDOW BIT(5)
> +#define BD957X_WDG_TYPE_SLOW 0
> +#define BD957X_WDG_TYPE_MASK BIT(5)
> +#define BD957X_WDG_NG_RATIO_MASK 0x18
> +#define BD957X_WDG_FASTNG_MASK 0x7
> +
> +static int bd957x_set_wdt_mode(struct bd9576_wdt_priv *priv, int hw_margin,
> +			       int hw_margin_min)
> +{
> +	int ret, fastng, slowng, type, reg, mask;
> +	struct device *dev = priv->dev;
> +
> +	/* convert to 100uS */
> +	hw_margin *= 10;
> +	hw_margin_min *= 10;
> +	if (hw_margin_min) {
> +		int min;
> +
> +		type = BD957X_WDG_TYPE_WINDOW;
> +		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;
> +		}
> +
> +	} else {
> +		type = BD957X_WDG_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");
> +			return ret;
> +		}
> +	}
> +
> +	slowng <<= ffs(BD957X_WDG_NG_RATIO_MASK) - 1;
> +	reg = type | slowng | fastng;
> +	mask = BD957X_WDG_TYPE_MASK | BD957X_WDG_NG_RATIO_MASK |
> +	       BD957X_WDG_FASTNG_MASK;
> +	ret = regmap_update_bits(priv->regmap, BD957X_REG_WDT_CONF,
> +				 mask, reg);
> +
> +	return ret;
> +}
> +
> +static int bd9576_wdt_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct device_node *np = dev->parent->of_node;
> +	struct bd9576_wdt_priv *priv;
> +	u32 hw_margin, hw_margin_min;
> +	int ret;
> +
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	platform_set_drvdata(pdev, priv);
> +
> +	priv->dev = dev;
> +	priv->regmap = dev_get_regmap(dev->parent, NULL);
> +	if (!priv->regmap) {
> +		dev_err(dev, "No regmap found\n");
> +		return -ENODEV;
> +	}
> +
> +	priv->gpiod_en = devm_gpiod_get_from_of_node(dev, dev->parent->of_node,
> +						     "rohm,watchdog-enable-gpios",
> +						     0, GPIOD_OUT_LOW,
> +						     "watchdog-enable");
> +	if (IS_ERR(priv->gpiod_en)) {

devm_gpiod_get_from_of_node() can return -EPROBE_DEFER in which case we don't want to see
an error message. I would suggest to use dev_err_probe().

> +		dev_err(dev, "getting watchdog-enable GPIO failed\n");
> +		return PTR_ERR(priv->gpiod_en);
> +	}
> +
> +	priv->gpiod_ping = devm_gpiod_get_from_of_node(dev, dev->parent->of_node,
> +						     "rohm,watchdog-ping-gpios",
> +						     0, GPIOD_OUT_LOW,
> +						     "watchdog-ping");
> +	if (IS_ERR(priv->gpiod_ping)) {
> +		dev_err(dev, "getting watchdog-ping GPIO failed\n");
> +		return PTR_ERR(priv->gpiod_ping);
> +	}

Same as above.

> +
> +	ret = of_property_read_u32(np,
> +				   "hw_margin_ms", &hw_margin);

Line splits are arbitrary. Why is this "hw_margin_ms" and not "rohm,hw_margin_ms" ?

> +	if (ret) {
> +		if (ret != -EINVAL)
> +			return ret;
> +
> +		hw_margin = BD957X_WDT_DEFAULT_MARGIN;
> +	}
> +
> +	ret = of_property_read_u32(np, "rohm,hw-margin-min-ms", &hw_margin_min);
> +	if (ret == -EINVAL)
> +		hw_margin_min = 0;
> +	else if (ret)
> +		return ret;

Please use a single mechanism to handle -EINVAL after of_property_read_u32().

> +
> +	ret = bd957x_set_wdt_mode(priv, hw_margin, hw_margin_min);
> +	if (ret)
> +		return ret;
> +
> +	priv->always_running = of_property_read_bool(np,
> +						     "always-running");

Another arbitrary line split.

> +
> +	watchdog_set_drvdata(&priv->wdd, priv);
> +
> +	priv->wdd.info			= &bd957x_wdt_ident;
> +	priv->wdd.ops			= &bd957x_wdt_ops;
> +	priv->wdd.min_hw_heartbeat_ms	= hw_margin_min;
> +	priv->wdd.max_hw_heartbeat_ms	= hw_margin;
> +	priv->wdd.parent		= dev;
> +	priv->wdd.timeout		= (hw_margin / 2) * 1000;
> +
> +	watchdog_init_timeout(&priv->wdd, 0, dev);
> +	watchdog_set_nowayout(&priv->wdd, nowayout);
> +
> +	watchdog_stop_on_reboot(&priv->wdd);
> +
> +	if (priv->always_running)
> +		bd9576_wdt_start(&priv->wdd);
> +
> +	return devm_watchdog_register_device(dev, &priv->wdd);
> +}
> +
> +static struct platform_driver bd9576_wdt_driver = {
> +	.driver	= {
> +		.name		= "bd9576-wdt",
> +	},
> +	.probe	= bd9576_wdt_probe,
> +};
> +
> +module_platform_driver(bd9576_wdt_driver);
> +
> +MODULE_AUTHOR("Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>");
> +MODULE_DESCRIPTION("ROHM BD9576/BD9573 Watchdog driver");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:bd9576-wdt");
>
Vaittinen, Matti Sept. 18, 2020, 6:06 a.m. UTC | #3
Thanks (again) for review Guenter!

On Thu, 2020-09-17 at 22:45 -0700, Guenter Roeck wrote:
> On 9/17/20 1:03 AM, Matti Vaittinen wrote:
> > Add Watchdog support for ROHM BD9576MUF and BD9573MUF PMICs which
> > are
> > mainly used to power the R-Car series processors. The watchdog is
> > pinged using a GPIO and enabled using another GPIO. Additionally
> > watchdog time-out can be configured to HW prior starting the
> > watchdog.
> > Watchdog timeout can be configured to detect only delayed ping or
> > in
> > a window mode where also too fast pings are detected.
> > 
> > Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
> > ---
> >  drivers/watchdog/Kconfig      |  13 ++
> >  drivers/watchdog/Makefile     |   1 +
> >  drivers/watchdog/bd9576_wdt.c | 295
> > ++++++++++++++++++++++++++++++++++
> >  3 files changed, 309 insertions(+)
> >  create mode 100644 drivers/watchdog/bd9576_wdt.c

> 
> > +
> > +	ret = of_property_read_u32(np,
> > +				   "hw_margin_ms", &hw_margin);
> 
> Line splits are arbitrary. Why is this "hw_margin_ms" and not
> "rohm,hw_margin_ms" ?

"hw_margin_ms" is an existing binding for specifying the maximum TMO in
HW (if I understood it correctly). (It is used at least by the generig
GPIO watchdog) I thought it's better to not invent a new vendor
specific binding when we have a generic one.

https://elixir.bootlin.com/linux/v5.9-rc2/source/Documentation/devicetree/bindings/watchdog/gpio-wdt.txt

> 
> > +	if (ret) {
> > +		if (ret != -EINVAL)
> > +			return ret;
> > +
> > +		hw_margin = BD957X_WDT_DEFAULT_MARGIN;
> > +	}
> > +
> > +	ret = of_property_read_u32(np, "rohm,hw-margin-min-ms",
> > &hw_margin_min);
> > +	if (ret == -EINVAL)
> > +		hw_margin_min = 0;
> > +	else if (ret)
> > +		return ret;
> 
> Please use a single mechanism to handle -EINVAL after
> of_property_read_u32().

Sorry Guenter - I am probably a bit slow today but I am unsure if I
understand the suggestion. Do you mean something like:

hw_margin_min = 0;

ret = of_property_read_u32(np, "rohm,hw-margin-min-ms",
&hw_margin_min);
if (ret && ret != -EINVAL)
	return ret;

Other than that - all findings are clear to me. I'll craft a new
version but I'll wait for a while to see if Lee has time to send some
feedback on MFD so I could get both WDG and MFD fixes to same version
:)

Best Regards
	--Matti


--
Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland
SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND

~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~

Simon says - in Latin please.
"non cogito me" dixit Rene Descarte, deinde evanescavit

(Thanks for the translation Simon)
Guenter Roeck Sept. 19, 2020, 1:49 a.m. UTC | #4
On 9/17/20 11:06 PM, Vaittinen, Matti wrote:
> Thanks (again) for review Guenter!
> 
> On Thu, 2020-09-17 at 22:45 -0700, Guenter Roeck wrote:
>> On 9/17/20 1:03 AM, Matti Vaittinen wrote:
>>> Add Watchdog support for ROHM BD9576MUF and BD9573MUF PMICs which
>>> are
>>> mainly used to power the R-Car series processors. The watchdog is
>>> pinged using a GPIO and enabled using another GPIO. Additionally
>>> watchdog time-out can be configured to HW prior starting the
>>> watchdog.
>>> Watchdog timeout can be configured to detect only delayed ping or
>>> in
>>> a window mode where also too fast pings are detected.
>>>
>>> Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
>>> ---
>>>  drivers/watchdog/Kconfig      |  13 ++
>>>  drivers/watchdog/Makefile     |   1 +
>>>  drivers/watchdog/bd9576_wdt.c | 295
>>> ++++++++++++++++++++++++++++++++++
>>>  3 files changed, 309 insertions(+)
>>>  create mode 100644 drivers/watchdog/bd9576_wdt.c
> 
>>
>>> +
>>> +	ret = of_property_read_u32(np,
>>> +				   "hw_margin_ms", &hw_margin);
>>
>> Line splits are arbitrary. Why is this "hw_margin_ms" and not
>> "rohm,hw_margin_ms" ?
> 
> "hw_margin_ms" is an existing binding for specifying the maximum TMO in
> HW (if I understood it correctly). (It is used at least by the generig
> GPIO watchdog) I thought it's better to not invent a new vendor
> specific binding when we have a generic one.
> 
> https://elixir.bootlin.com/linux/v5.9-rc2/source/Documentation/devicetree/bindings/watchdog/gpio-wdt.txt
> 
>>
>>> +	if (ret) {
>>> +		if (ret != -EINVAL)
>>> +			return ret;
>>> +
>>> +		hw_margin = BD957X_WDT_DEFAULT_MARGIN;
>>> +	}
>>> +
>>> +	ret = of_property_read_u32(np, "rohm,hw-margin-min-ms",
>>> &hw_margin_min);
>>> +	if (ret == -EINVAL)
>>> +		hw_margin_min = 0;
>>> +	else if (ret)
>>> +		return ret;
>>
>> Please use a single mechanism to handle -EINVAL after
>> of_property_read_u32().
> 
> Sorry Guenter - I am probably a bit slow today but I am unsure if I
> understand the suggestion. Do you mean something like:
> 

    if (ret) {
>>> +		if (ret != -EINVAL)
>>> +			return ret;
>>> +
>>> +		hw_margin = BD957X_WDT_DEFAULT_MARGIN;
>>> +	}

vs.

>>> +	if (ret == -EINVAL)
>>> +		hw_margin_min = 0;
>>> +	else if (ret)
>>> +		return ret;

is not very consistent to me.

Guenter

> hw_margin_min = 0;
> 
> ret = of_property_read_u32(np, "rohm,hw-margin-min-ms",
> &hw_margin_min);
> if (ret && ret != -EINVAL)
> 	return ret;
> 
> Other than that - all findings are clear to me. I'll craft a new
> version but I'll wait for a while to see if Lee has time to send some
> feedback on MFD so I could get both WDG and MFD fixes to same version
> :)
> 
> Best Regards
> 	--Matti
> 
> 
> --
> Matti Vaittinen, Linux device drivers
> ROHM Semiconductors, Finland
> SWDC
> Kiviharjunlenkki 1E
> 90220 OULU
> FINLAND
> 
> ~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~
> 
> Simon says - in Latin please.
> "non cogito me" dixit Rene Descarte, deinde evanescavit
> 
> (Thanks for the translation Simon)
>