On Mon, Jul 27, 2020 at 03:53:54PM -0500, Daniel Dadap wrote:
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -34,6 +34,7 @@
> #include <linux/module.h>
> #include <linux/slab.h>
> #include <linux/vga_switcheroo.h>
> +#include <linux/pci.h>
Nit: Alphabetic ordering.
> +static u64 *get_dod_entries(acpi_handle handle, int *count)
> +{
> + acpi_status status;
> + struct acpi_buffer buf = { ACPI_ALLOCATE_BUFFER, NULL };
> + union acpi_object *dod;
> + int i;
> + u64 *ret = NULL;
Nits: Reverse christmas tree.
"ret" is a poor name, I'd suggest "entries" or something like that.
The spec says that _DOD is a list of 32-bit values, not 64-bit.
> + status = acpi_evaluate_object(handle, "_DOD", NULL, &buf);
> +
> + if (ACPI_FAILURE(status))
> + return NULL;
Nit: No blank line between function invocation and error check.
> + dod = buf.pointer;
> +
> + if (dod == NULL || dod->type != ACPI_TYPE_PACKAGE)
> + goto done;
Same.
> + ret = kmalloc_array(dod->package.count, sizeof(*ret), GFP_KERNEL);
> + if (ret == NULL)
> + goto done;
Nit: Usually we use "if (!ret)" or "if (ret)".
> + list_for_each_safe(node, next, &device->children) {
No, that's not safe because the ACPI namespace may change concurrently,
e.g. because a DSDT patch is applied by the user via sysfs.
Use acpi_walk_namespace() with a depth of 1 instead.
> + for (i = 0; i < num_dod_entries; i++) {
> + if (adr == dod_entries[i]) {
> + ret = do_acpi_ddc(child->handle);
> +
> + if (ret != NULL)
> + goto done;
I guess ideally we'd want to correlate the display objects with
drm_connectors or at least constrain the search to Display Type
"Internal/Integrated Digital Flat Panel" instead of picking the
first EDID found. Otherwise we might erroneously use the DDC
for an externally attached display.
> +struct edid *drm_get_edid_acpi(void)
> +{
> +#if defined(CONFIG_ACPI) && defined(CONFIG_PCI)
No, put an empty inline stub in the header file instead of using #ifdef, see:
https://www.kernel.org/doc/html/latest/process/coding-style.html#conditional-compilation
Patches 2, 3 and 4 need a "drm/" prefix in the Subject, e.g.
"drm/i915: ".
Please cc all ACPI-related patches to linux-acpi.
Thanks,
Lukas
_______________________________________________
amd-gfx mailing list
amd-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
