Messages in this thread | | | Subject | Re: [PATCH arm/aspeed/ast2500 v2] ipmi: add an Aspeed KCS IPMI BMC driver | From | Andy Shevchenko <> | Date | Wed, 24 Jan 2018 19:05:14 +0200 |
| |
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.shevchenko@linux.intel.com> Intel Finland Oy
| |