| Message ID | 20180622134036.23182-8-f4bug@amsat.org |
|---|---|
| State | New |
| Headers | show |
| Series | hw/arm: use qemu_log_mask instead of fprintf | expand |
On 22.06.2018 15:40, Philippe Mathieu-Daudé wrote: > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > --- > hw/i2c/omap_i2c.c | 20 ++++++++++++-------- > 1 file changed, 12 insertions(+), 8 deletions(-) > > diff --git a/hw/i2c/omap_i2c.c b/hw/i2c/omap_i2c.c > index 26e3e5ebf6..690876e43e 100644 > --- a/hw/i2c/omap_i2c.c > +++ b/hw/i2c/omap_i2c.c > @@ -17,6 +17,7 @@ > * with this program; if not, see <http://www.gnu.org/licenses/>. > */ > #include "qemu/osdep.h" > +#include "qemu/log.h" > #include "hw/hw.h" > #include "hw/i2c/i2c.h" > #include "hw/arm/omap.h" > @@ -339,14 +340,15 @@ static void omap_i2c_write(void *opaque, hwaddr addr, > } > break; > } > - if ((value & (1 << 15)) && !(value & (1 << 10))) { /* MST */ > - fprintf(stderr, "%s: I^2C slave mode not supported\n", > - __func__); > + if ((value & (1 << 15)) && !(value & (1 << 10))) { /* MST */ Please keep the white spaces before the comment if you don't change anything else. > + qemu_log_mask(LOG_UNIMP, "%s: I^2C slave mode not supported\n", > + __func__); > break; > } > - if ((value & (1 << 15)) && value & (1 << 8)) { /* XA */ > - fprintf(stderr, "%s: 10-bit addressing mode not supported\n", > - __func__); > + if ((value & (1 << 15)) && value & (1 << 8)) { /* XA */ dito. > + qemu_log_mask(LOG_UNIMP, > + "%s: 10-bit addressing mode not supported\n", > + __func__); Apart from the nits, looks fine to me, so once you've fixed that: Reviewed-by: Thomas Huth <thuth@redhat.com>
On 06/22/2018 04:38 PM, Thomas Huth wrote: > On 22.06.2018 15:40, Philippe Mathieu-Daudé wrote: >> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> >> --- >> hw/i2c/omap_i2c.c | 20 ++++++++++++-------- >> 1 file changed, 12 insertions(+), 8 deletions(-) >> >> diff --git a/hw/i2c/omap_i2c.c b/hw/i2c/omap_i2c.c >> index 26e3e5ebf6..690876e43e 100644 >> --- a/hw/i2c/omap_i2c.c >> +++ b/hw/i2c/omap_i2c.c >> @@ -17,6 +17,7 @@ >> * with this program; if not, see <http://www.gnu.org/licenses/>. >> */ >> #include "qemu/osdep.h" >> +#include "qemu/log.h" >> #include "hw/hw.h" >> #include "hw/i2c/i2c.h" >> #include "hw/arm/omap.h" >> @@ -339,14 +340,15 @@ static void omap_i2c_write(void *opaque, hwaddr addr, >> } >> break; >> } >> - if ((value & (1 << 15)) && !(value & (1 << 10))) { /* MST */ >> - fprintf(stderr, "%s: I^2C slave mode not supported\n", >> - __func__); >> + if ((value & (1 << 15)) && !(value & (1 << 10))) { /* MST */ > > Please keep the white spaces before the comment if you don't change > anything else. This is a <tab> and checkpatch complains... I can use 4 spaces for this tab. I tried to align with other tab-aligned comments I didn't modify, but the result is messier. Thus a simple space. > >> + qemu_log_mask(LOG_UNIMP, "%s: I^2C slave mode not supported\n", >> + __func__); >> break; >> } >> - if ((value & (1 << 15)) && value & (1 << 8)) { /* XA */ >> - fprintf(stderr, "%s: 10-bit addressing mode not supported\n", >> - __func__); >> + if ((value & (1 << 15)) && value & (1 << 8)) { /* XA */ > > dito. > >> + qemu_log_mask(LOG_UNIMP, >> + "%s: 10-bit addressing mode not supported\n", >> + __func__); > > Apart from the nits, looks fine to me, so once you've fixed that: > > Reviewed-by: Thomas Huth <thuth@redhat.com> >
On 22.06.2018 22:10, Philippe Mathieu-Daudé wrote: > On 06/22/2018 04:38 PM, Thomas Huth wrote: >> On 22.06.2018 15:40, Philippe Mathieu-Daudé wrote: >>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> >>> --- >>> hw/i2c/omap_i2c.c | 20 ++++++++++++-------- >>> 1 file changed, 12 insertions(+), 8 deletions(-) >>> >>> diff --git a/hw/i2c/omap_i2c.c b/hw/i2c/omap_i2c.c >>> index 26e3e5ebf6..690876e43e 100644 >>> --- a/hw/i2c/omap_i2c.c >>> +++ b/hw/i2c/omap_i2c.c >>> @@ -17,6 +17,7 @@ >>> * with this program; if not, see <http://www.gnu.org/licenses/>. >>> */ >>> #include "qemu/osdep.h" >>> +#include "qemu/log.h" >>> #include "hw/hw.h" >>> #include "hw/i2c/i2c.h" >>> #include "hw/arm/omap.h" >>> @@ -339,14 +340,15 @@ static void omap_i2c_write(void *opaque, hwaddr addr, >>> } >>> break; >>> } >>> - if ((value & (1 << 15)) && !(value & (1 << 10))) { /* MST */ >>> - fprintf(stderr, "%s: I^2C slave mode not supported\n", >>> - __func__); >>> + if ((value & (1 << 15)) && !(value & (1 << 10))) { /* MST */ >> >> Please keep the white spaces before the comment if you don't change >> anything else. > > This is a <tab> and checkpatch complains... > > I can use 4 spaces for this tab. I tried to align with other tab-aligned > comments I didn't modify, but the result is messier. Thus a simple space. Oh, sorry, I didn't notice that you've replaced a TAB here. I guess it's ok then. But why does checkpatch complain if it is just in the context of your modification? That's weird. Thomas
On 06/25/2018 03:08 AM, Thomas Huth wrote: > On 22.06.2018 22:10, Philippe Mathieu-Daudé wrote: >> On 06/22/2018 04:38 PM, Thomas Huth wrote: >>> On 22.06.2018 15:40, Philippe Mathieu-Daudé wrote: >>>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> >>>> --- >>>> hw/i2c/omap_i2c.c | 20 ++++++++++++-------- >>>> 1 file changed, 12 insertions(+), 8 deletions(-) >>>> >>>> diff --git a/hw/i2c/omap_i2c.c b/hw/i2c/omap_i2c.c >>>> index 26e3e5ebf6..690876e43e 100644 >>>> --- a/hw/i2c/omap_i2c.c >>>> +++ b/hw/i2c/omap_i2c.c >>>> @@ -17,6 +17,7 @@ >>>> * with this program; if not, see <http://www.gnu.org/licenses/>. >>>> */ >>>> #include "qemu/osdep.h" >>>> +#include "qemu/log.h" >>>> #include "hw/hw.h" >>>> #include "hw/i2c/i2c.h" >>>> #include "hw/arm/omap.h" >>>> @@ -339,14 +340,15 @@ static void omap_i2c_write(void *opaque, hwaddr addr, >>>> } >>>> break; >>>> } >>>> - if ((value & (1 << 15)) && !(value & (1 << 10))) { /* MST */ >>>> - fprintf(stderr, "%s: I^2C slave mode not supported\n", >>>> - __func__); >>>> + if ((value & (1 << 15)) && !(value & (1 << 10))) { /* MST */ >>> >>> Please keep the white spaces before the comment if you don't change >>> anything else. >> >> This is a <tab> and checkpatch complains... >> >> I can use 4 spaces for this tab. I tried to align with other tab-aligned >> comments I didn't modify, but the result is messier. Thus a simple space. > > Oh, sorry, I didn't notice that you've replaced a TAB here. I guess it's > ok then. But why does checkpatch complain if it is just in the context > of your modification? That's weird. The first 2 contexts (MST and XA) are fine, however checkpatch complains with the last one (ST_EN): ERROR: code indent should never use tabs #38: FILE: hw/i2c/omap_i2c.c:397: + if (value & (1 << 15)) {^I^I^I^I^I/* ST_EN */$ Since I replaced this one, I also did with the 2 previous. Now I realize I can _not_ add the brackets so I don't have to update the <tabs>: if (value & (1 << 15))^I^I^I^I^I/* ST_EN */ - fprintf(stderr, "%s: System Test not supported\n", __func__); + qemu_log_mask(LOG_UNIMP, + "%s: System Test not supported\n", __func__); break; I think if it better to unify the code style when possible, but it is up to you, if you prefer I can resend with tabs and no brackets. Regards, Phil.
On 25.06.2018 14:52, Philippe Mathieu-Daudé wrote: > On 06/25/2018 03:08 AM, Thomas Huth wrote: >> On 22.06.2018 22:10, Philippe Mathieu-Daudé wrote: >>> On 06/22/2018 04:38 PM, Thomas Huth wrote: >>>> On 22.06.2018 15:40, Philippe Mathieu-Daudé wrote: >>>>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> >>>>> --- >>>>> hw/i2c/omap_i2c.c | 20 ++++++++++++-------- >>>>> 1 file changed, 12 insertions(+), 8 deletions(-) >>>>> >>>>> diff --git a/hw/i2c/omap_i2c.c b/hw/i2c/omap_i2c.c >>>>> index 26e3e5ebf6..690876e43e 100644 >>>>> --- a/hw/i2c/omap_i2c.c >>>>> +++ b/hw/i2c/omap_i2c.c >>>>> @@ -17,6 +17,7 @@ >>>>> * with this program; if not, see <http://www.gnu.org/licenses/>. >>>>> */ >>>>> #include "qemu/osdep.h" >>>>> +#include "qemu/log.h" >>>>> #include "hw/hw.h" >>>>> #include "hw/i2c/i2c.h" >>>>> #include "hw/arm/omap.h" >>>>> @@ -339,14 +340,15 @@ static void omap_i2c_write(void *opaque, hwaddr addr, >>>>> } >>>>> break; >>>>> } >>>>> - if ((value & (1 << 15)) && !(value & (1 << 10))) { /* MST */ >>>>> - fprintf(stderr, "%s: I^2C slave mode not supported\n", >>>>> - __func__); >>>>> + if ((value & (1 << 15)) && !(value & (1 << 10))) { /* MST */ >>>> >>>> Please keep the white spaces before the comment if you don't change >>>> anything else. >>> >>> This is a <tab> and checkpatch complains... >>> >>> I can use 4 spaces for this tab. I tried to align with other tab-aligned >>> comments I didn't modify, but the result is messier. Thus a simple space. >> >> Oh, sorry, I didn't notice that you've replaced a TAB here. I guess it's >> ok then. But why does checkpatch complain if it is just in the context >> of your modification? That's weird. > > The first 2 contexts (MST and XA) are fine, however checkpatch complains > with the last one (ST_EN): > > ERROR: code indent should never use tabs > #38: FILE: hw/i2c/omap_i2c.c:397: > + if (value & (1 << 15)) {^I^I^I^I^I/* ST_EN */$ > > Since I replaced this one, I also did with the 2 previous. > > Now I realize I can _not_ add the brackets so I don't have to update the > <tabs>: > > if (value & (1 << 15))^I^I^I^I^I/* ST_EN */ > - fprintf(stderr, "%s: System Test not supported\n", __func__); > + qemu_log_mask(LOG_UNIMP, > + "%s: System Test not supported\n", __func__); > break; > > I think if it better to unify the code style when possible, but it is up > to you, if you prefer I can resend with tabs and no brackets. I think it's OK to fix up the coding style here, too. Maybe just mention it in the patch description ("While we're at it, change TABs to spaces and add missing curly braces to the surounding if-statements" or so). Thomas
On 06/25/2018 11:30 AM, Thomas Huth wrote: > On 25.06.2018 14:52, Philippe Mathieu-Daudé wrote: >> On 06/25/2018 03:08 AM, Thomas Huth wrote: >>> On 22.06.2018 22:10, Philippe Mathieu-Daudé wrote: >>>> On 06/22/2018 04:38 PM, Thomas Huth wrote: >>>>> On 22.06.2018 15:40, Philippe Mathieu-Daudé wrote: >>>>>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> >>>>>> --- >>>>>> hw/i2c/omap_i2c.c | 20 ++++++++++++-------- >>>>>> 1 file changed, 12 insertions(+), 8 deletions(-) >>>>>> >>>>>> diff --git a/hw/i2c/omap_i2c.c b/hw/i2c/omap_i2c.c >>>>>> index 26e3e5ebf6..690876e43e 100644 >>>>>> --- a/hw/i2c/omap_i2c.c >>>>>> +++ b/hw/i2c/omap_i2c.c >>>>>> @@ -17,6 +17,7 @@ >>>>>> * with this program; if not, see <http://www.gnu.org/licenses/>. >>>>>> */ >>>>>> #include "qemu/osdep.h" >>>>>> +#include "qemu/log.h" >>>>>> #include "hw/hw.h" >>>>>> #include "hw/i2c/i2c.h" >>>>>> #include "hw/arm/omap.h" >>>>>> @@ -339,14 +340,15 @@ static void omap_i2c_write(void *opaque, hwaddr addr, >>>>>> } >>>>>> break; >>>>>> } >>>>>> - if ((value & (1 << 15)) && !(value & (1 << 10))) { /* MST */ >>>>>> - fprintf(stderr, "%s: I^2C slave mode not supported\n", >>>>>> - __func__); >>>>>> + if ((value & (1 << 15)) && !(value & (1 << 10))) { /* MST */ >>>>> >>>>> Please keep the white spaces before the comment if you don't change >>>>> anything else. >>>> >>>> This is a <tab> and checkpatch complains... >>>> >>>> I can use 4 spaces for this tab. I tried to align with other tab-aligned >>>> comments I didn't modify, but the result is messier. Thus a simple space. >>> >>> Oh, sorry, I didn't notice that you've replaced a TAB here. I guess it's >>> ok then. But why does checkpatch complain if it is just in the context >>> of your modification? That's weird. >> >> The first 2 contexts (MST and XA) are fine, however checkpatch complains >> with the last one (ST_EN): >> >> ERROR: code indent should never use tabs >> #38: FILE: hw/i2c/omap_i2c.c:397: >> + if (value & (1 << 15)) {^I^I^I^I^I/* ST_EN */$ >> >> Since I replaced this one, I also did with the 2 previous. >> >> Now I realize I can _not_ add the brackets so I don't have to update the >> <tabs>: >> >> if (value & (1 << 15))^I^I^I^I^I/* ST_EN */ >> - fprintf(stderr, "%s: System Test not supported\n", __func__); >> + qemu_log_mask(LOG_UNIMP, >> + "%s: System Test not supported\n", __func__); >> break; >> >> I think if it better to unify the code style when possible, but it is up >> to you, if you prefer I can resend with tabs and no brackets. > > I think it's OK to fix up the coding style here, too. Maybe just mention > it in the patch description ("While we're at it, change TABs to spaces > and add missing curly braces to the surounding if-statements" or so). OK. If that's fine with you I won't respin the whole series for this comment, but if I have to respin for another reason I'll improve the comment. I kindly learned the lesson for the next time although. Thanks for the series review, Phil.
On 25.06.2018 17:07, Philippe Mathieu-Daudé wrote: > On 06/25/2018 11:30 AM, Thomas Huth wrote: >> On 25.06.2018 14:52, Philippe Mathieu-Daudé wrote: >>> On 06/25/2018 03:08 AM, Thomas Huth wrote: >>>> On 22.06.2018 22:10, Philippe Mathieu-Daudé wrote: >>>>> On 06/22/2018 04:38 PM, Thomas Huth wrote: >>>>>> On 22.06.2018 15:40, Philippe Mathieu-Daudé wrote: >>>>>>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> >>>>>>> --- >>>>>>> hw/i2c/omap_i2c.c | 20 ++++++++++++-------- >>>>>>> 1 file changed, 12 insertions(+), 8 deletions(-) >>>>>>> >>>>>>> diff --git a/hw/i2c/omap_i2c.c b/hw/i2c/omap_i2c.c >>>>>>> index 26e3e5ebf6..690876e43e 100644 >>>>>>> --- a/hw/i2c/omap_i2c.c >>>>>>> +++ b/hw/i2c/omap_i2c.c >>>>>>> @@ -17,6 +17,7 @@ >>>>>>> * with this program; if not, see <http://www.gnu.org/licenses/>. >>>>>>> */ >>>>>>> #include "qemu/osdep.h" >>>>>>> +#include "qemu/log.h" >>>>>>> #include "hw/hw.h" >>>>>>> #include "hw/i2c/i2c.h" >>>>>>> #include "hw/arm/omap.h" >>>>>>> @@ -339,14 +340,15 @@ static void omap_i2c_write(void *opaque, hwaddr addr, >>>>>>> } >>>>>>> break; >>>>>>> } >>>>>>> - if ((value & (1 << 15)) && !(value & (1 << 10))) { /* MST */ >>>>>>> - fprintf(stderr, "%s: I^2C slave mode not supported\n", >>>>>>> - __func__); >>>>>>> + if ((value & (1 << 15)) && !(value & (1 << 10))) { /* MST */ >>>>>> >>>>>> Please keep the white spaces before the comment if you don't change >>>>>> anything else. >>>>> >>>>> This is a <tab> and checkpatch complains... >>>>> >>>>> I can use 4 spaces for this tab. I tried to align with other tab-aligned >>>>> comments I didn't modify, but the result is messier. Thus a simple space. >>>> >>>> Oh, sorry, I didn't notice that you've replaced a TAB here. I guess it's >>>> ok then. But why does checkpatch complain if it is just in the context >>>> of your modification? That's weird. >>> >>> The first 2 contexts (MST and XA) are fine, however checkpatch complains >>> with the last one (ST_EN): >>> >>> ERROR: code indent should never use tabs >>> #38: FILE: hw/i2c/omap_i2c.c:397: >>> + if (value & (1 << 15)) {^I^I^I^I^I/* ST_EN */$ >>> >>> Since I replaced this one, I also did with the 2 previous. >>> >>> Now I realize I can _not_ add the brackets so I don't have to update the >>> <tabs>: >>> >>> if (value & (1 << 15))^I^I^I^I^I/* ST_EN */ >>> - fprintf(stderr, "%s: System Test not supported\n", __func__); >>> + qemu_log_mask(LOG_UNIMP, >>> + "%s: System Test not supported\n", __func__); >>> break; >>> >>> I think if it better to unify the code style when possible, but it is up >>> to you, if you prefer I can resend with tabs and no brackets. >> >> I think it's OK to fix up the coding style here, too. Maybe just mention >> it in the patch description ("While we're at it, change TABs to spaces >> and add missing curly braces to the surounding if-statements" or so). > > OK. If that's fine with you I won't respin the whole series for this > comment, but if I have to respin for another reason I'll improve the > comment. Fine for me! Thomas
diff --git a/hw/i2c/omap_i2c.c b/hw/i2c/omap_i2c.c index 26e3e5ebf6..690876e43e 100644 --- a/hw/i2c/omap_i2c.c +++ b/hw/i2c/omap_i2c.c @@ -17,6 +17,7 @@ * with this program; if not, see <http://www.gnu.org/licenses/>. */ #include "qemu/osdep.h" +#include "qemu/log.h" #include "hw/hw.h" #include "hw/i2c/i2c.h" #include "hw/arm/omap.h" @@ -339,14 +340,15 @@ static void omap_i2c_write(void *opaque, hwaddr addr, } break; } - if ((value & (1 << 15)) && !(value & (1 << 10))) { /* MST */ - fprintf(stderr, "%s: I^2C slave mode not supported\n", - __func__); + if ((value & (1 << 15)) && !(value & (1 << 10))) { /* MST */ + qemu_log_mask(LOG_UNIMP, "%s: I^2C slave mode not supported\n", + __func__); break; } - if ((value & (1 << 15)) && value & (1 << 8)) { /* XA */ - fprintf(stderr, "%s: 10-bit addressing mode not supported\n", - __func__); + if ((value & (1 << 15)) && value & (1 << 8)) { /* XA */ + qemu_log_mask(LOG_UNIMP, + "%s: 10-bit addressing mode not supported\n", + __func__); break; } if ((value & (1 << 15)) && value & (1 << 0)) { /* STT */ @@ -392,8 +394,10 @@ static void omap_i2c_write(void *opaque, hwaddr addr, s->stat |= 0x3f; omap_i2c_interrupts_update(s); } - if (value & (1 << 15)) /* ST_EN */ - fprintf(stderr, "%s: System Test not supported\n", __func__); + if (value & (1 << 15)) { /* ST_EN */ + qemu_log_mask(LOG_UNIMP, + "%s: System Test not supported\n", __func__); + } break; default:
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> --- hw/i2c/omap_i2c.c | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-)