Re: [PATCH 3/6] ptp: qcom: Add PTP driver for the Qualcomm TSC hardware

[Krzysztof Kozlowski <krzk@kernel.org>]

On 27/07/2026 16:10, Imran Shaik wrote:
> Add a PTP Hardware Clock driver for the Qualcomm Timestamp Counter (TSC)
> hardware found on Qualcomm Lemans and QDU1000 SoCs. The TSC is a free
> running hardware counter used for time synchronization, clocked by an AHB
> configuration clock and a global counter clock, with the counter resolution
> varying across SoCs.
> 
> Co-developed-by: Taniya Das <taniya.das@oss.qualcomm.com>
> Signed-off-by: Taniya Das <taniya.das@oss.qualcomm.com>
> Signed-off-by: Imran Shaik <imran.shaik@oss.qualcomm.com>
> ---
>  drivers/ptp/Kconfig        |  10 ++
>  drivers/ptp/Makefile       |   1 +
>  drivers/ptp/ptp_qcom_tsc.c | 413 +++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 424 insertions(+)
> 
> diff --git a/drivers/ptp/Kconfig b/drivers/ptp/Kconfig
> index b93640ca08b7286fd4298519a17d1daf19056498..d3ceb1ee01a505cdd0bd056649d245308fc9f140 100644
> --- a/drivers/ptp/Kconfig
> +++ b/drivers/ptp/Kconfig
> @@ -263,4 +263,14 @@ config PTP_NETC_V4_TIMER
>  	  synchronization. It also supports periodic output signal (e.g. PPS)
>  	  and external trigger timestamping.
>  
> +config PTP_QCOM_TSC
> +	tristate "Qualcomm TSC as PTP clock"

Missing depensd on ARCH_QCOM

> +	depends on COMMON_CLK && PTP_1588_CLOCK
> +	help
> +	  This driver adds support for using the Qualcomm Timestamp Counter
> +	  Subsystem (TSCSS) as a PTP clock.
> +
> +	  To compile this driver as a module, choose M here: the module
> +	  will be called ptp_qcom_tsc.
> +
>  endmenu

...

> +
> +static const struct of_device_id qcom_tsc_of_match[] = {
> +	{ .compatible = "qcom,lemans-tscss", .data = &qcom_tsc_lemans },
> +	{ .compatible = "qcom,qdu1000-tscss", .data = &qcom_tsc_qdu1000 },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, qcom_tsc_of_match);
> +
> +static void qcom_ptp_tsc_remove(struct platform_device *pdev)

Completely broken order of ID table, remove() and probe(). Remove ALWAYS
follows probe. Table is next to them. Please look at other drivers.

> +{
> +	struct qcom_tsc *tsc = platform_get_drvdata(pdev);
> +
> +	if (tsc && tsc->ptp) {
> +		ptp_clock_unregister(tsc->ptp);
> +		tsc->ptp = NULL;
> +	}
> +}
> +
> +static int qcom_ptp_tsc_probe(struct platform_device *pdev)
> +{
> +	const struct qcom_tsc_soc_data *soc;
> +	struct qcom_tsc *tsc;
> +	struct resource *r_mem;
> +	int ret;
> +
> +	soc = of_device_get_match_data(&pdev->dev);
> +	if (!soc)
> +		return -ENODEV;
> +
> +	tsc = devm_kzalloc(&pdev->dev, sizeof(*tsc), GFP_KERNEL);
> +	if (!tsc)
> +		return -ENOMEM;
> +
> +	tsc->dev = &pdev->dev;
> +	tsc->soc = soc;
> +
> +	ret = devm_mutex_init(&pdev->dev, &tsc->lock);
> +	if (ret)
> +		return ret;
> +
> +	r_mem = platform_get_resource_byname(pdev, IORESOURCE_MEM, "tsc");
> +	if (!r_mem) {
> +		dev_err(&pdev->dev, "no IO resource defined\n");
> +		return -ENXIO;
> +	}
> +
> +	tsc->base = devm_ioremap_resource(&pdev->dev, r_mem);
> +	if (IS_ERR(tsc->base))
> +		return PTR_ERR(tsc->base);
> +
> +	tsc->ahb_clk = devm_clk_get_enabled(&pdev->dev, "ahb");
> +	if (IS_ERR(tsc->ahb_clk))
> +		return PTR_ERR(tsc->ahb_clk);
> +
> +	tsc->cntr_clk = devm_clk_get_enabled(&pdev->dev, "cntr");
> +	if (IS_ERR(tsc->cntr_clk))
> +		return PTR_ERR(tsc->cntr_clk);
> +
> +	/* ETU clock is optional, only required when the ETU block is used */
> +	tsc->etu_clk = devm_clk_get_optional_enabled(&pdev->dev, "etu");
> +	if (IS_ERR(tsc->etu_clk))
> +		return PTR_ERR(tsc->etu_clk);
> +
> +	tsc->ptp_info = qcom_ptp_info;
> +
> +	tsc->ptp = ptp_clock_register(&tsc->ptp_info, &pdev->dev);
> +	if (IS_ERR(tsc->ptp)) {
> +		ret = PTR_ERR(tsc->ptp);
> +		dev_err(&pdev->dev, "Failed to register ptp clock\n");
> +		return ret;

Why aren't you using dev_err_probe?

> +	}
> +
> +	platform_set_drvdata(pdev, tsc);
> +
> +	return 0;
> +}
> +
> +static struct platform_driver qcom_ptp_tsc_driver = {
> +	.probe  = qcom_ptp_tsc_probe,
> +	.remove = qcom_ptp_tsc_remove,
> +	.driver = {
> +		.name = "qcom_ptp_tsc",
> +		.of_match_table = qcom_tsc_of_match,
> +	},
> +};
> +module_platform_driver(qcom_ptp_tsc_driver);
> +
> +MODULE_DESCRIPTION("PTP QCOM TSC driver");
> +MODULE_LICENSE("GPL");
> 


Best regards,
Krzysztof