Matt DeVillier has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/37539 )
Change subject: drivers/i2c/rt5663/: fix missing header include ......................................................................
drivers/i2c/rt5663/: fix missing header include
'struct acpi_gpio' and 'struct acpi_irq' require the inclusion of acpi_device.h. The only reason this wasn't caught previously is due to the header being included with another driver compiled first on the one board using it (google/eve).
Change-Id: I987f0ec6f769e550f3421629e0ef0c579a3d12f9 Signed-off-by: Matt DeVillier matt.devillier@gmail.com --- M src/drivers/i2c/rt5663/chip.h M src/drivers/i2c/rt5663/rt5663.c 2 files changed, 1 insertion(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/39/37539/1
diff --git a/src/drivers/i2c/rt5663/chip.h b/src/drivers/i2c/rt5663/chip.h index b1ed4b6..eb5ac4a 100644 --- a/src/drivers/i2c/rt5663/chip.h +++ b/src/drivers/i2c/rt5663/chip.h @@ -15,7 +15,7 @@ * Realtek RT5663 audio codec devicetree bindings */
-#include <stdint.h> +#include <arch/acpi_device.h>
struct drivers_i2c_rt5663_config { /* I2C Bus Frequency in Hertz (default 400kHz) */ diff --git a/src/drivers/i2c/rt5663/rt5663.c b/src/drivers/i2c/rt5663/rt5663.c index 6f4e032..b6e8925 100644 --- a/src/drivers/i2c/rt5663/rt5663.c +++ b/src/drivers/i2c/rt5663/rt5663.c @@ -12,7 +12,6 @@ */
#include <arch/acpi.h> -#include <arch/acpi_device.h> #include <arch/acpigen.h> #include <console/console.h> #include <device/i2c.h>
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37539 )
Change subject: drivers/i2c/rt5663/: fix missing header include ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37539/1/src/drivers/i2c/rt5663/chip... File src/drivers/i2c/rt5663/chip.h:
https://review.coreboot.org/c/coreboot/+/37539/1/src/drivers/i2c/rt5663/chip... PS1, Line 18: #include <stdint.h> Why do you drop stdint.h?
Matt DeVillier has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37539 )
Change subject: drivers/i2c/rt5663/: fix missing header include ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37539/1/src/drivers/i2c/rt5663/chip... File src/drivers/i2c/rt5663/chip.h:
https://review.coreboot.org/c/coreboot/+/37539/1/src/drivers/i2c/rt5663/chip... PS1, Line 18: #include <stdint.h>
Why do you drop stdint. […]
because it's already included in acpi_device.h
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37539 )
Change subject: drivers/i2c/rt5663/: fix missing header include ......................................................................
Patch Set 1:
(2 comments)
Generally, relying on indirect includes is discouraged (unless a header file is specifically crafted to provide another file's definitions).
https://review.coreboot.org/c/coreboot/+/37539/1/src/drivers/i2c/rt5663/chip... File src/drivers/i2c/rt5663/chip.h:
https://review.coreboot.org/c/coreboot/+/37539/1/src/drivers/i2c/rt5663/chip... PS1, Line 18: #include <stdint.h>
because it's already included in acpi_device. […]
But it's used independently from ACPI. e.g. if one would remove the struct acpi* fields below, they shouldn't have to add the include back. So it should stay.
https://review.coreboot.org/c/coreboot/+/37539/1/src/drivers/i2c/rt5663/rt56... File src/drivers/i2c/rt5663/rt5663.c:
https://review.coreboot.org/c/coreboot/+/37539/1/src/drivers/i2c/rt5663/rt56... PS1, Line 15: #include <arch/acpi_device.h> Same here, if one would remove the fields in `chip.h`...
Hello Angel Pons, Arthur Heymans, build bot (Jenkins), Nico Huber,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/37539
to look at the new patch set (#2).
Change subject: drivers/i2c/rt5663/: fix missing header include ......................................................................
drivers/i2c/rt5663/: fix missing header include
'struct acpi_gpio' and 'struct acpi_irq' require the inclusion of acpi_device.h. The only reason this wasn't caught previously is due to the header being included with another driver compiled first on the one board using it (google/eve).
Change-Id: I987f0ec6f769e550f3421629e0ef0c579a3d12f9 Signed-off-by: Matt DeVillier matt.devillier@gmail.com --- M src/drivers/i2c/rt5663/chip.h 1 file changed, 1 insertion(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/39/37539/2
Matt DeVillier has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37539 )
Change subject: drivers/i2c/rt5663/: fix missing header include ......................................................................
Patch Set 2:
(2 comments)
https://review.coreboot.org/c/coreboot/+/37539/1/src/drivers/i2c/rt5663/chip... File src/drivers/i2c/rt5663/chip.h:
https://review.coreboot.org/c/coreboot/+/37539/1/src/drivers/i2c/rt5663/chip... PS1, Line 18: #include <stdint.h>
But it's used independently from ACPI. e.g. if one would remove the […]
Done
https://review.coreboot.org/c/coreboot/+/37539/1/src/drivers/i2c/rt5663/rt56... File src/drivers/i2c/rt5663/rt5663.c:
https://review.coreboot.org/c/coreboot/+/37539/1/src/drivers/i2c/rt5663/rt56... PS1, Line 15: #include <arch/acpi_device.h>
Same here, if one would remove the fields in `chip.h`...
Done
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37539 )
Change subject: drivers/i2c/rt5663/: fix missing header include ......................................................................
Patch Set 2: Code-Review+2
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/37539 )
Change subject: drivers/i2c/rt5663/: fix missing header include ......................................................................
drivers/i2c/rt5663/: fix missing header include
'struct acpi_gpio' and 'struct acpi_irq' require the inclusion of acpi_device.h. The only reason this wasn't caught previously is due to the header being included with another driver compiled first on the one board using it (google/eve).
Change-Id: I987f0ec6f769e550f3421629e0ef0c579a3d12f9 Signed-off-by: Matt DeVillier matt.devillier@gmail.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/37539 Reviewed-by: Nico Huber nico.h@gmx.de Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/drivers/i2c/rt5663/chip.h 1 file changed, 1 insertion(+), 0 deletions(-)
Approvals: build bot (Jenkins): Verified Nico Huber: Looks good to me, approved
diff --git a/src/drivers/i2c/rt5663/chip.h b/src/drivers/i2c/rt5663/chip.h index b1ed4b6..235c253 100644 --- a/src/drivers/i2c/rt5663/chip.h +++ b/src/drivers/i2c/rt5663/chip.h @@ -15,6 +15,7 @@ * Realtek RT5663 audio codec devicetree bindings */
+#include <arch/acpi_device.h> #include <stdint.h>
struct drivers_i2c_rt5663_config {
-
Angel Pons (Code Review) -
Matt DeVillier (Code Review)
-
Nico Huber (Code Review)
-
Patrick Georgi (Code Review)