Re: [PATCH 5/6] soc/tegra: fuse: Add ACPI support for Tegra194 and Tegra234

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Fri, Aug 18, 2023 at 03:00:27PM +0530, Kartik wrote:
> Add tegra_fuse_acpi_probe() to initialize Tegra fuse while using ACPI.
> Also, drop '__init' keyword for tegra_soc_device_register() as this is also
> used by tegra_fuse_acpi_probe().
> 
> Note that as ACPI subsystem initialize at subsys init, function
> tegra_fuse_acpi_probe() also contains the necessary initialization
> that we are currently doing for device-tree boot as a part of
> early init.

...

> +#include <linux/acpi.h>

You meed mod_devicetable.h and possibly property.h, not this header
(see below).

...

> +static const struct acpi_device_id tegra_fuse_acpi_match[] = {
> +	{
> +		.id = "NVDA200F",
> +	},

Single line, no inner comma.

> +	{ /* sentinel */ },

The idea of sentinel is to guard, the trailing comma ruins this contract.

> +};

...

> +static int tegra_fuse_acpi_probe(struct platform_device *pdev)
> +{

Why you need a separate function?

> +	struct resource *res;
> +	u8 chip;
> +	int err;
> +
> +	tegra_acpi_init_apbmisc();
> +
> +	chip = tegra_get_chip_id();
> +	switch (chip) {
> +#if defined(CONFIG_ARCH_TEGRA_194_SOC)

Can we avoid ugly ifdeffery?

> +	case TEGRA194:
> +		fuse->soc = &tegra194_fuse_soc;
> +		break;
> +#endif
> +#if defined(CONFIG_ARCH_TEGRA_234_SOC)

Ditto.

> +	case TEGRA234:
> +		fuse->soc = &tegra234_fuse_soc;
> +		break;
> +#endif
> +	default:
> +		dev_err(&pdev->dev, "Unsupported SoC: %02x\n", chip);
> +		return -EINVAL;

		return dev_err_probe(...);

> +	}
> +
> +	fuse->base = devm_platform_get_and_ioremap_resource(pdev, 0, &res);
> +	if (IS_ERR(fuse->base))
> +		return PTR_ERR(fuse->base);
> +	fuse->phys = res->start;

> +	platform_set_drvdata(pdev, fuse);

Is it being used?

> +	fuse->dev = &pdev->dev;
> +
> +	err = tegra_fuse_nvmem_register(fuse, &pdev->dev);
> +	if (err)
> +		return err;
> +
> +	fuse->soc->init(fuse);
> +	tegra_soc_device_register();
> +	tegra_fuse_pr_sku_info(&tegra_sku_info);
> +
> +	err = tegra_fuse_add_lookups(fuse);
> +	if (err) {

> +		dev_err(&pdev->dev, "failed to add FUSE lookups\n");
> +		return err;

		return dev_err_probe(...);

> +	}
> +
> +	return 0;
> +}

...

> +	if (has_acpi_companion(&pdev->dev))
> +		return tegra_fuse_acpi_probe(pdev);

Why is the ACPI so special here? Why you can't go same flow?

...

> +	/* fuse->clk is not required when ACPI is used. */
> +	if (!fuse->read || (!fuse->clk && !has_acpi_companion(fuse->dev)))

No, just make CLK optional and that's it.

>  		return -EPROBE_DEFER;

-- 
With Best Regards,
Andy Shevchenko