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

[Andy Shevchenko <andy.shevchenko@xxxxxxxxx>]

On Thu, Sep 1, 2022 at 5:17 PM Russell King <rmk+kernel@xxxxxxxxxxxxxxx> wrote:
>
> From: Hector Martin <marcan@xxxxxxxxx>
>
> This driver implements the GPIO service on top of the SMC framework
> on Apple Mac machines. In particular, these are the GPIOs present in the
> PMU IC which are used to control power to certain on-board devices.
>
> Although the underlying hardware supports various pin config settings
> (input/output, open drain, etc.), this driver does not implement that
> functionality and leaves it up to the firmware to configure things
> properly. We also don't yet support interrupts/events. This is
> sufficient for device power control, which is the only thing we need to
> support at this point. More features will be implemented when needed.
>
> To our knowledge, only Apple Silicon Macs implement this SMC feature.

...

> +/*
> + * Commands 0-6 are, presumably, the intended API.

> + * Command 0xff lets you get/set the pin configuration in detail directly,
> + * but the bit meanings seem not to be stable between devices/PMU hardware
> + * versions.

Probably for debugging purposes...

> + *
> + * We're going to try to make do with the low commands for now.
> + * We don't implement pin mode changes at this time.
> + */

...

> +/*
> + * output modes seem to differ depending on the PMU in use... ?

Output

> + * j274 / M1 (Sera PMU):
> + *   0 = input
> + *   1 = output
> + *   2 = open drain
> + *   3 = disable
> + * j314 / M1Pro (Maverick PMU):
> + *   0 = input
> + *   1 = open drain
> + *   2 = output
> + *   3 = ?
> + */

...

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

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

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

...

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

...

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

> +       if (ret < 0)
> +               return ret;

...

> +       ret = apple_smc_write_u32(smcgp->smc, key, CMD_OUTPUT | value);
> +       if (ret < 0)
> +               dev_err(smcgp->dev, "GPIO set failed %p4ch = 0x%x\n", &key, value);

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.


...

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

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

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

> +                       continue;
> +               }
> +
> +               set_bit(gpio_nr, valid_mask);
> +       }
> +
> +       return 0;
> +}

...

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

--
With Best Regards,
Andy Shevchenko