> Subject: Re: [PATCH v2 09/10] hw/intc: Remove redundant statement in > exynos4210_combiner_read() > > On Tue, 25 Aug 2020 at 12:26, Chen Qun <kuhn.chen...@huawei.com> wrote: > > > > Clang static code analyzer show warning: > > hw/intc/exynos4210_combiner.c:231:9: warning: Value stored to 'val' is never > read > > val = s->reg_set[offset >> 2]; > > > > The default value of 'val' is '0', so we can break the 'default' branch and > > return > 'val'. > > > > Reported-by: Euler Robot <euler.ro...@huawei.com> > > Signed-off-by: Chen Qun <kuhn.chen...@huawei.com> > > --- > > Cc: Igor Mitsyanko <i.mitsya...@gmail.com> > > Cc: Peter Maydell <peter.mayd...@linaro.org> > > --- > > hw/intc/exynos4210_combiner.c | 3 +-- > > 1 file changed, 1 insertion(+), 2 deletions(-) > > > > diff --git a/hw/intc/exynos4210_combiner.c > > b/hw/intc/exynos4210_combiner.c index b8561e4180..e2e745bbaa 100644 > > --- a/hw/intc/exynos4210_combiner.c > > +++ b/hw/intc/exynos4210_combiner.c > > @@ -228,8 +228,7 @@ exynos4210_combiner_read(void *opaque, hwaddr > offset, unsigned size) > > hw_error("exynos4210.combiner: overflow of reg_set by 0x" > > TARGET_FMT_plx "offset\n", offset); > > } > > - val = s->reg_set[offset >> 2]; > > - return 0; > > + break; > > } > > return val; > > } > > The code as it stands is definitely wrong, but I'm not sure this is the > correct fix. > Surely the intention must have been to return the actual register value from > the reg_set[] array, not to return 0 ? > > I suspect the correct fix here is simply to delete the "return 0" line and > leave > the assignment to val as it is.
Hi Peter, I think you're right. > Ideally you should check the h/w datasheet to confirm. I checked the Exynos 4210 datasheet and found that 'Interrupt Combiner Operation' had only five types of registers. Register Description Type ---------------------------------------------------------------------------------------------------------------------------------------- IESR(0/1/2/3) Interrupt Enable Set Register for Group (0~3/ 4 ~7/ 8~ 11/ 12~ 15) RW IECR(0/1/2/3) Interrupt Enable Clear Register for Group (0~3/ 4 ~7/ 8~ 11/ 12~ 15) RW ISTR(0/1/2/3) Interrupt Status Register for Group (0~3/ 4 ~7/ 8~ 11/ 12~ 15) R IMSR(0/1/2/3) Interrupt Masked Status Register for Group(0~3/ 4 ~7/ 8~ 11/ 12~ 15) R CIPSR0 Combined Interrupt Pending Status0 R The exynos4210_combiner_write() function has write operations for IESR and IECR. The CIPSR, ISTR, and IMSR read operations are specified in the exynos4210_combiner_read() function. So, This 'default' branch in exynos4210_combiner_read() function read operation is only for IESR and IECR. And, in the exynos4210_combiner_write function, the default assignment statement for IESR and IECR is " s->reg_set[offset >> 2] = val;". Therefore, if we read the two register(IESR and IECR) values, we can directly use this assignment statement "val = s->reg_set[offset >> 2];". Please confirm that if there is no problem, I will modify it in later version. Thanks, Chen Qun