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
