pinctrl: cy8c95x0: Bugfixes and cleanups

PATCH In review Andy Shevchenko 2 participants 19 messages Feb 3, 2025 – Feb 4, 2025

Subsystems (3)

GPIO Pin control Cypress CY8C95X0 pinctrl driver

lore

Share & tools

lore

Andy Shevchenko

author

2025-02-03 13:15 UTC #0 lore raw

v2 cover pinctrl: cy8c95x0: Bugfixes and cleanups

This a set of the bugfixes and cleanups I have collected so far while testing the driver on Intel Galileo Gen 1 last year.

Patches 1-2 are kinda important fixes, patch 3 is half-half, it helps a lot when debugging, patch 4 is semantically a fix, but can wait. The rest is number of refactoring and cleaning up changes.

Changelog v2:
- dropped wrong patch (Patrick)
- reshuffled fixes from most important to less important (Linus)
- rebased on top of v6.14-rc1

Andy Shevchenko (14):
pinctrl: cy8c95x0: Fix off-by-one in the regmap range settings
pinctrl: cy8c95x0: Avoid accessing reserved registers
pinctrl: cy8c95x0: Enable regmap locking for debug
pinctrl: cy8c95x0: Rename PWMSEL to SELPWM
pinctrl: cy8c95x0: Use better bitmap APIs where appropriate
pinctrl: cy8c95x0; Switch to use for_each_set_clump8()
pinctrl: cy8c95x0: Transform to cy8c95x0_regmap_read_bits()
pinctrl: cy8c95x0: Remove redundant check in

    cy8c95x0_regmap_update_bits_base()

pinctrl: cy8c95x0: Replace 'return ret' by 'return 0' in some cases
pinctrl: cy8c95x0: Initialise boolean variable with boolean values
pinctrl: cy8c95x0: Get rid of cy8c95x0_pinmux_direction() forward
declaration
pinctrl: cy8c95x0: Drop unneeded casting
pinctrl: cy8c95x0: Separate EEPROM related register definitios
pinctrl: cy8c95x0: Fix comment style

 drivers/pinctrl/pinctrl-cy8c95x0.c | 231 ++++++++++++++---------------
 1 file changed, 112 insertions(+), 119 deletions(-)

Signature · 1 line

2.43.0.rc1.1336.g36b5255a03ac

Andy Shevchenko

author

2025-02-03 13:15 UTC #1 lore raw

PATCH v2 1/14 pinctrl: cy8c95x0: Fix off-by-one in the regmap range settings

The range_max is inclusive, so we need to use the number of the last accessible register address.

Fixes 8670de9fae49 ("pinctrl: cy8c95x0: Use regmap ranges")

Signed-off-by Andy Shevchenko <[email protected]>

---
 drivers/pinctrl/pinctrl-cy8c95x0.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

drivers/pinctrl/pinctrl-cy8c95x0.c +3 -3

index 825bd1e528b5..cda9e1b6fed6 100644
@@ -1438,15 +1438,15 @@ static int cy8c95x0_probe(struct i2c_client *client)
 	switch (chip->tpin) {
 	case 20:
 		strscpy(chip->name, cy8c95x0_id[0].name);
-		regmap_range_conf.range_max = CY8C95X0_VIRTUAL + 3 * MUXED_STRIDE;
+		regmap_range_conf.range_max = CY8C95X0_VIRTUAL + 3 * MUXED_STRIDE - 1;
 		break;
 	case 40:
 		strscpy(chip->name, cy8c95x0_id[1].name);
-		regmap_range_conf.range_max = CY8C95X0_VIRTUAL + 6 * MUXED_STRIDE;
+		regmap_range_conf.range_max = CY8C95X0_VIRTUAL + 6 * MUXED_STRIDE - 1;
 		break;
 	case 60:
 		strscpy(chip->name, cy8c95x0_id[2].name);
-		regmap_range_conf.range_max = CY8C95X0_VIRTUAL + 8 * MUXED_STRIDE;
+		regmap_range_conf.range_max = CY8C95X0_VIRTUAL + 8 * MUXED_STRIDE - 1;
 		break;
 	default:
 		return -ENODEV;

Signature · 1 line

2.43.0.rc1.1336.g36b5255a03ac

Andy Shevchenko

author

2025-02-03 13:15 UTC #2 lore raw

PATCH v2 2/14 pinctrl: cy8c95x0: Avoid accessing reserved registers

In reply to Andy Shevchenko

The checks for vrtual registers in the cy8c95x0_readable_register() and cy8c95x0_writeable_register() are not aligned and broken.

Fix that by explicitly avoiding reserved registers to be accessed.

Fixes 71e4001a0455 ("pinctrl: pinctrl-cy8c95x0: Fix regcache")

Signed-off-by Andy Shevchenko <[email protected]>

---
 drivers/pinctrl/pinctrl-cy8c95x0.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

drivers/pinctrl/pinctrl-cy8c95x0.c +10 -6

index cda9e1b6fed6..192a37c28a1d 100644
@@ -328,14 +328,14 @@ static int cypress_get_pin_mask(struct cy8c95x0_pinctrl *chip, unsigned int pin)
 static bool cy8c95x0_readable_register(struct device *dev, unsigned int reg)
 {
 	/*
-	 * Only 12 registers are present per port (see Table 6 in the
-	 * datasheet).
+	 * Only 12 registers are present per port (see Table 6 in the datasheet).
 	 */
-	if (reg >= CY8C95X0_VIRTUAL && (reg % MUXED_STRIDE) < 12)
-		return true;
+	if (reg >= CY8C95X0_VIRTUAL && (reg % MUXED_STRIDE) >= 12)
+		return false;
 
 	switch (reg) {
 	case 0x24 ... 0x27:
+	case 0x31 ... 0x3f:
 		return false;
 	default:
 		return true;

@@ -344,8 +344,11 @@ static bool cy8c95x0_readable_register(struct device *dev, unsigned int reg)
 
 static bool cy8c95x0_writeable_register(struct device *dev, unsigned int reg)
 {
-	if (reg >= CY8C95X0_VIRTUAL)
-		return true;
+	/*
+	 * Only 12 registers are present per port (see Table 6 in the datasheet).
+	 */
+	if (reg >= CY8C95X0_VIRTUAL && (reg % MUXED_STRIDE) >= 12)
+		return false;
 
 	switch (reg) {
 	case CY8C95X0_INPUT_(0) ... CY8C95X0_INPUT_(7):

@@ -353,6 +356,7 @@ static bool cy8c95x0_writeable_register(struct device *dev, unsigned int reg)
 	case CY8C95X0_DEVID:
 		return false;
 	case 0x24 ... 0x27:
+	case 0x31 ... 0x3f:
 		return false;
 	default:
 		return true;

Signature · 1 line

2.43.0.rc1.1336.g36b5255a03ac

Andy Shevchenko

author

2025-02-03 13:15 UTC #3 lore raw

PATCH v2 3/14 pinctrl: cy8c95x0: Enable regmap locking for debug

In reply to Andy Shevchenko

When regmap locking is disabled, debugfs is also disabled. Enable locking for debug when CONFIG_DEBUG_PINCTRL is set.

Fixes f71aba339a66 ("pinctrl: cy8c95x0: Use single I2C lock")

Signed-off-by Andy Shevchenko <[email protected]>

---
 drivers/pinctrl/pinctrl-cy8c95x0.c | 4 ++++
 1 file changed, 4 insertions(+)

drivers/pinctrl/pinctrl-cy8c95x0.c +4 -0

index 192a37c28a1d..0bcecebb1c0c 100644
@@ -470,7 +470,11 @@ static const struct regmap_config cy8c9520_i2c_regmap = {
 	.max_register = 0,		/* Updated at runtime */
 	.num_reg_defaults_raw = 0,	/* Updated at runtime */
 	.use_single_read = true,	/* Workaround for regcache bug */
+#if IS_ENABLED(CONFIG_DEBUG_PINCTRL)
+	.disable_locking = false,
+#else
 	.disable_locking = true,
+#endif
 };
 
 static inline int cy8c95x0_regmap_update_bits_base(struct cy8c95x0_pinctrl *chip,

Signature · 1 line

2.43.0.rc1.1336.g36b5255a03ac

Andy Shevchenko

author

2025-02-03 13:15 UTC #4 lore raw

PATCH v2 4/14 pinctrl: cy8c95x0: Rename PWMSEL to SELPWM

In reply to Andy Shevchenko

There are two registers in the hardware, one, "Select PWM", is per-port configuration enabling PWM function instead of GPIO. The other one is "PWM Select" is per-PWM selector to configure PWM itself. Original code uses abbreviation of the latter to describe the former. Rename it to follow the datasheet.

Fixes e6cbbe42944d ("pinctrl: Add Cypress cy8c95x0 support")

Signed-off-by Andy Shevchenko <[email protected]>

---
 drivers/pinctrl/pinctrl-cy8c95x0.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

drivers/pinctrl/pinctrl-cy8c95x0.c +7 -7

index 0bcecebb1c0c..d73004b4a45e 100644
@@ -42,7 +42,7 @@
 #define CY8C95X0_PORTSEL	0x18
 /* Port settings, write PORTSEL first */
 #define CY8C95X0_INTMASK	0x19
-#define CY8C95X0_PWMSEL		0x1A
+#define CY8C95X0_SELPWM		0x1A
 #define CY8C95X0_INVERT		0x1B
 #define CY8C95X0_DIRECTION	0x1C
 /* Drive mode register change state on writing '1' */

@@ -369,8 +369,8 @@ static bool cy8c95x0_volatile_register(struct device *dev, unsigned int reg)
 	case CY8C95X0_INPUT_(0) ... CY8C95X0_INPUT_(7):
 	case CY8C95X0_INTSTATUS_(0) ... CY8C95X0_INTSTATUS_(7):
 	case CY8C95X0_INTMASK:
+	case CY8C95X0_SELPWM:
 	case CY8C95X0_INVERT:
-	case CY8C95X0_PWMSEL:
 	case CY8C95X0_DIRECTION:
 	case CY8C95X0_DRV_PU:
 	case CY8C95X0_DRV_PD:

@@ -399,7 +399,7 @@ static bool cy8c95x0_muxed_register(unsigned int reg)
 {
 	switch (reg) {
 	case CY8C95X0_INTMASK:
-	case CY8C95X0_PWMSEL:
+	case CY8C95X0_SELPWM:
 	case CY8C95X0_INVERT:
 	case CY8C95X0_DIRECTION:
 	case CY8C95X0_DRV_PU:

@@ -797,7 +797,7 @@ static int cy8c95x0_gpio_get_pincfg(struct cy8c95x0_pinctrl *chip,
 		reg = CY8C95X0_DIRECTION;
 		break;
 	case PIN_CONFIG_MODE_PWM:
-		reg = CY8C95X0_PWMSEL;
+		reg = CY8C95X0_SELPWM;
 		break;
 	case PIN_CONFIG_OUTPUT:
 		reg = CY8C95X0_OUTPUT;

@@ -876,7 +876,7 @@ static int cy8c95x0_gpio_set_pincfg(struct cy8c95x0_pinctrl *chip,
 		reg = CY8C95X0_DRV_PP_FAST;
 		break;
 	case PIN_CONFIG_MODE_PWM:
-		reg = CY8C95X0_PWMSEL;
+		reg = CY8C95X0_SELPWM;
 		break;
 	case PIN_CONFIG_OUTPUT_ENABLE:
 		return cy8c95x0_pinmux_direction(chip, off, !arg);

@@ -1161,7 +1161,7 @@ static void cy8c95x0_pin_dbg_show(struct pinctrl_dev *pctldev, struct seq_file *
 	bitmap_zero(mask, MAX_LINE);
 	__set_bit(pin, mask);
 
-	if (cy8c95x0_read_regs_mask(chip, CY8C95X0_PWMSEL, pwm, mask)) {
+	if (cy8c95x0_read_regs_mask(chip, CY8C95X0_SELPWM, pwm, mask)) {
 		seq_puts(s, "not available");
 		return;
 	}

@@ -1206,7 +1206,7 @@ static int cy8c95x0_set_mode(struct cy8c95x0_pinctrl *chip, unsigned int off, bo
 	u8 port = cypress_get_port(chip, off);
 	u8 bit = cypress_get_pin_mask(chip, off);
 
-	return cy8c95x0_regmap_write_bits(chip, CY8C95X0_PWMSEL, port, bit, mode ? bit : 0);
+	return cy8c95x0_regmap_write_bits(chip, CY8C95X0_SELPWM, port, bit, mode ? bit : 0);
 }
 
 static int cy8c95x0_pinmux_mode(struct cy8c95x0_pinctrl *chip,

Signature · 1 line

2.43.0.rc1.1336.g36b5255a03ac

Andy Shevchenko

author

2025-02-03 13:15 UTC #5 lore raw

PATCH v2 5/14 pinctrl: cy8c95x0: Use better bitmap APIs where appropriate

In reply to Andy Shevchenko

There are bitmap_gather() and bitmap_scatter() that are factually reimplemented in the driver. Use better bitmap APIs where appropriate.

Signed-off-by Andy Shevchenko <[email protected]>

---
 drivers/pinctrl/pinctrl-cy8c95x0.c | 33 +++++++++++-------------------
 1 file changed, 12 insertions(+), 21 deletions(-)

drivers/pinctrl/pinctrl-cy8c95x0.c +12 ~~-21~~

index d73004b4a45e..52d8a44bb44e 100644
@@ -137,7 +137,7 @@ static const struct dmi_system_id cy8c95x0_dmi_acpi_irq_info[] = {
  * @irq_trig_low:   I/O bits affected by a low voltage level
  * @irq_trig_high:  I/O bits affected by a high voltage level
  * @push_pull:      I/O bits configured as push pull driver
- * @shiftmask:      Mask used to compensate for Gport2 width
+ * @map:            Mask used to compensate for Gport2 width
  * @nport:          Number of Gports in this chip
  * @gpio_chip:      gpiolib chip
  * @driver_data:    private driver data

@@ -158,7 +158,7 @@ struct cy8c95x0_pinctrl {
 	DECLARE_BITMAP(irq_trig_low, MAX_LINE);
 	DECLARE_BITMAP(irq_trig_high, MAX_LINE);
 	DECLARE_BITMAP(push_pull, MAX_LINE);
-	DECLARE_BITMAP(shiftmask, MAX_LINE);
+	DECLARE_BITMAP(map, MAX_LINE);
 	unsigned int nport;
 	struct gpio_chip gpio_chip;
 	unsigned long driver_data;

@@ -622,13 +622,8 @@ static int cy8c95x0_write_regs_mask(struct cy8c95x0_pinctrl *chip, int reg,
 	int ret;
 
 	/* Add the 4 bit gap of Gport2 */
-	bitmap_andnot(tmask, mask, chip->shiftmask, MAX_LINE);
-	bitmap_shift_left(tmask, tmask, 4, MAX_LINE);
-	bitmap_replace(tmask, tmask, mask, chip->shiftmask, BANK_SZ * 3);
-
-	bitmap_andnot(tval, val, chip->shiftmask, MAX_LINE);
-	bitmap_shift_left(tval, tval, 4, MAX_LINE);
-	bitmap_replace(tval, tval, val, chip->shiftmask, BANK_SZ * 3);
+	bitmap_scatter(tmask, mask, chip->map, MAX_LINE);
+	bitmap_scatter(tval, val, chip->map, MAX_LINE);
 
 	for (unsigned int i = 0; i < chip->nport; i++) {
 		/* Skip over unused banks */

@@ -653,19 +648,13 @@ static int cy8c95x0_read_regs_mask(struct cy8c95x0_pinctrl *chip, int reg,
 {
 	DECLARE_BITMAP(tmask, MAX_LINE);
 	DECLARE_BITMAP(tval, MAX_LINE);
-	DECLARE_BITMAP(tmp, MAX_LINE);
 	int read_val;
 	u8 bits;
 	int ret;
 
 	/* Add the 4 bit gap of Gport2 */
-	bitmap_andnot(tmask, mask, chip->shiftmask, MAX_LINE);
-	bitmap_shift_left(tmask, tmask, 4, MAX_LINE);
-	bitmap_replace(tmask, tmask, mask, chip->shiftmask, BANK_SZ * 3);
-
-	bitmap_andnot(tval, val, chip->shiftmask, MAX_LINE);
-	bitmap_shift_left(tval, tval, 4, MAX_LINE);
-	bitmap_replace(tval, tval, val, chip->shiftmask, BANK_SZ * 3);
+	bitmap_scatter(tmask, mask, chip->map, MAX_LINE);
+	bitmap_scatter(tval, val, chip->map, MAX_LINE);
 
 	for (unsigned int i = 0; i < chip->nport; i++) {
 		/* Skip over unused banks */

@@ -685,8 +674,7 @@ static int cy8c95x0_read_regs_mask(struct cy8c95x0_pinctrl *chip, int reg,
 	}
 
 	/* Fill the 4 bit gap of Gport2 */
-	bitmap_shift_right(tmp, tval, 4, MAX_LINE);
-	bitmap_replace(val, tmp, tval, chip->shiftmask, MAX_LINE);
+	bitmap_gather(tval, val, chip->map, MAX_LINE);
 
 	return 0;
 }

@@ -1486,8 +1474,11 @@ static int cy8c95x0_probe(struct i2c_client *client)
 		return PTR_ERR(chip->regmap);
 
 	bitmap_zero(chip->push_pull, MAX_LINE);
-	bitmap_zero(chip->shiftmask, MAX_LINE);
-	bitmap_set(chip->shiftmask, 0, 20);
+
+	/* Setup HW pins mapping */
+	bitmap_fill(chip->map, MAX_LINE);
+	bitmap_clear(chip->map, 20, 4);
+
 	mutex_init(&chip->i2c_lock);
 
 	if (dmi_first_match(cy8c95x0_dmi_acpi_irq_info)) {

Signature · 1 line

2.43.0.rc1.1336.g36b5255a03ac

Andy Shevchenko

author

2025-02-03 13:36 UTC #6 lore raw

Re: [PATCH v2 05/14] pinctrl: cy8c95x0: Use better bitmap APIs where appropriate

Quoted 2 lines · Andy Shevchenko

There are bitmap_gather() and bitmap_scatter() that are factually reimplemented in the driver. Use better bitmap APIs where appropriate.

...

drivers/pinctrl/pinctrl-cy8c95x0.c static int cy8c95x0_read_regs_mask(struct cy8c95x0_pinctrl *chip, int reg, · line 677

Quoted 1 line

drivers/pinctrl/pinctrl-cy8c95x0.c +1 -0
+	bitmap_gather(tval, val, chip->map, MAX_LINE);

Oh, here is a typo, has to be:

	bitmap_gather(val, tval, chip->map, MAX_LINE);

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Andy Shevchenko

author

2025-02-03 13:15 UTC #7 lore raw

PATCH v2 6/14 pinctrl: cy8c95x0; Switch to use for_each_set_clump8()

In reply to Andy Shevchenko

for_each_set_clump8() has embedded check for unset clump to skip. Switch driver to use for_each_set_clump8().

Signed-off-by Andy Shevchenko <[email protected]>

---
 drivers/pinctrl/pinctrl-cy8c95x0.c | 24 +++++++++---------------
 1 file changed, 9 insertions(+), 15 deletions(-)

drivers/pinctrl/pinctrl-cy8c95x0.c +9 ~~-15~~

index 52d8a44bb44e..93fb8afab643 100644
@@ -617,21 +617,18 @@ static int cy8c95x0_write_regs_mask(struct cy8c95x0_pinctrl *chip, int reg,
 {
 	DECLARE_BITMAP(tmask, MAX_LINE);
 	DECLARE_BITMAP(tval, MAX_LINE);
+	unsigned long bits, offset;
 	int write_val;
-	u8 bits;
 	int ret;
 
 	/* Add the 4 bit gap of Gport2 */
 	bitmap_scatter(tmask, mask, chip->map, MAX_LINE);
 	bitmap_scatter(tval, val, chip->map, MAX_LINE);
 
-	for (unsigned int i = 0; i < chip->nport; i++) {
-		/* Skip over unused banks */
-		bits = bitmap_get_value8(tmask, i * BANK_SZ);
-		if (!bits)
-			continue;
+	for_each_set_clump8(offset, bits, tmask, chip->tpin) {
+		unsigned int i = offset / 8;
 
-		write_val = bitmap_get_value8(tval, i * BANK_SZ);
+		write_val = bitmap_get_value8(tval, offset);
 
 		ret = cy8c95x0_regmap_update_bits(chip, reg, i, bits, write_val);
 		if (ret < 0) {

@@ -648,19 +645,16 @@ static int cy8c95x0_read_regs_mask(struct cy8c95x0_pinctrl *chip, int reg,
 {
 	DECLARE_BITMAP(tmask, MAX_LINE);
 	DECLARE_BITMAP(tval, MAX_LINE);
+	unsigned long bits, offset;
 	int read_val;
-	u8 bits;
 	int ret;
 
 	/* Add the 4 bit gap of Gport2 */
 	bitmap_scatter(tmask, mask, chip->map, MAX_LINE);
 	bitmap_scatter(tval, val, chip->map, MAX_LINE);
 
-	for (unsigned int i = 0; i < chip->nport; i++) {
-		/* Skip over unused banks */
-		bits = bitmap_get_value8(tmask, i * BANK_SZ);
-		if (!bits)
-			continue;
+	for_each_set_clump8(offset, bits, tmask, chip->tpin) {
+		unsigned int i = offset / 8;
 
 		ret = cy8c95x0_regmap_read(chip, reg, i, &read_val);
 		if (ret < 0) {

@@ -669,8 +663,8 @@ static int cy8c95x0_read_regs_mask(struct cy8c95x0_pinctrl *chip, int reg,
 		}
 
 		read_val &= bits;
-		read_val |= bitmap_get_value8(tval, i * BANK_SZ) & ~bits;
-		bitmap_set_value8(tval, read_val, i * BANK_SZ);
+		read_val |= bitmap_get_value8(tval, offset) & ~bits;
+		bitmap_set_value8(tval, read_val, offset);
 	}
 
 	/* Fill the 4 bit gap of Gport2 */

Signature · 1 line

2.43.0.rc1.1336.g36b5255a03ac

Andy Shevchenko

author

2025-02-03 13:15 UTC #8 lore raw

PATCH v2 7/14 pinctrl: cy8c95x0: Transform to cy8c95x0_regmap_read_bits()

In reply to Andy Shevchenko

The returned value of cy8c95x0_regmap_read() is used always with a bitmask being applied. Move that bitmasking code into the function. At the same time transform it to cy8c95x0_regmap_read_bits() which will be in align with the write and update counterparts.

Signed-off-by Andy Shevchenko <[email protected]>

---
 drivers/pinctrl/pinctrl-cy8c95x0.c | 45 +++++++++++++++++-------------
 1 file changed, 25 insertions(+), 20 deletions(-)

drivers/pinctrl/pinctrl-cy8c95x0.c +25 ~~-20~~

index 93fb8afab643..02b692118cb2 100644
@@ -575,12 +575,13 @@ static int cy8c95x0_regmap_update_bits(struct cy8c95x0_pinctrl *chip, unsigned i
 }
 
 /**
- * cy8c95x0_regmap_read() - reads a register using the regmap cache
+ * cy8c95x0_regmap_read_bits() - reads a register using the regmap cache
  * @chip: The pinctrl to work on
  * @reg: The register to read from. Can be direct access or muxed register.
  * @port: The port to be used for muxed registers or quick path direct access
  *        registers. Otherwise unused.
- * @read_val: Value read from hardware or cache
+ * @mask: Bitmask to apply
+ * @val: Value read from hardware or cache
  *
  * This function handles the register reads from the direct access registers and
  * the muxed registers while caching all register accesses, internally handling

@@ -590,10 +591,12 @@ static int cy8c95x0_regmap_update_bits(struct cy8c95x0_pinctrl *chip, unsigned i
  *
  * Return: 0 for successful request, else a corresponding error value
  */
-static int cy8c95x0_regmap_read(struct cy8c95x0_pinctrl *chip, unsigned int reg,
-				unsigned int port, unsigned int *read_val)
+static int cy8c95x0_regmap_read_bits(struct cy8c95x0_pinctrl *chip, unsigned int reg,
+				     unsigned int port, unsigned int mask, unsigned int *val)
 {
-	int off, ret;
+	unsigned int off;
+	unsigned int tmp;
+	int ret;
 
 	/* Registers behind the PORTSEL mux have their own range in regmap */
 	if (cy8c95x0_muxed_register(reg)) {

@@ -605,11 +608,14 @@ static int cy8c95x0_regmap_read(struct cy8c95x0_pinctrl *chip, unsigned int reg,
 		else
 			off = reg;
 	}
-	guard(mutex)(&chip->i2c_lock);
 
-	ret = regmap_read(chip->regmap, off, read_val);
+	scoped_guard(mutex, &chip->i2c_lock)
+		ret = regmap_read(chip->regmap, off, &tmp);
+	if (ret)
+		return ret;
 
-	return ret;
+	*val = tmp & mask;
+	return 0;
 }
 
 static int cy8c95x0_write_regs_mask(struct cy8c95x0_pinctrl *chip, int reg,

@@ -646,7 +652,7 @@ static int cy8c95x0_read_regs_mask(struct cy8c95x0_pinctrl *chip, int reg,
 	DECLARE_BITMAP(tmask, MAX_LINE);
 	DECLARE_BITMAP(tval, MAX_LINE);
 	unsigned long bits, offset;
-	int read_val;
+	unsigned int read_val;
 	int ret;
 
 	/* Add the 4 bit gap of Gport2 */

@@ -656,13 +662,12 @@ static int cy8c95x0_read_regs_mask(struct cy8c95x0_pinctrl *chip, int reg,
 	for_each_set_clump8(offset, bits, tmask, chip->tpin) {
 		unsigned int i = offset / 8;
 
-		ret = cy8c95x0_regmap_read(chip, reg, i, &read_val);
+		ret = cy8c95x0_regmap_read_bits(chip, reg, i, bits, &read_val);
 		if (ret < 0) {
 			dev_err(chip->dev, "failed reading register %d, port %u: err %d\n", reg, i, ret);
 			return ret;
 		}
 
-		read_val &= bits;
 		read_val |= bitmap_get_value8(tval, offset) & ~bits;
 		bitmap_set_value8(tval, read_val, offset);
 	}

@@ -699,10 +704,10 @@ static int cy8c95x0_gpio_get_value(struct gpio_chip *gc, unsigned int off)
 	struct cy8c95x0_pinctrl *chip = gpiochip_get_data(gc);
 	u8 port = cypress_get_port(chip, off);
 	u8 bit = cypress_get_pin_mask(chip, off);
-	u32 reg_val;
+	unsigned int reg_val;
 	int ret;
 
-	ret = cy8c95x0_regmap_read(chip, CY8C95X0_INPUT, port, &reg_val);
+	ret = cy8c95x0_regmap_read_bits(chip, CY8C95X0_INPUT, port, bit, &reg_val);
 	if (ret < 0) {
 		/*
 		 * NOTE:

@@ -713,7 +718,7 @@ static int cy8c95x0_gpio_get_value(struct gpio_chip *gc, unsigned int off)
 		return 0;
 	}
 
-	return !!(reg_val & bit);
+	return reg_val ? 1 : 0;
 }
 
 static void cy8c95x0_gpio_set_value(struct gpio_chip *gc, unsigned int off,

@@ -731,14 +736,14 @@ static int cy8c95x0_gpio_get_direction(struct gpio_chip *gc, unsigned int off)
 	struct cy8c95x0_pinctrl *chip = gpiochip_get_data(gc);
 	u8 port = cypress_get_port(chip, off);
 	u8 bit = cypress_get_pin_mask(chip, off);
-	u32 reg_val;
+	unsigned int reg_val;
 	int ret;
 
-	ret = cy8c95x0_regmap_read(chip, CY8C95X0_DIRECTION, port, &reg_val);
+	ret = cy8c95x0_regmap_read_bits(chip, CY8C95X0_DIRECTION, port, bit, &reg_val);
 	if (ret < 0)
 		return ret;
 
-	if (reg_val & bit)
+	if (reg_val)
 		return GPIO_LINE_DIRECTION_IN;
 
 	return GPIO_LINE_DIRECTION_OUT;

@@ -751,8 +756,8 @@ static int cy8c95x0_gpio_get_pincfg(struct cy8c95x0_pinctrl *chip,
 	enum pin_config_param param = pinconf_to_config_param(*config);
 	u8 port = cypress_get_port(chip, off);
 	u8 bit = cypress_get_pin_mask(chip, off);
+	unsigned int reg_val;
 	unsigned int reg;
-	u32 reg_val;
 	u16 arg = 0;
 	int ret;

@@ -809,11 +814,11 @@ static int cy8c95x0_gpio_get_pincfg(struct cy8c95x0_pinctrl *chip,
 	 * Writing 1 to one of the drive mode registers will automatically
 	 * clear conflicting set bits in the other drive mode registers.
 	 */
-	ret = cy8c95x0_regmap_read(chip, reg, port, &reg_val);
+	ret = cy8c95x0_regmap_read_bits(chip, reg, port, bit, &reg_val);
 	if (ret < 0)
 		return ret;
 
-	if (reg_val & bit)
+	if (reg_val)
 		arg = 1;
 	if (param == PIN_CONFIG_OUTPUT_ENABLE)
 		arg = !arg;

Signature · 1 line

2.43.0.rc1.1336.g36b5255a03ac

Andy Shevchenko

author

2025-02-03 13:15 UTC #9 lore raw

PATCH v2 8/14 pinctrl: cy8c95x0: Remove redundant check in cy8c95x0_regmap_update_bits_base()

In reply to Andy Shevchenko

The function is never called with the PORTSEL register in the argument. Drop unneeded check, but rescue a comment. While at it, drop inline and allow any compiler to choose better stragy (note, that inline in C code is only a recomendation to most of the modern compilers anyway).

Signed-off-by Andy Shevchenko <[email protected]>

---
 drivers/pinctrl/pinctrl-cy8c95x0.c | 16 +++++-----------
 1 file changed, 5 insertions(+), 11 deletions(-)

drivers/pinctrl/pinctrl-cy8c95x0.c +5 ~~-11~~

index 02b692118cb2..cd191cb1101e 100644
@@ -477,20 +477,14 @@ static const struct regmap_config cy8c9520_i2c_regmap = {
 #endif
 };
 
-static inline int cy8c95x0_regmap_update_bits_base(struct cy8c95x0_pinctrl *chip,
-						   unsigned int reg,
-						   unsigned int port,
-						   unsigned int mask,
-						   unsigned int val,
-						   bool *change, bool async,
-						   bool force)
+/* Caller should never modify PORTSEL directly */
+static int cy8c95x0_regmap_update_bits_base(struct cy8c95x0_pinctrl *chip,
+					    unsigned int reg, unsigned int port,
+					    unsigned int mask, unsigned int val,
+					    bool *change, bool async, bool force)
 {
 	int ret, off, i;
 
-	/* Caller should never modify PORTSEL directly */
-	if (reg == CY8C95X0_PORTSEL)
-		return -EINVAL;
-
 	/* Registers behind the PORTSEL mux have their own range in regmap */
 	if (cy8c95x0_muxed_register(reg)) {
 		off = CY8C95X0_MUX_REGMAP_TO_OFFSET(reg, port);

Signature · 1 line

2.43.0.rc1.1336.g36b5255a03ac

Andy Shevchenko

author

2025-02-03 13:15 UTC #10 lore raw

PATCH v2 9/14 pinctrl: cy8c95x0: Replace 'return ret' by 'return 0' in some cases

In reply to Andy Shevchenko

When it's known that the returned value can't be non-zero, use 'return 0' explicitly.

Signed-off-by Andy Shevchenko <[email protected]>

---
 drivers/pinctrl/pinctrl-cy8c95x0.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

drivers/pinctrl/pinctrl-cy8c95x0.c +3 -3

index cd191cb1101e..e02cab05cbfc 100644
@@ -517,7 +517,7 @@ static int cy8c95x0_regmap_update_bits_base(struct cy8c95x0_pinctrl *chip,
 		regcache_cache_only(chip->regmap, false);
 	}
 
-	return ret;
+	return 0;
 }
 
 /**

@@ -1286,7 +1286,7 @@ static int cy8c95x0_pinconf_set(struct pinctrl_dev *pctldev, unsigned int pin,
 				unsigned long *configs, unsigned int num_configs)
 {
 	struct cy8c95x0_pinctrl *chip = pinctrl_dev_get_drvdata(pctldev);
-	int ret = 0;
+	int ret;
 	int i;
 
 	for (i = 0; i < num_configs; i++) {

@@ -1295,7 +1295,7 @@ static int cy8c95x0_pinconf_set(struct pinctrl_dev *pctldev, unsigned int pin,
 			return ret;
 	}
 
-	return ret;
+	return 0;
 }
 
 static const struct pinconf_ops cy8c95x0_pinconf_ops = {

Signature · 1 line

2.43.0.rc1.1336.g36b5255a03ac

Andy Shevchenko

author

2025-02-03 13:15 UTC #11 lore raw

PATCH v2 10/14 pinctrl: cy8c95x0: Initialise boolean variable with boolean values

In reply to Andy Shevchenko

The 'ret' variable in cy8c95x0_irq_handler() is defined as bool, but is intialised with integers. Avoid implicit castings and initialise boolean variable with boolean values.

Signed-off-by Andy Shevchenko <[email protected]>

---
 drivers/pinctrl/pinctrl-cy8c95x0.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

drivers/pinctrl/pinctrl-cy8c95x0.c +2 -2

index e02cab05cbfc..0aad4ed79699 100644
@@ -1076,7 +1076,7 @@ static irqreturn_t cy8c95x0_irq_handler(int irq, void *devid)
 	if (!ret)
 		return IRQ_RETVAL(0);
 
-	ret = 0;
+	ret = false;
 	for_each_set_bit(level, pending, MAX_LINE) {
 		/* Already accounted for 4bit gap in GPort2 */
 		nested_irq = irq_find_mapping(gc->irq.domain, level);

@@ -1095,7 +1095,7 @@ static irqreturn_t cy8c95x0_irq_handler(int irq, void *devid)
 		else
 			handle_nested_irq(nested_irq);
 
-		ret = 1;
+		ret = true;
 	}
 
 	return IRQ_RETVAL(ret);

Signature · 1 line

2.43.0.rc1.1336.g36b5255a03ac

Andy Shevchenko

author

2025-02-03 13:15 UTC #12 lore raw

PATCH v2 11/14 pinctrl: cy8c95x0: Get rid of cy8c95x0_pinmux_direction() forward declaration

In reply to Andy Shevchenko

The function is used before being defined. Just move it up enough to get rid of forward declaration.

Signed-off-by Andy Shevchenko <[email protected]>

---
 drivers/pinctrl/pinctrl-cy8c95x0.c | 54 ++++++++++++++----------------
 1 file changed, 25 insertions(+), 29 deletions(-)

drivers/pinctrl/pinctrl-cy8c95x0.c +25 ~~-29~~

index 0aad4ed79699..a83a1b13a97f 100644
@@ -310,9 +310,6 @@ static const char * const cy8c95x0_groups[] = {
 	"gp77",
 };
 
-static int cy8c95x0_pinmux_direction(struct cy8c95x0_pinctrl *chip,
-				     unsigned int pin, bool input);
-
 static inline u8 cypress_get_port(struct cy8c95x0_pinctrl *chip, unsigned int pin)
 {
 	/* Account for GPORT2 which only has 4 bits */

@@ -672,6 +669,31 @@ static int cy8c95x0_read_regs_mask(struct cy8c95x0_pinctrl *chip, int reg,
 	return 0;
 }
 
+static int cy8c95x0_pinmux_direction(struct cy8c95x0_pinctrl *chip, unsigned int pin, bool input)
+{
+	u8 port = cypress_get_port(chip, pin);
+	u8 bit = cypress_get_pin_mask(chip, pin);
+	int ret;
+
+	ret = cy8c95x0_regmap_write_bits(chip, CY8C95X0_DIRECTION, port, bit, input ? bit : 0);
+	if (ret)
+		return ret;
+
+	/*
+	 * Disable driving the pin by forcing it to HighZ. Only setting
+	 * the direction register isn't sufficient in Push-Pull mode.
+	 */
+	if (input && test_bit(pin, chip->push_pull)) {
+		ret = cy8c95x0_regmap_write_bits(chip, CY8C95X0_DRV_HIZ, port, bit, bit);
+		if (ret)
+			return ret;
+
+		__clear_bit(pin, chip->push_pull);
+	}
+
+	return 0;
+}
+
 static int cy8c95x0_gpio_direction_input(struct gpio_chip *gc, unsigned int off)
 {
 	return pinctrl_gpio_direction_input(gc, off);

@@ -1229,32 +1251,6 @@ static int cy8c95x0_gpio_request_enable(struct pinctrl_dev *pctldev,
 	return cy8c95x0_set_mode(chip, pin, false);
 }
 
-static int cy8c95x0_pinmux_direction(struct cy8c95x0_pinctrl *chip,
-				     unsigned int pin, bool input)
-{
-	u8 port = cypress_get_port(chip, pin);
-	u8 bit = cypress_get_pin_mask(chip, pin);
-	int ret;
-
-	ret = cy8c95x0_regmap_write_bits(chip, CY8C95X0_DIRECTION, port, bit, input ? bit : 0);
-	if (ret)
-		return ret;
-
-	/*
-	 * Disable driving the pin by forcing it to HighZ. Only setting
-	 * the direction register isn't sufficient in Push-Pull mode.
-	 */
-	if (input && test_bit(pin, chip->push_pull)) {
-		ret = cy8c95x0_regmap_write_bits(chip, CY8C95X0_DRV_HIZ, port, bit, bit);
-		if (ret)
-			return ret;
-
-		__clear_bit(pin, chip->push_pull);
-	}
-
-	return 0;
-}
-
 static int cy8c95x0_gpio_set_direction(struct pinctrl_dev *pctldev,
 				       struct pinctrl_gpio_range *range,
 				       unsigned int pin, bool input)

Signature · 1 line

2.43.0.rc1.1336.g36b5255a03ac

Andy Shevchenko

author

2025-02-03 13:15 UTC #13 lore raw

PATCH v2 12/14 pinctrl: cy8c95x0: Drop unneeded casting

In reply to Andy Shevchenko

The 'arg' variable in cy8c95x0_gpio_get_pincfg() is already type of u16. No need to cast it, so drop unneeded casting.

Signed-off-by Andy Shevchenko <[email protected]>

---
 drivers/pinctrl/pinctrl-cy8c95x0.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/pinctrl/pinctrl-cy8c95x0.c +1 -1

index a83a1b13a97f..28374490d47d 100644
@@ -839,7 +839,7 @@ static int cy8c95x0_gpio_get_pincfg(struct cy8c95x0_pinctrl *chip,
 	if (param == PIN_CONFIG_OUTPUT_ENABLE)
 		arg = !arg;
 
-	*config = pinconf_to_config_packed(param, (u16)arg);
+	*config = pinconf_to_config_packed(param, arg);
 	return 0;
 }

Signature · 1 line

2.43.0.rc1.1336.g36b5255a03ac

Andy Shevchenko

author

2025-02-03 13:15 UTC #14 lore raw

PATCH v2 13/14 pinctrl: cy8c95x0: Separate EEPROM related register definitios

In reply to Andy Shevchenko

Currently it's not easy to see at a glance the group of the registers that are per port. Add a blank line and a comment to make it better.

Also add a missing definition for one of the EEPROM related registers.

Signed-off-by Andy Shevchenko <[email protected]>

---
 drivers/pinctrl/pinctrl-cy8c95x0.c | 4 ++++
 1 file changed, 4 insertions(+)

drivers/pinctrl/pinctrl-cy8c95x0.c +4 -0

index 28374490d47d..f03775341b60 100644
@@ -40,6 +40,7 @@
 
 /* Port Select configures the port */
 #define CY8C95X0_PORTSEL	0x18
+
 /* Port settings, write PORTSEL first */
 #define CY8C95X0_INTMASK	0x19
 #define CY8C95X0_SELPWM		0x1A

@@ -53,6 +54,9 @@
 #define CY8C95X0_DRV_PP_FAST	0x21
 #define CY8C95X0_DRV_PP_SLOW	0x22
 #define CY8C95X0_DRV_HIZ	0x23
+
+/* Internal device configuration */
+#define CY8C95X0_ENABLE_WDE	0x2D
 #define CY8C95X0_DEVID		0x2E
 #define CY8C95X0_WATCHDOG	0x2F
 #define CY8C95X0_COMMAND	0x30

Signature · 1 line

2.43.0.rc1.1336.g36b5255a03ac

Andy Shevchenko

author

2025-02-03 13:15 UTC #15 lore raw

PATCH v2 14/14 pinctrl: cy8c95x0: Fix comment style

In reply to Andy Shevchenko

One comment style is not aligned with the rest. Fix that.

Signed-off-by Andy Shevchenko <[email protected]>

---
 drivers/pinctrl/pinctrl-cy8c95x0.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

drivers/pinctrl/pinctrl-cy8c95x0.c +2 -1

index f03775341b60..96e34b9eba4a 100644
@@ -502,7 +502,8 @@ static int cy8c95x0_regmap_update_bits_base(struct cy8c95x0_pinctrl *chip,
 	if (ret < 0)
 		return ret;
 
-	/* Mimic what hardware does and update the cache when a WC bit is written.
+	/*
+	 * Mimic what hardware does and update the cache when a WC bit is written.
 	 * Allows to mark the registers as non-volatile and reduces I/O cycles.
 	 */
 	if (cy8c95x0_wc_register(reg) && (mask & val)) {

Signature · 1 line

2.43.0.rc1.1336.g36b5255a03ac

Linus Walleij

2025-02-03 13:35 UTC #16 lore raw

In reply to Andy Shevchenko

Quoted 6 lines · Andy Shevchenko

This a set of the bugfixes and cleanups I have collected so far while testing the driver on Intel Galileo Gen 1 last year.
Patches 1-2 are kinda important fixes, patch 3 is half-half, it helps a lot when debugging, patch 4 is semantically a fix, but can wait. The rest is number of refactoring and cleaning up changes.

I applied patches 1-4 for fixes since we are early in the rc cycle.

I will probably start devel off -rc2 or so and then I can queue the rest on top of that.

Thanks for fixing it all up Andy!

Yours,
Linus Walleij

Andy Shevchenko

author

2025-02-03 14:33 UTC #17 lore raw

Quoted 11 lines · Linus Walleij

Quoted 6 lines · Andy Shevchenko
This a set of the bugfixes and cleanups I have collected so far while testing the driver on Intel Galileo Gen 1 last year.
Patches 1-2 are kinda important fixes, patch 3 is half-half, it helps a lot when debugging, patch 4 is semantically a fix, but can wait. The rest is number of refactoring and cleaning up changes.
I applied patches 1-4 for fixes since we are early in the rc cycle.

Hmm... But I do not see the first patch from v1 to be applied. I was under impression that you have it somewhere in your local trees and now it seems disappeared. Can you check on your side what happens?

Quoted 4 lines

I will probably start devel off -rc2 or so and then I can queue the rest on top of that.
Thanks for fixing it all up Andy!

Thanks for taking them in!

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Andy Shevchenko

author

2025-02-04 17:48 UTC #18 lore raw

Quoted 11 lines · Andy Shevchenko

Quoted 10 lines · Linus Walleij
Quoted 6 lines · Andy Shevchenko
This a set of the bugfixes and cleanups I have collected so far while testing the driver on Intel Galileo Gen 1 last year.
Patches 1-2 are kinda important fixes, patch 3 is half-half, it helps a lot when debugging, patch 4 is semantically a fix, but can wait. The rest is number of refactoring and cleaning up changes.

Quoted 5 lines

Quoted 1 line
I applied patches 1-4 for fixes since we are early in the rc cycle.
Hmm... But I do not see the first patch from v1 to be applied. I was under impression that you have it somewhere in your local trees and now it seems disappeared. Can you check on your side what happens?

Okay, now it's there. So far, so good.

Signature · 2 lines

With Best Regards,
Andy Shevchenko