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

  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>
---
 drivers/gpio/Kconfig     |   8 ++
 drivers/gpio/Makefile    |   1 +
 drivers/gpio/gpio-scmi.c | 154 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 163 insertions(+)
 create mode 100644 drivers/gpio/gpio-scmi.c
  

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
  

Hi Linus,

On Tue, Oct 03, 2023 at 11:35:31PM +0200, Linus Walleij wrote:
> 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.

I'm still not sure whether my approach can be applied to any other
pinctrl-based gpio drivers, in which extra (driver-specific) operations
might be needed around the generic pinctrl_gpio helpers (i.e. gpiolib.c).
For instance, look at gpio-tegra.c:

! static int tegra_gpio_direction_input(struct gpio_chip *chip,
!                                       unsigned int offset)
! {
!         struct tegra_gpio_info *tgi = gpiochip_get_data(chip);
! 
!         tegra_gpio_mask_write(tgi, GPIO_MSK_OE(tgi, offset), offset, 0);
!         tegra_gpio_enable(tgi, offset);
! 
!         ret = pinctrl_gpio_direction_input(chip->base + offset);
!	  ...
! }

That said, I will send a next version incorporating the changes you
suggest here.

> >  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

Okay.


> > +       tristate "GPIO support based on SCMI pinctrl"
> 
> "GPIO support based on a pure pin control back-end"

Okay.

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

Okay, I don't know how device properties work, though.

> > +       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."

Okay.

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

Actually we need neither to compile the code.

> > +#include "gpiolib.h"
> 
> Why?

Because we need to access members of struct gpio_device.

> 
> > +static int scmi_gpio_get_direction(struct gpio_chip *chip, unsigned int offset)
> 
> Rename all functions pinctrl_gpio_*

Well, this change will result in name conflicts against existing
pinctrl_gpio_direction_[in|out]out(). So use "pin_control_gpio_" prefix.

> > +{
> > +       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).

Yes.

> > +       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.)
>      */
> }

Not sure how the last case (in_en && out_en && DRIVE_OPEN_DRAIN) works.

In order to be able to read a value as an input pin, I think, we need
to set the output status to Hi-Z. Then we should recognize it as "INPUT"?
In this case, however, we cannot distinguish the other case where we want
to use the pin as OUTPUT and drive it to (active) high.

> /* 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!

Yes.

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

Unfortunately, the function prototype of "set" in struct gpio_device is
        void (*set)(struct gpio_chip *gc, unsigned int offset, int value);

So we cannot propagate an error to the caller.

> > +{
> > +       unsigned long config;
> > +
> > +       config = PIN_CONF_PACKED(PIN_CONFIG_OUTPUT, val & 0x1);
> 
> No need to add & 0x01, the gpiolib core already does this.

Which part of gpiolib core?
The argument is shifted by 8 in PIN_CONF_PACKED(), but never normalized.
Since the driver code, however, should verify the value in some way, I will
drop the masking here.


> > +       pinctrl_gpio_set_config(chip->gpiodev->base + offset, config);
> 
> return pinctrl_gpio_set_config(); so error is propagated.

See above.

> > +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.

I don't get your point.
However many pins SCMI firmware (or other normal pin controllers) might
expose, the total number of pins available by this driver is limited by
"gpio-ranges" property.
So the sum as "ngpios" should make sense unless a user accidentally
specifies a wrong range of pins.

Do I misunderstand anything?

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

Okay. This code is a remnant from the original driver that I referred to
as a base.

> > +       /* 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;

Yes.

> 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.

Well, the error will be determined by SCMI firmware(server)
not the driver itself :)

> > +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.

Okay.

> > +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.

Thank you for your kind suggestions.

-Takahiro Akashi


> Yours,
> Linus Walleij
  

Hi Takahiro,

I see you are on track with this!

Some clarifications:

On Wed, Oct 4, 2023 at 8:53 AM AKASHI Takahiro
<takahiro.akashi@linaro.org> wrote:

> I'm still not sure whether my approach can be applied to any other
> pinctrl-based gpio drivers, in which extra (driver-specific) operations
> might be needed around the generic pinctrl_gpio helpers (i.e. gpiolib.c).
> For instance, look at gpio-tegra.c:

Yeah, it kind of requires a "pure" pin controller underneath that don't
want to do anything else on any operations, otherwise we are back
to a per-soc pin control driver.

But I think it is appropriate for abstractions that strive to provide
"total abstraction behind a firmware", so such as SCMI or ACPI (heh).

> > Skip this, let's use device properties instead. They will anyways just translate
> > to OF properties in the OF case.
>
> Okay, I don't know how device properties work, though.

They are pretty much 1-to-1 slot-ins for the corresponding of_*
functions, passing struct device * instead of struct device_node *,
if you look in include/linux/property.h you will feel at home very
quickly.

> > > +static int scmi_gpio_get_direction(struct gpio_chip *chip, unsigned int offset)
> >
> > Rename all functions pinctrl_gpio_*
>
> Well, this change will result in name conflicts against existing
> pinctrl_gpio_direction_[in|out]out(). So use "pin_control_gpio_" prefix.

Yeah that works, or pincontro_by_gpio_ or such.

> Not sure how the last case (in_en && out_en && DRIVE_OPEN_DRAIN) works.

I wrote some documentation! But it is hidden deep in the docs:
https://docs.kernel.org/driver-api/gpio/driver.html#gpio-lines-with-open-drain-source-support

> In order to be able to read a value as an input pin, I think, we need
> to set the output status to Hi-Z. Then we should recognize it as "INPUT"?
> In this case, however, we cannot distinguish the other case where we want
> to use the pin as OUTPUT and drive it to (active) high.

With open drain, on GPIO controllers that do not support a native
open drain mode, we emulate open drain output high by switching
the line into input mode. The line in this case has a pull-up resistor
(internal or external) and as input mode is high-Z the pull up resistor
will pull the signal high, to any level - could be e.g 48V which is
helpful for some serial links.

But this case is really tricky so it can be hard to get things right,
I get a bit confused and so we need to think about it a few times.

> > > +static void scmi_gpio_set(struct gpio_chip *chip, unsigned int offset, int val)
> >
> > static int?
>
> Unfortunately, the function prototype of "set" in struct gpio_device is
>         void (*set)(struct gpio_chip *gc, unsigned int offset, int value);
>
> So we cannot propagate an error to the caller.

Grrr that must be my fault. Sorry about not fixing this :(

> > No need to add & 0x01, the gpiolib core already does this.
>
> Which part of gpiolib core?

chip->set = scmi_gpio_set; gets called like this in gpiolib:

 gpiod_direction_output_raw_commit(..., int value)
{
    int val = !!value;
(...)
    gc->set(gc, gpio_chip_hwgpio(desc), val);

Notice clamping int val = !!value; will make the passed val 0 or 1.

> > > +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.
>
> I don't get your point.
> However many pins SCMI firmware (or other normal pin controllers) might
> expose, the total number of pins available by this driver is limited by
> "gpio-ranges" property.
> So the sum as "ngpios" should make sense unless a user accidentally
> specifies a wrong range of pins.

Yes.

And it is this fact that the same number need to appear in two places
and double-specification will sooner or later bring us to the situation
where the two do not agree, and what do we do then?

If the ranges come from firmware, which is subject to change such
as "oops we forgot this pin", the GPIO number will just insert itself
among the already existing ones: say we have two ranges:

1: 0..5
2: 6..9

Ooops forgot a GPIO in the first range, it has to be bumped to
0..6.

But somewhere in the device tree there is:

foo-gpios = <&scmi_gpio 7 GPIO_OUT_LOW>;

So now this is wrong (need to be changed to 8) and we have zero tooling
to detect this, the author just has to be very careful all the time.

But I honestly do not know any better way.

> > 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.
>
> Well, the error will be determined by SCMI firmware(server)
> not the driver itself :)

Hehe, I think it is good that the SCMI firmware gets some exercise
from day 1!

Yours,
Linus Walleij
  

Hi Linus,

On Wed, Oct 04, 2023 at 10:35:05AM +0200, Linus Walleij wrote:
> Hi Takahiro,
> 
> I see you are on track with this!
> 
> Some clarifications:
> 
> On Wed, Oct 4, 2023 at 8:53???AM AKASHI Takahiro
> <takahiro.akashi@linaro.org> wrote:
> 
> > I'm still not sure whether my approach can be applied to any other
> > pinctrl-based gpio drivers, in which extra (driver-specific) operations
> > might be needed around the generic pinctrl_gpio helpers (i.e. gpiolib.c).
> > For instance, look at gpio-tegra.c:
> 
> Yeah, it kind of requires a "pure" pin controller underneath that don't
> want to do anything else on any operations, otherwise we are back
> to a per-soc pin control driver.
> 
> But I think it is appropriate for abstractions that strive to provide
> "total abstraction behind a firmware", so such as SCMI or ACPI (heh).

Right. So we are on the same page now.

> > > Skip this, let's use device properties instead. They will anyways just translate
> > > to OF properties in the OF case.
> >
> > Okay, I don't know how device properties work, though.
> 
> They are pretty much 1-to-1 slot-ins for the corresponding of_*
> functions, passing struct device * instead of struct device_node *,
> if you look in include/linux/property.h you will feel at home very
> quickly.
> 
> > > > +static int scmi_gpio_get_direction(struct gpio_chip *chip, unsigned int offset)
> > >
> > > Rename all functions pinctrl_gpio_*
> >
> > Well, this change will result in name conflicts against existing
> > pinctrl_gpio_direction_[in|out]out(). So use "pin_control_gpio_" prefix.
> 
> Yeah that works, or pincontro_by_gpio_ or such.

I will use "pin_control_gpio_", which still sounds confusing though.
Please modify it if you don't like.

> > Not sure how the last case (in_en && out_en && DRIVE_OPEN_DRAIN) works.
> 
> I wrote some documentation! But it is hidden deep in the docs:
> https://docs.kernel.org/driver-api/gpio/driver.html#gpio-lines-with-open-drain-source-support
> 
> > In order to be able to read a value as an input pin, I think, we need
> > to set the output status to Hi-Z. Then we should recognize it as "INPUT"?
> > In this case, however, we cannot distinguish the other case where we want
> > to use the pin as OUTPUT and drive it to (active) high.
> 
> With open drain, on GPIO controllers that do not support a native
> open drain mode, we emulate open drain output high by switching
> the line into input mode. The line in this case has a pull-up resistor
> (internal or external) and as input mode is high-Z the pull up resistor
> will pull the signal high, to any level - could be e.g 48V which is
> helpful for some serial links.

I now think I see what you meant here, but still not sure why we need to
assert CONFIG_INPUT and CONFIG_OUT at the same time from API viewpoint.

Anyhow, I will follow the logic that you suggested.

> But this case is really tricky so it can be hard to get things right,
> I get a bit confused and so we need to think about it a few times.
> 
> > > > +static void scmi_gpio_set(struct gpio_chip *chip, unsigned int offset, int val)
> > >
> > > static int?
> >
> > Unfortunately, the function prototype of "set" in struct gpio_device is
> >         void (*set)(struct gpio_chip *gc, unsigned int offset, int value);
> >
> > So we cannot propagate an error to the caller.
> 
> Grrr that must be my fault. Sorry about not fixing this :(
> 
> > > No need to add & 0x01, the gpiolib core already does this.
> >
> > Which part of gpiolib core?
> 
> chip->set = scmi_gpio_set; gets called like this in gpiolib:
> 
>  gpiod_direction_output_raw_commit(..., int value)
> {
>     int val = !!value;
> (...)
>     gc->set(gc, gpio_chip_hwgpio(desc), val);
> 
> Notice clamping int val = !!value; will make the passed val 0 or 1.

Yeah.

> > > > +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.
> >
> > I don't get your point.
> > However many pins SCMI firmware (or other normal pin controllers) might
> > expose, the total number of pins available by this driver is limited by
> > "gpio-ranges" property.
> > So the sum as "ngpios" should make sense unless a user accidentally
> > specifies a wrong range of pins.
> 
> Yes.
> 
> And it is this fact that the same number need to appear in two places
> and double-specification will sooner or later bring us to the situation
> where the two do not agree, and what do we do then?
> 
> If the ranges come from firmware, which is subject to change such
> as "oops we forgot this pin", the GPIO number will just insert itself
> among the already existing ones: say we have two ranges:
> 
> 1: 0..5
> 2: 6..9
> 
> Ooops forgot a GPIO in the first range, it has to be bumped to
> 0..6.
> 
> But somewhere in the device tree there is:
> 
> foo-gpios = <&scmi_gpio 7 GPIO_OUT_LOW>;
> 
> So now this is wrong (need to be changed to 8) and we have zero tooling
> to detect this, the author just has to be very careful all the time.

Well, even without a change by an user, this kind of human error
may happen. There is no way to verify the correct *pin number*,
say, if I specify 100 instead of 7 in an above example.

> But I honestly do not know any better way.

One good practice to mitigate those cases might be to use a (gpio or
gpio-group) name instead of a pin number, or a "virtual" gpio device.

        foo_gpio: gpio@0 {
            compatibles = "pin-control-gpio";

            gpio-range = <&scmi_pinctrl 0 0 0>;
            gpio-range-group-name = "pins_for_foo";
        }
        baa_gpio: gpio@1 {
            compatibles = "pin-control-gpio";

            gpio-range = <&scmi_pinctrl 0 0 0>;
            gpio-range-group-name = "pins_for_baa";
        }

# Not sure multiple "pin-control-gpio" devices are possible.

-Takahiro Akashi

> > > 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.
> >
> > Well, the error will be determined by SCMI firmware(server)
> > not the driver itself :)
> 
> Hehe, I think it is good that the SCMI firmware gets some exercise
> from day 1!
> 
> Yours,
> Linus Walleij