Received-SPF: pass (google.com: domain of linux-kernel+bounces-2297-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Date: Sat, 16 Dec 2023 17:42:33 +0000
From: Simon Horman <horms@kernel.org>
To: Min Li <lnimi@hotmail.com>
Cc: richardcochran@gmail.com, lee@kernel.org, linux-kernel@vger.kernel.org,
	netdev@vger.kernel.org, Min Li <min.li.xe@renesas.com>
Subject: Re: [PATCH net-next v3 2/2] ptp: add FemtoClock3 Wireless as ptp
 hardware clock
Message-ID: <20231216174233.GA48471@kernel.org>
References: <20231214163625.17939-1-lnimi@hotmail.com>
 <PH7PR03MB7064D4543F1EE6E0A45053C8A08CA@PH7PR03MB7064.namprd03.prod.outlook.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <PH7PR03MB7064D4543F1EE6E0A45053C8A08CA@PH7PR03MB7064.namprd03.prod.outlook.com>

On Thu, Dec 14, 2023 at 11:36:25AM -0500, Min Li wrote:

...

> diff --git a/drivers/ptp/ptp_fc3.c b/drivers/ptp/ptp_fc3.c

...

> +static inline s64 ns2counters(struct idtfc3 *idtfc3, s64 nsec, u32 *sub_ns)

Sorry, I missed this in my earlier response.

Please don't use inline in .c files unless htere is a demonstrable
reason to do so. Rather, please leave inlining up to the compiler.

> +{
> +	s64 sync;
> +	s32 rem;
> +
> +	if (likely(nsec >= 0)) {
> +		sync = div_u64_rem(nsec, idtfc3->ns_per_sync, &rem);
> +		*sub_ns = rem;
> +	} else if (nsec < 0) {
> +		sync = -div_u64_rem(-nsec - 1, idtfc3->ns_per_sync, &rem) - 1;
> +		*sub_ns = idtfc3->ns_per_sync - rem - 1;
> +	}
> +
> +	return sync * idtfc3->ns_per_sync;
> +}
> +
> +static inline s64 tdc_meas2offset(struct idtfc3 *idtfc3, u64 meas_read)
> +{
> +	s64 coarse, fine;
> +
> +	fine = sign_extend64(FIELD_GET(FINE_MEAS_MASK, meas_read), 12);
> +	coarse = sign_extend64(FIELD_GET(COARSE_MEAS_MASK, meas_read), (39 - 13));
> +
> +	fine = div64_s64(fine * NSEC_PER_SEC, idtfc3->tdc_apll_freq * 62LL);
> +	coarse = div64_s64(coarse * NSEC_PER_SEC, idtfc3->time_ref_freq);
> +
> +	return coarse + fine;
> +}
> +
> +static inline s64 tdc_offset2phase(struct idtfc3 *idtfc3, s64 offset_ns)
> +{
> +	if (offset_ns > idtfc3->ns_per_sync / 2)
> +		offset_ns -= idtfc3->ns_per_sync;
> +
> +	return offset_ns * idtfc3->tdc_offset_sign;
> +}

...

> +static inline bool get_tdc_meas(struct idtfc3 *idtfc3, s64 *offset_ns)
> +{
> +	bool valid = false;
> +	u8 buf[9];
> +	u8 val;
> +	int err;
> +
> +	while (true) {
> +		err = regmap_bulk_read(idtfc3->regmap, TDC_FIFO_STS,
> +				       &val, sizeof(val));
> +		if (err)
> +			return false;
> +
> +		if (val & FIFO_EMPTY)
> +			break;
> +
> +		err = regmap_bulk_read(idtfc3->regmap, TDC_FIFO_READ_REQ,
> +				       &buf, sizeof(buf));
> +		if (err)
> +			return false;
> +
> +		valid = true;
> +	}
> +
> +	if (valid)
> +		*offset_ns = tdc_meas2offset(idtfc3, get_unaligned_le64(&buf[1]));
> +
> +	return valid;
> +}
> +
> +static inline int check_tdc_fifo_overrun(struct idtfc3 *idtfc3)
> +{
> +	u8 val;
> +	int err;
> +
> +	/* Check if FIFO is overrun */
> +	err = regmap_bulk_read(idtfc3->regmap, TDC_FIFO_STS, &val, sizeof(val));
> +	if (err)
> +		return err;
> +
> +	if (!(val & FIFO_FULL))
> +		return 0;
> +
> +	dev_warn(idtfc3->dev, "TDC FIFO overrun !!!");
> +
> +	err = idtfc3_enable_tdc(idtfc3, true, CONTINUOUS);
> +	if (err)
> +		return err;
> +
> +	return 0;
> +}

...

> +static int idtfc3_remove(struct platform_device *pdev)
> +{
> +	struct idtfc3 *idtfc3 = platform_get_drvdata(pdev);
> +
> +	ptp_clock_unregister(idtfc3->ptp_clock);

FWIIW, I'm slightly surprised that more cleanup isn't needed.

> +
> +	return 0;
> +}
> +
> +static struct platform_driver idtfc3_driver = {
> +	.driver = {
> +		.name = "rc38xxx-phc",
> +	},
> +	.probe = idtfc3_probe,
> +	.remove	= idtfc3_remove,
> +};
> +
> +module_platform_driver(idtfc3_driver);

...