Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933064AbbKMSCp (ORCPT ); Fri, 13 Nov 2015 13:02:45 -0500 Received: from unicorn.mansr.com ([81.2.72.234]:34202 "EHLO unicorn.mansr.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932432AbbKMSCo convert rfc822-to-8bit (ORCPT ); Fri, 13 Nov 2015 13:02:44 -0500 From: =?iso-8859-1?Q?M=E5ns_Rullg=E5rd?= To: Guenter Roeck Cc: Wim Van Sebroeck , linux-kernel@vger.kernel.org, linux-watchdog@vger.kernel.org Subject: Re: [RESEND][PATCH] watchdog: add support for Sigma Designs SMP86xx References: <1447420459-5301-1-git-send-email-mans@mansr.com> <56461155.80808@roeck-us.net> <5646240C.5070105@roeck-us.net> Date: Fri, 13 Nov 2015 18:02:35 +0000 In-Reply-To: <5646240C.5070105@roeck-us.net> (Guenter Roeck's message of "Fri, 13 Nov 2015 09:55:24 -0800") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Transfer-Encoding: 8BIT Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2824 Lines: 74 Guenter Roeck writes: >>>> +config TANGOX_WDT >>>> + tristate "SMP86xx watchdog" >>>> + select WATCHDOG_CORE >>>> + depends on ARCH_TANGOX >>>> + help >>>> + Watchdog driver for Sigma Designs SMP86xx. >>> >>> Not really; it is for SMP8642, and we don't know if other (later ?) chips >>> will be supported by the same driver. You should be explicit here. More chips >>> can be added later (that would be needed for the devicetree bindings anyway) >>> as they are tested. >> >> I have tested it on SMP8642 and SMP8759. The documentation for SMP8654 >> agrees. >> > > We should have that information somewhere - maybe in the driver header. > It is very useful to know which hardware this was tested with and which > hardware is supposed to work. OK, I'll add that info somewhere. >>>> +static int tangox_wdt_set_timeout(struct watchdog_device *wdt, >>>> + unsigned int new_timeout) >>>> +{ >>>> + struct tangox_wdt_device *dev = watchdog_get_drvdata(wdt); >>>> + >>>> + wdt->timeout = new_timeout; >>>> + dev->timeout = 1 + new_timeout * clk_get_rate(dev->clk); >>> >>> Why "1 +" ? >> >> The counter counts down from the loaded value and asserts the reset pin >> when it reaches 1. Setting it to zero disables the watchdog. > > You might want to explain that somewhere. Maybe use a define, explain > it there, and use the define here and below. Will do. >>>> +static const struct of_device_id tangox_wdt_dt_ids[] = { >>>> + { .compatible = "sigma,smp8642-wdt" }, >>> >>> So this is really for smp8642 only, not for any other chips in the series ? >> >> It's for about a dozen SMP86xx, SMP87xx, and SMP89xx chips. Should I >> list them all? I don't even know where to find a comprehensive list of >> device numbers. >> > I thought so, but I am not a devicetree expert, and I see some "xx" in > existing devicetree bindings. Something to ask when you submit the > bindings to the devicetree mailing list. Either case, I think it would > be either something like "sigma,smp86xx-wdt" or a list of all of them, > but not "sigma,smp8642-wdt" to be used for all chips. The general advice is to not use wildcards in DT bindings since the next chip matching the pattern might not be compatible at all. New chips known to be compatible with an old one can specify both the exact chip and the older one such that existing drivers will work automatically. If at some point they are found not to be compatible after all (hardware bugs, perhaps) a fixed driver will work with existing device trees. Thanks for the review. -- M?ns Rullg?rd mans@mansr.com -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/