Skip to contentSkip to conversation

Fix pinctrl-single pcs_pin_dbg_show()

PATCH In review Hanna Hawa 4 participants 10 messages Mar 19, 2021 – Mar 25, 2021

Subsystems (3)
lore
Share & tools
lore

Hanna Hawa

author
#0 lore raw

v4 cover 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 <[email protected]>)

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(-)
Signature · 1 line
2.17.1
author
#1 lore raw

PATCH v4 1/3 pinctrl: pinctrl-single: remove unused variable

Remove unused parameter 'num_pins_in_register' from pcs_allocate_pin_table().

---
 drivers/pinctrl/pinctrl-single.c | 2 --
 1 file changed, 2 deletions(-)
drivers/pinctrl/pinctrl-single.c +0 -2
index 7771316dfffa..91c638b85d2c 100644
@@ -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;
 	}
Signature · 1 line
2.17.1
maintainer Reviewed-by on v4 1/3
#2 lore raw

Re: [PATCH v4 1/3] pinctrl: pinctrl-single: remove unused variable

Quoted 2 lines · Hanna Hawa

Remove unused parameter 'num_pins_in_register' from pcs_allocate_pin_table().

author
#3 lore raw

PATCH v4 2/3 pinctrl: pinctrl-single: remove unused parameter

In reply to Hanna Hawa

Remove unused parameter 'pin_pos' from pcs_add_pin().

---
 drivers/pinctrl/pinctrl-single.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)
drivers/pinctrl/pinctrl-single.c +2 -6
index 91c638b85d2c..f3394517cb2e 100644
@@ -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;
Signature · 1 line
2.17.1
author
#5 lore raw

PATCH v4 3/3 pinctrl: pinctrl-single: fix pcs_pin_dbg_show() when bits_per_mux is not zero

In reply to Hanna Hawa

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")
---
 drivers/pinctrl/pinctrl-single.c | 55 ++++++++++++++++++++------------
 1 file changed, 35 insertions(+), 20 deletions(-)
drivers/pinctrl/pinctrl-single.c +35 -20
index f3394517cb2e..39aac32ed09c 100644
@@ -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);
Signature · 1 line
2.17.1
maintainer Reviewed-by on v4 3/3
#6 lore raw

Re: [PATCH v4 3/3] pinctrl: pinctrl-single: fix pcs_pin_dbg_show() when bits_per_mux is not zero

Quoted 5 lines · Hanna Hawa

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 on the v4 series
#7 lore raw
In reply to Hanna Hawa
Quoted 22 lines · Hanna Hawa

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 <[email protected]>)

Quoted 12 lines

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(-)
Signature · 1 line
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

Thanks,
Drew

#8 lore raw
Quoted 1 line · Drew Fustini

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.

Quoted 27 lines

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
Quoted 1 line

Thanks for review and verify the change.

Thanks,
Hanna

Quoted 3 lines

Thanks,
Drew

#9 lore raw
In reply to Hanna Hawa
Quoted 4 lines · Hanna Hawa

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