Maxim Polyakov has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/45372 )
Change subject: ec/kontron/kempld: Fix code style ......................................................................
ec/kontron/kempld: Fix code style
Change-Id: Ia5ad0715b742427dffa6c0c507269d904fe19bcb Signed-off-by: Maxim Polyakov max.senia.poliak@gmail.com --- M src/ec/kontron/kempld/chip.h M src/ec/kontron/kempld/early_kempld.c M src/ec/kontron/kempld/kempld.c M src/ec/kontron/kempld/kempld_i2c.c M src/ec/kontron/kempld/kempld_internal.h 5 files changed, 30 insertions(+), 46 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/72/45372/1
diff --git a/src/ec/kontron/kempld/chip.h b/src/ec/kontron/kempld/chip.h index 2ac0949..b520e6e 100644 --- a/src/ec/kontron/kempld/chip.h +++ b/src/ec/kontron/kempld/chip.h @@ -3,7 +3,7 @@ #ifndef EC_KONTRON_KEMPLD_CHIP_H #define EC_KONTRON_KEMPLD_CHIP_H
-#define KEMPLD_NUM_UARTS 2 +#define KEMPLD_NUM_UARTS 2
enum kempld_uart_io { KEMPLD_UART_3F8 = 0, @@ -19,7 +19,7 @@
struct ec_kontron_kempld_config { struct kempld_uart uart[KEMPLD_NUM_UARTS]; - unsigned short i2c_freq_khz; + unsigned short i2c_freq_khz; };
#endif /* EC_KONTRON_KEMPLD_CHIP_H */ diff --git a/src/ec/kontron/kempld/early_kempld.c b/src/ec/kontron/kempld/early_kempld.c index cb33a8b..d25896c 100644 --- a/src/ec/kontron/kempld/early_kempld.c +++ b/src/ec/kontron/kempld/early_kempld.c @@ -3,7 +3,6 @@ #include <stdint.h> #include <arch/io.h> #include <delay.h> - #include "chip.h" #include "kempld.h" #include "kempld_internal.h" @@ -43,13 +42,11 @@ switch (CONFIG_UART_FOR_CONSOLE) { case 0: kempld_write8(KEMPLD_UART_0, - KEMPLD_UART_ENABLE | - KEMPLD_UART_3F8 << KEMPLD_UART_IO_SHIFT); + KEMPLD_UART_ENABLE | KEMPLD_UART_3F8 << KEMPLD_UART_IO_SHIFT); break; case 1: kempld_write8(KEMPLD_UART_1, - KEMPLD_UART_ENABLE | - KEMPLD_UART_2F8 << KEMPLD_UART_IO_SHIFT); + KEMPLD_UART_ENABLE | KEMPLD_UART_2F8 << KEMPLD_UART_IO_SHIFT); break; default: break; diff --git a/src/ec/kontron/kempld/kempld.c b/src/ec/kontron/kempld/kempld.c index 19376c7..22e5376 100644 --- a/src/ec/kontron/kempld/kempld.c +++ b/src/ec/kontron/kempld/kempld.c @@ -2,7 +2,6 @@
#include <console/console.h> #include <device/device.h> - #include "chip.h" #include "kempld.h" #include "kempld_internal.h" @@ -10,27 +9,24 @@ static void kempld_uart_read_resources(struct device *dev) { static const unsigned int io_addr[] = { 0x3f8, 0x2f8, 0x3e8, 0x2e8 }; - const struct ec_kontron_kempld_config *const config = dev->chip_info; - struct resource *const res_io = new_resource(dev, 0); struct resource *const res_irq = new_resource(dev, 1); const unsigned int uart = dev->path.generic.subid; + if (!config || !res_io || !res_irq || uart >= KEMPLD_NUM_UARTS) return;
const enum kempld_uart_io io = config->uart[uart].io; if (io >= ARRAY_SIZE(io_addr)) { - printk(BIOS_ERR, "KEMPLD: Bad io value '%d' for UART#%u\n.", - io, uart); + printk(BIOS_ERR, "KEMPLD: Bad io value '%d' for UART#%u\n.", io, uart); dev->enabled = false; return; }
const int irq = config->uart[uart].irq; if (irq >= 16) { - printk(BIOS_ERR, "KEMPLD: Bad irq value '%d' for UART#%u\n.", - irq, uart); + printk(BIOS_ERR, "KEMPLD: Bad irq value '%d' for UART#%u\n.", irq, uart); dev->enabled = false; return; } @@ -49,22 +45,23 @@
const uint8_t reg = uart ? KEMPLD_UART_1 : KEMPLD_UART_0; const uint8_t val = kempld_read8(reg); - kempld_write8(reg, (val & ~(KEMPLD_UART_IO_MASK | KEMPLD_UART_IRQ_MASK)) - | io << KEMPLD_UART_IO_SHIFT - | irq << KEMPLD_UART_IRQ_SHIFT); + kempld_write8(reg, + (val & ~(KEMPLD_UART_IO_MASK | KEMPLD_UART_IRQ_MASK)) | + io << KEMPLD_UART_IO_SHIFT | + irq << KEMPLD_UART_IRQ_SHIFT);
kempld_release_mutex(); }
static void kempld_uart_enable_resources(struct device *dev) { + const unsigned int uart = dev->path.generic.subid; + const uint8_t reg = uart ? KEMPLD_UART_1 : KEMPLD_UART_0; + if (kempld_get_mutex(100) < 0) return;
- const unsigned int uart = dev->path.generic.subid; - const uint8_t reg = uart ? KEMPLD_UART_1 : KEMPLD_UART_0; kempld_write8(reg, kempld_read8(reg) | KEMPLD_UART_ENABLE); - kempld_release_mutex(); }
@@ -90,9 +87,7 @@ } /* Fall through. */ default: - printk(BIOS_WARNING, - "KEMPLD: Spurious device %s.\n", - dev_path(dev)); + printk(BIOS_WARNING, "KEMPLD: Spurious device %s.\n", dev_path(dev)); break; } } diff --git a/src/ec/kontron/kempld/kempld_i2c.c b/src/ec/kontron/kempld/kempld_i2c.c index 0145d97..8701205 100644 --- a/src/ec/kontron/kempld/kempld_i2c.c +++ b/src/ec/kontron/kempld/kempld_i2c.c @@ -1,18 +1,11 @@ /* SPDX-License-Identifier: GPL-2.0-only */
-/* - * I2C bus driver for Kontron COM modules - * - * Based on the similar driver in Linux. - */ - #include <stdint.h> #include <console/console.h> #include <device/device.h> #include <device/i2c_bus.h> #include <timer.h> #include <delay.h> - #include "chip.h" #include "kempld.h" #include "kempld_internal.h" @@ -41,10 +34,10 @@ #define I2C_CMD_READ_NACK 0x29 #define I2C_CMD_IACK 0x01
-#define EIO 5 -#define ENXIO 6 -#define EAGAIN 11 -#define EBUSY 16 +#define EIO 5 +#define ENXIO 6 +#define EAGAIN 11 +#define EBUSY 16 #define ETIMEDOUT 110
enum kempld_i2c_freq_stadart { @@ -66,10 +59,10 @@ };
struct kempld_i2c_data { - const struct i2c_msg *msg; - size_t pos; - size_t nmsgs; - enum kempld_i2c_state state; + const struct i2c_msg *msg; + size_t pos; + size_t nmsgs; + enum kempld_i2c_state state; };
/* @@ -158,8 +151,7 @@ i2c->state = STATE_ADDR; return 0; } - i2c->state = (msg->flags & I2C_M_RD) - ? STATE_READ : STATE_WRITE; + i2c->state = (msg->flags & I2C_M_RD) ? STATE_READ : STATE_WRITE; } else { i2c->state = STATE_DONE; kempld_write8(KEMPLD_I2C_CMD, I2C_CMD_STOP); @@ -229,8 +221,8 @@ };
static struct device_operations kempld_i2c_dev_ops = { - .scan_bus = &scan_smbus, - .ops_i2c_bus = &kempld_i2c_bus_ops, + .scan_bus = &scan_smbus, + .ops_i2c_bus = &kempld_i2c_bus_ops, };
void kempld_i2c_device_init(struct device *const dev) diff --git a/src/ec/kontron/kempld/kempld_internal.h b/src/ec/kontron/kempld/kempld_internal.h index 014778c..4fb42dc 100644 --- a/src/ec/kontron/kempld/kempld_internal.h +++ b/src/ec/kontron/kempld/kempld_internal.h @@ -6,9 +6,9 @@ #include <device/device.h>
/* i/o ports */ -#define KEMPLD_IDX 0xa80 -#define KEMPLD_MUTEX_KEY 0x80 -#define KEMPLD_DAT 0xa81 +#define KEMPLD_IDX 0xa80 +#define KEMPLD_MUTEX_KEY 0x80 +#define KEMPLD_DAT 0xa81
/* indexed registers */ #define KEMPLD_SPEC 0x06 @@ -26,7 +26,7 @@ #define KEMPLD_UART_IO_MASK (0x3 << KEMPLD_UART_IO_SHIFT) #define KEMPLD_UART_ENABLE 0x80
-#define KEMPLD_CLK 33333333 /* 33MHz */ +#define KEMPLD_CLK 33333333 /* 33MHz */
void kempld_i2c_device_init(struct device *const dev);
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45372 )
Change subject: ec/kontron/kempld: Fix code style ......................................................................
Patch Set 2:
(16 comments)
https://review.coreboot.org/c/coreboot/+/45372/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/45372/2//COMMIT_MSG@7 PS2, Line 7: Fix I'm afraid that this isn't the case here.
https://review.coreboot.org/c/coreboot/+/45372/2/src/ec/kontron/kempld/chip.... File src/ec/kontron/kempld/chip.h:
https://review.coreboot.org/c/coreboot/+/45372/2/src/ec/kontron/kempld/chip.... PS2, Line 6: #define KEMPLD_NUM_UARTS 2 I doubt using a tab here provides any visual benefit.
https://review.coreboot.org/c/coreboot/+/45372/2/src/ec/kontron/kempld/early... File src/ec/kontron/kempld/early_kempld.c:
https://review.coreboot.org/c/coreboot/+/45372/2/src/ec/kontron/kempld/early... PS2, Line 6: This newline is intentional, since both types of headers are different.
https://review.coreboot.org/c/coreboot/+/45372/2/src/ec/kontron/kempld/early... PS2, Line 45: KEMPLD_UART_ENABLE | KEMPLD_UART_3F8 << KEMPLD_UART_IO_SHIFT); Please keep the line aligned:
kempld_write8(KEMPLD_UART_0, KEMPLD_UART_ENABLE | KEMPLD_UART_3F8 << KEMPLD_UART_IO_SHIFT);
https://review.coreboot.org/c/coreboot/+/45372/2/src/ec/kontron/kempld/early... PS2, Line 49: KEMPLD_UART_ENABLE | KEMPLD_UART_2F8 << KEMPLD_UART_IO_SHIFT); Same here
https://review.coreboot.org/c/coreboot/+/45372/2/src/ec/kontron/kempld/kempl... File src/ec/kontron/kempld/kempld.c:
https://review.coreboot.org/c/coreboot/+/45372/2/src/ec/kontron/kempld/kempl... PS2, Line 5: Same as before
https://review.coreboot.org/c/coreboot/+/45372/2/src/ec/kontron/kempld/kempl... PS2, Line 13: This blank line separates fixed constants from dev-dependent constants.
https://review.coreboot.org/c/coreboot/+/45372/2/src/ec/kontron/kempld/kempl... PS2, Line 15: This one separates regular variable assignments from function calls.
https://review.coreboot.org/c/coreboot/+/45372/2/src/ec/kontron/kempld/kempl... PS2, Line 58: const unsigned int uart = dev->path.generic.subid; I'd rather keep these where they were before. If we can't get the mutex, we don't need these.
https://review.coreboot.org/c/coreboot/+/45372/2/src/ec/kontron/kempld/kempl... File src/ec/kontron/kempld/kempld_i2c.c:
https://review.coreboot.org/c/coreboot/+/45372/2/src/ec/kontron/kempld/kempl... PS2, Line 7: */ Why is this gone?
https://review.coreboot.org/c/coreboot/+/45372/2/src/ec/kontron/kempld/kempl... PS2, Line 15: Same as before
https://review.coreboot.org/c/coreboot/+/45372/2/src/ec/kontron/kempld/kempl... PS2, Line 40: #define EBUSY 16 What does this fix? With the spaces, the numbers were aligned: ones with ones, tens with tens...
https://review.coreboot.org/c/coreboot/+/45372/2/src/ec/kontron/kempld/kempl... PS2, Line 66: }; This was aligned with tabs so that the difference between types and member names was clearer.
https://review.coreboot.org/c/coreboot/+/45372/2/src/ec/kontron/kempld/kempl... PS2, Line 225: .ops_i2c_bus = &kempld_i2c_bus_ops, Why break the alignment?
https://review.coreboot.org/c/coreboot/+/45372/2/src/ec/kontron/kempld/kempl... File src/ec/kontron/kempld/kempld_internal.h:
https://review.coreboot.org/c/coreboot/+/45372/2/src/ec/kontron/kempld/kempl... PS2, Line 11: #define KEMPLD_DAT 0xa81 These aren't indexed registers, so there's no need to align them with those.
https://review.coreboot.org/c/coreboot/+/45372/2/src/ec/kontron/kempld/kempl... PS2, Line 29: #define KEMPLD_CLK 33333333 /* 33MHz */ I don't think this should be aligned with the indexed registers either.
Hello build bot (Jenkins), Paul Menzel, Angel Pons,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/45372
to look at the new patch set (#3).
Change subject: ec/kontron/kempld: Reflow long lines ......................................................................
ec/kontron/kempld: Reflow long lines
Change-Id: Ia5ad0715b742427dffa6c0c507269d904fe19bcb Signed-off-by: Maxim Polyakov max.senia.poliak@gmail.com --- M src/ec/kontron/kempld/early_kempld.c M src/ec/kontron/kempld/kempld.c M src/ec/kontron/kempld/kempld_i2c.c M src/ec/kontron/kempld/kempld_internal.h 4 files changed, 12 insertions(+), 17 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/72/45372/3
Maxim Polyakov has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45372 )
Change subject: ec/kontron/kempld: Reflow long lines ......................................................................
Patch Set 4:
(15 comments)
Thanks for the review!
https://review.coreboot.org/c/coreboot/+/45372/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/45372/2//COMMIT_MSG@7 PS2, Line 7: Fix
I'm afraid that this isn't the case here.
I reworked this patch according to your comments.
https://review.coreboot.org/c/coreboot/+/45372/2/src/ec/kontron/kempld/chip.... File src/ec/kontron/kempld/chip.h:
https://review.coreboot.org/c/coreboot/+/45372/2/src/ec/kontron/kempld/chip.... PS2, Line 6: #define KEMPLD_NUM_UARTS 2
I doubt using a tab here provides any visual benefit.
Ack
https://review.coreboot.org/c/coreboot/+/45372/2/src/ec/kontron/kempld/early... File src/ec/kontron/kempld/early_kempld.c:
https://review.coreboot.org/c/coreboot/+/45372/2/src/ec/kontron/kempld/early... PS2, Line 6:
This newline is intentional, since both types of headers are different.
Ack
https://review.coreboot.org/c/coreboot/+/45372/2/src/ec/kontron/kempld/early... PS2, Line 45: KEMPLD_UART_ENABLE | KEMPLD_UART_3F8 << KEMPLD_UART_IO_SHIFT);
Please keep the line aligned: […]
Done
https://review.coreboot.org/c/coreboot/+/45372/2/src/ec/kontron/kempld/early... PS2, Line 49: KEMPLD_UART_ENABLE | KEMPLD_UART_2F8 << KEMPLD_UART_IO_SHIFT);
Same here
Done
https://review.coreboot.org/c/coreboot/+/45372/2/src/ec/kontron/kempld/kempl... File src/ec/kontron/kempld/kempld.c:
https://review.coreboot.org/c/coreboot/+/45372/2/src/ec/kontron/kempld/kempl... PS2, Line 5:
Same as before
Ack
https://review.coreboot.org/c/coreboot/+/45372/2/src/ec/kontron/kempld/kempl... PS2, Line 13:
This blank line separates fixed constants from dev-dependent constants.
Ack
https://review.coreboot.org/c/coreboot/+/45372/2/src/ec/kontron/kempld/kempl... PS2, Line 15:
This one separates regular variable assignments from function calls.
Ack
https://review.coreboot.org/c/coreboot/+/45372/2/src/ec/kontron/kempld/kempl... PS2, Line 58: const unsigned int uart = dev->path.generic.subid;
I'd rather keep these where they were before. If we can't get the mutex, we don't need these.
Ack
https://review.coreboot.org/c/coreboot/+/45372/2/src/ec/kontron/kempld/kempl... File src/ec/kontron/kempld/kempld_i2c.c:
https://review.coreboot.org/c/coreboot/+/45372/2/src/ec/kontron/kempld/kempl... PS2, Line 7: */
Why is this gone?
canceled
https://review.coreboot.org/c/coreboot/+/45372/2/src/ec/kontron/kempld/kempl... PS2, Line 15:
Same as before
canceled
https://review.coreboot.org/c/coreboot/+/45372/2/src/ec/kontron/kempld/kempl... PS2, Line 40: #define EBUSY 16
What does this fix? With the spaces, the numbers were aligned: ones with ones, tens with tens...
canceled
https://review.coreboot.org/c/coreboot/+/45372/2/src/ec/kontron/kempld/kempl... PS2, Line 66: };
This was aligned with tabs so that the difference between types and member names was clearer.
canceled
https://review.coreboot.org/c/coreboot/+/45372/2/src/ec/kontron/kempld/kempl... PS2, Line 225: .ops_i2c_bus = &kempld_i2c_bus_ops,
Why break the alignment?
canceled
https://review.coreboot.org/c/coreboot/+/45372/2/src/ec/kontron/kempld/kempl... File src/ec/kontron/kempld/kempld_internal.h:
https://review.coreboot.org/c/coreboot/+/45372/2/src/ec/kontron/kempld/kempl... PS2, Line 11: #define KEMPLD_DAT 0xa81
These aren't indexed registers, so there's no need to align them with those.
Ack
Hello build bot (Jenkins), Paul Menzel, Angel Pons,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/45372
to look at the new patch set (#5).
Change subject: ec/kontron/kempld: Reflow long lines ......................................................................
ec/kontron/kempld: Reflow long lines
Change-Id: Ia5ad0715b742427dffa6c0c507269d904fe19bcb Signed-off-by: Maxim Polyakov max.senia.poliak@gmail.com --- M src/ec/kontron/kempld/early_kempld.c M src/ec/kontron/kempld/kempld.c M src/ec/kontron/kempld/kempld_i2c.c 3 files changed, 11 insertions(+), 16 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/72/45372/5
Maxim Polyakov has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45372 )
Change subject: ec/kontron/kempld: Reflow long lines ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45372/2/src/ec/kontron/kempld/kempl... File src/ec/kontron/kempld/kempld_internal.h:
https://review.coreboot.org/c/coreboot/+/45372/2/src/ec/kontron/kempld/kempl... PS2, Line 29: #define KEMPLD_CLK 33333333 /* 33MHz */
I don't think this should be aligned with the indexed registers either.
Ack
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45372 )
Change subject: ec/kontron/kempld: Reflow long lines ......................................................................
Patch Set 5: Code-Review+2
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45372 )
Change subject: ec/kontron/kempld: Reflow long lines ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45372/7/src/ec/kontron/kempld/kempl... File src/ec/kontron/kempld/kempld.c:
https://review.coreboot.org/c/coreboot/+/45372/7/src/ec/kontron/kempld/kempl... PS7, Line 54: irq << KEMPLD_UART_IRQ_SHIFT); Is the additional line break intentional?
Maxim Polyakov has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45372 )
Change subject: ec/kontron/kempld: Reflow long lines ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45372/7/src/ec/kontron/kempld/kempl... File src/ec/kontron/kempld/kempld.c:
https://review.coreboot.org/c/coreboot/+/45372/7/src/ec/kontron/kempld/kempl... PS7, Line 54: irq << KEMPLD_UART_IRQ_SHIFT);
Is the additional line break intentional?
Do you mean a line break between the arguments? Yes, I would like to move the complex argument to a new line.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45372 )
Change subject: ec/kontron/kempld: Reflow long lines ......................................................................
Patch Set 7:
I'm going to submit this because it's already reviewed and I don't like to bikeshed. But generally, I don't like such changes. It's individual taste how lines should be broken. To me, half of the changed lines look better and the other half looks worse. So in my humble theory, somebody else could push an exact revert of this tomorrow, say that it looks better this way and it might go in (because it improves half of the lines).
Nico Huber has submitted this change. ( https://review.coreboot.org/c/coreboot/+/45372 )
Change subject: ec/kontron/kempld: Reflow long lines ......................................................................
ec/kontron/kempld: Reflow long lines
Change-Id: Ia5ad0715b742427dffa6c0c507269d904fe19bcb Signed-off-by: Maxim Polyakov max.senia.poliak@gmail.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/45372 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Angel Pons th3fanbus@gmail.com --- M src/ec/kontron/kempld/early_kempld.c M src/ec/kontron/kempld/kempld.c M src/ec/kontron/kempld/kempld_i2c.c 3 files changed, 11 insertions(+), 16 deletions(-)
Approvals: build bot (Jenkins): Verified Angel Pons: Looks good to me, approved
diff --git a/src/ec/kontron/kempld/early_kempld.c b/src/ec/kontron/kempld/early_kempld.c index cb33a8b..3cafb1e 100644 --- a/src/ec/kontron/kempld/early_kempld.c +++ b/src/ec/kontron/kempld/early_kempld.c @@ -43,13 +43,11 @@ switch (CONFIG_UART_FOR_CONSOLE) { case 0: kempld_write8(KEMPLD_UART_0, - KEMPLD_UART_ENABLE | - KEMPLD_UART_3F8 << KEMPLD_UART_IO_SHIFT); + KEMPLD_UART_ENABLE | KEMPLD_UART_3F8 << KEMPLD_UART_IO_SHIFT); break; case 1: kempld_write8(KEMPLD_UART_1, - KEMPLD_UART_ENABLE | - KEMPLD_UART_2F8 << KEMPLD_UART_IO_SHIFT); + KEMPLD_UART_ENABLE | KEMPLD_UART_2F8 << KEMPLD_UART_IO_SHIFT); break; default: break; diff --git a/src/ec/kontron/kempld/kempld.c b/src/ec/kontron/kempld/kempld.c index 19376c7..f8371a8 100644 --- a/src/ec/kontron/kempld/kempld.c +++ b/src/ec/kontron/kempld/kempld.c @@ -16,21 +16,20 @@ struct resource *const res_io = new_resource(dev, 0); struct resource *const res_irq = new_resource(dev, 1); const unsigned int uart = dev->path.generic.subid; + if (!config || !res_io || !res_irq || uart >= KEMPLD_NUM_UARTS) return;
const enum kempld_uart_io io = config->uart[uart].io; if (io >= ARRAY_SIZE(io_addr)) { - printk(BIOS_ERR, "KEMPLD: Bad io value '%d' for UART#%u\n.", - io, uart); + printk(BIOS_ERR, "KEMPLD: Bad io value '%d' for UART#%u\n.", io, uart); dev->enabled = false; return; }
const int irq = config->uart[uart].irq; if (irq >= 16) { - printk(BIOS_ERR, "KEMPLD: Bad irq value '%d' for UART#%u\n.", - irq, uart); + printk(BIOS_ERR, "KEMPLD: Bad irq value '%d' for UART#%u\n.", irq, uart); dev->enabled = false; return; } @@ -49,9 +48,10 @@
const uint8_t reg = uart ? KEMPLD_UART_1 : KEMPLD_UART_0; const uint8_t val = kempld_read8(reg); - kempld_write8(reg, (val & ~(KEMPLD_UART_IO_MASK | KEMPLD_UART_IRQ_MASK)) - | io << KEMPLD_UART_IO_SHIFT - | irq << KEMPLD_UART_IRQ_SHIFT); + kempld_write8(reg, + (val & ~(KEMPLD_UART_IO_MASK | KEMPLD_UART_IRQ_MASK)) | + io << KEMPLD_UART_IO_SHIFT | + irq << KEMPLD_UART_IRQ_SHIFT);
kempld_release_mutex(); } @@ -90,9 +90,7 @@ } /* Fall through. */ default: - printk(BIOS_WARNING, - "KEMPLD: Spurious device %s.\n", - dev_path(dev)); + printk(BIOS_WARNING, "KEMPLD: Spurious device %s.\n", dev_path(dev)); break; } } diff --git a/src/ec/kontron/kempld/kempld_i2c.c b/src/ec/kontron/kempld/kempld_i2c.c index 8abbb4b..b99c0e4 100644 --- a/src/ec/kontron/kempld/kempld_i2c.c +++ b/src/ec/kontron/kempld/kempld_i2c.c @@ -151,8 +151,7 @@ i2c->state = STATE_ADDR; return 0; } - i2c->state = (msg->flags & I2C_M_RD) - ? STATE_READ : STATE_WRITE; + i2c->state = (msg->flags & I2C_M_RD) ? STATE_READ : STATE_WRITE; } else { i2c->state = STATE_DONE; kempld_write8(KEMPLD_I2C_CMD, I2C_CMD_STOP);
HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45372 )
Change subject: ec/kontron/kempld: Reflow long lines ......................................................................
Patch Set 8:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45372/8/src/ec/kontron/kempld/kempl... File src/ec/kontron/kempld/kempld.c:
https://review.coreboot.org/c/coreboot/+/45372/8/src/ec/kontron/kempld/kempl... PS8, Line 52: kempld_write8(reg, (val & ~(KEMPLD_UART_IO_MASK | KEMPLD_UART_IRQ_MASK)) : | io << KEMPLD_UART_IO_SHIFT : | irq << KEMPLD_UART_IRQ_SHIFT) looks better.
I think we need a rule...
-
Angel Pons (Code Review) -
HAOUAS Elyes (Code Review)
-
Maxim Polyakov (Code Review)
-
Nico Huber (Code Review)