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:33 PM Russell King (Oracle)
<linux@xxxxxxxxxxxxxxx> wrote:
> On Fri, Sep 02, 2022 at 01:37:14PM +0300, Andy Shevchenko wrote:
> > On Fri, Sep 2, 2022 at 1:05 PM Russell King (Oracle)
> > <linux@xxxxxxxxxxxxxxx> wrote:
> > > On Thu, Sep 01, 2022 at 09:55:23PM +0300, Andy Shevchenko wrote:
> > > > > +static int macsmc_gpio_nr(smc_key key)
> > > > > +{
> > > > > +       int low = hex_to_bin(key & 0xff);
> > > > > +       int high = hex_to_bin((key >> 8) & 0xff);
> > > > > +
> > > > > +       if (low < 0 || high < 0)
> > > > > +               return -1;
> > > > > +
> > > > > +       return low | (high << 4);
> > > > > +}
> > > >
> > > > NIH hex2bin().
> > >
> > > Is using hex2bin really better?
> >
> > Yes.
> >
> > > static int macsmc_gpio_nr(smc_key key)
> > > {
> > >         char k[2];
> > >         u8 result;
> > >         int ret;
> > >
> > >         k[0] = key;
> > >         k[1] = key >> 8;
> > >
> > >         ret = hex2bin(&result, k, 2);
> > >         if (ret < 0)
> > >                 return ret;
> > >
> > >         return result;
> > > }
> > >
> > > This looks to me like it consumes more CPU cycles - because we have to
> > > write each "character" to the stack, then call a function, only to then
> > > call the hex_to_bin() function. One can't just pass "key" into hex2bin
> > > because that will bring with it endian issues.
> >
> > With one detail missed, why do you need all that if you can use
> > byteorder helpers()? What's the stack? Just replace this entire
> > function with the respectful calls to hex2bin().
>
> Sorry, I don't understand what you're suggesting, because it doesn't
> make sense to me. The byteorder helpers do not give a char array, which
> is what hex2bin() wants, so we end up with something like:
>
>         __le16 foo = cpu_to_le16(key);
>         u8 result;
>
>         ret = hex2bin(&result, (char *)&foo, 1);
>         if (ret < 0)
>                 return ret;
>
>         return result;
>
> This to me looks like yucky code, It still results in "foo" having to
> be on the stack, because the out-of-line hex2bin() requires a pointer
> to be passed as the second argument.
>
> Maybe you could provide an example of what you're thinking of, because
> I'm at a loss to understand what you're thinking this should look like.

So, let's look into the real callers to see, oh wait, it's a single caller!
Why can't you simply do

         ret = hex2bin(&result, (char *)&cpu_to_le16(key), 1);
         if (ret < 0)
                 return ret;

in-place there?

-- 
With Best Regards,
Andy Shevchenko