lkml.org 
[lkml]   [2018]   [Jan]   [24]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH arm/aspeed/ast2500 v2] ipmi: add an Aspeed KCS IPMI BMC driver
    From
    Date
    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

    \
     
     \ /
      Last update: 2018-01-24 18:06    [W:3.662 / U:0.008 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site