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

[Martin Povišer <povik@xxxxxxxxxxx>]

> On 2. 9. 2022, at 15:33, Andy Shevchenko <andy.shevchenko@xxxxxxxxx> wrote:
> 
> 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));
> 
> ?

That would compile? I thought that’s not valid C, taking an
address of function’s return value.

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

Absolutely agreed on the latter point.

Martin