Skip site navigation (1)Skip section navigation (2)
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>