In order to use inclusive terminology, rename pci_xbox_blacklisted() as pci_xbox_ignore_device(), and remove an obvious comment.
Suggested-by: Mark Cave-Ayland mark.cave-ayland@ilande.co.uk Signed-off-by: Philippe Mathieu-Daudé philmd@redhat.com --- drivers/pci.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/drivers/pci.c b/drivers/pci.c index 34ae69a..0e12dc7 100644 --- a/drivers/pci.c +++ b/drivers/pci.c @@ -1356,7 +1356,7 @@ static void ob_pci_add_properties(phandle_t phandle, }
#ifdef CONFIG_XBOX -static char pci_xbox_blacklisted (int bus, int devnum, int fn) +static char pci_xbox_ignore_device (int bus, int devnum, int fn) { /* * The Xbox MCPX chipset is a derivative of the nForce 1 @@ -1387,9 +1387,6 @@ static char pci_xbox_blacklisted (int bus, int devnum, int fn) if (bus >= 2) return 1;
- /* - * The device is not blacklisted. - */ return 0; } #endif @@ -1708,7 +1705,7 @@ static int ob_pci_read_identification(int bus, int devnum, int fn, pci_addr addr;
#ifdef CONFIG_XBOX - if (pci_xbox_blacklisted (bus, devnum, fn)) + if (pci_xbox_ignore_device (bus, devnum, fn)) return; #endif addr = PCI_ADDR(bus, devnum, fn);
On Thu, Feb 11, 2021 at 03:42:44PM +0100, Philippe Mathieu-Daudé wrote:
In order to use inclusive terminology, rename pci_xbox_blacklisted() as pci_xbox_ignore_device(), and remove an obvious comment.
But that loses information, there is no hint *why* the device is ignored anymore. Maybe you can do a better name (one that actually means the same as blacklisted), or add a comment somewhere?
Segher
Hi Segher,
On 2/11/21 3:55 PM, Segher Boessenkool wrote:
On Thu, Feb 11, 2021 at 03:42:44PM +0100, Philippe Mathieu-Daudé wrote:
In order to use inclusive terminology, rename pci_xbox_blacklisted() as pci_xbox_ignore_device(), and remove an obvious comment.
But that loses information, there is no hint *why* the device is ignored anymore. Maybe you can do a better name (one that actually means the same as blacklisted), or add a comment somewhere?
Any suggestion for better names?
I followed these guidelines:
https://github.com/conscious-lang/conscious-lang-docs/blob/main/faq.md#what-...
Regards,
Phil.
On Thu, Feb 11, 2021 at 04:34:38PM +0100, Philippe Mathieu-Daudé wrote:
Hi Segher,
On 2/11/21 3:55 PM, Segher Boessenkool wrote:
On Thu, Feb 11, 2021 at 03:42:44PM +0100, Philippe Mathieu-Daudé wrote:
In order to use inclusive terminology, rename pci_xbox_blacklisted() as pci_xbox_ignore_device(), and remove an obvious comment.
But that loses information, there is no hint *why* the device is ignored anymore. Maybe you can do a better name (one that actually means the same as blacklisted), or add a comment somewhere?
Any suggestion for better names?
ignore_broken_device perhaps... Something like that. Someone who knows exactly why the device is blacklisted can tell you.
Just "ignore_device" would fit the code perfectly well here, but then you need to add a comment (where it is used) saying why that is done. The existing name suggests it is because the device is broken or similar; that it cannot be handled like other devices anyway.
You cannot just replace terminology with other words that do not have the same connotations without losing important information, so any such replacement should be done by people who understand the code well :-(
Segher
On 11/02/2021 16:07, Segher Boessenkool wrote:
But that loses information, there is no hint *why* the device is ignored anymore. Maybe you can do a better name (one that actually means the same as blacklisted), or add a comment somewhere?
Any suggestion for better names?
ignore_broken_device perhaps... Something like that. Someone who knows exactly why the device is blacklisted can tell you.
FWIW the comments in the pci_xbox_blacklisted() already give the reasoning:
/* * The Xbox MCPX chipset is a derivative of the nForce 1 * chipset. It almost has the same bus layout; some devices * cannot be used, because they have been removed. */
...
/* * Devices 00:00.1 and 00:00.2 used to be memory controllers on * the nForce chipset, but on the Xbox, using them will lockup * the chipset. */
...
/* * Bus 1 only contains a VGA controller at 01:00.0. When you try * to probe beyond that device, you only get garbage, which * could cause lockups. */
...
/* * Bus 2 used to contain the AGP controller, but the Xbox MCPX * doesn't have one. Probing it can cause lockups. */
Just "ignore_device" would fit the code perfectly well here, but then you need to add a comment (where it is used) saying why that is done. The existing name suggests it is because the device is broken or similar; that it cannot be handled like other devices anyway.
I prefer to keep the pci_xbox prefix as per Philippe's v3 patch since it makes it clear that this only relevant to XBox (and indeed, the #ifdef enforces it).
How about a single line comment above the call to pci_xbox_ignore_device() that says something like this?
/* Some in-built XBox PCI devices are unsupported/unavailable */
You cannot just replace terminology with other words that do not have the same connotations without losing important information, so any such replacement should be done by people who understand the code well :-(
Indeed, it is important to understand the context of the original code when coming up with a replacement.
ATB,
Mark.
Hi!
On Thu, Feb 11, 2021 at 04:52:06PM +0000, Mark Cave-Ayland wrote:
On 11/02/2021 16:07, Segher Boessenkool wrote:
But that loses information, there is no hint *why* the device is ignored anymore. Maybe you can do a better name (one that actually means the same as blacklisted), or add a comment somewhere?
Any suggestion for better names?
ignore_broken_device perhaps... Something like that. Someone who knows exactly why the device is blacklisted can tell you.
FWIW the comments in the pci_xbox_blacklisted() already give the reasoning:
<snip>
Ah excellent :-) Then it probably is fine. (The chommit message should mention this though!)
Just "ignore_device" would fit the code perfectly well here, but then you need to add a comment (where it is used) saying why that is done. The existing name suggests it is because the device is broken or similar; that it cannot be handled like other devices anyway.
I prefer to keep the pci_xbox prefix
Yes of course, I'm just a lazy typist.
How about a single line comment above the call to pci_xbox_ignore_device() that says something like this?
/* Some in-built XBox PCI devices are unsupported/unavailable */
Excellent :-)
Thanks guys,
Segher
On 11/02/2021 17:39, Segher Boessenkool wrote:
Hi!
On Thu, Feb 11, 2021 at 04:52:06PM +0000, Mark Cave-Ayland wrote:
On 11/02/2021 16:07, Segher Boessenkool wrote:
But that loses information, there is no hint *why* the device is ignored anymore. Maybe you can do a better name (one that actually means the same as blacklisted), or add a comment somewhere?
Any suggestion for better names?
ignore_broken_device perhaps... Something like that. Someone who knows exactly why the device is blacklisted can tell you.
FWIW the comments in the pci_xbox_blacklisted() already give the reasoning:
<snip>
Ah excellent :-) Then it probably is fine. (The chommit message should mention this though!)
Just "ignore_device" would fit the code perfectly well here, but then you need to add a comment (where it is used) saying why that is done. The existing name suggests it is because the device is broken or similar; that it cannot be handled like other devices anyway.
I prefer to keep the pci_xbox prefix
Yes of course, I'm just a lazy typist.
How about a single line comment above the call to pci_xbox_ignore_device() that says something like this?
/* Some in-built XBox PCI devices are unsupported/unavailable */Excellent :-)
Thanks guys,
Great! I can take Philippe's v3 and add the extra comment during merge. I'll give the outstanding patches a test and push them to git if everything seems good.
ATB,
Mark.
On 15/02/2021 20:34, Mark Cave-Ayland wrote:
Great! I can take Philippe's v3 and add the extra comment during merge. I'll give the outstanding patches a test and push them to git if everything seems good.
This seems to work fine here, so I've pushed both this patch and your 40p patchset to git master.
Note: I ended up having to fix up the patches by hand after applying since each line was terminated with a ^M instead of a LF. Do you have any non-standard core.autocrlf settings in your git config?
ATB,
Mark.
On 2/15/21 11:05 PM, Mark Cave-Ayland wrote:
On 15/02/2021 20:34, Mark Cave-Ayland wrote:
Great! I can take Philippe's v3 and add the extra comment during merge. I'll give the outstanding patches a test and push them to git if everything seems good.
This seems to work fine here, so I've pushed both this patch and your 40p patchset to git master.
Thanks!
Note: I ended up having to fix up the patches by hand after applying since each line was terminated with a ^M instead of a LF. Do you have any non-standard core.autocrlf settings in your git config?
No I don't... This is odd. I sent the series using git-publish, but it doesn't seem to enforce that neither.
$ git config --global --get core.autocrlf $ git config --system --get core.autocrlf $ git config --local --get core.autocrlf $
Sorry you had to do manual fixup :S
Regards,
Phil.
-
Mark Cave-Ayland -
Philippe Mathieu-Daudé -
Segher Boessenkool