Re: [PATCH 5/6] gpio: Add new gpio-macsmc driver for Apple Macs

[Andy Shevchenko <andy.shevchenko@xxxxxxxxx>]

On Fri, Sep 2, 2022 at 2:12 PM Martin Povišer <povik@xxxxxxxxxxx> wrote:
> > On 2. 9. 2022, at 12:23, Andy Shevchenko <andy.shevchenko@xxxxxxxxx> wrote:
> > On Fri, Sep 2, 2022 at 12:47 PM Martin Povišer <povik@xxxxxxxxxxx> wrote:
> >>> On 2. 9. 2022, at 8:31, Andy Shevchenko <andy.shevchenko@xxxxxxxxx> wrote:
> >>> On Fri, Sep 2, 2022 at 12:52 AM Martin Povišer <povik@xxxxxxxxxxx> wrote:

...

> >>> I don't see why we need that. The %.4s (0x%08x) is repeating that with
> >>> the existing codebase. (I do understand why v4l2/drm have it). Ideally
> >>> the first should use %4pE, but it might not be suitable in some cases.
> >>
> >> Just from a superficial understanding of things: %p4ch on little-endian
> >> will print in a reversed order to %.4s. As I see it the handling of
> >> endianness is the value proposition of the new specifiers.
> >
> > So, what prevents you from adding this to %pE?
> > The preferred way is not adding a specifier for a single user with a
> > particular case, esp. when it's covered by the existing ones.
>
> Adding the endianness conversion into %pE as, ehm, an ‘escaping flag’?
> If you think that would be accepted...
>
> I guess this was added on the assumption that keys like this will
> be a common occurrence in interaction with Apple firmware. Though
> greping the ‘asahi’ staging tree for ‘%p4ch’ I only see it in the
> SMC code (9 times):
>
> ./drivers/power/reset/macsmc-reboot.c
> ./drivers/platform/apple/smc_core.c
> ./drivers/gpio/gpio-macsmc.c

> >> %p4ch - interpret as an u32, print the character in most significant byte first
> >
> > %.4s + be32_to_cpu()
>
> Well, AIUI instead of
>
>   printk(“%p4ch = ...\n”, &key);
>
> you need to do
>
>   u32 key_be = cpu_to_be32(key);
>   printk(“%.4s = ...\n”, &key_be);
>
> in at least 9 places now, the number of which will probably grow.
> Just to make the case for *some* printk helper.

Wouldn't this be one line

  printk(“%.4s = ...\n”, &cpu_to_be32(key));

?

So I don't see disadvantages right now. Later on we can consider a new
specifier _separately_, otherwise this series would be dragging for no
sense.

-- 
With Best Regards,
Andy Shevchenko