Hi Tomasz On Sat, Nov 16, 2013 at 5:27 AM, Tomasz Figa <tomasz.figa@xxxxxxxxx> wrote: > On Thursday 14 of November 2013 18:49:34 Guenter Roeck wrote: >> On 11/11/2013 10:34 PM, Leela Krishna Amudala wrote: >> > Add device tree support 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. >> > >> If the driver didn't support devicetree before, why did it have #ifdef CONFIG_OF, and >> why does its devicetree bindings file already exist ? >> >> Seems to me the description does not match the patch; it appears that you are really >> adding Exynos5 support to the driver. > > Yes, the driver did support Device Tree before. Patch description needs > to be corrected. > Okay, modified the patch description >> > diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c >> > index 23aad7c..ccf755d 100644 >> > --- a/drivers/watchdog/s3c2410_wdt.c >> > +++ b/drivers/watchdog/s3c2410_wdt.c > [snip] >> > + >> > +static const struct platform_device_id s3c2410_wdt_ids[] = { >> > + { >> > + .name = "s3c2410-wdt", >> > + .driver_data = (unsigned long)&pmu_config_s3c2410, >> > + }, >> > + {} >> > +}; >> > +MODULE_DEVICE_TABLE(platform, s3c2410_wdt_ids); >> > + >> >> What is this second device ID and MODULE_DEVICE_TABLE for ? >> I assume it is to provide driver_data. If so, you should probably remove the >> platform MODULE_ALIAS at the end of the driver. > > Yes. I suggested adding it to simplify the code a bit by having driver > data supplied even when booting without DT. Good catch with MODULE_ALIAS, > though. > Okay, Deleted MODULE_ALIAS and tested non-dt case also on 5420 SoC >> >> > /* watchdog control routines */ >> > >> > #define DBG(fmt, ...) \ >> > @@ -111,6 +166,30 @@ 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->pmu_config->mask_bit; >> > + u32 val = 0; >> > + >> > + if (mask) >> > + val = mask_val; >> > + >> > + ret = regmap_update_bits(wdt->pmureg, >> > + wdt->pmu_config->disable_reg, >> > + mask_val, val); >> > + if (ret < 0) >> > + return ret; >> > + >> > + ret = regmap_update_bits(wdt->pmureg, >> > + wdt->pmu_config->mask_reset_reg, >> > + mask_val, val); >> > + if (ret < 0) >> > + return ret; >> > + >> > + return 0; >> >> The above code is identical to >> return ret; > > I'd even say that the above code is identical to > > return regmap_update_bits(wdt->pmureg, > wdt->pmu_config->mask_reset_reg, > mask_val, val); > Okay, looks fine changed it >> >> > +} >> > + >> > static int s3c2410wdt_keepalive(struct watchdog_device *wdd) >> > { >> > struct s3c2410_wdt *wdt = watchdog_get_drvdata(wdd); >> > @@ -332,6 +411,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 +447,16 @@ static int s3c2410wdt_probe(struct platform_device *pdev) >> > spin_lock_init(&wdt->lock); >> > wdt->wdt_device = s3c2410_wdd; >> > >> > + wdt->pmu_config = get_wdt_drv_data(pdev); >> > + if (wdt->pmu_config->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 +547,14 @@ static int s3c2410wdt_probe(struct platform_device *pdev) >> > (wtcon & S3C2410_WTCON_RSTEN) ? "en" : "dis", >> > (wtcon & S3C2410_WTCON_INTEN) ? "en" : "dis"); >> > >> > + if (wdt->pmu_config->quirks & QUIRK_NEEDS_PMU_CONFIG) { >> >> Might be easier to check for wdt->pmureg in all those secondary conditionals. > > IMHO not really much easier and less meaningful. Furthermore, in case of > other quirks showing up in future that wouldn't have alternative way > of checking, they will have consistent style of checks. > Yes, Still used if (wdt->pmu_config->quirks & QUIRK_NEEDS_PMU_CONFIG) for conditional checks. > However, I think I missed to point out the ugly field name in my previous > reviews. Instead of calling it "pmu_config", "drv_data" would be much > better. > Okay, changed from "pmu_config" to "drv_data" >> >> > + ret = s3c2410wdt_mask_and_disable_reset(wdt, false); >> > + if (ret < 0) { >> > + dev_warn(wdt->dev, "failed to update pmu register"); >> >> Is this a warning or an error ? >> >> If it is (only) a warning, why do you bail out ? >> >> On a side note, I am not sure if all those errors can really happen in practice. >> Becasue if not, all you do is to increase code size with no real benefit. > > Yes, that's a good point. I was under impression that returned error codes > should not be ignored, but maybe I was a bit too strict over this. > > Generally on currently supported platforms that will end up as a simple > MMIO register access locked with a spinlock, so really no way to fail. > > Anyway, a failure to write these PMU registers would end up with the > watchdog not generating the reset, so IMHO it would be an error. > Yes, changed it to dev_err and kept the dev_warn in other functions as it is Best Wishes, Leela Krishna. > Best regards, > Tomasz > > -- > To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in > the body of a message to majordomo@xxxxxxxxxxxxxxx > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html