On Fri, Aug 18, 2023 at 03:00:23PM +0530, Kartik wrote:
> In preparation to ACPI support in Tegra fuse driver add function
> tegra_acpi_init_apbmisc() and move common code used for both ACPI and
> device-tree into a new helper function tegra_init_apbmisc_base().
>
> Note that function tegra_acpi_init_apbmisc() is not placed in the __init
> section, because it will be called during probe.
...
> void tegra_init_revision(void);
> void tegra_init_apbmisc(void);
> +void tegra_acpi_init_apbmisc(void);
Why do you need a separate function?
...
> +static const struct acpi_device_id apbmisc_acpi_match[] = {
> + { .id = "NVDA2010", 0 },
We rarely use C99 initializers for ACPI ID table.
Also 0 is not needed.
> + { /* sentinel */ }
> +};
...
> + if (!apbmisc_base)
> + pr_err("failed to map APBMISC registers\n");
> + else
> + chipid = readl_relaxed(apbmisc_base + 4);
Why not positive conditional as you have two branches anyway?
...
> + if (!strapping_base) {
> + pr_err("failed to map strapping options registers\n");
> + } else {
> + strapping = readl_relaxed(strapping_base);
> + iounmap(strapping_base);
> + }
Ditto.
...
> - apbmisc_base = ioremap(apbmisc.start, resource_size(&apbmisc));
> - if (!apbmisc_base) {
> - pr_err("failed to map APBMISC registers\n");
> - } else {
> - chipid = readl_relaxed(apbmisc_base + 4);
> - }
> -
> - strapping_base = ioremap(straps.start, resource_size(&straps));
> - if (!strapping_base) {
> - pr_err("failed to map strapping options registers\n");
> - } else {
> - strapping = readl_relaxed(strapping_base);
> - iounmap(strapping_base);
> - }
> -
> + tegra_init_apbmisc_base(&apbmisc, &straps);
This split can be done in a separate precursor patch.
...
> +void tegra_acpi_init_apbmisc(void)
> +{
> + struct acpi_device *adev = NULL;
> + struct resource *apbmisc, *straps;
> + struct list_head resource_list;
> + struct resource_entry *rentry;
> + int rcount;
> +
> + adev = acpi_dev_get_first_match_dev(apbmisc_acpi_match[0].id, NULL, -1);
> + if (!adev)
> + return;
> +
> + INIT_LIST_HEAD(&resource_list);
> +
> + rcount = acpi_dev_get_memory_resources(adev, &resource_list);
> + if (rcount != 2) {
> + pr_err("failed to get APBMISC memory resources");
(1)
> + return;
> + }
> + rentry = list_first_entry(&resource_list, struct resource_entry, node);
> + apbmisc = rentry->res;
> + rentry = list_next_entry(rentry, node);
> + straps = rentry->res;
We don't do like this, we walk through them and depending on the type and index
do something. The above if error prone and not scalable code.
> + tegra_init_apbmisc_base(apbmisc, straps);
> + acpi_dev_free_resource_list(&resource_list);
Not okay in (1).
> + acpi_dev_put(adev);
Not okay in (1).
> +}
--
With Best Regards,
Andy Shevchenko
