[PATCH v1 0/6] pinctrl: cy8c95x0: A portion of cleanups

[PATCH v1 0/6] pinctrl: cy8c95x0: A portion of cleanups

[Andy Shevchenko]

A portion of ad-hoc cleanups to the driver. Saves a few dozens of LoCs.

Andy Shevchenko (6):
  pinctrl: cy8c95x0: Use 2-argument strscpy()
  pinctrl: cy8c95x0: switch to using devm_regulator_get_enable()
  pinctrl: cy8c95x0: use flexible sleeping in reset function
  pinctrl: cy8c95x0: Use temporary variable for struct device
  pinctrl: cy8c95x0: embed iterator to the for-loop
  pinctrl: cy8c95x0: remove unneeded goto labels

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

-- 
2.47.0

[PATCH v1 1/6] pinctrl: cy8c95x0: Use 2-argument strscpy()

[Andy Shevchenko]

Use 2-argument strscpy(), which is not only shorter but also provides
an additional check that destination buffer is an array.

Signed-off-by: Andy Shevchenko <andy.shevchenko@gmail.com>
---
 drivers/pinctrl/pinctrl-cy8c95x0.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-cy8c95x0.c b/drivers/pinctrl/pinctrl-cy8c95x0.c
index 5096ccdd459e..5f533dff4417 100644
--- a/drivers/pinctrl/pinctrl-cy8c95x0.c
+++ b/drivers/pinctrl/pinctrl-cy8c95x0.c
@@ -1424,7 +1424,7 @@ static int cy8c95x0_detect(struct i2c_client *client,
 	}
 
 	dev_info(&client->dev, "Found a %s chip at 0x%02x.\n", name, client->addr);
-	strscpy(info->type, name, I2C_NAME_SIZE);
+	strscpy(info->type, name);
 
 	return 0;
 }
@@ -1457,15 +1457,15 @@ static int cy8c95x0_probe(struct i2c_client *client)
 
 	switch (chip->tpin) {
 	case 20:
-		strscpy(chip->name, cy8c95x0_id[0].name, I2C_NAME_SIZE);
+		strscpy(chip->name, cy8c95x0_id[0].name);
 		regmap_range_conf.range_max = CY8C95X0_VIRTUAL + 3 * MUXED_STRIDE;
 		break;
 	case 40:
-		strscpy(chip->name, cy8c95x0_id[1].name, I2C_NAME_SIZE);
+		strscpy(chip->name, cy8c95x0_id[1].name);
 		regmap_range_conf.range_max = CY8C95X0_VIRTUAL + 6 * MUXED_STRIDE;
 		break;
 	case 60:
-		strscpy(chip->name, cy8c95x0_id[2].name, I2C_NAME_SIZE);
+		strscpy(chip->name, cy8c95x0_id[2].name);
 		regmap_range_conf.range_max = CY8C95X0_VIRTUAL + 8 * MUXED_STRIDE;
 		break;
 	default:
-- 
2.47.0

[PATCH v1 2/6] pinctrl: cy8c95x0: switch to using devm_regulator_get_enable()

[Andy Shevchenko]

The driver does not actively manage regulator state past probe() time,
so we can use devm_regulator_get_enable() to simplify the code.

Signed-off-by: Andy Shevchenko <andy.shevchenko@gmail.com>
---
 drivers/pinctrl/pinctrl-cy8c95x0.c | 58 ++++++------------------------
 1 file changed, 11 insertions(+), 47 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-cy8c95x0.c b/drivers/pinctrl/pinctrl-cy8c95x0.c
index 5f533dff4417..cb6d9458c1e8 100644
--- a/drivers/pinctrl/pinctrl-cy8c95x0.c
+++ b/drivers/pinctrl/pinctrl-cy8c95x0.c
@@ -141,7 +141,6 @@ static const struct dmi_system_id cy8c95x0_dmi_acpi_irq_info[] = {
  * @nport:          Number of Gports in this chip
  * @gpio_chip:      gpiolib chip
  * @driver_data:    private driver data
- * @regulator:      Pointer to the regulator for the IC
  * @dev:            struct device
  * @pctldev:        pin controller device
  * @pinctrl_desc:   pin controller description
@@ -163,7 +162,6 @@ struct cy8c95x0_pinctrl {
 	int nport;
 	struct gpio_chip gpio_chip;
 	unsigned long driver_data;
-	struct regulator *regulator;
 	struct device *dev;
 	struct pinctrl_dev *pctldev;
 	struct pinctrl_desc pinctrl_desc;
@@ -1434,7 +1432,6 @@ 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;
 
 	chip = devm_kzalloc(&client->dev, sizeof(*chip), GFP_KERNEL);
@@ -1448,8 +1445,6 @@ static int cy8c95x0_probe(struct i2c_client *client)
 	if (!chip->driver_data)
 		return -ENODEV;
 
-	i2c_set_clientdata(client, chip);
-
 	chip->tpin = chip->driver_data & CY8C95X0_GPIO_MASK;
 	chip->nport = DIV_ROUND_UP(CY8C95X0_PIN_TO_OFFSET(chip->tpin), BANK_SZ);
 
@@ -1472,26 +1467,15 @@ static int cy8c95x0_probe(struct i2c_client *client)
 		return -ENODEV;
 	}
 
-	reg = devm_regulator_get(&client->dev, "vdd");
-	if (IS_ERR(reg)) {
-		if (PTR_ERR(reg) == -EPROBE_DEFER)
-			return -EPROBE_DEFER;
-	} else {
-		ret = regulator_enable(reg);
-		if (ret) {
-			dev_err(&client->dev, "failed to enable regulator vdd: %d\n", ret);
-			return ret;
-		}
-		chip->regulator = reg;
-	}
+	ret = devm_regulator_get_enable(&client->dev, "vdd");
+	if (ret)
+		return dev_err_probe(&client->dev, ret, "failed to enable regulator vdd\n");
 
 	/* bring the chip out of reset if reset pin is provided */
 	chip->gpio_reset = devm_gpiod_get_optional(&client->dev, "reset", GPIOD_OUT_HIGH);
-	if (IS_ERR(chip->gpio_reset)) {
-		ret = dev_err_probe(chip->dev, PTR_ERR(chip->gpio_reset),
-				    "Failed to get GPIO 'reset'\n");
-		goto err_exit;
-	} else if (chip->gpio_reset) {
+	if (IS_ERR(chip->gpio_reset))
+		return dev_err_probe(chip->dev, PTR_ERR(chip->gpio_reset), "Failed to get GPIO 'reset'\n");
+	if (chip->gpio_reset) {
 		usleep_range(1000, 2000);
 		gpiod_set_value_cansleep(chip->gpio_reset, 0);
 		usleep_range(250000, 300000);
@@ -1506,10 +1490,8 @@ static int cy8c95x0_probe(struct i2c_client *client)
 	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;
-	}
+	if (IS_ERR(chip->regmap))
+		return PTR_ERR(chip->regmap);
 
 	bitmap_zero(chip->push_pull, MAX_LINE);
 	bitmap_zero(chip->shiftmask, MAX_LINE);
@@ -1525,31 +1507,14 @@ static int cy8c95x0_probe(struct i2c_client *client)
 	if (client->irq) {
 		ret = cy8c95x0_irq_setup(chip, client->irq);
 		if (ret)
-			goto err_exit;
+			return ret;
 	}
 
 	ret = cy8c95x0_setup_pinctrl(chip);
 	if (ret)
-		goto err_exit;
+		return ret;
 
-	ret = cy8c95x0_setup_gpiochip(chip);
-	if (ret)
-		goto err_exit;
-
-	return 0;
-
-err_exit:
-	if (!IS_ERR_OR_NULL(chip->regulator))
-		regulator_disable(chip->regulator);
-	return ret;
-}
-
-static void cy8c95x0_remove(struct i2c_client *client)
-{
-	struct cy8c95x0_pinctrl *chip = i2c_get_clientdata(client);
-
-	if (!IS_ERR_OR_NULL(chip->regulator))
-		regulator_disable(chip->regulator);
+	return cy8c95x0_setup_gpiochip(chip);
 }
 
 static const struct acpi_device_id cy8c95x0_acpi_ids[] = {
@@ -1565,7 +1530,6 @@ static struct i2c_driver cy8c95x0_driver = {
 		.acpi_match_table = cy8c95x0_acpi_ids,
 	},
 	.probe		= cy8c95x0_probe,
-	.remove		= cy8c95x0_remove,
 	.id_table	= cy8c95x0_id,
 	.detect		= cy8c95x0_detect,
 };
-- 
2.47.0

[PATCH v1 3/6] pinctrl: cy8c95x0: use flexible sleeping in reset function

[Andy Shevchenko]

The device reset assert and deassert length was created by usleep_range()
but that does not ensure optimal handling of all the different values from
device tree properties. By switching to the new flexible sleeping helper
function, fsleep(), the correct delay function is called depending on
delay length, e.g. udelay(), usleep_range() or msleep().

While at it, move out GPIO consumer name setting of the reset conditional,
the all necessary checks are already done there, so logically that call is
not related to the reset sequence.

Signed-off-by: Andy Shevchenko <andy.shevchenko@gmail.com>
---
 drivers/pinctrl/pinctrl-cy8c95x0.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-cy8c95x0.c b/drivers/pinctrl/pinctrl-cy8c95x0.c
index cb6d9458c1e8..54b080f63226 100644
--- a/drivers/pinctrl/pinctrl-cy8c95x0.c
+++ b/drivers/pinctrl/pinctrl-cy8c95x0.c
@@ -1475,12 +1475,11 @@ static int cy8c95x0_probe(struct i2c_client *client)
 	chip->gpio_reset = devm_gpiod_get_optional(&client->dev, "reset", GPIOD_OUT_HIGH);
 	if (IS_ERR(chip->gpio_reset))
 		return dev_err_probe(chip->dev, PTR_ERR(chip->gpio_reset), "Failed to get GPIO 'reset'\n");
+	gpiod_set_consumer_name(chip->gpio_reset, "CY8C95X0 RESET");
 	if (chip->gpio_reset) {
-		usleep_range(1000, 2000);
+		fsleep(1000);
 		gpiod_set_value_cansleep(chip->gpio_reset, 0);
-		usleep_range(250000, 300000);
-
-		gpiod_set_consumer_name(chip->gpio_reset, "CY8C95X0 RESET");
+		fsleep(250000);
 	}
 
 	/* Regmap for direct and paged registers */
-- 
2.47.0

[PATCH v1 4/6] pinctrl: cy8c95x0: Use temporary variable for struct device

[Andy Shevchenko]

Use a temporary variable for the struct device pointers to avoid
dereferencing. This makes code a bit neater.

Signed-off-by: Andy Shevchenko <andy.shevchenko@gmail.com>
---
 drivers/pinctrl/pinctrl-cy8c95x0.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-cy8c95x0.c b/drivers/pinctrl/pinctrl-cy8c95x0.c
index 54b080f63226..8b118fd09e9e 100644
--- a/drivers/pinctrl/pinctrl-cy8c95x0.c
+++ b/drivers/pinctrl/pinctrl-cy8c95x0.c
@@ -1429,16 +1429,17 @@ static int cy8c95x0_detect(struct i2c_client *client,
 
 static int cy8c95x0_probe(struct i2c_client *client)
 {
+	struct device *dev = &client->dev;
 	struct cy8c95x0_pinctrl *chip;
 	struct regmap_config regmap_conf;
 	struct regmap_range_cfg regmap_range_conf;
 	int ret;
 
-	chip = devm_kzalloc(&client->dev, sizeof(*chip), GFP_KERNEL);
+	chip = devm_kzalloc(dev, sizeof(*chip), GFP_KERNEL);
 	if (!chip)
 		return -ENOMEM;
 
-	chip->dev = &client->dev;
+	chip->dev = dev;
 
 	/* Set the device type */
 	chip->driver_data = (uintptr_t)i2c_get_match_data(client);
@@ -1467,14 +1468,14 @@ static int cy8c95x0_probe(struct i2c_client *client)
 		return -ENODEV;
 	}
 
-	ret = devm_regulator_get_enable(&client->dev, "vdd");
+	ret = devm_regulator_get_enable(dev, "vdd");
 	if (ret)
-		return dev_err_probe(&client->dev, ret, "failed to enable regulator vdd\n");
+		return dev_err_probe(dev, ret, "failed to enable regulator vdd\n");
 
 	/* bring the chip out of reset if reset pin is provided */
-	chip->gpio_reset = devm_gpiod_get_optional(&client->dev, "reset", GPIOD_OUT_HIGH);
+	chip->gpio_reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
 	if (IS_ERR(chip->gpio_reset))
-		return dev_err_probe(chip->dev, PTR_ERR(chip->gpio_reset), "Failed to get GPIO 'reset'\n");
+		return dev_err_probe(dev, PTR_ERR(chip->gpio_reset), "Failed to get GPIO 'reset'\n");
 	gpiod_set_consumer_name(chip->gpio_reset, "CY8C95X0 RESET");
 	if (chip->gpio_reset) {
 		fsleep(1000);
-- 
2.47.0

[PATCH v1 5/6] pinctrl: cy8c95x0: embed iterator to the for-loop

[Andy Shevchenko]

When we iterate through nports the iterator variable is effectively
being not used outside of the loop. Make it clear by moving its definition
into the for-loop. This makes code cleaner as well.

Signed-off-by: Andy Shevchenko <andy.shevchenko@gmail.com>
---
 drivers/pinctrl/pinctrl-cy8c95x0.c | 36 +++++++++++++-----------------
 1 file changed, 15 insertions(+), 21 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-cy8c95x0.c b/drivers/pinctrl/pinctrl-cy8c95x0.c
index 8b118fd09e9e..8c611abd4745 100644
--- a/drivers/pinctrl/pinctrl-cy8c95x0.c
+++ b/drivers/pinctrl/pinctrl-cy8c95x0.c
@@ -159,7 +159,7 @@ struct cy8c95x0_pinctrl {
 	DECLARE_BITMAP(irq_trig_high, MAX_LINE);
 	DECLARE_BITMAP(push_pull, MAX_LINE);
 	DECLARE_BITMAP(shiftmask, MAX_LINE);
-	int nport;
+	unsigned int nport;
 	struct gpio_chip gpio_chip;
 	unsigned long driver_data;
 	struct device *dev;
@@ -610,9 +610,8 @@ static int cy8c95x0_write_regs_mask(struct cy8c95x0_pinctrl *chip, int reg,
 	DECLARE_BITMAP(tmask, MAX_LINE);
 	DECLARE_BITMAP(tval, MAX_LINE);
 	int write_val;
-	int ret = 0;
-	int i;
 	u8 bits;
+	int ret;
 
 	/* Add the 4 bit gap of Gport2 */
 	bitmap_andnot(tmask, mask, chip->shiftmask, MAX_LINE);
@@ -623,7 +622,7 @@ static int cy8c95x0_write_regs_mask(struct cy8c95x0_pinctrl *chip, int reg,
 	bitmap_shift_left(tval, tval, 4, MAX_LINE);
 	bitmap_replace(tval, tval, val, chip->shiftmask, BANK_SZ * 3);
 
-	for (i = 0; i < chip->nport; i++) {
+	for (unsigned int i = 0; i < chip->nport; i++) {
 		/* Skip over unused banks */
 		bits = bitmap_get_value8(tmask, i * BANK_SZ);
 		if (!bits)
@@ -632,15 +631,13 @@ static int cy8c95x0_write_regs_mask(struct cy8c95x0_pinctrl *chip, int reg,
 		write_val = bitmap_get_value8(tval, i * BANK_SZ);
 
 		ret = cy8c95x0_regmap_update_bits(chip, reg, i, bits, write_val);
-		if (ret < 0)
-			goto out;
+		if (ret < 0) {
+			dev_err(chip->dev, "failed writing register %d, port %u: err %d\n", reg, i, ret);
+			return ret;
+		}
 	}
-out:
 
-	if (ret < 0)
-		dev_err(chip->dev, "failed writing register %d, port %d: err %d\n", reg, i, ret);
-
-	return ret;
+	return 0;
 }
 
 static int cy8c95x0_read_regs_mask(struct cy8c95x0_pinctrl *chip, int reg,
@@ -650,9 +647,8 @@ static int cy8c95x0_read_regs_mask(struct cy8c95x0_pinctrl *chip, int reg,
 	DECLARE_BITMAP(tval, MAX_LINE);
 	DECLARE_BITMAP(tmp, MAX_LINE);
 	int read_val;
-	int ret = 0;
-	int i;
 	u8 bits;
+	int ret;
 
 	/* Add the 4 bit gap of Gport2 */
 	bitmap_andnot(tmask, mask, chip->shiftmask, MAX_LINE);
@@ -663,15 +659,17 @@ static int cy8c95x0_read_regs_mask(struct cy8c95x0_pinctrl *chip, int reg,
 	bitmap_shift_left(tval, tval, 4, MAX_LINE);
 	bitmap_replace(tval, tval, val, chip->shiftmask, BANK_SZ * 3);
 
-	for (i = 0; i < chip->nport; i++) {
+	for (unsigned int i = 0; i < chip->nport; i++) {
 		/* Skip over unused banks */
 		bits = bitmap_get_value8(tmask, i * BANK_SZ);
 		if (!bits)
 			continue;
 
 		ret = cy8c95x0_regmap_read(chip, reg, i, &read_val);
-		if (ret < 0)
-			goto out;
+		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, i * BANK_SZ) & ~bits;
@@ -682,11 +680,7 @@ static int cy8c95x0_read_regs_mask(struct cy8c95x0_pinctrl *chip, int reg,
 	bitmap_shift_right(tmp, tval, 4, MAX_LINE);
 	bitmap_replace(val, tmp, tval, chip->shiftmask, MAX_LINE);
 
-out:
-	if (ret < 0)
-		dev_err(chip->dev, "failed reading register %d, port %d: err %d\n", reg, i, ret);
-
-	return ret;
+	return 0;
 }
 
 static int cy8c95x0_gpio_direction_input(struct gpio_chip *gc, unsigned int off)
-- 
2.47.0

[PATCH v1 6/6] pinctrl: cy8c95x0: remove unneeded goto labels

[Andy Shevchenko]

In some cases the code uses goto labels to just return an error code.
Replace those with direct return:s and drop unneeded goto labels.

Signed-off-by: Andy Shevchenko <andy.shevchenko@gmail.com>
---
 drivers/pinctrl/pinctrl-cy8c95x0.c | 26 ++++++++------------------
 1 file changed, 8 insertions(+), 18 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-cy8c95x0.c b/drivers/pinctrl/pinctrl-cy8c95x0.c
index 8c611abd4745..0d6c2027d4c1 100644
--- a/drivers/pinctrl/pinctrl-cy8c95x0.c
+++ b/drivers/pinctrl/pinctrl-cy8c95x0.c
@@ -746,14 +746,12 @@ static int cy8c95x0_gpio_get_direction(struct gpio_chip *gc, unsigned int off)
 
 	ret = cy8c95x0_regmap_read(chip, CY8C95X0_DIRECTION, port, &reg_val);
 	if (ret < 0)
-		goto out;
+		return ret;
 
 	if (reg_val & bit)
 		return GPIO_LINE_DIRECTION_IN;
 
 	return GPIO_LINE_DIRECTION_OUT;
-out:
-	return ret;
 }
 
 static int cy8c95x0_gpio_get_pincfg(struct cy8c95x0_pinctrl *chip,
@@ -815,8 +813,7 @@ static int cy8c95x0_gpio_get_pincfg(struct cy8c95x0_pinctrl *chip,
 	case PIN_CONFIG_SLEEP_HARDWARE_STATE:
 	case PIN_CONFIG_SLEW_RATE:
 	default:
-		ret = -ENOTSUPP;
-		goto out;
+		return -ENOTSUPP;
 	}
 	/*
 	 * Writing 1 to one of the drive mode registers will automatically
@@ -824,7 +821,7 @@ static int cy8c95x0_gpio_get_pincfg(struct cy8c95x0_pinctrl *chip,
 	 */
 	ret = cy8c95x0_regmap_read(chip, reg, port, &reg_val);
 	if (ret < 0)
-		goto out;
+		return ret;
 
 	if (reg_val & bit)
 		arg = 1;
@@ -832,8 +829,7 @@ static int cy8c95x0_gpio_get_pincfg(struct cy8c95x0_pinctrl *chip,
 		arg = !arg;
 
 	*config = pinconf_to_config_packed(param, (u16)arg);
-out:
-	return ret;
+	return 0;
 }
 
 static int cy8c95x0_gpio_set_pincfg(struct cy8c95x0_pinctrl *chip,
@@ -845,7 +841,6 @@ static int cy8c95x0_gpio_set_pincfg(struct cy8c95x0_pinctrl *chip,
 	unsigned long param = pinconf_to_config_param(config);
 	unsigned long arg = pinconf_to_config_argument(config);
 	unsigned int reg;
-	int ret;
 
 	switch (param) {
 	case PIN_CONFIG_BIAS_PULL_UP:
@@ -876,22 +871,17 @@ static int cy8c95x0_gpio_set_pincfg(struct cy8c95x0_pinctrl *chip,
 		reg = CY8C95X0_PWMSEL;
 		break;
 	case PIN_CONFIG_OUTPUT_ENABLE:
-		ret = cy8c95x0_pinmux_direction(chip, off, !arg);
-		goto out;
+		return cy8c95x0_pinmux_direction(chip, off, !arg);
 	case PIN_CONFIG_INPUT_ENABLE:
-		ret = cy8c95x0_pinmux_direction(chip, off, arg);
-		goto out;
+		return cy8c95x0_pinmux_direction(chip, off, arg);
 	default:
-		ret = -ENOTSUPP;
-		goto out;
+		return -ENOTSUPP;
 	}
 	/*
 	 * Writing 1 to one of the drive mode registers will automatically
 	 * clear conflicting set bits in the other drive mode registers.
 	 */
-	ret = cy8c95x0_regmap_write_bits(chip, reg, port, bit, bit);
-out:
-	return ret;
+	return cy8c95x0_regmap_write_bits(chip, reg, port, bit, bit);
 }
 
 static int cy8c95x0_gpio_get_multiple(struct gpio_chip *gc,
-- 
2.47.0

Re: [PATCH v1 0/6] pinctrl: cy8c95x0: A portion of cleanups

[Linus Walleij]

On Sun, Nov 10, 2024 at 10:00 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:

> A portion of ad-hoc cleanups to the driver. Saves a few dozens of LoCs.
>
> Andy Shevchenko (6):
>   pinctrl: cy8c95x0: Use 2-argument strscpy()
>   pinctrl: cy8c95x0: switch to using devm_regulator_get_enable()
>   pinctrl: cy8c95x0: use flexible sleeping in reset function
>   pinctrl: cy8c95x0: Use temporary variable for struct device
>   pinctrl: cy8c95x0: embed iterator to the for-loop
>   pinctrl: cy8c95x0: remove unneeded goto labels

I really like the patches so I merged them so we get some rotation in
linux-next.

Thanks Andy!

Yours,
Linus Walleij