[PATCH v1 01/10] ipmi_si: Switch to use platform_get_mem_or_io()

[PATCH v1 01/10] ipmi_si: Switch to use platform_get_mem_or_io()

[Andy Shevchenko]

Switch to use new platform_get_mem_or_io() instead of home grown analogue.
Note, we also introduce ipmi_set_addr_data_and_space() helper here.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/char/ipmi/ipmi_si_platform.c | 40 +++++++++++-----------------
 1 file changed, 16 insertions(+), 24 deletions(-)

diff --git a/drivers/char/ipmi/ipmi_si_platform.c b/drivers/char/ipmi/ipmi_si_platform.c
index 129b5713f187..d7bd093f80e9 100644
--- a/drivers/char/ipmi/ipmi_si_platform.c
+++ b/drivers/char/ipmi/ipmi_si_platform.c
@@ -100,35 +100,32 @@ static int acpi_gpe_irq_setup(struct si_sm_io *io)
 }
 #endif
 
+static void ipmi_set_addr_data_and_space(struct resource *r, struct si_sm_io *io)
+{
+	io->addr_data = r->start;
+	if (resource_type(r) == IORESOURCE_IO)
+		io->addr_space = IPMI_IO_ADDR_SPACE;
+	else
+		io->addr_space = IPMI_MEM_ADDR_SPACE;
+}
+
 static struct resource *
 ipmi_get_info_from_resources(struct platform_device *pdev,
 			     struct si_sm_io *io)
 {
-	struct resource *res, *res_second;
+	struct resource *res, *second;
 
-	res = platform_get_resource(pdev, IORESOURCE_IO, 0);
-	if (res) {
-		io->addr_space = IPMI_IO_ADDR_SPACE;
-	} else {
-		res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-		if (res)
-			io->addr_space = IPMI_MEM_ADDR_SPACE;
-	}
+	res = platform_get_mem_or_io(pdev, 0);
 	if (!res) {
 		dev_err(&pdev->dev, "no I/O or memory address\n");
 		return NULL;
 	}
-	io->addr_data = res->start;
+	ipmi_set_addr_data_and_space(res, io);
 
 	io->regspacing = DEFAULT_REGSPACING;
-	res_second = platform_get_resource(pdev,
-			       (io->addr_space == IPMI_IO_ADDR_SPACE) ?
-					IORESOURCE_IO : IORESOURCE_MEM,
-			       1);
-	if (res_second) {
-		if (res_second->start > io->addr_data)
-			io->regspacing = res_second->start - io->addr_data;
-	}
+	second = platform_get_mem_or_io(pdev, 1);
+	if (second && resource_type(second) == resource_type(res) && second->start > io->addr_data)
+		io->regspacing = second->start - io->addr_data;
 
 	return res;
 }
@@ -275,12 +272,7 @@ static int of_ipmi_probe(struct platform_device *pdev)
 	io.addr_source	= SI_DEVICETREE;
 	io.irq_setup	= ipmi_std_irq_setup;
 
-	if (resource.flags & IORESOURCE_IO)
-		io.addr_space = IPMI_IO_ADDR_SPACE;
-	else
-		io.addr_space = IPMI_MEM_ADDR_SPACE;
-
-	io.addr_data	= resource.start;
+	ipmi_set_addr_data_and_space(&resource, &io);
 
 	io.regsize	= regsize ? be32_to_cpup(regsize) : DEFAULT_REGSIZE;
 	io.regspacing	= regspacing ? be32_to_cpup(regspacing) : DEFAULT_REGSPACING;
-- 
2.30.2

[PATCH v1 02/10] ipmi_si: Remove bogus err_free label

[Andy Shevchenko]

There is no more 'free' in the error path, so drop the label and
return errors inline.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/char/ipmi/ipmi_si_platform.c | 17 +++++------------
 1 file changed, 5 insertions(+), 12 deletions(-)

diff --git a/drivers/char/ipmi/ipmi_si_platform.c b/drivers/char/ipmi/ipmi_si_platform.c
index d7bd093f80e9..009563073d30 100644
--- a/drivers/char/ipmi/ipmi_si_platform.c
+++ b/drivers/char/ipmi/ipmi_si_platform.c
@@ -314,7 +314,6 @@ static int acpi_ipmi_probe(struct platform_device *pdev)
 	acpi_status status;
 	unsigned long long tmp;
 	struct resource *res;
-	int rv = -EINVAL;
 
 	if (!si_tryacpi)
 		return -ENODEV;
@@ -334,7 +333,7 @@ static int acpi_ipmi_probe(struct platform_device *pdev)
 	if (ACPI_FAILURE(status)) {
 		dev_err(&pdev->dev,
 			"Could not find ACPI IPMI interface type\n");
-		goto err_free;
+		return -EINVAL;
 	}
 
 	switch (tmp) {
@@ -348,21 +347,18 @@ static int acpi_ipmi_probe(struct platform_device *pdev)
 		io.si_type = SI_BT;
 		break;
 	case 4: /* SSIF, just ignore */
-		rv = -ENODEV;
-		goto err_free;
+		return -ENODEV;
 	default:
 		dev_info(&pdev->dev, "unknown IPMI type %lld\n", tmp);
-		goto err_free;
+		return -EINVAL;
 	}
 
 	io.regsize = DEFAULT_REGSIZE;
 	io.regshift = 0;
 
 	res = ipmi_get_info_from_resources(pdev, &io);
-	if (!res) {
-		rv = -EINVAL;
-		goto err_free;
-	}
+	if (!res)
+		return -EINVAL;
 
 	/* If _GPE exists, use it; otherwise use standard interrupts */
 	status = acpi_evaluate_integer(handle, "_GPE", NULL, &tmp);
@@ -388,9 +384,6 @@ static int acpi_ipmi_probe(struct platform_device *pdev)
 	request_module("acpi_ipmi");
 
 	return ipmi_si_add_smi(&io);
-
-err_free:
-	return rv;
 }
 
 static const struct acpi_device_id acpi_ipmi_match[] = {
-- 
2.30.2

[PATCH v1 03/10] ipmi_si: Utilize temporary variable to hold device pointer

[Andy Shevchenko]

By one of the previous clean up change we got a temporary variable to hold
a device pointer. It can be utilized in other calls in the ->probe() and
save a bit of LOCs.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/char/ipmi/ipmi_si_platform.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/drivers/char/ipmi/ipmi_si_platform.c b/drivers/char/ipmi/ipmi_si_platform.c
index 009563073d30..954c297b459b 100644
--- a/drivers/char/ipmi/ipmi_si_platform.c
+++ b/drivers/char/ipmi/ipmi_si_platform.c
@@ -309,6 +309,7 @@ static int find_slave_address(struct si_sm_io *io, int slave_addr)
 
 static int acpi_ipmi_probe(struct platform_device *pdev)
 {
+	struct device *dev = &pdev->dev;
 	struct si_sm_io io;
 	acpi_handle handle;
 	acpi_status status;
@@ -318,21 +319,20 @@ static int acpi_ipmi_probe(struct platform_device *pdev)
 	if (!si_tryacpi)
 		return -ENODEV;
 
-	handle = ACPI_HANDLE(&pdev->dev);
+	handle = ACPI_HANDLE(dev);
 	if (!handle)
 		return -ENODEV;
 
 	memset(&io, 0, sizeof(io));
 	io.addr_source = SI_ACPI;
-	dev_info(&pdev->dev, "probing via ACPI\n");
+	dev_info(dev, "probing via ACPI\n");
 
 	io.addr_info.acpi_info.acpi_handle = handle;
 
 	/* _IFT tells us the interface type: KCS, BT, etc */
 	status = acpi_evaluate_integer(handle, "_IFT", NULL, &tmp);
 	if (ACPI_FAILURE(status)) {
-		dev_err(&pdev->dev,
-			"Could not find ACPI IPMI interface type\n");
+		dev_err(dev, "Could not find ACPI IPMI interface type\n");
 		return -EINVAL;
 	}
 
@@ -349,10 +349,11 @@ static int acpi_ipmi_probe(struct platform_device *pdev)
 	case 4: /* SSIF, just ignore */
 		return -ENODEV;
 	default:
-		dev_info(&pdev->dev, "unknown IPMI type %lld\n", tmp);
+		dev_info(dev, "unknown IPMI type %lld\n", tmp);
 		return -EINVAL;
 	}
 
+	io.dev = dev;
 	io.regsize = DEFAULT_REGSIZE;
 	io.regshift = 0;
 
@@ -376,9 +377,7 @@ static int acpi_ipmi_probe(struct platform_device *pdev)
 
 	io.slave_addr = find_slave_address(&io, io.slave_addr);
 
-	io.dev = &pdev->dev;
-
-	dev_info(io.dev, "%pR regsize %d spacing %d irq %d\n",
+	dev_info(dev, "%pR regsize %d spacing %d irq %d\n",
 		 res, io.regsize, io.regspacing, io.irq);
 
 	request_module("acpi_ipmi");
-- 
2.30.2

[PATCH v1 04/10] ipmi_si: Use proper ACPI macros to check error code for failures

[Andy Shevchenko]

Instead of direct comparison, use proper ACPI macros to check error code
for failures.

While at it, drop unneeded 'else' keyword.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/char/ipmi/ipmi_si_platform.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/char/ipmi/ipmi_si_platform.c b/drivers/char/ipmi/ipmi_si_platform.c
index 954c297b459b..023c88ea9c4c 100644
--- a/drivers/char/ipmi/ipmi_si_platform.c
+++ b/drivers/char/ipmi/ipmi_si_platform.c
@@ -85,18 +85,18 @@ static int acpi_gpe_irq_setup(struct si_sm_io *io)
 					  ACPI_GPE_LEVEL_TRIGGERED,
 					  &ipmi_acpi_gpe,
 					  io);
-	if (status != AE_OK) {
+	if (ACPI_FAILURE(status)) {
 		dev_warn(io->dev,
 			 "Unable to claim ACPI GPE %d, running polled\n",
 			 io->irq);
 		io->irq = 0;
 		return -EINVAL;
-	} else {
-		io->irq_cleanup = acpi_gpe_irq_cleanup;
-		ipmi_irq_finish_setup(io);
-		dev_info(io->dev, "Using ACPI GPE %d\n", io->irq);
-		return 0;
 	}
+
+	io->irq_cleanup = acpi_gpe_irq_cleanup;
+	ipmi_irq_finish_setup(io);
+	dev_info(io->dev, "Using ACPI GPE %d\n", io->irq);
+	return 0;
 }
 #endif
 
-- 
2.30.2

[PATCH v1 05/10] ipmi_si: Introduce panic_event_str array

[Andy Shevchenko]

Instead of twice repeat the constant literals, introduce
panic_event_str array. It allows to simplify the code with
help of match_string() API.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/char/ipmi/ipmi_msghandler.c | 49 ++++++++++-------------------
 1 file changed, 17 insertions(+), 32 deletions(-)

diff --git a/drivers/char/ipmi/ipmi_msghandler.c b/drivers/char/ipmi/ipmi_msghandler.c
index f19f0f967e28..c7d37366d7bb 100644
--- a/drivers/char/ipmi/ipmi_msghandler.c
+++ b/drivers/char/ipmi/ipmi_msghandler.c
@@ -52,8 +52,12 @@ static bool drvregistered;
 enum ipmi_panic_event_op {
 	IPMI_SEND_PANIC_EVENT_NONE,
 	IPMI_SEND_PANIC_EVENT,
-	IPMI_SEND_PANIC_EVENT_STRING
+	IPMI_SEND_PANIC_EVENT_STRING,
+	IPMI_SEND_PANIC_EVENT_MAX
 };
+
+static const char *const panic_event_str[] = { "none", "event", "string", NULL };
+
 #ifdef CONFIG_IPMI_PANIC_STRING
 #define IPMI_PANIC_DEFAULT IPMI_SEND_PANIC_EVENT_STRING
 #elif defined(CONFIG_IPMI_PANIC_EVENT)
@@ -68,46 +72,27 @@ static int panic_op_write_handler(const char *val,
 				  const struct kernel_param *kp)
 {
 	char valcp[16];
-	char *s;
-
-	strncpy(valcp, val, 15);
-	valcp[15] = '\0';
+	int e;
 
-	s = strstrip(valcp);
-
-	if (strcmp(s, "none") == 0)
-		ipmi_send_panic_event = IPMI_SEND_PANIC_EVENT_NONE;
-	else if (strcmp(s, "event") == 0)
-		ipmi_send_panic_event = IPMI_SEND_PANIC_EVENT;
-	else if (strcmp(s, "string") == 0)
-		ipmi_send_panic_event = IPMI_SEND_PANIC_EVENT_STRING;
-	else
-		return -EINVAL;
+	strscpy(valcp, val, sizeof(valcp));
+	e = match_string(panic_event_str, -1, strstrip(valcp));
+	if (e < 0)
+		return e;
 
+	ipmi_send_panic_event = e;
 	return 0;
 }
 
 static int panic_op_read_handler(char *buffer, const struct kernel_param *kp)
 {
-	switch (ipmi_send_panic_event) {
-	case IPMI_SEND_PANIC_EVENT_NONE:
-		strcpy(buffer, "none\n");
-		break;
-
-	case IPMI_SEND_PANIC_EVENT:
-		strcpy(buffer, "event\n");
-		break;
-
-	case IPMI_SEND_PANIC_EVENT_STRING:
-		strcpy(buffer, "string\n");
-		break;
+	const char *event_str;
 
-	default:
-		strcpy(buffer, "???\n");
-		break;
-	}
+	if (ipmi_send_panic_event >= IPMI_SEND_PANIC_EVENT_MAX)
+		event_str = "???";
+	else
+		event_str = panic_event_str[ipmi_send_panic_event];
 
-	return strlen(buffer);
+	return sprintf(buffer, "%s\n", event_str);
 }
 
 static const struct kernel_param_ops panic_op_ops = {
-- 
2.30.2

[PATCH v1 06/10] ipmi_si: Reuse si_to_str array in ipmi_hardcode_init_one()

[Andy Shevchenko]

Instead of making the comparison one by one, reuse si_to_str array
in ipmi_hardcode_init_one() in conjunction with match_string() API.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/char/ipmi/ipmi_si.h          |  3 +++
 drivers/char/ipmi/ipmi_si_hardcode.c | 23 +++++++++--------------
 drivers/char/ipmi/ipmi_si_intf.c     |  2 --
 3 files changed, 12 insertions(+), 16 deletions(-)

diff --git a/drivers/char/ipmi/ipmi_si.h b/drivers/char/ipmi/ipmi_si.h
index bac0ff86e48e..fd3167d1e1e9 100644
--- a/drivers/char/ipmi/ipmi_si.h
+++ b/drivers/char/ipmi/ipmi_si.h
@@ -22,6 +22,9 @@ enum si_type {
 	SI_TYPE_INVALID, SI_KCS, SI_SMIC, SI_BT
 };
 
+/* 'invalid' to allow a firmware-specified interface to be disabled */
+static __maybe_unused const char *const si_to_str[] = { "invalid", "kcs", "smic", "bt" };
+
 enum ipmi_addr_space {
 	IPMI_IO_ADDR_SPACE, IPMI_MEM_ADDR_SPACE
 };
diff --git a/drivers/char/ipmi/ipmi_si_hardcode.c b/drivers/char/ipmi/ipmi_si_hardcode.c
index f6ece7569504..cf3797523469 100644
--- a/drivers/char/ipmi/ipmi_si_hardcode.c
+++ b/drivers/char/ipmi/ipmi_si_hardcode.c
@@ -80,26 +80,21 @@ static void __init ipmi_hardcode_init_one(const char *si_type_str,
 					  enum ipmi_addr_space addr_space)
 {
 	struct ipmi_plat_data p;
+	int t;
 
 	memset(&p, 0, sizeof(p));
 
 	p.iftype = IPMI_PLAT_IF_SI;
-	if (!si_type_str || !*si_type_str || strcmp(si_type_str, "kcs") == 0) {
+	if (!si_type_str || !*si_type_str) {
 		p.type = SI_KCS;
-	} else if (strcmp(si_type_str, "smic") == 0) {
-		p.type = SI_SMIC;
-	} else if (strcmp(si_type_str, "bt") == 0) {
-		p.type = SI_BT;
-	} else if (strcmp(si_type_str, "invalid") == 0) {
-		/*
-		 * Allow a firmware-specified interface to be
-		 * disabled.
-		 */
-		p.type = SI_TYPE_INVALID;
 	} else {
-		pr_warn("Interface type specified for interface %d, was invalid: %s\n",
-			i, si_type_str);
-		return;
+		t = match_string(si_to_str, ARRAY_SIZE(si_to_str), si_type_str);
+		if (t < 0) {
+			pr_warn("Interface type specified for interface %d, was invalid: %s\n",
+				i, si_type_str);
+			return;
+		}
+		p.type = t;
 	}
 
 	p.regsize = regsizes[i];
diff --git a/drivers/char/ipmi/ipmi_si_intf.c b/drivers/char/ipmi/ipmi_si_intf.c
index be41a473e3c2..ff448098f185 100644
--- a/drivers/char/ipmi/ipmi_si_intf.c
+++ b/drivers/char/ipmi/ipmi_si_intf.c
@@ -70,8 +70,6 @@ enum si_intf_state {
 #define IPMI_BT_INTMASK_CLEAR_IRQ_BIT	2
 #define IPMI_BT_INTMASK_ENABLE_IRQ_BIT	1
 
-static const char * const si_to_str[] = { "invalid", "kcs", "smic", "bt" };
-
 static bool initialized;
 
 /*
-- 
2.30.2

[PATCH v1 07/10] ipmi_si: Get rid of ->addr_source_cleanup()

[Andy Shevchenko]

The ->addr_source_cleanup() callback is solely used by PCI driver
and only for one purpose, i.e. to disable device. Get rid of
->addr_source_cleanup() by switching to PCI managed API.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/char/ipmi/ipmi_si.h      |  2 --
 drivers/char/ipmi/ipmi_si_intf.c |  4 ----
 drivers/char/ipmi/ipmi_si_pci.c  | 18 ++----------------
 3 files changed, 2 insertions(+), 22 deletions(-)

diff --git a/drivers/char/ipmi/ipmi_si.h b/drivers/char/ipmi/ipmi_si.h
index fd3167d1e1e9..53463f7762e0 100644
--- a/drivers/char/ipmi/ipmi_si.h
+++ b/drivers/char/ipmi/ipmi_si.h
@@ -51,8 +51,6 @@ struct si_sm_io {
 	enum ipmi_addr_space addr_space;
 	unsigned long addr_data;
 	enum ipmi_addr_src addr_source; /* ACPI, PCI, SMBIOS, hardcode, etc. */
-	void (*addr_source_cleanup)(struct si_sm_io *io);
-	void *addr_source_data;
 	union ipmi_smi_info_union addr_info;
 
 	int (*io_setup)(struct si_sm_io *info);
diff --git a/drivers/char/ipmi/ipmi_si_intf.c b/drivers/char/ipmi/ipmi_si_intf.c
index ff448098f185..1f568cc88b39 100644
--- a/drivers/char/ipmi/ipmi_si_intf.c
+++ b/drivers/char/ipmi/ipmi_si_intf.c
@@ -2203,10 +2203,6 @@ static void shutdown_smi(void *send_info)
 	if (smi_info->handlers)
 		smi_info->handlers->cleanup(smi_info->si_sm);
 
-	if (smi_info->io.addr_source_cleanup) {
-		smi_info->io.addr_source_cleanup(&smi_info->io);
-		smi_info->io.addr_source_cleanup = NULL;
-	}
 	if (smi_info->io.io_cleanup) {
 		smi_info->io.io_cleanup(&smi_info->io);
 		smi_info->io.io_cleanup = NULL;
diff --git a/drivers/char/ipmi/ipmi_si_pci.c b/drivers/char/ipmi/ipmi_si_pci.c
index 95bbcfba5408..0bc7efb6902c 100644
--- a/drivers/char/ipmi/ipmi_si_pci.c
+++ b/drivers/char/ipmi/ipmi_si_pci.c
@@ -21,13 +21,6 @@ MODULE_PARM_DESC(trypci, "Setting this to zero will disable the"
 
 #define PCI_DEVICE_ID_HP_MMC 0x121A
 
-static void ipmi_pci_cleanup(struct si_sm_io *io)
-{
-	struct pci_dev *pdev = io->addr_source_data;
-
-	pci_disable_device(pdev);
-}
-
 static int ipmi_pci_probe_regspacing(struct si_sm_io *io)
 {
 	if (io->si_type == SI_KCS) {
@@ -97,15 +90,12 @@ static int ipmi_pci_probe(struct pci_dev *pdev,
 		return -ENOMEM;
 	}
 
-	rv = pci_enable_device(pdev);
+	rv = pcim_enable_device(pdev);
 	if (rv) {
 		dev_err(&pdev->dev, "couldn't enable PCI device\n");
 		return rv;
 	}
 
-	io.addr_source_cleanup = ipmi_pci_cleanup;
-	io.addr_source_data = pdev;
-
 	if (pci_resource_flags(pdev, 0) & IORESOURCE_IO) {
 		io.addr_space = IPMI_IO_ADDR_SPACE;
 		io.io_setup = ipmi_si_port_setup;
@@ -128,11 +118,7 @@ static int ipmi_pci_probe(struct pci_dev *pdev,
 	dev_info(&pdev->dev, "%pR regsize %d spacing %d irq %d\n",
 		 &pdev->resource[0], io.regsize, io.regspacing, io.irq);
 
-	rv = ipmi_si_add_smi(&io);
-	if (rv)
-		pci_disable_device(pdev);
-
-	return rv;
+	return ipmi_si_add_smi(&io);
 }
 
 static void ipmi_pci_remove(struct pci_dev *pdev)
-- 
2.30.2

[PATCH v1 08/10] ipmi_si: Use strstrip() to remove surrounding spaces

[Andy Shevchenko]

Instead of home grown analogue, use strstrip() from the kernel library.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/char/ipmi/ipmi_si_hotmod.c | 16 ++++------------
 1 file changed, 4 insertions(+), 12 deletions(-)

diff --git a/drivers/char/ipmi/ipmi_si_hotmod.c b/drivers/char/ipmi/ipmi_si_hotmod.c
index 4fbb4e18bae2..087f5eb1ebc0 100644
--- a/drivers/char/ipmi/ipmi_si_hotmod.c
+++ b/drivers/char/ipmi/ipmi_si_hotmod.c
@@ -185,24 +185,16 @@ static atomic_t hotmod_nr;
 
 static int hotmod_handler(const char *val, const struct kernel_param *kp)
 {
-	char *str = kstrdup(val, GFP_KERNEL), *curr, *next;
 	int  rv;
 	struct ipmi_plat_data h;
-	unsigned int len;
-	int ival;
+	char *str, *curr, *next;
 
+	str = kstrdup(val, GFP_KERNEL);
 	if (!str)
 		return -ENOMEM;
 
 	/* Kill any trailing spaces, as we can get a "\n" from echo. */
-	len = strlen(str);
-	ival = len - 1;
-	while ((ival >= 0) && isspace(str[ival])) {
-		str[ival] = '\0';
-		ival--;
-	}
-
-	for (curr = str; curr; curr = next) {
+	for (curr = strstrip(str); curr; curr = next) {
 		enum hotmod_op op;
 
 		next = strchr(curr, ':');
@@ -235,7 +227,7 @@ static int hotmod_handler(const char *val, const struct kernel_param *kp)
 				put_device(dev);
 		}
 	}
-	rv = len;
+	rv = strlen(val);
 out:
 	kfree(str);
 	return rv;
-- 
2.30.2

[PATCH v1 09/10] ipmi_si: Drop redundant check before calling put_device()

[Andy Shevchenko]

put_device() is NULL aware, drop redundant check before calling it.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/char/ipmi/ipmi_si_hotmod.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/char/ipmi/ipmi_si_hotmod.c b/drivers/char/ipmi/ipmi_si_hotmod.c
index 087f5eb1ebc0..a07ef37c0e3f 100644
--- a/drivers/char/ipmi/ipmi_si_hotmod.c
+++ b/drivers/char/ipmi/ipmi_si_hotmod.c
@@ -223,8 +223,7 @@ static int hotmod_handler(const char *val, const struct kernel_param *kp)
 				if (strcmp(pdev->name, "hotmod-ipmi-si") == 0)
 					platform_device_unregister(pdev);
 			}
-			if (dev)
-				put_device(dev);
+			put_device(dev);
 		}
 	}
 	rv = strlen(val);
-- 
2.30.2

[PATCH v1 10/10] ipmi_si: Join string literals back

[Andy Shevchenko]

For easy grepping on debug purposes join string literals back in
the messages.

No functional change.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/char/ipmi/ipmi_msghandler.c  |  3 +-
 drivers/char/ipmi/ipmi_si_hardcode.c | 50 +++++++++-------------------
 drivers/char/ipmi/ipmi_si_hotmod.c   |  5 ++-
 drivers/char/ipmi/ipmi_si_intf.c     | 25 +++++---------
 drivers/char/ipmi/ipmi_si_pci.c      |  4 +--
 drivers/char/ipmi/ipmi_si_platform.c | 17 +++++-----
 6 files changed, 38 insertions(+), 66 deletions(-)

diff --git a/drivers/char/ipmi/ipmi_msghandler.c b/drivers/char/ipmi/ipmi_msghandler.c
index c7d37366d7bb..aaca8873a486 100644
--- a/drivers/char/ipmi/ipmi_msghandler.c
+++ b/drivers/char/ipmi/ipmi_msghandler.c
@@ -5207,7 +5207,6 @@ module_exit(cleanup_ipmi);
 module_init(ipmi_init_msghandler_mod);
 MODULE_LICENSE("GPL");
 MODULE_AUTHOR("Corey Minyard <minyard@mvista.com>");
-MODULE_DESCRIPTION("Incoming and outgoing message routing for an IPMI"
-		   " interface.");
+MODULE_DESCRIPTION("Incoming and outgoing message routing for an IPMI interface.");
 MODULE_VERSION(IPMI_DRIVER_VERSION);
 MODULE_SOFTDEP("post: ipmi_devintf");
diff --git a/drivers/char/ipmi/ipmi_si_hardcode.c b/drivers/char/ipmi/ipmi_si_hardcode.c
index cf3797523469..47eee958edcc 100644
--- a/drivers/char/ipmi/ipmi_si_hardcode.c
+++ b/drivers/char/ipmi/ipmi_si_hardcode.c
@@ -32,47 +32,29 @@ static int slave_addrs[SI_MAX_PARMS] __initdata;
 static unsigned int num_slave_addrs __initdata;
 
 module_param_string(type, si_type_str, MAX_SI_TYPE_STR, 0);
-MODULE_PARM_DESC(type, "Defines the type of each interface, each"
-		 " interface separated by commas.  The types are 'kcs',"
-		 " 'smic', and 'bt'.  For example si_type=kcs,bt will set"
-		 " the first interface to kcs and the second to bt");
+MODULE_PARM_DESC(type,
+		 "Defines the type of each interface, each interface separated by commas.  The types are 'kcs', 'smic', and 'bt'.  For example si_type=kcs,bt will set the first interface to kcs and the second to bt");
 module_param_hw_array(addrs, ulong, iomem, &num_addrs, 0);
-MODULE_PARM_DESC(addrs, "Sets the memory address of each interface, the"
-		 " addresses separated by commas.  Only use if an interface"
-		 " is in memory.  Otherwise, set it to zero or leave"
-		 " it blank.");
+MODULE_PARM_DESC(addrs,
+		 "Sets the memory address of each interface, the addresses separated by commas.  Only use if an interface is in memory.  Otherwise, set it to zero or leave it blank.");
 module_param_hw_array(ports, uint, ioport, &num_ports, 0);
-MODULE_PARM_DESC(ports, "Sets the port address of each interface, the"
-		 " addresses separated by commas.  Only use if an interface"
-		 " is a port.  Otherwise, set it to zero or leave"
-		 " it blank.");
+MODULE_PARM_DESC(ports,
+		 "Sets the port address of each interface, the addresses separated by commas.  Only use if an interface is a port.  Otherwise, set it to zero or leave it blank.");
 module_param_hw_array(irqs, int, irq, &num_irqs, 0);
-MODULE_PARM_DESC(irqs, "Sets the interrupt of each interface, the"
-		 " addresses separated by commas.  Only use if an interface"
-		 " has an interrupt.  Otherwise, set it to zero or leave"
-		 " it blank.");
+MODULE_PARM_DESC(irqs,
+		 "Sets the interrupt of each interface, the addresses separated by commas.  Only use if an interface has an interrupt.  Otherwise, set it to zero or leave it blank.");
 module_param_hw_array(regspacings, int, other, &num_regspacings, 0);
-MODULE_PARM_DESC(regspacings, "The number of bytes between the start address"
-		 " and each successive register used by the interface.  For"
-		 " instance, if the start address is 0xca2 and the spacing"
-		 " is 2, then the second address is at 0xca4.  Defaults"
-		 " to 1.");
+MODULE_PARM_DESC(regspacings,
+		 "The number of bytes between the start address and each successive register used by the interface.  For instance, if the start address is 0xca2 and the spacing is 2, then the second address is at 0xca4.  Defaults to 1.");
 module_param_hw_array(regsizes, int, other, &num_regsizes, 0);
-MODULE_PARM_DESC(regsizes, "The size of the specific IPMI register in bytes."
-		 " This should generally be 1, 2, 4, or 8 for an 8-bit,"
-		 " 16-bit, 32-bit, or 64-bit register.  Use this if you"
-		 " the 8-bit IPMI register has to be read from a larger"
-		 " register.");
+MODULE_PARM_DESC(regsizes,
+		 "The size of the specific IPMI register in bytes. This should generally be 1, 2, 4, or 8 for an 8-bit, 16-bit, 32-bit, or 64-bit register.  Use this if you the 8-bit IPMI register has to be read from a larger register.");
 module_param_hw_array(regshifts, int, other, &num_regshifts, 0);
-MODULE_PARM_DESC(regshifts, "The amount to shift the data read from the."
-		 " IPMI register, in bits.  For instance, if the data"
-		 " is read from a 32-bit word and the IPMI data is in"
-		 " bit 8-15, then the shift would be 8");
+MODULE_PARM_DESC(regshifts,
+		 "The amount to shift the data read from the. IPMI register, in bits.  For instance, if the data is read from a 32-bit word and the IPMI data is in bit 8-15, then the shift would be 8");
 module_param_hw_array(slave_addrs, int, other, &num_slave_addrs, 0);
-MODULE_PARM_DESC(slave_addrs, "Set the default IPMB slave address for"
-		 " the controller.  Normally this is 0x20, but can be"
-		 " overridden by this parm.  This is an array indexed"
-		 " by interface number.");
+MODULE_PARM_DESC(slave_addrs,
+		 "Set the default IPMB slave address for the controller.  Normally this is 0x20, but can be overridden by this parm.  This is an array indexed by interface number.");
 
 static void __init ipmi_hardcode_init_one(const char *si_type_str,
 					  unsigned int i,
diff --git a/drivers/char/ipmi/ipmi_si_hotmod.c b/drivers/char/ipmi/ipmi_si_hotmod.c
index a07ef37c0e3f..6b12a83ccd4c 100644
--- a/drivers/char/ipmi/ipmi_si_hotmod.c
+++ b/drivers/char/ipmi/ipmi_si_hotmod.c
@@ -17,9 +17,8 @@
 static int hotmod_handler(const char *val, const struct kernel_param *kp);
 
 module_param_call(hotmod, hotmod_handler, NULL, NULL, 0200);
-MODULE_PARM_DESC(hotmod, "Add and remove interfaces.  See"
-		 " Documentation/driver-api/ipmi.rst in the kernel sources for the"
-		 " gory details.");
+MODULE_PARM_DESC(hotmod,
+		 "Add and remove interfaces.  See Documentation/driver-api/ipmi.rst in the kernel sources for the gory details.");
 
 /*
  * Parms come in as <op1>[:op2[:op3...]].  ops are:
diff --git a/drivers/char/ipmi/ipmi_si_intf.c b/drivers/char/ipmi/ipmi_si_intf.c
index 1f568cc88b39..3fb991949f99 100644
--- a/drivers/char/ipmi/ipmi_si_intf.c
+++ b/drivers/char/ipmi/ipmi_si_intf.c
@@ -1167,9 +1167,8 @@ static int smi_start_processing(void            *send_info,
 		new_smi->thread = kthread_run(ipmi_thread, new_smi,
 					      "kipmi%d", new_smi->si_num);
 		if (IS_ERR(new_smi->thread)) {
-			dev_notice(new_smi->io.dev, "Could not start"
-				   " kernel thread due to error %ld, only using"
-				   " timers to drive the interface\n",
+			dev_notice(new_smi->io.dev,
+				   "Could not start kernel thread due to error %ld, only using timers to drive the interface\n",
 				   PTR_ERR(new_smi->thread));
 			new_smi->thread = NULL;
 		}
@@ -1221,18 +1220,14 @@ static int smi_num; /* Used to sequence the SMIs */
 static const char * const addr_space_to_str[] = { "i/o", "mem" };
 
 module_param_array(force_kipmid, int, &num_force_kipmid, 0);
-MODULE_PARM_DESC(force_kipmid, "Force the kipmi daemon to be enabled (1) or"
-		 " disabled(0).  Normally the IPMI driver auto-detects"
-		 " this, but the value may be overridden by this parm.");
+MODULE_PARM_DESC(force_kipmid,
+		 "Force the kipmi daemon to be enabled (1) or disabled(0).  Normally the IPMI driver auto-detects this, but the value may be overridden by this parm.");
 module_param(unload_when_empty, bool, 0);
-MODULE_PARM_DESC(unload_when_empty, "Unload the module if no interfaces are"
-		 " specified or found, default is 1.  Setting to 0"
-		 " is useful for hot add of devices using hotmod.");
+MODULE_PARM_DESC(unload_when_empty,
+		 "Unload the module if no interfaces are specified or found, default is 1.  Setting to 0 is useful for hot add of devices using hotmod.");
 module_param_array(kipmid_max_busy_us, uint, &num_max_busy_us, 0644);
 MODULE_PARM_DESC(kipmid_max_busy_us,
-		 "Max time (in microseconds) to busy-wait for IPMI data before"
-		 " sleeping. 0 (default) means to wait forever. Set to 100-500"
-		 " if kipmid is using up a lot of CPU time.");
+		 "Max time (in microseconds) to busy-wait for IPMI data before sleeping. 0 (default) means to wait forever. Set to 100-500 if kipmid is using up a lot of CPU time.");
 
 void ipmi_irq_finish_setup(struct si_sm_io *io)
 {
@@ -1268,8 +1263,7 @@ int ipmi_std_irq_setup(struct si_sm_io *io)
 			 SI_DEVICE_NAME,
 			 io->irq_handler_data);
 	if (rv) {
-		dev_warn(io->dev, "%s unable to claim interrupt %d,"
-			 " running polled\n",
+		dev_warn(io->dev, "%s unable to claim interrupt %d, running polled\n",
 			 SI_DEVICE_NAME, io->irq);
 		io->irq = 0;
 	} else {
@@ -2298,5 +2292,4 @@ module_exit(cleanup_ipmi_si);
 MODULE_ALIAS("platform:dmi-ipmi-si");
 MODULE_LICENSE("GPL");
 MODULE_AUTHOR("Corey Minyard <minyard@mvista.com>");
-MODULE_DESCRIPTION("Interface to the IPMI driver for the KCS, SMIC, and BT"
-		   " system interfaces.");
+MODULE_DESCRIPTION("Interface to the IPMI driver for the KCS, SMIC, and BT system interfaces.");
diff --git a/drivers/char/ipmi/ipmi_si_pci.c b/drivers/char/ipmi/ipmi_si_pci.c
index 0bc7efb6902c..74fa2055868b 100644
--- a/drivers/char/ipmi/ipmi_si_pci.c
+++ b/drivers/char/ipmi/ipmi_si_pci.c
@@ -16,8 +16,8 @@ static bool pci_registered;
 static bool si_trypci = true;
 
 module_param_named(trypci, si_trypci, bool, 0);
-MODULE_PARM_DESC(trypci, "Setting this to zero will disable the"
-		 " default scan of the interfaces identified via pci");
+MODULE_PARM_DESC(trypci,
+		 "Setting this to zero will disable the default scan of the interfaces identified via pci");
 
 #define PCI_DEVICE_ID_HP_MMC 0x121A
 
diff --git a/drivers/char/ipmi/ipmi_si_platform.c b/drivers/char/ipmi/ipmi_si_platform.c
index 023c88ea9c4c..32fa86b9b876 100644
--- a/drivers/char/ipmi/ipmi_si_platform.c
+++ b/drivers/char/ipmi/ipmi_si_platform.c
@@ -34,23 +34,22 @@ static bool          si_trydmi = false;
 #endif
 
 module_param_named(tryplatform, si_tryplatform, bool, 0);
-MODULE_PARM_DESC(tryplatform, "Setting this to zero will disable the"
-		 " default scan of the interfaces identified via platform"
-		 " interfaces besides ACPI, OpenFirmware, and DMI");
+MODULE_PARM_DESC(tryplatform,
+		 "Setting this to zero will disable the default scan of the interfaces identified via platform interfaces besides ACPI, OpenFirmware, and DMI");
 #ifdef CONFIG_ACPI
 module_param_named(tryacpi, si_tryacpi, bool, 0);
-MODULE_PARM_DESC(tryacpi, "Setting this to zero will disable the"
-		 " default scan of the interfaces identified via ACPI");
+MODULE_PARM_DESC(tryacpi,
+		 "Setting this to zero will disable the default scan of the interfaces identified via ACPI");
 #endif
 #ifdef CONFIG_OF
 module_param_named(tryopenfirmware, si_tryopenfirmware, bool, 0);
-MODULE_PARM_DESC(tryopenfirmware, "Setting this to zero will disable the"
-		 " default scan of the interfaces identified via OpenFirmware");
+MODULE_PARM_DESC(tryopenfirmware,
+		 "Setting this to zero will disable the default scan of the interfaces identified via OpenFirmware");
 #endif
 #ifdef CONFIG_DMI
 module_param_named(trydmi, si_trydmi, bool, 0);
-MODULE_PARM_DESC(trydmi, "Setting this to zero will disable the"
-		 " default scan of the interfaces identified via DMI");
+MODULE_PARM_DESC(trydmi,
+		 "Setting this to zero will disable the default scan of the interfaces identified via DMI");
 #endif
 
 #ifdef CONFIG_ACPI
-- 
2.30.2

Re: [PATCH v1 03/10] ipmi_si: Utilize temporary variable to hold device pointer

[Corey Minyard]

On Tue, Mar 30, 2021 at 09:16:42PM +0300, Andy Shevchenko wrote:
> By one of the previous clean up change we got a temporary variable to hold
> a device pointer. It can be utilized in other calls in the ->probe() and
> save a bit of LOCs.

The description here isn't accurate, there is no previous change where a
temporary variable comes in.  This change adds the temporary variable.

This change is ok, but doesn't add much value.

-corey

> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/char/ipmi/ipmi_si_platform.c | 15 +++++++--------
>  1 file changed, 7 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/char/ipmi/ipmi_si_platform.c b/drivers/char/ipmi/ipmi_si_platform.c
> index 009563073d30..954c297b459b 100644
> --- a/drivers/char/ipmi/ipmi_si_platform.c
> +++ b/drivers/char/ipmi/ipmi_si_platform.c
> @@ -309,6 +309,7 @@ static int find_slave_address(struct si_sm_io *io, int slave_addr)
>  
>  static int acpi_ipmi_probe(struct platform_device *pdev)
>  {
> +	struct device *dev = &pdev->dev;
>  	struct si_sm_io io;
>  	acpi_handle handle;
>  	acpi_status status;
> @@ -318,21 +319,20 @@ static int acpi_ipmi_probe(struct platform_device *pdev)
>  	if (!si_tryacpi)
>  		return -ENODEV;
>  
> -	handle = ACPI_HANDLE(&pdev->dev);
> +	handle = ACPI_HANDLE(dev);
>  	if (!handle)
>  		return -ENODEV;
>  
>  	memset(&io, 0, sizeof(io));
>  	io.addr_source = SI_ACPI;
> -	dev_info(&pdev->dev, "probing via ACPI\n");
> +	dev_info(dev, "probing via ACPI\n");
>  
>  	io.addr_info.acpi_info.acpi_handle = handle;
>  
>  	/* _IFT tells us the interface type: KCS, BT, etc */
>  	status = acpi_evaluate_integer(handle, "_IFT", NULL, &tmp);
>  	if (ACPI_FAILURE(status)) {
> -		dev_err(&pdev->dev,
> -			"Could not find ACPI IPMI interface type\n");
> +		dev_err(dev, "Could not find ACPI IPMI interface type\n");
>  		return -EINVAL;
>  	}
>  
> @@ -349,10 +349,11 @@ static int acpi_ipmi_probe(struct platform_device *pdev)
>  	case 4: /* SSIF, just ignore */
>  		return -ENODEV;
>  	default:
> -		dev_info(&pdev->dev, "unknown IPMI type %lld\n", tmp);
> +		dev_info(dev, "unknown IPMI type %lld\n", tmp);
>  		return -EINVAL;
>  	}
>  
> +	io.dev = dev;
>  	io.regsize = DEFAULT_REGSIZE;
>  	io.regshift = 0;
>  
> @@ -376,9 +377,7 @@ static int acpi_ipmi_probe(struct platform_device *pdev)
>  
>  	io.slave_addr = find_slave_address(&io, io.slave_addr);
>  
> -	io.dev = &pdev->dev;
> -
> -	dev_info(io.dev, "%pR regsize %d spacing %d irq %d\n",
> +	dev_info(dev, "%pR regsize %d spacing %d irq %d\n",
>  		 res, io.regsize, io.regspacing, io.irq);
>  
>  	request_module("acpi_ipmi");
> -- 
> 2.30.2
> 

Re: [PATCH v1 05/10] ipmi_si: Introduce panic_event_str array

[Corey Minyard]

On Tue, Mar 30, 2021 at 09:16:44PM +0300, Andy Shevchenko wrote:
> Instead of twice repeat the constant literals, introduce
> panic_event_str array. It allows to simplify the code with
> help of match_string() API.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/char/ipmi/ipmi_msghandler.c | 49 ++++++++++-------------------
>  1 file changed, 17 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/char/ipmi/ipmi_msghandler.c b/drivers/char/ipmi/ipmi_msghandler.c
> index f19f0f967e28..c7d37366d7bb 100644
> --- a/drivers/char/ipmi/ipmi_msghandler.c
> +++ b/drivers/char/ipmi/ipmi_msghandler.c
> @@ -52,8 +52,12 @@ static bool drvregistered;
>  enum ipmi_panic_event_op {
>  	IPMI_SEND_PANIC_EVENT_NONE,
>  	IPMI_SEND_PANIC_EVENT,
> -	IPMI_SEND_PANIC_EVENT_STRING
> +	IPMI_SEND_PANIC_EVENT_STRING,
> +	IPMI_SEND_PANIC_EVENT_MAX
>  };

This is a nice change.  Can you add a comment here so that readers know
that the above enum and the following array are tied numerically?

-corey

> +
> +static const char *const panic_event_str[] = { "none", "event", "string", NULL };
> +
>  #ifdef CONFIG_IPMI_PANIC_STRING
>  #define IPMI_PANIC_DEFAULT IPMI_SEND_PANIC_EVENT_STRING
>  #elif defined(CONFIG_IPMI_PANIC_EVENT)
> @@ -68,46 +72,27 @@ static int panic_op_write_handler(const char *val,
>  				  const struct kernel_param *kp)
>  {
>  	char valcp[16];
> -	char *s;
> -
> -	strncpy(valcp, val, 15);
> -	valcp[15] = '\0';
> +	int e;
>  
> -	s = strstrip(valcp);
> -
> -	if (strcmp(s, "none") == 0)
> -		ipmi_send_panic_event = IPMI_SEND_PANIC_EVENT_NONE;
> -	else if (strcmp(s, "event") == 0)
> -		ipmi_send_panic_event = IPMI_SEND_PANIC_EVENT;
> -	else if (strcmp(s, "string") == 0)
> -		ipmi_send_panic_event = IPMI_SEND_PANIC_EVENT_STRING;
> -	else
> -		return -EINVAL;
> +	strscpy(valcp, val, sizeof(valcp));
> +	e = match_string(panic_event_str, -1, strstrip(valcp));
> +	if (e < 0)
> +		return e;
>  
> +	ipmi_send_panic_event = e;
>  	return 0;
>  }
>  
>  static int panic_op_read_handler(char *buffer, const struct kernel_param *kp)
>  {
> -	switch (ipmi_send_panic_event) {
> -	case IPMI_SEND_PANIC_EVENT_NONE:
> -		strcpy(buffer, "none\n");
> -		break;
> -
> -	case IPMI_SEND_PANIC_EVENT:
> -		strcpy(buffer, "event\n");
> -		break;
> -
> -	case IPMI_SEND_PANIC_EVENT_STRING:
> -		strcpy(buffer, "string\n");
> -		break;
> +	const char *event_str;
>  
> -	default:
> -		strcpy(buffer, "???\n");
> -		break;
> -	}
> +	if (ipmi_send_panic_event >= IPMI_SEND_PANIC_EVENT_MAX)
> +		event_str = "???";
> +	else
> +		event_str = panic_event_str[ipmi_send_panic_event];
>  
> -	return strlen(buffer);
> +	return sprintf(buffer, "%s\n", event_str);
>  }
>  
>  static const struct kernel_param_ops panic_op_ops = {
> -- 
> 2.30.2
> 

Re: [PATCH v1 06/10] ipmi_si: Reuse si_to_str array in ipmi_hardcode_init_one()

[Corey Minyard]

On Tue, Mar 30, 2021 at 09:16:45PM +0300, Andy Shevchenko wrote:
> Instead of making the comparison one by one, reuse si_to_str array
> in ipmi_hardcode_init_one() in conjunction with match_string() API.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/char/ipmi/ipmi_si.h          |  3 +++
>  drivers/char/ipmi/ipmi_si_hardcode.c | 23 +++++++++--------------
>  drivers/char/ipmi/ipmi_si_intf.c     |  2 --
>  3 files changed, 12 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/char/ipmi/ipmi_si.h b/drivers/char/ipmi/ipmi_si.h
> index bac0ff86e48e..fd3167d1e1e9 100644
> --- a/drivers/char/ipmi/ipmi_si.h
> +++ b/drivers/char/ipmi/ipmi_si.h
> @@ -22,6 +22,9 @@ enum si_type {
>  	SI_TYPE_INVALID, SI_KCS, SI_SMIC, SI_BT
>  };
>  
> +/* 'invalid' to allow a firmware-specified interface to be disabled */
> +static __maybe_unused const char *const si_to_str[] = { "invalid", "kcs", "smic", "bt" };

Can we just make this non-static and leave the definition where it is?
That would save a little space and wouldn't affect performance at all.

-corey

> +
>  enum ipmi_addr_space {
>  	IPMI_IO_ADDR_SPACE, IPMI_MEM_ADDR_SPACE
>  };
> diff --git a/drivers/char/ipmi/ipmi_si_hardcode.c b/drivers/char/ipmi/ipmi_si_hardcode.c
> index f6ece7569504..cf3797523469 100644
> --- a/drivers/char/ipmi/ipmi_si_hardcode.c
> +++ b/drivers/char/ipmi/ipmi_si_hardcode.c
> @@ -80,26 +80,21 @@ static void __init ipmi_hardcode_init_one(const char *si_type_str,
>  					  enum ipmi_addr_space addr_space)
>  {
>  	struct ipmi_plat_data p;
> +	int t;
>  
>  	memset(&p, 0, sizeof(p));
>  
>  	p.iftype = IPMI_PLAT_IF_SI;
> -	if (!si_type_str || !*si_type_str || strcmp(si_type_str, "kcs") == 0) {
> +	if (!si_type_str || !*si_type_str) {
>  		p.type = SI_KCS;
> -	} else if (strcmp(si_type_str, "smic") == 0) {
> -		p.type = SI_SMIC;
> -	} else if (strcmp(si_type_str, "bt") == 0) {
> -		p.type = SI_BT;
> -	} else if (strcmp(si_type_str, "invalid") == 0) {
> -		/*
> -		 * Allow a firmware-specified interface to be
> -		 * disabled.
> -		 */
> -		p.type = SI_TYPE_INVALID;
>  	} else {
> -		pr_warn("Interface type specified for interface %d, was invalid: %s\n",
> -			i, si_type_str);
> -		return;
> +		t = match_string(si_to_str, ARRAY_SIZE(si_to_str), si_type_str);
> +		if (t < 0) {
> +			pr_warn("Interface type specified for interface %d, was invalid: %s\n",
> +				i, si_type_str);
> +			return;
> +		}
> +		p.type = t;
>  	}
>  
>  	p.regsize = regsizes[i];
> diff --git a/drivers/char/ipmi/ipmi_si_intf.c b/drivers/char/ipmi/ipmi_si_intf.c
> index be41a473e3c2..ff448098f185 100644
> --- a/drivers/char/ipmi/ipmi_si_intf.c
> +++ b/drivers/char/ipmi/ipmi_si_intf.c
> @@ -70,8 +70,6 @@ enum si_intf_state {
>  #define IPMI_BT_INTMASK_CLEAR_IRQ_BIT	2
>  #define IPMI_BT_INTMASK_ENABLE_IRQ_BIT	1
>  
> -static const char * const si_to_str[] = { "invalid", "kcs", "smic", "bt" };
> -
>  static bool initialized;
>  
>  /*
> -- 
> 2.30.2
> 

Re: [PATCH v1 01/10] ipmi_si: Switch to use platform_get_mem_or_io()

[Corey Minyard]

On Tue, Mar 30, 2021 at 09:16:40PM +0300, Andy Shevchenko wrote:
> Switch to use new platform_get_mem_or_io() instead of home grown analogue.
> Note, we also introduce ipmi_set_addr_data_and_space() helper here.

You didn't send a part 0 that I saw, so just using this.  This is a nice
cleanup set, I just had a few very minor nits.  Thanks for this.

-corey

> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/char/ipmi/ipmi_si_platform.c | 40 +++++++++++-----------------
>  1 file changed, 16 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/char/ipmi/ipmi_si_platform.c b/drivers/char/ipmi/ipmi_si_platform.c
> index 129b5713f187..d7bd093f80e9 100644
> --- a/drivers/char/ipmi/ipmi_si_platform.c
> +++ b/drivers/char/ipmi/ipmi_si_platform.c
> @@ -100,35 +100,32 @@ static int acpi_gpe_irq_setup(struct si_sm_io *io)
>  }
>  #endif
>  
> +static void ipmi_set_addr_data_and_space(struct resource *r, struct si_sm_io *io)
> +{
> +	io->addr_data = r->start;
> +	if (resource_type(r) == IORESOURCE_IO)
> +		io->addr_space = IPMI_IO_ADDR_SPACE;
> +	else
> +		io->addr_space = IPMI_MEM_ADDR_SPACE;
> +}
> +
>  static struct resource *
>  ipmi_get_info_from_resources(struct platform_device *pdev,
>  			     struct si_sm_io *io)
>  {
> -	struct resource *res, *res_second;
> +	struct resource *res, *second;
>  
> -	res = platform_get_resource(pdev, IORESOURCE_IO, 0);
> -	if (res) {
> -		io->addr_space = IPMI_IO_ADDR_SPACE;
> -	} else {
> -		res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> -		if (res)
> -			io->addr_space = IPMI_MEM_ADDR_SPACE;
> -	}
> +	res = platform_get_mem_or_io(pdev, 0);
>  	if (!res) {
>  		dev_err(&pdev->dev, "no I/O or memory address\n");
>  		return NULL;
>  	}
> -	io->addr_data = res->start;
> +	ipmi_set_addr_data_and_space(res, io);
>  
>  	io->regspacing = DEFAULT_REGSPACING;
> -	res_second = platform_get_resource(pdev,
> -			       (io->addr_space == IPMI_IO_ADDR_SPACE) ?
> -					IORESOURCE_IO : IORESOURCE_MEM,
> -			       1);
> -	if (res_second) {
> -		if (res_second->start > io->addr_data)
> -			io->regspacing = res_second->start - io->addr_data;
> -	}
> +	second = platform_get_mem_or_io(pdev, 1);
> +	if (second && resource_type(second) == resource_type(res) && second->start > io->addr_data)
> +		io->regspacing = second->start - io->addr_data;
>  
>  	return res;
>  }
> @@ -275,12 +272,7 @@ static int of_ipmi_probe(struct platform_device *pdev)
>  	io.addr_source	= SI_DEVICETREE;
>  	io.irq_setup	= ipmi_std_irq_setup;
>  
> -	if (resource.flags & IORESOURCE_IO)
> -		io.addr_space = IPMI_IO_ADDR_SPACE;
> -	else
> -		io.addr_space = IPMI_MEM_ADDR_SPACE;
> -
> -	io.addr_data	= resource.start;
> +	ipmi_set_addr_data_and_space(&resource, &io);
>  
>  	io.regsize	= regsize ? be32_to_cpup(regsize) : DEFAULT_REGSIZE;
>  	io.regspacing	= regspacing ? be32_to_cpup(regspacing) : DEFAULT_REGSPACING;
> -- 
> 2.30.2
>