Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Received-SPF: Pass (protection.outlook.com: domain of nvidia.com designates
 216.228.118.232 as permitted sender) receiver=protection.outlook.com;
 client-ip=216.228.118.232; helo=mail.nvidia.com; pr=C
From:   Kartik <kkartik@nvidia.com>
To:     <andriy.shevchenko@linux.intel.com>
CC:     <arnd@arndb.de>, <digetx@gmail.com>, <frank.li@vivo.com>,
        <jonathanh@nvidia.com>, <kkartik@nvidia.com>,
        <linux-kernel@vger.kernel.org>, <linux-tegra@vger.kernel.org>,
        <pdeschrijver@nvidia.com>, <petlozup@nvidia.com>,
        <pshete@nvidia.com>, <robh@kernel.org>, <stefank@nvidia.com>,
        <sumitg@nvidia.com>, <thierry.reding@gmail.com>, <windhl@126.com>
Subject: Re: [PATCH 5/6] soc/tegra: fuse: Add ACPI support for Tegra194 and Tegra234
Date:   Mon, 21 Aug 2023 17:08:19 +0530
Message-ID: <20230821113819.4400-1-kkartik@nvidia.com>
In-Reply-To: <ZN+JSrT1nH/XMnGu@smile.fi.intel.com>
References: <ZN+JSrT1nH/XMnGu@smile.fi.intel.com>
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Content-Type: text/plain
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 21 Aug 2023 11:38:42.5438
 (UTC)
X-MS-Exchange-CrossTenant-Network-Message-Id: c67aa2e4-434b-46a1-d60d-08dba23b2be6
X-MS-Exchange-CrossTenant-Id: 43083d15-7273-40c1-b7db-39efd9ccc17a
X-MS-Exchange-CrossTenant-OriginalAttributedTenantConnectingIp: TenantId=43083d15-7273-40c1-b7db-39efd9ccc17a;Ip=[216.228.118.232];Helo=[mail.nvidia.com]
X-MS-Exchange-CrossTenant-AuthSource: CO1PEPF000042A8.namprd03.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Anonymous
X-MS-Exchange-CrossTenant-FromEntityHeader: HybridOnPrem
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM4PR12MB6614
X-Spam-Status: No, score=1.4 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FORGED_SPF_HELO,
        RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,SORTED_RECIPS,SPF_HELO_PASS,
        SPF_NONE autolearn=no autolearn_force=no version=3.4.6
X-Spam-Level: *
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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