pinctrl: cy8c95x0: Use single I2C lock

PATCH In review Patrick Rudolph 4 participants 14 messages May 21, 2024 – Aug 26, 2024

Subsystems (3)

GPIO Pin control Cypress CY8C95X0 pinctrl driver

lore

Share & tools

lore

Patrick Rudolph

author maintainer

2024-05-21 15:26 UTC #0 lore raw

PATCH 1/3 pinctrl: cy8c95x0: Use single I2C lock

Currently there are 3 locks being used when accessing the chip, one in the driver and one in each regmap. Reduce that to one driver only lock that protects all regmap and regcache accesses.

Signed-off-by Patrick Rudolph <[email protected]>

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

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

index 981c569bd671..ca54d91fdc77 100644
@@ -453,7 +453,6 @@ cy8c95x0_mux_reg_read(void *context, unsigned int off, unsigned int *val)
 	u8 port = CY8C95X0_MUX_REGMAP_TO_PORT(off);
 	int ret, reg = CY8C95X0_MUX_REGMAP_TO_REG(off);
 
-	mutex_lock(&chip->i2c_lock);
 	/* Select the correct bank */
 	ret = regmap_write(chip->regmap, CY8C95X0_PORTSEL, port);
 	if (ret < 0)

@@ -463,11 +462,7 @@ cy8c95x0_mux_reg_read(void *context, unsigned int off, unsigned int *val)
 	 * Read the register through direct access regmap. The target range
 	 * is marked volatile.
 	 */
-	ret = regmap_read(chip->regmap, reg, val);
-out:
-	mutex_unlock(&chip->i2c_lock);
-
-	return ret;
+	return regmap_read(chip->regmap, reg, val);
 }
 
 static int

@@ -477,7 +472,6 @@ cy8c95x0_mux_reg_write(void *context, unsigned int off, unsigned int val)
 	u8 port = CY8C95X0_MUX_REGMAP_TO_PORT(off);
 	int ret, reg = CY8C95X0_MUX_REGMAP_TO_REG(off);
 
-	mutex_lock(&chip->i2c_lock);
 	/* Select the correct bank */
 	ret = regmap_write(chip->regmap, CY8C95X0_PORTSEL, port);
 	if (ret < 0)

@@ -487,11 +481,7 @@ cy8c95x0_mux_reg_write(void *context, unsigned int off, unsigned int val)
 	 * Write the register through direct access regmap. The target range
 	 * is marked volatile.
 	 */
-	ret = regmap_write(chip->regmap, reg, val);
-out:
-	mutex_unlock(&chip->i2c_lock);
-
-	return ret;
+	return regmap_write(chip->regmap, reg, val);
 }
 
 static bool cy8c95x0_mux_accessible_register(struct device *dev, unsigned int off)

@@ -524,6 +514,7 @@ static const struct regmap_config cy8c95x0_muxed_regmap = {
 	.num_reg_defaults_raw = MUXED_STRIDE * BANK_SZ,
 	.readable_reg = cy8c95x0_mux_accessible_register,
 	.writeable_reg = cy8c95x0_mux_accessible_register,
+	.disable_locking = true,
 };
 
 /* Direct access regmap */

@@ -542,6 +533,7 @@ static const struct regmap_config cy8c95x0_i2c_regmap = {
 
 	.cache_type = REGCACHE_FLAT,
 	.max_register = CY8C95X0_COMMAND,
+	.disable_locking = true,
 };
 
 static inline int cy8c95x0_regmap_update_bits_base(struct cy8c95x0_pinctrl *chip,

@@ -559,6 +551,8 @@ static inline int cy8c95x0_regmap_update_bits_base(struct cy8c95x0_pinctrl *chip
 	if (reg == CY8C95X0_PORTSEL)
 		return -EINVAL;
 
+	mutex_lock(&chip->i2c_lock);
+
 	/* Registers behind the PORTSEL mux have their own regmap */
 	if (cy8c95x0_muxed_register(reg)) {
 		regmap = chip->muxed_regmap;

@@ -574,7 +568,7 @@ static inline int cy8c95x0_regmap_update_bits_base(struct cy8c95x0_pinctrl *chip
 
 	ret = regmap_update_bits_base(regmap, off, mask, val, change, async, force);
 	if (ret < 0)
-		return ret;
+		goto out;
 
 	/* Update the cache when a WC bit is written */
 	if (cy8c95x0_wc_register(reg) && (mask & val)) {

@@ -595,6 +589,8 @@ static inline int cy8c95x0_regmap_update_bits_base(struct cy8c95x0_pinctrl *chip
 			regcache_cache_only(regmap, false);
 		}
 	}
+out:
+	mutex_unlock(&chip->i2c_lock);
 
 	return ret;
 }

@@ -667,7 +663,9 @@ static int cy8c95x0_regmap_read(struct cy8c95x0_pinctrl *chip, unsigned int reg,
 				unsigned int port, unsigned int *read_val)
 {
 	struct regmap *regmap;
-	int off;
+	int off, ret;
+
+	mutex_lock(&chip->i2c_lock);
 
 	/* Registers behind the PORTSEL mux have their own regmap */
 	if (cy8c95x0_muxed_register(reg)) {

@@ -682,7 +680,11 @@ static int cy8c95x0_regmap_read(struct cy8c95x0_pinctrl *chip, unsigned int reg,
 			off = reg;
 	}
 
-	return regmap_read(regmap, off, read_val);
+	ret = regmap_read(regmap, off, read_val);
+
+	mutex_unlock(&chip->i2c_lock);
+
+	return ret;
 }
 
 static int cy8c95x0_write_regs_mask(struct cy8c95x0_pinctrl *chip, int reg,

Signature · 1 line

2.44.0

Patrick Rudolph

author maintainer

2024-05-21 15:26 UTC #1 lore raw

PATCH 2/3 pinctrl: cy8c95x0: Use regmap ranges

Instead of implementing a custom register paging mechanism in the driver use the existing regmap ranges feature.

Signed-off-by Patrick Rudolph <[email protected]>

---
 drivers/pinctrl/pinctrl-cy8c95x0.c | 179 +++++++++--------------------
 1 file changed, 53 insertions(+), 126 deletions(-)

drivers/pinctrl/pinctrl-cy8c95x0.c +53 ~~-126~~

index ca54d91fdc77..9570de598193 100644
@@ -58,9 +58,14 @@
 
 #define CY8C95X0_PIN_TO_OFFSET(x) (((x) >= 20) ? ((x) + 4) : (x))
 
-#define CY8C95X0_MUX_REGMAP_TO_PORT(x) ((x) / MUXED_STRIDE)
-#define CY8C95X0_MUX_REGMAP_TO_REG(x) (((x) % MUXED_STRIDE) + CY8C95X0_INTMASK)
-#define CY8C95X0_MUX_REGMAP_TO_OFFSET(x, p) ((x) - CY8C95X0_INTMASK + (p) * MUXED_STRIDE)
+#define MAX_BANK		8
+#define BANK_SZ			8
+#define MAX_LINE		(MAX_BANK * BANK_SZ)
+#define MUXED_STRIDE		(CY8C95X0_DRV_HIZ - CY8C95X0_INTMASK)
+#define CY8C95X0_GPIO_MASK	GENMASK(7, 0)
+#define CY8C95X0_VIRTUAL	(CY8C95X0_COMMAND + 1)
+#define CY8C95X0_MUX_REGMAP_TO_OFFSET(x, p) \
+	(CY8C95X0_VIRTUAL + (x) - CY8C95X0_INTMASK + (p) * MUXED_STRIDE)
 
 static const struct i2c_device_id cy8c95x0_id[] = {
 	{ "cy8c9520", 20, },

@@ -120,18 +125,11 @@ static const struct dmi_system_id cy8c95x0_dmi_acpi_irq_info[] = {
 	{}
 };
 
-#define MAX_BANK 8
-#define BANK_SZ 8
-#define MAX_LINE	(MAX_BANK * BANK_SZ)
-#define MUXED_STRIDE	16
-#define CY8C95X0_GPIO_MASK		GENMASK(7, 0)
-
 /**
  * struct cy8c95x0_pinctrl - driver data
  * @regmap:         Device's regmap. Only direct access registers.
- * @muxed_regmap:   Regmap for all muxed registers.
  * @irq_lock:       IRQ bus lock
- * @i2c_lock:       Mutex for the device internal mux register
+ * @i2c_lock:       Mutex to hold while using the regmap
  * @irq_mask:       I/O bits affected by interrupts
  * @irq_trig_raise: I/O bits affected by raising voltage level
  * @irq_trig_fall:  I/O bits affected by falling voltage level

@@ -152,7 +150,6 @@ static const struct dmi_system_id cy8c95x0_dmi_acpi_irq_info[] = {
  */
 struct cy8c95x0_pinctrl {
 	struct regmap *regmap;
-	struct regmap *muxed_regmap;
 	struct mutex irq_lock;
 	struct mutex i2c_lock;
 	DECLARE_BITMAP(irq_mask, MAX_LINE);

@@ -331,6 +328,9 @@ static int cypress_get_pin_mask(struct cy8c95x0_pinctrl *chip, unsigned int pin)
 
 static bool cy8c95x0_readable_register(struct device *dev, unsigned int reg)
 {
+	if (reg >= CY8C95X0_VIRTUAL)
+		return true;
+
 	switch (reg) {
 	case 0x24 ... 0x27:
 		return false;

@@ -341,6 +341,9 @@ 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;
+
 	switch (reg) {
 	case CY8C95X0_INPUT_(0) ... CY8C95X0_INPUT_(7):
 		return false;

@@ -433,106 +436,33 @@ static bool cy8c95x0_quick_path_register(unsigned int reg)
 	}
 }
 
-static const struct reg_default cy8c95x0_reg_defaults[] = {
-	{ CY8C95X0_OUTPUT_(0), GENMASK(7, 0) },
-	{ CY8C95X0_OUTPUT_(1), GENMASK(7, 0) },
-	{ CY8C95X0_OUTPUT_(2), GENMASK(7, 0) },
-	{ CY8C95X0_OUTPUT_(3), GENMASK(7, 0) },
-	{ CY8C95X0_OUTPUT_(4), GENMASK(7, 0) },
-	{ CY8C95X0_OUTPUT_(5), GENMASK(7, 0) },
-	{ CY8C95X0_OUTPUT_(6), GENMASK(7, 0) },
-	{ CY8C95X0_OUTPUT_(7), GENMASK(7, 0) },
-	{ CY8C95X0_PORTSEL, 0 },
-	{ CY8C95X0_PWMSEL, 0 },
-};
-
-static int
-cy8c95x0_mux_reg_read(void *context, unsigned int off, unsigned int *val)
-{
-	struct cy8c95x0_pinctrl *chip = context;
-	u8 port = CY8C95X0_MUX_REGMAP_TO_PORT(off);
-	int ret, reg = CY8C95X0_MUX_REGMAP_TO_REG(off);
-
-	/* Select the correct bank */
-	ret = regmap_write(chip->regmap, CY8C95X0_PORTSEL, port);
-	if (ret < 0)
-		goto out;
-
-	/*
-	 * Read the register through direct access regmap. The target range
-	 * is marked volatile.
-	 */
-	return regmap_read(chip->regmap, reg, val);
-}
-
-static int
-cy8c95x0_mux_reg_write(void *context, unsigned int off, unsigned int val)
-{
-	struct cy8c95x0_pinctrl *chip = context;
-	u8 port = CY8C95X0_MUX_REGMAP_TO_PORT(off);
-	int ret, reg = CY8C95X0_MUX_REGMAP_TO_REG(off);
-
-	/* Select the correct bank */
-	ret = regmap_write(chip->regmap, CY8C95X0_PORTSEL, port);
-	if (ret < 0)
-		goto out;
-
-	/*
-	 * Write the register through direct access regmap. The target range
-	 * is marked volatile.
-	 */
-	return regmap_write(chip->regmap, reg, val);
-}
-
-static bool cy8c95x0_mux_accessible_register(struct device *dev, unsigned int off)
-{
-	struct i2c_client *i2c = to_i2c_client(dev);
-	struct cy8c95x0_pinctrl *chip = i2c_get_clientdata(i2c);
-	u8 port = CY8C95X0_MUX_REGMAP_TO_PORT(off);
-	u8 reg = CY8C95X0_MUX_REGMAP_TO_REG(off);
-
-	if (port >= chip->nport)
-		return false;
-
-	return cy8c95x0_muxed_register(reg);
-}
-
-static struct regmap_bus cy8c95x0_regmap_bus = {
-	.reg_read = cy8c95x0_mux_reg_read,
-	.reg_write = cy8c95x0_mux_reg_write,
-};
-
-/* Regmap for muxed registers CY8C95X0_INTMASK - CY8C95X0_DRV_HIZ */
-static const struct regmap_config cy8c95x0_muxed_regmap = {
-	.name = "muxed",
-	.reg_bits = 8,
-	.val_bits = 8,
-	.cache_type = REGCACHE_FLAT,
-	.use_single_read = true,
-	.use_single_write = true,
-	.max_register = MUXED_STRIDE * BANK_SZ,
-	.num_reg_defaults_raw = MUXED_STRIDE * BANK_SZ,
-	.readable_reg = cy8c95x0_mux_accessible_register,
-	.writeable_reg = cy8c95x0_mux_accessible_register,
-	.disable_locking = true,
+static const struct regmap_range_cfg cy8c95x0_ranges[] = {
+	{
+		.range_min = CY8C95X0_VIRTUAL,
+		.range_max = 0,		/* Updated at runtime */
+		.selector_reg = CY8C95X0_PORTSEL,
+		.selector_mask = 0x07,
+		.selector_shift = 0x0,
+		.window_start = CY8C95X0_INTMASK,
+		.window_len = MUXED_STRIDE,
+	}
 };
 
-/* Direct access regmap */
-static const struct regmap_config cy8c95x0_i2c_regmap = {
-	.name = "direct",
+static const struct regmap_config cy8c9520_i2c_regmap = {
 	.reg_bits = 8,
 	.val_bits = 8,
 
-	.reg_defaults = cy8c95x0_reg_defaults,
-	.num_reg_defaults = ARRAY_SIZE(cy8c95x0_reg_defaults),
-
 	.readable_reg = cy8c95x0_readable_register,
 	.writeable_reg = cy8c95x0_writeable_register,
 	.volatile_reg = cy8c95x0_volatile_register,
 	.precious_reg = cy8c95x0_precious_register,
 
 	.cache_type = REGCACHE_FLAT,
-	.max_register = CY8C95X0_COMMAND,
+	.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 */
 	.disable_locking = true,
 };

@@ -544,7 +474,6 @@ static inline int cy8c95x0_regmap_update_bits_base(struct cy8c95x0_pinctrl *chip
 						   bool *change, bool async,
 						   bool force)
 {
-	struct regmap *regmap;
 	int ret, off, i, read_val;
 
 	/* Caller should never modify PORTSEL directly */

@@ -553,12 +482,10 @@ static inline int cy8c95x0_regmap_update_bits_base(struct cy8c95x0_pinctrl *chip
 
 	mutex_lock(&chip->i2c_lock);
 
-	/* Registers behind the PORTSEL mux have their own regmap */
+	/* Registers behind the PORTSEL mux have their own range in regmap */
 	if (cy8c95x0_muxed_register(reg)) {
-		regmap = chip->muxed_regmap;
 		off = CY8C95X0_MUX_REGMAP_TO_OFFSET(reg, port);
 	} else {
-		regmap = chip->regmap;
 		/* Quick path direct access registers honor the port argument */
 		if (cy8c95x0_quick_path_register(reg))
 			off = reg + port;

@@ -566,7 +493,7 @@ static inline int cy8c95x0_regmap_update_bits_base(struct cy8c95x0_pinctrl *chip
 			off = reg;
 	}
 
-	ret = regmap_update_bits_base(regmap, off, mask, val, change, async, force);
+	ret = regmap_update_bits_base(chip->regmap, off, mask, val, change, async, force);
 	if (ret < 0)
 		goto out;

@@ -577,16 +504,16 @@ static inline int cy8c95x0_regmap_update_bits_base(struct cy8c95x0_pinctrl *chip
 				continue;
 			off = CY8C95X0_MUX_REGMAP_TO_OFFSET(i, port);
 
-			ret = regmap_read(regmap, off, &read_val);
+			ret = regmap_read(chip->regmap, off, &read_val);
 			if (ret < 0)
 				continue;
 
 			if (!(read_val & mask & val))
 				continue;
 
-			regcache_cache_only(regmap, true);
-			regmap_update_bits(regmap, off, mask & val, 0);
-			regcache_cache_only(regmap, false);
+			regcache_cache_only(chip->regmap, true);
+			regmap_update_bits(chip->regmap, off, mask & val, 0);
+			regcache_cache_only(chip->regmap, false);
 		}
 	}
 out:

@@ -662,17 +589,14 @@ static int cy8c95x0_regmap_update_bits(struct cy8c95x0_pinctrl *chip, unsigned i
 static int cy8c95x0_regmap_read(struct cy8c95x0_pinctrl *chip, unsigned int reg,
 				unsigned int port, unsigned int *read_val)
 {
-	struct regmap *regmap;
 	int off, ret;
 
 	mutex_lock(&chip->i2c_lock);
 
-	/* Registers behind the PORTSEL mux have their own regmap */
+	/* Registers behind the PORTSEL mux have their own range in regmap */
 	if (cy8c95x0_muxed_register(reg)) {
-		regmap = chip->muxed_regmap;
 		off = CY8C95X0_MUX_REGMAP_TO_OFFSET(reg, port);
 	} else {
-		regmap = chip->regmap;
 		/* Quick path direct access registers honor the port argument */
 		if (cy8c95x0_quick_path_register(reg))
 			off = reg + port;

@@ -680,7 +604,7 @@ static int cy8c95x0_regmap_read(struct cy8c95x0_pinctrl *chip, unsigned int reg,
 			off = reg;
 	}
 
-	ret = regmap_read(regmap, off, read_val);
+	ret = regmap_read(chip->regmap, off, read_val);
 
 	mutex_unlock(&chip->i2c_lock);

@@ -1513,6 +1437,8 @@ static int cy8c95x0_detect(struct i2c_client *client,
 static int cy8c95x0_probe(struct i2c_client *client)
 {
 	struct cy8c95x0_pinctrl *chip;
+	struct regmap_config regmap_conf;
+	struct regmap_range_cfg regmap_range_conf;
 	struct regulator *reg;
 	int ret;

@@ -1532,15 +1458,20 @@ static int cy8c95x0_probe(struct i2c_client *client)
 	chip->tpin = chip->driver_data & CY8C95X0_GPIO_MASK;
 	chip->nport = DIV_ROUND_UP(CY8C95X0_PIN_TO_OFFSET(chip->tpin), BANK_SZ);
 
+	memcpy(&regmap_range_conf, &cy8c95x0_ranges[0], sizeof(regmap_range_conf));
+
 	switch (chip->tpin) {
 	case 20:
 		strscpy(chip->name, cy8c95x0_id[0].name, I2C_NAME_SIZE);
+		regmap_range_conf.range_max = CY8C95X0_VIRTUAL + 3 * MUXED_STRIDE;
 		break;
 	case 40:
 		strscpy(chip->name, cy8c95x0_id[1].name, I2C_NAME_SIZE);
+		regmap_range_conf.range_max = CY8C95X0_VIRTUAL + 6 * MUXED_STRIDE;
 		break;
 	case 60:
 		strscpy(chip->name, cy8c95x0_id[2].name, I2C_NAME_SIZE);
+		regmap_range_conf.range_max = CY8C95X0_VIRTUAL + 8 * MUXED_STRIDE;
 		break;
 	default:
 		return -ENODEV;

@@ -1573,22 +1504,18 @@ static int cy8c95x0_probe(struct i2c_client *client)
 		gpiod_set_consumer_name(chip->gpio_reset, "CY8C95X0 RESET");
 	}
 
-	/* Generic regmap for direct access registers */
-	chip->regmap = devm_regmap_init_i2c(client, &cy8c95x0_i2c_regmap);
+	/* Regmap for direct and paged registers */
+	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)) {
 		ret = PTR_ERR(chip->regmap);
 		goto err_exit;
 	}
 
-	/* Port specific regmap behind PORTSEL mux */
-	chip->muxed_regmap = devm_regmap_init(&client->dev, &cy8c95x0_regmap_bus,
-					      chip, &cy8c95x0_muxed_regmap);
-	if (IS_ERR(chip->muxed_regmap)) {
-		ret = dev_err_probe(&client->dev, PTR_ERR(chip->muxed_regmap),
-				    "Failed to register muxed regmap\n");
-		goto err_exit;
-	}
-
 	bitmap_zero(chip->push_pull, MAX_LINE);
 	bitmap_zero(chip->shiftmask, MAX_LINE);
 	bitmap_set(chip->shiftmask, 0, 20);

Signature · 1 line

2.44.0

Andy Shevchenko

2024-05-21 17:34 UTC #2 lore raw

Re: [PATCH 2/3] pinctrl: cy8c95x0: Use regmap ranges

Quoted 3 lines · Patrick Rudolph

Instead of implementing a custom register paging mechanism in the driver use the existing regmap ranges feature.

driver, use

...

drivers/pinctrl/pinctrl-cy8c95x0.c static bool cy8c95x0_quick_path_register(unsigned int reg) · line 442

Quoted 4 lines

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

+static const struct regmap_range_cfg cy8c95x0_ranges[] = {
+       {
+               .range_min = CY8C95X0_VIRTUAL,
+               .range_max = 0,         /* Updated at runtime */

This and similar comments are misleading since the data is defined as const. Updated --> Filled or alike here and elsewhere.

Quoted 7 lines

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

+               .selector_reg = CY8C95X0_PORTSEL,
+               .selector_mask = 0x07,
+               .selector_shift = 0x0,
+               .window_start = CY8C95X0_INTMASK,
+               .window_len = MUXED_STRIDE,
+       }
 };

...

drivers/pinctrl/pinctrl-cy8c95x0.c static inline int cy8c95x0_regmap_update_bits_base(struct cy8c95x0_pinctrl *chip · line 516

Quoted 3 lines

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

+                       regcache_cache_only(chip->regmap, true);
+                       regmap_update_bits(chip->regmap, off, mask & val, 0);
+                       regcache_cache_only(chip->regmap, false);

A side note: It's strange to see mask & val, 0 in the parameters of regmap calls. Perhaps refactor this (in a separate patch) to use standard approach?

...

Quoted 1 line

+ memcpy(&regmap_range_conf, &cy8c95x0_ranges[0], sizeof(regmap_range_conf));

       memcpy(&regmap_range_conf, cy8c95x0_ranges, sizeof(regmap_range_conf));

...

Note, if you are not using --histogram diff algo, please start using it from the next version of this series.

P.S. I will test the next version on Intel Galileo Gen1 as currently I have some issues with the HW I need to fix first.

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Andy Shevchenko

Reviewed-by

2024-06-07 20:29 UTC #3 lore raw

Re: [PATCH 2/3] pinctrl: cy8c95x0: Use regmap ranges

Tue, May 21, 2024 at 08:34:18PM +0300, Andy Shevchenko kirjoitti:

Quoted 7 lines

Quoted 3 lines · Patrick Rudolph
Instead of implementing a custom register paging mechanism in the driver use the existing regmap ranges feature.
driver, use

...

Quoted 15 lines

drivers/pinctrl/pinctrl-cy8c95x0.c static bool cy8c95x0_quick_path_register(unsigned int reg) · line 442
Quoted 4 lines
drivers/pinctrl/pinctrl-cy8c95x0.c +4 -0
+static const struct regmap_range_cfg cy8c95x0_ranges[] = {
+       {
+               .range_min = CY8C95X0_VIRTUAL,
+               .range_max = 0,         /* Updated at runtime */
This and similar comments are misleading since the data is defined as const. Updated --> Filled or alike here and elsewhere.
Quoted 7 lines
drivers/pinctrl/pinctrl-cy8c95x0.c +6 -0
+               .selector_reg = CY8C95X0_PORTSEL,
+               .selector_mask = 0x07,
+               .selector_shift = 0x0,
+               .window_start = CY8C95X0_INTMASK,
+               .window_len = MUXED_STRIDE,
+       }
 };

...

Quoted 7 lines

drivers/pinctrl/pinctrl-cy8c95x0.c static inline int cy8c95x0_regmap_update_bits_base(struct cy8c95x0_pinctrl *chip · line 516
Quoted 3 lines
drivers/pinctrl/pinctrl-cy8c95x0.c +3 -0
+                       regcache_cache_only(chip->regmap, true);
+                       regmap_update_bits(chip->regmap, off, mask & val, 0);
+                       regcache_cache_only(chip->regmap, false);
A side note: It's strange to see mask & val, 0 in the parameters of regmap calls. Perhaps refactor this (in a separate patch) to use standard approach?

...

Quoted 3 lines

Quoted 1 line
+ memcpy(&regmap_range_conf, &cy8c95x0_ranges[0], sizeof(regmap_range_conf));
       memcpy(&regmap_range_conf, cy8c95x0_ranges, sizeof(regmap_range_conf));

I hope the all above can be tweaked by Linus when applying.

...

Quoted 5 lines

Note, if you are not using --histogram diff algo, please start using it from the next version of this series.
P.S. I will test the next version on Intel Galileo Gen1 as currently I have some issues with the HW I need to fix first.

Unfortunately I wasn't able to ressurect the HW and now I'm going to be off for a while. Let's go with this and if any problem appears, I probably can try to fix myself.

Reviewed-by Andy Shevchenko <[email protected]>

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Andy Shevchenko

2024-08-09 12:13 UTC #4 lore raw

Re: [PATCH 2/3] pinctrl: cy8c95x0: Use regmap ranges

In reply to Patrick Rudolph

Quoted 2 lines · Patrick Rudolph

Instead of implementing a custom register paging mechanism in the driver use the existing regmap ranges feature.

...

Quoted 4 lines

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

+#define MUXED_STRIDE		(CY8C95X0_DRV_HIZ - CY8C95X0_INTMASK)
 
+#define CY8C95X0_MUX_REGMAP_TO_OFFSET(x, p) \
+	(CY8C95X0_VIRTUAL + (x) - CY8C95X0_INTMASK + (p) * MUXED_STRIDE)

drivers/pinctrl/pinctrl-cy8c95x0.c static bool cy8c95x0_quick_path_register(unsigned int reg) · line 447

Quoted 10 lines

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

 
+static const struct regmap_range_cfg cy8c95x0_ranges[] = {
+	{
+		.range_min = CY8C95X0_VIRTUAL,
+		.range_max = 0,		/* Updated at runtime */
+		.selector_reg = CY8C95X0_PORTSEL,
+		.selector_mask = 0x07,
+		.selector_shift = 0x0,
+		.window_start = CY8C95X0_INTMASK,
+		.window_len = MUXED_STRIDE,

Looking at this again, are you sure you have no off-by-one error in MUXED_STRIDE value?

Also a comment in the regmap core suggests that we may include selector in each of the window.

With this, we probably should simply use PORTSEL as window start with a fixed window len of 16, having a few more (reserved) registers in the dump seems not a big deal to me, but it will be much easier to decipher a port number based on (virtual) offset.

Besides above, why virtual start is not well aligned? I would expect not 0x31, but 0x40 or alike. It might explain some bugs with cache you have seen.

P.S. It might still be bugs in regmap core, if it is the case, they need to be addressed.

Quoted 2 lines

drivers/pinctrl/pinctrl-cy8c95x0.c +1 -0
+	}
 };

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Patrick Rudolph

author maintainer

2024-08-20 08:30 UTC #5 lore raw

Re: [PATCH 2/3] pinctrl: cy8c95x0: Use regmap ranges

Quoted 22 lines · Andy Shevchenko

Quoted 2 lines · Patrick Rudolph
Instead of implementing a custom register paging mechanism in the driver use the existing regmap ranges feature.
...
Quoted 4 lines
drivers/pinctrl/pinctrl-cy8c95x0.c +3 -0
+#define MUXED_STRIDE         (CY8C95X0_DRV_HIZ - CY8C95X0_INTMASK)
 
+#define CY8C95X0_MUX_REGMAP_TO_OFFSET(x, p) \
+     (CY8C95X0_VIRTUAL + (x) - CY8C95X0_INTMASK + (p) * MUXED_STRIDE)
drivers/pinctrl/pinctrl-cy8c95x0.c static bool cy8c95x0_quick_path_register(unsigned int reg) · line 447
Quoted 10 lines
drivers/pinctrl/pinctrl-cy8c95x0.c +9 -0
 
+static const struct regmap_range_cfg cy8c95x0_ranges[] = {
+     {
+             .range_min = CY8C95X0_VIRTUAL,
+             .range_max = 0,         /* Updated at runtime */
+             .selector_reg = CY8C95X0_PORTSEL,
+             .selector_mask = 0x07,
+             .selector_shift = 0x0,
+             .window_start = CY8C95X0_INTMASK,
+             .window_len = MUXED_STRIDE,

Quoted 2 lines

Looking at this again, are you sure you have no off-by-one error in MUXED_STRIDE value?

You are right. I tried to save some memory here. Will send out a fix.

Quoted 9 lines

Also a comment in the regmap core suggests that we may include selector in each of the window.
With this, we probably should simply use PORTSEL as window start with a fixed window len of 16, having a few more (reserved) registers in the dump seems not a big deal to me, but it will be much easier to decipher a port number based on (virtual) offset.

That's true.

Quoted 4 lines

Besides above, why virtual start is not well aligned? I would expect not 0x31, but 0x40 or alike. It might explain some bugs with cache you have seen.

I didn't find any rules on this, so I used the next free index.

Quoted 11 lines

P.S. It might still be bugs in regmap core, if it is the case, they need to be addressed.
Quoted 2 lines
+ }
};
--
With Best Regards,
Andy Shevchenko

Andy Shevchenko

2024-08-20 16:18 UTC #6 lore raw

Re: [PATCH 2/3] pinctrl: cy8c95x0: Use regmap ranges

Quoted 2 lines · Patrick Rudolph

Quoted 1 line · Andy Shevchenko
On Tue, May 21, 2024 at 05:25:58PM +0200, Patrick Rudolph wrote:

...

Quoted 5 lines

Quoted 4 lines
Besides above, why virtual start is not well aligned? I would expect not 0x31, but 0x40 or alike. It might explain some bugs with cache you have seen.
I didn't find any rules on this, so I used the next free index.

I don't know either if any exists, but here is just my common sense. When we have the offsets being aligned it's much easier to decypher and, depending on implementation, may be more robust (in case there is some bit arithmetics is being used, I hope not though). That's why the 0x40 or any aligned offset seems natural choice to me.

Quoted 2 lines

Quoted 2 lines
P.S. It might still be bugs in regmap core, if it is the case, they need to be addressed.

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Linus Walleij

2024-08-26 08:26 UTC #7 lore raw

Re: [PATCH 2/3] pinctrl: cy8c95x0: Use regmap ranges

In reply to Patrick Rudolph

Quoted 4 lines · Patrick Rudolph

Instead of implementing a custom register paging mechanism in the driver use the existing regmap ranges feature.
Signed-off-by Patrick Rudolph <[email protected]>

I didn't end up applying this patch set (I don't know why, perhaps just forgot it).

Could you revisit it, look into Andys further comments, rebase on v6.11-rc1 and resend?

Yours,
Linus Walleij

Patrick Rudolph

author maintainer

2024-05-21 15:26 UTC #8 lore raw

PATCH 3/3 pinctrl: cy8c95x0: Use REGCACHE_MAPLE

In reply to Patrick Rudolph

Use REGCACHE_MAPLE instead of REGCACHE_FLAT.

Signed-off-by Patrick Rudolph <[email protected]>

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

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

index 9570de598193..4efb8b5cc2d3 100644
@@ -457,7 +457,7 @@ static const struct regmap_config cy8c9520_i2c_regmap = {
 	.volatile_reg = cy8c95x0_volatile_register,
 	.precious_reg = cy8c95x0_precious_register,
 
-	.cache_type = REGCACHE_FLAT,
+	.cache_type = REGCACHE_MAPLE,
 	.ranges	= NULL,			/* Updated at runtime */
 	.num_ranges = 1,
 	.max_register = 0,		/* Updated at runtime */

Signature · 1 line

2.44.0

Andy Shevchenko

Reviewed-by on v1 3/3

2024-05-21 17:35 UTC #9 lore raw

Re: [PATCH 3/3] pinctrl: cy8c95x0: Use REGCACHE_MAPLE

Quoted 2 lines · Patrick Rudolph

Use REGCACHE_MAPLE instead of REGCACHE_FLAT.

Reviewed-by Andy Shevchenko <[email protected]>

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Andy Shevchenko

2024-05-21 17:25 UTC #10 lore raw

In reply to Patrick Rudolph

Quoted 4 lines · Patrick Rudolph

Currently there are 3 locks being used when accessing the chip, one in the driver and one in each regmap. Reduce that to one driver only lock that protects all regmap and regcache accesses.

Right. But please consider converting the driver to use cleanup.h. Dunno if it requires a separate patch or can be folded into this one as it seems you anyway touch almost all mutex calls in the code. Linus?

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Linus Walleij

2024-05-28 11:58 UTC #11 lore raw

Quoted 11 lines · Andy Shevchenko

Quoted 4 lines · Patrick Rudolph
Currently there are 3 locks being used when accessing the chip, one in the driver and one in each regmap. Reduce that to one driver only lock that protects all regmap and regcache accesses.
Right. But please consider converting the driver to use cleanup.h. Dunno if it requires a separate patch or can be folded into this one as it seems you anyway touch almost all mutex calls in the code. Linus?

I think it's best to add a separate patch for this for bisection, just right after this one.

Yours,
Linus Walleij

Andy Shevchenko

Reviewed-by on v1 1/3

2024-06-07 20:26 UTC #12 lore raw

In reply to Patrick Rudolph

Tue, May 21, 2024 at 05:25:57PM +0200, Patrick Rudolph kirjoitti:

Quoted 3 lines

Currently there are 3 locks being used when accessing the chip, one in the driver and one in each regmap. Reduce that to one driver only lock that protects all regmap and regcache accesses.

Reviewed-by Andy Shevchenko <[email protected]>

(As discussed the conversion to cleanup.h may be done separately)

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Linus Walleij

2024-06-07 20:38 UTC #13 lore raw

In reply to Patrick Rudolph

Quoted 5 lines · Patrick Rudolph

Currently there are 3 locks being used when accessing the chip, one in the driver and one in each regmap. Reduce that to one driver only lock that protects all regmap and regcache accesses.
Signed-off-by Patrick Rudolph <[email protected]>

All three patches applied, thanks!

(Looking forward to a <linux/cleanup.h> patch!)

Yours,
Linus Walleij