Re: [Qemu-trivial] [PATCH v2 07/14] hw/i2c/omap_i2c: Use qemu_log_mask(UNIMP) instead of fprintf

[Thomas Huth]

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é <address@hidden>
>>>>> ---
>>>>>  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