All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH linux v2 1/3] drivers: misc: Character driver for seven segment display
@ 2016-10-17 17:10 Jaghathiswari Rankappagounder Natarajan
  2016-10-17 17:10 ` [PATCH linux v2 2/3] drivers: misc: Platform " Jaghathiswari Rankappagounder Natarajan
                   ` (2 more replies)
  0 siblings, 3 replies; 5+ messages in thread
From: Jaghathiswari Rankappagounder Natarajan @ 2016-10-17 17:10 UTC (permalink / raw)
  To: openbmc; +Cc: joel, raltherr, xow, Jaghathiswari Rankappagounder Natarajan

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

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

diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
index 4617ddc..cf07817 100644
--- a/drivers/misc/Kconfig
+++ b/drivers/misc/Kconfig
@@ -804,6 +804,11 @@ 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_DISPLAY
+	tristate "Character driver for seven segment display support"
+	help
+	  Character driver for seven segment display support
+
 config ASPEED_BT_IPMI_HOST
 	tristate "BT IPMI host driver"
 	help
diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
index 724861b..718dc2b 100644
--- a/drivers/misc/Makefile
+++ b/drivers/misc/Makefile
@@ -57,4 +57,5 @@ obj-$(CONFIG_ECHO)		+= echo/
 obj-$(CONFIG_VEXPRESS_SYSCFG)	+= vexpress-syscfg.o
 obj-$(CONFIG_CXL_BASE)		+= cxl/
 obj-$(CONFIG_PANEL)             += panel.o
+obj-$(CONFIG_SEVEN_SEGMENT_DISPLAY)	+= seven_seg_disp.o
 obj-$(CONFIG_ASPEED_BT_IPMI_HOST)	+= bt-host.o
diff --git a/drivers/misc/seven_seg_disp.c b/drivers/misc/seven_seg_disp.c
new file mode 100644
index 0000000..88df4a0
--- /dev/null
+++ b/drivers/misc/seven_seg_disp.c
@@ -0,0 +1,192 @@
+/*
+ * Seven segment display character driver
+ * * 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 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 "seven_seg_gen.h"
+
+#define MAX_DISP_CHAR_SIZE 3
+
+#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)
+{
+	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)
+{
+	char tmp[MAX_DISP_CHAR_SIZE];
+	int length = len - 1;
+	int i;
+	u8 result;
+
+	struct seven_seg_disp_dev *disp_dev;
+
+	if (length != MAX_DISP_CHAR_SIZE) {
+		return -EINVAL;
+	}
+
+	if (copy_from_user(tmp, buf, length) != 0) {
+		return -EFAULT;
+	}
+
+	for (i = 0; i < MAX_DISP_CHAR_SIZE; i++) {
+		if (!isxdigit(tmp[i]))
+			return -EINVAL;
+	}
+
+	result = convert_to_disp_data(tmp);
+
+	disp_dev = filp->private_data;
+	disp_dev->update_seven_seg_data(&disp_dev->parent, result);
+
+	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 rem_cdev(struct seven_seg_disp_dev *disp_dev)
+{
+	cdev_del(&disp_dev->cdev);
+	device_destroy(seven_seg_disp_class, seven_seg_devno);
+}
+
+int setup_cdev(struct device parent,
+	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, &parent, seven_seg_devno,
+			NULL, "seven_seg_disp_val");
+	if (dev == NULL)
+		return -1;
+	disp_dev->parent = parent;
+	disp_dev->dev = dev;
+	disp_dev->update_seven_seg_data = update_disp_data;
+	cdev_init(&disp_dev->cdev, &seven_seg_disp_fops);
+	disp_dev->cdev.ops = &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 unregister;
+		return -1;
+	}
+
+unregister:
+	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_gen.h b/drivers/misc/seven_seg_gen.h
new file mode 100644
index 0000000..5748a18
--- /dev/null
+++ b/drivers/misc/seven_seg_gen.h
@@ -0,0 +1,32 @@
+/*
+ * Seven segment display support
+ * * 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 as
+ * * published by the Free Software Foundation.
+ *
+ */
+
+#ifndef SEVEN_SEG_H
+#define SEVEN_SEG_H
+
+#include <linux/device.h>
+#include <linux/cdev.h>
+
+#define DEFAULT_REFRESH_INTERVAL 1000
+
+struct seven_seg_disp_dev {
+	struct device parent;
+	struct device *dev;
+	struct cdev cdev;
+	void (*update_seven_seg_data)(struct device *, u16 data);
+};
+
+int setup_cdev(struct device parent,
+	struct seven_seg_disp_dev *disp_dev,
+	void (*update_disp_data)(struct device *, u16 data));
+
+void rem_cdev(struct seven_seg_disp_dev *disp_dev);
+
+#endif
--
2.8.0.rc3.226.g39d4020

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

* [PATCH linux v2 2/3] drivers: misc: Platform driver for seven segment display
  2016-10-17 17:10 [PATCH linux v2 1/3] drivers: misc: Character driver for seven segment display Jaghathiswari Rankappagounder Natarajan
@ 2016-10-17 17:10 ` Jaghathiswari Rankappagounder Natarajan
  2016-10-17 17:10 ` [PATCH linux v2 3/3] devicetree: Add devicetree changes to support seven segment display on zaius Jaghathiswari Rankappagounder Natarajan
  2016-10-17 19:55 ` [PATCH linux v2 1/3] drivers: misc: Character driver for seven segment display Rick Altherr
  2 siblings, 0 replies; 5+ messages in thread
From: Jaghathiswari Rankappagounder Natarajan @ 2016-10-17 17:10 UTC (permalink / raw)
  To: openbmc; +Cc: joel, raltherr, xow, Jaghathiswari Rankappagounder Natarajan

Platform driver provides an API for displaying on a 7-segment display,
and implements the required bit-banging.

Signed-off-by: Jaghathiswari Rankappagounder Natarajan <jaghu@google.com>
---
 .../bindings/misc/seven_segment_disp.txt           |  23 +++
 drivers/misc/Kconfig                               |   7 +
 drivers/misc/Makefile                              |   1 +
 drivers/misc/seven_seg_gpio.c                      | 202 +++++++++++++++++++++
 4 files changed, 233 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/misc/seven_segment_disp.txt
 create mode 100644 drivers/misc/seven_seg_gpio.c

diff --git a/Documentation/devicetree/bindings/misc/seven_segment_disp.txt b/Documentation/devicetree/bindings/misc/seven_segment_disp.txt
new file mode 100644
index 0000000..9c01b97
--- /dev/null
+++ b/Documentation/devicetree/bindings/misc/seven_segment_disp.txt
@@ -0,0 +1,23 @@
+Seven segment display support connected to GPIO lines
+
+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>;
+};
diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
index cf07817..7adb9e3 100644
--- a/drivers/misc/Kconfig
+++ b/drivers/misc/Kconfig
@@ -814,6 +814,13 @@ config ASPEED_BT_IPMI_HOST
 	help
 	  Support for the Aspeed BT ipmi host.

+config SEVEN_SEGMENT_GPIO
+	tristate "Platform driver to update seven segment display"
+	depends on SEVEN_SEGMENT_DISPLAY
+	help
+	  Support to periodically update display on two seven segment displays.
+	  The hardware assumed is 74HC164 wired to two seven 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 718dc2b..71e364d 100644
--- a/drivers/misc/Makefile
+++ b/drivers/misc/Makefile
@@ -59,3 +59,4 @@ obj-$(CONFIG_CXL_BASE)		+= cxl/
 obj-$(CONFIG_PANEL)             += panel.o
 obj-$(CONFIG_SEVEN_SEGMENT_DISPLAY)	+= seven_seg_disp.o
 obj-$(CONFIG_ASPEED_BT_IPMI_HOST)	+= bt-host.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..f64f280
--- /dev/null
+++ b/drivers/misc/seven_seg_gpio.c
@@ -0,0 +1,202 @@
+/*
+ * Platform driver to periodically update two 7-segment LED
+ *  * displays. Hardware assumed is 74HC164 shift register wired to two
+ *  * 7-segment LED displays.
+ *  *
+ *  * 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 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_gen.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 {
+	u8 disp_data_valid;
+	u16 curr_disp_value;
+	u16 refresh_interval;
+	struct mutex mutex;
+	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 vpu_subdev_data\n");
+		return;
+	}
+
+	mutex_lock(&gpio_info->mutex);
+	gpio_info->curr_disp_value = data;
+	gpio_info->disp_data_valid = 1;
+	mutex_unlock(&gpio_info->mutex);
+
+}
+
+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)
+{
+	u8 valid = 0;
+	u16 disp_data;
+	struct seven_seg_gpio_info *gpio_info =
+		(struct seven_seg_gpio_info *)data;
+	mutex_lock(&gpio_info->mutex);
+	if (gpio_info->disp_data_valid) {
+		valid = 1;
+		disp_data = gpio_info->curr_disp_value;
+	}
+	mutex_unlock(&gpio_info->mutex);
+
+	if (valid)
+		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 :
+		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;
+
+	mutex_init(&gpio_info->mutex);
+
+	platform_set_drvdata(pdev, gpio_info);
+
+	disp_dev = devm_kzalloc(dev, sizeof(struct seven_seg_disp_dev),
+			GFP_KERNEL);
+	setup_cdev(*dev, 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);
+	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] 5+ messages in thread

* [PATCH linux v2 3/3] devicetree: Add devicetree changes to support seven segment display on zaius
  2016-10-17 17:10 [PATCH linux v2 1/3] drivers: misc: Character driver for seven segment display Jaghathiswari Rankappagounder Natarajan
  2016-10-17 17:10 ` [PATCH linux v2 2/3] drivers: misc: Platform " Jaghathiswari Rankappagounder Natarajan
@ 2016-10-17 17:10 ` Jaghathiswari Rankappagounder Natarajan
  2016-10-17 19:55 ` [PATCH linux v2 1/3] drivers: misc: Character driver for seven segment display Rick Altherr
  2 siblings, 0 replies; 5+ messages in thread
From: Jaghathiswari Rankappagounder Natarajan @ 2016-10-17 17:10 UTC (permalink / raw)
  To: openbmc; +Cc: joel, raltherr, xow, Jaghathiswari Rankappagounder Natarajan

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

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

diff --git a/arch/arm/boot/dts/aspeed-bmc-opp-zaius.dts b/arch/arm/boot/dts/aspeed-bmc-opp-zaius.dts
index 4c4754b..7581540 100644
--- a/arch/arm/boot/dts/aspeed-bmc-opp-zaius.dts
+++ b/arch/arm/boot/dts/aspeed-bmc-opp-zaius.dts
@@ -1,6 +1,7 @@
 /dts-v1/;

 #include "aspeed-g5.dtsi"
+#include <dt-bindings/gpio/aspeed-gpio.h>

 / {
 	model = "Zaius BMC";
@@ -58,6 +59,14 @@
 			};
 		};
 	};
+
+	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>;
+	};
 };

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

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

* Re: [PATCH linux v2 1/3] drivers: misc: Character driver for seven segment display
  2016-10-17 17:10 [PATCH linux v2 1/3] drivers: misc: Character driver for seven segment display Jaghathiswari Rankappagounder Natarajan
  2016-10-17 17:10 ` [PATCH linux v2 2/3] drivers: misc: Platform " Jaghathiswari Rankappagounder Natarajan
  2016-10-17 17:10 ` [PATCH linux v2 3/3] devicetree: Add devicetree changes to support seven segment display on zaius Jaghathiswari Rankappagounder Natarajan
@ 2016-10-17 19:55 ` Rick Altherr
  2016-11-01 16:53   ` Jaghathiswari Rankappagounder Natarajan
  2 siblings, 1 reply; 5+ messages in thread
From: Rick Altherr @ 2016-10-17 19:55 UTC (permalink / raw)
  To: Jaghathiswari Rankappagounder Natarajan
  Cc: OpenBMC Maillist, Joel Stanley, Xo Wang

[-- Attachment #1: Type: text/plain, Size: 9668 bytes --]

On Mon, Oct 17, 2016 at 10:10 AM, Jaghathiswari Rankappagounder Natarajan <
jaghu@google.com> wrote:

> The character device driver implements the user-space API for letting
> a user write to the display including any conversion methods necessary
> to map the user input to a 7-segment display.
>
> Signed-off-by: Jaghathiswari Rankappagounder Natarajan <jaghu@google.com>
> ---
>  drivers/misc/Kconfig          |   5 ++
>  drivers/misc/Makefile         |   1 +
>  drivers/misc/seven_seg_disp.c | 192 ++++++++++++++++++++++++++++++
> ++++++++++++
>  drivers/misc/seven_seg_gen.h  |  32 +++++++
>  4 files changed, 230 insertions(+)
>  create mode 100644 drivers/misc/seven_seg_disp.c
>  create mode 100644 drivers/misc/seven_seg_gen.h
>
> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
> index 4617ddc..cf07817 100644
> --- a/drivers/misc/Kconfig
> +++ b/drivers/misc/Kconfig
> @@ -804,6 +804,11 @@ 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_DISPLAY
> +       tristate "Character driver for seven segment display support"
> +       help
> +         Character driver for seven segment display support
> +
>  config ASPEED_BT_IPMI_HOST
>         tristate "BT IPMI host driver"
>         help
> diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
> index 724861b..718dc2b 100644
> --- a/drivers/misc/Makefile
> +++ b/drivers/misc/Makefile
> @@ -57,4 +57,5 @@ obj-$(CONFIG_ECHO)            += echo/
>  obj-$(CONFIG_VEXPRESS_SYSCFG)  += vexpress-syscfg.o
>  obj-$(CONFIG_CXL_BASE)         += cxl/
>  obj-$(CONFIG_PANEL)             += panel.o
> +obj-$(CONFIG_SEVEN_SEGMENT_DISPLAY)    += seven_seg_disp.o
>  obj-$(CONFIG_ASPEED_BT_IPMI_HOST)      += bt-host.o
> diff --git a/drivers/misc/seven_seg_disp.c b/drivers/misc/seven_seg_disp.c
> new file mode 100644
> index 0000000..88df4a0
> --- /dev/null
> +++ b/drivers/misc/seven_seg_disp.c
> @@ -0,0 +1,192 @@
> +/*
> + * Seven segment display character driver
> + * * 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 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 "seven_seg_gen.h"
> +
> +#define MAX_DISP_CHAR_SIZE 3
> +
> +#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)
> +{
>
May as well return the current value to them.


> +       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)
> +{
> +       char tmp[MAX_DISP_CHAR_SIZE];
> +       int length = len - 1;
> +       int i;
> +       u8 result;
> +
> +       struct seven_seg_disp_dev *disp_dev;
> +
> +       if (length != MAX_DISP_CHAR_SIZE) {
> +               return -EINVAL;
> +       }
> +
> +       if (copy_from_user(tmp, buf, length) != 0) {
> +               return -EFAULT;
> +       }
> +
> +       for (i = 0; i < MAX_DISP_CHAR_SIZE; i++) {
>
Only check the bytes actually provided by the user.  If they do a 1-byte
write, you'll read uninit data.


> +               if (!isxdigit(tmp[i]))
> +                       return -EINVAL;
> +       }
> +
> +       result = convert_to_disp_data(tmp);
>
Pick a more descriptive name than 'result' and 'tmp'.  Describe what the
variable contains.


> +
> +       disp_dev = filp->private_data;
> +       disp_dev->update_seven_seg_data(&disp_dev->parent, result);
> +
> +       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 rem_cdev(struct seven_seg_disp_dev *disp_dev)
> +{
> +       cdev_del(&disp_dev->cdev);
> +       device_destroy(seven_seg_disp_class, seven_seg_devno);
> +}
> +
> +int setup_cdev(struct device parent,
> +       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, &parent,
> seven_seg_devno,
> +                       NULL, "seven_seg_disp_val");
> +       if (dev == NULL)
> +               return -1;
> +       disp_dev->parent = parent;
> +       disp_dev->dev = dev;
> +       disp_dev->update_seven_seg_data = update_disp_data;
> +       cdev_init(&disp_dev->cdev, &seven_seg_disp_fops);
> +       disp_dev->cdev.ops = &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 unregister;
> +               return -1;
> +       }
> +
> +unregister:
> +       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_gen.h b/drivers/misc/seven_seg_gen.h
> new file mode 100644
> index 0000000..5748a18
> --- /dev/null
> +++ b/drivers/misc/seven_seg_gen.h
>

Why is this header a different name from the .c?


> @@ -0,0 +1,32 @@
> +/*
> + * Seven segment display support
> + * * 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 as
> + * * published by the Free Software Foundation.
> + *
> + */
> +
> +#ifndef SEVEN_SEG_H
> +#define SEVEN_SEG_H
>

Guards don't match header name.


> +
> +#include <linux/device.h>
> +#include <linux/cdev.h>
> +
> +#define DEFAULT_REFRESH_INTERVAL 1000
>

This doesn't seem to be used by any of the APIs provided.


> +
> +struct seven_seg_disp_dev {
> +       struct device parent;
> +       struct device *dev;
> +       struct cdev cdev;
> +       void (*update_seven_seg_data)(struct device *, u16 data);
> +};
> +
> +int setup_cdev(struct device parent,
> +       struct seven_seg_disp_dev *disp_dev,
> +       void (*update_disp_data)(struct device *, u16 data));
>

Way too generic of a method name.  This name will be exposed in any .c that
includes this header.  Pick something that describes what it is doing _and_
what subsystem it belongs to.


> +
> +void rem_cdev(struct seven_seg_disp_dev *disp_dev);
> +
> +#endif
> --
> 2.8.0.rc3.226.g39d4020
>
>

[-- Attachment #2: Type: text/html, Size: 12978 bytes --]

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

* Re: [PATCH linux v2 1/3] drivers: misc: Character driver for seven segment display
  2016-10-17 19:55 ` [PATCH linux v2 1/3] drivers: misc: Character driver for seven segment display Rick Altherr
@ 2016-11-01 16:53   ` Jaghathiswari Rankappagounder Natarajan
  0 siblings, 0 replies; 5+ messages in thread
From: Jaghathiswari Rankappagounder Natarajan @ 2016-11-01 16:53 UTC (permalink / raw)
  To: Rick Altherr; +Cc: OpenBMC Maillist, Joel Stanley, Xo Wang

[-- Attachment #1: Type: text/plain, Size: 10458 bytes --]

On Mon, Oct 17, 2016 at 12:55 PM, Rick Altherr <raltherr@google.com> wrote:

>
>
> On Mon, Oct 17, 2016 at 10:10 AM, Jaghathiswari Rankappagounder Natarajan
> <jaghu@google.com> wrote:
>
>> The character device driver implements the user-space API for letting
>> a user write to the display including any conversion methods necessary
>> to map the user input to a 7-segment display.
>>
>> Signed-off-by: Jaghathiswari Rankappagounder Natarajan <jaghu@google.com>
>> ---
>>  drivers/misc/Kconfig          |   5 ++
>>  drivers/misc/Makefile         |   1 +
>>  drivers/misc/seven_seg_disp.c | 192 ++++++++++++++++++++++++++++++
>> ++++++++++++
>>  drivers/misc/seven_seg_gen.h  |  32 +++++++
>>  4 files changed, 230 insertions(+)
>>  create mode 100644 drivers/misc/seven_seg_disp.c
>>  create mode 100644 drivers/misc/seven_seg_gen.h
>>
>> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
>> index 4617ddc..cf07817 100644
>> --- a/drivers/misc/Kconfig
>> +++ b/drivers/misc/Kconfig
>> @@ -804,6 +804,11 @@ 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_DISPLAY
>> +       tristate "Character driver for seven segment display support"
>> +       help
>> +         Character driver for seven segment display support
>> +
>>  config ASPEED_BT_IPMI_HOST
>>         tristate "BT IPMI host driver"
>>         help
>> diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
>> index 724861b..718dc2b 100644
>> --- a/drivers/misc/Makefile
>> +++ b/drivers/misc/Makefile
>> @@ -57,4 +57,5 @@ obj-$(CONFIG_ECHO)            += echo/
>>  obj-$(CONFIG_VEXPRESS_SYSCFG)  += vexpress-syscfg.o
>>  obj-$(CONFIG_CXL_BASE)         += cxl/
>>  obj-$(CONFIG_PANEL)             += panel.o
>> +obj-$(CONFIG_SEVEN_SEGMENT_DISPLAY)    += seven_seg_disp.o
>>  obj-$(CONFIG_ASPEED_BT_IPMI_HOST)      += bt-host.o
>> diff --git a/drivers/misc/seven_seg_disp.c b/drivers/misc/seven_seg_disp.
>> c
>> new file mode 100644
>> index 0000000..88df4a0
>> --- /dev/null
>> +++ b/drivers/misc/seven_seg_disp.c
>> @@ -0,0 +1,192 @@
>> +/*
>> + * Seven segment display character driver
>> + * * 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 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 "seven_seg_gen.h"
>> +
>> +#define MAX_DISP_CHAR_SIZE 3
>> +
>> +#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)
>> +{
>>
> May as well return the current value to them.
>
>
>> +       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)
>> +{
>> +       char tmp[MAX_DISP_CHAR_SIZE];
>> +       int length = len - 1;
>> +       int i;
>> +       u8 result;
>> +
>> +       struct seven_seg_disp_dev *disp_dev;
>> +
>> +       if (length != MAX_DISP_CHAR_SIZE) {
>> +               return -EINVAL;
>> +       }
>> +
>> +       if (copy_from_user(tmp, buf, length) != 0) {
>> +               return -EFAULT;
>> +       }
>> +
>> +       for (i = 0; i < MAX_DISP_CHAR_SIZE; i++) {
>>
> Only check the bytes actually provided by the user.  If they do a 1-byte
> write, you'll read uninit data.
>
>
I am checking if the user is writing MAX_DISP_CHAR_SIZE  data (3 bytes of
data) in the previous step (length != MAX_DISP_CHAR_SIZE).
Wanted to make sure user is writing 3 bytes of data so that conversion
could happen correctly.

+               if (!isxdigit(tmp[i]))
>> +                       return -EINVAL;
>> +       }
>> +
>> +       result = convert_to_disp_data(tmp);
>>
> Pick a more descriptive name than 'result' and 'tmp'.  Describe what the
> variable contains.
>
>
>> +
>> +       disp_dev = filp->private_data;
>> +       disp_dev->update_seven_seg_data(&disp_dev->parent, result);
>> +
>> +       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 rem_cdev(struct seven_seg_disp_dev *disp_dev)
>> +{
>> +       cdev_del(&disp_dev->cdev);
>> +       device_destroy(seven_seg_disp_class, seven_seg_devno);
>> +}
>> +
>> +int setup_cdev(struct device parent,
>> +       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, &parent,
>> seven_seg_devno,
>> +                       NULL, "seven_seg_disp_val");
>> +       if (dev == NULL)
>> +               return -1;
>> +       disp_dev->parent = parent;
>> +       disp_dev->dev = dev;
>> +       disp_dev->update_seven_seg_data = update_disp_data;
>> +       cdev_init(&disp_dev->cdev, &seven_seg_disp_fops);
>> +       disp_dev->cdev.ops = &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 unregister;
>> +               return -1;
>> +       }
>> +
>> +unregister:
>> +       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_gen.h b/drivers/misc/seven_seg_gen.h
>> new file mode 100644
>> index 0000000..5748a18
>> --- /dev/null
>> +++ b/drivers/misc/seven_seg_gen.h
>>
>
> Why is this header a different name from the .c?
>
>
>> @@ -0,0 +1,32 @@
>> +/*
>> + * Seven segment display support
>> + * * 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 as
>> + * * published by the Free Software Foundation.
>> + *
>> + */
>> +
>> +#ifndef SEVEN_SEG_H
>> +#define SEVEN_SEG_H
>>
>
> Guards don't match header name.
>
>
>> +
>> +#include <linux/device.h>
>> +#include <linux/cdev.h>
>> +
>> +#define DEFAULT_REFRESH_INTERVAL 1000
>>
>
> This doesn't seem to be used by any of the APIs provided.
>

If the refresh-interval-ms property is not provided in the device tree then
this value is used as default value(in file seven_seg_gpio.c)


> +
>> +struct seven_seg_disp_dev {
>> +       struct device parent;
>> +       struct device *dev;
>> +       struct cdev cdev;
>> +       void (*update_seven_seg_data)(struct device *, u16 data);
>> +};
>> +
>> +int setup_cdev(struct device parent,
>> +       struct seven_seg_disp_dev *disp_dev,
>> +       void (*update_disp_data)(struct device *, u16 data));
>>
>
> Way too generic of a method name.  This name will be exposed in any .c
> that includes this header.  Pick something that describes what it is doing
> _and_ what subsystem it belongs to.
>
>
>> +
>> +void rem_cdev(struct seven_seg_disp_dev *disp_dev);
>> +
>> +#endif
>> --
>> 2.8.0.rc3.226.g39d4020
>>
>>
>

[-- Attachment #2: Type: text/html, Size: 14749 bytes --]

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

end of thread, other threads:[~2016-11-01 16:54 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-10-17 17:10 [PATCH linux v2 1/3] drivers: misc: Character driver for seven segment display Jaghathiswari Rankappagounder Natarajan
2016-10-17 17:10 ` [PATCH linux v2 2/3] drivers: misc: Platform " Jaghathiswari Rankappagounder Natarajan
2016-10-17 17:10 ` [PATCH linux v2 3/3] devicetree: Add devicetree changes to support seven segment display on zaius Jaghathiswari Rankappagounder Natarajan
2016-10-17 19:55 ` [PATCH linux v2 1/3] drivers: misc: Character driver for seven segment display Rick Altherr
2016-11-01 16:53   ` Jaghathiswari Rankappagounder Natarajan

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.