On Sun, 28 Jul 2019, Mark Cave-Ayland wrote:
On 28/07/2019 11:39, BALATON Zoltan wrote:
On Sun, 28 Jul 2019, Mark Cave-Ayland wrote:
Yeah, it's a bug - I missed that in the case of unknown devices we were still falling back to the old behaviour of force-creating devices at known paths which is the behaviour that this patchset is removing.
Attached is a fix that works for me when applied on top of the v2 series if you can test it?
Thanks it works but isn't it simpler to just write it as below? device-name is the same as encode-string " name" property so why open code it? device-name seems to be used more commonly elsewhere. Also your patch uses spaces for indent while lines around it still use tabs so maybe this should also indent with tabs for consistency. (There are also a few more whitespace errors elsewhere in this file from previous patches, git diff or git log -p shows them in red for me.)
diff --git a/drivers/pci.c b/drivers/pci.c index 4bc02c9..2c7d3ef 100644 --- a/drivers/pci.c +++ b/drivers/pci.c @@ -1261,21 +1285,15 @@ static void ob_pci_add_properties(phandle_t phandle, class_prog = pci_config_read8(addr, PCI_CLASS_PROG); class_code = pci_config_read16(addr, PCI_CLASS_DEVICE);
- if (pci_dev) {
/**/if (pci_dev->name) {push_str(pci_dev->name);fword("device-name");} else {char path[256];snprintf(path, sizeof(path),"pci%x,%x", vendor_id, device_id);push_str(path);fword("device-name");}- } else {
PCI_DPRINTF("*** missing pci_dev\n");- }
if (pci_dev && pci_dev->name) {
push_str(pci_dev->name);} else {
char path[256];snprintf(path, sizeof(path),"pci%x,%x", vendor_id, device_id);push_str(path);}
fword("device-name");
/* create properties as described in 2.5 */
Certainly that is one way of writing it, however given that this is going to get squashed into the PCI changeover patch I'll probably leave it as-is for 2 reasons: 1) it's easier from the diff to see which logic has changed rather than throwing device-name into the mix, and 2) it was helpful for me to have separate breakpoints
Your current v2 "pci: convert to use BIND_NODE_METHODS() macro" patch already changes open coded device-name to one fword so squsahing this in probably would simplify it while your version reverts that change but as you like. This can also be a separate change later to replace open coded device-names in one separate patch. For keeping 3 branches when 2 is enough I find the simpler version easier to read than having two identical branches. I'm usually against code duplication because it's easier to get it out of sync than when we only have 2 branches.
Regarding whitespace errors that is a long-running issue with OpenBIOS in that over time people have used either tabs and/or spaces, so I tend to use what's around me to keep the diffs readable. Sadly some files mix up tabs and spaces liberally so you'll never end up with a perfect diff :(
Not only tabs vs. spaces but previous commits added white space at end of line or on empty lines. This can be seen with git diff /dev/null drivers/pci.c for example. This patch did not have that problem but I've seen this in other lines when looking at git diff output.
Regards, BALATON Zoltan