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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
The usual error code is -ETIMEDOUT, the currently used -ETIME is specific
for timers.
Signed-off-by: Andy Shevchenko <[email protected]>
---
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
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 <[email protected]>
Reviewed-by: Rafael J. Wysocki <[email protected]>
> ---
> 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);
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 <[email protected]>
Reviewed-by: Rafael J. Wysocki <[email protected]>
> ---
> 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,
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 <[email protected]>
Reviewed-by: Rafael J. Wysocki <[email protected]>
> ---
> 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);
>
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<[email protected]>
Acked-by: John Garry <[email protected]>
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 <[email protected]>
Reviewed-by: Rafael J. Wysocki <[email protected]>
> ---
> 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;
> }
>
> /*
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<[email protected]>
Acked-by: John Garry <[email protected]>
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 <[email protected]>
> ---
> 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,
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 <[email protected]>
>
> Signed-off-by: Andy Shevchenko <[email protected]>
> ---
> 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);
On Tue, Jul 5, 2022 at 5:11 PM John Garry <[email protected]> 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 <[email protected]>
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
On Tue, Jul 5, 2022 at 5:02 PM John Garry <[email protected]> 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
On Tue, Jul 5, 2022 at 5:27 PM John Garry <[email protected]> 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