Re: [RFC 3/4] gpio: scmi: add SCMI pinctrl based gpio driver

Linus Walleij
2023-10-03T21:35:50Z

On Mon, Oct 2, 2023 at 4:17 AM AKASHI Takahiro
<takahiro.akashi@linaro.org> wrote:

> SCMI pin control protocol supports not only pin controllers, but also
> gpio controllers by design. This patch includes a generic gpio driver
> which allows consumer drivers to access gpio pins that are handled
> through SCMI interfaces.
>
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>

I would write a bit that this is intended for SCMI but it actually
is a GPIO front-end to any pin controller that supports the
necessary pin config operations.

> drivers/gpio/gpio-scmi.c | 154 +++++++++++++++++++++++++++++++++++++++

So I would name it gpio-by-pinctrl.c
(clear and hard to misunderstand)

> +config GPIO_SCMI

GPIO_BY_PINCTRL

> + tristate "GPIO support based on SCMI pinctrl"

"GPIO support based on a pure pin control back-end"

> + depends on OF_GPIO

Skip this, let's use device properties instead. They will anyways just translate
to OF properties in the OF case.

> + depends on PINCTRL_SCMI
> + help
> + Select this option to support GPIO devices based on SCMI pin
> + control protocol.

"GPIO devices based solely on pin control, specifically pin configuration, such
as SCMI."

> +#include <linux/of.h>

Use #include <linux/property.h> so we remove reliance on OF.

> +#include "gpiolib.h"

Why?

> +static int scmi_gpio_get_direction(struct gpio_chip *chip, unsigned int offset)

Rename all functions pinctrl_gpio_*

> +{
> + unsigned long config;
> +
> + config = PIN_CONFIG_OUTPUT_ENABLE;
> + if (pinctrl_gpio_get_config(chip->gpiodev->base + offset, &config))
> + return -1;

Probably you want to return the error code from pinctrl_gpio_get_config()
rather than -1? (same below).

> + if (config)
> + return GPIO_LINE_DIRECTION_OUT;
> +
> + config = PIN_CONFIG_INPUT_ENABLE;
> + if (pinctrl_gpio_get_config(chip->gpiodev->base + offset, &config))
> + return -1;
> + if (config)
> + return GPIO_LINE_DIRECTION_IN;

I would actually not return after checking PIN_CONFIG_OUTPUT_ENABLE.
I would call *both* something like:

int ret;
bool out_en, in_en;

config = PIN_CONFIG_OUTPUT_ENABLE;
ret = pinctrl_gpio_get_config(chip->gpiodev->base + offset, &config);
if (ret)
return ret;
/* Maybe check for "not implemented" error code here and let that pass
* setting out_en = false; not sure. Maybe we should mandate support
* for this.
*/
out_en = !!config;
config = PIN_CONFIG_INPUT_ENABLE;
ret = pinctrl_gpio_get_config(chip->gpiodev->base + offset, &config);
if (ret)
return ret;
in_en = !!config;

/* Consistency check - in theory both can be enabled! */
if (in_en && !out_en)
return GPIO_LINE_DIRECTION_IN;
if (!in_en && out_en)
return GPIO_LINE_DIRECTION_OUT;
if (in_en && out_en) {
/*
* This is e.g. open drain emulation!
* In this case check @PIN_CONFIG_DRIVE_OPEN_DRAIN
* if this is enabled, return GPIO_LINE_DIRECTION_OUT,
* else return an error. (I think.)
*/
}

/* We get here for (!in_en && !out_en) */
return -EINVAL;

> +static int scmi_gpio_get(struct gpio_chip *chip, unsigned int offset)
> +{
> + unsigned long config;
> +
> + /* FIXME: currently, PIN_CONFIG_INPUT not defined */
> + config = PIN_CONFIG_INPUT;
> + if (pinctrl_gpio_get_config(chip->gpiodev->base + offset, &config))
> + return -1;
> +
> + /* FIXME: the packed format not defined */
> + if (config >> 8)
> + return 1;
> +
> + return 0;
> +}

Proper error code instead of -1 otherwise looks good!

> +static void scmi_gpio_set(struct gpio_chip *chip, unsigned int offset, int val)

static int?

> +{
> + unsigned long config;
> +
> + config = PIN_CONF_PACKED(PIN_CONFIG_OUTPUT, val & 0x1);

No need to add & 0x01, the gpiolib core already does this.

> + pinctrl_gpio_set_config(chip->gpiodev->base + offset, config);

return pinctrl_gpio_set_config(); so error is propagated.

> +static u16 sum_up_ngpios(struct gpio_chip *chip)
> +{
> + struct gpio_pin_range *range;
> + struct gpio_device *gdev = chip->gpiodev;
> + u16 ngpios = 0;
> +
> + list_for_each_entry(range, &gdev->pin_ranges, node) {
> + ngpios += range->range.npins;
> + }

This works but isn't really the intended use case of the ranges.
Feel a bit uncertain about it, but I can't think of anything better.
And I guess these come directly out of SCMI so it's first hand
information about all GPIOs.

> +static int scmi_gpio_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct device_node *parent_np;

Skip (not used)

> + /* FIXME: who should be the parent */
> + parent_np = NULL;

Skip (not used)

> + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> + if (!priv)
> + return -ENOMEM;
> +
> + chip = &priv->chip;
> + chip->label = dev_name(dev);
> + chip->parent = dev;

This is the actual parent, which is good enough?

> + chip->base = -1;
> +
> + chip->request = gpiochip_generic_request;
> + chip->free = gpiochip_generic_free;
> + chip->get_direction = scmi_gpio_get_direction;
> + chip->direction_input = scmi_gpio_direction_input;
> + chip->direction_output = scmi_gpio_direction_output;

Add:
chip->set_config = gpiochip_generic_config;

which in turn becomes just pinctrl_gpio_set_config(), which
is what we want.

The second cell in two-cell GPIOs already supports passing
GPIO_PUSH_PULL, GPIO_OPEN_DRAIN, GPIO_OPEN_SOURCE,
GPIO_PULL_UP, GPIO_PULL_DOWN, GPIO_PULL_DISABLE,
which you can this way trivially pass down to the pin control driver.

NB: make sure the scmi pin control driver returns error for
unknown configs.

> +static int scmi_gpio_remove(struct platform_device *pdev)
> +{
> + struct scmi_gpio_priv *priv = platform_get_drvdata(pdev);
> +
> + gpiochip_remove(&priv->chip);

You are using devm_* to add it so this is not needed!

Just drop the remove function.

> +static const struct of_device_id scmi_gpio_match[] = {
> + { .compatible = "arm,scmi-gpio-generic" },

"pin-control-gpio" is my suggestion for this!

I hope this helps.

Yours,
Linus Walleij