Date: Sat, 9 May 2015 23:35:32 -0300 From: Luiz Otavio O Souza <loos.br@gmail.com> To: John Baldwin <jhb@freebsd.org> Cc: Luiz Otavio O Souza <loos@freebsd.org>, src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r282674 - in head/sys/dev: iicbus ofw Message-ID: <CAJ8CS7qLnENZMyKDrZ5V_De3WeTkByWRQCrQsNKws4wzV9Jr4w@mail.gmail.com> In-Reply-To: <2165864.Vg8k3tllF7@ralph.baldwin.cx> References: <201505090305.t4935jYk086983@svn.freebsd.org> <2165864.Vg8k3tllF7@ralph.baldwin.cx>
next in thread | previous in thread | raw e-mail | index | archive | help
On Sat, May 9, 2015 at 7:45 AM, John Baldwin wrote: > On Saturday, May 09, 2015 03:05:45 AM Luiz Otavio O Souza wrote: >> Author: loos >> Date: Sat May 9 03:05:44 2015 >> New Revision: 282674 >> URL: https://svnweb.freebsd.org/changeset/base/282674 >> >> Log: >> Handle IRQ resources on iicbus and ofw_iicbus. >> >> Based on a patch submitted by Michal Meloun <meloun@miracle.cz>. >> >> Modified: >> head/sys/dev/iicbus/iicbus.c >> head/sys/dev/iicbus/iicbus.h >> head/sys/dev/ofw/ofw_iicbus.c >> >> Modified: head/sys/dev/iicbus/iicbus.c >> ============================================================================== >> --- head/sys/dev/iicbus/iicbus.c Sat May 9 00:48:44 2015 (r282673) >> +++ head/sys/dev/iicbus/iicbus.c Sat May 9 03:05:44 2015 (r282674) >> @@ -157,9 +159,9 @@ iicbus_probe_nomatch(device_t bus, devic >> { >> struct iicbus_ivar *devi = IICBUS_IVAR(child); >> >> - device_printf(bus, "<unknown card>"); >> - printf(" at addr %#x\n", devi->addr); >> - return; >> + device_printf(bus, "<unknown card> at addr %#x", devi->addr); >> + resource_list_print_type(&devi->rl, "irq", SYS_RES_IRQ, "%ld"); >> + printf("\n"); > > Other bus drivers do not print resources for nomatch devices (or at > least PCI doesn't). Given that, I'm not sure it makes sense to do > that here? That's right, it was an overzealous from my part. > >> +static int >> +iicbus_set_resource(device_t dev, device_t child, int type, int rid, >> + u_long start, u_long count) >> +{ >> + struct iicbus_ivar *devi; >> + struct resource_list_entry *rle; >> + >> + devi = IICBUS_IVAR(child); >> + rle = resource_list_add(&devi->rl, type, rid, start, >> + start + count - 1, count); >> + if (rle == NULL) >> + return (ENXIO); >> + >> + return (0); >> +} > > Isn't this the same as bus_generic_rl_set_resource()? > >> + >> +static struct resource * >> +iicbus_alloc_resource(device_t bus, device_t child, int type, int *rid, >> + u_long start, u_long end, u_long count, u_int flags) >> +{ >> + struct resource_list *rl; >> + struct resource_list_entry *rle; >> + >> + /* Only IRQ resources are supported. */ >> + if (type != SYS_RES_IRQ) >> + return (NULL); >> + >> + /* >> + * Request for the default allocation with a given rid: use resource >> + * list stored in the local device info. >> + */ >> + if ((start == 0UL) && (end == ~0UL)) { >> + rl = BUS_GET_RESOURCE_LIST(bus, child); >> + if (rl == NULL) >> + return (NULL); >> + rle = resource_list_find(rl, type, *rid); >> + if (rle == NULL) { >> + if (bootverbose) >> + device_printf(bus, "no default resources for " >> + "rid = %d, type = %d\n", *rid, type); >> + return (NULL); >> + } >> + start = rle->start; >> + end = rle->end; >> + count = rle->count; >> + } >> + >> + return (bus_generic_alloc_resource(bus, child, type, rid, start, end, >> + count, flags)); > > If you are using a resource list, you should be using resource_list_alloc(). > However, I think you can replace this entire method with just > bus_generic_rl_alloc_resource(). > >> @@ -297,6 +366,16 @@ static device_method_t iicbus_methods[] >> DEVMETHOD(device_detach, iicbus_detach), >> >> /* bus interface */ >> + DEVMETHOD(bus_setup_intr, bus_generic_setup_intr), >> + DEVMETHOD(bus_teardown_intr, bus_generic_teardown_intr), >> + DEVMETHOD(bus_release_resource, bus_generic_release_resource), > > After fixing alloc_resource to use resource_list_alloc() (either directly > or via the generic method), this should be set to > bus_generic_rl_release_resource(). > >> Modified: head/sys/dev/ofw/ofw_iicbus.c >> ============================================================================== >> --- head/sys/dev/ofw/ofw_iicbus.c Sat May 9 00:48:44 2015 (r282673) >> +++ head/sys/dev/ofw/ofw_iicbus.c Sat May 9 03:05:44 2015 (r282674) >> @@ -199,3 +205,12 @@ ofw_iicbus_get_devinfo(device_t bus, dev >> dinfo = device_get_ivars(dev); >> return (&dinfo->opd_obdinfo); >> } >> + >> +static struct resource_list * >> +ofw_iicbus_get_resource_list(device_t bus __unused, device_t child) >> +{ >> + struct ofw_iicbus_devinfo *devi; >> + >> + devi = device_get_ivars(child); >> + return (&devi->opd_dinfo.rl); >> +} > > I think you don't actually need this since the inherited method should > already work. > > -- > John Baldwin Yeah, that's all correct, Michal had already warned about a few of these issues. Fixed in r282702. Thanks for the review. Luiz
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAJ8CS7qLnENZMyKDrZ5V_De3WeTkByWRQCrQsNKws4wzV9Jr4w>