[PATCH v4 0/3] Fix pinctrl-single pcs_pin_dbg_show()

These patches fix the pcs_pin_dbg_show() function for the scenario where
a single register controls multiple pins (i.e. bits_per_mux is not zero)
Additionally, the common formula is moved to a separate function to
allow reuse.

Changes since v3:
-----------------
- define and set variable 'mux_bytes' in one line
- update commit message

Changes since v2:
-----------------
- move read() register to be outside of if condition (as it common
  read()).
- Remove extra parentheses
- replace offset variable by direct return statements

Changes since v1:
-----------------
- remove unused variable in In function 'pcs_allocate_pin_table'
  (Reported-by: kernel test robot [off-list ref])

Hanna Hawa (3):
  pinctrl: pinctrl-single: remove unused variable
  pinctrl: pinctrl-single: remove unused parameter
  pinctrl: pinctrl-single: fix pcs_pin_dbg_show() when bits_per_mux is
    not zero

 drivers/pinctrl/pinctrl-single.c | 65 ++++++++++++++++++--------------
 1 file changed, 37 insertions(+), 28 deletions(-)

-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Remove unused parameter 'pin_pos' from pcs_add_pin().

Signed-off-by: Hanna Hawa <redacted>
---
 drivers/pinctrl/pinctrl-single.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-single.c b/drivers/pinctrl/pinctrl-single.c
index 91c638b85d2c..f3394517cb2e 100644
--- a/drivers/pinctrl/pinctrl-single.c
+++ b/drivers/pinctrl/pinctrl-single.c

@@ -656,10 +656,8 @@ static const struct pinconf_ops pcs_pinconf_ops = {
  * pcs_add_pin() - add a pin to the static per controller pin array
  * @pcs: pcs driver instance
  * @offset: register offset from base
- * @pin_pos: unused
  */
-static int pcs_add_pin(struct pcs_device *pcs, unsigned offset,
-		unsigned pin_pos)
+static int pcs_add_pin(struct pcs_device *pcs, unsigned int offset)
 {
 	struct pcs_soc_data *pcs_soc = &pcs->socdata;
 	struct pinctrl_pin_desc *pin;

@@ -727,16 +725,14 @@ static int pcs_allocate_pin_table(struct pcs_device *pcs)
 		unsigned offset;
 		int res;
 		int byte_num;
-		int pin_pos = 0;
 
 		if (pcs->bits_per_mux) {
 			byte_num = (pcs->bits_per_pin * i) / BITS_PER_BYTE;
 			offset = (byte_num / mux_bytes) * mux_bytes;
-			pin_pos = i % num_pins_in_register;
 		} else {
 			offset = i * mux_bytes;
 		}
-		res = pcs_add_pin(pcs, offset, pin_pos);
+		res = pcs_add_pin(pcs, offset);
 		if (res < 0) {
 			dev_err(pcs->dev, "error adding pins: %i\n", res);
 			return res;

-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

* Hanna Hawa [off-list ref] [700101 02:00]:

Remove unused parameter 'pin_pos' from pcs_add_pin().

Reviewed-by: Tony Lindgren <tony@atomide.com>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Remove unused parameter 'num_pins_in_register' from
pcs_allocate_pin_table().

Reported-by: kernel test robot <redacted>
Signed-off-by: Hanna Hawa <redacted>
---
 drivers/pinctrl/pinctrl-single.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-single.c b/drivers/pinctrl/pinctrl-single.c
index 7771316dfffa..91c638b85d2c 100644
--- a/drivers/pinctrl/pinctrl-single.c
+++ b/drivers/pinctrl/pinctrl-single.c

@@ -703,14 +703,12 @@ static int pcs_add_pin(struct pcs_device *pcs, unsigned offset,
 static int pcs_allocate_pin_table(struct pcs_device *pcs)
 {
 	int mux_bytes, nr_pins, i;
-	int num_pins_in_register = 0;
 
 	mux_bytes = pcs->width / BITS_PER_BYTE;
 
 	if (pcs->bits_per_mux) {
 		pcs->bits_per_pin = fls(pcs->fmask);
 		nr_pins = (pcs->size * BITS_PER_BYTE) / pcs->bits_per_pin;
-		num_pins_in_register = pcs->width / pcs->bits_per_pin;
 	} else {
 		nr_pins = pcs->size / mux_bytes;
 	}

-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

* Hanna Hawa [off-list ref] [700101 02:00]:

Remove unused parameter 'num_pins_in_register' from
pcs_allocate_pin_table().

Reviewed-by: Tony Lindgren <tony@atomide.com>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

A System Error (SError, followed by kernel panic) was detected when
trying to print the supported pins in a pinctrl device which supports
multiple pins per register. This change fixes the pcs_pin_dbg_show() in
pinctrl-single driver when bits_per_mux is not zero. In addition move
offset calculation and pin offset in register to common function.

Fixes: 4e7e8017a80e ("pinctrl: pinctrl-single: enhance to configure multiple pins of different modules")
Signed-off-by: Hanna Hawa <redacted>
Reviewed-by: Andy Shevchenko <redacted>
---
 drivers/pinctrl/pinctrl-single.c | 55 ++++++++++++++++++++------------
 1 file changed, 35 insertions(+), 20 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-single.c b/drivers/pinctrl/pinctrl-single.c
index f3394517cb2e..39aac32ed09c 100644
--- a/drivers/pinctrl/pinctrl-single.c
+++ b/drivers/pinctrl/pinctrl-single.c

@@ -270,20 +270,44 @@ static void __maybe_unused pcs_writel(unsigned val, void __iomem *reg)
 	writel(val, reg);
 }
 
+static unsigned int pcs_pin_reg_offset_get(struct pcs_device *pcs,
+					   unsigned int pin)
+{
+	unsigned int mux_bytes = pcs->width / BITS_PER_BYTE;
+
+	if (pcs->bits_per_mux) {
+		unsigned int pin_offset_bytes;
+
+		pin_offset_bytes = (pcs->bits_per_pin * pin) / BITS_PER_BYTE;
+		return (pin_offset_bytes / mux_bytes) * mux_bytes;
+	}
+
+	return pin * mux_bytes;
+}
+
+static unsigned int pcs_pin_shift_reg_get(struct pcs_device *pcs,
+					  unsigned int pin)
+{
+	return (pin % (pcs->width / pcs->bits_per_pin)) * pcs->bits_per_pin;
+}
+
 static void pcs_pin_dbg_show(struct pinctrl_dev *pctldev,
 					struct seq_file *s,
 					unsigned pin)
 {
 	struct pcs_device *pcs;
-	unsigned val, mux_bytes;
+	unsigned int val;
 	unsigned long offset;
 	size_t pa;
 
 	pcs = pinctrl_dev_get_drvdata(pctldev);
 
-	mux_bytes = pcs->width / BITS_PER_BYTE;
-	offset = pin * mux_bytes;
+	offset = pcs_pin_reg_offset_get(pcs, pin);
 	val = pcs->read(pcs->base + offset);
+
+	if (pcs->bits_per_mux)
+		val &= pcs->fmask << pcs_pin_shift_reg_get(pcs, pin);
+
 	pa = pcs->res->start + offset;
 
 	seq_printf(s, "%zx %08x %s ", pa, val, DRIVER_NAME);

@@ -384,7 +408,6 @@ static int pcs_request_gpio(struct pinctrl_dev *pctldev,
 	struct pcs_device *pcs = pinctrl_dev_get_drvdata(pctldev);
 	struct pcs_gpiofunc_range *frange = NULL;
 	struct list_head *pos, *tmp;
-	int mux_bytes = 0;
 	unsigned data;
 
 	/* If function mask is null, return directly. */

@@ -392,29 +415,27 @@ static int pcs_request_gpio(struct pinctrl_dev *pctldev,
 		return -ENOTSUPP;
 
 	list_for_each_safe(pos, tmp, &pcs->gpiofuncs) {
+		u32 offset;
+
 		frange = list_entry(pos, struct pcs_gpiofunc_range, node);
 		if (pin >= frange->offset + frange->npins
 			|| pin < frange->offset)
 			continue;
-		mux_bytes = pcs->width / BITS_PER_BYTE;
 
-		if (pcs->bits_per_mux) {
-			int byte_num, offset, pin_shift;
+		offset = pcs_pin_reg_offset_get(pcs, pin);
 
-			byte_num = (pcs->bits_per_pin * pin) / BITS_PER_BYTE;
-			offset = (byte_num / mux_bytes) * mux_bytes;
-			pin_shift = pin % (pcs->width / pcs->bits_per_pin) *
-				    pcs->bits_per_pin;
+		if (pcs->bits_per_mux) {
+			int pin_shift = pcs_pin_shift_reg_get(pcs, pin);
 
 			data = pcs->read(pcs->base + offset);
 			data &= ~(pcs->fmask << pin_shift);
 			data |= frange->gpiofunc << pin_shift;
 			pcs->write(data, pcs->base + offset);
 		} else {
-			data = pcs->read(pcs->base + pin * mux_bytes);
+			data = pcs->read(pcs->base + offset);
 			data &= ~pcs->fmask;
 			data |= frange->gpiofunc;
-			pcs->write(data, pcs->base + pin * mux_bytes);
+			pcs->write(data, pcs->base + offset);
 		}
 		break;
 	}

@@ -724,14 +745,8 @@ static int pcs_allocate_pin_table(struct pcs_device *pcs)
 	for (i = 0; i < pcs->desc.npins; i++) {
 		unsigned offset;
 		int res;
-		int byte_num;
 
-		if (pcs->bits_per_mux) {
-			byte_num = (pcs->bits_per_pin * i) / BITS_PER_BYTE;
-			offset = (byte_num / mux_bytes) * mux_bytes;
-		} else {
-			offset = i * mux_bytes;
-		}
+		offset = pcs_pin_reg_offset_get(pcs, i);
 		res = pcs_add_pin(pcs, offset);
 		if (res < 0) {
 			dev_err(pcs->dev, "error adding pins: %i\n", res);

-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

* Hanna Hawa [off-list ref] [700101 02:00]:

A System Error (SError, followed by kernel panic) was detected when
trying to print the supported pins in a pinctrl device which supports
multiple pins per register. This change fixes the pcs_pin_dbg_show() in
pinctrl-single driver when bits_per_mux is not zero. In addition move
offset calculation and pin offset in register to common function.

Reviewed-by: Tony Lindgren <tony@atomide.com>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

On Fri, Mar 19, 2021 at 05:21:30PM +0200, Hanna Hawa wrote:

These patches fix the pcs_pin_dbg_show() function for the scenario where
a single register controls multiple pins (i.e. bits_per_mux is not zero)
Additionally, the common formula is moved to a separate function to
allow reuse.

Changes since v3:
-----------------
- define and set variable 'mux_bytes' in one line
- update commit message

Changes since v2:
-----------------
- move read() register to be outside of if condition (as it common
  read()).
- Remove extra parentheses
- replace offset variable by direct return statements

Changes since v1:
-----------------
- remove unused variable in In function 'pcs_allocate_pin_table'
  (Reported-by: kernel test robot [off-list ref])

Hanna Hawa (3):
  pinctrl: pinctrl-single: remove unused variable
  pinctrl: pinctrl-single: remove unused parameter
  pinctrl: pinctrl-single: fix pcs_pin_dbg_show() when bits_per_mux is
    not zero

 drivers/pinctrl/pinctrl-single.c | 65 ++++++++++++++++++--------------
 1 file changed, 37 insertions(+), 28 deletions(-)

-- 
2.17.1

I'm curious what SoC are you using?

It's good to know who has hardware to test bits_per_mux in the future.

I pay attention to pinctrl-single as that is the driver used for the TI
AM3358 SoC used in a variety of BeagleBone boards.  It does not use 
bits_per_mux, but I can verify that this does not cause any regression
for the AM3358 SoC:

  /sys/kernel/debug/pinctrl/44e10800.pinmux-pinctrl-single# cat pins
  registered pins: 142
  pin 0 (PIN0) 0:? 44e10800 00000027 pinctrl-single
  pin 1 (PIN1) 0:? 44e10804 00000027 pinctrl-single
  pin 2 (PIN2) 0:? 44e10808 00000027 pinctrl-single
  pin 3 (PIN3) 0:? 44e1080c 00000027 pinctrl-single
  pin 4 (PIN4) 0:? 44e10810 00000027 pinctrl-single
  pin 5 (PIN5) 0:? 44e10814 00000027 pinctrl-single
  pin 6 (PIN6) 0:? 44e10818 00000027 pinctrl-single
  pin 7 (PIN7) 0:? 44e1081c 00000027 pinctrl-single
  pin 8 (PIN8) 22:gpio-96-127 44e10820 00000027 pinctrl-single
  pin 9 (PIN9) 23:gpio-96-127 44e10824 00000037 pinctrl-single
  pin 10 (PIN10) 26:gpio-96-127 44e10828 00000037 pinctrl-single
  pin 11 (PIN11) 27:gpio-96-127 44e1082c 00000037 pinctrl-single
  pin 12 (PIN12) 0:? 44e10830 00000037 pinctrl-single
  <snip>
  pin 140 (PIN140) 0:? 44e10a30 00000028 pinctrl-single
  pin 141 (PIN141) 13:gpio-64-95 44e10a34 00000020 pinctrl-single

Reviewed-by: Drew Fustini <redacted>

Thanks,
Drew

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

On 3/22/2021 7:56 AM, Drew Fustini wrote:

I'm curious what SoC are you using?

I'm working on Amazon Annapurna Labs SoCs (based on ARM cortex 
processors). That include multiple pins controlled with same register.

It's good to know who has hardware to test bits_per_mux in the future.

I pay attention to pinctrl-single as that is the driver used for the TI
AM3358 SoC used in a variety of BeagleBone boards.  It does not use
bits_per_mux, but I can verify that this does not cause any regression
for the AM3358 SoC:

   /sys/kernel/debug/pinctrl/44e10800.pinmux-pinctrl-single# cat pins
   registered pins: 142
   pin 0 (PIN0) 0:? 44e10800 00000027 pinctrl-single
   pin 1 (PIN1) 0:? 44e10804 00000027 pinctrl-single
   pin 2 (PIN2) 0:? 44e10808 00000027 pinctrl-single
   pin 3 (PIN3) 0:? 44e1080c 00000027 pinctrl-single
   pin 4 (PIN4) 0:? 44e10810 00000027 pinctrl-single
   pin 5 (PIN5) 0:? 44e10814 00000027 pinctrl-single
   pin 6 (PIN6) 0:? 44e10818 00000027 pinctrl-single
   pin 7 (PIN7) 0:? 44e1081c 00000027 pinctrl-single
   pin 8 (PIN8) 22:gpio-96-127 44e10820 00000027 pinctrl-single
   pin 9 (PIN9) 23:gpio-96-127 44e10824 00000037 pinctrl-single
   pin 10 (PIN10) 26:gpio-96-127 44e10828 00000037 pinctrl-single
   pin 11 (PIN11) 27:gpio-96-127 44e1082c 00000037 pinctrl-single
   pin 12 (PIN12) 0:? 44e10830 00000037 pinctrl-single
   <snip>
   pin 140 (PIN140) 0:? 44e10a30 00000028 pinctrl-single
   pin 141 (PIN141) 13:gpio-64-95 44e10a34 00000020 pinctrl-single

Reviewed-by: Drew Fustini<redacted>

Thanks for review and verify the change.

Thanks,
Hanna

Thanks,
Drew

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

On Fri, Mar 19, 2021 at 4:22 PM Hanna Hawa [off-list ref] wrote:

These patches fix the pcs_pin_dbg_show() function for the scenario where
a single register controls multiple pins (i.e. bits_per_mux is not zero)
Additionally, the common formula is moved to a separate function to
allow reuse.

This v4 patch set applied!

Yours,
Linus Walleij

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel