lkml.org 
[lkml]   [2023]   [Oct]   [3]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [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

    \
     
     \ /
      Last update: 2023-10-03 23:37    [W:3.164 / U:0.016 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site