Re: [PATCH V8 2/3] watchdog: s3c2410_wdt: add device tree support and use syscon regmap interface to configure pmu register

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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-samsung-soc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Linux SoC Development]     [Linux Rockchip Development]     [Linux USB Development]     [Video for Linux]     [Linux Audio Users]     [Linux SCSI]     [Yosemite News]

  Powered by Linux