Re: [PATCH 1/6] soc/tegra: fuse: Add tegra_acpi_init_apbmisc()

[Kartik <kkartik@nvidia.com>]

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