Re: [jic23-iio:testing 124/129] drivers/iio/accel/msa311.c:993:24: warning: format specifies type 'unsigned char' but the argument has type 'unsigned int'

[Dmitry Rokosov]

On Wed, Aug 31, 2022 at 09:38:10AM +0100, Jonathan Cameron wrote:
> On Wed, 31 Aug 2022 03:24:05 +0300
> Dmitry Rokosov <ddrokosov@xxxxxxxxxxxxxx> wrote:
> 
> > Hello Jonathan and Andy,
> > 
> > Sorry for such a late response, a couple of days ago my daughter was born.
> > So I couldn't reach my laptop :)
> 
> Congratulations and good luck! :)

Thank you! :)

> > > > > >> drivers/iio/accel/msa311.c:993:24: warning: format specifies type 'unsigned char' but the argument has type 'unsigned int' [-Wformat]  
> > > > >                                               "msa311-%hhx", partid);
> > > > >                                                       ~~~~   ^~~~~~
> > > > >                                                       %x
> > > > >    1 warning generated.  
> > >   
> > > > >    992                msa311->chip_name = devm_kasprintf(dev, GFP_KERNEL,  
> > > > >  > 993                                                   "msa311-%hhx", partid);  
> > >   
> > > > I'm thinking intent here was to limit range of what was printed. Maybe better to use
> > > > local u8 variable or cast?
> > > >
> > > > I can fix it up if that's fine with you - or even better send me a patch that fixes
> > > > it however you prefer!  
> > > 
> > > Looking back at what Linus said about those specifiers, I would rather
> > > go with simple %x or %02x.
> > > 
> > > P.S. Surprisingly many C developers don't know the difference between
> > > %hhx and %02x, which is easy to check by
> > > 
> > >   char a = -1;
> > >   printf("%hhx <==> %02x\n", a, a);
> > >   a = 217;
> > >   printf("%hhx <==> %02x\n", a, a);  
> > 
> > Thank you for pointing to Linus answer. I have explored it at the link:
> > 
> > https://lore.kernel.org/lkml/CAHk-=wgoxnmsj8GEVFJSvTwdnWm8wVJthefNk2n6+4TC=20e0Q@xxxxxxxxxxxxxx/
> > 
> > Actually, Linus described one exception to this rule, which I have
> > in my patch. I have an integer which I want to print as a char.
> > I see that Linus mentions it's a bad idea. I agree with that. But
> > currently %hhx => %02x replacement breaks the requested behavior, %02x
> > will not shrink integer value to char. I want to say, maybe it's better
> > just cast the value to u8 type and print as %x. What do you think? I can
> > prepare such a patch.
> > 
> > P.S. Andy's example to show the difference between %hhx and %02x makes
> > more clear why such a replacement is not acceptable here.
> > 
> > Output:
> > ff <==> ffffffff
> > d9 <==> ffffffd9
> > 
> In this case the storage is an unsigned int, not an unsigned char.
> Hence the value will be small and positive.  So I'm fairly sure you
> won't hit the above because it's
> 
> 0x000000ff --> ff
> 0x000000d9 --> d9
> 
> The range is limited to 8 bits because that's all the underlying register
> holds.

>From "data" format point of view you are right. We have regmap over I2C
and register values will be limited to 8 bits only. But in general
unsigned int value bigger than 0xff formatted by %02x will not be
limited by two positions only. In other words, we can use a simple %x
with the same success.
I want to say if our goal is shrinking the unsigned int value to first
byte in hex format w/o %hhx using, we need to cast unsigned int value to
unsigned char and printout it using simple %x or %02x.

For example, in my opinion, in the below code snippet, only first and
third printout formatting are correct. Currently, we are using the
second in the merged patchset.

>>>
    unsigned int a = 0xDEADBEEF;
    printf("%hhx <==> %02x (uint8_t:%02x)\n", a, a, (unsigned char)a);
<<<
Output:
ef <==> deadbeef (uint8_t:ef)
===

Anyway, regmap over I2C abstraction limits our value to the 8-bit range,
so functionally %02x is working well here.

-- 
Thank you,
Dmitry