Change in flashrom[master]: realtek_mst_i2c_spi.c: Trigger gpio 88 toggle down after write

[Shiyu Sun (Code Review)]

Shiyu Sun has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/46448 )

Change subject: realtek_mst_i2c_spi.c: Trigger gpio 88 toggle down after write
......................................................................

realtek_mst_i2c_spi.c: Trigger gpio 88 toggle down after write

BUG=b:152558985,b:148745673
BRANCH=none
TEST=builds

Change-Id: I1407714e1bb4cf2472090bae8a613c7103a5938c
Signed-off-by: Shiyu Sun 
---
M realtek_mst_i2c_spi.c
1 file changed, 1 insertion(+), 2 deletions(-)

git pull ssh://review.coreboot.org:29418/flashrom refs/changes/48/46448/1

diff --git a/realtek_mst_i2c_spi.c b/realtek_mst_i2c_spi.c
index 63ff510..43cf25b 100644
--- a/realtek_mst_i2c_spi.c
+++ b/realtek_mst_i2c_spi.c
@@ -393,8 +393,7 @@
    		break;
    }

-
-	/* TODO: re-enable the write protection? */
+	ret |= realtek_mst_i2c_spi_toggle_gpio_88_strap(fd, false);

return ret;
 }

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/46448 )

Change subject: realtek_mst_i2c_spi.c: Trigger gpio 88 toggle down after write
......................................................................

Patch Set 1: Code-Review+2

I'd have a `const bool wp_enable` to see how this toggles the write-protect state:

/* Write-protect is active low */
 const bool wp_enable = false;

realtek_mst_i2c_spi_toggle_gpio_88_strap(fd, !wp_enabled);

/* do stuff */

realtek_mst_i2c_spi_toggle_gpio_88_strap(fd, wp_enabled);

But then again, it's not really a big deal.

[Edward O'Callaghan (Code Review)]

Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/46448 )

Change subject: realtek_mst_i2c_spi.c: Trigger gpio 88 toggle down after write
......................................................................

Patch Set 1: Code-Review+2
> Patch Set 1: Code-Review+2
>
> I'd have a `const bool wp_enable` to see how this toggles the write-protect state:
>
> /* Write-protect is active low */
>  const bool wp_enable = false;
>
> realtek_mst_i2c_spi_toggle_gpio_88_strap(fd, !wp_enabled);
>
> /* do stuff */
>
> realtek_mst_i2c_spi_toggle_gpio_88_strap(fd, wp_enabled);
>
> But then again, it's not really a big deal.

I think we should collectively move Flashrom away from having so many singleton states in each spi master and not more. The above feels influenced by Haskell do-notation, did you happen to program in Haskell? :)

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/46448 )

Change subject: realtek_mst_i2c_spi.c: Trigger gpio 88 toggle down after write
......................................................................

Patch Set 1:
> Patch Set 1: Code-Review+2
> > Patch Set 1: Code-Review+2
> >
> > I'd have a `const bool wp_enable` to see how this toggles the write-protect state:
> >
> > /* Write-protect is active low */
> >  const bool wp_enable = false;
> >
> > realtek_mst_i2c_spi_toggle_gpio_88_strap(fd, !wp_enabled);
> >
> > /* do stuff */
> >
> > realtek_mst_i2c_spi_toggle_gpio_88_strap(fd, wp_enabled);
> >
> > But then again, it's not really a big deal.
>
> I think we should collectively move Flashrom away from having so many singleton states in each spi master and not more. The above feels influenced by Haskell do-notation, did you happen to program in Haskell? :)

Nope, I've never programmed in Haskell. The closest thing I've done is to program Haswell's memory controller 😄

[Edward O'Callaghan (Code Review)]

Edward O'Callaghan has submitted this change. ( https://review.coreboot.org/c/flashrom/+/46448 )

Change subject: realtek_mst_i2c_spi.c: Trigger gpio 88 toggle down after write
......................................................................

realtek_mst_i2c_spi.c: Trigger gpio 88 toggle down after write

BUG=b:152558985,b:148745673
BRANCH=none
TEST=builds

Change-Id: I1407714e1bb4cf2472090bae8a613c7103a5938c
Signed-off-by: Shiyu Sun 
Reviewed-on: https://review.coreboot.org/c/flashrom/+/46448
Reviewed-by: Angel Pons 
Reviewed-by: Edward O'Callaghan 
Tested-by: build bot (Jenkins) 
---
M realtek_mst_i2c_spi.c
1 file changed, 1 insertion(+), 2 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Angel Pons: Looks good to me, approved
  Edward O'Callaghan: Looks good to me, approved

diff --git a/realtek_mst_i2c_spi.c b/realtek_mst_i2c_spi.c
index 63ff510..43cf25b 100644
--- a/realtek_mst_i2c_spi.c
+++ b/realtek_mst_i2c_spi.c
@@ -393,8 +393,7 @@
    		break;
    }

-
-	/* TODO: re-enable the write protection? */
+	ret |= realtek_mst_i2c_spi_toggle_gpio_88_strap(fd, false);

return ret;
 }