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

[Andy Shevchenko]

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