[PATCH v2 0/7] gpio: 74x164: Refactor and clean up the driver

[PATCH v2 0/7] gpio: 74x164: Refactor and clean up the driver

[Andy Shevchenko]

Seems like I have had a cleanup series for 74x164, but forgot to send it
last year, here it is.

Changelog v2:
- remove ->remove() leftover (Bart)
- collected tags (Geert, Gustavo)

Andy Shevchenko (7):
  gpio: 74x164: Remove unneeded dependency to OF_GPIO
  gpio: 74x164: Simplify code with cleanup helpers
  gpio: 74x164: Annotate buffer with __counted_by()
  gpio: 74x164: Make use of the macros from bits.h
  gpio: 74x164: Fully convert to use managed resources
  gpio: 74x164: Switch to use dev_err_probe()
  gpio: 74x164: Utilise temporary variable for struct device

 drivers/gpio/Kconfig       |  1 -
 drivers/gpio/gpio-74x164.c | 70 +++++++++++++++++++-------------------
 2 files changed, 35 insertions(+), 36 deletions(-)

-- 
2.43.0.rc1.1336.g36b5255a03ac

[PATCH v2 1/7] gpio: 74x164: Remove unneeded dependency to OF_GPIO

[Andy Shevchenko]

Remove unneeded dependency to OF_GPIO which driver does not use.

Fixes: 3c7469514dbe ("gpio: 74x164: Make use of device properties")
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/gpio/Kconfig | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 24c7dabe153b..4dbf2198a858 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -1766,7 +1766,6 @@ menu "SPI GPIO expanders"
 
 config GPIO_74X164
 	tristate "74x164 serial-in/parallel-out 8-bits shift register"
-	depends on OF_GPIO
 	help
 	  Driver for 74x164 compatible serial-in/parallel-out 8-outputs
 	  shift registers. This driver can be used to provide access
-- 
2.43.0.rc1.1336.g36b5255a03ac

[PATCH v2 2/7] gpio: 74x164: Simplify code with cleanup helpers

[Andy Shevchenko]

Use macros defined in linux/cleanup.h to automate resource lifetime
control in the driver.

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

diff --git a/drivers/gpio/gpio-74x164.c b/drivers/gpio/gpio-74x164.c
index fca6cd2eb1dd..70c662bbca7b 100644
--- a/drivers/gpio/gpio-74x164.c
+++ b/drivers/gpio/gpio-74x164.c
@@ -7,6 +7,7 @@
  */
 
 #include <linux/bitops.h>
+#include <linux/cleanup.h>
 #include <linux/gpio/consumer.h>
 #include <linux/gpio/driver.h>
 #include <linux/module.h>
@@ -43,13 +44,10 @@ static int gen_74x164_get_value(struct gpio_chip *gc, unsigned offset)
 	struct gen_74x164_chip *chip = gpiochip_get_data(gc);
 	u8 bank = chip->registers - 1 - offset / 8;
 	u8 pin = offset % 8;
-	int ret;
 
-	mutex_lock(&chip->lock);
-	ret = (chip->buffer[bank] >> pin) & 0x1;
-	mutex_unlock(&chip->lock);
+	guard(mutex)(&chip->lock);
 
-	return ret;
+	return (chip->buffer[bank] >> pin) & 0x1;
 }
 
 static void gen_74x164_set_value(struct gpio_chip *gc,
@@ -59,14 +57,14 @@ static void gen_74x164_set_value(struct gpio_chip *gc,
 	u8 bank = chip->registers - 1 - offset / 8;
 	u8 pin = offset % 8;
 
-	mutex_lock(&chip->lock);
+	guard(mutex)(&chip->lock);
+
 	if (val)
 		chip->buffer[bank] |= (1 << pin);
 	else
 		chip->buffer[bank] &= ~(1 << pin);
 
 	__gen_74x164_write_config(chip);
-	mutex_unlock(&chip->lock);
 }
 
 static void gen_74x164_set_multiple(struct gpio_chip *gc, unsigned long *mask,
@@ -78,7 +76,8 @@ static void gen_74x164_set_multiple(struct gpio_chip *gc, unsigned long *mask,
 	size_t bank;
 	unsigned long bitmask;
 
-	mutex_lock(&chip->lock);
+	guard(mutex)(&chip->lock);
+
 	for_each_set_clump8(offset, bankmask, mask, chip->registers * 8) {
 		bank = chip->registers - 1 - offset / 8;
 		bitmask = bitmap_get_value8(bits, offset) & bankmask;
@@ -87,7 +86,6 @@ static void gen_74x164_set_multiple(struct gpio_chip *gc, unsigned long *mask,
 		chip->buffer[bank] |= bitmask;
 	}
 	__gen_74x164_write_config(chip);
-	mutex_unlock(&chip->lock);
 }
 
 static int gen_74x164_direction_output(struct gpio_chip *gc,
-- 
2.43.0.rc1.1336.g36b5255a03ac

[PATCH v2 3/7] gpio: 74x164: Annotate buffer with __counted_by()

[Andy Shevchenko]

Add the __counted_by() compiler attribute to the flexible array member
volumes to improve access bounds-checking via CONFIG_UBSAN_BOUNDS and
CONFIG_FORTIFY_SOURCE.

Use struct_size() instead of manually calculating the number of bytes to
allocate the private structure with a buffer.

Reviewed-by: Gustavo A. R. Silva <gustavoars@kernel.org>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/gpio/gpio-74x164.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/gpio/gpio-74x164.c b/drivers/gpio/gpio-74x164.c
index 70c662bbca7b..7844f8a58834 100644
--- a/drivers/gpio/gpio-74x164.c
+++ b/drivers/gpio/gpio-74x164.c
@@ -30,7 +30,7 @@ struct gen_74x164_chip {
 	 * register at the end of the transfer. So, to have a logical
 	 * numbering, store the bytes in reverse order.
 	 */
-	u8			buffer[];
+	u8			buffer[] __counted_by(registers);
 };
 
 static int __gen_74x164_write_config(struct gen_74x164_chip *chip)
@@ -97,6 +97,7 @@ static int gen_74x164_direction_output(struct gpio_chip *gc,
 
 static int gen_74x164_probe(struct spi_device *spi)
 {
+	struct device *dev = &spi->dev;
 	struct gen_74x164_chip *chip;
 	u32 nregs;
 	int ret;
@@ -116,10 +117,12 @@ static int gen_74x164_probe(struct spi_device *spi)
 		return -EINVAL;
 	}
 
-	chip = devm_kzalloc(&spi->dev, sizeof(*chip) + nregs, GFP_KERNEL);
+	chip = devm_kzalloc(dev, struct_size(chip, buffer, nregs), GFP_KERNEL);
 	if (!chip)
 		return -ENOMEM;
 
+	chip->registers = nregs;
+
 	chip->gpiod_oe = devm_gpiod_get_optional(&spi->dev, "enable",
 						 GPIOD_OUT_LOW);
 	if (IS_ERR(chip->gpiod_oe))
@@ -133,10 +136,7 @@ static int gen_74x164_probe(struct spi_device *spi)
 	chip->gpio_chip.set = gen_74x164_set_value;
 	chip->gpio_chip.set_multiple = gen_74x164_set_multiple;
 	chip->gpio_chip.base = -1;
-
-	chip->registers = nregs;
 	chip->gpio_chip.ngpio = GEN_74X164_NUMBER_GPIOS * chip->registers;
-
 	chip->gpio_chip.can_sleep = true;
 	chip->gpio_chip.parent = &spi->dev;
 	chip->gpio_chip.owner = THIS_MODULE;
-- 
2.43.0.rc1.1336.g36b5255a03ac

[PATCH v2 4/7] gpio: 74x164: Make use of the macros from bits.h

[Andy Shevchenko]

Make use of BIT() and GENMASK() where it makes sense.

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

diff --git a/drivers/gpio/gpio-74x164.c b/drivers/gpio/gpio-74x164.c
index 7844f8a58834..0f720d539fa7 100644
--- a/drivers/gpio/gpio-74x164.c
+++ b/drivers/gpio/gpio-74x164.c
@@ -47,7 +47,7 @@ static int gen_74x164_get_value(struct gpio_chip *gc, unsigned offset)
 
 	guard(mutex)(&chip->lock);
 
-	return (chip->buffer[bank] >> pin) & 0x1;
+	return !!(chip->buffer[bank] & BIT(pin));
 }
 
 static void gen_74x164_set_value(struct gpio_chip *gc,
@@ -60,9 +60,9 @@ static void gen_74x164_set_value(struct gpio_chip *gc,
 	guard(mutex)(&chip->lock);
 
 	if (val)
-		chip->buffer[bank] |= (1 << pin);
+		chip->buffer[bank] |= BIT(pin);
 	else
-		chip->buffer[bank] &= ~(1 << pin);
+		chip->buffer[bank] &= ~BIT(pin);
 
 	__gen_74x164_write_config(chip);
 }
-- 
2.43.0.rc1.1336.g36b5255a03ac

[PATCH v2 5/7] gpio: 74x164: Fully convert to use managed resources

[Andy Shevchenko]

Convert the driver probe stage to use managed resources.

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

diff --git a/drivers/gpio/gpio-74x164.c b/drivers/gpio/gpio-74x164.c
index 0f720d539fa7..ecd691de8539 100644
--- a/drivers/gpio/gpio-74x164.c
+++ b/drivers/gpio/gpio-74x164.c
@@ -95,6 +95,19 @@ static int gen_74x164_direction_output(struct gpio_chip *gc,
 	return 0;
 }
 
+static void gen_74x164_deactivate(void *data)
+{
+	struct gen_74x164_chip *chip = data;
+
+	gpiod_set_value_cansleep(chip->gpiod_oe, 0);
+}
+
+static int gen_74x164_activate(struct device *dev, struct gen_74x164_chip *chip)
+{
+	gpiod_set_value_cansleep(chip->gpiod_oe, 1);
+	return devm_add_action_or_reset(dev, gen_74x164_deactivate, chip);
+}
+
 static int gen_74x164_probe(struct spi_device *spi)
 {
 	struct device *dev = &spi->dev;
@@ -128,8 +141,6 @@ static int gen_74x164_probe(struct spi_device *spi)
 	if (IS_ERR(chip->gpiod_oe))
 		return PTR_ERR(chip->gpiod_oe);
 
-	spi_set_drvdata(spi, chip);
-
 	chip->gpio_chip.label = spi->modalias;
 	chip->gpio_chip.direction_output = gen_74x164_direction_output;
 	chip->gpio_chip.get = gen_74x164_get_value;
@@ -149,18 +160,13 @@ static int gen_74x164_probe(struct spi_device *spi)
 	if (ret)
 		return dev_err_probe(&spi->dev, ret, "Config write failed\n");
 
-	gpiod_set_value_cansleep(chip->gpiod_oe, 1);
+	ret = gen_74x164_activate(dev, chip);
+	if (ret)
+		return ret;
 
 	return devm_gpiochip_add_data(&spi->dev, &chip->gpio_chip, chip);
 }
 
-static void gen_74x164_remove(struct spi_device *spi)
-{
-	struct gen_74x164_chip *chip = spi_get_drvdata(spi);
-
-	gpiod_set_value_cansleep(chip->gpiod_oe, 0);
-}
-
 static const struct spi_device_id gen_74x164_spi_ids[] = {
 	{ .name = "74hc595" },
 	{ .name = "74lvc594" },
@@ -181,7 +187,6 @@ static struct spi_driver gen_74x164_driver = {
 		.of_match_table	= gen_74x164_dt_ids,
 	},
 	.probe		= gen_74x164_probe,
-	.remove		= gen_74x164_remove,
 	.id_table	= gen_74x164_spi_ids,
 };
 module_spi_driver(gen_74x164_driver);
-- 
2.43.0.rc1.1336.g36b5255a03ac

[PATCH v2 6/7] gpio: 74x164: Switch to use dev_err_probe()

[Andy Shevchenko]

Switch to use dev_err_probe() to simplify the error path and
unify a message template.

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

diff --git a/drivers/gpio/gpio-74x164.c b/drivers/gpio/gpio-74x164.c
index ecd691de8539..bbeef03e3797 100644
--- a/drivers/gpio/gpio-74x164.c
+++ b/drivers/gpio/gpio-74x164.c
@@ -125,10 +125,8 @@ static int gen_74x164_probe(struct spi_device *spi)
 		return ret;
 
 	ret = device_property_read_u32(&spi->dev, "registers-number", &nregs);
-	if (ret) {
-		dev_err(&spi->dev, "Missing 'registers-number' property.\n");
-		return -EINVAL;
-	}
+	if (ret)
+		return dev_err_probe(dev, ret, "Missing 'registers-number' property.\n");
 
 	chip = devm_kzalloc(dev, struct_size(chip, buffer, nregs), GFP_KERNEL);
 	if (!chip)
-- 
2.43.0.rc1.1336.g36b5255a03ac

[PATCH v2 7/7] gpio: 74x164: Utilise temporary variable for struct device

[Andy Shevchenko]

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

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

diff --git a/drivers/gpio/gpio-74x164.c b/drivers/gpio/gpio-74x164.c
index bbeef03e3797..db41abcd73e1 100644
--- a/drivers/gpio/gpio-74x164.c
+++ b/drivers/gpio/gpio-74x164.c
@@ -124,7 +124,7 @@ static int gen_74x164_probe(struct spi_device *spi)
 	if (ret < 0)
 		return ret;
 
-	ret = device_property_read_u32(&spi->dev, "registers-number", &nregs);
+	ret = device_property_read_u32(dev, "registers-number", &nregs);
 	if (ret)
 		return dev_err_probe(dev, ret, "Missing 'registers-number' property.\n");
 
@@ -134,8 +134,7 @@ static int gen_74x164_probe(struct spi_device *spi)
 
 	chip->registers = nregs;
 
-	chip->gpiod_oe = devm_gpiod_get_optional(&spi->dev, "enable",
-						 GPIOD_OUT_LOW);
+	chip->gpiod_oe = devm_gpiod_get_optional(dev, "enable", GPIOD_OUT_LOW);
 	if (IS_ERR(chip->gpiod_oe))
 		return PTR_ERR(chip->gpiod_oe);
 
@@ -147,7 +146,7 @@ static int gen_74x164_probe(struct spi_device *spi)
 	chip->gpio_chip.base = -1;
 	chip->gpio_chip.ngpio = GEN_74X164_NUMBER_GPIOS * chip->registers;
 	chip->gpio_chip.can_sleep = true;
-	chip->gpio_chip.parent = &spi->dev;
+	chip->gpio_chip.parent = dev;
 	chip->gpio_chip.owner = THIS_MODULE;
 
 	ret = devm_mutex_init(&spi->dev, &chip->lock);
-- 
2.43.0.rc1.1336.g36b5255a03ac

Re: [PATCH v2 7/7] gpio: 74x164: Utilise temporary variable for struct device

[Andy Shevchenko]

On Fri, Feb 07, 2025 at 05:17:14PM +0200, Andy Shevchenko wrote:
> We have a temporary variable to keep a pointer to struct device.
> Utilise it where it makes sense.

Urgh, this seems incomplete...
There are more lines to convert, however they do not affect ± statistics.

Tell me if I need to send full v3 or just this patch.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 7/7] gpio: 74x164: Utilise temporary variable for struct device

[Bartosz Golaszewski]

On Fri, Feb 7, 2025 at 4:42 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Fri, Feb 07, 2025 at 05:17:14PM +0200, Andy Shevchenko wrote:
> > We have a temporary variable to keep a pointer to struct device.
> > Utilise it where it makes sense.
>
> Urgh, this seems incomplete...
> There are more lines to convert, however they do not affect ą statistics.
>
> Tell me if I need to send full v3 or just this patch.
>
> --
> With Best Regards,
> Andy Shevchenko
>
>

No worries, I may fix it when applying.

I'll still give this series a few more days on the list.

Bart

Re: [PATCH v2 7/7] gpio: 74x164: Utilise temporary variable for struct device

[Andy Shevchenko]

On Fri, Feb 07, 2025 at 08:56:45PM +0100, Bartosz Golaszewski wrote:
> On Fri, Feb 7, 2025 at 4:42 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> >
> > On Fri, Feb 07, 2025 at 05:17:14PM +0200, Andy Shevchenko wrote:
> > > We have a temporary variable to keep a pointer to struct device.
> > > Utilise it where it makes sense.
> >
> > Urgh, this seems incomplete...
> > There are more lines to convert, however they do not affect ą statistics.
> >
> > Tell me if I need to send full v3 or just this patch.
> 
> No worries, I may fix it when applying.
> 
> I'll still give this series a few more days on the list.

Sure, thanks!


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 0/7] gpio: 74x164: Refactor and clean up the driver

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>


On Fri, 07 Feb 2025 17:17:07 +0200, Andy Shevchenko wrote:
> Seems like I have had a cleanup series for 74x164, but forgot to send it
> last year, here it is.
> 
> Changelog v2:
> - remove ->remove() leftover (Bart)
> - collected tags (Geert, Gustavo)
> 
> [...]

Applied, thanks!

[1/7] gpio: 74x164: Remove unneeded dependency to OF_GPIO
      commit: cd323c6e62dd98035c141b6f751e5b2b7d490b2e
[2/7] gpio: 74x164: Simplify code with cleanup helpers
      commit: bdd603acf6a2b5056dc174e52bc8b285da529dc4
[3/7] gpio: 74x164: Annotate buffer with __counted_by()
      commit: d746cc6e64027e331769f871a595a3dd2c6b30ff
[4/7] gpio: 74x164: Make use of the macros from bits.h
      commit: e742e6b02d858ff9f6a7b43d0b1b5aae9c7e5cf5
[5/7] gpio: 74x164: Fully convert to use managed resources
      commit: abe3817fa1dcae480ec7b71ec1608454cb65d0b8
[6/7] gpio: 74x164: Switch to use dev_err_probe()
      commit: 9bd2dbe4066b3821b68f3e18ba91a3a1cd6354df
[7/7] gpio: 74x164: Utilise temporary variable for struct device
      commit: 5892cfc7db9814a71c991da7024fa03384e48924

Best regards,
-- 
Bartosz Golaszewski <bartosz.golaszewski@linaro.org>