Re: [RFC] winbond watchdog driver for FreeBSD/i386 and FreeBSD/amd64

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