Message-ID: <CACRpkdZojfC2qr7gfzL9fj=DEYJcuPR=a1+zVWTMysK9BH_m_Q@mail.gmail.com>
Date:   Tue, 3 Oct 2023 23:35:31 +0200
From:   Linus Walleij <linus.walleij@...aro.org>
To:     AKASHI Takahiro <takahiro.akashi@...aro.org>
Cc:     sudeep.holla@....com, cristian.marussi@....com, robh+dt@...nel.org,
        krzysztof.kozlowski+dt@...aro.org, conor+dt@...nel.org,
        Oleksii_Moisieiev@...m.com, linux-arm-kernel@...ts.infradead.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-gpio@...r.kernel.org
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@...aro.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@...aro.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

Powered by blists - more mailing lists