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é <f4...@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.
