Thanks for reviewing the patches Andy!
On Fri, 2023-08-18 at 16:21 +0300, Andy Shevchenko wrote:
>> void tegra_init_revision(void);
>> void tegra_init_apbmisc(void);
>> +void tegra_acpi_init_apbmisc(void);
>
>Why do you need a separate function?
Function tegra_init_apbmisc() is called from tegra_init_fuse() which
is invoked at early init and it also has `__init` keyword. If we use
the same function for both ACPI/DT, then we will get init section
mismatches when the Tegra Fuse driver probes using ACPI.
We can use the same function by dropping the `init` keyword. But
the way we are getting the resources for device-tree and on ACPI is
slightly different. Hence, I kept a separate function for ACPI
and move the common bits to a function shared between
tegra_init_apbmisc() and tegra_acpi_init_apbmisc().
>
>
>> +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.
>
I will update this in the next patch.
...
>> + 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.
>
>...
>
Sure, I will update these in the next patch.
...
>> - 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.
ACK.
...
>> +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.
I will fix this logic in next patch.
>
>> + 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).
Yes, these won't be called when rcount is `1`. I will update this
in the next patch set.
> +}
Regards,
Kartik
