[PATCH linux v2 1/3] drivers: misc: Character driver for seven segment display

Jaghathiswari Rankappagounder Natarajan jaghu at google.com
Wed Nov 2 03:53:53 AEDT 2016


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

>
>
> On Mon, Oct 17, 2016 at 10:10 AM, Jaghathiswari Rankappagounder Natarajan
> <jaghu at 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 at 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 at 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
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.ozlabs.org/pipermail/openbmc/attachments/20161101/2c115465/attachment-0001.html>


More information about the openbmc mailing list