Driver for 7-segment displays connected over GPIOs

 .../devicetree/bindings/misc/gpio-7seg.txt         |   18 +++
 drivers/misc/Kconfig                               |   13 ++
 drivers/misc/Makefile                              |    1 +
 drivers/misc/gpio-7seg.c                           |  168 ++++++++++++++++++++

Quoted 4 lines

 .../devicetree/bindings/misc/gpio-7seg.txt         |   18 +++
 drivers/misc/Kconfig                               |   13 ++
 drivers/misc/Makefile                              |    1 +
 drivers/misc/gpio-7seg.c                           |  168 ++++++++++++++++++++

I wonder if it would make sense to merge this into the LED subsystem rather than having it as a standalone driver.

Quoted 5 lines

Documentation/devicetree/bindings/misc/gpio-7seg.txt new file +0 -0

new file mode 100644
index 0000000..107d178

Documentation/devicetree/bindings/misc/gpio-7seg.txt line 5

Quoted 6 lines

Documentation/devicetree/bindings/misc/gpio-7seg.txt new file +5 -0

@@ -0,0 +1,18 @@
+* 7-segment driver connected over GPIO through a BCD decoder
+
+Required properties:
+- compatible: "generic,gpio-7seg"
+- gpios: list of GPIOs to use to control the 7-segment display

Maybe list the minimum and maximum number of gpio lines here?

Documentation/devicetree/bindings/misc/gpio-7seg.txt line 9

Quoted 4 lines

Documentation/devicetree/bindings/misc/gpio-7seg.txt new file +4 -0

+	sdev->dev_attr.attr.name = "value";
+	sdev->dev_attr.attr.mode = S_IRUGO | S_IWUGO;
+	sdev->dev_attr.show = gpio_7seg_show;
+	sdev->dev_attr.store = gpio_7seg_store;

Any reason why you are not using the DEVICE_ATTR macro?

Dear Arnd Bergmann,

Quoted 5 lines · Arnd Bergmann

Quoted 4 lines

 .../devicetree/bindings/misc/gpio-7seg.txt         |   18 +++
 drivers/misc/Kconfig                               |   13 ++
 drivers/misc/Makefile                              |    1 +
 drivers/misc/gpio-7seg.c                           |  168 ++++++++++++++++++++

Quoted 2 lines

I wonder if it would make sense to merge this into the LED subsystem rather than having it as a standalone driver.

Hum, maybe. How do you see it fitting inside the LED subsystem? The purpose of the device is quite different in its userspace to kernel interface, no?

Quoted 6 lines

drivers/misc/gpio-7seg.c line 116

Quoted 4 lines

drivers/misc/gpio-7seg.c new file +4 -0

+	sdev->dev_attr.attr.name = "value";
+	sdev->dev_attr.attr.mode = S_IRUGO | S_IWUGO;
+	sdev->dev_attr.show = gpio_7seg_show;
+	sdev->dev_attr.store = gpio_7seg_store;

Any reason why you are not using the DEVICE_ATTR macro?

DEVICE_ATTR declares the structure, and I wanted one per gpio_7seg device, which is dynamically allocated, so I thought that using DEVICE_ATTR was not possible. But now that you point this, I realize that I was stupid. I can perfectly live with one single global 'struct device_attribute' that I register to several devices using device_create_file(), no?

That said, is it worth the effort to continue polishing this driver, after seeing the feedback from Greg about it? I don't mind working on it more and fix the problems of course, but if the general answer is that we don't want such a driver in the kernel, then I'd better not spend more time on this.

Quoted 9 lines · Thomas Petazzoni

Dear Arnd Bergmann,

Quoted 5 lines · Arnd Bergmann

Quoted 4 lines

 .../devicetree/bindings/misc/gpio-7seg.txt         |   18 +++
 drivers/misc/Kconfig                               |   13 ++
 drivers/misc/Makefile                              |    1 +
 drivers/misc/gpio-7seg.c                           |  168 ++++++++++++++++++++

Quoted 6 lines

Quoted 2 lines

I wonder if it would make sense to merge this into the LED subsystem rather than having it as a standalone driver.

Hum, maybe. How do you see it fitting inside the LED subsystem? The purpose of the device is quite different in its userspace to kernel interface, no?

The LED subsystem can be used by both in-kernel and by user space front-ends. It currently supports on/off, blinking and variable brightness LEDs. Adding 7-segment LEDs may be a stretch but also would fit into the same basic interface I think.

In addition, it would not have to be GPIO based, although that may be the only implementation we need for the foreseeable future. Well, unless you make a driver for PC-style "port-0x80" PCI cards.

Quoted 7 lines

Quoted 6 lines

drivers/misc/gpio-7seg.c line 116

Quoted 4 lines

drivers/misc/gpio-7seg.c new file +4 -0

+	sdev->dev_attr.attr.name = "value";
+	sdev->dev_attr.attr.mode = S_IRUGO | S_IWUGO;
+	sdev->dev_attr.show = gpio_7seg_show;
+	sdev->dev_attr.store = gpio_7seg_store;

Any reason why you are not using the DEVICE_ATTR macro?

Quoted 6 lines

DEVICE_ATTR declares the structure, and I wanted one per gpio_7seg device, which is dynamically allocated, so I thought that using DEVICE_ATTR was not possible. But now that you point this, I realize that I was stupid. I can perfectly live with one single global 'struct device_attribute' that I register to several devices using device_create_file(), no?

Correct.

Quoted 5 lines

That said, is it worth the effort to continue polishing this driver, after seeing the feedback from Greg about it? I don't mind working on it more and fix the problems of course, but if the general answer is that we don't want such a driver in the kernel, then I'd better not spend more time on this.

I only saw the discussion after I had sent my reply. I think that this driver in the form of a drivers/misc sysfs driver is not worth pursuing, but it may still be possible to integrate it nicely into the LED framework.

This patch adds a driver to control 7-segment displays connected over GPIOs through a BCD encoder. Typically, up to four GPIOs go into a BCD encoder, that drives the 7-segment display to display a numeric value between 0 and 2^ngpios-1.

Once this driver is enabled, it creates a 'value' field in the sysfs directory of the device, which allows to set the current value displayed by the 7-segment.

It has been successfully tested on the Marvell Armada 370 and Marvell Armada XP evaluation boards. Both of them have a 7-segment display that can be controlled using 3 GPIOs.

Signed-off-by Thomas Petazzoni <[email protected]>

---
 .../devicetree/bindings/misc/gpio-7seg.txt         |   18 +++
 drivers/misc/Kconfig                               |   13 ++
 drivers/misc/Makefile                              |    1 +
 drivers/misc/gpio-7seg.c                           |  168 ++++++++++++++++++++
 4 files changed, 200 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/misc/gpio-7seg.txt
 create mode 100644 drivers/misc/gpio-7seg.c

Documentation/devicetree/bindings/misc/gpio-7seg.txt new file +8 -0

new file mode 100644
index 0000000..107d178
@@ -0,0 +1,18 @@
+* 7-segment driver connected over GPIO through a BCD decoder
+
+Required properties:
+- compatible: "generic,gpio-7seg"
+- gpios: list of GPIOs to use to control the 7-segment display
+
+Optional properties:
+- default-value: default value shown by the 7-segment display at boot

drivers/misc/Kconfig +13 -0

index b151b7c..19f9d22 100644
@@ -93,6 +93,19 @@ config ATMEL_TCB_CLKSRC_BLOCK
 	  TC can be used for other purposes, such as PWM generation and
 	  interval timing.
 
+config GPIO_7SEG
+	tristate "GPIO-connected 7-segment display driver"
+	help
+	  This option enables a driver to control 7-segment displays
+	  connected over GPIOs through a BCD encoder. Typically, up to
+	  four GPIOs go into a BCD encoder, that drives the 7-segment
+	  display to display a numeric value between 0 and
+	  2^ngpios-1.
+
+	  Once this driver is enabled, it creates a 'value' field in
+	  the sysfs directory of the device, which allows to set the
+	  current value displayed by the 7-segment.
+
 config IBM_ASM
 	tristate "Device driver for IBM RSA service processor"
 	depends on X86 && PCI && INPUT

drivers/misc/Makefile +1 -0

index 2129377..b789358 100644
@@ -10,6 +10,7 @@ obj-$(CONFIG_INTEL_MID_PTI)	+= pti.o
 obj-$(CONFIG_ATMEL_PWM)		+= atmel_pwm.o
 obj-$(CONFIG_ATMEL_SSC)		+= atmel-ssc.o
 obj-$(CONFIG_ATMEL_TCLIB)	+= atmel_tclib.o
+obj-$(CONFIG_GPIO_7SEG)         += gpio-7seg.o
 obj-$(CONFIG_BMP085)		+= bmp085.o
 obj-$(CONFIG_BMP085_I2C)	+= bmp085-i2c.o
 obj-$(CONFIG_BMP085_SPI)	+= bmp085-spi.o

drivers/misc/gpio-7seg.c new file +168 -0

new file mode 100644
index 0000000..8cc5113
@@ -0,0 +1,168 @@
+/*
+ * Simple driver to control a 7 segment display connected through a
+ * BCD decoder using GPIOs.
+ *
+ * Copyright (C) 2012 Marvell
+ *
+ * Thomas Petazzoni <[email protected]>
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2.  This program is licensed "as is" without any
+ * warranty of any kind, whether express or implied.
+ */
+
+#include <linux/gpio.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_gpio.h>
+#include <linux/platform_device.h>
+
+struct gpio_7seg_dev {
+	struct device_attribute dev_attr;
+	int *gpios;
+	int ngpios;
+	int value;
+	int maxvalue;
+};
+
+static void gpio_7seg_display_val(struct gpio_7seg_dev *sdev,
+				  int value)
+{
+	int i;
+
+	for (i = 0; i < sdev->ngpios; i++) {
+		int bitval = (value & BIT(i)) ? 1 : 0;
+		gpio_set_value_cansleep(sdev->gpios[i], bitval);
+	}
+}
+
+static ssize_t gpio_7seg_show(struct device *dev, struct device_attribute *attr,
+			      char *buf)
+{
+	struct gpio_7seg_dev *sdev = dev_get_drvdata(dev);
+	return snprintf(buf, PAGE_SIZE, "%d\n", sdev->value);
+}
+
+static ssize_t gpio_7seg_store(struct device *dev, struct device_attribute *attr,
+			       const char *buf, size_t count)
+{
+	struct gpio_7seg_dev *sdev = dev_get_drvdata(dev);
+	unsigned long value;
+	char *end;
+
+	value = simple_strtoul(buf, &end, 0);
+	if (end == buf)
+		return -EINVAL;
+
+	if (value >= sdev->maxvalue)
+		return -EINVAL;
+
+	gpio_7seg_display_val(sdev, value);
+	sdev->value = value;
+
+	return count;
+}
+
+static int gpio_7seg_probe(struct platform_device *pdev)
+{
+	struct device_node *np = pdev->dev.of_node;
+	struct gpio_7seg_dev *sdev;
+	int ret, i;
+
+	/*
+	 * We only support being probed through the device tree, for
+	 * now
+	 */
+	if (!np)
+		return -EINVAL;
+
+	sdev = devm_kzalloc(&pdev->dev, sizeof(struct gpio_7seg_dev),
+			    GFP_KERNEL);
+	if (!sdev)
+		return -ENOMEM;
+
+	sdev->ngpios = of_gpio_count(np);
+	if (!sdev->ngpios)
+		return -EINVAL;
+
+	sdev->gpios = devm_kzalloc(&pdev->dev, sizeof(int) * sdev->ngpios,
+				   GFP_KERNEL);
+	if (!sdev->gpios)
+		return -ENOMEM;
+
+	for (i = 0; i < sdev->ngpios; i++) {
+		sdev->gpios[i] = of_get_gpio(np, i);
+		ret = gpio_request_one(sdev->gpios[i], GPIOF_DIR_OUT, NULL);
+		if (ret) {
+			/*
+			 * Mark this GPIO as non-requested for the
+			 * error handling code
+			 */
+			sdev->gpios[i] = ret;
+			goto gpio_cleanup;
+		}
+	}
+
+	ret = of_property_read_u32(np, "default-value", &sdev->value);
+	if (ret)
+		sdev->value = 0;
+
+	sdev->maxvalue = 1 << sdev->ngpios;
+
+	sdev->dev_attr.attr.name = "value";
+	sdev->dev_attr.attr.mode = S_IRUGO | S_IWUGO;
+	sdev->dev_attr.show = gpio_7seg_show;
+	sdev->dev_attr.store = gpio_7seg_store;
+
+	ret = device_create_file(&pdev->dev, &sdev->dev_attr);
+	if (ret)
+		goto gpio_cleanup;
+
+	gpio_7seg_display_val(sdev, sdev->value);
+
+	platform_set_drvdata(pdev, sdev);
+	return 0;
+
+gpio_cleanup:
+	for (i = 0; i < sdev->ngpios; i++)
+		if (gpio_is_valid(sdev->gpios[i]))
+			gpio_free(sdev->gpios[i]);
+	return ret;
+}
+
+static int gpio_7seg_remove(struct platform_device *pdev)
+{
+	struct gpio_7seg_dev *sdev;
+	int i;
+
+	sdev = platform_get_drvdata(pdev);
+
+	device_remove_file(&pdev->dev, &sdev->dev_attr);
+
+	for (i = 0; i < sdev->ngpios; i++)
+		if (gpio_is_valid(sdev->gpios[i]))
+			gpio_free(sdev->gpios[i]);
+	return 0;
+}
+
+static const struct of_device_id gpio_7seg_of_match_table[] = {
+	{ .compatible = "generic,gpio-7seg" },
+	{},
+};
+
+static struct platform_driver gpio_7seg_driver = {
+	.probe  = gpio_7seg_probe,
+	.remove = gpio_7seg_remove,
+	.driver = {
+		.owner = THIS_MODULE,
+		.name  = "gpio-7seg",
+		.of_match_table = of_match_ptr(gpio_7seg_of_match_table),
+	},
+};
+
+module_platform_driver(gpio_7seg_driver);
+
+MODULE_AUTHOR("Thomas Petazzoni <[email protected]>");
+MODULE_DESCRIPTION("Simple GPIO-connected 7-segment display driver");
+MODULE_LICENSE("GPL");

Signature · 7 lines

1.7.9.5


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Now that we have a 7-segment display driver in the kernel, use it on the Marvell Armada XP DB evaluation board.

Signed-off-by Thomas Petazzoni <[email protected]>

---
 arch/arm/boot/dts/armada-xp-db.dts |    5 +++++
 1 file changed, 5 insertions(+)

arch/arm/boot/dts/armada-xp-db.dts +0 -0

index 8e53b25..8654c1b 100644

Dear drivers/{char,misc}/ maintainers,

This patch series implements a simple driver for 7-segment displays connected over GPIOs through a BCD encoder. The userspace interface of the driver is very simple: just a "value" file in the sysfs directory of the device, which can be written to (changes the value shown on the 7-segment display) and read (gives the current value).

If you ever add/remove/modify sysfs files, you have to also do the same for the Documentation/ABI/ files as well, please redo that in this patch series.

But, the bigger question is, why is this a kernel driver at all? Can't you do this from userspace today without any new kernel code?

Dear Greg Kroah-Hartman,

Quoted 3 lines · Greg Kroah-Hartman

If you ever add/remove/modify sysfs files, you have to also do the same for the Documentation/ABI/ files as well, please redo that in this patch series.

Sure.

Quoted 2 lines

But, the bigger question is, why is this a kernel driver at all? Can't you do this from userspace today without any new kernel code?

Indeed, it can be done from userspace since we're just controlling GPIOs. Having a kernel driver allows to describe this device in the Device Tree, like all other devices, and have it "magically" appear, with a convenient user-space interface.

Not having a kernel driver means that gazillions of applications re-invent the same piece of code over and over again, have to hardcode the GPIO numbers for a given piece of hardware, while the kernel abstract all of this very nicely.

Quoted 4 lines

Indeed, it can be done from userspace since we're just controlling GPIOs. Having a kernel driver allows to describe this device in the Device Tree, like all other devices, and have it "magically" appear, with a convenient user-space interface.

Ok, that means you want to use the kernel-standard userspace interface for displays, right? If so, why not use it? I thought we already supported LCD displays already, isn't there an interface for it? Don't create random magic sysfs files without really thinking about it.

Quoted 4 lines

Not having a kernel driver means that gazillions of applications re-invent the same piece of code over and over again, have to hardcode the GPIO numbers for a given piece of hardware, while the kernel abstract all of this very nicely.

That sounds like a wonderful use of a userspace library to do this properly. Much like libusb does, right?

I still think as this can be done in userspace, it probably should be.

Quoted 9 lines

Quoted 4 lines

Not having a kernel driver means that gazillions of applications re-invent the same piece of code over and over again, have to hardcode the GPIO numbers for a given piece of hardware, while the kernel abstract all of this very nicely.

That sounds like a wonderful use of a userspace library to do this properly. Much like libusb does, right?

I still think as this can be done in userspace, it probably should be.

Understood. Patches discarded.

Quoted 10 lines · Thomas Petazzoni

Quoted 9 lines

Quoted 4 lines

Not having a kernel driver means that gazillions of applications re-invent the same piece of code over and over again, have to hardcode the GPIO numbers for a given piece of hardware, while the kernel abstract all of this very nicely.

That sounds like a wonderful use of a userspace library to do this properly. Much like libusb does, right?

I still think as this can be done in userspace, it probably should be.

Quoted 1 line

Understood. Patches discarded.

Thinking more about this. How would your userspace library know on which GPIOs your 7-seg segment device is connected? Should it parse the Device Tree from userspace? Given by the user-space application who would have to hardcode the GPIO numbers, completely defeating the hardware abstraction layer that the kernel intends to be?

Quoted 12 lines · Thomas Petazzoni

Quoted 10 lines · Thomas Petazzoni

Quoted 9 lines

Quoted 4 lines

Not having a kernel driver means that gazillions of applications re-invent the same piece of code over and over again, have to hardcode the GPIO numbers for a given piece of hardware, while the kernel abstract all of this very nicely.

That sounds like a wonderful use of a userspace library to do this properly. Much like libusb does, right?

I still think as this can be done in userspace, it probably should be.

Quoted 7 lines

Quoted 1 line

Understood. Patches discarded.

Thinking more about this. How would your userspace library know on which GPIOs your 7-seg segment device is connected? Should it parse the Device Tree from userspace? Given by the user-space application who would have to hardcode the GPIO numbers, completely defeating the hardware abstraction layer that the kernel intends to be?

Maybe an 'init' of 'config' function in the library should allow the user to define the GPIOs that are connected to the 7-seg display? Or store the GPIOs in some config file that the library parses when it's loaded.

This does require the user to know what GPIOs are connected and forces then to do a bit of setup to use the display. But the actual use of the display is still abstracted.

The library should also allow for multiple 7-seg displays. The old PC "post" diagnostic boards usually had two displays on them.

Quoted 10 lines · Thomas Petazzoni

Quoted 9 lines

Quoted 4 lines

Not having a kernel driver means that gazillions of applications re-invent the same piece of code over and over again, have to hardcode the GPIO numbers for a given piece of hardware, while the kernel abstract all of this very nicely.

That sounds like a wonderful use of a userspace library to do this properly. Much like libusb does, right?

I still think as this can be done in userspace, it probably should be.

Quoted 1 line

Understood. Patches discarded.

Thinking more about this. How would your userspace library know on which GPIOs your 7-seg segment device is connected? Should it parse the Device Tree from userspace? Given by the user-space application who would have to hardcode the GPIO numbers, completely defeating the hardware abstraction layer that the kernel intends to be?

Quoted 4 lines

Not having a kernel driver means that gazillions of applications re-invent the same piece of code over and over again, have to hardcode the GPIO numbers for a given piece of hardware, while the kernel abstract all of this very nicely.

That sounds like a wonderful use of a userspace library to do this properly. Much like libusb does, right?

I still think as this can be done in userspace, it probably should be.

Dear Greg Kroah-Hartman,

Quoted 8 lines · Greg Kroah-Hartman

Quoted 4 lines

Not having a kernel driver means that gazillions of applications re-invent the same piece of code over and over again, have to hardcode the GPIO numbers for a given piece of hardware, while the kernel abstract all of this very nicely.

That sounds like a wonderful use of a userspace library to do this properly. Much like libusb does, right?

Quoted 1 line

I still think as this can be done in userspace, it probably should be.

So that's why we have drivers/usb/misc/usbsevseg.c in the kernel, which uses sysfs files to output text or numbers on a 7-segment display, while it could all be done using libusb from userspace? Seems like back in 2008 you Signed-off-by on the patch adding this driver :-)

I don't remember what I committed a month ago, let alone 4 years ago. So trying to use that as an excuse as to why I should take your patch isn't a really good idea, sorry :)

As Russell points out, if you want this in the kernel, it needs to have a good userspace api, and that needs a lot more work than just a single sysfs file.

This patch series implements a simple driver for 7-segment displays connected over GPIOs through a BCD encoder. The userspace interface of the driver is very simple: just a "value" file in the sysfs directory of the device, which can be written to (changes the value shown on the 7-segment display) and read (gives the current value).

Quoted 5 lines · <[email protected]> wrote:

This patch series implements a simple driver for 7-segment displays connected over GPIOs through a BCD encoder. The userspace interface of the driver is very simple: just a "value" file in the sysfs directory of the device, which can be written to (changes the value shown on the 7-segment display) and read (gives the current value).

The ARM Integrator AP and CP base boards I have here have 7-segment displays as well. But it's not on GPIO but dedicated registers, so I could implement these as, e.g. LEDs.

But with this framework I'd just make a driver that pokes directly into the registers.

However as Russell points out, this needs to be thought through. I have a period on my display between the two characters, so how do we model that period?