[PATCH 0/5] pinctrl_gpio_get_direction & ingenic fixes

[PATCH 0/5] pinctrl_gpio_get_direction & ingenic fixes

[Paul Cercueil]

Hi Linus,

Here's a set of (rather RFC) patches, to implement
pinctrl_gpio_get_direction(). I did that, because my gpio-ingenic driver
calls pinctrl_gpio_set_direction() within its gpio_chip's .set_direction
callback, but there was no corresponding function to implement the
.get_direction callback. If that's not the right way to do it, please
advise.

If not merging the whole series, patch [3/5] is a real fix that should
go through.

Note that it doesn't make checkpatch.pl happy, I wasn't sure whether I
should try to comply to checkpatch.pl or match the coding style in the
pinctrl subsystem, I chose the latter.

Thanks,
-Paul

[PATCH 1/5] pinctrl: Add core function pinmux_gpio_get_direction

[Paul Cercueil]

Right now there is a core function to set the direction (input or output)
of a GPIO, pinmux_gpio_set_direction(). This function is provided to the
consumers with two wrappers, pinctrl_gpio_direction_input() /
pinctrl_gpio_direction_output(). Typically, these functions are used
within GPIO drivers, where the .direction_input/.direction_output
callbacks of the "gpio_chip" simply call into these pinctrl functions.

However, until now there was no corresponding pinmux_get_direction()
core function (nor pinctrl_gpio_get_direction), which means that it was
not possible to call into the pinctrl subsystem for the .get_direction
callback of the "gpio_chip".

Here, we introduce the pinmux_gpio_get_direction() core function, as
well as the .gpio_get_direction callback in the pinmux_ops structure.
Now it's up to the pinctrl drivers to implement that callback.

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---
 drivers/pinctrl/pinmux.c       | 17 +++++++++++++++++
 drivers/pinctrl/pinmux.h       |  3 +++
 include/linux/pinctrl/pinmux.h |  5 +++++
 3 files changed, 25 insertions(+)

diff --git a/drivers/pinctrl/pinmux.c b/drivers/pinctrl/pinmux.c
index b8e9bda8ec98..e502a4f5be6f 100644
--- a/drivers/pinctrl/pinmux.c
+++ b/drivers/pinctrl/pinmux.c
@@ -291,6 +291,23 @@ int pinmux_gpio_direction(struct pinctrl_dev *pctldev,
 	return ret;
 }
 
+int pinmux_gpio_get_direction(struct pinctrl_dev *pctldev,
+			      struct pinctrl_gpio_range *range,
+			      unsigned int pin)
+{
+	const struct pinmux_ops *ops;
+	int ret;
+
+	ops = pctldev->desc->pmxops;
+
+	if (ops->gpio_get_direction)
+		ret = ops->gpio_get_direction(pctldev, range, pin);
+	else
+		ret = -ENXIO;
+
+	return ret;
+}
+
 static int pinmux_func_name_to_selector(struct pinctrl_dev *pctldev,
 					const char *function)
 {
diff --git a/drivers/pinctrl/pinmux.h b/drivers/pinctrl/pinmux.h
index a331fcdbedd9..8cdab9e71870 100644
--- a/drivers/pinctrl/pinmux.h
+++ b/drivers/pinctrl/pinmux.h
@@ -24,6 +24,9 @@ void pinmux_free_gpio(struct pinctrl_dev *pctldev, unsigned pin,
 int pinmux_gpio_direction(struct pinctrl_dev *pctldev,
 			  struct pinctrl_gpio_range *range,
 			  unsigned pin, bool input);
+int pinmux_gpio_get_direction(struct pinctrl_dev *pctldev,
+			  struct pinctrl_gpio_range *range,
+			  unsigned pin);
 
 int pinmux_map_to_setting(const struct pinctrl_map *map,
 			  struct pinctrl_setting *setting);
diff --git a/include/linux/pinctrl/pinmux.h b/include/linux/pinctrl/pinmux.h
index ace60d775b20..b3e73deffb49 100644
--- a/include/linux/pinctrl/pinmux.h
+++ b/include/linux/pinctrl/pinmux.h
@@ -56,6 +56,8 @@ struct pinctrl_dev;
  *	depending on whether the GPIO is configured as input or output,
  *	a direction selector function may be implemented as a backing
  *	to the GPIO controllers that need pin muxing.
+ * @gpio_get_direction: Return the direction (input or output) of the given
+ *	GPIO.
  * @strict: do not allow simultaneous use of the same pin for GPIO and another
  *	function. Check both gpio_owner and mux_owner strictly before approving
  *	the pin request.
@@ -82,6 +84,9 @@ struct pinmux_ops {
 				   struct pinctrl_gpio_range *range,
 				   unsigned offset,
 				   bool input);
+	int (*gpio_get_direction) (struct pinctrl_dev *pctrldev,
+				   struct pinctrl_gpio_range *range,
+				   unsigned int offset);
 	bool strict;
 };
 
-- 
2.11.0

[PATCH 2/5] pinctrl: Add API function pinctrl_gpio_get_direction

[Paul Cercueil]

This function can be used to retrieve the direction (input/output) of a
given GPIO.

It should *ONLY* be used from gpiolib-based GPIO drivers, as part of their
gpio_get_direction() semantics, platforms and individual drivers shall
*NOT* touch pin control GPIO calls.

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---
 drivers/pinctrl/core.c           | 31 +++++++++++++++++++++++++++++++
 include/linux/pinctrl/consumer.h |  1 +
 2 files changed, 32 insertions(+)

diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
index e5a303002021..7fee8347fb51 100644
--- a/drivers/pinctrl/core.c
+++ b/drivers/pinctrl/core.c
@@ -849,6 +849,37 @@ int pinctrl_gpio_direction_output(unsigned gpio)
 EXPORT_SYMBOL_GPL(pinctrl_gpio_direction_output);
 
 /**
+ * pinctrl_gpio_get_direction() - Get the direction (input/output) of a GPIO pin
+ * @gpio: the GPIO pin number from the GPIO subsystem number space
+ *
+ * This function should *ONLY* be used from gpiolib-based GPIO drivers,
+ * as part of their gpio_get_direction() semantics, platforms and individual
+ * drivers shall *NOT* touch pin control GPIO calls.
+ */
+int pinctrl_gpio_get_direction(unsigned int gpio)
+{
+	struct pinctrl_dev *pctldev;
+	struct pinctrl_gpio_range *range;
+	int ret;
+	int pin;
+
+	ret = pinctrl_get_device_gpio_range(gpio, &pctldev, &range);
+	if (ret)
+		return ret;
+
+	mutex_lock(&pctldev->mutex);
+
+	/* Convert to the pin controllers number space */
+	pin = gpio_to_pin(range, gpio);
+	ret = pinmux_gpio_get_direction(pctldev, range, pin);
+
+	mutex_unlock(&pctldev->mutex);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(pinctrl_gpio_get_direction);
+
+/**
  * pinctrl_gpio_set_config() - Apply config to given GPIO pin
  * @gpio: the GPIO pin number from the GPIO subsystem number space
  * @config: the configuration to apply to the GPIO
diff --git a/include/linux/pinctrl/consumer.h b/include/linux/pinctrl/consumer.h
index 0412cc9833e9..5f945c329f1c 100644
--- a/include/linux/pinctrl/consumer.h
+++ b/include/linux/pinctrl/consumer.h
@@ -29,6 +29,7 @@ extern int pinctrl_gpio_request(unsigned gpio);
 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_get_direction(unsigned gpio);
 extern int pinctrl_gpio_set_config(unsigned gpio, unsigned long config);
 
 extern struct pinctrl * __must_check pinctrl_get(struct device *dev);
-- 
2.11.0

Re: [PATCH 2/5] pinctrl: Add API function pinctrl_gpio_get_direction

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 2405 bytes --]

Hi Paul,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on pinctrl/devel]
[also build test ERROR on v4.18-rc2 next-20180627]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Paul-Cercueil/pinctrl-Add-core-function-pinmux_gpio_get_direction/20180627-201609
base:   https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-pinctrl.git devel
config: x86_64-randconfig-x002-201825 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-16) 7.3.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All errors (new ones prefixed by >>):

   drivers//pinctrl/core.c: In function 'pinctrl_gpio_get_direction':
>> drivers//pinctrl/core.c:873:8: error: implicit declaration of function 'pinmux_gpio_get_direction'; did you mean 'pinmux_gpio_direction'? [-Werror=implicit-function-declaration]
     ret = pinmux_gpio_get_direction(pctldev, range, pin);
           ^~~~~~~~~~~~~~~~~~~~~~~~~
           pinmux_gpio_direction
   cc1: some warnings being treated as errors

vim +873 drivers//pinctrl/core.c

   849	
   850	/**
   851	 * pinctrl_gpio_get_direction() - Get the direction (input/output) of a GPIO pin
   852	 * @gpio: the GPIO pin number from the GPIO subsystem number space
   853	 *
   854	 * This function should *ONLY* be used from gpiolib-based GPIO drivers,
   855	 * as part of their gpio_get_direction() semantics, platforms and individual
   856	 * drivers shall *NOT* touch pin control GPIO calls.
   857	 */
   858	int pinctrl_gpio_get_direction(unsigned int gpio)
   859	{
   860		struct pinctrl_dev *pctldev;
   861		struct pinctrl_gpio_range *range;
   862		int ret;
   863		int pin;
   864	
   865		ret = pinctrl_get_device_gpio_range(gpio, &pctldev, &range);
   866		if (ret)
   867			return ret;
   868	
   869		mutex_lock(&pctldev->mutex);
   870	
   871		/* Convert to the pin controllers number space */
   872		pin = gpio_to_pin(range, gpio);
 > 873		ret = pinmux_gpio_get_direction(pctldev, range, pin);
   874	
   875		mutex_unlock(&pctldev->mutex);
   876	
   877		return ret;
   878	}
   879	EXPORT_SYMBOL_GPL(pinctrl_gpio_get_direction);
   880	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 33709 bytes --]

Re: [PATCH 2/5] pinctrl: Add API function pinctrl_gpio_get_direction

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 9112 bytes --]

Hi Paul,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on pinctrl/devel]
[also build test ERROR on v4.18-rc2 next-20180627]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Paul-Cercueil/pinctrl-Add-core-function-pinmux_gpio_get_direction/20180627-201609
base:   https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-pinctrl.git devel
config: x86_64-randconfig-s0-06272107 (attached as .config)
compiler: gcc-6 (Debian 6.4.0-9) 6.4.0 20171026
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All errors (new ones prefixed by >>):

   drivers//pinctrl/core.c: In function 'pinctrl_gpio_get_direction':
>> drivers//pinctrl/core.c:873:8: error: implicit declaration of function 'pinmux_gpio_get_direction' [-Werror=implicit-function-declaration]
     ret = pinmux_gpio_get_direction(pctldev, range, pin);
           ^~~~~~~~~~~~~~~~~~~~~~~~~
   Cyclomatic Complexity 5 include/linux/compiler.h:__write_once_size
   Cyclomatic Complexity 1 include/linux/kasan-checks.h:kasan_check_write
   Cyclomatic Complexity 1 arch/x86/include/asm/bitops.h:fls64
   Cyclomatic Complexity 1 include/linux/log2.h:__ilog2_u64
   Cyclomatic Complexity 1 include/linux/list.h:INIT_LIST_HEAD
   Cyclomatic Complexity 1 include/linux/list.h:__list_del
   Cyclomatic Complexity 1 include/asm-generic/getorder.h:__get_order
   Cyclomatic Complexity 1 arch/x86/include/asm/atomic.h:arch_atomic_set
   Cyclomatic Complexity 1 include/asm-generic/atomic-instrumented.h:atomic_set
   Cyclomatic Complexity 3 include/linux/string.h:kmemdup
   Cyclomatic Complexity 1 include/linux/err.h:ERR_PTR
   Cyclomatic Complexity 1 include/linux/err.h:PTR_ERR
   Cyclomatic Complexity 1 include/linux/spinlock.h:spinlock_check
   Cyclomatic Complexity 1 include/linux/refcount.h:refcount_set
   Cyclomatic Complexity 1 include/linux/kref.h:kref_init
   Cyclomatic Complexity 1 include/linux/kobject.h:kobject_name
   Cyclomatic Complexity 2 include/linux/device.h:dev_name
   Cyclomatic Complexity 28 include/linux/slab.h:kmalloc_index
   Cyclomatic Complexity 68 include/linux/slab.h:kmalloc_large
   Cyclomatic Complexity 5 include/linux/slab.h:kmalloc
   Cyclomatic Complexity 1 include/linux/slab.h:kzalloc
   Cyclomatic Complexity 1 drivers//pinctrl/devicetree.h:pinctrl_dt_to_map
   Cyclomatic Complexity 1 drivers//pinctrl/devicetree.h:pinctrl_dt_free_maps
   Cyclomatic Complexity 1 drivers//pinctrl/pinmux.h:pinmux_check_ops
   Cyclomatic Complexity 1 drivers//pinctrl/pinmux.h:pinmux_validate_map
   Cyclomatic Complexity 1 drivers//pinctrl/pinmux.h:pinmux_request_gpio
   Cyclomatic Complexity 1 drivers//pinctrl/pinmux.h:pinmux_free_gpio
   Cyclomatic Complexity 1 drivers//pinctrl/pinmux.h:pinmux_gpio_direction
   Cyclomatic Complexity 1 drivers//pinctrl/pinmux.h:pinmux_map_to_setting
   Cyclomatic Complexity 1 drivers//pinctrl/pinmux.h:pinmux_free_setting
   Cyclomatic Complexity 1 drivers//pinctrl/pinmux.h:pinmux_enable_setting
   Cyclomatic Complexity 1 drivers//pinctrl/pinmux.h:pinmux_disable_setting
   Cyclomatic Complexity 1 drivers//pinctrl/pinmux.h:pinmux_show_map
   Cyclomatic Complexity 1 drivers//pinctrl/pinmux.h:pinmux_show_setting
   Cyclomatic Complexity 1 drivers//pinctrl/pinmux.h:pinmux_init_device_debugfs
   Cyclomatic Complexity 1 drivers//pinctrl/pinmux.h:pinmux_generic_free_functions
   Cyclomatic Complexity 2 drivers//pinctrl/core.c:gpio_to_pin
   Cyclomatic Complexity 1 drivers//pinctrl/core.c:pinctrl_generic_free_groups
   Cyclomatic Complexity 1 drivers//pinctrl/core.c:devm_pinctrl_match
   Cyclomatic Complexity 2 drivers//pinctrl/core.c:map_type
   Cyclomatic Complexity 4 drivers//pinctrl/core.c:pinctrl_check_ops
   Cyclomatic Complexity 1 drivers//pinctrl/core.h:pin_desc_get
   Cyclomatic Complexity 2 include/linux/list.h:__list_add
   Cyclomatic Complexity 1 include/linux/list.h:list_add_tail
   Cyclomatic Complexity 2 drivers//pinctrl/core.c:create_state
   Cyclomatic Complexity 2 include/linux/list.h:__list_del_entry
   Cyclomatic Complexity 1 include/linux/list.h:list_del
   Cyclomatic Complexity 6 drivers//pinctrl/core.c:pinctrl_match_gpio_range
   Cyclomatic Complexity 5 drivers//pinctrl/core.c:pinctrl_get_device_gpio_range
   Cyclomatic Complexity 2 drivers//pinctrl/core.c:pinctrl_gpio_direction
   Cyclomatic Complexity 1 include/linux/err.h:IS_ERR
   Cyclomatic Complexity 3 include/linux/err.h:IS_ERR_OR_NULL
   Cyclomatic Complexity 5 drivers//pinctrl/core.c:devm_pinctrl_dev_match
   Cyclomatic Complexity 1 include/asm-generic/gpio.h:gpio_to_chip
   Cyclomatic Complexity 11 drivers//pinctrl/core.c:pinctrl_ready_for_gpio_range
   Cyclomatic Complexity 5 drivers//pinctrl/core.c:find_pinctrl
   Cyclomatic Complexity 1 include/linux/kref.h:kref_get
   Cyclomatic Complexity 5 drivers//pinctrl/core.c:find_state
   Cyclomatic Complexity 4 drivers//pinctrl/core.c:pinctrl_free_setting
   Cyclomatic Complexity 10 drivers//pinctrl/core.c:pinctrl_free
   Cyclomatic Complexity 2 drivers//pinctrl/core.c:pinctrl_release
   Cyclomatic Complexity 2 include/linux/kref.h:kref_put
   Cyclomatic Complexity 3 drivers//pinctrl/core.c:pinctrl_init_debugfs
   Cyclomatic Complexity 1 drivers//pinctrl/core.c:pinctrl_init
   Cyclomatic Complexity 8 drivers//pinctrl/core.c:pinctrl_init_device_debugfs
   Cyclomatic Complexity 1 drivers//pinctrl/core.c:pinctrl_gpioranges_open
   Cyclomatic Complexity 1 drivers//pinctrl/core.c:pinctrl_groups_open
   Cyclomatic Complexity 1 drivers//pinctrl/core.c:pinctrl_pins_open
   Cyclomatic Complexity 1 drivers//pinctrl/core.c:pinctrl_open
   Cyclomatic Complexity 1 drivers//pinctrl/core.c:pinctrl_maps_open
   Cyclomatic Complexity 1 drivers//pinctrl/core.c:pinctrl_devices_open
   Cyclomatic Complexity 6 drivers//pinctrl/core.c:pinctrl_gpioranges_show
   Cyclomatic Complexity 4 drivers//pinctrl/core.c:pinctrl_pins_show
   Cyclomatic Complexity 1 include/linux/radix-tree.h:radix_tree_insert
   Cyclomatic Complexity 5 drivers//pinctrl/core.c:pinctrl_register_one_pin
   Cyclomatic Complexity 3 drivers//pinctrl/core.c:pinctrl_register_pins
   Cyclomatic Complexity 4 drivers//pinctrl/core.c:pinctrl_free_pindescs
   Cyclomatic Complexity 10 drivers//pinctrl/core.c:pinctrl_init_controller
   Cyclomatic Complexity 1 drivers//pinctrl/core.c:pinctrl_remove_device_debugfs
   Cyclomatic Complexity 8 drivers//pinctrl/core.c:pinctrl_maps_show
   Cyclomatic Complexity 6 drivers//pinctrl/core.c:pinctrl_devices_show
   Cyclomatic Complexity 1 drivers//pinctrl/core.c:pinctrl_provide_dummies
   Cyclomatic Complexity 1 drivers//pinctrl/core.c:pinctrl_dev_get_name
   Cyclomatic Complexity 13 drivers//pinctrl/core.c:pinctrl_show
   Cyclomatic Complexity 1 drivers//pinctrl/core.c:pinctrl_dev_get_devname
   Cyclomatic Complexity 1 drivers//pinctrl/core.c:pinctrl_dev_get_drvdata
   Cyclomatic Complexity 6 drivers//pinctrl/core.c:get_pinctrl_dev_from_devname
   Cyclomatic Complexity 11 drivers//pinctrl/core.c:add_setting
   Cyclomatic Complexity 12 drivers//pinctrl/core.c:create_pinctrl
   Cyclomatic Complexity 5 drivers//pinctrl/core.c:get_pinctrl_dev_from_of_node
   Cyclomatic Complexity 4 drivers//pinctrl/core.c:pin_get_from_name
   Cyclomatic Complexity 2 drivers//pinctrl/core.c:pin_get_name
   Cyclomatic Complexity 7 drivers//pinctrl/core.c:pinctrl_groups_show
   Cyclomatic Complexity 2 drivers//pinctrl/core.c:pin_is_valid
   Cyclomatic Complexity 1 drivers//pinctrl/core.c:pinctrl_add_gpio_range
   Cyclomatic Complexity 2 drivers//pinctrl/core.c:pinctrl_add_gpio_ranges
   Cyclomatic Complexity 2 drivers//pinctrl/core.c:pinctrl_find_and_add_gpio_range
   Cyclomatic Complexity 9 drivers//pinctrl/core.c:pinctrl_find_gpio_range_from_pin_nolock

vim +/pinmux_gpio_get_direction +873 drivers//pinctrl/core.c

   849	
   850	/**
   851	 * pinctrl_gpio_get_direction() - Get the direction (input/output) of a GPIO pin
   852	 * @gpio: the GPIO pin number from the GPIO subsystem number space
   853	 *
   854	 * This function should *ONLY* be used from gpiolib-based GPIO drivers,
   855	 * as part of their gpio_get_direction() semantics, platforms and individual
   856	 * drivers shall *NOT* touch pin control GPIO calls.
   857	 */
   858	int pinctrl_gpio_get_direction(unsigned int gpio)
   859	{
   860		struct pinctrl_dev *pctldev;
   861		struct pinctrl_gpio_range *range;
   862		int ret;
   863		int pin;
   864	
   865		ret = pinctrl_get_device_gpio_range(gpio, &pctldev, &range);
   866		if (ret)
   867			return ret;
   868	
   869		mutex_lock(&pctldev->mutex);
   870	
   871		/* Convert to the pin controllers number space */
   872		pin = gpio_to_pin(range, gpio);
 > 873		ret = pinmux_gpio_get_direction(pctldev, range, pin);
   874	
   875		mutex_unlock(&pctldev->mutex);
   876	
   877		return ret;
   878	}
   879	EXPORT_SYMBOL_GPL(pinctrl_gpio_get_direction);
   880	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 26141 bytes --]

[PATCH 3/5] pinctrl: ingenic: Fix inverted direction for < JZ4770

[Paul Cercueil]

The .gpio_set_direction() callback was setting inverted direction
for SoCs older than the JZ4770, this restores the correct behaviour.

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---
 drivers/pinctrl/pinctrl-ingenic.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pinctrl/pinctrl-ingenic.c b/drivers/pinctrl/pinctrl-ingenic.c
index a1d7156d0a43..6a1b6058b991 100644
--- a/drivers/pinctrl/pinctrl-ingenic.c
+++ b/drivers/pinctrl/pinctrl-ingenic.c
@@ -536,7 +536,7 @@ static int ingenic_pinmux_gpio_set_direction(struct pinctrl_dev *pctldev,
 		ingenic_config_pin(jzpc, pin, JZ4770_GPIO_PAT1, input);
 	} else {
 		ingenic_config_pin(jzpc, pin, JZ4740_GPIO_SELECT, false);
-		ingenic_config_pin(jzpc, pin, JZ4740_GPIO_DIR, input);
+		ingenic_config_pin(jzpc, pin, JZ4740_GPIO_DIR, !input);
 		ingenic_config_pin(jzpc, pin, JZ4740_GPIO_FUNC, false);
 	}
 
-- 
2.11.0

Re: [PATCH 3/5] pinctrl: ingenic: Fix inverted direction for < JZ4770

[Linus Walleij]

On Wed, Jun 27, 2018 at 1:49 PM Paul Cercueil <paul@crapouillou.net> wrote:

> The .gpio_set_direction() callback was setting inverted direction
> for SoCs older than the JZ4770, this restores the correct behaviour.
>
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>

As you pointed out that this was an important fix I picked it
out and applied for fixes (v4.18) before looking at the rest of
the patches.

Yours,
Linus Walleij

[PATCH 4/5] pinctrl: ingenic: Implement pinmux callback .gpio_get_direction

[Paul Cercueil]

This will allow the GPIO driver to use the previously introduced
pinctrl_gpio_get_direction function to implement its .get_direction
callback.

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---
 drivers/pinctrl/pinctrl-ingenic.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/pinctrl/pinctrl-ingenic.c b/drivers/pinctrl/pinctrl-ingenic.c
index 6a1b6058b991..2dab52cf119b 100644
--- a/drivers/pinctrl/pinctrl-ingenic.c
+++ b/drivers/pinctrl/pinctrl-ingenic.c
@@ -543,12 +543,25 @@ static int ingenic_pinmux_gpio_set_direction(struct pinctrl_dev *pctldev,
 	return 0;
 }
 
+static int ingenic_pinmux_gpio_get_direction(struct pinctrl_dev *pctldev,
+		struct pinctrl_gpio_range *range,
+		unsigned int pin)
+{
+	struct ingenic_pinctrl *jzpc = pinctrl_dev_get_drvdata(pctldev);
+
+	if (jzpc->version >= ID_JZ4770)
+		return ingenic_get_pin_config(jzpc, pin, JZ4770_GPIO_PAT1);
+	else
+		return !ingenic_get_pin_config(jzpc, pin, JZ4740_GPIO_DIR);
+}
+
 static const struct pinmux_ops ingenic_pmxops = {
 	.get_functions_count = pinmux_generic_get_function_count,
 	.get_function_name = pinmux_generic_get_function_name,
 	.get_function_groups = pinmux_generic_get_function_groups,
 	.set_mux = ingenic_pinmux_set_mux,
 	.gpio_set_direction = ingenic_pinmux_gpio_set_direction,
+	.gpio_get_direction = ingenic_pinmux_gpio_get_direction,
 };
 
 static int ingenic_pinconf_get(struct pinctrl_dev *pctldev,
-- 
2.11.0

[PATCH 5/5] GPIO: ingenic: Implement .get_direction

[Paul Cercueil]

Use the newly introduced pinctrl_gpio_get_direction to implement the
.get_direction callback.

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---
 drivers/gpio/gpio-ingenic.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/gpio/gpio-ingenic.c b/drivers/gpio/gpio-ingenic.c
index e738e384a5ca..79e8b67fe83d 100644
--- a/drivers/gpio/gpio-ingenic.c
+++ b/drivers/gpio/gpio-ingenic.c
@@ -274,6 +274,12 @@ static int ingenic_gpio_direction_output(struct gpio_chip *gc,
 	return pinctrl_gpio_direction_output(gc->base + offset);
 }
 
+static int ingenic_gpio_get_direction(struct gpio_chip *gc,
+		unsigned int offset)
+{
+	return pinctrl_gpio_get_direction(gc->base + offset);
+}
+
 static const struct of_device_id ingenic_gpio_of_match[] = {
 	{ .compatible = "ingenic,jz4740-gpio", .data = (void *)ID_JZ4740 },
 	{ .compatible = "ingenic,jz4770-gpio", .data = (void *)ID_JZ4770 },
@@ -325,6 +331,7 @@ static int ingenic_gpio_probe(struct platform_device *pdev)
 
 	jzgc->gc.set = ingenic_gpio_set;
 	jzgc->gc.get = ingenic_gpio_get;
+	jzgc->gc.get_direction = ingenic_gpio_get_direction;
 	jzgc->gc.direction_input = ingenic_gpio_direction_input;
 	jzgc->gc.direction_output = ingenic_gpio_direction_output;
 
-- 
2.11.0

Re: [PATCH 0/5] pinctrl_gpio_get_direction & ingenic fixes

[Andy Shevchenko]

On Wed, Jun 27, 2018 at 2:48 PM, Paul Cercueil <paul@crapouillou.net> wrote:
> Hi Linus,
>
> Here's a set of (rather RFC) patches, to implement
> pinctrl_gpio_get_direction(). I did that, because my gpio-ingenic driver
> calls pinctrl_gpio_set_direction() within its gpio_chip's .set_direction
> callback, but there was no corresponding function to implement the
> .get_direction callback. If that's not the right way to do it, please
> advise.
>
> If not merging the whole series, patch [3/5] is a real fix that should
> go through.
>
> Note that it doesn't make checkpatch.pl happy, I wasn't sure whether I
> should try to comply to checkpatch.pl or match the coding style in the
> pinctrl subsystem, I chose the latter.

I dunno what Linus would going to say about this, but I would like to
see a schematics for this piece of IP.
Even if GPIO and pin muxing has only one set of buffers to indicate
input or output (same registers in use) it's a GPIO driver business to
get direction from GPIO part of IP.

Looking into the existing code I would rather say that
pinctrl-ingenic.c should incorporate gpio-ingenic.c as they are
(partially) sharing same registers.
To ->get_direction() implementation it's pretty straight forward, just
read necessary registers in the gpio-ingenic.c directly. No need to
have pin control or pin muxing to be involved.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 0/5] pinctrl_gpio_get_direction & ingenic fixes

[Paul Cercueil]

Hi Andy,

Le mer. 27 juin 2018 à 19:18, Andy Shevchenko 
<andy.shevchenko@gmail.com> a écrit :
> On Wed, Jun 27, 2018 at 2:48 PM, Paul Cercueil <paul@crapouillou.net> 
> wrote:
>>  Hi Linus,
>> 
>>  Here's a set of (rather RFC) patches, to implement
>>  pinctrl_gpio_get_direction(). I did that, because my gpio-ingenic 
>> driver
>>  calls pinctrl_gpio_set_direction() within its gpio_chip's 
>> .set_direction
>>  callback, but there was no corresponding function to implement the
>>  .get_direction callback. If that's not the right way to do it, 
>> please
>>  advise.
>> 
>>  If not merging the whole series, patch [3/5] is a real fix that 
>> should
>>  go through.
>> 
>>  Note that it doesn't make checkpatch.pl happy, I wasn't sure 
>> whether I
>>  should try to comply to checkpatch.pl or match the coding style in 
>> the
>>  pinctrl subsystem, I chose the latter.
> 
> I dunno what Linus would going to say about this, but I would like to
> see a schematics for this piece of IP.

ftp://ftp.ingenic.com/SOC/JZ4780/JZ4780_pm.pdf

> Even if GPIO and pin muxing has only one set of buffers to indicate
> input or output (same registers in use) it's a GPIO driver business to
> get direction from GPIO part of IP.

If I follow that logic it's also a GPIO driver business to set the 
direction
of a GPIO, right? Truth is the pinctrl subsystem takes care of that. So 
why
have "set direction" and no "get direction"?

> Looking into the existing code I would rather say that
> pinctrl-ingenic.c should incorporate gpio-ingenic.c as they are
> (partially) sharing same registers.
> To ->get_direction() implementation it's pretty straight forward, just
> read necessary registers in the gpio-ingenic.c directly. No need to
> have pin control or pin muxing to be involved.

Sure, it'd be pretty straightforward to do it from the GPIO driver, but 
I'd
still like to hear Linus' point of view about this.

As for merging pinctrl-ingenic.c and gpio-ingenic.c... I wouldn't 
disagree more,
even if they share registers, they belong to different subsystems. 
Besides,
your platform might need the pinctrl driver but not the GPIO one, or 
you might
want to provide the GPIO driver as a loadable module, etc.

Regards,
-Paul

Re: [PATCH 0/5] pinctrl_gpio_get_direction & ingenic fixes

[Andy Shevchenko]

On Thu, Jun 28, 2018 at 10:11 PM, Paul Cercueil <paul@crapouillou.net> wrote:
> Le mer. 27 juin 2018 à 19:18, Andy Shevchenko <andy.shevchenko@gmail.com> a
> écrit :
>> On Wed, Jun 27, 2018 at 2:48 PM, Paul Cercueil <paul@crapouillou.net>
>> wrote:

>> I dunno what Linus would going to say about this, but I would like to
>> see a schematics for this piece of IP.

> ftp://ftp.ingenic.com/SOC/JZ4780/JZ4780_pm.pdf

I'm not sure you referred to a proper document "Mobile Application
Processor. Programming Manual"

I didn't find quickly what I'm looking for. Care to point me out to
the page number?
Thanks!

>> Even if GPIO and pin muxing has only one set of buffers to indicate
>> input or output (same registers in use) it's a GPIO driver business to
>> get direction from GPIO part of IP.
>
>
> If I follow that logic it's also a GPIO driver business to set the direction
> of a GPIO, right? Truth is the pinctrl subsystem takes care of that. So why
> have "set direction" and no "get direction"?

Because it's a pin muxing business depending on the function chosen.

It solely depends on a schematic. For the example, everyone knows that
MOSI is out always, otherwise SPI function would not work.
.get_direction() has useful meaning only for GPIO function. (This is
my understanding from electrical configuration of buffers).

>> Looking into the existing code I would rather say that
>> pinctrl-ingenic.c should incorporate gpio-ingenic.c as they are
>> (partially) sharing same registers.
>> To ->get_direction() implementation it's pretty straight forward, just
>> read necessary registers in the gpio-ingenic.c directly. No need to
>> have pin control or pin muxing to be involved.

> Sure, it'd be pretty straightforward to do it from the GPIO driver, but I'd
> still like to hear Linus' point of view about this.

Sure.

> As for merging pinctrl-ingenic.c and gpio-ingenic.c... I wouldn't disagree
> more,
> even if they share registers, they belong to different subsystems. Besides,
> your platform might need the pinctrl driver but not the GPIO one, or you
> might
> want to provide the GPIO driver as a loadable module, etc.

It was just my observation, I'm not a maintainer of that code  nor
author, so, up to you how to proceed.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 0/5] pinctrl_gpio_get_direction & ingenic fixes

[Linus Walleij]

On Thu, Jun 28, 2018 at 9:11 PM Paul Cercueil <paul@crapouillou.net> wrote:

> Sure, it'd be pretty straightforward to do it from the GPIO driver, but
> I'd still like to hear Linus' point of view about this.

I'm not sure about it, I guess it would be my second choice.

> As for merging pinctrl-ingenic.c and gpio-ingenic.c... I wouldn't
> disagree more,
> even if they share registers, they belong to different subsystems.

We actually have many pin control drivers also exposing
a GPIO chip. (Or several.) The way I see it is that we use
two kernel APIs for the same hardware, but one hardware
piece should still just have one driver, albeit exposing two
interfaces.

We actually also have serial ports and wireless network
cards exposing random GPIO chips because they happen
to have some arbitrary GPIO lines up for grabs.

So it is not just for the pin control and GPIO usecase, it is
for any usecase where the same hardware has something
plus a GPIO API. Pin control with GPIO as front-end is
special by being jitted together with the ranges though, so
they are closer related.

A good reason to keep it together in one driver is e.g. that they
have common clocking, so otherwise two drivers are grabbing
the same silicon clock and enabling/disabling it, runtime PM
would be a mess if both drivers start to request prepare
enable disable clocks at the same time etc. (I don't know
if this applied to Ingenic.)

> Besides,
> your platform might need the pinctrl driver but not the GPIO one, or
> you might
> want to provide the GPIO driver as a loadable module, etc.

I understand that concern. I think it's as nice with one
loadable module for pin control and GPIO at the same
time though, especially if the split is anyways kind
artificial between the two APIs.

Yours,
Linus Walleij

Re: [PATCH 0/5] pinctrl_gpio_get_direction & ingenic fixes

[Linus Walleij]

Hi folks,

On Wed, Jun 27, 2018 at 7:18 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:

> Even if GPIO and pin muxing has only one set of buffers to indicate
> input or output (same registers in use) it's a GPIO driver business to
> get direction from GPIO part of IP.
>
> Looking into the existing code I would rather say that
> pinctrl-ingenic.c should incorporate gpio-ingenic.c as they are
> (partially) sharing same registers.

Usually we only split the functionality into two drivers if the two features
pin control and GPIO are explicitly in different hardware blocks,
and typically not sharing the same memory range.

If these registers are intermingled and the hardware actually
just one piece of silicon, I would suggest to try to merge the
two drivers into a combined pin control and GPIO driver
inside drivers/pinctrl/pinctrl-ingenic.c.

We have a few drivers like that already, good textbook
examples of how to do this include
drivers/pinctrl/pinctrl-sx150x.c where the two blocks are
handled in one driver using both APIs.

Paul could you have a look at if we can simply merge these
two into one big driver? It is much more natural to write
into the same set of registers when we do that.

If you still prefer to proceed with the GPIO/pinctrl as separate
drivers we need to look into this patch set, which I am
a bit ambivalent about, because it makes sense but at the
same time I want to keep GPIO and pin control business
separate because separation of concerns is just nice.

Yours,
Linus Walleij

Re: [PATCH 0/5] pinctrl_gpio_get_direction & ingenic fixes

[Paul Cercueil]

Hi Linus,


Le lun. 9 juil. 2018 à 14:09, Linus Walleij <linus.walleij@linaro.org> 
a écrit :
> Hi folks,
> 
> On Wed, Jun 27, 2018 at 7:18 PM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> 
>>  Even if GPIO and pin muxing has only one set of buffers to indicate
>>  input or output (same registers in use) it's a GPIO driver business 
>> to
>>  get direction from GPIO part of IP.
>> 
>>  Looking into the existing code I would rather say that
>>  pinctrl-ingenic.c should incorporate gpio-ingenic.c as they are
>>  (partially) sharing same registers.
> 
> Usually we only split the functionality into two drivers if the two 
> features
> pin control and GPIO are explicitly in different hardware blocks,
> and typically not sharing the same memory range.
> 
> If these registers are intermingled and the hardware actually
> just one piece of silicon, I would suggest to try to merge the
> two drivers into a combined pin control and GPIO driver
> inside drivers/pinctrl/pinctrl-ingenic.c.
> 
> We have a few drivers like that already, good textbook
> examples of how to do this include
> drivers/pinctrl/pinctrl-sx150x.c where the two blocks are
> handled in one driver using both APIs.
> 
> Paul could you have a look at if we can simply merge these
> two into one big driver? It is much more natural to write
> into the same set of registers when we do that.

Well I wish you had told me that when I submitted the ingenic 
pinctrl/gpio
patchset :)

I won't have much time before 4.19-rc1, but I can have a look after 
that.

> If you still prefer to proceed with the GPIO/pinctrl as separate
> drivers we need to look into this patch set, which I am
> a bit ambivalent about, because it makes sense but at the
> same time I want to keep GPIO and pin control business
> separate because separation of concerns is just nice.

Well I can still implement the get_direction() function in the GPIO
driver by reading the registers instead of calling into pinctrl.

I just thought it felt illogic as set_direction() does that.

> Yours,
> Linus Walleij

Thanks,
-Paul