[PATCH v1 00/16] pinctrl: cy8c95x0: Bugfixes and cleanups

[PATCH v1 00/16] pinctrl: cy8c95x0: Bugfixes and cleanups

[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.

Andy Shevchenko (16):
  pinctrl: cy8c95x0: Respect IRQ trigger settings from firmware
  pinctrl: cy8c95x0: Rename PWMSEL to SELPWM
  pinctrl: cy8c95x0: Enable regmap locking for debug
  pinctrl: cy8c95x0: Fix off-by-one in the regmap range settings
  pinctrl: cy8c95x0: Remove incorrectly set fields in regmap
    configuration
  pinctrl: cy8c95x0: Avoid accessing reserved registers
  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 | 235 ++++++++++++++---------------
 1 file changed, 113 insertions(+), 122 deletions(-)

-- 
2.43.0.rc1.1336.g36b5255a03ac

[PATCH v1 01/16] pinctrl: cy8c95x0: Respect IRQ trigger settings from firmware

[Andy Shevchenko]

Som of the platforms may connect the INT pin via inversion logic
effectively make the triggering to be active-low.
Remove explicit trigger flag to respect the settings from firmware.

Without this change even idling chip produces spurious interrupts
and kernel disables the line in the result:

  irq 33: nobody cared (try booting with the "irqpoll" option)
  CPU: 0 UID: 0 PID: 125 Comm: irq/33-i2c-INT3 Not tainted 6.12.0-00236-g8b874ed11dae #64
  Hardware name: Intel Corp. QUARK/Galileo, BIOS 0x01000900 01/01/2014
  ...
  handlers:
  [<86e86bea>] irq_default_primary_handler threaded [<d153e44a>] cy8c95x0_irq_handler [pinctrl_cy8c95x0]
  Disabling IRQ #33

Fixes: e6cbbe42944d ("pinctrl: Add Cypress cy8c95x0 support")
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/pinctrl/pinctrl-cy8c95x0.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pinctrl/pinctrl-cy8c95x0.c b/drivers/pinctrl/pinctrl-cy8c95x0.c
index 0d6c2027d4c1..825bd1e528b5 100644
--- a/drivers/pinctrl/pinctrl-cy8c95x0.c
+++ b/drivers/pinctrl/pinctrl-cy8c95x0.c
@@ -1347,7 +1347,7 @@ static int cy8c95x0_irq_setup(struct cy8c95x0_pinctrl *chip, int irq)
 
 	ret = devm_request_threaded_irq(chip->dev, irq,
 					NULL, cy8c95x0_irq_handler,
-					IRQF_ONESHOT | IRQF_SHARED | IRQF_TRIGGER_HIGH,
+					IRQF_ONESHOT | IRQF_SHARED,
 					dev_name(chip->dev), chip);
 	if (ret) {
 		dev_err(chip->dev, "failed to request irq %d\n", irq);
-- 
2.43.0.rc1.1336.g36b5255a03ac

[PATCH v1 02/16] pinctrl: cy8c95x0: Rename PWMSEL to SELPWM

[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 <andriy.shevchenko@linux.intel.com>
---
 drivers/pinctrl/pinctrl-cy8c95x0.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-cy8c95x0.c b/drivers/pinctrl/pinctrl-cy8c95x0.c
index 825bd1e528b5..d767bbc66256 100644
--- a/drivers/pinctrl/pinctrl-cy8c95x0.c
+++ b/drivers/pinctrl/pinctrl-cy8c95x0.c
@@ -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' */
@@ -365,8 +365,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:
@@ -395,7 +395,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:
@@ -789,7 +789,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;
@@ -868,7 +868,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);
@@ -1153,7 +1153,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;
 	}
@@ -1198,7 +1198,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,
-- 
2.43.0.rc1.1336.g36b5255a03ac

[PATCH v1 03/16] pinctrl: cy8c95x0: Enable regmap locking for debug

[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 <andriy.shevchenko@linux.intel.com>
---
 drivers/pinctrl/pinctrl-cy8c95x0.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/pinctrl/pinctrl-cy8c95x0.c b/drivers/pinctrl/pinctrl-cy8c95x0.c
index d767bbc66256..6833ac41d419 100644
--- a/drivers/pinctrl/pinctrl-cy8c95x0.c
+++ b/drivers/pinctrl/pinctrl-cy8c95x0.c
@@ -466,7 +466,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,
-- 
2.43.0.rc1.1336.g36b5255a03ac

[PATCH v1 04/16] pinctrl: cy8c95x0: Fix off-by-one in the regmap range settings

[Andy Shevchenko]

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 <andriy.shevchenko@linux.intel.com>
---
 drivers/pinctrl/pinctrl-cy8c95x0.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-cy8c95x0.c b/drivers/pinctrl/pinctrl-cy8c95x0.c
index 6833ac41d419..a94fade0ebc2 100644
--- a/drivers/pinctrl/pinctrl-cy8c95x0.c
+++ b/drivers/pinctrl/pinctrl-cy8c95x0.c
@@ -1442,15 +1442,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;
-- 
2.43.0.rc1.1336.g36b5255a03ac

[PATCH v1 05/16] pinctrl: cy8c95x0: Remove incorrectly set fields in regmap configuration

[Andy Shevchenko]

We don't provide defaults for the regmap, we shouldn't provide
the number of them either.

Remove incorrectly set fields in regmap configuration.

Fixes: 8670de9fae49 ("pinctrl: cy8c95x0: Use regmap ranges")
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/pinctrl/pinctrl-cy8c95x0.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-cy8c95x0.c b/drivers/pinctrl/pinctrl-cy8c95x0.c
index a94fade0ebc2..e98eba17cd13 100644
--- a/drivers/pinctrl/pinctrl-cy8c95x0.c
+++ b/drivers/pinctrl/pinctrl-cy8c95x0.c
@@ -464,7 +464,6 @@ static const struct regmap_config cy8c9520_i2c_regmap = {
 	.ranges	= NULL,			/* Updated at runtime */
 	.num_ranges = 1,
 	.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,
@@ -1475,7 +1474,6 @@ static int cy8c95x0_probe(struct i2c_client *client)
 	memcpy(&regmap_conf, &cy8c9520_i2c_regmap, sizeof(regmap_conf));
 	regmap_conf.ranges = &regmap_range_conf;
 	regmap_conf.max_register = regmap_range_conf.range_max;
-	regmap_conf.num_reg_defaults_raw = regmap_range_conf.range_max;
 
 	chip->regmap = devm_regmap_init_i2c(client, &regmap_conf);
 	if (IS_ERR(chip->regmap))
-- 
2.43.0.rc1.1336.g36b5255a03ac

[PATCH v1 06/16] pinctrl: cy8c95x0: Avoid accessing reserved registers

[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 <andriy.shevchenko@linux.intel.com>
---
 drivers/pinctrl/pinctrl-cy8c95x0.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-cy8c95x0.c b/drivers/pinctrl/pinctrl-cy8c95x0.c
index e98eba17cd13..970b6842b05b 100644
--- a/drivers/pinctrl/pinctrl-cy8c95x0.c
+++ b/drivers/pinctrl/pinctrl-cy8c95x0.c
@@ -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;
-- 
2.43.0.rc1.1336.g36b5255a03ac

[PATCH v1 07/16] pinctrl: cy8c95x0: Use better bitmap APIs where appropriate

[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 <andriy.shevchenko@linux.intel.com>
---
 drivers/pinctrl/pinctrl-cy8c95x0.c | 33 +++++++++++-------------------
 1 file changed, 12 insertions(+), 21 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-cy8c95x0.c b/drivers/pinctrl/pinctrl-cy8c95x0.c
index 970b6842b05b..d3f4e20d219f 100644
--- a/drivers/pinctrl/pinctrl-cy8c95x0.c
+++ b/drivers/pinctrl/pinctrl-cy8c95x0.c
@@ -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;
@@ -621,13 +621,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 */
@@ -652,19 +647,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 */
@@ -684,8 +673,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;
 }
@@ -1484,8 +1472,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)) {
-- 
2.43.0.rc1.1336.g36b5255a03ac

[PATCH v1 08/16] pinctrl: cy8c95x0; Switch to use for_each_set_clump8()

[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 <andriy.shevchenko@linux.intel.com>
---
 drivers/pinctrl/pinctrl-cy8c95x0.c | 24 +++++++++---------------
 1 file changed, 9 insertions(+), 15 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-cy8c95x0.c b/drivers/pinctrl/pinctrl-cy8c95x0.c
index d3f4e20d219f..851b4b4fd4cb 100644
--- a/drivers/pinctrl/pinctrl-cy8c95x0.c
+++ b/drivers/pinctrl/pinctrl-cy8c95x0.c
@@ -616,21 +616,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) {
@@ -647,19 +644,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) {
@@ -668,8 +662,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 */
-- 
2.43.0.rc1.1336.g36b5255a03ac

[PATCH v1 09/16] pinctrl: cy8c95x0: Transform to cy8c95x0_regmap_read_bits()

[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 <andriy.shevchenko@linux.intel.com>
---
 drivers/pinctrl/pinctrl-cy8c95x0.c | 45 +++++++++++++++++-------------
 1 file changed, 25 insertions(+), 20 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-cy8c95x0.c b/drivers/pinctrl/pinctrl-cy8c95x0.c
index 851b4b4fd4cb..61225beb0714 100644
--- a/drivers/pinctrl/pinctrl-cy8c95x0.c
+++ b/drivers/pinctrl/pinctrl-cy8c95x0.c
@@ -574,12 +574,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
@@ -589,10 +590,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)) {
@@ -604,11 +607,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,
@@ -645,7 +651,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 */
@@ -655,13 +661,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);
 	}
@@ -698,10 +703,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:
@@ -712,7 +717,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,
@@ -730,14 +735,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;
@@ -750,8 +755,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;
 
@@ -808,11 +813,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;
-- 
2.43.0.rc1.1336.g36b5255a03ac

[PATCH v1 10/16] pinctrl: cy8c95x0: Remove redundant check in cy8c95x0_regmap_update_bits_base()

[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 <andriy.shevchenko@linux.intel.com>
---
 drivers/pinctrl/pinctrl-cy8c95x0.c | 16 +++++-----------
 1 file changed, 5 insertions(+), 11 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-cy8c95x0.c b/drivers/pinctrl/pinctrl-cy8c95x0.c
index 61225beb0714..3907970a3e6e 100644
--- a/drivers/pinctrl/pinctrl-cy8c95x0.c
+++ b/drivers/pinctrl/pinctrl-cy8c95x0.c
@@ -476,20 +476,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);
-- 
2.43.0.rc1.1336.g36b5255a03ac

[PATCH v1 11/16] pinctrl: cy8c95x0: Replace 'return ret' by 'return 0' in some cases

[Andy Shevchenko]

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

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/pinctrl/pinctrl-cy8c95x0.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-cy8c95x0.c b/drivers/pinctrl/pinctrl-cy8c95x0.c
index 3907970a3e6e..d29898151a9e 100644
--- a/drivers/pinctrl/pinctrl-cy8c95x0.c
+++ b/drivers/pinctrl/pinctrl-cy8c95x0.c
@@ -516,7 +516,7 @@ static int cy8c95x0_regmap_update_bits_base(struct cy8c95x0_pinctrl *chip,
 		regcache_cache_only(chip->regmap, false);
 	}
 
-	return ret;
+	return 0;
 }
 
 /**
@@ -1285,7 +1285,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++) {
@@ -1294,7 +1294,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 = {
-- 
2.43.0.rc1.1336.g36b5255a03ac

[PATCH v1 12/16] pinctrl: cy8c95x0: Initialise boolean variable with boolean values

[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 <andriy.shevchenko@linux.intel.com>
---
 drivers/pinctrl/pinctrl-cy8c95x0.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-cy8c95x0.c b/drivers/pinctrl/pinctrl-cy8c95x0.c
index d29898151a9e..657e9fe40e38 100644
--- a/drivers/pinctrl/pinctrl-cy8c95x0.c
+++ b/drivers/pinctrl/pinctrl-cy8c95x0.c
@@ -1075,7 +1075,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);
@@ -1094,7 +1094,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);
-- 
2.43.0.rc1.1336.g36b5255a03ac

[PATCH v1 13/16] pinctrl: cy8c95x0: Get rid of cy8c95x0_pinmux_direction() forward declaration

[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 <andriy.shevchenko@linux.intel.com>
---
 drivers/pinctrl/pinctrl-cy8c95x0.c | 54 ++++++++++++++----------------
 1 file changed, 25 insertions(+), 29 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-cy8c95x0.c b/drivers/pinctrl/pinctrl-cy8c95x0.c
index 657e9fe40e38..fc9c6d293198 100644
--- a/drivers/pinctrl/pinctrl-cy8c95x0.c
+++ b/drivers/pinctrl/pinctrl-cy8c95x0.c
@@ -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 */
@@ -671,6 +668,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);
@@ -1228,32 +1250,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)
-- 
2.43.0.rc1.1336.g36b5255a03ac

[PATCH v1 14/16] pinctrl: cy8c95x0: Drop unneeded casting

[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 <andriy.shevchenko@linux.intel.com>
---
 drivers/pinctrl/pinctrl-cy8c95x0.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pinctrl/pinctrl-cy8c95x0.c b/drivers/pinctrl/pinctrl-cy8c95x0.c
index fc9c6d293198..7f7bc374c2fc 100644
--- a/drivers/pinctrl/pinctrl-cy8c95x0.c
+++ b/drivers/pinctrl/pinctrl-cy8c95x0.c
@@ -838,7 +838,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;
 }
 
-- 
2.43.0.rc1.1336.g36b5255a03ac

[PATCH v1 15/16] pinctrl: cy8c95x0: Separate EEPROM related register definitios

[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 <andriy.shevchenko@linux.intel.com>
---
 drivers/pinctrl/pinctrl-cy8c95x0.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/pinctrl/pinctrl-cy8c95x0.c b/drivers/pinctrl/pinctrl-cy8c95x0.c
index 7f7bc374c2fc..6ee21e697e43 100644
--- a/drivers/pinctrl/pinctrl-cy8c95x0.c
+++ b/drivers/pinctrl/pinctrl-cy8c95x0.c
@@ -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
-- 
2.43.0.rc1.1336.g36b5255a03ac

[PATCH v1 16/16] pinctrl: cy8c95x0: Fix comment style

[Andy Shevchenko]

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

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/pinctrl/pinctrl-cy8c95x0.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/pinctrl/pinctrl-cy8c95x0.c b/drivers/pinctrl/pinctrl-cy8c95x0.c
index 6ee21e697e43..b0e3d675dc8d 100644
--- a/drivers/pinctrl/pinctrl-cy8c95x0.c
+++ b/drivers/pinctrl/pinctrl-cy8c95x0.c
@@ -501,7 +501,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)) {
-- 
2.43.0.rc1.1336.g36b5255a03ac

Re: [PATCH v1 05/16] pinctrl: cy8c95x0: Remove incorrectly set fields in regmap configuration

[Patrick Rudolph]

Hi Andy,
On Fri, Jan 17, 2025 at 3:23 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> We don't provide defaults for the regmap, we shouldn't provide
> the number of them either.
The intention is to read back the defaults from HW to initialize the
regmap cache.
The defaults are applied at each POR from the device's internal EEPROM.
See regcache_hw_init() for details.
>
> Remove incorrectly set fields in regmap configuration.
>
> Fixes: 8670de9fae49 ("pinctrl: cy8c95x0: Use regmap ranges")
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/pinctrl/pinctrl-cy8c95x0.c | 2 --
>  1 file changed, 2 deletions(-)
>
> diff --git a/drivers/pinctrl/pinctrl-cy8c95x0.c b/drivers/pinctrl/pinctrl-cy8c95x0.c
> index a94fade0ebc2..e98eba17cd13 100644
> --- a/drivers/pinctrl/pinctrl-cy8c95x0.c
> +++ b/drivers/pinctrl/pinctrl-cy8c95x0.c
> @@ -464,7 +464,6 @@ static const struct regmap_config cy8c9520_i2c_regmap = {
>         .ranges = NULL,                 /* Updated at runtime */
>         .num_ranges = 1,
>         .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,
> @@ -1475,7 +1474,6 @@ static int cy8c95x0_probe(struct i2c_client *client)
>         memcpy(&regmap_conf, &cy8c9520_i2c_regmap, sizeof(regmap_conf));
>         regmap_conf.ranges = &regmap_range_conf;
>         regmap_conf.max_register = regmap_range_conf.range_max;
> -       regmap_conf.num_reg_defaults_raw = regmap_range_conf.range_max;
>
>         chip->regmap = devm_regmap_init_i2c(client, &regmap_conf);
>         if (IS_ERR(chip->regmap))
> --
> 2.43.0.rc1.1336.g36b5255a03ac
>

Re: [PATCH v1 05/16] pinctrl: cy8c95x0: Remove incorrectly set fields in regmap configuration

[Andy Shevchenko]

On Fri, Jan 17, 2025 at 04:01:43PM +0100, Patrick Rudolph wrote:
> Hi Andy,
> On Fri, Jan 17, 2025 at 3:23 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> >
> > We don't provide defaults for the regmap, we shouldn't provide
> > the number of them either.
> The intention is to read back the defaults from HW to initialize the
> regmap cache.
> The defaults are applied at each POR from the device's internal EEPROM.
> See regcache_hw_init() for details.

Yes, I was looking a lot and that code and it doesn't work as intended.
I can reproduce the issue, but it's real issue and I don't think we need
to read back anything from the HW forcibly. It will be done naturally, no?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 05/16] pinctrl: cy8c95x0: Remove incorrectly set fields in regmap configuration

[Andy Shevchenko]

On Fri, Jan 17, 2025 at 05:13:36PM +0200, Andy Shevchenko wrote:
> On Fri, Jan 17, 2025 at 04:01:43PM +0100, Patrick Rudolph wrote:
> > Hi Andy,
> > On Fri, Jan 17, 2025 at 3:23 PM Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> > >
> > > We don't provide defaults for the regmap, we shouldn't provide
> > > the number of them either.
> > The intention is to read back the defaults from HW to initialize the
> > regmap cache.
> > The defaults are applied at each POR from the device's internal EEPROM.
> > See regcache_hw_init() for details.
> 
> Yes, I was looking a lot and that code and it doesn't work as intended.
> I can reproduce the issue, but it's real issue and I don't think we need
> to read back anything from the HW forcibly. It will be done naturally, no?

I think I now remember, it has a NULL pointer dereference.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 05/16] pinctrl: cy8c95x0: Remove incorrectly set fields in regmap configuration

[Andy Shevchenko]

On Fri, Jan 17, 2025 at 05:15:06PM +0200, Andy Shevchenko wrote:
> On Fri, Jan 17, 2025 at 05:13:36PM +0200, Andy Shevchenko wrote:
> > On Fri, Jan 17, 2025 at 04:01:43PM +0100, Patrick Rudolph wrote:
> > > Hi Andy,
> > > On Fri, Jan 17, 2025 at 3:23 PM Andy Shevchenko
> > > <andriy.shevchenko@linux.intel.com> wrote:
> > > >
> > > > We don't provide defaults for the regmap, we shouldn't provide
> > > > the number of them either.
> > > The intention is to read back the defaults from HW to initialize the
> > > regmap cache.
> > > The defaults are applied at each POR from the device's internal EEPROM.
> > > See regcache_hw_init() for details.
> > 
> > Yes, I was looking a lot and that code and it doesn't work as intended.
> > I can reproduce the issue, but it's real issue and I don't think we need
> > to read back anything from the HW forcibly. It will be done naturally, no?
> 
> I think I now remember, it has a NULL pointer dereference.

Hmm... I have just tested and no issues found so far. I'm now puzzled what was
that when I tested this the first time a few months ago...

Okay, let's say I will drop this patch in v2. Can you look at the rest of
the series?

Thanks for review!

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 01/16] pinctrl: cy8c95x0: Respect IRQ trigger settings from firmware

[Linus Walleij]

On Fri, Jan 17, 2025 at 3:23 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:

> Som of the platforms may connect the INT pin via inversion logic
> effectively make the triggering to be active-low.
> Remove explicit trigger flag to respect the settings from firmware.
>
> Without this change even idling chip produces spurious interrupts
> and kernel disables the line in the result:
>
>   irq 33: nobody cared (try booting with the "irqpoll" option)
>   CPU: 0 UID: 0 PID: 125 Comm: irq/33-i2c-INT3 Not tainted 6.12.0-00236-g8b874ed11dae #64
>   Hardware name: Intel Corp. QUARK/Galileo, BIOS 0x01000900 01/01/2014
>   ...
>   handlers:
>   [<86e86bea>] irq_default_primary_handler threaded [<d153e44a>] cy8c95x0_irq_handler [pinctrl_cy8c95x0]
>   Disabling IRQ #33
>
> Fixes: e6cbbe42944d ("pinctrl: Add Cypress cy8c95x0 support")
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Patch applied for fixes! (Speling mistake on first line fixed in the process.)

Yours,
Linus Walleij

Re: [PATCH v1 05/16] pinctrl: cy8c95x0: Remove incorrectly set fields in regmap configuration

[Andy Shevchenko]

On Fri, Jan 17, 2025 at 05:15:06PM +0200, Andy Shevchenko wrote:
> On Fri, Jan 17, 2025 at 05:13:36PM +0200, Andy Shevchenko wrote:
> > On Fri, Jan 17, 2025 at 04:01:43PM +0100, Patrick Rudolph wrote:
> > > On Fri, Jan 17, 2025 at 3:23 PM Andy Shevchenko
> > > <andriy.shevchenko@linux.intel.com> wrote:
> > > >
> > > > We don't provide defaults for the regmap, we shouldn't provide
> > > > the number of them either.
> > > The intention is to read back the defaults from HW to initialize the
> > > regmap cache.
> > > The defaults are applied at each POR from the device's internal EEPROM.
> > > See regcache_hw_init() for details.
> > 
> > Yes, I was looking a lot and that code and it doesn't work as intended.
> > I can reproduce the issue, but it's real issue and I don't think we need
> > to read back anything from the HW forcibly. It will be done naturally, no?
> 
> I think I now remember, it has a NULL pointer dereference.

FWIW, okay, it was induced by a fix in regmap core, so this NULL pointer
dereference is for real, but it's a regression in the regmap core.

For the reference read this thread:
20250116124303.3941583-1-andriy.shevchenko@linux.intel.com.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 01/16] pinctrl: cy8c95x0: Respect IRQ trigger settings from firmware

[Andy Shevchenko]

On Mon, Jan 27, 2025 at 11:03:02AM +0100, Linus Walleij wrote:
> On Fri, Jan 17, 2025 at 3:23 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> 
> > Som of the platforms may connect the INT pin via inversion logic
> > effectively make the triggering to be active-low.
> > Remove explicit trigger flag to respect the settings from firmware.
> >
> > Without this change even idling chip produces spurious interrupts
> > and kernel disables the line in the result:
> >
> >   irq 33: nobody cared (try booting with the "irqpoll" option)
> >   CPU: 0 UID: 0 PID: 125 Comm: irq/33-i2c-INT3 Not tainted 6.12.0-00236-g8b874ed11dae #64
> >   Hardware name: Intel Corp. QUARK/Galileo, BIOS 0x01000900 01/01/2014
> >   ...
> >   handlers:
> >   [<86e86bea>] irq_default_primary_handler threaded [<d153e44a>] cy8c95x0_irq_handler [pinctrl_cy8c95x0]
> >   Disabling IRQ #33
> >
> > Fixes: e6cbbe42944d ("pinctrl: Add Cypress cy8c95x0 support")
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> 
> Patch applied for fixes! (Speling mistake on first line fixed in the process.)

Thanks! Any plans to take the rest of the fixes (except the one which Patrick
rightfully objected on)?

-- 
With Best Regards,
Andy Shevchenko