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

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

[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 <patrick.rudolph@9elements.com>
---
 drivers/pinctrl/pinctrl-cy8c95x0.c | 32 ++++++++++++++++--------------
 1 file changed, 17 insertions(+), 15 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-cy8c95x0.c b/drivers/pinctrl/pinctrl-cy8c95x0.c
index 981c569bd671..ca54d91fdc77 100644
--- a/drivers/pinctrl/pinctrl-cy8c95x0.c
+++ b/drivers/pinctrl/pinctrl-cy8c95x0.c
@@ -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,
-- 
2.44.0

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

[Patrick Rudolph]

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

Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com>
---
 drivers/pinctrl/pinctrl-cy8c95x0.c | 179 +++++++++--------------------
 1 file changed, 53 insertions(+), 126 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-cy8c95x0.c b/drivers/pinctrl/pinctrl-cy8c95x0.c
index ca54d91fdc77..9570de598193 100644
--- a/drivers/pinctrl/pinctrl-cy8c95x0.c
+++ b/drivers/pinctrl/pinctrl-cy8c95x0.c
@@ -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);
-- 
2.44.0

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

[Patrick Rudolph]

Use REGCACHE_MAPLE instead of REGCACHE_FLAT.

Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.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 9570de598193..4efb8b5cc2d3 100644
--- a/drivers/pinctrl/pinctrl-cy8c95x0.c
+++ b/drivers/pinctrl/pinctrl-cy8c95x0.c
@@ -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 */
-- 
2.44.0

Re: [PATCH 1/3] pinctrl: cy8c95x0: Use single I2C lock

[Andy Shevchenko]

On Tue, May 21, 2024 at 6:26 PM Patrick Rudolph
<patrick.rudolph@9elements.com> wrote:
>
> 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?

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Tue, May 21, 2024 at 6:26 PM Patrick Rudolph
<patrick.rudolph@9elements.com> wrote:
>
> Instead of implementing a custom register paging mechanism in
> the driver use the existing regmap ranges feature.

driver, use

...


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

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

...

> +                       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?

...

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

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Tue, May 21, 2024 at 6:26 PM Patrick Rudolph
<patrick.rudolph@9elements.com> wrote:
>
> Use REGCACHE_MAPLE instead of REGCACHE_FLAT.

Reviewed-by: Andy Shevchenko <andy@kernel.org>

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/3] pinctrl: cy8c95x0: Use single I2C lock

[Linus Walleij]

On Tue, May 21, 2024 at 7:25 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Tue, May 21, 2024 at 6:26 PM Patrick Rudolph
> <patrick.rudolph@9elements.com> wrote:
> >
> > 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

Re: [PATCH 1/3] pinctrl: cy8c95x0: Use single I2C lock

[Andy Shevchenko]

Tue, May 21, 2024 at 05:25:57PM +0200, Patrick Rudolph kirjoitti:
> 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 <andy@kernel.org>

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

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

Tue, May 21, 2024 at 08:34:18PM +0300, Andy Shevchenko kirjoitti:
> On Tue, May 21, 2024 at 6:26 PM Patrick Rudolph
> <patrick.rudolph@9elements.com> wrote:
> >
> > Instead of implementing a custom register paging mechanism in
> > the driver use the existing regmap ranges feature.
> 
> driver, use

...

> > +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.
> 
> > +               .selector_reg = CY8C95X0_PORTSEL,
> > +               .selector_mask = 0x07,
> > +               .selector_shift = 0x0,
> > +               .window_start = CY8C95X0_INTMASK,
> > +               .window_len = MUXED_STRIDE,
> > +       }
> >  };

...

> > +                       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?

...

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

...

> 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 <andy@kernel.org>

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/3] pinctrl: cy8c95x0: Use single I2C lock

[Linus Walleij]

On Tue, May 21, 2024 at 5:26 PM Patrick Rudolph
<patrick.rudolph@9elements.com> wrote:

> 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 <patrick.rudolph@9elements.com>

All three patches applied, thanks!

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

Yours,
Linus Walleij

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

[Andy Shevchenko]

On Tue, May 21, 2024 at 05:25:58PM +0200, Patrick Rudolph wrote:
> Instead of implementing a custom register paging mechanism in
> the driver use the existing regmap ranges feature.

...

> +#define MUXED_STRIDE		(CY8C95X0_DRV_HIZ - CY8C95X0_INTMASK)

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

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

> +	}
>  };

-- 
With Best Regards,
Andy Shevchenko

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

[Patrick Rudolph]

On Fri, Aug 9, 2024 at 2:13 PM Andy Shevchenko <andy@black.fi.intel.com> wrote:
>
> On Tue, May 21, 2024 at 05:25:58PM +0200, Patrick Rudolph wrote:
> > Instead of implementing a custom register paging mechanism in
> > the driver use the existing regmap ranges feature.
>
> ...
>
> > +#define MUXED_STRIDE         (CY8C95X0_DRV_HIZ - CY8C95X0_INTMASK)
>
> > +#define CY8C95X0_MUX_REGMAP_TO_OFFSET(x, p) \
> > +     (CY8C95X0_VIRTUAL + (x) - CY8C95X0_INTMASK + (p) * MUXED_STRIDE)
>
> > +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?
You are right. I tried to save some memory here. Will send out a fix.

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

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

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

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

[Andy Shevchenko]

On Tue, Aug 20, 2024 at 11:30 AM Patrick Rudolph
<patrick.rudolph@9elements.com> wrote:
> On Fri, Aug 9, 2024 at 2:13 PM Andy Shevchenko <andy@black.fi.intel.com> wrote:
> > On Tue, May 21, 2024 at 05:25:58PM +0200, Patrick Rudolph wrote:

...

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

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

-- 
With Best Regards,
Andy Shevchenko

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

[Linus Walleij]

On Tue, May 21, 2024 at 5:26 PM Patrick Rudolph
<patrick.rudolph@9elements.com> wrote:

> Instead of implementing a custom register paging mechanism in
> the driver use the existing regmap ranges feature.
>
> Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com>

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