Hello Hung-Te Lin, Nico Huber,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/37432
to review the following change.
Change subject: mmio: Add clrsetbitsXX() API in place of updateX() ......................................................................
mmio: Add clrsetbitsXX() API in place of updateX()
This patch removes the recently added update8/16/32/64() API and replaces it with clrsetbits8/16/32/64(). This is more in line with the existing endian-specific clrsetbits_le16/32/64() functions that have been used for this task on some platforms already. Rename clrsetbits_8() to clrsetbits8() to be in line with the new naming.
Keep this stuff in <device/mmio.h> and get rid of <mmio.h> again because having both is confusing and we seem to have been standardizing on <device/mmio.h> as the standard arch-independent header that all platforms should include already.
Also sync libpayload back up with what we have in coreboot. (I'm the original author of the clrsetbits_le32-definitions so I'm relicensing them to BSD here.)
Change-Id: Ie4f7b9fdbdf9e8c0174427b4288f79006d56978b Signed-off-by: Julius Werner jwerner@chromium.org --- M payloads/libpayload/include/endian.h M src/drivers/maxim/max77686/max77686.c M src/include/device/mmio.h M src/include/endian.h D src/include/mmio.h 5 files changed, 62 insertions(+), 65 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/32/37432/1
diff --git a/payloads/libpayload/include/endian.h b/payloads/libpayload/include/endian.h index b387e66..ec0825d 100644 --- a/payloads/libpayload/include/endian.h +++ b/payloads/libpayload/include/endian.h @@ -181,18 +181,47 @@
/* Handy bit manipulation macros */
-#define clrsetbits_le32(addr, clear, set) writel(htole32((le32toh(readl(addr)) \ - & ~(clear)) | (set)), (addr)) -#define setbits_le32(addr, set) writel(htole32(le32toh(readl(addr)) \ - | (set)), (addr)) -#define clrbits_le32(addr, clear) writel(htole32(le32toh(readl(addr)) \ - & ~(clear)), (addr)) +#define __clrsetbits(endian, bits, addr, clear, set) \ + write##bits(addr, hto##endian##bits((endian##bits##toh( \ + read##bits(addr)) & ~((uint##bits##_t)(clear))) | (set)))
-#define clrsetbits_be32(addr, clear, set) writel(htobe32((be32toh(readl(addr)) \ - & ~(clear)) | (set)), (addr)) -#define setbits_be32(addr, set) writel(htobe32(be32toh(readl(addr)) \ - | (set)), (addr)) -#define clrbits_be32(addr, clear) writel(htobe32(be32toh(readl(addr)) \ - & ~(clear)), (addr)) +#define clrbits_le64(addr, clear) __clrsetbits(le, 64, addr, clear, 0) +#define clrbits_be64(addr, clear) __clrsetbits(be, 64, addr, clear, 0) +#define clrbits_le32(addr, clear) __clrsetbits(le, 32, addr, clear, 0) +#define clrbits_be32(addr, clear) __clrsetbits(be, 32, addr, clear, 0) +#define clrbits_le16(addr, clear) __clrsetbits(le, 16, addr, clear, 0) +#define clrbits_be16(addr, clear) __clrsetbits(be, 16, addr, clear, 0) + +#define setbits_le64(addr, set) __clrsetbits(le, 64, addr, 0, set) +#define setbits_be64(addr, set) __clrsetbits(be, 64, addr, 0, set) +#define setbits_le32(addr, set) __clrsetbits(le, 32, addr, 0, set) +#define setbits_be32(addr, set) __clrsetbits(be, 32, addr, 0, set) +#define setbits_le16(addr, set) __clrsetbits(le, 16, addr, 0, set) +#define setbits_be16(addr, set) __clrsetbits(be, 16, addr, 0, set) + +#define clrsetbits_le64(addr, clear, set) __clrsetbits(le, 64, addr, clear, set) +#define clrsetbits_be64(addr, clear, set) __clrsetbits(be, 64, addr, clear, set) +#define clrsetbits_le32(addr, clear, set) __clrsetbits(le, 32, addr, clear, set) +#define clrsetbits_be32(addr, clear, set) __clrsetbits(be, 32, addr, clear, set) +#define clrsetbits_le16(addr, clear, set) __clrsetbits(le, 16, addr, clear, set) +#define clrsetbits_be16(addr, clear, set) __clrsetbits(be, 16, addr, clear, set) + +#define __clrsetbits_impl(bits, addr, clear, set) write##bits(addr, \ + (read##bits(addr) & ~((uint##bits##_t)(clear))) | (set)) + +#define clrsetbits8(addr, clear, set) __clrsetbits_impl(8, addr, clear, set) +#define clrsetbits16(addr, clear, set) __clrsetbits_impl(16, addr, clear, set) +#define clrsetbits32(addr, clear, set) __clrsetbits_impl(32, addr, clear, set) +#define clrsetbits64(addr, clear, set) __clrsetbits_impl(64, addr, clear, set) + +#define setbits8(addr, set) clrsetbits8(addr, 0, set) +#define setbits16(addr, set) clrsetbits16(addr, 0, set) +#define setbits32(addr, set) clrsetbits32(addr, 0, set) +#define setbits64(addr, set) clrsetbits64(addr, 0, set) + +#define clrbits8(addr, clear) clrsetbits8(addr, clear, 0) +#define clrbits16(addr, clear) clrsetbits16(addr, clear, 0) +#define clrbits32(addr, clear) clrsetbits32(addr, clear, 0) +#define clrbits64(addr, clear) clrsetbits64(addr, clear, 0)
#endif /* _ENDIAN_H_ */ diff --git a/src/drivers/maxim/max77686/max77686.c b/src/drivers/maxim/max77686/max77686.c index 54e3a6c..cfbf912 100644 --- a/src/drivers/maxim/max77686/max77686.c +++ b/src/drivers/maxim/max77686/max77686.c @@ -127,10 +127,10 @@ }
if (enable == REG_DISABLE) { - clrbits_8(&read_data, + clrbits8(&read_data, pmic->reg_enbitmask << pmic->reg_enbitpos); } else { - clrsetbits_8(&read_data, + clrsetbits8(&read_data, pmic->reg_enbitmask << pmic->reg_enbitpos, pmic->reg_enbiton << pmic->reg_enbitpos); } @@ -177,7 +177,7 @@ } vol_level /= (u32)pmic->vol_div;
- clrsetbits_8(&read_data, pmic->vol_bitmask << pmic->vol_bitpos, + clrsetbits8(&read_data, pmic->vol_bitmask << pmic->vol_bitpos, vol_level << pmic->vol_bitpos);
ret = max77686_i2c_write(bus, MAX77686_I2C_ADDR, pmic->vol_addr, read_data); diff --git a/src/include/device/mmio.h b/src/include/device/mmio.h index 6596cf8..9c5e27c 100644 --- a/src/include/device/mmio.h +++ b/src/include/device/mmio.h @@ -19,6 +19,24 @@ #include <endian.h> #include <types.h>
+#define __clrsetbits_impl(bits, addr, clear, set) write##bits(addr, \ + (read##bits(addr) & ~((uint##bits##_t)(clear))) | (set)) + +#define clrsetbits8(addr, clear, set) __clrsetbits_impl(8, addr, clear, set) +#define clrsetbits16(addr, clear, set) __clrsetbits_impl(16, addr, clear, set) +#define clrsetbits32(addr, clear, set) __clrsetbits_impl(32, addr, clear, set) +#define clrsetbits64(addr, clear, set) __clrsetbits_impl(64, addr, clear, set) + +#define setbits8(addr, set) clrsetbits8(addr, 0, set) +#define setbits16(addr, set) clrsetbits16(addr, 0, set) +#define setbits32(addr, set) clrsetbits32(addr, 0, set) +#define setbits64(addr, set) clrsetbits64(addr, 0, set) + +#define clrbits8(addr, clear) clrsetbits8(addr, clear, 0) +#define clrbits16(addr, clear) clrsetbits16(addr, clear, 0) +#define clrbits32(addr, clear) clrsetbits32(addr, clear, 0) +#define clrbits64(addr, clear) clrsetbits64(addr, clear, 0) + #ifndef __ROMCC__ /* * Reads a transfer buffer from 32-bit FIFO registers. fifo_stride is the diff --git a/src/include/endian.h b/src/include/endian.h index 8dc1854..f16f668 100644 --- a/src/include/endian.h +++ b/src/include/endian.h @@ -79,11 +79,6 @@ #define clrsetbits_le16(addr, clear, set) __clrsetbits(le, 16, addr, clear, set) #define clrsetbits_be16(addr, clear, set) __clrsetbits(be, 16, addr, clear, set)
-#define clrsetbits_8(addr, clear, set) \ - write8(addr, (read8(addr) & ~(clear)) | (set)) -#define clrbits_8(addr, clear) clrsetbits_8(addr, clear, 0) -#define setbits_8(addr, set) setbits_8(addr, 0, set) - #ifndef __ROMCC__ /* be16dec/be32dec/be64dec/le16dec/le32dec/le64dec family of functions. */ #define DEFINE_ENDIAN_DEC(endian, width) \ diff --git a/src/include/mmio.h b/src/include/mmio.h deleted file mode 100644 index 4f2c806..0000000 --- a/src/include/mmio.h +++ /dev/null @@ -1,45 +0,0 @@ -/* - * This file is part of the coreboot project. - * - * This program is free software; you can redistribute it and/or modify - * it under the terms of the GNU General Public License as published by - * the Free Software Foundation; version 2 of the License. - * - * This program is distributed in the hope that it will be useful, - * but WITHOUT ANY WARRANTY; without even the implied warranty of - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the - * GNU General Public License for more details. - */ - -/* - * mmio.h provides update*() as well as read/write*() from arch/mmio.h - */ - -#ifndef __MMIO_H__ -#define __MMIO_H__ - -#include <arch/mmio.h> -#include <stdint.h> - -static __always_inline void update8(volatile void *addr, uint8_t mask, uint8_t or) -{ - uint8_t reg = read8(addr); - reg = (reg & mask) | or; - write8(addr, reg); -} - -static __always_inline void update16(volatile void *addr, uint16_t mask, uint16_t or) -{ - uint16_t reg = read16(addr); - reg = (reg & mask) | or; - write16(addr, reg); -} - -static __always_inline void update32(volatile void *addr, uint32_t mask, uint32_t or) -{ - uint32_t reg = read32(addr); - reg = (reg & mask) | or; - write32(addr, reg); -} - -#endif /* __MMIO_H__ */
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37432 )
Change subject: mmio: Add clrsetbitsXX() API in place of updateX() ......................................................................
Patch Set 1: Code-Review+2
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37432 )
Change subject: mmio: Add clrsetbitsXX() API in place of updateX() ......................................................................
Patch Set 1: Code-Review+2
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37432 )
Change subject: mmio: Add clrsetbitsXX() API in place of updateX() ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/37432/1/src/include/device/mmio.h File src/include/device/mmio.h:
https://review.coreboot.org/c/coreboot/+/37432/1/src/include/device/mmio.h@1... PS1, Line 123: _le32 ... and remove this
https://review.coreboot.org/c/coreboot/+/37432/1/src/include/device/mmio.h@1... PS1, Line 190: clrsetbits_le32 Can you change this as well?
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37432 )
Change subject: mmio: Add clrsetbitsXX() API in place of updateX() ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/37432/1/src/include/device/mmio.h File src/include/device/mmio.h:
https://review.coreboot.org/c/coreboot/+/37432/1/src/include/device/mmio.h@1... PS1, Line 123: _le32
... […]
Ack
https://review.coreboot.org/c/coreboot/+/37432/1/src/include/device/mmio.h@1... PS1, Line 190: clrsetbits_le32
Can you change this as well?
It's part of the next patch. I wanted to separate adding the macros and changing existing code.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37432 )
Change subject: mmio: Add clrsetbitsXX() API in place of updateX() ......................................................................
Patch Set 2: Code-Review+2
Any reason not to use inline functions? Currently, `addr` is always evaluated twice. It shouldn't matter in practice, just wondering.
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37432 )
Change subject: mmio: Add clrsetbitsXX() API in place of updateX() ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37432/1/src/include/device/mmio.h File src/include/device/mmio.h:
https://review.coreboot.org/c/coreboot/+/37432/1/src/include/device/mmio.h@1... PS1, Line 190: clrsetbits_le32
It's part of the next patch. I wanted to separate adding the macros and changing existing code.
Ack
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/37432 )
Change subject: mmio: Add clrsetbitsXX() API in place of updateX() ......................................................................
mmio: Add clrsetbitsXX() API in place of updateX()
This patch removes the recently added update8/16/32/64() API and replaces it with clrsetbits8/16/32/64(). This is more in line with the existing endian-specific clrsetbits_le16/32/64() functions that have been used for this task on some platforms already. Rename clrsetbits_8() to clrsetbits8() to be in line with the new naming.
Keep this stuff in <device/mmio.h> and get rid of <mmio.h> again because having both is confusing and we seem to have been standardizing on <device/mmio.h> as the standard arch-independent header that all platforms should include already.
Also sync libpayload back up with what we have in coreboot. (I'm the original author of the clrsetbits_le32-definitions so I'm relicensing them to BSD here.)
Change-Id: Ie4f7b9fdbdf9e8c0174427b4288f79006d56978b Signed-off-by: Julius Werner jwerner@chromium.org Reviewed-on: https://review.coreboot.org/c/coreboot/+/37432 Reviewed-by: Nico Huber nico.h@gmx.de Reviewed-by: Furquan Shaikh furquan@google.com Reviewed-by: Hung-Te Lin hungte@chromium.org Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M payloads/libpayload/include/endian.h M src/drivers/maxim/max77686/max77686.c M src/include/device/mmio.h M src/include/endian.h D src/include/mmio.h 5 files changed, 62 insertions(+), 65 deletions(-)
Approvals: build bot (Jenkins): Verified Nico Huber: Looks good to me, approved Furquan Shaikh: Looks good to me, approved Hung-Te Lin: Looks good to me, approved
diff --git a/payloads/libpayload/include/endian.h b/payloads/libpayload/include/endian.h index b387e66..ec0825d 100644 --- a/payloads/libpayload/include/endian.h +++ b/payloads/libpayload/include/endian.h @@ -181,18 +181,47 @@
/* Handy bit manipulation macros */
-#define clrsetbits_le32(addr, clear, set) writel(htole32((le32toh(readl(addr)) \ - & ~(clear)) | (set)), (addr)) -#define setbits_le32(addr, set) writel(htole32(le32toh(readl(addr)) \ - | (set)), (addr)) -#define clrbits_le32(addr, clear) writel(htole32(le32toh(readl(addr)) \ - & ~(clear)), (addr)) +#define __clrsetbits(endian, bits, addr, clear, set) \ + write##bits(addr, hto##endian##bits((endian##bits##toh( \ + read##bits(addr)) & ~((uint##bits##_t)(clear))) | (set)))
-#define clrsetbits_be32(addr, clear, set) writel(htobe32((be32toh(readl(addr)) \ - & ~(clear)) | (set)), (addr)) -#define setbits_be32(addr, set) writel(htobe32(be32toh(readl(addr)) \ - | (set)), (addr)) -#define clrbits_be32(addr, clear) writel(htobe32(be32toh(readl(addr)) \ - & ~(clear)), (addr)) +#define clrbits_le64(addr, clear) __clrsetbits(le, 64, addr, clear, 0) +#define clrbits_be64(addr, clear) __clrsetbits(be, 64, addr, clear, 0) +#define clrbits_le32(addr, clear) __clrsetbits(le, 32, addr, clear, 0) +#define clrbits_be32(addr, clear) __clrsetbits(be, 32, addr, clear, 0) +#define clrbits_le16(addr, clear) __clrsetbits(le, 16, addr, clear, 0) +#define clrbits_be16(addr, clear) __clrsetbits(be, 16, addr, clear, 0) + +#define setbits_le64(addr, set) __clrsetbits(le, 64, addr, 0, set) +#define setbits_be64(addr, set) __clrsetbits(be, 64, addr, 0, set) +#define setbits_le32(addr, set) __clrsetbits(le, 32, addr, 0, set) +#define setbits_be32(addr, set) __clrsetbits(be, 32, addr, 0, set) +#define setbits_le16(addr, set) __clrsetbits(le, 16, addr, 0, set) +#define setbits_be16(addr, set) __clrsetbits(be, 16, addr, 0, set) + +#define clrsetbits_le64(addr, clear, set) __clrsetbits(le, 64, addr, clear, set) +#define clrsetbits_be64(addr, clear, set) __clrsetbits(be, 64, addr, clear, set) +#define clrsetbits_le32(addr, clear, set) __clrsetbits(le, 32, addr, clear, set) +#define clrsetbits_be32(addr, clear, set) __clrsetbits(be, 32, addr, clear, set) +#define clrsetbits_le16(addr, clear, set) __clrsetbits(le, 16, addr, clear, set) +#define clrsetbits_be16(addr, clear, set) __clrsetbits(be, 16, addr, clear, set) + +#define __clrsetbits_impl(bits, addr, clear, set) write##bits(addr, \ + (read##bits(addr) & ~((uint##bits##_t)(clear))) | (set)) + +#define clrsetbits8(addr, clear, set) __clrsetbits_impl(8, addr, clear, set) +#define clrsetbits16(addr, clear, set) __clrsetbits_impl(16, addr, clear, set) +#define clrsetbits32(addr, clear, set) __clrsetbits_impl(32, addr, clear, set) +#define clrsetbits64(addr, clear, set) __clrsetbits_impl(64, addr, clear, set) + +#define setbits8(addr, set) clrsetbits8(addr, 0, set) +#define setbits16(addr, set) clrsetbits16(addr, 0, set) +#define setbits32(addr, set) clrsetbits32(addr, 0, set) +#define setbits64(addr, set) clrsetbits64(addr, 0, set) + +#define clrbits8(addr, clear) clrsetbits8(addr, clear, 0) +#define clrbits16(addr, clear) clrsetbits16(addr, clear, 0) +#define clrbits32(addr, clear) clrsetbits32(addr, clear, 0) +#define clrbits64(addr, clear) clrsetbits64(addr, clear, 0)
#endif /* _ENDIAN_H_ */ diff --git a/src/drivers/maxim/max77686/max77686.c b/src/drivers/maxim/max77686/max77686.c index 54e3a6c..cfbf912 100644 --- a/src/drivers/maxim/max77686/max77686.c +++ b/src/drivers/maxim/max77686/max77686.c @@ -127,10 +127,10 @@ }
if (enable == REG_DISABLE) { - clrbits_8(&read_data, + clrbits8(&read_data, pmic->reg_enbitmask << pmic->reg_enbitpos); } else { - clrsetbits_8(&read_data, + clrsetbits8(&read_data, pmic->reg_enbitmask << pmic->reg_enbitpos, pmic->reg_enbiton << pmic->reg_enbitpos); } @@ -177,7 +177,7 @@ } vol_level /= (u32)pmic->vol_div;
- clrsetbits_8(&read_data, pmic->vol_bitmask << pmic->vol_bitpos, + clrsetbits8(&read_data, pmic->vol_bitmask << pmic->vol_bitpos, vol_level << pmic->vol_bitpos);
ret = max77686_i2c_write(bus, MAX77686_I2C_ADDR, pmic->vol_addr, read_data); diff --git a/src/include/device/mmio.h b/src/include/device/mmio.h index 6596cf8..9c5e27c 100644 --- a/src/include/device/mmio.h +++ b/src/include/device/mmio.h @@ -19,6 +19,24 @@ #include <endian.h> #include <types.h>
+#define __clrsetbits_impl(bits, addr, clear, set) write##bits(addr, \ + (read##bits(addr) & ~((uint##bits##_t)(clear))) | (set)) + +#define clrsetbits8(addr, clear, set) __clrsetbits_impl(8, addr, clear, set) +#define clrsetbits16(addr, clear, set) __clrsetbits_impl(16, addr, clear, set) +#define clrsetbits32(addr, clear, set) __clrsetbits_impl(32, addr, clear, set) +#define clrsetbits64(addr, clear, set) __clrsetbits_impl(64, addr, clear, set) + +#define setbits8(addr, set) clrsetbits8(addr, 0, set) +#define setbits16(addr, set) clrsetbits16(addr, 0, set) +#define setbits32(addr, set) clrsetbits32(addr, 0, set) +#define setbits64(addr, set) clrsetbits64(addr, 0, set) + +#define clrbits8(addr, clear) clrsetbits8(addr, clear, 0) +#define clrbits16(addr, clear) clrsetbits16(addr, clear, 0) +#define clrbits32(addr, clear) clrsetbits32(addr, clear, 0) +#define clrbits64(addr, clear) clrsetbits64(addr, clear, 0) + #ifndef __ROMCC__ /* * Reads a transfer buffer from 32-bit FIFO registers. fifo_stride is the diff --git a/src/include/endian.h b/src/include/endian.h index 8dc1854..f16f668 100644 --- a/src/include/endian.h +++ b/src/include/endian.h @@ -79,11 +79,6 @@ #define clrsetbits_le16(addr, clear, set) __clrsetbits(le, 16, addr, clear, set) #define clrsetbits_be16(addr, clear, set) __clrsetbits(be, 16, addr, clear, set)
-#define clrsetbits_8(addr, clear, set) \ - write8(addr, (read8(addr) & ~(clear)) | (set)) -#define clrbits_8(addr, clear) clrsetbits_8(addr, clear, 0) -#define setbits_8(addr, set) setbits_8(addr, 0, set) - #ifndef __ROMCC__ /* be16dec/be32dec/be64dec/le16dec/le32dec/le64dec family of functions. */ #define DEFINE_ENDIAN_DEC(endian, width) \ diff --git a/src/include/mmio.h b/src/include/mmio.h deleted file mode 100644 index 4f2c806..0000000 --- a/src/include/mmio.h +++ /dev/null @@ -1,45 +0,0 @@ -/* - * This file is part of the coreboot project. - * - * This program is free software; you can redistribute it and/or modify - * it under the terms of the GNU General Public License as published by - * the Free Software Foundation; version 2 of the License. - * - * This program is distributed in the hope that it will be useful, - * but WITHOUT ANY WARRANTY; without even the implied warranty of - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the - * GNU General Public License for more details. - */ - -/* - * mmio.h provides update*() as well as read/write*() from arch/mmio.h - */ - -#ifndef __MMIO_H__ -#define __MMIO_H__ - -#include <arch/mmio.h> -#include <stdint.h> - -static __always_inline void update8(volatile void *addr, uint8_t mask, uint8_t or) -{ - uint8_t reg = read8(addr); - reg = (reg & mask) | or; - write8(addr, reg); -} - -static __always_inline void update16(volatile void *addr, uint16_t mask, uint16_t or) -{ - uint16_t reg = read16(addr); - reg = (reg & mask) | or; - write16(addr, reg); -} - -static __always_inline void update32(volatile void *addr, uint32_t mask, uint32_t or) -{ - uint32_t reg = read32(addr); - reg = (reg & mask) | or; - write32(addr, reg); -} - -#endif /* __MMIO_H__ */
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37432 )
Change subject: mmio: Add clrsetbitsXX() API in place of updateX() ......................................................................
Patch Set 3:
Any reason not to use inline functions? Currently, `addr` is always evaluated twice. It shouldn't matter in practice, just wondering.
No sorry, I just expanded on what was there already. Inline functions make this a little harder to write (you need either very fancy macro magic or a lot of copy&paste), but not opposed to changing it if you want to.
-
Furquan Shaikh (Code Review) -
Hung-Te Lin (Code Review)
-
Julius Werner (Code Review)
-
Nico Huber (Code Review)
-
Patrick Georgi (Code Review)