On Thu, 2018-01-25 at 00:06 +0800, Haiyue Wang wrote: > The KCS (Keyboard Controller Style) interface is used to perform in- > band > IPMI communication between a server host and its BMC (BaseBoard > Management > Controllers). > > This driver exposes the KCS interface on ASpeed SOCs (AST2400 and > AST2500) > as a character device. Such SOCs are commonly used as BMCs and this > driver > implements the BMC side of the KCS interface.
> +config IPMI_KCS_BMC > + tristate 'IPMI KCS BMC Interface' > + help > + Provides a device driver for the KCS (Keyboard Controller > Style) > + IPMI interface which meets the requirement of the BMC > (Baseboard > + Management Controllers) side for handling the IPMI request > from > + host system software. Now time to split to two patches. > +config ASPEED_KCS_IPMI_BMC > + depends on ARCH_ASPEED || COMPILE_TEST > + depends on IPMI_KCS_BMC > + select REGMAP_MMIO > + tristate "Aspeed KCS IPMI BMC driver" > + help > + Provides a driver for the KCS (Keyboard Controller Style) > IPMI > + interface found on Aspeed SOCs (AST2400 and AST2500). > + > + The driver implements the BMC side of the KCS contorller, > it > + provides the access of KCS IO space for BMC side. > +obj-$(CONFIG_IPMI_KCS_BMC) += kcs_bmc.o > obj-$(CONFIG_ASPEED_BT_IPMI_BMC) += bt-bmc.o > +obj-$(CONFIG_ASPEED_KCS_IPMI_BMC) += kcs_bmc_aspeed.o > \ No newline at end of file Do something with your text editor. The end of text file is a \n at the end. > +/* IPMI 2.0 - Table 9-1, KCS Interface Status Register Bits */ > +#define KCS_STATUS_STATE(state) (state << 6) > +#define KCS_STATUS_STATE_MASK KCS_STATUS_STATE(0x3) GENMASK(8, 6) > + > + Remove extra line in such cases > +static inline u8 read_data(struct kcs_bmc *kcs_bmc) > +{ > + return kcs_bmc->io_inputb(kcs_bmc, kcs_bmc->ioreg.idr); > +} > + > +static inline void write_data(struct kcs_bmc *kcs_bmc, u8 data) > +{ > + kcs_bmc->io_outputb(kcs_bmc, kcs_bmc->ioreg.odr, data); > +} > + > +static inline u8 read_status(struct kcs_bmc *kcs_bmc) > +{ > + return kcs_bmc->io_inputb(kcs_bmc, kcs_bmc->ioreg.str); > +} > + > +static inline void write_status(struct kcs_bmc *kcs_bmc, u8 data) > +{ > + kcs_bmc->io_outputb(kcs_bmc, kcs_bmc->ioreg.str, data); > +} > + > +static void update_status_bits(struct kcs_bmc *kcs_bmc, u8 mask, u8 > val) > +{ > + u8 tmp; > + > + tmp = read_status(kcs_bmc); > + > + tmp &= ~mask; > + tmp |= val & mask; > + > + write_status(kcs_bmc, tmp); > +} Shouldn't be above some kind of regmap API? > +int kcs_bmc_handle_event(struct kcs_bmc *kcs_bmc) > +{ > + unsigned long flags; > + int ret = 0; > + u8 status; > + > + spin_lock_irqsave(&kcs_bmc->lock, flags); > + > + status = read_status(kcs_bmc) & (KCS_STATUS_IBF | > KCS_STATUS_CMD_DAT); > + > + switch (status) { > + case KCS_STATUS_IBF | KCS_STATUS_CMD_DAT: > + kcs_bmc_handle_command(kcs_bmc); > + break; > + > + case KCS_STATUS_IBF: > + kcs_bmc_handle_data(kcs_bmc); > + break; > + > + default: > + ret = -1; Use proper errno. > + break; > + } > + > + spin_unlock_irqrestore(&kcs_bmc->lock, flags); > + > + return ret; > +} > +EXPORT_SYMBOL(kcs_bmc_handle_event); > + > +static inline struct kcs_bmc *file_kcs_bmc(struct file *filp) > +{ > + return container_of(filp->private_data, struct kcs_bmc, > miscdev); > +} Such helper we call to_<smth>() where <smth> in your cases kcs_bmc > +static ssize_t kcs_bmc_write(struct file *filp, const char *buf, > + size_t count, loff_t *offset) > +{ > + struct kcs_bmc *kcs_bmc = file_kcs_bmc(filp); > + ssize_t ret = count; > + > + if (count < 1 || count > KCS_MSG_BUFSIZ) > + return -EINVAL; Is the first part even possible? > +} > +struct kcs_bmc *kcs_bmc_alloc(struct device *dev, int sizeof_priv, > u32 channel) > +{ > + struct kcs_bmc *kcs_bmc; > + int rc; What compiler does think about this? > + > + kcs_bmc = devm_kzalloc(dev, sizeof(*kcs_bmc) + sizeof_priv, > GFP_KERNEL); > + if (!kcs_bmc) > + return NULL; > + dev_set_name(dev, "ipmi-kcs%u", channel); > + > + spin_lock_init(&kcs_bmc->lock); > + kcs_bmc->channel = channel; > + > + init_waitqueue_head(&kcs_bmc->queue); > + kcs_bmc->data_in = devm_kmalloc(dev, KCS_MSG_BUFSIZ, > GFP_KERNEL); > + kcs_bmc->data_out = devm_kmalloc(dev, KCS_MSG_BUFSIZ, > GFP_KERNEL); > + if (kcs_bmc->data_in == NULL || kcs_bmc->data_out == NULL) { > + dev_err(dev, "Failed to allocate data buffers\n"); > + return NULL; > + } Split checks per allocation. > + kcs_bmc->miscdev.minor = MISC_DYNAMIC_MINOR; > + kcs_bmc->miscdev.name = dev_name(dev); > + kcs_bmc->miscdev.fops = &kcs_bmc_fops; > + > + return kcs_bmc; > +} > +EXPORT_SYMBOL(kcs_bmc_alloc); > > +/* Different phases of the KCS BMC module */ > +enum kcs_phases { > + /* BMC should not be expecting nor sending any data. */ > + KCS_PHASE_IDLE, Perhaps kernel-doc? > +}; > +/* IPMI 2.0 - 9.5, KCS Interface Registers */ > +struct kcs_ioreg { > + u32 idr; /* Input Data Register */ > + u32 odr; /* Output Data Register */ > + u32 str; /* Status Register */ kernel-doc > +}; > + > +struct kcs_bmc { > + spinlock_t lock; > + > + u32 channel; > + int running; > + > + /* Setup by BMC KCS controller driver */ > + struct kcs_ioreg ioreg; > + u8 (*io_inputb)(struct kcs_bmc *kcs_bmc, u32 reg); > + void (*io_outputb)(struct kcs_bmc *kcs_bmc, u32 reg, u8 b); > + > + enum kcs_phases phase; > + enum kcs_errors error; > + > + wait_queue_head_t queue; > + bool data_in_avail; > + int data_in_idx; > + u8 *data_in; > + > + int data_out_idx; > + int data_out_len; > + u8 *data_out; > + > + struct miscdevice miscdev; > + > + unsigned long long priv[]; unsigned long is enough. > +}; > + > +static inline void *kcs_bmc_priv(const struct kcs_bmc *kcs_bmc) > +{ > + return kcs_bmc->priv; > +} > + > +extern int kcs_bmc_handle_event(struct kcs_bmc *kcs_bmc); > +extern struct kcs_bmc *kcs_bmc_alloc(struct device *dev, int > sizeof_priv, > + u32 channel); Drop extern. > +#endif Next one could be reviewed when you split this patch to two. -- Andy Shevchenko <andriy.shevche...@linux.intel.com> Intel Finland Oy ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot _______________________________________________ Openipmi-developer mailing list Openipmi-developer@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openipmi-developer