Change in coreboot[master]: drivers/i2c/sx9324: Add more registers and reorder

[EricR Lai (Code Review)]

EricR Lai has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/47866 )

Change subject: drivers/i2c/sx9324: Add more registers and reorder
......................................................................

drivers/i2c/sx9324: Add more registers and reorder

Export all registers that driver is looking for. And put in alphabet order.
The missing registers are:
reg_irq_msk
reg_irq_cfg0
reg_irq_cfg2
reg_afe_ph0/1/2/3

BUG=b:172397658
BRANCH=zork
TEST=Build passed

Signed-off-by: Eric Lai 
Change-Id: Ic9d7a959b1769b6846bba302e3aeab9a3a1cedac
---
M src/drivers/i2c/sx9324/registers.h
1 file changed, 32 insertions(+), 23 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/66/47866/1

diff --git a/src/drivers/i2c/sx9324/registers.h b/src/drivers/i2c/sx9324/registers.h
index 160e5e7..7d00261 100644
--- a/src/drivers/i2c/sx9324/registers.h
+++ b/src/drivers/i2c/sx9324/registers.h
@@ -4,29 +4,6 @@
 #error "define REGISTER(NAME) before including this file"
 #endif

-REGISTER(reg_gnrl_ctrl0);
-REGISTER(reg_gnrl_ctrl1);
-
-REGISTER(reg_afe_ctrl0);
-REGISTER(reg_afe_ctrl1);
-REGISTER(reg_afe_ctrl2);
-REGISTER(reg_afe_ctrl3);
-REGISTER(reg_afe_ctrl4);
-REGISTER(reg_afe_ctrl5);
-REGISTER(reg_afe_ctrl6);
-REGISTER(reg_afe_ctrl7);
-REGISTER(reg_afe_ctrl8);
-REGISTER(reg_afe_ctrl9);
-
-REGISTER(reg_prox_ctrl0);
-REGISTER(reg_prox_ctrl1);
-REGISTER(reg_prox_ctrl2);
-REGISTER(reg_prox_ctrl3);
-REGISTER(reg_prox_ctrl4);
-REGISTER(reg_prox_ctrl5);
-REGISTER(reg_prox_ctrl6);
-REGISTER(reg_prox_ctrl7);
-
 REGISTER(reg_adv_ctrl0);
 REGISTER(reg_adv_ctrl1);
 REGISTER(reg_adv_ctrl2);
@@ -48,3 +25,35 @@
 REGISTER(reg_adv_ctrl18);
 REGISTER(reg_adv_ctrl19);
 REGISTER(reg_adv_ctrl20);
+
+REGISTER(reg_afe_ctrl0);
+REGISTER(reg_afe_ctrl1);
+REGISTER(reg_afe_ctrl2);
+REGISTER(reg_afe_ctrl3);
+REGISTER(reg_afe_ctrl4);
+REGISTER(reg_afe_ctrl5);
+REGISTER(reg_afe_ctrl6);
+REGISTER(reg_afe_ctrl7);
+REGISTER(reg_afe_ctrl8);
+REGISTER(reg_afe_ctrl9);
+
+REGISTER(reg_afe_ph0);
+REGISTER(reg_afe_ph1);
+REGISTER(reg_afe_ph2);
+REGISTER(reg_afe_ph3);
+
+REGISTER(reg_gnrl_ctrl0);
+REGISTER(reg_gnrl_ctrl1);
+
+REGISTER(reg_irq_msk);
+REGISTER(reg_irq_cfg0);
+REGISTER(reg_irq_cfg2);
+
+REGISTER(reg_prox_ctrl0);
+REGISTER(reg_prox_ctrl1);
+REGISTER(reg_prox_ctrl2);
+REGISTER(reg_prox_ctrl3);
+REGISTER(reg_prox_ctrl4);
+REGISTER(reg_prox_ctrl5);
+REGISTER(reg_prox_ctrl6);
+REGISTER(reg_prox_ctrl7);

[Paul Menzel (Code Review)]

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47866 )

Change subject: drivers/i2c/sx9324: Add more registers and reorder
......................................................................

Patch Set 1:

(2 comments)

https://review.coreboot.org/c/coreboot/+/47866/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/47866/1//COMMIT_MSG@9 
PS1, Line 9: alphabet
alphabetic

https://review.coreboot.org/c/coreboot/+/47866/1//COMMIT_MSG@15 
PS1, Line 15: 
Please name the datasheet name and revision.

[EricR Lai (Code Review)]

Hello build bot (Jenkins), Furquan Shaikh, Tim Wawrzynczak, Angel Pons,

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

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

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

Change subject: drivers/i2c/sx9324: Add more registers and reorder
......................................................................

drivers/i2c/sx9324: Add more registers and reorder

Export all registers that driver is looking for. And put in alphabetic order.
The missing registers for kernel v5.4 sx93xx are:
reg_irq_msk
reg_irq_cfg0
reg_irq_cfg2
reg_afe_ph0/1/2/3

BUG=b:172397658
BRANCH=zork
TEST=Build passed

Signed-off-by: Eric Lai 
Change-Id: Ic9d7a959b1769b6846bba302e3aeab9a3a1cedac
---
M src/drivers/i2c/sx9324/registers.h
1 file changed, 32 insertions(+), 23 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/66/47866/2

[EricR Lai (Code Review)]

EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47866 )

Change subject: drivers/i2c/sx9324: Add more registers and reorder
......................................................................

Patch Set 2:

(2 comments)

https://review.coreboot.org/c/coreboot/+/47866/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/47866/1//COMMIT_MSG@9 
PS1, Line 9: alphabet
> alphabetic

Done

https://review.coreboot.org/c/coreboot/+/47866/1//COMMIT_MSG@15 
PS1, Line 15:
> Please name the datasheet name and revision.

This is reflected from kernel driver. I put the kernel version and driver name.

[Tim Wawrzynczak (Code Review)]

Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47866 )

Change subject: drivers/i2c/sx9324: Add more registers and reorder
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/47866/2/src/drivers/i2c/sx9324/regi... 
File src/drivers/i2c/sx9324/registers.h:

https://review.coreboot.org/c/coreboot/+/47866/2/src/drivers/i2c/sx9324/regi... 
PS2, Line 7: REGISTER(reg_adv_ctrl0);
           : REGISTER(reg_adv_ctrl1);
           : REGISTER(reg_adv_ctrl2);
           : REGISTER(reg_adv_ctrl3);
           : REGISTER(reg_adv_ctrl4);
           : REGISTER(reg_adv_ctrl5);
           : REGISTER(reg_adv_ctrl6);
           : REGISTER(reg_adv_ctrl7);
           : REGISTER(reg_adv_ctrl8);
           : REGISTER(reg_adv_ctrl9);
           : REGISTER(reg_adv_ctrl10);
           : REGISTER(reg_adv_ctrl11);
           : REGISTER(reg_adv_ctrl12);
           : REGISTER(reg_adv_ctrl13);
           : REGISTER(reg_adv_ctrl14);
           : REGISTER(reg_adv_ctrl15);
           : REGISTER(reg_adv_ctrl16);
           : REGISTER(reg_adv_ctrl17);
           : REGISTER(reg_adv_ctrl18);
           : REGISTER(reg_adv_ctrl19);
           : REGISTER(reg_adv_ctrl20);
           : 
           : REGISTER(reg_afe_ctrl0);
           : REGISTER(reg_afe_ctrl1);
           : REGISTER(reg_afe_ctrl2);
           : REGISTER(reg_afe_ctrl3);
           : REGISTER(reg_afe_ctrl4);
           : REGISTER(reg_afe_ctrl5);
           : REGISTER(reg_afe_ctrl6);
           : REGISTER(reg_afe_ctrl7);
           : REGISTER(reg_afe_ctrl8);
           : REGISTER(reg_afe_ctrl9);
           : 
           : REGISTER(reg_afe_ph0);
           : REGISTER(reg_afe_ph1);
           : REGISTER(reg_afe_ph2);
           : REGISTER(reg_afe_ph3);
           : 
           : REGISTER(reg_gnrl_ctrl0);
           : REGISTER(reg_gnrl_ctrl1);
           : 
           : REGISTER(reg_irq_msk);
           : REGISTER(reg_irq_cfg0);
           : REGISTER(reg_irq_cfg2);
           : 
           : REGISTER(reg_prox_ctrl0);
           : REGISTER(reg_prox_ctrl1);
           : REGISTER(reg_prox_ctrl2);
           : REGISTER(reg_prox_ctrl3);
           : REGISTER(reg_prox_ctrl4);
           : REGISTER(reg_prox_ctrl5);
           : REGISTER(reg_prox_ctrl6);
           : REGISTER(reg_prox_ctrl7);
Hey Eric, I wonder if maybe viewing this mostly as separate arrays would make this cleaner than resorting to X macros (https://en.wikipedia.org/wiki/X_Macro).

For example:
```
struct drivers_i2c_sx9234_config {
       ....
       uint8_t adv_ctrl[21];
       uint8_t afe_ctrl[10];
       uint8_t afe_ph[4];
       uint8_t gnrl_ctrl[2];
       uint8_t irq_mask;
       uint8_t irq_cfg0;
       uint8_t irq_cfg2;
       uint8_t prox_ctrl[8];
}
```

This comes with the advantage that many of the register settings can be "collapsed" in the devicetree, e.g.:
instead of:
```
    		register "reg_afe_ctrl0" = "0x00"
    		register "reg_afe_ctrl1" = "0x10"
    		register "reg_afe_ctrl2" = "0x00"
    		register "reg_afe_ctrl3" = "0x00"
    		register "reg_afe_ctrl4" = "0x07"
    		register "reg_afe_ctrl5" = "0x00"
    		register "reg_afe_ctrl6" = "0x00"
    		register "reg_afe_ctrl7" = "0x07"
    		register "reg_afe_ctrl8" = "0x12"
    		register "reg_afe_ctrl9" = "0x0f"
```
it looks like:
```
    		register "reg_afe_ctrl" = "{0x00, 0x10, 0x00, 0x00, 0x07, 0x00, 0x00, 0x07, 0x12, 0x0f}"
```

what do you think?

[EricR Lai (Code Review)]

EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47866 )

Change subject: drivers/i2c/sx9324: Add more registers and reorder
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/47866/2/src/drivers/i2c/sx9324/regi... 
File src/drivers/i2c/sx9324/registers.h:

https://review.coreboot.org/c/coreboot/+/47866/2/src/drivers/i2c/sx9324/regi... 
PS2, Line 7: REGISTER(reg_adv_ctrl0);
           : REGISTER(reg_adv_ctrl1);
           : REGISTER(reg_adv_ctrl2);
           : REGISTER(reg_adv_ctrl3);
           : REGISTER(reg_adv_ctrl4);
           : REGISTER(reg_adv_ctrl5);
           : REGISTER(reg_adv_ctrl6);
           : REGISTER(reg_adv_ctrl7);
           : REGISTER(reg_adv_ctrl8);
           : REGISTER(reg_adv_ctrl9);
           : REGISTER(reg_adv_ctrl10);
           : REGISTER(reg_adv_ctrl11);
           : REGISTER(reg_adv_ctrl12);
           : REGISTER(reg_adv_ctrl13);
           : REGISTER(reg_adv_ctrl14);
           : REGISTER(reg_adv_ctrl15);
           : REGISTER(reg_adv_ctrl16);
           : REGISTER(reg_adv_ctrl17);
           : REGISTER(reg_adv_ctrl18);
           : REGISTER(reg_adv_ctrl19);
           : REGISTER(reg_adv_ctrl20);
           : 
           : REGISTER(reg_afe_ctrl0);
           : REGISTER(reg_afe_ctrl1);
           : REGISTER(reg_afe_ctrl2);
           : REGISTER(reg_afe_ctrl3);
           : REGISTER(reg_afe_ctrl4);
           : REGISTER(reg_afe_ctrl5);
           : REGISTER(reg_afe_ctrl6);
           : REGISTER(reg_afe_ctrl7);
           : REGISTER(reg_afe_ctrl8);
           : REGISTER(reg_afe_ctrl9);
           : 
           : REGISTER(reg_afe_ph0);
           : REGISTER(reg_afe_ph1);
           : REGISTER(reg_afe_ph2);
           : REGISTER(reg_afe_ph3);
           : 
           : REGISTER(reg_gnrl_ctrl0);
           : REGISTER(reg_gnrl_ctrl1);
           : 
           : REGISTER(reg_irq_msk);
           : REGISTER(reg_irq_cfg0);
           : REGISTER(reg_irq_cfg2);
           : 
           : REGISTER(reg_prox_ctrl0);
           : REGISTER(reg_prox_ctrl1);
           : REGISTER(reg_prox_ctrl2);
           : REGISTER(reg_prox_ctrl3);
           : REGISTER(reg_prox_ctrl4);
           : REGISTER(reg_prox_ctrl5);
           : REGISTER(reg_prox_ctrl6);
           : REGISTER(reg_prox_ctrl7);
> Hey Eric, I wonder if maybe viewing this mostly as separate arrays would make this cleaner than reso […]

This problem is sx9324.c. This use the trick redefine marco to generate the string name.
If you can give the new marco for the structure, I think it is fine to me.
I have another CL for totally refactor it https://review.coreboot.org/c/coreboot/+/47639.

#define REGISTER(NAME) acpi_dp_add_integer(dsd, \
    				I2C_SX9324_ACPI_ID "," #NAME, \
    				config->NAME)

[EricR Lai (Code Review)]

EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47866 )

Change subject: drivers/i2c/sx9324: Add more registers and reorder
......................................................................

Patch Set 2:

@Tim, any suggestion here?

[Tim Wawrzynczak (Code Review)]

Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47866 )

Change subject: drivers/i2c/sx9324: Add more registers and reorder
......................................................................

Patch Set 2: Code-Review+2

(1 comment)

https://review.coreboot.org/c/coreboot/+/47866/2/src/drivers/i2c/sx9324/regi... 
File src/drivers/i2c/sx9324/registers.h:

https://review.coreboot.org/c/coreboot/+/47866/2/src/drivers/i2c/sx9324/regi... 
PS2, Line 7: REGISTER(reg_adv_ctrl0);
           : REGISTER(reg_adv_ctrl1);
           : REGISTER(reg_adv_ctrl2);
           : REGISTER(reg_adv_ctrl3);
           : REGISTER(reg_adv_ctrl4);
           : REGISTER(reg_adv_ctrl5);
           : REGISTER(reg_adv_ctrl6);
           : REGISTER(reg_adv_ctrl7);
           : REGISTER(reg_adv_ctrl8);
           : REGISTER(reg_adv_ctrl9);
           : REGISTER(reg_adv_ctrl10);
           : REGISTER(reg_adv_ctrl11);
           : REGISTER(reg_adv_ctrl12);
           : REGISTER(reg_adv_ctrl13);
           : REGISTER(reg_adv_ctrl14);
           : REGISTER(reg_adv_ctrl15);
           : REGISTER(reg_adv_ctrl16);
           : REGISTER(reg_adv_ctrl17);
           : REGISTER(reg_adv_ctrl18);
           : REGISTER(reg_adv_ctrl19);
           : REGISTER(reg_adv_ctrl20);
           : 
           : REGISTER(reg_afe_ctrl0);
           : REGISTER(reg_afe_ctrl1);
           : REGISTER(reg_afe_ctrl2);
           : REGISTER(reg_afe_ctrl3);
           : REGISTER(reg_afe_ctrl4);
           : REGISTER(reg_afe_ctrl5);
           : REGISTER(reg_afe_ctrl6);
           : REGISTER(reg_afe_ctrl7);
           : REGISTER(reg_afe_ctrl8);
           : REGISTER(reg_afe_ctrl9);
           : 
           : REGISTER(reg_afe_ph0);
           : REGISTER(reg_afe_ph1);
           : REGISTER(reg_afe_ph2);
           : REGISTER(reg_afe_ph3);
           : 
           : REGISTER(reg_gnrl_ctrl0);
           : REGISTER(reg_gnrl_ctrl1);
           : 
           : REGISTER(reg_irq_msk);
           : REGISTER(reg_irq_cfg0);
           : REGISTER(reg_irq_cfg2);
           : 
           : REGISTER(reg_prox_ctrl0);
           : REGISTER(reg_prox_ctrl1);
           : REGISTER(reg_prox_ctrl2);
           : REGISTER(reg_prox_ctrl3);
           : REGISTER(reg_prox_ctrl4);
           : REGISTER(reg_prox_ctrl5);
           : REGISTER(reg_prox_ctrl6);
           : REGISTER(reg_prox_ctrl7);
> This problem is sx9324.c. This use the trick redefine marco to generate the string name. […]

Looks like you're working on something to improve this, so go ahead with this.

[Patrick Georgi (Code Review)]

Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/47866 )

Change subject: drivers/i2c/sx9324: Add more registers and reorder
......................................................................

drivers/i2c/sx9324: Add more registers and reorder

Export all registers that driver is looking for. And put in alphabetic order.
The missing registers for kernel v5.4 sx93xx are:
reg_irq_msk
reg_irq_cfg0
reg_irq_cfg2
reg_afe_ph0/1/2/3

BUG=b:172397658
BRANCH=zork
TEST=Build passed

Signed-off-by: Eric Lai 
Change-Id: Ic9d7a959b1769b6846bba302e3aeab9a3a1cedac
Reviewed-on: https://review.coreboot.org/c/coreboot/+/47866
Reviewed-by: Tim Wawrzynczak 
Tested-by: build bot (Jenkins) 
---
M src/drivers/i2c/sx9324/registers.h
1 file changed, 32 insertions(+), 23 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Tim Wawrzynczak: Looks good to me, approved

diff --git a/src/drivers/i2c/sx9324/registers.h b/src/drivers/i2c/sx9324/registers.h
index 160e5e7..7d00261 100644
--- a/src/drivers/i2c/sx9324/registers.h
+++ b/src/drivers/i2c/sx9324/registers.h
@@ -4,29 +4,6 @@
 #error "define REGISTER(NAME) before including this file"
 #endif

-REGISTER(reg_gnrl_ctrl0);
-REGISTER(reg_gnrl_ctrl1);
-
-REGISTER(reg_afe_ctrl0);
-REGISTER(reg_afe_ctrl1);
-REGISTER(reg_afe_ctrl2);
-REGISTER(reg_afe_ctrl3);
-REGISTER(reg_afe_ctrl4);
-REGISTER(reg_afe_ctrl5);
-REGISTER(reg_afe_ctrl6);
-REGISTER(reg_afe_ctrl7);
-REGISTER(reg_afe_ctrl8);
-REGISTER(reg_afe_ctrl9);
-
-REGISTER(reg_prox_ctrl0);
-REGISTER(reg_prox_ctrl1);
-REGISTER(reg_prox_ctrl2);
-REGISTER(reg_prox_ctrl3);
-REGISTER(reg_prox_ctrl4);
-REGISTER(reg_prox_ctrl5);
-REGISTER(reg_prox_ctrl6);
-REGISTER(reg_prox_ctrl7);
-
 REGISTER(reg_adv_ctrl0);
 REGISTER(reg_adv_ctrl1);
 REGISTER(reg_adv_ctrl2);
@@ -48,3 +25,35 @@
 REGISTER(reg_adv_ctrl18);
 REGISTER(reg_adv_ctrl19);
 REGISTER(reg_adv_ctrl20);
+
+REGISTER(reg_afe_ctrl0);
+REGISTER(reg_afe_ctrl1);
+REGISTER(reg_afe_ctrl2);
+REGISTER(reg_afe_ctrl3);
+REGISTER(reg_afe_ctrl4);
+REGISTER(reg_afe_ctrl5);
+REGISTER(reg_afe_ctrl6);
+REGISTER(reg_afe_ctrl7);
+REGISTER(reg_afe_ctrl8);
+REGISTER(reg_afe_ctrl9);
+
+REGISTER(reg_afe_ph0);
+REGISTER(reg_afe_ph1);
+REGISTER(reg_afe_ph2);
+REGISTER(reg_afe_ph3);
+
+REGISTER(reg_gnrl_ctrl0);
+REGISTER(reg_gnrl_ctrl1);
+
+REGISTER(reg_irq_msk);
+REGISTER(reg_irq_cfg0);
+REGISTER(reg_irq_cfg2);
+
+REGISTER(reg_prox_ctrl0);
+REGISTER(reg_prox_ctrl1);
+REGISTER(reg_prox_ctrl2);
+REGISTER(reg_prox_ctrl3);
+REGISTER(reg_prox_ctrl4);
+REGISTER(reg_prox_ctrl5);
+REGISTER(reg_prox_ctrl6);
+REGISTER(reg_prox_ctrl7);