[coreboot] [PATCH] i82801gx updates

[Stefan Reinauer]

On 11.03.2009 13:47 Uhr, Carl-Daniel Hailfinger wrote:
> I don't know the code well, so this is not a real review. Just a nitpick.
>
> On 11.03.2009 13:23, Stefan Reinauer wrote:
>   
>> +static void smbus_set_subsystem(device_t dev, unsigned vendor, unsigned device)
>> +{
>> +	if (!vendor || !device) {
>>   
>>     
>
> This case should output a diagnostic message at SPEW or DEBUG level.
>   
I don't think this is a good idea. It's a perfectly fine case that we
don't want to warn about. Plus, the calling function already does a
debug level print in the case that a subsystem function is specified.

>> +		pci_write_config32(dev, PCI_SUBSYSTEM_VENDOR_ID,
>> +				pci_read_config32(dev, 0));
>>   
>>     
>
> PCI_VENDOR_ID instead of 0 for consistency.
>   

Good point.


>   
>> +	} else {
>> +		pci_write_config32(dev, PCI_SUBSYSTEM_VENDOR_ID,
>> +				((device & 0xffff) << 16) | (vendor & 0xffff));
>> +	}
>> +}
>>   
>>     
>
> Same for the other *_set_subsystem. Maybe we even want a generic
> function to handle this since the *_set_subsystem functions in the patch
> seem to be identical.
>   
Yes, mostly. I was going to look into that as a next step. So far I only
found information from intel that they suggest this behavior, so I
didn't want to make it a global rule.


-- 
coresystems GmbH • Brahmsstr. 16 • D-79104 Freiburg i. Br.
      Tel.: +49 761 7668825 • Fax: +49 761 7664613
Email: info at coresystems.de  • http://www.coresystems.de/
Registergericht: Amtsgericht Freiburg • HRB 7656
Geschäftsführer: Stefan Reinauer • Ust-IdNr.: DE245674866

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.coreboot.org/pipermail/coreboot/attachments/20090311/e4cfd8fa/attachment.html>
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: i82801gx_update.diff
URL: <http://www.coreboot.org/pipermail/coreboot/attachments/20090311/e4cfd8fa/attachment.ksh>