On Mon, Oct 2, 2023 at 4:17 AM AKASHI Takahiro
<takahiro.akashi@xxxxxxxxxx> 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@xxxxxxxxxx>
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
