Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1562539 > unrolled thread

[PATCH 0/3] pinctrl / gpio: Allow GPIO chips to use generic pinconfig

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/3] pinctrl / gpio: Allow GPIO chips to use generic pinconfig Mika Westerberg <mika.westerberg@linux.intel.com> - 2017-01-19 11:00 +0100
    [PATCH 1/3] pinctrl: Widen the generic pinconf argument from 16 to 24 bits Mika Westerberg <mika.westerberg@linux.intel.com> - 2017-01-19 11:00 +0100
      Re: [PATCH 1/3] pinctrl: Widen the generic pinconf argument from 16  to 24 bits Andy Shevchenko <andriy.shevchenko@linux.intel.com> - 2017-01-19 13:10 +0100
      Re: [PATCH 1/3] pinctrl: Widen the generic pinconf argument from 16  to 24 bits Linus Walleij <linus.walleij@linaro.org> - 2017-01-20 10:00 +0100
    Re: [PATCH 0/3] pinctrl / gpio: Allow GPIO chips to use generic  pinconfig Andy Shevchenko <andriy.shevchenko@linux.intel.com> - 2017-01-19 13:20 +0100
    Re: [PATCH 3/3] pinctrl / gpio: Introduce .set_config() callback  for GPIO chips Andy Shevchenko <andriy.shevchenko@linux.intel.com> - 2017-01-19 13:20 +0100
    Re: [PATCH 3/3] pinctrl / gpio: Introduce .set_config() callback for  GPIO chips Linus Walleij <linus.walleij@linaro.org> - 2017-01-20 10:20 +0100
      Re: [PATCH 3/3] pinctrl / gpio: Introduce .set_config() callback for  GPIO chips Neil Armstrong <narmstrong@baylibre.com> - 2017-01-20 10:30 +0100
        Re: [PATCH 3/3] pinctrl / gpio: Introduce .set_config() callback for  GPIO chips Linus Walleij <linus.walleij@linaro.org> - 2017-01-20 11:00 +0100
      Re: [PATCH 3/3] pinctrl / gpio: Introduce .set_config() callback for  GPIO chips Mika Westerberg <mika.westerberg@linux.intel.com> - 2017-01-20 14:40 +0100

#1562539 — [PATCH 0/3] pinctrl / gpio: Allow GPIO chips to use generic pinconfig

This series makes it possible to configure pins from GPIO chip drivers by
implementing a new callback .set_config(). This callback replaces the
existing .set_single_ended() and .set_debounce() simply because adding new
callbacks for each possible configuration type does not scale. So instead
we re-use the existing generic pinconf types and the packed format.

This is a follow up of discussion on:

  https://patchwork.ozlabs.org/patch/713289/

While doing that, it was found out that the current packed format does not
support all realistic debounce time values. The limit is ~64ms which does
not cover mechanical switches connected to GPIOs that migh require values
up to hundreths of milliseconds.

To solve that we change the packed format so that the value takes 24 bits
instead of 16, and change the callers to use 32-bit integer instead.

We also make it possible for GPIO chip driver to call pinctrl directly by
providing a new function pinctrl_gpio_set_config() following
pinctrl_gpio_direction_output() and friends.

I've tested this on Intel Gemini Lake SoC. Non-Intel drivers are compile
tested only because I do not have the hardware.

Mika Westerberg (3):
  pinctrl: Widen the generic pinconf argument from 16 to 24 bits
  pinctrl: Allow configuration of pins from gpiolib based drivers
  pinctrl / gpio: Introduce .set_config() callback for GPIO chips

 Documentation/gpio/driver.txt                    |  9 ++--
 drivers/gpio/gpio-bcm-kona.c                     | 14 +++++-
 drivers/gpio/gpio-dln2.c                         | 12 ++++--
 drivers/gpio/gpio-dwapb.c                        | 14 +++++-
 drivers/gpio/gpio-ep93xx.c                       | 11 +++--
 drivers/gpio/gpio-f7188x.c                       | 19 ++++----
 drivers/gpio/gpio-lp873x.c                       | 14 +++---
 drivers/gpio/gpio-max77620.c                     | 20 ++++-----
 drivers/gpio/gpio-menz127.c                      | 34 +++++++++++----
 drivers/gpio/gpio-merrifield.c                   | 14 +++++-
 drivers/gpio/gpio-omap.c                         | 14 +++++-
 drivers/gpio/gpio-tc3589x.c                      | 15 +++----
 drivers/gpio/gpio-tegra.c                        | 14 +++++-
 drivers/gpio/gpio-tps65218.c                     | 14 +++---
 drivers/gpio/gpio-vx855.c                        | 13 +++---
 drivers/gpio/gpio-wcove.c                        | 13 +++---
 drivers/gpio/gpio-wm831x.c                       | 21 +++++----
 drivers/gpio/gpio-wm8994.c                       | 13 +++---
 drivers/gpio/gpiolib.c                           | 41 +++++++++---------
 drivers/pinctrl/bcm/pinctrl-bcm281xx.c           |  6 +--
 drivers/pinctrl/bcm/pinctrl-iproc-gpio.c         |  2 +-
 drivers/pinctrl/bcm/pinctrl-ns2-mux.c            |  6 +--
 drivers/pinctrl/bcm/pinctrl-nsp-gpio.c           |  6 +--
 drivers/pinctrl/core.c                           | 28 ++++++++++++
 drivers/pinctrl/intel/pinctrl-cherryview.c       |  4 +-
 drivers/pinctrl/mediatek/pinctrl-mtk-common.c    | 14 +++++-
 drivers/pinctrl/meson/pinctrl-meson.c            |  2 -
 drivers/pinctrl/pinconf.c                        | 12 ++++++
 drivers/pinctrl/pinconf.h                        |  9 ++++
 drivers/pinctrl/pinctrl-amd.c                    | 14 +++++-
 drivers/pinctrl/pinctrl-da850-pupd.c             |  2 -
 drivers/pinctrl/pinctrl-lpc18xx.c                | 10 ++---
 drivers/pinctrl/pinctrl-max77620.c               |  2 +-
 drivers/pinctrl/pinctrl-palmas.c                 |  2 +-
 drivers/pinctrl/pinctrl-rockchip.c               |  2 +-
 drivers/pinctrl/pinctrl-single.c                 |  2 +-
 drivers/pinctrl/pinctrl-sx150x.c                 | 55 ++++++++----------------
 drivers/pinctrl/sirf/pinctrl-atlas7.c            |  3 +-
 drivers/pinctrl/sunxi/pinctrl-sunxi.c            |  2 +-
 drivers/pinctrl/uniphier/pinctrl-uniphier-core.c |  4 +-
 drivers/pinctrl/vt8500/pinctrl-wmt.c             |  2 +-
 drivers/rtc/rtc-omap.c                           |  2 +-
 drivers/staging/greybus/gpio.c                   | 15 ++++---
 drivers/usb/serial/cp210x.c                      | 13 +++---
 include/linux/gpio/driver.h                      | 35 +++------------
 include/linux/pinctrl/consumer.h                 |  6 +++
 include/linux/pinctrl/pinconf-generic.h          | 51 +++++++++++-----------
 47 files changed, 376 insertions(+), 254 deletions(-)

-- 
2.11.0

[toc] | [next] | [standalone]

#1562549 — [PATCH 1/3] pinctrl: Widen the generic pinconf argument from 16 to 24 bits

The current pinconf packed format allows only 16-bit argument limiting
the maximum value 65535. For most types this is enough. However,
debounce time can be in range of hundreths of milliseconds in case of
mechanical switches so we cannot represent the worst case using the
current format.

In order to support larger values change the packed format so that the
lower 8 bits are used as type which leaves 24 bits for the argument.
This allows representing values up to 16777215 and debounce times up to
16 seconds.

We also convert the existing users to use 32-bit integer when extracting
argument from the packed configuration value.

Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/pinctrl/bcm/pinctrl-bcm281xx.c           |  6 +++---
 drivers/pinctrl/bcm/pinctrl-iproc-gpio.c         |  2 +-
 drivers/pinctrl/bcm/pinctrl-ns2-mux.c            |  6 +++---
 drivers/pinctrl/bcm/pinctrl-nsp-gpio.c           |  6 +++---
 drivers/pinctrl/intel/pinctrl-cherryview.c       |  4 ++--
 drivers/pinctrl/meson/pinctrl-meson.c            |  2 --
 drivers/pinctrl/pinctrl-da850-pupd.c             |  2 --
 drivers/pinctrl/pinctrl-lpc18xx.c                | 10 +++++-----
 drivers/pinctrl/pinctrl-max77620.c               |  2 +-
 drivers/pinctrl/pinctrl-palmas.c                 |  2 +-
 drivers/pinctrl/pinctrl-rockchip.c               |  2 +-
 drivers/pinctrl/pinctrl-single.c                 |  2 +-
 drivers/pinctrl/sirf/pinctrl-atlas7.c            |  3 ++-
 drivers/pinctrl/sunxi/pinctrl-sunxi.c            |  2 +-
 drivers/pinctrl/uniphier/pinctrl-uniphier-core.c |  4 ++--
 drivers/pinctrl/vt8500/pinctrl-wmt.c             |  2 +-
 drivers/rtc/rtc-omap.c                           |  2 +-
 include/linux/pinctrl/pinconf-generic.h          | 19 +++++++++++--------
 18 files changed, 39 insertions(+), 39 deletions(-)

diff --git a/drivers/pinctrl/bcm/pinctrl-bcm281xx.c b/drivers/pinctrl/bcm/pinctrl-bcm281xx.c
index a5331fdfc795..810a81786f62 100644
--- a/drivers/pinctrl/bcm/pinctrl-bcm281xx.c
+++ b/drivers/pinctrl/bcm/pinctrl-bcm281xx.c
@@ -1106,7 +1106,7 @@ static int bcm281xx_std_pin_update(struct pinctrl_dev *pctldev,
 	struct bcm281xx_pinctrl_data *pdata = pinctrl_dev_get_drvdata(pctldev);
 	int i;
 	enum pin_config_param param;
-	u16 arg;
+	u32 arg;
 
 	for (i = 0; i < num_configs; i++) {
 		param = pinconf_to_config_param(configs[i]);
@@ -1222,7 +1222,7 @@ static int bcm281xx_i2c_pin_update(struct pinctrl_dev *pctldev,
 	struct bcm281xx_pinctrl_data *pdata = pinctrl_dev_get_drvdata(pctldev);
 	int i, j;
 	enum pin_config_param param;
-	u16 arg;
+	u32 arg;
 
 	for (i = 0; i < num_configs; i++) {
 		param = pinconf_to_config_param(configs[i]);
@@ -1292,7 +1292,7 @@ static int bcm281xx_hdmi_pin_update(struct pinctrl_dev *pctldev,
 	struct bcm281xx_pinctrl_data *pdata = pinctrl_dev_get_drvdata(pctldev);
 	int i;
 	enum pin_config_param param;
-	u16 arg;
+	u32 arg;
 
 	for (i = 0; i < num_configs; i++) {
 		param = pinconf_to_config_param(configs[i]);
diff --git a/drivers/pinctrl/bcm/pinctrl-iproc-gpio.c b/drivers/pinctrl/bcm/pinctrl-iproc-gpio.c
index 5d1e505c3c63..3ca925dfefd1 100644
--- a/drivers/pinctrl/bcm/pinctrl-iproc-gpio.c
+++ b/drivers/pinctrl/bcm/pinctrl-iproc-gpio.c
@@ -619,7 +619,7 @@ static int iproc_pin_config_set(struct pinctrl_dev *pctldev, unsigned pin,
 {
 	struct iproc_gpio *chip = pinctrl_dev_get_drvdata(pctldev);
 	enum pin_config_param param;
-	u16 arg;
+	u32 arg;
 	unsigned i, gpio = iproc_pin_to_gpio(pin);
 	int ret = -ENOTSUPP;
 
diff --git a/drivers/pinctrl/bcm/pinctrl-ns2-mux.c b/drivers/pinctrl/bcm/pinctrl-ns2-mux.c
index 13a4c2774157..4b5cf0e0f16e 100644
--- a/drivers/pinctrl/bcm/pinctrl-ns2-mux.c
+++ b/drivers/pinctrl/bcm/pinctrl-ns2-mux.c
@@ -703,7 +703,7 @@ static int ns2_pin_get_enable(struct pinctrl_dev *pctrldev, unsigned int pin)
 }
 
 static int ns2_pin_set_slew(struct pinctrl_dev *pctrldev, unsigned int pin,
-			    u16 slew)
+			    u32 slew)
 {
 	struct ns2_pinctrl *pinctrl = pinctrl_dev_get_drvdata(pctrldev);
 	struct ns2_pin *pin_data = pctrldev->desc->pins[pin].drv_data;
@@ -793,7 +793,7 @@ static void ns2_pin_get_pull(struct pinctrl_dev *pctrldev,
 }
 
 static int ns2_pin_set_strength(struct pinctrl_dev *pctrldev, unsigned int pin,
-				u16 strength)
+				u32 strength)
 {
 	struct ns2_pinctrl *pinctrl = pinctrl_dev_get_drvdata(pctrldev);
 	struct ns2_pin *pin_data = pctrldev->desc->pins[pin].drv_data;
@@ -904,7 +904,7 @@ static int ns2_pin_config_set(struct pinctrl_dev *pctrldev, unsigned int pin,
 	struct ns2_pin *pin_data = pctrldev->desc->pins[pin].drv_data;
 	enum pin_config_param param;
 	unsigned int i;
-	u16 arg;
+	u32 arg;
 	int ret = -ENOTSUPP;
 
 	if (pin_data->pin_conf.base == -1)
diff --git a/drivers/pinctrl/bcm/pinctrl-nsp-gpio.c b/drivers/pinctrl/bcm/pinctrl-nsp-gpio.c
index c8deb8be1da7..91ea32dc1e7f 100644
--- a/drivers/pinctrl/bcm/pinctrl-nsp-gpio.c
+++ b/drivers/pinctrl/bcm/pinctrl-nsp-gpio.c
@@ -366,7 +366,7 @@ static const struct pinctrl_ops nsp_pctrl_ops = {
 	.dt_free_map = pinctrl_utils_free_map,
 };
 
-static int nsp_gpio_set_slew(struct nsp_gpio *chip, unsigned gpio, u16 slew)
+static int nsp_gpio_set_slew(struct nsp_gpio *chip, unsigned gpio, u32 slew)
 {
 	if (slew)
 		nsp_set_bit(chip, IO_CTRL, NSP_GPIO_SLEW_RATE_EN, gpio, true);
@@ -403,7 +403,7 @@ static void nsp_gpio_get_pull(struct nsp_gpio *chip, unsigned gpio,
 }
 
 static int nsp_gpio_set_strength(struct nsp_gpio *chip, unsigned gpio,
-				 u16 strength)
+				 u32 strength)
 {
 	u32 offset, shift, i;
 	u32 val;
@@ -522,7 +522,7 @@ static int nsp_pin_config_set(struct pinctrl_dev *pctldev, unsigned pin,
 {
 	struct nsp_gpio *chip = pinctrl_dev_get_drvdata(pctldev);
 	enum pin_config_param param;
-	u16 arg;
+	u32 arg;
 	unsigned int i, gpio;
 	int ret = -ENOTSUPP;
 
diff --git a/drivers/pinctrl/intel/pinctrl-cherryview.c b/drivers/pinctrl/intel/pinctrl-cherryview.c
index 5e66860a5e67..f80134e3e0b6 100644
--- a/drivers/pinctrl/intel/pinctrl-cherryview.c
+++ b/drivers/pinctrl/intel/pinctrl-cherryview.c
@@ -1059,7 +1059,7 @@ static int chv_config_get(struct pinctrl_dev *pctldev, unsigned pin,
 }
 
 static int chv_config_set_pull(struct chv_pinctrl *pctrl, unsigned pin,
-			       enum pin_config_param param, u16 arg)
+			       enum pin_config_param param, u32 arg)
 {
 	void __iomem *reg = chv_padreg(pctrl, pin, CHV_PADCTRL0);
 	unsigned long flags;
@@ -1151,7 +1151,7 @@ static int chv_config_set(struct pinctrl_dev *pctldev, unsigned pin,
 	struct chv_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
 	enum pin_config_param param;
 	int i, ret;
-	u16 arg;
+	u32 arg;
 
 	if (chv_pad_locked(pctrl, pin))
 		return -EBUSY;
diff --git a/drivers/pinctrl/meson/pinctrl-meson.c b/drivers/pinctrl/meson/pinctrl-meson.c
index 620c231a2889..cf1686e04378 100644
--- a/drivers/pinctrl/meson/pinctrl-meson.c
+++ b/drivers/pinctrl/meson/pinctrl-meson.c
@@ -260,7 +260,6 @@ static int meson_pinconf_set(struct pinctrl_dev *pcdev, unsigned int pin,
 	enum pin_config_param param;
 	unsigned int reg, bit;
 	int i, ret;
-	u16 arg;
 
 	ret = meson_get_bank(pc, pin, &bank);
 	if (ret)
@@ -268,7 +267,6 @@ static int meson_pinconf_set(struct pinctrl_dev *pcdev, unsigned int pin,
 
 	for (i = 0; i < num_configs; i++) {
 		param = pinconf_to_config_param(configs[i]);
-		arg = pinconf_to_config_argument(configs[i]);
 
 		switch (param) {
 		case PIN_CONFIG_BIAS_DISABLE:
diff --git a/drivers/pinctrl/pinctrl-da850-pupd.c b/drivers/pinctrl/pinctrl-da850-pupd.c
index b36a90a3f3e4..44d5f5f5b07f 100644
--- a/drivers/pinctrl/pinctrl-da850-pupd.c
+++ b/drivers/pinctrl/pinctrl-da850-pupd.c
@@ -113,7 +113,6 @@ static int da850_pupd_pin_config_group_set(struct pinctrl_dev *pctldev,
 	struct da850_pupd_data *data = pinctrl_dev_get_drvdata(pctldev);
 	u32 ena, sel;
 	enum pin_config_param param;
-	u16 arg;
 	int i;
 
 	ena = readl(data->base + DA850_PUPD_ENA);
@@ -121,7 +120,6 @@ static int da850_pupd_pin_config_group_set(struct pinctrl_dev *pctldev,
 
 	for (i = 0; i < num_configs; i++) {
 		param = pinconf_to_config_param(configs[i]);
-		arg = pinconf_to_config_argument(configs[i]);
 
 		switch (param) {
 		case PIN_CONFIG_BIAS_DISABLE:
diff --git a/drivers/pinctrl/pinctrl-lpc18xx.c b/drivers/pinctrl/pinctrl-lpc18xx.c
index e053f1fa5512..d090f37ca4a1 100644
--- a/drivers/pinctrl/pinctrl-lpc18xx.c
+++ b/drivers/pinctrl/pinctrl-lpc18xx.c
@@ -904,7 +904,7 @@ static int lpc18xx_pconf_get(struct pinctrl_dev *pctldev, unsigned pin,
 
 static int lpc18xx_pconf_set_usb1(struct pinctrl_dev *pctldev,
 				  enum pin_config_param param,
-				  u16 param_val, u32 *reg)
+				  u32 param_val, u32 *reg)
 {
 	switch (param) {
 	case PIN_CONFIG_LOW_POWER_MODE:
@@ -932,7 +932,7 @@ static int lpc18xx_pconf_set_usb1(struct pinctrl_dev *pctldev,
 
 static int lpc18xx_pconf_set_i2c0(struct pinctrl_dev *pctldev,
 				  enum pin_config_param param,
-				  u16 param_val, u32 *reg,
+				  u32 param_val, u32 *reg,
 				  unsigned pin)
 {
 	u8 shift;
@@ -982,7 +982,7 @@ static int lpc18xx_pconf_set_i2c0(struct pinctrl_dev *pctldev,
 }
 
 static int lpc18xx_pconf_set_gpio_pin_int(struct pinctrl_dev *pctldev,
-					  u16 param_val, unsigned pin)
+					  u32 param_val, unsigned pin)
 {
 	struct lpc18xx_scu_data *scu = pinctrl_dev_get_drvdata(pctldev);
 	u32 val, reg_val, reg_offset = LPC18XX_SCU_PINTSEL0;
@@ -1008,7 +1008,7 @@ static int lpc18xx_pconf_set_gpio_pin_int(struct pinctrl_dev *pctldev,
 }
 
 static int lpc18xx_pconf_set_pin(struct pinctrl_dev *pctldev, unsigned param,
-				 u16 param_val, u32 *reg, unsigned pin,
+				 u32 param_val, u32 *reg, unsigned pin,
 				 struct lpc18xx_pin_caps *pin_cap)
 {
 	switch (param) {
@@ -1088,7 +1088,7 @@ static int lpc18xx_pconf_set(struct pinctrl_dev *pctldev, unsigned pin,
 	struct lpc18xx_scu_data *scu = pinctrl_dev_get_drvdata(pctldev);
 	struct lpc18xx_pin_caps *pin_cap;
 	enum pin_config_param param;
-	u16 param_val;
+	u32 param_val;
 	u32 reg;
 	int ret;
 	int i;
diff --git a/drivers/pinctrl/pinctrl-max77620.c b/drivers/pinctrl/pinctrl-max77620.c
index d9ff53e8f715..b8d2180a2bea 100644
--- a/drivers/pinctrl/pinctrl-max77620.c
+++ b/drivers/pinctrl/pinctrl-max77620.c
@@ -402,7 +402,7 @@ static int max77620_pinconf_set(struct pinctrl_dev *pctldev,
 	struct device *dev = mpci->dev;
 	struct max77620_fps_config *fps_config;
 	int param;
-	u16 param_val;
+	u32 param_val;
 	unsigned int val;
 	unsigned int pu_val;
 	unsigned int pd_val;
diff --git a/drivers/pinctrl/pinctrl-palmas.c b/drivers/pinctrl/pinctrl-palmas.c
index a30146da7ffd..4d6a5015b927 100644
--- a/drivers/pinctrl/pinctrl-palmas.c
+++ b/drivers/pinctrl/pinctrl-palmas.c
@@ -860,7 +860,7 @@ static int palmas_pinconf_set(struct pinctrl_dev *pctldev,
 {
 	struct palmas_pctrl_chip_info *pci = pinctrl_dev_get_drvdata(pctldev);
 	enum pin_config_param param;
-	u16 param_val;
+	u32 param_val;
 	const struct palmas_pingroup *g;
 	const struct palmas_pin_info *opt;
 	int ret;
diff --git a/drivers/pinctrl/pinctrl-rockchip.c b/drivers/pinctrl/pinctrl-rockchip.c
index 08765f58253c..7813599e43fa 100644
--- a/drivers/pinctrl/pinctrl-rockchip.c
+++ b/drivers/pinctrl/pinctrl-rockchip.c
@@ -1441,7 +1441,7 @@ static int rockchip_pinconf_set(struct pinctrl_dev *pctldev, unsigned int pin,
 	struct rockchip_pinctrl *info = pinctrl_dev_get_drvdata(pctldev);
 	struct rockchip_pin_bank *bank = pin_to_bank(info, pin);
 	enum pin_config_param param;
-	u16 arg;
+	u32 arg;
 	int i;
 	int rc;
 
diff --git a/drivers/pinctrl/pinctrl-single.c b/drivers/pinctrl/pinctrl-single.c
index a5a0392ab817..f71f2e813ea6 100644
--- a/drivers/pinctrl/pinctrl-single.c
+++ b/drivers/pinctrl/pinctrl-single.c
@@ -622,7 +622,7 @@ static int pcs_pinconf_set(struct pinctrl_dev *pctldev,
 	struct pcs_device *pcs = pinctrl_dev_get_drvdata(pctldev);
 	struct pcs_function *func;
 	unsigned offset = 0, shift = 0, i, data, ret;
-	u16 arg;
+	u32 arg;
 	int j;
 
 	ret = pcs_get_function(pctldev, pin, &func);
diff --git a/drivers/pinctrl/sirf/pinctrl-atlas7.c b/drivers/pinctrl/sirf/pinctrl-atlas7.c
index 7f3041697813..82b8a429743d 100644
--- a/drivers/pinctrl/sirf/pinctrl-atlas7.c
+++ b/drivers/pinctrl/sirf/pinctrl-atlas7.c
@@ -5322,7 +5322,8 @@ static int atlas7_pin_config_set(struct pinctrl_dev *pctldev,
 				unsigned pin, unsigned long *configs,
 				unsigned num_configs)
 {
-	u16 param, arg;
+	u16 param;
+	u32 arg;
 	int idx, err;
 
 	for (idx = 0; idx < num_configs; idx++) {
diff --git a/drivers/pinctrl/sunxi/pinctrl-sunxi.c b/drivers/pinctrl/sunxi/pinctrl-sunxi.c
index 0eb51e33cb1b..28bfa5f413e4 100644
--- a/drivers/pinctrl/sunxi/pinctrl-sunxi.c
+++ b/drivers/pinctrl/sunxi/pinctrl-sunxi.c
@@ -540,7 +540,7 @@ static int sunxi_pconf_group_set(struct pinctrl_dev *pctldev,
 		enum pin_config_param param;
 		unsigned long flags;
 		u32 offset, shift, mask, reg;
-		u16 arg, val;
+		u32 arg, val;
 		int ret;
 
 		param = pinconf_to_config_param(configs[i]);
diff --git a/drivers/pinctrl/uniphier/pinctrl-uniphier-core.c b/drivers/pinctrl/uniphier/pinctrl-uniphier-core.c
index 9b2ee717bccc..546f23c9040c 100644
--- a/drivers/pinctrl/uniphier/pinctrl-uniphier-core.c
+++ b/drivers/pinctrl/uniphier/pinctrl-uniphier-core.c
@@ -297,7 +297,7 @@ static int uniphier_conf_pin_config_get(struct pinctrl_dev *pctldev,
 
 static int uniphier_conf_pin_bias_set(struct pinctrl_dev *pctldev,
 				      const struct pin_desc *desc,
-				      enum pin_config_param param, u16 arg)
+				      enum pin_config_param param, u32 arg)
 {
 	struct uniphier_pinctrl_priv *priv = pinctrl_dev_get_drvdata(pctldev);
 	enum uniphier_pin_pull_dir pull_dir =
@@ -468,7 +468,7 @@ static int uniphier_conf_pin_config_set(struct pinctrl_dev *pctldev,
 	for (i = 0; i < num_configs; i++) {
 		enum pin_config_param param =
 					pinconf_to_config_param(configs[i]);
-		u16 arg = pinconf_to_config_argument(configs[i]);
+		u32 arg = pinconf_to_config_argument(configs[i]);
 
 		switch (param) {
 		case PIN_CONFIG_BIAS_DISABLE:
diff --git a/drivers/pinctrl/vt8500/pinctrl-wmt.c b/drivers/pinctrl/vt8500/pinctrl-wmt.c
index 270ca2a47a8c..c207e60b734f 100644
--- a/drivers/pinctrl/vt8500/pinctrl-wmt.c
+++ b/drivers/pinctrl/vt8500/pinctrl-wmt.c
@@ -428,7 +428,7 @@ static int wmt_pinconf_set(struct pinctrl_dev *pctldev, unsigned pin,
 {
 	struct wmt_pinctrl_data *data = pinctrl_dev_get_drvdata(pctldev);
 	enum pin_config_param param;
-	u16 arg;
+	u32 arg;
 	u32 bank = WMT_BANK_FROM_PIN(pin);
 	u32 bit = WMT_BIT_FROM_PIN(pin);
 	u32 reg_pull_en = data->banks[bank].reg_pull_en;
diff --git a/drivers/rtc/rtc-omap.c b/drivers/rtc/rtc-omap.c
index 51e52446eacb..73594f38c453 100644
--- a/drivers/rtc/rtc-omap.c
+++ b/drivers/rtc/rtc-omap.c
@@ -610,7 +610,7 @@ static int rtc_pinconf_set(struct pinctrl_dev *pctldev,
 	struct omap_rtc *rtc = pinctrl_dev_get_drvdata(pctldev);
 	u32 val;
 	unsigned int param;
-	u16 param_val;
+	u32 param_val;
 	int i;
 
 	rtc->type->unlock(rtc);
diff --git a/include/linux/pinctrl/pinconf-generic.h b/include/linux/pinctrl/pinconf-generic.h
index 12343caa114e..4962800234c4 100644
--- a/include/linux/pinctrl/pinconf-generic.h
+++ b/include/linux/pinctrl/pinconf-generic.h
@@ -92,6 +92,8 @@
  * @PIN_CONFIG_END: this is the last enumerator for pin configurations, if
  *	you need to pass in custom configurations to the pin controller, use
  *	PIN_CONFIG_END+1 as the base offset.
+ * @PIN_CONFIG_MAX: this is the maximum configuration value that can be
+ *	presented using the packed format.
  */
 enum pin_config_param {
 	PIN_CONFIG_BIAS_BUS_HOLD,
@@ -112,7 +114,8 @@ enum pin_config_param {
 	PIN_CONFIG_OUTPUT,
 	PIN_CONFIG_POWER_SOURCE,
 	PIN_CONFIG_SLEW_RATE,
-	PIN_CONFIG_END = 0x7FFF,
+	PIN_CONFIG_END = 0x7F,
+	PIN_CONFIG_MAX = 0xFF,
 };
 
 #ifdef CONFIG_DEBUG_FS
@@ -130,27 +133,27 @@ struct pin_config_item {
 /*
  * Helpful configuration macro to be used in tables etc.
  */
-#define PIN_CONF_PACKED(p, a) ((a << 16) | ((unsigned long) p & 0xffffUL))
+#define PIN_CONF_PACKED(p, a) ((a << 8) | ((unsigned long) p & 0xffUL))
 
 /*
  * The following inlines stuffs a configuration parameter and data value
  * into and out of an unsigned long argument, as used by the generic pin config
- * system. We put the parameter in the lower 16 bits and the argument in the
- * upper 16 bits.
+ * system. We put the parameter in the lower 8 bits and the argument in the
+ * upper 24 bits.
  */
 
 static inline enum pin_config_param pinconf_to_config_param(unsigned long config)
 {
-	return (enum pin_config_param) (config & 0xffffUL);
+	return (enum pin_config_param) (config & 0xffUL);
 }
 
-static inline u16 pinconf_to_config_argument(unsigned long config)
+static inline u32 pinconf_to_config_argument(unsigned long config)
 {
-	return (enum pin_config_param) ((config >> 16) & 0xffffUL);
+	return (enum pin_config_param) ((config >> 8) & 0xffffffUL);
 }
 
 static inline unsigned long pinconf_to_config_packed(enum pin_config_param param,
-						     u16 argument)
+						     u32 argument)
 {
 	return PIN_CONF_PACKED(param, argument);
 }
-- 
2.11.0

[toc] | [prev] | [next] | [standalone]

#1562658 — Re: [PATCH 1/3] pinctrl: Widen the generic pinconf argument from 16 to 24 bits

On Thu, 2017-01-19 at 12:48 +0300, Mika Westerberg wrote:
> The current pinconf packed format allows only 16-bit argument limiting
> the maximum value 65535. For most types this is enough. However,
> debounce time can be in range of hundreths of milliseconds in case of
> mechanical switches so we cannot represent the worst case using the
> current format.
> 


>  static inline enum pin_config_param pinconf_to_config_param(unsigned
> long config)
>  {
> -	return (enum pin_config_param) (config & 0xffffUL);
> +	return (enum pin_config_param) (config & 0xffUL);
>  }
>  
> -static inline u16 pinconf_to_config_argument(unsigned long config)
> +static inline u32 pinconf_to_config_argument(unsigned long config)
>  {
> -	return (enum pin_config_param) ((config >> 16) & 0xffffUL);
> +	return (enum pin_config_param) ((config >> 8) & 0xffffffUL);

Looks like copy'n'paste error in the initial code, I mean casting should
go as (u16) -> (u32).

Perhaps, in case you submit new version, do the first patch with a fix
of this one. Then conversion would look more logical.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

[toc] | [prev] | [next] | [standalone]

#1563394 — Re: [PATCH 1/3] pinctrl: Widen the generic pinconf argument from 16 to 24 bits

On Thu, Jan 19, 2017 at 10:48 AM, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:

> The current pinconf packed format allows only 16-bit argument limiting
> the maximum value 65535. For most types this is enough. However,
> debounce time can be in range of hundreths of milliseconds in case of
> mechanical switches so we cannot represent the worst case using the
> current format.
>
> In order to support larger values change the packed format so that the
> lower 8 bits are used as type which leaves 24 bits for the argument.
> This allows representing values up to 16777215 and debounce times up to
> 16 seconds.
>
> We also convert the existing users to use 32-bit integer when extracting
> argument from the packed configuration value.
>
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>

I like the looks of this :)

Look into Andy's comment and whatever else appears, then let's
apply the v2 of this.

Yours,
Linus Walleij

[toc] | [prev] | [next] | [standalone]

#1562669 — Re: [PATCH 0/3] pinctrl / gpio: Allow GPIO chips to use generic pinconfig

On Thu, 2017-01-19 at 12:48 +0300, Mika Westerberg wrote:
> This series makes it possible to configure pins from GPIO chip drivers
> by
> implementing a new callback .set_config(). This callback replaces the
> existing .set_single_ended() and .set_debounce() simply because adding
> new
> callbacks for each possible configuration type does not scale. So
> instead
> we re-use the existing generic pinconf types and the packed format.
> 
> This is a follow up of discussion on:
> 
>   https://patchwork.ozlabs.org/patch/713289/
> 
> While doing that, it was found out that the current packed format does
> not
> support all realistic debounce time values. The limit is ~64ms which
> does
> not cover mechanical switches connected to GPIOs that migh require
> values
> up to hundreths of milliseconds.
> 
> To solve that we change the packed format so that the value takes 24
> bits
> instead of 16, and change the callers to use 32-bit integer instead.
> 
> We also make it possible for GPIO chip driver to call pinctrl directly
> by
> providing a new function pinctrl_gpio_set_config() following
> pinctrl_gpio_direction_output() and friends.
> 
> I've tested this on Intel Gemini Lake SoC. Non-Intel drivers are
> compile
> tested only because I do not have the hardware.
> 

Some minor comments, and take my

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>


> Mika Westerberg (3):
>   pinctrl: Widen the generic pinconf argument from 16 to 24 bits
>   pinctrl: Allow configuration of pins from gpiolib based drivers
>   pinctrl / gpio: Introduce .set_config() callback for GPIO chips
> 
>  Documentation/gpio/driver.txt                    |  9 ++--
>  drivers/gpio/gpio-bcm-kona.c                     | 14 +++++-
>  drivers/gpio/gpio-dln2.c                         | 12 ++++--
>  drivers/gpio/gpio-dwapb.c                        | 14 +++++-
>  drivers/gpio/gpio-ep93xx.c                       | 11 +++--
>  drivers/gpio/gpio-f7188x.c                       | 19 ++++----
>  drivers/gpio/gpio-lp873x.c                       | 14 +++---
>  drivers/gpio/gpio-max77620.c                     | 20 ++++-----
>  drivers/gpio/gpio-menz127.c                      | 34 +++++++++++----
>  drivers/gpio/gpio-merrifield.c                   | 14 +++++-
>  drivers/gpio/gpio-omap.c                         | 14 +++++-
>  drivers/gpio/gpio-tc3589x.c                      | 15 +++----
>  drivers/gpio/gpio-tegra.c                        | 14 +++++-
>  drivers/gpio/gpio-tps65218.c                     | 14 +++---
>  drivers/gpio/gpio-vx855.c                        | 13 +++---
>  drivers/gpio/gpio-wcove.c                        | 13 +++---
>  drivers/gpio/gpio-wm831x.c                       | 21 +++++----
>  drivers/gpio/gpio-wm8994.c                       | 13 +++---
>  drivers/gpio/gpiolib.c                           | 41 +++++++++----
> -----
>  drivers/pinctrl/bcm/pinctrl-bcm281xx.c           |  6 +--
>  drivers/pinctrl/bcm/pinctrl-iproc-gpio.c         |  2 +-
>  drivers/pinctrl/bcm/pinctrl-ns2-mux.c            |  6 +--
>  drivers/pinctrl/bcm/pinctrl-nsp-gpio.c           |  6 +--
>  drivers/pinctrl/core.c                           | 28 ++++++++++++
>  drivers/pinctrl/intel/pinctrl-cherryview.c       |  4 +-
>  drivers/pinctrl/mediatek/pinctrl-mtk-common.c    | 14 +++++-
>  drivers/pinctrl/meson/pinctrl-meson.c            |  2 -
>  drivers/pinctrl/pinconf.c                        | 12 ++++++
>  drivers/pinctrl/pinconf.h                        |  9 ++++
>  drivers/pinctrl/pinctrl-amd.c                    | 14 +++++-
>  drivers/pinctrl/pinctrl-da850-pupd.c             |  2 -
>  drivers/pinctrl/pinctrl-lpc18xx.c                | 10 ++---
>  drivers/pinctrl/pinctrl-max77620.c               |  2 +-
>  drivers/pinctrl/pinctrl-palmas.c                 |  2 +-
>  drivers/pinctrl/pinctrl-rockchip.c               |  2 +-
>  drivers/pinctrl/pinctrl-single.c                 |  2 +-
>  drivers/pinctrl/pinctrl-sx150x.c                 | 55 ++++++++-------
> ---------
>  drivers/pinctrl/sirf/pinctrl-atlas7.c            |  3 +-
>  drivers/pinctrl/sunxi/pinctrl-sunxi.c            |  2 +-
>  drivers/pinctrl/uniphier/pinctrl-uniphier-core.c |  4 +-
>  drivers/pinctrl/vt8500/pinctrl-wmt.c             |  2 +-
>  drivers/rtc/rtc-omap.c                           |  2 +-
>  drivers/staging/greybus/gpio.c                   | 15 ++++---
>  drivers/usb/serial/cp210x.c                      | 13 +++---
>  include/linux/gpio/driver.h                      | 35 +++------------
>  include/linux/pinctrl/consumer.h                 |  6 +++
>  include/linux/pinctrl/pinconf-generic.h          | 51 +++++++++++--
> ---------
>  47 files changed, 376 insertions(+), 254 deletions(-)
> 

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

[toc] | [prev] | [next] | [standalone]

#1562671 — Re: [PATCH 3/3] pinctrl / gpio: Introduce .set_config() callback for GPIO chips

On Thu, 2017-01-19 at 12:48 +0300, Mika Westerberg wrote:
> Currently we already have two pin configuration related callbacks
> available for GPIO chips .set_single_ended() and .set_debounce(). In
> future we expect to have even more, which does not scale well if we
> need
> to add yet another callback to the GPIO chip structure for each
> possible
> configuration parameter.
> 
> Better solution is to reuse what we already have available in the
> generic pinconf.
> 
> To support this, we introduce a new .set_config() callback for GPIO
> chips. The callback takes a single packed pin configuration value as
> parameter. This can then be extended easily beyond what is currently
> supported by just adding new types to the generic pinconf enum.
> 
> We then convert the existing drivers over .set_config() and finally
> remove the .set_single_ended() and .set_debounce() callbacks.
> 
 
> +#ifdef CONFIG_GENERIC_PINCONF
> +
> +#ifdef CONFIG_DEBUG_FS
> +#define PCONFDUMP(a, b, c, d) { .param = a, .display = b, .format =
> c, \
> +				.has_arg = d }

I understand you moved existing code, though 3 liner here would look
better.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

[toc] | [prev] | [next] | [standalone]

#1563405 — Re: [PATCH 3/3] pinctrl / gpio: Introduce .set_config() callback for GPIO chips

On Thu, Jan 19, 2017 at 10:48 AM, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:

> Currently we already have two pin configuration related callbacks
> available for GPIO chips .set_single_ended() and .set_debounce(). In
> future we expect to have even more, which does not scale well if we need
> to add yet another callback to the GPIO chip structure for each possible
> configuration parameter.
>
> Better solution is to reuse what we already have available in the
> generic pinconf.
>
> To support this, we introduce a new .set_config() callback for GPIO
> chips. The callback takes a single packed pin configuration value as
> parameter. This can then be extended easily beyond what is currently
> supported by just adding new types to the generic pinconf enum.
>
> We then convert the existing drivers over .set_config() and finally
> remove the .set_single_ended() and .set_debounce() callbacks.
>
> Suggested-by: Linus Walleij <linus.walleij@linaro.org>
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Yes!!! This is exactly how this should be done.

Can't wait to apply the final version of this.

> +static int gpio_set_drive_mode(struct gpio_chip *gc, unsigned offset,
> +                              enum pin_config_param mode)
> +{
> +       unsigned long config = { PIN_CONF_PACKED(mode, 0) };
> +
> +       return gc->set_config ? gc->set_config(gc, offset, config) : -ENOTSUPP;
> +}

I would name it gpio_set_drive_single_ended() as the open source/open
drain is all we support here.

>         if (test_bit(FLAG_OPEN_DRAIN, &desc->flags)) {
>                 /* First see if we can enable open drain in hardware */
> -               if (gc->set_single_ended) {
> -                       ret = gc->set_single_ended(gc, gpio_chip_hwgpio(desc),
> -                                                  LINE_MODE_OPEN_DRAIN);
> -                       if (!ret)
> -                               goto set_output_value;
> -               }
> +               ret = gpio_set_drive_mode(gc, gpio_chip_hwgpio(desc),
> +                                         PIN_CONFIG_DRIVE_OPEN_DRAIN);
> +               if (!ret)
> +                       goto set_output_value;

Aha I see, so if we fail to set single ended we get to the next step.
Nice.

>                 /* Emulate open drain by not actively driving the line high */
>                 if (val)
>                         return gpiod_direction_input(desc);
>         }

(...)

> -               if (gc->set_single_ended) {
> -                       ret = gc->set_single_ended(gc, gpio_chip_hwgpio(desc),
> -                                                  LINE_MODE_OPEN_SOURCE);
> -                       if (!ret)
> -                               goto set_output_value;
> -               }
> +               ret = gpio_set_drive_mode(gc, gpio_chip_hwgpio(desc),
> +                                         PIN_CONFIG_DRIVE_OPEN_SOURCE);
>                 /* Emulate open source by not actively driving the line low */
>                 if (!val)
>                         return gpiod_direction_input(desc);

But here the handling seems to be wrong? You still need
the if (!ret) goto set_output_value?

>         } else {
> -               /* Make sure to disable open drain/source hardware, if any */
> -               if (gc->set_single_ended)
> -                       gc->set_single_ended(gc,
> -                                            gpio_chip_hwgpio(desc),
> -                                            LINE_MODE_PUSH_PULL);
> +               gpio_set_drive_mode(gc, gpio_chip_hwgpio(desc),
> +                                   PIN_CONFIG_DRIVE_PUSH_PULL);

Nice.

> -static int sx150x_gpio_set_single_ended(struct gpio_chip *chip,
> -                                       unsigned int offset,
> -                                       enum single_ended_mode mode)
> +static int sx150x_gpio_set_config(struct gpio_chip *chip, unsigned int offset,
> +                                 unsigned long config)
>  {
> -       struct sx150x_pinctrl *pctl = gpiochip_get_data(chip);
> -       int ret;
> -
> -       switch (mode) {
> -       case LINE_MODE_PUSH_PULL:
> -               if (pctl->data->model != SX150X_789 ||
> -                   sx150x_pin_is_oscio(pctl, offset))
> -                       return 0;
> -
> -               ret = regmap_write_bits(pctl->regmap,
> -                                       pctl->data->pri.x789.reg_drain,
> -                                       BIT(offset), 0);
> -               break;
> -
> -       case LINE_MODE_OPEN_DRAIN:
> -               if (pctl->data->model != SX150X_789 ||
> -                   sx150x_pin_is_oscio(pctl, offset))
> -                       return -ENOTSUPP;
> -
> -               ret = regmap_write_bits(pctl->regmap,
> -                                       pctl->data->pri.x789.reg_drain,
> -                                       BIT(offset), BIT(offset));
> -               break;
> -       default:
> -               ret = -ENOTSUPP;
> -               break;
> -       }
> -
> -       return ret;
> +       return pinctrl_gpio_set_config(chip->base + offset, config);
>  }

This is the beauty of it all..

> @@ -811,16 +782,26 @@ static int sx150x_pinconf_set(struct pinctrl_dev *pctldev, unsigned int pin,
>                         break;
>
>                 case PIN_CONFIG_DRIVE_OPEN_DRAIN:
> -                       ret = sx150x_gpio_set_single_ended(&pctl->gpio,
> -                                               pin, LINE_MODE_OPEN_DRAIN);
> +                       if (pctl->data->model != SX150X_789 ||
> +                           sx150x_pin_is_oscio(pctl, pin))
> +                               return -ENOTSUPP;
> +
> +                       ret = regmap_write_bits(pctl->regmap,
> +                                               pctl->data->pri.x789.reg_drain,
> +                                               BIT(pin), BIT(pin));
>                         if (ret < 0)
>                                 return ret;
>
>                         break;
>
>                 case PIN_CONFIG_DRIVE_PUSH_PULL:
> -                       ret = sx150x_gpio_set_single_ended(&pctl->gpio,
> -                                               pin, LINE_MODE_PUSH_PULL);
> +                       if (pctl->data->model != SX150X_789 ||
> +                           sx150x_pin_is_oscio(pctl, pin))
> +                               return 0;
> +
> +                       ret = regmap_write_bits(pctl->regmap,
> +                                               pctl->data->pri.x789.reg_drain,
> +                                               BIT(pin), 0);
>                         if (ret < 0)
>                                 return ret;
>
> @@ -1178,7 +1159,7 @@ static int sx150x_probe(struct i2c_client *client,
>         pctl->gpio.direction_output = sx150x_gpio_direction_output;
>         pctl->gpio.get = sx150x_gpio_get;
>         pctl->gpio.set = sx150x_gpio_set;
> -       pctl->gpio.set_single_ended = sx150x_gpio_set_single_ended;
> +       pctl->gpio.set_config = sx150x_gpio_set_config;

Would be nice if some SX150x person could look at this but it seems just
right to me.

> diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h
> index c2748accea71..2939c1d84add 100644
> --- a/include/linux/gpio/driver.h
> +++ b/include/linux/gpio/driver.h
> @@ -8,6 +8,7 @@
>  #include <linux/irqdomain.h>
>  #include <linux/lockdep.h>
>  #include <linux/pinctrl/pinctrl.h>
> +#include <linux/pinctrl/pinconf-generic.h>

Yups there is lands...

Thanks, I'm impressed.

Hope I'll be able to apply v2.

Yours,
Linus Walleij

[toc] | [prev] | [next] | [standalone]

#1563421 — Re: [PATCH 3/3] pinctrl / gpio: Introduce .set_config() callback for GPIO chips

On 01/20/2017 10:13 AM, Linus Walleij wrote:
> On Thu, Jan 19, 2017 at 10:48 AM, Mika Westerberg
> <mika.westerberg@linux.intel.com> wrote:
> 
>> Currently we already have two pin configuration related callbacks
>> available for GPIO chips .set_single_ended() and .set_debounce(). In
>> future we expect to have even more, which does not scale well if we need
>> to add yet another callback to the GPIO chip structure for each possible
>> configuration parameter.
>>
>> Better solution is to reuse what we already have available in the
>> generic pinconf.
>>
>> To support this, we introduce a new .set_config() callback for GPIO
>> chips. The callback takes a single packed pin configuration value as
>> parameter. This can then be extended easily beyond what is currently
>> supported by just adding new types to the generic pinconf enum.
>>
>> We then convert the existing drivers over .set_config() and finally
>> remove the .set_single_ended() and .set_debounce() callbacks.
>>
>> Suggested-by: Linus Walleij <linus.walleij@linaro.org>
>> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> 
> Yes!!! This is exactly how this should be done.
> 
> Can't wait to apply the final version of this.
> 
>> +static int gpio_set_drive_mode(struct gpio_chip *gc, unsigned offset,
>> +                              enum pin_config_param mode)
>> +{
>> +       unsigned long config = { PIN_CONF_PACKED(mode, 0) };
>> +
>> +       return gc->set_config ? gc->set_config(gc, offset, config) : -ENOTSUPP;
>> +}
> 
> I would name it gpio_set_drive_single_ended() as the open source/open
> drain is all we support here.
> 
>>         if (test_bit(FLAG_OPEN_DRAIN, &desc->flags)) {
>>                 /* First see if we can enable open drain in hardware */
>> -               if (gc->set_single_ended) {
>> -                       ret = gc->set_single_ended(gc, gpio_chip_hwgpio(desc),
>> -                                                  LINE_MODE_OPEN_DRAIN);
>> -                       if (!ret)
>> -                               goto set_output_value;
>> -               }
>> +               ret = gpio_set_drive_mode(gc, gpio_chip_hwgpio(desc),
>> +                                         PIN_CONFIG_DRIVE_OPEN_DRAIN);
>> +               if (!ret)
>> +                       goto set_output_value;
> 
> Aha I see, so if we fail to set single ended we get to the next step.
> Nice.
> 
>>                 /* Emulate open drain by not actively driving the line high */
>>                 if (val)
>>                         return gpiod_direction_input(desc);
>>         }
> 
> (...)
> 
>> -               if (gc->set_single_ended) {
>> -                       ret = gc->set_single_ended(gc, gpio_chip_hwgpio(desc),
>> -                                                  LINE_MODE_OPEN_SOURCE);
>> -                       if (!ret)
>> -                               goto set_output_value;
>> -               }
>> +               ret = gpio_set_drive_mode(gc, gpio_chip_hwgpio(desc),
>> +                                         PIN_CONFIG_DRIVE_OPEN_SOURCE);
>>                 /* Emulate open source by not actively driving the line low */
>>                 if (!val)
>>                         return gpiod_direction_input(desc);
> 
> But here the handling seems to be wrong? You still need
> the if (!ret) goto set_output_value?
> 
>>         } else {
>> -               /* Make sure to disable open drain/source hardware, if any */
>> -               if (gc->set_single_ended)
>> -                       gc->set_single_ended(gc,
>> -                                            gpio_chip_hwgpio(desc),
>> -                                            LINE_MODE_PUSH_PULL);
>> +               gpio_set_drive_mode(gc, gpio_chip_hwgpio(desc),
>> +                                   PIN_CONFIG_DRIVE_PUSH_PULL);
> 
> Nice.
> 
>> -static int sx150x_gpio_set_single_ended(struct gpio_chip *chip,
>> -                                       unsigned int offset,
>> -                                       enum single_ended_mode mode)
>> +static int sx150x_gpio_set_config(struct gpio_chip *chip, unsigned int offset,
>> +                                 unsigned long config)
>>  {
>> -       struct sx150x_pinctrl *pctl = gpiochip_get_data(chip);
>> -       int ret;
>> -
>> -       switch (mode) {
>> -       case LINE_MODE_PUSH_PULL:
>> -               if (pctl->data->model != SX150X_789 ||
>> -                   sx150x_pin_is_oscio(pctl, offset))
>> -                       return 0;
>> -
>> -               ret = regmap_write_bits(pctl->regmap,
>> -                                       pctl->data->pri.x789.reg_drain,
>> -                                       BIT(offset), 0);
>> -               break;
>> -
>> -       case LINE_MODE_OPEN_DRAIN:
>> -               if (pctl->data->model != SX150X_789 ||
>> -                   sx150x_pin_is_oscio(pctl, offset))
>> -                       return -ENOTSUPP;
>> -
>> -               ret = regmap_write_bits(pctl->regmap,
>> -                                       pctl->data->pri.x789.reg_drain,
>> -                                       BIT(offset), BIT(offset));
>> -               break;
>> -       default:
>> -               ret = -ENOTSUPP;
>> -               break;
>> -       }
>> -
>> -       return ret;
>> +       return pinctrl_gpio_set_config(chip->base + offset, config);
>>  }
> 
> This is the beauty of it all..

It would be even cooler it this becomes a generic helper !

> 
>> @@ -811,16 +782,26 @@ static int sx150x_pinconf_set(struct pinctrl_dev *pctldev, unsigned int pin,
>>                         break;
>>
>>                 case PIN_CONFIG_DRIVE_OPEN_DRAIN:
>> -                       ret = sx150x_gpio_set_single_ended(&pctl->gpio,
>> -                                               pin, LINE_MODE_OPEN_DRAIN);
>> +                       if (pctl->data->model != SX150X_789 ||
>> +                           sx150x_pin_is_oscio(pctl, pin))
>> +                               return -ENOTSUPP;
>> +
>> +                       ret = regmap_write_bits(pctl->regmap,
>> +                                               pctl->data->pri.x789.reg_drain,
>> +                                               BIT(pin), BIT(pin));
>>                         if (ret < 0)
>>                                 return ret;
>>
>>                         break;
>>
>>                 case PIN_CONFIG_DRIVE_PUSH_PULL:
>> -                       ret = sx150x_gpio_set_single_ended(&pctl->gpio,
>> -                                               pin, LINE_MODE_PUSH_PULL);
>> +                       if (pctl->data->model != SX150X_789 ||
>> +                           sx150x_pin_is_oscio(pctl, pin))
>> +                               return 0;
>> +
>> +                       ret = regmap_write_bits(pctl->regmap,
>> +                                               pctl->data->pri.x789.reg_drain,
>> +                                               BIT(pin), 0);
>>                         if (ret < 0)
>>                                 return ret;
>>
>> @@ -1178,7 +1159,7 @@ static int sx150x_probe(struct i2c_client *client,
>>         pctl->gpio.direction_output = sx150x_gpio_direction_output;
>>         pctl->gpio.get = sx150x_gpio_get;
>>         pctl->gpio.set = sx150x_gpio_set;
>> -       pctl->gpio.set_single_ended = sx150x_gpio_set_single_ended;
>> +       pctl->gpio.set_config = sx150x_gpio_set_config;
> 
> Would be nice if some SX150x person could look at this but it seems just
> right to me.

Seems good, please CC us in v2 so we can add a Tested-by.

Thanks,
Neil

> 
>> diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h
>> index c2748accea71..2939c1d84add 100644
>> --- a/include/linux/gpio/driver.h
>> +++ b/include/linux/gpio/driver.h
>> @@ -8,6 +8,7 @@
>>  #include <linux/irqdomain.h>
>>  #include <linux/lockdep.h>
>>  #include <linux/pinctrl/pinctrl.h>
>> +#include <linux/pinctrl/pinconf-generic.h>
> 
> Yups there is lands...
> 
> Thanks, I'm impressed.
> 
> Hope I'll be able to apply v2.
> 
> Yours,
> Linus Walleij
> 

[toc] | [prev] | [next] | [standalone]

#1563427 — Re: [PATCH 3/3] pinctrl / gpio: Introduce .set_config() callback for GPIO chips

On Fri, Jan 20, 2017 at 10:24 AM, Neil Armstrong
<narmstrong@baylibre.com> wrote:

>>> -       return ret;
>>> +       return pinctrl_gpio_set_config(chip->base + offset, config);
>>>  }
>>
>> This is the beauty of it all..
>
> It would be even cooler it this becomes a generic helper !

It does, with this series we can push debounce and open source/drain
back to the pinctrl back-end.

Then the road is open to push any config this way, which I think will
also be needed for userspace GPIO to set up not just this but also
pull-ups etc. But we will add that step by step.

This provides a piece of much-needed infrastructure for this.

Yours,
Linus Walleij

[toc] | [prev] | [next] | [standalone]

#1563582 — Re: [PATCH 3/3] pinctrl / gpio: Introduce .set_config() callback for GPIO chips

On Fri, Jan 20, 2017 at 10:13:05AM +0100, Linus Walleij wrote:
> > +static int gpio_set_drive_mode(struct gpio_chip *gc, unsigned offset,
> > +                              enum pin_config_param mode)
> > +{
> > +       unsigned long config = { PIN_CONF_PACKED(mode, 0) };
> > +
> > +       return gc->set_config ? gc->set_config(gc, offset, config) : -ENOTSUPP;
> > +}
> 
> I would name it gpio_set_drive_single_ended() as the open source/open
> drain is all we support here.

OK.

> >         if (test_bit(FLAG_OPEN_DRAIN, &desc->flags)) {
> >                 /* First see if we can enable open drain in hardware */
> > -               if (gc->set_single_ended) {
> > -                       ret = gc->set_single_ended(gc, gpio_chip_hwgpio(desc),
> > -                                                  LINE_MODE_OPEN_DRAIN);
> > -                       if (!ret)
> > -                               goto set_output_value;
> > -               }
> > +               ret = gpio_set_drive_mode(gc, gpio_chip_hwgpio(desc),
> > +                                         PIN_CONFIG_DRIVE_OPEN_DRAIN);
> > +               if (!ret)
> > +                       goto set_output_value;
> 
> Aha I see, so if we fail to set single ended we get to the next step.
> Nice.
> 
> >                 /* Emulate open drain by not actively driving the line high */
> >                 if (val)
> >                         return gpiod_direction_input(desc);
> >         }
> 
> (...)
> 
> > -               if (gc->set_single_ended) {
> > -                       ret = gc->set_single_ended(gc, gpio_chip_hwgpio(desc),
> > -                                                  LINE_MODE_OPEN_SOURCE);
> > -                       if (!ret)
> > -                               goto set_output_value;
> > -               }
> > +               ret = gpio_set_drive_mode(gc, gpio_chip_hwgpio(desc),
> > +                                         PIN_CONFIG_DRIVE_OPEN_SOURCE);
> >                 /* Emulate open source by not actively driving the line low */
> >                 if (!val)
> >                         return gpiod_direction_input(desc);
> 
> But here the handling seems to be wrong? You still need
> the if (!ret) goto set_output_value?

Good point. I'll fix that in the next version.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web