[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20230821113220.4255-1-kkartik@nvidia.com>
Date: Mon, 21 Aug 2023 17:02:20 +0530
From: Kartik <kkartik@...dia.com>
To: <andriy.shevchenko@...ux.intel.com>
CC: <arnd@...db.de>, <digetx@...il.com>, <frank.li@...o.com>,
<jonathanh@...dia.com>, <kkartik@...dia.com>,
<linux-kernel@...r.kernel.org>, <linux-tegra@...r.kernel.org>,
<pdeschrijver@...dia.com>, <petlozup@...dia.com>,
<pshete@...dia.com>, <robh@...nel.org>, <stefank@...dia.com>,
<sumitg@...dia.com>, <thierry.reding@...il.com>, <windhl@....com>
Subject: Re: [PATCH 1/6] soc/tegra: fuse: Add tegra_acpi_init_apbmisc()
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
Powered by blists - more mailing lists