Messages in this thread | | | From | Linus Walleij <> | Date | Tue, 3 Oct 2023 23:35:31 +0200 | Subject | Re: [RFC 3/4] gpio: scmi: add SCMI pinctrl based gpio driver |
| |
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
| |