U-Boot Archive on lore.kernel.org
 help / color / mirror / Atom feed
From: Christoph Niedermaier <cniedermaier@dh-electronics.com>
To: Tom Rini <trini@konsulko.com>
Cc: Quentin Schulz <quentin.schulz@cherry.de>,
	"u-boot@lists.denx.de" <u-boot@lists.denx.de>,
	Benedikt Spranger <b.spranger@linutronix.de>,
	"Simon Glass" <sjg@chromium.org>,
	John Ogness <john.ogness@linutronix.de>,
	"Jerome Forissier" <jerome.forissier@linaro.org>,
	Ilias Apalodimas <ilias.apalodimas@linaro.org>,
	Marek Vasut <marex@denx.de>
Subject: RE: [PATCH] tiny-printf: Add support for upper case hex values
Date: Thu, 20 Mar 2025 18:41:33 +0000	[thread overview]
Message-ID: <6a3d121dec474f4e83bfdc8ee00d8fd8@dh-electronics.com> (raw)
In-Reply-To: <20250320182829.GS2640854@bill-the-cat>

From: Tom Rini <trini@konsulko.com>
Sent: Thursday, March 20, 2025 7:28 PM
> On Thu, Mar 20, 2025 at 05:58:56PM +0000, Christoph Niedermaier wrote:
> > From: Tom Rini <trini@konsulko.com>
> > Sent: Thursday, March 20, 2025 3:19 PM
> > > On Thu, Mar 20, 2025 at 03:14:03PM +0100, Marek Vasut wrote:
> > > > On 3/20/25 3:00 PM, Quentin Schulz wrote:
> > > > > Hi Marek,
> > > > >
> > > > > On 3/20/25 12:49 PM, Marek Vasut wrote:
> > > > > > On 3/20/25 11:23 AM, Christoph Niedermaier wrote:
> > > > > > > If tiny printf is used with 0x%08X (upper case X) the output is
> > > > > > > always 0x00000000. It could be confusing if upper case instead
> > > > > > > of lower case is used intentionally or accidentally because the
> > > > > > > actual value is not output. To avoid this confusion, tiny printf
> > > > > > > is extended to support also the formatting with %X.
> > > > > > >
> > > > > > > Signed-off-by: Christoph Niedermaier <cniedermaier@dh-electronics.com>
> > > > > > TINY_PRINTF is meant to be tiny, i.e. not consume a lot of space, at
> > > > > > the expense of functionality. This is meant to be used in size
> > > > > > constrained environments, like the SPL. If you need full vsprintf()
> > > > > > formatting support, disable TINY_PRINTF in your config and use the
> > > > > > regular vsprintf() implementation.
> > > > >
> > > > > The issue is that disabling TINY_PRINTF may not be possible (size
> > > > > constraints) and some code is compiled for different stages and people
> > > > > typically don't check whether the format used in printf is valid with
> > > > > tiny_printf. I've had this issue already in the past, I vaguely recall
> > > > > "complaining" about it on IRC.
> > > > >
> > > > > Maybe there's something we can do to verify that the code is working how
> > > > > we expect it to work, regardless of tiny_printf/full printf selection?
> > > > > checkpatch or a compile-time check for the formats maybe?
> > > > >
> > > > > But yeah, essentially the whole thing is... if we continue like this,
> > > > > we'll just end up getting closer and closer to the full printf which is
> > > > > not something we want :)
> > > > Shall we maybe patch tiny printf to print '?' on unsupported formatting
> > > > characters, or outright complain that users should fix their code ?
> > >
> > > This sounds good to me, adding ? in the output.
> > >
> > > > For the %x/%X thing, we could technically fall back from %X to %x , which
> > > > would do the printing with minimum footprint increase, albeit slightly
> > > > malformed:
> > >
> > > There's 109 hits on "%X" and another 489 on "%0.X", so I think it's
> > > reasonable to do either of:
> > > - Misprint A-F as a-f (in other words, treat it like 'x'
> > > - Audit the callers and change them to 'x' from 'X'. We normally don't
> > >   capitalize the output and there's all sorts of patches over the years
> > >   changing them to lowercase in other places.
> > >
> > > We have done both for other format specifiers and tiny-printf before in
> > > the past.
> >
> > If we taking about adding feature, I think that the patch don't really
> > add a new feature, it's just a variant of %x. I reuse part of the %x code.
> > So if size matters here the size of the object file (not stripped):
> >
> > Before:
> > -rw-r--r-- 1 developer developer 19340 Mar 20 15:32 tiny-printf.o
> >
> > After with current patch:
> > -rw-r--r-- 1 developer developer 21212 Mar 20 15:38 tiny-printf.o
> > => Diff: 1872 Bytes (+9,67%)
> >
> > I have another patch, where I don't introduce two new function and
> > don't use an enum. Then it looks like this:
> > -rw-r--r-- 1 developer developer 19888 Mar 20 16:53 tiny-printf.o
> > => Diff: 548 Bytes (+2,83%)
> >
> > Would this increase in size be OK for %X?
> > So there will be no misprint.
> >
> > Otherwise, a misprint for %X would be fine with me, because I still
> > get the correct value.
> 
> Tiny really does mean tiny in this case so yes, I would prefer the
> single digit byte increase of adding 'X' to the 'x' case. Thanks!

OK, I will make a V2 with this changes.


Regards
Christoph

  reply	other threads:[~2025-03-20 18:42 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2025-03-20 10:23 [PATCH] tiny-printf: Add support for upper case hex values Christoph Niedermaier
2025-03-20 11:49 ` Marek Vasut
2025-03-20 13:53   ` Tom Rini
2025-03-20 14:00   ` Quentin Schulz
2025-03-20 14:14     ` Marek Vasut
2025-03-20 14:18       ` Tom Rini
2025-03-20 17:58         ` Christoph Niedermaier
2025-03-20 18:28           ` Tom Rini
2025-03-20 18:41             ` Christoph Niedermaier [this message]
2025-04-01  8:55     ` Michael Walle
2025-04-01 14:08       ` Christoph Niedermaier
2025-04-02  7:01         ` Michael Walle
2025-04-02  9:03           ` Christoph Niedermaier
2025-04-04  8:33             ` Michael Walle
2025-04-04 14:58               ` Tom Rini
2025-04-07  5:27                 ` Michael Walle
2025-04-07  6:55                   ` Christoph Niedermaier

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=6a3d121dec474f4e83bfdc8ee00d8fd8@dh-electronics.com \
    --to=cniedermaier@dh-electronics.com \
    --cc=b.spranger@linutronix.de \
    --cc=ilias.apalodimas@linaro.org \
    --cc=jerome.forissier@linaro.org \
    --cc=john.ogness@linutronix.de \
    --cc=marex@denx.de \
    --cc=quentin.schulz@cherry.de \
    --cc=sjg@chromium.org \
    --cc=trini@konsulko.com \
    --cc=u-boot@lists.denx.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox