[PATCH v1 1/4] bus: hisi_lpc: Don't dereference fwnode handle

[PATCH v1 1/4] bus: hisi_lpc: Don't dereference fwnode handle

[Andy Shevchenko]

Use dev_fwnode() and acpi_fwnode_handle() instead of dereferencing
an fwnode handle directly.

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

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.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.1

[PATCH v1 2/4] bus: hisi_lpc: Use devm_platform_ioremap_resource

[Andy Shevchenko]

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>
---
 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.1

[PATCH v1 3/4] bus: hisi_lpc: Correct error code for timeout

[Andy Shevchenko]

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

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.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.1

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

[Andy Shevchenko]

The OF is not guarded neither ACPI needs. The IDs do not depend
to 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>
---
 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.1

Re: [PATCH v1 1/4] bus: hisi_lpc: Don't dereference fwnode handle

[Rafael J. Wysocki]

On 7/5/2022 1:43 PM, Andy Shevchenko wrote:
> Use dev_fwnode() and acpi_fwnode_handle() instead of dereferencing
> an fwnode handle directly.
>
> 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>


> ---
>   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);

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

[Rafael J. Wysocki]

On 7/5/2022 1:43 PM, Andy Shevchenko wrote:
> The OF is not guarded neither ACPI needs. The IDs do not depend
> to 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>


> ---
>   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,

Re: [PATCH v1 2/4] bus: hisi_lpc: Use devm_platform_ioremap_resource

[Rafael J. Wysocki]

On 7/5/2022 1:43 PM, Andy Shevchenko wrote:
> 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>


> ---
>   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);
>   

Re: [PATCH v1 3/4] bus: hisi_lpc: Correct error code for timeout

[Rafael J. Wysocki]

On 7/5/2022 1:43 PM, Andy Shevchenko wrote:
> 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>


> ---
>   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;
>   }
>   
>   /*

Re: [PATCH v1 2/4] bus: hisi_lpc: Use devm_platform_ioremap_resource

[John Garry]

On 05/07/2022 12:43, Andy Shevchenko wrote:
> 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>

Acked-by: John Garry <john.garry@huawei.com>

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

[John Garry]

On 05/07/2022 12:43, Andy Shevchenko wrote:
> The OF is not guarded neither ACPI needs.

This doesn't read well.

> The IDs do not depend
> to the configuration. Hence drop ACPI_PTR() from the code and
> move ID table closer to its user.

Do you need to explicitly include mod_devicetable.h, which has the 
definition of acpi_device_id?

I saw a similar change for another driver and it was claimed that 
including mod_devicetable.h was required.

Thanks,
John

> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.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,

Re: [PATCH v1 3/4] bus: hisi_lpc: Correct error code for timeout

[John Garry]

On 05/07/2022 12:43, Andy Shevchenko wrote:
> The usual error code is -ETIMEDOUT, the currently used -ETIME is specific
> for timers.
> 
> Signed-off-by: Andy Shevchenko<andriy.shevchenko@linux.intel.com>

Acked-by: John Garry <john.garry@huawei.com>

Re: [PATCH v1 1/4] bus: hisi_lpc: Don't dereference fwnode handle

[John Garry]

On 05/07/2022 12:43, Andy Shevchenko wrote:
> Use dev_fwnode() and acpi_fwnode_handle() instead of dereferencing
> an fwnode handle directly.

...which is a better coding practice, right? If so, it would be nice to 
mention it - well at least I think so.

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

Apart from above and nit, below:
Acked-by: John Garry <john.garry@huawei.com>

> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.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);

nit: currently the driver keeps to the old 80 character line limit. 
While the rules may have been relaxed, I'd rather we still maintain it.

>   	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);

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

[Andy Shevchenko]

On Tue, Jul 5, 2022 at 5:02 PM John Garry <john.garry@huawei.com> wrote:
> On 05/07/2022 12:43, Andy Shevchenko wrote:
> > The OF is not guarded, neither ACPI needs.
>
> This doesn't read well.

"The OF is not guarded, neither ACPI needs it."

Better? Otherwise please propose how it can be amended here.

> > The IDs do not depend
> > to the configuration. Hence drop ACPI_PTR() from the code and
> > move ID table closer to its user.
>
> Do you need to explicitly include mod_devicetable.h, which has the
> definition of acpi_device_id?
>
> I saw a similar change for another driver and it was claimed that
> including mod_devicetable.h was required.

Strictly speaking, yes we need mod_devicetable.h. But of.h and acpi.h
include it.

What you have seen is probably dropping of.h and/or acpi.h completely
from the user. In such cases the mod_devicetable.h is compulsory.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/4] bus: hisi_lpc: Don't dereference fwnode handle

[Andy Shevchenko]

On Tue, Jul 5, 2022 at 5:11 PM John Garry <john.garry@huawei.com> wrote:
> On 05/07/2022 12:43, Andy Shevchenko wrote:
> > Use dev_fwnode() and acpi_fwnode_handle() instead of dereferencing
> > an fwnode handle directly.
>
> ...which is a better coding practice, right? If so, it would be nice to
> mention it - well at least I think so.

Not only. In the case of fwnode it's a long story behind its corner
case(s) where in the future we might switch from embedded structure to
linked list, for example, in order to address those corner cases.
Should I write a paragraph for that as well?

...

> Apart from above and nit, below:

See below my answer.

> Acked-by: John Garry <john.garry@huawei.com>

Thanks.

...

> > -     sys_port = logic_pio_trans_hwaddr(&host->fwnode, res->start, len);
> > +     sys_port = logic_pio_trans_hwaddr(acpi_fwnode_handle(host), res->start, len);
>
> nit: currently the driver keeps to the old 80 character line limit.
> While the rules may have been relaxed, I'd rather we still maintain it.

First of all, even before the 100 characters era the rule had two exceptions:
1) the string literals;
2) the readability over strictness of the 80 characters rule.

While I agree in general with you, in this case I think keeping
strictness makes readability worse.

-- 
With Best Regards,
Andy Shevchenko

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

[John Garry]

On 05/07/2022 16:15, Andy Shevchenko wrote:

With a change to the commit message along the line below:

Acked-by: John Garry <john.garry@huawei.com>

> On Tue, Jul 5, 2022 at 5:02 PM John Garry<john.garry@huawei.com>  wrote:
>> On 05/07/2022 12:43, Andy Shevchenko wrote:
>>> The OF is not guarded, neither ACPI needs.
>> This doesn't read well.
> "The OF is not guarded, neither ACPI needs it."
> 
> Better? Otherwise please propose how it can be amended here.

How about "The OF ID table is not guarded, and the ACPI table does not 
needs it either."?

> 
>>> The IDs do not depend
>>> to the configuration. Hence drop ACPI_PTR() from the code and
>>> move ID table closer to its user.
>> Do you need to explicitly include mod_devicetable.h, which has the
>> definition of acpi_device_id?
>>
>> I saw a similar change for another driver and it was claimed that
>> including mod_devicetable.h was required.
> Strictly speaking, yes we need mod_devicetable.h. But of.h and acpi.h
> include it.

acpi.h does not include it for !CONFIG_ACPI, which is the only one which 
I had checked. But now I see that of.h always includes it, so what you 
are doing is ok.

> 
> What you have seen is probably dropping of.h and/or acpi.h completely
> from the user.

Right

> In such cases the mod_devicetable.h is compulsory.

Sure

Thanks,
John

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

[Andy Shevchenko]

On Tue, Jul 5, 2022 at 5:27 PM John Garry <john.garry@huawei.com> wrote:
> On 05/07/2022 16:15, Andy Shevchenko wrote:

...

> >>> The OF is not guarded, neither ACPI needs.
> >> This doesn't read well.
> > "The OF is not guarded, neither ACPI needs it."
> >
> > Better? Otherwise please propose how it can be amended here.
>
> How about "The OF ID table is not guarded, and the ACPI table does not
> needs it either."?

FIne with me.

...

> > Strictly speaking, yes we need mod_devicetable.h. But of.h and acpi.h
> > include it.
>
> acpi.h does not include it for !CONFIG_ACPI, which is the only one which
> I had checked. But now I see that of.h always includes it, so what you
> are doing is ok.

What a surprise. I was under the impression that acpi.h always
includes it. Hmm... Probably we never had drivers that in Kconfig have
something like "depends on ACPI || COMPILE_TEST (and at the same time
have no explicit mod_devicetable.h inclusion nor implicit providers
like of.h), which should immediately point to the issue.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/4] bus: hisi_lpc: Don't dereference fwnode handle

[John Garry]

On 05/07/2022 16:23, Andy Shevchenko wrote:
> On Tue, Jul 5, 2022 at 5:11 PM John Garry<john.garry@huawei.com>  wrote:
>> On 05/07/2022 12:43, Andy Shevchenko wrote:
>>> Use dev_fwnode() and acpi_fwnode_handle() instead of dereferencing
>>> an fwnode handle directly.
>> ...which is a better coding practice, right? If so, it would be nice to
>> mention it - well at least I think so.
> Not only. In the case of fwnode it's a long story behind its corner
> case(s) where in the future we might switch from embedded structure to
> linked list, for example, in order to address those corner cases.
> Should I write a paragraph for that as well?
> 

If you just say that it's a better coding practice to use available APIs 
to access structure members rather than access them directly, then that 
is good enough. Or maybe you can think of something better along the 
lines of what you wrote above. My issue is that there was no reason. So 
I'll leave it to you.

> ...
> 
>> Apart from above and nit, below:
> See below my answer.
> 
>> Acked-by: John Garry<john.garry@huawei.com>
> Thanks.
> 
> ...
> 
>>> -     sys_port = logic_pio_trans_hwaddr(&host->fwnode, res->start, len);
>>> +     sys_port = logic_pio_trans_hwaddr(acpi_fwnode_handle(host), res->start, len);
>> nit: currently the driver keeps to the old 80 character line limit.
>> While the rules may have been relaxed, I'd rather we still maintain it.
> First of all, even before the 100 characters era the rule had two exceptions:
> 1) the string literals;

Sure

> 2) the readability over strictness of the 80 characters rule.
> 
> While I agree in general with you, in this case I think keeping
> strictness makes readability worse.

ok, fine. I was going to suggest introduce a varible to hold 
acpi_fwnode_handle(host) but then we may not want a variable of 
fwnode_handle* type hanging around. Anyway I don't feel too strongly 
about it.

Thanks,
John