[PATCH v5] GPIO: add support for 74x164 serial-in/parallel-out 8-bit shift register

[PATCH v5] GPIO: add support for 74x164 serial-in/parallel-out 8-bit shift register

[Florian Fainelli]

From: Miguel Gaio <miguel.gaio@efixo.com>

This patch adds support for generic 74x164 serial-in/parallel-out 8-bits
shift register. This driver can be used as a GPIO output expander.

Signed-off-by: Miguel Gaio <miguel.gaio@efixo.com>
Signed-off-by: Juhos Gabor <juhosg@openwrt.org>
Signed-off-by: Florian Fainelli <florian@openwrt.org>
---
Changes since v1:
- renamed nxp_ to gen_ since this driver is generic to all 74HC164 chips
- added comment on this driver not handling the 74HC164 daisy-chaining
- renamed misused GPIO expanders to Shift registers

Changes since v2:
- rename 74hc164 to 74x164

Changes since v3:
- support daisy-chaining of 74x164 up to 32 gpios

Changes since v4:
- converted to a SPI device driver

diff --git a/drivers/gpio/74x164.c b/drivers/gpio/74x164.c
new file mode 100644
index 0000000..ebb183f
--- /dev/null
+++ b/drivers/gpio/74x164.c
@@ -0,0 +1,184 @@
+/*
+ *  74Hx164 - Generic serial-in/parallel-out 8-bits shift register GPIO driver
+ *
+ *  Copyright (C) 2010 Gabor Juhos <juhosg@openwrt.org>
+ *  Copyright (C) 2010 Miguel Gaio <miguel.gaio@efixo.com>
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License version 2 as
+ *  published by the Free Software Foundation.
+ */
+
+#include <linux/init.h>
+#include <linux/mutex.h>
+#include <linux/spi/spi.h>
+#include <linux/spi/74x164.h>
+#include <linux/gpio.h>
+#include <linux/slab.h>
+
+#define GEN_74X164_GPIO_COUNT	8
+
+
+struct gen_74x164_chip {
+	struct spi_device	*spi;
+	struct gpio_chip	gpio_chip;
+	struct mutex		lock;
+	u8			port_config;
+};
+
+static void gen_74x164_set_value(struct gpio_chip *, unsigned, int);
+
+static struct gen_74x164_chip *gpio_to_chip(struct gpio_chip *gc)
+{
+	return container_of(gc, struct gen_74x164_chip, gpio_chip);
+}
+
+static int __gen_74x164_write_config(struct gen_74x164_chip *chip)
+{
+	return spi_write(chip->spi,
+			 &chip->port_config, sizeof(chip->port_config));
+}
+
+static int gen_74x164_direction_output(struct gpio_chip *gc,
+		unsigned offset, int val)
+{
+	gen_74x164_set_value(gc, offset, val);
+	return 0;
+}
+
+static int gen_74x164_get_value(struct gpio_chip *gc, unsigned offset)
+{
+	struct gen_74x164_chip *chip = gpio_to_chip(gc);
+	int ret;
+
+	mutex_lock(&chip->lock);
+	ret = (chip->port_config >> offset) & 0x1;
+	mutex_unlock(&chip->lock);
+
+	return ret;
+}
+
+static void gen_74x164_set_value(struct gpio_chip *gc,
+		unsigned offset, int val)
+{
+	struct gen_74x164_chip *chip = gpio_to_chip(gc);
+	bool refresh;
+
+	mutex_lock(&chip->lock);
+	if (val)
+		chip->port_config |= (1 << offset);
+	else
+		chip->port_config &= ~(1 << offset);
+
+	__gen_74x164_write_config(chip);
+	mutex_unlock(&chip->lock);
+}
+
+static int __devinit gen_74x164_probe(struct spi_device *spi)
+{
+	struct gen_74x164_chip *chip;
+	struct gen_74x164_chip_platform_data *pdata;
+	int ret;
+
+	pdata = spi->dev.platform_data;
+	if (!pdata || !pdata->base) {
+		dev_dbg(&spi->dev, "incorrect or missing platform data\n");
+		return -EINVAL;
+	}
+
+	/*
+	 * bits_per_word cannot be configured in platform data
+	 */
+	spi->bits_per_word = 8;
+
+	ret = spi_setup(spi);
+	if (ret < 0)
+		return ret;
+
+	chip = kzalloc(sizeof(*chip), GFP_KERNEL);
+	if (!chip)
+		return -ENOMEM;
+
+	mutex_init(&chip->lock);
+
+	dev_set_drvdata(&spi->dev, chip);
+
+	chip->spi = spi;
+
+	chip->gpio_chip.label = GEN_74X164_DRIVER_NAME,
+		chip->gpio_chip.direction_output = gen_74x164_direction_output;
+	chip->gpio_chip.get = gen_74x164_get_value;
+	chip->gpio_chip.set = gen_74x164_set_value;
+	chip->gpio_chip.base = pdata->base;
+	chip->gpio_chip.ngpio = GEN_74X164_GPIO_COUNT;
+	chip->gpio_chip.can_sleep = 1;
+	chip->gpio_chip.dev = &spi->dev;
+	chip->gpio_chip.owner = THIS_MODULE;
+
+	ret = __gen_74x164_write_config(chip);
+	if (ret) {
+		dev_err(&spi->dev, "Failed writing: %d\n", ret);
+		goto exit_destroy;
+	}
+
+	ret = gpiochip_add(&chip->gpio_chip);
+	if (ret)
+		goto exit_destroy;
+
+	return ret;
+
+exit_destroy:
+	dev_set_drvdata(&spi->dev, NULL);
+	mutex_destroy(&chip->lock);
+	kfree(chip);
+	return ret;
+}
+
+static int gen_74x164_remove(struct spi_device *spi)
+{
+	struct gen_74x164_chip *chip;
+	int ret;
+
+	chip = dev_get_drvdata(&spi->dev);
+	if (chip == NULL)
+		return -ENODEV;
+
+	dev_set_drvdata(&spi->dev, NULL);
+
+	ret = gpiochip_remove(&chip->gpio_chip);
+	if (!ret) {
+		mutex_destroy(&chip->lock);
+		kfree(chip);
+	} else
+		dev_err(&spi->dev, "Failed to remove the GPIO controller: %d\n",
+				ret);
+
+	return ret;
+}
+
+static struct spi_driver gen_74x164_driver = {
+	.driver = {
+		.name		= GEN_74X164_DRIVER_NAME,
+		.owner		= THIS_MODULE,
+	},
+	.probe		= gen_74x164_probe,
+	.remove		= __devexit_p(gen_74x164_remove),
+};
+
+static int __init gen_74x164_init(void)
+{
+	return spi_register_driver(&gen_74x164_driver);
+}
+subsys_initcall(gen_74x164_init);
+
+static void __exit gen_74x164_exit(void)
+{
+	spi_unregister_driver(&gen_74x164_driver);
+}
+module_exit(gen_74x164_exit);
+
+MODULE_AUTHOR("Gabor Juhos <juhosg@openwrt.org>");
+MODULE_AUTHOR("Miguel Gaio <miguel.gaio@efixo.com>");
+MODULE_DESCRIPTION("GPIO expander driver for 74X164 8-bits shift register");
+MODULE_LICENSE("GPL v2");
+
diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 510aa20..614965c 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -339,6 +339,14 @@ config GPIO_MC33880
 	  SPI driver for Freescale MC33880 high-side/low-side switch.
 	  This provides GPIO interface supporting inputs and outputs.
 
+config GPIO_74X164
+	tristate "74x164 serial-in/parallel-out 8-bits shift register"
+	depends on SPI_MASTER
+	help
+	  Platform driver for 74x164 compatible serial-in/parallel-out
+	  8-outputs shift registers. This driver can be used to provide access
+	  to more gpio outputs.
+
 comment "AC97 GPIO expanders:"
 
 config GPIO_UCB1400
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index fc6019d..60456a4 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -17,6 +17,7 @@ obj-$(CONFIG_GPIO_MAX7301)	+= max7301.o
 obj-$(CONFIG_GPIO_MAX732X)	+= max732x.o
 obj-$(CONFIG_GPIO_MC33880)	+= mc33880.o
 obj-$(CONFIG_GPIO_MCP23S08)	+= mcp23s08.o
+obj-$(CONFIG_GPIO_74X164)	+= 74x164.o
 obj-$(CONFIG_GPIO_PCA953X)	+= pca953x.o
 obj-$(CONFIG_GPIO_PCF857X)	+= pcf857x.o
 obj-$(CONFIG_GPIO_PL061)	+= pl061.o
diff --git a/include/linux/spi/74x164.h b/include/linux/spi/74x164.h
new file mode 100644
index 0000000..d85c52f
--- /dev/null
+++ b/include/linux/spi/74x164.h
@@ -0,0 +1,11 @@
+#ifndef LINUX_SPI_74X164_H
+#define LINUX_SPI_74X164_H
+
+#define GEN_74X164_DRIVER_NAME "74x164"
+
+struct gen_74x164_chip_platform_data {
+	/* number assigned to the first GPIO */
+	unsigned	base;
+};
+
+#endif

Re: [PATCH v5] GPIO: add support for 74x164 serial-in/parallel-out 8-bit shift register

[Miguel Ojeda]

On Mon, Oct 18, 2010 at 9:35 PM, Florian Fainelli <florian@openwrt.org> wrote:
> From: Miguel Gaio <miguel.gaio@efixo.com>
>
> This patch adds support for generic 74x164 serial-in/parallel-out 8-bits
> shift register. This driver can be used as a GPIO output expander.
>
> Signed-off-by: Miguel Gaio <miguel.gaio@efixo.com>
> Signed-off-by: Juhos Gabor <juhosg@openwrt.org>
> Signed-off-by: Florian Fainelli <florian@openwrt.org>
> ---
> Changes since v1:
> - renamed nxp_ to gen_ since this driver is generic to all 74HC164 chips
> - added comment on this driver not handling the 74HC164 daisy-chaining
> - renamed misused GPIO expanders to Shift registers
>
> Changes since v2:
> - rename 74hc164 to 74x164
>
> Changes since v3:
> - support daisy-chaining of 74x164 up to 32 gpios
>
> Changes since v4:
> - converted to a SPI device driver
>
> diff --git a/drivers/gpio/74x164.c b/drivers/gpio/74x164.c
> new file mode 100644
> index 0000000..ebb183f
> --- /dev/null
> +++ b/drivers/gpio/74x164.c
> @@ -0,0 +1,184 @@
> +/*
> + *  74Hx164 - Generic serial-in/parallel-out 8-bits shift register GPIO driver
> + *
> + *  Copyright (C) 2010 Gabor Juhos <juhosg@openwrt.org>
> + *  Copyright (C) 2010 Miguel Gaio <miguel.gaio@efixo.com>
> + *
> + *  This program is free software; you can redistribute it and/or modify
> + *  it under the terms of the GNU General Public License version 2 as
> + *  published by the Free Software Foundation.
> + */
> +
> +#include <linux/init.h>
> +#include <linux/mutex.h>
> +#include <linux/spi/spi.h>
> +#include <linux/spi/74x164.h>
> +#include <linux/gpio.h>
> +#include <linux/slab.h>
> +
> +#define GEN_74X164_GPIO_COUNT  8
> +
> +
> +struct gen_74x164_chip {
> +       struct spi_device       *spi;
> +       struct gpio_chip        gpio_chip;
> +       struct mutex            lock;
> +       u8                      port_config;
> +};
> +
> +static void gen_74x164_set_value(struct gpio_chip *, unsigned, int);
> +
> +static struct gen_74x164_chip *gpio_to_chip(struct gpio_chip *gc)
> +{
> +       return container_of(gc, struct gen_74x164_chip, gpio_chip);
> +}
> +
> +static int __gen_74x164_write_config(struct gen_74x164_chip *chip)
> +{
> +       return spi_write(chip->spi,
> +                        &chip->port_config, sizeof(chip->port_config));
> +}
> +
> +static int gen_74x164_direction_output(struct gpio_chip *gc,
> +               unsigned offset, int val)
> +{
> +       gen_74x164_set_value(gc, offset, val);
> +       return 0;
> +}
> +
> +static int gen_74x164_get_value(struct gpio_chip *gc, unsigned offset)
> +{
> +       struct gen_74x164_chip *chip = gpio_to_chip(gc);
> +       int ret;
> +
> +       mutex_lock(&chip->lock);
> +       ret = (chip->port_config >> offset) & 0x1;
> +       mutex_unlock(&chip->lock);
> +
> +       return ret;
> +}
> +
> +static void gen_74x164_set_value(struct gpio_chip *gc,
> +               unsigned offset, int val)
> +{
> +       struct gen_74x164_chip *chip = gpio_to_chip(gc);
> +       bool refresh;
> +
> +       mutex_lock(&chip->lock);
> +       if (val)
> +               chip->port_config |= (1 << offset);
> +       else
> +               chip->port_config &= ~(1 << offset);

set_bit(), clear_bit() ?

> +
> +       __gen_74x164_write_config(chip);
> +       mutex_unlock(&chip->lock);
> +}
> +
> +static int __devinit gen_74x164_probe(struct spi_device *spi)
> +{
> +       struct gen_74x164_chip *chip;
> +       struct gen_74x164_chip_platform_data *pdata;
> +       int ret;
> +
> +       pdata = spi->dev.platform_data;
> +       if (!pdata || !pdata->base) {
> +               dev_dbg(&spi->dev, "incorrect or missing platform data\n");
> +               return -EINVAL;
> +       }
> +
> +       /*
> +        * bits_per_word cannot be configured in platform data
> +        */
> +       spi->bits_per_word = 8;
> +
> +       ret = spi_setup(spi);
> +       if (ret < 0)
> +               return ret;
> +
> +       chip = kzalloc(sizeof(*chip), GFP_KERNEL);
> +       if (!chip)
> +               return -ENOMEM;
> +
> +       mutex_init(&chip->lock);
> +
> +       dev_set_drvdata(&spi->dev, chip);
> +
> +       chip->spi = spi;
> +
> +       chip->gpio_chip.label = GEN_74X164_DRIVER_NAME,
> +               chip->gpio_chip.direction_output = gen_74x164_direction_output;
> +       chip->gpio_chip.get = gen_74x164_get_value;
> +       chip->gpio_chip.set = gen_74x164_set_value;
> +       chip->gpio_chip.base = pdata->base;
> +       chip->gpio_chip.ngpio = GEN_74X164_GPIO_COUNT;
> +       chip->gpio_chip.can_sleep = 1;
> +       chip->gpio_chip.dev = &spi->dev;
> +       chip->gpio_chip.owner = THIS_MODULE;
> +
> +       ret = __gen_74x164_write_config(chip);
> +       if (ret) {
> +               dev_err(&spi->dev, "Failed writing: %d\n", ret);
> +               goto exit_destroy;
> +       }
> +
> +       ret = gpiochip_add(&chip->gpio_chip);
> +       if (ret)
> +               goto exit_destroy;
> +
> +       return ret;
> +
> +exit_destroy:
> +       dev_set_drvdata(&spi->dev, NULL);
> +       mutex_destroy(&chip->lock);
> +       kfree(chip);
> +       return ret;
> +}
> +
> +static int gen_74x164_remove(struct spi_device *spi)
> +{
> +       struct gen_74x164_chip *chip;
> +       int ret;
> +
> +       chip = dev_get_drvdata(&spi->dev);
> +       if (chip == NULL)
> +               return -ENODEV;
> +
> +       dev_set_drvdata(&spi->dev, NULL);
> +
> +       ret = gpiochip_remove(&chip->gpio_chip);
> +       if (!ret) {
> +               mutex_destroy(&chip->lock);
> +               kfree(chip);
> +       } else
> +               dev_err(&spi->dev, "Failed to remove the GPIO controller: %d\n",
> +                               ret);
> +
> +       return ret;
> +}
> +
> +static struct spi_driver gen_74x164_driver = {
> +       .driver = {
> +               .name           = GEN_74X164_DRIVER_NAME,
> +               .owner          = THIS_MODULE,
> +       },
> +       .probe          = gen_74x164_probe,
> +       .remove         = __devexit_p(gen_74x164_remove),
> +};
> +
> +static int __init gen_74x164_init(void)
> +{
> +       return spi_register_driver(&gen_74x164_driver);
> +}
> +subsys_initcall(gen_74x164_init);
> +
> +static void __exit gen_74x164_exit(void)
> +{
> +       spi_unregister_driver(&gen_74x164_driver);
> +}
> +module_exit(gen_74x164_exit);
> +
> +MODULE_AUTHOR("Gabor Juhos <juhosg@openwrt.org>");
> +MODULE_AUTHOR("Miguel Gaio <miguel.gaio@efixo.com>");
> +MODULE_DESCRIPTION("GPIO expander driver for 74X164 8-bits shift register");
> +MODULE_LICENSE("GPL v2");
> +
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index 510aa20..614965c 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -339,6 +339,14 @@ config GPIO_MC33880
>          SPI driver for Freescale MC33880 high-side/low-side switch.
>          This provides GPIO interface supporting inputs and outputs.
>
> +config GPIO_74X164
> +       tristate "74x164 serial-in/parallel-out 8-bits shift register"
> +       depends on SPI_MASTER
> +       help
> +         Platform driver for 74x164 compatible serial-in/parallel-out
> +         8-outputs shift registers. This driver can be used to provide access
> +         to more gpio outputs.
> +
>  comment "AC97 GPIO expanders:"
>
>  config GPIO_UCB1400
> diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
> index fc6019d..60456a4 100644
> --- a/drivers/gpio/Makefile
> +++ b/drivers/gpio/Makefile
> @@ -17,6 +17,7 @@ obj-$(CONFIG_GPIO_MAX7301)    += max7301.o
>  obj-$(CONFIG_GPIO_MAX732X)     += max732x.o
>  obj-$(CONFIG_GPIO_MC33880)     += mc33880.o
>  obj-$(CONFIG_GPIO_MCP23S08)    += mcp23s08.o
> +obj-$(CONFIG_GPIO_74X164)      += 74x164.o
>  obj-$(CONFIG_GPIO_PCA953X)     += pca953x.o
>  obj-$(CONFIG_GPIO_PCF857X)     += pcf857x.o
>  obj-$(CONFIG_GPIO_PL061)       += pl061.o
> diff --git a/include/linux/spi/74x164.h b/include/linux/spi/74x164.h
> new file mode 100644
> index 0000000..d85c52f
> --- /dev/null
> +++ b/include/linux/spi/74x164.h
> @@ -0,0 +1,11 @@
> +#ifndef LINUX_SPI_74X164_H
> +#define LINUX_SPI_74X164_H
> +
> +#define GEN_74X164_DRIVER_NAME "74x164"
> +
> +struct gen_74x164_chip_platform_data {
> +       /* number assigned to the first GPIO */
> +       unsigned        base;
> +};
> +
> +#endif
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>

Re: [PATCH v5] GPIO: add support for 74x164 serial-in/parallel-out 8-bit shift register

[Andrew Morton]

On Tue, 19 Oct 2010 09:26:42 +0200
Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote:

> On Mon, Oct 18, 2010 at 9:35 PM, Florian Fainelli <florian@openwrt.org> wrote:
> > From: Miguel Gaio <miguel.gaio@efixo.com>
> >
> > This patch adds support for generic 74x164 serial-in/parallel-out 8-bits
> > shift register. This driver can be used as a GPIO output expander.
> >
> ...
> > +struct gen_74x164_chip {
> > +       struct spi_device       *spi;
> > +       struct gpio_chip        gpio_chip;
> > +       struct mutex            lock;
> > +       u8                      port_config;
> > +};
> ...
> > +static void gen_74x164_set_value(struct gpio_chip *gc,
> > +               unsigned offset, int val)
> > +{
> > +       struct gen_74x164_chip *chip = gpio_to_chip(gc);
> > +       bool refresh;
> > +
> > +       mutex_lock(&chip->lock);
> > +       if (val)
> > +               chip->port_config |= (1 << offset);
> > +       else
> > +               chip->port_config &= ~(1 << offset);
> 
> set_bit(), clear_bit() ?

They're only to be used on `unsigned long' types, and `port_config' is
u8.

Re: [PATCH v5] GPIO: add support for 74x164 serial-in/parallel-out 8-bit shift register

[Miguel Ojeda]

On Wed, Oct 20, 2010 at 1:00 AM, Andrew Morton
<akpm@linux-foundation.org> wrote:
> On Tue, 19 Oct 2010 09:26:42 +0200
> Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote:
>
>> On Mon, Oct 18, 2010 at 9:35 PM, Florian Fainelli <florian@openwrt.org> wrote:
>> > From: Miguel Gaio <miguel.gaio@efixo.com>
>> >
>> > This patch adds support for generic 74x164 serial-in/parallel-out 8-bits
>> > shift register. This driver can be used as a GPIO output expander.
>> >
>> ...
>> > +struct gen_74x164_chip {
>> > +       struct spi_device       *spi;
>> > +       struct gpio_chip        gpio_chip;
>> > +       struct mutex            lock;
>> > +       u8                      port_config;
>> > +};
>> ...
>> > +static void gen_74x164_set_value(struct gpio_chip *gc,
>> > +               unsigned offset, int val)
>> > +{
>> > +       struct gen_74x164_chip *chip = gpio_to_chip(gc);
>> > +       bool refresh;
>> > +
>> > +       mutex_lock(&chip->lock);
>> > +       if (val)
>> > +               chip->port_config |= (1 << offset);
>> > +       else
>> > +               chip->port_config &= ~(1 << offset);
>>
>> set_bit(), clear_bit() ?
>
> They're only to be used on `unsigned long' types, and `port_config' is
> u8.
>

Right as always! Maybe BIT()? Don't we have a {SET,CLEAR}_BIT()-like
macros somewhere?

#define SET_BIT(var,nr) (var) |= BIT((nr))
#define CLEAR_BIT(var,nr) (var) &= ~BIT((nr))
#define PUT_BIT(var,nr,value) do { \
        if ((value)) \
                SET_BIT((var), (nr)); \
        else \
                CLEAR_BIT((var), (nr)); \
} while(0)

May I make a patch and try to see who could use it? I suppose a
Coccinelle's semantic patch would be great here.

Re: [PATCH v5] GPIO: add support for 74x164 serial-in/parallel-out 8-bit shift register

[Miguel Ojeda]

On Wed, Oct 20, 2010 at 9:52 AM, Miguel Ojeda
<miguel.ojeda.sandonis@gmail.com> wrote:
> On Wed, Oct 20, 2010 at 1:00 AM, Andrew Morton
> <akpm@linux-foundation.org> wrote:
>> On Tue, 19 Oct 2010 09:26:42 +0200
>> Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote:
>>
>>> On Mon, Oct 18, 2010 at 9:35 PM, Florian Fainelli <florian@openwrt.org> wrote:
>>> > From: Miguel Gaio <miguel.gaio@efixo.com>
>>> >
>>> > This patch adds support for generic 74x164 serial-in/parallel-out 8-bits
>>> > shift register. This driver can be used as a GPIO output expander.
>>> >
>>> ...
>>> > +struct gen_74x164_chip {
>>> > +       struct spi_device       *spi;
>>> > +       struct gpio_chip        gpio_chip;
>>> > +       struct mutex            lock;
>>> > +       u8                      port_config;
>>> > +};
>>> ...
>>> > +static void gen_74x164_set_value(struct gpio_chip *gc,
>>> > +               unsigned offset, int val)
>>> > +{
>>> > +       struct gen_74x164_chip *chip = gpio_to_chip(gc);
>>> > +       bool refresh;
>>> > +
>>> > +       mutex_lock(&chip->lock);
>>> > +       if (val)
>>> > +               chip->port_config |= (1 << offset);
>>> > +       else
>>> > +               chip->port_config &= ~(1 << offset);
>>>
>>> set_bit(), clear_bit() ?
>>
>> They're only to be used on `unsigned long' types, and `port_config' is
>> u8.
>>
>
> Right as always! Maybe BIT()? Don't we have a {SET,CLEAR}_BIT()-like
> macros somewhere?
>
> #define SET_BIT(var,nr) (var) |= BIT((nr))
> #define CLEAR_BIT(var,nr) (var) &= ~BIT((nr))
> #define PUT_BIT(var,nr,value) do { \
>        if ((value)) \
>                SET_BIT((var), (nr)); \
>        else \
>                CLEAR_BIT((var), (nr)); \
> } while(0)
>
> May I make a patch and try to see who could use it? I suppose a
> Coccinelle's semantic patch would be great here.
>

Well, after trying a few minutes spatch for my first time it has
already found a lot of places where the macros could be applied. I
will prepare a patch.

Re: [PATCH v5] GPIO: add support for 74x164 serial-in/parallel-out 8-bit shift register

[Florian Fainelli]

On Wednesday 20 October 2010 10:17:32 Miguel Ojeda wrote:
> On Wed, Oct 20, 2010 at 9:52 AM, Miguel Ojeda
> 
> <miguel.ojeda.sandonis@gmail.com> wrote:
> > On Wed, Oct 20, 2010 at 1:00 AM, Andrew Morton
> > 
> > <akpm@linux-foundation.org> wrote:
> >> On Tue, 19 Oct 2010 09:26:42 +0200
> >> 
> >> Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote:
> >>> On Mon, Oct 18, 2010 at 9:35 PM, Florian Fainelli <florian@openwrt.org> 
wrote:
> >>> > From: Miguel Gaio <miguel.gaio@efixo.com>
> >>> > 
> >>> > This patch adds support for generic 74x164 serial-in/parallel-out
> >>> > 8-bits shift register. This driver can be used as a GPIO output
> >>> > expander.
> >>> 
> >>> ...
> >>> 
> >>> > +struct gen_74x164_chip {
> >>> > +       struct spi_device       *spi;
> >>> > +       struct gpio_chip        gpio_chip;
> >>> > +       struct mutex            lock;
> >>> > +       u8                      port_config;
> >>> > +};
> >>> 
> >>> ...
> >>> 
> >>> > +static void gen_74x164_set_value(struct gpio_chip *gc,
> >>> > +               unsigned offset, int val)
> >>> > +{
> >>> > +       struct gen_74x164_chip *chip = gpio_to_chip(gc);
> >>> > +       bool refresh;
> >>> > +
> >>> > +       mutex_lock(&chip->lock);
> >>> > +       if (val)
> >>> > +               chip->port_config |= (1 << offset);
> >>> > +       else
> >>> > +               chip->port_config &= ~(1 << offset);
> >>> 
> >>> set_bit(), clear_bit() ?
> >> 
> >> They're only to be used on `unsigned long' types, and `port_config' is
> >> u8.
> > 
> > Right as always! Maybe BIT()? Don't we have a {SET,CLEAR}_BIT()-like
> > macros somewhere?
> > 
> > #define SET_BIT(var,nr) (var) |= BIT((nr))
> > #define CLEAR_BIT(var,nr) (var) &= ~BIT((nr))
> > #define PUT_BIT(var,nr,value) do { \
> >        if ((value)) \
> >                SET_BIT((var), (nr)); \
> >        else \
> >                CLEAR_BIT((var), (nr)); \
> > } while(0)
> > 
> > May I make a patch and try to see who could use it? I suppose a
> > Coccinelle's semantic patch would be great here.
> 
> Well, after trying a few minutes spatch for my first time it has
> already found a lot of places where the macros could be applied. I
> will prepare a patch.

Though this is certainly valid, what's the benefit in using a macro to do that 
instead of open coding the toggle of a bit in a variable?
--
Florian

Re: [PATCH v5] GPIO: add support for 74x164 serial-in/parallel-out 8-bit shift register

[Bastien ROUCARIES]

On Mon, Oct 18, 2010 at 9:35 PM, Florian Fainelli <florian@openwrt.org> wrote:
> From: Miguel Gaio <miguel.gaio@efixo.com>
>
> This patch adds support for generic 74x164 serial-in/parallel-out 8-bits
> shift register. This driver can be used as a GPIO output expander.

Could you add some documentation ? how to wire it both in soft and hard part?

Bastien

Re: [PATCH v5] GPIO: add support for 74x164 serial-in/parallel-out 8-bit shift register

[Miguel Ojeda]

On Wed, Oct 20, 2010 at 2:42 PM, Florian Fainelli <florian@openwrt.org> wrote:
> On Wednesday 20 October 2010 10:17:32 Miguel Ojeda wrote:
>> On Wed, Oct 20, 2010 at 9:52 AM, Miguel Ojeda
>>
>> <miguel.ojeda.sandonis@gmail.com> wrote:
>> > On Wed, Oct 20, 2010 at 1:00 AM, Andrew Morton
>> >
>> > <akpm@linux-foundation.org> wrote:
>> >> On Tue, 19 Oct 2010 09:26:42 +0200
>> >>
>> >> Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote:
>> >>> On Mon, Oct 18, 2010 at 9:35 PM, Florian Fainelli <florian@openwrt.org>
> wrote:
>> >>> > From: Miguel Gaio <miguel.gaio@efixo.com>
>> >>> >
>> >>> > This patch adds support for generic 74x164 serial-in/parallel-out
>> >>> > 8-bits shift register. This driver can be used as a GPIO output
>> >>> > expander.
>> >>>
>> >>> ...
>> >>>
>> >>> > +struct gen_74x164_chip {
>> >>> > +       struct spi_device       *spi;
>> >>> > +       struct gpio_chip        gpio_chip;
>> >>> > +       struct mutex            lock;
>> >>> > +       u8                      port_config;
>> >>> > +};
>> >>>
>> >>> ...
>> >>>
>> >>> > +static void gen_74x164_set_value(struct gpio_chip *gc,
>> >>> > +               unsigned offset, int val)
>> >>> > +{
>> >>> > +       struct gen_74x164_chip *chip = gpio_to_chip(gc);
>> >>> > +       bool refresh;
>> >>> > +
>> >>> > +       mutex_lock(&chip->lock);
>> >>> > +       if (val)
>> >>> > +               chip->port_config |= (1 << offset);
>> >>> > +       else
>> >>> > +               chip->port_config &= ~(1 << offset);
>> >>>
>> >>> set_bit(), clear_bit() ?
>> >>
>> >> They're only to be used on `unsigned long' types, and `port_config' is
>> >> u8.
>> >
>> > Right as always! Maybe BIT()? Don't we have a {SET,CLEAR}_BIT()-like
>> > macros somewhere?
>> >
>> > #define SET_BIT(var,nr) (var) |= BIT((nr))
>> > #define CLEAR_BIT(var,nr) (var) &= ~BIT((nr))
>> > #define PUT_BIT(var,nr,value) do { \
>> >        if ((value)) \
>> >                SET_BIT((var), (nr)); \
>> >        else \
>> >                CLEAR_BIT((var), (nr)); \
>> > } while(0)
>> >
>> > May I make a patch and try to see who could use it? I suppose a
>> > Coccinelle's semantic patch would be great here.
>>
>> Well, after trying a few minutes spatch for my first time it has
>> already found a lot of places where the macros could be applied. I
>> will prepare a patch.
>
> Though this is certainly valid, what's the benefit in using a macro to do that
> instead of open coding the toggle of a bit in a variable?

To avoid "code duplication" and simplify (it is harder to make a
mistake in the PUT_BIT() case I think) and maybe optimize [*]. See for
example this change from drivers/gpio/timbgpio.c:

-		bflr &= ~(1 << offset);
-		flr |= 1 << offset;
-		if (trigger & IRQ_TYPE_EDGE_FALLING)
-			lvr &= ~(1 << offset);
-		else
-			lvr |= 1 << offset;
+		CLEAR_BIT(bflr, offset);
+		SET_BIT(flr, offset);
+		PUT_BIT(lvr, offset, !(trigger & IRQ_TYPE_EDGE_FALLING));

I think it is clearer. Possibly it is only worth it in the PUT_BIT case.

[*] I know that gcc already optimizes this stuff, but maybe we could
call __set_bit()/__clear_bit()/__toggle_bit() from the macro if the
argument's type is UL. I am supposing here that __*_bit() functions
were coded for some reason.).

On the other hand, if *bit_() functions are out there for UL, I
suppose it is good to have a macro for the general case.

> --
> Florian
>

Re: [PATCH v5] GPIO: add support for 74x164 serial-in/parallel-out 8-bit shift register

[David Brownell]

Just for the record ... spamming my mailbox
with dozens of copies of a patch is a very bad
way to get me to allocate scarce review time.
Please reconsider your strategy, and switch to
one which doesn't try to waste so much of
the patch recipients' time.

Re: [PATCH v5] GPIO: add support for 74x164 serial-in/parallel-out 8-bit shift register

[Florian Fainelli]

On Wednesday 20 October 2010 14:51:42 Bastien ROUCARIES wrote:
> On Mon, Oct 18, 2010 at 9:35 PM, Florian Fainelli <florian@openwrt.org> 
wrote:
> > From: Miguel Gaio <miguel.gaio@efixo.com>
> > 
> > This patch adds support for generic 74x164 serial-in/parallel-out 8-bits
> > shift register. This driver can be used as a GPIO output expander.
> 
> Could you add some documentation ? how to wire it both in soft and hard
> part?

This is just an example of course, and I will add a subsequent patch adding an 
entry for it in Documentation/

static struct spi_board_info board_spi_devices[] = {
 {
	.modalias = "74x164",
	.max_speed_hz = 781000,
	.bus_num = 1,
	.controller_data = (void *) SPI_GPIO_NO_CHIPSELECT,
	.mode = SPI_MODE_0,
	.platform_data = &nb4_74x164_platform_data
}
};

and then registering your board_spi_devices with:

spi_register_board_info(board_spi_devices, ARRAY_SIZE(board_spi_devices);
--
Florian