linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH linux v1 0/4] Seven segment display support
@ 2016-12-14  7:55 Jaghathiswari Rankappagounder Natarajan
  2016-12-14  7:55 ` [PATCH linux v1 1/4] Documentation: dt-bindings: Document bindings for seven " Jaghathiswari Rankappagounder Natarajan
                   ` (4 more replies)
  0 siblings, 5 replies; 18+ messages in thread
From: Jaghathiswari Rankappagounder Natarajan @ 2016-12-14  7:55 UTC (permalink / raw)
  To: openbmc, robh+dt, mark.rutland, linux, arnd, gregkh
  Cc: Jaghathiswari Rankappagounder Natarajan, devicetree,
	linux-kernel, linux-arm-kernel, joel

This patchset includes:

Documentation for the binding which provides an interface for adding clock,
data and clear signal GPIO lines to control seven segment display.

The platform device driver provides an API for displaying on two 7-segment
displays, and implements the required bit-banging. The hardware assumed is
74HC164 wired to two 7-segment displays.

The character device driver implements the user-space API for letting a user
write to two 7-segment displays including any conversion methods necessary
to map the user input to two 7-segment displays.

Adding clock, data and clear signal GPIO lines in the devicetree to control
seven segment display on zaius platform.

The platform driver matches on the device tree node; the platform driver also
initializes the character device.

Tested that the seven segment display works properly by writing to the
character device file on a EVB AST2500 board which also has 74HC164 wired
to two 7-segment displays.

Jaghathiswari Rankappagounder Natarajan (4):
  Documentation: dt-bindings: Document bindings for seven segment
    display support
  drivers: misc: Character device driver for seven segment display
  drivers: misc: Platform driver for seven segment display support
  arm: dts: Add dt-binding to support seven segment display on zaius

 .../devicetree/bindings/misc/seven-seg-gpio.txt    |  27 +++
 arch/arm/boot/dts/aspeed-bmc-opp-zaius.dts         |   8 +
 drivers/misc/Kconfig                               |  16 ++
 drivers/misc/Makefile                              |   2 +
 drivers/misc/seven_seg_disp.c                      | 197 ++++++++++++++++++++
 drivers/misc/seven_seg_disp.h                      |  34 ++++
 drivers/misc/seven_seg_gpio.c                      | 206 +++++++++++++++++++++
 7 files changed, 490 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/misc/seven-seg-gpio.txt
 create mode 100644 drivers/misc/seven_seg_disp.c
 create mode 100644 drivers/misc/seven_seg_disp.h
 create mode 100644 drivers/misc/seven_seg_gpio.c

--
2.8.0.rc3.226.g39d4020

^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCH linux v1 1/4] Documentation: dt-bindings: Document bindings for seven segment display support
  2016-12-14  7:55 [PATCH linux v1 0/4] Seven segment display support Jaghathiswari Rankappagounder Natarajan
@ 2016-12-14  7:55 ` Jaghathiswari Rankappagounder Natarajan
  2016-12-14  7:55 ` [PATCH linux v1 2/4] drivers: misc: Character device driver for seven segment display Jaghathiswari Rankappagounder Natarajan
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 18+ messages in thread
From: Jaghathiswari Rankappagounder Natarajan @ 2016-12-14  7:55 UTC (permalink / raw)
  To: openbmc, robh+dt, mark.rutland, linux, arnd, gregkh
  Cc: Jaghathiswari Rankappagounder Natarajan, devicetree,
	linux-kernel, linux-arm-kernel, joel

This binding provides interface for adding clock, data and clear signal GPIO
lines to control seven segment display.

Signed-off-by: Jaghathiswari Rankappagounder Natarajan <jaghu@google.com>
---
 .../devicetree/bindings/misc/seven-seg-gpio.txt    | 27 ++++++++++++++++++++++
 1 file changed, 27 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/misc/seven-seg-gpio.txt

diff --git a/Documentation/devicetree/bindings/misc/seven-seg-gpio.txt b/Documentation/devicetree/bindings/misc/seven-seg-gpio.txt
new file mode 100644
index 0000000..9a4e255
--- /dev/null
+++ b/Documentation/devicetree/bindings/misc/seven-seg-gpio.txt
@@ -0,0 +1,27 @@
+This binding defines interface to add clock, data and clear GPIO lines required
+for seven segment display support.
+
+Required properties:
+- compatible : should be "seven-seg-gpio-dev".
+- clock-gpios :  Should specify the GPIO pin connected to the Clock line on the
+  hardware.
+- data-gpios : Should specify the GPIO pin connected to Data line on the
+  hardware.
+- clear-gpios : Should specify the GPIO pin connected to Clear line on the
+  hardware.
+
+Optional properties:
+- refresh-interval-ms : The interval at which to refresh the display.
+  If this property is not present, the default value is 1000.
+
+Examples:
+
+#include <dt-bindings/gpio/gpio.h>
+
+seven-seg-disp {
+	compatible = "seven-seg-gpio-dev";
+	refresh-interval-ms = "1000";
+	clock-gpios = <&gpio 0 GPIO_ACTIVE_LOW>;
+	data-gpios = <&gpio 1 GPIO_ACTIVE_HIGH>;
+	clear-gpios = <&gpio 2 GPIO_ACTIVE_HIGH>;
+};
--
2.8.0.rc3.226.g39d4020

^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH linux v1 2/4] drivers: misc: Character device driver for seven segment display
  2016-12-14  7:55 [PATCH linux v1 0/4] Seven segment display support Jaghathiswari Rankappagounder Natarajan
  2016-12-14  7:55 ` [PATCH linux v1 1/4] Documentation: dt-bindings: Document bindings for seven " Jaghathiswari Rankappagounder Natarajan
@ 2016-12-14  7:55 ` Jaghathiswari Rankappagounder Natarajan
  2016-12-14 12:32   ` Russell King - ARM Linux
  2016-12-14  7:55 ` [PATCH linux v1 3/4] drivers: misc: Platform driver for seven segment display support Jaghathiswari Rankappagounder Natarajan
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 18+ messages in thread
From: Jaghathiswari Rankappagounder Natarajan @ 2016-12-14  7:55 UTC (permalink / raw)
  To: openbmc, robh+dt, mark.rutland, linux, arnd, gregkh
  Cc: Jaghathiswari Rankappagounder Natarajan, devicetree,
	linux-kernel, linux-arm-kernel, joel

Character device driver which implements the user-space
API for letting a user write to two 7-segment displays including
any conversion methods necessary to map the user input
to two 7-segment displays.

Signed-off-by: Jaghathiswari Rankappagounder Natarajan <jaghu@google.com>
---
 drivers/misc/Kconfig          |   8 ++
 drivers/misc/Makefile         |   1 +
 drivers/misc/seven_seg_disp.c | 197 ++++++++++++++++++++++++++++++++++++++++++
 drivers/misc/seven_seg_disp.h |  34 ++++++++
 4 files changed, 240 insertions(+)
 create mode 100644 drivers/misc/seven_seg_disp.c
 create mode 100644 drivers/misc/seven_seg_disp.h

diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
index a216b46..a21aec1 100644
--- a/drivers/misc/Kconfig
+++ b/drivers/misc/Kconfig
@@ -791,6 +791,14 @@ config PANEL_CHANGE_MESSAGE
 	  If you say 'Y' here, you'll be able to choose a message yourself. Otherwise,
 	  say 'N' and keep the default message with the version.

+config SEVEN_SEGMENT_DISPLAY
+	tristate "Character driver for seven segment display support"
+	help
+	  Character device driver which implements the user-space
+	  API for letting a user write to two 7-segment displays including
+	  any conversion methods necessary to map the user input
+	  to two 7-segment displays.
+
 config PANEL_BOOT_MESSAGE
 	depends on PANEL && PANEL_CHANGE_MESSAGE="y"
 	string "New initialization message"
diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
index b2fb6dbf..c2defbd 100644
--- a/drivers/misc/Makefile
+++ b/drivers/misc/Makefile
@@ -56,4 +56,5 @@ obj-$(CONFIG_GENWQE)		+= genwqe/
 obj-$(CONFIG_ECHO)		+= echo/
 obj-$(CONFIG_VEXPRESS_SYSCFG)	+= vexpress-syscfg.o
 obj-$(CONFIG_CXL_BASE)		+= cxl/
+obj-$(CONFIG_SEVEN_SEGMENT_DISPLAY)	+= seven_seg_disp.o
 obj-$(CONFIG_PANEL)             += panel.o
diff --git a/drivers/misc/seven_seg_disp.c b/drivers/misc/seven_seg_disp.c
new file mode 100644
index 0000000..4daeac5
--- /dev/null
+++ b/drivers/misc/seven_seg_disp.c
@@ -0,0 +1,197 @@
+/*
+ * Copyright (c) 2016 Google, Inc
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 or later as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/module.h>
+#include <linux/version.h>
+#include <linux/kernel.h>
+#include <linux/types.h>
+#include <linux/kdev_t.h>
+#include <linux/fs.h>
+#include <linux/uaccess.h>
+#include <linux/ctype.h>
+#include <linux/of.h>
+#include <linux/io.h>
+#include <linux/slab.h>
+#include <linux/platform_device.h>
+
+#include "seven_seg_disp.h"
+
+#define LED_DOT 0x01
+
+/*
+ * 0   1   2   3   4   5   6   7   8   9   A   B   C   D   E   F
+ *  _       _   _       _   _   _   _   _   _       _       _   _
+ * | |   |  _|  _| |_| |_  |_    | |_| |_| |_| |_  |    _| |_  |_
+ * |_|   | |_   _|   |  _| |_|   | |_|   | | | |_| |_  |_| |_  |
+ *
+ * data[7:1] = led[a:g]
+ */
+const u8 seven_seg_bits[] = {
+	0xFC, 0x60, 0xDA, 0xF2, 0x66, 0xB6, 0xBE, 0xE0,
+	0xFE, 0xF6, 0xEE, 0x3E, 0x9C, 0x7A, 0x9E, 0x8E
+	};
+
+/*
+ * 0   1   2   3   4   5   6   7   8   9   A   B   C   D   E   F
+ *      _       _   _                              _            _
+ *     |   |_  |_| |_  _   _   _   _   _   _   _  |_    _|  _| | |
+ *     |_  |_  |   |                               _|  |_| |_| | |
+ *
+ * data[7:1] = led[a:g]
+ */
+const u8 special_seven_seg_bits[] = {
+	0x00, 0x9C, 0x1E, 0xCE, 0x8E, 0x02, 0x02, 0x02,
+	0x02, 0x02, 0x02, 0x02, 0xB6, 0x7A, 0x7A, 0xEC
+	};
+
+static dev_t seven_seg_devno;
+static struct class *seven_seg_disp_class;
+
+static int seven_seg_disp_open(struct inode *inode, struct file *filp)
+{
+	struct seven_seg_disp_dev *disp_dev;
+
+	disp_dev = container_of(inode->i_cdev,
+				 struct seven_seg_disp_dev, cdev);
+	filp->private_data = disp_dev;
+	return 0;
+}
+
+static int seven_seg_disp_close(struct inode *inode, struct file *filp)
+{
+	filp->private_data = NULL;
+	return 0;
+}
+
+static ssize_t seven_seg_disp_read(struct file *filp, char __user *buf, size_t
+				len, loff_t *off)
+{
+	struct seven_seg_disp_dev *disp_dev = filp->private_data;
+
+	if (disp_dev->disp_data_valid)
+		return -EINVAL;
+
+	if (copy_to_user(buf, disp_dev->seven_seg_disp_data_array,
+				MAX_DISP_CHAR_SIZE) != 0) {
+		return -EFAULT;
+	}
+
+	return 0;
+}
+
+static u16 convert_to_disp_data(char *buf)
+{
+	u8 low_display;
+	u8 high_display;
+	u16 led_value;
+
+	low_display = seven_seg_bits[hex_to_bin(buf[2])];
+
+	high_display = (buf[0] == '1') ?
+	special_seven_seg_bits[hex_to_bin(buf[1])] :
+	seven_seg_bits[hex_to_bin(buf[1])];
+
+	led_value = low_display | (high_display << 8);
+	if (buf[0] == '1')
+		led_value |= LED_DOT | (LED_DOT << 8);
+
+	return led_value;
+}
+
+static ssize_t seven_seg_disp_write(struct file *filp, const char __user *buf,
+				size_t len, loff_t *off)
+{
+	int length = len - 1;
+	int i;
+
+	struct seven_seg_disp_dev *disp_dev = filp->private_data;
+
+	if (length != MAX_DISP_CHAR_SIZE)
+		return -EINVAL;
+
+	if (copy_from_user(disp_dev->seven_seg_disp_data_array,
+				buf, length) != 0) {
+		return -EFAULT;
+	}
+
+	for (i = 0; i < MAX_DISP_CHAR_SIZE; i++) {
+		if (!isxdigit(disp_dev->seven_seg_disp_data_array[i]))
+			return -EINVAL;
+	}
+
+	disp_dev->current_seven_seg_disp_data = convert_to_disp_data(
+			disp_dev->seven_seg_disp_data_array);
+	disp_dev->disp_data_valid = true;
+	disp_dev->update_seven_seg_data(&disp_dev->parent,
+			disp_dev->current_seven_seg_disp_data);
+
+	return len;
+}
+
+static const struct file_operations seven_seg_disp_fops = {
+
+	.owner = THIS_MODULE,
+	.open = seven_seg_disp_open,
+	.release = seven_seg_disp_close,
+	.read = seven_seg_disp_read,
+	.write = seven_seg_disp_write
+};
+
+void seven_seg_rem_cdev(struct seven_seg_disp_dev *disp_dev)
+{
+	cdev_del(&disp_dev->cdev);
+	device_destroy(seven_seg_disp_class, seven_seg_devno);
+}
+
+int seven_seg_setup_cdev(struct seven_seg_disp_dev *disp_dev,
+	void (*update_disp_data)(struct device *, u16 data))
+{
+	struct device *dev;
+	int err;
+
+	dev = device_create(seven_seg_disp_class, &disp_dev->parent,
+			seven_seg_devno,
+			NULL, "seven_seg_disp_val");
+	if (dev == NULL)
+		return -1;
+	disp_dev->dev = dev;
+	disp_dev->update_seven_seg_data = update_disp_data;
+	disp_dev->disp_data_valid = false;
+
+	cdev_init(&disp_dev->cdev, &seven_seg_disp_fops);
+	err = cdev_add(&disp_dev->cdev, seven_seg_devno, 1);
+	if (err)
+		device_destroy(seven_seg_disp_class, seven_seg_devno);
+	return err;
+}
+
+static int __init seven_seg_disp_init(void)
+{
+	if (alloc_chrdev_region(&seven_seg_devno, 0, 1, "disp_state") < 0)
+		return -1;
+
+	seven_seg_disp_class = class_create(THIS_MODULE, "disp_state");
+	if (seven_seg_disp_class == NULL)
+		goto unreg_chrdev;
+
+unreg_chrdev:
+	unregister_chrdev_region(seven_seg_devno, 1);
+	return -1;
+}
+
+static void __exit seven_seg_disp_exit(void)
+{
+	class_destroy(seven_seg_disp_class);
+	unregister_chrdev_region(seven_seg_devno, 1);
+}
+
+module_init(seven_seg_disp_init);
+module_exit(seven_seg_disp_exit);
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Jaghathiswari Rankappagounder Natarajan <jaghu@google.com>");
+MODULE_DESCRIPTION("Seven segment display character driver");
diff --git a/drivers/misc/seven_seg_disp.h b/drivers/misc/seven_seg_disp.h
new file mode 100644
index 0000000..b0f93c5
--- /dev/null
+++ b/drivers/misc/seven_seg_disp.h
@@ -0,0 +1,34 @@
+/*
+ * Copyright (c) 2016 Google, Inc
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 or later as
+ * published by the Free Software Foundation.
+ */
+
+#ifndef SEVEN_SEG_DISP_H
+#define SEVEN_SEG_DISP_H
+
+#include <linux/device.h>
+#include <linux/cdev.h>
+
+#define MAX_DISP_CHAR_SIZE 3
+
+#define DEFAULT_REFRESH_INTERVAL_MS 1000
+
+struct seven_seg_disp_dev {
+	bool disp_data_valid;
+	u16 current_seven_seg_disp_data;
+	char seven_seg_disp_data_array[MAX_DISP_CHAR_SIZE];
+	struct device parent;
+	struct device *dev;
+	struct cdev cdev;
+	void (*update_seven_seg_data)(struct device *, u16 data);
+};
+
+int seven_seg_setup_cdev(struct seven_seg_disp_dev *disp_dev,
+	void (*update_disp_data)(struct device *, u16 data));
+
+void seven_seg_rem_cdev(struct seven_seg_disp_dev *disp_dev);
+
+#endif
--
2.8.0.rc3.226.g39d4020

^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH linux v1 3/4] drivers: misc: Platform driver for seven segment display support
  2016-12-14  7:55 [PATCH linux v1 0/4] Seven segment display support Jaghathiswari Rankappagounder Natarajan
  2016-12-14  7:55 ` [PATCH linux v1 1/4] Documentation: dt-bindings: Document bindings for seven " Jaghathiswari Rankappagounder Natarajan
  2016-12-14  7:55 ` [PATCH linux v1 2/4] drivers: misc: Character device driver for seven segment display Jaghathiswari Rankappagounder Natarajan
@ 2016-12-14  7:55 ` Jaghathiswari Rankappagounder Natarajan
  2016-12-14  7:55 ` [PATCH linux v1 4/4] arm: dts: Add dt-binding to support seven segment display on zaius Jaghathiswari Rankappagounder Natarajan
  2016-12-14 12:45 ` [PATCH linux v1 0/4] Seven segment display support Thomas Petazzoni
  4 siblings, 0 replies; 18+ messages in thread
From: Jaghathiswari Rankappagounder Natarajan @ 2016-12-14  7:55 UTC (permalink / raw)
  To: openbmc, robh+dt, mark.rutland, linux, arnd, gregkh
  Cc: Jaghathiswari Rankappagounder Natarajan, devicetree,
	linux-kernel, linux-arm-kernel, joel

Platform device driver which provides an API for displaying on two
7-segment displays, and implements the required bit-banging.
The hardware assumed is 74HC164 wired to two 7-segment displays.

Signed-off-by: Jaghathiswari Rankappagounder Natarajan <jaghu@google.com>
---
 drivers/misc/Kconfig          |   8 ++
 drivers/misc/Makefile         |   1 +
 drivers/misc/seven_seg_gpio.c | 206 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 215 insertions(+)
 create mode 100644 drivers/misc/seven_seg_gpio.c

diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
index a21aec1..6508108 100644
--- a/drivers/misc/Kconfig
+++ b/drivers/misc/Kconfig
@@ -812,6 +812,14 @@ config PANEL_BOOT_MESSAGE
 	  An empty message will only clear the display at driver init time. Any other
 	  printf()-formatted message is valid with newline and escape codes.

+config SEVEN_SEGMENT_GPIO
+	tristate "Platform driver to update seven segment display"
+	depends on SEVEN_SEGMENT_DISPLAY
+	help
+	  Platform device driver which provides an API for displaying on two
+	  7-segment displays, and implements the required bit-banging.
+	  The hardware assumed is 74HC164 wired to two 7-segment displays.
+
 source "drivers/misc/c2port/Kconfig"
 source "drivers/misc/eeprom/Kconfig"
 source "drivers/misc/cb710/Kconfig"
diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
index c2defbd..d9c0d20 100644
--- a/drivers/misc/Makefile
+++ b/drivers/misc/Makefile
@@ -58,3 +58,4 @@ obj-$(CONFIG_VEXPRESS_SYSCFG)	+= vexpress-syscfg.o
 obj-$(CONFIG_CXL_BASE)		+= cxl/
 obj-$(CONFIG_SEVEN_SEGMENT_DISPLAY)	+= seven_seg_disp.o
 obj-$(CONFIG_PANEL)             += panel.o
+obj-$(CONFIG_SEVEN_SEGMENT_GPIO)	+= seven_seg_gpio.o
diff --git a/drivers/misc/seven_seg_gpio.c b/drivers/misc/seven_seg_gpio.c
new file mode 100644
index 0000000..3dcb903
--- /dev/null
+++ b/drivers/misc/seven_seg_gpio.c
@@ -0,0 +1,206 @@
+/*
+ * Copyright (C) 2016 Google, Inc
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 or later as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/timer.h>
+#include <linux/jiffies.h>
+#include <linux/sizes.h>
+#include <linux/io.h>
+#include <linux/delay.h>
+#include <linux/uaccess.h>
+#include <linux/mutex.h>
+#include <linux/of_platform.h>
+#include <linux/gpio/consumer.h>
+
+#include "seven_seg_disp.h"
+
+#define DELAY_INTVL_US 1
+
+#define CLOCK_GPIO_NAME "clock"
+#define DATA_GPIO_NAME "data"
+#define CLEAR_GPIO_NAME "clear"
+
+struct seven_seg_gpio_info {
+	u16 curr_disp_value;
+	u16 refresh_interval;
+	struct timer_list update_timer;
+	struct gpio_desc *clock_gpio;
+	struct gpio_desc *data_gpio;
+	struct gpio_desc *clear_gpio;
+};
+
+static void update_seven_seg_gpio_data(struct device *dev, u16 data)
+{
+	struct platform_device *pdev;
+	struct seven_seg_gpio_info *gpio_info;
+
+	pdev = container_of(dev, struct platform_device, dev);
+	if (pdev == NULL) {
+		pr_err("invalid NULL platform_device\n");
+		return;
+	}
+
+	gpio_info = platform_get_drvdata(pdev);
+	if (gpio_info == NULL) {
+		pr_err("invalid NULL gpio_info\n");
+		return;
+	}
+
+	gpio_info->curr_disp_value = data;
+}
+
+static void clear_seven_seg_gpio_data(struct device *dev, u16 data)
+{
+	struct platform_device *pdev;
+	struct seven_seg_gpio_info *gpio_info;
+
+	pdev = container_of(dev, struct platform_device, dev);
+	if (pdev == NULL) {
+		pr_err("invalid NULL platform_device\n");
+		return;
+	}
+
+	gpio_info = platform_get_drvdata(pdev);
+	if (gpio_info == NULL) {
+		pr_err("invalid NULL gpio_info\n");
+		return;
+	}
+
+	gpio_info->curr_disp_value = 0;
+}
+
+static void send_seven_seg_gpio_data(u16 disp_data,
+		struct seven_seg_gpio_info *gpio_info)
+{
+	int i;
+
+	gpiod_set_value(gpio_info->clear_gpio, 0);
+	udelay(DELAY_INTVL_US);
+	gpiod_set_value(gpio_info->clear_gpio, 1);
+	udelay(DELAY_INTVL_US);
+
+	for (i = 0; i < 16; i++) {
+		if (disp_data & 0x01)
+			gpiod_set_value(gpio_info->data_gpio, 1);
+		else
+			gpiod_set_value(gpio_info->data_gpio, 0);
+
+		udelay(DELAY_INTVL_US);
+
+		gpiod_set_value(gpio_info->clock_gpio, 0);
+		udelay(DELAY_INTVL_US);
+		gpiod_set_value(gpio_info->clock_gpio, 1);
+		udelay(DELAY_INTVL_US);
+
+		disp_data >>= 1;
+	}
+}
+
+static void disp_refresh_timer_handler(unsigned long data)
+{
+	u16 disp_data;
+	struct seven_seg_gpio_info *gpio_info =
+		(struct seven_seg_gpio_info *)data;
+	disp_data = gpio_info->curr_disp_value;
+
+	send_seven_seg_gpio_data(disp_data, gpio_info);
+	mod_timer(&gpio_info->update_timer,
+		jiffies + msecs_to_jiffies(gpio_info->refresh_interval));
+}
+
+static const struct of_device_id of_seven_seg_gpio_match[] = {
+		{ .compatible = "seven-seg-gpio-dev" },
+		{},
+};
+
+MODULE_DEVICE_TABLE(of, of_seven_seg_gpio_match);
+
+static int seven_seg_gpio_probe(struct platform_device *pdev)
+{
+	u16 interval;
+	int result;
+	struct seven_seg_gpio_info *gpio_info;
+	struct device *dev = &pdev->dev;
+	struct seven_seg_disp_dev *disp_dev;
+
+	gpio_info = devm_kzalloc(dev,
+			sizeof(struct seven_seg_gpio_info),
+			GFP_KERNEL);
+	if (gpio_info == NULL)
+		return -ENOMEM;
+
+	/* Requesting the clock gpio */
+	gpio_info->clock_gpio = devm_gpiod_get(dev, CLOCK_GPIO_NAME,
+		GPIOD_OUT_HIGH);
+	if (IS_ERR(gpio_info->clock_gpio))
+		return PTR_ERR(gpio_info->clock_gpio);
+
+	/* Requesting the data gpio */
+	gpio_info->data_gpio = devm_gpiod_get(dev, DATA_GPIO_NAME,
+		GPIOD_OUT_HIGH);
+	if (IS_ERR(gpio_info->data_gpio))
+		return PTR_ERR(gpio_info->data_gpio);
+
+	/* Requesting the clear gpio */
+	gpio_info->clear_gpio = devm_gpiod_get(dev, CLEAR_GPIO_NAME,
+		GPIOD_OUT_HIGH);
+	if (IS_ERR(gpio_info->clear_gpio))
+		return PTR_ERR(gpio_info->clear_gpio);
+
+	result = of_property_read_u16(pdev->dev.of_node,
+		"refresh-interval-ms", &interval);
+	gpio_info->refresh_interval = result ? DEFAULT_REFRESH_INTERVAL_MS :
+		interval;
+
+	/* Start timer to update seven segment display every second */
+	setup_timer(&gpio_info->update_timer, disp_refresh_timer_handler,
+			(unsigned long)gpio_info);
+	result = mod_timer(&gpio_info->update_timer,
+			jiffies +
+			msecs_to_jiffies(gpio_info->refresh_interval));
+	if (result)
+		return result;
+
+	gpio_info->curr_disp_value = 0;
+
+	platform_set_drvdata(pdev, gpio_info);
+
+	disp_dev = devm_kzalloc(dev, sizeof(struct seven_seg_disp_dev),
+				GFP_KERNEL);
+	disp_dev->parent = *dev;
+	seven_seg_setup_cdev(disp_dev, &update_seven_seg_gpio_data);
+	return 0;
+}
+
+static int seven_seg_gpio_remove(struct platform_device *pdev)
+{
+	struct seven_seg_gpio_info *gpio_info = platform_get_drvdata(pdev);
+	struct seven_seg_disp_dev *disp_dev =
+				container_of(&pdev->dev,
+				struct seven_seg_disp_dev, parent);
+	seven_seg_rem_cdev(disp_dev);
+	del_timer_sync(&gpio_info->update_timer);
+	platform_set_drvdata(pdev, NULL);
+	return 0;
+}
+
+static struct platform_driver seven_seg_gpio_driver = {
+	.probe		= seven_seg_gpio_probe,
+	.remove		= seven_seg_gpio_remove,
+	.driver		= {
+		.name	= "seven-seg-gpio",
+		.of_match_table = of_seven_seg_gpio_match,
+	},
+};
+
+module_platform_driver(seven_seg_gpio_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Jaghathiswari Rankappagounder Natarajan <jaghu@google.com>");
+MODULE_DESCRIPTION("Seven segment display driver using GPIO config");
--
2.8.0.rc3.226.g39d4020

^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH linux v1 4/4] arm: dts: Add dt-binding to support seven segment display on zaius
  2016-12-14  7:55 [PATCH linux v1 0/4] Seven segment display support Jaghathiswari Rankappagounder Natarajan
                   ` (2 preceding siblings ...)
  2016-12-14  7:55 ` [PATCH linux v1 3/4] drivers: misc: Platform driver for seven segment display support Jaghathiswari Rankappagounder Natarajan
@ 2016-12-14  7:55 ` Jaghathiswari Rankappagounder Natarajan
  2016-12-14  8:55   ` Arnd Bergmann
  2016-12-14  9:02   ` Joel Stanley
  2016-12-14 12:45 ` [PATCH linux v1 0/4] Seven segment display support Thomas Petazzoni
  4 siblings, 2 replies; 18+ messages in thread
From: Jaghathiswari Rankappagounder Natarajan @ 2016-12-14  7:55 UTC (permalink / raw)
  To: openbmc, robh+dt, mark.rutland, linux, arnd, gregkh
  Cc: Jaghathiswari Rankappagounder Natarajan, devicetree,
	linux-kernel, linux-arm-kernel, joel

Add clock, data and clear signal GPIO lines to control seven segment display on
zaius platform.

Signed-off-by: Jaghathiswari Rankappagounder Natarajan <jaghu@google.com>
---
 arch/arm/boot/dts/aspeed-bmc-opp-zaius.dts | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/arch/arm/boot/dts/aspeed-bmc-opp-zaius.dts b/arch/arm/boot/dts/aspeed-bmc-opp-zaius.dts
index 8ef4ece..ccb8147 100644
--- a/arch/arm/boot/dts/aspeed-bmc-opp-zaius.dts
+++ b/arch/arm/boot/dts/aspeed-bmc-opp-zaius.dts
@@ -43,6 +43,14 @@
 			gpios = <&gpio ASPEED_GPIO(H, 7) GPIO_ACTIVE_LOW>;
 		};
 	};
+
+	seven-seg-disp {
+		compatible = "seven-seg-gpio-dev";
+		refresh-interval-ms = "1000";
+		clock-gpios = <&gpio ASPEED_GPIO(J, 0) GPIO_ACTIVE_HIGH>;
+		data-gpios = <&gpio ASPEED_GPIO(J, 2) GPIO_ACTIVE_HIGH>;
+		clear-gpios = <&gpio ASPEED_GPIO(J, 1) GPIO_ACTIVE_HIGH>;
+	};
 };

 &fmc {
--
2.8.0.rc3.226.g39d4020

^ permalink raw reply related	[flat|nested] 18+ messages in thread

* Re: [PATCH linux v1 4/4] arm: dts: Add dt-binding to support seven segment display on zaius
  2016-12-14  7:55 ` [PATCH linux v1 4/4] arm: dts: Add dt-binding to support seven segment display on zaius Jaghathiswari Rankappagounder Natarajan
@ 2016-12-14  8:55   ` Arnd Bergmann
  2016-12-14  9:00     ` Arnd Bergmann
  2016-12-14  9:02   ` Joel Stanley
  1 sibling, 1 reply; 18+ messages in thread
From: Arnd Bergmann @ 2016-12-14  8:55 UTC (permalink / raw)
  To: linux-arm-kernel
  Cc: Jaghathiswari Rankappagounder Natarajan, openbmc, robh+dt,
	mark.rutland, linux, gregkh, joel, linux-kernel, devicetree

On Tuesday, December 13, 2016 11:55:04 PM CET Jaghathiswari Rankappagounder Natarajan wrote:
> Add clock, data and clear signal GPIO lines to control seven segment display on
> zaius platform.
> 
> Signed-off-by: Jaghathiswari Rankappagounder Natarajan <jaghu@google.com>
> ---
>  arch/arm/boot/dts/aspeed-bmc-opp-zaius.dts | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/aspeed-bmc-opp-zaius.dts b/arch/arm/boot/dts/aspeed-bmc-opp-zaius.dts
> index 8ef4ece..ccb8147 100644
> --- a/arch/arm/boot/dts/aspeed-bmc-opp-zaius.dts
> +++ b/arch/arm/boot/dts/aspeed-bmc-opp-zaius.dts
> @@ -43,6 +43,14 @@
>  			gpios = <&gpio ASPEED_GPIO(H, 7) GPIO_ACTIVE_LOW>;
>  		};
>  	};
> +
> +	seven-seg-disp {
> +		compatible = "seven-seg-gpio-dev";
> +		refresh-interval-ms = "1000";
> +		clock-gpios = <&gpio ASPEED_GPIO(J, 0) GPIO_ACTIVE_HIGH>;
> +		data-gpios = <&gpio ASPEED_GPIO(J, 2) GPIO_ACTIVE_HIGH>;
> +		clear-gpios = <&gpio ASPEED_GPIO(J, 1) GPIO_ACTIVE_HIGH>;
> +	};
>  };


According to your introductory mail, the interface is assumed to be
a 74HC164. Should we use that ID in the compatible string?

We can always add other strings later if we want to support multiple
wire formats.

	Arnd

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH linux v1 4/4] arm: dts: Add dt-binding to support seven segment display on zaius
  2016-12-14  8:55   ` Arnd Bergmann
@ 2016-12-14  9:00     ` Arnd Bergmann
  2016-12-14 11:06       ` Russell King - ARM Linux
  0 siblings, 1 reply; 18+ messages in thread
From: Arnd Bergmann @ 2016-12-14  9:00 UTC (permalink / raw)
  To: linux-arm-kernel
  Cc: Jaghathiswari Rankappagounder Natarajan, openbmc, robh+dt,
	mark.rutland, linux, gregkh, joel, linux-kernel, devicetree,
	linux-gpio, linus.walleij

On Wednesday, December 14, 2016 9:55:47 AM CET Arnd Bergmann wrote:
> According to your introductory mail, the interface is assumed to be
> a 74HC164. Should we use that ID in the compatible string?
> 
> We can always add other strings later if we want to support multiple
> wire formats.

Actually, looking up 74hc164, that seems to be a gpio expander,
so maybe a more flexible way to do the same is to put a driver
for the expander into drivers/gpio/ and have the main driver
access the outputs of that using the gpiolib interface.

	Arnd

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH linux v1 4/4] arm: dts: Add dt-binding to support seven segment display on zaius
  2016-12-14  7:55 ` [PATCH linux v1 4/4] arm: dts: Add dt-binding to support seven segment display on zaius Jaghathiswari Rankappagounder Natarajan
  2016-12-14  8:55   ` Arnd Bergmann
@ 2016-12-14  9:02   ` Joel Stanley
  1 sibling, 0 replies; 18+ messages in thread
From: Joel Stanley @ 2016-12-14  9:02 UTC (permalink / raw)
  To: Jaghathiswari Rankappagounder Natarajan
  Cc: OpenBMC Maillist, Rob Herring, Mark Rutland, Russell King,
	Arnd Bergmann, gregkh, devicetree, linux-kernel,
	linux-arm-kernel

Hello Jagha,

On Wed, Dec 14, 2016 at 6:25 PM, Jaghathiswari Rankappagounder
Natarajan <jaghu@google.com> wrote:
> Add clock, data and clear signal GPIO lines to control seven segment display on
> zaius platform.
>
> Signed-off-by: Jaghathiswari Rankappagounder Natarajan <jaghu@google.com>

The Zaius device tree is not upstream. I suggest you submit it through
the Aspeed maintainer's tree (me!) for inclusion in the next merge
window.

For the time being, drop this patch from your series as it will not
apply to the upstream kernel.

As a general rule make sure you're basing the patches you send
upstream on a tag from an upstream tree. Linus' v4.9 tag would be the
best one at this point in time.

Cheers,

Joel

> ---
>  arch/arm/boot/dts/aspeed-bmc-opp-zaius.dts | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/arch/arm/boot/dts/aspeed-bmc-opp-zaius.dts b/arch/arm/boot/dts/aspeed-bmc-opp-zaius.dts
> index 8ef4ece..ccb8147 100644
> --- a/arch/arm/boot/dts/aspeed-bmc-opp-zaius.dts
> +++ b/arch/arm/boot/dts/aspeed-bmc-opp-zaius.dts
> @@ -43,6 +43,14 @@
>                         gpios = <&gpio ASPEED_GPIO(H, 7) GPIO_ACTIVE_LOW>;
>                 };
>         };
> +
> +       seven-seg-disp {
> +               compatible = "seven-seg-gpio-dev";
> +               refresh-interval-ms = "1000";
> +               clock-gpios = <&gpio ASPEED_GPIO(J, 0) GPIO_ACTIVE_HIGH>;
> +               data-gpios = <&gpio ASPEED_GPIO(J, 2) GPIO_ACTIVE_HIGH>;
> +               clear-gpios = <&gpio ASPEED_GPIO(J, 1) GPIO_ACTIVE_HIGH>;
> +       };
>  };
>
>  &fmc {
> --
> 2.8.0.rc3.226.g39d4020
>

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH linux v1 4/4] arm: dts: Add dt-binding to support seven segment display on zaius
  2016-12-14  9:00     ` Arnd Bergmann
@ 2016-12-14 11:06       ` Russell King - ARM Linux
  2016-12-14 11:40         ` Russell King - ARM Linux
  0 siblings, 1 reply; 18+ messages in thread
From: Russell King - ARM Linux @ 2016-12-14 11:06 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: linux-arm-kernel, Jaghathiswari Rankappagounder Natarajan,
	openbmc, robh+dt, mark.rutland, gregkh, joel, linux-kernel,
	devicetree, linux-gpio, linus.walleij

On Wed, Dec 14, 2016 at 10:00:46AM +0100, Arnd Bergmann wrote:
> On Wednesday, December 14, 2016 9:55:47 AM CET Arnd Bergmann wrote:
> > According to your introductory mail, the interface is assumed to be
> > a 74HC164. Should we use that ID in the compatible string?
> > 
> > We can always add other strings later if we want to support multiple
> > wire formats.
> 
> Actually, looking up 74hc164, that seems to be a gpio expander,
> so maybe a more flexible way to do the same is to put a driver
> for the expander into drivers/gpio/ and have the main driver
> access the outputs of that using the gpiolib interface.

There already is - drivers/gpio/gpio-74x164.c

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH linux v1 4/4] arm: dts: Add dt-binding to support seven segment display on zaius
  2016-12-14 11:06       ` Russell King - ARM Linux
@ 2016-12-14 11:40         ` Russell King - ARM Linux
  2016-12-15 23:07           ` Linus Walleij
  0 siblings, 1 reply; 18+ messages in thread
From: Russell King - ARM Linux @ 2016-12-14 11:40 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: mark.rutland, Jaghathiswari Rankappagounder Natarajan,
	devicetree, gregkh, openbmc, linux-kernel, linux-gpio, robh+dt,
	joel, linus.walleij, linux-arm-kernel

On Wed, Dec 14, 2016 at 11:06:35AM +0000, Russell King - ARM Linux wrote:
> On Wed, Dec 14, 2016 at 10:00:46AM +0100, Arnd Bergmann wrote:
> > On Wednesday, December 14, 2016 9:55:47 AM CET Arnd Bergmann wrote:
> > > According to your introductory mail, the interface is assumed to be
> > > a 74HC164. Should we use that ID in the compatible string?
> > > 
> > > We can always add other strings later if we want to support multiple
> > > wire formats.
> > 
> > Actually, looking up 74hc164, that seems to be a gpio expander,
> > so maybe a more flexible way to do the same is to put a driver
> > for the expander into drivers/gpio/ and have the main driver
> > access the outputs of that using the gpiolib interface.
> 
> There already is - drivers/gpio/gpio-74x164.c

Looking at this more, it's a SPI driver, presumably because the first
case where it appeared was on a SPI bus.

However, it's not a SPI device as such, it's a piece of standard,
general purpose logic that's been around for many years, pre-dating
the SPI bus.

Now, as for DT, we have this "DT represents the hardware, not the
implementation" edict, which now brings up an interesting problem.

If we want to use this driver in its existing form, we need to:

- declare in DT a spi-gpio driver to provide a SPI bus on the GPIO
  pins connected to the 74HC164.
- attach the 74HC164 to the SPI bus.

The problem with that is it's not representative of the hardware -
what we're saying is that we want to reuse our existing implementation
and make DT conform to the implementation.  At that point, we might as
well scrap our "DT is implementation independent" edict above.

What if, tomorrow, we end up with 74HC164 connected to via a different
method?

I think a much more sensible approach would be to turn the GPIO side
of the 74x164 driver into a library, which can be re-used by multiple
bus-specific drivers - one for SPI which allows it to be used in its
current form, one for our platform bus which takes the GPIO lines for
the data, clock and clear signals.

I also don't see why they shouldn't use the same compatible - they're
the same _device_ at the end of the day, just wired up differently.
It makes the binding documentation a little fun wrt what are required
and optional properties, but nothing that shouldn't be too difficult.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH linux v1 2/4] drivers: misc: Character device driver for seven segment display
  2016-12-14  7:55 ` [PATCH linux v1 2/4] drivers: misc: Character device driver for seven segment display Jaghathiswari Rankappagounder Natarajan
@ 2016-12-14 12:32   ` Russell King - ARM Linux
  0 siblings, 0 replies; 18+ messages in thread
From: Russell King - ARM Linux @ 2016-12-14 12:32 UTC (permalink / raw)
  To: Jaghathiswari Rankappagounder Natarajan
  Cc: openbmc, robh+dt, mark.rutland, arnd, gregkh, devicetree,
	linux-kernel, linux-arm-kernel, joel

On Tue, Dec 13, 2016 at 11:55:02PM -0800, Jaghathiswari Rankappagounder Natarajan wrote:
> +int seven_seg_setup_cdev(struct seven_seg_disp_dev *disp_dev,
> +	void (*update_disp_data)(struct device *, u16 data))
> +{
> +	struct device *dev;
> +	int err;
> +
> +	dev = device_create(seven_seg_disp_class, &disp_dev->parent,
> +			seven_seg_devno,
> +			NULL, "seven_seg_disp_val");
> +	if (dev == NULL)
> +		return -1;

Do not use return -1 in kernel code.

> +	disp_dev->dev = dev;
> +	disp_dev->update_seven_seg_data = update_disp_data;
> +	disp_dev->disp_data_valid = false;
> +
> +	cdev_init(&disp_dev->cdev, &seven_seg_disp_fops);
> +	err = cdev_add(&disp_dev->cdev, seven_seg_devno, 1);
> +	if (err)
> +		device_destroy(seven_seg_disp_class, seven_seg_devno);
> +	return err;
> +}
> +
> +static int __init seven_seg_disp_init(void)
> +{
> +	if (alloc_chrdev_region(&seven_seg_devno, 0, 1, "disp_state") < 0)
> +		return -1;

Do not use return -1 in kernel code.

> +
> +	seven_seg_disp_class = class_create(THIS_MODULE, "disp_state");
> +	if (seven_seg_disp_class == NULL)
> +		goto unreg_chrdev;
> +
> +unreg_chrdev:
> +	unregister_chrdev_region(seven_seg_devno, 1);
> +	return -1;

Do not use return -1 in kernel code.

(Look up what an errno value of '1' means.  Negative values returned from
functions are interpreted as negated errno values.)

Always propagate error codes, or select an appropriate errno value to
return.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH linux v1 0/4] Seven segment display support
  2016-12-14  7:55 [PATCH linux v1 0/4] Seven segment display support Jaghathiswari Rankappagounder Natarajan
                   ` (3 preceding siblings ...)
  2016-12-14  7:55 ` [PATCH linux v1 4/4] arm: dts: Add dt-binding to support seven segment display on zaius Jaghathiswari Rankappagounder Natarajan
@ 2016-12-14 12:45 ` Thomas Petazzoni
  2016-12-14 12:56   ` Greg KH
  4 siblings, 1 reply; 18+ messages in thread
From: Thomas Petazzoni @ 2016-12-14 12:45 UTC (permalink / raw)
  To: Jaghathiswari Rankappagounder Natarajan
  Cc: openbmc, robh+dt, mark.rutland, linux, arnd, gregkh, joel,
	linux-kernel, linux-arm-kernel, devicetree

Hello,

On Tue, 13 Dec 2016 23:55:00 -0800, Jaghathiswari Rankappagounder
Natarajan wrote:

> Documentation for the binding which provides an interface for adding clock,
> data and clear signal GPIO lines to control seven segment display.
> 
> The platform device driver provides an API for displaying on two 7-segment
> displays, and implements the required bit-banging. The hardware assumed is
> 74HC164 wired to two 7-segment displays.
> 
> The character device driver implements the user-space API for letting a user
> write to two 7-segment displays including any conversion methods necessary
> to map the user input to two 7-segment displays.
> 
> Adding clock, data and clear signal GPIO lines in the devicetree to control
> seven segment display on zaius platform.
> 
> The platform driver matches on the device tree node; the platform driver also
> initializes the character device.
> 
> Tested that the seven segment display works properly by writing to the
> character device file on a EVB AST2500 board which also has 74HC164 wired
> to two 7-segment displays.

FWIW, I proposed a driver for seven segment displays back in 2013:

   http://lists.infradead.org/pipermail/linux-arm-kernel/2013-January/139986.html

And the feedback from Greg KH was: we don't need a driver for that, do
it from userspace. See:

   http://lists.infradead.org/pipermail/linux-arm-kernel/2013-January/139992.html

So: good luck :-)

Best regards,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH linux v1 0/4] Seven segment display support
  2016-12-14 12:45 ` [PATCH linux v1 0/4] Seven segment display support Thomas Petazzoni
@ 2016-12-14 12:56   ` Greg KH
  2016-12-14 13:12     ` Neil Armstrong
  0 siblings, 1 reply; 18+ messages in thread
From: Greg KH @ 2016-12-14 12:56 UTC (permalink / raw)
  To: Thomas Petazzoni
  Cc: Jaghathiswari Rankappagounder Natarajan, openbmc, robh+dt,
	mark.rutland, linux, arnd, joel, linux-kernel, linux-arm-kernel,
	devicetree

On Wed, Dec 14, 2016 at 01:45:30PM +0100, Thomas Petazzoni wrote:
> Hello,
> 
> On Tue, 13 Dec 2016 23:55:00 -0800, Jaghathiswari Rankappagounder
> Natarajan wrote:
> 
> > Documentation for the binding which provides an interface for adding clock,
> > data and clear signal GPIO lines to control seven segment display.
> > 
> > The platform device driver provides an API for displaying on two 7-segment
> > displays, and implements the required bit-banging. The hardware assumed is
> > 74HC164 wired to two 7-segment displays.
> > 
> > The character device driver implements the user-space API for letting a user
> > write to two 7-segment displays including any conversion methods necessary
> > to map the user input to two 7-segment displays.
> > 
> > Adding clock, data and clear signal GPIO lines in the devicetree to control
> > seven segment display on zaius platform.
> > 
> > The platform driver matches on the device tree node; the platform driver also
> > initializes the character device.
> > 
> > Tested that the seven segment display works properly by writing to the
> > character device file on a EVB AST2500 board which also has 74HC164 wired
> > to two 7-segment displays.
> 
> FWIW, I proposed a driver for seven segment displays back in 2013:
> 
>    http://lists.infradead.org/pipermail/linux-arm-kernel/2013-January/139986.html
> 
> And the feedback from Greg KH was: we don't need a driver for that, do
> it from userspace. See:
> 
>    http://lists.infradead.org/pipermail/linux-arm-kernel/2013-January/139992.html
> 
> So: good luck :-)

Did anyone ever write a library for this type of thing?

Again, I don't want to see one-off drivers for random devices like this
that should be able to all be controlled from userspace in a common
manner.  Much like we did for fingerprint readers a long long time
ago...

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH linux v1 0/4] Seven segment display support
  2016-12-14 12:56   ` Greg KH
@ 2016-12-14 13:12     ` Neil Armstrong
  2016-12-14 14:15       ` Arnd Bergmann
  2016-12-14 16:50       ` Greg KH
  0 siblings, 2 replies; 18+ messages in thread
From: Neil Armstrong @ 2016-12-14 13:12 UTC (permalink / raw)
  To: Greg KH, Thomas Petazzoni
  Cc: mark.rutland, Jaghathiswari Rankappagounder Natarajan, arnd,
	devicetree, openbmc, linux, linux-kernel, robh+dt, joel,
	linux-arm-kernel

On 12/14/2016 01:56 PM, Greg KH wrote:
> On Wed, Dec 14, 2016 at 01:45:30PM +0100, Thomas Petazzoni wrote:
>> Hello,
>>
>> On Tue, 13 Dec 2016 23:55:00 -0800, Jaghathiswari Rankappagounder
>> Natarajan wrote:
>>
>>> Documentation for the binding which provides an interface for adding clock,
>>> data and clear signal GPIO lines to control seven segment display.
>>>
>>> The platform device driver provides an API for displaying on two 7-segment
>>> displays, and implements the required bit-banging. The hardware assumed is
>>> 74HC164 wired to two 7-segment displays.
>>>
>>> The character device driver implements the user-space API for letting a user
>>> write to two 7-segment displays including any conversion methods necessary
>>> to map the user input to two 7-segment displays.
>>>
>>> Adding clock, data and clear signal GPIO lines in the devicetree to control
>>> seven segment display on zaius platform.
>>>
>>> The platform driver matches on the device tree node; the platform driver also
>>> initializes the character device.
>>>
>>> Tested that the seven segment display works properly by writing to the
>>> character device file on a EVB AST2500 board which also has 74HC164 wired
>>> to two 7-segment displays.
>>
>> FWIW, I proposed a driver for seven segment displays back in 2013:
>>
>>    http://lists.infradead.org/pipermail/linux-arm-kernel/2013-January/139986.html
>>
>> And the feedback from Greg KH was: we don't need a driver for that, do
>> it from userspace. See:
>>
>>    http://lists.infradead.org/pipermail/linux-arm-kernel/2013-January/139992.html
>>
>> So: good luck :-)
> 
> Did anyone ever write a library for this type of thing?
> 
> Again, I don't want to see one-off drivers for random devices like this
> that should be able to all be controlled from userspace in a common
> manner.  Much like we did for fingerprint readers a long long time
> ago...
> 
> thanks,
> 
> greg k-h

Hi Greg,

Actually, it's more than a random interface, a lot of SoCs and boards actually have such displays
and it's a pity to use UIO, sysfs gpio bitbanging and all sort of ugly stuff to only print a few
characters a simple and clean driver could achieve.
Some very well known SoCs even have integrated registers to lower the BOM and bypass the need for
a 74HC164 like component and avoid gpio bit banging.

My personal concern is that you could also need to drive more segments, thus 7-segments
is too restrictive.

But this driver is well structured, the gpio-bitbanging sub-driver is welcome.

Neil

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH linux v1 0/4] Seven segment display support
  2016-12-14 13:12     ` Neil Armstrong
@ 2016-12-14 14:15       ` Arnd Bergmann
  2016-12-14 20:05         ` David Daney
  2016-12-14 16:50       ` Greg KH
  1 sibling, 1 reply; 18+ messages in thread
From: Arnd Bergmann @ 2016-12-14 14:15 UTC (permalink / raw)
  To: Neil Armstrong
  Cc: Greg KH, Thomas Petazzoni, mark.rutland,
	Jaghathiswari Rankappagounder Natarajan, devicetree, openbmc,
	linux, linux-kernel, robh+dt, joel, linux-arm-kernel

On Wednesday, December 14, 2016 2:12:41 PM CET Neil Armstrong wrote:
> On 12/14/2016 01:56 PM, Greg KH wrote:
> > On Wed, Dec 14, 2016 at 01:45:30PM +0100, Thomas Petazzoni wrote:
> >> Hello,
> >>
> >> On Tue, 13 Dec 2016 23:55:00 -0800, Jaghathiswari Rankappagounder
> >> Natarajan wrote:
> >>
> >>> Documentation for the binding which provides an interface for adding clock,
> >>> data and clear signal GPIO lines to control seven segment display.
> >>>
> >>> The platform device driver provides an API for displaying on two 7-segment
> >>> displays, and implements the required bit-banging. The hardware assumed is
> >>> 74HC164 wired to two 7-segment displays.
> >>>
> >>> The character device driver implements the user-space API for letting a user
> >>> write to two 7-segment displays including any conversion methods necessary
> >>> to map the user input to two 7-segment displays.
> >>>
> >>> Adding clock, data and clear signal GPIO lines in the devicetree to control
> >>> seven segment display on zaius platform.
> >>>
> >>> The platform driver matches on the device tree node; the platform driver also
> >>> initializes the character device.
> >>>
> >>> Tested that the seven segment display works properly by writing to the
> >>> character device file on a EVB AST2500 board which also has 74HC164 wired
> >>> to two 7-segment displays.
> >>
> >> FWIW, I proposed a driver for seven segment displays back in 2013:
> >>
> >>    http://lists.infradead.org/pipermail/linux-arm-kernel/2013-January/139986.html
> >>
> >> And the feedback from Greg KH was: we don't need a driver for that, do
> >> it from userspace. See:
> >>
> >>    http://lists.infradead.org/pipermail/linux-arm-kernel/2013-January/139992.html
> >>
> >> So: good luck 
> > 
> > Did anyone ever write a library for this type of thing?
> > 
> > Again, I don't want to see one-off drivers for random devices like this
> > that should be able to all be controlled from userspace in a common
> > manner.  Much like we did for fingerprint readers a long long time
> > ago...
> > 

> Actually, it's more than a random interface, a lot of SoCs and boards actually have such displays
> and it's a pity to use UIO, sysfs gpio bitbanging and all sort of ugly stuff to only print a few
> characters a simple and clean driver could achieve.
> Some very well known SoCs even have integrated registers to lower the BOM and bypass the need for
> a 74HC164 like component and avoid gpio bit banging.
> 
> My personal concern is that you could also need to drive more segments, thus 7-segments
> is too restrictive.
> 
> But this driver is well structured, the gpio-bitbanging sub-driver is welcome.

Maybe we can find a way to fit this into the existing drivers/leds/ subsystem?

That already supports blinking, brightness and colour attributes of LEDs,
so could this be extended to support (one of) digit, number, character
or string with a common sysfs attribute and a way to hook up a led driver
to that?

	Arnd

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH linux v1 0/4] Seven segment display support
  2016-12-14 13:12     ` Neil Armstrong
  2016-12-14 14:15       ` Arnd Bergmann
@ 2016-12-14 16:50       ` Greg KH
  1 sibling, 0 replies; 18+ messages in thread
From: Greg KH @ 2016-12-14 16:50 UTC (permalink / raw)
  To: Neil Armstrong
  Cc: Thomas Petazzoni, mark.rutland,
	Jaghathiswari Rankappagounder Natarajan, arnd, devicetree,
	openbmc, linux, linux-kernel, robh+dt, joel, linux-arm-kernel

On Wed, Dec 14, 2016 at 02:12:41PM +0100, Neil Armstrong wrote:
> On 12/14/2016 01:56 PM, Greg KH wrote:
> > On Wed, Dec 14, 2016 at 01:45:30PM +0100, Thomas Petazzoni wrote:
> >> Hello,
> >>
> >> On Tue, 13 Dec 2016 23:55:00 -0800, Jaghathiswari Rankappagounder
> >> Natarajan wrote:
> >>
> >>> Documentation for the binding which provides an interface for adding clock,
> >>> data and clear signal GPIO lines to control seven segment display.
> >>>
> >>> The platform device driver provides an API for displaying on two 7-segment
> >>> displays, and implements the required bit-banging. The hardware assumed is
> >>> 74HC164 wired to two 7-segment displays.
> >>>
> >>> The character device driver implements the user-space API for letting a user
> >>> write to two 7-segment displays including any conversion methods necessary
> >>> to map the user input to two 7-segment displays.
> >>>
> >>> Adding clock, data and clear signal GPIO lines in the devicetree to control
> >>> seven segment display on zaius platform.
> >>>
> >>> The platform driver matches on the device tree node; the platform driver also
> >>> initializes the character device.
> >>>
> >>> Tested that the seven segment display works properly by writing to the
> >>> character device file on a EVB AST2500 board which also has 74HC164 wired
> >>> to two 7-segment displays.
> >>
> >> FWIW, I proposed a driver for seven segment displays back in 2013:
> >>
> >>    http://lists.infradead.org/pipermail/linux-arm-kernel/2013-January/139986.html
> >>
> >> And the feedback from Greg KH was: we don't need a driver for that, do
> >> it from userspace. See:
> >>
> >>    http://lists.infradead.org/pipermail/linux-arm-kernel/2013-January/139992.html
> >>
> >> So: good luck :-)
> > 
> > Did anyone ever write a library for this type of thing?
> > 
> > Again, I don't want to see one-off drivers for random devices like this
> > that should be able to all be controlled from userspace in a common
> > manner.  Much like we did for fingerprint readers a long long time
> > ago...
> > 
> > thanks,
> > 
> > greg k-h
> 
> Hi Greg,
> 
> Actually, it's more than a random interface, a lot of SoCs and boards actually have such displays
> and it's a pity to use UIO, sysfs gpio bitbanging and all sort of ugly stuff to only print a few
> characters a simple and clean driver could achieve.

Great, then let's make an API that all devices of this type could use,
and not just take individual drivers that all have a custom char or
sysfs interface which requires custom userspace code to be able to drive
all of the different devices in a common way (i.e. a library would have
to be written anyways...)

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH linux v1 0/4] Seven segment display support
  2016-12-14 14:15       ` Arnd Bergmann
@ 2016-12-14 20:05         ` David Daney
  0 siblings, 0 replies; 18+ messages in thread
From: David Daney @ 2016-12-14 20:05 UTC (permalink / raw)
  To: Arnd Bergmann, Neil Armstrong
  Cc: Thomas Petazzoni, mark.rutland,
	Jaghathiswari Rankappagounder Natarajan, devicetree, Greg KH,
	openbmc, linux, linux-kernel, robh+dt, joel, linux-arm-kernel

On 12/14/2016 06:15 AM, Arnd Bergmann wrote:
> On Wednesday, December 14, 2016 2:12:41 PM CET Neil Armstrong wrote:
>> On 12/14/2016 01:56 PM, Greg KH wrote:
>>> On Wed, Dec 14, 2016 at 01:45:30PM +0100, Thomas Petazzoni wrote:
>>>> Hello,
>>>>
>>>> On Tue, 13 Dec 2016 23:55:00 -0800, Jaghathiswari Rankappagounder
>>>> Natarajan wrote:
>>>>
>>>>> Documentation for the binding which provides an interface for adding clock,
>>>>> data and clear signal GPIO lines to control seven segment display.
>>>>>
>>>>> The platform device driver provides an API for displaying on two 7-segment
>>>>> displays, and implements the required bit-banging. The hardware assumed is
>>>>> 74HC164 wired to two 7-segment displays.
>>>>>
>>>>> The character device driver implements the user-space API for letting a user
>>>>> write to two 7-segment displays including any conversion methods necessary
>>>>> to map the user input to two 7-segment displays.
>>>>>
>>>>> Adding clock, data and clear signal GPIO lines in the devicetree to control
>>>>> seven segment display on zaius platform.
>>>>>
>>>>> The platform driver matches on the device tree node; the platform driver also
>>>>> initializes the character device.
>>>>>
>>>>> Tested that the seven segment display works properly by writing to the
>>>>> character device file on a EVB AST2500 board which also has 74HC164 wired
>>>>> to two 7-segment displays.
>>>>
>>>> FWIW, I proposed a driver for seven segment displays back in 2013:
>>>>
>>>>    http://lists.infradead.org/pipermail/linux-arm-kernel/2013-January/139986.html
>>>>
>>>> And the feedback from Greg KH was: we don't need a driver for that, do
>>>> it from userspace. See:
>>>>
>>>>    http://lists.infradead.org/pipermail/linux-arm-kernel/2013-January/139992.html
>>>>
>>>> So: good luck
>>>
>>> Did anyone ever write a library for this type of thing?
>>>
>>> Again, I don't want to see one-off drivers for random devices like this
>>> that should be able to all be controlled from userspace in a common
>>> manner.  Much like we did for fingerprint readers a long long time
>>> ago...
>>>
>
>> Actually, it's more than a random interface, a lot of SoCs and boards actually have such displays
>> and it's a pity to use UIO, sysfs gpio bitbanging and all sort of ugly stuff to only print a few
>> characters a simple and clean driver could achieve.
>> Some very well known SoCs even have integrated registers to lower the BOM and bypass the need for
>> a 74HC164 like component and avoid gpio bit banging.
>>
>> My personal concern is that you could also need to drive more segments, thus 7-segments
>> is too restrictive.
>>
>> But this driver is well structured, the gpio-bitbanging sub-driver is welcome.
>
> Maybe we can find a way to fit this into the existing drivers/leds/ subsystem?
>
> That already supports blinking, brightness and colour attributes of LEDs,
> so could this be extended to support (one of) digit, number, character
> or string with a common sysfs attribute and a way to hook up a led driver
> to that?

We have a lot of boards with an 8-cell dot matrix LED. Each cell is 
programmed with an 8-bit value.  The mapping of these values to the dots 
defaults to ASCII character rendering, but there is the facility to 
install other bitmaps as well.

Really I view these things not as part of the LED subsystem, but more as 
a very small frame buffer.

We like to display entire words, and the most useful interface from a 
user point of view is something that consumes entire strings rather than 
having to manage each cell independently.

You could imagine that if the text to be displayed were longer than the 
display, that the driver would make it continuously scroll.  I would 
like to see a framework where a simple character device were exposed, 
and from userspace you could do: "echo message > /dev/small-display" and 
get something sensible.

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

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH linux v1 4/4] arm: dts: Add dt-binding to support seven segment display on zaius
  2016-12-14 11:40         ` Russell King - ARM Linux
@ 2016-12-15 23:07           ` Linus Walleij
  0 siblings, 0 replies; 18+ messages in thread
From: Linus Walleij @ 2016-12-15 23:07 UTC (permalink / raw)
  To: Russell King - ARM Linux
  Cc: Arnd Bergmann, Mark Rutland,
	Jaghathiswari Rankappagounder Natarajan, devicetree, Greg KH,
	OpenBMC Maillist, linux-kernel, linux-gpio, Rob Herring,
	Joel Stanley, linux-arm-kernel

On Wed, Dec 14, 2016 at 12:40 PM, Russell King - ARM Linux
<linux@armlinux.org.uk> wrote:

> Looking at this more, it's a SPI driver, presumably because the first
> case where it appeared was on a SPI bus.
>
> However, it's not a SPI device as such, it's a piece of standard,
> general purpose logic that's been around for many years, pre-dating
> the SPI bus.

Indeed.

> I think a much more sensible approach would be to turn the GPIO side
> of the 74x164 driver into a library, which can be re-used by multiple
> bus-specific drivers - one for SPI which allows it to be used in its
> current form, one for our platform bus which takes the GPIO lines for
> the data, clock and clear signals.
>
> I also don't see why they shouldn't use the same compatible - they're
> the same _device_ at the end of the day, just wired up differently.
> It makes the binding documentation a little fun wrt what are required
> and optional properties, but nothing that shouldn't be too difficult.

I agree on both accounts.

Sorry for not seeing this in the first place, I was well aware that this
is a standard component and may be connected in a myriad of ways,
so I should have known better :(

Yours,
Linus Walleij

^ permalink raw reply	[flat|nested] 18+ messages in thread

end of thread, other threads:[~2016-12-15 23:07 UTC | newest]

Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-12-14  7:55 [PATCH linux v1 0/4] Seven segment display support Jaghathiswari Rankappagounder Natarajan
2016-12-14  7:55 ` [PATCH linux v1 1/4] Documentation: dt-bindings: Document bindings for seven " Jaghathiswari Rankappagounder Natarajan
2016-12-14  7:55 ` [PATCH linux v1 2/4] drivers: misc: Character device driver for seven segment display Jaghathiswari Rankappagounder Natarajan
2016-12-14 12:32   ` Russell King - ARM Linux
2016-12-14  7:55 ` [PATCH linux v1 3/4] drivers: misc: Platform driver for seven segment display support Jaghathiswari Rankappagounder Natarajan
2016-12-14  7:55 ` [PATCH linux v1 4/4] arm: dts: Add dt-binding to support seven segment display on zaius Jaghathiswari Rankappagounder Natarajan
2016-12-14  8:55   ` Arnd Bergmann
2016-12-14  9:00     ` Arnd Bergmann
2016-12-14 11:06       ` Russell King - ARM Linux
2016-12-14 11:40         ` Russell King - ARM Linux
2016-12-15 23:07           ` Linus Walleij
2016-12-14  9:02   ` Joel Stanley
2016-12-14 12:45 ` [PATCH linux v1 0/4] Seven segment display support Thomas Petazzoni
2016-12-14 12:56   ` Greg KH
2016-12-14 13:12     ` Neil Armstrong
2016-12-14 14:15       ` Arnd Bergmann
2016-12-14 20:05         ` David Daney
2016-12-14 16:50       ` Greg KH

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).