Hi Leela,
Thanks for addressing my comments for previous version. However now this
won't even build when CONFIG_OF is disabled. See my comments inline.
On Monday 11 of November 2013 18:14:57 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.
>
> Signed-off-by: Leela Krishna Amudala <l.krishna@xxxxxxxxxxx>
> ---
> .../devicetree/bindings/watchdog/samsung-wdt.txt | 21 ++-
> drivers/watchdog/Kconfig | 1 +
> drivers/watchdog/s3c2410_wdt.c | 154 +++++++++++++++++++-
> 3 files changed, 167 insertions(+), 9 deletions(-)
[snip]
> diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c
> index 23aad7c..b57ccae 100644
> --- a/drivers/watchdog/s3c2410_wdt.c
> +++ b/drivers/watchdog/s3c2410_wdt.c
[snip]
> @@ -94,8 +107,56 @@ struct s3c2410_wdt {
> unsigned long wtdat_save;
> struct watchdog_device wdt_device;
> struct notifier_block freq_transition;
> + struct s3c2410_wdt_variant *pmu_config;
> + struct regmap *pmureg;
> +};
> +
> +#ifdef CONFIG_OF
> +static const struct s3c2410_wdt_variant pmu_config_s3c2410 = {
> + .quirks = 0
> +};
> +
> +static const struct s3c2410_wdt_variant pmu_config_5250 = {
> + .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 pmu_config_5420 = {
> + .disable_reg = WDT_DISABLE_REG_OFFSET,
> + .mask_reset_reg = WDT_MASK_RESET_REG_OFFSET,
> + .mask_bit = 0,
> + .quirks = QUIRK_NEEDS_PMU_CONFIG
> +};
The three variant data structs above are under #ifdef CONFIG_OF, while
they are always needed for the platform match table.
However, since Exynos supports only DT-based device instantation,
I would move only the basic pmu_config_s3c2410 out of the ifdef and
limit the platform match table only to this single variant.
> +
> +static const struct of_device_id s3c2410_wdt_match[] = {
> + { .compatible = "samsung,s3c2410-wdt",
> + .data = &pmu_config_s3c2410 },
> + { .compatible = "samsung,exynos5250-wdt",
> + .data = &pmu_config_5250 },
> + { .compatible = "samsung,exynos5420-wdt",
> + .data = &pmu_config_5420 },
> + {},
> +};
> +MODULE_DEVICE_TABLE(of, s3c2410_wdt_match);
> +#endif
> +
> +static const struct platform_device_id s3c_wdt_driver_ids[] = {
nit: For consistency with other names in this file, what about
calling this s3c2410_wdt_ids[] instead?
> + {
> + .name = "s3c2410-wdt",
> + .driver_data = (unsigned long)&pmu_config_s3c2410,
> + }, {
> + .name = "exynos5250-wdt",
> + .driver_data = (unsigned long)&pmu_config_5250,
> + }, {
> + .name = "exynos5420-wdt",
> + .driver_data = (unsigned long)&pmu_config_5420,
> + },
So, generally, you don't need the two Exynos variants above in this array.
Otherwise, the patch is fine.
Best regards,
Tomasz
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
