Skip to contentSkip to conversation

pinctrl: cy8c95x0: Yet another cleanup series and a fix

PATCH In review Andy Shevchenko 2 participants 10 messages Feb 23 – Feb 26

Subsystems (4)
Stable branch GPIO Pin control Cypress CY8C95X0 pinctrl driver
lore
Share & tools
lore

Andy Shevchenko

author
#0 lore raw

cover pinctrl: cy8c95x0: Yet another cleanup series and a fix

While playing on with Intel Galileo board again, I found that I messed up a bit with the driver. Here is a fix followed by a set of ad-hoc cleanups. Obviously tested on the above mentioned board.

Andy Shevchenko (8):
pinctrl: cy8c95x0: Don't miss reading the last bank registers
pinctrl: cy8c95x0: Use devm_mutex_init() for mutex initialization
pinctrl: cy8c95x0: remove duplicate error message
pinctrl: cy8c95x0: Unify messages with help of dev_err_probe()
pinctrl: cy8c95x0: Move driver data to the local variable in ->probe()
pinctrl: cy8c95x0: Drop unused 'name' in struct cy8c95x0_pinctrl
pinctrl: cy8c95x0: Eliminate fragile use of I²C ID table
pinctrl: cy8c95x0: Gather ID tables in one place

 drivers/pinctrl/pinctrl-cy8c95x0.c | 97 ++++++++++++------------------
 1 file changed, 40 insertions(+), 57 deletions(-)
Signature · 1 line
2.50.1
author
#1 lore raw

PATCH 1/8 pinctrl: cy8c95x0: Don't miss reading the last bank registers

When code had been changed to use for_each_set_clump8(), it mistakenly switched from chip->nport to chip->tpin since the cy8c9540 and cy8c9560 have a 4-pin gap. This, in particular, led to the missed read of the last bank interrupt status register and hence missing interrupts on those pins. Restore the upper limit in for_each_set_clump8() to take into consideration that gap.

Fixes 83e29a7a1fdf ("pinctrl: cy8c95x0; Switch to use for_each_set_clump8()")
Cc [email protected]
Signed-off-by Andy Shevchenko <[email protected]>
---
 drivers/pinctrl/pinctrl-cy8c95x0.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/pinctrl/pinctrl-cy8c95x0.c +2 -2
index a4b04bf6d081..5c055d344ac9 100644
@@ -627,7 +627,7 @@ static int cy8c95x0_write_regs_mask(struct cy8c95x0_pinctrl *chip, int reg,
 	bitmap_scatter(tmask, mask, chip->map, MAX_LINE);
 	bitmap_scatter(tval, val, chip->map, MAX_LINE);
 
-	for_each_set_clump8(offset, bits, tmask, chip->tpin) {
+	for_each_set_clump8(offset, bits, tmask, chip->nport * BANK_SZ) {
 		unsigned int i = offset / 8;
 
 		write_val = bitmap_get_value8(tval, offset);

@@ -655,7 +655,7 @@ static int cy8c95x0_read_regs_mask(struct cy8c95x0_pinctrl *chip, int reg,
 	bitmap_scatter(tmask, mask, chip->map, MAX_LINE);
 	bitmap_scatter(tval, val, chip->map, MAX_LINE);
 
-	for_each_set_clump8(offset, bits, tmask, chip->tpin) {
+	for_each_set_clump8(offset, bits, tmask, chip->nport * BANK_SZ) {
 		unsigned int i = offset / 8;
 
 		ret = cy8c95x0_regmap_read_bits(chip, reg, i, bits, &read_val);
Signature · 1 line
2.50.1
author
#2 lore raw

PATCH 2/8 pinctrl: cy8c95x0: Use devm_mutex_init() for mutex initialization

In reply to Andy Shevchenko

Use devm_mutex_init() since it brings some benefits when CONFIG_DEBUG_MUTEXES is enabled.

Signed-off-by Andy Shevchenko <[email protected]>
---
 drivers/pinctrl/pinctrl-cy8c95x0.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)
drivers/pinctrl/pinctrl-cy8c95x0.c +6 -2
index 5c055d344ac9..0647c7f94162 100644
@@ -1312,7 +1312,9 @@ static int cy8c95x0_irq_setup(struct cy8c95x0_pinctrl *chip, int irq)
 	DECLARE_BITMAP(pending_irqs, MAX_LINE);
 	int ret;
 
-	mutex_init(&chip->irq_lock);
+	ret = devm_mutex_init(chip->dev, &chip->irq_lock);
+	if (ret)
+		return ret;
 
 	bitmap_zero(pending_irqs, MAX_LINE);
 

@@ -1474,7 +1476,9 @@ static int cy8c95x0_probe(struct i2c_client *client)
 	bitmap_fill(chip->map, MAX_LINE);
 	bitmap_clear(chip->map, 20, 4);
 
-	mutex_init(&chip->i2c_lock);
+	ret = devm_mutex_init(dev, &chip->i2c_lock);
+	if (ret)
+		return ret;
 
 	if (dmi_first_match(cy8c95x0_dmi_acpi_irq_info)) {
 		ret = cy8c95x0_acpi_get_irq(&client->dev);
Signature · 1 line
2.50.1
author
#3 lore raw

PATCH 3/8 pinctrl: cy8c95x0: remove duplicate error message

In reply to Andy Shevchenko

The pin control core is covered to report any error via message. The devm_request_threaded_irq() already prints an error message. Remove the duplicates.

While at it, drop the info message as the same information about an IRQ in use can be retrieved differently.

Signed-off-by Andy Shevchenko <[email protected]>
---
 drivers/pinctrl/pinctrl-cy8c95x0.c | 21 +++++----------------
 1 file changed, 5 insertions(+), 16 deletions(-)
drivers/pinctrl/pinctrl-cy8c95x0.c +5 -16
index 0647c7f94162..165ccbda035f 100644
@@ -1310,6 +1310,7 @@ static int cy8c95x0_irq_setup(struct cy8c95x0_pinctrl *chip, int irq)
 {
 	struct gpio_irq_chip *girq = &chip->gpio_chip.irq;
 	DECLARE_BITMAP(pending_irqs, MAX_LINE);
+	struct device *dev = chip->dev;
 	int ret;
 
 	ret = devm_mutex_init(chip->dev, &chip->irq_lock);

@@ -1338,17 +1339,9 @@ static int cy8c95x0_irq_setup(struct cy8c95x0_pinctrl *chip, int irq)
 	girq->handler = handle_simple_irq;
 	girq->threaded = true;
 
-	ret = devm_request_threaded_irq(chip->dev, irq,
-					NULL, cy8c95x0_irq_handler,
-					IRQF_ONESHOT | IRQF_SHARED,
-					dev_name(chip->dev), chip);
-	if (ret) {
-		dev_err(chip->dev, "failed to request irq %d\n", irq);
-		return ret;
-	}
-	dev_info(chip->dev, "Registered threaded IRQ\n");
-
-	return 0;
+	return devm_request_threaded_irq(dev, irq, NULL, cy8c95x0_irq_handler,
+					 IRQF_ONESHOT | IRQF_SHARED,
+					 dev_name(chip->dev), chip);
 }
 
 static int cy8c95x0_setup_pinctrl(struct cy8c95x0_pinctrl *chip)

@@ -1364,11 +1357,7 @@ static int cy8c95x0_setup_pinctrl(struct cy8c95x0_pinctrl *chip)
 	pd->owner = THIS_MODULE;
 
 	chip->pctldev = devm_pinctrl_register(chip->dev, pd, chip);
-	if (IS_ERR(chip->pctldev))
-		return dev_err_probe(chip->dev, PTR_ERR(chip->pctldev),
-			"can't register controller\n");
-
-	return 0;
+	return PTR_ERR_OR_ZERO(chip->pctldev);
 }
 
 static int cy8c95x0_detect(struct i2c_client *client,
Signature · 1 line
2.50.1
author
#4 lore raw

PATCH 4/8 pinctrl: cy8c95x0: Unify messages with help of dev_err_probe()

In reply to Andy Shevchenko

Unify error messages that might appear during probe phase by switching to use dev_err_probe().

Signed-off-by Andy Shevchenko <[email protected]>
---
 drivers/pinctrl/pinctrl-cy8c95x0.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)
drivers/pinctrl/pinctrl-cy8c95x0.c +2 -4
index 165ccbda035f..94a599486638 100644
@@ -1321,10 +1321,8 @@ static int cy8c95x0_irq_setup(struct cy8c95x0_pinctrl *chip, int irq)
 
 	/* Read IRQ status register to clear all pending interrupts */
 	ret = cy8c95x0_irq_pending(chip, pending_irqs);
-	if (ret) {
-		dev_err(chip->dev, "failed to clear irq status register\n");
-		return ret;
-	}
+	if (ret)
+		return dev_err_probe(dev, ret, "failed to clear irq status register\n");
 
 	/* Mask all interrupts */
 	bitmap_fill(chip->irq_mask, MAX_LINE);
Signature · 1 line
2.50.1
author
#5 lore raw

PATCH 5/8 pinctrl: cy8c95x0: Move driver data to the local variable in ->probe()

In reply to Andy Shevchenko

For all these years of driver existence the driver_data has been used only as a raw material for other fields in the struct cy8c95x0_pinctrl. Move it from the structure to be just a local variable in ->probe(). Later, if ever need arises, we may reconsider that.

While at it, drop an unneeded validation and replace uintptr_t with plain unsigned long which is more readable and works in the same way.

Signed-off-by Andy Shevchenko <[email protected]>
---
 drivers/pinctrl/pinctrl-cy8c95x0.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)
drivers/pinctrl/pinctrl-cy8c95x0.c +3 -6
index 94a599486638..4f804a9358b7 100644
@@ -144,7 +144,6 @@ static const struct dmi_system_id cy8c95x0_dmi_acpi_irq_info[] = {
  * @map:            Mask used to compensate for Gport2 width
  * @nport:          Number of Gports in this chip
  * @gpio_chip:      gpiolib chip
- * @driver_data:    private driver data
  * @dev:            struct device
  * @pctldev:        pin controller device
  * @pinctrl_desc:   pin controller description

@@ -165,7 +164,6 @@ struct cy8c95x0_pinctrl {
 	DECLARE_BITMAP(map, MAX_LINE);
 	unsigned int nport;
 	struct gpio_chip gpio_chip;
-	unsigned long driver_data;
 	struct device *dev;
 	struct pinctrl_dev *pctldev;
 	struct pinctrl_desc pinctrl_desc;

@@ -1397,6 +1395,7 @@ static int cy8c95x0_probe(struct i2c_client *client)
 	struct cy8c95x0_pinctrl *chip;
 	struct regmap_config regmap_conf;
 	struct regmap_range_cfg regmap_range_conf;
+	unsigned long driver_data;
 	int ret;
 
 	chip = devm_kzalloc(dev, sizeof(*chip), GFP_KERNEL);

@@ -1406,11 +1405,9 @@ static int cy8c95x0_probe(struct i2c_client *client)
 	chip->dev = dev;
 
 	/* Set the device type */
-	chip->driver_data = (uintptr_t)i2c_get_match_data(client);
-	if (!chip->driver_data)
-		return -ENODEV;
+	driver_data = (unsigned long)i2c_get_match_data(client);
 
-	chip->tpin = chip->driver_data & CY8C95X0_GPIO_MASK;
+	chip->tpin = 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));
Signature · 1 line
2.50.1
author
#6 lore raw

PATCH 6/8 pinctrl: cy8c95x0: Drop unused 'name' in struct cy8c95x0_pinctrl

In reply to Andy Shevchenko

The 'name' is only assigned and never used. Drop it for good.

Signed-off-by Andy Shevchenko <[email protected]>
---
 drivers/pinctrl/pinctrl-cy8c95x0.c | 5 -----
 1 file changed, 5 deletions(-)
drivers/pinctrl/pinctrl-cy8c95x0.c +0 -5
index 4f804a9358b7..0bcc9ac19696 100644
@@ -147,7 +147,6 @@ static const struct dmi_system_id cy8c95x0_dmi_acpi_irq_info[] = {
  * @dev:            struct device
  * @pctldev:        pin controller device
  * @pinctrl_desc:   pin controller description
- * @name:           Chip controller name
  * @tpin:           Total number of pins
  * @gpio_reset:     GPIO line handler that can reset the IC
  */

@@ -167,7 +166,6 @@ struct cy8c95x0_pinctrl {
 	struct device *dev;
 	struct pinctrl_dev *pctldev;
 	struct pinctrl_desc pinctrl_desc;
-	char name[32];
 	unsigned int tpin;
 	struct gpio_desc *gpio_reset;
 };

@@ -1414,15 +1412,12 @@ 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 - 1;
 		break;
 	case 40:
-		strscpy(chip->name, cy8c95x0_id[1].name);
 		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 - 1;
 		break;
 	default:
Signature · 1 line
2.50.1
author
#7 lore raw

PATCH 7/8 pinctrl: cy8c95x0: Eliminate fragile use of I²C ID table

In reply to Andy Shevchenko

The I²C ID table is a subject to new entries that may potentially break the order of the existing ones. Avoid this by using string literals for the chip naming. Note, linker will deduplicate same string literals and use only a single copy, hence it won't be the change in size in data section.

Signed-off-by Andy Shevchenko <[email protected]>
---
 drivers/pinctrl/pinctrl-cy8c95x0.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
drivers/pinctrl/pinctrl-cy8c95x0.c +3 -3
index 0bcc9ac19696..1d3e0617b235 100644
@@ -1369,13 +1369,13 @@ static int cy8c95x0_detect(struct i2c_client *client,
 		return ret;
 	switch (ret & GENMASK(7, 4)) {
 	case 0x20:
-		name = cy8c95x0_id[0].name;
+		name = "cy8c9520";
 		break;
 	case 0x40:
-		name = cy8c95x0_id[1].name;
+		name = "cy8c9540";
 		break;
 	case 0x60:
-		name = cy8c95x0_id[2].name;
+		name = "cy8c9560";
 		break;
 	default:
 		return -ENODEV;
Signature · 1 line
2.50.1
author
#8 lore raw

PATCH 8/8 pinctrl: cy8c95x0: Gather ID tables in one place

In reply to Andy Shevchenko

We have three ID tables spread over the driver code. Move all of them closer to the end of the file where the first user appears to be. With that done, drop unneeded trailing commas.

Signed-off-by Andy Shevchenko <[email protected]>
---
 drivers/pinctrl/pinctrl-cy8c95x0.c | 38 +++++++++++++++---------------
 1 file changed, 19 insertions(+), 19 deletions(-)
drivers/pinctrl/pinctrl-cy8c95x0.c +19 -19
index 1d3e0617b235..0d295ebc33d1 100644
@@ -72,24 +72,6 @@
 #define CY8C95X0_MUX_REGMAP_TO_OFFSET(x, p) \
 	(CY8C95X0_VIRTUAL + (x) - CY8C95X0_PORTSEL + (p) * MUXED_STRIDE)
 
-static const struct i2c_device_id cy8c95x0_id[] = {
-	{ "cy8c9520", 20, },
-	{ "cy8c9540", 40, },
-	{ "cy8c9560", 60, },
-	{ }
-};
-MODULE_DEVICE_TABLE(i2c, cy8c95x0_id);
-
-#define OF_CY8C95X(__nrgpio) ((void *)(__nrgpio))
-
-static const struct of_device_id cy8c95x0_dt_ids[] = {
-	{ .compatible = "cypress,cy8c9520", .data = OF_CY8C95X(20), },
-	{ .compatible = "cypress,cy8c9540", .data = OF_CY8C95X(40), },
-	{ .compatible = "cypress,cy8c9560", .data = OF_CY8C95X(60), },
-	{ }
-};
-MODULE_DEVICE_TABLE(of, cy8c95x0_dt_ids);
-
 static const struct acpi_gpio_params cy8c95x0_irq_gpios = { 0, 0, true };
 
 static const struct acpi_gpio_mapping cy8c95x0_acpi_irq_gpios[] = {

@@ -1478,8 +1460,26 @@ static int cy8c95x0_probe(struct i2c_client *client)
 	return cy8c95x0_setup_gpiochip(chip);
 }
 
+static const struct i2c_device_id cy8c95x0_id[] = {
+	{ "cy8c9520", 20 },
+	{ "cy8c9540", 40 },
+	{ "cy8c9560", 60 },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, cy8c95x0_id);
+
+#define OF_CY8C95X(__nrgpio) ((void *)(__nrgpio))
+
+static const struct of_device_id cy8c95x0_dt_ids[] = {
+	{ .compatible = "cypress,cy8c9520", .data = OF_CY8C95X(20) },
+	{ .compatible = "cypress,cy8c9540", .data = OF_CY8C95X(40) },
+	{ .compatible = "cypress,cy8c9560", .data = OF_CY8C95X(60) },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, cy8c95x0_dt_ids);
+
 static const struct acpi_device_id cy8c95x0_acpi_ids[] = {
-	{ "INT3490", 40, },
+	{ "INT3490", 40 },
 	{ }
 };
 MODULE_DEVICE_TABLE(acpi, cy8c95x0_acpi_ids);
Signature · 1 line
2.50.1
maintainer
#9 lore raw
In reply to Andy Shevchenko
Quoted 6 lines · Andy Shevchenko

While playing on with Intel Galileo board again, I found that I messed up a bit with the driver. Here is a fix followed by a set of ad-hoc cleanups. Obviously tested on the above mentioned board.

Andy Shevchenko (8):
pinctrl: cy8c95x0: Don't miss reading the last bank registers

This patch applied for fixes!

Quoted 7 lines

pinctrl: cy8c95x0: Use devm_mutex_init() for mutex initialization
pinctrl: cy8c95x0: remove duplicate error message
pinctrl: cy8c95x0: Unify messages with help of dev_err_probe()
pinctrl: cy8c95x0: Move driver data to the local variable in ->probe()
pinctrl: cy8c95x0: Drop unused 'name' in struct cy8c95x0_pinctrl
pinctrl: cy8c95x0: Eliminate fragile use of I²C ID table
pinctrl: cy8c95x0: Gather ID tables in one place

The rest 7 for next.

Yours,
Linus Walleij