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
_______________________________________________
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
