* [Qemu-devel] [PATCH 1/3] hw/arm/exynos: Convert fprintf to error_report()
@ 2017-03-05 21:48 Krzysztof Kozlowski
2017-03-05 21:48 ` [Qemu-devel] [PATCH 2/3] hw/arm/exynos4210: Constify data pointed by few arguments and variables Krzysztof Kozlowski
` (3 more replies)
0 siblings, 4 replies; 17+ messages in thread
From: Krzysztof Kozlowski @ 2017-03-05 21:48 UTC (permalink / raw)
To: Igor Mitsyanko, Peter Maydell, Paolo Bonzini, qemu-arm, qemu-devel
Cc: Krzysztof Kozlowski
error_report() is preferred over fprintf() for logging errors. Also
remove square brackets [] and additional new line characters in printed
messages.
Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
---
hw/arm/exynos4_boards.c | 6 +++---
hw/timer/exynos4210_mct.c | 5 +++--
hw/timer/exynos4210_pwm.c | 11 +++++------
hw/timer/exynos4210_rtc.c | 16 +++++++---------
4 files changed, 18 insertions(+), 20 deletions(-)
diff --git a/hw/arm/exynos4_boards.c b/hw/arm/exynos4_boards.c
index 0efa19405409..5cd94d402b52 100644
--- a/hw/arm/exynos4_boards.c
+++ b/hw/arm/exynos4_boards.c
@@ -22,6 +22,7 @@
*/
#include "qemu/osdep.h"
+#include "qemu/error-report.h"
#include "qemu-common.h"
#include "cpu.h"
#include "sysemu/sysemu.h"
@@ -101,9 +102,8 @@ static Exynos4210State *exynos4_boards_init_common(MachineState *machine,
MachineClass *mc = MACHINE_GET_CLASS(machine);
if (smp_cpus != EXYNOS4210_NCPUS && !qtest_enabled()) {
- fprintf(stderr, "%s board supports only %d CPU cores. Ignoring smp_cpus"
- " value.\n",
- mc->name, EXYNOS4210_NCPUS);
+ error_report("%s board supports only %d CPU cores. Ignoring smp_cpus value.",
+ mc->name, EXYNOS4210_NCPUS);
}
exynos4_board_binfo.ram_size = exynos4_board_ram_size[board_type];
diff --git a/hw/timer/exynos4210_mct.c b/hw/timer/exynos4210_mct.c
index 6069116942a4..48041ab036a6 100644
--- a/hw/timer/exynos4210_mct.c
+++ b/hw/timer/exynos4210_mct.c
@@ -53,6 +53,7 @@
*/
#include "qemu/osdep.h"
+#include "qemu/error-report.h"
#include "hw/sysbus.h"
#include "qemu/timer.h"
#include "qemu/main-loop.h"
@@ -1364,8 +1365,8 @@ static void exynos4210_mct_write(void *opaque, hwaddr offset,
case L0_TCNTO: case L1_TCNTO:
case L0_ICNTO: case L1_ICNTO:
case L0_FRCNTO: case L1_FRCNTO:
- fprintf(stderr, "\n[exynos4210.mct: write to RO register "
- TARGET_FMT_plx "]\n\n", offset);
+ error_report("exynos4210.mct: write to RO register " TARGET_FMT_plx,
+ offset);
break;
case L0_INT_CSTAT: case L1_INT_CSTAT:
diff --git a/hw/timer/exynos4210_pwm.c b/hw/timer/exynos4210_pwm.c
index f5765075c720..f8826e24e63d 100644
--- a/hw/timer/exynos4210_pwm.c
+++ b/hw/timer/exynos4210_pwm.c
@@ -21,6 +21,7 @@
*/
#include "qemu/osdep.h"
+#include "qemu/error-report.h"
#include "hw/sysbus.h"
#include "qemu/timer.h"
#include "qemu-common.h"
@@ -252,9 +253,8 @@ static uint64_t exynos4210_pwm_read(void *opaque, hwaddr offset,
break;
default:
- fprintf(stderr,
- "[exynos4210.pwm: bad read offset " TARGET_FMT_plx "]\n",
- offset);
+ error_report("exynos4210.pwm: bad read offset " TARGET_FMT_plx,
+ offset);
break;
}
return value;
@@ -343,9 +343,8 @@ static void exynos4210_pwm_write(void *opaque, hwaddr offset,
break;
default:
- fprintf(stderr,
- "[exynos4210.pwm: bad write offset " TARGET_FMT_plx "]\n",
- offset);
+ error_report("exynos4210.pwm: bad write offset " TARGET_FMT_plx,
+ offset);
break;
}
diff --git a/hw/timer/exynos4210_rtc.c b/hw/timer/exynos4210_rtc.c
index 1a648c5d9e67..f4548cd555f4 100644
--- a/hw/timer/exynos4210_rtc.c
+++ b/hw/timer/exynos4210_rtc.c
@@ -26,6 +26,7 @@
*/
#include "qemu/osdep.h"
+#include "qemu/error-report.h"
#include "hw/sysbus.h"
#include "qemu/timer.h"
#include "qemu-common.h"
@@ -370,9 +371,8 @@ static uint64_t exynos4210_rtc_read(void *opaque, hwaddr offset,
break;
default:
- fprintf(stderr,
- "[exynos4210.rtc: bad read offset " TARGET_FMT_plx "]\n",
- offset);
+ error_report("exynos4210.rtc: bad read offset " TARGET_FMT_plx,
+ offset);
break;
}
return value;
@@ -433,9 +433,8 @@ static void exynos4210_rtc_write(void *opaque, hwaddr offset,
if (value > TICNT_THRESHOLD) {
s->reg_ticcnt = value;
} else {
- fprintf(stderr,
- "[exynos4210.rtc: bad TICNT value %u ]\n",
- (uint32_t)value);
+ error_report("exynos4210.rtc: bad TICNT value %u",
+ (uint32_t)value);
}
break;
@@ -500,9 +499,8 @@ static void exynos4210_rtc_write(void *opaque, hwaddr offset,
break;
default:
- fprintf(stderr,
- "[exynos4210.rtc: bad write offset " TARGET_FMT_plx "]\n",
- offset);
+ error_report("exynos4210.rtc: bad write offset " TARGET_FMT_plx,
+ offset);
break;
}
--
2.9.3
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [Qemu-devel] [PATCH 2/3] hw/arm/exynos4210: Constify data pointed by few arguments and variables
2017-03-05 21:48 [Qemu-devel] [PATCH 1/3] hw/arm/exynos: Convert fprintf to error_report() Krzysztof Kozlowski
@ 2017-03-05 21:48 ` Krzysztof Kozlowski
2017-03-06 3:58 ` [Qemu-devel] [Qemu-arm] " Philippe Mathieu-Daudé
2017-03-06 9:44 ` [Qemu-devel] " Peter Maydell
2017-03-05 21:48 ` [Qemu-devel] [PATCH 3/3] hw/misc/exynos4210_pmu: Reorder local variables for readability Krzysztof Kozlowski
` (2 subsequent siblings)
3 siblings, 2 replies; 17+ messages in thread
From: Krzysztof Kozlowski @ 2017-03-05 21:48 UTC (permalink / raw)
To: Igor Mitsyanko, Peter Maydell, Paolo Bonzini, qemu-arm, qemu-devel
Cc: Krzysztof Kozlowski
In few places the function arguments and local variables are not
modifying data passed through pointers so this can be made const for
code safeness. Also the static array exynos4210_uart_regs is not being
modified.
Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
---
hw/char/exynos4210_uart.c | 10 +++++-----
hw/intc/exynos4210_combiner.c | 2 +-
hw/intc/exynos4210_gic.c | 8 ++++----
hw/misc/exynos4210_pmu.c | 2 +-
4 files changed, 11 insertions(+), 11 deletions(-)
diff --git a/hw/char/exynos4210_uart.c b/hw/char/exynos4210_uart.c
index b75f28d473bf..83e1be253255 100644
--- a/hw/char/exynos4210_uart.c
+++ b/hw/char/exynos4210_uart.c
@@ -102,7 +102,7 @@ typedef struct Exynos4210UartReg {
uint32_t reset_value;
} Exynos4210UartReg;
-static Exynos4210UartReg exynos4210_uart_regs[] = {
+static const Exynos4210UartReg exynos4210_uart_regs[] = {
{"ULCON", ULCON, 0x00000000},
{"UCON", UCON, 0x00003000},
{"UFCON", UFCON, 0x00000000},
@@ -220,7 +220,7 @@ static uint8_t fifo_retrieve(Exynos4210UartFIFO *q)
return ret;
}
-static int fifo_elements_number(Exynos4210UartFIFO *q)
+static int fifo_elements_number(const Exynos4210UartFIFO *q)
{
if (q->sp < q->rp) {
return q->size - q->rp + q->sp;
@@ -229,7 +229,7 @@ static int fifo_elements_number(Exynos4210UartFIFO *q)
return q->sp - q->rp;
}
-static int fifo_empty_elements_number(Exynos4210UartFIFO *q)
+static int fifo_empty_elements_number(const Exynos4210UartFIFO *q)
{
return q->size - fifo_elements_number(q);
}
@@ -245,7 +245,7 @@ static void fifo_reset(Exynos4210UartFIFO *q)
q->rp = 0;
}
-static uint32_t exynos4210_uart_Tx_FIFO_trigger_level(Exynos4210UartState *s)
+static uint32_t exynos4210_uart_Tx_FIFO_trigger_level(const Exynos4210UartState *s)
{
uint32_t level = 0;
uint32_t reg;
@@ -488,7 +488,7 @@ static const MemoryRegionOps exynos4210_uart_ops = {
static int exynos4210_uart_can_receive(void *opaque)
{
- Exynos4210UartState *s = (Exynos4210UartState *)opaque;
+ const Exynos4210UartState *s = (Exynos4210UartState *)opaque;
return fifo_empty_elements_number(&s->rx);
}
diff --git a/hw/intc/exynos4210_combiner.c b/hw/intc/exynos4210_combiner.c
index f19a7062be3a..b057921e0504 100644
--- a/hw/intc/exynos4210_combiner.c
+++ b/hw/intc/exynos4210_combiner.c
@@ -180,7 +180,7 @@ void exynos4210_combiner_get_gpioin(Exynos4210Irq *irqs, DeviceState *dev,
static uint64_t
exynos4210_combiner_read(void *opaque, hwaddr offset, unsigned size)
{
- struct Exynos4210CombinerState *s =
+ const struct Exynos4210CombinerState *s =
(struct Exynos4210CombinerState *)opaque;
uint32_t req_quad_base_n; /* Base of registers quad. Multiply it by 4 and
get a start of corresponding group quad */
diff --git a/hw/intc/exynos4210_gic.c b/hw/intc/exynos4210_gic.c
index 2a55817b7660..432b8425d09d 100644
--- a/hw/intc/exynos4210_gic.c
+++ b/hw/intc/exynos4210_gic.c
@@ -190,7 +190,7 @@ combiner_grp_to_gic_id[64-EXYNOS4210_MAX_EXT_COMBINER_OUT_IRQ][8] = {
static void exynos4210_irq_handler(void *opaque, int irq, int level)
{
- Exynos4210Irq *s = (Exynos4210Irq *)opaque;
+ const Exynos4210Irq *s = (Exynos4210Irq *)opaque;
/* Bypass */
qemu_set_irq(s->board_irqs[irq], level);
@@ -277,7 +277,7 @@ typedef struct {
static void exynos4210_gic_set_irq(void *opaque, int irq, int level)
{
- Exynos4210GicState *s = (Exynos4210GicState *)opaque;
+ const Exynos4210GicState *s = (Exynos4210GicState *)opaque;
qemu_set_irq(qdev_get_gpio_in(s->gic, irq), level);
}
@@ -401,7 +401,7 @@ static const VMStateDescription vmstate_exynos4210_irq_gate = {
/* Process a change in IRQ input. */
static void exynos4210_irq_gate_handler(void *opaque, int irq, int level)
{
- Exynos4210IRQGateState *s = (Exynos4210IRQGateState *)opaque;
+ const Exynos4210IRQGateState *s = (Exynos4210IRQGateState *)opaque;
uint32_t i;
assert(irq < s->n_in);
@@ -420,7 +420,7 @@ static void exynos4210_irq_gate_handler(void *opaque, int irq, int level)
static void exynos4210_irq_gate_reset(DeviceState *d)
{
- Exynos4210IRQGateState *s = EXYNOS4210_IRQ_GATE(d);
+ const Exynos4210IRQGateState *s = EXYNOS4210_IRQ_GATE(d);
memset(s->level, 0, s->n_in * sizeof(*s->level));
}
diff --git a/hw/misc/exynos4210_pmu.c b/hw/misc/exynos4210_pmu.c
index e30dbc7d3d83..cbdfa0614600 100644
--- a/hw/misc/exynos4210_pmu.c
+++ b/hw/misc/exynos4210_pmu.c
@@ -400,7 +400,7 @@ typedef struct Exynos4210PmuState {
static uint64_t exynos4210_pmu_read(void *opaque, hwaddr offset,
unsigned size)
{
- Exynos4210PmuState *s = (Exynos4210PmuState *)opaque;
+ const Exynos4210PmuState *s = (Exynos4210PmuState *)opaque;
unsigned i;
const Exynos4210PmuReg *reg_p = exynos4210_pmu_regs;
--
2.9.3
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [Qemu-devel] [PATCH 3/3] hw/misc/exynos4210_pmu: Reorder local variables for readability
2017-03-05 21:48 [Qemu-devel] [PATCH 1/3] hw/arm/exynos: Convert fprintf to error_report() Krzysztof Kozlowski
2017-03-05 21:48 ` [Qemu-devel] [PATCH 2/3] hw/arm/exynos4210: Constify data pointed by few arguments and variables Krzysztof Kozlowski
@ 2017-03-05 21:48 ` Krzysztof Kozlowski
2017-03-06 4:06 ` [Qemu-devel] [Qemu-arm] " Philippe Mathieu-Daudé
2017-03-05 21:56 ` [Qemu-devel] [Qemu-arm] [PATCH 1/3] hw/arm/exynos: Convert fprintf to error_report() Philippe Mathieu-Daudé
2017-03-06 16:55 ` [Qemu-devel] " Eric Blake
3 siblings, 1 reply; 17+ messages in thread
From: Krzysztof Kozlowski @ 2017-03-05 21:48 UTC (permalink / raw)
To: Igor Mitsyanko, Peter Maydell, Paolo Bonzini, qemu-arm, qemu-devel
Cc: Krzysztof Kozlowski
Short declaration of 'i' was in the middle of declarations with
assignments. Make it a little bit more readable. No functional change.
Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
---
hw/misc/exynos4210_pmu.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/hw/misc/exynos4210_pmu.c b/hw/misc/exynos4210_pmu.c
index cbdfa0614600..60d1545c0baa 100644
--- a/hw/misc/exynos4210_pmu.c
+++ b/hw/misc/exynos4210_pmu.c
@@ -401,8 +401,8 @@ static uint64_t exynos4210_pmu_read(void *opaque, hwaddr offset,
unsigned size)
{
const Exynos4210PmuState *s = (Exynos4210PmuState *)opaque;
- unsigned i;
const Exynos4210PmuReg *reg_p = exynos4210_pmu_regs;
+ unsigned i;
for (i = 0; i < PMU_NUM_OF_REGISTERS; i++) {
if (reg_p->offset == offset) {
@@ -420,8 +420,8 @@ static void exynos4210_pmu_write(void *opaque, hwaddr offset,
uint64_t val, unsigned size)
{
Exynos4210PmuState *s = (Exynos4210PmuState *)opaque;
- unsigned i;
const Exynos4210PmuReg *reg_p = exynos4210_pmu_regs;
+ unsigned i;
for (i = 0; i < PMU_NUM_OF_REGISTERS; i++) {
if (reg_p->offset == offset) {
--
2.9.3
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [Qemu-arm] [PATCH 1/3] hw/arm/exynos: Convert fprintf to error_report()
2017-03-05 21:48 [Qemu-devel] [PATCH 1/3] hw/arm/exynos: Convert fprintf to error_report() Krzysztof Kozlowski
2017-03-05 21:48 ` [Qemu-devel] [PATCH 2/3] hw/arm/exynos4210: Constify data pointed by few arguments and variables Krzysztof Kozlowski
2017-03-05 21:48 ` [Qemu-devel] [PATCH 3/3] hw/misc/exynos4210_pmu: Reorder local variables for readability Krzysztof Kozlowski
@ 2017-03-05 21:56 ` Philippe Mathieu-Daudé
2017-03-06 17:09 ` Krzysztof Kozlowski
2017-03-06 16:55 ` [Qemu-devel] " Eric Blake
3 siblings, 1 reply; 17+ messages in thread
From: Philippe Mathieu-Daudé @ 2017-03-05 21:56 UTC (permalink / raw)
To: Krzysztof Kozlowski, Igor Mitsyanko, Peter Maydell,
Paolo Bonzini, qemu-arm, qemu-devel
Hi Krzysztof,
On 03/05/2017 06:48 PM, Krzysztof Kozlowski wrote:
> error_report() is preferred over fprintf() for logging errors. Also
> remove square brackets [] and additional new line characters in printed
> messages.
>
> Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
> ---
> hw/arm/exynos4_boards.c | 6 +++---
> hw/timer/exynos4210_mct.c | 5 +++--
> hw/timer/exynos4210_pwm.c | 11 +++++------
> hw/timer/exynos4210_rtc.c | 16 +++++++---------
> 4 files changed, 18 insertions(+), 20 deletions(-)
>
> diff --git a/hw/arm/exynos4_boards.c b/hw/arm/exynos4_boards.c
> index 0efa19405409..5cd94d402b52 100644
> --- a/hw/arm/exynos4_boards.c
> +++ b/hw/arm/exynos4_boards.c
> @@ -22,6 +22,7 @@
> */
>
> #include "qemu/osdep.h"
> +#include "qemu/error-report.h"
> #include "qemu-common.h"
> #include "cpu.h"
> #include "sysemu/sysemu.h"
> @@ -101,9 +102,8 @@ static Exynos4210State *exynos4_boards_init_common(MachineState *machine,
> MachineClass *mc = MACHINE_GET_CLASS(machine);
>
> if (smp_cpus != EXYNOS4210_NCPUS && !qtest_enabled()) {
> - fprintf(stderr, "%s board supports only %d CPU cores. Ignoring smp_cpus"
> - " value.\n",
> - mc->name, EXYNOS4210_NCPUS);
> + error_report("%s board supports only %d CPU cores. Ignoring smp_cpus value.",
> + mc->name, EXYNOS4210_NCPUS);
ok, to inform the user
> }
>
> exynos4_board_binfo.ram_size = exynos4_board_ram_size[board_type];
> diff --git a/hw/timer/exynos4210_mct.c b/hw/timer/exynos4210_mct.c
> index 6069116942a4..48041ab036a6 100644
> --- a/hw/timer/exynos4210_mct.c
> +++ b/hw/timer/exynos4210_mct.c
> @@ -53,6 +53,7 @@
> */
>
> #include "qemu/osdep.h"
> +#include "qemu/error-report.h"
> #include "hw/sysbus.h"
> #include "qemu/timer.h"
> #include "qemu/main-loop.h"
> @@ -1364,8 +1365,8 @@ static void exynos4210_mct_write(void *opaque, hwaddr offset,
> case L0_TCNTO: case L1_TCNTO:
> case L0_ICNTO: case L1_ICNTO:
> case L0_FRCNTO: case L1_FRCNTO:
> - fprintf(stderr, "\n[exynos4210.mct: write to RO register "
> - TARGET_FMT_plx "]\n\n", offset);
> + error_report("exynos4210.mct: write to RO register " TARGET_FMT_plx,
> + offset);
no need to annoy the user here, it seems better to use
qemu_log_mask(LOG_UNIMP, ...
same for following diffs
> break;
>
> case L0_INT_CSTAT: case L1_INT_CSTAT:
> diff --git a/hw/timer/exynos4210_pwm.c b/hw/timer/exynos4210_pwm.c
> index f5765075c720..f8826e24e63d 100644
> --- a/hw/timer/exynos4210_pwm.c
> +++ b/hw/timer/exynos4210_pwm.c
> @@ -21,6 +21,7 @@
> */
>
> #include "qemu/osdep.h"
> +#include "qemu/error-report.h"
> #include "hw/sysbus.h"
> #include "qemu/timer.h"
> #include "qemu-common.h"
> @@ -252,9 +253,8 @@ static uint64_t exynos4210_pwm_read(void *opaque, hwaddr offset,
> break;
>
> default:
> - fprintf(stderr,
> - "[exynos4210.pwm: bad read offset " TARGET_FMT_plx "]\n",
> - offset);
> + error_report("exynos4210.pwm: bad read offset " TARGET_FMT_plx,
> + offset);
> break;
> }
> return value;
> @@ -343,9 +343,8 @@ static void exynos4210_pwm_write(void *opaque, hwaddr offset,
> break;
>
> default:
> - fprintf(stderr,
> - "[exynos4210.pwm: bad write offset " TARGET_FMT_plx "]\n",
> - offset);
> + error_report("exynos4210.pwm: bad write offset " TARGET_FMT_plx,
> + offset);
> break;
>
> }
> diff --git a/hw/timer/exynos4210_rtc.c b/hw/timer/exynos4210_rtc.c
> index 1a648c5d9e67..f4548cd555f4 100644
> --- a/hw/timer/exynos4210_rtc.c
> +++ b/hw/timer/exynos4210_rtc.c
> @@ -26,6 +26,7 @@
> */
>
> #include "qemu/osdep.h"
> +#include "qemu/error-report.h"
> #include "hw/sysbus.h"
> #include "qemu/timer.h"
> #include "qemu-common.h"
> @@ -370,9 +371,8 @@ static uint64_t exynos4210_rtc_read(void *opaque, hwaddr offset,
> break;
>
> default:
> - fprintf(stderr,
> - "[exynos4210.rtc: bad read offset " TARGET_FMT_plx "]\n",
> - offset);
> + error_report("exynos4210.rtc: bad read offset " TARGET_FMT_plx,
> + offset);
> break;
> }
> return value;
> @@ -433,9 +433,8 @@ static void exynos4210_rtc_write(void *opaque, hwaddr offset,
> if (value > TICNT_THRESHOLD) {
> s->reg_ticcnt = value;
> } else {
> - fprintf(stderr,
> - "[exynos4210.rtc: bad TICNT value %u ]\n",
> - (uint32_t)value);
> + error_report("exynos4210.rtc: bad TICNT value %u",
> + (uint32_t)value);
> }
> break;
>
> @@ -500,9 +499,8 @@ static void exynos4210_rtc_write(void *opaque, hwaddr offset,
> break;
>
> default:
> - fprintf(stderr,
> - "[exynos4210.rtc: bad write offset " TARGET_FMT_plx "]\n",
> - offset);
> + error_report("exynos4210.rtc: bad write offset " TARGET_FMT_plx,
> + offset);
> break;
>
> }
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [Qemu-arm] [PATCH 2/3] hw/arm/exynos4210: Constify data pointed by few arguments and variables
2017-03-05 21:48 ` [Qemu-devel] [PATCH 2/3] hw/arm/exynos4210: Constify data pointed by few arguments and variables Krzysztof Kozlowski
@ 2017-03-06 3:58 ` Philippe Mathieu-Daudé
2017-03-06 9:44 ` [Qemu-devel] " Peter Maydell
1 sibling, 0 replies; 17+ messages in thread
From: Philippe Mathieu-Daudé @ 2017-03-06 3:58 UTC (permalink / raw)
To: Krzysztof Kozlowski, Igor Mitsyanko, Peter Maydell,
Paolo Bonzini, qemu-arm, qemu-devel
On 03/05/2017 06:48 PM, Krzysztof Kozlowski wrote:
> In few places the function arguments and local variables are not
> modifying data passed through pointers so this can be made const for
> code safeness. Also the static array exynos4210_uart_regs is not being
> modified.
>
> Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
> hw/char/exynos4210_uart.c | 10 +++++-----
> hw/intc/exynos4210_combiner.c | 2 +-
> hw/intc/exynos4210_gic.c | 8 ++++----
> hw/misc/exynos4210_pmu.c | 2 +-
> 4 files changed, 11 insertions(+), 11 deletions(-)
>
> diff --git a/hw/char/exynos4210_uart.c b/hw/char/exynos4210_uart.c
> index b75f28d473bf..83e1be253255 100644
> --- a/hw/char/exynos4210_uart.c
> +++ b/hw/char/exynos4210_uart.c
> @@ -102,7 +102,7 @@ typedef struct Exynos4210UartReg {
> uint32_t reset_value;
> } Exynos4210UartReg;
>
> -static Exynos4210UartReg exynos4210_uart_regs[] = {
> +static const Exynos4210UartReg exynos4210_uart_regs[] = {
> {"ULCON", ULCON, 0x00000000},
> {"UCON", UCON, 0x00003000},
> {"UFCON", UFCON, 0x00000000},
> @@ -220,7 +220,7 @@ static uint8_t fifo_retrieve(Exynos4210UartFIFO *q)
> return ret;
> }
>
> -static int fifo_elements_number(Exynos4210UartFIFO *q)
> +static int fifo_elements_number(const Exynos4210UartFIFO *q)
> {
> if (q->sp < q->rp) {
> return q->size - q->rp + q->sp;
> @@ -229,7 +229,7 @@ static int fifo_elements_number(Exynos4210UartFIFO *q)
> return q->sp - q->rp;
> }
>
> -static int fifo_empty_elements_number(Exynos4210UartFIFO *q)
> +static int fifo_empty_elements_number(const Exynos4210UartFIFO *q)
> {
> return q->size - fifo_elements_number(q);
> }
> @@ -245,7 +245,7 @@ static void fifo_reset(Exynos4210UartFIFO *q)
> q->rp = 0;
> }
>
> -static uint32_t exynos4210_uart_Tx_FIFO_trigger_level(Exynos4210UartState *s)
> +static uint32_t exynos4210_uart_Tx_FIFO_trigger_level(const Exynos4210UartState *s)
> {
> uint32_t level = 0;
> uint32_t reg;
> @@ -488,7 +488,7 @@ static const MemoryRegionOps exynos4210_uart_ops = {
>
> static int exynos4210_uart_can_receive(void *opaque)
> {
> - Exynos4210UartState *s = (Exynos4210UartState *)opaque;
> + const Exynos4210UartState *s = (Exynos4210UartState *)opaque;
>
> return fifo_empty_elements_number(&s->rx);
> }
> diff --git a/hw/intc/exynos4210_combiner.c b/hw/intc/exynos4210_combiner.c
> index f19a7062be3a..b057921e0504 100644
> --- a/hw/intc/exynos4210_combiner.c
> +++ b/hw/intc/exynos4210_combiner.c
> @@ -180,7 +180,7 @@ void exynos4210_combiner_get_gpioin(Exynos4210Irq *irqs, DeviceState *dev,
> static uint64_t
> exynos4210_combiner_read(void *opaque, hwaddr offset, unsigned size)
> {
> - struct Exynos4210CombinerState *s =
> + const struct Exynos4210CombinerState *s =
> (struct Exynos4210CombinerState *)opaque;
> uint32_t req_quad_base_n; /* Base of registers quad. Multiply it by 4 and
> get a start of corresponding group quad */
> diff --git a/hw/intc/exynos4210_gic.c b/hw/intc/exynos4210_gic.c
> index 2a55817b7660..432b8425d09d 100644
> --- a/hw/intc/exynos4210_gic.c
> +++ b/hw/intc/exynos4210_gic.c
> @@ -190,7 +190,7 @@ combiner_grp_to_gic_id[64-EXYNOS4210_MAX_EXT_COMBINER_OUT_IRQ][8] = {
>
> static void exynos4210_irq_handler(void *opaque, int irq, int level)
> {
> - Exynos4210Irq *s = (Exynos4210Irq *)opaque;
> + const Exynos4210Irq *s = (Exynos4210Irq *)opaque;
>
> /* Bypass */
> qemu_set_irq(s->board_irqs[irq], level);
> @@ -277,7 +277,7 @@ typedef struct {
>
> static void exynos4210_gic_set_irq(void *opaque, int irq, int level)
> {
> - Exynos4210GicState *s = (Exynos4210GicState *)opaque;
> + const Exynos4210GicState *s = (Exynos4210GicState *)opaque;
> qemu_set_irq(qdev_get_gpio_in(s->gic, irq), level);
> }
>
> @@ -401,7 +401,7 @@ static const VMStateDescription vmstate_exynos4210_irq_gate = {
> /* Process a change in IRQ input. */
> static void exynos4210_irq_gate_handler(void *opaque, int irq, int level)
> {
> - Exynos4210IRQGateState *s = (Exynos4210IRQGateState *)opaque;
> + const Exynos4210IRQGateState *s = (Exynos4210IRQGateState *)opaque;
> uint32_t i;
>
> assert(irq < s->n_in);
> @@ -420,7 +420,7 @@ static void exynos4210_irq_gate_handler(void *opaque, int irq, int level)
>
> static void exynos4210_irq_gate_reset(DeviceState *d)
> {
> - Exynos4210IRQGateState *s = EXYNOS4210_IRQ_GATE(d);
> + const Exynos4210IRQGateState *s = EXYNOS4210_IRQ_GATE(d);
>
> memset(s->level, 0, s->n_in * sizeof(*s->level));
> }
> diff --git a/hw/misc/exynos4210_pmu.c b/hw/misc/exynos4210_pmu.c
> index e30dbc7d3d83..cbdfa0614600 100644
> --- a/hw/misc/exynos4210_pmu.c
> +++ b/hw/misc/exynos4210_pmu.c
> @@ -400,7 +400,7 @@ typedef struct Exynos4210PmuState {
> static uint64_t exynos4210_pmu_read(void *opaque, hwaddr offset,
> unsigned size)
> {
> - Exynos4210PmuState *s = (Exynos4210PmuState *)opaque;
> + const Exynos4210PmuState *s = (Exynos4210PmuState *)opaque;
> unsigned i;
> const Exynos4210PmuReg *reg_p = exynos4210_pmu_regs;
>
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [Qemu-arm] [PATCH 3/3] hw/misc/exynos4210_pmu: Reorder local variables for readability
2017-03-05 21:48 ` [Qemu-devel] [PATCH 3/3] hw/misc/exynos4210_pmu: Reorder local variables for readability Krzysztof Kozlowski
@ 2017-03-06 4:06 ` Philippe Mathieu-Daudé
2017-03-06 9:46 ` Peter Maydell
2017-03-06 17:23 ` Krzysztof Kozlowski
0 siblings, 2 replies; 17+ messages in thread
From: Philippe Mathieu-Daudé @ 2017-03-06 4:06 UTC (permalink / raw)
To: Krzysztof Kozlowski, Igor Mitsyanko, Peter Maydell,
Paolo Bonzini, qemu-arm, qemu-devel
Hi Krzysztof,
On 03/05/2017 06:48 PM, Krzysztof Kozlowski wrote:
> Short declaration of 'i' was in the middle of declarations with
> assignments. Make it a little bit more readable. No functional change.
>
> Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
> ---
> hw/misc/exynos4210_pmu.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/hw/misc/exynos4210_pmu.c b/hw/misc/exynos4210_pmu.c
> index cbdfa0614600..60d1545c0baa 100644
> --- a/hw/misc/exynos4210_pmu.c
> +++ b/hw/misc/exynos4210_pmu.c
> @@ -401,8 +401,8 @@ static uint64_t exynos4210_pmu_read(void *opaque, hwaddr offset,
> unsigned size)
> {
> const Exynos4210PmuState *s = (Exynos4210PmuState *)opaque;
> - unsigned i;
> const Exynos4210PmuReg *reg_p = exynos4210_pmu_regs;
> + unsigned i;
your change seems OK but while you are here, 'unsigned' is considered
harmful since more than a decade. why not use 'size_t i' since
PMU_NUM_OF_REGISTERS is indeed an ARRAY_SIZE()?
>
> for (i = 0; i < PMU_NUM_OF_REGISTERS; i++) {
> if (reg_p->offset == offset) {
> @@ -420,8 +420,8 @@ static void exynos4210_pmu_write(void *opaque, hwaddr offset,
> uint64_t val, unsigned size)
> {
> Exynos4210PmuState *s = (Exynos4210PmuState *)opaque;
> - unsigned i;
> const Exynos4210PmuReg *reg_p = exynos4210_pmu_regs;
> + unsigned i;
>
> for (i = 0; i < PMU_NUM_OF_REGISTERS; i++) {
> if (reg_p->offset == offset) {
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH 2/3] hw/arm/exynos4210: Constify data pointed by few arguments and variables
2017-03-05 21:48 ` [Qemu-devel] [PATCH 2/3] hw/arm/exynos4210: Constify data pointed by few arguments and variables Krzysztof Kozlowski
2017-03-06 3:58 ` [Qemu-devel] [Qemu-arm] " Philippe Mathieu-Daudé
@ 2017-03-06 9:44 ` Peter Maydell
2017-03-06 17:14 ` Krzysztof Kozlowski
1 sibling, 1 reply; 17+ messages in thread
From: Peter Maydell @ 2017-03-06 9:44 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: Igor Mitsyanko, Paolo Bonzini, qemu-arm, QEMU Developers
On 5 March 2017 at 21:48, Krzysztof Kozlowski <krzk@kernel.org> wrote:
> In few places the function arguments and local variables are not
> modifying data passed through pointers so this can be made const for
> code safeness. Also the static array exynos4210_uart_regs is not being
> modified.
>
> Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
> ---
> hw/char/exynos4210_uart.c | 10 +++++-----
> hw/intc/exynos4210_combiner.c | 2 +-
> hw/intc/exynos4210_gic.c | 8 ++++----
> hw/misc/exynos4210_pmu.c | 2 +-
> 4 files changed, 11 insertions(+), 11 deletions(-)
>
> diff --git a/hw/char/exynos4210_uart.c b/hw/char/exynos4210_uart.c
> index b75f28d473bf..83e1be253255 100644
> --- a/hw/char/exynos4210_uart.c
> +++ b/hw/char/exynos4210_uart.c
> @@ -102,7 +102,7 @@ typedef struct Exynos4210UartReg {
> uint32_t reset_value;
> } Exynos4210UartReg;
>
> -static Exynos4210UartReg exynos4210_uart_regs[] = {
> +static const Exynos4210UartReg exynos4210_uart_regs[] = {
> {"ULCON", ULCON, 0x00000000},
> {"UCON", UCON, 0x00003000},
> {"UFCON", UFCON, 0x00000000},
> @@ -220,7 +220,7 @@ static uint8_t fifo_retrieve(Exynos4210UartFIFO *q)
> return ret;
> }
Constifying this sort of thing is OK...
> -static int fifo_elements_number(Exynos4210UartFIFO *q)
> +static int fifo_elements_number(const Exynos4210UartFIFO *q)
> {
> if (q->sp < q->rp) {
> return q->size - q->rp + q->sp;
> @@ -229,7 +229,7 @@ static int fifo_elements_number(Exynos4210UartFIFO *q)
> return q->sp - q->rp;
> }
>
> -static int fifo_empty_elements_number(Exynos4210UartFIFO *q)
> +static int fifo_empty_elements_number(const Exynos4210UartFIFO *q)
> {
> return q->size - fifo_elements_number(q);
> }
> @@ -245,7 +245,7 @@ static void fifo_reset(Exynos4210UartFIFO *q)
> q->rp = 0;
> }
>
> -static uint32_t exynos4210_uart_Tx_FIFO_trigger_level(Exynos4210UartState *s)
> +static uint32_t exynos4210_uart_Tx_FIFO_trigger_level(const Exynos4210UartState *s)
> {
> uint32_t level = 0;
> uint32_t reg;
...but I disagree here somewhat...
> @@ -488,7 +488,7 @@ static const MemoryRegionOps exynos4210_uart_ops = {
>
> static int exynos4210_uart_can_receive(void *opaque)
> {
> - Exynos4210UartState *s = (Exynos4210UartState *)opaque;
> + const Exynos4210UartState *s = (Exynos4210UartState *)opaque;
...and definitely here.
These are effectively method implementations for QEMU objects,
and defining the "this" pointer as const in some methods
because it happens not to be modifying things just makes
the code look out of line with every other method implementation
in the code base (and means somebody will have to drop the 'const'
again at some point if the method needs to be updated to
modify state in the future).
thanks
-- PMM
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [Qemu-arm] [PATCH 3/3] hw/misc/exynos4210_pmu: Reorder local variables for readability
2017-03-06 4:06 ` [Qemu-devel] [Qemu-arm] " Philippe Mathieu-Daudé
@ 2017-03-06 9:46 ` Peter Maydell
2017-03-06 13:03 ` Paolo Bonzini
2017-03-06 17:23 ` Krzysztof Kozlowski
1 sibling, 1 reply; 17+ messages in thread
From: Peter Maydell @ 2017-03-06 9:46 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Krzysztof Kozlowski, Igor Mitsyanko, Paolo Bonzini, qemu-arm,
QEMU Developers
On 6 March 2017 at 04:06, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> your change seems OK but while you are here, 'unsigned' is considered
> harmful since more than a decade.
Considered harmful by who, and why?
thanks
-- PMM
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [Qemu-arm] [PATCH 3/3] hw/misc/exynos4210_pmu: Reorder local variables for readability
2017-03-06 9:46 ` Peter Maydell
@ 2017-03-06 13:03 ` Paolo Bonzini
2017-03-06 13:16 ` Peter Maydell
0 siblings, 1 reply; 17+ messages in thread
From: Paolo Bonzini @ 2017-03-06 13:03 UTC (permalink / raw)
To: Peter Maydell, Philippe Mathieu-Daudé
Cc: Krzysztof Kozlowski, Igor Mitsyanko, qemu-arm, QEMU Developers
On 06/03/2017 10:46, Peter Maydell wrote:
> On 6 March 2017 at 04:06, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>> your change seems OK but while you are here, 'unsigned' is considered
>> harmful since more than a decade.
>
> Considered harmful by who, and why?
It is true that unsigned has all the disadvantages of "int" (it may be
smaller than the size of the object" and all the disadvantages of
"size_t" (it doesn't optimize as well because the compiler cannot
exploit undefined behavior).
I wouldn't call it harmful, but it feels like the worst of both worlds.
Paolo
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [Qemu-arm] [PATCH 3/3] hw/misc/exynos4210_pmu: Reorder local variables for readability
2017-03-06 13:03 ` Paolo Bonzini
@ 2017-03-06 13:16 ` Peter Maydell
2017-03-06 13:34 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 17+ messages in thread
From: Peter Maydell @ 2017-03-06 13:16 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Philippe Mathieu-Daudé,
Krzysztof Kozlowski, Igor Mitsyanko, qemu-arm, QEMU Developers
On 6 March 2017 at 13:03, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
>
> On 06/03/2017 10:46, Peter Maydell wrote:
>> On 6 March 2017 at 04:06, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>> your change seems OK but while you are here, 'unsigned' is considered
>>> harmful since more than a decade.
>>
>> Considered harmful by who, and why?
>
> It is true that unsigned has all the disadvantages of "int" (it may be
> smaller than the size of the object" and all the disadvantages of
> "size_t" (it doesn't optimize as well because the compiler cannot
> exploit undefined behavior).
>
> I wouldn't call it harmful, but it feels like the worst of both worlds.
size_t is a pretty silly choice for a variable that's looping
through the number of registers in the device, which is a value
that would fit in 16 bits, let alone 32. I would probably have
written this with 'int', but use of 'unsigned' doesn't come
very high up on my list of things to complain about.
(since we use -fwrapv the compiler can't exploit undefined
behaviour for signed loop indexes either.)
thanks
-- PMM
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [Qemu-arm] [PATCH 3/3] hw/misc/exynos4210_pmu: Reorder local variables for readability
2017-03-06 13:16 ` Peter Maydell
@ 2017-03-06 13:34 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 17+ messages in thread
From: Philippe Mathieu-Daudé @ 2017-03-06 13:34 UTC (permalink / raw)
To: Peter Maydell, Paolo Bonzini
Cc: Krzysztof Kozlowski, Igor Mitsyanko, qemu-arm, QEMU Developers
On 03/06/2017 10:16 AM, Peter Maydell wrote:
> On 6 March 2017 at 13:03, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> On 06/03/2017 10:46, Peter Maydell wrote:
>>> On 6 March 2017 at 04:06, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>>> your change seems OK but while you are here, 'unsigned' is considered
>>>> harmful since more than a decade.
>>>
>>> Considered harmful by who, and why?
sorry to cry wolf :)
>> It is true that unsigned has all the disadvantages of "int" (it may be
>> smaller than the size of the object" and all the disadvantages of
>> "size_t" (it doesn't optimize as well because the compiler cannot
>> exploit undefined behavior).
>>
>> I wouldn't call it harmful, but it feels like the worst of both worlds.
>
> size_t is a pretty silly choice for a variable that's looping
> through the number of registers in the device, which is a value
> that would fit in 16 bits, let alone 32. I would probably have
> written this with 'int', but use of 'unsigned' doesn't come
> very high up on my list of things to complain about.
silly would be "typeof(PMU_NUM_OF_REGISTERS) i;" :p
I got your point about the complain priority list.
> (since we use -fwrapv the compiler can't exploit undefined
> behaviour for signed loop indexes either.)
regards,
Phil.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH 1/3] hw/arm/exynos: Convert fprintf to error_report()
2017-03-05 21:48 [Qemu-devel] [PATCH 1/3] hw/arm/exynos: Convert fprintf to error_report() Krzysztof Kozlowski
` (2 preceding siblings ...)
2017-03-05 21:56 ` [Qemu-devel] [Qemu-arm] [PATCH 1/3] hw/arm/exynos: Convert fprintf to error_report() Philippe Mathieu-Daudé
@ 2017-03-06 16:55 ` Eric Blake
2017-03-06 17:07 ` Krzysztof Kozlowski
3 siblings, 1 reply; 17+ messages in thread
From: Eric Blake @ 2017-03-06 16:55 UTC (permalink / raw)
To: Krzysztof Kozlowski, Igor Mitsyanko, Peter Maydell,
Paolo Bonzini, qemu-arm, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 1119 bytes --]
On 03/05/2017 03:48 PM, Krzysztof Kozlowski wrote:
> error_report() is preferred over fprintf() for logging errors. Also
> remove square brackets [] and additional new line characters in printed
> messages.
>
> Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
> ---
I don't see a 0/3 cover letter. When sending multiple patches, it's
always best to package them in-reply-to the cover letter ('git config
format.coverLetter auto' makes it easy to remember).
> @@ -101,9 +102,8 @@ static Exynos4210State *exynos4_boards_init_common(MachineState *machine,
> MachineClass *mc = MACHINE_GET_CLASS(machine);
>
> if (smp_cpus != EXYNOS4210_NCPUS && !qtest_enabled()) {
> - fprintf(stderr, "%s board supports only %d CPU cores. Ignoring smp_cpus"
> - " value.\n",
> - mc->name, EXYNOS4210_NCPUS);
> + error_report("%s board supports only %d CPU cores. Ignoring smp_cpus value.",
Most uses of error_report() avoid trailing dot.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH 1/3] hw/arm/exynos: Convert fprintf to error_report()
2017-03-06 16:55 ` [Qemu-devel] " Eric Blake
@ 2017-03-06 17:07 ` Krzysztof Kozlowski
2017-03-06 21:41 ` Peter Maydell
0 siblings, 1 reply; 17+ messages in thread
From: Krzysztof Kozlowski @ 2017-03-06 17:07 UTC (permalink / raw)
To: Eric Blake
Cc: Igor Mitsyanko, Peter Maydell, Paolo Bonzini, qemu-arm, qemu-devel
On Mon, Mar 06, 2017 at 10:55:12AM -0600, Eric Blake wrote:
> On 03/05/2017 03:48 PM, Krzysztof Kozlowski wrote:
> > error_report() is preferred over fprintf() for logging errors. Also
> > remove square brackets [] and additional new line characters in printed
> > messages.
> >
> > Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
> > ---
>
> I don't see a 0/3 cover letter. When sending multiple patches, it's
> always best to package them in-reply-to the cover letter ('git config
> format.coverLetter auto' makes it easy to remember).
The patches are trivial cleanups without any dependencies so the cover
letter would be one sentence of "trivial cleanup". :)
If that's the practice in qemu, I can send them, no problem.
>
>
> > @@ -101,9 +102,8 @@ static Exynos4210State *exynos4_boards_init_common(MachineState *machine,
> > MachineClass *mc = MACHINE_GET_CLASS(machine);
> >
> > if (smp_cpus != EXYNOS4210_NCPUS && !qtest_enabled()) {
> > - fprintf(stderr, "%s board supports only %d CPU cores. Ignoring smp_cpus"
> > - " value.\n",
> > - mc->name, EXYNOS4210_NCPUS);
> > + error_report("%s board supports only %d CPU cores. Ignoring smp_cpus value.",
>
> Most uses of error_report() avoid trailing dot.
Sure, I can rephrase it to one sentence without dot.
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [Qemu-arm] [PATCH 1/3] hw/arm/exynos: Convert fprintf to error_report()
2017-03-05 21:56 ` [Qemu-devel] [Qemu-arm] [PATCH 1/3] hw/arm/exynos: Convert fprintf to error_report() Philippe Mathieu-Daudé
@ 2017-03-06 17:09 ` Krzysztof Kozlowski
0 siblings, 0 replies; 17+ messages in thread
From: Krzysztof Kozlowski @ 2017-03-06 17:09 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Igor Mitsyanko, Peter Maydell, Paolo Bonzini, qemu-arm, qemu-devel
On Sun, Mar 05, 2017 at 06:56:09PM -0300, Philippe Mathieu-Daudé wrote:
> Hi Krzysztof,
>
> On 03/05/2017 06:48 PM, Krzysztof Kozlowski wrote:
> > error_report() is preferred over fprintf() for logging errors. Also
> > remove square brackets [] and additional new line characters in printed
> > messages.
> >
> > Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
> > ---
> > hw/arm/exynos4_boards.c | 6 +++---
> > hw/timer/exynos4210_mct.c | 5 +++--
> > hw/timer/exynos4210_pwm.c | 11 +++++------
> > hw/timer/exynos4210_rtc.c | 16 +++++++---------
> > 4 files changed, 18 insertions(+), 20 deletions(-)
> >
> > diff --git a/hw/arm/exynos4_boards.c b/hw/arm/exynos4_boards.c
> > index 0efa19405409..5cd94d402b52 100644
> > --- a/hw/arm/exynos4_boards.c
> > +++ b/hw/arm/exynos4_boards.c
> > @@ -22,6 +22,7 @@
> > */
> >
> > #include "qemu/osdep.h"
> > +#include "qemu/error-report.h"
> > #include "qemu-common.h"
> > #include "cpu.h"
> > #include "sysemu/sysemu.h"
> > @@ -101,9 +102,8 @@ static Exynos4210State *exynos4_boards_init_common(MachineState *machine,
> > MachineClass *mc = MACHINE_GET_CLASS(machine);
> >
> > if (smp_cpus != EXYNOS4210_NCPUS && !qtest_enabled()) {
> > - fprintf(stderr, "%s board supports only %d CPU cores. Ignoring smp_cpus"
> > - " value.\n",
> > - mc->name, EXYNOS4210_NCPUS);
> > + error_report("%s board supports only %d CPU cores. Ignoring smp_cpus value.",
> > + mc->name, EXYNOS4210_NCPUS);
>
> ok, to inform the user
By inform you mean error_report or some lower level?
>
> > }
> >
> > exynos4_board_binfo.ram_size = exynos4_board_ram_size[board_type];
> > diff --git a/hw/timer/exynos4210_mct.c b/hw/timer/exynos4210_mct.c
> > index 6069116942a4..48041ab036a6 100644
> > --- a/hw/timer/exynos4210_mct.c
> > +++ b/hw/timer/exynos4210_mct.c
> > @@ -53,6 +53,7 @@
> > */
> >
> > #include "qemu/osdep.h"
> > +#include "qemu/error-report.h"
> > #include "hw/sysbus.h"
> > #include "qemu/timer.h"
> > #include "qemu/main-loop.h"
> > @@ -1364,8 +1365,8 @@ static void exynos4210_mct_write(void *opaque, hwaddr offset,
> > case L0_TCNTO: case L1_TCNTO:
> > case L0_ICNTO: case L1_ICNTO:
> > case L0_FRCNTO: case L1_FRCNTO:
> > - fprintf(stderr, "\n[exynos4210.mct: write to RO register "
> > - TARGET_FMT_plx "]\n\n", offset);
> > + error_report("exynos4210.mct: write to RO register " TARGET_FMT_plx,
> > + offset);
>
> no need to annoy the user here, it seems better to use
> qemu_log_mask(LOG_UNIMP, ...
>
> same for following diffs
Sure, I'll fix that as well.
Thanks for review.
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH 2/3] hw/arm/exynos4210: Constify data pointed by few arguments and variables
2017-03-06 9:44 ` [Qemu-devel] " Peter Maydell
@ 2017-03-06 17:14 ` Krzysztof Kozlowski
0 siblings, 0 replies; 17+ messages in thread
From: Krzysztof Kozlowski @ 2017-03-06 17:14 UTC (permalink / raw)
To: Peter Maydell; +Cc: Igor Mitsyanko, Paolo Bonzini, qemu-arm, QEMU Developers
On Mon, Mar 06, 2017 at 09:44:49AM +0000, Peter Maydell wrote:
> On 5 March 2017 at 21:48, Krzysztof Kozlowski <krzk@kernel.org> wrote:
> > In few places the function arguments and local variables are not
> > modifying data passed through pointers so this can be made const for
> > code safeness. Also the static array exynos4210_uart_regs is not being
> > modified.
> >
> > Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
> > ---
> > hw/char/exynos4210_uart.c | 10 +++++-----
> > hw/intc/exynos4210_combiner.c | 2 +-
> > hw/intc/exynos4210_gic.c | 8 ++++----
> > hw/misc/exynos4210_pmu.c | 2 +-
> > 4 files changed, 11 insertions(+), 11 deletions(-)
> >
> > diff --git a/hw/char/exynos4210_uart.c b/hw/char/exynos4210_uart.c
> > index b75f28d473bf..83e1be253255 100644
> > --- a/hw/char/exynos4210_uart.c
> > +++ b/hw/char/exynos4210_uart.c
> > @@ -102,7 +102,7 @@ typedef struct Exynos4210UartReg {
> > uint32_t reset_value;
> > } Exynos4210UartReg;
> >
> > -static Exynos4210UartReg exynos4210_uart_regs[] = {
> > +static const Exynos4210UartReg exynos4210_uart_regs[] = {
> > {"ULCON", ULCON, 0x00000000},
> > {"UCON", UCON, 0x00003000},
> > {"UFCON", UFCON, 0x00000000},
> > @@ -220,7 +220,7 @@ static uint8_t fifo_retrieve(Exynos4210UartFIFO *q)
> > return ret;
> > }
>
> Constifying this sort of thing is OK...
>
> > -static int fifo_elements_number(Exynos4210UartFIFO *q)
> > +static int fifo_elements_number(const Exynos4210UartFIFO *q)
> > {
> > if (q->sp < q->rp) {
> > return q->size - q->rp + q->sp;
> > @@ -229,7 +229,7 @@ static int fifo_elements_number(Exynos4210UartFIFO *q)
> > return q->sp - q->rp;
> > }
> >
> > -static int fifo_empty_elements_number(Exynos4210UartFIFO *q)
> > +static int fifo_empty_elements_number(const Exynos4210UartFIFO *q)
> > {
> > return q->size - fifo_elements_number(q);
> > }
> > @@ -245,7 +245,7 @@ static void fifo_reset(Exynos4210UartFIFO *q)
> > q->rp = 0;
> > }
> >
> > -static uint32_t exynos4210_uart_Tx_FIFO_trigger_level(Exynos4210UartState *s)
> > +static uint32_t exynos4210_uart_Tx_FIFO_trigger_level(const Exynos4210UartState *s)
> > {
> > uint32_t level = 0;
> > uint32_t reg;
>
> ...but I disagree here somewhat...
This is a static function not modifying the state. Const in argument is
a clear indication of that for the readers.
>
> > @@ -488,7 +488,7 @@ static const MemoryRegionOps exynos4210_uart_ops = {
> >
> > static int exynos4210_uart_can_receive(void *opaque)
> > {
> > - Exynos4210UartState *s = (Exynos4210UartState *)opaque;
> > + const Exynos4210UartState *s = (Exynos4210UartState *)opaque;
>
> ...and definitely here.
>
> These are effectively method implementations for QEMU objects,
> and defining the "this" pointer as const in some methods
> because it happens not to be modifying things just makes
> the code look out of line with every other method implementation
I get it, better to keep consistent style.
> in the code base (and means somebody will have to drop the 'const'
> again at some point if the method needs to be updated to
> modify state in the future).
I think the code should be const-correct for current implementation
(following the HACKING guide). If we would have to predict future
changes, then almost no const would be possible to add...
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [Qemu-arm] [PATCH 3/3] hw/misc/exynos4210_pmu: Reorder local variables for readability
2017-03-06 4:06 ` [Qemu-devel] [Qemu-arm] " Philippe Mathieu-Daudé
2017-03-06 9:46 ` Peter Maydell
@ 2017-03-06 17:23 ` Krzysztof Kozlowski
1 sibling, 0 replies; 17+ messages in thread
From: Krzysztof Kozlowski @ 2017-03-06 17:23 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Igor Mitsyanko, Peter Maydell, Paolo Bonzini, qemu-arm, qemu-devel
On Mon, Mar 06, 2017 at 01:06:29AM -0300, Philippe Mathieu-Daudé wrote:
> Hi Krzysztof,
>
> On 03/05/2017 06:48 PM, Krzysztof Kozlowski wrote:
> > Short declaration of 'i' was in the middle of declarations with
> > assignments. Make it a little bit more readable. No functional change.
> >
> > Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
> > ---
> > hw/misc/exynos4210_pmu.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/hw/misc/exynos4210_pmu.c b/hw/misc/exynos4210_pmu.c
> > index cbdfa0614600..60d1545c0baa 100644
> > --- a/hw/misc/exynos4210_pmu.c
> > +++ b/hw/misc/exynos4210_pmu.c
> > @@ -401,8 +401,8 @@ static uint64_t exynos4210_pmu_read(void *opaque, hwaddr offset,
> > unsigned size)
> > {
> > const Exynos4210PmuState *s = (Exynos4210PmuState *)opaque;
> > - unsigned i;
> > const Exynos4210PmuReg *reg_p = exynos4210_pmu_regs;
> > + unsigned i;
>
> your change seems OK but while you are here, 'unsigned' is considered
> harmful since more than a decade. why not use 'size_t i' since
> PMU_NUM_OF_REGISTERS is indeed an ARRAY_SIZE()?
It is a loop counter. I think "unsigned int" for loop counters is the
most common pattern - easy to understand the true meaning of variable.
I can expand it to "unsigned int" but that wouldn't change much...
Let's look at current style:
$ git grep "unsigned int i[,;$]" | wc -l
188
$ git grep "unsigned i[,;$]" | wc -l
92
$ git grep "size_t i[,;$]" | wc -l
114
... where do we go now?
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH 1/3] hw/arm/exynos: Convert fprintf to error_report()
2017-03-06 17:07 ` Krzysztof Kozlowski
@ 2017-03-06 21:41 ` Peter Maydell
0 siblings, 0 replies; 17+ messages in thread
From: Peter Maydell @ 2017-03-06 21:41 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: Eric Blake, Igor Mitsyanko, Paolo Bonzini, qemu-arm, QEMU Developers
On 6 March 2017 at 17:07, Krzysztof Kozlowski <krzk@kernel.org> wrote:
> On Mon, Mar 06, 2017 at 10:55:12AM -0600, Eric Blake wrote:
>> I don't see a 0/3 cover letter. When sending multiple patches, it's
>> always best to package them in-reply-to the cover letter ('git config
>> format.coverLetter auto' makes it easy to remember).
>
> The patches are trivial cleanups without any dependencies so the cover
> letter would be one sentence of "trivial cleanup". :)
>
> If that's the practice in qemu, I can send them, no problem.
Yeah, cover letter always for multipatch sets, please. We have
automated tooling that relies on this (and also manual humans).
thanks
-- PMM
^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2017-03-06 21:41 UTC | newest]
Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-03-05 21:48 [Qemu-devel] [PATCH 1/3] hw/arm/exynos: Convert fprintf to error_report() Krzysztof Kozlowski
2017-03-05 21:48 ` [Qemu-devel] [PATCH 2/3] hw/arm/exynos4210: Constify data pointed by few arguments and variables Krzysztof Kozlowski
2017-03-06 3:58 ` [Qemu-devel] [Qemu-arm] " Philippe Mathieu-Daudé
2017-03-06 9:44 ` [Qemu-devel] " Peter Maydell
2017-03-06 17:14 ` Krzysztof Kozlowski
2017-03-05 21:48 ` [Qemu-devel] [PATCH 3/3] hw/misc/exynos4210_pmu: Reorder local variables for readability Krzysztof Kozlowski
2017-03-06 4:06 ` [Qemu-devel] [Qemu-arm] " Philippe Mathieu-Daudé
2017-03-06 9:46 ` Peter Maydell
2017-03-06 13:03 ` Paolo Bonzini
2017-03-06 13:16 ` Peter Maydell
2017-03-06 13:34 ` Philippe Mathieu-Daudé
2017-03-06 17:23 ` Krzysztof Kozlowski
2017-03-05 21:56 ` [Qemu-devel] [Qemu-arm] [PATCH 1/3] hw/arm/exynos: Convert fprintf to error_report() Philippe Mathieu-Daudé
2017-03-06 17:09 ` Krzysztof Kozlowski
2017-03-06 16:55 ` [Qemu-devel] " Eric Blake
2017-03-06 17:07 ` Krzysztof Kozlowski
2017-03-06 21:41 ` Peter Maydell
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.