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

[Andy Shevchenko <andy.shevchenko@gmail.com>]

On Fri, Sep 2, 2022 at 1:05 PM Russell King (Oracle)
<linux@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().

...

> > > +static int macsmc_gpio_key(unsigned int offset)
> > > +{
> > > +       return _SMC_KEY("gP\0\0") | (hex_asc_hi(offset) << 8) | hex_asc_lo(offset);
> > > +}
> >
> > NIH hex_byte_pack().
>
> This would become:
>
>         char buf[2];
>
>         hex_byte_pack(buf, offset);
>
>         return _SMC_KEY("gP\0\0") | buf[0] << 8 | buf[1];

You have a point here.

> to avoid the endian issues. It just seems to be a more complex way to
> do the conversion. One could then argue that this is just a NIH
> sprintf(), so it could then be written:

No, no. snprintf() is too much here.

> which looks nicer, but involves a lot more code.
>
> Since this is called for every GPIO operation, and you were worred above
> about the layout of the macsmc_gpio structure (which is a micro-
> optimisation), it seems weird to be concerned about the efficiency of
> the structure layout and then suggest less efficient code in each of the
> functional paths of the driver. There seems to be a contradiction.

...

> > > +       /* First try reading the explicit pin mode register */
> > > +       ret = apple_smc_rw_u32(smcgp->smc, key, CMD_PINMODE, &val);
> > > +       if (!ret)
> > > +               return (val & MODE_OUTPUT) ? GPIO_LINE_DIRECTION_OUT : GPIO_LINE_DIRECTION_IN;
> > > +
> > > +       /*
> > > +        * Less common IRQ configs cause CMD_PINMODE to fail, and so does open drain mode.
> > > +        * Fall back to reading IRQ mode, which will only succeed for inputs.
> > > +        */
> > > +       ret = apple_smc_rw_u32(smcgp->smc, key, CMD_IRQ_MODE, &val);
> > > +       return (!ret) ? GPIO_LINE_DIRECTION_IN : GPIO_LINE_DIRECTION_OUT;
> >
> > What is the meaning of val in this case?
>
> Reading the comment, it seems that "val" is irrelevant. I'm not sure that
> needs explaining given there's a comment that's already explaining what
> is going on here.

OK.
Just convert then (!ret) --> ret.

...

> > > +       if (ret == GPIO_LINE_DIRECTION_OUT)
> > > +               ret = apple_smc_rw_u32(smcgp->smc, key, CMD_OUTPUT, &val);
> > > +       else
> > > +               ret = apple_smc_rw_u32(smcgp->smc, key, CMD_INPUT, &val);
> >
> > > +
> >
> > Unnecessary blank line.
>
> I think that's personal style preference, it isn't mentioned in the coding
> style. However, the following is much nicer and likely produces better
> code:
>
>         if (ret == GPIO_LINE_DIRECTION_OUT)
>                 cmd = CMD_OUTPUT;
>         else
>                 cmd = CMD_INPUT;
>
>         ret = apple_smc_rw_u32(smcgp->smc, key, cmd, &val);
>         if (ret < 0)
>                 return ret;

Go for it!

...

> > > +       if (count > MAX_GPIO)
> > > +               count = MAX_GPIO;
> >
> > Hmm... When can it be the case?
>
> That's a question for the Asahi folk - it's not obvious whether it could
> or could not be from the code. I think it depends on firmware.

If it's the case, why does the code not support higher GPIOs? Needs at
least a comment.

...

> > > +       bitmap_zero(valid_mask, ngpios);
> > > +
> > > +       for (i = 0; i < count; i++) {
> > > +               smc_key key;
> > > +               int gpio_nr;
> >
> > > +               int ret = apple_smc_get_key_by_index(smcgp->smc, smcgp->first_index + i, &key);
> >
> > Ditto.
>
> What does "ditto" here mean, because I don't think you mean "Hmm...
> When can it be the case?" and "I would split this assignment and move
> it closer to the first user." doesn't seem to be relevant either.

Split

  int ret = foo();

to

    int ret;
    ret = foo();

...

> > > +       pdev->dev.of_node = of_get_child_by_name(pdev->dev.parent->of_node, "gpio");
> >
> > Can we use fwnode APIs instead?
> > Or do you really need this?
>
> Ouch, that's not nice. I can change this to:

(Some background on why my eye caught this. We as GPIO SIG in the
kernel want to move the library to be fwnode one without looking into
the underneath property provider. This kind of lines makes driver look
a bit ugly from that perspective)

>         fwnode = device_get_named_child_node(pdev->dev.parent, "gpio");
>         device_set_node(&pdev->dev, fwnode);
>
> but even that isn't _that_ nice. I'd like to hear comments from the Asahi
> folk about whether these sub-blocks of the SMC can have compatibles, so
> that the MFD layer can automatically fill in the firmware nodes on the
> struct device before the probe function gets called.

> If not, then I think it would be reasonable to have a discussion with
> Lee about extending MFD to be able to have mfd cells name a child, so
> that MFD can do the above instead of having it littered amongst drivers.

MFD cells can be matched by compatible strings.

-- 
With Best Regards,
Andy Shevchenko