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

["Russell King (Oracle)" <linux@xxxxxxxxxxxxxxx>]

On Fri, Sep 02, 2022 at 03:37:27PM +0200, Martin Povišer wrote:
> 
> > 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.

It isn't legal C.

int foo(int bar);

int blah(int *v);

int test(int v)
{
        return blah(&foo(v));
}

t.c: In function ‘test’:
t.c:7:14: error: lvalue required as unary ‘&’ operand

And just to make sure that it's not just my test that is wrong, and
there's something magical about cpu_to_be32()...

In file included from include/linux/device.h:15,
                 from drivers/gpio/gpio-macsmc.c:11:
drivers/gpio/gpio-macsmc.c: In function 'macsmc_gpio_probe':
drivers/gpio/gpio-macsmc.c:356:49: error: lvalue required as unary '&' operand
  356 |  dev_info(smcgp->dev, "First GPIO key: %.4s\n", &cpu_to_be32(key));
      |                                                 ^
include/linux/dev_printk.h:110:23: note: in definition of macro 'dev_printk_index_wrap'
  110 |   _p_func(dev, fmt, ##__VA_ARGS__);   \
      |                       ^~~~~~~~~~~
drivers/gpio/gpio-macsmc.c:356:2: note: in expansion of macro 'dev_info'
  356 |  dev_info(smcgp->dev, "First GPIO key: %.4s\n", &cpu_to_be32(key));
      |  ^~~~~~~~
make[3]: *** [scripts/Makefile.build:249: drivers/gpio/gpio-macsmc.o] Error 1
make[2]: *** [scripts/Makefile.build:466: drivers/gpio] Error 2
make[1]: *** [Makefile:1843: drivers] Error 2
make[1]: *** Waiting for unfinished jobs....
make: *** [Makefile:219: __sub-make] Error 2

So, sorry Andy, but this suggestion does not appear to be legal C.

This also applies to your suggestion in the other sub-thread of:

         ret = hex2bin(&result, (char *)&cpu_to_le16(key), 1);

As we've now discovered that this is not legal C, can we back up *both*
discussions and start again on these points.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!