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

[Hector Martin <marcan@xxxxxxxxx>]

On 02/09/2022 03.55, Andy Shevchenko wrote:
>> +struct macsmc_gpio {
>> +       struct device *dev;
>> +       struct apple_smc *smc;
>> +       struct gpio_chip gc;
> 
> You might save some CPU cycles / code by shuffling members around.
> Usually we put gpio_chip as a first one, so pointer arithmetics to get
> it becomes a bit simpler, but it needs to be checked by the tool and
> also paying attention to what is used in critical paths (so
> performance-wise).

This is a GPIO chip accessed via a remote CPU. Saving two cycles on
pointer arithmetic is the very definition of premature optimization.

> 
>> +       int first_index;
>> +};
> 
> ...
> 
>> +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().

No. hex2bin() works on string buffers. This works on an integer
containing packed characters. They are not the same, and do not have the
same semantics endian-wise. Integer represent numbers abstractly, byte
buffers represent bytes in memory in sequence.

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

Same comment as above.

>> +       /* 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?

Have you tried reading the comment above the code?

When I write code doing something unintuitive and put a comment on top,
I expect reviewers to *read* it. If you're not going to do that, I might
as well stop writing comments.

> Strange specifier... It seems like a hashed pointer with added (or
> skipped? I don't remember) '4ch'. Perhaps you meant one of '%pE',
> '%p4cc'?
> Ditto for other cases.

As mentioned in the other thread, there was a missed dependency that
added this specifier.

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

It is one line away from the first user.

> 
>> +       int i;
>> +
>> +       if (count > MAX_GPIO)
>> +               count = MAX_GPIO;
> 
> Hmm... When can it be the case?

Let's find out! Two lines above:

+	int count = apple_smc_get_key_count(smcgp->smc) - smcgp->first_index;

So I get the toal SMC key count, which is probably 1000 or so, then
subtract the index of the first GPIO key, to get an upper bound on the
last GPIO key just to make sure we don't run off the end of the key list.

In other words, pretty much always.

But you didn't read two lines prior, did you.

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

This is zero lines away from the first user.

> 
>> +
>> +               if (ret < 0)
>> +                       return ret;
>> +
>> +               if (key > SMC_KEY(gPff))
>> +                       break;
>> +
>> +               gpio_nr = macsmc_gpio_nr(key);
>> +               if (gpio_nr < 0 || gpio_nr > MAX_GPIO) {
>> +                       dev_err(smcgp->dev, "Bad GPIO key %p4ch\n", &key);
> 
> Yeah, according to the code it will print something you didn't want.

What?

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

This is a producer, not a consumer. It needs to set the of_node so there
is something for consumers to target in the device tree. The consumers
may well use fwnode APIs.

- Hector