Change in coreboot[master]: src/{device,include}: Use PNP_IDX_EN instead of magic number

[HAOUAS Elyes (Code Review)]

HAOUAS Elyes has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/44835 )

Change subject: src/{device,include}: Use PNP_IDX_EN instead of magic number
......................................................................

src/{device,include}: Use PNP_IDX_EN instead of magic number

Change-Id: I68590605e261ecaace9f3cea28cfa6ec3b913a8a
Signed-off-by: Elyes HAOUAS 
---
M src/device/pnp_device.c
M src/include/device/pnp_ops.h
2 files changed, 7 insertions(+), 5 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/35/44835/1

diff --git a/src/device/pnp_device.c b/src/device/pnp_device.c
index cd7adf3..9fa032e 100644
--- a/src/device/pnp_device.c
+++ b/src/device/pnp_device.c
@@ -5,6 +5,7 @@
 #include <arch/io.h>
 #include <device/device.h>
 #include <device/pnp.h>
+#include <device/pnp_def.h>

/* PNP config mode wrappers */

@@ -56,7 +57,7 @@
 {
    u8 tmp, bitpos;

-	tmp = pnp_read_config(dev, 0x30);
+	tmp = pnp_read_config(dev, PNP_IDX_EN);

/* Handle virtual devices, which share the same LDN register. */
    bitpos = (dev->path.pnp.device >> 8) & 0x7;
@@ -66,14 +67,14 @@
    else
    	tmp &= ~(1 << bitpos);

-	pnp_write_config(dev, 0x30, tmp);
+	pnp_write_config(dev, PNP_IDX_EN, tmp);
 }

int pnp_read_enable(struct device *dev)
 {
    u8 tmp, bitpos;

-	tmp = pnp_read_config(dev, 0x30);
+	tmp = pnp_read_config(dev, PNP_IDX_EN);

/* Handle virtual devices, which share the same LDN register. */
    bitpos = (dev->path.pnp.device >> 8) & 0x7;
diff --git a/src/include/device/pnp_ops.h b/src/include/device/pnp_ops.h
index 0cfdd61..18b35be 100644
--- a/src/include/device/pnp_ops.h
+++ b/src/include/device/pnp_ops.h
@@ -6,6 +6,7 @@
 #include <stdint.h>
 #include <arch/io.h>
 #include <device/pnp.h>
+#include <device/pnp_def.h>
 #include <device/pnp_type.h>

#if ENV_PNP_SIMPLE_DEVICE
@@ -32,13 +33,13 @@
 static __always_inline
 void pnp_set_enable(pnp_devfn_t dev, int enable)
 {
-	pnp_write_config(dev, 0x30, enable?0x1:0x0);
+	pnp_write_config(dev, PNP_IDX_EN, enable?0x1:0x0);
 }

static __always_inline
 int pnp_read_enable(pnp_devfn_t dev)
 {
-	return !!pnp_read_config(dev, 0x30);
+	return !!pnp_read_config(dev, PNP_IDX_EN);
 }

static __always_inline

[HAOUAS Elyes (Code Review)]

Hello build bot (Jenkins),

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/44835

to look at the new patch set (#2).

Change subject: src/{device,include}: Use PNP_IDX_EN instead of magic number
......................................................................

src/{device,include}: Use PNP_IDX_EN instead of magic number

Change-Id: I68590605e261ecaace9f3cea28cfa6ec3b913a8a
Signed-off-by: Elyes HAOUAS 
---
M src/device/pnp_device.c
M src/include/device/pnp_ops.h
2 files changed, 6 insertions(+), 5 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/35/44835/2

[Felix Held (Code Review)]

Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44835 )

Change subject: src/{device,include}: Use PNP_IDX_EN instead of magic number
......................................................................

Patch Set 2: Code-Review+1

(1 comment)

https://review.coreboot.org/c/coreboot/+/44835/2/src/device/pnp_device.c 
File src/device/pnp_device.c:

https://review.coreboot.org/c/coreboot/+/44835/2/src/device/pnp_device.c@7 
PS2, Line 7: #include <device/pnp.h>
is it intended that device/pnp_def.h is included indirectly via this one?

[HAOUAS Elyes (Code Review)]

HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44835 )

Change subject: src/{device,include}: Use PNP_IDX_EN instead of magic number
......................................................................

Patch Set 2:

(1 comment)

Thank you

https://review.coreboot.org/c/coreboot/+/44835/2/src/device/pnp_device.c 
File src/device/pnp_device.c:

https://review.coreboot.org/c/coreboot/+/44835/2/src/device/pnp_device.c@7 
PS2, Line 7: #include <device/pnp.h>
> is it intended that device/pnp_def. […]

I forget to add that include.
But I like the idea to suppose that <device/pnp.h> provides indirectly <device/pnp_def.h> (when device/pnp.h is needed).

it needs a separate patch (if you also like the idea).

[HAOUAS Elyes (Code Review)]

HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44835 )

Change subject: src/{device,include}: Use PNP_IDX_EN instead of magic number
......................................................................

Patch Set 2:

(1 comment)

<device/pnp.h> did not use <device/pnp_def.h>...

https://review.coreboot.org/c/coreboot/+/44835/2/src/device/pnp_device.c 
File src/device/pnp_device.c:

https://review.coreboot.org/c/coreboot/+/44835/2/src/device/pnp_device.c@7 
PS2, Line 7: #include <device/pnp.h>
> I forget to add that include. […]

humm, looks like <device/pnp.h> didn't use <device/pnp_def.h> ...

[HAOUAS Elyes (Code Review)]

Hello build bot (Jenkins), Felix Held,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/44835

to look at the new patch set (#3).

Change subject: src/{device,include}: Use PNP_IDX_EN instead of magic number
......................................................................

src/{device,include}: Use PNP_IDX_EN instead of magic number

Change-Id: I68590605e261ecaace9f3cea28cfa6ec3b913a8a
Signed-off-by: Elyes HAOUAS 
---
M src/device/pnp_device.c
M src/include/device/pnp_ops.h
2 files changed, 7 insertions(+), 5 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/35/44835/3

[Felix Held (Code Review)]

Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44835 )

Change subject: src/{device,include}: Use PNP_IDX_EN instead of magic number
......................................................................

Patch Set 3: Code-Review+2

(1 comment)

https://review.coreboot.org/c/coreboot/+/44835/2/src/device/pnp_device.c 
File src/device/pnp_device.c:

https://review.coreboot.org/c/coreboot/+/44835/2/src/device/pnp_device.c@7 
PS2, Line 7: #include <device/pnp.h>
> humm, looks like <device/pnp.h> didn't use <device/pnp_def.h> ...

i'm not sure if it would be a good idea to remove device/pnp_def.h from device/pnp.h, but that discussion should happen on the other patch

[Felix Held (Code Review)]

Felix Held has submitted this change. ( https://review.coreboot.org/c/coreboot/+/44835 )

Change subject: src/{device,include}: Use PNP_IDX_EN instead of magic number
......................................................................

src/{device,include}: Use PNP_IDX_EN instead of magic number

Change-Id: I68590605e261ecaace9f3cea28cfa6ec3b913a8a
Signed-off-by: Elyes HAOUAS 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/44835
Tested-by: build bot (Jenkins) 
Reviewed-by: Felix Held 
---
M src/device/pnp_device.c
M src/include/device/pnp_ops.h
2 files changed, 7 insertions(+), 5 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Felix Held: Looks good to me, approved

diff --git a/src/device/pnp_device.c b/src/device/pnp_device.c
index cd7adf3..9fa032e 100644
--- a/src/device/pnp_device.c
+++ b/src/device/pnp_device.c
@@ -5,6 +5,7 @@
 #include <arch/io.h>
 #include <device/device.h>
 #include <device/pnp.h>
+#include <device/pnp_def.h>

/* PNP config mode wrappers */

@@ -56,7 +57,7 @@
 {
    u8 tmp, bitpos;

-	tmp = pnp_read_config(dev, 0x30);
+	tmp = pnp_read_config(dev, PNP_IDX_EN);

/* Handle virtual devices, which share the same LDN register. */
    bitpos = (dev->path.pnp.device >> 8) & 0x7;
@@ -66,14 +67,14 @@
    else
    	tmp &= ~(1 << bitpos);

-	pnp_write_config(dev, 0x30, tmp);
+	pnp_write_config(dev, PNP_IDX_EN, tmp);
 }

int pnp_read_enable(struct device *dev)
 {
    u8 tmp, bitpos;

-	tmp = pnp_read_config(dev, 0x30);
+	tmp = pnp_read_config(dev, PNP_IDX_EN);

/* Handle virtual devices, which share the same LDN register. */
    bitpos = (dev->path.pnp.device >> 8) & 0x7;
diff --git a/src/include/device/pnp_ops.h b/src/include/device/pnp_ops.h
index 0cfdd61..18b35be 100644
--- a/src/include/device/pnp_ops.h
+++ b/src/include/device/pnp_ops.h
@@ -6,6 +6,7 @@
 #include <stdint.h>
 #include <arch/io.h>
 #include <device/pnp.h>
+#include <device/pnp_def.h>
 #include <device/pnp_type.h>

#if ENV_PNP_SIMPLE_DEVICE
@@ -32,13 +33,13 @@
 static __always_inline
 void pnp_set_enable(pnp_devfn_t dev, int enable)
 {
-	pnp_write_config(dev, 0x30, enable?0x1:0x0);
+	pnp_write_config(dev, PNP_IDX_EN, enable?0x1:0x0);
 }

static __always_inline
 int pnp_read_enable(pnp_devfn_t dev)
 {
-	return !!pnp_read_config(dev, 0x30);
+	return !!pnp_read_config(dev, PNP_IDX_EN);
 }

static __always_inline