On Thu, Jun 04, 2015 at 05:19:44PM +0300, Dan Carpenter wrote: > On Thu, Jun 04, 2015 at 09:22:40AM -0400, Benjamin Romer wrote: > > diff --git a/drivers/staging/unisys/visorbus/visorchipset.c b/drivers/staging/unisys/visorbus/visorchipset.c > > index 4dd0a07..618732b 100644 > > --- a/drivers/staging/unisys/visorbus/visorchipset.c > > +++ b/drivers/staging/unisys/visorbus/visorchipset.c > > @@ -56,6 +56,8 @@ > > #define UNISYS_SPAR_ID_ECX 0x70537379 > > #define UNISYS_SPAR_ID_EDX 0x34367261 > > > > +#define BUS_ROOT_DEVICE UINT_MAX > > + > > /* > > * Module parameters > > */ > > @@ -727,8 +729,8 @@ static int match_visorbus_dev_by_id(struct device *dev, void *data) > > u32 bus_no = id->bus_no; > > u32 dev_no = id->dev_no; > > > > - if (((bus_no == -1) || (vdev->chipset_bus_no == bus_no)) && > > - ((dev_no == -1) || (vdev->chipset_dev_no == dev_no))) > > + if ((vdev->chipset_bus_no == bus_no) && > > + (vdev->chipset_dev_no == dev_no)) > > return 1; > > This talk about "root device" confuses me. I thought that -1 was a wild > card. Anyway, we introduce the BUS_ROOT_DEVICE macro but don't use it. > Shouldn't it be defined as 0 since that it the root device? Hi Dan, Thanks for the review. My latest round of patch re-arrangement to make this patchset bisectable makes this patch look awkward. Awkward because as you said BUS_ROOT_DEVIC isn't used. However, it is used in patch 5 (the next patch), but is also moved from a .c file to a .h to handle another corner case in another .c file. So patch5 is a better study of the usage and this patch is the fallout of a patch written later and then re-arranged to be earlier. But lets back up to 'what is root device'. Before unisys can load its io drivers, it needs a virtual bus to sit on. Their s-par sends 'create bus' commands and I convert those into linux device creations. When an io driver is created later using a 'device create' command from the s-par, it too creates a linux device with its parent pointing to the 'bus root device' or the initial virtual bus created. This mimics what other virtual/real buses are doing in linux (I believe). So to back up a little bit, when the 'device create' command is sent from the s-par, it provides a bus number to sit on and a device number to assign it to. Considering all the buses and devices are all lumped together under one subsystem 'visorbus' in the device tree link-list, it is easy to walk that link-list using the linux infrastructure. The only problem is how to separate an actual device from the actual bus it sits on when both have the same bus no. In that case, I use -1 to represent the 'bus root device'. I agree traditionally that infers a wild card. I was scratching my head on a better mechanism without re-implementing the wheel and at the same time get something working. Hopefully that clarifies the term of 'bus root device'. Suggestions welcomed. :-) To answer your second question, why not use 0? Originally I did, but unisys explained to me that it is valid for their device numbering to start at 0. Their testing uncovered this bug and hence the need for this patch. Their device numbering is dynamic but probably only goes as far as 8 or so. I chose -1 to be extremely safe. :-) I can look to see if their docs have a MAX_DEVICE_NUMBER and use that instead for the bus root device, if that would make more sense from a code readability perspective and leave -1 as a possibility for a wild card in the future. Thoughts? Cheers, Don _______________________________________________ devel mailing list devel@xxxxxxxxxxxxxxxxxxxxxx http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
