On Fri, 2023-08-18 at 18:07 +0300, Andy Shevchenko wrote:
>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.
ACK.
>
>> + { /* sentinel */ },
>
>The idea of sentinel is to guard, the trailing comma ruins this contract.
>
>> +};
ACK.
>
>...
>
>> +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?
>
No, the SoC data is defined only when the SoC specific config is
enabled. So, guarding these with ifdef's is required here.
>> + 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(...);
>
ACK.
>> + }
>> +
>> + 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?
>
Looks like this is not 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?
>
>...
>
We need to initialize the soc data before we continue the probe.
I guess we can have a conditional here for this initialization.
and re-use the same function. I will update this in the next patch.
>> + /* 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;
>
>--
If the Fuse driver is probed using device-tree. Then we need to make
sure that fuse->clk has been initilaized.
>With Best Regards,
>Andy Shevchenko
Regards,
Kartik
