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

[Kartik]

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