[coreboot] [PATCH] v3: improve PCI device doxygen comments and printks

Carl-Daniel Hailfinger c-d.hailfinger.devel.2006 at gmx.net
Mon Aug 18 13:06:51 CEST 2008


On 18.08.2008 09:27, Stefan Reinauer wrote:
> Carl-Daniel Hailfinger wrote:
>   
>> Index: corebootv3-pci_device_better_prints_comments/device/pci_device.c
>> ===================================================================
>> --- corebootv3-pci_device_better_prints_comments/device/pci_device.c	(Revision 780)
>> +++ corebootv3-pci_device_better_prints_comments/device/pci_device.c	(Arbeitskopie)
>> @@ -950,12 +950,30 @@
>>  /** 
>>   * Scan a PCI bus.
>>   *
>> - * Determine the existence of a given PCI device.
>> + * Determine the existence of a given PCI device. Allocate a new struct device
>> + * if dev==NULL was passed in and the device exists in hardware.
>>   *
>> + * @param dev Pointer to the device structure if it already is in the device
>> + *         tree, i.e. was specified in the dts. It may not exist on hardware,
>> + *         however. Looking for hardware not yet in the device tree has this
>> + *         parameter as NULL.
>>   * @param bus Pointer to the bus structure.
>>   * @param devfn A device/function number.
>> - * @return The device structure for the device (if found)
>> - *         or the NULL if no device is found.
>> + * @return The device structure for the device if it exists in hardware
>> + *         or the passed in device structure with enabled=0 if the device
>> + *         does not exist in hardware and only in the tree
>> + *         or NULL if no device is found and dev==NULL was passed in.
>> + *
>> + * There are three cases:
>>   
>>     
> .. of what?
>   

What about "There are three cases for which this function is called:"

>> + * - known knowns. In this case the device is in the tree, i.e. not NULL,
>> + * and we know it's there: it's soldered down or part of the on-chip
>> + * hardware. In this case dev is not NULL.
>> + * - known unknowns. This is a device that might be there, but we don't
>> + * know. So we have to probe it. It's in the dts, which is why
>> + * it is a known unknown.
>>   
>>     
> How s the function called for the known knowns? Those are neither PCI
>   

Did you mean "called for the known UNknowns"?

> cards nor stuff in the DTS according to your description? So where does
> it come from?
>   

The first two cases are for devices which are in the dts and therefore
in the tree.

>> + * - unknown unknowns. A PCI card in a PCI slot. We can't cover all
>> + * possible cards. dev is NULL. We are checking to see if something is
>> + * there; if so, we will allocate a dev and put it in the three.
>>   
>>     
>
>   
>> @@ -1011,8 +1029,8 @@
>>  		if ((id == 0xffffffff) || (id == 0x00000000) ||
>>  		    (id == 0x0000ffff) || (id == 0xffff0000)) {
>>  			if (dev->enabled) {
>> -				printk(BIOS_INFO,
>> -				       "Disabling static device: %s\n",
>> +				printk(BIOS_INFO, "PCI: Static device not "
>> +				       "found, setting enabled=0: %s\n",
>>  				       dev_path(dev));
>>   
>>     
> I dislike this change of output. It's not a BIOS_DEBUG message.
>   

Do you dislike the severity level (same as before) or the new wording
(the old wording was wrong)?

>>  				dev->enabled = 0;
>>  			}
>>     


Regards,
Carl-Daniel

-- 
http://www.hailfinger.org/





More information about the coreboot mailing list