[PATCH v2 0/4] sh-pfc: handle pin array holes in sh_pfc_map_pins()

[PATCH v2 0/4] sh-pfc: handle pin array holes in sh_pfc_map_pins()

[Sergei Shtylyov]

Hello.

   Here's the set of 4 patches against the 'devel' branch of Linus Walleij's
'linux-pinctrl.git' repo. Here we add proper handling of the pin array "holes"
and then remove unused GPIO pins for R8A7790/1 SoCs.

[1/4] sh-pfc: do not call sh_pfc_get_pin_index() when unnecessary
[2/4] sh-pfc: handle pin array holes
[3/4] sh-pfc: r8a7790 remove non-existing GPIO pins
[4/4] sh-pfc: r8a7791 remove non-existing GPIO pins

WBR, Sergei

[PATCH v2 0/4] sh-pfc: handle pin array holes in sh_pfc_map_pins()

[Sergei Shtylyov]

Hello.

   Here's the set of 4 patches against the 'devel' branch of Linus Walleij's
'linux-pinctrl.git' repo. Here we add proper handling of the pin array "holes"
and then remove unused GPIO pins for R8A7790/1 SoCs.

[1/4] sh-pfc: do not call sh_pfc_get_pin_index() when unnecessary
[2/4] sh-pfc: handle pin array holes
[3/4] sh-pfc: r8a7790 remove non-existing GPIO pins
[4/4] sh-pfc: r8a7791 remove non-existing GPIO pins

WBR, Sergei

[PATCH v2 1/4] sh-pfc: do not call sh_pfc_get_pin_index() when unnecessary

[Sergei Shtylyov]

Calling sh_pfc_get_pin_index()  to calculate a pin index based on the collected
pin range data is unnecessary when we're dealing with 'pfc->info->pins' and
'chip->pins' arrays as those always reperesent the pins starting from index 0
sequentially. Being a  mere  optimization at this time, this change will become
crucial when we'll allow the "holes" in those arrays...

Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>

---
Changes in version 2:
- new patch.

 drivers/pinctrl/sh-pfc/gpio.c    |    6 ++----
 drivers/pinctrl/sh-pfc/pinctrl.c |    7 +++----
 2 files changed, 5 insertions(+), 8 deletions(-)

Index: linux-pinctrl/drivers/pinctrl/sh-pfc/gpio.c
=================================--- linux-pinctrl.orig/drivers/pinctrl/sh-pfc/gpio.c
+++ linux-pinctrl/drivers/pinctrl/sh-pfc/gpio.c
@@ -52,8 +52,7 @@ static void gpio_get_data_reg(struct sh_
 			      struct sh_pfc_gpio_data_reg **reg,
 			      unsigned int *bit)
 {
-	int idx = sh_pfc_get_pin_index(chip->pfc, offset);
-	struct sh_pfc_gpio_pin *gpio_pin = &chip->pins[idx];
+	struct sh_pfc_gpio_pin *gpio_pin = &chip->pins[offset];
 
 	*reg = &chip->regs[gpio_pin->dreg];
 	*bit = gpio_pin->dbit;
@@ -138,9 +137,8 @@ static int gpio_setup_data_regs(struct s
 static int gpio_pin_request(struct gpio_chip *gc, unsigned offset)
 {
 	struct sh_pfc *pfc = gpio_to_pfc(gc);
-	int idx = sh_pfc_get_pin_index(pfc, offset);
 
-	if (idx < 0 || pfc->info->pins[idx].enum_id = 0)
+	if (pfc->info->pins[offset].enum_id = 0)
 		return -EINVAL;
 
 	return pinctrl_request_gpio(offset);
Index: linux-pinctrl/drivers/pinctrl/sh-pfc/pinctrl.c
=================================--- linux-pinctrl.orig/drivers/pinctrl/sh-pfc/pinctrl.c
+++ linux-pinctrl/drivers/pinctrl/sh-pfc/pinctrl.c
@@ -370,7 +370,7 @@ static int sh_pfc_gpio_request_enable(st
 		/* If GPIOs are handled externally the pin mux type need to be
 		 * set to GPIO here.
 		 */
-		const struct sh_pfc_pin *pin = &pfc->info->pins[idx];
+		const struct sh_pfc_pin *pin = &pfc->info->pins[offset];
 
 		ret = sh_pfc_config_mux(pfc, pin->enum_id, PINMUX_TYPE_GPIO);
 		if (ret < 0)
@@ -410,7 +410,7 @@ static int sh_pfc_gpio_set_direction(str
 	struct sh_pfc *pfc = pmx->pfc;
 	int new_type = input ? PINMUX_TYPE_INPUT : PINMUX_TYPE_OUTPUT;
 	int idx = sh_pfc_get_pin_index(pfc, offset);
-	const struct sh_pfc_pin *pin = &pfc->info->pins[idx];
+	const struct sh_pfc_pin *pin = &pfc->info->pins[offset];
 	struct sh_pfc_pin_config *cfg = &pmx->configs[idx];
 	unsigned long flags;
 	unsigned int dir;
@@ -452,8 +452,7 @@ static const struct pinmux_ops sh_pfc_pi
 static bool sh_pfc_pinconf_validate(struct sh_pfc *pfc, unsigned int _pin,
 				    enum pin_config_param param)
 {
-	int idx = sh_pfc_get_pin_index(pfc, _pin);
-	const struct sh_pfc_pin *pin = &pfc->info->pins[idx];
+	const struct sh_pfc_pin *pin = &pfc->info->pins[_pin];
 
 	switch (param) {
 	case PIN_CONFIG_BIAS_DISABLE:

[PATCH v2 1/4] sh-pfc: do not call sh_pfc_get_pin_index() when unnecessary

[Sergei Shtylyov]

Calling sh_pfc_get_pin_index()  to calculate a pin index based on the collected
pin range data is unnecessary when we're dealing with 'pfc->info->pins' and
'chip->pins' arrays as those always reperesent the pins starting from index 0
sequentially. Being a  mere  optimization at this time, this change will become
crucial when we'll allow the "holes" in those arrays...

Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>

---
Changes in version 2:
- new patch.

 drivers/pinctrl/sh-pfc/gpio.c    |    6 ++----
 drivers/pinctrl/sh-pfc/pinctrl.c |    7 +++----
 2 files changed, 5 insertions(+), 8 deletions(-)

Index: linux-pinctrl/drivers/pinctrl/sh-pfc/gpio.c
===================================================================
--- linux-pinctrl.orig/drivers/pinctrl/sh-pfc/gpio.c
+++ linux-pinctrl/drivers/pinctrl/sh-pfc/gpio.c
@@ -52,8 +52,7 @@ static void gpio_get_data_reg(struct sh_
 			      struct sh_pfc_gpio_data_reg **reg,
 			      unsigned int *bit)
 {
-	int idx = sh_pfc_get_pin_index(chip->pfc, offset);
-	struct sh_pfc_gpio_pin *gpio_pin = &chip->pins[idx];
+	struct sh_pfc_gpio_pin *gpio_pin = &chip->pins[offset];
 
 	*reg = &chip->regs[gpio_pin->dreg];
 	*bit = gpio_pin->dbit;
@@ -138,9 +137,8 @@ static int gpio_setup_data_regs(struct s
 static int gpio_pin_request(struct gpio_chip *gc, unsigned offset)
 {
 	struct sh_pfc *pfc = gpio_to_pfc(gc);
-	int idx = sh_pfc_get_pin_index(pfc, offset);
 
-	if (idx < 0 || pfc->info->pins[idx].enum_id == 0)
+	if (pfc->info->pins[offset].enum_id == 0)
 		return -EINVAL;
 
 	return pinctrl_request_gpio(offset);
Index: linux-pinctrl/drivers/pinctrl/sh-pfc/pinctrl.c
===================================================================
--- linux-pinctrl.orig/drivers/pinctrl/sh-pfc/pinctrl.c
+++ linux-pinctrl/drivers/pinctrl/sh-pfc/pinctrl.c
@@ -370,7 +370,7 @@ static int sh_pfc_gpio_request_enable(st
 		/* If GPIOs are handled externally the pin mux type need to be
 		 * set to GPIO here.
 		 */
-		const struct sh_pfc_pin *pin = &pfc->info->pins[idx];
+		const struct sh_pfc_pin *pin = &pfc->info->pins[offset];
 
 		ret = sh_pfc_config_mux(pfc, pin->enum_id, PINMUX_TYPE_GPIO);
 		if (ret < 0)
@@ -410,7 +410,7 @@ static int sh_pfc_gpio_set_direction(str
 	struct sh_pfc *pfc = pmx->pfc;
 	int new_type = input ? PINMUX_TYPE_INPUT : PINMUX_TYPE_OUTPUT;
 	int idx = sh_pfc_get_pin_index(pfc, offset);
-	const struct sh_pfc_pin *pin = &pfc->info->pins[idx];
+	const struct sh_pfc_pin *pin = &pfc->info->pins[offset];
 	struct sh_pfc_pin_config *cfg = &pmx->configs[idx];
 	unsigned long flags;
 	unsigned int dir;
@@ -452,8 +452,7 @@ static const struct pinmux_ops sh_pfc_pi
 static bool sh_pfc_pinconf_validate(struct sh_pfc *pfc, unsigned int _pin,
 				    enum pin_config_param param)
 {
-	int idx = sh_pfc_get_pin_index(pfc, _pin);
-	const struct sh_pfc_pin *pin = &pfc->info->pins[idx];
+	const struct sh_pfc_pin *pin = &pfc->info->pins[_pin];
 
 	switch (param) {
 	case PIN_CONFIG_BIAS_DISABLE:

[PATCH v2 2/4] sh-pfc: handle pin array holes

[Sergei Shtylyov]

The pin array  handled by sh_pfc_init_ranges() and sh_pfc_map_pins() may contain
"holes" representing non-existing pins.  We have to first count  the pin ranges
and valid pins in order to calculate the size of the memory to be allocated,
then to skip over the non-existing pins when initializing the allocated arrays
(we still assume that a pin at index 0 always exists). We also have  to return
the number of valid pins  from sh_pfc_map_pins() instead of 0 on success.

As we have to touch devm_kzalloc() calls in sh_pfc_map_pins() anyway, use more
fitting devm_kcalloc() instead which additionally checks the array size.  And
since PINMUX_TYPE_NONE is #define'd as 0, stop re-initializing already zeroed
out 'pmx->configs' array.

While at it, add spaces around the minus signs in sh_pfc_init_ranges().

Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>

---
Changes in version 2:
- added the handling of "holes" to sh_pfc_init_ranges();
- renamed the 'n' local variable in sh_pfc_map_pins() to 'num_pins' and declared
  it on a separate line;
- moved the 'info' local variable inside  2nd loop in sh_pfc_map_pins();
- added comment before the 2nd devm_kcalloc() call in sh_pfc_map_pins();
- added comment to the 2nd loop in sh_pfc_map_pins();
- renamed and refreshed the patch.

 drivers/pinctrl/sh-pfc/core.c    |   29 +++++++++++++++++++----------
 drivers/pinctrl/sh-pfc/pinctrl.c |   29 +++++++++++++++++++----------
 2 files changed, 38 insertions(+), 20 deletions(-)

Index: linux-pinctrl/drivers/pinctrl/sh-pfc/core.c
=================================--- linux-pinctrl.orig/drivers/pinctrl/sh-pfc/core.c
+++ linux-pinctrl/drivers/pinctrl/sh-pfc/core.c
@@ -405,7 +405,10 @@ static int sh_pfc_init_ranges(struct sh_
 	 * last.
 	 */
 	for (i = 1, nr_ranges = 1; i < pfc->info->nr_pins; ++i) {
-		if (pfc->info->pins[i-1].pin != pfc->info->pins[i].pin - 1)
+		/* Check for the "holes" first */
+		if (!pfc->info->pins[i].enum_id && !pfc->info->pins[i].configs)
+			continue;
+		if (pfc->info->pins[i - 1].pin != pfc->info->pins[i].pin - 1)
 			nr_ranges++;
 	}
 
@@ -419,19 +422,25 @@ static int sh_pfc_init_ranges(struct sh_
 	range->start = pfc->info->pins[0].pin;
 
 	for (i = 1; i < pfc->info->nr_pins; ++i) {
-		if (pfc->info->pins[i-1].pin = pfc->info->pins[i].pin - 1)
+		if (pfc->info->pins[i - 1].pin = pfc->info->pins[i].pin - 1)
 			continue;
 
-		range->end = pfc->info->pins[i-1].pin;
-		if (!(pfc->info->pins[i-1].configs & SH_PFC_PIN_CFG_NO_GPIO))
-			pfc->nr_gpio_pins = range->end + 1;
-
-		range++;
-		range->start = pfc->info->pins[i].pin;
+		if (pfc->info->pins[i - 1].enum_id ||
+		    pfc->info->pins[i - 1].configs) {
+			range->end = pfc->info->pins[i - 1].pin;
+			if (!(pfc->info->pins[i - 1].configs &
+			      SH_PFC_PIN_CFG_NO_GPIO))
+				pfc->nr_gpio_pins = range->end + 1;
+		}
+
+		if (pfc->info->pins[i].enum_id || pfc->info->pins[i].configs) {
+			range++;
+			range->start = pfc->info->pins[i].pin;
+		}
 	}
 
-	range->end = pfc->info->pins[i-1].pin;
-	if (!(pfc->info->pins[i-1].configs & SH_PFC_PIN_CFG_NO_GPIO))
+	range->end = pfc->info->pins[i - 1].pin;
+	if (!(pfc->info->pins[i - 1].configs & SH_PFC_PIN_CFG_NO_GPIO))
 		pfc->nr_gpio_pins = range->end + 1;
 
 	return 0;
Index: linux-pinctrl/drivers/pinctrl/sh-pfc/pinctrl.c
=================================--- linux-pinctrl.orig/drivers/pinctrl/sh-pfc/pinctrl.c
+++ linux-pinctrl/drivers/pinctrl/sh-pfc/pinctrl.c
@@ -570,33 +570,42 @@ static const struct pinconf_ops sh_pfc_p
 /* PFC ranges -> pinctrl pin descs */
 static int sh_pfc_map_pins(struct sh_pfc *pfc, struct sh_pfc_pinctrl *pmx)
 {
+	struct pinctrl_pin_desc *pin;
+	unsigned int num_pins;
 	unsigned int i;
 
+	/* Count the valid pins. */
+	for (i = 0, num_pins = 0; i < pfc->info->nr_pins; i++) {
+		if (pfc->info->pins[i].enum_id || pfc->info->pins[i].configs)
+			num_pins++;
+	}
+
 	/* Allocate and initialize the pins and configs arrays. */
-	pmx->pins = devm_kzalloc(pfc->dev,
-				 sizeof(*pmx->pins) * pfc->info->nr_pins,
+	pmx->pins = devm_kcalloc(pfc->dev, num_pins, sizeof(*pmx->pins),
 				 GFP_KERNEL);
 	if (unlikely(!pmx->pins))
 		return -ENOMEM;
 
-	pmx->configs = devm_kzalloc(pfc->dev,
-				    sizeof(*pmx->configs) * pfc->info->nr_pins,
+	/* Equivalent to setting the type field to PINMUX_TYPE_NONE */
+	pmx->configs = devm_kcalloc(pfc->dev, num_pins, sizeof(*pmx->configs),
 				    GFP_KERNEL);
 	if (unlikely(!pmx->configs))
 		return -ENOMEM;
 
-	for (i = 0; i < pfc->info->nr_pins; ++i) {
+	for (i = 0, pin = pmx->pins; i < pfc->info->nr_pins; i++) {
 		const struct sh_pfc_pin *info = &pfc->info->pins[i];
-		struct sh_pfc_pin_config *cfg = &pmx->configs[i];
-		struct pinctrl_pin_desc *pin = &pmx->pins[i];
+
+		/* Skip the "hole" */
+		if (!info->enum_id && !info->configs)
+			continue;
 
 		/* If the pin number is equal to -1 all pins are considered */
 		pin->number = info->pin != (u16)-1 ? info->pin : i;
 		pin->name = info->name;
-		cfg->type = PINMUX_TYPE_NONE;
+		pin++;
 	}
 
-	return 0;
+	return num_pins;
 }
 
 int sh_pfc_register_pinctrl(struct sh_pfc *pfc)
@@ -621,7 +630,7 @@ int sh_pfc_register_pinctrl(struct sh_pf
 	pmx->pctl_desc.pmxops = &sh_pfc_pinmux_ops;
 	pmx->pctl_desc.confops = &sh_pfc_pinconf_ops;
 	pmx->pctl_desc.pins = pmx->pins;
-	pmx->pctl_desc.npins = pfc->info->nr_pins;
+	pmx->pctl_desc.npins = ret;
 
 	pmx->pctl = pinctrl_register(&pmx->pctl_desc, pfc->dev, pmx);
 	if (pmx->pctl = NULL)

[PATCH v2 2/4] sh-pfc: handle pin array holes

[Sergei Shtylyov]

The pin array  handled by sh_pfc_init_ranges() and sh_pfc_map_pins() may contain
"holes" representing non-existing pins.  We have to first count  the pin ranges
and valid pins in order to calculate the size of the memory to be allocated,
then to skip over the non-existing pins when initializing the allocated arrays
(we still assume that a pin at index 0 always exists). We also have  to return
the number of valid pins  from sh_pfc_map_pins() instead of 0 on success.

As we have to touch devm_kzalloc() calls in sh_pfc_map_pins() anyway, use more
fitting devm_kcalloc() instead which additionally checks the array size.  And
since PINMUX_TYPE_NONE is #define'd as 0, stop re-initializing already zeroed
out 'pmx->configs' array.

While at it, add spaces around the minus signs in sh_pfc_init_ranges().

Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>

---
Changes in version 2:
- added the handling of "holes" to sh_pfc_init_ranges();
- renamed the 'n' local variable in sh_pfc_map_pins() to 'num_pins' and declared
  it on a separate line;
- moved the 'info' local variable inside  2nd loop in sh_pfc_map_pins();
- added comment before the 2nd devm_kcalloc() call in sh_pfc_map_pins();
- added comment to the 2nd loop in sh_pfc_map_pins();
- renamed and refreshed the patch.

 drivers/pinctrl/sh-pfc/core.c    |   29 +++++++++++++++++++----------
 drivers/pinctrl/sh-pfc/pinctrl.c |   29 +++++++++++++++++++----------
 2 files changed, 38 insertions(+), 20 deletions(-)

Index: linux-pinctrl/drivers/pinctrl/sh-pfc/core.c
===================================================================
--- linux-pinctrl.orig/drivers/pinctrl/sh-pfc/core.c
+++ linux-pinctrl/drivers/pinctrl/sh-pfc/core.c
@@ -405,7 +405,10 @@ static int sh_pfc_init_ranges(struct sh_
 	 * last.
 	 */
 	for (i = 1, nr_ranges = 1; i < pfc->info->nr_pins; ++i) {
-		if (pfc->info->pins[i-1].pin != pfc->info->pins[i].pin - 1)
+		/* Check for the "holes" first */
+		if (!pfc->info->pins[i].enum_id && !pfc->info->pins[i].configs)
+			continue;
+		if (pfc->info->pins[i - 1].pin != pfc->info->pins[i].pin - 1)
 			nr_ranges++;
 	}
 
@@ -419,19 +422,25 @@ static int sh_pfc_init_ranges(struct sh_
 	range->start = pfc->info->pins[0].pin;
 
 	for (i = 1; i < pfc->info->nr_pins; ++i) {
-		if (pfc->info->pins[i-1].pin == pfc->info->pins[i].pin - 1)
+		if (pfc->info->pins[i - 1].pin == pfc->info->pins[i].pin - 1)
 			continue;
 
-		range->end = pfc->info->pins[i-1].pin;
-		if (!(pfc->info->pins[i-1].configs & SH_PFC_PIN_CFG_NO_GPIO))
-			pfc->nr_gpio_pins = range->end + 1;
-
-		range++;
-		range->start = pfc->info->pins[i].pin;
+		if (pfc->info->pins[i - 1].enum_id ||
+		    pfc->info->pins[i - 1].configs) {
+			range->end = pfc->info->pins[i - 1].pin;
+			if (!(pfc->info->pins[i - 1].configs &
+			      SH_PFC_PIN_CFG_NO_GPIO))
+				pfc->nr_gpio_pins = range->end + 1;
+		}
+
+		if (pfc->info->pins[i].enum_id || pfc->info->pins[i].configs) {
+			range++;
+			range->start = pfc->info->pins[i].pin;
+		}
 	}
 
-	range->end = pfc->info->pins[i-1].pin;
-	if (!(pfc->info->pins[i-1].configs & SH_PFC_PIN_CFG_NO_GPIO))
+	range->end = pfc->info->pins[i - 1].pin;
+	if (!(pfc->info->pins[i - 1].configs & SH_PFC_PIN_CFG_NO_GPIO))
 		pfc->nr_gpio_pins = range->end + 1;
 
 	return 0;
Index: linux-pinctrl/drivers/pinctrl/sh-pfc/pinctrl.c
===================================================================
--- linux-pinctrl.orig/drivers/pinctrl/sh-pfc/pinctrl.c
+++ linux-pinctrl/drivers/pinctrl/sh-pfc/pinctrl.c
@@ -570,33 +570,42 @@ static const struct pinconf_ops sh_pfc_p
 /* PFC ranges -> pinctrl pin descs */
 static int sh_pfc_map_pins(struct sh_pfc *pfc, struct sh_pfc_pinctrl *pmx)
 {
+	struct pinctrl_pin_desc *pin;
+	unsigned int num_pins;
 	unsigned int i;
 
+	/* Count the valid pins. */
+	for (i = 0, num_pins = 0; i < pfc->info->nr_pins; i++) {
+		if (pfc->info->pins[i].enum_id || pfc->info->pins[i].configs)
+			num_pins++;
+	}
+
 	/* Allocate and initialize the pins and configs arrays. */
-	pmx->pins = devm_kzalloc(pfc->dev,
-				 sizeof(*pmx->pins) * pfc->info->nr_pins,
+	pmx->pins = devm_kcalloc(pfc->dev, num_pins, sizeof(*pmx->pins),
 				 GFP_KERNEL);
 	if (unlikely(!pmx->pins))
 		return -ENOMEM;
 
-	pmx->configs = devm_kzalloc(pfc->dev,
-				    sizeof(*pmx->configs) * pfc->info->nr_pins,
+	/* Equivalent to setting the type field to PINMUX_TYPE_NONE */
+	pmx->configs = devm_kcalloc(pfc->dev, num_pins, sizeof(*pmx->configs),
 				    GFP_KERNEL);
 	if (unlikely(!pmx->configs))
 		return -ENOMEM;
 
-	for (i = 0; i < pfc->info->nr_pins; ++i) {
+	for (i = 0, pin = pmx->pins; i < pfc->info->nr_pins; i++) {
 		const struct sh_pfc_pin *info = &pfc->info->pins[i];
-		struct sh_pfc_pin_config *cfg = &pmx->configs[i];
-		struct pinctrl_pin_desc *pin = &pmx->pins[i];
+
+		/* Skip the "hole" */
+		if (!info->enum_id && !info->configs)
+			continue;
 
 		/* If the pin number is equal to -1 all pins are considered */
 		pin->number = info->pin != (u16)-1 ? info->pin : i;
 		pin->name = info->name;
-		cfg->type = PINMUX_TYPE_NONE;
+		pin++;
 	}
 
-	return 0;
+	return num_pins;
 }
 
 int sh_pfc_register_pinctrl(struct sh_pfc *pfc)
@@ -621,7 +630,7 @@ int sh_pfc_register_pinctrl(struct sh_pf
 	pmx->pctl_desc.pmxops = &sh_pfc_pinmux_ops;
 	pmx->pctl_desc.confops = &sh_pfc_pinconf_ops;
 	pmx->pctl_desc.pins = pmx->pins;
-	pmx->pctl_desc.npins = pfc->info->nr_pins;
+	pmx->pctl_desc.npins = ret;
 
 	pmx->pctl = pinctrl_register(&pmx->pctl_desc, pfc->dev, pmx);
 	if (pmx->pctl == NULL)

[PATCH v2 3/4] sh-pfc: r8a7790: remove non-existing GPIO pins

[Sergei Shtylyov]

From: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

GPIO banks 1 and 2 are missing pins 30 and 31. Remove them.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>

---
Changes in version 2:
- renamed the patch.

 drivers/pinctrl/sh-pfc/pfc-r8a7790.c |   21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)

Index: linux-pinctrl/drivers/pinctrl/sh-pfc/pfc-r8a7790.c
=================================--- linux-pinctrl.orig/drivers/pinctrl/sh-pfc/pfc-r8a7790.c
+++ linux-pinctrl/drivers/pinctrl/sh-pfc/pfc-r8a7790.c
@@ -27,10 +27,27 @@
 #include "core.h"
 #include "sh_pfc.h"
 
+#define PORT_GP_30(bank, fn, sfx)					\
+	PORT_GP_1(bank, 0,  fn, sfx), PORT_GP_1(bank, 1,  fn, sfx),	\
+	PORT_GP_1(bank, 2,  fn, sfx), PORT_GP_1(bank, 3,  fn, sfx),	\
+	PORT_GP_1(bank, 4,  fn, sfx), PORT_GP_1(bank, 5,  fn, sfx),	\
+	PORT_GP_1(bank, 6,  fn, sfx), PORT_GP_1(bank, 7,  fn, sfx),	\
+	PORT_GP_1(bank, 8,  fn, sfx), PORT_GP_1(bank, 9,  fn, sfx),	\
+	PORT_GP_1(bank, 10, fn, sfx), PORT_GP_1(bank, 11, fn, sfx),	\
+	PORT_GP_1(bank, 12, fn, sfx), PORT_GP_1(bank, 13, fn, sfx),	\
+	PORT_GP_1(bank, 14, fn, sfx), PORT_GP_1(bank, 15, fn, sfx),	\
+	PORT_GP_1(bank, 16, fn, sfx), PORT_GP_1(bank, 17, fn, sfx),	\
+	PORT_GP_1(bank, 18, fn, sfx), PORT_GP_1(bank, 19, fn, sfx),	\
+	PORT_GP_1(bank, 20, fn, sfx), PORT_GP_1(bank, 21, fn, sfx),	\
+	PORT_GP_1(bank, 22, fn, sfx), PORT_GP_1(bank, 23, fn, sfx),	\
+	PORT_GP_1(bank, 24, fn, sfx), PORT_GP_1(bank, 25, fn, sfx),	\
+	PORT_GP_1(bank, 26, fn, sfx), PORT_GP_1(bank, 27, fn, sfx),	\
+	PORT_GP_1(bank, 28, fn, sfx), PORT_GP_1(bank, 29, fn, sfx)
+
 #define CPU_ALL_PORT(fn, sfx)						\
 	PORT_GP_32(0, fn, sfx),						\
-	PORT_GP_32(1, fn, sfx),						\
-	PORT_GP_32(2, fn, sfx),						\
+	PORT_GP_30(1, fn, sfx),						\
+	PORT_GP_30(2, fn, sfx),						\
 	PORT_GP_32(3, fn, sfx),						\
 	PORT_GP_32(4, fn, sfx),						\
 	PORT_GP_32(5, fn, sfx)

[PATCH v2 3/4] sh-pfc: r8a7790: remove non-existing GPIO pins

[Sergei Shtylyov]

From: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

GPIO banks 1 and 2 are missing pins 30 and 31. Remove them.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>

---
Changes in version 2:
- renamed the patch.

 drivers/pinctrl/sh-pfc/pfc-r8a7790.c |   21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)

Index: linux-pinctrl/drivers/pinctrl/sh-pfc/pfc-r8a7790.c
===================================================================
--- linux-pinctrl.orig/drivers/pinctrl/sh-pfc/pfc-r8a7790.c
+++ linux-pinctrl/drivers/pinctrl/sh-pfc/pfc-r8a7790.c
@@ -27,10 +27,27 @@
 #include "core.h"
 #include "sh_pfc.h"
 
+#define PORT_GP_30(bank, fn, sfx)					\
+	PORT_GP_1(bank, 0,  fn, sfx), PORT_GP_1(bank, 1,  fn, sfx),	\
+	PORT_GP_1(bank, 2,  fn, sfx), PORT_GP_1(bank, 3,  fn, sfx),	\
+	PORT_GP_1(bank, 4,  fn, sfx), PORT_GP_1(bank, 5,  fn, sfx),	\
+	PORT_GP_1(bank, 6,  fn, sfx), PORT_GP_1(bank, 7,  fn, sfx),	\
+	PORT_GP_1(bank, 8,  fn, sfx), PORT_GP_1(bank, 9,  fn, sfx),	\
+	PORT_GP_1(bank, 10, fn, sfx), PORT_GP_1(bank, 11, fn, sfx),	\
+	PORT_GP_1(bank, 12, fn, sfx), PORT_GP_1(bank, 13, fn, sfx),	\
+	PORT_GP_1(bank, 14, fn, sfx), PORT_GP_1(bank, 15, fn, sfx),	\
+	PORT_GP_1(bank, 16, fn, sfx), PORT_GP_1(bank, 17, fn, sfx),	\
+	PORT_GP_1(bank, 18, fn, sfx), PORT_GP_1(bank, 19, fn, sfx),	\
+	PORT_GP_1(bank, 20, fn, sfx), PORT_GP_1(bank, 21, fn, sfx),	\
+	PORT_GP_1(bank, 22, fn, sfx), PORT_GP_1(bank, 23, fn, sfx),	\
+	PORT_GP_1(bank, 24, fn, sfx), PORT_GP_1(bank, 25, fn, sfx),	\
+	PORT_GP_1(bank, 26, fn, sfx), PORT_GP_1(bank, 27, fn, sfx),	\
+	PORT_GP_1(bank, 28, fn, sfx), PORT_GP_1(bank, 29, fn, sfx)
+
 #define CPU_ALL_PORT(fn, sfx)						\
 	PORT_GP_32(0, fn, sfx),						\
-	PORT_GP_32(1, fn, sfx),						\
-	PORT_GP_32(2, fn, sfx),						\
+	PORT_GP_30(1, fn, sfx),						\
+	PORT_GP_30(2, fn, sfx),						\
 	PORT_GP_32(3, fn, sfx),						\
 	PORT_GP_32(4, fn, sfx),						\
 	PORT_GP_32(5, fn, sfx)

[PATCH v2 4/4] sh-pfc: r8a7791 remove non-existing GPIO pins

[Sergei Shtylyov]

From: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

GPIO banks 1 and 7 are missing pins 26 to 31. Remove them.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>

---
Changes in version 2:
- renamed the patch.

 drivers/pinctrl/sh-pfc/pfc-r8a7791.c |   19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

Index: linux-pinctrl/drivers/pinctrl/sh-pfc/pfc-r8a7791.c
=================================--- linux-pinctrl.orig/drivers/pinctrl/sh-pfc/pfc-r8a7791.c
+++ linux-pinctrl/drivers/pinctrl/sh-pfc/pfc-r8a7791.c
@@ -14,15 +14,30 @@
 #include "core.h"
 #include "sh_pfc.h"
 
+#define PORT_GP_26(bank, fn, sfx)					\
+	PORT_GP_1(bank, 0,  fn, sfx), PORT_GP_1(bank, 1,  fn, sfx),	\
+	PORT_GP_1(bank, 2,  fn, sfx), PORT_GP_1(bank, 3,  fn, sfx),	\
+	PORT_GP_1(bank, 4,  fn, sfx), PORT_GP_1(bank, 5,  fn, sfx),	\
+	PORT_GP_1(bank, 6,  fn, sfx), PORT_GP_1(bank, 7,  fn, sfx),	\
+	PORT_GP_1(bank, 8,  fn, sfx), PORT_GP_1(bank, 9,  fn, sfx),	\
+	PORT_GP_1(bank, 10, fn, sfx), PORT_GP_1(bank, 11, fn, sfx),	\
+	PORT_GP_1(bank, 12, fn, sfx), PORT_GP_1(bank, 13, fn, sfx),	\
+	PORT_GP_1(bank, 14, fn, sfx), PORT_GP_1(bank, 15, fn, sfx),	\
+	PORT_GP_1(bank, 16, fn, sfx), PORT_GP_1(bank, 17, fn, sfx),	\
+	PORT_GP_1(bank, 18, fn, sfx), PORT_GP_1(bank, 19, fn, sfx),	\
+	PORT_GP_1(bank, 20, fn, sfx), PORT_GP_1(bank, 21, fn, sfx),	\
+	PORT_GP_1(bank, 22, fn, sfx), PORT_GP_1(bank, 23, fn, sfx),	\
+	PORT_GP_1(bank, 24, fn, sfx), PORT_GP_1(bank, 25, fn, sfx)
+
 #define CPU_ALL_PORT(fn, sfx)						\
 	PORT_GP_32(0, fn, sfx),						\
-	PORT_GP_32(1, fn, sfx),						\
+	PORT_GP_26(1, fn, sfx),						\
 	PORT_GP_32(2, fn, sfx),						\
 	PORT_GP_32(3, fn, sfx),						\
 	PORT_GP_32(4, fn, sfx),						\
 	PORT_GP_32(5, fn, sfx),						\
 	PORT_GP_32(6, fn, sfx),						\
-	PORT_GP_32(7, fn, sfx)
+	PORT_GP_26(7, fn, sfx)
 
 enum {
 	PINMUX_RESERVED = 0,

[PATCH v2 4/4] sh-pfc: r8a7791 remove non-existing GPIO pins

[Sergei Shtylyov]

From: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

GPIO banks 1 and 7 are missing pins 26 to 31. Remove them.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>

---
Changes in version 2:
- renamed the patch.

 drivers/pinctrl/sh-pfc/pfc-r8a7791.c |   19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

Index: linux-pinctrl/drivers/pinctrl/sh-pfc/pfc-r8a7791.c
===================================================================
--- linux-pinctrl.orig/drivers/pinctrl/sh-pfc/pfc-r8a7791.c
+++ linux-pinctrl/drivers/pinctrl/sh-pfc/pfc-r8a7791.c
@@ -14,15 +14,30 @@
 #include "core.h"
 #include "sh_pfc.h"
 
+#define PORT_GP_26(bank, fn, sfx)					\
+	PORT_GP_1(bank, 0,  fn, sfx), PORT_GP_1(bank, 1,  fn, sfx),	\
+	PORT_GP_1(bank, 2,  fn, sfx), PORT_GP_1(bank, 3,  fn, sfx),	\
+	PORT_GP_1(bank, 4,  fn, sfx), PORT_GP_1(bank, 5,  fn, sfx),	\
+	PORT_GP_1(bank, 6,  fn, sfx), PORT_GP_1(bank, 7,  fn, sfx),	\
+	PORT_GP_1(bank, 8,  fn, sfx), PORT_GP_1(bank, 9,  fn, sfx),	\
+	PORT_GP_1(bank, 10, fn, sfx), PORT_GP_1(bank, 11, fn, sfx),	\
+	PORT_GP_1(bank, 12, fn, sfx), PORT_GP_1(bank, 13, fn, sfx),	\
+	PORT_GP_1(bank, 14, fn, sfx), PORT_GP_1(bank, 15, fn, sfx),	\
+	PORT_GP_1(bank, 16, fn, sfx), PORT_GP_1(bank, 17, fn, sfx),	\
+	PORT_GP_1(bank, 18, fn, sfx), PORT_GP_1(bank, 19, fn, sfx),	\
+	PORT_GP_1(bank, 20, fn, sfx), PORT_GP_1(bank, 21, fn, sfx),	\
+	PORT_GP_1(bank, 22, fn, sfx), PORT_GP_1(bank, 23, fn, sfx),	\
+	PORT_GP_1(bank, 24, fn, sfx), PORT_GP_1(bank, 25, fn, sfx)
+
 #define CPU_ALL_PORT(fn, sfx)						\
 	PORT_GP_32(0, fn, sfx),						\
-	PORT_GP_32(1, fn, sfx),						\
+	PORT_GP_26(1, fn, sfx),						\
 	PORT_GP_32(2, fn, sfx),						\
 	PORT_GP_32(3, fn, sfx),						\
 	PORT_GP_32(4, fn, sfx),						\
 	PORT_GP_32(5, fn, sfx),						\
 	PORT_GP_32(6, fn, sfx),						\
-	PORT_GP_32(7, fn, sfx)
+	PORT_GP_26(7, fn, sfx)
 
 enum {
 	PINMUX_RESERVED = 0,

Re: [PATCH v2 0/4] sh-pfc: handle pin array holes in sh_pfc_map_pins()

[Sergei Shtylyov]

    Oops, the subject should have been "[PATCH v2 0/4] Remove non-existing 
GPIO pins for R8A7790/1".

WBR, Sergei

Re: [PATCH v2 0/4] sh-pfc: handle pin array holes in sh_pfc_map_pins()

[Sergei Shtylyov]

    Oops, the subject should have been "[PATCH v2 0/4] Remove non-existing 
GPIO pins for R8A7790/1".

WBR, Sergei

Re: [PATCH v2 0/4] sh-pfc: handle pin array holes in sh_pfc_map_pins()

[Linus Walleij]

On Fri, Jun 5, 2015 at 1:23 AM, Sergei Shtylyov
<sergei.shtylyov@cogentembedded.com> wrote:

> Hello.
>
>    Here's the set of 4 patches against the 'devel' branch of Linus Walleij's
> 'linux-pinctrl.git' repo. Here we add proper handling of the pin array "holes"
> and then remove unused GPIO pins for R8A7790/1 SoCs.
>
> [1/4] sh-pfc: do not call sh_pfc_get_pin_index() when unnecessary
> [2/4] sh-pfc: handle pin array holes
> [3/4] sh-pfc: r8a7790 remove non-existing GPIO pins
> [4/4] sh-pfc: r8a7791 remove non-existing GPIO pins

Waiting for Laurent to review this series.

Yours,
Linus Walleij

Re: [PATCH v2 0/4] sh-pfc: handle pin array holes in sh_pfc_map_pins()

[Linus Walleij]

On Fri, Jun 5, 2015 at 1:23 AM, Sergei Shtylyov
<sergei.shtylyov@cogentembedded.com> wrote:

> Hello.
>
>    Here's the set of 4 patches against the 'devel' branch of Linus Walleij's
> 'linux-pinctrl.git' repo. Here we add proper handling of the pin array "holes"
> and then remove unused GPIO pins for R8A7790/1 SoCs.
>
> [1/4] sh-pfc: do not call sh_pfc_get_pin_index() when unnecessary
> [2/4] sh-pfc: handle pin array holes
> [3/4] sh-pfc: r8a7790 remove non-existing GPIO pins
> [4/4] sh-pfc: r8a7791 remove non-existing GPIO pins

Waiting for Laurent to review this series.

Yours,
Linus Walleij

Re: [PATCH v2 0/4] sh-pfc: handle pin array holes in sh_pfc_map_pins()

[Sergei Shtylyov]

Hello.

On 6/10/2015 10:49 AM, Linus Walleij wrote:

>>     Here's the set of 4 patches against the 'devel' branch of Linus Walleij's
>> 'linux-pinctrl.git' repo. Here we add proper handling of the pin array "holes"
>> and then remove unused GPIO pins for R8A7790/1 SoCs.

>> [1/4] sh-pfc: do not call sh_pfc_get_pin_index() when unnecessary
>> [2/4] sh-pfc: handle pin array holes
>> [3/4] sh-pfc: r8a7790 remove non-existing GPIO pins
>> [4/4] sh-pfc: r8a7791 remove non-existing GPIO pins

> Waiting for Laurent to review this series.

    Me too. :-)
    Note that the last 2 patches are Laurent's.

> Yours,
> Linus Walleij

WBR, Sergei

Re: [PATCH v2 0/4] sh-pfc: handle pin array holes in sh_pfc_map_pins()

[Sergei Shtylyov]

Hello.

On 6/10/2015 10:49 AM, Linus Walleij wrote:

>>     Here's the set of 4 patches against the 'devel' branch of Linus Walleij's
>> 'linux-pinctrl.git' repo. Here we add proper handling of the pin array "holes"
>> and then remove unused GPIO pins for R8A7790/1 SoCs.

>> [1/4] sh-pfc: do not call sh_pfc_get_pin_index() when unnecessary
>> [2/4] sh-pfc: handle pin array holes
>> [3/4] sh-pfc: r8a7790 remove non-existing GPIO pins
>> [4/4] sh-pfc: r8a7791 remove non-existing GPIO pins

> Waiting for Laurent to review this series.

    Me too. :-)
    Note that the last 2 patches are Laurent's.

> Yours,
> Linus Walleij

WBR, Sergei

Re: [PATCH v2 1/4] sh-pfc: do not call sh_pfc_get_pin_index() when unnecessary

[Laurent Pinchart]

Hi Sergei,

Thank you for the patch.

On Thursday 04 June 2015 16:25:01 Sergei Shtylyov wrote:
> Calling sh_pfc_get_pin_index()  to calculate a pin index based on the
> collected pin range data is unnecessary when we're dealing with
> 'pfc->info->pins' and 'chip->pins' arrays as those always reperesent the
> pins starting from index 0 sequentially. Being a  mere  optimization at
> this time, this change will become crucial when we'll allow the "holes" in
> those arrays...

Pin information is stored in per-SoC pins arrays (a.k.a. pfc->info->pins). The 
driver support two models to number pins:

- The sequential model, in which pins are numbered sequentially starting at 0. 
Pin numbers are equal to the index in the array.

- The explicit numbering model, in which each pin entry has an explicit number 
(stored in struct sh_pfc_pin.pin). Pins numbers are not necessarily equal to 
the index of the pin entry in the array.

The sh_pfc_get_pin_index() function converts a pin number to the pin index in 
the pins array.

Let's consider the sh_pfc_pinconf_validate() from which your patch removes the 
call to sh_pfc_get_pin_index() and uses the pin number directly. The function 
is called from the .pin_config_get() and .pin_config_set() handlers. One 
possible call path is

pinconf_pins_show() -> pinconf_dump_pin() -> pinconf_dump_pins() -> 
pinconf_generic_dump_pins() -> pinconf_generic_dump_one() -> 
pin_config_get_for_pin() -> .pin_config_get()

The pin value passed to the .pin_config_get() function is pctldev->desc-
>pins[i].number, which is the pin number, not its index. It thus looks like 
this patch introduces a bug.

There might be something obvious I'm not getting though, so please feel free 
to prove me wrong.

> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
> 
> ---
> Changes in version 2:
> - new patch.
> 
>  drivers/pinctrl/sh-pfc/gpio.c    |    6 ++----
>  drivers/pinctrl/sh-pfc/pinctrl.c |    7 +++----
>  2 files changed, 5 insertions(+), 8 deletions(-)
> 
> Index: linux-pinctrl/drivers/pinctrl/sh-pfc/gpio.c
> =================================> --- linux-pinctrl.orig/drivers/pinctrl/sh-pfc/gpio.c
> +++ linux-pinctrl/drivers/pinctrl/sh-pfc/gpio.c
> @@ -52,8 +52,7 @@ static void gpio_get_data_reg(struct sh_
>  			      struct sh_pfc_gpio_data_reg **reg,
>  			      unsigned int *bit)
>  {
> -	int idx = sh_pfc_get_pin_index(chip->pfc, offset);
> -	struct sh_pfc_gpio_pin *gpio_pin = &chip->pins[idx];
> +	struct sh_pfc_gpio_pin *gpio_pin = &chip->pins[offset];
> 
>  	*reg = &chip->regs[gpio_pin->dreg];
>  	*bit = gpio_pin->dbit;
> @@ -138,9 +137,8 @@ static int gpio_setup_data_regs(struct s
>  static int gpio_pin_request(struct gpio_chip *gc, unsigned offset)
>  {
>  	struct sh_pfc *pfc = gpio_to_pfc(gc);
> -	int idx = sh_pfc_get_pin_index(pfc, offset);
> 
> -	if (idx < 0 || pfc->info->pins[idx].enum_id = 0)
> +	if (pfc->info->pins[offset].enum_id = 0)
>  		return -EINVAL;
> 
>  	return pinctrl_request_gpio(offset);
> Index: linux-pinctrl/drivers/pinctrl/sh-pfc/pinctrl.c
> =================================> --- linux-pinctrl.orig/drivers/pinctrl/sh-pfc/pinctrl.c
> +++ linux-pinctrl/drivers/pinctrl/sh-pfc/pinctrl.c
> @@ -370,7 +370,7 @@ static int sh_pfc_gpio_request_enable(st
>  		/* If GPIOs are handled externally the pin mux type need to be
>  		 * set to GPIO here.
>  		 */
> -		const struct sh_pfc_pin *pin = &pfc->info->pins[idx];
> +		const struct sh_pfc_pin *pin = &pfc->info->pins[offset];
> 
>  		ret = sh_pfc_config_mux(pfc, pin->enum_id, PINMUX_TYPE_GPIO);
>  		if (ret < 0)
> @@ -410,7 +410,7 @@ static int sh_pfc_gpio_set_direction(str
>  	struct sh_pfc *pfc = pmx->pfc;
>  	int new_type = input ? PINMUX_TYPE_INPUT : PINMUX_TYPE_OUTPUT;
>  	int idx = sh_pfc_get_pin_index(pfc, offset);
> -	const struct sh_pfc_pin *pin = &pfc->info->pins[idx];
> +	const struct sh_pfc_pin *pin = &pfc->info->pins[offset];
>  	struct sh_pfc_pin_config *cfg = &pmx->configs[idx];
>  	unsigned long flags;
>  	unsigned int dir;
> @@ -452,8 +452,7 @@ static const struct pinmux_ops sh_pfc_pi
>  static bool sh_pfc_pinconf_validate(struct sh_pfc *pfc, unsigned int _pin,
>  				    enum pin_config_param param)
>  {
> -	int idx = sh_pfc_get_pin_index(pfc, _pin);
> -	const struct sh_pfc_pin *pin = &pfc->info->pins[idx];
> +	const struct sh_pfc_pin *pin = &pfc->info->pins[_pin];
> 
>  	switch (param) {
>  	case PIN_CONFIG_BIAS_DISABLE:

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 1/4] sh-pfc: do not call sh_pfc_get_pin_index() when unnecessary

[Laurent Pinchart]

Hi Sergei,

Thank you for the patch.

On Thursday 04 June 2015 16:25:01 Sergei Shtylyov wrote:
> Calling sh_pfc_get_pin_index()  to calculate a pin index based on the
> collected pin range data is unnecessary when we're dealing with
> 'pfc->info->pins' and 'chip->pins' arrays as those always reperesent the
> pins starting from index 0 sequentially. Being a  mere  optimization at
> this time, this change will become crucial when we'll allow the "holes" in
> those arrays...

Pin information is stored in per-SoC pins arrays (a.k.a. pfc->info->pins). The 
driver support two models to number pins:

- The sequential model, in which pins are numbered sequentially starting at 0. 
Pin numbers are equal to the index in the array.

- The explicit numbering model, in which each pin entry has an explicit number 
(stored in struct sh_pfc_pin.pin). Pins numbers are not necessarily equal to 
the index of the pin entry in the array.

The sh_pfc_get_pin_index() function converts a pin number to the pin index in 
the pins array.

Let's consider the sh_pfc_pinconf_validate() from which your patch removes the 
call to sh_pfc_get_pin_index() and uses the pin number directly. The function 
is called from the .pin_config_get() and .pin_config_set() handlers. One 
possible call path is

pinconf_pins_show() -> pinconf_dump_pin() -> pinconf_dump_pins() -> 
pinconf_generic_dump_pins() -> pinconf_generic_dump_one() -> 
pin_config_get_for_pin() -> .pin_config_get()

The pin value passed to the .pin_config_get() function is pctldev->desc-
>pins[i].number, which is the pin number, not its index. It thus looks like 
this patch introduces a bug.

There might be something obvious I'm not getting though, so please feel free 
to prove me wrong.

> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
> 
> ---
> Changes in version 2:
> - new patch.
> 
>  drivers/pinctrl/sh-pfc/gpio.c    |    6 ++----
>  drivers/pinctrl/sh-pfc/pinctrl.c |    7 +++----
>  2 files changed, 5 insertions(+), 8 deletions(-)
> 
> Index: linux-pinctrl/drivers/pinctrl/sh-pfc/gpio.c
> ===================================================================
> --- linux-pinctrl.orig/drivers/pinctrl/sh-pfc/gpio.c
> +++ linux-pinctrl/drivers/pinctrl/sh-pfc/gpio.c
> @@ -52,8 +52,7 @@ static void gpio_get_data_reg(struct sh_
>  			      struct sh_pfc_gpio_data_reg **reg,
>  			      unsigned int *bit)
>  {
> -	int idx = sh_pfc_get_pin_index(chip->pfc, offset);
> -	struct sh_pfc_gpio_pin *gpio_pin = &chip->pins[idx];
> +	struct sh_pfc_gpio_pin *gpio_pin = &chip->pins[offset];
> 
>  	*reg = &chip->regs[gpio_pin->dreg];
>  	*bit = gpio_pin->dbit;
> @@ -138,9 +137,8 @@ static int gpio_setup_data_regs(struct s
>  static int gpio_pin_request(struct gpio_chip *gc, unsigned offset)
>  {
>  	struct sh_pfc *pfc = gpio_to_pfc(gc);
> -	int idx = sh_pfc_get_pin_index(pfc, offset);
> 
> -	if (idx < 0 || pfc->info->pins[idx].enum_id == 0)
> +	if (pfc->info->pins[offset].enum_id == 0)
>  		return -EINVAL;
> 
>  	return pinctrl_request_gpio(offset);
> Index: linux-pinctrl/drivers/pinctrl/sh-pfc/pinctrl.c
> ===================================================================
> --- linux-pinctrl.orig/drivers/pinctrl/sh-pfc/pinctrl.c
> +++ linux-pinctrl/drivers/pinctrl/sh-pfc/pinctrl.c
> @@ -370,7 +370,7 @@ static int sh_pfc_gpio_request_enable(st
>  		/* If GPIOs are handled externally the pin mux type need to be
>  		 * set to GPIO here.
>  		 */
> -		const struct sh_pfc_pin *pin = &pfc->info->pins[idx];
> +		const struct sh_pfc_pin *pin = &pfc->info->pins[offset];
> 
>  		ret = sh_pfc_config_mux(pfc, pin->enum_id, PINMUX_TYPE_GPIO);
>  		if (ret < 0)
> @@ -410,7 +410,7 @@ static int sh_pfc_gpio_set_direction(str
>  	struct sh_pfc *pfc = pmx->pfc;
>  	int new_type = input ? PINMUX_TYPE_INPUT : PINMUX_TYPE_OUTPUT;
>  	int idx = sh_pfc_get_pin_index(pfc, offset);
> -	const struct sh_pfc_pin *pin = &pfc->info->pins[idx];
> +	const struct sh_pfc_pin *pin = &pfc->info->pins[offset];
>  	struct sh_pfc_pin_config *cfg = &pmx->configs[idx];
>  	unsigned long flags;
>  	unsigned int dir;
> @@ -452,8 +452,7 @@ static const struct pinmux_ops sh_pfc_pi
>  static bool sh_pfc_pinconf_validate(struct sh_pfc *pfc, unsigned int _pin,
>  				    enum pin_config_param param)
>  {
> -	int idx = sh_pfc_get_pin_index(pfc, _pin);
> -	const struct sh_pfc_pin *pin = &pfc->info->pins[idx];
> +	const struct sh_pfc_pin *pin = &pfc->info->pins[_pin];
> 
>  	switch (param) {
>  	case PIN_CONFIG_BIAS_DISABLE:

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 1/4] sh-pfc: do not call sh_pfc_get_pin_index() when unnecessary

[Sergei Shtylyov]

Hello.

On 06/18/2015 10:05 PM, Laurent Pinchart wrote:

>> Calling sh_pfc_get_pin_index()  to calculate a pin index based on the
>> collected pin range data is unnecessary when we're dealing with
>> 'pfc->info->pins' and 'chip->pins' arrays as those always reperesent the
>> pins starting from index 0 sequentially. Being a  mere  optimization at
>> this time, this change will become crucial when we'll allow the "holes" in
>> those arrays...

> Pin information is stored in per-SoC pins arrays (a.k.a. pfc->info->pins). The
> driver support two models to number pins:

> - The sequential model, in which pins are numbered sequentially starting at 0.
> Pin numbers are equal to the index in the array.

    And I didn't touch this case.

> - The explicit numbering model, in which each pin entry has an explicit number
> (stored in struct sh_pfc_pin.pin). Pins numbers are not necessarily equal to
> the index of the pin entry in the array.

    Ah... I was just looking at _GP_GPIO() which still assigns sequential pin 
#'s equal to the indices.

> The sh_pfc_get_pin_index() function converts a pin number to the pin index in
> the pins array.

> Let's consider the sh_pfc_pinconf_validate() from which your patch removes the
> call to sh_pfc_get_pin_index() and uses the pin number directly. The function
> is called from the .pin_config_get() and .pin_config_set() handlers. One
> possible call path is

> pinconf_pins_show() -> pinconf_dump_pin() -> pinconf_dump_pins() ->
> pinconf_generic_dump_pins() -> pinconf_generic_dump_one() ->
> pin_config_get_for_pin() -> .pin_config_get()

> The pin value passed to the .pin_config_get() function is pctldev->desc->
> pins[i].number, which is the pin number, not its index. It thus looks like
> this patch introduces a bug.

> There might be something obvious I'm not getting though, so please feel free
> to prove me wrong.

    The bug seems more like theoretical one at this point (unless you have the 
examples with non-sequential pin #'s)...

WBR, Sergei

--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in

Re: [PATCH v2 1/4] sh-pfc: do not call sh_pfc_get_pin_index() when unnecessary

[Sergei Shtylyov]

Hello.

On 06/18/2015 10:05 PM, Laurent Pinchart wrote:

>> Calling sh_pfc_get_pin_index()  to calculate a pin index based on the
>> collected pin range data is unnecessary when we're dealing with
>> 'pfc->info->pins' and 'chip->pins' arrays as those always reperesent the
>> pins starting from index 0 sequentially. Being a  mere  optimization at
>> this time, this change will become crucial when we'll allow the "holes" in
>> those arrays...

> Pin information is stored in per-SoC pins arrays (a.k.a. pfc->info->pins). The
> driver support two models to number pins:

> - The sequential model, in which pins are numbered sequentially starting at 0.
> Pin numbers are equal to the index in the array.

    And I didn't touch this case.

> - The explicit numbering model, in which each pin entry has an explicit number
> (stored in struct sh_pfc_pin.pin). Pins numbers are not necessarily equal to
> the index of the pin entry in the array.

    Ah... I was just looking at _GP_GPIO() which still assigns sequential pin 
#'s equal to the indices.

> The sh_pfc_get_pin_index() function converts a pin number to the pin index in
> the pins array.

> Let's consider the sh_pfc_pinconf_validate() from which your patch removes the
> call to sh_pfc_get_pin_index() and uses the pin number directly. The function
> is called from the .pin_config_get() and .pin_config_set() handlers. One
> possible call path is

> pinconf_pins_show() -> pinconf_dump_pin() -> pinconf_dump_pins() ->
> pinconf_generic_dump_pins() -> pinconf_generic_dump_one() ->
> pin_config_get_for_pin() -> .pin_config_get()

> The pin value passed to the .pin_config_get() function is pctldev->desc->
> pins[i].number, which is the pin number, not its index. It thus looks like
> this patch introduces a bug.

> There might be something obvious I'm not getting though, so please feel free
> to prove me wrong.

    The bug seems more like theoretical one at this point (unless you have the 
examples with non-sequential pin #'s)...

WBR, Sergei

--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in

Re: [PATCH v2 1/4] sh-pfc: do not call sh_pfc_get_pin_index() when unnecessary

[Sergei Shtylyov]

Hello.

On 06/23/2015 01:42 AM, Sergei Shtylyov wrote:

>>> Calling sh_pfc_get_pin_index()  to calculate a pin index based on the
>>> collected pin range data is unnecessary when we're dealing with
>>> 'pfc->info->pins' and 'chip->pins' arrays as those always reperesent the
>>> pins starting from index 0 sequentially. Being a  mere  optimization at
>>> this time, this change will become crucial when we'll allow the "holes" in
>>> those arrays...

>> Pin information is stored in per-SoC pins arrays (a.k.a. pfc->info->pins). The
>> driver support two models to number pins:

>> - The sequential model, in which pins are numbered sequentially starting at 0.
>> Pin numbers are equal to the index in the array.

>     And I didn't touch this case.

>> - The explicit numbering model, in which each pin entry has an explicit number
>> (stored in struct sh_pfc_pin.pin). Pins numbers are not necessarily equal to
>> the index of the pin entry in the array.

>     Ah... I was just looking at _GP_GPIO() which still assigns sequential pin
> #'s equal to the indices.

    And forgot about SH_PFC_PIN_NAMED()...

>> The sh_pfc_get_pin_index() function converts a pin number to the pin index in
>> the pins array.

>> Let's consider the sh_pfc_pinconf_validate() from which your patch removes the
>> call to sh_pfc_get_pin_index() and uses the pin number directly. The function
>> is called from the .pin_config_get() and .pin_config_set() handlers. One
>> possible call path is

>> pinconf_pins_show() -> pinconf_dump_pin() -> pinconf_dump_pins() ->
>> pinconf_generic_dump_pins() -> pinconf_generic_dump_one() ->
>> pin_config_get_for_pin() -> .pin_config_get()

>> The pin value passed to the .pin_config_get() function is pctldev->desc->
>> pins[i].number, which is the pin number, not its index. It thus looks like
>> this patch introduces a bug.

>> There might be something obvious I'm not getting though, so please feel free
>> to prove me wrong.

>     The bug seems more like theoretical one at this point (unless you have the
> examples with non-sequential pin #'s)...

    No, with non-GPIO pins it seems to be an actual bug.

    What if we remove the explicit array index from _GP_GPIO() and so don't 
have the holes at all?

WBR, Sergei

Re: [PATCH v2 1/4] sh-pfc: do not call sh_pfc_get_pin_index() when unnecessary

[Sergei Shtylyov]

Hello.

On 06/23/2015 01:42 AM, Sergei Shtylyov wrote:

>>> Calling sh_pfc_get_pin_index()  to calculate a pin index based on the
>>> collected pin range data is unnecessary when we're dealing with
>>> 'pfc->info->pins' and 'chip->pins' arrays as those always reperesent the
>>> pins starting from index 0 sequentially. Being a  mere  optimization at
>>> this time, this change will become crucial when we'll allow the "holes" in
>>> those arrays...

>> Pin information is stored in per-SoC pins arrays (a.k.a. pfc->info->pins). The
>> driver support two models to number pins:

>> - The sequential model, in which pins are numbered sequentially starting at 0.
>> Pin numbers are equal to the index in the array.

>     And I didn't touch this case.

>> - The explicit numbering model, in which each pin entry has an explicit number
>> (stored in struct sh_pfc_pin.pin). Pins numbers are not necessarily equal to
>> the index of the pin entry in the array.

>     Ah... I was just looking at _GP_GPIO() which still assigns sequential pin
> #'s equal to the indices.

    And forgot about SH_PFC_PIN_NAMED()...

>> The sh_pfc_get_pin_index() function converts a pin number to the pin index in
>> the pins array.

>> Let's consider the sh_pfc_pinconf_validate() from which your patch removes the
>> call to sh_pfc_get_pin_index() and uses the pin number directly. The function
>> is called from the .pin_config_get() and .pin_config_set() handlers. One
>> possible call path is

>> pinconf_pins_show() -> pinconf_dump_pin() -> pinconf_dump_pins() ->
>> pinconf_generic_dump_pins() -> pinconf_generic_dump_one() ->
>> pin_config_get_for_pin() -> .pin_config_get()

>> The pin value passed to the .pin_config_get() function is pctldev->desc->
>> pins[i].number, which is the pin number, not its index. It thus looks like
>> this patch introduces a bug.

>> There might be something obvious I'm not getting though, so please feel free
>> to prove me wrong.

>     The bug seems more like theoretical one at this point (unless you have the
> examples with non-sequential pin #'s)...

    No, with non-GPIO pins it seems to be an actual bug.

    What if we remove the explicit array index from _GP_GPIO() and so don't 
have the holes at all?

WBR, Sergei