[PATCH v2 0/5] Misc hisi_lpc changes

[PATCH v2 0/5] Misc hisi_lpc changes

[John Garry]

Hi Xu Wei,

This is a series of small improvements to the driver from Andy
and myself.

Andy sent his patches originally in the following:
https://lore.kernel.org/lkml/20220708210859.6774-1-andriy.shevchenko@linux.intel.com/

Please consider sending through the arm-soc tree for v6.1 .

Based on v6.0-rc3.

Differences to v1:
- remove blank lines in patch 5/5, as requested by Andy

Thanks,
John

Andy Shevchenko (4):
  bus: hisi_lpc: Don't dereference fwnode handle
  bus: hisi_lpc: Use devm_platform_ioremap_resource
  bus: hisi_lpc: Correct error code for timeout
  bus: hisi_lpc: Don't guard ACPI IDs with ACPI_PTR()

John Garry (1):
  bus: hisi_lpc: Use platform_device_register_full()

 drivers/bus/hisi_lpc.c | 92 +++++++++++++++++++-----------------------
 1 file changed, 42 insertions(+), 50 deletions(-)

-- 
2.35.3

[PATCH v2 1/5] bus: hisi_lpc: Don't dereference fwnode handle

[John Garry]

From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Use dev_fwnode() and acpi_fwnode_handle() instead of dereferencing
an fwnode handle directly, which is a better coding practice.

While at it, reuse fwnode instead of ACPI_COMPANION().

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Signed-off-by: John Garry <john.garry@huawei.com>
---
 drivers/bus/hisi_lpc.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/bus/hisi_lpc.c b/drivers/bus/hisi_lpc.c
index 2e564803e786..6d432a07cbba 100644
--- a/drivers/bus/hisi_lpc.c
+++ b/drivers/bus/hisi_lpc.c
@@ -347,7 +347,7 @@ static int hisi_lpc_acpi_xlat_io_res(struct acpi_device *adev,
 	unsigned long sys_port;
 	resource_size_t len = resource_size(res);
 
-	sys_port = logic_pio_trans_hwaddr(&host->fwnode, res->start, len);
+	sys_port = logic_pio_trans_hwaddr(acpi_fwnode_handle(host), res->start, len);
 	if (sys_port == ~0UL)
 		return -EFAULT;
 
@@ -615,7 +615,6 @@ static void hisi_lpc_acpi_remove(struct device *hostdev)
 static int hisi_lpc_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
-	struct acpi_device *acpi_device = ACPI_COMPANION(dev);
 	struct logic_pio_hwaddr *range;
 	struct hisi_lpc_dev *lpcdev;
 	resource_size_t io_end;
@@ -637,7 +636,7 @@ static int hisi_lpc_probe(struct platform_device *pdev)
 	if (!range)
 		return -ENOMEM;
 
-	range->fwnode = dev->fwnode;
+	range->fwnode = dev_fwnode(dev);
 	range->flags = LOGIC_PIO_INDIRECT;
 	range->size = PIO_INDIRECT_SIZE;
 	range->hostdata = lpcdev;
@@ -651,7 +650,7 @@ static int hisi_lpc_probe(struct platform_device *pdev)
 	}
 
 	/* register the LPC host PIO resources */
-	if (acpi_device)
+	if (is_acpi_device_node(range->fwnode))
 		ret = hisi_lpc_acpi_probe(dev);
 	else
 		ret = of_platform_populate(dev->of_node, NULL, NULL, dev);
@@ -672,11 +671,10 @@ static int hisi_lpc_probe(struct platform_device *pdev)
 static int hisi_lpc_remove(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
-	struct acpi_device *acpi_device = ACPI_COMPANION(dev);
 	struct hisi_lpc_dev *lpcdev = dev_get_drvdata(dev);
 	struct logic_pio_hwaddr *range = lpcdev->io_host;
 
-	if (acpi_device)
+	if (is_acpi_device_node(range->fwnode))
 		hisi_lpc_acpi_remove(dev);
 	else
 		of_platform_depopulate(dev);
-- 
2.35.3

[PATCH v2 2/5] bus: hisi_lpc: Use devm_platform_ioremap_resource

[John Garry]

From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

The struct resource is not used for anything else, so we can simplify
the code a bit by using the helper function.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Signed-off-by: John Garry <john.garry@huawei.com>
---
 drivers/bus/hisi_lpc.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/bus/hisi_lpc.c b/drivers/bus/hisi_lpc.c
index 6d432a07cbba..03d4d96ff794 100644
--- a/drivers/bus/hisi_lpc.c
+++ b/drivers/bus/hisi_lpc.c
@@ -618,7 +618,6 @@ static int hisi_lpc_probe(struct platform_device *pdev)
 	struct logic_pio_hwaddr *range;
 	struct hisi_lpc_dev *lpcdev;
 	resource_size_t io_end;
-	struct resource *res;
 	int ret;
 
 	lpcdev = devm_kzalloc(dev, sizeof(*lpcdev), GFP_KERNEL);
@@ -627,8 +626,7 @@ static int hisi_lpc_probe(struct platform_device *pdev)
 
 	spin_lock_init(&lpcdev->cycle_lock);
 
-	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	lpcdev->membase = devm_ioremap_resource(dev, res);
+	lpcdev->membase = devm_platform_ioremap_resource(pdev, 0);
 	if (IS_ERR(lpcdev->membase))
 		return PTR_ERR(lpcdev->membase);
 
-- 
2.35.3

[PATCH v2 3/5] bus: hisi_lpc: Correct error code for timeout

[John Garry]

From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

The usual error code is -ETIMEDOUT, the currently used -ETIME is specific
for timers.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Signed-off-by: John Garry <john.garry@huawei.com>
---
 drivers/bus/hisi_lpc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/bus/hisi_lpc.c b/drivers/bus/hisi_lpc.c
index 03d4d96ff794..a6513a571d7b 100644
--- a/drivers/bus/hisi_lpc.c
+++ b/drivers/bus/hisi_lpc.c
@@ -85,7 +85,7 @@ static int wait_lpc_idle(void __iomem *mbase, unsigned int waitcnt)
 		ndelay(LPC_NSEC_PERWAIT);
 	} while (--waitcnt);
 
-	return -ETIME;
+	return -ETIMEDOUT;
 }
 
 /*
-- 
2.35.3

[PATCH v2 4/5] bus: hisi_lpc: Don't guard ACPI IDs with ACPI_PTR()

[John Garry]

From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

The OF ID table is not guarded, and the ACPI table does not needs it either.
The IDs do not depend on the configuration. Hence drop ACPI_PTR() from the
code and move ID table closer to its user.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Signed-off-by: John Garry <john.garry@huawei.com>
---
 drivers/bus/hisi_lpc.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/bus/hisi_lpc.c b/drivers/bus/hisi_lpc.c
index a6513a571d7b..74f4448bff9d 100644
--- a/drivers/bus/hisi_lpc.c
+++ b/drivers/bus/hisi_lpc.c
@@ -589,11 +589,6 @@ static int hisi_lpc_acpi_probe(struct device *hostdev)
 
 	return ret;
 }
-
-static const struct acpi_device_id hisi_lpc_acpi_match[] = {
-	{"HISI0191"},
-	{}
-};
 #else
 static int hisi_lpc_acpi_probe(struct device *dev)
 {
@@ -688,11 +683,16 @@ static const struct of_device_id hisi_lpc_of_match[] = {
 	{}
 };
 
+static const struct acpi_device_id hisi_lpc_acpi_match[] = {
+	{"HISI0191"},
+	{}
+};
+
 static struct platform_driver hisi_lpc_driver = {
 	.driver = {
 		.name           = DRV_NAME,
 		.of_match_table = hisi_lpc_of_match,
-		.acpi_match_table = ACPI_PTR(hisi_lpc_acpi_match),
+		.acpi_match_table = hisi_lpc_acpi_match,
 	},
 	.probe = hisi_lpc_probe,
 	.remove = hisi_lpc_remove,
-- 
2.35.3

[PATCH v2 5/5] bus: hisi_lpc: Use platform_device_register_full()

[John Garry]

The code to create the child platform device is essentially the same as
what platform_device_register_full() does, so change over to use
that same function to reduce duplication.

Signed-off-by: John Garry <john.garry@huawei.com>
Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/bus/hisi_lpc.c | 64 ++++++++++++++++++++----------------------
 1 file changed, 30 insertions(+), 34 deletions(-)

diff --git a/drivers/bus/hisi_lpc.c b/drivers/bus/hisi_lpc.c
index 74f4448bff9d..3555a6857214 100644
--- a/drivers/bus/hisi_lpc.c
+++ b/drivers/bus/hisi_lpc.c
@@ -472,9 +472,7 @@ static int hisi_lpc_acpi_clear_enumerated(struct acpi_device *adev, void *not_us
 
 struct hisi_lpc_acpi_cell {
 	const char *hid;
-	const char *name;
-	void *pdata;
-	size_t pdata_size;
+	const struct platform_device_info *pdevinfo;
 };
 
 static void hisi_lpc_acpi_remove(struct device *hostdev)
@@ -505,28 +503,45 @@ static int hisi_lpc_acpi_add_child(struct acpi_device *child, void *data)
 		/* ipmi */
 		{
 			.hid = "IPI0001",
-			.name = "hisi-lpc-ipmi",
+			.pdevinfo = (struct platform_device_info []) {
+				{
+					.parent = hostdev,
+					.fwnode = acpi_fwnode_handle(child),
+					.name = "hisi-lpc-ipmi",
+					.id = PLATFORM_DEVID_AUTO,
+					.res = res,
+					.num_res = num_res,
+				},
+			},
 		},
 		/* 8250-compatible uart */
 		{
 			.hid = "HISI1031",
-			.name = "serial8250",
-			.pdata = (struct plat_serial8250_port []) {
+			.pdevinfo = (struct platform_device_info []) {
 				{
-					.iobase = res->start,
-					.uartclk = 1843200,
-					.iotype = UPIO_PORT,
-					.flags = UPF_BOOT_AUTOCONF,
+					.parent = hostdev,
+					.fwnode = acpi_fwnode_handle(child),
+					.name = "serial8250",
+					.id = PLATFORM_DEVID_AUTO,
+					.res = res,
+					.num_res = num_res,
+					.data = (struct plat_serial8250_port []) {
+						{
+							.iobase = res->start,
+							.uartclk = 1843200,
+							.iotype = UPIO_PORT,
+							.flags = UPF_BOOT_AUTOCONF,
+						},
+						{}
+					},
+					.size_data =  2 * sizeof(struct plat_serial8250_port),
 				},
-				{}
 			},
-			.pdata_size = 2 *
-				sizeof(struct plat_serial8250_port),
 		},
 		{}
 	};
 
-	for (; cell && cell->name; cell++) {
+	for (; cell && cell->hid; cell++) {
 		if (!strcmp(cell->hid, hid)) {
 			found = true;
 			break;
@@ -540,31 +555,12 @@ static int hisi_lpc_acpi_add_child(struct acpi_device *child, void *data)
 		return 0;
 	}
 
-	pdev = platform_device_alloc(cell->name, PLATFORM_DEVID_AUTO);
+	pdev = platform_device_register_full(cell->pdevinfo);
 	if (!pdev)
 		return -ENOMEM;
 
-	pdev->dev.parent = hostdev;
-	ACPI_COMPANION_SET(&pdev->dev, child);
-
-	ret = platform_device_add_resources(pdev, res, num_res);
-	if (ret)
-		goto fail;
-
-	ret = platform_device_add_data(pdev, cell->pdata, cell->pdata_size);
-	if (ret)
-		goto fail;
-
-	ret = platform_device_add(pdev);
-	if (ret)
-		goto fail;
-
 	acpi_device_set_enumerated(child);
 	return 0;
-
-fail:
-	platform_device_put(pdev);
-	return ret;
 }
 
 /*
-- 
2.35.3

Re: [PATCH v2 5/5] bus: hisi_lpc: Use platform_device_register_full()

[Andy Shevchenko]

On Fri, Sep 2, 2022 at 11:10 AM John Garry <john.garry@huawei.com> wrote:
>
> The code to create the child platform device is essentially the same as
> what platform_device_register_full() does, so change over to use
> that same function to reduce duplication.

Now statistics plays for you and not against.
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

> Signed-off-by: John Garry <john.garry@huawei.com>
> Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/bus/hisi_lpc.c | 64 ++++++++++++++++++++----------------------
>  1 file changed, 30 insertions(+), 34 deletions(-)
>
> diff --git a/drivers/bus/hisi_lpc.c b/drivers/bus/hisi_lpc.c
> index 74f4448bff9d..3555a6857214 100644
> --- a/drivers/bus/hisi_lpc.c
> +++ b/drivers/bus/hisi_lpc.c
> @@ -472,9 +472,7 @@ static int hisi_lpc_acpi_clear_enumerated(struct acpi_device *adev, void *not_us
>
>  struct hisi_lpc_acpi_cell {
>         const char *hid;
> -       const char *name;
> -       void *pdata;
> -       size_t pdata_size;
> +       const struct platform_device_info *pdevinfo;
>  };
>
>  static void hisi_lpc_acpi_remove(struct device *hostdev)
> @@ -505,28 +503,45 @@ static int hisi_lpc_acpi_add_child(struct acpi_device *child, void *data)
>                 /* ipmi */
>                 {
>                         .hid = "IPI0001",
> -                       .name = "hisi-lpc-ipmi",
> +                       .pdevinfo = (struct platform_device_info []) {
> +                               {
> +                                       .parent = hostdev,
> +                                       .fwnode = acpi_fwnode_handle(child),
> +                                       .name = "hisi-lpc-ipmi",
> +                                       .id = PLATFORM_DEVID_AUTO,
> +                                       .res = res,
> +                                       .num_res = num_res,
> +                               },
> +                       },
>                 },
>                 /* 8250-compatible uart */
>                 {
>                         .hid = "HISI1031",
> -                       .name = "serial8250",
> -                       .pdata = (struct plat_serial8250_port []) {
> +                       .pdevinfo = (struct platform_device_info []) {
>                                 {
> -                                       .iobase = res->start,
> -                                       .uartclk = 1843200,
> -                                       .iotype = UPIO_PORT,
> -                                       .flags = UPF_BOOT_AUTOCONF,
> +                                       .parent = hostdev,
> +                                       .fwnode = acpi_fwnode_handle(child),
> +                                       .name = "serial8250",
> +                                       .id = PLATFORM_DEVID_AUTO,
> +                                       .res = res,
> +                                       .num_res = num_res,
> +                                       .data = (struct plat_serial8250_port []) {
> +                                               {
> +                                                       .iobase = res->start,
> +                                                       .uartclk = 1843200,
> +                                                       .iotype = UPIO_PORT,
> +                                                       .flags = UPF_BOOT_AUTOCONF,
> +                                               },
> +                                               {}
> +                                       },
> +                                       .size_data =  2 * sizeof(struct plat_serial8250_port),
>                                 },
> -                               {}
>                         },
> -                       .pdata_size = 2 *
> -                               sizeof(struct plat_serial8250_port),
>                 },
>                 {}
>         };
>
> -       for (; cell && cell->name; cell++) {
> +       for (; cell && cell->hid; cell++) {
>                 if (!strcmp(cell->hid, hid)) {
>                         found = true;
>                         break;
> @@ -540,31 +555,12 @@ static int hisi_lpc_acpi_add_child(struct acpi_device *child, void *data)
>                 return 0;
>         }
>
> -       pdev = platform_device_alloc(cell->name, PLATFORM_DEVID_AUTO);
> +       pdev = platform_device_register_full(cell->pdevinfo);
>         if (!pdev)
>                 return -ENOMEM;
>
> -       pdev->dev.parent = hostdev;
> -       ACPI_COMPANION_SET(&pdev->dev, child);
> -
> -       ret = platform_device_add_resources(pdev, res, num_res);
> -       if (ret)
> -               goto fail;
> -
> -       ret = platform_device_add_data(pdev, cell->pdata, cell->pdata_size);
> -       if (ret)
> -               goto fail;
> -
> -       ret = platform_device_add(pdev);
> -       if (ret)
> -               goto fail;
> -
>         acpi_device_set_enumerated(child);
>         return 0;
> -
> -fail:
> -       platform_device_put(pdev);
> -       return ret;
>  }
>
>  /*
> --
> 2.35.3
>


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 5/5] bus: hisi_lpc: Use platform_device_register_full()

[Andy Shevchenko]

On Fri, Sep 2, 2022 at 12:18 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Fri, Sep 2, 2022 at 11:10 AM John Garry <john.garry@huawei.com> wrote:
> >
> > The code to create the child platform device is essentially the same as
> > what platform_device_register_full() does, so change over to use
> > that same function to reduce duplication.
>
> Now statistics plays for you and not against.
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

Too fast to give a tag, see below (keep tag if addressing it)

...

> > -       pdev = platform_device_alloc(cell->name, PLATFORM_DEVID_AUTO);
> > +       pdev = platform_device_register_full(cell->pdevinfo);

> >         if (!pdev)
> >                 return -ENOMEM;

As per kernel doc:
 * Returns &struct platform_device pointer on success, or ERR_PTR() on error.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 5/5] bus: hisi_lpc: Use platform_device_register_full()

[John Garry]

On 02/09/2022 10:21, Andy Shevchenko wrote:
>> Reviewed-by: Andy Shevchenko<andy.shevchenko@gmail.com>
> Too fast to give a tag, see below (keep tag if addressing it)

thanks

> 
> ...
> 
>>> -       pdev = platform_device_alloc(cell->name, PLATFORM_DEVID_AUTO);
>>> +       pdev = platform_device_register_full(cell->pdevinfo);
>>>          if (!pdev)
>>>                  return -ENOMEM;
> As per kernel doc:
>   * Returns &struct platform_device pointer on success, or ERR_PTR() on error.

ah, right, I thought that I got that correct....

John