From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751007AbdKESeU (ORCPT ); Sun, 5 Nov 2017 13:34:20 -0500 Received: from mail-pg0-f65.google.com ([74.125.83.65]:47087 "EHLO mail-pg0-f65.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750731AbdKESeT (ORCPT ); Sun, 5 Nov 2017 13:34:19 -0500 X-Google-Smtp-Source: ABhQp+QsBalfHfBT49pJPZJAa3bxS026NBSvuDis596lkdoR+da2hkNPLWpGGte0WZLkld1GW7CrKw== Subject: Re: [PATCH v10 4/5] watchdog: Add RAVE SP watchdog driver To: Johan Hovold , Andrey Smirnov Cc: linux-kernel@vger.kernel.org, linux-watchdog@vger.kernel.org, cphealy@gmail.com, Lucas Stach , Nikita Yushchenko , Lee Jones , Greg Kroah-Hartman , Pavel Machek , Andy Shevchenko , Rob Herring , Sebastian Reichel References: <20171031163656.24552-1-andrew.smirnov@gmail.com> <20171031163656.24552-5-andrew.smirnov@gmail.com> <20171105154725.GA11226@localhost> From: Guenter Roeck Message-ID: <2a09a907-cc64-232d-95c3-56da9ae05f4a@roeck-us.net> Date: Sun, 5 Nov 2017 10:34:15 -0800 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.4.0 MIME-Version: 1.0 In-Reply-To: <20171105154725.GA11226@localhost> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 11/05/2017 07:47 AM, Johan Hovold wrote: > On Tue, Oct 31, 2017 at 09:36:55AM -0700, Andrey Smirnov wrote: >> This driver provides access to RAVE SP watchdog functionality. >> >> Cc: linux-kernel@vger.kernel.org >> Cc: linux-watchdog@vger.kernel.org >> Cc: cphealy@gmail.com >> Cc: Lucas Stach >> Cc: Nikita Yushchenko >> Cc: Lee Jones >> Cc: Greg Kroah-Hartman >> Cc: Pavel Machek >> Cc: Andy Shevchenko >> Cc: Guenter Roeck >> Cc: Rob Herring >> Cc: Johan Hovold >> Cc: Sebastian Reichel >> Signed-off-by: Nikita Yushchenko >> Signed-off-by: Andrey Smirnov > >> +static const struct of_device_id rave_sp_wdt_of_match[] = { >> + { .compatible = "zii,rave-sp-watchdog" }, >> + {} >> +}; > >> +static const struct of_device_id rave_sp_wdt_variants[] = { >> + { .compatible = COMPATIBLE_RAVE_SP_NIU, .data = &rave_sp_wdt_legacy }, >> + { .compatible = COMPATIBLE_RAVE_SP_MEZZ, .data = &rave_sp_wdt_legacy }, >> + { .compatible = COMPATIBLE_RAVE_SP_ESB, .data = &rave_sp_wdt_legacy }, >> + { .compatible = COMPATIBLE_RAVE_SP_RDU1, .data = &rave_sp_wdt_rdu }, >> + { .compatible = COMPATIBLE_RAVE_SP_RDU2, .data = &rave_sp_wdt_rdu }, >> + { /* sentinel */ } >> +}; >> + >> +static int rave_sp_wdt_probe(struct platform_device *pdev) >> +{ >> + struct device *dev = &pdev->dev; >> + const struct of_device_id *id; >> + struct watchdog_device *wdd; >> + struct rave_sp_wdt *sp_wd; >> + struct nvmem_cell *cell; >> + __le16 timeout = 0; >> + int ret; >> + >> + id = of_match_device(rave_sp_wdt_variants, dev->parent); > > So as I already mentioned in a comment on an earlier version of the MFD > driver, I find this matching on the parent of_node to be an odd > pattern, which also does not seem to have any precedent in mainline. > > It seems you really should be using two compatible strings for the > watchdog (for the legacy and rdu variants) rather than keep an entry for > every parent compatible-string in every child/cell driver (which all > need to be kept in sync). > > But let's see what Rob and Lee says about this. > >> + if (!id) { >> + dev_err(dev, "Unknown parent device variant. Bailing out\n"); >> + return -ENODEV; >> + } >> + >> + sp_wd = devm_kzalloc(dev, sizeof(*sp_wd), GFP_KERNEL); >> + if (!sp_wd) >> + return -ENOMEM; >> + >> + sp_wd->variant = id->data; >> + sp_wd->sp = dev_get_drvdata(dev->parent); >> + >> + wdd = &sp_wd->wdd; >> + wdd->parent = dev; >> + wdd->info = &rave_sp_wdt_info; >> + wdd->ops = &rave_sp_wdt_ops; >> + wdd->min_timeout = sp_wd->variant->min_timeout; >> + wdd->max_timeout = sp_wd->variant->max_timeout; >> + wdd->status = WATCHDOG_NOWAYOUT_INIT_STATUS; >> + wdd->timeout = 60; >> + >> + cell = nvmem_cell_get(dev, "wdt-timeout"); >> + if (!IS_ERR(cell)) { >> + size_t len; >> + void *value = nvmem_cell_read(cell, &len); >> + >> + if (!IS_ERR(value)) { >> + memcpy(&timeout, value, min(len, sizeof(timeout))); >> + kfree(value); >> + } >> + nvmem_cell_put(cell); >> + } >> + watchdog_init_timeout(wdd, le16_to_cpu(timeout), dev); >> + watchdog_set_restart_priority(wdd, 255); >> + >> + sp_wd->reboot_notifier.notifier_call = rave_sp_wdt_reboot_notifier; >> + ret = devm_register_reboot_notifier(dev, &sp_wd->reboot_notifier); >> + if (ret) { >> + dev_err(dev, "Failed to register reboot notifier\n"); >> + return ret; >> + } >> + >> + /* >> + * We don't know if watchdog is running now. To be sure, let's >> + * start it and depend on watchdog core to ping it >> + */ >> + wdd->max_hw_heartbeat_ms = wdd->max_timeout * 1000; >> + ret = rave_sp_wdt_start(wdd); >> + if (ret) { >> + dev_err(dev, "Watchdog didn't start\n"); >> + return ret; >> + } >> + >> + return devm_watchdog_register_device(dev, wdd); > > What about stopping the watchdog on errors here? > > And does watchdog core take care of calling stop() on unregister (i.e. > at unbind)? > Both good points. This needs watchdog_stop_on_unregister(), and the probe function needs to stop the watchdog if registration fails. Guenter >> +} >> + >> +static struct platform_driver rave_sp_wdt_driver = { >> + .probe = rave_sp_wdt_probe, >> + .driver = { >> + .name = KBUILD_MODNAME, >> + .of_match_table = rave_sp_wdt_of_match, >> + }, >> +}; > > Johan > -- > To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html >