Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Zev Weiss <zweiss@equinix.com>
To:     Andrew Jeffery <andrew@aj.id.au>
CC:     "openipmi-developer@lists.sourceforge.net" 
        <openipmi-developer@lists.sourceforge.net>,
        "openbmc@lists.ozlabs.org" <openbmc@lists.ozlabs.org>,
        "minyard@acm.org" <minyard@acm.org>,
        "devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
        "ryan_chen@aspeedtech.com" <ryan_chen@aspeedtech.com>,
        "tmaimon77@gmail.com" <tmaimon77@gmail.com>,
        "linux-aspeed@lists.ozlabs.org" <linux-aspeed@lists.ozlabs.org>,
        "avifishman70@gmail.com" <avifishman70@gmail.com>,
        "venture@google.com" <venture@google.com>,
        "linus.walleij@linaro.org" <linus.walleij@linaro.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "tali.perry1@gmail.com" <tali.perry1@gmail.com>,
        "linux-gpio@vger.kernel.org" <linux-gpio@vger.kernel.org>,
        "robh+dt@kernel.org" <robh+dt@kernel.org>,
        "lee.jones@linaro.org" <lee.jones@linaro.org>,
        "chiawei_wang@aspeedtech.com" <chiawei_wang@aspeedtech.com>,
        "linux-arm-kernel@lists.infradead.org" 
        <linux-arm-kernel@lists.infradead.org>,
        "benjaminfair@google.com" <benjaminfair@google.com>
Subject: Re: [PATCH v2 10/21] ipmi: kcs_bmc: Turn the driver data-structures
 inside-out
Thread-Topic: [PATCH v2 10/21] ipmi: kcs_bmc: Turn the driver data-structures
 inside-out
Thread-Index: AQHXLPRrPdVC3RFcvUCHtDapr0x+sg==
Date:   Fri, 9 Apr 2021 03:57:12 +0000
Message-ID: <YG/Ql9z9X/mtOSvl@packtop>
References: <20210319062752.145730-1-andrew@aj.id.au>
 <20210319062752.145730-10-andrew@aj.id.au>
In-Reply-To: <20210319062752.145730-10-andrew@aj.id.au>
Accept-Language: en-US
Content-Language: en-US
Content-Type: text/plain; charset="us-ascii"
Content-ID: <17439EB88E9CC84BBF52D18548986FCE@namprd04.prod.outlook.com>
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: 4db0e9de-949d-4d8e-79a4-08d8fb0b8e65
X-MS-Exchange-CrossTenant-originalarrivaltime: 09 Apr 2021 03:57:12.3213
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 72adb271-2fc7-4afe-a5ee-9de6a59f6bfb
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: Xjx4kOtZI4OzpvNzQUEsOAxn6wlcclUL9B/cLQowyU9tx3kEXOcGxC0/Ak8VcR6yBeYUSXiT5u7ds3qpsdWmSg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM6PR04MB6281
X-Proofpoint-ORIG-GUID: JUEQpJoPCEqPaQMqpmz7kWtbMW6fmDEw
X-Proofpoint-GUID: JUEQpJoPCEqPaQMqpmz7kWtbMW6fmDEw
X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.391,18.0.761
 definitions=2021-04-09_03:2021-04-08,2021-04-09 signatures=0
X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 clxscore=1015 malwarescore=0
 suspectscore=0 priorityscore=1501 mlxlogscore=999 adultscore=0
 impostorscore=0 mlxscore=0 lowpriorityscore=0 spamscore=0 phishscore=0
 bulkscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2104060000 definitions=main-2104090025
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Fri, Mar 19, 2021 at 01:27:41AM CDT, Andrew Jeffery wrote:
>Make the KCS device drivers responsible for allocating their own memory.
>
>Until now the private data for the device driver was allocated internal
>to the private data for the chardev interface. This coupling required
>the slightly awkward API of passing through the struct size for the
>driver private data to the chardev constructor, and then retrieving a
>pointer to the driver private data from the allocated chardev memory.
>
>In addition to being awkward, the arrangement prevents the
>implementation of alternative userspace interfaces as the device driver
>private data is not independent.
>
>Peel a layer off the onion and turn the data-structures inside out by
>exploiting container_of() and embedding `struct kcs_device` in the
>driver private data.
>
>Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
>---
> drivers/char/ipmi/kcs_bmc.c           | 15 +++++--
> drivers/char/ipmi/kcs_bmc.h           | 12 ++----
> drivers/char/ipmi/kcs_bmc_aspeed.c    | 60 ++++++++++++++++-----------
> drivers/char/ipmi/kcs_bmc_cdev_ipmi.c | 60 ++++++++++++++++++---------
> drivers/char/ipmi/kcs_bmc_npcm7xx.c   | 37 ++++++++++-------
> 5 files changed, 113 insertions(+), 71 deletions(-)
>
>diff --git a/drivers/char/ipmi/kcs_bmc.c b/drivers/char/ipmi/kcs_bmc.c
>index ef5c48ffe74a..709b6bdec165 100644
>--- a/drivers/char/ipmi/kcs_bmc.c
>+++ b/drivers/char/ipmi/kcs_bmc.c
>@@ -44,12 +44,19 @@ int kcs_bmc_handle_event(struct kcs_bmc *kcs_bmc)
> }
> EXPORT_SYMBOL(kcs_bmc_handle_event);
>
>-struct kcs_bmc *kcs_bmc_ipmi_alloc(struct device *dev, int sizeof_priv, u=
32 channel);
>-struct kcs_bmc *kcs_bmc_alloc(struct device *dev, int sizeof_priv, u32 ch=
annel)
>+int kcs_bmc_ipmi_attach_cdev(struct kcs_bmc *kcs_bmc);

Another declaration perhaps intended for kcs_bmc.h?

>+int kcs_bmc_add_device(struct kcs_bmc *kcs_bmc)
> {
>-	return kcs_bmc_ipmi_alloc(dev, sizeof_priv, channel);
>+	return kcs_bmc_ipmi_attach_cdev(kcs_bmc);
> }
>-EXPORT_SYMBOL(kcs_bmc_alloc);
>+EXPORT_SYMBOL(kcs_bmc_add_device);
>+
>+int kcs_bmc_ipmi_detach_cdev(struct kcs_bmc *kcs_bmc);

Here too.

>+int kcs_bmc_remove_device(struct kcs_bmc *kcs_bmc)
>+{
>+	return kcs_bmc_ipmi_detach_cdev(kcs_bmc);
>+}
>+EXPORT_SYMBOL(kcs_bmc_remove_device);
>
> 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 febea0c8deb4..bf0ae327997f 100644
>--- a/drivers/char/ipmi/kcs_bmc.h
>+++ b/drivers/char/ipmi/kcs_bmc.h
>@@ -67,6 +67,8 @@ struct kcs_ioreg {
> };
>
> struct kcs_bmc {
>+	struct device *dev;
>+
> 	spinlock_t lock;
>
> 	u32 channel;
>@@ -94,17 +96,11 @@ 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;
>-}
>-
> int kcs_bmc_handle_event(struct kcs_bmc *kcs_bmc);
>-struct kcs_bmc *kcs_bmc_alloc(struct device *dev, int sizeof_priv, u32 ch=
annel);
>+int kcs_bmc_add_device(struct kcs_bmc *kcs_bmc);
>+int kcs_bmc_remove_device(struct kcs_bmc *kcs_bmc);
>
> u8 kcs_bmc_read_data(struct kcs_bmc *kcs_bmc);
> void kcs_bmc_write_data(struct kcs_bmc *kcs_bmc, u8 data);
>diff --git a/drivers/char/ipmi/kcs_bmc_aspeed.c b/drivers/char/ipmi/kcs_bm=
c_aspeed.c
>index 630cf095560e..0416ac78ce68 100644
>--- a/drivers/char/ipmi/kcs_bmc_aspeed.c
>+++ b/drivers/char/ipmi/kcs_bmc_aspeed.c
>@@ -61,6 +61,8 @@
> #define LPC_STR4             0x11C
>
> struct aspeed_kcs_bmc {
>+	struct kcs_bmc kcs_bmc;
>+
> 	struct regmap *map;
> };
>
>@@ -69,9 +71,14 @@ struct aspeed_kcs_of_ops {
> 	int (*get_io_address)(struct platform_device *pdev);
> };
>
>+static inline struct aspeed_kcs_bmc *to_aspeed_kcs_bmc(struct kcs_bmc *kc=
s_bmc)
>+{
>+	return container_of(kcs_bmc, struct aspeed_kcs_bmc, kcs_bmc);
>+}
>+
> static u8 aspeed_kcs_inb(struct kcs_bmc *kcs_bmc, u32 reg)
> {
>-	struct aspeed_kcs_bmc *priv =3D kcs_bmc_priv(kcs_bmc);
>+	struct aspeed_kcs_bmc *priv =3D to_aspeed_kcs_bmc(kcs_bmc);
> 	u32 val =3D 0;
> 	int rc;
>
>@@ -83,7 +90,7 @@ static u8 aspeed_kcs_inb(struct kcs_bmc *kcs_bmc, u32 re=
g)
>
> static void aspeed_kcs_outb(struct kcs_bmc *kcs_bmc, u32 reg, u8 data)
> {
>-	struct aspeed_kcs_bmc *priv =3D kcs_bmc_priv(kcs_bmc);
>+	struct aspeed_kcs_bmc *priv =3D to_aspeed_kcs_bmc(kcs_bmc);
> 	int rc;
>
> 	rc =3D regmap_write(priv->map, reg, data);
>@@ -92,7 +99,7 @@ static void aspeed_kcs_outb(struct kcs_bmc *kcs_bmc, u32=
 reg, u8 data)
>
> static void aspeed_kcs_updateb(struct kcs_bmc *kcs_bmc, u32 reg, u8 mask,=
 u8 val)
> {
>-	struct aspeed_kcs_bmc *priv =3D kcs_bmc_priv(kcs_bmc);
>+	struct aspeed_kcs_bmc *priv =3D to_aspeed_kcs_bmc(kcs_bmc);
> 	int rc;
>
> 	rc =3D regmap_update_bits(priv->map, reg, mask, val);
>@@ -114,7 +121,7 @@ static void aspeed_kcs_updateb(struct kcs_bmc *kcs_bmc=
, u32 reg, u8 mask, u8 val
>  */
> static void aspeed_kcs_set_address(struct kcs_bmc *kcs_bmc, u16 addr)
> {
>-	struct aspeed_kcs_bmc *priv =3D kcs_bmc_priv(kcs_bmc);
>+	struct aspeed_kcs_bmc *priv =3D to_aspeed_kcs_bmc(kcs_bmc);
>
> 	switch (kcs_bmc->channel) {
> 	case 1:
>@@ -148,7 +155,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 enabl=
e)
> {
>-	struct aspeed_kcs_bmc *priv =3D kcs_bmc_priv(kcs_bmc);
>+	struct aspeed_kcs_bmc *priv =3D to_aspeed_kcs_bmc(kcs_bmc);
>
> 	switch (kcs_bmc->channel) {
> 	case 1:
>@@ -323,16 +330,16 @@ static int aspeed_kcs_of_v2_get_io_address(struct pl=
atform_device *pdev)
> static int aspeed_kcs_probe(struct platform_device *pdev)
> {
> 	const struct aspeed_kcs_of_ops *ops;
>-	struct device *dev =3D &pdev->dev;
>+	struct aspeed_kcs_bmc *priv;
> 	struct kcs_bmc *kcs_bmc;
> 	struct device_node *np;
> 	int rc, channel, addr;
>
>-	np =3D dev->of_node->parent;
>+	np =3D pdev->dev.of_node->parent;
> 	if (!of_device_is_compatible(np, "aspeed,ast2400-lpc-v2") &&
> 	    !of_device_is_compatible(np, "aspeed,ast2500-lpc-v2") &&
> 	    !of_device_is_compatible(np, "aspeed,ast2600-lpc-v2")) {
>-		dev_err(dev, "unsupported LPC device binding\n");
>+		dev_err(&pdev->dev, "unsupported LPC device binding\n");
> 		return -ENODEV;
> 	}
> 	ops =3D of_device_get_match_data(&pdev->dev);
>@@ -343,18 +350,27 @@ static int aspeed_kcs_probe(struct platform_device *=
pdev)
> 	if (channel < 0)
> 		return channel;
>
>-	kcs_bmc =3D kcs_bmc_alloc(&pdev->dev, sizeof(struct aspeed_kcs_bmc), cha=
nnel);
>-	if (!kcs_bmc)
>+	addr =3D ops->get_io_address(pdev);
>+	if (addr < 0)
>+		return addr;
>+
>+	priv =3D devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
>+	if (!priv)
> 		return -ENOMEM;
>
>+	kcs_bmc =3D &priv->kcs_bmc;
>+	kcs_bmc->dev =3D &pdev->dev;
>+	kcs_bmc->channel =3D channel;
> 	kcs_bmc->ioreg =3D ast_kcs_bmc_ioregs[channel - 1];
> 	kcs_bmc->io_inputb =3D aspeed_kcs_inb;
> 	kcs_bmc->io_outputb =3D aspeed_kcs_outb;
> 	kcs_bmc->io_updateb =3D aspeed_kcs_updateb;
>
>-	addr =3D ops->get_io_address(pdev);
>-	if (addr < 0)
>-		return addr;
>+	priv->map =3D syscon_node_to_regmap(pdev->dev.parent->of_node);
>+	if (IS_ERR(priv->map)) {
>+		dev_err(&pdev->dev, "Couldn't get regmap\n");
>+		return -ENODEV;
>+	}

The reanimated priv->map initialization I suspect wasn't meant to
have been removed in the first place...

>
> 	aspeed_kcs_set_address(kcs_bmc, addr);
>
>@@ -362,29 +378,25 @@ static int aspeed_kcs_probe(struct platform_device *=
pdev)
> 	if (rc)
> 		return rc;
>
>-	dev_set_drvdata(dev, kcs_bmc);
>+	platform_set_drvdata(pdev, priv);
>
> 	aspeed_kcs_enable_channel(kcs_bmc, true);
>
>-	rc =3D misc_register(&kcs_bmc->miscdev);
>-	if (rc) {
>-		dev_err(dev, "Unable to register device\n");
>+	rc =3D kcs_bmc_add_device(&priv->kcs_bmc);
>+	if (rc < 0)
> 		return rc;
>-	}
>
>-	dev_dbg(&pdev->dev,
>-		"Probed KCS device %d (IDR=3D0x%x, ODR=3D0x%x, STR=3D0x%x)\n",
>-		kcs_bmc->channel, kcs_bmc->ioreg.idr, kcs_bmc->ioreg.odr,
>-		kcs_bmc->ioreg.str);
>+	dev_info(&pdev->dev, "Initialised channel %d at 0x%x\n", kcs_bmc->channe=
l, addr);

Is the dbg->info change here intentional?  (I have no particular
objection if so, but it's often a change I make myself during
testing/debugging and then forget to revert...)

>
> 	return 0;
> }
>
> static int aspeed_kcs_remove(struct platform_device *pdev)
> {
>-	struct kcs_bmc *kcs_bmc =3D dev_get_drvdata(&pdev->dev);
>+	struct aspeed_kcs_bmc *priv =3D platform_get_drvdata(pdev);
>+	struct kcs_bmc *kcs_bmc =3D &priv->kcs_bmc;
>
>-	misc_deregister(&kcs_bmc->miscdev);
>+	kcs_bmc_remove_device(kcs_bmc);

Should we propagate the return value outward here?

>
> 	return 0;
> }
>diff --git a/drivers/char/ipmi/kcs_bmc_cdev_ipmi.c b/drivers/char/ipmi/kcs=
_bmc_cdev_ipmi.c
>index 82c77994e481..0ca71c135a1a 100644
>--- a/drivers/char/ipmi/kcs_bmc_cdev_ipmi.c
>+++ b/drivers/char/ipmi/kcs_bmc_cdev_ipmi.c
>@@ -382,7 +382,7 @@ static int kcs_bmc_ipmi_release(struct inode *inode, s=
truct file *filp)
> 	return 0;
> }
>
>-static const struct file_operations kcs_bmc_fops =3D {
>+static const struct file_operations kcs_bmc_ipmi_fops =3D {
> 	.owner          =3D THIS_MODULE,
> 	.open           =3D kcs_bmc_ipmi_open,
> 	.read           =3D kcs_bmc_ipmi_read,
>@@ -392,36 +392,58 @@ static const struct file_operations kcs_bmc_fops =3D=
 {
> 	.unlocked_ioctl =3D kcs_bmc_ipmi_ioctl,
> };
>
>-struct kcs_bmc *kcs_bmc_ipmi_alloc(struct device *dev, int sizeof_priv, u=
32 channel);
>-struct kcs_bmc *kcs_bmc_ipmi_alloc(struct device *dev, int sizeof_priv, u=
32 channel)
>+int kcs_bmc_ipmi_attach_cdev(struct kcs_bmc *kcs_bmc);

Errant declaration again?

>+int kcs_bmc_ipmi_attach_cdev(struct kcs_bmc *kcs_bmc)
> {
>-	struct kcs_bmc *kcs_bmc;
>-
>-	kcs_bmc =3D 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 =3D channel;
>-
> 	mutex_init(&kcs_bmc->mutex);
> 	init_waitqueue_head(&kcs_bmc->queue);
>
>-	kcs_bmc->data_in =3D devm_kmalloc(dev, KCS_MSG_BUFSIZ, GFP_KERNEL);
>-	kcs_bmc->data_out =3D devm_kmalloc(dev, KCS_MSG_BUFSIZ, GFP_KERNEL);
>-	kcs_bmc->kbuffer =3D devm_kmalloc(dev, KCS_MSG_BUFSIZ, GFP_KERNEL);
>+	kcs_bmc->data_in =3D devm_kmalloc(kcs_bmc->dev, KCS_MSG_BUFSIZ, GFP_KERN=
EL);
>+	kcs_bmc->data_out =3D devm_kmalloc(kcs_bmc->dev, KCS_MSG_BUFSIZ, GFP_KER=
NEL);
>+	kcs_bmc->kbuffer =3D devm_kmalloc(kcs_bmc->dev, KCS_MSG_BUFSIZ, GFP_KERN=
EL);
>
> 	kcs_bmc->miscdev.minor =3D MISC_DYNAMIC_MINOR;
>-	kcs_bmc->miscdev.name =3D devm_kasprintf(dev, GFP_KERNEL, "%s%u",
>-					       DEVICE_NAME, channel);
>+	kcs_bmc->miscdev.name =3D devm_kasprintf(kcs_bmc->dev, GFP_KERNEL, "%s%u=
",
>+					       DEVICE_NAME, kcs_bmc->channel);
> 	if (!kcs_bmc->data_in || !kcs_bmc->data_out || !kcs_bmc->kbuffer ||
> 	    !kcs_bmc->miscdev.name)
>-		return NULL;
>-	kcs_bmc->miscdev.fops =3D &kcs_bmc_fops;
>+		return -ENOMEM;
>
>-	return kcs_bmc;
>+	kcs_bmc->miscdev.fops =3D &kcs_bmc_ipmi_fops;
>+
>+	rc =3D misc_register(&kcs_bmc->miscdev);
>+	if (rc) {
>+		dev_err(kcs_bmc->dev, "Unable to register device: %d\n", rc);
>+		return rc;
>+	}
>+
>+	dev_info(kcs_bmc->dev, "Initialised IPMI client for channel %d", kcs_bmc=
->channel);
>+
>+	return 0;
>+}
>+EXPORT_SYMBOL(kcs_bmc_ipmi_attach_cdev);
>+
>+int kcs_bmc_ipmi_detach_cdev(struct kcs_bmc *kcs_bmc);

Same here.

>+int kcs_bmc_ipmi_detach_cdev(struct kcs_bmc *kcs_bmc)
>+{
>+	misc_deregister(&kcs_bmc->miscdev);
>+
>+	spin_lock_irq(&kcs_bmc->lock);
>+	kcs_bmc->running =3D 0;
>+	kcs_bmc_ipmi_force_abort(kcs_bmc);
>+	spin_unlock_irq(&kcs_bmc->lock);
>+
>+	devm_kfree(kcs_bmc->dev, kcs_bmc->kbuffer);
>+	devm_kfree(kcs_bmc->dev, kcs_bmc->data_out);
>+	devm_kfree(kcs_bmc->dev, kcs_bmc->data_in);
>+	devm_kfree(kcs_bmc->dev, kcs_bmc);
>+
>+	return 0;
> }
>-EXPORT_SYMBOL(kcs_bmc_ipmi_alloc);
>+EXPORT_SYMBOL(kcs_bmc_ipmi_detach_cdev);
>
> 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_b=
mc_npcm7xx.c
>index 1f44aadec9e8..5d017498dc69 100644
>--- a/drivers/char/ipmi/kcs_bmc_npcm7xx.c
>+++ b/drivers/char/ipmi/kcs_bmc_npcm7xx.c
>@@ -65,6 +65,8 @@ struct npcm7xx_kcs_reg {
> };
>
> struct npcm7xx_kcs_bmc {
>+	struct kcs_bmc kcs_bmc;
>+
> 	struct regmap *map;
>
> 	const struct npcm7xx_kcs_reg *reg;
>@@ -76,9 +78,14 @@ static const struct npcm7xx_kcs_reg npcm7xx_kcs_reg_tbl=
[KCS_CHANNEL_MAX] =3D {
> 	{ .sts =3D KCS3ST, .dob =3D KCS3DO, .dib =3D KCS3DI, .ctl =3D KCS3CTL, .=
ie =3D KCS3IE },
> };
>
>+static inline struct npcm7xx_kcs_bmc *to_npcm7xx_kcs_bmc(struct kcs_bmc *=
kcs_bmc)
>+{
>+	return container_of(kcs_bmc, struct npcm7xx_kcs_bmc, kcs_bmc);
>+}
>+
> static u8 npcm7xx_kcs_inb(struct kcs_bmc *kcs_bmc, u32 reg)
> {
>-	struct npcm7xx_kcs_bmc *priv =3D kcs_bmc_priv(kcs_bmc);
>+	struct npcm7xx_kcs_bmc *priv =3D to_npcm7xx_kcs_bmc(kcs_bmc);
> 	u32 val =3D 0;
> 	int rc;
>
>@@ -90,7 +97,7 @@ static u8 npcm7xx_kcs_inb(struct kcs_bmc *kcs_bmc, u32 r=
eg)
>
> static void npcm7xx_kcs_outb(struct kcs_bmc *kcs_bmc, u32 reg, u8 data)
> {
>-	struct npcm7xx_kcs_bmc *priv =3D kcs_bmc_priv(kcs_bmc);
>+	struct npcm7xx_kcs_bmc *priv =3D to_npcm7xx_kcs_bmc(kcs_bmc);
> 	int rc;
>
> 	rc =3D regmap_write(priv->map, reg, data);
>@@ -99,7 +106,7 @@ static void npcm7xx_kcs_outb(struct kcs_bmc *kcs_bmc, u=
32 reg, u8 data)
>
> static void npcm7xx_kcs_updateb(struct kcs_bmc *kcs_bmc, u32 reg, u8 mask=
, u8 data)
> {
>-	struct npcm7xx_kcs_bmc *priv =3D kcs_bmc_priv(kcs_bmc);
>+	struct npcm7xx_kcs_bmc *priv =3D to_npcm7xx_kcs_bmc(kcs_bmc);
> 	int rc;
>
> 	rc =3D regmap_update_bits(priv->map, reg, mask, data);
>@@ -108,7 +115,7 @@ static void npcm7xx_kcs_updateb(struct kcs_bmc *kcs_bm=
c, u32 reg, u8 mask, u8 da
>
> static void npcm7xx_kcs_enable_channel(struct kcs_bmc *kcs_bmc, bool enab=
le)
> {
>-	struct npcm7xx_kcs_bmc *priv =3D kcs_bmc_priv(kcs_bmc);
>+	struct npcm7xx_kcs_bmc *priv =3D to_npcm7xx_kcs_bmc(kcs_bmc);
>
> 	regmap_update_bits(priv->map, priv->reg->ctl, KCS_CTL_IBFIE,
> 			   enable ? KCS_CTL_IBFIE : 0);
>@@ -155,11 +162,10 @@ static int npcm7xx_kcs_probe(struct platform_device =
*pdev)
> 		return -ENODEV;
> 	}
>
>-	kcs_bmc =3D kcs_bmc_alloc(dev, sizeof(*priv), chan);
>-	if (!kcs_bmc)
>+	priv =3D devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
>+	if (!priv)
> 		return -ENOMEM;
>
>-	priv =3D kcs_bmc_priv(kcs_bmc);
> 	priv->map =3D syscon_node_to_regmap(dev->parent->of_node);
> 	if (IS_ERR(priv->map)) {
> 		dev_err(dev, "Couldn't get regmap\n");
>@@ -167,6 +173,9 @@ static int npcm7xx_kcs_probe(struct platform_device *p=
dev)
> 	}
> 	priv->reg =3D &npcm7xx_kcs_reg_tbl[chan - 1];
>
>+	kcs_bmc =3D &priv->kcs_bmc;
>+	kcs_bmc->dev =3D &pdev->dev;
>+	kcs_bmc->channel =3D chan;
> 	kcs_bmc->ioreg.idr =3D priv->reg->dib;
> 	kcs_bmc->ioreg.odr =3D priv->reg->dob;
> 	kcs_bmc->ioreg.str =3D priv->reg->sts;
>@@ -174,31 +183,27 @@ static int npcm7xx_kcs_probe(struct platform_device =
*pdev)
> 	kcs_bmc->io_outputb =3D npcm7xx_kcs_outb;
> 	kcs_bmc->io_updateb =3D npcm7xx_kcs_updateb;
>
>-	dev_set_drvdata(dev, kcs_bmc);
>+	platform_set_drvdata(pdev, priv);
>
> 	npcm7xx_kcs_enable_channel(kcs_bmc, true);
> 	rc =3D npcm7xx_kcs_config_irq(kcs_bmc, pdev);
> 	if (rc)
> 		return rc;
>
>-	rc =3D misc_register(&kcs_bmc->miscdev);
>-	if (rc) {
>-		dev_err(dev, "Unable to register device\n");
>-		return rc;
>-	}
>
> 	pr_info("channel=3D%u idr=3D0x%x odr=3D0x%x str=3D0x%x\n",
> 		chan,
> 		kcs_bmc->ioreg.idr, kcs_bmc->ioreg.odr, kcs_bmc->ioreg.str);
>
>-	return 0;
>+	return kcs_bmc_add_device(kcs_bmc);
> }
>
> static int npcm7xx_kcs_remove(struct platform_device *pdev)
> {
>-	struct kcs_bmc *kcs_bmc =3D dev_get_drvdata(&pdev->dev);
>+	struct npcm7xx_kcs_bmc *priv =3D platform_get_drvdata(pdev);
>+	struct kcs_bmc *kcs_bmc =3D &priv->kcs_bmc;
>
>-	misc_deregister(&kcs_bmc->miscdev);
>+	kcs_bmc_remove_device(kcs_bmc);

As with the corresponding aspeed code, should we propagate the return
value here?

>
> 	return 0;
> }
>--=20
>2.27.0
>=