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

[Martin Povišer <povik@xxxxxxxxxxx>]

Pardon, I lost the CC list in my earlier reply. Adding it back now.

> 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:
>>>>> On 1. 9. 2022, at 20:55, Andy Shevchenko <andy.shevchenko@xxxxxxxxx> wrote:
>>>>> On Thu, Sep 1, 2022 at 5:17 PM Russell King <rmk+kernel@xxxxxxxxxxxxxxx> wrote:
>>>> 
>>>>>> + ret = apple_smc_write_u32(smcgp->smc, key, CMD_OUTPUT | value);
>>>>>> + if (ret < 0)
>>>>>> + dev_err(smcgp->dev, "GPIO set failed %p4ch = 0x%x\n", &key, value);
>>>>> 
>>>>> Strange specifier... It seems like a hashed pointer with added (or
>>>>> skipped? I don't remember) '4ch'. Perhaps you meant one of '%pE',
>>>>> '%p4cc'?
>>>>> Ditto for other cases.
>>>> 
>>>> As was pointed out by Sven elsewhere in the thread, this is an
>>>> unupstreamed specifier (that was missed as a dependency of this
>>>> code).
>>>> 
>>>> https://github.com/AsahiLinux/linux/blob/f8c0d18173a7b649999ee27515393f7aae40310c/Documentation/core-api/printk-formats.rst#generic-fourcc-code
>>> 
>>> 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

>> So
>> 
>> %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.

> 
>> %p4cb - the same as %.4s
> 
>> %p4cl - reversed to %.4s
> 
> %.4s + swab32()

Sure, these two are uninteresting, probably added for completeness.

> 
> So?

Well, so nothing. I am primarily explaining how that strange specifier
came to be.

Martin

> -- 
> With Best Regards,
> Andy Shevchenko