lkml.org 
[lkml]   [2008]   [Jan]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 1/7] driver-core : add class iteration api
    On Jan 12, 2008 6:50 PM, Stefan Richter <stefanr@s5r6.in-berlin.de> wrote:
    > Dave Young wrote:
    > > Add the following class iteration functions for driver use:
    >
    > Thanks Dave. I will check the ieee1394 part in detail later.
    >
    > ...
    > > +/**
    > > + * class_find_device - device iterator for locating a particular device
    > > + * @class: the class we're iterating
    > > + * @data: data for the match function
    > > + * @match: function to check device
    > > + *
    > > + * This is similar to the class_for_each_dev() function above, but it
    > > + * returns a reference to a device that is 'found' for later use, as
    > > + * determined by the @match callback.
    >
    > Maybe add "Drop the reference with put_device() after use." for the
    > really slow driver programmers like me?

    Sounds good, thanks.

    >
    > > + *
    > > + * The callback should return 0 if the device doesn't match and non-zero
    > > + * if it does. If the callback returns non-zero, this function will
    > > + * return to the caller and not iterate over any more devices.
    > > + */
    > > +struct device *class_find_device(struct class *class, void *data,
    > > + int (*match)(struct device *, void *))
    > > +{
    >
    > A general comment on the linux/device.h API (not a direct comment on
    > your patch):
    >
    > The match argument in bus_find_device(), driver_find_device(),
    > device_find_child(), class_find_device(), class_find_child() could be
    > changed to
    >
    > bool (*match)(struct device *, void *)).
    >
    > Then the semantics are IMO a little bit clearer. Ditto for the
    > dr_match_t type and the struct bus_type.match member.

    Yes, from semantics side it's better.
    But IMHO int is good as well, although it need a little bit more
    understanding of the api.

    >
    > I don't know though whether the churn of doing such a change everywhere
    > would be justified by the result.
    >
    >
    > A comment on patch 2/7...6/7:
    >
    > You can bring most or all of the various __match implementations into a
    > slightly terser but IMO easy to read form, like this:
    >
    > static int __match_ne(struct device *dev, void *data)
    > {
    > struct unit_directory *ud;
    > struct node_entry *ne = (struct node_entry *)data;
    >
    > ud = container_of(dev, struct unit_directory, unit_dev);
    > - if (ud->ne == ne)
    > - return 1;
    > - return 0;
    > + return ud->ne == ne;
    > }
    >
    > Here it is also easy to see that readability would improve if the return
    > type was bool rather than int.

    Ok, thanks.

    > --
    > Stefan Richter
    > -=====-==--- ---= -==--
    > http://arcgraph.de/sr/
    >


    \
     
     \ /
      Last update: 2008-01-14 02:35    [W:0.043 / U:30.904 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site