<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Oct 17, 2016 at 12:55 PM, Rick Altherr <span dir="ltr"><<a href="mailto:raltherr@google.com" target="_blank">raltherr@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote"><div><div class="gmail-h5">On Mon, Oct 17, 2016 at 10:10 AM, Jaghathiswari Rankappagounder Natarajan <span dir="ltr"><<a href="mailto:jaghu@google.com" target="_blank">jaghu@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">The character device driver implements the user-space API for letting<br>
a user write to the display including any conversion methods necessary<br>
to map the user input to a 7-segment display.<br>
<br>
Signed-off-by: Jaghathiswari Rankappagounder Natarajan <<a href="mailto:jaghu@google.com" target="_blank">jaghu@google.com</a>><br>
---<br>
 drivers/misc/Kconfig          |   5 ++<br>
 drivers/misc/Makefile         |   1 +<br>
 drivers/misc/seven_seg_disp.c | 192 ++++++++++++++++++++++++++++++<wbr>++++++++++++<br>
 drivers/misc/seven_seg_gen.h  |  32 +++++++<br>
 4 files changed, 230 insertions(+)<br>
 create mode 100644 drivers/misc/seven_seg_disp.c<br>
 create mode 100644 drivers/misc/seven_seg_gen.h<br>
<br>
diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig<br>
index 4617ddc..cf07817 100644<br>
--- a/drivers/misc/Kconfig<br>
+++ b/drivers/misc/Kconfig<br>
@@ -804,6 +804,11 @@ config PANEL_BOOT_MESSAGE<br>
          An empty message will only clear the display at driver init time. Any other<br>
          printf()-formatted message is valid with newline and escape codes.<br>
<br>
+config SEVEN_SEGMENT_DISPLAY<br>
+       tristate "Character driver for seven segment display support"<br>
+       help<br>
+         Character driver for seven segment display support<br>
+<br>
 config ASPEED_BT_IPMI_HOST<br>
        tristate "BT IPMI host driver"<br>
        help<br>
diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile<br>
index 724861b..718dc2b 100644<br>
--- a/drivers/misc/Makefile<br>
+++ b/drivers/misc/Makefile<br>
@@ -57,4 +57,5 @@ obj-$(CONFIG_ECHO)            += echo/<br>
 obj-$(CONFIG_VEXPRESS_SYSCFG)<wbr>  += vexpress-syscfg.o<br>
 obj-$(CONFIG_CXL_BASE)         += cxl/<br>
 obj-$(CONFIG_PANEL)             += panel.o<br>
+obj-$(CONFIG_SEVEN_SEGMENT_DI<wbr>SPLAY)    += seven_seg_disp.o<br>
 obj-$(CONFIG_ASPEED_BT_IPMI_H<wbr>OST)      += bt-host.o<br>
diff --git a/drivers/misc/seven_seg_disp.<wbr>c b/drivers/misc/seven_seg_disp.<wbr>c<br>
new file mode 100644<br>
index 0000000..88df4a0<br>
--- /dev/null<br>
+++ b/drivers/misc/seven_seg_disp.<wbr>c<br>
@@ -0,0 +1,192 @@<br>
+/*<br>
+ * Seven segment display character driver<br>
+ * * Copyright (c) 2016 Google, Inc<br>
+ *<br>
+ * * This program is free software; you can redistribute it and/or modify<br>
+ * * it under the terms of the GNU General Public License version 2 as<br>
+ * * published by the Free Software Foundation.<br>
+ *<br>
+ */<br>
+<br>
+#include <linux/module.h><br>
+#include <linux/version.h><br>
+#include <linux/kernel.h><br>
+#include <linux/types.h><br>
+#include <linux/kdev_t.h><br>
+#include <linux/fs.h><br>
+#include <linux/uaccess.h><br>
+#include <linux/ctype.h><br>
+<br>
+#include "seven_seg_gen.h"<br>
+<br>
+#define MAX_DISP_CHAR_SIZE 3<br>
+<br>
+#define LED_DOT 0x01<br>
+<br>
+/*<br>
+ * 0   1   2   3   4   5   6   7   8   9   A   B   C   D   E   F<br>
+ *  _       _   _       _   _   _   _   _   _       _       _   _<br>
+ * | |   |  _|  _| |_| |_  |_    | |_| |_| |_| |_  |    _| |_  |_<br>
+ * |_|   | |_   _|   |  _| |_|   | |_|   | | | |_| |_  |_| |_  |<br>
+ *<br>
+ * data[7:1] = led[a:g]<br>
+ */<br>
+const u8 seven_seg_bits[] = {<br>
+       0xFC, 0x60, 0xDA, 0xF2, 0x66, 0xB6, 0xBE, 0xE0,<br>
+       0xFE, 0xF6, 0xEE, 0x3E, 0x9C, 0x7A, 0x9E, 0x8E<br>
+       };<br>
+<br>
+/*<br>
+ * 0   1   2   3   4   5   6   7   8   9   A   B   C   D   E   F<br>
+ *      _       _   _                              _            _<br>
+ *     |   |_  |_| |_  _   _   _   _   _   _   _  |_    _|  _| | |<br>
+ *     |_  |_  |   |                               _|  |_| |_| | |<br>
+ *<br>
+ * data[7:1] = led[a:g]<br>
+ */<br>
+const u8 special_seven_seg_bits[] = {<br>
+       0x00, 0x9C, 0x1E, 0xCE, 0x8E, 0x02, 0x02, 0x02,<br>
+       0x02, 0x02, 0x02, 0x02, 0xB6, 0x7A, 0x7A, 0xEC<br>
+       };<br>
+<br>
+static dev_t seven_seg_devno;<br>
+static struct class *seven_seg_disp_class;<br>
+<br>
+static int seven_seg_disp_open(struct inode *inode, struct file *filp)<br>
+{<br>
+       struct seven_seg_disp_dev *disp_dev;<br>
+<br>
+       disp_dev = container_of(inode->i_cdev,<br>
+                                struct seven_seg_disp_dev, cdev);<br>
+       filp->private_data = disp_dev;<br>
+       return 0;<br>
+}<br>
+<br>
+static int seven_seg_disp_close(struct inode *inode, struct file *filp)<br>
+{<br>
+       filp->private_data = NULL;<br>
+       return 0;<br>
+}<br>
+<br>
+static ssize_t seven_seg_disp_read(struct file *filp, char __user *buf, size_t<br>
+                               len, loff_t *off)<br>
+{<br>
</blockquote></div></div><div>May as well return the current value to them.</div><div><div class="gmail-h5"><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">+       return 0;<br>
+}<br>
+<br>
+static u16 convert_to_disp_data(char *buf)<br>
+{<br>
+       u8 low_display;<br>
+       u8 high_display;<br>
+       u16 led_value;<br>
+<br>
+       low_display = seven_seg_bits[hex_to_bin(buf[<wbr>2])];<br>
+<br>
+       high_display = (buf[0] == '1') ?<br>
+       special_seven_seg_bits[hex_<wbr>to_bin(buf[1])] :<br>
+       seven_seg_bits[hex_to_bin(<wbr>buf[1])];<br>
+<br>
+       led_value = low_display | (high_display << 8);<br>
+       if (buf[0] == '1') {<br>
+               led_value |= LED_DOT | (LED_DOT << 8);<br>
+       }<br>
+<br>
+       return led_value;<br>
+}<br>
+<br>
+static ssize_t seven_seg_disp_write(struct file *filp, const char __user *buf,<br>
+                               size_t len, loff_t *off)<br>
+{<br>
+       char tmp[MAX_DISP_CHAR_SIZE];<br>
+       int length = len - 1;<br>
+       int i;<br>
+       u8 result;<br>
+<br>
+       struct seven_seg_disp_dev *disp_dev;<br>
+<br>
+       if (length != MAX_DISP_CHAR_SIZE) {<br>
+               return -EINVAL;<br>
+       }<br>
+<br>
+       if (copy_from_user(tmp, buf, length) != 0) {<br>
+               return -EFAULT;<br>
+       }<br>
+<br>
+       for (i = 0; i < MAX_DISP_CHAR_SIZE; i++) {<br></blockquote></div></div><div>Only check the bytes actually provided by the user.  If they do a 1-byte write, you'll read uninit data.</div><span class="gmail-"><div> </div></span></div></div></div></blockquote><div>I am checking if the user is writing MAX_DISP_CHAR_SIZE  data (3 bytes of data) in the previous step (<span style="color:rgb(80,0,80)">length != MAX_DISP_CHAR_SIZE)</span>.</div><div>Wanted to make sure user is writing 3 bytes of data so that conversion could happen correctly.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><span class="gmail-"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
+               if (!isxdigit(tmp[i]))<br>
+                       return -EINVAL;<br>
+       }<br>
+<br>
+       result = convert_to_disp_data(tmp);<br></blockquote></span><div>Pick a more descriptive name than 'result' and 'tmp'.  Describe what the variable contains.</div><div><div class="gmail-h5"><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
+<br>
+       disp_dev = filp->private_data;<br>
+       disp_dev->update_seven_seg_da<wbr>ta(&disp_dev->parent, result);<br>
+<br>
+       return len;<br>
+}<br>
+<br>
+static const struct file_operations seven_seg_disp_fops = {<br>
+<br>
+       .owner = THIS_MODULE,<br>
+       .open = seven_seg_disp_open,<br>
+       .release = seven_seg_disp_close,<br>
+       .read = seven_seg_disp_read,<br>
+       .write = seven_seg_disp_write<br>
+};<br>
+<br>
+void rem_cdev(struct seven_seg_disp_dev *disp_dev)<br>
+{<br>
+       cdev_del(&disp_dev->cdev);<br>
+       device_destroy(seven_seg_<wbr>disp_class, seven_seg_devno);<br>
+}<br>
+<br>
+int setup_cdev(struct device parent,<br>
+       struct seven_seg_disp_dev *disp_dev,<br>
+       void (*update_disp_data)(struct device *, u16 data))<br>
+{<br>
+       struct device *dev;<br>
+       int err;<br>
+<br>
+       dev = device_create(seven_seg_disp_c<wbr>lass, &parent, seven_seg_devno,<br>
+                       NULL, "seven_seg_disp_val");<br>
+       if (dev == NULL)<br>
+               return -1;<br>
+       disp_dev->parent = parent;<br>
+       disp_dev->dev = dev;<br>
+       disp_dev->update_seven_seg_da<wbr>ta = update_disp_data;<br>
+       cdev_init(&disp_dev->cdev, &seven_seg_disp_fops);<br>
+       disp_dev->cdev.ops = &seven_seg_disp_fops;<br>
+       err = cdev_add(&disp_dev->cdev, seven_seg_devno, 1);<br>
+       if (err)<br>
+               device_destroy(seven_seg_<wbr>disp_class, seven_seg_devno);<br>
+       return err;<br>
+}<br>
+<br>
+static int __init seven_seg_disp_init(void)<br>
+{<br>
+       if (alloc_chrdev_region(&seven_se<wbr>g_devno, 0, 1, "disp_state") < 0) {<br>
+               return -1;<br>
+       }<br>
+<br>
+       seven_seg_disp_class = class_create(THIS_MODULE, "disp_state");<br>
+       if (seven_seg_disp_class == NULL) {<br>
+               goto unregister;<br>
+               return -1;<br>
+       }<br>
+<br>
+unregister:<br>
+       unregister_chrdev_region(seve<wbr>n_seg_devno, 1);<br>
+       return -1;<br>
+}<br>
+<br>
+static void __exit seven_seg_disp_exit(void)<br>
+{<br>
+       class_destroy(seven_seg_disp_<wbr>class);<br>
+       unregister_chrdev_region(seve<wbr>n_seg_devno, 1);<br>
+}<br>
+<br>
+module_init(seven_seg_disp_in<wbr>it);<br>
+module_exit(seven_seg_disp_ex<wbr>it);<br>
+MODULE_LICENSE("GPL");<br>
+MODULE_AUTHOR("Jaghathiswari Rankappagounder Natarajan <<a href="mailto:jaghu@google.com" target="_blank">jaghu@google.com</a>>");<br>
+MODULE_DESCRIPTION("Seven segment display character driver");<br>
diff --git a/drivers/misc/seven_seg_gen.h b/drivers/misc/seven_seg_gen.h<br>
new file mode 100644<br>
index 0000000..5748a18<br>
--- /dev/null<br>
+++ b/drivers/misc/seven_seg_gen.h<br></blockquote><div><br></div></div></div><div>Why is this header a different name from the .c?</div><span class="gmail-"><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
@@ -0,0 +1,32 @@<br>
+/*<br>
+ * Seven segment display support<br>
+ * * Copyright (c) 2016 Google, Inc<br>
+ *<br>
+ * * This program is free software; you can redistribute it and/or modify<br>
+ * * it under the terms of the GNU General Public License version 2 as<br>
+ * * published by the Free Software Foundation.<br>
+ *<br>
+ */<br>
+<br>
+#ifndef SEVEN_SEG_H<br>
+#define SEVEN_SEG_H<br></blockquote><div><br></div></span><div>Guards don't match header name.</div><span class="gmail-"><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
+<br>
+#include <linux/device.h><br>
+#include <linux/cdev.h><br>
+<br>
+#define DEFAULT_REFRESH_INTERVAL 1000<br></blockquote><div><br></div></span><div>This doesn't seem to be used by any of the APIs provided.</div></div></div></div></blockquote><div><br></div><div>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)</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><span class="gmail-"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
+<br>
+struct seven_seg_disp_dev {<br>
+       struct device parent;<br>
+       struct device *dev;<br>
+       struct cdev cdev;<br>
+       void (*update_seven_seg_data)(struc<wbr>t device *, u16 data);<br>
+};<br>
+<br>
+int setup_cdev(struct device parent,<br>
+       struct seven_seg_disp_dev *disp_dev,<br>
+       void (*update_disp_data)(struct device *, u16 data));<br></blockquote><div><br></div></span><div>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.</div><span class="gmail-"><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
+<br>
+void rem_cdev(struct seven_seg_disp_dev *disp_dev);<br>
+<br>
+#endif<br>
--<br>
2.8.0.rc3.226.g39d4020<br>
<br>
</blockquote></span></div><br></div></div>
</blockquote></div><br></div></div>