From: Guenter Roeck <linux@roeck-us.net>
To: Leela Krishna Amudala <l.krishna@samsung.com>
Cc: linux-samsung-soc <linux-samsung-soc@vger.kernel.org>,
Kukjin Kim <kgene.kim@samsung.com>,
Wim Van Sebroeck <wim@iguana.be>,
Tomasz Figa <t.figa@samsung.com>,
devicetree@vger.kernel.org,
Douglas Anderson <dianders@chromium.org>,
linux-watchdog@vger.kernel.org, cpgs@samsung.com,
Sachin Kamat <sachin.kamat@linaro.org>
Subject: Re: [PATCH V9 2/3] watchdog: s3c2410_wdt: use syscon regmap interface to configure pmu register
Date: Mon, 18 Nov 2013 23:02:30 -0800 [thread overview]
Message-ID: <528B0D06.9080903@roeck-us.net> (raw)
In-Reply-To: <CAL1wa8eXDfP3Swfx-LmNxt7MJBAjFYKHuQmiioX+Vs9aJW8giA@mail.gmail.com>
On 11/18/2013 09:26 PM, Leela Krishna Amudala wrote:
> Hi Guenter Roeck,
>
> On Tue, Nov 19, 2013 at 10:30 AM, Guenter Roeck <linux@roeck-us.net> wrote:
>> On 11/18/2013 08:36 PM, Leela Krishna Amudala wrote:
>>>
>>> Hi Guenter Roeck,
>>>
>>> Thanks for reviewing the patch.
>>>
>>>
>>> On Mon, Nov 18, 2013 at 10:12 PM, Guenter Roeck <linux@roeck-us.net>
>>> wrote:
>>>>
>>>> On Mon, Nov 18, 2013 at 03:19:48PM +0530, Leela Krishna Amudala wrote:
>>>>>
>>>>> Add device tree support for exynos5250 and 5420 SoCs and use syscon
>>>>> regmap interface
>>>>> to configure AUTOMATIC_WDT_RESET_DISABLE and MASK_WDT_RESET_REQUEST
>>>>> registers of PMU
>>>>> to mask/unmask enable/disable of watchdog in probe and s2r scenarios.
>>>>>
>>>>> Signed-off-by: Leela Krishna Amudala <l.krishna@samsung.com>
>>>>> ---
>>>>> .../devicetree/bindings/watchdog/samsung-wdt.txt | 21 ++-
>>>>> drivers/watchdog/Kconfig | 1 +
>>>>> drivers/watchdog/s3c2410_wdt.c | 145
>>>>> ++++++++++++++++++--
>>>>> 3 files changed, 157 insertions(+), 10 deletions(-)
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/watchdog/samsung-wdt.txt
>>>>> b/Documentation/devicetree/bindings/watchdog/samsung-wdt.txt
>>>>> index 2aa486c..5dea363 100644
>>>>> --- a/Documentation/devicetree/bindings/watchdog/samsung-wdt.txt
>>>>> +++ b/Documentation/devicetree/bindings/watchdog/samsung-wdt.txt
>>>>> @@ -5,10 +5,29 @@ after a preset amount of time during which the WDT
>>>>> reset event has not
>>>>> occurred.
>>>>>
>>>>> Required properties:
>>>>> -- compatible : should be "samsung,s3c2410-wdt"
>>>>> +- compatible : should be one among the following
>>>>> + (a) "samsung,s3c2410-wdt" for Exynos4 and previous SoCs
>>>>> + (b) "samsung,exynos5250-wdt" for Exynos5250
>>>>> + (c) "samsung,exynos5420-wdt" for Exynos5420
>>>>> +
>>>>> - reg : base physical address of the controller and length of memory
>>>>> mapped
>>>>> region.
>>>>> - interrupts : interrupt number to the cpu.
>>>>> +- samsung,syscon-phandle : reference to syscon node (This property
>>>>> required only
>>>>> + in case of compatible being "samsung,exynos5250-wdt" or
>>>>> "samsung,exynos5420-wdt".
>>>>> + In case of Exynos5250 and 5420 this property points to syscon node
>>>>> holding the PMU
>>>>> + base address)
>>>>>
>>>>> Optional properties:
>>>>> - timeout-sec : contains the watchdog timeout in seconds.
>>>>> +
>>>>> +Example:
>>>>> +
>>>>> +watchdog@101D0000 {
>>>>> + compatible = "samsung,exynos5250-wdt";
>>>>> + reg = <0x101D0000 0x100>;
>>>>> + interrupts = <0 42 0>;
>>>>> + clocks = <&clock 336>;
>>>>> + clock-names = "watchdog";
>>>>> + samsung,syscon-phandle = <&pmu_sys_reg>;
>>>>> +};
>>>>> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
>>>>> index d1d53f3..0d272ae 100644
>>>>> --- a/drivers/watchdog/Kconfig
>>>>> +++ b/drivers/watchdog/Kconfig
>>>>> @@ -188,6 +188,7 @@ config S3C2410_WATCHDOG
>>>>> tristate "S3C2410 Watchdog"
>>>>> depends on HAVE_S3C2410_WATCHDOG
>>>>> select WATCHDOG_CORE
>>>>> + select MFD_SYSCON if ARCH_EXYNOS5
>>>>> help
>>>>> Watchdog timer block in the Samsung SoCs. This will reboot
>>>>> the system when the timer expires with the watchdog enabled.
>>>>> diff --git a/drivers/watchdog/s3c2410_wdt.c
>>>>> b/drivers/watchdog/s3c2410_wdt.c
>>>>> index 23aad7c..42e0fd3 100644
>>>>> --- a/drivers/watchdog/s3c2410_wdt.c
>>>>> +++ b/drivers/watchdog/s3c2410_wdt.c
>>>>> @@ -41,6 +41,8 @@
>>>>> #include <linux/slab.h>
>>>>> #include <linux/err.h>
>>>>> #include <linux/of.h>
>>>>> +#include <linux/mfd/syscon.h>
>>>>> +#include <linux/regmap.h>
>>>>>
>>>>> #define S3C2410_WTCON 0x00
>>>>> #define S3C2410_WTDAT 0x04
>>>>> @@ -61,6 +63,10 @@
>>>>> #define CONFIG_S3C2410_WATCHDOG_ATBOOT (0)
>>>>> #define CONFIG_S3C2410_WATCHDOG_DEFAULT_TIME (15)
>>>>>
>>>>> +#define WDT_DISABLE_REG_OFFSET 0x0408
>>>>> +#define WDT_MASK_RESET_REG_OFFSET 0x040c
>>>>> +#define QUIRK_NEEDS_PMU_CONFIG (1 << 0)
>>>>> +
>>>>> static bool nowayout = WATCHDOG_NOWAYOUT;
>>>>> static int tmr_margin;
>>>>> static int tmr_atboot = CONFIG_S3C2410_WATCHDOG_ATBOOT;
>>>>> @@ -84,6 +90,13 @@ MODULE_PARM_DESC(soft_noboot, "Watchdog action, set
>>>>> to 1 to ignore reboots, "
>>>>> "0 to reboot (default 0)");
>>>>> MODULE_PARM_DESC(debug, "Watchdog debug, set to >1 for debug (default
>>>>> 0)");
>>>>>
>>>>> +struct s3c2410_wdt_variant {
>>>>> + int disable_reg;
>>>>> + int mask_reset_reg;
>>>>> + int mask_bit;
>>>>> + u32 quirks;
>>>>> +};
>>>>> +
>>>>> struct s3c2410_wdt {
>>>>> struct device *dev;
>>>>> struct clk *clock;
>>>>> @@ -94,7 +107,49 @@ struct s3c2410_wdt {
>>>>> unsigned long wtdat_save;
>>>>> struct watchdog_device wdt_device;
>>>>> struct notifier_block freq_transition;
>>>>> + struct s3c2410_wdt_variant *drv_data;
>>>>> + struct regmap *pmureg;
>>>>> +};
>>>>> +
>>>>> +static const struct s3c2410_wdt_variant drv_data_s3c2410 = {
>>>>> + .quirks = 0
>>>>> +};
>>>>> +
>>>>> +#ifdef CONFIG_OF
>>>>> +static const struct s3c2410_wdt_variant drv_data_exynos5250 = {
>>>>> + .disable_reg = WDT_DISABLE_REG_OFFSET,
>>>>> + .mask_reset_reg = WDT_MASK_RESET_REG_OFFSET,
>>>>> + .mask_bit = 20,
>>>>> + .quirks = QUIRK_NEEDS_PMU_CONFIG
>>>>> +};
>>>>> +
>>>>> +static const struct s3c2410_wdt_variant drv_data_exynos5420 = {
>>>>> + .disable_reg = WDT_DISABLE_REG_OFFSET,
>>>>> + .mask_reset_reg = WDT_MASK_RESET_REG_OFFSET,
>>>>> + .mask_bit = 0,
>>>>> + .quirks = QUIRK_NEEDS_PMU_CONFIG
>>>>> +};
>>>>> +
>>>>> +static const struct of_device_id s3c2410_wdt_match[] = {
>>>>> + { .compatible = "samsung,s3c2410-wdt",
>>>>> + .data = &drv_data_s3c2410 },
>>>>> + { .compatible = "samsung,exynos5250-wdt",
>>>>> + .data = &drv_data_exynos5250 },
>>>>> + { .compatible = "samsung,exynos5420-wdt",
>>>>> + .data = &drv_data_exynos5420 },
>>>>> + {},
>>>>> };
>>>>> +MODULE_DEVICE_TABLE(of, s3c2410_wdt_match);
>>>>> +#endif
>>>>> +
>>>>> +static const struct platform_device_id s3c2410_wdt_ids[] = {
>>>>> + {
>>>>> + .name = "s3c2410-wdt",
>>>>> + .driver_data = (unsigned long)&drv_data_s3c2410,
>>>>> + },
>>>>> + {}
>>>>> +};
>>>>> +MODULE_DEVICE_TABLE(platform, s3c2410_wdt_ids);
>>>>>
>>>>> /* watchdog control routines */
>>>>>
>>>>> @@ -111,6 +166,26 @@ static inline struct s3c2410_wdt
>>>>> *freq_to_wdt(struct notifier_block *nb)
>>>>> return container_of(nb, struct s3c2410_wdt, freq_transition);
>>>>> }
>>>>>
>>>>> +static int s3c2410wdt_mask_and_disable_reset(struct s3c2410_wdt *wdt,
>>>>> bool mask)
>>>>> +{
>>>>> + int ret;
>>>>> + u32 mask_val = 1 << wdt->drv_data->mask_bit;
>>>>> + u32 val = 0;
>>>>> +
>>>>> + if (mask)
>>>>> + val = mask_val;
>>>>> +
>>>>> + ret = regmap_update_bits(wdt->pmureg,
>>>>> + wdt->drv_data->disable_reg,
>>>>> + mask_val, val);
>>>>> + if (ret < 0)
>>>>> + return ret;
>>>>> +
>>>>> + return regmap_update_bits(wdt->pmureg,
>>>>> + wdt->drv_data->mask_reset_reg,
>>>>> + mask_val, val);
>>>>> +}
>>>>> +
>>>>> static int s3c2410wdt_keepalive(struct watchdog_device *wdd)
>>>>> {
>>>>> struct s3c2410_wdt *wdt = watchdog_get_drvdata(wdd);
>>>>> @@ -332,6 +407,20 @@ static inline void
>>>>> s3c2410wdt_cpufreq_deregister(struct s3c2410_wdt *wdt)
>>>>> }
>>>>> #endif
>>>>>
>>>>> +/* s3c2410_get_wdt_driver_data */
>>>>> +static inline struct s3c2410_wdt_variant *
>>>>> +get_wdt_drv_data(struct platform_device *pdev)
>>>>> +{
>>>>> + if (pdev->dev.of_node) {
>>>>> + const struct of_device_id *match;
>>>>> + match = of_match_node(s3c2410_wdt_match,
>>>>> pdev->dev.of_node);
>>>>> + return (struct s3c2410_wdt_variant *)match->data;
>>>>> + } else {
>>>>> + return (struct s3c2410_wdt_variant *)
>>>>> + platform_get_device_id(pdev)->driver_data;
>>>>> + }
>>>>> +}
>>>>> +
>>>>> static int s3c2410wdt_probe(struct platform_device *pdev)
>>>>> {
>>>>> struct device *dev;
>>>>> @@ -354,6 +443,16 @@ static int s3c2410wdt_probe(struct platform_device
>>>>> *pdev)
>>>>> spin_lock_init(&wdt->lock);
>>>>> wdt->wdt_device = s3c2410_wdd;
>>>>>
>>>>> + wdt->drv_data = get_wdt_drv_data(pdev);
>>>>> + if (wdt->drv_data->quirks & QUIRK_NEEDS_PMU_CONFIG) {
>>>>> + wdt->pmureg =
>>>>> syscon_regmap_lookup_by_phandle(dev->of_node,
>>>>> +
>>>>> "samsung,syscon-phandle");
>>>>> + if (IS_ERR(wdt->pmureg)) {
>>>>> + dev_err(dev, "syscon regmap lookup failed.\n");
>>>>> + return PTR_ERR(wdt->pmureg);
>>>>> + }
>>>>> + }
>>>>> +
>>>>> wdt_irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
>>>>> if (wdt_irq == NULL) {
>>>>> dev_err(dev, "no irq resource specified\n");
>>>>> @@ -444,6 +543,14 @@ static int s3c2410wdt_probe(struct platform_device
>>>>> *pdev)
>>>>> (wtcon & S3C2410_WTCON_RSTEN) ? "en" : "dis",
>>>>> (wtcon & S3C2410_WTCON_INTEN) ? "en" : "dis");
>>>>>
>>>>> + if (wdt->drv_data->quirks & QUIRK_NEEDS_PMU_CONFIG) {
>>>>> + ret = s3c2410wdt_mask_and_disable_reset(wdt, false);
>>>>> + if (ret < 0) {
>>>>> + dev_err(wdt->dev, "failed to update pmu
>>>>> register");
>>>>
>>>>
>>>> Hmm ... still not happy ;-). This message is the same for each call
>>>> to s3c2410wdt_mask_and_disable_reset(). Why not create it there ?
>>>> Sure, you'd have to decide if you want to use dev_warn() or dev_err(),
>>>> but that would still be better than repeating the same message (and code)
>>>> five times.
>>>>
>>>> The same is true for the if() statement. It might be worthwhile calling
>>>> s3c2410wdt_mask_and_disable_reset() unconditionally and adding something
>>>> like
>>>>
>>>> if (!(wdt->drv_data->quirks & QUIRK_NEEDS_PMU_CONFIG))
>>>> return 0;
>>>>
>>>> to it.
>>>>
>>>
>>> As Tomasz Figa suggested I handled the error conditions here
>>> Please go through this link for your reference
>>> https://patchwork.kernel.org/patch/3118771/
>>>
>>
>> His proposed function had the error message in the function,
>> so I am not entirely following your logic.
>>
>> He said you should _handle_ the error condition in the calling code.
>> Dumping an error message and doing nothing isn't really "handling"
>> the error condition.
>>
>
> I know printing an error message is not really "handling" the error condition.
> But apart from probe function I don't have anything to handle in remove,
> shutdown, suspend and resume functions.
> In remove, shutdown, suspend funtions I must do stop/deregister
> the device even if regmap_update_bits fails so I simply do dev_warn
> and continuing
>
> So I removed the error message prints in the s3c2410wdt_mask_and_disable_reset
> function and added it in the above mentioned functions as part of
> error handling.
>
That doesn't make sense to me. I'll leave it up to Wim to decide how to handle this patch.
Guenter
next prev parent reply other threads:[~2013-11-19 7:02 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-11-18 9:49 [PATCH V9 0/3] Add watchdog DT nodes and use syscon regmap interface to configure pmu registers Leela Krishna Amudala
2013-11-18 9:49 ` [PATCH V9 1/3] ARM: dts: Add pmu sysreg node to exynos5250 and exynos5420 dtsi files Leela Krishna Amudala
2013-11-25 22:24 ` Doug Anderson
2013-11-18 9:49 ` [PATCH V9 2/3] watchdog: s3c2410_wdt: use syscon regmap interface to configure pmu register Leela Krishna Amudala
2013-11-18 16:42 ` Guenter Roeck
2013-11-19 4:36 ` Leela Krishna Amudala
2013-11-19 5:00 ` Guenter Roeck
2013-11-19 5:26 ` Leela Krishna Amudala
2013-11-19 7:02 ` Guenter Roeck [this message]
2013-11-25 22:44 ` Doug Anderson
2013-11-26 0:12 ` Tomasz Figa
2013-11-26 12:31 ` Leela Krishna Amudala
2013-11-18 9:49 ` [PATCH V9 3/3] ARM: dts: update watchdog device nodes for Exynos5250 and Exynos5420 Leela Krishna Amudala
[not found] ` <1384768189-2839-4-git-send-email-l.krishna-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
2013-11-25 22:46 ` Doug Anderson
2013-11-18 11:05 ` [PATCH V9 0/3] Add watchdog DT nodes and use syscon regmap interface to configure pmu registers Tomasz Figa
2013-11-21 5:19 ` Leela Krishna Amudala
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=528B0D06.9080903@roeck-us.net \
--to=linux@roeck-us.net \
--cc=cpgs@samsung.com \
--cc=devicetree@vger.kernel.org \
--cc=dianders@chromium.org \
--cc=kgene.kim@samsung.com \
--cc=l.krishna@samsung.com \
--cc=linux-samsung-soc@vger.kernel.org \
--cc=linux-watchdog@vger.kernel.org \
--cc=sachin.kamat@linaro.org \
--cc=t.figa@samsung.com \
--cc=wim@iguana.be \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).