[PATCH 1/4] ipmi: kcs_bmc: Simplify irq handling

[PATCH 1/4] ipmi: kcs_bmc: Simplify irq handling

[William A. Kennington III]

Platforms specific IRQ handles repeat the same logic, calling a
sub-handler in the kcs_bmc generic code that should just conform to the
irqhandler callback.

Signed-off-by: William A. Kennington III <wak@google.com>
---
 drivers/char/ipmi/kcs_bmc.c         | 10 +++++-----
 drivers/char/ipmi/kcs_bmc.h         |  3 ++-
 drivers/char/ipmi/kcs_bmc_aspeed.c  | 12 +-----------
 drivers/char/ipmi/kcs_bmc_npcm7xx.c | 12 +-----------
 4 files changed, 9 insertions(+), 28 deletions(-)

diff --git a/drivers/char/ipmi/kcs_bmc.c b/drivers/char/ipmi/kcs_bmc.c
index f292e74bd4a5..ccb35f1645cf 100644
--- a/drivers/char/ipmi/kcs_bmc.c
+++ b/drivers/char/ipmi/kcs_bmc.c
@@ -209,10 +209,11 @@ static void kcs_bmc_handle_cmd(struct kcs_bmc *kcs_bmc)
 	}
 }
 
-int kcs_bmc_handle_event(struct kcs_bmc *kcs_bmc)
+irqreturn_t kcs_bmc_irq(int irq, void *arg)
 {
+	struct kcs_bmc *kcs_bmc = arg;
 	unsigned long flags;
-	int ret = -ENODATA;
+	irqreturn_t ret = IRQ_NONE;
 	u8 status;
 
 	spin_lock_irqsave(&kcs_bmc->lock, flags);
@@ -225,15 +226,14 @@ int kcs_bmc_handle_event(struct kcs_bmc *kcs_bmc)
 			kcs_bmc_handle_cmd(kcs_bmc);
 		else
 			kcs_bmc_handle_data(kcs_bmc);
-
-		ret = 0;
+		ret = IRQ_HANDLED;
 	}
 
 	spin_unlock_irqrestore(&kcs_bmc->lock, flags);
 
 	return ret;
 }
-EXPORT_SYMBOL(kcs_bmc_handle_event);
+EXPORT_SYMBOL(kcs_bmc_irq);
 
 static inline struct kcs_bmc *to_kcs_bmc(struct file *filp)
 {
diff --git a/drivers/char/ipmi/kcs_bmc.h b/drivers/char/ipmi/kcs_bmc.h
index eb9ea4ce78b8..959d7042e6d2 100644
--- a/drivers/char/ipmi/kcs_bmc.h
+++ b/drivers/char/ipmi/kcs_bmc.h
@@ -6,6 +6,7 @@
 #ifndef __KCS_BMC_H__
 #define __KCS_BMC_H__
 
+#include <linux/irqreturn.h>
 #include <linux/miscdevice.h>
 
 /* Different phases of the KCS BMC module.
@@ -102,7 +103,7 @@ static inline void *kcs_bmc_priv(struct kcs_bmc *kcs_bmc)
 	return kcs_bmc->priv;
 }
 
-int kcs_bmc_handle_event(struct kcs_bmc *kcs_bmc);
+irqreturn_t kcs_bmc_irq(int irq, void *arg);
 struct kcs_bmc *kcs_bmc_alloc(struct device *dev, int sizeof_priv,
 					u32 channel);
 #endif /* __KCS_BMC_H__ */
diff --git a/drivers/char/ipmi/kcs_bmc_aspeed.c b/drivers/char/ipmi/kcs_bmc_aspeed.c
index a140203c079b..6451a8af2664 100644
--- a/drivers/char/ipmi/kcs_bmc_aspeed.c
+++ b/drivers/char/ipmi/kcs_bmc_aspeed.c
@@ -203,16 +203,6 @@ static void aspeed_kcs_enable_channel(struct kcs_bmc *kcs_bmc, bool enable)
 	}
 }
 
-static irqreturn_t aspeed_kcs_irq(int irq, void *arg)
-{
-	struct kcs_bmc *kcs_bmc = arg;
-
-	if (!kcs_bmc_handle_event(kcs_bmc))
-		return IRQ_HANDLED;
-
-	return IRQ_NONE;
-}
-
 static int aspeed_kcs_config_irq(struct kcs_bmc *kcs_bmc,
 			struct platform_device *pdev)
 {
@@ -223,7 +213,7 @@ static int aspeed_kcs_config_irq(struct kcs_bmc *kcs_bmc,
 	if (irq < 0)
 		return irq;
 
-	return devm_request_irq(dev, irq, aspeed_kcs_irq, IRQF_SHARED,
+	return devm_request_irq(dev, irq, kcs_bmc_irq, IRQF_SHARED,
 				dev_name(dev), kcs_bmc);
 }
 
diff --git a/drivers/char/ipmi/kcs_bmc_npcm7xx.c b/drivers/char/ipmi/kcs_bmc_npcm7xx.c
index 722f7391fe1f..f417813cf900 100644
--- a/drivers/char/ipmi/kcs_bmc_npcm7xx.c
+++ b/drivers/char/ipmi/kcs_bmc_npcm7xx.c
@@ -108,16 +108,6 @@ static void npcm7xx_kcs_enable_channel(struct kcs_bmc *kcs_bmc, bool enable)
 			   enable ? KCS_IE_IRQE | KCS_IE_HIRQE : 0);
 }
 
-static irqreturn_t npcm7xx_kcs_irq(int irq, void *arg)
-{
-	struct kcs_bmc *kcs_bmc = arg;
-
-	if (!kcs_bmc_handle_event(kcs_bmc))
-		return IRQ_HANDLED;
-
-	return IRQ_NONE;
-}
-
 static int npcm7xx_kcs_config_irq(struct kcs_bmc *kcs_bmc,
 				  struct platform_device *pdev)
 {
@@ -128,7 +118,7 @@ static int npcm7xx_kcs_config_irq(struct kcs_bmc *kcs_bmc,
 	if (irq < 0)
 		return irq;
 
-	return devm_request_irq(dev, irq, npcm7xx_kcs_irq, IRQF_SHARED,
+	return devm_request_irq(dev, irq, kcs_bmc_irq, IRQF_SHARED,
 				dev_name(dev), kcs_bmc);
 }
 
-- 
2.30.0.478.g8a0d178c01-goog

[PATCH 2/4] ipmi: kcs_bmc: Remove register abstraction

[William A. Kennington III]

The generic kcs_bmc code doesn't care about how byte reads / writes are
implemented. This pushes that logic to the device specific handlers, so
that kcs_bmc doesn't need to understand registers and how to read
or write them.

Signed-off-by: William A. Kennington III <wak@google.com>
---
 drivers/char/ipmi/kcs_bmc.c         |  60 +++++----------
 drivers/char/ipmi/kcs_bmc.h         |  13 +---
 drivers/char/ipmi/kcs_bmc_aspeed.c  | 110 +++++++++++++++++-----------
 drivers/char/ipmi/kcs_bmc_npcm7xx.c |  60 +++++++++------
 4 files changed, 130 insertions(+), 113 deletions(-)

diff --git a/drivers/char/ipmi/kcs_bmc.c b/drivers/char/ipmi/kcs_bmc.c
index ccb35f1645cf..aa2093323622 100644
--- a/drivers/char/ipmi/kcs_bmc.c
+++ b/drivers/char/ipmi/kcs_bmc.c
@@ -45,34 +45,12 @@ enum kcs_states {
 #define KCS_CMD_WRITE_END         0x62
 #define KCS_CMD_READ_BYTE         0x68
 
-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 = read_status(kcs_bmc);
-
+	u8 tmp = kcs_bmc->read_status(kcs_bmc);
 	tmp &= ~mask;
 	tmp |= val & mask;
-
-	write_status(kcs_bmc, tmp);
+	kcs_bmc->write_status(kcs_bmc, tmp);
 }
 
 static inline void set_state(struct kcs_bmc *kcs_bmc, u8 state)
@@ -84,8 +62,8 @@ static inline void set_state(struct kcs_bmc *kcs_bmc, u8 state)
 static void kcs_force_abort(struct kcs_bmc *kcs_bmc)
 {
 	set_state(kcs_bmc, ERROR_STATE);
-	read_data(kcs_bmc);
-	write_data(kcs_bmc, KCS_ZERO_DATA);
+	kcs_bmc->read_data_in(kcs_bmc);
+	kcs_bmc->write_data_out(kcs_bmc, KCS_ZERO_DATA);
 
 	kcs_bmc->phase = KCS_PHASE_ERROR;
 	kcs_bmc->data_in_avail = false;
@@ -104,9 +82,9 @@ static void kcs_bmc_handle_data(struct kcs_bmc *kcs_bmc)
 	case KCS_PHASE_WRITE_DATA:
 		if (kcs_bmc->data_in_idx < KCS_MSG_BUFSIZ) {
 			set_state(kcs_bmc, WRITE_STATE);
-			write_data(kcs_bmc, KCS_ZERO_DATA);
+			kcs_bmc->write_data_out(kcs_bmc, KCS_ZERO_DATA);
 			kcs_bmc->data_in[kcs_bmc->data_in_idx++] =
-						read_data(kcs_bmc);
+						kcs_bmc->read_data_in(kcs_bmc);
 		} else {
 			kcs_force_abort(kcs_bmc);
 			kcs_bmc->error = KCS_LENGTH_ERROR;
@@ -117,7 +95,7 @@ static void kcs_bmc_handle_data(struct kcs_bmc *kcs_bmc)
 		if (kcs_bmc->data_in_idx < KCS_MSG_BUFSIZ) {
 			set_state(kcs_bmc, READ_STATE);
 			kcs_bmc->data_in[kcs_bmc->data_in_idx++] =
-						read_data(kcs_bmc);
+						kcs_bmc->read_data_in(kcs_bmc);
 			kcs_bmc->phase = KCS_PHASE_WRITE_DONE;
 			kcs_bmc->data_in_avail = true;
 			wake_up_interruptible(&kcs_bmc->queue);
@@ -131,34 +109,34 @@ static void kcs_bmc_handle_data(struct kcs_bmc *kcs_bmc)
 		if (kcs_bmc->data_out_idx == kcs_bmc->data_out_len)
 			set_state(kcs_bmc, IDLE_STATE);
 
-		data = read_data(kcs_bmc);
+		data = kcs_bmc->read_data_in(kcs_bmc);
 		if (data != KCS_CMD_READ_BYTE) {
 			set_state(kcs_bmc, ERROR_STATE);
-			write_data(kcs_bmc, KCS_ZERO_DATA);
+			kcs_bmc->write_data_out(kcs_bmc, KCS_ZERO_DATA);
 			break;
 		}
 
 		if (kcs_bmc->data_out_idx == kcs_bmc->data_out_len) {
-			write_data(kcs_bmc, KCS_ZERO_DATA);
+			kcs_bmc->write_data_out(kcs_bmc, KCS_ZERO_DATA);
 			kcs_bmc->phase = KCS_PHASE_IDLE;
 			break;
 		}
 
-		write_data(kcs_bmc,
+		kcs_bmc->write_data_out(kcs_bmc,
 			kcs_bmc->data_out[kcs_bmc->data_out_idx++]);
 		break;
 
 	case KCS_PHASE_ABORT_ERROR1:
 		set_state(kcs_bmc, READ_STATE);
-		read_data(kcs_bmc);
-		write_data(kcs_bmc, kcs_bmc->error);
+		kcs_bmc->read_data_in(kcs_bmc);
+		kcs_bmc->write_data_out(kcs_bmc, kcs_bmc->error);
 		kcs_bmc->phase = KCS_PHASE_ABORT_ERROR2;
 		break;
 
 	case KCS_PHASE_ABORT_ERROR2:
 		set_state(kcs_bmc, IDLE_STATE);
-		read_data(kcs_bmc);
-		write_data(kcs_bmc, KCS_ZERO_DATA);
+		kcs_bmc->read_data_in(kcs_bmc);
+		kcs_bmc->write_data_out(kcs_bmc, KCS_ZERO_DATA);
 		kcs_bmc->phase = KCS_PHASE_IDLE;
 		break;
 
@@ -173,9 +151,9 @@ static void kcs_bmc_handle_cmd(struct kcs_bmc *kcs_bmc)
 	u8 cmd;
 
 	set_state(kcs_bmc, WRITE_STATE);
-	write_data(kcs_bmc, KCS_ZERO_DATA);
+	kcs_bmc->write_data_out(kcs_bmc, KCS_ZERO_DATA);
 
-	cmd = read_data(kcs_bmc);
+	cmd = kcs_bmc->read_data_in(kcs_bmc);
 	switch (cmd) {
 	case KCS_CMD_WRITE_START:
 		kcs_bmc->phase = KCS_PHASE_WRITE_START;
@@ -218,7 +196,7 @@ irqreturn_t kcs_bmc_irq(int irq, void *arg)
 
 	spin_lock_irqsave(&kcs_bmc->lock, flags);
 
-	status = read_status(kcs_bmc);
+	status = kcs_bmc->read_status(kcs_bmc);
 	if (status & KCS_STATUS_IBF) {
 		if (!kcs_bmc->running)
 			kcs_force_abort(kcs_bmc);
@@ -355,7 +333,7 @@ static ssize_t kcs_bmc_write(struct file *filp, const char __user *buf,
 		kcs_bmc->data_out_idx = 1;
 		kcs_bmc->data_out_len = count;
 		memcpy(kcs_bmc->data_out, kcs_bmc->kbuffer, count);
-		write_data(kcs_bmc, kcs_bmc->data_out[0]);
+		kcs_bmc->write_data_out(kcs_bmc, kcs_bmc->data_out[0]);
 		ret = count;
 	} else {
 		ret = -EINVAL;
diff --git a/drivers/char/ipmi/kcs_bmc.h b/drivers/char/ipmi/kcs_bmc.h
index 959d7042e6d2..8c541251fe97 100644
--- a/drivers/char/ipmi/kcs_bmc.h
+++ b/drivers/char/ipmi/kcs_bmc.h
@@ -61,12 +61,6 @@ enum kcs_errors {
  * @odr: Output Data Register
  * @str: Status Register
  */
-struct kcs_ioreg {
-	u32 idr;
-	u32 odr;
-	u32 str;
-};
-
 struct kcs_bmc {
 	spinlock_t lock;
 
@@ -74,9 +68,10 @@ struct kcs_bmc {
 	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);
+	u8 (*read_status)(struct kcs_bmc *kcs_bmc);
+	void (*write_status)(struct kcs_bmc *kcs_bmc, u8 b);
+	u8 (*read_data_in)(struct kcs_bmc *kcs_bmc);
+	void (*write_data_out)(struct kcs_bmc *kcs_bmc, u8 b);
 
 	enum kcs_phases phase;
 	enum kcs_errors error;
diff --git a/drivers/char/ipmi/kcs_bmc_aspeed.c b/drivers/char/ipmi/kcs_bmc_aspeed.c
index 6451a8af2664..d3b771e4d039 100644
--- a/drivers/char/ipmi/kcs_bmc_aspeed.c
+++ b/drivers/char/ipmi/kcs_bmc_aspeed.c
@@ -62,30 +62,55 @@
 #define LPC_ODR4             0x098
 #define LPC_STR4             0x09C
 
-struct aspeed_kcs_bmc {
+struct aspeed_kcs_reg {
+	u32 idr;
+	u32 odr;
+	u32 str;
+};
+
+struct aspeed_kcs {
 	struct regmap *map;
+
+	const struct aspeed_kcs_reg *reg;
 };
 
 
-static u8 aspeed_kcs_inb(struct kcs_bmc *kcs_bmc, u32 reg)
+static u8 aspeed_kcs_inb(struct aspeed_kcs *aspeed_kcs, u32 reg)
 {
-	struct aspeed_kcs_bmc *priv = kcs_bmc_priv(kcs_bmc);
 	u32 val = 0;
-	int rc;
-
-	rc = regmap_read(priv->map, reg, &val);
+	int rc = regmap_read(aspeed_kcs->map, reg, &val);
 	WARN(rc != 0, "regmap_read() failed: %d\n", rc);
+	return val;
+}
 
-	return rc == 0 ? (u8) val : 0;
+static void aspeed_kcs_outb(struct aspeed_kcs *aspeed_kcs, u32 reg, u8 data)
+{
+	int rc = regmap_write(aspeed_kcs->map, reg, data);
+	WARN(rc != 0, "regmap_write() failed: %d\n", rc);
 }
 
-static void aspeed_kcs_outb(struct kcs_bmc *kcs_bmc, u32 reg, u8 data)
+static u8 aspeed_kcs_read_status(struct kcs_bmc *kcs_bmc)
 {
-	struct aspeed_kcs_bmc *priv = kcs_bmc_priv(kcs_bmc);
-	int rc;
+	struct aspeed_kcs *aspeed_kcs = kcs_bmc_priv(kcs_bmc);
+	return aspeed_kcs_inb(aspeed_kcs, aspeed_kcs->reg->str);
+}
 
-	rc = regmap_write(priv->map, reg, data);
-	WARN(rc != 0, "regmap_write() failed: %d\n", rc);
+static void aspeed_kcs_write_status(struct kcs_bmc *kcs_bmc, u8 data)
+{
+	struct aspeed_kcs *aspeed_kcs = kcs_bmc_priv(kcs_bmc);
+	aspeed_kcs_outb(aspeed_kcs, aspeed_kcs->reg->str, data);
+}
+
+static u8 aspeed_kcs_read_data_in(struct kcs_bmc *kcs_bmc)
+{
+	struct aspeed_kcs *aspeed_kcs = kcs_bmc_priv(kcs_bmc);
+	return aspeed_kcs_inb(aspeed_kcs, aspeed_kcs->reg->idr);
+}
+
+static void aspeed_kcs_write_data_out(struct kcs_bmc *kcs_bmc, u8 data)
+{
+	struct aspeed_kcs *aspeed_kcs = kcs_bmc_priv(kcs_bmc);
+	aspeed_kcs_outb(aspeed_kcs, aspeed_kcs->reg->odr, data);
 }
 
 
@@ -104,7 +129,7 @@ static void aspeed_kcs_outb(struct kcs_bmc *kcs_bmc, u32 reg, u8 data)
  */
 static void aspeed_kcs_set_address(struct kcs_bmc *kcs_bmc, u16 addr)
 {
-	struct aspeed_kcs_bmc *priv = kcs_bmc_priv(kcs_bmc);
+	struct aspeed_kcs *priv = kcs_bmc_priv(kcs_bmc);
 
 	switch (kcs_bmc->channel) {
 	case 1:
@@ -138,7 +163,7 @@ static void aspeed_kcs_set_address(struct kcs_bmc *kcs_bmc, u16 addr)
 
 static void aspeed_kcs_enable_channel(struct kcs_bmc *kcs_bmc, bool enable)
 {
-	struct aspeed_kcs_bmc *priv = kcs_bmc_priv(kcs_bmc);
+	struct aspeed_kcs *priv = kcs_bmc_priv(kcs_bmc);
 
 	switch (kcs_bmc->channel) {
 	case 1:
@@ -217,7 +242,7 @@ static int aspeed_kcs_config_irq(struct kcs_bmc *kcs_bmc,
 				dev_name(dev), kcs_bmc);
 }
 
-static const struct kcs_ioreg ast_kcs_bmc_ioregs[KCS_CHANNEL_MAX] = {
+static const struct aspeed_kcs_reg aspeed_kcs_regs[KCS_CHANNEL_MAX] = {
 	{ .idr = LPC_IDR1, .odr = LPC_ODR1, .str = LPC_STR1 },
 	{ .idr = LPC_IDR2, .odr = LPC_ODR2, .str = LPC_STR2 },
 	{ .idr = LPC_IDR3, .odr = LPC_ODR3, .str = LPC_STR3 },
@@ -226,7 +251,7 @@ static const struct kcs_ioreg ast_kcs_bmc_ioregs[KCS_CHANNEL_MAX] = {
 
 static struct kcs_bmc *aspeed_kcs_probe_of_v1(struct platform_device *pdev)
 {
-	struct aspeed_kcs_bmc *priv;
+	struct aspeed_kcs *priv;
 	struct device_node *np;
 	struct kcs_bmc *kcs;
 	u32 channel;
@@ -241,7 +266,7 @@ static struct kcs_bmc *aspeed_kcs_probe_of_v1(struct platform_device *pdev)
 		return ERR_PTR(-EINVAL);
 	}
 
-	kcs = kcs_bmc_alloc(&pdev->dev, sizeof(struct aspeed_kcs_bmc), channel);
+	kcs = kcs_bmc_alloc(&pdev->dev, sizeof(struct aspeed_kcs), channel);
 	if (!kcs)
 		return ERR_PTR(-ENOMEM);
 
@@ -258,18 +283,18 @@ static struct kcs_bmc *aspeed_kcs_probe_of_v1(struct platform_device *pdev)
 		return ERR_PTR(-EINVAL);
 	}
 
-	kcs->ioreg = ast_kcs_bmc_ioregs[channel - 1];
+	priv->reg = &aspeed_kcs_regs[channel - 1];
 	aspeed_kcs_set_address(kcs, slave);
 
 	return kcs;
 }
 
-static int aspeed_kcs_calculate_channel(const struct kcs_ioreg *regs)
+static int aspeed_kcs_calculate_channel(const struct aspeed_kcs_reg *reg)
 {
 	int i;
 
-	for (i = 0; i < ARRAY_SIZE(ast_kcs_bmc_ioregs); i++) {
-		if (!memcmp(&ast_kcs_bmc_ioregs[i], regs, sizeof(*regs)))
+	for (i = 0; i < ARRAY_SIZE(aspeed_kcs_regs); i++) {
+		if (!memcmp(&aspeed_kcs_regs[i], reg, sizeof(*reg)))
 			return i + 1;
 	}
 
@@ -278,11 +303,11 @@ static int aspeed_kcs_calculate_channel(const struct kcs_ioreg *regs)
 
 static struct kcs_bmc *aspeed_kcs_probe_of_v2(struct platform_device *pdev)
 {
-	struct aspeed_kcs_bmc *priv;
+	struct aspeed_kcs *priv;
 	struct device_node *np;
-	struct kcs_ioreg ioreg;
+	struct aspeed_kcs_reg reg;
 	struct kcs_bmc *kcs;
-	const __be32 *reg;
+	const __be32 *of_reg;
 	int channel;
 	u32 slave;
 	int rc;
@@ -290,32 +315,31 @@ static struct kcs_bmc *aspeed_kcs_probe_of_v2(struct platform_device *pdev)
 	np = pdev->dev.of_node;
 
 	/* Don't translate addresses, we want offsets for the regmaps */
-	reg = of_get_address(np, 0, NULL, NULL);
-	if (!reg)
+	of_reg = of_get_address(np, 0, NULL, NULL);
+	if (!of_reg)
 		return ERR_PTR(-EINVAL);
-	ioreg.idr = be32_to_cpup(reg);
+	reg.idr = be32_to_cpup(of_reg);
 
-	reg = of_get_address(np, 1, NULL, NULL);
-	if (!reg)
+	of_reg = of_get_address(np, 1, NULL, NULL);
+	if (!of_reg)
 		return ERR_PTR(-EINVAL);
-	ioreg.odr = be32_to_cpup(reg);
+	reg.odr = be32_to_cpup(of_reg);
 
-	reg = of_get_address(np, 2, NULL, NULL);
-	if (!reg)
+	of_reg = of_get_address(np, 2, NULL, NULL);
+	if (!of_reg)
 		return ERR_PTR(-EINVAL);
-	ioreg.str = be32_to_cpup(reg);
+	reg.str = be32_to_cpup(of_reg);
 
-	channel = aspeed_kcs_calculate_channel(&ioreg);
+	channel = aspeed_kcs_calculate_channel(&reg);
 	if (channel < 0)
 		return ERR_PTR(channel);
 
-	kcs = kcs_bmc_alloc(&pdev->dev, sizeof(struct aspeed_kcs_bmc), channel);
+	kcs = kcs_bmc_alloc(&pdev->dev, sizeof(struct aspeed_kcs), channel);
 	if (!kcs)
 		return ERR_PTR(-ENOMEM);
 
-	kcs->ioreg = ioreg;
-
 	priv = kcs_bmc_priv(kcs);
+	priv->reg = &aspeed_kcs_regs[channel - 1];
 	priv->map = syscon_node_to_regmap(pdev->dev.parent->of_node);
 	if (IS_ERR(priv->map)) {
 		dev_err(&pdev->dev, "Couldn't get regmap\n");
@@ -335,6 +359,7 @@ static int aspeed_kcs_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
 	struct kcs_bmc *kcs_bmc;
+	struct aspeed_kcs *priv;
 	struct device_node *np;
 	int rc;
 
@@ -351,8 +376,10 @@ static int aspeed_kcs_probe(struct platform_device *pdev)
 	if (IS_ERR(kcs_bmc))
 		return PTR_ERR(kcs_bmc);
 
-	kcs_bmc->io_inputb = aspeed_kcs_inb;
-	kcs_bmc->io_outputb = aspeed_kcs_outb;
+	kcs_bmc->read_status = aspeed_kcs_read_status;
+	kcs_bmc->write_status = aspeed_kcs_write_status;
+	kcs_bmc->read_data_in = aspeed_kcs_read_data_in;
+	kcs_bmc->write_data_out = aspeed_kcs_write_data_out;
 
 	rc = aspeed_kcs_config_irq(kcs_bmc, pdev);
 	if (rc)
@@ -368,10 +395,11 @@ static int aspeed_kcs_probe(struct platform_device *pdev)
 		return rc;
 	}
 
+	priv = kcs_bmc_priv(kcs_bmc);
 	dev_dbg(&pdev->dev,
 		"Probed KCS device %d (IDR=0x%x, ODR=0x%x, STR=0x%x)\n",
-		kcs_bmc->channel, kcs_bmc->ioreg.idr, kcs_bmc->ioreg.odr,
-		kcs_bmc->ioreg.str);
+		kcs_bmc->channel, priv->reg->idr, priv->reg->odr,
+		priv->reg->str);
 
 	return 0;
 }
diff --git a/drivers/char/ipmi/kcs_bmc_npcm7xx.c b/drivers/char/ipmi/kcs_bmc_npcm7xx.c
index f417813cf900..572501f7da71 100644
--- a/drivers/char/ipmi/kcs_bmc_npcm7xx.c
+++ b/drivers/char/ipmi/kcs_bmc_npcm7xx.c
@@ -64,7 +64,7 @@ struct npcm7xx_kcs_reg {
 	u32 ie;
 };
 
-struct npcm7xx_kcs_bmc {
+struct npcm7xx_kcs {
 	struct regmap *map;
 
 	const struct npcm7xx_kcs_reg *reg;
@@ -76,30 +76,47 @@ static const struct npcm7xx_kcs_reg npcm7xx_kcs_reg_tbl[KCS_CHANNEL_MAX] = {
 	{ .sts = KCS3ST, .dob = KCS3DO, .dib = KCS3DI, .ctl = KCS3CTL, .ie = KCS3IE },
 };
 
-static u8 npcm7xx_kcs_inb(struct kcs_bmc *kcs_bmc, u32 reg)
+static u8 npcm7xx_kcs_inb(struct npcm7xx_kcs *npcm7xx_kcs, u32 reg)
 {
-	struct npcm7xx_kcs_bmc *priv = kcs_bmc_priv(kcs_bmc);
 	u32 val = 0;
-	int rc;
-
-	rc = regmap_read(priv->map, reg, &val);
+	int rc = regmap_read(npcm7xx_kcs->map, reg, &val);
 	WARN(rc != 0, "regmap_read() failed: %d\n", rc);
+	return val;
+}
 
-	return rc == 0 ? (u8)val : 0;
+static void npcm7xx_kcs_outb(struct npcm7xx_kcs *npcm7xx_kcs, u32 reg, u8 data)
+{
+	int rc = regmap_write(npcm7xx_kcs->map, reg, data);
+	WARN(rc != 0, "regmap_write() failed: %d\n", rc);
 }
 
-static void npcm7xx_kcs_outb(struct kcs_bmc *kcs_bmc, u32 reg, u8 data)
+static u8 npcm7xx_kcs_read_status(struct kcs_bmc *kcs_bmc)
 {
-	struct npcm7xx_kcs_bmc *priv = kcs_bmc_priv(kcs_bmc);
-	int rc;
+	struct npcm7xx_kcs *npcm7xx_kcs = kcs_bmc_priv(kcs_bmc);
+	return npcm7xx_kcs_inb(npcm7xx_kcs, npcm7xx_kcs->reg->sts);
+}
 
-	rc = regmap_write(priv->map, reg, data);
-	WARN(rc != 0, "regmap_write() failed: %d\n", rc);
+static void npcm7xx_kcs_write_status(struct kcs_bmc *kcs_bmc, u8 data)
+{
+	struct npcm7xx_kcs *npcm7xx_kcs = kcs_bmc_priv(kcs_bmc);
+	npcm7xx_kcs_outb(npcm7xx_kcs, npcm7xx_kcs->reg->sts, data);
+}
+
+static u8 npcm7xx_kcs_read_data_in(struct kcs_bmc *kcs_bmc)
+{
+	struct npcm7xx_kcs *npcm7xx_kcs = kcs_bmc_priv(kcs_bmc);
+	return npcm7xx_kcs_inb(npcm7xx_kcs, npcm7xx_kcs->reg->dib);
+}
+
+static void npcm7xx_kcs_write_data_out(struct kcs_bmc *kcs_bmc, u8 data)
+{
+	struct npcm7xx_kcs *npcm7xx_kcs = kcs_bmc_priv(kcs_bmc);
+	npcm7xx_kcs_outb(npcm7xx_kcs, npcm7xx_kcs->reg->dob, data);
 }
 
 static void npcm7xx_kcs_enable_channel(struct kcs_bmc *kcs_bmc, bool enable)
 {
-	struct npcm7xx_kcs_bmc *priv = kcs_bmc_priv(kcs_bmc);
+	struct npcm7xx_kcs *priv = kcs_bmc_priv(kcs_bmc);
 
 	regmap_update_bits(priv->map, priv->reg->ctl, KCS_CTL_IBFIE,
 			   enable ? KCS_CTL_IBFIE : 0);
@@ -125,7 +142,7 @@ static int npcm7xx_kcs_config_irq(struct kcs_bmc *kcs_bmc,
 static int npcm7xx_kcs_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
-	struct npcm7xx_kcs_bmc *priv;
+	struct npcm7xx_kcs *priv;
 	struct kcs_bmc *kcs_bmc;
 	u32 chan;
 	int rc;
@@ -148,11 +165,10 @@ static int npcm7xx_kcs_probe(struct platform_device *pdev)
 	}
 	priv->reg = &npcm7xx_kcs_reg_tbl[chan - 1];
 
-	kcs_bmc->ioreg.idr = priv->reg->dib;
-	kcs_bmc->ioreg.odr = priv->reg->dob;
-	kcs_bmc->ioreg.str = priv->reg->sts;
-	kcs_bmc->io_inputb = npcm7xx_kcs_inb;
-	kcs_bmc->io_outputb = npcm7xx_kcs_outb;
+	kcs_bmc->read_status = npcm7xx_kcs_read_status;
+	kcs_bmc->write_status = npcm7xx_kcs_write_status;
+	kcs_bmc->read_data_in = npcm7xx_kcs_read_data_in;
+	kcs_bmc->write_data_out = npcm7xx_kcs_write_data_out;
 
 	dev_set_drvdata(dev, kcs_bmc);
 
@@ -167,9 +183,9 @@ static int npcm7xx_kcs_probe(struct platform_device *pdev)
 		return rc;
 	}
 
-	pr_info("channel=%u idr=0x%x odr=0x%x str=0x%x\n",
-		chan,
-		kcs_bmc->ioreg.idr, kcs_bmc->ioreg.odr, kcs_bmc->ioreg.str);
+	dev_dbg(&pdev->dev,
+		"Probed KCS device %d (DIB=0x%x, DOB=0x%x, STS=0x%x )\n",
+		chan, priv->reg->dib, priv->reg->dob, priv->reg->sts);
 
 	return 0;
 }
-- 
2.30.0.478.g8a0d178c01-goog

[PATCH 3/4] ipmi: kcs_bmc: Invert allocation pattern

[William A. Kennington III]

The generic kcs_bmc code shouldn't need to store data on behalf of the
platform specific driver that initializes it. Instead, have the platform
specific driver allocate memory and call a generic initialization
routine.

Signed-off-by: William A. Kennington III <wak@google.com>
---
 drivers/char/ipmi/kcs_bmc.c         |  40 +++---
 drivers/char/ipmi/kcs_bmc.h         |  13 +-
 drivers/char/ipmi/kcs_bmc_aspeed.c  | 216 ++++++++++++++--------------
 drivers/char/ipmi/kcs_bmc_npcm7xx.c |  61 ++++----
 4 files changed, 163 insertions(+), 167 deletions(-)

diff --git a/drivers/char/ipmi/kcs_bmc.c b/drivers/char/ipmi/kcs_bmc.c
index aa2093323622..16a378d79db9 100644
--- a/drivers/char/ipmi/kcs_bmc.c
+++ b/drivers/char/ipmi/kcs_bmc.c
@@ -276,9 +276,6 @@ static ssize_t kcs_bmc_read(struct file *filp, char __user *buf,
 	}
 
 	if (count < data_len) {
-		pr_err("channel=%u with too large data : %zu\n",
-			kcs_bmc->channel, data_len);
-
 		spin_lock_irq(&kcs_bmc->lock);
 		kcs_force_abort(kcs_bmc);
 		spin_unlock_irq(&kcs_bmc->lock);
@@ -401,17 +398,11 @@ static const struct file_operations kcs_bmc_fops = {
 	.unlocked_ioctl = kcs_bmc_ioctl,
 };
 
-struct kcs_bmc *kcs_bmc_alloc(struct device *dev, int sizeof_priv, u32 channel)
+int kcs_bmc_init(struct kcs_bmc *kcs_bmc, struct device *dev, u32 channel)
 {
-	struct kcs_bmc *kcs_bmc;
-
-	kcs_bmc = devm_kzalloc(dev, sizeof(*kcs_bmc) + sizeof_priv, GFP_KERNEL);
-	if (!kcs_bmc)
-		return NULL;
+	int rc;
 
 	spin_lock_init(&kcs_bmc->lock);
-	kcs_bmc->channel = channel;
-
 	mutex_init(&kcs_bmc->mutex);
 	init_waitqueue_head(&kcs_bmc->queue);
 
@@ -419,17 +410,28 @@ struct kcs_bmc *kcs_bmc_alloc(struct device *dev, int sizeof_priv, u32 channel)
 	kcs_bmc->data_out = devm_kmalloc(dev, KCS_MSG_BUFSIZ, GFP_KERNEL);
 	kcs_bmc->kbuffer = devm_kmalloc(dev, KCS_MSG_BUFSIZ, GFP_KERNEL);
 
-	kcs_bmc->miscdev.minor = MISC_DYNAMIC_MINOR;
-	kcs_bmc->miscdev.name = devm_kasprintf(dev, GFP_KERNEL, "%s%u",
-					       DEVICE_NAME, channel);
-	if (!kcs_bmc->data_in || !kcs_bmc->data_out || !kcs_bmc->kbuffer ||
-	    !kcs_bmc->miscdev.name)
-		return NULL;
 	kcs_bmc->miscdev.fops = &kcs_bmc_fops;
+	kcs_bmc->miscdev.minor = MISC_DYNAMIC_MINOR;
+	kcs_bmc->miscdev.name =
+		devm_kasprintf(dev, GFP_KERNEL, "%s%u", DEVICE_NAME, channel);
+	if (!kcs_bmc->miscdev.name)
+		return -ENOMEM;
+
+	rc = misc_register(&kcs_bmc->miscdev);
+	if (rc) {
+		dev_err(dev, "Registering kcs_bmc: %d\n", rc);
+		return rc;
+	}
 
-	return kcs_bmc;
+	return 0;
+}
+EXPORT_SYMBOL(kcs_bmc_init);
+
+void kcs_bmc_stop(struct kcs_bmc *kcs_bmc)
+{
+	misc_deregister(&kcs_bmc->miscdev);
 }
-EXPORT_SYMBOL(kcs_bmc_alloc);
+EXPORT_SYMBOL(kcs_bmc_stop);
 
 MODULE_LICENSE("GPL v2");
 MODULE_AUTHOR("Haiyue Wang <haiyue.wang@linux.intel.com>");
diff --git a/drivers/char/ipmi/kcs_bmc.h b/drivers/char/ipmi/kcs_bmc.h
index 8c541251fe97..d65ffd479e9b 100644
--- a/drivers/char/ipmi/kcs_bmc.h
+++ b/drivers/char/ipmi/kcs_bmc.h
@@ -64,7 +64,6 @@ enum kcs_errors {
 struct kcs_bmc {
 	spinlock_t lock;
 
-	u32 channel;
 	int running;
 
 	/* Setup by BMC KCS controller driver */
@@ -89,16 +88,10 @@ struct kcs_bmc {
 	u8 *kbuffer;
 
 	struct miscdevice miscdev;
-
-	unsigned long priv[];
 };
 
-static inline void *kcs_bmc_priv(struct kcs_bmc *kcs_bmc)
-{
-	return kcs_bmc->priv;
-}
-
 irqreturn_t kcs_bmc_irq(int irq, void *arg);
-struct kcs_bmc *kcs_bmc_alloc(struct device *dev, int sizeof_priv,
-					u32 channel);
+int kcs_bmc_init(struct kcs_bmc *kcs_bmc, struct device *dev, u32 channel);
+void kcs_bmc_stop(struct kcs_bmc *kcs_bmc);
+
 #endif /* __KCS_BMC_H__ */
diff --git a/drivers/char/ipmi/kcs_bmc_aspeed.c b/drivers/char/ipmi/kcs_bmc_aspeed.c
index d3b771e4d039..b466569d049d 100644
--- a/drivers/char/ipmi/kcs_bmc_aspeed.c
+++ b/drivers/char/ipmi/kcs_bmc_aspeed.c
@@ -69,11 +69,29 @@ struct aspeed_kcs_reg {
 };
 
 struct aspeed_kcs {
+	struct kcs_bmc kcs_bmc;
+
+	u32 channel;
 	struct regmap *map;
+};
 
-	const struct aspeed_kcs_reg *reg;
+static const struct aspeed_kcs_reg aspeed_kcs_regs[KCS_CHANNEL_MAX] = {
+	{ .idr = LPC_IDR1, .odr = LPC_ODR1, .str = LPC_STR1 },
+	{ .idr = LPC_IDR2, .odr = LPC_ODR2, .str = LPC_STR2 },
+	{ .idr = LPC_IDR3, .odr = LPC_ODR3, .str = LPC_STR3 },
+	{ .idr = LPC_IDR4, .odr = LPC_ODR4, .str = LPC_STR4 },
 };
 
+static struct aspeed_kcs *to_aspeed_kcs(struct kcs_bmc *kcs_bmc)
+{
+	return container_of(kcs_bmc, struct aspeed_kcs, kcs_bmc);
+}
+
+static const struct aspeed_kcs_reg *to_aspeed_kcs_reg(
+		const struct aspeed_kcs *aspeed_kcs)
+{
+	return &aspeed_kcs_regs[aspeed_kcs->channel-1];
+}
 
 static u8 aspeed_kcs_inb(struct aspeed_kcs *aspeed_kcs, u32 reg)
 {
@@ -91,26 +109,26 @@ static void aspeed_kcs_outb(struct aspeed_kcs *aspeed_kcs, u32 reg, u8 data)
 
 static u8 aspeed_kcs_read_status(struct kcs_bmc *kcs_bmc)
 {
-	struct aspeed_kcs *aspeed_kcs = kcs_bmc_priv(kcs_bmc);
-	return aspeed_kcs_inb(aspeed_kcs, aspeed_kcs->reg->str);
+	struct aspeed_kcs *aspeed_kcs = to_aspeed_kcs(kcs_bmc);
+	return aspeed_kcs_inb(aspeed_kcs, to_aspeed_kcs_reg(aspeed_kcs)->str);
 }
 
 static void aspeed_kcs_write_status(struct kcs_bmc *kcs_bmc, u8 data)
 {
-	struct aspeed_kcs *aspeed_kcs = kcs_bmc_priv(kcs_bmc);
-	aspeed_kcs_outb(aspeed_kcs, aspeed_kcs->reg->str, data);
+	struct aspeed_kcs *aspeed_kcs = to_aspeed_kcs(kcs_bmc);
+	aspeed_kcs_outb(aspeed_kcs, to_aspeed_kcs_reg(aspeed_kcs)->str, data);
 }
 
 static u8 aspeed_kcs_read_data_in(struct kcs_bmc *kcs_bmc)
 {
-	struct aspeed_kcs *aspeed_kcs = kcs_bmc_priv(kcs_bmc);
-	return aspeed_kcs_inb(aspeed_kcs, aspeed_kcs->reg->idr);
+	struct aspeed_kcs *aspeed_kcs = to_aspeed_kcs(kcs_bmc);
+	return aspeed_kcs_inb(aspeed_kcs, to_aspeed_kcs_reg(aspeed_kcs)->idr);
 }
 
 static void aspeed_kcs_write_data_out(struct kcs_bmc *kcs_bmc, u8 data)
 {
-	struct aspeed_kcs *aspeed_kcs = kcs_bmc_priv(kcs_bmc);
-	aspeed_kcs_outb(aspeed_kcs, aspeed_kcs->reg->odr, data);
+	struct aspeed_kcs *aspeed_kcs = to_aspeed_kcs(kcs_bmc);
+	aspeed_kcs_outb(aspeed_kcs, to_aspeed_kcs_reg(aspeed_kcs)->odr, data);
 }
 
 
@@ -127,32 +145,31 @@ static void aspeed_kcs_write_data_out(struct kcs_bmc *kcs_bmc, u8 data)
  *     C. KCS4
  *        D / C : CA4h / CA5h
  */
-static void aspeed_kcs_set_address(struct kcs_bmc *kcs_bmc, u16 addr)
+static void aspeed_kcs_set_address(
+		struct aspeed_kcs *aspeed_kcs, u16 addr)
 {
-	struct aspeed_kcs *priv = kcs_bmc_priv(kcs_bmc);
-
-	switch (kcs_bmc->channel) {
+	switch (aspeed_kcs->channel) {
 	case 1:
-		regmap_update_bits(priv->map, LPC_HICR4,
+		regmap_update_bits(aspeed_kcs->map, LPC_HICR4,
 				LPC_HICR4_LADR12AS, 0);
-		regmap_write(priv->map, LPC_LADR12H, addr >> 8);
-		regmap_write(priv->map, LPC_LADR12L, addr & 0xFF);
+		regmap_write(aspeed_kcs->map, LPC_LADR12H, addr >> 8);
+		regmap_write(aspeed_kcs->map, LPC_LADR12L, addr & 0xFF);
 		break;
 
 	case 2:
-		regmap_update_bits(priv->map, LPC_HICR4,
+		regmap_update_bits(aspeed_kcs->map, LPC_HICR4,
 				LPC_HICR4_LADR12AS, LPC_HICR4_LADR12AS);
-		regmap_write(priv->map, LPC_LADR12H, addr >> 8);
-		regmap_write(priv->map, LPC_LADR12L, addr & 0xFF);
+		regmap_write(aspeed_kcs->map, LPC_LADR12H, addr >> 8);
+		regmap_write(aspeed_kcs->map, LPC_LADR12L, addr & 0xFF);
 		break;
 
 	case 3:
-		regmap_write(priv->map, LPC_LADR3H, addr >> 8);
-		regmap_write(priv->map, LPC_LADR3L, addr & 0xFF);
+		regmap_write(aspeed_kcs->map, LPC_LADR3H, addr >> 8);
+		regmap_write(aspeed_kcs->map, LPC_LADR3L, addr & 0xFF);
 		break;
 
 	case 4:
-		regmap_write(priv->map, LPC_LADR4, ((addr + 1) << 16) |
+		regmap_write(aspeed_kcs->map, LPC_LADR4, ((addr + 1) << 16) |
 			addr);
 		break;
 
@@ -161,64 +178,63 @@ static void aspeed_kcs_set_address(struct kcs_bmc *kcs_bmc, u16 addr)
 	}
 }
 
-static void aspeed_kcs_enable_channel(struct kcs_bmc *kcs_bmc, bool enable)
+static void aspeed_kcs_enable_channel(
+		struct aspeed_kcs *aspeed_kcs, bool enable)
 {
-	struct aspeed_kcs *priv = kcs_bmc_priv(kcs_bmc);
-
-	switch (kcs_bmc->channel) {
+	switch (aspeed_kcs->channel) {
 	case 1:
 		if (enable) {
-			regmap_update_bits(priv->map, LPC_HICR2,
+			regmap_update_bits(aspeed_kcs->map, LPC_HICR2,
 					LPC_HICR2_IBFIF1, LPC_HICR2_IBFIF1);
-			regmap_update_bits(priv->map, LPC_HICR0,
+			regmap_update_bits(aspeed_kcs->map, LPC_HICR0,
 					LPC_HICR0_LPC1E, LPC_HICR0_LPC1E);
 		} else {
-			regmap_update_bits(priv->map, LPC_HICR0,
+			regmap_update_bits(aspeed_kcs->map, LPC_HICR0,
 					LPC_HICR0_LPC1E, 0);
-			regmap_update_bits(priv->map, LPC_HICR2,
+			regmap_update_bits(aspeed_kcs->map, LPC_HICR2,
 					LPC_HICR2_IBFIF1, 0);
 		}
 		break;
 
 	case 2:
 		if (enable) {
-			regmap_update_bits(priv->map, LPC_HICR2,
+			regmap_update_bits(aspeed_kcs->map, LPC_HICR2,
 					LPC_HICR2_IBFIF2, LPC_HICR2_IBFIF2);
-			regmap_update_bits(priv->map, LPC_HICR0,
+			regmap_update_bits(aspeed_kcs->map, LPC_HICR0,
 					LPC_HICR0_LPC2E, LPC_HICR0_LPC2E);
 		} else {
-			regmap_update_bits(priv->map, LPC_HICR0,
+			regmap_update_bits(aspeed_kcs->map, LPC_HICR0,
 					LPC_HICR0_LPC2E, 0);
-			regmap_update_bits(priv->map, LPC_HICR2,
+			regmap_update_bits(aspeed_kcs->map, LPC_HICR2,
 					LPC_HICR2_IBFIF2, 0);
 		}
 		break;
 
 	case 3:
 		if (enable) {
-			regmap_update_bits(priv->map, LPC_HICR2,
+			regmap_update_bits(aspeed_kcs->map, LPC_HICR2,
 					LPC_HICR2_IBFIF3, LPC_HICR2_IBFIF3);
-			regmap_update_bits(priv->map, LPC_HICR0,
+			regmap_update_bits(aspeed_kcs->map, LPC_HICR0,
 					LPC_HICR0_LPC3E, LPC_HICR0_LPC3E);
-			regmap_update_bits(priv->map, LPC_HICR4,
+			regmap_update_bits(aspeed_kcs->map, LPC_HICR4,
 					LPC_HICR4_KCSENBL, LPC_HICR4_KCSENBL);
 		} else {
-			regmap_update_bits(priv->map, LPC_HICR0,
+			regmap_update_bits(aspeed_kcs->map, LPC_HICR0,
 					LPC_HICR0_LPC3E, 0);
-			regmap_update_bits(priv->map, LPC_HICR4,
+			regmap_update_bits(aspeed_kcs->map, LPC_HICR4,
 					LPC_HICR4_KCSENBL, 0);
-			regmap_update_bits(priv->map, LPC_HICR2,
+			regmap_update_bits(aspeed_kcs->map, LPC_HICR2,
 					LPC_HICR2_IBFIF3, 0);
 		}
 		break;
 
 	case 4:
 		if (enable)
-			regmap_update_bits(priv->map, LPC_HICRB,
+			regmap_update_bits(aspeed_kcs->map, LPC_HICRB,
 					LPC_HICRB_IBFIF4 | LPC_HICRB_LPC4E,
 					LPC_HICRB_IBFIF4 | LPC_HICRB_LPC4E);
 		else
-			regmap_update_bits(priv->map, LPC_HICRB,
+			regmap_update_bits(aspeed_kcs->map, LPC_HICRB,
 					LPC_HICRB_IBFIF4 | LPC_HICRB_LPC4E,
 					0);
 		break;
@@ -242,18 +258,10 @@ static int aspeed_kcs_config_irq(struct kcs_bmc *kcs_bmc,
 				dev_name(dev), kcs_bmc);
 }
 
-static const struct aspeed_kcs_reg aspeed_kcs_regs[KCS_CHANNEL_MAX] = {
-	{ .idr = LPC_IDR1, .odr = LPC_ODR1, .str = LPC_STR1 },
-	{ .idr = LPC_IDR2, .odr = LPC_ODR2, .str = LPC_STR2 },
-	{ .idr = LPC_IDR3, .odr = LPC_ODR3, .str = LPC_STR3 },
-	{ .idr = LPC_IDR4, .odr = LPC_ODR4, .str = LPC_STR4 },
-};
-
-static struct kcs_bmc *aspeed_kcs_probe_of_v1(struct platform_device *pdev)
+static int aspeed_kcs_probe_of_v1(
+		struct aspeed_kcs *aspeed_kcs, struct platform_device *pdev)
 {
-	struct aspeed_kcs *priv;
 	struct device_node *np;
-	struct kcs_bmc *kcs;
 	u32 channel;
 	u32 slave;
 	int rc;
@@ -263,30 +271,25 @@ static struct kcs_bmc *aspeed_kcs_probe_of_v1(struct platform_device *pdev)
 	rc = of_property_read_u32(np, "kcs_chan", &channel);
 	if ((rc != 0) || (channel == 0 || channel > KCS_CHANNEL_MAX)) {
 		dev_err(&pdev->dev, "no valid 'kcs_chan' configured\n");
-		return ERR_PTR(-EINVAL);
+		return -EINVAL;
 	}
+	aspeed_kcs->channel = channel;
 
-	kcs = kcs_bmc_alloc(&pdev->dev, sizeof(struct aspeed_kcs), channel);
-	if (!kcs)
-		return ERR_PTR(-ENOMEM);
-
-	priv = kcs_bmc_priv(kcs);
-	priv->map = syscon_node_to_regmap(pdev->dev.parent->of_node);
-	if (IS_ERR(priv->map)) {
+	aspeed_kcs->map = syscon_node_to_regmap(pdev->dev.parent->of_node);
+	if (IS_ERR(aspeed_kcs->map)) {
 		dev_err(&pdev->dev, "Couldn't get regmap\n");
-		return ERR_PTR(-ENODEV);
+		return -ENODEV;
 	}
 
 	rc = of_property_read_u32(np, "kcs_addr", &slave);
 	if (rc) {
 		dev_err(&pdev->dev, "no valid 'kcs_addr' configured\n");
-		return ERR_PTR(-EINVAL);
+		return -EINVAL;
 	}
 
-	priv->reg = &aspeed_kcs_regs[channel - 1];
-	aspeed_kcs_set_address(kcs, slave);
+	aspeed_kcs_set_address(aspeed_kcs, slave);
 
-	return kcs;
+	return 0;
 }
 
 static int aspeed_kcs_calculate_channel(const struct aspeed_kcs_reg *reg)
@@ -301,12 +304,11 @@ static int aspeed_kcs_calculate_channel(const struct aspeed_kcs_reg *reg)
 	return -EINVAL;
 }
 
-static struct kcs_bmc *aspeed_kcs_probe_of_v2(struct platform_device *pdev)
+static int aspeed_kcs_probe_of_v2(
+		struct aspeed_kcs *aspeed_kcs, struct platform_device *pdev)
 {
-	struct aspeed_kcs *priv;
 	struct device_node *np;
 	struct aspeed_kcs_reg reg;
-	struct kcs_bmc *kcs;
 	const __be32 *of_reg;
 	int channel;
 	u32 slave;
@@ -317,120 +319,116 @@ static struct kcs_bmc *aspeed_kcs_probe_of_v2(struct platform_device *pdev)
 	/* Don't translate addresses, we want offsets for the regmaps */
 	of_reg = of_get_address(np, 0, NULL, NULL);
 	if (!of_reg)
-		return ERR_PTR(-EINVAL);
+		return -EINVAL;
 	reg.idr = be32_to_cpup(of_reg);
 
 	of_reg = of_get_address(np, 1, NULL, NULL);
 	if (!of_reg)
-		return ERR_PTR(-EINVAL);
+		return -EINVAL;
 	reg.odr = be32_to_cpup(of_reg);
 
 	of_reg = of_get_address(np, 2, NULL, NULL);
 	if (!of_reg)
-		return ERR_PTR(-EINVAL);
+		return -EINVAL;
 	reg.str = be32_to_cpup(of_reg);
 
 	channel = aspeed_kcs_calculate_channel(&reg);
 	if (channel < 0)
-		return ERR_PTR(channel);
+		return channel;
+	aspeed_kcs->channel = channel;
 
-	kcs = kcs_bmc_alloc(&pdev->dev, sizeof(struct aspeed_kcs), channel);
-	if (!kcs)
-		return ERR_PTR(-ENOMEM);
-
-	priv = kcs_bmc_priv(kcs);
-	priv->reg = &aspeed_kcs_regs[channel - 1];
-	priv->map = syscon_node_to_regmap(pdev->dev.parent->of_node);
-	if (IS_ERR(priv->map)) {
+	aspeed_kcs->map = syscon_node_to_regmap(pdev->dev.parent->of_node);
+	if (IS_ERR(aspeed_kcs->map)) {
 		dev_err(&pdev->dev, "Couldn't get regmap\n");
-		return ERR_PTR(-ENODEV);
+		return -ENODEV;
 	}
 
 	rc = of_property_read_u32(np, "aspeed,lpc-io-reg", &slave);
 	if (rc)
-		return ERR_PTR(rc);
+		return rc;
 
-	aspeed_kcs_set_address(kcs, slave);
+	aspeed_kcs_set_address(aspeed_kcs, slave);
 
-	return kcs;
+	return 0;
 }
 
 static int aspeed_kcs_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
 	struct kcs_bmc *kcs_bmc;
-	struct aspeed_kcs *priv;
+	struct aspeed_kcs *aspeed_kcs;
 	struct device_node *np;
+	const struct aspeed_kcs_reg *reg;
 	int rc;
 
+	aspeed_kcs = devm_kzalloc(dev, sizeof(*aspeed_kcs), GFP_KERNEL);
+	if (!aspeed_kcs)
+		return -ENOMEM;
+	kcs_bmc = &aspeed_kcs->kcs_bmc;
+	dev_set_drvdata(dev, kcs_bmc);
+
 	np = pdev->dev.of_node;
 	if (of_device_is_compatible(np, "aspeed,ast2400-kcs-bmc") ||
 			of_device_is_compatible(np, "aspeed,ast2500-kcs-bmc"))
-		kcs_bmc = aspeed_kcs_probe_of_v1(pdev);
+		rc = aspeed_kcs_probe_of_v1(aspeed_kcs, pdev);
 	else if (of_device_is_compatible(np, "aspeed,ast2400-kcs-bmc-v2") ||
 			of_device_is_compatible(np, "aspeed,ast2500-kcs-bmc-v2"))
-		kcs_bmc = aspeed_kcs_probe_of_v2(pdev);
+		rc = aspeed_kcs_probe_of_v2(aspeed_kcs, pdev);
 	else
-		return -EINVAL;
+		rc = -EINVAL;
+
+	if (rc)
+		return rc;
 
-	if (IS_ERR(kcs_bmc))
-		return PTR_ERR(kcs_bmc);
+	aspeed_kcs_enable_channel(aspeed_kcs, true);
 
 	kcs_bmc->read_status = aspeed_kcs_read_status;
 	kcs_bmc->write_status = aspeed_kcs_write_status;
 	kcs_bmc->read_data_in = aspeed_kcs_read_data_in;
 	kcs_bmc->write_data_out = aspeed_kcs_write_data_out;
 
-	rc = aspeed_kcs_config_irq(kcs_bmc, pdev);
+	rc = kcs_bmc_init(kcs_bmc, dev, aspeed_kcs->channel);
 	if (rc)
 		return rc;
 
-	dev_set_drvdata(dev, kcs_bmc);
-
-	aspeed_kcs_enable_channel(kcs_bmc, true);
-
-	rc = misc_register(&kcs_bmc->miscdev);
+	rc = aspeed_kcs_config_irq(kcs_bmc, pdev);
 	if (rc) {
-		dev_err(dev, "Unable to register device\n");
+		kcs_bmc_stop(kcs_bmc);
 		return rc;
 	}
 
-	priv = kcs_bmc_priv(kcs_bmc);
+	reg = to_aspeed_kcs_reg(aspeed_kcs);
 	dev_dbg(&pdev->dev,
 		"Probed KCS device %d (IDR=0x%x, ODR=0x%x, STR=0x%x)\n",
-		kcs_bmc->channel, priv->reg->idr, priv->reg->odr,
-		priv->reg->str);
+		aspeed_kcs->channel, reg->idr, reg->odr, reg->str);
 
 	return 0;
 }
 
 static int aspeed_kcs_remove(struct platform_device *pdev)
 {
-	struct kcs_bmc *kcs_bmc = dev_get_drvdata(&pdev->dev);
-
-	misc_deregister(&kcs_bmc->miscdev);
-
+	kcs_bmc_stop(dev_get_drvdata(&pdev->dev));
 	return 0;
 }
 
-static const struct of_device_id ast_kcs_bmc_match[] = {
+static const struct of_device_id aspeed_kcs_bmc_match[] = {
 	{ .compatible = "aspeed,ast2400-kcs-bmc" },
 	{ .compatible = "aspeed,ast2500-kcs-bmc" },
 	{ .compatible = "aspeed,ast2400-kcs-bmc-v2" },
 	{ .compatible = "aspeed,ast2500-kcs-bmc-v2" },
 	{ }
 };
-MODULE_DEVICE_TABLE(of, ast_kcs_bmc_match);
+MODULE_DEVICE_TABLE(of, aspeed_kcs_bmc_match);
 
-static struct platform_driver ast_kcs_bmc_driver = {
+static struct platform_driver aspeed_kcs_bmc_driver = {
 	.driver = {
 		.name           = DEVICE_NAME,
-		.of_match_table = ast_kcs_bmc_match,
+		.of_match_table = aspeed_kcs_bmc_match,
 	},
 	.probe  = aspeed_kcs_probe,
 	.remove = aspeed_kcs_remove,
 };
-module_platform_driver(ast_kcs_bmc_driver);
+module_platform_driver(aspeed_kcs_bmc_driver);
 
 MODULE_LICENSE("GPL v2");
 MODULE_AUTHOR("Haiyue Wang <haiyue.wang@linux.intel.com>");
diff --git a/drivers/char/ipmi/kcs_bmc_npcm7xx.c b/drivers/char/ipmi/kcs_bmc_npcm7xx.c
index 572501f7da71..e9ba95d0d560 100644
--- a/drivers/char/ipmi/kcs_bmc_npcm7xx.c
+++ b/drivers/char/ipmi/kcs_bmc_npcm7xx.c
@@ -65,8 +65,9 @@ struct npcm7xx_kcs_reg {
 };
 
 struct npcm7xx_kcs {
-	struct regmap *map;
+	struct kcs_bmc kcs_bmc;
 
+	struct regmap *map;
 	const struct npcm7xx_kcs_reg *reg;
 };
 
@@ -76,6 +77,11 @@ static const struct npcm7xx_kcs_reg npcm7xx_kcs_reg_tbl[KCS_CHANNEL_MAX] = {
 	{ .sts = KCS3ST, .dob = KCS3DO, .dib = KCS3DI, .ctl = KCS3CTL, .ie = KCS3IE },
 };
 
+static struct npcm7xx_kcs *to_npcm7xx_kcs(struct kcs_bmc *kcs_bmc)
+{
+	return container_of(kcs_bmc, struct npcm7xx_kcs, kcs_bmc);
+}
+
 static u8 npcm7xx_kcs_inb(struct npcm7xx_kcs *npcm7xx_kcs, u32 reg)
 {
 	u32 val = 0;
@@ -92,36 +98,35 @@ static void npcm7xx_kcs_outb(struct npcm7xx_kcs *npcm7xx_kcs, u32 reg, u8 data)
 
 static u8 npcm7xx_kcs_read_status(struct kcs_bmc *kcs_bmc)
 {
-	struct npcm7xx_kcs *npcm7xx_kcs = kcs_bmc_priv(kcs_bmc);
+	struct npcm7xx_kcs *npcm7xx_kcs = to_npcm7xx_kcs(kcs_bmc);
 	return npcm7xx_kcs_inb(npcm7xx_kcs, npcm7xx_kcs->reg->sts);
 }
 
 static void npcm7xx_kcs_write_status(struct kcs_bmc *kcs_bmc, u8 data)
 {
-	struct npcm7xx_kcs *npcm7xx_kcs = kcs_bmc_priv(kcs_bmc);
+	struct npcm7xx_kcs *npcm7xx_kcs = to_npcm7xx_kcs(kcs_bmc);
 	npcm7xx_kcs_outb(npcm7xx_kcs, npcm7xx_kcs->reg->sts, data);
 }
 
 static u8 npcm7xx_kcs_read_data_in(struct kcs_bmc *kcs_bmc)
 {
-	struct npcm7xx_kcs *npcm7xx_kcs = kcs_bmc_priv(kcs_bmc);
+	struct npcm7xx_kcs *npcm7xx_kcs = to_npcm7xx_kcs(kcs_bmc);
 	return npcm7xx_kcs_inb(npcm7xx_kcs, npcm7xx_kcs->reg->dib);
 }
 
 static void npcm7xx_kcs_write_data_out(struct kcs_bmc *kcs_bmc, u8 data)
 {
-	struct npcm7xx_kcs *npcm7xx_kcs = kcs_bmc_priv(kcs_bmc);
+	struct npcm7xx_kcs *npcm7xx_kcs = to_npcm7xx_kcs(kcs_bmc);
 	npcm7xx_kcs_outb(npcm7xx_kcs, npcm7xx_kcs->reg->dob, data);
 }
 
-static void npcm7xx_kcs_enable_channel(struct kcs_bmc *kcs_bmc, bool enable)
+static void npcm7xx_kcs_enable_channel(struct npcm7xx_kcs *npcm7xx_kcs, bool enable)
 {
-	struct npcm7xx_kcs *priv = kcs_bmc_priv(kcs_bmc);
-
-	regmap_update_bits(priv->map, priv->reg->ctl, KCS_CTL_IBFIE,
-			   enable ? KCS_CTL_IBFIE : 0);
+	regmap_update_bits(npcm7xx_kcs->map, npcm7xx_kcs->reg->ctl,
+			   KCS_CTL_IBFIE, enable ? KCS_CTL_IBFIE : 0);
 
-	regmap_update_bits(priv->map, priv->reg->ie, KCS_IE_IRQE | KCS_IE_HIRQE,
+	regmap_update_bits(npcm7xx_kcs->map, npcm7xx_kcs->reg->ie,
+			   KCS_IE_IRQE | KCS_IE_HIRQE,
 			   enable ? KCS_IE_IRQE | KCS_IE_HIRQE : 0);
 }
 
@@ -142,7 +147,7 @@ static int npcm7xx_kcs_config_irq(struct kcs_bmc *kcs_bmc,
 static int npcm7xx_kcs_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
-	struct npcm7xx_kcs *priv;
+	struct npcm7xx_kcs *npcm7xx_kcs;
 	struct kcs_bmc *kcs_bmc;
 	u32 chan;
 	int rc;
@@ -153,49 +158,47 @@ static int npcm7xx_kcs_probe(struct platform_device *pdev)
 		return -ENODEV;
 	}
 
-	kcs_bmc = kcs_bmc_alloc(dev, sizeof(*priv), chan);
-	if (!kcs_bmc)
+	npcm7xx_kcs = devm_kzalloc(dev, sizeof(*npcm7xx_kcs), GFP_KERNEL);
+	if (!npcm7xx_kcs)
 		return -ENOMEM;
+	kcs_bmc = &npcm7xx_kcs->kcs_bmc;
+	dev_set_drvdata(dev, kcs_bmc);
 
-	priv = kcs_bmc_priv(kcs_bmc);
-	priv->map = syscon_node_to_regmap(dev->parent->of_node);
-	if (IS_ERR(priv->map)) {
+	npcm7xx_kcs->map = syscon_node_to_regmap(dev->parent->of_node);
+	if (IS_ERR(npcm7xx_kcs->map)) {
 		dev_err(dev, "Couldn't get regmap\n");
 		return -ENODEV;
 	}
-	priv->reg = &npcm7xx_kcs_reg_tbl[chan - 1];
+	npcm7xx_kcs->reg = &npcm7xx_kcs_reg_tbl[chan - 1];
 
 	kcs_bmc->read_status = npcm7xx_kcs_read_status;
 	kcs_bmc->write_status = npcm7xx_kcs_write_status;
 	kcs_bmc->read_data_in = npcm7xx_kcs_read_data_in;
 	kcs_bmc->write_data_out = npcm7xx_kcs_write_data_out;
 
-	dev_set_drvdata(dev, kcs_bmc);
+	npcm7xx_kcs_enable_channel(npcm7xx_kcs, true);
 
-	npcm7xx_kcs_enable_channel(kcs_bmc, true);
-	rc = npcm7xx_kcs_config_irq(kcs_bmc, pdev);
+	rc = kcs_bmc_init(kcs_bmc, dev, chan);
 	if (rc)
 		return rc;
 
-	rc = misc_register(&kcs_bmc->miscdev);
+	rc = npcm7xx_kcs_config_irq(kcs_bmc, pdev);
 	if (rc) {
-		dev_err(dev, "Unable to register device\n");
+		kcs_bmc_stop(kcs_bmc);
 		return rc;
 	}
 
 	dev_dbg(&pdev->dev,
-		"Probed KCS device %d (DIB=0x%x, DOB=0x%x, STS=0x%x )\n",
-		chan, priv->reg->dib, priv->reg->dob, priv->reg->sts);
+		"Probed KCS device %d (DIB=0x%x, DOB=0x%x, STS=0x%x)\n",
+		chan, npcm7xx_kcs->reg->dib, npcm7xx_kcs->reg->dob,
+		npcm7xx_kcs->reg->sts);
 
 	return 0;
 }
 
 static int npcm7xx_kcs_remove(struct platform_device *pdev)
 {
-	struct kcs_bmc *kcs_bmc = dev_get_drvdata(&pdev->dev);
-
-	misc_deregister(&kcs_bmc->miscdev);
-
+	kcs_bmc_stop(dev_get_drvdata(&pdev->dev));
 	return 0;
 }
 
-- 
2.30.0.478.g8a0d178c01-goog

[PATCH 3/4] ipmi: kcs_bmc: Invert allocation

[William A. Kennington III]

This is a more typical pattern for having the platform specific drivers
store their own data around the generic driver data.

Signed-off-by: William A. Kennington III <wak@google.com>
---
 drivers/char/ipmi/kcs_bmc.c         |  40 +++---
 drivers/char/ipmi/kcs_bmc.h         |  13 +-
 drivers/char/ipmi/kcs_bmc_aspeed.c  | 216 ++++++++++++++--------------
 drivers/char/ipmi/kcs_bmc_npcm7xx.c |  61 ++++----
 4 files changed, 163 insertions(+), 167 deletions(-)

diff --git a/drivers/char/ipmi/kcs_bmc.c b/drivers/char/ipmi/kcs_bmc.c
index 4c504588e714..f182c4f6f982 100644
--- a/drivers/char/ipmi/kcs_bmc.c
+++ b/drivers/char/ipmi/kcs_bmc.c
@@ -277,9 +277,6 @@ static ssize_t kcs_bmc_read(struct file *filp, char __user *buf,
 	}
 
 	if (count < data_len) {
-		pr_err("channel=%u with too large data : %zu\n",
-			kcs_bmc->channel, data_len);
-
 		spin_lock_irq(&kcs_bmc->lock);
 		kcs_force_abort(kcs_bmc);
 		spin_unlock_irq(&kcs_bmc->lock);
@@ -402,17 +399,11 @@ static const struct file_operations kcs_bmc_fops = {
 	.unlocked_ioctl = kcs_bmc_ioctl,
 };
 
-struct kcs_bmc *kcs_bmc_alloc(struct device *dev, int sizeof_priv, u32 channel)
+int kcs_bmc_init(struct kcs_bmc *kcs_bmc, struct device *dev, u32 channel)
 {
-	struct kcs_bmc *kcs_bmc;
-
-	kcs_bmc = devm_kzalloc(dev, sizeof(*kcs_bmc) + sizeof_priv, GFP_KERNEL);
-	if (!kcs_bmc)
-		return NULL;
+	int rc;
 
 	spin_lock_init(&kcs_bmc->lock);
-	kcs_bmc->channel = channel;
-
 	mutex_init(&kcs_bmc->mutex);
 	init_waitqueue_head(&kcs_bmc->queue);
 
@@ -420,17 +411,28 @@ struct kcs_bmc *kcs_bmc_alloc(struct device *dev, int sizeof_priv, u32 channel)
 	kcs_bmc->data_out = devm_kmalloc(dev, KCS_MSG_BUFSIZ, GFP_KERNEL);
 	kcs_bmc->kbuffer = devm_kmalloc(dev, KCS_MSG_BUFSIZ, GFP_KERNEL);
 
-	kcs_bmc->miscdev.minor = MISC_DYNAMIC_MINOR;
-	kcs_bmc->miscdev.name = devm_kasprintf(dev, GFP_KERNEL, "%s%u",
-					       DEVICE_NAME, channel);
-	if (!kcs_bmc->data_in || !kcs_bmc->data_out || !kcs_bmc->kbuffer ||
-	    !kcs_bmc->miscdev.name)
-		return NULL;
 	kcs_bmc->miscdev.fops = &kcs_bmc_fops;
+	kcs_bmc->miscdev.minor = MISC_DYNAMIC_MINOR;
+	kcs_bmc->miscdev.name =
+		devm_kasprintf(dev, GFP_KERNEL, "%s%u", DEVICE_NAME, channel);
+	if (!kcs_bmc->miscdev.name)
+		return -ENOMEM;
+
+	rc = misc_register(&kcs_bmc->miscdev);
+	if (rc) {
+		dev_err(dev, "Registering kcs_bmc: %d\n", rc);
+		return rc;
+	}
 
-	return kcs_bmc;
+	return 0;
+}
+EXPORT_SYMBOL(kcs_bmc_init);
+
+void kcs_bmc_stop(struct kcs_bmc *kcs_bmc)
+{
+	misc_deregister(&kcs_bmc->miscdev);
 }
-EXPORT_SYMBOL(kcs_bmc_alloc);
+EXPORT_SYMBOL(kcs_bmc_stop);
 
 MODULE_LICENSE("GPL v2");
 MODULE_AUTHOR("Haiyue Wang <haiyue.wang@linux.intel.com>");
diff --git a/drivers/char/ipmi/kcs_bmc.h b/drivers/char/ipmi/kcs_bmc.h
index 8c541251fe97..d65ffd479e9b 100644
--- a/drivers/char/ipmi/kcs_bmc.h
+++ b/drivers/char/ipmi/kcs_bmc.h
@@ -64,7 +64,6 @@ enum kcs_errors {
 struct kcs_bmc {
 	spinlock_t lock;
 
-	u32 channel;
 	int running;
 
 	/* Setup by BMC KCS controller driver */
@@ -89,16 +88,10 @@ struct kcs_bmc {
 	u8 *kbuffer;
 
 	struct miscdevice miscdev;
-
-	unsigned long priv[];
 };
 
-static inline void *kcs_bmc_priv(struct kcs_bmc *kcs_bmc)
-{
-	return kcs_bmc->priv;
-}
-
 irqreturn_t kcs_bmc_irq(int irq, void *arg);
-struct kcs_bmc *kcs_bmc_alloc(struct device *dev, int sizeof_priv,
-					u32 channel);
+int kcs_bmc_init(struct kcs_bmc *kcs_bmc, struct device *dev, u32 channel);
+void kcs_bmc_stop(struct kcs_bmc *kcs_bmc);
+
 #endif /* __KCS_BMC_H__ */
diff --git a/drivers/char/ipmi/kcs_bmc_aspeed.c b/drivers/char/ipmi/kcs_bmc_aspeed.c
index d3b771e4d039..b466569d049d 100644
--- a/drivers/char/ipmi/kcs_bmc_aspeed.c
+++ b/drivers/char/ipmi/kcs_bmc_aspeed.c
@@ -69,11 +69,29 @@ struct aspeed_kcs_reg {
 };
 
 struct aspeed_kcs {
+	struct kcs_bmc kcs_bmc;
+
+	u32 channel;
 	struct regmap *map;
+};
 
-	const struct aspeed_kcs_reg *reg;
+static const struct aspeed_kcs_reg aspeed_kcs_regs[KCS_CHANNEL_MAX] = {
+	{ .idr = LPC_IDR1, .odr = LPC_ODR1, .str = LPC_STR1 },
+	{ .idr = LPC_IDR2, .odr = LPC_ODR2, .str = LPC_STR2 },
+	{ .idr = LPC_IDR3, .odr = LPC_ODR3, .str = LPC_STR3 },
+	{ .idr = LPC_IDR4, .odr = LPC_ODR4, .str = LPC_STR4 },
 };
 
+static struct aspeed_kcs *to_aspeed_kcs(struct kcs_bmc *kcs_bmc)
+{
+	return container_of(kcs_bmc, struct aspeed_kcs, kcs_bmc);
+}
+
+static const struct aspeed_kcs_reg *to_aspeed_kcs_reg(
+		const struct aspeed_kcs *aspeed_kcs)
+{
+	return &aspeed_kcs_regs[aspeed_kcs->channel-1];
+}
 
 static u8 aspeed_kcs_inb(struct aspeed_kcs *aspeed_kcs, u32 reg)
 {
@@ -91,26 +109,26 @@ static void aspeed_kcs_outb(struct aspeed_kcs *aspeed_kcs, u32 reg, u8 data)
 
 static u8 aspeed_kcs_read_status(struct kcs_bmc *kcs_bmc)
 {
-	struct aspeed_kcs *aspeed_kcs = kcs_bmc_priv(kcs_bmc);
-	return aspeed_kcs_inb(aspeed_kcs, aspeed_kcs->reg->str);
+	struct aspeed_kcs *aspeed_kcs = to_aspeed_kcs(kcs_bmc);
+	return aspeed_kcs_inb(aspeed_kcs, to_aspeed_kcs_reg(aspeed_kcs)->str);
 }
 
 static void aspeed_kcs_write_status(struct kcs_bmc *kcs_bmc, u8 data)
 {
-	struct aspeed_kcs *aspeed_kcs = kcs_bmc_priv(kcs_bmc);
-	aspeed_kcs_outb(aspeed_kcs, aspeed_kcs->reg->str, data);
+	struct aspeed_kcs *aspeed_kcs = to_aspeed_kcs(kcs_bmc);
+	aspeed_kcs_outb(aspeed_kcs, to_aspeed_kcs_reg(aspeed_kcs)->str, data);
 }
 
 static u8 aspeed_kcs_read_data_in(struct kcs_bmc *kcs_bmc)
 {
-	struct aspeed_kcs *aspeed_kcs = kcs_bmc_priv(kcs_bmc);
-	return aspeed_kcs_inb(aspeed_kcs, aspeed_kcs->reg->idr);
+	struct aspeed_kcs *aspeed_kcs = to_aspeed_kcs(kcs_bmc);
+	return aspeed_kcs_inb(aspeed_kcs, to_aspeed_kcs_reg(aspeed_kcs)->idr);
 }
 
 static void aspeed_kcs_write_data_out(struct kcs_bmc *kcs_bmc, u8 data)
 {
-	struct aspeed_kcs *aspeed_kcs = kcs_bmc_priv(kcs_bmc);
-	aspeed_kcs_outb(aspeed_kcs, aspeed_kcs->reg->odr, data);
+	struct aspeed_kcs *aspeed_kcs = to_aspeed_kcs(kcs_bmc);
+	aspeed_kcs_outb(aspeed_kcs, to_aspeed_kcs_reg(aspeed_kcs)->odr, data);
 }
 
 
@@ -127,32 +145,31 @@ static void aspeed_kcs_write_data_out(struct kcs_bmc *kcs_bmc, u8 data)
  *     C. KCS4
  *        D / C : CA4h / CA5h
  */
-static void aspeed_kcs_set_address(struct kcs_bmc *kcs_bmc, u16 addr)
+static void aspeed_kcs_set_address(
+		struct aspeed_kcs *aspeed_kcs, u16 addr)
 {
-	struct aspeed_kcs *priv = kcs_bmc_priv(kcs_bmc);
-
-	switch (kcs_bmc->channel) {
+	switch (aspeed_kcs->channel) {
 	case 1:
-		regmap_update_bits(priv->map, LPC_HICR4,
+		regmap_update_bits(aspeed_kcs->map, LPC_HICR4,
 				LPC_HICR4_LADR12AS, 0);
-		regmap_write(priv->map, LPC_LADR12H, addr >> 8);
-		regmap_write(priv->map, LPC_LADR12L, addr & 0xFF);
+		regmap_write(aspeed_kcs->map, LPC_LADR12H, addr >> 8);
+		regmap_write(aspeed_kcs->map, LPC_LADR12L, addr & 0xFF);
 		break;
 
 	case 2:
-		regmap_update_bits(priv->map, LPC_HICR4,
+		regmap_update_bits(aspeed_kcs->map, LPC_HICR4,
 				LPC_HICR4_LADR12AS, LPC_HICR4_LADR12AS);
-		regmap_write(priv->map, LPC_LADR12H, addr >> 8);
-		regmap_write(priv->map, LPC_LADR12L, addr & 0xFF);
+		regmap_write(aspeed_kcs->map, LPC_LADR12H, addr >> 8);
+		regmap_write(aspeed_kcs->map, LPC_LADR12L, addr & 0xFF);
 		break;
 
 	case 3:
-		regmap_write(priv->map, LPC_LADR3H, addr >> 8);
-		regmap_write(priv->map, LPC_LADR3L, addr & 0xFF);
+		regmap_write(aspeed_kcs->map, LPC_LADR3H, addr >> 8);
+		regmap_write(aspeed_kcs->map, LPC_LADR3L, addr & 0xFF);
 		break;
 
 	case 4:
-		regmap_write(priv->map, LPC_LADR4, ((addr + 1) << 16) |
+		regmap_write(aspeed_kcs->map, LPC_LADR4, ((addr + 1) << 16) |
 			addr);
 		break;
 
@@ -161,64 +178,63 @@ static void aspeed_kcs_set_address(struct kcs_bmc *kcs_bmc, u16 addr)
 	}
 }
 
-static void aspeed_kcs_enable_channel(struct kcs_bmc *kcs_bmc, bool enable)
+static void aspeed_kcs_enable_channel(
+		struct aspeed_kcs *aspeed_kcs, bool enable)
 {
-	struct aspeed_kcs *priv = kcs_bmc_priv(kcs_bmc);
-
-	switch (kcs_bmc->channel) {
+	switch (aspeed_kcs->channel) {
 	case 1:
 		if (enable) {
-			regmap_update_bits(priv->map, LPC_HICR2,
+			regmap_update_bits(aspeed_kcs->map, LPC_HICR2,
 					LPC_HICR2_IBFIF1, LPC_HICR2_IBFIF1);
-			regmap_update_bits(priv->map, LPC_HICR0,
+			regmap_update_bits(aspeed_kcs->map, LPC_HICR0,
 					LPC_HICR0_LPC1E, LPC_HICR0_LPC1E);
 		} else {
-			regmap_update_bits(priv->map, LPC_HICR0,
+			regmap_update_bits(aspeed_kcs->map, LPC_HICR0,
 					LPC_HICR0_LPC1E, 0);
-			regmap_update_bits(priv->map, LPC_HICR2,
+			regmap_update_bits(aspeed_kcs->map, LPC_HICR2,
 					LPC_HICR2_IBFIF1, 0);
 		}
 		break;
 
 	case 2:
 		if (enable) {
-			regmap_update_bits(priv->map, LPC_HICR2,
+			regmap_update_bits(aspeed_kcs->map, LPC_HICR2,
 					LPC_HICR2_IBFIF2, LPC_HICR2_IBFIF2);
-			regmap_update_bits(priv->map, LPC_HICR0,
+			regmap_update_bits(aspeed_kcs->map, LPC_HICR0,
 					LPC_HICR0_LPC2E, LPC_HICR0_LPC2E);
 		} else {
-			regmap_update_bits(priv->map, LPC_HICR0,
+			regmap_update_bits(aspeed_kcs->map, LPC_HICR0,
 					LPC_HICR0_LPC2E, 0);
-			regmap_update_bits(priv->map, LPC_HICR2,
+			regmap_update_bits(aspeed_kcs->map, LPC_HICR2,
 					LPC_HICR2_IBFIF2, 0);
 		}
 		break;
 
 	case 3:
 		if (enable) {
-			regmap_update_bits(priv->map, LPC_HICR2,
+			regmap_update_bits(aspeed_kcs->map, LPC_HICR2,
 					LPC_HICR2_IBFIF3, LPC_HICR2_IBFIF3);
-			regmap_update_bits(priv->map, LPC_HICR0,
+			regmap_update_bits(aspeed_kcs->map, LPC_HICR0,
 					LPC_HICR0_LPC3E, LPC_HICR0_LPC3E);
-			regmap_update_bits(priv->map, LPC_HICR4,
+			regmap_update_bits(aspeed_kcs->map, LPC_HICR4,
 					LPC_HICR4_KCSENBL, LPC_HICR4_KCSENBL);
 		} else {
-			regmap_update_bits(priv->map, LPC_HICR0,
+			regmap_update_bits(aspeed_kcs->map, LPC_HICR0,
 					LPC_HICR0_LPC3E, 0);
-			regmap_update_bits(priv->map, LPC_HICR4,
+			regmap_update_bits(aspeed_kcs->map, LPC_HICR4,
 					LPC_HICR4_KCSENBL, 0);
-			regmap_update_bits(priv->map, LPC_HICR2,
+			regmap_update_bits(aspeed_kcs->map, LPC_HICR2,
 					LPC_HICR2_IBFIF3, 0);
 		}
 		break;
 
 	case 4:
 		if (enable)
-			regmap_update_bits(priv->map, LPC_HICRB,
+			regmap_update_bits(aspeed_kcs->map, LPC_HICRB,
 					LPC_HICRB_IBFIF4 | LPC_HICRB_LPC4E,
 					LPC_HICRB_IBFIF4 | LPC_HICRB_LPC4E);
 		else
-			regmap_update_bits(priv->map, LPC_HICRB,
+			regmap_update_bits(aspeed_kcs->map, LPC_HICRB,
 					LPC_HICRB_IBFIF4 | LPC_HICRB_LPC4E,
 					0);
 		break;
@@ -242,18 +258,10 @@ static int aspeed_kcs_config_irq(struct kcs_bmc *kcs_bmc,
 				dev_name(dev), kcs_bmc);
 }
 
-static const struct aspeed_kcs_reg aspeed_kcs_regs[KCS_CHANNEL_MAX] = {
-	{ .idr = LPC_IDR1, .odr = LPC_ODR1, .str = LPC_STR1 },
-	{ .idr = LPC_IDR2, .odr = LPC_ODR2, .str = LPC_STR2 },
-	{ .idr = LPC_IDR3, .odr = LPC_ODR3, .str = LPC_STR3 },
-	{ .idr = LPC_IDR4, .odr = LPC_ODR4, .str = LPC_STR4 },
-};
-
-static struct kcs_bmc *aspeed_kcs_probe_of_v1(struct platform_device *pdev)
+static int aspeed_kcs_probe_of_v1(
+		struct aspeed_kcs *aspeed_kcs, struct platform_device *pdev)
 {
-	struct aspeed_kcs *priv;
 	struct device_node *np;
-	struct kcs_bmc *kcs;
 	u32 channel;
 	u32 slave;
 	int rc;
@@ -263,30 +271,25 @@ static struct kcs_bmc *aspeed_kcs_probe_of_v1(struct platform_device *pdev)
 	rc = of_property_read_u32(np, "kcs_chan", &channel);
 	if ((rc != 0) || (channel == 0 || channel > KCS_CHANNEL_MAX)) {
 		dev_err(&pdev->dev, "no valid 'kcs_chan' configured\n");
-		return ERR_PTR(-EINVAL);
+		return -EINVAL;
 	}
+	aspeed_kcs->channel = channel;
 
-	kcs = kcs_bmc_alloc(&pdev->dev, sizeof(struct aspeed_kcs), channel);
-	if (!kcs)
-		return ERR_PTR(-ENOMEM);
-
-	priv = kcs_bmc_priv(kcs);
-	priv->map = syscon_node_to_regmap(pdev->dev.parent->of_node);
-	if (IS_ERR(priv->map)) {
+	aspeed_kcs->map = syscon_node_to_regmap(pdev->dev.parent->of_node);
+	if (IS_ERR(aspeed_kcs->map)) {
 		dev_err(&pdev->dev, "Couldn't get regmap\n");
-		return ERR_PTR(-ENODEV);
+		return -ENODEV;
 	}
 
 	rc = of_property_read_u32(np, "kcs_addr", &slave);
 	if (rc) {
 		dev_err(&pdev->dev, "no valid 'kcs_addr' configured\n");
-		return ERR_PTR(-EINVAL);
+		return -EINVAL;
 	}
 
-	priv->reg = &aspeed_kcs_regs[channel - 1];
-	aspeed_kcs_set_address(kcs, slave);
+	aspeed_kcs_set_address(aspeed_kcs, slave);
 
-	return kcs;
+	return 0;
 }
 
 static int aspeed_kcs_calculate_channel(const struct aspeed_kcs_reg *reg)
@@ -301,12 +304,11 @@ static int aspeed_kcs_calculate_channel(const struct aspeed_kcs_reg *reg)
 	return -EINVAL;
 }
 
-static struct kcs_bmc *aspeed_kcs_probe_of_v2(struct platform_device *pdev)
+static int aspeed_kcs_probe_of_v2(
+		struct aspeed_kcs *aspeed_kcs, struct platform_device *pdev)
 {
-	struct aspeed_kcs *priv;
 	struct device_node *np;
 	struct aspeed_kcs_reg reg;
-	struct kcs_bmc *kcs;
 	const __be32 *of_reg;
 	int channel;
 	u32 slave;
@@ -317,120 +319,116 @@ static struct kcs_bmc *aspeed_kcs_probe_of_v2(struct platform_device *pdev)
 	/* Don't translate addresses, we want offsets for the regmaps */
 	of_reg = of_get_address(np, 0, NULL, NULL);
 	if (!of_reg)
-		return ERR_PTR(-EINVAL);
+		return -EINVAL;
 	reg.idr = be32_to_cpup(of_reg);
 
 	of_reg = of_get_address(np, 1, NULL, NULL);
 	if (!of_reg)
-		return ERR_PTR(-EINVAL);
+		return -EINVAL;
 	reg.odr = be32_to_cpup(of_reg);
 
 	of_reg = of_get_address(np, 2, NULL, NULL);
 	if (!of_reg)
-		return ERR_PTR(-EINVAL);
+		return -EINVAL;
 	reg.str = be32_to_cpup(of_reg);
 
 	channel = aspeed_kcs_calculate_channel(&reg);
 	if (channel < 0)
-		return ERR_PTR(channel);
+		return channel;
+	aspeed_kcs->channel = channel;
 
-	kcs = kcs_bmc_alloc(&pdev->dev, sizeof(struct aspeed_kcs), channel);
-	if (!kcs)
-		return ERR_PTR(-ENOMEM);
-
-	priv = kcs_bmc_priv(kcs);
-	priv->reg = &aspeed_kcs_regs[channel - 1];
-	priv->map = syscon_node_to_regmap(pdev->dev.parent->of_node);
-	if (IS_ERR(priv->map)) {
+	aspeed_kcs->map = syscon_node_to_regmap(pdev->dev.parent->of_node);
+	if (IS_ERR(aspeed_kcs->map)) {
 		dev_err(&pdev->dev, "Couldn't get regmap\n");
-		return ERR_PTR(-ENODEV);
+		return -ENODEV;
 	}
 
 	rc = of_property_read_u32(np, "aspeed,lpc-io-reg", &slave);
 	if (rc)
-		return ERR_PTR(rc);
+		return rc;
 
-	aspeed_kcs_set_address(kcs, slave);
+	aspeed_kcs_set_address(aspeed_kcs, slave);
 
-	return kcs;
+	return 0;
 }
 
 static int aspeed_kcs_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
 	struct kcs_bmc *kcs_bmc;
-	struct aspeed_kcs *priv;
+	struct aspeed_kcs *aspeed_kcs;
 	struct device_node *np;
+	const struct aspeed_kcs_reg *reg;
 	int rc;
 
+	aspeed_kcs = devm_kzalloc(dev, sizeof(*aspeed_kcs), GFP_KERNEL);
+	if (!aspeed_kcs)
+		return -ENOMEM;
+	kcs_bmc = &aspeed_kcs->kcs_bmc;
+	dev_set_drvdata(dev, kcs_bmc);
+
 	np = pdev->dev.of_node;
 	if (of_device_is_compatible(np, "aspeed,ast2400-kcs-bmc") ||
 			of_device_is_compatible(np, "aspeed,ast2500-kcs-bmc"))
-		kcs_bmc = aspeed_kcs_probe_of_v1(pdev);
+		rc = aspeed_kcs_probe_of_v1(aspeed_kcs, pdev);
 	else if (of_device_is_compatible(np, "aspeed,ast2400-kcs-bmc-v2") ||
 			of_device_is_compatible(np, "aspeed,ast2500-kcs-bmc-v2"))
-		kcs_bmc = aspeed_kcs_probe_of_v2(pdev);
+		rc = aspeed_kcs_probe_of_v2(aspeed_kcs, pdev);
 	else
-		return -EINVAL;
+		rc = -EINVAL;
+
+	if (rc)
+		return rc;
 
-	if (IS_ERR(kcs_bmc))
-		return PTR_ERR(kcs_bmc);
+	aspeed_kcs_enable_channel(aspeed_kcs, true);
 
 	kcs_bmc->read_status = aspeed_kcs_read_status;
 	kcs_bmc->write_status = aspeed_kcs_write_status;
 	kcs_bmc->read_data_in = aspeed_kcs_read_data_in;
 	kcs_bmc->write_data_out = aspeed_kcs_write_data_out;
 
-	rc = aspeed_kcs_config_irq(kcs_bmc, pdev);
+	rc = kcs_bmc_init(kcs_bmc, dev, aspeed_kcs->channel);
 	if (rc)
 		return rc;
 
-	dev_set_drvdata(dev, kcs_bmc);
-
-	aspeed_kcs_enable_channel(kcs_bmc, true);
-
-	rc = misc_register(&kcs_bmc->miscdev);
+	rc = aspeed_kcs_config_irq(kcs_bmc, pdev);
 	if (rc) {
-		dev_err(dev, "Unable to register device\n");
+		kcs_bmc_stop(kcs_bmc);
 		return rc;
 	}
 
-	priv = kcs_bmc_priv(kcs_bmc);
+	reg = to_aspeed_kcs_reg(aspeed_kcs);
 	dev_dbg(&pdev->dev,
 		"Probed KCS device %d (IDR=0x%x, ODR=0x%x, STR=0x%x)\n",
-		kcs_bmc->channel, priv->reg->idr, priv->reg->odr,
-		priv->reg->str);
+		aspeed_kcs->channel, reg->idr, reg->odr, reg->str);
 
 	return 0;
 }
 
 static int aspeed_kcs_remove(struct platform_device *pdev)
 {
-	struct kcs_bmc *kcs_bmc = dev_get_drvdata(&pdev->dev);
-
-	misc_deregister(&kcs_bmc->miscdev);
-
+	kcs_bmc_stop(dev_get_drvdata(&pdev->dev));
 	return 0;
 }
 
-static const struct of_device_id ast_kcs_bmc_match[] = {
+static const struct of_device_id aspeed_kcs_bmc_match[] = {
 	{ .compatible = "aspeed,ast2400-kcs-bmc" },
 	{ .compatible = "aspeed,ast2500-kcs-bmc" },
 	{ .compatible = "aspeed,ast2400-kcs-bmc-v2" },
 	{ .compatible = "aspeed,ast2500-kcs-bmc-v2" },
 	{ }
 };
-MODULE_DEVICE_TABLE(of, ast_kcs_bmc_match);
+MODULE_DEVICE_TABLE(of, aspeed_kcs_bmc_match);
 
-static struct platform_driver ast_kcs_bmc_driver = {
+static struct platform_driver aspeed_kcs_bmc_driver = {
 	.driver = {
 		.name           = DEVICE_NAME,
-		.of_match_table = ast_kcs_bmc_match,
+		.of_match_table = aspeed_kcs_bmc_match,
 	},
 	.probe  = aspeed_kcs_probe,
 	.remove = aspeed_kcs_remove,
 };
-module_platform_driver(ast_kcs_bmc_driver);
+module_platform_driver(aspeed_kcs_bmc_driver);
 
 MODULE_LICENSE("GPL v2");
 MODULE_AUTHOR("Haiyue Wang <haiyue.wang@linux.intel.com>");
diff --git a/drivers/char/ipmi/kcs_bmc_npcm7xx.c b/drivers/char/ipmi/kcs_bmc_npcm7xx.c
index 572501f7da71..e9ba95d0d560 100644
--- a/drivers/char/ipmi/kcs_bmc_npcm7xx.c
+++ b/drivers/char/ipmi/kcs_bmc_npcm7xx.c
@@ -65,8 +65,9 @@ struct npcm7xx_kcs_reg {
 };
 
 struct npcm7xx_kcs {
-	struct regmap *map;
+	struct kcs_bmc kcs_bmc;
 
+	struct regmap *map;
 	const struct npcm7xx_kcs_reg *reg;
 };
 
@@ -76,6 +77,11 @@ static const struct npcm7xx_kcs_reg npcm7xx_kcs_reg_tbl[KCS_CHANNEL_MAX] = {
 	{ .sts = KCS3ST, .dob = KCS3DO, .dib = KCS3DI, .ctl = KCS3CTL, .ie = KCS3IE },
 };
 
+static struct npcm7xx_kcs *to_npcm7xx_kcs(struct kcs_bmc *kcs_bmc)
+{
+	return container_of(kcs_bmc, struct npcm7xx_kcs, kcs_bmc);
+}
+
 static u8 npcm7xx_kcs_inb(struct npcm7xx_kcs *npcm7xx_kcs, u32 reg)
 {
 	u32 val = 0;
@@ -92,36 +98,35 @@ static void npcm7xx_kcs_outb(struct npcm7xx_kcs *npcm7xx_kcs, u32 reg, u8 data)
 
 static u8 npcm7xx_kcs_read_status(struct kcs_bmc *kcs_bmc)
 {
-	struct npcm7xx_kcs *npcm7xx_kcs = kcs_bmc_priv(kcs_bmc);
+	struct npcm7xx_kcs *npcm7xx_kcs = to_npcm7xx_kcs(kcs_bmc);
 	return npcm7xx_kcs_inb(npcm7xx_kcs, npcm7xx_kcs->reg->sts);
 }
 
 static void npcm7xx_kcs_write_status(struct kcs_bmc *kcs_bmc, u8 data)
 {
-	struct npcm7xx_kcs *npcm7xx_kcs = kcs_bmc_priv(kcs_bmc);
+	struct npcm7xx_kcs *npcm7xx_kcs = to_npcm7xx_kcs(kcs_bmc);
 	npcm7xx_kcs_outb(npcm7xx_kcs, npcm7xx_kcs->reg->sts, data);
 }
 
 static u8 npcm7xx_kcs_read_data_in(struct kcs_bmc *kcs_bmc)
 {
-	struct npcm7xx_kcs *npcm7xx_kcs = kcs_bmc_priv(kcs_bmc);
+	struct npcm7xx_kcs *npcm7xx_kcs = to_npcm7xx_kcs(kcs_bmc);
 	return npcm7xx_kcs_inb(npcm7xx_kcs, npcm7xx_kcs->reg->dib);
 }
 
 static void npcm7xx_kcs_write_data_out(struct kcs_bmc *kcs_bmc, u8 data)
 {
-	struct npcm7xx_kcs *npcm7xx_kcs = kcs_bmc_priv(kcs_bmc);
+	struct npcm7xx_kcs *npcm7xx_kcs = to_npcm7xx_kcs(kcs_bmc);
 	npcm7xx_kcs_outb(npcm7xx_kcs, npcm7xx_kcs->reg->dob, data);
 }
 
-static void npcm7xx_kcs_enable_channel(struct kcs_bmc *kcs_bmc, bool enable)
+static void npcm7xx_kcs_enable_channel(struct npcm7xx_kcs *npcm7xx_kcs, bool enable)
 {
-	struct npcm7xx_kcs *priv = kcs_bmc_priv(kcs_bmc);
-
-	regmap_update_bits(priv->map, priv->reg->ctl, KCS_CTL_IBFIE,
-			   enable ? KCS_CTL_IBFIE : 0);
+	regmap_update_bits(npcm7xx_kcs->map, npcm7xx_kcs->reg->ctl,
+			   KCS_CTL_IBFIE, enable ? KCS_CTL_IBFIE : 0);
 
-	regmap_update_bits(priv->map, priv->reg->ie, KCS_IE_IRQE | KCS_IE_HIRQE,
+	regmap_update_bits(npcm7xx_kcs->map, npcm7xx_kcs->reg->ie,
+			   KCS_IE_IRQE | KCS_IE_HIRQE,
 			   enable ? KCS_IE_IRQE | KCS_IE_HIRQE : 0);
 }
 
@@ -142,7 +147,7 @@ static int npcm7xx_kcs_config_irq(struct kcs_bmc *kcs_bmc,
 static int npcm7xx_kcs_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
-	struct npcm7xx_kcs *priv;
+	struct npcm7xx_kcs *npcm7xx_kcs;
 	struct kcs_bmc *kcs_bmc;
 	u32 chan;
 	int rc;
@@ -153,49 +158,47 @@ static int npcm7xx_kcs_probe(struct platform_device *pdev)
 		return -ENODEV;
 	}
 
-	kcs_bmc = kcs_bmc_alloc(dev, sizeof(*priv), chan);
-	if (!kcs_bmc)
+	npcm7xx_kcs = devm_kzalloc(dev, sizeof(*npcm7xx_kcs), GFP_KERNEL);
+	if (!npcm7xx_kcs)
 		return -ENOMEM;
+	kcs_bmc = &npcm7xx_kcs->kcs_bmc;
+	dev_set_drvdata(dev, kcs_bmc);
 
-	priv = kcs_bmc_priv(kcs_bmc);
-	priv->map = syscon_node_to_regmap(dev->parent->of_node);
-	if (IS_ERR(priv->map)) {
+	npcm7xx_kcs->map = syscon_node_to_regmap(dev->parent->of_node);
+	if (IS_ERR(npcm7xx_kcs->map)) {
 		dev_err(dev, "Couldn't get regmap\n");
 		return -ENODEV;
 	}
-	priv->reg = &npcm7xx_kcs_reg_tbl[chan - 1];
+	npcm7xx_kcs->reg = &npcm7xx_kcs_reg_tbl[chan - 1];
 
 	kcs_bmc->read_status = npcm7xx_kcs_read_status;
 	kcs_bmc->write_status = npcm7xx_kcs_write_status;
 	kcs_bmc->read_data_in = npcm7xx_kcs_read_data_in;
 	kcs_bmc->write_data_out = npcm7xx_kcs_write_data_out;
 
-	dev_set_drvdata(dev, kcs_bmc);
+	npcm7xx_kcs_enable_channel(npcm7xx_kcs, true);
 
-	npcm7xx_kcs_enable_channel(kcs_bmc, true);
-	rc = npcm7xx_kcs_config_irq(kcs_bmc, pdev);
+	rc = kcs_bmc_init(kcs_bmc, dev, chan);
 	if (rc)
 		return rc;
 
-	rc = misc_register(&kcs_bmc->miscdev);
+	rc = npcm7xx_kcs_config_irq(kcs_bmc, pdev);
 	if (rc) {
-		dev_err(dev, "Unable to register device\n");
+		kcs_bmc_stop(kcs_bmc);
 		return rc;
 	}
 
 	dev_dbg(&pdev->dev,
-		"Probed KCS device %d (DIB=0x%x, DOB=0x%x, STS=0x%x )\n",
-		chan, priv->reg->dib, priv->reg->dob, priv->reg->sts);
+		"Probed KCS device %d (DIB=0x%x, DOB=0x%x, STS=0x%x)\n",
+		chan, npcm7xx_kcs->reg->dib, npcm7xx_kcs->reg->dob,
+		npcm7xx_kcs->reg->sts);
 
 	return 0;
 }
 
 static int npcm7xx_kcs_remove(struct platform_device *pdev)
 {
-	struct kcs_bmc *kcs_bmc = dev_get_drvdata(&pdev->dev);
-
-	misc_deregister(&kcs_bmc->miscdev);
-
+	kcs_bmc_stop(dev_get_drvdata(&pdev->dev));
 	return 0;
 }
 
-- 
2.30.0.478.g8a0d178c01-goog

[PATCH 4/4] ipmi: kcs_bmc: Simplify state machine

[William A. Kennington III]

This makes a few changes, notably:

  - Removes the mutex primitive as it is not needed in the file
    operation path. The read path had multiple spinlock acquisitions
    with state changes that don't appear safe to reordering,
    specifically around forced aborts.

  - Removes multiple per-channel allocated data buffers, opting to only
    store the necessary data for the currently in flight transaction.
    Since only 1 data buffer can really be used at a given time, we
    don't need the previous 2-3.

  - Reduces the number of states in the KCS state machine. There are
    some states, like the second abort state, that effectively duplicate
    others.

  - Cleans up the initialization and register setting logic to include
    bits that were missed previously.

Signed-off-by: William A. Kennington III <wak@google.com>
---
 drivers/char/ipmi/kcs_bmc.c | 301 ++++++++++++++----------------------
 drivers/char/ipmi/kcs_bmc.h |  49 ++----
 2 files changed, 132 insertions(+), 218 deletions(-)

diff --git a/drivers/char/ipmi/kcs_bmc.c b/drivers/char/ipmi/kcs_bmc.c
index 16a378d79db9..f2fa43a516da 100644
--- a/drivers/char/ipmi/kcs_bmc.c
+++ b/drivers/char/ipmi/kcs_bmc.c
@@ -11,21 +11,14 @@
 #include <linux/module.h>
 #include <linux/platform_device.h>
 #include <linux/poll.h>
-#include <linux/sched.h>
-#include <linux/slab.h>
 
 #include "kcs_bmc.h"
 
 #define DEVICE_NAME "ipmi-kcs"
 
-#define KCS_MSG_BUFSIZ    1000
-
-#define KCS_ZERO_DATA     0
-
-
 /* IPMI 2.0 - Table 9-1, KCS Interface Status Register Bits */
 #define KCS_STATUS_STATE(state) (state << 6)
-#define KCS_STATUS_STATE_MASK   GENMASK(7, 6)
+#define KCS_STATUS_STATE_MASK   GENMASK(7, 4)
 #define KCS_STATUS_CMD_DAT      BIT(3)
 #define KCS_STATUS_SMS_ATN      BIT(2)
 #define KCS_STATUS_IBF          BIT(1)
@@ -33,19 +26,19 @@
 
 /* IPMI 2.0 - Table 9-2, KCS Interface State Bits */
 enum kcs_states {
-	IDLE_STATE  = 0,
-	READ_STATE  = 1,
-	WRITE_STATE = 2,
-	ERROR_STATE = 3,
+	KCS_STATE_IDLE  = 0,
+	KCS_STATE_READ  = 1,
+	KCS_STATE_WRITE = 2,
+	KCS_STATE_ERROR = 3,
 };
 
 /* IPMI 2.0 - Table 9-3, KCS Interface Control Codes */
 #define KCS_CMD_GET_STATUS_ABORT  0x60
 #define KCS_CMD_WRITE_START       0x61
 #define KCS_CMD_WRITE_END         0x62
-#define KCS_CMD_READ_BYTE         0x68
+#define KCS_DATA_READ_BYTE        0x68
 
-static void update_status_bits(struct kcs_bmc *kcs_bmc, u8 mask, u8 val)
+static void kcs_update_status_bits(struct kcs_bmc *kcs_bmc, u8 mask, u8 val)
 {
 	u8 tmp = kcs_bmc->read_status(kcs_bmc);
 	tmp &= ~mask;
@@ -53,95 +46,70 @@ static void update_status_bits(struct kcs_bmc *kcs_bmc, u8 mask, u8 val)
 	kcs_bmc->write_status(kcs_bmc, tmp);
 }
 
-static inline void set_state(struct kcs_bmc *kcs_bmc, u8 state)
+static void kcs_set_state(struct kcs_bmc *kcs_bmc, enum kcs_states state)
 {
-	update_status_bits(kcs_bmc, KCS_STATUS_STATE_MASK,
+	kcs_update_status_bits(kcs_bmc, KCS_STATUS_STATE_MASK,
 					KCS_STATUS_STATE(state));
 }
 
-static void kcs_force_abort(struct kcs_bmc *kcs_bmc)
+static void kcs_force_abort(struct kcs_bmc *kcs_bmc, enum kcs_errors error)
 {
-	set_state(kcs_bmc, ERROR_STATE);
+	kcs_bmc->phase = KCS_PHASE_ERROR;
+	kcs_bmc->error = error;
+	kcs_set_state(kcs_bmc, KCS_STATE_ERROR);
 	kcs_bmc->read_data_in(kcs_bmc);
-	kcs_bmc->write_data_out(kcs_bmc, KCS_ZERO_DATA);
+	kcs_bmc->write_data_out(kcs_bmc, 0);
+}
 
-	kcs_bmc->phase = KCS_PHASE_ERROR;
-	kcs_bmc->data_in_avail = false;
-	kcs_bmc->data_in_idx = 0;
+static void kcs_pump_data_out(struct kcs_bmc *kcs_bmc)
+{
+	kcs_bmc->write_data_out(kcs_bmc,
+			kcs_bmc->data_idx >= kcs_bmc->data_len ?
+			0 : kcs_bmc->data[kcs_bmc->data_idx++]);
 }
 
 static void kcs_bmc_handle_data(struct kcs_bmc *kcs_bmc)
 {
-	u8 data;
-
 	switch (kcs_bmc->phase) {
-	case KCS_PHASE_WRITE_START:
-		kcs_bmc->phase = KCS_PHASE_WRITE_DATA;
-		fallthrough;
-
 	case KCS_PHASE_WRITE_DATA:
-		if (kcs_bmc->data_in_idx < KCS_MSG_BUFSIZ) {
-			set_state(kcs_bmc, WRITE_STATE);
-			kcs_bmc->write_data_out(kcs_bmc, KCS_ZERO_DATA);
-			kcs_bmc->data_in[kcs_bmc->data_in_idx++] =
-						kcs_bmc->read_data_in(kcs_bmc);
-		} else {
-			kcs_force_abort(kcs_bmc);
-			kcs_bmc->error = KCS_LENGTH_ERROR;
-		}
+		if (kcs_bmc->data_len >= KCS_MSG_BUFSIZ)
+			return kcs_force_abort(kcs_bmc, KCS_LENGTH_ERROR);
+		kcs_bmc->data[kcs_bmc->data_len++] =
+			kcs_bmc->read_data_in(kcs_bmc);
+		kcs_bmc->write_data_out(kcs_bmc, 0);
 		break;
 
-	case KCS_PHASE_WRITE_END_CMD:
-		if (kcs_bmc->data_in_idx < KCS_MSG_BUFSIZ) {
-			set_state(kcs_bmc, READ_STATE);
-			kcs_bmc->data_in[kcs_bmc->data_in_idx++] =
-						kcs_bmc->read_data_in(kcs_bmc);
-			kcs_bmc->phase = KCS_PHASE_WRITE_DONE;
-			kcs_bmc->data_in_avail = true;
-			wake_up_interruptible(&kcs_bmc->queue);
-		} else {
-			kcs_force_abort(kcs_bmc);
-			kcs_bmc->error = KCS_LENGTH_ERROR;
-		}
+	case KCS_PHASE_WRITE_END:
+		if (kcs_bmc->data_len < 1 && kcs_bmc->data_len >= KCS_MSG_BUFSIZ)
+			return kcs_force_abort(kcs_bmc, KCS_LENGTH_ERROR);
+		kcs_bmc->phase = KCS_PHASE_WRITE_DONE;
+		kcs_set_state(kcs_bmc, KCS_STATE_READ);
+		kcs_bmc->data[kcs_bmc->data_len++] =
+			kcs_bmc->read_data_in(kcs_bmc);
+		wake_up_interruptible(&kcs_bmc->read_ready);
 		break;
 
 	case KCS_PHASE_READ:
-		if (kcs_bmc->data_out_idx == kcs_bmc->data_out_len)
-			set_state(kcs_bmc, IDLE_STATE);
-
-		data = kcs_bmc->read_data_in(kcs_bmc);
-		if (data != KCS_CMD_READ_BYTE) {
-			set_state(kcs_bmc, ERROR_STATE);
-			kcs_bmc->write_data_out(kcs_bmc, KCS_ZERO_DATA);
-			break;
-		}
-
-		if (kcs_bmc->data_out_idx == kcs_bmc->data_out_len) {
-			kcs_bmc->write_data_out(kcs_bmc, KCS_ZERO_DATA);
+		if (kcs_bmc->data_idx >= kcs_bmc->data_len) {
 			kcs_bmc->phase = KCS_PHASE_IDLE;
-			break;
+			kcs_set_state(kcs_bmc, KCS_STATE_IDLE);
 		}
-
-		kcs_bmc->write_data_out(kcs_bmc,
-			kcs_bmc->data_out[kcs_bmc->data_out_idx++]);
+		if (kcs_bmc->read_data_in(kcs_bmc) != KCS_DATA_READ_BYTE)
+			return kcs_force_abort(kcs_bmc, KCS_UNSPECIFIED_ERROR);
+		kcs_pump_data_out(kcs_bmc);
 		break;
 
-	case KCS_PHASE_ABORT_ERROR1:
-		set_state(kcs_bmc, READ_STATE);
-		kcs_bmc->read_data_in(kcs_bmc);
+	case KCS_PHASE_ABORT:
+		kcs_set_state(kcs_bmc, KCS_STATE_READ);
+		if (kcs_bmc->read_data_in(kcs_bmc) != 0)
+			return kcs_force_abort(kcs_bmc, KCS_UNSPECIFIED_ERROR);
+		kcs_bmc->phase = KCS_PHASE_READ;
+		kcs_bmc->data_len = 0;
 		kcs_bmc->write_data_out(kcs_bmc, kcs_bmc->error);
-		kcs_bmc->phase = KCS_PHASE_ABORT_ERROR2;
-		break;
-
-	case KCS_PHASE_ABORT_ERROR2:
-		set_state(kcs_bmc, IDLE_STATE);
-		kcs_bmc->read_data_in(kcs_bmc);
-		kcs_bmc->write_data_out(kcs_bmc, KCS_ZERO_DATA);
-		kcs_bmc->phase = KCS_PHASE_IDLE;
 		break;
 
 	default:
-		kcs_force_abort(kcs_bmc);
+		kcs_force_abort(kcs_bmc, KCS_UNSPECIFIED_ERROR);
 		break;
 	}
 }
@@ -150,40 +118,32 @@ static void kcs_bmc_handle_cmd(struct kcs_bmc *kcs_bmc)
 {
 	u8 cmd;
 
-	set_state(kcs_bmc, WRITE_STATE);
-	kcs_bmc->write_data_out(kcs_bmc, KCS_ZERO_DATA);
-
+	kcs_set_state(kcs_bmc, KCS_STATE_WRITE);
 	cmd = kcs_bmc->read_data_in(kcs_bmc);
+	kcs_bmc->write_data_out(kcs_bmc, 0);
+
 	switch (cmd) {
 	case KCS_CMD_WRITE_START:
-		kcs_bmc->phase = KCS_PHASE_WRITE_START;
-		kcs_bmc->error = KCS_NO_ERROR;
-		kcs_bmc->data_in_avail = false;
-		kcs_bmc->data_in_idx = 0;
+		if (kcs_bmc->phase != KCS_PHASE_IDLE)
+			return kcs_force_abort(kcs_bmc, KCS_UNSPECIFIED_ERROR);
+		kcs_bmc->phase = KCS_PHASE_WRITE_DATA;
+		kcs_bmc->data_len = 0;
 		break;
 
 	case KCS_CMD_WRITE_END:
-		if (kcs_bmc->phase != KCS_PHASE_WRITE_DATA) {
-			kcs_force_abort(kcs_bmc);
-			break;
-		}
-
-		kcs_bmc->phase = KCS_PHASE_WRITE_END_CMD;
+		if (kcs_bmc->phase != KCS_PHASE_WRITE_DATA)
+			return kcs_force_abort(kcs_bmc, KCS_UNSPECIFIED_ERROR);
+		kcs_bmc->phase = KCS_PHASE_WRITE_END;
 		break;
 
 	case KCS_CMD_GET_STATUS_ABORT:
-		if (kcs_bmc->error == KCS_NO_ERROR)
+		if (kcs_bmc->phase != KCS_PHASE_ERROR)
 			kcs_bmc->error = KCS_ABORTED_BY_COMMAND;
-
-		kcs_bmc->phase = KCS_PHASE_ABORT_ERROR1;
-		kcs_bmc->data_in_avail = false;
-		kcs_bmc->data_in_idx = 0;
+		kcs_bmc->phase = KCS_PHASE_ABORT;
 		break;
 
 	default:
-		kcs_force_abort(kcs_bmc);
-		kcs_bmc->error = KCS_ILLEGAL_CONTROL_CODE;
-		break;
+		kcs_force_abort(kcs_bmc, KCS_ILLEGAL_CONTROL_CODE);
 	}
 }
 
@@ -198,8 +158,8 @@ irqreturn_t kcs_bmc_irq(int irq, void *arg)
 
 	status = kcs_bmc->read_status(kcs_bmc);
 	if (status & KCS_STATUS_IBF) {
-		if (!kcs_bmc->running)
-			kcs_force_abort(kcs_bmc);
+		if (!kcs_bmc->open)
+			kcs_force_abort(kcs_bmc, KCS_NO_ERROR);
 		else if (status & KCS_STATUS_CMD_DAT)
 			kcs_bmc_handle_cmd(kcs_bmc);
 		else
@@ -218,30 +178,15 @@ static inline struct kcs_bmc *to_kcs_bmc(struct file *filp)
 	return container_of(filp->private_data, struct kcs_bmc, miscdev);
 }
 
-static int kcs_bmc_open(struct inode *inode, struct file *filp)
-{
-	struct kcs_bmc *kcs_bmc = to_kcs_bmc(filp);
-	int ret = 0;
-
-	spin_lock_irq(&kcs_bmc->lock);
-	if (!kcs_bmc->running)
-		kcs_bmc->running = 1;
-	else
-		ret = -EBUSY;
-	spin_unlock_irq(&kcs_bmc->lock);
-
-	return ret;
-}
-
 static __poll_t kcs_bmc_poll(struct file *filp, poll_table *wait)
 {
 	struct kcs_bmc *kcs_bmc = to_kcs_bmc(filp);
 	__poll_t mask = 0;
 
-	poll_wait(filp, &kcs_bmc->queue, wait);
+	poll_wait(filp, &kcs_bmc->read_ready, wait);
 
 	spin_lock_irq(&kcs_bmc->lock);
-	if (kcs_bmc->data_in_avail)
+	if (kcs_bmc->phase == KCS_PHASE_WRITE_DONE)
 		mask |= EPOLLIN;
 	spin_unlock_irq(&kcs_bmc->lock);
 
@@ -252,57 +197,36 @@ static ssize_t kcs_bmc_read(struct file *filp, char __user *buf,
 			    size_t count, loff_t *ppos)
 {
 	struct kcs_bmc *kcs_bmc = to_kcs_bmc(filp);
-	bool data_avail;
-	size_t data_len;
+	u8 data_in[KCS_MSG_BUFSIZ];
 	ssize_t ret;
 
-	if (!(filp->f_flags & O_NONBLOCK))
-		wait_event_interruptible(kcs_bmc->queue,
-					 kcs_bmc->data_in_avail);
-
-	mutex_lock(&kcs_bmc->mutex);
-
 	spin_lock_irq(&kcs_bmc->lock);
-	data_avail = kcs_bmc->data_in_avail;
-	if (data_avail) {
-		data_len = kcs_bmc->data_in_idx;
-		memcpy(kcs_bmc->kbuffer, kcs_bmc->data_in, data_len);
-	}
-	spin_unlock_irq(&kcs_bmc->lock);
 
-	if (!data_avail) {
-		ret = -EAGAIN;
-		goto out_unlock;
+	while (kcs_bmc->phase != KCS_PHASE_WRITE_DONE) {
+		if (filp->f_flags & O_NONBLOCK) {
+			spin_unlock_irq(&kcs_bmc->lock);
+			return -EAGAIN;
+		}
+		ret = wait_event_interruptible_lock_irq(kcs_bmc->read_ready,
+					 kcs_bmc->phase == KCS_PHASE_WRITE_DONE, kcs_bmc->lock);
+		if (ret) {
+			spin_unlock_irq(&kcs_bmc->lock);
+			return ret;
+		}
 	}
 
-	if (count < data_len) {
-		spin_lock_irq(&kcs_bmc->lock);
-		kcs_force_abort(kcs_bmc);
+	if (kcs_bmc->data_len > count) {
 		spin_unlock_irq(&kcs_bmc->lock);
-
-		ret = -EOVERFLOW;
-		goto out_unlock;
+		return -EOVERFLOW;
 	}
 
-	if (copy_to_user(buf, kcs_bmc->kbuffer, data_len)) {
-		ret = -EFAULT;
-		goto out_unlock;
-	}
-
-	ret = data_len;
-
-	spin_lock_irq(&kcs_bmc->lock);
-	if (kcs_bmc->phase == KCS_PHASE_WRITE_DONE) {
-		kcs_bmc->phase = KCS_PHASE_WAIT_READ;
-		kcs_bmc->data_in_avail = false;
-		kcs_bmc->data_in_idx = 0;
-	} else {
-		ret = -EAGAIN;
-	}
+	ret = kcs_bmc->data_len;
+	memcpy(data_in, kcs_bmc->data, kcs_bmc->data_len);
+	kcs_bmc->phase = KCS_PHASE_WAIT_READ;
 	spin_unlock_irq(&kcs_bmc->lock);
 
-out_unlock:
-	mutex_unlock(&kcs_bmc->mutex);
+	if (copy_to_user(buf, data_in, ret))
+		return -EFAULT;
 
 	return ret;
 }
@@ -312,34 +236,28 @@ static ssize_t kcs_bmc_write(struct file *filp, const char __user *buf,
 {
 	struct kcs_bmc *kcs_bmc = to_kcs_bmc(filp);
 	ssize_t ret;
+	u8 data_out[KCS_MSG_BUFSIZ];
 
 	/* a minimum response size '3' : netfn + cmd + ccode */
-	if (count < 3 || count > KCS_MSG_BUFSIZ)
+	if (count < 3 || count > sizeof(data_out))
 		return -EINVAL;
 
-	mutex_lock(&kcs_bmc->mutex);
-
-	if (copy_from_user(kcs_bmc->kbuffer, buf, count)) {
-		ret = -EFAULT;
-		goto out_unlock;
-	}
+	if (copy_from_user(data_out, buf, count))
+		return -EFAULT;
 
 	spin_lock_irq(&kcs_bmc->lock);
 	if (kcs_bmc->phase == KCS_PHASE_WAIT_READ) {
 		kcs_bmc->phase = KCS_PHASE_READ;
-		kcs_bmc->data_out_idx = 1;
-		kcs_bmc->data_out_len = count;
-		memcpy(kcs_bmc->data_out, kcs_bmc->kbuffer, count);
-		kcs_bmc->write_data_out(kcs_bmc, kcs_bmc->data_out[0]);
+		memcpy(kcs_bmc->data, data_out, count);
+		kcs_bmc->data_idx = 0;
+		kcs_bmc->data_len = count;
+		kcs_pump_data_out(kcs_bmc);
 		ret = count;
 	} else {
-		ret = -EINVAL;
+		ret = -EBUSY;
 	}
 	spin_unlock_irq(&kcs_bmc->lock);
 
-out_unlock:
-	mutex_unlock(&kcs_bmc->mutex);
-
 	return ret;
 }
 
@@ -350,27 +268,42 @@ static long kcs_bmc_ioctl(struct file *filp, unsigned int cmd,
 	long ret = 0;
 
 	spin_lock_irq(&kcs_bmc->lock);
-
 	switch (cmd) {
 	case IPMI_BMC_IOCTL_SET_SMS_ATN:
-		update_status_bits(kcs_bmc, KCS_STATUS_SMS_ATN,
-				   KCS_STATUS_SMS_ATN);
+		kcs_update_status_bits(kcs_bmc, KCS_STATUS_SMS_ATN,
+				KCS_STATUS_SMS_ATN);
 		break;
 
 	case IPMI_BMC_IOCTL_CLEAR_SMS_ATN:
-		update_status_bits(kcs_bmc, KCS_STATUS_SMS_ATN,
-				   0);
+		kcs_update_status_bits(kcs_bmc, KCS_STATUS_SMS_ATN, 0);
 		break;
 
 	case IPMI_BMC_IOCTL_FORCE_ABORT:
-		kcs_force_abort(kcs_bmc);
+		kcs_force_abort(kcs_bmc, KCS_NO_ERROR);
 		break;
 
 	default:
 		ret = -EINVAL;
 		break;
 	}
+	spin_unlock_irq(&kcs_bmc->lock);
 
+	return ret;
+}
+
+static int kcs_bmc_open(struct inode *inode, struct file *filp)
+{
+	struct kcs_bmc *kcs_bmc = to_kcs_bmc(filp);
+	int ret = 0;
+
+	spin_lock_irq(&kcs_bmc->lock);
+	if (kcs_bmc->open) {
+		ret = -EBUSY;
+	} else {
+		kcs_bmc->open = true;
+		kcs_update_status_bits(kcs_bmc, KCS_STATUS_SMS_ATN, 0);
+		kcs_force_abort(kcs_bmc, KCS_NO_ERROR);
+	}
 	spin_unlock_irq(&kcs_bmc->lock);
 
 	return ret;
@@ -381,8 +314,9 @@ static int kcs_bmc_release(struct inode *inode, struct file *filp)
 	struct kcs_bmc *kcs_bmc = to_kcs_bmc(filp);
 
 	spin_lock_irq(&kcs_bmc->lock);
-	kcs_bmc->running = 0;
-	kcs_force_abort(kcs_bmc);
+	kcs_bmc->open = false;
+	kcs_update_status_bits(kcs_bmc, KCS_STATUS_SMS_ATN, 0);
+	kcs_force_abort(kcs_bmc, KCS_NO_ERROR);
 	spin_unlock_irq(&kcs_bmc->lock);
 
 	return 0;
@@ -403,12 +337,9 @@ int kcs_bmc_init(struct kcs_bmc *kcs_bmc, struct device *dev, u32 channel)
 	int rc;
 
 	spin_lock_init(&kcs_bmc->lock);
-	mutex_init(&kcs_bmc->mutex);
-	init_waitqueue_head(&kcs_bmc->queue);
+	init_waitqueue_head(&kcs_bmc->read_ready);
 
-	kcs_bmc->data_in = devm_kmalloc(dev, KCS_MSG_BUFSIZ, GFP_KERNEL);
-	kcs_bmc->data_out = devm_kmalloc(dev, KCS_MSG_BUFSIZ, GFP_KERNEL);
-	kcs_bmc->kbuffer = devm_kmalloc(dev, KCS_MSG_BUFSIZ, GFP_KERNEL);
+	kcs_bmc->open = false;
 
 	kcs_bmc->miscdev.fops = &kcs_bmc_fops;
 	kcs_bmc->miscdev.minor = MISC_DYNAMIC_MINOR;
diff --git a/drivers/char/ipmi/kcs_bmc.h b/drivers/char/ipmi/kcs_bmc.h
index d65ffd479e9b..48c4fa8772d7 100644
--- a/drivers/char/ipmi/kcs_bmc.h
+++ b/drivers/char/ipmi/kcs_bmc.h
@@ -8,42 +8,37 @@
 
 #include <linux/irqreturn.h>
 #include <linux/miscdevice.h>
+#include <linux/spinlock.h>
+#include <linux/wait.h>
 
 /* Different phases of the KCS BMC module.
  *  KCS_PHASE_IDLE:
  *            BMC should not be expecting nor sending any data.
- *  KCS_PHASE_WRITE_START:
- *            BMC is receiving a WRITE_START command from system software.
  *  KCS_PHASE_WRITE_DATA:
  *            BMC is receiving a data byte from system software.
- *  KCS_PHASE_WRITE_END_CMD:
+ *  KCS_PHASE_WRITE_END:
  *            BMC is waiting a last data byte from system software.
  *  KCS_PHASE_WRITE_DONE:
- *            BMC has received the whole request from system software.
+ *            BMC is waiting to send request to the upper IPMI service.
  *  KCS_PHASE_WAIT_READ:
  *            BMC is waiting the response from the upper IPMI service.
  *  KCS_PHASE_READ:
  *            BMC is transferring the response to system software.
- *  KCS_PHASE_ABORT_ERROR1:
+ *  KCS_PHASE_ABORT:
  *            BMC is waiting error status request from system software.
- *  KCS_PHASE_ABORT_ERROR2:
- *            BMC is waiting for idle status afer error from system software.
  *  KCS_PHASE_ERROR:
  *            BMC has detected a protocol violation at the interface level.
  */
 enum kcs_phases {
 	KCS_PHASE_IDLE,
 
-	KCS_PHASE_WRITE_START,
 	KCS_PHASE_WRITE_DATA,
-	KCS_PHASE_WRITE_END_CMD,
+	KCS_PHASE_WRITE_END,
 	KCS_PHASE_WRITE_DONE,
-
 	KCS_PHASE_WAIT_READ,
 	KCS_PHASE_READ,
 
-	KCS_PHASE_ABORT_ERROR1,
-	KCS_PHASE_ABORT_ERROR2,
+	KCS_PHASE_ABORT,
 	KCS_PHASE_ERROR
 };
 
@@ -56,36 +51,24 @@ enum kcs_errors {
 	KCS_UNSPECIFIED_ERROR       = 0xFF
 };
 
-/* IPMI 2.0 - 9.5, KCS Interface Registers
- * @idr: Input Data Register
- * @odr: Output Data Register
- * @str: Status Register
- */
-struct kcs_bmc {
-	spinlock_t lock;
-
-	int running;
+#define KCS_MSG_BUFSIZ    960
 
+struct kcs_bmc {
 	/* Setup by BMC KCS controller driver */
 	u8 (*read_status)(struct kcs_bmc *kcs_bmc);
 	void (*write_status)(struct kcs_bmc *kcs_bmc, u8 b);
 	u8 (*read_data_in)(struct kcs_bmc *kcs_bmc);
 	void (*write_data_out)(struct kcs_bmc *kcs_bmc, u8 b);
 
+	spinlock_t lock;
+	wait_queue_head_t read_ready;
+
+	bool open;
 	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 mutex mutex;
-	u8 *kbuffer;
+	u8 data[KCS_MSG_BUFSIZ];
+	size_t data_idx;
+	size_t data_len;
 
 	struct miscdevice miscdev;
 };
-- 
2.30.0.478.g8a0d178c01-goog