This is a revised version of my previous RFC[1]. Although I modified
the commits to make them look SCMI-independent, they are still posted
as RFC because I have never tested them on real hardware.
(background)
I'm currently working on implementing SCMI pinctrl/gpio drivers
on U-Boot[2]. Although the pinctrl driver for the kernel[3] was submitted
by EPAM, it doesn't contain the gpio driver and I believe that we should
discuss a couple of points on the kernel side to finalize my design for
U-Boot.
So this RFC is intended for reviews, especially to raise some issues.
1) how to obtain a value on an input pin
All the existing gpio drivers are set to obtain a value on an input
pin by accessing the hardware directly. In SCMI case, however, this is
just impossible in its nature and must be supported via a protocol
using "Input-value" configuration type. (See the spec[4], table-23.)
The current pinconf framework is missing the feature (the pinconf
parameter and a helper function). See patch#1, #2 and #3.
Please note that there is an issue around the pin configuration in
EPAM's current pinctrl driver as I commented[5].
2) DT bindings
I would like to propose a generic binding for pinctrl based gpio driver.
This allows a "consumer" driver to handle gpio pins like as other
normal gpio controllers support. (patch#5)
3) generic GPIO driver
Based on (2), I tried to prototype a generic driver in patch#4.
Thanks to a set of existing pinctrl_gpio helper functions, except (1),
It seems that the driver can be implemented not relying on pin controller
specific code, at least for SCMI pinctrl.
I will appreciate any comments.
-Takahiro Akashi
[1] https://lkml.iu.edu//hypermail/linux/kernel/2310.0/00362.html
[2] https://lists.denx.de/pipermail/u-boot/2023-September/529765.html
[3] https://lkml.iu.edu/hypermail/linux/kernel/2308.1/01082.html
[4] https://developer.arm.com/documentation/den0056/
[5] https://lkml.iu.edu/hypermail/linux/kernel/2308.2/07483.html
AKASHI Takahiro (5):
pinctrl: define PIN_CONFIG_INPUT
pinctrl: always export pin_config_get_for_pin()
pinctrl: add pinctrl_gpio_get_config()
gpio: add pinctrl based generic gpio driver
dt-bindings: gpio: Add bindings for pinctrl based generic gpio driver
.../bindings/gpio/pin-control-gpio.yaml | 55 ++++++
drivers/gpio/Kconfig | 7 +
drivers/gpio/Makefile | 1 +
drivers/gpio/gpio-by-pinctrl.c | 165 ++++++++++++++++++
drivers/pinctrl/core.c | 19 ++
drivers/pinctrl/pinconf.h | 10 +-
include/linux/pinctrl/consumer.h | 8 +
include/linux/pinctrl/pinconf-generic.h | 5 +
8 files changed, 268 insertions(+), 2 deletions(-)
create mode 100644 Documentation/devicetree/bindings/gpio/pin-control-gpio.yaml
create mode 100644 drivers/gpio/gpio-by-pinctrl.c
--
2.34.1
This configuration is intended to be used to allow a pin controller based
GPIO driver to obtain a value at a gpio input pin.
Signed-off-by: AKASHI Takahiro <[email protected]>
---
RFC v2 (Oct 5, 2023)
* improve a comment against @PIN_CONFIG_INPUT as per Linus
RFC(Oct 2, 2023)
---
include/linux/pinctrl/pinconf-generic.h | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/include/linux/pinctrl/pinconf-generic.h b/include/linux/pinctrl/pinconf-generic.h
index d74b7a4ea154..da0d80aa532d 100644
--- a/include/linux/pinctrl/pinconf-generic.h
+++ b/include/linux/pinctrl/pinconf-generic.h
@@ -67,6 +67,10 @@ struct pinctrl_map;
* passed as argument. The argument is in mA.
* @PIN_CONFIG_DRIVE_STRENGTH_UA: the pin will sink or source at most the current
* passed as argument. The argument is in uA.
+ * @PIN_CONFIG_INPUT: This will obtain a value on an input pin. To put a line
+ * into input mode, @PIN_CONFIG_INPUT_ENABLE must be used. Otherwise,
+ * an error will be returned. The returned argument is 1 for logic high
+ * and 0 for logic low.
* @PIN_CONFIG_INPUT_DEBOUNCE: this will configure the pin to debounce mode,
* which means it will wait for signals to settle when reading inputs. The
* argument gives the debounce time in usecs. Setting the
@@ -128,6 +132,7 @@ enum pin_config_param {
PIN_CONFIG_DRIVE_PUSH_PULL,
PIN_CONFIG_DRIVE_STRENGTH,
PIN_CONFIG_DRIVE_STRENGTH_UA,
+ PIN_CONFIG_INPUT,
PIN_CONFIG_INPUT_DEBOUNCE,
PIN_CONFIG_INPUT_ENABLE,
PIN_CONFIG_INPUT_SCHMITT,
--
2.34.1
Some pin controllers provide not only a method to set up lines but
also gpio function. With this commit, a new generic gpio driver will
be provided. It is implemented purely by using pinctrl interfaces.
One of such pin controllers is Arm's SCMI.
Signed-off-by: AKASHI Takahiro <[email protected]>
---
RFC v2 (Oct 5, 2023)
* rename the driver to pin-control-gpio (CONFIG_GPIO_BY_PINCTRL)
* return meaningful error codes instead of -1
* remove the masking at PIN_CONFIG_PACKED
* handle emulated OPEN_DRAIN configuration at get_direction()
* define config_set in gpio_chip
* drop remove hook
RFC (Oct 2, 2023)
---
drivers/gpio/Kconfig | 7 ++
drivers/gpio/Makefile | 1 +
drivers/gpio/gpio-by-pinctrl.c | 165 +++++++++++++++++++++++++++++++++
3 files changed, 173 insertions(+)
create mode 100644 drivers/gpio/gpio-by-pinctrl.c
diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 673bafb8be58..a60972be114c 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -216,6 +216,13 @@ config GPIO_BRCMSTB
help
Say yes here to enable GPIO support for Broadcom STB (BCM7XXX) SoCs.
+config GPIO_BY_PINCTRL
+ tristate "GPIO support based on a pure pin control backend"
+ depends on GPIOLIB
+ help
+ Select this option to support GPIO devices based solely on pin
+ control, specifically pin configuration, such as SCMI.
+
config GPIO_CADENCE
tristate "Cadence GPIO support"
depends on OF_GPIO
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index eb73b5d633eb..71458d81e16a 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -43,6 +43,7 @@ obj-$(CONFIG_GPIO_BD71828) += gpio-bd71828.o
obj-$(CONFIG_GPIO_BD9571MWV) += gpio-bd9571mwv.o
obj-$(CONFIG_GPIO_BRCMSTB) += gpio-brcmstb.o
obj-$(CONFIG_GPIO_BT8XX) += gpio-bt8xx.o
+obj-$(CONFIG_GPIO_BY_PINCTRL) += gpio-by-pinctrl.o
obj-$(CONFIG_GPIO_CADENCE) += gpio-cadence.o
obj-$(CONFIG_GPIO_CLPS711X) += gpio-clps711x.o
obj-$(CONFIG_GPIO_SNPS_CREG) += gpio-creg-snps.o
diff --git a/drivers/gpio/gpio-by-pinctrl.c b/drivers/gpio/gpio-by-pinctrl.c
new file mode 100644
index 000000000000..c297a9633e03
--- /dev/null
+++ b/drivers/gpio/gpio-by-pinctrl.c
@@ -0,0 +1,165 @@
+// SPDX-License-Identifier: GPL-2.0
+//
+// Copyright (C) 2023 Linaro Inc.
+// Author: AKASHI takahiro <[email protected]>
+
+#include <linux/gpio/driver.h>
+#include <linux/list.h>
+#include <linux/module.h>
+#include <linux/pinctrl/consumer.h>
+#include <linux/platform_device.h>
+#include <linux/types.h>
+#include "gpiolib.h"
+
+struct pin_control_gpio_priv {
+ struct gpio_chip chip;
+};
+
+static int pin_control_gpio_get_direction(struct gpio_chip *chip,
+ unsigned int offset)
+{
+ unsigned long config;
+ bool out_en, in_en;
+ int ret;
+
+ config = PIN_CONFIG_OUTPUT_ENABLE;
+ ret = pinctrl_gpio_get_config(chip->gpiodev->base + offset, &config);
+ if (!ret)
+ out_en = !!config;
+ else if (ret == -EINVAL)
+ out_en = false;
+ else
+ return ret;
+
+ config = PIN_CONFIG_INPUT_ENABLE;
+ ret = pinctrl_gpio_get_config(chip->gpiodev->base + offset, &config);
+ if (!ret)
+ in_en = !!config;
+ else if (ret == -EINVAL)
+ in_en = false;
+ else
+ return ret;
+
+ 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 may be an emulation for output with open drain */
+ config = PIN_CONFIG_DRIVE_OPEN_DRAIN;
+ ret = pinctrl_gpio_get_config(chip->gpiodev->base + offset,
+ &config);
+ if (!ret && config)
+ return GPIO_LINE_DIRECTION_OUT;
+ }
+
+ return -EINVAL;
+}
+
+static int pin_control_gpio_direction_input(struct gpio_chip *chip,
+ unsigned int offset)
+{
+ return pinctrl_gpio_direction_input(chip->gpiodev->base + offset);
+}
+
+static int pin_control_gpio_direction_output(struct gpio_chip *chip,
+ unsigned int offset, int val)
+{
+ return pinctrl_gpio_direction_output(chip->gpiodev->base + offset);
+}
+
+static int pin_control_gpio_get(struct gpio_chip *chip, unsigned int offset)
+{
+ unsigned long config;
+ int ret;
+
+ config = PIN_CONFIG_INPUT;
+ ret = pinctrl_gpio_get_config(chip->gpiodev->base + offset, &config);
+ if (ret)
+ return ret;
+
+ if (config >> 8)
+ return 1;
+
+ return 0;
+}
+
+static void pin_control_gpio_set(struct gpio_chip *chip, unsigned int offset,
+ int val)
+{
+ unsigned long config;
+
+ config = PIN_CONF_PACKED(PIN_CONFIG_OUTPUT, val);
+
+ pinctrl_gpio_set_config(chip->gpiodev->base + offset, config);
+}
+
+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;
+ }
+
+ return ngpios;
+}
+
+static int pin_control_gpio_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct pin_control_gpio_priv *priv;
+ struct gpio_chip *chip;
+ int ret;
+
+ priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+ if (!priv)
+ return -ENOMEM;
+
+ chip = &priv->chip;
+ chip->label = dev_name(dev);
+ chip->parent = dev;
+ chip->base = -1;
+
+ chip->request = gpiochip_generic_request;
+ chip->free = gpiochip_generic_free;
+ chip->get_direction = pin_control_gpio_get_direction;
+ chip->direction_input = pin_control_gpio_direction_input;
+ chip->direction_output = pin_control_gpio_direction_output;
+ chip->get = pin_control_gpio_get;
+ chip->set = pin_control_gpio_set;
+ chip->set_config = gpiochip_generic_config;
+
+ ret = devm_gpiochip_add_data(dev, chip, priv);
+ if (ret)
+ return ret;
+
+ chip->ngpio = sum_up_ngpios(chip);
+
+ platform_set_drvdata(pdev, priv);
+
+ return 0;
+}
+
+static const struct of_device_id pin_control_gpio_match[] = {
+ { .compatible = "pin-control-gpio" },
+ { /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, pin_control_gpio_match);
+
+static struct platform_driver pin_control_gpio_driver = {
+ .probe = pin_control_gpio_probe,
+ .driver = {
+ .name = "pin-control-gpio",
+ .of_match_table = pin_control_gpio_match,
+ },
+};
+module_platform_driver(pin_control_gpio_driver);
+
+MODULE_AUTHOR("AKASHI Takahiro <[email protected]>");
+MODULE_DESCRIPTION("Pinctrl based GPIO driver");
+MODULE_LICENSE("GPL");
--
2.34.1
This is a counterpart of pinctrl_gpio_set_config() which will be used,
at least initially, to implement gpio_get interface in pin controller
based generic gpio driver.
Signed-off-by: AKASHI Takahiro <[email protected]>
Reviewed-by: Linus Walleij <[email protected]>
---
RFC (Oct 2, 2023)
---
drivers/pinctrl/core.c | 19 +++++++++++++++++++
include/linux/pinctrl/consumer.h | 8 ++++++++
2 files changed, 27 insertions(+)
diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
index e9dc9638120a..2f9c2efdfe0e 100644
--- a/drivers/pinctrl/core.c
+++ b/drivers/pinctrl/core.c
@@ -926,6 +926,25 @@ int pinctrl_gpio_set_config(unsigned gpio, unsigned long config)
}
EXPORT_SYMBOL_GPL(pinctrl_gpio_set_config);
+int pinctrl_gpio_get_config(unsigned int gpio, unsigned long *config)
+{
+ struct pinctrl_gpio_range *range;
+ struct pinctrl_dev *pctldev;
+ int ret, pin;
+
+ ret = pinctrl_get_device_gpio_range(gpio, &pctldev, &range);
+ if (ret)
+ return ret;
+
+ mutex_lock(&pctldev->mutex);
+ pin = gpio_to_pin(range, gpio);
+ ret = pin_config_get_for_pin(pctldev, pin, config);
+ mutex_unlock(&pctldev->mutex);
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(pinctrl_gpio_get_config);
+
static struct pinctrl_state *find_state(struct pinctrl *p,
const char *name)
{
diff --git a/include/linux/pinctrl/consumer.h b/include/linux/pinctrl/consumer.h
index 4729d54e8995..852fac97a79b 100644
--- a/include/linux/pinctrl/consumer.h
+++ b/include/linux/pinctrl/consumer.h
@@ -31,6 +31,8 @@ extern void pinctrl_gpio_free(unsigned gpio);
extern int pinctrl_gpio_direction_input(unsigned gpio);
extern int pinctrl_gpio_direction_output(unsigned gpio);
extern int pinctrl_gpio_set_config(unsigned gpio, unsigned long config);
+extern int pinctrl_gpio_get_config(unsigned int gpio,
+ unsigned long *config);
extern struct pinctrl * __must_check pinctrl_get(struct device *dev);
extern void pinctrl_put(struct pinctrl *p);
@@ -92,6 +94,12 @@ static inline int pinctrl_gpio_set_config(unsigned gpio, unsigned long config)
return 0;
}
+static inline int pinctrl_gpio_get_config(unsigned int gpio,
+ unsigned long *config)
+{
+ return 0;
+}
+
static inline struct pinctrl * __must_check pinctrl_get(struct device *dev)
{
return NULL;
--
2.34.1
On Thu, Oct 5, 2023 at 4:59 AM AKASHI Takahiro
<[email protected]> wrote:
> This configuration is intended to be used to allow a pin controller based
> GPIO driver to obtain a value at a gpio input pin.
>
> Signed-off-by: AKASHI Takahiro <[email protected]>
Reviewed-by: Linus Walleij <[email protected]>
Yours,
Linus Walleij
On Thu, Oct 5, 2023 at 4:59 AM AKASHI Takahiro
<[email protected]> wrote:
> Some pin controllers provide not only a method to set up lines but
> also gpio function. With this commit, a new generic gpio driver will
> be provided. It is implemented purely by using pinctrl interfaces.
> One of such pin controllers is Arm's SCMI.
>
> Signed-off-by: AKASHI Takahiro <[email protected]>
> ---
> RFC v2 (Oct 5, 2023)
RFC v2 looks very good to me, definitely something that can be merged
as a starting point once the hardware has been tested.
> +static int pin_control_gpio_direction_input(struct gpio_chip *chip,
> + unsigned int offset)
> +{
> + return pinctrl_gpio_direction_input(chip->gpiodev->base + offset);
> +}
> +
> +static int pin_control_gpio_direction_output(struct gpio_chip *chip,
> + unsigned int offset, int val)
> +{
> + return pinctrl_gpio_direction_output(chip->gpiodev->base + offset);
> +}
IIRC Bartosz is working on a patch set getting rid of this kludge having to
call with base + offset in every driver, replacing it with generic calls that
you can just assign in the gpio_chip.
When this gets applied these changes will likely be in place so you will
get rid of this too.
Yours,
Linus Walleij
Hi Linus and Oleksii,
On Tue, Oct 10, 2023 at 02:00:40PM +0200, Linus Walleij wrote:
> On Thu, Oct 5, 2023 at 4:59???AM AKASHI Takahiro
> <[email protected]> wrote:
>
>
> > Some pin controllers provide not only a method to set up lines but
> > also gpio function. With this commit, a new generic gpio driver will
> > be provided. It is implemented purely by using pinctrl interfaces.
> > One of such pin controllers is Arm's SCMI.
> >
> > Signed-off-by: AKASHI Takahiro <[email protected]>
> > ---
> > RFC v2 (Oct 5, 2023)
>
> RFC v2 looks very good to me, definitely something that can be merged
> as a starting point once the hardware has been tested.
Thank you for your support.
I think the easiest and best way to test the code is that Oleskii will try
my patch on his platform, r-car, on which I believe that SCMI FW for pin
controller is already available since he tested his pinctrl driver.
@Oleskii, can you please take a time for the test?
(I will assist you in case of any error.)
> > +static int pin_control_gpio_direction_input(struct gpio_chip *chip,
> > + unsigned int offset)
> > +{
> > + return pinctrl_gpio_direction_input(chip->gpiodev->base + offset);
> > +}
> > +
> > +static int pin_control_gpio_direction_output(struct gpio_chip *chip,
> > + unsigned int offset, int val)
> > +{
> > + return pinctrl_gpio_direction_output(chip->gpiodev->base + offset);
> > +}
>
> IIRC Bartosz is working on a patch set getting rid of this kludge having to
> call with base + offset in every driver, replacing it with generic calls that
> you can just assign in the gpio_chip.
>
> When this gets applied these changes will likely be in place so you will
> get rid of this too.
I will try to keep eyes on Bartosz's patch.
Thanks,
-Takahiro Akashi
> Yours,
> Linus Walleij
Thu, Oct 05, 2023 at 11:58:38AM +0900, AKASHI Takahiro kirjoitti:
> This is a revised version of my previous RFC[1]. Although I modified
> the commits to make them look SCMI-independent, they are still posted
> as RFC because I have never tested them on real hardware.
>
> (background)
> I'm currently working on implementing SCMI pinctrl/gpio drivers
> on U-Boot[2]. Although the pinctrl driver for the kernel[3] was submitted
> by EPAM, it doesn't contain the gpio driver and I believe that we should
> discuss a couple of points on the kernel side to finalize my design for
> U-Boot.
>
> So this RFC is intended for reviews, especially to raise some issues.
>
> 1) how to obtain a value on an input pin
> All the existing gpio drivers are set to obtain a value on an input
> pin by accessing the hardware directly. In SCMI case, however, this is
> just impossible in its nature and must be supported via a protocol
> using "Input-value" configuration type. (See the spec[4], table-23.)
>
> The current pinconf framework is missing the feature (the pinconf
> parameter and a helper function). See patch#1, #2 and #3.
>
> Please note that there is an issue around the pin configuration in
> EPAM's current pinctrl driver as I commented[5].
>
> 2) DT bindings
> I would like to propose a generic binding for pinctrl based gpio driver.
> This allows a "consumer" driver to handle gpio pins like as other
> normal gpio controllers support. (patch#5)
>
> 3) generic GPIO driver
> Based on (2), I tried to prototype a generic driver in patch#4.
> Thanks to a set of existing pinctrl_gpio helper functions, except (1),
> It seems that the driver can be implemented not relying on pin controller
> specific code, at least for SCMI pinctrl.
>
> I will appreciate any comments.
Any comment here: I'm listed as a designated reviewer of GPIO patches, why am I
not Cc'ed on this? I definitely have some comments against the code (no DT,
though). Please, use (up-to-date) MAINTAINERS in your v3.
--
With Best Regards,
Andy Shevchenko
Hi Andy,
On Fri, Oct 20, 2023 at 12:27:58AM +0300, [email protected] wrote:
> Thu, Oct 05, 2023 at 11:58:38AM +0900, AKASHI Takahiro kirjoitti:
> > This is a revised version of my previous RFC[1]. Although I modified
> > the commits to make them look SCMI-independent, they are still posted
> > as RFC because I have never tested them on real hardware.
> >
> > (background)
> > I'm currently working on implementing SCMI pinctrl/gpio drivers
> > on U-Boot[2]. Although the pinctrl driver for the kernel[3] was submitted
> > by EPAM, it doesn't contain the gpio driver and I believe that we should
> > discuss a couple of points on the kernel side to finalize my design for
> > U-Boot.
> >
> > So this RFC is intended for reviews, especially to raise some issues.
> >
> > 1) how to obtain a value on an input pin
> > All the existing gpio drivers are set to obtain a value on an input
> > pin by accessing the hardware directly. In SCMI case, however, this is
> > just impossible in its nature and must be supported via a protocol
> > using "Input-value" configuration type. (See the spec[4], table-23.)
> >
> > The current pinconf framework is missing the feature (the pinconf
> > parameter and a helper function). See patch#1, #2 and #3.
> >
> > Please note that there is an issue around the pin configuration in
> > EPAM's current pinctrl driver as I commented[5].
> >
> > 2) DT bindings
> > I would like to propose a generic binding for pinctrl based gpio driver.
> > This allows a "consumer" driver to handle gpio pins like as other
> > normal gpio controllers support. (patch#5)
> >
> > 3) generic GPIO driver
> > Based on (2), I tried to prototype a generic driver in patch#4.
> > Thanks to a set of existing pinctrl_gpio helper functions, except (1),
> > It seems that the driver can be implemented not relying on pin controller
> > specific code, at least for SCMI pinctrl.
> >
> > I will appreciate any comments.
>
> Any comment here: I'm listed as a designated reviewer of GPIO patches, why am I
> not Cc'ed on this?
My apologies. I will add you in Cc.
> I definitely have some comments against the code (no DT,
> though). Please, use (up-to-date) MAINTAINERS in your v3.
Please don't hesitate to make comments here on v2 so that I can
include your reviews in v3.
Thanks,
-Takahiro Akashi
>
> --
> With Best Regards,
> Andy Shevchenko
>
>