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

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

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?

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.

> > +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];

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:

	char buf[5];

	snprintf(buf, sizeof(buf), "gP%02x", offset);

	return _SMC_KEY(buf);

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.

> > +       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;

> > +       struct macsmc_gpio *smcgp = gpiochip_get_data(gc);
> > +       int count = apple_smc_get_key_count(smcgp->smc) - smcgp->first_index;
> 
> I would split this assignment and move it closer to the first user.
> 
> > +       int i;
> > +
> > +       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.

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

> > +       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:

        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.

Thanks for the review.

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