From owner-freebsd-current@FreeBSD.ORG Wed Jun 29 12:20:46 2011 Return-Path: Delivered-To: freebsd-current@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 8984B106564A; Wed, 29 Jun 2011 12:20:46 +0000 (UTC) (envelope-from jhb@freebsd.org) Received: from cyrus.watson.org (cyrus.watson.org [65.122.17.42]) by mx1.freebsd.org (Postfix) with ESMTP id 604198FC15; Wed, 29 Jun 2011 12:20:46 +0000 (UTC) Received: from bigwig.baldwin.cx (66.111.2.69.static.nyinternet.net [66.111.2.69]) by cyrus.watson.org (Postfix) with ESMTPSA id 0F0D946B1A; Wed, 29 Jun 2011 08:20:46 -0400 (EDT) Received: from jhbbsd.localnet (unknown [209.249.190.124]) by bigwig.baldwin.cx (Postfix) with ESMTPSA id 90BE08A01F; Wed, 29 Jun 2011 08:20:45 -0400 (EDT) From: John Baldwin To: freebsd-current@freebsd.org Date: Wed, 29 Jun 2011 08:11:34 -0400 User-Agent: KMail/1.13.5 (FreeBSD/8.2-CBSD-20110617; KDE/4.5.5; amd64; ; ) References: <4E0A5689.2020302@delphij.net> <4E0AEED2.7060702@FreeBSD.org> In-Reply-To: <4E0AEED2.7060702@FreeBSD.org> MIME-Version: 1.0 Content-Type: Text/Plain; charset="utf-8" Content-Transfer-Encoding: 7bit Message-Id: <201106290811.34443.jhb@freebsd.org> X-Greylist: Sender succeeded SMTP AUTH, not delayed by milter-greylist-4.2.6 (bigwig.baldwin.cx); Wed, 29 Jun 2011 08:20:45 -0400 (EDT) Cc: Xin LI , d@delphij.net, Andriy Gapon Subject: Re: [RFC] winbond watchdog driver for FreeBSD/i386 and FreeBSD/amd64 X-BeenThere: freebsd-current@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Discussions about the use of FreeBSD-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 29 Jun 2011 12:20:46 -0000 On Wednesday, June 29, 2011 5:22:26 am Andriy Gapon wrote: > on 29/06/2011 01:32 Xin LI said the following: > > +/* > > + * Look for Winbond device. > > + */ > > +static void > > +winbondwd_identify(driver_t *driver, device_t parent) > > +{ > > + unsigned int baseport; > > + device_t dev; > > + > > + if ((dev = device_find_child(parent, driver->name, 0)) == NULL) { > > + if (resource_int_value("winbondwd", 0, "baseport", &baseport) != 0) { > > + baseport = winbondwd_baseport_probe(); > > + if (baseport == (unsigned int)(-1)) { > > + printf("winbondwd0: Compatible Winbond Super I/O not found.\n"); > > + return; > > + } > > + } > > + > > + dev = BUS_ADD_CHILD(parent, 0, driver->name, 0); > > + > > + bus_set_resource(dev, SYS_RES_IOPORT, 0, baseport, 2); > > + } > > + > > + if (dev == NULL) > > + return; > > These last two lines are redundant? > > Also, maybe I am confused, but I think that in ISA identify method you don't > actually need to parse any hints/tunables. That is, you can use standard hints > approach like e.g.: > hint.winbondwd.0.at="isa" > hint.winbondwd.0.port="0x3F0" > and ISA will automatically add a winbondwd child with an I/O port resource at > 0x3F0. The identify method should only add a child for a no-hints/auto-probing > case. > E.g. see boiler-plate code for the ISA case in > share/examples/drivers/make_device_driver.sh, especially the comments. > > I am not saying that your approach won't work (apparently it does) or that it is > inherently bad. It just seems to be different from how other ISA drivers do > their identify+probe dance. I agree, it should probably look something like this: { if (device_find_child(parent, driver->name, 0) != NULL) return; if (resource_int_value(driver->name, 0, "port", &baseport) == 0) return; baseport = winbondwd_baseport_probe(); if (baseport == -1) /* No reason to warn on every boot here. */ return; dev = BUS_ADD_CHILD(parent, 0, driver->name, 0); if (dev != NULL) bus_set_resource(dev, SYS_RES_IOPORT, 0, baseport, 2); } > > + sc->wb_bst = rman_get_bustag(sc->wb_res); > > + sc->wb_bsh = rman_get_bushandle(sc->wb_res); Please don't use these. bus_read_X(sc->wb_wres) is easier on the eyes. One other comment, several places in the code are using various magic numbers for register offsets, bit field values, etc. For example: + winbondwd_write_reg(sc, /* LDN bit 7:1 */ 0x7, /* GPIO Port 2 */ 0x8); + winbondwd_write_reg(sc, /* CR30 */ 0x30, /* Activate */ 0x1); Could you add a winbondwdreg.h header and define constants for the registers and their bitfields instead? -- John Baldwin