[PATCH v1 01/10] gpio: pca953x: Drop unused fields in struct pca953x_platform_data

[PATCH v1 01/10] gpio: pca953x: Drop unused fields in struct pca953x_platform_data

[Andy Shevchenko]

New code should solely use firmware nodes for the specifics and
not any callbacks.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/gpio/gpio-pca953x.c           | 37 ++++++---------------------
 include/linux/platform_data/pca953x.h | 13 ----------
 2 files changed, 8 insertions(+), 42 deletions(-)

diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c
index bdd50a78e414..02695abd0eb1 100644
--- a/drivers/gpio/gpio-pca953x.c
+++ b/drivers/gpio/gpio-pca953x.c
@@ -211,7 +211,6 @@ struct pca953x_chip {
 
 	struct i2c_client *client;
 	struct gpio_chip gpio_chip;
-	const char *const *names;
 	unsigned long driver_data;
 	struct regulator *regulator;
 
@@ -712,7 +711,6 @@ static void pca953x_setup_gpio(struct pca953x_chip *chip, int gpios)
 	gc->label = dev_name(&chip->client->dev);
 	gc->parent = &chip->client->dev;
 	gc->owner = THIS_MODULE;
-	gc->names = chip->names;
 }
 
 #ifdef CONFIG_GPIO_PCA953X_IRQ
@@ -998,7 +996,7 @@ static int pca953x_irq_setup(struct pca953x_chip *chip,
 }
 #endif
 
-static int device_pca95xx_init(struct pca953x_chip *chip, u32 invert)
+static int device_pca95xx_init(struct pca953x_chip *chip)
 {
 	DECLARE_BITMAP(val, MAX_LINE);
 	u8 regaddr;
@@ -1016,24 +1014,21 @@ static int device_pca95xx_init(struct pca953x_chip *chip, u32 invert)
 	if (ret)
 		goto out;
 
-	/* set platform specific polarity inversion */
-	if (invert)
-		bitmap_fill(val, MAX_LINE);
-	else
-		bitmap_zero(val, MAX_LINE);
+	/* clear polarity inversion */
+	bitmap_zero(val, MAX_LINE);
 
 	ret = pca953x_write_regs(chip, chip->regs->invert, val);
 out:
 	return ret;
 }
 
-static int device_pca957x_init(struct pca953x_chip *chip, u32 invert)
+static int device_pca957x_init(struct pca953x_chip *chip)
 {
 	DECLARE_BITMAP(val, MAX_LINE);
 	unsigned int i;
 	int ret;
 
-	ret = device_pca95xx_init(chip, invert);
+	ret = device_pca95xx_init(chip);
 	if (ret)
 		goto out;
 
@@ -1054,9 +1049,8 @@ static int pca953x_probe(struct i2c_client *client)
 {
 	struct pca953x_platform_data *pdata;
 	struct pca953x_chip *chip;
-	int irq_base = 0;
+	int irq_base;
 	int ret;
-	u32 invert = 0;
 	struct regulator *reg;
 	const struct regmap_config *regmap_config;
 
@@ -1068,8 +1062,6 @@ static int pca953x_probe(struct i2c_client *client)
 	if (pdata) {
 		irq_base = pdata->irq_base;
 		chip->gpio_start = pdata->gpio_base;
-		invert = pdata->invert;
-		chip->names = pdata->names;
 	} else {
 		struct gpio_desc *reset_gpio;
 
@@ -1158,10 +1150,10 @@ static int pca953x_probe(struct i2c_client *client)
 	 */
 	if (PCA_CHIP_TYPE(chip->driver_data) == PCA957X_TYPE) {
 		chip->regs = &pca957x_regs;
-		ret = device_pca957x_init(chip, invert);
+		ret = device_pca957x_init(chip);
 	} else {
 		chip->regs = &pca953x_regs;
-		ret = device_pca95xx_init(chip, invert);
+		ret = device_pca95xx_init(chip);
 	}
 	if (ret)
 		goto err_exit;
@@ -1174,13 +1166,6 @@ static int pca953x_probe(struct i2c_client *client)
 	if (ret)
 		goto err_exit;
 
-	if (pdata && pdata->setup) {
-		ret = pdata->setup(client, chip->gpio_chip.base,
-				   chip->gpio_chip.ngpio, pdata->context);
-		if (ret < 0)
-			dev_warn(&client->dev, "setup failed, %d\n", ret);
-	}
-
 	return 0;
 
 err_exit:
@@ -1190,14 +1175,8 @@ static int pca953x_probe(struct i2c_client *client)
 
 static void pca953x_remove(struct i2c_client *client)
 {
-	struct pca953x_platform_data *pdata = dev_get_platdata(&client->dev);
 	struct pca953x_chip *chip = i2c_get_clientdata(client);
 
-	if (pdata && pdata->teardown) {
-		pdata->teardown(client, chip->gpio_chip.base,
-				chip->gpio_chip.ngpio, pdata->context);
-	}
-
 	regulator_disable(chip->regulator);
 }
 
diff --git a/include/linux/platform_data/pca953x.h b/include/linux/platform_data/pca953x.h
index 96c1a14ab365..3c3787c4d96c 100644
--- a/include/linux/platform_data/pca953x.h
+++ b/include/linux/platform_data/pca953x.h
@@ -11,21 +11,8 @@ struct pca953x_platform_data {
 	/* number of the first GPIO */
 	unsigned	gpio_base;
 
-	/* initial polarity inversion setting */
-	u32		invert;
-
 	/* interrupt base */
 	int		irq_base;
-
-	void		*context;	/* param to setup/teardown */
-
-	int		(*setup)(struct i2c_client *client,
-				unsigned gpio, unsigned ngpio,
-				void *context);
-	void		(*teardown)(struct i2c_client *client,
-				unsigned gpio, unsigned ngpio,
-				void *context);
-	const char	*const *names;
 };
 
 #endif /* _LINUX_PCA953X_H */
-- 
2.40.0.1.gaa8946217a0b

[PATCH v1 02/10] gpio: pca953x: Fully convert to device managed resources

[Andy Shevchenko]

Curtrently the error path is unsynchronised with removal due to
regulator being disabled before other device managed resources
are handled. Correct that by wrapping regulator enablement in
the respective call.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/gpio/gpio-pca953x.c | 68 +++++++++++++++++++------------------
 1 file changed, 35 insertions(+), 33 deletions(-)

diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c
index 02695abd0eb1..0dedb2265744 100644
--- a/drivers/gpio/gpio-pca953x.c
+++ b/drivers/gpio/gpio-pca953x.c
@@ -1045,13 +1045,40 @@ static int device_pca957x_init(struct pca953x_chip *chip)
 	return ret;
 }
 
+static void pca953x_disable_regulator(void *reg)
+{
+	regulator_disable(reg);
+}
+
+static int pca953x_get_and_enable_regulator(struct pca953x_chip *chip)
+{
+	struct device *dev = &chip->client->dev;
+	struct regulator *reg = chip->regulator;
+	int ret;
+
+	reg = devm_regulator_get(dev, "vcc");
+	if (IS_ERR(reg))
+		return dev_err_probe(dev, PTR_ERR(reg), "reg get err\n");
+
+	ret = regulator_enable(reg);
+	if (ret)
+	        return dev_err_probe(dev, ret, "reg en err\n");
+
+	ret = devm_add_action_or_reset(dev, pca953x_disable_regulator, reg);
+	if (ret)
+		return ret;
+
+	chip->regulator = reg;
+	return 0;
+}
+
 static int pca953x_probe(struct i2c_client *client)
 {
+	struct device *dev = &client->dev;
 	struct pca953x_platform_data *pdata;
 	struct pca953x_chip *chip;
 	int irq_base;
 	int ret;
-	struct regulator *reg;
 	const struct regmap_config *regmap_config;
 
 	chip = devm_kzalloc(&client->dev, sizeof(*chip), GFP_KERNEL);
@@ -1086,16 +1113,9 @@ static int pca953x_probe(struct i2c_client *client)
 	if (!chip->driver_data)
 		return -ENODEV;
 
-	reg = devm_regulator_get(&client->dev, "vcc");
-	if (IS_ERR(reg))
-		return dev_err_probe(&client->dev, PTR_ERR(reg), "reg get err\n");
-
-	ret = regulator_enable(reg);
-	if (ret) {
-		dev_err(&client->dev, "reg en err: %d\n", ret);
+	ret = pca953x_get_and_enable_regulator(chip);
+	if (ret)
 		return ret;
-	}
-	chip->regulator = reg;
 
 	i2c_set_clientdata(client, chip);
 
@@ -1118,10 +1138,8 @@ static int pca953x_probe(struct i2c_client *client)
 	}
 
 	chip->regmap = devm_regmap_init_i2c(client, regmap_config);
-	if (IS_ERR(chip->regmap)) {
-		ret = PTR_ERR(chip->regmap);
-		goto err_exit;
-	}
+	if (IS_ERR(chip->regmap))
+		return PTR_ERR(chip->regmap);
 
 	regcache_mark_dirty(chip->regmap);
 
@@ -1156,28 +1174,13 @@ static int pca953x_probe(struct i2c_client *client)
 		ret = device_pca95xx_init(chip);
 	}
 	if (ret)
-		goto err_exit;
+		return ret;
 
 	ret = pca953x_irq_setup(chip, irq_base);
 	if (ret)
-		goto err_exit;
+		return ret;
 
-	ret = devm_gpiochip_add_data(&client->dev, &chip->gpio_chip, chip);
-	if (ret)
-		goto err_exit;
-
-	return 0;
-
-err_exit:
-	regulator_disable(chip->regulator);
-	return ret;
-}
-
-static void pca953x_remove(struct i2c_client *client)
-{
-	struct pca953x_chip *chip = i2c_get_clientdata(client);
-
-	regulator_disable(chip->regulator);
+	return devm_gpiochip_add_data(dev, &chip->gpio_chip, chip);
 }
 
 #ifdef CONFIG_PM_SLEEP
@@ -1345,7 +1348,6 @@ static struct i2c_driver pca953x_driver = {
 		.acpi_match_table = pca953x_acpi_ids,
 	},
 	.probe		= pca953x_probe,
-	.remove		= pca953x_remove,
 	.id_table	= pca953x_id,
 };
 
-- 
2.40.0.1.gaa8946217a0b

[PATCH v1 03/10] gpio: pca953x: Utilise dev_err_probe() where it makes sense

[Andy Shevchenko]

At least in pca953x_irq_setup() we may use dev_err_probe().

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

diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c
index 0dedb2265744..4249ec350ace 100644
--- a/drivers/gpio/gpio-pca953x.c
+++ b/drivers/gpio/gpio-pca953x.c
@@ -926,6 +926,7 @@ static irqreturn_t pca953x_irq_handler(int irq, void *devid)
 static int pca953x_irq_setup(struct pca953x_chip *chip, int irq_base)
 {
 	struct i2c_client *client = chip->client;
+	struct device *dev = &client->dev;
 	DECLARE_BITMAP(reg_direction, MAX_LINE);
 	DECLARE_BITMAP(irq_stat, MAX_LINE);
 	struct gpio_irq_chip *girq;
@@ -974,11 +975,8 @@ static int pca953x_irq_setup(struct pca953x_chip *chip, int irq_base)
 					NULL, pca953x_irq_handler,
 					IRQF_ONESHOT | IRQF_SHARED,
 					dev_name(&client->dev), chip);
-	if (ret) {
-		dev_err(&client->dev, "failed to request irq %d\n",
-			client->irq);
-		return ret;
-	}
+	if (ret)
+		return dev_err_probe(dev, client->irq, "failed to request irq\n");
 
 	return 0;
 }
-- 
2.40.0.1.gaa8946217a0b

[PATCH v1 04/10] gpio: pca953x: Split pca953x_restore_context() and pca953x_save_context()

[Andy Shevchenko]

Split regcache handling to the respective helpers. It will allow to
have further refactoring with ease.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/gpio/gpio-pca953x.c | 44 ++++++++++++++++++++++++-------------
 1 file changed, 29 insertions(+), 15 deletions(-)

diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c
index 4249ec350ace..71d87d570a2b 100644
--- a/drivers/gpio/gpio-pca953x.c
+++ b/drivers/gpio/gpio-pca953x.c
@@ -1182,9 +1182,9 @@ static int pca953x_probe(struct i2c_client *client)
 }
 
 #ifdef CONFIG_PM_SLEEP
-static int pca953x_regcache_sync(struct device *dev)
+static int pca953x_regcache_sync(struct pca953x_chip *chip)
 {
-	struct pca953x_chip *chip = dev_get_drvdata(dev);
+	struct device *dev = &chip->client->dev;
 	int ret;
 	u8 regaddr;
 
@@ -1231,13 +1231,37 @@ static int pca953x_regcache_sync(struct device *dev)
 	return 0;
 }
 
-static int pca953x_suspend(struct device *dev)
+static int pca953x_restore_context(struct pca953x_chip *chip)
 {
-	struct pca953x_chip *chip = dev_get_drvdata(dev);
+	int ret;
 
+	mutex_lock(&chip->i2c_lock);
+
+	regcache_cache_only(chip->regmap, false);
+	regcache_mark_dirty(chip->regmap);
+	ret = pca953x_regcache_sync(chip);
+	if (ret) {
+		mutex_unlock(&chip->i2c_lock);
+		return ret;
+	}
+
+	ret = regcache_sync(chip->regmap);
+	mutex_unlock(&chip->i2c_lock);
+	return ret;
+}
+
+static void pca953x_save_context(struct pca953x_chip *chip)
+{
 	mutex_lock(&chip->i2c_lock);
 	regcache_cache_only(chip->regmap, true);
 	mutex_unlock(&chip->i2c_lock);
+}
+
+static int pca953x_suspend(struct device *dev)
+{
+	struct pca953x_chip *chip = dev_get_drvdata(dev);
+
+	pca953x_save_context(chip);
 
 	if (atomic_read(&chip->wakeup_path))
 		device_set_wakeup_path(dev);
@@ -1260,17 +1284,7 @@ static int pca953x_resume(struct device *dev)
 		}
 	}
 
-	mutex_lock(&chip->i2c_lock);
-	regcache_cache_only(chip->regmap, false);
-	regcache_mark_dirty(chip->regmap);
-	ret = pca953x_regcache_sync(dev);
-	if (ret) {
-		mutex_unlock(&chip->i2c_lock);
-		return ret;
-	}
-
-	ret = regcache_sync(chip->regmap);
-	mutex_unlock(&chip->i2c_lock);
+	ret = pca953x_restore_context(chip);
 	if (ret) {
 		dev_err(dev, "Failed to restore register map: %d\n", ret);
 		return ret;
-- 
2.40.0.1.gaa8946217a0b

[PATCH v1 05/10] gpio: pca953x: Simplify code with cleanup helpers

[Andy Shevchenko]

Use macros defined in linux/cleanup.h to automate resource lifetime
control in gpio-pca953x.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/gpio/gpio-pca953x.c | 77 ++++++++++++++-----------------------
 1 file changed, 29 insertions(+), 48 deletions(-)

diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c
index 71d87d570a2b..99379ee98749 100644
--- a/drivers/gpio/gpio-pca953x.c
+++ b/drivers/gpio/gpio-pca953x.c
@@ -10,6 +10,7 @@
 
 #include <linux/acpi.h>
 #include <linux/bitmap.h>
+#include <linux/cleanup.h>
 #include <linux/gpio/consumer.h>
 #include <linux/gpio/driver.h>
 #include <linux/i2c.h>
@@ -519,12 +520,10 @@ static int pca953x_gpio_direction_input(struct gpio_chip *gc, unsigned off)
 	struct pca953x_chip *chip = gpiochip_get_data(gc);
 	u8 dirreg = chip->recalc_addr(chip, chip->regs->direction, off);
 	u8 bit = BIT(off % BANK_SZ);
-	int ret;
 
-	mutex_lock(&chip->i2c_lock);
-	ret = regmap_write_bits(chip->regmap, dirreg, bit, bit);
-	mutex_unlock(&chip->i2c_lock);
-	return ret;
+	guard(mutex)(&chip->i2c_lock);
+
+	return regmap_write_bits(chip->regmap, dirreg, bit, bit);
 }
 
 static int pca953x_gpio_direction_output(struct gpio_chip *gc,
@@ -536,17 +535,15 @@ static int pca953x_gpio_direction_output(struct gpio_chip *gc,
 	u8 bit = BIT(off % BANK_SZ);
 	int ret;
 
-	mutex_lock(&chip->i2c_lock);
+	guard(mutex)(&chip->i2c_lock);
+
 	/* set output level */
 	ret = regmap_write_bits(chip->regmap, outreg, bit, val ? bit : 0);
 	if (ret)
-		goto exit;
+		return ret;
 
 	/* then direction */
-	ret = regmap_write_bits(chip->regmap, dirreg, bit, 0);
-exit:
-	mutex_unlock(&chip->i2c_lock);
-	return ret;
+	return regmap_write_bits(chip->regmap, dirreg, bit, 0);
 }
 
 static int pca953x_gpio_get_value(struct gpio_chip *gc, unsigned off)
@@ -557,9 +554,8 @@ static int pca953x_gpio_get_value(struct gpio_chip *gc, unsigned off)
 	u32 reg_val;
 	int ret;
 
-	mutex_lock(&chip->i2c_lock);
-	ret = regmap_read(chip->regmap, inreg, &reg_val);
-	mutex_unlock(&chip->i2c_lock);
+	scoped_guard(mutex, &chip->i2c_lock)
+		ret = regmap_read(chip->regmap, inreg, &reg_val);
 	if (ret < 0)
 		return ret;
 
@@ -572,9 +568,9 @@ static void pca953x_gpio_set_value(struct gpio_chip *gc, unsigned off, int val)
 	u8 outreg = chip->recalc_addr(chip, chip->regs->output, off);
 	u8 bit = BIT(off % BANK_SZ);
 
-	mutex_lock(&chip->i2c_lock);
+	guard(mutex)(&chip->i2c_lock);
+
 	regmap_write_bits(chip->regmap, outreg, bit, val ? bit : 0);
-	mutex_unlock(&chip->i2c_lock);
 }
 
 static int pca953x_gpio_get_direction(struct gpio_chip *gc, unsigned off)
@@ -585,9 +581,8 @@ static int pca953x_gpio_get_direction(struct gpio_chip *gc, unsigned off)
 	u32 reg_val;
 	int ret;
 
-	mutex_lock(&chip->i2c_lock);
-	ret = regmap_read(chip->regmap, dirreg, &reg_val);
-	mutex_unlock(&chip->i2c_lock);
+	scoped_guard(mutex, &chip->i2c_lock)
+		ret = regmap_read(chip->regmap, dirreg, &reg_val);
 	if (ret < 0)
 		return ret;
 
@@ -604,9 +599,8 @@ static int pca953x_gpio_get_multiple(struct gpio_chip *gc,
 	DECLARE_BITMAP(reg_val, MAX_LINE);
 	int ret;
 
-	mutex_lock(&chip->i2c_lock);
-	ret = pca953x_read_regs(chip, chip->regs->input, reg_val);
-	mutex_unlock(&chip->i2c_lock);
+	scoped_guard(mutex, &chip->i2c_lock)
+		ret = pca953x_read_regs(chip, chip->regs->input, reg_val);
 	if (ret)
 		return ret;
 
@@ -621,16 +615,15 @@ static void pca953x_gpio_set_multiple(struct gpio_chip *gc,
 	DECLARE_BITMAP(reg_val, MAX_LINE);
 	int ret;
 
-	mutex_lock(&chip->i2c_lock);
+	guard(mutex)(&chip->i2c_lock);
+
 	ret = pca953x_read_regs(chip, chip->regs->output, reg_val);
 	if (ret)
-		goto exit;
+		return;
 
 	bitmap_replace(reg_val, reg_val, bits, mask, gc->ngpio);
 
 	pca953x_write_regs(chip, chip->regs->output, reg_val);
-exit:
-	mutex_unlock(&chip->i2c_lock);
 }
 
 static int pca953x_gpio_set_pull_up_down(struct pca953x_chip *chip,
@@ -638,7 +631,6 @@ static int pca953x_gpio_set_pull_up_down(struct pca953x_chip *chip,
 					 unsigned long config)
 {
 	enum pin_config_param param = pinconf_to_config_param(config);
-
 	u8 pull_en_reg = chip->recalc_addr(chip, PCAL953X_PULL_EN, offset);
 	u8 pull_sel_reg = chip->recalc_addr(chip, PCAL953X_PULL_SEL, offset);
 	u8 bit = BIT(offset % BANK_SZ);
@@ -651,7 +643,7 @@ static int pca953x_gpio_set_pull_up_down(struct pca953x_chip *chip,
 	if (!(chip->driver_data & PCA_PCAL))
 		return -ENOTSUPP;
 
-	mutex_lock(&chip->i2c_lock);
+	guard(mutex)(&chip->i2c_lock);
 
 	/* Configure pull-up/pull-down */
 	if (param == PIN_CONFIG_BIAS_PULL_UP)
@@ -661,17 +653,13 @@ static int pca953x_gpio_set_pull_up_down(struct pca953x_chip *chip,
 	else
 		ret = 0;
 	if (ret)
-		goto exit;
+		return ret;
 
 	/* Disable/Enable pull-up/pull-down */
 	if (param == PIN_CONFIG_BIAS_DISABLE)
-		ret = regmap_write_bits(chip->regmap, pull_en_reg, bit, 0);
+		return regmap_write_bits(chip->regmap, pull_en_reg, bit, 0);
 	else
-		ret = regmap_write_bits(chip->regmap, pull_en_reg, bit, bit);
-
-exit:
-	mutex_unlock(&chip->i2c_lock);
-	return ret;
+		return regmap_write_bits(chip->regmap, pull_en_reg, bit, bit);
 }
 
 static int pca953x_gpio_set_config(struct gpio_chip *gc, unsigned int offset,
@@ -900,10 +888,8 @@ static irqreturn_t pca953x_irq_handler(int irq, void *devid)
 
 	bitmap_zero(pending, MAX_LINE);
 
-	mutex_lock(&chip->i2c_lock);
-	ret = pca953x_irq_pending(chip, pending);
-	mutex_unlock(&chip->i2c_lock);
-
+	scoped_guard(mutex, &chip->i2c_lock)
+		ret = pca953x_irq_pending(chip, pending);
 	if (ret) {
 		ret = 0;
 
@@ -1235,26 +1221,21 @@ static int pca953x_restore_context(struct pca953x_chip *chip)
 {
 	int ret;
 
-	mutex_lock(&chip->i2c_lock);
+	guard(mutex)(&chip->i2c_lock);
 
 	regcache_cache_only(chip->regmap, false);
 	regcache_mark_dirty(chip->regmap);
 	ret = pca953x_regcache_sync(chip);
-	if (ret) {
-		mutex_unlock(&chip->i2c_lock);
+	if (ret)
 		return ret;
-	}
 
-	ret = regcache_sync(chip->regmap);
-	mutex_unlock(&chip->i2c_lock);
-	return ret;
+	return regcache_sync(chip->regmap);
 }
 
 static void pca953x_save_context(struct pca953x_chip *chip)
 {
-	mutex_lock(&chip->i2c_lock);
+	guard(mutex)(&chip->i2c_lock);
 	regcache_cache_only(chip->regmap, true);
-	mutex_unlock(&chip->i2c_lock);
 }
 
 static int pca953x_suspend(struct device *dev)
-- 
2.40.0.1.gaa8946217a0b

[PATCH v1 06/10] gpio: pca953x: Utilise temporary variable for struct device

[Andy Shevchenko]

We have a temporary variable to keep pointer to struct device.
Utilise it where it makes sense.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/gpio/gpio-pca953x.c | 30 ++++++++++++++----------------
 1 file changed, 14 insertions(+), 16 deletions(-)

diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c
index 99379ee98749..4aa15128c91f 100644
--- a/drivers/gpio/gpio-pca953x.c
+++ b/drivers/gpio/gpio-pca953x.c
@@ -779,11 +779,11 @@ static int pca953x_irq_set_type(struct irq_data *d, unsigned int type)
 {
 	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
 	struct pca953x_chip *chip = gpiochip_get_data(gc);
+	struct device *dev = &chip->client->dev;
 	irq_hw_number_t hwirq = irqd_to_hwirq(d);
 
 	if (!(type & IRQ_TYPE_EDGE_BOTH)) {
-		dev_err(&chip->client->dev, "irq %d: unsupported type %d\n",
-			d->irq, type);
+		dev_err(dev, "irq %d: unsupported type %d\n", d->irq, type);
 		return -EINVAL;
 	}
 
@@ -919,7 +919,7 @@ static int pca953x_irq_setup(struct pca953x_chip *chip, int irq_base)
 	int ret;
 
 	if (dmi_first_match(pca953x_dmi_acpi_irq_info)) {
-		ret = pca953x_acpi_get_irq(&client->dev);
+		ret = pca953x_acpi_get_irq(dev);
 		if (ret > 0)
 			client->irq = ret;
 	}
@@ -957,10 +957,9 @@ static int pca953x_irq_setup(struct pca953x_chip *chip, int irq_base)
 	girq->threaded = true;
 	girq->first = irq_base; /* FIXME: get rid of this */
 
-	ret = devm_request_threaded_irq(&client->dev, client->irq,
-					NULL, pca953x_irq_handler,
-					IRQF_ONESHOT | IRQF_SHARED,
-					dev_name(&client->dev), chip);
+	ret = devm_request_threaded_irq(dev, client->irq, NULL, pca953x_irq_handler,
+					IRQF_ONESHOT | IRQF_SHARED, dev_name(dev),
+					chip);
 	if (ret)
 		return dev_err_probe(dev, client->irq, "failed to request irq\n");
 
@@ -968,13 +967,13 @@ static int pca953x_irq_setup(struct pca953x_chip *chip, int irq_base)
 }
 
 #else /* CONFIG_GPIO_PCA953X_IRQ */
-static int pca953x_irq_setup(struct pca953x_chip *chip,
-			     int irq_base)
+static int pca953x_irq_setup(struct pca953x_chip *chip, int irq_base)
 {
 	struct i2c_client *client = chip->client;
+	struct device *dev = &client->dev;
 
 	if (client->irq && irq_base != -1 && (chip->driver_data & PCA_INT))
-		dev_warn(&client->dev, "interrupt support not compiled in\n");
+		dev_warn(dev, "interrupt support not compiled in\n");
 
 	return 0;
 }
@@ -1065,11 +1064,11 @@ static int pca953x_probe(struct i2c_client *client)
 	int ret;
 	const struct regmap_config *regmap_config;
 
-	chip = devm_kzalloc(&client->dev, sizeof(*chip), GFP_KERNEL);
+	chip = devm_kzalloc(dev, sizeof(*chip), GFP_KERNEL);
 	if (chip == NULL)
 		return -ENOMEM;
 
-	pdata = dev_get_platdata(&client->dev);
+	pdata = dev_get_platdata(dev);
 	if (pdata) {
 		irq_base = pdata->irq_base;
 		chip->gpio_start = pdata->gpio_base;
@@ -1086,8 +1085,7 @@ static int pca953x_probe(struct i2c_client *client)
 		 * using "reset" GPIO. Otherwise any of those platform
 		 * must use _DSD method with corresponding property.
 		 */
-		reset_gpio = devm_gpiod_get_optional(&client->dev, "reset",
-						     GPIOD_OUT_LOW);
+		reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
 		if (IS_ERR(reset_gpio))
 			return PTR_ERR(reset_gpio);
 	}
@@ -1106,10 +1104,10 @@ static int pca953x_probe(struct i2c_client *client)
 	pca953x_setup_gpio(chip, chip->driver_data & PCA_GPIO_MASK);
 
 	if (NBANK(chip) > 2 || PCA_CHIP_TYPE(chip->driver_data) == PCA957X_TYPE) {
-		dev_info(&client->dev, "using AI\n");
+		dev_info(dev, "using AI\n");
 		regmap_config = &pca953x_ai_i2c_regmap;
 	} else {
-		dev_info(&client->dev, "using no AI\n");
+		dev_info(dev, "using no AI\n");
 		regmap_config = &pca953x_i2c_regmap;
 	}
 
-- 
2.40.0.1.gaa8946217a0b

[PATCH v1 07/10] gpio: pca953x: Utilise temporary variable for struct gpio_chip

[Andy Shevchenko]

We have a temporary variable to keep pointer to struct gpio_chip.
Utilise it where it makes sense.

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

diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c
index 4aa15128c91f..fe113c74b7b2 100644
--- a/drivers/gpio/gpio-pca953x.c
+++ b/drivers/gpio/gpio-pca953x.c
@@ -680,9 +680,7 @@ static int pca953x_gpio_set_config(struct gpio_chip *gc, unsigned int offset,
 
 static void pca953x_setup_gpio(struct pca953x_chip *chip, int gpios)
 {
-	struct gpio_chip *gc;
-
-	gc = &chip->gpio_chip;
+	struct gpio_chip *gc = &chip->gpio_chip;
 
 	gc->direction_input  = pca953x_gpio_direction_input;
 	gc->direction_output = pca953x_gpio_direction_output;
@@ -915,6 +913,7 @@ static int pca953x_irq_setup(struct pca953x_chip *chip, int irq_base)
 	struct device *dev = &client->dev;
 	DECLARE_BITMAP(reg_direction, MAX_LINE);
 	DECLARE_BITMAP(irq_stat, MAX_LINE);
+	struct gpio_chip *gc = &chip->gpio_chip;
 	struct gpio_irq_chip *girq;
 	int ret;
 
@@ -943,7 +942,7 @@ static int pca953x_irq_setup(struct pca953x_chip *chip, int irq_base)
 	 * this purpose.
 	 */
 	pca953x_read_regs(chip, chip->regs->direction, reg_direction);
-	bitmap_and(chip->irq_stat, irq_stat, reg_direction, chip->gpio_chip.ngpio);
+	bitmap_and(chip->irq_stat, irq_stat, reg_direction, gc->ngpio);
 	mutex_init(&chip->irq_lock);
 
 	girq = &chip->gpio_chip.irq;
-- 
2.40.0.1.gaa8946217a0b

[PATCH v1 08/10] gpio: pca953x: Switch to DEFINE_SIMPLE_DEV_PM_OPS()

[Andy Shevchenko]

SIMPLE_DEV_PM_OPS() is deprecated, replace it with pm_sleep_ptr()
and DEFINE_SIMPLE_DEV_PM_OPS() for setting the driver's PM routines.
We can now remove the ifdeffery surrounding the suspend and resume
functions.

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

diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c
index fe113c74b7b2..bf27e2d920f7 100644
--- a/drivers/gpio/gpio-pca953x.c
+++ b/drivers/gpio/gpio-pca953x.c
@@ -1164,7 +1164,6 @@ static int pca953x_probe(struct i2c_client *client)
 	return devm_gpiochip_add_data(dev, &chip->gpio_chip, chip);
 }
 
-#ifdef CONFIG_PM_SLEEP
 static int pca953x_regcache_sync(struct pca953x_chip *chip)
 {
 	struct device *dev = &chip->client->dev;
@@ -1270,7 +1269,8 @@ static int pca953x_resume(struct device *dev)
 
 	return 0;
 }
-#endif
+
+static DEFINE_SIMPLE_DEV_PM_OPS(pca953x_pm_ops, pca953x_suspend, pca953x_resume);
 
 /* convenience to stop overlong match-table lines */
 #define OF_653X(__nrgpio, __int) ((void *)(__nrgpio | PCAL653X_TYPE | __int))
@@ -1328,12 +1328,10 @@ static const struct of_device_id pca953x_dt_ids[] = {
 
 MODULE_DEVICE_TABLE(of, pca953x_dt_ids);
 
-static SIMPLE_DEV_PM_OPS(pca953x_pm_ops, pca953x_suspend, pca953x_resume);
-
 static struct i2c_driver pca953x_driver = {
 	.driver = {
 		.name	= "pca953x",
-		.pm	= &pca953x_pm_ops,
+		.pm	= pm_sleep_ptr(&pca953x_pm_ops),
 		.of_match_table = pca953x_dt_ids,
 		.acpi_match_table = pca953x_acpi_ids,
 	},
-- 
2.40.0.1.gaa8946217a0b

[PATCH v1 09/10] gpio: pca953x: Get rid of useless goto label

[Andy Shevchenko]

In a few functions goto label is useless as there are no locking,
no nothing that may justify its usage. Get rid of it.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/gpio/gpio-pca953x.c | 24 +++++++-----------------
 1 file changed, 7 insertions(+), 17 deletions(-)

diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c
index bf27e2d920f7..16f5e3043bf0 100644
--- a/drivers/gpio/gpio-pca953x.c
+++ b/drivers/gpio/gpio-pca953x.c
@@ -988,20 +988,18 @@ static int device_pca95xx_init(struct pca953x_chip *chip)
 	ret = regcache_sync_region(chip->regmap, regaddr,
 				   regaddr + NBANK(chip) - 1);
 	if (ret)
-		goto out;
+		return ret;
 
 	regaddr = chip->recalc_addr(chip, chip->regs->direction, 0);
 	ret = regcache_sync_region(chip->regmap, regaddr,
 				   regaddr + NBANK(chip) - 1);
 	if (ret)
-		goto out;
+		return ret;
 
 	/* clear polarity inversion */
 	bitmap_zero(val, MAX_LINE);
 
-	ret = pca953x_write_regs(chip, chip->regs->invert, val);
-out:
-	return ret;
+	return pca953x_write_regs(chip, chip->regs->invert, val);
 }
 
 static int device_pca957x_init(struct pca953x_chip *chip)
@@ -1012,19 +1010,13 @@ static int device_pca957x_init(struct pca953x_chip *chip)
 
 	ret = device_pca95xx_init(chip);
 	if (ret)
-		goto out;
+		return ret;
 
 	/* To enable register 6, 7 to control pull up and pull down */
 	for (i = 0; i < NBANK(chip); i++)
 		bitmap_set_value8(val, 0x02, i * BANK_SZ);
 
-	ret = pca953x_write_regs(chip, PCA957X_BKEN, val);
-	if (ret)
-		goto out;
-
-	return 0;
-out:
-	return ret;
+	return pca953x_write_regs(chip, PCA957X_BKEN, val);
 }
 
 static void pca953x_disable_regulator(void *reg)
@@ -1262,12 +1254,10 @@ static int pca953x_resume(struct device *dev)
 	}
 
 	ret = pca953x_restore_context(chip);
-	if (ret) {
+	if (ret)
 		dev_err(dev, "Failed to restore register map: %d\n", ret);
-		return ret;
-	}
 
-	return 0;
+	return ret;
 }
 
 static DEFINE_SIMPLE_DEV_PM_OPS(pca953x_pm_ops, pca953x_suspend, pca953x_resume);
-- 
2.40.0.1.gaa8946217a0b

[PATCH v1 10/10] gpio: pca953x: Revisit header inclusions

[Andy Shevchenko]

Some of the headers are not use, some are missing. Fix that.

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

diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c
index 16f5e3043bf0..b0d768ebf21d 100644
--- a/drivers/gpio/gpio-pca953x.c
+++ b/drivers/gpio/gpio-pca953x.c
@@ -8,23 +8,30 @@
  *  Derived from drivers/i2c/chips/pca9539.c
  */
 
-#include <linux/acpi.h>
+#include <linux/atomic.h>
 #include <linux/bitmap.h>
 #include <linux/cleanup.h>
-#include <linux/gpio/consumer.h>
-#include <linux/gpio/driver.h>
+#include <linux/device.h>
+#include <linux/errno.h>
 #include <linux/i2c.h>
 #include <linux/init.h>
 #include <linux/interrupt.h>
+#include <linux/irq.h>
+#include <linux/mod_devicetable.h>
 #include <linux/module.h>
-#include <linux/of_platform.h>
-#include <linux/platform_data/pca953x.h>
+#include <linux/mutex.h>
+#include <linux/pm.h>
 #include <linux/regmap.h>
 #include <linux/regulator/consumer.h>
 #include <linux/seq_file.h>
 #include <linux/slab.h>
 
-#include <asm/unaligned.h>
+#include <linux/gpio/consumer.h>
+#include <linux/gpio/driver.h>
+
+#include <linux/pinctrl/pinconf-generic.h>
+
+#include <linux/platform_data/pca953x.h>
 
 #define PCA953X_INPUT		0x00
 #define PCA953X_OUTPUT		0x01
@@ -119,6 +126,7 @@ MODULE_DEVICE_TABLE(i2c, pca953x_id);
 
 #ifdef CONFIG_GPIO_PCA953X_IRQ
 
+#include <linux/acpi.h>
 #include <linux/dmi.h>
 
 static const struct acpi_gpio_params pca953x_irq_gpios = { 0, 0, true };
-- 
2.40.0.1.gaa8946217a0b

Re: [PATCH v1 09/10] gpio: pca953x: Get rid of useless goto label

[Bartosz Golaszewski]

On Fri, Sep 1, 2023 at 3:40 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> In a few functions goto label is useless as there are no locking,
> no nothing that may justify its usage. Get rid of it.
>

I guess it was supposed to be "so nothing" but I'll fix it when applying.

Bart

> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/gpio/gpio-pca953x.c | 24 +++++++-----------------
>  1 file changed, 7 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c
> index bf27e2d920f7..16f5e3043bf0 100644
> --- a/drivers/gpio/gpio-pca953x.c
> +++ b/drivers/gpio/gpio-pca953x.c
> @@ -988,20 +988,18 @@ static int device_pca95xx_init(struct pca953x_chip *chip)
>         ret = regcache_sync_region(chip->regmap, regaddr,
>                                    regaddr + NBANK(chip) - 1);
>         if (ret)
> -               goto out;
> +               return ret;
>
>         regaddr = chip->recalc_addr(chip, chip->regs->direction, 0);
>         ret = regcache_sync_region(chip->regmap, regaddr,
>                                    regaddr + NBANK(chip) - 1);
>         if (ret)
> -               goto out;
> +               return ret;
>
>         /* clear polarity inversion */
>         bitmap_zero(val, MAX_LINE);
>
> -       ret = pca953x_write_regs(chip, chip->regs->invert, val);
> -out:
> -       return ret;
> +       return pca953x_write_regs(chip, chip->regs->invert, val);
>  }
>
>  static int device_pca957x_init(struct pca953x_chip *chip)
> @@ -1012,19 +1010,13 @@ static int device_pca957x_init(struct pca953x_chip *chip)
>
>         ret = device_pca95xx_init(chip);
>         if (ret)
> -               goto out;
> +               return ret;
>
>         /* To enable register 6, 7 to control pull up and pull down */
>         for (i = 0; i < NBANK(chip); i++)
>                 bitmap_set_value8(val, 0x02, i * BANK_SZ);
>
> -       ret = pca953x_write_regs(chip, PCA957X_BKEN, val);
> -       if (ret)
> -               goto out;
> -
> -       return 0;
> -out:
> -       return ret;
> +       return pca953x_write_regs(chip, PCA957X_BKEN, val);
>  }
>
>  static void pca953x_disable_regulator(void *reg)
> @@ -1262,12 +1254,10 @@ static int pca953x_resume(struct device *dev)
>         }
>
>         ret = pca953x_restore_context(chip);
> -       if (ret) {
> +       if (ret)
>                 dev_err(dev, "Failed to restore register map: %d\n", ret);
> -               return ret;
> -       }
>
> -       return 0;
> +       return ret;
>  }
>
>  static DEFINE_SIMPLE_DEV_PM_OPS(pca953x_pm_ops, pca953x_suspend, pca953x_resume);
> --
> 2.40.0.1.gaa8946217a0b
>

Re: [PATCH v1 01/10] gpio: pca953x: Drop unused fields in struct pca953x_platform_data

[Bartosz Golaszewski]

On Fri, Sep 1, 2023 at 3:40 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> New code should solely use firmware nodes for the specifics and
> not any callbacks.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/gpio/gpio-pca953x.c           | 37 ++++++---------------------
>  include/linux/platform_data/pca953x.h | 13 ----------
>  2 files changed, 8 insertions(+), 42 deletions(-)
>
> diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c
> index bdd50a78e414..02695abd0eb1 100644
> --- a/drivers/gpio/gpio-pca953x.c
> +++ b/drivers/gpio/gpio-pca953x.c
> @@ -211,7 +211,6 @@ struct pca953x_chip {
>
>         struct i2c_client *client;
>         struct gpio_chip gpio_chip;
> -       const char *const *names;
>         unsigned long driver_data;
>         struct regulator *regulator;
>
> @@ -712,7 +711,6 @@ static void pca953x_setup_gpio(struct pca953x_chip *chip, int gpios)
>         gc->label = dev_name(&chip->client->dev);
>         gc->parent = &chip->client->dev;
>         gc->owner = THIS_MODULE;
> -       gc->names = chip->names;
>  }
>
>  #ifdef CONFIG_GPIO_PCA953X_IRQ
> @@ -998,7 +996,7 @@ static int pca953x_irq_setup(struct pca953x_chip *chip,
>  }
>  #endif
>
> -static int device_pca95xx_init(struct pca953x_chip *chip, u32 invert)
> +static int device_pca95xx_init(struct pca953x_chip *chip)
>  {
>         DECLARE_BITMAP(val, MAX_LINE);
>         u8 regaddr;
> @@ -1016,24 +1014,21 @@ static int device_pca95xx_init(struct pca953x_chip *chip, u32 invert)
>         if (ret)
>                 goto out;
>
> -       /* set platform specific polarity inversion */
> -       if (invert)
> -               bitmap_fill(val, MAX_LINE);
> -       else
> -               bitmap_zero(val, MAX_LINE);
> +       /* clear polarity inversion */
> +       bitmap_zero(val, MAX_LINE);
>
>         ret = pca953x_write_regs(chip, chip->regs->invert, val);
>  out:
>         return ret;
>  }
>
> -static int device_pca957x_init(struct pca953x_chip *chip, u32 invert)
> +static int device_pca957x_init(struct pca953x_chip *chip)
>  {
>         DECLARE_BITMAP(val, MAX_LINE);
>         unsigned int i;
>         int ret;
>
> -       ret = device_pca95xx_init(chip, invert);
> +       ret = device_pca95xx_init(chip);
>         if (ret)
>                 goto out;
>
> @@ -1054,9 +1049,8 @@ static int pca953x_probe(struct i2c_client *client)
>  {
>         struct pca953x_platform_data *pdata;
>         struct pca953x_chip *chip;
> -       int irq_base = 0;
> +       int irq_base;
>         int ret;
> -       u32 invert = 0;
>         struct regulator *reg;
>         const struct regmap_config *regmap_config;
>
> @@ -1068,8 +1062,6 @@ static int pca953x_probe(struct i2c_client *client)
>         if (pdata) {
>                 irq_base = pdata->irq_base;
>                 chip->gpio_start = pdata->gpio_base;
> -               invert = pdata->invert;
> -               chip->names = pdata->names;
>         } else {
>                 struct gpio_desc *reset_gpio;
>
> @@ -1158,10 +1150,10 @@ static int pca953x_probe(struct i2c_client *client)
>          */
>         if (PCA_CHIP_TYPE(chip->driver_data) == PCA957X_TYPE) {
>                 chip->regs = &pca957x_regs;
> -               ret = device_pca957x_init(chip, invert);
> +               ret = device_pca957x_init(chip);
>         } else {
>                 chip->regs = &pca953x_regs;
> -               ret = device_pca95xx_init(chip, invert);
> +               ret = device_pca95xx_init(chip);
>         }
>         if (ret)
>                 goto err_exit;
> @@ -1174,13 +1166,6 @@ static int pca953x_probe(struct i2c_client *client)
>         if (ret)
>                 goto err_exit;
>
> -       if (pdata && pdata->setup) {
> -               ret = pdata->setup(client, chip->gpio_chip.base,
> -                                  chip->gpio_chip.ngpio, pdata->context);
> -               if (ret < 0)
> -                       dev_warn(&client->dev, "setup failed, %d\n", ret);
> -       }
> -
>         return 0;
>
>  err_exit:
> @@ -1190,14 +1175,8 @@ static int pca953x_probe(struct i2c_client *client)
>
>  static void pca953x_remove(struct i2c_client *client)
>  {
> -       struct pca953x_platform_data *pdata = dev_get_platdata(&client->dev);
>         struct pca953x_chip *chip = i2c_get_clientdata(client);
>
> -       if (pdata && pdata->teardown) {
> -               pdata->teardown(client, chip->gpio_chip.base,
> -                               chip->gpio_chip.ngpio, pdata->context);
> -       }
> -
>         regulator_disable(chip->regulator);
>  }
>
> diff --git a/include/linux/platform_data/pca953x.h b/include/linux/platform_data/pca953x.h
> index 96c1a14ab365..3c3787c4d96c 100644
> --- a/include/linux/platform_data/pca953x.h
> +++ b/include/linux/platform_data/pca953x.h
> @@ -11,21 +11,8 @@ struct pca953x_platform_data {
>         /* number of the first GPIO */
>         unsigned        gpio_base;
>
> -       /* initial polarity inversion setting */
> -       u32             invert;
> -
>         /* interrupt base */
>         int             irq_base;
> -
> -       void            *context;       /* param to setup/teardown */
> -
> -       int             (*setup)(struct i2c_client *client,
> -                               unsigned gpio, unsigned ngpio,
> -                               void *context);
> -       void            (*teardown)(struct i2c_client *client,
> -                               unsigned gpio, unsigned ngpio,
> -                               void *context);
> -       const char      *const *names;
>  };
>
>  #endif /* _LINUX_PCA953X_H */
> --
> 2.40.0.1.gaa8946217a0b
>

Ah, we're so close to getting rid of platform data entirely...

Series looks good to me, I'll pick it up next week after the merge
window closes.

Bart

Re: [PATCH v1 09/10] gpio: pca953x: Get rid of useless goto label

[Andy Shevchenko]

On Mon, Sep 4, 2023 at 10:41 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> On Fri, Sep 1, 2023 at 3:40 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> >
> > In a few functions goto label is useless as there are no locking,

are --> is (that's I missed)

> > no nothing that may justify its usage. Get rid of it.
>
> I guess it was supposed to be "so nothing" but I'll fix it when applying.

Either will work, while original is incorrect grammatically, the de
facto use of it is common
https://www.urbandictionary.com/define.php?term=no%20nothing


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 01/10] gpio: pca953x: Drop unused fields in struct pca953x_platform_data

[Andy Shevchenko]

On Mon, Sep 04, 2023 at 09:43:01AM +0200, Bartosz Golaszewski wrote:
> On Fri, Sep 1, 2023 at 3:40 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:

...

> >         /* number of the first GPIO */
> >         unsigned        gpio_base;
> >
> > -       /* initial polarity inversion setting */
> > -       u32             invert;
> > -
> >         /* interrupt base */
> >         int             irq_base;
> > -
> > -       void            *context;       /* param to setup/teardown */
> > -
> > -       int             (*setup)(struct i2c_client *client,
> > -                               unsigned gpio, unsigned ngpio,
> > -                               void *context);
> > -       void            (*teardown)(struct i2c_client *client,
> > -                               unsigned gpio, unsigned ngpio,
> > -                               void *context);
> > -       const char      *const *names;
> 
> Ah, we're so close to getting rid of platform data entirely...

Yep!

> Series looks good to me, I'll pick it up next week after the merge
> window closes.

Sure, thank you for the review!

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 01/10] gpio: pca953x: Drop unused fields in struct pca953x_platform_data

[Bartosz Golaszewski]

On Fri, Sep 1, 2023 at 3:40 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> New code should solely use firmware nodes for the specifics and
> not any callbacks.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---

Series applied, thanks!

Bart

Re: [PATCH v1 01/10] gpio: pca953x: Drop unused fields in struct pca953x_platform_data

[Linus Walleij]

On Fri, Sep 1, 2023 at 3:40 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:

> New code should solely use firmware nodes for the specifics and
> not any callbacks.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Patches look good to me:
Acked-by: Linus Walleij <linus.walleij@linaro.org>
for all patches.

Patch 8 looks HTML on my gmail but I guess the problem
is on my side.

Yours,
Linus Walleij

Re: [PATCH v1 01/10] gpio: pca953x: Drop unused fields in struct pca953x_platform_data

[Andy Shevchenko]

On Tue, Sep 12, 2023 at 09:50:06AM +0200, Linus Walleij wrote:
> On Fri, Sep 1, 2023 at 3:40 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> 
> > New code should solely use firmware nodes for the specifics and
> > not any callbacks.
> >
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> 
> Patches look good to me:
> Acked-by: Linus Walleij <linus.walleij@linaro.org>
> for all patches.

Thank you!

> Patch 8 looks HTML on my gmail but I guess the problem
> is on my side.

The entire series has been sent in the same way, it's quite unlikely that
the only patch got mangled while others are okay.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 01/10] gpio: pca953x: Drop unused fields in struct pca953x_platform_data

[Bartosz Golaszewski]

On Tue, Sep 12, 2023 at 11:31 AM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Tue, Sep 12, 2023 at 09:50:06AM +0200, Linus Walleij wrote:
> > On Fri, Sep 1, 2023 at 3:40 PM Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> >
> > > New code should solely use firmware nodes for the specifics and
> > > not any callbacks.
> > >
> > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> >
> > Patches look good to me:
> > Acked-by: Linus Walleij <linus.walleij@linaro.org>
> > for all patches.
>
> Thank you!
>
> > Patch 8 looks HTML on my gmail but I guess the problem
> > is on my side.
>
> The entire series has been sent in the same way, it's quite unlikely that
> the only patch got mangled while others are okay.
>

I'm using cmdg - a command-line interface to gmail - and all patches
look fine. Surprisingly, the web interface indeed does mangle patch 8
for some reason.

Bart

Re: [PATCH v1 05/10] gpio: pca953x: Simplify code with cleanup helpers

[Geert Uytterhoeven]

 	Hi Andy,

On Fri, 1 Sep 2023, Andy Shevchenko wrote:
> Use macros defined in linux/cleanup.h to automate resource lifetime
> control in gpio-pca953x.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Thanks for your patch, which is now commit 8e471b784a720f6f
("gpio: pca953x: Simplify code with cleanup helpers") in
gpio/gpio/for-next.

> --- a/drivers/gpio/gpio-pca953x.c
> +++ b/drivers/gpio/gpio-pca953x.c
> @@ -557,9 +554,8 @@ static int pca953x_gpio_get_value(struct gpio_chip *gc, unsigned off)
> 	u32 reg_val;
> 	int ret;
>
> -	mutex_lock(&chip->i2c_lock);
> -	ret = regmap_read(chip->regmap, inreg, &reg_val);
> -	mutex_unlock(&chip->i2c_lock);
> +	scoped_guard(mutex, &chip->i2c_lock)
> +		ret = regmap_read(chip->regmap, inreg, &reg_val);

I can't say I'm thrilled about the lack of curly braces.  I was also
surprised to discover that checkpatch nor gcc W=1 complain about the
indentation change.
I know we don't use curly braces in single-statement for_each_*() loops,
but at least these have the familiar "for"-prefix.  And having the scope
is very important here, so using braces, this would stand out more.

Hence can we please get curly braces, like

     scoped_guard(mutex, &chip->i2c_lock) {
 	    ret = regmap_read(chip->regmap, inreg, &reg_val);
     }

?

Thanks! ;-)

Gr{oetje,eeting}s,

 						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
 							    -- Linus Torvalds

Re: [PATCH v1 05/10] gpio: pca953x: Simplify code with cleanup helpers

[Bartosz Golaszewski]

On Wed, Sep 13, 2023 at 4:35 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
>         Hi Andy,
>
> On Fri, 1 Sep 2023, Andy Shevchenko wrote:
> > Use macros defined in linux/cleanup.h to automate resource lifetime
> > control in gpio-pca953x.
> >
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>
> Thanks for your patch, which is now commit 8e471b784a720f6f
> ("gpio: pca953x: Simplify code with cleanup helpers") in
> gpio/gpio/for-next.
>
> > --- a/drivers/gpio/gpio-pca953x.c
> > +++ b/drivers/gpio/gpio-pca953x.c
> > @@ -557,9 +554,8 @@ static int pca953x_gpio_get_value(struct gpio_chip *gc, unsigned off)
> >       u32 reg_val;
> >       int ret;
> >
> > -     mutex_lock(&chip->i2c_lock);
> > -     ret = regmap_read(chip->regmap, inreg, &reg_val);
> > -     mutex_unlock(&chip->i2c_lock);
> > +     scoped_guard(mutex, &chip->i2c_lock)
> > +             ret = regmap_read(chip->regmap, inreg, &reg_val);
>
> I can't say I'm thrilled about the lack of curly braces.  I was also
> surprised to discover that checkpatch nor gcc W=1 complain about the
> indentation change.
> I know we don't use curly braces in single-statement for_each_*() loops,
> but at least these have the familiar "for"-prefix.  And having the scope
> is very important here, so using braces, this would stand out more.
>
> Hence can we please get curly braces, like
>
>      scoped_guard(mutex, &chip->i2c_lock) {
>             ret = regmap_read(chip->regmap, inreg, &reg_val);
>      }
>
> ?
>
> Thanks! ;-)

I strongly disagree. The scope here is very clear - just like it is in
a for loop, in a while loop or in an if block:

if (foo)
    bar()

if (foo) {
    bar();
    baz();
}

Only compound statements need curly braces in the kernel and it has
been like this forever. I don't really see a need to make it an
exception.

That being said - I don't think the coding style for guard has ever
been addressed yet, so maybe bring it up with Peter Zijlstra?

Bart

>
> Gr{oetje,eeting}s,
>
>                                                 Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                                             -- Linus Torvalds

guard coding style (was: Re: [PATCH v1 05/10] gpio: pca953x: Simplify code with cleanup helpers)

[Geert Uytterhoeven]

Hi Bartosz,

On Wed, Sep 13, 2023 at 5:27 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> On Wed, Sep 13, 2023 at 4:35 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > On Fri, 1 Sep 2023, Andy Shevchenko wrote:
> > > Use macros defined in linux/cleanup.h to automate resource lifetime
> > > control in gpio-pca953x.
> > >
> > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> >
> > Thanks for your patch, which is now commit 8e471b784a720f6f
> > ("gpio: pca953x: Simplify code with cleanup helpers") in
> > gpio/gpio/for-next.
> >
> > > --- a/drivers/gpio/gpio-pca953x.c
> > > +++ b/drivers/gpio/gpio-pca953x.c
> > > @@ -557,9 +554,8 @@ static int pca953x_gpio_get_value(struct gpio_chip *gc, unsigned off)
> > >       u32 reg_val;
> > >       int ret;
> > >
> > > -     mutex_lock(&chip->i2c_lock);
> > > -     ret = regmap_read(chip->regmap, inreg, &reg_val);
> > > -     mutex_unlock(&chip->i2c_lock);
> > > +     scoped_guard(mutex, &chip->i2c_lock)
> > > +             ret = regmap_read(chip->regmap, inreg, &reg_val);
> >
> > I can't say I'm thrilled about the lack of curly braces.  I was also
> > surprised to discover that checkpatch nor gcc W=1 complain about the
> > indentation change.
> > I know we don't use curly braces in single-statement for_each_*() loops,
> > but at least these have the familiar "for"-prefix.  And having the scope
> > is very important here, so using braces, this would stand out more.
> >
> > Hence can we please get curly braces, like
> >
> >      scoped_guard(mutex, &chip->i2c_lock) {
> >             ret = regmap_read(chip->regmap, inreg, &reg_val);
> >      }
> >
> > ?
> >
> > Thanks! ;-)
>
> I strongly disagree. The scope here is very clear - just like it is in
> a for loop, in a while loop or in an if block:
>
> if (foo)
>     bar()
>
> if (foo) {
>     bar();
>     baz();
> }
>
> Only compound statements need curly braces in the kernel and it has
> been like this forever. I don't really see a need to make it an
> exception.
>
> That being said - I don't think the coding style for guard has ever
> been addressed yet, so maybe bring it up with Peter Zijlstra?

That's a good idea!

I see Peter always used curly braces (but he didn't have any
single-statement blocks, except for one with an "if", and we do tend
to use curly braces in "for"-statements containing a single "if", too),
but he does put a space after the "scoped_guard", as is also
shown in the template in include/linux/cleanup.h:

    scoped_guard (name, args...) { }:

Then, "guard" does not get a space (but it is funny syntax
anyway, with the double set of parentheses ;-).  The template in
include/linux/cleanup.h doesn't match actual usage as it lacks the
second set of parentheses:

    guard(name):

Peter: care to comment?
Or do you have a different bikeshed to paint today? ;-)

Thanks!

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: guard coding style (was: Re: [PATCH v1 05/10] gpio: pca953x: Simplify code with cleanup helpers)

[Mitchell Levy]

Hey all,
A brief disclaimer, I'm a fairly new kernel contributor, but since I
was cc'd directly, I figured I might as well drop into the
conversation.

On Thu, Sep 14, 2023 at 12:47 AM Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
>
> Hi Bartosz,
>
> On Wed, Sep 13, 2023 at 5:27 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> > On Wed, Sep 13, 2023 at 4:35 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > On Fri, 1 Sep 2023, Andy Shevchenko wrote:
> > > > Use macros defined in linux/cleanup.h to automate resource lifetime
> > > > control in gpio-pca953x.
> > > >
> > > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > >
> > > Thanks for your patch, which is now commit 8e471b784a720f6f
> > > ("gpio: pca953x: Simplify code with cleanup helpers") in
> > > gpio/gpio/for-next.
> > >
> > > > --- a/drivers/gpio/gpio-pca953x.c
> > > > +++ b/drivers/gpio/gpio-pca953x.c
> > > > @@ -557,9 +554,8 @@ static int pca953x_gpio_get_value(struct gpio_chip *gc, unsigned off)
> > > >       u32 reg_val;
> > > >       int ret;
> > > >
> > > > -     mutex_lock(&chip->i2c_lock);
> > > > -     ret = regmap_read(chip->regmap, inreg, &reg_val);
> > > > -     mutex_unlock(&chip->i2c_lock);
> > > > +     scoped_guard(mutex, &chip->i2c_lock)
> > > > +             ret = regmap_read(chip->regmap, inreg, &reg_val);
> > >
> > > I can't say I'm thrilled about the lack of curly braces.  I was also
> > > surprised to discover that checkpatch nor gcc W=1 complain about the
> > > indentation change.
> > > I know we don't use curly braces in single-statement for_each_*() loops,
> > > but at least these have the familiar "for"-prefix.  And having the scope
> > > is very important here, so using braces, this would stand out more.
> > >
> > > Hence can we please get curly braces, like
> > >
> > >      scoped_guard(mutex, &chip->i2c_lock) {
> > >             ret = regmap_read(chip->regmap, inreg, &reg_val);
> > >      }
> > >
> > > ?
> > >
> > > Thanks! ;-)
> >
> > I strongly disagree. The scope here is very clear - just like it is in
> > a for loop, in a while loop or in an if block:
> >
> > if (foo)
> >     bar()
> >
> > if (foo) {
> >     bar();
> >     baz();
> > }
> >
> > Only compound statements need curly braces in the kernel and it has
> > been like this forever. I don't really see a need to make it an
> > exception.

The more I think on this issue, the more I go back and forth. If we
only had guard(...), the only way to approximate scoped guard would be
to either just do what the macro does (i.e., a dummy for loop that
only runs once) or use an anonymous scope, e.g.,
{
    guard(...);
    my_one_statement();
}
Since this is how I've previously used std::lock_guard in C++, this
pattern feels very familiar to me, and the scoped_guard feels almost
like syntax sugar for this. As such, I feel like including the braces
is most natural because, as Geert mentioned, it emphasizes the scope
that "should" (in my brain, at least) be there.

Thanks,
Mitchell

> > That being said - I don't think the coding style for guard has ever
> > been addressed yet, so maybe bring it up with Peter Zijlstra?
>
> That's a good idea!
>
> I see Peter always used curly braces (but he didn't have any
> single-statement blocks, except for one with an "if", and we do tend
> to use curly braces in "for"-statements containing a single "if", too),
> but he does put a space after the "scoped_guard", as is also
> shown in the template in include/linux/cleanup.h:
>
>     scoped_guard (name, args...) { }:
>
> Then, "guard" does not get a space (but it is funny syntax
> anyway, with the double set of parentheses ;-).  The template in
> include/linux/cleanup.h doesn't match actual usage as it lacks the
> second set of parentheses:
>
>     guard(name):
>
> Peter: care to comment?
> Or do you have a different bikeshed to paint today? ;-)
>
> Thanks!
>
> Gr{oetje,eeting}s,
>
>                         Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds

Re: guard coding style (was: Re: [PATCH v1 05/10] gpio: pca953x: Simplify code with cleanup helpers)

[Peter Zijlstra]

On Thu, Sep 14, 2023 at 09:47:07AM +0200, Geert Uytterhoeven wrote:

> > Only compound statements need curly braces in the kernel and it has
> > been like this forever. I don't really see a need to make it an
> > exception.

Kernel coding style is a little different from what C demands.
Specifically, kernel style demands { } for anything multi-line.

Specifically:

	for (;;) {
		/* a comment */
		foo();
	}

or

	for (;;) {
		foo(a, very, long,
		    arg, chain);
	}

do both warrant a pile of curlies in kernel style where C does not
demand it.

> > That being said - I don't think the coding style for guard has ever
> > been addressed yet, so maybe bring it up with Peter Zijlstra?
> 
> That's a good idea!
> 
> I see Peter always used curly braces (but he didn't have any
> single-statement blocks, except for one with an "if", and we do tend
> to use curly braces in "for"-statements containing a single "if", too),
> but he does put a space after the "scoped_guard", as is also
> shown in the template in include/linux/cleanup.h:
> 
>     scoped_guard (name, args...) { }:
> 

Right, per scope_guard being a for loop I added the extra space. Our
coding style does;

	if (cond) { }

	while (cond) { }

	for (;;) { }

etc.. so I too did:

	scoped_guard (name, args...) { }

> Then, "guard" does not get a space (but it is funny syntax
> anyway, with the double set of parentheses ;-).  The template in
> include/linux/cleanup.h doesn't match actual usage as it lacks the
> second set of parentheses:
> 
>     guard(name):
> 
> Peter: care to comment?
> Or do you have a different bikeshed to paint today? ;-)

For guard I read the first pair as if it were a C++ template, that is, I
pretend, it is actually written like:

	guard<name>(args..);

Both are 'odd' in numerous ways and inconsistent vs where the 'args...'
go, but alas, we're trying to wrangle this inside the constraints
imposed upon us by C and CPP our dear pre-processor.

Re: guard coding style (was: Re: [PATCH v1 05/10] gpio: pca953x: Simplify code with cleanup helpers)

[Peter Zijlstra]

On Thu, Sep 14, 2023 at 01:51:01PM -0700, Mitchell Levy wrote:

> The more I think on this issue, the more I go back and forth. If we
> only had guard(...), the only way to approximate scoped guard would be
> to either just do what the macro does (i.e., a dummy for loop that
> only runs once) or use an anonymous scope, e.g.,
> {
>     guard(...);
>     my_one_statement();
> }
> Since this is how I've previously used std::lock_guard in C++, this
> pattern feels very familiar to me, and the scoped_guard feels almost
> like syntax sugar for this. As such, I feel like including the braces
> is most natural because, as Geert mentioned, it emphasizes the scope
> that "should" (in my brain, at least) be there.

AFAIC the anonymous scope thing doesn't much happen in kernel coding
style -- although I'm sure it's there, the code-base is simply too vast
to not have it *somewhere*.

Re: guard coding style (was: Re: [PATCH v1 05/10] gpio: pca953x: Simplify code with cleanup helpers)

[Peter Zijlstra]

On Thu, Sep 14, 2023 at 09:47:07AM +0200, Geert Uytterhoeven wrote:

> > > > +     scoped_guard(mutex, &chip->i2c_lock)
> > > > +             ret = regmap_read(chip->regmap, inreg, &reg_val);
> > >
> > > I can't say I'm thrilled about the lack of curly braces.  I was also
> > > surprised to discover that checkpatch nor gcc W=1 complain about the
> > > indentation change.

So in my kernel/events/core.c changes (that I still need to re-post and
aren't yet upstream) I have found a single instance where I've done the
same lack of curlies:

	scoped_guard (rcu)
		pmu = idr_find(&pmu_idr, type);

clearly self-consistency mandates I should not object to this style.

That said, I see the argument for having them, they do more clearly
demarcate the scope.

As with everything, small variations in style are bound to happen from
one maintainer to another... (or one self to itself over time).

Re: guard coding style (was: Re: [PATCH v1 05/10] gpio: pca953x: Simplify code with cleanup helpers)

[Peter Zijlstra]

On Fri, Sep 15, 2023 at 12:26:39AM +0200, Peter Zijlstra wrote:
> On Thu, Sep 14, 2023 at 01:51:01PM -0700, Mitchell Levy wrote:
> 
> > The more I think on this issue, the more I go back and forth. If we
> > only had guard(...), the only way to approximate scoped guard would be
> > to either just do what the macro does (i.e., a dummy for loop that
> > only runs once) or use an anonymous scope, e.g.,
> > {
> >     guard(...);
> >     my_one_statement();
> > }
> > Since this is how I've previously used std::lock_guard in C++, this
> > pattern feels very familiar to me, and the scoped_guard feels almost
> > like syntax sugar for this. As such, I feel like including the braces
> > is most natural because, as Geert mentioned, it emphasizes the scope
> > that "should" (in my brain, at least) be there.
> 
> AFAIC the anonymous scope thing doesn't much happen in kernel coding
> style -- although I'm sure it's there, the code-base is simply too vast
> to not have it *somewhere*.

The kernel typical style would be:

	do {
		...
	} while (0)

to create a 'pointless' scope. Apparently this is also what I've done in
some conversions where a conditional lock was involved.