On Mon, Sep 12, 2016 at 09:42:20AM +0000, Tommy Lo wrote: > > > > >Thanks for the patch, but can you resend it with a Signed-off-by: line, as > >described by Documentation/SubmittingPatches? We need that before we can > >take anything. > > >Also, you should run the code through scripts/checkpatch.pl and fix up the > >errors it finds. There are still a number of basic coding style fixes that > >need to be done. > > >And finally, this code really should tie the LED interaction into the LED > >kernel subsystem, so that you do not need a custom userspace program to > >handle ioctls just for LED control. Do you need help in making those > >changes? > > >thanks, > > >greg k-h > > Thanks for the information and advices sincerely. I ran the checkpatch.pl and > eliminated most of the errors.
Any reason you didn't fix the rest? > The precise behavior of the interaction is bundled in the user application. > While pushing the buttons it triggers a IQR to inform driver ,makes the CPLD > LED blinking, > and send the signal to make the app layer to umount the devices. > > If it succeeds , it will turn off the LED , or it stops blinking and keeps > the LED on. So the userspace program is in charge here entirely? That sounds odd. > On the other hand , while plugging in the disk , it will try to verify the > dev/devices wheather are shown in the file system. If succeeds,it will make > the driver turn off the LED, or keep it on. > > Because of this target , the behavior is bumdled in the app partially. We > also provide the user program for users to use directly or integrate into > their systems. > > This is the reason how it is designed. Could you please give me some > iformation or advices to confirm if it's adequate ? Please use the in-kernel LED api for your LED control, and then change your userspace application to use that api. After that, we can look at the rest of the user/kernel api you have here, but I really doubt you will need a char device node for it. Some comments on your code: > > Thanks! > Tommy > > Signed-off-by: Tommy Lo <[email protected]> > > > --- > drivers/char/Kconfig | 9 + > drivers/char/Makefile | 2 + > drivers/char/hdd_hp_btn.c | 632 > ++++++++++++++++++++++++++++++++++++++++++++++ > drivers/char/hdd_hp_btn.h | 20 ++ > 4 files changed, 663 insertions(+) > create mode 100644 drivers/char/hdd_hp_btn.c > create mode 100644 drivers/char/hdd_hp_btn.h > > diff --git a/drivers/char/Kconfig b/drivers/char/Kconfig > index dcc0973..ca84ca0 100644 > --- a/drivers/char/Kconfig > +++ b/drivers/char/Kconfig > @@ -590,5 +590,14 @@ config TILE_SROM > > source "drivers/char/xillybus/Kconfig" > > +config HDD_HP_BTN > + tristate "Advantech CPCI-1502 hard disk hot swap button driver" > + default n > + help > + This driver enables the support for the hard disk hot swap button > driver. > + ACPI6-B provide hot-plug buttons for each SAS HDD, and an interrupt > (IRQ5) is generated when pressing button. > + The module catches IRQ5,then raises signal to user space application > after checking corresponding status register to get HDD status. > + It also provides ioctl method which response to CPLD to notify LED > on/off. > + > endmenu > > diff --git a/drivers/char/Makefile b/drivers/char/Makefile > index 6e6c244..ad45107 100644 > --- a/drivers/char/Makefile > +++ b/drivers/char/Makefile > @@ -60,3 +60,5 @@ js-rtc-y = rtc.o > obj-$(CONFIG_TILE_SROM) += tile-srom.o > obj-$(CONFIG_XILLYBUS) += xillybus/ > obj-$(CONFIG_POWERNV_OP_PANEL) += powernv-op-panel.o > + > +obj-$(CONFIG_HDD_HP_BTN) += hdd_hp_btn.o > \ No newline at end of file > diff --git a/drivers/char/hdd_hp_btn.c b/drivers/char/hdd_hp_btn.c > new file mode 100644 > index 0000000..9aa60be > --- /dev/null > +++ b/drivers/char/hdd_hp_btn.c > @@ -0,0 +1,632 @@ > +/* > +* Advantech SAS hard disk hot swap button driver > +* > +* Copyright (C) 2016 Advantech > +* > +* 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/kernel.h> > +#include <linux/types.h> > +#include <linux/init.h> > +#include <linux/major.h> > +#include <linux/fs.h> > +#include <linux/cdev.h> > +#include <linux/device.h> > +#include <linux/uaccess.h> > +#include <linux/ioport.h> > +#include <linux/io.h> > +#include <linux/bitops.h> > +#include <linux/delay.h> > +#include <linux/kobject.h> > +#include <linux/sysfs.h> > +#include <linux/interrupt.h> > +#include <linux/workqueue.h> > +#include <linux/sched.h> > +#include <linux/slab.h> > + > +#include <asm/siginfo.h> > +#include <linux/rcupdate.h>//rcu_read_lock > +#include <linux/debugfs.h> > +#include <linux/uaccess.h> > +#include <linux/ioctl.h> > +#include <linux/moduleparam.h> > +#include "hdd_hp_btn.h" > + > +#define LPC_ADDR 0x900 > +#define EFB_WB_ADDR (LPC_ADDR+0x52) > +#define EFB_WB_WRITE (LPC_ADDR+0x53) > +#define EFB_WB_READ (LPC_ADDR+0x54) > +#define EFB_WB_CTRL (LPC_ADDR+0x55) > +#define EFB_WB_RD_CTL 0x2 > +#define EFB_WB_WR_CTL 0x1 > + > +#define FRU_LED_ADDR (LPC_ADDR+0x16) > + > +#define FPGA_SIRQ_CFG (LPC_ADDR+0x2C) > +#define FPGA_SIRQ_REG (LPC_ADDR+0x2E) > +#define FPGA_SIRQ_5 0x1 > +#define FPGA_SIRQ_6 0x2 > +#define FPGA_SIRQ_7 0x3 > +#define FPGA_SIRQ_9 0x4 > +#define FPGA_SIRQ_10 0x5 > +#define FPGA_SIRQ_11 0x6 > +#define FPGA_SIRQ_12 0x7 > +#define FPGA_SIRQ_13 0x8 > +#define FPGA_SIRQ_14 0x9 > +#define FPGA_SIRQ_15 0xA > +#define FPGA_SIRQ_INTA 0xC > +#define FPGA_SIRQ_INTB 0xD > +#define FPGA_SIRQ_INTC 0xE > +#define FPGA_SIRQ_INTD 0xF > +#define FPGA_SIRQ_NUM 15 > + > +#define IOCTL_EFB_WB_WRITE 0x7F > +#define IOCTL_EFB_WB_READ 0x7E > +#define IOCTL_FRU_LED_CTL 0x80 > +/*************************************************************/ > +/* Start of SYSFS kobject define */ > +/* */ > + > +static struct kobject *lpc_kobj; > +static struct kobject *lpc_user_led_kobj; > +static struct kobject *lpc_register_kobj; > +static struct kobject *lpc_rtm_kobj; Never use "raw" kobjects in a driver, that's a huge sign that something is wrong with it. They should not be needed at all. > + > +static int free_irq_num = FPGA_SIRQ_NUM; > +static int pid; > +static int signal_num = HDD_SWAP_SIG; > + > +module_param(signal_num, int, 0); > + > +static ssize_t reg40_show(struct kobject *kobj, struct kobj_attribute *attr, > char *buf) > +{ > + int reg; > + > + reg = inb(LPC_ADDR + 0x40); > + return sprintf(buf, "%d:%02X\n", reg, reg); > +} > + > +static ssize_t reg40_store(struct kobject *kobj, struct kobj_attribute > *attr, const char *buf, size_t count) > +{ > + int reg; > + > + if (sscanf(buf, "%d", ®) != 1) > + return -EINVAL; > + > + outb(reg, LPC_ADDR + 0x40); > + return count; > +} > + > +static struct kobj_attribute reg40_attribute = > + __ATTR(40, 0664, reg40_show, reg40_store); Not that you need this in the future, but please always use the __ATTR_RW() and other forms of the macro. > + > +static ssize_t reg41_show(struct kobject *kobj, struct kobj_attribute *attr, > char *buf) > +{ > + int reg; > + > + reg = inb(LPC_ADDR + 0x41); > + return sprintf(buf, "%d:%02X\n", reg, reg); > +} > + > +static ssize_t reg41_store(struct kobject *kobj, struct kobj_attribute > *attr, const char *buf, size_t count) > +{ > + int reg; > + > + if (sscanf(buf, "%d", ®) != 1) > + return -EINVAL; > + > + outb(reg, LPC_ADDR + 0x41); > + return count; > +} > + > +static struct kobj_attribute reg41_attribute = > + __ATTR(41, 0664, reg41_show, reg41_store); > + > +static ssize_t reg42_show(struct kobject *kobj, struct kobj_attribute *attr, > char *buf) > +{ > + int reg; > + > + reg = inb(LPC_ADDR + 0x42); > + return sprintf(buf, "%d:%02X\n", reg, reg); > +} > + > +static ssize_t reg42_store(struct kobject *kobj, struct kobj_attribute > *attr, const char *buf, size_t count) > +{ > + int reg; > + > + if (sscanf(buf, "%d", ®) != 1) > + return -EINVAL; > + > + outb(reg, LPC_ADDR + 0x42); > + return count; > +} > + > +static struct kobj_attribute reg42_attribute = > + __ATTR(42, 0664, reg42_show, reg42_store); > + > +static ssize_t reg43_show(struct kobject *kobj, struct kobj_attribute *attr, > char *buf) > +{ > + int reg; > + > + reg = inb(LPC_ADDR + 0x43); > + return sprintf(buf, "%d:%02X\n", reg, reg); > +} > + > +static ssize_t reg43_store(struct kobject *kobj, struct kobj_attribute > *attr, const char *buf, size_t count) > +{ > + int reg; > + > + if (sscanf(buf, "%d", ®) != 1) > + return -EINVAL; > + > + if (reg < 255 && reg >= 0) > + outb(reg, LPC_ADDR + 0x43); > + return count; > +} > + > +static struct kobj_attribute reg43_attribute = > + __ATTR(43, 0664, reg43_show, reg43_store); > + > +static ssize_t reg30_show(struct kobject *kobj, struct kobj_attribute *attr, > char *buf) > +{ > + int reg; > + > + reg = inb(LPC_ADDR + 0x30); > + return sprintf(buf, "%d\n", reg); > +} > + > +static ssize_t reg30_store(struct kobject *kobj, struct kobj_attribute > *attr, const char *buf, size_t count) > +{ > + int reg; > + > + if (sscanf(buf, "%d", ®) != 1) > + return -EINVAL; > + > + if (reg < 255 && reg >= 0) > + outb(reg, LPC_ADDR + 0x30); > + return count; > +} > + > +static struct kobj_attribute reg30_attribute = > + __ATTR(30, 0664, reg30_show, reg30_store); > + > +static struct attribute *attrs_register[] = { > + ®40_attribute.attr, > + ®41_attribute.attr, > + ®42_attribute.attr, > + ®43_attribute.attr, > + ®30_attribute.attr, > + NULL, > +}; shouldn't all of this just be some debugfs files? No userspace program cares about these registers. > + > +static DECLARE_WAIT_QUEUE_HEAD(bt_wq0); > +static int bt_wait0; > +static int bt_flag0; You only have support for one device in the system at a time? not good. > +/*******************************************************************/ > +/* Start of DEVFS define */ devfs went away over a decade ago, what code are you copying this from? > +static ssize_t drv_read(struct file *filp, char *buf, size_t count, loff_t > *ppos) > +{ > + return count; > +} why have read at all if you only act as a sink? > + > +static ssize_t drv_write(struct file *filp, const char *buf, size_t count, > loff_t *ppos) > +{ > + char mybuf[10]; > + /* read the value from user space */ > + if (count > 10) > + return -EINVAL; why 10? > + if (copy_from_user(mybuf, buf, count)) > + return -EFAULT; > + > + if (sscanf(mybuf, "%d", &pid) != 1) > + return -EFAULT; what are you reading? > + > + printk(KERN_INFO "User pid = %d\n", pid); why does the pid end up in the kernel log? > + > + return count; So you just write a pid into the kernel log? This seems really odd. > +} > + > +static int drv_open(struct inode *inode, struct file *filp) > +{ > + return 0; why is this needed? > +} > + > +long drv_ioctl(struct file *filp, unsigned int cmd, unsigned long arg) > +{ > + struct ioctl_cmd data; > + > + memset(&data, 0, sizeof(data)); > + > + switch (cmd) { > + case IOCTL_LED_ON: > + if (copy_from_user(&data, (int __user *) arg, sizeof(data))) > + return -EFAULT; > + > + if (data.val == 1) { > + outb(0x10, LPC_ADDR + 0x43); > + printk(KERN_INFO"Dev:LED1 ON\n"); > + } else if (data.val == 2) { > + outb(0x20, LPC_ADDR + 0x43); > + printk(KERN_INFO"Dev:LED2 ON\n"); > + } > + break; > + case IOCTL_LED_OFF: > + if (copy_from_user(&data, (int __user *) arg, sizeof(data))) > + return -EFAULT; > + > + if (data.val == 1) { > + outb(0x01, LPC_ADDR + 0x43); > + printk(KERN_INFO"Dev:LED1 OFF\n"); > + } else if (data.val == 2) { > + outb(0x02, LPC_ADDR + 0x43); > + printk(KERN_INFO"Dev:LED2 OFF\n"); > + } > + break; Just use the LED api. > + } > + return 0; > +} > + > +static int drv_release(struct inode *inode, struct file *filp) > +{ > + return 0; > +} again, not needed. > + > +irqreturn_t HDD_IRQ(int irq, void *dev_id) odd function name. > +{ > + unsigned char tmp = 0; > + > + printk(KERN_INFO"HDD_IRQ5:INTERRUPT\n"); > + tmp = inb(LPC_ADDR + 0x40); > + > + if ((tmp & 0x33) == 0) > + return IRQ_NONE; > + > + outb(tmp, LPC_ADDR + 0x40); > + > + /*HDD1_HP_SW*/ > + if (tmp & 0x10) { > + send_signal(signal_num, SIG_BUTTON1_INVOKE); > + bt_flag0 = 1; > + wake_up_interruptible(&bt_wq0); > + } > + > + /*HDD2_HP_SW*/ > + if (tmp & 0x20) { > + send_signal(signal_num, SIG_BUTTON2_INVOKE); > + bt_flag1 = 1; > + wake_up_interruptible(&bt_wq1); > + } > + > + /*HDD1_INSERT*/ > + if (tmp & 0x01) { > + send_signal(signal_num, SIG_HDD1_INSERT); > + prsnt_flag0 = 1; > + wake_up_interruptible(&prsnt_wq0); > + } > + > + /*HDD2_INSERT*/ > + if (tmp & 0x02) { > + send_signal(signal_num, SIG_HDD2_INSERT); > + prsnt_flag1 = 1; > + wake_up_interruptible(&prsnt_wq1); > + } > + > + return IRQ_HANDLED; > +}; > + > +const struct file_operations drv_fops = { > + owner: THIS_MODULE, > + read : drv_read, > + write : drv_write, > + unlocked_ioctl : drv_ioctl, > + open : drv_open, > + release : drv_release, > +}; > + > +#define DRIVER_NAME "hdd_hp_btn" > +static unsigned int demo_chrdev_alloc_major = 0; > +static unsigned int num_of_dev = 1; why 1? > +static struct cdev demo_chrdev_alloc_cdev; > +struct class *demo_class; Not that you really need it, but in the future, just use the misc_device interface, much simpler and it does not burn a whole major number for one minor device node. thanks, greg k-h
