[PATCH 5/6] gpio: Add new gpio-macsmc driver for Apple Macs
Andy Shevchenko
andy.shevchenko at gmail.com
Fri Sep 2 08:43:36 PDT 2022
On Fri, Sep 2, 2022 at 6:34 PM Russell King (Oracle)
<linux at armlinux.org.uk> wrote:
> On Fri, Sep 02, 2022 at 05:53:25PM +0300, Andy Shevchenko wrote:
> > On Fri, Sep 2, 2022 at 5:46 PM Russell King (Oracle)
> > <linux at armlinux.org.uk> wrote:
> > > On Fri, Sep 02, 2022 at 04:39:16PM +0300, Andy Shevchenko wrote:
> > > > On Fri, Sep 2, 2022 at 2:33 PM Russell King (Oracle)
> > > > <linux at armlinux.org.uk> 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 at armlinux.org.uk> 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?
> > >
> > > This is not legal C.
> >
> > I acknowledged this, sorry.
> >
> > > Please can we back up this discussion, and start
> > > over with legal C suggestions. Thanks.
> >
> > Suggestion was given as well, let's create a helper used by apple
> > stuff and later on we will consider the separate submission for the
> > (new) specifier. Would it work for you?
>
> This sub-thread isn't about the %p4ch specifier. It's about a
> reasonable implementation of macsmc_gpio_nr().
>
> Extracting from the context above, the original code was:
>
> 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);
> }
>
> I suggested:
>
> 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;
> }
>
> You didn't like that, so I then suggested:
>
> static int macsmc_gpio_nr(smc_key key)
> {
> __le16 foo = cpu_to_le16(key);
> u8 result;
> int ret;
>
> ret = hex2bin(&result, (char *)&foo, 1);
> if (ret < 0)
> return ret;
>
> return result;
> }
>
> which you also didn't like,
...based on the wrong suggestion below. That said, the above is fine to me.
> and then you suggested something that isn't
> legal C. So, I then asked you to backup this discussion...
>
> As I've made a number of suggestions, and you've essentially rejected
> them all, I still need to know what you would find acceptable for this,
> because I'm out of ideas.
--
With Best Regards,
Andy Shevchenko
More information about the linux-arm-kernel
mailing list